From a0de92fcbe31b9cb12d36ac2c4bbe3c4c7de829b Mon Sep 17 00:00:00 2001 From: Feng Ruohang Date: Sun, 8 Nov 2026 12:00:20 +0800 Subject: [PATCH] fix(iam): refresh LDAP groups on the stored credential, not a stale snapshot The hourly LDAP group refresh, which every server runs, reads the STS and service-account credentials from its own cache, queries the directory, and wrote each credential whose groups changed back whole through UpdateUserIdentity. A secret rotation or an edit made meanwhile, on this server or one whose notification it missed, was undone, and a credential deleted meanwhile came back enabled with its old secret. RefreshUserGroups replaces that writer, whose only caller was the refresh. Under the identity's cross-node revision lock it loads the stored record, refuses one that is absent, deleted, expired or now has another parent (whose groups the lookup did not compute), and otherwise changes only the groups of the stored record. A record without its access key gets it from the path, as the loaders do. A refused refresh drops the stale entry from the cache, which loads what is stored on next use; a written or unchanged one caches the stored record. An STS record keeps the lease of its token's remaining lifetime. Tests cover rotation and edit, deletion, a sibling server whose cache did not hear of a rotation or a deletion, recreation under the same and under another parent, unchanged groups, and a stored record without its access key. Written back as the old snapshot write, every case fails. Co-Authored-By: Claude Opus 5.5 Signed-off-by: Feng Ruohang --- cmd/batch-submitter-edits_test.go | 8 +- cmd/iam-ldap-group-refresh_test.go | 226 +++++++++++++++++++++++++++++ cmd/iam-revision.go | 46 ++++++ cmd/iam-store.go | 46 ++++-- cmd/iam.go | 6 +- 5 files changed, 312 insertions(+), 20 deletions(-) create mode 100644 cmd/iam-ldap-group-refresh_test.go 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) }