X-Forwarded-Proto doesn't handle multiple values

[NickCraver]

Came across this via aspnet/KestrelHttpServer#365 (comment) and I'm glad to see it in the core this round. There's a bug I see though:

• X-Forwarded-For assumes comma-separated values (OverrideHeaderMiddleware.cs#L43) (correct).
• X-Forwarded-Proto assumes a single value (OverrideHeaderMiddleware.cs#L75) (incorrect).

Both of these should support comma-separated values. For example if you have an edge node in europe that's HTTPS but then HTTP back to the origin (the load balancer adding a second entry) and then to the app, you'll get:

X-Forwarded-Proto: http, https

Or if it's HTTPS at both hops (yay security!):

X-Forwarded-Proto: https, https

While it doesn't answer the complicated question of what the app should get in complicated scenarios (this will definitely vary with such setups), it needs to not at least not break by default. I think using the 0-th entry is correct in both cases for the basic middleware use case.

[muratg]

@Tratcher Was this a miss or was it intentional?

[Tratcher]

It's a bug. I'll work on it as part of my other improvements.

[Tratcher]

44f03ef

[NickCraver]

@Tratcher I think a lot of invalid assumptions are made in 44f03ef to the point it's not actually usuable for any web traffic we see today. For example, the counts of items in each header all being equal: that certainly isn't the case in most of the traffic we see out in the wild. For example many proxies never append http to the X-Forwarded-Proto chain because it just wasn't a thing when they were created.

To be blunt, these headers are a giant mess across the internet. The code in the middleware assumes far more normalization than is actually realistic now and I'd wager at least the next 3-5 years. When HTTP/2 takes hold and all the proxies get updated or don't fool with the traffic I think sanity will gain traction...but this needs to be usable well before that.

[Tratcher]

How many proxies do you see a normal request coming through? I'd only expect you to apply settings for the reverse proxies between your server and the internet, not all the way back to the client.

If one header like X-Forwarded-Proto is out of sync with the others but you still want to apply it, and know how many entries to trust, you can have a separate instance of the middleware configured just for that header. The way things are written that shouldn't cause issues or extra overhead.

[NickCraver]

How many proxies do you see a normal request coming through?

It varies a lot. Most users aren't behind a proxy, but of those that are we see 1-3 proxies proxies before the request hits CloudFlare (if we're behind them at the time), then HAProxy, then our web tier. Proxies in general are just garbage at handling these. We see things like internal 10. IPs, IPv4 and IPv6 mixed on and off in the chain, and almost always a mismatch on the X-Forwarded-Proto length.

We can't just throw it out headers for several reasons. For example, if your site is behind a proxy wall with local terminations (let's say CloudFlare, Fastly, etc.), you need to trust the headers all the way back to them because you need to know if the backhaul is strict or mixed (all HTTPS or HTTP back to origin). In some scenarios using HTTP to the origin is more sane (pre-HTTP/2) when over a persistent, encrypted, and multiplexed connection is maintained to the origin as a tunnel.

But even if you couldn't trust the headers: it's rare that you control their behavior anyway. For example, I can't instruct CloudFlare to strip the header. From their (valid) view, there's no reason for them to allow such a thing - they simply have a correctly behaving proxy.

My middleware can't know the a hard length for trust either, since that's a network level change that affects dozens of applications. It's also not an instant change in the real world, DNS lags and traffic shifts over the course of minutes at a minimum - so a pub/sub to reset ForwardLimit isn't really an option either.

you can have a separate instance of the middleware configured just for that header. The way things are written that shouldn't cause issues or extra overhead.

Perhaps I'm just not following here. The way it's written doesn't just exclude a mis-matching length, it early returns making the entire Middleware a wasted operation - that's quite a bit of overhead, to the point I just can't use this Middleware. Am I missing the intended handling of mis-matched lengths here?

[Tratcher]

@blowdart @lodejard, any thoughts?

you can have a separate instance of the middleware configured just for that header. The way things are written that shouldn't cause issues or extra overhead.

Specific headers are only checked if they're enabled via options.ForwardedHeaders. See https://github.com/aspnet/BasicMiddleware/blob/dev/samples/HttpOverridesSample/Startup.cs#L15. Having two instances of the middleware with different headers enabled should not have any unnecessary cost overhead.

Yes, X-Forwarded-* is quite a mess. Hopefully proxies adopt the new Forwarded header at some point. https://github.com/aspnet/BasicMiddleware/issues/28.

[kevinchalet]

I think a lot of invalid assumptions are made in 44f03ef to the point it's not actually usuable for any web traffic we see today. For example, the counts of items in each header all being equal: that certainly isn't the case in most of the traffic we see out in the wild. For example many proxies never append http to the X-Forwarded-Proto chain because it just wasn't a thing when they were created.

👍

@Tratcher FYI, this change prevents the override middleware from working on Azure (the X-Forwarded-Proto header is ignored because there's a single value and the scheme is not restored):

These are the forwarding headers I'm receiving in the request:
X-Original-URL /connect/token
X-Forwarded-For 91.130.136.28:3439, 91.130.136.28:3439
X-ARR-SSL 2048|256|C=US, S=Washington, L=Redmond, O=Microsoft Corporation, OU=Microsoft IT, CN=Microsoft IT SSL SHA2|CN=*.azurewebsites.net
X-Forwarded-Proto https

Interesting, in the logs I'm seeing the line "Parameter count mismatch between X-Forwarded-For and X-Forwarded-Proto."

/cc @flagbug

[halter73]

Interesting. @Tratcher Do you think we should create a new issue in this repo to fix dotnet/aspnetcore#1331?

[Tratcher]

This may have already been addressed in HttpPlatformHandler (1.3, soon..). Previously it would overwrite the X-Forwarded-Proto header where now it will append to it. So if the request was forwarded twice it should end up with two For values and two Proto values.

[mikes-gh]

ARR in IIS 7.5 doesn't set the X-Forwarded-Proto either by default. I also came across this problem.
I think we need to be more tolerant and not fail with

Parameter count mismatch between X-Forwarded-For and X-Forwarded-Proto.

There are just too many situations where this will occur.

[mikes-gh]

@Tratcher Appending may not help the parameter count mismatch if the header arrives with just an X-Forwarded-For at IIS.(common senario from reading this Issue and my experiences)
There will still be a mismatch.

[mikes-gh]

@Tratcher just finished installing my app behind nginx which receives the request from a forward from the firewall so that's 2 hops

Nginx does not have an easy way to append to x-forwarded-proto.

It always replaces the header.

In fact the best way is to use this map and set the result to x-forwarded-proto

http://serverfault.com/questions/515957/how-to-have-nginx-forward-the-http-x-forwarded-proto-header

So header symmetry will fail for anything except a simple one hop forward.
Given that you already do a workaround for azure

https://github.com/aspnet/IISIntegration/pull/189/commits

I wonder whether header symmetry should be off by default? There's just so many cases where it fails.

Another suggestion is to raise an exception when processing requirehttps attribute if RequireHeaderSymetry is true but there is a parameter count mismatch.

Problem is the issue is hard to track down and usually manifests itself with a redirect loop when hitting requirehttps attribute. I spent the best part of half a day debugging this issue. (twice believe it or not) . Once for ARR and once for nginx.
I had to set Microsoft logging level to debug and write a quick header logging middleware to work it out. Raising an exception would have saved me hours.

In the end I just set requireheadersymetry to false. For nginx

[Tratcher]

@mikes-gh please open a new issue with your nginx data so we don't loose track of it.