From cc6cc343a5362d5dc7fb5bf5bec2834148f2f7f5 Mon Sep 17 00:00:00 2001 From: Cheems <94773058+itscheems@users.noreply.github.com> Date: Fri, 31 Jul 2026 09:29:16 +0800 Subject: [PATCH 1/4] fix(byok): reuse the Windows DPAPI worker Keep one PowerShell DPAPI worker alive for the daemon lifetime so packaged Windows pays the expensive script and assembly startup cost once, then reuses a local named pipe for credential operations. Preserve the existing DPAPI CurrentUser storage format and cover worker reuse across availability, set, get, and delete operations. --- apps/daemon/src/byok/credential-service.ts | 177 ++------ apps/daemon/src/byok/windows-dpapi-worker.ts | 396 ++++++++++++++++++ .../tests/byok/credential-service.test.ts | 34 +- 3 files changed, 455 insertions(+), 152 deletions(-) create mode 100644 apps/daemon/src/byok/windows-dpapi-worker.ts diff --git a/apps/daemon/src/byok/credential-service.ts b/apps/daemon/src/byok/credential-service.ts index 30be2170a1d..4a7ee8d1b45 100644 --- a/apps/daemon/src/byok/credential-service.ts +++ b/apps/daemon/src/byok/credential-service.ts @@ -11,6 +11,7 @@ import { isNodePtyUnavailableError, loadNodePty, } from '../services/node-pty.js'; +import { WindowsDpapiWorker } from './windows-dpapi-worker.js'; const PROFILE_ID_PATTERN = /^byok-[a-z0-9][a-z0-9._-]{2,95}$/u; const KEYCHAIN_SERVICE = 'dev.opendesign.byok'; @@ -435,91 +436,22 @@ class LinuxSecretServiceBackend implements ByokSecretBackend { } } -const WINDOWS_DPAPI_SCRIPT = ` -$ErrorActionPreference = 'Stop' -Add-Type -AssemblyName System.Security -$operation = $env:OD_BYOK_DPAPI_OPERATION -$secretPath = $env:OD_BYOK_DPAPI_PATH - -try { - switch ($operation) { - 'probe' { - $plain = [System.Text.Encoding]::UTF8.GetBytes('open-design-dpapi-probe') - $cipher = [System.Security.Cryptography.ProtectedData]::Protect( - $plain, - $null, - [System.Security.Cryptography.DataProtectionScope]::CurrentUser - ) - $roundTrip = [System.Security.Cryptography.ProtectedData]::Unprotect( - $cipher, - $null, - [System.Security.Cryptography.DataProtectionScope]::CurrentUser - ) - if ([System.Text.Encoding]::UTF8.GetString($roundTrip) -ne 'open-design-dpapi-probe') { - throw 'DPAPI probe failed' - } - } - 'set' { - $secret = [Console]::In.ReadToEnd() - if ([string]::IsNullOrWhiteSpace($secret)) { - throw 'Secret must not be empty' - } - $plain = [System.Text.Encoding]::UTF8.GetBytes($secret) - $cipher = [System.Security.Cryptography.ProtectedData]::Protect( - $plain, - $null, - [System.Security.Cryptography.DataProtectionScope]::CurrentUser - ) - $directory = [System.IO.Path]::GetDirectoryName($secretPath) - [System.IO.Directory]::CreateDirectory($directory) | Out-Null - $temporaryPath = "$secretPath.$([Guid]::NewGuid().ToString('N')).tmp" - try { - [System.IO.File]::WriteAllBytes($temporaryPath, $cipher) - Move-Item -LiteralPath $temporaryPath -Destination $secretPath -Force - } finally { - if (Test-Path -LiteralPath $temporaryPath) { - Remove-Item -LiteralPath $temporaryPath -Force - } - } - } - 'get' { - if (-not (Test-Path -LiteralPath $secretPath -PathType Leaf)) { - exit 44 - } - $cipher = [System.IO.File]::ReadAllBytes($secretPath) - $plain = [System.Security.Cryptography.ProtectedData]::Unprotect( - $cipher, - $null, - [System.Security.Cryptography.DataProtectionScope]::CurrentUser - ) - [Console]::Out.Write([System.Text.Encoding]::UTF8.GetString($plain)) - } - 'delete' { - if (-not (Test-Path -LiteralPath $secretPath -PathType Leaf)) { - exit 44 - } - Remove-Item -LiteralPath $secretPath -Force - } - default { - throw 'Unsupported DPAPI operation' - } - } -} catch { - [Console]::Error.WriteLine('Open Design secure credential operation failed.') - exit 1 -} -`; +type WindowsDpapiWorkerClient = Pick; -const WINDOWS_DPAPI_ENCODED_SCRIPT = Buffer.from( - WINDOWS_DPAPI_SCRIPT, - 'utf16le', -).toString('base64'); +export type WindowsDpapiBackendOptions = { + commandAvailable?: (command: string) => Promise; + createWorker?: () => Promise; +}; -class WindowsDpapiBackend implements ByokSecretBackend { +export class WindowsDpapiBackend implements ByokSecretBackend { readonly kind = 'windows-dpapi'; private availability: Promise | null = null; + private worker: Promise | null = null; - constructor(private readonly secretsDir: string) {} + constructor( + private readonly secretsDir: string, + private readonly options: WindowsDpapiBackendOptions = {}, + ) {} async available() { this.availability ??= this.probeAvailability(); @@ -527,9 +459,11 @@ class WindowsDpapiBackend implements ByokSecretBackend { } private async probeAvailability() { - if (!(await commandAvailable('powershell.exe'))) return false; + if (!(await (this.options.commandAvailable ?? commandAvailable)('powershell.exe'))) { + return false; + } try { - await runWindowsDpapiCommand('probe', this.secretsDir); + await (await this.getWorker()).ready(); return true; } catch { return false; @@ -538,7 +472,7 @@ class WindowsDpapiBackend implements ByokSecretBackend { async set(profileId: string, secret: string) { assertProfileId(profileId); - await runWindowsDpapiCommand( + await (await this.getWorker()).run( 'set', path.join(this.secretsDir, `${profileId}.bin`), secret, @@ -547,22 +481,24 @@ class WindowsDpapiBackend implements ByokSecretBackend { async get(profileId: string) { assertProfileId(profileId); - return runWindowsDpapiCommand( + const result = await (await this.getWorker()).run( 'get', path.join(this.secretsDir, `${profileId}.bin`), - undefined, - true, ); + return result.found ? result.value : null; } async delete(profileId: string) { assertProfileId(profileId); - return (await runWindowsDpapiCommand( + return (await (await this.getWorker()).run( 'delete', path.join(this.secretsDir, `${profileId}.bin`), - undefined, - true, - )) !== null; + )).found; + } + + private getWorker(): Promise { + this.worker ??= this.options.createWorker?.() ?? WindowsDpapiWorker.create(); + return this.worker; } } @@ -579,67 +515,6 @@ class UnavailableSecretBackend implements ByokSecretBackend { async delete() { return false; } } -async function runWindowsDpapiCommand( - operation: 'probe' | 'set' | 'get' | 'delete', - secretPath: string, - secretInput?: string, - allowNotFound = false, -): Promise { - return new Promise((resolve, reject) => { - const child = spawn('powershell.exe', [ - '-NoLogo', - '-NoProfile', - '-NonInteractive', - '-ExecutionPolicy', - 'Bypass', - '-EncodedCommand', - WINDOWS_DPAPI_ENCODED_SCRIPT, - ], { - env: { - ...process.env, - OD_BYOK_DPAPI_OPERATION: operation, - OD_BYOK_DPAPI_PATH: secretPath, - }, - stdio: ['pipe', 'pipe', 'pipe'], - windowsHide: true, - }); - const stdout: Buffer[] = []; - let stdoutBytes = 0; - let settled = false; - const finish = (error: Error | null, output?: string | null) => { - if (settled) return; - settled = true; - if (error) reject(error); - else resolve(output === undefined ? '' : output); - }; - child.stdout.on('data', (chunk: Buffer) => { - stdoutBytes += chunk.length; - if (stdoutBytes <= MAX_SECRET_OUTPUT_BYTES) stdout.push(chunk); - }); - child.stderr.resume(); - child.on('error', () => { - finish(new Error('Secure credential backend command failed.')); - }); - child.on('close', (code) => { - if (stdoutBytes > MAX_SECRET_OUTPUT_BYTES) { - finish(new Error('Secure credential backend command failed.')); - return; - } - if (code === 0) { - finish(null, Buffer.concat(stdout).toString('utf8')); - return; - } - if (allowNotFound && code === 44) { - finish(null, null); - return; - } - finish(new Error('Secure credential backend command failed.')); - }); - if (secretInput === undefined) child.stdin.end(); - else child.stdin.end(secretInput); - }); -} - async function commandAvailable(command: string): Promise { if (path.isAbsolute(command)) { try { diff --git a/apps/daemon/src/byok/windows-dpapi-worker.ts b/apps/daemon/src/byok/windows-dpapi-worker.ts new file mode 100644 index 00000000000..983b9f4d2e6 --- /dev/null +++ b/apps/daemon/src/byok/windows-dpapi-worker.ts @@ -0,0 +1,396 @@ +import type { ChildProcessWithoutNullStreams, SpawnOptionsWithoutStdio } from 'node:child_process'; +import { spawn } from 'node:child_process'; +import { randomUUID } from 'node:crypto'; +import type { Server, Socket } from 'node:net'; +import { createServer } from 'node:net'; + +const MAX_WORKER_LINE_BYTES = 64 * 1024; + +// Keep one PowerShell process alive for the daemon lifetime. Packaged Windows +// can spend tens of seconds scanning each new encoded PowerShell command, so a +// process per probe/set/get/delete makes a single BYOK request exceed its IPC +// timeout even though the DPAPI calls themselves finish in milliseconds. +export type WindowsDpapiWorkerOperation = 'set' | 'get' | 'delete'; + +export type WindowsDpapiWorkerResult = { + found: boolean; + value: string | null; +}; + +type WindowsDpapiWorkerRequest = { + id: string; + operation: WindowsDpapiWorkerOperation; + path: string; + secret?: string; +}; + +type WindowsDpapiWorkerResponse = { + found?: boolean; + id?: string; + ok?: boolean; + type?: string; + value?: string; +}; + +type SpawnWindowsDpapiWorker = ( + command: string, + args: readonly string[], + options: SpawnOptionsWithoutStdio & { + stdio: ['pipe', 'pipe', 'pipe']; + }, +) => ChildProcessWithoutNullStreams; + +const WINDOWS_DPAPI_WORKER_SCRIPT = ` +$ErrorActionPreference = 'Stop' +function Write-OpenDesignDpapiResponse([hashtable]$response) { + $writer.WriteLine(($response | ConvertTo-Json -Compress)) +} + +$pipe = New-Object System.IO.Pipes.NamedPipeClientStream( + '.', + $env:OD_BYOK_DPAPI_PIPE_NAME, + [System.IO.Pipes.PipeDirection]::InOut, + [System.IO.Pipes.PipeOptions]::None +) +$pipe.Connect() +$reader = New-Object System.IO.StreamReader( + $pipe, + [System.Text.Encoding]::UTF8, + $false, + 1024, + $true +) +$writer = New-Object System.IO.StreamWriter( + $pipe, + (New-Object System.Text.UTF8Encoding($false)), + 1024, + $true +) +$writer.AutoFlush = $true + +try { + [void][System.Reflection.Assembly]::LoadFrom( + [System.IO.Path]::Combine( + [System.Runtime.InteropServices.RuntimeEnvironment]::GetRuntimeDirectory(), + 'System.Security.dll' + ) + ) + $probePlain = [System.Text.Encoding]::UTF8.GetBytes('open-design-dpapi-probe') + $probeCipher = [System.Security.Cryptography.ProtectedData]::Protect( + $probePlain, + $null, + [System.Security.Cryptography.DataProtectionScope]::CurrentUser + ) + $probeRoundTrip = [System.Security.Cryptography.ProtectedData]::Unprotect( + $probeCipher, + $null, + [System.Security.Cryptography.DataProtectionScope]::CurrentUser + ) + if ([System.Text.Encoding]::UTF8.GetString($probeRoundTrip) -ne 'open-design-dpapi-probe') { + throw 'DPAPI probe failed' + } + Write-OpenDesignDpapiResponse @{ type = 'ready'; ok = $true } +} catch { + Write-OpenDesignDpapiResponse @{ type = 'ready'; ok = $false } + exit 1 +} + +while (($line = $reader.ReadLine()) -ne $null) { + $requestId = '' + try { + $request = $line | ConvertFrom-Json + $requestId = [string]$request.id + $operation = [string]$request.operation + $secretPath = [System.Text.Encoding]::UTF8.GetString( + [System.Convert]::FromBase64String([string]$request.path) + ) + + if ($operation -eq 'set') { + $secret = [System.Text.Encoding]::UTF8.GetString( + [System.Convert]::FromBase64String([string]$request.secret) + ) + if ([string]::IsNullOrWhiteSpace($secret)) { + throw 'Secret must not be empty' + } + $plain = [System.Text.Encoding]::UTF8.GetBytes($secret) + $cipher = [System.Security.Cryptography.ProtectedData]::Protect( + $plain, + $null, + [System.Security.Cryptography.DataProtectionScope]::CurrentUser + ) + $directory = [System.IO.Path]::GetDirectoryName($secretPath) + [System.IO.Directory]::CreateDirectory($directory) | Out-Null + $temporaryPath = "$secretPath.$([Guid]::NewGuid().ToString('N')).tmp" + try { + [System.IO.File]::WriteAllBytes($temporaryPath, $cipher) + Move-Item -LiteralPath $temporaryPath -Destination $secretPath -Force + } finally { + if (Test-Path -LiteralPath $temporaryPath) { + Remove-Item -LiteralPath $temporaryPath -Force + } + } + Write-OpenDesignDpapiResponse @{ id = $requestId; ok = $true; found = $true } + continue + } + + if ($operation -eq 'get') { + if (-not (Test-Path -LiteralPath $secretPath -PathType Leaf)) { + Write-OpenDesignDpapiResponse @{ id = $requestId; ok = $true; found = $false } + continue + } + $cipher = [System.IO.File]::ReadAllBytes($secretPath) + $plain = [System.Security.Cryptography.ProtectedData]::Unprotect( + $cipher, + $null, + [System.Security.Cryptography.DataProtectionScope]::CurrentUser + ) + Write-OpenDesignDpapiResponse @{ + id = $requestId + ok = $true + found = $true + value = [System.Convert]::ToBase64String($plain) + } + continue + } + + if ($operation -eq 'delete') { + if (-not (Test-Path -LiteralPath $secretPath -PathType Leaf)) { + Write-OpenDesignDpapiResponse @{ id = $requestId; ok = $true; found = $false } + continue + } + Remove-Item -LiteralPath $secretPath -Force + Write-OpenDesignDpapiResponse @{ id = $requestId; ok = $true; found = $true } + continue + } + + throw 'Unsupported DPAPI operation' + } catch { + Write-OpenDesignDpapiResponse @{ id = $requestId; ok = $false } + } +} +`; + +const WINDOWS_DPAPI_WORKER_ENCODED_SCRIPT = Buffer.from( + WINDOWS_DPAPI_WORKER_SCRIPT, + 'utf16le', +).toString('base64'); + +export class WindowsDpapiWorker { + private readonly readyPromise: Promise; + private resolveReady: (() => void) | null = null; + private rejectReady: ((error: Error) => void) | null = null; + private inputBuffer = ''; + private nextRequestId = 0; + private operationQueue: Promise = Promise.resolve(); + private pending: { + id: string; + reject(error: Error): void; + resolve(response: WindowsDpapiWorkerResponse): void; + } | null = null; + private closed = false; + + private constructor( + private readonly child: ChildProcessWithoutNullStreams, + private readonly pipe: Socket, + ) { + this.readyPromise = new Promise((resolve, reject) => { + this.resolveReady = resolve; + this.rejectReady = reject; + }); + this.pipe.on('data', (chunk: Buffer) => this.acceptInput(chunk)); + this.pipe.on('error', () => this.fail()); + this.pipe.on('close', () => this.fail()); + this.child.stdout.resume(); + this.child.stderr.resume(); + this.child.on('error', () => this.fail()); + this.child.on('close', () => this.fail()); + } + + static async create( + spawnWorker: SpawnWindowsDpapiWorker = spawn, + ): Promise { + const pipeName = `open-design-dpapi-${process.pid}-${randomUUID()}`; + const server = createServer(); + await listen(server, `\\\\.\\pipe\\${pipeName}`); + const connection = waitForConnection(server); + const child = spawnWorker( + 'powershell.exe', + [ + '-NoLogo', + '-NoProfile', + '-NonInteractive', + '-ExecutionPolicy', + 'Bypass', + '-EncodedCommand', + WINDOWS_DPAPI_WORKER_ENCODED_SCRIPT, + ], + { + env: { + ...process.env, + OD_BYOK_DPAPI_PIPE_NAME: pipeName, + }, + stdio: ['pipe', 'pipe', 'pipe'], + windowsHide: true, + }, + ); + // The packaged process must not retain an open stdin stream. Requests use + // the random per-process local pipe below, keeping plaintext credentials + // out of argv, environment variables, and temporary files. + child.stdin.end(); + try { + const pipe = await Promise.race([ + connection, + childFailure(child), + ]); + server.close(); + return new WindowsDpapiWorker(child, pipe); + } catch (error) { + server.close(); + if (!child.killed) child.kill(); + throw error; + } + } + + ready(): Promise { + return this.readyPromise; + } + + run( + operation: WindowsDpapiWorkerOperation, + secretPath: string, + secret?: string, + ): Promise { + const result = this.operationQueue.then( + () => this.runUnlocked(operation, secretPath, secret), + () => this.runUnlocked(operation, secretPath, secret), + ); + this.operationQueue = result.then( + () => undefined, + () => undefined, + ); + return result; + } + + private async runUnlocked( + operation: WindowsDpapiWorkerOperation, + secretPath: string, + secret?: string, + ): Promise { + await this.readyPromise; + if (this.closed || this.pending) throw secureCredentialCommandError(); + const id = String(++this.nextRequestId); + const request: WindowsDpapiWorkerRequest = { + id, + operation, + path: Buffer.from(secretPath, 'utf8').toString('base64'), + ...(secret === undefined + ? {} + : { secret: Buffer.from(secret, 'utf8').toString('base64') }), + }; + const response = await new Promise((resolve, reject) => { + this.pending = { id, reject, resolve }; + this.pipe.write(`${JSON.stringify(request)}\n`, 'utf8', (error) => { + if (error) this.fail(); + }); + }); + if (response.ok !== true || response.id !== id) { + throw secureCredentialCommandError(); + } + if (operation === 'get' && response.found === true) { + if (typeof response.value !== 'string') throw secureCredentialCommandError(); + const value = Buffer.from(response.value, 'base64'); + if (value.byteLength > MAX_WORKER_LINE_BYTES) throw secureCredentialCommandError(); + return { found: true, value: value.toString('utf8') }; + } + return { + found: response.found === true, + value: null, + }; + } + + private acceptInput(chunk: Buffer): void { + if (this.closed) return; + this.inputBuffer += chunk.toString('utf8'); + if (Buffer.byteLength(this.inputBuffer, 'utf8') > MAX_WORKER_LINE_BYTES) { + this.fail(); + return; + } + let newlineIndex = this.inputBuffer.indexOf('\n'); + while (newlineIndex >= 0) { + const line = this.inputBuffer.slice(0, newlineIndex).trim(); + this.inputBuffer = this.inputBuffer.slice(newlineIndex + 1); + if (line) this.acceptLine(line); + if (this.closed) return; + newlineIndex = this.inputBuffer.indexOf('\n'); + } + } + + private acceptLine(line: string): void { + let response: WindowsDpapiWorkerResponse; + try { + response = JSON.parse(line) as WindowsDpapiWorkerResponse; + } catch { + this.fail(); + return; + } + if (response.type === 'ready') { + if (response.ok === true && this.resolveReady) { + this.resolveReady(); + this.resolveReady = null; + this.rejectReady = null; + return; + } + this.fail(); + return; + } + if (!this.pending || response.id !== this.pending.id) { + this.fail(); + return; + } + const pending = this.pending; + this.pending = null; + pending.resolve(response); + } + + private fail(): void { + if (this.closed) return; + this.closed = true; + const error = secureCredentialCommandError(); + this.rejectReady?.(error); + this.resolveReady = null; + this.rejectReady = null; + this.pending?.reject(error); + this.pending = null; + this.pipe.destroy(); + if (!this.child.killed) this.child.kill(); + } +} + +function listen(server: Server, path: string): Promise { + return new Promise((resolve, reject) => { + const onError = (error: Error) => reject(error); + server.once('error', onError); + server.listen(path, () => { + server.off('error', onError); + resolve(); + }); + }); +} + +function waitForConnection(server: Server): Promise { + return new Promise((resolve, reject) => { + server.once('connection', resolve); + server.once('error', reject); + }); +} + +function childFailure(child: ChildProcessWithoutNullStreams): Promise { + return new Promise((_, reject) => { + child.once('error', () => reject(secureCredentialCommandError())); + child.once('close', () => reject(secureCredentialCommandError())); + }); +} + +function secureCredentialCommandError(): Error { + return new Error('Secure credential backend command failed.'); +} diff --git a/apps/daemon/tests/byok/credential-service.test.ts b/apps/daemon/tests/byok/credential-service.test.ts index bed134244bd..b8a6416be42 100644 --- a/apps/daemon/tests/byok/credential-service.test.ts +++ b/apps/daemon/tests/byok/credential-service.test.ts @@ -1,10 +1,11 @@ import { mkdir, mkdtemp, readFile, rm, writeFile } from 'node:fs/promises'; import { tmpdir } from 'node:os'; import path from 'node:path'; -import { afterEach, describe, expect, it } from 'vitest'; +import { afterEach, describe, expect, it, vi } from 'vitest'; import { ByokCredentialService, + WindowsDpapiBackend, createPlatformByokSecretBackend, type ByokSecretBackend, } from '../../src/byok/credential-service.js'; @@ -97,6 +98,37 @@ describe('BYOK credential service', () => { expect(backend.kind).toBe('windows-dpapi'); }); + it('reuses one Windows DPAPI worker across availability and credential operations', async () => { + const worker = { + ready: vi.fn(async () => undefined), + run: vi.fn(async (operation: 'set' | 'get' | 'delete') => { + if (operation === 'get') { + return { found: true, value: 'test-secret' }; + } + return { found: true, value: null }; + }), + }; + const createWorker = vi.fn(async () => worker); + const backend = new WindowsDpapiBackend('/test/byok/secrets', { + commandAvailable: async () => true, + createWorker, + }); + + await expect(backend.available()).resolves.toBe(true); + await expect(backend.available()).resolves.toBe(true); + await expect(backend.set('byok-worker-reuse', 'test-secret')).resolves.toBeUndefined(); + await expect(backend.get('byok-worker-reuse')).resolves.toBe('test-secret'); + await expect(backend.delete('byok-worker-reuse')).resolves.toBe(true); + + expect(createWorker).toHaveBeenCalledTimes(1); + expect(worker.ready).toHaveBeenCalledTimes(1); + expect(worker.run.mock.calls.map(([operation]) => operation)).toEqual([ + 'set', + 'get', + 'delete', + ]); + }); + it('serializes concurrent metadata mutations so profiles cannot overwrite each other', async () => { const dataDir = await mkdtemp(path.join(tmpdir(), 'od-byok-credentials-')); roots.push(dataDir); From 193e63d708003551a70a74c00ccd6147b0ff0a82 Mon Sep 17 00:00:00 2001 From: Cheems <94773058+itscheems@users.noreply.github.com> Date: Fri, 31 Jul 2026 09:29:27 +0800 Subject: [PATCH 2/4] fix(pack): allow Windows desktop evals to run for 60 seconds Match the packaged Windows smoke budget so the first DPAPI worker initialization can complete while retaining the existing timeout boundary for stalled desktop IPC calls. --- tools/pack/src/win/lifecycle.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/pack/src/win/lifecycle.ts b/tools/pack/src/win/lifecycle.ts index 994dddc4ac1..aaf8c75b261 100644 --- a/tools/pack/src/win/lifecycle.ts +++ b/tools/pack/src/win/lifecycle.ts @@ -491,7 +491,7 @@ async function requestDesktopEval( return await requestJsonIpc( ipc, { input: { expression }, type: SIDECAR_MESSAGES.EVAL }, - { timeoutMs: 5000 }, + { timeoutMs: 60_000 }, ); } catch (error) { return { From e95a5d56cf0c283d7e306dca195b159c24914e32 Mon Sep 17 00:00:00 2001 From: Cheems <94773058+itscheems@users.noreply.github.com> Date: Fri, 31 Jul 2026 14:57:04 +0800 Subject: [PATCH 3/4] fix(daemon): bound and recover the Windows DPAPI worker lifecycle Bound startup, handshake, operation, and shutdown phases, recycle failed worker generations without replaying uncertain mutations, and close owned PowerShell processes during daemon shutdown. Add secret-safe diagnostics, protocol and input limits, plus regression coverage for crash recovery, parent-death cleanup, and the 60-second packaged eval contract. Validated with focused BYOK lifecycle tests, daemon and tools-pack typechecks, pnpm guard, and the workspace typecheck. --- apps/daemon/src/byok/credential-service.ts | 373 ++++++++++++++- apps/daemon/src/byok/windows-dpapi-worker.ts | 447 +++++++++++++++--- apps/daemon/src/server.ts | 33 +- .../tests/byok/credential-service.test.ts | 61 +++ .../byok/credential-service.windows.test.ts | 70 +-- .../daemon/tests/byok/server-shutdown.test.ts | 113 +++++ .../windows-dpapi-backend-lifecycle.test.ts | 340 +++++++++++++ ...indows-dpapi-worker-orphan.windows.test.ts | 140 ++++++ .../tests/byok/windows-dpapi-worker.test.ts | 414 ++++++++++++++++ tools/pack/src/win/lifecycle.ts | 3 +- .../pack/tests/win-lifecycle-timeouts.test.ts | 9 + 11 files changed, 1885 insertions(+), 118 deletions(-) create mode 100644 apps/daemon/tests/byok/server-shutdown.test.ts create mode 100644 apps/daemon/tests/byok/windows-dpapi-backend-lifecycle.test.ts create mode 100644 apps/daemon/tests/byok/windows-dpapi-worker-orphan.windows.test.ts create mode 100644 apps/daemon/tests/byok/windows-dpapi-worker.test.ts create mode 100644 tools/pack/tests/win-lifecycle-timeouts.test.ts diff --git a/apps/daemon/src/byok/credential-service.ts b/apps/daemon/src/byok/credential-service.ts index 4a7ee8d1b45..096e278a35b 100644 --- a/apps/daemon/src/byok/credential-service.ts +++ b/apps/daemon/src/byok/credential-service.ts @@ -11,11 +11,17 @@ import { isNodePtyUnavailableError, loadNodePty, } from '../services/node-pty.js'; -import { WindowsDpapiWorker } from './windows-dpapi-worker.js'; +import { + WindowsDpapiWorker, + WindowsDpapiWorkerError, + type WindowsDpapiWorkerDiagnostic, + type WindowsDpapiWorkerPhase, +} from './windows-dpapi-worker.js'; const PROFILE_ID_PATTERN = /^byok-[a-z0-9][a-z0-9._-]{2,95}$/u; const KEYCHAIN_SERVICE = 'dev.opendesign.byok'; const MAX_SECRET_OUTPUT_BYTES = 64 * 1024; +const MAX_BYOK_API_KEY_BYTES = 32 * 1024; const INTERACTIVE_SECRET_TIMEOUT_MS = 10_000; type StoredProfile = Omit; @@ -27,6 +33,7 @@ type StoredDocument = { export interface ByokSecretBackend { readonly kind: string; available(): Promise; + close?(): Promise; set(profileId: string, secret: string): Promise; get(profileId: string): Promise; delete(profileId: string): Promise; @@ -51,6 +58,8 @@ export class ByokCredentialService { readonly backend: ByokSecretBackend; readonly metadataPath: string; private mutationQueue: Promise = Promise.resolve(); + private closePromise: Promise | null = null; + private closed = false; private readonly persistMetadata: NonNullable< ByokCredentialServiceOptions['persistMetadata'] >; @@ -65,6 +74,7 @@ export class ByokCredentialService { } async status(): Promise<{ available: boolean; backend: string }> { + this.assertOpen(); return { available: await this.backend.available(), backend: this.backend.kind, @@ -72,11 +82,13 @@ export class ByokCredentialService { } async list(): Promise { + this.assertOpen(); const document = await this.readDocument(); return Promise.all(document.profiles.map((profile) => this.toPublicProfile(profile))); } async get(profileId: string): Promise { + this.assertOpen(); assertProfileId(profileId); const stored = (await this.readDocument()).profiles.find((profile) => profile.id === profileId); return stored ? this.toPublicProfile(stored) : null; @@ -88,22 +100,28 @@ export class ByokCredentialService { * actually being started. */ async has(profileId: string): Promise { + this.assertOpen(); assertProfileId(profileId); return (await this.readDocument()).profiles.some((profile) => profile.id === profileId); } async upsert(input: UpsertByokCredentialProfileRequest): Promise { + this.assertOpen(); return this.serializeMutation(() => this.upsertUnlocked(input)); } private async upsertUnlocked( input: UpsertByokCredentialProfileRequest, ): Promise { + const normalized = normalizeProfileInput(input); + const apiKey = typeof input.apiKey === 'string' ? input.apiKey.trim() : ''; + if (Buffer.byteLength(apiKey, 'utf8') > MAX_BYOK_API_KEY_BYTES) { + throw new Error(`apiKey must be at most ${MAX_BYOK_API_KEY_BYTES} UTF-8 bytes.`); + } const available = await this.backend.available(); if (!available) { throw new Error('Secure credential storage is unavailable on this system.'); } - const normalized = normalizeProfileInput(input); const document = await this.readDocument(); const existingIndex = input.id ? document.profiles.findIndex((profile) => profile.id === input.id) @@ -112,7 +130,6 @@ export class ByokCredentialService { const id = input.id ?? createProfileId(); assertProfileId(id); const existing = existingIndex >= 0 ? document.profiles[existingIndex] : undefined; - const apiKey = typeof input.apiKey === 'string' ? input.apiKey.trim() : ''; if (normalized.requiresApiKey && !apiKey && !existing) { throw new Error('An API key is required when creating this BYOK profile.'); } @@ -146,6 +163,7 @@ export class ByokCredentialService { } async resolve(profileId: string): Promise { + this.assertOpen(); assertProfileId(profileId); const stored = (await this.readDocument()).profiles.find((profile) => profile.id === profileId); if (!stored) return null; @@ -167,9 +185,21 @@ export class ByokCredentialService { } async delete(profileId: string): Promise { + this.assertOpen(); return this.serializeMutation(() => this.deleteUnlocked(profileId)); } + async close(): Promise { + this.closePromise ??= this.closeUnlocked(); + return this.closePromise; + } + + private async closeUnlocked(): Promise { + this.closed = true; + await this.mutationQueue; + await this.backend.close?.(); + } + private async deleteUnlocked(profileId: string): Promise { assertProfileId(profileId); const document = await this.readDocument(); @@ -204,6 +234,12 @@ export class ByokCredentialService { return result; } + private assertOpen(): void { + if (this.closed) { + throw new Error('Secure credential service is closed.'); + } + } + private async toPublicProfile( profile: StoredProfile, knownSecret?: string, @@ -436,26 +472,92 @@ class LinuxSecretServiceBackend implements ByokSecretBackend { } } -type WindowsDpapiWorkerClient = Pick; +type WindowsDpapiWorkerClient = Pick; + +type WindowsDpapiWorkerTimeouts = { + connectMs: number; + startupMs: number; + readyMs: number; + operationMs: number; + shutdownMs: number; +}; + +type WindowsDpapiWorkerSlot = { + abortController: AbortController; + closePromise: Promise | null; + generation: number; + promise: Promise; + readyPromise: Promise | null; +}; + +const DEFAULT_WINDOWS_DPAPI_WORKER_TIMEOUTS: WindowsDpapiWorkerTimeouts = { + connectMs: 50_000, + startupMs: 55_000, + readyMs: 3_000, + operationMs: 5_000, + shutdownMs: 5_000, +}; export type WindowsDpapiBackendOptions = { commandAvailable?: (command: string) => Promise; - createWorker?: () => Promise; + createWorker?: (options?: { signal: AbortSignal }) => Promise; + onDiagnostic?: (diagnostic: WindowsDpapiWorkerDiagnostic) => void; + timeouts?: Partial; }; export class WindowsDpapiBackend implements ByokSecretBackend { readonly kind = 'windows-dpapi'; private availability: Promise | null = null; - private worker: Promise | null = null; + private worker: WindowsDpapiWorkerSlot | null = null; + private workerGeneration = 0; + private closePromise: Promise | null = null; + private closed = false; + private readonly timeouts: WindowsDpapiWorkerTimeouts; constructor( private readonly secretsDir: string, private readonly options: WindowsDpapiBackendOptions = {}, - ) {} + ) { + this.timeouts = { + connectMs: positiveWindowsDpapiTimeout( + options.timeouts?.connectMs, + DEFAULT_WINDOWS_DPAPI_WORKER_TIMEOUTS.connectMs, + ), + startupMs: positiveWindowsDpapiTimeout( + options.timeouts?.startupMs, + DEFAULT_WINDOWS_DPAPI_WORKER_TIMEOUTS.startupMs, + ), + readyMs: positiveWindowsDpapiTimeout( + options.timeouts?.readyMs, + DEFAULT_WINDOWS_DPAPI_WORKER_TIMEOUTS.readyMs, + ), + operationMs: positiveWindowsDpapiTimeout( + options.timeouts?.operationMs, + DEFAULT_WINDOWS_DPAPI_WORKER_TIMEOUTS.operationMs, + ), + shutdownMs: positiveWindowsDpapiTimeout( + options.timeouts?.shutdownMs, + DEFAULT_WINDOWS_DPAPI_WORKER_TIMEOUTS.shutdownMs, + ), + }; + } async available() { + if (this.closed) return false; this.availability ??= this.probeAvailability(); - return this.availability; + const attempt = this.availability; + try { + const available = await attempt; + if (!available && this.availability === attempt) { + this.availability = null; + } + return available; + } catch { + if (this.availability === attempt) { + this.availability = null; + } + return false; + } } private async probeAvailability() { @@ -463,7 +565,7 @@ export class WindowsDpapiBackend implements ByokSecretBackend { return false; } try { - await (await this.getWorker()).ready(); + await this.getReadyWorker(); return true; } catch { return false; @@ -472,7 +574,7 @@ export class WindowsDpapiBackend implements ByokSecretBackend { async set(profileId: string, secret: string) { assertProfileId(profileId); - await (await this.getWorker()).run( + await this.runWorker( 'set', path.join(this.secretsDir, `${profileId}.bin`), secret, @@ -481,7 +583,7 @@ export class WindowsDpapiBackend implements ByokSecretBackend { async get(profileId: string) { assertProfileId(profileId); - const result = await (await this.getWorker()).run( + const result = await this.runWorker( 'get', path.join(this.secretsDir, `${profileId}.bin`), ); @@ -490,16 +592,257 @@ export class WindowsDpapiBackend implements ByokSecretBackend { async delete(profileId: string) { assertProfileId(profileId); - return (await (await this.getWorker()).run( + return (await this.runWorker( 'delete', path.join(this.secretsDir, `${profileId}.bin`), )).found; } - private getWorker(): Promise { - this.worker ??= this.options.createWorker?.() ?? WindowsDpapiWorker.create(); - return this.worker; + async close(): Promise { + this.closePromise ??= this.closeUnlocked(); + return this.closePromise; + } + + private async closeUnlocked(): Promise { + this.closed = true; + this.availability = null; + const slot = this.worker; + this.worker = null; + if (slot) await this.disposeWorker(slot); + } + + private async runWorker( + operation: 'set' | 'get' | 'delete', + secretPath: string, + secret?: string, + ) { + const { slot, worker } = await this.getReadyWorker(); + const startedAt = Date.now(); + try { + return await withWindowsDpapiTimeout( + worker.run(operation, secretPath, secret), + this.timeouts.operationMs, + 'operation', + ); + } catch (error) { + throw await this.handleWorkerFailure( + slot, + error, + 'operation', + startedAt, + worker, + ); + } + } + + private async getReadyWorker(): Promise<{ + slot: WindowsDpapiWorkerSlot; + worker: WindowsDpapiWorkerClient; + }> { + const slot = this.getWorkerSlot(); + const startupStartedAt = Date.now(); + let worker: WindowsDpapiWorkerClient; + try { + worker = await withWindowsDpapiTimeout( + slot.promise, + this.timeouts.startupMs, + 'spawn', + ); + } catch (error) { + throw await this.handleWorkerFailure(slot, error, 'spawn', startupStartedAt); + } + const readyStartedAt = Date.now(); + try { + slot.readyPromise ??= worker.ready(); + await withWindowsDpapiTimeout( + slot.readyPromise, + this.timeouts.readyMs, + 'ready', + ); + } catch (error) { + throw await this.handleWorkerFailure( + slot, + error, + 'ready', + readyStartedAt, + worker, + ); + } + return { slot, worker }; } + + private getWorkerSlot(): WindowsDpapiWorkerSlot { + if (this.closed) { + throw new WindowsDpapiWorkerError('operation', 'closed', false); + } + if (this.worker) return this.worker; + const generation = ++this.workerGeneration; + const abortController = new AbortController(); + const promise = Promise.resolve().then( + () => this.options.createWorker?.({ + signal: abortController.signal, + }) ?? WindowsDpapiWorker.create({ + connectTimeoutMs: this.timeouts.connectMs, + signal: abortController.signal, + shutdownTimeoutMs: this.timeouts.shutdownMs, + }), + ); + const slot: WindowsDpapiWorkerSlot = { + abortController, + closePromise: null, + generation, + promise, + readyPromise: null, + }; + this.worker = slot; + return slot; + } + + private async handleWorkerFailure( + slot: WindowsDpapiWorkerSlot, + error: unknown, + fallbackPhase: WindowsDpapiWorkerPhase, + startedAt: number, + worker?: WindowsDpapiWorkerClient, + ): Promise { + const safeError = normalizeWindowsDpapiError(error, fallbackPhase); + if (this.closed && safeError.failureClass === 'closed') { + return safeError; + } + this.reportDiagnostic({ + phase: safeError.phase, + failureClass: safeError.failureClass, + durationMs: Math.max(0, Date.now() - startedAt), + workerGeneration: slot.generation, + }); + if (safeError.fatal) { + if (this.worker === slot) { + this.worker = null; + this.availability = null; + } + try { + await this.disposeWorker(slot, worker); + } catch { + // Preserve the operation failure; disposeWorker already emitted the + // separate safe shutdown diagnostic. + } + } + return safeError; + } + + private async disposeWorker( + slot: WindowsDpapiWorkerSlot, + knownWorker?: WindowsDpapiWorkerClient, + ): Promise { + slot.closePromise ??= this.disposeWorkerUnlocked(slot, knownWorker); + await slot.closePromise; + } + + private async disposeWorkerUnlocked( + slot: WindowsDpapiWorkerSlot, + knownWorker?: WindowsDpapiWorkerClient, + ): Promise { + slot.abortController.abort(); + let worker = knownWorker; + if (!worker) { + try { + worker = await withWindowsDpapiTimeout( + slot.promise, + this.timeouts.shutdownMs, + 'shutdown', + ); + } catch (error) { + const safeError = normalizeWindowsDpapiError(error, 'shutdown'); + if (safeError.failureClass === 'closed') return; + void slot.promise.then( + (lateWorker) => withWindowsDpapiTimeout( + lateWorker.close(), + this.timeouts.shutdownMs, + 'shutdown', + ).catch(() => undefined), + () => undefined, + ); + if (safeError.failureClass === 'timeout') { + this.reportDiagnostic({ + phase: 'shutdown', + failureClass: 'timeout', + durationMs: this.timeouts.shutdownMs, + workerGeneration: slot.generation, + }); + throw safeError; + } + return; + } + } + const startedAt = Date.now(); + try { + await withWindowsDpapiTimeout( + worker.close(), + this.timeouts.shutdownMs, + 'shutdown', + ); + } catch (error) { + const safeError = normalizeWindowsDpapiError(error, 'shutdown'); + this.reportDiagnostic({ + phase: safeError.phase, + failureClass: safeError.failureClass, + durationMs: Math.max(0, Date.now() - startedAt), + workerGeneration: slot.generation, + }); + throw safeError; + } + } + + private reportDiagnostic(diagnostic: WindowsDpapiWorkerDiagnostic): void { + try { + if (this.options.onDiagnostic) { + this.options.onDiagnostic(diagnostic); + } else { + console.warn('[byok] Windows DPAPI worker failure', diagnostic); + } + } catch { + // Observability must never change credential storage behavior. + } + } +} + +function withWindowsDpapiTimeout( + promise: Promise, + timeoutMs: number, + phase: WindowsDpapiWorkerPhase, +): Promise { + return new Promise((resolve, reject) => { + const timer = setTimeout(() => { + reject(new WindowsDpapiWorkerError(phase, 'timeout')); + }, timeoutMs); + timer.unref?.(); + promise.then( + (value) => { + clearTimeout(timer); + resolve(value); + }, + (error) => { + clearTimeout(timer); + reject(error); + }, + ); + }); +} + +function positiveWindowsDpapiTimeout( + value: number | undefined, + fallback: number, +): number { + return Number.isFinite(value) && Number(value) > 0 ? Number(value) : fallback; +} + +function normalizeWindowsDpapiError( + error: unknown, + phase: WindowsDpapiWorkerPhase, +): WindowsDpapiWorkerError { + return error instanceof WindowsDpapiWorkerError + ? error + : new WindowsDpapiWorkerError(phase, 'unknown'); } class UnavailableSecretBackend implements ByokSecretBackend { diff --git a/apps/daemon/src/byok/windows-dpapi-worker.ts b/apps/daemon/src/byok/windows-dpapi-worker.ts index 983b9f4d2e6..1a348a55d9f 100644 --- a/apps/daemon/src/byok/windows-dpapi-worker.ts +++ b/apps/daemon/src/byok/windows-dpapi-worker.ts @@ -5,6 +5,8 @@ import type { Server, Socket } from 'node:net'; import { createServer } from 'node:net'; const MAX_WORKER_LINE_BYTES = 64 * 1024; +const DEFAULT_CONNECT_TIMEOUT_MS = 50_000; +const DEFAULT_SHUTDOWN_TIMEOUT_MS = 5_000; // Keep one PowerShell process alive for the daemon lifetime. Packaged Windows // can spend tens of seconds scanning each new encoded PowerShell command, so a @@ -17,6 +19,43 @@ export type WindowsDpapiWorkerResult = { value: string | null; }; +export type WindowsDpapiWorkerPhase = + | 'spawn' + | 'connect' + | 'ready' + | 'operation' + | 'shutdown'; + +export type WindowsDpapiWorkerFailureClass = + | 'timeout' + | 'spawn' + | 'child-exit' + | 'pipe-close' + | 'protocol' + | 'input' + | 'dpapi' + | 'closed' + | 'unknown'; + +export type WindowsDpapiWorkerDiagnostic = { + phase: WindowsDpapiWorkerPhase; + failureClass: WindowsDpapiWorkerFailureClass; + durationMs: number; + workerGeneration: number; +}; + +export class WindowsDpapiWorkerError extends Error { + readonly name = 'WindowsDpapiWorkerError'; + + constructor( + readonly phase: WindowsDpapiWorkerPhase, + readonly failureClass: WindowsDpapiWorkerFailureClass, + readonly fatal = true, + ) { + super('Secure credential backend command failed.'); + } +} + type WindowsDpapiWorkerRequest = { id: string; operation: WindowsDpapiWorkerOperation; @@ -40,6 +79,23 @@ type SpawnWindowsDpapiWorker = ( }, ) => ChildProcessWithoutNullStreams; +type WindowsDpapiPipeAddress = { + connectionName: string; + listenPath: string; +}; + +export type WindowsDpapiWorkerCreateOptions = { + connectTimeoutMs?: number; + pipeAddress?: WindowsDpapiPipeAddress; + signal?: AbortSignal; + shutdownTimeoutMs?: number; + spawnWorker?: SpawnWindowsDpapiWorker; +}; + +type WindowsDpapiWorkerTransportOptions = { + shutdownTimeoutMs?: number; +}; + const WINDOWS_DPAPI_WORKER_SCRIPT = ` $ErrorActionPreference = 'Stop' function Write-OpenDesignDpapiResponse([hashtable]$response) { @@ -53,6 +109,9 @@ $pipe = New-Object System.IO.Pipes.NamedPipeClientStream( [System.IO.Pipes.PipeOptions]::None ) $pipe.Connect() +$parentProcess = [System.Diagnostics.Process]::GetProcessById( + [int]$env:OD_BYOK_DPAPI_PARENT_PID +) $reader = New-Object System.IO.StreamReader( $pipe, [System.Text.Encoding]::UTF8, @@ -95,7 +154,20 @@ try { exit 1 } -while (($line = $reader.ReadLine()) -ne $null) { +while ($true) { + $readTask = $reader.ReadLineAsync() + while (-not $readTask.Wait(500)) { + if ($parentProcess.HasExited) { + exit 0 + } + } + $line = $readTask.Result + if ($line -eq $null) { + break + } + if ($parentProcess.HasExited) { + exit 0 + } $requestId = '' try { $request = $line | ConvertFrom-Json @@ -177,84 +249,169 @@ const WINDOWS_DPAPI_WORKER_ENCODED_SCRIPT = Buffer.from( export class WindowsDpapiWorker { private readonly readyPromise: Promise; + private readonly childExitPromise: Promise; private resolveReady: (() => void) | null = null; private rejectReady: ((error: Error) => void) | null = null; private inputBuffer = ''; private nextRequestId = 0; private operationQueue: Promise = Promise.resolve(); + private closePromise: Promise | null = null; private pending: { id: string; reject(error: Error): void; resolve(response: WindowsDpapiWorkerResponse): void; } | null = null; private closed = false; + private closing = false; + private childExited = false; + private readonly shutdownTimeoutMs: number; private constructor( private readonly child: ChildProcessWithoutNullStreams, private readonly pipe: Socket, + options: WindowsDpapiWorkerTransportOptions = {}, ) { + this.shutdownTimeoutMs = positiveTimeout( + options.shutdownTimeoutMs, + DEFAULT_SHUTDOWN_TIMEOUT_MS, + ); this.readyPromise = new Promise((resolve, reject) => { this.resolveReady = resolve; this.rejectReady = reject; }); + void this.readyPromise.catch(() => undefined); + this.childExitPromise = new Promise((resolve) => { + const markExited = () => { + this.childExited = true; + resolve(); + }; + if (this.child.exitCode !== null || this.child.signalCode !== null) { + markExited(); + } else { + this.child.once('close', markExited); + } + }); this.pipe.on('data', (chunk: Buffer) => this.acceptInput(chunk)); - this.pipe.on('error', () => this.fail()); - this.pipe.on('close', () => this.fail()); + this.pipe.on('error', () => { + if (!this.closing) this.fail('pipe-close'); + }); + this.pipe.on('close', () => { + if (!this.closing) this.fail('pipe-close'); + }); this.child.stdout.resume(); this.child.stderr.resume(); - this.child.on('error', () => this.fail()); - this.child.on('close', () => this.fail()); + this.child.on('error', () => { + if (!this.closing) this.fail('child-exit'); + }); + this.child.on('close', () => { + if (!this.closing) this.fail('child-exit'); + }); } static async create( - spawnWorker: SpawnWindowsDpapiWorker = spawn, + options: WindowsDpapiWorkerCreateOptions = {}, ): Promise { - const pipeName = `open-design-dpapi-${process.pid}-${randomUUID()}`; + if (options.signal?.aborted) { + throw secureCredentialCommandError('shutdown', 'closed', false); + } + const pipeAddress = options.pipeAddress ?? createWindowsPipeAddress(); const server = createServer(); - await listen(server, `\\\\.\\pipe\\${pipeName}`); - const connection = waitForConnection(server); - const child = spawnWorker( - 'powershell.exe', - [ - '-NoLogo', - '-NoProfile', - '-NonInteractive', - '-ExecutionPolicy', - 'Bypass', - '-EncodedCommand', - WINDOWS_DPAPI_WORKER_ENCODED_SCRIPT, - ], - { - env: { - ...process.env, - OD_BYOK_DPAPI_PIPE_NAME: pipeName, + try { + await listen(server, pipeAddress.listenPath); + } catch { + await closeServer(server); + throw secureCredentialCommandError('spawn', 'spawn'); + } + if (options.signal?.aborted) { + await closeServer(server); + throw secureCredentialCommandError('shutdown', 'closed', false); + } + let child: ChildProcessWithoutNullStreams; + try { + child = (options.spawnWorker ?? spawn)( + 'powershell.exe', + [ + '-NoLogo', + '-NoProfile', + '-NonInteractive', + '-ExecutionPolicy', + 'Bypass', + '-EncodedCommand', + WINDOWS_DPAPI_WORKER_ENCODED_SCRIPT, + ], + { + env: { + ...process.env, + OD_BYOK_DPAPI_PARENT_PID: String(process.pid), + OD_BYOK_DPAPI_PIPE_NAME: pipeAddress.connectionName, + }, + stdio: ['pipe', 'pipe', 'pipe'], + windowsHide: true, }, - stdio: ['pipe', 'pipe', 'pipe'], - windowsHide: true, - }, - ); + ); + } catch { + await closeServer(server); + throw secureCredentialCommandError('spawn', 'spawn'); + } // The packaged process must not retain an open stdin stream. Requests use // the random per-process local pipe below, keeping plaintext credentials // out of argv, environment variables, and temporary files. child.stdin.end(); + const connection = waitForConnection(server); try { - const pipe = await Promise.race([ - connection, - childFailure(child), - ]); + const pipe = await withTimeout( + Promise.race([ + connection, + childFailure(child), + abortFailure(options.signal), + ]), + positiveTimeout(options.connectTimeoutMs, DEFAULT_CONNECT_TIMEOUT_MS), + 'connect', + ); server.close(); - return new WindowsDpapiWorker(child, pipe); + return new WindowsDpapiWorker(child, pipe, options); } catch (error) { - server.close(); - if (!child.killed) child.kill(); - throw error; + void connection.then( + (latePipe) => latePipe.destroy(), + () => undefined, + ); + if ( + !child.killed + && child.exitCode === null + && child.signalCode === null + ) { + try { + child.kill(); + } catch { + // The safe error below is the only detail that leaves this boundary. + } + } + await closeServer(server); + throw normalizeWorkerError(error, 'connect'); } } + /** + * Build a worker from an already-connected transport. The production create + * path and protocol tests share this boundary so lifecycle behavior is not + * reimplemented in a test-only fake. + */ + static fromConnectedTransport( + child: ChildProcessWithoutNullStreams, + pipe: Socket, + options: WindowsDpapiWorkerTransportOptions = {}, + ): WindowsDpapiWorker { + return new WindowsDpapiWorker(child, pipe, options); + } + ready(): Promise { return this.readyPromise; } + get processId(): number | undefined { + return this.child.pid; + } + run( operation: WindowsDpapiWorkerOperation, secretPath: string, @@ -277,7 +434,9 @@ export class WindowsDpapiWorker { secret?: string, ): Promise { await this.readyPromise; - if (this.closed || this.pending) throw secureCredentialCommandError(); + if (this.closed || this.closing || this.pending) { + throw secureCredentialCommandError('operation', 'closed'); + } const id = String(++this.nextRequestId); const request: WindowsDpapiWorkerRequest = { id, @@ -287,19 +446,33 @@ export class WindowsDpapiWorker { ? {} : { secret: Buffer.from(secret, 'utf8').toString('base64') }), }; + const encodedRequest = `${JSON.stringify(request)}\n`; + if (Buffer.byteLength(encodedRequest, 'utf8') > MAX_WORKER_LINE_BYTES) { + throw secureCredentialCommandError('operation', 'input', false); + } const response = await new Promise((resolve, reject) => { this.pending = { id, reject, resolve }; - this.pipe.write(`${JSON.stringify(request)}\n`, 'utf8', (error) => { - if (error) this.fail(); + this.pipe.write(encodedRequest, 'utf8', (error) => { + if (error) this.fail('pipe-close'); }); }); - if (response.ok !== true || response.id !== id) { - throw secureCredentialCommandError(); + if (response.id !== id) { + this.fail('protocol'); + throw secureCredentialCommandError('operation', 'protocol'); + } + if (response.ok !== true) { + throw secureCredentialCommandError('operation', 'dpapi', false); } if (operation === 'get' && response.found === true) { - if (typeof response.value !== 'string') throw secureCredentialCommandError(); - const value = Buffer.from(response.value, 'base64'); - if (value.byteLength > MAX_WORKER_LINE_BYTES) throw secureCredentialCommandError(); + if (typeof response.value !== 'string') { + this.fail('protocol'); + throw secureCredentialCommandError('operation', 'protocol'); + } + const value = decodeCanonicalBase64(response.value); + if (value === null || value.byteLength > MAX_WORKER_LINE_BYTES) { + this.fail('protocol'); + throw secureCredentialCommandError('operation', 'protocol'); + } return { found: true, value: value.toString('utf8') }; } return { @@ -312,7 +485,7 @@ export class WindowsDpapiWorker { if (this.closed) return; this.inputBuffer += chunk.toString('utf8'); if (Buffer.byteLength(this.inputBuffer, 'utf8') > MAX_WORKER_LINE_BYTES) { - this.fail(); + this.fail('protocol'); return; } let newlineIndex = this.inputBuffer.indexOf('\n'); @@ -330,7 +503,7 @@ export class WindowsDpapiWorker { try { response = JSON.parse(line) as WindowsDpapiWorkerResponse; } catch { - this.fail(); + this.fail('protocol'); return; } if (response.type === 'ready') { @@ -340,11 +513,11 @@ export class WindowsDpapiWorker { this.rejectReady = null; return; } - this.fail(); + this.fail(response.ok === false ? 'dpapi' : 'protocol'); return; } if (!this.pending || response.id !== this.pending.id) { - this.fail(); + this.fail('protocol'); return; } const pending = this.pending; @@ -352,17 +525,80 @@ export class WindowsDpapiWorker { pending.resolve(response); } - private fail(): void { + async close(): Promise { + this.closePromise ??= this.closeUnlocked(); + return this.closePromise; + } + + private async closeUnlocked(): Promise { + if (this.closed && this.childExited) return; + this.closing = true; + this.closed = true; + const closedError = secureCredentialCommandError('shutdown', 'closed'); + this.rejectReady?.(closedError); + this.resolveReady = null; + this.rejectReady = null; + this.pending?.reject(closedError); + this.pending = null; + if (!this.pipe.destroyed) { + this.pipe.end(); + } + const shutdownMarginMs = Math.min( + 250, + Math.max(1, Math.floor(this.shutdownTimeoutMs / 10)), + ); + const ownedShutdownBudgetMs = Math.max(0, this.shutdownTimeoutMs - shutdownMarginMs); + const gracefulShutdownBudgetMs = Math.floor(ownedShutdownBudgetMs / 2); + const forcedShutdownBudgetMs = ownedShutdownBudgetMs - gracefulShutdownBudgetMs; + if (await settlesBefore(this.childExitPromise, gracefulShutdownBudgetMs)) { + this.pipe.destroy(); + this.destroyStreams(); + return; + } + if (!this.childExited && !this.child.killed) { + try { + this.child.kill(); + } catch { + // Report a safe shutdown timeout after the bounded final wait. + } + } + if (!(await settlesBefore(this.childExitPromise, forcedShutdownBudgetMs))) { + this.pipe.destroy(); + this.destroyStreams(); + throw secureCredentialCommandError('shutdown', 'timeout'); + } + this.pipe.destroy(); + this.destroyStreams(); + } + + private fail(failureClass: WindowsDpapiWorkerFailureClass): void { if (this.closed) return; this.closed = true; - const error = secureCredentialCommandError(); + const phase = this.resolveReady ? 'ready' : 'operation'; + const error = secureCredentialCommandError(phase, failureClass); this.rejectReady?.(error); this.resolveReady = null; this.rejectReady = null; this.pending?.reject(error); this.pending = null; this.pipe.destroy(); - if (!this.child.killed) this.child.kill(); + if (!this.childExited && !this.child.killed) { + try { + this.child.kill(); + } catch { + // The worker is already terminal; preserve only the safe failure. + } + } + } + + private destroyStreams(): void { + for (const stream of [this.child.stdin, this.child.stdout, this.child.stderr]) { + try { + stream.destroy(); + } catch { + // Best-effort file-descriptor cleanup after the child is terminal. + } + } } } @@ -386,11 +622,112 @@ function waitForConnection(server: Server): Promise { function childFailure(child: ChildProcessWithoutNullStreams): Promise { return new Promise((_, reject) => { - child.once('error', () => reject(secureCredentialCommandError())); - child.once('close', () => reject(secureCredentialCommandError())); + child.once('error', () => reject(secureCredentialCommandError('connect', 'spawn'))); + child.once('close', () => reject(secureCredentialCommandError('connect', 'child-exit'))); + }); +} + +function abortFailure(signal: AbortSignal | undefined): Promise { + return new Promise((_, reject) => { + if (!signal) return; + const rejectAborted = () => { + reject(secureCredentialCommandError('shutdown', 'closed', false)); + }; + if (signal.aborted) { + rejectAborted(); + return; + } + signal.addEventListener('abort', rejectAborted, { once: true }); + }); +} + +function createWindowsPipeAddress(): WindowsDpapiPipeAddress { + const connectionName = `open-design-dpapi-${process.pid}-${randomUUID()}`; + return { + connectionName, + listenPath: `\\\\.\\pipe\\${connectionName}`, + }; +} + +function closeServer(server: Server): Promise { + return new Promise((resolve) => { + if (!server.listening) { + resolve(); + return; + } + server.close(() => resolve()); + }); +} + +function positiveTimeout(input: number | undefined, fallback: number): number { + return Number.isFinite(input) && Number(input) > 0 ? Number(input) : fallback; +} + +function withTimeout( + promise: Promise, + timeoutMs: number, + phase: WindowsDpapiWorkerPhase, +): Promise { + return new Promise((resolve, reject) => { + const timer = setTimeout(() => { + reject(secureCredentialCommandError(phase, 'timeout')); + }, timeoutMs); + timer.unref?.(); + promise.then( + (value) => { + clearTimeout(timer); + resolve(value); + }, + (error) => { + clearTimeout(timer); + reject(normalizeWorkerError(error, phase)); + }, + ); }); } -function secureCredentialCommandError(): Error { - return new Error('Secure credential backend command failed.'); +function settlesBefore(promise: Promise, timeoutMs: number): Promise { + return new Promise((resolve) => { + const timer = setTimeout(() => resolve(false), timeoutMs); + timer.unref?.(); + promise.then( + () => { + clearTimeout(timer); + resolve(true); + }, + () => { + clearTimeout(timer); + resolve(true); + }, + ); + }); +} + +function decodeCanonicalBase64(value: string): Buffer | null { + if (!/^(?:[A-Za-z0-9+/]{4})*(?:[A-Za-z0-9+/]{2}==|[A-Za-z0-9+/]{3}=)?$/u.test(value)) { + return null; + } + try { + const decoded = Buffer.from(value, 'base64'); + return decoded.toString('base64') === value ? decoded : null; + } catch { + return null; + } +} + +function normalizeWorkerError( + error: unknown, + phase: WindowsDpapiWorkerPhase, +): WindowsDpapiWorkerError { + return error instanceof WindowsDpapiWorkerError + ? error + : secureCredentialCommandError(phase, 'unknown'); +} + +function secureCredentialCommandError( + phase: WindowsDpapiWorkerPhase, + failureClass: WindowsDpapiWorkerFailureClass, + fatal = true, +): WindowsDpapiWorkerError { + return new WindowsDpapiWorkerError(phase, failureClass, fatal); } diff --git a/apps/daemon/src/server.ts b/apps/daemon/src/server.ts index 7f81705d818..301e7be3922 100644 --- a/apps/daemon/src/server.ts +++ b/apps/daemon/src/server.ts @@ -859,9 +859,6 @@ const SANDBOX_MODE_ENABLED = isSandboxModeEnabled(process.env); const RUNTIME_DATA_DIR = resolveDataDir(process.env.OD_DATA_DIR, PROJECT_ROOT, { requireExplicit: SANDBOX_MODE_ENABLED, }); -const defaultByokCredentialService = new ByokCredentialService({ - dataDir: RUNTIME_DATA_DIR, -}); const SANDBOX_RUNTIME = resolveSandboxRuntimeConfig(SANDBOX_MODE_ENABLED, RUNTIME_DATA_DIR); ensureSandboxRuntimeDirs(SANDBOX_RUNTIME); const PLUGIN_LOCKFILE_PATH = path.join(RUNTIME_DATA_DIR, 'od-plugin-lock.json'); @@ -2053,12 +2050,12 @@ export interface StartServerOptions { export interface StartServerResult { url: string; server: import('node:http').Server; - shutdown: () => Promise | void; + shutdown: () => Promise; routeInventory: import('./route-registration-guard.js').RouteRegistration[]; } export async function startServer({ - byokCredentialService = defaultByokCredentialService, + byokCredentialService = new ByokCredentialService({ dataDir: RUNTIME_DATA_DIR }), port = 7456, host = normalizeDaemonBindHost(process.env.OD_BIND_HOST), returnServer = false, @@ -9110,19 +9107,23 @@ export async function startServer({ // - `apps/daemon/sidecar/server.ts` → expects `{ url, server }` // - `apps/daemon/tests/version-route.test.ts` → expects `{ url, server }` return await new Promise((resolve, reject) => { - let daemonShutdownStarted = false; + let daemonShutdownPromise: Promise | null = null; const cleanupDaemonBackgroundWork = () => { composioConnectorProvider.stopCatalogRefreshLoop(); orbitService.stop(); routineService?.stop(); }; - const shutdownDaemonRuns = async () => { - if (daemonShutdownStarted) return; - daemonShutdownStarted = true; - daemonShuttingDown = true; - await design.runs.shutdownActive({ graceMs: resolveChatRunShutdownGraceMs() }); - await terminalService.shutdownActive(); - await design.analytics.shutdown(); + const shutdownDaemonRuns = () => { + daemonShutdownPromise ??= (async () => { + daemonShuttingDown = true; + const byokShutdown = byokCredentialService.close(); + void byokShutdown.catch(() => undefined); + await design.runs.shutdownActive({ graceMs: resolveChatRunShutdownGraceMs() }); + await terminalService.shutdownActive(); + await byokShutdown; + await design.analytics.shutdown(); + })(); + return daemonShutdownPromise; }; let server; try { @@ -9190,7 +9191,11 @@ export async function startServer({ return; } server.once('close', () => { - void shutdownDaemonRuns().finally(cleanupDaemonBackgroundWork); + void shutdownDaemonRuns() + .catch(() => { + console.error('[od] daemon shutdown cleanup failed'); + }) + .finally(cleanupDaemonBackgroundWork); }); // `app.listen` throws synchronously when the port is already in use on // some Node versions, but emits an `error` event on others (and for diff --git a/apps/daemon/tests/byok/credential-service.test.ts b/apps/daemon/tests/byok/credential-service.test.ts index b8a6416be42..8f047d4f5f3 100644 --- a/apps/daemon/tests/byok/credential-service.test.ts +++ b/apps/daemon/tests/byok/credential-service.test.ts @@ -29,6 +29,8 @@ class MemorySecretBackend implements ByokSecretBackend { async delete(profileId: string) { return this.secrets.delete(profileId); } + + async close() {} } describe('BYOK credential service', () => { @@ -89,6 +91,26 @@ describe('BYOK credential service', () => { })).rejects.toThrow(/secure credential storage is unavailable/i); }); + it('rejects oversized UTF-8 API keys before they can overflow the worker protocol', async () => { + const dataDir = await mkdtemp(path.join(tmpdir(), 'od-byok-credentials-')); + roots.push(dataDir); + const backend = new MemorySecretBackend(); + backend.set = vi.fn(backend.set.bind(backend)); + const service = new ByokCredentialService({ dataDir, backend }); + + await expect(service.upsert({ + id: 'byok-oversized-secret', + label: 'Oversized', + protocol: 'openai', + baseUrl: 'https://example.test/v1', + model: 'model', + apiKey: '密'.repeat(16_385), + })).rejects.toThrow('apiKey must be at most 32768 UTF-8 bytes.'); + + expect(backend.set).not.toHaveBeenCalled(); + expect(backend.secrets).toEqual(new Map()); + }); + it('dispatches native Windows credentials to a DPAPI backend rooted in OD_DATA_DIR', async () => { const dataDir = await mkdtemp(path.join(tmpdir(), 'od-byok-windows-dispatch-')); roots.push(dataDir); @@ -100,6 +122,7 @@ describe('BYOK credential service', () => { it('reuses one Windows DPAPI worker across availability and credential operations', async () => { const worker = { + close: vi.fn(async () => undefined), ready: vi.fn(async () => undefined), run: vi.fn(async (operation: 'set' | 'get' | 'delete') => { if (operation === 'get') { @@ -162,6 +185,44 @@ describe('BYOK credential service', () => { ]); }); + it('stops accepting work and waits for an accepted mutation before closing the backend', async () => { + const dataDir = await mkdtemp(path.join(tmpdir(), 'od-byok-credentials-')); + roots.push(dataDir); + const backend = new MemorySecretBackend(); + let releaseSet: (() => void) | undefined; + const setReleased = new Promise((resolve) => { + releaseSet = resolve; + }); + backend.set = vi.fn(async (profileId: string, secret: string) => { + await setReleased; + backend.secrets.set(profileId, secret); + }); + backend.close = vi.fn(async () => undefined); + const service = new ByokCredentialService({ dataDir, backend }); + const upsert = service.upsert({ + id: 'byok-close-boundary', + label: 'Close boundary', + protocol: 'openai', + baseUrl: 'https://example.test/v1', + model: 'model', + apiKey: 'accepted-secret', + }); + await vi.waitFor(() => { + expect(backend.set).toHaveBeenCalledTimes(1); + }); + + const close = service.close(); + await expect(service.status()).rejects.toThrow('Secure credential service is closed.'); + expect(backend.close).not.toHaveBeenCalled(); + + releaseSet?.(); + await expect(upsert).resolves.toMatchObject({ id: 'byok-close-boundary' }); + await expect(close).resolves.toBeUndefined(); + await expect(service.close()).resolves.toBeUndefined(); + + expect(backend.close).toHaveBeenCalledTimes(1); + }); + it('removes a newly created secret when metadata persistence fails', async () => { const dataDir = await mkdtemp(path.join(tmpdir(), 'od-byok-credentials-')); roots.push(dataDir); diff --git a/apps/daemon/tests/byok/credential-service.windows.test.ts b/apps/daemon/tests/byok/credential-service.windows.test.ts index 5d96a1cfa46..f7d35a10e89 100644 --- a/apps/daemon/tests/byok/credential-service.windows.test.ts +++ b/apps/daemon/tests/byok/credential-service.windows.test.ts @@ -25,40 +25,44 @@ describe.runIf(process.platform === 'win32')('Windows DPAPI BYOK credential smok const service = new ByokCredentialService({ dataDir, backend }); const apiKey = 'windows-dpapi-smoke-secret'; - await expect(service.status()).resolves.toEqual({ - available: true, - backend: 'windows-dpapi', - }); - const profile = await service.upsert({ - id: 'byok-windows-dpapi-smoke', - label: 'Windows DPAPI smoke', - protocol: 'openai', - baseUrl: 'https://api.openai.com/v1', - model: 'gpt-5.4', - apiKey, - }); + try { + await expect(service.status()).resolves.toEqual({ + available: true, + backend: 'windows-dpapi', + }); + const profile = await service.upsert({ + id: 'byok-windows-dpapi-smoke', + label: 'Windows DPAPI smoke', + protocol: 'openai', + baseUrl: 'https://api.openai.com/v1', + model: 'gpt-5.4', + apiKey, + }); - expect(profile).toMatchObject({ - id: 'byok-windows-dpapi-smoke', - configured: true, - keyTail: 'cret', - }); - expect(await service.resolve(profile.id)).toMatchObject({ - apiKey, - provider: { apiKey }, - }); - expect( - await readFile( - path.join(dataDir, 'byok', 'profiles.json'), - 'utf8', - ), - ).not.toContain(apiKey); - const encrypted = await readFile( - path.join(dataDir, 'byok', 'secrets', `${profile.id}.bin`), - ); - expect(encrypted.includes(Buffer.from(apiKey, 'utf8'))).toBe(false); + expect(profile).toMatchObject({ + id: 'byok-windows-dpapi-smoke', + configured: true, + keyTail: 'cret', + }); + expect(await service.resolve(profile.id)).toMatchObject({ + apiKey, + provider: { apiKey }, + }); + expect( + await readFile( + path.join(dataDir, 'byok', 'profiles.json'), + 'utf8', + ), + ).not.toContain(apiKey); + const encrypted = await readFile( + path.join(dataDir, 'byok', 'secrets', `${profile.id}.bin`), + ); + expect(encrypted.includes(Buffer.from(apiKey, 'utf8'))).toBe(false); - await expect(service.delete(profile.id)).resolves.toBe(true); - await expect(service.resolve(profile.id)).resolves.toBeNull(); + await expect(service.delete(profile.id)).resolves.toBe(true); + await expect(service.resolve(profile.id)).resolves.toBeNull(); + } finally { + await service.close(); + } }); }); diff --git a/apps/daemon/tests/byok/server-shutdown.test.ts b/apps/daemon/tests/byok/server-shutdown.test.ts new file mode 100644 index 00000000000..25b40335a32 --- /dev/null +++ b/apps/daemon/tests/byok/server-shutdown.test.ts @@ -0,0 +1,113 @@ +import type { Server } from 'node:http'; + +import { afterEach, describe, expect, it, vi } from 'vitest'; + +import { + ByokCredentialService, + type ByokSecretBackend, +} from '../../src/byok/credential-service.js'; +import { + startServer, + type StartServerResult, +} from '../../src/server.js'; + +class ShutdownTestBackend implements ByokSecretBackend { + readonly kind = 'shutdown-test'; + + async available() { + return true; + } + + async close() {} + + async set() {} + + async get() { + return null; + } + + async delete() { + return false; + } +} + +describe('daemon BYOK shutdown ownership', () => { + let server: Server | undefined; + let releaseClose: (() => void) | undefined; + + afterEach(async () => { + releaseClose?.(); + if (server?.listening) { + await new Promise((resolve) => server?.close(() => resolve())); + } + server = undefined; + releaseClose = undefined; + }); + + it('shares one shutdown promise and waits for the credential worker owner to close', async () => { + const service = new ByokCredentialService({ + dataDir: '/test/byok-shutdown', + backend: new ShutdownTestBackend(), + }); + let closeStarted: (() => void) | undefined; + const closeObserved = new Promise((resolve) => { + closeStarted = resolve; + }); + const closeGate = new Promise((resolve) => { + releaseClose = resolve; + }); + service.close = vi.fn(async () => { + closeStarted?.(); + await closeGate; + }); + const started = await startServer({ + byokCredentialService: service, + port: 0, + returnServer: true, + }) as StartServerResult; + server = started.server; + + const firstShutdown = started.shutdown() as Promise; + const secondShutdown = started.shutdown() as Promise; + expect(secondShutdown).toBe(firstShutdown); + await closeObserved; + + let settled = false; + void firstShutdown.then(() => { + settled = true; + }); + await Promise.resolve(); + expect(settled).toBe(false); + + releaseClose?.(); + await expect(Promise.all([firstShutdown, secondShutdown])).resolves.toEqual([ + undefined, + undefined, + ]); + expect(service.close).toHaveBeenCalledTimes(1); + }); + + it('closes the credential worker owner when the HTTP server closes first', async () => { + const service = new ByokCredentialService({ + dataDir: '/test/byok-server-close', + backend: new ShutdownTestBackend(), + }); + service.close = vi.fn(async () => undefined); + const started = await startServer({ + byokCredentialService: service, + port: 0, + returnServer: true, + }) as StartServerResult; + server = started.server; + + await new Promise((resolve, reject) => { + started.server.close((error) => { + if (error) reject(error); + else resolve(); + }); + }); + await vi.waitFor(() => { + expect(service.close).toHaveBeenCalledTimes(1); + }); + }); +}); diff --git a/apps/daemon/tests/byok/windows-dpapi-backend-lifecycle.test.ts b/apps/daemon/tests/byok/windows-dpapi-backend-lifecycle.test.ts new file mode 100644 index 00000000000..22e3cde70b7 --- /dev/null +++ b/apps/daemon/tests/byok/windows-dpapi-backend-lifecycle.test.ts @@ -0,0 +1,340 @@ +import { afterEach, describe, expect, it, vi } from 'vitest'; + +import { + WindowsDpapiBackend, + type WindowsDpapiBackendOptions, +} from '../../src/byok/credential-service.js'; +import type { + WindowsDpapiWorkerDiagnostic, + WindowsDpapiWorkerResult, +} from '../../src/byok/windows-dpapi-worker.js'; +import { WindowsDpapiWorkerError } from '../../src/byok/windows-dpapi-worker.js'; + +type WorkerOperation = 'set' | 'get' | 'delete'; + +type FakeWorker = { + close: ReturnType Promise>>; + ready: ReturnType Promise>>; + run: ReturnType< + typeof vi.fn< + ( + operation: WorkerOperation, + secretPath: string, + secret?: string, + ) => Promise + > + >; +}; + +function createWorker( + run: FakeWorker['run'] = vi.fn(async () => ({ found: true, value: null })), +): FakeWorker { + return { + close: vi.fn(async () => undefined), + ready: vi.fn(async () => undefined), + run, + }; +} + +function backendOptions( + input: WindowsDpapiBackendOptions & { + onDiagnostic?: (diagnostic: WindowsDpapiWorkerDiagnostic) => void; + }, +): WindowsDpapiBackendOptions { + return input as WindowsDpapiBackendOptions; +} + +describe('Windows DPAPI backend worker lifecycle', () => { + afterEach(() => { + vi.useRealTimers(); + }); + + it('does not permanently cache an initial worker creation failure', async () => { + const healthyWorker = createWorker(); + const createWorkerClient = vi.fn() + .mockRejectedValueOnce(new Error('first worker failed')) + .mockResolvedValueOnce(healthyWorker); + const backend = new WindowsDpapiBackend('/test/byok/secrets', { + commandAvailable: async () => true, + createWorker: createWorkerClient, + }); + + await expect(backend.available()).resolves.toBe(false); + await expect(backend.available()).resolves.toBe(true); + + expect(createWorkerClient).toHaveBeenCalledTimes(2); + expect(healthyWorker.ready).toHaveBeenCalledTimes(1); + }); + + it('does not permanently cache a rejected PowerShell availability probe', async () => { + const worker = createWorker(); + const commandAvailable = vi.fn() + .mockRejectedValueOnce(new Error('transient command probe failure')) + .mockResolvedValueOnce(true); + const createWorkerClient = vi.fn(async () => worker); + const backend = new WindowsDpapiBackend('/test/byok/secrets', { + commandAvailable, + createWorker: createWorkerClient, + }); + + await expect(backend.available()).resolves.toBe(false); + await expect(backend.available()).resolves.toBe(true); + + expect(commandAvailable).toHaveBeenCalledTimes(2); + expect(createWorkerClient).toHaveBeenCalledTimes(1); + expect(worker.ready).toHaveBeenCalledTimes(1); + }); + + it('fails the current operation once and rebuilds only for the next independent request', async () => { + const failedWorker = createWorker( + vi.fn(async () => { + throw new Error('worker exited'); + }), + ); + const healthyWorker = createWorker( + vi.fn(async (operation) => ({ + found: operation === 'get', + value: operation === 'get' ? 'recovered-secret' : null, + })), + ); + const createWorkerClient = vi.fn() + .mockResolvedValueOnce(failedWorker) + .mockResolvedValueOnce(healthyWorker); + const backend = new WindowsDpapiBackend('/test/byok/secrets', { + commandAvailable: async () => true, + createWorker: createWorkerClient, + }); + + await expect(backend.set('byok-worker-recovery', 'do-not-replay')).rejects.toThrow( + 'Secure credential backend command failed.', + ); + expect(failedWorker.run).toHaveBeenCalledTimes(1); + expect(createWorkerClient).toHaveBeenCalledTimes(1); + + await expect(backend.get('byok-worker-recovery')).resolves.toBe('recovered-secret'); + + expect(failedWorker.close).toHaveBeenCalledTimes(1); + expect(createWorkerClient).toHaveBeenCalledTimes(2); + expect(healthyWorker.run).toHaveBeenCalledTimes(1); + }); + + it('emits only allowlisted diagnostics and never includes the secret or secret path', async () => { + const secret = 'diagnostic-must-not-leak-this-secret'; + const diagnostics: WindowsDpapiWorkerDiagnostic[] = []; + const failedWorker = createWorker( + vi.fn(async () => { + throw new Error(`unsafe child detail ${secret} /test/byok/secrets/profile.bin`); + }), + ); + const backend = new WindowsDpapiBackend( + '/test/byok/secrets', + backendOptions({ + commandAvailable: async () => true, + createWorker: async () => failedWorker, + onDiagnostic: (diagnostic) => diagnostics.push(diagnostic), + }), + ); + + await expect(backend.set('byok-worker-diagnostics', secret)).rejects.toThrow( + 'Secure credential backend command failed.', + ); + + expect(diagnostics).toHaveLength(1); + expect(Object.keys(diagnostics[0] ?? {}).sort()).toEqual([ + 'durationMs', + 'failureClass', + 'phase', + 'workerGeneration', + ]); + expect(JSON.stringify(diagnostics)).not.toContain(secret); + expect(JSON.stringify(diagnostics)).not.toContain('/test/byok/secrets'); + }); + + it('bounds worker creation and closes a late generation without evicting the replacement', async () => { + vi.useFakeTimers(); + let resolveFirstWorker: ((worker: FakeWorker) => void) | undefined; + const lateWorker = createWorker(); + const healthyWorker = createWorker( + vi.fn(async () => ({ found: true, value: 'healthy-secret' })), + ); + const createWorkerClient = vi.fn() + .mockImplementationOnce(() => new Promise((resolve) => { + resolveFirstWorker = resolve; + })) + .mockResolvedValueOnce(healthyWorker); + const diagnostics: WindowsDpapiWorkerDiagnostic[] = []; + const backend = new WindowsDpapiBackend('/test/byok/secrets', { + commandAvailable: async () => true, + createWorker: createWorkerClient, + onDiagnostic: (diagnostic) => diagnostics.push(diagnostic), + timeouts: { + startupMs: 10, + readyMs: 10, + operationMs: 10, + shutdownMs: 10, + }, + }); + + const firstAvailability = backend.available(); + await vi.advanceTimersByTimeAsync(21); + await expect(firstAvailability).resolves.toBe(false); + await expect(backend.available()).resolves.toBe(true); + + resolveFirstWorker?.(lateWorker); + await vi.advanceTimersByTimeAsync(0); + + expect(lateWorker.close).toHaveBeenCalledTimes(1); + await expect(backend.get('byok-worker-generation')).resolves.toBe('healthy-secret'); + expect(createWorkerClient).toHaveBeenCalledTimes(2); + expect(diagnostics).toEqual([ + { + phase: 'spawn', + failureClass: 'timeout', + durationMs: 10, + workerGeneration: 1, + }, + { + phase: 'shutdown', + failureClass: 'timeout', + durationMs: 10, + workerGeneration: 1, + }, + ]); + }); + + it('bounds the ready phase and rebuilds on the next availability probe', async () => { + vi.useFakeTimers(); + const stalledWorker = createWorker(); + stalledWorker.ready.mockImplementation(() => new Promise(() => undefined)); + const healthyWorker = createWorker(); + const createWorkerClient = vi.fn() + .mockResolvedValueOnce(stalledWorker) + .mockResolvedValueOnce(healthyWorker); + const backend = new WindowsDpapiBackend('/test/byok/secrets', { + commandAvailable: async () => true, + createWorker: createWorkerClient, + timeouts: { + startupMs: 10, + readyMs: 10, + operationMs: 10, + shutdownMs: 10, + }, + }); + + const firstAvailability = backend.available(); + await vi.advanceTimersByTimeAsync(10); + await expect(firstAvailability).resolves.toBe(false); + await expect(backend.available()).resolves.toBe(true); + + expect(stalledWorker.close).toHaveBeenCalledTimes(1); + expect(createWorkerClient).toHaveBeenCalledTimes(2); + }); + + it('bounds an operation, does not replay it, and rebuilds for the next request', async () => { + vi.useFakeTimers(); + const stalledWorker = createWorker( + vi.fn(() => new Promise(() => undefined)), + ); + const healthyWorker = createWorker( + vi.fn(async () => ({ found: true, value: 'after-timeout' })), + ); + const createWorkerClient = vi.fn() + .mockResolvedValueOnce(stalledWorker) + .mockResolvedValueOnce(healthyWorker); + const backend = new WindowsDpapiBackend('/test/byok/secrets', { + commandAvailable: async () => true, + createWorker: createWorkerClient, + timeouts: { + startupMs: 10, + readyMs: 10, + operationMs: 10, + shutdownMs: 10, + }, + }); + + const timedOutSet = backend.set('byok-worker-operation-timeout', 'do-not-replay'); + const rejection = expect(timedOutSet).rejects.toThrow( + 'Secure credential backend command failed.', + ); + await vi.advanceTimersByTimeAsync(10); + await rejection; + + expect(stalledWorker.run).toHaveBeenCalledTimes(1); + await expect(backend.get('byok-worker-operation-timeout')).resolves.toBe('after-timeout'); + expect(createWorkerClient).toHaveBeenCalledTimes(2); + }); + + it('keeps a healthy worker after a non-fatal DPAPI operation error', async () => { + const worker = createWorker( + vi.fn() + .mockRejectedValueOnce(new WindowsDpapiWorkerError('operation', 'dpapi', false)) + .mockResolvedValueOnce({ found: true, value: 'still-healthy' }), + ); + const createWorkerClient = vi.fn(async () => worker); + const backend = new WindowsDpapiBackend('/test/byok/secrets', { + commandAvailable: async () => true, + createWorker: createWorkerClient, + }); + + await expect(backend.set('byok-worker-dpapi-error', 'secret')).rejects.toThrow( + 'Secure credential backend command failed.', + ); + await expect(backend.get('byok-worker-dpapi-error')).resolves.toBe('still-healthy'); + + expect(createWorkerClient).toHaveBeenCalledTimes(1); + expect(worker.close).not.toHaveBeenCalled(); + }); + + it('closes the owned worker once and rejects new operations after shutdown starts', async () => { + const worker = createWorker(); + const backend = new WindowsDpapiBackend('/test/byok/secrets', { + commandAvailable: async () => true, + createWorker: async () => worker, + }); + await expect(backend.available()).resolves.toBe(true); + + await expect(Promise.all([backend.close(), backend.close()])).resolves.toEqual([ + undefined, + undefined, + ]); + await expect(backend.available()).resolves.toBe(false); + await expect(backend.get('byok-worker-after-close')).rejects.toMatchObject({ + phase: 'operation', + failureClass: 'closed', + }); + + expect(worker.close).toHaveBeenCalledTimes(1); + }); + + it('propagates and diagnoses a bounded shutdown timeout without leaking worker details', async () => { + vi.useFakeTimers(); + const diagnostics: WindowsDpapiWorkerDiagnostic[] = []; + const worker = createWorker(); + worker.close.mockImplementation(() => new Promise(() => undefined)); + const backend = new WindowsDpapiBackend('/test/byok/secrets', { + commandAvailable: async () => true, + createWorker: async () => worker, + onDiagnostic: (diagnostic) => diagnostics.push(diagnostic), + timeouts: { shutdownMs: 10 }, + }); + await expect(backend.available()).resolves.toBe(true); + + const close = backend.close(); + const rejection = expect(close).rejects.toMatchObject({ + message: 'Secure credential backend command failed.', + phase: 'shutdown', + failureClass: 'timeout', + }); + await vi.advanceTimersByTimeAsync(10); + await rejection; + + expect(diagnostics).toEqual([ + { + phase: 'shutdown', + failureClass: 'timeout', + durationMs: 10, + workerGeneration: 1, + }, + ]); + }); +}); diff --git a/apps/daemon/tests/byok/windows-dpapi-worker-orphan.windows.test.ts b/apps/daemon/tests/byok/windows-dpapi-worker-orphan.windows.test.ts new file mode 100644 index 00000000000..73c9962869e --- /dev/null +++ b/apps/daemon/tests/byok/windows-dpapi-worker-orphan.windows.test.ts @@ -0,0 +1,140 @@ +import { spawn, type ChildProcessWithoutNullStreams } from 'node:child_process'; +import { once } from 'node:events'; +import { mkdtemp, rm, writeFile } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import path from 'node:path'; +import { pathToFileURL } from 'node:url'; + +import { afterEach, describe, expect, it } from 'vitest'; + +const describeWindows = describe.runIf(process.platform === 'win32'); + +describeWindows('Windows DPAPI worker orphan boundary', () => { + const roots: string[] = []; + const children: ChildProcessWithoutNullStreams[] = []; + const workerPids = new Set(); + + afterEach(async () => { + for (const child of children.splice(0)) { + if (child.exitCode === null && child.signalCode === null) child.kill('SIGKILL'); + } + for (const pid of workerPids) { + if (isProcessAlive(pid)) { + try { + process.kill(pid, 'SIGKILL'); + } catch { + // Exact owned PID cleanup only; already-exited workers are expected. + } + } + } + workerPids.clear(); + await Promise.all( + roots.splice(0).map((root) => rm(root, { recursive: true, force: true })), + ); + }); + + it('exits after its daemon parent is force-terminated without running shutdown handlers', { + timeout: 60_000, + }, async () => { + const root = await mkdtemp(path.join(tmpdir(), 'od-dpapi-orphan-')); + roots.push(root); + const fixturePath = path.join(root, 'worker-parent.mjs'); + const workerModuleUrl = pathToFileURL( + path.resolve('src/byok/windows-dpapi-worker.ts'), + ).href; + await writeFile( + fixturePath, + [ + `import { WindowsDpapiWorker } from ${JSON.stringify(workerModuleUrl)};`, + 'const worker = await WindowsDpapiWorker.create();', + 'await worker.ready();', + 'process.stdout.write(`${JSON.stringify({ workerPid: worker.processId })}\\n`);', + 'setInterval(() => {}, 1_000);', + ].join('\n'), + 'utf8', + ); + const parent = spawn(process.execPath, ['--import', 'tsx', fixturePath], { + cwd: path.resolve('.'), + env: process.env, + stdio: ['pipe', 'pipe', 'pipe'], + windowsHide: true, + }); + children.push(parent); + parent.stdin.end(); + const firstLine = await readFirstLine(parent); + const payload = JSON.parse(firstLine) as { workerPid?: unknown }; + expect(payload.workerPid).toEqual(expect.any(Number)); + const workerPid = Number(payload.workerPid); + workerPids.add(workerPid); + expect(isProcessAlive(workerPid)).toBe(true); + + parent.kill('SIGKILL'); + await waitForChildClose(parent, 5_000); + await expect(waitForProcessExit(workerPid, 5_000)).resolves.toBe(true); + workerPids.delete(workerPid); + }); +}); + +async function readFirstLine(child: ChildProcessWithoutNullStreams): Promise { + let stdout = ''; + let stderr = ''; + const onStderr = (chunk: Buffer) => { + if (stderr.length < 8_192) stderr += chunk.toString('utf8'); + }; + child.stderr.on('data', onStderr); + return new Promise((resolve, reject) => { + const timeout = setTimeout(() => { + reject(new Error(`Windows DPAPI worker parent did not become ready: ${stderr}`)); + }, 50_000); + timeout.unref?.(); + const onStdout = (chunk: Buffer) => { + stdout += chunk.toString('utf8'); + const newline = stdout.indexOf('\n'); + if (newline < 0) return; + clearTimeout(timeout); + child.stdout.off('data', onStdout); + resolve(stdout.slice(0, newline)); + }; + child.stdout.on('data', onStdout); + child.once('close', (code) => { + if (stdout.includes('\n')) return; + clearTimeout(timeout); + reject(new Error(`Windows DPAPI worker parent exited before ready (${code}): ${stderr}`)); + }); + }); +} + +async function waitForChildClose( + child: ChildProcessWithoutNullStreams, + timeoutMs: number, +): Promise { + if (child.exitCode !== null || child.signalCode !== null) return; + await Promise.race([ + once(child, 'close').then(() => undefined), + new Promise((_, reject) => { + const timeout = setTimeout(() => reject(new Error('parent process did not exit')), timeoutMs); + timeout.unref?.(); + }), + ]); +} + +function isProcessAlive(pid: number): boolean { + try { + process.kill(pid, 0); + return true; + } catch (error) { + return (error as NodeJS.ErrnoException).code !== 'ESRCH'; + } +} + +async function waitForProcessExit(pid: number, timeoutMs: number): Promise { + const deadline = Date.now() + timeoutMs; + while (Date.now() < deadline) { + if (!isProcessAlive(pid)) return true; + await new Promise((resolve) => { + const timer = setTimeout(resolve, 100); + timer.unref?.(); + }); + } + return !isProcessAlive(pid); +} diff --git a/apps/daemon/tests/byok/windows-dpapi-worker.test.ts b/apps/daemon/tests/byok/windows-dpapi-worker.test.ts new file mode 100644 index 00000000000..f1b1a6902fc --- /dev/null +++ b/apps/daemon/tests/byok/windows-dpapi-worker.test.ts @@ -0,0 +1,414 @@ +import type { ChildProcessWithoutNullStreams } from 'node:child_process'; +import { EventEmitter } from 'node:events'; +import { mkdtemp, rm } from 'node:fs/promises'; +import type { Socket } from 'node:net'; +import { tmpdir } from 'node:os'; +import path from 'node:path'; +import { Duplex, PassThrough } from 'node:stream'; + +import { afterEach, describe, expect, it, vi } from 'vitest'; + +import { + WindowsDpapiWorker, + WindowsDpapiWorkerError, +} from '../../src/byok/windows-dpapi-worker.js'; + +type FakeChild = ChildProcessWithoutNullStreams & { + emitClose(code?: number | null): void; + kill: ReturnType boolean>>; +}; + +type WorkerHarness = { + child: FakeChild; + inject(line: string): void; + pipe: Socket; + worker: WindowsDpapiWorker; + writes: string[]; +}; + +function createFakeChild({ exitOnKill = true }: { exitOnKill?: boolean } = {}): FakeChild { + const child = Object.assign(new EventEmitter(), { + stdin: new PassThrough(), + stdout: new PassThrough(), + stderr: new PassThrough(), + killed: false, + exitCode: null as number | null, + signalCode: null as NodeJS.Signals | null, + emitClose: (_code?: number | null) => undefined, + kill: vi.fn<(signal?: NodeJS.Signals | number) => boolean>(), + }); + child.emitClose = (code = 0) => { + if (child.exitCode !== null || child.signalCode !== null) return; + child.exitCode = code; + child.emit('close', code, null); + }; + child.kill = vi.fn(() => { + child.killed = true; + if (exitOnKill) queueMicrotask(() => child.emitClose(1)); + return true; + }); + return child as unknown as FakeChild; +} + +function createHarness({ + exitOnKill = true, + exitOnPipeEnd = false, + onWrite, + shutdownTimeoutMs = 10, +}: { + exitOnKill?: boolean; + exitOnPipeEnd?: boolean; + onWrite?: (line: string, harness: WorkerHarness) => void; + shutdownTimeoutMs?: number; +} = {}): WorkerHarness { + const child = createFakeChild({ exitOnKill }); + const writes: string[] = []; + let harness: WorkerHarness; + const pipe = new Duplex({ + read() {}, + write(chunk, _encoding, callback) { + const line = chunk.toString('utf8'); + writes.push(line); + onWrite?.(line, harness); + callback(); + }, + final(callback) { + if (exitOnPipeEnd) queueMicrotask(() => child.emitClose(0)); + callback(); + }, + }) as Socket; + const worker = WindowsDpapiWorker.fromConnectedTransport(child, pipe, { + shutdownTimeoutMs, + }); + harness = { + child, + inject(line) { + pipe.push(Buffer.from(line, 'utf8')); + }, + pipe, + worker, + writes, + }; + return harness; +} + +function ready(harness: WorkerHarness): Promise { + harness.inject('{"type":"ready","ok":true}\n'); + return harness.worker.ready(); +} + +function parsedRequest(line: string): { + id: string; + operation: 'set' | 'get' | 'delete'; +} { + return JSON.parse(line) as { + id: string; + operation: 'set' | 'get' | 'delete'; + }; +} + +describe('Windows DPAPI worker protocol and lifecycle', () => { + const roots: string[] = []; + + afterEach(() => { + vi.useRealTimers(); + return Promise.all( + roots.splice(0).map((root) => rm(root, { recursive: true, force: true })), + ); + }); + + it('bounds the pipe connection phase, terminates the child, and passes only a parent PID watchdog input', async () => { + vi.useFakeTimers(); + const root = await mkdtemp(path.join(tmpdir(), 'od-dpapi-worker-connect-')); + roots.push(root); + const child = createFakeChild(); + let observeSpawn: (() => void) | undefined; + const spawned = new Promise((resolve) => { + observeSpawn = resolve; + }); + let spawnEnvironment: NodeJS.ProcessEnv | undefined; + const creating = WindowsDpapiWorker.create({ + connectTimeoutMs: 10, + pipeAddress: { + connectionName: path.join(root, 'worker.sock'), + listenPath: path.join(root, 'worker.sock'), + }, + spawnWorker: (_command, _args, options) => { + spawnEnvironment = options.env; + observeSpawn?.(); + return child; + }, + }); + const rejection = expect(creating).rejects.toMatchObject({ + phase: 'connect', + failureClass: 'timeout', + }); + await spawned; + await vi.advanceTimersByTimeAsync(10); + await rejection; + + expect(child.kill).toHaveBeenCalledTimes(1); + expect(spawnEnvironment?.OD_BYOK_DPAPI_PARENT_PID).toBe(String(process.pid)); + expect(spawnEnvironment?.OD_BYOK_DPAPI_PIPE_NAME).toBe( + path.join(root, 'worker.sock'), + ); + expect(JSON.stringify(spawnEnvironment)).not.toContain('secretPath'); + }); + + it('aborts an in-progress connection immediately during daemon shutdown', async () => { + const root = await mkdtemp(path.join(tmpdir(), 'od-dpapi-worker-abort-')); + roots.push(root); + const child = createFakeChild(); + const abortController = new AbortController(); + let observeSpawn: (() => void) | undefined; + const spawned = new Promise((resolve) => { + observeSpawn = resolve; + }); + const creating = WindowsDpapiWorker.create({ + connectTimeoutMs: 50_000, + pipeAddress: { + connectionName: path.join(root, 'worker.sock'), + listenPath: path.join(root, 'worker.sock'), + }, + signal: abortController.signal, + spawnWorker: () => { + observeSpawn?.(); + return child; + }, + }); + const rejection = expect(creating).rejects.toMatchObject({ + phase: 'shutdown', + failureClass: 'closed', + fatal: false, + }); + await spawned; + + abortController.abort(); + await rejection; + + expect(child.kill).toHaveBeenCalledTimes(1); + }); + + it('rejects ready when the child exits before the handshake', async () => { + const harness = createHarness(); + const readyResult = harness.worker.ready(); + harness.child.emitClose(1); + + await expect(readyResult).rejects.toMatchObject({ + message: 'Secure credential backend command failed.', + phase: 'ready', + failureClass: 'child-exit', + fatal: true, + }); + }); + + it('fails a pending operation when the child exits', async () => { + let requestWritten: (() => void) | undefined; + const writeObserved = new Promise((resolve) => { + requestWritten = resolve; + }); + const harness = createHarness({ + onWrite: () => requestWritten?.(), + }); + await ready(harness); + const result = harness.worker.run('get', '/secret/path'); + await writeObserved; + harness.child.emitClose(1); + + await expect(result).rejects.toMatchObject({ + phase: 'operation', + failureClass: 'child-exit', + fatal: true, + }); + }); + + it('settles queued operations after a crash instead of leaving the queue hung', async () => { + let requestWritten: (() => void) | undefined; + const writeObserved = new Promise((resolve) => { + requestWritten = resolve; + }); + const harness = createHarness({ + onWrite: () => requestWritten?.(), + }); + await ready(harness); + const inFlight = harness.worker.run('set', '/secret/path', 'secret'); + const queued = harness.worker.run('get', '/secret/path'); + const inFlightRejection = expect(inFlight).rejects.toMatchObject({ + phase: 'operation', + failureClass: 'child-exit', + }); + const queuedRejection = expect(queued).rejects.toMatchObject({ + phase: 'operation', + failureClass: 'closed', + }); + await writeObserved; + + harness.child.emitClose(1); + + await Promise.all([inFlightRejection, queuedRejection]); + expect(harness.writes).toHaveLength(1); + }); + + it.each([ + ['malformed JSON', 'not-json\n'], + ['response ID mismatch', '{"id":"wrong","ok":true,"found":false}\n'], + ])('treats %s as a fatal protocol error', async (_label, response) => { + const harness = createHarness({ + onWrite: () => queueMicrotask(() => harness.inject(response)), + }); + await ready(harness); + + await expect(harness.worker.run('get', '/secret/path')).rejects.toMatchObject({ + phase: 'operation', + failureClass: 'protocol', + fatal: true, + }); + expect(harness.child.kill).toHaveBeenCalledTimes(1); + }); + + it('rejects an oversized response line and terminates the worker', async () => { + const harness = createHarness({ + onWrite: () => queueMicrotask(() => harness.inject('x'.repeat(64 * 1024 + 1))), + }); + await ready(harness); + + await expect(harness.worker.run('get', '/secret/path')).rejects.toMatchObject({ + phase: 'operation', + failureClass: 'protocol', + fatal: true, + }); + expect(harness.child.kill).toHaveBeenCalledTimes(1); + }); + + it('keeps the worker alive after a request-scoped DPAPI failure', async () => { + let requests = 0; + const harness = createHarness({ + onWrite: (line) => { + const request = parsedRequest(line); + requests += 1; + queueMicrotask(() => { + if (requests === 1) { + harness.inject(`${JSON.stringify({ id: request.id, ok: false })}\n`); + } else { + harness.inject(`${JSON.stringify({ + id: request.id, + ok: true, + found: true, + value: Buffer.from('recovered', 'utf8').toString('base64'), + })}\n`); + } + }); + }, + exitOnPipeEnd: true, + }); + await ready(harness); + + await expect(harness.worker.run('set', '/secret/path', 'secret')).rejects.toMatchObject({ + phase: 'operation', + failureClass: 'dpapi', + fatal: false, + }); + await expect(harness.worker.run('get', '/secret/path')).resolves.toEqual({ + found: true, + value: 'recovered', + }); + expect(harness.child.kill).not.toHaveBeenCalled(); + await harness.worker.close(); + }); + + it('rejects an oversized request before writing it and keeps the worker usable', async () => { + const harness = createHarness({ + onWrite: (line) => { + const request = parsedRequest(line); + queueMicrotask(() => harness.inject(`${JSON.stringify({ + id: request.id, + ok: true, + found: false, + })}\n`)); + }, + exitOnPipeEnd: true, + }); + await ready(harness); + + await expect( + harness.worker.run('get', `/secret/${'x'.repeat(64 * 1024)}`), + ).rejects.toMatchObject({ + phase: 'operation', + failureClass: 'input', + fatal: false, + }); + await expect(harness.worker.run('get', '/secret/path')).resolves.toEqual({ + found: false, + value: null, + }); + + expect(harness.writes).toHaveLength(1); + expect(harness.child.kill).not.toHaveBeenCalled(); + await harness.worker.close(); + }); + + it('closes gracefully on pipe EOF and is idempotent', async () => { + const harness = createHarness({ exitOnPipeEnd: true }); + await ready(harness); + + const firstClose = harness.worker.close(); + const secondClose = harness.worker.close(); + await expect(Promise.all([firstClose, secondClose])).resolves.toEqual([ + undefined, + undefined, + ]); + + expect(harness.child.kill).not.toHaveBeenCalled(); + }); + + it('force-terminates the owned child after the graceful shutdown budget', async () => { + vi.useFakeTimers(); + const harness = createHarness({ exitOnKill: true, shutdownTimeoutMs: 10 }); + await ready(harness); + + const close = harness.worker.close(); + await vi.advanceTimersByTimeAsync(10); + await expect(close).resolves.toBeUndefined(); + + expect(harness.child.kill).toHaveBeenCalledTimes(1); + }); + + it('reports a bounded safe shutdown failure when the child ignores termination', async () => { + vi.useFakeTimers(); + const harness = createHarness({ exitOnKill: false, shutdownTimeoutMs: 10 }); + await ready(harness); + + const close = harness.worker.close(); + const rejection = expect(close).rejects.toEqual( + expect.objectContaining({ + message: 'Secure credential backend command failed.', + phase: 'shutdown', + failureClass: 'timeout', + }), + ); + await vi.advanceTimersByTimeAsync(20); + await rejection; + + expect(harness.child.kill).toHaveBeenCalledTimes(1); + }); + + it('rejects non-canonical base64 without returning attacker-controlled bytes', async () => { + const harness = createHarness({ + onWrite: (line) => { + const request = parsedRequest(line); + queueMicrotask(() => harness.inject(`${JSON.stringify({ + id: request.id, + ok: true, + found: true, + value: '%%%%', + })}\n`)); + }, + }); + await ready(harness); + + await expect(harness.worker.run('get', '/secret/path')).rejects.toBeInstanceOf( + WindowsDpapiWorkerError, + ); + expect(harness.child.kill).toHaveBeenCalledTimes(1); + }); +}); diff --git a/tools/pack/src/win/lifecycle.ts b/tools/pack/src/win/lifecycle.ts index aaf8c75b261..e529f063257 100644 --- a/tools/pack/src/win/lifecycle.ts +++ b/tools/pack/src/win/lifecycle.ts @@ -64,6 +64,7 @@ import type { const PACKAGED_CONFIG_PATH_ENV = "OD_PACKAGED_CONFIG_PATH"; const UPDATE_ACTION_TIMEOUT_MS = 10 * 60 * 1000; +export const WIN_DESKTOP_EVAL_TIMEOUT_MS = 60_000; function desktopStamp(config: ToolPackConfig): SidecarStamp { return { @@ -491,7 +492,7 @@ async function requestDesktopEval( return await requestJsonIpc( ipc, { input: { expression }, type: SIDECAR_MESSAGES.EVAL }, - { timeoutMs: 60_000 }, + { timeoutMs: WIN_DESKTOP_EVAL_TIMEOUT_MS }, ); } catch (error) { return { diff --git a/tools/pack/tests/win-lifecycle-timeouts.test.ts b/tools/pack/tests/win-lifecycle-timeouts.test.ts new file mode 100644 index 00000000000..7687989eeda --- /dev/null +++ b/tools/pack/tests/win-lifecycle-timeouts.test.ts @@ -0,0 +1,9 @@ +import { describe, expect, it } from "vitest"; + +import { WIN_DESKTOP_EVAL_TIMEOUT_MS } from "../src/win/lifecycle.js"; + +describe("Windows packaged lifecycle timeout contracts", () => { + it("keeps desktop eval at the release smoke budget of exactly 60 seconds", () => { + expect(WIN_DESKTOP_EVAL_TIMEOUT_MS).toBe(60_000); + }); +}); From d439809ad6d2dd55243a5f953b5360cf41fc939e Mon Sep 17 00:00:00 2001 From: Cheems <94773058+itscheems@users.noreply.github.com> Date: Fri, 31 Jul 2026 15:13:02 +0800 Subject: [PATCH 4/4] fix(daemon): authenticate the Windows DPAPI worker pipe --- apps/daemon/src/byok/windows-dpapi-worker.ts | 184 +++++++++++++++++- .../tests/byok/windows-dpapi-worker.test.ts | 179 +++++++++++++++-- 2 files changed, 339 insertions(+), 24 deletions(-) diff --git a/apps/daemon/src/byok/windows-dpapi-worker.ts b/apps/daemon/src/byok/windows-dpapi-worker.ts index 1a348a55d9f..5be607b69ef 100644 --- a/apps/daemon/src/byok/windows-dpapi-worker.ts +++ b/apps/daemon/src/byok/windows-dpapi-worker.ts @@ -1,10 +1,14 @@ import type { ChildProcessWithoutNullStreams, SpawnOptionsWithoutStdio } from 'node:child_process'; import { spawn } from 'node:child_process'; -import { randomUUID } from 'node:crypto'; +import { createHmac, randomBytes, randomUUID, timingSafeEqual } from 'node:crypto'; import type { Server, Socket } from 'node:net'; import { createServer } from 'node:net'; const MAX_WORKER_LINE_BYTES = 64 * 1024; +const MAX_AUTHENTICATING_CONNECTIONS = 16; +const PIPE_AUTH_CHALLENGE_BYTES = 32; +const PIPE_AUTH_KEY_BYTES = 32; +const PIPE_AUTH_TIMEOUT_MS = 5_000; const DEFAULT_CONNECT_TIMEOUT_MS = 50_000; const DEFAULT_SHUTDOWN_TIMEOUT_MS = 5_000; @@ -67,6 +71,7 @@ type WindowsDpapiWorkerResponse = { found?: boolean; id?: string; ok?: boolean; + proof?: string; type?: string; value?: string; }; @@ -127,6 +132,41 @@ $writer = New-Object System.IO.StreamWriter( ) $writer.AutoFlush = $true +try { + $authenticationKey = [System.Convert]::FromBase64String( + [Console]::In.ReadLine() + ) + if ($authenticationKey.Length -ne 32) { + throw 'Invalid pipe authentication key' + } + $challengeEnvelope = $reader.ReadLine() | ConvertFrom-Json + if ([string]$challengeEnvelope.type -ne 'challenge') { + throw 'Invalid pipe authentication challenge' + } + $challenge = [System.Convert]::FromBase64String( + [string]$challengeEnvelope.challenge + ) + if ($challenge.Length -ne 32) { + throw 'Invalid pipe authentication challenge' + } + $hmac = [System.Security.Cryptography.HMACSHA256]::new($authenticationKey) + try { + $proof = $hmac.ComputeHash($challenge) + } finally { + $hmac.Dispose() + } + Write-OpenDesignDpapiResponse @{ + type = 'authenticate' + proof = [System.Convert]::ToBase64String($proof) + } +} catch { + exit 1 +} finally { + if ($authenticationKey -ne $null) { + [System.Array]::Clear($authenticationKey, 0, $authenticationKey.Length) + } +} + try { [void][System.Reflection.Assembly]::LoadFrom( [System.IO.Path]::Combine( @@ -298,6 +338,7 @@ export class WindowsDpapiWorker { this.pipe.on('close', () => { if (!this.closing) this.fail('pipe-close'); }); + this.pipe.resume(); this.child.stdout.resume(); this.child.stderr.resume(); this.child.on('error', () => { @@ -326,6 +367,7 @@ export class WindowsDpapiWorker { await closeServer(server); throw secureCredentialCommandError('shutdown', 'closed', false); } + const authenticationKey = randomBytes(PIPE_AUTH_KEY_BYTES); let child: ChildProcessWithoutNullStreams; try { child = (options.spawnWorker ?? spawn)( @@ -350,14 +392,21 @@ export class WindowsDpapiWorker { }, ); } catch { + authenticationKey.fill(0); await closeServer(server); throw secureCredentialCommandError('spawn', 'spawn'); } - // The packaged process must not retain an open stdin stream. Requests use - // the random per-process local pipe below, keeping plaintext credentials - // out of argv, environment variables, and temporary files. - child.stdin.end(); - const connection = waitForConnection(server); + // The packaged process must not retain an open stdin stream. Only this + // per-generation pipe-authentication capability crosses stdin; BYOK + // credentials stay out of argv, environment variables, and temporary + // files and are sent only after the child proves possession of the key. + child.stdin.end(`${authenticationKey.toString('base64')}\n`); + const connectionAbortController = new AbortController(); + const connection = waitForAuthenticatedConnection( + server, + authenticationKey, + connectionAbortController.signal, + ); try { const pipe = await withTimeout( Promise.race([ @@ -368,9 +417,12 @@ export class WindowsDpapiWorker { positiveTimeout(options.connectTimeoutMs, DEFAULT_CONNECT_TIMEOUT_MS), 'connect', ); + authenticationKey.fill(0); server.close(); return new WindowsDpapiWorker(child, pipe, options); } catch (error) { + authenticationKey.fill(0); + connectionAbortController.abort(); void connection.then( (latePipe) => latePipe.destroy(), () => undefined, @@ -613,10 +665,124 @@ function listen(server: Server, path: string): Promise { }); } -function waitForConnection(server: Server): Promise { +function waitForAuthenticatedConnection( + server: Server, + authenticationKey: Buffer, + signal: AbortSignal, +): Promise { return new Promise((resolve, reject) => { - server.once('connection', resolve); - server.once('error', reject); + let settled = false; + const authenticating = new Set(); + const cleanup = () => { + server.off('connection', onConnection); + server.off('error', onServerError); + signal.removeEventListener('abort', onAbort); + }; + const rejectOnce = (error: WindowsDpapiWorkerError) => { + if (settled) return; + settled = true; + cleanup(); + for (const pipe of authenticating) pipe.destroy(); + authenticating.clear(); + reject(error); + }; + const onServerError = () => { + rejectOnce(secureCredentialCommandError('connect', 'pipe-close')); + }; + const onAbort = () => { + rejectOnce(secureCredentialCommandError('shutdown', 'closed', false)); + }; + const onConnection = (pipe: Socket) => { + if (settled || authenticating.size >= MAX_AUTHENTICATING_CONNECTIONS) { + pipe.destroy(); + return; + } + authenticating.add(pipe); + void authenticatePipeClient(pipe, authenticationKey).then((authenticated) => { + authenticating.delete(pipe); + if (settled) { + pipe.destroy(); + return; + } + if (!authenticated) { + pipe.destroy(); + return; + } + settled = true; + cleanup(); + for (const otherPipe of authenticating) otherPipe.destroy(); + authenticating.clear(); + resolve(pipe); + }); + }; + server.on('connection', onConnection); + server.once('error', onServerError); + signal.addEventListener('abort', onAbort, { once: true }); + }); +} + +function authenticatePipeClient( + pipe: Socket, + authenticationKey: Buffer, +): Promise { + return new Promise((resolve) => { + const challenge = randomBytes(PIPE_AUTH_CHALLENGE_BYTES); + const expectedProof = createHmac('sha256', authenticationKey) + .update(challenge) + .digest(); + let inputBuffer = ''; + let settled = false; + const finish = (authenticated: boolean, remainder = '') => { + if (settled) return; + settled = true; + clearTimeout(timeout); + pipe.off('data', onData); + if (authenticated) { + pipe.pause(); + if (remainder) pipe.unshift(Buffer.from(remainder, 'utf8')); + } + challenge.fill(0); + expectedProof.fill(0); + resolve(authenticated); + }; + const onData = (chunk: Buffer) => { + inputBuffer += chunk.toString('utf8'); + if (Buffer.byteLength(inputBuffer, 'utf8') > MAX_WORKER_LINE_BYTES) { + finish(false); + return; + } + const newlineIndex = inputBuffer.indexOf('\n'); + if (newlineIndex < 0) return; + const line = inputBuffer.slice(0, newlineIndex).trim(); + const remainder = inputBuffer.slice(newlineIndex + 1); + let response: WindowsDpapiWorkerResponse; + try { + response = JSON.parse(line) as WindowsDpapiWorkerResponse; + } catch { + finish(false); + return; + } + const proof = typeof response.proof === 'string' + ? decodeCanonicalBase64(response.proof) + : null; + finish( + response.type === 'authenticate' + && proof?.byteLength === expectedProof.byteLength + && timingSafeEqual(proof, expectedProof), + remainder, + ); + }; + const timeout = setTimeout(() => finish(false), PIPE_AUTH_TIMEOUT_MS); + timeout.unref?.(); + pipe.once('error', () => finish(false)); + pipe.once('close', () => finish(false)); + pipe.on('data', onData); + pipe.write(`${JSON.stringify({ + type: 'challenge', + challenge: challenge.toString('base64'), + })}\n`, 'utf8', (error) => { + if (error) finish(false); + }); }); } diff --git a/apps/daemon/tests/byok/windows-dpapi-worker.test.ts b/apps/daemon/tests/byok/windows-dpapi-worker.test.ts index f1b1a6902fc..02c52f5366b 100644 --- a/apps/daemon/tests/byok/windows-dpapi-worker.test.ts +++ b/apps/daemon/tests/byok/windows-dpapi-worker.test.ts @@ -1,7 +1,8 @@ import type { ChildProcessWithoutNullStreams } from 'node:child_process'; -import { EventEmitter } from 'node:events'; +import { createHmac, randomUUID } from 'node:crypto'; +import { EventEmitter, once } from 'node:events'; import { mkdtemp, rm } from 'node:fs/promises'; -import type { Socket } from 'node:net'; +import { createConnection, type Socket } from 'node:net'; import { tmpdir } from 'node:os'; import path from 'node:path'; import { Duplex, PassThrough } from 'node:stream'; @@ -107,34 +108,100 @@ function parsedRequest(line: string): { }; } +function createTestPipeAddress(root: string): { + connectionName: string; + listenPath: string; +} { + if (process.platform === 'win32') { + const connectionName = `open-design-dpapi-test-${process.pid}-${randomUUID()}`; + return { + connectionName, + listenPath: `\\\\.\\pipe\\${connectionName}`, + }; + } + const listenPath = path.join(root, 'worker.sock'); + return { + connectionName: listenPath, + listenPath, + }; +} + +function connectSocket(listenPath: string): Promise { + return new Promise((resolve, reject) => { + const socket = createConnection(listenPath); + socket.once('connect', () => resolve(socket)); + socket.once('error', reject); + }); +} + +function readSocketLine(socket: Socket): Promise { + return new Promise((resolve, reject) => { + let buffer = ''; + const cleanup = () => { + socket.off('data', onData); + socket.off('error', onError); + socket.off('close', onClose); + }; + const onError = (error: Error) => { + cleanup(); + reject(error); + }; + const onClose = () => { + cleanup(); + reject(new Error('socket closed before a complete line was received')); + }; + const onData = (chunk: Buffer) => { + buffer += chunk.toString('utf8'); + const newlineIndex = buffer.indexOf('\n'); + if (newlineIndex < 0) return; + const line = buffer.slice(0, newlineIndex); + const remainder = buffer.slice(newlineIndex + 1); + socket.pause(); + cleanup(); + if (remainder) socket.unshift(Buffer.from(remainder, 'utf8')); + resolve(line); + }; + socket.on('data', onData); + socket.once('error', onError); + socket.once('close', onClose); + socket.resume(); + }); +} + describe('Windows DPAPI worker protocol and lifecycle', () => { const roots: string[] = []; + const sockets: Socket[] = []; afterEach(() => { vi.useRealTimers(); + for (const socket of sockets.splice(0)) socket.destroy(); return Promise.all( roots.splice(0).map((root) => rm(root, { recursive: true, force: true })), ); }); - it('bounds the pipe connection phase, terminates the child, and passes only a parent PID watchdog input', async () => { + it('bounds the pipe connection phase, terminates the child, and keeps its authentication key out of argv and env', async () => { vi.useFakeTimers(); const root = await mkdtemp(path.join(tmpdir(), 'od-dpapi-worker-connect-')); roots.push(root); + const pipeAddress = createTestPipeAddress(root); const child = createFakeChild(); let observeSpawn: (() => void) | undefined; const spawned = new Promise((resolve) => { observeSpawn = resolve; }); + let authenticationInput = ''; + let spawnArguments: readonly string[] = []; let spawnEnvironment: NodeJS.ProcessEnv | undefined; const creating = WindowsDpapiWorker.create({ connectTimeoutMs: 10, - pipeAddress: { - connectionName: path.join(root, 'worker.sock'), - listenPath: path.join(root, 'worker.sock'), - }, - spawnWorker: (_command, _args, options) => { + pipeAddress, + spawnWorker: (_command, args, options) => { + spawnArguments = args; spawnEnvironment = options.env; + child.stdin.on('data', (chunk: Buffer) => { + authenticationInput += chunk.toString('utf8'); + }); observeSpawn?.(); return child; }, @@ -149,15 +216,100 @@ describe('Windows DPAPI worker protocol and lifecycle', () => { expect(child.kill).toHaveBeenCalledTimes(1); expect(spawnEnvironment?.OD_BYOK_DPAPI_PARENT_PID).toBe(String(process.pid)); - expect(spawnEnvironment?.OD_BYOK_DPAPI_PIPE_NAME).toBe( - path.join(root, 'worker.sock'), - ); + expect(spawnEnvironment?.OD_BYOK_DPAPI_PIPE_NAME).toBe(pipeAddress.connectionName); + expect(spawnEnvironment?.OD_BYOK_DPAPI_AUTH_KEY).toBeUndefined(); expect(JSON.stringify(spawnEnvironment)).not.toContain('secretPath'); + const authenticationKey = authenticationInput.trim(); + expect(Buffer.from(authenticationKey, 'base64')).toHaveLength(32); + expect(JSON.stringify([spawnArguments, spawnEnvironment])).not.toContain( + authenticationKey, + ); + }); + + it('rejects an impostor before readiness and sends credentials only to the authenticated worker', async () => { + const root = await mkdtemp(path.join(tmpdir(), 'od-dpapi-worker-auth-')); + roots.push(root); + const pipeAddress = createTestPipeAddress(root); + const child = createFakeChild(); + let authenticationInput = ''; + let resolveAuthenticationInput: ((value: string) => void) | undefined; + const authenticationInputReady = new Promise((resolve) => { + resolveAuthenticationInput = resolve; + }); + const creating = WindowsDpapiWorker.create({ + pipeAddress, + spawnWorker: () => { + child.stdin.on('data', (chunk: Buffer) => { + authenticationInput += chunk.toString('utf8'); + }); + child.stdin.once('end', () => { + resolveAuthenticationInput?.(authenticationInput.trim()); + }); + return child; + }, + }); + + const impostor = await connectSocket(pipeAddress.listenPath); + sockets.push(impostor); + const impostorChallengeLine = await readSocketLine(impostor); + expect(JSON.parse(impostorChallengeLine)).toMatchObject({ + type: 'challenge', + }); + const impostorClosed = once(impostor, 'close'); + impostor.write('{"type":"ready","ok":true}\n'); + await impostorClosed; + + const authenticationKey = Buffer.from( + await authenticationInputReady, + 'base64', + ); + expect(authenticationKey).toHaveLength(32); + const authenticatedPipe = await connectSocket(pipeAddress.listenPath); + sockets.push(authenticatedPipe); + const challengeEnvelope = JSON.parse( + await readSocketLine(authenticatedPipe), + ) as { challenge: string; type: string }; + const proof = createHmac('sha256', authenticationKey) + .update(Buffer.from(challengeEnvelope.challenge, 'base64')) + .digest('base64'); + authenticatedPipe.write( + `${JSON.stringify({ type: 'authenticate', proof })}\n` + + '{"type":"ready","ok":true}\n', + ); + + const worker = await creating; + await worker.ready(); + const operation = worker.run('set', '/secret/path', 'credential-value'); + const requestLine = await readSocketLine(authenticatedPipe); + const request = JSON.parse(requestLine) as { + id: string; + operation: string; + secret: string; + }; + expect(request.operation).toBe('set'); + expect(Buffer.from(request.secret, 'base64').toString('utf8')).toBe( + 'credential-value', + ); + expect(impostorChallengeLine).not.toContain('credential-value'); + authenticatedPipe.write(`${JSON.stringify({ + id: request.id, + ok: true, + found: true, + })}\n`); + await expect(operation).resolves.toEqual({ found: true, value: null }); + + authenticatedPipe.once('end', () => { + authenticatedPipe.end(); + child.emitClose(0); + }); + authenticatedPipe.resume(); + await worker.close(); }); it('aborts an in-progress connection immediately during daemon shutdown', async () => { const root = await mkdtemp(path.join(tmpdir(), 'od-dpapi-worker-abort-')); roots.push(root); + const pipeAddress = createTestPipeAddress(root); const child = createFakeChild(); const abortController = new AbortController(); let observeSpawn: (() => void) | undefined; @@ -166,10 +318,7 @@ describe('Windows DPAPI worker protocol and lifecycle', () => { }); const creating = WindowsDpapiWorker.create({ connectTimeoutMs: 50_000, - pipeAddress: { - connectionName: path.join(root, 'worker.sock'), - listenPath: path.join(root, 'worker.sock'), - }, + pipeAddress, signal: abortController.signal, spawnWorker: () => { observeSpawn?.();