From bf71fe00394b27e4f85a6eda556bc48e5d0241d7 Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Wed, 18 Feb 2026 16:21:48 -0800 Subject: [PATCH] Fix IAM defaults and s3tables identities --- weed/s3api/iam_defaults_test.go | 123 +++++++++++++++++++ weed/s3api/s3api_server.go | 8 +- weed/s3api/s3tables/handler.go | 56 +++++++-- weed/s3api/s3tables/handler_identity_test.go | 81 ++++++++++++ weed/s3api/s3tables/permissions.go | 4 + 5 files changed, 260 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..591fbdab4 100644 --- a/weed/s3api/s3tables/handler.go +++ b/weed/s3api/s3tables/handler.go @@ -168,26 +168,46 @@ 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. +// 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. 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 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 { - id := idField.String() - return id + if principal := normalizePrincipalID(idField.String()); principal != "" { + return principal + } } } } @@ -195,15 +215,35 @@ 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 } +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 new file mode 100644 index 000000000..fe28334ce --- /dev/null +++ b/weed/s3api/s3tables/handler_identity_test.go @@ -0,0 +1,81 @@ +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 0db52b602..71ef6855f 100644 --- a/weed/s3api/s3tables/permissions.go +++ b/weed/s3api/s3tables/permissions.go @@ -208,6 +208,10 @@ 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