diff --git a/README.md b/README.md index 570f087..124d2ea 100644 --- a/README.md +++ b/README.md @@ -911,7 +911,14 @@ Every limiter here — receiver, login, and password change — identifies the client the same way, through one shared key function: the connection's own address, unless the peer is listed in `TRUSTED_PROXIES`, in which case the forwarded client address is used -instead. See [Trusted proxies](#trusted-proxies). Deployed without that +instead. That address becomes a bucket by family: IPv4 keys on the full +address, IPv6 on its `/64` prefix. A routed `/64` is the normal +residential and mobile IPv6 allocation, so keying IPv6 per address would +let one subscriber rotate source addresses and mint a fresh bucket per +request, evading these limits at the network layer without spoofing +anything; the cost is that distinct clients inside one `/64` share a +bucket. IPv4-mapped addresses (`::ffff:1.2.3.4`) key as the IPv4 address +they carry. See [Trusted proxies](#trusted-proxies). Deployed without that variable set, a client behind a reverse proxy shares one bucket with every other client behind the same proxy. Set `TRUSTED_PROXIES` to the proxy's address to get per-client limits back. What the shared bucket diff --git a/internal/middleware/ratelimit.go b/internal/middleware/ratelimit.go index c63b7c1..2ffcd36 100644 --- a/internal/middleware/ratelimit.go +++ b/internal/middleware/ratelimit.go @@ -48,6 +48,12 @@ const ( // bound every request pays a walk proportional to whatever the // client sent. maxForwardedHops = 64 + + // ipv6BucketBits is the prefix length IPv6 clients are bucketed + // on. A routed /64 is the normal residential and mobile + // allocation, so it is the unit an attacker gets addresses in + // and therefore the unit worth limiting. + ipv6BucketBits = 64 ) // normalizeAddr strips the IPv4-in-IPv6 wrapper and any zone from @@ -56,6 +62,40 @@ func normalizeAddr(addr netip.Addr) netip.Addr { return addr.Unmap().WithZone("") } +// bucketKey is the rate-limit bucket identity of a client address. +// IPv4 keys on the full address; IPv6 keys on its /64 prefix, +// because keying IPv6 per /128 lets one ordinary subscriber rotate +// source addresses inside its own routed /64 and mint a fresh bucket +// per request — evading every limiter here at the network layer, +// with no spoofing and nothing to detect. +// +// An IPv4-mapped address (::ffff:1.2.3.4) is keyed as the IPv4 +// address it carries, never masked to a /64: mapped form all shares +// the ::ffff:0:0/96 prefix, so masking would collapse every IPv4 +// client reaching a proxy that emits it into one bucket. Callers +// pass addresses through normalizeAddr, which already unmaps; the +// unmap here keeps the property true of the key function itself. +// +// The two families cannot collide: an IPv4 key is a bare dotted +// quad, and an IPv6 key always carries a "/64" suffix. +func bucketKey(addr netip.Addr) string { + addr = addr.Unmap() + + if addr.Is4() { + return addr.String() + } + + prefix, err := addr.Prefix(ipv6BucketBits) + if err != nil { + // Only reachable for an address shorter than 64 bits, + // i.e. the zero Addr. Key on the address itself rather + // than on a shared sentinel. + return addr.String() + } + + return prefix.String() +} + // isTrustedProxy reports whether addr belongs to a network the // operator listed in TRUSTED_PROXIES. The list is empty by default, // so by default nothing is trusted. @@ -143,6 +183,9 @@ func (m *Middleware) forwardedClientAddr( // another client's bucket, by picking an X-Forwarded-For value — // which makes every limit here decorative against a deliberate // attacker. +// +// The address that identifies the client is then reduced to a bucket +// by bucketKey: full address for IPv4, /64 prefix for IPv6. func (m *Middleware) rateLimitKey(r *http.Request) (string, error) { return m.clientKey(r), nil } @@ -152,23 +195,25 @@ func (m *Middleware) clientKey(r *http.Request) string { peer, err := netip.ParseAddr(ipFromHostPort(r.RemoteAddr)) if err != nil { // Not an address we can reason about; key on the raw - // value, the most specific identity left. On a - // Unix-socket listener every peer carries the same - // RemoteAddr and so shares one bucket, which is the - // fail-closed direction. + // value, the most specific identity left. Distinct + // RemoteAddr values stay in distinct buckets, so this + // path cannot silently collapse unrelated clients + // together. On a Unix-socket listener every peer + // carries the same RemoteAddr and so shares one bucket, + // which is the fail-closed direction. return r.RemoteAddr } peer = normalizeAddr(peer) if !m.isTrustedProxy(peer) { - return peer.String() + return bucketKey(peer) } if addr, ok := m.forwardedClientAddr(r); ok { - return addr.String() + return bucketKey(addr) } - return peer.String() + return bucketKey(peer) } // tooManyRequests returns the 429 handler used by the login, diff --git a/internal/middleware/ratelimit_test.go b/internal/middleware/ratelimit_test.go index 9569161..0958435 100644 --- a/internal/middleware/ratelimit_test.go +++ b/internal/middleware/ratelimit_test.go @@ -370,6 +370,12 @@ const ( headerXFF = "X-Forwarded-For" headerReal = "X-Real-IP" headerTrue = "True-Client-IP" + + // clientIPv4 is the sample IPv4 client address these tests key + // on, both directly and in IPv4-mapped form. clientIPv4Alt is + // its neighbour, used to show the two do not share a bucket. + clientIPv4 = "198.51.100.7" + clientIPv4Alt = "198.51.100.8" ) // assertSharedBucket drives the login limiter from peer with the @@ -528,7 +534,7 @@ func TestRateLimitKey_ForwardedHonouredFromTrustedPeer( const peer = "10.0.0.1:44444" - first := map[string]string{headerXFF: "198.51.100.7"} + first := map[string]string{headerXFF: clientIPv4} for range middleware.LoginRateLimitConst { postWithHeaders(handler, peer, loginPath, first) @@ -542,7 +548,7 @@ func TestRateLimitKey_ForwardedHonouredFromTrustedPeer( w = postWithHeaders( handler, peer, loginPath, - map[string]string{headerXFF: "198.51.100.8"}, + map[string]string{headerXFF: clientIPv4Alt}, ) assert.Equal( t, http.StatusOK, w.Code, @@ -835,3 +841,198 @@ func TestReceiverRateLimit_IgnoresForwardedFromUntrustedPeer( "not mint a fresh receiver bucket", ) } + +// clientKeyFor returns the bucket key m computes for a request whose +// direct peer is remoteAddr and which carries no forwarded headers. +func clientKeyFor( + t *testing.T, m *middleware.Middleware, remoteAddr string, +) string { + t.Helper() + + req := httptest.NewRequestWithContext( + context.Background(), http.MethodPost, loginPath, nil, + ) + req.RemoteAddr = remoteAddr + + return middleware.ClientKeyForTest(m, req) +} + +// TestRateLimitKey_IPv6BucketsByPrefix pins the key function's +// address-family behaviour. IPv6 clients must bucket by /64 — a +// routed /64 is the normal residential and mobile allocation, so +// per-/128 keying lets one subscriber rotate source addresses and +// mint a fresh bucket per request — while IPv4 keeps keying on the +// full address and IPv4-mapped form is keyed as the IPv4 address it +// carries. +func TestRateLimitKey_IPv6BucketsByPrefix(t *testing.T) { + t.Parallel() + + m := rateLimitMiddleware(t, &config.Config{}) + + for _, tc := range []struct { + name string + peer string + want string + about string + }{{ + name: "ipv6", + peer: "[2001:db8:1:2:3:4:5:6]:44444", + want: "2001:db8:1:2::/64", + about: "an IPv6 peer must key on its /64", + }, { + name: "ipv6-other-in-same-64", + peer: "[2001:db8:1:2:aaaa:bbbb:cccc:dddd]:1", + want: "2001:db8:1:2::/64", + about: "another address in the same /64 must key the same", + }, { + name: "ipv6-different-64", + peer: "[2001:db8:1:3::1]:44444", + want: "2001:db8:1:3::/64", + about: "a different /64 must key differently", + }, { + name: "ipv4", + peer: clientIPv4 + ":44444", + want: clientIPv4, + about: "IPv4 must keep keying on the full address", + }, { + name: "ipv4-neighbour", + peer: clientIPv4Alt + ":44444", + want: clientIPv4Alt, + about: "adjacent IPv4 addresses must not share a bucket", + }, { + name: "ipv4-mapped", + peer: "[::ffff:" + clientIPv4 + "]:44444", + want: clientIPv4, + about: "IPv4-mapped form must key as the IPv4 address, " + + "not be masked to a /64: mapped addresses all share " + + "::ffff:0:0/96, so masking would collapse every IPv4 " + + "client behind a mapping proxy into one bucket", + }} { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + assert.Equal( + t, tc.want, clientKeyFor(t, m, tc.peer), tc.about, + ) + }) + } +} + +// TestRateLimitKey_FamiliesDoNotCollide checks the property the /64 +// masking must not break: an IPv4 key and an IPv6 key can never name +// the same bucket, whatever the addresses. +func TestRateLimitKey_FamiliesDoNotCollide(t *testing.T) { + t.Parallel() + + m := rateLimitMiddleware(t, &config.Config{}) + + assert.NotEqual( + t, + clientKeyFor(t, m, clientIPv4+":44444"), + clientKeyFor(t, m, "[2001:db8::"+clientIPv4+"]:44444"), + "an IPv4 key must never equal an IPv6 key", + ) +} + +// TestRateLimitKey_UnparseablePeerKeepsDistinctBuckets covers the +// fallback path. A RemoteAddr that is not an address must not panic, +// and must not drop unrelated clients into one shared bucket by +// accident: the raw value is the most specific identity left, so +// distinct values stay in distinct buckets. +func TestRateLimitKey_UnparseablePeerKeepsDistinctBuckets( + t *testing.T, +) { + t.Parallel() + + m := rateLimitMiddleware(t, &config.Config{}) + + first := clientKeyFor(t, m, "not-an-address") + second := clientKeyFor(t, m, "also-not-an-address:1234") + + assert.NotEmpty(t, first) + assert.NotEqual( + t, first, second, + "unparseable peers must not collapse into one bucket", + ) +} + +// TestLoginRateLimit_IPv6SharesBucketWithinSlash64 is the behavioural +// half, and the regression test for the bypass itself: a client that +// rotates source addresses inside its own routed /64 must stay in one +// bucket. Reverting the masking makes this test fail, because each +// rotated address would mint a fresh bucket and nothing would be +// rejected. +func TestLoginRateLimit_IPv6SharesBucketWithinSlash64(t *testing.T) { + t.Parallel() + + m := rateLimitMiddleware(t, &config.Config{}) + handler := m.LoginRateLimit()(okHandler()) + + for i := range middleware.LoginRateLimitConst { + w := postWithHeaders( + handler, + fmt.Sprintf("[2001:db8:1:2::%d]:44444", i+1), + loginPath, nil, + ) + assert.Equal( + t, http.StatusOK, w.Code, "request %d should pass", i, + ) + } + + w := postWithHeaders( + handler, "[2001:db8:1:2::ffff]:44444", loginPath, nil, + ) + assert.Equal( + t, http.StatusTooManyRequests, w.Code, + "rotating source addresses inside one routed /64 must not "+ + "mint fresh buckets", + ) +} + +// TestLoginRateLimit_IPv6IndependentAcrossSlash64 is the other side +// of the trade: bucketing by /64 must not merge separate allocations, +// so a client in a different /64 keeps its own limit. +func TestLoginRateLimit_IPv6IndependentAcrossSlash64(t *testing.T) { + t.Parallel() + + m := rateLimitMiddleware(t, &config.Config{}) + handler := m.LoginRateLimit()(okHandler()) + + for range middleware.LoginRateLimitConst + 1 { + postWithHeaders( + handler, "[2001:db8:1:2::1]:44444", loginPath, nil, + ) + } + + w := postWithHeaders( + handler, "[2001:db8:1:3::1]:44444", loginPath, nil, + ) + assert.Equal( + t, http.StatusOK, w.Code, + "a different /64 must have its own bucket", + ) +} + +// TestLoginRateLimit_IPv4IndependentPerAddress guards against the +// masking leaking into IPv4: two addresses one apart must still hold +// separate buckets. +func TestLoginRateLimit_IPv4IndependentPerAddress(t *testing.T) { + t.Parallel() + + m := rateLimitMiddleware(t, &config.Config{}) + handler := m.LoginRateLimit()(okHandler()) + + for range middleware.LoginRateLimitConst + 1 { + postWithHeaders( + handler, clientIPv4+":44444", loginPath, nil, + ) + } + + w := postWithHeaders( + handler, clientIPv4Alt+":44444", loginPath, nil, + ) + assert.Equal( + t, http.StatusOK, w.Code, + "a second IPv4 address must have its own bucket", + ) +}