mirror of
https://github.com/andwati/zitadel.git
synced 2026-10-18 05:52:51 +00:00
Merge commit from fork
* 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]>
This commit is contained in:
co-authored by
Claude Opus 4.8
parent
78be618357
commit
dbbab75420
@@ -61,6 +61,27 @@ func TestServer_AddIDPLink(t *testing.T) {
|
||||
want: nil,
|
||||
wantErr: true,
|
||||
},
|
||||
{
|
||||
// Security regression (account takeover): a self-link, an unprivileged
|
||||
// user binding an external identity to their own account, must now be
|
||||
// denied. The previous self-link bypass skipped the user-write permission
|
||||
// check, letting any authenticated user pre-claim an unproven external
|
||||
// subject on themselves and hijack a victim's future federated logins.
|
||||
name: "self-link without permission, error",
|
||||
args: args{
|
||||
UserCTX,
|
||||
&user.AddIDPLinkRequest{
|
||||
UserId: Instance.Users.Get(integration.UserTypeNoPermission).ID,
|
||||
IdpLink: &user.IDPLink{
|
||||
IdpId: idpResp.Id,
|
||||
UserId: "external-selflink",
|
||||
UserName: "username",
|
||||
},
|
||||
},
|
||||
},
|
||||
want: nil,
|
||||
wantErr: true,
|
||||
},
|
||||
{
|
||||
name: "add link",
|
||||
args: args{
|
||||
|
||||
@@ -26,10 +26,15 @@ func (c *Commands) AddUserIDPLink(ctx context.Context, userID, resourceOwner str
|
||||
if !isUserStateExists(existingUser.UserState) {
|
||||
return nil, zerrors.ThrowPreconditionFailed(nil, "COMMAND-vzktar7b7f", "Errors.User.NotFound")
|
||||
}
|
||||
if userID != authz.GetCtxData(ctx).UserID {
|
||||
if err := c.checkPermission(ctx, domain.PermissionUserWrite, existingUser.ResourceOwner, existingUser.AggregateID); err != nil {
|
||||
return nil, err
|
||||
}
|
||||
// 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)
|
||||
@@ -48,6 +53,13 @@ func (c *Commands) AddUserIDPLink(ctx context.Context, userID, resourceOwner str
|
||||
}, 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")
|
||||
|
||||
@@ -8,6 +8,7 @@ import (
|
||||
"github.com/stretchr/testify/assert"
|
||||
"golang.org/x/text/language"
|
||||
|
||||
"github.com/zitadel/zitadel/internal/api/authz"
|
||||
"github.com/zitadel/zitadel/internal/domain"
|
||||
"github.com/zitadel/zitadel/internal/eventstore"
|
||||
"github.com/zitadel/zitadel/internal/eventstore/v1/models"
|
||||
@@ -733,6 +734,249 @@ func TestCommandSide_BulkAddUserIDPLinks(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
func TestCommandSide_AddUserIDPLink(t *testing.T) {
|
||||
type fields struct {
|
||||
eventstore func(*testing.T) *eventstore.Eventstore
|
||||
checkPermission domain.PermissionCheck
|
||||
}
|
||||
type args struct {
|
||||
ctx context.Context
|
||||
userID string
|
||||
resourceOwner string
|
||||
link *AddLink
|
||||
}
|
||||
type res struct {
|
||||
err func(error) bool
|
||||
}
|
||||
tests := []struct {
|
||||
name string
|
||||
fields fields
|
||||
args args
|
||||
res res
|
||||
}{
|
||||
{
|
||||
name: "missing userid, invalid argument error",
|
||||
fields: fields{
|
||||
eventstore: expectEventstore(),
|
||||
checkPermission: newMockPermissionCheckAllowed(),
|
||||
},
|
||||
args: args{
|
||||
ctx: context.Background(),
|
||||
userID: "",
|
||||
link: &AddLink{
|
||||
IDPID: "config1",
|
||||
IDPExternalID: "externaluser1",
|
||||
},
|
||||
},
|
||||
res: res{
|
||||
err: zerrors.IsErrorInvalidArgument,
|
||||
},
|
||||
},
|
||||
{
|
||||
name: "user not existing, precondition failed error",
|
||||
fields: fields{
|
||||
eventstore: expectEventstore(expectFilter()),
|
||||
checkPermission: newMockPermissionCheckAllowed(),
|
||||
},
|
||||
args: args{
|
||||
ctx: context.Background(),
|
||||
userID: "user1",
|
||||
resourceOwner: "org1",
|
||||
link: &AddLink{
|
||||
IDPID: "config1",
|
||||
IDPExternalID: "externaluser1",
|
||||
},
|
||||
},
|
||||
res: res{
|
||||
err: zerrors.IsPreconditionFailed,
|
||||
},
|
||||
},
|
||||
{
|
||||
// Security regression (account takeover): binding an external identity to a user
|
||||
// must require an explicit user-write permission even when the caller targets
|
||||
// their own account (self-link). The previous self-link bypass let any
|
||||
// authenticated user claim an unproven external subject on themselves and, because
|
||||
// external links are globally unique, hijack a victim's future federated logins.
|
||||
name: "self-link without permission, permission denied error",
|
||||
fields: fields{
|
||||
eventstore: expectEventstore(
|
||||
expectFilter(
|
||||
eventFromEventPusher(
|
||||
user.NewHumanAddedEvent(
|
||||
context.Background(),
|
||||
&user.NewAggregate("user1", "org1").Aggregate,
|
||||
"userName",
|
||||
"firstName",
|
||||
"lastName",
|
||||
"nickName",
|
||||
"displayName",
|
||||
language.German,
|
||||
domain.GenderFemale,
|
||||
"[email protected]",
|
||||
false,
|
||||
),
|
||||
),
|
||||
),
|
||||
),
|
||||
checkPermission: newMockPermissionCheckNotAllowed(),
|
||||
},
|
||||
args: args{
|
||||
// ctx acts as the target user itself (self-link).
|
||||
ctx: authz.SetCtxData(context.Background(), authz.CtxData{UserID: "user1"}),
|
||||
userID: "user1",
|
||||
resourceOwner: "org1",
|
||||
link: &AddLink{
|
||||
IDPID: "config1",
|
||||
DisplayName: "name",
|
||||
IDPExternalID: "externaluser1",
|
||||
},
|
||||
},
|
||||
res: res{
|
||||
err: zerrors.IsPermissionDenied,
|
||||
},
|
||||
},
|
||||
{
|
||||
// Self-link is allowed when the caller holds the user-write permission.
|
||||
// This pins the login-client flow (service user with user.write links a
|
||||
// verified external subject to its own IDP-intent session) so a future
|
||||
// refactor cannot silently break it.
|
||||
name: "self-link with permission, ok",
|
||||
fields: fields{
|
||||
eventstore: expectEventstore(
|
||||
expectFilter(
|
||||
eventFromEventPusher(
|
||||
user.NewHumanAddedEvent(
|
||||
context.Background(),
|
||||
&user.NewAggregate("user1", "org1").Aggregate,
|
||||
"userName",
|
||||
"firstName",
|
||||
"lastName",
|
||||
"nickName",
|
||||
"displayName",
|
||||
language.German,
|
||||
domain.GenderFemale,
|
||||
"[email protected]",
|
||||
false,
|
||||
),
|
||||
),
|
||||
),
|
||||
expectFilter(
|
||||
eventFromEventPusher(
|
||||
org.NewIDPConfigAddedEvent(
|
||||
context.Background(),
|
||||
&org.NewAggregate("org1").Aggregate,
|
||||
"config1",
|
||||
"name",
|
||||
domain.IDPConfigTypeOIDC,
|
||||
domain.IDPConfigStylingTypeUnspecified,
|
||||
false,
|
||||
),
|
||||
),
|
||||
),
|
||||
expectPush(
|
||||
user.NewUserIDPLinkAddedEvent(
|
||||
authz.SetCtxData(context.Background(), authz.CtxData{UserID: "user1"}),
|
||||
&user.NewAggregate("user1", "org1").Aggregate,
|
||||
"config1",
|
||||
"name",
|
||||
"externaluser1",
|
||||
),
|
||||
),
|
||||
),
|
||||
checkPermission: newMockPermissionCheckAllowed(),
|
||||
},
|
||||
args: args{
|
||||
// ctx acts as the target user itself (self-link).
|
||||
ctx: authz.SetCtxData(context.Background(), authz.CtxData{UserID: "user1"}),
|
||||
userID: "user1",
|
||||
resourceOwner: "org1",
|
||||
link: &AddLink{
|
||||
IDPID: "config1",
|
||||
DisplayName: "name",
|
||||
IDPExternalID: "externaluser1",
|
||||
},
|
||||
},
|
||||
res: res{},
|
||||
},
|
||||
{
|
||||
// Linking another user's account with the user-write permission is the
|
||||
// ordinary managed path and must keep working.
|
||||
name: "other-user link with permission, ok",
|
||||
fields: fields{
|
||||
eventstore: expectEventstore(
|
||||
expectFilter(
|
||||
eventFromEventPusher(
|
||||
user.NewHumanAddedEvent(
|
||||
context.Background(),
|
||||
&user.NewAggregate("user1", "org1").Aggregate,
|
||||
"userName",
|
||||
"firstName",
|
||||
"lastName",
|
||||
"nickName",
|
||||
"displayName",
|
||||
language.German,
|
||||
domain.GenderFemale,
|
||||
"[email protected]",
|
||||
false,
|
||||
),
|
||||
),
|
||||
),
|
||||
expectFilter(
|
||||
eventFromEventPusher(
|
||||
org.NewIDPConfigAddedEvent(
|
||||
context.Background(),
|
||||
&org.NewAggregate("org1").Aggregate,
|
||||
"config1",
|
||||
"name",
|
||||
domain.IDPConfigTypeOIDC,
|
||||
domain.IDPConfigStylingTypeUnspecified,
|
||||
false,
|
||||
),
|
||||
),
|
||||
),
|
||||
expectPush(
|
||||
user.NewUserIDPLinkAddedEvent(
|
||||
authz.SetCtxData(context.Background(), authz.CtxData{UserID: "admin1"}),
|
||||
&user.NewAggregate("user1", "org1").Aggregate,
|
||||
"config1",
|
||||
"name",
|
||||
"externaluser1",
|
||||
),
|
||||
),
|
||||
),
|
||||
checkPermission: newMockPermissionCheckAllowed(),
|
||||
},
|
||||
args: args{
|
||||
// ctx is a different user (e.g. an admin) than the link target.
|
||||
ctx: authz.SetCtxData(context.Background(), authz.CtxData{UserID: "admin1"}),
|
||||
userID: "user1",
|
||||
resourceOwner: "org1",
|
||||
link: &AddLink{
|
||||
IDPID: "config1",
|
||||
DisplayName: "name",
|
||||
IDPExternalID: "externaluser1",
|
||||
},
|
||||
},
|
||||
res: res{},
|
||||
},
|
||||
}
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
r := &Commands{
|
||||
eventstore: tt.fields.eventstore(t),
|
||||
checkPermission: tt.fields.checkPermission,
|
||||
}
|
||||
_, err := r.AddUserIDPLink(tt.args.ctx, tt.args.userID, tt.args.resourceOwner, tt.args.link)
|
||||
if tt.res.err == nil {
|
||||
assert.NoError(t, err)
|
||||
}
|
||||
if tt.res.err != nil && !tt.res.err(err) {
|
||||
t.Errorf("got wrong err: %v ", err)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func TestCommandSide_RemoveUserIDPLink(t *testing.T) {
|
||||
type fields struct {
|
||||
eventstore func(*testing.T) *eventstore.Eventstore
|
||||
|
||||
Reference in New Issue
Block a user