mesh: support configuring XFF trusted cidrs

[rudrakhp]

Ref istio/istio#53185
Related feature in envoy here.

[istio-policy-bot]

😊 Welcome @rudrakhp! This is either your first contribution to the Istio api repo, or it's been
a while since you've been here.

You can learn more about the Istio working groups, Code of Conduct, and contribution guidelines
by referring to Contributing to Istio.

Thanks for contributing!

Courtesy of your friendly welcome wagon.

[rudrakhp]

@istio/technical-oversight-committee bumping this up. Thanks!

[howardjohn]

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?

[rudrakhp]

@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

[rudrakhp]

@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?

[jaellio]

Is your goal to use a detection extension and set xff_trusted_cidrs? Is this change blocked on a similar change as above?

[rudrakhp]

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.

[istio-testing]

PR needs rebase.

Instructions 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.

[jaellio]

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 in xff_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.

[jaellio]

Created an envoy issue to track this envoyproxy/envoy#38462

[jaellio]

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)

[rudrakhp]

@jaellio thanks for confirming!