diff --git a/README.md b/README.md index 5729606..a5a2ff2 100644 --- a/README.md +++ b/README.md @@ -147,7 +147,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 (unset: all clients behind a proxy share one rate-limit bucket; a correct login password is never throttled either way) | `""` (none) | +| `TRUSTED_PROXIES` | CIDRs whose forwarded headers are trusted. A set value replaces the default. Under the default, any client with a private address, whether it connects directly or through the proxy, can choose its own rate-limit key by sending its own `X-Forwarded-For`; if any clients have private addresses, set it to the proxy's address alone. See [Trusted proxies](#trusted-proxies) | `10.0.0.0/8,172.16.0.0/12,192.168.0.0/16` (RFC 1918) | | `ALLOWED_EGRESS_CIDRS` | CIDRs that delivery targets may reach despite the SSRF blocklist. Read [Allowing egress to your own network](#allowing-egress-to-your-own-network) before setting it | `""` (none) | #### Allowing egress to your own network @@ -379,41 +379,48 @@ unlocked. `TRUSTED_PROXIES` is a comma-separated list of CIDR blocks (a bare address such as `192.168.1.7` is accepted and treated as a single host), for example `192.168.1.7, 2001:db8::5`. It decides whose -`X-Forwarded-For` header the rate limiters believe, so it should name -the addresses of your reverse proxies and nothing else. +`X-Forwarded-For` header the rate limiters believe, so it should cover +the addresses of your reverse proxies. `X-Forwarded-For` is honoured **only** when the connecting peer is inside one of these blocks; for every other peer the client identity is -the connection's own address and the header is ignored. The default is -the empty list, which trusts nobody — anything else would let any -client pick its own rate limit bucket, minting a fresh one per request -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. +the connection's own address and the header is ignored. Unset (or +empty), the list is the RFC 1918 private ranges: `10.0.0.0/8`, +`172.16.0.0/12` and `192.168.0.0/16`. That covers a reverse proxy +reaching webhooker over a Docker network or a private LAN without +anything set. A set value replaces the default entirely. 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 +Trusting those ranges has two consequences for clients with private +addresses: + +- Any such client, whether it connects directly or through the proxy, + can choose its own rate-limit key by sending its own + `X-Forwarded-For`. A direct client's header is walked because the + client is itself trusted; behind the proxy, the client's own address + is skipped as a trusted hop when the chain is walked (below), so the + entry it wrote is taken as the client. If any of your clients have + private addresses, you must set `TRUSTED_PROXIES` to the proxy's + address alone. +- A client behind the proxy that sends no `X-Forwarded-For` of its own + shares the proxy's bucket, because its own address is skipped too. + Setting the list to the proxy's address alone gives each its own + bucket. + +A proxy the list does not cover, such as nginx on the same host +reaching webhooker over loopback, is not trusted: every request through +it keys on the proxy's own address and all clients share a single bucket per limit. The receiver limits become service-wide ceilings, and the login endpoint's failure counting collapses onto one key, so a stranger's wrong passwords throttle every other client's wrong -passwords. +passwords. Set `TRUSTED_PROXIES` to that proxy's address to restore +per-client buckets. What it cannot do is lock the operator out. The login endpoint verifies credentials **before** it consults any limit and charges only failures, so a correct password is never throttled no matter how full the bucket is. See [Rate Limiting](#rate-limiting). -The remedy is to set `TRUSTED_PROXIES` to your reverse proxy's -address, which restores per-client buckets. webhooker logs a warning -at startup whenever `TRUSTED_PROXIES` is empty, in every environment, -because behind a proxy every client shares one bucket in `dev` and -`prod` alike. The warning is informational when nothing proxies to the -process: with no proxy in front, the peer address is the client's own -and the buckets are already per-client. 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 @@ -435,14 +442,14 @@ Two operator requirements follow: (nginx `$proxy_add_x_forwarded_for`, HAProxy `option forwardfor`, Caddy and AWS ALB by default), and must append a bare address with no port. -- List proxy hosts **only**. Any address inside `TRUSTED_PROXIES` +- Keep clients out of the list. Any address inside `TRUSTED_PROXIES` chooses its own rate-limit key: its `X-Forwarded-For` is walked, so it can name a different address on every request to get a fresh bucket each time, or name another client's address to drain that - client's bucket. Never list a block that also covers clients — a - broad `10.0.0.0/8` on a network where clients live in the same range - makes all three limits, including the unauthenticated webhook - receiver, silently bypassable by every client in the block. + client's bucket. A block that also covers clients — the default, on + a network where clients have private addresses — makes all three + limits, including the unauthenticated webhook receiver, silently + bypassable by every client in the block. #### Sessions @@ -764,10 +771,16 @@ repository's `Dockerfile` and runs it. The app needs: - **Environment variables:** - `WEBHOOKER_ENVIRONMENT=prod` - - `TRUSTED_PROXIES`: your reverse proxy's address on that Docker - network. The `remoteIP` field of the `http request` log line for a - request that came through the proxy shows it; the health check's - own lines show `::1`. See [Trusted proxies](#trusted-proxies). + - `TRUSTED_PROXIES`: Docker networks use private addresses, so the + default covers your reverse proxy on that network. Under the + default, any client with a private address, whether it connects + directly or through the proxy, can choose its own rate-limit key + by sending its own `X-Forwarded-For`. If any clients have private + addresses, or the network's addresses are outside the RFC 1918 + ranges, set it to the proxy's address there. The `remoteIP` field + of the `http request` log line for a request that came through + the proxy shows it; the health check's own lines show `::1`. See + [Trusted proxies](#trusted-proxies). - Leave `BIND_ADDRESS` and `DATA_DIR` unset: the image sets `BIND_ADDRESS` to `0.0.0.0`, and `DATA_DIR` defaults to `/var/lib/webhooker`. @@ -830,12 +843,14 @@ reports. behind a proxy means the `X-Forwarded-Proto` header. The block below sets it; without it every request is read as plaintext and cookies ship without `Secure`. See [Configuration](#configuration). -3. **Set `TRUSTED_PROXIES` to the proxy's address.** Unset, every rate - limiter keys on the connecting peer, which behind a proxy is the - proxy on every request: all clients collapse into one global bucket - per limit and the receiver's per-IP limits become service-wide - ceilings. See [Trusted proxies](#trusted-proxies). List the proxy - and nothing else. +3. **Make sure `TRUSTED_PROXIES` covers the proxy's address.** Unset, + it covers the RFC 1918 private ranges, so a proxy on a Docker + network or a private LAN is covered and one on loopback is not. For + a proxy it does not cover, every rate limiter keys on the connecting + peer, which is the proxy on every request: all clients collapse into + one global bucket per limit and the receiver's per-IP limits become + service-wide ceilings. See [Trusted proxies](#trusted-proxies). If + any clients have private addresses, list the proxy and nothing else. 4. **Send `Host` as `$http_host`, not `$host`.** `$host` strips the port. webhooker's Origin/Referer check compares against the host it was given, so on any port other than 443 `$host` makes every form @@ -1396,10 +1411,11 @@ It uses: - **[go-chi/httprate](https://github.com/go-chi/httprate)** for sliding-window rate limiting of the password-change and webhook receiver endpoints. The bucket is per client IP only when - `TRUSTED_PROXIES` names the reverse proxy; unset, every client - behind that proxy shares one bucket per limit. The login endpoint - counts failed attempts itself instead, so that a correct password is - never throttled (see [Rate Limiting](#rate-limiting)) + `TRUSTED_PROXIES` covers the reverse proxy (by default it covers the + RFC 1918 private ranges); otherwise every client behind that proxy + shares one bucket per limit. The login endpoint counts failed + attempts itself instead, so that a correct password is never + throttled (see [Rate Limiting](#rate-limiting)) - **[Prometheus](https://prometheus.io)** for metrics, served at `/metrics` behind basic auth - **[Sentry](https://sentry.io)** for optional error reporting @@ -2580,30 +2596,30 @@ let one subscriber rotate source addresses and mint a fresh bucket per request, evading these limits at the network layer without spoofing anything; the cost is that distinct clients inside one `/64` share a bucket. IPv4-mapped addresses (`::ffff:1.2.3.4`) key as the IPv4 address -they carry. 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. Set `TRUSTED_PROXIES` to the -proxy's address to get per-client limits back. What the shared bucket +they carry. See [Trusted proxies](#trusted-proxies). When that variable +does not cover the reverse proxy, a client behind it shares one bucket +with 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`. + than for the per-entrypoint one: when `TRUSTED_PROXIES` does not + cover 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 make sure + `TRUSTED_PROXIES` covers its proxy. - For the **login and password-change** limits it costs precision, not availability. Login failures from every client land in one counter, so a stranger's wrong passwords make the operator's own wrong passwords answer `429` sooner; the operator's _correct_ password is never affected, because it is never counted. Production deployments - should still set `TRUSTED_PROXIES`; webhooker warns at startup - whenever it is empty, in any environment. + should still make sure `TRUSTED_PROXIES` covers their proxy. #### The login endpoint @@ -2706,8 +2722,10 @@ re-fills both verification slots on its first two requests. The remedies are to block the source at the reverse proxy, or to rate-limit `POST /pages/login` there — the one place a limit can be applied without reintroducing the lockout, because the proxy sees the -real client address. Setting `TRUSTED_PROXIES` does not stop the -saturation, but it makes the source visible in the failure logs. +real client address. `TRUSTED_PROXIES` does not stop the saturation. +The flood's source is in the proxy's access log: webhooker's own logs +record the proxy's address, not the client's (see +[Deployment behind a reverse proxy](#deployment-behind-a-reverse-proxy)). 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 @@ -3064,10 +3082,9 @@ check, see [The login endpoint](#the-login-endpoint). It runs behind session auth, so only a client already holding a valid session reaches it, and an operator throttled out of changing a password can still log in. The bucket is per client IP only when - `TRUSTED_PROXIES` names the reverse proxy; unset, every client + `TRUSTED_PROXIES` covers the reverse proxy; otherwise every client shares one bucket, which costs precision rather than availability - (see [Rate Limiting](#rate-limiting)). webhooker warns at startup - whenever `TRUSTED_PROXIES` is empty + (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 1b9a4e7..00bf0a0 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -75,6 +75,11 @@ const ( // internet-exposed endpoint. defaultReceiverRateLimit = 120 + // defaultTrustedProxies is TRUSTED_PROXIES when it is unset: the + // RFC 1918 private ranges, which a reverse proxy reaching the + // process over a Docker network or a private LAN connects from. + defaultTrustedProxies = "10.0.0.0/8,172.16.0.0/12,192.168.0.0/16" + // maxPort is the highest valid TCP port number. The lower // bound (at least 1) is enforced by envPositiveInt. maxPort = 65535 @@ -172,13 +177,14 @@ type Config struct { // TrustedProxies is the set of networks whose members are // allowed to speak for the client with X-Forwarded-For, the - // only forwarded header read. It is empty unless - // TRUSTED_PROXIES is set, and empty means no peer is - // trusted: forwarded headers are then ignored entirely and - // clients are identified by the connection's own address. - // Members can choose their own rate-limit key, so this must - // name proxy hosts only, never a block that also covers - // clients. + // only forwarded header read. Unless TRUSTED_PROXIES is set it + // is the RFC 1918 private ranges (defaultTrustedProxies). + // Other peers' forwarded headers are ignored and they are + // identified by the connection's own address. Under the + // default any client with a private address, directly or + // through a proxy, can choose its own rate-limit key, so + // where any clients have private addresses this must be set + // to the proxy hosts alone. TrustedProxies []netip.Prefix // AllowedEgressCIDRs is the set of networks a delivery target @@ -460,14 +466,15 @@ func parseCIDR(entry string) (netip.Prefix, error) { // envPrefixList returns the value of the named environment variable // parsed as a comma-separated list of CIDR blocks (bare addresses -// allowed). An unset, empty, or blank value yields an empty list. A -// set value containing an unparseable entry is a hard error naming -// the key and the bad entry, so startup fails loudly rather than -// silently running with a list the operator did not intend. -func envPrefixList(key string) ([]netip.Prefix, error) { +// allowed). An unset, empty, or blank value is read as defaultValue +// instead. A set value containing an unparseable entry is a hard +// error naming the key and the bad entry, so startup fails loudly +// rather than silently running with a list the operator did not +// intend. +func envPrefixList(key, defaultValue string) ([]netip.Prefix, error) { v := strings.TrimSpace(os.Getenv(key)) if v == "" { - return nil, nil + v = defaultValue } var prefixes []netip.Prefix @@ -681,12 +688,12 @@ func loadFromEnv() (*Config, error) { return nil, err } - trustedProxies, err := envPrefixList("TRUSTED_PROXIES") + trustedProxies, err := envPrefixList("TRUSTED_PROXIES", defaultTrustedProxies) if err != nil { return nil, err } - allowedEgressCIDRs, err := envPrefixList("ALLOWED_EGRESS_CIDRS") + allowedEgressCIDRs, err := envPrefixList("ALLOWED_EGRESS_CIDRS", "") if err != nil { return nil, err } @@ -760,50 +767,6 @@ func (c *Config) warnEgressAllowlist(log *slog.Logger) { ) } -// warnSharedRateLimitBucket logs a startup warning whenever -// TRUSTED_PROXIES is empty, in any environment. -// -// With no trusted proxies every rate limiter keys on the connecting -// peer's address. Whether that is harmless or dangerous depends on -// what is in front of the process, which this code cannot observe: -// with nothing in front, the peer is the client and the limits are -// per-client as intended; behind a reverse proxy the peer is the proxy -// for every request, so all clients share one bucket per limiter. -// -// The login endpoint no longer spends budget on arrival — it verifies -// credentials first and charges only failures — so a shared bucket -// cannot deny the operator a correct password. What it does collapse -// is the failure counting: one client's wrong passwords throttle -// everyone else's wrong passwords, and the receiver's limits become -// service-wide ceilings. -// -// The warning is deliberately not gated on WEBHOOKER_ENVIRONMENT: -// behind a proxy every client shares one bucket in dev and prod alike. -// -// 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 len(c.TrustedProxies) > 0 { - return - } - - log.Warn( - "TRUSTED_PROXIES is empty: every rate limit keys on the "+ - "connecting peer's address. With nothing proxying to "+ - "this process that is the client itself and the limits "+ - "are per-client as intended. Behind a reverse proxy the "+ - "peer is the proxy on every request, so all clients "+ - "share one bucket per limit: the receiver limits become "+ - "service-wide ceilings, and one client's failed logins "+ - "throttle every other client's failed logins — a "+ - "correct password still gets in. If anything proxies to "+ - "this process, set TRUSTED_PROXIES to its 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. @@ -849,7 +812,6 @@ func New(lc fx.Lifecycle, params ConfigParams) (*Config, error) { "hasMetricsAuth", s.MetricsAuthEnabled(), ) - s.warnSharedRateLimitBucket(log) s.warnEgressAllowlist(log) return s, nil diff --git a/internal/config/config_test.go b/internal/config/config_test.go index a7cc76e..e3682e0 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -551,6 +551,11 @@ func testReceiverRateLimitSuccess( } func TestTrustedProxies(t *testing.T) { + // Unset, the RFC 1918 private ranges are trusted, so a reverse + // proxy on a Docker network or a private LAN is covered without + // configuration. + defaultProxies := []string{cidrPrivateV4, "172.16.0.0/12", "192.168.0.0/16"} + tests := []struct { name string set bool @@ -559,18 +564,21 @@ func TestTrustedProxies(t *testing.T) { expected []string }{ { - // The default must be "trust nobody": an empty list - // means forwarded headers are ignored, never that - // every peer may speak for the client. name: caseUnsetUsesDefault, set: false, - expected: []string{}, + expected: defaultProxies, }, { - name: "blank value trusts nothing", + name: "blank value uses default", set: true, value: " ", - expected: []string{}, + expected: defaultProxies, + }, + { + name: "set value replaces the default entirely", + set: true, + value: "203.0.113.7", + expected: []string{"203.0.113.7/32"}, }, { name: caseValidValueParsed, @@ -845,101 +853,6 @@ func TestEgressAllowlistWarning(t *testing.T) { } } -// TestSharedRateLimitBucketWarning covers the startup warning that -// tells an operator a deployment behind a reverse proxy shares one -// rate-limit bucket between every client, which turns the receiver -// limits into service-wide ceilings and collapses login failure -// counting. It must fire whenever TRUSTED_PROXIES is empty, in any -// environment, because behind a proxy every client shares one bucket -// in dev and prod alike. It stays quiet once proxies are named. -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, - }, - { - name: "dev without trusted proxies warns", - environment: config.EnvironmentDev, - expectWarning: true, - }, - { - name: "dev with trusted proxies is quiet", - environment: config.EnvironmentDev, - trustedProxies: cidrPrivateV4, - 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, "share one bucket") - assert.Contains( - t, logged, "throttle every other client's failed logins", - ) - // The warning must not claim a lockout the login - // endpoint no longer permits: credentials are verified - // before any budget is spent. - assert.Contains( - t, logged, "a correct password still gets in", - ) - // The text must stay accurate for a developer with - // nothing in front of the process, where an empty - // list costs nothing. - assert.Contains( - t, logged, "nothing proxying to this process", - ) - }) - } -} - // metricsEnv describes what one subtest below puts in the // environment for a single METRICS_ variable. A variable that is // set to the empty string and one that is not set at all are diff --git a/internal/config/export_test.go b/internal/config/export_test.go index d4d3be8..5b3ee92 100644 --- a/internal/config/export_test.go +++ b/internal/config/export_test.go @@ -6,21 +6,6 @@ import "log/slog" // 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 -} - // WarnEgressAllowlistForTest loads a Config from the current // environment and emits its egress-allowlist startup warning to // log, so a test can assert both that the warning fires only when diff --git a/internal/handlers/auth.go b/internal/handlers/auth.go index 39fa5dc..bef9dff 100644 --- a/internal/handlers/auth.go +++ b/internal/handlers/auth.go @@ -103,9 +103,10 @@ func (h *Handlers) renderLoginError( // The credential check runs BEFORE any rate-limit budget is // consulted, and only a failed check spends budget. That is what // keeps the single administrative path reachable: behind the reverse -// proxy this deployment requires, with TRUSTED_PROXIES unset, every -// client shares one bucket, so a limiter spent on arrival lets any -// stranger deny the operator's own correct password indefinitely. +// proxy this deployment requires, when TRUSTED_PROXIES does not cover +// it, every client shares one bucket, so a limiter spent on arrival +// lets any stranger deny the operator's own correct password +// indefinitely. // // Verifying first means every login POST costs an Argon2id hash, so // the work is taken under a bounded number of verification slots. diff --git a/internal/handlers/auth_test.go b/internal/handlers/auth_test.go index 98c64c8..07c417b 100644 --- a/internal/handlers/auth_test.go +++ b/internal/handlers/auth_test.go @@ -25,7 +25,7 @@ const ( // sharedProxyPeer is the whole point of this file. Production is // required to run behind a TLS-terminating reverse proxy, and - // TRUSTED_PROXIES defaults to empty, so every client — attacker + // when TRUSTED_PROXIES does not cover it every client — attacker // and operator alike — reaches the process from the proxy's // address and shares one rate-limit bucket. Both parties in // these tests therefore use the same RemoteAddr. @@ -115,11 +115,11 @@ func floodFailures( // done-criterion of https://git.eeqj.de/sneak/webhooker/issues/150. // // The attacker and the operator share one rate-limit bucket, because -// behind the mandated reverse proxy with TRUSTED_PROXIES unset every -// client keys on the proxy's address. The attacker floods the -// operator's own username — a single-admin product has a predictable -// one — far past the failure limit. The operator must still be able -// to log in with the correct password. +// behind the mandated reverse proxy, when TRUSTED_PROXIES does not +// cover it, every client keys on the proxy's address. The attacker +// floods the operator's own username — a single-admin product has a +// predictable one — far past the failure limit. The operator must +// still be able to log in with the correct password. // // This fails if credentials stop being verified ahead of the limiter. func TestLogin_StrangersFloodCannotLockOutTheOperator(t *testing.T) { diff --git a/internal/middleware/loginguard.go b/internal/middleware/loginguard.go index b69264b..2d8c385 100644 --- a/internal/middleware/loginguard.go +++ b/internal/middleware/loginguard.go @@ -108,10 +108,10 @@ type failureWindow struct { // // A limiter that spends budget on arrival cannot protect a // single-admin product: behind the reverse proxy the deployment -// requires, with TRUSTED_PROXIES unset, every client keys on the -// proxy, so a stranger trickling five POSTs a minute keeps the one -// bucket full and the operator's own correct password is answered 429 -// forever. There is no second administrative path. +// requires, when TRUSTED_PROXIES does not cover it, every client +// keys on the proxy, so a stranger trickling five POSTs a minute +// keeps the one bucket full and the operator's own correct password +// is answered 429 forever. There is no second administrative path. // // So budget is spent only by a FAILED verification. A correct // password is never throttled, whatever the counters say, which is diff --git a/internal/middleware/ratelimit.go b/internal/middleware/ratelimit.go index dc602ad..b3692d3 100644 --- a/internal/middleware/ratelimit.go +++ b/internal/middleware/ratelimit.go @@ -123,9 +123,8 @@ func bucketKey(addr netip.Addr) string { return prefix.String() } -// isTrustedProxy reports whether addr belongs to a network the -// operator listed in TRUSTED_PROXIES. The list is empty by default, -// so by default nothing is trusted. +// isTrustedProxy reports whether addr belongs to a network in +// TRUSTED_PROXIES, which by default is the RFC 1918 private ranges. func (m *Middleware) isTrustedProxy(addr netip.Addr) bool { for _, prefix := range m.params.Config.TrustedProxies { if prefix.Contains(addr) { diff --git a/internal/middleware/ratelimit_test.go b/internal/middleware/ratelimit_test.go index 678354f..75cdd1a 100644 --- a/internal/middleware/ratelimit_test.go +++ b/internal/middleware/ratelimit_test.go @@ -426,8 +426,8 @@ func assertSharedBucket( } // TestRateLimitKey_SpoofedForwardedFromUntrustedPeer is the test -// this gating exists for: with no trusted proxies configured (the -// default), a client that rotates a forwarded header on every +// this gating exists for: from a peer that is not a trusted +// proxy, a client that rotates a forwarded header on every // request must stay in one bucket. If forwarded headers were // trusted unconditionally, each spoofed value would mint a fresh // bucket and the limit would stop no one. diff --git a/internal/server/routes.go b/internal/server/routes.go index 05872f8..a15d6ca 100644 --- a/internal/server/routes.go +++ b/internal/server/routes.go @@ -140,11 +140,12 @@ func (s *Server) setupPageRoutes() { r.Use(s.mw.NoCache()) // The login POST carries no pre-emptive rate limiter. Behind - // the reverse proxy production requires, with TRUSTED_PROXIES - // unset, every client shares one bucket, so a limiter spent - // on arrival lets any stranger deny the operator the only - // administrative path. The handler verifies credentials first - // and charges only failures; see Handlers.authenticateUser. + // the reverse proxy production requires, when TRUSTED_PROXIES + // does not cover it, every client shares one bucket, so a + // limiter spent on arrival lets any stranger deny the operator + // the only administrative path. The handler verifies + // credentials first and charges only failures; see + // Handlers.authenticateUser. r.Get("/login", s.h.HandleLoginPage()) r.Post("/login", s.h.HandleLoginSubmit())