New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 4 users

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Bug



Sign in to add a comment
link

Issue 684681: Plaintext websockets does not work via HTTP/2 proxy

Reported by baranov...@yandex-team.ru, Jan 24 2017

Issue description

UserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_2) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/55.0.2883.95 Safari/537.36

Example URL:

Steps to reproduce the problem:
1. Set up secure proxy that supports http/2
2. Visit website with plaintext websockets 

What is the expected behavior?
Plaintext websockets work when using H2 proxy

What went wrong?
ERR_NOT_IMPLEMENTED is returned (logged into console)

Did this work before? No 

Chrome version: 55.0.2883.95  Channel: stable
OS Version: OS X 10.12.2
Flash Version: Shockwave Flash 24.0 r0
 

Comment 1 Deleted

Comment 2 by zhongyi@chromium.org, Jan 24 2017

Labels: Needs-Feedback
Thanks for filing bug.

Are you able to grab a net logs so that we could investigate the issue? Instructions are available at https://dev.chromium.org/for-testers/providing-network-details.

Comment 3 by baranov...@yandex-team.ru, Jan 24 2017

Nope, instead I uploaded CL to fix it. But may also record a net-log.

https://codereview.chromium.org/2642723008/

Comment 4 by bugdroid1@chromium.org, Jan 25 2017

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a027199ed55462ac711f04cc5c23eff59678beb1

commit a027199ed55462ac711f04cc5c23eff59678beb1
Author: baranovich <baranovich@yandex-team.ru>
Date: Wed Jan 25 15:02:13 2017

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}

[modify] https://crrev.com/a027199ed55462ac711f04cc5c23eff59678beb1/net/http/http_network_transaction_unittest.cc
[modify] https://crrev.com/a027199ed55462ac711f04cc5c23eff59678beb1/net/http/http_stream_factory_impl_job.cc

Comment 5 by ligim...@chromium.org, Jan 25 2017

Cc: ligim...@chromium.org bustamante@chromium.org
Labels: -Needs-Feedback M-56 Merge-Request-56
M56 full stable release is scheduled next week.
Does this fix needs a merge? If yes, please apply Merge-Request-56 label once the fix is verified in canary.

Comment 6 by sheriffbot@chromium.org, Jan 25 2017

Project Member
Labels: -Merge-Request-56 Merge-Review-56 Hotlist-Merge-Review
This bug requires manual review: We are only 5 days from stable.
Please contact the milestone owner if you have questions.
Owners: amineer@(clank), cmasso@(bling), gkihumba@(cros), bustamante@(desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 7 by bustamante@google.com, Jan 27 2017

Labels: -Merge-Review-56 Merge-Rejected-56
Since this isn't a regression (plaintext websockets previously didn't work via HTTP/2 proxy) let's wait until M57.  We're really late in the cycle on M56 and are only taking high severity regressions/security changes at this point.

Comment 8 by wangyix@chromium.org, Jan 27 2017

Status: Fixed (was: Unconfirmed)
https://codereview.chromium.org/2642723008

Comment 9 by bugdroid1@chromium.org, Feb 2 2017

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c4e94751fb488deafe919a85f95037ac6c30491c

commit c4e94751fb488deafe919a85f95037ac6c30491c
Author: ricea <ricea@chromium.org>
Date: Thu Feb 02 20:34:32 2017

Revert of Allow proxying plaintext websockets over http/2 proxy (patchset #4 id:60001 of https://codereview.chromium.org/2642723008/ )

Reason for revert:
I believe this is causing the crashes in http://crbug.com/685968.

Original issue's description:
> 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/+/a027199ed55462ac711f04cc5c23eff59678beb1

TBR=mmenke@chromium.org,bnc@chromium.org,baranovich@yandex-team.ru
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG=684681

Review-Url: https://codereview.chromium.org/2669313002
Cr-Commit-Position: refs/heads/master@{#447832}

[modify] https://crrev.com/c4e94751fb488deafe919a85f95037ac6c30491c/net/http/http_network_transaction_unittest.cc
[modify] https://crrev.com/c4e94751fb488deafe919a85f95037ac6c30491c/net/http/http_stream_factory_impl_job.cc

Comment 10 by bemasc@chromium.org, Jul 24 2017

Cc: sfrolov@google.com bemasc@chromium.org
Labels: -OS-Mac OS-All
Owner: ricea@chromium.org
Status: Untriaged (was: Fixed)
Since the change was reverted, this bug is now present again: standard websockets are broken when using a standard HTTP/2 proxy.

The change here is very simple, so if it is causing a crash, it should be easy to fix.

Comment 11 by ricea@chromium.org, Jul 25 2017

Labels: -Pri-2 Pri-3
The change is simple but the crash is not.

Comment 12 by xunji...@chromium.org, Jul 28 2017

Status: Assigned (was: Untriaged)

Comment 13 by starsare...@gmail.com, Feb 1 2018

One of our users experienced this error in Chrome today for a website that was using secure websockets. However, the website worked fine in Firefox.

The user might have been behind an HTTP/2 proxy.

Comment 14 by bugdroid1@chromium.org, Mar 30 2018

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/420bb2d8f03a42d0c369597c241a32aca989811f

commit 420bb2d8f03a42d0c369597c241a32aca989811f
Author: Bence Béky <bnc@chromium.org>
Date: Fri Mar 30 17:25:26 2018

Add tests for WebSocket over HTTP/2 proxy.

Bug: 684681, 819101
Change-Id: Ifb7c4ee4f91d00e195c6e7e2da4ca65c86def4a7
Reviewed-on: https://chromium-review.googlesource.com/987518
Reviewed-by: Ryan Hamilton <rch@chromium.org>
Commit-Queue: Bence Béky <bnc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#547199}
[modify] https://crrev.com/420bb2d8f03a42d0c369597c241a32aca989811f/net/spdy/chromium/spdy_network_transaction_unittest.cc

Sign in to add a comment