diff --git a/cmd/batch-submitter-edits_test.go b/cmd/batch-submitter-edits_test.go index 372f7536d..d319faf44 100644 --- a/cmd/batch-submitter-edits_test.go +++ b/cmd/batch-submitter-edits_test.go @@ -66,9 +66,11 @@ func TestBatchSubmitterCredentialEdits(t *testing.T) { if !ok { t.Fatal("service account missing") } - c := u.Credentials - c.Groups = append(c.Groups, "refreshed-group") - mustIAM(t, f.sys.store.UpdateUserIdentity(f.ctx, c)) + wrote, err := f.sys.store.RefreshUserGroups(f.ctx, u.Credentials, append(u.Credentials.Groups, "refreshed-group")) + mustIAM(t, err) + if !wrote { + t.Fatal("control: the refresh did not write") + } }, true}, {"secret-rotated", false, func(t *testing.T, cred auth.Credentials) { _, err := f.sys.UpdateServiceAccount(f.ctx, cred.AccessKey, updateServiceAccountOpts{secretKey: "edit-rotated-secret-key"}) diff --git a/cmd/iam-ldap-group-refresh_test.go b/cmd/iam-ldap-group-refresh_test.go new file mode 100644 index 000000000..d9455d29c --- /dev/null +++ b/cmd/iam-ldap-group-refresh_test.go @@ -0,0 +1,226 @@ +// Copyright (c) 2026 PGSTY +// SPDX-License-Identifier: AGPL-3.0-or-later + +package cmd + +import ( + "context" + "slices" + "testing" + + "github.com/minio/minio/internal/auth" +) + +// The LDAP group refresh computes groups from a snapshot of a server's cache +// taken before a slow directory lookup, then writes them. These tests take +// such a snapshot, change the credential the way an administrator would on +// this or another server, and run the write: it must change only the +// groups of what is stored now, and never undo the change. + +type groupRefreshFixture struct { + ctx context.Context + sys *IAMSys + obj ObjectLayer + parent auth.Credentials +} + +func newGroupRefreshFixture(t *testing.T) *groupRefreshFixture { + t.Helper() + ctx, sys, obj := prepareIAMRevisionFixture(t) + batchAuthzPolicy(ctx, t, sys, "refresh-parent", batchAuthzFullAccess) + parent := iamAdminUser(ctx, t, sys, "refresh-parent", "refresh-parent") + return &groupRefreshFixture{ctx: ctx, sys: sys, obj: obj, parent: parent} +} + +// service creates a service account of the fixture's parent and returns +// its stored credential, the snapshot a refresh would take. +func (f *groupRefreshFixture) service(t *testing.T, accessKey string) auth.Credentials { + t.Helper() + _, _, err := f.sys.NewServiceAccount(f.ctx, f.parent.AccessKey, nil, newServiceAccountOpts{accessKey: accessKey, secretKey: "refresh-original-secret"}) + mustIAM(t, err) + u, ok := f.sys.store.GetUser(accessKey) + if !ok { + t.Fatal("service account missing") + } + return u.Credentials +} + +// sibling is another server: the same storage, its own cache. +func (f *groupRefreshFixture) sibling(t *testing.T) *IAMStoreSys { + t.Helper() + s := &IAMStoreSys{IAMStorageAPI: newIAMObjectStore(f.obj, f.sys.usersSysType)} + mustIAM(t, s.LoadIAMCache(f.ctx, false)) + return s +} + +// stored is the record as a server that starts now loads it. +func (f *groupRefreshFixture) stored(t *testing.T, accessKey string) (UserIdentity, bool) { + t.Helper() + return f.sibling(t).GetUser(accessKey) +} + +func TestIAMGroupRefreshKeepsRotation(t *testing.T) { + f := newGroupRefreshFixture(t) + snapshot := f.service(t, "refresh-rotate") + _, err := f.sys.UpdateServiceAccount(f.ctx, snapshot.AccessKey, updateServiceAccountOpts{secretKey: "refresh-rotated-secret", description: "rotated"}) + mustIAM(t, err) + + wrote, err := f.sys.store.RefreshUserGroups(f.ctx, snapshot, []string{"g1"}) + mustIAM(t, err) + if !wrote { + t.Fatal("a refresh of a live credential must write its new groups") + } + check := func(where string, u UserIdentity, ok bool) { + t.Helper() + switch { + case !ok: + t.Fatalf("%s: credential missing", where) + case u.Credentials.SecretKey != "refresh-rotated-secret": + t.Errorf("%s: the refresh undid the rotation", where) + case u.Credentials.Description != "rotated": + t.Errorf("%s: the refresh undid an edit", where) + case !slices.Equal(u.Credentials.Groups, []string{"g1"}): + t.Errorf("%s: groups %v, want [g1]", where, u.Credentials.Groups) + } + } + u, ok := f.sys.store.GetUser(snapshot.AccessKey) + check("cache", u, ok) + u, ok = f.stored(t, snapshot.AccessKey) + check("storage", u, ok) + if _, valid, _ := f.sys.CheckKey(f.ctx, snapshot.AccessKey); !valid { + t.Error("the rotated credential must stay valid") + } +} + +func TestIAMGroupRefreshKeepsDeletion(t *testing.T) { + f := newGroupRefreshFixture(t) + snapshot := f.service(t, "refresh-delete") + mustIAM(t, f.sys.DeleteServiceAccount(f.ctx, snapshot.AccessKey, false)) + + wrote, err := f.sys.store.RefreshUserGroups(f.ctx, snapshot, []string{"g1"}) + mustIAM(t, err) + if wrote { + t.Error("a refresh wrote a deleted credential") + } + if _, ok := f.sys.store.GetUser(snapshot.AccessKey); ok { + t.Error("a refresh brought a deleted credential back into the cache") + } + if _, ok := f.stored(t, snapshot.AccessKey); ok { + t.Error("a refresh brought a deleted credential back in storage") + } +} + +// Another server changed the credential and this server's cache has not +// heard of it: its snapshot and its cache are both stale. +func TestIAMGroupRefreshStaleSiblingCache(t *testing.T) { + for _, tc := range []struct { + name string + change func(t *testing.T, f *groupRefreshFixture, accessKey string) + check func(t *testing.T, u UserIdentity, ok bool) + }{ + {"rotated", func(t *testing.T, f *groupRefreshFixture, accessKey string) { + _, err := f.sys.UpdateServiceAccount(f.ctx, accessKey, updateServiceAccountOpts{secretKey: "refresh-rotated-secret"}) + mustIAM(t, err) + }, func(t *testing.T, u UserIdentity, ok bool) { + if !ok || u.Credentials.SecretKey != "refresh-rotated-secret" { + t.Errorf("a stale server undid the rotation of another (present=%v)", ok) + } + }}, + {"deleted", func(t *testing.T, f *groupRefreshFixture, accessKey string) { + mustIAM(t, f.sys.DeleteServiceAccount(f.ctx, accessKey, false)) + }, func(t *testing.T, u UserIdentity, ok bool) { + if ok { + t.Error("a stale server brought back a credential another deleted") + } + }}, + } { + t.Run(tc.name, func(t *testing.T) { + f := newGroupRefreshFixture(t) + snapshot := f.service(t, "refresh-sib-"+tc.name) + other := f.sibling(t) + stale, ok := other.GetUser(snapshot.AccessKey) + if !ok { + t.Fatal("sibling fixture did not load the credential") + } + tc.change(t, f, snapshot.AccessKey) + if cached, _ := other.GetUser(snapshot.AccessKey); cached.Credentials.SecretKey != stale.Credentials.SecretKey { + t.Fatal("control: the sibling cache must not have heard of the change") + } + _, err := other.RefreshUserGroups(f.ctx, stale.Credentials, []string{"g1"}) + mustIAM(t, err) + u, ok := f.stored(t, snapshot.AccessKey) + tc.check(t, u, ok) + // The refreshing server's own cache follows storage too: it holds + // the stored record, or nothing, so a lookup loads it. + u, ok = other.GetUser(snapshot.AccessKey) + tc.check(t, u, ok) + }) + } +} + +// A stored record may lack its access key; the loaders take it from the +// path, and so does the refresh. Such a credential still gets its groups, +// including a removal. +func TestIAMGroupRefreshRecordWithoutAccessKey(t *testing.T) { + f := newGroupRefreshFixture(t) + snapshot := f.service(t, "refresh-nokey") + path := getUserIdentityPath(snapshot.AccessKey, svcUser) + var u UserIdentity + mustIAM(t, f.sys.store.loadIAMConfig(f.ctx, &u, path)) + u.Credentials.AccessKey = "" + u.Credentials.Groups = []string{"admins"} + mustIAM(t, f.sys.store.saveIAMConfig(f.ctx, &u, path)) + loaded, ok := f.stored(t, snapshot.AccessKey) + if !ok || loaded.Credentials.AccessKey != snapshot.AccessKey { + t.Fatal("control: the loaders must take the access key from the path") + } + wrote, err := f.sys.store.RefreshUserGroups(f.ctx, loaded.Credentials, []string{"users"}) + mustIAM(t, err) + after, ok := f.stored(t, snapshot.AccessKey) + if !wrote || !ok || !slices.Equal(after.Credentials.Groups, []string{"users"}) { + t.Errorf("wrote=%v present=%v groups=%v: a record without its access key must still lose a group", wrote, ok, after.Credentials.Groups) + } +} + +// The access key was deleted and created again. Under the same parent the +// groups the lookup computed are that parent's, and the new record gets +// them with everything else it has; under another parent it is left alone. +func TestIAMGroupRefreshRecreated(t *testing.T) { + f := newGroupRefreshFixture(t) + snapshot := f.service(t, "refresh-recreate") + mustIAM(t, f.sys.DeleteServiceAccount(f.ctx, snapshot.AccessKey, false)) + _, _, err := f.sys.NewServiceAccount(f.ctx, f.parent.AccessKey, nil, newServiceAccountOpts{accessKey: snapshot.AccessKey, secretKey: "refresh-original-secret", description: "recreated"}) + mustIAM(t, err) + _, err = f.sys.store.RefreshUserGroups(f.ctx, snapshot, []string{"g1"}) + mustIAM(t, err) + u, ok := f.stored(t, snapshot.AccessKey) + if !ok || u.Credentials.Description != "recreated" || !slices.Equal(u.Credentials.Groups, []string{"g1"}) { + t.Errorf("same parent: present=%v description=%q groups=%v, want the recreated record with [g1]", ok, u.Credentials.Description, u.Credentials.Groups) + } + + other := iamAdminUser(f.ctx, t, f.sys, "refresh-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: "refresh-other-secret"}) + mustIAM(t, err) + wrote, err := f.sys.store.RefreshUserGroups(f.ctx, snapshot, []string{"g2"}) + mustIAM(t, err) + u, ok = f.stored(t, snapshot.AccessKey) + if wrote || !ok || u.Credentials.ParentUser != other.AccessKey || slices.Contains(u.Credentials.Groups, "g2") { + t.Errorf("another parent: wrote=%v present=%v parent=%q groups=%v, want it left alone", wrote, ok, u.Credentials.ParentUser, u.Credentials.Groups) + } +} + +// Groups the record already has are not written again. +func TestIAMGroupRefreshUnchanged(t *testing.T) { + f := newGroupRefreshFixture(t) + snapshot := f.service(t, "refresh-unchanged") + _, err := f.sys.store.RefreshUserGroups(f.ctx, snapshot, []string{"g1"}) + mustIAM(t, err) + before, _ := f.stored(t, snapshot.AccessKey) + wrote, err := f.sys.store.RefreshUserGroups(f.ctx, snapshot, []string{"g1"}) + mustIAM(t, err) + after, _ := f.stored(t, snapshot.AccessKey) + if wrote || !after.UpdatedAt.Equal(before.UpdatedAt) { + t.Errorf("wrote=%v, revision %s -> %s: unchanged groups must not be written", wrote, before.UpdatedAt, after.UpdatedAt) + } +} diff --git a/cmd/iam-revision.go b/cmd/iam-revision.go index 3ce8cb639..6f7980705 100644 --- a/cmd/iam-revision.go +++ b/cmd/iam-revision.go @@ -16,6 +16,7 @@ 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" @@ -45,6 +46,22 @@ 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 { + UserIdentity // the record written, or found unchanged + accessKey string + parentUser string + groups []string + unchanged bool // the stored record already has the groups +} + // The authoritative revocation is durable even if dependent cleanup fails. // Callers must publish it to sibling caches before returning the error. type iamCommittedCleanupError struct { @@ -252,6 +269,35 @@ func saveIAMRevision(ctx context.Context, store IAMStorageAPI, path string, item if err != nil { return err } + if op, ok := item.(*iamGroupRefresh); ok { + var current UserIdentity + err := store.loadIAMConfig(ctx, ¤t, path) + if errors.Is(err, errConfigNotFound) { + return errIAMStaleUpdate + } + if err != nil { + return err + } + 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 set.CreateStringSet(current.Credentials.Groups...).Equals(set.CreateStringSet(op.groups...)) { + op.UserIdentity, op.unchanged = current, true + return nil + } + current.Credentials.Groups = op.groups + current.UpdatedAt = time.Time{} // a new revision, as for any local edit + op.UserIdentity = current + item = &op.UserIdentity + if strings.HasPrefix(path, iamConfigSTSPrefix) { + // Keep the record expiring with its token, as SetTempUser stores it. + opts = []options{{ttl: max(1, int64(time.Until(current.Credentials.Expiration).Seconds()))}} + } + } if _, expiring := item.(*iamExpireIdentity); expiring { sts := strings.HasPrefix(path, iamConfigSTSPrefix) if previous.Deleted { diff --git a/cmd/iam-store.go b/cmd/iam-store.go index a647d24a7..77fd3dad4 100644 --- a/cmd/iam-store.go +++ b/cmd/iam-store.go @@ -3144,27 +3144,43 @@ func (store *IAMStoreSys) getSTSAndServiceAccounts(cache *iamCache) []auth.Crede return res } -// UpdateUserIdentity - updates a user credential. -func (store *IAMStoreSys) UpdateUserIdentity(ctx context.Context, cred auth.Credentials) error { +// 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. +func (store *IAMStoreSys) RefreshUserGroups(ctx context.Context, cred auth.Credentials, groups []string) (bool, error) { cache := store.lock() defer store.unlock() - cache.updatedAt = time.Now() - - userType := regUser - if cred.IsServiceAccount() { - userType = svcUser - } else if cred.IsTemp() { + userType := svcUser + if !cred.IsServiceAccount() { + if !cred.IsTemp() { + return false, errInvalidArgument + } userType = stsUser } - ui := newUserIdentity(cred) - // Overwrite the user identity here. As store should be - // atomic, it shouldn't cause any corruption. - if err := store.saveUserIdentity(ctx, cred.AccessKey, userType, &ui); err != nil { - return err + 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) + } + return false, nil } - - return cache.updateUserWithClaims(cred.AccessKey, ui) + 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 !op.unchanged, nil } // LoadUser - attempts to load user info from storage and updates cache. diff --git a/cmd/iam.go b/cmd/iam.go index 2ff019e0c..c1260cf7c 100644 --- a/cmd/iam.go +++ b/cmd/iam.go @@ -1658,8 +1658,10 @@ func (sys *IAMSys) updateGroupMembershipsForLDAP(ctx context.Context) { continue } - cred.Groups = currGroups - if err := sys.store.UpdateUserIdentity(ctx, cred); err != nil { + // Only the groups change, on the credential as it is stored now: + // cred is a snapshot from before the lookup, and a rotation, an + // edit or a deletion since then must stand. + if _, err := sys.store.RefreshUserGroups(ctx, cred, currGroups); err != nil { // Log and continue error - perhaps it'll work the next time. iamLogIf(GlobalContext, err) }