Add support for configuring trusted_proxies XFF depth

[nebez]

This relates to #5104

Firstly, wonderful software that really makes great default decisions. Thanks for caddy.

I suggest adding a configuration parameter to trusted_proxies which would allow deciding the depth at which the Client IP is in the X-Forwarded-For (referred to as XFF in this issue) stack of IPs. This is a feature request but also a bug/security issue. Currently, it is very easy to misuse trusted_proxies due to how XFF handling is implemented in caddy.

In the original PR, it is noted:

This implementation takes the left-most, first valid IP.

Taking the left-most value is actually the least trustworthy value in nearly all cases. Wikipedia entry re: XFF:

Since it is easy to forge an X-Forwarded-For field the given information should be used with care. The right-most IP address is always the IP address that connects to the last proxy, which means it is the most reliable source of information.

And since adam-p published his article on XFF, MDN has since updated their documentation as well to reflect the security risk in trusting the left-most IP address (thanks to a PR from adam-p).

Recommendation

There are two ways to manage XFF headers safely. The first is by specifying the depth at which your caddy reverse proxy exists (trusted proxy count), and the second is finding the first right-most IP address that isn't in your trusted proxy list (trust proxy list).

Currently caddy pulls the left-most valid IP address. This is not how most reverse proxies work with the header. There is only 1 reverse proxy I know of that behaves very incorrectly, which is Microsoft's IIS. It inserts previous hops to the right of the header instead of the left, which would make finding the left-most the appropriate approach. Instead, caddy should apply the same logic but find the right-most untrusted address. This would be the second Trusted proxy list approach, described above.

However to maintain backwards compatibility, you can also consider adding the Trusted proxy count option. This is as simple as defining a number that specifies how from from the right is the client IP address in the XFF header.

e.g. a header value of X-Forwarded-For: <spoofed address>, <client address>, <proxy 1 address>, <proxy 2 address>, we would use xff_depth 3 to pull the real client_ip address out and ignore the spoofed address.

Right now, the implementation of trusted_proxies means caddy can never safely extract the client IP while behind an AWS Application Load Balancer. See this documentation from AWS on how values are forwarded.

[francislavoie]

Thanks; yeah we can probably expand on it. A new truested_proxy_count <n> server option probably makes sense.

Would you be willing to contribute a PR to implement it?

Right now, the implementation of trusted_proxies means caddy can never safely extract the client IP while behind an AWS Application Load Balancer. See this documentation from AWS on how values are forwarded.

Sigh. Why don't they have a "replace" mode that ignores all incoming XFF and just sets it to the remote addr? That's so silly.

FWIW Cloudflare also does it incorrectly by default but it is possible to fix using their "rules". Explained here https://www.authelia.com/integration/proxies/fowarded-headers/#cloudflare

[nebez]

Yes, the AWS ALB approach is both odd and correct. Odd, because AWS CloudFront and all major reverse proxies do indeed provide a "wipe all incoming values, inject appropriate client IP". And correct because reverse proxies are meant to append in that direction, so you can have guarantees the right-most IP in XFF is the safest one. It seems like a huge oversight on AWS' part that they did not include this option in AWS ALB. I've flagged this to them in a Slack channel. Waiting to hear back.

I've never programmed in Go before but happy to take a stab at it. I saw @mholt post that your focus is on adding test coverage throughout the codebase. Spent the last little bit learning how to write tests, so I'll continue down that direction. A draft PR is open here: #5805

I believe this commit accurately describes the issue best. b0028d1

This should be a failing test – the only failing test I've written – which breaks backwards compatibility and highlights the potential security bug in a left-most approach. Take a look at the test + inline comment I've left:

In reality, caddy returns 30.30.30.30 here. Per MDN security recommendations This should be 45.54.45.54. The client IP logic in caddy is: if remoteAddress is trusted, starting from the left of XFF header, return the first IP address
The client IP logic should instead be:
if remote address is trusted, starting from the right of XFF header, return the first IP address that is untrusted.

I'd like to propose introducing a backwards-compatible switch to trusted_proxy config. Instead of specifying depth, the user can instead opt-in to a right-most trusted proxy scan. This functionality will, from right to left, find the first untrusted IP in the XFF stack and use that as the client IP. This follows both MDN's and adam-p's recommendations.

The other option is to break backwards compat and opt everyone into the right-most... but that seems annoying, even if it is the safe thing to do. Thoughts @francislavoie @mholt ?

I'll avoid moving much further until hearing back. It's my first time writing Go, and I don't want to commit too much time writing non-idiomatic code or tests if I'm way off track here. Let me know.

[nebez]

I made an attempt at introducing a new flag. The changes related to the flag can be seen in this commit. The previous tests in the PR all still pass. I think this should maintain backwards compatibility for anyone who wants to keep their existing client_ip behaviour.

af126fd

Let me know your thoughts on approach and naming.

[francislavoie]

This is mostly done in #5805, I think the only remaining piece is to support an optional <count> argument