From 07ce95dfdafb22449e4b77385c5ec66f3282c670 Mon Sep 17 00:00:00 2001 From: Feng Ruohang Date: Sun, 8 Nov 2026 14:19:44 +0800 Subject: [PATCH] fix(iam): protect stale creates and cache loads Signed-off-by: Feng Ruohang --- cmd/iam-load-user-race_test.go | 79 ++++++++++++++++++++++++++++ cmd/iam-revision.go | 25 +++++++++ cmd/iam-store.go | 12 ++++- cmd/iam-stored-identity-edit_test.go | 29 ++++++++++ 4 files changed, 144 insertions(+), 1 deletion(-) create mode 100644 cmd/iam-load-user-race_test.go diff --git a/cmd/iam-load-user-race_test.go b/cmd/iam-load-user-race_test.go new file mode 100644 index 000000000..b798a9d6b --- /dev/null +++ b/cmd/iam-load-user-race_test.go @@ -0,0 +1,79 @@ +// Copyright (c) 2026 PGSTY +// SPDX-License-Identifier: AGPL-3.0-or-later + +package cmd + +import ( + "context" + "sync" + "testing" + "time" +) + +// Hold a lazy user load after it has read storage. This makes the ordering +// between an in-flight read and a completed credential edit deterministic. +type iamLoadUserBlocker struct { + IAMStorageAPI + key string + waiting chan struct{} + release chan struct{} + once sync.Once +} + +func (s *iamLoadUserBlocker) loadUser(ctx context.Context, key string, userType IAMUserType, out map[string]UserIdentity) error { + err := s.IAMStorageAPI.loadUser(ctx, key, userType, out) + if err == nil && key == s.key && userType == regUser { + s.once.Do(func() { close(s.waiting) }) + select { + case <-s.release: + case <-ctx.Done(): + return ctx.Err() + } + } + return err +} + +func TestIAMLoadUserDoesNotRestoreRotatedCredential(t *testing.T) { + ctx, sys, _ := prepareIAMRevisionFixture(t) + user := iamAdminUser(ctx, t, sys, "load-user-race", "readwrite") + original := sys.store.IAMStorageAPI + blocker := &iamLoadUserBlocker{ + IAMStorageAPI: original, + key: user.AccessKey, + waiting: make(chan struct{}), + release: make(chan struct{}), + } + sys.store.IAMStorageAPI = blocker + t.Cleanup(func() { sys.store.IAMStorageAPI = original }) + + loadDone := make(chan error, 1) + go func() { loadDone <- sys.store.LoadUser(ctx, user.AccessKey) }() + select { + case <-blocker.waiting: + case <-time.After(5 * time.Second): + t.Fatal("LoadUser did not reach the controlled read barrier") + } + + const rotatedSecret = "load-user-rotated-secret" + if err := sys.SetUserSecretKey(ctx, user.AccessKey, rotatedSecret); err != nil { + t.Fatal(err) + } + current, valid, err := sys.CheckKey(ctx, user.AccessKey) + if err != nil || !valid || current.Credentials.SecretKey != rotatedSecret { + t.Fatalf("rotation did not update the serving cache: valid=%v secret=%q err=%v", valid, current.Credentials.SecretKey, err) + } + + close(blocker.release) + select { + case err := <-loadDone: + if err != nil { + t.Fatal(err) + } + case <-time.After(5 * time.Second): + t.Fatal("LoadUser did not finish after the read barrier was released") + } + loaded, valid, err := sys.CheckKey(ctx, user.AccessKey) + if err != nil || !valid || loaded.Credentials.SecretKey != rotatedSecret { + t.Fatalf("an in-flight LoadUser restored the old credential: valid=%v secret=%q err=%v", valid, loaded.Credentials.SecretKey, err) + } +} diff --git a/cmd/iam-revision.go b/cmd/iam-revision.go index 3df5baed0..09a8ae6b5 100644 --- a/cmd/iam-revision.go +++ b/cmd/iam-revision.go @@ -60,6 +60,15 @@ type iamIdentityEdit struct { unchanged bool } +// An identity create is checked against the record under the same revision +// lock as the write. Local callers may recreate a deleted access key, but may +// never replace a live stored service account that their cache did not contain. +// Replicated creates retain the normal revision ordering below and may replace +// an older live snapshot when their source revision wins. +type iamIdentityCreate struct { + UserIdentity +} + var ( errIAMIdentityGone = errors.New("IAM identity is absent or deleted in storage") errIAMIdentityUnchanged = errors.New("IAM identity edit changes nothing") @@ -272,6 +281,22 @@ func saveIAMRevision(ctx context.Context, store IAMStorageAPI, path string, item if err != nil { return err } + if op, ok := item.(*iamIdentityCreate); ok { + if _, replicated := iamReplicationTime(ctx); !replicated { + var current UserIdentity + err := store.loadIAMConfig(ctx, ¤t, path) + switch { + case err == nil && !current.Deleted: + return fmt.Errorf("%w: access key already exists in storage", errIAMServiceAccountNotAllowed) + case err == nil, errors.Is(err, errConfigNotFound): + // A tombstone or an absent record can be recreated. The + // existing revision path below retains its revocation boundary. + default: + return err + } + } + item = &op.UserIdentity + } if op, ok := item.(*iamIdentityEdit); ok { var current UserIdentity err := store.loadIAMConfig(ctx, ¤t, path) diff --git a/cmd/iam-store.go b/cmd/iam-store.go index 9c9538569..788f73a29 100644 --- a/cmd/iam-store.go +++ b/cmd/iam-store.go @@ -2831,10 +2831,12 @@ func (store *IAMStoreSys) AddServiceAccount(ctx context.Context, cred auth.Crede } u := newUserIdentity(cred) - err = store.saveUserIdentity(ctx, u.Credentials.AccessKey, svcUser, &u) + create := &iamIdentityCreate{UserIdentity: u} + err = store.saveIAMRevision(ctx, getUserIdentityPath(u.Credentials.AccessKey, svcUser), create) if err != nil { return updatedAt, err } + u = create.UserIdentity if err := cache.updateUserWithClaims(u.Credentials.AccessKey, u); err != nil { // Persistence already replaced the credential. A failed refresh must @@ -3223,6 +3225,7 @@ func (store *IAMStoreSys) forgetCachedIdentity(cache *iamCache, accessKey string // LoadUser - attempts to load user info from storage and updates cache. func (store *IAMStoreSys) LoadUser(ctx context.Context, accessKey string) error { + loadedAt := time.Now() groupLoad := env.Get("_MINIO_IAM_GROUP_REFRESH", config.EnableOff) == config.EnableOn newCachePopulate := func() (val any, err error) { @@ -3317,6 +3320,13 @@ func (store *IAMStoreSys) LoadUser(ctx context.Context, accessKey string) error cache := store.lock() defer store.unlock() + // A partial load may have started before a credential edit or deletion and + // finished after it. Do not merge that snapshot over the newer cache state; + // the next notification or load will supply the missing entry. + if cache.updatedAt.After(loadedAt) { + return nil + } + // We need to merge the new cache with the existing cache because the // periodic IAM reload is partial. The periodic load here is to account. newCache.iamGroupPolicyMap.Range(func(k string, v MappedPolicy) bool { diff --git a/cmd/iam-stored-identity-edit_test.go b/cmd/iam-stored-identity-edit_test.go index 42c22c437..04164bb23 100644 --- a/cmd/iam-stored-identity-edit_test.go +++ b/cmd/iam-stored-identity-edit_test.go @@ -59,6 +59,35 @@ func TestIAMStaleServiceAccountEdit(t *testing.T) { }) } +// A server that missed the create notification must not replace a live +// service account merely because its local cache has no entry for the key. +func TestIAMStaleServiceAccountCreate(t *testing.T) { + f := newGroupRefreshFixture(t) + other := f.sibling(t) + original := f.service(t, "create-stale") + if _, ok := other.GetUser(original.AccessKey); ok { + t.Fatal("control: the stale server must not have loaded the new account") + } + + claims := map[string]any{ + parentClaim: f.parent.AccessKey, + iamPolicyClaimNameSA(): inheritedPolicyType, + } + mustIAM(t, setIAMParentRevocationClaim(f.ctx, other, f.parent.AccessKey, claims)) + replacement, err := auth.CreateNewCredentialsWithMetadata(original.AccessKey, "create-replacement-secret", claims, "create-replacement-secret") + mustIAM(t, err) + replacement.ParentUser = f.parent.AccessKey + replacement.Status = auth.AccountOn + if _, err := other.AddServiceAccount(f.ctx, replacement); !errors.Is(err, errIAMServiceAccountNotAllowed) { + t.Fatalf("stale create should be rejected, got %v", err) + } + + stored, ok := f.stored(t, original.AccessKey) + if !ok || stored.Credentials.SecretKey != original.SecretKey || stored.Credentials.ParentUser != original.ParentUser { + t.Fatalf("stale create changed the live account: present=%v parent=%q secret=%q", ok, stored.Credentials.ParentUser, stored.Credentials.SecretKey) + } +} + // A session policy edit stores the claims of the token it signs. func TestIAMServiceAccountEditStoresNewClaims(t *testing.T) { f := newGroupRefreshFixture(t)