envoy
envoy copied to clipboard
envoyproxy
xff: add support for configuring a list of trusted CIDRs
Commit Message: xff: add support for configuring a list of trusted CIDRs
The original client IP address can be determined from the x-forwarded-for header either by a fixed number of trusted hops, or by evaluating the client IP address against a list of trusted addresses.
This adds support for configuring a list of CIDRs in the xff original IP detection extension. The remote IP address is evaluated against these, and optionally recurses through XFF to find the last non-trusted address.
Additional Description: This feature is generally used by people with a CDN in front of their edge proxy to ensure that XFF is only parsed when the remote connection comes from a CDN server.
The behaviour of the new functionality should be the same as Nginx's realip module.
Disclaimer: This is my first time writing C++ so I'm not certain my changes are completely idiomatic, but I've tried to stick with existing style in the codebase. Feedback very welcome!
Risk Level: Medium Testing: Unit tests, manual tests Docs Changes: Updates to HTTP Connection Manager header manipulation docs, and proto docs. Release Notes: Added to changelogs/current.yaml Platform Specific Features: None Fixes #21639 Relates to #31296
Hi @jamesog, welcome and thank you for your contribution.
We will try to review your Pull Request as quickly as possible.
In the meantime, please take a look at the contribution guidelines if you have not done so already.
:cat:
Caused by: https://github.com/envoyproxy/envoy/pull/31831 was opened by jamesog.
![repokitteh-read-only[bot] avatar](https://avatars.githubusercontent.com/in/16729?v=4 "Published by a pub.dev verified publisher"){kind=small}
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @markdroth
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).
:cat:
Caused by: https://github.com/envoyproxy/envoy/pull/31831 was opened by jamesog.
Apologies for the delay in pushing updates. I got caught in the weeds a bit trying to understand some of the internals.
I think I've addressed all of Alyssa's feedback, and I added more documentation and examples to headers.rst.
The one change I'm not sure about is a587a23d55ca8d042250c87b33d1d7db3af4f138 - I've tried to understand what "internal" means to Envoy and I still can't say I'm 100% sure. PR #2232 added some comments about it needing to be a single IP in XFF, so I've made a similar change here but I don't feel quite right about it.
Argh sorry I think the CI issue is real since some of this code goes in the mobile build which doesn't use exceptions. Can you take a look? Hopefully it will be an easy fix.
/wait
Argh sorry I think the CI issue is real since some of this code goes in the mobile build which doesn't use exceptions. Can you take a look? Hopefully it will be an easy fix.
Ah, I meant to follow up on this one, I noticed that failure too. I could use some advice here actually. In an earlier change I had the constructor for the xff extension forcibly setting xff_num_trusted_hops to 0 when xff_trusted_cidrs was set, but Alyssa recommended logging an exception instead.
What's the best way to fix this to work for mobile, do you think? Should I set it back to overwriting the value and logging it, or is there another way to make it fail that works for Mobile?
What's the best way to fix this to work for mobile, do you think? Should I set it back to overwriting the value and logging it, or is there another way to make it fail that works for Mobile?
Probably the best way would be to somehow make this code return an error and then somehow fail with an exception outside in the server (non mobile code) but we probably will need to ask @alyssawilk about this tomorrow when she is around.
Digging through the codebase a bit, I think I could either:
-
ENVOY_LOG(warn, "setting both xff_num_trusted_hops and xff_trusted_cidrs is not recommended"); or - Revert some of d976354a8906b186d4b01f2ab995594b2cca5933 and:
- Force set
xff_num_trusted_hops_to 0 whenhas_xff_trusted_cidrs() -
ENVOY_LOG(error, "cannot set both xff_num_trusted_hops and xff_trusted_cidrs; xff_num_trusted_hops is ignored")
- Force set
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
Not stale, waiting for feedback! But I'm also trying to find other examples in the codebase that do similar things, but there's a lot of code to examine...
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!
I put it back into draft because I need to make some more changes but I haven't had the time. I'm hoping to get back to it soon.
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!
Sorry for the long delay here. Life kind of got in the way and it took me some time to trawl through existing code to see if there was an existing pattern for handling the case on mobile to remove the throw. All tests are passing now, hopefully this covers the last of the feedback.
hm, CI looks wedged. can you do a main merge - maybe sorting current.yaml will fix?
