Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Websockets not working behind Skipper reverse proxy due to Transfer-Encoding: chunked #3798

Closed
nebularazer opened this issue May 24, 2019 · 6 comments

Comments

@nebularazer
Copy link
Contributor

commented May 24, 2019

Long story short

When running the websockets example server behind Skipper the websocket connection is dropped immediately after connect. "One or more reserved bits are on: reserved1 = 0, reserved2 = 1, reserved3 = 1"

Expected behaviour

Websocket connection is established without problems when running behind Skipper.

Actual behaviour

Websocket connection is dropped immediately after connect.
"One or more reserved bits are on: reserved1 = 0, reserved2 = 1, reserved3 = 1"

Wireshark output
wireshark

@szuecs helped to debug this issue and found that Transfer-Encoding: chunked seems to be the issue.

When i commented out the Transfer-Encoding header in:

The example started to work.
Question: Is Transfer encoding really necessary for websockets or could it be removed / made optional to increase compatibility with http proxy servers like Skipper?

Steps to reproduce

  • Run skipper with docker (forwards 8000 to 8080):
docker run --rm -it --network host registry.opensource.zalan.do/pathfinder/skipper:v0.10.216 skipper -experimental-upgrade -experimental-upgrade-audit -application-log-level DEBUG -address :8000 -inline-routes 'r: * -> "http://127.0.0.1:8080/"'
  • Run the web_ws.py example
  • Check the logs in the Browser and Skipper console

Your environment

  • Fedora Linux 29 64bit
  • python 3.7.3 (also tested with python 3.6.8)
  • aiohttp 3.5.4
  • skipper v0.10.216
  • Chrome 74.0.3729.169

This issue was initially raised @ miguelgrinberg/python-socketio#300

@asvetlov

This comment has been minimized.

Copy link
Member

commented May 24, 2019

That's interesting.
After re-reading RFC 6455 I found no Transfter-Encoding mentions.
From my understanding, it means that the protocol is chunk-neutral on WebSocket upgrading.
Looks like a minor bug in Skipper.
On the other hand, chunked-encoding is redundant.
I wrote this code but don't remember why I added the chunked encoding mode. It just works (except Skipper).

I'm open to dropping Transfer-Encoding on WebSocket handshake.
Would you provide a Pull Request?

@szuecs

This comment has been minimized.

Copy link

commented May 24, 2019

@asvetlov I am also open to change skipper code. Maybe a configuration option to disable it in aiohttp project would be good.

What I don't understand is, that after connection upgrade, we Hijack the connection and only have 2 goroutines that copy data in both directions.
For whatever reason the chunked Transfer-Encoding seems no to be working with this.
We hijack it at https://github.com/zalando/skipper/blob/master/proxy/upgrade.go#L136.
Then we start the copy goroutines https://github.com/zalando/skipper/blob/master/proxy/upgrade.go#L156-L162. The upgrade was from skipper point of view successful and logged https://github.com/zalando/skipper/blob/master/proxy/upgrade.go#L163 , in the same second we get an unexpected finished websocket session https://github.com/zalando/skipper/blob/master/proxy/proxy.go#L777.

Any ideas what the problem could be?

nebularazer added a commit to nebularazer/aiohttp that referenced this issue May 24, 2019
@nebularazer

This comment has been minimized.

Copy link
Contributor Author

commented May 24, 2019

I added a PR which detects websockets and does not add the chunked header.

@asvetlov

This comment has been minimized.

Copy link
Member

commented May 24, 2019

I think that support an option to switch between chunked and plain cases in WebSocket upgrade doesn't make sense for aiohttp.
It adds a burden of documenting the unused by 99% users parameter, teaching people that the parameter is not a thing to touch, support tests, etc. We need to pick the best approach and use it.

Regarding help with Skipper: sorry, I have no time to dig into the code.
Python 3.8 feature freeze is coming in a week, I've committed to delivering a bunch of new important features. Even after 3.8-beta releasing, I should dedicate my time to work on things that are important too but don't change the public API, name it bug fixing.

My wild idea is: on WebSocket hijacking you don't switch the underlying parser back to plain mode. It reads chunk length but skips the delivering these 4 bytes to upstream, which corrupts the WebSocket protocol. Or something like this.

@szuecs

This comment has been minimized.

Copy link

commented May 24, 2019

@asvetlov thanks for your input.

asvetlov added a commit that referenced this issue Jul 19, 2019
asvetlov added a commit that referenced this issue Jul 19, 2019
[3.5] removed transfer-encoding header from websocket responses #3798 (
…#3800)

(cherry picked from commit 79c1ca4)

Co-authored-by: Florian S <nebularazer@gmail.com>
@asvetlov

This comment has been minimized.

Copy link
Member

commented Jul 19, 2019

Fixed by #3800

@asvetlov asvetlov closed this Jul 19, 2019

asvetlov added a commit that referenced this issue Jul 20, 2019
[3.5] removed transfer-encoding header from websocket responses #3798 (
…#3800) (#3920)

(cherry picked from commit 79c1ca4)

Co-authored-by: Florian S <nebularazer@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.