diff --git a/weed/s3api/iam_defaults_test.go b/weed/s3api/iam_defaults_test.go index 37aeeb482..e2fcfaefd 100644 --- a/weed/s3api/iam_defaults_test.go +++ b/weed/s3api/iam_defaults_test.go @@ -1,12 +1,10 @@ package s3api import ( - "context" "os" "path/filepath" "testing" - "github.com/seaweedfs/seaweedfs/weed/iam/integration" "github.com/stretchr/testify/assert" ) @@ -156,124 +154,3 @@ func TestLoadIAMManagerFromConfig_MissingKeyError(t *testing.T) { assert.Error(t, err) assert.Contains(t, err.Error(), "no signing key found for STS service") } - -func TestLoadIAMManagerFromConfig_ExplicitFileDefaultsToDeny(t *testing.T) { - tmpDir := t.TempDir() - configPath := filepath.Join(tmpDir, "iam_config_implicit_policy_defaults.json") - - // Explicit config file with no policy.defaultEffect should default to Deny. - configContent := `{ - "sts": { - "issuer": "explicit-config" - } - }` - - err := os.WriteFile(configPath, []byte(configContent), 0644) - assert.NoError(t, err) - - filerProvider := func() string { return "localhost:8888" } - defaultSigningKeyProvider := func() string { return "fallback-key-for-explicit-config" } - - manager, err := loadIAMManagerFromConfig(configPath, filerProvider, defaultSigningKeyProvider) - assert.NoError(t, err) - assert.NotNil(t, manager) - assert.False(t, manager.DefaultAllow()) -} - -func TestLoadIAMManagerFromConfig_NoFileDefaultsToAllow(t *testing.T) { - // No explicit IAM file should preserve zero-config startup behavior. - filerProvider := func() string { return "localhost:8888" } - defaultSigningKeyProvider := func() string { return "fallback-key-for-zero-config" } - - manager, err := loadIAMManagerFromConfig("", filerProvider, defaultSigningKeyProvider) - assert.NoError(t, err) - assert.NotNil(t, manager) - assert.True(t, manager.DefaultAllow()) -} - -func TestLoadIAMManagerFromConfig_ExplicitFileEnforcesUserScopedPolicy(t *testing.T) { - tmpDir := t.TempDir() - configPath := filepath.Join(tmpDir, "iam_regression_8366.json") - - // Regression coverage for #8366: - // with explicit IAM config and omitted policy.defaultEffect, unrestricted bucket creation - // must NOT be allowed. - configContent := `{ - "sts": { - "issuer": "seaweedfs-sts" - }, - "policies": [ - { - "name": "S3UserPolicy", - "document": { - "Version": "2012-10-17", - "Statement": [ - { - "Effect": "Allow", - "Action": ["s3:ListAllMyBuckets"], - "Resource": ["arn:aws:s3:::*"] - }, - { - "Effect": "Allow", - "Action": ["s3:*"], - "Resource": [ - "arn:aws:s3:::user-${jwt:preferred_username}", - "arn:aws:s3:::user-${jwt:preferred_username}/*" - ] - } - ] - } - } - ], - "roles": [ - { - "roleName": "S3UserRole", - "roleArn": "arn:aws:iam::role/S3UserRole", - "attachedPolicies": ["S3UserPolicy"], - "trustPolicy": { - "Version": "2012-10-17", - "Statement": [ - { - "Effect": "Allow", - "Principal": {"Federated": "*"}, - "Action": ["sts:AssumeRoleWithWebIdentity"] - } - ] - } - } - ] - }` - - err := os.WriteFile(configPath, []byte(configContent), 0644) - assert.NoError(t, err) - - filerProvider := func() string { return "localhost:8888" } - defaultSigningKeyProvider := func() string { return "fallback-key-for-regression-8366" } - - manager, err := loadIAMManagerFromConfig(configPath, filerProvider, defaultSigningKeyProvider) - assert.NoError(t, err) - assert.NotNil(t, manager) - assert.False(t, manager.DefaultAllow()) - - ctx := context.Background() - principal := "arn:aws:sts::000000000000:assumed-role/S3UserRole/alice-session" - reqCtx := map[string]interface{}{"jwt:preferred_username": "alice"} - - allowed, err := manager.IsActionAllowed(ctx, &integration.ActionRequest{ - Principal: principal, - Action: "s3:CreateBucket", - Resource: "arn:aws:s3:::arbitrary-bucket", - RequestContext: reqCtx, - }) - assert.NoError(t, err) - assert.False(t, allowed, "arbitrary bucket creation should be denied") - - allowed, err = manager.IsActionAllowed(ctx, &integration.ActionRequest{ - Principal: principal, - Action: "s3:CreateBucket", - Resource: "arn:aws:s3:::user-alice", - RequestContext: reqCtx, - }) - assert.NoError(t, err) - assert.True(t, allowed, "user-scoped bucket creation should be allowed") -} diff --git a/weed/s3api/s3api_server.go b/weed/s3api/s3api_server.go index 5193199b2..39f6f7939 100644 --- a/weed/s3api/s3api_server.go +++ b/weed/s3api/s3api_server.go @@ -884,10 +884,10 @@ func loadIAMManagerFromConfig(configPath string, filerAddressProvider func() str configRoot.Policy.StoreType = sts.StoreTypeMemory } if configRoot.Policy.DefaultEffect == "" { - // Secure default when an explicit IAM config file is provided: - // omitted defaultEffect should be Deny to avoid unintentional privilege escalation. - // Keep zero-config startup behavior (no config file path) open for memory store. - if configPath == "" && configRoot.Policy.StoreType == sts.StoreTypeMemory { + // Default to Allow (open) with in-memory store so that + // users can start using STS without locking themselves out immediately. + // For other stores (e.g. filer), default to Deny (closed) for security. + if configRoot.Policy.StoreType == sts.StoreTypeMemory { configRoot.Policy.DefaultEffect = sts.EffectAllow } else { configRoot.Policy.DefaultEffect = sts.EffectDeny diff --git a/weed/s3api/s3tables/handler.go b/weed/s3api/s3tables/handler.go index 591fbdab4..563436430 100644 --- a/weed/s3api/s3tables/handler.go +++ b/weed/s3api/s3tables/handler.go @@ -168,46 +168,26 @@ func (h *S3TablesHandler) HandleRequest(w http.ResponseWriter, r *http.Request, // Principal/authorization helpers -// getAccountID returns a stable caller identifier for ownership and permission checks. -// Prefer JWT/STS user claims to avoid collapsing all assumed-role callers to the same -// account ID (e.g. 000000000000), which would effectively grant cross-user access. +// getAccountID returns the authenticated account ID from the request or the handler's default. +// This is also used as the principal for permission checks, ensuring alignment between +// the caller identity and ownership verification when IAM is enabled. func (h *S3TablesHandler) getAccountID(r *http.Request) string { identityRaw := s3_constants.GetIdentityFromContext(r) if identityRaw != nil { - // Use reflection to access identity fields and avoid import cycles. + // Use reflection to access the Account.Id field to avoid import cycle val := reflect.ValueOf(identityRaw) if val.Kind() == reflect.Ptr { val = val.Elem() } if val.Kind() == reflect.Struct { - // Prefer stable user claims from JWT/STS identities. - claimsField := val.FieldByName("Claims") - if claimsField.IsValid() && claimsField.Kind() == reflect.Map && !claimsField.IsNil() && claimsField.Type().Key().Kind() == reflect.String { - for _, claimKey := range []string{"preferred_username", "sub", "email"} { - claimVal := claimsField.MapIndex(reflect.ValueOf(claimKey)) - if !claimVal.IsValid() { - continue - } - if claimVal.Kind() == reflect.Interface && !claimVal.IsNil() { - claimVal = claimVal.Elem() - } - if claimVal.Kind() == reflect.String { - if principal := normalizePrincipalID(claimVal.String()); principal != "" { - return principal - } - } - } - } - accountField := val.FieldByName("Account") if accountField.IsValid() && !accountField.IsNil() { accountVal := accountField.Elem() if accountVal.Kind() == reflect.Struct { idField := accountVal.FieldByName("Id") if idField.IsValid() && idField.Kind() == reflect.String { - if principal := normalizePrincipalID(idField.String()); principal != "" { - return principal - } + id := idField.String() + return id } } } @@ -215,35 +195,15 @@ func (h *S3TablesHandler) getAccountID(r *http.Request) string { } if identityName := s3_constants.GetIdentityNameFromContext(r); identityName != "" { - if principal := normalizePrincipalID(identityName); principal != "" { - return principal - } + return identityName } if accountID := r.Header.Get(s3_constants.AmzAccountId); accountID != "" { - if principal := normalizePrincipalID(accountID); principal != "" { - return principal - } + return accountID } return h.accountID } -func normalizePrincipalID(id string) string { - id = strings.TrimSpace(id) - if id == "" { - return "" - } - // If this is an ARN (common for assumed roles), use the trailing segment as a - // stable-ish principal key instead of embedding the full ARN in ownership fields. - if strings.HasPrefix(id, "arn:") { - if idx := strings.LastIndex(id, "/"); idx >= 0 && idx+1 < len(id) { - return strings.TrimSpace(id[idx+1:]) - } - return "" - } - return id -} - // getIdentityActions extracts the action list from the identity object in the request context. // Uses reflection to avoid import cycles with s3api package. func getIdentityActions(r *http.Request) []string { diff --git a/weed/s3api/s3tables/handler_identity_test.go b/weed/s3api/s3tables/handler_identity_test.go deleted file mode 100644 index fe28334ce..000000000 --- a/weed/s3api/s3tables/handler_identity_test.go +++ /dev/null @@ -1,81 +0,0 @@ -package s3tables - -import ( - "net/http" - "net/http/httptest" - "testing" - - "github.com/seaweedfs/seaweedfs/weed/s3api/s3_constants" -) - -type testIdentityAccount struct { - Id string -} - -type testIdentity struct { - Account *testIdentityAccount - Claims map[string]interface{} -} - -func TestGetAccountIDPrefersClaimsOverAccountID(t *testing.T) { - h := NewS3TablesHandler() - id := &testIdentity{ - Account: &testIdentityAccount{Id: s3_constants.AccountAdminId}, - Claims: map[string]interface{}{ - "preferred_username": "alice", - "sub": "alice-sub", - }, - } - - req := httptest.NewRequest(http.MethodGet, "/", nil) - req = req.WithContext(s3_constants.SetIdentityInContext(req.Context(), id)) - - got := h.getAccountID(req) - if got != "alice" { - t.Fatalf("expected preferred_username claim to be used, got %q", got) - } -} - -func TestGetAccountIDUsesSubWhenPreferredUsernameMissing(t *testing.T) { - h := NewS3TablesHandler() - id := &testIdentity{ - Account: &testIdentityAccount{Id: s3_constants.AccountAdminId}, - Claims: map[string]interface{}{ - "sub": "user-123", - }, - } - - req := httptest.NewRequest(http.MethodGet, "/", nil) - req = req.WithContext(s3_constants.SetIdentityInContext(req.Context(), id)) - - got := h.getAccountID(req) - if got != "user-123" { - t.Fatalf("expected sub claim to be used, got %q", got) - } -} - -func TestGetAccountIDFallsBackToAccountID(t *testing.T) { - h := NewS3TablesHandler() - id := &testIdentity{ - Account: &testIdentityAccount{Id: s3_constants.AccountAdminId}, - } - - req := httptest.NewRequest(http.MethodGet, "/", nil) - req = req.WithContext(s3_constants.SetIdentityInContext(req.Context(), id)) - - got := h.getAccountID(req) - if got != s3_constants.AccountAdminId { - t.Fatalf("expected fallback account id %q, got %q", s3_constants.AccountAdminId, got) - } -} - -func TestGetAccountIDNormalizesArnIdentityName(t *testing.T) { - h := NewS3TablesHandler() - req := httptest.NewRequest(http.MethodGet, "/", nil) - req = req.WithContext(s3_constants.SetIdentityNameInContext(req.Context(), "arn:aws:sts::123456789012:assumed-role/S3UserRole/alice-session")) - - got := h.getAccountID(req) - if got != "alice-session" { - t.Fatalf("expected ARN session suffix, got %q", got) - } -} diff --git a/weed/s3api/s3tables/permissions.go b/weed/s3api/s3tables/permissions.go index 71ef6855f..0db52b602 100644 --- a/weed/s3api/s3tables/permissions.go +++ b/weed/s3api/s3tables/permissions.go @@ -208,10 +208,6 @@ func hasIdentityPermission(operation string, ctx *PolicyContext) bool { candidates = append(candidates, operation+":"+ctx.TableBucketName, fullAction+":"+ctx.TableBucketName) } for _, action := range ctx.IdentityActions { - // Legacy static identities may still use broad admin markers. - if action == "*" || action == "Admin" || action == string(s3_constants.ACTION_ADMIN) || action == "s3:*" || action == "s3tables:*" { - return true - } for _, candidate := range candidates { if action == candidate { return true