From 24e5d466be68dffdbcc55fda0058cb2866e354fc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tim=20M=C3=B6hlmann?= Date: Wed, 28 Aug 2024 20:19:50 +0200 Subject: [PATCH] fix(oidc): don't push introspection client events (#8481) # Which Problems Are Solved Do not push secret succeeded and failed events for API and OIDC clients on the introspection endpoint. On instances where introspection was fequently called, the pushed events created issues on duplicate primary keys, due to collisions on the `sequence` column in the eventstore. As the event pusher retries on this collision and we pushed above mentioned events async, it would create a backpressure of concurrent pushers and effectively cripple an instance. We considered that pushing these events have little value with regards to the audit trail, as we do not push similar events when client assertion is used. Also, before #7657 the events were defined, but not pushed. # How the Problems Are Solved - Removed API secret check succeeded and faild event definitions - Removed OIDC secret check succeeded and faild event definitions - Push only Hash Updated event when needed # Additional Changes - None # Additional Context - Fixes https://github.com/zitadel/zitadel/issues/8479 - Closes https://github.com/zitadel/zitadel/issues/8430 - Intoduced in https://github.com/zitadel/zitadel/pull/7657 (cherry picked from commit 90b908c36123981f058314fee58dd9bf9b0d7925) --- internal/api/oidc/client.go | 5 +- internal/api/oidc/introspect.go | 17 ++-- internal/command/project_application_api.go | 35 ++----- .../command/project_application_api_test.go | 6 -- internal/command/project_application_oidc.go | 35 ++----- .../command/project_application_oidc_test.go | 6 -- internal/repository/project/api_config.go | 94 +------------------ internal/repository/project/eventstore.go | 2 - internal/repository/project/oidc_config.go | 94 +------------------ 9 files changed, 38 insertions(+), 256 deletions(-) diff --git a/internal/api/oidc/client.go b/internal/api/oidc/client.go index 31b33049e1..f9b6b04d3c 100644 --- a/internal/api/oidc/client.go +++ b/internal/api/oidc/client.go @@ -1037,10 +1037,11 @@ func (s *Server) verifyClientSecret(ctx context.Context, client *query.OIDCClien updated, err := s.hasher.Verify(client.HashedSecret, secret) spanPasswordComparison.EndWithError(err) if err != nil { - s.command.OIDCSecretCheckFailed(ctx, client.AppID, client.ProjectID, client.Settings.ResourceOwner) return oidc.ErrInvalidClient().WithParent(err).WithDescription("invalid secret") } - s.command.OIDCSecretCheckSucceeded(ctx, client.AppID, client.ProjectID, client.Settings.ResourceOwner, updated) + if updated != "" { + s.command.OIDCUpdateSecret(ctx, client.AppID, client.ProjectID, client.Settings.ResourceOwner, updated) + } return nil } diff --git a/internal/api/oidc/introspect.go b/internal/api/oidc/introspect.go index 1d53b78110..d21af96414 100644 --- a/internal/api/oidc/introspect.go +++ b/internal/api/oidc/introspect.go @@ -183,17 +183,13 @@ func (s *Server) introspectionClientAuth(ctx context.Context, cc *op.ClientCrede var errNoAppType = errors.New("introspection client without app type") func (s *Server) introspectionClientSecretAuth(ctx context.Context, client *query.IntrospectionClient, secret string) error { - var ( - successCommand func(ctx context.Context, appID, projectID, resourceOwner, updated string) - failedCommand func(ctx context.Context, appID, projectID, resourceOwner string) - ) + var updateCommand func(ctx context.Context, appID, projectID, resourceOwner, updated string) + switch client.AppType { case query.AppTypeAPI: - successCommand = s.command.APISecretCheckSucceeded - failedCommand = s.command.APISecretCheckFailed + updateCommand = s.command.APIUpdateSecret case query.AppTypeOIDC: - successCommand = s.command.OIDCSecretCheckSucceeded - failedCommand = s.command.OIDCSecretCheckFailed + updateCommand = s.command.OIDCUpdateSecret default: return zerrors.ThrowInternal(errNoAppType, "OIDC-ooD5Ot", "Errors.Internal") } @@ -202,10 +198,11 @@ func (s *Server) introspectionClientSecretAuth(ctx context.Context, client *quer updated, err := s.hasher.Verify(client.HashedSecret, secret) spanPasswordComparison.EndWithError(err) if err != nil { - failedCommand(ctx, client.AppID, client.ProjectID, client.ResourceOwner) return err } - successCommand(ctx, client.AppID, client.ProjectID, client.ResourceOwner, updated) + if updated != "" { + updateCommand(ctx, client.AppID, client.ProjectID, client.ResourceOwner, updated) + } return nil } diff --git a/internal/command/project_application_api.go b/internal/command/project_application_api.go index a697305ac9..c537070c36 100644 --- a/internal/command/project_application_api.go +++ b/internal/command/project_application_api.go @@ -250,22 +250,18 @@ func (c *Commands) VerifyAPIClientSecret(ctx context.Context, projectID, appID, ctx, spanPasswordComparison := tracing.NewNamedSpan(ctx, "passwap.Verify") updated, err := c.secretHasher.Verify(app.HashedSecret, secret) spanPasswordComparison.EndWithError(err) - if err == nil { - c.apiSecretCheckSucceeded(ctx, projectAgg, app.AppID, updated) - return err + if err != nil { + return zerrors.ThrowInvalidArgument(err, "COMMAND-SADfg", "Errors.Project.App.ClientSecretInvalid") } - c.apiSecretCheckFailed(ctx, projectAgg, app.AppID) - return zerrors.ThrowInvalidArgument(err, "COMMAND-SADfg", "Errors.Project.App.ClientSecretInvalid") + if updated != "" { + c.apiUpdateSecret(ctx, projectAgg, app.AppID, updated) + } + return nil } -func (c *Commands) APISecretCheckSucceeded(ctx context.Context, appID, projectID, resourceOwner, updated string) { +func (c *Commands) APIUpdateSecret(ctx context.Context, appID, projectID, resourceOwner, updated string) { agg := project_repo.NewAggregate(projectID, resourceOwner) - c.apiSecretCheckSucceeded(ctx, &agg.Aggregate, appID, updated) -} - -func (c *Commands) APISecretCheckFailed(ctx context.Context, appID, projectID, resourceOwner string) { - agg := project_repo.NewAggregate(projectID, resourceOwner) - c.apiSecretCheckFailed(ctx, &agg.Aggregate, appID) + c.apiUpdateSecret(ctx, &agg.Aggregate, appID, updated) } func (c *Commands) getAPIAppWriteModel(ctx context.Context, projectID, appID, resourceOwner string) (_ *APIApplicationWriteModel, err error) { @@ -280,17 +276,6 @@ func (c *Commands) getAPIAppWriteModel(ctx context.Context, projectID, appID, re return appWriteModel, nil } -func (c *Commands) apiSecretCheckSucceeded(ctx context.Context, agg *eventstore.Aggregate, appID, updated string) { - cmds := append( - make([]eventstore.Command, 0, 2), - project_repo.NewAPIConfigSecretCheckSucceededEvent(ctx, agg, appID), - ) - if updated != "" { - cmds = append(cmds, project_repo.NewAPIConfigSecretHashUpdatedEvent(ctx, agg, appID, updated)) - } - c.asyncPush(ctx, cmds...) -} - -func (c *Commands) apiSecretCheckFailed(ctx context.Context, agg *eventstore.Aggregate, appID string) { - c.asyncPush(ctx, project_repo.NewAPIConfigSecretCheckFailedEvent(ctx, agg, appID)) +func (c *Commands) apiUpdateSecret(ctx context.Context, agg *eventstore.Aggregate, appID, updated string) { + c.asyncPush(ctx, project_repo.NewAPIConfigSecretHashUpdatedEvent(ctx, agg, appID, updated)) } diff --git a/internal/command/project_application_api_test.go b/internal/command/project_application_api_test.go index 7f3d249bbc..7393580cc2 100644 --- a/internal/command/project_application_api_test.go +++ b/internal/command/project_application_api_test.go @@ -783,9 +783,6 @@ func TestCommands_VerifyAPIClientSecret(t *testing.T) { project.NewAPIConfigAddedEvent(context.Background(), &agg.Aggregate, "appID", "clientID", hashedSecret, domain.APIAuthMethodTypePrivateKeyJWT), ), ), - expectPush( - project.NewAPIConfigSecretCheckSucceededEvent(context.Background(), &agg.Aggregate, "appID"), - ), ), }, { @@ -800,9 +797,6 @@ func TestCommands_VerifyAPIClientSecret(t *testing.T) { project.NewAPIConfigAddedEvent(context.Background(), &agg.Aggregate, "appID", "clientID", hashedSecret, domain.APIAuthMethodTypePrivateKeyJWT), ), ), - expectPush( - project.NewAPIConfigSecretCheckFailedEvent(context.Background(), &agg.Aggregate, "appID"), - ), ), wantErr: zerrors.ThrowInvalidArgument(err, "COMMAND-SADfg", "Errors.Project.App.ClientSecretInvalid"), }, diff --git a/internal/command/project_application_oidc.go b/internal/command/project_application_oidc.go index 6c9df8a69a..50ddc6d07d 100644 --- a/internal/command/project_application_oidc.go +++ b/internal/command/project_application_oidc.go @@ -331,22 +331,18 @@ func (c *Commands) VerifyOIDCClientSecret(ctx context.Context, projectID, appID, ctx, spanPasswordComparison := tracing.NewNamedSpan(ctx, "passwap.Verify") updated, err := c.secretHasher.Verify(app.HashedSecret, secret) spanPasswordComparison.EndWithError(err) - if err == nil { - c.oidcSecretCheckSucceeded(ctx, projectAgg, appID, updated) - return nil + if err != nil { + return zerrors.ThrowInvalidArgument(err, "COMMAND-Bz542", "Errors.Project.App.ClientSecretInvalid") } - c.oidcSecretCheckFailed(ctx, projectAgg, appID) - return zerrors.ThrowInvalidArgument(err, "COMMAND-Bz542", "Errors.Project.App.ClientSecretInvalid") + if updated != "" { + c.oidcUpdateSecret(ctx, projectAgg, appID, updated) + } + return nil } -func (c *Commands) OIDCSecretCheckSucceeded(ctx context.Context, appID, projectID, resourceOwner, updated string) { +func (c *Commands) OIDCUpdateSecret(ctx context.Context, appID, projectID, resourceOwner, updated string) { agg := project_repo.NewAggregate(projectID, resourceOwner) - c.oidcSecretCheckSucceeded(ctx, &agg.Aggregate, appID, updated) -} - -func (c *Commands) OIDCSecretCheckFailed(ctx context.Context, appID, projectID, resourceOwner string) { - agg := project_repo.NewAggregate(projectID, resourceOwner) - c.oidcSecretCheckFailed(ctx, &agg.Aggregate, appID) + c.oidcUpdateSecret(ctx, &agg.Aggregate, appID, updated) } func (c *Commands) getOIDCAppWriteModel(ctx context.Context, projectID, appID, resourceOwner string) (_ *OIDCApplicationWriteModel, err error) { @@ -382,17 +378,6 @@ func trimStringSliceWhiteSpaces(slice []string) []string { return slice } -func (c *Commands) oidcSecretCheckSucceeded(ctx context.Context, agg *eventstore.Aggregate, appID, updated string) { - cmds := append( - make([]eventstore.Command, 0, 2), - project_repo.NewOIDCConfigSecretCheckSucceededEvent(ctx, agg, appID), - ) - if updated != "" { - cmds = append(cmds, project_repo.NewOIDCConfigSecretHashUpdatedEvent(ctx, agg, appID, updated)) - } - c.asyncPush(ctx, cmds...) -} - -func (c *Commands) oidcSecretCheckFailed(ctx context.Context, agg *eventstore.Aggregate, appID string) { - c.asyncPush(ctx, project_repo.NewOIDCConfigSecretCheckFailedEvent(ctx, agg, appID)) +func (c *Commands) oidcUpdateSecret(ctx context.Context, agg *eventstore.Aggregate, appID, updated string) { + c.asyncPush(ctx, project_repo.NewOIDCConfigSecretHashUpdatedEvent(ctx, agg, appID, updated)) } diff --git a/internal/command/project_application_oidc_test.go b/internal/command/project_application_oidc_test.go index 73cb837057..57d69828b1 100644 --- a/internal/command/project_application_oidc_test.go +++ b/internal/command/project_application_oidc_test.go @@ -1298,9 +1298,6 @@ func TestCommands_VerifyOIDCClientSecret(t *testing.T) { ), ), ), - expectPush( - project.NewOIDCConfigSecretCheckSucceededEvent(context.Background(), &agg.Aggregate, "appID"), - ), ), }, { @@ -1335,9 +1332,6 @@ func TestCommands_VerifyOIDCClientSecret(t *testing.T) { ), ), ), - expectPush( - project.NewOIDCConfigSecretCheckFailedEvent(context.Background(), &agg.Aggregate, "appID"), - ), ), wantErr: zerrors.ThrowInvalidArgument(err, "COMMAND-Bz542", "Errors.Project.App.ClientSecretInvalid"), }, diff --git a/internal/repository/project/api_config.go b/internal/repository/project/api_config.go index eac410847b..6316bce36f 100644 --- a/internal/repository/project/api_config.go +++ b/internal/repository/project/api_config.go @@ -10,12 +10,10 @@ import ( ) const ( - APIConfigAddedType = applicationEventTypePrefix + "config.api.added" - APIConfigChangedType = applicationEventTypePrefix + "config.api.changed" - APIConfigSecretChangedType = applicationEventTypePrefix + "config.api.secret.changed" - APIClientSecretCheckSucceededType = applicationEventTypePrefix + "api.secret.check.succeeded" - APIClientSecretCheckFailedType = applicationEventTypePrefix + "api.secret.check.failed" - APIConfigSecretHashUpdatedType = applicationEventTypePrefix + "config.api.secret.updated" + APIConfigAddedType = applicationEventTypePrefix + "config.api.added" + APIConfigChangedType = applicationEventTypePrefix + "config.api.changed" + APIConfigSecretChangedType = applicationEventTypePrefix + "config.api.secret.changed" + APIConfigSecretHashUpdatedType = applicationEventTypePrefix + "config.api.secret.updated" ) type APIConfigAddedEvent struct { @@ -202,90 +200,6 @@ func APIConfigSecretChangedEventMapper(event eventstore.Event) (eventstore.Event return e, nil } -type APIConfigSecretCheckSucceededEvent struct { - eventstore.BaseEvent `json:"-"` - - AppID string `json:"appId"` -} - -func (e *APIConfigSecretCheckSucceededEvent) Payload() interface{} { - return e -} - -func (e *APIConfigSecretCheckSucceededEvent) UniqueConstraints() []*eventstore.UniqueConstraint { - return nil -} - -func NewAPIConfigSecretCheckSucceededEvent( - ctx context.Context, - aggregate *eventstore.Aggregate, - appID string, -) *APIConfigSecretCheckSucceededEvent { - return &APIConfigSecretCheckSucceededEvent{ - BaseEvent: *eventstore.NewBaseEventForPush( - ctx, - aggregate, - APIClientSecretCheckSucceededType, - ), - AppID: appID, - } -} - -func APIConfigSecretCheckSucceededEventMapper(event eventstore.Event) (eventstore.Event, error) { - e := &APIConfigSecretCheckSucceededEvent{ - BaseEvent: *eventstore.BaseEventFromRepo(event), - } - - err := event.Unmarshal(e) - if err != nil { - return nil, zerrors.ThrowInternal(err, "API-837gV", "unable to unmarshal api config") - } - - return e, nil -} - -type APIConfigSecretCheckFailedEvent struct { - eventstore.BaseEvent `json:"-"` - - AppID string `json:"appId"` -} - -func (e *APIConfigSecretCheckFailedEvent) Payload() interface{} { - return e -} - -func (e *APIConfigSecretCheckFailedEvent) UniqueConstraints() []*eventstore.UniqueConstraint { - return nil -} - -func NewAPIConfigSecretCheckFailedEvent( - ctx context.Context, - aggregate *eventstore.Aggregate, - appID string, -) *APIConfigSecretCheckFailedEvent { - return &APIConfigSecretCheckFailedEvent{ - BaseEvent: *eventstore.NewBaseEventForPush( - ctx, - aggregate, - APIClientSecretCheckFailedType, - ), - AppID: appID, - } -} - -func APIConfigSecretCheckFailedEventMapper(event eventstore.Event) (eventstore.Event, error) { - e := &APIConfigSecretCheckFailedEvent{ - BaseEvent: *eventstore.BaseEventFromRepo(event), - } - - err := event.Unmarshal(e) - if err != nil { - return nil, zerrors.ThrowInternal(err, "API-987g%", "unable to unmarshal api config") - } - - return e, nil -} - type APIConfigSecretHashUpdatedEvent struct { *eventstore.BaseEvent `json:"-"` diff --git a/internal/repository/project/eventstore.go b/internal/repository/project/eventstore.go index fe8e14a508..5705649739 100644 --- a/internal/repository/project/eventstore.go +++ b/internal/repository/project/eventstore.go @@ -35,8 +35,6 @@ func init() { eventstore.RegisterFilterEventMapper(AggregateType, OIDCConfigAddedType, OIDCConfigAddedEventMapper) eventstore.RegisterFilterEventMapper(AggregateType, OIDCConfigChangedType, OIDCConfigChangedEventMapper) eventstore.RegisterFilterEventMapper(AggregateType, OIDCConfigSecretChangedType, OIDCConfigSecretChangedEventMapper) - eventstore.RegisterFilterEventMapper(AggregateType, OIDCClientSecretCheckSucceededType, OIDCConfigSecretCheckSucceededEventMapper) - eventstore.RegisterFilterEventMapper(AggregateType, OIDCClientSecretCheckFailedType, OIDCConfigSecretCheckFailedEventMapper) eventstore.RegisterFilterEventMapper(AggregateType, OIDCConfigSecretHashUpdatedType, eventstore.GenericEventMapper[OIDCConfigSecretHashUpdatedEvent]) eventstore.RegisterFilterEventMapper(AggregateType, APIConfigAddedType, APIConfigAddedEventMapper) eventstore.RegisterFilterEventMapper(AggregateType, APIConfigChangedType, APIConfigChangedEventMapper) diff --git a/internal/repository/project/oidc_config.go b/internal/repository/project/oidc_config.go index 68f2a59949..5ea20c220a 100644 --- a/internal/repository/project/oidc_config.go +++ b/internal/repository/project/oidc_config.go @@ -11,12 +11,10 @@ import ( ) const ( - OIDCConfigAddedType = applicationEventTypePrefix + "config.oidc.added" - OIDCConfigChangedType = applicationEventTypePrefix + "config.oidc.changed" - OIDCConfigSecretChangedType = applicationEventTypePrefix + "config.oidc.secret.changed" - OIDCClientSecretCheckSucceededType = applicationEventTypePrefix + "oidc.secret.check.succeeded" - OIDCClientSecretCheckFailedType = applicationEventTypePrefix + "oidc.secret.check.failed" - OIDCConfigSecretHashUpdatedType = applicationEventTypePrefix + "config.oidc.secret.updated" + OIDCConfigAddedType = applicationEventTypePrefix + "config.oidc.added" + OIDCConfigChangedType = applicationEventTypePrefix + "config.oidc.changed" + OIDCConfigSecretChangedType = applicationEventTypePrefix + "config.oidc.secret.changed" + OIDCConfigSecretHashUpdatedType = applicationEventTypePrefix + "config.oidc.secret.updated" ) type OIDCConfigAddedEvent struct { @@ -409,90 +407,6 @@ func OIDCConfigSecretChangedEventMapper(event eventstore.Event) (eventstore.Even return e, nil } -type OIDCConfigSecretCheckSucceededEvent struct { - eventstore.BaseEvent `json:"-"` - - AppID string `json:"appId"` -} - -func (e *OIDCConfigSecretCheckSucceededEvent) Payload() interface{} { - return e -} - -func (e *OIDCConfigSecretCheckSucceededEvent) UniqueConstraints() []*eventstore.UniqueConstraint { - return nil -} - -func NewOIDCConfigSecretCheckSucceededEvent( - ctx context.Context, - aggregate *eventstore.Aggregate, - appID string, -) *OIDCConfigSecretCheckSucceededEvent { - return &OIDCConfigSecretCheckSucceededEvent{ - BaseEvent: *eventstore.NewBaseEventForPush( - ctx, - aggregate, - OIDCClientSecretCheckSucceededType, - ), - AppID: appID, - } -} - -func OIDCConfigSecretCheckSucceededEventMapper(event eventstore.Event) (eventstore.Event, error) { - e := &OIDCConfigSecretCheckSucceededEvent{ - BaseEvent: *eventstore.BaseEventFromRepo(event), - } - - err := event.Unmarshal(e) - if err != nil { - return nil, zerrors.ThrowInternal(err, "OIDC-837gV", "unable to unmarshal oidc config") - } - - return e, nil -} - -type OIDCConfigSecretCheckFailedEvent struct { - eventstore.BaseEvent `json:"-"` - - AppID string `json:"appId"` -} - -func (e *OIDCConfigSecretCheckFailedEvent) Payload() interface{} { - return e -} - -func (e *OIDCConfigSecretCheckFailedEvent) UniqueConstraints() []*eventstore.UniqueConstraint { - return nil -} - -func NewOIDCConfigSecretCheckFailedEvent( - ctx context.Context, - aggregate *eventstore.Aggregate, - appID string, -) *OIDCConfigSecretCheckFailedEvent { - return &OIDCConfigSecretCheckFailedEvent{ - BaseEvent: *eventstore.NewBaseEventForPush( - ctx, - aggregate, - OIDCClientSecretCheckFailedType, - ), - AppID: appID, - } -} - -func OIDCConfigSecretCheckFailedEventMapper(event eventstore.Event) (eventstore.Event, error) { - e := &OIDCConfigSecretCheckFailedEvent{ - BaseEvent: *eventstore.BaseEventFromRepo(event), - } - - err := event.Unmarshal(e) - if err != nil { - return nil, zerrors.ThrowInternal(err, "OIDC-987g%", "unable to unmarshal oidc config") - } - - return e, nil -} - type OIDCConfigSecretHashUpdatedEvent struct { *eventstore.BaseEvent `json:"-"`