fix: reject match-all skip_hosts and unreachable routes - #839
Conversation
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) <noreply@anthropic.com>
Signed-off-by: Hai Huang <huang195@gmail.com>
|
Warning Review limit reachedNext included review available in 2 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (8)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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) <noreply@anthropic.com>
Signed-off-by: Hai Huang <huang195@gmail.com>
huang195
changed the title
mrsabath
left a comment
mrsabath
left a comment
There was a problem hiding this comment.
Clean, well-reasoned security hardening. It closes a live IBAC/token-exchange bypass (match-all skip_hosts spellings beyond */**), consolidates three duplicated glob-compile sites into one leaf hostglob package, and adds unreachable-route + empty-host guards to both routers.
I verified the probe behaviour against gobwas/glob v0.2.3 directly: the guard catches even spellings the PR doesn't enumerate (*?, [a-z]*, ?**, {a,*}) and correctly leaves narrow patterns (*.*, otel-*, *-prod, ?) untouched. I also confirmed both empty-host branches in Router.Resolve and brokerRouter.resolve are byte-identical to their existing no-match fallthrough, so no traffic behaviour changes. hostglob.Shadows is soundly conservative exactly as documented.
The operational note is the right call and clearly stated: shadowed/duplicate routes in authproxy-routes now fail the pod at boot rather than being silently ignored, with a one-line rollback to a warning if the availability trade isn't wanted. Since a shadowed route was already dead, traffic is unchanged.
Areas reviewed: Go, security, tests
Commits: 2, both signed-off, both carry the correct Assisted-By: Claude (Anthropic AI) trailer
CI: all green
一石二鸟 (yi shi er niao) — one stone, two birds: you consolidated the duplication and killed two hidden bugs in the same motion. Nice work.
One non-blocking design note left inline.
| // 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", |
There was a problem hiding this comment.
Nice that the shortest probe is a single char "a" — that's exactly what lets genuinely-narrow patterns like ??* and *-* escape the flag (verified against gobwas/glob). Worth a note for future readers on the flip side: because the guarantee is precisely "matches all four probe shapes," a pattern that matches every real service name but requires ≥2 characters (e.g. ??*) is not flagged, and single-char in-cluster Service names are legal DNS labels. That's deliberately the safe direction here (under-reject rather than fail a pod at boot), and it fits your "behavioural probe, not a proof of universal breadth" framing — so this is just documenting the boundary, not asking for a change. Adding more single-char probes wouldn't move it; only widening intent-detection would.
github-project-automation
Bot
moved this from New/ToDo
to Done
in Rossoctl Issue Prioritization
3 participants
Summary
Three packages match a destination Host against operator-supplied patterns, each compiling
glob.Compile(pattern, '.')itself and reasoning about breadth on its own:listener/skiphostroutingplugins/tokenbrokerThat duplication is the root cause of everything below. This PR gives the behaviour one owner and fixes the two bugs the duplication hid.
1. skip_hosts accepted most spellings of match-all
skiphost.New's guard identified footgun patterns by string equality:It caught the two obvious spellings and nothing else. Under
.-separated globs these are all equally broad and all passed:***,****, longer runs**{**}{*,**}?*Any one of them in
listener.skip_hostsexempted every outbound host from IBAC and token-exchange, which is exactly what the guard's doc comment says it prevents. Present under both glob v0.2.3 and v1.0.0, so unrelated to any pending bump — it is live onmaintoday.Now rejected by behaviour rather than spelling: compile the pattern, then reject it if it matches every short single-label Kubernetes service name, which is the shape that made bare
*dangerous to begin with.2. The two routers had no breadth check at all
They also deliberately do not copy skip_hosts' blanket rejection, because a match-all in a router is not a bypass — as the final route it is a legitimate explicit catch-all, equivalent to
defaultAction.The real hazard is first-match-wins: a broad early pattern silently swallows every route beneath it. A
***typo at the top ofauthproxy-routes, or a plain*(which matches every short in-cluster service name), kills every route below — and if the shadowing route is apassthrough, token exchange or brokering is silently off for hosts the operator explicitly listed.So both routers reject unreachable routes instead.
hostglob.Shadowsis sound rather than complete — it reports only certain unreachability:A later wildcard shadowed by a narrower earlier wildcard (
*.a.comafter*.*.com) is not reported. Deciding that means deciding glob subsumption, and a false positive fails the pod at boot, so the check stays conservative.Both routers additionally now reject an empty host pattern — a route whose
host:key is missing fromroutes.yamlcompiles into a pattern matching only an empty Host header, so it can never match 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.3. One owner:
authlib/internal/hostglobThe separator, the breadth probes and the shadowing rule now live in one leaf package that depends on nothing inside authlib. Beyond fixing the above, it means the eventual glob v1 migration (blocked on OPA — see #829) lands in one file instead of three.
The production router is built from the
authproxy-routesConfigMap, 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. If that trade is not wanted, downgrading the shadowing check to a logged warning is a one-line change; say so and I will make it. The empty-host-pattern and skip_hosts checks are lower risk (no real config can rely on either).
Over-rejection is the risk that matters
A wrongly rejected pattern fails the pod at boot, so the dangerous direction here is being too aggressive. Pinned as accepted:
TestNew_AcceptsNonMatchAllWildcards— twelve operator-plausible skip_hosts wildcardsTestNewRouter_AcceptsTrailingCatchAll— match-all in the final positionTestNewRouter_AcceptsSingleStarBeforeFQDN—*before an FQDN route, which stays reachable because*spans one labelTestNewBrokerRouter_AcceptsReachableRoutes— five genuinely-reachable route listsTestMatchesEverySingleLabel_NarrowPatterns— eleven patterns that must not be flaggedTestNew_AcceptsLeadingStarTest fixture changed
TestResolve_FirstMatchWinsconfigured the host"service"twice — dead config, now rejected. Rewritten to prove the same property with two genuinely overlapping patterns (svc-*and*-prod) where neither route is unreachable, plus an assertion that the second route really is reachable. It also ignoredNewRouter's error, which is why the new guard surfaced as a nil-pointer panic rather than a clean failure; it checks the error now.Verification
go test ./...across all of authlib: passgo build ./... && go vet ./...for every authbridge module (all butcmd/authbridge-cpex, which has no packages without-tags cpexand cannot link locally withoutlibcpex_ffi): passgolangci-lint runoninternal/hostglob,routing,listener/skiphost: 0 issuesgofmt: cleanplugins/tokenbrokerhas 12 pre-existing golangci-lint findings inclient/*_test.go,plugin_edge_test.goandplugin_testing.go— none in files this PR touches, and none introduced here.Related: #837 pins the glob matching contract these three packages share; #829 is the blocked glob v1 bump that prompted the audit.
Assisted-By: Claude Code