Skip to content

Source Cedar claims from the upstream ID token for opaque access tokens - #6052

Open
JAORMX wants to merge 2 commits into
mainfrom
fix-cedar-opaque-claim-source-6048
Open

Source Cedar claims from the upstream ID token for opaque access tokens#6052
JAORMX wants to merge 2 commits into
mainfrom
fix-cedar-opaque-claim-source-6048

Conversation

@JAORMX

@JAORMX JAORMX commented Jul 27, 2026

Copy link
Copy Markdown
Collaborator

Summary

Cedar resolved policy claims along two different paths depending on whether the
pinned upstream's access token happened to be JWT-shaped, and the two disagreed
about both the claim source and the principal. The JWT branch (fixed in #6022/#6036)
reads that provider's access token plus its id_token; the opaque branch — the
!looksLikeJWT fallback from #5147 — read the ToolHive-issued token instead.

Two silent consequences, both fixed here:

  • The principal was not the upstream subject. On the opaque path the AS-issued
    sub is ToolHive's internal user ID (a UserResolver UUID), so
    forbid(principal == Client::"okta|alice", ...) matched with a JWT-issuing upstream
    and silently did not with an opaque-token one. Cedar has no has-style guard in
    the principal position, so there was no diagnostic.
  • Profile claims were misattributed in multi-upstream chains. The claims the auth
    server mirrors into its own token always come from the first configured upstream
    (validateChain pins chain[0] to upstreams[0]; only the first leg resolves
    identity), which need not be the provider primaryUpstreamProvider names.

The opaque branch now reads the pinned provider's id_token
(identity.UpstreamIDTokens[provider]), admitting sub plus the same
profileClaimsFromIDToken allowlist the JWT branch uses — so both paths carry that
provider's provenance and the principal is the upstream subject either way.

Fixes #6048

Type of change

  • Bug fix
  • New feature
  • Refactoring (no behavior change)
  • Dependency update
  • Documentation
  • Other (describe):

Test plan

  • Unit tests (task test)
  • Linting (task lint-fix)
  • E2E tests (task test-e2e)

New: TestResolveClaimsOpaqueUpstreamToken (id_token supplies principal + profile;
claims outside the allowlist not admitted; expired id_token still used; id_token
without sub leaves the principal absent; no id_token and unparsable id_token both
fall back) and TestAuthorizeWithJWTClaims_OpaqueUpstreamTokenPrincipal (end to end:
a banned upstream subject is forbidden, i.e. the principal really is the upstream
subject; an id_token without email denies rather than using the AS-issued value).
Plus toolhive_groups_ignored_when_upstream_token_is_opaque, extending the existing
issued-token invariant to the newly-changed branch.

All pinned security tests pass unchanged: authorization_bearing_claims_are_not_filled,
toolhive_groups_ignored_when_upstream_configured,
direct_groups_ignored_when_upstream_configured,
TestAuthorizeWithJWTClaims_PrincipalStaysUpstreamSourced,
tampered_upstream_jwt_still_denies.

task lint-fix 0 issues; codespell clean; go test -race ./pkg/authz/... ./pkg/auth/...
clean; task test shows only three failures that are pre-existing sandbox limitations
(pkg/api TestNewServer_ReadTimeoutConfigured and TestSecurityHeaders — no
container runtime; pkg/secrets/keyring TestCompositeProvider_RealProviders — no
keyring).

Does this introduce a user-facing change?

Yes, and it is a behaviour change on a shipped path — please read the three points
below before merging.

1. The Cedar principal changes on opaque-token deployments. It becomes
Client::"<upstream-subject>" instead of Client::"<ToolHive internal UUID>". Any
policy keyed on the UUID form stops matching. That is unlikely to be deliberate —
nobody sensibly writes a policy against a random UUID — but it is a real migration
consideration.

2. This does NOT fix every opaque-token upstream, and a release note saying
"Google and GitHub are fixed" would be half wrong.
The fix needs a stored
id_token:

  • Google (OIDC, opaque ya29.* access token, has an id_token) — fixed.
  • GitHub as type: oauth2 (opaque gho_*, never an id_token) — keeps the old
    behaviour permanently. Identity carries no record of an OAuth-2.0-only provider's
    upstream subject at all: Subject/PlatformUserID are the internal user ID,
    Name/Email are the first upstream's mirror, Groups is never populated by
    middleware, and UpstreamIDTokens has no entry. The subject exists in storage as
    UpstreamSubject but is consumed only inside the auth server and never projected
    onto Identity. Making that case work needs that projection, which is a wider
    change to pkg/auth and is filed separately.

So the no-id_token case is not an edge case — it is the GitHub-OAuth-app
configuration in our own operator guide's multi-upstream example. It therefore keeps
the identity.Claims fallback rather than failing closed: failing closed would deny
every request on those deployments with no configuration an operator could change to
recover, recreating exactly the #5916 deny-all trap #6022 removed. A rate-limited
WARN names the provider and the consequence, and the documented remedy is to request
the openid scope for that upstream.

3. claim_client_id, claim_iss, claim_aud and claim_exp are no longer
visible to policies on the opaque path.
The old path handed Cedar the whole
AS-issued claim set — client_id from session.New, plus fosite's iss/aud/exp/
iat/jti. A rule referencing claim_client_id silently stops matching. This
removes an asymmetry rather than creating one: the JWT branch never surfaced the
AS-issued client_id either. Worth knowing it was arguably a trap before — an
RFC 9068 upstream access token can carry its own client_id, which is the auth
server's client registration at the upstream, not the calling MCP client, so on the
JWT branch a claim_client_id == "vscode" rule could find a different value under
the same name
. The opaque path was the only place that claim meant what a policy
author would assume. It fails toward deny (principal has claim_client_id becomes
false), and the guide's upgrade note points at gating on the upstream subject or a
claim the upstream asserts instead.

Special notes for reviewers

Why sub is taken from the id_token here when #6036 forbids supplementing it.
#6036's rule is that sub must never be supplemented — it becomes the Cedar
principal entity ID, and Cedar offers no has-guard there, so a missing access-token
sub must fail closed. On this branch there is no access-token sub to override:
the claim set is replaced, not supplemented, and the source is the pinned
provider's own id_token, which is the correct provenance and the whole point of the
issue. An id_token that omits sub leaves the key absent, so
AuthorizeWithJWTClaims still fails closed with ErrMissingPrincipal — pinned by a
test.

Why only sub + profileClaimsFromIDToken, not the whole id_token body. Keeping
the same allowlist on both branches means #6049 (group claims from the id_token)
widens them together as one deliberate release decision, and nothing this PR does
grants access that #6049 is meant to decide. Taking the whole body would newly admit
groups/scope on the opaque path only — an inconsistency in the opposite direction
from the one this PR removes.

Interaction with #6049. That branch also touches resolveClaims, but only its
doc comment; the !looksLikeJWT branch body is untouched there, so whichever lands
second needs a comment-block rebase only.

Generated with Claude Code

@codecov

codecov Bot commented Jul 27, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.27%. Comparing base (33023da) to head (8a1e5a1).
⚠️ Report is 6 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6052      +/-   ##
==========================================
+ Coverage   72.26%   72.27%   +0.01%     
==========================================
  Files         722      724       +2     
  Lines       75138    75367     +229     
==========================================
+ Hits        54296    54474     +178     
- Misses      16980    17010      +30     
- Partials     3862     3883      +21     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

JAORMX and others added 2 commits July 28, 2026 10:14
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
Narrowing the opaque branch to the pinned provider's id_token also drops the
claims only the ToolHive-issued token carried: `client_id` (session.New) plus
fosite's `iss`/`aud`/`exp`/`iat`/`jti`. A policy gating tool access on
`claim_client_id` therefore stops matching, which an operator needs told
rather than left to discover.

This removes an asymmetry rather than creating one: the JWT branch never
exposed those claims either, since this auth server's client_id appears in
neither the upstream access token nor its id_token, and a `client_id` an
upstream access token does assert is the auth server's own registration at
that upstream rather than the calling MCP client. It also fails toward deny,
as `principal has claim_client_id` becomes false.

State it in the resolveClaims contract and in the operator guide's upgrade
note, and pin it with a case asserting `client_id` reaches Cedar from neither
token. The unit test's ToolHive-issued claim set now matches what
claimsToIdentity actually produces — `client_id` present, `tsid` filtered.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Y5WXPQr5Da9Cox2nZWhg6n
@JAORMX
JAORMX force-pushed the fix-cedar-opaque-claim-source-6048 branch from 4a99b16 to 8a1e5a1 Compare July 28, 2026 07:14
@github-actions github-actions Bot added size/M Medium PR: 300-599 lines changed and removed size/M Medium PR: 300-599 lines changed labels Jul 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/M Medium PR: 300-599 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Cedar claim source and principal differ between JWT and opaque upstream access tokens

1 participant