fix(auth): require audience when introspecting via Google tokeninfo - #6236
fix(auth): require audience when introspecting via Google tokeninfo#6236SashaMIT wants to merge 1 commit into
Conversation
Google's tokeninfo response carries no iss claim; the GoogleProvider synthesises iss locally, so a configured-issuer check against it is self-satisfying and proves nothing about which OAuth client the token was minted for. The only real binding to the deployment is the audience check - which is skipped when audience is empty. A deployment pointed at https://oauth2.googleapis.com/tokeninfo with no audience accepted ANY valid Google access token, including one minted for an unrelated attacker-controlled OAuth client. NewTokenValidator now refuses that combination at startup instead of silently accepting cross-client tokens at runtime.
jhrozek
left a comment
jhrozek
left a comment
There was a problem hiding this comment.
Nice fix, this closes a real gap — tokeninfo doesn't give you a verifiable iss so audience was the only thing actually binding the token to this deployment. Two small optional nits below, neither blocking.
| // ANY valid Google access token (e.g. one minted for an attacker's own | ||
| // unrelated OAuth client) passes validation. Refuse the combination at | ||
| // startup instead of silently accepting cross-client tokens at runtime. | ||
| if config.IntrospectionURL == GoogleTokeninfoURL && config.Audience == "" { |
There was a problem hiding this comment.
Minor: this only catches a literally empty audience. If someone sets it to " " by accident (bad env templating etc.) it'd sail past this check, then fail every single token at runtime instead of failing loudly at startup. Might be worth trimming, same as the issuer check does elsewhere:
| if config.IntrospectionURL == GoogleTokeninfoURL && config.Audience == "" { | |
| if config.IntrospectionURL == GoogleTokeninfoURL && strings.TrimSpace(config.Audience) == "" { |
Optional, not blocking.
| }) | ||
| } | ||
|
|
||
| func TestNewTokenValidator_GoogleTokeninfoRequiresAudience(t *testing.T) { |
There was a problem hiding this comment.
Optional style nit: this reads fine as-is, but since it's really one test with two input variants (empty vs set audience), our testing rules lean toward table-driven for this shape rather than two sequential imperative blocks. Up to you whether it's worth the churn for two cases.
2 participants
Problem
The Google tokeninfo path has a validation hole:
issclaim.GoogleProvider.parseGoogleResponse(token.go) compensates by settingclaims["iss"] = "https://accounts.google.com"locally.validateClaimsthen compares that fabricated value against the configured issuer — a check that is self-satisfying: it passes for every token tokeninfo accepts and proves nothing about which OAuth client the token was minted for.if v.audience != "". Audience is optional everywhere —NewTokenValidatordoesn't require it,--oidc-audiencedefaults to empty, the operator CRD marks it Optional.Net effect: a deployment configured with issuer
https://accounts.google.com+ introspection URLhttps://oauth2.googleapis.com/tokeninfo+ no audience accepts any valid Google OAuth access token — including one minted for an unrelated attacker-registered OAuth client. That token passesValidateToken, becomes anauth.Identity, and lands in the request context.Fix
NewTokenValidatorrefusesIntrospectionURL == GoogleTokeninfoURLwith an empty audience at startup, with an error explaining why. Fail-closed beats a silently-accepting runtime.Tests
New
TestNewTokenValidator_GoogleTokeninfoRequiresAudience: tokeninfo-without-audience is rejected with the explanatory error; the same config with an audience is accepted. Fullpkg/authsuite passes.Made with Cursor
Made with Cursor