713026 - websockets bootstrap via proxy should always CONNECT

Status: RESOLVED FIXED

(Core :: Networking: WebSockets, defect)

Description

[Patrick McManus [:mcmanus]]

This was a misreading of the spec by me.

When bootstrapping websockets on a HTTP GET and using an HTTP proxy that proxy should always have a CONNECT transaction on it to build a tunnel through it. Right now we only do the CONNECT when establishing a SSL tunnel for wss:// but not for plain ws://, instead relying on the proxy to proxy the HTTP GET - which is contrary to 4.1.3 of rfc6455.

"If the SOCKS proxy is explicitly cleared such that only the HTTP proxy is specified, Firefox does not send a CONNECT to that proxy to route the traffic through it. Instead, it simply sends the plain GET with an Upgrade header. This does not match Chrome or IE10 and I believe it also probably violates the spec. As a result of this shortcoming, proxy servers assume that the WebSocket response is normal HTTP traffic and some (e.g. ISA) will mangle that response by removing the “Connection: Upgrade” header, failing the socket connection. If Firefox properly wraps the traffic in a GET, it will work properly."

Comment 1

[Jason Duell]

Attachment: v1: fix CONNECT logic for non-SSL connections

Honza: this can wait for Patrick to get back if you're not comfortable (or too busy) to review.  Note that the code changes are all HTTP, not websockets :)

So this isn't a "misreading of the spec" by Patrick: we've been passing RESOLVE_ALWAYS_TUNNEL for non-SSL websockets.   The problem is that the HTTP layer hasn't been honoring this flag due to it not being consistent about checking for both SSL and RESOLVE_ALWAYS_TUNNEL everywhere it needs to.

The key line in the patch is in nsHttpConnectionInfo::SetOriginServer.  We were only branching on mUsingSSL there, and as a result, we hashed ALL non-ssl websockets into the same nsHttpConnectionInfo as the one for regular (non-CONNECT) HTTP proxied connections.  This results in a race:  if the first non-SSL proxied HTTP connection is a regular HTTP connection, its cxnInfo gets stored in mCT, and all following non-SSL HTTP connections (including web sockets) use its setting (and so CONNECT isn't ever used, and websockets fail).   If the first non-SSL proxied HTTP connection is a web socket (this would be rare in practice, but is possible for a cached page load at startup: I've made it happen in the debugger), then all HTTP connections try to use CONNECT (but with proxy syntax, ie asking for resources with "http://" syntax), and the browser becomes basically unusable!  For the fix it seems to be enough to just use the target host/port instead of the proxies: I don't seem to need to add a special character in the hash (like the 'P' and 'S' we add for proxied/SSL connections).

Besides this fix, I audited all the uses of usingSSL(), and found a few more places that needed to be fixed for non-SSL CONNECTs, including counting them toward the per-host connection limits and not the per-proxy limits, making sure we add/strip proxy headers correctly, and determining whether they can be pipelined.  The rest of the patch is just cleanup to use my new UsingConnect() function where it's simpler to do so.

With this patch the test at

 http://websocketstest.com/

now works with http/https proxying turned on (using squid).  That's as good of a test as I know for this.

Comment 2

[Honza Bambas (:mayhemer)]

I'll definitely won't get to this today.  I'll drop a note when I start reviewing this.

Patrick, when you get to this sooner, then please let you drop a note you've started.

Comment 3

[Patrick McManus [:mcmanus]]

Comment on attachment 632639 [details] [diff] [review]
v1: fix CONNECT logic for non-SSL connections

I'll steal the review here and get it next week. Jason let me know if that's too late.

Comment 4

[Jason Duell]

No rush, as long as we can resolve by the end of the month, so wait till your vacation is done :)

Comment 5

[Patrick McManus [:mcmanus]]

(In reply to Jason Duell (:jduell) from comment #1)
> Created attachment 632639 [details] [diff] [review]
> v1: fix CONNECT logic for non-SSL connections

> The key line in the patch is in nsHttpConnectionInfo::SetOriginServer.  We
> were only branching on mUsingSSL there, and as a result, we hashed ALL
> non-ssl websockets into the same nsHttpConnectionInfo as the one for regular
> (non-CONNECT) HTTP proxied connections.  

nice

Comment 6

[Patrick McManus [:mcmanus]]

Comment on attachment 632639 [details] [diff] [review]
v1: fix CONNECT logic for non-SSL connections

Review of attachment 632639 [details] [diff] [review]:
-----------------------------------------------------------------

I like it!

Comment 7

[Jason Duell]

Comment on attachment 632639 [details] [diff] [review]
v1: fix CONNECT logic for non-SSL connections

https://hg.mozilla.org/integration/mozilla-inbound/rev/a367517095e3

[Approval Request Comment]
Bug caused by (feature/regressing bug #): always broken AFAICT
User impact if declined:  users using HTTP proxies will only be able to use web sockets over SSL.  Non-SSL websockets fail to connect.  The majority of web sites probably use SSL, but there is surely some % that don't.
Testing completed (on m-c, etc.): Manually tested.
Risk to taking this patch (and alternatives if risky):  Fairly low.  A bunch of locations changed, but all using the same idiom (change if(SSL) to if(usingConnect).
String or UUID changes made by this patch: none.

Comment 8

[Alex Keybl [:akeybl]]

(In reply to Jason Duell (:jduell) from comment #7)
> User impact if declined:  users using HTTP proxies will only be able to use
> web sockets over SSL.  Non-SSL websockets fail to connect.  The majority of
> web sites probably use SSL, but there is surely some % that don't.
> Risk to taking this patch (and alternatives if risky):  Fairly low.  A bunch
> of locations changed, but all using the same idiom (change if(SSL) to
> if(usingConnect).

Longstanding bug, little user impact - this can ride the trains.

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a367517095e3