diff --git a/core/listener/forwardproxy/hostmismatch_test.go b/core/listener/forwardproxy/hostmismatch_test.go new file mode 100644 index 000000000..a308c54be --- /dev/null +++ b/core/listener/forwardproxy/hostmismatch_test.go @@ -0,0 +1,167 @@ +package forwardproxy + +import ( + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/rossoctl/cortex/core/pipeline" + "github.com/rossoctl/cortex/core/session" +) + +// MOCA MI1 §8.5 item 5: on a decrypted request, Host must equal the CONNECT host, +// else 421 and an audit event. +// +// Why this is worth enforcing even though the request is always dialed to the +// CONNECT host regardless: bridgedHandler sets r.URL.Host to the authority, so the +// bytes reach the right origin whatever Host says. What a mismatched Host reaches is +// the PIPELINE — pctx.Host is what a plugin reads to choose a credential or a route. +// A guest that CONNECTs to an allowlisted host and then sends `Host: elsewhere` +// inside the tunnel shows one name to the policy layer and another to the socket. + +// mismatchServer builds a Server whose bridged handler can be driven directly, +// without forging TLS: bridgedHandler is an ordinary http.Handler over an +// authority, which is exactly the unit under test. +func mismatchServer(t *testing.T) (*Server, *session.Store) { + t.Helper() + store := session.New(5*time.Minute, 100, 0) + t.Cleanup(store.Close) + p, err := pipeline.New(nil) + if err != nil { + t.Fatal(err) + } + return &Server{ + OutboundPipeline: pipeline.NewHolder(p), + Sessions: store, + Client: http.DefaultClient, + }, store +} + +func TestBridgedHandler_HostMismatch_Is421AndRecorded(t *testing.T) { + s, store := mismatchServer(t) + + req := httptest.NewRequest("GET", "https://evil.example/steal", nil) + req.Host = "evil.example" // the guest's claim, inside a tunnel to api.example + rec := httptest.NewRecorder() + + s.bridgedHandler("api.example:443", discardTunnel()).ServeHTTP(rec, req) + + if rec.Code != http.StatusMisdirectedRequest { + t.Errorf("status = %d, want 421", rec.Code) + } + + // The event must name BOTH hosts: either one alone looks unremarkable, and the + // disagreement is the entire diagnostic. + var found *pipeline.SessionEvent + for _, sid := range []string{session.DefaultSessionID} { + if v := store.View(sid); v != nil { + for i := range v.Events { + if v.Events[i].Error != nil && v.Events[i].Error.Kind == "host_mismatch" { + found = &v.Events[i] + } + } + } + } + if found == nil { + t.Fatal("no host_mismatch event recorded — the refusal must be auditable") + } + if found.Phase != pipeline.SessionDenied { + t.Errorf("phase = %v, want denied", found.Phase) + } + if found.StatusCode != http.StatusMisdirectedRequest { + t.Errorf("event statusCode = %d, want 421", found.StatusCode) + } + if found.Host != "api.example" { + t.Errorf("event host = %q, want the CONNECT host api.example", found.Host) + } + if msg := found.Error.Message; msg == "" { + t.Error("error message empty — it must name the host the guest claimed") + } +} + +// The overwhelmingly common case: a matching Host passes through untouched. Without +// this, a check that refused everything would pass the test above. +func TestBridgedHandler_HostMatches_PassesThrough(t *testing.T) { + var reached bool + // TLS, because bridgedHandler sets the scheme to https — a decrypted request on a + // bridged tunnel is always one the client sent over TLS. + backend := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + reached = true + w.WriteHeader(http.StatusOK) + })) + defer backend.Close() + + s, store := mismatchServer(t) + s.Client = backend.Client() // trusts the test server's forged CA + // Point the bridged request at the test backend rather than a real origin. + authority := backend.Listener.Addr().String() + + req := httptest.NewRequest("GET", "https://"+authority+"/ok", nil) + req.Host = authority + rec := httptest.NewRecorder() + + s.bridgedHandler(authority, discardTunnel()).ServeHTTP(rec, req) + + if rec.Code == http.StatusMisdirectedRequest { + t.Fatal("a matching Host was refused with 421") + } + if !reached { + t.Error("request never reached the origin") + } + if v := store.View(session.DefaultSessionID); v != nil { + for _, e := range v.Events { + if e.Error != nil && e.Error.Kind == "host_mismatch" { + t.Error("a matching Host recorded a host_mismatch event") + } + } + } +} + +// A client may legitimately omit a default :443 from Host while the authority carries +// it. Comparing authority-to-Host verbatim would refuse these, so the comparison is +// host-only — and a port disagreement cannot reach a different origin anyway, because +// the dial uses the authority and not Host. +func TestBridgedHandler_HostWithoutDefaultPort_Matches(t *testing.T) { + s, _ := mismatchServer(t) + + for _, tc := range []struct{ name, authority, host string }{ + {"host omits default port", "api.example:443", "api.example"}, + {"host carries the port", "api.example:443", "api.example:443"}, + {"non-443 origin, port omitted", "api.example:8443", "api.example"}, + {"case differs", "API.example:443", "api.example"}, + } { + t.Run(tc.name, func(t *testing.T) { + req := httptest.NewRequest("GET", "https://"+tc.host+"/x", nil) + req.Host = tc.host + rec := httptest.NewRecorder() + + // Dial will fail (nothing is listening) — that is fine and not what is + // asserted. All that matters is that it was not refused as a mismatch. + s.bridgedHandler(tc.authority, discardTunnel()).ServeHTTP(rec, req) + + if rec.Code == http.StatusMisdirectedRequest { + t.Errorf("Host %q against CONNECT %q was refused 421; ports must not be compared", + tc.host, tc.authority) + } + }) + } +} + +// An empty Host must be refused rather than treated as matching. net/http moves +// HTTP/1.1's Host header to r.Host and fills r.Host from :authority on HTTP/2, so +// reading the Host *header* directly would see "" on both and pass everything — the +// mistake this pins against. +func TestBridgedHandler_EmptyHost_IsRefused(t *testing.T) { + s, _ := mismatchServer(t) + + req := httptest.NewRequest("GET", "https://api.example/x", nil) + req.Host = "" + rec := httptest.NewRecorder() + + s.bridgedHandler("api.example:443", discardTunnel()).ServeHTTP(rec, req) + + if rec.Code != http.StatusMisdirectedRequest { + t.Errorf("status = %d, want 421 — an absent Host names no host and cannot match", rec.Code) + } +} diff --git a/core/listener/forwardproxy/server.go b/core/listener/forwardproxy/server.go index fb5d36653..a638676f7 100644 --- a/core/listener/forwardproxy/server.go +++ b/core/listener/forwardproxy/server.go @@ -949,10 +949,88 @@ func (s *Server) bridgedHandler(authority string, tl *tunnelLog) http.Handler { } r.URL.Scheme = "https" r.URL.Host = authority // host:port — preserves non-443 origins + if !s.hostMatchesConnect(w, r, authority, tl) { + return + } s.serveOutbound(w, r, tl) }) } +// hostMatchesConnect enforces that a decrypted request's Host names the host the +// tunnel was dialed to, answering 421 and recording an event when it does not. +// It reports whether the request may proceed. +// +// This is the bridged path only, and deliberately: it is the one place where a +// request arrives already inside a connection whose destination the proxy chose +// from a CONNECT it approved. On an ordinary forward-proxy request there is no +// second name to disagree with — the Host IS the destination — so there is +// nothing to check. +// +// Why it matters even though the request is dialed correctly regardless: +// bridgedHandler sets r.URL.Host to the authority, so the bytes always reach the +// origin the CONNECT named, whatever Host says. What Host reaches is the +// PIPELINE — pctx.Host is what a plugin reads to pick a credential, a route, or +// a policy. A client that opens a CONNECT to an allowlisted host and then sends +// `Host: elsewhere` inside the tunnel therefore shows one name to the policy +// layer and another to the socket, and a credential chosen for the first would +// travel to the second. Refusing the mismatch is what makes "the credential +// decision is made for the host the connection is dialed to" true rather than +// merely usually true (MOCA MI1 §8.5 item 5). +// +// Compared on host only, with the port excluded. A client may legitimately omit +// a default :443 from Host, hostOnly(authority) is already how the terminator is +// given its name, and a port disagreement cannot reach a different origin — the +// dial uses the authority, not Host. So a port check would refuse correct +// requests and buy no safety. +func (s *Server) hostMatchesConnect(w http.ResponseWriter, r *http.Request, authority string, tl *tunnelLog) bool { + want := hostOnly(authority) + // r.Host, not r.Header.Get("Host"): net/http moves HTTP/1.1's Host header + // there, and for an HTTP/2 request it is already the :authority pseudo-header. + // Reading the header directly sees "" on both, which would pass everything. + got := hostOnly(r.Host) + if strings.EqualFold(got, want) { + return true + } + + slog.Warn("forward-proxy: bridged request Host does not match CONNECT host", + "connect_host", want, "request_host", got, "path", r.URL.Path) + + if s.Sessions != nil { + // Recorded as a denial, like a plugin's terminal rejection: the request + // never reached the pipeline, so there is no paired response event to + // come. The event names both hosts — which is the whole diagnostic + // value, since either one alone looks unremarkable. + pctx := &pipeline.Context{ + Direction: pipeline.Outbound, + Method: r.Method, + Host: want, + Path: r.URL.Path, + StartedAt: time.Now(), + } + sid := s.recordingSessionID("", r.Header, s.clientChain(r)) + ev := pipeline.SessionEvent{ + At: time.Now(), + Direction: pipeline.Outbound, + Phase: pipeline.SessionDenied, + RequestID: pctx.RequestID(), + Host: want, + HTTPMethod: r.Method, + HTTPPath: r.URL.Path, + StatusCode: http.StatusMisdirectedRequest, + Error: &pipeline.EventError{ + Kind: "host_mismatch", + Message: fmt.Sprintf("Host %q does not match CONNECT host %q", got, want), + }, + Client: pctx.ClientInfo(), + } + s.appendOutbound(tl, sid, ev) + } + + http.Error(w, `{"error":"request Host does not match CONNECT host"}`, + http.StatusMisdirectedRequest) + return false +} + // resolveOutboundSessionID picks the bucket an outbound event is recorded // under, in descending order of trustworthiness: // diff --git a/docs/superpowers/specs/2026-10-28-intercepted-websocket-upgrades-design.md b/docs/superpowers/specs/2026-10-28-intercepted-websocket-upgrades-design.md new file mode 100644 index 000000000..af65bb3ba --- /dev/null +++ b/docs/superpowers/specs/2026-10-28-intercepted-websocket-upgrades-design.md @@ -0,0 +1,216 @@ +# WebSocket upgrades on intercepted connections + +**Date:** 2026-10-28 +**Status:** designed +**Issue:** #1488 (parent #1490) +**Contract:** MOCA MI1 §8.5 item 5 + +## Goal + +A sandbox tool that speaks WebSockets through an intercepted host works, with the +CONNECT-host check applied to the upgrade request and one audit event recorded. + +## The contract clause + +MOCA §8.5 item 5, "Every decrypted request", asks for two things this document treats +separately because they have very different costs: + +| Clause | Cost | +|---|---| +| "`Host` (HTTP/1.1) or `:authority` (HTTP/2) must equal the CONNECT host, else `421` and an audit event" | small, and implemented alongside this note | +| "HTTP/2 and WebSocket upgrades are allowed; the `Host` check applies to the upgrade request" | a new relay path — the subject of this note | + +The first is unconditional and applies to every decrypted request, not only upgrades. +It is the narrower, better-understood half, so it ships first and independently; the +upgrade work then has a host check to apply rather than needing to invent one. + +## Prior art: PR #1467 already carries a WebSocket + +**Open PR #1467 ("Carry Codex's Responses WebSocket and parse every turn on it", +`feat/codex-websocket`, `CHANGES_REQUESTED` as of 2026-10-28) builds the relay this +note's §"Proposed shape" describes** — `core/listener/forwardproxy/websocket.go` plus +an RFC 6455 codec in `core/listener/internal/wsframe`. It is scoped to Codex against +OpenAI's hosts (`chatgpt.com`, `api.openai.com`) and gated by +`listener.responses_websockets: parse | decline | forward`, default `parse`. + +That changes this note from a build plan into a scoping question, and it settles +several of the cost arguments below empirically rather than by estimate: + +| This note's claim | What #1467 shows | +|---|---| +| `Client.Do` cannot surrender the connection on a `101` | Confirmed — it completes the upgrade over the bridge's upstream client and hijacks both sides, which is why it needed a new file rather than an edit | +| Frames must not reach the body-reading plugins | Confirmed, and it goes further: it *parses* turns deliberately, feeding `response.create` messages through the pipeline as synthetic `POST`s | +| The handshake needs the `Sec-*` headers intact | Confirmed, and it strips `permessage-deflate` from the offer so frames stay readable | +| A `WritesResponseBody` / `WritesDestination` plugin is a problem | Confirmed — such a pipeline declines the socket with a 426 and the client falls back to HTTPS | + +It also did **not** amend `TestStripForUpstream_StillRemovesTheUpgrade`: the strip +survives as the explicit `forward` mode, with the test pinned to that mode. So the +deliberate-decision reading below is right, and the carve-out is additive to the strip +rather than a replacement for it — a useful precedent for #1488. + +**What #1467 does not do, and #1488 still needs:** + +- **No Host check.** No `421` / `StatusMisdirectedRequest` anywhere in its diff, so the + "the `Host` check applies to the upgrade request" half of item 5 is unmet by it. That + is the half this branch implements, and it applies to #1467's handshake for free, + because the handshake arrives through `bridgedHandler`. +- **Wrong hosts for MOCA.** The carry is deliberately limited to OpenAI, because what + the socket carries is only known for Codex. A MOCA sandbox's tools are arbitrary, so + #1488 needs a path that relays frames it does **not** understand — opaque relay, not + turn parsing. + +**The open question #1467 raises for #1488** is whether to generalise its machinery — +reuse `wsframe` and the hijack/relay with parsing switched off for non-OpenAI +intercepted hosts — or to keep a separate opaque path. Generalising is clearly +preferable to a second codec, but it should wait for #1467 to land, since it is under +review and its internals may move. Recommend #1488's relay be specified as "`wsframe` +relay with no turn parsing, on an intercepted host, behind the Host check" once #1467 +merges, and not started before then. + +## What Cortex does today + +`stripForUpstream` (`core/listener/forwardproxy/server.go:1046`) deletes `Upgrade` +along with the rest of the hop-by-hop set, per RFC 9110 §7.6.1. It also deletes +`Connection`, which matters: a WebSocket handshake needs both. + +The strip is **deliberate and asserted**. PR #1414 added `pctx.RecordStrippedUpgrade` +(`core/pipeline/context.go:844`), called at `server.go:464`, so that the resulting +failure is explainable rather than mysterious — the Codex case, where +`wss://chatgpt.com/backend-api/codex/responses` loses its `Upgrade`, the origin sees a +bare `GET` on an upgrade-or-POST endpoint, and answers `405`. The marker reaches the +timeline as `UpgradeStripped` (`core/pipeline/session.go:252`) and the TUI +(`cmd/agentop/tui/detail_pane.go:126`). + +`upgradestripped_test.go:83`, `TestStripForUpstream_StillRemovesTheUpgrade`, pins the +strip, and its comment is explicit that recording the value while leaving the header in +place "would pass the two tests above while breaking the behaviour they describe". + +**So this is not a bug to fix. It is a deliberate decision to revisit for a case it did +not consider: an intercepted connection whose CONNECT host is known.** Any change here +amends that test rather than merely adding to it, and the amendment should keep its +intent — that a *tunnelled or non-upgrade* request still has `Upgrade` removed. + +## Why the header is the small part + +Keeping `Upgrade` and `Connection` on the forwarded request is necessary and nowhere +near sufficient. The current outbound path cannot carry a handshake: + +1. **The request is made with `s.Client.Do`** (`server.go:1096-1130`). Go's + `http.Transport` does not surrender the connection on a `101`: `Do` returns the + response, and there is no documented route to the underlying `net.Conn` for raw + framing. A WebSocket relay needs the socket, so this path needs a dedicated + round-trip that hijacks both sides rather than `Client.Do`. +2. **`stripForUpstream` runs on resends too** (the `MaxResends` loop at `:1061`). A + resend of an upgrade is meaningless — after a `101` there is no request to resend — + so the upgrade path must bypass the resend loop, not just the strip. +3. **The handshake has four more headers.** `Sec-WebSocket-Key`, + `Sec-WebSocket-Version`, and conditionally `Sec-WebSocket-Protocol` and + `-Extensions` must pass through unmodified, and the `101`'s + `Sec-WebSocket-Accept` must reach the client unrewritten. Any body-rewriting plugin + on the response path would corrupt the handshake. +4. **Downstream needs a hijack.** The bridged handler writes through an + `http.ResponseWriter` (`bridgedHandler`, `server.go:941`). Relaying frames means + hijacking that too and copying bytes both ways until either side closes. +5. **Plugins must not see frames.** §8.5 says bodies on this path are "streamed, never + inspected or buffered". WebSocket frames are not HTTP bodies; the SSE/MCP/inference + parsers would read them as garbage. The relay must run with the body-reading + pipeline disengaged. + +That is a new code path parallel to the existing request path, not an edit to it. It is +the reason this note stops at design and does not carry an implementation. + +## Proposed shape + +Written before #1467 was found; read it as the shape to reach by **generalising +#1467's relay**, not as a separate implementation to start from scratch. The +conditions and the event accounting below still stand. + +A narrow carve-out, taken only where all three conditions hold: + +- the connection is **intercepted** (`bridgedHandler`, so a CONNECT authority is known), +- the request is a well-formed WebSocket upgrade (`Connection` contains `Upgrade`, + `Upgrade: websocket`, `Sec-WebSocket-Key` present, `GET`), +- the Host check passes. + +Everything else keeps today's strip and today's marker. Concretely: + +1. In `bridgedHandler`, apply the Host check (shipping now, see below). On mismatch, + `421` + audit event; the upgrade never starts. +2. Detect the upgrade and branch to a `serveUpgrade` path before `serveOutbound`. +3. `serveUpgrade` dials the origin itself, replays the handshake with the `Sec-*` + headers and `Upgrade`/`Connection` intact, and requires a `101`. A non-`101` is + relayed back as an ordinary response — the origin declining is not our error. +4. On `101`, hijack the downstream conn, write the response head verbatim, then + `io.Copy` both directions with the pipeline disengaged. +5. Record **one** audit event at handshake time, in §8.5's shape, with the + `UpgradeStripped` field left empty — nothing was stripped — and a new field saying + the upgrade was *allowed*. The "one event" in the issue's "Done when" is this one; + frames are not individually evented. + +### Open questions for review + +Three of the four below are **answered by #1467**, which is why adopting its machinery +is the recommendation rather than one option among several. + +- **~~`UpgradeStripped` vs. a new field~~ — answered.** #1467 needs no new field: a + carried socket simply leaves `UpgradeStripped` empty (asserted at + `websocket_review_test.go`, "carries upgradeStripped=%q on a carried socket") and + the socket is identified by `tunnelReason: "websocket"` on its open/close pair + instead. Reuse that vocabulary — an `UpgradeAllowed` sibling is unnecessary. +- **~~Idle timeouts~~ — answered.** #1467 models the socket as a *tunnel*, not a + request: the handshake `GET` is the open, the close carries status 101 with the whole + socket's duration and bytes, and turns are request/response pairs between them. + Tunnel rows are already excluded from `/v1/usage` ("a tunnel's lifetime is not a + request latency"), which is exactly the accounting a long-lived relay needs. +- **~~h2c~~ — answered, and moot.** #1467 records that an HTTP/2 client never reaches + the handler at all: Go's HTTP/2 server refuses a request carrying `Upgrade` with a + 400. Combined with the terminator offering `h2` via ALPN + (`core/tlsbridge/terminator.go:14`), intercepted h2 arrives negotiated and never + upgraded. Scope h2c out of #1488. +- **Interaction with `MOCA_EGRESS_INSPECT` — still open.** A host on the inspect list + wants request detail, which an opaque frame relay cannot give. #1467 does not face + this because it *parses* its one known protocol; a generalised opaque relay does. + The likely answer is that an inspect-listed host refuses the upgrade rather than + relaying blind, but that is MOCA's call to make. + +## The Host check, shipping now + +Independent of the above, and the prerequisite for it. + +**Finding: Cortex enforces nothing like this today.** A search for `421` / +`StatusMisdirectedRequest` across the tree returns only an unrelated number in a +comment (`core/pipeline/promptcontext.go:34`). An intercepted connection currently +accepts any `Host` on a decrypted request. + +This matters beyond upgrades. `bridgedHandler` sets `r.URL.Host = authority` +(`server.go:952`) — the CONNECT authority — so the request is *dialed* to the right +origin regardless. But `Host` is what a plugin reads for its credential decision +(`pctx.Host`), and §8.5 wants the credential decision made "always for the host the +connection is dialed to". A guest that opens a CONNECT to an allowlisted host and then +sends `Host: other.example` inside the tunnel presents one host to the policy layer and +another to the socket. Closing that is the point of the clause. + +`bridgedHandler` is the correct and only insertion point: it is the sole path where a +decrypted request has a known CONNECT authority to be checked against. + +Comparison is on host only, with the port excluded — `hostOnly(authority)` is already +how the terminator is given its name (`server.go:826`), a client may legitimately omit +a default `:443` from `Host`, and a port mismatch cannot reach a different origin +because the dial uses the authority, not `Host`. + +## Testing + +In-process Go, no cluster: the "Done when" is a WebSocket echo through an intercepted +host and one audit event, which an `httptest` origin plus a real `Server` covers +directly. #1467 proves the approach — its `websocket_test.go` drives a real CONNECT +bridge to a TLS WebSocket origin addressed as `chatgpt.com` — so #1488's echo test +should be built on that fixture rather than a new one. A Kind-based MOCA sandbox run is +worth doing once the relay exists, to exercise a real tool against a real origin, but +it would prove nothing the Go test cannot about the handshake itself. + +The Host check ships in this branch with tests for: a mismatch answering `421` and +recording a `host_mismatch` denial event naming both hosts; a matching `Host` reaching +a TLS origin; four port and case variants (default `:443` omitted, port present, +non-443 origin, differing case) all matching; and an empty `Host` refused — that last +pinning against reading the `Host` *header* instead of `r.Host`, which would see `""` +on both HTTP/1.1 and HTTP/2 and so pass everything.