From 5a8fc853c3a5bbc31bc38768db4a4e50a4031d89 Mon Sep 17 00:00:00 2001 From: Codeman maintainer Date: Sat, 24 Oct 2026 14:54:44 +0200 Subject: [PATCH] fix(auth): share one verify between identical in-flight logins Reserving a slot per request refused a correct password with the slots it had filled itself: after a restart the browser replays its cached Basic header on many parallel requests. Identical credentials from one IP now join the verify already in flight for them (keyed on a SHA-256 digest), skipping the gates and its scrypt cost. Co-Authored-By: Claude Opus 5.5 (1M context) --- docs/security-architecture.md | 6 +- src/web/middleware/auth.ts | 160 ++++++++++++++++++++++++---------- test/multiuser-auth.test.ts | 91 ++++++++++++++++++- 3 files changed, 210 insertions(+), 47 deletions(-) diff --git a/docs/security-architecture.md b/docs/security-architecture.md index 3edf581b..21f0b9e5 100644 --- a/docs/security-architecture.md +++ b/docs/security-architecture.md @@ -169,7 +169,11 @@ 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. +slot, even one carrying the right password, but identical credentials from the +same IP that arrive during it share that verify, its slot and its outcome, and +skip both gates: they add no guess and no scrypt work. So a browser replaying its +cached login on many parallel requests after a restart (cookie sessions live in +memory) is not refused by the slots its own password filled. --- diff --git a/src/web/middleware/auth.ts b/src/web/middleware/auth.ts index ce4d0837..ea7c7767 100644 --- a/src/web/middleware/auth.ts +++ b/src/web/middleware/auth.ts @@ -9,7 +9,7 @@ */ import type { FastifyInstance, FastifyReply, FastifyRequest } from 'fastify'; -import { randomBytes, timingSafeEqual } from 'node:crypto'; +import { createHash, randomBytes, timingSafeEqual } from 'node:crypto'; import { StaleExpirationMap } from '../../utils/index.js'; import type { AuthSessionRecord } from '../ports/auth-port.js'; import { isAllowedRequestHost, isAllowedRequestOrigin, type HostPolicy } from '../network-auth-policy.js'; @@ -33,7 +33,7 @@ import { lostWebviewFramePage, LOST_FRAME_PAGE_CSP, } from '../webview-proxy.js'; -import { ApiErrorCode, createErrorResponse, type AuthUser } from '../../types.js'; +import { ApiErrorCode, createErrorResponse, type AuthUser, type UserRecord } from '../../types.js'; // Request-scoped identity (multi-user). Single-user leaves it undefined and the // ownership helpers default to a synthetic admin (see route-helpers). @@ -79,6 +79,22 @@ function parseBasicAuth(header?: string): { username: string; password: string } } } +/** + * Key of the multi-user in-flight verify map: a SHA-256 digest of (client IP, + * normalized username, password), so the plaintext password is never held as a key. + * The JSON array keeps the three parts unambiguous for any strings (a NUL-joined + * string is not, since a username may carry a NUL). Never logged or persisted, and + * gone from the map once its verify settles. + */ +function verifyFlightKey(clientIp: string, normUser: string, password: string): string { + return createHash('sha256') + .update(JSON.stringify([clientIp, normUser, password])) + .digest('hex'); +} + +/** What one Basic verify decided, shared by the request that ran it and every joiner. */ +type BasicVerifyOutcome = { user: UserRecord } | { user: null; limited: boolean }; + /** * The `/api/hook-event` + `/api/status-telemetry` localhost bypass, shared by the * single-user and multi-user auth hooks so the security-critical logic has ONE @@ -490,8 +506,8 @@ export function registerAuthMiddleware(app: FastifyInstance, https: boolean, bas * Multi-user auth hook (async, because password verification runs scrypt). Verifies * `username:password` against the user store, mints an identity-carrying cookie, * decorates `req.authUser`, enforces the per-IP, per-(username, IP) and per-username - * rate limits, and the `mustChangePassword` lockbox. The single-user hook above is - * left untouched. + * rate limits, and the `mustChangePassword` lockbox. Identical logins in flight at once + * share one verify. The single-user hook above is left untouched. */ function registerMultiUserAuthHook( app: FastifyInstance, @@ -541,6 +557,75 @@ function registerMultiUserAuthHook( return false; }; + // Basic verifies in flight, by verifyFlightKey(). A request whose identical + // credentials from the same IP are already being verified awaits that verify + // instead of starting its own. Each entry is removed when its verify settles, so the + // map never holds more than the distinct verifies in flight. + const verifiesInFlight = new Map>(); + + // The verify plus its once-only side effects, run by the first request for a key. + // A success clears the three buckets and records the login; a failure is counted + // here once (the per-IP and pair buckets already hold it: the caller reserved them). + const verifyOnce = async ( + username: string, + password: string, + normUser: string, + clientIp: string, + pairKey: string + ): Promise => { + const result = await verifyPassword(username, password); + if (result) { + const { user, needsRehash: rehash } = result; + if (rehash) void setPassword(user.username, password).catch(() => {}); + void touchLastLogin(user.username).catch(() => {}); + authFailures.delete(clientIp); + userFailures.delete(normUser); + userIpFailures.delete(pairKey); + return { user }; + } + // Failed guess: 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); + return { user: null, limited: uFail >= AUTH_FAILURE_MAX }; + }; + + // Answer one request from a shared outcome. Every successful request, joiners + // included, gets its own cookie session under the usual capacity rules. + const sendBasicOutcome = ( + req: FastifyRequest, + reply: FastifyReply, + outcome: BasicVerifyOutcome, + normUser: string + ): void => { + if (outcome.user) { + const { user } = outcome; + const token = randomBytes(32).toString('hex'); + if (authSessions.size >= MAX_AUTH_SESSIONS) evictForCapacity(user.username); + authSessions.set(token, { + ip: req.ip, + ua: req.headers['user-agent'] ?? '', + createdAt: Date.now(), + method: 'basic', + username: user.username, + role: user.role, + mustChangePassword: !!user.mustChangePassword, + }); + req.authUser = { username: user.username, role: user.role }; + setSessionCookie(reply, token); + enforcePasswordChange(req, reply, !!user.mustChangePassword); + return; + } + if (outcome.limited) { + sendAuthRateLimit(reply, userFailures, normUser); + return; + } + reply.header('WWW-Authenticate', 'Basic realm="Codeman"'); + reply.code(401).send('Unauthorized'); + }; + app.addHook('onRequest', async (req, reply) => { const bypass = checkHookSecretBypass(req, reply, hookSecretFailures); if (bypass === 'bypass' || bypass === 'rejected') return; @@ -611,14 +696,27 @@ function registerMultiUserAuthHook( // 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. + // A request whose IDENTICAL credentials from the same IP are already being verified + // joins that verify instead: it skips both gates, takes no slot, runs no scrypt and + // gets the same outcome, since it adds no guess and no CPU. Reserving for each of + // them refused a correct password with the slots that password itself had filled, + // which is every reload after a restart: cookie sessions live in memory, so the + // browser replays its cached Basic header on many parallel requests at once. A + // request with no verify in flight for its credentials is gated as normal. + const creds = parseBasicAuth(req.headers.authorization); + const normUser = creds ? creds.username.trim().toLowerCase() : ''; + const flightKey = creds ? verifyFlightKey(clientIp, normUser, creds.password) : ''; + const shared = creds ? verifiesInFlight.get(flightKey) : undefined; + if (shared) { + sendBasicOutcome(req, reply, await shared, normUser); + return; + } const ipFail = authFailures.get(clientIp) ?? 0; if (ipFail >= AUTH_IP_FAILURE_MAX) { sendAuthRateLimit(reply, authFailures, clientIp); return; } - const creds = parseBasicAuth(req.headers.authorization); if (creds) { - const normUser = creds.username.trim().toLowerCase(); // IPs never contain a newline, so this key is unambiguous for any username. const pairKey = `${clientIp}\n${normUser}`; const pairFail = userIpFailures.get(pairKey) ?? 0; @@ -626,8 +724,9 @@ 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. + // Reserve both slots and publish this verify for joiners, 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 @@ -635,44 +734,15 @@ function registerMultiUserAuthHook( // throwaway IPs lock out a known account (incl. admin) even with the correct // password. A correct password that passes the two gates above always wins and // self-heals the per-IP, per-username and this pair's buckets. - const result = await verifyPassword(creds.username, creds.password); - if (result) { - const { user, needsRehash: rehash } = result; - if (rehash) void setPassword(user.username, creds.password).catch(() => {}); - void touchLastLogin(user.username).catch(() => {}); - authFailures.delete(clientIp); - userFailures.delete(normUser); - userIpFailures.delete(pairKey); - - const token = randomBytes(32).toString('hex'); - if (authSessions.size >= MAX_AUTH_SESSIONS) evictForCapacity(user.username); - authSessions.set(token, { - ip: clientIp, - ua: req.headers['user-agent'] ?? '', - createdAt: Date.now(), - method: 'basic', - username: user.username, - role: user.role, - mustChangePassword: !!user.mustChangePassword, - }); - req.authUser = { username: user.username, role: user.role }; - setSessionCookie(reply, token); - enforcePasswordChange(req, reply, !!user.mustChangePassword); - return; + const verify = verifyOnce(creds.username, creds.password, normUser, clientIp, pairKey); + verifiesInFlight.set(flightKey, verify); + let outcome: BasicVerifyOutcome; + try { + outcome = await verify; + } finally { + verifiesInFlight.delete(flightKey); } - // 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); - if (uFail >= AUTH_FAILURE_MAX) { - sendAuthRateLimit(reply, userFailures, normUser); - return; - } - reply.header('WWW-Authenticate', 'Basic realm="Codeman"'); - reply.code(401).send('Unauthorized'); + sendBasicOutcome(req, reply, outcome, normUser); return; } diff --git a/test/multiuser-auth.test.ts b/test/multiuser-auth.test.ts index aea71a08..196e779e 100644 --- a/test/multiuser-auth.test.ts +++ b/test/multiuser-auth.test.ts @@ -16,13 +16,21 @@ import path from 'node:path'; import { WebServer } from '../src/web/server.js'; import { TmuxManager } from '../src/tmux-manager.js'; import { TunnelManager } from '../src/tunnel-manager.js'; -import { createUser, invalidateUsersCache } from '../src/user-store.js'; +import { createUser, invalidateUsersCache, verifyPassword } 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); +// Count scrypt verifies: the real verifyPassword, wrapped, so the middleware still checks +// the real store and a test can tell how many requests reached it. +vi.mock('../src/user-store.js', async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, verifyPassword: vi.fn(actual.verifyPassword) }; +}); +const verifyCalls = vi.mocked(verifyPassword); + function basic(user: string, pass: string): string { return 'Basic ' + Buffer.from(`${user}:${pass}`).toString('base64'); } @@ -318,6 +326,87 @@ describe('multi-user auth', () => { expect((await login('203.0.113.51', 'bob', 'bobpass123')).statusCode).toBe(200); }); + it('identical in-flight logins share one verify: a burst of correct ones all log in', async () => { + const { login, buckets } = await startIpServer(); + const ip = '203.0.113.60'; + verifyCalls.mockClear(); + + // A browser whose cookie died with a server restart replays its cached Basic header + // on many parallel requests. Had each reserved its own slot, every one past + // AUTH_FAILURE_MAX would be refused although all carry the right password. + const burst = []; + for (let i = 0; i < AUTH_FAILURE_MAX + 5; i++) burst.push(login(ip, 'bob', 'bobpass123')); + const responses = await Promise.all(burst); + + expect(responses.map((r) => r.statusCode)).toEqual(burst.map(() => 200)); + expect(verifyCalls).toHaveBeenCalledTimes(1); + // Each request still gets a cookie session of its own. + const tokens = responses.map((r) => /codeman_session=([^;]+)/.exec(String(r.headers['set-cookie']))?.[1]); + expect(new Set(tokens).size).toBe(burst.length); + expect(tokens).not.toContain(undefined); + // The one success cleared the buckets, the reservation included. + expect(buckets.userIpFailures.get(`${ip}\nbob`)).toBeUndefined(); + expect(buckets.authFailures.get(ip)).toBeUndefined(); + + // The entry left the map when its verify settled: the next login verifies afresh. + expect((await login(ip, 'bob', 'bobpass123')).statusCode).toBe(200); + expect(verifyCalls).toHaveBeenCalledTimes(2); + }); + + it('identical in-flight logins share one verify: a burst of one wrong password counts once', async () => { + const { login, buckets } = await startIpServer(); + const ip = '203.0.113.70'; + verifyCalls.mockClear(); + + const burst = []; + for (let i = 0; i < AUTH_FAILURE_MAX + 5; i++) burst.push(login(ip, 'bob', 'same-wrong')); + const codes = (await Promise.all(burst)).map((r) => r.statusCode); + + // One guess, verified once, counted once in each bucket, and the same 401 for all. + expect(codes).toEqual(burst.map(() => 401)); + expect(verifyCalls).toHaveBeenCalledTimes(1); + expect(buckets.userFailures.get('bob')).toBe(1); + expect(buckets.userIpFailures.get(`${ip}\nbob`)).toBe(1); + expect(buckets.authFailures.get(ip)).toBe(1); + }); + + it('identical in-flight logins skip the gates: the slot their own verify took does not refuse them', async () => { + const { login } = await startIpServer(); + const ip = '203.0.113.80'; + for (let i = 0; i < AUTH_FAILURE_MAX - 1; i++) { + expect((await login(ip, 'bob', `bad-${i}`)).statusCode, `attempt ${i + 1}`).toBe(401); + } + verifyCalls.mockClear(); + + // The first of these takes the pair's last slot; the rest join its verify, so a gate + // check of their own would refuse them with a full bucket. + const burst = []; + for (let i = 0; i < 5; i++) burst.push(login(ip, 'bob', 'bobpass123')); + const codes = (await Promise.all(burst)).map((r) => r.statusCode); + + expect(codes).toEqual(burst.map(() => 200)); + expect(verifyCalls).toHaveBeenCalledTimes(1); + }); + + it('a verify that throws leaves the in-flight map, and its joiners share the failure', async () => { + const { login } = await startIpServer(); + const ip = '203.0.113.90'; + verifyCalls.mockClear(); + verifyCalls.mockRejectedValueOnce(new Error('user store unreadable')); + + const burst = []; + for (let i = 0; i < 3; i++) burst.push(login(ip, 'bob', 'bobpass123')); + const codes = (await Promise.all(burst)).map((r) => r.statusCode); + + expect(verifyCalls).toHaveBeenCalledTimes(1); + expect(new Set(codes).size).toBe(1); + expect(codes[0]).toBeGreaterThanOrEqual(500); + + // Removed in a finally, so the next identical login runs a verify of its own. + expect((await login(ip, 'bob', 'bobpass123')).statusCode).toBe(200); + expect(verifyCalls).toHaveBeenCalledTimes(2); + }); + it('per-(username, IP) gate: a correct password clears the pair bucket', async () => { const { login } = await startIpServer(); const ip = '203.0.113.30';