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

Make forwarded scheme handle comma separated schemes #3

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kgann
Copy link

@kgann kgann commented Feb 20, 2015

When using HAProxy to add X-Forwarded-Proto via reqadd 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?

@jcf
Copy link
Member

jcf commented Feb 21, 2015

The draft spec for x-forwarded-proto suggests the header is supposed to inform a client which protocol is being used. The idea being you can modify your response to provide the correct protocol in links etc.

I'm not sure http, https makes sense in this context, and wonder how an application would behave with a confusing value like this.

Can HAProxy be configured to pass http or https based on which is actually being used?

@kgann
Copy link
Author

kgann commented Feb 21, 2015

This issue is 100% due to a poorly configured HAProxy.

I'm not sure http, https makes sense in this context, and wonder how an application would behave with a confusing value like this.

Perhaps the test is confusing - the real header value that broke the application was "https,https".

Originally HAProxy was configured correctly and the application worked fine. Someone made a change (duplicating: reqadd X-Forwarded-Proto:\ https) that caused the app to throw an assertion error.

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.

@jcf
Copy link
Member

jcf commented Feb 21, 2015

https,https isnt a valid value according to the spec either…

A few ideas spring to mind:

  1. Change the code to accept any valid scheme rather than just HTTP and HTTPS
  2. Return a client error rather than server error when we can't parse the header
  3. Expect clients to be well behaved (which is perhaps naive)

What do you think @weavejester?

@weavejester
Copy link
Member

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.

@kgann
Copy link
Author

kgann commented Feb 23, 2015

I think the assertion did its job in alerting me to the issue. Thanks for the feedback.

@kgann kgann closed this Feb 23, 2015
@rwilson
Copy link

rwilson commented Jul 24, 2018

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 x-forwarded-* headers, and given the wide usage of that service, it might be deemed close to standard.

For context:
We encountered this while debugging issues with using Readme.io's ability to test API calls from the browser. Their system proxies the request through a chain, each of which appends the previous client to the x-forwarded-* headers, before arriving at our ELB, which appends the final proxy from Readme.io. By the time it reaches us, the header value is "https,https,http,http,http,http,http,http".

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:

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.

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.

@weavejester
Copy link
Member

Further, AWS ELB behavior appends to the chain of x-forwarded-* headers, and given the wide usage of that service, it might be deemed close to standard.

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.

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.

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."

@rwilson
Copy link

rwilson commented Jul 25, 2018

Regarding the paper trail, the AWS documentation doesn't specify the behavior of ELBs if x-forwarded-* headers are present on an incoming request, so I have reached out to support for clarification. I'll follow-up when I've heard back.

@rwilson
Copy link

rwilson commented Aug 7, 2018

Finally got sufficient context on this; it depends on the ELB listener mode.

If configured as HTTP/HTTPS, the behavior is, from AWS:

"X-Forwarded-For” – ELB will append to existing value if this header existed in request header, otherwise it will be added
"X-Forwarded-Proto” – ELB will add this header. If this header existed, ALB will replace it.
"X-Forwarded-Port” – ELB will add this header. If this header existed, ALB will replace it.

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:

When you use TCP (layer 4) for both front-end and back-end connections, the load balancer forwards the request to the back-end instances without modifying the headers. In that case, you should see the headers as they are sent by the client.

@weavejester
Copy link
Member

Do you have references for those quotations? Or is that just what support told you?

@rwilson
Copy link

rwilson commented Aug 24, 2018

Unfortunately not, that was from the support response.

@weavejester weavejester reopened this Aug 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants