fabiolb / fabio

Consul Load-Balancing made simple
https://fabiolb.net
MIT License
7.25k stars 621 forks source link

X-Forwarded-For doesn't seem to be working correctly #828

Open far-blue opened 3 years ago

far-blue commented 3 years ago

I'm struggling with the Forwarded and X-Forwarded-For headers on http traffic passing through Fabio.

Our Fabio instances sit behind a Cloudflare style WAF/LB setup and I can see, via tcpdump, that this setup is correctly passing X-Forwarded-For headers. However, traffic passing through Fabio seems to drop all that info and the only entry in the X-Forwarded-For (and Forwarded) headers is the IP address of the WAF/LB.

For instance: With a client IP of 1.2.3.4 and a WAF IP of 5.6.7.8 and a Fabio IP of 9.9.9.9

I see the following using tcpdump from the WAF:

X-Forwarded-For: 1.2.3.4 X-Forwarded-Proto: https

I see the following using tcpdump coming from Fabio:

X-Forwarded-For: 5.6.7.8 X-Forwarded-Proto: https Forwarded: for=5.6.7.8; proto=https; by=9.9.9.9; httpproto=http/2.0;

I would expect instead to see:

X-Forwarded-For: 5.6.7.8, 1.2.3.4 X-Forwarded-Proto: https Forwarded: for=1.2.3.4; proto=https; by=9.9.9.9, by=5.6.7.8, httpproto=http/2.0;

I've tried leaving proxy.header.clientip blank and also setting it to 'X-Forwarded-For'. Looking at the go http source code I would have expected a blank value to equate to the behaviour I'm seeing and 'X-Forwarded-For' to correctly retain and extend the X-Forwarded-For header. But this is not what I'm seeing.

Anyone got any ideas?

aaronhurt commented 3 years ago

Hi Robert,

Thank you for the report. The HTTP header code hasn't changed in quite a while. Setting proxy.header.clientip to X-Forwarded-For or X-Real-Ip is going to be ignored. That's a noop.

https://github.com/fabiolb/fabio/blob/81ed7aaf12ee494c676e0bbd830fd549ef0a4e1d/proxy/http_headers.go#L43-L50

The Forwarded behavior is expected. We don't pull from X-Forwarded-For to add to the Forwarded header. The Forwarded header is always set but will preserve existing information already present in a Forwarded header.

https://github.com/fabiolb/fabio/blob/81ed7aaf12ee494c676e0bbd830fd549ef0a4e1d/proxy/http_headers.go#L94-L114

The one condition where we might replace an upstream X-Forwarded-For header is in the case of a websocket connection.

https://github.com/fabiolb/fabio/blob/81ed7aaf12ee494c676e0bbd830fd549ef0a4e1d/proxy/http_headers.go#L59-L62

The only other handling of the X-Forwarded-For header is done in the golang standard http library.

https://golang.org/pkg/net/http/httputil/#ReverseProxy

If you have more information of a case with full headers I'd be happy to take a further look.

far-blue commented 3 years ago

Hi :)

Where you say setting proxy.header.clientip to X-Forwarded-For is a noop, my understanding is that it was passed to the underlying go http proxy library and used as per lines 278-285 here: https://golang.org/src/net/http/httputil/reverseproxy.go in order to instruct the go proxy code to preserve the existing X-Forwarded-For content. But that isn't working.

What I start with, before a request passes through Fabio, is a correctly filled X-Forwarded-For header (including the correct source IP) and no Forwarded header. What I see coming out the other side is a Forwarded header that states the upstream proxy is is source IP and an X-Forwarded-For header that has lost all the previous content and which has been reset to also state the source IP is the upstream proxy. This is for an http connection (technically originally http/2.0), not a web socket.

All I really want to know is the original source IP address so I'm not really bothered how this is achieved but at the moment neither of the headers are giving me the info I need.

When you say you'd like full headers, could you be a bit more specific? I'm pulling these from the wire using tcpdump so I'll need to target the headers you want to see.

aaronhurt commented 3 years ago

I'm saying that setting proxy.header.clientip to X-Forwarded-For is a noop because we ignore the proxy.header.clientip setting if it's set to X-Forwarded-For or X-Real-Ip. We always set X-Real-Ip but let reverseproxy.go handle setting X-Forwarded-For, unless it's a websocket, because websockets do not go through that same code you linked.

We look for and modify the Forwarded header but only touch X-Forwarded-For within Fabio for websocket requests. For standard http connections Fabio should not be modifying the upstream X-Forwarded-For header.

If the upstream X-Forwarded-For header is being lost I do not see where that would happen within Fabio. I'll mock up a test with curl and fabio pointing to an nc instance to see the full headers and post the results.

far-blue commented 3 years ago

Hi :)

I wonder whether the problem might be that the string value of X-Forwarded-For isn't making it into the reverse proxy config in go. I notice there was a change of behaviour note in the code that a null (maybe also an empty string?) would cause the reverse proxy code to drop the upstream X-Forwarded-For header content and create a new one - which seems very much the behaviour I'm seeing.

I really appreciate your help and offer of some testing. If there's anything more I can provide please let me know and I'll do what I can :)