663871 - Websockets Protocol IETF 08

Status: RESOLVED FIXED

(Core :: Networking: WebSockets, defect)

Description

[Patrick McManus [:mcmanus]]

As expected, last week IETF published a websockets protocol -08 with minor changes to -07. IMO this is the one we want to ship.

There are 5 changes (plus the version number bump). In general these are just mandated behaviors for previously undefined errors, a couple new codes for closes conditions (which will be consumed by the eventual API changes if the editor's draft is any indication), and a SHOULD level change to TCP termination.

Comment 1

[Patrick McManus [:mcmanus]]

Attachment: utf8 replacement char v1

websockets -08 sec 8.1 requires UTF8 REPLACEMENT_CHAR substitution invalid portions of received strings that are supposed to be UTF-8 already. I couldn't find code that did this all in UTF-8, so I modeled this on the existing IsUTF8().

Benjamin, I'm taking a guess at a reviewer - feel free to reroute if that's appropos.

Comment 2

[Patrick McManus [:mcmanus]]

Attachment: unknown opcode v1

websockets -08 sec 4.2 now requires frames with unknown opcodes to be ignored .. we were previously throwing errors.

Comment 3

[Patrick McManus [:mcmanus]]

Attachment: rsvd bits v1

websockets -08 sec 4.2 requires frames using rsvd bits to be ignored, previous we were throwing errors.

Comment 4

[Patrick McManus [:mcmanus]]

Attachment: close codes v1

websockets -08 sec 7.4.1 defines two new close codes

Comment 5

[Benjamin Smedberg]

Comment on attachment 538942 [details] [diff] [review]
utf8 replacement char v1

No tests, no review ;-) Obviously a necko peer will need to review the websocket bits.

Comment 6

[Patrick McManus [:mcmanus]]

(In reply to comment #5)
> Comment on attachment 538942 [details] [diff] [review] [review]
> utf8 replacement char v1
> 
> No tests, no review ;-)

you don't need to r-anything it yet, but could you comment on whether or not placing such a function in that spot is conceptually acceptable?

Comment 7

[Patrick McManus [:mcmanus]]

Attachment: delay tcp close v1

This is the most annoying of the new requirements:

"The underlying TCP connection, in most normal cases, SHOULD be closed
first by the server, so that it holds the TIME_WAIT state and not the
client"

It is specified this way because the TIME_WAIT does not prevent the client from reusing recycled port numbers within 2 MSL. So if we are racing through port numbers and servers are sending the FIN quickly, this helps scalability.

OTOH, if we are racing through sessions but servers are slow in sending their FINs, then this means we are keeping the websockethandler state around for a long time (and a lot of them).

This patch limits the wait to a max of 1 full second which accomodates network delay. Of course if the FIN is recvd before that cleanup is done immediately. The delay does not impact the time at which the JS application receives the onclose() event. Furthermore, if we have more than 50 websocket handlers concurrently active we don't honor the SHOULD at all in order to focus on resource reduction immediately.

Comment 8

[Patrick McManus [:mcmanus]]

Attachment: update version number v1

update our version string and the version string in the server to -08.

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 538953 [details] [diff] [review]
update version number v1

your server changes should also get an entry in the README

Comment 10

[Patrick McManus [:mcmanus]]

Attachment: utf8 replacement char v2

updated patch now with horrifically ugly tests due to internal symbol limitation in tests.. but tests nonetheless.

Comment 11

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

As was mentioned in the security review, the silent discarding of unrecognized data (unknown opcode v1, rsvd bits v1, utf8 replacement char v2) seems totally bogus. I think we should speak up about getting these changes undone in the spec, and avoid shipping these changes. We agreed in the secteam meeting that it is OK to ship a stricter-than-than-spec implementation (i.e. without those three patches) of v8 in FF6, assuming no other blocking issues.

Comment 12

[Patrick McManus [:mcmanus]]

I agree - we should fail on protocol violations of opcode, rsvd bits and utf-8.

Comment 13

[Patrick McManus [:mcmanus]]

Comment on attachment 539329 [details] [diff] [review]
utf8 replacement char v2

For now at least we will fail the bad recvd utf-8 case rather than silently rewriting it.

Comment 14

[Patrick McManus [:mcmanus]]

Attachment: enforce utf8 recvrs 1

sec-review decided that the right way to handle receipt of a non-conformant utf-8 message is to fail the connection.

Comment 15

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 539832 [details] [diff] [review]
enforce utf8 recvrs 1

seems like we really should get the spec changed instead of violating it, but r=biesi

Comment 16

[Patrick McManus [:mcmanus]]

(In reply to comment #15)
> Comment on attachment 539832 [details] [diff] [review] [review]
> enforce utf8 recvrs 1
> 
> seems like we really should get the spec changed instead of violating it,

we've sent comments - it might be adjusted going forward.

Comment 17

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/680da2e79681

Comment 18

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/11e697c1d28b
http://hg.mozilla.org/mozilla-central/rev/05ca6815b26d
http://hg.mozilla.org/mozilla-central/rev/ba9d84835cb6

Comment 19

[Asa Dotzler [:asa]]

not going to track this and I think it's probably too late for 6 but we can have that discussion if someone nominates the patch.

Comment 20

[Patrick McManus [:mcmanus]]

(In reply to comment #19)
> not going to track this and I think it's probably too late for 6 but we can
> have that discussion if someone nominates the patch.

given that the JS is prefixed, I'm ok leaving it out of FF6 and having a different protocol version in FF7.

Comment 21

[Eric Shepherd [:sheppy]]

The protocol details aren't documented yet (we're not going to document the protocol itself until things settle down more, maybe once we support IETF-09 or later). However, the main landing page now lists our support for IETF-08 in Firefox 7:

https://developer.mozilla.org/en/WebSockets

This is also mentioned on Firefox 7 for developers.

Comment 22

[Patrick McManus [:mcmanus]]

(In reply to Eric Shepherd [:sheppy] from comment #21)
> The protocol details aren't documented yet (we're not going to document the
> protocol itself until things settle down more, maybe once we support IETF-09
> or later). However, the main landing page now lists our support for IETF-08
> in Firefox 7:
> 
> https://developer.mozilla.org/en/WebSockets
> 
> This is also mentioned on Firefox 7 for developers.

Oh man, I'm going to reply with a pedantic semi-correction. I'm sorry in advance.

Firefox 7 (and 8 and hopefully >8 for that matter) support Version 8 of the IETF websocket protocol as defined by ietf draft 10. (which is to say that the protocol version that draft-10 defines is version 8. That is the same protocol version defined by drafts 08 and 09 because the changes in those drafts did not make the wire protocol incompatible.). Most people are calling this 'draft-10 support' and I wouldn't want them to think we didn't support draft-10, because we do! There will surely be a draft-11 to make more changes in standardese but there is a high likelihood it will still work with protocol version 8... but now I'm trying to predict the future in a bugzilla commment - so I'll stop.

Firefox 6 did indeed implement protocol version 7 (which also corresponded to only draft-07.).

Comment 23

[Eric Shepherd [:sheppy]]

Wow, I hate the IETF. :D

I will make the needed corrections. Thanks!

Comment 24

[Eric Shepherd [:sheppy]]

Updated.