Repository navigation
fix(egress): keep-alive requests bypassed the gate + credential injection (the post-#830 gh 401s) - #846
Conversation
…ection The interceptor gates, strips, and injects only the FIRST request on an intercepted TLS connection — everything after the buffered prefix streams verbatim through copy_bidirectional. A keep-alive client's second request therefore reached the upstream ungated, still carrying the guest's placeholder credential; GitHub answers that with 401 Bad credentials. Every multi-request `gh` command broke on request #2+ (`gh pr create`'s existing-PR pre-check, `gh pr checks`' feature-detection probes, `gh run list`'s workflows+runs pair) while single-request probes worked, which made it masquerade as the #830 credential bug after that fix shipped. Force `Connection: close` (and strip Keep-Alive/Proxy-Connection) on every intercepted request — the rewrite the observe path always applied, now shared via `observe::force_connection_close`. The upstream answers once and closes, the client reconnects, and every request is gated + injected. Upgrade (websocket) requests are left untouched. Covers the substitution plane too. ADR 0059 records the pitfall and the diagnosis tells. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
`gh` probes schema capabilities before commands like `gh pr checks` and `gh pr create` via aliased `__type(name: ...)` introspection queries and hard-fails the command when the probe dies. `__type` was not a covered query field in github.json, so the set-coverage gate denied the probe (connection close -> `Post https://github.com/proxy/api.github.com/graphql: EOF`). Gate it under the same every-read-power grant set as `viewer`/`rateLimit` — it returns public schema metadata, no resource access. A TLS e2e regression pins the exact aliased multi-field probe shape. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
|
…ession The test deliberately writes request #2 into the torn-down tunnel; which teardown error the proxy task surfaces is a platform/timing coin flip — macOS yields close_notify/UnexpectedEof, Linux CI yields Broken pipe. All mean the same thing the test asserts: the connection died. The substantive assertions (Connection: close rewrite, injected first request, no second request upstream, client EOF) are unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
✅ engrams review — complete. 1 finding posted. · View details |
| // Connection-upgrade guard: forcing close on a websocket handshake would | ||
| // break the stream it negotiates. Leave the request untouched. | ||
| if split_crlf(header_region) | ||
| .iter() | ||
| .any(|l| header_name_lower(l) == "upgrade") | ||
| { | ||
| return prefix; | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy · MEDIUM — Guest-supplied Upgrade header reopens the keep-alive gate bypass this PR closes
WHAT — The new rewrite_request_headers returns the request untouched (no forced Connection: close) whenever the header block contains any Upgrade: header (observe.rs:88-95). Since force_connection_close is the sole mechanism stopping request #2+ on a keep-alive connection from streaming through copy_bidirectional ungated (intercept.rs:461-474), a guest defeats the fix by adding Upgrade: websocket (+ Connection: Upgrade) to their gated request #1.
WHY — The guard's assumption (doc comment + ADR 0059) is "post-upgrade traffic is one logical stream, not smuggle-able HTTP requests." That holds only if the upstream actually upgrades (responds 101). The guest controls the request and can send an Upgrade request to a host that does NOT speak websockets, e.g. api.github.com REST/GraphQL. Such a server ignores the unsupported Upgrade header and answers normally over a still-persistent HTTP/1.1 connection (the guest sent Connection: Upgrade, not close). copy_bidirectional then forwards the guest's request #2 verbatim upstream: no (method,path) gate, no inject-policy check, no placeholder substitution — exactly the bypass keep_alive_second_request_cannot_bypass_the_gate guards against. The real injected credential is not applied to #2, so this is an egress policy-gate bypass, not a credential leak; blast radius is limited to what the intercepted host does with uncredentialed/placeholder traffic on forbidden paths.
Actor: the sandbox guest (runs arbitrary code, crafts raw HTTP, holds its placeholder credential). This is distinct from the ADR's "accepted residual" (which needs a non-compliant upstream to ignore Connection: close) — here the proxy itself emits no close, on guest command.
HOW — Only skip the rewrite when the connection genuinely becomes a websocket (upstream returns 101), otherwise still force Connection: close; or restrict the Upgrade exception to hosts/policies that declare websocket support rather than trusting a guest-supplied header.
There was a problem hiding this comment.
Accurate and confirmed — fixed in a0e2f19.
You're right that the Upgrade guard was itself a bypass. The exemption assumed post-upgrade traffic is one logical stream, but that only holds if the upstream actually returns 101. A guest can send Upgrade: websocket + Connection: Upgrade to a REST/GraphQL host that ignores the unsupported Upgrade and keeps the connection persistent — and since the proxy emitted no close, copy_bidirectional then forwards request #2 ungated. As you noted, this is a policy-gate bypass rather than a credential leak (the real injected token isn't applied to #2), but it fully reopens what this PR closes.
Fix: Upgrade is now stripped, never honored — treated like Keep-Alive/Proxy-Connection, so every intercepted request still gets a forced Connection: close. No intercepted (credential/observe) host speaks websockets; genuine websocket support would need an explicit per-policy opt-in plus a 101-aware tunnel, not trust in a client header.
Regressions:
guest_upgrade_header_cannot_reopen_the_bypass— e2e driving the actual attack: request build(deps): bump the actions group with 3 updates #1 carriesUpgrade: websocketagainst a non-upgrading upstream, and request chore(docker): bump rust from 1.83-slim to 1.95-slim in /docker in the docker-base group across 1 directory #2 must still die (dead connection, never reaches upstream, Upgrade stripped).force_close_strips_guest_supplied_upgrade— unit test pinning the strip + forced close.
ADR 0059 updated to record the strip-never-honor decision and its rationale.
…846 review) The Upgrade exemption in force_connection_close was itself a bypass: the header is guest-supplied, and any REST/GraphQL upstream that doesn't upgrade ignores it and keeps the connection persistent. A guest could dress gated request #1 with `Upgrade: websocket` + `Connection: Upgrade` and stream an ungated request #2 through copy_bidirectional — reopening the exact keep-alive gate bypass this PR closes (no method/path gate, no inject policy, no placeholder substitution on #2). Strip Upgrade like the other connection-management headers and always force `Connection: close`. No intercepted (credential/observe) host speaks websockets; genuine support would need a per-policy opt-in plus a 101-aware tunnel, not trust in a client header. New e2e regression drives the actual attack (Upgrade-dressed #1 → dead #2); unit test pins the strip. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
✅ engrams review — complete. 0 findings posted. · View details |
What happened
PR #830 shipped and works — but session
af28cac4(2026-07-21) still showedghGraphQL 401s. Live diagnosis in that session found a different bug that had been masquerading as the credential one:The interceptor gates, strips, and injects only the FIRST request on an intercepted TLS connection. After forwarding the rewritten prefix it streams both directions verbatim (
copy_bidirectional). A keep-alive client's second request therefore reached the upstream ungated, still carrying the guest's placeholder credential — GitHub answers that with401 Bad credentials.That's why the failure pattern was so confusing:
ghcommand broke on request chore(docker): bump rust from 1.83-slim to 1.95-slim in /docker in the docker-base group across 1 directory #2+ —gh pr create's existing-PR pre-check,gh pr checks' schema feature-detection probes,gh run list's workflows+runs pairgh api,gh pr view,git pushghcaches the 401s from its feature-detection queries in~/.cache/gh, so commands kept failing instantly even on fresh connectionsReproduced conclusively in the prod session: two identical requests on one curl connection →
req1=200 req2=401.The fix (commit 1)
Force
Connection: close(and stripKeep-Alive/Proxy-Connection) on every intercepted request — the exact rewrite the observe path has always applied, now shared viaobserve::force_connection_close. A compliant upstream answers once and closes; the client reconnects; every request is gated + injected.Upgrade(websocket) requests are left untouched. This also covers the substitution plane (a reused connection's second request skipped placeholder substitution too). ADR 0059 records the pitfall and the diagnosis tells.The follow-on gap (commit 2)
With keep-alive fixed,
gh pr checks/gh pr create's feature-detection probes (aliased top-level__typeintrospection) would still be policy-rejected —__typewasn't a covered query field ingithub.json, andghhard-fails the command when the probe dies. Cover it under the same every-read-power grant set asviewer/rateLimit(public schema metadata, no resource access).Validation
keep_alive_second_request_cannot_bypass_the_gate(rewrite + one-request-per-connection contract) andgraphql_allows_aliased_type_introspection(the exactghprobe shape)just check— 2028/2028bun run typecheck+bun test— 855 pass (connector registry loads the new entry)Residual (accepted, in the ADR): an upstream that ignores
Connection: closeleaves the tunnel open for ungated-but-uncredentialed requests; real API hosts honor it. A per-request gating loop in the tunnel would close that fully — follow-up if ever needed.🤖 Generated with Claude Code