diff --git a/CLAUDE.md b/CLAUDE.md index cab08c207..0a5980c5d 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) @@ -636,7 +641,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. A response relayed straight through is counted, not zero — both proxies record the row after the copy (#1251's fix), so the bytes they relayed through their own process are on it. The one arm that still reports ↓0 is extproc with a body-free pipeline: it asked Envoy for body mode NONE, so the bytes it never saw are bytes it asked not to see — `core/listener/parity`'s `TestDivergence_InboundResponseBodyCountWhenNoPluginReadsIt` pins that disagreement with its reason. 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. Everywhere else the 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..994c78f01 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,28 @@ 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 + // 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 // TLS so the outbound pipeline sees decrypted HTTPS. Constructed // here and set on fpSrv below (mirroring fpSrv.SkipHosts / fpSrv.Shared). @@ -886,7 +909,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,10 +997,14 @@ 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) } + 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/config/config.go b/core/config/config.go index fc64f3e45..1d4e45ef2 100644 --- a/core/config/config.go +++ b/core/config/config.go @@ -944,6 +944,49 @@ 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, 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 + // 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, 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: + // 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, multicast or the AWS IPv6 + // metadata address (fd00:ec2::254). + 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/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 new file mode 100644 index 000000000..dca372552 --- /dev/null +++ b/core/listener/egressguard/guard.go @@ -0,0 +1,279 @@ +// 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, 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 ( + "context" + "fmt" + "net" + "net/netip" + "strings" +) + +// 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) + + // 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 +// 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) + }, + ifaceAddrs: net.InterfaceAddrs, + }, 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] + } + // 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 { + 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() + // 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. + if embedded, ok := nat64Embedded(checkAddr); ok { + checkAddr = embedded + } + 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 +// 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) +} + +// 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, 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(): + 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, 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() || cgnat.Contains(ip) { + return true + } + 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. + return true + } + 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.Addr().Unmap() == 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..7d275a22c --- /dev/null +++ b/core/listener/egressguard/guard_test.go @@ -0,0 +1,430 @@ +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 + {"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) + 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", + "[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", + } { + 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") + } +} + +// 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) + } + } +} + +// 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. +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..f25d40712 --- /dev/null +++ b/core/listener/forwardproxy/egressguard_test.go @@ -0,0 +1,318 @@ +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) + } +} + +// 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/server.go b/core/listener/forwardproxy/server.go index 8438c98bf..81a970ad1 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,33 @@ 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", + // 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", + 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. @@ -2210,7 +2305,54 @@ 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", + // 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. + 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. + // 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) + 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) @@ -2264,8 +2406,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..9d172b3d4 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,48 @@ 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 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()) + // 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, + 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 +222,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 +309,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/client.go b/core/pipeline/client.go index 36065eeb6..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,7 +142,7 @@ 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 { +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. @@ -157,7 +163,7 @@ func sanitizeUA(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 @@ -229,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 @@ -442,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 @@ -455,17 +461,17 @@ 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(sanitizeUA(strings.ReplaceAll(ua, "\t", " "))) + ua = capUA(SanitizeControlRunes(strings.ReplaceAll(ua, "\t", " "))) c := &EventClient{Raw: ua} // The product token is the first SPACE-delimited word, so the trailing comment Claude // Code appends — "(external, cli)" — is ignored rather than having to be matched. diff --git a/core/pipeline/context.go b/core/pipeline/context.go index a00315031..2aa24bfdc 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. @@ -480,7 +489,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 { @@ -904,7 +913,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 diff --git a/core/pipeline/session.go b/core/pipeline/session.go index a9aa73ddb..3b6cb5ec6 100644 --- a/core/pipeline/session.go +++ b/core/pipeline/session.go @@ -252,6 +252,21 @@ 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. + // + // 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 // whose Upgrade header the forward proxy removed — see // pipeline.Context.UpgradeStripped for why that is both correct and @@ -442,6 +457,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 @@ -566,6 +589,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) { @@ -599,6 +626,7 @@ func (e SessionEvent) MarshalJSON() ([]byte, error) { Client: e.Client, Subagent: e.Subagent, ParentSubagent: e.ParentSubagent, + DialAddr: e.DialAddr, }) } @@ -640,6 +668,7 @@ func (e *SessionEvent) UnmarshalJSON(data []byte) error { Client: w.Client, Subagent: w.Subagent, ParentSubagent: w.ParentSubagent, + DialAddr: w.DialAddr, } return nil } @@ -666,6 +695,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 b4d6b924f..b4ab62b6d 100644 --- a/core/pipeline/session_test.go +++ b/core/pipeline/session_test.go @@ -96,6 +96,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) @@ -130,7 +131,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 b9907c4e8..0b48e5f9f 100644 --- a/core/sessionapi/summary_test.go +++ b/core/sessionapi/summary_test.go @@ -371,7 +371,11 @@ func TestSummarizeEvent_ShapeIsGuarded(t *testing.T) { // its timing: line from the projected event, splitting a response's duration // into the origin's answer time and the body's (#1251). A scalar, so the struct // copy keeps it; asserted above. - {"SessionEvent", reflect.TypeOf(pipeline.SessionEvent{}), 29}, + // 30 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{}), 30}, // 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..9e4bfcabc 100644 --- a/docs/framework-architecture.md +++ b/docs/framework-architecture.md @@ -931,6 +931,54 @@ 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, 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: + 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, 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 +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 7ca899afe..9abafad33 100644 --- a/docs/session-api.md +++ b/docs/session-api.md @@ -257,7 +257,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. - `ttfbMs` — on a delivered response row, the wall time from request entry to the upstream's response **headers** arriving. `durationMs` − `ttfbMs` is then how long the body took — for a streamed completion, the stream time. The pair @@ -296,7 +301,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`