diff --git a/README.md b/README.md index d089ea3..85a5ace 100644 --- a/README.md +++ b/README.md @@ -94,7 +94,7 @@ TTY detection, and security headers are always applied. | `METRICS_PASSWORD` | Basic auth password for `/metrics` | `""` | | `SENTRY_DSN` | Sentry error reporting DSN | `""` | | `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 @@ -851,6 +851,20 @@ 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. + +Requests to a `/webhook/` path that names no entrypoint are logged at +`DEBUG` only, since the path is attacker-controlled; the request itself +still appears in the access log. + 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 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..db796a9 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 @@ -242,15 +251,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 +282,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.tooManyRequests( + "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..477eee0 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,77 @@ 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", + ) +} + +// 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.