ietf
[Top] [All Lists]

Gen-ART last call review of draft-ietf-hybi-thewebsocketprotocol-10

2011-07-19 22:03:27
I am the assigned Gen-ART reviewer for this draft. For background on 
Gen-ART, please see the FAQ at 
<http://wiki.tools.ietf.org/area/gen/trac/wiki/GenArtfaq>.

Please resolve these comments along with any other Last Call comments 
you may receive.

Document: draft-ietf-hybi-thewebsocketprotocol-10.txt
Reviewer: Richard Barnes
Review Date: 19 July 2011
IETF LC End Date: 25 July 2011
IESG Telechat date: (if known) -

Summary:
Not ready.  

Major issues:

[Is GET/Upgrade appropriate?]
It seems like the use of GET here goes beyond the normal "safe" semantics for 
that method.  To quote RFC 2616:
"
   In particular, the convention has been established that the GET and
   HEAD methods SHOULD NOT have the significance of taking an action
   other than retrieval.
"
In addition, alternative protocols specified in Upgrade headers are generally 
optional, whereas in this case, the transaction fails overall if the server 
does not support the websocket protocol.  It doesn't look to me like any of the 
current methods provide much better semantics (except possiblyCONNECT or the 
use of OPTIONS as in RFC 2817), and it seems like defining a new method could 
help get around the Upgrade issue as well.

[Huge buffers]
The frame length can be 7, 16, or 64 bits long.  Since the client is expected 
to buffer data until the end of a frame, this is asking clients to buffer 128 
B, 64 KB, or 16 EB.  If it were 32 bits, the max would be 4 MB.  Why not just 
make this a 32-bit fixed length field?

[Why is masking necessary?]
I seriously question the necessity of the masking of data frames.  As I 
understand it, the goal is to prevent proxies that don't understand Upgrade 
from confusing WebSocket data with HTTP data.  This risk seems a little dubious 
to me; has such a poisoning attack been demonstrated?  It seems like there are 
much simpler ways of doing this, like using a method other than GET (either 
CONNECT or something new).   

[Why only client-to-server masking?]
Why isn't masking required on server-to-client frames?

[Unlimited buffering with fragmentation]
Much like with the frame length issue above, the fragmentation mechanism here 
seems like it imposes a heavy burden on the receive side.  Since the receiving 
client is supposed to buffer data until the end of a frame, it seems like 
fragmentation could be used to cause a receiving client to buffer a frame of 
indefinite size. 


Minor issues:

[Setting protocol]
In the NOTE in Section 1.3, the document observes that Sec- headers can't be 
set in XHRs.  But clearly the Sec-WebSocket-Protocol header can be set through 
some API that applications use to get to this protocol.  So it seems a little 
misleading to imply that scripts can't set these headers (besides -Version and 
-Origin).

[Use existing Origin header]
Why is this document creating a separate Sec-WebSocket-Origin header, rather 
than using the Origin header from draft-ietf-websec-origin?   

[Need a reference to CORS]
It would be helpful to have a reference to Cross-Origin Resource Sharing in the 
NOTE in this section, since the pre-request check that is described is exactly 
a CORS priming request.

[Key is over-engineered]
What is the need for the hash and GUID in the handshake processing of the key?  
It seems like it would be sufficient for the server to do anything 
deterministic with the key, even something as simple as incrementing it or 
flipping the bits.



Nits / Editorial comments:

[Why not plain sockets?]
The introduction makes clear why this protocol is needed instead of HTTP, but 
not why this protocol improves over providing a plain socket interface.  
Presumably this is because the HTTP header provides a space where the browser 
can inject trusted information?  

[Put the handshake section first]
It would really increase readability to put all the negotiation/handshake parts 
up front, before you get to the data format (Section 4).  As it is, when I got 
to the extension data part, I didn't realize that there was an extension 
mechanism, since it hadn't been mentioned yet.

[Stray sections]
It seems like Section 8 (Error Handling) should be moved up to Section 4.6, and 
Section 9 (Extensions) should be part of section 4.  

_______________________________________________
Ietf mailing list
Ietf(_at_)ietf(_dot_)org
https://www.ietf.org/mailman/listinfo/ietf

<Prev in Thread] Current Thread [Next in Thread>