From 5469b7c58f389ae95b9352c07a772386fd7e3048 Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Wed, 24 Dec 2025 10:29:30 -0800 Subject: [PATCH 1/3] fix: resolve inconsistent S3 API authorization for DELETE operations (issue #7864) (#7865) * fix(iam): add support for fine-grained S3 actions in IAM policies Add support for fine-grained S3 actions like s3:DeleteObject, s3:PutObject, and other specific S3 actions in IAM policy mapping. Previously, only coarse-grained action patterns (Put*, Get*, etc.) were supported, causing IAM policies with specific actions to be rejected with 'not a valid action' error. Fixes issue #7864 part 2: s3:DeleteObject IAM action is now supported. Changes: - Extended MapToStatementAction() to handle fine-grained S3 actions - Maps S3-specific actions to appropriate internal action constants - Supports 30+ S3 actions including DeleteObject, PutObject, GetObject, etc. * fix(s3api): correct resource ARN generation for subpath permissions Fix convertSingleAction() to properly handle subpath patterns in legacy actions. Previously, when a user was granted Write permission to a subpath (e.g., Write:bucket/sub_path/*), the resource ARN was incorrectly generated, causing DELETE operations to be denied even though s3:DeleteObject was included in the Write action. The fix: - Extract bucket name and prefix path separately from patterns like 'bucket/prefix/*' - Generate correct S3 ARN format: arn:aws:s3:::bucket/prefix/* - Ensure all permission checks (Read, Write, List, Tagging, etc.) work correctly with subpaths - Support nested paths (e.g., bucket/a/b/c/*) Fixes issue #7864 part 1: Write permission on subpath now allows DELETE. Example: - Permission: Write:mybucket/documents/* - Objects can now be: PUT, DELETE, or ACL operations on mybucket/documents/* - Objects outside this path are still denied * test(iam): add tests for fine-grained S3 action mappings Extend TestMapToStatementAction with test cases for fine-grained S3 actions: - s3:DeleteObject - s3:PutObject - s3:GetObject - s3:ListBucket - s3:PutObjectAcl - s3:GetObjectAcl Ensures the new action mapping support is working correctly. * test(s3api): add comprehensive tests for subpath permission handling Add new test file with comprehensive tests for convertSingleAction(): 1. TestConvertSingleActionDeleteObject: Verifies s3:DeleteObject is included in Write actions (fixes issue #7864 part 2) 2. TestConvertSingleActionSubpath: Tests proper resource ARN generation for different permission patterns: - Bucket-level: Write:mybucket -> arn:aws:s3:::mybucket - Wildcard: Write:mybucket/* -> arn:aws:s3:::mybucket/* - Subpath: Write:mybucket/sub_path/* -> arn:aws:s3:::mybucket/sub_path/* - Nested: Read:mybucket/documents/* -> arn:aws:s3:::mybucket/documents/* 3. TestConvertSingleActionSubpathDeleteAllowed: Specifically validates that subpath Write permissions allow DELETE operations 4. TestConvertSingleActionNestedPaths: Tests deeply nested path handling (e.g., bucket/a/b/c/*) All tests pass and validate the fixes for issue #7864. * fix: address review comments from PR #7865 - Fix critical bug: use parsed 'bucket' instead of 'resourcePattern' for GetObjectRetention, GetObjectLegalHold, and PutObjectLegalHold actions to avoid malformed ARNs like arn:aws:s3:::bucket/*/* - Refactor large switch statement in MapToStatementAction() into a map-based lookup for better performance and maintainability * fmt * refactor: extract extractBucketAndPrefix helper and simplify convertSingleAction - Extract extractBucketAndPrefix as a package-level function for better testability and reusability - Remove unused bucketName parameter from convertSingleAction signature - Update GetResourcesFromLegacyAction to use the extracted helper for consistent ARN generation - Update all call sites in tests to match new function signature - All tests pass and module compiles without errors * fix: use extracted bucket variable consistently in all ARN generation branches Replace resourcePattern with extracted bucket variable in else branches and bucket-level cases to avoid malformed ARNs like 'arn:aws:s3:::mybucket/*/*': - Read case: bucket-level else branch - Write case: bucket-level else branch - Admin case: both bucket and object ARNs - List case: bucket-level else branch - GetBucketObjectLockConfiguration: bucket extraction - PutBucketObjectLockConfiguration: bucket extraction This ensures consistent ARN format: arn:aws:s3:::bucket or arn:aws:s3:::bucket/* * fix: address remaining review comments from PR #7865 High priority fixes: - Write action on bucket-level now generates arn:aws:s3:::mybucket/* instead of arn:aws:s3:::mybucket to enable object-level S3 actions (s3:PutObject, s3:DeleteObject) - GetResourcesFromLegacyAction now generates both bucket and object ARNs for /* patterns to maintain backward compatibility with mixed action groups Medium priority improvements: - Remove unused 'bucket' field from TestConvertSingleActionSubpath test struct - Update test to use assert.ElementsMatch instead of assert.Contains for more comprehensive resource ARN validation - Clarify test expectations with expectedResources slice instead of single expectedResource All tests pass, compilation verified * test: improve TestConvertSingleActionNestedPaths with comprehensive assertions Update test to use assert.ElementsMatch for more robust resource ARN verification: - Change struct from single expectedResource to expectedResources slice - Update Read nested path test to expect both bucket and prefix ARNs - Use assert.ElementsMatch to verify all generated resources match exactly - Provides complete coverage for nested path handling This matches the improvement pattern used in TestConvertSingleActionSubpath * refactor: simplify S3 action map and improve resource ARN detection - Refactor fineGrainedActionMap to use init() function for programmatic population of both prefixed (s3:Action) and unprefixed (Action) variants, eliminating 70+ duplicate entries - Add buildObjectResourceArn() helper to eliminate duplicated resource ARN generation logic across switch cases - Fix bucket vs object-level access detection: only use HasSuffix(/*) check instead of Contains('/') which incorrectly matched patterns like 'bucket/prefix' without wildcard - Apply buildObjectResourceArn() consistently to Tagging, BypassGovernanceRetention, GetObjectRetention, PutObjectRetention, GetObjectLegalHold, and PutObjectLegalHold cases * fmt * fix: generate object-level ARNs for bucket-level read access When bucket-level read access is granted (e.g., 'Read:mybucket'), generate both bucket and object ARNs so that object-level actions like s3:GetObject can properly authorize. Similarly, in GetResourcesFromLegacyAction, bucket-level patterns should generate both ARN levels for consistency with patterns that include wildcards. This ensures that users with bucket-level permissions can read objects, not just the bucket itself. * fix: address Copilot code review comments - Remove unused bucketName parameter from ConvertIdentityToPolicy signature - Update all callers in examples.go and engine_test.go - Bucket is now extracted from action string itself - Update extractBucketAndPrefix documentation - Add nested path example (bucket/a/b/c/*) - Clarify that prefix can contain multiple path segments - Make GetResourcesFromLegacyAction action-aware - Different action types have different resource requirements - List actions only need bucket ARN (bucket-only operations) - Read/Write/Tagging actions need both bucket and object ARNs - Aligns with convertSingleAction logic for consistency All tests pass successfully * test: add comprehensive tests for GetResourcesFromLegacyAction consistency - Add TestGetResourcesFromLegacyAction to verify action-aware resource generation - Validate consistency with convertSingleAction for all action types: * List actions: bucket-only ARNs (s3:ListBucket is bucket-level operation) * Read actions: both bucket and object ARNs * Write actions: object-only ARNs (subpaths) or object ARNs (bucket-level) * Admin actions: both bucket and object ARNs - Update GetResourcesFromLegacyAction to generate Admin ARNs consistent with convertSingleAction - All tests pass (35+ test cases across integration_test.go) * refactor: eliminate code duplication in GetResourcesFromLegacyAction - Simplify GetResourcesFromLegacyAction to delegate to convertSingleAction - Eliminates ~50 lines of duplicated action-type-specific logic - Ensures single source of truth for resource ARN generation - Improves maintainability: changes to ARN logic only need to be made in one place - All tests pass: any inconsistencies would be caught immediately - Addresses Gemini Code Assist review comment about code duplication * fix: remove fragile 'dummy' action type in CreatePolicyFromLegacyIdentity - Replace hardcoded 'dummy:' prefix with proper representative action type - Use first valid action type from the action list to determine resource requirements - Ensures GetResourcesFromLegacyAction receives a valid action type - Prevents silent failures when convertSingleAction encounters unknown action - Improves code clarity: explains why representative action type is needed - All tests pass: policy engine tests verify correct behavior * security: prevent privilege escalation in Admin action subpath handling - Admin action with subpath (e.g., Admin:bucket/admin/*) now correctly restricts to the specified subpath instead of granting full bucket access - If prefix exists: resources restricted to bucket + bucket/prefix/* - If no prefix: full bucket access (unchanged behavior for root Admin) - Added test case Admin_on_subpath to validate the security fix - All 40+ policy engine tests pass * refactor: address Copilot code review comments on S3 authorization - Fix GetObjectTagging mapping: change from ACTION_READ to ACTION_TAGGING (tagging operations should not be classified as general read operations) - Enhance extractBucketAndPrefix edge case handling: - Add input validation (reject empty strings, whitespace, slash-only) - Normalize double slashes and trailing slashes - Return empty bucket/prefix for invalid patterns - Prevent generation of malformed ARNs - Separate Read action from ListBucket (AWS S3 IAM semantics): - ListBucket is a bucket-level operation, not object-level - Read action now only includes s3:GetObject, s3:GetObjectVersion - This aligns with AWS S3 IAM policy best practices - Update buildObjectResourceArn to handle invalid bucket names gracefully: - Return empty slice if bucket is empty after validation - Prevents malformed ARN generation - Add comprehensive TestExtractBucketAndPrefixEdgeCases with 8 test cases: - Validates empty strings, whitespace, special characters - Confirms proper normalization of double/trailing slashes - Ensures robust parsing of nested paths - Update existing tests to reflect removed ListBucket from Read action All 40+ policy engine tests pass * fix: aggregate resource ARNs from all action types in CreatePolicyFromLegacyIdentity CRITICAL FIX: The previous implementation incorrectly used a single representative action type to determine resource ARNs when multiple legacy actions targeted the same resource pattern. This caused incorrect policy generation when action types with different resource requirements (e.g., List vs Write) were grouped together. Example of the bug: - Input: List:mybucket/path/*, Write:mybucket/path/* - Old behavior: Used only List's resources (bucket-level ARN) - Result: Policy had Write actions (s3:PutObject) but only bucket ARN - Consequence: s3:PutObject would be denied due to missing object-level ARN Solution: - Iterate through all action types for a given resource pattern - For each action type, call GetResourcesFromLegacyAction to get required ARNs - Aggregate all ARNs into a set to eliminate duplicates - Use the merged set for the final policy statement - Admin action short-circuits (always includes full permissions) Example of correct behavior: - Input: List:mybucket/path/*, Write:mybucket/path/* - New behavior: Aggregates both List and Write resource requirements - Result: Policy has Write actions with BOTH bucket and object-level ARNs - Outcome: s3:PutObject works correctly on mybucket/path/* Added TestCreatePolicyFromLegacyIdentityMultipleActions with 3 test cases: 1. List + Write on subpath: verifies bucket + object ARN aggregation 2. Read + Tagging on bucket: verifies action-specific ARN combinations 3. Admin with other actions: verifies Admin dominates resource ARNs All 45+ policy engine tests pass * fix: remove bucket-level ARN from Read action for consistency ISSUE: The Read action was including bucket-level ARNs (arn:aws:s3:::bucket) even though the only S3 actions in Read are s3:GetObject and s3:GetObjectVersion, which are object-level operations. This created a mismatch between the actions and resources in the policy statement. ROOT CAUSE: s3:ListBucket was previously removed from the Read action, but the bucket-level ARN was not removed, creating an inconsistency. SOLUTION: Update Read action to only generate object-level ARNs using buildObjectResourceArn, consistent with how Write and Tagging actions work. This ensures: - Read:mybucket generates arn:aws:s3:::mybucket/* (not bucket ARN) - Read:bucket/prefix/* generates arn:aws:s3:::bucket/prefix/* (object-level only) - Consistency: same actions, same resources, same logic across all object operations Updated test expectations: - TestConvertSingleActionSubpath: Read_on_subpath now expects only object ARN - TestConvertSingleActionNestedPaths: Read nested path now expects only object ARN - TestConvertIdentityToPolicy: Read resources now 1 instead of 2 - TestCreatePolicyFromLegacyIdentityMultipleActions: Read+Tagging aggregates to 1 ARN All 45+ policy engine tests pass * doc * fmt * fix: address Copilot code review on Read action consistency and missing S3 action mappings - Clarify MapToStatementAction comment to reflect exact lookup (not pattern matching) - Add missing S3 actions to baseS3ActionMap: - ListBucketVersions, ListAllMyBuckets for bucket operations - GetBucketCors, PutBucketCors, DeleteBucketCors for CORS - GetBucketNotification, PutBucketNotification for notifications - GetBucketObjectLockConfiguration, PutBucketObjectLockConfiguration for object lock - GetObjectVersionTagging for version tagging - GetObjectVersionAcl, PutBucketAcl for ACL operations - PutBucketTagging, DeleteBucketTagging for bucket tagging - Fix Read action scope inconsistency with GetActionMappings(): - Previously: only included GetObject, GetObjectVersion - Now: includes full Read set (14 actions) from GetActionMappings - Includes both bucket-level (ListBucket*, GetBucket*) and object-level (GetObject*) ARNs - Bucket ARN enables ListBucket operations, object ARN enables GetObject operations - Update all test expectations: - TestConvertSingleActionSubpath: Read now returns 2 ARNs (bucket + objects) - TestConvertSingleActionNestedPaths: Read nested path now includes bucket ARN - TestGetResourcesFromLegacyAction: Read test cases updated for consistency - TestCreatePolicyFromLegacyIdentityMultipleActions: Read_and_Tagging now returns 2 ARNs - TestConvertIdentityToPolicy: Updated to expect 14 Read actions and 2 resources Fixes: Inconsistency between convertSingleAction Read case and GetActionMappings function * fmt * fix: align convertSingleAction with GetActionMappings and add bucket validation - Fix Write action: now includes all 16 actions from GetActionMappings (object and bucket operations) - Includes PutBucketVersioning, PutBucketCors, PutBucketAcl, PutBucketTagging, etc. - Generates both bucket and object ARNs to support bucket-level operations - Fix List action: add ListAllMyBuckets from GetActionMappings - Previously: ListBucket, ListBucketVersions - Now: ListBucket, ListBucketVersions, ListAllMyBuckets - Add bucket validation to prevent malformed ARNs with empty bucket - Fix Tagging action: include bucket-level tagging operations - Previously: only object-level (GetObjectTagging, PutObjectTagging, DeleteObjectTagging) - Now: includes bucket-level (GetBucketTagging, PutBucketTagging, DeleteBucketTagging) - Generates both bucket and object ARNs to support bucket-level operations - Add bucket validation to prevent malformed ARNs: - Admin: return error if bucket is empty - List: generate empty resources if bucket is empty - Tagging: check bucket before generating ARNs - GetBucketObjectLockConfiguration, PutBucketObjectLockConfiguration: validate bucket - Fix TrimRight issue in extractBucketAndPrefix: - Change from strings.TrimRight(pattern, "/") to remove only one trailing slash - Prevents loss of prefix when pattern has multiple trailing slashes - Update all test cases: - TestConvertSingleActionSubpath: Write now returns 16 actions and bucket+object ARNs - TestConvertSingleActionNestedPaths: Write includes bucket ARN - TestGetResourcesFromLegacyAction: Updated Write and Tagging expectations - TestCreatePolicyFromLegacyIdentityMultipleActions: Updated action/resource counts Fixes: Inconsistencies between convertSingleAction and GetActionMappings for Write/List/Tagging actions * fmt * fix: resolve ListMultipartUploads/ListParts mapping inconsistency and add action validation - Fix ListMultipartUploads and ListParts mapping in helpers.go: - Changed from ACTION_LIST to ACTION_WRITE for consistency with GetActionMappings - These operations are part of the multipart write workflow and should map to Write action - Prevents inconsistent behavior when same actions processed through different code paths - Add documentation to clarify multipart operations in Write action: - Explain why ListMultipartUploads and ListParts are part of Write permissions - These are required for meaningful multipart upload workflow management - Add action validation to CreatePolicyFromLegacyIdentity: - Validates action format before processing using ValidateActionMapping - Logs warnings for invalid actions instead of silently skipping them - Provides clearer error messages when invalid action types are used - Ensures users know when their intended permissions weren't applied - Consistent with ConvertLegacyActions validation approach Fixes: Inconsistent action type mappings and silent failure for invalid actions --- weed/iam/helpers.go | 104 ++++-- weed/iam/helpers_test.go | 17 +- weed/s3api/policy_engine/engine_test.go | 24 +- weed/s3api/policy_engine/examples.go | 2 +- weed/s3api/policy_engine/integration.go | 314 +++++++++++----- weed/s3api/policy_engine/integration_test.go | 373 +++++++++++++++++++ 6 files changed, 707 insertions(+), 127 deletions(-) create mode 100644 weed/s3api/policy_engine/integration_test.go diff --git a/weed/iam/helpers.go b/weed/iam/helpers.go index e05219149..ef94940af 100644 --- a/weed/iam/helpers.go +++ b/weed/iam/helpers.go @@ -68,28 +68,92 @@ func StringSlicesEqual(a, b []string) bool { return true } +// fineGrainedActionMap maps S3 IAM action names to internal S3 action constants. +// Supports both prefixed (e.g., "s3:DeleteObject") and unprefixed (e.g., "DeleteObject") formats. +// Populated in init() to avoid duplication of prefixed/unprefixed variants. +var fineGrainedActionMap = map[string]string{ + // Coarse-grained actions (populated statically) + StatementActionAdmin: s3_constants.ACTION_ADMIN, + StatementActionWrite: s3_constants.ACTION_WRITE, + StatementActionWriteAcp: s3_constants.ACTION_WRITE_ACP, + StatementActionRead: s3_constants.ACTION_READ, + StatementActionReadAcp: s3_constants.ACTION_READ_ACP, + StatementActionList: s3_constants.ACTION_LIST, + StatementActionTagging: s3_constants.ACTION_TAGGING, + StatementActionDelete: s3_constants.ACTION_DELETE_BUCKET, +} + +// baseS3ActionMap defines the base S3 actions that will be populated with both +// prefixed (s3:Action) and unprefixed (Action) variants in init(). +var baseS3ActionMap = map[string]string{ + // Object operations + "DeleteObject": s3_constants.ACTION_WRITE, + "PutObject": s3_constants.ACTION_WRITE, + "GetObject": s3_constants.ACTION_READ, + "DeleteObjectVersion": s3_constants.ACTION_WRITE, + "GetObjectVersion": s3_constants.ACTION_READ, + // Tagging operations + "GetObjectTagging": s3_constants.ACTION_TAGGING, + "GetObjectVersionTagging": s3_constants.ACTION_TAGGING, + "PutObjectTagging": s3_constants.ACTION_TAGGING, + "DeleteObjectTagging": s3_constants.ACTION_TAGGING, + "GetBucketTagging": s3_constants.ACTION_TAGGING, + "PutBucketTagging": s3_constants.ACTION_TAGGING, + "DeleteBucketTagging": s3_constants.ACTION_TAGGING, + // ACL operations + "PutObjectAcl": s3_constants.ACTION_WRITE_ACP, + "GetObjectAcl": s3_constants.ACTION_READ_ACP, + "GetObjectVersionAcl": s3_constants.ACTION_READ_ACP, + "PutBucketAcl": s3_constants.ACTION_WRITE_ACP, + "GetBucketAcl": s3_constants.ACTION_READ_ACP, + // Bucket operations + "DeleteBucket": s3_constants.ACTION_DELETE_BUCKET, + "DeleteBucketPolicy": s3_constants.ACTION_ADMIN, + "ListBucket": s3_constants.ACTION_LIST, + "ListBucketVersions": s3_constants.ACTION_LIST, + "ListAllMyBuckets": s3_constants.ACTION_LIST, + "GetBucketLocation": s3_constants.ACTION_READ, + "GetBucketVersioning": s3_constants.ACTION_READ, + "PutBucketVersioning": s3_constants.ACTION_WRITE, + "GetBucketCors": s3_constants.ACTION_READ, + "PutBucketCors": s3_constants.ACTION_WRITE, + "DeleteBucketCors": s3_constants.ACTION_WRITE, + "GetBucketNotification": s3_constants.ACTION_READ, + "PutBucketNotification": s3_constants.ACTION_WRITE, + "GetBucketObjectLockConfiguration": s3_constants.ACTION_READ, + "PutBucketObjectLockConfiguration": s3_constants.ACTION_WRITE, + // Multipart upload operations + "CreateMultipartUpload": s3_constants.ACTION_WRITE, + "UploadPart": s3_constants.ACTION_WRITE, + "CompleteMultipartUpload": s3_constants.ACTION_WRITE, + "AbortMultipartUpload": s3_constants.ACTION_WRITE, + "ListMultipartUploads": s3_constants.ACTION_WRITE, + "ListParts": s3_constants.ACTION_WRITE, + // Retention and legal hold operations + "GetObjectRetention": s3_constants.ACTION_READ, + "PutObjectRetention": s3_constants.ACTION_WRITE, + "GetObjectLegalHold": s3_constants.ACTION_READ, + "PutObjectLegalHold": s3_constants.ACTION_WRITE, + "BypassGovernanceRetention": s3_constants.ACTION_WRITE, +} + +func init() { + // Populate both prefixed and unprefixed variants for all base S3 actions. + // This avoids duplication and makes it easy to add new actions in one place. + for action, constant := range baseS3ActionMap { + fineGrainedActionMap[action] = constant // unprefixed: "DeleteObject" + fineGrainedActionMap["s3:"+action] = constant // prefixed: "s3:DeleteObject" + } +} + // MapToStatementAction converts a policy statement action to an S3 action constant. +// It handles both coarse-grained statement actions (e.g., "Put*", "Get*") and +// fine-grained S3 actions (e.g., "s3:DeleteObject", "s3:PutObject") via exact lookup. func MapToStatementAction(action string) string { - switch action { - case StatementActionAdmin: - return s3_constants.ACTION_ADMIN - case StatementActionWrite: - return s3_constants.ACTION_WRITE - case StatementActionWriteAcp: - return s3_constants.ACTION_WRITE_ACP - case StatementActionRead: - return s3_constants.ACTION_READ - case StatementActionReadAcp: - return s3_constants.ACTION_READ_ACP - case StatementActionList: - return s3_constants.ACTION_LIST - case StatementActionTagging: - return s3_constants.ACTION_TAGGING - case StatementActionDelete: - return s3_constants.ACTION_DELETE_BUCKET - default: - return "" + if val, ok := fineGrainedActionMap[action]; ok { + return val } + return "" } // MapToIdentitiesAction converts an S3 action constant to a policy statement action. @@ -123,5 +187,3 @@ func MaskAccessKey(accessKeyId string) string { } return accessKeyId } - - diff --git a/weed/iam/helpers_test.go b/weed/iam/helpers_test.go index 6b6744475..f2da389c3 100644 --- a/weed/iam/helpers_test.go +++ b/weed/iam/helpers_test.go @@ -88,12 +88,25 @@ func TestMapToStatementAction(t *testing.T) { {StatementActionRead, s3_constants.ACTION_READ}, {StatementActionList, s3_constants.ACTION_LIST}, {StatementActionDelete, s3_constants.ACTION_DELETE_BUCKET}, + // Test fine-grained S3 action mappings (Issue #7864) + {"DeleteObject", s3_constants.ACTION_WRITE}, + {"s3:DeleteObject", s3_constants.ACTION_WRITE}, + {"PutObject", s3_constants.ACTION_WRITE}, + {"s3:PutObject", s3_constants.ACTION_WRITE}, + {"GetObject", s3_constants.ACTION_READ}, + {"s3:GetObject", s3_constants.ACTION_READ}, + {"ListBucket", s3_constants.ACTION_LIST}, + {"s3:ListBucket", s3_constants.ACTION_LIST}, + {"PutObjectAcl", s3_constants.ACTION_WRITE_ACP}, + {"s3:PutObjectAcl", s3_constants.ACTION_WRITE_ACP}, + {"GetObjectAcl", s3_constants.ACTION_READ_ACP}, + {"s3:GetObjectAcl", s3_constants.ACTION_READ_ACP}, {"unknown", ""}, } for _, test := range tests { result := MapToStatementAction(test.input) - assert.Equal(t, test.expected, result) + assert.Equal(t, test.expected, result, "Failed for input: %s", test.input) } } @@ -132,5 +145,3 @@ func TestMaskAccessKey(t *testing.T) { assert.Equal(t, test.expected, result) } } - - diff --git a/weed/s3api/policy_engine/engine_test.go b/weed/s3api/policy_engine/engine_test.go index 4de537ac1..f8297b56b 100644 --- a/weed/s3api/policy_engine/engine_test.go +++ b/weed/s3api/policy_engine/engine_test.go @@ -232,7 +232,7 @@ func TestConvertIdentityToPolicy(t *testing.T) { "Admin:bucket2", } - policy, err := ConvertIdentityToPolicy(identityActions, "bucket1") + policy, err := ConvertIdentityToPolicy(identityActions) if err != nil { t.Fatalf("Failed to convert identity to policy: %v", err) } @@ -252,13 +252,17 @@ func TestConvertIdentityToPolicy(t *testing.T) { } actions := normalizeToStringSlice(stmt.Action) - if len(actions) != 3 { - t.Errorf("Expected 3 read actions, got %d", len(actions)) + // Read action now includes: GetObject, GetObjectVersion, ListBucket, ListBucketVersions, + // GetObjectAcl, GetObjectVersionAcl, GetObjectTagging, GetObjectVersionTagging, + // GetBucketLocation, GetBucketVersioning, GetBucketAcl, GetBucketCors, GetBucketTagging, GetBucketNotification + if len(actions) != 14 { + t.Errorf("Expected 14 read actions, got %d: %v", len(actions), actions) } resources := normalizeToStringSlice(stmt.Resource) + // Read action now includes both bucket ARN (for ListBucket*) and object ARN (for GetObject*) if len(resources) != 2 { - t.Errorf("Expected 2 resources, got %d", len(resources)) + t.Errorf("Expected 2 resources (bucket and bucket/*), got %d: %v", len(resources), resources) } } @@ -797,8 +801,8 @@ func TestExistingObjectTagCondition(t *testing.T) { t.Run(tt.name, func(t *testing.T) { args := &PolicyEvaluationArgs{ Action: "s3:GetObject", - Resource: "arn:aws:s3:::test-bucket/test-object", - Principal: "*", + Resource: "arn:aws:s3:::test-bucket/test-object", + Principal: "*", ObjectEntry: tagsToEntry(tt.objectTags), } @@ -874,8 +878,8 @@ func TestExistingObjectTagConditionMultipleTags(t *testing.T) { t.Run(tt.name, func(t *testing.T) { args := &PolicyEvaluationArgs{ Action: "s3:GetObject", - Resource: "arn:aws:s3:::test-bucket/test-object", - Principal: "*", + Resource: "arn:aws:s3:::test-bucket/test-object", + Principal: "*", ObjectEntry: tagsToEntry(tt.objectTags), } @@ -946,8 +950,8 @@ func TestExistingObjectTagDenyPolicy(t *testing.T) { t.Run(tt.name, func(t *testing.T) { args := &PolicyEvaluationArgs{ Action: "s3:GetObject", - Resource: "arn:aws:s3:::test-bucket/test-object", - Principal: "*", + Resource: "arn:aws:s3:::test-bucket/test-object", + Principal: "*", ObjectEntry: tagsToEntry(tt.objectTags), } diff --git a/weed/s3api/policy_engine/examples.go b/weed/s3api/policy_engine/examples.go index 6f14127f3..c4cea6512 100644 --- a/weed/s3api/policy_engine/examples.go +++ b/weed/s3api/policy_engine/examples.go @@ -391,7 +391,7 @@ func ExampleLegacyIntegration() { } // Convert to policy - policy, err := ConvertIdentityToPolicy(legacyActions, "bucket1") + policy, err := ConvertIdentityToPolicy(legacyActions) if err != nil { fmt.Printf("Error converting identity to policy: %v\n", err) return diff --git a/weed/s3api/policy_engine/integration.go b/weed/s3api/policy_engine/integration.go index 17bcec112..8c9495088 100644 --- a/weed/s3api/policy_engine/integration.go +++ b/weed/s3api/policy_engine/integration.go @@ -123,12 +123,70 @@ func (p *PolicyBackedIAM) evaluateUsingPolicyConversion(action, bucketName, obje return false } +// extractBucketAndPrefix extracts bucket name and prefix from a resource pattern. +// Examples: +// +// "bucket" -> bucket="bucket", prefix="" +// "bucket/*" -> bucket="bucket", prefix="" +// "bucket/prefix/*" -> bucket="bucket", prefix="prefix" +// "bucket/a/b/c/*" -> bucket="bucket", prefix="a/b/c" +func extractBucketAndPrefix(pattern string) (string, string) { + // Validate input + pattern = strings.TrimSpace(pattern) + if pattern == "" || pattern == "/" { + return "", "" + } + + // Remove trailing /* if present + pattern = strings.TrimSuffix(pattern, "/*") + + // Remove a single trailing slash to avoid empty path segments + if strings.HasSuffix(pattern, "/") { + pattern = pattern[:len(pattern)-1] + } + if pattern == "" { + return "", "" + } + + // Split on the first / + parts := strings.SplitN(pattern, "/", 2) + bucket := strings.TrimSpace(parts[0]) + if bucket == "" { + return "", "" + } + + if len(parts) == 1 { + // No slash, entire pattern is bucket + return bucket, "" + } + // Has slash, first part is bucket, rest is prefix + prefix := strings.Trim(parts[1], "/") + return bucket, prefix +} + +// buildObjectResourceArn generates ARNs for object-level access. +// It properly handles both bucket-level (all objects) and prefix-level access. +// Returns empty slice if bucket is invalid to prevent generating malformed ARNs. +func buildObjectResourceArn(resourcePattern string) []string { + bucket, prefix := extractBucketAndPrefix(resourcePattern) + // If bucket is empty, the pattern is invalid; avoid generating malformed ARNs + if bucket == "" { + return []string{} + } + if prefix != "" { + // Prefix-based access: restrict to objects under this prefix + return []string{fmt.Sprintf("arn:aws:s3:::%s/%s/*", bucket, prefix)} + } + // Bucket-level access: all objects in bucket + return []string{fmt.Sprintf("arn:aws:s3:::%s/*", bucket)} +} + // ConvertIdentityToPolicy converts a legacy identity action to an AWS policy -func ConvertIdentityToPolicy(identityActions []string, bucketName string) (*PolicyDocument, error) { +func ConvertIdentityToPolicy(identityActions []string) (*PolicyDocument, error) { statements := make([]PolicyStatement, 0) for _, action := range identityActions { - stmt, err := convertSingleAction(action, bucketName) + stmt, err := convertSingleAction(action) if err != nil { glog.Warningf("Failed to convert action %s: %v", action, err) continue @@ -148,8 +206,9 @@ func ConvertIdentityToPolicy(identityActions []string, bucketName string) (*Poli }, nil } -// convertSingleAction converts a single legacy action to a policy statement -func convertSingleAction(action, bucketName string) (*PolicyStatement, error) { +// convertSingleAction converts a single legacy action to a policy statement. +// action format: "ActionType:ResourcePattern" (e.g., "Write:bucket/prefix/*") +func convertSingleAction(action string) (*PolicyStatement, error) { parts := strings.Split(action, ":") if len(parts) != 2 { return nil, fmt.Errorf("invalid action format: %s", action) @@ -163,111 +222,158 @@ func convertSingleAction(action, bucketName string) (*PolicyStatement, error) { switch actionType { case "Read": - s3Actions = []string{"s3:GetObject", "s3:GetObjectVersion", "s3:ListBucket"} - if strings.HasSuffix(resourcePattern, "/*") { - // Object-level read access - bucket := strings.TrimSuffix(resourcePattern, "/*") - resources = []string{ - fmt.Sprintf("arn:aws:s3:::%s", bucket), - fmt.Sprintf("arn:aws:s3:::%s/*", bucket), - } + // Read includes both object-level (GetObject, GetObjectAcl, GetObjectTagging, GetObjectVersions) + // and bucket-level operations (ListBucket, GetBucketLocation, GetBucketVersioning, GetBucketCors, etc.) + s3Actions = []string{ + "s3:GetObject", + "s3:GetObjectVersion", + "s3:GetObjectAcl", + "s3:GetObjectVersionAcl", + "s3:GetObjectTagging", + "s3:GetObjectVersionTagging", + "s3:ListBucket", + "s3:ListBucketVersions", + "s3:GetBucketLocation", + "s3:GetBucketVersioning", + "s3:GetBucketAcl", + "s3:GetBucketCors", + "s3:GetBucketTagging", + "s3:GetBucketNotification", + } + bucket, _ := extractBucketAndPrefix(resourcePattern) + objectResources := buildObjectResourceArn(resourcePattern) + // Include both bucket ARN (for ListBucket* and Get*Bucket operations) and object ARNs (for GetObject* operations) + if bucket != "" { + resources = append([]string{fmt.Sprintf("arn:aws:s3:::%s", bucket)}, objectResources...) } else { - // Bucket-level read access - resources = []string{fmt.Sprintf("arn:aws:s3:::%s", resourcePattern)} + resources = objectResources } case "Write": - s3Actions = []string{"s3:PutObject", "s3:DeleteObject", "s3:PutObjectAcl"} - if strings.HasSuffix(resourcePattern, "/*") { - // Object-level write access - bucket := strings.TrimSuffix(resourcePattern, "/*") - resources = []string{fmt.Sprintf("arn:aws:s3:::%s/*", bucket)} + // Write includes object-level writes (PutObject, DeleteObject, PutObjectAcl, DeleteObjectVersion, DeleteObjectTagging, PutObjectTagging) + // and bucket-level writes (PutBucketVersioning, PutBucketCors, DeleteBucketCors, PutBucketAcl, PutBucketTagging, DeleteBucketTagging, PutBucketNotification) + // and multipart upload operations (AbortMultipartUpload, ListMultipartUploads, ListParts). + // ListMultipartUploads and ListParts are included because they are part of the multipart upload workflow + // and require Write permissions to be meaningful (no point listing uploads if you can't abort/complete them). + s3Actions = []string{ + "s3:PutObject", + "s3:PutObjectAcl", + "s3:PutObjectTagging", + "s3:DeleteObject", + "s3:DeleteObjectVersion", + "s3:DeleteObjectTagging", + "s3:AbortMultipartUpload", + "s3:ListMultipartUploads", + "s3:ListParts", + "s3:PutBucketAcl", + "s3:PutBucketCors", + "s3:PutBucketTagging", + "s3:PutBucketNotification", + "s3:PutBucketVersioning", + "s3:DeleteBucketTagging", + "s3:DeleteBucketCors", + } + bucket, _ := extractBucketAndPrefix(resourcePattern) + objectResources := buildObjectResourceArn(resourcePattern) + // Include bucket ARN so bucket-level write operations (e.g., PutBucketVersioning, PutBucketCors) + // have the correct resource, while still allowing object-level writes. + if bucket != "" { + resources = append([]string{fmt.Sprintf("arn:aws:s3:::%s", bucket)}, objectResources...) } else { - // Bucket-level write access - resources = []string{fmt.Sprintf("arn:aws:s3:::%s", resourcePattern)} + resources = objectResources } case "Admin": s3Actions = []string{"s3:*"} - resources = []string{ - fmt.Sprintf("arn:aws:s3:::%s", resourcePattern), - fmt.Sprintf("arn:aws:s3:::%s/*", resourcePattern), + bucket, prefix := extractBucketAndPrefix(resourcePattern) + if bucket == "" { + // Invalid pattern, return error + return nil, fmt.Errorf("Admin action requires a valid bucket name") } - - case "List": - s3Actions = []string{"s3:ListBucket", "s3:ListBucketVersions"} - if strings.HasSuffix(resourcePattern, "/*") { - // Object-level list access - extract bucket from "bucket/prefix/*" pattern - patternWithoutWildcard := strings.TrimSuffix(resourcePattern, "/*") - parts := strings.SplitN(patternWithoutWildcard, "/", 2) - bucket := parts[0] + if prefix != "" { + // Subpath admin access: restrict to objects under this prefix + resources = []string{ + fmt.Sprintf("arn:aws:s3:::%s", bucket), + fmt.Sprintf("arn:aws:s3:::%s/%s/*", bucket, prefix), + } + } else { + // Bucket-level admin access: full bucket permissions resources = []string{ fmt.Sprintf("arn:aws:s3:::%s", bucket), fmt.Sprintf("arn:aws:s3:::%s/*", bucket), } + } + + case "List": + // List includes bucket listing operations and also ListAllMyBuckets + s3Actions = []string{"s3:ListBucket", "s3:ListBucketVersions", "s3:ListAllMyBuckets"} + // ListBucket actions only require bucket ARN, not object-level ARNs + bucket, _ := extractBucketAndPrefix(resourcePattern) + if bucket != "" { + resources = []string{fmt.Sprintf("arn:aws:s3:::%s", bucket)} } else { - // Bucket-level list access - resources = []string{fmt.Sprintf("arn:aws:s3:::%s", resourcePattern)} + // Invalid pattern, return empty resources to fail validation + resources = []string{} } case "Tagging": - s3Actions = []string{"s3:GetObjectTagging", "s3:PutObjectTagging", "s3:DeleteObjectTagging"} - resources = []string{fmt.Sprintf("arn:aws:s3:::%s/*", resourcePattern)} + // Tagging includes both object-level and bucket-level tagging operations + s3Actions = []string{ + "s3:GetObjectTagging", + "s3:PutObjectTagging", + "s3:DeleteObjectTagging", + "s3:GetBucketTagging", + "s3:PutBucketTagging", + "s3:DeleteBucketTagging", + } + bucket, _ := extractBucketAndPrefix(resourcePattern) + objectResources := buildObjectResourceArn(resourcePattern) + // Include bucket ARN so bucket-level tagging operations have the correct resource + if bucket != "" { + resources = append([]string{fmt.Sprintf("arn:aws:s3:::%s", bucket)}, objectResources...) + } else { + resources = objectResources + } case "BypassGovernanceRetention": s3Actions = []string{"s3:BypassGovernanceRetention"} - if strings.HasSuffix(resourcePattern, "/*") { - // Object-level bypass governance access - bucket := strings.TrimSuffix(resourcePattern, "/*") - resources = []string{fmt.Sprintf("arn:aws:s3:::%s/*", bucket)} - } else { - // Bucket-level bypass governance access - resources = []string{fmt.Sprintf("arn:aws:s3:::%s/*", resourcePattern)} - } + resources = buildObjectResourceArn(resourcePattern) case "GetObjectRetention": s3Actions = []string{"s3:GetObjectRetention"} - if strings.HasSuffix(resourcePattern, "/*") { - bucket := strings.TrimSuffix(resourcePattern, "/*") - resources = []string{fmt.Sprintf("arn:aws:s3:::%s/*", bucket)} - } else { - resources = []string{fmt.Sprintf("arn:aws:s3:::%s/*", resourcePattern)} - } + resources = buildObjectResourceArn(resourcePattern) case "PutObjectRetention": s3Actions = []string{"s3:PutObjectRetention"} - if strings.HasSuffix(resourcePattern, "/*") { - bucket := strings.TrimSuffix(resourcePattern, "/*") - resources = []string{fmt.Sprintf("arn:aws:s3:::%s/*", bucket)} - } else { - resources = []string{fmt.Sprintf("arn:aws:s3:::%s/*", resourcePattern)} - } + resources = buildObjectResourceArn(resourcePattern) case "GetObjectLegalHold": s3Actions = []string{"s3:GetObjectLegalHold"} - if strings.HasSuffix(resourcePattern, "/*") { - bucket := strings.TrimSuffix(resourcePattern, "/*") - resources = []string{fmt.Sprintf("arn:aws:s3:::%s/*", bucket)} - } else { - resources = []string{fmt.Sprintf("arn:aws:s3:::%s/*", resourcePattern)} - } + resources = buildObjectResourceArn(resourcePattern) case "PutObjectLegalHold": s3Actions = []string{"s3:PutObjectLegalHold"} - if strings.HasSuffix(resourcePattern, "/*") { - bucket := strings.TrimSuffix(resourcePattern, "/*") - resources = []string{fmt.Sprintf("arn:aws:s3:::%s/*", bucket)} - } else { - resources = []string{fmt.Sprintf("arn:aws:s3:::%s/*", resourcePattern)} - } + resources = buildObjectResourceArn(resourcePattern) case "GetBucketObjectLockConfiguration": s3Actions = []string{"s3:GetBucketObjectLockConfiguration"} - resources = []string{fmt.Sprintf("arn:aws:s3:::%s", resourcePattern)} + bucket, _ := extractBucketAndPrefix(resourcePattern) + if bucket != "" { + resources = []string{fmt.Sprintf("arn:aws:s3:::%s", bucket)} + } else { + // Invalid pattern, return empty resources to fail validation + resources = []string{} + } case "PutBucketObjectLockConfiguration": s3Actions = []string{"s3:PutBucketObjectLockConfiguration"} - resources = []string{fmt.Sprintf("arn:aws:s3:::%s", resourcePattern)} + bucket, _ := extractBucketAndPrefix(resourcePattern) + if bucket != "" { + resources = []string{fmt.Sprintf("arn:aws:s3:::%s", bucket)} + } else { + // Invalid pattern, return empty resources to fail validation + resources = []string{} + } default: return nil, fmt.Errorf("unknown action type: %s", actionType) @@ -416,27 +522,15 @@ func ConvertLegacyActions(legacyActions []string) ([]string, error) { return uniqueActions, nil } -// GetResourcesFromLegacyAction extracts resources from a legacy action +// GetResourcesFromLegacyAction extracts resources from a legacy action. +// It delegates to convertSingleAction to ensure consistent resource ARN generation +// across the codebase and avoid duplicating action-type-specific logic. func GetResourcesFromLegacyAction(legacyAction string) ([]string, error) { - parts := strings.Split(legacyAction, ":") - if len(parts) != 2 { - return nil, fmt.Errorf("invalid action format: %s", legacyAction) + stmt, err := convertSingleAction(legacyAction) + if err != nil { + return nil, err } - - resourcePattern := parts[1] - resources := make([]string, 0) - - if strings.HasSuffix(resourcePattern, "/*") { - // Object-level access - bucket := strings.TrimSuffix(resourcePattern, "/*") - resources = append(resources, fmt.Sprintf("arn:aws:s3:::%s", bucket)) - resources = append(resources, fmt.Sprintf("arn:aws:s3:::%s/*", bucket)) - } else { - // Bucket-level access - resources = append(resources, fmt.Sprintf("arn:aws:s3:::%s", resourcePattern)) - } - - return resources, nil + return stmt.Resource.Strings(), nil } // CreatePolicyFromLegacyIdentity creates a policy document from legacy identity actions @@ -447,6 +541,12 @@ func CreatePolicyFromLegacyIdentity(identityName string, actions []string) (*Pol resourceActions := make(map[string][]string) for _, action := range actions { + // Validate action format before processing + if err := ValidateActionMapping(action); err != nil { + glog.Warningf("Skipping invalid action %q for identity %q: %v", action, identityName, err) + continue + } + parts := strings.Split(action, ":") if len(parts) != 2 { continue @@ -464,23 +564,53 @@ func CreatePolicyFromLegacyIdentity(identityName string, actions []string) (*Pol // Create statements for each resource pattern for resourcePattern, actionTypes := range resourceActions { s3Actions := make([]string, 0) - + resourceSet := make(map[string]struct{}) + + // Collect S3 actions and aggregate resource ARNs from all action types. + // Different action types have different resource ARN requirements: + // - List: bucket-level ARNs only + // - Read/Write/Tagging: object-level ARNs + // - Admin: full bucket access + // We must merge all required ARNs for the combined policy statement. for _, actionType := range actionTypes { if actionType == "Admin" { s3Actions = []string{"s3:*"} + // Admin action determines the resources, so we can break after processing it. + res, err := GetResourcesFromLegacyAction(fmt.Sprintf("Admin:%s", resourcePattern)) + if err != nil { + glog.Warningf("Failed to get resources for Admin action on %s: %v", resourcePattern, err) + resourceSet = nil // Invalidate to skip this statement + break + } + for _, r := range res { + resourceSet[r] = struct{}{} + } break } if mapped, exists := GetActionMappings()[actionType]; exists { s3Actions = append(s3Actions, mapped...) + res, err := GetResourcesFromLegacyAction(fmt.Sprintf("%s:%s", actionType, resourcePattern)) + if err != nil { + glog.Warningf("Failed to get resources for %s action on %s: %v", actionType, resourcePattern, err) + resourceSet = nil // Invalidate to skip this statement + break + } + for _, r := range res { + resourceSet[r] = struct{}{} + } } } - resources, err := GetResourcesFromLegacyAction(fmt.Sprintf("dummy:%s", resourcePattern)) - if err != nil { + if resourceSet == nil || len(s3Actions) == 0 { continue } + resources := make([]string, 0, len(resourceSet)) + for r := range resourceSet { + resources = append(resources, r) + } + statement := PolicyStatement{ Sid: fmt.Sprintf("%s-%s", identityName, strings.ReplaceAll(resourcePattern, "/", "-")), Effect: PolicyEffectAllow, diff --git a/weed/s3api/policy_engine/integration_test.go b/weed/s3api/policy_engine/integration_test.go new file mode 100644 index 000000000..6e74e51cb --- /dev/null +++ b/weed/s3api/policy_engine/integration_test.go @@ -0,0 +1,373 @@ +package policy_engine + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +// TestConvertSingleActionDeleteObject tests support for s3:DeleteObject action (Issue #7864) +func TestConvertSingleActionDeleteObject(t *testing.T) { + // Test that Write action includes DeleteObject S3 action + stmt, err := convertSingleAction("Write:bucket") + assert.NoError(t, err) + assert.NotNil(t, stmt) + + // Check that s3:DeleteObject is included in the actions + actions := stmt.Action.Strings() + assert.Contains(t, actions, "s3:DeleteObject", "Write action should include s3:DeleteObject") + assert.Contains(t, actions, "s3:PutObject", "Write action should include s3:PutObject") +} + +// TestConvertSingleActionSubpath tests subpath handling for legacy actions (Issue #7864) +func TestConvertSingleActionSubpath(t *testing.T) { + testCases := []struct { + name string + action string + expectedActions []string + expectedResources []string + description string + }{ + { + name: "Write_on_bucket", + action: "Write:mybucket", + expectedActions: []string{"s3:PutObject", "s3:DeleteObject", "s3:PutObjectAcl", "s3:DeleteObjectVersion", "s3:PutObjectTagging", "s3:DeleteObjectTagging", "s3:AbortMultipartUpload", "s3:ListMultipartUploads", "s3:ListParts", "s3:PutBucketAcl", "s3:PutBucketCors", "s3:PutBucketTagging", "s3:PutBucketNotification", "s3:PutBucketVersioning", "s3:DeleteBucketTagging", "s3:DeleteBucketCors"}, + expectedResources: []string{"arn:aws:s3:::mybucket", "arn:aws:s3:::mybucket/*"}, + description: "Write permission on bucket should include bucket and object ARNs", + }, + { + name: "Write_on_bucket_with_wildcard", + action: "Write:mybucket/*", + expectedActions: []string{"s3:PutObject", "s3:DeleteObject", "s3:PutObjectAcl", "s3:DeleteObjectVersion", "s3:PutObjectTagging", "s3:DeleteObjectTagging", "s3:AbortMultipartUpload", "s3:ListMultipartUploads", "s3:ListParts", "s3:PutBucketAcl", "s3:PutBucketCors", "s3:PutBucketTagging", "s3:PutBucketNotification", "s3:PutBucketVersioning", "s3:DeleteBucketTagging", "s3:DeleteBucketCors"}, + expectedResources: []string{"arn:aws:s3:::mybucket", "arn:aws:s3:::mybucket/*"}, + description: "Write permission with /* should include bucket and object ARNs", + }, + { + name: "Write_on_subpath", + action: "Write:mybucket/sub_path/*", + expectedActions: []string{"s3:PutObject", "s3:DeleteObject", "s3:PutObjectAcl", "s3:DeleteObjectVersion", "s3:PutObjectTagging", "s3:DeleteObjectTagging", "s3:AbortMultipartUpload", "s3:ListMultipartUploads", "s3:ListParts", "s3:PutBucketAcl", "s3:PutBucketCors", "s3:PutBucketTagging", "s3:PutBucketNotification", "s3:PutBucketVersioning", "s3:DeleteBucketTagging", "s3:DeleteBucketCors"}, + expectedResources: []string{"arn:aws:s3:::mybucket", "arn:aws:s3:::mybucket/sub_path/*"}, + description: "Write permission on subpath should include bucket and subpath objects ARNs", + }, + { + name: "Read_on_subpath", + action: "Read:mybucket/documents/*", + expectedActions: []string{"s3:GetObject", "s3:GetObjectVersion", "s3:ListBucket", "s3:ListBucketVersions", "s3:GetObjectAcl", "s3:GetObjectVersionAcl", "s3:GetObjectTagging", "s3:GetObjectVersionTagging", "s3:GetBucketLocation", "s3:GetBucketVersioning", "s3:GetBucketAcl", "s3:GetBucketCors", "s3:GetBucketTagging", "s3:GetBucketNotification"}, + expectedResources: []string{"arn:aws:s3:::mybucket", "arn:aws:s3:::mybucket/documents/*"}, + description: "Read permission on subpath should include bucket ARN and subpath objects", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + stmt, err := convertSingleAction(tc.action) + assert.NoError(t, err, tc.description) + assert.NotNil(t, stmt) + + // Check actions + actions := stmt.Action.Strings() + for _, expectedAction := range tc.expectedActions { + assert.Contains(t, actions, expectedAction, + "Action %s should be included for %s", expectedAction, tc.action) + } + + // Check resources - verify all expected resources are present + resources := stmt.Resource.Strings() + assert.ElementsMatch(t, resources, tc.expectedResources, + "Resources should match exactly for %s. Got %v, expected %v", tc.action, resources, tc.expectedResources) + }) + } +} + +// TestConvertSingleActionSubpathDeleteAllowed tests that DeleteObject works on subpaths +func TestConvertSingleActionSubpathDeleteAllowed(t *testing.T) { + // This test specifically addresses Issue #7864 part 1: + // "when a user is granted permission to a subpath, eg s3.configure -user someuser + // -actions Write -buckets some_bucket/sub_path/* -apply + // the user will only be able to put, but not delete object under somebucket/sub_path" + + stmt, err := convertSingleAction("Write:some_bucket/sub_path/*") + assert.NoError(t, err) + + // The fix: s3:DeleteObject should be in the allowed actions + actions := stmt.Action.Strings() + assert.Contains(t, actions, "s3:DeleteObject", + "Write permission on subpath should allow deletion of objects in that path") + + // The resource should be restricted to the subpath + resources := stmt.Resource.Strings() + assert.Contains(t, resources, "arn:aws:s3:::some_bucket/sub_path/*", + "Delete permission should apply to objects under the subpath") +} + +// TestConvertSingleActionNestedPaths tests deeply nested paths +func TestConvertSingleActionNestedPaths(t *testing.T) { + testCases := []struct { + action string + expectedResources []string + }{ + { + action: "Write:bucket/a/b/c/*", + expectedResources: []string{"arn:aws:s3:::bucket", "arn:aws:s3:::bucket/a/b/c/*"}, + }, + { + action: "Read:bucket/data/documents/2024/*", + expectedResources: []string{"arn:aws:s3:::bucket", "arn:aws:s3:::bucket/data/documents/2024/*"}, + }, + } + + for _, tc := range testCases { + stmt, err := convertSingleAction(tc.action) + assert.NoError(t, err) + + resources := stmt.Resource.Strings() + assert.ElementsMatch(t, resources, tc.expectedResources) + } +} + +// TestGetResourcesFromLegacyAction tests that GetResourcesFromLegacyAction generates +// action-appropriate resources consistent with convertSingleAction +func TestGetResourcesFromLegacyAction(t *testing.T) { + testCases := []struct { + name string + action string + expectedResources []string + description string + }{ + // List actions - bucket-only (no object ARNs) + { + name: "List_on_bucket", + action: "List:mybucket", + expectedResources: []string{"arn:aws:s3:::mybucket"}, + description: "List action should only have bucket ARN", + }, + { + name: "List_on_bucket_with_wildcard", + action: "List:mybucket/*", + expectedResources: []string{"arn:aws:s3:::mybucket"}, + description: "List action should only have bucket ARN regardless of wildcard", + }, + // Read actions - bucket and object-level ARNs (includes List* and Get* operations) + { + name: "Read_on_bucket", + action: "Read:mybucket", + expectedResources: []string{"arn:aws:s3:::mybucket", "arn:aws:s3:::mybucket/*"}, + description: "Read action should have both bucket and object ARNs", + }, + { + name: "Read_on_subpath", + action: "Read:mybucket/documents/*", + expectedResources: []string{"arn:aws:s3:::mybucket", "arn:aws:s3:::mybucket/documents/*"}, + description: "Read action on subpath should have bucket ARN and object ARN for subpath", + }, + // Write actions - bucket and object ARNs (includes bucket-level operations) + { + name: "Write_on_subpath", + action: "Write:mybucket/sub_path/*", + expectedResources: []string{"arn:aws:s3:::mybucket", "arn:aws:s3:::mybucket/sub_path/*"}, + description: "Write action should have bucket and object ARNs", + }, + // Admin actions - both bucket and object ARNs + { + name: "Admin_on_bucket", + action: "Admin:mybucket", + expectedResources: []string{"arn:aws:s3:::mybucket", "arn:aws:s3:::mybucket/*"}, + description: "Admin action should have both bucket and object ARNs", + }, + { + name: "Admin_on_subpath", + action: "Admin:mybucket/admin/section/*", + expectedResources: []string{"arn:aws:s3:::mybucket", "arn:aws:s3:::mybucket/admin/section/*"}, + description: "Admin action on subpath should restrict to subpath, preventing privilege escalation", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + resources, err := GetResourcesFromLegacyAction(tc.action) + assert.NoError(t, err, tc.description) + assert.ElementsMatch(t, resources, tc.expectedResources, + "Resources should match expected. Got %v, expected %v", resources, tc.expectedResources) + + // Also verify consistency with convertSingleAction where applicable + stmt, err := convertSingleAction(tc.action) + assert.NoError(t, err) + + stmtResources := stmt.Resource.Strings() + assert.ElementsMatch(t, resources, stmtResources, + "GetResourcesFromLegacyAction should match convertSingleAction resources for %s", tc.action) + }) + } +} + +// TestExtractBucketAndPrefixEdgeCases validates edge case handling in extractBucketAndPrefix +func TestExtractBucketAndPrefixEdgeCases(t *testing.T) { + testCases := []struct { + name string + pattern string + expectedBucket string + expectedPrefix string + description string + }{ + { + name: "Empty string", + pattern: "", + expectedBucket: "", + expectedPrefix: "", + description: "Empty pattern should return empty strings", + }, + { + name: "Whitespace only", + pattern: " ", + expectedBucket: "", + expectedPrefix: "", + description: "Whitespace-only pattern should return empty strings", + }, + { + name: "Slash only", + pattern: "/", + expectedBucket: "", + expectedPrefix: "", + description: "Slash-only pattern should return empty strings", + }, + { + name: "Double slash prefix", + pattern: "bucket//prefix/*", + expectedBucket: "bucket", + expectedPrefix: "prefix", + description: "Double slash should be normalized (trailing slashes removed)", + }, + { + name: "Normal bucket", + pattern: "mybucket", + expectedBucket: "mybucket", + expectedPrefix: "", + description: "Bucket-only pattern should work correctly", + }, + { + name: "Bucket with prefix", + pattern: "mybucket/myprefix/*", + expectedBucket: "mybucket", + expectedPrefix: "myprefix", + description: "Bucket with prefix should be parsed correctly", + }, + { + name: "Nested prefix", + pattern: "mybucket/a/b/c/*", + expectedBucket: "mybucket", + expectedPrefix: "a/b/c", + description: "Nested prefix should be preserved", + }, + { + name: "Bucket with trailing slash", + pattern: "mybucket/", + expectedBucket: "mybucket", + expectedPrefix: "", + description: "Trailing slash on bucket should be normalized", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + bucket, prefix := extractBucketAndPrefix(tc.pattern) + assert.Equal(t, tc.expectedBucket, bucket, tc.description) + assert.Equal(t, tc.expectedPrefix, prefix, tc.description) + }) + } +} + +// TestCreatePolicyFromLegacyIdentityMultipleActions validates correct resource ARN aggregation +// when multiple action types target the same resource pattern +func TestCreatePolicyFromLegacyIdentityMultipleActions(t *testing.T) { + testCases := []struct { + name string + identityName string + actions []string + expectedStatements int + expectedActionsInStmt1 []string + expectedResourcesInStmt1 []string + description string + }{ + { + name: "List_and_Write_on_subpath", + identityName: "data-manager", + actions: []string{"List:mybucket/data/*", "Write:mybucket/data/*"}, + expectedStatements: 1, + expectedActionsInStmt1: []string{ + "s3:ListBucket", "s3:ListBucketVersions", "s3:ListAllMyBuckets", + "s3:PutObject", "s3:DeleteObject", "s3:PutObjectAcl", "s3:DeleteObjectVersion", + "s3:PutObjectTagging", "s3:DeleteObjectTagging", "s3:AbortMultipartUpload", + "s3:ListMultipartUploads", "s3:ListParts", "s3:PutBucketAcl", "s3:PutBucketCors", + "s3:PutBucketTagging", "s3:PutBucketNotification", "s3:PutBucketVersioning", + "s3:DeleteBucketTagging", "s3:DeleteBucketCors", + }, + expectedResourcesInStmt1: []string{ + "arn:aws:s3:::mybucket", // From List and Write actions + "arn:aws:s3:::mybucket/data/*", // From Write action + }, + description: "List + Write on same subpath should aggregate all actions and both bucket and object ARNs", + }, + { + name: "Read_and_Tagging_on_bucket", + identityName: "tag-reader", + actions: []string{"Read:mybucket", "Tagging:mybucket"}, + expectedStatements: 1, + expectedActionsInStmt1: []string{ + "s3:GetObject", "s3:GetObjectVersion", + "s3:ListBucket", "s3:ListBucketVersions", + "s3:GetObjectAcl", "s3:GetObjectVersionAcl", + "s3:GetObjectTagging", "s3:GetObjectVersionTagging", + "s3:PutObjectTagging", "s3:DeleteObjectTagging", + "s3:GetBucketLocation", "s3:GetBucketVersioning", + "s3:GetBucketAcl", "s3:GetBucketCors", "s3:GetBucketTagging", + "s3:GetBucketNotification", "s3:PutBucketTagging", "s3:DeleteBucketTagging", + }, + expectedResourcesInStmt1: []string{ + "arn:aws:s3:::mybucket", + "arn:aws:s3:::mybucket/*", + }, + description: "Read + Tagging on same bucket should aggregate all bucket and object-level actions and ARNs", + }, + { + name: "Admin_with_other_actions", + identityName: "admin-user", + actions: []string{"Admin:mybucket/admin/*", "Write:mybucket/admin/*"}, + expectedStatements: 1, + expectedActionsInStmt1: []string{"s3:*"}, + expectedResourcesInStmt1: []string{ + "arn:aws:s3:::mybucket", + "arn:aws:s3:::mybucket/admin/*", + }, + description: "Admin action should dominate and set s3:*, other actions still processed for resources", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + policy, err := CreatePolicyFromLegacyIdentity(tc.identityName, tc.actions) + assert.NoError(t, err, tc.description) + assert.NotNil(t, policy) + + // Check statement count + assert.Equal(t, tc.expectedStatements, len(policy.Statement), + "Expected %d statement(s), got %d", tc.expectedStatements, len(policy.Statement)) + + if tc.expectedStatements > 0 { + stmt := policy.Statement[0] + + // Check actions + actualActions := stmt.Action.Strings() + for _, expectedAction := range tc.expectedActionsInStmt1 { + assert.Contains(t, actualActions, expectedAction, + "Action %s should be included in statement", expectedAction) + } + + // Check resources - all expected resources should be present + actualResources := stmt.Resource.Strings() + assert.ElementsMatch(t, tc.expectedResourcesInStmt1, actualResources, + "Statement should aggregate all required resource ARNs. Got %v, expected %v", + actualResources, tc.expectedResourcesInStmt1) + } + }) + } +} From 26acebdef1ffa45da9bff392bef71394b273454c Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Wed, 24 Dec 2025 10:50:05 -0800 Subject: [PATCH 2/3] fix: restore TimeToFirstByte metric for S3 GetObject operations (issue #7869) (#7870) * fix(iam): add support for fine-grained S3 actions in IAM policies Add support for fine-grained S3 actions like s3:DeleteObject, s3:PutObject, and other specific S3 actions in IAM policy mapping. Previously, only coarse-grained action patterns (Put*, Get*, etc.) were supported, causing IAM policies with specific actions to be rejected with 'not a valid action' error. Fixes issue #7864 part 2: s3:DeleteObject IAM action is now supported. Changes: - Extended MapToStatementAction() to handle fine-grained S3 actions - Maps S3-specific actions to appropriate internal action constants - Supports 30+ S3 actions including DeleteObject, PutObject, GetObject, etc. * fix(s3api): correct resource ARN generation for subpath permissions Fix convertSingleAction() to properly handle subpath patterns in legacy actions. Previously, when a user was granted Write permission to a subpath (e.g., Write:bucket/sub_path/*), the resource ARN was incorrectly generated, causing DELETE operations to be denied even though s3:DeleteObject was included in the Write action. The fix: - Extract bucket name and prefix path separately from patterns like 'bucket/prefix/*' - Generate correct S3 ARN format: arn:aws:s3:::bucket/prefix/* - Ensure all permission checks (Read, Write, List, Tagging, etc.) work correctly with subpaths - Support nested paths (e.g., bucket/a/b/c/*) Fixes issue #7864 part 1: Write permission on subpath now allows DELETE. Example: - Permission: Write:mybucket/documents/* - Objects can now be: PUT, DELETE, or ACL operations on mybucket/documents/* - Objects outside this path are still denied * test(s3api): add comprehensive tests for subpath permission handling Add new test file with comprehensive tests for convertSingleAction(): 1. TestConvertSingleActionDeleteObject: Verifies s3:DeleteObject is included in Write actions (fixes issue #7864 part 2) 2. TestConvertSingleActionSubpath: Tests proper resource ARN generation for different permission patterns: - Bucket-level: Write:mybucket -> arn:aws:s3:::mybucket - Wildcard: Write:mybucket/* -> arn:aws:s3:::mybucket/* - Subpath: Write:mybucket/sub_path/* -> arn:aws:s3:::mybucket/sub_path/* - Nested: Read:mybucket/documents/* -> arn:aws:s3:::mybucket/documents/* 3. TestConvertSingleActionSubpathDeleteAllowed: Specifically validates that subpath Write permissions allow DELETE operations 4. TestConvertSingleActionNestedPaths: Tests deeply nested path handling (e.g., bucket/a/b/c/*) All tests pass and validate the fixes for issue #7864. * fix: address review comments from PR #7865 - Fix critical bug: use parsed 'bucket' instead of 'resourcePattern' for GetObjectRetention, GetObjectLegalHold, and PutObjectLegalHold actions to avoid malformed ARNs like arn:aws:s3:::bucket/*/* - Refactor large switch statement in MapToStatementAction() into a map-based lookup for better performance and maintainability * fmt * refactor: extract extractBucketAndPrefix helper and simplify convertSingleAction - Extract extractBucketAndPrefix as a package-level function for better testability and reusability - Remove unused bucketName parameter from convertSingleAction signature - Update GetResourcesFromLegacyAction to use the extracted helper for consistent ARN generation - Update all call sites in tests to match new function signature - All tests pass and module compiles without errors * fix: use extracted bucket variable consistently in all ARN generation branches Replace resourcePattern with extracted bucket variable in else branches and bucket-level cases to avoid malformed ARNs like 'arn:aws:s3:::mybucket/*/*': - Read case: bucket-level else branch - Write case: bucket-level else branch - Admin case: both bucket and object ARNs - List case: bucket-level else branch - GetBucketObjectLockConfiguration: bucket extraction - PutBucketObjectLockConfiguration: bucket extraction This ensures consistent ARN format: arn:aws:s3:::bucket or arn:aws:s3:::bucket/* * fix: address remaining review comments from PR #7865 High priority fixes: - Write action on bucket-level now generates arn:aws:s3:::mybucket/* instead of arn:aws:s3:::mybucket to enable object-level S3 actions (s3:PutObject, s3:DeleteObject) - GetResourcesFromLegacyAction now generates both bucket and object ARNs for /* patterns to maintain backward compatibility with mixed action groups Medium priority improvements: - Remove unused 'bucket' field from TestConvertSingleActionSubpath test struct - Update test to use assert.ElementsMatch instead of assert.Contains for more comprehensive resource ARN validation - Clarify test expectations with expectedResources slice instead of single expectedResource All tests pass, compilation verified * test: improve TestConvertSingleActionNestedPaths with comprehensive assertions Update test to use assert.ElementsMatch for more robust resource ARN verification: - Change struct from single expectedResource to expectedResources slice - Update Read nested path test to expect both bucket and prefix ARNs - Use assert.ElementsMatch to verify all generated resources match exactly - Provides complete coverage for nested path handling This matches the improvement pattern used in TestConvertSingleActionSubpath * refactor: simplify S3 action map and improve resource ARN detection - Refactor fineGrainedActionMap to use init() function for programmatic population of both prefixed (s3:Action) and unprefixed (Action) variants, eliminating 70+ duplicate entries - Add buildObjectResourceArn() helper to eliminate duplicated resource ARN generation logic across switch cases - Fix bucket vs object-level access detection: only use HasSuffix(/*) check instead of Contains('/') which incorrectly matched patterns like 'bucket/prefix' without wildcard - Apply buildObjectResourceArn() consistently to Tagging, BypassGovernanceRetention, GetObjectRetention, PutObjectRetention, GetObjectLegalHold, and PutObjectLegalHold cases * fmt * fix: generate object-level ARNs for bucket-level read access When bucket-level read access is granted (e.g., 'Read:mybucket'), generate both bucket and object ARNs so that object-level actions like s3:GetObject can properly authorize. Similarly, in GetResourcesFromLegacyAction, bucket-level patterns should generate both ARN levels for consistency with patterns that include wildcards. This ensures that users with bucket-level permissions can read objects, not just the bucket itself. * fix: address Copilot code review comments - Remove unused bucketName parameter from ConvertIdentityToPolicy signature - Update all callers in examples.go and engine_test.go - Bucket is now extracted from action string itself - Update extractBucketAndPrefix documentation - Add nested path example (bucket/a/b/c/*) - Clarify that prefix can contain multiple path segments - Make GetResourcesFromLegacyAction action-aware - Different action types have different resource requirements - List actions only need bucket ARN (bucket-only operations) - Read/Write/Tagging actions need both bucket and object ARNs - Aligns with convertSingleAction logic for consistency All tests pass successfully * test: add comprehensive tests for GetResourcesFromLegacyAction consistency - Add TestGetResourcesFromLegacyAction to verify action-aware resource generation - Validate consistency with convertSingleAction for all action types: * List actions: bucket-only ARNs (s3:ListBucket is bucket-level operation) * Read actions: both bucket and object ARNs * Write actions: object-only ARNs (subpaths) or object ARNs (bucket-level) * Admin actions: both bucket and object ARNs - Update GetResourcesFromLegacyAction to generate Admin ARNs consistent with convertSingleAction - All tests pass (35+ test cases across integration_test.go) * refactor: eliminate code duplication in GetResourcesFromLegacyAction - Simplify GetResourcesFromLegacyAction to delegate to convertSingleAction - Eliminates ~50 lines of duplicated action-type-specific logic - Ensures single source of truth for resource ARN generation - Improves maintainability: changes to ARN logic only need to be made in one place - All tests pass: any inconsistencies would be caught immediately - Addresses Gemini Code Assist review comment about code duplication * fix: remove fragile 'dummy' action type in CreatePolicyFromLegacyIdentity - Replace hardcoded 'dummy:' prefix with proper representative action type - Use first valid action type from the action list to determine resource requirements - Ensures GetResourcesFromLegacyAction receives a valid action type - Prevents silent failures when convertSingleAction encounters unknown action - Improves code clarity: explains why representative action type is needed - All tests pass: policy engine tests verify correct behavior * security: prevent privilege escalation in Admin action subpath handling - Admin action with subpath (e.g., Admin:bucket/admin/*) now correctly restricts to the specified subpath instead of granting full bucket access - If prefix exists: resources restricted to bucket + bucket/prefix/* - If no prefix: full bucket access (unchanged behavior for root Admin) - Added test case Admin_on_subpath to validate the security fix - All 40+ policy engine tests pass * refactor: address Copilot code review comments on S3 authorization - Fix GetObjectTagging mapping: change from ACTION_READ to ACTION_TAGGING (tagging operations should not be classified as general read operations) - Enhance extractBucketAndPrefix edge case handling: - Add input validation (reject empty strings, whitespace, slash-only) - Normalize double slashes and trailing slashes - Return empty bucket/prefix for invalid patterns - Prevent generation of malformed ARNs - Separate Read action from ListBucket (AWS S3 IAM semantics): - ListBucket is a bucket-level operation, not object-level - Read action now only includes s3:GetObject, s3:GetObjectVersion - This aligns with AWS S3 IAM policy best practices - Update buildObjectResourceArn to handle invalid bucket names gracefully: - Return empty slice if bucket is empty after validation - Prevents malformed ARN generation - Add comprehensive TestExtractBucketAndPrefixEdgeCases with 8 test cases: - Validates empty strings, whitespace, special characters - Confirms proper normalization of double/trailing slashes - Ensures robust parsing of nested paths - Update existing tests to reflect removed ListBucket from Read action All 40+ policy engine tests pass * fix: aggregate resource ARNs from all action types in CreatePolicyFromLegacyIdentity CRITICAL FIX: The previous implementation incorrectly used a single representative action type to determine resource ARNs when multiple legacy actions targeted the same resource pattern. This caused incorrect policy generation when action types with different resource requirements (e.g., List vs Write) were grouped together. Example of the bug: - Input: List:mybucket/path/*, Write:mybucket/path/* - Old behavior: Used only List's resources (bucket-level ARN) - Result: Policy had Write actions (s3:PutObject) but only bucket ARN - Consequence: s3:PutObject would be denied due to missing object-level ARN Solution: - Iterate through all action types for a given resource pattern - For each action type, call GetResourcesFromLegacyAction to get required ARNs - Aggregate all ARNs into a set to eliminate duplicates - Use the merged set for the final policy statement - Admin action short-circuits (always includes full permissions) Example of correct behavior: - Input: List:mybucket/path/*, Write:mybucket/path/* - New behavior: Aggregates both List and Write resource requirements - Result: Policy has Write actions with BOTH bucket and object-level ARNs - Outcome: s3:PutObject works correctly on mybucket/path/* Added TestCreatePolicyFromLegacyIdentityMultipleActions with 3 test cases: 1. List + Write on subpath: verifies bucket + object ARN aggregation 2. Read + Tagging on bucket: verifies action-specific ARN combinations 3. Admin with other actions: verifies Admin dominates resource ARNs All 45+ policy engine tests pass * fix: remove bucket-level ARN from Read action for consistency ISSUE: The Read action was including bucket-level ARNs (arn:aws:s3:::bucket) even though the only S3 actions in Read are s3:GetObject and s3:GetObjectVersion, which are object-level operations. This created a mismatch between the actions and resources in the policy statement. ROOT CAUSE: s3:ListBucket was previously removed from the Read action, but the bucket-level ARN was not removed, creating an inconsistency. SOLUTION: Update Read action to only generate object-level ARNs using buildObjectResourceArn, consistent with how Write and Tagging actions work. This ensures: - Read:mybucket generates arn:aws:s3:::mybucket/* (not bucket ARN) - Read:bucket/prefix/* generates arn:aws:s3:::bucket/prefix/* (object-level only) - Consistency: same actions, same resources, same logic across all object operations Updated test expectations: - TestConvertSingleActionSubpath: Read_on_subpath now expects only object ARN - TestConvertSingleActionNestedPaths: Read nested path now expects only object ARN - TestConvertIdentityToPolicy: Read resources now 1 instead of 2 - TestCreatePolicyFromLegacyIdentityMultipleActions: Read+Tagging aggregates to 1 ARN All 45+ policy engine tests pass * doc * fmt * fix: address Copilot code review on Read action consistency and missing S3 action mappings - Clarify MapToStatementAction comment to reflect exact lookup (not pattern matching) - Add missing S3 actions to baseS3ActionMap: - ListBucketVersions, ListAllMyBuckets for bucket operations - GetBucketCors, PutBucketCors, DeleteBucketCors for CORS - GetBucketNotification, PutBucketNotification for notifications - GetBucketObjectLockConfiguration, PutBucketObjectLockConfiguration for object lock - GetObjectVersionTagging for version tagging - GetObjectVersionAcl, PutBucketAcl for ACL operations - PutBucketTagging, DeleteBucketTagging for bucket tagging - Fix Read action scope inconsistency with GetActionMappings(): - Previously: only included GetObject, GetObjectVersion - Now: includes full Read set (14 actions) from GetActionMappings - Includes both bucket-level (ListBucket*, GetBucket*) and object-level (GetObject*) ARNs - Bucket ARN enables ListBucket operations, object ARN enables GetObject operations - Update all test expectations: - TestConvertSingleActionSubpath: Read now returns 2 ARNs (bucket + objects) - TestConvertSingleActionNestedPaths: Read nested path now includes bucket ARN - TestGetResourcesFromLegacyAction: Read test cases updated for consistency - TestCreatePolicyFromLegacyIdentityMultipleActions: Read_and_Tagging now returns 2 ARNs - TestConvertIdentityToPolicy: Updated to expect 14 Read actions and 2 resources Fixes: Inconsistency between convertSingleAction Read case and GetActionMappings function * fmt * fix: align convertSingleAction with GetActionMappings and add bucket validation - Fix Write action: now includes all 16 actions from GetActionMappings (object and bucket operations) - Includes PutBucketVersioning, PutBucketCors, PutBucketAcl, PutBucketTagging, etc. - Generates both bucket and object ARNs to support bucket-level operations - Fix List action: add ListAllMyBuckets from GetActionMappings - Previously: ListBucket, ListBucketVersions - Now: ListBucket, ListBucketVersions, ListAllMyBuckets - Add bucket validation to prevent malformed ARNs with empty bucket - Fix Tagging action: include bucket-level tagging operations - Previously: only object-level (GetObjectTagging, PutObjectTagging, DeleteObjectTagging) - Now: includes bucket-level (GetBucketTagging, PutBucketTagging, DeleteBucketTagging) - Generates both bucket and object ARNs to support bucket-level operations - Add bucket validation to prevent malformed ARNs: - Admin: return error if bucket is empty - List: generate empty resources if bucket is empty - Tagging: check bucket before generating ARNs - GetBucketObjectLockConfiguration, PutBucketObjectLockConfiguration: validate bucket - Fix TrimRight issue in extractBucketAndPrefix: - Change from strings.TrimRight(pattern, "/") to remove only one trailing slash - Prevents loss of prefix when pattern has multiple trailing slashes - Update all test cases: - TestConvertSingleActionSubpath: Write now returns 16 actions and bucket+object ARNs - TestConvertSingleActionNestedPaths: Write includes bucket ARN - TestGetResourcesFromLegacyAction: Updated Write and Tagging expectations - TestCreatePolicyFromLegacyIdentityMultipleActions: Updated action/resource counts Fixes: Inconsistencies between convertSingleAction and GetActionMappings for Write/List/Tagging actions * fmt * fix: resolve ListMultipartUploads/ListParts mapping inconsistency and add action validation - Fix ListMultipartUploads and ListParts mapping in helpers.go: - Changed from ACTION_LIST to ACTION_WRITE for consistency with GetActionMappings - These operations are part of the multipart write workflow and should map to Write action - Prevents inconsistent behavior when same actions processed through different code paths - Add documentation to clarify multipart operations in Write action: - Explain why ListMultipartUploads and ListParts are part of Write permissions - These are required for meaningful multipart upload workflow management - Add action validation to CreatePolicyFromLegacyIdentity: - Validates action format before processing using ValidateActionMapping - Logs warnings for invalid actions instead of silently skipping them - Provides clearer error messages when invalid action types are used - Ensures users know when their intended permissions weren't applied - Consistent with ConvertLegacyActions validation approach Fixes: Inconsistent action type mappings and silent failure for invalid actions * fix: restore TimeToFirstByte metric for S3 GetObject operations (issue #7869) --- weed/s3api/s3api_object_handlers.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/weed/s3api/s3api_object_handlers.go b/weed/s3api/s3api_object_handlers.go index 9b1495128..054a26264 100644 --- a/weed/s3api/s3api_object_handlers.go +++ b/weed/s3api/s3api_object_handlers.go @@ -1028,6 +1028,9 @@ func (s3a *S3ApiServer) streamFromVolumeServers(w http.ResponseWriter, r *http.R w.WriteHeader(http.StatusOK) } + // Track time to first byte metric + TimeToFirstByte(r.Method, t0, r) + // Stream directly to response with counting wrapper tStreamExec := time.Now() glog.V(4).Infof("streamFromVolumeServers: starting streamFn, offset=%d, size=%d", offset, size) @@ -1236,8 +1239,13 @@ func (s3a *S3ApiServer) streamFromVolumeServersWithSSE(w http.ResponseWriter, r // Now write status code (headers are all set) if isRangeRequest { w.WriteHeader(http.StatusPartialContent) + } else { + w.WriteHeader(http.StatusOK) } + // Track time to first byte metric + TimeToFirstByte(r.Method, t0, r) + // Full Range Optimization: Use ViewFromChunks to only fetch/decrypt needed chunks tDecryptSetup := time.Now() From 911aca74f3294592c0b900ded1af47c0143be173 Mon Sep 17 00:00:00 2001 From: Sheya Bernstein Date: Wed, 24 Dec 2025 18:52:40 +0000 Subject: [PATCH 3/3] Support volume server ID in Helm chart (#7867) helm: Support volume server ID --- k8s/charts/seaweedfs/templates/volume/volume-statefulset.yaml | 3 +++ k8s/charts/seaweedfs/values.yaml | 4 ++++ 2 files changed, 7 insertions(+) diff --git a/k8s/charts/seaweedfs/templates/volume/volume-statefulset.yaml b/k8s/charts/seaweedfs/templates/volume/volume-statefulset.yaml index 6a551a6c9..045b95c2e 100644 --- a/k8s/charts/seaweedfs/templates/volume/volume-statefulset.yaml +++ b/k8s/charts/seaweedfs/templates/volume/volume-statefulset.yaml @@ -176,6 +176,9 @@ spec: {{- if $volume.dataCenter }} -dataCenter={{ $volume.dataCenter }} \ {{- end }} + {{- if $volume.id }} + -id={{ $volume.id }} \ + {{- end }} -ip.bind={{ $volume.ipBind }} \ -readMode={{ $volume.readMode }} \ {{- if $volume.whiteList }} diff --git a/k8s/charts/seaweedfs/values.yaml b/k8s/charts/seaweedfs/values.yaml index dd14f1ca0..c4fd3a841 100644 --- a/k8s/charts/seaweedfs/values.yaml +++ b/k8s/charts/seaweedfs/values.yaml @@ -401,6 +401,10 @@ volume: # Volume server's rack name rack: null + # Stable identifier for the volume server, independent of IP address + # Useful for Kubernetes environments with hostPath volumes to maintain stable identity + id: null + # Volume server's data center name dataCenter: null