Commit 526809d
fix(serve): let channel workers reach TLS-enabled daemons (#9392)
* 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)
* fix(serve): make the worker TLS trust injection actually establish trust
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-<pid>.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.
* test(serve): declare the worker TLS trust check's NODE_EXTRA_CA_CERTS reads
The `Test (ubuntu-latest, Node 22.x)` job failed on 04c954d with a single
red test: `serve process.env guard > allows only documented process-scoped
process.env expressions`. 04c954d 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.
* fix(serve): judge the worker TLS trust gaps on the whole serving file
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
* fix(serve): judge the worker CA bundle by what Node's loader accepts
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 <noreply@anthropic.com>
* fix(serve): refuse a non-CA chain terminator and document the worker TLS hop
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 <noreply@anthropic.com>
* fix(serve): judge worker CA files with the loader's own rules, on both sides
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.
* fix(serve): judge worker CA framing the way Node's loader does
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.
* fix(serve): judge a chain terminator and a marker line the way OpenSSL does
Round 5's three Critical findings, each measured against a real handshake on
Node v22.23.0 / OpenSSL 3.0.13 before and after.
R5-1 — the self-signed-terminator check read basicConstraints' PRESENCE, so a
v3 root carrying only a subjectKeyIdentifier (`.ca === false`, no
basicConstraints OID, no keyCertSign — a minimal `openssl req -x509` config)
was reported anchored while OpenSSL refuses it as an issuer: measured
`authorized=false code=INVALID_PURPOSE` with the boot log, /health and the
daemon all green and every worker restart-looping. Replaced with
`cannotIssueCertificates`, which mirrors `check_ca()` in `v3_purp.c` in the
same order — keyUsage first, then basicConstraints, then the v1-root and
keyCertSign exemptions — reading the extensions out of the DER through a real
element walk instead of scanning `cert.raw` for OID bytes that also occur
inside a signature. Six shapes measured, all six agree: v3/SKI-only refused,
keyCertSign-only accepted, CA:TRUE+keyCertSign accepted, v1 root accepted,
CA:TRUE with keyUsage lacking keyCertSign refused, CA:FALSE with keyCertSign
refused.
R4-2 — `normalizePemLine` stripped LEADING whitespace before the marker match,
so a CA file whose `-----BEGIN/END CERTIFICATE-----` markers are indented was
counted anchorable. Node's loader takes nothing from such a file (measured:
`UNABLE_TO_VERIFY_LEAF_SIGNATURE`, no `Ignoring extra certs` warning, and
`openssl storeutl -certs` reports 0), while the same file un-indented
handshakes `authorized=true`; trailing whitespace, CRLF and a BOM in front of
the marker all load and stay tolerated. The marker match is now anchored at
column 0, which is also what `pemMarkerLabel`'s own doc already claimed.
The same finding's fifth entrance is closed too: the loader decodes EVERY
block's body whatever its label and stops the file on a bad decode, so a
corrupt or empty leading PRIVATE KEY block now stops the scan instead of being
skipped unvalidated (measured on both shapes).
R5-17 — `hands workers an absolute --tls-cert path` asserted that
`path.relative(process.cwd(), certPath)` is relative, with the cert minted
under `os.tmpdir()`. On the required merge-queue job `Test (windows-latest,
Node 22.x)` the workspace is on D: and `os.tmpdir()` on C:, where cross-drive
`path.relative` returns the ABSOLUTE target and the precondition fails —
verified through `path.win32`. `TMPDIR` cannot move it (win32 `os.tmpdir()`
reads TMP/TEMP/USERPROFILE). The fixture now falls back to a directory under
the vitest cwd exactly when the relative path comes back absolute, so the Linux
path is unchanged.
Round 5's Suggestions, all pinned by tests whose mutants were measured green
beforehand:
- R5-2: the `no-daemon-blocks` fallback had no test. Added one built on a
serving PEM whose first block lacks its END line followed by a complete
block — accepted by `tls.createSecureContext`, so the daemon boots and
serves, while the loader takes nothing.
- R5-5 / R5-6 / R5-27: the exit hook's body is now
`cleanupMintedWorkerCaBundleDirs()`, exported and returning what it swept.
One test pins that exactly one such listener is registered, that a
superseded bundle has already left the registry, and that the sweep empties
it. Deleting the registration, the `delete` or the `clear` each turn it red;
before, deleting the whole `process.once('exit', …)` registration left all
103 tests green.
- R5-9: the minted directory is registered before the bundle write, not after,
so a write that throws (ENOSPC/EDQUOT on a size-capped tmpfs) leaves a
directory the exit hook can still see rather than an untracked 0700 orphan
per failing respawn. No test: forcing that write to fail needs `node:fs`
mocked file-wide, which this suite cannot do without changing how its other
104 tests resolve fs.
- R5-26: added a leaf ← v1 intermediate ← CA:TRUE root fixture. Narrowing the
intermediate check to the terminator's test shipped green before it.
- R5-28: added a key-BEFORE-cert file. A stop-at-first-non-certificate mutant
shipped green before it; the loader skips the key block and loads the cert.
Verification: `packages/cli` — pem-certificate-blocks (19), run-qwen-serve
(298), channel-worker-supervisor (105) and daemon-worker (85), 507 passing.
ESLint and Prettier clean on the six touched files. `tsc --noEmit` reports the
same 105 errors before and after the change; all of them are the worktree's
stale `@qwen-code/acp-bridge` dist, none in these files.
* test(serve): pin the failed-mint registry order R5-9 left untested
`4a935b38fa` fixed R5-9 — the merged-bundle directory is registered before the
write, not after — and stated it could not pin it: forcing `writeFileSync` to
throw looked like it needed `node:fs` mocked file-wide, which would change how
the other 105 tests in that suite resolve fs.
It does not need that. `vi.doMock` is not hoisted, so it binds only to the
dynamic `import()` beside it: that one supervisor instance sees a throwing
`writeFileSync` while every other test in the file keeps the real `node:fs` it
imported at load. (`vi.spyOn(fs, 'writeFileSync')` is the approach that cannot
work here — an ESM module namespace is not configurable.)
The test drives a spawn whose bundle write fails with ENOSPC, then asserts the
three things the fix is about: workers fall back to the daemon cert alone,
exactly one directory was minted and is still on disk, and
`cleanupMintedWorkerCaBundleDirs()` returns it and removes it.
Mutation-verified against the pre-fix order: moving
`mintedWorkerCaBundleDirs.add(dir)` back below the write turns this test red
and leaves the other 105 green — which is the finding's own claim about what
the suite could not see.
Verification: `channel-worker-supervisor.test.ts` 106 passed. ESLint, Prettier
and `tsc --noEmit -p packages/cli` clean on the file.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(serve): frame worker CA files and judge chains the way OpenSSL does
Round 6 reported seven Criticals against the worker TLS trust surface. Each
fix below is measured against the real oracle — a `tls.connect` from a child
process holding the file under test as `NODE_EXTRA_CA_CERTS`, against a server
serving the leaf, on Node v22.23.0 / OpenSSL 3.0.13 — and mutation-verified.
R4-2, the class finding, is closed structurally rather than entrance by
entrance. `extractCertificateBlocks` now models the loader's own framing
decisions instead of re-deriving what a well-formed file looks like:
- the certificate label alias set is {CERTIFICATE, X509 CERTIFICATE}
- a block is `header CRLF CRLF data`; the first blank line splits it, and a
header section without `Proc-Type` fails the load (`not proc type`) while one
with it is a key the loader consumes and reads past
- a BOM is tolerated only in front of a BEGIN marker, which is the only
position measured to load; in front of an END marker, or inside a base64
line, the loader takes nothing
- non-certificate bodies are judged by a strict base64 predicate, not by the
alphabet alone (`====` and `AAAAA` are alphabet-valid and both take the
whole file down)
Sixteen shapes were measured against real handshakes and the module now agrees
with the loader on all sixteen, including one divergence (a BOM inside a base64
line) that predates this round.
The remaining six are in the boot-time diagnostic:
- R6-1: an unreadable serving file is a gap whether or not an operator CA is
set. Gating it on `operatorChain` reported zero gaps on the no-operator path
while every worker restart-looped.
- R6-2: the leaf every downstream check judges is the one BOOT parsed, not the
first match of an unanchored regex. A block whose BEGIN line is indented is
prose to the column-0 readers but matched the regex, so the SAN, expiry and
issuer checks judged a certificate the daemon never serves.
- R6-3: in the discard scenario the operator CA anchored nothing only because
it was thrown away with the unloadable serving file. Saying its contents "do
not carry a certificate that anchors it" is false, and its remedy is a no-op
when the variable already points at the issuing CA.
- R6-4: an unreadable `NODE_EXTRA_CA_CERTS` file is now named as unreadable,
with its error code, instead of being downgraded to "no contents" — the old
message asserted an unknowable content fact and prescribed an action the
operator had already taken, when the real fix is permissions.
- R6-5: issuers are found by name match plus signature, and the capability
judgment runs separately. `checkIssued` enforces the issuer's keyUsage, so
using it as the SEARCH predicate meant a CA:TRUE intermediate without
`keyCertSign` was never found and the walk fell through to a generic gap
whose cause, error code and remedy are all wrong for that shape.
- R6-6: `pathLenConstraint` is modelled. It sits inside the same
basicConstraints value the capability checks already read, and went unread —
a `pathlen:0` root over one intermediate walked to `anchored: true` with zero
gaps while every worker handshake failed PATH_LENGTH_EXCEEDED.
Verification: 29/29 pem-certificate-blocks, 305/305 run-qwen-serve, 5246 passed
across `src/serve` with the same 3 pre-existing root-permission failures as the
unmodified branch; eslint clean; typecheck unchanged at 105 pre-existing
module-resolution errors. Seventeen mutation arms — one per fixed behaviour,
plus an off-by-one on the new constraint check — each turn a test red.
* fix(serve): name the cause a refused anchor actually has
Round 7 of the review found both remaining boot-gap messages asserting a
cause and an outcome the measured chain does not have.
R7-1: `cannotIssueCertificates` refuses a self-signed terminator for three
independent reasons — keyUsage without keyCertSign (whatever
basicConstraints says), basicConstraints present with `!ca`, and the
non-v1 no-basicConstraints shape — and the `nonCaTerminator` message
described only the second. A root minted as `basicConstraints critical
CA:TRUE` + `keyUsage critical digitalSignature` was told it "carries
basicConstraints CA:FALSE" (false), that handshakes fail INVALID_PURPOSE,
and to "Reissue that certificate with CA:TRUE" — which it already is. The
other offered remedy cannot work either: nothing but itself anchors a
self-signed certificate. Both remedies being no-ops, the operator loops
reissue/restart with no usable guidance. Split the branch on
`issuerRefusedForKeyUsage`, the same way the sibling `incapableIssuer`
branch already does, and widen the remaining arm to cover the
no-basicConstraints shape it also fires on.
Measured on Node v22.23.0 / OpenSSL 3.0.13 with the new fixture: `openssl
verify` reports `error 32 ... key usage does not include certificate
signing`, and a real worker-shape handshake (fullchain as the trust store)
fails with that same text — not INVALID_PURPOSE. The message now says so.
R7-2: the NODE_EXTRA_CA_CERTS read-error gap announced a certain
UNABLE_TO_VERIFY_LEAF_SIGNATURE outage that does not happen when the
serving file anchors itself. `resolveWorkerCaCertPath`'s catch hands each
worker the serving file as its extra-CA store, so a fullchain — certbot
and mkcert's normal shape — loads its own root and every handshake
succeeds, while the anchor walk in this very function returns
`anchored: true` for exactly that shape. The diagnostic knew the config
worked and announced an outage anyway; its only hedge covered the workers'
DEFAULT trust store, not the CA the serving file itself carries. The one
test setting `operatorCaCertReadError` used a leaf-only serving file,
where the claim happens to hold. The gap is now pushed after the anchor
walk and its failure sentence is conditional on the chain not anchoring;
the serving-file gap moved with it so the emitted order is unchanged.
Behaviour flip: both messages change text an operator reads at boot. The
keyUsage terminator now names keyUsage rather than basicConstraints and
predicts the measured error text rather than INVALID_PURPOSE; the
read-error gap stops predicting a handshake failure when the serving chain
anchors. No test pinned the old claims for these shapes — no fixture
exercised a keyUsage-refused terminator at all, and the CA:FALSE
terminator test still asserts INVALID_PURPOSE unchanged.
Verification: `npx vitest run src/serve/run-qwen-serve.test.ts` -> 307
passed. Mutants, each red on exactly one new test: force the R7-1 split to
the CA:FALSE arm; force the R7-2 claim unconditional; force it always
anchored (caught by the leaf-only arm, which pins that the outage sentence
still fires where it is true). `npx tsc -p tsconfig.json --noEmit` reports
5 errors with and without this change, all in unrelated files.
`npx eslint` clean on both. `npx vitest run src/serve/` -> 5248 passed,
3 failed; the same 3 fail on the stashed tree (chmod-based tests that
cannot constrain uid 0).
* fix(serve): read a headed PEM block the way the loader's own label rules do
Closes the two Criticals of review round 8.
R4-2 (pem-certificate-blocks.ts): `extractCertificateBlocks` enforced RFC
1421's "the first header must be `Proc-Type`" rule for blocks of EVERY label,
while the `NODE_EXTRA_CA_CERTS` loader inspects a header section only on a
block it tries to consume — and it consumes certificate labels alone. An
operator CA file holding, say, a `PRIVATE KEY` block whose header section
starts with `Comment:` therefore loaded fine for the workers themselves
(handshake `authorized: true`) while this scan returned `undefined`, so
`resolveWorkerCaCertPath` fired its no-operator-blocks fallback, discarded the
operator CA, handed workers the daemon cert alone and blamed marker/decode
defects the file does not have. Pre-PR the env value reached workers
untouched, so this was a regression against the PR's own "merged, not
replaced" contract.
Rather than close that entrance alone, the header branch now follows the two
rules the loader was measured to actually have, which closes the round's other
two reported divergences with it:
- A header section on a CERTIFICATE-family block stops the file whatever it
says. `Proc-Type` does not spare it — the loader goes on to decrypt and
aborts `bad decrypt` (with `DEK-Info`) or `not dek info` (without). Such a
block used to be SKIPPED, so the scan read straight past a stop.
- The body BELOW a header section is still decoded for every label, so an
encrypted key with an undecodable body is `bad base64 decode` and stops the
file. The old branch `continue`d before the base64 judgment and reported
certificates behind that stop as anchors the workers never got.
R8-1 (run-qwen-serve.ts): `describeWorkerTlsTrustGaps` assumed
`servingBlocks[0]` is the served leaf. A serving file whose leaf carries the
`TRUSTED CERTIFICATE` label (what `openssl x509 -trustout` writes) followed by
its root yields `servingBlocks = [root]`, so the anchor walk started at the
root, at depth 0, where the leaf-depth exemption waives the CA-capability
check — the walk returned anchored and the diagnostic reported zero gaps while
every worker handshake failed. Boot stays green throughout: `X509Certificate`
reads the trusted label and `createSecureContext` serves the file. The walk now
anchors at the certificate boot parsed whenever `servingBlocks` does not
contain it, mirroring the `servingBlocks === undefined` fallback beside it.
Every rule above was measured on Node v22.23.0 / OpenSSL 3.0.13 through real
`NODE_EXTRA_CA_CERTS` handshakes in the worker shape before it was written
down, including the quiet controls: a capable root over a label-hidden leaf
authorizes and the diagnostic stays silent, and a well-formed legacy encrypted
key is still read past to the certificates behind it.
Verification: `vitest run src/serve/` — 5252 passed, 3 failed; the same 3 fail
on the unmodified branch (5248 passed) and are the known root-uid failures
where `chmod` cannot block a read or unlink. Each of the five fixes was
mutation-verified by reverting it alone, and each turned at least one test red.
* fix(serve): judge a PEM block the way the loader's own parser does
R4-2 and R8-1 of round 9, both measured against Node v22.23.0 with real
`NODE_EXTRA_CA_CERTS` handshakes rather than reasoned about.
R4-2, two divergences from the loader in `extractCertificateBlocks`:
- The X509 gate parsed the re-rendered PEM, which is stricter than the
loader by exactly one shape: a body carrying a complete DER certificate
followed by extra bytes. `new X509Certificate(<that PEM>)` throws
`wrong tag`; the loader TAKES the block (`authorized: true`, no
`Ignoring extra certs` warning, 3 trailing bytes appended to a root).
The gate now parses the decoded bytes, which accept what the loader
accepts and still throw on truncated or invalid DER. Judging the PEM
dropped that block and every block behind it, so the merge discarded a
CA the workers' own loader reads and the operator was told the file
holds no loadable certificate block.
- A BEGIN marker inside a body was folded into the body, where the
base64 judgment failed on its `-` characters and dropped the WHOLE
file. The loader ends the block there, takes what it collected, and
reads nothing further. Four measured shapes pin both halves:
`[root without its END line][full root]` authorizes with no warning
(the truncated body is taken); `[leaf without its END line][full
root]` fails UNABLE_TO_VERIFY_LEAF_SIGNATURE with no warning (the root
BEHIND it is not taken, so the loader stops rather than resuming at
the marker); `[full leaf][leaf without its END line][full root]`
likewise; and an unclosed block at EOF is still `bad end line`.
R8-1, the trust-gap diagnostic was blind to a self-signed served leaf
the workers never receive. The fingerprint check only decides whether to
prepend the boot-parsed leaf to the modeled worker store; once prepended,
a self-signed leaf self-anchored the walk at path length 1 and boot
reported zero gaps. A self-signed certificate verifies only when it is
itself in the trust store. Measured for a `TRUSTED CERTIFICATE`-labelled
self-signed loopback leaf plus an unrelated plain root:
`createSecureContext` serves the file while every worker handshake fails
DEPTH_ZERO_SELF_SIGNED_CERT with an EMPTY stderr. The walk now refuses
to anchor on a leaf the workers do not hold, and the new gap names the
label and the remedy instead of the generic "issued by another CA"
message, which would have sent the operator after a CA that does not
exist.
BEHAVIOUR FLIP: the `no-daemon-blocks` test arm in
channel-worker-supervisor.test.ts pinned `[block without its END
line][complete block]` as a file the loader takes nothing from. That is
the divergence above recorded as truth — re-measured, the loader takes
the truncated block. The fixture is re-pointed at a `TRUSTED
CERTIFICATE` block, which the same probe shows IS the shape the arm
describes: `createSecureContext` accepts it (the daemon boots and
serves) and the loader takes nothing from it, silently.
Verification: 451 tests across pem-certificate-blocks,
channel-worker-supervisor and run-qwen-serve pass. Each of the three
fixes was mutation-verified — reverting the DER gate fails 2 tests,
dropping the BEGIN-marker termination fails 2, and dropping the
unheld-leaf check fails 1. `tsc --noEmit` on packages/cli reports the
same 6 pre-existing errors before and after, none in these files.
* fix(serve): align worker TLS validation
* fix(serve): verify channel worker TLS trust
* fix(serve): close TLS startup review gaps
* fix(serve): allow TLS channels after startup
* fix(serve): align PEM marker attempts with Node
* fix(serve): match PEM loader line semantics
* fix(serve): delegate PEM loading to Node
Close the repeated NODE_EXTRA_CA_CERTS emulation divergence by asking a short-lived child of the worker Node executable which certificates it actually loads. Keep older Node 22 releases fail-closed, inspect production source files without copying combined PEM key material, and pin the current-head buffer, EOF, NUL, BOM, and nested-label regressions.
* fix(serve): make certificate oracle fail closed
* fix(serve): fail closed on legacy CA oracle gaps
* fix(serve): preserve legacy CA loader tolerance
* fix(serve): match legacy CA byte boundaries
* fix(serve): fail closed on legacy CA inspection
* fix(serve): separate failed cert inspection from empty verdicts
* fix(serve): model worker TLS trust the way workers actually verify
* fix(serve): record NODE_TLS_REJECT_UNAUTHORIZED in the serve env guard
* fix(serve): gate loader-oracle tests on tls.getCACertificates
* fix(serve): normalize killed TLS trust probes to the generic failure code
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Co-authored-by: qqqys <266654365+qqqys@users.noreply.github.com>
Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com>1 parent 814b18d commit 526809d
14 files changed
Lines changed: 6118 additions & 116 deletions
File tree
- docs/users
- packages/cli/src
- commands/channel
- serve
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
396 | 396 | | |
397 | 397 | | |
398 | 398 | | |
| 399 | + | |
| 400 | + | |
399 | 401 | | |
400 | 402 | | |
401 | 403 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1332 | 1332 | | |
1333 | 1333 | | |
1334 | 1334 | | |
1335 | | - | |
| 1335 | + | |
1336 | 1336 | | |
1337 | 1337 | | |
1338 | | - | |
1339 | | - | |
1340 | | - | |
1341 | | - | |
1342 | | - | |
1343 | | - | |
1344 | | - | |
1345 | | - | |
| 1338 | + | |
| 1339 | + | |
| 1340 | + | |
| 1341 | + | |
| 1342 | + | |
| 1343 | + | |
| 1344 | + | |
| 1345 | + | |
| 1346 | + | |
| 1347 | + | |
| 1348 | + | |
| 1349 | + | |
| 1350 | + | |
1346 | 1351 | | |
1347 | 1352 | | |
1348 | 1353 | | |
| 1354 | + | |
| 1355 | + | |
| 1356 | + | |
| 1357 | + | |
| 1358 | + | |
| 1359 | + | |
| 1360 | + | |
| 1361 | + | |
| 1362 | + | |
| 1363 | + | |
| 1364 | + | |
| 1365 | + | |
| 1366 | + | |
| 1367 | + | |
| 1368 | + | |
| 1369 | + | |
| 1370 | + | |
| 1371 | + | |
| 1372 | + | |
| 1373 | + | |
| 1374 | + | |
| 1375 | + | |
| 1376 | + | |
| 1377 | + | |
| 1378 | + | |
| 1379 | + | |
| 1380 | + | |
| 1381 | + | |
| 1382 | + | |
| 1383 | + | |
| 1384 | + | |
| 1385 | + | |
| 1386 | + | |
| 1387 | + | |
| 1388 | + | |
| 1389 | + | |
| 1390 | + | |
| 1391 | + | |
| 1392 | + | |
| 1393 | + | |
| 1394 | + | |
| 1395 | + | |
| 1396 | + | |
| 1397 | + | |
1349 | 1398 | | |
1350 | 1399 | | |
1351 | 1400 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
324 | 324 | | |
325 | 325 | | |
326 | 326 | | |
327 | | - | |
328 | | - | |
| 327 | + | |
| 328 | + | |
| 329 | + | |
| 330 | + | |
| 331 | + | |
329 | 332 | | |
330 | 333 | | |
331 | 334 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
141 | 141 | | |
142 | 142 | | |
143 | 143 | | |
| 144 | + | |
| 145 | + | |
| 146 | + | |
| 147 | + | |
| 148 | + | |
| 149 | + | |
| 150 | + | |
| 151 | + | |
| 152 | + | |
| 153 | + | |
| 154 | + | |
| 155 | + | |
| 156 | + | |
| 157 | + | |
| 158 | + | |
| 159 | + | |
| 160 | + | |
| 161 | + | |
| 162 | + | |
| 163 | + | |
| 164 | + | |
| 165 | + | |
| 166 | + | |
| 167 | + | |
| 168 | + | |
| 169 | + | |
| 170 | + | |
| 171 | + | |
| 172 | + | |
| 173 | + | |
| 174 | + | |
| 175 | + | |
| 176 | + | |
| 177 | + | |
144 | 178 | | |
145 | 179 | | |
146 | 180 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
103 | 103 | | |
104 | 104 | | |
105 | 105 | | |
| 106 | + | |
106 | 107 | | |
107 | 108 | | |
108 | 109 | | |
| |||
231 | 232 | | |
232 | 233 | | |
233 | 234 | | |
| 235 | + | |
| 236 | + | |
| 237 | + | |
234 | 238 | | |
235 | 239 | | |
236 | 240 | | |
| |||
0 commit comments