diff --git a/internal/capability/compose.go b/internal/capability/compose.go index 240f4ed..c64a600 100644 --- a/internal/capability/compose.go +++ b/internal/capability/compose.go @@ -6,6 +6,7 @@ import ( "errors" "fmt" "log/slog" + "net" "net/http" "net/url" "regexp" @@ -339,6 +340,9 @@ func Compose(ctx context.Context, docs []CapabilityDocument, opts ComposeOptions // Sanctioned identity-forwarding hosts. A malformed entry fails composition // closed, exactly as a malformed SSRF allow-list entry does. + if err := validateHostAllowList("CAPABILITY_IDENTITY_FORWARD_HOSTS", opts.IdentityForwardHosts); err != nil { + return nil, err + } sanctioned, err := parseHostAllowList(opts.IdentityForwardHosts, nil) if err != nil { return nil, err @@ -356,7 +360,7 @@ func Compose(ctx context.Context, docs []CapabilityDocument, opts ComposeOptions // Sanctioned MCP endpoint hosts (the gateway). Same fail-closed parse as the // two allow-lists above: a mis-typed entry must not silently leave the dial // site unguarded. - if err := validateEndpointHosts(opts.AllowedMCPEndpointHosts); err != nil { + if err := validateHostAllowList("CAPABILITY_MCP_ENDPOINT_HOSTS", opts.AllowedMCPEndpointHosts); err != nil { return nil, err } dialable, err := parseHostAllowList(opts.AllowedMCPEndpointHosts, nil) @@ -586,22 +590,31 @@ func ScopeDocuments(docs []CapabilityDocument, expectedProject string, logger *s return kept } -// validateEndpointHosts rejects an AllowedMCPEndpointHosts entry that is not a -// bare host. parseHostAllowList only hard-errors on a malformed CIDR, and this -// knob takes no CIDRs, so without this a mis-typed entry — a full URL -// ("https://gateway.example/mcp"), a host:port, a CIDR — would normalize into a -// string that can never match u.Hostname() and the allow-list would quietly -// sanction nothing. That failure mode is the worst one available here: every -// endpoint gets skipped and provider tools disappear cluster-wide with no -// configuration error to point at. Fail closed at startup instead. -func validateEndpointHosts(entries []string) error { +// validateHostAllowList rejects an entry that is not a bare host, for either of +// the two host allow-lists. +// +// Both are matched against a URL's hostname, so an entry written as a URL or as +// host:port can never match anything. Nothing else notices: the list parses, the +// assistant starts, and the setting silently does nothing — which for the dial +// allow-list means every provider tool disappears, and for the identity list +// means provider calls quietly stop carrying the caller. Refusing to start is +// the kinder outcome. +// +// knob names the setting in the error so an operator knows which one to fix. +func validateHostAllowList(knob string, entries []string) error { for _, e := range entries { trimmed := strings.TrimSpace(e) if trimmed == "" { continue // blank entries are dropped, as elsewhere } if strings.ContainsAny(trimmed, "/ \t") { - return fmt.Errorf("invalid MCP endpoint host %q: expected a bare host, not a URL, host:port, or CIDR", e) + return fmt.Errorf("invalid %s entry %q: expected a bare host, not a URL or CIDR", knob, e) + } + // A port is the easy mistake to make, since the endpoints these match + // nearly always carry one. SplitHostPort succeeds only on host:port, so + // a bare IPv6 address still passes. + if _, _, err := net.SplitHostPort(trimmed); err == nil { + return fmt.Errorf("invalid %s entry %q: expected a bare host, drop the port", knob, e) } } return nil diff --git a/internal/capability/compose_test.go b/internal/capability/compose_test.go index 8200846..4553672 100644 --- a/internal/capability/compose_test.go +++ b/internal/capability/compose_test.go @@ -1070,3 +1070,68 @@ func TestComposeObserver_PanickingCallbackDoesNotBreakTheTurn(t *testing.T) { t.Fatalf("expected the panic to be logged; logs:\n%s", buf.String()) } } + +// A host allow-list entry written as a URL or with a port can never match the +// hostname it is compared against, so composition refuses to start rather than +// leaving the setting silently doing nothing. +func TestComposeHostAllowLists_RejectEntriesThatCouldNeverMatch(t *testing.T) { + doc := mcpEndpointDoc("http://gateway.example/mcp") + + for _, tc := range []struct { + name string + opts ComposeOptions + want string + }{ + { + name: "dial list with a port", + opts: ComposeOptions{AllowedMCPEndpointHosts: []string{"gateway.example:8080"}}, + want: "CAPABILITY_MCP_ENDPOINT_HOSTS", + }, + { + name: "dial list as a URL", + opts: ComposeOptions{AllowedMCPEndpointHosts: []string{"https://gateway.example/mcp"}}, + want: "CAPABILITY_MCP_ENDPOINT_HOSTS", + }, + { + name: "identity list with a port", + opts: ComposeOptions{IdentityForwardHosts: []string{"compute-mcp.compute-system.svc.cluster.local:8080"}}, + want: "CAPABILITY_IDENTITY_FORWARD_HOSTS", + }, + { + name: "identity list as a URL", + opts: ComposeOptions{IdentityForwardHosts: []string{"http://compute-mcp.compute-system.svc.cluster.local/mcp"}}, + want: "CAPABILITY_IDENTITY_FORWARD_HOSTS", + }, + } { + t.Run(tc.name, func(t *testing.T) { + composed, err := Compose(context.Background(), []CapabilityDocument{doc}, tc.opts) + if err == nil { + composed.Close() + t.Fatalf("compose succeeded; want a rejection naming %s", tc.want) + } + if !strings.Contains(err.Error(), tc.want) { + t.Fatalf("error = %q, want it to name %s", err, tc.want) + } + }) + } +} + +// A bare IPv6 address is a legitimate host and must survive the port check. +func TestComposeHostAllowLists_BareIPv6IsAccepted(t *testing.T) { + var dialed []string + session := newFakeSession("streams_list") + doc := mcpEndpointDoc("http://[2001:db8::1]/mcp") + composed, err := Compose(context.Background(), []CapabilityDocument{doc}, ComposeOptions{ + connect: dialRecorder(map[string]*fakeSession{"http://[2001:db8::1]/mcp": session}, &dialed), + AllowedMCPEndpointHosts: []string{"2001:db8::1"}, + IdentityForwardHosts: []string{"2001:db8::1"}, + }) + if err != nil { + t.Fatalf("compose: %v", err) + } + defer composed.Close() + + if len(dialed) != 1 { + t.Fatalf("dialed = %v, want the sanctioned endpoint", dialed) + } +}