diff --git a/README.md b/README.md index 5a84f60f..024bd3a0 100644 --- a/README.md +++ b/README.md @@ -338,10 +338,25 @@ Such a client is then pointed at the gateway with `HTTPS_PROXY` and with that inbound channel off. No admin rights are needed and the machine-wide system keychain is not touched. On other platforms trust stays file-scoped to the client's own settings. `hyp status` shows the - fingerprint and whether the keychain still trusts it. - `hyp detach ` keeps the CA and the trust, so re-attaching does not - ask again; `hyp detach --purge` and `hyp daemon uninstall` remove - both. + fingerprint, every host the CA is permitted to vouch for, and whether the + keychain still trusts it. `hyp detach ` keeps the CA and the + trust, so re-attaching does not ask again; `hyp detach --purge` + and `hyp daemon uninstall` remove both. +- **On macOS, a proxy attach also leaves a login-session variable behind.** + Bun picks its trust store before any settings file is read, so a keychain + root only counts if `NODE_USE_SYSTEM_CA=1` is already in the process + environment. The attach that trusted the CA therefore ran `launchctl + setenv NODE_USE_SYSTEM_CA 1` and installed a small LaunchAgent, + `~/Library/LaunchAgents/com.hyperparam.hypaware.node-system-ca.plist`, + whose only job is to re-run that command at each login. It stays a login + item on your machine until it is removed, and it is a session-wide + variable that other Node programs read too. `launchctl setenv` reaches + processes launched after it, so a terminal app that was already running + must be fully quit and reopened. `hyp detach ` unsets the + variable and removes the agent, as do `hyp detach --purge` and + `hyp daemon uninstall`; `hyp attach claude` unwinds it when it migrates a + previously proxied machine; and `hyp status` shows whether the variable is + currently live. - **Only the hosts a registered upstream names are decrypted.** Every other host the client talks to is tunnelled through without being decrypted. - **What gets recorded does not change.** Only the recorded API paths are diff --git a/docs/PRIVACY.md b/docs/PRIVACY.md index e20634c9..d237091f 100644 --- a/docs/PRIVACY.md +++ b/docs/PRIVACY.md @@ -81,9 +81,22 @@ rights, and the machine-wide system keychain and other user accounts are never modified. On other platforms the CA is trusted only by Claude Code, through that client's own settings. -**Its lifetime.** `hyp status` shows the fingerprint and whether the -keychain still trusts it. `hyp detach claude` deliberately keeps the CA and -the trust in place, so re-attaching later does not ask for your password +**What else macOS attach leaves behind.** The keychain root only takes +effect if `NODE_USE_SYSTEM_CA=1` is in the environment before Claude Code +starts, so attach also runs `launchctl setenv NODE_USE_SYSTEM_CA 1` and +installs a LaunchAgent at +`~/Library/LaunchAgents/com.hyperparam.hypaware.node-system-ca.plist` that +re-runs that one command at each login. What it runs is `/bin/launchctl` +itself, once, which sets the variable and exits: there is no resident +process, no HypAware code in it, and nothing is sent anywhere. It is still a +login item on your machine, and a session-wide variable that other Node +programs will also read. `hyp detach claude` unsets the variable and removes +the agent, as do `hyp detach claude --purge` and `hyp daemon uninstall`. + +**Its lifetime.** `hyp status` shows the fingerprint, every host the CA is +permitted to vouch for, whether the keychain still trusts it, and whether +the launchd variable is live. `hyp detach claude` deliberately keeps the CA +and the trust in place, so re-attaching later does not ask for your password again; `hyp detach claude --purge` and `hyp daemon uninstall` remove the CA and its keychain trust. diff --git a/src/core/commands/clients.js b/src/core/commands/clients.js index 171bf88e..a79b9786 100644 --- a/src/core/commands/clients.js +++ b/src/core/commands/clients.js @@ -1616,7 +1616,16 @@ async function purgeProxyTrustResidue({ ctx }) { try { const trust = await removeCaTrust({ homeDir }) if (trust.removed) lines.push('removed the HypAware Local CA keychain trust') - else if (trust.detail) lines.push(`! keychain trust could not be removed (${trust.detail})`) + // Removal deletes duplicate roots one pass at a time, so "some went" and + // "something is left" are not exclusive: a detail alongside a removal + // has to be its own line or the residue it names goes unreported. + if (trust.detail) { + lines.push( + trust.removed + ? `! keychain trust may not be fully removed (${trust.detail})` + : `! keychain trust could not be removed (${trust.detail})` + ) + } } catch (err) { lines.push( `! keychain trust could not be removed (${err instanceof Error ? err.message : String(err)})` diff --git a/src/core/commands/status.js b/src/core/commands/status.js index e31fd647..38905a5c 100644 --- a/src/core/commands/status.js +++ b/src/core/commands/status.js @@ -313,9 +313,11 @@ export function renderStatusJson({ report, clientNames, datasets, cacheRoot }) { // `ca_trusted` / `launchd_env_set` are tri-state: `null` means the probe // could not run, which a consumer must not read as "not trusted". // @ref LLP 0237#consequences [implements]: --json carries the trust state next to the CA fingerprint + // @ref LLP 0238#consequences [implements]: and the full permitted host set the grant covers proxy_trust: report.proxyTrust ? { ca_fingerprint: report.proxyTrust.caFingerprint, + permitted_hosts: report.proxyTrust.hosts, ca_trusted: report.proxyTrust.trusted, launchd_env_set: report.proxyTrust.launchdEnvSet, } @@ -539,11 +541,18 @@ export function renderStatusText({ report, clientNames, datasets, cacheRoot, std // dialog was cancelled last month" and "the CA was re-minted and the // keychain still trusts the old one", neither of which any other line here // can be read for. + // + // The permitted hosts are named here and not only in the attach dialog: the + // grant covers every provider host the product can intercept, so on an + // install that captures Claude alone it is wider than anything the config + // shows, and after attach this is the only place it can be re-read. // @ref LLP 0237#consequences [implements]: the trust state is reported next to the CA fingerprint, so a cancelled dialog is diagnosable without re-running attach + // @ref LLP 0238#consequences [implements]: hyp status names all permitted hosts, so the standing grant stays informed and not just the moment it was asked for // @ref LLP 0239#terminals-predating-attach [implements]: and next to it, whether the launchd environment carries the variable if (report.proxyTrust) { stdout.write(' proxy trust:\n') stdout.write(` ca fingerprint: ${report.proxyTrust.caFingerprint}\n`) + stdout.write(` permitted: ${describePermittedHosts(report.proxyTrust.hosts)}\n`) stdout.write(` login keychain: ${describeCaTrust(report.proxyTrust.trusted)}\n`) stdout.write(` launchd env: ${describeLaunchdEnv(report.proxyTrust.launchdEnvSet)}\n`) } @@ -691,6 +700,27 @@ function describeCaTrust(trusted) { return 'unknown - the keychain probe could not run' } +/** + * The host set the trust grant covers. Printed as the certificate's own + * permitted subtrees, in the order the DER carries them, so the line is the + * grant rather than a restatement of the configured providers. + * + * An empty set is not "no hosts": a CA carrying no `dNSName` constraint at + * all can vouch for anything, which is the one reading the user most needs, + * so it is named rather than rendered as a blank line. HypAware's own mint + * never produces one (LLP 0238#full-provider-constraints), so this arm only + * fires for a foreign or damaged certificate at the CA path. That same + * certificate is why `collectProxyTrust` sanitizes the entries before they + * reach here: they are bytes off disk, not strings we wrote. + * + * @param {string[]} hosts + * @returns {string} + */ +function describePermittedHosts(hosts) { + if (hosts.length === 0) return 'no dNSName constraints found - this CA is not host-limited' + return hosts.join(', ') +} + /** * The launchd half of the `proxy trust` block. Same tri-state, same reason. * diff --git a/src/core/daemon/launchd_env.js b/src/core/daemon/launchd_env.js index 779cc098..c2113307 100644 --- a/src/core/daemon/launchd_env.js +++ b/src/core/daemon/launchd_env.js @@ -138,13 +138,17 @@ export async function removeLaunchdEnv({ homeDir, run = defaultRunner } = {}) { /** * Whether the variable is present in the launchd user environment, for - * `hyp status` style reporting. + * `hyp status` style reporting. `timeoutMs` bounds the spawn, for the same + * reason `isCaTrusted` takes one: a status run has nobody waiting on it who + * could decide to give up. * * @param {object} args * @param {TrustCommandRunner} [args.run] + * @param {number} [args.timeoutMs] * @returns {Promise} */ -export async function isLaunchdEnvSet({ run = defaultRunner } = {}) { - const result = await run('launchctl', ['getenv', ENV_VAR_NAME]) +export async function isLaunchdEnvSet({ run, timeoutMs } = {}) { + const runner = run ?? ((cmd, args) => runServiceCommand(cmd, args, { timeoutMs })) + const result = await runner('launchctl', ['getenv', ENV_VAR_NAME]) return result.exitCode === 0 && result.stdout.trim() === ENV_VAR_VALUE } diff --git a/src/core/daemon/service_ops.js b/src/core/daemon/service_ops.js index 0ba72937..5b3fd319 100644 --- a/src/core/daemon/service_ops.js +++ b/src/core/daemon/service_ops.js @@ -46,6 +46,24 @@ export class ServiceManagerSandboxError extends ServiceOpError { } } +/** + * Error raised when a service-manager command outlived the timeout its + * caller set. A rejection rather than a non-zero result on purpose: a + * command that was killed never answered, and a probe that maps exit codes + * to a boolean would otherwise read "killed" as "no" - a false negative + * dressed as a measurement. Callers that already treat "the probe could not + * run" as unknown get that answer for free. + */ +export class ServiceCommandTimeoutError extends ServiceOpError { + /** + * @param {string} message + */ + constructor(message) { + super(message) + this.name = 'ServiceCommandTimeoutError' + } +} + /** * Env var that opts a test back into driving the host's own service * manager. Deliberately awkward to type: there is no legitimate use for @@ -115,24 +133,58 @@ function serviceManagerSpawnRefusal(bin, args) { * {@link ensureOk}). Rejects without spawning under the test runner (see * {@link serviceManagerSpawnRefusal}). * + * `timeoutMs` bounds how long the child may take. Opt-in, because the + * commands that reach here are not alike: a mutation the user is answering + * a password dialog for may legitimately take minutes, while a read-only + * probe run from `hyp status` that has not answered in seconds is not going + * to. Whoever knows which one it is sets the bound. On expiry the child is + * killed and the promise rejects with {@link ServiceCommandTimeoutError}; + * `SIGKILL` rather than `SIGTERM` because the case worth bounding is a + * process blocked on a GUI keychain prompt, which is exactly the state that + * ignores a polite signal. + * * @param {string} bin * @param {string[]} args + * @param {{ timeoutMs?: number }} [opts] * @returns {Promise} */ -export function runServiceCommand(bin, args) { +export function runServiceCommand(bin, args, opts = {}) { const refusal = serviceManagerSpawnRefusal(bin, args) // Reject rather than throw: callers such as `installLaunchAgent`'s // best-effort bootout attach a `.catch()` to the returned promise, which a // synchronous throw would sail straight past. if (refusal) return Promise.reject(refusal) + const { timeoutMs } = opts return new Promise(function(resolve, reject) { const proc = spawn(bin, args, { stdio: ['ignore', 'pipe', 'pipe'] }) let stdout = '' let stderr = '' + let timedOut = false + /** @type {NodeJS.Timeout | undefined} */ + let timer + if (timeoutMs !== undefined) { + timer = setTimeout(function() { + timedOut = true + proc.kill('SIGKILL') + reject(new ServiceCommandTimeoutError( + `'${[bin, ...args].join(' ')}' did not finish within ${timeoutMs}ms and was killed` + )) + }, timeoutMs) + // Nothing should be kept alive waiting for this: the timer exists to + // end a wait, never to extend the process past one. + timer.unref() + } proc.stdout.on('data', function(chunk) { stdout += chunk.toString('utf8') }) proc.stderr.on('data', function(chunk) { stderr += chunk.toString('utf8') }) - proc.on('error', reject) + proc.on('error', function(err) { + if (timer) clearTimeout(timer) + reject(err) + }) proc.on('close', function(code) { + if (timer) clearTimeout(timer) + // The close that follows our own SIGKILL is not an answer, and the + // promise has already rejected with the one that is. + if (timedOut) return resolve({ exitCode: code === null ? -1 : code, stdout, stderr }) }) }) diff --git a/src/core/daemon/status.js b/src/core/daemon/status.js index c4834f83..46d5dbf1 100644 --- a/src/core/daemon/status.js +++ b/src/core/daemon/status.js @@ -30,7 +30,7 @@ import { readLocalOnlyDirs, } from '../usage-policy/index.js' import { readFirstSyncDeadline } from '../usage-policy/first_sync_hold.js' -import { readLocalCaInfo } from '../tls/ca.js' +import { displayableCaHosts, readLocalCaInfo } from '../tls/ca.js' import { isCaTrusted as probeCaTrusted } from '../tls/darwin_trust.js' import { isLaunchdEnvSet as probeLaunchdEnvSet } from './launchd_env.js' import { resolveClientSettingsPath } from './client_settings_path.js' @@ -1463,8 +1463,10 @@ export async function collectHypAwareStatus(opts = {}) { const proxyTrust = await collectProxyTrust({ platform, stateRoot, - isCaTrustedFn: opts.isCaTrusted ?? probeCaTrusted, - isLaunchdEnvSetFn: opts.isLaunchdEnvSet ?? probeLaunchdEnvSet, + isCaTrustedFn: opts.isCaTrusted + ?? ((args) => probeCaTrusted({ ...args, timeoutMs: TRUST_PROBE_TIMEOUT_MS })), + isLaunchdEnvSetFn: opts.isLaunchdEnvSet + ?? (() => probeLaunchdEnvSet({ timeoutMs: TRUST_PROBE_TIMEOUT_MS })), }) // ----- recent errors ----- @@ -1523,6 +1525,19 @@ export async function collectHypAwareStatus(opts = {}) { } } +/** + * How long either trust probe may take before `hyp status` gives up on it. + * + * Both are table reads (`security verify-cert` against a local root with no + * AIA to chase, `launchctl getenv`), so the bound is not a performance + * budget: it is there because a locked login keychain can put `security` + * behind a GUI prompt, and `hyp status` is a report, not a dialog - nobody + * is watching it who could decide to stop waiting. Timing out reports + * `unknown` for that half, which is the honest answer and is exactly what + * the probe-failure path already renders. + */ +const TRUST_PROBE_TIMEOUT_MS = 5_000 + /** * Proxy mode's two invisible preconditions, read once so `hyp status` can * state them: does the login keychain still trust the CA on disk @@ -1539,10 +1554,26 @@ export async function collectHypAwareStatus(opts = {}) { * The two probes shell out, so each is caught independently: a probe that * could not run reports `null` (unknown), never `false`, because "the * dialog was cancelled" and "`security` did not run" are different answers - * and only the first is actionable. Nothing here carries text from another - * process onto the terminal - the fingerprint is computed locally from the - * DER and is `[0-9A-F:]` by construction, and probe stderr is deliberately - * not surfaced - so no LLP 0225 sanitizing applies. + * and only the first is actionable. The fingerprint is computed locally from + * the DER and is `[0-9A-F:]` by construction, and probe stderr is deliberately + * not surfaced, so neither needs bounding. + * + * The permitted host set travels with the fingerprint because the grant is + * wider than any one install uses: the CA is constrained to the whole static + * provider set, so a user who trusts it while capturing only Claude still + * carries a grant covering `api.openai.com` and `chatgpt.com`. The attach + * dialog names them; so must this, or the standing grant is only ever stated + * once, at the moment it is asked for. The strings come from the DER's own + * permitted subtrees, so they are the grant itself rather than a + * config-derived guess that could drift from it. + * + * That last property is also why the hosts are the one field here that does + * need sanitizing (LLP 0225): they are bytes off disk rather than strings we + * wrote, so a foreign or damaged certificate at the CA path can carry an + * `ESC` run, a newline, or ten thousand subtrees into a line `hyp status` + * prints. `displayableCaHosts` is that policy, shared with the attach dialog + * that names the same grant, and applied here at collection like every other + * label in this file so `--json` carries exactly what was printed. * * @param {object} args * @param {NodeJS.Platform} args.platform @@ -1551,6 +1582,7 @@ export async function collectHypAwareStatus(opts = {}) { * @param {() => Promise} args.isLaunchdEnvSetFn * @returns {Promise} * @ref LLP 0237#consequences [implements]: hyp status reports the trust state alongside the CA fingerprint, so a cancelled dialog is diagnosable without re-running attach + * @ref LLP 0238#consequences [implements]: hyp status names all permitted hosts, so a grant wider than the configured providers stays informed * @ref LLP 0239#terminals-predating-attach [implements]: hyp status reports whether the variable is present in the launchd environment */ async function collectProxyTrust({ platform, stateRoot, isCaTrustedFn, isLaunchdEnvSetFn }) { @@ -1580,7 +1612,7 @@ async function collectProxyTrust({ platform, stateRoot, isCaTrustedFn, isLaunchd launchdEnvSet = null } - return { caFingerprint: ca.fingerprint, trusted, launchdEnvSet } + return { caFingerprint: ca.fingerprint, hosts: displayableCaHosts(ca.hosts), trusted, launchdEnvSet } } /** diff --git a/src/core/daemon/types.d.ts b/src/core/daemon/types.d.ts index c5ea98f7..f967d236 100644 --- a/src/core/daemon/types.d.ts +++ b/src/core/daemon/types.d.ts @@ -310,6 +310,17 @@ export interface ServiceState { export interface ProxyTrustReport { /** SHA-256 fingerprint of the CA on disk, colon-separated uppercase hex. */ caFingerprint: string + /** + * The CA's permitted `dNSName` subtrees: every host this grant lets the CA + * vouch for, which is the full provider set and not the subset this install + * captures (LLP 0238#full-provider-constraints). Empty only for a + * certificate carrying no dNSName constraints at all. Passed through + * `displayableCaHosts`, because the bytes come from the certificate on disk + * rather than from us (LLP 0225): entries are sanitized, and a list longer + * than any real CA's ends in a `(+N more ...)` entry rather than being + * silently shortened. + */ + hosts: string[] /** `security verify-cert -p ssl` against the CA, or null when it could not run. */ trusted: boolean | null /** `launchctl getenv NODE_USE_SYSTEM_CA` is `1`, or null when it could not run. */ diff --git a/src/core/tls/ca.js b/src/core/tls/ca.js index 79f7e9f9..e6cb8258 100644 --- a/src/core/tls/ca.js +++ b/src/core/tls/ca.js @@ -6,7 +6,7 @@ import os from 'node:os' import path from 'node:path' import tls from 'node:tls' -import { atomicWriteFile, errCode } from 'hypaware/core/util' +import { atomicWriteFile, errCode, sanitizeLabel } from 'hypaware/core/util' import { generateKeyPair, mintCertificate, readNameConstraints } from './x509.js' /** @@ -375,6 +375,50 @@ export async function readLocalCaInfo({ stateRoot }) { } } +/** + * How many permitted subtrees any surface will name before it stops listing + * and starts counting. Our own mint produces one entry per provider host, so + * the bound is never reached by a certificate this build wrote; it exists for + * the certificate it did not. + */ +const MAX_NAMED_CA_HOSTS = 24 + +/** + * `LocalCaInfo.hosts` made safe to put in front of a person. + * + * The entries are the only part of a read-back CA that is bytes off disk + * rather than a string this build wrote: a `dNSName` is an IA5String, and + * `readNameConstraints` hands back whatever the DER held, decoded as latin1, + * with no charset, length or count check anywhere on the way. Our own mint + * refuses a non-printable host (`assertAsciiHost`), so reaching one takes a + * foreign or damaged certificate at the CA path - which is exactly the case + * both callers already have an arm for, so it is reachable by construction. + * All three of the ways such a value is hostile are answered here, at the one + * point the hosts leave this module for a terminal: control and invisible + * bytes and unbounded length (`sanitizeLabel`), and unbounded count (the + * bound below). + * + * Strip rather than escape, and nothing is silently dropped. LLP 0225#scope + * leaves `hyp status` and the attach adapters to make that argument for + * themselves, and here it is: these are label-plane values, so the shortest + * safe answer is to drop the bytes - but both callers exist to state how wide + * a trust grant is, and a short list understates it. So an entry that + * sanitizes away is still named as an entry, and a truncated list says how + * much it left out. + * + * @ref LLP 0225#scope [implements]: the per-surface strip-versus-escape argument 0225 leaves to hyp status and the attach adapters + * @param {string[]} hosts + * @returns {string[]} + */ +export function displayableCaHosts(hosts) { + const named = hosts + .slice(0, MAX_NAMED_CA_HOSTS) + .map((host) => sanitizeLabel(host) ?? '(unprintable dNSName)') + const omitted = hosts.length - named.length + if (omitted > 0) named.push(`(+${omitted} more dNSName constraints)`) + return named +} + /** * Wait, bounded, for the gateway to mint the local CA. Two callers, one * reason: proxy attach preflights on the CA file's existence (LLP 0232), so diff --git a/src/core/tls/darwin_trust.js b/src/core/tls/darwin_trust.js index 38891401..40abc2ae 100644 --- a/src/core/tls/darwin_trust.js +++ b/src/core/tls/darwin_trust.js @@ -51,13 +51,21 @@ export function loginKeychainPath(homeDir = os.homedir()) { * to show the user a password dialog at all. * @ref LLP 0237#trust-preflight-is-idempotent [implements] * + * `timeoutMs` bounds the spawn for callers that cannot afford to block on it. + * Silent is the expectation, not a guarantee: a locked login keychain can put + * `security` in front of a GUI prompt, and a caller nobody is watching (`hyp + * status`) would then wait on an answer forever. Left unset the wait is + * unbounded, which is what an interactive attach wants. + * * @param {object} args * @param {string} args.certPath * @param {TrustCommandRunner} [args.run] + * @param {number} [args.timeoutMs] * @returns {Promise} */ -export async function isCaTrusted({ certPath, run = defaultRunner }) { - const result = await run('security', ['verify-cert', '-c', certPath, '-p', 'ssl']) +export async function isCaTrusted({ certPath, run, timeoutMs }) { + const runner = run ?? ((cmd, cmdArgs) => runServiceCommand(cmd, cmdArgs, { timeoutMs })) + const result = await runner('security', ['verify-cert', '-c', certPath, '-p', 'ssl']) return result.exitCode === 0 } @@ -85,13 +93,32 @@ export async function installCaTrust({ certPath, homeDir, run = defaultRunner }) return { installed: false, detail: (result.stderr || result.stdout).trim() || `exit ${result.exitCode}` } } +/** + * How many `delete-certificate` passes a single removal will make. High + * enough to clear any plausible re-mint history, low enough that a + * `security` build which somehow kept reporting success could never spin + * here. Exhausting it is reported, never silently accepted. + */ +const MAX_TRUST_REMOVAL_PASSES = 8 + /** * Remove the CA and its trust settings from the login keychain. `-t` deletes * the user-domain trust settings along with the certificate, mirroring the * install; without it a removed certificate leaves orphaned trust behind. * - * Idempotent: a certificate that is not there is the desired end state, and - * `security` reporting "could not be found" is success. + * `delete-certificate -c` addresses a certificate by common name, and every + * CA this product mints carries the same one, so a machine whose CA has been + * re-minted holds several indistinguishable trusted roots. One invocation + * clears one of them; the rest would outlive the uninstall that was supposed + * to end the grant, each still vouching for the provider set, and none of + * them holding a key the user still has. So this deletes in a bounded loop + * until the keychain reports no match left, which is also why "could not be + * found" has to read as the end state rather than as a failure. + * @ref LLP 0238#ca-survives-detach [implements]: uninstall and purge are the two paths that end the grant, so they must end all of it + * + * Idempotent at every entry point: a keychain with no matching certificate + * makes the first pass the last one and reports `removed: false` with no + * detail, which is the desired end state and not an error. * * @param {object} args * @param {string} [args.homeDir] @@ -99,14 +126,29 @@ export async function installCaTrust({ certPath, homeDir, run = defaultRunner }) * @returns {Promise<{ removed: boolean, detail?: string }>} */ export async function removeCaTrust({ homeDir, run = defaultRunner }) { - const result = await run('security', [ + const args = [ 'delete-certificate', '-c', CA_COMMON_NAME, '-t', loginKeychainPath(homeDir), - ]) - if (result.exitCode === 0) return { removed: true } - const detail = (result.stderr || result.stdout).trim() - if (/could not be found|SecKeychainSearchCopyNext/i.test(detail)) return { removed: false } - return { removed: false, detail: detail || `exit ${result.exitCode}` } + ] + let removed = false + for (let pass = 0; pass < MAX_TRUST_REMOVAL_PASSES; pass += 1) { + const result = await run('security', args) + if (result.exitCode === 0) { + removed = true + continue + } + const detail = (result.stderr || result.stdout).trim() + // Nothing left under this common name: the loop's exit condition, and on + // the first pass the already-clean case. + if (/could not be found|SecKeychainSearchCopyNext/i.test(detail)) return { removed } + return { removed, detail: detail || `exit ${result.exitCode}` } + } + return { + removed, + detail: + `stopped after ${MAX_TRUST_REMOVAL_PASSES} passes; more certificates named ` + + `'${CA_COMMON_NAME}' may remain - remove them in Keychain Access`, + } } diff --git a/test/core/service-manager-test-sandbox.test.js b/test/core/service-manager-test-sandbox.test.js index 4507cd93..b6cecc2b 100644 --- a/test/core/service-manager-test-sandbox.test.js +++ b/test/core/service-manager-test-sandbox.test.js @@ -237,3 +237,75 @@ test('the explicit opt-in still spawns', () => { assert.deepEqual(JSON.parse(run.stdout), { exitCode: 0, stdout: 'spawned', stderr: '' }) }) }) + +// `hyp status` shells out through this helper on darwin (`security +// verify-cert`, `launchctl getenv`). A locked login keychain can put +// `security` behind a GUI prompt, and an unbounded spawn then hangs a command +// nobody is waiting on interactively. The bound has to live here, at the one +// spawn seam, and it has to reject rather than return a non-zero result: a +// probe that maps exit codes to a boolean would read a killed process as +// "not trusted". +// +// Driven in a child for the same reason as the opt-in test above: the +// guard's env opt-in is process-global, and the command spawned here is a +// sleeping `node`, never a service manager. +test('runServiceCommand kills a command that outlives its timeout', () => { + withTempDir((dir) => { + writeFileSync(path.join(dir, 'package.json'), '{"type":"module"}') + const script = path.join(dir, 'timeout.test.js') + writeFileSync(script, [ + `import { runServiceCommand } from ${JSON.stringify(SERVICE_OPS_URL)}`, + 'const started = Date.now()', + 'let outcome', + 'try {', + " const res = await runServiceCommand(process.execPath, ['-e', 'setTimeout(() => {}, 120000)'], { timeoutMs: 250 })", + " outcome = { kind: 'resolved', res }", + '} catch (err) {', + " outcome = { kind: 'rejected', name: err.name, message: err.message }", + '}', + 'process.stdout.write(JSON.stringify({ ...outcome, elapsed: Date.now() - started }))', + '', + ].join('\n')) + + const run = spawnSync(process.execPath, [script], { + encoding: 'utf8', + env: { ...process.env, [ALLOW_REAL_SERVICE_MANAGER_ENV]: '1' }, + // Well under the 120s the spawned child would otherwise sleep for, so + // an unbounded helper fails here as a timeout rather than passing late. + timeout: 20_000, + }) + + assert.equal(run.status, 0, `${run.stdout ?? ''}${run.stderr ?? ''}`) + const outcome = JSON.parse(run.stdout) + assert.equal(outcome.kind, 'rejected', `expected a rejection, got ${run.stdout}`) + assert.equal(outcome.name, 'ServiceCommandTimeoutError') + assert.match(outcome.message, /did not finish within 250ms and was killed/) + // And the child really was killed: the script exited long before the + // 120s its own child was told to sleep for. + assert.ok(outcome.elapsed < 15_000, `waited ${outcome.elapsed}ms`) + }) +}) + +// Nothing sets a bound by accident: with no `timeoutMs` the helper waits, and +// every mutation that raises a macOS password dialog depends on that. +test('runServiceCommand leaves the wait unbounded when no timeout is given', () => { + withTempDir((dir) => { + writeFileSync(path.join(dir, 'package.json'), '{"type":"module"}') + const script = path.join(dir, 'unbounded.test.js') + writeFileSync(script, [ + `import { runServiceCommand } from ${JSON.stringify(SERVICE_OPS_URL)}`, + "const res = await runServiceCommand(process.execPath, ['-e', 'setTimeout(() => {}, 800)'])", + 'process.stdout.write(JSON.stringify(res))', + '', + ].join('\n')) + + const run = spawnSync(process.execPath, [script], { + encoding: 'utf8', + env: { ...process.env, [ALLOW_REAL_SERVICE_MANAGER_ENV]: '1' }, + timeout: 20_000, + }) + + assert.equal(run.status, 0, `${run.stdout ?? ''}${run.stderr ?? ''}`) + assert.deepEqual(JSON.parse(run.stdout), { exitCode: 0, stdout: '', stderr: '' }) + }) +}) diff --git a/test/core/status-proxy-trust.test.js b/test/core/status-proxy-trust.test.js index 91b64b73..ce72f1b9 100644 --- a/test/core/status-proxy-trust.test.js +++ b/test/core/status-proxy-trust.test.js @@ -2,6 +2,7 @@ import test from 'node:test' import assert from 'node:assert/strict' +import crypto from 'node:crypto' import fs from 'node:fs/promises' import os from 'node:os' import path from 'node:path' @@ -10,6 +11,7 @@ import { collectHypAwareStatus } from '../../src/core/daemon/status.js' import { renderStatusJson, renderStatusText } from '../../src/core/commands/status.js' import { defaultConfigPath } from '../../src/core/config/schema.js' import { ensureLocalCa } from '../../src/core/tls/ca.js' +import { derToPem } from '../../src/core/tls/x509.js' /** @import { CollectStatusOptions, HypAwareStatusReport } from '../../src/core/daemon/types.js' */ @@ -75,6 +77,7 @@ test('hyp status reports the trust state alongside the CA fingerprint, and the l const report = await collectHypAwareStatus(collectOpts(hypHome)) assert.deepEqual(report.proxyTrust, { caFingerprint: ca.fingerprint, + hosts: ['api.anthropic.com'], trusted: true, launchdEnvSet: true, }) @@ -93,6 +96,7 @@ test('hyp status reports the trust state alongside the CA fingerprint, and the l }) assert.deepEqual(json.proxy_trust, { ca_fingerprint: ca.fingerprint, + permitted_hosts: ['api.anthropic.com'], ca_trusted: true, launchd_env_set: true, }) @@ -101,6 +105,91 @@ test('hyp status reports the trust state alongside the CA fingerprint, and the l } }) +// The grant is wider than the install: the CA permits every provider host the +// product can ever intercept, so a user capturing Claude alone still carries +// one covering the OpenAI hosts. The attach dialog names them once; after that +// this is the only surface that can, and LLP 0238 requires it to. +test('every permitted host the trust grant covers is named on both surfaces', async () => { + const { hypHome, stateRoot } = await makeHome() + try { + // The full static provider set of LLP 0238#full-provider-constraints, on + // an install that captures only the first of them. + const hosts = ['api.anthropic.com', 'api.openai.com', 'chatgpt.com'] + const ca = await ensureLocalCa({ stateRoot, hosts }) + + const report = await collectHypAwareStatus(collectOpts(hypHome)) + assert.deepEqual(report.proxyTrust?.hosts, hosts) + + const text = renderText(report, path.join(stateRoot, 'cache')) + for (const host of hosts) { + assert.ok(text.includes(host), `${host} is named on the text surface`) + } + assert.match(text, /permitted: {6}api\.anthropic\.com, api\.openai\.com, chatgpt\.com\n/) + + const json = renderStatusJson({ + report, + clientNames: [], + datasets: [], + cacheRoot: path.join(stateRoot, 'cache'), + }) + assert.deepEqual(json.proxy_trust?.permitted_hosts, hosts) + // The hosts are read back off the certificate, not from config, so the + // line can never drift from what the keychain actually vouches for. + assert.deepEqual(json.proxy_trust?.ca_fingerprint, ca.fingerprint) + } finally { + await fs.rm(hypHome, { recursive: true, force: true }) + } +}) + +// The permitted hosts are the one field of this report that is bytes off disk +// rather than a string we wrote: a `dNSName` is an IA5String, read out of +// whatever certificate sits at the CA path and decoded as latin1 with no +// charset check anywhere on the way. Our own mint refuses a non-printable host +// (`assertAsciiHost`), so the only way to one is a foreign or damaged +// certificate at that path - which is exactly the case the "not host-limited" +// arm of the renderer exists for. It must not be able to repaint the terminal +// of the person reading `hyp status`. +// @ref LLP 0225#decision [tests]: a status label carrying captured bytes is sanitized before it is rendered +test('a permitted host carrying terminal control bytes cannot repaint hyp status', async () => { + const { hypHome, stateRoot } = await makeHome() + try { + // Minted with a filler of the payload's exact width, then byte-patched in + // the DER: same length keeps every ASN.1 length valid, and nothing on the + // read path verifies the signature the patch invalidates. This is the only + // way to produce the certificate, since the mint refuses the host outright. + const hostile = 'evil\u001b[2K\nforged.example' + const filler = 'z'.repeat(hostile.length) + await ensureLocalCa({ stateRoot, hosts: ['api.anthropic.com', filler] }) + + const certPath = path.join(stateRoot, 'tls', 'ca-cert.pem') + const der = Buffer.from(new crypto.X509Certificate(await fs.readFile(certPath, 'utf8')).raw) + const at = der.indexOf(Buffer.from(filler, 'latin1')) + assert.ok(at >= 0, 'the filler host is in the DER to patch') + Buffer.from(hostile, 'latin1').copy(der, at) + await fs.writeFile(certPath, derToPem(der)) + + const report = await collectHypAwareStatus(collectOpts(hypHome)) + // Stripped, not dropped: the subtree is still part of the grant, so it is + // still named, just without the bytes that drive a terminal. + assert.deepEqual(report.proxyTrust?.hosts, ['api.anthropic.com', 'evil[2Kforged.example']) + + const text = renderText(report, path.join(stateRoot, 'cache')) + assert.ok(!text.includes('\u001b'), 'no escape sequence reaches the text surface') + assert.match(text, /permitted: {6}api\.anthropic\.com, evil\[2Kforged\.example\n/) + + const json = renderStatusJson({ + report, + clientNames: [], + datasets: [], + cacheRoot: path.join(stateRoot, 'cache'), + }) + // Sanitized at collection, so `--json` carries exactly what was printed. + assert.deepEqual(json.proxy_trust?.permitted_hosts, ['api.anthropic.com', 'evil[2Kforged.example']) + } finally { + await fs.rm(hypHome, { recursive: true, force: true }) + } +}) + // The state this exists for: the dialog was cancelled, or a re-mint stranded // the trust. Capture keeps working, so nothing else in the report degrades and // only this line can say so. diff --git a/test/core/tls-ca.test.js b/test/core/tls-ca.test.js index 41d3f821..f573ceb7 100644 --- a/test/core/tls-ca.test.js +++ b/test/core/tls-ca.test.js @@ -12,6 +12,7 @@ import { caPaths, createLeafStore, deleteLocalCa, + displayableCaHosts, ensureLocalCa, readLocalCaInfo, waitForLocalCa, @@ -231,3 +232,65 @@ test('waitForLocalCa reports not-ready at the deadline when no CA ever appears', assert.deepEqual(result, { ready: false }) assert.equal(polls, 4, 'exactly the polls the deadline allows, then it stops') }) + +// `LocalCaInfo.hosts` is the only part of a read-back CA that is bytes off +// disk: `readNameConstraints` hands back whatever the DER's permitted subtrees +// held, decoded as latin1, with no charset, length or count check. Two +// surfaces put those bytes in front of a person (`hyp status`, and the line +// the attach dialog writes just before macOS raises its password prompt), so +// the policy that makes them safe is shared and lives here. +// @ref LLP 0225#scope [tests]: the label-plane surfaces 0225 left to argue for themselves strip, and say what they stripped +test('displayableCaHosts leaves an ordinary permitted set exactly as it is', () => { + const hosts = ['api.anthropic.com', 'api.openai.com', 'chatgpt.com'] + assert.deepEqual(displayableCaHosts(hosts), hosts) + assert.deepEqual(displayableCaHosts([]), []) +}) + +test('displayableCaHosts strips bytes that would drive a terminal', () => { + assert.deepEqual( + displayableCaHosts(['api.anthropic.com', `evil\nforged.example`]), + ['api.anthropic.com', 'evil[2Kforged.example'] + ) +}) + +// Stripped, never dropped: both callers exist to state how wide a trust grant +// is, and an entry that vanished would understate it by exactly one subtree. +test('displayableCaHosts names a host that sanitizes away rather than losing it', () => { + const shown = displayableCaHosts(['', 'api.anthropic.com']) + assert.equal(shown.length, 2) + assert.equal(shown[0], '(unprintable dNSName)') + assert.equal(shown[1], 'api.anthropic.com') +}) + +// The third way one of these values is hostile, after control bytes and +// length: count. Nothing between the certificate file and the terminal bounds +// how many subtrees it carries, and a list too long to read is a list that +// hides what it says. +test('displayableCaHosts bounds the list and says how much it left out', () => { + const many = Array.from({ length: 30 }, (_, i) => `h${i}.example`) + const shown = displayableCaHosts(many) + + assert.equal(shown.length, 25, '24 named hosts plus the count of the rest') + assert.deepEqual(shown.slice(0, 24), many.slice(0, 24)) + assert.equal(shown[24], '(+6 more dNSName constraints)') + // A list that exactly fills the bound is not truncated, so no reader is + // told something was withheld when nothing was. + assert.deepEqual(displayableCaHosts(many.slice(0, 24)), many.slice(0, 24)) +}) + +// The bound has to survive a real certificate, not just an array: the same +// hosts have to come back out of the DER for `hyp status` to be reading the +// grant rather than a copy of the config. +test('a CA carrying more subtrees than the bound reports them bounded', async (t) => { + const stateRoot = await tempRoot() + t.after(() => fsp.rm(stateRoot, { recursive: true, force: true })) + + const hosts = Array.from({ length: 30 }, (_, i) => `h${i}.example`) + await ensureLocalCa({ stateRoot, hosts }) + + const info = await readLocalCaInfo({ stateRoot }) + assert.equal(info?.hosts.length, 30, 'the certificate itself keeps every constraint') + const shown = displayableCaHosts(/** @type {string[]} */ (info?.hosts)) + assert.equal(shown.length, 25) + assert.equal(shown[24], '(+6 more dNSName constraints)') +}) diff --git a/test/core/tls-darwin-trust.test.js b/test/core/tls-darwin-trust.test.js index 5c969ef1..da995cf2 100644 --- a/test/core/tls-darwin-trust.test.js +++ b/test/core/tls-darwin-trust.test.js @@ -31,6 +31,31 @@ function recordingRunner(result) { return { calls, run } } +/** What `security` says once nothing matches the common name any more. */ +const NOT_FOUND_STDERR = + 'security: SecKeychainSearchCopyNext: The specified item could not be found in the keychain.' + +/** + * A keychain holding `count` certificates under the same common name, the + * shape a re-minted machine really has. Each `delete-certificate` removes + * one; once the last is gone, `security` reports no match. + * + * @param {number} count + */ +function keychainWithDuplicates(count) { + let remaining = count + /** @type {{ cmd: string, args: string[] }[]} */ + const calls = [] + /** @type {TrustCommandRunner} */ + const run = async (cmd, args) => { + calls.push({ cmd, args }) + if (remaining === 0) return { exitCode: 1, stdout: '', stderr: NOT_FOUND_STDERR } + remaining -= 1 + return { exitCode: 0, stdout: '', stderr: '' } + } + return { calls, run, remaining: () => remaining } +} + // The trust CN is how removal finds the certificate, so it must be exactly the // CN the CA mints (`CA_SUBJECT` in ca.js). A drift here would install one name // and try to delete another, stranding trusted roots. @@ -92,23 +117,79 @@ test('a cancelled dialog surfaces as installed:false with the detail', async () }) test('removeCaTrust deletes trust settings too and is idempotent', async () => { - const { calls, run } = recordingRunner({ exitCode: 0 }) - const removed = await removeCaTrust({ homeDir: '/Users/u', run }) + const keychain = keychainWithDuplicates(1) + const removed = await removeCaTrust({ homeDir: '/Users/u', run: keychain.run }) assert.equal(removed.removed, true) - assert.deepEqual(calls[0].args, [ + assert.equal(removed.detail, undefined) + assert.deepEqual(keychain.calls[0].args, [ 'delete-certificate', '-c', CA_COMMON_NAME, '-t', '/Users/u/Library/Keychains/login.keychain-db', ]) - const absent = recordingRunner({ - exitCode: 1, - stderr: 'security: SecKeychainSearchCopyNext: The specified item could not be found in the keychain.', - }) + const absent = recordingRunner({ exitCode: 1, stderr: NOT_FOUND_STDERR }) const alreadyGone = await removeCaTrust({ run: absent.run }) assert.equal(alreadyGone.removed, false) assert.equal(alreadyGone.detail, undefined) + // One pass, not a bounded sweep, when there was never anything to delete. + assert.equal(absent.calls.length, 1) +}) + +// Every HypAware CA carries the same common name, and `delete-certificate -c` +// deletes one certificate per call, so a machine whose CA has been re-minted +// keeps trusting the older roots after an uninstall that reported success. +// Those roots outlive the key they were minted with, and nothing else ever +// looks for them again. +// @ref LLP 0238#ca-survives-detach [tests]: purge and uninstall end the whole grant, not one certificate of it +test('removeCaTrust clears every identically named root, not just the first', async () => { + const keychain = keychainWithDuplicates(3) + const result = await removeCaTrust({ homeDir: '/Users/u', run: keychain.run }) + + assert.equal(result.removed, true) + assert.equal(result.detail, undefined) + assert.equal(keychain.remaining(), 0, 'no identically named root survives the removal') + // Three deletions plus the pass that finds nothing left, which is the loop's + // only exit condition. + assert.equal(keychain.calls.length, 4) + for (const call of keychain.calls) { + assert.deepEqual(call.args, [ + 'delete-certificate', + '-c', CA_COMMON_NAME, + '-t', + '/Users/u/Library/Keychains/login.keychain-db', + ]) + } +}) + +// A real failure (a locked keychain, a denied authorization) is not "nothing +// left to delete", so the sweep stops on it and hands the detail back rather +// than retrying into a wall. +test('removeCaTrust stops on a real failure and reports what it managed', async () => { + let pass = 0 + /** @type {TrustCommandRunner} */ + const run = async () => { + pass += 1 + if (pass === 1) return { exitCode: 0, stdout: '', stderr: '' } + return { exitCode: 1, stdout: '', stderr: 'security: User interaction is not allowed.' } + } + const result = await removeCaTrust({ run }) + assert.equal(pass, 2) + assert.equal(result.removed, true) + assert.match(result.detail ?? '', /User interaction is not allowed/) +}) + +// The loop is bounded: a `security` that never stops reporting success must +// not spin, and the residue it leaves behind is named rather than assumed +// away. +test('removeCaTrust is bounded and says so when the bound is hit', async () => { + const { calls, run } = recordingRunner({ exitCode: 0 }) + const result = await removeCaTrust({ run }) + + assert.equal(result.removed, true) + assert.equal(calls.length, 8) + assert.match(result.detail ?? '', /stopped after 8 passes/) + assert.match(result.detail ?? '', /Keychain Access/) }) test('loginKeychainPath resolves under the given home', () => {