-
-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
caddyhttp: Security enhancements for client IP parsing #5805
Conversation
Awesome, thanks for working on this! :) |
@francislavoie I've renamed the options, I like this new name better. Ty for suggestion. @francislavoie @mholt can I suggest this be a patch version of current 2.7.x release? This is a backwards compatible change and keeps the left-most parsing as a default. As-is, the For my web services behind caddy, I've disabled In addition, if you point me to the right direction, I will also include changes to the documentation of the website to mention this new opt-in option. Users should understand the behaviour of left-most vs. right-most XFF parsing and the security dangers of using left-most on untrusted upstreams. edit: I've put up a PR for documentation here caddyserver/website#346 |
Sorry for my latency on this, been a very busy time -- it is still on my radar though! I will circle back soon enough.
I think we can do that since this is basically a bugfix / not a new feature. |
No worries. When you've got the time, can you please also review caddyserver/website#346 as part of the same change? |
Yes we will try to do that :D (I will be a little busy for a while with some family obligations.) |
Great PR! I look forward to support for this case landing in Caddy. I must say though, I prefer the option that was suggested in the original issue (#5804) of instead having There are a lot of use cases where there could be an intermediary proxy, such as WAF -> Load Balancer -> Caddy, where it is useful to trust the second right-most value. I.e., trusting Matching the functionality of this PR would be achieved by setting I'd love to hear other perspectives on this. |
I'll also be able to revisit this PR soon @nebez |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Sorry for the delay. We had a feature freeze which we eventually cancelled, then the holidays.
Thanks for the thorough tests. This is only an opt-in behaviour change, so I'm comfortable to merge this safely. If there's any issues that crop up, they can be fixed in a followup.
Actually, I'll make a tiny adjustment to this right now as per @lachlan2k's comments. I think it's valid to say that count should be supported. Since the current "strict" behaviour effectively maps to count of "1", we can change the type from I don't want to block merging this any longer for an additional feature that can be added after the fact. |
That sounds quite pragmatic to me. Thanks for tackling that @francislavoie Once you're comfortable merging + tagging it, I'll update caddyserver/website#346 and then it will be ready for your review |
This is the first stab at addressing #5804
Commits are readable in order. This review is easiest done by going through them one-by-one:
client_ip_headers
functionalitytrusted_proxy
functionality with custom rangestrusted_proxy
+client_ip_headers
configurationclient_ip_headers
The first 3 commits provide confidence in maintaining backwards compatibility for those who rely on the existing, left-most parsing of XFF headers. The remaining commits introduce tests + config options (default off) to enable the new right-most, first untrusted IP XFF parsing.