From 39ad66bded614d1f85746916d3d354596d6e69c1 Mon Sep 17 00:00:00 2001 From: Livio Spring <9405495+livio-a@users.noreply.github.com> Date: Thu, 9 Jul 2026 14:33:11 +0200 Subject: [PATCH] fix: correct scope validation in token exchange (#12312) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Which Problems Are Solved The v4.15.3 token exchange hardening (`#12319`, `#12322`) introduced two regressions: 1. **Logic bug**: scope validation used intersection (`!inSubject || !inActor`) instead of union, so any explicit `scope` on a `user_id` subject always failed — even `openid` when present on the actor token. 2. **Design gap**: even with union logic, scope-less subjects (`user_id`, `id_token`) cannot satisfy a rule that requires all requested scopes to exist on input tokens. This breaks documented impersonation flows (e.g. service account actor + `user_id` subject requesting `email`). ## How the Problems Are Solved - **Standard exchange** (no actor, or subject carries scopes): keep union validation — requested scopes must be ⊆ subject ∪ actor. For plain exchange this is effectively a subset of the subject token. - **Scope-less subject impersonation** (`user_id`, `id_token` on actor path): split validation: - **Subject-data scopes** (`openid`, `profile`, `email`, …): client allowlist only - **Authorization scopes** (`offline_access`, `:aud`, `projects:roles`, `role:*`): still ⊆ subject ∪ actor - Restore subject → actor fallback when `scope` is omitted. GHSA protections remain: cross-client token binding and authorization-scope escalation are still rejected. Closes #12319 Closes #12322 ## Additional Changes - Unit tests for scope validators (`token_exchange_test.go`) - Integration tests covering `user_id`/`id_token` paths, union-path rejection, and client-credentials actor → `user_id` exchange ## Additional Context - Relates to https://github.com/zitadel/zitadel/releases/tag/v4.15.3 - Security fix: GHSA-vrh8-c9cm-wh8v --------- Co-authored-by: Cursor Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- .../integration_test/token_exchange_test.go | 148 ++++++++++++++++++ internal/api/oidc/token_exchange.go | 127 +++++++++++++-- internal/api/oidc/token_exchange_test.go | 112 +++++++++++++ 3 files changed, 378 insertions(+), 9 deletions(-) create mode 100644 internal/api/oidc/token_exchange_test.go diff --git a/internal/api/oidc/integration_test/token_exchange_test.go b/internal/api/oidc/integration_test/token_exchange_test.go index fca9aea15..dfba1b8ed 100644 --- a/internal/api/oidc/integration_test/token_exchange_test.go +++ b/internal/api/oidc/integration_test/token_exchange_test.go @@ -19,8 +19,11 @@ import ( "google.golang.org/grpc/status" oidc_api "github.com/zitadel/zitadel/internal/api/oidc" + "github.com/zitadel/zitadel/internal/domain" "github.com/zitadel/zitadel/internal/integration" "github.com/zitadel/zitadel/pkg/grpc/admin" + "github.com/zitadel/zitadel/pkg/grpc/app" + "github.com/zitadel/zitadel/pkg/grpc/management" ) func setImpersonationPolicy(t *testing.T, instance *integration.Instance, value bool) { @@ -477,6 +480,105 @@ func TestServer_TokenExchangeImpersonation(t *testing.T) { verifyIDToken: idTokenVerifier(ctx, relyingParty, userResp.GetUserId(), orgUserID), }, }, + { + name: "IMPERSONATION: subject: userID, actor: access token, reduced scopes, success", + args: args{ + SubjectToken: userResp.GetUserId(), + SubjectTokenType: oidc_api.UserIDTokenType, + RequestedTokenType: oidc.AccessTokenType, + ActorToken: orgImpersonatorPAT, + ActorTokenType: oidc.AccessTokenType, + Scopes: patScopes[:2], + }, + want: result{ + issuedTokenType: oidc.AccessTokenType, + tokenType: oidc.BearerToken, + expiresIn: 43100, + scopes: patScopes[:2], + verifyAccessToken: accessTokenVerifier(ctx, resourceServer, userResp.GetUserId(), orgUserID), + verifyIDToken: idTokenVerifier(ctx, relyingParty, userResp.GetUserId(), orgUserID), + }, + }, + { + name: "IMPERSONATION: subject: userID, actor: access token, email scope not on actor, success", + args: args{ + SubjectToken: userResp.GetUserId(), + SubjectTokenType: oidc_api.UserIDTokenType, + RequestedTokenType: oidc.AccessTokenType, + ActorToken: orgImpersonatorPAT, + ActorTokenType: oidc.AccessTokenType, + Scopes: []string{oidc.ScopeOpenID, oidc.ScopeProfile, oidc.ScopeEmail}, + }, + want: result{ + issuedTokenType: oidc.AccessTokenType, + tokenType: oidc.BearerToken, + expiresIn: 43100, + scopes: oidc.SpaceDelimitedArray{oidc.ScopeOpenID, oidc.ScopeProfile, oidc.ScopeEmail}, + verifyAccessToken: accessTokenVerifier(ctx, resourceServer, userResp.GetUserId(), orgUserID), + verifyIDToken: idTokenVerifier(ctx, relyingParty, userResp.GetUserId(), orgUserID), + }, + }, + { + name: "IMPERSONATION: subject: userID, actor: access token, openid only, success", + args: args{ + SubjectToken: userResp.GetUserId(), + SubjectTokenType: oidc_api.UserIDTokenType, + RequestedTokenType: oidc.AccessTokenType, + ActorToken: orgImpersonatorPAT, + ActorTokenType: oidc.AccessTokenType, + Scopes: []string{oidc.ScopeOpenID}, + }, + want: result{ + issuedTokenType: oidc.AccessTokenType, + tokenType: oidc.BearerToken, + expiresIn: 43100, + scopes: oidc.SpaceDelimitedArray{oidc.ScopeOpenID}, + verifyAccessToken: accessTokenVerifier(ctx, resourceServer, userResp.GetUserId(), orgUserID), + }, + }, + { + name: "IMPERSONATION: subject: userID, actor: access token, offline_access not on actor, rejected", + args: args{ + SubjectToken: userResp.GetUserId(), + SubjectTokenType: oidc_api.UserIDTokenType, + RequestedTokenType: oidc.AccessTokenType, + ActorToken: orgImpersonatorPAT, + ActorTokenType: oidc.AccessTokenType, + Scopes: []string{oidc.ScopeOpenID, oidc.ScopeOfflineAccess}, + }, + wantErr: true, + }, + { + name: "IMPERSONATION: subject: ID token, actor: access token, email scope not on actor, success", + args: args{ + SubjectToken: teResp.IDToken, + SubjectTokenType: oidc.IDTokenType, + RequestedTokenType: oidc.AccessTokenType, + ActorToken: orgImpersonatorPAT, + ActorTokenType: oidc.AccessTokenType, + Scopes: []string{oidc.ScopeOpenID, oidc.ScopeProfile, oidc.ScopeEmail}, + }, + want: result{ + issuedTokenType: oidc.AccessTokenType, + tokenType: oidc.BearerToken, + expiresIn: 43100, + scopes: oidc.SpaceDelimitedArray{oidc.ScopeOpenID, oidc.ScopeProfile, oidc.ScopeEmail}, + verifyAccessToken: accessTokenVerifier(ctx, resourceServer, serviceUserID, orgUserID), + verifyIDToken: idTokenVerifier(ctx, relyingParty, serviceUserID, orgUserID), + }, + }, + { + name: "IMPERSONATION: subject: access token, actor: access token, email not on input tokens, rejected", + args: args{ + SubjectToken: teResp.AccessToken, + SubjectTokenType: oidc.AccessTokenType, + RequestedTokenType: oidc.AccessTokenType, + ActorToken: orgImpersonatorPAT, + ActorTokenType: oidc.AccessTokenType, + Scopes: []string{oidc.ScopeOpenID, oidc.ScopeProfile, oidc.ScopeEmail}, + }, + wantErr: true, + }, { name: "ORG IMPERSONATION: subject: access token, actor: access token, success", args: args{ @@ -586,6 +688,52 @@ func TestServer_TokenExchangeImpersonation(t *testing.T) { } } +func TestServer_TokenExchangeImpersonationClientCredentialsActor(t *testing.T) { + instance := integration.NewInstance(CTX) + ctx := instance.WithAuthorization(CTX, integration.UserTypeIAMOwner) + userResp := instance.CreateHumanUser(ctx) + + setImpersonationPolicy(t, instance, true) + + project := instance.CreateProject(ctx, t, "", integration.ProjectName(), false, false) + client, keyData, err := instance.CreateOIDCWebClientJWT(ctx, "", "", project.GetId(), + app.OIDCGrantType_OIDC_GRANT_TYPE_TOKEN_EXCHANGE, + app.OIDCGrantType_OIDC_GRANT_TYPE_AUTHORIZATION_CODE, + app.OIDCGrantType_OIDC_GRANT_TYPE_REFRESH_TOKEN, + ) + require.NoError(t, err) + signer, err := rp.SignerFromKeyFile(keyData)() + require.NoError(t, err) + exchanger, err := tokenexchange.NewTokenExchangerJWTProfile(ctx, instance.OIDCIssuer(), client.GetClientId(), signer) + require.NoError(t, err) + resourceServer, err := instance.CreateResourceServerJWTProfile(ctx, keyData) + require.NoError(t, err) + + impersonatorUser := instance.CreateMachineUser(ctx) + _, err = instance.Client.Mgmt.AddOrgMember(ctx, &management.AddOrgMemberRequest{ + UserId: impersonatorUser.GetUserId(), + Roles: []string{"ORG_ADMIN_IMPERSONATOR"}, + }) + require.NoError(t, err) + secret, err := instance.Client.Mgmt.GenerateMachineSecret(ctx, &management.GenerateMachineSecretRequest{ + UserId: impersonatorUser.GetUserId(), + }) + require.NoError(t, err) + + audScope := domain.ProjectIDScope + project.GetId() + domain.AudSuffix + actorScopes := []string{oidc.ScopeOpenID, oidc.ScopeProfile, oidc_api.ScopeProjectsRoles, audScope} + credentialsProvider, err := rp.NewRelyingPartyOIDC(ctx, instance.OIDCIssuer(), secret.GetClientId(), secret.GetClientSecret(), "", actorScopes) + require.NoError(t, err) + actorTokens, err := rp.ClientCredentials(ctx, credentialsProvider, nil) + require.NoError(t, err) + + step2Scopes := []string{oidc.ScopeOpenID, oidc.ScopeProfile, oidc.ScopeEmail, oidc_api.ScopeProjectsRoles, audScope} + got, err := tokenexchange.ExchangeToken(ctx, exchanger, userResp.GetUserId(), oidc_api.UserIDTokenType, actorTokens.AccessToken, oidc.AccessTokenType, nil, nil, step2Scopes, oidc.AccessTokenType) + require.NoError(t, err) + assert.Equal(t, oidc.SpaceDelimitedArray(step2Scopes), got.Scopes) + accessTokenVerifier(ctx, resourceServer, userResp.GetUserId(), impersonatorUser.GetUserId())(t, got.AccessToken) +} + // This test tries to call the zitadel API with an impersonated token, // which should fail. func TestImpersonation_API_Call(t *testing.T) { diff --git a/internal/api/oidc/token_exchange.go b/internal/api/oidc/token_exchange.go index 68c176bca..eff9b8d6a 100644 --- a/internal/api/oidc/token_exchange.go +++ b/internal/api/oidc/token_exchange.go @@ -3,6 +3,7 @@ package oidc import ( "context" "slices" + "strings" "time" "github.com/zitadel/oidc/v3/pkg/oidc" @@ -57,7 +58,12 @@ func (s *Server) tokenExchange(ctx context.Context, r *op.ClientRequest[oidc.Tok } actorToken := subjectToken // see [createExchangeTokens] comment. + // actorPath: a separate actor_token was verified (impersonation/delegation). + // Standard exchange leaves actorToken == subjectToken; this branch runs for + // user_id/JWT subjects or when the client sends actor_token explicitly. + actorPath := false if subjectToken.tokenType == UserIDTokenType || subjectToken.tokenType == oidc.JWTTokenType || r.Data.ActorToken != "" { + actorPath = true if !authz.GetInstance(ctx).EnableImpersonation() { return nil, zerrors.ThrowPermissionDenied(nil, "OIDC-Fae5w", "Errors.TokenExchange.Impersonation.PolicyDisabled") } @@ -75,7 +81,13 @@ func (s *Server) tokenExchange(ctx context.Context, r *op.ClientRequest[oidc.Tok if err != nil { return nil, err } - scopes, err := validateTokenExchangeScopes(client, r.Data.Scopes, subjectToken.scopes, actorToken.scopes) + // scopelessActorPath: actorPath with a subject that cannot carry scopes + // (user_id or id_token). Subject-data scopes (email, profile, …) are taken + // from the impersonated user at mint time; only authorization scopes must + // appear on subject_token ∪ actor_token. Access/JWT subjects always use + // union validation even when their scope claim is empty. + scopelessActorPath := actorPath && (subjectToken.tokenType == UserIDTokenType || subjectToken.tokenType == oidc.IDTokenType) + scopes, err := validateTokenExchangeScopes(client, r.Data.Scopes, subjectToken.scopes, actorToken.scopes, scopelessActorPath) if err != nil { return nil, err } @@ -178,25 +190,122 @@ func (s *Server) jwtProfileUserCheck(ctx context.Context, resourceOwner *string, }) } -func validateTokenExchangeScopes(client *Client, requestedScopes, subjectScopes, actorScopes []string) ([]string, error) { - // Scope always has 1 empty string if the space delimited array was an empty string. - requestedScopes = slices.DeleteFunc(requestedScopes, func(s string) bool { - return s == "" - }) +// validateTokenExchangeScopes picks the scope rule set for this exchange. +// scopelessActorPath selects impersonation rules (user_id/id_token subjects +// on the actor path); otherwise every requested scope must ⊆ subject ∪ actor. +func validateTokenExchangeScopes( + client *Client, + requestedScopes, subjectScopes, actorScopes []string, + scopelessActorPath bool, +) ([]string, error) { + requestedScopes = normalizeRequestedScopes(requestedScopes) + if scopelessActorPath { + return validateImpersonationTokenExchangeScopes(client, requestedScopes, subjectScopes, actorScopes) + } + + return validateUnionTokenExchangeScopes(client, requestedScopes, subjectScopes, actorScopes) +} + +// validateUnionTokenExchangeScopes applies when the requested scopes must be a subset +// of the scopes present on the subject and/or actor tokens (union). +// This covers standard exchange (actor == subject) and actor-path exchanges with +// access/JWT subject tokens. Actor-path exchanges with scope-less subjects +// (user_id, id_token) use validateImpersonationTokenExchangeScopes instead. +// Rule: every scope must be present on the subject or actor token (union). +// For standard exchange both lists are identical, so this is effectively +// "subset of subject token only". +func validateUnionTokenExchangeScopes( + client *Client, + requestedScopes, subjectScopes, actorScopes []string, +) ([]string, error) { if len(requestedScopes) == 0 { - return op.ValidateAuthReqScopes(client, actorScopes) + requestedScopes = subjectScopes + } + if len(requestedScopes) == 0 { + requestedScopes = actorScopes } for _, scope := range requestedScopes { - if !slices.Contains(subjectScopes, scope) || !slices.Contains(actorScopes, scope) { - return nil, oidc.ErrInvalidScope().WithDescription("scope %q not found in subject or actor token", scope) + if !scopeInUnion(scope, subjectScopes, actorScopes) { + return nil, oidc.ErrInvalidScope(). + WithDescription("scope %q not found in subject or actor token", scope) } } return op.ValidateAuthReqScopes(client, requestedScopes) } +// validateImpersonationTokenExchangeScopes applies when the subject token +// cannot carry scopes (user_id, id_token). The actor token authorizes the +// request; impersonation permission is checked later in CreateOIDCSession. +// +// Two scope classes: +// - authorization scopes: privilege/audience/roles — must be on input tokens +// - subject-data scopes: user claims (email, profile, …) — client allowlist only +func validateImpersonationTokenExchangeScopes( + client *Client, + requestedScopes, subjectScopes, actorScopes []string, +) ([]string, error) { + // Subject has no scopes to inherit; fall back to actor. + if len(requestedScopes) == 0 { + requestedScopes = actorScopes + } + + for _, scope := range requestedScopes { + // Prevent privilege escalation: offline_access, audiences, roles, etc. + // must still come from subject or actor token. + if isTokenExchangeAuthorizationScope(scope) && !scopeInUnion(scope, subjectScopes, actorScopes) { + return nil, oidc.ErrInvalidScope(). + WithDescription("scope %q not found in subject or actor token", scope) + } + + // Subject-data scopes (openid, profile, email, urn:zitadel:iam:user:*, …) + // skip the union check — validated by client allowlist below. + } + + return op.ValidateAuthReqScopes(client, requestedScopes) +} + +func normalizeRequestedScopes(scopes []string) []string { + // Space-delimited empty scope produces a single "" entry. + return slices.DeleteFunc(scopes, func(s string) bool { + return s == "" + }) +} + +func scopeInUnion(scope string, subjectScopes, actorScopes []string) bool { + return slices.Contains(subjectScopes, scope) || slices.Contains(actorScopes, scope) +} + +// isTokenExchangeAuthorizationScope identifies scopes that control what a token +// may do (audiences, refresh, roles), not which user claims to embed. +func isTokenExchangeAuthorizationScope(scope string) bool { + switch scope { + case oidc.ScopeOfflineAccess, ScopeProjectsRoles: + return true + } + if strings.HasPrefix(scope, ScopeProjectRolePrefix) { + return true // urn:zitadel:iam:org:project:role:* + } + if strings.HasPrefix(scope, domain.ProjectIDScope) { + return true // urn:zitadel:iam:org:project:id:*:aud + } + if strings.HasPrefix(scope, domain.OrgIDScope) { + return true + } + if strings.HasPrefix(scope, domain.OrgDomainPrimaryScope) { + return true + } + if strings.HasPrefix(scope, domain.OrgRoleIDScope) { + return true + } + if strings.HasPrefix(scope, domain.SelectIDPScope) { + return true + } + return false +} + func validateTokenExchangeAudience(requestedAudience, subjectAudience, actorAudience []string) ([]string, error) { if len(requestedAudience) == 0 { if len(subjectAudience) > 0 { diff --git a/internal/api/oidc/token_exchange_test.go b/internal/api/oidc/token_exchange_test.go new file mode 100644 index 000000000..fa2efee44 --- /dev/null +++ b/internal/api/oidc/token_exchange_test.go @@ -0,0 +1,112 @@ +package oidc + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/zitadel/oidc/v3/pkg/oidc" + + "github.com/zitadel/zitadel/internal/domain" +) + +func testTokenExchangeClient() *Client { + return &Client{} +} + +func Test_validateTokenExchangeScopes(t *testing.T) { + client := testTokenExchangeClient() + actorScopes := []string{ + oidc.ScopeOpenID, + oidc.ScopeProfile, + ScopeProjectsRoles, + ScopeProjectRolePrefix + "TOKEN_EXCHANGE_IMPERSONATOR", + domain.ProjectIDScope + "288694447250664408" + domain.AudSuffix, + } + + t.Run("exchange: subset of subject", func(t *testing.T) { + subjectScopes := []string{oidc.ScopeOpenID, oidc.ScopeProfile, oidc.ScopeOfflineAccess} + got, err := validateTokenExchangeScopes(client, + []string{oidc.ScopeOpenID, oidc.ScopeProfile}, + subjectScopes, subjectScopes, false) + require.NoError(t, err) + assert.Equal(t, []string{oidc.ScopeOpenID, oidc.ScopeProfile}, got) + }) + + t.Run("exchange: escalation rejected", func(t *testing.T) { + subjectScopes := []string{oidc.ScopeOpenID} + _, err := validateTokenExchangeScopes(client, + []string{oidc.ScopeOpenID, oidc.ScopeOfflineAccess}, + subjectScopes, subjectScopes, false) + require.Error(t, err) + }) + + t.Run("impersonation: subject-data scope not on actor allowed", func(t *testing.T) { + got, err := validateTokenExchangeScopes(client, + []string{oidc.ScopeOpenID, oidc.ScopeProfile, oidc.ScopeEmail}, + nil, actorScopes, true) + require.NoError(t, err) + assert.Contains(t, got, oidc.ScopeEmail) + }) + + t.Run("impersonation: authorization scope not on actor rejected", func(t *testing.T) { + _, err := validateTokenExchangeScopes(client, + []string{oidc.ScopeOpenID, oidc.ScopeOfflineAccess}, + nil, actorScopes, true) + require.Error(t, err) + }) + + t.Run("impersonation: authorization scope on actor allowed", func(t *testing.T) { + got, err := validateTokenExchangeScopes(client, + []string{oidc.ScopeOpenID, ScopeProjectsRoles, domain.ProjectIDScope + "288694447250664408" + domain.AudSuffix}, + nil, actorScopes, true) + require.NoError(t, err) + assert.Contains(t, got, ScopeProjectsRoles) + }) + + t.Run("impersonation: omitted scope inherits actor", func(t *testing.T) { + got, err := validateTokenExchangeScopes(client, nil, nil, actorScopes, true) + require.NoError(t, err) + assert.Equal(t, []string{ + oidc.ScopeOpenID, + oidc.ScopeProfile, + ScopeProjectsRoles, + domain.ProjectIDScope + "288694447250664408" + domain.AudSuffix, + }, got) + }) + + t.Run("actor path with subject scopes uses exchange validator", func(t *testing.T) { + subjectScopes := []string{oidc.ScopeOpenID, oidc.ScopeProfile} + _, err := validateTokenExchangeScopes(client, + []string{oidc.ScopeEmail}, + subjectScopes, actorScopes, false) + require.Error(t, err) + }) + + t.Run("actor path with JWT subject and empty scopes uses exchange validator", func(t *testing.T) { + _, err := validateTokenExchangeScopes(client, + []string{oidc.ScopeEmail}, + nil, actorScopes, false) + require.Error(t, err) + }) +} + +func Test_isTokenExchangeAuthorizationScope(t *testing.T) { + tests := []struct { + scope string + want bool + }{ + {oidc.ScopeOpenID, false}, + {oidc.ScopeEmail, false}, + {oidc.ScopeOfflineAccess, true}, + {ScopeProjectsRoles, true}, + {ScopeProjectRolePrefix + "admin", true}, + {domain.ProjectIDScope + "proj" + domain.AudSuffix, true}, + {ScopeUserMetaData, false}, + } + for _, tt := range tests { + t.Run(tt.scope, func(t *testing.T) { + assert.Equal(t, tt.want, isTokenExchangeAuthorizationScope(tt.scope)) + }) + } +}