aboutsummaryrefslogtreecommitdiff
path: root/weed/s3api
diff options
context:
space:
mode:
authorChris Lu <chrislusf@users.noreply.github.com>2025-11-13 16:10:46 -0800
committerGitHub <noreply@github.com>2025-11-13 16:10:46 -0800
commit4e73cc778c2624aac4a5a509ca153974a8da3a94 (patch)
tree5e3a6a2c73df8321fe7852e4e200913f05f11612 /weed/s3api
parent5b9a5263100696c0b597cbac32eec5c9e8c3b5cf (diff)
downloadseaweedfs-4e73cc778c2624aac4a5a509ca153974a8da3a94.tar.xz
seaweedfs-4e73cc778c2624aac4a5a509ca153974a8da3a94.zip
S3: add context aware action resolution (#7479)
* add context aware action resolution * isAnonymous * add s3 action resolver * refactor * correct action name * no need for action copy object * Simplify by removing the method-action mismatch path * use PUT instead of DELETE action * refactor * constants * versionId vs versions * address comments * comment * adjust messages * ResolveS3Action * address comments * refactor * simplify * more checks * not needed * simplify
Diffstat (limited to 'weed/s3api')
-rw-r--r--weed/s3api/auth_credentials.go3
-rw-r--r--weed/s3api/s3_action_resolver.go334
-rw-r--r--weed/s3api/s3_constants/s3_action_strings.go84
-rw-r--r--weed/s3api/s3_granular_action_security_test.go316
-rw-r--r--weed/s3api/s3_iam_middleware.go173
-rw-r--r--weed/s3api/s3_iam_simple_test.go6
-rw-r--r--weed/s3api/s3_list_parts_action_test.go48
-rw-r--r--weed/s3api/s3_multipart_iam_test.go4
-rw-r--r--weed/s3api/s3api_bucket_handlers.go3
-rw-r--r--weed/s3api/s3api_bucket_policy_engine.go117
10 files changed, 812 insertions, 276 deletions
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
- }
-
- // 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:*"
-
- // ACL operations
- case s3_constants.ACTION_READ_ACP:
- return "s3:GetObjectAcl"
- case s3_constants.ACTION_WRITE_ACP:
- return "s3:PutObjectAcl"
-
- // Bucket operations
- case s3_constants.ACTION_DELETE_BUCKET:
- return "s3:DeleteBucket"
-
- // 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"
+// 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")
+ }
+
+ // Convert action to S3 action format using request context
+ // ResolveS3Action handles nil request internally (falls back to mapBaseActionToS3Format)
+ s3Action := ResolveS3Action(r, action, bucket, object)
+
+ // Build resource ARN
+ resource := buildResourceARN(bucket, object)
+
+ glog.V(4).Infof("EvaluatePolicyWithContext: bucket=%s, resource=%s, action=%s (from %s), principal=%s",
+ bucket, resource, s3Action, action, principal)
+
+ // 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.