fix: key refused stored passwords by a secret-keyed hash and never hold a JMAP call for the shared store #1066

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<typeof switchableSharedStore>;
 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<symbol, unknown>;
+    const replicas = new Map<string, unknown>();
+    const as = async (name: string, fn: () => Promise<unknown>) => {
+      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();
+  });
 });

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;
 }
 

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<string, number>;
+  /** Refusals seen here that the shared store didn't take (it was down): honoured and written later. */
+  unsynced: Map<string, number>;
+  /** 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<void> {
+  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<boolean> {
   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<void> {
+async function recordShared(key: string, status: number, now: number): Promise<void> {
   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<unknown>[] = [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<T>(promise: Promise<T>, ms: number): Promise<T> {
+  let timer: ReturnType<typeof setTimeout> | undefined;
+  const deadline = new Promise<never>((_, 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<boolean> {
-  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<void> {
-  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');
   }
 }