stacklok / stacklok/toolhive

Cedar must not take its principal from an unverified id_token

Open
#6,081 0 comments 0 reactions 0 assignees View on GitHub

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) extracts token.Extra("id_token") with no provider-type gate.
  • BaseOAuth2Provider — used standalone for type: oauth2 — has no verifier field.
  • callback.go:163 persists it verbatim; pkg/auth/token.go:1273-1278 projects it into Identity.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.Subject via userinfo, so it gains nothing by lying in an id_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:

  1. It breaks XAA in a legitimate config. pkg/vmcp/auth/strategies/xaa.go:113 presents the stored id_token as an RFC 8693 subject_token, failing with ErrUpstreamTokenNotFound when empty. InjectSubjectProviderNames (pkg/vmcp/config/defaults.go:114-119) auto-selects the subject provider from rc.Upstreams with no provider-type filter, and nothing cross-validates strategy against upstream type. Critically, validateEndpointOrigin (upstream/oidc.go:554) requires token_endpoint to share the issuer's origin, so a genuine OIDC IdP behind a gateway cannot be type: oidc and is forced onto type: oauth2 — where its id_token is real and signed, and XAA presenting it is correct.
  2. Flag polarity must be fail-safe. A discardUnverifiedIDToken bool defaults to "keep the token", so safety depends on remembering to opt in, and any future provider built by struct literal (the pattern oidc.go:188-191 already uses) silently inherits unverified storage. Invert it: verifiesIDToken, set only by OIDCProviderImpl, so the zero value discards.
  3. The invariant would not hold for existing sessions. refresher.go:156-158 deliberately 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 by RefreshTokenLifespan).
  4. The break would be silent. No WARN when a non-empty id_token is dropped, and docs/operator/virtualmcpserver-kubernetes-guide.md:712 advises "Requesting the openid scope for that upstream is what fixes it" — which becomes a literal no-op for type: oauth2. Correct advice is to configure the upstream as type: 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

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.