Bucket IPv6 rate-limit keys by /64 (closes #125)

Rate-limit keys were per-address, i.e. per /128 for IPv6. A routed /64
is the normal residential and mobile IPv6 allocation, so a client could
rotate source addresses inside its own prefix and mint a fresh bucket
per request, evading every limiter here at the network layer with no
spoofing and nothing to detect.

The shared key function now reduces the client address to a bucket by
family: IPv4 keys on the full address, IPv6 on its /64 prefix. All four
limiter instances (login, password change, and the receiver's
per-entrypoint and aggregate limits) go through that one function, so
all of them are covered.

Both branches of that function are covered by tests: the direct-peer
branch, and the trusted-proxy branch that takes the client address out
of X-Forwarded-For. The second is the one a production deployment
takes, since it is required to run behind a reverse proxy with
TRUSTED_PROXIES set.

IPv4-mapped addresses (::ffff:1.2.3.4) key as the IPv4 address they
carry rather than being masked, which would otherwise collapse every
IPv4 client behind a mapping proxy into the ::ffff:0:0/96 bucket. An
unparseable RemoteAddr still keys on its raw value, so those stay in
distinct buckets instead of collapsing together.

No new configuration surface.

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",
+	)
+}