diff --git a/docs/operator/virtualmcpserver-kubernetes-guide.md b/docs/operator/virtualmcpserver-kubernetes-guide.md index 0f43265c42..b186cfcdc5 100644 --- a/docs/operator/virtualmcpserver-kubernetes-guide.md +++ b/docs/operator/virtualmcpserver-kubernetes-guide.md @@ -650,6 +650,49 @@ 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, `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 access token lacks profile claims; using the upstream ID token's + values for Cedar evaluation provider=okta claims="[email]" +``` + +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. + +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** > > The field used to live under diff --git a/pkg/authz/authorizers/cedar/core.go b/pkg/authz/authorizers/cedar/core.go index ef5a538428..aa03265344 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,7 +172,8 @@ 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 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 @@ -190,6 +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 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. @@ -205,9 +217,17 @@ 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 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 @@ -321,6 +341,8 @@ 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), + missingIDTokenLog: syncutil.NewAtMost(30 * time.Second), multiValuedClaims: options.MultiValuedClaims, } @@ -532,56 +554,204 @@ 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 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). +// +// 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. +// +// The supplement is deliberately restricted to profileClaimsFromIDToken rather +// than being a general merge of the two token bodies. Read this before widening it: +// +// - 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. +// +// 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) { - 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 := 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 + // 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 access token lacks profile claims; using the upstream ID token's values "+ + "for Cedar evaluation", + "provider", a.primaryUpstreamProvider, + "claims", filled) + }) + } + return merged +} + +// 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. +// +// 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 tokens is missing from the result, which keeps `has`-guarded +// policies failing closed. +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 profileClaimsFromIDToken { + if _, ok := merged[name]; ok { + // The access token asserted this claim; it always wins. + continue } - a.logClaimKeys("upstream", parsedClaims) - return parsedClaims, nil + v, ok := idTokenClaims[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 for a canonical, easily-greppable log line — profileClaimsFromIDToken + // is ordered for readability (name, email), not alphabetically. + 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..936e50fc90 100644 --- a/pkg/authz/authorizers/cedar/core_test.go +++ b/pkg/authz/authorizers/cedar/core_test.go @@ -8,6 +8,8 @@ import ( "context" "encoding/json" "log/slog" + "maps" + "sync" "testing" cedar "github.com/cedar-policy/cedar-go" @@ -1483,6 +1485,11 @@ func TestAuthorizeWithJWTClaims_UpstreamProvider(t *testing.T) { }), }, }, + // `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", }, @@ -1516,6 +1523,497 @@ func TestAuthorizeWithJWTClaims_UpstreamProvider(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 + accessTokenClaims jwt.MapClaims + idTokenClaims jwt.MapClaims + wantMerged jwt.MapClaims + wantFilled []string + }{ + { + 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, + }, + { + // 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", + "email": "alice@example.com", + "name": "Alice", + }, + // Sorted, so the logged claim list is stable across runs. + wantFilled: []string{"email", "name"}, + }, + { + // 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", + "hd": "example.com", + }, + wantMerged: jwt.MapClaims{"sub": "okta|alice"}, + wantFilled: nil, + }, + { + // 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, + }, + wantFilled: nil, + }, + { + // `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", + accessTokenClaims: jwt.MapClaims{"sub": "okta|alice"}, + idTokenClaims: jwt.MapClaims{"sub": "okta|alice"}, + wantMerged: jwt.MapClaims{"sub": "okta|alice"}, + 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", + accessTokenClaims: nil, + idTokenClaims: 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. + accessBefore := maps.Clone(tt.accessTokenClaims) + idTokenBefore := maps.Clone(tt.idTokenClaims) + + merged, filled := supplementProfileClaims(tt.accessTokenClaims, tt.idTokenClaims) + + assert.Equal(t, tt.wantMerged, merged) + assert.Equal(t, tt.wantFilled, filled) + 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.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 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: "access_token_without_email_falls_back_to_id_token_email", + provider: providerName, + upstreamToken: makeUnsignedJWT(jwt.MapClaims{ + "sub": "hub|alice", + "iss": "https://hub.example.com", + // 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", + // The id_token's own aud/nonce are absent: they describe that + // token, not the user. + }, + }, + { + name: "access_token_claims_win_over_id_token_claims", + provider: providerName, + upstreamToken: makeUnsignedJWT(jwt.MapClaims{ + "sub": "hub|alice", + "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@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 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"}, + }, + } + + 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: "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) + 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 fix 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 — 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 + idTokenClaims jwt.MapClaims + wantAuthorize bool + }{ + { + // 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, + }, + { + // 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 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, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + identity := &auth.Identity{ + 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) + + authorized, err := authorizer.AuthorizeWithJWTClaims( + ctx, + authorizers.MCPFeatureTool, + authorizers.MCPOperationCall, + "deploy", + nil, + ) + require.NoError(t, err) + assert.Equal(t, tt.wantAuthorize, authorized) + }) + } +} + +// 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), + }, + // 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) + + 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) { @@ -3635,14 +4133,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)) @@ -3706,14 +4232,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 217ee072b7..b8cd12473a 100644 --- a/pkg/authz/integration_test.go +++ b/pkg/authz/integration_test.go @@ -1187,3 +1187,172 @@ 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 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() + + 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: 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": "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_access_token_without_email_uses_id_token_email", + upstreamToken: makeUnsignedJWT(t, jwt.MapClaims{ + "sub": "hub|alice", + "iss": "https://hub.example.com", + }), + expectAllowed: true, + }, + { + // 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", + }), + expectAllowed: false, + }, + { + // Regression guard for the #5147 path, which this change leaves intact: + // an opaque access token has no claims to read, so that branch still + // evaluates the ToolHive-issued token's claims. Those carry the + // upstream profile only because the auth server mirrors the FIRST + // configured upstream, so this case supplies a single-upstream + // AS token rather than the leak sentinel the JWT cases use. + // + // That leaves the two branches inconsistent about claim provenance + // (and about the principal: this branch's `sub` is ToolHive's internal + // user ID, not the upstream subject). Unifying them on the id_token is + // a follow-up — it changes behaviour on a shipped path. + name: "opaque_upstream_token_still_falls_back", + upstreamToken: "ya29.opaque-google-style-token", + requestClaims: jwt.MapClaims{ + "sub": "7f3c1e64-9b2a-4d51-8e77-1c0a5f3b9d42", + "email": "alice@example.com", + "name": "Alice", + }, + expectAllowed: true, + }, + { + // 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", + 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") + + requestClaims := tt.requestClaims + if requestClaims == nil { + requestClaims = asIssuedClaims + } + + identity := &auth.Identity{ + PrincipalInfo: auth.PrincipalInfo{ + Subject: "7f3c1e64-9b2a-4d51-8e77-1c0a5f3b9d42", + Claims: requestClaims, + }, + UpstreamTokens: map[string]string{providerName: tt.upstreamToken}, + UpstreamIDTokens: map[string]string{providerName: upstreamIDToken}, + } + 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) + } + }) + } +}