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