diff --git a/CLAUDE.md b/CLAUDE.md index 789cfc8e3..4b4e50340 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -611,6 +611,24 @@ Every event on `/v1/sessions/{id}` and `/v1/events` carries: destination resolved into loopback, link-local, private or this host's own address space and no permit covered it. The Finisher sees an allow with status 403, like the 426, and `/v1/usage` does not count it as an error. + **A 403 `destination_denied` refusal is the egress allow list's answer, not + the upstream's** (`listener.egress_allow_hosts`, or `MOCA_EGRESS_ALLOW`; + #1483): the destination was not on the deny-by-default list, and the proxy + refused it by name — header `X-Moca-Egress-Error`, text/plain body + `destination_denied: \n`, the moca egress conformance contract's + shape — before the pipeline, before DNS and before any dial. Off-list + CONNECT targets, absolute-form hosts, and the target a plugin's redirect + moved a request to all land this way, as a `phase: "denied"` row carrying + an `egress-allowlist` invocation whose `details.host` names the host; the + row's own `host` is the full authority, as every outbound row's is. + **A skip_hosts pattern cannot exempt an off-list host:** the allowlist is + checked first — it is the outer boundary, and a bypass must not become an + egress bypass. + **The two 403s are different boundaries and compose** — the allow list + judges the NAME the client asked for, before DNS; the egress guard judges + the ADDRESS that name resolved to, at the dial. A host can be on the + allow list and still be refused `destination_private`, which is why an + allow-list entry is not a permit for the guard. **A request whose body could not be buffered is the other exception:** it never reached the pipeline, so its request row carries no parse and no Finisher runs. Over the forward proxy's `maxRequestBodySize` (32 MiB) @@ -1191,6 +1209,15 @@ resulting `/shared/client-id.txt` and `/shared/client-secret.txt`. ``` Any change to `listener.skip_hosts` requires a pod restart (same rule as other `listener.*` fields). Do NOT add hosts here that need IBAC / token-exchange policy applied — bypass means bypass. + - **Egress boundary — `listener.egress_allow_hosts`** (#1483, MOCA MI1 §8.5): the forward proxy's deny-by-default destination allow list. When non-empty, an outbound destination not on the list — a CONNECT target, an absolute-form `http://` host, or the host a plugin's redirect moved a request to — is refused with 403 `destination_denied` naming the host, in the moca egress conformance contract's refusal shape (header `X-Moca-Egress-Error`, text/plain body `destination_denied: \n`), before the pipeline, before DNS resolution and before any dial, so an off-list host is answered by name rather than left to time out. Entries are exact-match `host:port` — no globs, and the shapes that would widen the boundary (portless, wildcard) are startup errors, as is an unbracketed IPv6 literal, which would *empty* it (`2001:db8::1` reads its last colon as the port separator and can never match — write `[2001:db8::1]:443`); a destination naming no port is judged at its scheme's (80 http, 443 otherwise). A destination off the wire is never trusted to parse: a malformed authority is denied by name, never a panic. When the config field is empty, the binary falls back to the `MOCA_EGRESS_ALLOW` env var (comma-separated `host:port`) — the channel the MOCA conformance harness drives the target with. Both empty keeps egress exactly as it was. **The allowlist is checked BEFORE `skip_hosts`** — when on, it is the outer boundary, and a skip pattern cannot exempt an off-list host. Example: + ```yaml + listener: + egress_allow_hosts: + - "github-tool-mcp:8080" + - "api.anthropic.com:443" + ``` + Env delivery instead: `MOCA_EGRESS_ALLOW=github-tool-mcp:8080,api.anthropic.com:443`. Any change requires a pod restart (same rule as other `listener.*` fields). Each refusal leaves a `phase: "denied"` session row carrying an `egress-allowlist` invocation naming the host. **Scope: the forward proxy only** — "outer boundary" is outer with respect to `skip_hosts`, not with respect to every exit from the pod. **Egress that iptables REDIRECTs to `listener.transparent_proxy_addr` (proxy-sidecar enforce-redirect mode) is not filtered by it** — that listener runs the shared pipeline but dials its recovered destination without consulting the list — and the extproc listener (envoy-sidecar) has no wiring at all: only `cmd/cortex` builds a matcher, onto the forward proxy server. Note the contrast with `skip_hosts`, which the transparent listener also does not consult: there the omission hardens the boundary (a skip cannot self-exempt), here it leaves a way around this one. That matches the issue's scope (MOCA shape A is the proxy-sidecar forward proxy, CONNECT plus absolute-form), but a deployment that also captures traffic by iptables REDIRECT, or runs envoy-sidecar, must hold its egress somewhere else too: deny-by-default here is not deny-by-default for the pod. The sibling `listener.egress_guard` (#1539) is a different boundary and composes — this list judges the **name** before DNS, the guard judges the **address** it resolved to at the dial, so an allow-list entry is not a permit for the guard. + - **Backstop — intent pin in the eviction policy**: this one only applies when `session.max_events` is set, because with no cap there is no FIFO eviction to survive. Where it is set, and even with `skip_hosts` empty, the session store pins the most-recent inbound A2A request event against that eviction. If the buffer overflows, every other event evicts in normal chronological order; the protected intent stays at its original timestamp, leaving a visible time gap in the timeline. Older intents from earlier turns are NOT pinned — only the latest one — so a multi-turn conversation with huge fan-out can't pile up stale intents and starve the buffer. The pin protects against FIFO eviction only: IBAC's `LastIntent()` survives buffer overflow as long as the session is still alive and an inbound A2A request has landed in it, but can still return nil after explicit deletion, eviction once `max_sessions` is exceeded, a configured `session.ttl` elapsing, or before the first inbound request arrives. The pin is defense-in-depth; reach for `skip_hosts` first when the offending traffic is identifiable infrastructure. ## DCO Sign-Off (Mandatory) diff --git a/cmd/cortex/egressallow_test.go b/cmd/cortex/egressallow_test.go new file mode 100644 index 000000000..5b1aaec6a --- /dev/null +++ b/cmd/cortex/egressallow_test.go @@ -0,0 +1,41 @@ +package main + +import "testing" + +// resolveEgressAllow picks the allow list's source: config wins over the +// MOCA_EGRESS_ALLOW env var, the env var is split on commas with blanks +// dropped, and nothing configured anywhere means the feature is off (nil). +// The env var is the MOCA conformance harness's only channel into the target, +// so the fallback is the contract, not a convenience. +func TestResolveEgressAllow(t *testing.T) { + configured := []string{"a.test:443", "b.test:80"} + if got := resolveEgressAllow(configured, func(string) string { + t.Fatal("config entries must win without consulting the environment") + return "" + }); len(got) != 2 || got[0] != "a.test:443" || got[1] != "b.test:80" { + t.Errorf("config wins: got %v, want the configured entries", got) + } + + env := func(name, value string) func(string) string { + return func(k string) string { + if k == envEgressAllow { + return value + } + return "" + } + } + + if got := resolveEgressAllow(nil, env(envEgressAllow, " a.test:443 , ,b.test:80 ")); len(got) != 2 || + got[0] != "a.test:443" || got[1] != "b.test:80" { + t.Errorf("env fallback: got %v, want the two comma-separated entries, trimmed", got) + } + if got := resolveEgressAllow(nil, env(envEgressAllow, " ")); got != nil { + t.Errorf("blank env: got %v, want nil — off, not an empty-but-armed list", got) + } + if got := resolveEgressAllow(nil, env(envEgressAllow, "")); got != nil { + t.Errorf("unset env: got %v, want nil", got) + } + if got := resolveEgressAllow(nil, func(string) string { return "" }); got != nil { + t.Errorf("nothing configured: got %v, want nil", got) + } +} diff --git a/cmd/cortex/main.go b/cmd/cortex/main.go index 994c78f01..423534425 100644 --- a/cmd/cortex/main.go +++ b/cmd/cortex/main.go @@ -54,6 +54,7 @@ import ( // Only HTTP listeners are compiled in: no extproc // (no gRPC, no envoy types). + "github.com/rossoctl/cortex/core/listener/egressallow" "github.com/rossoctl/cortex/core/listener/egressguard" "github.com/rossoctl/cortex/core/listener/forwardproxy" "github.com/rossoctl/cortex/core/listener/reverseproxy" @@ -275,6 +276,34 @@ func ledgerDefaultOnValue(cfg *config.Config, configPath string) bool { return on } +// envEgressAllow is the MOCA egress contract's delivery channel: the +// conformance harness arms the target's allow list by setting it, with no +// config file involved. The name is that contract's, used verbatim. +const envEgressAllow = "MOCA_EGRESS_ALLOW" + +// resolveEgressAllow picks the egress allow list's source: the config +// field when it names entries, else MOCA_EGRESS_ALLOW (comma-separated +// host:port) when that is set, else nothing — the feature off. Config +// wins over the env var so an operator who sets both gets one answer, +// and getenv is a parameter so tests drive it without touching the +// process environment. +func resolveEgressAllow(configured []string, getenv func(string) string) []string { + if len(configured) > 0 { + return configured + } + raw := strings.TrimSpace(getenv(envEgressAllow)) + if raw == "" { + return nil + } + var entries []string + for _, e := range strings.Split(raw, ",") { + if e = strings.TrimSpace(e); e != "" { + entries = append(entries, e) + } + } + return entries +} + // closeLedgerOnFatal hands the ledger's open minute back before a fatal exit. Set once, when the // ledger opens; nil whenever there is no ledger to flush. // @@ -1014,6 +1043,21 @@ func main() { fatalf("listener.skip_hosts: %v", serr) } fpSrv.SkipHosts = skipHosts + // Egress allow list (#1483): deny-by-default destinations for the + // forward proxy — off-list CONNECT targets and absolute-form hosts + // are refused with 403 destination_denied before any resolve or + // dial. Config first; MOCA_EGRESS_ALLOW is the fallback channel + // because it is the one the MOCA conformance harness drives the + // target with. Both empty keeps egress exactly as it was. + if entries := resolveEgressAllow(cfg.Listener.EgressAllowHosts, os.Getenv); len(entries) > 0 { + allow, aerr := egressallow.New(entries) + if aerr != nil { + fatalf("listener.egress_allow_hosts: %v", aerr) + } + fpSrv.EgressAllow = allow + slog.Info("egress allow list on: off-list destinations refused with 403 destination_denied", + "entries", len(entries)) + } fpSrv.TLSBridge = bridge fpSrv.Shared = sharedStore // A Responses API WebSocket is answered 426 so Codex falls back to HTTPS at once, diff --git a/core/config/config.go b/core/config/config.go index 1d4e45ef2..1e5af8f9c 100644 --- a/core/config/config.go +++ b/core/config/config.go @@ -928,6 +928,10 @@ type ListenerConfig struct { // consult SkipHosts — that is the hard egress guard and must not // be self-exemptable via the agent's outbound destination. // + // When EgressAllowHosts (or MOCA_EGRESS_ALLOW) is non-empty, the + // allowlist is checked FIRST and a skip pattern cannot exempt an + // off-list host: the allowlist is the outer boundary. + // // Match-all patterns ("*", "**", whitespace-only) and patterns // containing ":port" are rejected at startup so a single // misconfigured entry can't silently disable all outbound @@ -935,6 +939,59 @@ type ListenerConfig struct { // in #496. SkipHosts []string `yaml:"skip_hosts" json:"skip_hosts"` + // EgressAllowHosts is the forward proxy's deny-by-default destination + // allow list (#1483, MOCA MI1 §8.5): when non-empty, an outbound + // destination not on the list is refused with 403 destination_denied + // naming the host — header X-Moca-Egress-Error, text/plain body + // "destination_denied: \n", the moca egress conformance + // contract's refusal shape — before the plugin pipeline, before DNS + // resolution and before any dial, so an off-list host is answered by + // name rather than left to time out. Applies to CONNECT targets, + // absolute-form http:// hosts, and the target a plugin's redirect + // moved a request to. + // + // Entries are "host:port", exact match (no globs — a wildcard in a + // security boundary is one typo from "allow everything", so the shapes + // that would widen it are rejected at startup). Hosts compare + // lowercased without a trailing dot; a destination naming no port + // defaults to its scheme's (80 http, 443 otherwise). + // + // Checked BEFORE skip_hosts: when the list is on it is the outer + // boundary, and a skip_hosts entry for an off-list host is refused, + // not honored — bypass must not become an egress bypass. + // + // When this field is empty, the binary falls back to the + // MOCA_EGRESS_ALLOW environment variable (comma-separated host:port + // entries) — the channel the MOCA conformance harness drives the + // target with. Both empty means the feature is off and outbound + // behavior is unchanged. + // + // SCOPE — this gates the FORWARD PROXY's paths only, and "outer + // boundary" above means outer with respect to skip_hosts, not with + // respect to every way a byte can leave the pod. Not covered: + // + // - the transparent listener (listener.transparent_proxy_addr, + // proxy-sidecar enforce-redirect mode): egress that iptables + // REDIRECTs there is NOT filtered by this list. + // HandleTransparentConn runs the shared pipeline but dials the + // recovered destination without consulting it. Note the contrast + // with SkipHosts above, which that listener also does not consult + // — there the omission hardens the boundary, here it leaves a way + // around this one. + // - the extproc listener (envoy-sidecar mode): no wiring at all — + // only cmd/cortex builds a Matcher, and only onto the forward + // proxy server. + // + // MOCA shape A is the proxy-sidecar forward proxy and #1483 names + // CONNECT plus absolute-form only, so that is the scope this field + // claims. A deployment that also captures traffic by iptables + // REDIRECT, or runs envoy-sidecar, needs its egress held somewhere + // else too — deny-by-default here is not deny-by-default for the pod. + // + // A listener.* field: changing it requires a restart, like every other + // listener knob. + EgressAllowHosts []string `yaml:"egress_allow_hosts,omitempty" json:"egress_allow_hosts,omitempty"` + // DeclineResponsesWebSockets: whether the forward proxy answers a WebSocket // upgrade on an OpenAI Responses API path (one ending /responses, on chatgpt.com // or api.openai.com) with 426 itself, instead of forwarding the request with its Upgrade stripped. nil means diff --git a/core/listener/egressallow/egressallow.go b/core/listener/egressallow/egressallow.go new file mode 100644 index 000000000..40190da01 --- /dev/null +++ b/core/listener/egressallow/egressallow.go @@ -0,0 +1,207 @@ +// Package egressallow implements the forward proxy's deny-by-default +// destination allow list (#1483, MOCA MI1 §8.5): a sandbox may reach only +// the host:port entries the operator lists, and a request for anything +// else is refused by name — never left to time out — before DNS +// resolution and before any dial. +// +// Matching is exact, host:port against host:port. That is deliberate and +// narrower than every other host list in this tree: skip_hosts and +// authproxy-routes take globs because they route, but an allowlist is a +// security boundary, and a wildcard in it is one typo away from "allow +// everything" — so the shapes that would widen the boundary are rejected +// at construction instead of interpreted. +// +// Host normalization mirrors forwardproxy.splitAuthority so a request and +// an entry written in different but equivalent spellings agree: hosts +// compare lowercased without a trailing dot, IPv6 literals by their +// unmapped value, and a destination that names no port defaults to the +// scheme's (80 for http, 443 for everything else, CONNECT included). +// +// The package depends on nothing inside core, so the listener and the +// binary that arms it can import it without risking an import cycle, and +// tests can exercise the matcher in isolation from the listener machinery. +package egressallow + +import ( + "fmt" + "net/netip" + "strconv" + "strings" +) + +// Matcher answers "is this destination on the allow list?". A nil Matcher +// allows everything — the feature off, today's behavior — so callers gate +// on the nil check rather than on a configured-but-empty distinction. +type Matcher struct { + entries map[string]struct{} +} + +// New builds a Matcher from raw "host:port" entries. Construction-time +// guards reject the entries that would silently widen the boundary: +// +// - empty / whitespace-only entries: no intent expressed. +// - portless entries: the contract is host:port, and guessing a port +// (or matching any) would widen the boundary the list exists to hold. +// A portless entry is almost certainly a typo, so it is a startup +// error, mirroring skiphost's rejection of colon-bearing patterns — +// the same "would never do what the operator meant" shape. +// - non-numeric or out-of-range ports, and an empty host. +// - "*" / "**" hosts: exact match means these could only ever match a +// literal host of that name, which is not what anyone means by them. +// - unbracketed IPv6 literals ("2001:db8::1"): the last colon reads as +// the port separator, leaving an entry no destination can match. That +// empties the boundary rather than widening it, but it is just as +// wrong, so it is an error naming "[
]:". +// +// Hosts are normalized (see the package comment), so "Example.COM:443" +// and "example.com:443" are one entry. An empty input is valid and yields +// a Matcher that allows nothing — but note cmd/cortex only builds a +// Matcher when the resolved entry list is non-empty, so an empty config +// keeps the feature off rather than refusing all egress. +func New(entries []string) (*Matcher, error) { + m := &Matcher{entries: make(map[string]struct{}, len(entries))} + for _, e := range entries { + key, err := normalizeEntry(e) + if err != nil { + return nil, err + } + m.entries[key] = struct{}{} + } + return m, nil +} + +// normalizeEntry validates one raw entry and returns its match key. +func normalizeEntry(e string) (string, error) { + trimmed := strings.TrimSpace(e) + if trimmed == "" { + return "", fmt.Errorf("egressallow: empty entry in the allow list") + } + host, port, err := splitHostPort(trimmed) + if err != nil { + return "", fmt.Errorf("egressallow: entry %q: %w", e, err) + } + if port == 0 { + return "", fmt.Errorf("egressallow: entry %q names no port; the contract is host:port, "+ + "and a guessed port would widen the boundary the list exists to hold", e) + } + if host == "" { + return "", fmt.Errorf("egressallow: entry %q has no host", e) + } + if host == "*" || host == "**" { + return "", fmt.Errorf("egressallow: entry %q is a wildcard; the allow list is exact-match "+ + "by design — list each host:port the sandbox may reach", e) + } + // A colon left in the host of an entry that was NOT bracketed means an + // unbracketed IPv6 literal: "2001:db8::1" splits at its last colon into + // host "2001:db8:" and port "1" — a valid-looking entry that no + // destination can ever match, because a request carries a v6 literal + // bracketed and that normalizes to "2001:db8::1". The operator would + // believe they allowed a host and have allowed nothing. It fails closed, + // but an entry that silently empties the boundary is a startup error + // here just as a portless or wildcard one is — named, with the spelling + // that works. The bracketed form is excluded because its host keeps its + // colons legitimately: "[::1]:443" → host "::1", port 443. + if !strings.HasPrefix(trimmed, "[") && strings.Contains(host, ":") { + return "", fmt.Errorf("egressallow: entry %q looks like an unbracketed IPv6 literal; "+ + "write it as [
]: — unbracketed, the last colon is read as the "+ + "port separator and the entry can never match", e) + } + return host + ":" + strconv.Itoa(port), nil +} + +// Allows reports whether dest (a "host:port" dial target, or a bare host +// whose port defaults to the scheme's) is on the list. A nil Matcher +// allows everything; a Matcher built from entries allows only what was +// listed — the deny-by-default contract. scheme defaults the port when +// dest names none: "http" means 80, anything else 443 (CONNECT tunnels +// are TLS, so 443 is the honest default for "tcp"). +// +// A dest that cannot be parsed (empty host, bad port) is NOT allowed: +// deny-by-default means an unparseable destination is refused, not +// guessed at. +func (m *Matcher) Allows(dest, scheme string) bool { + if m == nil { + return true + } + host, port, err := splitHostPort(strings.TrimSpace(dest)) + if err != nil || host == "" { + return false + } + // A destination that names no port is read at its scheme's — the + // same default splitAuthority gives a Host header. + if port == 0 { + port = defaultPort(scheme) + } + _, ok := m.entries[host+":"+strconv.Itoa(port)] + return ok +} + +// defaultPort is the port a destination without one is read as. +func defaultPort(scheme string) int { + if strings.EqualFold(scheme, "http") { + return 80 + } + return 443 +} + +// splitHostPort splits "host:port" into a normalized host and a numeric +// port. The port may be absent (port == 0, no error) — the caller decides +// what that means. IPv6 literals carry their brackets, which +// net.SplitHostPort understands but a bare strings.Split would not. +func splitHostPort(s string) (host string, port int, err error) { + host, p, serr := splitPort(s) + if serr != nil { + return "", 0, serr + } + host = strings.TrimSuffix(strings.ToLower(host), ".") + if ip, perr := netip.ParseAddr(host); perr == nil { + host = ip.Unmap().String() + } + if p == "" { + return host, 0, nil + } + n, aerr := strconv.Atoi(p) + if aerr != nil || n < 1 || n > 65535 { + return "", 0, fmt.Errorf("port %q is not a number 1-65535", p) + } + return host, n, nil +} + +// splitPort separates the port from the host, tolerating bracketed IPv6 +// literals ("[::1]:443" → "::1", "443"; "[::1]" → "::1", "") and returning +// the host WITHOUT its brackets, so netip.ParseAddr in the caller can +// canonicalize it. +// +// The bracketed branch is taken only on a LEADING "[", never on a stray +// "]" anywhere in the string. The destination here is client-chosen — a +// CONNECT authority or an absolute-form URL host — so "]:443" arrives from +// the wire, and reading it as a bracketed literal would index s[1:0] and +// panic. A parser on a trust boundary does not get to panic: a stray "]" +// is simply not host:port, and falls through to be denied like any other +// unparseable destination. +func splitPort(s string) (host, port string, err error) { + // A leading bracket means the host part is a bracketed IPv6 literal; + // everything after its closing colon is the port. + if strings.HasPrefix(s, "[") { + i := strings.LastIndex(s, "]") + if i < 0 { + return "", "", fmt.Errorf("%q is not host:port", s) + } + host = s[1:i] // strip the brackets + rest := s[i+1:] + if rest == "" { + return host, "", nil + } + if !strings.HasPrefix(rest, ":") { + return "", "", fmt.Errorf("%q is not host:port", s) + } + return host, rest[1:], nil + } + if strings.Contains(s, "]") { + return "", "", fmt.Errorf("%q is not host:port", s) + } + if i := strings.LastIndex(s, ":"); i >= 0 { + return s[:i], s[i+1:], nil + } + return s, "", nil +} diff --git a/core/listener/egressallow/egressallow_test.go b/core/listener/egressallow/egressallow_test.go new file mode 100644 index 000000000..20885a8c4 --- /dev/null +++ b/core/listener/egressallow/egressallow_test.go @@ -0,0 +1,213 @@ +package egressallow + +import ( + "strings" + "testing" +) + +// TestNew_RejectsWideningEntries pins the construction guards: the entries that +// would silently widen — or silently narrow to nothing — the boundary the list +// exists to hold are startup errors, not interpreted. +func TestNew_RejectsWideningEntries(t *testing.T) { + for _, e := range []string{ + "", + " ", + "example.com", // portless: the contract is host:port + "example.com:", // empty port + "example.com:http", // service names are not the contract either + "example.com:0", + "example.com:65536", + "example.com:notaport", + ":443", // no host + "*:443", // wildcard host + "**:443", + } { + if _, err := New([]string{e}); err == nil { + t.Errorf("New(%q) = nil error, want a startup error", e) + } + } +} + +// TestNew_AcceptsExactEntries: the shapes the contract names compile. +func TestNew_AcceptsExactEntries(t *testing.T) { + if _, err := New([]string{"allowed.test:443", "Example.COM:80", "[::1]:8443"}); err != nil { + t.Fatalf("New: %v", err) + } +} + +// TestAllows_ExactMatch is the core contract: on-list allows, everything else +// — other host, other port, other case spelling of the same pair — is denied. +func TestAllows_ExactMatch(t *testing.T) { + m, err := New([]string{"allowed.test:443", "allowed.test:8080", "other.test:80"}) + if err != nil { + t.Fatalf("New: %v", err) + } + for _, allowed := range []string{ + "allowed.test:443", + "ALLOWED.test:443", // host compares lowercased + "allowed.test.:443", // trailing dot is the same name + "allowed.test:8080", + "other.test:80", + } { + if !m.Allows(allowed, "https") { + t.Errorf("Allows(%q) = false, want true", allowed) + } + } + for _, denied := range []string{ + "off.test:443", + "allowed.test:8443", // right host, wrong port + "allowed.test:80", // portless defaults by scheme, and 80 is not listed for it + "sub.allowed.test:443", + "", + "allowed.test:notaport", + } { + if m.Allows(denied, "https") { + t.Errorf("Allows(%q) = true, want false", denied) + } + } +} + +// TestAllows_PortlessDestinationDefaultsByScheme: a destination naming no port +// is read at its scheme's — http 80, https and CONNECT's tcp 443 — and judged +// against the entry for that pair. +func TestAllows_PortlessDestinationDefaultsByScheme(t *testing.T) { + m, err := New([]string{"host.test:80"}) + if err != nil { + t.Fatalf("New: %v", err) + } + if !m.Allows("host.test", "http") { + t.Error("Allows(host.test, http) = false, want true — portless http is host.test:80") + } + if m.Allows("host.test", "https") { + t.Error("Allows(host.test, https) = true, want false — portless https is host.test:443, not listed") + } + if m.Allows("host.test", "tcp") { + t.Error("Allows(host.test, tcp) = true, want false — CONNECT's portless default is 443, not listed") + } +} + +// TestAllows_IPv6Literals: bracketed v6 destinations — the only spelling a +// URL host, Host header or CONNECT authority carries them in — match their +// entries. An unbracketed v6 literal cannot be told from a host:port split +// gone wrong, so it is denied rather than guessed at (fail closed, same rule +// as any unparseable destination). +func TestAllows_IPv6Literals(t *testing.T) { + m, err := New([]string{"[2001:db8::1]:443", "203.0.113.7:80"}) + if err != nil { + t.Fatalf("New: %v", err) + } + if !m.Allows("[2001:db8::1]:443", "https") { + t.Error("Allows([2001:db8::1]:443) = false, want true") + } + if !m.Allows("[2001:0db8::1]:443", "https") { + t.Error("Allows([2001:0db8::1]:443) = false, want true — same address, longer spelling") + } + if !m.Allows("203.0.113.7:80", "http") { + t.Error("Allows(203.0.113.7:80) = false, want true") + } + if m.Allows("::ffff:203.0.113.7", "http") { + t.Error("Allows(unbracketed v6) = true, want false — fail closed, not guessed") + } +} + +// TestAllows_NilMatcherAllowsEverything: nil is the feature off — today's +// behavior, byte for byte. The zero-value matcher (empty entry set) allows +// nothing, which is why cmd/cortex only builds one for a non-empty list. +func TestAllows_NilMatcherAllowsEverything(t *testing.T) { + var m *Matcher + if !m.Allows("anything.test:1", "https") { + t.Error("nil Matcher denied a destination; nil means the feature is off") + } + empty, err := New(nil) + if err != nil { + t.Fatalf("New(nil): %v", err) + } + if empty.Allows("anything.test:443", "https") { + t.Error("empty matcher allowed a destination; built = deny-by-default") + } +} + +// TestAllows_StrayBracketDeniedNotPanic: the destination comes off the wire, +// so a hostile spelling must be denied rather than panic. A bare "]" used to +// take the bracketed-literal branch on a LastIndex hit at 0 and index s[1:0]; +// only a LEADING "[" takes that branch now, and a stray "]" is not host:port. +func TestAllows_StrayBracketDeniedNotPanic(t *testing.T) { + m, err := New([]string{"allowed.test:443"}) + if err != nil { + t.Fatalf("New: %v", err) + } + for _, dest := range []string{ + "]:443", + "]", + "]]:443", + "a]b:443", + "[::1", // opened, never closed + "[]:443", + "[::1]x", // closed, then junk instead of :port + "allowed.test]:443", + } { + func() { + defer func() { + if r := recover(); r != nil { + t.Errorf("Allows(%q) panicked: %v", dest, r) + } + }() + if m.Allows(dest, "https") { + t.Errorf("Allows(%q) = true, want false", dest) + } + }() + } + // The spelling that is genuinely bracketed still parses. + v6, err := New([]string{"[::1]:443"}) + if err != nil { + t.Fatalf("New([::1]:443): %v", err) + } + if !v6.Allows("[::1]:443", "https") { + t.Error("Allows([::1]:443) = false, want true — the bracketed branch must still work") + } +} + +// TestNew_RejectsUnbracketedIPv6Entry: an unbracketed v6 entry parses into a +// host:port that no destination can match ("2001:db8::1" → host "2001:db8:", +// port 1), so the operator would believe they allowed a host and have allowed +// nothing. It fails closed, but the contract is that entries which silently +// empty the boundary are startup errors — so it is one, naming the bracketed +// spelling that works. +func TestNew_RejectsUnbracketedIPv6Entry(t *testing.T) { + for _, e := range []string{"::1", "2001:db8::1", "fd00::2"} { + _, err := New([]string{e}) + if err == nil { + t.Errorf("New(%q) = nil error, want a startup error", e) + continue + } + if !strings.Contains(err.Error(), "IPv6") { + t.Errorf("New(%q) error = %q, want it to name the IPv6 spelling", e, err) + } + } + // A v6 entry whose last segment is not numeric trips the port check + // first — a zone id, or an embedded v4 address. Still a startup error, + // just reported as the bad port rather than as the bracket spelling. + for _, e := range []string{"fe80::1%eth0", "::ffff:203.0.113.7"} { + if _, err := New([]string{e}); err == nil { + t.Errorf("New(%q) = nil error, want a startup error", e) + } + } + // The bracketed spellings the message points at are accepted. + if _, err := New([]string{"[::1]:443", "[2001:db8::1]:8443"}); err != nil { + t.Errorf("New(bracketed v6) = %v, want nil", err) + } +} + +// TestAllows_UnparseableDestinationDenied: deny-by-default means a destination +// that cannot be parsed is refused, not guessed at. +func TestAllows_UnparseableDestinationDenied(t *testing.T) { + m, err := New([]string{"allowed.test:443"}) + if err != nil { + t.Fatalf("New: %v", err) + } + for _, dest := range []string{"allowed.test:0", "allowed.test:99999", ":443"} { + if m.Allows(dest, "https") { + t.Errorf("Allows(%q) = true, want false — unparseable is denied", dest) + } + } +} diff --git a/core/listener/forwardproxy/egressallow_gate_test.go b/core/listener/forwardproxy/egressallow_gate_test.go new file mode 100644 index 000000000..2407edf26 --- /dev/null +++ b/core/listener/forwardproxy/egressallow_gate_test.go @@ -0,0 +1,349 @@ +package forwardproxy + +import ( + "io" + "net/http" + "net/http/httptest" + "strings" + "sync/atomic" + "testing" + "time" + + "github.com/rossoctl/cortex/core/auth" + "github.com/rossoctl/cortex/core/listener/egressallow" + "github.com/rossoctl/cortex/core/listener/skiphost" + "github.com/rossoctl/cortex/core/pipeline" + "github.com/rossoctl/cortex/core/session" +) + +// The egress allow list (#1483, MOCA MI1 §8.5): when on, an off-list +// destination — CONNECT target or absolute-form http:// host — is refused +// with 403 destination_denied naming the host, in the moca egress +// conformance suite's refusal shape (header X-Moca-Egress-Error, text/plain +// body "destination_denied: \n"), before the pipeline, before DNS and +// before any dial. The in-tree pin for "the upstream was never contacted" is +// the origin's accept counter plus the gate's pre-dial placement; the +// conformance suite's zero-DNS-queries assertion needs its Target adapter for +// cortex (moca main has only conformance/), so upstream-accepts-0 is the +// in-tree equivalent. + +// allowHarness is a forward proxy with the egress allow list armed over +// entries, in front of an origin that counts what reaches it. +type allowHarness struct { + store *session.Store + proxy *httptest.Server + origin *httptest.Server + // allow is the harness's matcher, reusable by tests that build their own + // proxy server over the same entries (skip-hosts, redirect). + allow *egressallow.Matcher + // originAddr is the origin's host:port — the on-list spelling. + originAddr string + reached atomic.Int32 +} + +// newAllowHarness arms the allow list over the origin's own address, so +// requests naming the origin pass and anything else is off-list. +func newAllowHarness(t *testing.T, entries ...string) *allowHarness { + t.Helper() + h := &allowHarness{store: session.New(5*time.Minute, 100, 0)} + t.Cleanup(h.store.Close) + h.origin = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + h.reached.Add(1) + _, _ = io.WriteString(w, "origin") + })) + t.Cleanup(h.origin.Close) + h.originAddr = h.origin.Listener.Addr().String() + + if len(entries) == 0 { + entries = []string{h.originAddr} + } + allow, err := egressallow.New(entries) + if err != nil { + t.Fatalf("egressallow.New: %v", err) + } + h.allow = allow + srv := &Server{ + OutboundPipeline: outboundPipelineFromAuth(t, auth.New(auth.Config{})), + Sessions: h.store, + Client: http.DefaultClient, + EgressAllow: allow, + } + h.proxy = httptest.NewServer(srv.Handler()) + t.Cleanup(h.proxy.Close) + return h +} + +// assertMocaRefusal pins the conformance suite's wantRefusal shape on a +// response: status, X-Moca-Egress-Error, text/plain, and the exact body. +func assertMocaRefusal(t *testing.T, resp *http.Response, host string) { + t.Helper() + if resp.StatusCode != http.StatusForbidden { + t.Fatalf("status = %d, want 403", resp.StatusCode) + } + if got := resp.Header.Get("X-Moca-Egress-Error"); got != "destination_denied" { + t.Errorf("X-Moca-Egress-Error = %q, want destination_denied", got) + } + if ct := resp.Header.Get("Content-Type"); !strings.HasPrefix(ct, "text/plain") { + t.Errorf("Content-Type = %q, want text/plain*", ct) + } + body, _ := io.ReadAll(resp.Body) + if want := "destination_denied: " + host + "\n"; string(body) != want { + t.Errorf("body = %q, want %q", body, want) + } +} + +// deniedRow finds the session's denied-phase row and pins that it carries the +// egress-allowlist invocation — the audit event the conformance suite counts +// one of per refusal (#1482's JSON audit channel reads the same row). +func deniedRow(t *testing.T, h *allowHarness) *pipeline.SessionEvent { + t.Helper() + v := h.store.View(session.DefaultSessionID) + if v == nil { + t.Fatal("no session recorded for the refusal") + } + for i := range v.Events { + if v.Events[i].Phase == pipeline.SessionDenied { + return &v.Events[i] + } + } + t.Fatalf("no denied row; events = %+v", v.Events) + return nil +} + +// TestEgressAllow_RefusesOffListCONNECT is the moca off_allowlist_named_403 +// shape, CONNECT half: an off-list target is answered 403 by name, the origin +// is never contacted, and the denial leaves its audit row. +func TestEgressAllow_RefusesOffListCONNECT(t *testing.T) { + h := newAllowHarness(t) + line, br, conn := connectRaw(t, h.proxy, "off.test:9443") + defer conn.Close() + if !strings.Contains(line, "403") { + t.Fatalf("CONNECT off-list = %q, want 403", line) + } + body, _ := io.ReadAll(br) + if want := "destination_denied: off.test\n"; string(body) != want { + t.Errorf("body = %q, want %q", body, want) + } + if n := h.reached.Load(); n != 0 { + t.Errorf("origin accepted %d connection(s), want 0 — the refusal must precede the dial", n) + } + row := deniedRow(t, h) + // The row's Host is the authority, as every CONNECT row's is; the + // host-only name the conformance audit reads is in Details and the body. + if row.Host != "off.test:9443" { + t.Errorf("denied row host = %q, want off.test:9443", row.Host) + } + if row.Invocations == nil { + t.Fatal("denied row carries no invocations") + } + var inv *pipeline.Invocation + for i := range row.Invocations.Outbound { + if row.Invocations.Outbound[i].Plugin == "egress-allowlist" { + inv = &row.Invocations.Outbound[i] + } + } + if inv == nil { + t.Fatalf("no egress-allowlist invocation on the denied row: %+v", row.Invocations) + } + if inv.Action != pipeline.ActionDeny || inv.Reason != "destination_denied" || + inv.Details["host"] != "off.test" { + t.Errorf("invocation = %+v, want deny/destination_denied naming off.test", inv) + } +} + +// TestEgressAllow_RefusesOffListGET is the absolute-form half: a plain GET to +// an off-list host:port gets the same named 403 and the same audit row, and +// the origin is never contacted. +func TestEgressAllow_RefusesOffListGET(t *testing.T) { + h := newAllowHarness(t) + resp, err := proxyClient(h.proxy, nil).Get("http://off.test:9443/x") + if err != nil { + t.Fatalf("GET through the proxy: %v", err) + } + defer resp.Body.Close() + assertMocaRefusal(t, resp, "off.test") + if n := h.reached.Load(); n != 0 { + t.Errorf("origin reached %d time(s), want 0", n) + } + if row := deniedRow(t, h); row.Host != "off.test:9443" { + t.Errorf("denied row host = %q, want off.test:9443", row.Host) + } +} + +// TestEgressAllow_PortlessURLDefaultsTo80: a portless absolute-form URL is +// judged at its scheme's port (http → 80), so it is denied when only another +// port of the host is listed — deny-by-default, not guess-the-port. +func TestEgressAllow_PortlessURLDefaultsTo80(t *testing.T) { + h := newAllowHarness(t) // lists only originAddr + resp, err := proxyClient(h.proxy, nil).Get("http://off.test/refused?x=1") + if err != nil { + t.Fatalf("GET through the proxy: %v", err) + } + defer resp.Body.Close() + assertMocaRefusal(t, resp, "off.test") +} + +// TestEgressAllow_OnListFlowsThrough: the permissive half — a CONNECT to the +// listed origin tunnels (200), and a GET naming it is forwarded and answered. +func TestEgressAllow_OnListFlowsThrough(t *testing.T) { + h := newAllowHarness(t) + + line, _, conn := connectRaw(t, h.proxy, h.originAddr) + defer conn.Close() + if !strings.Contains(line, "200") { + t.Fatalf("CONNECT on-list = %q, want 200", line) + } + + resp, err := proxyClient(h.proxy, nil).Get("http://" + h.originAddr + "/x") + if err != nil { + t.Fatalf("GET on-list through the proxy: %v", err) + } + defer resp.Body.Close() + if resp.StatusCode != http.StatusOK { + t.Fatalf("status = %d, want 200", resp.StatusCode) + } + if n := h.reached.Load(); n != 1 { + t.Errorf("origin reached %d time(s), want 1 (the GET; the CONNECT tunnel carries no HTTP)", n) + } +} + +// TestEgressAllow_NilKeepsBehavior pins the compatibility contract: with no +// allow list, a CONNECT to a dead loopback port is the old dial-failure 502, +// not a 403 — off must mean byte-for-byte unchanged. +func TestEgressAllow_NilKeepsBehavior(t *testing.T) { + srv := &Server{ + OutboundPipeline: outboundPipelineFromAuth(t, auth.New(auth.Config{})), + Sessions: session.New(5*time.Minute, 100, 0), + Client: http.DefaultClient, + } + proxy := httptest.NewServer(srv.Handler()) + defer proxy.Close() + line, _, conn := connectRaw(t, proxy, "127.0.0.1:1") + defer conn.Close() + if !strings.Contains(line, "502") { + t.Fatalf("CONNECT with the allow list off = %q, want the dial-failure 502", line) + } + if strings.Contains(line, "403") { + t.Fatalf("CONNECT with the allow list off = %q, must not be a destination_denied 403", line) + } +} + +// TestEgressAllow_WinsOverSkipHosts: the allowlist is the outer boundary — a +// skip_hosts pattern matching the off-list host does not exempt it, because a +// bypass must not become an egress bypass. +func TestEgressAllow_WinsOverSkipHosts(t *testing.T) { + h := newAllowHarness(t) + skip, err := skiphost.New([]string{"off.test"}) + if err != nil { + t.Fatalf("skiphost.New: %v", err) + } + srv := &Server{ + OutboundPipeline: outboundPipelineFromAuth(t, auth.New(auth.Config{})), + Sessions: h.store, + Client: http.DefaultClient, + EgressAllow: h.allow, + SkipHosts: skip, + } + proxy := httptest.NewServer(srv.Handler()) + defer proxy.Close() + + line, _, conn := connectRaw(t, proxy, "off.test:9443") + defer conn.Close() + if !strings.Contains(line, "403") { + t.Fatalf("CONNECT off-list under a matching skip pattern = %q, want 403 — the allowlist is checked first", line) + } + if n := h.reached.Load(); n != 0 { + t.Errorf("origin accepted %d connection(s), want 0", n) + } +} + +// TestEgressAllow_RefusesOffListRedirect: a plugin's redirect can move the +// dial target after the top gate passed it; deny-by-default means the dial +// target is what counts, so an off-list redirect is refused before the +// exchange. The request row has landed; the denial row follows it. +func TestEgressAllow_RefusesOffListRedirect(t *testing.T) { + h := newAllowHarness(t) + away := &redirectPlugin{target: "http://off.test:9443"} + p, err := pipeline.New([]pipeline.Plugin{away}) + if err != nil { + t.Fatal(err) + } + srv := &Server{ + OutboundPipeline: pipeline.NewHolder(p), + Sessions: h.store, + Client: http.DefaultClient, + EgressAllow: h.allow, + } + proxy := httptest.NewServer(srv.Handler()) + defer proxy.Close() + + // The request names the on-list origin; the plugin steers it off-list. + resp, err := proxyClient(proxy, nil).Get("http://" + h.originAddr + "/x") + if err != nil { + t.Fatalf("GET through the proxy: %v", err) + } + defer resp.Body.Close() + assertMocaRefusal(t, resp, "off.test") + if n := h.reached.Load(); n != 0 { + t.Errorf("origin reached %d time(s), want 0 — the steered request must not dial", n) + } + // Both rows: the request row for the on-list host, then the denial. + v := h.store.View(session.DefaultSessionID) + if v == nil || len(v.Events) < 2 { + t.Fatalf("want a request row and a denial row, got %+v", v) + } +} + +// TestEgressAllow_RefusesStrayBracketDestination: the destination is +// client-chosen, so a parser on this boundary must not panic on a hostile +// spelling. A bare "]" authority used to index s[1:0] inside splitPort and +// panic — net/http recovered it, so the proxy stayed up and nothing was +// dialed, but any client could write a stack trace into the log on every +// request and the refusal row never landed. It is now simply not host:port, +// which deny-by-default refuses by name like any other unparseable +// destination — with the audit row that a panic skipped. +func TestEgressAllow_RefusesStrayBracketDestination(t *testing.T) { + // "[::1" (opened, never closed) is not in this list: net/http rejects + // that request line with a 400 before the handler runs, so the gate + // never sees it. Refused either way; only these reach the gate. + for _, target := range []string{"]:443", "]", "]]:443", "a]b:443"} { + t.Run(target, func(t *testing.T) { + h := newAllowHarness(t) + line, br, conn := connectRaw(t, h.proxy, target) + defer conn.Close() + if !strings.Contains(line, "403") { + t.Fatalf("CONNECT %q = %q, want 403", target, line) + } + body, _ := io.ReadAll(br) + if !strings.HasPrefix(string(body), "destination_denied: ") { + t.Errorf("body = %q, want a destination_denied refusal", body) + } + if n := h.reached.Load(); n != 0 { + t.Errorf("origin accepted %d connection(s), want 0", n) + } + // The row a panic used to cost: a recovered panic unwound + // before anything was recorded. + if row := deniedRow(t, h); row.Invocations == nil { + t.Error("denied row carries no invocations") + } + }) + } +} + +// TestEgressAllow_StrayBracketGETRefused is the absolute-form half of the +// same hostile input: url.ParseRequestURI accepts "http://]:80/", so it +// reaches the gate too. +func TestEgressAllow_StrayBracketGETRefused(t *testing.T) { + h := newAllowHarness(t) + resp, err := proxyClient(h.proxy, nil).Get("http://]:80/x") + if err != nil { + t.Fatalf("GET through the proxy: %v", err) + } + defer resp.Body.Close() + if resp.StatusCode != http.StatusForbidden { + t.Fatalf("status = %d, want 403", resp.StatusCode) + } + if n := h.reached.Load(); n != 0 { + t.Errorf("origin reached %d time(s), want 0", n) + } +} diff --git a/core/listener/forwardproxy/server.go b/core/listener/forwardproxy/server.go index 889e57e0d..9ee1f72af 100644 --- a/core/listener/forwardproxy/server.go +++ b/core/listener/forwardproxy/server.go @@ -24,6 +24,7 @@ import ( "errors" "github.com/rossoctl/cortex/core/attribution" + "github.com/rossoctl/cortex/core/listener/egressallow" "github.com/rossoctl/cortex/core/listener/egressguard" "github.com/rossoctl/cortex/core/listener/httpx" "github.com/rossoctl/cortex/core/listener/internal/bodycount" @@ -117,6 +118,19 @@ type Server struct { // motivation. SkipHosts *skiphost.Matcher + // EgressAllow, when non-nil, is the deny-by-default destination allow + // list (#1483, MOCA MI1 §8.5): an outbound destination not on the list + // is refused with 403 destination_denied naming the host, before the + // pipeline and before DNS resolution or any dial — the moca egress + // conformance contract's refusal shape (header X-Moca-Egress-Error, + // text/plain body "destination_denied: \n"). Applied to CONNECT + // targets, absolute-form http:// hosts, and the target a plugin's + // redirect moved a request to. Checked BEFORE SkipHosts: when the list + // is on it is the outer boundary, and a skip_hosts entry for an off-list + // host must not turn a bypass into an egress bypass. nil keeps the + // listener's behavior unchanged; see config.ListenerConfig.EgressAllowHosts. + EgressAllow *egressallow.Matcher + TLSBridge *tlsbridge.Engine // nil = disabled; set by caller after NewServer // SessionIDHeaders are the request headers consulted, in order, for a @@ -435,6 +449,31 @@ func (s *Server) serveOutbound(w http.ResponseWriter, r *http.Request, tl *tunne } } + // Egress allow list (#1483), the request-path half: a plain (non-bridged) + // absolute-form request is judged on its dial target, r.URL.Host, the same + // boundary the CONNECT gate holds — before SkipHosts (the allowlist is the + // outer boundary; a skip entry must not turn a bypass into an egress + // bypass), before the pipeline, and before the transport resolves or dials + // anything. A bridged request is not re-judged here: its destination was + // gated at its CONNECT, and the misdirected check above pins its Host to + // that same authority. Plain WriteRejection for the same reason as there: + // the named 403 is the contract, not something to re-frame for MCP. + if !isBridge && s.EgressAllow != nil && r.URL.Host != "" && !s.EgressAllow.Allows(r.URL.Host, r.URL.Scheme) { + host := hostOnly(r.URL.Host) + action := egressAllowRefusal(pctx, host) + chain := s.clientChain(r) + s.recordOutboundRejectIn(tl, pctx, action, s.recordingSessionID("", r.Header, chain)) + // Sanitized: host (the URL authority), Method and Path are all client-controlled, + // and a control rune forged into any of them would forge a log entry — the same + // CWE-150 shape #1539's egress guard was flagged for. + slog.Info("forward-proxy: destination refused by egress allow list", + "host", pipeline.SanitizeControlRunes(host), + "method", pipeline.SanitizeControlRunes(r.Method), + "path", pipeline.SanitizeControlRunes(r.URL.Path)) + httpx.WriteRejection(w, action) + return + } + // Every request that reaches here is re-originated from pctx below, plain or // TLS-bridged, so a plugin may redirect it. The CONNECT and transparent-connection // contexts are never marked: both dial the address the client chose, so a redirect @@ -614,6 +653,32 @@ func (s *Server) serveOutbound(w http.ResponseWriter, r *http.Request, tl *tunne // Clear RequestURI — set by the server but must be empty for client requests r.RequestURI = "" + // Egress allow list, redirect re-check (#1483): a plugin's redirect — + // inference-router moves requests between model servers — can change the + // dial target after the gate at the top of this function passed it, and + // deny-by-default means the DIAL TARGET is what counts, not the host the + // client named. An off-list redirect target is refused here, before the + // exchange: the request row has already landed above, and this denial row + // follows it. Unconditional rather than only-when-redirected, so the + // invariant "nothing dials off-list" holds by one check at the dial + // rather than by reasoning about every path that can reach it. + if s.EgressAllow != nil && !s.EgressAllow.Allows(r.URL.Host, r.URL.Scheme) { + host := hostOnly(r.URL.Host) + action := egressAllowRefusal(pctx, host) + s.recordOutboundRejectIn(tl, pctx, action, s.recordingSessionID(sessionID, r.Header, chain)) + // A delivered answer, not a failed exchange — the 426 rule: the deferred + // RunFinish reads zero StatusCode as OutcomeError, and the allowlist + // refusing an off-list redirect is the policy working, not a failure. + pctx.StatusCode = http.StatusForbidden + // Sanitized like the gate at the top of this function: both hosts are + // client-controlled (pctx.Host is the one the client named, pre-redirect). + slog.Info("forward-proxy: destination refused by egress allow list (redirect re-check)", + "host", pipeline.SanitizeControlRunes(host), + "requested", pipeline.SanitizeControlRunes(pctx.Host)) + httpx.WriteRejection(w, action) + return + } + client := s.Client if isBridge && s.TLSBridge != nil { client = s.TLSBridge.Upstream @@ -2181,6 +2246,27 @@ func (s *Server) recordOutboundRejectIn(tl *tunnelLog, pctx *pipeline.Context, a s.appendOutbound(tl, sid, sessionevent.Deny(pctx, action, pipeline.Outbound)) } +// egressAllowRefusal builds the destination_denied refusal for an off-list +// destination and records the deny invocation on pctx. The invocation is +// what makes the denied-phase session row land at all — recordOutboundRejectIn +// skips events with no invocations, and a listener gate is not a plugin, so +// the record is ours to make. Phase is stamped explicitly because the gate +// runs outside a pipeline pass, where the framework's currentPhase is empty +// and the row's snapshot filters by it. Details name the host the way the +// moca conformance suite's audit event reads it ({decision: +// destination_denied, host, ...}); #1482's JSON audit channel builds on the +// same row. +func egressAllowRefusal(pctx *pipeline.Context, host string) pipeline.Action { + pctx.Record(pipeline.Invocation{ + Plugin: "egress-allowlist", + Action: pipeline.ActionDeny, + Phase: pipeline.InvocationPhaseRequest, + Reason: "destination_denied", + Details: map[string]string{"host": host}, + }) + return pipeline.MocaRefusal(http.StatusForbidden, "destination_denied", host) +} + // connectDialTimeout bounds the upstream TCP dial for a CONNECT tunnel. // Once the tunnel is open the timeout no longer applies — the agent's TLS // handshake and subsequent traffic flow at their own pace. @@ -2228,6 +2314,32 @@ func (s *Server) handleConnect(w http.ResponseWriter, r *http.Request) { // attributed to the agent that opened it. pctx.ResolveClient() + // Egress allow list (#1483): when on, an off-list CONNECT target is refused + // here — before the SkipHosts match, so a skip entry cannot exempt an + // off-list host (the allowlist is the outer boundary), and before the + // pipeline and the dial, so the destination is neither resolved nor + // contacted: the refusal happens at a point Go's resolver has not run + // (it runs inside net.DialTimeout, which this return never reaches), which + // is the conformance contract's "no DNS, no connect" by construction. The + // named 403 is written with plain WriteRejection, never + // WriteRejectionForRequest: the egress boundary's answer is the contract, + // and the MCP JSON-RPC 200 rewrite would hide it behind a protocol frame. + if s.EgressAllow != nil && !s.EgressAllow.Allows(r.Host, "tcp") { + host := hostOnly(r.Host) + action := egressAllowRefusal(pctx, host) + // Session resolution the recordUnbufferedRequest way — pre-pipeline, + // from the client's headers and process only — so the denied row lands + // in the bucket the request would have recorded under. + chain := s.clientChain(r) + s.recordOutboundReject(pctx, action, s.recordingSessionID("", r.Header, chain)) + // Sanitized like the serveOutbound refusal: host is client-controlled and + // lands in a log file. + slog.Info("forward-proxy: CONNECT destination refused by egress allow list", + "host", pipeline.SanitizeControlRunes(host)) + httpx.WriteRejection(w, action) + return + } + // SkipHosts short-circuit: open the tunnel without running the // pipeline or recording a session event. The pipeline never ran, // so there's nothing to RunFinish — defer is suppressed. Mirrors diff --git a/core/pipeline/action.go b/core/pipeline/action.go index abe1e83cf..298a584b8 100644 --- a/core/pipeline/action.go +++ b/core/pipeline/action.go @@ -3,6 +3,7 @@ package pipeline import ( "encoding/json" "fmt" + "html" "net/http" "time" ) @@ -171,6 +172,46 @@ func DenyStatus(status int, code, reason string) Action { }} } +// MocaRefusal builds the MOCA egress contract's refusal shape: a Reject +// whose rendered response is the status, the header +// "X-Moca-Egress-Error: ", Content-Type text/plain, and the body +// ": \n" — exactly what the moca egress conformance suite's +// wantRefusal asserts for every refusal family (destination_denied, +// destination_private, plaintext_credential, conn_cap, +// upstream_unreachable). name is family-specific: destination_denied names +// the host, conn_cap the VM, upstream_unreachable the host. +// +// "X-Moca-Egress-Error" is that contract's wire identifier, used verbatim +// for the same reason x-authbridge-* is: another system's harness matches +// it byte for byte, and a "nicer" spelling here breaks the conformance run. +// +// Because the Violation carries a pre-rendered body, Violation.Render emits +// it unchanged and every listener's WriteRejection delivers the contract +// shape as-is. Do NOT route one of these through WriteRejectionForRequest +// on a path where the named refusal IS the answer: the MCP JSON-RPC 200 +// rewrite would hide it behind a protocol frame. +// +// name is HTML-escaped into the body. Every name the contract defines — a +// host, a VM id — is drawn from a character set EscapeString never touches, +// so the conformance bytes are identical for every legitimate input; the +// escape only bites on a hostile "host" carrying HTML metacharacters, which +// no valid destination can. The body is client-visible reflection, so it +// gets the same neutralisation any reflected value owes (CodeQL's reflected- +// XSS query, the high-severity alert this keeps closed), and the refusal's +// header stays the raw code, which is a constant of the contract. +func MocaRefusal(status int, code, name string) Action { + h := http.Header{} + h.Set("X-Moca-Egress-Error", code) + return Action{Type: Reject, Violation: &Violation{ + Status: status, + Code: code, + Reason: name, + Headers: h, + Body: []byte(code + ": " + html.EscapeString(name) + "\n"), + BodyType: "text/plain", + }} +} + // DenyWithDetails attaches plugin-arbitrary structured context. The // details map surfaces as a "details" object in the default JSON body // and is available to clients parsing the error programmatically. diff --git a/core/pipeline/action_test.go b/core/pipeline/action_test.go index 62ac991d0..46c2cde09 100644 --- a/core/pipeline/action_test.go +++ b/core/pipeline/action_test.go @@ -217,3 +217,47 @@ func TestRateLimited(t *testing.T) { t.Errorf("default code = %q", a.Violation.Code) } } + +// TestMocaRefusal pins the MOCA egress contract's refusal shape byte for +// byte: status, X-Moca-Egress-Error, text/plain, and the body +// ": \n" — what the moca egress conformance suite's wantRefusal +// asserts for every refusal family. A change to any of the four breaks the +// conformance run, so this test is the in-tree stand-in for it. +func TestMocaRefusal(t *testing.T) { + a := MocaRefusal(403, "destination_denied", "off.test") + status, h, body := a.Violation.Render() + if status != 403 { + t.Errorf("status = %d, want 403", status) + } + if got := h.Get("X-Moca-Egress-Error"); got != "destination_denied" { + t.Errorf("X-Moca-Egress-Error = %q, want destination_denied", got) + } + if got := h.Get("Content-Type"); got != "text/plain" { + t.Errorf("Content-Type = %q, want text/plain", got) + } + if want := "destination_denied: off.test\n"; string(body) != want { + t.Errorf("body = %q, want %q", body, want) + } + if a.Type != Reject { + t.Errorf("Type = %v, want Reject", a.Type) + } + // Render must not mutate the receiver's headers — the same action is + // rendered once for the client and possibly read again for a row. + if a.Violation.Headers.Get("Content-Type") != "" { + t.Error("Render leaked a Content-Type into the action's own headers") + } + + // The body is client-visible reflection of a client-named destination, so + // a hostile "host" carrying HTML metacharacters is escaped, not echoed: the + // refusal names the destination without becoming a rendering surface. For + // every legitimate host — the conformance suite's included — EscapeString + // is the identity, which is why the case above pins exact bytes. + hostile := MocaRefusal(403, "destination_denied", ``) + _, _, hbody := hostile.Violation.Render() + if want := "destination_denied: <script>x</script>\n"; string(hbody) != want { + t.Errorf("body = %q, want %q — HTML metacharacters must not reach the body raw", hbody, want) + } + if hostile.Violation.Reason != `` { + t.Error("Reason must stay raw — it never reaches the wire body and is read as data") + } +}