Closed Bug 663871 Opened 9 years ago Closed 9 years ago

Websockets Protocol IETF 08

Categories

(Core :: Networking: WebSockets, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla7
Tracking Status
firefox6 - ---

People

(Reporter: mcmanus, Assigned: mcmanus)

References

Details

(Keywords: dev-doc-complete)

Attachments

(4 files, 4 obsolete files)

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.
Attached patch utf8 replacement char v1 (obsolete) — Splinter Review
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.
Assignee: nobody → mcmanus
Status: NEW → ASSIGNED
Attachment #538942 - Flags: review?(benjamin)
Attached patch unknown opcode v1 (obsolete) — Splinter Review
websockets -08 sec 4.2 now requires frames with unknown opcodes to be ignored .. we were previously throwing errors.
Attachment #538945 - Flags: review?(cbiesinger)
Attached patch rsvd bits v1 (obsolete) — Splinter Review
websockets -08 sec 4.2 requires frames using rsvd bits to be ignored, previous we were throwing errors.
Attachment #538946 - Flags: review?(cbiesinger)
Attached patch close codes v1Splinter Review
websockets -08 sec 7.4.1 defines two new close codes
Attachment #538947 - Flags: review?(cbiesinger)
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.
Attachment #538942 - Flags: review?(benjamin) → review-
(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?
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.
Attachment #538952 - Flags: review?(cbiesinger)
update our version string and the version string in the server to -08.
Attachment #538953 - Flags: review?(cbiesinger)
Attachment #538945 - Flags: review?(cbiesinger) → review+
Attachment #538946 - Flags: review?(cbiesinger) → review+
Attachment #538947 - Flags: review?(cbiesinger) → review+
Comment on attachment 538953 [details] [diff] [review]
update version number v1

your server changes should also get an entry in the README
Attachment #538953 - Flags: review?(cbiesinger) → review+
Attachment #538952 - Flags: review?(cbiesinger) → review+
Attached patch utf8 replacement char v2 (obsolete) — Splinter Review
updated patch now with horrifically ugly tests due to internal symbol limitation in tests.. but tests nonetheless.
Attachment #538942 - Attachment is obsolete: true
Attachment #539329 - Flags: review?(benjamin)
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.
I agree - we should fail on protocol violations of opcode, rsvd bits and utf-8.
Attachment #538945 - Attachment is obsolete: true
Attachment #538946 - Attachment is obsolete: true
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.
Attachment #539329 - Attachment is obsolete: true
Attachment #539329 - Flags: review?(benjamin)
sec-review decided that the right way to handle receipt of a non-conformant utf-8 message is to fail the connection.
Attachment #539832 - Flags: review?(cbiesinger)
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
Attachment #539832 - Flags: review?(cbiesinger) → review+
(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.
Whiteboard: [inbound]
http://hg.mozilla.org/mozilla-central/rev/680da2e79681
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla7
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.
(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.
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.
(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.).
Wow, I hate the IETF. :D

I will make the needed corrections. Thanks!
Updated.
Blocks: 783133
You need to log in before you can comment on or make changes to this bug.