Bound the receiver rate limit per client IP across /webhook/* (closes #139)
All checks were successful
check / check (push) Superseded by a newer commit; never tested
All checks were successful
check / check (push) Superseded by a newer commit; never tested
The receiver limiter keyed on the request path, and /webhook/{uuid} matches any single segment, so a client minted a fresh bucket per invented path and had unlimited aggregate rate against the only unauthenticated endpoint. An outer limiter keyed on the client address alone now bounds that, chained in front of the unchanged per-entrypoint limiter. Its rejections log at DEBUG without the path, and the README states what each limit does and does not bound.
This commit was merged in pull request #143.
This commit is contained in:
36
README.md
36
README.md
@@ -95,7 +95,7 @@ TTY detection, and security headers are always applied.
|
||||
| `SENTRY_DSN` | Sentry error reporting DSN | `""` |
|
||||
| `RETENTION_SWEEP_INTERVAL` | How often the retention reaper and archive sweeper run (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
|
||||
@@ -859,6 +859,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
|
||||
@@ -867,7 +892,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
|
||||
|
||||
Reference in New Issue
Block a user