From b9fa05153aaf6ad3cb81a2a69cc925bc16c8eee4 Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Wed, 25 Feb 2026 12:31:04 -0800 Subject: [PATCH] Allow multipart upload operations when s3:PutObject is authorized (#8445) * Allow multipart upload operations when s3:PutObject is authorized Multipart upload is an implementation detail of putting objects, not a separate permission. When a policy grants s3:PutObject, it should implicitly allow: - s3:CreateMultipartUpload - s3:UploadPart - s3:CompleteMultipartUpload - s3:AbortMultipartUpload - s3:ListParts This fixes a compatibility issue where clients like PyArrow that use multipart uploads by default would fail even though the role had s3:PutObject permission. The session policy intersection still applies - both the identity-based policy AND session policy must allow s3:PutObject for multipart operations to work. Implementation: - Added constants for S3 multipart action strings - Added multipartActionSet to efficiently check if action is multipart-related - Updated MatchesAction method to implicitly grant multipart when PutObject allowed * Update weed/s3api/policy_engine/types.go Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * Add s3:ListMultipartUploads to multipart action set Include s3:ListMultipartUploads in the multipartActionSet so that listing multipart uploads is implicitly granted when s3:PutObject is authorized. ListMultipartUploads is a critical part of the multipart upload workflow, allowing clients to query in-progress uploads before completing them. Changes: - Added s3ListMultipartUploads constant definition - Included s3ListMultipartUploads in multipartActionSet initialization - Existing references to multipartActionSet automatically now cover ListMultipartUploads All policy engine tests pass (0.351s execution time) * Refactor: reuse multipart action constants from s3_constants package Remove duplicate constant definitions from policy_engine/types.go and import the canonical definitions from s3api/s3_constants/s3_actions.go instead. This eliminates duplication and ensures a single source of truth for multipart action strings: - ACTION_CREATE_MULTIPART_UPLOAD - ACTION_UPLOAD_PART - ACTION_COMPLETE_MULTIPART - ACTION_ABORT_MULTIPART - ACTION_LIST_PARTS - ACTION_LIST_MULTIPART_UPLOADS All policy engine tests pass (0.350s execution time) * Fix S3_ACTION_LIST_MULTIPART_UPLOADS constant value Move S3_ACTION_LIST_MULTIPART_UPLOADS from bucket operations to multipart operations section and change value from 's3:ListBucketMultipartUploads' to 's3:ListMultipartUploads' to match the action strings used in policy_engine and s3_actions.go. This ensures consistent action naming across all S3 constant definitions. * refactor names * Fix S3 action constant mismatches and MatchesAction early return bug Fix two critical issues in policy engine: 1. S3Actions map had incorrect multipart action mappings: - 'ListMultipartUploads' was 's3:ListMultipartUploads' (should be 's3:ListBucketMultipartUploads') - 'ListParts' was 's3:ListParts' (should be 's3:ListMultipartUploadParts') These mismatches caused authorization checks to fail for list operations 2. CompiledStatement.MatchesAction() had early return bug: - Previously returned true immediately upon first direct action match - This prevented scanning remaining matchers for s3:PutObject permission - Now scans ALL matchers before returning, tracking both direct match and PutObject grant - Ensures multipart operations inherit s3:PutObject authorization even when explicitly requested action doesn't match (e.g., s3:ListMultipartUploadParts) Changes: - Track matchedAction flag to defer Fix two critical issues in policy engine: 1. S3Actions map had incorrect multipart action mappings: - 'ListMultipartUploads' was 's3:ListMultipartUplPer 1. S3Actions map had incorrect multiparAll - 'ListMultipartUploads(0.334s execution time) * Refactor S3Actions map to use s3_constants Replace hardcoded action strings in the S3Actions map with references to canonical S3_ACTION_* constants from s3_constants/s3_action_strings.go. Benefits: - Single source of truth for S3 action values - Eliminates string duplication across codebase - Ensures consistency between policy engine and middleware - Reduces maintenance burden when action strings need updates All policy engine tests pass (0.334s execution time) * Remove unused S3Actions map The S3Actions map in types.go was never referenced anywhere in the codebase. All action mappings are handled by GetActionMappings() in integration.go instead. This removes 42 lines of dead code. * Fix test: reload configuration function must also reload IAM state TestEmbeddedIamAttachUserPolicyRefreshesIAM was failing because the test's reloadConfigurationFunc only updated mockConfig but didn't reload the actual IAM state. When AttachUserPolicy calls refreshIAMConfiguration(), it would use the test's incomplete reload function instead of the real LoadS3ApiConfigurationFromCredentialManager(). Fixed by making the test's reloadConfigurationFunc also call e.iam.LoadS3ApiConfigurationFromCredentialManager() so lookupByIdentityName() sees the updated policy attachments. --------- Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> --- weed/s3api/policy_engine/types.go | 78 +++++++++----------- weed/s3api/s3_constants/s3_action_strings.go | 20 ++--- weed/s3api/s3_constants/s3_actions.go | 8 -- weed/s3api/s3_iam_middleware.go | 20 ++--- weed/s3api/s3_iam_simple_test.go | 4 +- weed/s3api/s3_multipart_iam.go | 12 +-- weed/s3api/s3_multipart_iam_test.go | 12 +-- weed/s3api/s3api_embedded_iam_test.go | 4 + 8 files changed, 72 insertions(+), 86 deletions(-) diff --git a/weed/s3api/policy_engine/types.go b/weed/s3api/policy_engine/types.go index 0af425de4..60cc87216 100644 --- a/weed/s3api/policy_engine/types.go +++ b/weed/s3api/policy_engine/types.go @@ -9,6 +9,7 @@ import ( "time" "github.com/seaweedfs/seaweedfs/weed/glog" + s3const "github.com/seaweedfs/seaweedfs/weed/s3api/s3_constants" ) // Policy Engine Types @@ -35,6 +36,17 @@ const ( var ( // PolicyVariableRegex detects AWS IAM policy variables like ${aws:username} PolicyVariableRegex = regexp.MustCompile(`\$\{([^}]+)\}`) + + // multipartActionSet contains all S3 multipart upload actions + // These are treated as equivalent to s3:PutObject for authorization purposes + multipartActionSet = map[string]bool{ + s3const.S3_ACTION_CREATE_MULTIPART: true, + s3const.S3_ACTION_UPLOAD_PART: true, + s3const.S3_ACTION_COMPLETE_MULTIPART: true, + s3const.S3_ACTION_ABORT_MULTIPART: true, + s3const.S3_ACTION_LIST_PARTS: true, + s3const.S3_ACTION_LIST_MULTIPART_UPLOADS: true, + } ) // StringOrStringSlice represents a value that can be either a string or []string @@ -455,55 +467,33 @@ func IsObjectResource(resource string) bool { return strings.Contains(resource, "/") } -// S3Actions contains common S3 actions -var S3Actions = map[string]string{ - "GetObject": "s3:GetObject", - "PutObject": "s3:PutObject", - "DeleteObject": "s3:DeleteObject", - "GetObjectVersion": "s3:GetObjectVersion", - "DeleteObjectVersion": "s3:DeleteObjectVersion", - "ListBucket": "s3:ListBucket", - "ListBucketVersions": "s3:ListBucketVersions", - "GetBucketLocation": "s3:GetBucketLocation", - "GetBucketVersioning": "s3:GetBucketVersioning", - "PutBucketVersioning": "s3:PutBucketVersioning", - "GetBucketAcl": "s3:GetBucketAcl", - "PutBucketAcl": "s3:PutBucketAcl", - "GetObjectAcl": "s3:GetObjectAcl", - "PutObjectAcl": "s3:PutObjectAcl", - "GetBucketPolicy": "s3:GetBucketPolicy", - "PutBucketPolicy": "s3:PutBucketPolicy", - "DeleteBucketPolicy": "s3:DeleteBucketPolicy", - "GetBucketCors": "s3:GetBucketCors", - "PutBucketCors": "s3:PutBucketCors", - "DeleteBucketCors": "s3:DeleteBucketCors", - "GetBucketNotification": "s3:GetBucketNotification", - "PutBucketNotification": "s3:PutBucketNotification", - "GetBucketTagging": "s3:GetBucketTagging", - "PutBucketTagging": "s3:PutBucketTagging", - "DeleteBucketTagging": "s3:DeleteBucketTagging", - "GetObjectTagging": "s3:GetObjectTagging", - "PutObjectTagging": "s3:PutObjectTagging", - "DeleteObjectTagging": "s3:DeleteObjectTagging", - "ListMultipartUploads": "s3:ListMultipartUploads", - "AbortMultipartUpload": "s3:AbortMultipartUpload", - "ListParts": "s3:ListParts", - "GetObjectRetention": "s3:GetObjectRetention", - "PutObjectRetention": "s3:PutObjectRetention", - "GetObjectLegalHold": "s3:GetObjectLegalHold", - "PutObjectLegalHold": "s3:PutObjectLegalHold", - "GetBucketObjectLockConfiguration": "s3:GetBucketObjectLockConfiguration", - "PutBucketObjectLockConfiguration": "s3:PutBucketObjectLockConfiguration", - "BypassGovernanceRetention": "s3:BypassGovernanceRetention", -} - -// MatchesAction checks if an action matches any of the compiled action matchers +// MatchesAction checks if an action matches any of the compiled action matchers. +// It also implicitly grants multipart upload actions if s3:PutObject is allowed, +// since multipart upload is an implementation detail of putting objects. func (cs *CompiledStatement) MatchesAction(action string) bool { + var matchedAction, hasPutObjectPermission bool + + // Scan all matchers to check for both direct action match and s3:PutObject grant for _, matcher := range cs.ActionMatchers { if matcher.Match(action) { - return true + matchedAction = true + } + if !hasPutObjectPermission && matcher.Match("s3:PutObject") { + hasPutObjectPermission = true } } + + // Return true if action matched directly or if multipart action with PutObject permission + if matchedAction { + return true + } + + // Multipart upload operations are part of s3:PutObject permission + // If s3:PutObject is allowed, implicitly allow multipart operations + if hasPutObjectPermission && multipartActionSet[action] { + return true + } + return false } diff --git a/weed/s3api/s3_constants/s3_action_strings.go b/weed/s3api/s3_constants/s3_action_strings.go index c7d5541c9..20e848997 100644 --- a/weed/s3api/s3_constants/s3_action_strings.go +++ b/weed/s3api/s3_constants/s3_action_strings.go @@ -27,18 +27,18 @@ const ( 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" + 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" + S3_ACTION_LIST_MULTIPART_UPLOADS = "s3:ListBucketMultipartUploads" // 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" + 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" // Bucket ACL operations S3_ACTION_GET_BUCKET_ACL = "s3:GetBucketAcl" diff --git a/weed/s3api/s3_constants/s3_actions.go b/weed/s3api/s3_constants/s3_actions.go index 835146bf3..713f5a3b8 100644 --- a/weed/s3api/s3_constants/s3_actions.go +++ b/weed/s3api/s3_constants/s3_actions.go @@ -17,14 +17,6 @@ const ( ACTION_GET_BUCKET_OBJECT_LOCK_CONFIG = "GetBucketObjectLockConfiguration" ACTION_PUT_BUCKET_OBJECT_LOCK_CONFIG = "PutBucketObjectLockConfiguration" - // Granular multipart upload actions for fine-grained IAM policies - ACTION_CREATE_MULTIPART_UPLOAD = "s3:CreateMultipartUpload" - ACTION_UPLOAD_PART = "s3:UploadPart" - ACTION_COMPLETE_MULTIPART = "s3:CompleteMultipartUpload" - ACTION_ABORT_MULTIPART = "s3:AbortMultipartUpload" - ACTION_LIST_MULTIPART_UPLOADS = "s3:ListMultipartUploads" - ACTION_LIST_PARTS = "s3:ListParts" - SeaweedStorageDestinationHeader = "x-seaweedfs-destination" MultipartUploadsFolder = ".uploads" VersionsFolder = ".versions" diff --git a/weed/s3api/s3_iam_middleware.go b/weed/s3api/s3_iam_middleware.go index e5ae898ee..d3489f461 100644 --- a/weed/s3api/s3_iam_middleware.go +++ b/weed/s3api/s3_iam_middleware.go @@ -479,17 +479,17 @@ func mapLegacyActionToIAM(legacyAction Action) string { return "s3:*" // Fallback for unmapped admin operations // Handle granular multipart actions (already correctly mapped) - case s3_constants.ACTION_CREATE_MULTIPART_UPLOAD: - return "s3:CreateMultipartUpload" - case s3_constants.ACTION_UPLOAD_PART: - return "s3:UploadPart" - case s3_constants.ACTION_COMPLETE_MULTIPART: - return "s3:CompleteMultipartUpload" - case s3_constants.ACTION_ABORT_MULTIPART: - return "s3:AbortMultipartUpload" - case s3_constants.ACTION_LIST_MULTIPART_UPLOADS: + case s3_constants.S3_ACTION_CREATE_MULTIPART: + return s3_constants.S3_ACTION_CREATE_MULTIPART + case s3_constants.S3_ACTION_UPLOAD_PART: + return s3_constants.S3_ACTION_UPLOAD_PART + case s3_constants.S3_ACTION_COMPLETE_MULTIPART: + return s3_constants.S3_ACTION_COMPLETE_MULTIPART + case s3_constants.S3_ACTION_ABORT_MULTIPART: + return s3_constants.S3_ACTION_ABORT_MULTIPART + case s3_constants.S3_ACTION_LIST_MULTIPART_UPLOADS: return s3_constants.S3_ACTION_LIST_MULTIPART_UPLOADS - case s3_constants.ACTION_LIST_PARTS: + case s3_constants.S3_ACTION_LIST_PARTS: return s3_constants.S3_ACTION_LIST_PARTS default: diff --git a/weed/s3api/s3_iam_simple_test.go b/weed/s3api/s3_iam_simple_test.go index ff30eb520..cb0d084ce 100644 --- a/weed/s3api/s3_iam_simple_test.go +++ b/weed/s3api/s3_iam_simple_test.go @@ -370,8 +370,8 @@ func TestMapLegacyActionToIAM(t *testing.T) { }, { name: "granular_multipart_action", - legacyAction: s3_constants.ACTION_CREATE_MULTIPART_UPLOAD, - expected: "s3:CreateMultipartUpload", + legacyAction: s3_constants.S3_ACTION_CREATE_MULTIPART, + expected: s3_constants.S3_ACTION_CREATE_MULTIPART, }, { name: "unknown_action_with_s3_prefix", diff --git a/weed/s3api/s3_multipart_iam.go b/weed/s3api/s3_multipart_iam.go index 9b56efc07..de3bccae9 100644 --- a/weed/s3api/s3_multipart_iam.go +++ b/weed/s3api/s3_multipart_iam.go @@ -284,17 +284,17 @@ func (s3a *S3ApiServer) UploadPartWithIAM(w http.ResponseWriter, r *http.Request func determineMultipartS3Action(operation MultipartOperation) Action { switch operation { case MultipartOpInitiate: - return s3_constants.ACTION_CREATE_MULTIPART_UPLOAD + return s3_constants.S3_ACTION_CREATE_MULTIPART case MultipartOpUploadPart: - return s3_constants.ACTION_UPLOAD_PART + return s3_constants.S3_ACTION_UPLOAD_PART case MultipartOpComplete: - return s3_constants.ACTION_COMPLETE_MULTIPART + return s3_constants.S3_ACTION_COMPLETE_MULTIPART case MultipartOpAbort: - return s3_constants.ACTION_ABORT_MULTIPART + return s3_constants.S3_ACTION_ABORT_MULTIPART case MultipartOpList: - return s3_constants.ACTION_LIST_MULTIPART_UPLOADS + return s3_constants.S3_ACTION_LIST_MULTIPART_UPLOADS case MultipartOpListParts: - return s3_constants.ACTION_LIST_PARTS + return s3_constants.S3_ACTION_LIST_PARTS default: // Fail closed for unmapped operations to prevent unintended access glog.Errorf("unmapped multipart operation: %s", operation) diff --git a/weed/s3api/s3_multipart_iam_test.go b/weed/s3api/s3_multipart_iam_test.go index 7169891c0..12546eb7a 100644 --- a/weed/s3api/s3_multipart_iam_test.go +++ b/weed/s3api/s3_multipart_iam_test.go @@ -281,12 +281,12 @@ func TestMultipartS3ActionMapping(t *testing.T) { operation MultipartOperation expectedAction Action }{ - {MultipartOpInitiate, s3_constants.ACTION_CREATE_MULTIPART_UPLOAD}, - {MultipartOpUploadPart, s3_constants.ACTION_UPLOAD_PART}, - {MultipartOpComplete, s3_constants.ACTION_COMPLETE_MULTIPART}, - {MultipartOpAbort, s3_constants.ACTION_ABORT_MULTIPART}, - {MultipartOpList, s3_constants.ACTION_LIST_MULTIPART_UPLOADS}, - {MultipartOpListParts, s3_constants.ACTION_LIST_PARTS}, + {MultipartOpInitiate, s3_constants.S3_ACTION_CREATE_MULTIPART}, + {MultipartOpUploadPart, s3_constants.S3_ACTION_UPLOAD_PART}, + {MultipartOpComplete, s3_constants.S3_ACTION_COMPLETE_MULTIPART}, + {MultipartOpAbort, s3_constants.S3_ACTION_ABORT_MULTIPART}, + {MultipartOpList, s3_constants.S3_ACTION_LIST_MULTIPART_UPLOADS}, + {MultipartOpListParts, s3_constants.S3_ACTION_LIST_PARTS}, {MultipartOperation("unknown"), "s3:InternalErrorUnknownMultipartAction"}, // Fail-closed for security } diff --git a/weed/s3api/s3api_embedded_iam_test.go b/weed/s3api/s3api_embedded_iam_test.go index 1c0d351f8..9eab6321c 100644 --- a/weed/s3api/s3api_embedded_iam_test.go +++ b/weed/s3api/s3api_embedded_iam_test.go @@ -83,6 +83,10 @@ func NewEmbeddedIamApiForTest() *EmbeddedIamApiForTest { return err } e.mockConfig = config + // Also refresh the IAM state so lookup functions see the updated configuration + if err := e.iam.LoadS3ApiConfigurationFromCredentialManager(); err != nil { + return err + } return nil } return e