Support Linux in ts dev proxy - #1171
ChristianPavilonis wants to merge 2 commits into
Conversation
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
Extends ts dev proxy from macOS-only to macOS + Linux: dependency and cfg gates widened, Chrome/Chromium and Firefox launcher discovery added, and a new trust.rs that manages the dev CA through the user's NSS database. The CA-trust work is the substantial part, and it is careful — imports are journalled before NSS is touched, full DER equality authorizes every mutation, and two lock layers serialize CA commands and shared-store writes.
No blocking findings. The comments below are design observations, one seedling, and documentation drift; none of them gate the merge.
All comments are prose — nothing is offered as a one-click suggestion, because the two documentation lines worth changing fall outside the diff hunks, the script fix is a file-mode change, and the remainder are judgement calls that should stay the author's.
Non-blocking
🤔 thinking
- Whole-store NSS export scan hard-fails on any single bad entry — see inline at
crates/trusted-server-cli/src/commands/dev/proxy/trust.rs:283 - Every
try_lockfailure reports as "another operation is running" — see inline atcrates/trusted-server-cli/src/commands/dev/proxy/trust.rs:59 - A rejected install still creates a new NSS store — see inline at
crates/trusted-server-cli/src/commands/dev/proxy/trust.rs:332
🌱 seedling
- Persisted nickname derives from
DefaultHasher— see inline atcrates/trusted-server-cli/src/commands/dev/proxy/trust.rs:339
⛏ nitpick
ConfigError::Browseromitssafarion macOS — see inline atcrates/trusted-server-cli/src/commands/dev/proxy/config.rs:50- Firefox trust flags drifted from the documented manual command — below
- The guide's
--helptranscript is now wrong on Linux — below - New script is not executable — below
👍 praise
- Trust-mutation design and its failure-path assertions — see inline at
crates/trusted-server-cli/tests/proxy_trust_linux.rs:76
Cross-cutting / body-level findings
-
⛏ Firefox trust flags drifted from the documented manual command —
import_firefox(trust.rs:94) narrowed the NSS trust flags fromCT,,(the previousbrowser.rscode) toC,,. Fine on the merits: server-authentication CA trust is all a proxy needs, and dropping the client-auth bit is tighter. But the manual import command indocs/guide/ts-dev-proxy.md:200still says-t "CT,,", so a developer following the documented path now grants different trust bits than--launch firefoxdoes. Either align the doc block toC,,or note why they differ. (Body-level: line 200 is outside the diff hunks, so it can't carry an inline comment.) -
⛏ The guide's
--helptranscript is now wrong on Linux —docs/guide/ts-dev-proxy.md:347advertises--launch <LIST> Browsers to launch (chrome,firefox,safari or all), butsafariis rejected on Linux by the newcfginBrowser::parse_list. Line 355's--ca-dirdefault reads~/Library/Application Support/trusted-server/dev-proxy on macOSwith no Linux equivalent, while the prose added above it documents$XDG_DATA_HOME/trusted-server/dev-proxy. Separately, and pre-existing onmain: neither line matches clap's real output — the actual--launchhelp string is "Browsers to launch + configure (comma list orall)", andca_diris anOption<String>with nodefault_value, so clap prints no default at all. The transcript is hand-maintained and already drifting; this PR is a reasonable moment to regenerate or trim it. -
⛏
scripts/test-linux-dev-proxy-browser.pyis mode644— every other file inscripts/is755. It carries a#!/usr/bin/env python3shebang but cannot be executed directly. Nothing breaks, since the guide invokes it aspython3 scripts/..., butgit update-index --chmod=+xwould make it consistent withtest-cli.shand friends.
CI Status
- cargo test (ts CLI, native) (ubuntu-latest): PASS
- cargo test (ts CLI, native) (macos-latest): PASS
- cargo fmt: PASS (required)
- cargo test: PASS (required)
- format-docs: PASS (required)
- format-typescript: PASS (required)
- cargo test (axum native): PASS
- cargo test (cross-adapter parity): PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- vitest: PASS
- prepare integration artifacts: PASS
- CLAUDE.md symlink guard: PASS
- Analyze (actions): PASS
- Analyze (javascript-typescript): PASS
- CodeQL: SKIPPED
- Analyze (rust): PENDING
- integration tests: PENDING
- integration tests (Fastly EC lifecycle): PENDING
- browser integration tests: PENDING
No failed checks. Four checks were still in progress at review time; the Linux and macOS CLI jobs this PR actually exercises have both passed.
|
|
||
| /// NSS named exports include every certificate with the same subject. | ||
| /// Reject a different certificate with that subject before importing anything. | ||
| fn check_subject_conflicts(entry: &Destination) -> Result<()> { |
There was a problem hiding this comment.
🤔 thinking — ca install scans and exports every nickname in the shared user NSS database, and hard-fails on any one of them.
check_subject_conflicts walks all nicknames from nicknames() and runs certutil -L -n <nick> -a per entry. Any failure — a duplicate nickname, an orphaned entry, a cert whose named export isn't parseable PEM — propagates as TrustError::Command / TrustError::State and aborts the install.
On the fixture databases in tests/proxy_trust_linux.rs that is fine. On a real developer's ~/.pki/nssdb (corporate roots, leftovers from mkcert or earlier nss-tools experiments) a single bad entry makes ca install permanently unusable, and there is no documented recovery path other than editing the NSS store by hand — which docs/guide/ts-dev-proxy.md tells the user not to do.
Fail-closed is right for an identity check, but a per-nickname export failure isn't an identity conflict. Two options that keep the guarantee:
- Only hard-fail when a successfully-parsed certificate has a matching subject and different bytes; treat an unparseable/failed export as "not a conflict for this subject" and warn.
- Keep the strict behaviour and document the escape hatch (which nickname failed, and what to do about it) — the
checked()attach already carries the certutil stderr, so the diagnostic exists, it just isn't actionable in the guide.
| .mode(0o600) | ||
| .open(path) | ||
| .change_context(TrustError::Io)?; | ||
| file.try_lock().change_context(TrustError::Busy)?; |
There was a problem hiding this comment.
🤔 thinking — every try_lock failure is reported as "another operation is running".
TryLockError covers WouldBlock and genuine I/O failures — ENOLCK, or a filesystem that doesn't support advisory locks. NFS-mounted home directories are a live case on Linux, which is the platform this PR is adding.
Both map to TrustError::Busy, so the user is told another CA or NSS trust operation is running; retry after it exits and retries forever against a condition that will never clear. Splitting the two keeps the diagnostic honest:
match file.try_lock() {
Ok(()) => {}
Err(std::fs::TryLockError::WouldBlock) => return Err(Report::new(TrustError::Busy)),
Err(err) => return Err(Report::new(err).change_context(TrustError::Io)),
}Apply manually — worth confirming the exact TryLockError variant names against the pinned 1.95 toolchain before committing to this shape.
|
|
||
| fn install_into(ca_dir: &Path, cert_path: &Path, database: &Path) -> Result<()> { | ||
| let mut entries = read_record(ca_dir)?; | ||
| fs::create_dir_all(database).change_context(TrustError::Io)?; |
There was a problem hiding this comment.
🤔 thinking — a rejected install still leaves a freshly created NSS store behind.
install_into runs fs::create_dir_all(database) here and initialize() (which runs certutil -N) on the next line, both before the contains() / check_subject_conflicts() preflight further down. So an install rejected for a same-subject conflict creates ~/.local/share/pki/nssdb with a new cert9.db / key4.db that did not exist beforehand.
It's benign — browsers create that store themselves — but it's a side effect of a command that otherwise advertises "rejected before mutation", and real_nss_same_subject_install_conflict_is_rejected_before_mutation doesn't cover it because the database already exists in that fixture. If the ordering can't move (the preflight needs a queryable database), it may be worth saying so in a comment here.
| initialize(&database)?; | ||
| let certificate = certificate(cert_path)?; | ||
| // The hash only names the entry. Full DER equality authorizes all mutations. | ||
| let mut hash = DefaultHasher::new(); |
There was a problem hiding this comment.
🌱 seedling — the persisted nickname is derived from DefaultHasher, whose output isn't stable across Rust releases.
The resulting ts-dev-proxy-{:016x} string is written to managed-nss-trust.json and into the NSS database. Today that's safe, and the comment right above says why: the entry lookup keys on (database, certificate) and uninstall replays the recorded nickname, so cross-version stability is never required on the happy path.
The one case it bites is recovery: journal lost, or a different --ca-dir used, while the NSS import survives. The orphan can then only be re-derived under the exact toolchain that wrote it, so ca uninstall can never reach it. A stable digest over the DER — SHA-256 is already reachable through the rustls/ring stack this crate pulls in — would make that recoverable.
Not for this PR; the 16-hex-digit shape read_record validates would be unchanged either way.
| /// An unknown browser name was passed to `--launch`. | ||
| #[display("unknown browser `{value}` (expected chrome|firefox|safari|all)")] | ||
| /// An unknown or unsupported browser was passed to `--launch`. | ||
| #[display("unsupported browser `{value}` (use chrome|firefox|all; safari is macOS-only)")] |
There was a problem hiding this comment.
⛏ nitpick — the message drops safari from the accepted list on macOS, where it is accepted.
A macOS user who typos --launch chrom is told to "use chrome|firefox|all", which reads as though safari isn't an option on the platform they're actually on. The trailing clause is doing double duty as both a restriction and an implicit list member.
Something that reads correctly on both targets without needing a cfg:
#[display("unsupported browser `{value}` (use chrome|firefox|all, plus safari on macOS)")]Apply manually — left as prose rather than a one-click suggestion so the wording stays yours.
| bin | ||
| } | ||
|
|
||
| fn assert_rotation_fails_unchanged(&self, path: &Path) { |
There was a problem hiding this comment.
👍 praise — assert_rotation_fails_unchanged re-asserting the key, the certificate and the journal bytes on every failure path is the right shape for testing a trust-store mutator: it turns "rotation never proceeds on an unconfirmed revoke" into a property rather than a comment.
Same for the design it's testing — record-before-mutate journal, full DER equality (not the nickname) as the authorization check, a CA-directory lock plus a per-NSS-directory lock so distinct --ca-dirs serialize against a shared store, and the same-subject preflight that stops NSS's named-export ambiguity from silently rebinding someone else's certificate. That last one is a genuinely non-obvious NSS behaviour to have found and closed before shipping.
2 participants
Summary
ts dev proxyavailable on Linux. It was previously excluded at compile time even though the proxy engine can run natively on Linux.Scope
This is a CLI-only platform extension, not a proxy-engine rewrite. The changes span command registration, browser launch, certificate trust, tests, CI, and documentation because removing the compile gates alone would leave Linux browsers without working trust management.
Most of the new code and tests cover safe certificate installation and removal. A reproduced NSS behavior required an extra check: importing a different certificate with the same subject can make the original certificate's nickname export ambiguous. Installation now rejects that conflict before import, and shared-database locks serialize
tstrust changes.Supported automation is limited to native Chrome/Chromium and Firefox. Safari stays macOS-only. Windows, Snap/Flatpak automation, root operations, and desktop-wide proxy or certificate-store changes are excluded. Known packaging paths are skipped, but arbitrary shell wrappers are not classified.
Changes
CLI paths below are relative to
crates/trusted-server-cli/; other paths are repository-relative..github/workflows/test.ymlCargo.tomlsrc/lib.rssrc/run.rssrc/commands/dev/mod.rssrc/commands/dev/proxy/mod.rssrc/commands/dev/proxy/browser.rssrc/commands/dev/proxy/config.rssrc/commands/dev/proxy/trust.rstests/proxy_cli.rstests/proxy_e2e.rstests/proxy_perf.rstests/proxy_trust_linux.rstests/proxy_trust_macos.rsscripts/test-linux-dev-proxy-browser.pydocs/guide/ts-dev-proxy.mddocs/superpowers/plans/linux-dev-proxy.mdCloses
Closes #1170
Test plan
./scripts/test-cli.sh: 182 unit tests and 42 integration/configuration tests passed.cargo test_cli_linux --test proxy_trust_linux -- --include-ignored: all 11 trust tests passed, including real NSS checks.cargo clippy --package trusted-server-cli --target x86_64-unknown-linux-gnu --all-targets -- -D warningscargo fmt --all -- --checkcd docs && npm run format, with the existing JS dependency directory added to PATH because docs dependencies were not installed separately.All real certificate/browser tests used disposable HOME, XDG, CA, profile, and NSS directories. They did not modify the developer's real trust stores or disable TLS verification or the browser sandbox. Adapter, JavaScript, and WASM suites were not rerun because those implementations are unchanged. Manual performance workloads remain opt-in.
Checklist
unwrap()in production code.