Verify login credentials before spending rate-limit budget (closes #150)
All checks were successful
check / check (push) Successful in 2m47s
All checks were successful
check / check (push) Successful in 2m47s
With TRUSTED_PROXIES empty behind the reverse proxy production is required to run behind, every login POST keyed on the proxy's address and shared one 5/minute bucket. A stranger sending five POSTs a minute -- 0.08 requests per second, from anywhere -- kept that bucket permanently full, and the operator's own correct password was answered 429 indefinitely with no second administrative path. The login POST no longer has a pre-emptive limiter. The handler verifies credentials first and spends budget only on a FAILED attempt, so a correct password is never throttled whatever the counters hold. Three things follow, and are implemented together because the first is unsafe without the other two: - Failures are counted per (client bucket, submitted username), five per minute, after which further failures get 429 with a Retry-After. A successful login clears the counter, so mistyping and then succeeding does not leave the operator throttled. - Both key sets are capped at 1024 entries. The submitted username is attacker-controlled, so past the first cap failures fall back to a counter keyed on the client alone, and past both caps a failure is answered as throttled without being recorded. Tracked state stays under half a megabyte and does not grow with invented usernames. - Concurrent Argon2id verifications are capped at two, a 128 MB ceiling at 64 MB per hash, and the queue for those slots is capped at 16 waiters. Every password-hashing endpoint takes a slot, including the password-change endpoint, which holds one across both its hashes. A request that waits five seconds without a slot is answered 503, and one that arrives with the queue already full is shed with 503 immediately rather than joining it. Bounding the wait alone would not bound memory, and the queue depth is sized from what a parked waiter measurably retains rather than from the 1 MB body cap, which bounds only the raw body read. The body-cap, CSRF and form-parsing middleware all run before the guard, so a waiter holds its parsed form plus its request header block for the whole wait. Measured on the pinned go1.26.1 toolchain as the HeapAlloc delta across two GCs with 64 waiters parked in the handler: an ordinary two-field login form retains ~0 MB, a 1 MB urlencoded body at Go's 10,000-parameter parse cap retains 2.82 MB (3.09 MB with %41 escapes), and the ~0.9 MB of headers the 1 MB header cap allows takes it to 4.18 MB. The retained parse and the header block dominate, not the raw body. So 16 waiters: 16 x 4.18 MB is about 67 MB of committed queue memory, and two slots drain a full 16-deep queue in about 0.6 s, far inside the deadline. Peak commitment for the endpoint is about 203 MB — 128 MB of Argon2id plus the 18 requests holding a parsed form, 16 queued and the 2 being hashed, at about 75 MB. An unknown username is verified against a dummy hash instead of returning early, so a nonexistent account costs the same time as a real one and the response cannot be used to enumerate usernames. The password-change limiter is unchanged: RequireAuth runs ahead of it, so only a request already carrying a valid session reaches its bucket. Two consequences are documented rather than fixed, because they follow from the shape the issue asks for. Online guessing throughput rises from 5 a minute to roughly 27 a second, about 2.3 million a day: the credential check always precedes the counter, so the 429 is a label on the response rather than a gate in front of the hash, and what bounds brute force is the semaphore. And under a sustained flood the residual exposure is a loss of login availability, not merely of latency -- above about 27 requests a second most attempts are shed with 503, so a determined flood still denies login for as long as it runs. It costs roughly 400x more to run, nothing accumulates, and the first attempt after it stops succeeds. Restarting the service does not help: the counters a restart clears are not what is saturated. Also adds the missing test for the third bucketKey call site, where the peer is a trusted proxy but the forwarded chain names no client. Every existing test of that fallback uses an IPv4 proxy, where bucketKey is the identity function, so dropping the /64 masking there left the suite green. README and the TRUSTED_PROXIES startup warning updated: a shared bucket now costs precision, not the availability of the admin path.
This commit is contained in:
455
internal/handlers/auth_test.go
Normal file
455
internal/handlers/auth_test.go
Normal file
@@ -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",
|
||||
)
|
||||
}
|
||||
Reference in New Issue
Block a user