Skip to content

Conversation

@zhaohuabing
Copy link
Member

@zhaohuabing zhaohuabing commented Dec 20, 2024

Commit Message: this PR resolves the issue of incorrect handling of XFF trusted hops, which was inconsistent between the two approaches—OriginalIpDetectionExtension and HCM xffNumTrustedHops—used for retrieving the remote IP from the XFF header. Additionally, the old behavior in the OriginalIpDetectionExtension was also not aligned with the Envoy documentation, which specifies that the original IP should correspond to the rightmost trusted hop in the XFF header.

For example, for a request going thourgh two trusted proxies, like this:

client(203.0.113.128) ----->proxy 1( 203.0.113.10) --------->proxy 2(203.0.113.1)--------> Enovy

proxy1 will add the client ip 92.168.1.1 to the XFF header, and proxy 2 will append it's direct upstream ip 92.168.1.2 to the XFF header. When Envoy finally receives the request, the xff header is X-Forwarded-For: 203.0.113.128, 203.0.113.10.

In this setup, the trusted hops is 2, and the correct client IP is the second rightmost ip in the XFF header 192.168.1.1

More details in the issue: #34241 (comment)

Disclaimer: I may have misunderstood the functionality of HCM xff_num_trusted_hops and the XFF extension. If the current behavior is correct, we would appreciate any assistance in addressing the related issue in Envoy Gateway envoyproxy/gateway#4702

Additional Description:
Risk Level:
Testing:
Docs Changes: No
Release Notes: Yes
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #34241]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

Related Envoy Gateay issue: envoyproxy/gateway#4702

Signed-off-by: Huabing Zhao <[email protected]>
@zhaohuabing zhaohuabing marked this pull request as draft December 20, 2024 08:53
@zhaohuabing

This comment was marked as outdated.

@phlax
Copy link
Member

phlax commented Dec 23, 2024

note: envoy maintainers will be mostly on vacation until 6th jan

tests are failing

/wait

@zhaohuabing
Copy link
Member Author

tests are failing

I'm holding off this PR until envoy maintainers confirm that this is the correct approach.

@alyssawilk
Copy link
Contributor

yeah I think all the XFF code is confusing as heck. commented on the original issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Thexff_num_trusted_hops in XffIPDetection is not aligned with the doc and the xffNumTrustedHops option in HCM

3 participants