mirror of
https://github.com/andwati/zitadel.git
synced 2026-10-18 05:52:51 +00:00
* fix(security): prevent unauthenticated IDP-linking account takeover Two distinct defects allowed an attacker to bind their own external identity to a victim's account and, because external IDP links are globally unique (first claim wins), hijack the victim's future federated logins. LoginV2: resolveUserIdFromSession returned the session's user id after only a null check, trusting an "identify-only" session (login name submitted, no verified primary factor). An unauthenticated attacker who knew a victim's login name could drive explicit linking against that session. Gate it on isSessionValid so only a fully authenticated session can link. Backend: AddUserIDPLink skipped the user-write permission check when the caller targeted their own account (userID == ctx user). Because the external subject is caller-supplied and unverified, this let any authenticated user self-link an unproven identity. Make the permission check unconditional. The login/federation flow uses a service account (userID != ctx user) and already took the permission branch, so it is unaffected; verified self-service linking goes through the IDP-intent flow. Adds a LoginV2 regression test (identify-only session is rejected) and the first command-level test for AddUserIDPLink (self-link without permission is denied). Co-Authored-By: Claude Opus 4.8 <[email protected]> * test(user_idp_link): address review on AddUserIDPLink hardening - add success regression cases for AddUserIDPLink (self-link with permission, other-user link with permission) so the login-client and managed-admin paths are pinned and the assert.NoError branch is live - add an integration case: UserCTX self-linking on its own account is now denied - document the intentional no-permission-check invariant on BulkAddedUserIDPLinks (verified Login V1 flow only; not API-exposed) Co-Authored-By: Claude Opus 4.8 <[email protected]> Claude-Session: https://claude.ai/code/session_01RUhwzSqHhycHNAJEfMcrdd * fix(login): authorize IDP linking via enrollment guard, not isSessionValid Replace the isSessionValid gate in resolveUserIdFromSession with getEnrollmentAuthorizationError. isSessionValid requires a verified primary factor, which rejected the legitimate "link an IDP as the first authenticator" onboarding flow (a freshly verified user with no credentials yet). The enrollment guard authorizes when the session has a verified, non-expired primary factor OR the user has zero auth methods and passed the user-verification check (email/invite-code proof bound to the browser fingerprint) — closing the identify-only ATO while preserving first-authenticator linking. Replace the single mocked rejection test with four that run the real guard: identify-only + has credentials -> rejected; identify-only + no credentials + no verification check -> rejected; expired primary factor -> rejected; identify-only + no credentials + verification check -> allowed. Co-Authored-By: Claude Opus 4.8 <[email protected]> Claude-Session: https://claude.ai/code/session_01T24WwsRLbaC7aHxBmyznnd --------- Co-authored-by: Claude Opus 4.8 <[email protected]>
222 lines
9.2 KiB
Go
222 lines
9.2 KiB
Go
package command
|
|
|
|
import (
|
|
"context"
|
|
|
|
"github.com/zitadel/zitadel/internal/api/authz"
|
|
"github.com/zitadel/zitadel/internal/domain"
|
|
"github.com/zitadel/zitadel/internal/eventstore"
|
|
"github.com/zitadel/zitadel/internal/repository/user"
|
|
"github.com/zitadel/zitadel/internal/telemetry/tracing"
|
|
"github.com/zitadel/zitadel/internal/zerrors"
|
|
)
|
|
|
|
func (c *Commands) AddUserIDPLink(ctx context.Context, userID, resourceOwner string, link *AddLink) (_ *domain.ObjectDetails, err error) {
|
|
ctx, span := tracing.NewSpan(ctx)
|
|
defer func() { span.EndWithError(err) }()
|
|
|
|
if userID == "" {
|
|
return nil, zerrors.ThrowInvalidArgument(nil, "COMMAND-03j8f", "Errors.IDMissing")
|
|
}
|
|
|
|
existingUser, err := c.userWriteModelByID(ctx, userID, resourceOwner)
|
|
if err != nil {
|
|
return nil, err
|
|
}
|
|
if !isUserStateExists(existingUser.UserState) {
|
|
return nil, zerrors.ThrowPreconditionFailed(nil, "COMMAND-vzktar7b7f", "Errors.User.NotFound")
|
|
}
|
|
// Binding an external identity (idpID + externalUserID) to a user must always require a
|
|
// user-write permission, even when the caller targets their own account. The external
|
|
// subject is caller-supplied and unverified here, and external links are globally unique
|
|
// on a first-claim-wins basis, so a self-service link of an unproven subject lets an
|
|
// attacker claim a victim's external identity and hijack their future federated logins
|
|
// (account takeover). Verified self-service linking is performed through the IDP-intent
|
|
// flow, whose service account holds this permission.
|
|
if err := c.checkPermission(ctx, domain.PermissionUserWrite, existingUser.ResourceOwner, existingUser.AggregateID); err != nil {
|
|
return nil, err
|
|
}
|
|
//nolint:staticcheck
|
|
event, err := addLink(ctx, c.eventstore.Filter, user.NewAggregate(existingUser.AggregateID, existingUser.ResourceOwner), link)
|
|
if err != nil {
|
|
return nil, err
|
|
}
|
|
|
|
events, err := c.eventstore.Push(ctx, event)
|
|
if err != nil {
|
|
return nil, err
|
|
}
|
|
return &domain.ObjectDetails{
|
|
Sequence: events[len(events)-1].Sequence(),
|
|
EventDate: events[len(events)-1].CreatedAt(),
|
|
ResourceOwner: events[len(events)-1].Aggregate().ResourceOwner,
|
|
}, nil
|
|
}
|
|
|
|
// BulkAddedUserIDPLinks intentionally performs no user-write permission check.
|
|
// It is reachable only from the Login V1 auth-request flow, and only after the
|
|
// external authentication has already been verified, so the external subjects it
|
|
// binds are proven rather than caller-supplied. No API surface exposes this
|
|
// command. Do NOT wire it to an API endpoint as-is: without a permission check
|
|
// (or verified IDP-intent proof, as AddUserIDPLink requires) it would reintroduce
|
|
// the account-takeover via unproven external-identity pre-claim.
|
|
func (c *Commands) BulkAddedUserIDPLinks(ctx context.Context, userID, resourceOwner string, links []*domain.UserIDPLink) (err error) {
|
|
if userID == "" {
|
|
return zerrors.ThrowInvalidArgument(nil, "COMMAND-03j8f", "Errors.IDMissing")
|
|
}
|
|
if len(links) == 0 {
|
|
return zerrors.ThrowInvalidArgument(nil, "COMMAND-Ek9s", "Errors.User.ExternalIDP.MinimumExternalIDPNeeded")
|
|
}
|
|
|
|
if _, err := c.checkUserExists(ctx, userID, resourceOwner); err != nil {
|
|
return err
|
|
}
|
|
|
|
events := make([]eventstore.Command, len(links))
|
|
for i, link := range links {
|
|
linkWriteModel := NewUserIDPLinkWriteModel(userID, link.IDPConfigID, link.ExternalUserID, resourceOwner)
|
|
userAgg := UserAggregateFromWriteModel(&linkWriteModel.WriteModel)
|
|
|
|
events[i], err = c.addUserIDPLink(ctx, userAgg, link, true)
|
|
if err != nil {
|
|
return err
|
|
}
|
|
}
|
|
|
|
_, err = c.eventstore.Push(ctx, events...)
|
|
return err
|
|
}
|
|
|
|
func (c *Commands) addUserIDPLink(ctx context.Context, human *eventstore.Aggregate, link *domain.UserIDPLink, linkToExistingUser bool) (eventstore.Command, error) {
|
|
if link.AggregateID != "" && human.ID != link.AggregateID {
|
|
return nil, zerrors.ThrowInvalidArgument(nil, "COMMAND-33M0g", "Errors.IDMissing")
|
|
}
|
|
if !link.IsValid() {
|
|
return nil, zerrors.ThrowInvalidArgument(nil, "COMMAND-6m9Kd", "Errors.User.ExternalIDP.Invalid")
|
|
}
|
|
idpWriteModel, err := IDPProviderWriteModel(ctx, c.eventstore.Filter, link.IDPConfigID)
|
|
if err != nil {
|
|
return nil, zerrors.ThrowPreconditionFailed(err, "COMMAND-39nfs", "Errors.IDPConfig.NotExisting")
|
|
}
|
|
options := idpWriteModel.GetProviderOptions()
|
|
// IDP user will either be linked or created on a new user
|
|
// Therefore we need to either check if linking is allowed or creation:
|
|
if linkToExistingUser && !options.IsLinkingAllowed && options.AutoLinkingOption == domain.AutoLinkingOptionUnspecified {
|
|
return nil, zerrors.ThrowPreconditionFailed(err, "COMMAND-Sfee2", "Errors.ExternalIDP.LinkingNotAllowed")
|
|
}
|
|
if !linkToExistingUser && !options.IsCreationAllowed && !options.IsAutoCreation {
|
|
return nil, zerrors.ThrowPreconditionFailed(err, "COMMAND-SJI3g", "Errors.ExternalIDP.CreationNotAllowed")
|
|
}
|
|
return user.NewUserIDPLinkAddedEvent(ctx, human, link.IDPConfigID, link.DisplayName, link.ExternalUserID), nil
|
|
|
|
}
|
|
|
|
func (c *Commands) RemoveUserIDPLink(ctx context.Context, link *domain.UserIDPLink) (*domain.ObjectDetails, error) {
|
|
event, linkWriteModel, err := c.removeUserIDPLink(ctx, link, false)
|
|
if err != nil {
|
|
return nil, err
|
|
}
|
|
pushedEvents, err := c.eventstore.Push(ctx, event)
|
|
if err != nil {
|
|
return nil, err
|
|
}
|
|
err = AppendAndReduce(linkWriteModel, pushedEvents...)
|
|
if err != nil {
|
|
return nil, err
|
|
}
|
|
return writeModelToObjectDetails(&linkWriteModel.WriteModel), nil
|
|
}
|
|
|
|
func (c *Commands) removeUserIDPLink(ctx context.Context, link *domain.UserIDPLink, cascade bool) (eventstore.Command, *UserIDPLinkWriteModel, error) {
|
|
if !link.IsValid() || link.AggregateID == "" {
|
|
return nil, nil, zerrors.ThrowInvalidArgument(nil, "COMMAND-3M9ds", "Errors.IDMissing")
|
|
}
|
|
|
|
existingLink, err := c.userIDPLinkWriteModelByID(ctx, link.AggregateID, link.IDPConfigID, link.ExternalUserID, link.ResourceOwner)
|
|
if err != nil {
|
|
return nil, nil, err
|
|
}
|
|
if existingLink.State == domain.UserIDPLinkStateUnspecified || existingLink.State == domain.UserIDPLinkStateRemoved {
|
|
return nil, nil, zerrors.ThrowNotFound(nil, "COMMAND-1M9xR", "Errors.User.ExternalIDP.NotFound")
|
|
}
|
|
if existingLink.AggregateID != authz.GetCtxData(ctx).UserID {
|
|
if err := c.checkPermission(ctx, domain.PermissionUserWrite, existingLink.ResourceOwner, existingLink.AggregateID); err != nil {
|
|
return nil, nil, err
|
|
}
|
|
}
|
|
userAgg := UserAggregateFromWriteModel(&existingLink.WriteModel)
|
|
if cascade {
|
|
return user.NewUserIDPLinkCascadeRemovedEvent(ctx, userAgg, link.IDPConfigID, link.ExternalUserID), existingLink, nil
|
|
}
|
|
return user.NewUserIDPLinkRemovedEvent(ctx, userAgg, link.IDPConfigID, link.ExternalUserID), existingLink, nil
|
|
}
|
|
|
|
func (c *Commands) UserIDPLoginChecked(ctx context.Context, orgID, userID string, authRequest *domain.AuthRequest) (err error) {
|
|
if userID == "" {
|
|
return zerrors.ThrowInvalidArgument(nil, "COMMAND-5n8sM", "Errors.IDMissing")
|
|
}
|
|
|
|
existingHuman, err := c.getHumanWriteModelByID(ctx, userID, orgID)
|
|
if err != nil {
|
|
return err
|
|
}
|
|
if existingHuman.UserState == domain.UserStateUnspecified || existingHuman.UserState == domain.UserStateDeleted {
|
|
return zerrors.ThrowPreconditionFailed(nil, "COMMAND-dn88J", "Errors.User.NotFound")
|
|
}
|
|
|
|
userAgg := UserAggregateFromWriteModel(&existingHuman.WriteModel)
|
|
_, err = c.eventstore.Push(ctx, user.NewUserIDPCheckSucceededEvent(ctx, userAgg, authRequestDomainToAuthRequestInfo(authRequest)))
|
|
return err
|
|
}
|
|
|
|
func (c *Commands) MigrateUserIDP(ctx context.Context, userID, orgID, idpConfigID, previousID, newID string) (err error) {
|
|
if userID == "" {
|
|
return zerrors.ThrowInvalidArgument(nil, "COMMAND-Sn3l1", "Errors.IDMissing")
|
|
}
|
|
|
|
writeModel, err := c.userIDPLinkWriteModelByID(ctx, userID, idpConfigID, previousID, orgID)
|
|
if err != nil {
|
|
return err
|
|
}
|
|
if writeModel.State != domain.UserIDPLinkStateActive {
|
|
return zerrors.ThrowPreconditionFailed(nil, "COMMAND-KJH2o", "Errors.User.ExternalIDP.NotFound")
|
|
}
|
|
|
|
userAgg := UserAggregateFromWriteModel(&writeModel.WriteModel)
|
|
_, err = c.eventstore.Push(ctx, user.NewUserIDPExternalIDMigratedEvent(ctx, userAgg, idpConfigID, previousID, newID))
|
|
return err
|
|
}
|
|
|
|
func (c *Commands) UpdateUserIDPLinkUsername(ctx context.Context, userID, orgID, idpConfigID, externalID, newUsername string) (err error) {
|
|
if userID == "" {
|
|
return zerrors.ThrowInvalidArgument(nil, "COMMAND-SFegz", "Errors.IDMissing")
|
|
}
|
|
|
|
writeModel, err := c.userIDPLinkWriteModelByID(ctx, userID, idpConfigID, externalID, orgID)
|
|
if err != nil {
|
|
return err
|
|
}
|
|
if writeModel.State != domain.UserIDPLinkStateActive {
|
|
return zerrors.ThrowPreconditionFailed(nil, "COMMAND-DGhre", "Errors.User.ExternalIDP.NotFound")
|
|
}
|
|
if writeModel.DisplayName == newUsername {
|
|
return nil
|
|
}
|
|
|
|
userAgg := UserAggregateFromWriteModel(&writeModel.WriteModel) //nolint:contextcheck
|
|
_, err = c.eventstore.Push(ctx, user.NewUserIDPExternalUsernameEvent(ctx, userAgg, idpConfigID, externalID, newUsername))
|
|
return err
|
|
}
|
|
|
|
func (c *Commands) userIDPLinkWriteModelByID(ctx context.Context, userID, idpConfigID, externalUserID, resourceOwner string) (writeModel *UserIDPLinkWriteModel, err error) {
|
|
ctx, span := tracing.NewSpan(ctx)
|
|
defer func() { span.EndWithError(err) }()
|
|
|
|
writeModel = NewUserIDPLinkWriteModel(userID, idpConfigID, externalUserID, resourceOwner)
|
|
err = c.eventstore.FilterToQueryReducer(ctx, writeModel)
|
|
if err != nil {
|
|
return nil, err
|
|
}
|
|
return writeModel, nil
|
|
}
|