Fix IAM defaults and S3Tables IAM regression (#8374)

* Fix IAM defaults and s3tables identities

* Refine S3Tables identity tests

* Clarify identity tests

weed/s3api/iam_defaults_test.go

@@ -1,10 +1,12 @@
 package s3api
 
 import (
+	"context"
 	"os"
 	"path/filepath"
 	"testing"
 
+	"github.com/seaweedfs/seaweedfs/weed/iam/integration"
 	"github.com/stretchr/testify/assert"
 )
 
@@ -154,3 +156,124 @@ 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")
+}

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 == "" {
-		// 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 {
+		// 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 {
 			configRoot.Policy.DefaultEffect = sts.EffectAllow
 		} else {
 			configRoot.Policy.DefaultEffect = sts.EffectDeny

weed/s3api/s3tables/handler.go

@@ -168,26 +168,50 @@ func (h *S3TablesHandler) HandleRequest(w http.ResponseWriter, r *http.Request,
 
 // Principal/authorization helpers
 
-// 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.
+// getAccountID returns a stable caller identifier for ownership and permission checks.
+// Reflection depends on the identity shape produced by JWT/STS auth (Account *struct{Id string},
+// Claims map[string]interface{} containing string values for preferred_username/sub, and optional
+// identity name/header values). Changing those fields without updating the reflection here will
+// break the handler, so refactorers should replace this with a typed interface if needed.
 func (h *S3TablesHandler) getAccountID(r *http.Request) string {
 	identityRaw := s3_constants.GetIdentityFromContext(r)
 	if identityRaw != nil {
-		// Use reflection to access the Account.Id field to avoid import cycle
+		// Use reflection to access identity fields and avoid import cycles.
 		val := reflect.ValueOf(identityRaw)
 		if val.Kind() == reflect.Ptr {
 			val = val.Elem()
 		}
 		if val.Kind() == reflect.Struct {
+			// Prefer stable claims from JWT/STS identities. Only "sub" is guaranteed durable per OIDC;
+			// preferred_username is ergonomic but can rotate and may orphan ownership data, while email
+			// is explicitly excluded to avoid storing PII in metadata.
+			claimsField := val.FieldByName("Claims")
+			if claimsField.IsValid() && claimsField.Kind() == reflect.Map && !claimsField.IsNil() && claimsField.Type().Key().Kind() == reflect.String {
+				for _, claimKey := range []string{"sub", "preferred_username"} {
+					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 {
-						id := idField.String()
-						return id
+						if principal := normalizePrincipalID(idField.String()); principal != "" {
+							return principal
+						}
 					}
 				}
 			}
@@ -195,15 +219,43 @@ func (h *S3TablesHandler) getAccountID(r *http.Request) string {
 	}
 
 	if identityName := s3_constants.GetIdentityNameFromContext(r); identityName != "" {
-		return identityName
+		if principal := normalizePrincipalID(identityName); principal != "" {
+			return principal
+		}
 	}
 
 	if accountID := r.Header.Get(s3_constants.AmzAccountId); accountID != "" {
-		return accountID
+		if principal := normalizePrincipalID(accountID); principal != "" {
+			return principal
+		}
 	}
 	return h.accountID
 }
 
+// normalizePrincipalID collapses ARN and identity strings to a key that is stable within a single account.
+// WARNING: this assumes identity names are unique per account; distinct principals such as
+// arn:aws:iam::111:user/alice and arn:aws:iam::222:user/alice will both normalize to "alice".
+// If future work adds multi-account support, revisit this function to include the account ID or full ARN
+// so ownership checks remain correct.
+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:])
+		}
+		if idx := strings.LastIndex(id, ":"); idx >= 0 && idx+1 < len(id) {
+			return strings.TrimSpace(id[idx+1:])
+		}
+		return strings.TrimSpace(id)
+	}
+	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 {

weed/s3api/s3tables/handler_identity_test.go

@@ -0,0 +1,124 @@
+package s3tables
+
+import (
+	"net/http"
+	"net/http/httptest"
+	"testing"
+
+	"github.com/seaweedfs/seaweedfs/weed/s3api/s3_constants"
+	"github.com/stretchr/testify/assert"
+)
+
+// testIdentity/testIdentityAccount mirror the production identity shape used via reflection.
+// Keep these field names in sync with getAccountID to avoid silent breaks.
+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)
+	assert.Equal(t, "alice-sub", got, "expected sub claim to be used before preferred_username")
+	assert.NotEqual(t, DefaultAccountID, got, "claims should override default handler account")
+}
+
+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)
+	assert.Equal(t, "user-123", got, "expected sub claim to be used when preferred_username missing")
+	assert.NotEqual(t, DefaultAccountID, got, "claims should override default handler account")
+}
+
+func TestGetAccountIDFallsBackToHandlerDefaultAccount(t *testing.T) {
+	h := NewS3TablesHandler()
+	req := httptest.NewRequest(http.MethodGet, "/", nil)
+
+	assert.Equal(t, DefaultAccountID, h.getAccountID(req), "expected handler default account to be returned when no identity is set")
+}
+
+func TestGetAccountIDIgnoresEmptyClaimValues(t *testing.T) {
+	h := NewS3TablesHandler()
+	id := &testIdentity{
+		Account: &testIdentityAccount{Id: s3_constants.AccountAdminId},
+		Claims: map[string]interface{}{
+			"preferred_username": " ",
+			"sub":                "user-123",
+		},
+	}
+	req := httptest.NewRequest(http.MethodGet, "/", nil)
+	req = req.WithContext(s3_constants.SetIdentityInContext(req.Context(), id))
+
+	assert.Equal(t, "user-123", h.getAccountID(req), "expected whitespace preferred_username to be ignored")
+}
+
+func TestGetAccountIDFallsBackToIdentityName(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"))
+
+	assert.Equal(t, "alice-session", h.getAccountID(req), "expected ARN session suffix to be extracted")
+}
+
+func TestGetAccountIDFallsBackToARNColonSegment(t *testing.T) {
+	h := NewS3TablesHandler()
+	req := httptest.NewRequest(http.MethodGet, "/", nil)
+	req = req.WithContext(s3_constants.SetIdentityNameInContext(req.Context(), "arn:aws:iam::123456789012:root"))
+
+	assert.Equal(t, "root", h.getAccountID(req), "expected ARN colon segment to be returned as principal")
+}
+
+func TestGetAccountIDFallsBackToAmzAccountIdHeader(t *testing.T) {
+	h := NewS3TablesHandler()
+	req := httptest.NewRequest(http.MethodGet, "/", nil)
+	req.Header.Set(s3_constants.AmzAccountId, "header-account")
+
+	assert.Equal(t, "header-account", h.getAccountID(req), "expected header value to be used when no identity is present")
+}
+
+func TestGetAccountIDFallsBackToAccountID(t *testing.T) {
+	h := NewS3TablesHandler()
+	id := &testIdentity{
+		Account: &testIdentityAccount{Id: "my-account-id"},
+	}
+	req := httptest.NewRequest(http.MethodGet, "/", nil)
+	req = req.WithContext(s3_constants.SetIdentityInContext(req.Context(), id))
+
+	assert.Equal(t, "my-account-id", h.getAccountID(req), "expected Account.Id to be returned when claims are missing")
+}
+
+func TestGetAccountIDNormalizesAccountIDARN(t *testing.T) {
+	h := NewS3TablesHandler()
+	id := &testIdentity{
+		Account: &testIdentityAccount{Id: "arn:aws:iam::123456789012:user/bob"},
+	}
+	req := httptest.NewRequest(http.MethodGet, "/", nil)
+	req = req.WithContext(s3_constants.SetIdentityInContext(req.Context(), id))
+
+	assert.Equal(t, "bob", h.getAccountID(req), "expected ARN account ID to be normalized to the suffix")
+}

weed/s3api/s3tables/permissions.go

@@ -208,6 +208,11 @@ 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 or s3 wildcards.
+		// s3:* is treated as s3tables:* so shared admin policies still permit table access.
+		if action == "*" || action == string(s3_constants.ACTION_ADMIN) || action == "s3:*" || action == "s3tables:*" {
+			return true
+		}
 		for _, candidate := range candidates {
 			if action == candidate {
 				return true