1040930 - problems using websockets over https proxy

Status: RESOLVED FIXED

(Core :: Networking: WebSockets, defect)

Description

[Patrick McManus [:mcmanus]]

note that we will proxy over https but only over h1. websockets requires the no-spdy flag (h2 doesn't implement upgrade to bootstrap it) and that flag is only implemented hop to hop right now in gecko. Theoretically we could tunnel a connect stream carrying h1 over an h2 proxy but we don't do that for ws. (we do sometimes do that, but we never prohibit h2 on it - that's just a result of the negotiation).

the bugs here can be tested with a h1 https proxy such as squid.

Comment 1

[Patrick McManus [:mcmanus]]

https://tbpl.mozilla.org/?tree=Try&rev=23bac7902be8

Comment 2

[Patrick McManus [:mcmanus]]

Attachment: [PATCH 4/4] wss over https proxying

From c7e490aba7c84093b9fb2afb00902737e3513e30 Mon Sep 17 00:00:00 2001
---
 netwerk/protocol/http/TunnelUtils.cpp      | 154 +++++++++++++++++++++++++++++
 netwerk/protocol/http/TunnelUtils.h        |   4 +
 netwerk/protocol/http/nsHttpConnection.cpp |  24 +++--
 3 files changed, 175 insertions(+), 7 deletions(-)

Comment 3

[Patrick McManus [:mcmanus]]

Attachment: [PATCH 3/4] https is a transaction property not a connection property

From d98cd4e34f664995dd8546eefd04dd696f2eb270 Mon Sep 17 00:00:00 2001
 when proxying
---
 netwerk/protocol/http/nsHttpConnection.cpp | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

Comment 4

[Patrick McManus [:mcmanus]]

Attachment: [PATCH 2/4] allow proxy connects if uri host is not yet known

From af7f1374123d65800bb2d8cf904eb090b70ce940 Mon Sep 17 00:00:00 2001
---
 netwerk/base/src/nsSocketTransport2.cpp | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Comment 5

[Patrick McManus [:mcmanus]]

Attachment: [PATCH 1/4] mixed content websockets should be based on scheme

From 557dfd32f86e11efeaf412a552f33ffe5d0c8289 Mon Sep 17 00:00:00 2001
---
 content/base/src/WebSocket.cpp | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

Comment 6

[Patrick McManus [:mcmanus]]

Comment on attachment 8458903 [details] [diff] [review]
[PATCH 1/4] mixed content websockets should be based on scheme

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

the current approach doesn't work when the http:// resource is carried of a TLS session to a proxy.. it mis construes that as a https:// page.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8458903 [details] [diff] [review]
[PATCH 1/4] mixed content websockets should be based on scheme

This doesn't look right either.
Per the current spec "If secure is false but the origin specified by the entry settings object has a scheme component that is itself a secure protocol, e.g. HTTPS, then throw a SecurityError exception and abort these steps."

The API to get entry settings object is in flux atm, so better to ask
bholley how to get the right object.
Perhaps GetWebIDLCallerPrincipal() ?

Comment 8

[Bobby Holley (:bholley)]

(In reply to Olli Pettay [:smaug] from comment #7)
> Comment on attachment 8458903 [details] [diff] [review]
> [PATCH 1/4] mixed content websockets should be based on scheme
> 
> This doesn't look right either.
> Per the current spec "If secure is false but the origin specified by the
> entry settings object has a scheme component that is itself a secure
> protocol, e.g. HTTPS, then throw a SecurityError exception and abort these
> steps."
> 
> The API to get entry settings object is in flux atm, so better to ask
> bholley how to get the right object.

The best we have right now is BrokenGetEntryGlobal. GetEntryGlobal will be implemented once we finish bug 951991.

> Perhaps GetWebIDLCallerPrincipal() ?

No, this is something pretty specific and different.

Comment 9

[Patrick McManus [:mcmanus]]

try for brokengetentryglobal() version https://tbpl.mozilla.org/?tree=Try&rev=8637b70cb6e9

Comment 10

[Patrick McManus [:mcmanus]]

Attachment: part 1/4 mixed content websockets should be based on scheme

a version that uses BrokenGetEntryGlobal() - that's new to me. Is this where we should be going?

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

BrokenGetEntryGlobal() does indeed sound silly, but I believe bholley has plans to make it less
Broken and drop then Broken prefix.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8460710 [details] [diff] [review]
part 1/4 mixed content websockets should be based on scheme

I would nest the 'if's.
Use 2 space for indentation consistently.

Comment 13

[Patrick McManus [:mcmanus]]

Attachment: part 1/4 mixed content websockets should be based on scheme

Comment 14

[Patrick McManus [:mcmanus]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6983eedbe0be
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/62f039a68b94
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1cbf6b2482d8
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/bb1af5ba3a3f

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6983eedbe0be
https://hg.mozilla.org/mozilla-central/rev/62f039a68b94
https://hg.mozilla.org/mozilla-central/rev/1cbf6b2482d8
https://hg.mozilla.org/mozilla-central/rev/bb1af5ba3a3f

Comment 16

[Patrick McManus [:mcmanus]]

Comment on attachment 8461041 [details] [diff] [review]
part 1/4 mixed content websockets should be based on scheme

Approval Request Comment
[Feature/regressing bug #]:378637
[User impact if declined]: inability to use websockets if using https based proxy
[Describe test coverage new/current, TBPL]: all proxy code is hand tested - this has been verified on aurora
[Risks and why]: changes made are 95% scoped to new 378637, so regression risk is small and the patch is straight forward
[String/UUID change made/needed]: none

I'm requesting approval for all 4 parts of this patch with this one request. They were just broken out for the convenience of the reviewer.

Comment 17

[Patrick McManus [:mcmanus]]

gecko 33

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/a0d165f5d2cf
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/c0b72625a706
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/94d1bc81af82
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/117437846f56