Repository navigation
fix: enforce outbound destination policy with an egress proxy - #63
Conversation
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. |
Route every browser tier's HTTP(S)/WebSocket traffic through an in-process filtering proxy that validates each destination (including redirect hops that Playwright route handlers cannot see), resolves it once, and connects only to the validated address. Obscura is launched with --proxy; Chromium/Firefox use a context proxy, and local Chromium also gets a browser-level proxy so /cdp-created contexts are covered. Workers are attested with HTTP and CONNECT canaries at startup, after reconnects, and after restarts; a worker that does not route through the proxy is disabled rather than used unprotected. /cdp is off by default and rejected when remote workers are configured. Also: special-purpose address ranges (multicast, NAT64, 6to4/Teredo, SRv6, documentation) are refused; upstream chaining sends the validated IP by default; control characters are rejected and request lines rebuilt; proxy Basic auth, DNS/drain/idle timeouts, a connection cap, strict boolean parsing, egress metrics, and documentation are included.
8cee1d8 to
78f2b7c
Compare
![clementine-oaklight[bot]](https://avatars.githubusercontent.com/in/3956685?s=60&v=4){kind=small}
There was a problem hiding this comment.
Security/edge-case pass on 78f2b7c (I did not re-do correctness; Milo has that side).
Ran locally: pytest -q → 156 passed, 4 failed, all four in tests/test_fonts.py — the pre-existing failures you flagged, so the test claim holds. GitGuardian is now green on this head; the earlier s3cret hit is gone after the token-randomisation commit.
Bypass matrix I threw at a live instance of the proxy (raw sockets, EgressProxy() on loopback, no NPS/upstream):
CONNECT → all 403: 127.0.0.1, localhost, 169.254.169.254, 10.0.0.1, 192.168.1.1, 100.64.0.1, 0.0.0.0, [::1], [::ffff:127.0.0.1], [::ffff:169.254.169.254], [64:ff9b::7f00:1], [64:ff9b::a9fe:a9fe], [2002:7f00:1::], [fe80::1], [fc00::1], and the three numeric-encoding tricks the browser normalises but a raw client does not — 2130706433, 0x7f000001, 017700000001, 127.1 (all resolve to 127.0.0.1 via glibc, all refused at the validate-the-resolved-address step).
Absolute-form → 403: http://127.0.0.1/, http://[::ffff:7f00:1]/, http://169.254.169.254/latest/meta-data/, https://example.com/ (must use CONNECT), http://user:pass@example.com/ (credentials in URL). Token mode without Proxy-Authorization → 407. Legitimate traffic is untouched: CONNECT example.com:443 → 200 Connection Established, GET http://example.com/ relayed a real 200 from Cloudflare.
Design points I checked and agree with: one resolve then connect to the validated sockaddr (no check/connect gap for rebinding); every DNS result must be global, so a split answer cannot pick the private one; <-loopback> in both the context proxy and the Chromium launch flag (otherwise loopback navigation bypasses everything); browser-level proxy for /cdp contexts; launch-proxy canary skipped only when a token is set, where those contexts fail closed with 407 as documented; remote Obscura refused unless VEILRENDER_OBSCURA_REMOTE_PROXIED and then attested by canary; from_settings refuses a non-loopback bind without a token; Proxy-Authorization/Proxy-Connection stripped before relaying; control chars, port 0 and oversized headers rejected; worker attestation re-run after reconnect, recovery and stuck-restart, not just at startup.
Two inline suggestions, neither a policy bypass: a slow-DNS path that starves the process-wide default executor, and Host not being pinned on absolute-form requests. Nits in the body: (1) _mark_canary runs before _authorized, so unauthenticated traffic can set a canary event — tokens are 128-bit so it is not exploitable, but attestation should be reachable only by authenticated peers; (2) _IDLE_TIMEOUT = 300 with _MAX_CONNECTIONS = 1024 lets one hostile page hold ~1000 tunnels to a public host for five minutes each — a lower idle timeout or a per-client cap keeps that from starving a shared gateway.
I could not exercise the QUIC/WebRTC path or a real redirect chain here; the code path for redirects (each hop re-enters _handle_connect/_handle_absolute) is right, and both gaps are stated in SECURITY.md rather than left implicit.
|
|
||
| try: | ||
| infos = await asyncio.wait_for( | ||
| asyncio.get_running_loop().getaddrinfo( |
There was a problem hiding this comment.
A timed-out lookup keeps occupying a thread in the loop's shared default executor, so a hostname with a deliberately slow authoritative NS can stall every other run_in_executor user in the process — including the next request's DNS.
Measured on this head with socket.getaddrinfo patched to sleep 6s (nothing else changed), 16 CPUs, _DNS_TIMEOUT=5:
40 concurrent _resolve_addresses calls -> 40/40 EgressPolicyError at 5.0s (fail closed, good)
resolver threads actually entered -> 20 (= min(32, cpu+4), the default executor bound)
an unrelated run_in_executor(0.1s job) -> finished after 5.0s
So the requests fail closed but the abandoned lookups hold all 20 threads until the OS resolver gives up, and anything else that needs the default executor queues behind them. A page can trigger this by pointing at a domain whose NS never answers — no special privileges needed.
Suggestion: give DNS its own small ThreadPoolExecutor with a bounded queue (reject/503 when full) instead of loop.getaddrinfo, or cap in-flight lookups with a semaphore. wait_for cancels the await, not the thread, so the bound has to be on the executor, not on the timeout.
There was a problem hiding this comment.
Extending this with the one path I checked that is not in that blast radius: canaries are marked before any resolution (_mark_canary(target) runs ahead of _handle_connect/_handle_absolute, egress_proxy.py:566), so attestation survives a stalled resolver.
Measured with _resolve_addresses replaced by a 6s sleep on this head:
canary request, resolver stalled -> HTTP/1.1 403 Forbidden in 0.000s, token event set=True
real GET http://example.com/ -> 200 OK in 6.029s
So _verify_worker will not flap workers unhealthy when the executor is saturated: the damage stays at "real navigations fail", which is the good news for your fix — a bounded executor (reject when full) changes only the navigations, not the health signal.
There was a problem hiding this comment.
Fixed in 6be87cd. DNS now runs on a dedicated ThreadPoolExecutor (_DNS_WORKERS=8, thread_name_prefix="egress-dns") with an asyncio.Semaphore capping in-flight lookups at 32, created lazily by EgressProxy._resolve and shut down in stop(). The 5s timeout bounds only the wait; a stalled resolver can hold at most the 8 dedicated threads and cannot queue work behind the loop's shared default executor. tests/test_egress_proxy.py::TestDnsIsolation patches socket.getaddrinfo to block and asserts the lookup thread is an egress-dns thread while a default-executor job still completes.
There was a problem hiding this comment.
Agreed, and the dedicated executor preserves this. Canaries are still recognised before resolution (now after _authorized, 6be87cd), so a saturated DNS pool cannot flap a healthy worker; it only delays real navigations, which is the fail-closed direction.
| continue | ||
| text = line.decode("latin-1") | ||
| if _has_forbidden_header_chars(text): | ||
| raise EgressPolicyError("Control characters in request header") |
There was a problem hiding this comment.
_rebuild_head drops only Proxy-*, so the client's Host is forwarded verbatim to the origin:
>>> _rebuild_head([b'Host: metadata.internal', b'Authorization: Bearer x', b'Proxy-Authorization: Basic abc', b'Proxy-Connection: keep-alive'], 'GET /path HTTP/1.1')
GET /path HTTP/1.1
Host: metadata.internal
Authorization: Bearer x
Destination policy is unaffected — the connection still goes to the validated IP — but on a shared edge/CDN, Host is exactly the routing key, so a hand-crafted absolute-form request reaches a different backend on that public IP than the URL names. Browsers always send a matching Host; the clients that do not are the ones using /cdp (or a compromised worker), which is the threat model this proxy exists for.
Suggestion: for absolute-form, rewrite Host to the validated host[:port] (same value already used to build request_line), and drop any duplicate Host lines rather than leaving the first one in place.
There was a problem hiding this comment.
Same hole one layer further out: with VEILRENDER_UPSTREAM_PROXY set, _handle_absolute builds fwd_target from the validated host, but _rebuild_head still forwards the client's Host verbatim — so the upstream proxy receives a request line naming the validated host and a Host: naming something else.
>>> _rebuild_head([b'Host: metadata.internal', b'Proxy-Authorization: Basic abc'],
... 'GET http://93.184.216.34:80/p HTTP/1.1', ['Proxy-Authorization: Basic up'],
... force_close=True)
GET http://93.184.216.34:80/p HTTP/1.1
Host: metadata.internal
Connection: close
Proxy-Authorization: Basic up
Your fix closes both: the absolute request line already carries the authority to copy, and force_close keeps the upstream connection from being reused for another Host.
There was a problem hiding this comment.
Fixed in 6be87cd. _rebuild_head takes host_header; _handle_absolute passes _format_authority(host, parsed.port) built from the validated URL, so every client Host line is dropped and replaced with a single validated one. This covers the direct, SOCKS5, and HTTP-upstream branches. Regression test: test_absolute_form_host_rewritten_to_validated_authority sends Host: attacker.test plus a duplicate and asserts the origin sees exactly one Host: public.test.
There was a problem hiding this comment.
Confirmed. The Host rewrite in 6be87cd applies to the upstream branches too: the HTTP-upstream request line keeps the validated authority (pinned IP or hostname) and now carries a matching Host, while force_close still prevents the connection from being reused for another destination.
![milo-oaklight[bot]](https://avatars.githubusercontent.com/in/3956677?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verified at 78f2b7c. The branch was squashed while I was working (I measured 8cee1d8), and git diff 8cee1d8 78f2b7c -- src/ deploy/ SECURITY.md README_en.md README_zh.md AGENTS.md CHANGELOG.md is empty — the only change is tests/test_egress_proxy.py, where the s3cret fixture became secrets.token_hex. Every source measurement below therefore applies to this head unchanged.
Suite and checks: pytest -q -> 156 passed, 4 failed, all four in tests/test_fonts.py. I ran those four on the merge base 7dfc9a8 and they fail identically (4 failed, 5 passed), so they are pre-existing. ty check src/ clean, and make lint's ruff invocation is clean (the two UP035/UP037 hits are under src/veilrender/_vendor/, which the target excludes). Checks on this head: GitGuardian pass, docker-safety pass x2, lint pass. The GitGuardian red on the previous head was that test fixture, not a real secret.
Security pass, measured
is_blocked_ip over 37 literals: loopback, RFC1918, CGNAT, link-local/metadata, unspecified, broadcast, documentation, benchmarking, reserved, ::1, fe80::, fc00::, ff02::, ::ffff:127.0.0.1, ::ffff:169.254.169.254, NAT64 64:ff9b::7f00:1 / 64:ff9b::a00:1 / 64:ff9b:1::, 6to4 2002:7f00:1::, Teredo, 2001:db8::, 3fff::, 5f00::, ORCHID all refused; 8.8.8.8, 1.1.1.1, 93.184.216.34, 2606:4700:4700::1111, 2001:4860:4860::8888 allowed. 37/37 as expected, 0 mismatches. is_global_host refused localhost, LOCALHOST., metadata.google.internal, 2130706433, 0x7f000001, 127.1 and allowed example.com, 1.1.1.1.
Raw request lines into a live EgressProxy:
| request | response |
|---|---|
GET http://127.0.0.1:9/ |
403 |
GET http://2130706433:9/ |
403 |
GET http://[::ffff:127.0.0.1]:9/ |
403 |
GET http://169.254.169.254/latest/meta-data/ |
403 |
GET http://evil.example@127.0.0.1:9/ |
403 (credentials in URL) |
GET / (origin-form) |
400 |
CONNECT 127.0.0.1:9 |
403 |
CONNECT [::1]:9 |
403 |
| canary URL | 403, token event set |
GET http://example.com/ |
200, response relayed |
_open_validated does fall through the resolved list (first address raising OSError still lands on the second: 2 attempts, then success), which is what keeps dual-stack destinations working when the container has no IPv6 route. Worth keeping, since a single-address attempt would turn a large slice of the web into 502s.
One blocker, availability only: a worker that restarts itself never gets healthy back when the health loop is not running (inline at browser.py:477). Only the egress-disabled single-worker configuration is affected; the security core is not.
One coverage gap: the attestation decision itself (_verify_worker / _run_canary) has no tests (inline at browser.py:885).
Two nits, no thread
- Destination ports are unrestricted: any 1-65535 to a globally routable address is relayed. A renderer needs 80/443, so an allowlist (with an operator override) would cut the service's usefulness as a generic egress hop without affecting normal use.
_active_connectionsis incremented before_authorizedand capped at 1024 with a 10s header timeout, so on a non-loopback bind an unauthenticated peer can occupy every slot and starve the browser's own proxy traffic (renders then fail). Counting only authenticated connections, or capping pre-auth ones separately, bounds that.
Clementine's two open threads (DNS executor starvation, Host forwarding) are real; I replied on both with measurements that extend theirs rather than opening duplicates.
- run DNS lookups on a dedicated bounded ThreadPoolExecutor with an in-flight semaphore, so a stalled resolver cannot occupy the loop's shared default executor - rewrite Host to the validated authority on absolute-form requests and drop duplicate Host lines - require proxy authentication before recognising canary requests - lower the tunnel idle timeout from 300s to 60s
start() no longer set healthy, and in single-worker mode with the egress proxy disabled there is no health loop to re-attest a restarted worker, so the service stayed dead until the process restarted. Mark a started worker healthy when there is no proxy to attest; with enforcement on the manager still owns the flag and sets it only after a canary. Add attestation tests: healthy when canaries reach the proxy (HTTP and CONNECT, with and without auth), unhealthy when navigation bypasses it, the browser-level proxy check for local engines, and re-attestation after a reconnect.
|
Both nontrivial findings are addressed:
Clementine's two inline suggestions (dedicated bounded DNS executor, validated |
There was a problem hiding this comment.
Re-reviewed f9deba1. All four points I raised are closed, and the two nits with them. Verified rather than read.
Correction first: my DNS number in the previous pass was measured badly — I timed the unrelated job after awaiting the gather, so the 5.0s I reported was the gather's own duration. Re-measured properly (three run_in_executor jobs recording their own start timestamps, submitted behind 40 stalled lookups):
old path (loop.getaddrinfo, shared default executor) -> unrelated jobs started at 6.0s
new path (proxy._resolve, dedicated pool) -> unrelated jobs started at 0.0s
resolver threads entered -> 8 (pool max 8, was 20 = min(32, cpu+4))
outcomes of the 40 lookups -> 40/40 EgressPolicyError, fail closed
So the hole was real, my evidence for it was sloppy, and the fix closes it: the semaphore is acquired inside the wait_for, so a saturated pool fails closed inside the same 5s budget instead of queueing indefinitely, and stop() cancels queued futures so a restart gets a fresh pool.
Host rewrite, end to end. Unit: Host: metadata.internal plus a duplicate Host: dup both dropped, a single Host: example.com appended after the remaining headers. Live: GET http://example.com/ HTTP/1.1 with Host: metadata.internal previously came back 403 from the origin (Cloudflare rejecting the bogus vhost) and now returns 200 — the origin sees the validated authority. _format_authority brackets IPv6 and omits the port when it was default, so the header matches the request line in the direct, SOCKS5 and HTTP-upstream branches.
Canary after auth. Canary request without Proxy-Authorization → 407, canary_blocked unchanged. Same request with the token → 403, counter +1. An unauthenticated local process can no longer drive attestation, and a stalled resolver still cannot flap a healthy worker because the canary is still recognised before any lookup.
Attestation on restart (f9deba1). _mark_started() is a no-op while enforcement is on, so BrowserManager remains the only writer of healthy and still gates it on the canary; it only restores the flag when there is no proxy to attest. The seven tests in tests/test_browser_attestation.py cover the paths that matter, including test_unauthenticated_canary_does_not_attest and test_mark_started_only_restores_health_without_enforcement.
Regression pass. pytest -q → 165 passed, 4 failed, all in tests/test_fonts.py (unchanged pre-existing). Project-scoped ruff check, ruff format --check and ty check src/veilrender/ clean. My 15-case CONNECT matrix still returns exactly the expected statuses (403 for loopback, RFC1918, CGNAT, unspecified, v6 loopback, v4-mapped, NAT64, 6to4, link-local, ULA, and the decimal/hex/short IPv4 forms; 200 Connection Established for a public host). Idle timeout is 60s as intended.
Approving. The residual risks in SECURITY.md (QUIC/WebRTC UDP, /cdp, upstream pinning trade-off, one-request-per-connection) are stated plainly rather than buried, which is the part I care about most in a change like this.
There was a problem hiding this comment.
Verified at f9deba1. It is a fast-forward from the head I last reviewed (78f2b7c is an ancestor), so the delta is exactly 6be87cd + f9deba1, and I re-ran the whole battery rather than just the diff.
Suite and CI: 165 passed, 4 failed — the same four in tests/test_fonts.py that also fail at the merge base. ruff check src/ clean on the lint target, ruff format --check src/ clean, ty check src/ clean. Checks: GitGuardian pass, docker-safety pass, lint pass.
DNS executor (the starvation case): repeated Clementine's conditions — socket.getaddrinfo patched to sleep 6s, 40 concurrent lookups.
40 lookups -> 40/40 EgressPolicyError at 5.0s (fail closed, unchanged)
lookup threads -> egress-dns_0 .. egress-dns_7 (dedicated pool)
unrelated default-executor job-> 0.1s job finished at 0.30s (5.01s on the previous head)
That is the fix landing where it matters: the slow lookups still fail closed, but they no longer queue unrelated run_in_executor work behind them.
Host rewrite: end to end through a real proxy against a local origin, client sending GET http://public.test/ with Host: attacker.test, a duplicate Host: dup.test, and an unrelated header:
origin saw Host lines : ['Host: public.test'] (duplicates dropped, validated authority)
X-Keep preserved : True
after response : 200 relayed
One _rebuild_head call site after the direct/SOCKS5/upstream branch means all three get it, and _format_authority brackets IPv6 destinations correctly.
Canary after auth:
no credentials -> 407, token event unset
veilrender:<token> -> 403, token event set
So a local process can no longer forge an attestation, and a browser that fails to authenticate can no longer pass health — both directions are now fail-closed.
Health restore: _mark_started() sets healthy with egress_proxy=false and leaves it alone with enforcement on, and it is called from all four start() implementations. My exact failure case — single local worker, egress off, browser self-restart — heals now instead of leaving _pick_worker with an empty list.
The new tests actually pin this. I reverted each fix and checked that a test fails:
| mutation | result |
|---|---|
host_header=None |
1 failed |
_mark_started body no-op |
1 failed |
_resolve back on the loop's default executor |
1 failed |
| restored | 53 passed (test_egress_proxy.py + test_browser_attestation.py) |
Two observations, neither blocking. The 300s to 60s idle timeout cuts a connection that stays silent for over a minute — irrelevant for goto/wait_until windows, but visible to anyone driving /cdp by hand with a long-poll or SSE stream open. And _DNS_WORKERS=8 against _DNS_MAX_INFLIGHT=32 means up to 24 callers can hold a semaphore slot waiting for a thread and then time out at 5s; the pool is the real bound, which is fine, just not the number the constant suggests.
Everything from my previous review is addressed, and the two threads Clementine opened are closed by fixes I measured rather than read. Approving.
Summary
Closes the blind-SSRF gap left by #61. Every browser tier now sends HTTP(S)/WebSocket traffic through an in-process filtering egress proxy (
src/veilrender/egress_proxy.py):--proxyat launch for Obscura; local Chromium also gets a browser-level proxy so/cdp-created contexts are covered;Also in this PR:
/cdpis disabled by default (VEILRENDER_CDP_ENABLED=trueto opt in) and rejected when remote workers are configured.VEILRENDER_UPSTREAM_PROXY) sends the validated IP upstream by default (VEILRENDER_UPSTREAM_PIN_IP)./metricsexposes egress proxy counters;SECURITY.md,AGENTS.md, READMEs, pool compose examples and the changelog are updated.Test plan
pytest— 156 passed; the only failures are the 4 pre-existingtests/test_fonts.pycasesty checkandruff checkcleanfirefox.connect: public 200, loopback 403--proxy; navigation fails when the proxy is stopped/cdpdefault contexts cannot reach loopback when enabled;/cdpreturns 503 with remote workersCONNECT <validated-ip>:443, not the hostnameAI was used to assist with implementation; all changes were reviewed and tested.