diff --git a/weed/s3api/auth_credentials.go b/weed/s3api/auth_credentials.go index 85002377b..54293e95a 100644 --- a/weed/s3api/auth_credentials.go +++ b/weed/s3api/auth_credentials.go @@ -513,7 +513,8 @@ func (iam *IdentityAccessManagement) authRequest(r *http.Request, action Action) // - No policy or indeterminate → fall through to IAM checks if iam.policyEngine != nil && bucket != "" { principal := buildPrincipalARN(identity) - allowed, evaluated, err := iam.policyEngine.EvaluatePolicy(bucket, object, string(action), principal) + // Use context-aware policy evaluation to get the correct S3 action + allowed, evaluated, err := iam.policyEngine.EvaluatePolicyWithContext(bucket, object, string(action), principal, r) if err != nil { // SECURITY: Fail-close on policy evaluation errors diff --git a/weed/s3api/s3_action_resolver.go b/weed/s3api/s3_action_resolver.go new file mode 100644 index 000000000..83431424c --- /dev/null +++ b/weed/s3api/s3_action_resolver.go @@ -0,0 +1,334 @@ +package s3api + +import ( + "net/http" + "net/url" + "strings" + + "github.com/seaweedfs/seaweedfs/weed/s3api/s3_constants" +) + +// ResolveS3Action determines the specific S3 action from HTTP request context. +// This is the unified implementation used by both the bucket policy engine +// and the IAM integration for consistent action resolution. +// +// It examines the HTTP method, path, query parameters, and headers to determine +// the most specific S3 action string (e.g., "s3:DeleteObject", "s3:PutObjectTagging"). +// +// Parameters: +// - r: HTTP request containing method, URL, query params, and headers +// - baseAction: Coarse-grained action constant (e.g., ACTION_WRITE, ACTION_READ) +// - bucket: Bucket name from the request path +// - object: Object key from the request path (may be empty for bucket operations) +// +// Returns: +// - Specific S3 action string (e.g., "s3:DeleteObject") +// - Falls back to base action mapping if no specific resolution is possible +// - Always returns a valid S3 action string (never empty) +func ResolveS3Action(r *http.Request, baseAction string, bucket string, object string) string { + if r == nil || r.URL == nil { + // No HTTP context available: fall back to coarse-grained mapping + // This ensures consistent behavior and avoids returning empty strings + return mapBaseActionToS3Format(baseAction) + } + + method := r.Method + query := r.URL.Query() + + // Determine if this is an object or bucket operation + // Note: "/" is treated as bucket-level, not object-level + hasObject := object != "" && object != "/" + + // Priority 1: Check for specific query parameters that indicate specific actions + // These override everything else because they explicitly indicate the operation type + if action := resolveFromQueryParameters(query, method, hasObject); action != "" { + return action + } + + // Priority 2: Handle basic operations based on method and resource type + // Only use the result if a specific action was resolved; otherwise fall through to Priority 3 + if hasObject { + if action := resolveObjectLevelAction(method, baseAction); action != "" { + return action + } + } else if bucket != "" { + if action := resolveBucketLevelAction(method, baseAction); action != "" { + return action + } + } + + // Priority 3: Fallback to legacy action mapping + return mapBaseActionToS3Format(baseAction) +} + +// bucketQueryActions maps bucket-level query parameters to their corresponding S3 actions by HTTP method +var bucketQueryActions = map[string]map[string]string{ + "policy": { + http.MethodGet: s3_constants.S3_ACTION_GET_BUCKET_POLICY, + http.MethodPut: s3_constants.S3_ACTION_PUT_BUCKET_POLICY, + http.MethodDelete: s3_constants.S3_ACTION_DELETE_BUCKET_POLICY, + }, + "cors": { + http.MethodGet: s3_constants.S3_ACTION_GET_BUCKET_CORS, + http.MethodPut: s3_constants.S3_ACTION_PUT_BUCKET_CORS, + http.MethodDelete: s3_constants.S3_ACTION_DELETE_BUCKET_CORS, + }, + "lifecycle": { + http.MethodGet: s3_constants.S3_ACTION_GET_BUCKET_LIFECYCLE, + http.MethodPut: s3_constants.S3_ACTION_PUT_BUCKET_LIFECYCLE, + http.MethodDelete: s3_constants.S3_ACTION_PUT_BUCKET_LIFECYCLE, // DELETE uses same permission as PUT + }, + "versioning": { + http.MethodGet: s3_constants.S3_ACTION_GET_BUCKET_VERSIONING, + http.MethodPut: s3_constants.S3_ACTION_PUT_BUCKET_VERSIONING, + }, + "notification": { + http.MethodGet: s3_constants.S3_ACTION_GET_BUCKET_NOTIFICATION, + http.MethodPut: s3_constants.S3_ACTION_PUT_BUCKET_NOTIFICATION, + }, + "object-lock": { + http.MethodGet: s3_constants.S3_ACTION_GET_BUCKET_OBJECT_LOCK, + http.MethodPut: s3_constants.S3_ACTION_PUT_BUCKET_OBJECT_LOCK, + }, +} + +// resolveFromQueryParameters checks query parameters to determine specific S3 actions +func resolveFromQueryParameters(query url.Values, method string, hasObject bool) string { + // Multipart upload operations with uploadId parameter (object-level only) + // All multipart operations require an object in the path + if hasObject && query.Has("uploadId") { + switch method { + case http.MethodPut: + if query.Has("partNumber") { + return s3_constants.S3_ACTION_UPLOAD_PART + } + case http.MethodPost: + return s3_constants.S3_ACTION_COMPLETE_MULTIPART + case http.MethodDelete: + return s3_constants.S3_ACTION_ABORT_MULTIPART + case http.MethodGet: + return s3_constants.S3_ACTION_LIST_PARTS + } + } + + // Multipart upload operations + // CreateMultipartUpload: POST /bucket/object?uploads (object-level) + // ListMultipartUploads: GET /bucket?uploads (bucket-level) + if query.Has("uploads") { + if method == http.MethodPost && hasObject { + return s3_constants.S3_ACTION_CREATE_MULTIPART + } else if method == http.MethodGet && !hasObject { + return s3_constants.S3_ACTION_LIST_MULTIPART_UPLOADS + } + } + + // ACL operations + if query.Has("acl") { + switch method { + case http.MethodGet, http.MethodHead: + if hasObject { + return s3_constants.S3_ACTION_GET_OBJECT_ACL + } + return s3_constants.S3_ACTION_GET_BUCKET_ACL + case http.MethodPut: + if hasObject { + return s3_constants.S3_ACTION_PUT_OBJECT_ACL + } + return s3_constants.S3_ACTION_PUT_BUCKET_ACL + } + } + + // Tagging operations + if query.Has("tagging") { + switch method { + case http.MethodGet: + if hasObject { + return s3_constants.S3_ACTION_GET_OBJECT_TAGGING + } + return s3_constants.S3_ACTION_GET_BUCKET_TAGGING + case http.MethodPut: + if hasObject { + return s3_constants.S3_ACTION_PUT_OBJECT_TAGGING + } + return s3_constants.S3_ACTION_PUT_BUCKET_TAGGING + case http.MethodDelete: + if hasObject { + return s3_constants.S3_ACTION_DELETE_OBJECT_TAGGING + } + return s3_constants.S3_ACTION_DELETE_BUCKET_TAGGING + } + } + + // Versioning operations - distinguish between versionId (specific version) and versions (list versions) + // versionId: Used to access/delete a specific version of an object (e.g., GET /bucket/key?versionId=xyz) + if query.Has("versionId") { + if hasObject { + switch method { + case http.MethodGet, http.MethodHead: + return s3_constants.S3_ACTION_GET_OBJECT_VERSION + case http.MethodDelete: + return s3_constants.S3_ACTION_DELETE_OBJECT_VERSION + } + } + } + + // versions: Used to list all versions of objects in a bucket (e.g., GET /bucket?versions) + if query.Has("versions") { + if method == http.MethodGet && !hasObject { + return s3_constants.S3_ACTION_LIST_BUCKET_VERSIONS + } + } + + // Check bucket-level query parameters using data-driven approach + // These are strictly bucket-level operations, so only apply when !hasObject + if !hasObject { + for param, actions := range bucketQueryActions { + if query.Has(param) { + if action, ok := actions[method]; ok { + return action + } + } + } + } + + // Location (GET only, bucket-level) + if query.Has("location") && method == http.MethodGet && !hasObject { + return s3_constants.S3_ACTION_GET_BUCKET_LOCATION + } + + // Object retention and legal hold operations (object-level only) + if hasObject { + if query.Has("retention") { + switch method { + case http.MethodGet: + return s3_constants.S3_ACTION_GET_OBJECT_RETENTION + case http.MethodPut: + return s3_constants.S3_ACTION_PUT_OBJECT_RETENTION + } + } + + if query.Has("legal-hold") { + switch method { + case http.MethodGet: + return s3_constants.S3_ACTION_GET_OBJECT_LEGAL_HOLD + case http.MethodPut: + return s3_constants.S3_ACTION_PUT_OBJECT_LEGAL_HOLD + } + } + } + + // Batch delete - POST request with delete query parameter (bucket-level operation) + // Example: POST /bucket?delete (not POST /bucket/object?delete) + if query.Has("delete") && method == http.MethodPost && !hasObject { + return s3_constants.S3_ACTION_DELETE_OBJECT + } + + return "" +} + +// resolveObjectLevelAction determines the S3 action for object-level operations +func resolveObjectLevelAction(method string, baseAction string) string { + switch method { + case http.MethodGet, http.MethodHead: + if baseAction == s3_constants.ACTION_READ { + return s3_constants.S3_ACTION_GET_OBJECT + } + + case http.MethodPut: + if baseAction == s3_constants.ACTION_WRITE { + // Note: CopyObject operations also use s3:PutObject permission (same as MinIO/AWS) + // Copy requires s3:PutObject on destination and s3:GetObject on source + return s3_constants.S3_ACTION_PUT_OBJECT + } + + case http.MethodDelete: + // CRITICAL: Map DELETE method to s3:DeleteObject + // This fixes the architectural limitation where ACTION_WRITE was mapped to s3:PutObject + if baseAction == s3_constants.ACTION_WRITE { + return s3_constants.S3_ACTION_DELETE_OBJECT + } + + case http.MethodPost: + // POST without query params is typically multipart or form upload + if baseAction == s3_constants.ACTION_WRITE { + return s3_constants.S3_ACTION_PUT_OBJECT + } + } + + return "" +} + +// resolveBucketLevelAction determines the S3 action for bucket-level operations +func resolveBucketLevelAction(method string, baseAction string) string { + switch method { + case http.MethodGet, http.MethodHead: + if baseAction == s3_constants.ACTION_LIST || baseAction == s3_constants.ACTION_READ { + return s3_constants.S3_ACTION_LIST_BUCKET + } + + case http.MethodPut: + if baseAction == s3_constants.ACTION_WRITE { + return s3_constants.S3_ACTION_CREATE_BUCKET + } + + case http.MethodDelete: + if baseAction == s3_constants.ACTION_DELETE_BUCKET { + return s3_constants.S3_ACTION_DELETE_BUCKET + } + + case http.MethodPost: + // POST to bucket is typically form upload + if baseAction == s3_constants.ACTION_WRITE { + return s3_constants.S3_ACTION_PUT_OBJECT + } + } + + return "" +} + +// mapBaseActionToS3Format converts coarse-grained base actions to S3 format +// This is the fallback when no specific resolution is found +func mapBaseActionToS3Format(baseAction string) string { + // Handle actions that already have s3: prefix + if strings.HasPrefix(baseAction, "s3:") { + return baseAction + } + + // Map coarse-grained actions to their most common S3 equivalent + // Note: The s3_constants values ARE the string values (e.g., ACTION_READ = "Read") + switch baseAction { + case s3_constants.ACTION_READ: // "Read" + return s3_constants.S3_ACTION_GET_OBJECT + case s3_constants.ACTION_WRITE: // "Write" + return s3_constants.S3_ACTION_PUT_OBJECT + case s3_constants.ACTION_LIST: // "List" + return s3_constants.S3_ACTION_LIST_BUCKET + case s3_constants.ACTION_TAGGING: // "Tagging" + return s3_constants.S3_ACTION_PUT_OBJECT_TAGGING + case s3_constants.ACTION_ADMIN: // "Admin" + return s3_constants.S3_ACTION_ALL + case s3_constants.ACTION_READ_ACP: // "ReadAcp" + return s3_constants.S3_ACTION_GET_OBJECT_ACL + case s3_constants.ACTION_WRITE_ACP: // "WriteAcp" + return s3_constants.S3_ACTION_PUT_OBJECT_ACL + case s3_constants.ACTION_DELETE_BUCKET: // "DeleteBucket" + return s3_constants.S3_ACTION_DELETE_BUCKET + case s3_constants.ACTION_BYPASS_GOVERNANCE_RETENTION: + return s3_constants.S3_ACTION_BYPASS_GOVERNANCE + case s3_constants.ACTION_GET_OBJECT_RETENTION: + return s3_constants.S3_ACTION_GET_OBJECT_RETENTION + case s3_constants.ACTION_PUT_OBJECT_RETENTION: + return s3_constants.S3_ACTION_PUT_OBJECT_RETENTION + case s3_constants.ACTION_GET_OBJECT_LEGAL_HOLD: + return s3_constants.S3_ACTION_GET_OBJECT_LEGAL_HOLD + case s3_constants.ACTION_PUT_OBJECT_LEGAL_HOLD: + return s3_constants.S3_ACTION_PUT_OBJECT_LEGAL_HOLD + case s3_constants.ACTION_GET_BUCKET_OBJECT_LOCK_CONFIG: + return s3_constants.S3_ACTION_GET_BUCKET_OBJECT_LOCK + case s3_constants.ACTION_PUT_BUCKET_OBJECT_LOCK_CONFIG: + return s3_constants.S3_ACTION_PUT_BUCKET_OBJECT_LOCK + default: + // For unknown actions, prefix with s3: to maintain format consistency + return "s3:" + baseAction + } +} diff --git a/weed/s3api/s3_constants/s3_action_strings.go b/weed/s3api/s3_constants/s3_action_strings.go new file mode 100644 index 000000000..c7d5541c9 --- /dev/null +++ b/weed/s3api/s3_constants/s3_action_strings.go @@ -0,0 +1,84 @@ +package s3_constants + +// S3 action strings for bucket policy evaluation +// These match the official AWS S3 action format used in IAM and bucket policies +const ( + // Object operations + S3_ACTION_GET_OBJECT = "s3:GetObject" + S3_ACTION_PUT_OBJECT = "s3:PutObject" + S3_ACTION_DELETE_OBJECT = "s3:DeleteObject" + S3_ACTION_DELETE_OBJECT_VERSION = "s3:DeleteObjectVersion" + S3_ACTION_GET_OBJECT_VERSION = "s3:GetObjectVersion" + + // Object ACL operations + S3_ACTION_GET_OBJECT_ACL = "s3:GetObjectAcl" + S3_ACTION_PUT_OBJECT_ACL = "s3:PutObjectAcl" + + // Object tagging operations + S3_ACTION_GET_OBJECT_TAGGING = "s3:GetObjectTagging" + S3_ACTION_PUT_OBJECT_TAGGING = "s3:PutObjectTagging" + S3_ACTION_DELETE_OBJECT_TAGGING = "s3:DeleteObjectTagging" + + // Object retention and legal hold + S3_ACTION_GET_OBJECT_RETENTION = "s3:GetObjectRetention" + S3_ACTION_PUT_OBJECT_RETENTION = "s3:PutObjectRetention" + S3_ACTION_GET_OBJECT_LEGAL_HOLD = "s3:GetObjectLegalHold" + S3_ACTION_PUT_OBJECT_LEGAL_HOLD = "s3:PutObjectLegalHold" + S3_ACTION_BYPASS_GOVERNANCE = "s3:BypassGovernanceRetention" + + // Multipart upload operations + S3_ACTION_CREATE_MULTIPART = "s3:CreateMultipartUpload" + S3_ACTION_UPLOAD_PART = "s3:UploadPart" + S3_ACTION_COMPLETE_MULTIPART = "s3:CompleteMultipartUpload" + S3_ACTION_ABORT_MULTIPART = "s3:AbortMultipartUpload" + S3_ACTION_LIST_PARTS = "s3:ListMultipartUploadParts" + + // Bucket operations + S3_ACTION_CREATE_BUCKET = "s3:CreateBucket" + S3_ACTION_DELETE_BUCKET = "s3:DeleteBucket" + S3_ACTION_LIST_BUCKET = "s3:ListBucket" + S3_ACTION_LIST_BUCKET_VERSIONS = "s3:ListBucketVersions" + S3_ACTION_LIST_MULTIPART_UPLOADS = "s3:ListBucketMultipartUploads" + + // Bucket ACL operations + S3_ACTION_GET_BUCKET_ACL = "s3:GetBucketAcl" + S3_ACTION_PUT_BUCKET_ACL = "s3:PutBucketAcl" + + // Bucket policy operations + S3_ACTION_GET_BUCKET_POLICY = "s3:GetBucketPolicy" + S3_ACTION_PUT_BUCKET_POLICY = "s3:PutBucketPolicy" + S3_ACTION_DELETE_BUCKET_POLICY = "s3:DeleteBucketPolicy" + + // Bucket tagging operations + S3_ACTION_GET_BUCKET_TAGGING = "s3:GetBucketTagging" + S3_ACTION_PUT_BUCKET_TAGGING = "s3:PutBucketTagging" + S3_ACTION_DELETE_BUCKET_TAGGING = "s3:DeleteBucketTagging" + + // Bucket CORS operations + S3_ACTION_GET_BUCKET_CORS = "s3:GetBucketCors" + S3_ACTION_PUT_BUCKET_CORS = "s3:PutBucketCors" + S3_ACTION_DELETE_BUCKET_CORS = "s3:DeleteBucketCors" + + // Bucket lifecycle operations + // Note: Both PUT and DELETE lifecycle operations use s3:PutLifecycleConfiguration + S3_ACTION_GET_BUCKET_LIFECYCLE = "s3:GetLifecycleConfiguration" + S3_ACTION_PUT_BUCKET_LIFECYCLE = "s3:PutLifecycleConfiguration" + + // Bucket versioning operations + S3_ACTION_GET_BUCKET_VERSIONING = "s3:GetBucketVersioning" + S3_ACTION_PUT_BUCKET_VERSIONING = "s3:PutBucketVersioning" + + // Bucket location + S3_ACTION_GET_BUCKET_LOCATION = "s3:GetBucketLocation" + + // Bucket notification + S3_ACTION_GET_BUCKET_NOTIFICATION = "s3:GetBucketNotification" + S3_ACTION_PUT_BUCKET_NOTIFICATION = "s3:PutBucketNotification" + + // Bucket object lock operations + S3_ACTION_GET_BUCKET_OBJECT_LOCK = "s3:GetBucketObjectLockConfiguration" + S3_ACTION_PUT_BUCKET_OBJECT_LOCK = "s3:PutBucketObjectLockConfiguration" + + // Wildcard for all S3 actions + S3_ACTION_ALL = "s3:*" +) diff --git a/weed/s3api/s3_granular_action_security_test.go b/weed/s3api/s3_granular_action_security_test.go index 404638d14..3def7e9d2 100644 --- a/weed/s3api/s3_granular_action_security_test.go +++ b/weed/s3api/s3_granular_action_security_test.go @@ -3,12 +3,49 @@ package s3api import ( "net/http" "net/url" + "strings" "testing" "github.com/seaweedfs/seaweedfs/weed/s3api/s3_constants" "github.com/stretchr/testify/assert" ) +// createTestRequestWithQueryParams creates a test HTTP request with query parameters +// and extracts bucket/object from the path. This helper reduces duplication in test setup. +func createTestRequestWithQueryParams(method, path string, queryParams map[string]string) (*http.Request, string, string, error) { + // Parse the URL + u, err := url.Parse(path) + if err != nil { + return nil, "", "", err + } + + // Add query parameters + q := u.Query() + for k, v := range queryParams { + q.Add(k, v) + } + u.RawQuery = q.Encode() + + // Create HTTP request + req, err := http.NewRequest(method, u.String(), nil) + if err != nil { + return nil, "", "", err + } + + // Parse path to extract bucket and object + parts := strings.Split(strings.TrimPrefix(u.Path, "/"), "/") + bucket := "" + object := "" + if len(parts) > 0 { + bucket = parts[0] + } + if len(parts) > 1 { + object = "/" + strings.Join(parts[1:], "/") + } + + return req, bucket, object, nil +} + // TestGranularActionMappingSecurity demonstrates how the new granular action mapping // fixes critical security issues that existed with the previous coarse mapping func TestGranularActionMappingSecurity(t *testing.T) { @@ -83,10 +120,10 @@ func TestGranularActionMappingSecurity(t *testing.T) { bucket: "inventory-bucket", objectKey: "", queryParams: map[string]string{"uploads": ""}, - description: "Listing multipart uploads should map to s3:ListMultipartUploads", + description: "Listing multipart uploads should map to s3:ListBucketMultipartUploads", problemWithOldMapping: "Old mapping would use generic s3:ListBucket for all bucket operations, " + "preventing fine-grained control over who can see ongoing multipart operations", - granularActionResult: "s3:ListMultipartUploads", + granularActionResult: "s3:ListBucketMultipartUploads", }, { name: "delete_object_tagging_precision", @@ -116,8 +153,8 @@ func TestGranularActionMappingSecurity(t *testing.T) { } req.URL.RawQuery = query.Encode() - // Test the new granular action determination - result := determineGranularS3Action(req, s3_constants.ACTION_WRITE, tt.bucket, tt.objectKey) + // Test the granular action determination + result := ResolveS3Action(req, string(s3_constants.ACTION_WRITE), tt.bucket, tt.objectKey) assert.Equal(t, tt.granularActionResult, result, "Security Fix Test: %s\n"+ @@ -191,7 +228,7 @@ func TestBackwardCompatibilityFallback(t *testing.T) { URL: &url.URL{Path: "/" + tt.bucket + "/" + tt.objectKey}, } - result := determineGranularS3Action(req, tt.fallbackAction, tt.bucket, tt.objectKey) + result := ResolveS3Action(req, string(tt.fallbackAction), tt.bucket, tt.objectKey) assert.Equal(t, tt.expectedResult, result, "Backward Compatibility Test: %s\nDescription: %s\nExpected: %s, Got: %s", @@ -292,16 +329,281 @@ func TestPolicyEnforcementScenarios(t *testing.T) { } req.URL.RawQuery = query.Encode() - result := determineGranularS3Action(req, s3_constants.ACTION_WRITE, scenario.bucket, scenario.objectKey) + result := ResolveS3Action(req, string(s3_constants.ACTION_WRITE), scenario.bucket, scenario.objectKey) assert.Equal(t, scenario.expectedAction, result, "Policy Enforcement Scenario: %s\nExpected Action: %s, Got: %s", scenario.name, scenario.expectedAction, result) - t.Logf("🔒 SECURITY SCENARIO: %s", scenario.name) + t.Logf("SECURITY SCENARIO: %s", scenario.name) t.Logf(" Expected Action: %s", result) t.Logf(" Security Benefit: %s", scenario.securityBenefit) t.Logf(" Policy Example:\n%s", scenario.policyExample) }) } } + +// TestDeleteObjectPolicyEnforcement demonstrates that the architectural limitation has been fixed +// Previously, DeleteObject operations were mapped to s3:PutObject, preventing fine-grained policies from working +func TestDeleteObjectPolicyEnforcement(t *testing.T) { + tests := []struct { + name string + method string + bucket string + objectKey string + baseAction Action + expectedS3Action string + policyScenario string + }{ + { + name: "delete_object_maps_to_correct_action", + method: http.MethodDelete, + bucket: "test-bucket", + objectKey: "test-object.txt", + baseAction: s3_constants.ACTION_WRITE, + expectedS3Action: "s3:DeleteObject", + policyScenario: "Policy that denies s3:DeleteObject but allows s3:PutObject should now work correctly", + }, + { + name: "put_object_maps_to_correct_action", + method: http.MethodPut, + bucket: "test-bucket", + objectKey: "test-object.txt", + baseAction: s3_constants.ACTION_WRITE, + expectedS3Action: "s3:PutObject", + policyScenario: "Policy that allows s3:PutObject but denies s3:DeleteObject should allow uploads", + }, + { + name: "batch_delete_maps_to_delete_action", + method: http.MethodPost, + bucket: "test-bucket", + objectKey: "", + baseAction: s3_constants.ACTION_WRITE, + expectedS3Action: "s3:DeleteObject", + policyScenario: "Batch delete operations should also map to s3:DeleteObject", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Create HTTP request + req := &http.Request{ + Method: tt.method, + URL: &url.URL{Path: "/" + tt.bucket + "/" + tt.objectKey}, + Header: http.Header{}, + } + + // For batch delete, add the delete query parameter + if tt.method == http.MethodPost && tt.expectedS3Action == "s3:DeleteObject" { + query := req.URL.Query() + query.Set("delete", "") + req.URL.RawQuery = query.Encode() + } + + // Test the action resolution + result := ResolveS3Action(req, string(tt.baseAction), tt.bucket, tt.objectKey) + + assert.Equal(t, tt.expectedS3Action, result, + "Action Resolution Test: %s\n"+ + "HTTP Method: %s\n"+ + "Base Action: %s\n"+ + "Policy Scenario: %s\n"+ + "Expected: %s, Got: %s", + tt.name, tt.method, tt.baseAction, tt.policyScenario, tt.expectedS3Action, result) + + t.Logf("ARCHITECTURAL FIX VERIFIED: %s", tt.name) + t.Logf(" Method: %s -> S3 Action: %s", tt.method, result) + t.Logf(" Policy Scenario: %s", tt.policyScenario) + }) + } +} + +// TestFineGrainedPolicyExample demonstrates a real-world use case that now works +// This test verifies the exact scenario described in the original TODO comment +func TestFineGrainedPolicyExample(t *testing.T) { + // Example policy: Allow PutObject but Deny DeleteObject + // This is a common pattern for "append-only" buckets or write-once scenarios + policyExample := `{ + "Version": "2012-10-17", + "Statement": [ + { + "Sid": "AllowObjectUploads", + "Effect": "Allow", + "Action": "s3:PutObject", + "Resource": "arn:aws:s3:::test-bucket/*" + }, + { + "Sid": "DenyObjectDeletion", + "Effect": "Deny", + "Action": "s3:DeleteObject", + "Resource": "arn:aws:s3:::test-bucket/*" + } + ] + }` + + testCases := []struct { + operation string + method string + objectKey string + queryParams map[string]string + baseAction Action + expectedAction string + shouldBeAllowed bool + rationale string + }{ + { + operation: "PUT object", + method: http.MethodPut, + objectKey: "document.txt", + queryParams: map[string]string{}, + baseAction: s3_constants.ACTION_WRITE, + expectedAction: "s3:PutObject", + shouldBeAllowed: true, + rationale: "Policy explicitly allows s3:PutObject - upload should succeed", + }, + { + operation: "DELETE object", + method: http.MethodDelete, + objectKey: "document.txt", + queryParams: map[string]string{}, + baseAction: s3_constants.ACTION_WRITE, + expectedAction: "s3:DeleteObject", + shouldBeAllowed: false, + rationale: "Policy explicitly denies s3:DeleteObject - deletion should be blocked", + }, + { + operation: "Batch DELETE", + method: http.MethodPost, + objectKey: "", + queryParams: map[string]string{"delete": ""}, + baseAction: s3_constants.ACTION_WRITE, + expectedAction: "s3:DeleteObject", + shouldBeAllowed: false, + rationale: "Policy explicitly denies s3:DeleteObject - batch deletion should be blocked", + }, + } + + t.Logf("\nTesting Fine-Grained Policy:") + t.Logf("%s\n", policyExample) + + for _, tc := range testCases { + t.Run(tc.operation, func(t *testing.T) { + // Create HTTP request + req := &http.Request{ + Method: tc.method, + URL: &url.URL{Path: "/test-bucket/" + tc.objectKey}, + Header: http.Header{}, + } + + // Add query parameters + query := req.URL.Query() + for key, value := range tc.queryParams { + query.Set(key, value) + } + req.URL.RawQuery = query.Encode() + + // Determine the S3 action + actualAction := ResolveS3Action(req, string(tc.baseAction), "test-bucket", tc.objectKey) + + // Verify the action mapping is correct + assert.Equal(t, tc.expectedAction, actualAction, + "Operation: %s\nExpected Action: %s\nGot: %s", + tc.operation, tc.expectedAction, actualAction) + + // Log the result + allowStatus := "[DENIED]" + if tc.shouldBeAllowed { + allowStatus = "[ALLOWED]" + } + + t.Logf("%s %s -> %s", allowStatus, tc.operation, actualAction) + t.Logf(" Rationale: %s", tc.rationale) + }) + } + + t.Logf("\nARCHITECTURAL LIMITATION RESOLVED!") + t.Logf(" Fine-grained policies like 'allow PUT but deny DELETE' now work correctly") + t.Logf(" The policy engine can distinguish between s3:PutObject and s3:DeleteObject") +} + +// TestCoarseActionResolution verifies that ResolveS3Action correctly maps +// coarse-grained ACTION_WRITE to specific S3 actions based on HTTP context. +// This demonstrates the fix for the architectural limitation where ACTION_WRITE +// was always mapped to s3:PutObject, preventing fine-grained policies from working. +func TestCoarseActionResolution(t *testing.T) { + testCases := []struct { + name string + method string + path string + queryParams map[string]string + coarseAction Action + expectedS3Action string + policyScenario string + }{ + { + name: "PUT_with_ACTION_WRITE_resolves_to_PutObject", + method: http.MethodPut, + path: "/test-bucket/test-file.txt", + queryParams: map[string]string{}, + coarseAction: s3_constants.ACTION_WRITE, + expectedS3Action: "s3:PutObject", + policyScenario: "Policy allowing s3:PutObject should match PUT requests", + }, + { + name: "DELETE_with_ACTION_WRITE_resolves_to_DeleteObject", + method: http.MethodDelete, + path: "/test-bucket/test-file.txt", + queryParams: map[string]string{}, + coarseAction: s3_constants.ACTION_WRITE, + expectedS3Action: "s3:DeleteObject", + policyScenario: "Policy denying s3:DeleteObject should block DELETE requests", + }, + { + name: "batch_DELETE_with_ACTION_WRITE_resolves_to_DeleteObject", + method: http.MethodPost, + path: "/test-bucket", + queryParams: map[string]string{"delete": ""}, + coarseAction: s3_constants.ACTION_WRITE, + expectedS3Action: "s3:DeleteObject", + policyScenario: "Policy denying s3:DeleteObject should block batch DELETE", + }, + { + name: "POST_multipart_with_ACTION_WRITE_resolves_to_CreateMultipartUpload", + method: http.MethodPost, + path: "/test-bucket/large-file.mp4", + queryParams: map[string]string{"uploads": ""}, + coarseAction: s3_constants.ACTION_WRITE, + expectedS3Action: "s3:CreateMultipartUpload", + policyScenario: "Policy allowing s3:PutObject but denying s3:CreateMultipartUpload can now work", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + // Create test request with query parameters and extract bucket/object + req, bucket, object, err := createTestRequestWithQueryParams(tc.method, tc.path, tc.queryParams) + assert.NoError(t, err) + + // Call ResolveS3Action with coarse action constant + resolvedAction := ResolveS3Action(req, string(tc.coarseAction), bucket, object) + + // Verify correct S3 action is resolved + assert.Equal(t, tc.expectedS3Action, resolvedAction, + "Coarse action %s with method %s should resolve to %s", + tc.coarseAction, tc.method, tc.expectedS3Action) + + t.Logf("SUCCESS: %s", tc.name) + t.Logf(" Input: %s %s + ACTION_WRITE", tc.method, tc.path) + t.Logf(" Output: %s", resolvedAction) + t.Logf(" Policy impact: %s", tc.policyScenario) + }) + } + + t.Log("\n=== ARCHITECTURAL LIMITATION RESOLVED ===") + t.Log("Handlers can use coarse ACTION_WRITE constant, and the context-aware") + t.Log("resolver will map it to the correct specific S3 action (PutObject,") + t.Log("DeleteObject, CreateMultipartUpload, etc.) based on HTTP method and") + t.Log("query parameters. This enables fine-grained bucket policies like:") + t.Log(" - Allow s3:PutObject but Deny s3:DeleteObject (append-only)") + t.Log(" - Allow regular uploads but Deny multipart (size limits)") +} diff --git a/weed/s3api/s3_iam_middleware.go b/weed/s3api/s3_iam_middleware.go index 230b2d2cb..4cb14490a 100644 --- a/weed/s3api/s3_iam_middleware.go +++ b/weed/s3api/s3_iam_middleware.go @@ -184,7 +184,7 @@ func (s3iam *S3IAMIntegration) AuthorizeAction(ctx context.Context, identity *IA requestContext := extractRequestContext(r) // Determine the specific S3 action based on the HTTP request details - specificAction := determineGranularS3Action(r, action, bucket, objectKey) + specificAction := ResolveS3Action(r, string(action), bucket, objectKey) // Create action request actionRequest := &integration.ActionRequest{ @@ -246,176 +246,11 @@ func buildS3ResourceArn(bucket string, objectKey string) string { } // Remove leading slash from object key if present - if strings.HasPrefix(objectKey, "/") { - objectKey = objectKey[1:] - } + objectKey = strings.TrimPrefix(objectKey, "/") return "arn:aws:s3:::" + bucket + "/" + objectKey } -// determineGranularS3Action determines the specific S3 IAM action based on HTTP request details -// This provides granular, operation-specific actions for accurate IAM policy enforcement -func determineGranularS3Action(r *http.Request, fallbackAction Action, bucket string, objectKey string) string { - method := r.Method - query := r.URL.Query() - - // Check if there are specific query parameters indicating granular operations - // If there are, always use granular mapping regardless of method-action alignment - hasGranularIndicators := hasSpecificQueryParameters(query) - - // Only check for method-action mismatch when there are NO granular indicators - // This provides fallback behavior for cases where HTTP method doesn't align with intended action - if !hasGranularIndicators && isMethodActionMismatch(method, fallbackAction) { - return mapLegacyActionToIAM(fallbackAction) - } - - // Handle object-level operations when method and action are aligned - if objectKey != "" && objectKey != "/" { - switch method { - case "GET", "HEAD": - // Object read operations - check for specific query parameters - if _, hasAcl := query["acl"]; hasAcl { - return "s3:GetObjectAcl" - } - if _, hasTagging := query["tagging"]; hasTagging { - return "s3:GetObjectTagging" - } - if _, hasRetention := query["retention"]; hasRetention { - return "s3:GetObjectRetention" - } - if _, hasLegalHold := query["legal-hold"]; hasLegalHold { - return "s3:GetObjectLegalHold" - } - if _, hasVersions := query["versions"]; hasVersions { - return "s3:GetObjectVersion" - } - if _, hasUploadId := query["uploadId"]; hasUploadId { - return "s3:ListParts" - } - // Default object read - return "s3:GetObject" - - case "PUT", "POST": - // Object write operations - check for specific query parameters - if _, hasAcl := query["acl"]; hasAcl { - return "s3:PutObjectAcl" - } - if _, hasTagging := query["tagging"]; hasTagging { - return "s3:PutObjectTagging" - } - if _, hasRetention := query["retention"]; hasRetention { - return "s3:PutObjectRetention" - } - if _, hasLegalHold := query["legal-hold"]; hasLegalHold { - return "s3:PutObjectLegalHold" - } - // Check for multipart upload operations - if _, hasUploads := query["uploads"]; hasUploads { - return "s3:CreateMultipartUpload" - } - if _, hasUploadId := query["uploadId"]; hasUploadId { - if _, hasPartNumber := query["partNumber"]; hasPartNumber { - return "s3:UploadPart" - } - return "s3:CompleteMultipartUpload" // Complete multipart upload - } - // Default object write - return "s3:PutObject" - - case "DELETE": - // Object delete operations - if _, hasTagging := query["tagging"]; hasTagging { - return "s3:DeleteObjectTagging" - } - if _, hasUploadId := query["uploadId"]; hasUploadId { - return "s3:AbortMultipartUpload" - } - // Default object delete - return "s3:DeleteObject" - } - } - - // Handle bucket-level operations - if bucket != "" { - switch method { - case "GET", "HEAD": - // Bucket read operations - check for specific query parameters - if _, hasAcl := query["acl"]; hasAcl { - return "s3:GetBucketAcl" - } - if _, hasPolicy := query["policy"]; hasPolicy { - return "s3:GetBucketPolicy" - } - if _, hasTagging := query["tagging"]; hasTagging { - return "s3:GetBucketTagging" - } - if _, hasCors := query["cors"]; hasCors { - return "s3:GetBucketCors" - } - if _, hasVersioning := query["versioning"]; hasVersioning { - return "s3:GetBucketVersioning" - } - if _, hasNotification := query["notification"]; hasNotification { - return "s3:GetBucketNotification" - } - if _, hasObjectLock := query["object-lock"]; hasObjectLock { - return "s3:GetBucketObjectLockConfiguration" - } - if _, hasUploads := query["uploads"]; hasUploads { - return "s3:ListMultipartUploads" - } - if _, hasVersions := query["versions"]; hasVersions { - return "s3:ListBucketVersions" - } - // Default bucket read/list - return "s3:ListBucket" - - case "PUT": - // Bucket write operations - check for specific query parameters - if _, hasAcl := query["acl"]; hasAcl { - return "s3:PutBucketAcl" - } - if _, hasPolicy := query["policy"]; hasPolicy { - return "s3:PutBucketPolicy" - } - if _, hasTagging := query["tagging"]; hasTagging { - return "s3:PutBucketTagging" - } - if _, hasCors := query["cors"]; hasCors { - return "s3:PutBucketCors" - } - if _, hasVersioning := query["versioning"]; hasVersioning { - return "s3:PutBucketVersioning" - } - if _, hasNotification := query["notification"]; hasNotification { - return "s3:PutBucketNotification" - } - if _, hasObjectLock := query["object-lock"]; hasObjectLock { - return "s3:PutBucketObjectLockConfiguration" - } - // Default bucket creation - return "s3:CreateBucket" - - case "DELETE": - // Bucket delete operations - check for specific query parameters - if _, hasPolicy := query["policy"]; hasPolicy { - return "s3:DeleteBucketPolicy" - } - if _, hasTagging := query["tagging"]; hasTagging { - return "s3:DeleteBucketTagging" - } - if _, hasCors := query["cors"]; hasCors { - return "s3:DeleteBucketCors" - } - // Default bucket delete - return "s3:DeleteBucket" - } - } - - // Fallback to legacy mapping for specific known actions - return mapLegacyActionToIAM(fallbackAction) -} - // hasSpecificQueryParameters checks if the request has query parameters that indicate specific granular operations func hasSpecificQueryParameters(query url.Values) bool { // Check for object-level operation indicators @@ -525,9 +360,9 @@ func mapLegacyActionToIAM(legacyAction Action) string { case s3_constants.ACTION_ABORT_MULTIPART: return "s3:AbortMultipartUpload" case s3_constants.ACTION_LIST_MULTIPART_UPLOADS: - return "s3:ListMultipartUploads" + return s3_constants.S3_ACTION_LIST_MULTIPART_UPLOADS case s3_constants.ACTION_LIST_PARTS: - return "s3:ListParts" + return s3_constants.S3_ACTION_LIST_PARTS default: // If it's already a properly formatted S3 action, return as-is diff --git a/weed/s3api/s3_iam_simple_test.go b/weed/s3api/s3_iam_simple_test.go index 36691bb8f..41dbbbed8 100644 --- a/weed/s3api/s3_iam_simple_test.go +++ b/weed/s3api/s3_iam_simple_test.go @@ -294,7 +294,7 @@ func TestDetermineGranularS3Action(t *testing.T) { objectKey: "", queryParams: map[string]string{"uploads": ""}, fallbackAction: s3_constants.ACTION_LIST, - expected: "s3:ListMultipartUploads", + expected: "s3:ListBucketMultipartUploads", description: "List multipart uploads in bucket", }, @@ -336,8 +336,8 @@ func TestDetermineGranularS3Action(t *testing.T) { } req.URL.RawQuery = query.Encode() - // Test the granular action determination - result := determineGranularS3Action(req, tt.fallbackAction, tt.bucket, tt.objectKey) + // Test the action determination + result := ResolveS3Action(req, string(tt.fallbackAction), tt.bucket, tt.objectKey) assert.Equal(t, tt.expected, result, "Test %s failed: %s. Expected %s but got %s", diff --git a/weed/s3api/s3_list_parts_action_test.go b/weed/s3api/s3_list_parts_action_test.go index c0e9aa8a1..e83be9be7 100644 --- a/weed/s3api/s3_list_parts_action_test.go +++ b/weed/s3api/s3_list_parts_action_test.go @@ -39,8 +39,8 @@ func TestListPartsActionMapping(t *testing.T) { objectKey: "test-object.txt", queryParams: map[string]string{"uploadId": "test-upload-id"}, fallbackAction: s3_constants.ACTION_READ, - expectedAction: "s3:ListParts", - description: "GET request with uploadId should map to s3:ListParts (this was the missing mapping)", + expectedAction: "s3:ListMultipartUploadParts", + description: "GET request with uploadId should map to s3:ListMultipartUploadParts (this was the missing mapping)", }, { name: "get_object_with_uploadId_and_other_params", @@ -53,18 +53,18 @@ func TestListPartsActionMapping(t *testing.T) { "part-number-marker": "50", }, fallbackAction: s3_constants.ACTION_READ, - expectedAction: "s3:ListParts", - description: "GET request with uploadId plus other multipart params should map to s3:ListParts", + expectedAction: "s3:ListMultipartUploadParts", + description: "GET request with uploadId plus other multipart params should map to s3:ListMultipartUploadParts", }, { - name: "get_object_versions", + name: "get_object_with_versionId", method: "GET", bucket: "test-bucket", objectKey: "test-object.txt", - queryParams: map[string]string{"versions": ""}, + queryParams: map[string]string{"versionId": "version-123"}, fallbackAction: s3_constants.ACTION_READ, expectedAction: "s3:GetObjectVersion", - description: "GET request with versions should still map to s3:GetObjectVersion (precedence check)", + description: "GET request with versionId should map to s3:GetObjectVersion", }, { name: "get_object_acl_without_uploadId", @@ -103,8 +103,8 @@ func TestListPartsActionMapping(t *testing.T) { } req.URL.RawQuery = query.Encode() - // Call the granular action determination function - action := determineGranularS3Action(req, tc.fallbackAction, tc.bucket, tc.objectKey) + // Call the action resolver directly + action := ResolveS3Action(req, string(tc.fallbackAction), tc.bucket, tc.objectKey) // Verify the action mapping assert.Equal(t, tc.expectedAction, action, @@ -127,17 +127,17 @@ func TestListPartsActionMappingSecurityScenarios(t *testing.T) { query1 := req1.URL.Query() query1.Set("uploadId", "active-upload-123") req1.URL.RawQuery = query1.Encode() - action1 := determineGranularS3Action(req1, s3_constants.ACTION_READ, "secure-bucket", "confidential-document.pdf") + action1 := ResolveS3Action(req1, string(s3_constants.ACTION_READ), "secure-bucket", "confidential-document.pdf") // Test request 2: Get object without uploadId req2 := &http.Request{ Method: "GET", URL: &url.URL{Path: "/secure-bucket/confidential-document.pdf"}, } - action2 := determineGranularS3Action(req2, s3_constants.ACTION_READ, "secure-bucket", "confidential-document.pdf") + action2 := ResolveS3Action(req2, string(s3_constants.ACTION_READ), "secure-bucket", "confidential-document.pdf") // These should be different actions, allowing different permissions - assert.Equal(t, "s3:ListParts", action1, "Listing multipart parts should require s3:ListParts permission") + assert.Equal(t, "s3:ListMultipartUploadParts", action1, "Listing multipart parts should require s3:ListMultipartUploadParts permission") assert.Equal(t, "s3:GetObject", action2, "Reading object content should require s3:GetObject permission") assert.NotEqual(t, action1, action2, "ListParts and GetObject should be separate permissions for security") }) @@ -155,8 +155,8 @@ func TestListPartsActionMappingSecurityScenarios(t *testing.T) { { description: "List multipart upload parts", queryParams: map[string]string{"uploadId": "upload-abc123"}, - expectedAction: "s3:ListParts", - securityNote: "FIXED: Now correctly maps to s3:ListParts instead of s3:GetObject", + expectedAction: "s3:ListMultipartUploadParts", + securityNote: "FIXED: Now correctly maps to s3:ListMultipartUploadParts instead of s3:GetObject", }, { description: "Get actual object content", @@ -167,7 +167,7 @@ func TestListPartsActionMappingSecurityScenarios(t *testing.T) { { description: "Get object with complex upload ID", queryParams: map[string]string{"uploadId": "complex-upload-id-with-hyphens-123-abc-def"}, - expectedAction: "s3:ListParts", + expectedAction: "s3:ListMultipartUploadParts", securityNote: "FIXED: Complex upload IDs now correctly detected", }, } @@ -184,7 +184,7 @@ func TestListPartsActionMappingSecurityScenarios(t *testing.T) { } req.URL.RawQuery = query.Encode() - action := determineGranularS3Action(req, s3_constants.ACTION_READ, "test-bucket", "test-object") + action := ResolveS3Action(req, string(s3_constants.ACTION_READ), "test-bucket", "test-object") assert.Equal(t, tc.expectedAction, action, "%s - %s", tc.description, tc.securityNote) @@ -205,7 +205,7 @@ func TestListPartsActionRealWorldScenarios(t *testing.T) { query1 := req1.URL.Query() query1.Set("uploads", "") req1.URL.RawQuery = query1.Encode() - action1 := determineGranularS3Action(req1, s3_constants.ACTION_WRITE, "data", "large-dataset.csv") + action1 := ResolveS3Action(req1, string(s3_constants.ACTION_WRITE), "data", "large-dataset.csv") // Step 2: List existing parts (GET with uploadId query) - THIS WAS THE MISSING MAPPING req2 := &http.Request{ @@ -215,7 +215,7 @@ func TestListPartsActionRealWorldScenarios(t *testing.T) { query2 := req2.URL.Query() query2.Set("uploadId", "dataset-upload-20240827-001") req2.URL.RawQuery = query2.Encode() - action2 := determineGranularS3Action(req2, s3_constants.ACTION_READ, "data", "large-dataset.csv") + action2 := ResolveS3Action(req2, string(s3_constants.ACTION_READ), "data", "large-dataset.csv") // Step 3: Upload a part (PUT with uploadId and partNumber) req3 := &http.Request{ @@ -226,7 +226,7 @@ func TestListPartsActionRealWorldScenarios(t *testing.T) { query3.Set("uploadId", "dataset-upload-20240827-001") query3.Set("partNumber", "5") req3.URL.RawQuery = query3.Encode() - action3 := determineGranularS3Action(req3, s3_constants.ACTION_WRITE, "data", "large-dataset.csv") + action3 := ResolveS3Action(req3, string(s3_constants.ACTION_WRITE), "data", "large-dataset.csv") // Step 4: Complete multipart upload (POST with uploadId) req4 := &http.Request{ @@ -236,11 +236,11 @@ func TestListPartsActionRealWorldScenarios(t *testing.T) { query4 := req4.URL.Query() query4.Set("uploadId", "dataset-upload-20240827-001") req4.URL.RawQuery = query4.Encode() - action4 := determineGranularS3Action(req4, s3_constants.ACTION_WRITE, "data", "large-dataset.csv") + action4 := ResolveS3Action(req4, string(s3_constants.ACTION_WRITE), "data", "large-dataset.csv") // Verify each step has the correct action mapping assert.Equal(t, "s3:CreateMultipartUpload", action1, "Step 1: Initiate upload") - assert.Equal(t, "s3:ListParts", action2, "Step 2: List parts (FIXED by this PR)") + assert.Equal(t, "s3:ListMultipartUploadParts", action2, "Step 2: List parts (FIXED by this PR)") assert.Equal(t, "s3:UploadPart", action3, "Step 3: Upload part") assert.Equal(t, "s3:CompleteMultipartUpload", action4, "Step 4: Complete upload") @@ -277,10 +277,10 @@ func TestListPartsActionRealWorldScenarios(t *testing.T) { query.Set("uploadId", uploadId) req.URL.RawQuery = query.Encode() - action := determineGranularS3Action(req, s3_constants.ACTION_READ, "test-bucket", "test-file.bin") + action := ResolveS3Action(req, string(s3_constants.ACTION_READ), "test-bucket", "test-file.bin") - assert.Equal(t, "s3:ListParts", action, - "Upload ID format %s should be correctly detected and mapped to s3:ListParts", uploadId) + assert.Equal(t, "s3:ListMultipartUploadParts", action, + "Upload ID format %s should be correctly detected and mapped to s3:ListMultipartUploadParts", uploadId) } }) } diff --git a/weed/s3api/s3_multipart_iam_test.go b/weed/s3api/s3_multipart_iam_test.go index 608d30042..725bd0304 100644 --- a/weed/s3api/s3_multipart_iam_test.go +++ b/weed/s3api/s3_multipart_iam_test.go @@ -546,8 +546,8 @@ func setupTestRolesForMultipart(ctx context.Context, manager *integration.IAMMan "s3:UploadPart", "s3:CompleteMultipartUpload", "s3:AbortMultipartUpload", - "s3:ListMultipartUploads", - "s3:ListParts", + "s3:ListBucketMultipartUploads", + "s3:ListMultipartUploadParts", }, Resource: []string{ "arn:aws:s3:::*", diff --git a/weed/s3api/s3api_bucket_handlers.go b/weed/s3api/s3api_bucket_handlers.go index 6ccf82e27..5ebb06b21 100644 --- a/weed/s3api/s3api_bucket_handlers.go +++ b/weed/s3api/s3api_bucket_handlers.go @@ -610,7 +610,8 @@ func (s3a *S3ApiServer) AuthWithPublicRead(handler http.HandlerFunc, action Acti // Check bucket policy for anonymous access using the policy engine principal := "*" // Anonymous principal - allowed, evaluated, err := s3a.policyEngine.EvaluatePolicy(bucket, object, string(action), principal) + // Use context-aware policy evaluation to get the correct S3 action + allowed, evaluated, err := s3a.policyEngine.EvaluatePolicyWithContext(bucket, object, string(action), principal, r) if err != nil { // SECURITY: Fail-close on policy evaluation errors // If we can't evaluate the policy, deny access rather than falling through to IAM diff --git a/weed/s3api/s3api_bucket_policy_engine.go b/weed/s3api/s3api_bucket_policy_engine.go index ca1093178..278e3e1ae 100644 --- a/weed/s3api/s3api_bucket_policy_engine.go +++ b/weed/s3api/s3api_bucket_policy_engine.go @@ -3,13 +3,12 @@ package s3api import ( "encoding/json" "fmt" - "strings" + "net/http" "github.com/seaweedfs/seaweedfs/weed/glog" "github.com/seaweedfs/seaweedfs/weed/iam/policy" "github.com/seaweedfs/seaweedfs/weed/pb/filer_pb" "github.com/seaweedfs/seaweedfs/weed/s3api/policy_engine" - "github.com/seaweedfs/seaweedfs/weed/s3api/s3_constants" ) // BucketPolicyEngine wraps the policy_engine to provide bucket policy evaluation @@ -102,8 +101,8 @@ func (bpe *BucketPolicyEngine) EvaluatePolicy(bucket, object, action, principal return false, false, fmt.Errorf("action cannot be empty") } - // Convert action to S3 action format - s3Action := convertActionToS3Format(action) + // Convert action to S3 action format using base mapping (no HTTP context available) + s3Action := mapBaseActionToS3Format(action) // Build resource ARN resource := buildResourceARN(bucket, object) @@ -135,72 +134,52 @@ func (bpe *BucketPolicyEngine) EvaluatePolicy(bucket, object, action, principal } } -// convertActionToS3Format converts internal action strings to S3 action format -// -// KNOWN LIMITATION: The current Action type uses coarse-grained constants -// (ACTION_READ, ACTION_WRITE, etc.) that map to specific S3 actions, but these -// are used for multiple operations. For example, ACTION_WRITE is used for both -// PutObject and DeleteObject, but this function maps it to only s3:PutObject. -// This means bucket policies requiring fine-grained permissions (e.g., allowing -// s3:DeleteObject but not s3:PutObject) will not work correctly. -// -// TODO: Refactor to use specific S3 action strings throughout the S3 API handlers -// instead of coarse-grained Action constants. This is a major architectural change -// that should be done in a separate PR. -// -// This function explicitly maps all known actions to prevent security issues from -// overly permissive default behavior. -func convertActionToS3Format(action string) string { - // Handle multipart actions that already have s3: prefix - if strings.HasPrefix(action, "s3:") { - return action +// EvaluatePolicyWithContext evaluates whether an action is allowed by bucket policy using HTTP request context +// This version uses the HTTP request to determine the actual S3 action more accurately +func (bpe *BucketPolicyEngine) EvaluatePolicyWithContext(bucket, object, action, principal string, r *http.Request) (allowed bool, evaluated bool, err error) { + // Validate required parameters + if bucket == "" { + return false, false, fmt.Errorf("bucket cannot be empty") + } + if action == "" { + return false, false, fmt.Errorf("action cannot be empty") } - // Explicit mapping for all known actions - switch action { - // Basic operations - case s3_constants.ACTION_READ: - return "s3:GetObject" - case s3_constants.ACTION_WRITE: - return "s3:PutObject" - case s3_constants.ACTION_LIST: - return "s3:ListBucket" - case s3_constants.ACTION_TAGGING: - return "s3:PutObjectTagging" - case s3_constants.ACTION_ADMIN: - return "s3:*" + // Convert action to S3 action format using request context + // ResolveS3Action handles nil request internally (falls back to mapBaseActionToS3Format) + s3Action := ResolveS3Action(r, action, bucket, object) - // ACL operations - case s3_constants.ACTION_READ_ACP: - return "s3:GetObjectAcl" - case s3_constants.ACTION_WRITE_ACP: - return "s3:PutObjectAcl" + // Build resource ARN + resource := buildResourceARN(bucket, object) - // Bucket operations - case s3_constants.ACTION_DELETE_BUCKET: - return "s3:DeleteBucket" + glog.V(4).Infof("EvaluatePolicyWithContext: bucket=%s, resource=%s, action=%s (from %s), principal=%s", + bucket, resource, s3Action, action, principal) - // Object Lock operations - case s3_constants.ACTION_BYPASS_GOVERNANCE_RETENTION: - return "s3:BypassGovernanceRetention" - case s3_constants.ACTION_GET_OBJECT_RETENTION: - return "s3:GetObjectRetention" - case s3_constants.ACTION_PUT_OBJECT_RETENTION: - return "s3:PutObjectRetention" - case s3_constants.ACTION_GET_OBJECT_LEGAL_HOLD: - return "s3:GetObjectLegalHold" - case s3_constants.ACTION_PUT_OBJECT_LEGAL_HOLD: - return "s3:PutObjectLegalHold" - case s3_constants.ACTION_GET_BUCKET_OBJECT_LOCK_CONFIG: - return "s3:GetBucketObjectLockConfiguration" - case s3_constants.ACTION_PUT_BUCKET_OBJECT_LOCK_CONFIG: - return "s3:PutBucketObjectLockConfiguration" + // Evaluate using the policy engine + args := &policy_engine.PolicyEvaluationArgs{ + Action: s3Action, + Resource: resource, + Principal: principal, + } + result := bpe.engine.EvaluatePolicy(bucket, args) + + switch result { + case policy_engine.PolicyResultAllow: + glog.V(3).Infof("EvaluatePolicyWithContext: ALLOW - bucket=%s, action=%s, principal=%s", bucket, s3Action, principal) + return true, true, nil + case policy_engine.PolicyResultDeny: + glog.V(3).Infof("EvaluatePolicyWithContext: DENY - bucket=%s, action=%s, principal=%s", bucket, s3Action, principal) + return false, true, nil + case policy_engine.PolicyResultIndeterminate: + // No policy exists for this bucket + glog.V(4).Infof("EvaluatePolicyWithContext: INDETERMINATE (no policy) - bucket=%s", bucket) + return false, false, nil default: - // Log warning for unmapped actions to help catch issues - glog.Warningf("convertActionToS3Format: unmapped action '%s', prefixing with 's3:'", action) - // For unknown actions, prefix with s3: to maintain format consistency - // This maintains backward compatibility while alerting developers - return "s3:" + action + return false, false, fmt.Errorf("unknown policy result: %v", result) } } + +// NOTE: The convertActionToS3Format wrapper has been removed for simplicity. +// EvaluatePolicy and EvaluatePolicyWithContext now call ResolveS3Action or +// mapBaseActionToS3Format directly, making the control flow more explicit.