Repository navigation
fix(ip): strip IPv6 zone before parsing IPs in ExtractIPFromXFFHeader - #3180
TheNetherWatcher wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
The fix addresses the reported IPv6 zone issue, but the current implementation introduces a couple of edge cases.
stripIPv6Zone() strips everything after % without validating whether the address is actually IPv6. This means malformed XFF entries like 127.0.0.2%garbage are now accepted instead of triggering the previous fallback behavior.
Additionally, with TrustLinkLocal(false), the same untrusted peer (fe80::1%eth0) produces different RealIP() values depending on whether an XFF header is present. This could lead to inconsistent IP-based rate limiting or access control.
Could we validate scoped IPv6 addresses more strictly, preserve the original direct peer address when it's untrusted, and add regression tests for these cases?
The overall approach looks good, but these edge cases should be addressed before merging. @aldas @vishr
|
|
||
| // stripIPv6Zone removes an IPv6 zone, e.g. "fe80::1%eth0", which net.ParseIP rejects. | ||
| func stripIPv6Zone(host string) string { | ||
| if i := strings.IndexByte(host, '%'); i != -1 { |
There was a problem hiding this comment.
The helper strips % suffixes from any address, including malformed IPv4 addresses such as 127.0.0.2%garbage. This allows previously invalid XFF entries to pass validation.
| ips[i] = strings.TrimPrefix(ips[i], "[") | ||
| ips[i] = strings.TrimSuffix(ips[i], "]") | ||
| ip := net.ParseIP(ips[i]) | ||
| ip := net.ParseIP(stripIPv6Zone(ips[i])) |
There was a problem hiding this comment.
With TrustLinkLocal(false), an untrusted zoned IPv6 peer returns fe80::1%eth0 without XFF but fe80::1 when XFF is present. This causes inconsistent RealIP() values for the same peer.
Use netip.ParseAddr so an IPv4 address with a "%" suffix (e.g. 127.0.0.2%garbage) is still rejected. Return the direct IP unchanged when the peer itself is untrusted, so RealIP() is the same with or without an X-Forwarded-For header.
TheNetherWatcher
left a comment
TheNetherWatcher
left a comment
There was a problem hiding this comment.
Thanks for the review, both points are addressed in db18e4b:
stripIPv6Zonenow usesnetip.ParseAddrand only drops the zone from a scoped IPv6 address. Anything else, including127.0.0.2%garbage, is returned unchanged, sonet.ParseIPstill rejects it and the extractor falls back to the direct IP as before.- When the direct peer itself is untrusted, the extractor returns
directIPunchanged. WithTrustLinkLocal(false),[fe80::1%eth0]:8080now givesfe80::1%eth0with or without an XFF header.
Added regression tests for both cases. They fail on the previous commit and pass now.
2 participants
Fixes #3179
ExtractIPFromXFFHeaderpassed the peer address straight tonet.ParseIP. For a link-local peer with a zone (fe80::1%eth0) that returnsnil, so the loop gave up and returned the proxy address instead of the client IP fromX-Forwarded-For.Changes:
stripIPv6Zoneand use it beforenet.ParseIPin the XFF loop. The returned value is unchanged when every hop is trusted (still the raw furthest entry).isTrustedPeer(scheme.go), which already stripped the zone inline.TestExtractIPFromXFFHeader: a zoned link-local peer with an external XFF entry, and an all-trusted zoned chain.go test -race ./...andgo vetpass. Both new cases fail on master.