diff --git a/internal/api/oidc/integration_test/auth_request_test.go b/internal/api/oidc/integration_test/auth_request_test.go index 403360907..e42ca1513 100644 --- a/internal/api/oidc/integration_test/auth_request_test.go +++ b/internal/api/oidc/integration_test/auth_request_test.go @@ -117,6 +117,62 @@ func TestOPStorage_CreateAccessToken_code(t *testing.T) { } } +func TestOPStorage_CreateAccessToken_code_invalid_client(t *testing.T) { + tests := []struct { + name string + authorizeClientID string + codeExchangeClientID string + authRequestID func(t testing.TB, instance *integration.Instance, clientID, redirectURI string, scope ...string) string + }{ + { + name: "login header", + authorizeClientID: func() string { + clientID, _ := createClient(t, Instance) + return clientID + }(), + codeExchangeClientID: func() string { + clientID, _ := createClient(t, Instance) + return clientID + }(), + authRequestID: createAuthRequest, + }, + { + name: "login v2 config", + authorizeClientID: func() string { + clientID, _ := createClientLoginV2(t, Instance) + return clientID + }(), + codeExchangeClientID: func() string { + clientID, _ := createClientLoginV2(t, Instance) + return clientID + }(), + authRequestID: createAuthRequestNoLoginClientHeader, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + authRequestID := createAuthRequest(t, Instance, tt.authorizeClientID, redirectURI) + sessionID, sessionToken, _, _ := Instance.CreateVerifiedWebAuthNSession(t, CTXLOGIN, User.GetUserId()) + linkResp, err := Instance.Client.OIDCv2.CreateCallback(CTXLOGIN, &oidc_pb.CreateCallbackRequest{ + AuthRequestId: authRequestID, + CallbackKind: &oidc_pb.CreateCallbackRequest_Session{ + Session: &oidc_pb.Session{ + SessionId: sessionID, + SessionToken: sessionToken, + }, + }, + }) + require.NoError(t, err) + + code := assertCodeResponse(t, linkResp.GetCallbackUrl()) + // the client used for code exchange does not match the client in the auth request -> must fail + tokens, err := exchangeTokens(t, Instance, tt.codeExchangeClientID, code, redirectURI) + require.ErrorContains(t, err, "client_id does not correspond to the client_id in the authorization request") + assert.Nil(t, tokens) + }) + } +} + func TestOPStorage_CreateAccessToken_implicit(t *testing.T) { tests := []struct { name string @@ -295,6 +351,68 @@ func TestOPStorage_CreateAccessAndRefreshTokens_refresh(t *testing.T) { } } +func TestOPStorage_CreateAccessAndRefreshTokens_refresh_invalid_client(t *testing.T) { + tests := []struct { + name string + clientID string + otherClientID string + authRequestID func(t testing.TB, instance *integration.Instance, clientID, redirectURI string, scope ...string) string + }{ + { + name: "login header", + clientID: func() string { + clientID, _ := createClient(t, Instance) + return clientID + }(), + otherClientID: func() string { + clientID, _ := createClient(t, Instance) + return clientID + }(), + authRequestID: createAuthRequest, + }, + { + name: "login v2 config", + clientID: func() string { + clientID, _ := createClientLoginV2(t, Instance) + return clientID + }(), + otherClientID: func() string { + clientID, _ := createClient(t, Instance) + return clientID + }(), + authRequestID: createAuthRequestNoLoginClientHeader, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + authRequestID := tt.authRequestID(t, Instance, tt.clientID, redirectURI, oidc.ScopeOpenID, oidc.ScopeOfflineAccess) + sessionID, sessionToken, startTime, changeTime := Instance.CreateVerifiedWebAuthNSession(t, CTXLOGIN, User.GetUserId()) + linkResp, err := Instance.Client.OIDCv2.CreateCallback(CTXLOGIN, &oidc_pb.CreateCallbackRequest{ + AuthRequestId: authRequestID, + CallbackKind: &oidc_pb.CreateCallbackRequest_Session{ + Session: &oidc_pb.Session{ + SessionId: sessionID, + SessionToken: sessionToken, + }, + }, + }) + require.NoError(t, err) + + // code exchange + code := assertCodeResponse(t, linkResp.GetCallbackUrl()) + tokens, err := exchangeTokens(t, Instance, tt.clientID, code, redirectURI) + require.NoError(t, err) + assertTokens(t, tokens, true) + assertIDTokenClaims(t, tokens.IDTokenClaims, User.GetUserId(), armPasskey, startTime, changeTime, sessionID) + + // refresh grant with a different client -> must fail + newTokens, err := refreshTokens(t, tt.otherClientID, tokens.RefreshToken) + require.ErrorContains(t, err, "client_id does not correspond to the client_id in the refresh token") + assert.Nil(t, newTokens) + }) + } +} + func TestOPStorage_RevokeToken_access_token(t *testing.T) { clientID, _ := createClient(t, Instance) provider, err := Instance.CreateRelyingParty(CTX, clientID, redirectURI) diff --git a/internal/api/oidc/integration_test/token_device_test.go b/internal/api/oidc/integration_test/token_device_test.go index 81f385499..c0b4d4d47 100644 --- a/internal/api/oidc/integration_test/token_device_test.go +++ b/internal/api/oidc/integration_test/token_device_test.go @@ -124,3 +124,58 @@ func TestServer_DeviceAuth(t *testing.T) { }) } } + +func TestServer_DeviceAuth_invalid_client(t *testing.T) { + project := Instance.CreateProject(CTX, t, "", integration.ProjectName(), false, false) + client, err := Instance.CreateOIDCClient(CTX, redirectURI, logoutRedirectURI, project.GetId(), app.OIDCAppType_OIDC_APP_TYPE_NATIVE, app.OIDCAuthMethodType_OIDC_AUTH_METHOD_TYPE_NONE, false, app.OIDCGrantType_OIDC_GRANT_TYPE_DEVICE_CODE) + require.NoError(t, err) + + scope := []string{oidc.ScopeOpenID, oidc.ScopeProfile, oidc.ScopeEmail} + + provider, err := rp.NewRelyingPartyOIDC(CTX, Instance.OIDCIssuer(), client.GetClientId(), "", "", scope) + require.NoError(t, err) + + deviceAuthorization, err := rp.DeviceAuthorization(CTX, scope, provider, nil) + require.NoError(t, err) + + relyingPartyDone := make(chan struct{}) + go func() { + ctx, cancel := context.WithTimeout(CTX, 1*time.Minute) + defer func() { + cancel() + relyingPartyDone <- struct{}{} + }() + + var req *oidc_pb.GetDeviceAuthorizationRequestResponse + retryDuration, tick := integration.WaitForAndTickWithMaxDuration(CTX, time.Minute) + assert.EventuallyWithT(t, func(collectT *assert.CollectT) { + req, err = Instance.Client.OIDCv2.GetDeviceAuthorizationRequest(CTX, &oidc_pb.GetDeviceAuthorizationRequestRequest{ + UserCode: deviceAuthorization.UserCode, + }) + assert.NoError(collectT, err) + }, retryDuration, tick) + sessionID, sessionToken, _, _ := Instance.CreateVerifiedWebAuthNSession(t, CTXLOGIN, User.GetUserId()) + _, err = Instance.Client.OIDCv2.AuthorizeOrDenyDeviceAuthorization(CTXLOGIN, &oidc_pb.AuthorizeOrDenyDeviceAuthorizationRequest{ + DeviceAuthorizationId: req.GetDeviceAuthorizationRequest().GetId(), + Decision: &oidc_pb.AuthorizeOrDenyDeviceAuthorizationRequest_Session{ + Session: &oidc_pb.Session{ + SessionId: sessionID, + SessionToken: sessionToken, + }, + }, + }) + require.NoError(t, err) + + otherClient, err := Instance.CreateOIDCClient(CTX, redirectURI, logoutRedirectURI, project.GetId(), app.OIDCAppType_OIDC_APP_TYPE_NATIVE, app.OIDCAuthMethodType_OIDC_AUTH_METHOD_TYPE_NONE, false, app.OIDCGrantType_OIDC_GRANT_TYPE_DEVICE_CODE) + require.NoError(t, err) + + otherProvider, err := rp.NewRelyingPartyOIDC(CTX, Instance.OIDCIssuer(), otherClient.GetClientId(), "", "", scope) + require.NoError(t, err) + tokens, err := rp.DeviceAccessToken(ctx, deviceAuthorization.DeviceCode, time.Duration(deviceAuthorization.Interval)*time.Second, otherProvider) + require.ErrorIs(t, err, oidc.ErrInvalidClient()) + require.ErrorContains(t, err, "client_id does not correspond to the client_id in the authorization request") + require.Nil(t, tokens) + }() + + <-relyingPartyDone +} diff --git a/internal/api/oidc/token_code.go b/internal/api/oidc/token_code.go index e52bd2039..2ba7e041c 100644 --- a/internal/api/oidc/token_code.go +++ b/internal/api/oidc/token_code.go @@ -62,6 +62,10 @@ func (s *Server) codeExchangeV1(ctx context.Context, client *Client, req *oidc.A return nil, err } + if authReq.GetClientID() != client.client.ClientID { + return nil, oidc.ErrInvalidClient().WithDescription("client_id does not correspond to the client_id in the authorization request") + } + if challenge := authReq.GetCodeChallenge(); challenge != nil || client.AuthMethod() == oidc.AuthMethodNone { if err = op.AuthorizeCodeChallenge(req.CodeVerifier, challenge); err != nil { return nil, err @@ -121,6 +125,9 @@ func (s *Server) getAuthRequestV1ByID(ctx context.Context, id string) (*AuthRequ func codeExchangeComplianceChecker(client *Client, req *oidc.AccessTokenRequest) command.AuthRequestComplianceChecker { return func(ctx context.Context, authReq *command.AuthRequestWriteModel) error { + if authReq.ClientID != client.client.ClientID { + return oidc.ErrInvalidClient().WithDescription("client_id does not correspond to the client_id in the authorization request") + } if authReq.CodeChallenge != nil || client.AuthMethod() == oidc.AuthMethodNone { err := op.AuthorizeCodeChallenge(req.CodeVerifier, CodeChallengeToOIDC(authReq.CodeChallenge)) if err != nil { diff --git a/internal/api/oidc/token_device.go b/internal/api/oidc/token_device.go index 39b6686ed..63e3c2a8b 100644 --- a/internal/api/oidc/token_device.go +++ b/internal/api/oidc/token_device.go @@ -25,13 +25,16 @@ func (s *Server) DeviceToken(ctx context.Context, r *op.ClientRequest[oidc.Devic if !ok { return nil, zerrors.ThrowInternal(nil, "OIDC-Ae2ph", "Error.Internal") } - session, err := s.command.CreateOIDCSessionFromDeviceAuth(ctx, r.Data.DeviceCode, client.client.BackChannelLogoutURI) + session, err := s.command.CreateOIDCSessionFromDeviceAuth(ctx, r.Data.DeviceCode, client.client.BackChannelLogoutURI, client.client.ClientID) if err == nil { return response(s.accessTokenResponseFromSession(ctx, client, session, "", client.client.ProjectID, client.client.ProjectRoleAssertion, client.client.AccessTokenRoleAssertion, client.client.IDTokenRoleAssertion, client.client.IDTokenUserinfoAssertion)) } if errors.Is(err, context.DeadlineExceeded) { return nil, oidc.ErrSlowDown().WithParent(err).WithReturnParentToClient(authz.GetFeatures(ctx).DebugOIDCParentError) } + if errors.Is(err, oidc.ErrInvalidClient()) { + return nil, err + } var target command.DeviceAuthStateError if errors.As(err, &target) { diff --git a/internal/api/oidc/token_refresh.go b/internal/api/oidc/token_refresh.go index 4a965ae19..8de060dc7 100644 --- a/internal/api/oidc/token_refresh.go +++ b/internal/api/oidc/token_refresh.go @@ -26,7 +26,7 @@ func (s *Server) RefreshToken(ctx context.Context, r *op.ClientRequest[oidc.Refr return nil, zerrors.ThrowInternal(nil, "OIDC-ga0EP", "Error.Internal") } - session, err := s.command.ExchangeOIDCSessionRefreshAndAccessToken(ctx, r.Data.RefreshToken, r.Data.Scopes, refreshTokenComplianceChecker()) + session, err := s.command.ExchangeOIDCSessionRefreshAndAccessToken(ctx, r.Data.RefreshToken, r.Data.Scopes, client.client.ClientID, refreshTokenComplianceChecker()) if err == nil { return response(s.accessTokenResponseFromSession(ctx, client, session, "", client.client.ProjectID, client.client.ProjectRoleAssertion, client.client.AccessTokenRoleAssertion, client.client.IDTokenRoleAssertion, client.client.IDTokenUserinfoAssertion)) } else if errors.Is(err, zerrors.ThrowPreconditionFailed(nil, "OIDCS-JOI23", "Errors.OIDCSession.RefreshTokenInvalid")) { @@ -86,7 +86,10 @@ func (s *Server) refreshTokenV1(ctx context.Context, client *Client, r *op.Clien // refreshTokenComplianceChecker validates that the requested scope is a subset of the original auth request scope. func refreshTokenComplianceChecker() command.RefreshTokenComplianceChecker { - return func(_ context.Context, model *command.OIDCSessionWriteModel, requestedScope []string) ([]string, error) { + return func(_ context.Context, model *command.OIDCSessionWriteModel, requestedScope []string, reqClientID string) ([]string, error) { + if model.ClientID != reqClientID { + return nil, oidc.ErrInvalidClient().WithDescription("client_id does not correspond to the client_id in the refresh token") + } return validateRefreshTokenScopes(model.Scope, requestedScope) } } diff --git a/internal/command/device_auth.go b/internal/command/device_auth.go index ca3e2e2cc..dd2c2958b 100644 --- a/internal/command/device_auth.go +++ b/internal/command/device_auth.go @@ -5,6 +5,7 @@ import ( "fmt" "time" + "github.com/zitadel/oidc/v3/pkg/oidc" "golang.org/x/text/language" "github.com/zitadel/zitadel/internal/api/authz" @@ -184,7 +185,7 @@ func (e DeviceAuthStateError) Error() string { // As devices can poll at various intervals, an explicit state takes precedence over expiry. // This is to prevent cases where users might approve or deny the authorization on time, but the next poll // happens after expiry. -func (c *Commands) CreateOIDCSessionFromDeviceAuth(ctx context.Context, deviceCode, backChannelLogoutURI string) (_ *OIDCSession, err error) { +func (c *Commands) CreateOIDCSessionFromDeviceAuth(ctx context.Context, deviceCode, backChannelLogoutURI, clientID string) (_ *OIDCSession, err error) { ctx, span := tracing.NewSpan(ctx) defer func() { span.EndWithError(err) }() @@ -193,6 +194,10 @@ func (c *Commands) CreateOIDCSessionFromDeviceAuth(ctx context.Context, deviceCo return nil, err } + if deviceAuthModel.ClientID != clientID { + return nil, oidc.ErrInvalidClient().WithDescription("client_id does not correspond to the client_id in the authorization request") + } + switch deviceAuthModel.State { case domain.DeviceAuthStateApproved: break diff --git a/internal/command/oidc_session.go b/internal/command/oidc_session.go index 3909a865b..b52e1a21c 100644 --- a/internal/command/oidc_session.go +++ b/internal/command/oidc_session.go @@ -192,11 +192,11 @@ func (c *Commands) CreateOIDCSession(ctx context.Context, return session, nil } -type RefreshTokenComplianceChecker func(ctx context.Context, wm *OIDCSessionWriteModel, requestedScope []string) (scope []string, err error) +type RefreshTokenComplianceChecker func(ctx context.Context, wm *OIDCSessionWriteModel, requestedScope []string, reqClientID string) (scope []string, err error) // ExchangeOIDCSessionRefreshAndAccessToken updates an existing OIDC Session, creates a new access and refresh token. // It returns the access token id and expiration and the new refresh token. -func (c *Commands) ExchangeOIDCSessionRefreshAndAccessToken(ctx context.Context, refreshToken string, scope []string, complianceCheck RefreshTokenComplianceChecker) (_ *OIDCSession, err error) { +func (c *Commands) ExchangeOIDCSessionRefreshAndAccessToken(ctx context.Context, refreshToken string, scope []string, reqClientID string, complianceCheck RefreshTokenComplianceChecker) (_ *OIDCSession, err error) { ctx, span := tracing.NewSpan(ctx) defer func() { span.EndWithError(err) }() @@ -204,7 +204,7 @@ func (c *Commands) ExchangeOIDCSessionRefreshAndAccessToken(ctx context.Context, if err != nil { return nil, err } - scope, err = complianceCheck(ctx, cmd.oidcSessionWriteModel, scope) + scope, err = complianceCheck(ctx, cmd.oidcSessionWriteModel, scope, reqClientID) if err != nil { return nil, err } diff --git a/internal/command/oidc_session_test.go b/internal/command/oidc_session_test.go index 717afc9d5..fe2db4446 100644 --- a/internal/command/oidc_session_test.go +++ b/internal/command/oidc_session_test.go @@ -1644,10 +1644,13 @@ func TestCommands_CreateOIDCSession(t *testing.T) { } func mockRefreshTokenComplianceChecker(returnErr error) RefreshTokenComplianceChecker { - return func(_ context.Context, wm *OIDCSessionWriteModel, scope []string) ([]string, error) { + return func(_ context.Context, wm *OIDCSessionWriteModel, scope []string, clientID string) ([]string, error) { if returnErr != nil { return nil, returnErr } + if wm.ClientID != clientID { + return nil, zerrors.ThrowInvalidArgument(nil, "test", "invalid clientID") + } if len(scope) > 0 { return scope, nil } @@ -1668,6 +1671,7 @@ func TestCommands_ExchangeOIDCSessionRefreshAndAccessToken(t *testing.T) { ctx context.Context refreshToken string scope []string + reqClientID string complianceCheck RefreshTokenComplianceChecker } type res struct { @@ -1831,6 +1835,58 @@ func TestCommands_ExchangeOIDCSessionRefreshAndAccessToken(t *testing.T) { err: zerrors.ThrowPreconditionFailed(nil, "OIDCS-J39h2", "Errors.User.NotActive"), }, }, + { + "refresh with an invalid client id fails", + fields{ + eventstore: expectEventstore( + expectFilter( + eventFromEventPusherWithCreationDateNow( + oidcsession.NewAddedEvent(context.Background(), &oidcsession.NewAggregate("V2_oidcSessionID", "org1").Aggregate, + "userID", "org1", "sessionID", "clientID", []string{"audience"}, []string{"openid", "profile", "offline_access"}, + []domain.UserAuthMethodType{domain.UserAuthMethodTypePassword}, testNow, "nonce", &language.Afrikaans, + &domain.UserAgent{FingerprintID: gu.Ptr("browserFP")}, + ), + ), + eventFromEventPusherWithCreationDateNow( + oidcsession.NewAccessTokenAddedEvent(context.Background(), &oidcsession.NewAggregate("V2_oidcSessionID", "org1").Aggregate, + "at_accessTokenID", []string{"openid", "profile", "offline_access"}, time.Hour, domain.TokenReasonAuthRequest, nil), + ), + eventFromEventPusherWithCreationDateNow( + oidcsession.NewRefreshTokenAddedEvent(context.Background(), &oidcsession.NewAggregate("V2_oidcSessionID", "org1").Aggregate, + "rt_refreshTokenID", 7*24*time.Hour, 24*time.Hour), + ), + ), + expectFilter( + user.NewHumanAddedEvent( + context.Background(), + &user.NewAggregate("userID", "org1").Aggregate, + "username", + "firstname", + "lastname", + "nickname", + "displayname", + language.Afrikaans, + domain.GenderUnspecified, + "email", + false, + ), + ), + expectFilter(), + ), + idGenerator: mock.NewIDGeneratorExpectIDs(t), + keyAlgorithm: crypto.CreateMockEncryptionAlg(gomock.NewController(t)), + }, + args{ + ctx: authz.WithInstanceID(context.Background(), "instanceID"), + refreshToken: "V2_oidcSessionID-rt_refreshTokenID:userID", //V2_oidcSessionID:rt_refreshTokenID:userID + scope: []string{"openid", "offline_access"}, + reqClientID: "differentClientID", + complianceCheck: mockRefreshTokenComplianceChecker(nil), + }, + res{ + err: zerrors.ThrowInvalidArgument(nil, "test", "invalid clientID"), + }, + }, { "refresh successful", fields{ @@ -1885,6 +1941,7 @@ func TestCommands_ExchangeOIDCSessionRefreshAndAccessToken(t *testing.T) { ctx: authz.WithInstanceID(context.Background(), "instanceID"), refreshToken: "V2_oidcSessionID-rt_refreshTokenID:userID", //V2_oidcSessionID:rt_refreshTokenID:userID scope: []string{"openid", "offline_access"}, + reqClientID: "clientID", complianceCheck: mockRefreshTokenComplianceChecker(nil), }, res{ @@ -1918,7 +1975,7 @@ func TestCommands_ExchangeOIDCSessionRefreshAndAccessToken(t *testing.T) { keyAlgorithm: tt.fields.keyAlgorithm, authAlgorithm: &mockAuthCrypto{}, } - got, err := c.ExchangeOIDCSessionRefreshAndAccessToken(tt.args.ctx, tt.args.refreshToken, tt.args.scope, tt.args.complianceCheck) + got, err := c.ExchangeOIDCSessionRefreshAndAccessToken(tt.args.ctx, tt.args.refreshToken, tt.args.scope, tt.args.reqClientID, tt.args.complianceCheck) require.ErrorIs(t, err, tt.res.err) if got != nil { assert.WithinRange(t, got.AuthTime, tt.res.session.AuthTime.Add(-time.Second), tt.res.session.AuthTime.Add(time.Second))