security: add HTTP rate limit and max-concurrent middleware#31
Conversation
CockroachDB admission control protects the cluster but not this server process. Without per-client throttling, a single misbehaving client can saturate the worker pool and starve other agents on the same instance. This is the HTTP-mode half of TM-006; pool sizing already shipped in 94fba07. Add two HTTP middlewares in the existing auth package, both no-op when their knob is zero: - RateLimit: per-client token bucket via golang.org/x/time/rate. Clients are keyed by bearer-token hash (so a legit caller shares one bucket across reconnects), then X-Forwarded-For first hop, then RemoteAddr. Per-key map is LRU-capped at 10000 entries so an attacker can't grow it without bound. Exceeded buckets get 429 with Retry-After: 1. - MaxConcurrent: semaphore around the chain. Excess in-flight requests get 503 server busy (not blocked) so a burst of slow handlers cannot exhaust http.Server's goroutine pool. Chain order in newHTTPMux is outer to inner: MaxConcurrent (cheapest reject) -> Bearer (cheap reject of bad tokens) -> RateLimit (per-client throttle on accepted callers) -> mcpHandler. /healthz and /ready bypass the chain so probes are never throttled. New env vars (all default 0 = disabled): - CRDB_MCP_HTTP_RPS - CRDB_MCP_HTTP_BURST (defaults to 2*RPS when unset) - CRDB_MCP_HTTP_MAX_CONCURRENT Addresses SECSERV-424.
f7e883f to
5633998
Compare
|
|
||
| // clientKey returns a stable per-client identifier, preferring an | ||
| // authenticated identity. Bearer tokens are hashed so the key cannot be | ||
| // reversed from a memory dump. X-Forwarded-For is used only when trustXFF is |
There was a problem hiding this comment.
🟢 Confidence: 35 (Low)
Rate-limit bypass via Authorization header spoofing when CRDB_MCP_ALLOW_NO_BEARER=true.
clientKey preferentially keys by Authorization: Bearer <token> hash (line 89-91). When bearer enforcement is disabled (AllowNoBearer=true), the Bearer middleware is absent from the chain, so no validation occurs on the Authorization header. An attacker can send a unique Authorization: Bearer <random> value per request, each hashing to a distinct rate-limit key with a fresh burst allowance, effectively bypassing per-client rate limiting entirely.
Mitigated by the documented assumption that AllowNoBearer deployments sit behind a trusted proxy, and by MaxConcurrent as a global backstop. Consider either: (a) only keying by bearer token when the Bearer middleware is active (pass a flag), or (b) documenting that rate limiting is ineffective without bearer enforcement or XFF trust.
| busy++ | ||
| } | ||
| } | ||
| require.Equal(t, 2, ok, "only the cap should succeed while 3 others race the semaphore") |
There was a problem hiding this comment.
🟢 Confidence: 25 (Low)
Potentially flaky test assertion due to goroutine scheduling race.
The test launches 5 goroutines against a limit-2 semaphore, waits for 2 to be in-flight (require.Eventually), then immediately close(hold) to unblock them. The assertion requires exactly ok=2, busy=3, but there's no synchronization ensuring all 5 goroutines have attempted the semaphore before close(hold) frees slots. If a goroutine is delayed by the scheduler past the close(hold), it finds a free slot and succeeds with 200 instead of 503.
Suggested fix: add a barrier (e.g. a sync.WaitGroup or second channel) that each goroutine signals after calling h.ServeHTTP, and wait on it before closing hold. Alternatively, weaken the assertion to require.GreaterOrEqual(t, ok, 2) and require.LessOrEqual(t, busy, 3).
| e = &limiterEntry{limiter: rate.NewLimiter(s.limit, s.burst)} | ||
| s.keys[key] = e | ||
| } | ||
| e.lastSeen = time.Now() |
There was a problem hiding this comment.
🟢 Confidence: 20 (Low)
O(n) eviction scan under the global mutex at map capacity.
evictOldestLocked() linearly scans all maxLimiterKeys (10,000) entries to find the oldest. This runs under s.mu, blocking all concurrent allow() callers. Under sustained load from many unique clients (e.g. a proxy forwarding diverse IPs), the map stays at capacity and every new client triggers the scan, adding lock-contention latency to every in-flight request.
Suggested fix: maintain a min-heap ordered by lastSeen or use container/list as an LRU for O(1) eviction. Alternatively, batch-evict (e.g. drop the oldest 10%) to amortize the cost.
|
|
2 participants
Summary
Addresses SECSERV-424 (TM-006). CockroachDB admission control protects the cluster but not this server process. Without per-client throttling, a single misbehaving client can saturate the worker pool and starve other agents sharing the same instance. Pool sizing already shipped in 94fba07; this is the HTTP-mode half.
Adds two HTTP middlewares in the existing
auth/package, both no-op when their knob is zero, so default installs pay nothing.Middleware chain (outer to inner)
MaxConcurrent(cheap 503 reject under load) ->Bearer(skipped whenCRDB_MCP_ALLOW_NO_BEARER=true) ->RateLimit(token-bucket per client key) -> MCP handler.Client keying (rate limit)
CRDB_MCP_HTTP_RPSis effectively a global rate in single-token deployments (documented in README).RemoteAddrhost (IPv6-safe vianet.SplitHostPort).X-Forwarded-Foris ignored by default because it is client-forgeable.CRDB_MCP_HTTP_TRUST_XFF=trueopts in for trusted-proxy deployments and uses the rightmost entry (appended by the nearest proxy), never the client-supplied first entry.Config
CRDB_MCP_HTTP_RPS(float, 0 disables),CRDB_MCP_HTTP_BURST(defaults to 2*RPS; rejected if set while RPS=0),CRDB_MCP_HTTP_MAX_CONCURRENT(0 disables),CRDB_MCP_HTTP_TRUST_XFF(bool, default false).Test plan
go test ./...including new t.Run coverage: burst-then-429, independent buckets, XFF ignored-unless-trusted, rightmost-XFF keying, IPv6 RemoteAddr, semaphore cap under concurrency, config validation (negative/non-numeric/non-bool, burst-without-rps)golangci-lint runclean (fixes revive max-builtin and ST1011 failures)