diff --git a/docs/security-architecture.md b/docs/security-architecture.md index cc9bb362..3edf581b 100644 --- a/docs/security-architecture.md +++ b/docs/security-architecture.md @@ -165,6 +165,11 @@ account from another IP still logs in), which is why the per-IP cap is the widest. The per-username bucket stays after the verify on purpose: checked first, a username-wide lockout let throwaway IPs keep a known account (admin included) locked out even with the right password. +Both pre-verify gates count a request the moment it passes them, before its +verify starts, so a burst of concurrent attempts cannot all slip through on one +stale count; a correct password then clears its slot with the rest, and a +failure is not counted a second time. A verify still in flight therefore holds a +slot, even one carrying the right password. --- diff --git a/src/web/middleware/auth.ts b/src/web/middleware/auth.ts index 08d8e573..ce4d0837 100644 --- a/src/web/middleware/auth.ts +++ b/src/web/middleware/auth.ts @@ -365,8 +365,8 @@ export function registerAuthMiddleware(app: FastifyInstance, https: boolean, bas // caps verified guesses against one account from one source. Keyed on the pair, // so other IPs can't lock the account out and two users behind one shared IP // don't lock each other out. Same decay as the other buckets: it holds one key - // per (IP, username) pair that failed a verify, swept by the same TTL, so like - // them it grows only with failed scrypt verifies. + // per (IP, username) pair whose verify failed or is still in flight, swept by + // the same TTL, so like them it grows only with scrypt verifies. state.userIpFailures = new StaleExpirationMap({ ttlMs: AUTH_FAILURE_WINDOW_MS, refreshOnGet: false, @@ -606,6 +606,11 @@ function registerMultiUserAuthHook( // - per username (AUTH_FAILURE_MAX), checked AFTER the verify; see below. // Neither pre-verify refusal is counted, since nothing was verified (and counting // a locked pair's retries would fill the shared per-IP bucket for everyone). + // A request that passes both gates RESERVES its slot in both buckets before the + // scrypt await, as if it had already failed. Counting only after the verify let a + // concurrent burst pass the gates on the same stale read: 29 wrong guesses plus the + // right password, all in flight at once from one IP, logged in. A success deletes + // the reservation with the rest of the buckets; a failure is not counted again. const ipFail = authFailures.get(clientIp) ?? 0; if (ipFail >= AUTH_IP_FAILURE_MAX) { sendAuthRateLimit(reply, authFailures, clientIp); @@ -621,6 +626,10 @@ function registerMultiUserAuthHook( sendAuthRateLimit(reply, userIpFailures, pairKey); return; } + // Reserve both slots now, from the values the gates just read. Correct only while + // nothing between those reads and these writes awaits: keep it that way. + authFailures.set(clientIp, ipFail + 1); + userIpFailures.set(pairKey, pairFail + 1); // Security: the per-username bucket is consulted only AFTER the verify, and only // FAILED attempts are throttled by it. Consulting it before verifying let // throwaway IPs lock out a known account (incl. admin) even with the correct @@ -651,15 +660,13 @@ function registerMultiUserAuthHook( enforcePasswordChange(req, reply, !!user.mustChangePassword); return; } - // Failed guess: count it against all three buckets. Once the per-username bucket - // reaches the cap, further FAILED attempts get 429 from any IP, but this path is - // only reached on a wrong password, so it can never deny a correct one. - // Every count is re-read after the await: `ipFail` was read before it, and - // concurrent failures from one IP would otherwise all write the same value. + // Failed guess: the per-IP and pair buckets already hold it (reserved above), so + // only the per-username bucket counts it here. Once that bucket reaches the cap, + // further FAILED attempts get 429 from any IP, but this path is only reached on a + // wrong password, so it can never deny a correct one. Its count is re-read after + // the await, so concurrent failures against one account each add one. const uFail = (userFailures.get(normUser) ?? 0) + 1; userFailures.set(normUser, uFail); - userIpFailures.set(pairKey, (userIpFailures.get(pairKey) ?? 0) + 1); - authFailures.set(clientIp, (authFailures.get(clientIp) ?? 0) + 1); if (uFail >= AUTH_FAILURE_MAX) { sendAuthRateLimit(reply, userFailures, normUser); return; diff --git a/test/multiuser-auth.test.ts b/test/multiuser-auth.test.ts index 54bbf11c..aea71a08 100644 --- a/test/multiuser-auth.test.ts +++ b/test/multiuser-auth.test.ts @@ -19,6 +19,7 @@ import { TunnelManager } from '../src/tunnel-manager.js'; import { createUser, invalidateUsersCache } from '../src/user-store.js'; import type { FastifyInstance } from 'fastify'; import { AUTH_FAILURE_MAX, AUTH_IP_FAILURE_MAX } from '../src/config/auth-config.js'; +import type { StaleExpirationMap } from '../src/utils/stale-expiration-map.js'; vi.spyOn(TmuxManager, 'isTmuxAvailable').mockReturnValue(true); @@ -88,6 +89,9 @@ afterAll(async () => { const url = (p: string) => `http://localhost:${server.boundPort}${p}`; +/** The server's private failure buckets, read to count how many guesses reached the verify. */ +type FailureBuckets = Record<'authFailures' | 'userFailures' | 'userIpFailures', StaleExpirationMap>; + /** * A fresh multi-user server (empty failure buckets) and a login helper that sets the * client IP through inject's `remoteAddress`: a real fetch always arrives from @@ -98,8 +102,9 @@ async function startIpServer() { await s.start(); ipServers.push(s); const app = (s as unknown as { app: FastifyInstance }).app; - return (ip: string, user: string, pass: string) => + const login = (ip: string, user: string, pass: string) => app.inject({ method: 'GET', url: '/api/status', headers: { authorization: basic(user, pass) }, remoteAddress: ip }); + return { login, buckets: s as unknown as FailureBuckets }; } describe('multi-user auth', () => { @@ -231,7 +236,7 @@ describe('multi-user auth', () => { }); it('per-(username, IP) gate: failures lock that account on that IP only, before scrypt', async () => { - const login = await startIpServer(); + const { login } = await startIpServer(); const ipA = '203.0.113.10'; const ipB = '203.0.113.20'; @@ -257,26 +262,64 @@ describe('multi-user auth', () => { expect((await login(ipA, 'bob', 'bobpass123')).statusCode).toBe(429); }); - it('per-IP pre-gate: concurrent failures from one IP are all counted', async () => { - const login = await startIpServer(); + it('per-IP pre-gate: a concurrent burst from one IP reaches the verify at most AUTH_IP_FAILURE_MAX times', async () => { + const { login, buckets } = await startIpServer(); const ip = '203.0.113.40'; - // AUTH_IP_FAILURE_MAX wrong passwords in flight at once, spread so neither the pair - // nor the per-username bucket can refuse any of them. The per-IP count is written - // after the scrypt await, so each write must re-read it or the burst counts as one. + // More than AUTH_IP_FAILURE_MAX wrong passwords in flight at once, spread so neither + // the pair nor the per-username bucket can refuse any of them. All the gate reads + // happen before the first verify ends, so only a slot reserved before the scrypt + // await stops the burst passing the pre-gate on one stale count. + const extra = 10; const perUser = AUTH_FAILURE_MAX - 1; const burst = []; - for (let i = 0; i < AUTH_IP_FAILURE_MAX; i++) { + for (let i = 0; i < AUTH_IP_FAILURE_MAX + extra; i++) { burst.push(login(ip, `burst${Math.floor(i / perUser)}`, `bad-${i}`)); } - await Promise.all(burst); + const codes = (await Promise.all(burst)).map((r) => r.statusCode); - // The bucket is full, so the next request from that IP is refused before scrypt. + // No account fills a bucket of its own here, so every verified guess is a 401 and + // every 429 is the pre-gate. + expect(codes.filter((c) => c === 401).length).toBe(AUTH_IP_FAILURE_MAX); + expect(codes.filter((c) => c === 429).length).toBe(extra); + // Each admitted guess counted once: reserved before the verify, not again after it. + expect(buckets.authFailures.get(ip)).toBe(AUTH_IP_FAILURE_MAX); + + // The bucket is full, so the next request from that IP is refused before scrypt, + // a correct password included. expect((await login(ip, 'burst-next', 'bad')).statusCode).toBe(429); + expect((await login(ip, 'alice', 'alicepass1')).statusCode).toBe(429); + }); + + it('per-(username, IP) gate: a concurrent burst cannot carry the right password past the cap', async () => { + const { login, buckets } = await startIpServer(); + const ip = '203.0.113.50'; + + // AUTH_FAILURE_MAX + 5 logins for bob from one IP, all in flight at once, the LAST + // carrying the correct password. Counting only after the verify let every one of + // them past the gate on the same stale read, and the correct one logged in. + const burst = []; + for (let i = 0; i < AUTH_FAILURE_MAX + 4; i++) burst.push(login(ip, 'bob', `bad-${i}`)); + burst.push(login(ip, 'bob', 'bobpass123')); + const codes = (await Promise.all(burst)).map((r) => r.statusCode); + + expect(codes.at(-1)).toBe(429); + expect(codes.filter((c) => c !== 401 && c !== 429)).toEqual([]); + // Every failed verify adds one to bob's per-username bucket, so it counts the guesses + // that reached scrypt: exactly the AUTH_FAILURE_MAX the pair gate admits. (The last + // of those is a 429 from that bucket, which is why the 401s are fewer.) + expect(buckets.userFailures.get('bob')).toBe(AUTH_FAILURE_MAX); + expect(codes.filter((c) => c === 401).length).toBeLessThanOrEqual(AUTH_FAILURE_MAX); + // Reserved once each, not counted again on failure. + expect(buckets.userIpFailures.get(`${ip}\nbob`)).toBe(AUTH_FAILURE_MAX); + expect(buckets.authFailures.get(ip)).toBe(AUTH_FAILURE_MAX); + + // The lockout is that pair's only: bob still logs in from another IP. + expect((await login('203.0.113.51', 'bob', 'bobpass123')).statusCode).toBe(200); }); it('per-(username, IP) gate: a correct password clears the pair bucket', async () => { - const login = await startIpServer(); + const { login } = await startIpServer(); const ip = '203.0.113.30'; for (let i = 0; i < AUTH_FAILURE_MAX - 1; i++) {