|
|
Created:
3 years, 11 months ago by baranovich Modified:
3 years, 11 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, Ryan Hamilton Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAllow proxying plaintext websockets over http/2 proxy
Both secure and insecure websockets are proxied using HTTP CONNECT
tunnels. But if secure proxy supports HTTP/2 plaintext websockets won't
work (ERR_NOT_IMPLEMENTED will be returned).
To initiate standard websocket handshake over established tunnel, in
HttpStreamFactoryImpl::Job::DoCreateStream we should follow
if (!using_spdy_) { ... } code branch, otherwise we end up in
SetSpdyHttpStreamOrBidirectionalStreamImpl which returns
ERR_NOT_IMPLEMENTED for websocket requests.
For wss: scheme, everything works, since ProxyClientSocket (which is
HttpProxyClientSocketWrapper) is wrapped with SSLClientSocket
establishing TLS connection over the tunnel,
HttpStreamFactoryImpl::Job::DoInitConnectionComplete works as
expected. But for ws: scheme, |using_ssl_| is false,
|proxy_info_.is_https()| is true and we get into the branch where
|using_spdy_| is set to true (which is fine for non-websocket
requests).
This change adds very cumbersome condition allowing plaintext
websockets requests over HTTP/2 proxies follow the same code path
secure websockets do.
BUG=684681
TEST=HttpNetworkTransactionTest.PlaintextWebsocketOverSpdyProxy
R=mmenke@chromium.org,ricea@chromium.org
Review-Url: https://codereview.chromium.org/2642723008
Cr-Commit-Position: refs/heads/master@{#446020}
Committed: https://chromium.googlesource.com/chromium/src/+/a027199ed55462ac711f04cc5c23eff59678beb1
Patch Set 1 #
Total comments: 2
Patch Set 2 : Update comments #Patch Set 3 : Fix review nits #
Total comments: 4
Patch Set 4 : Fix codestyle #Messages
Total messages: 22 (9 generated)
Description was changed from ========== Allow proxying plaintext websockets over http/2 proxy Both secure and insecure websockets are proxied using HTTP CONNECT tunnels. But if secure proxy supports HTTP/2 plaintext websockets won't work (ERR_NOT_IMPLEMENTED will be returned). To initiate standard websocket handshake over established tunnel, in HttpStreamFactoryImpl::Job::DoCreateStream we should follow if (!using_spdy_) { ... } code branch, otherwise we end up in SetSpdyHttpStreamOrBidirectionalStreamImpl which returns ERR_NOT_IMPLEMENTED for websocket requests. For wss: scheme, everything works, since ProxyClientSocket (which is HttpProxyClientSocketWrapper) is wrapped with SSLClientSocket establishing TLS connection over the tunnel, HttpStreamFactoryImpl::Job::DoInitConnectionComplete works as expected. But for ws: scheme, |using_ssl_| is false, |proxy_info_.is_https()| is true and we get into the branch where |using_spdy_| is set to true (which is fine for non-websocket requests). This change adds very cumbersome condition allowing plaintext websockets requests over HTTP/2 proxies follow the same code path secure websockets do. BUG= TEST=HttpNetworkTransactionTest.PlaintextWebsocketOverSpdyProxy R=mmenke@chromium.org,ricea@chromium.org ========== to ========== Allow proxying plaintext websockets over http/2 proxy Both secure and insecure websockets are proxied using HTTP CONNECT tunnels. But if secure proxy supports HTTP/2 plaintext websockets won't work (ERR_NOT_IMPLEMENTED will be returned). To initiate standard websocket handshake over established tunnel, in HttpStreamFactoryImpl::Job::DoCreateStream we should follow if (!using_spdy_) { ... } code branch, otherwise we end up in SetSpdyHttpStreamOrBidirectionalStreamImpl which returns ERR_NOT_IMPLEMENTED for websocket requests. For wss: scheme, everything works, since ProxyClientSocket (which is HttpProxyClientSocketWrapper) is wrapped with SSLClientSocket establishing TLS connection over the tunnel, HttpStreamFactoryImpl::Job::DoInitConnectionComplete works as expected. But for ws: scheme, |using_ssl_| is false, |proxy_info_.is_https()| is true and we get into the branch where |using_spdy_| is set to true (which is fine for non-websocket requests). This change adds very cumbersome condition allowing plaintext websockets requests over HTTP/2 proxies follow the same code path secure websockets do. BUG= TEST=HttpNetworkTransactionTest.PlaintextWebsocketOverSpdyProxy R=mmenke@chromium.org,ricea@chromium.org ==========
baranovich@yandex-team.ru changed reviewers: + juliatuttle@chromium.org - mmenke@chromium.org
Thanks for this. lgtm with nits. https://codereview.chromium.org/2642723008/diff/1/net/http/http_network_trans... File net/http/http_network_transaction_unittest.cc (right): https://codereview.chromium.org/2642723008/diff/1/net/http/http_network_trans... net/http/http_network_transaction_unittest.cc:16575: SSLSocketDataProvider ssl2(ASYNC, OK); This second SSLSocketDataProvider does not appear to be necessary. https://codereview.chromium.org/2642723008/diff/1/net/http/http_network_trans... net/http/http_network_transaction_unittest.cc:16578: TestCompletionCallback callback1; Extreme nitpick: Since there is only one callback in this test, it can be called just "callback" rather than "callback1".
juliatuttle@, PTAL
baranovich@yandex-team.ru changed reviewers: + mmenke@chromium.org - juliatuttle@chromium.org
Matt, would you plz take a look? git cl suggested Julia as a reviewer, but she is not responding...
mmenke@chromium.org changed reviewers: + bnc@chromium.org
[+bnc]: Mind taking this one? I'm not really familiar with how H2 proxies work.
What is the context here, why do we want to allow insecure websockets over HTTP/2 proxies? How does this relate to our secure-only HTTP/2 policy? What is the use case, are there many sites out there doing plaintext websockets? Also, is there a crbug that this CL can link to?
Description was changed from ========== Allow proxying plaintext websockets over http/2 proxy Both secure and insecure websockets are proxied using HTTP CONNECT tunnels. But if secure proxy supports HTTP/2 plaintext websockets won't work (ERR_NOT_IMPLEMENTED will be returned). To initiate standard websocket handshake over established tunnel, in HttpStreamFactoryImpl::Job::DoCreateStream we should follow if (!using_spdy_) { ... } code branch, otherwise we end up in SetSpdyHttpStreamOrBidirectionalStreamImpl which returns ERR_NOT_IMPLEMENTED for websocket requests. For wss: scheme, everything works, since ProxyClientSocket (which is HttpProxyClientSocketWrapper) is wrapped with SSLClientSocket establishing TLS connection over the tunnel, HttpStreamFactoryImpl::Job::DoInitConnectionComplete works as expected. But for ws: scheme, |using_ssl_| is false, |proxy_info_.is_https()| is true and we get into the branch where |using_spdy_| is set to true (which is fine for non-websocket requests). This change adds very cumbersome condition allowing plaintext websockets requests over HTTP/2 proxies follow the same code path secure websockets do. BUG= TEST=HttpNetworkTransactionTest.PlaintextWebsocketOverSpdyProxy R=mmenke@chromium.org,ricea@chromium.org ========== to ========== Allow proxying plaintext websockets over http/2 proxy Both secure and insecure websockets are proxied using HTTP CONNECT tunnels. But if secure proxy supports HTTP/2 plaintext websockets won't work (ERR_NOT_IMPLEMENTED will be returned). To initiate standard websocket handshake over established tunnel, in HttpStreamFactoryImpl::Job::DoCreateStream we should follow if (!using_spdy_) { ... } code branch, otherwise we end up in SetSpdyHttpStreamOrBidirectionalStreamImpl which returns ERR_NOT_IMPLEMENTED for websocket requests. For wss: scheme, everything works, since ProxyClientSocket (which is HttpProxyClientSocketWrapper) is wrapped with SSLClientSocket establishing TLS connection over the tunnel, HttpStreamFactoryImpl::Job::DoInitConnectionComplete works as expected. But for ws: scheme, |using_ssl_| is false, |proxy_info_.is_https()| is true and we get into the branch where |using_spdy_| is set to true (which is fine for non-websocket requests). This change adds very cumbersome condition allowing plaintext websockets requests over HTTP/2 proxies follow the same code path secure websockets do. BUG=684681 TEST=HttpNetworkTransactionTest.PlaintextWebsocketOverSpdyProxy R=mmenke@chromium.org,ricea@chromium.org ==========
On 2017/01/24 19:28:21, Bence wrote: > What is the context here, why do we want to allow insecure websockets over > HTTP/2 proxies? Well, we just want to:) we would like to proxy them via secure connection through proxy (think of data reduction proxy, but from Yandex). Current behavior is not documented and maybe surprising. Anyone who is setting up for example, corporate system-wide proxy for all traffic with http/2 support (which is quite rare for now, I think) should be aware of this. Another option might be disabling h2 ALPN advertisment for websocket requests but I don't see why it may be better. > How does this relate to our secure-only HTTP/2 policy? I guess is question is more to Matt than to me, but since secure proxies (HTTPS proxy for HTTP traffic) do exist, I see no reason why they should not support plaintext websockets as well. > What is > the use case, are there many sites out there doing plaintext websockets? Not so long ago, tut.by (quite popular in Belorussia) did this, but now they are https. Unfortunately I don't have exact data about plaintext websockets usage in the wild, but afaik they are not deprecated (maybe discouraged). > > Also, is there a crbug that this CL can link to? Filed bug, crbug.com/684681
Thanks for the detailed explanation and for filing the issue. https://codereview.chromium.org/2642723008/diff/40001/net/http/http_network_t... File net/http/http_network_transaction_unittest.cc (right): https://codereview.chromium.org/2642723008/diff/40001/net/http/http_network_t... net/http/http_network_transaction_unittest.cc:16536: NULL, 0, 1, LOWEST, HostPortPair("www.example.org", 80))); Please use nullptr instead of NULL in new code, here and in line 16547. See https://google.github.io/styleguide/cppguide.html#0_and_nullptr/NULL. https://codereview.chromium.org/2642723008/diff/40001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/2642723008/diff/40001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.cc:1171: // While websockets over HTTP/2 are not supported, it is still valid to Please move this comment one line above (or below) so as not to cut into the multi-line condition.
https://codereview.chromium.org/2642723008/diff/40001/net/http/http_network_t... File net/http/http_network_transaction_unittest.cc (right): https://codereview.chromium.org/2642723008/diff/40001/net/http/http_network_t... net/http/http_network_transaction_unittest.cc:16536: NULL, 0, 1, LOWEST, HostPortPair("www.example.org", 80))); On 2017/01/24 22:54:19, Bence wrote: > Please use nullptr instead of NULL in new code, here and in line 16547. See > https://google.github.io/styleguide/cppguide.html#0_and_nullptr/NULL. Done. https://codereview.chromium.org/2642723008/diff/40001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/2642723008/diff/40001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.cc:1171: // While websockets over HTTP/2 are not supported, it is still valid to On 2017/01/24 22:54:19, Bence wrote: > Please move this comment one line above (or below) so as not to cut into the > multi-line condition. Done.
LGTM. CC-ing rch@ for FYI.
The CQ bit was checked by baranovich@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from ricea@chromium.org Link to the patchset: https://codereview.chromium.org/2642723008/#ps60001 (title: "Fix codestyle")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 60001, "attempt_start_ts": 1485350034605400, "parent_rev": "c204064bdde6e7bde09c2b07b895cc64bcb58c13", "commit_rev": "a027199ed55462ac711f04cc5c23eff59678beb1"}
Message was sent while issue was closed.
Description was changed from ========== Allow proxying plaintext websockets over http/2 proxy Both secure and insecure websockets are proxied using HTTP CONNECT tunnels. But if secure proxy supports HTTP/2 plaintext websockets won't work (ERR_NOT_IMPLEMENTED will be returned). To initiate standard websocket handshake over established tunnel, in HttpStreamFactoryImpl::Job::DoCreateStream we should follow if (!using_spdy_) { ... } code branch, otherwise we end up in SetSpdyHttpStreamOrBidirectionalStreamImpl which returns ERR_NOT_IMPLEMENTED for websocket requests. For wss: scheme, everything works, since ProxyClientSocket (which is HttpProxyClientSocketWrapper) is wrapped with SSLClientSocket establishing TLS connection over the tunnel, HttpStreamFactoryImpl::Job::DoInitConnectionComplete works as expected. But for ws: scheme, |using_ssl_| is false, |proxy_info_.is_https()| is true and we get into the branch where |using_spdy_| is set to true (which is fine for non-websocket requests). This change adds very cumbersome condition allowing plaintext websockets requests over HTTP/2 proxies follow the same code path secure websockets do. BUG=684681 TEST=HttpNetworkTransactionTest.PlaintextWebsocketOverSpdyProxy R=mmenke@chromium.org,ricea@chromium.org ========== to ========== Allow proxying plaintext websockets over http/2 proxy Both secure and insecure websockets are proxied using HTTP CONNECT tunnels. But if secure proxy supports HTTP/2 plaintext websockets won't work (ERR_NOT_IMPLEMENTED will be returned). To initiate standard websocket handshake over established tunnel, in HttpStreamFactoryImpl::Job::DoCreateStream we should follow if (!using_spdy_) { ... } code branch, otherwise we end up in SetSpdyHttpStreamOrBidirectionalStreamImpl which returns ERR_NOT_IMPLEMENTED for websocket requests. For wss: scheme, everything works, since ProxyClientSocket (which is HttpProxyClientSocketWrapper) is wrapped with SSLClientSocket establishing TLS connection over the tunnel, HttpStreamFactoryImpl::Job::DoInitConnectionComplete works as expected. But for ws: scheme, |using_ssl_| is false, |proxy_info_.is_https()| is true and we get into the branch where |using_spdy_| is set to true (which is fine for non-websocket requests). This change adds very cumbersome condition allowing plaintext websockets requests over HTTP/2 proxies follow the same code path secure websockets do. BUG=684681 TEST=HttpNetworkTransactionTest.PlaintextWebsocketOverSpdyProxy R=mmenke@chromium.org,ricea@chromium.org Review-Url: https://codereview.chromium.org/2642723008 Cr-Commit-Position: refs/heads/master@{#446020} Committed: https://chromium.googlesource.com/chromium/src/+/a027199ed55462ac711f04cc5c23... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/a027199ed55462ac711f04cc5c23...
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/2669313002/ by ricea@chromium.org. The reason for reverting is: I believe this is causing the crashes in http://crbug.com/685968.. |