feat: middleware.ClientIP, a replacement for middleware.RealIP#967
feat: middleware.ClientIP, a replacement for middleware.RealIP#967VojtechVitek wants to merge 4 commits into
Conversation
convto
left a comment
There was a problem hiding this comment.
I’ve checked the implementation, and I completely agree with the approach.
It addresses the concerns beautifully, and I’m very grateful for the effort put into this!
c2354ea to
4b70ef1
Compare
Rebased against master. The CI tests are now passing since we dropped support for Go 1.19 and older and thus can we use @jawnsy @adam-p @convto @Dirbaio @pkieltyka @mfridman @lrstanley @n33pm @c2h5oh I'd appreciate any more reviews 👀 🙏 . |
|
test cases ip spoofing (feel free to edit/delete) |
Covers single/multiple trusted proxy chains, spoofing attempts, prefix boundary values, and no-prefix baseline as requested in PR go-chi#967.
|
Thanks everyone for the thorough reviews and feedback on this PR. I've been figuring out how to work through it all — the comments, open advisories, and decisions needed. With a bit of help from Opus 4.7, I've worked through everything and landed on a plan. Give it a read and let me know if anything stands out. PR #967 — ClientIP middleware: Plan to finishA plan to address the open review feedback on Why this matters — three open advisoriesChi currently has three published security advisories, all
All three reporters propose the same fix shape: rightmost-untrusted This PR is the patch for all three advisories and the What the source-of-truth references sayadam-p's "The perils of the 'real' client IP"
Summary of reviewer feedback on PR #967c2h5oh (CHANGES_REQUESTED)
adam-p (CHANGES_REQUESTED)
rezmoss (proposed tests on PR #967)Four ready-to-paste test functions:
Saku0512 (follow-up PR #1087 on top of #967)Adds ConvergenceBoth blocking reviewers and all three advisory reporters Is the API right?Final surface for this PR: // Trust a single-IP header set by your infrastructure.
func ClientIPFromHeader(trustedHeader string) func(http.Handler) http.Handler
// Trust XFF, walking right-to-left and skipping the listed trusted CIDRs.
// Zero args ⇒ rightmost parseable IP (safe only behind one trusted hop).
func ClientIPFromXFF(trustedIPPrefixes ...string) func(http.Handler) http.Handler
// Trust XFF, knowing the exact number of trusted proxies in front of you.
// Returns xff[len-numTrustedProxies], or empty if XFF has fewer entries.
// Panics if numTrustedProxies < 1.
func ClientIPFromXFFTrustedProxies(numTrustedProxies int) func(http.Handler) http.Handler
// Trust the TCP RemoteAddr (only safe when directly connected to the internet).
func ClientIPFromRemoteAddr(h http.Handler) http.Handler
// Accessors.
func GetClientIP(ctx context.Context) string // empty if not set
func GetClientIPAddr(ctx context.Context) netip.Addr // zero value if not setScored against the blog + advisory checklist:
Why two XFF functions and not one?
They overlap intentionally for the trivial case: Proposed planPhase 1 — Code changes to
|
| Your setup | Use |
|---|---|
| Directly on the public internet, no proxy | ClientIPFromRemoteAddr |
Behind nginx (X-Real-IP), Cloudflare (CF-Connecting-IP), Apache (X-Client-IP) |
ClientIPFromHeader(...) |
| Behind one or more proxies whose IP ranges you can list | ClientIPFromXFF(cidr1, cidr2, …) |
| Behind a fixed number of proxies with dynamic IPs | ClientIPFromXFFTrustedProxies(n) |
The implementation is rightmost-untrusted, never second-guesses what
the user trusts, doesn't mutate r.RemoteAddr, and stores
netip.Addr in context. It is the patch for GHSA-3fxj-6jh8-hvhx,
GHSA-rjr7-jggh-pgcp, and GHSA-9g5q-2w5x-hmxf, and supersedes the
now-deprecated middleware.RealIP.
Rework the new ClientIP* middlewares per reviewer feedback (c2h5oh, adam-p, rezmoss, Saku0512) and align with the documented attack patterns in GHSA-3fxj-6jh8-hvhx, GHSA-rjr7-jggh-pgcp, GHSA-9g5q-2w5x-hmxf. API: - Rename ClientIPFromXFFHeader -> ClientIPFromXFF. - Add ClientIPFromXFFTrustedProxies(n) for dynamic proxy pools where enumerating CIDRs isn't practical (autoscaling, ephemeral containers). - Add GetClientIPAddr alongside GetClientIP; both back to a single netip.Addr stored in context. r.RemoteAddr is never mutated. - Drop IsLoopback/IsPrivate filtering: the user's explicit trust configuration is authoritative (k8s nginx-ingress and similar legitimately surface those values). - Merge multiple X-Forwarded-For header instances before walking (RFC 2616), defeating duplicate-header attacks. - Deprecate middleware.RealIP with citations to all three advisories and guidance pointing at the new API. Docs: - Per-function godoc explains exactly when each variant applies. - Example_clientIP is a single, consolidated decision guide rendered on pkg.go.dev. Tests: 56 subtests including explicit PoC reproductions of each advisory, /24 boundary cases (Saku0512), IPv6, multi-header merging, spoofing prevention, and middleware chaining (rezmoss). Co-authored-by: Cursor <cursoragent@cursor.com> Co-Authored-By: Claude Sonnet 4.7 <noreply@anthropic.com>
2268552 to
716b67a
Compare
Saku0512
left a comment
There was a problem hiding this comment.
Requesting changes for one documentation issue that can lead to incorrect client IP selection in real deployments. The implementation looks good, but the fallback ordering guidance for ClientIPFromXFFTrustedProxies appears reversed for chi's middleware execution order.
Saku0512 correctly flagged in #967 that the godoc told users to register ClientIPFromRemoteAddr *after* ClientIPFromXFFTrustedProxies for fallback behavior. With last-write-wins semantics (482e855) that recipe is actively wrong: on the happy path RemoteAddr would silently overwrite the legitimate XFF-derived client IP with the immediate proxy's address — reintroducing the spoofing-prone behavior this API was written to prevent. Rather than just reverse the order in the example, drop the chaining hint entirely. The whole point of 482e855 was to push users toward picking exactly one ClientIPFrom* middleware; documenting a chaining recipe in one specific function works against that. The honest statement is "no client IP is set and GetClientIP returns \"\"" — what the caller does about it is on them. Also rename TestClientIPChaining -> TestClientIPLastWriteWins and reword its subtests to describe the property being locked down rather than endorsing a fallback recipe. Same coverage, no implicit recommendation.
A proposed solution for the RealIP middleware issues outlined at:
#708
https://adam-p.ca/blog/2022/03/x-forwarded-for/
#711
#453
#908
Fixes GHSA-9g5q-2w5x-hmxf, GHSA-rjr7-jggh-pgcp and GHSA-3fxj-6jh8-hvhx.
Summary of differences:
RealIP,ClientIPmiddleware doesn't change the value ofreq.RemoteAddr.GetClientIP()getterbelonging to their infrastructure, such as proxies or Load Balances, depending on their setup
Kudos to Adam Pritchard, Jonathan Yu, Yuya Okumura, Liam Stanley, and everyone else involved in the detailed reports and discussion.
I'm seeking early feedback and reviews on the proposed API.
Big thanks to everyone involved:
@jawnsy @adam-p @convto @Dirbaio @rezmoss @c2h5oh @Saku0512 @mfridman @lrstanley @n33pm
P.S.: This PR uses the
"net/netip"package and assumes that we'll accept proposal to drop support for Go 1.17 and older. Please have a look!Example usage:
Get the client IP address value in HTTP handler: