From 1127672d101b5d139814b5a419ac20ff434d7596 Mon Sep 17 00:00:00 2001 From: Yoann Katchourine <810667+ykatchou@users.noreply.github.com> Date: Fri, 1 May 2026 10:14:15 +0200 Subject: [PATCH] fix(s3api,iamapi): avoid full SaveConfiguration when creating a single IAM user (#9261) * fix(s3api,iamapi): avoid full SaveConfiguration when creating a single IAM user When CreateUser was called, both the embedded IAM path (s3api_embedded_iam.go) and the standalone IAM path (iamapi_management_handlers.go) would: 1. Load the full config (all N users) 2. Append the new user 3. Call SaveConfiguration, which re-writes ALL N user files in the filer_etc store, triggering N file-change events and N reload cycles. The fix replaces the full-save path with credentialManager.CreateUser, which writes only the single new user's file. Set changed=false to skip the redundant SaveConfiguration call, and add "CreateUser" to the reload-after-targeted-write block so the in-memory config is refreshed. Also adds a nil guard around iama.iam.GetCredentialManager() in DoActions to avoid a nil-pointer panic in legacy test fixtures that leave iam unset. Add SetCredentialManagerForTest to IdentityAccessManagement so tests can inject an in-memory credential store without touching production code paths. * test(s3api,iamapi): regression tests for CreateUser targeted-write fix Add TestCreateUserDoesNotSaveAllUsers (iamapi) and TestEmbeddedIamCreateUserDoesNotSaveAllUsers (s3api) to guard against the regression where CreateUser would call SaveConfiguration and re-write all N existing user files. Both tests: - Pre-populate 3 existing users - Invoke CreateUser via the HTTP API - Assert PutS3ApiConfiguration (full-config save) was NOT called - Assert the new user is visible in the credential store - Assert all pre-existing users are still intact Also update TestEmbeddedIamExecuteAction to verify persistence via the credential manager directly (mockConfig is no longer updated on CreateUser since we skip the SaveConfiguration path). * refactor(s3api,iamapi): share credential-error to IAM-code mapping Move the credentialErrToIamErrCode helper from weed/iamapi to weed/s3api as exported CredentialErrToIamErrCode and call it from both the standalone IAM handler (iamapi) and the embedded IAM handler (s3api). Previously the standalone path used the helper while the embedded path duplicated the same switch inline; the two sites could drift out of sync. Also extend the mapping to cover ErrUserNotFound and ErrAccessKeyNotFound (404 NoSuchEntity) so non-CreateUser callers that opt into the helper get the right HTTP status. * test(s3api): seed credential store explicitly in CreateUser regression Previously the test relied on getS3ApiConfigurationFunc's syncOnce side effect to populate the credential store with mockConfig identities. If that fixture ever stopped seeding, the post-test "pre-existing users still exist" assertion would silently start passing for the wrong reason (the users were never created). Seed cm.SaveConfiguration directly and assert the seed precondition before the API call so any seeding regression fails loudly. * fix(s3api): honor skipPersist in embedded CreateUser targeted path ExecuteAction documents that skipPersist=true means "the changed configuration is not saved to the persistent store", but the targeted credentialManager.CreateUser added for the avoid-bulk-save fix ran unconditionally. Gate it on !skipPersist so no-persist callers don't silently leak a write to the credential store. Leave changed=true in the skipPersist branch so the tail's existing `if changed { if !skipPersist { ... } }` block keeps suppressing persistence the same way it does for every other action. Add TestEmbeddedIamCreateUserSkipPersist to pin the contract: a CreateUser invocation with skipPersist=true must not call PutS3ApiConfiguration and must not leave the user in the credential store. --------- Co-authored-by: Chris Lu --- weed/iamapi/iamapi_handlers.go | 2 +- weed/iamapi/iamapi_management_handlers.go | 13 +++ weed/iamapi/iamapi_test.go | 102 ++++++++++++++++++++++ weed/s3api/auth_credentials.go | 6 ++ weed/s3api/credential_iam_errors.go | 27 ++++++ weed/s3api/s3api_embedded_iam.go | 16 +++- weed/s3api/s3api_embedded_iam_test.go | 101 +++++++++++++++++++-- 7 files changed, 255 insertions(+), 12 deletions(-) create mode 100644 weed/s3api/credential_iam_errors.go diff --git a/weed/iamapi/iamapi_handlers.go b/weed/iamapi/iamapi_handlers.go index 1ab3c1282..695ce293d 100644 --- a/weed/iamapi/iamapi_handlers.go +++ b/weed/iamapi/iamapi_handlers.go @@ -37,7 +37,7 @@ func writeIamErrorResponse(w http.ResponseWriter, r *http.Request, reqID string, s3err.WriteXMLResponse(w, r, http.StatusNotFound, errorResp) case iam.ErrCodeMalformedPolicyDocumentException, iam.ErrCodeInvalidInputException: s3err.WriteXMLResponse(w, r, http.StatusBadRequest, errorResp) - case iam.ErrCodeDeleteConflictException: + case iam.ErrCodeDeleteConflictException, iam.ErrCodeEntityAlreadyExistsException: s3err.WriteXMLResponse(w, r, http.StatusConflict, errorResp) case iam.ErrCodeServiceFailureException: // We do not want to expose internal server error to the client diff --git a/weed/iamapi/iamapi_management_handlers.go b/weed/iamapi/iamapi_management_handlers.go index cee4cc048..4c6f0faf9 100644 --- a/weed/iamapi/iamapi_management_handlers.go +++ b/weed/iamapi/iamapi_management_handlers.go @@ -18,6 +18,7 @@ import ( iamlib "github.com/seaweedfs/seaweedfs/weed/iam" "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" "github.com/seaweedfs/seaweedfs/weed/s3api/s3err" "github.com/seaweedfs/seaweedfs/weed/util/request_id" @@ -1123,6 +1124,18 @@ func (iama *IamApiServer) DoActions(w http.ResponseWriter, r *http.Request) { changed = false case "CreateUser": response = iama.CreateUser(s3cfg, values) + // Use targeted create to avoid rewriting all existing user files via SaveConfiguration. + // credentialManager.CreateUser writes only the new user's file. + if iama.iam != nil { + if cm := iama.iam.GetCredentialManager(); cm != nil { + userName := values.Get("UserName") + if err := cm.CreateUser(r.Context(), &iam_pb.Identity{Name: userName}); err != nil { + writeIamErrorResponse(w, r, reqID, &IamError{Code: s3api.CredentialErrToIamErrCode(err), Error: err}) + return + } + changed = false + } + } case "GetUser": userName := values.Get("UserName") var err *IamError diff --git a/weed/iamapi/iamapi_test.go b/weed/iamapi/iamapi_test.go index 509fc5745..bedb0c073 100644 --- a/weed/iamapi/iamapi_test.go +++ b/weed/iamapi/iamapi_test.go @@ -1,6 +1,7 @@ package iamapi import ( + "context" "encoding/json" "encoding/xml" "net/http" @@ -14,11 +15,14 @@ import ( "github.com/aws/aws-sdk-go/service/iam" "github.com/gorilla/mux" "github.com/jinzhu/copier" + "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" "github.com/seaweedfs/seaweedfs/weed/s3api/policy_engine" "github.com/seaweedfs/seaweedfs/weed/util/request_id" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) var GetS3ApiConfiguration func(s3cfg *iam_pb.S3ApiConfiguration) (err error) @@ -385,3 +389,101 @@ func mustMarshalJSON(t *testing.T, v interface{}) []byte { } return data } + +// iamApiServerWithCredentialManager builds an IamApiServer backed by a real +// in-memory credential store. Used for tests that exercise the targeted +// credentialManager paths (e.g. CreateUser) instead of the legacy mock. +func iamApiServerWithCredentialManager() (*IamApiServer, *credential.CredentialManager, *countingConfigSaver) { + store := &memory.MemoryStore{} + store.Initialize(nil, "") + cm := &credential.CredentialManager{Store: store} + + counter := &countingConfigSaver{cm: cm} + iamInstance := &s3api.IdentityAccessManagement{} + // Wire up the credential manager so GetCredentialManager() returns it. + iamInstance.SetCredentialManagerForTest(cm) + + srv := &IamApiServer{ + s3ApiConfig: counter, + iam: iamInstance, + } + return srv, cm, counter +} + +// countingConfigSaver wraps the credential manager and counts full-config saves. +type countingConfigSaver struct { + cm *credential.CredentialManager + putCalled int +} + +func (c *countingConfigSaver) GetS3ApiConfiguration(s3cfg *iam_pb.S3ApiConfiguration) error { + config, err := c.cm.LoadConfiguration(context.Background()) + if err != nil { + return err + } + s3cfg.Identities = config.Identities + s3cfg.Policies = config.Policies + return nil +} + +func (c *countingConfigSaver) PutS3ApiConfiguration(s3cfg *iam_pb.S3ApiConfiguration) error { + c.putCalled++ + return c.cm.SaveConfiguration(context.Background(), s3cfg) +} + +func (c *countingConfigSaver) GetPolicies(policies *Policies) error { + return nil +} + +func (c *countingConfigSaver) PutPolicies(policies *Policies) error { + return nil +} + +func executeRequestWith(srv *IamApiServer, req *http.Request, v interface{}) (*httptest.ResponseRecorder, error) { + rr := httptest.NewRecorder() + apiRouter := mux.NewRouter().SkipClean(true) + apiRouter.Path("/").Methods(http.MethodPost).HandlerFunc(srv.DoActions) + apiRouter.ServeHTTP(rr, req) + return rr, xml.Unmarshal(rr.Body.Bytes(), &v) +} + +// TestCreateUserDoesNotSaveAllUsers is a regression test for the bug where +// creating a new user triggered a full SaveConfiguration over all existing +// identities (causing N file writes + reload cycles in the filer_etc store). +// The fix uses credentialManager.CreateUser for a targeted single-file write. +func TestCreateUserDoesNotSaveAllUsers(t *testing.T) { + srv, cm, counter := iamApiServerWithCredentialManager() + ctx := context.Background() + + // Pre-populate three existing users. + for _, name := range []string{"existing-1", "existing-2", "existing-3"} { + require.NoError(t, cm.CreateUser(ctx, &iam_pb.Identity{Name: name})) + } + + // Create a new user via the HTTP API. + params := &iam.CreateUserInput{UserName: aws.String("new-user")} + req, _ := iam.New(session.New()).CreateUserRequest(params) + _ = req.Build() + out := CreateUserResponse{} + resp, err := executeRequestWith(srv, req.HTTPRequest, &out) + require.NoError(t, err) + require.Equal(t, http.StatusOK, resp.Code) + + // The new user must appear in the store. + newUser, userErr := cm.GetUser(ctx, "new-user") + require.NoError(t, userErr) + require.Equal(t, "new-user", newUser.Name) + + // Critical: full SaveConfiguration (PutS3ApiConfiguration) must NOT have + // been called. Before the fix it was called once per CreateUser, rewriting + // every existing user file and triggering a cascade of reload events. + assert.Equal(t, 0, counter.putCalled, + "CreateUser must not trigger a full PutS3ApiConfiguration over all users") + + // All pre-existing users must still be intact. + for _, name := range []string{"existing-1", "existing-2", "existing-3"} { + u, err := cm.GetUser(ctx, name) + require.NoError(t, err, "pre-existing user %s should still exist", name) + assert.Equal(t, name, u.Name) + } +} diff --git a/weed/s3api/auth_credentials.go b/weed/s3api/auth_credentials.go index 895888521..fa70e3e36 100644 --- a/weed/s3api/auth_credentials.go +++ b/weed/s3api/auth_credentials.go @@ -1635,6 +1635,12 @@ func (iam *IdentityAccessManagement) GetCredentialManager() *credential.Credenti return iam.credentialManager } +// SetCredentialManagerForTest replaces the credential manager. Only intended +// for use in tests that need to inject a pre-configured in-memory store. +func (iam *IdentityAccessManagement) SetCredentialManagerForTest(cm *credential.CredentialManager) { + iam.credentialManager = cm +} + type managedPolicyLoader interface { LoadManagedPolicies(ctx context.Context) ([]*iam_pb.Policy, error) } diff --git a/weed/s3api/credential_iam_errors.go b/weed/s3api/credential_iam_errors.go new file mode 100644 index 000000000..e6fc1dcce --- /dev/null +++ b/weed/s3api/credential_iam_errors.go @@ -0,0 +1,27 @@ +package s3api + +import ( + "errors" + + "github.com/aws/aws-sdk-go/service/iam" + "github.com/seaweedfs/seaweedfs/weed/credential" +) + +// CredentialErrToIamErrCode maps a credential store error to the appropriate +// IAM error code. Permanent client errors (e.g. duplicate entity, missing +// entity) are mapped to their specific IAM codes; all other errors fall back +// to ServiceFailure so the caller receives an HTTP 500 rather than a 200. +// +// This is shared between the standalone IAM server (weed/iamapi) and the +// embedded IAM handler (weed/s3api) so error granularity stays in sync. +func CredentialErrToIamErrCode(err error) string { + switch { + case errors.Is(err, credential.ErrUserAlreadyExists): + return iam.ErrCodeEntityAlreadyExistsException + case errors.Is(err, credential.ErrUserNotFound), + errors.Is(err, credential.ErrAccessKeyNotFound): + return iam.ErrCodeNoSuchEntityException + default: + return iam.ErrCodeServiceFailureException + } +} diff --git a/weed/s3api/s3api_embedded_iam.go b/weed/s3api/s3api_embedded_iam.go index 69c666714..b69585e72 100644 --- a/weed/s3api/s3api_embedded_iam.go +++ b/weed/s3api/s3api_embedded_iam.go @@ -2195,6 +2195,18 @@ func (e *EmbeddedIamApi) ExecuteAction(ctx context.Context, values url.Values, s if iamErr != nil { return nil, iamErr } + // Use targeted create to avoid rewriting all existing user files via SaveConfiguration. + // credentialManager.CreateUser writes only the new user's file. + // Skip when skipPersist is set: the contract is no persistent write, so leave + // changed=true and let the tail's `if changed { if !skipPersist { ... } }` block + // suppress persistence the same way it does for every other action. + if e.credentialManager != nil && !skipPersist { + userName := values.Get("UserName") + if err := e.credentialManager.CreateUser(ctx, &iam_pb.Identity{Name: userName}); err != nil { + return nil, &iamError{Code: CredentialErrToIamErrCode(err), Error: err} + } + changed = false + } case "GetUser": userName := values.Get("UserName") var iamErr *iamError @@ -2469,11 +2481,11 @@ 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 == "DeletePolicy" { + } else if action == "AttachUserPolicy" || action == "DetachUserPolicy" || action == "CreatePolicy" || action == "DeletePolicy" || action == "CreateUser" { // 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 { - glog.Errorf("Failed to reload IAM configuration after managed policy mutation: %v", err) + glog.Errorf("Failed to reload IAM configuration after mutation: %v", err) } } response.SetRequestId(reqID) diff --git a/weed/s3api/s3api_embedded_iam_test.go b/weed/s3api/s3api_embedded_iam_test.go index 925f663f1..8905241f1 100644 --- a/weed/s3api/s3api_embedded_iam_test.go +++ b/weed/s3api/s3api_embedded_iam_test.go @@ -190,6 +190,91 @@ func TestEmbeddedIamCreateUser(t *testing.T) { assert.Equal(t, "TestUser", api.mockConfig.Identities[0].Name) } +// TestEmbeddedIamCreateUserDoesNotSaveAllUsers is a regression test for the bug +// where creating a new user triggered a full SaveConfiguration over all existing +// identities (causing N file writes + reload cycles in the filer_etc store). +// The fix uses credentialManager.CreateUser for a targeted single-file write. +func TestEmbeddedIamCreateUserDoesNotSaveAllUsers(t *testing.T) { + api := NewEmbeddedIamApiForTest() + ctx := context.Background() + + // Seed both mockConfig and the credential store directly so the test does + // not rely on the getS3ApiConfigurationFunc fixture's syncOnce side effect + // to populate the store. Pre-call assertions below fail loudly if seeding + // breaks for any reason. + existing := []string{"existing-user-1", "existing-user-2", "existing-user-3"} + api.mockConfig = &iam_pb.S3ApiConfiguration{} + for _, name := range existing { + api.mockConfig.Identities = append(api.mockConfig.Identities, &iam_pb.Identity{Name: name}) + } + require.NoError(t, api.credentialManager.SaveConfiguration(ctx, api.mockConfig)) + for _, name := range existing { + u, err := api.credentialManager.GetUser(ctx, name) + require.NoError(t, err, "seed precondition: %s must exist", name) + require.Equal(t, name, u.Name) + } + + // Track calls to PutS3ApiConfiguration (the full-config write path). + putConfigCalls := 0 + api.putS3ApiConfigurationFunc = func(s3cfg *iam_pb.S3ApiConfiguration) error { + putConfigCalls++ + api.mockConfig = proto.Clone(s3cfg).(*iam_pb.S3ApiConfiguration) + return api.credentialManager.SaveConfiguration(ctx, s3cfg) + } + + params := &iam.CreateUserInput{UserName: aws.String("new-user")} + req, _ := iam.New(session.New()).CreateUserRequest(params) + _ = req.Build() + out := iamCreateUserResponse{} + response, err := executeEmbeddedIamRequest(api, req.HTTPRequest, &out) + require.NoError(t, err) + require.Equal(t, http.StatusOK, response.Code) + + // The new user must be visible. + newUser, userErr := api.credentialManager.GetUser(ctx, "new-user") + require.NoError(t, userErr) + require.Equal(t, "new-user", newUser.Name) + + // The critical assertion: full SaveConfiguration must NOT have been called. + // Previously this was called once for every CreateUser, re-writing all N user + // files and triggering N reload loops. + assert.Equal(t, 0, putConfigCalls, "CreateUser must not call PutS3ApiConfiguration (full-config save)") + + // Pre-existing users must be untouched. + for _, name := range existing { + u, err := api.credentialManager.GetUser(ctx, name) + require.NoError(t, err, "pre-existing user %s should still exist", name) + assert.Equal(t, name, u.Name) + } +} + +// TestEmbeddedIamCreateUserSkipPersist asserts that ExecuteAction("CreateUser", +// skipPersist=true) performs no persistent write to the credential store. The +// targeted-create optimization must respect the skipPersist contract documented +// on ExecuteAction; otherwise no-persist callers would silently leak writes. +func TestEmbeddedIamCreateUserSkipPersist(t *testing.T) { + api := NewEmbeddedIamApiForTest() + ctx := context.Background() + + api.putS3ApiConfigurationFunc = func(s3cfg *iam_pb.S3ApiConfiguration) error { + t.Fatalf("PutS3ApiConfiguration must not be called when skipPersist=true") + return nil + } + + vals := url.Values{} + vals.Set("Action", "CreateUser") + vals.Set("UserName", "no-persist-user") + + resp, iamErr := api.ExecuteAction(ctx, vals, true /*skipPersist*/, "") + require.Nil(t, iamErr) + require.NotNil(t, resp) + + // The credential store must not have the user — skipPersist contract. + _, err := api.credentialManager.GetUser(ctx, "no-persist-user") + require.ErrorIs(t, err, credential.ErrUserNotFound, + "skipPersist=true must not write the new user to the credential store") +} + // TestEmbeddedIamListUsers tests listing users via the embedded IAM API func TestEmbeddedIamListUsers(t *testing.T) { api := NewEmbeddedIamApiForTest() @@ -2503,17 +2588,13 @@ func TestEmbeddedIamExecuteAction(t *testing.T) { api := NewEmbeddedIamApiForTest() api.mockConfig = &iam_pb.S3ApiConfiguration{} - // Explicitly set hook to debug panic - api.EmbeddedIamApi.reloadConfigurationFunc = func() error { - return nil - } - // Test case: CreateUser via ExecuteAction vals := url.Values{} vals.Set("Action", "CreateUser") vals.Set("UserName", "ExecuteActionUser") - resp, iamErr := api.ExecuteAction(context.Background(), vals, false, "") + ctx := context.Background() + resp, iamErr := api.ExecuteAction(ctx, vals, false, "") assert.Nil(t, iamErr) // Verify response type @@ -2521,9 +2602,11 @@ func TestEmbeddedIamExecuteAction(t *testing.T) { assert.True(t, ok) assert.Equal(t, "ExecuteActionUser", *createResp.CreateUserResult.User.UserName) - // Verify persistence - assert.Len(t, api.mockConfig.Identities, 1) - assert.Equal(t, "ExecuteActionUser", api.mockConfig.Identities[0].Name) + // CreateUser now uses a targeted write (credentialManager.CreateUser) rather than + // SaveConfiguration, so persistence is checked via the credential manager directly. + user, err := api.credentialManager.GetUser(ctx, "ExecuteActionUser") + assert.NoError(t, err) + assert.Equal(t, "ExecuteActionUser", user.Name) } // TestEmbeddedIamReadOnly tests that write operations are blocked when readOnly is true