From d1fecdface260a8d51de66de58ab69270e61a9a9 Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Wed, 18 Feb 2026 18:20:03 -0800 Subject: [PATCH] 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 | 123 ++++++++++++++++++ weed/s3api/s3api_server.go | 8 +- weed/s3api/s3tables/handler.go | 68 ++++++++-- weed/s3api/s3tables/handler_identity_test.go | 124 +++++++++++++++++++ weed/s3api/s3tables/permissions.go | 5 + 5 files changed, 316 insertions(+), 12 deletions(-) create mode 100644 weed/s3api/s3tables/handler_identity_test.go diff --git a/weed/s3api/iam_defaults_test.go b/weed/s3api/iam_defaults_test.go index e2fcfaefd..37aeeb482 100644 --- a/weed/s3api/iam_defaults_test.go +++ b/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") +} diff --git a/weed/s3api/s3api_server.go b/weed/s3api/s3api_server.go index 39f6f7939..5193199b2 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 == "" { - // 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 diff --git a/weed/s3api/s3tables/handler.go b/weed/s3api/s3tables/handler.go index 563436430..962d3bfc9 100644 --- a/weed/s3api/s3tables/handler.go +++ b/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 { diff --git a/weed/s3api/s3tables/handler_identity_test.go b/weed/s3api/s3tables/handler_identity_test.go new file mode 100644 index 000000000..1e928a054 --- /dev/null +++ b/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") +} diff --git a/weed/s3api/s3tables/permissions.go b/weed/s3api/s3tables/permissions.go index 0db52b602..e699526bd 100644 --- a/weed/s3api/s3tables/permissions.go +++ b/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