diff --git a/README.md b/README.md index e73b6b2..5a89c34 100644 --- a/README.md +++ b/README.md @@ -111,7 +111,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) | `""` (none) | +| `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 @@ -134,13 +134,15 @@ 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. +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. + +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 @@ -378,11 +380,12 @@ It uses: - **[gorilla/csrf](https://github.com/gorilla/csrf)** for CSRF protection (cookie-based double-submit tokens) - **[go-chi/httprate](https://github.com/go-chi/httprate)** for - sliding-window rate limiting of the login, password-change and - webhook receiver endpoints. The bucket is per client IP only when + 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 (see - [Rate Limiting](#rate-limiting)) + 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 @@ -1025,14 +1028,56 @@ opposite directions: 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 whenever it is - empty, in any environment. +- 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. + +#### The login endpoint + +The login `POST` is the one endpoint with no pre-emptive limiter in +front of it, and that is deliberate. A limiter that spends budget on +arrival is a lockout in this deployment shape: sharing one bucket, a +stranger sending five POSTs a minute — about 0.08 requests per second, +from anywhere — keeps it permanently full, and the operator has no +second administrative path. So the handler inverts the order: + +1. **Credentials are verified first, and only a failed attempt spends + budget.** A correct password is never rate-limited, whatever the + counters hold. This is what guarantees the admin UI stays + reachable. +2. **Failures are counted per (client bucket, submitted username)**, + five per minute, after which further _failures_ from that pair are + answered `429` with a `Retry-After`. A successful login clears the + counter, so mistyping a few times and then getting it right leaves + you unthrottled. Because the submitted username is + attacker-controlled, at most 1024 username counters and 1024 + fallback address counters are tracked; past the first cap failures + fall back to the address counter, and past both they are answered + as throttled without being recorded. Total tracked state is under + half a megabyte and does not grow with the number of usernames an + attacker invents. +3. **Concurrent password verifications are capped at two.** Verifying + before counting means every login request costs an Argon2id hash, + and Argon2id here is 64 MB per hash — two slots is a 128 MB ceiling + on password hashing. Every endpoint that hashes a password takes a + slot, including the password-change endpoint, which holds one + across both the verification and the new hash. A request that waits + five seconds without getting a slot is answered `503 Service + Unavailable` and no hash is computed for it. + +An unknown username is verified against a dummy hash rather than +rejected early, so a nonexistent account costs the same time as a real +one and the response cannot be used to enumerate usernames. + +The residual exposure is bounded and self-clearing: a flood can keep +both verification slots busy, so logins queue and some are shed with +`503` until it stops. That is degraded latency for everyone rather +than a permanent lockout of the operator, and an operator under one +can block the source at the reverse proxy or restart the service. 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 @@ -1053,8 +1098,8 @@ abuse limit later; they are tracked as future work. | Method | Path | Description | | ------ | --------------- | ----------- | -| `GET` | `/pages/login` | Login page (not rate limited; the limiter applies to POST only) | -| `POST` | `/pages/login` | Login form submission (5 per minute per bucket, then 429) | +| `GET` | `/pages/login` | Login page (not rate limited) | +| `POST` | `/pages/login` | Login form submission. Credentials are verified before any limit is consulted, so a correct password is never throttled; 5 FAILED attempts per minute per bucket per submitted username, then `429`. `503` if no verification slot frees up within 5s (see [Rate Limiting](#rate-limiting)) | | `POST` | `/pages/logout` | Logout (destroys session) | #### Authenticated Endpoints @@ -1062,7 +1107,7 @@ abuse limit later; they are tracked as future work. | Method | Path | Description | | ------ | ------------------------ | ----------- | | `GET` | `/user/{username}` | User profile page | -| `POST` | `/user/{username}/password` | Change the user's password (5 per minute per bucket, then 429) | +| `POST` | `/user/{username}/password` | Change the user's password (5 per minute per bucket, then `429`; `503` if no verification slot frees up within 5s) | | `GET` | `/sources` | List user's webhooks | | `GET` | `/sources/new` | Create webhook form | | `POST` | `/sources/new` | Create webhook submission | @@ -1164,6 +1209,7 @@ webhooker/ │ │ ├── middleware.go # Logging, CORS, Auth, Metrics, MetricsAuth, SecurityHeaders, MaxBodySize │ │ ├── csrf.go # CSRF protection middleware (gorilla/csrf) │ │ ├── ratelimit.go # Per-IP rate limiting middleware (go-chi/httprate) +│ │ ├── loginguard.go # Login failure counters and the Argon2id verification semaphore │ │ └── testing.go # NewForTest: Middleware without the fx lifecycle │ ├── server/ │ │ ├── server.go # Server struct, fx lifecycle, signal handling diff --git a/internal/config/config.go b/internal/config/config.go index a0413b6..0d5acb5 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -430,10 +430,14 @@ func loadFromEnv() (*Config, error) { // 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 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. +// 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. That // variable defaults to dev, so gating on it would silence the warning @@ -454,11 +458,11 @@ func (c *Config) warnSharedRateLimitBucket(log *slog.Logger) { "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 and any remote client can "+ - "keep the login limit full, denying the admin login — "+ - "the only administrative path — until restart. If "+ - "anything proxies to this process, set TRUSTED_PROXIES "+ - "to its address.", + "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), ) diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 11b521b..d7b3bc7 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -629,8 +629,9 @@ func testTrustedProxiesSuccess( // TestSharedRateLimitBucketWarning covers the startup warning that // tells an operator a deployment behind a reverse proxy shares one -// rate-limit bucket between every client, which makes the admin login -// remotely deniable. It must fire whenever TRUSTED_PROXIES is empty, +// 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: WEBHOOKER_ENVIRONMENT defaults to dev, so gating // on it would silence the warning for exactly the operator who never // configured the deployment. It stays quiet once proxies are named. @@ -707,7 +708,15 @@ func TestSharedRateLimitBucketWarning(t *testing.T) { assert.Contains(t, logged, `"level":"WARN"`) assert.Contains(t, logged, "TRUSTED_PROXIES") assert.Contains(t, logged, "share one bucket") - assert.Contains(t, logged, "denying the admin login") + 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. diff --git a/internal/database/export_test.go b/internal/database/export_test.go index ad4add1..8d38b86 100644 --- a/internal/database/export_test.go +++ b/internal/database/export_test.go @@ -65,3 +65,9 @@ func (r *RetentionReaper) ExportWedgeLoop( func (r *RetentionReaper) ExportSetInterval(d time.Duration) { r.interval = d } + +// DummyPasswordHashForTest exposes the encoded hash that unknown +// usernames are verified against. +func DummyPasswordHashForTest() string { + return dummyPasswordHash() +} diff --git a/internal/database/password.go b/internal/database/password.go index 24fe5f6..92ce50a 100644 --- a/internal/database/password.go +++ b/internal/database/password.go @@ -8,6 +8,7 @@ import ( "fmt" "math/big" "strings" + "sync" "golang.org/x/crypto/argon2" ) @@ -29,6 +30,10 @@ const hashParts = 6 // triggers per-character-class complexity enforcement. const minPasswordComplexityLen = 4 +// dummyPasswordLen is the length of the throwaway password behind +// dummyPasswordHash. +const dummyPasswordLen = 32 + // Sentinel errors returned by decodeHash. var ( errInvalidHashFormat = errors.New("invalid hash format") @@ -122,6 +127,38 @@ func VerifyPassword( return subtle.ConstantTimeCompare(hash, otherHash) == 1, nil } +// dummyPasswordHash is an encoded Argon2id hash of a random +// password, computed once on first use. Nothing can match it: the +// password it encodes is discarded as soon as it is hashed. It is +// process-wide because building it per request would add a second +// 64 MB Argon2id pass to every login for an unknown username. +// +//nolint:gochecknoglobals // computed once, see above +var dummyPasswordHash = sync.OnceValue(func() string { + password, err := GenerateRandomPassword(dummyPasswordLen) + if err != nil { + panic(fmt.Sprintf("generating the dummy password: %v", err)) + } + + hash, err := HashPassword(password) + if err != nil { + panic(fmt.Sprintf("hashing the dummy password: %v", err)) + } + + return hash +}) + +// VerifyDummyPassword performs a credential verification that cannot +// succeed, at the same cost as a real one. +// +// Login must charge an unknown username the same work as a known +// one. Returning early for an account that does not exist answers in +// microseconds where a real account takes tens of milliseconds, which +// is a username oracle any client can read off the response time. +func VerifyDummyPassword(password string) { + _, _ = VerifyPassword(password, dummyPasswordHash()) +} + // decodeHash extracts parameters, salt, and hash from an // encoded hash string. func decodeHash( diff --git a/internal/database/password_test.go b/internal/database/password_test.go index ae3e2e4..e3f4726 100644 --- a/internal/database/password_test.go +++ b/internal/database/password_test.go @@ -191,3 +191,41 @@ func TestHashPasswordUniqueness(t *testing.T) { ) } } + +// TestVerifyDummyPassword_DoesRealWork covers the anti-enumeration +// path. Login charges an unknown username a verification against a +// dummy hash so that a nonexistent account is not answered in +// microseconds where a real one takes tens of milliseconds. That only +// works if the dummy hash is a real, decodable Argon2id hash: a +// malformed one would make VerifyPassword fail on the decode and +// return before hashing anything. +func TestVerifyDummyPassword_DoesRealWork(t *testing.T) { + t.Parallel() + + // Runs the OnceValue that builds the dummy hash, so a panic in + // it surfaces here rather than on a live login. + database.VerifyDummyPassword("whatever was submitted") + + dummy := database.DummyPasswordHashForTest() + + // A hash the verifier cannot decode would make VerifyPassword + // return on the decode error, before hashing anything — the + // timing oracle this path exists to close. + valid, err := database.VerifyPassword("whatever", dummy) + if err != nil { + t.Fatalf( + "the dummy hash must decode like a real one: %v", err, + ) + } + + if valid { + t.Error("nothing may authenticate against the dummy hash") + } + + if !strings.HasPrefix(dummy, "$argon2id$") { + t.Errorf( + "the dummy hash must use the same algorithm as real "+ + "hashes, got %q", dummy, + ) + } +} diff --git a/internal/handlers/auth.go b/internal/handlers/auth.go index e79d7f5..9b01229 100644 --- a/internal/handlers/auth.go +++ b/internal/handlers/auth.go @@ -2,6 +2,7 @@ package handlers import ( "net/http" + "strconv" "sneak.berlin/go/webhooker/internal/database" ) @@ -93,6 +94,16 @@ func (h *Handlers) renderLoginError( // authenticateUser looks up and verifies a user's credentials. // On failure it writes an HTTP response and returns an error. +// +// 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. +// +// Verifying first means every login POST costs an Argon2id hash, so +// the work is taken under a bounded number of verification slots. func (h *Handlers) authenticateUser( w http.ResponseWriter, r *http.Request, @@ -100,16 +111,37 @@ func (h *Handlers) authenticateUser( ) (database.User, error) { var user database.User + release, ok := h.mw.BeginPasswordVerification(r.Context()) + if !ok { + h.log.Warn( + "password verification capacity exhausted", + "path", r.URL.Path, + ) + h.renderLoginError( + w, r, + "The server is busy verifying credentials. "+ + "Please try again.", + http.StatusServiceUnavailable, + ) + + return user, errVerificationBusy + } + + defer release() + err := h.db.DB().Where( "username = ?", username, ).First(&user).Error if err != nil { + // A username that does not exist is charged the same work + // as one that does. Skipping the hash here would answer in + // microseconds where a real account takes tens of + // milliseconds, handing every client a username oracle. + h.dummyVerifications.Add(1) + database.VerifyDummyPassword(password) + h.log.Debug("user not found", "username", username) - h.renderLoginError( - w, r, - "Invalid username or password", - http.StatusUnauthorized, - ) + h.rejectLogin(w, r, username) return user, err } @@ -127,16 +159,49 @@ func (h *Handlers) authenticateUser( if !valid { h.log.Debug("invalid password", "username", username) + h.rejectLogin(w, r, username) + + return user, errInvalidPassword + } + + // The password was correct, so forgive whatever failures this + // client accumulated: an operator who mistypes a few times and + // then gets it right must not stay throttled afterwards. + h.mw.ForgiveLoginFailures(r, username) + + return user, nil +} + +// rejectLogin counts one failed credential verification and answers +// it: 401 while this client still has failure budget against the +// submitted username, 429 with a Retry-After once it is spent. +// +// The 429 throttles wrong passwords only. A correct one never +// reaches here, so no amount of failure — from this client or any +// other sharing its bucket — can keep the operator out. +func (h *Handlers) rejectLogin( + w http.ResponseWriter, + r *http.Request, + username string, +) { + if !h.mw.RecordLoginFailure(r, username) { h.renderLoginError( w, r, "Invalid username or password", http.StatusUnauthorized, ) - return user, errInvalidPassword + return } - return user, nil + w.Header().Set("Retry-After", strconv.Itoa(int( + h.mw.LoginFailureInterval().Seconds(), + ))) + h.renderLoginError( + w, r, + "Too many failed login attempts. Please try again later.", + http.StatusTooManyRequests, + ) } // createAuthenticatedSession regenerates the session and stores diff --git a/internal/handlers/auth_test.go b/internal/handlers/auth_test.go new file mode 100644 index 0000000..98c64c8 --- /dev/null +++ b/internal/handlers/auth_test.go @@ -0,0 +1,455 @@ +package handlers_test + +import ( + "context" + "fmt" + "net/http" + "net/http/httptest" + "net/url" + "strings" + "sync" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "sneak.berlin/go/webhooker/internal/database" + "sneak.berlin/go/webhooker/internal/handlers" + "sneak.berlin/go/webhooker/internal/session" +) + +const ( + // operatorUser and operatorPassword are the single admin account + // these tests defend. + operatorUser = "admin" + operatorPassword = "correct horse battery staple" + + // 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 + // 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. + sharedProxyPeer = "10.0.0.1:44444" + + // loginFailureLimit is the failure budget one client has against + // one submitted username. Restated here rather than imported + // from the middleware package, so that changing the production + // limit fails these tests instead of silently moving with them. + loginFailureLimit = 5 +) + +// seedOperator gives the bootstrapped admin account a password these +// tests know. The account itself is created at startup with a random +// password, which is exactly why its username is predictable to an +// attacker and why keying failures by username alone does not fix +// this issue. +func seedOperator(t *testing.T, db *database.Database) { + t.Helper() + + hash, err := database.HashPassword(operatorPassword) + require.NoError(t, err) + + result := db.DB().Model(&database.User{}). + Where("username = ?", operatorUser). + Update("password", hash) + + require.NoError(t, result.Error) + require.EqualValues( + t, 1, result.RowsAffected, + "the bootstrap admin account must exist", + ) +} + +// loginPost builds a login form POST arriving from peer. +func loginPost(peer, username, password string) *http.Request { + form := url.Values{} + form.Set("username", username) + form.Set("password", password) + + req := httptest.NewRequestWithContext( + context.Background(), + http.MethodPost, + "/pages/login", + strings.NewReader(form.Encode()), + ) + req.Header.Set( + "Content-Type", "application/x-www-form-urlencoded", + ) + req.RemoteAddr = peer + + return req +} + +// submitLogin drives one login POST through the handler. +func submitLogin( + h *handlers.Handlers, peer, username, password string, +) *httptest.ResponseRecorder { + w := httptest.NewRecorder() + h.HandleLoginSubmit().ServeHTTP(w, loginPost( + peer, username, password, + )) + + return w +} + +// floodFailures sends attempts wrong-password logins for username +// from peer, which is what an attacker does. +func floodFailures( + t *testing.T, + h *handlers.Handlers, + peer, username string, + attempts int, +) { + t.Helper() + + for i := range attempts { + w := submitLogin(h, peer, username, fmt.Sprintf("guess-%d", i)) + require.NotEqual( + t, http.StatusSeeOther, w.Code, + "attempt %d must not authenticate", i, + ) + } +} + +// TestLogin_StrangersFloodCannotLockOutTheOperator is the +// 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. +// +// This fails if credentials stop being verified ahead of the limiter. +func TestLogin_StrangersFloodCannotLockOutTheOperator(t *testing.T) { + t.Parallel() + + var ( + h *handlers.Handlers + db *database.Database + ) + + app := newTestApp(t, &h, &db) + app.RequireStart() + + t.Cleanup(app.RequireStop) + + seedOperator(t, db) + + // Well past the limit, and from the same bucket the operator + // will arrive in. + floodFailures( + t, h, sharedProxyPeer, operatorUser, + loginFailureLimit*2, + ) + + w := submitLogin( + h, sharedProxyPeer, operatorUser, operatorPassword, + ) + + assert.Equal( + t, http.StatusSeeOther, w.Code, + "a correct password must never be throttled: the operator "+ + "has no second administrative path", + ) + assert.Equal(t, "/", w.Header().Get("Location")) +} + +// TestLogin_StrangersFloodCannotDenyAnotherAccount is the +// cross-account half: flooding one username must not spend another +// account's budget, even from the same shared bucket. +func TestLogin_StrangersFloodCannotDenyAnotherAccount(t *testing.T) { + t.Parallel() + + var ( + h *handlers.Handlers + db *database.Database + ) + + app := newTestApp(t, &h, &db) + app.RequireStart() + + t.Cleanup(app.RequireStop) + + seedOperator(t, db) + + floodFailures( + t, h, sharedProxyPeer, "someone-else", + loginFailureLimit*2, + ) + + w := submitLogin(h, sharedProxyPeer, operatorUser, "wrong") + + assert.Equal( + t, http.StatusUnauthorized, w.Code, + "a flood against one username must not spend another "+ + "account's failure budget", + ) +} + +// TestLogin_RepeatedWrongPasswordsAreThrottled is the brute-force +// half. Verifying before counting must not remove the throttle: +// repeated wrong passwords for one username from one client key run +// out of budget and are answered 429 with a Retry-After. +func TestLogin_RepeatedWrongPasswordsAreThrottled(t *testing.T) { + t.Parallel() + + var ( + h *handlers.Handlers + db *database.Database + ) + + app := newTestApp(t, &h, &db) + app.RequireStart() + + t.Cleanup(app.RequireStop) + + seedOperator(t, db) + + for i := range loginFailureLimit - 1 { + w := submitLogin( + h, sharedProxyPeer, operatorUser, + fmt.Sprintf("guess-%d", i), + ) + assert.Equal( + t, http.StatusUnauthorized, w.Code, + "attempt %d is still inside the budget", i, + ) + } + + w := submitLogin(h, sharedProxyPeer, operatorUser, "guess-last") + assert.Equal( + t, http.StatusTooManyRequests, w.Code, + "wrong passwords must still run out of budget", + ) + assert.NotEmpty( + t, w.Header().Get("Retry-After"), + "a throttled login must say when to come back", + ) +} + +// TestLogin_SuccessForgivesEarlierMistakes covers the operator who +// mistypes several times and then gets it right: the successful +// attempt clears the counter, so the next mistake is answered 401 +// rather than 429. +func TestLogin_SuccessForgivesEarlierMistakes(t *testing.T) { + t.Parallel() + + var ( + h *handlers.Handlers + db *database.Database + ) + + app := newTestApp(t, &h, &db) + app.RequireStart() + + t.Cleanup(app.RequireStop) + + seedOperator(t, db) + + floodFailures( + t, h, sharedProxyPeer, operatorUser, + loginFailureLimit, + ) + + require.Equal( + t, http.StatusSeeOther, + submitLogin( + h, sharedProxyPeer, operatorUser, operatorPassword, + ).Code, + ) + + w := submitLogin(h, sharedProxyPeer, operatorUser, "typo") + assert.Equal( + t, http.StatusUnauthorized, w.Code, + "a success must forgive the failures before it", + ) +} + +// TestLogin_UnknownUsernameCostsTheSameVerification is the +// username-enumeration guard. Verifying credentials before the +// limiter means response time is observable per attempt, so an +// unknown username must be charged an equivalent-cost verification +// against a dummy hash rather than returning early. +// +// The assertion is on the code path, not on wall-clock time: timing +// assertions are flaky, and what actually has to hold is that the +// hash is computed. +func TestLogin_UnknownUsernameCostsTheSameVerification(t *testing.T) { + t.Parallel() + + var ( + h *handlers.Handlers + db *database.Database + ) + + app := newTestApp(t, &h, &db) + app.RequireStart() + + t.Cleanup(app.RequireStop) + + seedOperator(t, db) + + require.Zero(t, h.DummyVerificationsForTest()) + + // A username that exists, with the wrong password: a real + // Argon2id verification runs, and no dummy is needed. + require.Equal( + t, http.StatusUnauthorized, + submitLogin(h, sharedProxyPeer, operatorUser, "wrong").Code, + ) + assert.Zero( + t, h.DummyVerificationsForTest(), + "a known username verifies against its own hash", + ) + + // A username that does not exist: indistinguishable response, + // and the equivalent-cost verification must have run. + require.Equal( + t, http.StatusUnauthorized, + submitLogin(h, sharedProxyPeer, "nosuchuser", "wrong").Code, + ) + assert.Equal( + t, uint64(1), h.DummyVerificationsForTest(), + "an unknown username must still pay for a hash, or the "+ + "response time says whether the account exists", + ) +} + +// TestLogin_ConcurrentLoginsAreAllAnswered covers the login path +// under the verification bound. The bound itself is pinned in the +// middleware package; what matters here is that funnelling every +// login through two slots does not lose or wedge a request — each one +// is answered, whether it got a slot or was shed with 503. +func TestLogin_ConcurrentLoginsAreAllAnswered(t *testing.T) { + t.Parallel() + + const workers = 4 + + var ( + h *handlers.Handlers + db *database.Database + ) + + app := newTestApp(t, &h, &db) + app.RequireStart() + + t.Cleanup(app.RequireStop) + + seedOperator(t, db) + + var ( + wg sync.WaitGroup + mu sync.Mutex + answers = map[int]int{} + ) + + for i := range workers { + wg.Go(func() { + w := submitLogin( + h, fmt.Sprintf("203.0.113.%d:5000", i), + operatorUser, fmt.Sprintf("guess-%d", i), + ) + + mu.Lock() + answers[w.Code]++ + mu.Unlock() + }) + } + + wg.Wait() + + mu.Lock() + defer mu.Unlock() + + assert.Zero( + t, answers[http.StatusInternalServerError], + "concurrent logins must not error", + ) + assert.Equal( + t, workers, + answers[http.StatusUnauthorized]+ + answers[http.StatusTooManyRequests]+ + answers[http.StatusServiceUnavailable], + "every concurrent login must be answered, whether it got "+ + "a verification slot or was shed with 503", + ) +} + +// TestLogin_MissingCredentialsRejectedBeforeAnyHash pins that the +// empty-field check still runs ahead of the verification slot, so a +// client sending nothing cannot occupy one. +func TestLogin_MissingCredentialsRejectedBeforeAnyHash(t *testing.T) { + t.Parallel() + + var ( + h *handlers.Handlers + db *database.Database + ) + + app := newTestApp(t, &h, &db) + app.RequireStart() + + t.Cleanup(app.RequireStop) + + seedOperator(t, db) + + w := submitLogin(h, sharedProxyPeer, "", "") + + assert.Equal(t, http.StatusBadRequest, w.Code) + assert.Zero( + t, h.DummyVerificationsForTest(), + "an empty submission must not cost a hash", + ) +} + +// TestLogin_SuccessCreatesSession is the control for the tests above: +// the success path they assert on really does authenticate. +func TestLogin_SuccessCreatesSession(t *testing.T) { + t.Parallel() + + var ( + h *handlers.Handlers + db *database.Database + sess *session.Session + ) + + app := newTestApp(t, &h, &db, &sess) + app.RequireStart() + + t.Cleanup(app.RequireStop) + + seedOperator(t, db) + + w := submitLogin( + h, sharedProxyPeer, operatorUser, operatorPassword, + ) + + require.Equal(t, http.StatusSeeOther, w.Code) + require.NotEmpty( + t, w.Result().Cookies(), "a session cookie must be issued", + ) + + next := httptest.NewRequestWithContext( + context.Background(), http.MethodGet, "/", nil, + ) + + // Login regenerates the session, so the response carries two + // Set-Cookie headers under the same name: one expiring the + // pre-login cookie and one issuing the new one. A browser keeps + // only the second, so replay only the one that is not an + // expiry. + for _, c := range w.Result().Cookies() { + if c.MaxAge >= 0 { + next.AddCookie(c) + } + } + + s, err := sess.Get(next) + require.NoError(t, err) + assert.True( + t, sess.IsAuthenticated(s), + "the issued cookie must carry an authenticated session", + ) +} diff --git a/internal/handlers/export_test.go b/internal/handlers/export_test.go index 78a6a59..b7d301f 100644 --- a/internal/handlers/export_test.go +++ b/internal/handlers/export_test.go @@ -11,6 +11,14 @@ import ( // to the handlers_test package. const MaxRenderedBodyBytesForTest = maxRenderedBodyBytes +// DummyVerificationsForTest reports how many equivalent-cost +// verifications were charged for usernames that do not exist. It +// lets a test prove the anti-enumeration path ran without timing +// anything. +func (s *Handlers) DummyVerificationsForTest() uint64 { + return s.dummyVerifications.Load() +} + // TrimPartialRuneForTest exposes trimPartialRune for use in the // handlers_test package. func TrimPartialRuneForTest(b []byte) []byte { diff --git a/internal/handlers/handlers.go b/internal/handlers/handlers.go index 79403a6..8d451bc 100644 --- a/internal/handlers/handlers.go +++ b/internal/handlers/handlers.go @@ -10,6 +10,7 @@ import ( "html/template" "log/slog" "net/http" + "sync/atomic" "go.uber.org/fx" "sneak.berlin/go/webhooker/internal/database" @@ -39,6 +40,12 @@ const ( // errInvalidPassword is returned when a password does not match. var errInvalidPassword = errors.New("invalid password") +// errVerificationBusy is returned when no password-verification slot +// became free before the wait elapsed, so no password was verified. +var errVerificationBusy = errors.New( + "password verification capacity exhausted", +) + //nolint:revive // HandlersParams is a standard fx naming convention. type HandlersParams struct { fx.In @@ -49,6 +56,7 @@ type HandlersParams struct { WebhookDBMgr *database.WebhookDBManager Healthcheck *healthcheck.Healthcheck Session *session.Session + Middleware *middleware.Middleware Notifier delivery.Notifier Evictor delivery.WebhookEvictor } @@ -62,9 +70,15 @@ type Handlers struct { db *database.Database dbMgr *database.WebhookDBManager session *session.Session + mw *middleware.Middleware notifier delivery.Notifier evictor delivery.WebhookEvictor templates map[string]*template.Template + + // dummyVerifications counts the equivalent-cost verifications + // charged for usernames that do not exist. It exists so a test + // can prove that path runs without measuring wall-clock time. + dummyVerifications atomic.Uint64 } // parsePageTemplate parses a page-specific template set from the @@ -97,6 +111,7 @@ func New( s.db = params.Database s.dbMgr = params.WebhookDBMgr s.session = params.Session + s.mw = params.Middleware s.notifier = params.Notifier s.evictor = params.Evictor diff --git a/internal/handlers/handlers_test.go b/internal/handlers/handlers_test.go index 3390fe0..83269d0 100644 --- a/internal/handlers/handlers_test.go +++ b/internal/handlers/handlers_test.go @@ -20,6 +20,7 @@ import ( "sneak.berlin/go/webhooker/internal/handlers" "sneak.berlin/go/webhooker/internal/healthcheck" "sneak.berlin/go/webhooker/internal/logger" + "sneak.berlin/go/webhooker/internal/middleware" "sneak.berlin/go/webhooker/internal/session" ) @@ -82,6 +83,7 @@ func newTestApp( func(r *recordingEvictor) delivery.WebhookEvictor { return r }, + middleware.New, handlers.New, ), fx.Populate(targets...), diff --git a/internal/handlers/profile.go b/internal/handlers/profile.go index 027027e..7009383 100644 --- a/internal/handlers/profile.go +++ b/internal/handlers/profile.go @@ -1,6 +1,7 @@ package handlers import ( + "context" "net/http" "github.com/go-chi/chi" @@ -42,6 +43,7 @@ func (h *Handlers) HandlePasswordChange() http.HandlerFunc { } successMessage, errorMessage, handled := h.applyPasswordChange( + r.Context(), w, sessionUsername, r.FormValue("current_password"), @@ -66,9 +68,30 @@ func (h *Handlers) HandlePasswordChange() http.HandlerFunc { // 500 response itself and returns handled=false, signalling the caller // to stop without re-rendering the page. func (h *Handlers) applyPasswordChange( + ctx context.Context, w http.ResponseWriter, username, currentPassword, newPassword, confirmPassword string, ) (string, string, bool) { + // This endpoint verifies one password and hashes another, at + // 64 MB each, so it takes a slot from the same bound the login + // endpoint uses. The bound is per hash, not per endpoint: leaving + // this path outside it would leave a hole in it. The slot is held + // across both hashes. + release, ok := h.mw.BeginPasswordVerification(ctx) + if !ok { + h.log.Warn("password verification capacity exhausted") + http.Error( + w, + "The server is busy verifying credentials. "+ + "Please try again.", + http.StatusServiceUnavailable, + ) + + return "", "", false + } + + defer release() + // Load the user row so we can verify the current password and // persist the new hash. var user database.User diff --git a/internal/middleware/export_test.go b/internal/middleware/export_test.go index ecf7d98..8c1c6aa 100644 --- a/internal/middleware/export_test.go +++ b/internal/middleware/export_test.go @@ -1,7 +1,9 @@ package middleware import ( + "context" "net/http" + "time" ) // NewLoggingResponseWriterForTest wraps newLoggingResponseWriter @@ -35,9 +37,69 @@ func IsClientTLS(r *http.Request) bool { return isClientTLS(r) } -// LoginRateLimitConst exposes the loginRateLimit constant. +// LoginRateLimitConst exposes the loginRateLimit constant: the +// number of FAILED login attempts one client may make against one +// submitted username per interval. const LoginRateLimitConst = loginRateLimit +// LoginFailureMaxKeysConst exposes the cap on each of the login +// guard's key sets. +const LoginFailureMaxKeysConst = loginFailureMaxKeys + +// PasswordVerifyConcurrencyConst exposes the bound on concurrent +// Argon2id verifications. +const PasswordVerifyConcurrencyConst = passwordVerifyConcurrency + +// LoginGuard is the login failure counter and verification +// semaphore, exposed for direct testing. +type LoginGuard = loginGuard + +// NewLoginGuardForTest builds a guard with test-sized parameters. +func NewLoginGuardForTest( + limit int, + interval time.Duration, + maxKeys, concurrency int, + wait time.Duration, +) *LoginGuard { + return newLoginGuard( + limit, interval, maxKeys, concurrency, wait, + ) +} + +// SetNowForTest replaces the guard's clock. +func (g *LoginGuard) SetNowForTest(now func() time.Time) { + g.mu.Lock() + defer g.mu.Unlock() + + g.now = now +} + +// FailForTest exposes fail. +func (g *LoginGuard) FailForTest(clientKey, username string) bool { + return g.fail(clientKey, username) +} + +// SucceedForTest exposes succeed. +func (g *LoginGuard) SucceedForTest(clientKey, username string) { + g.succeed(clientKey, username) +} + +// AcquireForTest exposes acquire. +func (g *LoginGuard) AcquireForTest( + ctx context.Context, +) (func(), bool) { + return g.acquire(ctx) +} + +// TrackedKeysForTest reports how many failure counters the guard +// holds, per-username and per-address respectively. +func (g *LoginGuard) TrackedKeysForTest() (int, int) { + g.mu.Lock() + defer g.mu.Unlock() + + return len(g.byUser), len(g.byAddr) +} + // PasswordChangeRateLimitConst exposes the // passwordChangeRateLimit constant. const PasswordChangeRateLimitConst = passwordChangeRateLimit diff --git a/internal/middleware/loginguard.go b/internal/middleware/loginguard.go new file mode 100644 index 0000000..9702d7d --- /dev/null +++ b/internal/middleware/loginguard.go @@ -0,0 +1,299 @@ +package middleware + +import ( + "context" + "crypto/sha256" + "encoding/hex" + "net/http" + "sync" + "time" +) + +const ( + // loginFailureMaxKeys bounds how many distinct failure counters + // each of the guard's two key sets holds. The submitted username + // is part of a key, so the key set is attacker-influenced and + // needs a hard cap or the limiter becomes the memory + // amplification surface it exists to protect. + // + // A single-admin deployment has a handful of legitimate (client, + // username) pairs, so 1024 is three orders of magnitude of + // headroom before a real operator can be pushed onto the + // fallback. It costs little: a counter is a ~64-byte key string, + // a 32-byte window and map overhead, call it 170 bytes, so both + // key sets full is 2 * 1024 * 170 bytes, under 0.4 MB. + loginFailureMaxKeys = 1024 + + // passwordVerifyConcurrency bounds how many Argon2id + // verifications may run at once across every password-verifying + // endpoint. Because credentials are now verified before any + // limiter budget is spent, an attacker can force one hash per + // request, and each hash allocates argon2Memory — 64 MB. Two + // slots commit at most 128 MB to password hashing, which fits + // inside the smallest container this service is realistically + // given alongside its own working set; four would commit 256 MB + // and crowd it. A single-admin product needs no concurrent + // logins at all, so the second slot exists only so that one + // stalled request does not serialise the endpoint. + passwordVerifyConcurrency = 2 + + // passwordVerifyWait is how long a request waits for a + // verification slot before it is answered 503. Slots are handed + // out in arrival order, so a legitimate request queues behind + // the requests already waiting rather than behind the flood as a + // whole. The wait is well inside the 60s request timeout. + passwordVerifyWait = 5 * time.Second + + // failureKeyHashBytes is how much of the username digest goes + // into a failure key. 64 bits over at most loginFailureMaxKeys + // live keys makes a collision negligible, and a collision would + // only merge two usernames' failure counters, which throttles + // sooner rather than later. + failureKeyHashBytes = 8 +) + +// failureWindow counts failed credential verifications for one +// bucket, and records when that count lapses. +type failureWindow struct { + count int + resetAt time.Time +} + +// loginGuard is what replaced the pre-emptive rate limiter on the +// login POST. +// +// 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. +// +// So budget is spent only by a FAILED verification. A correct +// password is never throttled, whatever the counters say, which is +// the only shape that guarantees the operator can get in. Two +// consequences follow and are handled here: +// +// - Every login request now costs an Argon2id hash, so the number +// running concurrently is bounded by slots. Without that bound +// this trades an admin lockout for memory exhaustion, which is +// strictly worse. +// - Counting per (client, username) makes the key set +// attacker-influenced, so both key sets are capped. Beyond the +// per-username cap, failures fall back to a counter keyed on the +// client alone; beyond that cap too, a failure is answered as +// throttled without being recorded, since refusing to answer a +// wrong password costs the operator nothing. +type loginGuard struct { + mu sync.Mutex + byUser map[string]*failureWindow + byAddr map[string]*failureWindow + + slots chan struct{} + + limit int + interval time.Duration + maxKeys int + wait time.Duration + + // now is time.Now outside tests. + now func() time.Time +} + +// newLoginGuard builds a guard with the given failure limit per +// interval, key-set cap, verification concurrency and slot wait. +func newLoginGuard( + limit int, + interval time.Duration, + maxKeys, concurrency int, + wait time.Duration, +) *loginGuard { + return &loginGuard{ + byUser: make(map[string]*failureWindow), + byAddr: make(map[string]*failureWindow), + slots: make(chan struct{}, concurrency), + limit: limit, + interval: interval, + maxKeys: maxKeys, + wait: wait, + now: time.Now, + } +} + +// acquire reserves a verification slot, waiting up to the guard's +// wait for one. It reports false when none became available or the +// request was cancelled first; the caller must then answer 503 +// without verifying anything. The returned function releases the +// slot and must be called exactly once. +func (g *loginGuard) acquire(ctx context.Context) (func(), bool) { + timer := time.NewTimer(g.wait) + defer timer.Stop() + + select { + case g.slots <- struct{}{}: + return func() { <-g.slots }, true + case <-timer.C: + return nil, false + case <-ctx.Done(): + return nil, false + } +} + +// fail records one failed credential verification by clientKey +// against username, and reports whether this client has now spent +// its failure budget and should be answered 429. +func (g *loginGuard) fail(clientKey, username string) bool { + g.mu.Lock() + defer g.mu.Unlock() + + now := g.now() + + window := g.window( + g.byUser, userFailureKey(clientKey, username), now, + ) + if window == nil { + window = g.window(g.byAddr, clientKey, now) + } + + if window == nil { + // Both key sets are full and neither already tracks this + // client, so nothing can be counted without unbounded + // growth. Answering the failure as throttled is the safe + // direction: it never touches a correct password. + return true + } + + window.count++ + + return window.count >= g.limit +} + +// succeed forgives clientKey's failures against username. A correct +// password clears the counters, so an operator who mistypes several +// times and then gets it right is not throttled afterwards. +func (g *loginGuard) succeed(clientKey, username string) { + g.mu.Lock() + defer g.mu.Unlock() + + delete(g.byUser, userFailureKey(clientKey, username)) + delete(g.byAddr, clientKey) +} + +// window returns the live counter for key in set, resetting a lapsed +// one and creating a missing one when the cap allows. It returns nil +// only when key is absent and set is full even after lapsed entries +// are swept. +func (g *loginGuard) window( + set map[string]*failureWindow, + key string, + now time.Time, +) *failureWindow { + window, ok := set[key] + if ok { + if !now.Before(window.resetAt) { + window.count = 0 + window.resetAt = now.Add(g.interval) + } + + return window + } + + if len(set) >= g.maxKeys { + sweepLapsed(set, now) + } + + if len(set) >= g.maxKeys { + return nil + } + + window = &failureWindow{resetAt: now.Add(g.interval)} + set[key] = window + + return window +} + +// sweepLapsed drops counters whose interval has elapsed. +func sweepLapsed(set map[string]*failureWindow, now time.Time) { + for key, window := range set { + if !now.Before(window.resetAt) { + delete(set, key) + } + } +} + +// userFailureKey identifies one (client, submitted username) pair. +// The username is hashed rather than embedded: a submitted username +// is attacker-controlled text of attacker-chosen length, and hashing +// makes every key the same size whatever was sent. +func userFailureKey(clientKey, username string) string { + sum := sha256.Sum256([]byte(username)) + + return clientKey + "|" + + hex.EncodeToString(sum[:failureKeyHashBytes]) +} + +// guard returns the middleware's login guard, building it on first +// use so that every construction path — fx and the test constructor +// alike — gets one. +func (m *Middleware) guard() *loginGuard { + m.loginGuardOnce.Do(func() { + m.loginGuard = newLoginGuard( + loginRateLimit, + loginRateInterval, + loginFailureMaxKeys, + passwordVerifyConcurrency, + passwordVerifyWait, + ) + }) + + return m.loginGuard +} + +// BeginPasswordVerification reserves one of the bounded Argon2id +// verification slots. It reports false when none became free within +// passwordVerifyWait, in which case the caller must answer 503 and +// must not verify a password. The returned function releases the +// slot and must be called exactly once. +// +// Every endpoint that hashes a password on request must go through +// this, or the bound has a hole: the memory is committed per hash, +// not per endpoint. +func (m *Middleware) BeginPasswordVerification( + ctx context.Context, +) (func(), bool) { + return m.guard().acquire(ctx) +} + +// RecordLoginFailure counts a failed credential verification for the +// request's client against the submitted username, and reports +// whether the response should be 429 rather than 401. +func (m *Middleware) RecordLoginFailure( + r *http.Request, + username string, +) bool { + throttled := m.guard().fail(m.clientKey(r), username) + if throttled { + m.log.Warn( + "login failure limit exceeded", "path", r.URL.Path, + ) + } + + return throttled +} + +// ForgiveLoginFailures clears the failure counters for the request's +// client and the submitted username after a successful +// authentication. +func (m *Middleware) ForgiveLoginFailures( + r *http.Request, + username string, +) { + m.guard().succeed(m.clientKey(r), username) +} + +// LoginFailureInterval is how long a spent login failure budget +// takes to refill, which is what a throttled login answers as +// Retry-After. +func (m *Middleware) LoginFailureInterval() time.Duration { + return m.guard().interval +} diff --git a/internal/middleware/loginguard_test.go b/internal/middleware/loginguard_test.go new file mode 100644 index 0000000..05edb54 --- /dev/null +++ b/internal/middleware/loginguard_test.go @@ -0,0 +1,352 @@ +package middleware_test + +import ( + "context" + "fmt" + "sync" + "sync/atomic" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "sneak.berlin/go/webhooker/internal/middleware" +) + +const ( + // guardInterval is the failure window these tests use. It is + // long enough that nothing lapses mid-test on its own; tests + // that need a lapse drive the clock instead. + guardInterval = time.Minute + + // guardWait is the slot wait for tests that expect to get a + // slot. Tests that expect to be refused set their own. + guardWait = 2 * time.Second + + guardClient = "198.51.100.7" + guardUser = "admin" +) + +// newGuard builds a guard with production-shaped defaults and the +// given key-set cap and verification concurrency. +func newGuard(maxKeys, concurrency int) *middleware.LoginGuard { + return middleware.NewLoginGuardForTest( + middleware.LoginRateLimitConst, + guardInterval, + maxKeys, + concurrency, + guardWait, + ) +} + +// TestLoginGuard_ThrottlesRepeatedFailures is the brute-force half: +// wrong passwords for one username from one client key still run out +// of budget and are answered 429. +func TestLoginGuard_ThrottlesRepeatedFailures(t *testing.T) { + t.Parallel() + + g := newGuard(middleware.LoginFailureMaxKeysConst, 1) + + for i := range middleware.LoginRateLimitConst - 1 { + assert.False( + t, g.FailForTest(guardClient, guardUser), + "failure %d is still inside the budget", i, + ) + } + + assert.True( + t, g.FailForTest(guardClient, guardUser), + "the last failure of the budget must throttle", + ) + assert.True( + t, g.FailForTest(guardClient, guardUser), + "failures past the budget must stay throttled", + ) +} + +// TestLoginGuard_SuccessForgivesFailures pins the forgiveness rule: +// an operator who mistypes several times and then gets it right must +// not be left throttled. +func TestLoginGuard_SuccessForgivesFailures(t *testing.T) { + t.Parallel() + + g := newGuard(middleware.LoginFailureMaxKeysConst, 1) + + for range middleware.LoginRateLimitConst { + g.FailForTest(guardClient, guardUser) + } + + g.SucceedForTest(guardClient, guardUser) + + assert.False( + t, g.FailForTest(guardClient, guardUser), + "a success must reset the counter, so the next mistake "+ + "starts a fresh budget", + ) +} + +// TestLoginGuard_FailuresAreKeyedPerUsername proves the second half +// of the keying: one username's spent budget does not throttle +// another's from the same client. +func TestLoginGuard_FailuresAreKeyedPerUsername(t *testing.T) { + t.Parallel() + + g := newGuard(middleware.LoginFailureMaxKeysConst, 1) + + for range middleware.LoginRateLimitConst { + g.FailForTest(guardClient, guardUser) + } + + assert.True(t, g.FailForTest(guardClient, guardUser)) + assert.False( + t, g.FailForTest(guardClient, "someone-else"), + "a different submitted username must have its own budget", + ) +} + +// TestLoginGuard_WindowLapses covers the interval: a counter that has +// gone quiet for the whole window starts again from zero. +func TestLoginGuard_WindowLapses(t *testing.T) { + t.Parallel() + + g := newGuard(middleware.LoginFailureMaxKeysConst, 1) + + var now atomic.Int64 + + now.Store(time.Now().UnixNano()) + g.SetNowForTest(func() time.Time { + return time.Unix(0, now.Load()) + }) + + for range middleware.LoginRateLimitConst { + g.FailForTest(guardClient, guardUser) + } + + assert.True(t, g.FailForTest(guardClient, guardUser)) + + now.Add(int64(guardInterval) + 1) + + assert.False( + t, g.FailForTest(guardClient, guardUser), + "a lapsed window must start a fresh budget", + ) +} + +// TestLoginGuard_UsernameKeySetIsBounded is the memory bound. The +// submitted username is attacker-controlled, so an attacker rotating +// usernames must not be able to grow the guard without limit: past +// the cap, tracking falls back to a counter keyed on the client +// address alone. +func TestLoginGuard_UsernameKeySetIsBounded(t *testing.T) { + t.Parallel() + + const ( + maxKeys = 8 + attempts = 500 + ) + + g := newGuard(maxKeys, 1) + + for i := range attempts { + g.FailForTest(guardClient, fmt.Sprintf("user-%d", i)) + } + + byUser, byAddr := g.TrackedKeysForTest() + + assert.LessOrEqual( + t, byUser, maxKeys, + "the per-username key set must not grow past its cap", + ) + assert.LessOrEqual( + t, byAddr, maxKeys, + "the fallback key set must not grow past its cap either", + ) + assert.Positive( + t, byAddr, + "past the cap, failures must fall back to the address "+ + "bucket rather than being dropped", + ) + assert.Less( + t, byUser+byAddr, attempts, + "memory must not grow with the number of distinct "+ + "usernames submitted", + ) +} + +// TestLoginGuard_BeyondBothCapsStaysThrottled covers the hard stop. +// When both key sets are full of live counters and the client is in +// neither, there is nothing to count without unbounded growth, so the +// failure is answered as throttled. That costs the operator nothing: +// a correct password never reaches this path. +func TestLoginGuard_BeyondBothCapsStaysThrottled(t *testing.T) { + t.Parallel() + + const maxKeys = 4 + + g := newGuard(maxKeys, 1) + + // Fill the per-username set from one client, then fill the + // address set from distinct clients. + for i := range maxKeys { + g.FailForTest(guardClient, fmt.Sprintf("user-%d", i)) + } + + for i := range maxKeys { + g.FailForTest(fmt.Sprintf("203.0.113.%d", i), "whoever") + } + + assert.True( + t, g.FailForTest("203.0.113.200", "brand-new"), + "a client that fits in neither full key set must be "+ + "answered as throttled rather than tracked", + ) + + byUser, byAddr := g.TrackedKeysForTest() + assert.LessOrEqual(t, byUser, maxKeys) + assert.LessOrEqual(t, byAddr, maxKeys) +} + +// TestLoginGuard_SemaphoreBoundsConcurrentVerifications is the memory +// bound on the hashing itself. Verifying credentials before spending +// limiter budget means an attacker can force one Argon2id hash per +// request, and each allocates 64 MB; without this bound the fix for +// an admin lockout would be a memory-exhaustion DoS instead. +func TestLoginGuard_SemaphoreBoundsConcurrentVerifications( + t *testing.T, +) { + t.Parallel() + + const ( + concurrency = 2 + workers = 12 + ) + + g := newGuard(middleware.LoginFailureMaxKeysConst, concurrency) + + var ( + mu sync.Mutex + inside int + highest int + wg sync.WaitGroup + ) + + for range workers { + wg.Go(func() { + release, ok := g.AcquireForTest(context.Background()) + if !ok { + return + } + + defer release() + + mu.Lock() + + inside++ + if inside > highest { + highest = inside + } + + mu.Unlock() + + // Hold the slot long enough that the other workers are + // certainly contending for it. + time.Sleep(10 * time.Millisecond) + + mu.Lock() + inside-- + mu.Unlock() + }) + } + + wg.Wait() + + mu.Lock() + defer mu.Unlock() + + assert.Equal( + t, concurrency, highest, + "no more than %d verifications may run at once", concurrency, + ) +} + +// TestLoginGuard_SaturatedSemaphoreRefusesRatherThanQueueing pins +// what happens when every slot is taken for longer than the wait: the +// request is refused, so the caller answers 503 without allocating +// another 64 MB hash. +func TestLoginGuard_SaturatedSemaphoreRefusesRatherThanQueueing( + t *testing.T, +) { + t.Parallel() + + g := middleware.NewLoginGuardForTest( + middleware.LoginRateLimitConst, + guardInterval, + middleware.LoginFailureMaxKeysConst, + 1, + 10*time.Millisecond, + ) + + release, ok := g.AcquireForTest(context.Background()) + require.True(t, ok, "the first acquire must get the only slot") + + _, ok = g.AcquireForTest(context.Background()) + assert.False( + t, ok, + "with the only slot held, a second request must be refused "+ + "rather than wait indefinitely", + ) + + release() + + release, ok = g.AcquireForTest(context.Background()) + assert.True( + t, ok, "the slot must be reusable once released", + ) + + release() +} + +// TestLoginGuard_AcquireHonoursCancellation proves a client that +// disconnects while queued frees its place immediately instead of +// holding it for the full wait. +func TestLoginGuard_AcquireHonoursCancellation(t *testing.T) { + t.Parallel() + + g := newGuard(middleware.LoginFailureMaxKeysConst, 1) + + release, ok := g.AcquireForTest(context.Background()) + require.True(t, ok) + + defer release() + + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + _, ok = g.AcquireForTest(ctx) + assert.False( + t, ok, "a cancelled request must not wait for a slot", + ) +} + +// TestPasswordVerifyConcurrency_MatchesMemoryBudget pins the +// concurrency constant to the arithmetic behind it: Argon2id here is +// 64 MB per hash, so the number of slots is the number of 64 MB +// allocations the process is willing to commit to password hashing. +// Raising it raises peak resident memory by 64 MB a slot. +func TestPasswordVerifyConcurrency_MatchesMemoryBudget(t *testing.T) { + t.Parallel() + + const ( + argon2MemoryMB = 64 + budgetMB = 128 + ) + + assert.Equal( + t, + budgetMB/argon2MemoryMB, + middleware.PasswordVerifyConcurrencyConst, + "the verification concurrency is %d MB of Argon2id memory "+ + "divided by %d MB per hash", + budgetMB, argon2MemoryMB, + ) +} diff --git a/internal/middleware/middleware.go b/internal/middleware/middleware.go index 975ecc0..2b61087 100644 --- a/internal/middleware/middleware.go +++ b/internal/middleware/middleware.go @@ -6,6 +6,7 @@ import ( "log/slog" "net" "net/http" + "sync" "time" basicauth "github.com/99designs/basicauth-go" @@ -43,6 +44,12 @@ type Middleware struct { log *slog.Logger params *MiddlewareParams session *session.Session + + // loginGuard counts failed credential verifications and bounds + // concurrent password hashing. It is built on first use so that + // every construction path gets one; see guard(). + loginGuardOnce sync.Once + loginGuard *loginGuard } // New creates a Middleware from the provided fx parameters. diff --git a/internal/middleware/ratelimit.go b/internal/middleware/ratelimit.go index 0de9fd1..7efca06 100644 --- a/internal/middleware/ratelimit.go +++ b/internal/middleware/ratelimit.go @@ -12,11 +12,15 @@ import ( ) const ( - // loginRateLimit is the maximum number of login attempts - // per interval. + // loginRateLimit is the maximum number of FAILED login attempts + // one client may make against one submitted username per + // interval before further failures are answered 429. Successful + // attempts are never counted and never throttled — see + // loginGuard. loginRateLimit = 5 - // loginRateInterval is the time window for the rate limit. + // loginRateInterval is the time window for the login failure + // limit. loginRateInterval = 1 * time.Minute // passwordChangeRateLimit is the maximum number of password @@ -216,7 +220,7 @@ func (m *Middleware) clientKey(r *http.Request) string { return bucketKey(peer) } -// tooManyRequests returns the 429 handler used by the login, +// tooManyRequests returns the 429 handler used by the // password-change and per-entrypoint receiver limiters: it logs the // rejection with logMessage and answers with responseMessage. // httprate adds the Retry-After header (RFC 6585). The aggregate @@ -255,26 +259,15 @@ func (m *Middleware) floodTooManyRequests( } } -// LoginRateLimit returns middleware that enforces per-IP rate -// limiting on login attempts using go-chi/httprate. Only POST -// requests are rate-limited; GET requests (rendering the login -// form) pass through unaffected. When the rate limit is exceeded, -// a 429 Too Many Requests response is returned. Clients are -// identified by rateLimitKey. -func (m *Middleware) LoginRateLimit() func(http.Handler) http.Handler { - return m.postRateLimit( - loginRateLimit, - loginRateInterval, - "login rate limit exceeded", - "Too many login attempts. Please try again later.", - ) -} - // PasswordChangeRateLimit returns middleware that enforces // per-IP rate limiting on password change attempts. The change // endpoint verifies the current password, so without a limit a -// stolen session could be used to brute-force it; the limit -// matches the login endpoint's. +// stolen session could be used to brute-force it. +// +// Unlike the login POST this limit is still spent on arrival, which +// is safe here: RequireAuth runs ahead of it, so only a request +// already carrying a valid session can reach the bucket, and an +// operator locked out of changing a password can still log in. func (m *Middleware) PasswordChangeRateLimit() func(http.Handler) http.Handler { return m.postRateLimit( passwordChangeRateLimit, diff --git a/internal/middleware/ratelimit_test.go b/internal/middleware/ratelimit_test.go index 2d22a3f..678354f 100644 --- a/internal/middleware/ratelimit_test.go +++ b/internal/middleware/ratelimit_test.go @@ -20,14 +20,14 @@ import ( "sneak.berlin/go/webhooker/internal/middleware" ) -func TestLoginRateLimit_AllowsGET(t *testing.T) { +func TestPostRateLimit_AllowsGET(t *testing.T) { t.Parallel() m, _ := testMiddleware(t, config.EnvironmentDev) var callCount int - handler := m.LoginRateLimit()(http.HandlerFunc( + handler := m.PasswordChangeRateLimit()(http.HandlerFunc( func(w http.ResponseWriter, _ *http.Request) { callCount++ @@ -39,7 +39,7 @@ func TestLoginRateLimit_AllowsGET(t *testing.T) { for i := range 20 { req := httptest.NewRequestWithContext( context.Background(), - http.MethodGet, "/pages/login", nil, + http.MethodGet, "/user/admin/password", nil, ) req.RemoteAddr = "192.168.1.1:12345" @@ -110,20 +110,6 @@ func runPostLimitTest( assert.Equal(t, limit, callCount) } -func TestLoginRateLimit_LimitsPOST(t *testing.T) { - t.Parallel() - - m, _ := testMiddleware(t, config.EnvironmentDev) - - runPostLimitTest( - t, - m.LoginRateLimit(), - middleware.LoginRateLimitConst, - "/pages/login", - "10.0.0.1:12345", - ) -} - func TestPasswordChangeRateLimit_LimitsPOST(t *testing.T) { t.Parallel() @@ -138,19 +124,19 @@ func TestPasswordChangeRateLimit_LimitsPOST(t *testing.T) { ) } -func TestLoginRateLimit_IndependentPerIP(t *testing.T) { +func TestPostRateLimit_IndependentPerIP(t *testing.T) { t.Parallel() m, _ := testMiddleware(t, config.EnvironmentDev) - handler := m.LoginRateLimit()(http.HandlerFunc( + handler := m.PasswordChangeRateLimit()(http.HandlerFunc( func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusOK) }, )) // Exhaust limit for IP1 - for range middleware.LoginRateLimitConst { + for range middleware.PasswordChangeRateLimitConst { req := httptest.NewRequestWithContext( context.Background(), http.MethodPost, "/pages/login", nil, @@ -367,7 +353,14 @@ func TestReceiverRateLimit_CountsEveryMethod(t *testing.T) { } const ( - loginPath = "/pages/login" + // limitedPath is the endpoint these tests drive the shared POST + // rate limiter through. It is the password-change path: since + // the login POST verifies credentials before spending any + // budget, the password-change limiter is the only pre-emptive + // POST limiter left, and it is what pins the shared key + // function's behaviour here. + limitedPath = "/user/admin/password" + headerXFF = "X-Forwarded-For" headerReal = "X-Real-IP" headerTrue = "True-Client-IP" @@ -415,10 +408,10 @@ func assertSharedBucket( m := rateLimitMiddleware( t, &config.Config{TrustedProxies: proxies}, ) - handler := m.LoginRateLimit()(okHandler()) + handler := m.PasswordChangeRateLimit()(okHandler()) - for i := range middleware.LoginRateLimitConst { - w := postWithHeaders(handler, peer, loginPath, headers(i)) + for i := range middleware.PasswordChangeRateLimitConst { + w := postWithHeaders(handler, peer, limitedPath, headers(i)) assert.Equal( t, http.StatusOK, w.Code, "request %d should pass", i, @@ -426,8 +419,8 @@ func assertSharedBucket( } w := postWithHeaders( - handler, peer, loginPath, - headers(middleware.LoginRateLimitConst), + handler, peer, limitedPath, + headers(middleware.PasswordChangeRateLimitConst), ) assert.Equal(t, http.StatusTooManyRequests, w.Code, msg) } @@ -549,24 +542,24 @@ func TestRateLimitKey_ForwardedHonouredFromTrustedPeer( m := rateLimitMiddleware(t, &config.Config{ TrustedProxies: trustedProxies(trustedProxyCIDR), }) - handler := m.LoginRateLimit()(okHandler()) + handler := m.PasswordChangeRateLimit()(okHandler()) const peer = trustedPeer first := map[string]string{headerXFF: clientIPv4} - for range middleware.LoginRateLimitConst { - postWithHeaders(handler, peer, loginPath, first) + for range middleware.PasswordChangeRateLimitConst { + postWithHeaders(handler, peer, limitedPath, first) } - w := postWithHeaders(handler, peer, loginPath, first) + w := postWithHeaders(handler, peer, limitedPath, first) assert.Equal( t, http.StatusTooManyRequests, w.Code, "the forwarded client's own bucket must fill up", ) w = postWithHeaders( - handler, peer, loginPath, + handler, peer, limitedPath, map[string]string{headerXFF: clientIPv4Alt}, ) assert.Equal( @@ -662,7 +655,7 @@ func TestRateLimitKey_LongChainAllocationIsBounded(t *testing.T) { }) req := httptest.NewRequestWithContext( - context.Background(), http.MethodPost, loginPath, nil, + context.Background(), http.MethodPost, limitedPath, nil, ) req.RemoteAddr = trustedPeer req.Header.Set( @@ -869,7 +862,7 @@ func clientKeyFor( t.Helper() req := httptest.NewRequestWithContext( - context.Background(), http.MethodPost, loginPath, nil, + context.Background(), http.MethodPost, limitedPath, nil, ) req.RemoteAddr = remoteAddr @@ -1019,23 +1012,23 @@ func TestRateLimitKey_UnparseablePeerKeepsDistinctBuckets( ) } -// TestLoginRateLimit_IPv6SharesBucketWithinSlash64 is the behavioural +// TestPostRateLimit_IPv6SharesBucketWithinSlash64 is the behavioural // half, and the regression test for the bypass itself: a client that // rotates source addresses inside its own routed /64 must stay in one // bucket. Reverting the masking makes this test fail, because each // rotated address would mint a fresh bucket and nothing would be // rejected. -func TestLoginRateLimit_IPv6SharesBucketWithinSlash64(t *testing.T) { +func TestPostRateLimit_IPv6SharesBucketWithinSlash64(t *testing.T) { t.Parallel() m := rateLimitMiddleware(t, &config.Config{}) - handler := m.LoginRateLimit()(okHandler()) + handler := m.PasswordChangeRateLimit()(okHandler()) - for i := range middleware.LoginRateLimitConst { + for i := range middleware.PasswordChangeRateLimitConst { w := postWithHeaders( handler, fmt.Sprintf("[2001:db8:1:2::%d]:44444", i+1), - loginPath, nil, + limitedPath, nil, ) assert.Equal( t, http.StatusOK, w.Code, "request %d should pass", i, @@ -1043,7 +1036,7 @@ func TestLoginRateLimit_IPv6SharesBucketWithinSlash64(t *testing.T) { } w := postWithHeaders( - handler, "[2001:db8:1:2::ffff]:44444", loginPath, nil, + handler, "[2001:db8:1:2::ffff]:44444", limitedPath, nil, ) assert.Equal( t, http.StatusTooManyRequests, w.Code, @@ -1052,23 +1045,23 @@ func TestLoginRateLimit_IPv6SharesBucketWithinSlash64(t *testing.T) { ) } -// TestLoginRateLimit_IPv6IndependentAcrossSlash64 is the other side +// TestPostRateLimit_IPv6IndependentAcrossSlash64 is the other side // of the trade: bucketing by /64 must not merge separate allocations, // so a client in a different /64 keeps its own limit. -func TestLoginRateLimit_IPv6IndependentAcrossSlash64(t *testing.T) { +func TestPostRateLimit_IPv6IndependentAcrossSlash64(t *testing.T) { t.Parallel() m := rateLimitMiddleware(t, &config.Config{}) - handler := m.LoginRateLimit()(okHandler()) + handler := m.PasswordChangeRateLimit()(okHandler()) - for range middleware.LoginRateLimitConst + 1 { + for range middleware.PasswordChangeRateLimitConst + 1 { postWithHeaders( - handler, "[2001:db8:1:2::1]:44444", loginPath, nil, + handler, "[2001:db8:1:2::1]:44444", limitedPath, nil, ) } w := postWithHeaders( - handler, "[2001:db8:1:3::1]:44444", loginPath, nil, + handler, "[2001:db8:1:3::1]:44444", limitedPath, nil, ) assert.Equal( t, http.StatusOK, w.Code, @@ -1076,23 +1069,23 @@ func TestLoginRateLimit_IPv6IndependentAcrossSlash64(t *testing.T) { ) } -// TestLoginRateLimit_IPv4IndependentPerAddress guards against the +// TestPostRateLimit_IPv4IndependentPerAddress guards against the // masking leaking into IPv4: two addresses one apart must still hold // separate buckets. -func TestLoginRateLimit_IPv4IndependentPerAddress(t *testing.T) { +func TestPostRateLimit_IPv4IndependentPerAddress(t *testing.T) { t.Parallel() m := rateLimitMiddleware(t, &config.Config{}) - handler := m.LoginRateLimit()(okHandler()) + handler := m.PasswordChangeRateLimit()(okHandler()) - for range middleware.LoginRateLimitConst + 1 { + for range middleware.PasswordChangeRateLimitConst + 1 { postWithHeaders( - handler, clientIPv4+":44444", loginPath, nil, + handler, clientIPv4+":44444", limitedPath, nil, ) } w := postWithHeaders( - handler, clientIPv4Alt+":44444", loginPath, nil, + handler, clientIPv4Alt+":44444", limitedPath, nil, ) assert.Equal( t, http.StatusOK, w.Code, @@ -1112,7 +1105,7 @@ func forwardedKeyFor( t.Helper() req := httptest.NewRequestWithContext( - context.Background(), http.MethodPost, loginPath, nil, + context.Background(), http.MethodPost, limitedPath, nil, ) req.RemoteAddr = trustedPeer req.Header.Set(headerXFF, forwarded) @@ -1177,11 +1170,77 @@ func TestRateLimitKey_ForwardedIPv6BucketsByPrefix(t *testing.T) { } } -// TestLoginRateLimit_ForwardedIPv6SharesBucketWithinSlash64 is the +// TestRateLimitKey_TrustedPeerUnusableForwardedMasksPeer covers the +// third bucketKey call site: the peer IS a trusted proxy, but the +// forwarded chain cannot name a client, so the key falls back to the +// peer address — and that fallback owes the same /64 masking every +// other key gets. +// +// Every existing test of this fallback uses an IPv4 proxy, where +// bucketKey is the identity function, so replacing the call with +// peer.String() leaves the whole suite green. Only operator-listed +// addresses reach this line and the fallback is fail-closed, so this +// pins behaviour rather than fixing a defect. +func TestRateLimitKey_TrustedPeerUnusableForwardedMasksPeer( + t *testing.T, +) { + t.Parallel() + + const ( + proxyCIDR = "2001:db8:ffff::/48" + proxyPeer = "[2001:db8:ffff:1::5]:44444" + wantKey = "2001:db8:ffff:1::/64" + ) + + m := rateLimitMiddleware(t, &config.Config{ + TrustedProxies: trustedProxies(proxyCIDR), + }) + + for _, tc := range []struct { + name string + forwarded string + about string + }{{ + name: "absent", + about: "no X-Forwarded-For at all falls back to the peer", + }, { + name: "unreadable-hop", + forwarded: "unknown", + about: "a hop that is not a bare address ends the walk " + + "and falls back to the peer", + }, { + name: "all-hops-trusted", + forwarded: "2001:db8:ffff:2::9", + about: "a chain naming only trusted proxies names no " + + "client, so the peer is used", + }} { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + req := httptest.NewRequestWithContext( + context.Background(), + http.MethodPost, limitedPath, nil, + ) + req.RemoteAddr = proxyPeer + + if tc.forwarded != "" { + req.Header.Set(headerXFF, tc.forwarded) + } + + assert.Equal( + t, wantKey, + middleware.ClientKeyForTest(m, req), + "%s, masked to its /64", tc.about, + ) + }) + } +} + +// TestPostRateLimit_ForwardedIPv6SharesBucketWithinSlash64 is the // behavioural half on the production path: behind a trusted proxy, a // client rotating source addresses inside its own routed /64 must // stay in one bucket. -func TestLoginRateLimit_ForwardedIPv6SharesBucketWithinSlash64( +func TestPostRateLimit_ForwardedIPv6SharesBucketWithinSlash64( t *testing.T, ) { t.Parallel() @@ -1198,10 +1257,10 @@ func TestLoginRateLimit_ForwardedIPv6SharesBucketWithinSlash64( ) } -// TestLoginRateLimit_ForwardedIPv6IndependentAcrossSlash64 is the +// TestPostRateLimit_ForwardedIPv6IndependentAcrossSlash64 is the // other side of that trade on the same path: bucketing by /64 must // not merge two allocations reaching the proxy. -func TestLoginRateLimit_ForwardedIPv6IndependentAcrossSlash64( +func TestPostRateLimit_ForwardedIPv6IndependentAcrossSlash64( t *testing.T, ) { t.Parallel() @@ -1209,15 +1268,15 @@ func TestLoginRateLimit_ForwardedIPv6IndependentAcrossSlash64( m := rateLimitMiddleware(t, &config.Config{ TrustedProxies: trustedProxies(trustedProxyCIDR), }) - handler := m.LoginRateLimit()(okHandler()) + handler := m.PasswordChangeRateLimit()(okHandler()) spent := map[string]string{headerXFF: clientIPv6} - for range middleware.LoginRateLimitConst + 1 { - postWithHeaders(handler, trustedPeer, loginPath, spent) + for range middleware.PasswordChangeRateLimitConst + 1 { + postWithHeaders(handler, trustedPeer, limitedPath, spent) } w := postWithHeaders( - handler, trustedPeer, loginPath, + handler, trustedPeer, limitedPath, map[string]string{headerXFF: clientIPv6Other}, ) assert.Equal( diff --git a/internal/server/routes.go b/internal/server/routes.go index b282e12..b6357a0 100644 --- a/internal/server/routes.go +++ b/internal/server/routes.go @@ -96,11 +96,14 @@ func (s *Server) setupPageRoutes() { r.Use(s.mw.CSRF()) r.Use(s.mw.NoCache()) - r.Group(func(r chi.Router) { - r.Use(s.mw.LoginRateLimit()) - r.Get("/login", s.h.HandleLoginPage()) - r.Post("/login", s.h.HandleLoginSubmit()) - }) + // 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. + r.Get("/login", s.h.HandleLoginPage()) + r.Post("/login", s.h.HandleLoginSubmit()) r.Post("/logout", s.h.HandleLogout()) })