diff --git a/authbridge/authlib/listener/skiphost/globcontract_test.go b/authbridge/authlib/listener/skiphost/globcontract_test.go new file mode 100644 index 000000000..cc98e59f6 --- /dev/null +++ b/authbridge/authlib/listener/skiphost/globcontract_test.go @@ -0,0 +1,141 @@ +package skiphost + +import ( + "testing" + + "github.com/gobwas/glob" +) + +// Three packages compile operator-supplied host patterns the same way — +// glob.Compile(pattern, '.') then Match(host): +// +// - listener/skiphost/skiphost.go (bypasses the whole pipeline) +// - routing/router.go (token-exchange routing) +// - plugins/tokenbroker/plugin.go (broker routing) +// +// None of them owns that behaviour; gobwas/glob does. skiphost.New's doc +// comment then makes load-bearing claims about it — that "*" matches every +// single-label host, that "**" is match-all, and that "*.*", +// "*.svc.cluster.local" and "service-*" are safe to accept precisely because +// they are NOT match-all. Those claims decide which patterns the boot-time +// guard rejects, and a wrong one means an operator pattern silently exempts +// traffic from enforcement. +// +// Nothing pinned those claims to the library, so a glob upgrade could quietly +// invalidate them. gobwas/glob v1.0.0 is described by its own release notes as +// "rewrite glob for a much simpler and _correct_ engine", which is exactly the +// kind of change that would. This file pins the contract directly against the +// library so the next bump has to prove it still holds. +// +// Deliberately not covered: the empty host. Match strips it before it reaches +// glob (MatchPattern returns early on host == ""), and it is the only input +// whose result differs between v0.2.3 and v1.0.0 — "?" matched "" in v0.2.3 +// and does not in v1.0.0, and a run of three or more stars flipped the other +// way. Both need a degenerate pattern to reach, and skiphost never asks glob +// about an empty host, so pinning those cells would pin noise. + +// TestGlobContract_SeparatorSemantics pins the single property every host +// pattern in the repo depends on: a lone "*" is confined to one dot-separated +// label, and "**" is not. +func TestGlobContract_SeparatorSemantics(t *testing.T) { + for _, tc := range []struct { + pattern string + host string + want bool + why string + }{ + // "*" stays within one label. + {"*", "otel-collector", true, "single-label host is one label"}, + {"*", "otel-collector.svc", false, "* must not cross a separator"}, + {"*.example.com", "a.example.com", true, "one label before the suffix"}, + {"*.example.com", "a.b.example.com", false, "* must not span two labels"}, + {"*.example.com", "example.com", false, "* requires a label to be present"}, + {"otel-*", "otel-collector", true, "suffix wildcard within a label"}, + {"otel-*", "otel-collector.svc", false, "suffix wildcard stops at separator"}, + + // "**" crosses separators. + {"**", "otel-collector", true, "** matches a single label"}, + {"**", "a.b.c.d.e", true, "** crosses every separator"}, + {"**.svc.cluster.local", "a.b.svc.cluster.local", true, "** spans two labels"}, + + // Fixed label count / fixed suffix keep these from being match-all. + {"*.*", "a.b", true, "exactly two labels"}, + {"*.*", "a", false, "*.* requires a separator"}, + {"*.*", "a.b.c", false, "*.* is pinned to two labels"}, + {"service-*", "service-foo", true, "fixed prefix, one label"}, + {"service-*", "otel-collector", false, "fixed prefix must match"}, + {"service-*", "service-foo.bar", false, "fixed prefix does not cross separator"}, + {"*.svc.cluster.local", "otel.svc.cluster.local", true, "one label plus fixed suffix"}, + {"*.svc.cluster.local", "otel.ns.svc.cluster.local", false, "suffix pattern is label-exact"}, + {"otel-collector.*.svc.cluster.local", "otel-collector.ns.svc.cluster.local", true, "middle wildcard label"}, + {"otel-collector.*.svc.cluster.local", "otel-collector.a.b.svc.cluster.local", false, "middle wildcard is one label"}, + } { + g, err := glob.Compile(tc.pattern, '.') + if err != nil { + t.Fatalf("glob.Compile(%q, '.') err = %v", tc.pattern, err) + } + if got := g.Match(tc.host); got != tc.want { + t.Errorf("glob.Compile(%q, '.').Match(%q) = %t, want %t (%s)", + tc.pattern, tc.host, got, tc.want, tc.why) + } + } +} + +// TestGlobContract_GuardedPatternsAreMatchAll pins the premise behind New's +// rejection of "*" and "**": both would exempt the hosts the listener actually +// sees. "*" is rejected because every short in-cluster service name is a +// single label, which it matches. +func TestGlobContract_GuardedPatternsAreMatchAll(t *testing.T) { + // Short Kubernetes service names — the case New's doc comment names. + singleLabel := []string{"github-tool-mcp", "otel-collector", "x"} + for _, host := range singleLabel { + g, err := glob.Compile("*", '.') + if err != nil { + t.Fatalf(`glob.Compile("*", '.') err = %v`, err) + } + if !g.Match(host) { + t.Errorf(`"*" no longer matches single-label host %q; New rejects "*" on the premise that it does`, host) + } + } + + // "**" must match everything, single-label and FQDN alike. + g, err := glob.Compile("**", '.') + if err != nil { + t.Fatalf(`glob.Compile("**", '.') err = %v`, err) + } + for _, host := range append(singleLabel, + "otel-collector.rossoctl-system.svc.cluster.local", + "api.anthropic.com", + ) { + if !g.Match(host) { + t.Errorf(`"**" no longer matches %q; New rejects "**" as the unambiguous match-all`, host) + } + } +} + +// TestGlobContract_AcceptedPatternsAreNotMatchAll is the converse guard: the +// three patterns New's doc comment explicitly accepts as safe must not match +// an unrelated host. If a future engine widened any of them to match-all, the +// guard would be waving through a full enforcement bypass. +func TestGlobContract_AcceptedPatternsAreNotMatchAll(t *testing.T) { + unrelated := []string{ + "api.anthropic.com", + "github-tool-mcp", + "otel-collector.rossoctl-system.svc.cluster.local", + } + for _, pattern := range []string{"*.*", "*.svc.cluster.local", "service-*"} { + g, err := glob.Compile(pattern, '.') + if err != nil { + t.Fatalf("glob.Compile(%q, '.') err = %v", pattern, err) + } + matched := 0 + for _, host := range unrelated { + if g.Match(host) { + matched++ + } + } + if matched == len(unrelated) { + t.Errorf("pattern %q now matches every sample host; New accepts it on the premise that it is not match-all", pattern) + } + } +}