From a0f30afe5fcacd045fef18e58d6051a5c2d0c241 Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Wed, 4 Nov 2026 05:26:07 -0800 Subject: [PATCH] s3: keep service account keys when a pushed identity replaces its parent (#12394) * s3: keep service account keys when a pushed identity replaces its parent Service account changes push the parent user, whose proto carries only the user's own keys. The merge replaced the cached parent with that proto and dropped every service account key until a restart. Read the parent's service accounts from the credential store so the push rebuilds them, deleted and disabled ones included. Co-Authored-By: Claude Opus 5.5 * s3: test that a parent identity push keeps its service account keys Co-Authored-By: Claude Opus 5.5 * s3: test a sibling service account survives a deletion push Also check the restored secret and status, not just the key. Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: Claude Opus 5.5 --- weed/s3api/auth_credentials.go | 11 ++++ weed/s3api/auth_credentials_upsert_test.go | 71 ++++++++++++++++++++++ 2 files changed, 82 insertions(+) diff --git a/weed/s3api/auth_credentials.go b/weed/s3api/auth_credentials.go index 01ec4eea64..c2be4829b7 100644 --- a/weed/s3api/auth_credentials.go +++ b/weed/s3api/auth_credentials.go @@ -1412,6 +1412,17 @@ func (iam *IdentityAccessManagement) UpsertIdentity(ident *iam_pb.Identity) erro config.Policies = append(config.Policies, &iam_pb.Policy{Name: runtimeName, Content: string(content)}) } } + // The proto carries only the user's own keys; the merge replaces the + // identity, so its service account keys have to come from the store. + serviceAccounts, err := iam.credentialManager.ListServiceAccounts(context.Background()) + if err != nil { + return err + } + for _, sa := range serviceAccounts { + if sa.ParentUser == ident.Name { + config.ServiceAccounts = append(config.ServiceAccounts, sa) + } + } } if err := iam.MergeS3ApiConfiguration(config, false, false); err != nil { return err diff --git a/weed/s3api/auth_credentials_upsert_test.go b/weed/s3api/auth_credentials_upsert_test.go index bc272f330a..9bf045c0c4 100644 --- a/weed/s3api/auth_credentials_upsert_test.go +++ b/weed/s3api/auth_credentials_upsert_test.go @@ -367,3 +367,74 @@ func TestPutGroupRaceKeepsNewerInlinePolicy(t *testing.T) { s3a.iam.VerifyActionPermission(req, ident, s3_constants.ACTION_READ, "bucket", "obj"), "a stale group push must not restore a grant the newer policy revoked") } + +// Service account changes push the parent identity, whose proto carries only +// the user's own keys. The upsert must rebuild the parent's service account +// keys from the store instead of dropping them. +func TestUpsertIdentityKeepsServiceAccountKeys(t *testing.T) { + s3a := newTestS3ApiServerWithMemoryIAM(t, []*iam_pb.Identity{{ + Name: "pacs", + Credentials: []*iam_pb.Credential{{AccessKey: "AKPACS", SecretKey: "secret"}}, + Actions: []string{"Read:bucket"}, + }}) + cm := s3a.iam.credentialManager + ctx := context.Background() + expiration := time.Now().Add(time.Hour).Unix() + newServiceAccount := func(id, accessKey string) *iam_pb.ServiceAccount { + return &iam_pb.ServiceAccount{ + Id: id, + ParentUser: "pacs", + Credential: &iam_pb.Credential{AccessKey: accessKey, SecretKey: "sa-secret", Status: "Active"}, + Expiration: expiration, + } + } + + first := newServiceAccount("sa:pacs:1", "AKSA1") + require.NoError(t, cm.CreateServiceAccount(ctx, first)) + parent, err := cm.GetUser(ctx, "pacs") + require.NoError(t, err) + require.NoError(t, s3a.iam.ReplaceS3ApiConfiguration(&iam_pb.S3ApiConfiguration{ + Identities: []*iam_pb.Identity{parent}, + ServiceAccounts: []*iam_pb.ServiceAccount{first}, + })) + + second := newServiceAccount("sa:pacs:2", "AKSA2") + require.NoError(t, cm.CreateServiceAccount(ctx, second)) + pushed, err := cm.GetUser(ctx, "pacs") + require.NoError(t, err) + require.NoError(t, s3a.iam.UpsertIdentity(pushed)) + + for _, accessKey := range []string{"AKPACS", "AKSA1", "AKSA2"} { + ident, cred, found := s3a.iam.LookupByAccessKey(accessKey) + require.True(t, found, "access key %s must survive the parent push", accessKey) + assert.Equal(t, "pacs", ident.Name) + if accessKey != "AKPACS" { + assert.Equal(t, "sa-secret", cred.SecretKey) + assert.Equal(t, "Active", cred.Status) + assert.Equal(t, expiration, cred.Expiration) + } + } + + require.NoError(t, cm.DeleteServiceAccount(ctx, first.Id)) + pushed, err = cm.GetUser(ctx, "pacs") + require.NoError(t, err) + require.NoError(t, s3a.iam.UpsertIdentity(pushed)) + _, _, found := s3a.iam.LookupByAccessKey("AKSA1") + assert.False(t, found, "a deleted service account must stop authenticating") + _, _, found = s3a.iam.LookupByAccessKey("AKSA2") + assert.True(t, found, "a sibling service account must survive another one's deletion") + + disabled := newServiceAccount(second.Id, "AKSA2") + disabled.Disabled = true + require.NoError(t, cm.UpdateServiceAccount(ctx, second.Id, disabled)) + pushed, err = cm.GetUser(ctx, "pacs") + require.NoError(t, err) + require.NoError(t, s3a.iam.UpsertIdentity(pushed)) + + _, _, found = s3a.iam.LookupByAccessKey("AKSA1") + assert.False(t, found) + _, _, found = s3a.iam.LookupByAccessKey("AKSA2") + assert.False(t, found, "a disabled service account must stop authenticating") + _, _, found = s3a.iam.LookupByAccessKey("AKPACS") + assert.True(t, found) +}