Fall back to request-token claims for opaque upstream tokens - #5147
Merged
jhrozek merged 1 commit intoJun 1, 2026
Conversation
cjohnhanson
requested review from
ChrisJBurns,
JAORMX,
jhrozek and
yrobla
as code owners
April 30, 2026 23:15
jhrozek
previously approved these changes
May 13, 2026
Contributor
|
hey @cjohnhanson first of all sorry it took so long to review your PR, it somehow slipped through the review queue. That shouldn't happen! I've included just one nit inline, let me know if you want to address it if not, I'm fine sending a follow up (feeling a bit bad to ask you to do more work after letting you wait for almost 2 weeks..) |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5147 +/- ##
==========================================
- Coverage 68.83% 68.82% -0.01%
==========================================
Files 627 627
Lines 63590 63599 +9
==========================================
Hits 43772 43772
- Misses 16568 16575 +7
- Partials 3250 3252 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
|
oh looks like the linter is unhappy as well, this might require another push |
VirtualMCPServer (Cedar incoming authz) denied every request when the embedded auth servers upstream provider issues opaque OAuth 2.0 access tokens (Googles ya29.*, GitHubs gho_*). resolveClaims tried to JWT-parse the upstream token unconditionally and returned the parse error verbatim, so every authorization check failed and the gateway skipped every tool. Discriminate by token shape: if the upstream token is not three dot- separated segments it cannot be a JWT, so fall back to identity.Claims (the request-token claims). The embedded auth server already mirrors the upstream OIDC sub, email and name into its issued AS token (see pkg/authserver/server/session/session.go), so policies referencing standard OIDC claims continue to evaluate correctly. JWT-shaped tokens (three segments) that fail to parse still return the error: a tampered or corrupted upstream JWT must not silently degrade to fallback claims. Closes stacklok#5146 Signed-off-by: Cody J. Hanson <cjohnhanson@users.noreply.github.com>
cjohnhanson
force-pushed
the
fix/cedar-opaque-token-fallback
branch
from
May 26, 2026 15:24
81dfe3a to
dc40547
Compare
Author
|
@jhrozek no worries, sorry for the delay on coming back to this. Force-pushed with the nit addressed. |
github-actions
Bot
added
size/XS
jhrozek
approved these changes
Jun 1, 2026
9 tasks
JAORMX
added a commit
that referenced
this pull request
Jul 27, 2026
With the embedded auth server active, the runner force-injects the first upstream provider as Cedar's PrimaryUpstreamProvider, so policies are evaluated against the upstream IdP's access token. Many OIDC providers assert profile claims only in the id_token and omit them from the access token (JetBrains Hub is a public example). Those access tokens are JWT-shaped and parse cleanly, so the opaque-token fallback added in #5147 never applied, and any policy referencing claim_email evaluated a claim set that could not contain it: tools/list returned empty and tools/call returned 403 for every user, with no diagnostic. Audit logging reads the AS-issued token, so it kept reporting the correct user and pointed investigations away from authorization. The auth server already mirrors the upstream sub/name/email into the token it issues, so let those three claims — and only those — fall back to the request token's values when the upstream access token omits them. An upstream value always wins where the IdP asserts one. The supplement stops there on purpose: groups, roles and scopes must remain upstream-only so a claim on the ToolHive-issued token can never grant access the upstream did not assert, and the AS token's own protocol claims (iss, aud, exp, jti, client_id) describe that token rather than the upstream identity. Nothing is fabricated, so has-guarded policies still fail closed on claims neither token carries. Also log once per rate-limit window naming the claims that were supplemented, the JWT-branch counterpart of the existing opaque-token warning, so this is diagnosable from logs alone. Fixes #5916 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
JAORMX
added a commit
that referenced
this pull request
Jul 27, 2026
With the embedded auth server active, the runner force-injects the first upstream as Cedar's PrimaryUpstreamProvider, so policies were evaluated against that provider's access token alone. Many OIDC providers assert profile claims only in the id_token, so a JWT-shaped access token without `email` parsed cleanly, never hit the opaque-token fallback from #5147, and left `principal has claim_email` false for every user: tools/list returned empty and tools/call 403, with no diagnostic. Audit logging reads the AS-issued token, so it kept naming the correct user and pointed investigations away from authorization. Supplement the missing profile claims from the same provider's stored id_token. `name` and `email` fall back to it when the access token omits them; access-token values always win. Provenance is preserved by construction, so `principal has claim_email` still means "this upstream asserted an email". The ToolHive-issued token is deliberately not a claim source here. In a multi-upstream chain its mirrored name/email always belong to the FIRST configured upstream (validateChain pins chain[0] to upstreams[0], and only the first leg resolves identity from its provider), while primaryUpstreamProvider may name any upstream — so using it would attribute one IdP's email to another. `sub` is never supplemented: it becomes the Cedar principal entity ID, where Cedar offers no `has`-style guard, so a missing access-token `sub` fails closed with ErrMissingPrincipal rather than having a principal chosen for it. The id_token is read without checking `exp`, deliberately. It is a record of what the upstream asserted at login, not a presented credential; enforcement of that contract belongs at the RFC 8693 consumer. Enforcing it here would let a policy permit early in a session and silently deny later, since sessions outlive id_tokens by days. Two rate-limited warnings make the condition diagnosable from logs alone, distinguishing "used the id_token" from "no id_token stored, will deny". Also makes captureSlogWarn concurrency-safe: it installs a capturing handler via the process-global slog.SetDefault, which the new warning turned into a reproducible data race under -race. Fixes #5916
This was referenced Jul 27, 2026
JAORMX
added a commit
that referenced
this pull request
Jul 28, 2026
With the embedded auth server active, Cedar resolved claims from two sources depending on an accident of the pinned upstream's access token format. A JWT access token gave claims from that upstream, so the Cedar principal was the upstream subject. An opaque one (Google's ya29.*, GitHub's gho_*) hit the #5147 fallback and gave the claims of the token ToolHive itself issued, so the principal became ToolHive's internal user UUID and `forbid(principal == Client::"okta|alice", ...)` silently stopped matching. In a multi-upstream chain the same fallback attributed the FIRST configured upstream's profile to the pinned provider, since that is whose name/email the auth server mirrors. Read the pinned provider's own id_token on that branch instead. It supplies the principal `sub` plus the profileClaimsFromIDToken allowlist that already governs the JWT path, so both paths now admit the same claim names from the same provider and session, and widening the allowlist widens them together. Claims outside it stay out: an id_token's iss/aud/nonce describe the token rather than the user, and admitting more here than on the JWT path would recreate the inconsistency being removed. `sub` is not being supplemented in the sense the JWT path forbids: an opaque token asserts nothing, so there is no access-token `sub` to override, and an id_token that omits `sub` still fails closed with ErrMissingPrincipal rather than borrowing the auth server's subject. `exp` is not enforced, as for the existing supplement — the token records what the upstream asserted at login, and rejecting it once expired would flip a policy from permit to deny mid-session. A pure OAuth 2.0 upstream never asked for the `openid` scope has no id_token at all (GitHub OAuth apps are the common case) and no other record of that provider's subject exists in the identity. For that configuration alone the ToolHive-issued token's claims are still used, which is what shipped before: failing closed would deny every request on those deployments with nothing an operator could configure to recover, so the weaker attribution is the lesser harm. A rate-limited WARN names the provider and the consequence. Fixes #6048 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y5WXPQr5Da9Cox2nZWhg6n
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ya29.*, GitHub'sgho_*).resolveClaimsJWT-parsed the upstream token unconditionally and returned the parse error verbatim, so every authorization check failed and the gateway skipped every tool.identity.Claims(the request-token claims). The embedded auth server already mirrors the upstream OIDCsub/email/nameinto its issued AS token (seepkg/authserver/server/session/session.go), so policies referencing standard OIDC claims continue to evaluate correctly.looksLikeJWT(tokenStr) = strings.Count(tokenStr, ".") == 2discriminator keeps that boundary explicit.PrimaryUpstreamProvideradded in Fix Cedar upstream-claim evaluation on VirtualMCPServer #5002 (operator side,cmd/thv-operator/pkg/vmcpconfig/converter.go) doesn't expose an opt-out CRD field, so users with opaque-token providers had no in-config workaround. This change unblocks them without changing operator behavior.Closes #5146
Type of change
Test plan
go test ./pkg/authz/...)go vet ./pkg/authz/...gofmt -l pkg/authz/authorizers/cedar/(clean)VirtualMCPServerwithaccounts.google.comupstream, Cedar policy referencingclaim_email. Pre-fix:tools/listreturns empty withAuthorization check failed for tool, skippingper tool. Post-fix: full aggregated tool list returned, single WARN lineupstream token is not a JWT; falling back to request-token claims for Cedar evaluation provider=google.The existing
TestParseUpstreamJWTClaims/opaque_token_returns_errorrow stays unchanged — the helper still returns the same error; only the caller's recovery behavior changes.API Compatibility
v1beta1API. No CRD orConfigOptionsschema change.Changes
pkg/authz/authorizers/cedar/core.goresolveClaimsdiscriminateslooksLikeJWT(tokenStr)before falling back. Opaque (≠3 segments) → fall back toidentity.Claimswith a WARN. JWT-shaped but unparseable → return the original parse error (deny). Adds thelooksLikeJWThelper.pkg/authz/authorizers/cedar/core_test.goupstream_token_opaque_not_parseablerow inTestAuthorizeWithJWTClaims_UpstreamProviderwith three rows: opaque-fallback-denied (policy mismatch via fallback claims), opaque-fallback-permitted (policy matches via fallback claims), and JWT-shaped-but-malformed-still-errors (security-regression guard for tampered JWTs).Does this introduce a user-facing change?
Yes — bug-fix only. Operators with Google (or any opaque-access-token) upstream OIDC provider now receive the aggregated tools list instead of an empty one. No CRD or configuration change required. A WARN log surfaces each fallback so operators can observe the path their identity claims take.
Tampered or corrupted upstream JWTs still hard-deny — there is no behavior change for JWT-access-token providers (Okta, Azure AD with JWT access tokens, etc.).
Special notes for reviewers
A complementary operator-side change — only auto-set
PrimaryUpstreamProviderwhen the user hasn't asked for the explicit fallback — would expose the documented escape hatch via a CRD field, since the docstring onPrimaryUpstreamProvideralready says "When empty, claims from the ToolHive-issued token are used." That route is more invasive (CRD + types + converter + tests). Happy to PR it separately if you'd prefer the opt-in surface over the implicit fallback in this PR. Either approach unblocks the failing case.The fallback could also be argued to belong behind a
ConfigOptions.AllowOpaqueTokenFallbackknob (default false). I left it implicit here because (a) the discriminator preserves the deny for tampered JWTs, which is the security-relevant case, and (b) for opaque-token providers the previous behavior was always-deny — there's no operator who relied on it doing anything useful. Open to adding the gate if you'd prefer the more conservative surface.