From c31c690ce686c71a463b301051d40e085685e9dc Mon Sep 17 00:00:00 2001 From: Mariusz Sabath Date: Wed, 28 Oct 2026 21:32:01 -0400 Subject: [PATCH 1/5] fix: refuse private destinations at the forward proxy dial (#1484) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Arms an opt-in SSRF dial guard on every outbound dial the forward proxy makes: CONNECT tunnels, plain and bridged requests, transparent redirects, and skip_hosts traffic too — a bypass must not become an SSRF bypass. Each destination is resolved once, refused with 403 destination_private when the address is loopback, link-local (including 169.254.169.254), RFC 1918 / unique-local space, or one of this host's own interface addresses and no permit covers it, and then dialed at the checked address, so DNS rebinding cannot re-answer between the check and the connect. Link-local, unspecified and multicast addresses are never permitted, whatever permit_cidrs says. Off by default: a sidecar's ordinary upstreams — Keycloak, in-cluster services, a laptop's local model — live in exactly the refused space, so defaulting this on would break every existing deployment. - core/listener/egressguard: the guard itself (CheckAddr / DialContext / DialTimeout, typed Refusal), resolver injectable so the tests pin the resolve-once property against a counting stub - forwardproxy: WithEgressGuard option (NewServer signature unchanged); under mTLS the guard sits inside the dialer so a refused address never gets a TLS ClientHello; handleConnect and the transparent listener check before the dial and reuse the checked address for the bridge fall-open re-dial; serveOutbound renders a Refusal as the listener's standard 403 rejection with error.kind destination_private - pipeline: SessionEvent.DialAddr on the wire (dialAddr, omitempty) — the audit trail pairing the name the client asked for with the address dialed; new tunnelReason "dest-private" (within agentop's 18-char PLUGIN column cap) - tlsbridge: WithDialContext so the bridge's re-origination client dials through the guard — the bridge must not become the path around it - config: listener.egress_guard {enabled, permit_cidrs}; a listener.* field, so it inherits the restart-on-reload rule The moca conformance shapes are pinned by tests: private_address_403 (CONNECT and GET to loopback, upstream never dialed), metadata_address_403_even_if_permitted (169.254.169.254 under 0.0.0.0/0), and the rebinding case's Cortex half — resolve exactly once, dial the first checked answer, open row carries dialAddr. Assisted-By: Claude (Anthropic AI) Signed-off-by: Mariusz Sabath --- CLAUDE.md | 8 +- cmd/cortex/main.go | 33 ++- core/config/config.go | 39 +++ core/config/config_test.go | 34 +++ core/listener/egressguard/guard.go | 208 ++++++++++++++ core/listener/egressguard/guard_test.go | 257 +++++++++++++++++ .../listener/forwardproxy/egressguard_test.go | 261 ++++++++++++++++++ core/listener/forwardproxy/server.go | 147 +++++++++- core/listener/forwardproxy/transparent.go | 42 ++- core/pipeline/context.go | 9 + core/pipeline/session.go | 36 +++ core/pipeline/session_test.go | 3 +- core/sessionapi/summary_test.go | 6 +- core/tlsbridge/upstream.go | 27 +- docs/framework-architecture.md | 40 +++ docs/session-api.md | 18 +- 16 files changed, 1151 insertions(+), 17 deletions(-) create mode 100644 core/listener/egressguard/guard.go create mode 100644 core/listener/egressguard/guard_test.go create mode 100644 core/listener/forwardproxy/egressguard_test.go diff --git a/CLAUDE.md b/CLAUDE.md index 3a41f1b71..fadb64781 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -606,6 +606,11 @@ 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 with `error.kind: "destination_private"` is the egress guard's + answer, not the upstream's** (`listener.egress_guard`, off by default): the + 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 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) @@ -619,7 +624,8 @@ Every event on `/v1/sessions/{id}` and `/v1/events` carries: - `requestedHost` — present only when a plugin redirected the request (`pctx.Redirect`): the host the client asked for. `host` is where the request actually went, and usage, the cost ledger and pricing all follow `host`. agentop's detail pane shows both on a `redirected:` line. - `inference.requestedModel` — present only when a plugin changed the model (`pctx.SetRequestModel`): the model the client asked for. `inference.model` is the model the request was sent for, which settlement prices; agentop's detail pane shows both on a `model:` line. - `bytesUp`, `bytesDown` — **body** bytes, one direction per row: a request row carries `bytesUp`, the body **as forwarded** (so a `tool-prune`-style rewrite is reflected, since the count is taken after the pipeline ran), and a response row carries `bytesDown`, the body **as it came back from the destination** — not as delivered, because extproc records the row before it emits a response mutator's replacement. Headers are not counted. A tunnel close is the one row carrying both. **Zero means the listener counted nothing, not that the body was empty**, and there is no third value for "unknown": every listener buffers bodies only when a plugin asks, so an unbuffered request reports zero, and so does a response relayed straight through — both proxies copy that one after the row is appended. agentop renders a zero side blank rather than as `0B`, and that is why its BYTES column can be trusted as a size when it shows one. All three listeners count the same thing for the same response, which took some doing: extproc sums the chunks Envoy hands it, while the proxies count off the upstream body at one choke point each, so every arm — buffered, block-by-block SSE, or chunk copy — reports the same figure. `core/listener/internal/bodycount` and `core/listener/parity`'s bytes fixtures hold that line. -- `tunnel`, `tunnelReason` — an opaque CONNECT (or transparent-redirect) tunnel records two rows sharing a `requestId`: the open (`phase: "request"`, `tunnelReason` saying why the bytes stayed opaque) and, when the tunnel ends, the close (`phase: "response"`). The close carries the CONNECT's own `statusCode` — 200, or 502 with the dial error in `error` when the destination could not be reached (`tunnelReason: "dial-failed"`) — plus `durationMs` for how long the tunnel stayed open and, in `bytesUp`/`bytesDown`, the bytes it carried each way (up = client to destination; opaque bytes have no body/header split, so these are everything that crossed). It is not the destination's status: that travels inside the client's end-to-end TLS. A bridged tunnel's open is recorded with its first decrypted request — in that request's session, directly before it, stamped with its time — and records no close, because the request carries its own response; a bridged tunnel that recorded no request gets its open and a close when it ends. Tunnel rows are kept out of `/v1/usage`: a tunnel's lifetime is not a request latency. +- `tunnel`, `tunnelReason` — an opaque CONNECT (or transparent-redirect) tunnel records two rows sharing a `requestId`: the open (`phase: "request"`, `tunnelReason` saying why the bytes stayed opaque) and, when the tunnel ends, the close (`phase: "response"`). The close carries the CONNECT's own `statusCode` — 200, or 502 with the dial error in `error` when the destination could not be reached (`tunnelReason: "dial-failed"`) — plus `durationMs` for how long the tunnel stayed open and, in `bytesUp`/`bytesDown`, the bytes it carried each way (up = client to destination; opaque bytes have no body/header split, so these are everything that crossed). It is not the destination's status: that travels inside the client's end-to-end TLS. A bridged tunnel's open is recorded with its first decrypted request — in that request's session, directly before it, stamped with its time — and records no close, because the request carries its own response; a bridged tunnel that recorded no request gets its open and a close when it ends. Tunnel rows are kept out of `/v1/usage`: a tunnel's lifetime is not a request latency. `tunnelReason: "dest-private"` with a 403 close and `error.kind: "destination_private"` is the egress guard refusing the destination before any dial — never confused with `dial-failed`, which attempted. +- `dialAddr` — the address the egress guard checked and the proxy dialed, `"ip:port"`, on a tunnel's open row when `listener.egress_guard` is on. With `host` it is the audit trail against DNS rebinding: under the guard the proxy resolves once, checks that answer, and dials exactly it, so the two fields together say "asked for the name, reached this address". Absent when the guard is off and on rows that dialed nothing. - `httpMethod`, `httpPath` — the HTTP verb and path, so a request no parser recognized is still identifiable rather than showing only a host. Distinct from the `method` inside `a2a` / `mcp`, which is a protocol method name. On an opaque tunnel `httpMethod` is `CONNECT` and `httpPath` is absent — opaque bytes carry no request line. The path is query-stripped and percent-decoded, so query-borne credentials never reach the timeline, but a secret in a path *segment* (a bot token, a webhook path) does survive on this unauthenticated surface — worth knowing before exporting events off-box. - `subagent` — present on rows from a Claude Code subagent or agent-team teammate: the `X-Claude-Code-Agent-Id` the request carried. For a subagent that is also the `` in `~/.claude/projects///subagents/agent-.meta.json`; for a teammate the join is unverified. Resolved by `Context.Subagent`, pinned with the User-Agent by `ResolveClient` so no plugin can re-file a row, and **refused rather than sanitised** when unusable (control runes, over 128 bytes), because it is a join key and a cleaned-up id joins nothing. It does not move the row: a subagent's requests stay in their parent's session, so the header must not be added to `session.id_headers`. **It names an agent, it does not classify the row:** `inference.agentRole` stays the field that says whether a row is a subagent's, and a teammate's row is `agentRole: main` with `subagent` set — Claude Code writes `cc_is_subagent=true` only for `agentType: subagent`. - `parentSubagent` — the agent that started `subagent`, from `X-Claude-Code-Parent-Agent-Id`, resolved with it in one memo (`Context.ParentSubagent`) and under the same rules. Claude Code sends it only when the spawner was itself an agent, so **absent on a row that names a `subagent` means the conversation started it**, not "unknown". The two keys are the tree's node and its edge. diff --git a/cmd/cortex/main.go b/cmd/cortex/main.go index 30ec44450..dd3804ee5 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/egressguard" "github.com/rossoctl/cortex/core/listener/forwardproxy" "github.com/rossoctl/cortex/core/listener/reverseproxy" "github.com/rossoctl/cortex/core/listener/skiphost" @@ -799,6 +800,26 @@ func main() { slog.Info("mTLS disabled (no mtls block in config)") } + // Egress guard (#1484): when listener.egress_guard arms it, every outbound + // dial the forward proxy makes — CONNECT tunnels, plain and bridged + // requests, transparent redirects, skip_hosts traffic included, because a + // bypass must not become an SSRF bypass — resolves once, refuses loopback, + // link-local, private and host-local destinations with 403 + // destination_private, and dials the checked address. Built before the TLS + // bridge below so the re-origination client's dialer is guarded too: the + // bridge must not become the path around the guard. Off unless configured + // (a sidecar's ordinary upstreams live in exactly the refused space). + var egress *egressguard.Guard + if cfg.Listener.EgressGuard != nil && cfg.Listener.EgressGuard.Enabled { + g, gerr := egressguard.New(cfg.Listener.EgressGuard.PermitCIDRs) + if gerr != nil { + fatalf("listener.egress_guard: %v", gerr) + } + egress = g + slog.Info("egress guard on: refusing loopback, link-local, private and host-local destinations", + "permit_cidrs", cfg.Listener.EgressGuard.PermitCIDRs) + } + // TLS bridge: when enabled, the forward proxy terminates agent outbound // TLS so the outbound pipeline sees decrypted HTTPS. Constructed // here and set on fpSrv below (mirroring fpSrv.SkipHosts / fpSrv.Shared). @@ -886,7 +907,15 @@ func main() { fatalf("tls-bridge upstream_ca_bundle read failed: %v", err) } } - up, uerr := tlsbridge.NewUpstreamClient(extra, cfg.TLSBridge.UpstreamInsecure) + // The bridge's re-origination client dials through the egress guard + // when one is armed, so a bridged request is checked exactly like a + // plaintext one. No option when the guard is off: nil DialContext is + // the transport's default dialer. + var upOpts []tlsbridge.UpstreamOption + if egress != nil { + upOpts = append(upOpts, tlsbridge.WithDialContext(egress.DialContext)) + } + up, uerr := tlsbridge.NewUpstreamClient(extra, cfg.TLSBridge.UpstreamInsecure, upOpts...) if uerr != nil { fatalf("tls-bridge upstream client failed: %v", uerr) } @@ -966,7 +995,7 @@ func main() { // declared here so shutdown can close it whether or not the forward role ran. var transparentLn *net.TCPListener if roles[config.RoleForward] { - fpSrv, ferr := forwardproxy.NewServer(outboundH, sessions, fpMTLS) + fpSrv, ferr := forwardproxy.NewServer(outboundH, sessions, fpMTLS, forwardproxy.WithEgressGuard(egress)) if ferr != nil { fatalf("creating forward proxy: %v", ferr) } diff --git a/core/config/config.go b/core/config/config.go index fc64f3e45..2c1c965df 100644 --- a/core/config/config.go +++ b/core/config/config.go @@ -944,6 +944,45 @@ type ListenerConfig struct { // A pointer for the reason SessionConfig.Enabled is one: with a plain bool, // "unset" and "false" are the same value, and the default could not be on. DeclineResponsesWebSockets *bool `yaml:"decline_responses_websockets,omitempty" json:"decline_responses_websockets,omitempty"` + + // EgressGuard arms the SSRF dial guard on the forward proxy's every + // outbound dial (#1484): each destination is resolved once, refused with + // 403 destination_private when it lands in loopback, link-local (cloud + // metadata, 169.254.169.254), RFC 1918 / unique-local space, or one of + // this host's own addresses — and dialed at the checked address, so DNS + // rebinding cannot re-answer between check and connect. CONNECT tunnels, + // plain and bridged requests, transparent redirects and skip_hosts traffic + // are all covered: a bypass must not become an SSRF bypass. + // + // OFF BY DEFAULT, deliberately: a sidecar's ordinary upstreams — Keycloak, + // in-cluster services, a laptop's local model — live in exactly the + // refused space, so defaulting this on would break every existing + // deployment. An operator who wants the hard boundary (or runs the MOCA + // shape, where the sandbox's egress is meant to be public-only) enables it + // and lists the private prefixes still allowed in permit_cidrs. + // + // permit_cidrs covers prefixes within the refused space only; link-local, + // unspecified and multicast addresses are NEVER permitted, whatever the + // list says — the metadata range is what this guard exists among other + // things to protect. + // + // A pointer so "absent" (off) stays distinguishable from an explicit + // enabled: false, mirroring DeclineResponsesWebSockets. Listener.* field: + // changing it requires a restart, like every other listener knob. + EgressGuard *EgressGuardConfig `yaml:"egress_guard,omitempty" json:"egress_guard,omitempty"` +} + +// EgressGuardConfig configures the forward proxy's SSRF dial guard. +type EgressGuardConfig struct { + // Enabled arms the guard. False (or an absent egress_guard block) keeps + // the dial behavior unchanged. + Enabled bool `yaml:"enabled" json:"enabled"` + + // PermitCIDRs lists CIDR prefixes within the refused space that remain + // dialable — e.g. ["127.0.0.1/32"] for a laptop proxy whose upstream is a + // local model, or ["10.0.0.0/8"] for an in-cluster deployment. Never has + // any effect on link-local, unspecified or multicast addresses. + PermitCIDRs []string `yaml:"permit_cidrs,omitempty" json:"permit_cidrs,omitempty"` } // DeclinesResponsesWebSockets reports whether the forward proxy answers a WebSocket diff --git a/core/config/config_test.go b/core/config/config_test.go index cdfe70113..e44b18fb8 100644 --- a/core/config/config_test.go +++ b/core/config/config_test.go @@ -1070,6 +1070,40 @@ func TestListenerConfig_DeclineResponsesWebSockets(t *testing.T) { } } +// TestListenerConfig_EgressGuard pins the default (off) and the block that arms +// it. Unset must mean off with or without a listener block: a sidecar's ordinary +// upstreams live in exactly the refused space, so a config predating the key +// turning the guard on would break the deployment it was written for. +func TestListenerConfig_EgressGuard(t *testing.T) { + var noBlock Config + if err := yaml.Unmarshal([]byte("mode: proxy-sidecar\n"), &noBlock); err != nil { + t.Fatal(err) + } + if noBlock.Listener.EgressGuard != nil { + t.Error("no listener block: EgressGuard is non-nil, want off") + } + + var armed Config + if err := yaml.Unmarshal([]byte("listener:\n egress_guard:\n enabled: true\n permit_cidrs: [\"127.0.0.1/32\", \"10.0.0.0/8\"]\n"), &armed); err != nil { + t.Fatal(err) + } + g := armed.Listener.EgressGuard + if g == nil || !g.Enabled { + t.Fatalf("armed config: EgressGuard = %+v, want enabled", g) + } + if len(g.PermitCIDRs) != 2 || g.PermitCIDRs[0] != "127.0.0.1/32" || g.PermitCIDRs[1] != "10.0.0.0/8" { + t.Errorf("PermitCIDRs = %v, want the two configured prefixes", g.PermitCIDRs) + } + + var presentButOff Config + if err := yaml.Unmarshal([]byte("listener:\n egress_guard:\n enabled: false\n"), &presentButOff); err != nil { + t.Fatal(err) + } + if g := presentButOff.Listener.EgressGuard; g == nil || g.Enabled { + t.Errorf("explicit false: EgressGuard = %+v, want present and disabled", g) + } +} + // TestLoad_PreAffinityLaptopConfigGetsClientAffinity loads a laptop config written before // session.client_affinity existed — no session block at all, which is what every install // made before it still has on disk. With affinity off, Bob Shell's header-less startup diff --git a/core/listener/egressguard/guard.go b/core/listener/egressguard/guard.go new file mode 100644 index 000000000..e3b4a3483 --- /dev/null +++ b/core/listener/egressguard/guard.go @@ -0,0 +1,208 @@ +// Package egressguard implements the forward proxy's SSRF dial guard +// (#1484, MOCA MI1 §8.5): resolve a destination once, refuse addresses a +// sandbox must never reach through the proxy, and dial the address that was +// checked — never a second resolution's answer. +// +// The refused set is loopback, link-local (including the cloud metadata +// address 169.254.169.254), RFC 1918 / unique-local space, and the host's own +// interface addresses. An operator may permit prefixes within that space +// (a laptop proxy whose upstream is a local model, an in-cluster deployment) +// — but link-local, unspecified and multicast addresses are never permitted: +// 0.0.0.0 reaches loopback on every mainstream stack and 169.254.169.254 is +// the metadata service the guard exists among other things to protect. +package egressguard + +import ( + "context" + "fmt" + "net" + "net/netip" + "strconv" + "strings" + "time" +) + +// Guard checks and dials outbound destinations. Construct with New; the zero +// value refuses nothing and dials nothing. Safe for concurrent use. +type Guard struct { + // permit holds the operator-configured prefixes within the refused space. + permit []netip.Prefix + + // resolve resolves a host name to addresses, once per call. A field so + // tests can pin "resolved exactly once" against a counting fake; the + // production value is net.DefaultResolver.LookupNetIP. + resolve func(ctx context.Context, host string) ([]netip.Addr, error) +} + +// New builds a Guard that refuses the destinations described in the package +// comment, except where a permit prefix covers them. Permit CIDRs are +// accepted even when they cover never-permitted space (an operator copying a +// broad list is not a boot failure); the never-permitted ranges simply win. +func New(permitCIDRs []string) (*Guard, error) { + permit := make([]netip.Prefix, 0, len(permitCIDRs)) + for _, cidr := range permitCIDRs { + prefix, err := netip.ParsePrefix(strings.TrimSpace(cidr)) + if err != nil { + return nil, fmt.Errorf("egressguard: permit cidr %q: %w", cidr, err) + } + permit = append(permit, prefix.Masked()) + } + return &Guard{ + permit: permit, + resolve: func(ctx context.Context, host string) ([]netip.Addr, error) { + return net.DefaultResolver.LookupNetIP(ctx, "ip", host) + }, + }, nil +} + +// Refusal is the typed error a refused destination produces. Host is the +// destination as the request named it (name or literal); Addr is the refused +// address the check saw. Callers render it as 403 destination_private — the +// MOCA egress contract's identifier, used verbatim so the conformance suite +// Cortex shares with moca-egress matches it without a translation table. +type Refusal struct { + Host string + Addr netip.Addr +} + +func (r *Refusal) Error() string { + return fmt.Sprintf("refusing to reach private destination %s (%s)", r.Host, r.Addr) +} + +// CheckAddr resolves network+addr ONCE, checks the address it will dial, and +// reports that address: the literal when addr named one, else the FIRST +// resolved answer. The caller must dial the returned address, not the +// original host. +// +// The rebinding window this closes is between the CHECK and the DIAL — check +// one address, dial another — and the fix is to commit to one answer and dial +// it verbatim, never re-resolving. Later records in the same response are +// deliberately not consulted: the moca conformance case pins the shape, a +// name answering 127.0.0.1 then 10.0.0.9 with only 127.0.0.1/32 permitted +// must tunnel to 127.0.0.1, the first checked answer. What can never happen +// is the divergence the attack needs: the checked address and the dialed one +// are the same value by construction, and a second lookup has no window to +// change either. +func (g *Guard) CheckAddr(ctx context.Context, network, addr string) (string, error) { + host, port, err := net.SplitHostPort(addr) + if err != nil { + return "", err + } + if strings.HasPrefix(host, "[") && strings.HasSuffix(host, "]") { + host = host[1 : len(host)-1] + } + + var dial []netip.Addr + if ip, err := netip.ParseAddr(host); err == nil { + dial = []netip.Addr{ip.Unmap()} + } else { + if g.resolve == nil { + return "", fmt.Errorf("egressguard: no resolver configured") + } + addrs, rerr := g.resolve(ctx, host) + if rerr != nil { + return "", rerr + } + if len(addrs) == 0 { + return "", &net.DNSError{Err: "no such host", Name: host, IsNotFound: true} + } + dial = addrs + } + + // Check the first answer — the one the caller will dial. See CheckAddr's + // doc for why the later records are not consulted. + dialAddr := dial[0].Unmap() + if ref := g.check(host, dialAddr); ref != nil { + return "", ref + } + p, err := strconv.ParseUint(port, 10, 16) + if err != nil { + return "", err + } + return netip.AddrPortFrom(dialAddr, uint16(p)).String(), nil +} + +// DialContext is CheckAddr followed by a dial of the checked address — +// dialer-shaped, for http.Transport. Only the dial address is replaced; the +// transport's TLS ServerName and certificate verification still follow the +// hostname the request named. +func (g *Guard) DialContext(ctx context.Context, network, addr string) (net.Conn, error) { + checked, err := g.CheckAddr(ctx, network, addr) + if err != nil { + return nil, err + } + var d net.Dialer + return d.DialContext(ctx, network, checked) +} + +// DialTimeout is DialContext with a fixed timeout, for the CONNECT-tunnel +// dial sites that predate contexts. +func (g *Guard) DialTimeout(ctx context.Context, network, addr string, timeout time.Duration) (net.Conn, error) { + checked, err := g.CheckAddr(ctx, network, addr) + if err != nil { + return nil, err + } + return net.DialTimeout(network, checked, timeout) +} + +// check applies the refusal rules to one address. It returns the Refusal to +// answer the request with, or nil when the address may be dialed. +func (g *Guard) check(host string, ip netip.Addr) *Refusal { + if ref := g.neverPermitted(host, ip); ref != nil { + return ref + } + if !g.refusable(ip) { + return nil + } + // Refused space, permit-able: an operator prefix covering the address + // makes it dialable (the laptop local-model and in-cluster cases). + for _, p := range g.permit { + if p.Contains(ip) { + return nil + } + } + return &Refusal{Host: host, Addr: ip} +} + +// neverPermitted reports the ranges no permit can open, as a Refusal when +// the address is in one. Link-local unicast carries the cloud metadata +// service (169.254.169.254 on every major cloud); unspecified reaches +// loopback on mainstream stacks; multicast is not a unicast destination. +func (g *Guard) neverPermitted(host string, ip netip.Addr) *Refusal { + switch { + case ip.IsLinkLocalUnicast(): + return &Refusal{Host: host, Addr: ip} + case ip.IsUnspecified(): + return &Refusal{Host: host, Addr: ip} + case ip.IsMulticast(): + return &Refusal{Host: host, Addr: ip} + } + return nil +} + +// refusable reports whether ip is in the refused-but-permit-able space: +// loopback, RFC 1918 / unique-local, or one of this host's own interface +// addresses. The last set is enumerated per check — an interface address +// added after boot is guarded without a restart, and InterfaceAddrs costs +// microseconds against a dial that costs milliseconds. +func (g *Guard) refusable(ip netip.Addr) bool { + if ip.IsLoopback() || ip.IsPrivate() { + return true + } + ifaces, err := net.InterfaceAddrs() + if err != nil { + // Fail closed: unable to enumerate the host's addresses, treat the + // candidate as one of them. + return true + } + for _, ia := range ifaces { + prefix, err := netip.ParsePrefix(ia.String()) + if err != nil { + continue + } + if prefix.Contains(ip) { + return true + } + } + return false +} diff --git a/core/listener/egressguard/guard_test.go b/core/listener/egressguard/guard_test.go new file mode 100644 index 000000000..2c7b60987 --- /dev/null +++ b/core/listener/egressguard/guard_test.go @@ -0,0 +1,257 @@ +package egressguard + +import ( + "context" + "errors" + "fmt" + "net" + "net/netip" + "testing" +) + +func mustAddr(t *testing.T, s string) netip.Addr { + t.Helper() + a, err := netip.ParseAddr(s) + if err != nil { + t.Fatalf("ParseAddr(%q): %v", s, err) + } + return a +} + +// stubResolver answers each host from a scripted table and counts lookups, so +// the resolve-once property is asserted rather than assumed. +type stubResolver struct { + answers map[string][]netip.Addr + lookups int +} + +func (s *stubResolver) resolve(_ context.Context, host string) ([]netip.Addr, error) { + s.lookups++ + if a, ok := s.answers[host]; ok { + return a, nil + } + return nil, &net.DNSError{Err: "no such host", Name: host, IsNotFound: true} +} + +func newStubGuard(t *testing.T, permit []string, answers map[string][]netip.Addr) *Guard { + t.Helper() + g, err := New(permit) + if err != nil { + t.Fatalf("New: %v", err) + } + g.resolve = (&stubResolver{answers: answers}).resolve + return g +} + +// TestCheckAddr_RefusedRanges pins the refusal set: loopback, link-local +// (metadata), RFC 1918, unique-local, unspecified and multicast are all +// refused with no permit configured. A public address passes. +func TestCheckAddr_RefusedRanges(t *testing.T) { + g, err := New(nil) + if err != nil { + t.Fatalf("New: %v", err) + } + for _, c := range []struct { + addr string + refIP string + }{ + {"127.0.0.1:443", "127.0.0.1"}, + {"[::1]:443", "::1"}, + {"169.254.169.254:80", "169.254.169.254"}, // cloud metadata + {"[fe80::1]:443", "fe80::1"}, + {"10.0.0.9:8080", "10.0.0.9"}, + {"172.16.5.4:80", "172.16.5.4"}, + {"192.168.1.10:80", "192.168.1.10"}, + {"[fc00::9]:443", "fc00::9"}, // unique-local + {"0.0.0.0:80", "0.0.0.0"}, // unspecified reaches loopback + {"224.0.0.1:80", "224.0.0.1"}, + } { + _, err := g.CheckAddr(context.Background(), "tcp", c.addr) + var ref *Refusal + if !errors.As(err, &ref) { + t.Errorf("CheckAddr(%q) err = %v, want a Refusal", c.addr, err) + continue + } + if ref.Addr != mustAddr(t, c.refIP) { + t.Errorf("CheckAddr(%q) refused addr = %s, want %s", c.addr, ref.Addr, c.refIP) + } + } + + // A public literal passes and is its own dial target. + checked, err := g.CheckAddr(context.Background(), "tcp", "203.0.113.7:443") + if err != nil { + t.Fatalf("public literal: %v", err) + } + if checked != "203.0.113.7:443" { + t.Errorf("public literal dial addr = %q, want it verbatim", checked) + } +} + +// TestCheckAddr_NeverPermitted pins the ranges no permit can open: the moca +// conformance case metadata_address_403_even_if_permitted permits +// 169.254.0.0/16 and 0.0.0.0/0 and the metadata address is still refused. +func TestCheckAddr_NeverPermitted(t *testing.T) { + g, err := New([]string{"169.254.0.0/16", "0.0.0.0/0", "::/0"}) + if err != nil { + t.Fatalf("New: %v", err) + } + for _, addr := range []string{ + "169.254.169.254:80", + "169.254.0.9:80", + "[fe80::9]:443", + "0.0.0.0:80", + "[ff02::1]:443", + } { + if _, err := g.CheckAddr(context.Background(), "tcp", addr); err == nil { + t.Errorf("CheckAddr(%q) with blanket permits = nil, want a Refusal", addr) + } + } +} + +// TestCheckAddr_PermitOpensPrivateSpace pins the permit-able half: loopback +// and RFC 1918 become dialable when a configured prefix covers them, which is +// the moca dns_rebinding case's EnvPermit (127.0.0.1/32) and the in-cluster / +// laptop-local-model deployments. +func TestCheckAddr_PermitOpensPrivateSpace(t *testing.T) { + g, err := New([]string{"127.0.0.1/32", "10.0.0.0/8"}) + if err != nil { + t.Fatalf("New: %v", err) + } + for _, addr := range []string{"127.0.0.1:8080", "10.1.2.3:443"} { + if _, err := g.CheckAddr(context.Background(), "tcp", addr); err != nil { + t.Errorf("CheckAddr(%q) with covering permit: %v", addr, err) + } + } + // Uncovered private space stays refused: the permit is a prefix, not a toggle. + if _, err := g.CheckAddr(context.Background(), "tcp", "192.168.1.4:80"); err == nil { + t.Error("CheckAddr(192.168.1.4) outside the permits = nil, want a Refusal") + } +} + +// TestCheckAddr_ResolvesOnceAndDialsFirstChecked is the DNS-rebinding case: +// the resolver is consulted exactly once, a mixed public/private answer is +// refused outright (fail closed), and a clean answer dials the FIRST address. +func TestCheckAddr_ResolvesOnceAndDialsFirstChecked(t *testing.T) { + g, err := New([]string{"127.0.0.1/32"}) + if err != nil { + t.Fatalf("New: %v", err) + } + stub := &stubResolver{answers: map[string][]netip.Addr{ + "rebind.test": {mustAddr(t, "127.0.0.1"), mustAddr(t, "10.0.0.9")}, + }} + g.resolve = stub.resolve + + checked, err := g.CheckAddr(context.Background(), "tcp", "rebind.test:9443") + if err != nil { + t.Fatalf("rebind: %v", err) + } + if checked != "127.0.0.1:9443" { + t.Errorf("dial addr = %q, want the first checked answer 127.0.0.1:9443", checked) + } + if stub.lookups != 1 { + t.Errorf("resolver ran %d times, want exactly 1", stub.lookups) + } +} + +// TestCheckAddr_FirstAnswerRules: the FIRST record decides. When it is a +// refused address the name is refused even though later records are public — +// the reverse of the conformance case, and the shape an attacker controls by +// record order. +func TestCheckAddr_FirstAnswerRules(t *testing.T) { + g, err := New(nil) + if err != nil { + t.Fatalf("New: %v", err) + } + g.resolve = (&stubResolver{answers: map[string][]netip.Addr{ + "mixed.test": {mustAddr(t, "127.0.0.1"), mustAddr(t, "203.0.113.7")}, + }}).resolve + if _, err := g.CheckAddr(context.Background(), "tcp", "mixed.test:443"); err == nil { + t.Error("first answer refused, second public = nil, want a Refusal") + } +} + +// TestCheckAddr_MappedIPv6: a v4-mapped v6 literal is checked as its v4 +// half, so ::ffff:127.0.0.1 cannot slip past the loopback rule. +func TestCheckAddr_MappedIPv6(t *testing.T) { + g, err := New(nil) + if err != nil { + t.Fatalf("New: %v", err) + } + if _, err := g.CheckAddr(context.Background(), "tcp", "[::ffff:127.0.0.1]:443"); err == nil { + t.Error("v4-mapped loopback = nil, want a Refusal") + } +} + +// TestRefusalErrorsAs: the typed error must be reachable through the wrapping +// net/http puts around dial errors (*url.Error), which is how serveOutbound +// detects it. +func TestRefusalErrorsAs(t *testing.T) { + g, err := New(nil) + if err != nil { + t.Fatalf("New: %v", err) + } + _, cerr := g.CheckAddr(context.Background(), "tcp", "127.0.0.1:443") + var ref *Refusal + if !errors.As(cerr, &ref) { + t.Fatalf("errors.As on the raw error failed: %v", cerr) + } + wrapped := fmt.Errorf(`Get "http://127.0.0.1:443/x": %w`, cerr) + if !errors.As(wrapped, &ref) { + t.Fatalf("errors.As through a wrapping error failed: %v", wrapped) + } + if ref.Host != "127.0.0.1" || ref.Addr != mustAddr(t, "127.0.0.1") { + t.Errorf("Refusal = %s (%s), want host and addr from the check", ref.Host, ref.Addr) + } +} + +// TestNew_RejectsBadPermit: a malformed permit CIDR is a construction error, +// not a silently ignored entry. +func TestNew_RejectsBadPermit(t *testing.T) { + if _, err := New([]string{"127.0.0.1/99"}); err == nil { + t.Error("New with an invalid mask = nil, want an error") + } + if _, err := New([]string{"not-a-cidr"}); err == nil { + t.Error("New with a non-CIDR = nil, want an error") + } +} + +// TestDialContext_DialsCheckedAddress: the dialer-shaped entry point dials +// the address CheckAddr returned. A loopback listener stands in for the +// destination, permitted so the guard lets the dial through — what is pinned +// is that the connection lands on the stub resolver's FIRST answer, not on +// whatever a second resolution might have said. +func TestDialContext_DialsCheckedAddress(t *testing.T) { + ln, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Skipf("loopback listen unavailable: %v", err) + } + defer ln.Close() + go func() { + for { + c, err := ln.Accept() + if err != nil { + return + } + _ = c.Close() + } + }() + + g, err := New([]string{"127.0.0.1/32"}) + if err != nil { + t.Fatalf("New: %v", err) + } + port := ln.Addr().(*net.TCPAddr).Port + first := netip.MustParseAddr("127.0.0.1") + g.resolve = (&stubResolver{answers: map[string][]netip.Addr{ + "dst.test": {first, netip.MustParseAddr("127.0.0.2")}, + }}).resolve + + conn, derr := g.DialContext(context.Background(), "tcp", fmt.Sprintf("dst.test:%d", port)) + if derr != nil { + t.Fatalf("DialContext: %v", derr) + } + defer conn.Close() + if got := conn.RemoteAddr().String(); got != fmt.Sprintf("127.0.0.1:%d", port) { + t.Errorf("dialed %s, want the first checked address 127.0.0.1:%d", got, port) + } +} diff --git a/core/listener/forwardproxy/egressguard_test.go b/core/listener/forwardproxy/egressguard_test.go new file mode 100644 index 000000000..a9d0eda9d --- /dev/null +++ b/core/listener/forwardproxy/egressguard_test.go @@ -0,0 +1,261 @@ +package forwardproxy + +import ( + "bufio" + "io" + "net" + "net/http" + "net/http/httptest" + "net/url" + "strings" + "sync/atomic" + "testing" + "time" + + "github.com/rossoctl/cortex/core/auth" + "github.com/rossoctl/cortex/core/listener/egressguard" + "github.com/rossoctl/cortex/core/pipeline" + "github.com/rossoctl/cortex/core/session" +) + +// guardProxy builds a forward proxy with the egress guard armed over the given +// permits, plus the session store its rows land in. +func guardProxy(t *testing.T, permits []string) (*httptest.Server, *session.Store) { + t.Helper() + g, err := egressguard.New(permits) + if err != nil { + t.Fatalf("egressguard.New: %v", err) + } + store := session.New(5*time.Minute, 100, 0) + srv, err := NewServer(outboundPipelineFromAuth(t, auth.New(auth.Config{})), store, nil, WithEgressGuard(g)) + if err != nil { + t.Fatalf("NewServer: %v", err) + } + proxy := httptest.NewServer(srv.Handler()) + t.Cleanup(proxy.Close) + return proxy, store +} + +// connectRaw speaks one CONNECT to the proxy over raw TCP and returns the +// response's status line and everything after it still buffered. +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) + } + // Drain the response headers up to the empty line, so a caller reading + // tunnel bytes off br starts at the first application byte. + 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 +} + +// TestForwardProxy_EgressGuardRefusesLoopbackCONNECT is the moca +// private_address_403 shape: a CONNECT to a loopback literal is answered 403 +// destination_private by the proxy itself and the upstream is never dialed. +func TestForwardProxy_EgressGuardRefusesLoopbackCONNECT(t *testing.T) { + var hits atomic.Int32 + upstream := httptest.NewServer(http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) { + hits.Add(1) + })) + defer upstream.Close() + target := strings.TrimPrefix(upstream.URL, "http://") + + proxy, store := guardProxy(t, nil) + line, br, conn := connectRaw(t, proxy, target) + defer conn.Close() + + if !strings.Contains(line, "403") { + t.Fatalf("CONNECT to loopback = %q, want 403", line) + } + body, _ := io.ReadAll(br) + if !strings.Contains(string(body), "destination_private") { + t.Errorf("403 body = %q, want the destination_private code", body) + } + if n := hits.Load(); n != 0 { + t.Errorf("upstream dialed %d times, want 0 — the refusal must precede the dial", n) + } + + // The refusal leaves its two rows: an open that names the reason and a + // 403 close carrying error.kind destination_private. + v := store.View(session.DefaultSessionID) + if v == nil || len(v.Events) != 2 { + t.Fatalf("want a tunnel open and close row, got %+v", v) + } + open, closeRow := v.Events[0], v.Events[1] + if !open.Tunnel || open.TunnelReason != pipeline.TunnelDestPrivate { + t.Errorf("open row = tunnel:%v reason:%q, want dest-private", open.Tunnel, open.TunnelReason) + } + if closeRow.StatusCode != 403 || closeRow.Error == nil || closeRow.Error.Kind != "destination_private" { + t.Errorf("close row = status:%d error:%+v, want 403 destination_private", closeRow.StatusCode, closeRow.Error) + } +} + +// TestForwardProxy_EgressGuardRefusesPrivateGET pins the request path: a +// plain GET to a loopback destination is refused by the guarded transport +// dial and answered 403 destination_private, with both rows recorded. +func TestForwardProxy_EgressGuardRefusesPrivateGET(t *testing.T) { + var hits atomic.Int32 + upstream := httptest.NewServer(http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) { + hits.Add(1) + })) + defer upstream.Close() + + proxy, store := guardProxy(t, nil) + client := &http.Client{Transport: &http.Transport{Proxy: mustParseProxy(proxy)}} + resp, err := client.Get(upstream.URL + "/path") + if err != nil { + t.Fatalf("GET through the guarded proxy: %v", err) + } + defer resp.Body.Close() + body, _ := io.ReadAll(resp.Body) + + if resp.StatusCode != http.StatusForbidden { + t.Errorf("status = %d, want 403", resp.StatusCode) + } + if !strings.Contains(string(body), "destination_private") { + t.Errorf("body = %q, want the destination_private code", body) + } + if n := hits.Load(); n != 0 { + t.Errorf("upstream hit %d times, want 0", n) + } + + v := store.View(session.DefaultSessionID) + if v == nil || len(v.Events) != 2 { + t.Fatalf("want a request and a response row, got %+v", v) + } + if v.Events[1].StatusCode != 403 || v.Events[1].Error == nil || v.Events[1].Error.Kind != "destination_private" { + t.Errorf("response row = status:%d error:%+v, want 403 destination_private", + v.Events[1].StatusCode, v.Events[1].Error) + } +} + +// TestForwardProxy_EgressGuardMetadataEvenIfPermitted is the moca +// metadata_address_403_even_if_permitted shape: 169.254.169.254 stays refused +// under permits that cover it (0.0.0.0/0 no less) — link-local is never +// permit-able. +func TestForwardProxy_EgressGuardMetadataEvenIfPermitted(t *testing.T) { + proxy, _ := guardProxy(t, []string{"169.254.0.0/16", "0.0.0.0/0"}) + for _, target := range []string{"169.254.169.254:443", "169.254.0.9:80"} { + line, _, conn := connectRaw(t, proxy, target) + if !strings.Contains(line, "403") { + t.Errorf("CONNECT %s with covering permits = %q, want 403 — link-local is never permit-able", target, line) + } + _ = conn.Close() + } +} + +// TestForwardProxy_EgressGuardPermitTunnelsAndRecordsDialAddr is the +// permissive half of the moca contract: with 127.0.0.1/32 permitted, a CONNECT +// to a loopback upstream tunnels, and the open row carries dialAddr — the +// audit trail that says the address checked is the address dialed. +func TestForwardProxy_EgressGuardPermitTunnelsAndRecordsDialAddr(t *testing.T) { + ln, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Skipf("loopback listen unavailable: %v", err) + } + defer ln.Close() + go func() { + for { + c, aerr := ln.Accept() + if aerr != nil { + return + } + go func() { + defer c.Close() + buf := make([]byte, 1024) + for { + n, rerr := c.Read(buf) + if rerr != nil { + return + } + if _, werr := c.Write(buf[:n]); werr != nil { + return + } + } + }() + } + }() + target := ln.Addr().String() + + proxy, store := guardProxy(t, []string{"127.0.0.1/32"}) + line, br, conn := connectRaw(t, proxy, target) + defer conn.Close() + if !strings.Contains(line, "200") { + t.Fatalf("CONNECT to permitted loopback = %q, want 200", line) + } + if _, err := conn.Write([]byte("ping")); err != nil { + t.Fatalf("write through tunnel: %v", err) + } + got := make([]byte, 4) + _ = conn.SetReadDeadline(time.Now().Add(2 * time.Second)) + if _, err := io.ReadFull(br, got); err != nil || string(got) != "ping" { + t.Fatalf("echo through tunnel = %q (%v), want ping", got, err) + } + _ = conn.Close() + + // The close row lands after the tunnel ends; poll for the pair. + deadline := time.Now().Add(2 * time.Second) + for { + v := store.View(session.DefaultSessionID) + if v != nil && len(v.Events) == 2 { + if v.Events[0].DialAddr != target { + t.Errorf("open row dialAddr = %q, want %q", v.Events[0].DialAddr, target) + } + if !v.Events[0].Tunnel { + t.Errorf("open row is not a tunnel row: %+v", v.Events[0]) + } + return + } + if time.Now().After(deadline) { + t.Fatalf("want a tunnel open and close row, got %+v", v) + } + time.Sleep(10 * time.Millisecond) + } +} + +// TestForwardProxy_EgressGuardOffByDefault pins the compatibility contract: +// with no guard passed, a CONNECT to loopback tunnels exactly as before. +func TestForwardProxy_EgressGuardOffByDefault(t *testing.T) { + ln, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Skipf("loopback listen unavailable: %v", err) + } + defer ln.Close() + + srv, err := NewServer(outboundPipelineFromAuth(t, auth.New(auth.Config{})), nil, nil) + if err != nil { + t.Fatalf("NewServer: %v", err) + } + proxy := httptest.NewServer(srv.Handler()) + defer proxy.Close() + + line, _, conn := connectRaw(t, proxy, ln.Addr().String()) + defer conn.Close() + if !strings.Contains(line, "200") { + t.Fatalf("CONNECT with the guard off = %q, want 200 — off must mean byte-for-byte unchanged", line) + } +} + +func mustParseProxy(proxy *httptest.Server) func(*http.Request) (*url.URL, error) { + return func(*http.Request) (*url.URL, error) { return mustParseURL(proxy.URL), nil } +} diff --git a/core/listener/forwardproxy/server.go b/core/listener/forwardproxy/server.go index ca41b2603..e675bb8a8 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/egressguard" "github.com/rossoctl/cortex/core/listener/httpx" "github.com/rossoctl/cortex/core/listener/internal/bodycount" "github.com/rossoctl/cortex/core/listener/internal/bodyread" @@ -139,6 +140,18 @@ type Server struct { // for byte. DeclineResponsesWebSockets bool + // EgressGuard, when non-nil, checks every destination this proxy dials — + // CONNECT tunnels, plain and bridged requests, transparent redirects, and + // skip_hosts traffic included, because a bypass must not become an SSRF + // bypass — against the refusal set in core/listener/egressguard: resolved + // once, refused with 403 destination_private when the address is loopback, + // link-local, RFC 1918 / unique-local, or one of this host's own addresses + // and no permit covers it, and dialed at the checked address so a second + // resolution can never re-answer (the DNS-rebinding window). nil keeps the + // listener's dial behavior unchanged; see WithEgressGuard and + // config.ListenerConfig.EgressGuard. + EgressGuard *egressguard.Guard + // Processes, when non-nil, names the process behind each client connection, and a // request with no session header is filed under that process's session — or its // nearest ancestor's — before client affinity is consulted; see @@ -202,10 +215,37 @@ type MTLSOptions struct { Metrics *authtls.Metrics } +// Option configures optional forward-proxy behavior in NewServer. Variadic so +// the core constructor signature — and every existing caller — is unchanged. +type Option func(*options) + +type options struct { + egress *egressguard.Guard +} + +// WithEgressGuard arms the SSRF dial guard (#1484): every destination the +// proxy dials is resolved once, refused with 403 destination_private when it +// lands in loopback / link-local / private / this host's own address space +// uncovered by a permit, and dialed at the checked address so DNS rebinding +// cannot re-answer between the check and the connect. Applies to CONNECT +// tunnels (Server.EgressGuard, checked in handleConnect and the transparent +// listener) and to every request the transport dials, mTLS included — under +// mTLS the check runs before the TCP connect, so the TLS handshake never +// reaches a refused address. A nil guard is accepted and keeps the dial +// behavior unchanged. +func WithEgressGuard(g *egressguard.Guard) Option { + return func(o *options) { o.egress = g } +} + // NewServer creates a forward proxy server with a default HTTP client. // When mtls is non-nil, every outbound dial does TLS-or-fail using the // local SVID; see MTLSOptions for semantics. -func NewServer(outbound *pipeline.Holder, sessions *session.Store, mtls *MTLSOptions) (*Server, error) { +func NewServer(outbound *pipeline.Holder, sessions *session.Store, mtls *MTLSOptions, opts ...Option) (*Server, error) { + var o options + for _, opt := range opts { + opt(&o) + } + transport := &http.Transport{ // Sane Go defaults for everything except DialContext, which we // customize when mTLS is on. @@ -232,12 +272,17 @@ func NewServer(outbound *pipeline.Holder, sessions *session.Store, mtls *MTLSOpt if err != nil { return nil, fmt.Errorf("forwardproxy: build client tls config: %w", err) } - transport.DialContext = mtlsDialer(tlsCfg, mtls.Metrics).DialContext + // The guard sits INSIDE the mTLS dialer: check first, then connect, + // then handshake — a refused address never gets a TLS ClientHello. + transport.DialContext = mtlsDialer(tlsCfg, mtls.Metrics).withGuard(o.egress).DialContext + } else if o.egress != nil { + transport.DialContext = o.egress.DialContext } return &Server{ OutboundPipeline: outbound, Sessions: sessions, + EgressGuard: o.egress, Client: &http.Client{ // No Client.Timeout — Go applies that to the entire // request lifecycle including body read, which kills @@ -259,6 +304,10 @@ type mtlsDialFunc struct { plain *net.Dialer tlsCfg *cryptotls.Config metrics *authtls.Metrics + // egress, when non-nil, checks the destination before the plain dial — + // the TLS handshake is the step after the connect, so it never reaches an + // address the guard refused. nil keeps the unguarded dial. + egress *egressguard.Guard } func mtlsDialer(cfg *cryptotls.Config, metrics *authtls.Metrics) *mtlsDialFunc { @@ -269,8 +318,27 @@ func mtlsDialer(cfg *cryptotls.Config, metrics *authtls.Metrics) *mtlsDialFunc { } } +// withGuard returns d with the egress guard set. A nil guard is fine: the +// dialer then behaves exactly as without it, which is why NewServer can pass +// o.egress unconditionally under mTLS. +func (d *mtlsDialFunc) withGuard(g *egressguard.Guard) *mtlsDialFunc { + d.egress = g + return d +} + func (d *mtlsDialFunc) DialContext(ctx context.Context, network, addr string) (net.Conn, error) { - plain, err := d.plain.DialContext(ctx, network, addr) + // SNI below reads the ORIGINAL addr, so the name the request used stays + // the handshake's ServerName; only the TCP connect follows the checked + // address. A literal destination is its own checked address. + dialAddr := addr + if d.egress != nil { + checked, err := d.egress.CheckAddr(ctx, network, addr) + if err != nil { + return nil, err + } + dialAddr = checked + } + plain, err := d.plain.DialContext(ctx, network, dialAddr) if err != nil { // TCP failure — separate bug class from "peer doesn't speak TLS". // Returned as-is so callers see the underlying dial error. @@ -548,6 +616,30 @@ func (s *Server) serveOutbound(w http.ResponseWriter, r *http.Request, tl *tunne resp, err = s.resendIfAsked(r, pctx, client, resp, tl, ownEncoding) } if err != nil { + // The egress guard's refusal, BEFORE the generic 502: a destination that + // resolved into refused space is a policy answer (403 destination_private), + // not a failed exchange. errors.As traverses the *url.Error client.Do wraps + // every dial error in; see TransportError's note on that wrapping. + var ref *egressguard.Refusal + if errors.As(err, &ref) { + slog.Debug("forward-proxy: destination refused by egress guard", + "host", r.Host, "method", r.Method, "addr", ref.Addr.String()) + // The listener's standard rejection shape, code verbatim from the MOCA + // egress contract so the shared conformance suite matches. + httpx.WriteRejection(w, pipeline.DenyStatus(http.StatusForbidden, "destination_private", + fmt.Sprintf("refusing to reach private destination %s (%s)", ref.Host, ref.Addr))) + // A delivered answer, not a failed exchange — the 426 rule: the Finisher + // reads non-zero StatusCode as an allow, and the guard did what it was + // configured to. usage keys off EventError.Kind, which stays set below. + pctx.StatusCode = http.StatusForbidden + if !skipped { + s.recordOutboundResponseEvent(pctx, http.StatusForbidden, &pipeline.EventError{ + Kind: pipeline.ErrorKindDestinationPrivate, + Message: ref.Error(), + }) + } + return + } // A 502 with the cause, or a 499 when the client hung up first — which, // with no timeout on this client, is how a hung upstream usually ends. // See pipeline.ExchangeFailure. @@ -2178,7 +2270,48 @@ func (s *Server) handleConnect(w http.ResponseWriter, r *http.Request) { // Plain TCP dial. See package-level comment on why mTLS doesn't // apply here. r.Host on a CONNECT carries "host:port" already. - upstream, err := net.DialTimeout("tcp", r.Host, connectDialTimeout) + // + // dialTarget is what actually gets dialed: r.Host itself when the guard is + // off, else the address the guard checked — resolved once here, and reused + // verbatim by the bridge-fall-open re-dial below, so no second resolution + // can ever re-answer between check and connect (the DNS-rebinding window). + dialTarget := r.Host + if s.EgressGuard != nil { + checked, cerr := s.EgressGuard.CheckAddr(r.Context(), "tcp", r.Host) + if cerr != nil { + var ref *egressguard.Refusal + if errors.As(cerr, &ref) { + slog.Warn("forward-proxy: CONNECT destination refused by egress guard", + "host", r.Host, "addr", ref.Addr.String()) + // A policy answer, not a failed exchange. skip_hosts destinations + // are refused too — a bypass must not become an SSRF bypass — and + // tl's rows are no-ops for them, exactly as the dial failure's are. + httpx.WriteRejection(w, pipeline.DenyStatus(http.StatusForbidden, "destination_private", + fmt.Sprintf("refusing to reach private destination %s (%s)", ref.Host, ref.Addr))) + // The 426 rule again: a delivered 403 the Finisher reads as an + // allow, because the guard did what it was configured to. + pctx.StatusCode = http.StatusForbidden + tl.open(pipeline.TunnelDestPrivate) + tl.close(http.StatusForbidden, &pipeline.EventError{ + Kind: pipeline.ErrorKindDestinationPrivate, + Message: ref.Error(), + }, 0, 0) + return + } + // Not a refusal: the name would not resolve. Same shape as a dial + // failure — nothing was dialed, and the client gets the 502. + slog.Warn("forward-proxy: CONNECT destination resolution failed", "host", r.Host, "error", cerr) + http.Error(w, `{"error":"bad gateway"}`, http.StatusBadGateway) + tl.open(pipeline.TunnelDialFailed) + tl.close(http.StatusBadGateway, dialError(cerr), 0, 0) + return + } + dialTarget = checked + // The audit trail against rebinding: "asked for the name, dialed this + // address" — moca's egress conformance reads it off the tunnel open row. + pctx.DialedAddr = checked + } + upstream, err := net.DialTimeout("tcp", dialTarget, connectDialTimeout) if err != nil { slog.Warn("forward-proxy: CONNECT upstream dial failed", "host", r.Host, "error", err) http.Error(w, `{"error":"bad gateway"}`, http.StatusBadGateway) @@ -2232,8 +2365,10 @@ func (s *Server) handleConnect(w http.ResponseWriter, r *http.Request) { return } // fell open → re-dial for the tunnel. That tunnel is opaque, so it closes - // like any other opaque one. - up2, derr := net.DialTimeout("tcp", r.Host, connectDialTimeout) + // like any other opaque one. dialTarget, not r.Host: the address was + // already checked once, and re-resolving here would reopen the rebinding + // window the check closed. + up2, derr := net.DialTimeout("tcp", dialTarget, connectDialTimeout) if derr != nil { // The client already has its 200, so that stays the status, and the // error says why the tunnel died. Closing the client is what makes diff --git a/core/listener/forwardproxy/transparent.go b/core/listener/forwardproxy/transparent.go index 1a4c50459..5ac2e6d03 100644 --- a/core/listener/forwardproxy/transparent.go +++ b/core/listener/forwardproxy/transparent.go @@ -2,12 +2,14 @@ package forwardproxy import ( "context" + "errors" "io" "log/slog" "net" "net/http" "time" + "github.com/rossoctl/cortex/core/listener/egressguard" "github.com/rossoctl/cortex/core/pipeline" "github.com/rossoctl/cortex/core/session" "github.com/rossoctl/cortex/core/tlsbridge" @@ -140,7 +142,34 @@ func (s *Server) HandleTransparentConn(clientConn net.Conn, dst string) { // Always dial the original IP (dst), never the sniffed name — the agent // already chose the IP, and re-resolving the name could diverge from it. - upstream, err := net.DialTimeout("tcp", dst, connectDialTimeout) + // + // The egress guard applies here too, dst being a literal IP: its own + // check is over the literal, so no resolution can intervene. The refusal + // is recorded but never SENT — this path speaks no HTTP to the client — + // the same way the dial failure's synthetic 502 is. + dialTarget := dst + if s.EgressGuard != nil { + checked, cerr := s.EgressGuard.CheckAddr(ctx, "tcp", dst) + if cerr != nil { + var ref *egressguard.Refusal + if errors.As(cerr, &ref) { + slog.Warn("transparent-proxy: destination refused by egress guard", + "host", host, "dst", dst, "addr", ref.Addr.String()) + tl.open(pipeline.TunnelDestPrivate) + tl.close(http.StatusForbidden, &pipeline.EventError{ + Kind: pipeline.ErrorKindDestinationPrivate, + Message: ref.Error(), + }, 0, 0) + return + } + tl.open(pipeline.TunnelDialFailed) + tl.close(http.StatusBadGateway, dialError(cerr), 0, 0) + return + } + dialTarget = checked + pctx.DialedAddr = checked + } + upstream, err := net.DialTimeout("tcp", dialTarget, connectDialTimeout) if err != nil { slog.Warn("transparent-proxy: upstream dial failed", "host", host, "dst", dst, "error", err) // The 502 is synthetic — this path sends the client no status at all — but the @@ -179,7 +208,9 @@ func (s *Server) HandleTransparentConn(clientConn net.Conn, dst string) { return } // bridgeServe fell open (upstream-verify failed) → re-dial for the tunnel. - up2, derr := net.DialTimeout("tcp", dst, connectDialTimeout) + // dialTarget: the checked address, never re-resolved — same rule as + // handleConnect's fall-open re-dial. + up2, derr := net.DialTimeout("tcp", dialTarget, connectDialTimeout) if derr != nil { tl.close(http.StatusOK, dialError(derr), 0, 0) return @@ -264,7 +295,12 @@ func (s *Server) tunnelOpenEvent(pctx *pipeline.Context, reason pipeline.TunnelR // Why the bytes stayed opaque. The caller knows; this function does // not, which is why it is a parameter rather than something derived // here from host shape. - TunnelReason: reason, + TunnelReason: reason, + // The address the guard checked and the tunnel dialed — set only when + // the egress guard is on, else absent (SessionEvent.DialAddr). On the + // open row, which is where a reader pairs it with the Host the client + // named; the close row pairs by RequestID and needs no copy. + DialAddr: pctx.DialedAddr, Client: pctx.ClientInfo(), Subagent: pctx.Subagent(), ParentSubagent: pctx.ParentSubagent(), diff --git a/core/pipeline/context.go b/core/pipeline/context.go index b80176246..841c554a1 100644 --- a/core/pipeline/context.go +++ b/core/pipeline/context.go @@ -150,6 +150,15 @@ type Context struct { // an origin quietly serves the un-upgraded request instead. UpgradeStripped string + // DialedAddr is the address the listener dialed for this request, "ip:port", + // set when the egress guard is enabled — the resolved address the guard + // checked, which may differ from Host whenever the destination was named. + // Read by the recorder onto SessionEvent.DialAddr; moca's egress conformance + // pins it as the audit trail against DNS rebinding. Listener-set: a plugin + // has no way to write it, for the same reason it has no way to write Host + // after the dial. + DialedAddr string + // StartedAt is the wall-clock time this context was constructed by the // listener at the start of a request. Used on the response path to // compute SessionEvent.Duration without walking the event history. diff --git a/core/pipeline/session.go b/core/pipeline/session.go index f29d6c7e8..649a3a0b1 100644 --- a/core/pipeline/session.go +++ b/core/pipeline/session.go @@ -240,6 +240,19 @@ type SessionEvent struct { // listing — none of which the timeline hinted at. TunnelReason TunnelReason + // DialAddr is the address the proxy actually dialed for this event's + // destination, "ip:port", set by the listener when the egress guard is + // enabled (listener.egress_guard). It differs from Host whenever the + // destination was named: Host is what the request said, DialAddr is the + // resolved address the guard checked and the only address the connection + // can reach. On a DNS-rebindable name the two together are the audit trail + // — "asked for example.com, dialed 203.0.113.7" — and moca's egress + // conformance reads this field (its dial_addr) off the tunnel open row. + // + // Absent when the guard is off, and on events that dialed nothing. Set + // only by the listener, never by a plugin. + DialAddr string + // UpgradeStripped names the protocol a client asked to switch to on a request // whose Upgrade header the forward proxy removed — see // pipeline.Context.UpgradeStripped for why that is both correct and @@ -430,6 +443,14 @@ const ( // case can never produce the EMPTY string, which a consumer reads as "bridged" — // the exact opposite of what happened, and invisible in a timeline. TunnelPassthroughUnknown TunnelReason = "passthrough-unknown" + // TunnelDestPrivate — the egress guard refused the destination before any + // dial: it resolved to loopback, link-local (cloud metadata), RFC 1918 / + // unique-local space, or one of this host's own addresses, and no permit + // covered it (link-local is never permit-able). Recorded with a 403 close + // row carrying error.kind "destination_private" — the MOCA egress contract's + // identifier, used verbatim. Never confused with dial-failed: a refused + // destination was never attempted. + TunnelDestPrivate TunnelReason = "dest-private" ) // EventTLS describes the TLS state of a connection that produced a @@ -547,6 +568,10 @@ type sessionEventWire struct { Subagent string `json:"subagent,omitempty"` // omitempty: absent on every row but those of an agent another agent started. ParentSubagent string `json:"parentSubagent,omitempty"` + // omitempty: absent on every row the egress guard did not dial for — it is + // off by default, so the common row costs nothing — and the same skew + // reason as TunnelReason above. + DialAddr string `json:"dialAddr,omitempty"` } func (e SessionEvent) MarshalJSON() ([]byte, error) { @@ -579,6 +604,7 @@ func (e SessionEvent) MarshalJSON() ([]byte, error) { Client: e.Client, Subagent: e.Subagent, ParentSubagent: e.ParentSubagent, + DialAddr: e.DialAddr, }) } @@ -619,6 +645,7 @@ func (e *SessionEvent) UnmarshalJSON(data []byte) error { Client: w.Client, Subagent: w.Subagent, ParentSubagent: w.ParentSubagent, + DialAddr: w.DialAddr, } return nil } @@ -645,6 +672,15 @@ type EventError struct { // usage does not count it as an error, and the Finisher sees an allow. const ErrorKindUpgradeDeclined = "upgrade_declined" +// ErrorKindDestinationPrivate is the egress guard refusing a destination that +// resolved into loopback, link-local, private or this host's own address space +// (listener.egress_guard), rendered as 403 destination_private. Like +// upgrade_declined it rides on an EventError to say whose answer a 4xx is, and +// usage does not count it as an error: the guard did what it was configured to. +// The string is the MOCA egress contract's identifier, used verbatim so the +// shared conformance suite matches without a translation table. +const ErrorKindDestinationPrivate = "destination_private" + // SessionSource is how a listener resolved Context.Session. type SessionSource string diff --git a/core/pipeline/session_test.go b/core/pipeline/session_test.go index e67727142..555816f47 100644 --- a/core/pipeline/session_test.go +++ b/core/pipeline/session_test.go @@ -88,6 +88,7 @@ func TestSessionEvent_JSONRoundTrip(t *testing.T) { // A different id from Subagent's, so the round trip catches Marshal and // Unmarshal disagreeing about which field is which. ParentSubagent: "a0974b0b8591dc76b", + DialAddr: "203.0.113.7:443", } first, err := json.Marshal(orig) @@ -122,7 +123,7 @@ func TestSessionEvent_MarshalJSON_OmitsEmpty(t *testing.T) { // recorded none of these must not emit the keys at all, so a client // predating them sees the same bytes it always did. "tunnel", "tunnelReason", "httpMethod", "httpPath", "requestedHost", - "subagent", "parentSubagent", + "subagent", "parentSubagent", "dialAddr", } { if strings.Contains(s, `"`+field+`":`) { t.Errorf("expected %q omitted when zero: %s", field, s) diff --git a/core/sessionapi/summary_test.go b/core/sessionapi/summary_test.go index 17de745e9..f5595fd11 100644 --- a/core/sessionapi/summary_test.go +++ b/core/sessionapi/summary_test.go @@ -360,7 +360,11 @@ func TestSummarizeEvent_ShapeIsGuarded(t *testing.T) { // 27 since Subagent, which is TIMELINE data: it says which agent made the row. A // scalar capped at maxClientLen, so the struct copy keeps it; asserted above. // 28 since ParentSubagent, TIMELINE data for the same reason; asserted above. - {"SessionEvent", reflect.TypeOf(pipeline.SessionEvent{}), 28}, + // 29 since DialAddr, which is kept: agentop's detail pane can draw the dialed + // address beside Host the way it draws redirected: — and the moca egress + // conformance reads it off the projected row. A scalar, so the struct copy + // keeps it. + {"SessionEvent", reflect.TypeOf(pipeline.SessionEvent{}), 29}, // 25 since AgentRole, which is TIMELINE data: agentop's CONTEXT gauge reads it on every // row the timeline serves. It needs no assertion of its own in the projection test // beyond the equality one there — a scalar survives the struct copy, unlike the two diff --git a/core/tlsbridge/upstream.go b/core/tlsbridge/upstream.go index b2cfa0217..9714c64f0 100644 --- a/core/tlsbridge/upstream.go +++ b/core/tlsbridge/upstream.go @@ -1,20 +1,44 @@ package tlsbridge import ( + "context" "crypto/tls" "crypto/x509" "fmt" + "net" "net/http" "time" ) +// UpstreamOption configures the client NewUpstreamClient builds. Variadic so +// the existing two-argument signature — and every caller outside this change — +// is unchanged. +type UpstreamOption func(*upstreamOptions) + +type upstreamOptions struct { + dial func(ctx context.Context, network, addr string) (net.Conn, error) +} + +// WithDialContext replaces the re-origination client's dialer. The forward +// proxy passes the egress guard's DialContext here (#1484) so a bridged +// request is subject to the same destination check as a plaintext one — the +// bridge must not become the path around the SSRF guard. +func WithDialContext(dial func(ctx context.Context, network, addr string) (net.Conn, error)) UpstreamOption { + return func(o *upstreamOptions) { o.dial = dial } +} + // NewUpstreamClient builds the HTTP client the TLS bridge uses to re-originate // to the real origin. RootCAs = system roots + extraRootsPEM (the agent's // injected upstream trust). When insecure is true it sets InsecureSkipVerify so // re-origination does NOT verify the origin cert — for internal/self-signed // upstreams (e.g. a private LiteLLM endpoint). Prefer extraRootsPEM // (upstream_ca_bundle) over insecure whenever the origin CA is available. -func NewUpstreamClient(extraRootsPEM []byte, insecure bool) (*http.Client, error) { +func NewUpstreamClient(extraRootsPEM []byte, insecure bool, opts ...UpstreamOption) (*http.Client, error) { + var o upstreamOptions + for _, opt := range opts { + opt(&o) + } + pool, err := x509.SystemCertPool() if err != nil || pool == nil { pool = x509.NewCertPool() @@ -32,6 +56,7 @@ func NewUpstreamClient(extraRootsPEM []byte, insecure bool) (*http.Client, error IdleConnTimeout: 90 * time.Second, TLSHandshakeTimeout: 10 * time.Second, ExpectContinueTimeout: time.Second, + DialContext: o.dial, }, CheckRedirect: func(_ *http.Request, _ []*http.Request) error { return http.ErrUseLastResponse }, }, nil diff --git a/docs/framework-architecture.md b/docs/framework-architecture.md index ba1a43fd7..c6a5a3bc6 100644 --- a/docs/framework-architecture.md +++ b/docs/framework-architecture.md @@ -931,6 +931,46 @@ apply, like every `listener.*` field. The key goes in the runtime config: on a l `authbridge-runtime-config` ConfigMap, then a pod restart. See [`agents/codex.md`](agents/codex.md#what-cortex-shows-for-it) for the measured difference. +#### Refusing private destinations (`listener.egress_guard`) + +Arms the forward proxy's SSRF dial guard (#1484): every destination the proxy +dials — CONNECT tunnels, plain and bridged requests, transparent redirects, +and `skip_hosts` traffic too, because a bypass must not become an SSRF bypass +— is resolved **once**, refused with **403 `destination_private`** when the +address is loopback, link-local (including the cloud metadata address +`169.254.169.254`), RFC 1918 / unique-local space, or one of this host's own +interface addresses, and then **dialed at the checked address**. That last +part is the DNS-rebinding defense: the address the guard checked and the +address the proxy connects to are the same value by construction, so a second +lookup can never re-answer between check and connect. + +```yaml +listener: + egress_guard: + enabled: true + permit_cidrs: ["127.0.0.1/32"] # prefixes within the refused space +``` + +**Off by default, deliberately.** A sidecar's ordinary upstreams — Keycloak, +in-cluster services, a laptop's local model — live in exactly the refused +space, so defaulting this on would break every existing deployment. Enable it +where the sandbox's egress is meant to be public-only, and list the private +prefixes still needed in `permit_cidrs`. + +`permit_cidrs` covers prefixes within the refused space only. Link-local, +unspecified and multicast addresses are **never** permitted, whatever the list +says: `0.0.0.0` reaches loopback on every mainstream stack, and +`169.254.169.254` is the metadata service this guard exists among other +things to protect — a permit of `0.0.0.0/0` does not open it. + +A refused CONNECT leaves its two tunnel rows with +`tunnelReason: "dest-private"` and `error.kind: "destination_private"`; a +refused request leaves an ordinary response row with the same error kind. +A permitted tunnel's open row carries `dialAddr`, the checked address that +was dialed — the audit trail that pairs the name the client asked for with +the address the proxy reached. Restart to apply, like every `listener.*` +field. See `core/listener/egressguard` for the refusal set's exact rules. + --- ## 10. Writing a plugin diff --git a/docs/session-api.md b/docs/session-api.md index eb40f0daa..920e023f4 100644 --- a/docs/session-api.md +++ b/docs/session-api.md @@ -250,7 +250,12 @@ Every event on `/v1/sessions/{id}` and `/v1/events` carries: "upgrade_declined"` is the forward proxy's own answer to a WebSocket upgrade on OpenAI's Responses API (`chatgpt.com` or `api.openai.com`): nothing is forwarded, and a client such as Codex reads it as "use HTTPS" and retries at once over - HTTPS. It is not counted as an error in `/v1/usage`. + HTTPS. It is not counted as an error in `/v1/usage`. A **403** with + `error.kind: "destination_private"` is the egress guard's refusal + (`listener.egress_guard`): the destination resolved into loopback, + link-local, private or this host's own address space and no permit covered + it. Also the proxy's own answer rather than the upstream's, and also not + counted as an error — the guard did what it was configured to. - `requestedHost` — present only when a plugin redirected the request: the host the client asked for. `host` is where the request actually went, and usage, the cost ledger and pricing all follow `host`. @@ -270,7 +275,16 @@ Every event on `/v1/sessions/{id}` and `/v1/events` carries: "response"`), carrying the CONNECT's own status, the duration, and the bytes each way. It is not the destination's status: that travels inside the client's end-to-end TLS. Tunnel rows are kept out of `/v1/usage`: a tunnel's - lifetime is not a request latency. + lifetime is not a request latency. `tunnelReason: "dest-private"` is the + egress guard's refusal: the close row carries 403 and + `error.kind: "destination_private"`, and no tunnel was ever opened to the + destination — the same refusal a plain request gets, one layer down. +- `dialAddr` — present on a tunnel's open row when the egress guard is on: + the address the guard checked and the proxy dialed, `"ip:port"`. Together + with `host` it is the audit trail against DNS rebinding — "asked for the + name, dialed this address" — because under the guard the proxy never dials + an address it did not check. Absent when the guard is off and on rows that + dialed nothing. - `httpMethod`, `httpPath` — the HTTP verb and query-stripped, percent-decoded path, so a request no parser recognized is still identifiable. On an opaque tunnel `httpMethod` is `CONNECT` and `httpPath` From 00f29e8b15a83ff8570ca3cce86007fbbb692ba6 Mon Sep 17 00:00:00 2001 From: Mariusz Sabath Date: Wed, 28 Oct 2026 21:45:54 -0400 Subject: [PATCH 2/5] fix: tighten two review findings on the egress guard (#1484) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Self-review of #1539, both accuracy rather than behavior: - SessionEvent.DialAddr's comment claimed the field is absent "on events that dialed nothing", but a guarded plain or bridged request dials through the guarded transport and records no dialAddr either — only a tunnel's open row carries it. Say that instead: present only on a tunnel's open row, absent when the guard is off. session-api.md already said exactly this. - The "egress guard on" boot log fired wherever the guard was configured, including a reverse-only deployment where no forward proxy exists to enforce it. The guard is still built before the TLS bridge (the bridge's re-origination client consumes it), but the announcement now logs in the forward-role block, where it is true. Assisted-By: Claude (Anthropic AI) Signed-off-by: Mariusz Sabath --- cmd/cortex/main.go | 10 ++++++++-- core/pipeline/session.go | 6 ++++-- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/cmd/cortex/main.go b/cmd/cortex/main.go index dd3804ee5..994c78f01 100644 --- a/cmd/cortex/main.go +++ b/cmd/cortex/main.go @@ -816,8 +816,10 @@ func main() { fatalf("listener.egress_guard: %v", gerr) } egress = g - slog.Info("egress guard on: refusing loopback, link-local, private and host-local destinations", - "permit_cidrs", cfg.Listener.EgressGuard.PermitCIDRs) + // The "on" announcement is logged down in the forward-role block, not + // here: the guard is only ever enforced through the forward proxy, and + // a reverse-only deployment must not boot announcing an enforcement no + // listener applies. } // TLS bridge: when enabled, the forward proxy terminates agent outbound @@ -999,6 +1001,10 @@ func main() { if ferr != nil { fatalf("creating forward proxy: %v", ferr) } + if egress != nil { + slog.Info("egress guard on: refusing loopback, link-local, private and host-local destinations", + "permit_cidrs", cfg.Listener.EgressGuard.PermitCIDRs) + } // SkipHosts: outbound destinations that bypass the pipeline AND // session recording entirely. See ListenerConfig.SkipHosts for the // motivating case (chatty observability sidecars evicting the diff --git a/core/pipeline/session.go b/core/pipeline/session.go index 649a3a0b1..9694f2cf5 100644 --- a/core/pipeline/session.go +++ b/core/pipeline/session.go @@ -249,8 +249,10 @@ type SessionEvent struct { // — "asked for example.com, dialed 203.0.113.7" — and moca's egress // conformance reads this field (its dial_addr) off the tunnel open row. // - // Absent when the guard is off, and on events that dialed nothing. Set - // only by the listener, never by a plugin. + // Present only on a tunnel's open row: a plain or bridged request dials + // through the guarded transport too, but nothing threads the checked + // address back to its pctx, so its rows carry Host alone. Absent when the + // guard is off. Set only by the listener, never by a plugin. DialAddr string // UpgradeStripped names the protocol a client asked to switch to on a request From 2a3cc17a969580ed5d3525555b5370ba156c40be Mon Sep 17 00:00:00 2001 From: Mariusz Sabath Date: Wed, 28 Oct 2026 21:54:36 -0400 Subject: [PATCH 3/5] fix: sanitize client-controlled values in the egress guard's log lines (#1484) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeQL on #1539 flagged "Log entries created from user input" at the serveOutbound refusal Debug: r.Host and r.Method are client-controlled, and a control rune forged into either would forge a log entry — the same CWE-150 shape the User-Agent path already guards against. - Rename pipeline.sanitizeUA to pipeline.SanitizeControlRunes. The rule (C0/DEL/C1/bidi/zero-width and invalid UTF-8 replaced with U+FFFD) and its rationale are unchanged; only the name now says what it is: the rule applies to any client-controlled string headed for a surface a terminal or a file renders, not only a User-Agent. Exported so the listeners can use the one copy rather than grow a third loop beside the ledger's sanitizeLabel. - Apply it at every log site this PR added: the serveOutbound refusal Debug (Host, Method), handleConnect's refusal Warn (Host) and resolution-failure Warn (Host and the error string, which embeds the name via net.DNSError), and the transparent listener's refusal Warn (host — sniffed off the client's own bytes as SNI or a Host header). Values that are not client-controlled — the guard's parsed address, the kernel's SO_ORIGINAL_DST literal — stay raw. Assisted-By: Claude (Anthropic AI) Signed-off-by: Mariusz Sabath --- core/listener/forwardproxy/server.go | 15 ++++++++++++--- core/listener/forwardproxy/transparent.go | 7 ++++++- core/pipeline/client.go | 10 ++++++++-- core/pipeline/context.go | 2 +- 4 files changed, 27 insertions(+), 7 deletions(-) diff --git a/core/listener/forwardproxy/server.go b/core/listener/forwardproxy/server.go index e675bb8a8..69fc9421b 100644 --- a/core/listener/forwardproxy/server.go +++ b/core/listener/forwardproxy/server.go @@ -623,7 +623,10 @@ func (s *Server) serveOutbound(w http.ResponseWriter, r *http.Request, tl *tunne var ref *egressguard.Refusal if errors.As(err, &ref) { slog.Debug("forward-proxy: destination refused by egress guard", - "host", r.Host, "method", r.Method, "addr", ref.Addr.String()) + // Sanitized: Host and Method are client-controlled, and a newline + // forged into either would forge a log entry (#1539's CodeQL finding). + "host", pipeline.SanitizeControlRunes(r.Host), + "method", pipeline.SanitizeControlRunes(r.Method), "addr", ref.Addr.String()) // The listener's standard rejection shape, code verbatim from the MOCA // egress contract so the shared conformance suite matches. httpx.WriteRejection(w, pipeline.DenyStatus(http.StatusForbidden, "destination_private", @@ -2282,7 +2285,9 @@ func (s *Server) handleConnect(w http.ResponseWriter, r *http.Request) { var ref *egressguard.Refusal if errors.As(cerr, &ref) { slog.Warn("forward-proxy: CONNECT destination refused by egress guard", - "host", r.Host, "addr", ref.Addr.String()) + // Sanitized like the Debug line in serveOutbound: r.Host is + // client-controlled and lands in a log file. + "host", pipeline.SanitizeControlRunes(r.Host), "addr", ref.Addr.String()) // A policy answer, not a failed exchange. skip_hosts destinations // are refused too — a bypass must not become an SSRF bypass — and // tl's rows are no-ops for them, exactly as the dial failure's are. @@ -2300,7 +2305,11 @@ func (s *Server) handleConnect(w http.ResponseWriter, r *http.Request) { } // Not a refusal: the name would not resolve. Same shape as a dial // failure — nothing was dialed, and the client gets the 502. - slog.Warn("forward-proxy: CONNECT destination resolution failed", "host", r.Host, "error", cerr) + // Both values sanitized: the name is client-controlled, and the error + // embeds it (net.DNSError quotes the name it failed to resolve). + slog.Warn("forward-proxy: CONNECT destination resolution failed", + "host", pipeline.SanitizeControlRunes(r.Host), + "error", pipeline.SanitizeControlRunes(cerr.Error())) http.Error(w, `{"error":"bad gateway"}`, http.StatusBadGateway) tl.open(pipeline.TunnelDialFailed) tl.close(http.StatusBadGateway, dialError(cerr), 0, 0) diff --git a/core/listener/forwardproxy/transparent.go b/core/listener/forwardproxy/transparent.go index 5ac2e6d03..7f5a480c4 100644 --- a/core/listener/forwardproxy/transparent.go +++ b/core/listener/forwardproxy/transparent.go @@ -154,7 +154,12 @@ func (s *Server) HandleTransparentConn(clientConn net.Conn, dst string) { var ref *egressguard.Refusal if errors.As(cerr, &ref) { slog.Warn("transparent-proxy: destination refused by egress guard", - "host", host, "dst", dst, "addr", ref.Addr.String()) + // host is sniffed off the client's own bytes (SNI or Host header), + // so it gets the sanitizer; dst is the kernel's SO_ORIGINAL_DST + // literal and ref.Addr the guard's parsed address — neither is + // client-controlled. + "host", pipeline.SanitizeControlRunes(host), + "dst", dst, "addr", ref.Addr.String()) tl.open(pipeline.TunnelDestPrivate) tl.close(http.StatusForbidden, &pipeline.EventError{ Kind: pipeline.ErrorKindDestinationPrivate, 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 841c554a1..e9e19d7e0 100644 --- a/core/pipeline/context.go +++ b/core/pipeline/context.go @@ -900,7 +900,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 b3dcd1f58d6610948c4819c888c59d21bb3c9f46 Mon Sep 17 00:00:00 2001 From: Mariusz Sabath Date: Thu, 29 Oct 2026 09:06:57 -0400 Subject: [PATCH 4/5] fix: address review findings on the egress guard (#1484) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Hai's review of #1539, all six findings: - countsAsError only exempted upgrade_declined, so /v1/usage counted the guard's 403 destination_private as an error while four places of documentation said it does not. The kind is now exempted alongside upgrade_declined — a refusal is the policy working, not the proxy failing — with a test next to the declined-upgrade one that pins the distinction from an origin's own 403. - The "host's own addresses" check tested the interface's on-link PREFIX, refusing every LAN neighbour and, on a cloud VM with a public address on a large on-link prefix, ordinary public upstreams. hostOwns now compares the interface's address itself; a test pins the distinction with the exact prefixes from the review. - fd00:ec2::254 (AWS metadata, IPv6 form) is ULA and so sat in the permit-able set; it is never-permitted now. 100.64.0.0/10 (RFC 6598 CGNAT, home of Alibaba's metadata service) is refused like private space and permit-able like it. A well-known-prefix NAT64 address (64:ff9b::/96) is checked by the IPv4 it embeds, so the wrapped 127.0.0.1 is refused while the v6 form stays the dial target. All three are named in the package comment and the docs. - DialTimeout had no caller; deleted. - The port is parsed with net.LookupPort before the resolve: a service-name port ("example.com:https") dials again instead of 502ing, and a bad port costs no DNS lookup. - The SanitizeControlRunes doc block opened with "sanitizeUA replaces…", a function that no longer exists; the rename paragraph now leads the block and the seven other comment references follow the new name. Assisted-By: Claude (Anthropic AI) Signed-off-by: Mariusz Sabath --- core/config/config.go | 22 +++-- core/cost/usage/usage.go | 9 +- core/cost/usage/usage_test.go | 26 ++++++ core/listener/egressguard/guard.go | 111 +++++++++++++++++------- core/listener/egressguard/guard_test.go | 100 ++++++++++++++++++++- core/pipeline/client.go | 28 +++--- core/pipeline/context.go | 2 +- docs/framework-architecture.md | 26 ++++-- 8 files changed, 256 insertions(+), 68 deletions(-) diff --git a/core/config/config.go b/core/config/config.go index 2c1c965df..1d4e45ef2 100644 --- a/core/config/config.go +++ b/core/config/config.go @@ -948,11 +948,13 @@ type ListenerConfig struct { // EgressGuard arms the SSRF dial guard on the forward proxy's every // outbound dial (#1484): each destination is resolved once, refused with // 403 destination_private when it lands in loopback, link-local (cloud - // metadata, 169.254.169.254), RFC 1918 / unique-local space, or one of - // this host's own addresses — and dialed at the checked address, so DNS - // rebinding cannot re-answer between check and connect. CONNECT tunnels, - // plain and bridged requests, transparent redirects and skip_hosts traffic - // are all covered: a bypass must not become an SSRF bypass. + // metadata, 169.254.169.254), RFC 1918 / unique-local space, RFC 6598 + // CGNAT (100.64.0.0/10), or one of this host's own interface addresses — + // the addresses themselves, not their on-link subnets — and dialed at the + // checked address, so DNS rebinding cannot re-answer between check and + // connect. CONNECT tunnels, plain and bridged requests, transparent + // redirects and skip_hosts traffic are all covered: a bypass must not + // become an SSRF bypass. // // OFF BY DEFAULT, deliberately: a sidecar's ordinary upstreams — Keycloak, // in-cluster services, a laptop's local model — live in exactly the @@ -962,9 +964,10 @@ type ListenerConfig struct { // and lists the private prefixes still allowed in permit_cidrs. // // permit_cidrs covers prefixes within the refused space only; link-local, - // unspecified and multicast addresses are NEVER permitted, whatever the - // list says — the metadata range is what this guard exists among other - // things to protect. + // unspecified, multicast and the AWS IPv6 metadata address + // (fd00:ec2::254) are NEVER permitted, whatever the list says — the + // metadata ranges are what this guard exists among other things to + // protect. // // A pointer so "absent" (off) stays distinguishable from an explicit // enabled: false, mirroring DeclineResponsesWebSockets. Listener.* field: @@ -981,7 +984,8 @@ type EgressGuardConfig struct { // PermitCIDRs lists CIDR prefixes within the refused space that remain // dialable — e.g. ["127.0.0.1/32"] for a laptop proxy whose upstream is a // local model, or ["10.0.0.0/8"] for an in-cluster deployment. Never has - // any effect on link-local, unspecified or multicast addresses. + // any effect on link-local, unspecified, multicast or the AWS IPv6 + // metadata address (fd00:ec2::254). PermitCIDRs []string `yaml:"permit_cidrs,omitempty" json:"permit_cidrs,omitempty"` } diff --git a/core/cost/usage/usage.go b/core/cost/usage/usage.go index 9f7b20867..88a2b3fe0 100644 --- a/core/cost/usage/usage.go +++ b/core/cost/usage/usage.go @@ -1270,8 +1270,15 @@ func (a *Aggregator) evictColdestLocked() { // 426 is Cortex telling Codex to use HTTPS, which it then does, once per session: by // design and on the default path, so counting it would give every healthy Codex // fleet a nonzero error rate. The Finisher sees it as an allow for the same reason. +// +// And except the egress guard's refusal (pipeline.ErrorKindDestinationPrivate). Same +// shape, same reasoning: its 403 is the guard doing exactly what the operator +// configured it to, on traffic that was never going to be allowed through — a +// nonzero "error rate" made of refusals is the policy working, not the proxy +// failing. The Finisher sees it as an allow for the same reason. func countsAsError(e *pipeline.SessionEvent) bool { - if e.Error != nil && e.Error.Kind == pipeline.ErrorKindUpgradeDeclined { + if e.Error != nil && (e.Error.Kind == pipeline.ErrorKindUpgradeDeclined || + e.Error.Kind == pipeline.ErrorKindDestinationPrivate) { return false } return e.StatusCode >= 400 || e.Phase == pipeline.SessionDenied diff --git a/core/cost/usage/usage_test.go b/core/cost/usage/usage_test.go index 86c3187f8..59f94b714 100644 --- a/core/cost/usage/usage_test.go +++ b/core/cost/usage/usage_test.go @@ -549,6 +549,32 @@ func TestRecord_DeclinedUpgradeIsNotAnError(t *testing.T) { } } +// Same shape for the egress guard: its 403 is the guard doing what the operator +// configured, so a refusal must not read as a failing proxy. The exemption is the +// kind, not the status — an origin's own 403 is still an error. +func TestRecord_DestinationPrivateIsNotAnError(t *testing.T) { + now := time.Date(2026, 9, 4, 23, 30, 30, 0, time.UTC) + a := New(WithClock(fixedClock(now))) + + refused := respEvent(now, 403, 0, "", 0) + refused.Error = &pipeline.EventError{Kind: pipeline.ErrorKindDestinationPrivate} + a.Record("s1", refused) + origin403 := respEvent(now, 403, 0, "", 0) + origin403.Error = &pipeline.EventError{Kind: "backend_error", Code: "403"} + a.Record("s1", origin403) + + b := a.Snapshot(time.Minute, BucketWidth, "", GroupStatus).Buckets[0] + if b.Requests != 2 { + t.Errorf("requests = %d, want 2", b.Requests) + } + if b.Errors != 1 { + t.Errorf("errors = %d, want 1 — the origin's 403 only, not the guard's refusal", b.Errors) + } + if _, ok := b.Series["403"]; !ok { + t.Errorf("both 403s still belong under their status; got keys %v", keys(b.Series)) + } +} + // Request events carry no status, duration or usage. Counting them would double // every request. func TestRecord_IgnoresRequestPhase(t *testing.T) { diff --git a/core/listener/egressguard/guard.go b/core/listener/egressguard/guard.go index e3b4a3483..f0851835b 100644 --- a/core/listener/egressguard/guard.go +++ b/core/listener/egressguard/guard.go @@ -4,12 +4,21 @@ // checked — never a second resolution's answer. // // The refused set is loopback, link-local (including the cloud metadata -// address 169.254.169.254), RFC 1918 / unique-local space, and the host's own -// interface addresses. An operator may permit prefixes within that space -// (a laptop proxy whose upstream is a local model, an in-cluster deployment) -// — but link-local, unspecified and multicast addresses are never permitted: -// 0.0.0.0 reaches loopback on every mainstream stack and 169.254.169.254 is -// the metadata service the guard exists among other things to protect. +// address 169.254.169.254), RFC 1918 / unique-local space, RFC 6598 CGNAT +// shared space (100.64.0.0/10 — Alibaba's metadata service, 100.100.100.200, +// lives there), and the host's own interface addresses — the addresses +// themselves, NOT their on-link subnets: a neighbour in the host's /24 is +// not this host. An operator may permit prefixes within that space (a laptop +// proxy whose upstream is a local model, an in-cluster deployment) — but +// link-local, unspecified, multicast and the AWS IPv6 metadata address +// (fd00:ec2::254) are never permitted: 0.0.0.0 reaches loopback on every +// mainstream stack, 169.254.169.254 is the metadata service the guard exists +// among other things to protect, and fd00:ec2::254 is that service's IPv6 +// form — ULA, so it would otherwise sit in the permit-able set. +// +// A well-known-prefix NAT64 address (64:ff9b::/96) is checked by the IPv4 +// address it embeds, so [64:ff9b::7f00:1] is refused as the 127.0.0.1 it +// carries; the v6 form is still what gets dialed. package egressguard import ( @@ -17,9 +26,7 @@ import ( "fmt" "net" "net/netip" - "strconv" "strings" - "time" ) // Guard checks and dials outbound destinations. Construct with New; the zero @@ -91,6 +98,15 @@ func (g *Guard) CheckAddr(ctx context.Context, network, addr string) (string, er if strings.HasPrefix(host, "[") && strings.HasSuffix(host, "]") { host = host[1 : len(host)-1] } + // The port BEFORE the resolve: net.LookupPort accepts both numbers and + // service names ("https" — what a CONNECT request may carry, and what + // net.DialTimeout would have resolved itself before the guard existed), + // and a bad port now costs no DNS lookup. It also range-checks, which is + // what the strconv.ParseUint here used to do. + p, err := net.LookupPort(network, port) + if err != nil { + return "", err + } var dial []netip.Addr if ip, err := netip.ParseAddr(host); err == nil { @@ -112,16 +128,45 @@ func (g *Guard) CheckAddr(ctx context.Context, network, addr string) (string, er // Check the first answer — the one the caller will dial. See CheckAddr's // doc for why the later records are not consulted. dialAddr := dial[0].Unmap() - if ref := g.check(host, dialAddr); ref != nil { - return "", ref + // A NAT64 well-known-prefix address is judged by the IPv4 it embeds — + // [64:ff9b::7f00:1] is 127.0.0.1 wearing a v6 hat — but the v6 form is + // what gets dialed and returned, since on a NAT64 host the v4 may not + // route. The mapping is arithmetic, not a lookup, so checking one form + // and dialing the other opens no check-vs-dial window. + checkAddr := dialAddr + if embedded, ok := nat64Embedded(dialAddr); ok { + checkAddr = embedded } - p, err := strconv.ParseUint(port, 10, 16) - if err != nil { - return "", err + if ref := g.check(host, checkAddr); ref != nil { + return "", ref } return netip.AddrPortFrom(dialAddr, uint16(p)).String(), nil } +// nat64Prefix is the NAT64 well-known prefix (RFC 6052): a v6 address in it +// carries the IPv4 destination in its low 32 bits. +var nat64Prefix = netip.MustParsePrefix("64:ff9b::/96") + +// nat64Embedded returns the IPv4 address a well-known-prefix NAT64 address +// embeds, and whether ip was one at all. +func nat64Embedded(ip netip.Addr) (netip.Addr, bool) { + if ip.Is4() || ip.Is4In6() || !nat64Prefix.Contains(ip) { + return netip.Addr{}, false + } + b := ip.As16() + return netip.AddrFrom4([4]byte(b[12:16])), true +} + +// cgnat is RFC 6598 shared address space: not RFC 1918, not globally routed, +// and home to Alibaba's metadata service (100.100.100.200). Refused like +// private space, permit-able like it too. +var cgnat = netip.MustParsePrefix("100.64.0.0/10") + +// awsMetadataV6 is the AWS instance metadata service's IPv6 form. It is ULA, +// so without this entry it would sit in the permit-able set — and an operator +// permitting fd00::/8 (plausible for a ULA-internal upstream) would open it. +var awsMetadataV6 = netip.MustParseAddr("fd00:ec2::254") + // DialContext is CheckAddr followed by a dial of the checked address — // dialer-shaped, for http.Transport. Only the dial address is replaced; the // transport's TLS ServerName and certificate verification still follow the @@ -135,16 +180,6 @@ func (g *Guard) DialContext(ctx context.Context, network, addr string) (net.Conn return d.DialContext(ctx, network, checked) } -// DialTimeout is DialContext with a fixed timeout, for the CONNECT-tunnel -// dial sites that predate contexts. -func (g *Guard) DialTimeout(ctx context.Context, network, addr string, timeout time.Duration) (net.Conn, error) { - checked, err := g.CheckAddr(ctx, network, addr) - if err != nil { - return nil, err - } - return net.DialTimeout(network, checked, timeout) -} - // check applies the refusal rules to one address. It returns the Refusal to // answer the request with, or nil when the address may be dialed. func (g *Guard) check(host string, ip netip.Addr) *Refusal { @@ -166,10 +201,13 @@ func (g *Guard) check(host string, ip netip.Addr) *Refusal { // neverPermitted reports the ranges no permit can open, as a Refusal when // the address is in one. Link-local unicast carries the cloud metadata -// service (169.254.169.254 on every major cloud); unspecified reaches -// loopback on mainstream stacks; multicast is not a unicast destination. +// service (169.254.169.254 on every major cloud, fd00:ec2::254 on AWS's +// IPv6 path); unspecified reaches loopback on mainstream stacks; multicast +// is not a unicast destination. func (g *Guard) neverPermitted(host string, ip netip.Addr) *Refusal { switch { + case ip == awsMetadataV6: + return &Refusal{Host: host, Addr: ip} case ip.IsLinkLocalUnicast(): return &Refusal{Host: host, Addr: ip} case ip.IsUnspecified(): @@ -181,12 +219,12 @@ func (g *Guard) neverPermitted(host string, ip netip.Addr) *Refusal { } // refusable reports whether ip is in the refused-but-permit-able space: -// loopback, RFC 1918 / unique-local, or one of this host's own interface -// addresses. The last set is enumerated per check — an interface address -// added after boot is guarded without a restart, and InterfaceAddrs costs -// microseconds against a dial that costs milliseconds. +// loopback, RFC 1918 / unique-local, RFC 6598 CGNAT, or one of this host's +// own interface addresses. The last set is enumerated per check — an +// interface address added after boot is guarded without a restart, and +// InterfaceAddrs costs microseconds against a dial that costs milliseconds. func (g *Guard) refusable(ip netip.Addr) bool { - if ip.IsLoopback() || ip.IsPrivate() { + if ip.IsLoopback() || ip.IsPrivate() || cgnat.Contains(ip) { return true } ifaces, err := net.InterfaceAddrs() @@ -195,12 +233,21 @@ func (g *Guard) refusable(ip netip.Addr) bool { // candidate as one of them. return true } - for _, ia := range ifaces { + return hostOwns(ip, ifaces) +} + +// hostOwns reports whether ip is one of the host's own interface addresses. +// The address ITSELF, not the interface's on-link subnet: net.InterfaceAddrs +// hands back ip/mask pairs, and testing the prefix would refuse every +// neighbour on the LAN — and on a cloud VM with a public address on a large +// on-link prefix, ordinary public upstreams (#1539 review). +func hostOwns(ip netip.Addr, ifaceAddrs []net.Addr) bool { + for _, ia := range ifaceAddrs { prefix, err := netip.ParsePrefix(ia.String()) if err != nil { continue } - if prefix.Contains(ip) { + if prefix.Addr().Unmap() == ip { return true } } diff --git a/core/listener/egressguard/guard_test.go b/core/listener/egressguard/guard_test.go index 2c7b60987..50100f72e 100644 --- a/core/listener/egressguard/guard_test.go +++ b/core/listener/egressguard/guard_test.go @@ -62,8 +62,9 @@ func TestCheckAddr_RefusedRanges(t *testing.T) { {"10.0.0.9:8080", "10.0.0.9"}, {"172.16.5.4:80", "172.16.5.4"}, {"192.168.1.10:80", "192.168.1.10"}, - {"[fc00::9]:443", "fc00::9"}, // unique-local - {"0.0.0.0:80", "0.0.0.0"}, // unspecified reaches loopback + {"[fc00::9]:443", "fc00::9"}, // unique-local + {"100.100.100.200:80", "100.100.100.200"}, // Alibaba metadata, in RFC 6598 CGNAT + {"0.0.0.0:80", "0.0.0.0"}, // unspecified reaches loopback {"224.0.0.1:80", "224.0.0.1"}, } { _, err := g.CheckAddr(context.Background(), "tcp", c.addr) @@ -99,6 +100,7 @@ func TestCheckAddr_NeverPermitted(t *testing.T) { "169.254.169.254:80", "169.254.0.9:80", "[fe80::9]:443", + "[fd00:ec2::254]:80", // AWS metadata, IPv6 form — ULA, so a fd00::/8 permit must not open it "0.0.0.0:80", "[ff02::1]:443", } { @@ -182,6 +184,100 @@ func TestCheckAddr_MappedIPv6(t *testing.T) { } } +// TestCheckAddr_NAT64CheckedByEmbeddedV4: a well-known-prefix address is +// judged by the IPv4 it embeds — the loopback one is refused, a public one +// passes — and the v6 form is what comes back as the dial target, since on a +// NAT64 host the bare v4 may not route. +func TestCheckAddr_NAT64CheckedByEmbeddedV4(t *testing.T) { + g, err := New(nil) + if err != nil { + t.Fatalf("New: %v", err) + } + if _, err := g.CheckAddr(context.Background(), "tcp", "[64:ff9b::7f00:1]:443"); err == nil { + t.Error("NAT64-wrapped 127.0.0.1 = nil, want a Refusal") + } + if _, err := g.CheckAddr(context.Background(), "tcp", "[64:ff9b::a9fe:a9fe]:80"); err == nil { + t.Error("NAT64-wrapped 169.254.169.254 = nil, want a Refusal") + } + checked, err := g.CheckAddr(context.Background(), "tcp", "[64:ff9b::102:304]:443") + if err != nil { + t.Fatalf("NAT64-wrapped 1.2.3.4: %v", err) + } + if checked != "[64:ff9b::102:304]:443" { + t.Errorf("NAT64 dial addr = %q, want the v6 form back", checked) + } +} + +// TestCheckAddr_CGNATPermittable: RFC 6598 space is refused by default but, +// like RFC 1918, a covering permit opens it — an operator whose carrier +// network legitimately sits there should not need 0.0.0.0/0 to say so. +func TestCheckAddr_CGNATPermittable(t *testing.T) { + g, err := New([]string{"100.64.0.0/10"}) + if err != nil { + t.Fatalf("New: %v", err) + } + if _, err := g.CheckAddr(context.Background(), "tcp", "100.100.100.200:80"); err != nil { + t.Errorf("CGNAT with a covering permit: %v", err) + } +} + +// TestCheckAddr_ServiceNamePort: a CONNECT target may name its port as a +// service ("example.com:https"), which net.DialTimeout resolved itself before +// the guard existed — LookupPort keeps that working — and an unparseable port +// is rejected BEFORE the resolve, so it costs no DNS lookup. +func TestCheckAddr_ServiceNamePort(t *testing.T) { + g, err := New(nil) + if err != nil { + t.Fatalf("New: %v", err) + } + stub := &stubResolver{answers: map[string][]netip.Addr{ + "dst.test": {mustAddr(t, "203.0.113.7")}, + }} + g.resolve = stub.resolve + + checked, err := g.CheckAddr(context.Background(), "tcp", "dst.test:https") + if err != nil { + t.Fatalf("service-name port: %v", err) + } + if checked != "203.0.113.7:443" { + t.Errorf("dial addr = %q, want 203.0.113.7:443", checked) + } + + stub.lookups = 0 + if _, err := g.CheckAddr(context.Background(), "tcp", "dst.test:notaport"); err == nil { + t.Error("unparseable port = nil, want an error") + } + if stub.lookups != 0 { + t.Errorf("resolver ran %d times on a bad port, want 0 — the port is checked first", stub.lookups) + } +} + +// ifaceAddr stands in for a net.InterfaceAddrs entry, whose String() is +// "ip/mask". +type ifaceAddr string + +func (a ifaceAddr) Network() string { return "ip+net" } +func (a ifaceAddr) String() string { return string(a) } + +// TestHostOwns pins the #1539 review fix: the host's own interface ADDRESS is +// refused, its on-link SUBNET is not. net.InterfaceAddrs hands back ip/mask +// pairs, and testing the prefix instead of the address would refuse every LAN +// neighbour — and, on a cloud VM with a public address on a large on-link +// prefix, ordinary public upstreams. +func TestHostOwns(t *testing.T) { + addrs := []net.Addr{ifaceAddr("192.168.1.167/24"), ifaceAddr("9.67.123.175/18")} + for _, own := range []string{"192.168.1.167", "9.67.123.175"} { + if !hostOwns(mustAddr(t, own), addrs) { + t.Errorf("hostOwns(%s) = false, want true — the interface's own address", own) + } + } + for _, neighbour := range []string{"192.168.1.1", "9.67.64.1", "9.67.127.254"} { + if hostOwns(mustAddr(t, neighbour), addrs) { + t.Errorf("hostOwns(%s) = true, want false — a subnet neighbour, not this host", neighbour) + } + } +} + // TestRefusalErrorsAs: the typed error must be reachable through the wrapping // net/http puts around dial errors (*url.Error), which is how serveOutbound // detects it. diff --git a/core/pipeline/client.go b/core/pipeline/client.go index 1f2d5af08..ff6b59477 100644 --- a/core/pipeline/client.go +++ b/core/pipeline/client.go @@ -44,7 +44,7 @@ type EventClient struct { // the agent was not recognised or sent no version. Version string `json:"version,omitempty"` // Raw is the User-Agent as it was sent, with tabs normalised to spaces, sanitised (see - // sanitizeUA) and capped at maxClientLen. + // SanitizeControlRunes) and capped at maxClientLen. // // "Verbatim" up to those three rules, which is as verbatim as a string that reaches a // terminal and a durable file can be. Worth knowing before comparing this against a @@ -97,7 +97,13 @@ type EventClient struct { // that order ever change, cap here at 96 rather than leaving the window open. const maxClientLen = 128 -// sanitizeUA replaces every control character in a User-Agent with U+FFFD. +// SanitizeControlRunes replaces every control character in s with U+FFFD. It +// is the rule that lived under sanitizeUA's name until #1539's CodeQL finding +// made the honest name necessary: 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, the same CWE-150 shape the UA path guards against. // // THE PRIMARY CHOKE POINT FOR THIS STRING. ParseUserAgent is the one place a header // becomes an EventClient, and Label() is what both consumers of the result read — the @@ -136,12 +142,6 @@ 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. -// 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 @@ -163,7 +163,7 @@ func SanitizeControlRunes(s string) string { return b.String() } -// hasControlRunes reports whether s carries anything sanitizeUA would replace. +// hasControlRunes reports whether s carries anything SanitizeControlRunes would replace. // // A RUNE scan rather than a byte scan, because the C1 block cannot be seen from the // bytes alone: 0xC2 0x9B is U+009B, and 0x9B on its own is a continuation byte of @@ -235,7 +235,7 @@ func IsControlRune(r rune) bool { // through cost/ledger the length of an appended line. Cutting on a rune boundary only // changes WHERE the byte cap lands (by up to three bytes), never that there is one. // -// It matters because of the order in ParseUserAgent: sanitizeUA runs first and can put +// It matters because of the order in ParseUserAgent: SanitizeControlRunes runs first and can put // 3-byte U+FFFD runes in this string, so a byte cut has a one-in-three chance of // leaving a fragment of one — invalid UTF-8 in a label that goes to a durable file and // to a chart, which is the failure the substitution was meant to avoid rather than @@ -448,7 +448,7 @@ func ParseUserAgent(ua string) *EventClient { } // SANITISED, THEN CAPPED, and both before ANY of it is retained or matched against, // so nothing derived from it downstream can carry a control character or exceed the - // bound. See sanitizeUA for what the first step removes and maxClientLen for what the + // bound. See SanitizeControlRunes for what the first step removes and maxClientLen for what the // second one is for. // // THE ORDER IS LOAD-BEARING, in the same direction as ledger.rowLabel's: a @@ -461,14 +461,14 @@ func ParseUserAgent(ua string) *EventClient { // TAB NORMALISED TO SPACE FIRST, ahead of the sanitiser, and it is not cosmetic. RFC // 9110 spells the separator RWS = 1*( SP / HTAB ), so a compliant client may separate // the product token from its comment with a tab — and a tab is a C0 control, so - // sanitizeUA turns it into U+FFFD and the split below then finds no whitespace at all. + // SanitizeControlRunes turns it into U+FFFD and the split below then finds no whitespace at all. // Measured without this: "claude-cli/2.1\t(external)" yielded Version // "2.1�(external)", a byAgent series key of its own, so one agent's spend divided // in two. // - // NORMALISATION, NOT SANITISATION, which is why it sits on this side of sanitizeUA + // NORMALISATION, NOT SANITISATION, which is why it sits on this side of SanitizeControlRunes // instead of inside it. Mapping legal whitespace onto the canonical whitespace is not - // the same job as neutralising what a terminal would act on. The rule inside sanitizeUA is + // the same job as neutralising what a terminal would act on. The rule inside SanitizeControlRunes 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(SanitizeControlRunes(strings.ReplaceAll(ua, "\t", " "))) diff --git a/core/pipeline/context.go b/core/pipeline/context.go index e9e19d7e0..90c1905db 100644 --- a/core/pipeline/context.go +++ b/core/pipeline/context.go @@ -476,7 +476,7 @@ type Context struct { // being parsed ten times; it does not escape. // // THE COPY IS AN ALLOCATION PER CALL, roughly ten per request, and that is the price of the -// line above. sanitizeUA's fast path is written not to allocate, which bounds what a hostile +// line above. SanitizeControlRunes's fast path is written not to allocate, which bounds what a hostile // header costs; this one is unconditional and buys the integrity claim instead. func (c *Context) ClientInfo() *EventClient { if c.clientParsed { diff --git a/docs/framework-architecture.md b/docs/framework-architecture.md index c6a5a3bc6..9e4bfcabc 100644 --- a/docs/framework-architecture.md +++ b/docs/framework-architecture.md @@ -938,11 +938,17 @@ dials — CONNECT tunnels, plain and bridged requests, transparent redirects, and `skip_hosts` traffic too, because a bypass must not become an SSRF bypass — is resolved **once**, refused with **403 `destination_private`** when the address is loopback, link-local (including the cloud metadata address -`169.254.169.254`), RFC 1918 / unique-local space, or one of this host's own -interface addresses, and then **dialed at the checked address**. That last -part is the DNS-rebinding defense: the address the guard checked and the -address the proxy connects to are the same value by construction, so a second -lookup can never re-answer between check and connect. +`169.254.169.254`), RFC 1918 / unique-local space, RFC 6598 CGNAT shared +space (`100.64.0.0/10`, where Alibaba's metadata service lives), or one of +this host's own interface addresses — the addresses themselves, not their +on-link subnets, so a LAN neighbour or a public neighbour on a large on-link +prefix is not caught by that clause — and then **dialed at the checked +address**. That last part is the DNS-rebinding defense: the address the guard +checked and the address the proxy connects to are the same value by +construction, so a second lookup can never re-answer between check and +connect. A well-known-prefix NAT64 address (`64:ff9b::/96`) is judged by the +IPv4 address it embeds, so `[64:ff9b::7f00:1]` is refused as the `127.0.0.1` +it carries. ```yaml listener: @@ -958,10 +964,12 @@ where the sandbox's egress is meant to be public-only, and list the private prefixes still needed in `permit_cidrs`. `permit_cidrs` covers prefixes within the refused space only. Link-local, -unspecified and multicast addresses are **never** permitted, whatever the list -says: `0.0.0.0` reaches loopback on every mainstream stack, and -`169.254.169.254` is the metadata service this guard exists among other -things to protect — a permit of `0.0.0.0/0` does not open it. +unspecified, multicast and the AWS IPv6 metadata address (`fd00:ec2::254`) +are **never** permitted, whatever the list says: `0.0.0.0` reaches loopback +on every mainstream stack, `169.254.169.254` is the metadata service this +guard exists among other things to protect — a permit of `0.0.0.0/0` does not +open it — and `fd00:ec2::254` is that service's IPv6 form, which would +otherwise sit in the permit-able ULA space. A refused CONNECT leaves its two tunnel rows with `tunnelReason: "dest-private"` and `error.kind: "destination_private"`; a From 3873b1c7961d501bcd4410486e307d80708e0d01 Mon Sep 17 00:00:00 2001 From: Mariusz Sabath Date: Thu, 29 Oct 2026 10:23:10 -0400 Subject: [PATCH 5/5] fix: strip the IPv6 zone before the refusal check; 403 on the transparent refusal (#1484) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two review findings from the CodeRabbit pass, each confirmed against the code before fixing and each with a test that fails without its fix. **A zoned IPv6 literal reached the host's own address.** netip.Addr equality includes the zone, and net.InterfaceAddrs reports the host's addresses unzoned, so `[2001:db8::5%eth0]` compared unequal to the `2001:db8::5` the host owns and walked past hostOwns — writing a zone the host itself would ignore was enough. CheckAddr now drops the zone for the CHECK and keeps it for the DIAL, since a link-scoped destination needs it to pick an interface. hostOwns was the whole exposure: loopback, link-local, private and CGNAT are range predicates, and a zone does not move an address out of a range (pinned by TestCheckAddr_ZonedV6StillRefused). Equality is the one place a zone bites. Stripping it also fixes a false refusal in the other direction — a permitted zoned ULA was being refused, because Prefix.Contains is false for any zoned address, so no permit could ever match one. The NAT64 decode now runs on the zone-stripped address too, which is strictly better: nat64Prefix.Contains was false for a zoned address, so a zoned NAT64 form previously skipped the embedded-v4 check entirely. Tests go through CheckAddr, not hostOwns: a test that strips the zone itself before calling hostOwns asserts the fix rather than the path, and passes against the unfixed code — I wrote that version first and it did. Verified both new tests fail before the fix (`= "[2001:db8::5%eth0]:443" with no refusal`) and pass after. `ifaceAddrs` joins `resolve` as a test seam, because the host-owned rule is the one refusal that depends on the machine the test runs on — on this laptop the only host-owned non-private address is IPv4, which cannot carry a zone, so the bug was untestable without it. **The transparent listener's refusal reported an error to Finishers.** handleConnect sets pctx.StatusCode = 403 before its tl.close; this path did not, and OutcomeFromContext reads a zero StatusCode as OutcomeError. So the three listener paths disagreed about the same refusal, this one reporting a failure for what CLAUDE.md, docs/session-api.md and framework-architecture.md all call a delivered 403 the Finisher sees as an allow. Nothing noticed because no status reaches the client on this path — it speaks no HTTP to the agent — so the field exists only for the Finishers and the row. Verified: full core suite, 73 packages, 0 failures. go vet and gofmt clean on every touched package. agentop's failures (launchd/systemd/service-manager and two server-picker TUI tests) reproduce identically on clean main — sandbox limitations, not this change. Assisted-By: Claude (Anthropic AI) Signed-off-by: Mariusz Sabath --- core/listener/egressguard/guard.go | 30 +++++++- core/listener/egressguard/guard_test.go | 77 +++++++++++++++++++ .../listener/forwardproxy/egressguard_test.go | 57 ++++++++++++++ core/listener/forwardproxy/transparent.go | 9 +++ 4 files changed, 170 insertions(+), 3 deletions(-) diff --git a/core/listener/egressguard/guard.go b/core/listener/egressguard/guard.go index f0851835b..dca372552 100644 --- a/core/listener/egressguard/guard.go +++ b/core/listener/egressguard/guard.go @@ -39,6 +39,13 @@ type Guard struct { // tests can pin "resolved exactly once" against a counting fake; the // production value is net.DefaultResolver.LookupNetIP. resolve func(ctx context.Context, host string) ([]netip.Addr, error) + + // ifaceAddrs enumerates this host's interface addresses. A field for the + // same reason as resolve: the host-owned rule is the one refusal that + // depends on the machine the test runs on, so a test asserting it has to + // supply the addresses rather than hope for them. Production is + // net.InterfaceAddrs. + ifaceAddrs func() ([]net.Addr, error) } // New builds a Guard that refuses the destinations described in the package @@ -59,6 +66,7 @@ func New(permitCIDRs []string) (*Guard, error) { resolve: func(ctx context.Context, host string) ([]netip.Addr, error) { return net.DefaultResolver.LookupNetIP(ctx, "ip", host) }, + ifaceAddrs: net.InterfaceAddrs, }, nil } @@ -128,13 +136,23 @@ func (g *Guard) CheckAddr(ctx context.Context, network, addr string) (string, er // Check the first answer — the one the caller will dial. See CheckAddr's // doc for why the later records are not consulted. dialAddr := dial[0].Unmap() + // The IPv6 zone is dropped for the CHECK and kept for the dial. netip.Addr + // equality includes the zone, so `[2001:db8::5%eth0]` compared unequal to the + // `2001:db8::5` that net.InterfaceAddrs reports and walked straight past + // hostOwns — the host's own address, reachable by writing a zone the host + // itself would ignore. Loopback, link-local and private were never exposed + // this way (those are range predicates, which a zone does not perturb), so + // hostOwns was the whole hole, and equality is exactly where a zone bites. + // A permit is unaffected either way: Prefix.Contains is false for any zoned + // address, so stripping the zone can only make a permit apply, never remove + // one (#1539 review). + checkAddr := dialAddr.WithZone("") // A NAT64 well-known-prefix address is judged by the IPv4 it embeds — // [64:ff9b::7f00:1] is 127.0.0.1 wearing a v6 hat — but the v6 form is // what gets dialed and returned, since on a NAT64 host the v4 may not // route. The mapping is arithmetic, not a lookup, so checking one form // and dialing the other opens no check-vs-dial window. - checkAddr := dialAddr - if embedded, ok := nat64Embedded(dialAddr); ok { + if embedded, ok := nat64Embedded(checkAddr); ok { checkAddr = embedded } if ref := g.check(host, checkAddr); ref != nil { @@ -227,7 +245,13 @@ func (g *Guard) refusable(ip netip.Addr) bool { if ip.IsLoopback() || ip.IsPrivate() || cgnat.Contains(ip) { return true } - ifaces, err := net.InterfaceAddrs() + enumerate := g.ifaceAddrs + if enumerate == nil { + // A zero-value Guard (and one built before this field existed) still + // asks the host. + enumerate = net.InterfaceAddrs + } + ifaces, err := enumerate() if err != nil { // Fail closed: unable to enumerate the host's addresses, treat the // candidate as one of them. diff --git a/core/listener/egressguard/guard_test.go b/core/listener/egressguard/guard_test.go index 50100f72e..7d275a22c 100644 --- a/core/listener/egressguard/guard_test.go +++ b/core/listener/egressguard/guard_test.go @@ -278,6 +278,83 @@ func TestHostOwns(t *testing.T) { } } +// A ZONED IPv6 LITERAL must not walk past the host-owned-address rule. +// netip.Addr equality includes the zone, and net.InterfaceAddrs reports the +// host's addresses unzoned, so `[2001:db8::5%eth0]` compared unequal to the +// `2001:db8::5` the host actually owns — writing a zone the host itself would +// ignore was enough to reach its own address. Confirmed against the code before +// the fix: hostOwns(bare) true, hostOwns(zoned) false (#1539 review). +// +// hostOwns is the whole exposure, which is why this test is here and not only in +// the CheckAddr table: loopback, link-local, private and CGNAT are all RANGE +// predicates, and a zone does not move an address out of a range. Equality is +// the one place a zone bites. +// Driven through CheckAddr, not hostOwns directly: a test that strips the zone +// itself before calling hostOwns asserts the fix rather than the path, and +// passes against the unfixed code. This one fails before the fix (verified) and +// passes after, because CheckAddr is where the stripping has to happen. +func TestCheckAddr_ZonedV6DoesNotEscapeHostOwned(t *testing.T) { + // A host whose own address is a GLOBAL IPv6 one — deliberately not loopback, + // link-local or ULA, so hostOwns is the only rule that can refuse it and the + // test cannot pass for an unrelated reason. + g, err := New(nil) + if err != nil { + t.Fatalf("New: %v", err) + } + g.ifaceAddrs = func() ([]net.Addr, error) { + return []net.Addr{ifaceAddr("2001:db8::5/64")}, nil + } + + // The premise: unzoned, the host's own address is refused. + if _, cerr := g.CheckAddr(context.Background(), "tcp", "[2001:db8::5]:443"); cerr == nil { + t.Fatal("CheckAddr([2001:db8::5]:443) = nil, want a refusal — the premise of this test") + } + // The hole: the same address with a zone the host itself would ignore. + got, cerr := g.CheckAddr(context.Background(), "tcp", "[2001:db8::5%eth0]:443") + if cerr == nil { + t.Errorf("CheckAddr([2001:db8::5%%eth0]:443) = %q with no refusal; a written zone reached the host's own address", got) + } +} + +// The zone must survive into the DIALED address even though the check ignores +// it: on a link-scoped destination Go needs the zone to pick an interface, so +// stripping it for the dial would break a legitimate request rather than +// refusing an illegitimate one. A permitted zoned address is the only way to +// observe this, since every refused one returns "". +func TestCheckAddr_ZoneKeptOnTheDialedAddress(t *testing.T) { + // fe80::/10 is never-permitted, so use a ULA with a covering permit: ULA is + // refused-but-permit-able, which is the state that lets a zoned address + // through to the return value. + g, err := New([]string{"fd00::/8"}) + if err != nil { + t.Fatalf("New: %v", err) + } + g.ifaceAddrs = func() ([]net.Addr, error) { return nil, nil } + + got, cerr := g.CheckAddr(context.Background(), "tcp", "[fd00::5%eth0]:443") + if cerr != nil { + t.Fatalf("CheckAddr on a permitted ULA = %v, want it allowed", cerr) + } + if want := "[fd00::5%eth0]:443"; got != want { + t.Errorf("dialed address = %q, want %q — the zone is dropped for the check, not for the dial", got, want) + } +} + +// The same hole through the full CheckAddr path, for the ranges a zone never +// moved: a zoned loopback and link-local stay refused. Pins that stripping the +// zone for the check did not loosen anything that was already closed. +func TestCheckAddr_ZonedV6StillRefused(t *testing.T) { + g, err := New(nil) + if err != nil { + t.Fatalf("New: %v", err) + } + for _, addr := range []string{"[::1%lo0]:443", "[fe80::1%eth0]:443"} { + if _, cerr := g.CheckAddr(context.Background(), "tcp", addr); cerr == nil { + t.Errorf("CheckAddr(%s) = nil, want a refusal", addr) + } + } +} + // TestRefusalErrorsAs: the typed error must be reachable through the wrapping // net/http puts around dial errors (*url.Error), which is how serveOutbound // detects it. diff --git a/core/listener/forwardproxy/egressguard_test.go b/core/listener/forwardproxy/egressguard_test.go index a9d0eda9d..f25d40712 100644 --- a/core/listener/forwardproxy/egressguard_test.go +++ b/core/listener/forwardproxy/egressguard_test.go @@ -256,6 +256,63 @@ func TestForwardProxy_EgressGuardOffByDefault(t *testing.T) { } } +// The TRANSPARENT listener's refusal must reach Finishers as an allow with 403, +// the same as both CONNECT paths. handleConnect sets pctx.StatusCode before its +// tl.close; this path did not, and OutcomeFromContext reads a zero StatusCode as +// OutcomeError — so the three listener paths disagreed about the same refusal, +// this one reporting a failure for what the docs call a delivered 403, and +// lineage labelling a deliberate answer a failure (#1539 review). +// +// No status reaches the client here: a transparently redirected connection +// speaks no HTTP to the agent, so the field exists for the Finishers and the +// row, which is exactly why nothing else noticed it was missing. +func TestTransparent_EgressGuardRefusalReportsAnAllowToFinishers(t *testing.T) { + g, err := egressguard.New(nil) + if err != nil { + t.Fatalf("egressguard.New: %v", err) + } + // Something to be refused: a loopback literal, which is what SO_ORIGINAL_DST + // hands this path for a redirected connection to the host itself. + ln, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Skipf("loopback listen unavailable: %v", err) + } + defer func() { _ = ln.Close() }() + + fin := &outcomeRecorder{} + store := session.New(5*time.Minute, 100, 0) + defer store.Close() + p, err := pipeline.New([]pipeline.Plugin{fin}) + if err != nil { + t.Fatal(err) + } + srv := &Server{ + OutboundPipeline: pipeline.NewHolder(p), + Sessions: store, + EgressGuard: g, + } + + agentSide, proxySide := net.Pipe() + go srv.HandleTransparentConn(proxySide, ln.Addr().String()) + defer func() { _ = agentSide.Close() }() + + // OnFinish runs on the handler's goroutine, after the refusal. + var got *pipeline.Outcome + for i := 0; i < 200; i++ { + if got = fin.seen(); got != nil { + break + } + time.Sleep(10 * time.Millisecond) + } + if got == nil { + t.Fatal("OnFinish never ran, so the outcome is unasserted") + } + if got.FinalAction != pipeline.OutcomeAllow || got.StatusCode != http.StatusForbidden { + t.Errorf("outcome = %v/%d, want allow/403 — the same shape handleConnect reports", + got.FinalAction, got.StatusCode) + } +} + func mustParseProxy(proxy *httptest.Server) func(*http.Request) (*url.URL, error) { return func(*http.Request) (*url.URL, error) { return mustParseURL(proxy.URL), nil } } diff --git a/core/listener/forwardproxy/transparent.go b/core/listener/forwardproxy/transparent.go index 7f5a480c4..9d172b3d4 100644 --- a/core/listener/forwardproxy/transparent.go +++ b/core/listener/forwardproxy/transparent.go @@ -160,6 +160,15 @@ func (s *Server) HandleTransparentConn(clientConn net.Conn, dst string) { // client-controlled. "host", pipeline.SanitizeControlRunes(host), "dst", dst, "addr", ref.Addr.String()) + // The 426 rule, as on both CONNECT paths: a non-zero StatusCode is + // what OutcomeFromContext reads as an allow, and the guard did what + // it was configured to. Without it the RunFinish defer above sees + // StatusCode 0 and hands every Finisher an OutcomeError, so the + // three listener paths disagreed about the same refusal — this one + // reporting an error for what the docs call a delivered 403. No + // status reaches the client here (this path speaks no HTTP to it); + // the field exists for the Finishers and the row (#1539 review). + pctx.StatusCode = http.StatusForbidden tl.open(pipeline.TunnelDestPrivate) tl.close(http.StatusForbidden, &pipeline.EventError{ Kind: pipeline.ErrorKindDestinationPrivate,