From 66323f89a2bb665a3848727f4417348202e430c5 Mon Sep 17 00:00:00 2001 From: Juan Antonio Osorio Date: Mon, 27 Jul 2026 19:33:23 +0000 Subject: [PATCH 1/2] Source Cedar claims from id_token on opaque path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_01Y5WXPQr5Da9Cox2nZWhg6n --- .../virtualmcpserver-kubernetes-guide.md | 55 +++- pkg/authz/authorizers/cedar/core.go | 148 ++++++++-- pkg/authz/authorizers/cedar/core_test.go | 277 ++++++++++++++++++ pkg/authz/integration_test.go | 39 ++- 4 files changed, 464 insertions(+), 55 deletions(-) diff --git a/docs/operator/virtualmcpserver-kubernetes-guide.md b/docs/operator/virtualmcpserver-kubernetes-guide.md index 93e7a7b010..f89690ce39 100644 --- a/docs/operator/virtualmcpserver-kubernetes-guide.md +++ b/docs/operator/virtualmcpserver-kubernetes-guide.md @@ -671,28 +671,55 @@ days, expiring it out of policy evaluation would let the same user be permitted early in a session and denied later, with no configuration change. The token the client presented — the one ToolHive's auth server issued — is -never a claim source here, even though it mirrors a `name` and `email`. In a -multi-upstream chain those mirrored values come from the **first** configured -upstream (the identity provider), which need not be the provider -`primaryUpstreamProvider` names, so using them could attribute one IdP's email to -another. +never a claim source here, even though it mirrors a `name` and `email`, with the +single exception noted below. In a multi-upstream chain those mirrored values come +from the **first** configured upstream (the identity provider), which need not be +the provider `primaryUpstreamProvider` names, so using them could attribute one +IdP's email to another. + +**Opaque upstream access tokens**: some providers (Google's `ya29.…`, GitHub's +`gho_…`) issue access tokens that are not JWTs at all, so there are no claims in +them to read. For those, that same provider's `id_token` is the claim source +instead: it supplies the principal (`sub`) and the same `name`/`email` profile +claims, so a rule keyed on `Client::""` matches the upstream +identity here exactly as it does for a JWT access token. Claims the `id_token` +carries beyond those — `iss`, `aud`, `nonce`, groups, or provider-specific ones +like Google's `hd` — are not admitted, the same restriction that applies when a +JWT access token is supplemented. + +Earlier releases evaluated the ToolHive-issued token on this path instead, so a +policy written against that token's values needs re-checking against the pinned +provider's `id_token`: the principal is now the upstream subject rather than +ToolHive's internal user ID, and in a multi-upstream chain `claim_email` now names +the pinned provider's email rather than the first configured upstream's. An OAuth 2.0 upstream that was never asked for the `openid` scope has no stored -`id_token`, so nothing is available to fall back to and the claim stays absent. -The proxy says so once per 30s: +`id_token`. With a JWT access token, nothing is available to fall back to and the +profile claim stays absent. With an opaque access token there is no upstream claim +source at all, and only in that case does Cedar evaluate the claims of the token +the client presented — so the principal is ToolHive's internal user ID rather than +the upstream subject, and a rule keyed on the upstream subject will not match. +Requesting the `openid` scope for that upstream is what fixes it. Either way the +proxy says so once per 30s: ``` WARN no upstream ID token stored for provider; policies referencing profile claims the access token omits will deny provider=okta + +WARN no usable upstream ID token for provider with an opaque access token; + falling back to the claims of the ToolHive-issued token, so the Cedar + principal is ToolHive's internal user ID rather than the upstream subject + provider=github ``` -Every other claim is upstream-access-token-only. In particular, group, role and -scope claims are not substituted from anywhere, so a policy such as `principal in -THVGroup::"platform-eng"` only ever matches groups the upstream access token -asserts. The same holds for the principal: `sub` is never substituted, so a rule -keyed on `Client::""` keeps matching the upstream identity, and -an access token with no `sub` is rejected outright rather than having a principal -chosen for it. +Every other claim comes from the upstream access token or not at all. In +particular, group, role and scope claims are not substituted from anywhere, so a +policy such as `principal in THVGroup::"platform-eng"` only ever matches groups the +pinned upstream asserts. The principal is always that upstream's subject too: a JWT +access token's `sub` is never substituted, so a JWT that carries no `sub` is +rejected outright rather than having a principal chosen for it, and an opaque access +token takes the subject from the `id_token` of the same provider and session — never +from the token the client presented. If a policy references a claim that neither the access token nor the `id_token` carries, `principal has claim_x` is false and the policy denies — author domain diff --git a/pkg/authz/authorizers/cedar/core.go b/pkg/authz/authorizers/cedar/core.go index d6198e71fe..464a294f8c 100644 --- a/pkg/authz/authorizers/cedar/core.go +++ b/pkg/authz/authorizers/cedar/core.go @@ -172,7 +172,8 @@ type Authorizer struct { mu sync.RWMutex // primaryUpstreamProvider names the upstream IDP provider whose access token // is the source of JWT claims for Cedar evaluation, aside from the narrow - // user-profile supplement described in resolveClaims. + // user-profile supplement and the opaque-access-token case described in + // resolveClaims — both of which read the same provider's id_token. // When empty, claims from the token on the original client request are used, // which may be a ToolHive-issued token or any other bearer token. primaryUpstreamProvider string @@ -197,9 +198,10 @@ type Authorizer struct { // the window, hiding the claim-key dump on exactly the deployments that need it. supplementLog *syncutil.AtMost // missingIDTokenLog rate-limits the warning that the primary provider has no - // usable stored id_token, so profile claims cannot be supplemented at all. - // Separate from supplementLog because the two are mutually exclusive per - // request and describe opposite conditions. + // usable stored id_token — meaning either that profile claims cannot be + // supplemented (JWT access token) or that there is no upstream claim source at + // all (opaque access token). Separate from supplementLog because the two are + // mutually exclusive per request and describe opposite conditions. missingIDTokenLog *syncutil.AtMost // multiValuedClaims lists JWT claim names normalized to a canonical unpadded // space-delimited string, plus a companion Cedar Set, before Cedar evaluation. @@ -223,10 +225,13 @@ type ConfigOptions struct { // The profile claims in profileClaimsFromIDToken fall back to the SAME // provider's id_token when its access token omits them (many OIDC providers // assert `email` only in the id_token), so `principal has claim_email` keeps - // meaning "this upstream asserted an email". Every other claim, including all - // group/role/scope claims, comes from the upstream access token or not at all, - // and the ToolHive-issued token the client presented is never a claim source on - // this path. See resolveClaims for the full contract. + // meaning "this upstream asserted an email". An opaque access token has no + // claims at all, so that provider's id_token supplies the principal and those + // same profile claims instead. Every other claim, including all group/role/scope + // claims, comes from the upstream access token or not at all, and the + // ToolHive-issued token the client presented is a claim source only for an + // opaque-access-token provider with no stored id_token, which has no upstream + // claim source whatsoever. See resolveClaims for the full contract. PrimaryUpstreamProvider string `json:"primary_upstream_provider,omitempty" yaml:"primary_upstream_provider,omitempty"` // GroupClaimName is the JWT claim key that contains group membership for the @@ -578,7 +583,32 @@ func (a *Authorizer) IsAuthorized( // The ToolHive-issued token the client presented is deliberately NOT a claim // source here, even though it mirrors a name/email — in a multi-upstream chain // those belong to the first configured upstream, which need not be the pinned -// provider, so using them could attribute one IdP's email to another. +// provider, so using them could attribute one IdP's email to another. The single +// exception is the last-resort case at the end of this comment, where the pinned +// provider offers no claim source at all. +// +// When the pinned provider's access token is OPAQUE rather than JWT-shaped +// (Google's ya29.*, GitHub's gho_*) it carries no claims to read, so the same +// provider's id_token stands in as the claim carrier: it supplies the Cedar +// principal (`sub`) plus the profileClaimsFromIDToken it asserts, and nothing +// else. This keeps the principal on the upstream subject and the profile claims +// attributed to the pinned provider — what the two branches used to disagree +// about (#6048). `sub` is not "supplemented" here in the sense the bullets below +// forbid: it is the subject of the only claim source there is, for the same +// provider and session, and no access-token `sub` exists to be overridden. +// +// A pinned provider whose access token is opaque AND that has no usable stored +// id_token has no upstream claim source at all. That is a pure OAuth 2.0 upstream +// (pkg/authserver/upstream/oauth2.go) never asked for the `openid` scope, whose +// identity is resolved from a userinfo endpoint and never lands in an id_token; +// an OIDC upstream always has one. For that configuration ALONE the claims of the +// token on the original client request are used, exactly as they were before +// #6048 — so the Cedar principal is ToolHive's internal user ID and the profile +// claims are the auth server's mirror of the FIRST configured upstream. Failing +// closed instead would deny every request on those deployments with nothing an +// operator could configure to recover, which is the deny-all trap #5916 was; the +// weaker attribution is the lesser harm, and a rate-limited WARN names the +// provider so the condition is diagnosable from logs alone. // // The supplement is restricted to profileClaimsFromIDToken rather than merging // the two token bodies. Read this before widening it: @@ -594,7 +624,9 @@ func (a *Authorizer) IsAuthorized( // - `sub` is excluded because it becomes the Cedar principal entity ID rather // than an attribute, and Cedar has no `has`-style guard in the principal // position. An access token without `sub` violates RFC 9068; failing closed -// with ErrMissingPrincipal beats silently choosing a principal. +// with ErrMissingPrincipal beats silently choosing a principal. An opaque +// access token is a different case, not an exception to this one: it asserts +// nothing, so there is no access-token `sub` to override. // // An access-token value always wins, so a claim the access token does assert can // never be shadowed. A claim absent from both tokens stays absent, so `has`-guarded @@ -634,14 +666,7 @@ func (a *Authorizer) resolveClaims(identity *auth.Identity) (jwt.MapClaims, erro // namespaced claims) see those attributes as absent on this branch and // must be authored defensively (`principal has claim_groups && ...`). if !looksLikeJWT(upstreamToken) { - // The Warn shares claimKeyLog with logClaimKeys below so a busy - // Google/GitHub deployment does not emit one line per tool call. - a.claimKeyLog.Do(func() { - slog.Warn("upstream token is not a JWT; falling back to request-token claims for Cedar evaluation", - "provider", a.primaryUpstreamProvider) - }) - a.logClaimKeys("token-fallback", requestClaims) - return requestClaims, nil + return a.claimsForOpaqueUpstreamToken(identity, requestClaims), nil } return nil, fmt.Errorf("failed to parse upstream token for provider %q: %w", a.primaryUpstreamProvider, err) @@ -652,6 +677,69 @@ func (a *Authorizer) resolveClaims(identity *auth.Identity) (jwt.MapClaims, erro return merged, nil } +// errNoUpstreamIDToken reports that no id_token is stored for the pinned provider, +// so that "nothing to read" and "stored but unparsable" reach one log line. +var errNoUpstreamIDToken = errors.New("no ID token stored for provider") + +// claimsForOpaqueUpstreamToken returns the claim set to evaluate when the pinned +// provider's access token is opaque, so it carries no claims of its own. +// +// The same provider's id_token stands in as the claim carrier. Only its `sub` and +// the profileClaimsFromIDToken it asserts are admitted: `sub` because the Cedar +// principal has to come from somewhere and this is the pinned provider's own +// subject, and the profile claims through the same allowlist that governs the +// JWT-access-token path, so both paths admit exactly the same claim names and +// widening the allowlist (see #6049 for group claims) widens them together. The +// id_token's remaining claims stay out: an `iss`/`aud`/`nonce`/`at_hash` describes +// the token rather than the user, and admitting the rest here but not on the JWT +// path would recreate the very inconsistency this resolves. +// +// `exp` is not checked, as elsewhere in this file — see Identity.UpstreamIDTokens. +// +// requestClaims (the ToolHive-issued token's claims) is the last resort for a +// provider with no usable id_token, which is a pure OAuth 2.0 upstream; see +// resolveClaims for why that keeps working rather than failing closed. +func (a *Authorizer) claimsForOpaqueUpstreamToken(identity *auth.Identity, requestClaims jwt.MapClaims) jwt.MapClaims { + idToken := identity.UpstreamIDTokens[a.primaryUpstreamProvider] // nil map safe in Go + + idTokenClaims, err := func() (jwt.MapClaims, error) { + if idToken == "" { + return nil, errNoUpstreamIDToken + } + return parseUpstreamJWTClaims(idToken) + }() + if err != nil { + // Shares missingIDTokenLog with supplementFromIDToken: both report that the + // provider has no usable id_token, and a request takes only one of the two + // paths, so neither starves the other. + a.missingIDTokenLog.Do(func() { + slog.Warn("no usable upstream ID token for provider with an opaque access token; "+ + "falling back to the claims of the ToolHive-issued token, so the Cedar principal is "+ + "ToolHive's internal user ID rather than the upstream subject", + "provider", a.primaryUpstreamProvider, + "error", err) + }) + a.logClaimKeys("token-fallback", requestClaims) + return requestClaims + } + + claims := make(jwt.MapClaims, len(profileClaimsFromIDToken)+1) + // An id_token without `sub` violates OIDC Core 1.0 §2, and leaving the key + // absent makes AuthorizeWithJWTClaims fail closed with ErrMissingPrincipal + // rather than pick a principal from the ToolHive-issued token. + if sub, ok := idTokenClaims[oidcSubjectClaim]; ok { + claims[oidcSubjectClaim] = sub + } + for _, name := range profileClaimsFromIDToken { + if v, ok := idTokenClaims[name]; ok { + claims[name] = v + } + } + + a.logClaimKeys("upstream-id-token", claims) + return claims +} + // supplementFromIDToken returns upstreamClaims with the profileClaimsFromIDToken // it lacks filled in from the primary provider's stored id_token, logging what it // did. upstreamClaims is not mutated. @@ -704,16 +792,22 @@ func (a *Authorizer) supplementFromIDToken(identity *auth.Identity, upstreamClai return merged } -// OIDC Core 1.0 §5.1 standard claim names, declared here rather than borrowed -// from another package. What this code reads is an upstream provider's id_token, -// so these are wire names fixed by the OIDC spec — not names ToolHive chooses. -// Anchoring them to a ToolHive constant would invert the dependency: renaming that -// constant would silently change which claim Cedar reads out of a third party's -// token. Two literals also do not justify pulling an authorization-server -// dependency tree into the authorizer. +// OIDC Core 1.0 claim names — `sub` from §2, the profile claims from §5.1 — +// declared here rather than borrowed from another package. What this code reads is +// an upstream provider's id_token, so these are wire names fixed by the OIDC spec, +// not names ToolHive chooses. Anchoring them to a ToolHive constant would invert +// the dependency: renaming that constant would silently change which claim Cedar +// reads out of a third party's token. Three literals also do not justify pulling +// an authorization-server dependency tree into the authorizer. +// +// oidcSubjectClaim is deliberately NOT in profileClaimsFromIDToken: it is never +// supplemented into a claim set an access token already produced. It is read from +// the id_token only when the access token is opaque and the id_token is therefore +// the whole claim source — see claimsForOpaqueUpstreamToken. const ( - oidcNameClaim = "name" - oidcEmailClaim = "email" + oidcSubjectClaim = "sub" + oidcNameClaim = "name" + oidcEmailClaim = "email" ) // profileClaimsFromIDToken are the OIDC Core §5.1 profile claims that may be read diff --git a/pkg/authz/authorizers/cedar/core_test.go b/pkg/authz/authorizers/cedar/core_test.go index 936e50fc90..732871368a 100644 --- a/pkg/authz/authorizers/cedar/core_test.go +++ b/pkg/authz/authorizers/cedar/core_test.go @@ -1821,6 +1821,147 @@ func TestResolveClaimsUpstreamSupplement(t *testing.T) { } } +// TestResolveClaimsOpaqueUpstreamToken pins the claim-source contract when the +// pinned provider's access token is opaque (Google's ya29.*, GitHub's gho_*) and +// therefore carries no claims of its own: the SAME provider's id_token stands in, +// supplying the principal `sub` plus the profileClaimsFromIDToken allowlist, and +// the ToolHive-issued token is not a claim source (#6048). Before that fix this +// branch evaluated the ToolHive-issued token, making the principal an internal +// user UUID and attributing the first configured upstream's profile to the pinned +// provider. +// +// The one exception is a provider with no usable id_token — a pure OAuth 2.0 +// upstream never asked for `openid` — which has no upstream claim source at all +// and still evaluates the ToolHive-issued token's claims. +func TestResolveClaimsOpaqueUpstreamToken(t *testing.T) { + t.Parallel() + + const ( + providerName = "google" + // Opaque: two dots would make looksLikeJWT true, which is a different case. + opaqueToken = "ya29.opaque-google-style-token" + ) + + // The ToolHive-issued token's claims. Every value is a sentinel: any of them + // appearing in a resolved claim set is a leak, not a coincidence. + asIssuedClaims := map[string]any{ + "sub": "7f3c1e64-9b2a-4d51-8e77-1c0a5f3b9d42", + "email": "leaked-from-as-token@wrong-provider.example", + "name": "Leaked From AS Token", + "tsid": "sess-abc123", + } + + tests := []struct { + name string + idToken string + wantClaims jwt.MapClaims + }{ + { + // The principal becomes the upstream subject, and the profile claims + // come from the provider that actually asserted them. + name: "id_token_supplies_principal_and_profile_claims", + idToken: makeUnsignedJWT(jwt.MapClaims{ + "sub": "google|alice", + "email": "alice@example.com", + "name": "Alice", + }), + wantClaims: jwt.MapClaims{ + "sub": "google|alice", + "email": "alice@example.com", + "name": "Alice", + }, + }, + { + // The allowlist boundary. `iss`, `aud` and `nonce` describe the id_token + // rather than the user; `hd` is an upstream-only claim the JWT branch + // would not supplement either. Group and role claims are governed by the + // same allowlist, so widening it (#6049) widens both branches at once. + name: "claims_outside_the_allowlist_are_not_admitted", + idToken: makeUnsignedJWT(jwt.MapClaims{ + "sub": "google|alice", + "email": "alice@example.com", + "iss": "https://accounts.google.example", + "aud": "toolhive-as-client", + "nonce": "n-abc", + "hd": "example.com", + }), + wantClaims: jwt.MapClaims{ + "sub": "google|alice", + "email": "alice@example.com", + }, + }, + { + // Same rationale as the JWT branch: the id_token records what the + // upstream asserted at login, so enforcing `exp` would flip a policy + // from permit to deny mid-session. + name: "expired_id_token_is_still_used", + idToken: makeUnsignedJWT(jwt.MapClaims{ + "sub": "google|alice", + "email": "alice@example.com", + "exp": 1000000000, // long past + }), + wantClaims: jwt.MapClaims{ + "sub": "google|alice", + "email": "alice@example.com", + }, + }, + { + // An id_token violating OIDC Core 1.0 §2 by omitting `sub` leaves the + // principal unresolvable. It is NOT taken from the ToolHive-issued + // token; AuthorizeWithJWTClaims fails closed with ErrMissingPrincipal. + name: "id_token_without_sub_leaves_the_principal_absent", + idToken: makeUnsignedJWT(jwt.MapClaims{"email": "alice@example.com"}), + wantClaims: jwt.MapClaims{"email": "alice@example.com"}, + }, + { + // A pure OAuth 2.0 upstream: no `openid` scope was requested, so no + // id_token was ever captured and there is no upstream claim source. + // Denying every request instead would leave the operator no + // configuration that recovers, so the pre-#6048 behaviour stands. + name: "no_id_token_falls_back_to_the_toolhive_issued_claims", + idToken: "", + wantClaims: jwt.MapClaims(asIssuedClaims), + }, + { + // A corrupt stored id_token is the no-id_token case, not a hard failure: + // the same fallback keeps the workload usable. + name: "unparsable_id_token_falls_back_to_the_toolhive_issued_claims", + idToken: "not-base64.not-base64.not-base64", + wantClaims: jwt.MapClaims(asIssuedClaims), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + authorizer, err := NewCedarAuthorizer(ConfigOptions{ + Policies: []string{`permit(principal, action, resource);`}, + EntitiesJSON: `[]`, + PrimaryUpstreamProvider: providerName, + }, "") + require.NoError(t, err) + + identity := &auth.Identity{ + PrincipalInfo: auth.PrincipalInfo{Subject: "7f3c1e64-uuid", Claims: asIssuedClaims}, + UpstreamTokens: map[string]string{providerName: opaqueToken}, + } + if tt.idToken != "" { + identity.UpstreamIDTokens = map[string]string{providerName: tt.idToken} + } + claimsBefore := maps.Clone(asIssuedClaims) + + resolved, err := authorizer.(*Authorizer).resolveClaims(identity) + require.NoError(t, err) + + assert.Equal(t, tt.wantClaims, resolved) + // Identity MUST NOT be modified after it is placed in the request + // context, since it is read concurrently. + assert.Equal(t, claimsBefore, asIssuedClaims, "identity claims must not be mutated") + }) + } +} + // TestAuthorizeWithJWTClaims_UpstreamJWTMissingIdentityClaim is the end-to-end // authorizer-level reproducer for #5916: the exact policy shape from the report // (a defensively-authored `has`-guarded domain gate plus a tool permit) evaluated @@ -2014,6 +2155,122 @@ func TestAuthorizeWithJWTClaims_PrincipalStaysUpstreamSourced(t *testing.T) { } } +// TestAuthorizeWithJWTClaims_OpaqueUpstreamTokenPrincipal is the end-to-end +// consequence of #6048: with an opaque upstream access token, a rule keyed on the +// upstream subject must match, and a domain gate must be satisfied by the pinned +// provider's own email rather than by the one the ToolHive-issued token mirrors. +// Before the fix the principal was ToolHive's internal user UUID, so +// principal-keyed rules silently stopped matching. +func TestAuthorizeWithJWTClaims_OpaqueUpstreamTokenPrincipal(t *testing.T) { + t.Parallel() + + const ( + providerName = "google" + opaqueToken = "ya29.opaque-google-style-token" + asSubject = "7f3c1e64-9b2a-4d51-8e77-1c0a5f3b9d42" + ) + + // A banned-user rule keyed on the upstream subject, plus a domain-gated permit. + // A principal built from asSubject stops matching the forbid; an email read from + // the ToolHive-issued token fails the gate, since the sentinel is off-domain. + authorizer, err := NewCedarAuthorizer(ConfigOptions{ + Policies: []string{ + `forbid(principal == Client::"google|banned-alice", action, resource);`, + `permit(principal, action == Action::"call_tool", resource) + when { principal has claim_email && principal.claim_email like "*@example.com" };`, + }, + EntitiesJSON: `[]`, + PrimaryUpstreamProvider: providerName, + }, "") + require.NoError(t, err) + + // The claims of the token the client presented. The email is deliberately + // off-domain so any leak shows up as an unexpected permit/deny rather than + // agreeing with the id_token by coincidence. + asIssuedClaims := map[string]any{ + "sub": asSubject, + "email": "leaked-from-as-token@wrong-provider.example", + "name": "Leaked From AS Token", + } + + tests := []struct { + name string + idTokenClaims jwt.MapClaims + noIDToken bool + wantAuthorize bool + wantErr bool + }{ + { + // The principal is the upstream subject, so the forbid matches. + name: "banned_upstream_subject_from_id_token_is_forbidden", + idTokenClaims: jwt.MapClaims{"sub": "google|banned-alice", "email": "alice@example.com"}, + wantAuthorize: false, + }, + { + name: "other_upstream_subject_with_gated_email_is_permitted", + idTokenClaims: jwt.MapClaims{"sub": "google|bob", "email": "bob@example.com"}, + wantAuthorize: true, + }, + { + // The leak detector. The id_token asserts no email, so the gate must + // fail — the ToolHive-issued token's sentinel email must not stand in. + name: "id_token_without_email_denies_rather_than_using_the_toolhive_token", + idTokenClaims: jwt.MapClaims{"sub": "google|bob"}, + wantAuthorize: false, + }, + { + // No principal is derivable, and the ToolHive-issued subject does not + // stand in, so this fails closed instead of reaching the permit. + name: "id_token_without_sub_fails_closed", + idTokenClaims: jwt.MapClaims{"email": "bob@example.com"}, + wantErr: true, + }, + { + // Pure OAuth 2.0 upstream: no id_token exists, so the ToolHive-issued + // token's claims are the documented last resort. Its email is off-domain, + // so the gate denies — which is also what pins that this configuration + // reads that token and nothing else. + name: "no_id_token_falls_back_to_the_toolhive_issued_claims", + noIDToken: true, + wantAuthorize: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + identity := &auth.Identity{ + PrincipalInfo: auth.PrincipalInfo{Subject: asSubject, Claims: asIssuedClaims}, + UpstreamTokens: map[string]string{providerName: opaqueToken}, + } + if !tt.noIDToken { + identity.UpstreamIDTokens = map[string]string{ + providerName: makeUnsignedJWT(tt.idTokenClaims), + } + } + ctx := auth.WithIdentity(context.Background(), identity) + + authorized, err := authorizer.AuthorizeWithJWTClaims( + ctx, + authorizers.MCPFeatureTool, + authorizers.MCPOperationCall, + "deploy", + nil, + ) + + if tt.wantErr { + require.ErrorIs(t, err, ErrMissingPrincipal) + assert.False(t, authorized) + return + } + + require.NoError(t, err) + assert.Equal(t, tt.wantAuthorize, authorized) + }) + } +} + // TestAuthorizeWithJWTClaims_GroupMembership verifies that Cedar policies using // "principal in THVGroup::..." are enforced when groups are present in the claims. func TestAuthorizeWithJWTClaims_GroupMembership(t *testing.T) { @@ -2344,6 +2601,26 @@ func TestAuthorizeWithJWTClaims_UpstreamProviderWithGroups(t *testing.T) { }, wantAuthorize: false, }, + { + // Same invariant on the opaque-access-token branch, where the provider's + // id_token is the claim source: a group on the ToolHive-issued token + // still grants nothing. + name: "toolhive_groups_ignored_when_upstream_token_is_opaque", + identity: &auth.Identity{ + PrincipalInfo: auth.PrincipalInfo{ + Subject: "thv-user", + Claims: map[string]any{ + "sub": "thv-user", + "groups": []interface{}{"platform-eng"}, + }, + }, + UpstreamTokens: map[string]string{providerName: "gho_opaque-github-style-token"}, + UpstreamIDTokens: map[string]string{ + providerName: makeUnsignedJWT(jwt.MapClaims{"sub": "upstream-user"}), + }, + }, + wantAuthorize: false, + }, } for _, tt := range tests { diff --git a/pkg/authz/integration_test.go b/pkg/authz/integration_test.go index b8cd12473a..37d627a51b 100644 --- a/pkg/authz/integration_test.go +++ b/pkg/authz/integration_test.go @@ -1250,6 +1250,7 @@ func TestIntegrationUpstreamJWTWithoutEmailClaim(t *testing.T) { name string upstreamToken string requestClaims jwt.MapClaims + noIDToken bool expectAllowed bool }{ { @@ -1272,19 +1273,27 @@ func TestIntegrationUpstreamJWTWithoutEmailClaim(t *testing.T) { expectAllowed: false, }, { - // Regression guard for the #5147 path, which this change leaves intact: - // an opaque access token has no claims to read, so that branch still - // evaluates the ToolHive-issued token's claims. Those carry the - // upstream profile only because the auth server mirrors the FIRST - // configured upstream, so this case supplies a single-upstream - // AS token rather than the leak sentinel the JWT cases use. - // - // That leaves the two branches inconsistent about claim provenance - // (and about the principal: this branch's `sub` is ToolHive's internal - // user ID, not the upstream subject). Unifying them on the id_token is - // a follow-up — it changes behaviour on a shipped path. - name: "opaque_upstream_token_still_falls_back", + // The #5147 path, unified with the JWT one by #6048: an opaque access + // token (Google's ya29.*, GitHub's gho_*) has no claims to read, so the + // SAME provider's id_token becomes the claim source — supplying the + // principal and the profile claims — instead of the ToolHive-issued + // token. So this case keeps the leak sentinel that the JWT cases use: + // its off-domain email would fail the gate, and the permit proves the + // email came from the pinned provider's id_token. + name: "opaque_upstream_token_uses_id_token_claims", upstreamToken: "ya29.opaque-google-style-token", + expectAllowed: true, + }, + { + // The one configuration that still reads the ToolHive-issued token: a + // pure OAuth 2.0 upstream never asked for `openid` has no stored + // id_token, so an opaque access token leaves no upstream claim source at + // all. Denying outright would leave those deployments no configuration + // that recovers, so the pre-#6048 behaviour stands — with the principal + // being ToolHive's internal user ID rather than the upstream subject. + name: "opaque_upstream_token_without_id_token_falls_back", + upstreamToken: "ya29.opaque-google-style-token", + noIDToken: true, requestClaims: jwt.MapClaims{ "sub": "7f3c1e64-9b2a-4d51-8e77-1c0a5f3b9d42", "email": "alice@example.com", @@ -1330,8 +1339,10 @@ func TestIntegrationUpstreamJWTWithoutEmailClaim(t *testing.T) { Subject: "7f3c1e64-9b2a-4d51-8e77-1c0a5f3b9d42", Claims: requestClaims, }, - UpstreamTokens: map[string]string{providerName: tt.upstreamToken}, - UpstreamIDTokens: map[string]string{providerName: upstreamIDToken}, + UpstreamTokens: map[string]string{providerName: tt.upstreamToken}, + } + if !tt.noIDToken { + identity.UpstreamIDTokens = map[string]string{providerName: upstreamIDToken} } httpReq = httpReq.WithContext(auth.WithIdentity(httpReq.Context(), identity)) From 8a1e5a121fee68c850744316d98bfa6439e27f08 Mon Sep 17 00:00:00 2001 From: Juan Antonio Osorio Date: Mon, 27 Jul 2026 19:38:05 +0000 Subject: [PATCH 2/2] Document lost AS-issued claims on opaque path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_01Y5WXPQr5Da9Cox2nZWhg6n --- .../virtualmcpserver-kubernetes-guide.md | 16 +++++++-- pkg/authz/authorizers/cedar/core.go | 10 ++++++ pkg/authz/authorizers/cedar/core_test.go | 34 +++++++++++++++---- 3 files changed, 51 insertions(+), 9 deletions(-) diff --git a/docs/operator/virtualmcpserver-kubernetes-guide.md b/docs/operator/virtualmcpserver-kubernetes-guide.md index f89690ce39..8feef5b4e3 100644 --- a/docs/operator/virtualmcpserver-kubernetes-guide.md +++ b/docs/operator/virtualmcpserver-kubernetes-guide.md @@ -689,9 +689,19 @@ JWT access token is supplemented. Earlier releases evaluated the ToolHive-issued token on this path instead, so a policy written against that token's values needs re-checking against the pinned -provider's `id_token`: the principal is now the upstream subject rather than -ToolHive's internal user ID, and in a multi-upstream chain `claim_email` now names -the pinned provider's email rather than the first configured upstream's. +provider's `id_token`: + +* the principal is now the upstream subject rather than ToolHive's internal user + ID, so a rule keyed on `Client::""` stops matching and one keyed + on `Client::""` starts; +* in a multi-upstream chain `claim_email` now names the pinned provider's email + rather than the first configured upstream's; +* claims that only the ToolHive-issued token carried — `claim_client_id`, plus its + own `claim_iss`, `claim_aud` and `claim_exp` — are no longer visible, so a policy + gating tool access on a specific OAuth client via `claim_client_id` silently stops + matching. Those claims were never available when the pinned upstream issued a JWT + access token either, so this makes the two behave alike; gate on the upstream + subject or a claim the upstream asserts instead. An OAuth 2.0 upstream that was never asked for the `openid` scope has no stored `id_token`. With a JWT access token, nothing is available to fall back to and the diff --git a/pkg/authz/authorizers/cedar/core.go b/pkg/authz/authorizers/cedar/core.go index 464a294f8c..899daca76c 100644 --- a/pkg/authz/authorizers/cedar/core.go +++ b/pkg/authz/authorizers/cedar/core.go @@ -597,6 +597,16 @@ func (a *Authorizer) IsAuthorized( // forbid: it is the subject of the only claim source there is, for the same // provider and session, and no access-token `sub` exists to be overridden. // +// A consequence worth stating: the claims the ToolHive-issued token carries in its +// own right — `client_id` (see session.New in pkg/authserver/server/session) plus +// fosite's `iss`/`aud`/`exp`/`iat`/`jti` — are no longer visible to policies on +// this path, so a rule gating on `claim_client_id` stops matching. That removes an +// asymmetry rather than creating one: the JWT branch never had those either, since +// neither the upstream access token nor its id_token carries this auth server's +// client_id, and any `client_id` an upstream access token does assert is the auth +// server's own registration at that upstream, not the calling MCP client. The loss +// also fails toward deny, since `principal has claim_client_id` becomes false. +// // A pinned provider whose access token is opaque AND that has no usable stored // id_token has no upstream claim source at all. That is a pure OAuth 2.0 upstream // (pkg/authserver/upstream/oauth2.go) never asked for the `openid` scope, whose diff --git a/pkg/authz/authorizers/cedar/core_test.go b/pkg/authz/authorizers/cedar/core_test.go index 732871368a..c6996f5365 100644 --- a/pkg/authz/authorizers/cedar/core_test.go +++ b/pkg/authz/authorizers/cedar/core_test.go @@ -1842,13 +1842,15 @@ func TestResolveClaimsOpaqueUpstreamToken(t *testing.T) { opaqueToken = "ya29.opaque-google-style-token" ) - // The ToolHive-issued token's claims. Every value is a sentinel: any of them - // appearing in a resolved claim set is a leak, not a coincidence. + // The ToolHive-issued token's claims, in the shape claimsToIdentity produces: + // the internal user ID, the profile the auth server mirrored, and the + // `client_id` it embeds per RFC 9068 (session.New). Every value is a sentinel, + // so any of them appearing in a resolved claim set is a leak, not a coincidence. asIssuedClaims := map[string]any{ - "sub": "7f3c1e64-9b2a-4d51-8e77-1c0a5f3b9d42", - "email": "leaked-from-as-token@wrong-provider.example", - "name": "Leaked From AS Token", - "tsid": "sess-abc123", + "sub": "7f3c1e64-9b2a-4d51-8e77-1c0a5f3b9d42", + "email": "leaked-from-as-token@wrong-provider.example", + "name": "Leaked From AS Token", + "client_id": "leaked-from-as-token-vscode", } tests := []struct { @@ -1890,6 +1892,26 @@ func TestResolveClaimsOpaqueUpstreamToken(t *testing.T) { "email": "alice@example.com", }, }, + { + // The other direction of the allowlist, and a behaviour change worth + // pinning: `client_id` is the one claim the ToolHive-issued token + // contributed on this branch before #6048, so a policy gating on + // `claim_client_id` used to match here. It no longer does — which is + // the state the JWT branch was always in, since the auth server's + // client_id appears in neither upstream token. The value an upstream + // id_token may carry under that name is its own OAuth client + // registration, not the calling MCP client, so it stays out too. + name: "client_id_is_admitted_from_neither_token", + idToken: makeUnsignedJWT(jwt.MapClaims{ + "sub": "google|alice", + "email": "alice@example.com", + "client_id": "toolhive-as-client-at-google", + }), + wantClaims: jwt.MapClaims{ + "sub": "google|alice", + "email": "alice@example.com", + }, + }, { // Same rationale as the JWT branch: the id_token records what the // upstream asserted at login, so enforcing `exp` would flip a policy