Bucket IPv6 rate-limit keys by /64 (closes #125) #162
Reference in New Issue
Block a user
Delete Branch "issue-125-ipv6-prefix-buckets"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #125 — option 1 as ruled on the issue. IPv6 buckets by
/64, IPv4 unchanged, no new configuration surface.Why
Rate-limit keys were per-address, i.e. per
/128for IPv6. A routed/64is 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 the limiters at the network layer with no spoofing and nothing to detect. #88 closed the header half of this control; this is the network half.What changed
internal/middleware/ratelimit.gogainsbucketKey(netip.Addr) string, applied at the three return points ofclientKey. IPv4 keys on the full address; IPv6 keys onaddr.Prefix(64). Stdlibnet/netiponly, which the file already used — no new dependency, no hand-rolled byte masking.All three limiters share the key function — confirmed in the code
Every limiter instance is constructed with
httprate.WithKeyFuncs(m.rateLimitKey), andrateLimitKeydelegates toclientKey:LoginRateLimitandPasswordChangeRateLimitboth build throughpostRateLimit, which passesm.rateLimitKey.ReceiverRateLimitbuilds two limiters, and both passm.rateLimitKey: the per-entrypoint one asWithKeyFuncs(m.rateLimitKey, httprate.KeyByEndpoint), the aggregate one asWithKeyFuncs(m.rateLimitKey).That is four limiter instances across the three limiters, and
m.rateLimitKeyis the only key function any of them names — there is no other call site.Both branches of the key function
clientKeyhas two live branches: the direct-peer branch, and the trusted-proxy branch that takes the client address out ofX-Forwarded-For.README.mdrequires a production deployment to run behind a reverse proxy withTRUSTED_PROXIESset, so the forwarded branch is the one that carries production traffic. Both are masked, and both are now pinned by tests that fail when their own branch alone is reverted.IPv4-mapped addresses
::ffff:1.2.3.4keys as1.2.3.4, never masked. Mapped form all sits inside::ffff:0:0/96, so masking it to a/64would collapse every IPv4 client behind a proxy that emits mapped form into one shared bucket.clientKeyalready ran addresses throughnormalizeAddr(which unmaps);bucketKeycallsUnmap()again so the property holds of the key function itself rather than depending on its caller. Covered on both branches.Malformed addresses
Unchanged behaviour, deliberately preserved and now tested. An unparseable
RemoteAddrreturns the raw value, which is the most specific identity left — distinct values stay in distinct buckets, so this path cannot silently collapse unrelated clients together. (A Unix-socket listener, where every peer carries the sameRemoteAddr, still shares one bucket; that is the fail-closed direction and predates this change.)Addr.Prefixcannot error here: it is fallible only for a negative bit count, over 32 bits on an IPv4 address, or over 128 on IPv6, and the count is the constant 64 with the IPv4 case already returned above. The error is therefore discarded rather than branched on, with a comment stating why. The zeroAddrdoes not error either — it yields the zeroPrefix— and neither call site can produce one, since both parse the address first.Key collisions between families
Not possible. An IPv4 key is a bare dotted quad; an IPv6 key always carries a
/64suffix.TestRateLimitKey_FamiliesDoNotCollideasserts that structurally: every IPv4 key must parse as a barenetip.Addrand every IPv6 key as anetip.Prefixof 64 bits, with the two key sets disjoint.Tests
internal/middleware/ratelimit_test.go.Direct-peer branch:
TestRateLimitKey_IPv6BucketsByPrefix— table over the key function: two addresses in the same/64key identically, a different/64keys differently, two distinct IPv4 addresses key differently, and IPv4-mapped keys as its plain IPv4 form.TestLoginRateLimit_IPv6SharesBucketWithinSlash64— the behavioural regression test, driving a real limiter: rotating the source address on every request inside one/64still gets a 429.TestLoginRateLimit_IPv6IndependentAcrossSlash64— a different/64keeps its own limit.TestLoginRateLimit_IPv4IndependentPerAddress— no IPv4 regression; neighbouring addresses hold separate buckets.Trusted-proxy forwarded branch, i.e. the production shape:
TestRateLimitKey_ForwardedIPv6BucketsByPrefix— same table, but withTRUSTED_PROXIESset to10.0.0.0/8, the peer inside it, and the client named inX-Forwarded-For. Same-/64, different-/64, IPv4 and IPv4-mapped cases; a mapping proxy is exactly where mapped form shows up.TestLoginRateLimit_ForwardedIPv6SharesBucketWithinSlash64— behavioural: rotating the forwarded address inside one/64still gets a 429.TestLoginRateLimit_ForwardedIPv6IndependentAcrossSlash64— a different forwarded/64keeps its own limit.Plus
TestRateLimitKey_FamiliesDoNotCollideandTestRateLimitKey_UnparseablePeerKeepsDistinctBuckets.Test-only refactor: the IPv6 sample addresses, their expected
/64keys, and the trusted-proxy CIDR and peer are now named constants (clientIPv6,clientIPv6Same,clientIPv6Other,clientBucketV6,clientOtherBucketV6,trustedProxyCIDR,trustedPeer), substituted byte-identically at their existing use sites: six for the CIDR, six for the peer, and six in the direct-peer IPv6 table. No assertion changed. This keeps the file consistent now that the same values are used on both branches, and keepsgoconstoff it.Mutation verification
Both mutations were run, and reverted afterwards.
Forwarded branch alone.
internal/middleware/ratelimit.go:213reverted fromreturn bucketKey(addr)toreturn addr.String(), withipv6BucketBitsleft at 64.docker build --target builder --no-cache-filter=builder .exits 1:The direct-peer tests correctly all still pass under it, which is the point: this mutation is only visible to the new forwarded tests. The IPv4 and IPv4-mapped forwarded cases also still pass, since neither is masked.
Bucket width.
ipv6BucketBitsflipped from 64 to 128,make test:TestRateLimitKey_FamiliesDoNotCollideis in that list because it now asserts the shape of each key against a bucket width restated in the test rather than imported from the package under test. In its previous single-NotEqualform it survived this mutation.README
Surgical edit to the Rate Limiting section only — one inserted passage in the paragraph describing the shared key function, which previously implied per-address keying. It now states the per-family bucketing, why
/64is the unit, the cost (distinct clients inside one/64share a bucket), and the IPv4-mapped rule. No other part ofREADME.mdwas touched, and no reflow, since other units are editing the file concurrently.make fmtis Go-only in this repo and formats no markdown; the inserted lines are hand-wrapped to the file's existing width.Verification
Rebased onto
nextatc378690; the gates below were run after that rebase.docker build --no-cache-filter=lint --no-cache-filter=builder .— exit 0, checks demonstrably executed rather than replayed:RUN make lintDONE 55.9s,0 issues.RUN make testDONE 56.8s, zero(cached)markers anywhere in the log; real per-package durations, e.g.ok sneak.berlin/go/webhooker/internal/middleware 1.109s,ok .../internal/delivery 4.216s,ok .../internal/handlers 4.097s.make fmt-checkclean.make checkruns those same three targets and also exits 0 on the host, run with an isolatedGOLANGCI_LINT_CACHEso no shared cache could contribute; the Docker run above is the authoritative lint result.Not in this PR:
golangci-lintemits a deprecation warning for thegomodguardlinter (replaced bygomodguard_v2in v2.12.0). Pre-existing and unrelated.Also not in this PR, and filed rather than fixed here: an empty
RemoteAddrkeys as"", so all such peers share one bucket. Pre-existing and fail-closed. #168FAIL — needs-rework
Independent review of head
0946316. The fix itself is correct and reaches all four limiter instances; two defects below, both in the same file.1. The trusted-proxy forwarded path is unpinned by any test
internal/middleware/ratelimit.go:213—return bucketKey(addr)on theforwardedClientAddrbranch is correct, but nothing asserts it. I mutated exactly that line back toreturn addr.String(), leftipv6BucketBitsat 64, and rebuilt the builder stage: exit 0,ok sneak.berlin/go/webhooker/internal/middleware 1.085s, whole suite green. A silent revert of the fix on that branch is undetectable.Why it matters: every new test uses
&config.Config{}(no trusted proxies) and sets no headers, so all six exercise only the direct-peer branch at line 209.README.mdstates a production deployment is required to run behind a reverse proxy withTRUSTED_PROXIESset — that is the branch at line 213, i.e. the deployment shape where this bypass actually bites is the one with zero coverage. #125's done-criteria ("applies to all three limiters") is met in code, but the mutation-resistance the rest of this PR demonstrates does not extend here.Acceptable: one test with
TrustedProxiesset (e.g.10.0.0.0/8), peer10.0.0.1:44444, and rotating IPv6 client addresses inside one/64supplied viaX-Forwarded-For, asserting a shared bucket — plus the different-/64-is-independent counterpart. It must fail when line 213 is reverted toaddr.String().2.
internal/middleware/ratelimit.go:88-94— unreachable branch whose comment states a falsehoodnetip.Addr.Prefixreturns a non-nil error in exactly three cases:bnegative;bover 32 on az4address;bover 128 onz6. Herebis the constant 64, and the IPv4 case is already returned above by theIs4()check, soerris always nil — the branch is dead.The comment is also wrong about its own stated case. For the zero
Addr(z0),PrefixreturnsPrefix{}, nil— it does not error — so control reachesreturn prefix.String(), which is the literal"invalid Prefix". The stated remedy ("key on the address itself rather than on a shared sentinel") therefore never runs, and the zeroAddrdoes key on a shared sentinel. This is not exploitable, because no zeroAddrcan reachbucketKey(both call sites derive from a successfulnetip.ParseAddr), but a security-relevant function should not carry a comment asserting stdlib behaviour that is the opposite of what the stdlib does.Acceptable: drop the dead branch, or keep it and correct the comment to say the error is unreachable for a fixed 64-bit prefix on a non-IPv4 address.
Notes (not defects, not blocking)
TestRateLimitKey_FamiliesDoNotCollideis a singleNotEqualpair and passed under every mutation I ran. It documents the property rather than testing it. The property does hold structurally (bare dotted quad vs. a/64-suffixed string).RemoteAddrkeys as"", so all such peers share one bucket:ipFromHostPortreturns""on aSplitHostPorterror and the fallback then returns the emptyRemoteAddr. Pre-existing, fail-closed, untested. Recording it as a known gap, not as a defect of this PR.RemoteAddrwith no port falls to the raw-value key, which is byte-identical to the IPv4 bucket key for the same address. Pre-existing merge, harmless direction.normalizeAddr, andnetip.PrefixFromstrips zone regardless, so%eth0cannot fragment keys.Verified clean
All four limiter instances (
postRateLimitx2, receiver per-entrypoint and aggregate) namem.rateLimitKey;WithKeyFuncsappears at exactly three sites and nowhere else. Aggregate is outermost inaggregate(perEntrypoint(next)), so it still gates the inner limiter's key space. IPv4-mapped keys as the plain IPv4 address, covered and correct.clientIPv4/clientIPv4Altrefactor touched only lines 537 and 551 with byte-identical values — no pre-existing assertion changed. One commit, title ends(closes #125), basenext,TODO.mdandgo.moduntouched, stdlibnet/netiponly, no Claude/Anthropic references or attribution trailers. Merges cleanly ontonextat279effb. README edit is surgical and accurate.make fmtis Go-only in this repo, so the hand-wrapped markdown is fine.Gate evidence (run here, in Docker)
docker build --no-cache-filter=lint --no-cache-filter=builder .— exit 0.make lintstep DONE 56.7s,0 issues.make teststep DONE 60.2s, zero(cached)markers in the log; real per-package durations (internal/delivery 4.131s,internal/handlers 3.630s,internal/middleware 1.125s)make fmt-checkclean.make checkis exactly those three targets.Mutation
ipv6BucketBits64 to 128, rebuilt uncached: reproduced the author's claim exactly —TestLoginRateLimit_IPv6SharesBucketWithinSlash64and the threeipv6*subtests ofTestRateLimitKey_IPv6BucketsByPrefixFAIL, whileipv4,ipv4-neighbourandipv4-mappedstill PASS. Those tests are specific. Reverted.CI green on
0946316(check / check (push), 3m2s). Every container and image from this review removed.0946316844to9826cc600fFAIL — needs-rework
Independent review of head
9826cc6. Both findings from #162 (comment) are genuinely fixed, confirmed by my own mutations. One blocker remains, plus one coverage note.1. Commit is authored by
sneak, notclawbot(blocking)9826cc6carriesAuthor: sneak <sneak@sneak.berlin>. Its parentc378690and every merged commit onmain(4f5ecb1,734606b,ee7c626,81413c5,f6b929f) are authored byclawbot <clawbot@noreply.example.org>. This change was not written by the repo owner and must not be credited to him in history.Disclosure on how hard this is: it is not codified in
REPO_POLICIES.md, and Gitea's squash merge — this repo's default style — may rewrite the author to the PR poster. I could not verify that it does. Since the repo also permits rebase and fast-forward-only merges, and misattribution is unfixable once landed, fix it on the branch rather than relying on the merge style:git commit --amend --author='clawbot <clawbot@noreply.example.org>', force-push.2. Third
bucketKeycall site is unpinned by any test (note, not blocking)internal/middleware/ratelimit.go:216— the fallback taken when the peer is a trusted proxy butforwardedClientAddrreturnsok=false(malformed hop, empty chain, or overmaxForwardedHops). Mutating only that return topeer.String(), leaving lines 209 and 213 andipv6BucketBitsuntouched:make testexit 0, whole suite green.Structurally the same gap that made finding 1 of the previous round blocking, but I am not treating it as a defect and the consequence is not comparable: only addresses inside
TRUSTED_PROXIES— operator-controlled — can reach that return, every client on that path already shares the proxy's single bucket, and masking there can only merge operator proxies sitting in one/64. Fail-closed either way, nothing attacker-controlled. Recorded so the coverage asymmetry is a known choice rather than an oversight.Previous findings — verified fixed
Finding 1, forwarded branch. Reverted line 213 alone to
return addr.String(),ipv6BucketBitsleft at 64.make testexit 2 with exactly two top-level failures and nothing else in the suite:Every direct-peer test passed, every other package
ok. Selective, so the new tests target the right branch. Converse probe: reverting line 209 alone fails onlyTestRateLimitKey_IPv6BucketsByPrefix,TestLoginRateLimit_IPv6SharesBucketWithinSlash64andTestRateLimitKey_FamiliesDoNotCollide, both forwarded tests passing. The forwarded tests genuinely reach the forwarded branch rather than silently falling back to the peer —TestRateLimitKey_ForwardedIPv6BucketsByPrefixasserts the literal2001:db8:1:2::/64, unreachable from peer10.0.0.1, and the mutation confirms it.Finding 2,
Prefixcontract. The replacement comment matches the Go source exactly:Prefixerrors only onb < 0,b > 32forz4,b > 128forz6, andz0returnsPrefix{}, nil. Discarding the error is safe at both call sites —clientKeymasks only after a successfulnetip.ParseAddrplusnormalizeAddr, andforwardedClientAddrreturnsok=trueonly afternetip.ParseAddrsucceeds — so no zeroAddrreachesbucketKey.Width mutation.
ipv6BucketBits64 to 128: five top-level failures includingTestRateLimitKey_FamiliesDoNotCollide, IPv4 subtests still passing. The localconst wantBits = 64restatement is genuinely mutation-sensitive.18-site refactor — byte-identical. Verified against
nextrather than against the previous head: the diff removes exactly 12 pre-existing lines and every literal maps to a constant of identical value (10.0.0.0/8x6,10.0.0.1:44444x6,198.51.100.7,198.51.100.8). No pre-existing assertion altered. The other 6 sites are inside the IPv6 table this PR adds, where no pre-existing assertion exists to alter.Verified clean
All four limiter instances route through
m.rateLimitKeytoclientKeytobucketKey; IPv4-mapped keys as the plain IPv4 address on both branches; zones stripped by bothnormalizeAddrandPrefixFrom; #168 accurately describes the empty-RemoteAddrgap and is correctly out of scope; README edit surgical and accurate (this repo has no.prettierrcandmake fmtis Go-only, so the hand-wrapping is fine); no new dependency; one commit, title ends(closes #125), basenext,TODO.mdandgo.moduntouched; fast-forwards ontonextatc378690; no Claude/Anthropic references or attribution trailers; inclusive terminology.Gate evidence
docker build --no-cache-filter=lint --no-cache-filter=builder .— exit 0.RUN make fmt-checkDONE 1.2sRUN make lintDONE 56.1s,0 issues.RUN make testDONE 68.9s, zero(cached)markers anywhere in the log, real per-package durations (internal/middleware 1.119s,internal/delivery 3.926s,internal/handlers 3.609s)Host
make checkwith an isolatedGOLANGCI_LINT_CACHEalso exit 0; the Docker run is the authoritative lint. CI green on9826cc6(check / check (push), 2m53s). Image and all containers from this review removed; no prune run.