sec: add per-conn publish rate limit alongside per-key bucket - #21
Merged
Conversation
Closes #11. The existing per-API-key bucket (rateLimit) is shared across every connection a key owns. That's correct for fairness across the key's clients, but it doesn't bound what a single socket can push at the moment — a key with N conns sees the bucket drain N× faster but each individual socket is still capped only by the 64KB read limit and the network round-trip. Adds a per-conn rate.Limiter (golang.org/x/time/rate, already a dep via prometheus client). Default 50 publishes/sec sustained, burst 100 — generous for legitimate UI clients (chat apps see far less) and well below what's needed to amplify a 64KB message into a broadcast DoS at the per-channel cap of 10000 subscribers. handlePublish now checks per-key first (existing behavior), then per-conn. RATE_LIMITED stays for per-key over-budget; new RATE_LIMITED_CONN distinguishes the per-conn case so a chatty client can attribute the slowdown. Tunable via defaultConnPublishRate / defaultConnPublishBurst constants in conn.go; flag wiring deferred to whichever PR ends up landing the broader --publish-rate / --publish-burst flags.
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #11.
The per-API-key bucket (
c.rateLimit.Allow(c.apiKeyID)) is shared across every connection a key owns. That's correct for cross-conn fairness, but it doesn't bound what a single socket can push at the moment — a key with N conns drains the bucket N× faster, but each individual socket is otherwise capped only by the 64KB read limit and network RTT.Adds a per-conn
rate.Limiterfromgolang.org/x/time/rate(already a transitive dep of the Prometheus client). Default 50/s sustained, burst 100 per conn — generous for legitimate UI clients and well below what's needed to amplify a 64KB message into a broadcast DoS atdefaultMaxSubsPerChannel = 10000.handlePublishnow checks per-key first, then per-conn.RATE_LIMITEDis unchanged for the per-key path; newRATE_LIMITED_CONNdistinguishes the per-conn case so a client knows whether to slow down or open another connection.What changed
internal/conn/conn.go:Conn.connRate *rate.Limiterfield.defaultConnPublishRate = 50,defaultConnPublishBurst = 100.Run.internal/conn/handler.go:handlePublishruns the per-conn check after the per-key check.Test plan
go build ./...clean.go test ./internal/conn/...passes — existing tests publish a handful of messages, well within the 100-burst budget.RATE_LIMITED_CONN, while a second conn (same key) keeps publishing at its own limit until the per-key bucket drains.Conflict notes
Touches
internal/conn/conn.goandinternal/conn/handler.go. Conflicts at merge time with PR #20 (channel GC + sub rate-limit), which also adds rate-limit calls to subscribe/unsubscribe. Both are additive; resolution is keeping both calls in each handler.Out of scope
defaultMaxSubsPerChannel(the audit suggested 10000 is too high). Behavior change worth a separate decision; flagged as TODO in conn.go.