Add ts dev proxy: a local MITM dev proxy serving production hostnames from staging#798
Merged
Conversation
- Compare r.from case-insensitively in RuleTable::first_match to enforce the lowercase invariant regardless of how Rule.from was built - Reject trailing-colon inputs (empty port string) as RuleError::Port in Authority::parse; add rejects_empty_or_missing_port test - Assert scheme_is_tls in rewrite_default_preserves_from_host_and_sets_sni_to_to and rewrite_host_uses_to_authority_with_port
Add ConfigError::BasicAuthFile variant with a path-carrying display message so file-not-found and permission errors are no longer reported as the misleading "--basic-auth must be USER:PASS" format error. The read failure now maps to BasicAuthFile; only parse failures map to BasicAuth. Includes a unit test that asserts the correct variant is returned for a missing file.
Replace the write-then-chmod pattern in CertAuthority::persist() with a single OpenOptions::create_new(true).mode(0o600) open, eliminating the window where the private key was briefly world/group-readable on disk.
…NNECT - Thread the raw buffered bytes through `RequestHead` so `blind_forward_http` can write the complete request head to the upstream before piping the rest of the socket bidirectionally (spec §8.4). Previously the head was discarded, sending a truncated/empty request. - Update `blind_forward_http` doc comment to reflect that it now replays the original head rather than falsely claiming it always did. - Add `unmatched_connect_off_loopback_is_refused_with_403` integration test. The proxy listener is bound on `127.0.0.1:0` (real socket) but `cfg.listen` is patched to `0.0.0.0:<port>` before being handed to `serve_on`, so `is_loopback` is computed as false while the test can still connect via loopback. Asserts that an unmatched `CONNECT` receives `403` and no tunnel is established.
…s-dev-proxy-spec # Conflicts: # Cargo.toml
Replace the dead `std::thread::park()` restore thread in `launch_safari`
with a file-based persist-and-recover scheme. Before applying the PAC
URL, write `<ca_dir>/safari-proxy-restore` capturing the network service
name and the prior auto-proxy URL (or an empty second line when
auto-proxy was off). Add `restore_system_proxy_if_pending(ca_dir)`,
which reads and deletes that file then runs the appropriate `networksetup`
command to put things back.
Wire it into `run()` in two places: at startup (crash recovery from a
previous hard-killed run) and in a `tokio::select!` on `ctrl_c()` (clean
exit). Also move the function-local `use std::time::{SystemTime,
UNIX_EPOCH}` out of `make_temp_dir` to the top-level imports per project
convention.
…-spec' into worktree-review-ts-dev-proxy-spec
This was referenced Jun 26, 2026
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
Collaborator
There was a problem hiding this comment.
Automated review: I reviewed the new trusted-server-cli dev proxy implementation, CA/browser orchestration, CI wiring, and the related docs. CI is currently green and I did not find a blocking issue that requires requesting changes, but I found several correctness/security edge cases that should be addressed before relying on this tool broadly.
…v-proxy-spec # Conflicts: # .github/workflows/test.yml # .gitignore
…fixes - ca regenerate: propagate old cert/key deletion failures (tolerate already-absent) instead of `.ok()`, so a failed delete can't leave the stale key silently in use after printing "regenerated" (P1). - config: reject --basic-auth/--basic-auth-file on a non-loopback --listen; a matched CONNECT is MITM'd even off loopback, so injected credentials would be exposed to any reachable client (P1). - server: stamp an authoritative `X-Forwarded-Proto: https` (the browser leg is always TLS) and drop spoofable `Fastly-SSL`, so `--upstream-plaintext` (or a spoofed header) can't downgrade the first-party scheme (P2). - browser (Firefox): initialise an empty `sql:` NSS DB before `certutil -A`, and import into it, instead of importing against an uninitialised profile DB that fails with SEC_ERROR_BAD_DATABASE (P2). - browser: derive the browser-connect address from `cfg.listen` (normalising wildcard binds to loopback, bracketing IPv6) and use it consistently in Chrome/Firefox/Safari/PAC and the manual hints, instead of hard-coding 127.0.0.1 — so a non-default --listen is honored (P2). Adds unit tests for each.
Collaborator
Author
|
Thanks for the review — all five findings addressed in
Unit tests added for each; CLI crate fmt/clippy/test green. |
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
Collaborator
There was a problem hiding this comment.
Automated review: I reviewed the current head of PR #798 (new trusted-server-cli dev proxy, CA/browser orchestration, tests/CI, and docs) against main. CI is green and I did not find a blocking correctness/security issue requiring REQUEST_CHANGES, but I found several medium-risk hardening/correctness issues worth addressing before broad use of this MITM dev tool.
…form - server: strip RFC 7230 hop-by-hop request headers and every header named in an inbound `Connection` token before stamping authoritative headers, so a client can't mark `X-Forwarded-Host`/`X-Forwarded-Proto`/`Authorization` as connection-specific and have a downstream intermediary drop them (P2). - browser: create throwaway browser profiles with `tempfile` (random name, 0700, O_EXCL) instead of a predictable timestamped path + `create_dir_all` that a local racer could pre-create; keep the `TempDir` alive until the browser exits (P2). - ca: re-secure existing CA material on the reload path (dir 0700, key 0600) before reusing it, so a drifted/restored group- or world-readable private key is not used indefinitely (P2). - browser: scope `ca_uninstall`'s `find`/`delete-certificate` to the same login keychain `ca_install` trusts into, so uninstall/regenerate act on exactly the trust location this tool manages (P2). - main: on non-macOS the `ts` binary now prints a clear unsupported-platform error and exits nonzero instead of silently succeeding as an empty no-op (P2). Adds unit tests for the header stripping and the CA re-secure-on-reload.
Both branches introduced crate `trusted-server-cli`: main's `ts` operator CLI (auth/build/config/deploy/provision/serve, delegated to `edgezero-cli`) and this branch's macOS `ts dev proxy`. Reconcile them into one crate. - The dev proxy is now `ts dev proxy`, a `dev` subcommand of the unified CLI. `dev` is available on every host target; only its `proxy` subcommand (and the heavy TLS/networking deps behind it) is scoped to macOS, so `DevCommand` is an empty enum on other hosts. - Adopt main's entry point (`run_from_env`, `edgezero_cli::init_cli_logger`) and structure (workspace member, `[lints] workspace = true`). The proxy code now satisfies the shared workspace lints. - Move the crate's test-only lint allowances into the root `clippy.toml` (add `allow-print-in-tests` / `allow-dbg-in-tests`) instead of a per-crate `#![cfg_attr(test, allow(...))]` block. - Track edgezero on its `main` branch instead of a pinned rev.
…-spec' into worktree-review-ts-dev-proxy-spec
- `dev::run` takes `DevCommand` by value; on non-macOS targets the enum is empty, so the by-value parameter is consumed only by an empty match and clippy's `needless_pass_by_value` fires (a reference cannot be used — a zero-arm match is not exhaustive over `&EmptyEnum`). Allow the lint on non-macOS, where the owned value is required. - Point the excluded `trusted-server-integration-tests` crate at edgezero `branch = "main"` to match the root workspace. Mixing `rev = …` (integration crate) with `branch = main` (adapters) made Cargo build edgezero-core twice, breaking the cross-adapter parity test with a type mismatch and leaving the crate's `--locked` Cargo.lock stale. - Reorganize the `.cargo/config.toml` aliases by adapter, sorted within each section, and add `run_cli_macos` / `run_cli_linux` for one-command native runs.
The dev proxy pulled `webpki-roots 0.26`, but EdgeZero (and the rest of the workspace) resolves `1.0`. That extra 0.26 line in the workspace lockfile does not exist in the excluded integration crate's lockfile, so `check-integration-dependency-versions.sh` flagged a transitive-parity drift. `TLS_SERVER_ROOTS` is API-compatible across the two versions, so bump to `1`, collapsing the duplicate to a single `1.0.8` line shared with the adapters.
…kspace - Move trusted-server-integration-tests into the workspace. It now shares [workspace.dependencies] (one EdgeZero pin, one lockfile) with the adapters it exercises, so the duplicated pin, the second Cargo.lock, and the check-integration-dependency-versions.sh drift-check (plus its CI wiring) are gone. Being native + Docker-based, it is whitelisted out of the wasm builds. - Hoist every shared/external dependency into [workspace.dependencies] and reference it from each crate via `workspace = true` (preserving per-crate features): the internal crates (js, openrtb), the dev-proxy stack (hyper/rustls/rcgen/…), http-body-util, testcontainers, worker, getrandom, etc. - Sort every [dependencies]/[dev-dependencies] and [workspace.dependencies] section alphabetically. - Switch the Fastly aliases from a `--workspace --exclude <every native crate>` blacklist to a `-p <wasm crates>` whitelist, so adding a native crate no longer requires touching them. - Drop the CLI's unused `reqwest` dev-dependency.
…v-proxy-spec # Conflicts: # Cargo.lock # Cargo.toml # crates/trusted-server-cli/src/run.rs # crates/trusted-server-integration-tests/Cargo.lock # crates/trusted-server-integration-tests/Cargo.toml
Every subcommand's implementation now lives under commands/<name>/, matching the dev command's layout: audit moves to commands/audit/mod.rs, config init to commands/config/init.rs. AuditArgs moves from run.rs into the audit module alongside run_audit, so run.rs only wires subcommands together. audit and config are pub(crate) (crate-internal); dev stays pub for the proxy_e2e integration suite.
The spin adapter's tests read edgezero.toml via include_str!("../edgezero.toml"),
so it must not be deleted. Also drop the test-cli fmt step: now that the CLI is a
workspace member, cargo fmt --all covers it.
Rewrite the root edgezero.toml as a single EdgeZero manifest covering all four
adapters (fastly, axum, cloudflare, spin), aligned with edgezero's canonical
format: `[app]` with entry = crates/trusted-server-core, one `[adapters.<name>]`
block each with real crate/manifest paths, build target/profile/features, and
commands (Fastly's preserved verbatim).
The locked edgezero schema has no per-adapter store overrides and rejects
unknown fields, so stores declare the union of logical ids with `default` set to
the primary id. This is safe: nothing in src/ or build.rs reads edgezero.toml —
Fastly binds its real stores in fastly.toml, and core resolves store names from
trusted-server.toml at runtime. The manifest ids are portable metadata consumed
only by `ts config validate` and edgezero deploy tooling.
The Spin adapter's regression test now loads this root manifest
(include_str!("../../../edgezero.toml")), so the duplicate
adapter-spin/edgezero.toml is deleted and spin.toml's watch glob repointed.
Validated with `ts config validate` (strict); spin (29) and cli (8) tests pass.
prk-Jr
approved these changes
Jul 2, 2026
prk-Jr
left a comment
prk-Jr
left a comment
Collaborator
There was a problem hiding this comment.
Summary
ts dev proxy is a well-built, security-conscious addition. I read the full proxy surface (server, rewrite, config, ca, browser, mod, run), the e2e suite, module gating, dependency scoping, and CI wiring. The audit/config changes are pure moves (96–100% similarity — import-path + rustfmt only, no logic change). No blocking issues found; approving. The notes below are all optional follow-ups.
What stands out (👍)
- Security posture: loopback-only default with
403off-loopback refusal,421misdirected-request guard against Host smuggling over a MITM tunnel, hop-by-hop + inboundForwardedstripping so the injectedX-Forwarded-Hostwins, authoritativeX-Forwarded-Proto: https+Fastly-SSLdrop, refusing--basic-authon a non-loopback bind,0600CA key with re-secure-on-reload, andca regenerateaborting if OS trust cannot first be revoked. - Detail: byte-at-a-time head read so the TLS ClientHello is never consumed before the CONNECT
200; IP-literal hosts get an IP-type SAN. - Test + CI design: 43 unit/e2e tests including the blind-tunnel cert-identity assertion (proves an unmatched host is not MITM'd), IPv6
--resolve, keep-alive multi-request, and the 421/403 guards; macOS-native CI exercises the proxy while the Linux-native job covers audit and the non-macOS fallback arms.
Non-blocking notes
♻️ refactor / perf
- Per-request TLS client config —
crates/trusted-server-cli/src/commands/dev/proxy/server.rs:461(proxy_to_upstream) rebuilds arustls::ClientConfigand re-parses the wholewebpki_rootsstore on every upstream hop; the service closure atserver.rs:341also clonesrules/basic_auth/resolveper request. Build the client config once (Arc inResolvedConfigor aOnceLock) and share the immutable inputs. Dev-tool scale, so not urgent.
🌱 seedling
- No upstream timeout —
connect_upstream(server.rs:212) has no connect/read timeout; a black-holed upstream (easy with a wrong--resolvepin) stalls the browser tab until the OS TCP timeout. Atokio::time::timeoutaround dial + handshake would fail fast into the existing502path. - No upstream connection reuse — a fresh TCP + TLS handshake per request even within one keep-alive client tunnel (
server.rs:438). Acknowledged tradeoff; noting for a future pass.
🤔 thinking
- Concurrent Safari instances — two concurrent
ts dev proxy --launch safariruns collide: the second's startuprestore_system_proxy_if_pendingreverts the first's live system PAC and deletes its restore file (browser.rs:456). Single-instance is an implicit assumption; worth a doc note or a lockfile guard.
⛏ nitpick
- Credential trimming —
resolve_basic_auth(config.rs:305) trims the whole value/file; correct for the trailing newline but it also strips a password's meaningful leading/trailing whitespace. Trimming only trailing\r/\nis safer. is_valid_hostleniency — accepts leading/trailing.and..(config.rs:208). Safe for the PAC/URL/Host interpolation contexts (the stated purpose), just not a strict hostname.
CI Status
All checks green at time of review (fmt, clippy across adapters, rust tests incl. ts-CLI native + cross-adapter parity, vitest, docs format).
ChristianPavilonis
approved these changes
Jul 2, 2026
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
Collaborator
There was a problem hiding this comment.
Summary
📝 Approved with comments. I reviewed the current PR #798 diff against main, including the new ts dev proxy, CA/browser orchestration, workspace/EdgeZero manifest changes, CI, and docs. CI is green and I did not find any P0 blockers.
I left inline notes using the CREG guide: 🔧 for changes worth addressing before broad use, and 🌱 for lower-risk follow-ups/hardening.
- Strip hop-by-hop headers from the upstream response (not just the request), so a `Connection: close` cannot tear down the reusable MITM tunnel. - Reject malformed CONNECT cleanly: a non-numeric port or a truncated/oversized request head now returns 400 instead of falling back to 443 or routing a partially-read request. - Add `--connect-timeout` (default 10s) bounding each upstream dial, so a black-holed upstream fails fast into 502 instead of stalling the browser tab. - Cache the rustls client config per verification mode (OnceLock) instead of rebuilding it — and re-parsing the webpki root store — on every request. - `--listen …:0`: bind first and use the OS-assigned port for the PAC and browser launch instead of port 0. - Read the `--basic-auth-file` credential stripping only a trailing newline, so a password's leading/trailing spaces survive. - Shell-quote the Safari manual-setup fallback command. - Docs: `--rewrite-host` no longer claims blanket safety with Trusted Server upstreams — the Fastly/Spin adapters strip `X-Forwarded-Host`, so first-party URLs fall back to `Host: TO`; documented the caveat in the guide and code. - Docs/cleanup: the CLI is a workspace member now (drop the stale per-crate Cargo.lock; fix the guide and the integration-tests README). - edgezero.toml: store ids are the logical `trusted_server_*` names, overridable per adapter/runtime (documented inline).
- Strip hop-by-hop headers from the upstream response (not just the request), so a `Connection: close` cannot tear down the reusable MITM tunnel. - Reject malformed CONNECT cleanly: a non-numeric port or a truncated/oversized request head now returns 400 instead of falling back to 443 or routing a partially-read request. - Add `--connect-timeout` (default 10s) bounding each upstream dial, so a black-holed upstream fails fast into 502 instead of stalling the browser tab. - Cache the rustls client config per verification mode (OnceLock) instead of rebuilding it — and re-parsing the webpki root store — on every request. - `--listen …:0`: bind first and use the OS-assigned port for the PAC and browser launch instead of port 0. - Read the `--basic-auth-file` credential stripping only a trailing newline, so a password's leading/trailing spaces survive. - Shell-quote the Safari manual-setup fallback command. - Docs: `--rewrite-host` no longer claims blanket safety with Trusted Server upstreams — the Fastly/Spin adapters strip `X-Forwarded-Host`, so first-party URLs fall back to `Host: TO`; documented the caveat in the guide and code. - Docs/cleanup: the CLI is a workspace member now (drop the stale per-crate Cargo.lock; fix the guide and the integration-tests README). - edgezero.toml: store ids are the logical `trusted_server_*` names, overridable per adapter/runtime (documented inline).
prk-Jr
added a commit
that referenced
this pull request
Jul 7, 2026
Reconcile the ad-template CLI + generate feature onto the commands/ module layout the base branch adopted from main (ts dev proxy, #798): - audit/ -> commands/audit/ : mod.rs is the audit namespace; the #800 draft-gen plus slot reconstruction move under commands/audit/generate/ (base's duplicate commands/audit/{analyzer,browser_collector} dropped). - config_ad_templates.rs -> commands/config/ad_templates.rs. - app_config and ad_templates support modules stay at crate root. - run.rs/lib.rs wiring merged with the new `ts dev` command. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #824
Summary
Adds
ts dev proxy— a local TLS-terminating (MITM) developer proxy that serves a production publisher hostname from a dev/staging upstream by swapping the TLS SNI. A real browser shows the production domain in the address bar while a Compute/staging service answers, so you can exercise Trusted Server's host-anchored URL rewriting against real first-party behavior without DNS or/etc/hostschanges.What's included
New
tsdeveloper CLI (crates/trusted-server-cli, a native binary excluded from the wasm workspace):Proxy flags
--map FROM=TO(repeatable) or-f/--from+-t/--to HOST[:PORT]— the rewrite rule(s); rules are explicit (notrusted-server.tomlinference).--rewrite-host— sendHost: TOupstream (default keepsHost: FROMso core's URL rewriting stays anchored to the production host).--resolve HOST:IP(repeatable) — curl-style DNS pin; dials the given IP on every upstream path while keeping SNI/Hoston the hostname, so the cert still validates. Makes the proxy self-contained (no hosts-file edits).--basic-auth USER:PASS/--basic-auth-file PATH— injectAuthorizationwhen absent (clears a gated upstream).--listen ADDR(default127.0.0.1:18080),--launch chrome|firefox|safari,--insecure(accept self-signed upstreams),--allow-non-loopback.Example
--resolve staging.example.net:192.0.2.10dials192.0.2.10for the upstreamleg while leaving the TLS SNI and
Hostasstaging.example.net, so theupstream certificate still validates — the curl
--resolvemodel. Repeat theflag to pin multiple hosts. IPv6 targets work too (the value is split on the
first
:), e.g.--resolve staging.example.net:2001:db8::10.Architecture
TO) signed by a per-machine local CA; the decrypted stream is proxied request-by-request over keep-alive. Unmatched hosts are blind-tunnelled on loopback (never MITM'd), and aHostmatching no rule is refused421(no smuggling through the CONNECT-authority rule).ca installadds it to the login keychain;uninstall/regeneraterevoke prior trust before replacing key material)./proxy.pacroute; Chrome/Firefox/Safari launch helpers (Safari persists & restores system-proxy state across hard kills).X-Forwarded-Host/X-Orig-Host = FROMso the upstream can keep emitting first-party URLs on the production host even when--rewrite-hostsendsHost: TO.Safety
403(can't become an open proxy) unless--allow-non-loopback.cfg-scoped to macOS, so non-macOS targets compile to an empty shell with a clear message instead of a hardcompile_error!(which previously brokecargo buildunder the repo's wasm default target).Tests, CI, docs
tests/proxy_e2e.rs): MITM rewrite/forward, blind tunnel cert identity,--resolvepinning,--rewrite-hostForwarded-Host behavior, injected Basic auth, keep-alive multi-request,421/403guards.wasm32-wasip1by default).docs/guide/ts-dev-proxy.md(setup, trust, troubleshooting) + the design spec and implementation plan underdocs/superpowers/.Notes