Verify login credentials before spending rate-limit budget (closes #150)

In the shipped default, any stranger denied the operator the only
administrative path at 5 requests per minute: TRUSTED_PROXIES is empty,
the README requires a reverse proxy, so every login POST shared one
bucket keyed on the proxy.

Credentials are now verified first and only a FAILED attempt spends
budget, so a correct password is never throttled. Failures are counted
per (client bucket, submitted username), bounded. Concurrent Argon2id
verifications are capped at two, and the queue for them at 16 — because
verifying first lets an attacker force a 64 MB hash per request, and
bounding the wait alone bounds nothing.

The issue's own recommendation was insufficient and is rejected here:
keying by username stops an attacker locking out a DIFFERENT account,
but this is a single-admin product with a predictable bootstrap
username, so flooding the operator's own name still locks them out.

This is speculative — it implements a corrected recommendation ahead of
the owner's ruling so the decision can be made by merging or reverting.
Three things are disclosed rather than glossed: online guessing rises
from 5/min to roughly 27/s, because the 429 is a label on the response
and not a gate in front of the hash; the residual exposure is a loss of
login AVAILABILITY, not latency, and a determined flood still denies
login while it runs, at ~400x the cost and clearing the moment it
stops; and the endpoint should be provisioned for ~400 MB resident, not
the 203 MB of live commitment it itemises.

Independently reviewed four times. Reviewers disproved the suspected
FIFO starvation by measurement, then caught two successive memory
bounds the code did not have — the second by parking waiters and
reading the heap rather than checking the arithmetic.

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

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",
+	)
+}

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 {

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
 

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...),

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