-
Notifications
You must be signed in to change notification settings - Fork 9
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
Make forwarded scheme handle comma separated schemes #3
base: master
Are you sure you want to change the base?
Conversation
The draft spec for I'm not sure Can HAProxy be configured to pass |
A quick Google seems to suggest HAProxy can be configured to do the 'right' thing. |
This issue is 100% due to a poorly configured HAProxy.
Perhaps the test is confusing - the real header value that broke the application was Originally HAProxy was configured correctly and the application worked fine. Someone made a change (duplicating: The fix is to configure HAProxy correctly but I fear that in the future, a similar change could bring down the application again. I decided to open a PR and get some input. At any rate, I'm going to have to duplicate this middleware to prevent the errors in the future. I'm fine with closing this PR since you can't be expected to accommodate all possible incorrect header values. |
A few ideas spring to mind:
What do you think @weavejester? |
In HTTP, all header values can be concatenated together with a comma, so my guess is that it is technically correct, but it doesn't make any practical sense. For security purposes the proxy should replace the whole header, since otherwise a HTTP client could make the server believe that it's actually being served over HTTPS. Maybe we should have a more detailed error message, but I think this should throw an exception, because of the security implications. |
I think the assertion did its job in alerting me to the issue. Thanks for the feedback. |
Could reopening this issue be considered? The Forwarded HTTP Extension spec considers a chain of proxy servers and specifies an order for the chain. Further, AWS ELB behavior appends to the chain of For context: Bottom line: there are situations where you can't control all hops, and ring-ssl could be made flexible enough to handle those in a non-breaking manner. A final note, with regard to:
The opposite is also true: by replacing the whole header, a proxy could make a server believe the request is HTTPS end-to-end, when it may not be. Ultimately, any proxy outside your network (and maybe those inside?) can't be trusted. |
Do you have a reference for that? It seems reasonable to support the behavior if that's how AWS behaves, but I'd like a "paper trail" justifying the decision.
Sure, but if you have a proxy inside your network then that should take priority, and if you don't, then you shouldn't be using the middleware. By using the middleware you're saying: "I know a trustworthy proxy will set this value." |
Regarding the paper trail, the AWS documentation doesn't specify the behavior of ELBs if |
Finally got sufficient context on this; it depends on the ELB listener mode. If configured as HTTP/HTTPS, the behavior is, from AWS:
However, if configured as TCP/SSL (e.g. w/ proxy protocol to support WebSockets), the behavior is to preserve the client headers and append-only. Quoting AWS:
|
Do you have references for those quotations? Or is that just what support told you? |
Unfortunately not, that was from the support response. |
When using HAProxy to add
X-Forwarded-Proto
viareqadd X-Forwarded-Proto ...
and the header already exists, the value will be comma separated. This causes an assertion error to be thrown.The real issue is with the HAProxy configuration as it should detect the presence of the header and behave accordingly. I'm not sure if this should be a concern of ring-ssl. Thoughts?