diff --git a/README.md b/README.md index 9b764f9..63ebca2 100644 --- a/README.md +++ b/README.md @@ -95,7 +95,7 @@ TTY detection, and security headers are always applied. | `SENTRY_DSN` | Sentry error reporting DSN | `""` | | `RETENTION_SWEEP_INTERVAL` | Retention reaper period (Go duration, must be positive) | `1h` | | `SESSION_IDLE_TIMEOUT` | Idle session timeout (Go duration) | `24h` | -| `RECEIVER_RATE_LIMIT` | Receiver requests/minute per IP per entrypoint | `120` | +| `RECEIVER_RATE_LIMIT` | Receiver requests/minute per IP per entrypoint (10x that per IP across the route) | `120` | | `TRUSTED_PROXIES` | CIDRs whose forwarded headers are trusted | `""` (none) | #### Trusted proxies @@ -856,6 +856,31 @@ legitimate webhook senders). Requests over the limit receive HTTP 429 with a `Retry-After` header. A set-but-invalid `RECEIVER_RATE_LIMIT` value aborts startup rather than silently falling back to the default. +A second limit sits in front of that one, keyed on the client IP alone +and covering the whole route at ten times `RECEIVER_RATE_LIMIT` requests +per minute (default 1200). The per-entrypoint limit needs it: the route +pattern matches any single path segment, so a client that invents a +fresh path per request gets a fresh per-entrypoint bucket every time and +would otherwise have no aggregate limit at all — while each of those +requests still costs an entrypoint lookup before it 404s. The aggregate +limit leaves room for one address to drive several entrypoints at their +full rate, and it is not configurable separately. + +What that aggregate limit bounds is the database work an invented path +costs; log volume it caps rather than eliminates. A path that names no +entrypoint is recorded by the handler at `DEBUG`, and the aggregate +limiter logs its own rejections at `DEBUG` and without the path, so +neither appears at all under the default level. The per-entrypoint +limiter is the loud one: it still logs every rejection at `WARN` with +the request path, which on this route is attacker-controlled text. A +client hammering a single invented path is served `RECEIVER_RATE_LIMIT` +requests and has the rest of its aggregate budget rejected there, so +the aggregate limit is what bounds those `WARN` lines — to under ten +times `RECEIVER_RATE_LIMIT` per minute per client IP, 1080 at the +defaults, where before it there was no bound at all. The access log is +bounded by neither limit: every request is recorded once at `INFO` with +its full URL, served or rejected alike. + Every limiter here — receiver, login, and password change — identifies the client the same way, through one shared key function: the connection's own address, unless the peer is listed in @@ -864,7 +889,14 @@ instead. See [Trusted proxies](#trusted-proxies). Deployed without that variable set, a client behind a reverse proxy shares one bucket with every other client behind the same proxy, which is the safe direction to be wrong in: set `TRUSTED_PROXIES` to the proxy's address to get -per-client limits back. +per-client limits back. That shared bucket matters more for the +aggregate limit than for the per-entrypoint one: with `TRUSTED_PROXIES` +unset behind the reverse proxy a production deployment is required to +run behind, every request keys on the proxy, so the aggregate limit +becomes a service-wide ceiling of 1200 requests per minute across all +senders and all entrypoints, where the per-entrypoint limit's capacity +still grows with the number of entrypoints. Any deployment with more +than a handful of busy entrypoints must set `TRUSTED_PROXIES`. Finer-grained per-webhook rate limits (configured in the web UI and enforced in the webhook handler) can layer on top of this env-level diff --git a/internal/handlers/webhook.go b/internal/handlers/webhook.go index d84ce90..cb13320 100644 --- a/internal/handlers/webhook.go +++ b/internal/handlers/webhook.go @@ -39,12 +39,6 @@ func (h *Handlers) HandleWebhook() http.HandlerFunc { return } - h.log.Info("webhook request received", - "entrypoint_uuid", entrypointUUID, - "method", r.Method, - "remote_addr", r.RemoteAddr, - ) - entrypoint, ok := h.lookupEntrypoint( w, r, entrypointUUID, ) @@ -52,6 +46,18 @@ func (h *Handlers) HandleWebhook() http.HandlerFunc { return } + // Logged only once the UUID is known to name a real + // entrypoint. The UUID comes straight out of the path on + // the one unauthenticated endpoint, so logging it before + // the lookup let a client write an INFO line per invented + // path; the request itself is already in the access log + // and a miss is already logged at DEBUG. + h.log.Info("webhook request received", + "entrypoint_uuid", entrypointUUID, + "method", r.Method, + "remote_addr", r.RemoteAddr, + ) + if !entrypoint.Active { http.Error(w, "Gone", http.StatusGone) diff --git a/internal/middleware/export_test.go b/internal/middleware/export_test.go index 222ca64..ecf7d98 100644 --- a/internal/middleware/export_test.go +++ b/internal/middleware/export_test.go @@ -41,3 +41,13 @@ const LoginRateLimitConst = loginRateLimit // PasswordChangeRateLimitConst exposes the // passwordChangeRateLimit constant. const PasswordChangeRateLimitConst = passwordChangeRateLimit + +// ReceiverAggregateMultiplierConst exposes the +// receiverAggregateMultiplier constant. +const ReceiverAggregateMultiplierConst = receiverAggregateMultiplier + +// ReceiverAggregateLimitForTest exposes receiverAggregateLimit for +// testing. +func ReceiverAggregateLimitForTest(perEntrypoint int) int { + return receiverAggregateLimit(perEntrypoint) +} diff --git a/internal/middleware/ratelimit.go b/internal/middleware/ratelimit.go index e32ae52..c63b7c1 100644 --- a/internal/middleware/ratelimit.go +++ b/internal/middleware/ratelimit.go @@ -1,6 +1,7 @@ package middleware import ( + "math" "net/http" "net/netip" "slices" @@ -33,6 +34,14 @@ const ( // requests per minute. receiverRateInterval = 1 * time.Minute + // receiverAggregateMultiplier scales the configured + // per-entrypoint receiver limit into the aggregate limit one + // client IP may spend across the whole /webhook/* route. Ten + // entrypoints' worth lets a single sender address drive several + // entrypoints at their full rate, while still capping what one + // address costs the unauthenticated receiver. + receiverAggregateMultiplier = 10 + // maxForwardedHops bounds how many X-Forwarded-For entries the // chain walk examines. Real chains are one to three hops, but a // client can pad the header up to MaxHeaderBytes, so without a @@ -162,9 +171,11 @@ func (m *Middleware) clientKey(r *http.Request) string { return peer.String() } -// tooManyRequests returns the 429 handler shared by every limiter: -// it logs the rejection with logMessage and answers with -// responseMessage. httprate adds the Retry-After header (RFC 6585). +// tooManyRequests returns the 429 handler used by the login, +// password-change and per-entrypoint receiver limiters: it logs the +// rejection with logMessage and answers with responseMessage. +// httprate adds the Retry-After header (RFC 6585). The aggregate +// receiver limiter uses floodTooManyRequests instead. func (m *Middleware) tooManyRequests( logMessage, responseMessage string, ) http.HandlerFunc { @@ -174,6 +185,31 @@ func (m *Middleware) tooManyRequests( } } +// floodTooManyRequests returns the 429 handler for a limiter whose +// rejections are themselves the flood: it logs at DEBUG and without +// the path, then answers with responseMessage. +// +// The aggregate receiver limiter trips exactly when one address is +// sending faster than the receiver wants to serve, so its rejection +// log is one line per request of that flood. At WARN with "path" that +// hands a client a way to write its own text into the operator's log, +// at a level that trips alerting, once per request — the log-volume +// problem this limiter exists to bound. DEBUG is off in production by +// default, so a flood costs nothing here; the path is dropped so that +// turning DEBUG on to diagnose one does not restore the problem. +// +// This limiter bounds the database work an invented path costs, not +// the number of log lines it produces: the access log in +// middleware.go still records every request, served or rejected. +func (m *Middleware) floodTooManyRequests( + logMessage, responseMessage string, +) http.HandlerFunc { + return func(w http.ResponseWriter, _ *http.Request) { + m.log.Debug(logMessage) + http.Error(w, responseMessage, http.StatusTooManyRequests) + } +} + // LoginRateLimit returns middleware that enforces per-IP rate // limiting on login attempts using go-chi/httprate. Only POST // requests are rate-limited; GET requests (rendering the login @@ -242,15 +278,26 @@ func (m *Middleware) postRateLimit( } } -// ReceiverRateLimit returns middleware that rate-limits the -// public webhook receiver endpoint per client IP per request -// path (the path contains the entrypoint UUID, so each sender -// is limited per entrypoint without affecting other senders or -// other entrypoints). The limit is Config.ReceiverRateLimit -// requests per minute. Requests over the limit receive a 429. -// Clients are identified by rateLimitKey. +// ReceiverRateLimit returns middleware that rate-limits the public +// webhook receiver endpoint with two limits in series. +// +// The inner limit is per client IP per request path: the path +// contains the entrypoint UUID, so each sender is limited per +// entrypoint without affecting other senders or other entrypoints. +// It is Config.ReceiverRateLimit requests per minute. +// +// That limit alone bounds nothing in aggregate. The route pattern +// /webhook/{uuid} matches any single segment, so a client that +// invents a fresh path per request mints a fresh bucket per request +// and never refills one — and every such request still reaches the +// handler's entrypoint lookup before it 404s. The outer limit is +// therefore keyed on the client IP alone, capping what one address +// can spend across the whole route however it varies the path. +// +// Requests over either limit receive a 429. Clients are identified +// by rateLimitKey. func (m *Middleware) ReceiverRateLimit() func(http.Handler) http.Handler { - return httprate.Limit( + perEntrypoint := httprate.Limit( m.params.Config.ReceiverRateLimit, receiverRateInterval, httprate.WithKeyFuncs( @@ -262,4 +309,31 @@ func (m *Middleware) ReceiverRateLimit() func(http.Handler) http.Handler { "Too many requests. Please slow down.", )), ) + + aggregate := httprate.Limit( + receiverAggregateLimit(m.params.Config.ReceiverRateLimit), + receiverRateInterval, + httprate.WithKeyFuncs(m.rateLimitKey), + httprate.WithLimitHandler(m.floodTooManyRequests( + "webhook receiver aggregate rate limit exceeded", + "Too many requests. Please slow down.", + )), + ) + + return func(next http.Handler) http.Handler { + return aggregate(perEntrypoint(next)) + } +} + +// receiverAggregateLimit is the per-IP aggregate limit derived from +// the configured per-entrypoint limit. The operator sets the latter +// and nothing bounds it from above, so the multiplication is +// saturated rather than allowed to wrap into a negative limit that +// would reject every request. +func receiverAggregateLimit(perEntrypoint int) int { + if perEntrypoint > math.MaxInt/receiverAggregateMultiplier { + return math.MaxInt + } + + return perEntrypoint * receiverAggregateMultiplier } diff --git a/internal/middleware/ratelimit_test.go b/internal/middleware/ratelimit_test.go index 9740f4c..9569161 100644 --- a/internal/middleware/ratelimit_test.go +++ b/internal/middleware/ratelimit_test.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "log/slog" + "math" "net/http" "net/http/httptest" "net/netip" @@ -670,6 +671,129 @@ func TestRateLimitKey_LongChainAllocationIsBounded(t *testing.T) { ) } +// TestReceiverRateLimit_LimitsAggregateAcrossInventedPaths is the +// regression test for the per-path bucket key. The route pattern +// matches any single segment, so a client that never reuses a path +// never reuses a per-entrypoint bucket either, and its aggregate +// rate against the receiver is whatever it likes — with every +// request reaching an entrypoint lookup before it 404s. The IP-only +// aggregate limiter is what bounds that, so this must fail if the +// aggregate limiter is removed. +func TestReceiverRateLimit_LimitsAggregateAcrossInventedPaths( + t *testing.T, +) { + t.Parallel() + + const ( + limit = 3 + ip = "6.6.6.6:1234" + ) + + aggregate := limit * middleware.ReceiverAggregateMultiplierConst + + handler := receiverLimitedHandler(t, limit) + + // Every request goes to a path this client has never used, so + // none of them shares a per-entrypoint bucket with another. + for i := range aggregate { + w := receiverPost( + handler, ip, fmt.Sprintf("/webhook/invented-%d", i), + ) + assert.Equal( + t, http.StatusOK, w.Code, + "request %d to a distinct path should pass", i, + ) + } + + w := receiverPost( + handler, ip, fmt.Sprintf("/webhook/invented-%d", aggregate), + ) + assert.Equal( + t, http.StatusTooManyRequests, w.Code, + "a client must not be able to raise its aggregate rate "+ + "against /webhook/* by varying the path", + ) + + // The aggregate limit is still per client IP: exhausting one + // address must not throttle another. + w = receiverPost(handler, "6.6.6.7:1234", "/webhook/invented-0") + assert.Equal( + t, http.StatusOK, w.Code, + "a different client IP must not be affected", + ) +} + +// TestReceiverRateLimit_RejectedRequestsCountTowardAggregate pins the +// order the two limiters are chained in. The aggregate limiter has to +// be the outer one, so that it counts requests the per-entrypoint +// limiter rejects: those requests still arrive, and the aggregate +// limit exists to bound what one address can make the receiver do. +// +// One path is hammered past the per-entrypoint limit, which alone +// would leave the aggregate budget almost untouched; then a path the +// client has never used must be rejected, which only the aggregate +// limiter can do. Swap the two limiters and that last request is +// served, because the rejected ones never reached the aggregate +// limiter to be counted. +func TestReceiverRateLimit_RejectedRequestsCountTowardAggregate( + t *testing.T, +) { + t.Parallel() + + const ( + limit = 3 + ip = "6.6.6.8:1234" + ) + + aggregate := limit * middleware.ReceiverAggregateMultiplierConst + + handler := receiverLimitedHandler(t, limit) + + // Spend the whole aggregate budget on one path. Only the first + // limit requests are served; the rest are rejected by the + // per-entrypoint limiter but still count against the aggregate. + for i := range aggregate { + w := receiverPost(handler, ip, "/webhook/exhausted") + + want := http.StatusTooManyRequests + if i < limit { + want = http.StatusOK + } + + assert.Equal( + t, want, w.Code, + "request %d to the exhausted path", i, + ) + } + + w := receiverPost(handler, ip, "/webhook/never-used") + assert.Equal( + t, http.StatusTooManyRequests, w.Code, + "requests rejected per entrypoint must still count "+ + "toward the aggregate limit, so the aggregate "+ + "limiter has to run first", + ) +} + +// TestReceiverAggregateLimit_SaturatesOnOverflow covers the derived +// aggregate limit for a configured per-entrypoint limit large enough +// that multiplying it would wrap negative, which httprate would read +// as a limit that rejects every request. +func TestReceiverAggregateLimit_SaturatesOnOverflow(t *testing.T) { + t.Parallel() + + assert.Equal( + t, 1200, + middleware.ReceiverAggregateLimitForTest(120), + "the default limit scales by the multiplier", + ) + assert.Equal( + t, math.MaxInt, + middleware.ReceiverAggregateLimitForTest(math.MaxInt), + "an overflowing limit saturates instead of wrapping", + ) +} + // TestReceiverRateLimit_IgnoresForwardedFromUntrustedPeer proves // the receiver limiter uses the same gated key function as the // POST limiters.