From 0beeddd47572e014fd34af0ea0061e3f3fbf97cb Mon Sep 17 00:00:00 2001 From: sneak Date: Wed, 12 Aug 2026 11:42:04 +0000 Subject: [PATCH] Warn when production shares one rate-limit bucket (closes #149) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit With TRUSTED_PROXIES empty, every rate limiter keys on the connecting peer. Production runs behind a TLS-terminating reverse proxy, so the peer is that proxy for every request and all clients share one bucket per limit. For the login limiter that means any remote client sending five POSTs a minute holds the only administrative login at HTTP 429. The empty default is correct — trusting forwarded headers from arbitrary peers lets any client choose its own bucket — so this makes the consequence visible rather than changing the keying, the limits or the default: - config logs a WARN at startup when the environment is prod and TRUSTED_PROXIES is empty, naming the variable, the shared bucket and the deniable admin login. - The security-feature bullet's "per IP" login claim is now conditional on TRUSTED_PROXIES, which is the only case where it holds. - The rate-limiting section separates the receiver case (sharing costs throughput, the safe direction) from the login case (sharing costs availability of the only admin path, not safe). - The trusted-proxies configuration section states the consequence and names TRUSTED_PROXIES as the remedy. --- README.md | 59 +++++++++++++++++++------ internal/config/config.go | 34 +++++++++++++++ internal/config/config_test.go | 78 ++++++++++++++++++++++++++++++++++ internal/config/export_test.go | 17 ++++++++ 4 files changed, 175 insertions(+), 13 deletions(-) diff --git a/README.md b/README.md index b41e13b..ae8dcae 100644 --- a/README.md +++ b/README.md @@ -96,7 +96,7 @@ TTY detection, and security headers are always applied. | `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 (10x that per IP across the route) | `120` | -| `TRUSTED_PROXIES` | CIDRs whose forwarded headers are trusted | `""` (none) | +| `TRUSTED_PROXIES` | CIDRs whose forwarded headers are trusted (unset: all clients behind a proxy share one rate-limit bucket) | `""` (none) | #### Trusted proxies @@ -115,6 +115,23 @@ or draining someone else's. Set it to the address of your reverse proxy, and to nothing wider. A set but unparseable value aborts startup. +That default is safe against forged headers, but leaving it unset in +production has a cost you must know about. Production runs behind a +TLS-terminating reverse proxy, so with `TRUSTED_PROXIES` unset every +request keys on the proxy's own address and all clients share a single +bucket per limit. For the login and password-change limits that is a +denial of service anyone can perform: a steady five POSTs per minute +from any address on the internet keeps the shared login bucket full, +and the operator's own login then returns HTTP 429 for as long as the +trickle continues. There is no second administrative path and no +bypass. Restarting the service clears the in-memory buckets, but a +sustained trickle re-locks them immediately. + +The remedy is to set `TRUSTED_PROXIES` to your reverse proxy's +address, which restores per-client buckets. webhooker logs a warning +at startup when `WEBHOOKER_ENVIRONMENT=prod` and `TRUSTED_PROXIES` is +empty. See [Rate Limiting](#rate-limiting) for what each limit shares. + `X-Real-IP` and `True-Client-IP` are **never** read, from any peer. Reverse proxies append to `X-Forwarded-For` but forward other client headers verbatim, so a single-valued header is client-controlled even @@ -890,16 +907,29 @@ connection's own address, unless the peer is listed in `TRUSTED_PROXIES`, in which case the forwarded client address is used 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. 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`. +every other client behind the same proxy. Set `TRUSTED_PROXIES` to the +proxy's address to get per-client limits back. What the shared bucket +costs is not the same for every limiter, and the two cases pull in +opposite directions: + +- For the **receiver** limits it costs throughput, which is the safe + direction to be wrong in: sharing can only make a limit bind sooner, + never let a sender past it. It 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`. +- For the **login and password-change** limits it costs availability of + the only administrative path, which is not safe at all. Five POSTs + per minute from any address on the internet keeps the single shared + login bucket full, and the operator's own login returns HTTP 429 for + as long as that trickle continues. A restart clears the in-memory + buckets and a resumed trickle re-locks them. Production deployments + must set `TRUSTED_PROXIES`; webhooker warns at startup when it is + empty in `prod`. 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 @@ -1118,8 +1148,11 @@ downstream at form-parse time. (custom HTTP transport with SSRF-safe dialer that validates resolved IPs before connecting, preventing DNS rebinding attacks) - **Login rate limiting** via [go-chi/httprate](https://github.com/go-chi/httprate): - per-IP sliding-window rate limiter on the login endpoint (5 POST - attempts per minute per IP) to prevent brute-force attacks + sliding-window rate limiter on the login endpoint, 5 POST attempts + per minute per bucket, to slow brute-force attacks. The bucket is per + client IP only when `TRUSTED_PROXIES` names the reverse proxy; + unset, every client shares one bucket and the login becomes remotely + deniable (see [Rate Limiting](#rate-limiting)) - Prometheus metrics behind basic auth - Static assets embedded in binary (no filesystem access needed at runtime) diff --git a/internal/config/config.go b/internal/config/config.go index cae3820..073f97b 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -422,6 +422,38 @@ func loadFromEnv() (*Config, error) { }, nil } +// warnSharedRateLimitBucket logs a startup warning when a production +// deployment leaves TRUSTED_PROXIES empty. +// +// With no trusted proxies every rate limiter keys on the connecting +// peer's address. A production deployment is required to run behind a +// TLS-terminating reverse proxy, and the peer is then that proxy for +// every request, so all clients share one bucket per limiter. The +// login limiter's bucket is the dangerous one: any remote client can +// keep it full, which denies the only administrative login to +// everyone until the process restarts. +// +// The default of trusting nobody is deliberate — trusting forwarded +// headers from arbitrary peers lets any client choose its own bucket — +// so this warns rather than failing startup or changing the key. +func (c *Config) warnSharedRateLimitBucket(log *slog.Logger) { + if !c.IsProd() || len(c.TrustedProxies) > 0 { + return + } + + log.Warn( + "TRUSTED_PROXIES is empty: rate limits key on the "+ + "connecting peer, so behind the reverse proxy a "+ + "production deployment runs behind, every client "+ + "shares one bucket per limit. Any remote client can "+ + "then keep the login limit full and deny the admin "+ + "login, the only administrative path, until restart. "+ + "Set TRUSTED_PROXIES to your reverse proxy's address.", + "environment", c.Environment, + "trustedProxies", len(c.TrustedProxies), + ) +} + // New creates a Config by reading environment variables. // //nolint:revive // lc parameter is required by fx even if unused. @@ -466,5 +498,7 @@ func New(lc fx.Lifecycle, params ConfigParams) (*Config, error) { s.MetricsUsername != "" && s.MetricsPassword != "", ) + s.warnSharedRateLimitBucket(log) + return s, nil } diff --git a/internal/config/config_test.go b/internal/config/config_test.go index f8ff7be..d6558f6 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -1,6 +1,8 @@ package config_test import ( + "bytes" + "log/slog" "os" "testing" "time" @@ -624,3 +626,79 @@ func testTrustedProxiesSuccess( assert.Equal(t, expected, got) } + +// TestSharedRateLimitBucketWarning covers the startup warning that +// tells an operator their production deployment shares one rate-limit +// bucket between every client, which makes the admin login remotely +// deniable. It must fire when TRUSTED_PROXIES is empty in production +// and stay quiet otherwise. +func TestSharedRateLimitBucketWarning(t *testing.T) { + tests := []struct { + name string + environment string + trustedProxies string + expectWarning bool + }{ + { + name: "prod without trusted proxies warns", + environment: config.EnvironmentProd, + expectWarning: true, + }, + { + name: "prod with trusted proxies is quiet", + environment: config.EnvironmentProd, + trustedProxies: cidrPrivateV4, + expectWarning: false, + }, + { + // Development is not required to run behind a + // reverse proxy, so the shared bucket the warning + // describes is not the expected shape there. + name: "dev without trusted proxies is quiet", + environment: config.EnvironmentDev, + expectWarning: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Cannot use t.Parallel() here because t.Setenv + // is incompatible with parallel subtests. + t.Setenv("WEBHOOKER_ENVIRONMENT", tt.environment) + + if tt.trustedProxies == "" { + require.NoError( + t, os.Unsetenv("TRUSTED_PROXIES"), + ) + } else { + t.Setenv("TRUSTED_PROXIES", tt.trustedProxies) + } + + var buf bytes.Buffer + + log := slog.New(slog.NewJSONHandler( + &buf, &slog.HandlerOptions{ + Level: slog.LevelDebug, + }, + )) + + require.NoError( + t, + config.WarnSharedRateLimitBucketForTest(log), + ) + + if !tt.expectWarning { + assert.Empty(t, buf.String()) + + return + } + + logged := buf.String() + + assert.Contains(t, logged, `"level":"WARN"`) + assert.Contains(t, logged, "TRUSTED_PROXIES") + assert.Contains(t, logged, "shares one bucket") + assert.Contains(t, logged, "deny the admin login") + }) + } +} diff --git a/internal/config/export_test.go b/internal/config/export_test.go index c7fac51..1d137a6 100644 --- a/internal/config/export_test.go +++ b/internal/config/export_test.go @@ -1,9 +1,26 @@ package config +import "log/slog" + // This file exposes the unexported environment parsing helpers to // the external config_test package so each helper can be covered by // its own table-driven test without weakening the package API. +// WarnSharedRateLimitBucketForTest loads a Config from the current +// environment and emits its startup warnings to log. The real logger +// writes to stdout, so this lets the warning's firing condition be +// asserted against a handler the test controls. +func WarnSharedRateLimitBucketForTest(log *slog.Logger) error { + c, err := loadFromEnv() + if err != nil { + return err + } + + c.warnSharedRateLimitBucket(log) + + return nil +} + // EnvBoolForTest exposes envBool. func EnvBoolForTest(key string, defaultValue bool) (bool, error) { return envBool(key, defaultValue)