fix: reject match-all skip_hosts and unreachable routes

authbridge/authlib/internal/hostglob/hostglob.go

@@ -0,0 +1,145 @@
+// Package hostglob is the one place cortex turns an operator-supplied host
+// pattern into a matcher, and the one place that decides what counts as a
+// dangerously broad pattern.
+//
+// Three packages match a destination Host against operator patterns:
+//
+//   - listener/skiphost      — a match bypasses the pipeline AND session recording
+//   - routing                — a match selects token-exchange parameters
+//   - plugins/tokenbroker    — a match selects broker parameters
+//
+// Each of them used to call glob.Compile(pattern, '.') itself and reason about
+// breadth on its own. That duplication is why skiphost's footgun guard could
+// compare against the literal strings "*" and "**" and miss "***", "{**}",
+// "{*,**}" and "?*", and why the other two had no breadth check at all. One
+// definition here means a fix or an upgrade lands once.
+//
+// The package deliberately depends on nothing inside authlib, so every
+// consumer — listener, routing, plugins — can import it without any risk of
+// an import cycle.
+package hostglob
+
+import (
+	"strings"
+
+	"github.com/gobwas/glob"
+)
+
+// Separator is the glob separator for host patterns. With '.', a single "*"
+// is confined to one DNS label ("*.svc.cluster.local" matches
+// "otel.svc.cluster.local" but not "otel.ns.svc.cluster.local") while "**"
+// crosses labels. Every consumer must compile with the same separator or
+// identical patterns would mean different things in different packages.
+const Separator = '.'
+
+// Compile compiles an operator-supplied host pattern.
+func Compile(pattern string) (glob.Glob, error) {
+	return glob.Compile(pattern, Separator)
+}
+
+// singleLabelProbes are short single-label hostnames of varying length. This
+// is the shape in-cluster traffic actually arrives with: the Host header of a
+// request to a Kubernetes Service is usually the bare service name
+// ("Host: github-tool-mcp", "Host: otel-collector"), not an FQDN.
+//
+// A pattern matching every one of these covers every in-cluster destination,
+// which is why bare "*" is dangerous in skip_hosts even though it matches no
+// FQDN at all.
+var singleLabelProbes = []string{
+	"a",
+	"svc",
+	"otel-collector",
+	"github-tool-mcp",
+}
+
+// fqdnProbes are multi-label hostnames: in-cluster FQDNs and a public domain.
+// A pattern matching these as well as every singleLabelProbe is match-all in
+// the strongest sense — no host escapes it.
+var fqdnProbes = []string{
+	"a.b",
+	"otel-collector.rossoctl-system.svc.cluster.local",
+	"api.anthropic.com",
+}
+
+// MatchesEverySingleLabel reports whether g matches every single-label probe,
+// i.e. whether the pattern exempts (or captures) every in-cluster
+// short-service-name destination.
+//
+// This is a behavioural probe rather than a comparison against known-bad
+// strings on purpose. Under '.'-separated globs all of these are equally
+// broad, and a spelling check catches only the ones someone thought of:
+//
+//	"*"                       — every single label
+//	"**", "***", and longer runs of stars
+//	"{**}", "{*,**}"          — super-star in braces or alternation
+//	"?*"                      — one character then anything
+func MatchesEverySingleLabel(g glob.Glob) bool {
+	return matchesAll(g, singleLabelProbes)
+}
+
+// IsMatchAll reports whether g matches every probe host, single-label and
+// FQDN alike — the strongest form of "this pattern matches everything".
+//
+// Callers deciding whether an earlier entry makes a later one unreachable
+// want this rather than MatchesEverySingleLabel: "*" matches every single
+// label but no FQDN, so it shadows a later "github-tool-mcp" while leaving a
+// later "*.svc.cluster.local" perfectly reachable.
+func IsMatchAll(g glob.Glob) bool {
+	return matchesAll(g, singleLabelProbes) && matchesAll(g, fqdnProbes)
+}
+
+func matchesAll(g glob.Glob, hosts []string) bool {
+	for _, h := range hosts {
+		if !g.Match(h) {
+			return false
+		}
+	}
+	return true
+}
+
+// metacharacters are the glob syntax characters. A pattern containing none of
+// them matches exactly one host, which is what lets IsLiteral support exact
+// shadowing detection.
+const metacharacters = `*?[]{}\`
+
+// IsLiteral reports whether pattern contains no glob syntax, i.e. it matches
+// exactly itself and nothing else.
+//
+// This is what makes unreachable-route detection exact for the common case: a
+// literal later route is shadowed by an earlier pattern if and only if that
+// pattern matches the literal. For a later route that is itself a wildcard,
+// no such exact test exists (it would require deciding glob subsumption), so
+// callers fall back to IsMatchAll on the earlier entry.
+func IsLiteral(pattern string) bool {
+	return !strings.ContainsAny(pattern, metacharacters)
+}
+
+// Shadows reports whether an earlier entry in a first-match-wins list makes a
+// later one unreachable, so callers can reject dead configuration at boot
+// rather than silently ignoring it.
+//
+// Both route lists in authlib are first-match-wins, which makes a broad
+// early pattern quietly destructive: a "***" typo at the top of
+// authproxy-routes, or a plain "*", swallows every carefully-configured route
+// beneath it, and if the shadowing route is a passthrough then token exchange
+// is silently off for hosts the operator explicitly listed.
+//
+// Deliberately conservative — it answers yes only when unreachability is
+// certain:
+//
+//   - earlier is match-all, so nothing after it can ever be reached; or
+//   - later is a literal host and earlier already matches that exact host.
+//
+// A later wildcard shadowed by a narrower earlier wildcard (say "*.a.com"
+// after "*.*.com") is not reported. Deciding that in general means deciding
+// glob subsumption, and a false positive here fails the pod at boot — so the
+// check stays sound rather than complete.
+func Shadows(earlier glob.Glob, laterPattern string) bool {
+	if IsMatchAll(earlier) {
+		return true
+	}
+	if IsLiteral(laterPattern) {
+		return earlier.Match(laterPattern)
+	}
+	return false
+}

authbridge/authlib/internal/hostglob/hostglob_test.go

@@ -0,0 +1,158 @@
+package hostglob
+
+import "testing"
+
+func mustCompile(t *testing.T, pattern string) interface{ Match(string) bool } {
+	t.Helper()
+	g, err := Compile(pattern)
+	if err != nil {
+		t.Fatalf("Compile(%q) err = %v", pattern, err)
+	}
+	return g
+}
+
+// TestMatchesEverySingleLabel_BroadSpellings covers what a spelling check
+// misses. skiphost's guard used to compare against the literal strings "*"
+// and "**"; every other way of writing the same breadth slipped through and
+// silently bypassed the pipeline for every host.
+func TestMatchesEverySingleLabel_BroadSpellings(t *testing.T) {
+	for _, p := range []string{
+		"*",      // every single label
+		"**",     // match-all
+		"***",    // runs of three or more stars behave as "**"
+		"****",   //
+		"{**}",   // super-star in braces
+		"{*,**}", // alternation containing a super-star
+		"?*",     // one character then anything
+	} {
+		if !MatchesEverySingleLabel(mustCompile(t, p)) {
+			t.Errorf("MatchesEverySingleLabel(%q) = false, want true", p)
+		}
+	}
+}
+
+// TestMatchesEverySingleLabel_NarrowPatterns is the over-rejection guard.
+// Every pattern here needs a separator or a fixed prefix/suffix, so none
+// covers all in-cluster destinations and all must keep working — a wrongly
+// rejected pattern fails the pod at boot.
+func TestMatchesEverySingleLabel_NarrowPatterns(t *testing.T) {
+	for _, p := range []string{
+		"*.*",
+		"*.svc.cluster.local",
+		"service-*",
+		"otel-*",
+		"otel-collector*",
+		"*-anything",
+		"otel-collector.*.svc.cluster.local",
+		"**.svc.cluster.local",
+		"?",
+		"**.**",
+		"api.anthropic.com",
+	} {
+		if MatchesEverySingleLabel(mustCompile(t, p)) {
+			t.Errorf("MatchesEverySingleLabel(%q) = true, want false", p)
+		}
+	}
+}
+
+// TestIsMatchAll_DistinguishesSingleLabelFromTotal pins the difference
+// between the two predicates. A bare "*" covers every single label but no
+// FQDN, so it is broad enough to reject in skip_hosts yet does not make a
+// later FQDN route unreachable.
+func TestIsMatchAll_DistinguishesSingleLabelFromTotal(t *testing.T) {
+	star := mustCompile(t, "*")
+	if !MatchesEverySingleLabel(star) {
+		t.Error(`MatchesEverySingleLabel("*") = false, want true`)
+	}
+	if IsMatchAll(star) {
+		t.Error(`IsMatchAll("*") = true, want false ("*" matches no FQDN)`)
+	}
+
+	for _, p := range []string{"**", "***", "{**}", "{*,**}"} {
+		if !IsMatchAll(mustCompile(t, p)) {
+			t.Errorf("IsMatchAll(%q) = false, want true", p)
+		}
+	}
+	for _, p := range []string{"*.*", "*.svc.cluster.local", "service-*", "**.svc.cluster.local"} {
+		if IsMatchAll(mustCompile(t, p)) {
+			t.Errorf("IsMatchAll(%q) = true, want false", p)
+		}
+	}
+}
+
+func TestIsLiteral(t *testing.T) {
+	for _, tc := range []struct {
+		pattern string
+		want    bool
+	}{
+		{"api.example.com", true},
+		{"github-tool-mcp", true},
+		{"", true},
+		{"*", false},
+		{"*.example.com", false},
+		{"?", false},
+		{"{a,b}", false},
+		{"[a-z]", false},
+		{`a\*b`, false},
+	} {
+		if got := IsLiteral(tc.pattern); got != tc.want {
+			t.Errorf("IsLiteral(%q) = %t, want %t", tc.pattern, got, tc.want)
+		}
+	}
+}
+
+// TestShadows covers the first-match-wins hazard: an earlier entry that makes
+// a later one unreachable is dead configuration, and both route lists reject
+// it at boot rather than silently ignoring the later entry.
+func TestShadows(t *testing.T) {
+	for _, tc := range []struct {
+		earlier string
+		later   string
+		want    bool
+		why     string
+	}{
+		// A total match-all shadows anything.
+		{"**", "api.example.com", true, "match-all shadows every later route"},
+		{"***", "*.example.com", true, "match-all shadows later wildcards too"},
+		{"{*,**}", "github-tool-mcp", true, "brace alternation match-all"},
+
+		// Exact coverage of a later literal.
+		{"api.example.com", "api.example.com", true, "duplicate host"},
+		{"*.example.com", "api.example.com", true, "wildcard covers the later literal"},
+		{"*", "github-tool-mcp", true, "single-label wildcard covers a short service name"},
+
+		// Not shadowed: "*" is confined to one label.
+		{"*", "api.example.com", false, `"*" matches no FQDN`},
+		{"*.example.com", "api.sub.example.com", false, "single label does not span two"},
+
+		// Not shadowed: unrelated patterns.
+		{"otel-*", "github-tool-mcp", false, "prefix does not match"},
+		{"*.metrics.local", "api.example.com", false, "different suffix"},
+
+		// Deliberately not reported: later is a wildcard and earlier is not
+		// match-all. Deciding glob subsumption in general is out of scope, and
+		// a false positive would fail the pod at boot.
+		{"*.example.com", "api.example.com*", false, "sound, not complete"},
+	} {
+		g := mustCompile(t, tc.earlier)
+		if got := Shadows(g, tc.later); got != tc.want {
+			t.Errorf("Shadows(%q, %q) = %t, want %t (%s)", tc.earlier, tc.later, got, tc.want, tc.why)
+		}
+	}
+}
+
+// TestCompile_Separator pins the separator every consumer must share. If this
+// drifts, identical patterns would mean different things in skiphost, routing
+// and tokenbroker.
+func TestCompile_Separator(t *testing.T) {
+	if Separator != '.' {
+		t.Fatalf("Separator = %q, want '.'", Separator)
+	}
+	g := mustCompile(t, "*.example.com")
+	if !g.Match("api.example.com") {
+		t.Error("one label before the suffix must match")
+	}
+	if g.Match("api.sub.example.com") {
+		t.Error("* must not cross a separator")
+	}
+}

authbridge/authlib/listener/skiphost/skiphost.go

@@ -9,10 +9,11 @@
 // before matching so operators write patterns against the hostname alone
 // regardless of which port the upstream listens on.
 //
-// The package is a leaf — no dependencies inside authlib — so both
-// listener implementations (extproc, forwardproxy) can import it without
-// risking an import cycle, and tests can exercise the matcher in
-// isolation from the listener machinery.
+// The package depends on nothing inside authlib except
+// authlib/internal/hostglob, itself a leaf, so both listener
+// implementations (extproc, forwardproxy) can import it without risking an
+// import cycle, and tests can exercise the matcher in isolation from the
+// listener machinery.
 package skiphost
 
 import (
@@ -21,6 +22,8 @@ import (
 	"strings"
 
 	"github.com/gobwas/glob"
+
+	"github.com/rossoctl/cortex/authbridge/authlib/internal/hostglob"
 )
 
 // Matcher answers "does this host match any configured skip pattern?".
@@ -45,12 +48,14 @@ type compiled struct {
 //
 //   - empty / whitespace-only patterns: trivially-true matches with no
 //     intent expressed.
-//   - "*" — under our `.`-delimited glob semantics, matches every
-//     single-label hostname, which is how every short Kubernetes
-//     service name reaches the listener (`Host: github-tool-mcp`,
-//     `Host: otel-collector`, etc.). One wildcard would silently
-//     exempt every in-cluster outbound from IBAC + token-exchange.
-//   - "**" — the unambiguous match-all under gobwas/glob.
+//   - any pattern hostglob.MatchesEverySingleLabel flags — i.e. anything
+//     that covers every in-cluster destination. Under our `.`-delimited
+//     glob semantics that includes "*", which matches every single-label
+//     hostname, which is how every short Kubernetes service name reaches
+//     the listener (`Host: github-tool-mcp`, `Host: otel-collector`,
+//     etc.); "**", the unambiguous match-all; and every other spelling of
+//     the same thing (see hostglob for why this is a behavioural probe
+//     rather than a comparison against known-bad strings).
 //   - patterns containing ":" — Match strips the port from the
 //     incoming host before comparing, so colon-bearing patterns
 //     compile but never match. Almost certainly an operator typo.
@@ -72,20 +77,20 @@ func New(patterns []string) (*Matcher, error) {
 		if trimmed == "" {
 			return nil, fmt.Errorf("skiphost: empty pattern in skip_hosts list")
 		}
-		if trimmed == "*" || trimmed == "**" {
-			return nil, fmt.Errorf("skiphost: pattern %q matches everything; "+
-				"if you mean to disable outbound enforcement, remove the "+
-				"relevant plugins from the pipeline instead", p)
-		}
 		if strings.Contains(p, ":") {
 			return nil, fmt.Errorf("skiphost: pattern %q must not contain a port "+
 				"(Match strips the port from the incoming host before comparing, "+
 				"so a port-bearing pattern would never match)", p)
 		}
-		g, err := glob.Compile(p, '.')
+		g, err := hostglob.Compile(p)
 		if err != nil {
 			return nil, fmt.Errorf("skiphost: invalid pattern %q: %w", p, err)
 		}
+		if hostglob.MatchesEverySingleLabel(g) {
+			return nil, fmt.Errorf("skiphost: pattern %q matches everything; "+
+				"if you mean to disable outbound enforcement, remove the "+
+				"relevant plugins from the pipeline instead", p)
+		}
 		out = append(out, compiled{raw: p, glob: g})
 	}
 	return &Matcher{patterns: out}, nil