ClientIP middleware proposal, intended to replace RealIP #967

VojtechVitek · 2024-12-15T21:02:39Z

A proposed solution for the RealIP middleware issues outlined at:
#708
https://adam-p.ca/blog/2022/03/x-forwarded-for/
#711
#453
#908

Summary of differences:

Unlike RealIP, ClientIP middleware doesn't change the value of req.RemoteAddr
Instead, it parses and saves the client IP to a context and provides .GetClientIP() getter
Makes users choose what to trust explicitly, e.g. a custom header / XFF header / req.RemoteAddr
XFF algorithm now selects the rightmost trusted IP address from all X-Forwarded-For headers
Additionally, XFF accepts a list of trusted IP prefixes, so users can opt-in to trust IP range
belonging to their infrastructure, such as proxies or Load Balances, depending on their setup
Should prevent client IP spoofing, which could affect rate-limiting in https://github.com/go-chi/httprate

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.

CC @jawnsy @adam-p @convto @Dirbaio @pkieltyka @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:

// Trust Cloudflare header:
r.Use(middleware.ClientIPFromHeader("CF-Connecting-IP"))
r.Use(middleware.ClientIPFromHeader("True-Client-IP")) // alternative in Cloudflare Enterprise

// Trust Azure header:
r.Use(middleware.ClientIPFromHeader("X-Azure-ClientIP"))

// Trust header from Nginx with ngx_http_realip_module:
r.Use(middleware.ClientIPFromHeader("X-Real-IP"))

// Trust header from Apache with mod_remoteip:
r.Use(middleware.ClientIPFromHeader("X-Client-IP"))

// Trust X-Forwarded-For header set by reverse-proxy in a private network:
r.Use(middleware.ClientIPFromXFFHeader("203.0.113.0/24"))

// Trust X-Forwarded-For header set by AWS CloudFront:
r.Use(middleware.ClientIPFromXFFHeader(
    "13.32.0.0/15",   // CloudFront IPv4
    "52.46.0.0/18",   // CloudFront IPv4
    "2600:9000::/28", // CloudFront IPv6
))

// Go server serving the Internet traffic directly:
r.Use(middleware.ClientIPFromRemoteAddr)

Get the client IP address value in HTTP handler:

clientIP := middleware.GetClientIP(r.Context())
// log the clientIP .. or use it for rate-limiting

convto

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!

VojtechVitek · 2025-01-20T20:55:49Z

P.S.: This PR uses the "net/netip" package and assumes that we'll accept #963.

Rebased against master. The CI tests are now passing since we dropped support for Go 1.19 and older and thus can we use "net/netip" pkg.

@jawnsy @adam-p @convto @Dirbaio @pkieltyka @mfridman @lrstanley @n33pm @c2h5oh

I'd appreciate any more reviews 👀 🙏 .

c2h5oh · 2025-01-20T20:53:43Z

middleware/client_ip.go

+				// Ignore loopback, private, or unspecified addresses.
+				if ip.IsLoopback() || ip.IsPrivate() || ip.IsUnspecified() {
+					continue
+				}


If trusted prefixes are set then we only care and/or can trust the first value past those trusted prefixes. No loopback, private or unspecified addresses should exist past/in between trusted prefixes unless headers have been tampered with.

Move trusted prefixes check block before this

Loopback/private/unspecified check should exit without setting client IP if any trusted prefixes are set

I don't think the loopback/private/unspecified checks should be here at all. The user has specified that they trust certain reverse proxies, and I don't think we should have code here that second-guesses that. I think the first non-trusted IP should be returned regardless of what it is.

I would even prefer that there not be a check for IP validity at all (i.e., the netip.ParseAddr) call above, but it's obviously necessary for prefix checking. If the parse fails, I think it should trigger an error condition and not just be skipped.

These kinds of bad/incorrect IP failures indicate a configuration error. Which is pretty bad!

I'm even not sure I like "don't store the IP" as the way to handle error states. Panicking by default is unpalatable (although that's how I prefer to handle developer/configuration errors). Maybe there can be a flag or callback or something? I don't feel super strongly about it, though.

c2h5oh · 2025-01-20T20:57:54Z

middleware/client_ip.go

+//
+// Note: Private IP ranges (e.g., "10.0.0.0/8", "192.168.0.0/16", "172.16.0.0/12")
+// are automatically excluded by netip.Addr.IsPrivate() and do not need to be added here.
+func ClientIPFromXFFHeader(trustedIPPrefixes ...string) func(http.Handler) http.Handler {


Having numTrustedProxies as an alternative to trusted prefixes would be a useful option too. If you control exactly n proxies, but not necessarily all possible proxies on a network (e.g. corporate intranet situation).

When set we should look only at nth rightmost value and only check if it's unspecified or loopback to reject - private is fine.

adam-p · 2025-02-09T20:54:49Z

middleware/client_ip.go

+// - "True-Client-IP"   - Akamai, Cloudflare Enterprise
+// - "X-Azure-ClientIP" - Azure Front Door
+// - "Fastly-Client-IP" - Fastly


Back when I wrote the blog post I found (mostly by reading, not testing) that these are not "trusted headers":

Azure's X-Azure-ClientIP

Akamai's True-Client-IP

Fastly's Fastly-Client-IP

adam-p · 2025-02-09T21:06:04Z

middleware/client_ip.go

+				// Ignore loopback, private, or unspecified addresses.
+				if ip.IsLoopback() || ip.IsPrivate() || ip.IsUnspecified() {
+					continue
+				}


I don't think the loopback/private/unspecified checks should be here at all. The user has specified that they trust certain reverse proxies, and I don't think we should have code here that second-guesses that. I think the first non-trusted IP should be returned regardless of what it is.

I would even prefer that there not be a check for IP validity at all (i.e., the netip.ParseAddr) call above, but it's obviously necessary for prefix checking. If the parse fails, I think it should trigger an error condition and not just be skipped.

These kinds of bad/incorrect IP failures indicate a configuration error. Which is pretty bad!

I'm even not sure I like "don't store the IP" as the way to handle error states. Panicking by default is unpalatable (although that's how I prefer to handle developer/configuration errors). Maybe there can be a flag or callback or something? I don't feel super strongly about it, though.

adam-p · 2025-02-09T21:07:25Z

middleware/client_ip.go

+
+			// Parse the IP address from the trusted header.
+			ip, err := netip.ParseAddr(r.Header.Get(trustedHeader))
+			if err != nil || ip.IsLoopback() || ip.IsUnspecified() || ip.IsPrivate() {


For the same reasons as I mention in reply to @c2h5oh's comment below, I don't think these checks should be here at all. (In short: The user has said this is trustworthy and we should treat it as such.)

adam-p · 2025-02-09T21:08:29Z

middleware/client_ip.go

+// field of the HTTP request and injects it into the request context if it is
+// not already set. The parsed IP address can be retrieved using GetClientIP().
+//
+// The middleware ignores invalid or private IPs.


As above: Why? The user has indicated that this is the value they want.

But also... I don't think you're actually doing the loopback/private/unspecified check here, so I don't think this comment is true?

adam-p · 2025-02-09T21:12:08Z

middleware/client_ip.go

+// Returns an empty string if no valid IP is found.
+func GetClientIP(ctx context.Context) string {
+	ip, ok := ctx.Value(clientIPCtxKey).(netip.Addr)
+	if !ok || !ip.IsValid() {


Depending on above discussions, the ip.IsValid check might not be right.

adam-p · 2025-02-09T21:14:39Z

middleware/client_ip_test.go

+}
+
+func TestClientIPFromXFFHeader(t *testing.T) {
+	tt := []struct {


I would prefer to see tests that exercise the trusted range functionality.

This was referenced Dec 15, 2024

Add more sophisticated realip middleware #708

Open

Problems with use of X-Forwarded-For and other headers to get the "real" client IP #711

Open

RealIP sets r.RemoteAddr without port #453

Open

feat(): add CF-Connecting-IP #908

Merged

convto approved these changes Dec 16, 2024

View reviewed changes

ClientIP middleware proposal, intended to replace RealIP

4b70ef1

VojtechVitek force-pushed the clientip-middleware-intended-to-replace branch from c2354ea to 4b70ef1 Compare January 20, 2025 20:46

c2h5oh suggested changes Jan 20, 2025

View reviewed changes

adam-p suggested changes Feb 9, 2025

View reviewed changes

mschwager mentioned this pull request Feb 20, 2025

KeyByRealIP is susceptible to spoofing go-chi/httprate#53

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ClientIP middleware proposal, intended to replace RealIP #967

ClientIP middleware proposal, intended to replace RealIP #967

VojtechVitek commented Dec 15, 2024 •

edited

Loading

convto left a comment

VojtechVitek commented Jan 20, 2025 •

edited

Loading

c2h5oh Jan 20, 2025 •

edited

Loading

adam-p Feb 9, 2025

c2h5oh Jan 20, 2025

adam-p Feb 9, 2025

adam-p Feb 9, 2025

adam-p Feb 9, 2025

adam-p Feb 9, 2025

adam-p Feb 9, 2025

adam-p Feb 9, 2025

ClientIP middleware proposal, intended to replace RealIP #967

Are you sure you want to change the base?

ClientIP middleware proposal, intended to replace RealIP #967

Conversation

VojtechVitek commented Dec 15, 2024 • edited Loading

convto left a comment

Choose a reason for hiding this comment

VojtechVitek commented Jan 20, 2025 • edited Loading

c2h5oh Jan 20, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VojtechVitek commented Dec 15, 2024 •

edited

Loading

VojtechVitek commented Jan 20, 2025 •

edited

Loading

c2h5oh Jan 20, 2025 •

edited

Loading