From e63ae40c611e604996daf45d6df56da8b24dbf3c Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Sat, 31 Oct 2026 23:44:43 +0100 Subject: [PATCH] fix: keep one plugin signing key when several parts of the server create it at once The registry signature test now drops the cached key before each test: the key is cached once per process on globalThis, so resetting the modules no longer clears it. --- .../plugin-registry-signature.test.ts | 2 + .../__tests__/plugin-signing-race.test.ts | 79 +++++++++++++ lib/admin/plugin-signing.ts | 110 ++++++++++++++---- 3 files changed, 167 insertions(+), 24 deletions(-) create mode 100644 lib/admin/__tests__/plugin-signing-race.test.ts diff --git a/lib/admin/__tests__/plugin-registry-signature.test.ts b/lib/admin/__tests__/plugin-registry-signature.test.ts index 0afd4c529..e5e8754a3 100644 --- a/lib/admin/__tests__/plugin-registry-signature.test.ts +++ b/lib/admin/__tests__/plugin-registry-signature.test.ts @@ -10,6 +10,8 @@ beforeEach(async () => { dir = await mkdtemp(path.join(tmpdir(), 'bw-plugin-registry-')); vi.stubEnv('ADMIN_CONFIG_DIR', dir); vi.resetModules(); + // The signing key is cached on globalThis (one per process, not per module copy). + (await import('../plugin-signing')).invalidatePluginSigningCache(); }); afterEach(async () => { diff --git a/lib/admin/__tests__/plugin-signing-race.test.ts b/lib/admin/__tests__/plugin-signing-race.test.ts new file mode 100644 index 000000000..e22f125d0 --- /dev/null +++ b/lib/admin/__tests__/plugin-signing-race.test.ts @@ -0,0 +1,79 @@ +// @vitest-environment node +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { createPublicKey, verify } from 'node:crypto'; +import { mkdtemp, readFile, rm } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import path from 'node:path'; + +// Next evaluates plugin-signing.ts once per bundle entry (three copies in +// the build), and a second process can share the config dir. Whoever makes +// the key first must be the only one: before, each writer generated its +// own key and the last write won, so the signatures made with the other +// key stopped verifying. + +const GLOBAL_KEY = Symbol.for('bulwark.admin.pluginSigning'); + +let dir: string; + +beforeEach(async () => { + dir = await mkdtemp(path.join(tmpdir(), 'bw-signing-race-')); + vi.stubEnv('ADMIN_CONFIG_DIR', dir); + vi.stubEnv('PLUGIN_SIGNING_KEY_FILE', ''); + delete (globalThis as Record)[GLOBAL_KEY]; +}); + +afterEach(async () => { + delete (globalThis as Record)[GLOBAL_KEY]; + vi.unstubAllEnvs(); + await rm(dir, { recursive: true, force: true }); +}); + +/** A fresh copy of the module, as another bundle entry has. */ +async function moduleCopy() { + vi.resetModules(); + return import('../plugin-signing'); +} + +async function verifiesWithStoredKey(payload: string, signature: string): Promise { + const pem = await readFile(path.join(dir, 'plugin-signing.key'), 'utf-8'); + return verify(null, Buffer.from(payload), createPublicKey(pem), Buffer.from(signature, 'base64')); +} + +describe('plugin signing key created by several writers at once', () => { + it('keeps one key that every module copy signs with, also after a rotation', async () => { + const copies = [await moduleCopy(), await moduleCopy(), await moduleCopy()]; + const signatures = await Promise.all(copies.map((copy, i) => copy.signBytes(`payload ${i}`))); + for (const [i, signature] of signatures.entries()) { + expect(await verifiesWithStoredKey(`payload ${i}`, signature)).toBe(true); + } + + // The operator replaces the key; one copy is told, all of them use it. + const { generateKeyPairSync } = await import('node:crypto'); + const { writeFile } = await import('node:fs/promises'); + const pem = generateKeyPairSync('ed25519').privateKey.export({ type: 'pkcs8', format: 'pem' }) as string; + await writeFile(path.join(dir, 'plugin-signing.key'), pem); + copies[0].invalidatePluginSigningCache(); + for (const [i, copy] of copies.entries()) { + expect(await verifiesWithStoredKey(`rotated ${i}`, await copy.signBytes(`rotated ${i}`))).toBe(true); + } + }); + + it('keeps the first key when another writer (another process) creates it at the same moment', async () => { + const copies = [await moduleCopy(), await moduleCopy(), await moduleCopy(), await moduleCopy()]; + for (let round = 0; round < 5; round++) { + await rm(path.join(dir, 'plugin-signing.key'), { force: true }); + const writers = copies.map((copy, i) => { + // Each writer starts with nothing loaded and shares nothing in + // memory with the others, like a separate process. + delete (globalThis as Record)[GLOBAL_KEY]; + copy.invalidatePluginSigningCache(); + return copy.signBytes(`round ${round} writer ${i}`); + }); + delete (globalThis as Record)[GLOBAL_KEY]; + const signatures = await Promise.all(writers); + for (const [i, signature] of signatures.entries()) { + expect(await verifiesWithStoredKey(`round ${round} writer ${i}`, signature)).toBe(true); + } + } + }); +}); diff --git a/lib/admin/plugin-signing.ts b/lib/admin/plugin-signing.ts index f4591a9f8..6a1045d41 100644 --- a/lib/admin/plugin-signing.ts +++ b/lib/admin/plugin-signing.ts @@ -12,16 +12,32 @@ import { generateKeyPairSync, createPrivateKey, createPublicKey, sign as nodeSign, KeyObject } from 'node:crypto'; import { readFile, writeFile, chmod } from 'node:fs/promises'; -import { existsSync } from 'node:fs'; import { ensureConfigDir, getConfigPath, assertWritable } from './paths'; import { logger } from '@/lib/logger'; const KEY_FILENAME = 'plugin-signing.key'; -let cached: { privateKey: KeyObject; publicKey: KeyObject } | null = null; -let initPromise: Promise | null = null; +type KeyPair = { privateKey: KeyObject; publicKey: KeyObject }; -function parseKey(pem: string, source: string): { privateKey: KeyObject; publicKey: KeyObject } { +/** + * The loaded key, on globalThis: Next evaluates this module once per bundle + * entry (three times in the build), and every copy must sign with the same + * key and share one load. + */ +interface SigningGlobals { + cached: KeyPair | null; + init: Promise | null; + /** Bumped by invalidatePluginSigningCache, so a load that started before doesn't win. */ + generation: number; +} +const GLOBAL_KEY = Symbol.for('bulwark.admin.pluginSigning'); + +function signingGlobals(): SigningGlobals { + const g = globalThis as typeof globalThis & { [GLOBAL_KEY]?: SigningGlobals }; + return (g[GLOBAL_KEY] ??= { cached: null, init: null, generation: 0 }); +} + +function parseKey(pem: string, source: string): KeyPair { let privateKey: KeyObject; try { if (!pem.includes('-----BEGIN PRIVATE KEY-----')) throw new Error('not PKCS#8 PEM'); @@ -53,52 +69,96 @@ async function loadOrCreate(): Promise<{ privateKey: KeyObject; publicKey: KeyOb const externalPath = process.env.PLUGIN_SIGNING_KEY_FILE?.trim(); if (externalPath) return loadExternal(externalPath); + return loadOrCreateFile(); +} + +async function readKeyFile(path: string): Promise { + try { + return await readFile(path, 'utf-8'); + } catch (err) { + if ((err as NodeJS.ErrnoException).code === 'ENOENT') return null; + throw err; + } +} + +/** + * The key file in the admin config dir, made on first use. Created with + * `wx`: when two writers start at once (two processes, or module copies + * that missed each other's load), only one key is ever written and the + * other writer reads it back, so every signature verifies against the same + * key. Before, the last write won and signatures made with the first key + * stopped verifying. + */ +async function loadOrCreateFile(): Promise { await ensureConfigDir(); const path = getConfigPath(KEY_FILENAME); - if (existsSync(path)) { - const pem = await readFile(path, 'utf-8'); - return parseKey(pem, KEY_FILENAME); - } + const existing = await readKeyFile(path); + if (existing !== null) return parseExistingKey(path, existing); - // First boot: generate and persist. Use sync APIs so a half-written file - // never lingers if the process dies between writes. assertWritable('plugin-signing.generateKeypair'); const { privateKey, publicKey } = generateKeyPairSync('ed25519'); const pem = privateKey.export({ type: 'pkcs8', format: 'pem' }) as string; - await writeFile(path, pem, { encoding: 'utf-8', mode: 0o600 }); + try { + await writeFile(path, pem, { encoding: 'utf-8', mode: 0o600, flag: 'wx' }); + } catch (err) { + if ((err as NodeJS.ErrnoException).code !== 'EEXIST') throw err; + // Someone else made the key first: use theirs. + return parseExistingKey(path, (await readKeyFile(path)) ?? ''); + } // Ensure 0600 on filesystems that ignored mode on writeFile. try { await chmod(path, 0o600); } catch { /* best effort */ } logger.info('[plugin-signing] generated new Ed25519 keypair'); return { privateKey, publicKey }; } -async function ensureLoaded(): Promise { - if (cached) return; - if (!initPromise) { - initPromise = (async () => { +/** + * Parse a key file another writer may still be writing: an empty or cut + * off file is read again for up to a second before it counts as broken. + */ +async function parseExistingKey(path: string, pem: string): Promise { + for (let attempt = 0; ; attempt++) { + try { + return parseKey(pem, KEY_FILENAME); + } catch (err) { + if (attempt >= 20 || pem.includes('-----END PRIVATE KEY-----')) throw err; + await new Promise((resolve) => setTimeout(resolve, 50)); + pem = (await readKeyFile(path)) ?? ''; + } + } +} + +async function ensureLoaded(): Promise { + const g = signingGlobals(); + if (g.cached) return g.cached; + if (!g.init) { + const generation = g.generation; + g.init = (async () => { try { - cached = await loadOrCreate(); + const loaded = await loadOrCreate(); + // A rotation (invalidatePluginSigningCache) during the load wins. + if (g.generation === generation) g.cached = loaded; } catch (err) { - initPromise = null; + if (g.generation === generation) g.init = null; logger.error('[plugin-signing] keypair load failed', { error: err instanceof Error ? err.message : String(err) }); throw err; } })(); } - await initPromise; + await g.init; + return g.cached ?? ensureLoaded(); } // ─── Public API ────────────────────────────────────────────── /** Returns the public key as a raw 32-byte Uint8Array (Ed25519 standard form). */ export async function getPublicKeyRaw(): Promise { - await ensureLoaded(); + const { publicKey } = await ensureLoaded(); // Export as SPKI DER and pull the last 32 bytes (the raw key after the // 12-byte AlgorithmIdentifier prefix). Node has no built-in raw export // for Ed25519, but the SPKI prefix is fixed for Ed25519 so the slice is // safe. - const spki = cached!.publicKey.export({ type: 'spki', format: 'der' }) as Buffer; + const spki = publicKey.export({ type: 'spki', format: 'der' }) as Buffer; if (spki.length < 32) throw new Error('SPKI export too short'); return new Uint8Array(spki.subarray(spki.length - 32)); } @@ -111,14 +171,16 @@ export async function getPublicKeyBase64(): Promise { /** Sign `bytes` and return a base64-encoded 64-byte Ed25519 signature. */ export async function signBytes(bytes: Uint8Array | string): Promise { - await ensureLoaded(); + const { privateKey } = await ensureLoaded(); const data = typeof bytes === 'string' ? Buffer.from(bytes, 'utf-8') : Buffer.from(bytes); - const sig = nodeSign(null, data, cached!.privateKey); + const sig = nodeSign(null, data, privateKey); return sig.toString('base64'); } /** Force a re-read on next access. Used after operator rotates the key. */ export function invalidatePluginSigningCache(): void { - cached = null; - initPromise = null; + const g = signingGlobals(); + g.cached = null; + g.init = null; + g.generation++; }