diff --git a/docs/operator/virtualmcpserver-kubernetes-guide.md b/docs/operator/virtualmcpserver-kubernetes-guide.md index 93e7a7b010..8feef5b4e3 100644 --- a/docs/operator/virtualmcpserver-kubernetes-guide.md +++ b/docs/operator/virtualmcpserver-kubernetes-guide.md @@ -671,28 +671,65 @@ 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, 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`, 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..899daca76c 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,42 @@ 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 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 +// 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 +634,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 +676,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 +687,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 +802,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..c6996f5365 100644 --- a/pkg/authz/authorizers/cedar/core_test.go +++ b/pkg/authz/authorizers/cedar/core_test.go @@ -1821,6 +1821,169 @@ 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, 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", + "client_id": "leaked-from-as-token-vscode", + } + + 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", + }, + }, + { + // 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 + // 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 +2177,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 +2623,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))