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

Consider enabling strict externalPort optionally #1360

Closed
HackToHell opened this issue Apr 15, 2020 · 2 comments · Fixed by #1378
Closed

Consider enabling strict externalPort optionally #1360

HackToHell opened this issue Apr 15, 2020 · 2 comments · Fixed by #1378

Comments

@HackToHell
Copy link

Hello,
I had noticed that autobahn strictly enforces the RFC https://tools.ietf.org/html/rfc7230#section-5.4. As far as I can tell(I could be wrong) the externalPort isn't used anywhere else apart from the check at https://github.com/crossbario/autobahn-python/blob/master/autobahn/websocket/protocol.py#L2611-L2633.
The problem arises when this is used inside a system such as Kubernetes, where there's an optional SSL termination at edge, all requests are routed via an Ingress node. This means all users have to set the external port to 80. (Even though outside it's 443).
I found the issue from RobotWebTools/rosbridge_suite#468, by internally switching the library, it broke compatibility with no way to maintain backward compatibility.
Would you consider making the externalPort check non-default and having a strict enforce mode?

@oberstet
Copy link
Contributor

No, Autobahn implements RFC6455, and removing the check per-default would divert from the standard.

However: Autobahn provides the option to properly feed the external port to the check via externalPort or disable the check by providing None for that.

@pramodhkp
Copy link
Contributor

pramodhkp commented Apr 27, 2020

@oberstet It looks like even if you set externalPort to None, it'd still pick up the port from URL and set it to externalPort. Should this be the case?

Referring to this line here: https://github.com/crossbario/autobahn-python/blob/master/autobahn/websocket/protocol.py#L3198-L3203

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants