diff --git a/cmd/admin-handlers-users.go b/cmd/admin-handlers-users.go index 3a67c6688..7be574e56 100644 --- a/cmd/admin-handlers-users.go +++ b/cmd/admin-handlers-users.go @@ -903,6 +903,9 @@ func (a adminAPIHandlers) UpdateServiceAccount(w http.ResponseWriter, r *http.Re description: updateReq.NewDescription, expiration: updateReq.NewExpiration, sessionPolicy: sp, + // The edit applies to the stored account only while it still has + // the parent the checks above authorized. + parentUser: svcAccount.ParentUser, } updatedAt, err := globalIAMSys.UpdateServiceAccount(ctx, accessKey, opts) if err != nil { diff --git a/cmd/iam-revision.go b/cmd/iam-revision.go index 6f7980705..3df5baed0 100644 --- a/cmd/iam-revision.go +++ b/cmd/iam-revision.go @@ -16,7 +16,6 @@ import ( "time" "github.com/minio/madmin-go/v3" - "github.com/minio/minio-go/v7/pkg/set" "github.com/minio/minio/internal/auth" etcd "go.etcd.io/etcd/client/v3" "go.etcd.io/etcd/client/v3/concurrency" @@ -46,22 +45,26 @@ type iamGroupRevocation struct { // its revision so an older non-expiring credential cannot return. type iamExpireIdentity struct{} -// An LDAP group refresh sets the groups of the credential as it is stored -// now. The refresh computes the groups from a snapshot of a server's cache -// taken before a slow directory lookup; writing that snapshot back would -// undo a rotation or an edit made meanwhile on any server, and bring back a -// deleted credential. The refresh therefore changes only the groups, under -// the identity's revision lock and on the stored record, and leaves alone a -// credential that is gone, expired, or now has another parent, whose groups -// the lookup did not compute. -type iamGroupRefresh struct { +// An edit of an identity applies to the identity as it is stored now, under +// its revision lock, not to a server's cached copy. A copy written back +// whole would undo a rotation or an edit made meanwhile through this or +// another server, whose notification this one may have missed, and bring +// back an identity deleted meanwhile. edit receives the stored record and +// changes it in place; it returns errIAMIdentityUnchanged to write nothing, +// or any other error to refuse. An identity that is absent or deleted is +// refused with errIAMIdentityGone. +type iamIdentityEdit struct { UserIdentity // the record written, or found unchanged accessKey string - parentUser string - groups []string - unchanged bool // the stored record already has the groups + edit func(u *UserIdentity) error + unchanged bool } +var ( + errIAMIdentityGone = errors.New("IAM identity is absent or deleted in storage") + errIAMIdentityUnchanged = errors.New("IAM identity edit changes nothing") +) + // The authoritative revocation is durable even if dependent cleanup fails. // Callers must publish it to sibling caches before returning the error. type iamCommittedCleanupError struct { @@ -269,27 +272,45 @@ func saveIAMRevision(ctx context.Context, store IAMStorageAPI, path string, item if err != nil { return err } - if op, ok := item.(*iamGroupRefresh); ok { + if op, ok := item.(*iamIdentityEdit); ok { var current UserIdentity err := store.loadIAMConfig(ctx, ¤t, path) if errors.Is(err, errConfigNotFound) { - return errIAMStaleUpdate + return errIAMIdentityGone } if err != nil { return err } + if current.Deleted { + if origin, replicated := iamReplicationTime(ctx); replicated && !origin.After(previous.timestamp()) { + // A peer's edit from before this deletion: acknowledged. + return errIAMStaleUpdate + } + return errIAMIdentityGone + } + // Normalize the record as the identity loaders do: the path names the + // access key, the claims are the token's, and a deprecated comment + // stands in for an empty description. if current.Credentials.AccessKey == "" { - // As the identity loaders do: the path names the access key. current.Credentials.AccessKey = op.accessKey } - if current.Deleted || current.Credentials.ParentUser != op.parentUser || current.Credentials.IsExpired() { - return errIAMStaleUpdate + if current.Credentials.SessionToken != "" { + claims, err := extractJWTClaims(current) + if err != nil { + return err + } + current.Credentials.Claims = claims.Map() } - if set.CreateStringSet(current.Credentials.Groups...).Equals(set.CreateStringSet(op.groups...)) { - op.UserIdentity, op.unchanged = current, true - return nil + if current.Credentials.Description == "" { + current.Credentials.Description = current.Credentials.Comment + } + if err := op.edit(¤t); err != nil { + if errors.Is(err, errIAMIdentityUnchanged) { + op.UserIdentity, op.unchanged = current, true + return nil + } + return err } - current.Credentials.Groups = op.groups current.UpdatedAt = time.Time{} // a new revision, as for any local edit op.UserIdentity = current item = &op.UserIdentity diff --git a/cmd/iam-store.go b/cmd/iam-store.go index 77fd3dad4..05eb795f1 100644 --- a/cmd/iam-store.go +++ b/cmd/iam-store.go @@ -2778,37 +2778,26 @@ func (store *IAMStoreSys) SetUserStatus(ctx context.Context, accessKey string, s cache := store.lock() defer store.unlock() - ui, ok := cache.iamUsersMap[accessKey] - if !ok { - return updatedAt, errNoSuchUser - } - cred := ui.Credentials - - if cred.IsTemp() || cred.IsServiceAccount() { + if ui, ok := cache.iamUsersMap[accessKey]; ok && (ui.Credentials.IsTemp() || ui.Credentials.IsServiceAccount()) { return updatedAt, errIAMActionNotAllowed } - - uinfo := newUserIdentity(auth.Credentials{ - AccessKey: accessKey, - SecretKey: cred.SecretKey, - Status: func() string { - switch string(status) { - case string(madmin.AccountEnabled), string(auth.AccountOn): - return auth.AccountOn - } - return auth.AccountOff - }(), + u, _, err := store.editStoredIdentity(ctx, cache, accessKey, regUser, func(stored *UserIdentity) error { + if stored.Credentials.IsTemp() || stored.Credentials.IsServiceAccount() { + return errIAMActionNotAllowed + } + stored.Credentials.Status = auth.AccountOff + if status == madmin.AccountEnabled { + stored.Credentials.Status = auth.AccountOn + } + return nil }) - - if err := store.saveUserIdentity(ctx, accessKey, regUser, &uinfo); err != nil { + if errors.Is(err, errIAMIdentityGone) { + return updatedAt, errNoSuchUser + } + if err != nil { return updatedAt, err } - - if err := cache.updateUserWithClaims(accessKey, uinfo); err != nil { - return updatedAt, err - } - - return uinfo.UpdatedAt, nil + return u.UpdatedAt, nil } // AddServiceAccount - add a new service account @@ -2863,15 +2852,39 @@ func (store *IAMStoreSys) UpdateServiceAccount(ctx context.Context, accessKey st cache := store.lock() defer store.unlock() - ui, ok := cache.iamUsersMap[accessKey] - if !ok || !ui.Credentials.IsServiceAccount() { + u, _, err := store.editStoredIdentity(ctx, cache, accessKey, svcUser, func(stored *UserIdentity) error { + if !stored.Credentials.IsServiceAccount() { + return errNoSuchServiceAccount + } + if opts.parentUser != "" && stored.Credentials.ParentUser != opts.parentUser { + // Not the account the caller was authorized for: it was + // deleted and created again for another parent meanwhile. + return errNoSuchServiceAccount + } + cr, err := updatedServiceAccountCredentials(ctx, accessKey, stored.Credentials, opts) + if err != nil { + return err + } + stored.Credentials = cr + return nil + }) + if errors.Is(err, errIAMIdentityGone) { return updatedAt, errNoSuchServiceAccount } - cr := ui.Credentials + if err != nil { + return updatedAt, err + } + return u.UpdatedAt, nil +} + +// updatedServiceAccountCredentials returns cr, the stored credential of +// service account accessKey, with the edits of opts and a session token +// signed again for them. +func updatedServiceAccountCredentials(ctx context.Context, accessKey string, cr auth.Credentials, opts updateServiceAccountOpts) (auth.Credentials, error) { currentSecretKey := cr.SecretKey if opts.secretKey != "" { if !auth.IsSecretKeyValid(opts.secretKey) { - return updatedAt, auth.ErrInvalidSecretKeyLength + return cr, auth.ErrInvalidSecretKeyLength } cr.SecretKey = opts.secretKey } @@ -2887,7 +2900,7 @@ func (store *IAMStoreSys) UpdateServiceAccount(ctx context.Context, accessKey st if opts.expiration != nil { expirationInUTC := opts.expiration.UTC() if err := validateSvcExpirationInUTC(ctx, expirationInUTC); err != nil { - return updatedAt, err + return cr, err } cr.Expiration = expirationInUTC } @@ -2903,12 +2916,12 @@ func (store *IAMStoreSys) UpdateServiceAccount(ctx context.Context, accessKey st case auth.AccountOn, auth.AccountOff: cr.Status = opts.status default: - return updatedAt, errors.New("unknown account status value") + return cr, errors.New("unknown account status value") } m, err := getClaimsFromTokenWithSecret(cr.SessionToken, currentSecretKey) if err != nil { - return updatedAt, fmt.Errorf("unable to get svc acc claims: %v", err) + return cr, fmt.Errorf("unable to get svc acc claims: %v", err) } // Extracted session policy name string can be removed as its not useful @@ -2926,17 +2939,17 @@ func (store *IAMStoreSys) UpdateServiceAccount(ctx context.Context, accessKey st if opts.sessionPolicy != nil { // session policies is being updated if err := opts.sessionPolicy.Validate(); err != nil { - return updatedAt, err + return cr, err } if opts.sessionPolicy.Version != "" && len(opts.sessionPolicy.Statements) > 0 { policyBuf, err := json.Marshal(opts.sessionPolicy) if err != nil { - return updatedAt, err + return cr, err } if len(policyBuf) > maxSVCSessionPolicySize { - return updatedAt, errSessionPolicyTooLarge + return cr, errSessionPolicyTooLarge } // Overwrite session policy claims. @@ -2947,21 +2960,11 @@ func (store *IAMStoreSys) UpdateServiceAccount(ctx context.Context, accessKey st cr.SessionToken, err = auth.JWTSignWithAccessKey(accessKey, m.Map(), cr.SecretKey) if err != nil { - return updatedAt, err + return cr, err } - - u := newUserIdentity(cr) - if err := store.saveUserIdentity(ctx, u.Credentials.AccessKey, svcUser, &u); err != nil { - return updatedAt, err - } - - if err := cache.updateUserWithClaims(u.Credentials.AccessKey, u); err != nil { - delete(cache.iamUsersMap, accessKey) - cache.updatedAt = UTCNow() - return updatedAt, err - } - - return u.UpdatedAt, nil + // The stored claims are those of the new token; the save extracts them. + cr.Claims = nil + return cr, nil } // ListTempAccounts - lists only temporary accounts from the cache. @@ -3105,20 +3108,14 @@ func (store *IAMStoreSys) UpdateUserSecretKey(ctx context.Context, accessKey, se cache := store.lock() defer store.unlock() - cache.updatedAt = time.Now() - - ui, ok := cache.iamUsersMap[accessKey] - if !ok { + _, _, err := store.editStoredIdentity(ctx, cache, accessKey, regUser, func(stored *UserIdentity) error { + stored.Credentials.SecretKey = secretKey + return nil + }) + if errors.Is(err, errIAMIdentityGone) { return errNoSuchUser } - cred := ui.Credentials - cred.SecretKey = secretKey - u := newUserIdentity(cred) - if err := store.saveUserIdentity(ctx, accessKey, regUser, &u); err != nil { - return err - } - - return cache.updateUserWithClaims(accessKey, u) + return err } // GetSTSAndServiceAccounts - returns all STS and Service account credentials. @@ -3146,10 +3143,10 @@ func (store *IAMStoreSys) getSTSAndServiceAccounts(cache *iamCache) []auth.Crede // RefreshUserGroups sets the groups of the STS or service-account // credential that cred was read from to groups, on the record stored now -// and under its revision lock (see iamGroupRefresh). Nothing else of the -// record changes. A credential that is gone, expired or now has another -// parent is left alone in storage and dropped from the cache, which then -// loads what is stored on its next use. It reports whether it wrote. +// (see editStoredIdentity). Nothing else of the record changes. A +// credential that is gone, expired or now has another parent, whose groups +// the directory lookup did not compute, is left alone in storage and +// dropped from the cache. It reports whether it wrote. func (store *IAMStoreSys) RefreshUserGroups(ctx context.Context, cred auth.Credentials, groups []string) (bool, error) { cache := store.lock() defer store.unlock() @@ -3161,26 +3158,64 @@ func (store *IAMStoreSys) RefreshUserGroups(ctx context.Context, cred auth.Crede } userType = stsUser } - op := &iamGroupRefresh{accessKey: cred.AccessKey, parentUser: cred.ParentUser, groups: groups} - err := store.saveIAMRevision(ctx, getUserIdentityPath(cred.AccessKey, userType), op) - if errors.Is(err, errIAMStaleUpdate) { - // The cache still holds the stale record; storage says otherwise. - delete(cache.iamUsersMap, cred.AccessKey) - delete(cache.iamSTSAccountsMap, cred.AccessKey) - cache.updatedAt = time.Now() - if store.group != nil { - store.group.Forget(cred.AccessKey) + _, unchanged, err := store.editStoredIdentity(ctx, cache, cred.AccessKey, userType, func(stored *UserIdentity) error { + if stored.Credentials.ParentUser != cred.ParentUser || stored.Credentials.IsExpired() { + return errIAMIdentityGone } + if set.CreateStringSet(stored.Credentials.Groups...).Equals(set.CreateStringSet(groups...)) { + return errIAMIdentityUnchanged + } + stored.Credentials.Groups = groups + return nil + }) + if errors.Is(err, errIAMIdentityGone) { return false, nil } if err != nil { return false, err } - // The stored record is authoritative, also when it had the groups. - if err := cache.updateUserWithClaims(cred.AccessKey, op.UserIdentity); err != nil { - return false, err + return !unchanged, nil +} + +// editStoredIdentity applies edit to identity accessKey of userType as it +// is stored now, under its revision lock (see iamIdentityEdit), and caches +// the result. The caller holds the cache lock. When the identity is absent +// or deleted in storage, or edit returns errIAMIdentityGone because it is +// no longer the identity the edit addressed, the cached copy is dropped, so +// a lookup loads what is stored, and errIAMIdentityGone is returned. +func (store *IAMStoreSys) editStoredIdentity(ctx context.Context, cache *iamCache, accessKey string, userType IAMUserType, edit func(u *UserIdentity) error) (UserIdentity, bool, error) { + op := &iamIdentityEdit{accessKey: accessKey, edit: edit} + err := store.saveIAMRevision(ctx, getUserIdentityPath(accessKey, userType), op) + if errors.Is(err, errIAMIdentityGone) { + store.forgetCachedIdentity(cache, accessKey, userType) + } + if err != nil { + return UserIdentity{}, false, err + } + if err := cache.updateUserWithClaims(accessKey, op.UserIdentity); err != nil { + // Stored but not cached: a lookup loads it. + store.forgetCachedIdentity(cache, accessKey, userType) + return UserIdentity{}, false, err + } + return op.UserIdentity, op.unchanged, nil +} + +// forgetCachedIdentity drops identity accessKey of userType from the cache; +// a lookup then loads it from storage. Users and service accounts share a +// map, so an entry of the other kind under the same key is kept. +func (store *IAMStoreSys) forgetCachedIdentity(cache *iamCache, accessKey string, userType IAMUserType) { + switch userType { + case stsUser: + delete(cache.iamSTSAccountsMap, accessKey) + default: + if ui, ok := cache.iamUsersMap[accessKey]; ok && ui.Credentials.IsServiceAccount() == (userType == svcUser) { + delete(cache.iamUsersMap, accessKey) + } + } + cache.updatedAt = time.Now() + if store.group != nil { + store.group.Forget(accessKey) } - return !op.unchanged, nil } // LoadUser - attempts to load user info from storage and updates cache. diff --git a/cmd/iam-stored-identity-edit_test.go b/cmd/iam-stored-identity-edit_test.go new file mode 100644 index 000000000..6db73081a --- /dev/null +++ b/cmd/iam-stored-identity-edit_test.go @@ -0,0 +1,233 @@ +// Copyright (c) 2026 PGSTY +// SPDX-License-Identifier: AGPL-3.0-or-later + +package cmd + +import ( + "errors" + "strings" + "testing" + "time" + + "github.com/minio/madmin-go/v3" + "github.com/minio/minio/internal/auth" + "github.com/pgsty/silo-pkg/v3/policy" +) + +// An administrator edits an identity through a server whose cache missed +// a rotation or a deletion made through another. The edit applies to what +// is stored and must not write the stale copy back. + +func TestIAMStaleServiceAccountEdit(t *testing.T) { + t.Run("rotated", func(t *testing.T) { + f := newGroupRefreshFixture(t) + snapshot := f.service(t, "edit-rotated") + other := f.sibling(t) + _, err := f.sys.UpdateServiceAccount(f.ctx, snapshot.AccessKey, updateServiceAccountOpts{secretKey: "edit-rotated-secret"}) + mustIAM(t, err) + if cached, _ := other.GetUser(snapshot.AccessKey); cached.Credentials.SecretKey != snapshot.SecretKey { + t.Fatal("control: the stale server must not have heard of the rotation") + } + _, err = other.UpdateServiceAccount(f.ctx, snapshot.AccessKey, updateServiceAccountOpts{description: "edited on a stale server"}) + mustIAM(t, err) + for where, get := range map[string]func() (UserIdentity, bool){ + "storage": func() (UserIdentity, bool) { return f.stored(t, snapshot.AccessKey) }, + "stale server": func() (UserIdentity, bool) { return other.GetUser(snapshot.AccessKey) }, + } { + u, ok := get() + if !ok || u.Credentials.SecretKey != "edit-rotated-secret" || u.Credentials.Description != "edited on a stale server" { + t.Errorf("%s: present=%v rotated=%v description=%q: the edit must apply to the rotated credential", + where, ok, u.Credentials.SecretKey == "edit-rotated-secret", u.Credentials.Description) + } + } + }) + t.Run("deleted", func(t *testing.T) { + f := newGroupRefreshFixture(t) + snapshot := f.service(t, "edit-deleted") + other := f.sibling(t) + mustIAM(t, f.sys.DeleteServiceAccount(f.ctx, snapshot.AccessKey, false)) + _, err := other.UpdateServiceAccount(f.ctx, snapshot.AccessKey, updateServiceAccountOpts{description: "edited on a stale server"}) + if !errors.Is(err, errNoSuchServiceAccount) { + t.Errorf("an edit of a deleted account must fail with no such service account, got %v", err) + } + if _, ok := f.stored(t, snapshot.AccessKey); ok { + t.Error("an edit on a stale server brought a deleted account back") + } + if _, ok := other.GetUser(snapshot.AccessKey); ok { + t.Error("the stale server still caches the deleted account") + } + }) +} + +// A session policy edit stores the claims of the token it signs. +func TestIAMServiceAccountEditStoresNewClaims(t *testing.T) { + f := newGroupRefreshFixture(t) + snapshot := f.service(t, "edit-claims") + sp, err := policy.ParseConfig(strings.NewReader(`{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Action":["s3:GetObject"],"Resource":["arn:aws:s3:::edit/*"]}]}`)) + mustIAM(t, err) + _, err = f.sys.UpdateServiceAccount(f.ctx, snapshot.AccessKey, updateServiceAccountOpts{sessionPolicy: sp}) + mustIAM(t, err) + var raw UserIdentity + mustIAM(t, f.sys.store.loadIAMConfig(f.ctx, &raw, getUserIdentityPath(snapshot.AccessKey, svcUser))) + claims, err := getClaimsFromTokenWithSecret(raw.Credentials.SessionToken, raw.Credentials.SecretKey) + mustIAM(t, err) + if raw.Credentials.Claims[iamPolicyClaimNameSA()] != embeddedPolicyType || claims.MapClaims[iamPolicyClaimNameSA()] != embeddedPolicyType { + t.Errorf("stored claims %v, token claims %v: both must name the embedded policy", raw.Credentials.Claims[iamPolicyClaimNameSA()], claims.MapClaims[iamPolicyClaimNameSA()]) + } +} + +func TestIAMStaleUserEdit(t *testing.T) { + for _, tc := range []struct { + name string + change func(t *testing.T, f *groupRefreshFixture, user auth.Credentials) + edit func(s *IAMStoreSys, f *groupRefreshFixture, user auth.Credentials) error + check func(t *testing.T, u UserIdentity, ok bool, err error) + }{ + {"status-after-delete", func(t *testing.T, f *groupRefreshFixture, user auth.Credentials) { + mustIAM(t, f.sys.DeleteUser(f.ctx, user.AccessKey, false)) + }, func(s *IAMStoreSys, f *groupRefreshFixture, user auth.Credentials) error { + _, err := s.SetUserStatus(f.ctx, user.AccessKey, madmin.AccountEnabled) + return err + }, func(t *testing.T, u UserIdentity, ok bool, err error) { + if !errors.Is(err, errNoSuchUser) || ok { + t.Errorf("err=%v present=%v: a status change on a stale server must not bring a deleted user back", err, ok) + } + }}, + {"status-after-rotation", func(t *testing.T, f *groupRefreshFixture, user auth.Credentials) { + mustIAM(t, f.sys.SetUserSecretKey(f.ctx, user.AccessKey, "edit-user-rotated")) + }, func(s *IAMStoreSys, f *groupRefreshFixture, user auth.Credentials) error { + _, err := s.SetUserStatus(f.ctx, user.AccessKey, madmin.AccountDisabled) + return err + }, func(t *testing.T, u UserIdentity, ok bool, err error) { + if err != nil || !ok || u.Credentials.SecretKey != "edit-user-rotated" || u.Credentials.Status != auth.AccountOff { + t.Errorf("err=%v present=%v rotated=%v status=%q: the status change must apply to the rotated user", + err, ok, u.Credentials.SecretKey == "edit-user-rotated", u.Credentials.Status) + } + }}, + {"secret-after-delete", func(t *testing.T, f *groupRefreshFixture, user auth.Credentials) { + mustIAM(t, f.sys.DeleteUser(f.ctx, user.AccessKey, false)) + }, func(s *IAMStoreSys, f *groupRefreshFixture, user auth.Credentials) error { + return s.UpdateUserSecretKey(f.ctx, user.AccessKey, "edit-user-new-secret") + }, func(t *testing.T, u UserIdentity, ok bool, err error) { + if !errors.Is(err, errNoSuchUser) || ok { + t.Errorf("err=%v present=%v: a secret change on a stale server must not bring a deleted user back", err, ok) + } + }}, + {"secret-after-disable", func(t *testing.T, f *groupRefreshFixture, user auth.Credentials) { + _, err := f.sys.SetUserStatus(f.ctx, user.AccessKey, madmin.AccountDisabled) + mustIAM(t, err) + }, func(s *IAMStoreSys, f *groupRefreshFixture, user auth.Credentials) error { + return s.UpdateUserSecretKey(f.ctx, user.AccessKey, "edit-user-new-secret") + }, func(t *testing.T, u UserIdentity, ok bool, err error) { + if err != nil || !ok || u.Credentials.SecretKey != "edit-user-new-secret" || u.Credentials.Status != auth.AccountOff { + t.Errorf("err=%v present=%v status=%q: a secret change on a stale server must keep the user disabled", err, ok, u.Credentials.Status) + } + }}, + } { + t.Run(tc.name, func(t *testing.T) { + f := newGroupRefreshFixture(t) + user := iamAdminUser(f.ctx, t, f.sys, "edit-user", "refresh-parent") + other := f.sibling(t) + tc.change(t, f, user) + if cached, ok := other.GetUser(user.AccessKey); !ok || cached.Credentials.SecretKey != user.SecretKey { + t.Fatal("control: the stale server must not have heard of the change") + } + err := tc.edit(other, f, user) + u, ok := f.stored(t, user.AccessKey) + tc.check(t, u, ok, err) + }) + } +} + +// An edit that the caller was authorized for under one parent does not +// apply to an account stored under another parent: the account was deleted +// and created again for it meanwhile. +func TestIAMServiceAccountEditKeepsAuthorizedParent(t *testing.T) { + f := newGroupRefreshFixture(t) + snapshot := f.service(t, "edit-parent") + other := iamAdminUser(f.ctx, t, f.sys, "edit-other-parent", "refresh-parent") + mustIAM(t, f.sys.DeleteServiceAccount(f.ctx, snapshot.AccessKey, false)) + _, _, err := f.sys.NewServiceAccount(f.ctx, other.AccessKey, nil, newServiceAccountOpts{accessKey: snapshot.AccessKey, secretKey: "edit-other-secret"}) + mustIAM(t, err) + _, err = f.sys.store.UpdateServiceAccount(f.ctx, snapshot.AccessKey, updateServiceAccountOpts{secretKey: "edit-chosen-secret", parentUser: snapshot.ParentUser}) + if !errors.Is(err, errNoSuchServiceAccount) { + t.Errorf("an edit authorized for another parent must fail, got %v", err) + } + if u, ok := f.stored(t, snapshot.AccessKey); !ok || u.Credentials.SecretKey != "edit-other-secret" { + t.Errorf("present=%v: the account of the other parent must keep its secret", ok) + } + // Without a bound parent (replication), the edit applies. + _, err = f.sys.store.UpdateServiceAccount(f.ctx, snapshot.AccessKey, updateServiceAccountOpts{description: "replicated"}) + mustIAM(t, err) +} + +// A record stored without claims, or with only a deprecated comment, is +// edited as the loaders present it. +func TestIAMServiceAccountEditLegacyRecord(t *testing.T) { + f := newGroupRefreshFixture(t) + snapshot := f.service(t, "edit-legacy") + path := getUserIdentityPath(snapshot.AccessKey, svcUser) + var raw UserIdentity + mustIAM(t, f.sys.store.loadIAMConfig(f.ctx, &raw, path)) + raw.Credentials.Claims = nil + raw.Credentials.Description, raw.Credentials.Comment = "", "legacy comment" + mustIAM(t, f.sys.store.saveIAMConfig(f.ctx, &raw, path)) + _, err := f.sys.store.UpdateServiceAccount(f.ctx, snapshot.AccessKey, updateServiceAccountOpts{status: string(madmin.AccountDisabled)}) + mustIAM(t, err) + for where, get := range map[string]func() (UserIdentity, bool){ + "storage": func() (UserIdentity, bool) { return f.stored(t, snapshot.AccessKey) }, + "cache": func() (UserIdentity, bool) { return f.sys.store.GetUser(snapshot.AccessKey) }, + } { + u, ok := get() + if !ok || u.Credentials.Status != auth.AccountOff || u.Credentials.Description != "legacy comment" { + t.Errorf("%s: present=%v status=%q description=%q", where, ok, u.Credentials.Status, u.Credentials.Description) + } + } +} + +// A peer's edit from before a deletion this site applied is acknowledged as +// stale; one from after it is not found. +func TestIAMReplicatedEditAfterDeletion(t *testing.T) { + f := newGroupRefreshFixture(t) + snapshot := f.service(t, "edit-repl") + user := iamAdminUser(f.ctx, t, f.sys, "edit-repl-user", "refresh-parent") + before := UTCNow() + mustIAM(t, f.sys.DeleteServiceAccount(f.ctx, snapshot.AccessKey, false)) + mustIAM(t, f.sys.DeleteUser(f.ctx, user.AccessKey, false)) + after := UTCNow().Add(time.Second) + + _, err := f.sys.store.UpdateServiceAccount(withIAMReplicationTime(f.ctx, before), snapshot.AccessKey, updateServiceAccountOpts{description: "late"}) + if iamReplicationError(err) != nil { + t.Errorf("an older service-account edit must be acknowledged, got %v", err) + } + _, err = f.sys.store.SetUserStatus(withIAMReplicationTime(f.ctx, before), user.AccessKey, madmin.AccountDisabled) + if iamReplicationError(err) != nil { + t.Errorf("an older status edit must be acknowledged, got %v", err) + } + if _, err = f.sys.store.UpdateServiceAccount(withIAMReplicationTime(f.ctx, after), snapshot.AccessKey, updateServiceAccountOpts{description: "late"}); !errors.Is(err, errNoSuchServiceAccount) { + t.Errorf("a newer edit of a deleted account must not be acknowledged as stale, got %v", err) + } + if _, ok := f.stored(t, snapshot.AccessKey); ok { + t.Error("a replicated edit brought a deleted account back") + } +} + +// A status change addressed to a service account is refused as before, and +// the account stays cached. +func TestIAMUserStatusOnServiceAccount(t *testing.T) { + f := newGroupRefreshFixture(t) + snapshot := f.service(t, "edit-sa-status") + _, err := f.sys.store.SetUserStatus(f.ctx, snapshot.AccessKey, madmin.AccountDisabled) + if !errors.Is(err, errIAMActionNotAllowed) { + t.Errorf("got %v, want errIAMActionNotAllowed", err) + } + if _, ok := f.sys.store.GetUser(snapshot.AccessKey); !ok { + t.Error("a refused status change dropped the service account from the cache") + } + if err := f.sys.store.UpdateUserSecretKey(f.ctx, snapshot.AccessKey, "edit-not-a-user"); !errors.Is(err, errNoSuchUser) { + t.Errorf("a user secret change of a service account must find no user, got %v", err) + } + if u, ok := f.sys.store.GetUser(snapshot.AccessKey); !ok || u.Credentials.SecretKey != snapshot.SecretKey { + t.Error("a user secret change of a service account touched the account") + } +} diff --git a/cmd/iam.go b/cmd/iam.go index c1260cf7c..6f8ffad18 100644 --- a/cmd/iam.go +++ b/cmd/iam.go @@ -1211,6 +1211,9 @@ type updateServiceAccountOpts struct { status string name, description string expiration *time.Time + // parentUser, when set, is the parent the caller was authorized for; an + // account stored under another parent is refused as absent. + parentUser string } // UpdateServiceAccount - edit a service account