mesh: support configuring XFF trusted cidrs#3344
mesh: support configuring XFF trusted cidrs#3344rudrakhp wants to merge 1 commit intoistio:masterfrom
Conversation
|
😊 Welcome @rudrakhp! This is either your first contribution to the Istio api repo, or it's been You can learn more about the Istio working groups, Code of Conduct, and contribution guidelines Thanks for contributing! Courtesy of your friendly welcome wagon. |
istio-testing
added
the
size/S
rudrakhp
force-pushed
the
xff_trusted_cidrs
branch
from
e616c7d to
76fbb00
Compare
rudrakhp
force-pushed
the
xff_trusted_cidrs
branch
2 times, most recently
from
6e02798 to
902e15f
Compare
rudrakhp
force-pushed
the
xff_trusted_cidrs
branch
from
902e15f to
104a221
Compare
Signed-off-by: Rudrakh Panigrahi <rudrakh97@gmail.com>
rudrakhp
force-pushed
the
xff_trusted_cidrs
branch
from
104a221 to
f1f3a7b
Compare
|
@istio/technical-oversight-committee bumping this up. Thanks! |
| // If all addresses in X-Forwarded-For (XFF) header are within the trusted list, the first (leftmost) entry is used. | ||
| // See [Envoy XFF](https://www.envoyproxy.io/docs/envoy/latest/configuration/http/http_conn_man/headers#config-http-conn-man-headers-x-forwarded-for) | ||
| // header handling for more details. Only one of `numTrustedProxies` and `trustedCidrs` may be set. | ||
| repeated string trusted_cidrs = 4; |
There was a problem hiding this comment.
The envoy docs say we cannot use it with UseRemoteAddress. UseRemoteAddress is currently always set to 'true' for gateways.
What will be the behavior if a user sets this? Will we set useRemoteAddress=false? What side effects does this have beyond deciding how to parse the XFF header?
There was a problem hiding this comment.
@howardjohn Yes we will have to always set UseRemoteAddress to false if we decide to use the XFF original IP detection extension (which is recommended over the alternative we use today, might soon be on deprecation path).
What side effects does this have beyond deciding how to parse the XFF header?
From the docs it does look like the only thing that changes is how the original IP is determined, HCM does it natively today, we will be moving to original IP detection extension.
Meanwhile waiting on a probable fix for difference in behaviour of HCM and the XFF origin detection extension: envoyproxy/envoy#37780
There was a problem hiding this comment.
@howardjohn please refer envoyproxy/envoy#34241 (comment) for moving to original IP detection extension with backward compatibility. Corresponding implementation in envoyproxy/gateway as an example. WDYT?
There was a problem hiding this comment.
Is your goal to use a detection extension and set xff_trusted_cidrs? Is this change blocked on a similar change as above?
There was a problem hiding this comment.
Is your goal to use a detection extension and set xff_trusted_cidrs
Yes
Is this change blocked on a similar change as above?
It's technically not blocked. According to discussion in envoyproxy/envoy#34241 (comment), setting UseRemoteAddress to false will unblock setting trusted CIDRs, but will change the behaviour of XFFNumTrustedHops set today in HCM. So for XFFNumTrustedHops to remain backward compatible after unsetting USeRemoteAddress, we will have to decrement the user configured num hops before setting it in the detection extension. Please refer to comment referred above with an example of equivalent configurations.
There was a problem hiding this comment.
Could we move forward with this now @howardjohn ? We've tried working around this with EnvoyFilter but realize that with different gateway topologies this PR is the only proper fix.
The issue in the other comment seems to be resolved now.
istio-testing
added
the
needs-rebase
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
| // When the remote IP address matches a trusted CIDR and the X-Forwarded-For (XFF) header was sent, | ||
| // each entry in the X-Forwarded-For (XFF) header is evaluated from right to left | ||
| // and the first public non-trusted address is used as the original client address. |
There was a problem hiding this comment.
This differs from the current envoy documentation. https://github.com/envoyproxy/envoy/blob/main/docs/root/configuration/http/http_conn_man/headers.rst?plain=1#L274-L276
- If the remote address is contained by an entry in
xff_trusted_cidrs, and the last
(rightmost) entry is also contained by an entry inxff_trusted_cidrs, the trusted client
address is second-last IP address in XFF.
Based on the docs I'd expect the second to last IP address in the XFF header to be used as the original client address. Not sure if this documentation is correct though. The docs conflict with a later example https://github.com/envoyproxy/envoy/blob/main/docs/root/configuration/http/http_conn_man/headers.rst?plain=1#L370-L383
Example 7: Envoy as edge proxy, with one trusted CIDR
Settings:
| use_remote_address = false
| xff_trusted_cidrs = 192.0.2.0/24
Request details:
| Downstream IP address = 192.0.2.5
| XFF = "203.0.113.128, 203.0.113.10, 192.0.2.1"
Result:
| Trusted client address = 192.0.2.1
| X-Envoy-External-Address is set to 192.0.2.1
| XFF is changed to "203.0.113.128, 203.0.113.10, 192.0.2.1, 192.0.2.5"
| X-Envoy-Internal is removed (if it was present in the incoming request)```
Note the trusted client address is set to `192.0.2.1`. Based on the documentation I would have expected the trusted client address to be set to `203.0.113.10`. I thought maybe the `second-last` address was evaluated after appending the downstream IP address to the XFF but this also conflicts with a later example https://github.com/envoyproxy/envoy/blob/main/docs/root/configuration/http/http_conn_man/headers.rst?plain=1#L385-398
Example 8: Envoy as edge proxy, with two trusted CIDRs
Settings:
| use_remote_address = false
| xff_trusted_cidrs = 192.0.2.0/24, 198.51.100.0/24
Request details:
| Downstream IP address = 192.0.2.5
| XFF = "203.0.113.128, 203.0.113.10, 198.51.100.1"
Result:
| Trusted client address = 203.0.113.10
| X-Envoy-External-Address is set to 203.0.113.10
| XFF is changed to "203.0.113.128, 203.0.113.10, 198.51.100.1, 192.0.2.5"
| X-Envoy-Internal is removed (if it was present in the incoming request)```
In example 8, the trusted client address is set to 203.0.113.10 which is the expected second-last XFF IP.
@rudrakhp Could you please verify the expected behavior? I'd block merging until this is clarified.
There was a problem hiding this comment.
Created an envoy issue to track this envoyproxy/envoy#38462
There was a problem hiding this comment.
Based on my investigation and digging into the envoy code, your comment is correct. The envoy documentation is incorrect. Updated the envoy issue accordingly envoyproxy/envoy#38462 (comment)
8 participants
Ref istio/istio#53185
Related feature in envoy here.