fix(auth): bucket IPv6 connection limits by /64 - #3784
Conversation
`ip_rate_limit_key` keyed the connection fence on the full address, so an IPv6 client charged a different Redis counter for every source address it used. RFC 4291 §2.5.4 fixes the interface identifier at 64 bits and end sites are routinely delegated a /64 or shorter, so a single host can rotate through 2^64 addresses and receive a fresh quota each time. The fence counted addresses rather than clients, and `check_ip_connection` is the pre-auth edge control — it runs before host to community resolution, so nothing downstream re-checks it. Bucket IPv6 on the /64 prefix. IPv4 keeps the full address: v4 space is scarce and commonly shared behind carrier NAT, where widening the bucket would let one abusive client deny service to everyone behind the same egress. IPv4-mapped addresses (::ffff:a.b.c.d) are unmapped before bucketing. Masking them as ordinary v6 would place every IPv4 client arriving on a dual-stack socket into the single ::/64 bucket, converting the per-client fence into a global cap on all IPv4 traffic. Keys change shape for IPv6, so existing counters are orphaned; they expire with the rate-limit window and no migration is needed. Signed-off-by: DaX <daxdax89@gmail.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 922076f9fc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /// counter per connection. | ||
| pub fn ip_rate_limit_key(ip: &IpAddr) -> String { | ||
| format!("buzz:ratelimit:ip:{}:conn", ip) | ||
| format!("buzz:ratelimit:ip:{}:conn", ip_rate_limit_bucket(ip)) |
There was a problem hiding this comment.
For the relay path inspected, this normalization never runs: a repo-wide search finds check_ip_connection only in trait/implementation/test definitions, while router.rs upgrades directly into handle_connection and connection.rs checks only the process-wide semaphore. Consequently, an unauthenticated IPv6 peer rotating addresses still reaches connection admission without incrementing this Redis key, so the connection-slot exhaustion vulnerability described by this fix remains unchanged.
Useful? React with 👍 / 👎.
|
looks good. if theres a user-visible string change, a tiny screenshot helps a lot |
2 participants
Problem
ip_rate_limit_keykeyed the per-IP connection fence on the full address:For IPv6 that is a
/128. RFC 4291 §2.5.4 fixes the interface identifier at 64 bits, and end sites are routinely delegated a/64or shorter, so one host can rotate through 2^64 source addresses and charge a different Redis counter on every connection. The fence counts addresses, not clients.This is the pre-auth edge control —
check_ip_connectionruns before host to community resolution completes (and instead of it when resolution fails), so unlike the pubkey-keyed limits there is no authenticated identity downstream that re-checks it. It is the only thing standing between an unauthenticated peer and connection-slot exhaustion.Trace:
ip_rate_limit_key(crates/buzz-auth/src/rate_limit.rs:213) →check_ip_connection(crates/buzz-pubsub/src/rate_limiter.rs:118) →run_rate_limitfixed-window. No prefix normalization anywhere in the chain.Fix
Bucket IPv6 on the
/64prefix via a newip_rate_limit_bucket.IPv4 deliberately keeps the full address. v4 space is scarce and commonly shared behind carrier NAT, so widening that bucket would let one abusive client deny service to everyone behind the same egress — the opposite failure.
The IPv4-mapped trap
::ffff:a.b.c.darrives on dual-stack sockets. Masking those to/64like any other v6 address would place every IPv4 client into the single::/64bucket, silently converting a per-client fence into a global cap on all IPv4 traffic — a self-inflicted outage strictly worse than the bug being fixed. They are unmapped to their v4 form first, which also means a client cannot double its quota by switching socket family. There is a test pinning both directions.Tests
Five cases in
rate_limit.rs, all infra-free:/64share a counter (the regression)/64s stay independent (guards against over-blocking)rate_limit_keyalready holdsVerified the suite catches the bug: reverting
ip_rate_limit_buckettoip.to_string()fails exactly the/64and v4-mapped tests.Operational note
IPv6 keys change shape, so counters in flight at deploy are orphaned. They expire with the rate-limit window (seconds) and no migration is needed.
Checks
just fmt-check,just clippy(clean), andjust test-unit(all six suites) pass locally.buzz-pubsubis unaffected — the public signature is unchanged.Scope
Found while auditing
buzz-auth. Two other candidate findings from that pass are deliberately not included, since both turned out to be working as designed:verify_auth_event—SECURITY.mdstates channel membership is the only access-control mechanism and there are no capability taxonomies, so the scope layer is intentionally not a gate;api/bridge.rsalready enforces arequire_payloadcheck before verification, andapi/git/transport.rsdocuments itsbody: Noneas a considered trade-off.Happy to open an issue on either if maintainers see it differently.
Related
Searched open PRs and issues; nothing touches IP rate limiting. #3633 (
feat(auth): add provider-neutral authorization contract) also lives inbuzz-authbut only inlib.rsand a newprovider/module, so there is no overlap withrate_limit.rs.