diff --git a/weed/credential/grpc/grpc_identity.go b/weed/credential/grpc/grpc_identity.go index 831bad6175..0acbba550f 100644 --- a/weed/credential/grpc/grpc_identity.go +++ b/weed/credential/grpc/grpc_identity.go @@ -120,13 +120,20 @@ func (store *IamGrpcStore) CreateAccessKey(ctx context.Context, username string, } func (store *IamGrpcStore) DeleteAccessKey(ctx context.Context, username string, accessKey string) error { - return store.withIamClient(ctx, func(ctx context.Context, client iam_pb.SeaweedIdentityAccessManagementClient) error { + err := store.withIamClient(ctx, func(ctx context.Context, client iam_pb.SeaweedIdentityAccessManagementClient) error { _, err := client.DeleteAccessKey(ctx, &iam_pb.DeleteAccessKeyRequest{ Username: username, AccessKey: accessKey, }) return err }) + // The filer-side handler returns gRPC NotFound when the user or the key is + // absent; translate back to the sentinel so callers can use + // errors.Is(err, credential.ErrAccessKeyNotFound) uniformly across stores. + if err != nil && status.Code(err) == codes.NotFound { + return credential.ErrAccessKeyNotFound + } + return err } // AttachUserPolicy attaches a managed policy to a user by policy name diff --git a/weed/credential/grpc/grpc_store_test.go b/weed/credential/grpc/grpc_store_test.go index c157cc1438..ac2d546774 100644 --- a/weed/credential/grpc/grpc_store_test.go +++ b/weed/credential/grpc/grpc_store_test.go @@ -2,6 +2,7 @@ package grpc import ( "context" + "errors" "fmt" "net" "strings" @@ -106,3 +107,50 @@ func TestIamGrpcStore_AdminBearerToken(t *testing.T) { } }) } + +// TestIamGrpcStore_DeleteAccessKeyNotFound pins the not-found translation: +// the filer reports a missing user or key as gRPC NotFound, and the store must +// surface it as credential.ErrAccessKeyNotFound so repeat deletes stay a no-op +// for IAM dispatchers that tolerate the sentinel. +func TestIamGrpcStore_DeleteAccessKeyNotFound(t *testing.T) { + const goodKey = "iam-admin-itest-key" + + cm, err := credential.NewCredentialManager(credential.StoreTypeMemory, nil, "") + if err != nil { + t.Fatalf("NewCredentialManager: %v", err) + } + defer cm.Shutdown() + + iamSrv := weed_server.NewIamGrpcServer(cm, security.SigningKey(goodKey)) + + lis, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatalf("listen: %v", err) + } + grpcSrv := grpc.NewServer() + iam_pb.RegisterSeaweedIdentityAccessManagementServer(grpcSrv, iamSrv) + go func() { _ = grpcSrv.Serve(lis) }() + t.Cleanup(grpcSrv.Stop) + + _, portStr, err := net.SplitHostPort(lis.Addr().String()) + if err != nil { + t.Fatalf("split listener addr: %v", err) + } + serverAddr := pb.ServerAddress(fmt.Sprintf("127.0.0.1:0.%s", portStr)) + + store := &IamGrpcStore{} + store.SetFilerAddressFunc(func() pb.ServerAddress { return serverAddr }, grpc.WithTransportCredentials(insecure.NewCredentials())) + store.SetAdminSigning(security.SigningKey(goodKey), 0) + + ctx := context.Background() + if err := store.CreateUser(ctx, &iam_pb.Identity{Name: "alice"}); err != nil { + t.Fatalf("CreateUser: %v", err) + } + + if err := store.DeleteAccessKey(ctx, "alice", "AKIAMISSING0000000001"); !errors.Is(err, credential.ErrAccessKeyNotFound) { + t.Fatalf("DeleteAccessKey with missing key: got %v, want ErrAccessKeyNotFound", err) + } + if err := store.DeleteAccessKey(ctx, "ghost", "AKIAMISSING0000000001"); !errors.Is(err, credential.ErrAccessKeyNotFound) { + t.Fatalf("DeleteAccessKey with missing user: got %v, want ErrAccessKeyNotFound", err) + } +} diff --git a/weed/credential/memory/memory_identity.go b/weed/credential/memory/memory_identity.go index 9b578b1529..404b879770 100644 --- a/weed/credential/memory/memory_identity.go +++ b/weed/credential/memory/memory_identity.go @@ -142,6 +142,14 @@ func (store *MemoryStore) UpdateUser(ctx context.Context, username string, ident return credential.ErrUserNotFound } + if identity.Name != username { + // Rename: the destination name must not already be taken, matching + // RenameUser's collision check. + if _, clash := store.users[identity.Name]; clash { + return credential.ErrUserAlreadyExists + } + } + // Remove old access keys from index for _, cred := range existingUser.Credentials { delete(store.accessKeys, cred.AccessKey) @@ -156,11 +164,15 @@ func (store *MemoryStore) UpdateUser(ctx context.Context, username string, ident // Deep copy to avoid mutation issues identityCopy := store.deepCopyIdentity(identity) - store.users[username] = identityCopy + if identityCopy.Name != username { + // Rename: re-key the record, the way filer_etc does (delete old, save new). + delete(store.users, username) + } + store.users[identityCopy.Name] = identityCopy // Re-index access keys for _, cred := range identity.Credentials { - store.accessKeys[cred.AccessKey] = username + store.accessKeys[cred.AccessKey] = identityCopy.Name } return nil diff --git a/weed/credential/memory/memory_store_test.go b/weed/credential/memory/memory_store_test.go index 2b292fc1b4..52ce01cc7b 100644 --- a/weed/credential/memory/memory_store_test.go +++ b/weed/credential/memory/memory_store_test.go @@ -378,3 +378,82 @@ func TestMemoryStoreServiceAccountByAccessKey(t *testing.T) { t.Errorf("Expected ErrAccessKeyNotFound after delete, got %v", err) } } + +// TestMemoryStoreUpdateUserRename: UpdateUser with an identity whose Name +// differs from the key must re-key the record, the way filer_etc does +// (delete old, save new). The IAM rename path relies on this. +func TestMemoryStoreUpdateUserRename(t *testing.T) { + store := &MemoryStore{} + if err := store.Initialize(util.GetViper(), "credential."); err != nil { + t.Fatalf("Failed to initialize store: %v", err) + } + ctx := context.Background() + + err := store.CreateUser(ctx, &iam_pb.Identity{ + Name: "oldname", + Credentials: []*iam_pb.Credential{{AccessKey: "access123", SecretKey: "secret123"}}, + }) + if err != nil { + t.Fatalf("Failed to create user: %v", err) + } + + err = store.UpdateUser(ctx, "oldname", &iam_pb.Identity{ + Name: "newname", + Credentials: []*iam_pb.Credential{{AccessKey: "access123", SecretKey: "secret123"}}, + }) + if err != nil { + t.Fatalf("Failed to rename user via UpdateUser: %v", err) + } + + if _, err := store.GetUser(ctx, "oldname"); err != credential.ErrUserNotFound { + t.Fatalf("Expected ErrUserNotFound for old name, got %v", err) + } + renamed, err := store.GetUser(ctx, "newname") + if err != nil { + t.Fatalf("Failed to get renamed user: %v", err) + } + if renamed.Name != "newname" { + t.Errorf("Expected 'newname', got '%s'", renamed.Name) + } + byKey, err := store.GetUserByAccessKey(ctx, "access123") + if err != nil { + t.Fatalf("Access key must resolve to the renamed user: %v", err) + } + if byKey.Name != "newname" { + t.Errorf("Expected access key owner 'newname', got '%s'", byKey.Name) + } +} + +// TestMemoryStoreUpdateUserRenameCollision: a rename whose destination name is +// taken must fail rather than overwrite the existing user. +func TestMemoryStoreUpdateUserRenameCollision(t *testing.T) { + store := &MemoryStore{} + if err := store.Initialize(util.GetViper(), "credential."); err != nil { + t.Fatalf("Failed to initialize store: %v", err) + } + ctx := context.Background() + + if err := store.CreateUser(ctx, &iam_pb.Identity{Name: "alice"}); err != nil { + t.Fatalf("Failed to create alice: %v", err) + } + if err := store.CreateUser(ctx, &iam_pb.Identity{ + Name: "bob", + Credentials: []*iam_pb.Credential{{AccessKey: "bobkey123", SecretKey: "secret123"}}, + }); err != nil { + t.Fatalf("Failed to create bob: %v", err) + } + + err := store.UpdateUser(ctx, "alice", &iam_pb.Identity{Name: "bob"}) + if err != credential.ErrUserAlreadyExists { + t.Fatalf("Expected ErrUserAlreadyExists, got %v", err) + } + + // Both users must be intact. + if _, err := store.GetUser(ctx, "alice"); err != nil { + t.Fatalf("alice must still exist: %v", err) + } + bob, err := store.GetUserByAccessKey(ctx, "bobkey123") + if err != nil || bob.Name != "bob" { + t.Fatalf("bob's access key must still resolve to bob: %v", err) + } +} diff --git a/weed/iamapi/iamapi_management_handlers.go b/weed/iamapi/iamapi_management_handlers.go index e6250d9fbe..76fe3154e9 100644 --- a/weed/iamapi/iamapi_management_handlers.go +++ b/weed/iamapi/iamapi_management_handlers.go @@ -4,6 +4,7 @@ package iamapi // Common IAM types and helpers are imported from the shared weed/iam package. import ( + "context" "encoding/json" "errors" "fmt" @@ -14,6 +15,7 @@ import ( "sync" "github.com/aws/aws-sdk-go/service/iam" + "github.com/seaweedfs/seaweedfs/weed/credential" "github.com/seaweedfs/seaweedfs/weed/glog" iamlib "github.com/seaweedfs/seaweedfs/weed/iam" "github.com/seaweedfs/seaweedfs/weed/pb/filer_pb" @@ -1312,6 +1314,115 @@ func (iama *IamApiServer) handleImplicitUsername(r *http.Request, values url.Val values.Set("UserName", identity.Name) } +// targetedCredentialManager returns the credential manager through which a change to the named +// identity is written on its own instead of via SaveConfiguration, or nil when this server has +// none, or the identity is static: static identities live outside the store and keep the +// full-save path. +func (iama *IamApiServer) targetedCredentialManager(userName string) *credential.CredentialManager { + if iama.iam == nil { + return nil + } + cm := iama.iam.GetCredentialManager() + if cm == nil || cm.IsStaticIdentity(userName) { + return nil + } + return cm +} + +// findIdentityByName returns the identity named userName in s3cfg, or nil. +func findIdentityByName(s3cfg *iam_pb.S3ApiConfiguration, userName string) *iam_pb.Identity { + for _, ident := range s3cfg.Identities { + if ident.Name == userName { + return ident + } + } + return nil +} + +// groupsWithMember returns the groups in s3cfg that list userName as a member. Taken before a +// handler renames or removes the user, it names the group records that handler will change. +func groupsWithMember(s3cfg *iam_pb.S3ApiConfiguration, userName string) []*iam_pb.Group { + var groups []*iam_pb.Group + for _, g := range s3cfg.Groups { + for _, m := range g.Members { + if m == userName { + groups = append(groups, g) + break + } + } + } + return groups +} + +// serviceAccountsOf returns the service accounts in s3cfg whose parent is userName. +func serviceAccountsOf(s3cfg *iam_pb.S3ApiConfiguration, userName string) []*iam_pb.ServiceAccount { + var accounts []*iam_pb.ServiceAccount + for _, sa := range s3cfg.ServiceAccounts { + if sa.ParentUser == userName { + accounts = append(accounts, sa) + } + } + return accounts +} + +// persistIdentity writes the one identity a handler changed in s3cfg through the credential +// manager. The store writes that identity's record and propagates it alone; the other +// identities are not rewritten. +func persistIdentity(ctx context.Context, cm *credential.CredentialManager, s3cfg *iam_pb.S3ApiConfiguration, userName string) error { + ident := findIdentityByName(s3cfg, userName) + if ident == nil { + return credential.ErrUserNotFound + } + return cm.UpdateUser(ctx, userName, ident) +} + +// persistRenamedIdentity writes a user renamed from oldName to newName by UpdateUser through the +// credential manager: the identity, the groups that listed the old name and the service accounts +// it parented. UpdateUser under the old name moves the record in stores that treat a changed Name +// as a rename (filer_etc, memory), and the propagating store propagates the new identity and the +// old name's removal; UpdateUser already moved the inline policies through PutPolicies. A store +// that did not move the record still holds the user under the old name; the caller is told so +// (false) and keeps the full save, which is what renames such a store today. +func persistRenamedIdentity(ctx context.Context, cm *credential.CredentialManager, s3cfg *iam_pb.S3ApiConfiguration, oldName, newName string, groups []*iam_pb.Group, accounts []*iam_pb.ServiceAccount) (bool, error) { + ident := findIdentityByName(s3cfg, newName) + if ident == nil { + return false, credential.ErrUserNotFound + } + if err := cm.UpdateUser(ctx, oldName, ident); err != nil { + return false, err + } + if _, err := cm.GetUser(ctx, newName); err != nil { + return false, nil + } + for _, g := range groups { + if err := cm.UpdateGroup(ctx, g); err != nil { + return false, fmt.Errorf("update group %s: %w", g.Name, err) + } + } + for _, sa := range accounts { + if err := cm.UpdateServiceAccount(ctx, sa.Id, sa); err != nil { + return false, fmt.Errorf("update service account %s: %w", sa.Id, err) + } + } + return true, nil +} + +// deleteIdentity removes a user deleted by DeleteUser through the credential manager: its record +// (the store propagates the one removal) and the groups that listed it, whose membership +// DeleteUser already changed in s3cfg. DeleteUser already removed the inline policies through +// PutPolicies. +func deleteIdentity(ctx context.Context, cm *credential.CredentialManager, userName string, groups []*iam_pb.Group) error { + if err := cm.DeleteUser(ctx, userName); err != nil { + return err + } + for _, g := range groups { + if err := cm.UpdateGroup(ctx, g); err != nil { + return fmt.Errorf("update group %s: %w", g.Name, err) + } + } + return nil +} + func (iama *IamApiServer) DoActions(w http.ResponseWriter, r *http.Request) { // Lock to prevent concurrent read-modify-write race conditions policyLock.Lock() @@ -1333,6 +1444,9 @@ func (iama *IamApiServer) DoActions(w http.ResponseWriter, r *http.Request) { glog.V(4).Infof("DoActions: %+v", iamlib.RedactSensitiveFormValues(values)) var response iamlib.RequestIDSetter changed := true + // persisted is set by the actions that wrote one identity through the credential manager + // instead of SaveConfiguration; the in-memory identity maps are reloaded after them too. + persisted := false switch r.Form.Get("Action") { case "ListUsers": response = iama.ListUsers(s3cfg, values) @@ -1365,40 +1479,107 @@ func (iama *IamApiServer) DoActions(w http.ResponseWriter, r *http.Request) { } changed = false case "UpdateUser": + userName, newUserName := values.Get("UserName"), values.Get("NewUserName") + // Taken before the rename: the records the handler changes besides the identity. + groups := groupsWithMember(s3cfg, userName) + accounts := serviceAccountsOf(s3cfg, userName) var err *IamError response, err = iama.UpdateUser(s3cfg, values) if err != nil { writeIamErrorResponse(w, r, reqID, err) return } + // Write the renamed identity, its groups and service accounts through the credential + // manager instead of rewriting every identity via SaveConfiguration. A request that + // renames nothing changed nothing, so there is nothing to write either way. + if cm := iama.targetedCredentialManager(userName); cm != nil { + renamed := true + if newUserName != "" && newUserName != userName { + var err error + renamed, err = persistRenamedIdentity(r.Context(), cm, s3cfg, userName, newUserName, groups, accounts) + if err != nil { + writeIamErrorResponse(w, r, reqID, &IamError{Code: s3api.CredentialErrToIamErrCode(err), Error: err}) + return + } + } + changed, persisted = !renamed, renamed + } case "DeleteUser": userName := values.Get("UserName") + groups := groupsWithMember(s3cfg, userName) var err *IamError response, err = iama.DeleteUser(s3cfg, userName) if err != nil { writeIamErrorResponse(w, r, reqID, err) return } + // Targeted delete: one record removed and one removal propagated, plus the groups whose + // membership changed, instead of rewriting every remaining identity. + if cm := iama.targetedCredentialManager(userName); cm != nil { + if err := deleteIdentity(r.Context(), cm, userName, groups); err != nil { + writeIamErrorResponse(w, r, reqID, &IamError{Code: s3api.CredentialErrToIamErrCode(err), Error: err}) + return + } + changed, persisted = false, true + } case "CreateAccessKey": iama.handleImplicitUsername(r, values) - var err *IamError - response, err = iama.CreateAccessKey(s3cfg, values) + userName := values.Get("UserName") + resp, err := iama.CreateAccessKey(s3cfg, values) if err != nil { glog.Errorf("CreateAccessKey: %+v", err.Error) writeIamErrorResponse(w, r, reqID, err) return } + response = resp + // Targeted write: credentialManager.CreateAccessKey adds the key to the one identity and + // propagates that identity alone. The handler creates the identity for a user it does + // not know; that case still needs the full save. + if cm := iama.targetedCredentialManager(userName); cm != nil { + if _, known := cm.GetUser(r.Context(), userName); known == nil { + cred := &iam_pb.Credential{ + AccessKey: *resp.CreateAccessKeyResult.AccessKey.AccessKeyId, + SecretKey: *resp.CreateAccessKeyResult.AccessKey.SecretAccessKey, + Status: accessKeyStatusActive, + } + if err := cm.CreateAccessKey(r.Context(), userName, cred); err != nil { + writeIamErrorResponse(w, r, reqID, &IamError{Code: s3api.CredentialErrToIamErrCode(err), Error: err}) + return + } + changed, persisted = false, true + } + } case "DeleteAccessKey": iama.handleImplicitUsername(r, values) + userName, accessKeyId := values.Get("UserName"), values.Get("AccessKeyId") response = iama.DeleteAccessKey(s3cfg, values) + // Targeted write: credentialManager.DeleteAccessKey removes the key from the one identity + // and propagates that identity alone. The handler answers 200 for a key or user it does + // not know, so a not-found from the store is the same no-op it always was. + if cm := iama.targetedCredentialManager(userName); cm != nil { + err := cm.DeleteAccessKey(r.Context(), userName, accessKeyId) + if err != nil && !errors.Is(err, credential.ErrAccessKeyNotFound) && !errors.Is(err, credential.ErrUserNotFound) { + writeIamErrorResponse(w, r, reqID, &IamError{Code: s3api.CredentialErrToIamErrCode(err), Error: err}) + return + } + changed, persisted = false, true + } case "UpdateAccessKey": iama.handleImplicitUsername(r, values) + userName := values.Get("UserName") var err *IamError response, err = iama.UpdateAccessKey(s3cfg, values) if err != nil { writeIamErrorResponse(w, r, reqID, err) return } + if cm := iama.targetedCredentialManager(userName); cm != nil { + if err := persistIdentity(r.Context(), cm, s3cfg, userName); err != nil { + writeIamErrorResponse(w, r, reqID, &IamError{Code: s3api.CredentialErrToIamErrCode(err), Error: err}) + return + } + changed, persisted = false, true + } case "CreatePolicy": var err *IamError response, err = iama.CreatePolicy(s3cfg, values) @@ -1410,6 +1591,7 @@ func (iama *IamApiServer) DoActions(w http.ResponseWriter, r *http.Request) { // The `changed` flag is false because this does not modify the main s3cfg.Identities configuration. changed = false case "PutUserPolicy": + userName := values.Get("UserName") var err *IamError response, err = iama.PutUserPolicy(s3cfg, values) if err != nil { @@ -1417,6 +1599,15 @@ func (iama *IamApiServer) DoActions(w http.ResponseWriter, r *http.Request) { writeIamErrorResponse(w, r, reqID, err) return } + // The policy document is already stored through PutPolicies; what remains is the + // identity's recomputed Actions, written and propagated for that identity alone. + if cm := iama.targetedCredentialManager(userName); cm != nil { + if err := persistIdentity(r.Context(), cm, s3cfg, userName); err != nil { + writeIamErrorResponse(w, r, reqID, &IamError{Code: s3api.CredentialErrToIamErrCode(err), Error: err}) + return + } + changed, persisted = false, true + } case "GetUserPolicy": var err *IamError response, err = iama.GetUserPolicy(s3cfg, values) @@ -1426,12 +1617,20 @@ func (iama *IamApiServer) DoActions(w http.ResponseWriter, r *http.Request) { } changed = false case "DeleteUserPolicy": + userName := values.Get("UserName") var err *IamError response, err = iama.DeleteUserPolicy(s3cfg, values) if err != nil { writeIamErrorResponse(w, r, reqID, err) return } + if cm := iama.targetedCredentialManager(userName); cm != nil { + if err := persistIdentity(r.Context(), cm, s3cfg, userName); err != nil { + writeIamErrorResponse(w, r, reqID, &IamError{Code: s3api.CredentialErrToIamErrCode(err), Error: err}) + return + } + changed, persisted = false, true + } case "ListUserPolicies": iama.handleImplicitUsername(r, values) var err *IamError @@ -1642,6 +1841,8 @@ func (iama *IamApiServer) DoActions(w http.ResponseWriter, r *http.Request) { writeIamErrorResponse(w, r, reqID, &iamError) return } + } + if changed || persisted { // Reload in-memory identity maps so subsequent LookupByAccessKey calls // can see newly created or deleted keys immediately if iama.iam != nil { diff --git a/weed/iamapi/iamapi_targeted_writes_test.go b/weed/iamapi/iamapi_targeted_writes_test.go new file mode 100644 index 0000000000..a22b56033b --- /dev/null +++ b/weed/iamapi/iamapi_targeted_writes_test.go @@ -0,0 +1,302 @@ +package iamapi + +// Targeted-write regression tests: single-identity IAM actions must write only +// the identity they changed through the credential manager, not every identity +// via PutS3ApiConfiguration/SaveConfiguration. +// See https://github.com/seaweedfs/seaweedfs/issues/11865. + +import ( + "context" + "net/http" + "testing" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/aws/session" + "github.com/aws/aws-sdk-go/service/iam" + "github.com/seaweedfs/seaweedfs/weed/credential" + "github.com/seaweedfs/seaweedfs/weed/credential/memory" + "github.com/seaweedfs/seaweedfs/weed/pb/iam_pb" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// targetedRecordingStore wraps a memory store and counts the write methods +// the dispatcher used, so a test can assert the single-identity path was +// taken. It embeds the concrete store (rather than the CredentialStore +// interface) so optional interfaces like InlinePolicyStore stay visible +// through the wrapper. +type targetedRecordingStore struct { + *memory.MemoryStore + saves int + createAccessKeys int + deleteAccessKeys int + updateUsers int + deleteUsers int + updateGroups int +} + +func (r *targetedRecordingStore) SaveConfiguration(ctx context.Context, config *iam_pb.S3ApiConfiguration) error { + r.saves++ + return r.MemoryStore.SaveConfiguration(ctx, config) +} + +func (r *targetedRecordingStore) CreateAccessKey(ctx context.Context, username string, cred *iam_pb.Credential) error { + r.createAccessKeys++ + return r.MemoryStore.CreateAccessKey(ctx, username, cred) +} + +func (r *targetedRecordingStore) DeleteAccessKey(ctx context.Context, username string, accessKey string) error { + r.deleteAccessKeys++ + return r.MemoryStore.DeleteAccessKey(ctx, username, accessKey) +} + +func (r *targetedRecordingStore) UpdateUser(ctx context.Context, username string, identity *iam_pb.Identity) error { + r.updateUsers++ + return r.MemoryStore.UpdateUser(ctx, username, identity) +} + +func (r *targetedRecordingStore) DeleteUser(ctx context.Context, username string) error { + r.deleteUsers++ + return r.MemoryStore.DeleteUser(ctx, username) +} + +func (r *targetedRecordingStore) UpdateGroup(ctx context.Context, group *iam_pb.Group) error { + r.updateGroups++ + return r.MemoryStore.UpdateGroup(ctx, group) +} + +func targetedIamApiServer() (*IamApiServer, *credential.CredentialManager, *countingConfigSaver, *targetedRecordingStore) { + srv, cm, counter := iamApiServerWithCredentialManager() + rec := &targetedRecordingStore{MemoryStore: cm.Store.(*memory.MemoryStore)} + cm.Store = rec + return srv, cm, counter, rec +} + +const targetedTestPolicy = `{ + "Version": "2012-10-17", + "Statement": [{"Effect": "Allow", "Action": ["s3:GetObject"], "Resource": ["arn:aws:s3:::bench/*"]}] +}` + +func TestIamApiCreateAccessKeyTargeted(t *testing.T) { + srv, cm, counter, rec := targetedIamApiServer() + ctx := context.Background() + require.NoError(t, cm.CreateUser(ctx, &iam_pb.Identity{Name: "alice"})) + require.NoError(t, cm.CreateUser(ctx, &iam_pb.Identity{Name: "bob"})) + + req, _ := iam.New(session.New()).CreateAccessKeyRequest(&iam.CreateAccessKeyInput{UserName: aws.String("alice")}) + _ = req.Build() + out := CreateAccessKeyResponse{} + resp, err := executeRequestWith(srv, req.HTTPRequest, &out) + require.NoError(t, err) + require.Equal(t, http.StatusOK, resp.Code) + + assert.Equal(t, 0, counter.putCalled, "CreateAccessKey must not PutS3ApiConfiguration") + assert.Equal(t, 0, rec.saves) + assert.Equal(t, 1, rec.createAccessKeys) + + ident, err := cm.GetUser(ctx, "alice") + require.NoError(t, err) + require.Len(t, ident.Credentials, 1) + assert.Equal(t, *out.CreateAccessKeyResult.AccessKey.AccessKeyId, ident.Credentials[0].AccessKey) +} + +// CreateAccessKey for a user the store does not know still needs the full save: +// the handler creates the identity in the config, so there is no existing +// record to write a key into. +func TestIamApiCreateAccessKeyUnknownUserFallsBack(t *testing.T) { + srv, cm, counter, _ := targetedIamApiServer() + ctx := context.Background() + + req, _ := iam.New(session.New()).CreateAccessKeyRequest(&iam.CreateAccessKeyInput{UserName: aws.String("ghost")}) + _ = req.Build() + out := CreateAccessKeyResponse{} + resp, err := executeRequestWith(srv, req.HTTPRequest, &out) + require.NoError(t, err) + require.Equal(t, http.StatusOK, resp.Code) + + assert.Equal(t, 1, counter.putCalled) + ident, err := cm.GetUser(ctx, "ghost") + require.NoError(t, err) + assert.Len(t, ident.Credentials, 1) +} + +func TestIamApiDeleteAccessKeyTargeted(t *testing.T) { + srv, cm, counter, rec := targetedIamApiServer() + ctx := context.Background() + require.NoError(t, cm.CreateUser(ctx, &iam_pb.Identity{ + Name: "alice", + Credentials: []*iam_pb.Credential{{AccessKey: "AKIATESTFAKEKEY00001", SecretKey: "testsecretfake"}}, + })) + + req, _ := iam.New(session.New()).DeleteAccessKeyRequest(&iam.DeleteAccessKeyInput{ + UserName: aws.String("alice"), + AccessKeyId: aws.String("AKIATESTFAKEKEY00001"), + }) + _ = req.Build() + out := DeleteAccessKeyResponse{} + resp, err := executeRequestWith(srv, req.HTTPRequest, &out) + require.NoError(t, err) + require.Equal(t, http.StatusOK, resp.Code) + + assert.Equal(t, 0, counter.putCalled, "DeleteAccessKey must not PutS3ApiConfiguration") + assert.Equal(t, 1, rec.deleteAccessKeys) + + ident, err := cm.GetUser(ctx, "alice") + require.NoError(t, err) + assert.Empty(t, ident.Credentials) + + // Deleting a key that does not exist still answers 200 and stays targeted. + req, _ = iam.New(session.New()).DeleteAccessKeyRequest(&iam.DeleteAccessKeyInput{ + UserName: aws.String("alice"), + AccessKeyId: aws.String("AKIATESTFAKEKEY99999"), + }) + _ = req.Build() + resp, err = executeRequestWith(srv, req.HTTPRequest, &out) + require.NoError(t, err) + require.Equal(t, http.StatusOK, resp.Code) + assert.Equal(t, 0, counter.putCalled) +} + +func TestIamApiUpdateAccessKeyTargeted(t *testing.T) { + srv, cm, counter, rec := targetedIamApiServer() + ctx := context.Background() + require.NoError(t, cm.CreateUser(ctx, &iam_pb.Identity{ + Name: "alice", + Credentials: []*iam_pb.Credential{{AccessKey: "AKIATESTFAKEKEY00001", SecretKey: "testsecretfake", Status: "Active"}}, + })) + + req, _ := iam.New(session.New()).UpdateAccessKeyRequest(&iam.UpdateAccessKeyInput{ + UserName: aws.String("alice"), + AccessKeyId: aws.String("AKIATESTFAKEKEY00001"), + Status: aws.String(iam.StatusTypeInactive), + }) + _ = req.Build() + out := UpdateAccessKeyResponse{} + resp, err := executeRequestWith(srv, req.HTTPRequest, &out) + require.NoError(t, err) + require.Equal(t, http.StatusOK, resp.Code) + + assert.Equal(t, 0, counter.putCalled, "UpdateAccessKey must not PutS3ApiConfiguration") + assert.Equal(t, 1, rec.updateUsers) + + ident, err := cm.GetUser(ctx, "alice") + require.NoError(t, err) + require.Len(t, ident.Credentials, 1) + assert.Equal(t, iam.StatusTypeInactive, ident.Credentials[0].Status) +} + +func TestIamApiPutUserPolicyTargeted(t *testing.T) { + srv, cm, counter, rec := targetedIamApiServer() + ctx := context.Background() + require.NoError(t, cm.CreateUser(ctx, &iam_pb.Identity{Name: "alice"})) + require.NoError(t, cm.CreateUser(ctx, &iam_pb.Identity{Name: "bob"})) + + req, _ := iam.New(session.New()).PutUserPolicyRequest(&iam.PutUserPolicyInput{ + UserName: aws.String("alice"), + PolicyName: aws.String("read-bench"), + PolicyDocument: aws.String(targetedTestPolicy), + }) + _ = req.Build() + out := PutUserPolicyResponse{} + resp, err := executeRequestWith(srv, req.HTTPRequest, &out) + require.NoError(t, err) + require.Equal(t, http.StatusOK, resp.Code) + + assert.Equal(t, 0, counter.putCalled, "PutUserPolicy must not PutS3ApiConfiguration") + assert.Equal(t, 0, rec.saves) + assert.Equal(t, 1, rec.updateUsers) + + ident, err := cm.GetUser(ctx, "alice") + require.NoError(t, err) + assert.NotEmpty(t, ident.Actions, "recomputed Actions must be persisted for the one identity") +} + +func TestIamApiDeleteUserPolicyTargeted(t *testing.T) { + srv, cm, counter, rec := targetedIamApiServer() + ctx := context.Background() + require.NoError(t, cm.CreateUser(ctx, &iam_pb.Identity{Name: "alice"})) + + putReq, _ := iam.New(session.New()).PutUserPolicyRequest(&iam.PutUserPolicyInput{ + UserName: aws.String("alice"), + PolicyName: aws.String("read-bench"), + PolicyDocument: aws.String(targetedTestPolicy), + }) + _ = putReq.Build() + putOut := PutUserPolicyResponse{} + resp, err := executeRequestWith(srv, putReq.HTTPRequest, &putOut) + require.NoError(t, err) + require.Equal(t, http.StatusOK, resp.Code) + + delReq, _ := iam.New(session.New()).DeleteUserPolicyRequest(&iam.DeleteUserPolicyInput{ + UserName: aws.String("alice"), + PolicyName: aws.String("read-bench"), + }) + _ = delReq.Build() + delOut := DeleteUserPolicyResponse{} + resp, err = executeRequestWith(srv, delReq.HTTPRequest, &delOut) + require.NoError(t, err) + require.Equal(t, http.StatusOK, resp.Code) + + assert.Equal(t, 0, counter.putCalled, "DeleteUserPolicy must not PutS3ApiConfiguration") + assert.Equal(t, 2, rec.updateUsers) + + policies := Policies{} + require.NoError(t, srv.s3ApiConfig.GetPolicies(&policies)) + assert.Empty(t, policies.InlinePolicies["alice"]) +} + +func TestIamApiUpdateUserRenameTargeted(t *testing.T) { + srv, cm, counter, _ := targetedIamApiServer() + ctx := context.Background() + require.NoError(t, cm.CreateUser(ctx, &iam_pb.Identity{Name: "alice"})) + require.NoError(t, cm.CreateUser(ctx, &iam_pb.Identity{Name: "bob"})) + require.NoError(t, cm.CreateGroup(ctx, &iam_pb.Group{Name: "devs", Members: []string{"alice", "bob"}})) + + req, _ := iam.New(session.New()).UpdateUserRequest(&iam.UpdateUserInput{ + UserName: aws.String("alice"), + NewUserName: aws.String("alice-renamed"), + }) + _ = req.Build() + out := UpdateUserResponse{} + resp, err := executeRequestWith(srv, req.HTTPRequest, &out) + require.NoError(t, err) + require.Equal(t, http.StatusOK, resp.Code) + + assert.Equal(t, 0, counter.putCalled, "UpdateUser rename must not PutS3ApiConfiguration") + + _, err = cm.GetUser(ctx, "alice") + require.ErrorIs(t, err, credential.ErrUserNotFound) + _, err = cm.GetUser(ctx, "alice-renamed") + require.NoError(t, err) + + g, err := cm.GetGroup(ctx, "devs") + require.NoError(t, err) + assert.ElementsMatch(t, []string{"alice-renamed", "bob"}, g.Members) +} + +func TestIamApiDeleteUserTargeted(t *testing.T) { + srv, cm, counter, rec := targetedIamApiServer() + ctx := context.Background() + require.NoError(t, cm.CreateUser(ctx, &iam_pb.Identity{Name: "alice"})) + require.NoError(t, cm.CreateUser(ctx, &iam_pb.Identity{Name: "bob"})) + require.NoError(t, cm.CreateGroup(ctx, &iam_pb.Group{Name: "devs", Members: []string{"alice", "bob"}})) + + req, _ := iam.New(session.New()).DeleteUserRequest(&iam.DeleteUserInput{UserName: aws.String("alice")}) + _ = req.Build() + out := DeleteUserResponse{} + resp, err := executeRequestWith(srv, req.HTTPRequest, &out) + require.NoError(t, err) + require.Equal(t, http.StatusOK, resp.Code) + + assert.Equal(t, 0, counter.putCalled, "DeleteUser must not PutS3ApiConfiguration") + assert.Equal(t, 1, rec.deleteUsers) + assert.Equal(t, 1, rec.updateGroups) + + _, err = cm.GetUser(ctx, "alice") + require.ErrorIs(t, err, credential.ErrUserNotFound) + g, err := cm.GetGroup(ctx, "devs") + require.NoError(t, err) + assert.ElementsMatch(t, []string{"bob"}, g.Members) + _, err = cm.GetUser(ctx, "bob") + require.NoError(t, err, "other identities must be untouched") +} diff --git a/weed/iamapi/iamapi_test.go b/weed/iamapi/iamapi_test.go index e44c0832d2..893e907008 100644 --- a/weed/iamapi/iamapi_test.go +++ b/weed/iamapi/iamapi_test.go @@ -17,6 +17,7 @@ import ( "github.com/jinzhu/copier" "github.com/seaweedfs/seaweedfs/weed/credential" "github.com/seaweedfs/seaweedfs/weed/credential/memory" + "github.com/seaweedfs/seaweedfs/weed/pb/filer_pb" "github.com/seaweedfs/seaweedfs/weed/pb/iam_pb" "github.com/seaweedfs/seaweedfs/weed/s3api" "github.com/seaweedfs/seaweedfs/weed/s3api/policy_engine" @@ -607,6 +608,7 @@ func iamApiServerWithCredentialManager() (*IamApiServer, *credential.CredentialM type countingConfigSaver struct { cm *credential.CredentialManager putCalled int + policies Policies } func (c *countingConfigSaver) GetS3ApiConfiguration(s3cfg *iam_pb.S3ApiConfiguration) error { @@ -616,6 +618,8 @@ func (c *countingConfigSaver) GetS3ApiConfiguration(s3cfg *iam_pb.S3ApiConfigura } s3cfg.Identities = config.Identities s3cfg.Policies = config.Policies + s3cfg.Groups = config.Groups + s3cfg.ServiceAccounts = config.ServiceAccounts return nil } @@ -625,10 +629,15 @@ func (c *countingConfigSaver) PutS3ApiConfiguration(s3cfg *iam_pb.S3ApiConfigura } func (c *countingConfigSaver) GetPolicies(policies *Policies) error { + *policies = c.policies + if c.policies.Policies == nil && c.policies.InlinePolicies == nil { + return filer_pb.ErrNotFound + } return nil } func (c *countingConfigSaver) PutPolicies(policies *Policies) error { + c.policies = *policies return nil } diff --git a/weed/s3api/s3api_embedded_iam.go b/weed/s3api/s3api_embedded_iam.go index d364176a03..46cfddc3a6 100644 --- a/weed/s3api/s3api_embedded_iam.go +++ b/weed/s3api/s3api_embedded_iam.go @@ -67,6 +67,136 @@ func (e *EmbeddedIamApi) refreshIAMConfiguration() error { return nil } +// canPersistIdentity reports whether a change to the named identity can be written through the +// credential manager's single-identity methods instead of SaveConfiguration. It is false when +// there is no credential manager, when the caller asked for no persistent write (skipPersist), +// and for static identities, which live outside the store: those keep the full-save path. +func (e *EmbeddedIamApi) canPersistIdentity(skipPersist bool, userName string) bool { + return e.credentialManager != nil && !skipPersist && !e.credentialManager.IsStaticIdentity(userName) +} + +// groupsWithMember returns the groups in s3cfg that list userName as a member. Taken before a +// handler renames or removes the user, it names the group records that handler will change. +func groupsWithMember(s3cfg *iam_pb.S3ApiConfiguration, userName string) []*iam_pb.Group { + var groups []*iam_pb.Group + for _, g := range s3cfg.Groups { + for _, m := range g.Members { + if m == userName { + groups = append(groups, g) + break + } + } + } + return groups +} + +// serviceAccountsOf returns the service accounts in s3cfg whose parent is userName. +func serviceAccountsOf(s3cfg *iam_pb.S3ApiConfiguration, userName string) []*iam_pb.ServiceAccount { + var accounts []*iam_pb.ServiceAccount + for _, sa := range s3cfg.ServiceAccounts { + if sa.ParentUser == userName { + accounts = append(accounts, sa) + } + } + return accounts +} + +// persistIdentity writes the one identity a handler changed in s3cfg through the credential +// manager. The store writes that identity's record and propagates it alone; the other +// identities are not rewritten. +func (e *EmbeddedIamApi) persistIdentity(ctx context.Context, s3cfg *iam_pb.S3ApiConfiguration, userName string) error { + ident := findIdentityByName(s3cfg, userName) + if ident == nil { + return credential.ErrUserNotFound + } + return e.credentialManager.UpdateUser(ctx, userName, ident) +} + +// persistRenamedIdentity writes a user renamed from oldName to newName by UpdateUser through the +// credential manager: the identity, the groups that listed the old name and the service accounts +// it parented. Where the store has RenameUser the handler already moved the record and its inline +// policies, and the identity is written under the new name. Otherwise UpdateUser under the old +// name moves the record in stores that treat a changed Name as a rename (filer_etc, memory), the +// propagating store propagates the new identity and the old name's removal, and the inline policy +// copies the handler left under the old name are removed. A store that did neither still holds +// the user under the old name; the caller is told so (false) and keeps the full save, which is +// what renames such a store today. +func (e *EmbeddedIamApi) persistRenamedIdentity(ctx context.Context, s3cfg *iam_pb.S3ApiConfiguration, oldName, newName string, groups []*iam_pb.Group, accounts []*iam_pb.ServiceAccount) (bool, error) { + ident := findIdentityByName(s3cfg, newName) + if ident == nil { + return false, credential.ErrUserNotFound + } + storedAs := oldName + if _, err := e.credentialManager.GetUser(ctx, newName); err == nil { + storedAs = newName + } + if err := e.credentialManager.UpdateUser(ctx, storedAs, ident); err != nil { + return false, err + } + if storedAs == oldName { + if _, err := e.credentialManager.GetUser(ctx, newName); err != nil { + return false, nil + } + policyNames, err := e.credentialManager.ListUserInlinePolicies(ctx, oldName) + if err != nil { + return false, fmt.Errorf("list inline policies for %s: %w", oldName, err) + } + for _, policyName := range policyNames { + if err := e.credentialManager.DeleteUserInlinePolicy(ctx, oldName, policyName); err != nil { + return false, fmt.Errorf("remove inline policy %s from %s: %w", policyName, oldName, err) + } + } + } + for _, g := range groups { + if err := e.credentialManager.UpdateGroup(ctx, g); err != nil { + return false, fmt.Errorf("update group %s: %w", g.Name, err) + } + } + for _, sa := range accounts { + if err := e.credentialManager.UpdateServiceAccount(ctx, sa.Id, sa); err != nil { + return false, fmt.Errorf("update service account %s: %w", sa.Id, err) + } + } + return true, nil +} + +// deleteIdentity removes a user deleted by DeleteUser through the credential manager: its inline +// policies, its record (the store propagates the one removal) and the groups that listed it, +// whose membership DeleteUser already changed in s3cfg. +func (e *EmbeddedIamApi) deleteIdentity(ctx context.Context, userName string, groups []*iam_pb.Group) error { + policyNames, err := e.credentialManager.ListUserInlinePolicies(ctx, userName) + if err != nil { + return fmt.Errorf("list inline policies for %s: %w", userName, err) + } + for _, policyName := range policyNames { + if err := e.credentialManager.DeleteUserInlinePolicy(ctx, userName, policyName); err != nil { + return fmt.Errorf("remove inline policy %s from %s: %w", policyName, userName, err) + } + } + if err := e.credentialManager.DeleteUser(ctx, userName); err != nil { + return err + } + for _, g := range groups { + if err := e.credentialManager.UpdateGroup(ctx, g); err != nil { + return fmt.Errorf("update group %s: %w", g.Name, err) + } + } + return nil +} + +// reloadsAfterTargetedWrite names the actions that persist through the credential manager +// rather than SaveConfiguration; the in-memory identity maps are refreshed after them the way +// they are after a full save. +func reloadsAfterTargetedWrite(action string) bool { + switch action { + case "AttachUserPolicy", "DetachUserPolicy", "CreatePolicy", "CreatePolicyVersion", "DeletePolicy", + "CreateUser", "UpdateUser", "DeleteUser", "CreateAccessKey", "DeleteAccessKey", "UpdateAccessKey", + "SetUserStatus", "PutUserPolicy", "DeleteUserPolicy", "PutGroupPolicy", "DeleteGroupPolicy": + return true + } + return false +} + // Constants for service account identifiers const ( ServiceAccountIDLength = 12 // Length of the service account ID @@ -2735,27 +2865,79 @@ func (e *EmbeddedIamApi) ExecuteAction(ctx context.Context, values url.Values, s } changed = false case "UpdateUser": + userName, newUserName := values.Get("UserName"), values.Get("NewUserName") + // Taken before the rename: the records the handler changes besides the identity. + groups := groupsWithMember(s3cfg, userName) + accounts := serviceAccountsOf(s3cfg, userName) var iamErr *iamError response, iamErr = e.UpdateUser(ctx, s3cfg, values) if iamErr != nil { return nil, iamErr } + // Write the renamed identity, its groups and service accounts through the credential + // manager instead of rewriting every identity via SaveConfiguration. A request that + // renames nothing changed nothing, so there is nothing to write either way. + if e.canPersistIdentity(skipPersist, userName) { + renamed := true + if newUserName != "" && newUserName != userName { + var err error + renamed, err = e.persistRenamedIdentity(ctx, s3cfg, userName, newUserName, groups, accounts) + if err != nil { + return nil, &iamError{Code: CredentialErrToIamErrCode(err), Error: err} + } + } + changed = !renamed + } case "DeleteUser": userName := values.Get("UserName") + groups := groupsWithMember(s3cfg, userName) var iamErr *iamError response, iamErr = e.DeleteUser(s3cfg, userName) if iamErr != nil { return nil, iamErr } + // Targeted delete: one record removed and one removal propagated, plus the groups whose + // membership changed, instead of rewriting every remaining identity. + if e.canPersistIdentity(skipPersist, userName) { + if err := e.deleteIdentity(ctx, userName, groups); err != nil { + return nil, &iamError{Code: CredentialErrToIamErrCode(err), Error: err} + } + changed = false + } case "CreateAccessKey": - var iamErr *iamError - response, iamErr = e.CreateAccessKey(s3cfg, values) + userName := values.Get("UserName") + resp, iamErr := e.CreateAccessKey(s3cfg, values) if iamErr != nil { glog.Errorf("CreateAccessKey: %+v", iamErr.Error) return nil, iamErr } + response = resp + // Targeted write: credentialManager.CreateAccessKey adds the key to the one identity + // and propagates that identity alone. + if e.canPersistIdentity(skipPersist, userName) { + cred := &iam_pb.Credential{ + AccessKey: *resp.CreateAccessKeyResult.AccessKey.AccessKeyId, + SecretKey: *resp.CreateAccessKeyResult.AccessKey.SecretAccessKey, + Status: iamAccessKeyStatusActive, + } + if err := e.credentialManager.CreateAccessKey(ctx, userName, cred); err != nil { + return nil, &iamError{Code: CredentialErrToIamErrCode(err), Error: err} + } + changed = false + } case "DeleteAccessKey": + userName, accessKeyId := values.Get("UserName"), values.Get("AccessKeyId") response = e.DeleteAccessKey(s3cfg, values) + // Targeted write: credentialManager.DeleteAccessKey removes the key from the one identity + // and propagates that identity alone. The handler answers 200 for a key or user it does + // not know, so a not-found from the store is the same no-op it always was. + if e.canPersistIdentity(skipPersist, userName) { + err := e.credentialManager.DeleteAccessKey(ctx, userName, accessKeyId) + if err != nil && !errors.Is(err, credential.ErrAccessKeyNotFound) && !errors.Is(err, credential.ErrUserNotFound) { + return nil, &iamError{Code: CredentialErrToIamErrCode(err), Error: err} + } + changed = false + } case "CreatePolicy": var iamErr *iamError response, iamErr = e.CreatePolicy(ctx, values) @@ -2773,12 +2955,21 @@ func (e *EmbeddedIamApi) ExecuteAction(ctx context.Context, values url.Values, s } changed = false case "PutUserPolicy": + userName := values.Get("UserName") var iamErr *iamError response, iamErr = e.PutUserPolicy(s3cfg, values) if iamErr != nil { glog.Errorf("PutUserPolicy: %+v", iamErr.Error) return nil, iamErr } + // The policy document is already stored; what remains is the identity's recomputed + // Actions, written and propagated for that identity alone. + if e.canPersistIdentity(skipPersist, userName) { + if err := e.persistIdentity(ctx, s3cfg, userName); err != nil { + return nil, &iamError{Code: CredentialErrToIamErrCode(err), Error: err} + } + changed = false + } case "GetUserPolicy": var iamErr *iamError response, iamErr = e.GetUserPolicy(s3cfg, values) @@ -2787,11 +2978,18 @@ func (e *EmbeddedIamApi) ExecuteAction(ctx context.Context, values url.Values, s } changed = false case "DeleteUserPolicy": + userName := values.Get("UserName") var iamErr *iamError response, iamErr = e.DeleteUserPolicy(s3cfg, values) if iamErr != nil { return nil, iamErr } + if e.canPersistIdentity(skipPersist, userName) { + if err := e.persistIdentity(ctx, s3cfg, userName); err != nil { + return nil, &iamError{Code: CredentialErrToIamErrCode(err), Error: err} + } + changed = false + } case "ListUserPolicies": var iamErr *iamError response, iamErr = e.ListUserPolicies(s3cfg, values) @@ -2883,17 +3081,31 @@ func (e *EmbeddedIamApi) ExecuteAction(ctx context.Context, values url.Values, s } changed = false case "SetUserStatus": + userName := values.Get("UserName") var iamErr *iamError response, iamErr = e.SetUserStatus(s3cfg, values) if iamErr != nil { return nil, iamErr } + if e.canPersistIdentity(skipPersist, userName) { + if err := e.persistIdentity(ctx, s3cfg, userName); err != nil { + return nil, &iamError{Code: CredentialErrToIamErrCode(err), Error: err} + } + changed = false + } case "UpdateAccessKey": + userName := values.Get("UserName") var iamErr *iamError response, iamErr = e.UpdateAccessKey(s3cfg, values) if iamErr != nil { return nil, iamErr } + if e.canPersistIdentity(skipPersist, userName) { + if err := e.persistIdentity(ctx, s3cfg, userName); err != nil { + return nil, &iamError{Code: CredentialErrToIamErrCode(err), Error: err} + } + changed = false + } // Service Account actions case "CreateServiceAccount": createdBy := values.Get("CreatedBy") @@ -3034,7 +3246,7 @@ func (e *EmbeddedIamApi) ExecuteAction(ctx context.Context, values url.Values, s glog.Errorf("Failed to reload IAM configuration after mutation: %v", err) // Don't fail the request since the persistent save succeeded } - } else if action == "AttachUserPolicy" || action == "DetachUserPolicy" || action == "CreatePolicy" || action == "CreatePolicyVersion" || action == "DeletePolicy" || action == "CreateUser" || action == "PutGroupPolicy" || action == "DeleteGroupPolicy" { + } else if reloadsAfterTargetedWrite(action) { // Even if changed=false (persisted via credentialManager), we should still reload // if we are utilizing the local in-memory cache for speed if err := e.ReloadConfiguration(); err != nil { diff --git a/weed/s3api/s3api_embedded_iam_targeted_test.go b/weed/s3api/s3api_embedded_iam_targeted_test.go new file mode 100644 index 0000000000..a13d7e4ca3 --- /dev/null +++ b/weed/s3api/s3api_embedded_iam_targeted_test.go @@ -0,0 +1,362 @@ +package s3api + +// Targeted-write regression tests: single-identity IAM actions must write and +// propagate only the identity they changed, not every identity via +// SaveConfiguration. See https://github.com/seaweedfs/seaweedfs/issues/11865. + +import ( + "context" + "encoding/json" + "net/url" + "testing" + + "github.com/seaweedfs/seaweedfs/weed/credential" + "github.com/seaweedfs/seaweedfs/weed/credential/memory" + "github.com/seaweedfs/seaweedfs/weed/pb/iam_pb" + "github.com/seaweedfs/seaweedfs/weed/s3api/policy_engine" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// recordingStore wraps a memory store and counts the write methods the +// dispatcher used, so a test can assert the single-identity path was taken and +// SaveConfiguration was not. It embeds the concrete store (rather than the +// CredentialStore interface) so optional interfaces like InlinePolicyStore and +// UserRenamer stay visible through the wrapper. +type recordingStore struct { + *memory.MemoryStore + saves int + createAccessKeys int + deleteAccessKeys int + updateUsers int + deleteUsers int + updateGroups int +} + +func (r *recordingStore) SaveConfiguration(ctx context.Context, config *iam_pb.S3ApiConfiguration) error { + r.saves++ + return r.MemoryStore.SaveConfiguration(ctx, config) +} + +func (r *recordingStore) CreateAccessKey(ctx context.Context, username string, cred *iam_pb.Credential) error { + r.createAccessKeys++ + return r.MemoryStore.CreateAccessKey(ctx, username, cred) +} + +func (r *recordingStore) DeleteAccessKey(ctx context.Context, username string, accessKey string) error { + r.deleteAccessKeys++ + return r.MemoryStore.DeleteAccessKey(ctx, username, accessKey) +} + +func (r *recordingStore) UpdateUser(ctx context.Context, username string, identity *iam_pb.Identity) error { + r.updateUsers++ + return r.MemoryStore.UpdateUser(ctx, username, identity) +} + +func (r *recordingStore) DeleteUser(ctx context.Context, username string) error { + r.deleteUsers++ + return r.MemoryStore.DeleteUser(ctx, username) +} + +func (r *recordingStore) UpdateGroup(ctx context.Context, group *iam_pb.Group) error { + r.updateGroups++ + return r.MemoryStore.UpdateGroup(ctx, group) +} + +// newTargetedWriteTestApi seeds cfg into the store and wraps the store in a +// recordingStore. The initial sync save happens before the recorder is +// installed so it does not count. +func newTargetedWriteTestApi(t *testing.T, cfg *iam_pb.S3ApiConfiguration) (*EmbeddedIamApiForTest, *recordingStore) { + t.Helper() + api := NewEmbeddedIamApiForTest() + api.mockConfig = cfg + // Trigger the one-time mockConfig -> store seed through a read action. + _, iamErr := api.ExecuteAction(context.Background(), url.Values{"Action": {"ListUsers"}}, false, "") + require.Nil(t, iamErr) + rec := &recordingStore{MemoryStore: api.credentialManager.Store.(*memory.MemoryStore)} + api.credentialManager.Store = rec + return api, rec +} + +func executeAction(t *testing.T, api *EmbeddedIamApiForTest, vals url.Values) { + t.Helper() + _, iamErr := api.ExecuteAction(context.Background(), vals, false, "") + require.Nil(t, iamErr) +} + +func TestEmbeddedIamCreateAccessKeyTargeted(t *testing.T) { + api, rec := newTargetedWriteTestApi(t, &iam_pb.S3ApiConfiguration{ + Identities: []*iam_pb.Identity{{Name: "alice"}, {Name: "bob"}}, + }) + ctx := context.Background() + + resp, iamErr := api.ExecuteAction(ctx, url.Values{ + "Action": {"CreateAccessKey"}, + "UserName": {"alice"}, + }, false, "") + require.Nil(t, iamErr) + require.Equal(t, 0, rec.saves, "CreateAccessKey must not SaveConfiguration") + require.Equal(t, 1, rec.createAccessKeys) + + out := resp.(*iamCreateAccessKeyResponse) + ident, err := api.credentialManager.GetUser(ctx, "alice") + require.NoError(t, err) + require.Len(t, ident.Credentials, 1) + assert.Equal(t, *out.CreateAccessKeyResult.AccessKey.AccessKeyId, ident.Credentials[0].AccessKey) + + // The new key resolves in the store. + byKey, err := api.credentialManager.GetUserByAccessKey(ctx, *out.CreateAccessKeyResult.AccessKey.AccessKeyId) + require.NoError(t, err) + assert.Equal(t, "alice", byKey.Name) +} + +func TestEmbeddedIamDeleteAccessKeyTargeted(t *testing.T) { + api, rec := newTargetedWriteTestApi(t, &iam_pb.S3ApiConfiguration{ + Identities: []*iam_pb.Identity{{ + Name: "alice", + Credentials: []*iam_pb.Credential{{AccessKey: "AKIATESTFAKEKEY00001", SecretKey: "testsecretfake"}}, + }}, + }) + ctx := context.Background() + + executeAction(t, api, url.Values{ + "Action": {"DeleteAccessKey"}, + "UserName": {"alice"}, + "AccessKeyId": {"AKIATESTFAKEKEY00001"}, + }) + require.Equal(t, 0, rec.saves, "DeleteAccessKey must not SaveConfiguration") + require.Equal(t, 1, rec.deleteAccessKeys) + + ident, err := api.credentialManager.GetUser(ctx, "alice") + require.NoError(t, err) + assert.Empty(t, ident.Credentials) + _, err = api.credentialManager.GetUserByAccessKey(ctx, "AKIATESTFAKEKEY00001") + require.ErrorIs(t, err, credential.ErrAccessKeyNotFound) +} + +func TestEmbeddedIamUpdateAccessKeyTargeted(t *testing.T) { + api, rec := newTargetedWriteTestApi(t, &iam_pb.S3ApiConfiguration{ + Identities: []*iam_pb.Identity{{ + Name: "alice", + Credentials: []*iam_pb.Credential{ + {AccessKey: "AKIATESTFAKEKEY00001", SecretKey: "testsecretfake", Status: iamAccessKeyStatusActive}, + }, + }}, + }) + ctx := context.Background() + + executeAction(t, api, url.Values{ + "Action": {"UpdateAccessKey"}, + "UserName": {"alice"}, + "AccessKeyId": {"AKIATESTFAKEKEY00001"}, + "Status": {iamAccessKeyStatusInactive}, + }) + require.Equal(t, 0, rec.saves, "UpdateAccessKey must not SaveConfiguration") + require.Equal(t, 1, rec.updateUsers) + + ident, err := api.credentialManager.GetUser(ctx, "alice") + require.NoError(t, err) + require.Len(t, ident.Credentials, 1) + assert.Equal(t, iamAccessKeyStatusInactive, ident.Credentials[0].Status) +} + +const iamTargetedTestPolicy = `{ + "Version": "2012-10-17", + "Statement": [{"Effect": "Allow", "Action": ["s3:GetObject"], "Resource": ["arn:aws:s3:::bench/*"]}] +}` + +func TestEmbeddedIamPutUserPolicyTargeted(t *testing.T) { + api, rec := newTargetedWriteTestApi(t, &iam_pb.S3ApiConfiguration{ + Identities: []*iam_pb.Identity{{Name: "alice"}, {Name: "bob"}}, + }) + ctx := context.Background() + + executeAction(t, api, url.Values{ + "Action": {"PutUserPolicy"}, + "UserName": {"alice"}, + "PolicyName": {"read-bench"}, + "PolicyDocument": {iamTargetedTestPolicy}, + }) + require.Equal(t, 0, rec.saves, "PutUserPolicy must not SaveConfiguration") + require.Equal(t, 1, rec.updateUsers) + + ident, err := api.credentialManager.GetUser(ctx, "alice") + require.NoError(t, err) + assert.NotEmpty(t, ident.Actions, "identity Actions must be recomputed and persisted") + + doc, err := api.credentialManager.GetUserInlinePolicy(ctx, "alice", "read-bench") + require.NoError(t, err) + require.NotNil(t, doc) +} + +func TestEmbeddedIamDeleteUserPolicyTargeted(t *testing.T) { + api, rec := newTargetedWriteTestApi(t, &iam_pb.S3ApiConfiguration{ + Identities: []*iam_pb.Identity{{Name: "alice"}}, + }) + ctx := context.Background() + + executeAction(t, api, url.Values{ + "Action": {"PutUserPolicy"}, + "UserName": {"alice"}, + "PolicyName": {"read-bench"}, + "PolicyDocument": {iamTargetedTestPolicy}, + }) + require.Equal(t, 1, rec.updateUsers) + + executeAction(t, api, url.Values{ + "Action": {"DeleteUserPolicy"}, + "UserName": {"alice"}, + "PolicyName": {"read-bench"}, + }) + require.Equal(t, 0, rec.saves, "DeleteUserPolicy must not SaveConfiguration") + require.Equal(t, 2, rec.updateUsers) + + policies, err := api.credentialManager.ListUserInlinePolicies(ctx, "alice") + require.NoError(t, err) + assert.Empty(t, policies) + ident, err := api.credentialManager.GetUser(ctx, "alice") + require.NoError(t, err) + assert.Empty(t, ident.Actions) +} + +func TestEmbeddedIamSetUserStatusTargeted(t *testing.T) { + api, rec := newTargetedWriteTestApi(t, &iam_pb.S3ApiConfiguration{ + Identities: []*iam_pb.Identity{{Name: "alice"}}, + }) + ctx := context.Background() + + executeAction(t, api, url.Values{ + "Action": {"SetUserStatus"}, + "UserName": {"alice"}, + "Status": {iamAccessKeyStatusInactive}, + }) + require.Equal(t, 0, rec.saves, "SetUserStatus must not SaveConfiguration") + require.Equal(t, 1, rec.updateUsers) + + ident, err := api.credentialManager.GetUser(ctx, "alice") + require.NoError(t, err) + assert.True(t, ident.Disabled) +} + +func TestEmbeddedIamUpdateUserRenameTargeted(t *testing.T) { + api, rec := newTargetedWriteTestApi(t, &iam_pb.S3ApiConfiguration{ + Identities: []*iam_pb.Identity{{Name: "alice"}, {Name: "bob"}}, + Groups: []*iam_pb.Group{ + {Name: "devs", Members: []string{"alice", "bob"}}, + {Name: "ops", Members: []string{"bob"}}, + }, + }) + ctx := context.Background() + // Groups in the seed config must also be created in the store directly: + // the memory store only surfaces groups written via CreateGroup. + require.NoError(t, api.credentialManager.CreateGroup(ctx, &iam_pb.Group{Name: "devs", Members: []string{"alice", "bob"}})) + require.NoError(t, api.credentialManager.CreateGroup(ctx, &iam_pb.Group{Name: "ops", Members: []string{"bob"}})) + require.NoError(t, api.credentialManager.PutUserInlinePolicy(ctx, "alice", "p1", mustParseIamTargetedTestPolicy(t))) + + executeAction(t, api, url.Values{ + "Action": {"UpdateUser"}, + "UserName": {"alice"}, + "NewUserName": {"alice-renamed"}, + }) + require.Equal(t, 0, rec.saves, "UpdateUser rename must not SaveConfiguration") + + _, err := api.credentialManager.GetUser(ctx, "alice") + require.ErrorIs(t, err, credential.ErrUserNotFound) + _, err = api.credentialManager.GetUser(ctx, "alice-renamed") + require.NoError(t, err) + + // Group membership and inline policies follow the rename. + g, err := api.credentialManager.GetGroup(ctx, "devs") + require.NoError(t, err) + assert.ElementsMatch(t, []string{"alice-renamed", "bob"}, g.Members) + g, err = api.credentialManager.GetGroup(ctx, "ops") + require.NoError(t, err) + assert.ElementsMatch(t, []string{"bob"}, g.Members) + + policies, err := api.credentialManager.ListUserInlinePolicies(ctx, "alice-renamed") + require.NoError(t, err) + assert.Equal(t, []string{"p1"}, policies) + policies, err = api.credentialManager.ListUserInlinePolicies(ctx, "alice") + require.NoError(t, err) + assert.Empty(t, policies) +} + +func TestEmbeddedIamDeleteUserTargeted(t *testing.T) { + api, rec := newTargetedWriteTestApi(t, &iam_pb.S3ApiConfiguration{ + Identities: []*iam_pb.Identity{{Name: "alice"}, {Name: "bob"}}, + Groups: []*iam_pb.Group{{Name: "devs", Members: []string{"alice", "bob"}}}, + }) + ctx := context.Background() + require.NoError(t, api.credentialManager.CreateGroup(ctx, &iam_pb.Group{Name: "devs", Members: []string{"alice", "bob"}})) + + executeAction(t, api, url.Values{ + "Action": {"DeleteUser"}, + "UserName": {"alice"}, + }) + require.Equal(t, 0, rec.saves, "DeleteUser must not SaveConfiguration") + require.Equal(t, 1, rec.deleteUsers) + require.Equal(t, 1, rec.updateGroups) + + _, err := api.credentialManager.GetUser(ctx, "alice") + require.ErrorIs(t, err, credential.ErrUserNotFound) + g, err := api.credentialManager.GetGroup(ctx, "devs") + require.NoError(t, err) + assert.ElementsMatch(t, []string{"bob"}, g.Members) + _, err = api.credentialManager.GetUser(ctx, "bob") + require.NoError(t, err, "other identities must be untouched") +} + +// TestEmbeddedIamTargetedSkipPersist: skipPersist must suppress the targeted +// write exactly like it suppresses the full save. +func TestEmbeddedIamTargetedSkipPersist(t *testing.T) { + api, rec := newTargetedWriteTestApi(t, &iam_pb.S3ApiConfiguration{ + Identities: []*iam_pb.Identity{{Name: "alice"}}, + }) + ctx := context.Background() + + for _, vals := range []url.Values{ + {"Action": {"CreateAccessKey"}, "UserName": {"alice"}}, + {"Action": {"SetUserStatus"}, "UserName": {"alice"}, "Status": {iamAccessKeyStatusInactive}}, + {"Action": {"PutUserPolicy"}, "UserName": {"alice"}, "PolicyName": {"p"}, "PolicyDocument": {iamTargetedTestPolicy}}, + } { + _, iamErr := api.ExecuteAction(ctx, vals, true /*skipPersist*/, "") + require.Nil(t, iamErr, "action %v", vals.Get("Action")) + } + assert.Equal(t, 0, rec.saves) + assert.Equal(t, 0, rec.createAccessKeys) + assert.Equal(t, 0, rec.updateUsers) + + ident, err := api.credentialManager.GetUser(ctx, "alice") + require.NoError(t, err) + assert.Empty(t, ident.Credentials) + assert.False(t, ident.Disabled) +} + +// TestEmbeddedIamStaticIdentityKeepsFullSave: static identities live outside +// the store, so a change to one still takes the full-save path. +func TestEmbeddedIamStaticIdentityKeepsFullSave(t *testing.T) { + api := NewEmbeddedIamApiForTest() + api.mockConfig = &iam_pb.S3ApiConfiguration{} + api.credentialManager.SetStaticIdentities([]*iam_pb.Identity{{Name: "static-admin"}}) + rec := &recordingStore{MemoryStore: api.credentialManager.Store.(*memory.MemoryStore)} + api.credentialManager.Store = rec + + // Seed via a read action; static identities are merged into the loaded config. + _, iamErr := api.ExecuteAction(context.Background(), url.Values{"Action": {"ListUsers"}}, false, "") + require.Nil(t, iamErr) + rec.saves = 0 + + executeAction(t, api, url.Values{ + "Action": {"SetUserStatus"}, + "UserName": {"static-admin"}, + "Status": {iamAccessKeyStatusInactive}, + }) + assert.Equal(t, 1, rec.saves, "static identity changes keep the full-save path") + assert.Equal(t, 0, rec.updateUsers) +} + +func mustParseIamTargetedTestPolicy(t *testing.T) (doc policy_engine.PolicyDocument) { + t.Helper() + require.NoError(t, json.Unmarshal([]byte(iamTargetedTestPolicy), &doc)) + return doc +}