From 5801c8626a403fb013c1fc9eb46ae5fcf9bd7008 Mon Sep 17 00:00:00 2001 From: Juan Antonio Osorio Date: Mon, 27 Jul 2026 13:52:21 +0000 Subject: [PATCH 1/4] Fix Cedar deny-all when upstream JWT lacks email MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .../virtualmcpserver-kubernetes-guide.md | 21 ++ pkg/authz/authorizers/cedar/core.go | 197 ++++++++--- pkg/authz/authorizers/cedar/core_test.go | 326 +++++++++++++++++- pkg/authz/integration_test.go | 134 +++++++ 4 files changed, 628 insertions(+), 50 deletions(-) diff --git a/docs/operator/virtualmcpserver-kubernetes-guide.md b/docs/operator/virtualmcpserver-kubernetes-guide.md index 0f43265c42..75060505c9 100644 --- a/docs/operator/virtualmcpserver-kubernetes-guide.md +++ b/docs/operator/virtualmcpserver-kubernetes-guide.md @@ -650,6 +650,27 @@ spec: - 'permit(principal, action, resource);' ``` +**Which token's claims a policy sees**: Cedar reads the primary upstream +provider's *access token*. Claims that token asserts always win. Because many +OIDC providers put profile claims in the `id_token` only and omit them from the +access token, the three user-identity claims the auth server mirrors into the +token it issues — `sub`, `name` and `email` — fall back to that mirrored value +when the upstream access token does not carry them. When this happens the proxy +logs, once per 30s: + +``` +WARN upstream token lacks identity claims; using the request token's values for + Cedar evaluation provider=okta claims="[email]" +``` + +Every other claim is upstream-only. In particular, group, role and scope claims +are read from the upstream access token or not at all, so a policy such as +`principal in THVGroup::"platform-eng"` only ever matches groups the upstream IdP +asserts. If a policy references a claim neither token carries, `principal has +claim_x` is false and the policy denies — author domain and group gates +defensively with `has`, and confirm the claim is present in the upstream access +token (not just the `id_token`) before gating on it. + > **Migration: `primaryUpstreamProvider` location** > > The field used to live under diff --git a/pkg/authz/authorizers/cedar/core.go b/pkg/authz/authorizers/cedar/core.go index ef5a538428..146eccc78a 100644 --- a/pkg/authz/authorizers/cedar/core.go +++ b/pkg/authz/authorizers/cedar/core.go @@ -171,7 +171,9 @@ type Authorizer struct { // Mutex for thread safety mu sync.RWMutex // primaryUpstreamProvider names the upstream IDP provider whose access token - // should be used as the source of JWT claims for Cedar evaluation. + // is the primary source of JWT claims for Cedar evaluation. Its claims are + // merged over the request token's claims (upstream wins per key); see + // resolveClaims for the precedence rules and why they are not a plain replace. // 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 @@ -205,9 +207,15 @@ type ConfigOptions struct { EntitiesJSON string `json:"entities_json" yaml:"entities_json"` // PrimaryUpstreamProvider names the upstream IDP provider whose access - // token should be used as the source of JWT claims for Cedar evaluation. - // When empty, claims from the ToolHive-issued token are used (current behaviour). + // token is the primary source of JWT claims for Cedar evaluation. + // When empty, claims from the ToolHive-issued token are used. // Must match an entry in identity.UpstreamTokens (e.g. "default", "github"). + // + // The upstream token's claims do not replace the request token's claims: they + // are merged over them, so a claim the upstream access token omits (commonly + // `email`, which many providers place in the id_token only) falls back to the + // value the ToolHive-issued token carries, while every claim the upstream does + // assert wins. 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 @@ -532,56 +540,147 @@ func (a *Authorizer) IsAuthorized( } // resolveClaims determines which JWT claims to use for Cedar policy evaluation. -// When primaryUpstreamProvider is set, claims are extracted from the upstream -// IDP token stored in the identity. Otherwise, claims from the token on the -// original client request are used, which may be a ToolHive-issued token or -// any other bearer token. +// +// When primaryUpstreamProvider is empty (the default), the claims of the token on +// the original client request are used as-is. That may be a ToolHive-issued token +// or any other bearer token. +// +// When primaryUpstreamProvider is set (the embedded auth server path), the +// upstream IDP access token is the claim source, with one narrow supplement: the +// user-identity claims listed in mirroredIdentityClaims fall back to the request +// token's values when the upstream access token does not carry them. Using the +// upstream token alone was a deny-all trap, because many OIDC providers put +// identity claims such as `email` in the id_token only and omit them from the +// JWT access token (JetBrains Hub is a public example), so every policy +// referencing `claim_email` silently denied every request. See #5916. +// +// The supplement is deliberately restricted to mirroredIdentityClaims rather +// than being a general merge of the two claim sets, for two reasons: +// +// - Authorization-bearing claims stay single-sourced. `groups`, `roles`, `hd` +// and custom namespaced claims are asserted by the upstream IDP or not at +// all; a group on the ToolHive-issued token must never grant access that the +// upstream did not assert. +// - Everything else on the AS-issued token describes that token, not the +// upstream identity. Letting `iss`, `aud`, `exp`, `jti` or `client_id` fill +// upstream gaps would hand policies the auth server's own values under the +// guise of upstream claims. +// +// Within mirroredIdentityClaims an upstream value always wins, so a claim the +// upstream IDP does assert can never be shadowed by the token presented on the +// request. Both sources are server-side trusted: the request token's claims were +// signature-verified by the auth middleware before reaching any authorizer, and +// the upstream token is held in server-side session state after the auth server's +// code exchange, never supplied by the client. func (a *Authorizer) resolveClaims(identity *auth.Identity) (jwt.MapClaims, error) { - if a.primaryUpstreamProvider != "" { - // Embedded auth server path: use the upstream IDP token's claims. - upstreamToken, tokenFound := identity.UpstreamTokens[a.primaryUpstreamProvider] - if !tokenFound || upstreamToken == "" { - // The upstream token must be present if the authorizer is configured to use it. - // Missing token means the session has no upstream credential; deny. - return nil, fmt.Errorf("upstream token for provider %q not found in identity", - a.primaryUpstreamProvider) + requestClaims := jwt.MapClaims(identity.Claims) + + if a.primaryUpstreamProvider == "" { + // Default path: use claims from the original client request's token. + a.logClaimKeys("token", requestClaims) + return requestClaims, nil + } + + // Embedded auth server path: the upstream IDP token is the primary claim source. + upstreamToken, tokenFound := identity.UpstreamTokens[a.primaryUpstreamProvider] + if !tokenFound || upstreamToken == "" { + // The upstream token must be present if the authorizer is configured to use it. + // Missing token means the session has no upstream credential; deny. + return nil, fmt.Errorf("upstream token for provider %q not found in identity", + a.primaryUpstreamProvider) + } + + upstreamClaims, err := parseUpstreamJWTClaims(upstreamToken) + if err != nil { + // Distinguish "not JWT-shaped" (opaque OAuth 2.0 access token — + // Google's ya29.*, GitHub's gho_*, etc.) from "JWT-shaped but + // malformed/tampered" (a JWT with three segments that fails to + // parse). Only fall back for the former; preserve the deny for + // the latter so a tampered upstream JWT cannot bypass policy. + // + // Policies that reference upstream-only claims (groups, hd, custom + // 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 } - parsedClaims, err := parseUpstreamJWTClaims(upstreamToken) - if err != nil { - // Distinguish "not JWT-shaped" (opaque OAuth 2.0 access token — - // Google's ya29.*, GitHub's gho_*, etc.) from "JWT-shaped but - // malformed/tampered" (a JWT with three segments that fails to - // parse). Only fall back for the former; preserve the deny for - // the latter so a tampered upstream JWT cannot bypass policy. - // - // The embedded auth server already mirrors the upstream OIDC - // sub/email/name claims into its issued AS token (see - // pkg/authserver/server/session/session.go). For opaque-token - // providers, falling back to identity.Claims preserves identity - // for policies referencing standard OIDC claims; policies that - // reference upstream-only claims (groups, hd, custom namespaced - // claims) will see those attributes as absent and must be - // authored defensively (`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", jwt.MapClaims(identity.Claims)) - return jwt.MapClaims(identity.Claims), nil - } - return nil, fmt.Errorf("failed to parse upstream token for provider %q: %w", - a.primaryUpstreamProvider, err) + return nil, fmt.Errorf("failed to parse upstream token for provider %q: %w", + a.primaryUpstreamProvider, err) + } + + merged, filled := supplementUpstreamClaims(upstreamClaims, requestClaims) + if len(filled) > 0 { + // Shares claimKeyLog with logClaimKeys so a deployment whose upstream + // token permanently lacks these claims does not emit a line per + // authorization check. This is the JWT-branch counterpart of the + // opaque-token Warn above: without it, an operator whose upstream issues + // JWT access tokens without `email` gets no signal that policies + // referencing `claim_email` read a value sourced from the AS-issued token + // rather than the upstream one (#5916). + // + // Claim keys only — never claim values, which hold user PII. + a.claimKeyLog.Do(func() { + slog.Warn("upstream token lacks identity claims; using the request token's values for Cedar evaluation", + "provider", a.primaryUpstreamProvider, + "claims", filled) + }) + } + a.logClaimKeys("upstream", merged) + return merged, nil +} + +// mirroredIdentityClaims are the user-identity claims the embedded auth server +// copies from the upstream OIDC identity into the access token it issues (see +// session.New in pkg/authserver/server/session/session.go: the JWT subject plus +// the UserClaims name/email pair). Because the AS-issued values are copies of the +// upstream ones, they are the only claims that can stand in for a missing +// upstream claim without misrepresenting its source. +// +// Keep this list in sync with session.New. Adding a claim here widens what a +// non-upstream token can contribute to a policy decision, so anything +// authorization-bearing (groups, roles, scopes) belongs nowhere near it — see +// resolveClaims for the full rationale. +var mirroredIdentityClaims = []string{"sub", "name", "email"} + +// supplementUpstreamClaims returns a fresh claim set holding every upstream claim +// plus, for each name in mirroredIdentityClaims that the upstream token does not +// carry, the request token's value for that name. It also returns the sorted list +// of names actually supplied by the request token, for logging. +// +// Neither input is mutated and the result aliases neither, so the caller may hand +// it to code that adds synthetic keys. Absent claims are never fabricated: a name +// missing from both sides is missing from the result, which keeps `has`-guarded +// policies failing closed. +func supplementUpstreamClaims(upstreamClaims, requestClaims jwt.MapClaims) (merged jwt.MapClaims, filled []string) { + merged = make(jwt.MapClaims, len(upstreamClaims)+len(mirroredIdentityClaims)) + for k, v := range upstreamClaims { + merged[k] = v + } + + for _, name := range mirroredIdentityClaims { + if _, ok := merged[name]; ok { + // The upstream IDP asserted this claim; it always wins. + continue } - a.logClaimKeys("upstream", parsedClaims) - return parsedClaims, nil + v, ok := requestClaims[name] + if !ok { + continue + } + merged[name] = v + filled = append(filled, name) } - // Default path: use claims from the original client request's token. - claims := jwt.MapClaims(identity.Claims) - a.logClaimKeys("token", claims) - return claims, nil + + // Sorted so the logged claim list is stable across runs (map iteration is + // not, and mirroredIdentityClaims is ordered for readability, not sorted). + slices.Sort(filled) + return merged, filled } // looksLikeJWT returns true when the token has the three-segment shape of a diff --git a/pkg/authz/authorizers/cedar/core_test.go b/pkg/authz/authorizers/cedar/core_test.go index f49a0428f7..f56d762cc0 100644 --- a/pkg/authz/authorizers/cedar/core_test.go +++ b/pkg/authz/authorizers/cedar/core_test.go @@ -8,6 +8,7 @@ import ( "context" "encoding/json" "log/slog" + "maps" "testing" cedar "github.com/cedar-policy/cedar-go" @@ -1470,7 +1471,7 @@ func TestAuthorizeWithJWTClaims_UpstreamProvider(t *testing.T) { errContains: "upstream token for provider", }, { - name: "upstream_token_has_no_sub_claim", + name: "upstream_token_has_no_sub_claim_uses_request_subject", identity: &auth.Identity{ PrincipalInfo: auth.PrincipalInfo{ Subject: "thv-user", @@ -1483,6 +1484,29 @@ func TestAuthorizeWithJWTClaims_UpstreamProvider(t *testing.T) { }), }, }, + // `sub` is one of the mirrored identity claims, so the request + // token's subject stands in and the principal resolves instead of + // failing with ErrMissingPrincipal (#5916). The policy requires + // claim_sub == "upstream-user", so this is still denied — but by + // policy evaluation, which is diagnosable, rather than by an + // authorization error on a request whose user is well known. + wantAuthorize: false, + }, + { + name: "upstream_token_has_no_sub_claim_and_neither_does_request", + identity: &auth.Identity{ + PrincipalInfo: auth.PrincipalInfo{ + Subject: "thv-user", + Claims: map[string]any{"email": "user@example.com"}, + }, + UpstreamTokens: map[string]string{ + providerName: makeUnsignedJWT(jwt.MapClaims{ + "iss": "https://idp.example.com", + }), + }, + }, + // No subject on either side: nothing is fabricated, so the principal + // genuinely cannot be resolved. wantErr: true, errContains: "missing principal", }, @@ -1516,6 +1540,306 @@ func TestAuthorizeWithJWTClaims_UpstreamProvider(t *testing.T) { } } +// TestSupplementUpstreamClaims covers which request-token claims may stand in for +// a missing upstream claim, and which may never do so. +func TestSupplementUpstreamClaims(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + upstreamClaims jwt.MapClaims + requestClaims jwt.MapClaims + wantMerged jwt.MapClaims + wantFilled []string + }{ + { + name: "upstream_wins_on_shared_identity_claims", + upstreamClaims: jwt.MapClaims{"sub": "okta|alice", "email": "alice@upstream.example"}, + requestClaims: jwt.MapClaims{"sub": "thv|alice", "email": "alice@thv.example"}, + wantMerged: jwt.MapClaims{"sub": "okta|alice", "email": "alice@upstream.example"}, + wantFilled: nil, + }, + { + name: "request_token_fills_missing_identity_claims", + upstreamClaims: jwt.MapClaims{"sub": "okta|alice", "iss": "https://idp.example.com"}, + requestClaims: jwt.MapClaims{"sub": "thv|alice", "email": "alice@example.com", "name": "Alice"}, + wantMerged: jwt.MapClaims{ + "sub": "okta|alice", + "iss": "https://idp.example.com", + "email": "alice@example.com", + "name": "Alice", + }, + // Sorted, so the logged claim list is stable across runs. + wantFilled: []string{"email", "name"}, + }, + { + // The core security boundary: authorization-bearing claims are + // upstream-only. A group on the ToolHive-issued token must not reach + // Cedar when an upstream provider is configured. + name: "authorization_bearing_claims_are_never_filled", + upstreamClaims: jwt.MapClaims{"sub": "okta|alice"}, + requestClaims: jwt.MapClaims{ + "groups": []interface{}{"platform-eng"}, + "roles": []interface{}{"admin"}, + "scope": "tools:write", + "hd": "example.com", + }, + wantMerged: jwt.MapClaims{"sub": "okta|alice"}, + wantFilled: nil, + }, + { + // The AS-issued token's own protocol claims describe that token, not + // the upstream identity, so they must not fill upstream gaps. + name: "as_token_protocol_claims_are_never_filled", + upstreamClaims: jwt.MapClaims{"sub": "okta|alice"}, + requestClaims: jwt.MapClaims{ + "iss": "https://thv-as.example.com/", + "aud": "toolhive", + "exp": 1234567890, + "jti": "abc", + "client_id": "vscode", + }, + wantMerged: jwt.MapClaims{"sub": "okta|alice"}, + wantFilled: nil, + }, + { + name: "subject_falls_back_when_upstream_token_has_no_sub", + upstreamClaims: jwt.MapClaims{"iss": "https://idp.example.com"}, + requestClaims: jwt.MapClaims{"sub": "thv|alice"}, + wantMerged: jwt.MapClaims{"iss": "https://idp.example.com", "sub": "thv|alice"}, + wantFilled: []string{"sub"}, + }, + { + // Never fabricate: a claim absent from both sides stays absent so + // `has`-guarded policies keep failing closed. + name: "claim_absent_from_both_stays_absent", + upstreamClaims: jwt.MapClaims{"sub": "okta|alice"}, + requestClaims: jwt.MapClaims{"sub": "thv|alice"}, + wantMerged: jwt.MapClaims{"sub": "okta|alice"}, + wantFilled: nil, + }, + { + // A key the upstream maps to nil is still "asserted by the upstream": + // presence, not truthiness, decides precedence. + name: "explicit_nil_upstream_value_still_wins", + upstreamClaims: jwt.MapClaims{"email": nil}, + requestClaims: jwt.MapClaims{"email": "alice@example.com"}, + wantMerged: jwt.MapClaims{"email": nil}, + wantFilled: nil, + }, + { + name: "nil_inputs_yield_empty_result", + upstreamClaims: nil, + requestClaims: nil, + wantMerged: jwt.MapClaims{}, + wantFilled: nil, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + // Snapshot the inputs so the no-mutation contract can be asserted. + upstreamBefore := maps.Clone(tt.upstreamClaims) + requestBefore := maps.Clone(tt.requestClaims) + + merged, filled := supplementUpstreamClaims(tt.upstreamClaims, tt.requestClaims) + + assert.Equal(t, tt.wantMerged, merged) + assert.Equal(t, tt.wantFilled, filled) + assert.Equal(t, upstreamBefore, tt.upstreamClaims, "upstream claims must not be mutated") + assert.Equal(t, requestBefore, tt.requestClaims, "request claims must not be mutated") + + // The result must not alias either input, since the caller adds + // synthetic keys to the claim set it receives. + merged["injected-by-caller"] = true + assert.NotContains(t, tt.upstreamClaims, "injected-by-caller") + assert.NotContains(t, tt.requestClaims, "injected-by-caller") + }) + } +} + +// TestResolveClaimsUpstreamSupplement pins the claim-source contract of +// resolveClaims on the embedded-auth-server path: the upstream token's claims +// win, and the request token supplies only the mirrored identity claims the +// upstream token omits. The email-filling case is the #5916 reproducer — an +// upstream IdP (JetBrains Hub is the public example) whose access token is a +// well-formed JWT that carries no `email`, which used to make every +// `claim_email` policy deny. +func TestResolveClaimsUpstreamSupplement(t *testing.T) { + t.Parallel() + + const providerName = "hub" + + tests := []struct { + name string + provider string + requestClaims map[string]any + upstreamToken string + wantClaims jwt.MapClaims + }{ + { + name: "upstream_jwt_without_email_falls_back_to_as_token_email", + provider: providerName, + // What the embedded auth server mirrors into the token it issues. + requestClaims: map[string]any{ + "sub": "hub|alice", + "email": "alice@example.com", + "name": "Alice", + "client_id": "vscode", + }, + upstreamToken: makeUnsignedJWT(jwt.MapClaims{ + "sub": "hub|alice", + "iss": "https://hub.example.com", + // No email: it lives in the id_token only. + }), + wantClaims: jwt.MapClaims{ + "sub": "hub|alice", + "iss": "https://hub.example.com", + "email": "alice@example.com", + "name": "Alice", + // client_id is absent: it describes the AS-issued token, not the + // upstream identity, so it never fills an upstream gap. + }, + }, + { + name: "upstream_claims_win_over_request_claims", + provider: providerName, + requestClaims: map[string]any{ + "sub": "thv|alice", + "email": "alice@thv.example", + "groups": []interface{}{"admins"}, + }, + upstreamToken: makeUnsignedJWT(jwt.MapClaims{ + "sub": "hub|alice", + "email": "alice@upstream.example", + "groups": []interface{}{"engineering"}, + }), + wantClaims: jwt.MapClaims{ + "sub": "hub|alice", + "email": "alice@upstream.example", + "groups": []interface{}{"engineering"}, + }, + }, + { + name: "no_provider_configured_uses_request_claims_verbatim", + provider: "", + requestClaims: map[string]any{"sub": "thv|alice", "email": "alice@thv.example"}, + // Present but irrelevant: without a configured provider the upstream + // token is never consulted. + upstreamToken: makeUnsignedJWT(jwt.MapClaims{"sub": "hub|alice"}), + wantClaims: jwt.MapClaims{"sub": "thv|alice", "email": "alice@thv.example"}, + }, + } + + 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: tt.provider, + }, "") + require.NoError(t, err) + + identity := &auth.Identity{ + PrincipalInfo: auth.PrincipalInfo{Subject: "thv|alice", Claims: tt.requestClaims}, + UpstreamTokens: map[string]string{providerName: tt.upstreamToken}, + } + claimsBefore := maps.Clone(tt.requestClaims) + + 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, tt.requestClaims, "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 +// against an upstream access token that is a well-formed JWT without `email`. +// Before the merge this denied every request for every user. +func TestAuthorizeWithJWTClaims_UpstreamJWTMissingIdentityClaim(t *testing.T) { + t.Parallel() + + const providerName = "hub" + + authorizer, err := NewCedarAuthorizer(ConfigOptions{ + Policies: []string{ + `forbid(principal, action, resource) + unless { principal has claim_email && principal.claim_email like "*@example.com" };`, + `permit(principal, action == Action::"call_tool", resource);`, + }, + EntitiesJSON: `[]`, + PrimaryUpstreamProvider: providerName, + }, "") + require.NoError(t, err) + + // A JWT-shaped upstream access token that parses cleanly but carries no + // identity claims beyond `sub`. looksLikeJWT is true here, so the #5147 + // opaque-token fallback does not apply — merging is what recovers `email`. + upstreamToken := makeUnsignedJWT(jwt.MapClaims{ + "sub": "hub|alice", + "iss": "https://hub.example.com", + }) + + tests := []struct { + name string + requestClaims map[string]any + wantAuthorize bool + }{ + { + // The AS-issued token mirrors the upstream id_token's email, so the + // domain gate can be satisfied even though the access token omits it. + name: "as_token_email_in_gated_domain_permits", + requestClaims: map[string]any{"sub": "hub|alice", "email": "alice@example.com"}, + wantAuthorize: true, + }, + { + // Merging supplies the claim; it does not weaken the gate. + name: "as_token_email_outside_gated_domain_denies", + requestClaims: map[string]any{"sub": "hub|alice", "email": "alice@evil.example"}, + wantAuthorize: false, + }, + { + // Neither token carries `email`: the `has` guard fails and the + // forbid applies. Merging never fabricates a claim. + name: "email_absent_from_both_tokens_denies", + requestClaims: map[string]any{"sub": "hub|alice"}, + wantAuthorize: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + identity := &auth.Identity{ + PrincipalInfo: auth.PrincipalInfo{Subject: "thv|alice", Claims: tt.requestClaims}, + UpstreamTokens: map[string]string{providerName: upstreamToken}, + } + ctx := auth.WithIdentity(context.Background(), identity) + + authorized, err := authorizer.AuthorizeWithJWTClaims( + ctx, + authorizers.MCPFeatureTool, + authorizers.MCPOperationCall, + "deploy", + nil, + ) + 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) { diff --git a/pkg/authz/integration_test.go b/pkg/authz/integration_test.go index 217ee072b7..f3ad7f5996 100644 --- a/pkg/authz/integration_test.go +++ b/pkg/authz/integration_test.go @@ -1187,3 +1187,137 @@ func TestIntegrationPrimaryUpstreamProviderClaimAttributeAccess(t *testing.T) { }) } } + +// TestIntegrationUpstreamJWTWithoutEmailClaim is the end-to-end reproducer for +// stacklok/toolhive#5916, driven through the full authorization middleware with +// the exact policy pair from the report. +// +// When the embedded auth server is active, the runner force-injects the first +// upstream provider as Cedar's PrimaryUpstreamProvider, so Cedar sources claims +// from the upstream IdP's access token. Providers that issue JWT-shaped access +// tokens without `email` (JetBrains Hub is a public example) used to make every +// request deny: the token parsed cleanly, so the opaque-token fallback added in +// #5147 did not apply, and `principal has claim_email` was false for every user. +// Opaque-token upstreams (Google, GitHub) masked the bug because their tokens hit +// that fallback and saw the AS-issued token's mirrored `email`. +// +// The AS-issued token carries the upstream `email` in both cases (the auth server +// mirrors it from the upstream id_token), so the fix lets it stand in for the +// claim the access token omits. +func TestIntegrationUpstreamJWTWithoutEmailClaim(t *testing.T) { + t.Parallel() + + const providerName = "hub" + + authorizer, err := cedar.NewCedarAuthorizer(cedar.ConfigOptions{ + // The report's policies verbatim: a defensively `has`-guarded domain + // gate plus a tool permit. + Policies: []string{ + `forbid(principal, action, resource) + unless { principal has claim_email && principal.claim_email like "*@example.com" };`, + `permit(principal, action == Action::"call_tool", resource);`, + }, + EntitiesJSON: "[]", + PrimaryUpstreamProvider: providerName, + }, "") + require.NoError(t, err) + + // Claims of the token the client actually presented, as minted by the + // embedded auth server: upstream subject plus the mirrored id_token profile. + asIssuedClaims := jwt.MapClaims{ + "sub": "hub|alice", + "email": "alice@example.com", + "name": "Alice", + "iss": "https://thv-as.example.com/", + "aud": "toolhive", + "client_id": "vscode", + } + + tests := []struct { + name string + upstreamToken string + expectAllowed bool + }{ + { + // The reported case: parses as a JWT, carries no `email`. + name: "jwt_upstream_token_without_email_uses_as_token_email", + upstreamToken: makeUnsignedJWT(t, jwt.MapClaims{ + "sub": "hub|alice", + "iss": "https://hub.example.com", + }), + expectAllowed: true, + }, + { + // The upstream's own `email` still wins where it asserts one, and it + // is outside the gated domain here, so the gate must reject. + name: "upstream_email_wins_over_as_token_email", + upstreamToken: makeUnsignedJWT(t, jwt.MapClaims{ + "sub": "hub|alice", + "email": "alice@contractor.example", + }), + expectAllowed: false, + }, + { + // Regression guard for the #5147 path, which this change leaves intact. + name: "opaque_upstream_token_still_falls_back", + upstreamToken: "ya29.opaque-google-style-token", + expectAllowed: true, + }, + { + // A JWT-shaped but unparseable upstream token must stay a hard deny: + // silently degrading a tampered token to other claims would let it + // bypass policy. + name: "tampered_upstream_jwt_still_denies", + upstreamToken: "not-base64.not-base64.not-base64", + expectAllowed: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + params, err := json.Marshal(map[string]interface{}{ + "name": "deploy", + "arguments": map[string]interface{}{}, + }) + require.NoError(t, err) + callReq, err := jsonrpc2.NewCall(jsonrpc2.Int64ID(1), string(mcp.MethodToolsCall), json.RawMessage(params)) + require.NoError(t, err) + reqJSON, err := jsonrpc2.EncodeMessage(callReq) + require.NoError(t, err) + + httpReq, err := http.NewRequest(http.MethodPost, "/messages", bytes.NewBuffer(reqJSON)) + require.NoError(t, err) + httpReq.Header.Set("Content-Type", "application/json") + + identity := &auth.Identity{ + PrincipalInfo: auth.PrincipalInfo{ + Subject: "hub|alice", + Claims: asIssuedClaims, + }, + UpstreamTokens: map[string]string{providerName: tt.upstreamToken}, + } + httpReq = httpReq.WithContext(auth.WithIdentity(httpReq.Context(), identity)) + + rr := httptest.NewRecorder() + var handlerCalled bool + mockHandler := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + handlerCalled = true + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{"result": "ok"}`)) + }) + + middleware := mcpparser.ParsingMiddleware(Middleware(authorizer, mockHandler, nil)) + middleware.ServeHTTP(rr, httpReq) + + if tt.expectAllowed { + assert.Equal(t, http.StatusOK, rr.Code) + assert.True(t, handlerCalled) + } else { + assert.Equal(t, http.StatusForbidden, rr.Code) + assert.False(t, handlerCalled) + } + }) + } +} From 12d32eaeae088559757718d9519876477f6da826 Mon Sep 17 00:00:00 2001 From: Juan Antonio Osorio Date: Mon, 27 Jul 2026 13:57:09 +0000 Subject: [PATCH 2/4] Fix codespell: unparseable -> unparsable Co-Authored-By: Claude Opus 5 (1M context) --- pkg/authz/integration_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/authz/integration_test.go b/pkg/authz/integration_test.go index f3ad7f5996..e7174f3634 100644 --- a/pkg/authz/integration_test.go +++ b/pkg/authz/integration_test.go @@ -1264,7 +1264,7 @@ func TestIntegrationUpstreamJWTWithoutEmailClaim(t *testing.T) { expectAllowed: true, }, { - // A JWT-shaped but unparseable upstream token must stay a hard deny: + // A JWT-shaped but unparsable upstream token must stay a hard deny: // silently degrading a tampered token to other claims would let it // bypass policy. name: "tampered_upstream_jwt_still_denies", From c9954548a067dccc018fc57ab969fced9250a37f Mon Sep 17 00:00:00 2001 From: Juan Antonio Osorio Date: Mon, 27 Jul 2026 15:41:34 +0000 Subject: [PATCH 3/4] Exclude sub from the upstream claim supplement MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review found the allowlist's premise does not hold for `sub`. The AS-issued subject is not a copy of the upstream one: it is an internal ToolHive user ID minted as a fresh UUID by UserResolver, which callback.go carries separately from the upstream subject. And unlike name/email, `sub` becomes the Cedar principal entity ID rather than an attribute, where Cedar offers no `has`-style guard. Supplementing it would silently retarget the principal instead of failing closed, so a forbid keyed on an upstream subject would stop matching with no diagnostic. Drop `sub`, leaving the supplement as the name/email pair session.New actually mirrors, and source those two from the session package's own constants so a rename on either side becomes a compile error. An upstream access token with no `sub` returns ErrMissingPrincipal again, as it did before this branch. Give the supplement warning its own rate limiter. Sharing claimKeyLog meant the warning consumed the 30s window and permanently starved the claim-key dump that follows it — suppressing the diagnostic on precisely the deployments whose upstream token lacks the claims. Also make captureSlogWarn concurrency-safe. It installs a capturing handler via the process-global slog.SetDefault, so any parallel test can write into its bytes.Buffer; keeping the capturing test non-parallel never prevented that. The new warning made the latent data race reproducible under -race. The buffer is now mutex-guarded, and the test keys on its specific message instead of on the buffer being empty, since foreign records can legitimately land there. Co-Authored-By: Claude Opus 5 (1M context) --- .../virtualmcpserver-kubernetes-guide.md | 22 +- pkg/authz/authorizers/cedar/core.go | 93 ++++--- pkg/authz/authorizers/cedar/core_test.go | 231 +++++++++++++----- pkg/authz/integration_test.go | 7 +- 4 files changed, 251 insertions(+), 102 deletions(-) diff --git a/docs/operator/virtualmcpserver-kubernetes-guide.md b/docs/operator/virtualmcpserver-kubernetes-guide.md index 75060505c9..54cef13443 100644 --- a/docs/operator/virtualmcpserver-kubernetes-guide.md +++ b/docs/operator/virtualmcpserver-kubernetes-guide.md @@ -653,10 +653,11 @@ spec: **Which token's claims a policy sees**: Cedar reads the primary upstream provider's *access token*. Claims that token asserts always win. Because many OIDC providers put profile claims in the `id_token` only and omit them from the -access token, the three user-identity claims the auth server mirrors into the -token it issues — `sub`, `name` and `email` — fall back to that mirrored value -when the upstream access token does not carry them. When this happens the proxy -logs, once per 30s: +access token, the two user-profile claims the auth server mirrors into the token +it issues — `name` and `email` — fall back to that mirrored value when the +upstream access token does not carry them. So `principal has claim_email` means +"known from a trusted source", not "asserted by the upstream IdP itself". When a +fallback happens the proxy logs, once per 30s: ``` WARN upstream token lacks identity claims; using the request token's values for @@ -666,10 +667,15 @@ WARN upstream token lacks identity claims; using the request token's values for Every other claim is upstream-only. In particular, group, role and scope claims are read from the upstream access token or not at all, so a policy such as `principal in THVGroup::"platform-eng"` only ever matches groups the upstream IdP -asserts. If a policy references a claim neither token carries, `principal has -claim_x` is false and the policy denies — author domain and group gates -defensively with `has`, and confirm the claim is present in the upstream access -token (not just the `id_token`) before gating on it. +asserts. The same holds for the principal itself: `sub` is never substituted, so a +rule keyed on `Client::""` keeps matching the upstream identity, +and an upstream access token with no `sub` is rejected outright rather than +falling back to ToolHive's internal user ID. + +If a policy references a claim neither token carries, `principal has claim_x` is +false and the policy denies — author domain and group gates defensively with +`has`, and confirm the claim is present in the upstream access token (not just the +`id_token`) before gating on it. > **Migration: `primaryUpstreamProvider` location** > diff --git a/pkg/authz/authorizers/cedar/core.go b/pkg/authz/authorizers/cedar/core.go index 146eccc78a..652822046d 100644 --- a/pkg/authz/authorizers/cedar/core.go +++ b/pkg/authz/authorizers/cedar/core.go @@ -19,6 +19,7 @@ import ( "github.com/golang-jwt/jwt/v5" "github.com/stacklok/toolhive/pkg/auth" + "github.com/stacklok/toolhive/pkg/authserver/server/session" "github.com/stacklok/toolhive/pkg/authz/authorizers" "github.com/stacklok/toolhive/pkg/syncutil" ) @@ -171,9 +172,8 @@ type Authorizer struct { // Mutex for thread safety mu sync.RWMutex // primaryUpstreamProvider names the upstream IDP provider whose access token - // is the primary source of JWT claims for Cedar evaluation. Its claims are - // merged over the request token's claims (upstream wins per key); see - // resolveClaims for the precedence rules and why they are not a plain replace. + // is the source of JWT claims for Cedar evaluation, aside from the narrow + // user-profile supplement described in resolveClaims. // 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 @@ -192,6 +192,11 @@ type Authorizer struct { // claimKeyLog rate-limits the diagnostic log of resolved JWT claim keys // so it emits at most once per 30 seconds instead of once per authorization check. claimKeyLog *syncutil.AtMost + // supplementLog rate-limits the warning that the upstream token lacked + // identity claims. It is deliberately separate from claimKeyLog: a shared + // timer would let whichever fires first suppress the other for the rest of + // the window, hiding the claim-key dump on exactly the deployments that need it. + supplementLog *syncutil.AtMost // multiValuedClaims lists JWT claim names normalized to a canonical unpadded // space-delimited string, plus a companion Cedar Set, before Cedar evaluation. // See ConfigOptions.MultiValuedClaims. @@ -211,11 +216,12 @@ type ConfigOptions struct { // When empty, claims from the ToolHive-issued token are used. // Must match an entry in identity.UpstreamTokens (e.g. "default", "github"). // - // The upstream token's claims do not replace the request token's claims: they - // are merged over them, so a claim the upstream access token omits (commonly - // `email`, which many providers place in the id_token only) falls back to the - // value the ToolHive-issued token carries, while every claim the upstream does - // assert wins. See resolveClaims for the full contract. + // The user-profile claims in mirroredIdentityClaims fall back to the + // ToolHive-issued token when the upstream access token omits them, so + // `principal has claim_email` means "known from a trusted source", not + // "asserted by the upstream IDP itself". Every other claim, including all + // group/role/scope claims, comes from the upstream token or not at all. 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 @@ -329,6 +335,7 @@ func NewCedarAuthorizer(options ConfigOptions, serverName string) (authorizers.A roleClaimName: options.RoleClaimName, serverName: serverName, claimKeyLog: syncutil.NewAtMost(30 * time.Second), + supplementLog: syncutil.NewAtMost(30 * time.Second), multiValuedClaims: options.MultiValuedClaims, } @@ -547,15 +554,14 @@ func (a *Authorizer) IsAuthorized( // // When primaryUpstreamProvider is set (the embedded auth server path), the // upstream IDP access token is the claim source, with one narrow supplement: the -// user-identity claims listed in mirroredIdentityClaims fall back to the request -// token's values when the upstream access token does not carry them. Using the -// upstream token alone was a deny-all trap, because many OIDC providers put -// identity claims such as `email` in the id_token only and omit them from the -// JWT access token (JetBrains Hub is a public example), so every policy -// referencing `claim_email` silently denied every request. See #5916. +// claims listed in mirroredIdentityClaims fall back to the request token's values +// when the upstream access token does not carry them. Using the upstream token +// alone was a deny-all trap, because many OIDC providers put identity claims such +// as `email` in the id_token only and omit them from the access token, so every +// policy referencing `claim_email` silently denied every request (#5916). // // The supplement is deliberately restricted to mirroredIdentityClaims rather -// than being a general merge of the two claim sets, for two reasons: +// than being a general merge of the two claim sets. Read this before widening it: // // - Authorization-bearing claims stay single-sourced. `groups`, `roles`, `hd` // and custom namespaced claims are asserted by the upstream IDP or not at @@ -565,6 +571,10 @@ func (a *Authorizer) IsAuthorized( // upstream identity. Letting `iss`, `aud`, `exp`, `jti` or `client_id` fill // upstream gaps would hand policies the auth server's own values under the // guise of upstream claims. +// - `sub` is excluded for both reasons at once: the AS-issued subject is an +// internal ToolHive user ID rather than a copy of the upstream subject, and +// it becomes the Cedar principal entity ID rather than a mere attribute (see +// mirroredIdentityClaims). // // Within mirroredIdentityClaims an upstream value always wins, so a claim the // upstream IDP does assert can never be shadowed by the token presented on the @@ -572,6 +582,11 @@ func (a *Authorizer) IsAuthorized( // signature-verified by the auth middleware before reaching any authorizer, and // the upstream token is held in server-side session state after the auth server's // code exchange, never supplied by the client. +// +// Note the resulting policy semantics: `principal has claim_email` (and +// claim_name) means "known from a trusted source", not "asserted by the upstream +// IDP itself". A claim absent from both sides stays absent, so `has`-guarded +// policies still fail closed. func (a *Authorizer) resolveClaims(identity *auth.Identity) (jwt.MapClaims, error) { requestClaims := jwt.MapClaims(identity.Claims) @@ -617,16 +632,12 @@ func (a *Authorizer) resolveClaims(identity *auth.Identity) (jwt.MapClaims, erro merged, filled := supplementUpstreamClaims(upstreamClaims, requestClaims) if len(filled) > 0 { - // Shares claimKeyLog with logClaimKeys so a deployment whose upstream - // token permanently lacks these claims does not emit a line per - // authorization check. This is the JWT-branch counterpart of the - // opaque-token Warn above: without it, an operator whose upstream issues - // JWT access tokens without `email` gets no signal that policies - // referencing `claim_email` read a value sourced from the AS-issued token - // rather than the upstream one (#5916). + // Rate-limited on its own timer, not claimKeyLog: sharing one would let + // this Warn consume the window and permanently starve the claim-key dump + // below, which is the thing you want when debugging a denying policy. // // Claim keys only — never claim values, which hold user PII. - a.claimKeyLog.Do(func() { + a.supplementLog.Do(func() { slog.Warn("upstream token lacks identity claims; using the request token's values for Cedar evaluation", "provider", a.primaryUpstreamProvider, "claims", filled) @@ -636,18 +647,28 @@ func (a *Authorizer) resolveClaims(identity *auth.Identity) (jwt.MapClaims, erro return merged, nil } -// mirroredIdentityClaims are the user-identity claims the embedded auth server -// copies from the upstream OIDC identity into the access token it issues (see -// session.New in pkg/authserver/server/session/session.go: the JWT subject plus -// the UserClaims name/email pair). Because the AS-issued values are copies of the -// upstream ones, they are the only claims that can stand in for a missing -// upstream claim without misrepresenting its source. +// mirroredIdentityClaims are the claims that may stand in for a missing upstream +// claim: the user-profile pair the embedded auth server copies verbatim from the +// upstream OIDC identity into the token it issues (session.New in +// pkg/authserver/server/session). Referencing that package's constants makes a +// rename on either side a compile error; it does not catch a claim being ADDED to +// session.New, which still needs a deliberate decision here. +// +// `sub` is deliberately NOT in this list even though session.New sets a subject. +// The AS-issued subject is an internal ToolHive user ID — a fresh UUID from +// UserResolver (pkg/authserver/server/handlers/user.go), distinct from the +// upstream subject, which travels separately as UpstreamSubject +// (handlers/callback.go). It is also the one identity claim that becomes the Cedar +// principal entity ID via extractClientIDFromClaims rather than an attribute, and +// Cedar has no `has`-style guard in the principal position. Supplementing it +// would silently retarget the principal instead of failing closed, so a `forbid` +// keyed on an upstream subject would stop matching with no diagnostic. An upstream +// access token with no `sub` violates RFC 9068 and is better surfaced as +// ErrMissingPrincipal. // -// Keep this list in sync with session.New. Adding a claim here widens what a -// non-upstream token can contribute to a policy decision, so anything -// authorization-bearing (groups, roles, scopes) belongs nowhere near it — see -// resolveClaims for the full rationale. -var mirroredIdentityClaims = []string{"sub", "name", "email"} +// Widening this list widens what a non-upstream token can contribute to a policy +// decision — see resolveClaims before adding anything. +var mirroredIdentityClaims = []string{session.NameClaimKey, session.EmailClaimKey} // supplementUpstreamClaims returns a fresh claim set holding every upstream claim // plus, for each name in mirroredIdentityClaims that the upstream token does not @@ -677,8 +698,8 @@ func supplementUpstreamClaims(upstreamClaims, requestClaims jwt.MapClaims) (merg filled = append(filled, name) } - // Sorted so the logged claim list is stable across runs (map iteration is - // not, and mirroredIdentityClaims is ordered for readability, not sorted). + // Sorted for a canonical, easily-greppable log line — mirroredIdentityClaims is + // ordered for readability (name, email), not alphabetically. slices.Sort(filled) return merged, filled } diff --git a/pkg/authz/authorizers/cedar/core_test.go b/pkg/authz/authorizers/cedar/core_test.go index f56d762cc0..16908c7538 100644 --- a/pkg/authz/authorizers/cedar/core_test.go +++ b/pkg/authz/authorizers/cedar/core_test.go @@ -9,6 +9,7 @@ import ( "encoding/json" "log/slog" "maps" + "sync" "testing" cedar "github.com/cedar-policy/cedar-go" @@ -1471,7 +1472,7 @@ func TestAuthorizeWithJWTClaims_UpstreamProvider(t *testing.T) { errContains: "upstream token for provider", }, { - name: "upstream_token_has_no_sub_claim_uses_request_subject", + name: "upstream_token_has_no_sub_claim", identity: &auth.Identity{ PrincipalInfo: auth.PrincipalInfo{ Subject: "thv-user", @@ -1484,29 +1485,11 @@ func TestAuthorizeWithJWTClaims_UpstreamProvider(t *testing.T) { }), }, }, - // `sub` is one of the mirrored identity claims, so the request - // token's subject stands in and the principal resolves instead of - // failing with ErrMissingPrincipal (#5916). The policy requires - // claim_sub == "upstream-user", so this is still denied — but by - // policy evaluation, which is diagnosable, rather than by an - // authorization error on a request whose user is well known. - wantAuthorize: false, - }, - { - name: "upstream_token_has_no_sub_claim_and_neither_does_request", - identity: &auth.Identity{ - PrincipalInfo: auth.PrincipalInfo{ - Subject: "thv-user", - Claims: map[string]any{"email": "user@example.com"}, - }, - UpstreamTokens: map[string]string{ - providerName: makeUnsignedJWT(jwt.MapClaims{ - "iss": "https://idp.example.com", - }), - }, - }, - // No subject on either side: nothing is fabricated, so the principal - // genuinely cannot be resolved. + // `sub` is excluded from mirroredIdentityClaims, so the request + // token's subject must NOT stand in: the AS-issued subject is an + // internal user ID, and supplementing it would silently retarget the + // Cedar principal instead of failing closed. An upstream access token + // with no `sub` violates RFC 9068 and stays an error. wantErr: true, errContains: "missing principal", }, @@ -1553,16 +1536,22 @@ func TestSupplementUpstreamClaims(t *testing.T) { wantFilled []string }{ { - name: "upstream_wins_on_shared_identity_claims", - upstreamClaims: jwt.MapClaims{"sub": "okta|alice", "email": "alice@upstream.example"}, - requestClaims: jwt.MapClaims{"sub": "thv|alice", "email": "alice@thv.example"}, - wantMerged: jwt.MapClaims{"sub": "okta|alice", "email": "alice@upstream.example"}, - wantFilled: nil, + name: "upstream_wins_on_shared_identity_claims", + upstreamClaims: jwt.MapClaims{ + "sub": "okta|alice", "email": "alice@upstream.example", "name": "Upstream Alice", + }, + requestClaims: jwt.MapClaims{ + "sub": "7f3c1e64-uuid", "email": "alice@thv.example", "name": "THV Alice", + }, + wantMerged: jwt.MapClaims{ + "sub": "okta|alice", "email": "alice@upstream.example", "name": "Upstream Alice", + }, + wantFilled: nil, }, { name: "request_token_fills_missing_identity_claims", upstreamClaims: jwt.MapClaims{"sub": "okta|alice", "iss": "https://idp.example.com"}, - requestClaims: jwt.MapClaims{"sub": "thv|alice", "email": "alice@example.com", "name": "Alice"}, + requestClaims: jwt.MapClaims{"sub": "7f3c1e64-uuid", "email": "alice@example.com", "name": "Alice"}, wantMerged: jwt.MapClaims{ "sub": "okta|alice", "iss": "https://idp.example.com", @@ -1572,6 +1561,20 @@ func TestSupplementUpstreamClaims(t *testing.T) { // Sorted, so the logged claim list is stable across runs. wantFilled: []string{"email", "name"}, }, + { + // `sub` is excluded from mirroredIdentityClaims: the AS-issued subject + // is an internal ToolHive user ID, not a copy of the upstream one, and + // it becomes the Cedar principal entity ID. Supplementing it would + // silently retarget the principal rather than fail closed. + name: "subject_is_never_filled_from_the_request_token", + upstreamClaims: jwt.MapClaims{"iss": "https://idp.example.com"}, + requestClaims: jwt.MapClaims{"sub": "7f3c1e64-uuid", "email": "alice@example.com"}, + wantMerged: jwt.MapClaims{ + "iss": "https://idp.example.com", + "email": "alice@example.com", + }, + wantFilled: []string{"email"}, + }, { // The core security boundary: authorization-bearing claims are // upstream-only. A group on the ToolHive-issued token must not reach @@ -1602,19 +1605,12 @@ func TestSupplementUpstreamClaims(t *testing.T) { wantMerged: jwt.MapClaims{"sub": "okta|alice"}, wantFilled: nil, }, - { - name: "subject_falls_back_when_upstream_token_has_no_sub", - upstreamClaims: jwt.MapClaims{"iss": "https://idp.example.com"}, - requestClaims: jwt.MapClaims{"sub": "thv|alice"}, - wantMerged: jwt.MapClaims{"iss": "https://idp.example.com", "sub": "thv|alice"}, - wantFilled: []string{"sub"}, - }, { // Never fabricate: a claim absent from both sides stays absent so // `has`-guarded policies keep failing closed. name: "claim_absent_from_both_stays_absent", upstreamClaims: jwt.MapClaims{"sub": "okta|alice"}, - requestClaims: jwt.MapClaims{"sub": "thv|alice"}, + requestClaims: jwt.MapClaims{"sub": "7f3c1e64-uuid"}, wantMerged: jwt.MapClaims{"sub": "okta|alice"}, wantFilled: nil, }, @@ -1682,9 +1678,11 @@ func TestResolveClaimsUpstreamSupplement(t *testing.T) { { name: "upstream_jwt_without_email_falls_back_to_as_token_email", provider: providerName, - // What the embedded auth server mirrors into the token it issues. + // What the embedded auth server mirrors into the token it issues. The + // AS subject is an internal ToolHive user ID (a UserResolver UUID), + // deliberately NOT the upstream subject. requestClaims: map[string]any{ - "sub": "hub|alice", + "sub": "7f3c1e64-9b2a-4d51-8e77-1c0a5f3b9d42", "email": "alice@example.com", "name": "Alice", "client_id": "vscode", @@ -1707,7 +1705,7 @@ func TestResolveClaimsUpstreamSupplement(t *testing.T) { name: "upstream_claims_win_over_request_claims", provider: providerName, requestClaims: map[string]any{ - "sub": "thv|alice", + "sub": "7f3c1e64-9b2a-4d51-8e77-1c0a5f3b9d42", "email": "alice@thv.example", "groups": []interface{}{"admins"}, }, @@ -1725,11 +1723,11 @@ func TestResolveClaimsUpstreamSupplement(t *testing.T) { { name: "no_provider_configured_uses_request_claims_verbatim", provider: "", - requestClaims: map[string]any{"sub": "thv|alice", "email": "alice@thv.example"}, + requestClaims: map[string]any{"sub": "7f3c1e64-uuid", "email": "alice@thv.example"}, // Present but irrelevant: without a configured provider the upstream // token is never consulted. upstreamToken: makeUnsignedJWT(jwt.MapClaims{"sub": "hub|alice"}), - wantClaims: jwt.MapClaims{"sub": "thv|alice", "email": "alice@thv.example"}, + wantClaims: jwt.MapClaims{"sub": "7f3c1e64-uuid", "email": "alice@thv.example"}, }, } @@ -1745,7 +1743,7 @@ func TestResolveClaimsUpstreamSupplement(t *testing.T) { require.NoError(t, err) identity := &auth.Identity{ - PrincipalInfo: auth.PrincipalInfo{Subject: "thv|alice", Claims: tt.requestClaims}, + PrincipalInfo: auth.PrincipalInfo{Subject: "7f3c1e64-uuid", Claims: tt.requestClaims}, UpstreamTokens: map[string]string{providerName: tt.upstreamToken}, } claimsBefore := maps.Clone(tt.requestClaims) @@ -1799,20 +1797,20 @@ func TestAuthorizeWithJWTClaims_UpstreamJWTMissingIdentityClaim(t *testing.T) { // The AS-issued token mirrors the upstream id_token's email, so the // domain gate can be satisfied even though the access token omits it. name: "as_token_email_in_gated_domain_permits", - requestClaims: map[string]any{"sub": "hub|alice", "email": "alice@example.com"}, + requestClaims: map[string]any{"sub": "7f3c1e64-uuid", "email": "alice@example.com"}, wantAuthorize: true, }, { // Merging supplies the claim; it does not weaken the gate. name: "as_token_email_outside_gated_domain_denies", - requestClaims: map[string]any{"sub": "hub|alice", "email": "alice@evil.example"}, + requestClaims: map[string]any{"sub": "7f3c1e64-uuid", "email": "alice@evil.example"}, wantAuthorize: false, }, { // Neither token carries `email`: the `has` guard fails and the // forbid applies. Merging never fabricates a claim. name: "email_absent_from_both_tokens_denies", - requestClaims: map[string]any{"sub": "hub|alice"}, + requestClaims: map[string]any{"sub": "7f3c1e64-uuid"}, wantAuthorize: false, }, } @@ -1822,7 +1820,7 @@ func TestAuthorizeWithJWTClaims_UpstreamJWTMissingIdentityClaim(t *testing.T) { t.Parallel() identity := &auth.Identity{ - PrincipalInfo: auth.PrincipalInfo{Subject: "thv|alice", Claims: tt.requestClaims}, + PrincipalInfo: auth.PrincipalInfo{Subject: "7f3c1e64-uuid", Claims: tt.requestClaims}, UpstreamTokens: map[string]string{providerName: upstreamToken}, } ctx := auth.WithIdentity(context.Background(), identity) @@ -1840,6 +1838,96 @@ func TestAuthorizeWithJWTClaims_UpstreamJWTMissingIdentityClaim(t *testing.T) { } } +// TestAuthorizeWithJWTClaims_PrincipalStaysUpstreamSourced pins the property that +// keeps `sub` out of mirroredIdentityClaims: the Cedar principal entity ID is +// derived from the upstream subject, never from the AS-issued token's internal +// user ID. Unlike an attribute, the principal has no `has`-style guard available +// in Cedar, so a supplemented subject would silently retarget every principal-keyed +// rule instead of failing closed. +func TestAuthorizeWithJWTClaims_PrincipalStaysUpstreamSourced(t *testing.T) { + t.Parallel() + + const providerName = "hub" + + // A banned-user rule keyed on the upstream subject, plus a broad permit that + // would otherwise let the request through. If the principal were ever built + // from the AS-issued subject, the forbid would stop matching and the permit + // would apply — the exact silent-widening this guards against. + authorizer, err := NewCedarAuthorizer(ConfigOptions{ + Policies: []string{ + `forbid(principal == Client::"hub|banned-alice", action, resource);`, + `permit(principal, action == Action::"call_tool", resource);`, + }, + EntitiesJSON: `[]`, + PrimaryUpstreamProvider: providerName, + }, "") + require.NoError(t, err) + + // The AS-issued subject: a UserResolver UUID, unrelated to the upstream one. + const asSubject = "7f3c1e64-9b2a-4d51-8e77-1c0a5f3b9d42" + + tests := []struct { + name string + upstreamClaims jwt.MapClaims + wantAuthorize bool + wantErr bool + }{ + { + name: "banned_upstream_subject_is_forbidden", + upstreamClaims: jwt.MapClaims{"sub": "hub|banned-alice"}, + wantAuthorize: false, + }, + { + name: "other_upstream_subject_is_permitted", + upstreamClaims: jwt.MapClaims{"sub": "hub|bob"}, + wantAuthorize: true, + }, + { + // The sharp case. If `sub` were added to mirroredIdentityClaims, the + // AS subject would stand in, the principal would become + // Client::"7f3c1e64-..." and the broad permit would apply — the + // request would be ALLOWED instead of erroring. Fail closed instead. + name: "missing_upstream_subject_fails_closed_not_to_the_as_subject", + upstreamClaims: jwt.MapClaims{"iss": "https://hub.example.com"}, + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + identity := &auth.Identity{ + PrincipalInfo: auth.PrincipalInfo{ + Subject: asSubject, + Claims: map[string]any{"sub": asSubject, "email": "alice@example.com"}, + }, + UpstreamTokens: map[string]string{ + providerName: makeUnsignedJWT(tt.upstreamClaims), + }, + } + 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) { @@ -3959,14 +4047,42 @@ func TestNewCedarAuthorizerGroupEntityTypeValidation(t *testing.T) { } } -// captureSlogWarn redirects slog's default logger to a bytes.Buffer for the -// duration of f, then restores the original default. Returns the captured -// output. This helper exists because slog.SetDefault is a process-global -// side effect — tests that use it must NOT run in parallel. +// syncBuffer is a concurrency-safe io.Writer over a bytes.Buffer. +// +// captureSlogWarn needs one because slog.SetDefault is process-global: while the +// capturing handler is installed, ANY goroutine in the test binary can emit a +// record into it. Keeping the capturing test itself non-parallel does not prevent +// that — other tests already running in parallel still log — so the buffer must +// be safe on its own. A plain bytes.Buffer here is a data race that only shows up +// once some concurrently-reachable code path starts logging. +type syncBuffer struct { + mu sync.Mutex + buf bytes.Buffer +} + +func (b *syncBuffer) Write(p []byte) (int, error) { + b.mu.Lock() + defer b.mu.Unlock() + return b.buf.Write(p) +} + +func (b *syncBuffer) String() string { + b.mu.Lock() + defer b.mu.Unlock() + return b.buf.String() +} + +// captureSlogWarn redirects slog's default logger to a buffer for the duration of +// f, then restores the original default, and returns the captured output. +// +// Because slog.SetDefault is process-global, the returned output may also contain +// records emitted by other tests running concurrently. Callers must therefore +// assert on the specific record they expect (or its specific absence) and never +// on the output being empty overall. func captureSlogWarn(t *testing.T, f func()) string { t.Helper() - var buf bytes.Buffer + var buf syncBuffer handler := slog.NewJSONHandler(&buf, &slog.HandlerOptions{Level: slog.LevelWarn}) orig := slog.Default() slog.SetDefault(slog.New(handler)) @@ -4030,14 +4146,19 @@ func TestStaleTHVGroupWarning(t *testing.T) { require.NoError(t, err) }) + // Keyed on this specific warning rather than on the buffer being + // empty: slog.Default is process-global, so concurrently-running + // tests can also land records in the captured output. + const staleWarn = "Cedar entities_json contains THVGroup entities" + if tt.wantWarn { - require.NotEmpty(t, output, "expected a warn log") + require.Contains(t, output, staleWarn, "expected the stale-THVGroup warn log") for _, want := range tt.wantContains { assert.Contains(t, output, want, "warn log must mention %q", want) } } else { - assert.Empty(t, output, "no warning expected") + assert.NotContains(t, output, staleWarn, "no stale-THVGroup warning expected") } }) } diff --git a/pkg/authz/integration_test.go b/pkg/authz/integration_test.go index e7174f3634..1a320aa32b 100644 --- a/pkg/authz/integration_test.go +++ b/pkg/authz/integration_test.go @@ -1223,9 +1223,10 @@ func TestIntegrationUpstreamJWTWithoutEmailClaim(t *testing.T) { require.NoError(t, err) // Claims of the token the client actually presented, as minted by the - // embedded auth server: upstream subject plus the mirrored id_token profile. + // embedded auth server: an internal ToolHive user ID (a UserResolver UUID, + // deliberately NOT the upstream subject) plus the mirrored id_token profile. asIssuedClaims := jwt.MapClaims{ - "sub": "hub|alice", + "sub": "7f3c1e64-9b2a-4d51-8e77-1c0a5f3b9d42", "email": "alice@example.com", "name": "Alice", "iss": "https://thv-as.example.com/", @@ -1293,7 +1294,7 @@ func TestIntegrationUpstreamJWTWithoutEmailClaim(t *testing.T) { identity := &auth.Identity{ PrincipalInfo: auth.PrincipalInfo{ - Subject: "hub|alice", + Subject: "7f3c1e64-9b2a-4d51-8e77-1c0a5f3b9d42", Claims: asIssuedClaims, }, UpstreamTokens: map[string]string{providerName: tt.upstreamToken}, From c9431958244ef34848f470e18ef1ff45cdcfa1c7 Mon Sep 17 00:00:00 2001 From: Juan Antonio Osorio Date: Mon, 27 Jul 2026 15:59:24 +0000 Subject: [PATCH 4/4] Source the profile-claim fallback from the upstream ID token MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Research into the review question found that supplementing from the ToolHive-issued token is not merely less principled but wrong in a supported configuration. Multi-upstream chains are first-class: validateChain requires chain[0] to be the first configured upstream, and only that first leg resolves identity from its provider (callback.go), with later legs carrying those values forward. The name/email the auth server mirrors into the token it issues therefore always belong to the FIRST configured upstream. Meanwhile primaryUpstreamProvider may name any upstream — the operator validates membership, not position, and the guide documents pinning explicitly. Pinning a non-first provider made the supplement attribute the identity provider's email to a different IdP, silently, with no diagnostic. verifyChainIdentity keeps it the same person, but a gate distinguishing a corporate from a personal IdP is exactly the policy that would be subverted. Read the fallback from the same provider's stored id_token instead. It is keyed by the same provider name and projected from the same credential bundle, so provenance holds by construction, and `principal has claim_email` goes back to meaning "this upstream asserted an email" rather than "some trusted party did". No coverage is lost on the reported bug: an OIDC upstream cannot log in without an id_token (upstream/oidc.go rejects it) and that id_token is where the auth server read name/email from in the first place, so the reporter's provider supplies the identical value with better attribution. Drop the ToolHive-issued token as a claim source on this path entirely rather than keeping it as a third tier. Making the tier sound would require knowing whether the pinned provider is the identity provider, which is not derivable here, and its only remaining value is an OAuth 2.0 upstream that issues JWT access tokens while an id_token was never captured — where the claim genuinely is not available from that provider. The result is less code than the tier it replaces, and it retires the "known from a trusted source" caveat that previously had to be documented in three places. The id_token is used without checking `exp`, deliberately. It is read as a record of what the upstream asserted at login, not presented as a credential; the "callers MUST check exp" contract exists for RFC 8693 subject-token use and the owning service says enforcement belongs at that consumer. Enforcing it here would be actively harmful, since sessions live for the refresh-token lifespan (7 days by default) while id_tokens expire in minutes, so a policy would permit early in a session and silently deny later. The claims are no staler than the alternative: the mirrored profile claims are also captured once at login. Both guarantees are covered by tests that fail if reverted: reintroducing the issued-token source breaks five cases across both packages, and adding `sub` back breaks the principal guard. Co-Authored-By: Claude Opus 5 (1M context) --- .../virtualmcpserver-kubernetes-guide.md | 52 ++- pkg/authz/authorizers/cedar/core.go | 206 +++++++---- pkg/authz/authorizers/cedar/core_test.go | 332 +++++++++++------- pkg/authz/integration_test.go | 64 +++- 4 files changed, 420 insertions(+), 234 deletions(-) diff --git a/docs/operator/virtualmcpserver-kubernetes-guide.md b/docs/operator/virtualmcpserver-kubernetes-guide.md index 54cef13443..b186cfcdc5 100644 --- a/docs/operator/virtualmcpserver-kubernetes-guide.md +++ b/docs/operator/virtualmcpserver-kubernetes-guide.md @@ -653,29 +653,45 @@ spec: **Which token's claims a policy sees**: Cedar reads the primary upstream provider's *access token*. Claims that token asserts always win. Because many OIDC providers put profile claims in the `id_token` only and omit them from the -access token, the two user-profile claims the auth server mirrors into the token -it issues — `name` and `email` — fall back to that mirrored value when the -upstream access token does not carry them. So `principal has claim_email` means -"known from a trusted source", not "asserted by the upstream IdP itself". When a -fallback happens the proxy logs, once per 30s: +access token, `name` and `email` fall back to **that same provider's `id_token`** +(captured at login and stored alongside its access token) when the access token +does not carry them. Provenance is preserved either way: `principal has +claim_email` means "this upstream asserted an email". When a fallback happens the +proxy logs, once per 30s: ``` -WARN upstream token lacks identity claims; using the request token's values for - Cedar evaluation provider=okta claims="[email]" +WARN upstream access token lacks profile claims; using the upstream ID token's + values for Cedar evaluation provider=okta claims="[email]" ``` -Every other claim is upstream-only. In particular, group, role and scope claims -are read from the upstream access token or not at all, so a policy such as -`principal in THVGroup::"platform-eng"` only ever matches groups the upstream IdP -asserts. The same holds for the principal itself: `sub` is never substituted, so a -rule keyed on `Client::""` keeps matching the upstream identity, -and an upstream access token with no `sub` is rejected outright rather than -falling back to ToolHive's internal user ID. +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. -If a policy references a claim neither token carries, `principal has claim_x` is -false and the policy denies — author domain and group gates defensively with -`has`, and confirm the claim is present in the upstream access token (not just the -`id_token`) before gating on it. +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: + +``` +WARN no upstream ID token stored for provider; policies referencing profile + claims the access token omits will deny provider=okta +``` + +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. + +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 +and group gates defensively with `has`, and confirm the claim is present in one of +those two tokens before gating on it. > **Migration: `primaryUpstreamProvider` location** > diff --git a/pkg/authz/authorizers/cedar/core.go b/pkg/authz/authorizers/cedar/core.go index 652822046d..aa03265344 100644 --- a/pkg/authz/authorizers/cedar/core.go +++ b/pkg/authz/authorizers/cedar/core.go @@ -192,11 +192,16 @@ type Authorizer struct { // claimKeyLog rate-limits the diagnostic log of resolved JWT claim keys // so it emits at most once per 30 seconds instead of once per authorization check. claimKeyLog *syncutil.AtMost - // supplementLog rate-limits the warning that the upstream token lacked - // identity claims. It is deliberately separate from claimKeyLog: a shared + // supplementLog rate-limits the warning that the upstream access token lacked + // profile claims. It is deliberately separate from claimKeyLog: a shared // timer would let whichever fires first suppress the other for the rest of // 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. + missingIDTokenLog *syncutil.AtMost // multiValuedClaims lists JWT claim names normalized to a canonical unpadded // space-delimited string, plus a companion Cedar Set, before Cedar evaluation. // See ConfigOptions.MultiValuedClaims. @@ -216,12 +221,13 @@ type ConfigOptions struct { // When empty, claims from the ToolHive-issued token are used. // Must match an entry in identity.UpstreamTokens (e.g. "default", "github"). // - // The user-profile claims in mirroredIdentityClaims fall back to the - // ToolHive-issued token when the upstream access token omits them, so - // `principal has claim_email` means "known from a trusted source", not - // "asserted by the upstream IDP itself". Every other claim, including all - // group/role/scope claims, comes from the upstream token or not at all. See - // resolveClaims for the full contract. + // 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. PrimaryUpstreamProvider string `json:"primary_upstream_provider,omitempty" yaml:"primary_upstream_provider,omitempty"` // GroupClaimName is the JWT claim key that contains group membership for the @@ -336,6 +342,7 @@ func NewCedarAuthorizer(options ConfigOptions, serverName string) (authorizers.A serverName: serverName, claimKeyLog: syncutil.NewAtMost(30 * time.Second), supplementLog: syncutil.NewAtMost(30 * time.Second), + missingIDTokenLog: syncutil.NewAtMost(30 * time.Second), multiValuedClaims: options.MultiValuedClaims, } @@ -552,41 +559,57 @@ func (a *Authorizer) IsAuthorized( // the original client request are used as-is. That may be a ToolHive-issued token // or any other bearer token. // -// When primaryUpstreamProvider is set (the embedded auth server path), the -// upstream IDP access token is the claim source, with one narrow supplement: the -// claims listed in mirroredIdentityClaims fall back to the request token's values -// when the upstream access token does not carry them. Using the upstream token -// alone was a deny-all trap, because many OIDC providers put identity claims such -// as `email` in the id_token only and omit them from the access token, so every -// policy referencing `claim_email` silently denied every request (#5916). +// When primaryUpstreamProvider is set (the embedded auth server path), the claim +// source is that provider's upstream credentials — and only that provider's. Its +// access token is primary; the profile claims listed in profileClaimsFromIDToken +// fall back to the same provider's id_token when the access token does not carry +// them. The access token alone was a deny-all trap, because many OIDC providers +// put identity claims such as `email` in the id_token only and omit them from the +// access token, so every policy referencing `claim_email` silently denied every +// request (#5916). // -// The supplement is deliberately restricted to mirroredIdentityClaims rather -// than being a general merge of the two claim sets. Read this before widening it: +// Both tokens come from the same upstream login for the same provider name +// (projected side by side from one credential bundle in TokenValidator.Middleware), +// so a supplemented claim carries the same provenance as one read directly from +// the access token: `principal has claim_email` means "this upstream asserted an +// email", never "some other trusted party did". That is why the ToolHive-issued +// token the client presented is NOT a claim source on this path, even though it +// mirrors the upstream name/email: in a multi-upstream chain those mirrored values +// come from the FIRST configured upstream (the identity provider — see +// handlers/callback.go, and validateChain, which requires chain[0] to be +// upstreams[0]), which need not be the pinned provider. Using them would +// attribute the identity provider's email to a different IdP. // -// - Authorization-bearing claims stay single-sourced. `groups`, `roles`, `hd` -// and custom namespaced claims are asserted by the upstream IDP or not at -// all; a group on the ToolHive-issued token must never grant access that the -// upstream did not assert. -// - Everything else on the AS-issued token describes that token, not the -// upstream identity. Letting `iss`, `aud`, `exp`, `jti` or `client_id` fill -// upstream gaps would hand policies the auth server's own values under the -// guise of upstream claims. -// - `sub` is excluded for both reasons at once: the AS-issued subject is an -// internal ToolHive user ID rather than a copy of the upstream subject, and -// it becomes the Cedar principal entity ID rather than a mere attribute (see -// mirroredIdentityClaims). +// The supplement is deliberately restricted to profileClaimsFromIDToken rather +// than being a general merge of the two token bodies. Read this before widening it: // -// Within mirroredIdentityClaims an upstream value always wins, so a claim the -// upstream IDP does assert can never be shadowed by the token presented on the -// request. Both sources are server-side trusted: the request token's claims were -// signature-verified by the auth middleware before reaching any authorizer, and -// the upstream token is held in server-side session state after the auth server's -// code exchange, never supplied by the client. +// - An id_token's registered claims (`iss`, `aud`, `exp`, `nonce`, `at_hash`, +// `azp`) describe that token, not the user. Merging them would overwrite the +// access token's own `aud`/`exp` with values that mean something different. +// - Authorization-bearing claims (`groups`, `roles`, `scope`) are left alone for +// now. Taking them from this provider's id_token would be provenance-correct — +// unlike taking them from the ToolHive-issued token, which must never happen — +// but it would newly grant requests that deny today, so it belongs in its own +// change rather than riding along with a deny-all fix. +// - `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. // -// Note the resulting policy semantics: `principal has claim_email` (and -// claim_name) means "known from a trusted source", not "asserted by the upstream -// IDP itself". A claim absent from both sides stays absent, so `has`-guarded +// 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 // policies still fail closed. +// +// The stored id_token is used without checking `exp`, deliberately. It is read +// here as a record of what the upstream asserted at login, not presented as a +// credential — the "callers MUST check exp" contract on UpstreamCredential.IDToken +// exists for RFC 8693 subject-token use, and the service that owns the field says +// so (see the TODO in pkg/auth/upstreamtoken/service.go). Enforcing it here would +// be actively harmful: sessions live for the refresh-token lifespan (7 days by +// default) while id_tokens expire in minutes, so a policy would permit early in a +// session and silently deny later. The claims are no staler than the alternative — +// the ToolHive-issued token's mirrored profile claims are also captured once at +// login and never refreshed. func (a *Authorizer) resolveClaims(identity *auth.Identity) (jwt.MapClaims, error) { requestClaims := jwt.MapClaims(identity.Claims) @@ -630,67 +653,94 @@ func (a *Authorizer) resolveClaims(identity *auth.Identity) (jwt.MapClaims, erro a.primaryUpstreamProvider, err) } - merged, filled := supplementUpstreamClaims(upstreamClaims, requestClaims) + merged := a.supplementFromIDToken(identity, upstreamClaims) + a.logClaimKeys("upstream", merged) + return merged, nil +} + +// 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. +// +// A provider with no stored id_token (an OAuth 2.0 upstream that was never asked +// for `openid`, so nothing was captured at login) yields no supplement: the claims +// stay absent and `has`-guarded policies deny. OIDC upstreams always have one — +// the provider rejects a login without it (upstream/oidc.go) and a refresh that +// omits a rotated id_token carries the original forward (upstreamtoken/service.go), +// so it does not vanish mid-session. +func (a *Authorizer) supplementFromIDToken(identity *auth.Identity, upstreamClaims jwt.MapClaims) jwt.MapClaims { + idToken := identity.UpstreamIDTokens[a.primaryUpstreamProvider] // nil map safe in Go + if idToken == "" { + a.missingIDTokenLog.Do(func() { + slog.Warn("no upstream ID token stored for provider; policies referencing profile claims "+ + "the access token omits will deny", + "provider", a.primaryUpstreamProvider, + "profile_claims", profileClaimsFromIDToken) + }) + return upstreamClaims + } + + idTokenClaims, err := parseUpstreamJWTClaims(idToken) + if err != nil { + // Do not fail the request: the access token parsed, so policies that only + // reference its claims must keep working. An unparsable id_token means no + // supplement, which denies exactly the policies that needed it. + a.missingIDTokenLog.Do(func() { + slog.Warn("upstream ID token for provider is not a parsable JWT; not supplementing profile claims", + "provider", a.primaryUpstreamProvider, + "error", err) + }) + return upstreamClaims + } + + merged, filled := supplementProfileClaims(upstreamClaims, idTokenClaims) if len(filled) > 0 { // Rate-limited on its own timer, not claimKeyLog: sharing one would let // this Warn consume the window and permanently starve the claim-key dump - // below, which is the thing you want when debugging a denying policy. + // in resolveClaims, which is what you want when debugging a denying policy. // // Claim keys only — never claim values, which hold user PII. a.supplementLog.Do(func() { - slog.Warn("upstream token lacks identity claims; using the request token's values for Cedar evaluation", + slog.Warn("upstream access token lacks profile claims; using the upstream ID token's values "+ + "for Cedar evaluation", "provider", a.primaryUpstreamProvider, "claims", filled) }) } - a.logClaimKeys("upstream", merged) - return merged, nil + return merged } -// mirroredIdentityClaims are the claims that may stand in for a missing upstream -// claim: the user-profile pair the embedded auth server copies verbatim from the -// upstream OIDC identity into the token it issues (session.New in -// pkg/authserver/server/session). Referencing that package's constants makes a -// rename on either side a compile error; it does not catch a claim being ADDED to -// session.New, which still needs a deliberate decision here. +// profileClaimsFromIDToken are the OIDC Core §5.1 profile claims that may be read +// from the primary provider's id_token when its access token omits them. They are +// spelled with the auth server's own constants — the same two claims it extracts +// from that id_token into the token it issues (upstream/oidc.go into session.New) — +// so a rename on either side is a compile error. That does not catch a claim being +// ADDED there, which still needs a deliberate decision here. // -// `sub` is deliberately NOT in this list even though session.New sets a subject. -// The AS-issued subject is an internal ToolHive user ID — a fresh UUID from -// UserResolver (pkg/authserver/server/handlers/user.go), distinct from the -// upstream subject, which travels separately as UpstreamSubject -// (handlers/callback.go). It is also the one identity claim that becomes the Cedar -// principal entity ID via extractClientIDFromClaims rather than an attribute, and -// Cedar has no `has`-style guard in the principal position. Supplementing it -// would silently retarget the principal instead of failing closed, so a `forbid` -// keyed on an upstream subject would stop matching with no diagnostic. An upstream -// access token with no `sub` violates RFC 9068 and is better surfaced as -// ErrMissingPrincipal. -// -// Widening this list widens what a non-upstream token can contribute to a policy -// decision — see resolveClaims before adding anything. -var mirroredIdentityClaims = []string{session.NameClaimKey, session.EmailClaimKey} - -// supplementUpstreamClaims returns a fresh claim set holding every upstream claim -// plus, for each name in mirroredIdentityClaims that the upstream token does not -// carry, the request token's value for that name. It also returns the sorted list -// of names actually supplied by the request token, for logging. +// Widening this list widens what can satisfy a policy — see resolveClaims first. +var profileClaimsFromIDToken = []string{session.NameClaimKey, session.EmailClaimKey} + +// supplementProfileClaims returns a fresh claim set holding every access-token +// claim plus, for each name in profileClaimsFromIDToken that the access token does +// not carry, the id_token's value for that name. It also returns the sorted list of +// names actually supplied by the id_token, for logging. // // Neither input is mutated and the result aliases neither, so the caller may hand // it to code that adds synthetic keys. Absent claims are never fabricated: a name -// missing from both sides is missing from the result, which keeps `has`-guarded +// missing from both tokens is missing from the result, which keeps `has`-guarded // policies failing closed. -func supplementUpstreamClaims(upstreamClaims, requestClaims jwt.MapClaims) (merged jwt.MapClaims, filled []string) { - merged = make(jwt.MapClaims, len(upstreamClaims)+len(mirroredIdentityClaims)) - for k, v := range upstreamClaims { +func supplementProfileClaims(accessTokenClaims, idTokenClaims jwt.MapClaims) (merged jwt.MapClaims, filled []string) { + merged = make(jwt.MapClaims, len(accessTokenClaims)+len(profileClaimsFromIDToken)) + for k, v := range accessTokenClaims { merged[k] = v } - for _, name := range mirroredIdentityClaims { + for _, name := range profileClaimsFromIDToken { if _, ok := merged[name]; ok { - // The upstream IDP asserted this claim; it always wins. + // The access token asserted this claim; it always wins. continue } - v, ok := requestClaims[name] + v, ok := idTokenClaims[name] if !ok { continue } @@ -698,8 +748,8 @@ func supplementUpstreamClaims(upstreamClaims, requestClaims jwt.MapClaims) (merg filled = append(filled, name) } - // Sorted for a canonical, easily-greppable log line — mirroredIdentityClaims is - // ordered for readability (name, email), not alphabetically. + // Sorted for a canonical, easily-greppable log line — profileClaimsFromIDToken + // is ordered for readability (name, email), not alphabetically. slices.Sort(filled) return merged, filled } diff --git a/pkg/authz/authorizers/cedar/core_test.go b/pkg/authz/authorizers/cedar/core_test.go index 16908c7538..936e50fc90 100644 --- a/pkg/authz/authorizers/cedar/core_test.go +++ b/pkg/authz/authorizers/cedar/core_test.go @@ -1523,35 +1523,30 @@ func TestAuthorizeWithJWTClaims_UpstreamProvider(t *testing.T) { } } -// TestSupplementUpstreamClaims covers which request-token claims may stand in for -// a missing upstream claim, and which may never do so. -func TestSupplementUpstreamClaims(t *testing.T) { +// TestSupplementProfileClaims covers which id_token claims may stand in for a +// missing access-token claim, and which may never do so. +func TestSupplementProfileClaims(t *testing.T) { t.Parallel() tests := []struct { - name string - upstreamClaims jwt.MapClaims - requestClaims jwt.MapClaims - wantMerged jwt.MapClaims - wantFilled []string + name string + accessTokenClaims jwt.MapClaims + idTokenClaims jwt.MapClaims + wantMerged jwt.MapClaims + wantFilled []string }{ { - name: "upstream_wins_on_shared_identity_claims", - upstreamClaims: jwt.MapClaims{ - "sub": "okta|alice", "email": "alice@upstream.example", "name": "Upstream Alice", - }, - requestClaims: jwt.MapClaims{ - "sub": "7f3c1e64-uuid", "email": "alice@thv.example", "name": "THV Alice", - }, - wantMerged: jwt.MapClaims{ - "sub": "okta|alice", "email": "alice@upstream.example", "name": "Upstream Alice", - }, - wantFilled: nil, + name: "access_token_wins_on_shared_profile_claims", + accessTokenClaims: jwt.MapClaims{"sub": "okta|alice", "email": "at@example.com", "name": "AT Alice"}, + idTokenClaims: jwt.MapClaims{"sub": "okta|alice", "email": "idt@example.com", "name": "IDT Alice"}, + wantMerged: jwt.MapClaims{"sub": "okta|alice", "email": "at@example.com", "name": "AT Alice"}, + wantFilled: nil, }, { - name: "request_token_fills_missing_identity_claims", - upstreamClaims: jwt.MapClaims{"sub": "okta|alice", "iss": "https://idp.example.com"}, - requestClaims: jwt.MapClaims{"sub": "7f3c1e64-uuid", "email": "alice@example.com", "name": "Alice"}, + // The #5916 shape: provider asserts email in the id_token only. + name: "id_token_fills_missing_profile_claims", + accessTokenClaims: jwt.MapClaims{"sub": "okta|alice", "iss": "https://idp.example.com"}, + idTokenClaims: jwt.MapClaims{"sub": "okta|alice", "email": "alice@example.com", "name": "Alice"}, wantMerged: jwt.MapClaims{ "sub": "okta|alice", "iss": "https://idp.example.com", @@ -1562,26 +1557,12 @@ func TestSupplementUpstreamClaims(t *testing.T) { wantFilled: []string{"email", "name"}, }, { - // `sub` is excluded from mirroredIdentityClaims: the AS-issued subject - // is an internal ToolHive user ID, not a copy of the upstream one, and - // it becomes the Cedar principal entity ID. Supplementing it would - // silently retarget the principal rather than fail closed. - name: "subject_is_never_filled_from_the_request_token", - upstreamClaims: jwt.MapClaims{"iss": "https://idp.example.com"}, - requestClaims: jwt.MapClaims{"sub": "7f3c1e64-uuid", "email": "alice@example.com"}, - wantMerged: jwt.MapClaims{ - "iss": "https://idp.example.com", - "email": "alice@example.com", - }, - wantFilled: []string{"email"}, - }, - { - // The core security boundary: authorization-bearing claims are - // upstream-only. A group on the ToolHive-issued token must not reach - // Cedar when an upstream provider is configured. - name: "authorization_bearing_claims_are_never_filled", - upstreamClaims: jwt.MapClaims{"sub": "okta|alice"}, - requestClaims: jwt.MapClaims{ + // Taking these from this provider's id_token would be provenance-correct, + // but it would newly grant requests that deny today, so it is out of scope + // here and pinned as unchanged behaviour. + name: "authorization_bearing_claims_are_not_filled", + accessTokenClaims: jwt.MapClaims{"sub": "okta|alice"}, + idTokenClaims: jwt.MapClaims{ "groups": []interface{}{"platform-eng"}, "roles": []interface{}{"admin"}, "scope": "tools:write", @@ -1591,44 +1572,57 @@ func TestSupplementUpstreamClaims(t *testing.T) { wantFilled: nil, }, { - // The AS-issued token's own protocol claims describe that token, not - // the upstream identity, so they must not fill upstream gaps. - name: "as_token_protocol_claims_are_never_filled", - upstreamClaims: jwt.MapClaims{"sub": "okta|alice"}, - requestClaims: jwt.MapClaims{ - "iss": "https://thv-as.example.com/", - "aud": "toolhive", - "exp": 1234567890, - "jti": "abc", - "client_id": "vscode", + // An id_token's registered claims describe that token. Merging them would + // overwrite the access token's own aud/exp with different-meaning values. + name: "id_token_registered_claims_are_never_filled_or_overwritten", + accessTokenClaims: jwt.MapClaims{ + "sub": "okta|alice", "aud": "api://resource", "exp": 2000000000, + }, + idTokenClaims: jwt.MapClaims{ + "aud": "toolhive-as-client", "exp": 1000000000, + "nonce": "n-abc", "at_hash": "h-abc", "azp": "client-x", + }, + wantMerged: jwt.MapClaims{ + "sub": "okta|alice", "aud": "api://resource", "exp": 2000000000, }, - wantMerged: jwt.MapClaims{"sub": "okta|alice"}, wantFilled: nil, }, { - // Never fabricate: a claim absent from both sides stays absent so + // `sub` becomes the Cedar principal entity ID, where Cedar offers no + // `has`-style guard, so it fails closed rather than being chosen for us. + name: "subject_is_never_filled_from_the_id_token", + accessTokenClaims: jwt.MapClaims{"iss": "https://idp.example.com"}, + idTokenClaims: jwt.MapClaims{"sub": "okta|alice", "email": "alice@example.com"}, + wantMerged: jwt.MapClaims{ + "iss": "https://idp.example.com", + "email": "alice@example.com", + }, + wantFilled: []string{"email"}, + }, + { + // Never fabricate: a claim absent from both tokens stays absent so // `has`-guarded policies keep failing closed. - name: "claim_absent_from_both_stays_absent", - upstreamClaims: jwt.MapClaims{"sub": "okta|alice"}, - requestClaims: jwt.MapClaims{"sub": "7f3c1e64-uuid"}, - wantMerged: jwt.MapClaims{"sub": "okta|alice"}, - wantFilled: nil, + name: "claim_absent_from_both_stays_absent", + accessTokenClaims: jwt.MapClaims{"sub": "okta|alice"}, + idTokenClaims: jwt.MapClaims{"sub": "okta|alice"}, + wantMerged: jwt.MapClaims{"sub": "okta|alice"}, + wantFilled: nil, }, { - // A key the upstream maps to nil is still "asserted by the upstream": - // presence, not truthiness, decides precedence. - name: "explicit_nil_upstream_value_still_wins", - upstreamClaims: jwt.MapClaims{"email": nil}, - requestClaims: jwt.MapClaims{"email": "alice@example.com"}, - wantMerged: jwt.MapClaims{"email": nil}, - wantFilled: nil, + // A key the access token maps to nil is still "asserted by the access + // token": presence, not truthiness, decides precedence. + name: "explicit_nil_access_token_value_still_wins", + accessTokenClaims: jwt.MapClaims{"email": nil}, + idTokenClaims: jwt.MapClaims{"email": "alice@example.com"}, + wantMerged: jwt.MapClaims{"email": nil}, + wantFilled: nil, }, { - name: "nil_inputs_yield_empty_result", - upstreamClaims: nil, - requestClaims: nil, - wantMerged: jwt.MapClaims{}, - wantFilled: nil, + name: "nil_inputs_yield_empty_result", + accessTokenClaims: nil, + idTokenClaims: nil, + wantMerged: jwt.MapClaims{}, + wantFilled: nil, }, } @@ -1637,96 +1631,161 @@ func TestSupplementUpstreamClaims(t *testing.T) { t.Parallel() // Snapshot the inputs so the no-mutation contract can be asserted. - upstreamBefore := maps.Clone(tt.upstreamClaims) - requestBefore := maps.Clone(tt.requestClaims) + accessBefore := maps.Clone(tt.accessTokenClaims) + idTokenBefore := maps.Clone(tt.idTokenClaims) - merged, filled := supplementUpstreamClaims(tt.upstreamClaims, tt.requestClaims) + merged, filled := supplementProfileClaims(tt.accessTokenClaims, tt.idTokenClaims) assert.Equal(t, tt.wantMerged, merged) assert.Equal(t, tt.wantFilled, filled) - assert.Equal(t, upstreamBefore, tt.upstreamClaims, "upstream claims must not be mutated") - assert.Equal(t, requestBefore, tt.requestClaims, "request claims must not be mutated") + assert.Equal(t, accessBefore, tt.accessTokenClaims, "access token claims must not be mutated") + assert.Equal(t, idTokenBefore, tt.idTokenClaims, "id token claims must not be mutated") // The result must not alias either input, since the caller adds // synthetic keys to the claim set it receives. merged["injected-by-caller"] = true - assert.NotContains(t, tt.upstreamClaims, "injected-by-caller") - assert.NotContains(t, tt.requestClaims, "injected-by-caller") + assert.NotContains(t, tt.accessTokenClaims, "injected-by-caller") + assert.NotContains(t, tt.idTokenClaims, "injected-by-caller") }) } } // TestResolveClaimsUpstreamSupplement pins the claim-source contract of -// resolveClaims on the embedded-auth-server path: the upstream token's claims -// win, and the request token supplies only the mirrored identity claims the -// upstream token omits. The email-filling case is the #5916 reproducer — an -// upstream IdP (JetBrains Hub is the public example) whose access token is a -// well-formed JWT that carries no `email`, which used to make every -// `claim_email` policy deny. +// resolveClaims on the embedded-auth-server path: the pinned provider's access +// token wins, its id_token supplies only the profile claims the access token +// omits, and the ToolHive-issued token is not a claim source at all. +// +// The email-filling case is the #5916 reproducer — an upstream IdP whose access +// token is a well-formed JWT carrying no `email` (the provider asserts it in the +// id_token only), which used to make every `claim_email` policy deny. func TestResolveClaimsUpstreamSupplement(t *testing.T) { t.Parallel() const providerName = "hub" + // What the embedded auth server mirrors into the token it issues. In a + // multi-upstream chain these values come from the FIRST configured upstream, + // which need not be the pinned provider, so they must never reach Cedar on + // this path. Distinctive values make a leak obvious. + asIssuedClaims := map[string]any{ + "sub": "7f3c1e64-9b2a-4d51-8e77-1c0a5f3b9d42", + "email": "leaked-from-as-token@example.com", + "name": "Leaked From AS Token", + "client_id": "vscode", + } + tests := []struct { name string provider string requestClaims map[string]any upstreamToken string + idToken string wantClaims jwt.MapClaims }{ { - name: "upstream_jwt_without_email_falls_back_to_as_token_email", + name: "access_token_without_email_falls_back_to_id_token_email", provider: providerName, - // What the embedded auth server mirrors into the token it issues. The - // AS subject is an internal ToolHive user ID (a UserResolver UUID), - // deliberately NOT the upstream subject. - requestClaims: map[string]any{ - "sub": "7f3c1e64-9b2a-4d51-8e77-1c0a5f3b9d42", - "email": "alice@example.com", - "name": "Alice", - "client_id": "vscode", - }, upstreamToken: makeUnsignedJWT(jwt.MapClaims{ "sub": "hub|alice", "iss": "https://hub.example.com", - // No email: it lives in the id_token only. + // No email: the provider asserts it in the id_token only. }), + idToken: makeUnsignedJWT(jwt.MapClaims{ + "sub": "hub|alice", + "email": "alice@example.com", + "name": "Alice", + "aud": "toolhive-as-client", + "nonce": "n-abc", + }), + requestClaims: asIssuedClaims, wantClaims: jwt.MapClaims{ "sub": "hub|alice", "iss": "https://hub.example.com", "email": "alice@example.com", "name": "Alice", - // client_id is absent: it describes the AS-issued token, not the - // upstream identity, so it never fills an upstream gap. + // The id_token's own aud/nonce are absent: they describe that + // token, not the user. }, }, { - name: "upstream_claims_win_over_request_claims", + name: "access_token_claims_win_over_id_token_claims", provider: providerName, - requestClaims: map[string]any{ - "sub": "7f3c1e64-9b2a-4d51-8e77-1c0a5f3b9d42", - "email": "alice@thv.example", - "groups": []interface{}{"admins"}, - }, upstreamToken: makeUnsignedJWT(jwt.MapClaims{ "sub": "hub|alice", - "email": "alice@upstream.example", + "email": "alice@access-token.example", "groups": []interface{}{"engineering"}, }), + idToken: makeUnsignedJWT(jwt.MapClaims{ + "sub": "hub|alice", + "email": "alice@id-token.example", + "groups": []interface{}{"admins"}, + }), + requestClaims: asIssuedClaims, wantClaims: jwt.MapClaims{ "sub": "hub|alice", - "email": "alice@upstream.example", + "email": "alice@access-token.example", "groups": []interface{}{"engineering"}, }, }, + { + // The multi-upstream misattribution guard. With no id_token stored for + // the pinned provider, `email` stays absent and the policy denies — the + // ToolHive-issued token's mirrored email must NOT stand in, because in a + // chain it belongs to the first configured upstream, not this one. + name: "no_id_token_leaves_profile_claims_absent", + provider: providerName, + upstreamToken: makeUnsignedJWT(jwt.MapClaims{ + "sub": "hub|alice", + "iss": "https://hub.example.com", + }), + idToken: "", + requestClaims: asIssuedClaims, + wantClaims: jwt.MapClaims{ + "sub": "hub|alice", + "iss": "https://hub.example.com", + }, + }, + { + // An id_token is read as a record of what the upstream asserted at + // login, not presented as a credential, so `exp` is deliberately not + // enforced. Enforcing it would make a policy permit early in a session + // and silently deny later, since sessions outlive id_tokens by days. + name: "expired_id_token_is_still_used", + provider: providerName, + upstreamToken: makeUnsignedJWT(jwt.MapClaims{ + "sub": "hub|alice", + }), + idToken: makeUnsignedJWT(jwt.MapClaims{ + "sub": "hub|alice", + "email": "alice@example.com", + "exp": 1000000000, // long past + }), + requestClaims: asIssuedClaims, + wantClaims: jwt.MapClaims{ + "sub": "hub|alice", + "email": "alice@example.com", + }, + }, + { + // A malformed id_token must not fail the request: the access token + // parsed, so policies referencing only its claims keep working. + name: "unparsable_id_token_degrades_to_no_supplement", + provider: providerName, + upstreamToken: makeUnsignedJWT(jwt.MapClaims{ + "sub": "hub|alice", + }), + idToken: "not-base64.not-base64.not-base64", + requestClaims: asIssuedClaims, + wantClaims: jwt.MapClaims{"sub": "hub|alice"}, + }, { name: "no_provider_configured_uses_request_claims_verbatim", provider: "", requestClaims: map[string]any{"sub": "7f3c1e64-uuid", "email": "alice@thv.example"}, - // Present but irrelevant: without a configured provider the upstream - // token is never consulted. + // Present but irrelevant: without a configured provider neither upstream + // token is consulted. upstreamToken: makeUnsignedJWT(jwt.MapClaims{"sub": "hub|alice"}), + idToken: makeUnsignedJWT(jwt.MapClaims{"sub": "hub|alice"}), wantClaims: jwt.MapClaims{"sub": "7f3c1e64-uuid", "email": "alice@thv.example"}, }, } @@ -1746,6 +1805,9 @@ func TestResolveClaimsUpstreamSupplement(t *testing.T) { PrincipalInfo: auth.PrincipalInfo{Subject: "7f3c1e64-uuid", Claims: tt.requestClaims}, UpstreamTokens: map[string]string{providerName: tt.upstreamToken}, } + if tt.idToken != "" { + identity.UpstreamIDTokens = map[string]string{providerName: tt.idToken} + } claimsBefore := maps.Clone(tt.requestClaims) resolved, err := authorizer.(*Authorizer).resolveClaims(identity) @@ -1763,7 +1825,7 @@ func TestResolveClaimsUpstreamSupplement(t *testing.T) { // authorizer-level reproducer for #5916: the exact policy shape from the report // (a defensively-authored `has`-guarded domain gate plus a tool permit) evaluated // against an upstream access token that is a well-formed JWT without `email`. -// Before the merge this denied every request for every user. +// Before the fix this denied every request for every user. func TestAuthorizeWithJWTClaims_UpstreamJWTMissingIdentityClaim(t *testing.T) { t.Parallel() @@ -1782,35 +1844,45 @@ func TestAuthorizeWithJWTClaims_UpstreamJWTMissingIdentityClaim(t *testing.T) { // A JWT-shaped upstream access token that parses cleanly but carries no // identity claims beyond `sub`. looksLikeJWT is true here, so the #5147 - // opaque-token fallback does not apply — merging is what recovers `email`. + // opaque-token fallback does not apply — the id_token supplement is what + // recovers `email`. upstreamToken := makeUnsignedJWT(jwt.MapClaims{ "sub": "hub|alice", "iss": "https://hub.example.com", }) + // The ToolHive-issued token mirrors an email too, but it must never be the + // source on this path; it is outside the gated domain so a leak would show up + // as an unexpected deny in the permitting cases. + asIssuedClaims := map[string]any{ + "sub": "7f3c1e64-9b2a-4d51-8e77-1c0a5f3b9d42", + "email": "leaked-from-as-token@wrong-provider.example", + } + tests := []struct { name string - requestClaims map[string]any + idTokenClaims jwt.MapClaims wantAuthorize bool }{ { - // The AS-issued token mirrors the upstream id_token's email, so the - // domain gate can be satisfied even though the access token omits it. - name: "as_token_email_in_gated_domain_permits", - requestClaims: map[string]any{"sub": "7f3c1e64-uuid", "email": "alice@example.com"}, + // The upstream asserts the email in its id_token, so the domain gate + // can be satisfied even though the access token omits it. + name: "id_token_email_in_gated_domain_permits", + idTokenClaims: jwt.MapClaims{"sub": "hub|alice", "email": "alice@example.com"}, wantAuthorize: true, }, { - // Merging supplies the claim; it does not weaken the gate. - name: "as_token_email_outside_gated_domain_denies", - requestClaims: map[string]any{"sub": "7f3c1e64-uuid", "email": "alice@evil.example"}, + // The supplement supplies the claim; it does not weaken the gate. + name: "id_token_email_outside_gated_domain_denies", + idTokenClaims: jwt.MapClaims{"sub": "hub|alice", "email": "alice@evil.example"}, wantAuthorize: false, }, { - // Neither token carries `email`: the `has` guard fails and the - // forbid applies. Merging never fabricates a claim. - name: "email_absent_from_both_tokens_denies", - requestClaims: map[string]any{"sub": "7f3c1e64-uuid"}, + // Neither upstream token carries `email`: the `has` guard fails and the + // forbid applies. The supplement never fabricates a claim, and the + // ToolHive-issued token's mirrored email does not stand in. + name: "email_absent_from_both_upstream_tokens_denies", + idTokenClaims: jwt.MapClaims{"sub": "hub|alice"}, wantAuthorize: false, }, } @@ -1820,8 +1892,12 @@ func TestAuthorizeWithJWTClaims_UpstreamJWTMissingIdentityClaim(t *testing.T) { t.Parallel() identity := &auth.Identity{ - PrincipalInfo: auth.PrincipalInfo{Subject: "7f3c1e64-uuid", Claims: tt.requestClaims}, - UpstreamTokens: map[string]string{providerName: upstreamToken}, + PrincipalInfo: auth.PrincipalInfo{ + Subject: "7f3c1e64-9b2a-4d51-8e77-1c0a5f3b9d42", + Claims: asIssuedClaims, + }, + UpstreamTokens: map[string]string{providerName: upstreamToken}, + UpstreamIDTokens: map[string]string{providerName: makeUnsignedJWT(tt.idTokenClaims)}, } ctx := auth.WithIdentity(context.Background(), identity) @@ -1905,6 +1981,16 @@ func TestAuthorizeWithJWTClaims_PrincipalStaysUpstreamSourced(t *testing.T) { UpstreamTokens: map[string]string{ providerName: makeUnsignedJWT(tt.upstreamClaims), }, + // The id_token carries a subject too. It is the correct upstream + // subject, but `sub` is still excluded from the supplement so a + // missing access-token `sub` fails closed rather than having a + // principal chosen for it. + UpstreamIDTokens: map[string]string{ + providerName: makeUnsignedJWT(jwt.MapClaims{ + "sub": "hub|from-id-token", + "email": "alice@example.com", + }), + }, } ctx := auth.WithIdentity(context.Background(), identity) diff --git a/pkg/authz/integration_test.go b/pkg/authz/integration_test.go index 1a320aa32b..b8cd12473a 100644 --- a/pkg/authz/integration_test.go +++ b/pkg/authz/integration_test.go @@ -1201,9 +1201,10 @@ func TestIntegrationPrimaryUpstreamProviderClaimAttributeAccess(t *testing.T) { // Opaque-token upstreams (Google, GitHub) masked the bug because their tokens hit // that fallback and saw the AS-issued token's mirrored `email`. // -// The AS-issued token carries the upstream `email` in both cases (the auth server -// mirrors it from the upstream id_token), so the fix lets it stand in for the -// claim the access token omits. +// The provider does assert `email` — in its id_token, which the auth server stores +// alongside the access token for the same provider. The fix reads the profile +// claims the access token omits from that id_token, keeping the claim's provenance +// with the pinned upstream. func TestIntegrationUpstreamJWTWithoutEmailClaim(t *testing.T) { t.Parallel() @@ -1222,26 +1223,38 @@ func TestIntegrationUpstreamJWTWithoutEmailClaim(t *testing.T) { }, "") require.NoError(t, err) - // Claims of the token the client actually presented, as minted by the - // embedded auth server: an internal ToolHive user ID (a UserResolver UUID, - // deliberately NOT the upstream subject) plus the mirrored id_token profile. + // Claims of the token the client actually presented, as minted by the embedded + // auth server: an internal ToolHive user ID (a UserResolver UUID, deliberately + // NOT the upstream subject) plus the profile it mirrored from the FIRST + // configured upstream. In a multi-upstream chain that is not necessarily the + // pinned provider, so these must never reach Cedar on this path — the + // distinctive email makes a leak visible as an unexpected deny. asIssuedClaims := jwt.MapClaims{ "sub": "7f3c1e64-9b2a-4d51-8e77-1c0a5f3b9d42", - "email": "alice@example.com", - "name": "Alice", + "email": "leaked-from-as-token@wrong-provider.example", + "name": "Leaked From AS Token", "iss": "https://thv-as.example.com/", "aud": "toolhive", "client_id": "vscode", } + // The pinned provider's id_token, stored beside its access token at login. + // This is where this provider asserts the email. + upstreamIDToken := makeUnsignedJWT(t, jwt.MapClaims{ + "sub": "hub|alice", + "email": "alice@example.com", + "name": "Alice", + }) + tests := []struct { name string upstreamToken string + requestClaims jwt.MapClaims expectAllowed bool }{ { // The reported case: parses as a JWT, carries no `email`. - name: "jwt_upstream_token_without_email_uses_as_token_email", + name: "jwt_access_token_without_email_uses_id_token_email", upstreamToken: makeUnsignedJWT(t, jwt.MapClaims{ "sub": "hub|alice", "iss": "https://hub.example.com", @@ -1249,9 +1262,9 @@ func TestIntegrationUpstreamJWTWithoutEmailClaim(t *testing.T) { expectAllowed: true, }, { - // The upstream's own `email` still wins where it asserts one, and it - // is outside the gated domain here, so the gate must reject. - name: "upstream_email_wins_over_as_token_email", + // The access token's own `email` still wins where it asserts one, and + // it is outside the gated domain here, so the gate must reject. + name: "access_token_email_wins_over_id_token_email", upstreamToken: makeUnsignedJWT(t, jwt.MapClaims{ "sub": "hub|alice", "email": "alice@contractor.example", @@ -1259,9 +1272,24 @@ func TestIntegrationUpstreamJWTWithoutEmailClaim(t *testing.T) { expectAllowed: false, }, { - // Regression guard for the #5147 path, which this change leaves intact. + // 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", upstreamToken: "ya29.opaque-google-style-token", + requestClaims: jwt.MapClaims{ + "sub": "7f3c1e64-9b2a-4d51-8e77-1c0a5f3b9d42", + "email": "alice@example.com", + "name": "Alice", + }, expectAllowed: true, }, { @@ -1292,12 +1320,18 @@ func TestIntegrationUpstreamJWTWithoutEmailClaim(t *testing.T) { require.NoError(t, err) httpReq.Header.Set("Content-Type", "application/json") + requestClaims := tt.requestClaims + if requestClaims == nil { + requestClaims = asIssuedClaims + } + identity := &auth.Identity{ PrincipalInfo: auth.PrincipalInfo{ Subject: "7f3c1e64-9b2a-4d51-8e77-1c0a5f3b9d42", - Claims: asIssuedClaims, + Claims: requestClaims, }, - UpstreamTokens: map[string]string{providerName: tt.upstreamToken}, + UpstreamTokens: map[string]string{providerName: tt.upstreamToken}, + UpstreamIDTokens: map[string]string{providerName: upstreamIDToken}, } httpReq = httpReq.WithContext(auth.WithIdentity(httpReq.Context(), identity))