From 60019b88cb81429358923ccfec50a70845a90b0a Mon Sep 17 00:00:00 2001 From: Mariusz Sabath Date: Thu, 29 Oct 2026 10:10:10 -0400 Subject: [PATCH 1/6] feat: deny-by-default egress allow list for the forward proxy A MOCA sandbox may reach only the destinations its operator lists (#1483, MI1 8.5 item 3). listener.egress_allow_hosts holds exact host:port entries; when the list is on, an off-list CONNECT target, absolute-form URL host, or plugin-redirected dial target is refused before DNS resolution and before any dial, with the MOCA wire shape: 403, X-Moca-Egress-Error: destination_denied, text/plain body "destination_denied: \n". - core/listener/egressallow: the matcher. Entries are exact host:port (portless or wildcard entries are startup errors - a guessed port would widen the boundary); hosts compare lowercased, trailing-dot-stripped, IPv6 canonicalized. A portless destination is judged at its scheme's default (80 http, 443 otherwise), so http://host/x means host:80. Nil matcher = feature off, today's behavior unchanged. - pipeline.MocaRefusal: shared constructor for the named-refusal wire shape, so listeners and plugins (#1481's plaintext_credential gate) render one shape. - forwardproxy: the gate runs before skip_hosts (the allowlist is the outer boundary - a skip pattern cannot exempt an off-list host), records a denied-phase session row with an egress-allowlist invocation naming the host, and re-checks redirect targets before the dial so a redirecting plugin cannot steer bytes off-list. - cmd/cortex: config wins, the MOCA_EGRESS_ALLOW env var is the fallback channel the conformance harness drives. Auditing is the denied session row today; the moca JSON audit channel and vm_id are #1482/#1476-77. extproc and the transparent listener are follow-ups. Signed-off-by: Mariusz Sabath Assisted-By: Claude (Anthropic AI) --- CLAUDE.md | 22 ++ cmd/cortex/egressallow_test.go | 41 +++ cmd/cortex/main.go | 44 +++ core/config/config.go | 35 ++ core/listener/egressallow/egressallow.go | 173 +++++++++ core/listener/egressallow/egressallow_test.go | 139 ++++++++ .../forwardproxy/egressallow_gate_test.go | 328 ++++++++++++++++++ core/listener/forwardproxy/server.go | 108 ++++++ core/pipeline/action.go | 31 ++ core/pipeline/action_test.go | 30 ++ 10 files changed, 951 insertions(+) create mode 100644 cmd/cortex/egressallow_test.go create mode 100644 core/listener/egressallow/egressallow.go create mode 100644 core/listener/egressallow/egressallow_test.go create mode 100644 core/listener/forwardproxy/egressallow_gate_test.go diff --git a/CLAUDE.md b/CLAUDE.md index cab08c207..4bc9eb0eb 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -606,6 +606,19 @@ Every event on `/v1/sessions/{id}` and `/v1/events` carries: 426 sends it straight to HTTPS. Nothing is forwarded, the Finisher sees an allow with status 426, and `/v1/usage` does not count it as an error. On by default; `listener.decline_responses_websockets: false` forwards the upgrade instead. + **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. **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) @@ -1185,6 +1198,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; a destination naming no port is judged at its scheme's (80 http, 443 otherwise). 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. + - **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 30ec44450..908759dcc 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/forwardproxy" "github.com/rossoctl/cortex/core/listener/reverseproxy" "github.com/rossoctl/cortex/core/listener/skiphost" @@ -274,6 +275,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. // @@ -979,6 +1008,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 fc64f3e45..6654f2a10 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,37 @@ 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. + // + // 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..5b21d6d65 --- /dev/null +++ b/core/listener/egressallow/egressallow.go @@ -0,0 +1,173 @@ +// 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. +// +// 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) + } + 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. +func splitPort(s string) (host, port string, err error) { + // A right bracket means the host part is a bracketed IPv6 literal; + // everything after its closing colon is the port. + if i := strings.LastIndex(s, "]"); i >= 0 { + 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 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..ef44481d7 --- /dev/null +++ b/core/listener/egressallow/egressallow_test.go @@ -0,0 +1,139 @@ +package egressallow + +import "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_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..3922080f9 --- /dev/null +++ b/core/listener/forwardproxy/egressallow_gate_test.go @@ -0,0 +1,328 @@ +package forwardproxy + +import ( + "bufio" + "io" + "net" + "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 +} + +// connectRaw speaks one CONNECT to the proxy over raw TCP and returns the +// response's status line plus a reader positioned after the response headers. +func connectRaw(t *testing.T, proxy *httptest.Server, target string) (string, *bufio.Reader, net.Conn) { + t.Helper() + proxyAddr := strings.TrimPrefix(proxy.URL, "http://") + conn, err := net.DialTimeout("tcp", proxyAddr, 5*time.Second) + if err != nil { + t.Fatalf("dial proxy: %v", err) + } + if _, err := conn.Write([]byte("CONNECT " + target + " HTTP/1.1\r\nHost: " + target + "\r\n\r\n")); err != nil { + t.Fatalf("write CONNECT: %v", err) + } + br := bufio.NewReader(conn) + _ = conn.SetReadDeadline(time.Now().Add(5 * time.Second)) + line, err := br.ReadString('\n') + if err != nil { + t.Fatalf("read CONNECT response: %v", err) + } + for { + hdr, herr := br.ReadString('\n') + if herr != nil { + t.Fatalf("read CONNECT headers: %v", herr) + } + if hdr == "\r\n" || hdr == "\n" { + break + } + } + _ = conn.SetReadDeadline(time.Time{}) + return line, br, conn +} + +// 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) + } +} diff --git a/core/listener/forwardproxy/server.go b/core/listener/forwardproxy/server.go index 8438c98bf..4d23d7ff4 100644 --- a/core/listener/forwardproxy/server.go +++ b/core/listener/forwardproxy/server.go @@ -23,6 +23,7 @@ import ( "time" "errors" + "github.com/rossoctl/cortex/core/listener/egressallow" "github.com/rossoctl/cortex/core/listener/httpx" "github.com/rossoctl/cortex/core/listener/internal/bodycount" "github.com/rossoctl/cortex/core/listener/internal/bodyread" @@ -115,6 +116,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 @@ -360,6 +374,29 @@ 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) + sid := s.recordingSessionID("", r.Header, chain) + if chain != nil && s.Sessions != nil { + defer s.Sessions.TouchProcess(sid, chain) + } + s.recordOutboundRejectIn(tl, pctx, action, sid) + slog.Info("forward-proxy: destination refused by egress allow list", "host", host, "method", r.Method, "path", 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 @@ -539,6 +576,29 @@ 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 + slog.Info("forward-proxy: destination refused by egress allow list (redirect re-check)", + "host", host, "requested", pctx.Host) + httpx.WriteRejection(w, action) + return + } + client := s.Client if isBridge && s.TLSBridge != nil { client = s.TLSBridge.Upstream @@ -2067,6 +2127,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. @@ -2113,6 +2194,33 @@ 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) + sid := s.recordingSessionID("", r.Header, chain) + if chain != nil && s.Sessions != nil { + defer s.Sessions.TouchProcess(sid, chain) + } + s.recordOutboundReject(pctx, action, sid) + slog.Info("forward-proxy: CONNECT destination refused by egress allow list", "host", 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..19cab5a64 100644 --- a/core/pipeline/action.go +++ b/core/pipeline/action.go @@ -171,6 +171,37 @@ 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. +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 + ": " + 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..155c1a0f5 100644 --- a/core/pipeline/action_test.go +++ b/core/pipeline/action_test.go @@ -217,3 +217,33 @@ 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") + } +} From b5d4c955486c9e9ed105c8b5fbe723a6493a36a9 Mon Sep 17 00:00:00 2001 From: Mariusz Sabath Date: Thu, 29 Oct 2026 10:22:57 -0400 Subject: [PATCH 2/6] fix: sanitize client-controlled values in the egress allow list's log lines CodeQL on #1548 flagged "Log entries created from user input" at the serveOutbound refusal (host, method, path), the redirect re-check (host, requested host) and the CONNECT refusal (host): all are 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, fixed there in 2a3cc17a. - Port that commit's pipeline half verbatim - the same hunks, so the two PRs carry identical changes and whichever merges first wins cleanly: rename pipeline.sanitizeUA to pipeline.SanitizeControlRunes, the rule unchanged (C0/DEL/C1/bidi/zero-width and invalid UTF-8 replaced with U+FFFD). - Apply it at every log site this PR adds. The refusal's wire body and session details stay raw: the body is the MOCA contract's exact bytes, and the session API JSON-encodes what it serves. Signed-off-by: Mariusz Sabath Assisted-By: Claude (Anthropic AI) --- core/listener/forwardproxy/server.go | 18 +++++++++++++++--- core/pipeline/client.go | 10 ++++++++-- core/pipeline/context.go | 2 +- 3 files changed, 24 insertions(+), 6 deletions(-) diff --git a/core/listener/forwardproxy/server.go b/core/listener/forwardproxy/server.go index 4d23d7ff4..3f46d6343 100644 --- a/core/listener/forwardproxy/server.go +++ b/core/listener/forwardproxy/server.go @@ -392,7 +392,13 @@ func (s *Server) serveOutbound(w http.ResponseWriter, r *http.Request, tl *tunne defer s.Sessions.TouchProcess(sid, chain) } s.recordOutboundRejectIn(tl, pctx, action, sid) - slog.Info("forward-proxy: destination refused by egress allow list", "host", host, "method", r.Method, "path", r.URL.Path) + // 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 } @@ -593,8 +599,11 @@ func (s *Server) serveOutbound(w http.ResponseWriter, r *http.Request, tl *tunne // 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", host, "requested", pctx.Host) + "host", pipeline.SanitizeControlRunes(host), + "requested", pipeline.SanitizeControlRunes(pctx.Host)) httpx.WriteRejection(w, action) return } @@ -2216,7 +2225,10 @@ func (s *Server) handleConnect(w http.ResponseWriter, r *http.Request) { defer s.Sessions.TouchProcess(sid, chain) } s.recordOutboundReject(pctx, action, sid) - slog.Info("forward-proxy: CONNECT destination refused by egress allow list", "host", host) + // 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 } diff --git a/core/pipeline/client.go b/core/pipeline/client.go index 36065eeb6..1f2d5af08 100644 --- a/core/pipeline/client.go +++ b/core/pipeline/client.go @@ -136,7 +136,13 @@ const maxClientLen = 128 // one on a file that cannot be edited after the fact. Two copies of a five-line rule // is the right trade for a durable file; two DIFFERENT rules is not, so a change to // either belongs in both. -func sanitizeUA(s string) string { +// SanitizeControlRunes is sanitizeUA's rule under its honest name: it applies to any +// client-controlled string headed for a surface a terminal or a file renders, not only +// a User-Agent. Exported for the listeners — the forward proxy logs the destination +// Host on its egress-guard refusal paths, and a newline forged into that header would +// forge a log entry (#1539's CodeQL finding), which is the same CWE-150 shape the UA +// path guards against. The substitution, and every rationale below, is unchanged. +func SanitizeControlRunes(s string) string { if !hasControlRunes(s) { // The overwhelmingly common case, and it must not allocate: this runs on the // request path, twice per turn. @@ -465,7 +471,7 @@ func ParseUserAgent(ua string) *EventClient { // the same job as neutralising what a terminal would act on. The rule inside sanitizeUA is // shared rather than merely kept identical — every surface calls IsControlRune now — so this // side only has to leave it alone. Byte-length-neutral, so the cap arithmetic is untouched. - ua = capUA(sanitizeUA(strings.ReplaceAll(ua, "\t", " "))) + ua = capUA(SanitizeControlRunes(strings.ReplaceAll(ua, "\t", " "))) c := &EventClient{Raw: ua} // The product token is the first SPACE-delimited word, so the trailing comment Claude // Code appends — "(external, cli)" — is ignored rather than having to be matched. diff --git a/core/pipeline/context.go b/core/pipeline/context.go index a00315031..ee9a1072b 100644 --- a/core/pipeline/context.go +++ b/core/pipeline/context.go @@ -904,7 +904,7 @@ func (c *Context) RecordStrippedUpgrade(h http.Header) { if v == "" { return } - c.UpgradeStripped = capUA(sanitizeUA(strings.ReplaceAll(v, "\t", " "))) + c.UpgradeStripped = capUA(SanitizeControlRunes(strings.ReplaceAll(v, "\t", " "))) } // ResponseBodyMutated is the response-side analogue of BodyMutated, and From 6bd3bc61b5cb30832e6c017a7350166d089ce94c Mon Sep 17 00:00:00 2001 From: Mariusz Sabath Date: Thu, 29 Oct 2026 10:49:15 -0400 Subject: [PATCH 3/6] fix: HTML-escape the name in MocaRefusal's body (#1483) CodeQL on #1548 flagged one high-severity reflected-XSS alert at httpx.WriteRejection's body write: the refusal body reflects the client-named destination, and a "host" carrying HTML metacharacters would reach the response bytes raw. html.EscapeString the name. Every name the MOCA contract defines - a host, a VM id - is drawn from a character set EscapeString never touches, so the conformance bytes ("destination_denied: off.test\n") are identical for every legitimate input; the escape only bites on input no valid destination can carry. Reason stays raw: it never reaches the wire body on this path. TestMocaRefusal pins both. The three medium log-injection alerts on the refusal log lines remain open, matching #1539's precedent: the values pass through pipeline.SanitizeControlRunes (previous commit), which neutralises control runes at runtime but is not a barrier CodeQL can prove - a new medium alert does not fail the CodeQL check. Signed-off-by: Mariusz Sabath Assisted-By: Claude (Anthropic AI) --- core/pipeline/action.go | 12 +++++++++++- core/pipeline/action_test.go | 14 ++++++++++++++ 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/core/pipeline/action.go b/core/pipeline/action.go index 19cab5a64..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" ) @@ -189,6 +190,15 @@ func DenyStatus(status int, code, reason string) Action { // 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) @@ -197,7 +207,7 @@ func MocaRefusal(status int, code, name string) Action { Code: code, Reason: name, Headers: h, - Body: []byte(code + ": " + name + "\n"), + Body: []byte(code + ": " + html.EscapeString(name) + "\n"), BodyType: "text/plain", }} } diff --git a/core/pipeline/action_test.go b/core/pipeline/action_test.go index 155c1a0f5..46c2cde09 100644 --- a/core/pipeline/action_test.go +++ b/core/pipeline/action_test.go @@ -246,4 +246,18 @@ func TestMocaRefusal(t *testing.T) { 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") + } } From c4c96ae7242c7c673cc3cb909770d27c721bf93e Mon Sep 17 00:00:00 2001 From: Mariusz Sabath Date: Thu, 29 Oct 2026 11:27:42 -0400 Subject: [PATCH 4/6] fix: Deny a stray-bracket destination instead of panicking (#1483) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `splitPort` took its bracketed-IPv6 branch on a `]` found anywhere in the string, then indexed `s[1:i]` assuming a `[` at `s[0]`. The destination is client-chosen — a CONNECT authority or an absolute-form URL host — so `CONNECT ]:443` (or `GET http://]:80/`, which `url.ParseRequestURI` accepts) made `LastIndex` return 0 and `s[1:0]` 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 — the audit event a denial is supposed to leave was exactly what the panic skipped. A parser on a trust boundary does not get to panic. The branch is now taken only on a leading `[`; a stray `]` is simply not host:port and falls through to be denied like any other unparseable destination, with its audit row. An opened-but-unclosed `[::1` is an error rather than a different slice bound. Also: an unbracketed IPv6 entry (`2001:db8::1`, `::1`) passed `New` and split into host `2001:db8:` / port `1` — an entry no destination can ever match, because a request carries a v6 literal bracketed. It failed closed, but the operator believed they had allowed a host and had allowed nothing, and the contract is that entries which silently empty the boundary are startup errors. It is one now, naming the `[
]:` spelling that works. Bracketed entries are unaffected: their host keeps its colons legitimately. Tests: `TestEgressAllow_RefusesStrayBracketDestination` drives `]:443`, `]`, `]]:443` and `a]b:443` through the real `Handler()` and pins the 403, the untouched origin and the denied row — reverting only the source fix reproduces `http: panic serving … slice bounds out of range [1:0]`; `TestEgressAllow_StrayBracketGETRefused` covers the absolute-form half; `TestAllows_StrayBracketDeniedNotPanic` and `TestNew_RejectsUnbracketedIPv6Entry` pin the matcher directly, including that `[::1]:443` still parses. Reported by @huang195 in review of #1548. Assisted-By: Claude (Anthropic AI) Signed-off-by: Mariusz Sabath --- CLAUDE.md | 2 +- core/listener/egressallow/egressallow.go | 38 +++++++++- core/listener/egressallow/egressallow_test.go | 76 ++++++++++++++++++- .../forwardproxy/egressallow_gate_test.go | 54 +++++++++++++ 4 files changed, 166 insertions(+), 4 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 4bc9eb0eb..1738062fb 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1198,7 +1198,7 @@ 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; a destination naming no port is judged at its scheme's (80 http, 443 otherwise). 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: + - **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: diff --git a/core/listener/egressallow/egressallow.go b/core/listener/egressallow/egressallow.go index 5b21d6d65..40190da01 100644 --- a/core/listener/egressallow/egressallow.go +++ b/core/listener/egressallow/egressallow.go @@ -48,6 +48,10 @@ type Matcher struct { // - 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 @@ -87,6 +91,21 @@ func normalizeEntry(e string) (string, error) { 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 } @@ -152,10 +171,22 @@ func splitHostPort(s string) (host string, port int, err error) { // 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 right bracket means the host part is a bracketed IPv6 literal; + // A leading bracket means the host part is a bracketed IPv6 literal; // everything after its closing colon is the port. - if i := strings.LastIndex(s, "]"); i >= 0 { + 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 == "" { @@ -166,6 +197,9 @@ func splitPort(s string) (host, port string, err error) { } 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 } diff --git a/core/listener/egressallow/egressallow_test.go b/core/listener/egressallow/egressallow_test.go index ef44481d7..20885a8c4 100644 --- a/core/listener/egressallow/egressallow_test.go +++ b/core/listener/egressallow/egressallow_test.go @@ -1,6 +1,9 @@ package egressallow -import "testing" +import ( + "strings" + "testing" +) // TestNew_RejectsWideningEntries pins the construction guards: the entries that // would silently widen — or silently narrow to nothing — the boundary the list @@ -124,6 +127,77 @@ func TestAllows_NilMatcherAllowsEverything(t *testing.T) { } } +// 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) { diff --git a/core/listener/forwardproxy/egressallow_gate_test.go b/core/listener/forwardproxy/egressallow_gate_test.go index 3922080f9..0f5fe6468 100644 --- a/core/listener/forwardproxy/egressallow_gate_test.go +++ b/core/listener/forwardproxy/egressallow_gate_test.go @@ -326,3 +326,57 @@ func TestEgressAllow_RefusesOffListRedirect(t *testing.T) { 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) + } +} From 3f47224d6e62d897a5281b5b53482e91ca833b48 Mon Sep 17 00:00:00 2001 From: Mariusz Sabath Date: Thu, 29 Oct 2026 13:38:47 -0400 Subject: [PATCH 5/6] docs: Name the listener scope of the egress allow list (#1483) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The field comment and CLAUDE.md both called the allow list the "outer boundary" and "deny-by-default" without saying which listener it gates. It is the forward proxy's, and "outer" means outer with respect to `skip_hosts` — not with respect to every way a byte leaves the pod. Two paths it does not cover, both verified rather than assumed: - the transparent listener: HandleTransparentConn builds its own pctx and dials the recovered destination (transparent.go:143) without consulting EgressAllow. All three gates are on forward-proxy HTTP paths — two in serveOutbound, one in handleConnect. - extproc: no EgressAllow reference anywhere in the package. Only cmd/cortex builds a Matcher, and only onto the forward proxy. That matches the scope the PR claims (MOCA shape A is the proxy-sidecar forward proxy; #1483 names CONNECT plus absolute-form), so this is a documentation fix only — no enforcement change and no startup warning, per the review note. What was missing was telling an operator that a deployment which also redirects transparently, or runs envoy-sidecar, has to hold its egress somewhere else too. Raised by CodeRabbit in review of #1548. Assisted-By: Claude (Anthropic AI) Signed-off-by: Mariusz Sabath --- CLAUDE.md | 2 +- core/config/config.go | 17 +++++++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/CLAUDE.md b/CLAUDE.md index c06fc3b3e..8aeeff543 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1216,7 +1216,7 @@ resulting `/shared/client-id.txt` and `/shared/client-secret.txt`. - "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. 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. + 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. The transparent listener (`listener.transparent_proxy_addr`) 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. That matches the issue's scope (MOCA shape A is the proxy-sidecar forward proxy, CONNECT plus absolute-form), but a deployment that also redirects transparently, 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. diff --git a/core/config/config.go b/core/config/config.go index 2271d0cb0..cb7afd0f4 100644 --- a/core/config/config.go +++ b/core/config/config.go @@ -966,6 +966,23 @@ type ListenerConfig struct { // 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): + // HandleTransparentConn runs the shared pipeline but dials the + // recovered destination without consulting this list. + // - 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 redirects traffic transparently, 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"` From 80919316211253063df8e7a3c5d5fdac98b9d6d6 Mon Sep 17 00:00:00 2001 From: Mariusz Sabath Date: Thu, 29 Oct 2026 13:51:11 -0400 Subject: [PATCH 6/6] docs: Name enforce-redirect in the egress allow list's scope note (#1483) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Takes CodeRabbit's committable suggestion on top of 3f47224d, which had already named the two uncovered listeners but described the first one in the wrong vocabulary: "a deployment that also redirects traffic transparently" is the implementer's phrasing, not the operator's. The terms that appear in their own config and in the rest of this file are enforce-redirect and iptables REDIRECT, so both the field comment and the CLAUDE.md entry now say that egress which iptables REDIRECTs to listener.transparent_proxy_addr is not filtered by this list. Also records the contrast the suggestion sits next to, because the two notes are adjacent in this struct and read as contradictory otherwise: the transparent listener does not consult SkipHosts either, and transparent.go says so at length with a "don't add it" warning. There the omission HARDENS the boundary — a skip pattern must not be able to self-exempt from the hard egress guard — while here the same omission leaves a way around the allow list. Same listener, same absent check, opposite consequence. Verified rather than assumed, as with the parent commit: the transparent listener has no SkipHosts reference on its own path (only the comments explaining why), and no EgressAllow reference at all. Documentation only — no enforcement change, per the review note. gofmt, go vet and core/config tests clean. Assisted-By: Claude (Anthropic AI) Signed-off-by: Mariusz Sabath --- CLAUDE.md | 2 +- core/config/config.go | 15 ++++++++++----- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 1c48d0f6e..4b4e50340 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1216,7 +1216,7 @@ resulting `/shared/client-id.txt` and `/shared/client-secret.txt`. - "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. The transparent listener (`listener.transparent_proxy_addr`) 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. That matches the issue's scope (MOCA shape A is the proxy-sidecar forward proxy, CONNECT plus absolute-form), but a deployment that also redirects transparently, 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. + 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. diff --git a/core/config/config.go b/core/config/config.go index cb7afd0f4..1e5af8f9c 100644 --- a/core/config/config.go +++ b/core/config/config.go @@ -970,18 +970,23 @@ type ListenerConfig struct { // 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): + // - 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 this list. + // 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 redirects traffic transparently, or - // runs envoy-sidecar, needs its egress held somewhere else too — - // deny-by-default here is not deny-by-default for the pod. + // 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.