From 8c6822cd8cfc131edd3469b08061f34123683171 Mon Sep 17 00:00:00 2001 From: qqqys Date: Tue, 18 Aug 2026 14:53:03 +0800 Subject: [PATCH 01/26] fix(serve): let channel workers reach TLS-enabled daemons The channel worker supervisor always handed workers an http:// loopback URL, and workers rejected any other scheme, so on a daemon started with --tls-cert/--tls-key the worker's first capabilities fetch hit the HTTPS-only listener as plain HTTP and died with "fetch failed" before reporting ready ("Channel worker exited before ready (code=1)"). - Emit an https:// loopback URL for the worker when TLS is configured - Accept https loopback in the worker's QWEN_DAEMON_URL validation - Inject NODE_EXTRA_CA_CERTS with the daemon cert into the worker env (merged with an operator-set value, since it accepts a single file) --- .../commands/channel/daemon-worker.test.ts | 42 ++++++++--- .../cli/src/commands/channel/daemon-worker.ts | 7 +- .../cli/src/serve/channel-worker-group.ts | 4 ++ .../serve/channel-worker-supervisor.test.ts | 69 +++++++++++++++++++ .../src/serve/channel-worker-supervisor.ts | 41 +++++++++++ packages/cli/src/serve/run-qwen-serve.test.ts | 9 +++ packages/cli/src/serve/run-qwen-serve.ts | 8 ++- 7 files changed, 167 insertions(+), 13 deletions(-) diff --git a/packages/cli/src/commands/channel/daemon-worker.test.ts b/packages/cli/src/commands/channel/daemon-worker.test.ts index 33b5bed08ca..8dcac421a31 100644 --- a/packages/cli/src/commands/channel/daemon-worker.test.ts +++ b/packages/cli/src/commands/channel/daemon-worker.test.ts @@ -1332,20 +1332,44 @@ describe('runChannelDaemonWorker', () => { ).rejects.toThrow('Channel "missing" not found in settings.'); }); - it('rejects daemon URLs that are not http loopback URLs', async () => { + it('rejects daemon URLs that are not http(s) loopback URLs', async () => { const sdk = createSdk(); - await expect( - runChannelDaemonWorker({ - daemonUrl: 'http://attacker.example:4170', - workspace: '/workspace', - selection: { mode: 'names', names: ['telegram'] }, - loadDaemonSdk: async () => sdk, - }), - ).rejects.toThrow('QWEN_DAEMON_URL must use an http loopback URL.'); + for (const daemonUrl of [ + 'http://attacker.example:4170', + 'https://attacker.example:4170', + ]) { + await expect( + runChannelDaemonWorker({ + daemonUrl, + workspace: '/workspace', + selection: { mode: 'names', names: ['telegram'] }, + loadDaemonSdk: async () => sdk, + }), + ).rejects.toThrow('QWEN_DAEMON_URL must use an http(s) loopback URL.'); + } expect(sdk.DaemonClient).not.toHaveBeenCalled(); }); + it('accepts https loopback daemon URLs for TLS daemons', async () => { + const sdk = createSdk(); + mockLoadChannelsConfig.mockReturnValueOnce({ + telegram: { type: 'telegram' }, + }); + mockParseConfiguredChannels.mockResolvedValueOnce([parsedTelegram]); + + await runChannelDaemonWorker({ + daemonUrl: 'https://127.0.0.1:4170', + workspace: '/workspace', + selection: { mode: 'all' }, + loadDaemonSdk: async () => sdk, + }); + + expect(sdk.DaemonClient).toHaveBeenCalledWith({ + baseUrl: 'https://127.0.0.1:4170', + }); + }); + it('fails fast when no channels are configured', async () => { const sdk = createSdk(); mockLoadChannelsConfig.mockReturnValueOnce({}); diff --git a/packages/cli/src/commands/channel/daemon-worker.ts b/packages/cli/src/commands/channel/daemon-worker.ts index d3a802a7113..1d3fc844947 100644 --- a/packages/cli/src/commands/channel/daemon-worker.ts +++ b/packages/cli/src/commands/channel/daemon-worker.ts @@ -324,8 +324,11 @@ function validateDaemonWorkerUrl(daemonUrl: string): void { } catch { throw new Error(`${QWEN_DAEMON_URL_ENV} must be a valid URL.`); } - if (parsed.protocol !== 'http:' || !isLoopbackBind(parsed.hostname)) { - throw new Error(`${QWEN_DAEMON_URL_ENV} must use an http loopback URL.`); + if ( + (parsed.protocol !== 'http:' && parsed.protocol !== 'https:') || + !isLoopbackBind(parsed.hostname) + ) { + throw new Error(`${QWEN_DAEMON_URL_ENV} must use an http(s) loopback URL.`); } } diff --git a/packages/cli/src/serve/channel-worker-group.ts b/packages/cli/src/serve/channel-worker-group.ts index f6549db1f18..f71012a92da 100644 --- a/packages/cli/src/serve/channel-worker-group.ts +++ b/packages/cli/src/serve/channel-worker-group.ts @@ -103,6 +103,7 @@ export interface ChannelWorkerGroupSharedOptions { cliEntryPath: string; daemonUrl: string; daemonToken?: string; + workerTlsCaCertPath?: string; restartPolicy?: ChannelWorkerRestartPolicy; startupTimeoutMs?: number; heartbeatTimeoutMs?: number; @@ -231,6 +232,9 @@ export function createChannelWorkerGroup( ...(opts.shared.daemonToken ? { daemonToken: opts.shared.daemonToken } : {}), + ...(opts.shared.workerTlsCaCertPath + ? { tlsCaCertPath: opts.shared.workerTlsCaCertPath } + : {}), workspace: runtime.workspaceCwd, selection: group.selection, // Multi-workspace runtimes expose a per-workspace env overlay; a diff --git a/packages/cli/src/serve/channel-worker-supervisor.test.ts b/packages/cli/src/serve/channel-worker-supervisor.test.ts index 918660ed58d..e3721b30edd 100644 --- a/packages/cli/src/serve/channel-worker-supervisor.test.ts +++ b/packages/cli/src/serve/channel-worker-supervisor.test.ts @@ -1,4 +1,7 @@ import { EventEmitter } from 'node:events'; +import * as fs from 'node:fs'; +import * as os from 'node:os'; +import * as path from 'node:path'; import { afterEach, describe, expect, it, vi } from 'vitest'; import type { ChannelWebhookTask } from '@qwen-code/channel-base'; import { @@ -277,6 +280,72 @@ describe('createChannelWorkerSupervisor', () => { ).toBe(false); }); + it('injects NODE_EXTRA_CA_CERTS when the daemon serves TLS', async () => { + const child = new FakeChild(); + const spawnWorker = vi.fn( + (_execPath: string, _argv: string[], _options: unknown) => child, + ); + const supervisor = createChannelWorkerSupervisor({ + cliEntryPath: '/repo/dist/index.js', + daemonUrl: 'https://127.0.0.1:4170', + tlsCaCertPath: '/certs/daemon.pem', + workspace: '/workspace', + selection: { mode: 'names', names: ['telegram'] }, + workerBaseEnv: {}, + spawnWorker, + }); + + const started = supervisor.start(); + child.emit('message', { + type: 'ready', + pid: 54321, + channels: ['telegram'], + requestedChannels: ['telegram'], + }); + await started; + + const env = (spawnWorker.mock.calls[0]![2] as { env: NodeJS.ProcessEnv }) + .env; + expect(env['NODE_EXTRA_CA_CERTS']).toBe('/certs/daemon.pem'); + }); + + it('merges an operator-set NODE_EXTRA_CA_CERTS with the daemon cert', async () => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'qwen-ca-merge-')); + const operatorCa = path.join(dir, 'operator.pem'); + const daemonCa = path.join(dir, 'daemon.pem'); + fs.writeFileSync(operatorCa, 'OP-CERT\n'); + fs.writeFileSync(daemonCa, 'DAEMON-CERT\n'); + const child = new FakeChild(); + const spawnWorker = vi.fn( + (_execPath: string, _argv: string[], _options: unknown) => child, + ); + const supervisor = createChannelWorkerSupervisor({ + cliEntryPath: '/repo/dist/index.js', + daemonUrl: 'https://127.0.0.1:4170', + tlsCaCertPath: daemonCa, + workspace: '/workspace', + selection: { mode: 'names', names: ['telegram'] }, + workerBaseEnv: { NODE_EXTRA_CA_CERTS: operatorCa }, + spawnWorker, + }); + + const started = supervisor.start(); + child.emit('message', { + type: 'ready', + pid: 54321, + channels: ['telegram'], + requestedChannels: ['telegram'], + }); + await started; + + const env = (spawnWorker.mock.calls[0]![2] as { env: NodeJS.ProcessEnv }) + .env; + const combined = fs.readFileSync(env['NODE_EXTRA_CA_CERTS']!, 'utf8'); + expect(combined).toContain('OP-CERT'); + expect(combined).toContain('DAEMON-CERT'); + fs.rmSync(dir, { recursive: true, force: true }); + }); + it('ignores non-ready IPC messages before the ready message', async () => { const child = new FakeChild(); const supervisor = createChannelWorkerSupervisor({ diff --git a/packages/cli/src/serve/channel-worker-supervisor.ts b/packages/cli/src/serve/channel-worker-supervisor.ts index 1de48a5a3c5..7850f85d200 100644 --- a/packages/cli/src/serve/channel-worker-supervisor.ts +++ b/packages/cli/src/serve/channel-worker-supervisor.ts @@ -1,6 +1,9 @@ import { fork } from 'node:child_process'; import type { ChildProcess } from 'node:child_process'; import { randomUUID } from 'node:crypto'; +import * as fs from 'node:fs'; +import * as os from 'node:os'; +import * as path from 'node:path'; import { channelSelectionNames } from './channel-selection.js'; import type { ServeChannelSelection } from './types.js'; import { @@ -222,6 +225,11 @@ export interface CreateChannelWorkerSupervisorOptions { * daemon base env. */ workerBaseEnv?: Readonly; + /** + * PEM cert the worker must additionally trust when calling the daemon + * over a self-signed TLS listener. Injected via NODE_EXTRA_CA_CERTS. + */ + tlsCaCertPath?: string; startupTimeoutMs?: number; spawnWorker?: SpawnChannelWorker; onExit?: (snapshot: ChannelWorkerSnapshot) => void; @@ -372,11 +380,37 @@ function hasObservedExit(snapshot: ChannelWorkerSnapshot): boolean { return snapshot.exitCode !== undefined || snapshot.signal !== undefined; } +const NODE_EXTRA_CA_CERTS_ENV = 'NODE_EXTRA_CA_CERTS'; + +function resolveWorkerCaCertPath( + daemonCertPath: string, + existing: string | undefined, +): string { + if (!existing || existing === daemonCertPath) return daemonCertPath; + try { + // NODE_EXTRA_CA_CERTS takes a single file; merge so an operator-set CA + // (e.g. corporate proxy) keeps working alongside the daemon cert. + const combined = [ + fs.readFileSync(existing, 'utf8').trimEnd(), + fs.readFileSync(daemonCertPath, 'utf8').trimEnd(), + ].join('\n'); + const combinedPath = path.join( + os.tmpdir(), + `qwen-worker-ca-${process.pid}.pem`, + ); + fs.writeFileSync(combinedPath, `${combined}\n`, { mode: 0o600 }); + return combinedPath; + } catch { + return daemonCertPath; + } +} + function createWorkerEnv(opts: { daemonUrl: string; daemonToken?: string; workspace: string; baseEnv?: Readonly; + tlsCaCertPath?: string; }): NodeJS.ProcessEnv { const env: NodeJS.ProcessEnv = { ...(opts.baseEnv ?? process.env) }; env['QWEN_CODE_NO_RELAUNCH'] = 'true'; @@ -384,6 +418,12 @@ function createWorkerEnv(opts: { // the ACP channel fallback reports channel=daemon in usage statistics // (see cli/src/config/acp-channel-fallback.ts). env['QWEN_CODE_SERVE'] = '1'; + if (opts.tlsCaCertPath) { + env[NODE_EXTRA_CA_CERTS_ENV] = resolveWorkerCaCertPath( + opts.tlsCaCertPath, + env[NODE_EXTRA_CA_CERTS_ENV], + ); + } env[CHANNEL_DAEMON_WORKER_SENTINEL] = randomUUID(); env[QWEN_DAEMON_URL_ENV] = opts.daemonUrl; env[QWEN_DAEMON_WORKSPACE_ENV] = opts.workspace; @@ -791,6 +831,7 @@ export function createChannelWorkerSupervisor( workspace: opts.workspace, ...(opts.daemonToken ? { daemonToken: opts.daemonToken } : {}), ...(opts.workerBaseEnv ? { baseEnv: opts.workerBaseEnv } : {}), + ...(opts.tlsCaCertPath ? { tlsCaCertPath: opts.tlsCaCertPath } : {}), }); const promptAuthorization = env[CHANNEL_DAEMON_WORKER_SENTINEL]!; registerChannelWorkerPromptAuthorization( diff --git a/packages/cli/src/serve/run-qwen-serve.test.ts b/packages/cli/src/serve/run-qwen-serve.test.ts index d61c0cc9add..b9a9de76d30 100644 --- a/packages/cli/src/serve/run-qwen-serve.test.ts +++ b/packages/cli/src/serve/run-qwen-serve.test.ts @@ -1133,6 +1133,15 @@ describe('formatChannelWorkerDaemonUrl', () => { ); expect(isLoopbackBind('127.0.0.2')).toBe(true); }); + + it('uses https when the daemon serves TLS', () => { + expect(formatChannelWorkerDaemonUrl('0.0.0.0', 4170, true)).toBe( + 'https://127.0.0.1:4170', + ); + expect(formatChannelWorkerDaemonUrl('::1', 4170, true)).toBe( + 'https://[::1]:4170', + ); + }); }); /** diff --git a/packages/cli/src/serve/run-qwen-serve.ts b/packages/cli/src/serve/run-qwen-serve.ts index 46754b9d0fa..7c3748311b3 100644 --- a/packages/cli/src/serve/run-qwen-serve.ts +++ b/packages/cli/src/serve/run-qwen-serve.ts @@ -670,7 +670,9 @@ function workspaceRuntimeEffectiveEnv( export function formatChannelWorkerDaemonUrl( host: string, port: number, + tls = false, ): string { + const scheme = tls ? 'https' : 'http'; const normalized = host.trim().toLowerCase(); if ( normalized === '' || @@ -678,9 +680,9 @@ export function formatChannelWorkerDaemonUrl( normalized === '::' || normalized === '[::]' ) { - return `http://127.0.0.1:${port}`; + return `${scheme}://127.0.0.1:${port}`; } - return `http://${formatHostForUrl(host)}:${port}`; + return `${scheme}://${formatHostForUrl(host)}:${port}`; } /** @@ -7122,8 +7124,10 @@ async function runQwenServeImpl( daemonUrl: formatChannelWorkerDaemonUrl( opts.hostname, actualPort, + tlsOptions !== undefined, ), ...(token ? { daemonToken: token } : {}), + ...(tlsOptions ? { workerTlsCaCertPath: opts.tlsCert } : {}), }, onReady: (snapshot) => { if (runtimeStartupError !== undefined) return; From 04c954dcb9ca4cc4dd6057868dcc94ada1c0ba8d Mon Sep 17 00:00:00 2001 From: qqqys Date: Tue, 18 Aug 2026 17:54:57 +0800 Subject: [PATCH 02/26] fix(serve): make the worker TLS trust injection actually establish trust MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 1 review found three ways the CA injection this PR adds silently fails to give channel workers a usable trust anchor (R1-1, R1-2, R1-3), plus the diagnosability and coverage gaps around it (R1-4..R1-8). - R1-1: `--tls-cert` was forwarded to the worker verbatim. Workers are forked with `cwd: opts.workspace`, so a relative path resolved against the worker's cwd instead of the daemon's, Node silently ignored the unloadable extra cert, and every handshake failed DEPTH_ZERO_SELF_SIGNED_CERT — the exact pre-PR symptom. Resolve once at the source, next to the read that already validated it. - R1-2: the merged CA bundle went to `os.tmpdir()/qwen-worker-ca-.pem`, a path predictable from the daemon PID (CWE-377/CWE-59). A pre-planted symlink redirected the write; a pre-planted regular file kept attacker ownership and mode while receiving the full cert — the private key too, for a combined PEM. Write into an `mkdtempSync` 0700 directory instead, the same defence standalone-update.ts already uses in this tmpdir. - R1-3/R1-4: a serving cert only anchors trust when it signed itself, and only reaches the worker when its SANs cover the loopback host workers dial. Neither held for the `mkcert` flow this project documents, and boot validation checked parse/expiry/validity-window only — so the daemon booted green, browsers connected, and every worker restart-looped with /health still green. `describeWorkerTlsTrustGaps` names both at boot, the way the adjacent expiry guard does. The non-self-signed check stays quiet when the operator set NODE_EXTRA_CA_CERTS, since that value is merged into the worker bundle and may already carry the issuing root. - R1-5: the merge-failure `catch` dropped the operator-set NODE_EXTRA_CA_CERTS with no diagnostic, and Node stays silent when the remaining cert loads fine. Emit a process warning naming both paths. - R1-6: the bundle was never cleaned up. Merged bundles are now memoized per (operator CA, daemon cert) pair — workers respawn on every restart, so minting a directory per spawn would leak one per restart — and removed on daemon exit. - R1-7/R1-8: tests for the merge-failure fallback and for the `workerTlsCaCertPath` pass-through, plus an end-to-end test that boots the daemon with a relative `--tls-cert` and asserts the supervisor gets an absolute path and an https daemon URL. Verification: every fix was mutation-checked — reverting `path.resolve`, the mkdtemp write, the trust-gap detection, the merge-failure warning, the group pass-through, and the bundle memoization each turns at least one new test red. `npx vitest run src/serve/run-qwen-serve.test.ts src/serve/channel-worker-supervisor.test.ts src/serve/channel-worker-group.test.ts` → 403 passed. eslint and prettier clean on the six touched files. --- .../src/serve/channel-worker-group.test.ts | 34 ++++ .../serve/channel-worker-supervisor.test.ts | 136 +++++++++++++++ .../src/serve/channel-worker-supervisor.ts | 49 +++++- packages/cli/src/serve/run-qwen-serve.test.ts | 163 ++++++++++++++++++ packages/cli/src/serve/run-qwen-serve.ts | 110 +++++++++++- 5 files changed, 480 insertions(+), 12 deletions(-) diff --git a/packages/cli/src/serve/channel-worker-group.test.ts b/packages/cli/src/serve/channel-worker-group.test.ts index ae308fd93a9..fa38e8da97c 100644 --- a/packages/cli/src/serve/channel-worker-group.test.ts +++ b/packages/cli/src/serve/channel-worker-group.test.ts @@ -141,6 +141,40 @@ const deliveryRequest: ChannelDeliveryRequest = { }; describe('createChannelWorkerGroup', () => { + it('passes workerTlsCaCertPath through to every supervisor', () => { + const registry = fakeRegistry([fakeRuntime(PRIMARY, true)]); + const { createSupervisor, recorded } = makeCreateSupervisor(() => + snapshot({}), + ); + createChannelWorkerGroup({ + groups: [ + { workspaceCwd: PRIMARY, selection: { mode: 'names', names: ['b'] } }, + ], + registry, + createSupervisor, + shared: { ...shared, workerTlsCaCertPath: '/certs/daemon.pem' }, + }); + + expect(recorded[0]!.opts.tlsCaCertPath).toBe('/certs/daemon.pem'); + }); + + it('omits tlsCaCertPath when the daemon does not serve TLS', () => { + const registry = fakeRegistry([fakeRuntime(PRIMARY, true)]); + const { createSupervisor, recorded } = makeCreateSupervisor(() => + snapshot({}), + ); + createChannelWorkerGroup({ + groups: [ + { workspaceCwd: PRIMARY, selection: { mode: 'names', names: ['b'] } }, + ], + registry, + createSupervisor, + shared, + }); + + expect(recorded[0]!.opts.tlsCaCertPath).toBeUndefined(); + }); + it('wires loop MCP to the exact workspace session with owner-safe cleanup', async () => { const addSessionRuntimeMcpServer = vi.fn(async () => ({ toolCount: 3 })); const removeSessionRuntimeMcpServer = vi.fn(async () => ({})); diff --git a/packages/cli/src/serve/channel-worker-supervisor.test.ts b/packages/cli/src/serve/channel-worker-supervisor.test.ts index e3721b30edd..adad8c10cc3 100644 --- a/packages/cli/src/serve/channel-worker-supervisor.test.ts +++ b/packages/cli/src/serve/channel-worker-supervisor.test.ts @@ -343,7 +343,143 @@ describe('createChannelWorkerSupervisor', () => { const combined = fs.readFileSync(env['NODE_EXTRA_CA_CERTS']!, 'utf8'); expect(combined).toContain('OP-CERT'); expect(combined).toContain('DAEMON-CERT'); + fs.rmSync(path.dirname(env['NODE_EXTRA_CA_CERTS']!), { + recursive: true, + force: true, + }); + fs.rmSync(dir, { recursive: true, force: true }); + }); + + it('reuses one merged bundle across worker spawns', async () => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'qwen-ca-reuse-')); + const operatorCa = path.join(dir, 'operator.pem'); + const daemonCa = path.join(dir, 'daemon.pem'); + fs.writeFileSync(operatorCa, 'OP-CERT\n'); + fs.writeFileSync(daemonCa, 'DAEMON-CERT\n'); + const spawnWorker = vi.fn( + (_execPath: string, _argv: string[], _options: unknown) => + new FakeChild(), + ); + const makeSupervisor = () => + createChannelWorkerSupervisor({ + cliEntryPath: '/repo/dist/index.js', + daemonUrl: 'https://127.0.0.1:4170', + tlsCaCertPath: daemonCa, + workspace: '/workspace', + selection: { mode: 'names', names: ['telegram'] }, + workerBaseEnv: { NODE_EXTRA_CA_CERTS: operatorCa }, + spawnWorker, + }); + + for (const supervisor of [makeSupervisor(), makeSupervisor()]) { + const started = supervisor.start(); + const child = spawnWorker.mock.results.at(-1)!.value as FakeChild; + child.emit('message', { + type: 'ready', + pid: 54321, + channels: ['telegram'], + requestedChannels: ['telegram'], + }); + await started; + } + + // Workers respawn on every restart; minting a fresh bundle directory per + // spawn would leak one per restart for the daemon's whole lifetime. + const paths = spawnWorker.mock.calls.map( + (call) => + (call[2] as { env: NodeJS.ProcessEnv }).env['NODE_EXTRA_CA_CERTS'], + ); + expect(paths).toHaveLength(2); + expect(paths[0]).toBe(paths[1]); + fs.rmSync(path.dirname(paths[0]!), { recursive: true, force: true }); + fs.rmSync(dir, { recursive: true, force: true }); + }); + + it('keeps the daemon cert when the operator CA cannot be merged', async () => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'qwen-ca-fallback-')); + const daemonCa = path.join(dir, 'daemon.pem'); + fs.writeFileSync(daemonCa, 'DAEMON-CERT\n'); + const warnings: string[] = []; + const onWarning = (warning: Error) => warnings.push(warning.message); + process.on('warning', onWarning); + const child = new FakeChild(); + const spawnWorker = vi.fn( + (_execPath: string, _argv: string[], _options: unknown) => child, + ); + const supervisor = createChannelWorkerSupervisor({ + cliEntryPath: '/repo/dist/index.js', + daemonUrl: 'https://127.0.0.1:4170', + tlsCaCertPath: daemonCa, + workspace: '/workspace', + selection: { mode: 'names', names: ['telegram'] }, + workerBaseEnv: { + NODE_EXTRA_CA_CERTS: path.join(dir, 'missing-operator.pem'), + }, + spawnWorker, + }); + + const started = supervisor.start(); + child.emit('message', { + type: 'ready', + pid: 54321, + channels: ['telegram'], + requestedChannels: ['telegram'], + }); + await started; + + const env = (spawnWorker.mock.calls[0]![2] as { env: NodeJS.ProcessEnv }) + .env; + expect(env['NODE_EXTRA_CA_CERTS']).toBe(daemonCa); + await new Promise((resolve) => setImmediate(resolve)); + process.off('warning', onWarning); + expect( + warnings.some((message) => message.includes('missing-operator.pem')), + ).toBe(true); + fs.rmSync(dir, { recursive: true, force: true }); + }); + + it('writes the merged bundle into a private directory, not a predictable tmp path', async () => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'qwen-ca-private-')); + const operatorCa = path.join(dir, 'operator.pem'); + const daemonCa = path.join(dir, 'daemon.pem'); + fs.writeFileSync(operatorCa, 'OP-CERT\n'); + fs.writeFileSync(daemonCa, 'DAEMON-CERT\n'); + const child = new FakeChild(); + const spawnWorker = vi.fn( + (_execPath: string, _argv: string[], _options: unknown) => child, + ); + const supervisor = createChannelWorkerSupervisor({ + cliEntryPath: '/repo/dist/index.js', + daemonUrl: 'https://127.0.0.1:4170', + tlsCaCertPath: daemonCa, + workspace: '/workspace', + selection: { mode: 'names', names: ['telegram'] }, + workerBaseEnv: { NODE_EXTRA_CA_CERTS: operatorCa }, + spawnWorker, + }); + + const started = supervisor.start(); + child.emit('message', { + type: 'ready', + pid: 54321, + channels: ['telegram'], + requestedChannels: ['telegram'], + }); + await started; + + const bundlePath = ( + spawnWorker.mock.calls[0]![2] as { env: NodeJS.ProcessEnv } + ).env['NODE_EXTRA_CA_CERTS']!; + // A pre-planted path is only exploitable when it is predictable; the + // bundle now lives in a 0700 mkdtemp directory with a random suffix. + expect(bundlePath).not.toBe( + path.join(os.tmpdir(), `qwen-worker-ca-${process.pid}.pem`), + ); + const bundleDir = path.dirname(bundlePath); + expect(path.dirname(bundleDir)).toBe(os.tmpdir()); + expect(fs.statSync(bundleDir).mode & 0o777).toBe(0o700); fs.rmSync(dir, { recursive: true, force: true }); + fs.rmSync(bundleDir, { recursive: true, force: true }); }); it('ignores non-ready IPC messages before the ready message', async () => { diff --git a/packages/cli/src/serve/channel-worker-supervisor.ts b/packages/cli/src/serve/channel-worker-supervisor.ts index 7850f85d200..aea781a3493 100644 --- a/packages/cli/src/serve/channel-worker-supervisor.ts +++ b/packages/cli/src/serve/channel-worker-supervisor.ts @@ -382,11 +382,42 @@ function hasObservedExit(snapshot: ChannelWorkerSnapshot): boolean { const NODE_EXTRA_CA_CERTS_ENV = 'NODE_EXTRA_CA_CERTS'; +/** + * Merged bundles keyed by `${operatorCaPath}\0${daemonCertPath}`. Workers are + * respawned on every restart, so without this the daemon would mint a fresh + * bundle directory per spawn and leak all of them. + */ +const mergedWorkerCaBundles = new Map(); + +function writeMergedWorkerCaBundle(contents: string): string { + // mkdtempSync gives a 0700 directory with a random suffix, so the bundle + // path cannot be pre-planted the way a fixed `qwen-worker-ca-.pem` in + // the shared tmpdir can (CWE-377/CWE-59: a pre-planted symlink redirects + // the write, a pre-planted regular file keeps attacker ownership and mode + // while receiving the cert — the private key too, for a combined PEM). + // Same defence as standalone-update.ts's extract dir. + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'qwen-worker-ca-')); + const bundlePath = path.join(dir, 'ca-bundle.pem'); + fs.writeFileSync(bundlePath, contents, { mode: 0o600 }); + // Nothing else references this directory, so the daemon owns its lifetime. + process.once('exit', () => { + try { + fs.rmSync(dir, { recursive: true, force: true }); + } catch { + // Best effort: the daemon is already exiting. + } + }); + return bundlePath; +} + function resolveWorkerCaCertPath( daemonCertPath: string, existing: string | undefined, ): string { if (!existing || existing === daemonCertPath) return daemonCertPath; + const cacheKey = `${existing}\0${daemonCertPath}`; + const cached = mergedWorkerCaBundles.get(cacheKey); + if (cached) return cached; try { // NODE_EXTRA_CA_CERTS takes a single file; merge so an operator-set CA // (e.g. corporate proxy) keeps working alongside the daemon cert. @@ -394,13 +425,19 @@ function resolveWorkerCaCertPath( fs.readFileSync(existing, 'utf8').trimEnd(), fs.readFileSync(daemonCertPath, 'utf8').trimEnd(), ].join('\n'); - const combinedPath = path.join( - os.tmpdir(), - `qwen-worker-ca-${process.pid}.pem`, + const bundlePath = writeMergedWorkerCaBundle(`${combined}\n`); + mergedWorkerCaBundles.set(cacheKey, bundlePath); + return bundlePath; + } catch (err) { + // Falling back to the daemon cert alone silently drops the operator CA + // the merge above exists to preserve, and Node says nothing when the + // remaining cert loads fine — so say it here. + process.emitWarning( + `qwen: failed to merge ${NODE_EXTRA_CA_CERTS_ENV} "${existing}" with ` + + `the daemon cert "${daemonCertPath}": ` + + `${err instanceof Error ? err.message : String(err)}; channel ` + + `workers will trust only the daemon cert`, ); - fs.writeFileSync(combinedPath, `${combined}\n`, { mode: 0o600 }); - return combinedPath; - } catch { return daemonCertPath; } } diff --git a/packages/cli/src/serve/run-qwen-serve.test.ts b/packages/cli/src/serve/run-qwen-serve.test.ts index b9a9de76d30..0dffcfc42dc 100644 --- a/packages/cli/src/serve/run-qwen-serve.test.ts +++ b/packages/cli/src/serve/run-qwen-serve.test.ts @@ -16,6 +16,7 @@ import { createLazyBridgeProxy, extractContextFilename, formatChannelWorkerDaemonUrl, + describeWorkerTlsTrustGaps, InvalidPolicyConfigError, createDisabledChannelWorkerSupervisor, createBoundChannelDeliveryHandler, @@ -1144,6 +1145,119 @@ describe('formatChannelWorkerDaemonUrl', () => { }); }); +// A CA-issued leaf, the shape the documented `mkcert` flow produces: usable +// as a serving cert, useless as a trust anchor. Not a real secret. +const TEST_TLS_CERT_CA_ISSUED = `-----BEGIN CERTIFICATE----- +MIIDHjCCAgagAwIBAgIUMfJwZrF6DjLX1ypLgu2A4v/SwKEwDQYJKoZIhvcNAQEL +BQAwHDEaMBgGA1UEAwwRcXdlbiB0ZXN0IHJvb3QgQ0EwIBcNMjYwODE4MDk0NzE4 +WhgPMjEyNjA3MjUwOTQ3MThaMBQxEjAQBgNVBAMMCWxvY2FsaG9zdDCCASIwDQYJ +KoZIhvcNAQEBBQADggEPADCCAQoCggEBAOff38zsoMq+oe2koKyZJ7aoGJC8CuAc +oYoLcJaWdp6yJaj5BpYeHAnQt8QCQZB86Fj1f3yuK6KwmGm3p49NrVJMl/T39CnK +ZAcIWATBw8mCWLFWlWhRgqrIQ5ka935m+z63gVhSQiCq2mNkAzm9I4UcbeAucSXn +Plk0Bc/CBUh5knrjxPEebicbCUaKteWnG3SBe5PjgP6DKZojd0VakmbrDhTW+yD4 +9LRqURfzvQZghA7stqErp+WJREKAaJbNNUEhGvRSwucIsah6u7OAbYP1IRaYBGDm +nlxaYBETRg0/3Kzx4SnPUuyx3uR6YP9MNuSzK5udCf39+iWSFCC+AnMCAwEAAaNe +MFwwGgYDVR0RBBMwEYcEfwAAAYIJbG9jYWxob3N0MB0GA1UdDgQWBBSItY/bpVFx +QRATvUzvo+JRFVpuyjAfBgNVHSMEGDAWgBRfCBabaBn4orvntHRiDcBU8W3vEzAN +BgkqhkiG9w0BAQsFAAOCAQEAjIiKztoj9JtpKfP2qSYsTe+4nvCZ1ZT4PtmXQMVp +lyHI02iH+NSSY92/ZdvGn2jBMzAFpVgJFlI6aZOne/qHI5qMf1RW7BfHBXza7wF6 +mdILIKRUYzm96o6IEuObE+QkSjRuA5OpLkObzGZLWfem0+fxnz0djbzeEBhHpP+b +VUUcl7r2wFb3+ClobIYS24Y+tWCl53XF+2YFNebECkA+19TivHPYgyywljyFNmzk +jCELOKOvOESV6kWBGUcrj8rcXoaF3BABInxZURGMRqWuivfYSjkGj65Trf2sVCXS +9mkiDfB/mYPvq3ODVYLvOjcxqPFsKaRA0Gw5Nm7WKGiOhg== +-----END CERTIFICATE----- +`; + +// Self-signed, but its only SAN is a name the worker never dials — the +// classic `openssl req -x509 -subj "/CN=localhost"` cert. Not a real secret. +const TEST_TLS_CERT_NO_LOOPBACK_SAN = `-----BEGIN CERTIFICATE----- +MIIDJzCCAg+gAwIBAgIUAVVYUcnN8DryJZGEaaVCTk+wO8EwDQYJKoZIhvcNAQEL +BQAwFDESMBAGA1UEAwwJbG9jYWxob3N0MCAXDTI2MDgxODA5NDcxOFoYDzIxMjYw +NzI1MDk0NzE4WjAUMRIwEAYDVQQDDAlsb2NhbGhvc3QwggEiMA0GCSqGSIb3DQEB +AQUAA4IBDwAwggEKAoIBAQC/B3++tHrPbzLk0vSJrIbqxM1PYAIlEnxc/Jz/PAkX +TH2ChYAqdAIUnUK18/WecgDAVUMNbuOh8+JjS2O/+eOwa9McMFBD9KLzwClkFQXY +i9w0EQ+SI8haXYQhHo931KW/dP6JaNLhAxmGuTsypbvxRmJ3PKnOwcDZZYZ4uHgj +DOVROEVTMrm+QUh1gfPZRStPFePUFLggcjmaWzF0Zyi5DX9KKMvTMrgaSKm5nHev +WYMK/tEDTh7ofJqt1a9RRscixQlhp/8GkP39uXB2xfQjzHuybK0lvTYHLK4WMjw2 +tjU3ClZSaZ2kgxN6/cPn6dPMZeZWEyaH11wa0DDmzOWZAgMBAAGjbzBtMB0GA1Ud +DgQWBBQ36jRlglSAhVatEXwAqTfbvDaT0zAfBgNVHSMEGDAWgBQ36jRlglSAhVat +EXwAqTfbvDaT0zAPBgNVHRMBAf8EBTADAQH/MBoGA1UdEQQTMBGCD2V4YW1wbGUu +aW52YWxpZDANBgkqhkiG9w0BAQsFAAOCAQEAYSXyw7t8KeTir/G94izDvKIvkOZW +DxmdDDFDDEeeyKIo0MRttJoHbcmYkSTLz2UcOKn1bnAgx3ZQWjAm3NdeKF7XSiwH +NQTyGw0OxvTtzCX72xtBhS8md+dstcQ20YGN8rIEEgkUUOZlwJkhfe9URLNsSbBX +dcAfcNrfExtg49r1kpwhKL6lXmAi3lNKBgHz6+oyhJpCVehCEtoE4pvwRFW9oyrB +gI/irGYXddbzWJQla/KPV53wn5nK6Ho4dY1Z76slnwMoufrLM1oUt1QKUeyOKsOD +8rRyH3UVlQkkJUGlQHPaJ+OU65xrNMkTLS7MdSQfJ1Eti4GyiR2P0vySrg== +-----END CERTIFICATE----- +`; + +describe('describeWorkerTlsTrustGaps', () => { + const daemonUrl = 'https://127.0.0.1:4170'; + + it('reports nothing for a self-signed cert covering the dialled host', () => { + expect( + describeWorkerTlsTrustGaps({ + cert: Buffer.from(TEST_TLS_CERT), + certPath: '/certs/daemon.pem', + daemonUrl, + }), + ).toEqual([]); + }); + + it('names the leaf-as-trust-anchor gap for a CA-issued cert', () => { + const gaps = describeWorkerTlsTrustGaps({ + cert: Buffer.from(TEST_TLS_CERT_CA_ISSUED), + certPath: '/certs/daemon.pem', + daemonUrl, + }); + expect(gaps).toHaveLength(1); + expect(gaps[0]).toContain('UNABLE_TO_VERIFY_LEAF_SIGNATURE'); + expect(gaps[0]).toContain('qwen test root CA'); + }); + + it('stays quiet about a CA-issued cert when the operator supplies a CA', () => { + expect( + describeWorkerTlsTrustGaps({ + cert: Buffer.from(TEST_TLS_CERT_CA_ISSUED), + certPath: '/certs/daemon.pem', + daemonUrl, + operatorCaCertPath: '/certs/rootCA.pem', + }), + ).toEqual([]); + }); + + it('names the SAN gap when the cert does not cover the dialled host', () => { + const gaps = describeWorkerTlsTrustGaps({ + cert: Buffer.from(TEST_TLS_CERT_NO_LOOPBACK_SAN), + certPath: '/certs/daemon.pem', + daemonUrl, + }); + expect(gaps).toHaveLength(1); + expect(gaps[0]).toContain('ERR_TLS_CERT_ALTNAME_INVALID'); + expect(gaps[0]).toContain('127.0.0.1'); + }); + + it('checks the host actually dialled, not a fixed loopback literal', () => { + expect( + describeWorkerTlsTrustGaps({ + cert: Buffer.from(TEST_TLS_CERT_NO_LOOPBACK_SAN), + certPath: '/certs/daemon.pem', + daemonUrl: 'https://example.invalid:4170', + }), + ).toEqual([]); + }); + + it('defers to the boot parse guard on an unreadable certificate', () => { + expect( + describeWorkerTlsTrustGaps({ + cert: Buffer.from('not a certificate'), + certPath: '/certs/daemon.pem', + daemonUrl, + }), + ).toEqual([]); + }); +}); + /** * Wenshao review #4335 / 3272493818 — positive tests for the * `validatePolicyConfig` helper. Lock the contract so a future @@ -8907,6 +9021,55 @@ describe('runQwenServe channel worker supervisor', () => { } satisfies Partial); }); + it('hands workers an absolute --tls-cert path and an https daemon url', async () => { + // Workers are forked with `cwd: opts.workspace`, so a relative + // --tls-cert would resolve against the worker's cwd, load nothing, and + // fail every handshake with DEPTH_ZERO_SELF_SIGNED_CERT. + tmpDir = fs.realpathSync( + fs.mkdtempSync(path.join(os.tmpdir(), 'qws-channel-tls-')), + ); + const certPath = path.join(tmpDir, 'cert.pem'); + const keyPath = path.join(tmpDir, 'key.pem'); + fs.writeFileSync(certPath, TEST_TLS_CERT); + fs.writeFileSync(keyPath, TEST_TLS_KEY); + const relativeCert = path.relative(process.cwd(), certPath); + expect(path.isAbsolute(relativeCert)).toBe(false); + const worker = makeWorker({ + enabled: true, + state: 'running', + pid: 1234, + channels: ['telegram'], + }); + const factory = makeReadyWorkerFactory(worker); + + const handle = await runQwenServe( + { + port: 0, + hostname: '127.0.0.1', + mode: 'http-bridge', + workspace: tmpDir, + serveWebShell: false, + tlsCert: relativeCert, + tlsKey: path.relative(process.cwd(), keyPath), + channelSelection: { mode: 'names', names: ['telegram'] }, + }, + { + bridge: makeFakeBridge(), + channelWorkerSupervisorFactory: factory, + channelServicePidfile: makePidfileDeps(), + }, + ); + + try { + await handle.runtimeReady; + const opts = factory.mock.calls[0]![0]; + expect(opts.tlsCaCertPath).toBe(certPath); + expect(opts.daemonUrl).toMatch(/^https:\/\//); + } finally { + await handle.close(); + } + }); + it('forwards webhook tasks through the channel worker group', async () => { tmpDir = fs.realpathSync( fs.mkdtempSync(path.join(os.tmpdir(), 'qws-channel-webhook-')), diff --git a/packages/cli/src/serve/run-qwen-serve.ts b/packages/cli/src/serve/run-qwen-serve.ts index 7c3748311b3..8417c59cabe 100644 --- a/packages/cli/src/serve/run-qwen-serve.ts +++ b/packages/cli/src/serve/run-qwen-serve.ts @@ -8,6 +8,7 @@ import { X509Certificate, createHash, timingSafeEqual } from 'node:crypto'; import * as fs from 'node:fs'; import { createServer, type Server } from 'node:http'; import * as https from 'node:https'; +import { isIP } from 'node:net'; import * as path from 'node:path'; import * as os from 'node:os'; import { monitorEventLoopDelay, performance } from 'node:perf_hooks'; @@ -685,6 +686,85 @@ export function formatChannelWorkerDaemonUrl( return `${scheme}://${formatHostForUrl(host)}:${port}`; } +/** + * Two TLS misconfigurations boot green and break only the channel workers: + * a serving cert that is not its own trust anchor (workers fail + * `UNABLE_TO_VERIFY_LEAF_SIGNATURE`) and one whose SANs do not cover the + * loopback host workers dial (`ERR_TLS_CERT_ALTNAME_INVALID`). In both cases + * the daemon listens, browsers connect and `/health` stays green while every + * worker restart-loops, so name them at boot the way the expiry guard does. + */ +export function describeWorkerTlsTrustGaps(opts: { + cert: Buffer; + certPath: string; + daemonUrl: string; + operatorCaCertPath?: string; +}): string[] { + let x509: X509Certificate; + try { + x509 = new X509Certificate(opts.cert); + } catch { + // Boot validation already rejected unparseable certs with a better message. + return []; + } + const gaps: string[] = []; + // A leaf in NODE_EXTRA_CA_CERTS is a usable trust anchor only when it signed + // itself: chain verification has no PARTIAL_CHAIN flag here, so a CA-issued + // leaf (what the `mkcert` flow this project documents produces) never + // terminates the chain. An operator-set NODE_EXTRA_CA_CERTS is merged into + // the worker bundle and may already carry the issuing root, so only flag the + // case where the leaf is all the worker gets. + if (!opts.operatorCaCertPath && !isSelfSignedCert(x509)) { + gaps.push( + `--tls-cert "${opts.certPath}" is issued by another CA ` + + `(${x509.issuer.replace(/\r?\n/g, ', ')}), not self-signed, so the ` + + `certificate alone cannot anchor the channel workers' trust — every ` + + `worker handshake to the daemon will fail ` + + `UNABLE_TO_VERIFY_LEAF_SIGNATURE. Point NODE_EXTRA_CA_CERTS at the ` + + `issuing CA (for mkcert: "$(mkcert -CAROOT)/rootCA.pem") and restart.`, + ); + } + const host = workerDialHost(opts.daemonUrl); + if (host && !certCoversHost(x509, host)) { + gaps.push( + `--tls-cert "${opts.certPath}" has no subjectAltName covering ` + + `"${host}", the host channel workers dial — every worker handshake ` + + `will fail ERR_TLS_CERT_ALTNAME_INVALID. Reissue the certificate ` + + `with that host in its SANs and restart.`, + ); + } + return gaps; +} + +function isSelfSignedCert(x509: X509Certificate): boolean { + try { + return x509.verify(x509.publicKey); + } catch { + // Unsupported key type: assume self-signed rather than warn on a guess. + return true; + } +} + +function workerDialHost(daemonUrl: string): string | undefined { + try { + return new URL(daemonUrl).hostname || undefined; + } catch { + return undefined; + } +} + +function certCoversHost(x509: X509Certificate, host: string): boolean { + try { + // IP literals need an iPAddress SAN — checkServerIdentity has no CN + // fallback for them — while names go through the normal host match. + return isIP(host) + ? Boolean(x509.checkIP(host)) + : Boolean(x509.checkHost(host)); + } catch { + return true; + } +} + /** * Pull the `context.fileName` snapshot out of merged settings into a * typed string, falling back to `undefined` when the value is missing @@ -2417,6 +2497,7 @@ async function runQwenServeImpl( // downgrade would serve the web shell over an insecure transport they // believe is encrypted. let tlsOptions: { cert: Buffer; key: Buffer } | undefined; + let tlsCertPath: string | undefined; if ((opts.tlsCert && !opts.tlsKey) || (!opts.tlsCert && opts.tlsKey)) { throw new Error( `--tls-cert and --tls-key must be provided together (got only ` + @@ -2475,6 +2556,10 @@ async function runQwenServeImpl( ); } tlsOptions = { cert, key }; + // Workers are forked with `cwd: opts.workspace`, so a relative --tls-cert + // would resolve against the worker's cwd instead of the daemon's and load + // nothing. Resolve once here, against the cwd the daemon just read it with. + tlsCertPath = path.resolve(opts.tlsCert); } if (!isLoopbackBind(opts.hostname) && !token) { @@ -7111,6 +7196,23 @@ async function runQwenServeImpl( ); } const workerRuntime = await ensureChannelRuntime(); + const workerDaemonUrl = formatChannelWorkerDaemonUrl( + opts.hostname, + actualPort, + tlsOptions !== undefined, + ); + if (tlsOptions && tlsCertPath) { + for (const gap of describeWorkerTlsTrustGaps({ + cert: tlsOptions.cert, + certPath: tlsCertPath, + daemonUrl: workerDaemonUrl, + ...(process.env['NODE_EXTRA_CA_CERTS'] + ? { operatorCaCertPath: process.env['NODE_EXTRA_CA_CERTS'] } + : {}), + })) { + daemonLog.warn(gap); + } + } const createSupervisor = deps.channelWorkerSupervisorFactory ?? workerRuntime.createChannelWorkerSupervisor; @@ -7121,13 +7223,9 @@ async function runQwenServeImpl( createSupervisor, shared: { cliEntryPath: workerRuntime.findCliEntryPath(), - daemonUrl: formatChannelWorkerDaemonUrl( - opts.hostname, - actualPort, - tlsOptions !== undefined, - ), + daemonUrl: workerDaemonUrl, ...(token ? { daemonToken: token } : {}), - ...(tlsOptions ? { workerTlsCaCertPath: opts.tlsCert } : {}), + ...(tlsCertPath ? { workerTlsCaCertPath: tlsCertPath } : {}), }, onReady: (snapshot) => { if (runtimeStartupError !== undefined) return; From 06f6b8d90540adc391b3773fe7e0f50b05cc603a Mon Sep 17 00:00:00 2001 From: qqqys Date: Tue, 18 Aug 2026 18:21:17 +0800 Subject: [PATCH 03/26] test(serve): declare the worker TLS trust check's NODE_EXTRA_CA_CERTS reads The `Test (ubuntu-latest, Node 22.x)` job failed on 04c954dcb9 with a single red test: `serve process.env guard > allows only documented process-scoped process.env expressions`. 04c954dcb9 added the worker TLS trust-gap check, which reads `process.env['NODE_EXTRA_CA_CERTS']` twice in run-qwen-serve.ts (once to test for it, once to pass it), but did not add the matching entry to `allowedProcessEnvAccesses`. The guard is an explicit allowlist, so any undeclared process-scoped read is a failure by design. Declare `key:NODE_EXTRA_CA_CERTS: 2` and record why this particular read is process-scoped rather than request-scoped: NODE_EXTRA_CA_CERTS is the trust store Node already loaded for this process, so the check has to consult the same value to know whether the operator has already supplied the issuing CA. Mutation-verified: with the count at 1 instead of 2 the guard test goes red with the same mismatch shape, so the allowlist is genuinely counting the occurrences and not just matching the key. --- packages/cli/src/serve/process-env-guard.test.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/packages/cli/src/serve/process-env-guard.test.ts b/packages/cli/src/serve/process-env-guard.test.ts index dcc7eedfada..2568a170165 100644 --- a/packages/cli/src/serve/process-env-guard.test.ts +++ b/packages/cli/src/serve/process-env-guard.test.ts @@ -146,7 +146,10 @@ const allowedProcessEnvAccesses = normalizeAllowances([ 'packages/cli/src/serve/run-qwen-serve.ts', { reason: - 'The serve entry point owns daemon bootstrap, feature flags, child-process defaults, and the launch-env loader scrub.', + 'The serve entry point owns daemon bootstrap, feature flags, child-process defaults, and the launch-env loader scrub. ' + + 'NODE_EXTRA_CA_CERTS is read from the daemon process environment on purpose: it is the trust store Node itself ' + + 'already loaded for this process, so the worker TLS trust-gap check has to consult the same value to know whether ' + + 'an operator has already supplied the issuing CA.', accesses: { 'computed:EXTERNAL_TOOL_GUARD_TOKEN_ENV': 1, 'computed:QWEN_SERVER_TOKEN_ENV': 1, @@ -156,6 +159,7 @@ const allowedProcessEnvAccesses = normalizeAllowances([ 'computed:QWEN_SERVE_WRITER_IDLE_TIMEOUT_MS_ENV': 1, 'computed:RUNTIME_STARTUP_TIMEOUT_ENV': 1, 'key:DEV': 1, + 'key:NODE_EXTRA_CA_CERTS': 2, 'key:QWEN_CODE_IDE_WORKSPACE_PATH': 1, 'key:QWEN_SERVE_NO_MCP_POOL': 1, 'key:QWEN_SERVE_NO_PERSISTENT_REGISTRATION': 1, From c1161a736cd22eead87b75ea60e1fe7b818f4fbf Mon Sep 17 00:00:00 2001 From: qqqys Date: Tue, 18 Aug 2026 18:46:04 +0800 Subject: [PATCH 04/26] feat(serve): hide workspace Browse on headless daemon hosts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Web Shell "Add workspace" dialog shows a Browse button that opens a native OS directory picker on the daemon host (osascript on macOS, PowerShell on Windows, zenity on Linux). On headless hosts the picker can never open — zenity exits with "cannot open display" — so the button only surfaces a guaranteed error toast on every click. - Add isNativeDirectoryPickerAvailable: macOS/Windows always pass; Linux requires DISPLAY/WAYLAND_DISPLAY plus an executable zenity file on PATH (a directory named zenity does not count) - Advertise a new conditional native_directory_picker serve feature only when the probe passes; the bootstrap reduced set omits it like its sibling registration tags, which fails closed - Web Shell passes onPick to the Add workspace dialog only when the feature is present, so the button is hidden on headless hosts - Tolerate the host-conditional tag in the integration capabilities snapshot and register the new process.env access with the guard --- .gitignore | 3 + docs/developers/qwen-serve-protocol.md | 1 + .../cli/qwen-serve-routes.test.ts | 7 ++ packages/cli/src/serve/capabilities.ts | 10 +++ .../src/serve/native-directory-picker.test.ts | 82 ++++++++++++++++++- .../cli/src/serve/native-directory-picker.ts | 30 +++++++ .../cli/src/serve/process-env-guard.test.ts | 8 ++ packages/cli/src/serve/server.test.ts | 23 ++++++ packages/cli/src/serve/server.ts | 2 + .../cli/src/serve/server/serve-features.ts | 3 + packages/web-shell/client/App.test.tsx | 27 ++++++ packages/web-shell/client/App.tsx | 18 +++- 12 files changed, 208 insertions(+), 6 deletions(-) diff --git a/.gitignore b/.gitignore index d86a31bfb61..0ef32bd87c3 100644 --- a/.gitignore +++ b/.gitignore @@ -139,3 +139,6 @@ tmp/ # Auto-generated computer-use marker can also appear under nested packages. **/.qwen/computer-use/ .playwright-mcp/ + +# Tool state written to the repo root when $HOME is unset (e.g. gh CLI) +.local/ diff --git a/docs/developers/qwen-serve-protocol.md b/docs/developers/qwen-serve-protocol.md index 12b5e1fd363..f020aa23be5 100644 --- a/docs/developers/qwen-serve-protocol.md +++ b/docs/developers/qwen-serve-protocol.md @@ -480,6 +480,7 @@ operator diagnostic snapshot documented below. | `persistent_workspace_registration` | a workspace registration store is wired into the daemon. Production `runQwenServe` supplies the user-level store automatically; direct `createServeApp` embeds must inject one explicitly and own startup restoration of their workspace registry. | | `scratch_workspace_registration` | managed scratch workspace creation is available — a runtime factory, a validated managed scratch root, and runtime disposal are wired, and every managed runtime respects the scratch root boundary. | | `workspace_runtime_removal` | removable dynamic or persistence-restored secondary runtimes can be drained and removed through the management route. | +| `native_directory_picker` | the daemon host can open a native OS directory picker (`osascript` on macOS, PowerShell on Windows, `zenity` on a Linux host with a display). Headless hosts omit the tag so clients hide the Browse affordance instead of surfacing a guaranteed picker failure. | | `workspace_qualified_acp` | ACP HTTP and multi-workspace runtimes are active, so the plural ACP endpoint can select a secondary runtime. | | `workspace_qualified_voice` | multi-workspace runtimes and the shared ACP/Voice WebSocket listener are active, so every workspace-qualified Voice modality is reachable for a secondary runtime. | | `workspace_qualified_memory` | ACP HTTP and multi-workspace runtimes are active, so workspace-qualified managed-memory routes can select a per-workspace task lane for remember, forget, and dream operations. | diff --git a/integration-tests/cli/qwen-serve-routes.test.ts b/integration-tests/cli/qwen-serve-routes.test.ts index 0c4a6993ffb..7ad40fd8992 100644 --- a/integration-tests/cli/qwen-serve-routes.test.ts +++ b/integration-tests/cli/qwen-serve-routes.test.ts @@ -40,6 +40,7 @@ import { Storage, type ChatRecord, } from '@qwen-code/qwen-code-core'; +import { isNativeDirectoryPickerAvailable } from '../../packages/cli/src/serve/native-directory-picker.js'; const __dirname = path.dirname(fileURLToPath(import.meta.url)); // Match the rest of the integration suite: prefer the bundled CLI @@ -293,6 +294,9 @@ describe('qwen serve — capabilities envelope', () => { // `require_auth`, `allow_origin`, `cdp_tunnel_over_ws`, // `prompt_absolute_deadline`, `writer_idle_timeout`, // `workspace_voice_transcription`, `rate_limit`, `channel_reload`. + // `native_directory_picker` is host-conditional (the daemon host's GUI + // environment, not a spawn flag) and is spliced at its registry + // position below. // Pool tags (`mcp_workspace_pool`, `mcp_pool_restart`) ARE present // because the workspace MCP pool is on by default, as are // `workspace_settings`, `workspace_permissions`, `workspace_voice`, @@ -402,6 +406,9 @@ describe('qwen serve — capabilities envelope', () => { 'persistent_workspace_registration', 'workspace_display_name', 'workspace_runtime_removal', + ...(isNativeDirectoryPickerAvailable() + ? ['native_directory_picker'] + : []), 'workspace_qualified_rest_core', 'extension_management_v2', 'workspace_persisted_transcript', diff --git a/packages/cli/src/serve/capabilities.ts b/packages/cli/src/serve/capabilities.ts index 7af94d5f788..5f2a888274f 100644 --- a/packages/cli/src/serve/capabilities.ts +++ b/packages/cli/src/serve/capabilities.ts @@ -354,6 +354,11 @@ export const SERVE_CAPABILITY_REGISTRY = { workspace_display_name: { since: 'v1' }, scratch_workspace_registration: { since: 'v1' }, workspace_runtime_removal: { since: 'v1' }, + // A native OS directory picker can be opened on the daemon host + // (osascript on macOS, PowerShell on Windows, zenity on a Linux host + // with a display). Headless hosts omit the tag so clients hide the + // Browse affordance instead of surfacing a guaranteed picker failure. + native_directory_picker: { since: 'v1' }, // Workspace-qualified core REST routes under `/workspaces/:workspace/...`. // Covers core file read/write/upload, status/permissions/trust/lifecycle/MCP/tool, // memory, workspace agent CRUD, and persisted session organization surfaces. @@ -495,6 +500,7 @@ export interface AdvertiseFeatureToggles { persistentWorkspaceRegistrationAvailable?: boolean; scratchWorkspaceRegistrationAvailable?: boolean; workspaceRuntimeRemovalAvailable?: boolean; + nativeDirectoryPickerAvailable?: boolean; /** * Whether the HTTP ACP surface is enabled (default on; opts out via * QWEN_SERVE_ACP_HTTP=0). Workspace-qualified ACP is only advertised when on. @@ -624,6 +630,10 @@ export const CONDITIONAL_SERVE_FEATURES: ReadonlyMap< 'workspace_runtime_removal', (toggles) => toggles.workspaceRuntimeRemovalAvailable === true, ], + [ + 'native_directory_picker', + (toggles) => toggles.nativeDirectoryPickerAvailable === true, + ], [ 'workspace_qualified_acp', // The plural routes are pre-mounted for workspaces registered after app diff --git a/packages/cli/src/serve/native-directory-picker.test.ts b/packages/cli/src/serve/native-directory-picker.test.ts index 2b0bec1676a..375e0e98e67 100644 --- a/packages/cli/src/serve/native-directory-picker.test.ts +++ b/packages/cli/src/serve/native-directory-picker.test.ts @@ -4,6 +4,9 @@ * SPDX-License-Identifier: Apache-2.0 */ +import { chmodSync, mkdirSync, mkdtempSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; const { execFileAsyncMock } = vi.hoisted(() => ({ @@ -22,8 +25,21 @@ vi.mock('node:child_process', async (importOriginal) => { }; }); -const { pickNativeDirectory, NativeDirectoryPickerUnavailableError } = - await import('./native-directory-picker.js'); +const { + pickNativeDirectory, + isNativeDirectoryPickerAvailable, + NativeDirectoryPickerUnavailableError, +} = await import('./native-directory-picker.js'); + +const zenityDir = mkdtempSync(join(tmpdir(), 'picker-zenity-')); +writeFileSync(join(zenityDir, 'zenity'), '#!/bin/sh\nexit 0\n'); +chmodSync(join(zenityDir, 'zenity'), 0o755); +const emptyDir = mkdtempSync(join(tmpdir(), 'picker-empty-')); +const nonExecutableZenityDir = mkdtempSync(join(tmpdir(), 'picker-nonexec-')); +writeFileSync(join(nonExecutableZenityDir, 'zenity'), '#!/bin/sh\nexit 0\n'); +chmodSync(join(nonExecutableZenityDir, 'zenity'), 0o644); +const zenityDirectoryEntryDir = mkdtempSync(join(tmpdir(), 'picker-subdir-')); +mkdirSync(join(zenityDirectoryEntryDir, 'zenity')); function setPlatform(platform: NodeJS.Platform) { vi.spyOn(process, 'platform', 'get').mockReturnValue(platform); @@ -189,3 +205,65 @@ describe('pickNativeDirectory', () => { ); }); }); + +describe('isNativeDirectoryPickerAvailable', () => { + it('is available on macOS and Windows regardless of env', () => { + setPlatform('darwin'); + expect(isNativeDirectoryPickerAvailable({})).toBe(true); + setPlatform('win32'); + expect(isNativeDirectoryPickerAvailable({})).toBe(true); + }); + + it('is unavailable on unsupported platforms', () => { + setPlatform('aix'); + expect( + isNativeDirectoryPickerAvailable({ DISPLAY: ':0', PATH: zenityDir }), + ).toBe(false); + }); + + it('requires a display on Linux', () => { + setPlatform('linux'); + expect(isNativeDirectoryPickerAvailable({ PATH: zenityDir })).toBe(false); + }); + + it('requires an executable zenity on PATH on Linux', () => { + setPlatform('linux'); + expect( + isNativeDirectoryPickerAvailable({ DISPLAY: ':0', PATH: emptyDir }), + ).toBe(false); + expect( + isNativeDirectoryPickerAvailable({ DISPLAY: ':0', PATH: zenityDir }), + ).toBe(true); + expect( + isNativeDirectoryPickerAvailable({ + WAYLAND_DISPLAY: 'wayland-0', + PATH: zenityDir, + }), + ).toBe(true); + }); + + it('rejects a zenity without the executable bit on Linux', () => { + setPlatform('linux'); + expect( + isNativeDirectoryPickerAvailable({ + DISPLAY: ':0', + PATH: nonExecutableZenityDir, + }), + ).toBe(false); + }); + + it('rejects a directory named zenity on PATH on Linux', () => { + setPlatform('linux'); + expect( + isNativeDirectoryPickerAvailable({ + DISPLAY: ':0', + PATH: zenityDirectoryEntryDir, + }), + ).toBe(false); + }); + + it('requires PATH to be set on Linux', () => { + setPlatform('linux'); + expect(isNativeDirectoryPickerAvailable({ DISPLAY: ':0' })).toBe(false); + }); +}); diff --git a/packages/cli/src/serve/native-directory-picker.ts b/packages/cli/src/serve/native-directory-picker.ts index 715e0d2aa5e..a6ea0b787d0 100644 --- a/packages/cli/src/serve/native-directory-picker.ts +++ b/packages/cli/src/serve/native-directory-picker.ts @@ -5,6 +5,8 @@ */ import { execFile } from 'node:child_process'; +import { accessSync, constants, statSync } from 'node:fs'; +import { delimiter, join } from 'node:path'; import { promisify } from 'node:util'; const execFileAsync = promisify(execFile); @@ -16,6 +18,34 @@ const PICKER_TIMEOUT_MS = 300_000; export class NativeDirectoryPickerUnavailableError extends Error {} +// Startup probe so `/capabilities` can omit the picker feature on headless +// hosts and clients hide the Browse affordance instead of surfacing a +// guaranteed `cannot open display` failure. +export function isNativeDirectoryPickerAvailable( + env: Readonly> = process.env, +): boolean { + if (process.platform === 'darwin' || process.platform === 'win32') { + return true; + } + if (process.platform !== 'linux') return false; + if (!env['DISPLAY'] && !env['WAYLAND_DISPLAY']) return false; + return (env['PATH'] ?? '') + .split(delimiter) + .some((dir) => dir !== '' && isExecutableFile(join(dir, 'zenity'))); +} + +function isExecutableFile(file: string): boolean { + try { + // A directory passes an X_OK probe (search permission) but cannot be + // exec'd, so it must not count as an installed zenity. + if (!statSync(file).isFile()) return false; + accessSync(file, constants.X_OK); + return true; + } catch { + return false; + } +} + export async function pickNativeDirectory( signal?: AbortSignal, ): Promise { diff --git a/packages/cli/src/serve/process-env-guard.test.ts b/packages/cli/src/serve/process-env-guard.test.ts index dcc7eedfada..24d16db8d2f 100644 --- a/packages/cli/src/serve/process-env-guard.test.ts +++ b/packages/cli/src/serve/process-env-guard.test.ts @@ -142,6 +142,14 @@ const allowedProcessEnvAccesses = normalizeAllowances([ accesses: { 'key:QWEN_AUDIT_RAW_PATHS': 1 }, }, ], + [ + 'packages/cli/src/serve/native-directory-picker.ts', + { + reason: + 'The native directory picker availability probe defaults to the daemon process environment when no explicit environment is supplied.', + accesses: { whole: 1 }, + }, + ], [ 'packages/cli/src/serve/run-qwen-serve.ts', { diff --git a/packages/cli/src/serve/server.test.ts b/packages/cli/src/serve/server.test.ts index 8291229f78c..dd0c12c7cd1 100644 --- a/packages/cli/src/serve/server.test.ts +++ b/packages/cli/src/serve/server.test.ts @@ -66,6 +66,7 @@ import { SERVE_CAPABILITY_REGISTRY, type ServeProtocolVersion, } from './capabilities.js'; +import { isNativeDirectoryPickerAvailable } from './native-directory-picker.js'; import type { CancelNotification, PromptRequest, @@ -754,6 +755,7 @@ const EXPECTED_REGISTERED_FEATURES = [ 'workspace_display_name', 'scratch_workspace_registration', 'workspace_runtime_removal', + 'native_directory_picker', 'workspace_qualified_rest_core', 'workspace_qualified_voice', 'workspace_qualified_memory', @@ -3053,6 +3055,24 @@ describe('createServeApp', () => { ); continue; } + if (feature === 'native_directory_picker') { + expect(predicate({ nativeDirectoryPickerAvailable: true })).toBe( + true, + ); + expect(predicate({ nativeDirectoryPickerAvailable: false })).toBe( + false, + ); + expect(predicate({})).toBe(false); + expect( + getAdvertisedServeFeatures(undefined, { + nativeDirectoryPickerAvailable: true, + }), + ).toContain(feature); + expect(getAdvertisedServeFeatures(undefined, {})).not.toContain( + feature, + ); + continue; + } if (feature === 'workspace_trust_hot_reload') { expect(predicate({ workspaceTrustHotReloadAvailable: true })).toBe( true, @@ -3899,6 +3919,9 @@ describe('createServeApp', () => { sessionGenerationAvailable: true, workspaceGenerationAvailable: true, acpHttpEnabled: true, + // Mirror the server.ts probe so the expectation matches on both + // GUI and headless hosts. + nativeDirectoryPickerAvailable: isNativeDirectoryPickerAvailable(), }), ); expect(res.body.modelServices).toEqual([]); diff --git a/packages/cli/src/serve/server.ts b/packages/cli/src/serve/server.ts index a462864133b..a1204efcc7b 100644 --- a/packages/cli/src/serve/server.ts +++ b/packages/cli/src/serve/server.ts @@ -229,6 +229,7 @@ import { type WorkspaceManagementHandle, type WorkspaceRuntimeRemovalController, } from './routes/workspace-management.js'; +import { isNativeDirectoryPickerAvailable } from './native-directory-picker.js'; import type { WorkspaceRegistrationStore } from './workspace-registration-store.js'; import { registerWorkspaceGitRoutes, @@ -974,6 +975,7 @@ export function createServeApp( acpHttpEnabled: acpHttpEnabledAtBoot, workspaceRuntimeRemovalAvailable: deps.workspaceRuntimeRemoval !== undefined, + nativeDirectoryPickerAvailable: isNativeDirectoryPickerAvailable(), workspaceTrustHotReloadAvailable: deps.workspaceTrustHotReloadAvailable === true, isPrimaryWorkspaceTrusted: () => isPrimaryWorkspaceTrusted(), diff --git a/packages/cli/src/serve/server/serve-features.ts b/packages/cli/src/serve/server/serve-features.ts index fa73e9ca29c..7abc98e9463 100644 --- a/packages/cli/src/serve/server/serve-features.ts +++ b/packages/cli/src/serve/server/serve-features.ts @@ -57,6 +57,7 @@ interface CreateServeFeaturesDeps { realtimeVoiceEnabled: () => boolean; acpHttpEnabled?: boolean; workspaceRuntimeRemovalAvailable?: boolean; + nativeDirectoryPickerAvailable?: boolean; workspaceTrustHotReloadAvailable?: boolean; isPrimaryWorkspaceTrusted?: () => boolean; env?: Readonly>; @@ -91,6 +92,7 @@ export function createServeFeatures( realtimeVoiceEnabled, acpHttpEnabled, workspaceRuntimeRemovalAvailable, + nativeDirectoryPickerAvailable, workspaceTrustHotReloadAvailable, } = deps; const getEnv = deps.getEnv ?? (() => deps.env ?? process.env); @@ -143,6 +145,7 @@ export function createServeFeatures( scratchWorkspaceRegistrationAvailable: scratchWorkspaceRegistrationAvailable(), workspaceRuntimeRemovalAvailable, + nativeDirectoryPickerAvailable, workspaceTrustHotReloadAvailable, acpHttpEnabled: currentAcpHttpEnabled, realtimeVoiceEnabled: realtimeVoiceEnabled(), diff --git a/packages/web-shell/client/App.test.tsx b/packages/web-shell/client/App.test.tsx index 4030676c5f7..087c58d02ab 100644 --- a/packages/web-shell/client/App.test.tsx +++ b/packages/web-shell/client/App.test.tsx @@ -8733,6 +8733,7 @@ describe('App session callbacks', () => { 'dynamic_workspace_registration', 'persistent_workspace_registration', 'workspace_display_name', + 'native_directory_picker', ], workspaces: [ { @@ -8792,6 +8793,32 @@ describe('App session callbacks', () => { ).toHaveLength(1); }); + it('omits the directory picker on headless daemon hosts', async () => { + mockWorkspace.capabilities = { + features: ['dynamic_workspace_registration'], + workspaces: [ + { + id: 'primary', + cwd: '/tmp/project', + primary: true, + trusted: true, + }, + ], + } as typeof mockWorkspace.capabilities; + const { container } = renderApp(); + await flush(); + + act(() => { + container + .querySelector('[data-testid="open-add-workspace"]') + ?.click(); + }); + expect( + container.querySelectorAll('[data-testid="add-workspace-dialog"]'), + ).toHaveLength(1); + expect(testState.latestAddWorkspaceDialogProps?.onPick).toBeUndefined(); + }); + it('forwards a supported workspace display name through the shared mutation lane', async () => { const added = { id: 'payments', diff --git a/packages/web-shell/client/App.tsx b/packages/web-shell/client/App.tsx index d5bf7d2773b..e4bf3a5b1fc 100644 --- a/packages/web-shell/client/App.tsx +++ b/packages/web-shell/client/App.tsx @@ -2250,6 +2250,11 @@ export function App({ const workspaceDisplayNameSupported = workspace.capabilities?.features?.includes('workspace_display_name') === true; + // Headless daemon hosts omit the tag so the Browse affordance stays + // hidden instead of failing on every click. + const nativeDirectoryPickerSupported = + workspace.capabilities?.features?.includes('native_directory_picker') === + true; const gitHubPrsSupported = workspace.capabilities?.features?.includes('workspace_github_prs') === true; const [showAddWorkspaceDialog, setShowAddWorkspaceDialog] = useState(false); @@ -11308,10 +11313,15 @@ export function App({ onClose={() => setShowAddWorkspaceDialog(false)} onAdd={handleAddWorkspace} onSuggest={workspaceActions.suggestWorkspacePaths} - onPick={async () => { - const result = await workspaceActions.pickWorkspaceDirectory(); - return result.selected ? result.path : undefined; - }} + onPick={ + nativeDirectoryPickerSupported + ? async () => { + const result = + await workspaceActions.pickWorkspaceDirectory(); + return result.selected ? result.path : undefined; + } + : undefined + } persistenceSupported={ persistentWorkspaceRegistrationSupported } From cb34d1a7766e177d85cf614ff5405430f20df149 Mon Sep 17 00:00:00 2001 From: qqqys Date: Tue, 18 Aug 2026 20:28:54 +0800 Subject: [PATCH 05/26] fix(serve): judge the worker TLS trust gaps on the whole serving file MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit R2-1, R2-2, R2-5 from review round 2. R2-1. `workerDialHost` returned WHATWG `URL.hostname`, which keeps the brackets on an IPv6 literal (`[::1]`). `isIP('[::1]')` is 0, so `certCoversHost` took the DNS-name branch and `checkHost('[::1]')` could never match the iPAddress SAN the certificate actually carries — the boot diagnostic false-positived on every TLS daemon bound to `::1` with a correct cert, and told the operator to reissue it. The brackets are now stripped, so the address is checked as an address and also printed unbracketed the way a SAN spells it. R2-2. `describeWorkerTlsTrustGaps` built one `X509Certificate` from the file, which reads only the FIRST PEM block. A standard `fullchain.pem` (leaf + issuing CA) was therefore judged on its leaf alone and reported as unable to anchor worker trust — even though the supervisor injects that same whole file as the workers' `NODE_EXTRA_CA_CERTS`, root included, so trust does establish. The file is now split into every certificate it carries and the leaf's chain is walked through them; the gap is reported only when the chain fails to terminate in a self-signed certificate inside the file. A leaf-only file still reports it. The walk is bounded by a fingerprint set, so a cross-signed pair cannot loop. R2-5. The merged-CA-bundle test asserted `toContain('OP-CERT')` + `toContain('DAEMON-CERT')`, which both survive mutating the join separator to `''` — with real PEM inputs that mutant fuses `-----END CERTIFICATE-----` onto the next `-----BEGIN CERTIFICATE-----` and makes the bundle unparseable. It now asserts the exact bundle text, which pins the separator and the order. Verified: run-qwen-serve 275/275, channel-worker-supervisor 90/90, eslint and prettier clean on the touched files. Typecheck error count is 139 both with and without this change (worktree build skew against the main checkout's stale `@qwen-code/*` dist; the same 139 appear on the unmodified branch). Mutation-checked three ways, each reverting exactly one fix: - dropping the bracket strip fails both new IPv6 tests - `chainIsSelfAnchored` -> `isSelfSignedCert` fails the fullchain test - `.join('\n')` -> `.join('')` fails the merged-bundle test --- .../serve/channel-worker-supervisor.test.ts | 7 +- packages/cli/src/serve/run-qwen-serve.test.ts | 142 ++++++++++++++++++ packages/cli/src/serve/run-qwen-serve.ts | 85 ++++++++++- 3 files changed, 226 insertions(+), 8 deletions(-) diff --git a/packages/cli/src/serve/channel-worker-supervisor.test.ts b/packages/cli/src/serve/channel-worker-supervisor.test.ts index adad8c10cc3..0c3fe572450 100644 --- a/packages/cli/src/serve/channel-worker-supervisor.test.ts +++ b/packages/cli/src/serve/channel-worker-supervisor.test.ts @@ -341,8 +341,11 @@ describe('createChannelWorkerSupervisor', () => { const env = (spawnWorker.mock.calls[0]![2] as { env: NodeJS.ProcessEnv }) .env; const combined = fs.readFileSync(env['NODE_EXTRA_CA_CERTS']!, 'utf8'); - expect(combined).toContain('OP-CERT'); - expect(combined).toContain('DAEMON-CERT'); + // R2-5: exact text, not two `toContain`s — those survive a mutated + // separator, and with real PEM inputs that mutant fuses + // `-----END CERTIFICATE-----` onto the next `-----BEGIN CERTIFICATE-----` + // and makes the whole bundle unparseable. + expect(combined).toBe('OP-CERT\nDAEMON-CERT\n'); fs.rmSync(path.dirname(env['NODE_EXTRA_CA_CERTS']!), { recursive: true, force: true, diff --git a/packages/cli/src/serve/run-qwen-serve.test.ts b/packages/cli/src/serve/run-qwen-serve.test.ts index 0dffcfc42dc..f99d2e727d9 100644 --- a/packages/cli/src/serve/run-qwen-serve.test.ts +++ b/packages/cli/src/serve/run-qwen-serve.test.ts @@ -1191,6 +1191,98 @@ gI/irGYXddbzWJQla/KPV53wn5nK6Ho4dY1Z76slnwMoufrLM1oUt1QKUeyOKsOD -----END CERTIFICATE----- `; +// Self-signed and covering `::1` via an iPAddress SAN — the shape a daemon +// bound to IPv6 loopback needs. Not a real secret. +const TEST_TLS_CERT_IPV6_SAN = `-----BEGIN CERTIFICATE----- +MIIDOjCCAiKgAwIBAgIUYLTXyX2vAhC+OaM3JD4xKLtyfV8wDQYJKoZIhvcNAQEL +BQAwFDESMBAGA1UEAwwJbG9jYWxob3N0MCAXDTI2MDgxODEyMjEzMFoYDzIxMjYw +NzI1MTIyMTMwWjAUMRIwEAYDVQQDDAlsb2NhbGhvc3QwggEiMA0GCSqGSIb3DQEB +AQUAA4IBDwAwggEKAoIBAQCY6GprczPMvANzG1zLli+HDkEyyUk9lnk3Lsgu8yQJ +TqpBNBR+dTN7sYPccZpNbZ/N3G6vETbtvQ5VtKXI8izvliZMNGm2WNhr+OpMnWVb +RQ03qiwxzISFArGwYPF9mDTDpS+fwvkIN7B0N88rdmlaPez5Oy3egHQfwSrzrzId +dnd29tvGq9EnUps1xBgspFD8buK9fK1na4iypzSzYy9ub2tZ5ZliiqIGdmLxtE8j +FdyIiASOCujAxjovrDcJ+Xnr3ANRgyzHS3tQdbemLlEmu9zRk/ic7FFK2acNji/O +s5e8pJUKPmZnyyqIFlhFl8iIKvuZZev81p7Hnvmc0SHDAgMBAAGjgYEwfzAdBgNV +HQ4EFgQUzZj7sxpXzyOiet7XS0oRiV8TQtIwHwYDVR0jBBgwFoAUzZj7sxpXzyOi +et7XS0oRiV8TQtIwDwYDVR0TAQH/BAUwAwEB/zAsBgNVHREEJTAjhxAAAAAAAAAA +AAAAAAAAAAABhwR/AAABgglsb2NhbGhvc3QwDQYJKoZIhvcNAQELBQADggEBAFEK +M3+ggPGi6bFk3z9AjBWBLkJ2JsuHC1IwJ2ReXCBzwlzlHfJq8TyVTHeH+oHChVyK +KZlWn2GDXZMrDzLxZLwH52iKq3seYw/bZZ/TpugO6OHj1WmGXyl0sajMFye3VQAT ++M+irxpT/2eQqGV73lNbuNFvcwu4FaO3n8Ux6eG1BusQCx1vc6wvoK42kb9wSJN8 +qpqn1pNDH9P3Ub/1GbhWEytVsB8B3EewG/SE11cGhXSup1K4IiPcLMJX9SYGq+uM +GcrrZsPYuLm5eL6J3QjkFzMqyS6/4L881mihR/HKLdEnDyMzayGZ44O+DmZilUKv +w3Chmx7wNIzXjtZRcUk= +-----END CERTIFICATE----- +`; + +// A CA-issued leaf shipped together with its issuing root in one file, the +// standard `fullchain.pem` a real CA hands out. Not a real secret. +const TEST_TLS_CERT_FULLCHAIN = `-----BEGIN CERTIFICATE----- +MIIDMzCCAhugAwIBAgIUfMQ0J1fG/BhJQuzOilTasy8quOMwDQYJKoZIhvcNAQEL +BQAwJjEkMCIGA1UEAwwbcXdlbiBmdWxsY2hhaW4gdGVzdCByb290IENBMCAXDTI2 +MDgxODEyMjEzN1oYDzIxMjYwNzI1MTIyMTM3WjAUMRIwEAYDVQQDDAlsb2NhbGhv +c3QwggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQCE4sZa+FxusjUk7TzX +9FV/x7KAIy+lu7G20F2TjSeQ6mHhwkFb/rsADoi+9RU4MF+m/Mx+Lilccu2pVk+b +Ri+GksxX4xAC8L7XIhRwDdYWHHOMr1WnERKMqdRcEbzCAuQhR32Z0vFdg+T3o+TH +MkQ3AXQkQc0uu5r40e3VWuRweWnOfJqojH4VQfjk/44cLZBBRAS3owOWC9fAciJI +C5IgBnuYC7TOoZ1A69Lo+2C5UhIm6QEBzPtzVI87gzbDwqM7x8e3mMhLDQmNS3uD ++EVd05spuBq/KXMTSqHK7OIUPMFr3wXgVRm8i+68/4C2TnZANS6WSD5QZYivB4r7 +MUTtAgMBAAGjaTBnMBoGA1UdEQQTMBGHBH8AAAGCCWxvY2FsaG9zdDAJBgNVHRME +AjAAMB0GA1UdDgQWBBQr5x1h5l442pqfn0GFyn77KiWz0TAfBgNVHSMEGDAWgBSA +9DApi3O49l8S/UWWA91PrZWtQDANBgkqhkiG9w0BAQsFAAOCAQEAaxo+Y/u1iCNt +Vz4bmiRlqfhjVVe9yxa0Q8rzC/V9V7qrWpHjONXLErEKZ59oi8a80ndjugdXyw0g +gUKCmeykUtSbRLUTsZ741VKADjt87YceLrxsSVrtwMJjX2GoDNXIggRzmzdEjz3d +nRzDXFIEEn/g90kaNCYJSkPld2wk4M0IbEGpc8V7sO09I3T0igwfduVMO31X+mV4 +7A/J5QSE/oAF3PbuUvfzI9Hl1vdzgDUal3v8Sqh4oDgucc+YVZeCUuthq8zVT4Z5 +V6y0UOsNXHRE41EN7hK3zOKWJFwoS+ga2ACg+K4yuOnlhU+2MHa4XENVyaTsQaPo +B6U6dT+UdA== +-----END CERTIFICATE----- +-----BEGIN CERTIFICATE----- +MIIDLzCCAhegAwIBAgIUFRsEYHgjJUpGISLDoybs5vSsCmEwDQYJKoZIhvcNAQEL +BQAwJjEkMCIGA1UEAwwbcXdlbiBmdWxsY2hhaW4gdGVzdCByb290IENBMCAXDTI2 +MDgxODEyMjEzN1oYDzIxMjYwNzI1MTIyMTM3WjAmMSQwIgYDVQQDDBtxd2VuIGZ1 +bGxjaGFpbiB0ZXN0IHJvb3QgQ0EwggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEK +AoIBAQDuh/DLVFUTeOwksyGdtViAHe9TNBFWUdeq5zGlcxvX5c7Qbjkclyajclb3 +yY0pJ6zgD+uSw6zAE5jID4rtsmSyciakqgHMpPmBMn7GE0I5JCrBnxsN3g7P7EUE +qKzgj8rbUIyNQQt1E43wx4dJy4qN2hlKTusBvnUTtPB2ocRmC6+nXX/+nZWPahzI +MALyl17Nq5w/pzODEtSaC18jscN/bs8CkcDB0kjnKUV8UlbPtMy//n/WjXfDiQV4 +KUbQQ832Xl6xwuQTHEGq01Gb/NZO5CE/zdu86/82Jnn2Mv2uBDL+1CBL7kOxgZ0Y +axxW/a55BNpMElTbZxkgTSYivBwLAgMBAAGjUzBRMB0GA1UdDgQWBBSA9DApi3O4 +9l8S/UWWA91PrZWtQDAfBgNVHSMEGDAWgBSA9DApi3O49l8S/UWWA91PrZWtQDAP +BgNVHRMBAf8EBTADAQH/MA0GCSqGSIb3DQEBCwUAA4IBAQBfH7T/zM7pVxh4Qv+m +InAyvXPPDdBXmVfoBwHKEMyRts7rbxBHa7BV+qVyBkgXyfjXUL6QQmSXNfG/aHIx +rW9yVN1nM9sUwO5mTO3v07Hjqg00OJQYrqFMI8ba0nxpuIgr8Joj296/25/zwpxW +BjkdDp6EK2LHD4JU73jEMWDMDhQ3VMf8eb6bL3SxujhhhD1T7omTJkPKcUt4BCsn +boUKNFYlbk0HHXZmoXxTIoBxv8aOTWIIJ++sASqH7+9QY2iYtoW7kmdWLcM95nGb +Ptz8eWt0AkYE+GuX4GgOQxWJi0IuHzM7ke3fqjOw/tu01V1inWvVx2eg4Gv+vq2I +x2ZE +-----END CERTIFICATE----- +`; + +// The same leaf on its own — the chain does not terminate anywhere in the +// file, so the workers really would fail to verify it. Not a real secret. +const TEST_TLS_CERT_FULLCHAIN_LEAF_ONLY = `-----BEGIN CERTIFICATE----- +MIIDMzCCAhugAwIBAgIUfMQ0J1fG/BhJQuzOilTasy8quOMwDQYJKoZIhvcNAQEL +BQAwJjEkMCIGA1UEAwwbcXdlbiBmdWxsY2hhaW4gdGVzdCByb290IENBMCAXDTI2 +MDgxODEyMjEzN1oYDzIxMjYwNzI1MTIyMTM3WjAUMRIwEAYDVQQDDAlsb2NhbGhv +c3QwggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQCE4sZa+FxusjUk7TzX +9FV/x7KAIy+lu7G20F2TjSeQ6mHhwkFb/rsADoi+9RU4MF+m/Mx+Lilccu2pVk+b +Ri+GksxX4xAC8L7XIhRwDdYWHHOMr1WnERKMqdRcEbzCAuQhR32Z0vFdg+T3o+TH +MkQ3AXQkQc0uu5r40e3VWuRweWnOfJqojH4VQfjk/44cLZBBRAS3owOWC9fAciJI +C5IgBnuYC7TOoZ1A69Lo+2C5UhIm6QEBzPtzVI87gzbDwqM7x8e3mMhLDQmNS3uD ++EVd05spuBq/KXMTSqHK7OIUPMFr3wXgVRm8i+68/4C2TnZANS6WSD5QZYivB4r7 +MUTtAgMBAAGjaTBnMBoGA1UdEQQTMBGHBH8AAAGCCWxvY2FsaG9zdDAJBgNVHRME +AjAAMB0GA1UdDgQWBBQr5x1h5l442pqfn0GFyn77KiWz0TAfBgNVHSMEGDAWgBSA +9DApi3O49l8S/UWWA91PrZWtQDANBgkqhkiG9w0BAQsFAAOCAQEAaxo+Y/u1iCNt +Vz4bmiRlqfhjVVe9yxa0Q8rzC/V9V7qrWpHjONXLErEKZ59oi8a80ndjugdXyw0g +gUKCmeykUtSbRLUTsZ741VKADjt87YceLrxsSVrtwMJjX2GoDNXIggRzmzdEjz3d +nRzDXFIEEn/g90kaNCYJSkPld2wk4M0IbEGpc8V7sO09I3T0igwfduVMO31X+mV4 +7A/J5QSE/oAF3PbuUvfzI9Hl1vdzgDUal3v8Sqh4oDgucc+YVZeCUuthq8zVT4Z5 +V6y0UOsNXHRE41EN7hK3zOKWJFwoS+ga2ACg+K4yuOnlhU+2MHa4XENVyaTsQaPo +B6U6dT+UdA== +-----END CERTIFICATE----- +`; + describe('describeWorkerTlsTrustGaps', () => { const daemonUrl = 'https://127.0.0.1:4170'; @@ -1247,6 +1339,56 @@ describe('describeWorkerTlsTrustGaps', () => { ).toEqual([]); }); + it('anchors a fullchain serving file on the issuing CA it carries', () => { + // R2-2: `X509Certificate` reads only the first PEM block, so a fullchain + // used to be judged on its leaf alone and reported as unanchorable — even + // though the supervisor injects the whole file, root included, as the + // workers' trust store. + expect( + describeWorkerTlsTrustGaps({ + cert: Buffer.from(TEST_TLS_CERT_FULLCHAIN), + certPath: '/certs/fullchain.pem', + daemonUrl, + }), + ).toEqual([]); + }); + + it('still names the gap when the bundle stops short of a root', () => { + const gaps = describeWorkerTlsTrustGaps({ + cert: Buffer.from(TEST_TLS_CERT_FULLCHAIN_LEAF_ONLY), + certPath: '/certs/fullchain.pem', + daemonUrl, + }); + expect(gaps).toHaveLength(1); + expect(gaps[0]).toContain('UNABLE_TO_VERIFY_LEAF_SIGNATURE'); + expect(gaps[0]).toContain('qwen fullchain test root CA'); + }); + + it('checks an IPv6 dial host against the iPAddress SAN, brackets stripped', () => { + // R2-1: `URL.hostname` yields `[::1]`, which `isIP` rejects, so the check + // used to take the DNS-name branch and false-positive on every correct + // IPv6 serving cert. + expect( + describeWorkerTlsTrustGaps({ + cert: Buffer.from(TEST_TLS_CERT_IPV6_SAN), + certPath: '/certs/daemon.pem', + daemonUrl: 'https://[::1]:4170', + }), + ).toEqual([]); + }); + + it('names the SAN gap for an IPv6 host the certificate does not cover', () => { + const gaps = describeWorkerTlsTrustGaps({ + cert: Buffer.from(TEST_TLS_CERT_IPV6_SAN), + certPath: '/certs/daemon.pem', + daemonUrl: 'https://[fd00::1]:4170', + }); + expect(gaps).toHaveLength(1); + expect(gaps[0]).toContain('ERR_TLS_CERT_ALTNAME_INVALID'); + expect(gaps[0]).toContain('fd00::1'); + expect(gaps[0]).not.toContain('[fd00::1]'); + }); + it('defers to the boot parse guard on an unreadable certificate', () => { expect( describeWorkerTlsTrustGaps({ diff --git a/packages/cli/src/serve/run-qwen-serve.ts b/packages/cli/src/serve/run-qwen-serve.ts index 8417c59cabe..b36c3a35c8b 100644 --- a/packages/cli/src/serve/run-qwen-serve.ts +++ b/packages/cli/src/serve/run-qwen-serve.ts @@ -700,10 +700,13 @@ export function describeWorkerTlsTrustGaps(opts: { daemonUrl: string; operatorCaCertPath?: string; }): string[] { - let x509: X509Certificate; - try { - x509 = new X509Certificate(opts.cert); - } catch { + // A serving file is routinely a fullchain (leaf + issuing CA in one PEM), + // and the supervisor injects the whole file as the workers' + // NODE_EXTRA_CA_CERTS — so the trust question is about the file, not about + // its first block alone. + const chain = parseCertChain(opts.cert); + const x509 = chain[0]; + if (!x509) { // Boot validation already rejected unparseable certs with a better message. return []; } @@ -714,7 +717,7 @@ export function describeWorkerTlsTrustGaps(opts: { // terminates the chain. An operator-set NODE_EXTRA_CA_CERTS is merged into // the worker bundle and may already carry the issuing root, so only flag the // case where the leaf is all the worker gets. - if (!opts.operatorCaCertPath && !isSelfSignedCert(x509)) { + if (!opts.operatorCaCertPath && !chainIsSelfAnchored(chain)) { gaps.push( `--tls-cert "${opts.certPath}" is issued by another CA ` + `(${x509.issuer.replace(/\r?\n/g, ', ')}), not self-signed, so the ` + @@ -745,9 +748,79 @@ function isSelfSignedCert(x509: X509Certificate): boolean { } } +// Base64 never contains `-`, so the body match cannot run past its own +// end marker and cannot backtrack. +const PEM_CERTIFICATE_BLOCK = + /-----BEGIN CERTIFICATE-----[^-]*-----END CERTIFICATE-----/g; + +/** + * Every certificate in a PEM serving file, leaf first. `X509Certificate` reads + * only the first block of a bundle, so a fullchain file has to be split before + * any of it past the leaf can be reasoned about. A non-PEM (DER) buffer has no + * blocks to split and is handed over whole. + */ +function parseCertChain(cert: Buffer): X509Certificate[] { + const blocks = cert.toString('utf8').match(PEM_CERTIFICATE_BLOCK); + if (!blocks) { + try { + return [new X509Certificate(cert)]; + } catch { + return []; + } + } + const chain: X509Certificate[] = []; + for (const block of blocks) { + try { + chain.push(new X509Certificate(block)); + } catch { + // One malformed block does not make the rest of the file unusable. + } + } + return chain; +} + +function certIssuedBy(cert: X509Certificate, issuer: X509Certificate): boolean { + try { + return cert.checkIssued(issuer) && cert.verify(issuer.publicKey); + } catch { + return false; + } +} + +/** + * Whether the leaf's chain terminates inside the file itself. Workers get the + * whole file as their trust store, so a fullchain that walks up to a + * self-signed root anchors fine even though the leaf never could alone. + */ +function chainIsSelfAnchored(chain: readonly X509Certificate[]): boolean { + let next: X509Certificate | undefined = chain[0]; + const walked = new Set(); + while (next) { + const current: X509Certificate = next; + if (isSelfSignedCert(current)) return true; + walked.add(current.fingerprint256); + next = chain.find( + (candidate) => + !walked.has(candidate.fingerprint256) && + certIssuedBy(current, candidate), + ); + } + return false; +} + +/** + * WHATWG `URL.hostname` keeps the brackets on an IPv6 literal (`[::1]`), and + * `isIP` does not recognise the bracketed form — so an unstripped host falls + * through to the DNS-name branch of the SAN check, where it can never match + * the iPAddress SAN such a certificate actually carries. + */ function workerDialHost(daemonUrl: string): string | undefined { try { - return new URL(daemonUrl).hostname || undefined; + const hostname = new URL(daemonUrl).hostname; + if (!hostname) return undefined; + return hostname.startsWith('[') && hostname.endsWith(']') + ? hostname.slice(1, -1) + : hostname; } catch { return undefined; } From 2bc867616030b40b77657dce6f6be08f190596d3 Mon Sep 17 00:00:00 2001 From: qqqys Date: Wed, 19 Aug 2026 03:36:30 +0800 Subject: [PATCH 06/26] fix(serve): judge the worker CA bundle by what Node's loader accepts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 2 review findings on #9392: 2 Critical, 5 Suggestion. R2-11 (Critical): the merge treated a merely *readable* operator NODE_EXTRA_CA_CERTS as trustworthy. Node's certificate loader is line-strict and all-or-nothing — a bundle built with `cat a.pem b.pem` where a.pem lacks a trailing newline fuses `-----END CERTIFICATE----------BEGIN CERTIFICATE-----` onto one line, and Node then discards the WHOLE bundle, daemon cert included. The existing fallback only fired on a read *failure*, so this shape sailed through the success path and left every worker trusting neither the operator CA nor the daemon cert while /health stayed green. The merge now extracts blocks with a line-strict PEM matcher and takes the existing warn-and-fall-back path when the operator file yields no loadable block or has a marker that produced none. `tls.createSecureContext({ ca })` does not throw on that shape, so it is not used as the validator. R2-12 (Critical): guard the 0o700 bundle-directory mode assertion on win32. `fs.mkdtempSync` ignores the mode there and libuv synthesises st_mode from file attributes (0o666 for a writable directory, structurally never 0o700), so the merge queue's test_windows job would go red on a test that passes on Linux/macOS. Same guard shape as observed-contact-store.test.ts. R2-13: write only certificate blocks into the bundle. A combined cert+key serving PEM passes boot validation, which parses the first block alone, so its private key was being copied into a tmpdir file NODE_EXTRA_CA_CERTS never reads — and that copy outlives a SIGKILLed daemon, whose `exit` cleanup cannot run. R2-4: revalidate the merged-bundle cache. It was keyed on paths alone, so an in-place operator CA rotation never reached respawned workers for the daemon's whole lifetime (before this PR a respawn read the operator's file live), and an external tmp cleaner aging out the bundle directory left every future respawn pointed at a dead path. Cache entries now carry each source's mtime/size and the bundle's existence is re-checked on hit. R2-3: harden the boot-time trust-gap check along the three corners the review demonstrated, per its stated minimum. Coverage is judged on the operator CA's *contents* rather than on the variable being set; every member of the anchor walk has its validity window checked (`x509.verify` is signature-only and never consults dates, so an expired root anchored "fine" while every handshake failed CERT_HAS_EXPIRED); and the leaf-anchor message no longer asserts a certain failure, since the check cannot see the workers' default trust store. `chainIsSelfAnchored` becomes `walkWorkerAnchorPath`, which returns the certificates the walk relied on so the date check can scope itself to them. R2-14: pin worker-side acceptance of `https://[::1]:4170`. The formatter emits it for a `::1` TLS bind and nothing else pinned the `'[::1]'` entry in LOOPBACK_BINDS, so dropping it as redundant kept every test green while regressing this PR's own failure mode on IPv6. R2-6: cover the boot-time warning wiring end to end. Only the pure function was tested, so deleting the loop, inverting its guard or feeding it unresolved values all shipped green. Two runQwenServe tests now boot a real TLS daemon on `::1` (a real SAN gap for a fixture cert that still pairs with its key) and on 127.0.0.1, asserting the gap text does and does not reach the daemon log. BEHAVIOUR FLIP — leaf-anchor gap suppression. A set-but-unhelpful NODE_EXTRA_CA_CERTS used to silence this warning outright. It no longer does: a typo'd, unrelated or unloadable path anchors exactly as little as no CA at all, and suppressing on the variable's mere presence silenced the diagnostic in the cases it was written for. The test that pinned the old behaviour is rewritten to assert the new contract rather than deleted, and three tests cover the paths it used to hide (anchoring CA, non-anchoring CA, unreadable path). BEHAVIOUR FLIP — a DER-encoded operator NODE_EXTRA_CA_CERTS is now refused with a warning instead of concatenated. Node's loader rejects it either way; the difference is that it no longer takes the daemon cert down with it. Verification: packages/cli — run-qwen-serve (283), channel-worker- supervisor (94), daemon-worker (85), process-env-guard (3), channel-worker-group — 507 tests pass. eslint and prettier clean. `tsc --noEmit -p packages/cli` reports 2 errors, both TS6305 against packages/core/dist; the same 2 appear on the stashed tree, so they are worktree build skew, not this change. Mutation-verified, 11 of 11 mutants killed: loose PEM regex, whole-file copy (key retained), no source-stamp revalidation, no bundle stat, `'[::1]'` dropped from LOOPBACK_BINDS, path-only gap suppression, chain-date check deleted, unsoftened wording, warn loop gutted, warn guard inverted, wrong daemonUrl fed to the check. R2-12 is a test-only platform guard with no production code to mutate. Co-Authored-By: Claude Opus 5 --- .../commands/channel/daemon-worker.test.ts | 25 ++ .../serve/channel-worker-supervisor.test.ts | 245 +++++++++++++++++- .../src/serve/channel-worker-supervisor.ts | 132 ++++++++-- .../cli/src/serve/process-env-guard.test.ts | 5 +- packages/cli/src/serve/run-qwen-serve.test.ts | 184 ++++++++++++- packages/cli/src/serve/run-qwen-serve.ts | 92 +++++-- 6 files changed, 636 insertions(+), 47 deletions(-) diff --git a/packages/cli/src/commands/channel/daemon-worker.test.ts b/packages/cli/src/commands/channel/daemon-worker.test.ts index 8dcac421a31..16056fd4793 100644 --- a/packages/cli/src/commands/channel/daemon-worker.test.ts +++ b/packages/cli/src/commands/channel/daemon-worker.test.ts @@ -1370,6 +1370,31 @@ describe('runChannelDaemonWorker', () => { }); }); + it('accepts an IPv6 loopback daemon URL for a ::1-bound TLS daemon', async () => { + // R2-14: formatChannelWorkerDaemonUrl emits `https://[::1]:` for a + // `::1` TLS bind, and this side accepts it only because `'[::1]'` sits in + // LOOPBACK_BINDS. Nothing else pins that entry, so dropping it as + // redundant keeps every test green while every ::1-bound TLS daemon's + // workers reject their own URL at boot and restart-loop — the exact + // failure this PR exists to remove, regressing on IPv6 alone. + const sdk = createSdk(); + mockLoadChannelsConfig.mockReturnValueOnce({ + telegram: { type: 'telegram' }, + }); + mockParseConfiguredChannels.mockResolvedValueOnce([parsedTelegram]); + + await runChannelDaemonWorker({ + daemonUrl: 'https://[::1]:4170', + workspace: '/workspace', + selection: { mode: 'all' }, + loadDaemonSdk: async () => sdk, + }); + + expect(sdk.DaemonClient).toHaveBeenCalledWith({ + baseUrl: 'https://[::1]:4170', + }); + }); + it('fails fast when no channels are configured', async () => { const sdk = createSdk(); mockLoadChannelsConfig.mockReturnValueOnce({}); diff --git a/packages/cli/src/serve/channel-worker-supervisor.test.ts b/packages/cli/src/serve/channel-worker-supervisor.test.ts index 0c3fe572450..99f56317125 100644 --- a/packages/cli/src/serve/channel-worker-supervisor.test.ts +++ b/packages/cli/src/serve/channel-worker-supervisor.test.ts @@ -57,6 +57,83 @@ const deliveryRequest: ChannelDeliveryRequest = { text: 'inspection result', }; +/** + * Real PEM material: the merge now validates its inputs the way Node's + * certificate loader does, so placeholder text like `OP-CERT` no longer + * exercises the merge path at all. + */ +const OPERATOR_CA_PEM = `-----BEGIN CERTIFICATE----- +MIIDHjCCAgagAwIBAgIUMfJwZrF6DjLX1ypLgu2A4v/SwKEwDQYJKoZIhvcNAQEL +BQAwHDEaMBgGA1UEAwwRcXdlbiB0ZXN0IHJvb3QgQ0EwIBcNMjYwODE4MDk0NzE4 +WhgPMjEyNjA3MjUwOTQ3MThaMBQxEjAQBgNVBAMMCWxvY2FsaG9zdDCCASIwDQYJ +KoZIhvcNAQEBBQADggEPADCCAQoCggEBAOff38zsoMq+oe2koKyZJ7aoGJC8CuAc +oYoLcJaWdp6yJaj5BpYeHAnQt8QCQZB86Fj1f3yuK6KwmGm3p49NrVJMl/T39CnK +ZAcIWATBw8mCWLFWlWhRgqrIQ5ka935m+z63gVhSQiCq2mNkAzm9I4UcbeAucSXn +Plk0Bc/CBUh5knrjxPEebicbCUaKteWnG3SBe5PjgP6DKZojd0VakmbrDhTW+yD4 +9LRqURfzvQZghA7stqErp+WJREKAaJbNNUEhGvRSwucIsah6u7OAbYP1IRaYBGDm +nlxaYBETRg0/3Kzx4SnPUuyx3uR6YP9MNuSzK5udCf39+iWSFCC+AnMCAwEAAaNe +MFwwGgYDVR0RBBMwEYcEfwAAAYIJbG9jYWxob3N0MB0GA1UdDgQWBBSItY/bpVFx +QRATvUzvo+JRFVpuyjAfBgNVHSMEGDAWgBRfCBabaBn4orvntHRiDcBU8W3vEzAN +BgkqhkiG9w0BAQsFAAOCAQEAjIiKztoj9JtpKfP2qSYsTe+4nvCZ1ZT4PtmXQMVp +lyHI02iH+NSSY92/ZdvGn2jBMzAFpVgJFlI6aZOne/qHI5qMf1RW7BfHBXza7wF6 +mdILIKRUYzm96o6IEuObE+QkSjRuA5OpLkObzGZLWfem0+fxnz0djbzeEBhHpP+b +VUUcl7r2wFb3+ClobIYS24Y+tWCl53XF+2YFNebECkA+19TivHPYgyywljyFNmzk +jCELOKOvOESV6kWBGUcrj8rcXoaF3BABInxZURGMRqWuivfYSjkGj65Trf2sVCXS +9mkiDfB/mYPvq3ODVYLvOjcxqPFsKaRA0Gw5Nm7WKGiOhg== +-----END CERTIFICATE----- +`; + +const DAEMON_CERT_PEM = `-----BEGIN CERTIFICATE----- +MIIDJzCCAg+gAwIBAgIUfuVC8Ulq3HIg+1tf36JrjAa6dr4wDQYJKoZIhvcNAQEL +BQAwFDESMBAGA1UEAwwJbG9jYWxob3N0MCAXDTI2MDYzMDAyMjIxOVoYDzIxMjYw +NjA2MDIyMjE5WjAUMRIwEAYDVQQDDAlsb2NhbGhvc3QwggEiMA0GCSqGSIb3DQEB +AQUAA4IBDwAwggEKAoIBAQCnEk5caJsr2ShJwi4bkAMr1/IzzueiUFbnnqs3XpaB +ANxpIZxi8WN1gf8MoAOioZteH51Q2nz8Zb2MVHoDMH3zx4V36VcXUaeR+/wZbFRN +94NlzYCXPnzPH+Mw/vle1PTM/boPON8F4ATGJZkzmGT8+M5CqDCW4isHlpGvbn0T +SdmqnmzihNBdaREVVkGJYa7JSFcgRth52+wTAOIM8e8HC1VTMw1OhXDAus6ro7z+ +u5XKGpG+JfsCpimNPYzNOPSkIr/QmxuaMq7kmYwT9J1Gyw9cQQj8vcipyLq6q3Hz +iMhxUXbWp7moi4e6CzxLKyPrWwhuh+3SXqIYshAYRsKNAgMBAAGjbzBtMB0GA1Ud +DgQWBBSM8bvfq77vXg5fsuhYGXsLuKjqxzAfBgNVHSMEGDAWgBSM8bvfq77vXg5f +suhYGXsLuKjqxzAPBgNVHRMBAf8EBTADAQH/MBoGA1UdEQQTMBGHBH8AAAGCCWxv +Y2FsaG9zdDANBgkqhkiG9w0BAQsFAAOCAQEAGUBgaBYEO119e28j61PTijfhw7mV +Q8AxlUjlv+HHx+IAPR+E8w7jiS97oxvFSIkmbV+FAQOWwTE+oNvrL5qSFlG7cI60 +wj+Jxwxr+/SShV5Jm7JlynAGxOvOZ1mfxzyGrlm5cg4hoRvcoWAtB/qtiIyFIz/s +fDAdZiFXRoTaZnpyPWA6iydf3mc0ZOastHib+mlFb+aedKz9by/f2Z1CY6RfckEj +20c9Mar85RYkVtVTIWNSwItASmQVBaoXsXK33y4C0P1NmPoYBzyPSXsOlmIZXui5 +WYj2mrPe2DL5gCeNUxMhmzgv0bgoYiksHmdyNjRmO5AQlcdjX/7CHg0zEQ== +-----END CERTIFICATE----- +`; + +const DAEMON_KEY_PEM = `-----BEGIN PRIVATE KEY----- +MIIEvQIBADANBgkqhkiG9w0BAQEFAASCBKcwggSjAgEAAoIBAQCnEk5caJsr2ShJ +wi4bkAMr1/IzzueiUFbnnqs3XpaBANxpIZxi8WN1gf8MoAOioZteH51Q2nz8Zb2M +VHoDMH3zx4V36VcXUaeR+/wZbFRN94NlzYCXPnzPH+Mw/vle1PTM/boPON8F4ATG +JZkzmGT8+M5CqDCW4isHlpGvbn0TSdmqnmzihNBdaREVVkGJYa7JSFcgRth52+wT +AOIM8e8HC1VTMw1OhXDAus6ro7z+u5XKGpG+JfsCpimNPYzNOPSkIr/QmxuaMq7k +mYwT9J1Gyw9cQQj8vcipyLq6q3HziMhxUXbWp7moi4e6CzxLKyPrWwhuh+3SXqIY +shAYRsKNAgMBAAECggEAQW/tG0qphEog+orAznDgnRqOtfYTScLX1w6RlzVIE60H +p3HPs/1B7HOHNyWxZtCPbxVI47NAAwfCbyVjSL6EhqgeQbI2N173GDmvKzH/7y3D +3GraM+L4tZOSw80KVTdpzqSObInk6IMuu4FceRX2cBLvjrIbne1l1yoFU8Yd3SCM +t8J46vMys7Rh4yR0iOl1hFeLYj8KolTdp6uNYTxaHMt363G7/TcJYRqjrLkpBpXJ +dJiP58a3WulvVKVHBjZYVmHLlkvla7LQ9tPRsk0gUQfzNpLzl6oBacrNrRv1F7Oe +keYqt+Kpy9HhZIHt57ahwKmjhjrfIUpyQadF/me0rQKBgQDVbLV6VngGjMSCPQOQ +VZcAMFZ+y1fgaHeVZwuFeRlCEHBDDmw5eWdUdUQNIRckpqf0IlU39aP/cLgjNZ0W +nmxfUwhdgEMam2aHZ/8eqrOl0HTa+F5PWz8NPLKsQ970vPb1XCsoEtDVXEsMqK+s +4h+zjRzy6lLy2cWvYZrDr/KwywKBgQDIZmitKO0MIJOWeqwI3MQvbBXCz9aEIG+3 +0ISQreD/7Z/IEcwrMpDD+z1sOj9OUO2GFflECdhtqo416cv3uo8LLABxuzsYOgug +ZPgW9oPKVRLfqc43/n0JMtIvS+Na/7C/nCNwcZZZU91V+VG4+1rexINQybnCRbQw +cBZLcX8nBwKBgQDMdZhl2vChVbnsCwee/l/qjmROk/9bvLjTKCSheaH46Eaj9u03 +IlcbUjwfV9QUCJReDYYWVf0GebXuBS64vIyVxbX93SJsGvPeRILjniT8dPd9zvKK +k5+TztJctaiiTWVJKUMu4NevjvtW5UNnHDnCiS1yiYltnbMEkTzyu1yEgQKBgAYk +pYbRX1rk0MFnJ0jqQ5VUkeIz7taEDAiterLYsbIGvcQrT3/vf+KSHBLqQjCLaIyY +tdhxGNJbzRo3/YmtjV8BTU4vOCOI+/xBvB0wF2AndXmnweuTgI+8oBbVE7YhanCl +P6zdvocke/97shailemISqI6XNhovJpThUtwwj4XAoGATwSvzX0VLRpoWwDl30oi +hxyfpb0iCzGik49j/oL+ZB5C8F8AdBpza8eTXJAeAVP7L5nvWffMgvcXs5sGMF7e +ARaOwZHpfsTw4Aq74yAWUKXumVGFXQpZMRj/QWgQEItTYF7rJVARIssv5miDbHvW +1Qm2tDpPnmCd1BedIYWCnHA= +-----END PRIVATE KEY----- +`; + describe('createChannelWorkerSupervisor', () => { afterEach(() => { vi.useRealTimers(); @@ -313,8 +390,8 @@ describe('createChannelWorkerSupervisor', () => { const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'qwen-ca-merge-')); const operatorCa = path.join(dir, 'operator.pem'); const daemonCa = path.join(dir, 'daemon.pem'); - fs.writeFileSync(operatorCa, 'OP-CERT\n'); - fs.writeFileSync(daemonCa, 'DAEMON-CERT\n'); + fs.writeFileSync(operatorCa, OPERATOR_CA_PEM); + fs.writeFileSync(daemonCa, DAEMON_CERT_PEM); const child = new FakeChild(); const spawnWorker = vi.fn( (_execPath: string, _argv: string[], _options: unknown) => child, @@ -345,7 +422,9 @@ describe('createChannelWorkerSupervisor', () => { // separator, and with real PEM inputs that mutant fuses // `-----END CERTIFICATE-----` onto the next `-----BEGIN CERTIFICATE-----` // and makes the whole bundle unparseable. - expect(combined).toBe('OP-CERT\nDAEMON-CERT\n'); + expect(combined).toBe( + `${OPERATOR_CA_PEM.trimEnd()}\n${DAEMON_CERT_PEM.trimEnd()}\n`, + ); fs.rmSync(path.dirname(env['NODE_EXTRA_CA_CERTS']!), { recursive: true, force: true, @@ -357,8 +436,8 @@ describe('createChannelWorkerSupervisor', () => { const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'qwen-ca-reuse-')); const operatorCa = path.join(dir, 'operator.pem'); const daemonCa = path.join(dir, 'daemon.pem'); - fs.writeFileSync(operatorCa, 'OP-CERT\n'); - fs.writeFileSync(daemonCa, 'DAEMON-CERT\n'); + fs.writeFileSync(operatorCa, OPERATOR_CA_PEM); + fs.writeFileSync(daemonCa, DAEMON_CERT_PEM); const spawnWorker = vi.fn( (_execPath: string, _argv: string[], _options: unknown) => new FakeChild(), @@ -401,7 +480,7 @@ describe('createChannelWorkerSupervisor', () => { it('keeps the daemon cert when the operator CA cannot be merged', async () => { const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'qwen-ca-fallback-')); const daemonCa = path.join(dir, 'daemon.pem'); - fs.writeFileSync(daemonCa, 'DAEMON-CERT\n'); + fs.writeFileSync(daemonCa, DAEMON_CERT_PEM); const warnings: string[] = []; const onWarning = (warning: Error) => warnings.push(warning.message); process.on('warning', onWarning); @@ -441,12 +520,154 @@ describe('createChannelWorkerSupervisor', () => { fs.rmSync(dir, { recursive: true, force: true }); }); + async function startWorkerWithCaPaths( + daemonCa: string, + operatorCa: string, + ): Promise<{ env: NodeJS.ProcessEnv }> { + const child = new FakeChild(); + const spawnWorker = vi.fn( + (_execPath: string, _argv: string[], _options: unknown) => child, + ); + const supervisor = createChannelWorkerSupervisor({ + cliEntryPath: '/repo/dist/index.js', + daemonUrl: 'https://127.0.0.1:4170', + tlsCaCertPath: daemonCa, + workspace: '/workspace', + selection: { mode: 'names', names: ['telegram'] }, + workerBaseEnv: { NODE_EXTRA_CA_CERTS: operatorCa }, + spawnWorker, + }); + const started = supervisor.start(); + child.emit('message', { + type: 'ready', + pid: 54321, + channels: ['telegram'], + requestedChannels: ['telegram'], + }); + await started; + return spawnWorker.mock.calls[0]![2] as { env: NodeJS.ProcessEnv }; + } + + it('keeps the daemon cert when the operator CA is readable but unloadable', async () => { + // R2-11: `cat a.pem b.pem` with no trailing newline in a.pem fuses + // `-----END CERTIFICATE----------BEGIN CERTIFICATE-----` onto one line. + // Node's loader is all-or-nothing on that shape — it drops the WHOLE + // bundle with `bad end line`, taking the daemon cert appended after it + // down too, so every worker handshake fails while /health stays green. + // The read succeeds, so the ENOENT fallback above never sees this. + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'qwen-ca-fused-')); + const operatorCa = path.join(dir, 'operator.pem'); + const daemonCa = path.join(dir, 'daemon.pem'); + fs.writeFileSync( + operatorCa, + `${OPERATOR_CA_PEM.trimEnd()}${OPERATOR_CA_PEM}`, + ); + fs.writeFileSync(daemonCa, DAEMON_CERT_PEM); + expect(fs.readFileSync(operatorCa, 'utf8')).toContain( + '-----END CERTIFICATE----------BEGIN CERTIFICATE-----', + ); + const warnings: string[] = []; + const onWarning = (warning: Error) => warnings.push(warning.message); + process.on('warning', onWarning); + + const { env } = await startWorkerWithCaPaths(daemonCa, operatorCa); + + expect(env['NODE_EXTRA_CA_CERTS']).toBe(daemonCa); + await new Promise((resolve) => setImmediate(resolve)); + process.off('warning', onWarning); + expect( + warnings.some( + (message) => + message.includes(operatorCa) && + message.includes('no PEM certificate block Node can load'), + ), + ).toBe(true); + fs.rmSync(dir, { recursive: true, force: true }); + }); + + it('leaves the private key of a combined operator PEM out of the bundle', async () => { + // R2-13: boot validation parses the first block only, so a combined + // cert+key PEM serves fine — and copying its key into a tmpdir bundle + // NODE_EXTRA_CA_CERTS never reads leaves key material behind a SIGKILLed + // daemon, where the `exit` cleanup cannot run. + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'qwen-ca-combined-')); + const operatorCa = path.join(dir, 'operator.pem'); + const daemonCa = path.join(dir, 'daemon.pem'); + fs.writeFileSync(operatorCa, `${OPERATOR_CA_PEM}${DAEMON_KEY_PEM}`); + fs.writeFileSync(daemonCa, DAEMON_CERT_PEM); + + const { env } = await startWorkerWithCaPaths(daemonCa, operatorCa); + + const bundlePath = env['NODE_EXTRA_CA_CERTS']!; + expect(bundlePath).not.toBe(daemonCa); + const bundle = fs.readFileSync(bundlePath, 'utf8'); + expect(bundle).not.toContain('PRIVATE KEY'); + expect(bundle).toBe( + `${OPERATOR_CA_PEM.trimEnd()}\n${DAEMON_CERT_PEM.trimEnd()}\n`, + ); + fs.rmSync(path.dirname(bundlePath), { recursive: true, force: true }); + fs.rmSync(dir, { recursive: true, force: true }); + }); + + it('rebuilds the bundle after the operator CA is rotated in place', async () => { + // R2-4(a): before this bundle existed a respawned worker read the + // operator's file live, so a path-only cache turns an in-place rotation + // into stale trust that lasts the daemon's whole lifetime. + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'qwen-ca-rotate-')); + const operatorCa = path.join(dir, 'operator.pem'); + const daemonCa = path.join(dir, 'daemon.pem'); + fs.writeFileSync(operatorCa, OPERATOR_CA_PEM); + fs.writeFileSync(daemonCa, DAEMON_CERT_PEM); + + const first = await startWorkerWithCaPaths(daemonCa, operatorCa); + const firstPath = first.env['NODE_EXTRA_CA_CERTS']!; + expect(fs.readFileSync(firstPath, 'utf8')).toContain( + OPERATOR_CA_PEM.trimEnd(), + ); + + // Rotate to a different certificate under the same path. + fs.writeFileSync(operatorCa, DAEMON_CERT_PEM); + const rotated = await startWorkerWithCaPaths(daemonCa, operatorCa); + const rotatedPath = rotated.env['NODE_EXTRA_CA_CERTS']!; + expect(fs.readFileSync(rotatedPath, 'utf8')).toBe( + `${DAEMON_CERT_PEM.trimEnd()}\n${DAEMON_CERT_PEM.trimEnd()}\n`, + ); + + fs.rmSync(path.dirname(firstPath), { recursive: true, force: true }); + fs.rmSync(path.dirname(rotatedPath), { recursive: true, force: true }); + fs.rmSync(dir, { recursive: true, force: true }); + }); + + it('rebuilds the bundle after a tmp cleaner removes it', async () => { + // R2-4(b): systemd-tmpfiles-clean ages out /tmp. A path-only cache then + // hands every future respawn a dead path — Node logs "Ignoring extra + // certs … load failed" and the worker restart-loops until the daemon + // itself restarts. + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'qwen-ca-aged-')); + const operatorCa = path.join(dir, 'operator.pem'); + const daemonCa = path.join(dir, 'daemon.pem'); + fs.writeFileSync(operatorCa, OPERATOR_CA_PEM); + fs.writeFileSync(daemonCa, DAEMON_CERT_PEM); + + const first = await startWorkerWithCaPaths(daemonCa, operatorCa); + const firstPath = first.env['NODE_EXTRA_CA_CERTS']!; + fs.rmSync(path.dirname(firstPath), { recursive: true, force: true }); + + const respawned = await startWorkerWithCaPaths(daemonCa, operatorCa); + const respawnedPath = respawned.env['NODE_EXTRA_CA_CERTS']!; + expect(respawnedPath).not.toBe(firstPath); + expect(fs.existsSync(respawnedPath)).toBe(true); + + fs.rmSync(path.dirname(respawnedPath), { recursive: true, force: true }); + fs.rmSync(dir, { recursive: true, force: true }); + }); + it('writes the merged bundle into a private directory, not a predictable tmp path', async () => { const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'qwen-ca-private-')); const operatorCa = path.join(dir, 'operator.pem'); const daemonCa = path.join(dir, 'daemon.pem'); - fs.writeFileSync(operatorCa, 'OP-CERT\n'); - fs.writeFileSync(daemonCa, 'DAEMON-CERT\n'); + fs.writeFileSync(operatorCa, OPERATOR_CA_PEM); + fs.writeFileSync(daemonCa, DAEMON_CERT_PEM); const child = new FakeChild(); const spawnWorker = vi.fn( (_execPath: string, _argv: string[], _options: unknown) => child, @@ -480,7 +701,13 @@ describe('createChannelWorkerSupervisor', () => { ); const bundleDir = path.dirname(bundlePath); expect(path.dirname(bundleDir)).toBe(os.tmpdir()); - expect(fs.statSync(bundleDir).mode & 0o777).toBe(0o700); + // Windows ignores mkdtempSync's mode and libuv synthesises st_mode from + // file attributes (0o666 for a writable directory, structurally never + // 0o700), so this POSIX assertion is guarded the way + // observed-contact-store.test.ts guards the identical pair. + if (process.platform !== 'win32') { + expect(fs.statSync(bundleDir).mode & 0o777).toBe(0o700); + } fs.rmSync(dir, { recursive: true, force: true }); fs.rmSync(bundleDir, { recursive: true, force: true }); }); diff --git a/packages/cli/src/serve/channel-worker-supervisor.ts b/packages/cli/src/serve/channel-worker-supervisor.ts index aea781a3493..f985e544c1b 100644 --- a/packages/cli/src/serve/channel-worker-supervisor.ts +++ b/packages/cli/src/serve/channel-worker-supervisor.ts @@ -387,7 +387,66 @@ const NODE_EXTRA_CA_CERTS_ENV = 'NODE_EXTRA_CA_CERTS'; * respawned on every restart, so without this the daemon would mint a fresh * bundle directory per spawn and leak all of them. */ -const mergedWorkerCaBundles = new Map(); +const mergedWorkerCaBundles = new Map(); + +interface MergedWorkerCaBundle { + bundlePath: string; + /** `${mtimeMs}:${size}` of each source file, in merge order. */ + sourceStamps: readonly string[]; +} + +function sourceStamp(filePath: string): string { + const stat = fs.statSync(filePath); + return `${stat.mtimeMs}:${stat.size}`; +} + +/** + * A PEM certificate block with every marker alone on its own line. Node's + * certificate loader is line-strict AND all-or-nothing: one fused + * `-----END CERTIFICATE----------BEGIN CERTIFICATE-----` (what + * `cat a.pem b.pem` produces when `a.pem` has no trailing newline) makes it + * discard the WHOLE bundle with `bad end line` — including the daemon cert + * appended after it, so the workers lose the trust the merge exists to give + * them. `tls.createSecureContext({ ca })` does not throw on that shape, so it + * cannot stand in as the validator. Base64 never contains `-`, so the body + * match cannot run past its own end marker or backtrack. + */ +const STRICT_PEM_CERTIFICATE_BLOCK = + /^-----BEGIN CERTIFICATE-----\n(?:[A-Za-z0-9+/=]+\n)+-----END CERTIFICATE-----$/gm; + +/** + * The certificate blocks of `contents`, or `undefined` when Node's loader + * would reject the file: no block at all, or a `BEGIN CERTIFICATE` marker + * that did not yield a well-formed block. + * + * Only certificate blocks come back. A combined cert+key serving PEM passes + * boot validation (which parses the first block alone), and copying its + * private key into a tmpdir bundle `NODE_EXTRA_CA_CERTS` never reads would + * leave key material behind a SIGKILLed daemon, where the `exit` cleanup + * cannot run. + */ +function extractCertificateBlocks(contents: string): string[] | undefined { + const normalized = contents.replace(/\r\n/g, '\n'); + const blocks = normalized.match(STRICT_PEM_CERTIFICATE_BLOCK) ?? []; + const markers = normalized.match(/-----BEGIN CERTIFICATE-----/g) ?? []; + if (blocks.length === 0 || blocks.length !== markers.length) return undefined; + return blocks; +} + +function warnWorkerCaMergeFallback( + operatorCaPath: string, + daemonCertPath: string, + reason: string, +): void { + // Falling back to the daemon cert alone silently drops the operator CA + // the merge above exists to preserve, and Node says nothing when the + // remaining cert loads fine — so say it here. + process.emitWarning( + `qwen: failed to merge ${NODE_EXTRA_CA_CERTS_ENV} "${operatorCaPath}" ` + + `with the daemon cert "${daemonCertPath}": ${reason}; channel ` + + `workers will trust only the daemon cert`, + ); +} function writeMergedWorkerCaBundle(contents: string): string { // mkdtempSync gives a 0700 directory with a random suffix, so the bundle @@ -416,27 +475,66 @@ function resolveWorkerCaCertPath( ): string { if (!existing || existing === daemonCertPath) return daemonCertPath; const cacheKey = `${existing}\0${daemonCertPath}`; + const sources = [existing, daemonCertPath]; const cached = mergedWorkerCaBundles.get(cacheKey); - if (cached) return cached; + if (cached) { + try { + // Two ways a hit goes stale, both ending in workers that restart-loop + // while the daemon stays green: the operator rotates their CA file in + // place (before this bundle existed a respawned worker read that file + // live), and an external tmp cleaner ages out the bundle directory, + // leaving the cache pointing at a dead path. Re-stat both ends. + fs.statSync(cached.bundlePath); + if ( + sources.every((src, i) => sourceStamp(src) === cached.sourceStamps[i]) + ) { + return cached.bundlePath; + } + } catch { + // Unreadable source or vanished bundle: rebuild below. + } + mergedWorkerCaBundles.delete(cacheKey); + } try { + const sourceStamps = sources.map(sourceStamp); // NODE_EXTRA_CA_CERTS takes a single file; merge so an operator-set CA - // (e.g. corporate proxy) keeps working alongside the daemon cert. - const combined = [ - fs.readFileSync(existing, 'utf8').trimEnd(), - fs.readFileSync(daemonCertPath, 'utf8').trimEnd(), - ].join('\n'); - const bundlePath = writeMergedWorkerCaBundle(`${combined}\n`); - mergedWorkerCaBundles.set(cacheKey, bundlePath); + // (e.g. corporate proxy) keeps working alongside the daemon cert. A + // merely *readable* operator file is not enough — one Node's loader + // rejects takes the daemon cert down with it, which is strictly worse + // than the fallback below. + const operatorBlocks = extractCertificateBlocks( + fs.readFileSync(existing, 'utf8'), + ); + if (!operatorBlocks) { + warnWorkerCaMergeFallback( + existing, + daemonCertPath, + 'it holds no PEM certificate block Node can load (every ' + + '-----BEGIN/END CERTIFICATE----- marker must sit alone on its line)', + ); + return daemonCertPath; + } + const daemonBlocks = extractCertificateBlocks( + fs.readFileSync(daemonCertPath, 'utf8'), + ); + if (!daemonBlocks) { + warnWorkerCaMergeFallback( + existing, + daemonCertPath, + 'the daemon cert holds no PEM certificate block to merge into', + ); + return daemonCertPath; + } + const bundlePath = writeMergedWorkerCaBundle( + `${[...operatorBlocks, ...daemonBlocks].join('\n')}\n`, + ); + mergedWorkerCaBundles.set(cacheKey, { bundlePath, sourceStamps }); return bundlePath; } catch (err) { - // Falling back to the daemon cert alone silently drops the operator CA - // the merge above exists to preserve, and Node says nothing when the - // remaining cert loads fine — so say it here. - process.emitWarning( - `qwen: failed to merge ${NODE_EXTRA_CA_CERTS_ENV} "${existing}" with ` + - `the daemon cert "${daemonCertPath}": ` + - `${err instanceof Error ? err.message : String(err)}; channel ` + - `workers will trust only the daemon cert`, + warnWorkerCaMergeFallback( + existing, + daemonCertPath, + err instanceof Error ? err.message : String(err), ); return daemonCertPath; } diff --git a/packages/cli/src/serve/process-env-guard.test.ts b/packages/cli/src/serve/process-env-guard.test.ts index 2568a170165..8a7f3a96ff3 100644 --- a/packages/cli/src/serve/process-env-guard.test.ts +++ b/packages/cli/src/serve/process-env-guard.test.ts @@ -149,7 +149,8 @@ const allowedProcessEnvAccesses = normalizeAllowances([ 'The serve entry point owns daemon bootstrap, feature flags, child-process defaults, and the launch-env loader scrub. ' + 'NODE_EXTRA_CA_CERTS is read from the daemon process environment on purpose: it is the trust store Node itself ' + 'already loaded for this process, so the worker TLS trust-gap check has to consult the same value to know whether ' + - 'an operator has already supplied the issuing CA.', + "an operator has already supplied the issuing CA. Read once into a local: the check now needs the file's " + + 'contents, not just the path, and a second read could see a different value.', accesses: { 'computed:EXTERNAL_TOOL_GUARD_TOKEN_ENV': 1, 'computed:QWEN_SERVER_TOKEN_ENV': 1, @@ -159,7 +160,7 @@ const allowedProcessEnvAccesses = normalizeAllowances([ 'computed:QWEN_SERVE_WRITER_IDLE_TIMEOUT_MS_ENV': 1, 'computed:RUNTIME_STARTUP_TIMEOUT_ENV': 1, 'key:DEV': 1, - 'key:NODE_EXTRA_CA_CERTS': 2, + 'key:NODE_EXTRA_CA_CERTS': 1, 'key:QWEN_CODE_IDE_WORKSPACE_PATH': 1, 'key:QWEN_SERVE_NO_MCP_POOL': 1, 'key:QWEN_SERVE_NO_PERSISTENT_REGISTRATION': 1, diff --git a/packages/cli/src/serve/run-qwen-serve.test.ts b/packages/cli/src/serve/run-qwen-serve.test.ts index f99d2e727d9..fd0ade0b3e2 100644 --- a/packages/cli/src/serve/run-qwen-serve.test.ts +++ b/packages/cli/src/serve/run-qwen-serve.test.ts @@ -1286,6 +1286,14 @@ B6U6dT+UdA== describe('describeWorkerTlsTrustGaps', () => { const daemonUrl = 'https://127.0.0.1:4170'; + /** The issuing root of TEST_TLS_CERT_FULLCHAIN, on its own. */ + const fullchainRootPem = (): string => { + const blocks = TEST_TLS_CERT_FULLCHAIN.match( + /-----BEGIN CERTIFICATE-----[^-]*-----END CERTIFICATE-----/g, + ); + return `${blocks!.at(-1)}\n`; + }; + it('reports nothing for a self-signed cert covering the dialled host', () => { expect( describeWorkerTlsTrustGaps({ @@ -1307,13 +1315,116 @@ describe('describeWorkerTlsTrustGaps', () => { expect(gaps[0]).toContain('qwen test root CA'); }); - it('stays quiet about a CA-issued cert when the operator supplies a CA', () => { + it('stays quiet when the operator CA actually anchors the chain', () => { + // R2-3: BEHAVIOUR FLIP. A set `operatorCaCertPath` used to suppress this + // gap on its own. A typo'd, unrelated or unloadable NODE_EXTRA_CA_CERTS + // anchors exactly as little as no CA at all, so coverage is now judged on + // the file's contents — the certificates the workers really receive. expect( describeWorkerTlsTrustGaps({ - cert: Buffer.from(TEST_TLS_CERT_CA_ISSUED), + cert: Buffer.from(TEST_TLS_CERT_FULLCHAIN_LEAF_ONLY), certPath: '/certs/daemon.pem', daemonUrl, operatorCaCertPath: '/certs/rootCA.pem', + operatorCaCert: Buffer.from(fullchainRootPem()), + }), + ).toEqual([]); + }); + + it('still names the gap when the operator CA does not anchor the chain', () => { + // R2-3(a): the merge in resolveWorkerCaCertPath cannot make an unrelated + // CA anchor this leaf, so the operator lands in exactly the boot-green / + // workers-looping mode this warning exists to name. + const gaps = describeWorkerTlsTrustGaps({ + cert: Buffer.from(TEST_TLS_CERT_CA_ISSUED), + certPath: '/certs/daemon.pem', + daemonUrl, + operatorCaCertPath: '/certs/unrelated.pem', + operatorCaCert: Buffer.from(fullchainRootPem()), + }); + expect(gaps).toHaveLength(1); + expect(gaps[0]).toContain('UNABLE_TO_VERIFY_LEAF_SIGNATURE'); + expect(gaps[0]).toContain('/certs/unrelated.pem'); + }); + + it('still names the gap when the operator CA path is unreadable', () => { + // R2-3(a): a set-but-unreadable path reaches the check with no contents. + const gaps = describeWorkerTlsTrustGaps({ + cert: Buffer.from(TEST_TLS_CERT_CA_ISSUED), + certPath: '/certs/daemon.pem', + daemonUrl, + operatorCaCertPath: '/certs/typo.pem', + }); + expect(gaps).toHaveLength(1); + expect(gaps[0]).toContain('UNABLE_TO_VERIFY_LEAF_SIGNATURE'); + expect(gaps[0]).toContain('/certs/typo.pem'); + }); + + it('softens the leaf-anchor gap for a CA already in the default trust store', () => { + // R2-3(c): the static model cannot see the workers' default trust store, + // so an issuer already anchored there makes this warning cry wolf. Say + // what the check actually knows instead of asserting a certain failure. + const gaps = describeWorkerTlsTrustGaps({ + cert: Buffer.from(TEST_TLS_CERT_CA_ISSUED), + certPath: '/certs/daemon.pem', + daemonUrl, + }); + expect(gaps[0]).toContain( + "unless the issuing CA is already in the workers' default trust store", + ); + }); + + it('names an expired chain member the signature-only walk accepts', () => { + // R2-3(b): `x509.verify` checks signatures and never consults dates, so an + // expired root anchors a fullchain "fine" here while every worker + // handshake fails CERT_HAS_EXPIRED. Boot validation covers the leaf alone, + // so nothing else would ever name it. The fixture root outlives its leaf + // by design, so the clock — not a second fullchain — is what moves. + vi.useFakeTimers(); + vi.setSystemTime(new Date('2130-01-01T00:00:00Z')); + try { + const gaps = describeWorkerTlsTrustGaps({ + cert: Buffer.from(TEST_TLS_CERT_FULLCHAIN), + certPath: '/certs/fullchain.pem', + daemonUrl, + }); + expect(gaps).toHaveLength(1); + expect(gaps[0]).toContain('CERT_HAS_EXPIRED'); + expect(gaps[0]).toContain('qwen fullchain test root CA'); + // The leaf's own dates are boot validation's job, not this warning's. + expect(gaps[0]).not.toContain('CN=localhost,'); + } finally { + vi.useRealTimers(); + } + }); + + it('names a chain member whose validity window has not started', () => { + // Symmetric to the expiry branch: clock skew or a freshly minted root + // fails every handshake CERT_NOT_YET_VALID with the same silent boot. + vi.useFakeTimers(); + vi.setSystemTime(new Date('2020-01-01T00:00:00Z')); + try { + const gaps = describeWorkerTlsTrustGaps({ + cert: Buffer.from(TEST_TLS_CERT_FULLCHAIN), + certPath: '/certs/fullchain.pem', + daemonUrl, + }); + expect(gaps).toHaveLength(1); + expect(gaps[0]).toContain('CERT_NOT_YET_VALID'); + expect(gaps[0]).toContain('qwen fullchain test root CA'); + } finally { + vi.useRealTimers(); + } + }); + + it('says nothing about certificates outside the anchor path', () => { + // The bundle may carry unrelated CAs; only the members the leaf's walk + // leans on are members whose validity the handshake enforces. + expect( + describeWorkerTlsTrustGaps({ + cert: Buffer.from(`${TEST_TLS_CERT}${TEST_TLS_CERT_EXPIRED}`), + certPath: '/certs/daemon.pem', + daemonUrl, }), ).toEqual([]); }); @@ -9212,6 +9323,75 @@ describe('runQwenServe channel worker supervisor', () => { } }); + async function bootTlsDaemonForTrustGapLog( + hostname: string, + ): Promise { + tmpDir = fs.realpathSync( + fs.mkdtempSync(path.join(os.tmpdir(), 'qws-channel-gap-')), + ); + const certPath = path.join(tmpDir, 'cert.pem'); + const keyPath = path.join(tmpDir, 'key.pem'); + fs.writeFileSync(certPath, TEST_TLS_CERT); + fs.writeFileSync(keyPath, TEST_TLS_KEY); + const logBaseDir = path.join(tmpDir, 'debug'); + const worker = makeWorker({ + enabled: true, + state: 'running', + pid: 1234, + channels: ['telegram'], + }); + const handle = await runQwenServe( + { + port: 0, + hostname, + mode: 'http-bridge', + workspace: tmpDir, + serveWebShell: false, + tlsCert: certPath, + tlsKey: keyPath, + channelSelection: { mode: 'names', names: ['telegram'] }, + }, + { + bridge: makeFakeBridge(), + channelWorkerSupervisorFactory: makeReadyWorkerFactory(worker), + channelServicePidfile: makePidfileDeps(), + daemonLogBaseDir: logBaseDir, + }, + ); + try { + await handle.runtimeReady; + } finally { + await handle.close(); + } + return fs.readFileSync( + path.join(logBaseDir, 'daemon', 'daemon.log'), + 'utf8', + ); + } + + it('writes a worker TLS trust gap to the daemon log at boot', async () => { + // R2-6: only the pure describeWorkerTlsTrustGaps was covered, so deleting + // this loop, inverting its `tlsOptions && tlsCertPath` guard or feeding it + // unresolved values all shipped green — and operators were back in the + // silent mode this diagnostic exists to end: daemon boots, /health green, + // every channel worker restart-looping with no log line saying why. The + // fixture cert covers 127.0.0.1 and localhost, so a ::1 bind is a real + // SAN gap on a cert that still pairs with its key and boots. + const log = await bootTlsDaemonForTrustGapLog('::1'); + + expect(log).toContain('ERR_TLS_CERT_ALTNAME_INVALID'); + expect(log).toContain('::1'); + }); + + it('keeps the daemon log quiet when the serving cert covers the dialled host', async () => { + // The other half of the wiring: a guard stuck on would bury real boot + // warnings under a gap every TLS daemon reports. + const log = await bootTlsDaemonForTrustGapLog('127.0.0.1'); + + expect(log).not.toContain('ERR_TLS_CERT_ALTNAME_INVALID'); + expect(log).not.toContain('UNABLE_TO_VERIFY_LEAF_SIGNATURE'); + }); + it('forwards webhook tasks through the channel worker group', async () => { tmpDir = fs.realpathSync( fs.mkdtempSync(path.join(os.tmpdir(), 'qws-channel-webhook-')), diff --git a/packages/cli/src/serve/run-qwen-serve.ts b/packages/cli/src/serve/run-qwen-serve.ts index b36c3a35c8b..19d00222ebf 100644 --- a/packages/cli/src/serve/run-qwen-serve.ts +++ b/packages/cli/src/serve/run-qwen-serve.ts @@ -699,6 +699,13 @@ export function describeWorkerTlsTrustGaps(opts: { certPath: string; daemonUrl: string; operatorCaCertPath?: string; + /** + * Contents of `operatorCaCertPath`, when it was readable. A path alone says + * nothing — a typo'd, unrelated or unloadable NODE_EXTRA_CA_CERTS anchors + * exactly as little as no CA at all, and treating "the variable is set" as + * coverage is what silenced the warning in the cases it was written for. + */ + operatorCaCert?: Buffer; }): string[] { // A serving file is routinely a fullchain (leaf + issuing CA in one PEM), // and the supervisor injects the whole file as the workers' @@ -711,22 +718,56 @@ export function describeWorkerTlsTrustGaps(opts: { return []; } const gaps: string[] = []; + // Exactly what a worker gets: the serving file merged with the operator's + // CA file (see resolveWorkerCaCertPath in channel-worker-supervisor.ts). + const workerTrustStore = opts.operatorCaCert + ? [...chain, ...parseCertChain(opts.operatorCaCert)] + : chain; // A leaf in NODE_EXTRA_CA_CERTS is a usable trust anchor only when it signed // itself: chain verification has no PARTIAL_CHAIN flag here, so a CA-issued // leaf (what the `mkcert` flow this project documents produces) never - // terminates the chain. An operator-set NODE_EXTRA_CA_CERTS is merged into - // the worker bundle and may already carry the issuing root, so only flag the - // case where the leaf is all the worker gets. - if (!opts.operatorCaCertPath && !chainIsSelfAnchored(chain)) { + // terminates the chain — unless something else in the worker's bundle + // carries the issuer that does. + const anchorPath = walkWorkerAnchorPath(workerTrustStore); + if (!anchorPath.anchored) { gaps.push( `--tls-cert "${opts.certPath}" is issued by another CA ` + - `(${x509.issuer.replace(/\r?\n/g, ', ')}), not self-signed, so the ` + - `certificate alone cannot anchor the channel workers' trust — every ` + - `worker handshake to the daemon will fail ` + - `UNABLE_TO_VERIFY_LEAF_SIGNATURE. Point NODE_EXTRA_CA_CERTS at the ` + + `(${x509.issuer.replace(/\r?\n/g, ', ')}), not self-signed, and ` + + `${ + opts.operatorCaCertPath + ? `NODE_EXTRA_CA_CERTS "${opts.operatorCaCertPath}" does not ` + + `carry a certificate that anchors it` + : `no NODE_EXTRA_CA_CERTS is set` + }, so nothing in the channel workers' bundle anchors their trust — ` + + `every worker handshake to the daemon will fail ` + + `UNABLE_TO_VERIFY_LEAF_SIGNATURE unless the issuing CA is already in ` + + `the workers' default trust store. Point NODE_EXTRA_CA_CERTS at the ` + `issuing CA (for mkcert: "$(mkcert -CAROOT)/rootCA.pem") and restart.`, ); } + // `X509Certificate.verify` checks signatures only and never consults dates, + // so an expired root or intermediate anchors "fine" here while every worker + // handshake fails CERT_HAS_EXPIRED. Boot validation covers the leaf alone. + const now = Date.now(); + for (const member of anchorPath.path) { + if (member.fingerprint256 === x509.fingerprint256) continue; + const subject = member.subject.replace(/\r?\n/g, ', '); + if (new Date(member.validTo).getTime() < now) { + gaps.push( + `--tls-cert "${opts.certPath}" chains through "${subject}", which ` + + `expired on ${member.validTo} — every worker handshake to the ` + + `daemon will fail CERT_HAS_EXPIRED. Renew that chain member and ` + + `restart.`, + ); + } else if (new Date(member.validFrom).getTime() > now) { + gaps.push( + `--tls-cert "${opts.certPath}" chains through "${subject}", which is ` + + `not yet valid (validFrom: ${member.validFrom}) — every worker ` + + `handshake to the daemon will fail CERT_NOT_YET_VALID. Check that ` + + `chain member's notBefore date or the system clock.`, + ); + } + } const host = workerDialHost(opts.daemonUrl); if (host && !certCoversHost(x509, host)) { gaps.push( @@ -788,16 +829,24 @@ function certIssuedBy(cert: X509Certificate, issuer: X509Certificate): boolean { } /** - * Whether the leaf's chain terminates inside the file itself. Workers get the - * whole file as their trust store, so a fullchain that walks up to a - * self-signed root anchors fine even though the leaf never could alone. + * Walks the leaf up through the certificates the workers actually hold, and + * reports both whether the walk terminated on a self-signed anchor and the + * certificates it relied on. Workers get the whole bundle as their trust + * store, so a fullchain that walks up to a self-signed root anchors fine even + * though the leaf never could alone — and every member the walk leaned on is + * a member whose own validity window the handshake will enforce. */ -function chainIsSelfAnchored(chain: readonly X509Certificate[]): boolean { +function walkWorkerAnchorPath(chain: readonly X509Certificate[]): { + anchored: boolean; + path: readonly X509Certificate[]; +} { let next: X509Certificate | undefined = chain[0]; const walked = new Set(); + const path: X509Certificate[] = []; while (next) { const current: X509Certificate = next; - if (isSelfSignedCert(current)) return true; + path.push(current); + if (isSelfSignedCert(current)) return { anchored: true, path }; walked.add(current.fingerprint256); next = chain.find( (candidate) => @@ -805,7 +854,7 @@ function chainIsSelfAnchored(chain: readonly X509Certificate[]): boolean { certIssuedBy(current, candidate), ); } - return false; + return { anchored: false, path }; } /** @@ -7275,13 +7324,22 @@ async function runQwenServeImpl( tlsOptions !== undefined, ); if (tlsOptions && tlsCertPath) { + const operatorCaCertPath = process.env['NODE_EXTRA_CA_CERTS']; + let operatorCaCert: Buffer | undefined; + if (operatorCaCertPath) { + try { + operatorCaCert = fs.readFileSync(operatorCaCertPath); + } catch { + // Unreadable: it anchors nothing, which is what the gap check + // concludes from the missing contents. + } + } for (const gap of describeWorkerTlsTrustGaps({ cert: tlsOptions.cert, certPath: tlsCertPath, daemonUrl: workerDaemonUrl, - ...(process.env['NODE_EXTRA_CA_CERTS'] - ? { operatorCaCertPath: process.env['NODE_EXTRA_CA_CERTS'] } - : {}), + ...(operatorCaCertPath ? { operatorCaCertPath } : {}), + ...(operatorCaCert ? { operatorCaCert } : {}), })) { daemonLog.warn(gap); } From 016208709d1af0fedaf6a4719d5012aeaff7988d Mon Sep 17 00:00:00 2001 From: qqqys Date: Wed, 19 Aug 2026 03:42:41 +0800 Subject: [PATCH 07/26] fix(serve): refuse a non-CA chain terminator and document the worker TLS hop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Clears the four findings still open on #9392 from earlier rounds that round 2 did not re-report inline. R2-10: `chainIsSelfAnchored` modelled chain geometry only. OpenSSL also requires a certificate that SIGNS others to carry `basicConstraints CA:TRUE`, so a fullchain of leaf + self-signed CA:FALSE issuer was blessed as anchored while every worker handshake failed INVALID_PURPOSE — boot green, no warning, the exact silent outage this diagnostic exists to name. Measured on Node 22 with a real `tls.connect`: leaf + CA:FALSE self-signed issuer as the trust store → `INVALID_PURPOSE: unsuitable certificate purpose`. The constraint binds only PAST the leaf. The same probe shows a CA:FALSE self-signed cert in its OWN trust store is verified at depth 0 and handshakes fine (`authorized=true`) — which is what plain `openssl req -x509` produces — so requiring CA:TRUE there would cry wolf on the ordinary self-signed daemon cert. `walkWorkerAnchorPath` now rejects a non-CA terminator only when the walk took at least one step, and reports it separately so the gap text names INVALID_PURPOSE and the CA:FALSE remedy rather than UNABLE_TO_VERIFY_LEAF_SIGNATURE. R2-10's other shape — an expired self-signed root — is already covered by the chain-date check added in the previous commit. Two fixtures back this: a leaf signed by a self-signed CA:FALSE issuer, and a self-signed CA:FALSE leaf with loopback SANs. Both were minted with OpenSSL 3.0.13 and are the exact files the handshake probes above ran against. R2-7: no case drove the function to a two-gap outcome, so an inserted `return gaps` after the first push — or turning the SAN `if` into an `else if` — survived the whole suite. Under that mutant an operator fixes the trust anchor, restarts, and only then meets the SAN failure. Added a CA-issued cert dialled at a host its SANs miss, asserting both error names. R2-8: the documented mkcert flow produces a CA-issued leaf — precisely the shape the new boot warning flags — but the docs never connected channel workers to TLS (`grep -c NODE_EXTRA_CA_CERTS docs/users/qwen-serve.md` → 0). Added the HTTPS/TLS note: workers dial the daemon back over https, self-signed certs and self-carrying fullchains need nothing, the mkcert flow needs `NODE_EXTRA_CA_CERTS="$(mkcert -CAROOT)/rootCA.pem"` exported in the daemon's launch environment, and an operator-set value is merged with the daemon cert rather than replacing it. R2-9: documented the rotation asymmetry on the `tlsCaCertPath` option, per the finding's stated minimum. With no operator CA the worker gets the `--tls-cert` PATH and Node re-reads it at every respawn while the daemon still serves its boot-time bytes, so an in-place rotation makes respawned workers restart-loop; with an operator CA the merged bundle pins a snapshot instead. Either way the rotation needs a daemon restart, now said in both the JSDoc and the serve docs. Verification: packages/cli — 510 tests pass across run-qwen-serve (286), channel-worker-supervisor (94), daemon-worker (85), process-env-guard (3) and channel-worker-group. eslint clean; prettier clean including docs/users/qwen-serve.md. `tsc --noEmit -p packages/cli` reports the same 2 pre-existing TS6305 errors against packages/core/dist that the stashed tree reports — worktree build skew, not this change. Mutation-verified, 3 of 3 new mutants killed: CA check removed, CA check applied to the leaf as well, and the SAN gap suppressed once a trust-anchor gap exists. Co-Authored-By: Claude Opus 5 --- docs/users/qwen-serve.md | 10 +- .../src/serve/channel-worker-supervisor.ts | 12 +- packages/cli/src/serve/run-qwen-serve.test.ts | 118 ++++++++++++++++++ packages/cli/src/serve/run-qwen-serve.ts | 27 +++- 4 files changed, 160 insertions(+), 7 deletions(-) diff --git a/docs/users/qwen-serve.md b/docs/users/qwen-serve.md index daf03659a36..a8b0dc0f372 100644 --- a/docs/users/qwen-serve.md +++ b/docs/users/qwen-serve.md @@ -377,6 +377,8 @@ Notes: - **Both flags or neither** — boot fails if only one is given (a cert with no key can't start an HTTPS listener). - **TLS is orthogonal to auth** — HTTPS encrypts the transport; the bearer token still gates every API route. Non-loopback binds require a token with or without TLS. - **Scope is TLS termination only** — no auto-generation, no ACME / Let's Encrypt. This is a LAN / dev convenience; for internet-facing deployments terminate TLS at a reverse proxy (see the threat model below). +- **Channel workers dial the daemon back over `https://`** — so they need to trust the serving certificate too. A self-signed cert (or a fullchain that carries its own root) needs nothing: the daemon injects it into each worker's `NODE_EXTRA_CA_CERTS`. The mkcert flow above is **CA-issued**, so the leaf alone cannot anchor the chain — export `NODE_EXTRA_CA_CERTS="$(mkcert -CAROOT)/rootCA.pem"` in the daemon's launch environment before starting with `--channel`. An operator-set value is _merged_ with the daemon cert, not replaced. Without it the daemon boots green while every channel worker restart-loops on `UNABLE_TO_VERIFY_LEAF_SIGNATURE`; the daemon log names the gap at boot. +- **Rotating `--tls-cert` in place needs a daemon restart** — the daemon serves the bytes it read at boot, so until it restarts, respawned workers can load newer contents than the daemon presents and their handshakes fail. ## CLI flags @@ -385,7 +387,7 @@ Notes: | `--port ` | `4170` | TCP port. `0` = OS-assigned ephemeral port. | | `--hostname ` | `127.0.0.1` | Bind interface. Anything beyond loopback requires a token. | | `--local-control` | `false` | Share the Web Shell on one selected private IPv4 interface with a daemon-owned revocable pairing token, terminal QR code, exact browser origin, and best-effort sleep inhibition. Composes with `--token`, `--allow-origin`, and `--port 0`; conflicts with `--no-web` and non-default `--hostname`. Use `--local-control-address` when multiple LAN candidates are available, and add `--tls-cert` + `--tls-key` for secure-context browser APIs such as voice input. | -| `--local-control-address ` | — | Which LAN IPv4 address to share when the host has more than one candidate. Only needed if `--local-control` reports an ambiguous choice. | +| `--local-control-address ` | — | Which LAN IPv4 address to share when the host has more than one candidate. Only needed if `--local-control` reports an ambiguous choice. | | `--token ` | — | Bearer token. Falls back to `QWEN_SERVER_TOKEN` env var (with leading/trailing whitespace stripped — handy for `$(cat token.txt)`). | | `--require-auth` | `false` | Refuse to start without a bearer token, even on loopback. Hardens the `127.0.0.1` developer default for shared dev hosts / CI runners / multi-tenant workstations where any local user can hit the listener. Boots only with `--token` or `QWEN_SERVER_TOKEN` set; gates `/health` behind the bearer too. | | `--tls-cert ` | — | Path to a PEM certificate file. Serve over **HTTPS** instead of HTTP. Must be paired with `--tls-key` (boot fails if only one is given). Unlocks secure-context browser APIs — voice input (`getUserMedia`), WebRTC — over a LAN IP, which browsers otherwise block on plain `http://`. TLS termination only; no auto-generation / ACME. See [HTTPS / TLS](#https--tls-for-mobile--cross-device-access) below. | @@ -674,9 +676,9 @@ Agent text writes (`write_file`, `edit`, `notebook_edit`, and the shell tool's s Operators whose deployment convention is umask-driven (e.g. a systemd unit with `UMask=0002`, shared-group repositories) can opt new files into the standard POSIX handling with: -| Env var | Values | Default | What it does | -| -------------------------- | ---------------------------- | ------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | -| `QWEN_SERVE_NEW_FILE_MODE` | `owner` \| `system` | `owner` | `system` creates NEW files at `0o666 & ~umask`, so agent-created files follow the daemon process's umask like any other process on the machine. `owner` keeps the umask-independent `0600` default. Values are case-insensitive; the literal `0600` is accepted as an alias for `owner` (no other octal modes are supported), and any other value is rejected with a stderr warning and the `0600` default is kept. | +| Env var | Values | Default | What it does | +| -------------------------- | ------------------- | ------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `QWEN_SERVE_NEW_FILE_MODE` | `owner` \| `system` | `owner` | `system` creates NEW files at `0o666 & ~umask`, so agent-created files follow the daemon process's umask like any other process on the machine. `owner` keeps the umask-independent `0600` default. Values are case-insensitive; the literal `0600` is accepted as an alias for `owner` (no other octal modes are supported), and any other value is rejected with a stderr warning and the `0600` default is kept. | Scope and limits: diff --git a/packages/cli/src/serve/channel-worker-supervisor.ts b/packages/cli/src/serve/channel-worker-supervisor.ts index f985e544c1b..ce1f6ee9bf0 100644 --- a/packages/cli/src/serve/channel-worker-supervisor.ts +++ b/packages/cli/src/serve/channel-worker-supervisor.ts @@ -227,7 +227,17 @@ export interface CreateChannelWorkerSupervisorOptions { workerBaseEnv?: Readonly; /** * PEM cert the worker must additionally trust when calling the daemon - * over a self-signed TLS listener. Injected via NODE_EXTRA_CA_CERTS. + * over a self-signed TLS listener, injected as their `NODE_EXTRA_CA_CERTS`. + * + * With no operator CA set this is handed over as a PATH, and Node re-reads + * it at every (re)spawn — while the daemon keeps serving the bytes it read + * at boot. Rotating this file in place without restarting the daemon + * therefore leaves respawned workers trusting the NEW contents against the + * OLD served cert, and they restart-loop until the daemon restarts. (With an + * operator CA set the merged bundle pins a snapshot instead, so the same + * rotation is invisible to workers until the daemon restarts.) Either way, + * rotating `--tls-cert` requires a daemon restart; see the HTTPS / TLS notes + * in docs/users/qwen-serve.md. */ tlsCaCertPath?: string; startupTimeoutMs?: number; diff --git a/packages/cli/src/serve/run-qwen-serve.test.ts b/packages/cli/src/serve/run-qwen-serve.test.ts index fd0ade0b3e2..326cef1e905 100644 --- a/packages/cli/src/serve/run-qwen-serve.test.ts +++ b/packages/cli/src/serve/run-qwen-serve.test.ts @@ -1283,6 +1283,80 @@ B6U6dT+UdA== -----END CERTIFICATE----- `; +/** + * A leaf signed by a SELF-SIGNED issuer carrying `basicConstraints CA:FALSE`. + * Chain geometry alone calls this anchored; OpenSSL refuses the issuer the + * purpose of signing and every handshake fails INVALID_PURPOSE. + */ +const TEST_TLS_CERT_FULLCHAIN_NON_CA_ROOT = `-----BEGIN CERTIFICATE----- +MIIDLzCCAhegAwIBAgIUZ0Gvb+9679AdH7Z3UTSH3bpFClUwDQYJKoZIhvcNAQEL +BQAwIjEgMB4GA1UEAwwXcXdlbiBub24tQ0EgdGVzdCBpc3N1ZXIwIBcNMjYwODE4 +MTkzNzUxWhgPMjEyNjA3MjUxOTM3NTFaMBQxEjAQBgNVBAMMCWxvY2FsaG9zdDCC +ASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBAPV+RuIkLGY4UssJmrTVgT2N ++6NC/Z5zFOscBbe06uFIr+Afe4663XDykVKRBpsW99lGi4MHhuF7+an3RLpNE6NW +cr8IFMcPWJs6PlvG72CGiO84cbjSWSu9HuZc9usQpryqaENB672xD80eIpazwfsQ +rR0lgqhf8jACcOmGjbkmXZ7V5GOLFESmOT2W6Pyph0dRWRyDJ9hONYUURCWhKsDq +KrUqEzR7hAYbAqp4MIY9pptAnZsWlTjClaynG9xh4OQckT+0kKXeA375AyMojb12 +y9yCqTHgIedkgMS9kJkzcuJCFaN8fl04XCEhK9YZFNteDD2UuNFWSljKSLFR588C +AwEAAaNpMGcwCQYDVR0TBAIwADAaBgNVHREEEzARgglsb2NhbGhvc3SHBH8AAAEw +HQYDVR0OBBYEFMDmmEJGwv7U1oNopBAgez9nxWjbMB8GA1UdIwQYMBaAFPMJzoYw +2Kpic33Wk1grw6CvY51dMA0GCSqGSIb3DQEBCwUAA4IBAQAV+WxQ3pMUOPQUT92t +3VzRD4k679a5NDB47MaXh0HMPqVbph0UQRg4+BAg8pSpf7tF0Ba84eUgLHUxNrEB +sLRew6Sm5HQn6hRnYj9PQaeWmKvCLRZmKmeF93z5QyNCLtHsqb5ttJKbt+yM7ESH +KiQcEC1LnZjFMgR9ItEuK8Xtjb6IVkN05V54DkF6cq/MVAX6dvRGCjo1jkRqFpNJ ++sil11vK3aAjD7BP3iw/v/1iYU2qLwcpXbqNXuM7ucVTvsxP5C1Ae4WTYvoZ2fid +r/EUKFMwxP7u9kLyOXi9EzKkb0zpVhfAcd8+yOyTZ4bs8Cc7kIEBYvcXhNL/BrVi +6wO7 +-----END CERTIFICATE----- +-----BEGIN CERTIFICATE----- +MIIDNDCCAhygAwIBAgIUVsyE+9hGRXVXW98Yxi5WgUsuAsAwDQYJKoZIhvcNAQEL +BQAwIjEgMB4GA1UEAwwXcXdlbiBub24tQ0EgdGVzdCBpc3N1ZXIwIBcNMjYwODE4 +MTkzNzUxWhgPMjEyNjA3MjUxOTM3NTFaMCIxIDAeBgNVBAMMF3F3ZW4gbm9uLUNB +IHRlc3QgaXNzdWVyMIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAmS4z +ZtAqYZ+SnMeyaHlTkV6+601FGfgglpa3th/4ZO7fSILfsDZIN7X3aAaz3u8lOhm4 +IIE5OuqNTX0Nn5CIEoxaDBjP81/pw0vgIqIjmjRacTzLU7Pe1Jl4TtbXKLTOP1At +nfnGuiHS+shw4vkqo47/C9OT18gJLvXuB/aIDYyI6DwCpUfljagrAHdmCvzfhKMj +EuYw3E+OIErLYb3THAvShVcrpsnuW1Csj8WRnWsmB+S6FttEKFM6C5GnALOuGnmZ +p9x39qbDtYZoKRHMGxt8V2LWVnnWbEAyFbGnkItI+dA2xjoCFKkUoIrtr1Z4A+jF +f9gqDd3c0UNRaQvQ1QIDAQABo2AwXjAdBgNVHQ4EFgQU8wnOhjDYqmJzfdaTWCvD +oK9jnV0wHwYDVR0jBBgwFoAU8wnOhjDYqmJzfdaTWCvDoK9jnV0wDAYDVR0TAQH/ +BAIwADAOBgNVHQ8BAf8EBAMCAoQwDQYJKoZIhvcNAQELBQADggEBAFvpSYNXbtl6 +RHE9Dt+GjRfBWAb73n7CVi+Ep7KBB4M/G7eHY9646T+Rzp0y/+mxEn2JDJP5yUhE +FLEOzQvXUrc0bdwWUYPbsKhG0p0KhK4+B34GogORcv3+6AiXBvGdqjyMT5zwj9OJ +sVV/Nswkn2dfkZr+JXj8sikllPjEn+LXto8nXNPbjZRe3MzIU32bYyOcJl+wQc4t +TJEPPVrdusShzahl0xEBAfuctVmmVvdv9st1DsoFiaEQ+vGZkJzTbhm6ggXyKpNE +1u3aBtPdxWWjuaF3Oy6jb+HG5BrQMemzExU1izkIg+cb9VJTZyP+cm0NjOp1Oo+E +c9BNon/M6I0= +-----END CERTIFICATE----- +`; + +/** + * A self-signed leaf with `basicConstraints CA:FALSE` covering the loopback + * host — the shape `openssl req -x509` produces without an explicit CA + * extension. Measured on Node 22: trusted as its own anchor at depth 0, it + * handshakes fine, so the CA constraint must not be applied here. + */ +const TEST_TLS_CERT_SELF_SIGNED_NON_CA = `-----BEGIN CERTIFICATE----- +MIIDJDCCAgygAwIBAgIUDFbAwso3M9+z+ZUt6pZ8vk1wS+QwDQYJKoZIhvcNAQEL +BQAwFDESMBAGA1UEAwwJbG9jYWxob3N0MCAXDTI2MDgxODE5MzgxM1oYDzIxMjYw +NzI1MTkzODEzWjAUMRIwEAYDVQQDDAlsb2NhbGhvc3QwggEiMA0GCSqGSIb3DQEB +AQUAA4IBDwAwggEKAoIBAQCyGf9lrTPBEDjZE91Mhw+0yISrs5UdR8ZF6UBntzIt +xjnqOmv4SSNo9Uj+Nr2Fm3YZ2SRnyFgGVyfHICHdPddbMEo9YnFyq9PQeDy1CU2X +l2YspCdcsgMzlUWwjqj2uaGdzW5a3kPP17VwoxMOFRJ0dMOnu/OLJQWK/2ouSR/2 +vDcGvVYV8oLwB60G6sgiWmxaBAoymfscU+ljFa9Q+FV8ma0Grtw6MU5g70Eo8hUT +vdmhh3QdXEqPphN8Ehj1jnLkzgheLj6WUtSj0mXNgx9WiW82Eql+caQ65wyL66Lx +kb23+Bp9jXFO4EQqcExx0del6sWA04aD4zPNzzeGaz6zAgMBAAGjbDBqMB0GA1Ud +DgQWBBQAKIylvj34CVhT0ywSlIHBUdQtcTAfBgNVHSMEGDAWgBQAKIylvj34CVhT +0ywSlIHBUdQtcTAMBgNVHRMBAf8EAjAAMBoGA1UdEQQTMBGCCWxvY2FsaG9zdIcE +fwAAATANBgkqhkiG9w0BAQsFAAOCAQEAc9QfHZfvh5+tFWOa+7zZeXtH6EazImKu +50iVfu4sI9noV+k+pA2WokMnShT3dDhp2DP0n2VRXet9CMhACz7KAEpgtpG7JTr6 +EIHFgT42V+/WVte/uxw2Uj5hfMoycvRCy8J8JFuGzdPKc2z7bn2angtXoQxZfOAk +VNRXkOxi5lPsuJ3bW8of2DI1/q1EJkR/Ha1gdzuk/h0W6JpO2epuzOkuwckPMuu2 +FWlN+yXXWHUsIHCosHSqesOhS4qlxDoYihsggPJ2rWnibwMr7t6GC0Bo5xsMRWFS +6gw2hOfWIeMnoRQ0ZkCB5t5z+RYPMBuCOMJoR8HxUOKm1EMHjVhoaQ== +-----END CERTIFICATE----- +`; + describe('describeWorkerTlsTrustGaps', () => { const daemonUrl = 'https://127.0.0.1:4170'; @@ -1500,6 +1574,50 @@ describe('describeWorkerTlsTrustGaps', () => { expect(gaps[0]).not.toContain('[fd00::1]'); }); + it('reports both gaps when a CA-issued cert also misses the dialled host', () => { + // R2-7: every other case produces 0 or 1 gap, so an inserted `return gaps` + // after the first push — or turning the SAN `if` into an `else if` — + // survived the whole suite. Under that mutant an operator fixes the trust + // anchor, restarts, and only then discovers the SAN failure. + const gaps = describeWorkerTlsTrustGaps({ + cert: Buffer.from(TEST_TLS_CERT_CA_ISSUED), + certPath: '/certs/daemon.pem', + daemonUrl: 'https://example.invalid:4170', + }); + expect(gaps).toHaveLength(2); + expect(gaps.join('\n')).toContain('UNABLE_TO_VERIFY_LEAF_SIGNATURE'); + expect(gaps.join('\n')).toContain('ERR_TLS_CERT_ALTNAME_INVALID'); + }); + + it('names a self-signed chain terminator that is not a CA', () => { + // R2-10: chain geometry alone blesses this file as anchored, but OpenSSL + // also requires a signing certificate to carry CA:TRUE. Measured on Node + // 22: leaf + CA:FALSE self-signed issuer as the trust store fails + // INVALID_PURPOSE, while boot stays green and warning-free. + const gaps = describeWorkerTlsTrustGaps({ + cert: Buffer.from(TEST_TLS_CERT_FULLCHAIN_NON_CA_ROOT), + certPath: '/certs/fullchain.pem', + daemonUrl, + }); + expect(gaps).toHaveLength(1); + expect(gaps[0]).toContain('INVALID_PURPOSE'); + expect(gaps[0]).toContain('qwen non-CA test issuer'); + }); + + it('keeps trusting a self-signed leaf that carries CA:FALSE', () => { + // The constraint binds only past the leaf: a CA:FALSE self-signed cert in + // its OWN trust store is verified at depth 0 and handshakes fine + // (measured on Node 22), so requiring CA:TRUE here would cry wolf on the + // ordinary `openssl req -x509` daemon cert. + expect( + describeWorkerTlsTrustGaps({ + cert: Buffer.from(TEST_TLS_CERT_SELF_SIGNED_NON_CA), + certPath: '/certs/self-signed.pem', + daemonUrl, + }), + ).toEqual([]); + }); + it('defers to the boot parse guard on an unreadable certificate', () => { expect( describeWorkerTlsTrustGaps({ diff --git a/packages/cli/src/serve/run-qwen-serve.ts b/packages/cli/src/serve/run-qwen-serve.ts index 19d00222ebf..319ef999ffb 100644 --- a/packages/cli/src/serve/run-qwen-serve.ts +++ b/packages/cli/src/serve/run-qwen-serve.ts @@ -729,7 +729,18 @@ export function describeWorkerTlsTrustGaps(opts: { // terminates the chain — unless something else in the worker's bundle // carries the issuer that does. const anchorPath = walkWorkerAnchorPath(workerTrustStore); - if (!anchorPath.anchored) { + if (anchorPath.nonCaTerminator) { + gaps.push( + `--tls-cert "${opts.certPath}" chains up to ` + + `"${anchorPath.nonCaTerminator.subject.replace(/\r?\n/g, ', ')}", ` + + `which is self-signed but carries basicConstraints CA:FALSE — ` + + `OpenSSL refuses to let it issue the certificates below it, so every ` + + `worker handshake to the daemon will fail INVALID_PURPOSE ` + + `("unsuitable certificate purpose"). Reissue that certificate with ` + + `CA:TRUE, or point NODE_EXTRA_CA_CERTS at a real CA that anchors the ` + + `chain, and restart.`, + ); + } else if (!anchorPath.anchored) { gaps.push( `--tls-cert "${opts.certPath}" is issued by another CA ` + `(${x509.issuer.replace(/\r?\n/g, ', ')}), not self-signed, and ` + @@ -839,6 +850,8 @@ function certIssuedBy(cert: X509Certificate, issuer: X509Certificate): boolean { function walkWorkerAnchorPath(chain: readonly X509Certificate[]): { anchored: boolean; path: readonly X509Certificate[]; + /** Set when the walk terminated on a self-signed cert that is not a CA. */ + nonCaTerminator?: X509Certificate; } { let next: X509Certificate | undefined = chain[0]; const walked = new Set(); @@ -846,7 +859,17 @@ function walkWorkerAnchorPath(chain: readonly X509Certificate[]): { while (next) { const current: X509Certificate = next; path.push(current); - if (isSelfSignedCert(current)) return { anchored: true, path }; + if (isSelfSignedCert(current)) { + // OpenSSL applies `basicConstraints CA:TRUE` to certificates that sign + // OTHER certificates, not to a self-signed leaf trusted at depth 0. + // Measured on Node 22: a CA:FALSE self-signed leaf in its own trust + // store handshakes fine, while the same shape used as an issuer fails + // INVALID_PURPOSE — so the constraint binds only past the leaf. + if (path.length > 1 && !current.ca) { + return { anchored: false, path, nonCaTerminator: current }; + } + return { anchored: true, path }; + } walked.add(current.fingerprint256); next = chain.find( (candidate) => From 53197cfe0a518550b8c162a2975a465667326012 Mon Sep 17 00:00:00 2001 From: qqqys Date: Wed, 19 Aug 2026 03:50:31 +0800 Subject: [PATCH 08/26] =?UTF-8?q?test(serve):=20guard=20the=20exec-bit=20p?= =?UTF-8?q?robe=20on=20Windows=20and=20pin=20the=20Browse=E2=80=A6=20gate?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 1 review findings on #9406 that belong to this PR's own diff. The other four Criticals (R1-1..R1-4) and R1-6 were reported against the stale copy of #9392's channel-worker TLS commit this branch stacks on; they are cleared by merging #9392's reviewed head, not re-fixed here. R1-5 (Critical): `rejects a zenity without the executable bit on Linux` mocks `process.platform` to 'linux' but exercises the real host filesystem. Windows has no exec bit — libuv's `fs__access` ignores X_OK entirely, so `fs.accessSync` succeeds for any existing path and the probe returns true there, failing `.toBe(false)` deterministically. ci.yml's `test_windows` job is a merge-queue gate that collects the whole packages/cli suite, so this would block the PR in the queue. Guarded with `it.skipIf(process.platform === 'win32')`, the shape packages/core/src/utils/shellContextEnv.test.ts:157 already uses. The suite stays 21/21 on Linux, so the assertion still executes where the semantics it asserts exist. R1-7: nothing observed the `{onPick && (` guard in AddWorkspaceDialog. App.test.tsx mocks the dialog out entirely so it structurally cannot see the dialog DOM, and every `browseButton()` call site in AddWorkspaceDialog.test.tsx sits in a test that passes `onPick`. Turning that guard into an unconditional render therefore shipped green — and on a headless daemon host that puts back exactly the dead affordance this PR removes: a Browse… button whose handler returns immediately. Added two component tests: mounted without `onPick`, no Browse… button is in the document; mounted with it, the button is. Verification: packages/cli native-directory-picker 21 pass, packages/web-shell AddWorkspaceDialog 36 pass; eslint and prettier clean. Mutation-verified: `{onPick && (` → `{true && (` turns the new test red (1 failed / 35 passed). R1-5 has no production code to mutate; it is a platform guard on a test, verified by the suite still running green on Linux rather than being skipped there. Pre-existing and NOT from this change: `server.test.ts > accepts channel display text only from the workspace worker` fails in this worktree. It fails identically on c1161a736c, this branch's head before the merge; this PR's diff does not touch `promptDisplayText` in server.ts; and the test text is identical on upstream/main. A clean upstream/main baseline could not be taken in this worktree — the suite refuses to start there for missing `packages/acp-bridge/dist/sessionMedia.js` build output. CI reports this PR's checks green. Co-Authored-By: Claude Opus 5 --- .../src/serve/native-directory-picker.test.ts | 27 ++++++++++++------- .../dialogs/AddWorkspaceDialog.test.tsx | 20 ++++++++++++++ 2 files changed, 38 insertions(+), 9 deletions(-) diff --git a/packages/cli/src/serve/native-directory-picker.test.ts b/packages/cli/src/serve/native-directory-picker.test.ts index 375e0e98e67..2bb347d5e2e 100644 --- a/packages/cli/src/serve/native-directory-picker.test.ts +++ b/packages/cli/src/serve/native-directory-picker.test.ts @@ -242,15 +242,24 @@ describe('isNativeDirectoryPickerAvailable', () => { ).toBe(true); }); - it('rejects a zenity without the executable bit on Linux', () => { - setPlatform('linux'); - expect( - isNativeDirectoryPickerAvailable({ - DISPLAY: ':0', - PATH: nonExecutableZenityDir, - }), - ).toBe(false); - }); + // Windows has no exec bit: libuv's fs__access ignores X_OK entirely, so + // fs.accessSync succeeds for any existing path and this probe returns true + // there. `process.platform` is mocked to 'linux' but the filesystem is the + // real host's, so the assertion cannot hold on a Windows runner — and + // ci.yml's merge-queue `test_windows` job collects this file. Same shape as + // packages/core/src/utils/shellContextEnv.test.ts:157. + it.skipIf(process.platform === 'win32')( + 'rejects a zenity without the executable bit on Linux', + () => { + setPlatform('linux'); + expect( + isNativeDirectoryPickerAvailable({ + DISPLAY: ':0', + PATH: nonExecutableZenityDir, + }), + ).toBe(false); + }, + ); it('rejects a directory named zenity on PATH on Linux', () => { setPlatform('linux'); diff --git a/packages/web-shell/client/components/dialogs/AddWorkspaceDialog.test.tsx b/packages/web-shell/client/components/dialogs/AddWorkspaceDialog.test.tsx index 290942ba5fc..5256528a10f 100644 --- a/packages/web-shell/client/components/dialogs/AddWorkspaceDialog.test.tsx +++ b/packages/web-shell/client/components/dialogs/AddWorkspaceDialog.test.tsx @@ -75,6 +75,26 @@ describe('AddWorkspaceDialog', () => { expect(document.activeElement).toBe(input()); }); + it('renders no Browse… button when the host cannot pick a directory', () => { + // #9406 R1-7: every other browseButton() call site passes onPick, and + // App.test.tsx mocks this dialog out entirely, so nothing observed the + // `{onPick && (` guard — turning it into an unconditional render shipped + // green. On a headless daemon host that puts back the dead affordance + // this PR exists to remove: a Browse… button whose handler returns + // immediately. + mount(); + + expect(browseButton()).toBeUndefined(); + }); + + it('renders the Browse… button when the host can pick a directory', () => { + mount( + , + ); + + expect(browseButton()).toBeDefined(); + }); + it('hides the display name field unless the daemon supports it', () => { mount(); From 40d5240c42db18fe9105589af3247e12ec2f2d3b Mon Sep 17 00:00:00 2001 From: qqqys Date: Wed, 19 Aug 2026 06:40:43 +0800 Subject: [PATCH 09/26] fix(serve): judge worker CA files with the loader's own rules, on both sides MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 3 review: 2 Critical (R3-1, R3-2) and 6 Suggestions (R3-3..R3-8). R3-1 (Critical) — `extractCertificateBlocks` diverged from Node's NODE_EXTRA_CA_CERTS loader in both directions. Too lax: it validated block *shape* only, so a body of base64 characters that does not decode was merged ahead of the daemon cert, and Node then discarded the WHOLE bundle — workers lost trust in the operator CA *and* the daemon cert while /health stayed green. Each block is now parsed with `X509Certificate`, the loader's own parser. Too strict: a UTF-8 BOM, trailing whitespace after a marker line and leading whitespace on body lines were all rejected into the daemon-cert-only fallback with a warning that misdiagnosed the file; they are normalised away before matching. R3-2 (Critical) — the boot-time trust-gap diagnostic modelled the operator's CA with a looser parser than the spawn-time merge: a fused-marker bundle, or a DER file NODE_EXTRA_CA_CERTS never reads, was counted as an anchoring CA at boot while the merge discarded it and handed workers the daemon cert alone. The daemon log stayed clean and every worker handshake failed UNABLE_TO_VERIFY_LEAF_SIGNATURE — the exact silence this diagnostic exists to end. Both sides now share one extractor, moved to `pem-certificate-blocks.ts`, and an unloadable operator file is named in a gap instead of being trusted. R3-8 (behaviour flip) — `X509Certificate.ca` reads false both for an explicit `basicConstraints CA:FALSE` and for a v1/no-extension root, but OpenSSL accepts the second as an issuer. The INVALID_PURPOSE boot warning therefore fired on legacy anchors that work, telling operators to reissue a working CA. It now fires only when the certificate carries the extension and declares CA:FALSE. Measured on Node 22 / OpenSSL 3: a leaf anchored by a v1 root handshakes authorized=true, while the explicit CA:FALSE twin really does fail INVALID_PURPOSE. R3-5 — `warnWorkerCaMergeFallback` re-emitted on every spawn, so a crash-looping worker buried the log stream the operator reads to diagnose it. Deduped per path pair, keyed on the paths alone so flapping errno text cannot defeat it. R3-3 — the `tlsCaCertPath` comment claimed an operator CA pins a snapshot and makes in-place `--tls-cert` rotation invisible to workers. The code does the opposite: `resolveWorkerCaCertPath` stamps both sources, so rotation rebuilds the bundle from the new contents. Corrected to match the code and docs/users/qwen-serve.md:381. R3-6 — the probe is right that no test kills `mergedWorkerCaBundles.delete(cacheKey)`, but no test can: control always reaches the rebuild, which overwrites the key on success, and every hit re-stats the bundle and re-compares both stamps before returning it. The statement could not change an observable result, so it is removed rather than pinned by a test that would pass without it. The eviction *behaviour* stays covered by the rotation and tmp-cleaner tests. R3-4, R3-7 — new coverage: a CRLF operator bundle, a BOM operator bundle, a marker/body-whitespace bundle, an undecodable block, warn-once-per-pair, and three boot-log tests that drive the `process.env['NODE_EXTRA_CA_CERTS']` read and its try/catch end to end through `runQwenServe`. Every fix was mutation-verified: reverting each one turns exactly its own test(s) red (9 mutants, 9 kills). The loader claims above were measured against a real NODE_EXTRA_CA_CERTS handshake on Node 22.23, not inferred. --- .../serve/channel-worker-supervisor.test.ts | 133 +++++++++ .../src/serve/channel-worker-supervisor.ts | 61 ++-- .../cli/src/serve/pem-certificate-blocks.ts | 92 ++++++ packages/cli/src/serve/run-qwen-serve.test.ts | 282 +++++++++++++++++- packages/cli/src/serve/run-qwen-serve.ts | 59 +++- 5 files changed, 585 insertions(+), 42 deletions(-) create mode 100644 packages/cli/src/serve/pem-certificate-blocks.ts diff --git a/packages/cli/src/serve/channel-worker-supervisor.test.ts b/packages/cli/src/serve/channel-worker-supervisor.test.ts index 99f56317125..1f0c08c45e2 100644 --- a/packages/cli/src/serve/channel-worker-supervisor.test.ts +++ b/packages/cli/src/serve/channel-worker-supervisor.test.ts @@ -585,6 +585,139 @@ describe('createChannelWorkerSupervisor', () => { fs.rmSync(dir, { recursive: true, force: true }); }); + it('keeps the daemon cert when an operator CA block does not decode', async () => { + // R3-1(lax arm): the marker check validates block SHAPE only. A body made + // of base64 *characters* that does not decode (one misplaced `=` in a + // truncated or hand-edited cert) passed it, was merged ahead of the daemon + // cert, and Node's loader then discarded the WHOLE bundle with `bad base64 + // decode` — measured on Node 22: the worker ends up trusting NEITHER the + // operator CA nor the daemon cert, while /health stays green. + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'qwen-ca-badb64-')); + const operatorCa = path.join(dir, 'operator.pem'); + const daemonCa = path.join(dir, 'daemon.pem'); + const lines = OPERATOR_CA_PEM.trimEnd().split('\n'); + const body = Math.floor(lines.length / 2); + lines[body] = `${lines[body]!.slice(0, 10)}=${lines[body]!.slice(11)}`; + const corrupted = `${lines.join('\n')}\n`; + // Still matches the marker/alphabet shape — only decoding tells them apart. + expect(corrupted).toMatch( + /^-----BEGIN CERTIFICATE-----\n(?:[A-Za-z0-9+/=]+\n)+-----END CERTIFICATE-----\n$/, + ); + fs.writeFileSync(operatorCa, corrupted); + fs.writeFileSync(daemonCa, DAEMON_CERT_PEM); + + const { env } = await startWorkerWithCaPaths(daemonCa, operatorCa); + + expect(env['NODE_EXTRA_CA_CERTS']).toBe(daemonCa); + fs.rmSync(dir, { recursive: true, force: true }); + }); + + it('merges a CRLF-terminated operator CA file', async () => { + // R3-4: the CRLF normalization is the only thing keeping Windows-edited + // and vendor-exported bundles out of the daemon-cert-only fallback — + // Node's loader accepts CRLF PEM (measured: NODE_EXTRA_CA_CERTS with a + // CRLF root handshakes authorized=true), so rejecting it would drop an + // operator CA the loader would have taken. + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'qwen-ca-crlf-')); + const operatorCa = path.join(dir, 'operator.pem'); + const daemonCa = path.join(dir, 'daemon.pem'); + fs.writeFileSync(operatorCa, OPERATOR_CA_PEM.replace(/\n/g, '\r\n')); + fs.writeFileSync(daemonCa, DAEMON_CERT_PEM); + + const { env } = await startWorkerWithCaPaths(daemonCa, operatorCa); + + const bundlePath = env['NODE_EXTRA_CA_CERTS']!; + expect(bundlePath).not.toBe(daemonCa); + // Normalized to LF on the way in, so the bundle is canonical PEM. + expect(fs.readFileSync(bundlePath, 'utf8')).toBe( + `${OPERATOR_CA_PEM.trimEnd()}\n${DAEMON_CERT_PEM.trimEnd()}\n`, + ); + fs.rmSync(path.dirname(bundlePath), { recursive: true, force: true }); + fs.rmSync(dir, { recursive: true, force: true }); + }); + + it('merges an operator CA file behind a UTF-8 BOM', async () => { + // R3-1(strict arm): a corporate bundle saved by Windows tooling carries a + // BOM. Node's loader reads it fine (measured), so rejecting it sent the + // operator to edit a file that was never the problem. + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'qwen-ca-bom-')); + const operatorCa = path.join(dir, 'operator.pem'); + const daemonCa = path.join(dir, 'daemon.pem'); + fs.writeFileSync(operatorCa, `\uFEFF${OPERATOR_CA_PEM}`); + fs.writeFileSync(daemonCa, DAEMON_CERT_PEM); + + const { env } = await startWorkerWithCaPaths(daemonCa, operatorCa); + + const bundlePath = env['NODE_EXTRA_CA_CERTS']!; + expect(bundlePath).not.toBe(daemonCa); + expect(fs.readFileSync(bundlePath, 'utf8')).toBe( + `${OPERATOR_CA_PEM.trimEnd()}\n${DAEMON_CERT_PEM.trimEnd()}\n`, + ); + fs.rmSync(path.dirname(bundlePath), { recursive: true, force: true }); + fs.rmSync(dir, { recursive: true, force: true }); + }); + + it('merges an operator CA file with marker and body whitespace', async () => { + // R3-1(strict arm): Node's loader also accepts trailing whitespace after a + // marker line and leading whitespace on body lines (measured through a + // real NODE_EXTRA_CA_CERTS handshake), so the line-anchored match must not + // send either shape to the daemon-cert-only fallback. + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'qwen-ca-ws-')); + const operatorCa = path.join(dir, 'operator.pem'); + const daemonCa = path.join(dir, 'daemon.pem'); + const padded = OPERATOR_CA_PEM.split('\n') + .map((line) => + line.startsWith('-----') + ? `${line} ` + : line === '' + ? line + : ` ${line}`, + ) + .join('\n'); + fs.writeFileSync(operatorCa, padded); + fs.writeFileSync(daemonCa, DAEMON_CERT_PEM); + + const { env } = await startWorkerWithCaPaths(daemonCa, operatorCa); + + const bundlePath = env['NODE_EXTRA_CA_CERTS']!; + expect(bundlePath).not.toBe(daemonCa); + expect(fs.readFileSync(bundlePath, 'utf8')).toBe( + `${OPERATOR_CA_PEM.trimEnd()}\n${DAEMON_CERT_PEM.trimEnd()}\n`, + ); + fs.rmSync(path.dirname(bundlePath), { recursive: true, force: true }); + fs.rmSync(dir, { recursive: true, force: true }); + }); + + it('warns once per path pair, not once per spawn', async () => { + // R3-5: every fallback branch returns without caching and `launch()` + // rebuilds the env on each 'initial'/'restart' spawn, while + // `process.emitWarning` does not dedup identical text — so a crash-looping + // worker appended one identical multi-line warning per restart, burying + // the log stream the operator reads to diagnose the loop. + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'qwen-ca-dedup-')); + const operatorCa = path.join(dir, 'operator.pem'); + const daemonCa = path.join(dir, 'daemon.pem'); + fs.writeFileSync( + operatorCa, + `${OPERATOR_CA_PEM.trimEnd()}${OPERATOR_CA_PEM}`, + ); + fs.writeFileSync(daemonCa, DAEMON_CERT_PEM); + const warnings: string[] = []; + const onWarning = (warning: Error) => warnings.push(warning.message); + process.on('warning', onWarning); + + await startWorkerWithCaPaths(daemonCa, operatorCa); + await startWorkerWithCaPaths(daemonCa, operatorCa); + await startWorkerWithCaPaths(daemonCa, operatorCa); + + await new Promise((resolve) => setImmediate(resolve)); + process.off('warning', onWarning); + expect( + warnings.filter((message) => message.includes(operatorCa)), + ).toHaveLength(1); + fs.rmSync(dir, { recursive: true, force: true }); + }); + it('leaves the private key of a combined operator PEM out of the bundle', async () => { // R2-13: boot validation parses the first block only, so a combined // cert+key PEM serves fine — and copying its key into a tmpdir bundle diff --git a/packages/cli/src/serve/channel-worker-supervisor.ts b/packages/cli/src/serve/channel-worker-supervisor.ts index ce1f6ee9bf0..a6d3b44aa82 100644 --- a/packages/cli/src/serve/channel-worker-supervisor.ts +++ b/packages/cli/src/serve/channel-worker-supervisor.ts @@ -5,6 +5,7 @@ import * as fs from 'node:fs'; import * as os from 'node:os'; import * as path from 'node:path'; import { channelSelectionNames } from './channel-selection.js'; +import { extractCertificateBlocks } from './pem-certificate-blocks.js'; import type { ServeChannelSelection } from './types.js'; import { CHANNEL_DAEMON_WORKER_SENTINEL, @@ -233,11 +234,12 @@ export interface CreateChannelWorkerSupervisorOptions { * it at every (re)spawn — while the daemon keeps serving the bytes it read * at boot. Rotating this file in place without restarting the daemon * therefore leaves respawned workers trusting the NEW contents against the - * OLD served cert, and they restart-loop until the daemon restarts. (With an - * operator CA set the merged bundle pins a snapshot instead, so the same - * rotation is invisible to workers until the daemon restarts.) Either way, - * rotating `--tls-cert` requires a daemon restart; see the HTTPS / TLS notes - * in docs/users/qwen-serve.md. + * OLD served cert, and they restart-loop until the daemon restarts. An + * operator CA gives no cover: `resolveWorkerCaCertPath` stamps BOTH sources, + * so rotating this file in place invalidates the merged bundle and rebuilds + * it from the NEW contents — the same restart loop. Either way, rotating + * `--tls-cert` requires a daemon restart; see the HTTPS / TLS notes in + * docs/users/qwen-serve.md. */ tlsCaCertPath?: string; startupTimeoutMs?: number; @@ -411,43 +413,25 @@ function sourceStamp(filePath: string): string { } /** - * A PEM certificate block with every marker alone on its own line. Node's - * certificate loader is line-strict AND all-or-nothing: one fused - * `-----END CERTIFICATE----------BEGIN CERTIFICATE-----` (what - * `cat a.pem b.pem` produces when `a.pem` has no trailing newline) makes it - * discard the WHOLE bundle with `bad end line` — including the daemon cert - * appended after it, so the workers lose the trust the merge exists to give - * them. `tls.createSecureContext({ ca })` does not throw on that shape, so it - * cannot stand in as the validator. Base64 never contains `-`, so the body - * match cannot run past its own end marker or backtrack. + * Path pairs already warned about, keyed the way `mergedWorkerCaBundles` is. + * Every fallback branch returns without caching, `launch()` rebuilds the env + * on every 'initial' and 'restart' spawn, and `process.emitWarning` does not + * dedup identical text — so without this a crash-looping worker appends one + * identical multi-line warning per restart, burying the very log stream the + * operator reads to diagnose the loop. */ -const STRICT_PEM_CERTIFICATE_BLOCK = - /^-----BEGIN CERTIFICATE-----\n(?:[A-Za-z0-9+/=]+\n)+-----END CERTIFICATE-----$/gm; - -/** - * The certificate blocks of `contents`, or `undefined` when Node's loader - * would reject the file: no block at all, or a `BEGIN CERTIFICATE` marker - * that did not yield a well-formed block. - * - * Only certificate blocks come back. A combined cert+key serving PEM passes - * boot validation (which parses the first block alone), and copying its - * private key into a tmpdir bundle `NODE_EXTRA_CA_CERTS` never reads would - * leave key material behind a SIGKILLed daemon, where the `exit` cleanup - * cannot run. - */ -function extractCertificateBlocks(contents: string): string[] | undefined { - const normalized = contents.replace(/\r\n/g, '\n'); - const blocks = normalized.match(STRICT_PEM_CERTIFICATE_BLOCK) ?? []; - const markers = normalized.match(/-----BEGIN CERTIFICATE-----/g) ?? []; - if (blocks.length === 0 || blocks.length !== markers.length) return undefined; - return blocks; -} +const warnedWorkerCaMergeFallbacks = new Set(); function warnWorkerCaMergeFallback( operatorCaPath: string, daemonCertPath: string, reason: string, ): void { + // Keyed on the paths alone: `reason` varies with errno text, and keying on + // it would let a flapping error message defeat the dedup. + const key = `${operatorCaPath}\0${daemonCertPath}`; + if (warnedWorkerCaMergeFallbacks.has(key)) return; + warnedWorkerCaMergeFallbacks.add(key); // Falling back to the daemon cert alone silently drops the operator CA // the merge above exists to preserve, and Node says nothing when the // remaining cert loads fine — so say it here. @@ -503,7 +487,12 @@ function resolveWorkerCaCertPath( } catch { // Unreadable source or vanished bundle: rebuild below. } - mergedWorkerCaBundles.delete(cacheKey); + // No eviction here on purpose. Control always reaches the rebuild below, + // which overwrites this key on success, and every future hit re-stats the + // bundle and re-compares both stamps before returning it — so a stale + // entry can never be handed out, and deleting it changes no observable + // behaviour. The rebuild itself is pinned by the rotation and tmp-cleaner + // tests. } try { const sourceStamps = sources.map(sourceStamp); diff --git a/packages/cli/src/serve/pem-certificate-blocks.ts b/packages/cli/src/serve/pem-certificate-blocks.ts new file mode 100644 index 00000000000..151121eca56 --- /dev/null +++ b/packages/cli/src/serve/pem-certificate-blocks.ts @@ -0,0 +1,92 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import { X509Certificate } from 'node:crypto'; + +/** + * A PEM certificate block with every marker alone on its own line. Node's + * certificate loader is line-strict AND all-or-nothing: one fused + * `-----END CERTIFICATE----------BEGIN CERTIFICATE-----` (what + * `cat a.pem b.pem` produces when `a.pem` has no trailing newline) makes it + * discard the WHOLE bundle with `bad end line` — including any cert appended + * after it, so the workers lose the trust the merge exists to give them. + * `tls.createSecureContext({ ca })` does not throw on that shape, so it cannot + * stand in as the validator. Base64 never contains `-`, so the body match + * cannot run past its own end marker or backtrack. + */ +const STRICT_PEM_CERTIFICATE_BLOCK = + /^-----BEGIN CERTIFICATE-----\n(?:[A-Za-z0-9+/=]+\n)+-----END CERTIFICATE-----$/gm; + +/** + * Everything Node's certificate loader tolerates that the line-anchored match + * above does not: a UTF-8 BOM before the first block (what Windows tooling + * writes), CRLF terminators, trailing whitespace after a marker line, and + * leading whitespace on body lines. Measured on Node 22 through a real + * `NODE_EXTRA_CA_CERTS` handshake: all four shapes load and verify. Matching + * them verbatim would drop an operator CA the loader would have accepted, and + * blame the file for holding no loadable block. + */ +function normalizeForCertificateMatch(contents: string): string { + return contents + .replace(/^\uFEFF/, '') + .replace(/\r\n/g, '\n') + .replace(/^[ \t]+|[ \t]+$/gm, ''); +} + +/** + * The certificate blocks of `contents`, or `undefined` when Node's loader + * would reject the file: no block at all, a `BEGIN CERTIFICATE` marker that + * did not yield a well-formed block, or a block whose body does not decode. + * + * Only certificate blocks come back. A combined cert+key serving PEM passes + * boot validation (which parses the first block alone), and copying its + * private key into a tmpdir bundle `NODE_EXTRA_CA_CERTS` never reads would + * leave key material behind a SIGKILLed daemon, where the `exit` cleanup + * cannot run. + * + * Both the spawn-time merge (`resolveWorkerCaCertPath`) and the boot-time + * trust-gap diagnostic (`describeWorkerTlsTrustGaps`) go through here. Judging + * the same file with two different parsers is what let a fused or DER operator + * bundle be counted as an anchor at boot while the merge discarded it — the + * daemon log stayed clean while every worker handshake failed. + */ +export function extractCertificateBlocks( + contents: string, +): string[] | undefined { + const normalized = normalizeForCertificateMatch(contents); + const blocks = normalized.match(STRICT_PEM_CERTIFICATE_BLOCK) ?? []; + const markers = normalized.match(/-----BEGIN CERTIFICATE-----/g) ?? []; + if (blocks.length === 0 || blocks.length !== markers.length) return undefined; + // Shape is not loadability. A body made only of base64 *characters* still + // matches above while failing to decode (one misplaced `=` in a truncated or + // hand-edited cert), and Node's loader is all-or-nothing on that too: it + // discards the WHOLE bundle with `bad base64 decode`, taking the cert + // appended after it down with it — the very failure the marker check exists + // to prevent, through a sibling entrance. `X509Certificate` is the loader's + // own parser, so let it decide. Measured on Node 22: a corrupted block plus a + // good daemon cert leaves the worker trusting neither. + for (const block of blocks) { + try { + new X509Certificate(block); + } catch { + return undefined; + } + } + return blocks; +} + +/** + * The certificates a worker's loader would actually take from `contents`, or + * `undefined` when it would take none of them. + */ +export function loadableCertificates( + contents: string, +): X509Certificate[] | undefined { + const blocks = extractCertificateBlocks(contents); + if (!blocks) return undefined; + // `extractCertificateBlocks` already parsed each block, so this cannot throw. + return blocks.map((block) => new X509Certificate(block)); +} diff --git a/packages/cli/src/serve/run-qwen-serve.test.ts b/packages/cli/src/serve/run-qwen-serve.test.ts index 326cef1e905..c3e3fcb6d0d 100644 --- a/packages/cli/src/serve/run-qwen-serve.test.ts +++ b/packages/cli/src/serve/run-qwen-serve.test.ts @@ -7,6 +7,7 @@ import * as os from 'node:os'; import * as path from 'node:path'; import * as fs from 'node:fs'; +import { X509Certificate } from 'node:crypto'; import { createServer } from 'node:http'; import * as https from 'node:https'; import type { AddressInfo } from 'node:net'; @@ -1357,6 +1358,53 @@ FWlN+yXXWHUsIHCosHSqesOhS4qlxDoYihsggPJ2rWnibwMr7t6GC0Bo5xsMRWFS -----END CERTIFICATE----- `; +/** + * A leaf signed by an X.509 **v1** self-signed root — no extensions at all, so + * no `basicConstraints` either. `X509Certificate.ca` reads `false` for it + * exactly as it does for an explicit `CA:FALSE`, but OpenSSL accepts a v1 cert + * as an issuer (`X509_check_ca` returns 3, not 2). Measured on Node 22 / + * OpenSSL 3 with this very pair: serving the leaf with this root as the trust + * store handshakes `authorized: true, status 200`. + */ +const TEST_TLS_CERT_FULLCHAIN_V1_ROOT = `-----BEGIN CERTIFICATE----- +MIIDTTCCAjWgAwIBAgIUFRagk0s8Vw5T5dtWVS4OF+GJC2YwDQYJKoZIhvcNAQEL +BQAwHDEaMBgGA1UEAwwRcXdlbiB2MSB0ZXN0IHJvb3QwIBcNMjYwODE4MjIyOTM5 +WhgPMjEyNjA3MjUyMjI5MzlaMBQxEjAQBgNVBAMMCWxvY2FsaG9zdDCCASIwDQYJ +KoZIhvcNAQEBBQADggEPADCCAQoCggEBALB1h9AB5sJAdQaa0FRmK0EfDaVpaftc +OgsNYp7jkjBdvE/DKcINxoOOTUyO2Qs0/Q0lyWaZNjb5jTjzSqya+XBM9hYJdzHC +AJaNAPE86v/tS3sAsrCT1nKLjlPsHQsDcpyiQR3mJNjlAKOclrey2o44xBkhdiu4 +qBW1p1MEJe3tfu4rpH3//cDj6//T44ic9oNhIKH3hW+3QRunhqc/RCllAz82P37M +bI2/nQzWuBMhuGj4RHZN6njkhIlAqdyIt9Qgv2v7NPtCde5r0UiR6bLaSHpx4m9o +YmY7F7A9UOdUuRB+FY97/kzF7I9JmLH+/U9gyVcR0x+ML0SvXVN1Ph0CAwEAAaOB +jDCBiTAaBgNVHREEEzARgglsb2NhbGhvc3SHBH8AAAEwCQYDVR0TBAIwADAdBgNV +HQ4EFgQURZaMk15Cc6OZB/hbGR13/X/tAe8wQQYDVR0jBDowOKEgpB4wHDEaMBgG +A1UEAwwRcXdlbiB2MSB0ZXN0IHJvb3SCFHH+EHqV3xnb3Gh9bPdocEReVG0hMA0G +CSqGSIb3DQEBCwUAA4IBAQBhYyi5FHhgQ78b5kgFHGDrhZS313H+9IEGJgH6W/08 +9INcdz4MGlTgNoHAFRVmXOIz3hV7bgR6gQdQ7SSsL8fJ0suVG4Wh2tDhtRqzEMwK +JTUVD8fGOM/CG8t6LPMjLpnfHyTkbLjQfRhpQI1nCVMBP+KecOU/7kjUIBBKBcAA +T8MRH1zQvV0YeZitPPrmDQ/LK7pxEfQGcmrsN7ZbqEQ4lnlXPx4xP3RBdEy7ks7t +P23W8Ez2eO9E86lqXTbxnpk9W2eYxU7/SlaT07BdY3RbcABcCMLrQUvlAiLXS7NV +hFnvenEMXip5w/oNnaIdojARaUGdX8KjIt4u+dV7JzH4 +-----END CERTIFICATE----- +-----BEGIN CERTIFICATE----- +MIICwTCCAakCFHH+EHqV3xnb3Gh9bPdocEReVG0hMA0GCSqGSIb3DQEBCwUAMBwx +GjAYBgNVBAMMEXF3ZW4gdjEgdGVzdCByb290MCAXDTI2MDgxODIyMjkzOVoYDzIx +MjYwNzI1MjIyOTM5WjAcMRowGAYDVQQDDBFxd2VuIHYxIHRlc3Qgcm9vdDCCASIw +DQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBALfC+t0FBQURbzAdroq3P4BPP3Kl +ZB23rI9ZK2Qb7hHvC0hbeWLCjnt+CTtQALe8yFxVa9f0VI6+4mUinQ425C29tC7Z +5gPbzxukOurD+zFOGArW9uYg6DtQFqxaXmttuEv/cP+lOTsDbarEH42yrlVRg72v +OJmAGFoy/eXwXgVvsEM54SdU5uF0sPKuGJMZ4KPp3v8KKaTFfs/Ru8UPCOUlQ7nY +o7WemUlbMKIcpEfah/ZOo9f02br9K2P/+R8K1SHUpTq3kmW43vemh03sGsPZ5u1t +B7WT/0BRI6larvSHHQD9Hw331VbenxqRK5LWLiwpx0hu7QUmUn76L+2FPXcCAwEA +ATANBgkqhkiG9w0BAQsFAAOCAQEAEMnzsmDeOzmbSyKHuH3zzUTi8mWU1DgUtyLf +oFxDNDxGBef4o8ufTotKmkxhj6he6O2Mx4et5aYZgNu+KMyZpRIzgAh0+pC8ezEe +b29oLD570mOcEx8MpOOnjuJfYxnBzZigkhZq6VLh27A64hgxQhKAoySGxGDjN+C1 +Zw3xJnGWe7k7KvOPWMsbVYH1D0QCeUlzqWSmAl3L+e42OynIvOnKj6Vuh4/SxVdX +kBv3KlcMXmaGVC8AEo/M5Zzfkq/a+IC3aVYvhQPOkv1ByeObs5+Sjy2mKb2sNqU8 +sxyEYG5sNF7HPXac1j3PqROJ8O1X1lpXWyd2MChHhhCnFCteAQ== +-----END CERTIFICATE----- +`; + describe('describeWorkerTlsTrustGaps', () => { const daemonUrl = 'https://127.0.0.1:4170'; @@ -1627,6 +1675,85 @@ describe('describeWorkerTlsTrustGaps', () => { }), ).toEqual([]); }); + + it('keeps trusting a v1 root that carries no basicConstraints at all', () => { + // R3-8: `X509Certificate.ca` is false both for an explicit CA:FALSE and + // for a v1/no-extension root, but OpenSSL accepts the second as an issuer. + // Measured on Node 22 / OpenSSL 3 with this exact pair: serving the leaf + // with the v1 root as the trust store handshakes authorized=true, so the + // INVALID_PURPOSE warning here would send the operator to reissue a CA + // that already works. + expect( + describeWorkerTlsTrustGaps({ + cert: Buffer.from(TEST_TLS_CERT_FULLCHAIN_V1_ROOT), + certPath: '/certs/daemon.pem', + daemonUrl, + }), + ).toEqual([]); + }); + + it("names an operator CA file Node's loader cannot read", () => { + // R3-2: the boot diagnostic used a looser parser than the spawn-time + // merge. `cat a.pem b.pem` with no trailing newline in a.pem fuses the + // markers onto one line: the loose regex still sees a CA and judged the + // chain anchored, while the merge discarded the file and handed workers + // the daemon cert alone — daemon log clean, every worker handshake failing + // UNABLE_TO_VERIFY_LEAF_SIGNATURE. + const root = fullchainRootPem(); + const fused = `${root.trimEnd()}${root}`; + expect(fused).toContain( + '-----END CERTIFICATE----------BEGIN CERTIFICATE-----', + ); + const gaps = describeWorkerTlsTrustGaps({ + cert: Buffer.from(TEST_TLS_CERT_FULLCHAIN_LEAF_ONLY), + certPath: '/certs/daemon.pem', + daemonUrl, + operatorCaCertPath: '/certs/fused.pem', + operatorCaCert: Buffer.from(fused), + }); + expect(gaps.some((gap) => gap.includes('/certs/fused.pem'))).toBe(true); + expect( + gaps.some((gap) => + gap.includes("holds no PEM certificate block Node's loader can read"), + ), + ).toBe(true); + // And it no longer counts as an anchor. + expect( + gaps.some((gap) => gap.includes('UNABLE_TO_VERIFY_LEAF_SIGNATURE')), + ).toBe(true); + }); + + it('does not count a DER operator CA file as an anchor', () => { + // R3-2(b): NODE_EXTRA_CA_CERTS is PEM-only — Node never loads a DER file + // and says nothing about it — but the boot side used to fall back to a DER + // parse and count it as an anchoring CA. + const der = new X509Certificate(fullchainRootPem()).raw; + const gaps = describeWorkerTlsTrustGaps({ + cert: Buffer.from(TEST_TLS_CERT_FULLCHAIN_LEAF_ONLY), + certPath: '/certs/daemon.pem', + daemonUrl, + operatorCaCertPath: '/certs/root.der', + operatorCaCert: der, + }); + expect(gaps.some((gap) => gap.includes('/certs/root.der'))).toBe(true); + expect( + gaps.some((gap) => gap.includes('UNABLE_TO_VERIFY_LEAF_SIGNATURE')), + ).toBe(true); + }); + + it('still anchors on a CRLF-terminated operator CA file', () => { + // R3-1(strict arm): Node's loader accepts CRLF PEM (measured), so a + // vendor-exported CRLF bundle must not be reported as unreadable. + expect( + describeWorkerTlsTrustGaps({ + cert: Buffer.from(TEST_TLS_CERT_FULLCHAIN_LEAF_ONLY), + certPath: '/certs/daemon.pem', + daemonUrl, + operatorCaCertPath: '/certs/rootCA.pem', + operatorCaCert: Buffer.from(fullchainRootPem().replace(/\n/g, '\r\n')), + }), + ).toEqual([]); + }); }); /** @@ -3771,6 +3898,87 @@ ARaOwZHpfsTw4Aq74yAWUKXumVGFXQpZMRj/QWgQEItTYF7rJVARIssv5miDbHvW // A self-signed localhost cert/key whose validity window is entirely in the // past (notAfter = 2020-01-02). Not a real secret — and doubly worthless // since it's already expired. Used to exercise the boot-time expiry guard. +/** + * A CA-issued serving cert (leaf alone) with the key it pairs with, plus the + * root that signs it — the shape the documented `mkcert` flow produces. The + * leaf covers 127.0.0.1 and localhost and boots, but anchors nothing by + * itself: only an operator `NODE_EXTRA_CA_CERTS` pointing at the root closes + * the gap, which is what makes it able to tell the env wiring apart from a + * hard-coded `undefined`. + */ +const TEST_TLS_CERT_ISSUED_LEAF = `-----BEGIN CERTIFICATE----- +MIIDMDCCAhigAwIBAgIUSUiporSz6CoX5xzIrRzMJUWh23owDQYJKoZIhvcNAQEL +BQAwIzEhMB8GA1UEAwwYcXdlbiB3aXJpbmcgdGVzdCByb290IENBMCAXDTI2MDgx +ODIyMzI1OFoYDzIxMjYwNzI1MjIzMjU4WjAUMRIwEAYDVQQDDAlsb2NhbGhvc3Qw +ggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQCyATi7Mip/VT+YWLvMdqpa +IrgUpR5xLaZ9HGOI6kavvCvtveN/SEu9CT1XKQDRMT+NrJeTi97JV56GXZStGAES +68wVnJDwV7EE5/VpRrmEZamn0lxOHeBMdu34F22aQ1xhw9bdRw+00kA5kE2rHEN1 +ZVaE0orqBtj/tnfhWET11b/99W4V+91WJQn+P+HrJGbMUW58qGsoW9C6fa0Pj365 +UymzgAWCLzf5DBziIqsevbcRzLwFUVw5bogjxmFIrd8k/R/xrjqNQJrRJX5SbS9O +uI4tHmUeUmtjsAql53skJ2j2qpVvldZ0QFUS09O+vUkuSoZQ52s/b4/6SFwqdIXZ +AgMBAAGjaTBnMBoGA1UdEQQTMBGCCWxvY2FsaG9zdIcEfwAAATAJBgNVHRMEAjAA +MB0GA1UdDgQWBBQuqaRKepsHMIxA1OlXRfS9ZcyY+TAfBgNVHSMEGDAWgBSa5IYk +ecO4EMlhojegvuVPj9xRTDANBgkqhkiG9w0BAQsFAAOCAQEASjn63nPtLzzDVWvq +h7tITuKvE4CeWGATghhJGYYn9FOsyJxnbvVgQN0zmuzpPoTtxVdiGYob5qMEAjZB +UsCGfWDNsJ6znUc1De0/sjvkq/uHdMgzaOldIgjdT+FO5cbtnDJx+fUe3QANW9or +uX4mMvMI6ikw2LWPk8yavW1f0JyORa76gl0IoGyTmgBf9v9T4OIqskiR5xB1vE31 +nN5x4BuL5YnYN8x9GBDMOAGNn+HerAB8HSyygoB0A97eOGtxj5+DQh3EpirYx4bM +4uRnAzkc2poDlbMNG1lLO5MdXjvy4Hy5pu+tSOXkRokV7V3wJWm7TFNBRYjjogip +mt7FIw== +-----END CERTIFICATE----- +`; + +const TEST_TLS_KEY_ISSUED_LEAF = `-----BEGIN PRIVATE KEY----- +MIIEvAIBADANBgkqhkiG9w0BAQEFAASCBKYwggSiAgEAAoIBAQCyATi7Mip/VT+Y +WLvMdqpaIrgUpR5xLaZ9HGOI6kavvCvtveN/SEu9CT1XKQDRMT+NrJeTi97JV56G +XZStGAES68wVnJDwV7EE5/VpRrmEZamn0lxOHeBMdu34F22aQ1xhw9bdRw+00kA5 +kE2rHEN1ZVaE0orqBtj/tnfhWET11b/99W4V+91WJQn+P+HrJGbMUW58qGsoW9C6 +fa0Pj365UymzgAWCLzf5DBziIqsevbcRzLwFUVw5bogjxmFIrd8k/R/xrjqNQJrR +JX5SbS9OuI4tHmUeUmtjsAql53skJ2j2qpVvldZ0QFUS09O+vUkuSoZQ52s/b4/6 +SFwqdIXZAgMBAAECggEAP8YgRTEb+LLaLgLchcyeC90UhpEB7xqj438gShVlbeDE +/FBkCV4lhHyi9W9DU6+JTYDgbYRXNVum+AzfD4TiHZ1NaRDG/NTuHwvb6PPl04F4 +3x+G4pXhnoOdjp0WL4aiuoQnnu+uuOH7EKSarwtZP94muT+VdXMum68MFDhDvK9X +HPK8nS7LKYq4RMbkl6iY4HtudL7xFncrBM1rFW5tjJSemvoILmNzFNNndUO06qi5 +39RJWnWjOavlH3KGE0eBRxld+6OVJ1u14uI4ZIJ20nnaBlXPedbPMVoU0d3VGzBk +MQZXnEIzEn5gKHu0WOH5rFjxSsBJ/LCuoXfkWzR34QKBgQDeFltt8chUENX7fpjH +IAdSqhPU6IXeeK5bHlebmnEoHk9HmXVOvVRvoy3rfMaFuTUqr1xKrSff4BNrayRs +TZjRc6Uhp6OL1UqEV3Wj4oIRARnl+X/7THP4c9D37i73h7wIPASRsybw+kQjPHDp ++Cy7EsMR9MtuWCuLvuuhk6ExMwKBgQDNL6CzH82RjaPRXFYliaO+0VZ4WD2zF7tm +IM908rQRZ534yL4vTopYxiQDErmgIy6oudtdjvL/GqbsGfpA+eMzChuqrMLjoYkW +wJkjt+8B0IjNW06gxjMhpKvOSqmyhqQUZLS/lh9bLTaEn+rBFGLSgj/eu1b8gA00 +RLYRsPvEwwKBgCcB+EckW5JgbqVAxCbdekvLsbYIrVK5Ea7RcoPTKaLpR/WEf7U3 +zffZynv9K4VbVXpM2MIJDeLloaORaxFWw8uuK0fxAOnTqcX68p+5biz8a4cYPqFt ++USfWwnhHQC/J4iuugK5W9KhsowZ1p9RxtGI5xhlTcHw3J0sCIkVvA8/AoGAHONm +wbFplOOXO+O/MTvGtRfuD7WEwlFGDiPycWm2Vnj7Mcq5lBl/uu3ypggd4GDzscex +DeQRbD9JXxZtOHa2OTpkGMyIB9p3XZ+yL+g2m0/L4vXHBTXCfysbEUlLyRnRwhlH +pW2ybnjYIyYMvDBtlWvHKEnB/nzc3w4JgEYlvFcCgYBoYs7vAT44eu1LSanMeD7m +317U0Rp+8CzVLg7Jx6cdyoY/aw797tayqNePn28pQiv8sPcsi07tfuwyP7+fuDIP +HpK7Y2PYdLHDw2uvpM3U5uO/EeHdbcJsyPGsOH7hl2PrbRxWI3o1uH2a2TyVj4o3 +ZNK0I9xeA/IvxH54ZqElqA== +-----END PRIVATE KEY----- +`; + +const TEST_TLS_CERT_ISSUING_ROOT = `-----BEGIN CERTIFICATE----- +MIIDKTCCAhGgAwIBAgIUPcGOSM9P9TbZ6hP5evWMiwSygd0wDQYJKoZIhvcNAQEL +BQAwIzEhMB8GA1UEAwwYcXdlbiB3aXJpbmcgdGVzdCByb290IENBMCAXDTI2MDgx +ODIyMzI1OFoYDzIxMjYwNzI1MjIzMjU4WjAjMSEwHwYDVQQDDBhxd2VuIHdpcmlu +ZyB0ZXN0IHJvb3QgQ0EwggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQCf +LOHXauGBhN4HTDHi+R+91uInim+k8vumqw6dbdbj6FcKZWF8iIIs2NsKRuLRaaOo +/HLV3y4rTGWBAVYF3FcYSybqCeMoMa4JJX2FTqzgVdIAog+fNQeQdqOrsAtNXf+W +weB1c751qJwqunKCyk02i0zbBRqLqd/PJ6lnGDA/Fv6MNvtpjN5ScC018j+RBXwF +ZHQ0Dm8LkHShBteOr3r8ychU3Q5AMao7MnJK1fa2lixUKKimtaEeH0/SFyHMfHNt +io9/GxQluEFrunsLcC+6USpdp9sk/N6dxG1IqKPJlpjv5D5tM5qRbDlCZD+QWS9m +q0vXHbkd/yppytewSfYVAgMBAAGjUzBRMB0GA1UdDgQWBBSa5IYkecO4EMlhojeg +vuVPj9xRTDAfBgNVHSMEGDAWgBSa5IYkecO4EMlhojegvuVPj9xRTDAPBgNVHRMB +Af8EBTADAQH/MA0GCSqGSIb3DQEBCwUAA4IBAQAdypPZaHSwv1ptjv+l6hUIBM+O +5qNUV0Gk4jmt6UZD7lzIweurap8k2dQdYBF1BjHgo+2dCW7m4W4NHczgUzHwECCG +4jPNZhc3T0PyGQjqi/pQ1dwSfZn75H9qSq09GHbLq0Vqzp5zDav6Hv8OzSIy2Cm1 +1SDbgTndiZzHiShBJrUhJymLTcaVM2EDad3poKsVoZ7ArFPWViMlJi5fFd8sWjii +tsVzO5eP6Ln9kRVz9DhyN5a8ky1ceVOX/KsB0fS10e9Ortldm8lbFmcNDbpaVJy1 +35gz6UVTrBB7X/4E/XvBAo9rIiiL4PheAzLjHdDvhJuotdmHIzCfQ0LjQxIU +-----END CERTIFICATE----- +`; + const TEST_TLS_CERT_EXPIRED = `-----BEGIN CERTIFICATE----- MIIDCTCCAfGgAwIBAgIUW7rZvmhryKZI3pojRCfl3liQSEMwDQYJKoZIhvcNAQEL BQAwFDESMBAGA1UEAwwJbG9jYWxob3N0MB4XDTIwMDEwMTAwMDAwMFoXDTIwMDEw @@ -9443,14 +9651,18 @@ describe('runQwenServe channel worker supervisor', () => { async function bootTlsDaemonForTrustGapLog( hostname: string, + serving: { cert: string; key: string } = { + cert: TEST_TLS_CERT, + key: TEST_TLS_KEY, + }, ): Promise { tmpDir = fs.realpathSync( fs.mkdtempSync(path.join(os.tmpdir(), 'qws-channel-gap-')), ); const certPath = path.join(tmpDir, 'cert.pem'); const keyPath = path.join(tmpDir, 'key.pem'); - fs.writeFileSync(certPath, TEST_TLS_CERT); - fs.writeFileSync(keyPath, TEST_TLS_KEY); + fs.writeFileSync(certPath, serving.cert); + fs.writeFileSync(keyPath, serving.key); const logBaseDir = path.join(tmpDir, 'debug'); const worker = makeWorker({ enabled: true, @@ -9510,6 +9722,72 @@ describe('runQwenServe channel worker supervisor', () => { expect(log).not.toContain('UNABLE_TO_VERIFY_LEAF_SIGNATURE'); }); + const issuedLeafServing = { + cert: TEST_TLS_CERT_ISSUED_LEAF, + key: TEST_TLS_KEY_ISSUED_LEAF, + }; + + it('names the anchor gap for a CA-issued cert with no operator CA set', async () => { + // The control for the two wiring tests below: with NODE_EXTRA_CA_CERTS + // unset this serving cert really is unanchored, so a quiet log in the + // next test can only come from the operator CA being read. + vi.stubEnv('NODE_EXTRA_CA_CERTS', ''); + try { + const log = await bootTlsDaemonForTrustGapLog( + '127.0.0.1', + issuedLeafServing, + ); + expect(log).toContain('UNABLE_TO_VERIFY_LEAF_SIGNATURE'); + } finally { + vi.unstubAllEnvs(); + } + }); + + it('reads NODE_EXTRA_CA_CERTS from the environment when judging the gap', async () => { + // R3-7: nothing drove the wiring that reads the env, resolves it and + // hands the contents to describeWorkerTlsTrustGaps — replacing that read + // with `undefined` shipped green. Then an operator whose CA genuinely + // anchors the chain gets a false UNABLE_TO_VERIFY_LEAF_SIGNATURE warning + // on every startup, and the content-based fix becomes dead code. + const caDir = fs.realpathSync( + fs.mkdtempSync(path.join(os.tmpdir(), 'qws-operator-ca-')), + ); + const operatorCaPath = path.join(caDir, 'rootCA.pem'); + fs.writeFileSync(operatorCaPath, TEST_TLS_CERT_ISSUING_ROOT); + vi.stubEnv('NODE_EXTRA_CA_CERTS', operatorCaPath); + try { + const log = await bootTlsDaemonForTrustGapLog( + '127.0.0.1', + issuedLeafServing, + ); + expect(log).not.toContain('UNABLE_TO_VERIFY_LEAF_SIGNATURE'); + } finally { + vi.unstubAllEnvs(); + fs.rmSync(caDir, { recursive: true, force: true }); + } + }); + + it('still boots and still names the gap when NODE_EXTRA_CA_CERTS is unreadable', async () => { + // R3-7(b): the try/catch around the read is what lets a typo'd path + // degrade to "it anchors nothing". Without it the throw lands inside the + // channel-manager starting closure and channel startup fails on exactly + // the case the code says must degrade. + vi.stubEnv( + 'NODE_EXTRA_CA_CERTS', + path.join(os.tmpdir(), 'qws-no-such-ca-file.pem'), + ); + try { + const log = await bootTlsDaemonForTrustGapLog( + '127.0.0.1', + issuedLeafServing, + ); + expect(log).toContain('UNABLE_TO_VERIFY_LEAF_SIGNATURE'); + expect(log).toContain('qws-no-such-ca-file.pem'); + } finally { + vi.unstubAllEnvs(); + } + }); + it('forwards webhook tasks through the channel worker group', async () => { tmpDir = fs.realpathSync( fs.mkdtempSync(path.join(os.tmpdir(), 'qws-channel-webhook-')), diff --git a/packages/cli/src/serve/run-qwen-serve.ts b/packages/cli/src/serve/run-qwen-serve.ts index 319ef999ffb..812f7b71310 100644 --- a/packages/cli/src/serve/run-qwen-serve.ts +++ b/packages/cli/src/serve/run-qwen-serve.ts @@ -176,6 +176,7 @@ import type { ChannelWorkerSnapshot, CreateChannelWorkerSupervisorOptions, } from './channel-worker-supervisor.js'; +import { loadableCertificates } from './pem-certificate-blocks.js'; import { QWEN_SERVER_TOKEN_ENV } from './channel-worker-env.js'; import { ChannelWebhookEnqueueError } from './channel-webhook-ipc.js'; import { @@ -720,9 +721,37 @@ export function describeWorkerTlsTrustGaps(opts: { const gaps: string[] = []; // Exactly what a worker gets: the serving file merged with the operator's // CA file (see resolveWorkerCaCertPath in channel-worker-supervisor.ts). - const workerTrustStore = opts.operatorCaCert - ? [...chain, ...parseCertChain(opts.operatorCaCert)] - : chain; + // + // The merge is all-or-nothing, and judges both files with the loader's own + // rules — so an operator file Node cannot load contributes NOTHING to the + // workers' trust and makes the merge hand them the daemon cert alone. + // Judging it here with the looser `parseCertChain` (which also falls back to + // DER, a format NODE_EXTRA_CA_CERTS never reads) is how a fused or DER + // operator bundle got counted as an anchor at boot: the daemon log stayed + // clean while every worker handshake failed UNABLE_TO_VERIFY_LEAF_SIGNATURE. + const operatorChain = opts.operatorCaCert + ? loadableCertificates(opts.operatorCaCert.toString('utf8')) + : undefined; + if (opts.operatorCaCert && !operatorChain) { + gaps.push( + `NODE_EXTRA_CA_CERTS "${opts.operatorCaCertPath}" holds no PEM ` + + `certificate block Node's loader can read — every ` + + `-----BEGIN/END CERTIFICATE----- marker must sit alone on its own ` + + `line and every block must decode, and a DER file is never read at ` + + `all. Channel workers therefore receive the daemon cert alone and ` + + `anchor nothing through this file. Re-export it as PEM and restart.`, + ); + } + // Same rule for the serving file. In practice it always extracts — a fused + // or DER serving file cannot serve at all, `createSecureContext` throws at + // boot long before this runs — so the fallback below only keeps a leaf to + // reason about instead of reporting phantom gaps for a daemon that never + // started. + const servingChain = + loadableCertificates(opts.cert.toString('utf8')) ?? chain; + const workerTrustStore = operatorChain + ? [...servingChain, ...operatorChain] + : servingChain; // A leaf in NODE_EXTRA_CA_CERTS is a usable trust anchor only when it signed // itself: chain verification has no PARTIAL_CHAIN flag here, so a CA-issued // leaf (what the `mkcert` flow this project documents produces) never @@ -791,6 +820,28 @@ export function describeWorkerTlsTrustGaps(opts: { return gaps; } +/** DER for the basicConstraints OBJECT IDENTIFIER, 2.5.29.19. */ +const BASIC_CONSTRAINTS_OID_DER = Buffer.from([0x06, 0x03, 0x55, 0x1d, 0x13]); + +/** + * Whether `cert` says, in the extension itself, that it is not a CA. + * + * `X509Certificate.ca` is `false` in two very different cases: an explicit + * `basicConstraints CA:FALSE`, and an X.509 v1 / no-extension root (old + * internal PKIs, `openssl x509 -req -signkey`). OpenSSL accepts the second as + * an issuer — `X509_check_ca` returns 3 for a v1 cert and 2 only for an + * explicit CA:FALSE. Measured on Node 22 / OpenSSL 3: a leaf anchored by a v1 + * root completes a real handshake `authorized: true`, while the explicit + * CA:FALSE twin really does fail INVALID_PURPOSE. Warning on `.ca` alone + * therefore sends operators to reissue a CA that already works. + * + * `toLegacyObject()` exposes no more than `.ca` does, so read the DER: the + * extension's presence is what separates the two shapes. + */ +function declaresNotACa(cert: X509Certificate): boolean { + return !cert.ca && cert.raw.includes(BASIC_CONSTRAINTS_OID_DER); +} + function isSelfSignedCert(x509: X509Certificate): boolean { try { return x509.verify(x509.publicKey); @@ -865,7 +916,7 @@ function walkWorkerAnchorPath(chain: readonly X509Certificate[]): { // Measured on Node 22: a CA:FALSE self-signed leaf in its own trust // store handshakes fine, while the same shape used as an issuer fails // INVALID_PURPOSE — so the constraint binds only past the leaf. - if (path.length > 1 && !current.ca) { + if (path.length > 1 && declaresNotACa(current)) { return { anchored: false, path, nonCaTerminator: current }; } return { anchored: true, path }; From fb9239e11407e975057c9e0f2706885ecde93ba9 Mon Sep 17 00:00:00 2001 From: qqqys Date: Wed, 19 Aug 2026 10:39:49 +0800 Subject: [PATCH 10/26] fix(serve): judge worker CA framing the way Node's loader does MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 4 review of #9392: four Critical findings, three of them rooted in the same place — this code re-implemented Node's `NODE_EXTRA_CA_CERTS` loader instead of following it. R4-2 (Critical): `extractCertificateBlocks` pattern-matched what a well-formed PEM file looks like, and a new divergent shape surfaced in each of the last three rounds. Replaced with a line scanner that walks the file the way OpenSSL's `PEM_read_bio_X509` loop does. Three shapes Node loads and this rejected now extract: a `-----BEGIN CERTIFICATE-----` substring embedded in a line of prose (markers are matched at line start, not as unanchored substrings), whitespace inside a base64 body line, and a UTF-8 BOM in front of a block that is not the first in the file (what concatenating operator files produces). Every one of them silently fell back to daemon-cert-only while telling the operator the file "holds no PEM certificate block Node can load". BEHAVIOUR FLIP — the loader is prefix-loading, not all-or-nothing. The doc comment this module carried claimed a malformed block discards the whole bundle. Measured on Node 22 / OpenSSL 3 through real `NODE_EXTRA_CA_CERTS` handshakes: a good root followed by a fused block still handshakes `authorized=true` while Node prints `Ignoring extra certs … bad end line`. The loader keeps every certificate up to the first malformed block and loses that block and everything after it. So does this now; returning `undefined` for the whole file threw away anchors the workers do in fact receive. The fused-file and bad-decode cases still return `undefined`, because there the bad block IS the first one. Both behaviours were taken from the loader, not inferred: 15 shapes were written to disk, pointed at through `NODE_EXTRA_CA_CERTS` in a child process, and checked against a real `tls.connect` to a server holding the leaf they anchor. The parser agrees with the oracle on all 15, and `pem-certificate-blocks.test.ts` (new — this module had no direct coverage, which is how three rounds of shapes got through) pins each one with the measured verdict in the comment. R4-4 (Critical): `walkWorkerAnchorPath` applied the CA-suitability check only to the self-signed terminator, so a chain passing THROUGH an incapable issuer was reported anchored while every worker handshake failed. Issuer capability is now required of every non-self-signed chain member the walk leans on. Measured with real handshakes: a CA:FALSE intermediate and a v3 intermediate with no basicConstraints both fail INVALID_PURPOSE, and a keyCertSign-only intermediate fails INVALID_CA — all three reported gaps=NONE before. The self-signed terminator keeps its existing, looser rule, so the v1 root and CA:FALSE self-signed leaf cases stay unflagged as measured in earlier rounds. R4-3 (Critical): the boot diagnostic modelled a merged serving+operator trust store that the workers never receive when the serving file fails extraction — `resolveWorkerCaCertPath` finds `daemonBlocks === undefined`, discards the operator CA and hands them the serving file alone. Boot reported no gap while every worker handshake failed. The model now mirrors the fallback and names the discarded operator CA. The comment's premise (that such a file "cannot serve at all") was false and is gone. R4-1 (Critical): every `writeMergedWorkerCaBundle` call registered its own `process.once('exit')` listener. The merge cache is invalidated on purpose by in-place operator CA rotation and by tmp-cleaner aging, so a long-lived daemon accumulated a listener, a closure and an orphaned bundle directory per rebuild, and past the tenth printed `MaxListenersExceededWarning` into the log stream the fallback dedup exists to keep readable. One module-level hook now cleans up every minted directory, and a rebuild removes the directory it supersedes. R4-5 (Suggestion): the fallback-warning dedup was keyed on the path pair and add-only, so the first failure silenced every later one. Keyed on a coarse failure family now, and the keys are lifted when the pair merges successfully — a changed failure mode and a relapse after a fix are both new information. R4-6 (Suggestion): the fallback message blamed markers alone, but this PR's own X509 decode gate added a third rejection cause. Aligned with the boot-side wording, which already enumerates all three. R4-7 (Suggestion): the DER and fused operator-CA tests asserted gap presence via `.some()` without pinning the count, and never asserted the DER-specific text. Both now pin `toHaveLength(2)`, and the DER test asserts its own message. Every fix is mutation-verified: reverting it turns at least one test red (9 mutants run, 9 killed). Verification: `npx vitest run src/serve/pem-certificate-blocks.test.ts src/serve/channel-worker-supervisor.test.ts src/serve/run-qwen-serve.test.ts` — 411 passed; channel-worker-group / -manager / -diagnostics — 84 passed; eslint and prettier clean on the six touched files. `npm run build` and `npm run typecheck` do not complete in this worktree for reasons that predate this change and reproduce with it stashed (a `sharp` typing skew in packages/core and `@qwen-code/*` resolving to the sibling checkout's dist): 105 typecheck errors with and without the change, none in the touched files. --- .../serve/channel-worker-supervisor.test.ts | 147 ++++++++++++++ .../src/serve/channel-worker-supervisor.ts | 94 +++++++-- .../src/serve/pem-certificate-blocks.test.ts | 179 ++++++++++++++++++ .../cli/src/serve/pem-certificate-blocks.ts | 154 ++++++++++----- packages/cli/src/serve/run-qwen-serve.test.ts | 129 +++++++++++++ packages/cli/src/serve/run-qwen-serve.ts | 65 +++++-- 6 files changed, 702 insertions(+), 66 deletions(-) create mode 100644 packages/cli/src/serve/pem-certificate-blocks.test.ts diff --git a/packages/cli/src/serve/channel-worker-supervisor.test.ts b/packages/cli/src/serve/channel-worker-supervisor.test.ts index 1f0c08c45e2..f81aad77ee4 100644 --- a/packages/cli/src/serve/channel-worker-supervisor.test.ts +++ b/packages/cli/src/serve/channel-worker-supervisor.test.ts @@ -718,6 +718,153 @@ describe('createChannelWorkerSupervisor', () => { fs.rmSync(dir, { recursive: true, force: true }); }); + it('registers one exit hook however many times the bundle is rebuilt', async () => { + // R4-1: a `process.once('exit')` per mint accumulated a listener, a + // closure and an orphaned directory per rebuild — and the cache is + // rebuilt on purpose (in-place rotation, tmp-cleaner aging), so a + // long-lived daemon crossed Node's threshold and printed + // `MaxListenersExceededWarning: Possible EventEmitter memory leak + // detected` into the very log stream the fallback dedup keeps readable. + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'qwen-ca-exit-')); + const operatorCa = path.join(dir, 'operator.pem'); + const daemonCa = path.join(dir, 'daemon.pem'); + fs.writeFileSync(operatorCa, OPERATOR_CA_PEM); + fs.writeFileSync(daemonCa, DAEMON_CERT_PEM); + + const before = process.listenerCount('exit'); + const bundlePaths: string[] = []; + for (let round = 0; round < 5; round += 1) { + // Rotate in place so every spawn misses the cache and rebuilds. + fs.writeFileSync( + operatorCa, + round % 2 === 0 ? DAEMON_CERT_PEM : OPERATOR_CA_PEM, + ); + const { env } = await startWorkerWithCaPaths(daemonCa, operatorCa); + bundlePaths.push(env['NODE_EXTRA_CA_CERTS']!); + } + + expect(new Set(bundlePaths).size).toBe(5); + expect(process.listenerCount('exit')).toBeLessThanOrEqual(before + 1); + // Each rebuild also supersedes the previous bundle; holding those until + // process exit leaks a directory per rotation. + for (const superseded of bundlePaths.slice(0, -1)) { + expect(fs.existsSync(path.dirname(superseded))).toBe(false); + } + + fs.rmSync(path.dirname(bundlePaths.at(-1)!), { + recursive: true, + force: true, + }); + fs.rmSync(dir, { recursive: true, force: true }); + }); + + it('warns again when the SAME path pair fails a different way', async () => { + // R4-5(a): keying the dedup on the paths alone meant the first reason was + // the only one ever printed. An operator CA that is missing before a mount + // appears and a DER export afterwards are different fixes, and the second + // diagnosis was swallowed — sending the operator to fix mounts while the + // real problem was the format. + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'qwen-ca-family-')); + const operatorCa = path.join(dir, 'operator.pem'); + const daemonCa = path.join(dir, 'daemon.pem'); + fs.writeFileSync(daemonCa, DAEMON_CERT_PEM); + const warnings: string[] = []; + const onWarning = (warning: Error) => warnings.push(warning.message); + process.on('warning', onWarning); + + // (1) not there yet — read error. + await startWorkerWithCaPaths(daemonCa, operatorCa); + // (2) there, but nothing Node's loader can take from it. + fs.writeFileSync( + operatorCa, + `${OPERATOR_CA_PEM.trimEnd()}${OPERATOR_CA_PEM}`, + ); + await startWorkerWithCaPaths(daemonCa, operatorCa); + // (3) same failure as (2): still deduped. + await startWorkerWithCaPaths(daemonCa, operatorCa); + + await new Promise((resolve) => setImmediate(resolve)); + process.off('warning', onWarning); + const mine = warnings.filter((message) => message.includes(operatorCa)); + expect(mine).toHaveLength(2); + expect(mine[0]).toContain('ENOENT'); + expect(mine[1]).toContain('no PEM certificate block Node can load'); + fs.rmSync(dir, { recursive: true, force: true }); + }); + + it('warns again when a pair that merged successfully fails later', async () => { + // R4-5(b): the dedup Set was add-only, so a pair that once failed kept its + // key forever. A genuinely NEW later failure of the same pair was then + // swallowed and the workers restart-looped with no diagnostic at all. + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'qwen-ca-relapse-')); + const operatorCa = path.join(dir, 'operator.pem'); + const daemonCa = path.join(dir, 'daemon.pem'); + fs.writeFileSync(daemonCa, DAEMON_CERT_PEM); + const warnings: string[] = []; + const onWarning = (warning: Error) => warnings.push(warning.message); + process.on('warning', onWarning); + + fs.writeFileSync( + operatorCa, + `${OPERATOR_CA_PEM.trimEnd()}${OPERATOR_CA_PEM}`, + ); + await startWorkerWithCaPaths(daemonCa, operatorCa); + // Operator fixes the file; the merge works again. + fs.writeFileSync(operatorCa, OPERATOR_CA_PEM); + const merged = await startWorkerWithCaPaths(daemonCa, operatorCa); + expect(merged.env['NODE_EXTRA_CA_CERTS']).not.toBe(daemonCa); + // And breaks it again — new information, not a repeat. + fs.writeFileSync( + operatorCa, + `${OPERATOR_CA_PEM.trimEnd()}${OPERATOR_CA_PEM}`, + ); + const relapsed = await startWorkerWithCaPaths(daemonCa, operatorCa); + expect(relapsed.env['NODE_EXTRA_CA_CERTS']).toBe(daemonCa); + + await new Promise((resolve) => setImmediate(resolve)); + process.off('warning', onWarning); + expect( + warnings.filter((message) => message.includes(operatorCa)), + ).toHaveLength(2); + fs.rmSync(path.dirname(merged.env['NODE_EXTRA_CA_CERTS']!), { + recursive: true, + force: true, + }); + fs.rmSync(dir, { recursive: true, force: true }); + }); + + it('names decoding and DER, not just markers, in the fallback warning', async () => { + // R4-6: `extractCertificateBlocks` rejects for three reasons, and this + // commit's X509 decode gate added the third without updating the message. + // A CA corrupted by a misplaced `=` was told its markers must sit alone on + // their lines — while they already did. After boot this is the ONLY + // diagnostic the operator gets, so it has to name the same three the + // boot-time check does. + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'qwen-ca-cause-')); + const operatorCa = path.join(dir, 'operator.pem'); + const daemonCa = path.join(dir, 'daemon.pem'); + const lines = OPERATOR_CA_PEM.trimEnd().split('\n'); + const body = Math.floor(lines.length / 2); + lines[body] = `${lines[body]!.slice(0, 10)}=${lines[body]!.slice(11)}`; + fs.writeFileSync(operatorCa, `${lines.join('\n')}\n`); + fs.writeFileSync(daemonCa, DAEMON_CERT_PEM); + const warnings: string[] = []; + const onWarning = (warning: Error) => warnings.push(warning.message); + process.on('warning', onWarning); + + await startWorkerWithCaPaths(daemonCa, operatorCa); + + await new Promise((resolve) => setImmediate(resolve)); + process.off('warning', onWarning); + const warning = warnings.find((message) => message.includes(operatorCa)); + expect(warning).toBeDefined(); + // Markers ARE already alone on their lines here, so blaming them alone + // sends the operator to fix nothing. + expect(warning).toContain('every block must decode'); + expect(warning).toContain('a DER file is never read at all'); + fs.rmSync(dir, { recursive: true, force: true }); + }); + it('leaves the private key of a combined operator PEM out of the bundle', async () => { // R2-13: boot validation parses the first block only, so a combined // cert+key PEM serves fine — and copying its key into a tmpdir bundle diff --git a/packages/cli/src/serve/channel-worker-supervisor.ts b/packages/cli/src/serve/channel-worker-supervisor.ts index a6d3b44aa82..99b92061836 100644 --- a/packages/cli/src/serve/channel-worker-supervisor.ts +++ b/packages/cli/src/serve/channel-worker-supervisor.ts @@ -422,14 +422,37 @@ function sourceStamp(filePath: string): string { */ const warnedWorkerCaMergeFallbacks = new Set(); +/** + * The coarse reason a merge fell back, so a CHANGED failure mode re-warns once + * while a crash loop stays deduped. `reason` itself carries errno text and + * would let a flapping message defeat the dedup entirely; the paths alone + * swallowed the second, now-accurate diagnosis (`ENOENT` before a mount + * appears, then a DER export afterwards sends the operator to fix mounts). + */ +const WORKER_CA_MERGE_FALLBACK_FAMILIES = [ + 'read-error', + 'no-operator-blocks', + 'no-daemon-blocks', +] as const; + +type WorkerCaMergeFallbackFamily = + (typeof WORKER_CA_MERGE_FALLBACK_FAMILIES)[number]; + +function workerCaMergeFallbackKey( + operatorCaPath: string, + daemonCertPath: string, + family: WorkerCaMergeFallbackFamily, +): string { + return `${operatorCaPath}\0${daemonCertPath}\0${family}`; +} + function warnWorkerCaMergeFallback( operatorCaPath: string, daemonCertPath: string, + family: WorkerCaMergeFallbackFamily, reason: string, ): void { - // Keyed on the paths alone: `reason` varies with errno text, and keying on - // it would let a flapping error message defeat the dedup. - const key = `${operatorCaPath}\0${daemonCertPath}`; + const key = workerCaMergeFallbackKey(operatorCaPath, daemonCertPath, family); if (warnedWorkerCaMergeFallbacks.has(key)) return; warnedWorkerCaMergeFallbacks.add(key); // Falling back to the daemon cert alone silently drops the operator CA @@ -442,6 +465,26 @@ function warnWorkerCaMergeFallback( ); } +/** + * Bundle directories minted this process, cleaned up together. One + * `process.once('exit')` per mint accumulated a listener, a closure and an + * orphaned directory per rebuild — and the merge cache is rebuilt on purpose + * (in-place operator CA rotation, tmp-cleaner aging), so a long-lived daemon + * crossed Node's `MaxListenersExceededWarning` threshold and printed that + * leak warning into the very log stream `warnWorkerCaMergeFallback` dedups to + * keep readable. + */ +const mintedWorkerCaBundleDirs = new Set(); +let workerCaBundleExitHookRegistered = false; + +function cleanupWorkerCaBundleDir(dir: string): void { + try { + fs.rmSync(dir, { recursive: true, force: true }); + } catch { + // Best effort: a tmp cleaner may already have taken it. + } +} + function writeMergedWorkerCaBundle(contents: string): string { // mkdtempSync gives a 0700 directory with a random suffix, so the bundle // path cannot be pre-planted the way a fixed `qwen-worker-ca-.pem` in @@ -453,13 +496,16 @@ function writeMergedWorkerCaBundle(contents: string): string { const bundlePath = path.join(dir, 'ca-bundle.pem'); fs.writeFileSync(bundlePath, contents, { mode: 0o600 }); // Nothing else references this directory, so the daemon owns its lifetime. - process.once('exit', () => { - try { - fs.rmSync(dir, { recursive: true, force: true }); - } catch { - // Best effort: the daemon is already exiting. - } - }); + mintedWorkerCaBundleDirs.add(dir); + if (!workerCaBundleExitHookRegistered) { + workerCaBundleExitHookRegistered = true; + process.once('exit', () => { + for (const minted of mintedWorkerCaBundleDirs) { + cleanupWorkerCaBundleDir(minted); + } + mintedWorkerCaBundleDirs.clear(); + }); + } return bundlePath; } @@ -508,8 +554,16 @@ function resolveWorkerCaCertPath( warnWorkerCaMergeFallback( existing, daemonCertPath, + 'no-operator-blocks', + // Three causes reject a file, not one: markers, decoding, and DER. + // Blaming markers alone tells an operator whose CA is truncated or + // hand-edited to fix lines that are already correct — and after boot + // this warning is the only diagnostic they get, so it has to name the + // same three the boot-time check does. 'it holds no PEM certificate block Node can load (every ' + - '-----BEGIN/END CERTIFICATE----- marker must sit alone on its line)', + '-----BEGIN/END CERTIFICATE----- marker must sit alone on its own ' + + 'line and every block must decode, and a DER file is never read ' + + 'at all)', ); return daemonCertPath; } @@ -520,6 +574,7 @@ function resolveWorkerCaCertPath( warnWorkerCaMergeFallback( existing, daemonCertPath, + 'no-daemon-blocks', 'the daemon cert holds no PEM certificate block to merge into', ); return daemonCertPath; @@ -527,12 +582,29 @@ function resolveWorkerCaCertPath( const bundlePath = writeMergedWorkerCaBundle( `${[...operatorBlocks, ...daemonBlocks].join('\n')}\n`, ); + const superseded = mergedWorkerCaBundles.get(cacheKey); mergedWorkerCaBundles.set(cacheKey, { bundlePath, sourceStamps }); + if (superseded) { + // This rebuild orphaned the previous bundle; the exit hook would hold + // one per rotation until the daemon stops. + const dir = path.dirname(superseded.bundlePath); + mintedWorkerCaBundleDirs.delete(dir); + cleanupWorkerCaBundleDir(dir); + } + // The merge works again, so a LATER failure of the same pair is new + // information — without this, the first failure's key silenced it forever + // and the workers restart-looped with no diagnostic at all. + for (const family of WORKER_CA_MERGE_FALLBACK_FAMILIES) { + warnedWorkerCaMergeFallbacks.delete( + workerCaMergeFallbackKey(existing, daemonCertPath, family), + ); + } return bundlePath; } catch (err) { warnWorkerCaMergeFallback( existing, daemonCertPath, + 'read-error', err instanceof Error ? err.message : String(err), ); return daemonCertPath; diff --git a/packages/cli/src/serve/pem-certificate-blocks.test.ts b/packages/cli/src/serve/pem-certificate-blocks.test.ts new file mode 100644 index 00000000000..734c7bb86cb --- /dev/null +++ b/packages/cli/src/serve/pem-certificate-blocks.test.ts @@ -0,0 +1,179 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, expect, it } from 'vitest'; +import { + extractCertificateBlocks, + loadableCertificates, +} from './pem-certificate-blocks.js'; + +/** + * Every expectation in this file was taken from Node 22 / OpenSSL 3 itself: + * each shape was written to a file, pointed at through `NODE_EXTRA_CA_CERTS` + * in a child process, and a real `tls.connect` to a server holding the leaf + * these certificates anchor recorded whether the loader had taken them. The + * three rounds of divergence this module has been through all came from + * asserting what a well-formed PEM file looks like instead; the loader is the + * only oracle that settles it. + */ +const ROOT_PEM = `-----BEGIN CERTIFICATE----- +MIIDETCCAfmgAwIBAgIUcAV/pClZmXJcMTUQ7OBXsMJfVBkwDQYJKoZIhvcNAQEL +BQAwGDEWMBQGA1UEAwwNUHJvYmUgUm9vdCBDQTAeFw0yNjA4MTkwMjIxMzRaFw0z +NjA4MTYwMjIxMzRaMBgxFjAUBgNVBAMMDVByb2JlIFJvb3QgQ0EwggEiMA0GCSqG +SIb3DQEBAQUAA4IBDwAwggEKAoIBAQDICx6ZzCl3rP/Aa33Tqb8TbFOZ7ezouwe5 +7kDXA1MRFEc+gRvMP5doHiJRnhYuB9uRP+1VNGd8og8wGIY1FBtYdL1iy5rdQIF9 +i+I9URdt764y88h1W5p/iMlxNO/ZeMmmZwuG0cQtdTLfQpR8QoS9kfKWGW4qGEa6 ++B/ZOHRusgw5eMvG/vc8+roSttzHzbtEXrAg8GWWcCV8KQWqvN1YylJGsLWW4JGl +jDy9Q9xhPtLtYnT6zr87J/MJbdfBp0bKVveXoW2+7Nc3Ujr3gEdXhT3laQL7fFdQ +N5jW+NmVgHg5UGUjkDlusSUS44WyXdAk1NhYJimoganOafnbK9jfAgMBAAGjUzBR +MB0GA1UdDgQWBBTiORNFioXP/sD/HQpXuCMC2JN6EzAfBgNVHSMEGDAWgBTiORNF +ioXP/sD/HQpXuCMC2JN6EzAPBgNVHRMBAf8EBTADAQH/MA0GCSqGSIb3DQEBCwUA +A4IBAQB2Q7ivxcFlavCpt0hA6SX8Crtdg4HZYza+nLRrSDGxC0R0/c1Ax0W/nSrQ ++Cjr61zpAzFOJ516RmIKJ4La2Hv4Nwiub1rZpEMc9GXaMmgC+8Vy0rkma/RuX2ML +TjgJrasVxcR/DRB4PRDWykhdgcfp5gdubPhi/9xr1SNWyX+idR9+iJs/y/DNc9Jt +OI0+q5IPhHyu7dpgEDrOelnfSufFLl+SmS9No/EhRs1RC5zH78qsNLI8ggACOIJx +bh/8x87z7BA8oKl0WlFMNWZqOBQ1lR7cSmlhHUtC/QmJm6YNU71wpUP5+WgZHvPo +XKg878D/pA1BJ8fgC9Mrczhl6PIJ +-----END CERTIFICATE----- +`; + +const LEAF_PEM = `-----BEGIN CERTIFICATE----- +MIIDJjCCAg6gAwIBAgIUNWSrcDrHDnBRI6jTNPSQCS9RJcQwDQYJKoZIhvcNAQEL +BQAwGDEWMBQGA1UEAwwNUHJvYmUgUm9vdCBDQTAeFw0yNjA4MTkwMjIxMzRaFw0z +NjA4MTYwMjIxMzRaMBQxEjAQBgNVBAMMCWxvY2FsaG9zdDCCASIwDQYJKoZIhvcN +AQEBBQADggEPADCCAQoCggEBANJ7RJRMFrD94mIW4/OEJUSYs1feTLKMYWGfJzgb +GHlnEILhKcVnJA+DMHZ8gvfc13oOF0+OGN7RqLHx+SL7eG7AT03PS9z7Qj1tEIpS +BxHPLkIdha9fPwtL6Pvh2Pqfsm2aAOTXT4EaQjGkrVNXe5V1JxfOjoiYV+eWKFwt +d3ABd+piE9RoTZQf30guISSw2EEhQmcutaZx6w8m725tc6ZYl8jIz3yNmgtcgFfB +5CUPmowQYFfdIxOpK7MO2PHvuYx6tcnN0rwAErtuW8bVb0iWMFqJvJfdzO+1J5jW +gT/pFYjM3jTZZlibdYHwSYGTPFArUo/Z5At8MZKXqLNKsUMCAwEAAaNsMGowGgYD +VR0RBBMwEYIJbG9jYWxob3N0hwR/AAABMAwGA1UdEwEB/wQCMAAwHQYDVR0OBBYE +FLJwKoh8gKUwkfkbgIIQEBg1zWsHMB8GA1UdIwQYMBaAFOI5E0WKhc/+wP8dCle4 +IwLYk3oTMA0GCSqGSIb3DQEBCwUAA4IBAQAAcOqQ1QOnBm11zUjnamj2Co2IgWvL +HvyNQ2BTgwBVqxWtvBc0vf5VW5t9ikV+jq0uhYQJnRLZMKXlhJf73uHMsh1FRM8Z +t4HDNiD3EjHa316EnilTNVH8H+RVDstpQxo9ZXZ2ishFXBuTn1MiX74B72v3Gt5Y +u8NhUh5uAEveCCboMETItLNM6y+LwKfBazfwbnDY6MGcURjRE4/J7P2wEyIy1Ohu +ZcfT/LXbn1H8cBh1iqy9flUsQR3KRTHe84Btck0+O3KA3wGIpRGF1q/mitl7zCNJ +v6SP3sMzDfFDdKbveQk7uRIdMfMMkyDuApZHDuW1YRlQBISLn3G66YOo +-----END CERTIFICATE----- +`; + +/** The certificate body lines of `pem`, markers stripped. */ +const bodyLines = (pem: string): string[] => + pem.trim().split('\n').slice(1, -1); + +describe('extractCertificateBlocks', () => { + it('takes a canonical single-certificate file verbatim', () => { + expect(extractCertificateBlocks(ROOT_PEM)).toEqual([ROOT_PEM.trim()]); + }); + + it('walks past a marker embedded in a line of prose', () => { + // Oracle: authorized=true. OpenSSL matches `-----BEGIN ` at the START of a + // line, so this is a comment it never sees as a block — while counting + // markers as unanchored substrings saw markers=2 vs blocks=1 and rejected + // the whole file, dropping a CA the workers would have trusted. + // Ends WITH the marker on purpose: only line-start anchoring rules this + // out, so a mutant that merely requires the trailing `-----` still fails. + const withProse = `# exported by -----BEGIN CERTIFICATE-----\n${ROOT_PEM}`; + expect(extractCertificateBlocks(withProse)).toEqual([ROOT_PEM.trim()]); + }); + + it('takes a body line carrying interior whitespace', () => { + // Oracle: authorized=true. The base64 decoder skips whitespace anywhere in + // the body; a `[A-Za-z0-9+/=]+` line match rejected it. + const lines = bodyLines(ROOT_PEM); + const split = [...lines]; + split[1] = `${split[1]!.slice(0, 10)} ${split[1]!.slice(10)}`; + const spaced = `-----BEGIN CERTIFICATE-----\n${split.join('\n')}\n-----END CERTIFICATE-----\n`; + expect(extractCertificateBlocks(spaced)).toEqual([ROOT_PEM.trim()]); + }); + + it('takes a block behind a BOM that is not at the start of the file', () => { + // Oracle: authorized=true. Concatenating operator files puts a BOM in the + // MIDDLE of the result, and a file-start-anchored strip left + // a BOM-prefixed `-----BEGIN` line unmatched, so the second cert vanished. + const concatenated = `${LEAF_PEM}\uFEFF${ROOT_PEM}`; + expect(extractCertificateBlocks(concatenated)).toEqual([ + LEAF_PEM.trim(), + ROOT_PEM.trim(), + ]); + }); + + it('rejects a file whose only block has a fused end line', () => { + // Oracle: authorized=false, and Node prints `Ignoring extra certs … bad + // end line`. `cat a.pem b.pem` with no trailing newline in a.pem. + expect( + extractCertificateBlocks(`${LEAF_PEM.trimEnd()}${ROOT_PEM}`), + ).toBeUndefined(); + }); + + it('keeps the certificates loaded BEFORE a fused end line', () => { + // Oracle: authorized=true. The loader is prefix-loading, not + // all-or-nothing: it keeps everything up to the first malformed block and + // loses that block and everything after it. Returning `undefined` here + // would throw away an anchor the workers do receive. + expect( + extractCertificateBlocks(`${ROOT_PEM}${LEAF_PEM.trimEnd()}${LEAF_PEM}`), + ).toEqual([ROOT_PEM.trim()]); + }); + + it('stops at a block whose body does not decode, keeping the prefix', () => { + // Oracle: authorized=true — `bad base64 decode` ends the loop the same way + // a bad end line does. Shape is not loadability: this body is made only of + // base64 characters. + const lines = bodyLines(ROOT_PEM); + const corrupted = [...lines]; + corrupted[1] = `${corrupted[1]!.slice(0, 10)}=${corrupted[1]!.slice(11)}`; + const file = `${ROOT_PEM}-----BEGIN CERTIFICATE-----\n${corrupted.join('\n')}\n-----END CERTIFICATE-----\n`; + expect(extractCertificateBlocks(file)).toEqual([ROOT_PEM.trim()]); + }); + + it('rejects a block that never closes', () => { + expect( + extractCertificateBlocks('-----BEGIN CERTIFICATE-----\nAAAA\n'), + ).toBeUndefined(); + }); + + it('returns undefined for a file with no block at all', () => { + expect(extractCertificateBlocks('')).toBeUndefined(); + expect(extractCertificateBlocks('not a certificate\n')).toBeUndefined(); + }); + + it('leaves a private key out of a combined cert+key file', () => { + // The merged bundle is written to a tmpdir NODE_EXTRA_CA_CERTS never reads + // as a key, and a SIGKILLed daemon cannot run the `exit` cleanup — so key + // material must never reach it. + const combined = `${ROOT_PEM}-----BEGIN PRIVATE KEY-----\nQUJD\n-----END PRIVATE KEY-----\n`; + expect(extractCertificateBlocks(combined)).toEqual([ROOT_PEM.trim()]); + }); + + it('normalizes CRLF and marker/body padding to canonical PEM', () => { + // Oracle: authorized=true for both. The bundle this feeds is written to + // disk, so the output has to be canonical whatever the input looked like. + expect(extractCertificateBlocks(ROOT_PEM.replace(/\n/g, '\r\n'))).toEqual([ + ROOT_PEM.trim(), + ]); + const padded = ROOT_PEM.trim() + .split('\n') + .map((line) => (line.startsWith('-----') ? `${line} ` : ` ${line}`)) + .join('\n'); + expect(extractCertificateBlocks(padded)).toEqual([ROOT_PEM.trim()]); + }); +}); + +describe('loadableCertificates', () => { + it('parses every block it returns', () => { + const certs = loadableCertificates(`${LEAF_PEM}${ROOT_PEM}`); + expect(certs?.map((cert) => cert.subject)).toEqual([ + 'CN=localhost', + 'CN=Probe Root CA', + ]); + }); + + it('returns undefined when the loader would take nothing', () => { + expect(loadableCertificates('not a certificate\n')).toBeUndefined(); + }); +}); diff --git a/packages/cli/src/serve/pem-certificate-blocks.ts b/packages/cli/src/serve/pem-certificate-blocks.ts index 151121eca56..e38e7df7e1f 100644 --- a/packages/cli/src/serve/pem-certificate-blocks.ts +++ b/packages/cli/src/serve/pem-certificate-blocks.ts @@ -6,40 +6,69 @@ import { X509Certificate } from 'node:crypto'; +/** `-----BEGIN