Option to disable automatic X-Forwarded-For append/add#8576
Option to disable automatic X-Forwarded-For append/add#8576lextiz wants to merge 1 commit intoenvoyproxy:mainfrom
Conversation
✅ Deploy Preview for cerulean-figolla-1f9435 canceled.
|
b5eb4b4 to
54a782c
Compare
54a782c to
0eb05e9
Compare
0eb05e9 to
2856c19
Compare
internal/xds/translator/listener.go
Outdated
| UseRemoteAddress: &wrapperspb.BoolValue{Value: useRemoteAddress}, | ||
| SkipXffAppend: ptr.Deref(irListener.Headers, ir.HeaderSettings{}).DisableXForwardedFor, |
There was a problem hiding this comment.
When use_remote_address is true, XFF is already skipped.
There was a problem hiding this comment.
And if you are setting it to false for using client ip detection then it should be set in the extension not in HCM
There was a problem hiding this comment.
gateway/internal/xds/translator/listener.go
Lines 182 to 187 in 3c2fc03
There was a problem hiding this comment.
Thanks for the review! I have removed the HCM-level setting entirely and keep SkipXffAppend only in the XFF original IP detection extension config.
2856c19 to
e4673e9
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8576 +/- ##
==========================================
+ Coverage 74.29% 74.35% +0.05%
==========================================
Files 243 243
Lines 38155 38156 +1
==========================================
+ Hits 28347 28369 +22
+ Misses 7815 7799 -16
+ Partials 1993 1988 -5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
e4673e9 to
1e26506
Compare
|
@rudrakhp Is there a chance that this change would make it to v1.8? |
internal/ir/xds.go
Outdated
|
|
||
| // DisableXForwardedFor controls if Envoy should stop appending the downstream address to | ||
| // the X-Forwarded-For header. The default is to keep appending the downstream address. | ||
| DisableXForwardedFor bool `json:"disableXForwardedFor,omitempty" yaml:"disableXForwardedFor,omitempty"` |
There was a problem hiding this comment.
| DisableXForwardedFor bool `json:"disableXForwardedFor,omitempty" yaml:"disableXForwardedFor,omitempty"` | |
| DisableXForwardedForAppend bool `json:"disableXForwardedForAppend,omitempty" yaml:"disableXForwardedFor,omitempty"` |
| }) | ||
| } else if clientIPDetection.XForwardedFor.NumTrustedHops != nil { | ||
| xffHeaderConfigAny, _ = proto.ToAnyWithValidation(&xffv3.XffConfig{ | ||
| XffNumTrustedHops: xffNumTrustedHops(clientIPDetection), |
There was a problem hiding this comment.
Check if this logic in xffNumTrustedHops() is affected by change in appending behavior. For this we can write a E2E test with XFF append disabled and using num trusted hops.
64a8de7 to
8634dde
Compare
|
@rudrakhp Could you please approve running the |
8634dde to
e23f9f1
Compare
Signed-off-by: Alexander Bolshakov <lextiz@gmail.com>
e23f9f1 to
7dac72a
Compare
rudrakhp
left a comment
There was a problem hiding this comment.
Wondering about the value proposition here. XFF is appended only if XFF settings in client IP detection is set in CTP. You do this only if downstream is trusted and XFF is reliable. If downstream is NOT trusted (assuming that is why you want to skip appending it's IP to the XFF) then why would you trust the XFF header sent by the downstream? There is no point in configuring XFF based client IP detection.
cc: @envoyproxy/gateway-maintainers
What type of PR is this?
api: add ClientTrafficPolicy option to disable automatic X-Forwarded-For append
What this PR does / why we need it:
This PR adds a new
ClientTrafficPolicyoption to disable Envoy Gateway’s automatic appending of the downstream address to theX-Forwarded-Forheader.Today, EG appends to
X-Forwarded-Forin both the default HTTP connection manager path and the XFF original IP detection path, without a user-facing way to turn that off. This change exposes a policy-level switch and wires it through to the relevant Envoy settings while preserving existing behavior by default.The motivation is similar to #8527, which made
x-forwarded-hostbehavior configurable.Which issue(s) this PR fixes:
No issue exists
Release Notes: Yes