diff --git a/AGENTS.md b/AGENTS.md index bb0cdc8..dd5a1dd 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -10,7 +10,7 @@ The system uses the VS Code **companion extension pattern**: two extensions comm - `devcontainer-dev-certs.getAllCertMaterialV3({ includeDotNetDev, includeUserCerts })` — current multi-cert pull entry point with password-preserving `pfxBase64` and per-cert `installToDotNetStore` flag. - `devcontainer-dev-certs.getAllCertMaterial({ includeDotNetDev, includeUserCerts })` — v2 multi-cert pull entry point. Kept for workspace extensions pinned to the V2 wire contract. - `devcontainer-dev-certs.getCertMaterial(autoProvision)` — legacy single-cert pull entry point. Returns `null` when the host has disabled dotnet cert generation. - - `devcontainer-dev-certs.acceptContainerDevCert({ thumbprint, pemCertBase64 })` — **reverse-sync push entry point** (issue #63). Takes a **public-cert-only** PEM pushed from a Dev Container that opted into `syncContainerCert`, independently re-validates (`isValidDevCert` + `validateLocalSans`), prompts for one-time consent (`containerCertProvisionConsented` global state — distinct from the host-generation consent because the user is approving trust of a cert that came from a container they may or may not control), and **only trusts** the cert in the host's OS trust surfaces (Root store / OpenSSL trust dir / NSS / keychain trust). Does NOT save to `CurrentUser/My`, the keychain identity slot, or the .NET `my/` dir; the host doesn't need the private key (Kestrel runs in the container with its own copy). Gated on the SAME host settings as the generation flow: `devcontainerDevCerts.generateDotNetCert` and `devcontainer-dev-certs.autoProvision`. SAN-local restriction has an opt-out via `devcontainerDevCerts.allowNonLocalContainerCertSans`. Idempotent on repeat pushes — no `alreadyTrusted` short-circuit, each platform's `trustCertificate` is a no-op for an already-trusted cert. + - `devcontainer-dev-certs.acceptContainerDevCert({ thumbprint, pemCertBase64 })` — **reverse-sync push entry point** (issue #63). Takes a **public-cert-only** PEM pushed from a Dev Container that opted into `syncContainerCert`, independently re-validates (`isValidDevCert` + `validateLeafTrustShape` + `validateLocalSans`), prompts for one-time consent (`containerCertProvisionConsented` global state — distinct from the host-generation consent because the user is approving trust of a cert that came from a container they may or may not control), and **only trusts** the cert in the host's OS trust surfaces (Root store / OpenSSL trust dir / NSS / keychain trust). Does NOT save to `CurrentUser/My`, the keychain identity slot, or the .NET `my/` dir; the host doesn't need the private key (Kestrel runs in the container with its own copy). Gated on the SAME host settings as the generation flow: `devcontainerDevCerts.generateDotNetCert` and `devcontainer-dev-certs.autoProvision`. SAN-local restriction has an opt-out via `devcontainerDevCerts.allowNonLocalContainerCertSans`. Idempotent on repeat pushes, but by an explicit check rather than a platform guarantee: `CertManager.trustExternalCertificate` calls `store.isCertTrusted(cert)` and returns before invoking `trustCertificate`. Do not remove that short-circuit on the assumption that re-trusting is free — it is not on macOS, where `security add-trusted-cert` re-touches the trust-settings record and can re-prompt for the keychain password. Platform trust for the auto-generated cert is handled via PowerShell (Windows), the `security` CLI (macOS), and file-based stores with OpenSSL rehash (Linux). User-managed certs are never added to the host OS trust store. Container-pushed dev certs (when both opt-ins are on) ARE added to the host OS trust store via the same platform path as the auto-generated cert. @@ -21,7 +21,7 @@ The system uses the VS Code **companion extension pattern**: two extensions comm Additionally, when `DEVCONTAINER_DEV_CERTS_SYNC_FROM_CONTAINER=true` (set by the `syncContainerCert` feature option), the workspace extension runs `pushContainerCertToHost()` **before** the standard pull. That scans `~/.dotnet/corefx/cryptography/x509stores/my/*.pfx` using the same shared `classifyCandidate` + `selectBestDevCert` rules the host uses on its own platform stores, pre-validates the winning candidate, and pushes it to the host via `acceptContainerDevCert`. The standard pull still runs afterwards (V3 pulls naturally return whatever cert the host now has trusted, so the container side ends up with the same cert it pushed). -- **Shared package** (`src/shared/`) — TypeScript-only, no vscode dependency. Houses the cert primitives both extensions need: `cert/types.ts` (DevCert/DevKey wrappers around `@peculiar/x509`), `cert/properties.ts` (OIDs, version constants, default SANs), `cert/pfx.ts` (PKCS#12 build/parse), `cert/loader.ts` (PFX + PEM file loading), `cert/validation.ts` (`isValidDevCert`, `getCertificateVersion`, `validateLocalSans`), and `cert/classify.ts` (`classifyCandidate`, `selectBestDevCert`, `extractThumbprintHintFromFilename`). The classifier is side-effect-free — callers in vscode contexts opt into a localized log line by passing `onSkipped` / `onMultipleCandidates` callbacks. Both extensions independently localize via their own `vscode.l10n.t` bundles. +- **Shared package** (`src/shared/`) — TypeScript-only, no vscode dependency. Houses the cert primitives both extensions need: `cert/types.ts` (DevCert/DevKey wrappers around `@peculiar/x509`), `cert/properties.ts` (OIDs, version constants, default SANs), `cert/pfx.ts` (PKCS#12 build/parse), `cert/loader.ts` (PFX + PEM file loading), `cert/validation.ts` (`isValidDevCert`, `getCertificateVersion`, `validateLocalSans`), `cert/classify.ts` (`classifyCandidate`, `selectBestDevCert`, `extractThumbprintHintFromFilename`), and `cert/rehash.ts` (pure-TypeScript c_rehash, used by both the host's Linux store and the workspace installer). It also owns the platform trust-store layer (`platform/*`) and the generator backends (`backends/*`). Both extensions import it directly — there are no per-extension re-export shims; if you move something into shared, update the call sites rather than leaving a forwarding module behind. The classifier is side-effect-free — callers in vscode contexts opt into a localized log line by passing `onSkipped` / `onMultipleCandidates` callbacks. Both extensions independently localize via their own `vscode.l10n.t` bundles. - **Devcontainer feature** (`src/devcontainer-feature/`) — `install.sh` writes feature options and `SSL_CERT_DIR` into **three** sinks, because no single one reaches every kind of shell: (1) `/etc/profile.d/devcontainer-dev-certs.sh` (sourced by **login** shells; reached by VS Code's `userEnvProbe`, integrated terminals, and anything else that goes through `/etc/profile`), (2) `/etc/environment` (read by `pam_env` on PAM-based logins — sshd, console), and (3) the system-wide interactive bashrc — `/etc/bash.bashrc` on Debian/Ubuntu, else `/etc/bashrc` on the RPM/SUSE family — which sources sink (1) for **interactive non-login** shells. Sink (3) exists because a plain `docker exec -it bash` (the most common way to poke at a running container, as root or the remote user) starts an interactive non-login shell that reads *neither* profile.d *nor* `/etc/environment` — without it, exec'ing a bash shell saw no `SSL_CERT_DIR` at all. The bridge is marker-guarded (idempotent across feature reinstalls) and sources profile.d rather than duplicating the exports, so `$HOME` still expands per-user. `docker exec` (how VS Code attaches in the typical devcontainer flow) does NOT go through PAM, so `/etc/environment` alone also leaves `DEVCONTAINER_DEV_CERTS_SYNC_FROM_CONTAINER` and friends invisible to the extension host. `SSL_CERT_DIR` lives in profile.d with `$HOME` left unexpanded (per-user expansion at login) and in `/etc/environment` with a resolved `_REMOTE_USER_HOME` (pam_env doesn't expand `$HOME`). The manifest can't carry `SSL_CERT_DIR` because `${containerEnv:HOME}` doesn't resolve inside `containerEnv` and `remoteEnv` isn't allowed in features under strict-schema validation. `install.sh` creates `.dotnet/corefx/cryptography/x509stores/my/` and `.aspnet/dev-certs/trust/` directories, requests both extensions via `customizations.vscode.extensions`. `install.sh` also pre-creates any directories named in `extraCertDestinations` with `vscode` ownership so the remote extension can write without privileged escalation. @@ -33,11 +33,11 @@ These decisions were made deliberately. Do not change them without discussion. - **No `update-ca-certificates`.** OpenSSL trust is handled via `SSL_CERT_DIR` pointing to a directory with c_rehash hash symlinks. No system CA bundle modification. -- **No openssl binary dependency in the container.** The workspace extension implements c_rehash in pure TypeScript (`src/vscode-workspace-extension/src/util/rehash.ts`) — ASN.1 DER parsing + SHA-1 subject hash computation. +- **No openssl binary dependency — on the host OR in the container.** c_rehash is implemented in pure TypeScript in `src/shared/src/cert/rehash.ts` (ASN.1 DER parsing + canonical-name construction + SHA-1 subject hash), and BOTH sides use it: the workspace extension's `certInstaller` and the host's `LinuxCertificateStore.trustViaOpenSsl`. The host previously shelled out to `openssl x509 -hash`, which meant OpenSSL trust silently no-opped on any host without the binary — the host is a developer machine we don't control, so it must not be a runtime dependency. Note the hash is NOT SHA-1 over the raw subject DER: OpenSSL hashes `X509_NAME_canon` output — values **transcoded to UTF-8** (T61String from Latin-1, BMPString from UTF-16BE, UniversalString from UTF-32BE) and only then re-tagged UTF8String, ASCII-lowercased, and whitespace-folded, with RDN `SET OF` encodings concatenated *without* the Name's outer `SEQUENCE`. Two details the OpenSSL source reads as if it does otherwise, both settled against `openssl x509 -hash` on real certs: the transcode genuinely happens (a T61String holding UTF-8 bytes of `日本語` hashes to `02c4fa54`, not the `e2c402e4` you get from re-tagging those bytes), and folding covers every ASCII whitespace byte rather than just `0x20` (`"a\tb"`, `"a\nb"`, `"a b"` and `"A B"` all hash alike). Fixtures for each live in `tests/rehash.test.ts`; add to them rather than re-deriving from the source. Getting that wrong produces a `{hash}.N` nothing ever opens, which disables `SSL_CERT_DIR` trust while looking healthy on disk. `tests/rehash.test.ts` pins the values against `openssl x509 -hash`, and `tests/linuxStore.integration.test.ts` proves the result with `openssl verify -CApath`. - **No docker exec/cp.** Certificate material is transferred via VS Code's cross-host command routing, making the solution remote-transport-agnostic. Do not introduce Docker-specific commands. -- **Honor `DOTNET_DEV_CERTS_OPENSSL_CERTIFICATE_DIRECTORY`.** Both the UI extension's Linux store (`src/vscode-ui-extension/src/platform/linuxStore.ts`) and the workspace extension (`util/paths.ts`) respect this override, matching the official .NET `CertificateManager` behavior. +- **Honor `DOTNET_DEV_CERTS_OPENSSL_CERTIFICATE_DIRECTORY`.** Both the UI extension's Linux store (`src/shared/src/platform/linuxStore.ts`) and the workspace extension (`util/paths.ts`) respect this override, matching the official .NET `CertificateManager` behavior. - **`install.sh` sets `DOTNET_GENERATE_ASPNET_CERTIFICATE=false` only when the host is the dev cert source.** The race: on first `dotnet run` / `dotnet new webapi` / `dotnet build` of an HTTPS-enabled project, dotnet's implicit `CertificateManager` flow writes a self-signed cert into `~/.dotnet/corefx/cryptography/x509stores/my/`. When the workspace extension is concurrently writing OUR host-generated cert there, whichever write lands last wins on disk, but the OS trust + .NET Root-store state may have been driven by the other side — yielding a half-trusted, half-orphaned cert combo (the "partially valid certificate on first run" symptom). Setting the env var to false suppresses dotnet's IMPLICIT path only; explicit `dotnet dev-certs https` commands still work. Gating logic: suppress only when `generateDotNetCert: true` AND `syncContainerCert: false` (the default — host is the source). When `syncContainerCert: true`, the container is the source and the container-side "generate" step might literally BE dotnet's implicit auto-gen (a `dotnet run` somewhere bootstrapping the cert we then push to the host); suppressing it would break the source. When neither managed flow is on, there's nothing to race with, so leave dotnet alone. The variable is written to both `/etc/profile.d/devcontainer-dev-certs.sh` and `/etc/environment` (gated identically in both sinks) so it's visible regardless of session type. Do NOT make this unconditional — `syncContainerCert` flows in particular can rely on dotnet's auto-gen as their source. @@ -45,9 +45,31 @@ These decisions were made deliberately. Do not change them without discussion. - **`SSL_CERT_DIR` must include system CA paths.** Setting it overrides the system default entirely. The devcontainer feature includes all common distro paths (`/etc/ssl/certs`, `/usr/lib/ssl/certs`, `/etc/pki/tls/certs`, `/var/lib/ca-certificates/openssl`) and exposes `sslCertDirs` as an option for user override. **Non-existent dirs are pruned at install time, gated on the `pruneMissingCertDirs` option (default `true`):** the default list spans several distros and only a subset exists on any given image, and while OpenSSL ignores a missing dir, some consumers (Rust's `openssl-probe` / `rustls-native-certs`, which `read_dir` each entry) error on one. **Pruning is a dedicated boolean toggle, NOT inferred from whether `sslCertDirs` was overridden.** The devcontainer CLI exports a feature option's env var set to the declared default even when the user didn't specify it, so `install.sh` genuinely cannot distinguish "omitted" from "explicitly set to the default value" — any inference (an earlier `[ -z "$SSLCERTDIRS" ]` check, then a compare-to-`DEFAULT_SSL_CERT_DIRS` heuristic) is guesswork and was either a silent no-op or wrong for a user who sets the default verbatim. So pruning applies to whatever `sslCertDirs` resolves to (default *or* explicit) when `pruneMissingCertDirs` is true; set it false to use the list verbatim (e.g. a dir created after install but before it's needed). **`SSL_CERT_DIR` is owned exclusively by the feature's `install.sh`; the VS Code extensions do not configure it** — the workspace extension's old `ensureSslCertDir` + `devcontainer-dev-certs.sslCertDirs`/`ensureSslCertDir` settings were a perpetual no-op in devcontainers (install.sh got there first) and confusing dead weight, so they and `util/sslCertDir.ts` were removed. The UI extension's separate `ensureTerminalSslCertDir` (host-side local terminals) is unrelated and stays. Because pruning can empty the system-dir list, all sinks compose `SSL_CERT_DIR` empty-safe (trust dir alone, no trailing colon) — the same dangling-empty-element artifact the pruning avoids. `test/install-sh.test.mjs` exercises this hermetically (runs `install.sh` under a temp `DEVCERTS_SYSROOT` and asserts the produced profile.d / `/etc/environment` / bashrc contents, including the toggle on/off). -- **The UI extension has no user-facing commands.** It exposes only the internal `getCertMaterial` command. Certificate generation and trust happen automatically when the workspace extension requests material. +- **`runProcess` caps captured output at 32 MiB and reports truncation.** Node's `execFile` default is 1 MiB, which `security find-certificate -a` on macOS can exceed: it dumps every certificate in the login keychain, roughly 2 KB per entry for the attribute form and 1.5 KB for the PEM form, so ~500-700 certs. Uncommon, but MDM-pushed user certs and years of accumulated dev certs get there. The failure was silent and self-feeding: Node reports the overflow with a **string** `error.code` (`ERR_CHILD_PROCESS_STDIO_MAXBUFFER`), so `runProcess`'s `typeof error.code === "number" ? … : 1` folded it into `exitCode: 1` with an **empty stderr** — indistinguishable from a real failure. `isCertInKeychain` read that as "not in the keychain", force-skipped the on-disk PFX as an orphaned cache file, emptied `findExistingDevCert`, and sent `CertManager.trust()` down the `generate()` branch: a new cert plus an `add-trusted-cert` password prompt, on every request — and each new cert landed in the same keychain, so the next call truncated sooner. Hence two things: the cap is raised, and `ProcessResult.truncated` exists so callers can tell "no" from "couldn't tell". **`isCertInKeychain` deliberately fails OPEN on truncation** (assumes the cert IS present), inverting the usual instinct because here the closed direction is the destructive one; the open direction costs at most one redundant re-trust, since `checkStatus` establishes trust separately via `security verify-cert`. Any new caller that reads `exitCode !== 0` as a decision MUST check `truncated` first. -- **Container-to-host reverse-sync is off by default per-container.** The `syncContainerCert` feature option defaults to `false` and is the only opt-in toggle. Host-side gating reuses the existing `devcontainerDevCerts.generateDotNetCert` + `devcontainer-dev-certs.autoProvision` settings — there is no separate "accept container certs" host setting (a user disabling managed dev certs via those existing settings implicitly disables container-pushed acceptance too). The host independently re-validates anything pushed via `acceptContainerDevCert`; do not skip the `isValidDevCert` + `validateLocalSans` checks even if the workspace asserts the cert is valid. +- **Follow-up (needs a macOS host to verify): narrow the keychain queries.** Both `security find-certificate -a` calls enumerate the entire login keychain and then filter to `CN=localhost` in TypeScript. Pushing that filter to the CLI — `-a -Z -c localhost` — would drop the output to single digits and make the cap unreachable rather than merely distant. Not done yet because `security`'s `-c` matching semantics (exact vs. substring, case sensitivity, and its non-zero exit when nothing matches) can't be verified from a Linux dev box, and a false negative lands in the destructive direction described above. The cap and the `truncated` flag stay regardless — they cover the other call sites. + +- **Follow-up: evaluate `@azure/core-process` in place of direct `execFile`.** Real package (MIT, Azure SDK, "Secure, cross-platform process launching for Node.js") exposing `execFile`/`spawn`/`resolveExecutable` with `shell?: never`, a `maxBuffer` option, and PATH resolution — the surface `platform/processUtil.ts` hand-rolls. It also does something we do not: refuses `.cmd`/`.bat` on Windows by default (opt-in via `allowWindowsBatchFiles`), the batch-argument-injection class. Before adopting, confirm (a) that its resolver skips **relative** PATH entries, which is a documented property of our `resolveSafeExecPath` and the reason the cwd-first `CreateProcess` hijack is closed, (b) that overflow remains distinguishable from failure so `ProcessResult.truncated` survives, and (c) the bundle-size cost — the UI extension esbuilds its runtime deps into the VSIX. Note the caution: as of this writing the package has exactly one published version, `1.0.0` from 2026-08-13, which is very new for a security-sensitive dependency. + +- **The UI extension's user-facing commands are recovery affordances, never provisioning.** `contributes.commands` holds two: `devcontainer-dev-certs.trustInBrowsers` ("Dev Certs: Trust Certificate in Browsers"), a Linux-only retry for the Firefox / Chromium NSS import when the automatic attempt inside `trustCertificate` didn't complete, and `devcontainer-dev-certs.resetContainerCertConsent` ("Dev Certs: Reset Container Certificate Consent"), which clears the recorded container-cert decision. Everything else `activate()` registers is a cross-host IPC entry point the workspace extension calls — `getCertMaterial`, `getAllCertMaterial`, `getAllCertMaterialV3`, `acceptContainerDevCert` (listed in full in the Architecture section above) — and none of them appear in the palette. Certificate generation and trust happen automatically in response to a workspace request; there is deliberately no "generate a dev cert now" command, because provisioning is gated on consent plus host settings and a palette entry would be a second, unreviewed way in. + + `trustInBrowsers` resolves its target through `certManager.check()`, which reads the host's own platform store, so it can only re-import the **host-generated** cert. A cert accepted via `syncContainerCert` is public-cert-only and never written to `my/` (see the reverse-sync decision below), so `check()` cannot see it and the command reports "No development certificate found" — a consequence of that security boundary rather than an oversight, and documented for users in the README's command table. On Windows and macOS the command is registered but no-ops with an informational message, since browser trust follows the OS store there; it has no `when` clause, so it is still visible in the palette on those platforms. + +- **Container-cert consent is a tri-state, host-wide, and reversible.** `containerCertProvisionConsented` holds `"granted"` / `"denied"` / absent, read through `normalizeContainerCertConsent` (which maps the historical `true` to `"granted"` so existing consenters are never re-prompted). It replaced a boolean that could only ever record *yes*: an accept persisted forever while a decline persisted nothing, so the prompt returned on every container activation and the only durable state a security prompt could reach was the permissive one — a one-way ratchet whose only escape was editing `devcontainer.json` and rebuilding, or disabling host-side generation too. + + Three modal outcomes, and the distinction between the last two is the point: **Trust** records `granted` (after the trust step succeeds, so a failed `add-trusted-cert` leaves consent unpersisted and the next push re-prompts); **Never** records `denied` immediately, since there is no trust step that could fail; **Cancel / Escape** declines this push and records NOTHING, so a stray keystroke can't disable the feature permanently. A standing `denied` short-circuits before the prompt — that is what makes declining actually stop the asking. + + Consent is host-wide rather than per-thumbprint **because container dev certs rotate on rebuild**. Unless a container bakes the cert into its image or mounts a volume for `~/.dotnet/corefx/cryptography/x509stores/my/`, `dotnet dev-certs` mints a fresh one each rebuild, so a per-certificate memory would prompt on every rebuild — the shape of consent people learn to click through without reading. Do not "improve" this to per-thumbprint without changing that assumption first. + + `resetContainerCertConsent` exists so **Never** isn't itself a one-way ratchet — otherwise the fix would reintroduce the same defect pointing the other way. It clears the key (either direction) and deliberately does NOT untrust certificates already in the host store; there is no untrust path today (see the accumulation note below). + +- **Rebuild-rotation is the worst case, and it is the DEFAULT.** Most dotnet devcontainer base images mint a fresh dev cert on the container's first HTTPS build, and nothing lets this extension verify that a `devcontainer.json` setting `syncContainerCert: true` has actually persisted `~/.dotnet/corefx/cryptography/x509stores/my/` (via a baked-in cert or a volume). Persisting it is the sensible configuration, but it is unenforceable — so every design here has to assume a NEW certificate arrives on every rebuild, forever, and that the host's trust surfaces accumulate one entry per rebuild with nothing pruning them. + + This already broke once. Every dev cert is `CN=localhost`, so they all share one OpenSSL subject hash (`ce275665`) and consume `{hash}.{n}` slots by cert COUNT rather than by genuine collision. `ensureHashSymlink` capped at ten and then returned silently, so the eleventh rotation produced no symlink for the live cert while ten dead ones kept theirs: `openssl verify -CApath` passed for the oldest cert and failed for the current one. The cap is now 256, with a warning past ten and a **thrown error** at the bound — returning quietly would let an install report success for a certificate OpenSSL cannot find, which is the same failure merely delayed, and under rebuild-rotation the bound is reachable. `tests/linuxStore.integration.test.ts` drives twelve rotations through real `openssl verify`. Relatedly, "installed" and "trusted" both now require a *resolvable* hash symlink (`hasHashSymlink`), not just the PEM on disk: an install or host trusted before the hash was computed canonically has its link under the wrong name, and a files-only check would report health, skip the repair, and strand precisely the users the fix exists for. A PEM whose subject cannot be hashed is exempt — `ensureHashSymlink` is a no-op for it, so demanding a link would loop forever without converging. Note the corollary for any future pruning: `by_dir` stops at the first missing slot, so entries must be re-densified with `rehashDirectory`, never unlinked in place. + +- **Open: nothing prunes superseded certs, on any platform.** Under rebuild-rotation the host accrues one trusted `CN=localhost` leaf per rebuild in `CurrentUser\Root`, the login keychain, the .NET root store + OpenSSL trust dir, and (since NSS nicknames became per-thumbprint) the browser NSS databases. The 365-day validity window bounds the *security* exposure — accumulated certs expire and stop validating — but not the clutter, and not the operational effects: keychain enumeration output grows (see the `runProcess` output-cap decision), and trust surfaces fill with entries whose private keys live in containers that no longer exist. Closing this needs a targeted per-platform `untrustCertificate` plus a supersede policy, and the policy is the hard part: "untrust the previous container cert on accept" ping-pongs when two containers are open at once, which is precisely why `trustViaOpenSsl` was made additive in the first place. Whatever lands, it needs a real entry point — do not reintroduce unreferenced removal surface. + +- **Container-to-host reverse-sync is off by default per-container.** The `syncContainerCert` feature option defaults to `false` and is the only opt-in toggle. Host-side gating reuses the existing `devcontainerDevCerts.generateDotNetCert` + `devcontainer-dev-certs.autoProvision` settings — there is no separate "accept container certs" host setting (a user disabling managed dev certs via those existing settings implicitly disables container-pushed acceptance too). The host independently re-validates anything pushed via `acceptContainerDevCert`; do not skip the `isValidDevCert` + `validateLeafTrustShape` + `validateLocalSans` checks even if the workspace asserts the cert is valid. - **`syncContainerCert: true` overrides the per-container `generateDotNetCert` feature option.** When the container is pushing its own dev cert to the host, asking the host to ALSO generate and send back a different dotnet dev cert would leave the container with two trusted dev certs in `my/` — confusing for .NET / Aspire's selection logic. The workspace extension's pull-from-host request therefore forces `includeDotNetDev=false` whenever `DEVCONTAINER_DEV_CERTS_SYNC_FROM_CONTAINER=true`, regardless of `DEVCONTAINER_DEV_CERTS_GENERATE_DOTNET`. User certs (`syncUserCertificates`) are unaffected. The cleanup path (`detectStaleAndPromptCleanup` / `cleanupCommand`) augments the bundle's managed-thumbprint set with the container's own dev cert thumbprint when `syncContainerCert` is on, so the sweep doesn't classify the source cert as stale. Do NOT add a separate per-container `generateDotNetCert: false` requirement — making the user set both for the same intent is a footgun. @@ -55,7 +77,11 @@ These decisions were made deliberately. Do not change them without discussion. - **Trust is platform-uniform across the two flows.** `trustExternalCertificate` invokes the SAME `store.trustCertificate(cert)` method the host-generation flow's final step uses. That means every trust surface the platform store wires into `trustCertificate` runs for both flows — on Linux specifically that includes NSS browser DBs (the `linuxNssTrustReporter` is set once on `CertManager` construction and covers both paths). "Trusted on the host" is defined by `store.trustCertificate`; if you ever add a new trust surface (say, a new browser store handler), wire it into `trustCertificate` on the platform store, not into the accept handler — that keeps the two flows in lockstep automatically. `tests/manager.test.ts` pins the contract: `trustExternalCertificate` MUST call `store.trustCertificate` and MUST NOT call `store.saveCertificate` / `store.findExistingDevCert`. -- **SAN-local restriction is the default on container-pushed certs.** When accepting a container-pushed cert, `validateLocalSans` rejects SAN entries outside well-known local scopes (loopback, RFC1918 private IP, localhost / docker host names, `*.dev.localhost`, `*.dev.internal`). `devcontainerDevCerts.allowNonLocalContainerCertSans` is the explicit opt-out, surfaced in the consent modal so the user can see exactly which non-local entries they're agreeing to trust. +- **NSS trust flags are per browser family, and the split is deliberate.** `trustFlagsFor` in `nssTrust.ts` sends `P,,` to Chromium-family databases and `C,,` to Firefox profiles. Do not collapse these to one value. The dev cert is a self-signed **end entity** (`generateCertificate` emits `cA=FALSE`), so the correct encoding is `P` — `CERTDB_TRUSTED`, "trusted peer", the bit consulted when the cert *is* what's being validated; `C` is `CERTDB_TRUSTED_CA`, consulted only in an *issuer* position our cert never occupies. Firefox is an empirical exception: it ignores `P` for server certs, so `C` is what actually produces trust there. This mirrors `dotnet dev-certs https --trust` (`UnixCertificateManager.TryAddCertificateToNssDb`: `usage = nssDb.IsFirefox ? "C" : "P"`), validated by Microsoft against real browsers. We previously sent a blanket `CT,,` everywhere, which is right for Firefox and wrong for Chromium — `certutil -V -u V` rejects such an entry with "Issuer certificate is invalid", pinned by an integration test. That asymmetry is also why dotnet verifies Chromium with `-V -u V` but Firefox with only `-L`: `-V` cannot pass under `C`. Migration is handled by the existing delete-then-add, since `certutil -A` will not rewrite an existing nickname's trust string. + +- **A container-pushed cert must be a server-auth LEAF before its SANs mean anything.** `validateLeafTrustShape` gates `validateLocalSans` in `acceptContainerDevCert`, and the ordering is the whole point. `trustCertificate` puts the cert in `CurrentUser\Root` (Windows), the login keychain's SSL trust settings (macOS), and the .NET Root store + OpenSSL CApath + browser NSS databases (Linux) — anchor positions, and in the Firefox NSS case an explicit `C` "trusted CA" flag. Whether it can actually *act* as a CA from there comes down to basicConstraints. A SAN check cannot substitute: a CA's own SANs place no limit on what it may issue, so a CA carrying `localhost` SANs would pass a SAN-only gate and then sign a leaf for any name at all. So: basicConstraints must be **present** with `cA=FALSE` (absent leaves the question to each validator's historical quirks), and EKU must be **present**, include `id-kp-serverAuth`, and not include `anyExtendedKeyUsage` (absent EKU reads as "any purpose", and Windows `-addstore Root` applies no policy constraint of its own). Extra concrete usages like `clientAuth` are tolerated. Every genuine dev cert — ours via `generateCertificate`, .NET's via `CertificateManager` — carries both extensions in exactly this shape, which `tests/containerCertAccept.test.ts` pins by driving the real generator through the accept path. Do NOT relax these to "check only if present". + +- **SAN-local restriction is the default on container-pushed certs, and structural SAN failures are not overridable.** `validateLocalSans` rejects dNSName / iPAddress entries outside well-known local scopes (loopback, RFC1918 private IP, localhost / docker host names, `*.dev.localhost`, `*.dev.internal`). `devcontainerDevCerts.allowNonLocalContainerCertSans` is the explicit opt-out for that — and *only* that: it overrides `reason: "non-local"` and nothing else. The scanner (`scanSanEntries`) separately rejects a SAN set that is absent, undecodable, empty, or carrying a GeneralName type other than dNSName / iPAddress, all of which surface as `malformed-sans` and are never overridable. The reasoning: the override lets a user say "yes, I really do mean to trust this cert for that name", which is meaningless for a cert whose names we could not read. Reporting "SANs are local-only" after silently dropping the entries we didn't recognize was vouching for a cert we had only partially inspected. Note also that `@peculiar/x509` parses extensions lazily and *throws* from `getExtension` on bad DER — that used to escape into the accept handler's blanket `try/catch` and land as a generic parse failure, making fail-closed an accident of the call site. `scanSanEntries` now catches it and names the reason, so adding a `try/catch` inside the scanner can't silently invert the behavior. ## Build System diff --git a/README.md b/README.md index f27862e..4b1a69e 100644 --- a/README.md +++ b/README.md @@ -203,11 +203,12 @@ Set in your workspace settings or user settings inside the Dev Container / remot ### Commands -All three are available from the Command Palette (`F1`). The host command runs on your local machine; the remote commands run inside the Dev Container. +All four are available from the Command Palette (`F1`). The host commands run on your local machine; the remote commands run inside the Dev Container. | Command | Runs on | Command ID | Description | |---------|---------|------------|-------------| | **Dev Certs: Trust Certificate in Browsers** | Host | `devcontainer-dev-certs.trustInBrowsers` | Retry the Firefox / Chromium NSS import for the **host-generated** dev cert after the automatic attempt couldn't complete. Linux hosts only — on Windows and macOS browser trust follows the OS store automatically. Can't target a cert accepted via `syncContainerCert`. See "[Linux hosts: browser trust](#linux-hosts-browser-trust)". | +| **Dev Certs: Reset Container Certificate Consent** | Host | `devcontainer-dev-certs.resetContainerCertConsent` | Clear the recorded answer to the [`syncContainerCert`](#syncing-a-certificate-from-the-container-to-the-host) consent prompt, so the next container push asks again. Use it to undo either a **Never** (start accepting again) or a previous **Trust** (stop accepting silently). Does not untrust certificates already added to the host store. | | **Dev Certs: Inject Certificate into Remote** | Remote | `devcontainer-dev-certs.injectCert` | Re-run the certificate injection flow manually. Normally automatic on activation; needed when `autoInject` is `false`, or to retry after a failure. | | **Dev Certs: Clean Up Other Dev Certificates in Dev Container** | Remote | `devcontainer-dev-certs.cleanupStaleDevCerts` | Remove dev cert artifacts in the container's .NET stores that aren't the extension-managed one, then rehash the OpenSSL trust directory. Refuses to run when no managed dev cert is known, so it can't delete every dev cert on disk. | @@ -375,7 +376,7 @@ With `syncContainerCert` enabled: - If a usable cert is found, the workspace extension pushes **just the public certificate** (PEM-encoded) to the host via a new IPC command. The private key never leaves the container — Kestrel keeps using its own copy of the key inside the container, and the host's job in this flow is to act purely as a trust anchor (so forwarded HTTPS ports and browser-side validation work on the host) rather than as a cert distribution point. If no usable cert is found, the push is a no-op — there's no fallback to host generation. - **`syncContainerCert: true` overrides the `generateDotNetCert` feature option for this container.** You don't need to also set `generateDotNetCert: false` to opt out of host generation — when the container is pushing its own cert to the host, the workspace extension drops the dotnet dev cert from its pull-from-host request automatically. (Otherwise the container would end up with both its own cert AND a different host-generated cert in its .NET store.) User-managed certificates configured via `userCertificates` are unaffected — those still flow normally. - The host extension independently re-validates the cert (same `isValidDevCert` rules; matches dev-cert OID, version, validity window). It then restricts SAN entries to local-only scopes by default — `localhost`, `*.localhost`, `*.dev.localhost`, `*.dev.internal`, `host.docker.internal`, `host.containers.internal`, IPv4 loopback / RFC1918 / link-local, IPv6 loopback / unique-local / link-local. A cert with SAN entries outside that set is rejected. -- If validation passes, the host shows a modal consent prompt before adding the cert to the platform trust store. That consent is recorded once per host, not per certificate — accepting it covers subsequent container pushes too. Any OS-level authorization the platform requires still applies on top: on macOS the keychain may prompt when trust settings change, while on Windows and Linux the import is non-interactive. The cert lands in the OS trust surfaces only — the .NET root store on Linux, the login keychain's policy settings on macOS, CurrentUser/Root on Windows — never in `CurrentUser/My`, the keychain's identity slot, or the .NET store's `my/` directory. The host has nothing keyed by this thumbprint that contains a private key. +- If validation passes, the host shows a modal consent prompt before adding the cert to the platform trust store. It has three outcomes: **Trust** accepts and is recorded once per host, not per certificate, so subsequent container pushes are covered too; **Never Trust Container Certificates** records a refusal for this host and stops the prompting entirely; **Cancel** (or Escape) skips this one certificate and asks again next time, recording nothing — so a stray keystroke can't switch the feature off for good. Either recorded answer can be undone with **Dev Certs: Reset Container Certificate Consent**. The host-wide granularity is deliberate: unless a container bakes its dev cert into the image or mounts a volume for `~/.dotnet/corefx/cryptography/x509stores/my/`, it mints a fresh certificate on every rebuild, so a per-certificate prompt would fire on every rebuild. Any OS-level authorization the platform requires still applies on top: on macOS the keychain may prompt when trust settings change, while on Windows and Linux the import is non-interactive. The cert lands in the OS trust surfaces only — the .NET root store on Linux, the login keychain's policy settings on macOS, CurrentUser/Root on Windows — never in `CurrentUser/My`, the keychain's identity slot, or the .NET store's `my/` directory. The host has nothing keyed by this thumbprint that contains a private key. To allow SAN entries that aren't local (rare; security-sensitive — the cert will be trusted by your host browser for the listed names), opt in explicitly: @@ -520,11 +521,11 @@ src/ primitives) and dotnet (dev-certs pass-through) paths.ts .NET store and OpenSSL trust directory paths certName.ts userCertificates[].name pattern and guard + cert/rehash.ts Pure TypeScript c_rehash (OpenSSL canonical subject hash) logger.ts Pluggable logging (loggerVscode.ts binds the output channel) vscode-ui-extension/ VS Code host extension (extensionKind: ui) src/ - cert/, platform/ Re-export shims over the canonical copies in shared/ certProvider.ts Serves cert material to the workspace extension containerCertAccept.ts Validates and trusts container-pushed certs @@ -535,7 +536,6 @@ src/ containerCertPush.ts Reverse sync: scans for and pushes the container's cert defaultKestrelDebugProvider.ts Injects the Kestrel default-cert env vars into resolved coreclr debug configurations - util/rehash.ts Pure TypeScript c_rehash (OpenSSL subject hash computation) util/destinations.ts extraCertDestinations parsing util/upmap.ts V2 -> V3 cert material wire-contract upmap diff --git a/src/shared/src/backends/select.ts b/src/shared/src/backends/select.ts index 3c90f13..7fe09d0 100644 --- a/src/shared/src/backends/select.ts +++ b/src/shared/src/backends/select.ts @@ -1,6 +1,6 @@ import { DotnetBackend } from "./dotnet"; import { NativeBackend } from "./native"; -import type { Backend, BackendKind, BackendMode } from "./types"; +import type { Backend, BackendMode } from "./types"; /** * Resolve a `hostCertGenerator` choice (possibly `auto`) into a concrete @@ -30,19 +30,3 @@ async function autoSelect(): Promise { } return new NativeBackend(); } - -/** - * Report which backend `auto` would pick on this host without actually - * constructing it. Useful for status surfaces in the VS Code host extension. - * - * Callers that have already probed dotnet can pass the result via - * `dotnetAvailable` to avoid a second `dotnet --version` spawn. - */ -export async function describeAutoBackend( - dotnetAvailable?: boolean -): Promise { - if (process.platform !== "darwin") return "native"; - const available = - dotnetAvailable ?? (await new DotnetBackend().isAvailable()); - return available ? "dotnet" : "native"; -} diff --git a/src/shared/src/cert/generator.ts b/src/shared/src/cert/generator.ts index a10bc17..0461287 100644 --- a/src/shared/src/cert/generator.ts +++ b/src/shared/src/cert/generator.ts @@ -210,12 +210,29 @@ function defaultEcHash(curve: string): string { } } -function generateSerialNumber(): string { +/** + * 16-byte positive serial number, hex-encoded. + * + * Exported for testing: the guarantee below is probabilistic (a bad leading + * byte turns up about once in 128), so pinning it needs thousands of samples, + * and routing those through `generateCertificate` would mean thousands of RSA + * keygens. The production caller is `generateCertificate`, just below. + */ +export function generateSerialNumber(): string { const maxAttempts = 5; for (let attempt = 0; attempt < maxAttempts; attempt++) { const bytes = randomBytes(16); bytes[0] &= 0x7f; // ensure non-negative - if (bytes.some((value) => value !== 0)) { + // Reject a zero leading byte, not just an all-zero serial. Clearing the + // high bit keeps the DER INTEGER positive, but a resulting 0x00 leading + // byte is retained on the wire as sign padding (`02 10 00 b5 ...`) — + // correct, yet every textual readback drops it, so the serial then looks + // like a 15-byte value starting at or above 0x80. Requiring 0x01..0x7f + // yields a serial that is positive, non-zero, and minimally encoded, with + // no padding byte for downstream code to reason about. Rejection (rather + // than masking a 1 in) keeps the remaining bits uniform; five attempts + // leaves a (1/128)^5 failure chance. + if (bytes[0] !== 0) { return bytes.toString("hex"); } } diff --git a/src/shared/src/cert/manager.ts b/src/shared/src/cert/manager.ts index 94c882f..c4879b8 100644 --- a/src/shared/src/cert/manager.ts +++ b/src/shared/src/cert/manager.ts @@ -47,16 +47,16 @@ export class CertManager { /** * Generate a new dev cert and save it to the platform store. - * If force is true, removes existing certs first. + * + * Additive by design: a pre-existing dev cert in the store is left alone. + * `findExistingDevCert` / `selectBestDevCert` pick the winner by version + * then expiry, so a superseded cert stops being selected without anyone + * having to delete it — and nothing here can revoke a cert some other + * flow (or the user) deliberately trusted. */ - async generate(force: boolean = false): Promise { + async generate(): Promise { const store = await this.getStore(); - if (force) { - log("Removing existing certificates..."); - await store.removeCertificates(); - } - log("Generating new dev certificate..."); const now = new Date(); const expiry = new Date( @@ -227,16 +227,6 @@ export class CertManager { this.currentCert = null; } - /** - * Remove all dev certificates from the platform store. - */ - async clean(): Promise { - const store = await this.getStore(); - await store.removeCertificates(); - this.currentCert = null; - log("All dev certificates removed."); - } - /** * Ensure we have a loaded cert (from store or freshly generated). */ diff --git a/src/shared/src/cert/rehash.ts b/src/shared/src/cert/rehash.ts new file mode 100644 index 0000000..a140fce --- /dev/null +++ b/src/shared/src/cert/rehash.ts @@ -0,0 +1,604 @@ +import * as crypto from "crypto"; +import * as fs from "fs"; +import * as path from "path"; +import { log } from "../logger"; + +/** + * Pure TypeScript implementation of OpenSSL's c_rehash for certificate directories. + * + * OpenSSL's `X509_NAME_hash` — the value `by_dir` (i.e. `SSL_CERT_DIR` / + * `-CApath`) uses to find a certificate by subject — is NOT a hash of the + * subject's on-the-wire DER. It hashes the *canonical* encoding produced by + * `X509_NAME_canon`, which differs in two ways that both matter here: + * + * 1. Each attribute value is normalized: string types in `ASN1_MASK_CANON` + * are re-tagged as UTF8String, ASCII-lowercased, and have leading / + * trailing spaces trimmed with internal runs collapsed to one space. + * 2. The result is the bare concatenation of the DER `SET OF` encodings of + * the RDNs — the outer `SEQUENCE` header of the Name is NOT included. + * + * SHA-1 over that byte string, first 4 bytes read as a little-endian uint32, + * formatted as 8-character lowercase hex, is the `{hash}.N` filename OpenSSL + * looks for. Hashing the raw subject DER instead produces a name nothing ever + * looks up, which silently disables `SSL_CERT_DIR` trust. + */ + +/** + * ASN.1 string tags OpenSSL's `asn1_string_canon` normalizes (ASN1_MASK_CANON). + * Anything outside this set — NumericString included — is copied through with + * its original tag and bytes. + */ +const CANONICALIZED_STRING_TAGS = new Set([ + 0x0c, // UTF8String + 0x13, // PrintableString + 0x14, // T61String / TeletexString + 0x16, // IA5String + 0x1a, // VisibleString + 0x1c, // UniversalString + 0x1e, // BMPString +]); + +/** Tag OpenSSL re-labels every canonicalized string with. */ +const UTF8_STRING_TAG = 0x0c; + +/** + * Upper bound on `{hash}.{n}` slots probed for one subject hash. OpenSSL has + * no limit of its own; this exists only so a pathological directory can't spin + * forever. Set far above any plausible dev-cert count so exhausting it means + * something is genuinely wrong rather than merely busy. + */ +const MAX_HASH_SLOTS = 256; + +/** + * Slot index past which the trust directory is worth remarking on. Ten + * same-subject certs is already more than a healthy setup accumulates, and + * silence here is what let the old bound break trust unnoticed. + */ +const CROWDED_HASH_SLOTS = 10; + +/** + * Compute the OpenSSL subject hash from a PEM certificate string. + * Returns the 8-character hex hash string, or null if the cert cannot be parsed. + */ +export function computeSubjectHash(pemCert: string): string | null { + try { + // Parse the PEM to get DER bytes + const derBytes = pemToDer(pemCert); + if (!derBytes) return null; + + // Extract the subject field from the TBSCertificate + const subjectDer = extractSubjectDer(derBytes); + if (!subjectDer) return null; + + // Reduce it to OpenSSL's canonical form before hashing. + const canonical = canonicalizeName(subjectDer); + if (!canonical) return null; + + const hash = crypto.createHash("sha1").update(canonical).digest(); + + // Take first 4 bytes as little-endian uint32, format as 8-char hex + const value = hash.readUInt32LE(0); + return value.toString(16).padStart(8, "0"); + } catch { + return null; + } +} + +/** + * Create hash symlinks in a directory for all PEM/CRT files. + * Removes existing hash symlinks first, then creates new ones. + */ +export function rehashDirectory(directory: string): void { + const entries = fs.readdirSync(directory); + + // Remove existing hash symlinks + for (const entry of entries) { + const fullPath = path.join(directory, entry); + if (isHashSymlink(entry) && isSymlink(fullPath)) { + fs.unlinkSync(fullPath); + } + } + + // Create new hash symlinks + const certFiles = entries.filter((f) => /\.(pem|crt|cer)$/i.test(f)); + for (const certFile of certFiles) { + const fullPath = path.join(directory, certFile); + if (isSymlink(fullPath)) continue; // Skip symlinks themselves + ensureHashSymlink(directory, certFile, fs.readFileSync(fullPath, "utf-8")); + } +} + +/** + * Ensure an OpenSSL subject-hash symlink exists for `pemFileName` in + * `directory`. No-op when a valid slot already points at the same PEM — + * the caller can re-invoke this safely on every install without producing + * duplicate `{hash}.0`/`{hash}.1` pairs. Allocates the next free slot + * (`{hash}.0` … `{hash}.255`, see `MAX_HASH_SLOTS`) when an occupied slot + * points at a different, still-present target. + * + * Unlike `rehashDirectory`, this only touches the slot for our PEM — + * other PEMs' hash symlinks are left alone. + * + * Throws when no slot is reachable. Returning quietly would let an install + * report success for a certificate OpenSSL cannot find, which at the point of + * use is indistinguishable from it not being trusted at all. + */ +export function ensureHashSymlink( + directory: string, + pemFileName: string, + pemContent: string +): void { + const hash = computeSubjectHash(pemContent); + if (!hash) return; + // Every ASP.NET dev cert shares the subject `CN=localhost`, so every one of + // them collides on the SAME hash — slots are consumed by cert COUNT, not by + // genuine hash collisions. The old bound of 10 therefore ran out in ordinary + // use: a container that mints a fresh dev cert on each rebuild (the default + // for most dotnet devcontainer base images, and not something this extension + // can enforce otherwise) fills ten slots in ten rebuilds, after which this + // function fell out of the loop and returned silently. The host was then left + // trusting ten dead certs while the live one had no symlink at all and failed + // `openssl verify -CApath` — the feature inverted, with no error and no log. + // + // OpenSSL imposes no such limit: `by_dir` walks `{hash}.{n}` upward until a + // file is missing. The corollary is that slots must stay CONTIGUOUS from 0 — + // a gap makes everything past it unreachable — which is why pruning entries + // has to re-densify via `rehashDirectory` rather than unlink in place. + // + // Catch EEXIST so a concurrent rehash from another process doesn't crash the + // caller. + for (let i = 0; i < MAX_HASH_SLOTS; i++) { + const linkName = `${hash}.${i}`; + const linkPath = path.join(directory, linkName); + + // Use lstat (instead of existsSync) so dangling symlinks are detected + // as "occupied" and can be reclaimed. + let existing: fs.Stats | undefined; + try { + existing = fs.lstatSync(linkPath); + } catch { + // ENOENT — slot is free. + } + + if (existing) { + if (!existing.isSymbolicLink()) continue; // not ours; skip + let target: string; + try { + target = fs.readlinkSync(linkPath); + } catch { + continue; + } + if (target === pemFileName) return; // already correct + const targetPath = path.isAbsolute(target) + ? target + : path.join(directory, target); + if (fs.existsSync(targetPath)) continue; // live collision; next slot + // Dangling — reclaim the slot. Hash symlinks are written exclusively + // by this extension, so a dangling one is leftover state we own. + try { + fs.unlinkSync(linkPath); + } catch { + continue; + } + } + + try { + fs.symlinkSync(pemFileName, linkPath); + if (i >= CROWDED_HASH_SLOTS) { + log( + `OpenSSL trust dir ${directory} now holds ${i + 1} certificates sharing subject hash ${hash}. ` + + `Trust is still correct, but nothing prunes superseded certificates — see the accumulation ` + + `note in AGENTS.md.` + ); + } + return; + } catch (err: unknown) { + if ((err as NodeJS.ErrnoException).code === "EEXIST") continue; + throw err; + } + } + + // Never silently. A certificate with no reachable slot is one OpenSSL will + // not find, which at the point of use is indistinguishable from it not being + // trusted — so reporting a successful install would be a lie. Raising it here + // also surfaces the unbounded-accumulation problem loudly instead of letting + // the bound be reached and quietly ignored, which is exactly how the previous + // ten-slot limit went unnoticed. + throw new Error( + `Could not allocate an OpenSSL hash symlink for ${pemFileName} in ${directory}: ` + + `all ${MAX_HASH_SLOTS} slots for subject hash ${hash} are taken. This certificate would ` + + `not be found via SSL_CERT_DIR. Remove superseded certificates from that directory.` + ); +} + +/** + * Whether `pemFileName` already has a hash symlink OpenSSL would resolve to it. + * + * Exists so callers can treat "installed" as including a reachable link rather + * than just the files being on disk. An installation made before the subject + * hash was computed canonically has a symlink under the WRONG hash, so a check + * that only looks for the PEM and PFX sees a healthy install and skips the + * repair — leaving trust broken exactly for the users the fix is meant to + * reach. + */ +export function hasHashSymlink( + directory: string, + pemFileName: string, + pemContent: string +): boolean { + const hash = computeSubjectHash(pemContent); + if (!hash) return false; + for (let i = 0; i < MAX_HASH_SLOTS; i++) { + const linkPath = path.join(directory, `${hash}.${i}`); + + // A slot is "missing" only when nothing is there. A regular file sitting + // at `{hash}.N` is OCCUPIED, not a gap: `ensureHashSymlink` steps over it + // and puts our link in a later slot, and OpenSSL's `by_dir` likewise + // processes whatever it finds and keeps walking. Reading a non-symlink + // with readlink raises EINVAL, so treating any error as end-of-search + // would stop before reaching our link and report "not linked" for a + // certificate that is in fact reachable. + let entry: fs.Stats; + try { + entry = fs.lstatSync(linkPath); + } catch { + return false; // ENOENT — a real gap, and OpenSSL stops here too. + } + if (!entry.isSymbolicLink()) continue; + + let target: string; + try { + target = fs.readlinkSync(linkPath); + } catch { + continue; + } + if (target === pemFileName) return true; + } + return false; +} + +// --- Internal helpers --- + +const PEM_BEGIN = "-----BEGIN CERTIFICATE-----"; +const PEM_END = "-----END CERTIFICATE-----"; + +/** + * Extract the DER bytes of the first certificate in a PEM string. + * + * Deliberately `indexOf` + `slice` rather than one regex. The previous form, + * `/-----BEGIN CERTIFICATE-----\s*([\s\S]*?)\s*-----END CERTIFICATE-----/`, + * put two `\s*` quantifiers around a lazy `[\s\S]*?` — all three match + * whitespace, so the engine has an ambiguous split to backtrack over. On input + * that opens with the BEGIN marker and continues with many spaces but never + * reaches an END marker, matching degrades to quadratic (CodeQL + * `js/polynomial-redos`, flagged high). + * + * That input is reachable: `rehashDirectory` feeds this every `*.pem` file it + * finds in the OpenSSL trust directory, and nothing guarantees those files are + * well-formed. `indexOf` scans linearly and cannot backtrack, and the + * whitespace strip below uses `/\s/g` — a single character class with no + * quantifier — so it stays linear too. + * + * Behaviour is otherwise unchanged: the first BEGIN paired with the first + * following END, surrounding whitespace discarded, `null` when either marker + * is absent. + */ +function pemToDer(pem: string): Buffer | null { + const begin = pem.indexOf(PEM_BEGIN); + if (begin < 0) return null; + const bodyStart = begin + PEM_BEGIN.length; + + const end = pem.indexOf(PEM_END, bodyStart); + if (end < 0) return null; + + const base64 = pem.slice(bodyStart, end).replace(/\s/g, ""); + if (base64.length === 0) return null; + return Buffer.from(base64, "base64"); +} + +/** + * Extract the Subject field (as raw DER bytes) from an X.509 certificate. + * + * X.509 structure (simplified): + * Certificate ::= SEQUENCE { + * tbsCertificate TBSCertificate, + * ... + * } + * TBSCertificate ::= SEQUENCE { + * version [0] EXPLICIT INTEGER OPTIONAL, + * serialNumber INTEGER, + * signature AlgorithmIdentifier, + * issuer Name, + * validity Validity, + * subject Name, <-- this is what we want + * ... + * } + */ +function extractSubjectDer(certDer: Buffer): Buffer | null { + const offset = 0; + + // Certificate SEQUENCE + const cert = readTag(certDer, offset); + if (!cert || cert.tag !== 0x30) return null; + + // TBSCertificate SEQUENCE + const tbs = readTag(certDer, cert.contentOffset); + if (!tbs || tbs.tag !== 0x30) return null; + + let pos = tbs.contentOffset; + + // version [0] EXPLICIT - optional, skip if present + const maybeVersion = readTag(certDer, pos); + if (!maybeVersion) return null; + if (maybeVersion.tag === 0xa0) { + // version is present, skip it + pos = maybeVersion.contentOffset + maybeVersion.contentLength; + } + + // serialNumber INTEGER - skip + const serial = readTag(certDer, pos); + if (!serial) return null; + pos = serial.contentOffset + serial.contentLength; + + // signature AlgorithmIdentifier SEQUENCE - skip + const sigAlg = readTag(certDer, pos); + if (!sigAlg) return null; + pos = sigAlg.contentOffset + sigAlg.contentLength; + + // issuer Name SEQUENCE - skip + const issuer = readTag(certDer, pos); + if (!issuer) return null; + pos = issuer.contentOffset + issuer.contentLength; + + // validity Validity SEQUENCE - skip + const validity = readTag(certDer, pos); + if (!validity) return null; + pos = validity.contentOffset + validity.contentLength; + + // subject Name SEQUENCE - THIS IS WHAT WE WANT + const subject = readTag(certDer, pos); + if (!subject) return null; + + // Return the full TLV (tag + length + content) of the subject + return certDer.subarray(pos, subject.contentOffset + subject.contentLength); +} + +/** + * Reduce a DER-encoded X.509 `Name` to the byte string OpenSSL hashes in + * `X509_NAME_hash`: each RDN re-encoded as a DER `SET OF` over canonicalized + * attributes, concatenated, with NO outer `SEQUENCE` header. + */ +function canonicalizeName(nameDer: Buffer): Buffer | null { + const name = readTag(nameDer, 0); + if (!name || name.tag !== 0x30) return null; + + const nameEnd = name.contentOffset + name.contentLength; + if (nameEnd > nameDer.length) return null; + + const rdnEncodings: Buffer[] = []; + let pos = name.contentOffset; + while (pos < nameEnd) { + const rdn = readTag(nameDer, pos); + if (!rdn || rdn.tag !== 0x31) return null; + const rdnEnd = rdn.contentOffset + rdn.contentLength; + if (rdnEnd > nameEnd) return null; + + const attributes: Buffer[] = []; + let attrPos = rdn.contentOffset; + while (attrPos < rdnEnd) { + const attr = readTag(nameDer, attrPos); + if (!attr || attr.tag !== 0x30) return null; + const attrEnd = attr.contentOffset + attr.contentLength; + if (attrEnd > rdnEnd) return null; + + const type = readTag(nameDer, attr.contentOffset); + if (!type || type.tag !== 0x06) return null; + const typeEnd = type.contentOffset + type.contentLength; + if (typeEnd > attrEnd) return null; + + const value = readTag(nameDer, typeEnd); + if (!value) return null; + if (value.contentOffset + value.contentLength > attrEnd) return null; + + const canonValue = canonicalizeAttributeValue( + value.tag, + nameDer.subarray( + value.contentOffset, + value.contentOffset + value.contentLength + ) + ); + attributes.push( + derTlv( + 0x30, + Buffer.concat([nameDer.subarray(attr.contentOffset, typeEnd), canonValue]) + ) + ); + + attrPos = attrEnd; + } + + // DER requires SET OF members to be sorted by their encodings. + attributes.sort(compareDerSetMembers); + rdnEncodings.push(derTlv(0x31, Buffer.concat(attributes))); + + pos = rdnEnd; + } + + return Buffer.concat(rdnEncodings); +} + +/** + * OpenSSL's `asn1_string_canon`, verified against `openssl x509 -hash` rather + * than reconstructed from the source, because the source reads as if it folds + * bytes in place and it does not. + * + * Two things a byte-level reading gets wrong, both confirmed with real certs + * on OpenSSL 3.0.13: + * + * - **Non-UTF-8 string types are transcoded first.** A T61String holding the + * UTF-8 bytes of `日本語` hashes to `02c4fa54`; re-tagging those bytes as + * UTF8String and hashing gives `e2c402e4`. Only interpreting them as + * Latin-1 and re-encoding as UTF-8 reproduces OpenSSL. The same applies to + * BMPString (UTF-16BE) and UniversalString (UTF-32BE). + * - **Folding covers every ASCII whitespace byte, not just `0x20`.** With + * `CN` stored as `"a\tb"`, `"a\nb"`, `"a b"` and `"A B"`, OpenSSL + * returns `49cdc5e0` for all four — so runs of any ASCII whitespace collapse + * to one space, and ASCII letters lowercase. Leading whitespace is trimmed + * the same way: `"a"`, `" a"` and `"\ta"` all hash to `20b69a40`. + * + * Getting either wrong yields a plausible-looking `{hash}.N` link that OpenSSL + * never opens, which is indistinguishable from an untrusted certificate. + * + * Bytes outside ASCII pass through untouched, matching the `!ossl_isascii` + * branch — safe after transcoding, since every byte of a multi-byte UTF-8 + * sequence has the high bit set and so can't be mistaken for a letter. + */ +function canonicalizeAttributeValue(tag: number, content: Buffer): Buffer { + if (!CANONICALIZED_STRING_TAGS.has(tag)) return derTlv(tag, content); + + const utf8 = transcodeToUtf8(tag, content); + if (!utf8) return derTlv(tag, content); + + let start = 0; + let end = utf8.length; + while (start < end && isAsciiSpace(utf8[start])) start++; + while (end > start && isAsciiSpace(utf8[end - 1])) end--; + + const out: number[] = []; + for (let i = start; i < end; i++) { + const byte = utf8[i]; + if (isAsciiSpace(byte)) { + out.push(0x20); + while (i + 1 < end && isAsciiSpace(utf8[i + 1])) i++; + continue; + } + // ossl_tolower is ASCII-only; bytes with the MSB set pass through. + out.push(byte >= 0x41 && byte <= 0x5a ? byte + 0x20 : byte); + } + + return derTlv(UTF8_STRING_TAG, Buffer.from(out)); +} + +/** `ossl_isspace`: space, tab, newline, vertical tab, form feed, carriage return. */ +function isAsciiSpace(byte: number): boolean { + return ( + byte === 0x20 || (byte >= 0x09 && byte <= 0x0d) + ); +} + +/** + * Reinterpret an ASN.1 string's bytes as UTF-8, per its declared type. + * Returns null for a value whose length can't belong to its type (an odd-length + * BMPString, say) — the caller then passes the attribute through unchanged + * rather than inventing an encoding for malformed input. + */ +function transcodeToUtf8(tag: number, content: Buffer): Buffer | null { + switch (tag) { + case 0x0c: // UTF8String — already UTF-8. + return content; + case 0x13: // PrintableString + case 0x16: // IA5String + case 0x1a: // VisibleString + // Defined as ASCII subsets, so their bytes are already valid UTF-8. + return content; + case 0x14: // T61String — OpenSSL decodes these as Latin-1. + return Buffer.from(content.toString("latin1"), "utf8"); + case 0x1e: { + // BMPString — UTF-16BE. Node decodes UTF-16LE only, so swap pairs first. + if (content.length % 2 !== 0) return null; + const swapped = Buffer.from(content); + swapped.swap16(); + return Buffer.from(swapped.toString("utf16le"), "utf8"); + } + case 0x1c: { + // UniversalString — UTF-32BE, decoded a code point at a time. + if (content.length % 4 !== 0) return null; + let text = ""; + for (let i = 0; i < content.length; i += 4) { + const codePoint = content.readUInt32BE(i); + if (codePoint > 0x10ffff) return null; + text += String.fromCodePoint(codePoint); + } + return Buffer.from(text, "utf8"); + } + default: + return content; + } +} + +/** + * DER `SET OF` ordering, matching OpenSSL's `der_cmp`: compare the shared + * prefix, then let the shorter encoding sort first. + */ +function compareDerSetMembers(a: Buffer, b: Buffer): number { + const shared = Math.min(a.length, b.length); + const diff = Buffer.compare(a.subarray(0, shared), b.subarray(0, shared)); + return diff !== 0 ? diff : a.length - b.length; +} + +/** Encode a single DER TLV with a minimal definite-form length. */ +function derTlv(tag: number, content: Buffer): Buffer { + return Buffer.concat([Buffer.from([tag]), derLength(content.length), content]); +} + +function derLength(length: number): Buffer { + if (length < 0x80) return Buffer.from([length]); + const bytes: number[] = []; + let remaining = length; + while (remaining > 0) { + bytes.unshift(remaining & 0xff); + remaining >>>= 8; + } + return Buffer.from([0x80 | bytes.length, ...bytes]); +} + +interface TlvResult { + tag: number; + contentOffset: number; + contentLength: number; +} + +/** + * Read an ASN.1 TLV (Tag-Length-Value) at the given offset. + */ +function readTag(buf: Buffer, offset: number): TlvResult | null { + if (offset >= buf.length) return null; + + const tag = buf[offset]; + let pos = offset + 1; + + if (pos >= buf.length) return null; + + let length = buf[pos]; + pos++; + + if (length & 0x80) { + // Long form length + const numBytes = length & 0x7f; + if (numBytes > 4 || pos + numBytes > buf.length) return null; + length = 0; + for (let i = 0; i < numBytes; i++) { + length = (length << 8) | buf[pos]; + pos++; + } + } + + return { + tag, + contentOffset: pos, + contentLength: length, + }; +} + +function isHashSymlink(filename: string): boolean { + // Hash symlinks match pattern: 8 hex chars + dot + digit(s) + return /^[0-9a-f]{8}\.\d+$/.test(filename); +} + +function isSymlink(filePath: string): boolean { + try { + return fs.lstatSync(filePath).isSymbolicLink(); + } catch { + return false; + } +} diff --git a/src/shared/src/cert/validation.ts b/src/shared/src/cert/validation.ts index c158e97..506d9f9 100644 --- a/src/shared/src/cert/validation.ts +++ b/src/shared/src/cert/validation.ts @@ -98,11 +98,49 @@ export interface NonLocalSanEntry { value: string; } +/** + * Why a SAN set was rejected. Split into *structural* problems — the cert's + * SAN extension isn't the shape a dev cert has, so we can't meaningfully say + * what it's scoped to — and `non-local`, which means we read it fine and it + * covers names outside local scopes. + * + * The distinction is load-bearing at the call site: + * `allowNonLocalContainerCertSans` is an opt-out for *scope*, so it may + * override `non-local` and must never override a structural reject. Trusting + * a cert whose SAN we could not read is not a scope decision the user is in + * a position to make. + */ +export type SanRejectReason = + /** No SubjectAlternativeName extension at all. */ + | "missing" + /** SAN extension present but its DER doesn't decode. */ + | "unparseable" + /** SAN decoded but held zero entries. */ + | "no-host-entries" + /** SAN carried a GeneralName type other than dNSName / iPAddress. */ + | "unsupported-entry" + /** SAN read fine; at least one dNSName / iPAddress is outside local scope. */ + | "non-local"; + export interface SanLocalValidationResult { ok: boolean; + /** Set whenever `ok` is false. */ + reason?: SanRejectReason; + /** Populated only for `reason === "non-local"`. */ nonLocalEntries: NonLocalSanEntry[]; + /** Human-readable supplement for logs / UI. */ + detail?: string; } +/** Successful scan, or the structural reason the SAN set is unusable. */ +export type SanScanResult = + | { ok: true; entries: NonLocalSanEntry[] } + | { + ok: false; + reason: Exclude; + detail?: string; + }; + const ALLOWED_DNS_EXACT = new Set([ "localhost", "host.docker.internal", @@ -138,63 +176,227 @@ const ALLOWED_DNS_SUFFIXES = [ * link-local (169.254/16). * IPv6 — loopback (::1), unique-local (fc00::/7), link-local (fe80::/10). * - * A SAN entry that doesn't parse is treated as non-local — fail-closed. + * Fail-closed on anything we can't fully read: a missing, undecodable, empty, + * or non-dNSName/iPAddress SAN set is rejected outright rather than being + * treated as "nothing to object to". See `scanSanEntries`. */ export function validateLocalSans(cert: DevCert): SanLocalValidationResult { - const sanEntries = collectSanEntries(cert); - const nonLocalEntries: NonLocalSanEntry[] = []; - - for (const entry of sanEntries) { - if (entry.type === "dns") { - if (!isLocalDnsName(entry.value)) { - nonLocalEntries.push(entry); - } - } else { - if (!isLocalIp(entry.value)) { - nonLocalEntries.push(entry); - } - } + const scan = scanSanEntries(cert); + if (!scan.ok) { + return { + ok: false, + reason: scan.reason, + nonLocalEntries: [], + detail: scan.detail, + }; } - return { ok: nonLocalEntries.length === 0, nonLocalEntries }; + const nonLocalEntries = scan.entries.filter((entry) => + entry.type === "dns" + ? !isLocalDnsName(entry.value) + : !isLocalIp(entry.value) + ); + + if (nonLocalEntries.length > 0) { + return { ok: false, reason: "non-local", nonLocalEntries }; + } + return { ok: true, nonLocalEntries: [] }; } /** - * Extract DNS + IP entries from a cert's SubjectAlternativeName extension. - * Returns an empty list when the extension is missing — callers treat that - * as "no entries to inspect"; the surrounding `isValidDevCert` check - * separately enforces CN=localhost. + * Read a cert's SubjectAlternativeName entries, or say precisely why they + * can't be read. + * + * Every rejection here is deliberate rather than a silent drop, because the + * caller's next step is installing the cert into an OS trust store. Three + * cases that a "collect what we recognize and ignore the rest" reader would + * have waved through: + * + * - **No SAN extension / an empty one.** Nothing to scope-check, so the + * local-only restriction has nothing to bite on. No genuine ASP.NET dev + * cert looks like this (the canonical one carries seven entries), and a + * cert with no SAN can't authenticate a hostname to any modern client + * anyway — so there's no legitimate reason to trust one. + * - **A GeneralName type other than dNSName / iPAddress** (rfc822Name, + * uniformResourceIdentifier, directoryName, otherName…). These aren't + * used for TLS server identity, so ignoring them is defensible on paper — + * but it means reporting "SANs are local-only" about a cert we only + * partially inspected. A dev cert has no business carrying them, so + * rejecting costs nothing real and keeps the report honest. + * - **Undecodable SAN DER.** `@peculiar/x509` parses extensions lazily and + * throws from `getExtension`, so this used to surface as an exception that + * happened to be caught two frames up in the accept handler and mapped to + * a generic parse failure. That made fail-closed an accident of the call + * site: adding a `try/catch` here — the obvious defensive edit — would + * have silently turned it into fail-open. It's now explicit and local. */ -export function collectSanEntries(cert: DevCert): NonLocalSanEntry[] { - const ext = cert.inner.getExtension(SAN_EXTENSION_OID); - if (!ext) return []; - - // @peculiar/x509 exposes SubjectAlternativeNameExtension with parsed - // `names` (a GeneralNames sequence). Walking it via the wrapper avoids - // re-implementing ASN.1 parsing here. - const names = ( - ext as unknown as { - names?: { items?: { type?: string; value?: string }[] }; +export function scanSanEntries(cert: DevCert): SanScanResult { + let items: { type?: string; value?: string }[]; + try { + const ext = cert.inner.getExtension(SAN_EXTENSION_OID); + if (!ext) return { ok: false, reason: "missing" }; + + // @peculiar/x509 exposes SubjectAlternativeNameExtension with parsed + // `names` (a GeneralNames sequence). Walking it via the wrapper avoids + // re-implementing ASN.1 parsing here. + const names = ( + ext as unknown as { + names?: { items?: { type?: string; value?: string }[] }; + } + ).names; + if (!Array.isArray(names?.items)) { + return { + ok: false, + reason: "unparseable", + detail: "SAN extension did not decode into GeneralNames", + }; } - ).names; - const items = names?.items ?? []; + items = names.items; + } catch (err: unknown) { + return { + ok: false, + reason: "unparseable", + detail: err instanceof Error ? err.message : String(err), + }; + } - const out: NonLocalSanEntry[] = []; + const entries: NonLocalSanEntry[] = []; for (const item of items) { if (item.type === "dns" && typeof item.value === "string") { - out.push({ type: "dns", value: item.value }); + entries.push({ type: "dns", value: item.value }); } else if ( (item.type === "ip" || item.type === "ipAddress") && typeof item.value === "string" ) { - out.push({ type: "ip", value: item.value }); + entries.push({ type: "ip", value: item.value }); + } else { + return { + ok: false, + reason: "unsupported-entry", + detail: `SAN entry of type '${item.type ?? "unknown"}'`, + }; } } - return out; + + if (entries.length === 0) return { ok: false, reason: "no-host-entries" }; + return { ok: true, entries }; } const SAN_EXTENSION_OID = "2.5.29.17"; +// --------------------------------------------------------------------------- +// Trust-anchor shape validation — the second half of the container-push +// gate. `validateLocalSans` asks "what names is this cert scoped to?", which +// only constrains anything if the cert is a leaf that can authenticate ONLY +// itself. A CA certificate's own SANs say nothing about what it may issue +// for, so without the check below the SAN restriction is trivially bypassed: +// push a CA whose own SANs are `localhost`, then sign a leaf for any name +// you like and the host trusts the chain. +// --------------------------------------------------------------------------- + +export type LeafTrustRejectReason = + /** basicConstraints / EKU present but undecodable. */ + | "unreadable" + /** No basicConstraints extension, so "is this a CA?" is unanswerable. */ + | "missing-basic-constraints" + /** basicConstraints says cA=TRUE. */ + | "is-certificate-authority" + /** No extendedKeyUsage, which most stacks read as "any purpose". */ + | "missing-eku" + /** EKU contains anyExtendedKeyUsage, which re-opens "any purpose". */ + | "eku-any-purpose" + /** EKU present but without id-kp-serverAuth. */ + | "eku-no-server-auth"; + +export interface LeafTrustShapeResult { + ok: boolean; + reason?: LeafTrustRejectReason; + detail?: string; +} + +const BASIC_CONSTRAINTS_OID = "2.5.29.19"; +const EKU_OID = "2.5.29.37"; +const EKU_SERVER_AUTH_OID = "1.3.6.1.5.5.7.3.1"; +const EKU_ANY_PURPOSE_OID = "2.5.29.37.0"; + +/** + * Check that a certificate is safe to install as a trust anchor for TLS and + * nothing more: it must be a leaf (cannot issue other certificates) and it + * must be scoped to server authentication. + * + * Required, not merely preferred, because `trustCertificate` puts the cert in + * `CurrentUser\Root` on Windows, the login keychain's SSL trust settings on + * macOS, and the .NET Root store + OpenSSL CApath + browser NSS databases + * (with the `C` "trusted CA" flag) on Linux. Those are CA positions. Whether + * the cert can actually *act* as a CA from there comes down to + * basicConstraints — which nothing checked before this. + * + * Both extensions are required to be **present**, not just non-contradictory. + * An absent basicConstraints leaves "is this a CA?" to each validator's + * historical quirks, and an absent EKU reads as "any purpose" on Windows, + * where `certutil -addstore Root` applies no policy constraint of its own. + * Every genuine ASP.NET dev cert carries both — ours via `generateCertificate` + * and .NET's via `CertificateManager` — so requiring them rejects nothing + * legitimate. + * + * Additional specific EKUs (say `clientAuth`) are tolerated; only + * `anyExtendedKeyUsage` is refused, since it is equivalent to no constraint. + */ +export function validateLeafTrustShape(cert: DevCert): LeafTrustShapeResult { + let isCa: boolean | undefined; + let ekuUsages: string[] | undefined; + + try { + const bc = cert.inner.getExtension(BASIC_CONSTRAINTS_OID) as unknown as + | { ca?: boolean } + | null; + if (!bc) return { ok: false, reason: "missing-basic-constraints" }; + isCa = bc.ca; + + const eku = cert.inner.getExtension(EKU_OID) as unknown as + | { usages?: string[] } + | null; + ekuUsages = eku ? eku.usages : undefined; + if (eku && !Array.isArray(ekuUsages)) { + return { + ok: false, + reason: "unreadable", + detail: "extendedKeyUsage did not decode into a usage list", + }; + } + if (!eku) return { ok: false, reason: "missing-eku" }; + } catch (err: unknown) { + return { + ok: false, + reason: "unreadable", + detail: err instanceof Error ? err.message : String(err), + }; + } + + if (typeof isCa !== "boolean") { + return { + ok: false, + reason: "unreadable", + detail: "basicConstraints did not decode a cA flag", + }; + } + if (isCa) return { ok: false, reason: "is-certificate-authority" }; + + const usages = ekuUsages ?? []; + if (usages.includes(EKU_ANY_PURPOSE_OID)) { + return { ok: false, reason: "eku-any-purpose" }; + } + if (!usages.includes(EKU_SERVER_AUTH_OID)) { + return { + ok: false, + reason: "eku-no-server-auth", + detail: usages.join(", "), + }; + } + + return { ok: true }; +} + function isLocalDnsName(name: string): boolean { let candidate = name.trim().toLowerCase(); if (candidate.length === 0) return false; diff --git a/src/shared/src/index.ts b/src/shared/src/index.ts index be77796..b7aac58 100644 --- a/src/shared/src/index.ts +++ b/src/shared/src/index.ts @@ -40,6 +40,12 @@ export { SAN_DNS_NAMES, SAN_IP_ADDRESSES, } from "./cert/properties"; +export { + computeSubjectHash, + ensureHashSymlink, + hasHashSymlink, + rehashDirectory, +} from "./cert/rehash"; export { buildPfx, parsePfx } from "./cert/pfx"; export type { BuildPfxOptions, ParsedPfx } from "./cert/pfx"; export { loadPfx, loadPemPair } from "./cert/loader"; @@ -49,11 +55,16 @@ export { getCertificateVersion, computeThumbprint, validateLocalSans, - collectSanEntries, + scanSanEntries, + validateLeafTrustShape, } from "./cert/validation"; export type { NonLocalSanEntry, SanLocalValidationResult, + SanRejectReason, + SanScanResult, + LeafTrustShapeResult, + LeafTrustRejectReason, } from "./cert/validation"; export { classifyCandidate, @@ -99,12 +110,12 @@ export type { BaseStoreOptions, LinuxNssTrustReporter, } from "./platform/types"; -export { - BaseCertificateStore, - classifyCandidate as classifyPlatformCandidate, - selectBestDevCert as selectBestPlatformDevCert, -} from "./platform/baseStore"; -export type { ClassifyOptions as PlatformClassifyOptions } from "./platform/baseStore"; +// Only the base class is re-exported here. The platform-flavored (localized, +// logging) `classifyCandidate` / `selectBestDevCert` wrappers live in +// `./platform/baseStore`; consumers that want those import the submodule +// directly, so the barrel doesn't need aliases that shadow the pure +// classifier exported above. +export { BaseCertificateStore } from "./platform/baseStore"; export { LinuxCertificateStore } from "./platform/linuxStore"; export type { LinuxCertificateStoreOptions } from "./platform/linuxStore"; export { MacCertificateStore } from "./platform/macStore"; @@ -133,7 +144,7 @@ export type { // reimplementing availability detection / selection logic. export { NativeBackend } from "./backends/native"; export { DotnetBackend } from "./backends/dotnet"; -export { selectBackend, describeAutoBackend } from "./backends/select"; +export { selectBackend } from "./backends/select"; export type { Backend, BackendKind, diff --git a/src/shared/src/platform/baseStore.ts b/src/shared/src/platform/baseStore.ts index 4cea1de..3ab5565 100644 --- a/src/shared/src/platform/baseStore.ts +++ b/src/shared/src/platform/baseStore.ts @@ -146,7 +146,7 @@ function emitSkipLog(report: SkipReport, localize: Localizer): void { * `this.localize` so subclasses don't repeat the plumbing. * * Subclasses implement the platform-specific methods: findExistingDevCert, - * saveCertificate, trustCertificate, removeCertificates, and isTrusted. + * saveCertificate, trustCertificate, and isTrusted. */ export abstract class BaseCertificateStore implements PlatformCertificateStore { protected readonly localize: Localizer; @@ -192,8 +192,6 @@ export abstract class BaseCertificateStore implements PlatformCertificateStore { abstract trustCertificate(cert: DevCert): Promise; - abstract removeCertificates(): Promise; - /** * Public wrapper around `isTrusted` that satisfies the * `PlatformCertificateStore.isCertTrusted` contract — verify the diff --git a/src/shared/src/platform/linuxStore.ts b/src/shared/src/platform/linuxStore.ts index 086ca46..00f983d 100644 --- a/src/shared/src/platform/linuxStore.ts +++ b/src/shared/src/platform/linuxStore.ts @@ -2,11 +2,14 @@ import * as fs from "fs"; import * as path from "path"; import { BaseCertificateStore } from "./baseStore"; import { trustInNss, type NssTrustResult } from "./nssTrust"; -import { runProcess } from "./processUtil"; import { type LinuxNssTrustReporter, type BaseStoreOptions } from "./types"; import { type DevCert, type DevKey } from "../cert/types"; -import { ASPNET_HTTPS_OID } from "../cert/properties"; import { buildPfx } from "../cert/pfx"; +import { + computeSubjectHash, + ensureHashSymlink, + hasHashSymlink, +} from "../cert/rehash"; import { getDotNetStorePath, getDotNetRootStorePath, @@ -76,7 +79,7 @@ export class LinuxCertificateStore extends BaseCertificateStore { async trustCertificate(cert: DevCert): Promise { await this.trustInDotNetRootStore(cert); - await this.trustViaOpenSsl(cert); + this.trustViaOpenSsl(cert); await this.trustInNssBrowsers(cert); } @@ -113,32 +116,8 @@ export class LinuxCertificateStore extends BaseCertificateStore { this.nssTrustReporter(result, pemPath); } - async removeCertificates(): Promise { - await this.removeDevCertsFromDir(getDotNetStorePath()); - await this.removeDevCertsFromDir(this.dotNetRootStorePath); - - const trustDir = getOpenSslTrustDir(); - if (fs.existsSync(trustDir)) { - const entries = fs.readdirSync(trustDir); - for (const entry of entries) { - const fullPath = path.join(trustDir, entry); - if (entry.startsWith("aspnetcore-localhost-")) { - fs.unlinkSync(fullPath); - } else if (isHashSymlink(entry)) { - try { - if (fs.lstatSync(fullPath).isSymbolicLink()) { - fs.unlinkSync(fullPath); - } - } catch { - // ignore - } - } - } - } - } - protected isTrusted( - _cert: DevCert, + cert: DevCert, thumbprint: string ): Promise { // Both authoritative trust surfaces `trustCertificate` writes must be @@ -154,14 +133,45 @@ export class LinuxCertificateStore extends BaseCertificateStore { // reporter toast with manual guidance, and requiring it here would // flap `checkStatus().isTrusted` to permanently-false on hosts // without NSS tooling. - const pemPath = path.join(getOpenSslTrustDir(), getPemFileName(thumbprint)); + // + // The hash symlink is part of the OpenSSL surface, not an extra: without a + // link OpenSSL resolves, the PEM sitting in the directory establishes + // nothing. Checking it also repairs hosts trusted before the subject hash + // was computed canonically — their link is under the WRONG hash, so a + // PEM-and-PFX-only check would report "trusted", skip `trustCertificate`, + // and leave the broken link in place forever. + const trustDir = getOpenSslTrustDir(); + const pemFileName = getPemFileName(thumbprint); + const pemPath = path.join(trustDir, pemFileName); const rootPfxPath = path.join( this.dotNetRootStorePath, `${thumbprint}.pfx` ); - return Promise.resolve( - fs.existsSync(pemPath) && fs.existsSync(rootPfxPath) - ); + // The PEM's *contents* are checked, not just its existence. The filename + // is thumbprint-derived, so a different cert lands elsewhere — but a + // truncated or externally-rewritten file keeps the name, and a name-only + // check would report trust for bytes OpenSSL cannot load. The link is + // then resolved against what is actually on disk rather than against + // `cert.pem`, so a stale link can't vouch for replaced content. This + // mirrors `pemInstalledAndLinked` in the workspace extension's + // `certInstaller.ts`; the two sides of the same trust dir should not + // disagree about what "installed" means. + if (!fs.existsSync(rootPfxPath)) return Promise.resolve(false); + + let onDisk: string; + try { + onDisk = fs.readFileSync(pemPath, "utf-8"); + } catch { + return Promise.resolve(false); + } + if (onDisk !== cert.pem) return Promise.resolve(false); + + // A subject we can't hash gets no symlink from `ensureHashSymlink` + // either, so demanding one would re-trust on every activation and never + // converge. Same carve-out, same reason, as the workspace side. + if (computeSubjectHash(onDisk) === null) return Promise.resolve(true); + + return Promise.resolve(hasHashSymlink(trustDir, pemFileName, onDisk)); } // --- Linux-specific trust helpers --- @@ -178,7 +188,7 @@ export class LinuxCertificateStore extends BaseCertificateStore { fs.writeFileSync(certPath, pfxBytes, { mode: 0o644 }); } - private async trustViaOpenSsl(cert: DevCert): Promise { + private trustViaOpenSsl(cert: DevCert): void { const trustDir = getOpenSslTrustDir(); fs.mkdirSync(trustDir, { recursive: true }); @@ -204,90 +214,13 @@ export class LinuxCertificateStore extends BaseCertificateStore { // is idempotent (overwrites identical content); rehashing afterward // is a no-op when nothing changed. fs.writeFileSync(pemPath, cert.pem, { mode: 0o644 }); - await this.rehashDirectory(trustDir); - } - - private async rehashDirectory(directory: string): Promise { - const entries = fs.readdirSync(directory); - - // Remove existing hash symlinks - for (const entry of entries) { - if (isHashSymlink(entry)) { - const fullPath = path.join(directory, entry); - try { - if (fs.lstatSync(fullPath).isSymbolicLink()) { - fs.unlinkSync(fullPath); - } - } catch { - // ignore - } - } - } - - // Create new hash symlinks for all PEM/CRT files - const certFiles = fs - .readdirSync(directory) - .filter((f) => /\.(pem|crt|cer)$/i.test(f)); - - for (const certFile of certFiles) { - const fullPath = path.join(directory, certFile); - try { - if (fs.lstatSync(fullPath).isSymbolicLink()) continue; - } catch { - continue; - } - - const hash = await this.getOpenSslSubjectHash(fullPath); - if (!hash) continue; - - // Slot 0-9 covers any realistic collision count. Catch EEXIST so a - // concurrent rehash can't crash this one mid-loop. - for (let i = 0; i < 10; i++) { - const linkPath = path.join(directory, `${hash}.${i}`); - if (fs.existsSync(linkPath)) continue; - try { - fs.symlinkSync(certFile, linkPath); - break; - } catch (err: unknown) { - if ((err as NodeJS.ErrnoException).code === "EEXIST") continue; - throw err; - } - } - } - } - - private async getOpenSslSubjectHash( - certPath: string - ): Promise { - const result = await runProcess("openssl", [ - "x509", - "-hash", - "-noout", - "-in", - certPath, - ]); - if (result.exitCode !== 0) return null; - return result.stdout.trim() || null; - } - private async removeDevCertsFromDir(dir: string): Promise { - if (!fs.existsSync(dir)) return; - - const files = fs.readdirSync(dir).filter((f) => f.endsWith(".pfx")); - for (const file of files) { - const pfxPath = path.join(dir, file); - try { - const result = await this.loadPfx(pfxPath); - if (result && result.cert.hasExtension(ASPNET_HTTPS_OID)) { - fs.unlinkSync(pfxPath); - } - } catch { - // Skip files that can't be parsed - } - } + // Targeted symlink for our PEM only — the same call the workspace + // extension makes on its side of the sync, so both ends of the trust + // dir are maintained by one implementation. `ensureHashSymlink` is + // pure TypeScript: the host needs no `openssl` binary to establish + // OpenSSL trust, which matters because the host is a developer laptop + // we don't control, not a container image we build. + ensureHashSymlink(trustDir, pemFileName, cert.pem); } } - -function isHashSymlink(filename: string): boolean { - return /^[0-9a-f]{8}\.\d+$/.test(filename); -} diff --git a/src/shared/src/platform/macStore.ts b/src/shared/src/platform/macStore.ts index 16cb61c..870b460 100644 --- a/src/shared/src/platform/macStore.ts +++ b/src/shared/src/platform/macStore.ts @@ -12,6 +12,7 @@ import { getCertificateVersion, isValidDevCert } from "../cert/validation"; import { certToDer } from "../cert/exporter"; import { ASPNET_HTTPS_OID } from "../cert/properties"; import { DevCert, type DevKey } from "../cert/types"; +import { log } from "../logger"; /** * macOS certificate store implementation. @@ -141,6 +142,28 @@ export class MacCertificateStore extends BaseCertificateStore { "-Z", this.keychainPath, ]); + // Truncated output means we scanned a prefix of the keychain and simply + // don't know. Answer YES — deliberately failing open, which inverts the + // usual instinct because here the closed direction is the destructive one. + // + // A `false` gets the on-disk PFX force-skipped as an orphaned cache file, + // which empties `findExistingDevCert`, which makes `checkStatus()` report + // `exists: false`, which sends `CertManager.trust()` down the `generate()` + // branch: a brand-new cert plus an `add-trusted-cert` keychain password + // prompt. That new cert then lands in the same keychain, so the next call + // truncates even sooner — a self-feeding loop with no way out. + // + // The open direction costs at most one redundant re-trust: `checkStatus` + // establishes trust separately via `security verify-cert` in `isTrusted`, + // so a cert that genuinely isn't in the keychain is caught there. + if (result.truncated) { + log( + `macOS keychain enumeration exceeded the output cap while looking for ${thumbprint}; ` + + `assuming the certificate IS present rather than regenerating it. ` + + `A login keychain this large may want pruning.` + ); + return true; + } if (result.exitCode !== 0) return false; const needle = thumbprint.toUpperCase(); // Modern macOS prints both `SHA-256 hash:` and `SHA-1 hash:` lines @@ -167,7 +190,18 @@ export class MacCertificateStore extends BaseCertificateStore { "-Z", this.keychainPath, ]); - if (result.exitCode !== 0) return []; + // Unlike `isCertInKeychain`, an incomplete answer here is harmless: this + // pass only emits a "in the keychain but no cache PFX" warning, so a short + // list costs a log line rather than a decision. Say why, then carry on + // with whatever prefix we got. + if (result.truncated) { + log( + "macOS keychain enumeration exceeded the output cap; the keychain-resident " + + "dev cert warnings below cover only part of the keychain." + ); + } else if (result.exitCode !== 0) { + return []; + } const out: Array<{ cert: DevCert; thumbprint: string }> = []; const pemBlocks = extractPemBlocks(result.stdout); @@ -231,86 +265,6 @@ export class MacCertificateStore extends BaseCertificateStore { } } - async removeCertificates(): Promise { - // For each PFX we manage on disk, load it, run untrust + delete-from- - // keychain by thumbprint, then unlink the PFX. Three-step structure - // because trust settings are stored separately from the cert (in - // TrustSettings.plist) and reference it by hash — if we delete the - // cert from the keychain first, the trust settings become orphaned - // dangling entries that the next `add-trusted-cert` may flag as - // duplicates. - // - // Matching dev certs by filename (`aspnetcore-localhost-*.pfx`) + - // the dev-cert OID is narrower than matching keychain entries by - // `-c localhost`: the user may have unrelated `localhost` certs - // added for other tools, and bulk-untrusting by keychain or by CN - // would nuke those too. - if (!fs.existsSync(this.devCertsDir)) return; - - const pfxFiles = fs - .readdirSync(this.devCertsDir) - .filter( - (f) => f.startsWith("aspnetcore-localhost-") && f.endsWith(".pfx") - ); - - for (const pfxFile of pfxFiles) { - const pfxPath = path.join(this.devCertsDir, pfxFile); - let parsed: Awaited>; - try { - parsed = await this.loadPfx(pfxPath); - } catch { - // Unparseable — skip the untrust step but still unlink below - // so we don't leave stale files around. - parsed = null; - } - - if (parsed && parsed.cert.hasExtension(ASPNET_HTTPS_OID)) { - // Step 1: untrust. `security remove-trusted-cert` takes a - // cert file (DER / PEM) as its positional, NOT a keychain - // path. Trust settings were added without `-d` (user domain, - // matching `add-trusted-cert` above), so we remove without - // `-d` too. Non-zero exit just means there was no trust - // settings entry to remove — not an error in cleanup. - const tmpCert = path.join( - os.tmpdir(), - `devcert-untrust-${randomUUID()}.cer` - ); - fs.writeFileSync(tmpCert, certToDer(parsed.cert)); - try { - await runProcess("security", ["remove-trusted-cert", tmpCert]); - } finally { - try { - fs.unlinkSync(tmpCert); - } catch { - /* ignore */ - } - } - - // Step 2: delete the keychain entries. delete-certificate exits - // non-zero once there are no more entries matching the hash; - // loop with a generous bound to drain any duplicates left by - // past regenerations. - for (let i = 0; i < 100; i++) { - const result = await runProcess("security", [ - "delete-certificate", - "-Z", - parsed.thumbprint, - this.keychainPath, - ]); - if (result.exitCode !== 0) break; - } - } - - // Step 3: unlink the PFX. Done last so a mid-cleanup interruption - // leaves the file in place and the cleanup is restartable. - try { - fs.unlinkSync(pfxPath); - } catch { - /* ignore */ - } - } - } - protected async isTrusted( cert: DevCert, _thumbprint: string diff --git a/src/shared/src/platform/nssTrust.ts b/src/shared/src/platform/nssTrust.ts index d89b5f4..dab8cc8 100644 --- a/src/shared/src/platform/nssTrust.ts +++ b/src/shared/src/platform/nssTrust.ts @@ -3,16 +3,72 @@ import * as os from "os"; import * as path from "path"; import { runProcess } from "./processUtil"; import { log } from "../logger"; +import { DevCert } from "../cert/types"; export interface NssTrustResult { success: boolean; message: string; } +/** + * Nickname stem. NSS nicknames are unique per database, so the thumbprint is + * appended (see `nicknameFor`) — without it, trusting a second dev cert would + * evict the first from every browser DB, which is exactly the ping-ponging + * `LinuxCertificateStore.trustViaOpenSsl` was deliberately made additive to + * avoid. Host-generated and container-pushed certs have to coexist here too. + */ const CERT_NAME = "Dev Container Dev Cert"; +/** + * Nickname used by versions before per-cert nicknames existed. Removed + * alongside the per-cert entry on every add so an upgrade doesn't strand a + * permanently-trusted cert under a name we no longer write. + */ +const LEGACY_CERT_NAME = CERT_NAME; + +/** + * Per-certificate NSS nickname. Falls back to the bare stem when the PEM + * can't be parsed — `certutil -A` would fail on that input anyway, so the + * nickname is moot at that point. + */ +function nicknameFor(pemPath: string): string { + try { + const cert = new DevCert(fs.readFileSync(pemPath, "utf-8")); + return `${CERT_NAME} (${cert.thumbprintSha1})`; + } catch { + return CERT_NAME; + } +} + type NssTargetKind = "chromium-shared" | "firefox-profiles"; +/** + * NSS SSL trust flag, chosen per browser family. + * + * The cert we add is a self-signed **end entity** (`generateCertificate` + * emits `basicConstraints` `cA=FALSE`), not a CA, so the honest encoding is + * `P` — `CERTDB_TRUSTED`, "trusted peer", consulted when the cert *is* the + * certificate being validated. `C` is `CERTDB_TRUSTED_CA`, consulted only + * when the cert sits in an *issuer* position, which ours never does. + * + * Firefox is the exception, and it's an empirical one: it does not honour + * `P` for server certs, so `C` is what actually produces trust there. This + * mirrors `dotnet dev-certs https --trust`, whose `UnixCertificateManager` + * makes the same split (`usage = nssDb.IsFirefox ? "C" : "P"`) with the + * comment "Firefox doesn't seem to respected the more correct 'trusted + * peer' (P) usage". Microsoft validated that against real browsers; we + * follow it rather than re-deriving it. + * + * Sending `C` to a Chromium DB does not work: `certutil -V -u V` rejects + * such an entry with "Issuer certificate is invalid", because nothing ever + * consults the CA bit for an end entity. dotnet's own verify step encodes + * the same asymmetry — it runs `-V -u V` for Chromium but only `-L` + * (existence) for Firefox, since `-V` cannot pass under `C`. + */ +function trustFlagsFor(kind: NssTargetKind): string { + return kind === "firefox-profiles" ? "C,," : "P,,"; +} + interface NssTarget { label: string; kind: NssTargetKind; @@ -143,12 +199,13 @@ export async function trustInNss(pemPath: string): Promise { const outcomes: DbOutcome[] = []; const targets = getNssTargets(os.homedir()); + const nickname = nicknameFor(pemPath); for (const target of targets) { if (target.kind === "chromium-shared") { - await scanChromiumShared(target, pemPath, outcomes); + await scanChromiumShared(target, pemPath, nickname, outcomes); } else { - await scanFirefoxProfiles(target, pemPath, outcomes); + await scanFirefoxProfiles(target, pemPath, nickname, outcomes); } } @@ -178,13 +235,19 @@ export async function trustInNss(pemPath: string): Promise { async function scanChromiumShared( target: NssTarget, pemPath: string, + nickname: string, outcomes: DbOutcome[] ): Promise { if (!fs.existsSync(path.join(target.root, "cert9.db"))) { log(`NSS scan: ${target.label} not present at ${target.root}, skipping.`); return; } - const r = await trustInNssDb(`sql:${target.root}`, pemPath); + const r = await trustInNssDb( + `sql:${target.root}`, + pemPath, + nickname, + target.kind + ); outcomes.push({ label: target.label, ok: r.exitCode === 0, @@ -195,6 +258,7 @@ async function scanChromiumShared( async function scanFirefoxProfiles( target: NssTarget, pemPath: string, + nickname: string, outcomes: DbOutcome[] ): Promise { if (!fs.existsSync(target.root)) { @@ -226,7 +290,12 @@ async function scanFirefoxProfiles( for (const profile of profiles) { const dbPath = path.join(target.root, profile); - const r = await trustInNssDb(`sql:${dbPath}`, pemPath); + const r = await trustInNssDb( + `sql:${dbPath}`, + pemPath, + nickname, + target.kind + ); outcomes.push({ label: `${target.label} (${profile})`, ok: r.exitCode === 0, @@ -237,19 +306,34 @@ async function scanFirefoxProfiles( async function trustInNssDb( dbArg: string, - pemPath: string + pemPath: string, + nickname: string, + kind: NssTargetKind ): Promise<{ exitCode: number; stderr: string }> { - // Remove any existing cert with this name first to make the operation idempotent - await runProcess("certutil", ["-D", "-d", dbArg, "-n", CERT_NAME]); + // Drop the shared nickname older versions used, so upgrading doesn't leave + // a cert permanently trusted under a name we no longer manage. Skipped when + // this cert IS the legacy-named one (unparseable PEM fallback) — the + // per-nickname delete below covers that case. + if (nickname !== LEGACY_CERT_NAME) { + await runProcess("certutil", ["-D", "-d", dbArg, "-n", LEGACY_CERT_NAME]); + } + // Remove any existing cert with this name first to make the operation + // idempotent. Both deletes exit non-zero when there's nothing to remove; + // that's the common case and not an error. + await runProcess("certutil", ["-D", "-d", dbArg, "-n", nickname]); + // The deletes above also migrate an entry written by an older version + // under different flags: `certutil -A` does not rewrite the trust string + // of an existing nickname, so delete-then-add is what actually moves a + // Chromium DB off the previous blanket `CT,,`. const result = await runProcess("certutil", [ "-A", "-d", dbArg, "-t", - "CT,,", + trustFlagsFor(kind), "-n", - CERT_NAME, + nickname, "-i", pemPath, ]); diff --git a/src/shared/src/platform/processUtil.ts b/src/shared/src/platform/processUtil.ts index b474723..6904aec 100644 --- a/src/shared/src/platform/processUtil.ts +++ b/src/shared/src/platform/processUtil.ts @@ -5,10 +5,37 @@ import { promisify } from "util"; const execFileAsync = promisify(execFile); +/** + * Cap on captured stdout/stderr, well above Node's 1 MiB `execFile` default. + * + * The default is reachable in practice by `security find-certificate -a` on + * macOS, which dumps every certificate in the login keychain: roughly 2 KB per + * entry for the attribute form and 1.5 KB for the PEM form, so ~500-700 certs. + * That's uncommon but real — MDM-pushed user certs, heavy client-cert use, or + * years of accumulated dev certs. 32 MiB is ~16,000 keychain entries, and the + * buffer is only as large as the output actually produced. + */ +const MAX_OUTPUT_BYTES = 32 * 1024 * 1024; + +/** Node's `error.code` when a child exceeds `maxBuffer`. A string, not a number. */ +const MAXBUFFER_ERROR_CODE = "ERR_CHILD_PROCESS_STDIO_MAXBUFFER"; + export interface ProcessResult { exitCode: number; stdout: string; stderr: string; + /** + * True when the child was killed for exceeding `MAX_OUTPUT_BYTES` — the + * command may well have been on its way to succeeding, and `stdout` holds a + * truncated prefix of its output. + * + * Callers that read `exitCode !== 0` as "the answer is no" MUST check this + * first. Node reports the overflow with a *string* `error.code`, so it lands + * in the same `exitCode: 1` bucket as a genuine failure, and `stderr` comes + * back empty — without this flag there is nothing in the result to tell the + * two apart. + */ + truncated: boolean; } export interface ResolveSafeExecPathOptions { @@ -146,11 +173,20 @@ export async function runProcess( exitCode: 127, stdout: "", stderr: `command not found on PATH: ${command}`, + truncated: false, }; } try { - const result = await execFileAsync(resolved, args, { timeout }); - return { exitCode: 0, stdout: result.stdout, stderr: result.stderr }; + const result = await execFileAsync(resolved, args, { + timeout, + maxBuffer: MAX_OUTPUT_BYTES, + }); + return { + exitCode: 0, + stdout: result.stdout, + stderr: result.stderr, + truncated: false, + }; } catch (err: unknown) { const error = err as Error & { code?: number | string; @@ -163,6 +199,7 @@ export async function runProcess( exitCode, stdout: error.stdout ?? "", stderr: error.stderr ?? error.message, + truncated: error.code === MAXBUFFER_ERROR_CODE, }; } } diff --git a/src/shared/src/platform/types.ts b/src/shared/src/platform/types.ts index f67f462..c17c288 100644 --- a/src/shared/src/platform/types.ts +++ b/src/shared/src/platform/types.ts @@ -87,11 +87,6 @@ export interface PlatformCertificateStore { */ isCertTrusted(cert: DevCert): Promise; - /** - * Remove dev certificates from all stores. - */ - removeCertificates(): Promise; - /** * Check the status of the dev certificate. */ diff --git a/src/shared/src/platform/windowsStore.ts b/src/shared/src/platform/windowsStore.ts index 34bbddd..ca990ce 100644 --- a/src/shared/src/platform/windowsStore.ts +++ b/src/shared/src/platform/windowsStore.ts @@ -319,28 +319,6 @@ export class WindowsCertificateStore extends BaseCertificateStore { } } - async removeCertificates(): Promise { - const script = ` - $ErrorActionPreference = 'SilentlyContinue' - $oid = '${ASPNET_HTTPS_OID}' - foreach ($storePath in @('Cert:\\${this.storeLocation}\\My', 'Cert:\\${this.storeLocation}\\Root')) { - Get-ChildItem $storePath | Where-Object { - $_.Extensions | Where-Object { $_.Oid.Value -eq $oid } - } | ForEach-Object { - Remove-Item -LiteralPath $_.PSPath -Force - } - } - `; - - const pwsh = await getPowerShell(); - await runProcess(pwsh, [ - "-NoProfile", - "-NonInteractive", - "-Command", - script, - ]); - } - protected async isTrusted( _cert: DevCert, thumbprint: string diff --git a/src/vscode-ui-extension/package.json b/src/vscode-ui-extension/package.json index ff73877..8bef02b 100644 --- a/src/vscode-ui-extension/package.json +++ b/src/vscode-ui-extension/package.json @@ -41,6 +41,10 @@ { "command": "devcontainer-dev-certs.trustInBrowsers", "title": "Dev Certs: Trust Certificate in Browsers" + }, + { + "command": "devcontainer-dev-certs.resetContainerCertConsent", + "title": "Dev Certs: Reset Container Certificate Consent" } ], "configuration": { diff --git a/src/vscode-ui-extension/src/cert/exporter.ts b/src/vscode-ui-extension/src/cert/exporter.ts deleted file mode 100644 index 58f7f41..0000000 --- a/src/vscode-ui-extension/src/cert/exporter.ts +++ /dev/null @@ -1,14 +0,0 @@ -// Re-export shim: the canonical home for the exporter helpers is now -// `@devcontainer-dev-certs/shared`. Keeping this thin re-export so existing -// `./cert/exporter` imports across the UI extension (and its test suite) -// keep resolving without a sweeping rename. -export { - exportPfx, - exportPem, - exportRootPfx, - exportLoadedCert, - certToPem, - keyToPem, - certToDer, -} from "@devcontainer-dev-certs/shared"; -export type { ExportedLoadedCert } from "@devcontainer-dev-certs/shared"; diff --git a/src/vscode-ui-extension/src/cert/generator.ts b/src/vscode-ui-extension/src/cert/generator.ts deleted file mode 100644 index 7b106be..0000000 --- a/src/vscode-ui-extension/src/cert/generator.ts +++ /dev/null @@ -1,17 +0,0 @@ -// Re-export shim: the canonical home for `generateCertificate` is now -// `@devcontainer-dev-certs/shared`. Keeping this thin re-export so existing -// `./cert/generator` imports across the UI extension (and its test suite) -// keep resolving without a sweeping rename. The validation helpers -// (`isValidDevCert`, `getCertificateVersion`, `computeThumbprint`) used to be -// defined alongside the generator and were re-exported here for historical -// reasons — we preserve those re-exports so existing call sites still resolve. -export { - generateCertificate, - isValidDevCert, - getCertificateVersion, - computeThumbprint, -} from "@devcontainer-dev-certs/shared"; -export type { - GenerateAlgorithm, - GeneratedCert, -} from "@devcontainer-dev-certs/shared"; diff --git a/src/vscode-ui-extension/src/cert/loader.ts b/src/vscode-ui-extension/src/cert/loader.ts deleted file mode 100644 index b9a34d0..0000000 --- a/src/vscode-ui-extension/src/cert/loader.ts +++ /dev/null @@ -1,6 +0,0 @@ -// Re-export shim: the canonical home for loadPfx / loadPemPair / LoadedCert -// is now `@devcontainer-dev-certs/shared`. Keeping this thin re-export so -// existing `./cert/loader` imports across the UI extension (and its test -// suite) keep resolving without a sweeping rename. -export { loadPfx, loadPemPair } from "@devcontainer-dev-certs/shared"; -export type { LoadedCert } from "@devcontainer-dev-certs/shared"; diff --git a/src/vscode-ui-extension/src/cert/manager.ts b/src/vscode-ui-extension/src/cert/manager.ts deleted file mode 100644 index 78922f1..0000000 --- a/src/vscode-ui-extension/src/cert/manager.ts +++ /dev/null @@ -1,3 +0,0 @@ -// Re-export shim: canonical home is `@devcontainer-dev-certs/shared`. -export { CertManager } from "@devcontainer-dev-certs/shared"; -export type { CertManagerOptions } from "@devcontainer-dev-certs/shared"; diff --git a/src/vscode-ui-extension/src/cert/pfx.ts b/src/vscode-ui-extension/src/cert/pfx.ts deleted file mode 100644 index e686014..0000000 --- a/src/vscode-ui-extension/src/cert/pfx.ts +++ /dev/null @@ -1,9 +0,0 @@ -// Re-export shim: the canonical home for buildPfx / parsePfx is now -// `@devcontainer-dev-certs/shared`. Keeping this thin re-export so existing -// `./cert/pfx` imports across the UI extension (and its test suite) keep -// resolving without a sweeping rename. -export { buildPfx, parsePfx } from "@devcontainer-dev-certs/shared"; -export type { - BuildPfxOptions, - ParsedPfx, -} from "@devcontainer-dev-certs/shared"; diff --git a/src/vscode-ui-extension/src/cert/properties.ts b/src/vscode-ui-extension/src/cert/properties.ts deleted file mode 100644 index d8d0778..0000000 --- a/src/vscode-ui-extension/src/cert/properties.ts +++ /dev/null @@ -1,14 +0,0 @@ -// Re-export shim: the canonical home for these constants is now -// `@devcontainer-dev-certs/shared`. Keeping this thin re-export so existing -// `./cert/properties` imports across the UI extension (and its test suite) -// keep resolving without a sweeping rename. -export { - RSA_KEY_SIZE, - VALIDITY_DAYS, - ASPNET_HTTPS_OID, - ASPNET_HTTPS_OID_FRIENDLY_NAME, - CURRENT_CERTIFICATE_VERSION, - MINIMUM_CERTIFICATE_VERSION, - SAN_DNS_NAMES, - SAN_IP_ADDRESSES, -} from "@devcontainer-dev-certs/shared"; diff --git a/src/vscode-ui-extension/src/cert/types.ts b/src/vscode-ui-extension/src/cert/types.ts deleted file mode 100644 index 3df525b..0000000 --- a/src/vscode-ui-extension/src/cert/types.ts +++ /dev/null @@ -1,6 +0,0 @@ -// Re-export shim: the canonical home for DevCert / DevKey is now -// `@devcontainer-dev-certs/shared`. Keeping this thin re-export so existing -// `./cert/types` imports across the UI extension (and its test suite) keep -// resolving without a sweeping rename. -export { DevCert, DevKey } from "@devcontainer-dev-certs/shared"; -export type { DevKeyAlgorithm } from "@devcontainer-dev-certs/shared"; diff --git a/src/vscode-ui-extension/src/certProvider.ts b/src/vscode-ui-extension/src/certProvider.ts index 1f6e945..a34fe96 100644 --- a/src/vscode-ui-extension/src/certProvider.ts +++ b/src/vscode-ui-extension/src/certProvider.ts @@ -2,17 +2,19 @@ import * as fs from "fs"; import * as os from "os"; import * as path from "path"; import * as vscode from "vscode"; -import { type CertManager } from "./cert/manager"; -import { exportLoadedCert } from "./cert/exporter"; -import { loadPemPair, loadPfx } from "./cert/loader"; -import type { LoadedCert } from "./cert/loader"; -import { buildPfx } from "./cert/pfx"; import { + exportLoadedCert, + loadPemPair, + loadPfx, + buildPfx, assertValidCertName, log, selectBackend, + DOTNET_DEV_CERT_NAME, } from "@devcontainer-dev-certs/shared"; import type { + CertManager, + LoadedCert, BackendMode, CertBundle, CertBundleV3, @@ -22,7 +24,6 @@ import type { DefaultKestrelCertSelection, LinuxNssTrustReporter, } from "@devcontainer-dev-certs/shared"; -import { DOTNET_DEV_CERT_NAME } from "@devcontainer-dev-certs/shared"; export interface UserCertificateConfig { name: string; @@ -148,12 +149,6 @@ export class CertProvider { : { certs: v3Certs }; } - clearCache(): void { - this.cachedDotNet = null; - this.cachedUser.clear(); - this.warnedExpiredCerts.clear(); - } - private async collect( args: GetAllCertMaterialArgs ): Promise { diff --git a/src/vscode-ui-extension/src/containerCertAccept.ts b/src/vscode-ui-extension/src/containerCertAccept.ts index 370b46e..144dc3a 100644 --- a/src/vscode-ui-extension/src/containerCertAccept.ts +++ b/src/vscode-ui-extension/src/containerCertAccept.ts @@ -2,9 +2,10 @@ import { DevCert, isValidDevCert, log, + validateLeafTrustShape, validateLocalSans, - type NonLocalSanEntry, } from "@devcontainer-dev-certs/shared"; +import type { NonLocalSanEntry } from "@devcontainer-dev-certs/shared"; /** * Wire-protocol payload sent by the workspace extension when it scans the @@ -31,11 +32,61 @@ export interface AcceptedContainerCert { thumbprint: string; } +/** + * Persisted answer to "may Dev Containers put their own dev certs in this + * host's trust store?". + * + * Tri-state rather than a boolean because the previous shape could only ever + * record *yes*: an accept was written to global state forever, while a + * decline wrote nothing, so the prompt returned on every single activation. + * The only durable state a security prompt could reach was the permissive + * one — a one-way ratchet — and the sole way to stop being asked was to edit + * `devcontainer.json` and rebuild, or to disable host-side generation too. + * + * The decision is deliberately host-wide rather than per-certificate. Unless + * a container bakes its dev cert into the image or mounts a volume for + * `~/.dotnet/corefx/cryptography/x509stores/my/`, it mints a fresh one on + * every rebuild — so a per-thumbprint memory would re-prompt on every + * rebuild, which is the shape of consent people learn to click through + * without reading. + */ +export type ContainerCertConsent = "granted" | "denied" | "unset"; + +/** + * What the user did with the consent modal. + * + * `dismiss` (Escape / Cancel) is distinct from `never` on purpose: dismissing + * declines this push without recording anything, so an accidental Escape + * doesn't silently turn the feature off forever. Only an explicit `never` + * persists a denial. + */ +export type ContainerCertConsentChoice = "trust" | "never" | "dismiss"; + export type AcceptContainerCertRejectReason = | "host-setting-disabled" | "user-declined" | "parse-failed" + // The cert parsed and validated; establishing trust on the host failed. + // Kept distinct from `parse-failed` so the container can tell the user + // something true — the two have entirely different remedies. + | "trust-failed" | "not-valid-dev-cert" + /** + * basicConstraints says cA=TRUE, or is absent so we can't tell. Trusting + * it would put an issuing CA in the host's root store, which the SAN-local + * restriction cannot constrain — a CA's own SANs say nothing about what it + * may issue for. + */ + | "not-a-leaf-cert" + /** No extendedKeyUsage, anyExtendedKeyUsage, or no id-kp-serverAuth. */ + | "unsupported-eku" + /** + * The SAN set is structurally unusable (absent, undecodable, empty, or + * carrying a GeneralName type other than dNSName / iPAddress) — distinct + * from `non-local-sans`, and NOT overridable by + * `allowNonLocalContainerCertSans`. + */ + | "malformed-sans" | "non-local-sans"; export interface AcceptContainerCertResult { @@ -66,15 +117,15 @@ export interface AcceptContainerCertDeps { autoProvision: boolean; /** `devcontainerDevCerts.allowNonLocalContainerCertSans` host setting. */ allowNonLocalSans: boolean; - /** True iff the user has previously consented to container-cert sync. */ - hasConsent: () => boolean; - /** Persist consent for future pushes. */ - recordConsent: () => Promise; - /** Show the modal consent prompt. Returns true iff the user accepted. */ + /** The persisted host-wide decision, or `unset` if never answered. */ + readConsent: () => ContainerCertConsent; + /** Persist the host-wide decision. */ + recordConsent: (decision: "granted" | "denied") => Promise; + /** Show the modal consent prompt and report which action the user took. */ promptUser: ( cert: AcceptedContainerCert, nonLocalSansOverridden: NonLocalSanEntry[] - ) => Promise; + ) => Promise; /** * Trust the supplied cert in the host's OS trust store. The * implementation is the same code path the host-generation flow uses, @@ -100,18 +151,34 @@ export interface AcceptContainerCertDeps { * 3. Validate it actually is an ASP.NET dev cert (CN, validity, OID, * version) — independent of whatever the workspace asserted. * Failure → `not-valid-dev-cert`. - * 4. SAN-local restriction. Unless the - * `allowNonLocalContainerCertSans` override is on, any SAN entry - * outside well-known local scopes (see validateLocalSans) rejects - * the cert with `non-local-sans`. Defends against a malicious or - * misconfigured container tricking the host into trusting a cert - * valid for arbitrary domains. - * 5. Modal consent prompt (one-time, gated on `containerCertProvisionConsented` - * in extension global state — distinct from the host-generation - * consent because the user is approving trust of a cert that came - * from a container they may or may not control). Declining → + * 4. Trust-anchor shape (`validateLeafTrustShape`). The cert must be a + * leaf (basicConstraints present, cA=FALSE) and scoped to server + * auth (EKU present, includes serverAuth, not anyExtendedKeyUsage). + * Failure → `not-a-leaf-cert` / `unsupported-eku`. This gates step 5 + * rather than sitting beside it: step 5 asks what names the cert + * covers, which only constrains anything for a cert that can + * authenticate ONLY itself. A CA's own SANs place no limit on what + * it may issue, so without this check the SAN restriction is + * bypassed by pushing a CA with `localhost` SANs and then signing a + * leaf for any name at all. + * 5. SAN-local restriction. A structurally unusable SAN set (absent, + * undecodable, empty, or carrying a GeneralName type other than + * dNSName / iPAddress) rejects with `malformed-sans` and is NOT + * overridable. Otherwise, unless the `allowNonLocalContainerCertSans` + * override is on, any dNSName / iPAddress outside well-known local + * scopes (see validateLocalSans) rejects with `non-local-sans`. + * Together these defend against a malicious or misconfigured + * container tricking the host into trusting a cert valid for + * arbitrary domains. + * 6. Host-wide consent (`containerCertProvisionConsented` in extension + * global state — distinct from the host-generation consent because + * the user is approving trust of a cert that came from a container + * they may or may not control). `denied` short-circuits here without + * prompting; `unset` shows the modal, whose three outcomes are Trust + * (record `granted`), Never (record `denied`), and dismiss (decline + * this push, record nothing). All three non-Trust paths → * `user-declined`. - * 6. Trust the cert in the host platform store. Public-cert-only: + * 7. Trust the cert in the host platform store. Public-cert-only: * writes the cert to the OS trust surfaces (.NET Root / OpenSSL * trust dir / NSS / login keychain / CurrentUser-Root) but NEVER * to a my-store location and NEVER with a private key — the host @@ -211,7 +278,60 @@ async function acceptContainerDevCertInner( return { accepted: false, reason: "not-valid-dev-cert" }; } + // Shape before scope. A CA cert can be perfectly "local" by its own SANs + // and still issue a leaf for any name it likes, so this has to gate the + // SAN check rather than sit beside it — and its rejection is the more + // useful one to surface when a cert fails both. + const shape = validateLeafTrustShape(parsed.cert); + if (!shape.ok) { + const suffix = shape.detail ? ` (${shape.detail})` : ""; + if ( + shape.reason === "is-certificate-authority" || + shape.reason === "missing-basic-constraints" + ) { + log( + `acceptContainerDevCert: rejected ${parsed.thumbprint} — ${shape.reason}${suffix}. ` + + `Trusting it would install an issuing CA in this host's root store; the SAN-local ` + + `restriction cannot constrain what a CA signs.` + ); + return { + accepted: false, + reason: "not-a-leaf-cert", + detail: shape.reason, + }; + } + if (shape.reason === "unreadable") { + log( + `acceptContainerDevCert: rejected ${parsed.thumbprint} — could not read basicConstraints/EKU${suffix}.` + ); + return { accepted: false, reason: "parse-failed", detail: shape.detail }; + } + log( + `acceptContainerDevCert: rejected ${parsed.thumbprint} — ${shape.reason}${suffix}. ` + + `A dev cert this host will trust must be scoped to server authentication.` + ); + return { + accepted: false, + reason: "unsupported-eku", + detail: shape.detail ? `${shape.reason}: ${shape.detail}` : shape.reason, + }; + } + const sanResult = validateLocalSans(parsed.cert); + // Structural SAN failures are NOT scope decisions, so + // `allowNonLocalContainerCertSans` deliberately does not override them. + // That setting exists so a user can say "yes, I really do mean to trust + // this cert for that name" — it can't mean anything about a cert whose + // names we were unable to read in the first place. + if (!sanResult.ok && sanResult.reason !== "non-local") { + const detail = sanResult.detail + ? `${sanResult.reason}: ${sanResult.detail}` + : sanResult.reason; + log( + `acceptContainerDevCert: rejected ${parsed.thumbprint} — unusable SAN set (${detail}).` + ); + return { accepted: false, reason: "malformed-sans", detail }; + } if (!sanResult.ok && !deps.allowNonLocalSans) { const detail = sanResult.nonLocalEntries .map((e) => `${e.type}:${e.value}`) @@ -231,27 +351,66 @@ async function acceptContainerDevCertInner( ); } - const needsConsent = !deps.hasConsent(); - if (needsConsent) { - const consented = await deps.promptUser(parsed, nonLocalOverridden); - if (!consented) { + const consent = deps.readConsent(); + + // A standing denial short-circuits before the prompt, so declining once + // actually stops the asking instead of re-showing the modal on every + // container activation. + if (consent === "denied") { + log( + `acceptContainerDevCert: declining ${parsed.thumbprint} — the user previously chose ` + + `not to trust container certificates on this host. Run "Dev Certs: Reset Container ` + + `Certificate Consent" from the Command Palette to be asked again.` + ); + return { + accepted: false, + reason: "user-declined", + detail: "previously declined for this host", + }; + } + + if (consent === "unset") { + const choice = await deps.promptUser(parsed, nonLocalOverridden); + if (choice === "never") { + // Persisted immediately: unlike the grant below there is no trust step + // that could fail and leave the decision half-applied. + await deps.recordConsent("denied"); + log( + `acceptContainerDevCert: user declined trusting ${parsed.thumbprint} and asked not to be prompted again.` + ); + return { accepted: false, reason: "user-declined" }; + } + if (choice === "dismiss") { log( - `acceptContainerDevCert: user declined trusting ${parsed.thumbprint}.` + `acceptContainerDevCert: user dismissed the prompt for ${parsed.thumbprint}; not recording a decision.` ); return { accepted: false, reason: "user-declined" }; } } - // Run the trust step BEFORE persisting consent. If trustCertificate - // throws (macOS keychain dialog cancelled, NSS DB not writable, etc.) - // we let the outer try/catch convert it to `parse-failed` — and - // crucially the consent stays UN-persisted, so the next push retries - // the modal prompt with the same fresh state instead of silently - // re-trying trust without UX. - await deps.trustCertificate(parsed); + // Run the trust step BEFORE persisting consent, so that when it fails the + // consent stays UN-persisted and the next push retries the modal prompt + // with the same fresh state instead of silently re-trying trust without UX. + // + // Caught here rather than left to the outer handler: the certificate has + // already parsed and validated by this point, so a failure now is an + // install/trust failure, not a parse failure. Letting it fall through + // reported a cancelled macOS keychain dialog — or an `ensureHashSymlink` + // slot exhaustion, which throws as of this branch — to the user as "the + // host could not parse the certificate", which is both wrong and + // un-actionable. + try { + await deps.trustCertificate(parsed); + } catch (err: unknown) { + const message = err instanceof Error ? err.message : String(err); + log( + `acceptContainerDevCert: trusting ${parsed.thumbprint} on host failed: ${message}` + ); + return { accepted: false, reason: "trust-failed", detail: message }; + } - if (needsConsent) { - await deps.recordConsent(); + if (consent === "unset") { + await deps.recordConsent("granted"); } log(`acceptContainerDevCert: ${parsed.thumbprint} trusted on host.`); diff --git a/src/vscode-ui-extension/src/extension.ts b/src/vscode-ui-extension/src/extension.ts index 395385c..4555489 100644 --- a/src/vscode-ui-extension/src/extension.ts +++ b/src/vscode-ui-extension/src/extension.ts @@ -3,31 +3,55 @@ import * as fs from "fs"; import * as path from "path"; import * as vscode from "vscode"; import { getRenamedSetting } from "./settings"; -import { CertManager } from "./cert/manager"; -import { CertProvider } from "./certProvider"; -import type { GetAllCertMaterialArgs } from "./certProvider"; -import { - acceptContainerDevCert, - type AcceptContainerCertPayload, - type AcceptContainerCertResult, - type AcceptedContainerCert, -} from "./containerCertAccept"; -import { trustInNss } from "./platform/nssTrust"; import { + CertManager, + trustInNss, log, getOpenSslTrustDir, getPemFileName, - type NonLocalSanEntry, } from "@devcontainer-dev-certs/shared"; -import { initLogger } from "@devcontainer-dev-certs/shared/src/loggerVscode"; import type { + NonLocalSanEntry, CertBundle, CertBundleV3, LinuxNssTrustReporter, } from "@devcontainer-dev-certs/shared"; +import { CertProvider } from "./certProvider"; +import type { GetAllCertMaterialArgs } from "./certProvider"; +import { acceptContainerDevCert, type AcceptContainerCertPayload, type AcceptContainerCertResult, type AcceptedContainerCert, type ContainerCertConsent, type ContainerCertConsentChoice, } from "./containerCertAccept"; +import { initLogger } from "@devcontainer-dev-certs/shared/src/loggerVscode"; const CONTAINER_CERT_CONSENT_KEY = "containerCertProvisionConsented"; +/** + * Map whatever is sitting in global state onto the tri-state decision. + * + * Migrates the historical shape: this key used to hold a boolean that could + * only ever be `true` (an accept), so an existing consenter must read back as + * `granted` and never be re-prompted. Anything unrecognized — a stale value, a + * hand-edited state file — falls back to `unset`, which asks the user rather + * than silently opting them into or out of the feature. + * + * Exported for testing: the legacy-boolean path is the one that decides + * whether upgrading users get an unexpected modal, and it deserves a direct + * assertion rather than one mediated by a fake ExtensionContext. + */ +export function normalizeContainerCertConsent( + stored: unknown +): ContainerCertConsent { + if (stored === true || stored === "granted") return "granted"; + if (stored === "denied") return "denied"; + return "unset"; +} + +function readContainerCertConsent( + context: vscode.ExtensionContext +): ContainerCertConsent { + return normalizeContainerCertConsent( + context.globalState.get(CONTAINER_CERT_CONSENT_KEY) + ); +} + export function activate(context: vscode.ExtensionContext): void { context.subscriptions.push(initLogger("Dev Container Dev Certs")); @@ -207,14 +231,10 @@ export function activate(context: vscode.ExtensionContext): void { generateDotNetCert, autoProvision, allowNonLocalSans, - hasConsent: () => - context.globalState.get( - CONTAINER_CERT_CONSENT_KEY, - false - ), - recordConsent: () => + readConsent: () => readContainerCertConsent(context), + recordConsent: (decision) => Promise.resolve( - context.globalState.update(CONTAINER_CERT_CONSENT_KEY, true) + context.globalState.update(CONTAINER_CERT_CONSENT_KEY, decision) ).then(() => undefined), promptUser: (cert, nonLocal) => promptForContainerCertConsent(cert, nonLocal), @@ -228,6 +248,34 @@ export function activate(context: vscode.ExtensionContext): void { ) ); + // Undo a standing container-cert decision. Without this, "Never" would be + // a one-way ratchet in the opposite direction from the one it fixes — + // reachable only by clearing the extension's global state by hand. Also + // lets a user who granted consent stop future silent acceptances. + context.subscriptions.push( + vscode.commands.registerCommand( + "devcontainer-dev-certs.resetContainerCertConsent", + async () => { + const previous = readContainerCertConsent(context); + await context.globalState.update(CONTAINER_CERT_CONSENT_KEY, undefined); + log(`Container certificate consent reset (was: ${previous}).`); + if (previous === "unset") { + vscode.window.showInformationMessage( + vscode.l10n.t( + "Dev Certs: No container certificate decision was recorded; you will be asked the next time a Dev Container offers one." + ) + ); + return; + } + vscode.window.showInformationMessage( + vscode.l10n.t( + "Dev Certs: Container certificate consent reset. You will be asked again the next time a Dev Container offers one. Certificates already trusted on this host are not removed." + ) + ); + } + ) + ); + // Trust the dev certificate in browser NSS databases (Linux) context.subscriptions.push( vscode.commands.registerCommand( @@ -340,8 +388,9 @@ async function promptForCertConsent(): Promise { async function promptForContainerCertConsent( cert: AcceptedContainerCert, nonLocalSans: NonLocalSanEntry[] -): Promise { +): Promise { const trust = vscode.l10n.t("Trust"); + const never = vscode.l10n.t("Never Trust Container Certificates"); const platformDetail = process.platform === "darwin" ? vscode.l10n.t( @@ -384,17 +433,24 @@ async function promptForContainerCertConsent( sections.push( vscode.l10n.t( - "Declining skips trusting this certificate. To permanently disable this flow, set devcontainerDevCerts.generateDotNetCert to false (which also disables host-side generation)." + "Cancel skips this certificate and asks again next time. 'Never Trust Container Certificates' declines for this host permanently — no further prompts, and no container certificate is trusted — and can be undone with 'Dev Certs: Reset Container Certificate Consent' from the Command Palette." ) ); const detail = sections.join("\n\n"); + // Three outcomes, not two. Cancel/Escape has to stay separable from an + // explicit refusal: a dismissed dialog declines this one push and records + // nothing, so a stray Escape can't silently disable the feature forever, + // while "Never" is the durable answer that actually stops the prompting. const choice = await vscode.window.showInformationMessage( message, { modal: true, detail }, - trust + trust, + never ); - return choice === trust; + if (choice === trust) return "trust"; + if (choice === never) return "never"; + return "dismiss"; } export interface ResolveDotnetProvisioningDeps { diff --git a/src/vscode-ui-extension/src/platform/baseStore.ts b/src/vscode-ui-extension/src/platform/baseStore.ts deleted file mode 100644 index c64a6da..0000000 --- a/src/vscode-ui-extension/src/platform/baseStore.ts +++ /dev/null @@ -1,18 +0,0 @@ -// Re-export shim: the canonical home for the localized platform classifier -// wrappers and `BaseCertificateStore` is now in -// `@devcontainer-dev-certs/shared`. Imports go through the submodule path -// (rather than the barrel) because the barrel aliases the platform-flavored -// `classifyCandidate` / `selectBestDevCert` to disambiguate from the pure -// classifier; existing tests and call sites expect the unaliased names. -export { - BaseCertificateStore, - classifyCandidate, - selectBestDevCert, - extractThumbprintHintFromFilename, -} from "@devcontainer-dev-certs/shared/src/platform/baseStore"; -export type { - ClassifiedCandidate, - CandidateInput, - UsableDevCert, - ClassifyOptions, -} from "@devcontainer-dev-certs/shared/src/platform/baseStore"; diff --git a/src/vscode-ui-extension/src/platform/linuxStore.ts b/src/vscode-ui-extension/src/platform/linuxStore.ts deleted file mode 100644 index dac9222..0000000 --- a/src/vscode-ui-extension/src/platform/linuxStore.ts +++ /dev/null @@ -1,3 +0,0 @@ -// Re-export shim: canonical home is `@devcontainer-dev-certs/shared`. -export { LinuxCertificateStore } from "@devcontainer-dev-certs/shared"; -export type { LinuxCertificateStoreOptions } from "@devcontainer-dev-certs/shared"; diff --git a/src/vscode-ui-extension/src/platform/macStore.ts b/src/vscode-ui-extension/src/platform/macStore.ts deleted file mode 100644 index d7f2551..0000000 --- a/src/vscode-ui-extension/src/platform/macStore.ts +++ /dev/null @@ -1,2 +0,0 @@ -// Re-export shim: canonical home is `@devcontainer-dev-certs/shared`. -export { MacCertificateStore } from "@devcontainer-dev-certs/shared"; diff --git a/src/vscode-ui-extension/src/platform/nssTrust.ts b/src/vscode-ui-extension/src/platform/nssTrust.ts deleted file mode 100644 index dba495a..0000000 --- a/src/vscode-ui-extension/src/platform/nssTrust.ts +++ /dev/null @@ -1,3 +0,0 @@ -// Re-export shim: canonical home is `@devcontainer-dev-certs/shared`. -export { trustInNss } from "@devcontainer-dev-certs/shared"; -export type { NssTrustResult } from "@devcontainer-dev-certs/shared"; diff --git a/src/vscode-ui-extension/src/platform/processUtil.ts b/src/vscode-ui-extension/src/platform/processUtil.ts deleted file mode 100644 index df7686c..0000000 --- a/src/vscode-ui-extension/src/platform/processUtil.ts +++ /dev/null @@ -1,3 +0,0 @@ -// Re-export shim: canonical home is `@devcontainer-dev-certs/shared`. -export { runProcess } from "@devcontainer-dev-certs/shared"; -export type { ProcessResult } from "@devcontainer-dev-certs/shared"; diff --git a/src/vscode-ui-extension/src/platform/types.ts b/src/vscode-ui-extension/src/platform/types.ts deleted file mode 100644 index 12314f6..0000000 --- a/src/vscode-ui-extension/src/platform/types.ts +++ /dev/null @@ -1,12 +0,0 @@ -// Re-export shim: the canonical home for the platform store types and the -// `createPlatformStore` factory is now `@devcontainer-dev-certs/shared`. -// Keeping this thin re-export so existing `./platform/types` imports across -// the UI extension and its test suite keep resolving without a rename. -export type { - LinuxNssTrustReporter, - BaseStoreOptions, - CreatePlatformStoreOptions, - CertificateStatus, - PlatformCertificateStore, -} from "@devcontainer-dev-certs/shared"; -export { createPlatformStore } from "@devcontainer-dev-certs/shared"; diff --git a/src/vscode-ui-extension/src/platform/windowsStore.ts b/src/vscode-ui-extension/src/platform/windowsStore.ts deleted file mode 100644 index f3ab097..0000000 --- a/src/vscode-ui-extension/src/platform/windowsStore.ts +++ /dev/null @@ -1,9 +0,0 @@ -// Re-export shim: canonical home is `@devcontainer-dev-certs/shared`. -export { WindowsCertificateStore } from "@devcontainer-dev-certs/shared"; -export type { - WindowsStoreLocation, - PsCandidate, - PsSkipped, - PsSkipReason, - PsEnumeration, -} from "@devcontainer-dev-certs/shared"; diff --git a/src/vscode-ui-extension/tests/certProvider.test.ts b/src/vscode-ui-extension/tests/certProvider.test.ts index c90a5e6..05ef1db 100644 --- a/src/vscode-ui-extension/tests/certProvider.test.ts +++ b/src/vscode-ui-extension/tests/certProvider.test.ts @@ -4,18 +4,19 @@ import * as os from "os"; import * as path from "path"; import { CertProvider } from "../src/certProvider"; import type { UserCertificateConfig } from "../src/certProvider"; -import { exportPem } from "../src/cert/exporter"; -import { generateCertificate } from "../src/cert/generator"; -import { buildPfx, parsePfx } from "../src/cert/pfx"; -import { VALIDITY_DAYS } from "../src/cert/properties"; -import type { CertManager } from "../src/cert/manager"; -import { type DevCert, type DevKey } from "../src/cert/types"; import { - __resetConfig, - __setConfig, - errorMessages, - warningMessages, -} from "./__mocks__/vscode"; + exportPem, + generateCertificate, + buildPfx, + parsePfx, + VALIDITY_DAYS, +} from "@devcontainer-dev-certs/shared"; +import type { + CertManager, + DevCert, + DevKey, +} from "@devcontainer-dev-certs/shared"; +import { __resetConfig, __setConfig, errorMessages, warningMessages, } from "./__mocks__/vscode"; async function makeValidCert(): ReturnType { const now = new Date(); diff --git a/src/vscode-ui-extension/tests/classifyCandidate.test.ts b/src/vscode-ui-extension/tests/classifyCandidate.test.ts index 769d45c..48b2f04 100644 --- a/src/vscode-ui-extension/tests/classifyCandidate.test.ts +++ b/src/vscode-ui-extension/tests/classifyCandidate.test.ts @@ -1,9 +1,14 @@ import { describe, it, expect, beforeEach } from "vitest"; import { initLogger } from "@devcontainer-dev-certs/shared/src/loggerVscode"; import { logMessages } from "./__mocks__/vscode"; -import { classifyCandidate } from "../src/platform/baseStore"; -import { generateCertificate } from "../src/cert/generator"; -import { DevKey, DevCert } from "../src/cert/types"; +import { + classifyCandidate, +} from "@devcontainer-dev-certs/shared/src/platform/baseStore"; +import { + generateCertificate, + DevKey, + DevCert, +} from "@devcontainer-dev-certs/shared"; import { X509CertificateGenerator, cryptoProvider } from "@peculiar/x509"; import { webcrypto } from "node:crypto"; diff --git a/src/vscode-ui-extension/tests/containerCertAccept.test.ts b/src/vscode-ui-extension/tests/containerCertAccept.test.ts index 1332907..99ed8d2 100644 --- a/src/vscode-ui-extension/tests/containerCertAccept.test.ts +++ b/src/vscode-ui-extension/tests/containerCertAccept.test.ts @@ -1,24 +1,18 @@ import { describe, it, expect, beforeEach, vi, type Mock } from "vitest"; -import { - Extension, - SubjectAlternativeNameExtension, - X509CertificateGenerator, - cryptoProvider, -} from "@peculiar/x509"; +import { BasicConstraintsExtension, ExtendedKeyUsage, ExtendedKeyUsageExtension, Extension, SubjectAlternativeNameExtension, X509CertificateGenerator, cryptoProvider, } from "@peculiar/x509"; import { webcrypto } from "node:crypto"; import { DevCert, + generateCertificate, ASPNET_HTTPS_OID, CURRENT_CERTIFICATE_VERSION, SAN_DNS_NAMES, SAN_IP_ADDRESSES, + VALIDITY_DAYS, } from "@devcontainer-dev-certs/shared"; import { initLogger } from "@devcontainer-dev-certs/shared/src/loggerVscode"; -import { - acceptContainerDevCert, - type AcceptContainerCertDeps, - type AcceptContainerCertPayload, -} from "../src/containerCertAccept"; +import { acceptContainerDevCert, type AcceptContainerCertDeps, type AcceptContainerCertPayload, } from "../src/containerCertAccept"; +import { normalizeContainerCertConsent } from "../src/extension"; cryptoProvider.set(webcrypto as unknown as Crypto); initLogger("test"); @@ -29,7 +23,14 @@ interface MakeDevCertOptions { notBefore?: Date; notAfter?: Date; sans?: { type: "dns" | "ip"; value: string }[]; + omitSanExtension?: boolean; omitOid?: boolean; + /** basicConstraints cA value. Default false (a leaf), like a real dev cert. */ + ca?: boolean; + /** Drop basicConstraints entirely. */ + omitBasicConstraints?: boolean; + /** EKU OIDs. Default [serverAuth]. `null` drops the extension. */ + eku?: string[] | null; } /** @@ -56,9 +57,27 @@ async function makeDevPem( ...SAN_DNS_NAMES.map((d) => ({ type: "dns" as const, value: d })), ...SAN_IP_ADDRESSES.map((ip) => ({ type: "ip" as const, value: ip })), ]; - const extensions: Extension[] = [ - new SubjectAlternativeNameExtension(sans, true), - ]; + // Default to the shape a genuine ASP.NET dev cert has: a leaf + // (basicConstraints cA=FALSE, critical) scoped to server authentication. + // Both are required by `validateLeafTrustShape`, so the happy-path fixture + // has to carry them or every test would exercise a rejection. + const extensions: Extension[] = []; + if (!opts.omitSanExtension) { + extensions.push(new SubjectAlternativeNameExtension(sans, true)); + } + if (!opts.omitBasicConstraints) { + extensions.push( + new BasicConstraintsExtension(opts.ca ?? false, undefined, true) + ); + } + if (opts.eku !== null) { + extensions.push( + new ExtendedKeyUsageExtension( + opts.eku ?? [ExtendedKeyUsage.serverAuth], + true + ) + ); + } if (!opts.omitOid) { extensions.push( new Extension( @@ -109,7 +128,7 @@ function makeDeps( vi.fn(async () => undefined); const promptUser = overrides.promptUser ?? - vi.fn(async () => true); + vi.fn(async () => "trust"); const recordConsent = overrides.recordConsent ?? vi.fn(async () => undefined); @@ -117,7 +136,7 @@ function makeDeps( generateDotNetCert: true, autoProvision: true, allowNonLocalSans: false, - hasConsent: () => false, + readConsent: () => "unset" as const, ...overrides, trustCertificate, promptUser, @@ -282,9 +301,9 @@ describe("acceptContainerDevCert", () => { expect(deps.trustCertificate).toHaveBeenCalledTimes(1); }); - it("skips the modal when consent was previously recorded", async () => { + it("skips the modal when consent was previously granted", async () => { const { pemCertBase64, thumbprint } = await makeDevPem(); - const deps = makeDeps({ hasConsent: () => true }); + const deps = makeDeps({ readConsent: () => "granted" }); const result = await acceptContainerDevCert( { pemCertBase64, thumbprint }, deps @@ -295,9 +314,11 @@ describe("acceptContainerDevCert", () => { expect(deps.trustCertificate).toHaveBeenCalledTimes(1); }); - it("returns user-declined when the prompt is dismissed", async () => { + it("returns user-declined and records NOTHING when the prompt is dismissed", async () => { + // Cancel / Escape declines this push only. Recording a denial here would + // let a stray keystroke disable the feature for good. const { pemCertBase64, thumbprint } = await makeDevPem(); - const deps = makeDeps({ promptUser: vi.fn(async () => false) }); + const deps = makeDeps({ promptUser: vi.fn(async () => "dismiss") }); const result = await acceptContainerDevCert( { pemCertBase64, thumbprint }, deps @@ -307,6 +328,45 @@ describe("acceptContainerDevCert", () => { expect(deps.recordConsent).not.toHaveBeenCalled(); }); + it("records a denial when the user picks Never", async () => { + const { pemCertBase64, thumbprint } = await makeDevPem(); + const deps = makeDeps({ promptUser: vi.fn(async () => "never") }); + const result = await acceptContainerDevCert( + { pemCertBase64, thumbprint }, + deps + ); + expect(result).toEqual({ accepted: false, reason: "user-declined" }); + expect(deps.recordConsent).toHaveBeenCalledWith("denied"); + expect(deps.trustCertificate).not.toHaveBeenCalled(); + }); + + it("short-circuits a standing denial without re-prompting", async () => { + // The defect this fixes: an accept persisted forever while a decline + // persisted nothing, so the only durable state the prompt could reach was + // the permissive one and declining meant being asked again on every + // single container activation. + const { pemCertBase64, thumbprint } = await makeDevPem(); + const deps = makeDeps({ readConsent: () => "denied" }); + const result = await acceptContainerDevCert( + { pemCertBase64, thumbprint }, + deps + ); + expect(result.accepted).toBe(false); + expect(result.reason).toBe("user-declined"); + expect(deps.promptUser).not.toHaveBeenCalled(); + expect(deps.trustCertificate).not.toHaveBeenCalled(); + expect(deps.recordConsent).not.toHaveBeenCalled(); + }); + + it("records the grant as \"granted\", not a bare boolean", async () => { + // The stored value is read back through a migration that maps legacy + // `true` to "granted"; writing a boolean again would defeat the tri-state. + const { pemCertBase64, thumbprint } = await makeDevPem(); + const deps = makeDeps(); + await acceptContainerDevCert({ pemCertBase64, thumbprint }, deps); + expect(deps.recordConsent).toHaveBeenCalledWith("granted"); + }); + it("repeat pushes of the same cert call trustCertificate twice at the handler level — idempotency is enforced one layer down in CertManager.trustExternalCertificate via store.isCertTrusted", async () => { // The handler does NOT query the platform store for an already- // trusted thumbprint itself. The downstream `trustCertificate` @@ -317,7 +377,7 @@ describe("acceptContainerDevCert", () => { // invoked twice — the dep itself decides whether to make a real // platform-trust call. const { pemCertBase64, thumbprint } = await makeDevPem(); - const deps = makeDeps({ hasConsent: () => true }); + const deps = makeDeps({ readConsent: () => "granted" }); const first = await acceptContainerDevCert( { pemCertBase64, thumbprint }, deps @@ -372,13 +432,37 @@ describe("acceptContainerDevCert", () => { { pemCertBase64, thumbprint }, deps ); - // The outer try/catch maps the throw to parse-failed. expect(result.accepted).toBe(false); - expect(result.reason).toBe("parse-failed"); + // Reported as a trust failure, not a parse failure: the cert parsed and + // validated, so telling the user it was unreadable would be false and + // point them at the wrong remedy. + expect(result.reason).toBe("trust-failed"); + expect(result.detail).toContain("simulated trust failure"); expect(deps.promptUser).toHaveBeenCalledTimes(1); // KEY ASSERTION: consent was NOT persisted. expect(deps.recordConsent).not.toHaveBeenCalled(); }); + + it("reports trust-failed for an already-consented push whose trust step fails", async () => { + // The consented path skips the modal entirely, so this is the branch a + // returning user hits — e.g. `ensureHashSymlink` throwing on slot + // exhaustion, which this branch made a thrown error rather than a + // silent no-op. It must not surface as "could not parse". + const { pemCertBase64, thumbprint } = await makeDevPem(); + const deps = makeDeps({ + readConsent: () => "granted" as const, + trustCertificate: vi.fn(async () => { + throw new Error("no free hash slot"); + }), + }); + const result = await acceptContainerDevCert( + { pemCertBase64, thumbprint }, + deps + ); + expect(result.accepted).toBe(false); + expect(result.reason).toBe("trust-failed"); + expect(deps.promptUser).not.toHaveBeenCalled(); + }); }); describe("acceptContainerDevCert end-to-end against a generated dev cert", () => { @@ -396,3 +480,203 @@ describe("acceptContainerDevCert end-to-end against a generated dev cert", () => expect(trusted.thumbprint).toBe(thumbprint); }); }); + +/** + * The SAN-local restriction only constrains a certificate that can + * authenticate ONLY itself. A CA's own SANs place no limit on what it may + * issue, so a CA with `localhost` SANs sails through `validateLocalSans` and + * — once the host puts it in `CurrentUser\Root` / the login keychain / the + * OpenSSL CApath / NSS with the `C` flag — can mint a leaf for any name it + * likes. These tests pin the gate that closes that. + */ +describe("acceptContainerDevCert trust-anchor shape gate", () => { + it("refuses a CA certificate even when every SAN is local", async () => { + const { pemCertBase64, thumbprint } = await makeDevPem({ ca: true }); + const deps = makeDeps(); + const result = await acceptContainerDevCert( + { pemCertBase64, thumbprint }, + deps + ); + + expect(result.accepted).toBe(false); + expect(result.reason).toBe("not-a-leaf-cert"); + expect(deps.trustCertificate).not.toHaveBeenCalled(); + expect(deps.promptUser).not.toHaveBeenCalled(); + }); + + it("refuses a CA certificate even with allowNonLocalContainerCertSans on", async () => { + // The override relaxes SAN *scope*. It must not be readable as "trust + // whatever this container sends" — a CA is a different question entirely. + const { pemCertBase64, thumbprint } = await makeDevPem({ ca: true }); + const deps = makeDeps({ allowNonLocalSans: true }); + const result = await acceptContainerDevCert( + { pemCertBase64, thumbprint }, + deps + ); + + expect(result.accepted).toBe(false); + expect(result.reason).toBe("not-a-leaf-cert"); + expect(deps.trustCertificate).not.toHaveBeenCalled(); + }); + + it("refuses a certificate with no basicConstraints (cA is unanswerable)", async () => { + const { pemCertBase64, thumbprint } = await makeDevPem({ + omitBasicConstraints: true, + }); + const deps = makeDeps(); + const result = await acceptContainerDevCert( + { pemCertBase64, thumbprint }, + deps + ); + + expect(result.accepted).toBe(false); + expect(result.reason).toBe("not-a-leaf-cert"); + expect(deps.trustCertificate).not.toHaveBeenCalled(); + }); + + it("refuses a certificate with no extendedKeyUsage", async () => { + // Absent EKU reads as "any purpose"; Windows `certutil -addstore Root` + // applies no policy constraint of its own, so the cert would be trusted + // well beyond TLS. + const { pemCertBase64, thumbprint } = await makeDevPem({ eku: null }); + const deps = makeDeps(); + const result = await acceptContainerDevCert( + { pemCertBase64, thumbprint }, + deps + ); + + expect(result.accepted).toBe(false); + expect(result.reason).toBe("unsupported-eku"); + expect(deps.trustCertificate).not.toHaveBeenCalled(); + }); + + it("refuses anyExtendedKeyUsage, which re-opens 'any purpose'", async () => { + const { pemCertBase64, thumbprint } = await makeDevPem({ + eku: ["2.5.29.37.0"], + }); + const deps = makeDeps(); + const result = await acceptContainerDevCert( + { pemCertBase64, thumbprint }, + deps + ); + + expect(result.accepted).toBe(false); + expect(result.reason).toBe("unsupported-eku"); + }); + + it("refuses an EKU that omits serverAuth", async () => { + const { pemCertBase64, thumbprint } = await makeDevPem({ + eku: ["1.3.6.1.5.5.7.3.3"], // codeSigning + }); + const deps = makeDeps(); + const result = await acceptContainerDevCert( + { pemCertBase64, thumbprint }, + deps + ); + + expect(result.accepted).toBe(false); + expect(result.reason).toBe("unsupported-eku"); + }); + + it("accepts an EKU carrying serverAuth alongside other specific usages", async () => { + // Only anyExtendedKeyUsage is refused; extra concrete usages such as + // clientAuth are tolerated so the check isn't brittle. + const { pemCertBase64, thumbprint } = await makeDevPem({ + eku: ["1.3.6.1.5.5.7.3.1", "1.3.6.1.5.5.7.3.2"], + }); + const deps = makeDeps(); + const result = await acceptContainerDevCert( + { pemCertBase64, thumbprint }, + deps + ); + + expect(result.accepted).toBe(true); + expect(deps.trustCertificate).toHaveBeenCalledTimes(1); + }); +}); + +describe("acceptContainerDevCert structural SAN gate", () => { + it("refuses a certificate with no SAN extension", async () => { + const { pemCertBase64, thumbprint } = await makeDevPem({ + omitSanExtension: true, + }); + const deps = makeDeps(); + const result = await acceptContainerDevCert( + { pemCertBase64, thumbprint }, + deps + ); + + expect(result.accepted).toBe(false); + expect(result.reason).toBe("malformed-sans"); + expect(result.detail).toContain("missing"); + expect(deps.trustCertificate).not.toHaveBeenCalled(); + }); + + it("allowNonLocalContainerCertSans does NOT override a structural SAN failure", async () => { + // The override is a statement about scope — "yes, I mean to trust this + // cert for that name". It can't mean anything about a certificate whose + // names we were unable to read at all. + const { pemCertBase64, thumbprint } = await makeDevPem({ + omitSanExtension: true, + }); + const deps = makeDeps({ allowNonLocalSans: true }); + const result = await acceptContainerDevCert( + { pemCertBase64, thumbprint }, + deps + ); + + expect(result.accepted).toBe(false); + expect(result.reason).toBe("malformed-sans"); + expect(deps.trustCertificate).not.toHaveBeenCalled(); + }); +}); + +describe("acceptContainerDevCert accepts what this project actually generates", () => { + it("a cert straight from generateCertificate passes every gate", async () => { + // The gates above reject a cert that isn't a server-auth leaf with a + // local-only dNSName/iPAddress SAN set. That description is supposed to + // be exactly the certificate this project produces — and exactly what + // `dotnet dev-certs https` produces, which `generateCertificate` mirrors + // field for field. Driving the real generator (rather than the fixture + // factory above) means any future divergence between what we emit and + // what we're willing to trust fails here instead of in someone's + // container. + const now = new Date(); + const { cert } = await generateCertificate( + now, + new Date(now.getTime() + VALIDITY_DAYS * 86400_000) + ); + const payload: AcceptContainerCertPayload = { + pemCertBase64: Buffer.from(cert.pem, "utf-8").toString("base64"), + thumbprint: cert.thumbprintSha1, + }; + const deps = makeDeps(); + + const result = await acceptContainerDevCert(payload, deps); + + expect(result).toEqual({ accepted: true }); + expect(deps.trustCertificate).toHaveBeenCalledTimes(1); + }); +}); + +describe("normalizeContainerCertConsent", () => { + it("maps the legacy boolean grant to 'granted' so upgraders are not re-prompted", () => { + // The key previously held a boolean that could only ever be `true`. + // Reading that back as anything but a grant would show the modal again to + // every user who had already consented. + expect(normalizeContainerCertConsent(true)).toBe("granted"); + }); + + it("round-trips the tri-state values", () => { + expect(normalizeContainerCertConsent("granted")).toBe("granted"); + expect(normalizeContainerCertConsent("denied")).toBe("denied"); + }); + + it("falls back to 'unset' for anything unrecognized", () => { + // Ask, rather than silently opting the user in or out, for a stale value, + // a hand-edited state file, or a key that was never written. + for (const value of [undefined, null, false, 0, "", "yes", {}, []]) { + expect(normalizeContainerCertConsent(value)).toBe("unset"); + } + }); +}); diff --git a/src/vscode-ui-extension/tests/dotnetBackend.test.ts b/src/vscode-ui-extension/tests/dotnetBackend.test.ts index ab70bad..db849fa 100644 --- a/src/vscode-ui-extension/tests/dotnetBackend.test.ts +++ b/src/vscode-ui-extension/tests/dotnetBackend.test.ts @@ -1,11 +1,4 @@ -import { - describe, - it, - expect, - beforeEach, - afterEach, - vi, -} from "vitest"; +import { describe, it, expect, beforeEach, afterEach, vi, } from "vitest"; import * as fs from "fs"; import * as os from "os"; import * as path from "path"; @@ -31,14 +24,14 @@ vi.mock("@devcontainer-dev-certs/shared/src/platform/nssTrust", () => ({ trustInNss: vi.fn(), })); -import { DotnetBackend } from "@devcontainer-dev-certs/shared"; -import { runProcess } from "@devcontainer-dev-certs/shared/src/platform/processUtil"; -import { createPlatformStore } from "@devcontainer-dev-certs/shared/src/platform/types"; -import { trustInNss } from "@devcontainer-dev-certs/shared/src/platform/nssTrust"; import { + DotnetBackend, generateCertificate, VALIDITY_DAYS, } from "@devcontainer-dev-certs/shared"; +import { runProcess } from "@devcontainer-dev-certs/shared/src/platform/processUtil"; +import { createPlatformStore } from "@devcontainer-dev-certs/shared/src/platform/types"; +import { trustInNss } from "@devcontainer-dev-certs/shared/src/platform/nssTrust"; const mockedRunProcess = vi.mocked(runProcess); const mockedCreatePlatformStore = vi.mocked(createPlatformStore); @@ -69,7 +62,7 @@ describe("DotnetBackend.generate", () => { cleanupDirs.push(outDir); const generated = await makeCert(); - mockedRunProcess.mockResolvedValue({ exitCode: 0, stdout: "", stderr: "" }); + mockedRunProcess.mockResolvedValue({ exitCode: 0, stdout: "", stderr: "", truncated: false }); mockedCreatePlatformStore.mockResolvedValue({ findExistingDevCert: vi.fn(async () => generated), } as unknown as Shared.PlatformCertificateStore); @@ -100,7 +93,7 @@ describe("DotnetBackend.generate", () => { cleanupDirs.push(outDir); const generated = await makeCert(); - mockedRunProcess.mockResolvedValue({ exitCode: 0, stdout: "", stderr: "" }); + mockedRunProcess.mockResolvedValue({ exitCode: 0, stdout: "", stderr: "", truncated: false }); mockedCreatePlatformStore.mockResolvedValue({ findExistingDevCert: vi.fn(async () => generated), } as unknown as Shared.PlatformCertificateStore); @@ -119,7 +112,7 @@ describe("DotnetBackend.generate", () => { cleanupDirs.push(outDir); const generated = await makeCert(); - mockedRunProcess.mockResolvedValue({ exitCode: 0, stdout: "", stderr: "" }); + mockedRunProcess.mockResolvedValue({ exitCode: 0, stdout: "", stderr: "", truncated: false }); mockedCreatePlatformStore.mockResolvedValue({ findExistingDevCert: vi.fn(async () => generated), } as unknown as Shared.PlatformCertificateStore); @@ -152,7 +145,7 @@ describe("DotnetBackend.generate", () => { cleanupDirs.push(outDir); const generated = await makeCert(); - mockedRunProcess.mockResolvedValue({ exitCode: 0, stdout: "", stderr: "" }); + mockedRunProcess.mockResolvedValue({ exitCode: 0, stdout: "", stderr: "", truncated: false }); mockedCreatePlatformStore.mockResolvedValue({ findExistingDevCert: vi.fn(async () => generated), } as unknown as Shared.PlatformCertificateStore); @@ -212,7 +205,7 @@ describe("DotnetBackend.generate", () => { // there before the backend runs. fs.writeFileSync(persistentPem, "sentinel-existing-pem"); - mockedRunProcess.mockResolvedValue({ exitCode: 0, stdout: "", stderr: "" }); + mockedRunProcess.mockResolvedValue({ exitCode: 0, stdout: "", stderr: "", truncated: false }); mockedCreatePlatformStore.mockResolvedValue({ findExistingDevCert: vi.fn(async () => generated), } as unknown as Shared.PlatformCertificateStore); @@ -241,7 +234,7 @@ describe("DotnetBackend.generate", () => { cleanupDirs.push(outDir); const generated = await makeCert(); - mockedRunProcess.mockResolvedValue({ exitCode: 0, stdout: "", stderr: "" }); + mockedRunProcess.mockResolvedValue({ exitCode: 0, stdout: "", stderr: "", truncated: false }); mockedCreatePlatformStore.mockResolvedValue({ findExistingDevCert: vi.fn(async () => generated), } as unknown as Shared.PlatformCertificateStore); @@ -261,7 +254,7 @@ describe("DotnetBackend.generate", () => { cleanupDirs.push(outDir); const generated = await makeCert(); - mockedRunProcess.mockResolvedValue({ exitCode: 0, stdout: "", stderr: "" }); + mockedRunProcess.mockResolvedValue({ exitCode: 0, stdout: "", stderr: "", truncated: false }); mockedCreatePlatformStore.mockResolvedValue({ findExistingDevCert: vi.fn(async () => generated), } as unknown as Shared.PlatformCertificateStore); @@ -282,6 +275,7 @@ describe("DotnetBackend.generate", () => { exitCode: 1, stdout: "", stderr: "Unrecognized command or argument 'whatever'", + truncated: false, }); await expect( @@ -293,7 +287,7 @@ describe("DotnetBackend.generate", () => { const outDir = fs.mkdtempSync(path.join(os.tmpdir(), "devcerts-dotnet-")); cleanupDirs.push(outDir); - mockedRunProcess.mockResolvedValue({ exitCode: 0, stdout: "", stderr: "" }); + mockedRunProcess.mockResolvedValue({ exitCode: 0, stdout: "", stderr: "", truncated: false }); mockedCreatePlatformStore.mockResolvedValue({ findExistingDevCert: vi.fn(async () => null), } as unknown as Shared.PlatformCertificateStore); diff --git a/src/vscode-ui-extension/tests/dotnetMacosCache.integration.test.ts b/src/vscode-ui-extension/tests/dotnetMacosCache.integration.test.ts index 61b36a6..b93d716 100644 --- a/src/vscode-ui-extension/tests/dotnetMacosCache.integration.test.ts +++ b/src/vscode-ui-extension/tests/dotnetMacosCache.integration.test.ts @@ -4,7 +4,7 @@ import * as os from "os"; import * as path from "path"; import { execFileSync } from "child_process"; import * as pkijs from "pkijs"; -import { loadPfx } from "../src/cert/loader"; +import { loadPfx } from "@devcontainer-dev-certs/shared"; /** * macOS-only integration test guarding the read-side compatibility diff --git a/src/vscode-ui-extension/tests/dotnetPfx.integration.test.ts b/src/vscode-ui-extension/tests/dotnetPfx.integration.test.ts index 5e70ccf..ba8d668 100644 --- a/src/vscode-ui-extension/tests/dotnetPfx.integration.test.ts +++ b/src/vscode-ui-extension/tests/dotnetPfx.integration.test.ts @@ -3,9 +3,11 @@ import * as fs from "fs"; import * as os from "os"; import * as path from "path"; import { execFileSync, spawnSync } from "child_process"; -import { generateCertificate } from "../src/cert/generator"; -import { exportPfx } from "../src/cert/exporter"; -import { VALIDITY_DAYS } from "../src/cert/properties"; +import { + generateCertificate, + exportPfx, + VALIDITY_DAYS, +} from "@devcontainer-dev-certs/shared"; /** * Skips unless `dotnet --version` reports a major SDK version >= 10. The diff --git a/src/vscode-ui-extension/tests/exportLoadedCert.test.ts b/src/vscode-ui-extension/tests/exportLoadedCert.test.ts index 57b325e..fd57d4e 100644 --- a/src/vscode-ui-extension/tests/exportLoadedCert.test.ts +++ b/src/vscode-ui-extension/tests/exportLoadedCert.test.ts @@ -2,10 +2,13 @@ import { describe, it, expect, afterEach } from "vitest"; import * as fs from "fs"; import * as os from "os"; import * as path from "path"; -import { exportLoadedCert, exportPem } from "../src/cert/exporter"; -import { loadPemPair } from "../src/cert/loader"; -import { generateCertificate } from "../src/cert/generator"; -import { VALIDITY_DAYS } from "../src/cert/properties"; +import { + exportLoadedCert, + exportPem, + loadPemPair, + generateCertificate, + VALIDITY_DAYS, +} from "@devcontainer-dev-certs/shared"; async function makeTestCert(): ReturnType { const now = new Date(); diff --git a/src/vscode-ui-extension/tests/exporter.test.ts b/src/vscode-ui-extension/tests/exporter.test.ts index e472f55..5c0e151 100644 --- a/src/vscode-ui-extension/tests/exporter.test.ts +++ b/src/vscode-ui-extension/tests/exporter.test.ts @@ -9,10 +9,10 @@ import { certToPem, keyToPem, certToDer, -} from "../src/cert/exporter"; -import { generateCertificate } from "../src/cert/generator"; -import { VALIDITY_DAYS } from "../src/cert/properties"; -import { parsePfx } from "../src/cert/pfx"; + generateCertificate, + VALIDITY_DAYS, + parsePfx, +} from "@devcontainer-dev-certs/shared"; async function makeTestCert(): ReturnType { const now = new Date(); diff --git a/src/vscode-ui-extension/tests/generator.test.ts b/src/vscode-ui-extension/tests/generator.test.ts index 1f28600..164fde3 100644 --- a/src/vscode-ui-extension/tests/generator.test.ts +++ b/src/vscode-ui-extension/tests/generator.test.ts @@ -1,13 +1,12 @@ import { describe, it, expect } from "vitest"; import { X509Certificate } from "node:crypto"; +import { generateSerialNumber } from "@devcontainer-dev-certs/shared/src/cert/generator"; import { generateCertificate, isValidDevCert, getCertificateVersion, computeThumbprint, -} from "../src/cert/generator"; -import { DevCert } from "../src/cert/types"; -import { + DevCert, ASPNET_HTTPS_OID, CURRENT_CERTIFICATE_VERSION, MINIMUM_CERTIFICATE_VERSION, @@ -15,7 +14,7 @@ import { SAN_DNS_NAMES, SAN_IP_ADDRESSES, VALIDITY_DAYS, -} from "../src/cert/properties"; +} from "@devcontainer-dev-certs/shared"; import { Extension, X509CertificateGenerator } from "@peculiar/x509"; import { webcrypto } from "node:crypto"; @@ -97,6 +96,25 @@ describe("generateCertificate", () => { expect(serial).toMatch(/^[0-9a-f]+$/); const firstNibble = parseInt(serial[0], 16); expect(firstNibble).toBeLessThanOrEqual(7); + // Full 16 bytes: a leading 0x00 would be dropped from this readback (DER + // keeps it as sign padding, `cert.serialNumber` does not), which is + // exactly the case `generateSerialNumber` now rejects. + expect(serial).toHaveLength(32); + }); + + it("never emits a serial needing DER sign padding (10k samples)", async () => { + // This used to fail about one run in 256: `bytes[0] &= 0x7f` can leave + // 0x00, DER retains that byte as sign padding, and the textual readback + // drops it — so the serial appeared to start at or above 0x80 and the + // assertion above flaked. Sampled directly rather than through + // generateCertificate, which would mean 10k RSA keygens. + for (let i = 0; i < 10_000; i++) { + const serial = generateSerialNumber(); + expect(serial).toHaveLength(32); + const leading = parseInt(serial.slice(0, 2), 16); + expect(leading).toBeGreaterThanOrEqual(0x01); + expect(leading).toBeLessThanOrEqual(0x7f); + } }); it("produces an uppercase hex SHA-1 thumbprint on GeneratedCert", async () => { diff --git a/src/vscode-ui-extension/tests/hostCertGenerator.test.ts b/src/vscode-ui-extension/tests/hostCertGenerator.test.ts index 8dd8db9..8f48f11 100644 --- a/src/vscode-ui-extension/tests/hostCertGenerator.test.ts +++ b/src/vscode-ui-extension/tests/hostCertGenerator.test.ts @@ -1,15 +1,18 @@ -import { - describe, - it, - expect, - beforeEach, - vi, - type Mock, -} from "vitest"; +import { describe, it, expect, beforeEach, vi, type Mock, } from "vitest"; import * as fs from "fs"; import * as os from "os"; import * as path from "path"; -import type { Backend } from "@devcontainer-dev-certs/shared"; +import { + selectBackend, + generateCertificate, + VALIDITY_DAYS, +} from "@devcontainer-dev-certs/shared"; +import type { + Backend, + CertManager, + DevCert, + DevKey, +} from "@devcontainer-dev-certs/shared"; import type * as Shared from "@devcontainer-dev-certs/shared"; @@ -26,12 +29,7 @@ vi.mock("@devcontainer-dev-certs/shared", async () => { }; }); -import { selectBackend } from "@devcontainer-dev-certs/shared"; import { CertProvider } from "../src/certProvider"; -import { generateCertificate } from "../src/cert/generator"; -import { VALIDITY_DAYS } from "../src/cert/properties"; -import type { CertManager } from "../src/cert/manager"; -import type { DevCert, DevKey } from "../src/cert/types"; import { __resetConfig, __setConfig } from "./__mocks__/vscode"; const mockedSelectBackend = vi.mocked(selectBackend); diff --git a/src/vscode-ui-extension/tests/legacyPfxRejection.test.ts b/src/vscode-ui-extension/tests/legacyPfxRejection.test.ts index f44f372..c79f887 100644 --- a/src/vscode-ui-extension/tests/legacyPfxRejection.test.ts +++ b/src/vscode-ui-extension/tests/legacyPfxRejection.test.ts @@ -3,7 +3,7 @@ import * as fs from "fs"; import * as os from "os"; import * as path from "path"; import { execFileSync } from "child_process"; -import { parsePfx } from "../src/cert/pfx"; +import { parsePfx } from "@devcontainer-dev-certs/shared"; let opensslAvailable = false; try { diff --git a/src/vscode-ui-extension/tests/linuxStore.integration.test.ts b/src/vscode-ui-extension/tests/linuxStore.integration.test.ts index cddc839..89f57eb 100644 --- a/src/vscode-ui-extension/tests/linuxStore.integration.test.ts +++ b/src/vscode-ui-extension/tests/linuxStore.integration.test.ts @@ -3,11 +3,13 @@ import * as fs from "fs"; import * as path from "path"; import * as os from "os"; import { execFileSync } from "child_process"; -import type * as LinuxStoreModule from "../src/platform/linuxStore"; -import { generateCertificate } from "../src/cert/generator"; -import { VALIDITY_DAYS } from "../src/cert/properties"; -import { buildPfx } from "../src/cert/pfx"; -import { getPemFileName } from "@devcontainer-dev-certs/shared"; +import { + LinuxCertificateStore, + generateCertificate, + VALIDITY_DAYS, + buildPfx, + getPemFileName, +} from "@devcontainer-dev-certs/shared"; let opensslAvailable = false; try { @@ -32,18 +34,15 @@ async function makeTestCert(): ReturnType { describe.skipIf(!opensslAvailable)( "LinuxCertificateStore (integration)", () => { - let LinuxCertificateStore: typeof LinuxStoreModule.LinuxCertificateStore; - - beforeEach(async () => { + beforeEach(() => { tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "devcerts-integ-")); testStoreDir = path.join(tmpDir, "x509stores", "my"); testTrustDir = path.join(tmpDir, "trust"); + // Read at call time by `getOpenSslTrustDir`, so setting it here is + // enough — the store module can be imported statically. process.env["DOTNET_DEV_CERTS_OPENSSL_CERTIFICATE_DIRECTORY"] = testTrustDir; - - const mod = await import("../src/platform/linuxStore.js"); - LinuxCertificateStore = mod.LinuxCertificateStore; }); afterEach(() => { @@ -51,7 +50,7 @@ describe.skipIf(!opensslAvailable)( fs.rmSync(tmpDir, { recursive: true, force: true }); }); - it("full lifecycle: save → trust → find → checkStatus → remove", async () => { + it("full lifecycle: save → trust → PEM + canonical hash symlink on disk", async () => { const store = new LinuxCertificateStore(); const { cert, key, thumbprint } = await makeTestCert(); @@ -61,7 +60,8 @@ describe.skipIf(!opensslAvailable)( const pfxBytes = await buildPfx({ cert, key }); fs.writeFileSync(pfxPath, pfxBytes, { mode: 0o600 }); - // Trust — calls real openssl for hash computation. + // Trust — the subject hash is computed in-process; the assertion + // below cross-checks it against the real openssl binary. await store.trustCertificate(cert); const pemPath = path.join(testTrustDir, getPemFileName(thumbprint)); @@ -110,6 +110,45 @@ describe.skipIf(!opensslAvailable)( expect(result).toContain("OK"); }); + it("keeps the newest cert reachable after 12 rebuild-style rotations", async () => { + // The worst case the design has to survive: most dotnet devcontainer + // base images mint a fresh dev cert on every rebuild, and nothing lets + // this extension verify that a container opting into syncContainerCert + // has persisted its store. So assume rotation, and assume the host's + // trust dir accumulates. + // + // Every dev cert is CN=localhost, so all of them land on ONE subject + // hash. With the old ten-slot bound the eleventh rotation got no symlink + // at all: the host went on trusting ten dead certs while the live one + // failed `openssl verify -CApath`, silently and with the feature exactly + // inverted. openssl is the oracle here rather than the symlink count, + // because being reachable by `by_dir` is the only property that matters. + const store = new LinuxCertificateStore(); + let newestPem = ""; + + for (let rotation = 0; rotation < 12; rotation++) { + const { cert, thumbprint } = await makeTestCert(); + await store.trustCertificate(cert); + newestPem = path.join(testTrustDir, getPemFileName(thumbprint)); + } + + const pems = fs + .readdirSync(testTrustDir) + .filter((f) => f.endsWith(".pem")); + expect(pems).toHaveLength(12); + + const verified = execFileSync("openssl", [ + "verify", + "-CApath", + testTrustDir, + "-partial_chain", + newestPem, + ]) + .toString() + .trim(); + expect(verified).toContain("OK"); + }, 120_000); + it("trustCertificate is idempotent — re-trust replaces symlinks cleanly", async () => { const store = new LinuxCertificateStore(); const { cert } = await makeTestCert(); diff --git a/src/vscode-ui-extension/tests/linuxStore.test.ts b/src/vscode-ui-extension/tests/linuxStore.test.ts index 8143e17..30b00a7 100644 --- a/src/vscode-ui-extension/tests/linuxStore.test.ts +++ b/src/vscode-ui-extension/tests/linuxStore.test.ts @@ -4,9 +4,13 @@ import * as path from "path"; import * as os from "os"; import type * as SharedPaths from "@devcontainer-dev-certs/shared/src/paths"; import { initLogger } from "@devcontainer-dev-certs/shared/src/loggerVscode"; -import { generateCertificate } from "../src/cert/generator"; -import { VALIDITY_DAYS } from "../src/cert/properties"; -import { buildPfx, parsePfx } from "../src/cert/pfx"; +import { + generateCertificate, + VALIDITY_DAYS, + buildPfx, + parsePfx, + LinuxCertificateStore, +} from "@devcontainer-dev-certs/shared"; import { logMessages } from "./__mocks__/vscode"; initLogger("test"); @@ -22,6 +26,7 @@ vi.mock("@devcontainer-dev-certs/shared/src/platform/processUtil", () => ({ exitCode: 0, stdout: "abcd1234\n", stderr: "", + truncated: false, }), })); @@ -50,7 +55,6 @@ vi.mock("@devcontainer-dev-certs/shared/src/paths", async (importOriginal) => { }; }); -import { LinuxCertificateStore } from "../src/platform/linuxStore"; import { runProcess } from "@devcontainer-dev-certs/shared/src/platform/processUtil"; import { trustInNss } from "@devcontainer-dev-certs/shared/src/platform/nssTrust"; @@ -128,29 +132,97 @@ describe("LinuxCertificateStore", () => { expect(content).toContain("-----BEGIN CERTIFICATE-----"); }); - it("creates hash symlinks via openssl", async () => { - mockedRunProcess.mockResolvedValue({ - exitCode: 0, - stdout: "a1b2c3d4\n", - stderr: "", - }); - - const { cert } = await makeTestCert(); + it("names the hash symlink with OpenSSL's canonical subject hash", async () => { + // Every cert we manage is CN=localhost, so the subject hash is the + // fixed value `openssl x509 -hash` reports for that name. Asserting + // the literal is the point: a symlink under any other name is one + // OpenSSL's `by_dir` lookup will never open, which silently disables + // SSL_CERT_DIR trust while looking perfectly healthy on disk. + const { cert, thumbprint } = await makeTestCert(); await store.trustCertificate(cert); - const symlinkPath = path.join(testTrustDir, "a1b2c3d4.0"); + const symlinkPath = path.join(testTrustDir, "ce275665.0"); expect(fs.existsSync(symlinkPath)).toBe(true); expect(fs.lstatSync(symlinkPath).isSymbolicLink()).toBe(true); + expect(fs.readlinkSync(symlinkPath)).toBe( + `aspnetcore-localhost-${thumbprint}.pem` + ); }); - it("calls openssl x509 -hash to compute the subject hash", async () => { + it("computes the subject hash in-process, never shelling out to openssl", async () => { + // The host is a developer machine we don't control; requiring an + // `openssl` binary there would make OpenSSL trust silently no-op on + // any host without it (the old code returned null and skipped the + // symlink). Trust must not depend on host tooling. const { cert } = await makeTestCert(); await store.trustCertificate(cert); - expect(mockedRunProcess).toHaveBeenCalledWith( - "openssl", - expect.arrayContaining(["x509", "-hash", "-noout", "-in"]) + const spawned = mockedRunProcess.mock.calls.map((c) => c[0]); + expect(spawned).not.toContain("openssl"); + }); + + it("reports NOT trusted when the hash symlink is under the wrong hash", async () => { + // A host trusted before the subject hash was computed canonically has a + // link under the wrong name. If `isTrusted` only checked the PEM and root + // PFX, it would answer "trusted", CertManager.trust() would skip + // trustCertificate, and the broken link would survive every upgrade — + // leaving local OpenSSL trust dead for exactly the hosts this fixes. + const { cert, thumbprint } = await makeTestCert(); + await store.trustCertificate(cert); + expect(await store.isCertTrusted(cert)).toBe(true); + + const links = fs + .readdirSync(testTrustDir) + .filter((f) => /^[0-9a-f]{8}\.\d+$/.test(f)); + expect(links).toHaveLength(1); + const target = fs.readlinkSync(path.join(testTrustDir, links[0])); + fs.unlinkSync(path.join(testTrustDir, links[0])); + fs.symlinkSync(target, path.join(testTrustDir, "deadbeef.0")); + + // PEM and root PFX are both still present... + expect( + fs.existsSync(path.join(testTrustDir, `aspnetcore-localhost-${thumbprint}.pem`)) + ).toBe(true); + expect(fs.existsSync(path.join(testRootStoreDir, `${thumbprint}.pfx`))).toBe(true); + // ...but trust is not actually established. + expect(await store.isCertTrusted(cert)).toBe(false); + + // Re-trusting repairs it. + await store.trustCertificate(cert); + expect(await store.isCertTrusted(cert)).toBe(true); + }); + + it("reports NOT trusted when the PEM on disk no longer matches the cert", async () => { + // The filename is thumbprint-derived, so a *different* cert can't land + // here — but the file can still be truncated or rewritten in place + // while the hash link and root PFX survive. Checking only the name + // would report trust for bytes OpenSSL cannot load, and + // trustExternalCertificate's short-circuit would skip the repair. + const { cert, thumbprint } = await makeTestCert(); + await store.trustCertificate(cert); + expect(await store.isCertTrusted(cert)).toBe(true); + + const pemPath = path.join( + testTrustDir, + `aspnetcore-localhost-${thumbprint}.pem` ); + fs.writeFileSync( + pemPath, + "-----BEGIN CERTIFICATE-----\ntruncated\n" + ); + + // Link and root PFX are untouched — only the content changed. + const links = fs + .readdirSync(testTrustDir) + .filter((f) => /^[0-9a-f]{8}\.\d+$/.test(f)); + expect(links).toHaveLength(1); + expect(fs.existsSync(path.join(testRootStoreDir, `${thumbprint}.pfx`))).toBe(true); + + expect(await store.isCertTrusted(cert)).toBe(false); + + // Re-trusting rewrites the PEM and restores trust. + await store.trustCertificate(cert); + expect(await store.isCertTrusted(cert)).toBe(true); }); it("is purely additive — does NOT remove other aspnetcore-localhost-*.pem files in the trust dir", async () => { @@ -396,47 +468,6 @@ describe("LinuxCertificateStore", () => { }); }); - describe("removeCertificates", () => { - it("removes PFX from .NET store", async () => { - const { cert, key, thumbprint } = await makeTestCert(); - await store.saveCertificate(cert, key, thumbprint); - - const pfxPath = path.join(testStoreDir, `${thumbprint}.pfx`); - expect(fs.existsSync(pfxPath)).toBe(true); - - await store.removeCertificates(); - expect(fs.existsSync(pfxPath)).toBe(false); - }); - - it("removes PEM and hash symlinks from trust directory", async () => { - mockedRunProcess.mockResolvedValue({ - exitCode: 0, - stdout: "a1b2c3d4\n", - stderr: "", - }); - - const { cert, key, thumbprint } = await makeTestCert(); - await store.saveCertificate(cert, key, thumbprint); - await store.trustCertificate(cert); - - const pemPath = path.join( - testTrustDir, - `aspnetcore-localhost-${thumbprint}.pem` - ); - const symlinkPath = path.join(testTrustDir, "a1b2c3d4.0"); - expect(fs.existsSync(pemPath)).toBe(true); - expect(fs.existsSync(symlinkPath)).toBe(true); - - await store.removeCertificates(); - expect(fs.existsSync(pemPath)).toBe(false); - expect(fs.existsSync(symlinkPath)).toBe(false); - }); - - it("handles non-existent directories gracefully", async () => { - await expect(store.removeCertificates()).resolves.toBeUndefined(); - }); - }); - describe("checkStatus", () => { it("returns full status for a saved and trusted cert", async () => { const { cert, key, thumbprint } = await makeTestCert(); diff --git a/src/vscode-ui-extension/tests/loader.test.ts b/src/vscode-ui-extension/tests/loader.test.ts index 16ddc8a..a1a60b6 100644 --- a/src/vscode-ui-extension/tests/loader.test.ts +++ b/src/vscode-ui-extension/tests/loader.test.ts @@ -2,10 +2,14 @@ import { describe, it, expect, afterEach } from "vitest"; import * as fs from "fs"; import * as os from "os"; import * as path from "path"; -import { loadPfx, loadPemPair } from "../src/cert/loader"; -import { exportPfx, exportPem } from "../src/cert/exporter"; -import { generateCertificate } from "../src/cert/generator"; -import { VALIDITY_DAYS } from "../src/cert/properties"; +import { + loadPfx, + loadPemPair, + exportPfx, + exportPem, + generateCertificate, + VALIDITY_DAYS, +} from "@devcontainer-dev-certs/shared"; async function makeTestCert(): ReturnType { const now = new Date(); diff --git a/src/vscode-ui-extension/tests/macStore.test.ts b/src/vscode-ui-extension/tests/macStore.test.ts index 592ba42..fe9d7f2 100644 --- a/src/vscode-ui-extension/tests/macStore.test.ts +++ b/src/vscode-ui-extension/tests/macStore.test.ts @@ -4,9 +4,12 @@ import * as os from "os"; import * as path from "path"; import { initLogger } from "@devcontainer-dev-certs/shared/src/loggerVscode"; import { logMessages } from "./__mocks__/vscode"; -import { generateCertificate } from "../src/cert/generator"; -import { VALIDITY_DAYS } from "../src/cert/properties"; -import { buildPfx } from "../src/cert/pfx"; +import { + generateCertificate, + VALIDITY_DAYS, + buildPfx, + MacCertificateStore, +} from "@devcontainer-dev-certs/shared"; // Mock os.homedir so the macStore points at a writable temp dir. let testHomeDir = ""; @@ -22,7 +25,6 @@ vi.mock("@devcontainer-dev-certs/shared/src/platform/processUtil", () => ({ runProcess: vi.fn(), })); -import { MacCertificateStore } from "../src/platform/macStore"; import { runProcess } from "@devcontainer-dev-certs/shared/src/platform/processUtil"; const mockedRunProcess = vi.mocked(runProcess); @@ -55,12 +57,20 @@ function devCertsDir(): string { function setupSecurityMock(opts: { keychainThumbs?: Set; extraKeychainPems?: string[]; + /** + * Simulate `runProcess` killing `security` for exceeding the output cap: + * exitCode 1, empty stderr, a truncated stdout prefix. That is exactly the + * shape Node produces (`error.code` is the string + * ERR_CHILD_PROCESS_STDIO_MAXBUFFER, so it lands in the same exitCode 1 + * bucket as a real failure). + */ + truncateHashEnumeration?: boolean; } = {}) { const calls: Array<{ cmd: string; args: readonly string[] }> = []; mockedRunProcess.mockImplementation(async (cmd: string, args: readonly string[]) => { calls.push({ cmd, args: [...args] }); if (cmd !== "security") { - return { exitCode: 0, stdout: "", stderr: "" }; + return { exitCode: 0, stdout: "", stderr: "", truncated: false }; } const sub = args[0]; if (sub === "find-certificate") { @@ -73,7 +83,15 @@ function setupSecurityMock(opts: { `SHA-256 hash: ${"AB".repeat(32)}\nSHA-1 hash: ${t}\nkeychain: "/Users/test/Library/Keychains/login.keychain-db"\n` ) .join(""); - return { exitCode: 0, stdout, stderr: "" }; + if (opts.truncateHashEnumeration) { + return { + exitCode: 1, + stdout: stdout.slice(0, 32), + stderr: "", + truncated: true, + }; + } + return { exitCode: 0, stdout, stderr: "", truncated: false }; } // enumerate all as PEM if (args.includes("-a") && args.includes("-p")) { @@ -82,10 +100,11 @@ function setupSecurityMock(opts: { exitCode: 0, stdout: pems.join("\n"), stderr: "", + truncated: false, }; } } - return { exitCode: 0, stdout: "", stderr: "" }; + return { exitCode: 0, stdout: "", stderr: "", truncated: false }; }); return { calls, @@ -127,6 +146,64 @@ describe("MacCertificateStore.findExistingDevCert", () => { expect(sec.securityExportCalled()).toBe(false); }); + it("keeps the cert when keychain enumeration is truncated, instead of regenerating", async () => { + // `security find-certificate -a` dumps the whole login keychain, which on + // a large one blows past the output cap. Node reports that with a STRING + // error.code, so it arrives as exitCode 1 with empty stderr — + // indistinguishable from a real failure without the `truncated` flag. + // + // Reading it as "not in the keychain" is the destructive answer: the PFX + // gets force-skipped as an orphan, findExistingDevCert comes back empty, + // checkStatus reports exists:false, and CertManager.trust() generates a + // fresh cert plus an add-trusted-cert password prompt. That cert then + // lands in the same keychain, so the next call truncates sooner — a loop + // that feeds itself. So we deliberately fail OPEN here. + const { cert, key, thumbprint } = await makeTestCert(); + const pfxBytes = await buildPfx({ cert, key }); + fs.writeFileSync( + path.join(devCertsDir(), `aspnetcore-localhost-${thumbprint}.pfx`), + pfxBytes + ); + + setupSecurityMock({ + keychainThumbs: new Set([thumbprint]), + truncateHashEnumeration: true, + }); + + const found = await store.findExistingDevCert(); + expect(found?.thumbprint).toBe(thumbprint); + // Specifically NOT classified as an orphaned cache file. + expect( + logMessages.find((m) => m.includes("orphaned cache file")) + ).toBeUndefined(); + expect( + logMessages.find((m) => m.includes("exceeded the output cap")) + ).toBeDefined(); + }); + + it("still reports a genuinely absent cert as an orphan when output is complete", async () => { + // The fail-open above must be scoped to truncation only — an untruncated + // enumeration that simply doesn't list the thumbprint still means the PFX + // is orphaned, and that classification has to survive. + const { cert, key, thumbprint } = await makeTestCert(); + const pfxBytes = await buildPfx({ cert, key }); + fs.writeFileSync( + path.join(devCertsDir(), `aspnetcore-localhost-${thumbprint}.pfx`), + pfxBytes + ); + + setupSecurityMock({ + keychainThumbs: new Set(), + truncateHashEnumeration: false, + }); + + const found = await store.findExistingDevCert(); + expect(found).toBeNull(); + expect( + logMessages.find((m) => m.includes("orphaned cache file")) + ).toBeDefined(); + }); + it("excludes a PFX whose cert is NOT in the keychain and logs the orphan warning", async () => { const { cert, key, thumbprint } = await makeTestCert(); const pfxBytes = await buildPfx({ cert, key }); @@ -237,130 +314,3 @@ describe("MacCertificateStore.findExistingDevCert", () => { }); }); -describe("MacCertificateStore.removeCertificates", () => { - let store: MacCertificateStore; - - beforeEach(() => { - vi.clearAllMocks(); - logMessages.length = 0; - testHomeDir = fs.mkdtempSync(path.join(os.tmpdir(), "devcerts-mac-rm-")); - fs.mkdirSync(devCertsDir(), { recursive: true }); - store = new MacCertificateStore(); - }); - - afterEach(() => { - fs.rmSync(testHomeDir, { recursive: true, force: true }); - }); - - it("calls `security remove-trusted-cert ` for each dev cert (no `-d`, no keychain positional)", async () => { - const { cert, key, thumbprint } = await makeTestCert(); - const pfxBytes = await buildPfx({ cert, key }); - fs.writeFileSync( - path.join(devCertsDir(), `aspnetcore-localhost-${thumbprint}.pfx`), - pfxBytes - ); - - const sec = setupSecurityMock(); - await store.removeCertificates(); - - const untrust = sec.calls.filter( - (c) => c.cmd === "security" && c.args[0] === "remove-trusted-cert" - ); - expect(untrust).toHaveLength(1); - // No `-d` (we trusted to user domain, not admin) — using -d here - // would look in the wrong trust-settings file and silently miss - // our entry. - expect(untrust[0].args).not.toContain("-d"); - // The positional must be a cert file path under os.tmpdir() — NOT - // the keychain path. Past bug: we were passing the keychain path - // here, which made the command a no-op (or worse, errored). - const tmpDir = os.tmpdir(); - const positional = untrust[0].args[untrust[0].args.length - 1]; - expect(positional.startsWith(tmpDir)).toBe(true); - expect(positional).toMatch(/devcert-untrust-.*\.cer$/); - }); - - it("calls untrust BEFORE delete-certificate (so trust-settings entries aren't orphaned)", async () => { - const { cert, key, thumbprint } = await makeTestCert(); - const pfxBytes = await buildPfx({ cert, key }); - fs.writeFileSync( - path.join(devCertsDir(), `aspnetcore-localhost-${thumbprint}.pfx`), - pfxBytes - ); - - const sec = setupSecurityMock(); - await store.removeCertificates(); - - const untrustIdx = sec.calls.findIndex( - (c) => c.cmd === "security" && c.args[0] === "remove-trusted-cert" - ); - const deleteIdx = sec.calls.findIndex( - (c) => c.cmd === "security" && c.args[0] === "delete-certificate" - ); - expect(untrustIdx).toBeGreaterThanOrEqual(0); - expect(deleteIdx).toBeGreaterThan(untrustIdx); - }); - - it("unlinks the PFX from disk after the keychain teardown", async () => { - const { cert, key, thumbprint } = await makeTestCert(); - const pfxBytes = await buildPfx({ cert, key }); - const pfxPath = path.join( - devCertsDir(), - `aspnetcore-localhost-${thumbprint}.pfx` - ); - fs.writeFileSync(pfxPath, pfxBytes); - - setupSecurityMock(); - await store.removeCertificates(); - - expect(fs.existsSync(pfxPath)).toBe(false); - }); - - it("regression: never calls `security remove-trusted-cert -d `", async () => { - const { cert, key, thumbprint } = await makeTestCert(); - const pfxBytes = await buildPfx({ cert, key }); - fs.writeFileSync( - path.join(devCertsDir(), `aspnetcore-localhost-${thumbprint}.pfx`), - pfxBytes - ); - - const sec = setupSecurityMock(); - await store.removeCertificates(); - - const bad = sec.calls.find( - (c) => - c.cmd === "security" && - c.args[0] === "remove-trusted-cert" && - c.args.includes("-d") - ); - expect(bad).toBeUndefined(); - }); - - it("processes multiple dev cert PFXes independently", async () => { - const a = await makeTestCert(); - const b = await makeTestCert(); - fs.writeFileSync( - path.join(devCertsDir(), `aspnetcore-localhost-${a.thumbprint}.pfx`), - await buildPfx({ cert: a.cert, key: a.key }) - ); - fs.writeFileSync( - path.join(devCertsDir(), `aspnetcore-localhost-${b.thumbprint}.pfx`), - await buildPfx({ cert: b.cert, key: b.key }) - ); - - const sec = setupSecurityMock(); - await store.removeCertificates(); - - const untrustCount = sec.calls.filter( - (c) => c.cmd === "security" && c.args[0] === "remove-trusted-cert" - ).length; - expect(untrustCount).toBe(2); - }); - - it("no-ops cleanly when the devCertsDir doesn't exist", async () => { - fs.rmSync(devCertsDir(), { recursive: true, force: true }); - const sec = setupSecurityMock(); - await store.removeCertificates(); - expect(sec.calls).toHaveLength(0); - }); -}); diff --git a/src/vscode-ui-extension/tests/manager.test.ts b/src/vscode-ui-extension/tests/manager.test.ts index 71a2a1a..f07cd23 100644 --- a/src/vscode-ui-extension/tests/manager.test.ts +++ b/src/vscode-ui-extension/tests/manager.test.ts @@ -1,11 +1,14 @@ import { describe, it, expect, vi, beforeEach } from "vitest"; import type * as PlatformTypes from "@devcontainer-dev-certs/shared/src/platform/types"; import { - type PlatformCertificateStore, - type CertificateStatus, -} from "../src/platform/types"; -import { generateCertificate } from "../src/cert/generator"; -import { VALIDITY_DAYS } from "../src/cert/properties"; + generateCertificate, + VALIDITY_DAYS, + CertManager, +} from "@devcontainer-dev-certs/shared"; +import type { + PlatformCertificateStore, + CertificateStatus, +} from "@devcontainer-dev-certs/shared"; // Mock createPlatformStore so the CertManager uses our fake store. The // CertManager now lives in `@devcontainer-dev-certs/shared` and imports @@ -19,7 +22,6 @@ vi.mock("@devcontainer-dev-certs/shared/src/platform/types", async (importOrigin }; }); -import { CertManager } from "../src/cert/manager"; import { createPlatformStore } from "@devcontainer-dev-certs/shared/src/platform/types"; const mockedCreateStore = vi.mocked(createPlatformStore); @@ -44,7 +46,6 @@ function makeFakeStore( // in tests that pre-date that check. Tests that want to assert the // short-circuit fires override this to true. isCertTrusted: vi.fn().mockResolvedValue(false), - removeCertificates: vi.fn().mockResolvedValue(undefined), checkStatus: vi.fn().mockResolvedValue({ exists: false, isTrusted: false, @@ -72,19 +73,6 @@ describe("CertManager", () => { await manager.generate(); expect(store.saveCertificate).toHaveBeenCalledOnce(); }); - - it("removes existing certs when force is true", async () => { - const manager = new CertManager(); - await manager.generate(true); - expect(store.removeCertificates).toHaveBeenCalledOnce(); - expect(store.saveCertificate).toHaveBeenCalledOnce(); - }); - - it("does not remove existing certs when force is false", async () => { - const manager = new CertManager(); - await manager.generate(false); - expect(store.removeCertificates).not.toHaveBeenCalled(); - }); }); describe("trust", () => { @@ -240,14 +228,6 @@ describe("CertManager", () => { }); }); - describe("clean", () => { - it("delegates to the platform store", async () => { - const manager = new CertManager(); - await manager.clean(); - expect(store.removeCertificates).toHaveBeenCalledOnce(); - }); - }); - describe("exportCert", () => { it("throws if no cert is loaded and none in store", async () => { const manager = new CertManager(); diff --git a/src/vscode-ui-extension/tests/nativeBackend.test.ts b/src/vscode-ui-extension/tests/nativeBackend.test.ts index b041c87..d8b96b1 100644 --- a/src/vscode-ui-extension/tests/nativeBackend.test.ts +++ b/src/vscode-ui-extension/tests/nativeBackend.test.ts @@ -1,10 +1,4 @@ -import { - describe, - it, - expect, - beforeEach, - afterEach, -} from "vitest"; +import { describe, it, expect, beforeEach, afterEach, } from "vitest"; import * as fs from "fs"; import * as os from "os"; import * as path from "path"; diff --git a/src/vscode-ui-extension/tests/nssTrust.integration.test.ts b/src/vscode-ui-extension/tests/nssTrust.integration.test.ts index 44175eb..1e94630 100644 --- a/src/vscode-ui-extension/tests/nssTrust.integration.test.ts +++ b/src/vscode-ui-extension/tests/nssTrust.integration.test.ts @@ -3,10 +3,12 @@ import * as fs from "fs"; import * as path from "path"; import * as os from "os"; import { execFileSync } from "child_process"; -import { generateCertificate } from "../src/cert/generator"; -import { certToPem } from "../src/cert/exporter"; -import { VALIDITY_DAYS } from "../src/cert/properties"; -import { runProcess } from "../src/platform/processUtil"; +import { + generateCertificate, + certToPem, + VALIDITY_DAYS, + runProcess, +} from "@devcontainer-dev-certs/shared"; // Check if certutil is available — skip entire suite if not let certutilAvailable = false; @@ -60,7 +62,7 @@ describe.skipIf(!certutilAvailable)("NSS trust (integration)", () => { // Add the cert const addResult = await runProcess("certutil", [ "-A", "-d", `sql:${nssDbDir}`, - "-t", "CT,,", + "-t", "P,,", "-n", certName, "-i", pemPath, ]); @@ -80,7 +82,7 @@ describe.skipIf(!certutilAvailable)("NSS trust (integration)", () => { // Add the cert await runProcess("certutil", [ "-A", "-d", `sql:${nssDbDir}`, - "-t", "CT,,", "-n", certName, "-i", pemPath, + "-t", "P,,", "-n", certName, "-i", pemPath, ]); // Delete it @@ -92,7 +94,7 @@ describe.skipIf(!certutilAvailable)("NSS trust (integration)", () => { // Re-add it const readdResult = await runProcess("certutil", [ "-A", "-d", `sql:${nssDbDir}`, - "-t", "CT,,", "-n", certName, "-i", pemPath, + "-t", "P,,", "-n", certName, "-i", pemPath, ]); expect(readdResult.exitCode).toBe(0); @@ -103,19 +105,46 @@ describe.skipIf(!certutilAvailable)("NSS trust (integration)", () => { expect(listResult.stdout).toContain(certName); }); - it("certificate is trusted with CT flags after import", async () => { + it("stores the trust flags it was given", async () => { const certName = "Trust Flags Test"; await runProcess("certutil", [ "-A", "-d", `sql:${nssDbDir}`, - "-t", "CT,,", "-n", certName, "-i", pemPath, + "-t", "P,,", "-n", certName, "-i", pemPath, ]); // List with details to check trust flags const listResult = await runProcess("certutil", [ "-L", "-d", `sql:${nssDbDir}`, ]); - expect(listResult.stdout).toContain("CT"); + expect(listResult.stdout).toContain("P,,"); + }); + + // The reason `trustFlagsFor` sends `P` to Chromium-family databases. Our + // cert is a self-signed end entity (cA=FALSE), so the CA trust bit is + // never consulted for it and `C` yields a cert NSS refuses to validate. + // This is the regression guard for the blanket `CT,,` we used to send to + // every database — Chromium's included. Firefox is deliberately not + // covered: it ignores `P` and needs `C`, which by construction cannot + // pass `-V`, which is why dotnet's own check is existence-only there. + it("only the peer trust flag makes the dev cert usable for server auth", async () => { + const verifyWith = async (flags: string): Promise => { + const db = path.join(tmpDir, `verify-${flags.replace(/,/g, "")}`); + fs.mkdirSync(db, { recursive: true }); + execFileSync("certutil", ["-N", "-d", `sql:${db}`, "--empty-password"]); + await runProcess("certutil", [ + "-A", "-d", `sql:${db}`, + "-t", flags, "-n", "Dev Container Dev Cert", "-i", pemPath, + ]); + // -u V is server-auth usage; the same check dotnet runs on Chromium DBs. + const r = await runProcess("certutil", [ + "-V", "-d", `sql:${db}`, "-n", "Dev Container Dev Cert", "-u", "V", + ]); + return r.exitCode; + }; + + expect(await verifyWith("P,,")).toBe(0); + expect(await verifyWith("CT,,")).not.toBe(0); }); it("trustInNss finds and trusts in a Chromium-style NSS database", async () => { @@ -142,7 +171,7 @@ describe.skipIf(!certutilAvailable)("NSS trust (integration)", () => { ]); const addResult = await runProcess("certutil", [ "-A", "-d", `sql:${chromiumNssDir}`, - "-t", "CT,,", "-n", certName, "-i", pemPath, + "-t", "P,,", "-n", certName, "-i", pemPath, ]); expect(addResult.exitCode).toBe(0); @@ -151,7 +180,7 @@ describe.skipIf(!certutilAvailable)("NSS trust (integration)", () => { "-L", "-d", `sql:${chromiumNssDir}`, ]); expect(listResult.stdout).toContain(certName); - expect(listResult.stdout).toContain("CT"); + expect(listResult.stdout).toContain("P,,"); }); it("delete of non-existent cert does not fail the add", async () => { @@ -167,7 +196,7 @@ describe.skipIf(!certutilAvailable)("NSS trust (integration)", () => { // Add should still succeed const addResult = await runProcess("certutil", [ "-A", "-d", `sql:${nssDbDir}`, - "-t", "CT,,", "-n", certName, "-i", pemPath, + "-t", "P,,", "-n", certName, "-i", pemPath, ]); expect(addResult.exitCode).toBe(0); }); diff --git a/src/vscode-ui-extension/tests/nssTrust.test.ts b/src/vscode-ui-extension/tests/nssTrust.test.ts index fe1b55c..7931fd7 100644 --- a/src/vscode-ui-extension/tests/nssTrust.test.ts +++ b/src/vscode-ui-extension/tests/nssTrust.test.ts @@ -21,11 +21,35 @@ vi.mock("os", async (importOriginal) => { }; }); -import { trustInNss } from "../src/platform/nssTrust"; +import { trustInNss, DevCert } from "@devcontainer-dev-certs/shared"; import { runProcess } from "@devcontainer-dev-certs/shared/src/platform/processUtil"; const mockedRunProcess = vi.mocked(runProcess); +/** A real self-signed CN=localhost certificate — `nicknameFor` has to parse + * it to derive the per-cert NSS nickname. */ +const REAL_PEM = + "-----BEGIN CERTIFICATE-----\n" + + "MIIDCTCCAfGgAwIBAgIUKqotkm31fbIEbOVcgrem0favrgQwDQYJKoZIhvcNAQEL\n" + + "BQAwFDESMBAGA1UEAwwJbG9jYWxob3N0MB4XDTI2MDgyODAwMDM1OFoXDTM2MDgy\n" + + "NTAwMDM1OFowFDESMBAGA1UEAwwJbG9jYWxob3N0MIIBIjANBgkqhkiG9w0BAQEF\n" + + "AAOCAQ8AMIIBCgKCAQEAjgGYX2B2v2F5mSgDK2skLTZ7WtkYEJXZ/dD3i4Io5ZuQ\n" + + "5z4nt6VPSnCZFe8jBcDqcgdnCWUOG8yo7BP0pMQHMNRcqmyfMssIKWenPSPWU3U1\n" + + "qMkah8hJbzQkuPlL88yBRDGlHI5ioE6YJKkvwaXBEpaj7xwL0IeOg7ODBz/C6lev\n" + + "KGqfh8180tJ2/SJc6Hpgi0aaWFmkaYyB2/xZnxGTOaXlYtaU1WLVHSG0pJUdYEAm\n" + + "m8S/oaofwPNEG/GStb+X5NVQKxQS2ZhsPcrv55EoZ43ukRwvUCeE1jN0xAVx9KO6\n" + + "1PzYWxGwrneCv45VV+698LstLLn9tWL0FAe0MWxfcwIDAQABo1MwUTAdBgNVHQ4E\n" + + "FgQUszuVse2bqDyPBDxDgwodnoWFiSowHwYDVR0jBBgwFoAUszuVse2bqDyPBDxD\n" + + "gwodnoWFiSowDwYDVR0TAQH/BAUwAwEB/zANBgkqhkiG9w0BAQsFAAOCAQEAd8fg\n" + + "cVxi0bb27kpCjCBBkWGJkfu2SpY8D345PPvsQfxEoaBmvmPSo+V0uO5vPM6VQkMb\n" + + "nwOyGytTYM+uVWADA3YJ+gYpToRfWE+06hKh2ziCDves8rObymLHApFosU0ulT35\n" + + "HWw7S1Sv68k4Wqh7Q7neaYdKGjXWIpMbQ/aDUkUSRYYdmCyidmxAJFi71ROmkl0N\n" + + "SutU65eZyiU8Rh6GSn1u3iPn+DHtcI/3npplew/kXUSliw4gpI7lipD31uBHVJc+\n" + + "k8ge6yTGRi5QppCpiSYcpv0MJ1+DdaadFkYjOV4DPXid9xeJ7ZwQX2rK6Zbkj36Z\n" + + "dW1E/BkFPJeKGPofjA==\n" + + "-----END CERTIFICATE-----\n"; + + function makeNssDb(...segs: string[]): string { const dir = path.join(mockHomeDir, ...segs); fs.mkdirSync(dir, { recursive: true }); @@ -38,6 +62,7 @@ function whichOk(): void { exitCode: 0, stdout: "/usr/bin/certutil\n", stderr: "", + truncated: false, }); } @@ -47,6 +72,7 @@ function certutilOk(times: number): void { exitCode: 0, stdout: "", stderr: "", + truncated: false, }); } } @@ -75,6 +101,7 @@ describe("trustInNss", () => { exitCode: 1, stdout: "", stderr: "which: no certutil in PATH", + truncated: false, }); const result = await trustInNss(pemPath); @@ -89,6 +116,7 @@ describe("trustInNss", () => { exitCode: 0, stdout: "/usr/bin/certutil\n", stderr: "", + truncated: false, }); const result = await trustInNss(pemPath); @@ -111,7 +139,7 @@ describe("trustInNss", () => { (call) => call[0] === "certutil" && call[1].includes("-A") ); expect(addCall).toBeDefined(); - expect(addCall![1]).toContain("CT,,"); + expect(addCall![1]).toContain("P,,"); expect(addCall![1]).toContain(pemPath); expect(addCall![1]).toContain(`sql:${nssDir}`); }); @@ -201,11 +229,12 @@ describe("trustInNss", () => { makeNssDb(".mozilla", "firefox", "test.profile"); whichOk(); mockedRunProcess - .mockResolvedValueOnce({ exitCode: 0, stdout: "", stderr: "" }) // -D + .mockResolvedValueOnce({ exitCode: 0, stdout: "", stderr: "", truncated: false }) // -D .mockResolvedValueOnce({ exitCode: 1, stdout: "", stderr: "SEC_ERROR_BAD_DATABASE", + truncated: false, }); // -A fails const result = await trustInNss(pemPath); @@ -232,6 +261,51 @@ describe("trustInNss", () => { expect(addCall[1]).toContain("-A"); }); + // The cert is a self-signed end entity (cA=FALSE), so `P` (CERTDB_TRUSTED, + // "trusted peer") is the correct NSS encoding — except in Firefox, which + // empirically ignores `P` for server certs. `dotnet dev-certs https + // --trust` makes exactly this split, and Microsoft validated it against + // real browsers. A blanket flag is wrong for one family either way. + it("uses trusted-peer (P,,) for Chromium databases", async () => { + makeNssDb(".pki", "nssdb"); + whichOk(); + certutilOk(2); + + await trustInNss(pemPath); + + const addCall = mockedRunProcess.mock.calls[2]; + expect(addCall[1]).toContain("-A"); + expect(addCall[1][addCall[1].indexOf("-t") + 1]).toBe("P,,"); + }); + + it("uses trusted-CA (C,,) for Firefox profiles", async () => { + makeNssDb(".mozilla", "firefox", "flags.profile"); + whichOk(); + certutilOk(2); + + await trustInNss(pemPath); + + const addCall = mockedRunProcess.mock.calls[2]; + expect(addCall[1]).toContain("-A"); + expect(addCall[1][addCall[1].indexOf("-t") + 1]).toBe("C,,"); + }); + + it("picks the flag per database when both families are present", async () => { + makeNssDb(".pki", "nssdb"); + makeNssDb(".mozilla", "firefox", "both.default"); + whichOk(); + certutilOk(4); + + await trustInNss(pemPath); + + // getNssTargets is ordered Chromium-family first, so calls 2 and 4 are + // the two `-A` invocations (each preceded by its idempotency delete). + const chromiumAdd = mockedRunProcess.mock.calls[2]; + const firefoxAdd = mockedRunProcess.mock.calls[4]; + expect(chromiumAdd[1][chromiumAdd[1].indexOf("-t") + 1]).toBe("P,,"); + expect(firefoxAdd[1][firefoxAdd[1].indexOf("-t") + 1]).toBe("C,,"); + }); + it("handles native Chromium and Firefox in a single call", async () => { makeNssDb(".pki", "nssdb"); makeNssDb(".mozilla", "firefox", "multi.default"); @@ -277,4 +351,36 @@ describe("trustInNss", () => { "Trusted in: Chromium, Firefox (Snap) (p.default)" ); }); + + it("names each certificate by thumbprint so two dev certs can coexist", async () => { + // NSS nicknames are unique per database. A shared nickname made every + // `trustInNss` call evict the previously-trusted cert — so the host's + // generated cert and a container-pushed one could never both be trusted + // in browsers, even though the OpenSSL trust dir is deliberately + // additive for exactly that reason. + const realPemPath = path.join(tmpDir, "real-cert.pem"); + fs.writeFileSync(realPemPath, REAL_PEM); + const expectedNickname = `Dev Container Dev Cert (${new DevCert(REAL_PEM).thumbprintSha1})`; + + makeNssDb(".mozilla", "firefox", "thumbprint.profile"); + whichOk(); + certutilOk(3); + + await trustInNss(realPemPath); + + // 1: delete the pre-thumbprint nickname (upgrade cleanup), 2: delete our + // own nickname (idempotency), 3: add under our own nickname. + const legacyDelete = mockedRunProcess.mock.calls[1]; + expect(legacyDelete[1]).toContain("-D"); + expect(legacyDelete[1]).toContain("Dev Container Dev Cert"); + + const selfDelete = mockedRunProcess.mock.calls[2]; + expect(selfDelete[1]).toContain("-D"); + expect(selfDelete[1]).toContain(expectedNickname); + + const addCall = mockedRunProcess.mock.calls[3]; + expect(addCall[1]).toContain("-A"); + expect(addCall[1]).toContain(expectedNickname); + expect(addCall[1]).toContain(realPemPath); + }); }); diff --git a/src/vscode-ui-extension/tests/pkcs12LegacyPbe.test.ts b/src/vscode-ui-extension/tests/pkcs12LegacyPbe.test.ts index 73574c8..77764c8 100644 --- a/src/vscode-ui-extension/tests/pkcs12LegacyPbe.test.ts +++ b/src/vscode-ui-extension/tests/pkcs12LegacyPbe.test.ts @@ -2,12 +2,7 @@ import { describe, it, expect } from "vitest"; import * as fs from "fs"; import * as path from "path"; import { loadPfx } from "@devcontainer-dev-certs/shared"; -import { - SUPPORTED_LEGACY_PBE_OID, - decryptLegacyPbe, - isSupportedLegacyPbe, - pkcs12Kdf, -} from "@devcontainer-dev-certs/shared/src/cert/pkcs12LegacyPbe"; +import { SUPPORTED_LEGACY_PBE_OID, decryptLegacyPbe, isSupportedLegacyPbe, pkcs12Kdf, } from "@devcontainer-dev-certs/shared/src/cert/pkcs12LegacyPbe"; /** * Lifecycle: delete this file when the parent module is removed. See diff --git a/src/vscode-ui-extension/tests/resolveSafeExecPath.test.ts b/src/vscode-ui-extension/tests/resolveSafeExecPath.test.ts index 1f744c1..6091f12 100644 --- a/src/vscode-ui-extension/tests/resolveSafeExecPath.test.ts +++ b/src/vscode-ui-extension/tests/resolveSafeExecPath.test.ts @@ -1,5 +1,8 @@ import { describe, it, expect, afterEach, vi } from "vitest"; -import { resolveSafeExecPath, runProcess } from "@devcontainer-dev-certs/shared"; +import { + resolveSafeExecPath, + runProcess, +} from "@devcontainer-dev-certs/shared"; /** * The resolver is Windows-specific defense against `CreateProcess`'s @@ -251,3 +254,47 @@ describe("runProcess safe-exec guard", () => { } ); }); + +/** + * Output-cap behavior. Driven against real child processes rather than a mock + * because the whole point is Node's own semantics: on overflow it kills the + * child and rejects with a *string* `error.code` + * (ERR_CHILD_PROCESS_STDIO_MAXBUFFER), which `runProcess` folds into + * `exitCode: 1` with an empty stderr — the same shape as a genuine failure. + * `truncated` is the only thing that tells them apart. + */ +describe.skipIf(process.platform === "win32")("runProcess output cap", () => { + it("reports truncated: false for output under the cap", async () => { + const result = await runProcess("head", ["-c", "500000", "/dev/zero"]); + expect(result.exitCode).toBe(0); + expect(result.truncated).toBe(false); + expect(result.stdout).toHaveLength(500_000); + }, 30_000); + + it("carries 2 MiB of output that Node's 1 MiB default would have cut", async () => { + // The regression this guards: `security find-certificate -a` on a large + // macOS login keychain exceeds 1 MiB, and losing that output made + // isCertInKeychain answer "no" and trigger a regenerate loop. + const result = await runProcess("head", ["-c", "2000000", "/dev/zero"]); + expect(result.exitCode).toBe(0); + expect(result.truncated).toBe(false); + expect(result.stdout).toHaveLength(2_000_000); + }, 30_000); + + it("flags truncated: true when a command exceeds even the raised cap", async () => { + // `yes` never terminates, so it always overruns. Confirms the flag is + // actually wired to Node's string error code and not just defaulted. + const result = await runProcess("yes", ["truncation-probe"], 20_000); + expect(result.truncated).toBe(true); + // The failure is indistinguishable from a real one without the flag: + // non-zero exit, and nothing in stderr to explain it. + expect(result.exitCode).not.toBe(0); + expect(result.stderr).toBe(""); + }, 60_000); + + it("reports truncated: false for an ordinary non-zero exit", async () => { + const result = await runProcess("ls", ["/definitely/not/here"]); + expect(result.exitCode).not.toBe(0); + expect(result.truncated).toBe(false); + }, 30_000); +}); diff --git a/src/vscode-ui-extension/tests/selectBestDevCert.test.ts b/src/vscode-ui-extension/tests/selectBestDevCert.test.ts index 5b75abf..8b28530 100644 --- a/src/vscode-ui-extension/tests/selectBestDevCert.test.ts +++ b/src/vscode-ui-extension/tests/selectBestDevCert.test.ts @@ -1,8 +1,13 @@ import { describe, it, expect, beforeEach } from "vitest"; import { initLogger } from "@devcontainer-dev-certs/shared/src/loggerVscode"; import { logMessages } from "./__mocks__/vscode"; -import { selectBestDevCert, type UsableDevCert } from "../src/platform/baseStore"; -import { generateCertificate } from "../src/cert/generator"; +import { + selectBestDevCert, +} from "@devcontainer-dev-certs/shared/src/platform/baseStore"; +import type { + UsableDevCert, +} from "@devcontainer-dev-certs/shared/src/platform/baseStore"; +import { generateCertificate } from "@devcontainer-dev-certs/shared"; initLogger("test"); diff --git a/src/vscode-ui-extension/tests/validateLocalSans.test.ts b/src/vscode-ui-extension/tests/validateLocalSans.test.ts index 543ecf1..2fb153c 100644 --- a/src/vscode-ui-extension/tests/validateLocalSans.test.ts +++ b/src/vscode-ui-extension/tests/validateLocalSans.test.ts @@ -1,10 +1,5 @@ import { describe, it, expect } from "vitest"; -import { - Extension, - SubjectAlternativeNameExtension, - X509CertificateGenerator, - cryptoProvider, -} from "@peculiar/x509"; +import { Extension, SubjectAlternativeNameExtension, X509CertificateGenerator, cryptoProvider, } from "@peculiar/x509"; import { webcrypto } from "node:crypto"; import { DevCert, @@ -158,10 +153,12 @@ describe("validateLocalSans", () => { expect(result.ok).toBe(false); }); - it("treats a cert with no SAN extension as ok (empty list)", async () => { - // Build a cert without the SAN extension. The validator's contract is - // "no entries → no non-local entries"; callers separately enforce - // CN=localhost via isValidDevCert. + it("rejects a cert with no SAN extension rather than passing it", async () => { + // Previously this returned ok — "no entries, so no non-local entries". + // That reported "SANs are local-only" about a cert whose scope we never + // established. No genuine dev cert omits SAN (the canonical one carries + // seven entries) and a cert without one authenticates no hostname to any + // modern client, so there is nothing legitimate to let through. const keyPair = await webcrypto.subtle.generateKey( { name: "RSASSA-PKCS1-v1_5", @@ -185,8 +182,62 @@ describe("validateLocalSans", () => { extensions: [], }); const result = validateLocalSans(new DevCert(cert)); - expect(result.ok).toBe(true); - expect(result.nonLocalEntries).toEqual([]); + expect(result.ok).toBe(false); + expect(result.reason).toBe("missing"); + }); + + it("reports reason 'non-local' (not a structural failure) for an off-host name", async () => { + // The call site keys the `allowNonLocalContainerCertSans` override off + // this reason specifically, so the discrimination has to hold. + const cert = await makeCertWithSans([ + { type: "dns", value: "evil.example.com" }, + ]); + const result = validateLocalSans(cert); + expect(result.ok).toBe(false); + expect(result.reason).toBe("non-local"); + expect(result.nonLocalEntries).toEqual([ + { type: "dns", value: "evil.example.com" }, + ]); + }); + + it("rejects a SAN carrying a GeneralName type other than dNSName / iPAddress", async () => { + // rfc822Name / uniformResourceIdentifier / directoryName play no part in + // TLS server identity, so dropping them was defensible — but it meant + // vouching for a cert we had only partially inspected. A dev cert has no + // business carrying them. + const cert = await makeCertWithSans([ + { type: "dns", value: "localhost" }, + { type: "email", value: "a@evil.example.com" }, + ] as never); + const result = validateLocalSans(cert); + expect(result.ok).toBe(false); + expect(result.reason).toBe("unsupported-entry"); + expect(result.detail).toContain("email"); + }); + + it("rejects a SAN whose DER does not decode, without throwing", async () => { + // `@peculiar/x509` parses extensions lazily and throws from + // getExtension. That used to escape into the accept handler's blanket + // catch — fail-closed by accident of the call site. Now it is a reported + // reason, so a future `try/catch` added inside the scanner cannot + // silently invert it. + const cert = await makeCertWithSans([{ type: "dns", value: "localhost" }]); + const der = Buffer.from(cert.der); + const needle = Buffer.from([0x06, 0x03, 0x55, 0x1d, 0x11]); + const idx = der.indexOf(needle); + expect(idx).toBeGreaterThan(0); + let p = idx + needle.length; + if (der[p] === 0x01) p += 3; // skip the critical BOOLEAN + expect(der[p]).toBe(0x04); // extnValue OCTET STRING + const contentStart = p + 2; + const contentLength = der[p + 1]; + der.fill(0x00, contentStart, contentStart + contentLength); + der[contentStart] = 0x30; // SEQUENCE header over junk + der[contentStart + 1] = contentLength - 2; + + const result = validateLocalSans(new DevCert(der)); + expect(result.ok).toBe(false); + expect(result.reason).toBe("unparseable"); }); it("handles trailing-dot DNS names like 'localhost.'", async () => { diff --git a/src/vscode-ui-extension/tests/windowsStore.integration.test.ts b/src/vscode-ui-extension/tests/windowsStore.integration.test.ts index 61a5f98..ea19539 100644 --- a/src/vscode-ui-extension/tests/windowsStore.integration.test.ts +++ b/src/vscode-ui-extension/tests/windowsStore.integration.test.ts @@ -1,8 +1,10 @@ import { describe, it, expect, beforeAll, afterEach } from "vitest"; import { execFileSync } from "child_process"; -import { WindowsCertificateStore } from "../src/platform/windowsStore"; -import { generateCertificate } from "../src/cert/generator"; -import { VALIDITY_DAYS } from "../src/cert/properties"; +import { + WindowsCertificateStore, + generateCertificate, + VALIDITY_DAYS, +} from "@devcontainer-dev-certs/shared"; const enabled = process.platform === "win32" && diff --git a/src/vscode-ui-extension/tests/windowsStore.test.ts b/src/vscode-ui-extension/tests/windowsStore.test.ts index ad41aff..ad63a38 100644 --- a/src/vscode-ui-extension/tests/windowsStore.test.ts +++ b/src/vscode-ui-extension/tests/windowsStore.test.ts @@ -4,10 +4,18 @@ import * as os from "os"; import * as path from "path"; import { initLogger } from "@devcontainer-dev-certs/shared/src/loggerVscode"; import { logMessages } from "./__mocks__/vscode"; -import { generateCertificate } from "../src/cert/generator"; -import { VALIDITY_DAYS } from "../src/cert/properties"; -import { buildPfx } from "../src/cert/pfx"; -import { type DevCert, type DevKey } from "../src/cert/types"; +import { + generateCertificate, + VALIDITY_DAYS, + buildPfx, + WindowsCertificateStore, +} from "@devcontainer-dev-certs/shared"; +import type { + DevCert, + DevKey, + PsCandidate, + PsSkipped, +} from "@devcontainer-dev-certs/shared"; // Mock runProcess at the shared internal path — WindowsCertificateStore @@ -18,11 +26,6 @@ vi.mock("@devcontainer-dev-certs/shared/src/platform/processUtil", () => ({ runProcess: vi.fn(), })); -import { - WindowsCertificateStore, - type PsCandidate, - type PsSkipped, -} from "../src/platform/windowsStore"; import { runProcess } from "@devcontainer-dev-certs/shared/src/platform/processUtil"; const mockedRunProcess = vi.mocked(runProcess); @@ -44,7 +47,7 @@ function setupPsMock(opts: { mockedRunProcess.mockImplementation(async (cmd: string, args: readonly string[]) => { if (cmd === "pwsh" && args.includes("echo ok") && !psProbed) { psProbed = true; - return { exitCode: 0, stdout: "ok", stderr: "" }; + return { exitCode: 0, stdout: "ok", stderr: "", truncated: false }; } // Enumeration call — script is the last arg. const payload = { @@ -55,6 +58,7 @@ function setupPsMock(opts: { exitCode: 0, stdout: JSON.stringify(payload), stderr: "", + truncated: false, }; }); } diff --git a/src/vscode-workspace-extension/src/certInstaller.ts b/src/vscode-workspace-extension/src/certInstaller.ts index f2e3f44..a82238e 100644 --- a/src/vscode-workspace-extension/src/certInstaller.ts +++ b/src/vscode-workspace-extension/src/certInstaller.ts @@ -9,9 +9,12 @@ import { getPfxFileName, getPemFileName, getPemFileNameForUser, + computeSubjectHash, + ensureHashSymlink, + hasHashSymlink, + rehashDirectory, } from "@devcontainer-dev-certs/shared"; import type { CertMaterialV3 } from "@devcontainer-dev-certs/shared"; -import { ensureHashSymlink, rehashDirectory } from "./util/rehash"; import type { ExtraDestination } from "./util/destinations"; export type { @@ -155,8 +158,17 @@ export function installUserCert(material: CertMaterialV3): void { * dotnet-dev we check the three historic paths; for user certs we check that * the thumbprint-keyed PFX (when applicable) and, when trust is requested, * the named PEM exist. + * + * "Installed" includes a hash symlink OpenSSL would actually resolve, not just + * the PEM being present. Without that, a container installed before the subject + * hash was computed canonically has its symlink under the WRONG hash: the files + * all exist, this returns true, the install is skipped, and the symlink is never + * repaired — so upgrading would leave trust broken precisely for the users the + * fix exists for. Re-running the install is cheap and rewrites identical bytes. */ export function isCertInstalled(material: CertMaterialV3): boolean { + const trustDir = getOpenSslTrustDir(); + if (material.kind === "dotnet-dev") { const pfxPath = path.join( getDotNetStorePath(), @@ -166,34 +178,89 @@ export function isCertInstalled(material: CertMaterialV3): boolean { getDotNetRootStorePath(), getPfxFileName(material.thumbprint) ); - const pemPath = path.join( - getOpenSslTrustDir(), - getPemFileName(material.thumbprint) - ); - return ( - fs.existsSync(pfxPath) && - fs.existsSync(rootPfxPath) && - fs.existsSync(pemPath) + if (!fs.existsSync(pfxPath) || !fs.existsSync(rootPfxPath)) return false; + return pemInstalledAndLinked( + trustDir, + getPemFileName(material.thumbprint), + decodePem(material) ); } + const storePfxPath = path.join( + getDotNetStorePath(), + getPfxFileName(material.thumbprint) + ); if (material.installToDotNetStore) { - const pfxPath = path.join( - getDotNetStorePath(), - getPfxFileName(material.thumbprint) - ); - if (!fs.existsSync(pfxPath)) return false; + if (!fs.existsSync(storePfxPath)) return false; + } else if (fs.existsSync(storePfxPath)) { + // Opted out, but a passwordless copy from a previous opt-in is still + // sitting in the .NET store. Report "not installed" so the caller runs + // `installUserCert`, whose else-branch sweeps that file — otherwise the + // sweep is unreachable and the plain-text key copy lives on forever + // after the user flips `installUserCertsToDotNetStore` off (or adds + // `excludeFromDotNetStore`). + return false; } if (material.trustInContainer) { - const pemPath = path.join( - getOpenSslTrustDir(), - getPemFileNameForUser(material.name) + // `installUserCert` writes the .NET Root-store PFX too whenever the bundle + // carries one, so checking only the OpenSSL side would report a user cert + // fully installed after its Root PFX was deleted (or a previous install + // stopped between the two writes) — activation would skip the reinstall + // and .NET clients in the container would go on distrusting it. Gated on + // `rootPfxBase64` so a bundle that never supplied one isn't held to a file + // the install would not have written. + if (material.rootPfxBase64) { + const rootPfxPath = path.join( + getDotNetRootStorePath(), + getPfxFileName(material.thumbprint) + ); + if (!fs.existsSync(rootPfxPath)) return false; + } + return pemInstalledAndLinked( + trustDir, + getPemFileNameForUser(material.name), + decodePem(material) ); - if (!fs.existsSync(pemPath)) return false; } return true; } +function decodePem(material: CertMaterialV3): string { + return Buffer.from(material.pemCertBase64, "base64").toString("utf-8"); +} + +/** + * True when the trust directory already holds exactly this certificate under + * `pemFileName`, with a hash symlink OpenSSL can resolve to it. + * + * Compares content, not just existence, because user certs are keyed by the + * user-chosen `name` rather than by thumbprint: rotating the certificate while + * keeping the same name leaves a stale `{name}.pem` whose own hash link + * resolves perfectly well, so an existence check would report "installed" and + * the container would go on serving the superseded certificate indefinitely. + * (The dotnet-dev PEM is thumbprint-keyed, so rotation changes its filename — + * comparing content there is merely consistent rather than load-bearing.) + * + * A PEM whose subject can't be hashed counts as settled: `ensureHashSymlink` + * is a no-op for it, so demanding a link would re-run the install on every + * activation and never converge. + */ +function pemInstalledAndLinked( + trustDir: string, + pemFileName: string, + expectedPem: string +): boolean { + let onDisk: string; + try { + onDisk = fs.readFileSync(path.join(trustDir, pemFileName), "utf-8"); + } catch { + return false; + } + if (onDisk !== expectedPem) return false; + if (computeSubjectHash(onDisk) === null) return true; + return hasHashSymlink(trustDir, pemFileName, onDisk); +} + /** * Write the cert's artifacts to an extra destination per its format. Returns * the rehash directory (if any) so the caller can rehash once at the end. diff --git a/src/vscode-workspace-extension/src/cleanupCerts.ts b/src/vscode-workspace-extension/src/cleanupCerts.ts index 39dbd55..c9696e8 100644 --- a/src/vscode-workspace-extension/src/cleanupCerts.ts +++ b/src/vscode-workspace-extension/src/cleanupCerts.ts @@ -8,9 +8,9 @@ import { getPemFileName, getPfxFileName, parsePfx, + rehashDirectory, } from "@devcontainer-dev-certs/shared"; import type { CertBundleV3 } from "@devcontainer-dev-certs/shared"; -import { rehashDirectory } from "./util/rehash"; /** * Filename convention shared by .NET's OpenSslDirectoryBasedStoreProvider and diff --git a/src/vscode-workspace-extension/src/containerCertPush.ts b/src/vscode-workspace-extension/src/containerCertPush.ts index 659ba26..32b5a99 100644 --- a/src/vscode-workspace-extension/src/containerCertPush.ts +++ b/src/vscode-workspace-extension/src/containerCertPush.ts @@ -24,15 +24,24 @@ export interface AcceptContainerCertResult { /** * Failure code. `host-setting-disabled` means the user hasn't opted in * on the host; `user-declined` means the consent prompt was rejected; - * `non-local-sans` / `parse-failed` / `not-valid-dev-cert` describe - * server-side validation outcomes. + * `non-local-sans` / `malformed-sans` / `parse-failed` / + * `not-valid-dev-cert` / `not-a-leaf-cert` / `unsupported-eku` describe + * server-side validation outcomes; `trust-failed` means the cert passed + * every check and the host's trust step itself failed. An older host + * extension can only ever send the original five; a newer one can send + * codes this build doesn't know, which `reportAcceptOutcome`'s `default` + * branch handles. */ reason?: | "host-setting-disabled" | "user-declined" | "parse-failed" | "not-valid-dev-cert" - | "non-local-sans"; + | "not-a-leaf-cert" + | "unsupported-eku" + | "malformed-sans" + | "non-local-sans" + | "trust-failed"; /** Free-form supplemental detail (e.g. the offending SAN entries). */ detail?: string; } @@ -352,6 +361,53 @@ function reportAcceptOutcome( ) ); return; + case "not-a-leaf-cert": + log( + `Container cert sync: host rejected ${thumbprint} — the certificate is a CA (or omits basicConstraints)${detail}. ` + + `The host only trusts leaf server certificates; a CA would be able to issue certificates for any name.` + ); + void vscode.window.showWarningMessage( + vscode.l10n.t( + "Dev Certs: The container's certificate is a certificate authority, or does not declare itself a leaf (no basicConstraints), so the host refused to trust it. Only leaf server certificates are accepted — regenerate with 'dotnet dev-certs https' rather than using a CA." + ) + ); + return; + case "trust-failed": + // Not a rejection: the host accepted the certificate and then failed + // to install it. Distinct from `parse-failed` because the remedy is + // completely different — retry the trust prompt, free a hash slot, + // fix directory permissions — and none of it involves the cert we + // sent, which was fine. + log( + `Container cert sync: host validated ${thumbprint} but could not establish trust${detail}.` + ); + void vscode.window.showWarningMessage( + vscode.l10n.t( + "Dev Certs: The host accepted the container's dev certificate but could not add it to its trust stores. The certificate itself is fine — check the host's Dev Container Dev Certs output for the underlying error." + ) + ); + return; + case "unsupported-eku": + log( + `Container cert sync: host rejected ${thumbprint} — extended key usage is missing or not scoped to server authentication${detail}.` + ); + void vscode.window.showWarningMessage( + vscode.l10n.t( + "Dev Certs: The container's certificate is not scoped to server authentication (extended key usage), so the host refused to trust it." + ) + ); + return; + case "malformed-sans": + log( + `Container cert sync: host rejected ${thumbprint} — its subject alternative names could not be read as a dev cert's${detail}. ` + + `This is not overridable via devcontainerDevCerts.allowNonLocalContainerCertSans, which only relaxes the local-scope rule.` + ); + void vscode.window.showWarningMessage( + vscode.l10n.t( + "Dev Certs: The container's certificate has a subject alternative name extension the host could not use — missing, unreadable, empty, or carrying entry types other than DNS names and IP addresses — so it was not trusted." + ) + ); + return; case "non-local-sans": log( `Container cert sync: host rejected ${thumbprint} — certificate has non-local SAN entries${detail}. To override, set devcontainerDevCerts.allowNonLocalContainerCertSans to true in host VS Code settings.` diff --git a/src/vscode-workspace-extension/src/extension.ts b/src/vscode-workspace-extension/src/extension.ts index 22101f1..297b531 100644 --- a/src/vscode-workspace-extension/src/extension.ts +++ b/src/vscode-workspace-extension/src/extension.ts @@ -185,6 +185,14 @@ async function injectCertificate( if (bundle.certs.length === 0) { log("No certs returned from host extension."); + // Still run the Kestrel-default pass: with no certs the bundle carries + // no `defaultKestrelCert` pointer, so this sweeps the well-known PFX and + // clears the env vars. `environmentVariableCollection` is persisted by + // VS Code across window reloads, so returning early here would leave a + // previous selection's `__Path`/`__Password` applying indefinitely after + // the user cleared `defaultKestrelCertificate` or removed their user + // certs. + applyDefaultKestrelCert(context, bundle); return; } diff --git a/src/vscode-workspace-extension/src/util/rehash.ts b/src/vscode-workspace-extension/src/util/rehash.ts deleted file mode 100644 index b64d80c..0000000 --- a/src/vscode-workspace-extension/src/util/rehash.ts +++ /dev/null @@ -1,257 +0,0 @@ -import * as crypto from "crypto"; -import * as fs from "fs"; -import * as path from "path"; - -/** - * Pure TypeScript implementation of OpenSSL's c_rehash for certificate directories. - * - * OpenSSL uses the "subject name hash" to look up certificates by filename. - * The hash is computed as: SHA-1 of the DER-encoded canonical subject name, - * then the first 4 bytes interpreted as a little-endian 32-bit unsigned integer, - * formatted as 8-character lowercase hex. - * - * This matches the simplified c_rehash in .NET's UnixCertificateManager. - */ - -/** - * Compute the OpenSSL subject hash from a PEM certificate string. - * Returns the 8-character hex hash string, or null if the cert cannot be parsed. - */ -export function computeSubjectHash(pemCert: string): string | null { - try { - // Parse the PEM to get DER bytes - const derBytes = pemToDer(pemCert); - if (!derBytes) return null; - - // Extract the subject field from the TBSCertificate - const subjectDer = extractSubjectDer(derBytes); - if (!subjectDer) return null; - - // Compute SHA-1 hash of the DER-encoded subject - const hash = crypto.createHash("sha1").update(subjectDer).digest(); - - // Take first 4 bytes as little-endian uint32, format as 8-char hex - const value = hash.readUInt32LE(0); - return value.toString(16).padStart(8, "0"); - } catch { - return null; - } -} - -/** - * Create hash symlinks in a directory for all PEM/CRT files. - * Removes existing hash symlinks first, then creates new ones. - */ -export function rehashDirectory(directory: string): void { - const entries = fs.readdirSync(directory); - - // Remove existing hash symlinks - for (const entry of entries) { - const fullPath = path.join(directory, entry); - if (isHashSymlink(entry) && isSymlink(fullPath)) { - fs.unlinkSync(fullPath); - } - } - - // Create new hash symlinks - const certFiles = entries.filter((f) => /\.(pem|crt|cer)$/i.test(f)); - for (const certFile of certFiles) { - const fullPath = path.join(directory, certFile); - if (isSymlink(fullPath)) continue; // Skip symlinks themselves - ensureHashSymlink(directory, certFile, fs.readFileSync(fullPath, "utf-8")); - } -} - -/** - * Ensure an OpenSSL subject-hash symlink exists for `pemFileName` in - * `directory`. No-op when a valid slot already points at the same PEM — - * the caller can re-invoke this safely on every install without producing - * duplicate `{hash}.0`/`{hash}.1` pairs. Allocates the next free slot - * (`{hash}.0` … `{hash}.9`) on a real collision with a different target. - * - * Unlike `rehashDirectory`, this only touches the slot for our PEM — - * other PEMs' hash symlinks are left alone. - */ -export function ensureHashSymlink( - directory: string, - pemFileName: string, - pemContent: string -): void { - const hash = computeSubjectHash(pemContent); - if (!hash) return; - // Slot 0-9 covers any realistic number of collisions in a dev trust dir. - // Catch EEXIST so a concurrent rehash from another process doesn't crash - // the caller. - for (let i = 0; i < 10; i++) { - const linkName = `${hash}.${i}`; - const linkPath = path.join(directory, linkName); - - // Use lstat (instead of existsSync) so dangling symlinks are detected - // as "occupied" and can be reclaimed. - let existing: fs.Stats | undefined; - try { - existing = fs.lstatSync(linkPath); - } catch { - // ENOENT — slot is free. - } - - if (existing) { - if (!existing.isSymbolicLink()) continue; // not ours; skip - let target: string; - try { - target = fs.readlinkSync(linkPath); - } catch { - continue; - } - if (target === pemFileName) return; // already correct - const targetPath = path.isAbsolute(target) - ? target - : path.join(directory, target); - if (fs.existsSync(targetPath)) continue; // live collision; next slot - // Dangling — reclaim the slot. Hash symlinks are written exclusively - // by this extension, so a dangling one is leftover state we own. - try { - fs.unlinkSync(linkPath); - } catch { - continue; - } - } - - try { - fs.symlinkSync(pemFileName, linkPath); - return; - } catch (err: unknown) { - if ((err as NodeJS.ErrnoException).code === "EEXIST") continue; - throw err; - } - } -} - -// --- Internal helpers --- - -function pemToDer(pem: string): Buffer | null { - const match = pem.match( - /-----BEGIN CERTIFICATE-----\s*([\s\S]*?)\s*-----END CERTIFICATE-----/ - ); - if (!match) return null; - const base64 = match[1].replace(/\s/g, ""); - return Buffer.from(base64, "base64"); -} - -/** - * Extract the Subject field (as raw DER bytes) from an X.509 certificate. - * - * X.509 structure (simplified): - * Certificate ::= SEQUENCE { - * tbsCertificate TBSCertificate, - * ... - * } - * TBSCertificate ::= SEQUENCE { - * version [0] EXPLICIT INTEGER OPTIONAL, - * serialNumber INTEGER, - * signature AlgorithmIdentifier, - * issuer Name, - * validity Validity, - * subject Name, <-- this is what we want - * ... - * } - */ -function extractSubjectDer(certDer: Buffer): Buffer | null { - const offset = 0; - - // Certificate SEQUENCE - const cert = readTag(certDer, offset); - if (!cert || cert.tag !== 0x30) return null; - - // TBSCertificate SEQUENCE - const tbs = readTag(certDer, cert.contentOffset); - if (!tbs || tbs.tag !== 0x30) return null; - - let pos = tbs.contentOffset; - - // version [0] EXPLICIT - optional, skip if present - const maybeVersion = readTag(certDer, pos); - if (!maybeVersion) return null; - if (maybeVersion.tag === 0xa0) { - // version is present, skip it - pos = maybeVersion.contentOffset + maybeVersion.contentLength; - } - - // serialNumber INTEGER - skip - const serial = readTag(certDer, pos); - if (!serial) return null; - pos = serial.contentOffset + serial.contentLength; - - // signature AlgorithmIdentifier SEQUENCE - skip - const sigAlg = readTag(certDer, pos); - if (!sigAlg) return null; - pos = sigAlg.contentOffset + sigAlg.contentLength; - - // issuer Name SEQUENCE - skip - const issuer = readTag(certDer, pos); - if (!issuer) return null; - pos = issuer.contentOffset + issuer.contentLength; - - // validity Validity SEQUENCE - skip - const validity = readTag(certDer, pos); - if (!validity) return null; - pos = validity.contentOffset + validity.contentLength; - - // subject Name SEQUENCE - THIS IS WHAT WE WANT - const subject = readTag(certDer, pos); - if (!subject) return null; - - // Return the full TLV (tag + length + content) of the subject - return certDer.subarray(pos, subject.contentOffset + subject.contentLength); -} - -interface TlvResult { - tag: number; - contentOffset: number; - contentLength: number; -} - -/** - * Read an ASN.1 TLV (Tag-Length-Value) at the given offset. - */ -function readTag(buf: Buffer, offset: number): TlvResult | null { - if (offset >= buf.length) return null; - - const tag = buf[offset]; - let pos = offset + 1; - - if (pos >= buf.length) return null; - - let length = buf[pos]; - pos++; - - if (length & 0x80) { - // Long form length - const numBytes = length & 0x7f; - if (numBytes > 4 || pos + numBytes > buf.length) return null; - length = 0; - for (let i = 0; i < numBytes; i++) { - length = (length << 8) | buf[pos]; - pos++; - } - } - - return { - tag, - contentOffset: pos, - contentLength: length, - }; -} - -function isHashSymlink(filename: string): boolean { - // Hash symlinks match pattern: 8 hex chars + dot + digit(s) - return /^[0-9a-f]{8}\.\d+$/.test(filename); -} - -function isSymlink(filePath: string): boolean { - try { - return fs.lstatSync(filePath).isSymbolicLink(); - } catch { - return false; - } -} diff --git a/src/vscode-workspace-extension/src/util/upmap.ts b/src/vscode-workspace-extension/src/util/upmap.ts index 55136be..edd977d 100644 --- a/src/vscode-workspace-extension/src/util/upmap.ts +++ b/src/vscode-workspace-extension/src/util/upmap.ts @@ -1,3 +1,4 @@ +import { DOTNET_DEV_CERT_NAME } from "@devcontainer-dev-certs/shared"; import type { CertMaterial, CertMaterialV2, @@ -35,7 +36,7 @@ export function upmapV2ToV3(material: CertMaterialV2): CertMaterialV3 { export function upmapV1ToV3(legacy: CertMaterial): CertMaterialV3 { return { kind: "dotnet-dev", - name: "aspnetcore-dev", + name: DOTNET_DEV_CERT_NAME, thumbprint: legacy.thumbprint, pfxBase64: legacy.pfxBase64, pemCertBase64: legacy.pemCertBase64, diff --git a/src/vscode-workspace-extension/tests/installUserCert.test.ts b/src/vscode-workspace-extension/tests/installUserCert.test.ts index c4a6bf2..e75a7fb 100644 --- a/src/vscode-workspace-extension/tests/installUserCert.test.ts +++ b/src/vscode-workspace-extension/tests/installUserCert.test.ts @@ -43,14 +43,48 @@ afterEach(() => { cleanupDirs.length = 0; }); +const REAL_PEM = + "-----BEGIN CERTIFICATE-----\n" + + "MIIBkTCB+wIJANSsAUOhwHK7MA0GCSqGSIb3DQEBCwUAMBQxEjAQBgNVBAMMCWxv\n" + + "Y2FsaG9zdDAeFw0yNDAxMDEwMDAwMDBaFw0zNDAxMDEwMDAwMDBaMBQxEjAQBgNV\n" + + "BAMMCWxvY2FsaG9zdDCBnzANBgkqhkiG9w0BAQEFAAOBjQAwgYkCgYEAyx0qMlYa\n" + + "PEzL0c9XBYNcQ6KAjMjbDLp6FrW+lWZHCKf8/aSJW7CnH2tQHrPiU8r6QYBSWQ7c\n" + + "VTrA8h8wYy7eRdQk31uLR7tGzZ5JxBz2DYxcuxR1RJ/+QbR1m6Z5w9p5UqxQ4l3+\n" + + "AbsmPwy3J7t4cqo3PVPmF6mPiK7M+M0CAwEAATANBgkqhkiG9w0BAQsFAAOBgQAt\n" + + "-----END CERTIFICATE-----\n"; + +const ROTATED_PEM = + "-----BEGIN CERTIFICATE-----\n" + + "MIIDXzCCAkegAwIBAgIUbKzt8uWkwdhKI7QVANKvuaAuga4wDQYJKoZIhvcNAQEL\n" + + "BQAwPzELMAkGA1UEBhMCVVMxFjAUBgNVBAoMDUV4YW1wbGUgIE9yZyAxGDAWBgNV\n" + + "BAMMD01peGVkIENhc2UgTmFtZTAeFw0yNjA4MjgwMDAzNThaFw0zNjA4MjUwMDAz\n" + + "NThaMD8xCzAJBgNVBAYTAlVTMRYwFAYDVQQKDA1FeGFtcGxlICBPcmcgMRgwFgYD\n" + + "VQQDDA9NaXhlZCBDYXNlIE5hbWUwggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEK\n" + + "AoIBAQDLuNsJ2dI5mBGcGeK5lfzKA/8dY5Dunjl10gZybeKcLCUuBwIecUg4rHFR\n" + + "5OoH9s5UIIvOLA+aGR1gNxx4Jai3IUJtcGS67oh9Gz7F1w6hswO2y0rzXPVq0W+N\n" + + "mAXmEqDpRjqmS6sGHFqtQkKNtc3WRhxc42RD4FiuMuWDkq5//fEEPClg/16i16uF\n" + + "u/17fwq3rnJPQQbxMpxlJp/wJgJdfTNN0eypuvqRMc+4HYELcagtjOX0rBkIO3SG\n" + + "xXqm2uJOCyPMoxWCVZax3+tuZY4onqajxtaz1ztURlbLejxXw4DfEH2CI6VPIc7X\n" + + "bK/Ec5UBnyo1OVOaEcGNLIoQNjxFAgMBAAGjUzBRMB0GA1UdDgQWBBTLRAf/8wQx\n" + + "YLYQMDUW/g+HiamzSDAfBgNVHSMEGDAWgBTLRAf/8wQxYLYQMDUW/g+HiamzSDAP\n" + + "BgNVHRMBAf8EBTADAQH/MA0GCSqGSIb3DQEBCwUAA4IBAQAbc3i28qmW6cbOwpIR\n" + + "OzSgg0BlyK9dOyGrfwRI44i1NEyZGM9Y8ced4AS7DgnZpuKfy54QiibCKxMzENOX\n" + + "kogGgoDriLdDdGfdz2zrFQvHfYa2ccieJ6NV5Bi8Mgnnx+s/DGxZN6Yz76n5/Qic\n" + + "eqmw7pgOMeeqGB5spiOw28INsZK5bxZEcpTyhgPUbhC3EjFp0UMNd7SFstfY7zGo\n" + + "H6t+jC75hgl0PivQC97LrBpzNn0EZCdzoyCUomilR5XEk+L5WIC5H8Z+LxU1hBOS\n" + + "ziEyIosRJFOAv0D4KYNITnCe6km2AzD+AAC5juMXFwaaDYtzmfKUsTFzGGIvC3C8\n" + + "9l5Y\n" + + "-----END CERTIFICATE-----\n"; + function userMaterial(overrides: Partial = {}): CertMaterialV3 { return { kind: "user", name: "corp-ca", thumbprint: "AABBCCDDEEFF", - pemCertBase64: Buffer.from( - "-----BEGIN CERTIFICATE-----\nFAKE\n-----END CERTIFICATE-----\n" - ).toString("base64"), + // A real certificate, not a placeholder: `isCertInstalled` now requires a + // resolvable OpenSSL hash symlink, which can only be derived from a PEM + // whose subject actually parses. + pemCertBase64: Buffer.from(REAL_PEM).toString("base64"), pemKeyBase64: Buffer.from( "-----BEGIN PRIVATE KEY-----\nFAKE\n-----END PRIVATE KEY-----\n" ).toString("base64"), @@ -210,6 +244,104 @@ describe.skipIf(process.platform === "win32")("isCertInstalled", () => { ); }); + it("returns false when the PEM has no resolvable hash symlink (upgrade repair)", () => { + // An install made before the subject hash was computed canonically has its + // symlink under the WRONG hash. Every file is present, so a files-only + // check reports "installed", activation skips the install, and the bad link + // is never repaired — leaving trust broken for exactly the users this fix + // exists for. Reproduced by installing, then renaming the link to a + // wrong-hash name: the state such a container is actually in. + installUserCert(userMaterial()); + expect(isCertInstalled(userMaterial())).toBe(true); + + const links = fs + .readdirSync(trustDir) + .filter((f) => /^[0-9a-f]{8}\.\d+$/.test(f)); + expect(links).toHaveLength(1); + const target = fs.readlinkSync(path.join(trustDir, links[0])); + fs.unlinkSync(path.join(trustDir, links[0])); + fs.symlinkSync(target, path.join(trustDir, "deadbeef.0")); + + expect(isCertInstalled(userMaterial())).toBe(false); + + // Re-running the install repairs it, which is what activation will now do. + installUserCert(userMaterial()); + expect(isCertInstalled(userMaterial())).toBe(true); + }); + + it("returns false when a user cert rotated under the same name", () => { + // User certs are keyed by the user-chosen `name`, not by thumbprint, so a + // rotated certificate lands on the same `{name}.pem`. The stale file's own + // hash link resolves perfectly well, so an existence check would report + // "installed" and the container would keep serving the superseded cert + // indefinitely. Content comparison is what catches it. + installUserCert(userMaterial()); + expect(isCertInstalled(userMaterial())).toBe(true); + + const rotated = userMaterial({ + pemCertBase64: Buffer.from(ROTATED_PEM).toString("base64"), + }); + expect(isCertInstalled(rotated)).toBe(false); + + installUserCert(rotated); + expect(isCertInstalled(rotated)).toBe(true); + expect(fs.readFileSync(path.join(trustDir, "corp-ca.pem"), "utf-8")).toBe( + ROTATED_PEM + ); + }); + + it("returns false when a trusted user cert's .NET Root PFX is missing", () => { + // `installUserCert` writes the Root-store PFX alongside the OpenSSL PEM. + // Checking only the OpenSSL side would call the cert installed after that + // file was deleted, so activation would skip the reinstall and .NET + // clients in the container would keep distrusting it. + installUserCert(userMaterial()); + expect(isCertInstalled(userMaterial())).toBe(true); + + fs.rmSync(path.join(rootStoreDir, "AABBCCDDEEFF.pfx")); + expect(isCertInstalled(userMaterial())).toBe(false); + + installUserCert(userMaterial()); + expect(isCertInstalled(userMaterial())).toBe(true); + }); + + it("does not require a Root PFX the bundle never supplied", () => { + // Gated on `rootPfxBase64`: a bundle without one would never have had the + // file written, so demanding it would loop the install forever. + const noRoot = userMaterial({ rootPfxBase64: undefined }); + installUserCert(noRoot); + expect(isCertInstalled(noRoot)).toBe(true); + }); + + it("does not demand a symlink for a PEM whose subject cannot be hashed", () => { + // `ensureHashSymlink` is a no-op for an unparseable PEM, so reporting "not + // installed" would re-run the install every activation and never converge. + const unhashable = userMaterial({ + pemCertBase64: Buffer.from("-----BEGIN CERTIFICATE-----\nFAKE\n-----END CERTIFICATE-----\n").toString("base64"), + }); + installUserCert(unhashable); + expect(isCertInstalled(unhashable)).toBe(true); + }); + + it("returns false when opted out but a stale store PFX is still on disk", () => { + // The opt-out sweep lives in `installUserCert`'s else-branch, and the + // activation path only calls that when `isCertInstalled` says false. If + // this reported "installed" the sweep would be unreachable and the + // passwordless (plain-text-key) copy would survive the user flipping + // `installUserCertsToDotNetStore` off, forever. + installUserCert( + userMaterial({ + installToDotNetStore: true, + dotNetStorePfxBase64: Buffer.from("OLD").toString("base64"), + }) + ); + expect(fs.existsSync(path.join(storeDir, "AABBCCDDEEFF.pfx"))).toBe(true); + + expect(isCertInstalled(userMaterial({ installToDotNetStore: false }))).toBe( + false + ); + }); + it("returns false for an opted-in user cert when the store file is missing", () => { // Only the trust-dir copy was installed (simulate a half-completed install // or a manual deletion). The check should reflect that the store file is diff --git a/src/vscode-workspace-extension/tests/rehash.test.ts b/src/vscode-workspace-extension/tests/rehash.test.ts index f57eadc..37211d0 100644 --- a/src/vscode-workspace-extension/tests/rehash.test.ts +++ b/src/vscode-workspace-extension/tests/rehash.test.ts @@ -1,8 +1,14 @@ import { describe, it, expect, afterEach } from "vitest"; +import { execFileSync } from "child_process"; import * as fs from "fs"; import * as os from "os"; import * as path from "path"; -import { ensureHashSymlink, rehashDirectory } from "../src/util/rehash"; +import { + computeSubjectHash, + ensureHashSymlink, + hasHashSymlink, + rehashDirectory, +} from "@devcontainer-dev-certs/shared"; // Self-signed test cert; only used to give computeSubjectHash something real // to chew on. The actual hash value doesn't matter — only the symlink shape. @@ -149,27 +155,70 @@ describe.skipIf(process.platform === "win32")("ensureHashSymlink", () => { ); }); - it("returns silently when all 10 hash slots are taken by different PEMs", () => { - // Defensive bound check: 11th install must not throw and must not - // allocate slot 10+ (there's no slot 10 in c_rehash). + it("keeps allocating slots past 10 — every dev cert shares one subject hash", () => { + // This used to assert the opposite: that an 11th same-subject PEM was + // silently refused a slot. That bound was reachable in ordinary use rather + // than pathological, because every ASP.NET dev cert is CN=localhost, so + // slots are consumed by cert COUNT, not by real hash collisions. A + // container minting a fresh cert per rebuild — the default for most dotnet + // devcontainer base images, and not something this extension can enforce + // otherwise — exhausted all ten in ten rebuilds, after which the LIVE cert + // got no symlink while ten dead ones kept theirs, and `openssl verify + // -CApath` failed for the only cert that mattered. OpenSSL's `by_dir` has + // no such limit; it walks `{hash}.{n}` until a file is missing. const dir = tmp(); - // Same content under 10 distinct filenames → same subject hash. for (let i = 0; i < 10; i++) { const name = `collide${i}.pem`; fs.writeFileSync(path.join(dir, name), SAMPLE_PEM_A); ensureHashSymlink(dir, name, SAMPLE_PEM_A); } - const before = listHashSymlinks(dir); - expect(before).toHaveLength(10); + expect(listHashSymlinks(dir)).toHaveLength(10); + + fs.writeFileSync(path.join(dir, "eleventh.pem"), SAMPLE_PEM_A); + ensureHashSymlink(dir, "eleventh.pem", SAMPLE_PEM_A); + + const links = listHashSymlinks(dir); + expect(links).toHaveLength(11); + // The newcomer is the one that has to be reachable. + const mine = links.filter( + (l) => fs.readlinkSync(path.join(dir, l)) === "eleventh.pem" + ); + expect(mine).toHaveLength(1); + }); + + it("throws rather than silently skipping when every slot is taken", () => { + // Returning quietly would let an install report success for a certificate + // OpenSSL cannot find — the same failure the ten-slot bound produced, just + // later. Under the documented rebuild-rotation model with no pruning, the + // bound IS reachable, so it has to be loud. + const dir = tmp(); + for (let i = 0; i < 256; i++) { + const name = `full${i}.pem`; + fs.writeFileSync(path.join(dir, name), SAMPLE_PEM_A); + ensureHashSymlink(dir, name, SAMPLE_PEM_A); + } + expect(listHashSymlinks(dir)).toHaveLength(256); - // 11th attempt — must NOT throw and must NOT create an 11th symlink. fs.writeFileSync(path.join(dir, "overflow.pem"), SAMPLE_PEM_A); - expect(() => - ensureHashSymlink(dir, "overflow.pem", SAMPLE_PEM_A) - ).not.toThrow(); + expect(() => ensureHashSymlink(dir, "overflow.pem", SAMPLE_PEM_A)).toThrow( + /all 256 slots/ + ); + }, 30_000); - const after = listHashSymlinks(dir); - expect(after).toEqual(before); + it("allocates slots contiguously from 0, which is what OpenSSL requires", () => { + // `by_dir` stops at the first missing `{hash}.{n}`, so a gap makes every + // later slot unreachable. Anything that prunes entries must re-densify + // via rehashDirectory rather than unlink in place. + const dir = tmp(); + for (let i = 0; i < 12; i++) { + const name = `c${i}.pem`; + fs.writeFileSync(path.join(dir, name), SAMPLE_PEM_A); + ensureHashSymlink(dir, name, SAMPLE_PEM_A); + } + const suffixes = listHashSymlinks(dir) + .map((l) => Number(l.split(".")[1])) + .sort((a, b) => a - b); + expect(suffixes).toEqual([...Array(12).keys()]); }); it("leaves pre-existing hash symlinks for OTHER PEMs untouched", () => { @@ -200,3 +249,381 @@ describe.skipIf(process.platform === "win32")("ensureHashSymlink", () => { ); }); }); + +/** + * The symlink names only do anything if they match what OpenSSL's `by_dir` + * lookup (SSL_CERT_DIR / -CApath) actually searches for. That value is + * `X509_NAME_hash`: SHA-1 over the *canonical* name encoding — attribute + * values re-tagged UTF8String, ASCII-lowercased, space runs collapsed, and + * the RDN `SET OF` encodings concatenated WITHOUT the Name's outer + * `SEQUENCE`. Hashing the raw subject DER instead yields a plausible-looking + * `{hash}.0` that nothing ever opens, silently disabling container trust. + * + * The expected values below were produced by `openssl x509 -hash -noout` + * (OpenSSL 3.0.13). `CN=localhost` is the shape every dev cert we install + * has; the multi-RDN fixture pins the normalization rules (PrintableString + * plus UTF8String, uppercase letters, a doubled internal space, a trailing + * space). + */ +describe("computeSubjectHash", () => { + const hasOpenssl = (() => { + try { + execFileSync("openssl", ["version"], { stdio: "ignore" }); + return true; + } catch { + return false; + } + })(); + + // subject=CN = localhost + const PEM_LOCALHOST = + "-----BEGIN CERTIFICATE-----\n" + + "MIIDCTCCAfGgAwIBAgIUKqotkm31fbIEbOVcgrem0favrgQwDQYJKoZIhvcNAQEL\n" + + "BQAwFDESMBAGA1UEAwwJbG9jYWxob3N0MB4XDTI2MDgyODAwMDM1OFoXDTM2MDgy\n" + + "NTAwMDM1OFowFDESMBAGA1UEAwwJbG9jYWxob3N0MIIBIjANBgkqhkiG9w0BAQEF\n" + + "AAOCAQ8AMIIBCgKCAQEAjgGYX2B2v2F5mSgDK2skLTZ7WtkYEJXZ/dD3i4Io5ZuQ\n" + + "5z4nt6VPSnCZFe8jBcDqcgdnCWUOG8yo7BP0pMQHMNRcqmyfMssIKWenPSPWU3U1\n" + + "qMkah8hJbzQkuPlL88yBRDGlHI5ioE6YJKkvwaXBEpaj7xwL0IeOg7ODBz/C6lev\n" + + "KGqfh8180tJ2/SJc6Hpgi0aaWFmkaYyB2/xZnxGTOaXlYtaU1WLVHSG0pJUdYEAm\n" + + "m8S/oaofwPNEG/GStb+X5NVQKxQS2ZhsPcrv55EoZ43ukRwvUCeE1jN0xAVx9KO6\n" + + "1PzYWxGwrneCv45VV+698LstLLn9tWL0FAe0MWxfcwIDAQABo1MwUTAdBgNVHQ4E\n" + + "FgQUszuVse2bqDyPBDxDgwodnoWFiSowHwYDVR0jBBgwFoAUszuVse2bqDyPBDxD\n" + + "gwodnoWFiSowDwYDVR0TAQH/BAUwAwEB/zANBgkqhkiG9w0BAQsFAAOCAQEAd8fg\n" + + "cVxi0bb27kpCjCBBkWGJkfu2SpY8D345PPvsQfxEoaBmvmPSo+V0uO5vPM6VQkMb\n" + + "nwOyGytTYM+uVWADA3YJ+gYpToRfWE+06hKh2ziCDves8rObymLHApFosU0ulT35\n" + + "HWw7S1Sv68k4Wqh7Q7neaYdKGjXWIpMbQ/aDUkUSRYYdmCyidmxAJFi71ROmkl0N\n" + + "SutU65eZyiU8Rh6GSn1u3iPn+DHtcI/3npplew/kXUSliw4gpI7lipD31uBHVJc+\n" + + "k8ge6yTGRi5QppCpiSYcpv0MJ1+DdaadFkYjOV4DPXid9xeJ7ZwQX2rK6Zbkj36Z\n" + + "dW1E/BkFPJeKGPofjA==\n" + + "-----END CERTIFICATE-----\n"; + + // subject=C = US, O = "Example Org ", CN = Mixed Case Name + const PEM_MULTI_RDN = + "-----BEGIN CERTIFICATE-----\n" + + "MIIDXzCCAkegAwIBAgIUbKzt8uWkwdhKI7QVANKvuaAuga4wDQYJKoZIhvcNAQEL\n" + + "BQAwPzELMAkGA1UEBhMCVVMxFjAUBgNVBAoMDUV4YW1wbGUgIE9yZyAxGDAWBgNV\n" + + "BAMMD01peGVkIENhc2UgTmFtZTAeFw0yNjA4MjgwMDAzNThaFw0zNjA4MjUwMDAz\n" + + "NThaMD8xCzAJBgNVBAYTAlVTMRYwFAYDVQQKDA1FeGFtcGxlICBPcmcgMRgwFgYD\n" + + "VQQDDA9NaXhlZCBDYXNlIE5hbWUwggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEK\n" + + "AoIBAQDLuNsJ2dI5mBGcGeK5lfzKA/8dY5Dunjl10gZybeKcLCUuBwIecUg4rHFR\n" + + "5OoH9s5UIIvOLA+aGR1gNxx4Jai3IUJtcGS67oh9Gz7F1w6hswO2y0rzXPVq0W+N\n" + + "mAXmEqDpRjqmS6sGHFqtQkKNtc3WRhxc42RD4FiuMuWDkq5//fEEPClg/16i16uF\n" + + "u/17fwq3rnJPQQbxMpxlJp/wJgJdfTNN0eypuvqRMc+4HYELcagtjOX0rBkIO3SG\n" + + "xXqm2uJOCyPMoxWCVZax3+tuZY4onqajxtaz1ztURlbLejxXw4DfEH2CI6VPIc7X\n" + + "bK/Ec5UBnyo1OVOaEcGNLIoQNjxFAgMBAAGjUzBRMB0GA1UdDgQWBBTLRAf/8wQx\n" + + "YLYQMDUW/g+HiamzSDAfBgNVHSMEGDAWgBTLRAf/8wQxYLYQMDUW/g+HiamzSDAP\n" + + "BgNVHRMBAf8EBTADAQH/MA0GCSqGSIb3DQEBCwUAA4IBAQAbc3i28qmW6cbOwpIR\n" + + "OzSgg0BlyK9dOyGrfwRI44i1NEyZGM9Y8ced4AS7DgnZpuKfy54QiibCKxMzENOX\n" + + "kogGgoDriLdDdGfdz2zrFQvHfYa2ccieJ6NV5Bi8Mgnnx+s/DGxZN6Yz76n5/Qic\n" + + "eqmw7pgOMeeqGB5spiOw28INsZK5bxZEcpTyhgPUbhC3EjFp0UMNd7SFstfY7zGo\n" + + "H6t+jC75hgl0PivQC97LrBpzNn0EZCdzoyCUomilR5XEk+L5WIC5H8Z+LxU1hBOS\n" + + "ziEyIosRJFOAv0D4KYNITnCe6km2AzD+AAC5juMXFwaaDYtzmfKUsTFzGGIvC3C8\n" + + "9l5Y\n" + + "-----END CERTIFICATE-----\n"; + + // --- Minimal hand-built certs, so a subject can use any ASN.1 string type --- + // `computeSubjectHash` only walks as far as the subject, so nothing past it + // (or a valid signature) is needed. `openssl req` can't be coaxed into every + // encoding, and splicing real certs would obscure what is under test. + + function der(tag: number, content: Buffer): Buffer { + const len = content.length; + let lenBytes: Buffer; + if (len < 0x80) { + lenBytes = Buffer.from([len]); + } else { + const bytes: number[] = []; + let remaining = len; + while (remaining > 0) { + bytes.unshift(remaining & 0xff); + remaining >>>= 8; + } + lenBytes = Buffer.from([0x80 | bytes.length, ...bytes]); + } + return Buffer.concat([Buffer.from([tag]), lenBytes, content]); + } + + /** + * A certificate whose subject is a single CN with the given encoding. + * + * Deliberately a COMPLETE Certificate — signatureAlgorithm, a dummy + * signatureValue and a placeholder subjectPublicKeyInfo included — even + * though `computeSubjectHash` stops reading at the subject. Being parseable + * by `openssl x509` is what lets the expected hashes below be derived from + * OpenSSL rather than from this implementation, which would be circular. + */ + function certWithCn(valueTag: number, value: Buffer): string { + const oidCommonName = Buffer.from("0603550403", "hex"); + const oidSha256Rsa = Buffer.from("06092a864886f70d01010b", "hex"); + const oidRsaEncryption = Buffer.from("06092a864886f70d010101", "hex"); + const algorithmId = der( + 0x30, + Buffer.concat([oidSha256Rsa, der(0x05, Buffer.alloc(0))]) + ); + const name = der( + 0x30, + der(0x31, der(0x30, Buffer.concat([oidCommonName, der(valueTag, value)]))) + ); + // Placeholder SPKI: d2i decodes it as AlgorithmIdentifier + BIT STRING and + // only parses the key material lazily, so a stand-in modulus is fine. + const spki = der( + 0x30, + Buffer.concat([ + der(0x30, Buffer.concat([oidRsaEncryption, der(0x05, Buffer.alloc(0))])), + der( + 0x03, + Buffer.concat([ + Buffer.from([0x00]), + der( + 0x30, + Buffer.concat([ + der(0x02, Buffer.from([0x01, 0x00])), + der(0x02, Buffer.from([0x01, 0x01])), + ]) + ), + ]) + ), + ]) + ); + const tbs = der( + 0x30, + Buffer.concat([ + der(0xa0, der(0x02, Buffer.from([0x02]))), // version v3 + der(0x02, Buffer.from([0x01])), // serial + algorithmId, + name, // issuer + der( + 0x30, + Buffer.concat([ + der(0x17, Buffer.from("260101000000Z", "ascii")), + der(0x17, Buffer.from("360101000000Z", "ascii")), + ]) + ), + name, // subject + spki, + ]) + ); + const body = der( + 0x30, + Buffer.concat([ + tbs, + algorithmId, + der(0x03, Buffer.concat([Buffer.from([0x00]), Buffer.alloc(8)])), + ]) + ) + .toString("base64") + .replace(/(.{64})/g, "$1\n"); + return `-----BEGIN CERTIFICATE-----\n${body}\n-----END CERTIFICATE-----\n`; + } + + /** UTF-32BE bytes for a run of code points, i.e. an ASN.1 UniversalString. */ + function utf32be(...codePoints: number[]): Buffer { + const buf = Buffer.alloc(codePoints.length * 4); + codePoints.forEach((cp, i) => buf.writeUInt32BE(cp, i * 4)); + return buf; + } + + const certWithUtf8Cn = (cn: string): string => + certWithCn(0x0c, Buffer.from(cn, "utf8")); + + it("matches OpenSSL's subject hash for a CN=localhost dev cert", () => { + expect(computeSubjectHash(PEM_LOCALHOST)).toBe("ce275665"); + }); + + it("matches OpenSSL's subject hash for a multi-RDN subject needing canonicalization", () => { + expect(computeSubjectHash(PEM_MULTI_RDN)).toBe("90c9c9f3"); + }); + + /** + * Canonicalization cases that a byte-level reading of `asn1_string_canon` + * gets wrong. Values are the raw ASN.1 attribute bytes as stored, and every + * expected hash came from `openssl x509 -hash` on a real certificate. + */ + it("folds every ASCII whitespace byte, not just 0x20", () => { + // CN stored as "a\tb", "a\nb", "a b" and "A B" all canonicalize to + // "a b", so OpenSSL returns one hash for the lot. Handling only 0x20 gave + // the tab and newline forms distinct — and unusable — hashes. + for (const cn of ["a\tb", "a\nb", "a b", "A B"]) { + expect(computeSubjectHash(certWithUtf8Cn(cn))).toBe("49cdc5e0"); + } + }); + + it("trims leading and trailing whitespace of any ASCII kind", () => { + for (const cn of ["a", " a", "\ta", "a ", "a\n"]) { + expect(computeSubjectHash(certWithUtf8Cn(cn))).toBe("20b69a40"); + } + }); + + it("transcodes T61String to UTF-8 before folding", () => { + // A T61String holding the UTF-8 bytes of 日本語. Re-tagging those bytes as + // UTF8String and hashing yields e2c402e4; OpenSSL says 02c4fa54, which is + // reached only by reading them as Latin-1 and re-encoding as UTF-8. + const cn = Buffer.from("日本語", "utf8").toString("latin1"); + expect(computeSubjectHash(certWithCn(0x14, Buffer.from(cn, "latin1")))).toBe( + "02c4fa54" + ); + }); + + it("transcodes BMPString (UTF-16BE) to UTF-8 before folding", () => { + // Bytes lifted verbatim from a cert built with `string_mask = MASK:2048`, + // whose hash `openssl x509 -hash` reports as b02e8735. (openssl stored each + // byte of the UTF-8 input as its own UTF-16 code unit, so this is mojibake + // rather than the text that went in — which makes it a better fixture: the + // bytes are what matter, and they exercise the non-ASCII path.) + const utf16be = Buffer.from( + "0054006500730074002000c3009c006e00c300af0063006f00640065", + "hex" + ); + expect(computeSubjectHash(certWithCn(0x1e, utf16be))).toBe("b02e8735"); + }); + + it("hasHashSymlink steps over a regular file occupying a slot", () => { + // `ensureHashSymlink` treats a non-symlink `{hash}.N` as OCCUPIED and puts + // our link in a later slot; OpenSSL's `by_dir` likewise processes whatever + // it finds and keeps walking. Reading a regular file with readlink raises + // EINVAL, so treating any error as end-of-search would stop short of our + // link and wrongly report the certificate as unlinked — which, now that + // "installed" and "trusted" both consult this, means a perpetual reinstall. + const dir = tmp(); + fs.writeFileSync(path.join(dir, "mycert.pem"), SAMPLE_PEM_A); + ensureHashSymlink(dir, "mycert.pem", SAMPLE_PEM_A); + const slot0 = listHashSymlinks(dir)[0]; + + // Replace slot 0 with a squatting regular file and re-link into slot 1. + fs.unlinkSync(path.join(dir, slot0)); + fs.writeFileSync(path.join(dir, slot0), "not-a-symlink"); + ensureHashSymlink(dir, "mycert.pem", SAMPLE_PEM_A); + expect(fs.lstatSync(path.join(dir, slot0)).isSymbolicLink()).toBe(false); + + expect(hasHashSymlink(dir, "mycert.pem", SAMPLE_PEM_A)).toBe(true); + }); + + it("hasHashSymlink stops at a genuinely missing slot", () => { + // A real gap is where OpenSSL stops, so we must too — otherwise the scan + // would keep probing past the point `by_dir` gives up. + const dir = tmp(); + fs.writeFileSync(path.join(dir, "mycert.pem"), SAMPLE_PEM_A); + expect(hasHashSymlink(dir, "mycert.pem", SAMPLE_PEM_A)).toBe(false); + }); + + it("transcodes UniversalString (UTF-32BE) to UTF-8 before folding", () => { + // "Tëst". Expected value from `openssl x509 -hash` on this exact cert. + const universal = utf32be(0x54, 0xeb, 0x73, 0x74); + expect(computeSubjectHash(certWithCn(0x1c, universal))).toBe("ba8aa3f2"); + }); + + it("gives UniversalString and UTF8String of the same text the same hash", () => { + // The point of transcoding: encoding must not change identity. Without it + // the UTF-32BE bytes would be hashed raw and these would diverge. + const universal = certWithCn(0x1c, utf32be(0x54, 0xeb, 0x73, 0x74)); + const utf8 = certWithCn(0x0c, Buffer.from("Tëst", "utf8")); + expect(computeSubjectHash(universal)).toBe(computeSubjectHash(utf8)); + }); + + it.runIf(hasOpenssl)( + "agrees with openssl on every hand-built encoding fixture", + () => { + // The recorded values above are only trustworthy if OpenSSL still agrees + // with them, so re-derive rather than trusting the constants. + const dir = tmp(); + const cases: [string, string][] = [ + ["utf8", certWithCn(0x0c, Buffer.from("Tëst", "utf8"))], + ["universal", certWithCn(0x1c, utf32be(0x54, 0xeb, 0x73, 0x74))], + [ + "bmp", + certWithCn( + 0x1e, + Buffer.from( + "0054006500730074002000c3009c006e00c300af0063006f00640065", + "hex" + ) + ), + ], + [ + "t61", + certWithCn( + 0x14, + Buffer.from(Buffer.from("日本語", "utf8").toString("latin1"), "latin1") + ), + ], + ["printable-ws", certWithCn(0x13, Buffer.from("A B", "ascii"))], + ]; + for (const [label, pem] of cases) { + const file = path.join(dir, `${label}.pem`); + fs.writeFileSync(file, pem); + const expected = execFileSync("openssl", [ + "x509", + "-hash", + "-noout", + "-in", + file, + ]) + .toString() + .trim(); + expect(`${label}=${computeSubjectHash(pem)}`).toBe( + `${label}=${expected}` + ); + } + } + ); + + it("returns null for input that isn't a certificate", () => { + expect(computeSubjectHash("not a pem")).toBeNull(); + }); + + it("stays linear on a BEGIN marker followed by whitespace and no END", () => { + // CodeQL js/polynomial-redos, high: the old + // `/-----BEGIN CERTIFICATE-----\s*([\s\S]*?)\s*-----END CERTIFICATE-----/` + // wrapped a lazy `[\s\S]*?` in two `\s*` quantifiers. All three match + // whitespace, so this exact shape — BEGIN, a long run of spaces, no END — + // gave the engine an ambiguous split to backtrack over and matching went + // quadratic. `rehashDirectory` hands this function every *.pem file in the + // OpenSSL trust directory, none of which is guaranteed well-formed. + // + // 100k spaces is ~10^10 backtracking steps under the old regex, so a + // regression hangs this test rather than merely slowing it; the assertion + // is that we finish at all. + const pathological = `-----BEGIN CERTIFICATE-----${" ".repeat(100_000)}`; + const started = Date.now(); + expect(computeSubjectHash(pathological)).toBeNull(); + expect(Date.now() - started).toBeLessThan(1000); + }, 10_000); + + it("still reads a normal PEM, and takes the first cert when several are present", () => { + expect(computeSubjectHash(PEM_LOCALHOST)).toBe("ce275665"); + // Concatenated PEMs: the first BEGIN pairs with the first following END, + // matching the old lazy-quantifier behaviour. + expect(computeSubjectHash(PEM_LOCALHOST + PEM_MULTI_RDN)).toBe("ce275665"); + expect(computeSubjectHash(PEM_MULTI_RDN + PEM_LOCALHOST)).toBe("90c9c9f3"); + }); + + it("returns null when a marker is missing or the body is empty", () => { + expect(computeSubjectHash(PEM_LOCALHOST.replace("-----END CERTIFICATE-----", ""))).toBeNull(); + expect(computeSubjectHash("-----BEGIN CERTIFICATE-----\n-----END CERTIFICATE-----\n")).toBeNull(); + }); + + // Belt-and-braces: when the machine running the suite has openssl, verify + // the pinned values above still reflect what OpenSSL computes today rather + // than what it computed when they were recorded. + + it.runIf(hasOpenssl)( + "agrees with the local openssl binary", + () => { + const dir = tmp(); + for (const pem of [PEM_LOCALHOST, PEM_MULTI_RDN]) { + const p = path.join(dir, "cert.pem"); + fs.writeFileSync(p, pem); + const expected = execFileSync("openssl", [ + "x509", + "-hash", + "-noout", + "-in", + p, + ]) + .toString() + .trim(); + expect(computeSubjectHash(pem)).toBe(expected); + } + } + ); +}); diff --git a/test/sample-project/Project.AppHost/Project.AppHost.csproj b/test/sample-project/Project.AppHost/Project.AppHost.csproj index 74a69f1..09c4548 100644 --- a/test/sample-project/Project.AppHost/Project.AppHost.csproj +++ b/test/sample-project/Project.AppHost/Project.AppHost.csproj @@ -1,4 +1,4 @@ - + Exe