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 <noreply@anthropic.com> Signed-off-by: Feng Ruohang <rh@vonng.com>
2026-11-09 17:21:51 +03:00 · 2026-11-08 12:00:20 +08:00
parent af0afd2321
commit a0de92fcbe
5 changed files with 312 additions and 20 deletions
@@ -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"})
@@ -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)
+	}
+}
@@ -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, &current, 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 {
@@ -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.
@@ -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)
 			}