From 16ca25a45f21551349615d67421baef180e5b191 Mon Sep 17 00:00:00 2001 From: Hai Huang Date: Tue, 1 Sep 2026 09:02:32 -0400 Subject: [PATCH 1/2] fix: reject every spelling of match-all in skip_hosts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit New's guard existed to stop an operator pattern from silently disabling outbound enforcement — skip_hosts bypasses the plugin pipeline AND session recording for matched traffic — but it identified match-all patterns by string equality: if trimmed == "*" || trimmed == "**" { reject } That caught the two obvious spellings and nothing else. Under `.`-delimited gobwas/glob these are all equally match-all and all sailed through: "***", "****", and any longer run of stars "{**}", "{*,**}" — super-star wrapped in braces or alternation "?*" — one character then anything, i.e. any non-empty label Any one of them in listener.skip_hosts exempted every outbound host from IBAC and token-exchange, which is exactly the outcome the guard's doc comment says it prevents. Confirmed present under both glob v0.2.3 and v1.0.0, so this is not a consequence of any pending bump. Replaces the string comparison with a behavioural probe: compile the pattern, then reject it if it matches every host in matchAllProbeHosts — short single-label Kubernetes service names, the shape that made bare "*" dangerous in the first place. A pattern that matches all of those is match-all in practice however it is written. Single-label probes suffice: anything matching everything matches these too, and a pattern needing a separator or a fixed prefix/suffix ("*.*", "*.svc.cluster.local", "service-*") is untouched. Over-rejection is the failure mode that matters here — a rejected pattern fails the pod at boot — so TestNew_AcceptsNonMatchAllWildcards pins twelve operator-plausible wildcards as accepted, alongside the existing TestNew_AcceptsLeadingStar. The match-all check necessarily moves after glob.Compile now, since it needs a compiled pattern. Error text and the empty/whitespace and port guards are unchanged. Not addressed here: routing/router.go and plugins/tokenbroker/plugin.go compile host patterns the same way and have no match-all guard at all. A match-all there redirects rather than bypasses, so it is a different (and milder) question — left as follow-up. Assisted-By: Claude (Anthropic AI) Signed-off-by: Hai Huang --- .../authlib/listener/skiphost/skiphost.go | 64 ++++++++++++++++--- .../listener/skiphost/skiphost_test.go | 49 ++++++++++++++ 2 files changed, 103 insertions(+), 10 deletions(-) diff --git a/authbridge/authlib/listener/skiphost/skiphost.go b/authbridge/authlib/listener/skiphost/skiphost.go index a1d604964..cd48d143b 100644 --- a/authbridge/authlib/listener/skiphost/skiphost.go +++ b/authbridge/authlib/listener/skiphost/skiphost.go @@ -34,6 +34,47 @@ type compiled struct { glob glob.Glob } +// matchAllProbeHosts are single-label hostnames covering the shapes real +// in-cluster traffic arrives with: short Kubernetes service names of +// varying length. A pattern that matches every one of them exempts every +// in-cluster outbound destination from the pipeline, which is the outcome +// New's guard exists to prevent. +// +// This is a behavioural probe rather than a comparison against known-bad +// strings, because the string comparison it replaces ("*" and "**" by +// equality) only caught the two most obvious spellings. Under +// `.`-delimited gobwas/glob these are all equally match-all and all +// slipped through: +// +// "***", "****", and any longer run of stars +// "{**}", "{*,**}" — super-star wrapped in braces or alternation +// "?*" — one character then anything, i.e. any non-empty label +// +// Any of those in listener.skip_hosts silently bypassed the plugin +// pipeline AND session recording for every host. +// +// Single-label probes are sufficient: a pattern that matches everything +// necessarily matches these too, and a pattern that requires a separator +// ("*.*", "*.svc.cluster.local") is not match-all and must keep working — +// TestNew_AcceptsLeadingStar pins that direction. +var matchAllProbeHosts = []string{ + "a", + "svc", + "otel-collector", + "github-tool-mcp", +} + +// matchesEveryProbeHost reports whether g matches every probe host, i.e. +// whether the pattern is match-all in practice however it is spelled. +func matchesEveryProbeHost(g glob.Glob) bool { + for _, h := range matchAllProbeHosts { + if !g.Match(h) { + return false + } + } + return true +} + // New compiles a skip-host matcher from raw glob patterns. Returns an // error identifying the first invalid pattern so misconfigurations // surface at startup rather than at first request. An empty input is @@ -45,12 +86,15 @@ 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 +// - any pattern that matches every probe host in matchAllProbeHosts — +// i.e. anything that is match-all in practice. 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.). One wildcard would silently -// exempt every in-cluster outbound from IBAC + token-exchange. -// - "**" — the unambiguous match-all under gobwas/glob. +// `Host: otel-collector`, etc.); "**", the unambiguous match-all; +// and every other spelling of the same thing (see +// matchAllProbeHosts 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,11 +116,6 @@ 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, "+ @@ -86,6 +125,11 @@ func New(patterns []string) (*Matcher, error) { if err != nil { return nil, fmt.Errorf("skiphost: invalid pattern %q: %w", p, err) } + if matchesEveryProbeHost(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 diff --git a/authbridge/authlib/listener/skiphost/skiphost_test.go b/authbridge/authlib/listener/skiphost/skiphost_test.go index 29efe1307..8ac3f1015 100644 --- a/authbridge/authlib/listener/skiphost/skiphost_test.go +++ b/authbridge/authlib/listener/skiphost/skiphost_test.go @@ -134,3 +134,52 @@ func TestMatch_NoMatch(t *testing.T) { t.Error("Match returned true for an unrelated host") } } + +// TestNew_RejectsMatchAllSpellings covers the spellings that the previous +// guard let through. It compared patterns against the literal strings "*" +// and "**", so every other way of writing match-all was accepted and +// silently bypassed the plugin pipeline AND session recording for every +// host — the exact outcome the guard exists to prevent. +// +// Each of these matches every hostname the listener sees under +// `.`-delimited gobwas/glob. +func TestNew_RejectsMatchAllSpellings(t *testing.T) { + for _, p := range []string{ + "***", // runs of three or more stars behave as "**" + "****", // + "{**}", // super-star wrapped in braces + "{*,**}", // alternation containing a super-star + "?*", // one character then anything = any non-empty label + } { + if _, err := New([]string{p}); err == nil { + t.Errorf("New([%q]) returned nil error; pattern is match-all and must be rejected at boot", p) + } + } +} + +// TestNew_AcceptsNonMatchAllWildcards is the over-rejection guard for the +// behavioural probe. Everything here is a wildcard pattern an operator +// would plausibly write, and each requires either a separator or a fixed +// prefix/suffix, so none is match-all. If the probe ever starts rejecting +// these, it has become a footgun of its own — a bad pattern fails the pod +// at boot. +func TestNew_AcceptsNonMatchAllWildcards(t *testing.T) { + for _, p := range []string{ + "*.*", + "*.svc.cluster.local", + "service-*", + "*-anything", + "*.something", + "otel-*", + "otel-collector*", + "otel-collector.*.svc.cluster.local", + "*.rossoctl-system.svc.cluster.local", + "**.svc.cluster.local", + "?", + "**.**", + } { + if _, err := New([]string{p}); err != nil { + t.Errorf("New([%q]) returned err = %v; pattern is not match-all and must be accepted", p, err) + } + } +} From fc5e9f4403b2cae686aba625c58ef5352acab395 Mon Sep 17 00:00:00 2001 From: Hai Huang Date: Tue, 1 Sep 2026 09:26:00 -0400 Subject: [PATCH 2/2] fix: reject unreachable routes in routing and tokenbroker MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extends the skip_hosts guard to the other two consumers of host patterns, and gives all three a single owner for the behaviour. authlib/internal/hostglob is now the one place cortex compiles an operator-supplied host pattern and decides whether one is dangerously broad. Three packages each did this themselves — listener/skiphost, routing and plugins/tokenbroker — which is precisely why skiphost's guard could compare against the literal strings "*" and "**" and miss "***", "{**}", "{*,**}" and "?*", and why the other two had no breadth check at all. It also means the eventual glob v1 migration (blocked on OPA, see #829) lands in one file instead of three. The routers deliberately do NOT copy skip_hosts' blanket rejection. A match-all there is not a bypass: as the final route it is a legitimate explicit catch-all, equivalent to defaultAction. The hazard is first-match-wins — a broad early pattern silently swallows every route beneath it, and if that early route is a passthrough then token exchange or brokering is off for hosts the operator explicitly listed. So both routers now reject unreachable routes instead. hostglob.Shadows is sound rather than complete: it reports only certain unreachability — the earlier pattern is match-all, or the later pattern is a literal the earlier one already matches. A later wildcard shadowed by a narrower earlier wildcard is not reported, because deciding glob subsumption in general risks a false positive, and a false positive fails the pod at boot. Both routers also now reject an empty host pattern (a route whose `host:` key is missing from routes.yaml matches only an empty Host header, never real traffic), and both take the no-route path for an empty host rather than offering it to the patterns — a bare "*" matches the empty string under gobwas/glob, so an unset Host header would otherwise select a "*" route and mint or broker a token for a destination we cannot identify. OPERATIONAL NOTE: the production router is built from the authproxy-routes ConfigMap, so a live config containing a duplicate or shadowed route will now fail the pod at boot instead of silently ignoring that route. Traffic behaviour is unchanged — a shadowed route was already dead — so this trades availability on upgrade for a loud signal. Downgrading the shadowing check to a warning is a one-line change if that trade is not wanted. TestResolve_FirstMatchWins configured the host "service" twice, which is dead config and now rejected; it was rewritten to prove the same property with two genuinely overlapping patterns ("svc-*" and "*-prod") where neither route is unreachable. Assisted-By: Claude (Anthropic AI) Signed-off-by: Hai Huang --- .../authlib/internal/hostglob/hostglob.go | 145 ++++++++++++++++ .../internal/hostglob/hostglob_test.go | 158 ++++++++++++++++++ .../authlib/listener/skiphost/skiphost.go | 73 ++------ .../authlib/plugins/tokenbroker/plugin.go | 42 ++++- .../plugins/tokenbroker/router_guard_test.go | 95 +++++++++++ authbridge/authlib/routing/router.go | 57 ++++++- authbridge/authlib/routing/router_test.go | 131 ++++++++++++++- 7 files changed, 632 insertions(+), 69 deletions(-) create mode 100644 authbridge/authlib/internal/hostglob/hostglob.go create mode 100644 authbridge/authlib/internal/hostglob/hostglob_test.go create mode 100644 authbridge/authlib/plugins/tokenbroker/router_guard_test.go diff --git a/authbridge/authlib/internal/hostglob/hostglob.go b/authbridge/authlib/internal/hostglob/hostglob.go new file mode 100644 index 000000000..9d85ce757 --- /dev/null +++ b/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 +} diff --git a/authbridge/authlib/internal/hostglob/hostglob_test.go b/authbridge/authlib/internal/hostglob/hostglob_test.go new file mode 100644 index 000000000..8f2a1bcee --- /dev/null +++ b/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") + } +} diff --git a/authbridge/authlib/listener/skiphost/skiphost.go b/authbridge/authlib/listener/skiphost/skiphost.go index cd48d143b..458c7e49b 100644 --- a/authbridge/authlib/listener/skiphost/skiphost.go +++ b/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?". @@ -34,47 +37,6 @@ type compiled struct { glob glob.Glob } -// matchAllProbeHosts are single-label hostnames covering the shapes real -// in-cluster traffic arrives with: short Kubernetes service names of -// varying length. A pattern that matches every one of them exempts every -// in-cluster outbound destination from the pipeline, which is the outcome -// New's guard exists to prevent. -// -// This is a behavioural probe rather than a comparison against known-bad -// strings, because the string comparison it replaces ("*" and "**" by -// equality) only caught the two most obvious spellings. Under -// `.`-delimited gobwas/glob these are all equally match-all and all -// slipped through: -// -// "***", "****", and any longer run of stars -// "{**}", "{*,**}" — super-star wrapped in braces or alternation -// "?*" — one character then anything, i.e. any non-empty label -// -// Any of those in listener.skip_hosts silently bypassed the plugin -// pipeline AND session recording for every host. -// -// Single-label probes are sufficient: a pattern that matches everything -// necessarily matches these too, and a pattern that requires a separator -// ("*.*", "*.svc.cluster.local") is not match-all and must keep working — -// TestNew_AcceptsLeadingStar pins that direction. -var matchAllProbeHosts = []string{ - "a", - "svc", - "otel-collector", - "github-tool-mcp", -} - -// matchesEveryProbeHost reports whether g matches every probe host, i.e. -// whether the pattern is match-all in practice however it is spelled. -func matchesEveryProbeHost(g glob.Glob) bool { - for _, h := range matchAllProbeHosts { - if !g.Match(h) { - return false - } - } - return true -} - // New compiles a skip-host matcher from raw glob patterns. Returns an // error identifying the first invalid pattern so misconfigurations // surface at startup rather than at first request. An empty input is @@ -86,15 +48,14 @@ func matchesEveryProbeHost(g glob.Glob) bool { // // - empty / whitespace-only patterns: trivially-true matches with no // intent expressed. -// - any pattern that matches every probe host in matchAllProbeHosts — -// i.e. anything that is match-all in practice. 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 -// matchAllProbeHosts for why this is a behavioural probe rather -// than a comparison against known-bad strings). +// - 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. @@ -121,11 +82,11 @@ func New(patterns []string) (*Matcher, error) { "(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 matchesEveryProbeHost(g) { + 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) diff --git a/authbridge/authlib/plugins/tokenbroker/plugin.go b/authbridge/authlib/plugins/tokenbroker/plugin.go index ae1364d5e..6b6fd884c 100644 --- a/authbridge/authlib/plugins/tokenbroker/plugin.go +++ b/authbridge/authlib/plugins/tokenbroker/plugin.go @@ -14,6 +14,7 @@ import ( "github.com/gobwas/glob" "github.com/rossoctl/cortex/authbridge/authlib/auth" + "github.com/rossoctl/cortex/authbridge/authlib/internal/hostglob" "github.com/rossoctl/cortex/authbridge/authlib/pipeline" "github.com/rossoctl/cortex/authbridge/authlib/plugins" "github.com/rossoctl/cortex/authbridge/authlib/plugins/tokenbroker/client" @@ -93,15 +94,30 @@ type compiledBrokerRoute struct { // newBrokerRouter creates a router from the given routes. // defaultAction is "broker" or "passthrough" (applied when no route matches). -// Returns an error if any host pattern is invalid. +// +// Rejects configuration that cannot mean what it appears to say, matching +// authlib/routing: +// +// - an empty or whitespace-only host pattern, which matches only an empty +// Host header and so never matches real traffic. +// - a route made unreachable by an earlier one. Resolution is +// first-match-wins, so a broad early pattern swallows every route beneath +// it; if that early route is a passthrough, brokering is silently off for +// hosts the operator explicitly listed. +// +// A match-all as the final route is accepted — nothing follows it to shadow, +// so it is a legitimate explicit catch-all. func newBrokerRouter(defaultAction string, rules []tokenBrokerRoute) (*brokerRouter, error) { if defaultAction == "" { defaultAction = "passthrough" } compiled := make([]compiledBrokerRoute, 0, len(rules)) - for _, r := range rules { - // Use '.' as separator so *.example.com doesn't match foo.bar.example.com - g, err := glob.Compile(r.Host, '.') + for i, r := range rules { + if strings.TrimSpace(r.Host) == "" { + return nil, fmt.Errorf("route %d has an empty host pattern; "+ + "it would match only an empty Host header, never real traffic", i) + } + g, err := hostglob.Compile(r.Host) if err != nil { return nil, fmt.Errorf("invalid route pattern %q: %w", r.Host, err) } @@ -117,6 +133,16 @@ func newBrokerRouter(defaultAction string, rules []tokenBrokerRoute) (*brokerRou tokenEndpoint: r.TokenEndpoint, }) } + for j := range compiled { + for i := 0; i < j; i++ { + if hostglob.Shadows(compiled[i].glob, compiled[j].pattern) { + return nil, fmt.Errorf("route %d (%q) is unreachable: earlier route %d (%q) "+ + "already matches every host it would match, and resolution is "+ + "first-match-wins; reorder them or narrow the earlier pattern", + j, compiled[j].pattern, i, compiled[i].pattern) + } + } + } return &brokerRouter{routes: compiled, defaultAction: defaultAction}, nil } @@ -124,11 +150,19 @@ func newBrokerRouter(defaultAction string, rules []tokenBrokerRoute) (*brokerRou // Port is stripped from the host before matching. // Returns (shouldBroker, authorizationEndpoint, tokenEndpoint) where shouldBroker is true if a route matches with action "broker" // or if no route matches and default is "broker". +// An empty host takes the no-route path rather than being offered to the +// patterns: a bare "*" matches the empty string under gobwas/glob, so an +// unset Host header would otherwise select a "*" route and broker a token for +// a destination we cannot identify. Mirrors the empty-host defence in +// listener/skiphost and authlib/routing. func (r *brokerRouter) resolve(host string) (bool, string, string) { // Strip port if present if h, _, err := net.SplitHostPort(host); err == nil { host = h } + if host == "" { + return r.defaultAction == "broker", "", "" + } // Check for matching route for _, entry := range r.routes { diff --git a/authbridge/authlib/plugins/tokenbroker/router_guard_test.go b/authbridge/authlib/plugins/tokenbroker/router_guard_test.go new file mode 100644 index 000000000..343dbce5c --- /dev/null +++ b/authbridge/authlib/plugins/tokenbroker/router_guard_test.go @@ -0,0 +1,95 @@ +package tokenbroker + +import "testing" + +// newBrokerRouter rejects configuration that cannot mean what it says, so an +// operator mistake surfaces at boot instead of as traffic quietly skipping the +// broker. Mirrors the guards in authlib/routing. + +func TestNewBrokerRouter_RejectsEmptyHost(t *testing.T) { + if _, err := newBrokerRouter("passthrough", []tokenBrokerRoute{ + {Host: "", Action: "broker"}, + }); err == nil { + t.Error("expected error: an empty host pattern matches only an empty Host header, never real traffic") + } +} + +// TestNewBrokerRouter_RejectsShadowedRoute covers the first-match-wins +// hazard. A broad early pattern swallows every route beneath it, and if that +// early route is a passthrough then brokering is silently off for hosts the +// operator explicitly listed. +func TestNewBrokerRouter_RejectsShadowedRoute(t *testing.T) { + for _, tc := range []struct { + shadowing string + shadowed string + }{ + {"**", "api.example.com"}, + {"***", "api.example.com"}, + {"{**}", "api.example.com"}, + {"{*,**}", "api.example.com"}, + {"*", "internal-svc"}, // "*" covers a short service name, not an FQDN + {"api.example.com", "api.example.com"}, + {"*.example.com", "api.example.com"}, + } { + _, err := newBrokerRouter("passthrough", []tokenBrokerRoute{ + {Host: tc.shadowing, Action: "passthrough"}, + {Host: tc.shadowed, Action: "broker"}, + }) + if err == nil { + t.Errorf("expected error: leading route %q makes the %q route unreachable", + tc.shadowing, tc.shadowed) + } + } +} + +// TestNewBrokerRouter_AcceptsReachableRoutes is the over-rejection guard: a +// wrongly rejected route list fails the pod at boot, so patterns that are +// genuinely all reachable must be accepted — including a match-all in the +// final position, where nothing follows it to shadow. +func TestNewBrokerRouter_AcceptsReachableRoutes(t *testing.T) { + for _, rules := range [][]tokenBrokerRoute{ + {{Host: "api.example.com", Action: "broker"}, {Host: "other.example.com", Action: "passthrough"}}, + {{Host: "*", Action: "broker"}, {Host: "api.example.com", Action: "broker"}}, + {{Host: "svc-*", Action: "broker"}, {Host: "*-prod", Action: "passthrough"}}, + {{Host: "api.example.com", Action: "broker"}, {Host: "**", Action: "passthrough"}}, + {{Host: "*.metrics.local", Action: "passthrough"}, {Host: "*.svc.cluster.local", Action: "broker"}}, + } { + if _, err := newBrokerRouter("passthrough", rules); err != nil { + t.Errorf("newBrokerRouter(%+v) returned err = %v; all routes are reachable", rules, err) + } + } +} + +// TestBrokerRouter_ResolveEmptyHost pins the empty-host defence. A bare "*" +// matches the empty string under gobwas/glob, so without the guard an unset +// Host header would select this route and broker a token for a destination we +// cannot identify. +func TestBrokerRouter_ResolveEmptyHost(t *testing.T) { + r, err := newBrokerRouter("passthrough", []tokenBrokerRoute{ + {Host: "*", Action: "broker", TokenEndpoint: "https://should-not-be-used"}, + }) + if err != nil { + t.Fatal(err) + } + shouldBroker, authEndpoint, tokenEndpoint := r.resolve("") + if shouldBroker { + t.Error("empty host must not broker; it must fall through to defaultAction") + } + if authEndpoint != "" || tokenEndpoint != "" { + t.Errorf("empty host must return no endpoints, got auth=%q token=%q", authEndpoint, tokenEndpoint) + } +} + +// TestBrokerRouter_ResolveEmptyHost_DefaultBroker confirms the empty host +// takes the defaultAction path rather than being hardcoded to passthrough. +func TestBrokerRouter_ResolveEmptyHost_DefaultBroker(t *testing.T) { + r, err := newBrokerRouter("broker", []tokenBrokerRoute{ + {Host: "api.example.com", Action: "passthrough"}, + }) + if err != nil { + t.Fatal(err) + } + if shouldBroker, _, _ := r.resolve(""); !shouldBroker { + t.Error("empty host must follow defaultAction=broker, not a hardcoded passthrough") + } +} diff --git a/authbridge/authlib/routing/router.go b/authbridge/authlib/routing/router.go index 980e6d17c..775eb4871 100644 --- a/authbridge/authlib/routing/router.go +++ b/authbridge/authlib/routing/router.go @@ -5,8 +5,11 @@ package routing import ( "fmt" "net" + "strings" "github.com/gobwas/glob" + + "github.com/rossoctl/cortex/authbridge/authlib/internal/hostglob" ) // Route defines token exchange parameters for requests to a matching host. @@ -20,7 +23,7 @@ type Route struct { // ResolvedRoute is the result of resolving a host against the router. type ResolvedRoute struct { - Matched bool // true if a configured route matched; false for default action fallback + Matched bool // true if a configured route matched; false for default action fallback Audience string Scopes string TokenEndpoint string @@ -42,15 +45,36 @@ type Router struct { // NewRouter creates a router from the given routes. // defaultAction is "exchange" or "passthrough" (applied when no route matches). -// Returns an error if any host pattern is invalid. +// +// Rejects configuration that cannot do what it appears to say, so the +// mistake surfaces at boot instead of as traffic quietly taking the wrong +// path: +// +// - an empty or whitespace-only host pattern. A route whose `host:` key is +// missing from routes.yaml compiles into a pattern that matches only the +// empty Host header, i.e. never matches real traffic. +// - a route made unreachable by an earlier one. Resolution is +// first-match-wins, so a broad early pattern — a "***" typo, or a plain +// "*", which matches every short in-cluster service name — swallows every +// route beneath it. If the shadowing route is a passthrough, token +// exchange is silently off for hosts the operator explicitly listed. +// +// A match-all pattern is NOT rejected outright the way skip_hosts rejects +// one: as the last route it is a legitimate catch-all, equivalent to +// defaultAction but stated explicitly. Only shadowing is an error, which +// means this check can never reject a config that was working — a shadowed +// route was already dead. func NewRouter(defaultAction string, rules []Route) (*Router, error) { if defaultAction == "" { defaultAction = "passthrough" } compiled := make([]compiledRoute, 0, len(rules)) - for _, r := range rules { - // Use '.' as separator so *.example.com doesn't match foo.bar.example.com - g, err := glob.Compile(r.Host, '.') + for i, r := range rules { + if strings.TrimSpace(r.Host) == "" { + return nil, fmt.Errorf("route %d has an empty host pattern; "+ + "it would match only an empty Host header, never real traffic", i) + } + g, err := hostglob.Compile(r.Host) if err != nil { return nil, fmt.Errorf("invalid route pattern %q: %w", r.Host, err) } @@ -60,16 +84,39 @@ func NewRouter(defaultAction string, rules []Route) (*Router, error) { route: r, }) } + for j := range compiled { + for i := 0; i < j; i++ { + if hostglob.Shadows(compiled[i].glob, compiled[j].pattern) { + return nil, fmt.Errorf("route %d (%q) is unreachable: earlier route %d (%q) "+ + "already matches every host it would match, and resolution is "+ + "first-match-wins; reorder them or narrow the earlier pattern", + j, compiled[j].pattern, i, compiled[i].pattern) + } + } + } return &Router{routes: compiled, defaultAction: defaultAction}, nil } // Resolve returns the exchange configuration for the given host. // Returns nil if no route matches and the default action is "passthrough". // Port is stripped from the host before matching. +// +// An empty host takes the no-route path rather than being offered to the +// patterns. A bare "*" matches the empty string under gobwas/glob, so an +// unset Host header would otherwise select a "*" route and mint a token for +// a destination we cannot identify. Falling through to defaultAction is the +// safer reading of an unidentifiable request, and mirrors the empty-host +// defence in listener/skiphost. func (r *Router) Resolve(host string) *ResolvedRoute { if h, _, err := net.SplitHostPort(host); err == nil { host = h } + if host == "" { + if r.defaultAction == "exchange" { + return &ResolvedRoute{Matched: false} + } + return nil + } for _, entry := range r.routes { if entry.glob.Match(host) { action := entry.route.Action diff --git a/authbridge/authlib/routing/router_test.go b/authbridge/authlib/routing/router_test.go index ca5747b4f..e917ded1a 100644 --- a/authbridge/authlib/routing/router_test.go +++ b/authbridge/authlib/routing/router_test.go @@ -53,13 +53,136 @@ func TestResolve_PortStripping(t *testing.T) { } func TestResolve_FirstMatchWins(t *testing.T) { - r, _ := NewRouter("passthrough", []Route{ + // Two patterns that genuinely overlap without either being dead: + // "svc-*" covers svc-prod and svc-dev, "*-prod" covers svc-prod and + // api-prod, and only svc-prod hits both. Neither is unreachable, so + // NewRouter accepts the pair. + // + // This used to configure the host "service" twice, which is dead config + // — the second route can never fire — and NewRouter now rejects it. The + // property under test is unchanged; only the fixture is honest about + // being reachable. + r, err := NewRouter("passthrough", []Route{ + {Host: "svc-*", Audience: "first"}, + {Host: "*-prod", Audience: "second"}, + }) + if err != nil { + t.Fatal(err) + } + resolved := r.Resolve("svc-prod") + if resolved == nil || resolved.Audience != "first" { + t.Error("expected first-match-wins for a host both patterns match") + } + if resolved := r.Resolve("api-prod"); resolved == nil || resolved.Audience != "second" { + t.Error("second route must stay reachable for hosts the first does not match") + } +} + +func TestNewRouter_RejectsDuplicateHost(t *testing.T) { + _, err := NewRouter("passthrough", []Route{ {Host: "service", Audience: "first"}, {Host: "service", Audience: "second"}, }) - resolved := r.Resolve("service") - if resolved == nil || resolved.Audience != "first" { - t.Error("expected first-match-wins") + if err == nil { + t.Error("expected error: the second route repeats a host and can never fire") + } +} + +// TestNewRouter_RejectsShadowedRoute covers the hazard the unreachable-route +// check exists for: resolution is first-match-wins, so a broad early pattern +// silently swallows everything configured beneath it. A "***" typo or a bare +// "*" at the top of authproxy-routes disables every route below — and if the +// shadowing route is a passthrough, token exchange is off for hosts the +// operator explicitly listed. +func TestNewRouter_RejectsShadowedRoute(t *testing.T) { + for _, tc := range []struct { + shadowing string + shadowed string + }{ + // Total match-all: nothing after it is reachable, FQDN or not. + {"**", "api.example.com"}, + {"***", "api.example.com"}, + {"{**}", "api.example.com"}, + {"{*,**}", "api.example.com"}, + // A bare "*" is confined to one label, so it shadows a short + // in-cluster service name but NOT an FQDN — see + // TestNewRouter_AcceptsSingleStarBeforeFQDN. + {"*", "github-tool-mcp"}, + // An earlier pattern that simply covers the later literal. + {"api.example.com", "api.example.com"}, + {"*.example.com", "api.example.com"}, + } { + _, err := NewRouter("passthrough", []Route{ + {Host: tc.shadowing, Action: "passthrough"}, + {Host: tc.shadowed, Audience: "shadowed"}, + }) + if err == nil { + t.Errorf("expected error: leading route %q makes the %q route unreachable", + tc.shadowing, tc.shadowed) + } + } +} + +// TestNewRouter_AcceptsSingleStarBeforeFQDN pins the precision of the check. +// With '.' as the separator a bare "*" matches one label only, so an FQDN +// route after it is genuinely still reachable and must not be rejected — +// over-rejection here would fail the pod at boot. +func TestNewRouter_AcceptsSingleStarBeforeFQDN(t *testing.T) { + r, err := NewRouter("passthrough", []Route{ + {Host: "*", Audience: "single-label"}, + {Host: "api.example.com", Audience: "fqdn"}, + }) + if err != nil { + t.Fatalf("single-star before an FQDN route must be accepted, got err = %v", err) + } + if resolved := r.Resolve("api.example.com"); resolved == nil || resolved.Audience != "fqdn" { + t.Error("FQDN route must stay reachable behind a single-label wildcard") + } +} + +// TestNewRouter_AcceptsTrailingCatchAll is the deliberate difference from +// skip_hosts, which rejects match-all outright. Here a match-all as the last +// route is a legitimate explicit catch-all — equivalent to defaultAction — +// because nothing follows it to shadow. +func TestNewRouter_AcceptsTrailingCatchAll(t *testing.T) { + r, err := NewRouter("passthrough", []Route{ + {Host: "api.example.com", Audience: "specific"}, + {Host: "**", Audience: "catch-all"}, + }) + if err != nil { + t.Fatalf("trailing catch-all must be accepted, got err = %v", err) + } + if resolved := r.Resolve("api.example.com"); resolved == nil || resolved.Audience != "specific" { + t.Error("specific route must win over the trailing catch-all") + } + if resolved := r.Resolve("anything-else"); resolved == nil || resolved.Audience != "catch-all" { + t.Error("trailing catch-all must match everything the specific route does not") + } +} + +func TestNewRouter_RejectsEmptyHost(t *testing.T) { + _, err := NewRouter("passthrough", []Route{ + {Host: "", Audience: "nowhere"}, + }) + if err == nil { + t.Error("expected error: an empty host pattern matches only an empty Host header, never real traffic") + } +} + +// TestResolve_EmptyHost pins the empty-host defence. A bare "*" matches the +// empty string under gobwas/glob, so without the guard an unset Host header +// would select this route and mint a token for a destination we cannot +// identify. +func TestResolve_EmptyHost(t *testing.T) { + r, err := NewRouter("passthrough", []Route{ + {Host: "*-anything", Audience: "should-not-be-used"}, + {Host: "*", Audience: "should-not-be-used-either"}, + }) + if err != nil { + t.Fatal(err) + } + if resolved := r.Resolve(""); resolved != nil { + t.Errorf("empty host must take the no-route path, got %+v", resolved) } }