From b04b1f2995a8ced84d432304b2316ffd14ecf34e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Sat, 3 Oct 2026 18:27:28 +0200 Subject: [PATCH] fix(encryption): sample several stored values before declaring a key mismatch The boot check decided on the first value it found under a key id, so one damaged or foreign-sealed row could take a correctly keyed server down with 503 on every request. The probe now samples up to five values, one per column first. One value that opens clears the key; a mismatch needs at least two values and none opening. A single value that does not open is inconclusive: the server keeps serving, writes no canary, logs a warning and probes again at the next start. Integration setup truncates encryption_key_canaries between tests. --- src/lib/boot/key-check.ts | 17 ++- src/lib/crypto/__tests__/canary.test.ts | 85 ++++++++++++-- src/lib/crypto/canary.ts | 110 ++++++++++++++---- .../integration/encryption-key-check.test.ts | 37 ++++++ tests/integration/setup.ts | 3 + 5 files changed, 215 insertions(+), 37 deletions(-) diff --git a/src/lib/boot/key-check.ts b/src/lib/boot/key-check.ts index 0c77f820c..12c0c22bb 100644 --- a/src/lib/boot/key-check.ts +++ b/src/lib/boot/key-check.ts @@ -33,10 +33,19 @@ export async function runBootKeyCheck(client: CanaryClient): Promise { console.warn( `[boot] Encryption key check skipped: ${outcome.message.slice(0, 300)}`, ); - } else if (outcome.written.length > 0) { - console.info( - `[boot] Encryption key check: recorded key id(s) ${outcome.written.join(", ")}`, - ); + } else { + if (outcome.inconclusive.length > 0) { + console.warn( + `[boot] Encryption key check inconclusive for key id(s) ${outcome.inconclusive.join(", ")}: ` + + "the only stored value found under that id did not open. Serving; " + + "no canary written; the check runs again at the next start.", + ); + } + if (outcome.written.length > 0) { + console.info( + `[boot] Encryption key check: recorded key id(s) ${outcome.written.join(", ")}`, + ); + } } return false; } diff --git a/src/lib/crypto/__tests__/canary.test.ts b/src/lib/crypto/__tests__/canary.test.ts index b99220d2a..170e1e5bf 100644 --- a/src/lib/crypto/__tests__/canary.test.ts +++ b/src/lib/crypto/__tests__/canary.test.ts @@ -29,7 +29,8 @@ function useKey(hex: string) { /** An in-memory stand-in for the two raw statements and the probe delegates. */ function fakeClient(opts: { canaries?: Record; - probe?: Record>; + /** delegate → field → stored values (in row order). */ + probe?: Record>; }) { const canaries = new Map(Object.entries(opts.canaries ?? {})); const inserts: string[] = []; @@ -49,9 +50,16 @@ function fakeClient(opts: { } as unknown as CanaryClient & Record; for (const [delegate, fields] of Object.entries(opts.probe ?? {})) { client[delegate] = { - async findFirst(args: { select: Record }) { + async findMany(args: { + select: Record; + take: number; + skip?: number; + }) { const field = Object.keys(args.select)[0]; - return field in fields ? { [field]: fields[field] } : null; + const skip = args.skip ?? 0; + return (fields[field] ?? []) + .slice(skip, skip + args.take) + .map((value) => ({ [field]: value })); }, }; } @@ -67,7 +75,12 @@ describe("encryption key canary", () => { it("writes a canary for a configured key with no row and no data", async () => { const { client, canaries, inserts } = fakeClient({}); const outcome = await checkEncryptionKeyCanaries(client); - expect(outcome).toEqual({ state: "ok", written: ["v1"], verified: [] }); + expect(outcome).toEqual({ + state: "ok", + written: ["v1"], + verified: [], + inconclusive: [], + }); expect(inserts).toEqual(["v1"]); expect(canaries.get("v1")?.startsWith("v1.")).toBe(true); }); @@ -77,7 +90,12 @@ describe("encryption key canary", () => { canaries: { v1: encryptUnderKeyId(canaryPlaintext("v1"), "v1") }, }); const outcome = await checkEncryptionKeyCanaries(client); - expect(outcome).toEqual({ state: "ok", written: [], verified: ["v1"] }); + expect(outcome).toEqual({ + state: "ok", + written: [], + verified: ["v1"], + inconclusive: [], + }); expect(inserts).toEqual([]); }); @@ -100,10 +118,15 @@ describe("encryption key canary", () => { }); it("refuses to seal a wrong key when existing data predates the canary", async () => { - const existing = encrypt("a stored token"); + const existing = [encrypt("a stored token"), encrypt("another token")]; useKey(KEY_B); const { client, inserts } = fakeClient({ - probe: { user: { codexAccessTokenEncrypted: existing } }, + probe: { + user: { + codexAccessTokenEncrypted: [existing[0]], + codexRefreshTokenEncrypted: [existing[1]], + }, + }, }); const outcome = await checkEncryptionKeyCanaries(client); expect(outcome).toEqual({ state: "mismatch", keyIds: ["v1"] }); @@ -112,13 +135,59 @@ describe("encryption key canary", () => { it("writes the canary when existing data opens under the key", async () => { const { client, inserts } = fakeClient({ - probe: { user: { codexAccessTokenEncrypted: encrypt("a stored token") } }, + probe: { + user: { codexAccessTokenEncrypted: [encrypt("a stored token")] }, + }, }); const outcome = await checkEncryptionKeyCanaries(client); expect(outcome.state).toBe("ok"); expect(inserts).toEqual(["v1"]); }); + it("one damaged row among good ones does not take the server down", async () => { + // A row that claims key id v1 but does not open (damaged, or sealed by + // something else), found FIRST, then values that open. + const damaged = "v1." + Buffer.alloc(40, 7).toString("base64"); + const { client, inserts } = fakeClient({ + probe: { + user: { + codexAccessTokenEncrypted: [damaged], + codexRefreshTokenEncrypted: [encrypt("a stored token")], + }, + }, + }); + const outcome = await checkEncryptionKeyCanaries(client); + expect(outcome.state).toBe("ok"); + expect(inserts).toEqual(["v1"]); + }); + + it("several values in one column count, so a wrong key with one busy column still refuses", async () => { + const values = [encrypt("one"), encrypt("two"), encrypt("three")]; + useKey(KEY_B); + const { client, inserts } = fakeClient({ + probe: { user: { codexAccessTokenEncrypted: values } }, + }); + const outcome = await checkEncryptionKeyCanaries(client); + expect(outcome).toEqual({ state: "mismatch", keyIds: ["v1"] }); + expect(inserts).toEqual([]); + }); + + it("a single value that does not open is inconclusive: no mismatch, no canary", async () => { + const existing = encrypt("the only stored token"); + useKey(KEY_B); + const { client, inserts } = fakeClient({ + probe: { user: { codexAccessTokenEncrypted: [existing] } }, + }); + const outcome = await checkEncryptionKeyCanaries(client); + expect(outcome).toEqual({ + state: "ok", + written: [], + verified: [], + inconclusive: ["v1"], + }); + expect(inserts).toEqual([]); + }); + it("reports an error, not a mismatch, when the table cannot be read", async () => { const client = { $queryRaw: async () => { diff --git a/src/lib/crypto/canary.ts b/src/lib/crypto/canary.ts index e925b8d43..e4f77178e 100644 --- a/src/lib/crypto/canary.ts +++ b/src/lib/crypto/canary.ts @@ -12,16 +12,33 @@ * * - row present: it must decrypt to the expected value, else the key is not * the one the row was written with; - * - row absent: before writing one, look for ANY existing ciphertext under - * that key id in the registered string columns and try it. Without this + * - row absent: before writing one, sample existing ciphertext under that + * key id from the registered string columns and try it. Without this * probe, the first boot after an upgrade (or a re-keyed boot over a * database that predates the canary) would seal the wrong key as the - * right one. A value that opens, or no value at all, lets the canary be - * written (`ON CONFLICT DO NOTHING`, so two processes racing is fine). + * right one. * - * The probe is bounded by a time budget; past it, the canary is written - * without a verdict from existing data, which is exactly the behaviour before - * this check existed. + * The probe samples up to `PROBE_SAMPLE_LIMIT` values, one per column first + * (different columns are different writers, so one bad writer cannot speak + * for the whole database), then further values from columns that had any. + * Its verdict, and what each one does: + * + * - `opens`: at least one value opened. A wrong key opens nothing, so the + * key is right and any value that failed is a damaged or foreign row, not + * a key problem. The canary is written. + * - `fails`: two or more values were tried and none opened. That is what a + * wrong key looks like; the process refuses (mismatch). + * - `inconclusive`: exactly one value was found and it did not open. One + * row cannot tell a wrong key from one damaged row, and the two possible + * mistakes are not equal: declaring a mismatch takes a correctly keyed + * server down with 503 on every request, while serving keeps today's + * behaviour. So the process serves, but writes NO canary either (that + * would seal a key that may be wrong), logs a warning, and probes again + * at the next boot, by which time more data may decide it. + * - `none`: nothing found inside the time budget. The canary is written, + * which is exactly the behaviour before this check existed. + * + * Writes are `ON CONFLICT DO NOTHING`, so two processes racing is fine. * * Never logs or returns key material. The canary is written by raw SQL and is * deliberately not in the rotation registry: it belongs to its key id. @@ -40,6 +57,9 @@ export function canaryPlaintext(keyId: string): string { return `${CANARY_PREFIX}${keyId}`; } +/** How many existing values the probe tries at most per key id. */ +export const PROBE_SAMPLE_LIMIT = 5; + /** How long the existing-data probe may take before the canary is written anyway. */ export const PROBE_BUDGET_MS = 10_000; @@ -56,12 +76,16 @@ export interface CanaryClient { } type ProbeDelegate = { - findFirst: (args: { + findMany: (args: { where: Record; select: Record; - }) => Promise | null>; + take: number; + skip?: number; + }) => Promise>>; }; +export type ProbeVerdict = "opens" | "fails" | "inconclusive" | "none"; + export type KeyCheckOutcome = | { state: "ok"; @@ -69,6 +93,11 @@ export type KeyCheckOutcome = written: string[]; /** Key ids whose canary existed and opened. */ verified: string[]; + /** + * Key ids with no canary whose only existing value did not open: not a + * mismatch, no canary written, probed again at the next boot. + */ + inconclusive: string[]; } | { state: "mismatch"; keyIds: string[] } | { state: "error"; message: string }; @@ -84,36 +113,65 @@ function opensAs(ciphertext: string, keyId: string, expected?: string) { } /** - * Look for one existing value sealed under `keyId` and try it. Returns - * `"opens"`, `"fails"`, or `"none"` (no value found inside the budget). + * Sample existing values sealed under `keyId` and try them; see the module + * comment for what each verdict means and why. */ export async function probeExistingData( client: unknown, keyId: string, deadline: number, -): Promise<"opens" | "fails" | "none"> { +): Promise { const delegates = client as Record; - for (const column of ENCRYPTED_COLUMNS) { - if (Date.now() > deadline) return "none"; - if (column.kind !== "string" || column.codec || column.codecField) { - continue; - } + const columns = ENCRYPTED_COLUMNS.filter( + (c) => c.kind === "string" && !c.codec && !c.codecField, + ); + let tried = 0; + const columnsWithData: typeof columns = []; + + const sample = async ( + column: (typeof columns)[number], + skip: number, + take: number, + ): Promise => { const delegate = delegates[column.model.charAt(0).toLowerCase() + column.model.slice(1)]; - if (!delegate || typeof delegate.findFirst !== "function") continue; - let row: Record | null; + if (!delegate || typeof delegate.findMany !== "function") return []; try { - row = await delegate.findFirst({ + const rows = await delegate.findMany({ where: { [column.field]: { startsWith: `${keyId}.` } }, select: { [column.field]: true }, + take, + ...(skip > 0 ? { skip } : {}), }); + return rows + .map((row) => row[column.field]) + .filter((v): v is string => typeof v === "string"); } catch { - continue; + return []; } - const value = row?.[column.field]; - if (typeof value !== "string") continue; - return opensAs(value, keyId) ? "opens" : "fails"; + }; + + // Pass 1: one value per column, so different writers are heard first. + for (const column of columns) { + if (tried >= PROBE_SAMPLE_LIMIT || Date.now() > deadline) break; + const values = await sample(column, 0, 1); + if (values.length === 0) continue; + columnsWithData.push(column); + tried += 1; + if (opensAs(values[0], keyId)) return "opens"; } + // Pass 2: more values from the columns that had any, up to the limit. + for (const column of columnsWithData) { + if (tried >= PROBE_SAMPLE_LIMIT || Date.now() > deadline) break; + const values = await sample(column, 1, PROBE_SAMPLE_LIMIT - tried); + for (const value of values) { + tried += 1; + if (opensAs(value, keyId)) return "opens"; + } + } + + if (tried >= 2) return "fails"; + if (tried === 1) return "inconclusive"; return "none"; } @@ -159,9 +217,11 @@ export async function checkEncryptionKeyCanaries( const deadline = Date.now() + (options.probeBudgetMs ?? PROBE_BUDGET_MS); const toWrite: string[] = []; + const inconclusive: string[] = []; for (const keyId of missing) { const probe = await probeExistingData(client, keyId, deadline); if (probe === "fails") mismatched.push(keyId); + else if (probe === "inconclusive") inconclusive.push(keyId); else toWrite.push(keyId); } @@ -180,7 +240,7 @@ export async function checkEncryptionKeyCanaries( return { state: "error", message: (err as Error).message }; } - return { state: "ok", written: toWrite, verified }; + return { state: "ok", written: toWrite, verified, inconclusive }; } /** diff --git a/tests/integration/encryption-key-check.test.ts b/tests/integration/encryption-key-check.test.ts index 1838a7c58..926221f86 100644 --- a/tests/integration/encryption-key-check.test.ts +++ b/tests/integration/encryption-key-check.test.ts @@ -170,6 +170,7 @@ describe("boot encryption key check (real Postgres)", () => { username: "existing", email: "existing@example.test", codexAccessTokenEncrypted: encrypt("a stored token"), + codexRefreshTokenEncrypted: encrypt("another stored token"), }, }); @@ -186,6 +187,42 @@ describe("boot encryption key check (real Postgres)", () => { }); }); +describe("boot key check probe over existing data (real Postgres)", () => { + it("one damaged row among good ones still lets the right key through", async () => { + const prisma = getPrismaClient(); + await prisma.user.create({ + data: { + username: "damaged", + email: "damaged@example.test", + codexAccessTokenEncrypted: + "v1." + Buffer.alloc(40, 7).toString("base64"), + codexRefreshTokenEncrypted: encrypt("a good token"), + }, + }); + expect(await runBootKeyCheck(prisma)).toBe(false); + expect(isKeyMismatch()).toBe(false); + expect(await prisma.encryptionKeyCanary.count()).toBe(1); + }); + + it("a single value that does not open neither refuses nor writes a canary", async () => { + const prisma = getPrismaClient(); + await prisma.user.create({ + data: { + username: "single", + email: "single@example.test", + codexAccessTokenEncrypted: encrypt("the only token"), + }, + }); + useKey(KEY_B); + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + expect(await runBootKeyCheck(prisma)).toBe(false); + expect(warn.mock.calls[0]?.[0]).toContain("inconclusive"); + warn.mockRestore(); + expect(isKeyMismatch()).toBe(false); + expect(await prisma.encryptionKeyCanary.count()).toBe(0); + }); +}); + describe("encryption key backup step (real Postgres)", () => { it("is due, refuses a stale confirmation, records the right one, and is due again after a re-key", async () => { const prisma = getPrismaClient(); diff --git a/tests/integration/setup.ts b/tests/integration/setup.ts index 27d284385..44d3d9996 100644 --- a/tests/integration/setup.ts +++ b/tests/integration/setup.ts @@ -51,6 +51,9 @@ export async function truncateAllTables(client: PrismaClient): Promise { "coach_usage", "data_backups", "devices", + // Instance-wide, keyed by encryption key id: a canary left by one test + // would make the next test's boot check verify instead of write. + "encryption_key_canaries", "host_metrics", "idempotency_keys", "integration_statuses",