Cedar must not take its principal from an unverified id_token
Nobody has claimed this yet.
- Dominant language
- Go
- Stars
- 2.2k
- Forks
- 300
- Avg merge
- 1d 15h
- Merged PRs (30d)
- 184
Description
Problem
Since #6048, when a pinned upstream provider's access token is opaque, the Cedar authorizer sources policy claims from that provider's stored id_token — including sub, which becomes the Cedar principal entity ID.
For type: oidc upstreams that token is cryptographically validated before storage (at login, upstream/oidc.go:318, and again on refresh with a subject-stability check, oidc.go:506-524). For type: oauth2 upstreams it is not validated at all:
convertOAuth2Token(pkg/authserver/upstream/oauth2.go:257) extractstoken.Extra("id_token")with no provider-type gate.BaseOAuth2Provider— used standalone fortype: oauth2— has noverifierfield.callback.go:163persists it verbatim;pkg/auth/token.go:1273-1278projects it intoIdentity.UpstreamIDTokens.
So a type: oauth2 upstream that returns an id_token has its unverified sub become the authorization principal, while the authenticated subject comes from a separate path (userinfo / identityFromToken / synthesis). Nothing anywhere compares the two — grepped callback.go, oauth2.go, storage/*.go.
Severity
Not exploitable, and reviewed as such by four independent passes:
- Skipping signature validation is permitted here — the token arrives over TLS in a direct client↔token-endpoint response (OIDC Core 1.0 §3.1.3.7 item 6).
- A hostile upstream already controls
Identity.Subjectvia userinfo, so it gains nothing by lying in anid_token.
What remains is a provenance defect: for an honest upstream, ToolHive authenticates via one path and authorizes via a second that nothing cross-checks. Filed because it is a live, ungated inconsistency that will become a real bug for the next non-GitHub oauth2-typed upstream.
d0f5d9b (PR #6052 follow-up) corrected the doc comment that previously asserted this could not happen. This issue tracks actually enforcing it.
Why the obvious fix is not a one-liner
Clearing Tokens.IDToken for non-OIDC providers is ~4 lines, but a prototype surfaced four problems:
- It breaks XAA in a legitimate config.
pkg/vmcp/auth/strategies/xaa.go:113presents the stored id_token as an RFC 8693subject_token, failing withErrUpstreamTokenNotFoundwhen empty.InjectSubjectProviderNames(pkg/vmcp/config/defaults.go:114-119) auto-selects the subject provider fromrc.Upstreamswith no provider-type filter, and nothing cross-validates strategy against upstream type. Critically,validateEndpointOrigin(upstream/oidc.go:554) requirestoken_endpointto share the issuer's origin, so a genuine OIDC IdP behind a gateway cannot betype: oidcand is forced ontotype: oauth2— where its id_token is real and signed, and XAA presenting it is correct. - Flag polarity must be fail-safe. A
discardUnverifiedIDToken booldefaults to "keep the token", so safety depends on remembering to opt in, and any future provider built by struct literal (the patternoidc.go:188-191already uses) silently inherits unverified storage. Invert it:verifiesIDToken, set only byOIDCProviderImpl, so the zero value discards. - The invariant would not hold for existing sessions.
refresher.go:156-158deliberately carries the stored id_token forward when a refresh omits one, so every pre-upgrade session keeps re-persisting its old unverified id_token for the rest of its life (bounded byRefreshTokenLifespan). - The break would be silent. No WARN when a non-empty id_token is dropped, and
docs/operator/virtualmcpserver-kubernetes-guide.md:712advises "Requesting theopenidscope for that upstream is what fixes it" — which becomes a literal no-op fortype: oauth2. Correct advice is to configure the upstream astype: oidc.
Options
A. Gate at storage (root-cause location; all current and future consumers inherit it). Requires verifiesIDToken polarity, a WARN naming the type: oidc remedy, the guide fix, a decision on pre-upgrade sessions, and a release note for the XAA behaviour change.
B. Gate at the consumer — plumb OIDC-verified provenance through to Cedar so it refuses an unverified id_token principal while XAA keeps its token. Preserves the hybrid-flow capability, but duplicates upstream-type knowledge and needs a new field through UpstreamCredential ({AccessToken, IDToken} today) → Identity → cedar ConfigOptions.
A is the smaller, more durable shape and is preferred on root-cause grounds; B is the only one that does not break XAA-on-oauth2. The tradeoff is a product call about whether that configuration is supported.
Either way, pkg/auth/identity.go's UpstreamIDTokens doc should record the resulting provenance guarantee — it currently documents only the expiry contract, which is where a future consumer will look.
Test gap to close alongside
The seam is untested, which is why the prototype looked free. xaa_test.go:42 and the Cedar tests all construct Identity directly from literal maps, so no test connects "the provider stored an id_token" to "a downstream consumer reads it". A provider-layer change cannot fail any XAA or Cedar test. An integration test across provider → storage → Identity → consumer would have caught the regression immediately.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with convertOAuth2Token in pkg/authserver/upstream/oauth2.go, the refresh path in refresher.go, and the downstream XAA and Cedar consumers. Review the two proposed enforcement locations, the existing tests in xaa_test.go and the Cedar tests, and the operator guide. Done means the chosen provenance guarantee is enforced across existing and refreshed sessions, documented, warned about where applicable, and covered by an integration test.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- authentication, authorization, security
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100