From c3b6623be1065348632c3ac88e739248ee2567da Mon Sep 17 00:00:00 2001 From: clawbot Date: Mon, 17 Aug 2026 23:52:15 +0200 Subject: [PATCH] Bucket IPv6 rate-limit keys by /64 (closes #125) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rate-limit keys were per-address, i.e. per /128 for IPv6. A routed /64 is the normal residential and mobile allocation, so a client rotated source addresses inside its own prefix and minted a fresh bucket per request — evading every limiter at the network layer, with no spoofing and nothing to detect. #88 closed the header half of this control; this is the network half. IPv6 now keys on the /64, IPv4 on the full address, via stdlib net/netip. IPv4-mapped form is unmapped rather than masked, so clients behind a mapping proxy do not collapse into one bucket. Independently reviewed twice. The first round found the trusted-proxy forwarded path — the one carrying production traffic — had no coverage at all, so a silent revert there was undetectable; that is now pinned. The reviewer confirmed both branches are independently mutation-tested: reverting either the direct-peer return or the forwarded return alone fails only that branch's tests. The 18-site test-constant refactor was verified byte-identical against next, with no pre-existing assertion changed. Known remaining coverage gap, judged not a defect: the fallback when the peer is trusted but the forwarded address does not parse has no test. Only operator-controlled addresses inside TRUSTED_PROXIES reach it, they already share the proxy's single bucket, and masking there can only merge operator proxies — fail-closed, nothing attacker-controlled. --- README.md | 9 +- internal/middleware/ratelimit.go | 59 +++- internal/middleware/ratelimit_test.go | 415 +++++++++++++++++++++++++- 3 files changed, 463 insertions(+), 20 deletions(-) diff --git a/README.md b/README.md index 936b002..b1b0e44 100644 --- a/README.md +++ b/README.md @@ -1001,7 +1001,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..0de9fd1 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 errors only on a negative bit count, on over 32 bits + // for an IPv4 address, or on over 128 for IPv6. The count here + // is the constant 64 and the IPv4 case returned above, so the + // error is unreachable. (The zero Addr does not error either: it + // yields the zero Prefix. Neither call site can produce one, + // since both parse the address first.) + prefix, _ := addr.Prefix(ipv6BucketBits) + + 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..2d22a3f 100644 --- a/internal/middleware/ratelimit_test.go +++ b/internal/middleware/ratelimit_test.go @@ -15,6 +15,7 @@ import ( "time" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "sneak.berlin/go/webhooker/internal/config" "sneak.berlin/go/webhooker/internal/middleware" ) @@ -370,6 +371,30 @@ 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" + + // clientIPv6 and clientIPv6Same are two addresses inside one + // routed /64, so both must key on clientBucketV6. + // clientIPv6Other is a different allocation and must key on + // clientOtherBucketV6. + clientIPv6 = "2001:db8:1:2:3:4:5:6" + clientIPv6Same = "2001:db8:1:2:aaaa:bbbb:cccc:dddd" + clientIPv6Other = "2001:db8:1:3::1" + clientBucketV6 = "2001:db8:1:2::/64" + clientOtherBucketV6 = "2001:db8:1:3::/64" + + // trustedProxyCIDR is the proxy network the forwarded-path + // tests configure, and trustedPeer an address inside it. A + // production deployment is required to run behind a reverse + // proxy with TRUSTED_PROXIES set, so this is the shape the + // bucketing has to hold in. + trustedProxyCIDR = "10.0.0.0/8" + trustedPeer = "10.0.0.1:44444" ) // assertSharedBucket drives the login limiter from peer with the @@ -458,8 +483,8 @@ func TestRateLimitKey_SingleValuedHeadersIgnoredFromTrustedPeer( t.Parallel() assertSharedBucket( - t, trustedProxies("10.0.0.0/8"), - "10.0.0.1:44444", + t, trustedProxies(trustedProxyCIDR), + trustedPeer, func(i int) map[string]string { return map[string]string{ header: fmt.Sprintf( @@ -495,8 +520,8 @@ func TestRateLimitKey_MalformedRightmostHopFallsBackToPeer( t.Parallel() assertSharedBucket( - t, trustedProxies("10.0.0.0/8"), - "10.0.0.1:44444", + t, trustedProxies(trustedProxyCIDR), + trustedPeer, func(i int) map[string]string { return map[string]string{ headerXFF: fmt.Sprintf( @@ -522,13 +547,13 @@ func TestRateLimitKey_ForwardedHonouredFromTrustedPeer( t.Parallel() m := rateLimitMiddleware(t, &config.Config{ - TrustedProxies: trustedProxies("10.0.0.0/8"), + TrustedProxies: trustedProxies(trustedProxyCIDR), }) handler := m.LoginRateLimit()(okHandler()) - const peer = "10.0.0.1:44444" + const peer = trustedPeer - 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 +567,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, @@ -559,7 +584,7 @@ func TestRateLimitKey_ChainWalkSkipsClientPrepended(t *testing.T) { t.Parallel() assertSharedBucket( - t, trustedProxies("10.0.0.0/8"), "10.0.0.1:44444", + t, trustedProxies(trustedProxyCIDR), trustedPeer, func(i int) map[string]string { return map[string]string{ headerXFF: fmt.Sprintf( @@ -594,7 +619,7 @@ func TestRateLimitKey_LongChainCapsWalkAndFallsBackToPeer( start := time.Now() assertSharedBucket( - t, trustedProxies("10.0.0.0/8"), "10.0.0.1:44444", + t, trustedProxies(trustedProxyCIDR), trustedPeer, func(i int) map[string]string { return map[string]string{ headerXFF: fmt.Sprintf("9.9.9.%d%s", i+1, padding), @@ -633,13 +658,13 @@ func TestRateLimitKey_LongChainAllocationIsBounded(t *testing.T) { ) m := rateLimitMiddleware(t, &config.Config{ - TrustedProxies: trustedProxies("10.0.0.0/8"), + TrustedProxies: trustedProxies(trustedProxyCIDR), }) req := httptest.NewRequestWithContext( context.Background(), http.MethodPost, loginPath, nil, ) - req.RemoteAddr = "10.0.0.1:44444" + req.RemoteAddr = trustedPeer req.Header.Set( headerXFF, "9.9.9.9"+strings.Repeat(", 10.0.0.2", hops), ) @@ -835,3 +860,369 @@ 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: "[" + clientIPv6 + "]:44444", + want: clientBucketV6, + about: "an IPv6 peer must key on its /64", + }, { + name: "ipv6-other-in-same-64", + peer: "[" + clientIPv6Same + "]:1", + want: clientBucketV6, + about: "another address in the same /64 must key the same", + }, { + name: "ipv6-different-64", + peer: "[" + clientIPv6Other + "]:44444", + want: clientOtherBucketV6, + 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 pins the structure the +// no-collision property rests on, rather than one sample pair: every +// IPv4 key is a bare address and every IPv6 key is a /64 in CIDR +// form, so the two name spaces are disjoint by shape. Dropping the +// masking strips the suffix that guarantees it, which is why this +// asserts the form of each key and not just that two of them differ. +func TestRateLimitKey_FamiliesDoNotCollide(t *testing.T) { + t.Parallel() + + // Restated here rather than imported from the package under + // test, so that changing the production bucket width fails this + // test instead of silently moving with it. + const wantBits = 64 + + m := rateLimitMiddleware(t, &config.Config{}) + + v4Keys := map[string]bool{} + + for _, peer := range []string{ + clientIPv4 + ":44444", + clientIPv4Alt + ":44444", + "[::ffff:" + clientIPv4 + "]:44444", + } { + key := clientKeyFor(t, m, peer) + + addr, err := netip.ParseAddr(key) + require.NoError( + t, err, "%s: an IPv4 key must be a bare address", peer, + ) + assert.True( + t, addr.Is4(), + "%s: an IPv4 key must be a dotted quad, got %q", peer, key, + ) + + v4Keys[key] = true + } + + for _, peer := range []string{ + "[" + clientIPv6 + "]:44444", + "[" + clientIPv6Same + "]:44444", + "[" + clientIPv6Other + "]:44444", + "[2001:db8::" + clientIPv4 + "]:44444", + } { + key := clientKeyFor(t, m, peer) + + prefix, err := netip.ParsePrefix(key) + require.NoError( + t, err, "%s: an IPv6 key must be a CIDR prefix", peer, + ) + assert.Equal( + t, wantBits, prefix.Bits(), + "%s: an IPv6 key must name a /64", peer, + ) + assert.False( + t, v4Keys[key], + "%s: an IPv6 key must never equal an IPv4 key", peer, + ) + } +} + +// 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", + ) +} + +// forwardedKeyFor returns the bucket key m computes for a request +// that arrives from trustedPeer — a configured trusted proxy — and +// names forwarded as its client in X-Forwarded-For. That is the +// production path: a deployment is required to run behind a reverse +// proxy with TRUSTED_PROXIES set, so the forwarded address, not the +// peer, is what the limiters bucket on there. +func forwardedKeyFor( + t *testing.T, m *middleware.Middleware, forwarded string, +) string { + t.Helper() + + req := httptest.NewRequestWithContext( + context.Background(), http.MethodPost, loginPath, nil, + ) + req.RemoteAddr = trustedPeer + req.Header.Set(headerXFF, forwarded) + + return middleware.ClientKeyForTest(m, req) +} + +// TestRateLimitKey_ForwardedIPv6BucketsByPrefix pins the /64 +// bucketing on the trusted-proxy branch. The direct-peer tests above +// cannot reach it, so without this the masking could be reverted for +// forwarded clients alone — the only shape a production deployment +// runs in — and the rest of the suite would stay green. +func TestRateLimitKey_ForwardedIPv6BucketsByPrefix(t *testing.T) { + t.Parallel() + + m := rateLimitMiddleware(t, &config.Config{ + TrustedProxies: trustedProxies(trustedProxyCIDR), + }) + + for _, tc := range []struct { + name string + forwarded string + want string + about string + }{{ + name: "ipv6", + forwarded: clientIPv6, + want: clientBucketV6, + about: "a forwarded IPv6 client must key on its /64", + }, { + name: "ipv6-other-in-same-64", + forwarded: clientIPv6Same, + want: clientBucketV6, + about: "another forwarded address in the same /64 must " + + "key the same", + }, { + name: "ipv6-different-64", + forwarded: clientIPv6Other, + want: clientOtherBucketV6, + about: "a forwarded address in another /64 must differ", + }, { + name: "ipv4", + forwarded: clientIPv4, + want: clientIPv4, + about: "a forwarded IPv4 client must key on the address", + }, { + name: "ipv4-mapped", + forwarded: "::ffff:" + clientIPv4, + want: clientIPv4, + about: "a proxy that forwards IPv4-mapped form must key as " + + "the IPv4 address it carries, not be masked to a /64: " + + "mapped addresses all share ::ffff:0:0/96", + }} { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + assert.Equal( + t, tc.want, + forwardedKeyFor(t, m, tc.forwarded), tc.about, + ) + }) + } +} + +// TestLoginRateLimit_ForwardedIPv6SharesBucketWithinSlash64 is the +// behavioural half on the production path: behind a trusted proxy, a +// client rotating source addresses inside its own routed /64 must +// stay in one bucket. +func TestLoginRateLimit_ForwardedIPv6SharesBucketWithinSlash64( + t *testing.T, +) { + t.Parallel() + + assertSharedBucket( + t, trustedProxies(trustedProxyCIDR), trustedPeer, + func(i int) map[string]string { + return map[string]string{ + headerXFF: fmt.Sprintf("2001:db8:1:2::%d", i+1), + } + }, + "rotating forwarded source addresses inside one routed /64 "+ + "must not mint fresh buckets", + ) +} + +// TestLoginRateLimit_ForwardedIPv6IndependentAcrossSlash64 is the +// other side of that trade on the same path: bucketing by /64 must +// not merge two allocations reaching the proxy. +func TestLoginRateLimit_ForwardedIPv6IndependentAcrossSlash64( + t *testing.T, +) { + t.Parallel() + + m := rateLimitMiddleware(t, &config.Config{ + TrustedProxies: trustedProxies(trustedProxyCIDR), + }) + handler := m.LoginRateLimit()(okHandler()) + + spent := map[string]string{headerXFF: clientIPv6} + for range middleware.LoginRateLimitConst + 1 { + postWithHeaders(handler, trustedPeer, loginPath, spent) + } + + w := postWithHeaders( + handler, trustedPeer, loginPath, + map[string]string{headerXFF: clientIPv6Other}, + ) + assert.Equal( + t, http.StatusOK, w.Code, + "a forwarded client in a different /64 must have its own "+ + "bucket", + ) +}