From af661a06c82d0bf68d981f8aeb917c98cb6d9cde Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Sat, 31 Oct 2026 23:53:11 +0100 Subject: [PATCH] fix: key refused stored passwords by a secret-keyed hash and never hold a JMAP call for the shared store #1066 --- .../shared-stored-credential-budget.test.ts | 84 ++++++++++ lib/stalwart/server-fetch.ts | 8 +- lib/stalwart/stored-credential-budget.ts | 152 +++++++++++++----- 3 files changed, 205 insertions(+), 39 deletions(-) diff --git a/lib/__tests__/shared-stored-credential-budget.test.ts b/lib/__tests__/shared-stored-credential-budget.test.ts index 8d0270f0c..e6ed0cbb0 100644 --- a/lib/__tests__/shared-stored-credential-budget.test.ts +++ b/lib/__tests__/shared-stored-credential-budget.test.ts @@ -1,4 +1,7 @@ // @vitest-environment node +import { createHash } from 'node:crypto'; +import { tmpdir } from 'node:os'; +import path from 'node:path'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { isStoredCredentialRefused, @@ -22,6 +25,7 @@ let store: ReturnType; const otherReplica = () => resetStoredCredentialBudgetForTests(); beforeEach(() => { + vi.stubEnv('SESSION_SECRET', 'a-test-session-secret-of-at-least-32-chars'); resetLocalStoreForTests(); resetStoredCredentialBudgetForTests(); store = switchableSharedStore(); @@ -29,6 +33,8 @@ beforeEach(() => { }); afterEach(() => { + vi.unstubAllEnvs(); + vi.restoreAllMocks(); setStoreForTests(null); resetLocalStoreForTests(); resetStoredCredentialBudgetForTests(); @@ -81,4 +87,82 @@ describe('stored credential budget with a shared store', () => { await recordStoredCredentialStatus(basic('later'), 401, T0 + MIN); expect(await isStoredCredentialRefused(basic('later'), T0 + 2 * MIN)).toBe(true); }); + + it('lifts a refusal recorded on another replica even right after its own success was written', async () => { + // Each replica keeps its own process state; swap it to act as B, A and C. + const KEY = Symbol.for('bulwark.storedCredentialBudget'); + const g = globalThis as Record; + const replicas = new Map(); + const as = async (name: string, fn: () => Promise) => { + if (replicas.has(name)) g[KEY] = replicas.get(name); + else delete g[KEY]; + await fn(); + replicas.set(name, g[KEY]); + }; + await as('B', () => recordStoredCredentialStatus(basic('a'), 200, T0)); + await as('A', () => recordStoredCredentialStatus(basic('a'), 401, T0 + 10_000)); + // The user signs in again through B 20 s after B's last write. + await as('B', () => recordStoredCredentialStatus(basic('a'), 200, T0 + 20_000)); + await as('C', async () => expect(await isStoredCredentialRefused(basic('a'), T0 + 30_000)).toBe(false)); + }); + + it('keys the store by an HMAC of the session secret, never a plain hash of the credential', async () => { + const set = vi.spyOn(store.kv, 'set'); + await recordStoredCredentialStatus(basic('stale'), 401, T0); + const keys = set.mock.calls.map(([key]) => key); + const plain = createHash('sha256').update(basic('stale')).digest('base64url'); + expect(keys.length).toBeGreaterThan(0); + for (const key of keys) expect(key).not.toContain(plain); + + // Another secret, another key: a store backup alone can't be tried against passwords. + otherReplica(); + vi.stubEnv('SESSION_SECRET', 'another-test-session-secret-of-32-chars'); + expect(await isStoredCredentialRefused(basic('stale'), T0 + MIN)).toBe(false); + }); + + it('writes nothing to the store without a session secret', async () => { + vi.stubEnv('SESSION_SECRET', ''); + vi.stubEnv('ADMIN_STATE_DIR', path.join(tmpdir(), 'bw-no-secret-does-not-exist')); + const set = vi.spyOn(store.kv, 'set'); + const get = vi.spyOn(store.kv, 'get'); + await recordStoredCredentialStatus(basic('stale'), 401, T0); + expect(await isStoredCredentialRefused(basic('stale'), T0 + MIN)).toBe(true); + expect(set).not.toHaveBeenCalled(); + expect(get).not.toHaveBeenCalled(); + }); + + it('decides from this replica within the deadline when the store is slow', async () => { + await recordStoredCredentialStatus(basic('stale'), 401, T0); + vi.spyOn(store.kv, 'get').mockImplementation(() => new Promise((resolve) => setTimeout(() => resolve(null), 2000))); + const started = Date.now(); + expect(await isStoredCredentialRefused(basic('stale'), T0 + MIN)).toBe(true); + expect(await isStoredCredentialRefused(basic('fresh'), T0 + MIN)).toBe(false); + expect(Date.now() - started).toBeLessThan(1500); + }); + + it('honours and later writes a refusal seen while the store was down', async () => { + store.down = true; + await recordStoredCredentialStatus(basic('stale'), 401, T0); + store.down = false; + // The store doesn't know it yet; this replica still refuses it. + expect(await isStoredCredentialRefused(basic('stale'), T0 + MIN)).toBe(true); + // The next write carries it to the store, and other replicas refuse it too. + await recordStoredCredentialStatus(basic('other'), 200, T0 + MIN); + otherReplica(); + expect(await isStoredCredentialRefused(basic('stale'), T0 + 2 * MIN)).toBe(true); + }); + + it('answers a JMAP call without waiting for the shared writes', async () => { + vi.stubGlobal('fetch', vi.fn(async () => new Response('', { status: 401 }))); + vi.spyOn(store.kv, 'set').mockImplementation(() => new Promise(() => {})); + vi.spyOn(store.counters, 'hit').mockImplementation(() => new Promise(() => {})); + const { fetchJmapServer } = await import('@/lib/stalwart/server-fetch'); + const res = await Promise.race([ + fetchJmapServer('https://mail.example/jmap/session', { headers: { Authorization: basic('stale') } }, true), + new Promise<'hung'>((resolve) => setTimeout(() => resolve('hung'), 1000)), + ]); + expect(res).not.toBe('hung'); + expect((res as Response).status).toBe(401); + vi.unstubAllGlobals(); + }); }); diff --git a/lib/stalwart/server-fetch.ts b/lib/stalwart/server-fetch.ts index ebc402c86..d1adc2fe3 100644 --- a/lib/stalwart/server-fetch.ts +++ b/lib/stalwart/server-fetch.ts @@ -16,6 +16,7 @@ */ import { configManager } from '@/lib/admin/config-manager'; +import { logger } from '@/lib/logger'; import { parseJmapServers, resolveTrustedJmapUrl } from '@/lib/admin/jmap-servers'; import { fetchPublicUrl, type PublicFetchResponse } from '@/lib/security/url-guard'; import { @@ -72,7 +73,12 @@ export async function fetchJmapServer( const authHeader = new Headers(givenInit.headers).get('authorization'); if (authHeader && (await isStoredCredentialRefused(authHeader))) return refusedCredentialResponse(); const response = await fetchUnbudgeted(url, givenInit, trusted); - if (authHeader) await recordStoredCredentialStatus(authHeader, response.status); + // Not awaited: this process's copy is updated at once, the shared write follows. + if (authHeader) { + void recordStoredCredentialStatus(authHeader, response.status).catch((error) => { + logger.warn('Stored credential status not recorded', { error: error instanceof Error ? error.message : 'Unknown error' }); + }); + } return response; } diff --git a/lib/stalwart/stored-credential-budget.ts b/lib/stalwart/stored-credential-budget.ts index d84283f1e..49f5c2eee 100644 --- a/lib/stalwart/stored-credential-budget.ts +++ b/lib/stalwart/stored-credential-budget.ts @@ -1,5 +1,6 @@ -import { createHash } from 'node:crypto'; -import { getStore, isSharedMode } from '@/lib/shared-store'; +import { createHash, createHmac, hkdfSync } from 'node:crypto'; +import { getStore, isSharedMode, isStoreUnavailableError } from '@/lib/shared-store'; +import { getSessionSecret } from '@/lib/auth/session-secret'; import { logger } from '@/lib/logger'; /** @@ -21,13 +22,18 @@ import { logger } from '@/lib/logger'; * Only Basic credentials count: Stalwart does not count an expired or unknown * access token as a failed login, and access tokens expire all the time. * + * Credentials are known by an HMAC keyed from the session secret, never by a + * plain hash: a plain hash of a Basic header in the store (or a backup of it) + * could be tried against a password list offline. + * * With a shared store (#1066) refusals and the failure window are kept there, * so a credential one replica saw refused isn't tried again by the others. * Successes are remembered in this process and written to the store at most * once a minute per credential: this check runs on every server-side JMAP - * call. If the store can't be reached, this process's own copy decides, as a - * single instance does; refusing every call would take files, push previews - * and calendar sync down with the store. + * call. The check waits at most CHECK_DEADLINE_MS for the store; if the store + * is slow or down, this process's own copy decides, as a single instance + * does: refusing every call would take files, push previews and calendar sync + * down with the store. Without a session secret nothing goes to the store. */ const REFUSE_MS = 60 * 60 * 1000; @@ -38,6 +44,8 @@ const MAX_TRACKED = 10_000; const SHARED_SUCCESS_EVERY_MS = 60 * 1000; /** Shared keys outlive their window a little, so clock skew can't drop them early. */ const SHARED_GRACE_MS = 60 * 1000; +/** How long a request waits for the shared refusal check before this process decides. */ +export const CHECK_DEADLINE_MS = 200; const FAILURES_KEY = 'stored-credential:failures'; interface BudgetState { @@ -48,6 +56,10 @@ interface BudgetState { failureWindow: { failures: number; resetAt: number }; /** Credential key -> when this process last wrote its success to the shared store. */ sharedSuccess: Map; + /** Refusals seen here that the shared store didn't take (it was down): honoured and written later. */ + unsynced: Map; + /** The HMAC key and the secret it was derived from. */ + hmac?: { secret: string; key: Buffer }; } // On globalThis: one copy per process, not one per bundle. @@ -55,13 +67,43 @@ const STATE_KEY = Symbol.for('bulwark.storedCredentialBudget'); function state(): BudgetState { const g = globalThis as typeof globalThis & { [STATE_KEY]?: BudgetState }; - g[STATE_KEY] ??= { refused: new Map(), succeeded: new Map(), failureWindow: { failures: 0, resetAt: 0 }, sharedSuccess: new Map() }; + g[STATE_KEY] ??= { + refused: new Map(), + succeeded: new Map(), + failureWindow: { failures: 0, resetAt: 0 }, + sharedSuccess: new Map(), + unsynced: new Map(), + }; return g[STATE_KEY]; } -function keyOf(authHeader: string): string | null { +function sessionSecret(): string { + try { + return getSessionSecret(); + } catch { + return ''; + } +} + +interface CredentialKey { + key: string; + /** Keyed from the session secret, so it may go to the shared store. */ + shareable: boolean; +} + +function keyOf(authHeader: string): CredentialKey | null { if (!/^basic /i.test(authHeader)) return null; - return createHash('sha256').update(authHeader).digest('base64url'); + const secret = sessionSecret(); + if (!secret) { + // Kept in this process's memory only. + return { key: createHash('sha256').update(authHeader).digest('base64url'), shareable: false }; + } + const s = state(); + if (s.hmac?.secret !== secret) { + const root = createHash('sha256').update(secret).digest(); + s.hmac = { secret, key: Buffer.from(hkdfSync('sha256', root, Buffer.alloc(0), 'stored-credential', 32)) }; + } + return { key: createHmac('sha256', s.hmac.key).update(authHeader).digest('base64url'), shareable: true }; } const refusedKey = (key: string) => `stored-credential:refused:${key}`; @@ -98,88 +140,122 @@ function isRefusedLocally(key: string, now: number): boolean { } function recordLocally(key: string, status: number, now: number): void { - const { refused, succeeded } = state(); + const { refused, succeeded, unsynced } = state(); if (status === 401) { succeeded.delete(key); remember(refused, key, now + REFUSE_MS); currentWindow(now).failures++; } else if (status >= 200 && status < 400) { refused.delete(key); + unsynced.delete(key); remember(succeeded, key, now); } } // ─── shared store ────────────────────────────────────────────────────────── -function isUnavailable(error: unknown): boolean { - return error instanceof Error && error.name === 'StoreUnavailableError'; +/** Refusals this process couldn't write while the store was down, written now. */ +async function flushUnsynced(now: number): Promise { + const { unsynced } = state(); + for (const [key, until] of [...unsynced]) { + if (until <= now) { + unsynced.delete(key); + continue; + } + await getStore().kv.set(refusedKey(key), String(until), { ttlMs: until - now + SHARED_GRACE_MS }); + unsynced.delete(key); + } } async function isRefusedShared(key: string, now: number): Promise { const store = getStore(); + const s = state(); const [until, window] = await Promise.all([store.kv.get(refusedKey(key)), store.counters.get(FAILURES_KEY, now)]); const refusedUntil = Number(until); if (until !== null && refusedUntil > now) { // Kept here too, for when the store is down. - remember(state().refused, key, refusedUntil); + remember(s.refused, key, refusedUntil); return true; } + // Refused here while the store was down, and not written there yet. + const pending = s.unsynced.get(key); + if (pending !== undefined && pending > now) return true; if ((window?.count ?? 0) < GLOBAL_MAX_FAILURES) return false; - const local = state().succeeded.get(key); + const local = s.succeeded.get(key); if (local !== undefined && now - local <= WINDOW_MS) return false; const shared = Number(await store.kv.get(succeededKey(key))); return !(shared > 0 && now - shared <= WINDOW_MS); } -async function recordShared(key: string, status: number, now: number, wasRefusedHere: boolean): Promise { +async function recordShared(key: string, status: number, now: number): Promise { const store = getStore(); - const { sharedSuccess } = state(); + const s = state(); if (status === 401) { - sharedSuccess.delete(key); + s.sharedSuccess.delete(key); + remember(s.unsynced, key, now + REFUSE_MS); await Promise.all([ store.kv.set(refusedKey(key), String(now + REFUSE_MS), { ttlMs: REFUSE_MS + SHARED_GRACE_MS }), store.kv.del(succeededKey(key)), store.counters.hit(FAILURES_KEY, WINDOW_MS, now), ]); + s.unsynced.delete(key); } else if (status >= 200 && status < 400) { - const last = sharedSuccess.get(key); - // A success that lifts a refusal is written at once; otherwise once a minute. - if (!wasRefusedHere && last !== undefined && now - last >= 0 && now - last < SHARED_SUCCESS_EVERY_MS) return; - remember(sharedSuccess, key, now); - await Promise.all([ - store.kv.del(refusedKey(key)), - store.kv.set(succeededKey(key), String(now), { ttlMs: WINDOW_MS + SHARED_GRACE_MS }), - ]); + // A success always lifts a refusal, wherever it was recorded. + const writes: Promise[] = [store.kv.del(refusedKey(key))]; + const last = s.sharedSuccess.get(key); + // Only the "worked recently" mark is written at most once a minute. + if (last === undefined || now - last < 0 || now - last >= SHARED_SUCCESS_EVERY_MS) { + remember(s.sharedSuccess, key, now); + writes.push(store.kv.set(succeededKey(key), String(now), { ttlMs: WINDOW_MS + SHARED_GRACE_MS })); + } + await Promise.all(writes); } + if (s.unsynced.size > 0) await flushUnsynced(now); +} + +function withDeadline(promise: Promise, ms: number): Promise { + let timer: ReturnType | undefined; + const deadline = new Promise((_, reject) => { + timer = setTimeout(() => { + const error = new Error('The shared store did not answer in time'); + error.name = 'StoreUnavailableError'; + reject(error); + }, ms); + timer.unref?.(); + }); + return Promise.race([promise, deadline]).finally(() => clearTimeout(timer)); } // ─── API ─────────────────────────────────────────────────────────────────── /** True when `authHeader` must not be sent to the mail server right now. */ export async function isStoredCredentialRefused(authHeader: string, now: number = Date.now()): Promise { - const key = keyOf(authHeader); - if (!key) return false; - if (!isSharedMode()) return isRefusedLocally(key, now); + const credential = keyOf(authHeader); + if (!credential) return false; + if (!isSharedMode() || !credential.shareable) return isRefusedLocally(credential.key, now); try { - return await isRefusedShared(key, now); + return await withDeadline(isRefusedShared(credential.key, now), CHECK_DEADLINE_MS); } catch (error) { - if (!isUnavailable(error)) throw error; - return isRefusedLocally(key, now); + if (!isStoreUnavailableError(error)) throw error; + return isRefusedLocally(credential.key, now); } } -/** Record the mail server's answer to a request that carried `authHeader`. */ +/** + * Record the mail server's answer to a request that carried `authHeader`. + * This process's copy is updated before the first await, so a caller that + * doesn't wait for the shared write still sees it on its next check. + */ export async function recordStoredCredentialStatus(authHeader: string, status: number, now: number = Date.now()): Promise { - const key = keyOf(authHeader); - if (!key) return; - const wasRefusedHere = state().refused.has(key); + const credential = keyOf(authHeader); + if (!credential) return; // Always kept here: it is what decides while the shared store is down. - recordLocally(key, status, now); - if (!isSharedMode()) return; + recordLocally(credential.key, status, now); + if (!isSharedMode() || !credential.shareable) return; try { - await recordShared(key, status, now, wasRefusedHere); + await recordShared(credential.key, status, now); } catch (error) { - if (!isUnavailable(error)) throw error; + if (!isStoreUnavailableError(error)) throw error; logger.debug('Stored credential status kept in this process only: the shared store is unavailable'); } }