From d8f9d149b55ba5a0689944c7979e04da4467077b Mon Sep 17 00:00:00 2001 From: clawbot Date: Wed, 12 Aug 2026 13:49:39 +0200 Subject: [PATCH] Warn when TRUSTED_PROXIES is empty in production (closes #149) With no trusted proxies configured, every client behind the reverse proxy production requires shares one rate-limit bucket per limit, so five POSTs per minute from anywhere holds the login limit full and denies the admin login until restart. The default is still correct; it was the consequence that was invisible. Startup now warns, and the README no longer claims the login limit is per-IP unconditionally. --- 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)