From 87474c2f218bc77a2690785f06d86571b3aafbe0 Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Mon, 31 Aug 2026 10:24:22 -0700 Subject: [PATCH] s3: let attached policies authorize CreateBucket (#11049) * s3: resolve admin bucket subresources to their specific S3 actions Encryption, requestPayment, publicAccessBlock and ownershipControls requests reached the policy engines as s3:*, so only a policy granting all of s3 could authorize them. Map each subresource to its AWS action, with DELETE sharing the PUT permission as AWS does. Claude-Session: https://claude.ai/code/session_01XH7iM88ZqWMEvsLB8tkWPQ * s3: authorize CreateBucket as s3:CreateBucket in the policy engine A plain bucket-level PUT is registered with ACTION_ADMIN, which resolved to s3:*, so no attached policy short of s3:* could match it. Federated sessions whose policy explicitly allowed s3:CreateBucket were always denied while the same policy worked for object operations. Resolve it to s3:CreateBucket, like DeleteBucket already resolves. Claude-Session: https://claude.ai/code/session_01XH7iM88ZqWMEvsLB8tkWPQ --- weed/s3api/s3_action_resolver.go | 23 +++- weed/s3api/s3_action_resolver_test.go | 44 +++++++ weed/s3api/s3_constants/s3_action_strings.go | 19 +++ weed/s3api/s3_end_to_end_test.go | 129 +++++++++++++++++++ 4 files changed, 214 insertions(+), 1 deletion(-) diff --git a/weed/s3api/s3_action_resolver.go b/weed/s3api/s3_action_resolver.go index 4f25ef537..707f3171f 100644 --- a/weed/s3api/s3_action_resolver.go +++ b/weed/s3api/s3_action_resolver.go @@ -97,6 +97,25 @@ var bucketQueryActions = map[string]map[string]string{ http.MethodGet: s3_constants.S3_ACTION_GET_BUCKET_OBJECT_LOCK, http.MethodPut: s3_constants.S3_ACTION_PUT_BUCKET_OBJECT_LOCK, }, + "encryption": { + http.MethodGet: s3_constants.S3_ACTION_GET_BUCKET_ENCRYPTION, + http.MethodPut: s3_constants.S3_ACTION_PUT_BUCKET_ENCRYPTION, + http.MethodDelete: s3_constants.S3_ACTION_PUT_BUCKET_ENCRYPTION, // DELETE uses same permission as PUT + }, + "requestPayment": { + http.MethodGet: s3_constants.S3_ACTION_GET_BUCKET_REQUEST_PAYMENT, + http.MethodPut: s3_constants.S3_ACTION_PUT_BUCKET_REQUEST_PAYMENT, + }, + "publicAccessBlock": { + http.MethodGet: s3_constants.S3_ACTION_GET_BUCKET_PUBLIC_ACCESS_BLOCK, + http.MethodPut: s3_constants.S3_ACTION_PUT_BUCKET_PUBLIC_ACCESS_BLOCK, + http.MethodDelete: s3_constants.S3_ACTION_PUT_BUCKET_PUBLIC_ACCESS_BLOCK, // DELETE uses same permission as PUT + }, + "ownershipControls": { + http.MethodGet: s3_constants.S3_ACTION_GET_BUCKET_OWNERSHIP_CONTROLS, + http.MethodPut: s3_constants.S3_ACTION_PUT_BUCKET_OWNERSHIP_CONTROLS, + http.MethodDelete: s3_constants.S3_ACTION_PUT_BUCKET_OWNERSHIP_CONTROLS, // DELETE uses same permission as PUT + }, } // resolveFromQueryParameters checks query parameters to determine specific S3 actions @@ -281,7 +300,9 @@ func resolveBucketLevelAction(method string, baseAction string) string { } case http.MethodPut: - if baseAction == s3_constants.ACTION_WRITE { + // CreateBucket is registered with ACTION_ADMIN; resolving it to s3:* + // would make it unmatchable by a policy granting s3:CreateBucket. + if baseAction == s3_constants.ACTION_WRITE || baseAction == s3_constants.ACTION_ADMIN { return s3_constants.S3_ACTION_CREATE_BUCKET } diff --git a/weed/s3api/s3_action_resolver_test.go b/weed/s3api/s3_action_resolver_test.go index 2b5709d00..ef4caaf95 100644 --- a/weed/s3api/s3_action_resolver_test.go +++ b/weed/s3api/s3_action_resolver_test.go @@ -114,6 +114,50 @@ func TestResolveS3Action_AttributesBeforeVersionId(t *testing.T) { } } +// Bucket subresources registered with ACTION_ADMIN must resolve to their own +// S3 actions so a policy granting one of them does not need s3:*, and so no +// broader grant sweeps them in. +func TestResolveS3Action_AdminBucketSubresources(t *testing.T) { + tests := []struct { + name string + method string + query string + want string + }{ + {"get encryption", http.MethodGet, "encryption", s3_constants.S3_ACTION_GET_BUCKET_ENCRYPTION}, + {"put encryption", http.MethodPut, "encryption", s3_constants.S3_ACTION_PUT_BUCKET_ENCRYPTION}, + {"delete encryption", http.MethodDelete, "encryption", s3_constants.S3_ACTION_PUT_BUCKET_ENCRYPTION}, + {"get requestPayment", http.MethodGet, "requestPayment", s3_constants.S3_ACTION_GET_BUCKET_REQUEST_PAYMENT}, + {"put requestPayment", http.MethodPut, "requestPayment", s3_constants.S3_ACTION_PUT_BUCKET_REQUEST_PAYMENT}, + {"get publicAccessBlock", http.MethodGet, "publicAccessBlock", s3_constants.S3_ACTION_GET_BUCKET_PUBLIC_ACCESS_BLOCK}, + {"put publicAccessBlock", http.MethodPut, "publicAccessBlock", s3_constants.S3_ACTION_PUT_BUCKET_PUBLIC_ACCESS_BLOCK}, + {"delete publicAccessBlock", http.MethodDelete, "publicAccessBlock", s3_constants.S3_ACTION_PUT_BUCKET_PUBLIC_ACCESS_BLOCK}, + {"get ownershipControls", http.MethodGet, "ownershipControls", s3_constants.S3_ACTION_GET_BUCKET_OWNERSHIP_CONTROLS}, + {"put ownershipControls", http.MethodPut, "ownershipControls", s3_constants.S3_ACTION_PUT_BUCKET_OWNERSHIP_CONTROLS}, + {"delete ownershipControls", http.MethodDelete, "ownershipControls", s3_constants.S3_ACTION_PUT_BUCKET_OWNERSHIP_CONTROLS}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + r, _ := http.NewRequest(tt.method, "http://localhost/bucket?"+tt.query, nil) + got := ResolveS3Action(r, s3_constants.ACTION_ADMIN, "bucket", "") + if got != tt.want { + t.Errorf("ResolveS3Action() = %q, want %q", got, tt.want) + } + }) + } +} + +// CreateBucket is a plain bucket-level PUT registered with ACTION_ADMIN. It +// must resolve to s3:CreateBucket so an attached policy granting that action +// can authorize it without granting s3:*. +func TestResolveS3Action_CreateBucket(t *testing.T) { + r, _ := http.NewRequest(http.MethodPut, "http://localhost/new-bucket", nil) + if got := ResolveS3Action(r, s3_constants.ACTION_ADMIN, "new-bucket", ""); got != s3_constants.S3_ACTION_CREATE_BUCKET { + t.Errorf("ResolveS3Action() = %q, want %q", got, s3_constants.S3_ACTION_CREATE_BUCKET) + } +} + // A base action naming another service carries no S3 request shape, so a query // parameter on the request must not redirect it to an S3 action. func TestResolveS3ActionKeepsNonS3Service(t *testing.T) { diff --git a/weed/s3api/s3_constants/s3_action_strings.go b/weed/s3api/s3_constants/s3_action_strings.go index 037324ae5..618c4c7fa 100644 --- a/weed/s3api/s3_constants/s3_action_strings.go +++ b/weed/s3api/s3_constants/s3_action_strings.go @@ -81,6 +81,25 @@ const ( S3_ACTION_GET_BUCKET_OBJECT_LOCK = "s3:GetBucketObjectLockConfiguration" S3_ACTION_PUT_BUCKET_OBJECT_LOCK = "s3:PutBucketObjectLockConfiguration" + // Bucket encryption operations + // Note: DELETE bucket encryption uses s3:PutEncryptionConfiguration, matching AWS + S3_ACTION_GET_BUCKET_ENCRYPTION = "s3:GetEncryptionConfiguration" + S3_ACTION_PUT_BUCKET_ENCRYPTION = "s3:PutEncryptionConfiguration" + + // Bucket request payment operations + S3_ACTION_GET_BUCKET_REQUEST_PAYMENT = "s3:GetBucketRequestPayment" + S3_ACTION_PUT_BUCKET_REQUEST_PAYMENT = "s3:PutBucketRequestPayment" + + // Bucket public access block operations + // Note: DELETE uses s3:PutBucketPublicAccessBlock, matching AWS + S3_ACTION_GET_BUCKET_PUBLIC_ACCESS_BLOCK = "s3:GetBucketPublicAccessBlock" + S3_ACTION_PUT_BUCKET_PUBLIC_ACCESS_BLOCK = "s3:PutBucketPublicAccessBlock" + + // Bucket ownership controls operations + // Note: DELETE uses s3:PutBucketOwnershipControls, matching AWS + S3_ACTION_GET_BUCKET_OWNERSHIP_CONTROLS = "s3:GetBucketOwnershipControls" + S3_ACTION_PUT_BUCKET_OWNERSHIP_CONTROLS = "s3:PutBucketOwnershipControls" + // Wildcard for all S3 actions S3_ACTION_ALL = "s3:*" ) diff --git a/weed/s3api/s3_end_to_end_test.go b/weed/s3api/s3_end_to_end_test.go index 958323f7b..cc600d43b 100644 --- a/weed/s3api/s3_end_to_end_test.go +++ b/weed/s3api/s3_end_to_end_test.go @@ -17,6 +17,7 @@ import ( "github.com/seaweedfs/seaweedfs/weed/iam/oidc" "github.com/seaweedfs/seaweedfs/weed/iam/policy" "github.com/seaweedfs/seaweedfs/weed/iam/sts" + "github.com/seaweedfs/seaweedfs/weed/s3api/s3_constants" "github.com/seaweedfs/seaweedfs/weed/s3api/s3err" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -361,6 +362,134 @@ func TestS3ListObjectsV2PrefixCondition(t *testing.T) { } } +// TestS3CreateBucketWithAttachedPolicy verifies a federated session whose +// attached policy grants s3:CreateBucket on a bucket ARN pattern can create +// matching buckets. CreateBucket is registered with ACTION_ADMIN, which used +// to resolve to s3:* and made the operation unmatchable by any narrower grant. +func TestS3CreateBucketWithAttachedPolicy(t *testing.T) { + iamManager := integration.NewIAMManager() + config := &integration.IAMConfig{ + STS: &sts.STSConfig{ + TokenDuration: sts.FlexibleDuration{Duration: time.Hour}, + MaxSessionLength: sts.FlexibleDuration{Duration: time.Hour * 12}, + Issuer: "test-sts", + SigningKey: []byte("test-signing-key-32-characters-long"), + }, + Policy: &policy.PolicyEngineConfig{ + DefaultEffect: "Deny", + StoreType: "memory", + }, + Roles: &integration.RoleStoreConfig{ + StoreType: "memory", + }, + } + + err := iamManager.Initialize(config, func() string { return "localhost:8888" }) + require.NoError(t, err) + + setupTestProviders(t, iamManager) + + s3IAMIntegration := NewS3IAMIntegration(iamManager, "localhost:8888") + ctx := context.Background() + + bucketPolicy := &policy.PolicyDocument{ + Version: "2012-10-17", + Statement: []policy.Statement{ + { + Sid: "AllowBucketManagement", + Effect: "Allow", + Action: []string{"s3:CreateBucket", "s3:DeleteBucket"}, + Resource: []string{"arn:aws:s3:::ml-*"}, + }, + { + Sid: "AllowSTSSessionValidation", + Effect: "Allow", + Action: []string{"sts:ValidateSession"}, + Resource: []string{"*"}, + }, + }, + } + + iamManager.CreatePolicy(ctx, "", "BucketManagementPolicy", bucketPolicy) + iamManager.CreateRole(ctx, "", "BucketManagerRole", &integration.RoleDefinition{ + RoleName: "BucketManagerRole", + TrustPolicy: &policy.PolicyDocument{ + Version: "2012-10-17", + Statement: []policy.Statement{ + { + Effect: "Allow", + Principal: map[string]interface{}{"Federated": "test-oidc"}, + Action: []string{"sts:AssumeRoleWithWebIdentity"}, + }, + }, + }, + AttachedPolicies: []string{"BucketManagementPolicy"}, + }) + + validJWTToken := createTestJWTEndToEnd(t, "https://test-issuer.com", "test-user-123", "test-signing-key") + response, err := iamManager.AssumeRoleWithWebIdentity(ctx, &sts.AssumeRoleWithWebIdentityRequest{ + RoleArn: "arn:aws:iam::role/BucketManagerRole", + WebIdentityToken: validJWTToken, + RoleSessionName: "bucket-create-session", + }) + require.NoError(t, err) + + authReq := httptest.NewRequest("PUT", "/ml-models", http.NoBody) + authReq.Header.Set("Authorization", "Bearer "+response.Credentials.SessionToken) + identity, errCode := s3IAMIntegration.AuthenticateJWT(ctx, authReq) + require.Equal(t, s3err.ErrNone, errCode) + + tests := []struct { + name string + method string + url string + bucket string + action Action + expected s3err.ErrorCode + }{ + { + name: "CreateBucket on matching bucket is allowed", + method: "PUT", + url: "/ml-models", + bucket: "ml-models", + action: Action(s3_constants.ACTION_ADMIN), + expected: s3err.ErrNone, + }, + { + name: "CreateBucket outside the resource pattern is denied", + method: "PUT", + url: "/other-bucket", + bucket: "other-bucket", + action: Action(s3_constants.ACTION_ADMIN), + expected: s3err.ErrAccessDenied, + }, + { + name: "PutBucketEncryption is not swept into CreateBucket", + method: "PUT", + url: "/ml-models?encryption", + bucket: "ml-models", + action: Action(s3_constants.ACTION_ADMIN), + expected: s3err.ErrAccessDenied, + }, + { + name: "DeleteBucket on matching bucket is allowed", + method: "DELETE", + url: "/ml-models", + bucket: "ml-models", + action: Action(s3_constants.ACTION_DELETE_BUCKET), + expected: s3err.ErrNone, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + req := httptest.NewRequest(tt.method, tt.url, http.NoBody) + result := s3IAMIntegration.AuthorizeAction(ctx, identity, tt.action, tt.bucket, "", req) + assert.Equal(t, tt.expected, result) + }) + } +} + // TestS3CORSWithJWT tests CORS preflight requests with IAM func TestS3CORSWithJWT(t *testing.T) { s3Server, iamManager := setupCompleteS3IAMSystem(t)