Re: [apps-discuss] Review of draft-ietf-hybi-thewebsocketprotocol for apps-review

[Alexey Melnikov <[e-mail]>]

Hi Lisa,
Thank for your review.

Lisa Dusseault wrote:
> I have been selected as the Applications Area Review Team reviewer for 
> this draft (for background on apps-review, please 
> see http://www.apps.ietf.org/content/applications-area-review-team).
> Please resolve these comments along with any other Last Call comments 
> you may receive. Please wait for direction from your document shepherd 
> or AD before posting a new version of the draft.
>
> Document: draft-ietf-hybi-thewebsocketprotocol (reviewed -10)
> Title: The WebSocket protocol
> Reviewer: Lisa Dusseault
> Review Date: July 20, 2011
>
> Readiness Summary: This draft is almost ready for publication on the 
> Standards Track, but it has a few issues that should be fixed or 
> thought about before publication.
>
> Document Summary: The Websocket protocol defines a HTTP handshake to 
> transition from a Web context to a bi-directional connection.  It also 
> defines framing and masking for use in the bi-directional context.  It 
> addresses a number of security concerns involving untrusted 
> application code running in browsers, but assumes that browsers are 
> trusted and servers hosting Websockets are trusted to a greater extent. 
>
> Major Issues
>
> 1.  Masking
>
> It would be good to state what the purpose of masking is.  The 
> sentence is "Frames sent from the client to the server are masked to 
> avoid confusing network intermediaries", but I didn't upon reading 
> this, or looking at the specification text for masking, understand why 
> network intermediaries would be confused.  A citation or further 
> explanation would be good.  I see later there's an explanation in page 
> 50, so a forward reference would also work.
Right. Several people asked about that.
> 2.  Cookie handling
>
> Section 5.1, 
>  The request MAY include headers associated with sending cookies,
>     as defined by the appropriate specifications [RFC6265].  These
>     headers are referred to as _Headers to Send Appropriate
>     Cookies_.
>
>     and
>
>  "Additionally, if any headers in the
>   server's handshake indicate that cookies should be set (as defined by
>   [RFC6265]), these cookies are referred to as _Cookies Set During the
>   Server's Opening Handshake_."
>
>  and 
>
> Section 5.2.1, point 8
>   8.  Optionally, other headers, such as those used to send cookies to
>      a server.  Unknown headers MUST be ignored.
>
> First, a nit:   these important underlined terms are referred to 
> where?  I didn't see anywhere where these terms are used other than 
> where they are defined here.
I don't think they are used in the document, but I am wondering if they 
are used in the W3C's API draft. Ian?
> The more substantive issue: I'm left unclear as to whether cookies are 
> really expected to be used, or how the client might know that it needs 
> to use cookies or else the application will not work.  In many Web 
> sites, the site will not work if cookies are not used by the client, 
> and this is sufficiently rare that it's OK.  Is that OK for a 
> Websockets app?  How will the user know how to fix the problem?  Since 
> Websockets can't as easily reply with a Web page to explain how to 
> enable cookies, it would be good to be more clear on this.
My understanding is that cookies are truly optional. I will update the 
draft to clarify that.
> 3.  Failing a WebSocket Connection
>
> Section 7.1.7 is supposed to describe how to _Fail the WebSocket 
> Connection_.  It explains how the client does so.  However, there's at 
> least one case in section 9.1 where the server receiving a malformed 
> Sec-WebSocket-Extensions header must _Fail the WebSocket Connection_. 
>  How does the server do this?
Good catch. I've added missing text to section 7.1.7
> 4.  Dropping with extreme prejudice
>
> >From the Security Considerations: 
>
> "If at any time a server is faced with data that it does not
> understand, or that violates some criteria by which the server
> determines safety of input, or when the server sees an opening
> handshake that does not correspond to the values the server is
> expecting (e.g. incorrect path or origin), the server SHOULD just
> disconnect.  It is always safe to disconnect."
>
> This seems pretty excessive.  When the server just disconnects, the 
> client can't tell much about what went wrong, and whether an automated 
> retry would be any use at all.  This is bad for deployed use, and even 
> worse for development/debugging.  Yet we're recommending servers be 
> that unhelpful?  Wouldn't it be better to recommend that the server 
> reply with an error response (some HTTP status codes defined here, 
> too) that can help the client diagnose an incorrect origin?  An error 
> like that is often going to be a mis-configuration on one side or the 
> other, rather than an attack.
I agree with you. Here is the latest proposed text:

      <t>If at any time a server is faced with data that it does not 
understand,
      or that violates some criteria by which the server determines 
safety of input,
      or when the server sees an opening handshake that does not 
correspond to
      the values the server is expecting (e.g. incorrect path or origin),
      the server MAY drop the TCP connection. If the invalid data 
received after
      a successful WebSocket handshake, the server SHOULD send a Close 
frame with an
      appropriate status code <xref target='status_codes'/> before
      proceeding to <spanx style='emph'>Close the WebSocket 
Connection</spanx>.
      Use of a Close frame with an appropriate status code can help in 
diagnosing
      the problem.
      If the invalid data is sent during the WebSocket handshake the 
server SHOULD
      return an appropriate HTTP <xref target='RFC2616'/> status code.

Do you think this addresses your concern? I didn't specify any 
particular HTTP error code, because several might be applicable.
> I'd also like to know what the websockets server implementations 
> currently do.
>
>
> Minor Issues
>
> The reservation of different ranges of error codes for use by 
> extensions, libraries and frameworks, and application code, seems to 
> be intended to avoid conflicts between those kinds of use.  I don't 
> think it will work very well.  I don't understand where a library ends 
> and application code begins, even in code I write where I write an 
> error_handler to be called by the library I'm using (which might be a 
> "library" only for convenience sake or might be a library that dozens 
> of other software programs use).  
>
> The border between code and "extensions" is not even very clear; if 
> somebody writes an extension intending to publish an internet-draft 
> but never gets around to it, they may have ended up using the wrong 
> range with the right intentions.
>
> The border between extensions and "this protocol" is not very clear; 
> if I submit a internet-draft with a feature proposed to be a 
> requirement for websockets, I might prefer to use a 1000-1999 range 
> error code but if the feature is relegated to an optional draft it 
> should change to 2000-2999 range. 
>
> This kind of ambiguity leads to arguments at best, unnecessary changes 
> to code (when specs are changed late after such arguments) at worst. 
>  I'm not sure what to recommend; perhaps only one range reserved for 
>  "this protocol and extensions published within the IETF process", and 
> another range for "libraries, frameworks and application code".
I think I've convinced Ian that you are right :-). We will work on 
reducing the number of error code ranges.

Best Regards,
Alexey