Mirror/seaweedfs - seaweedfs - Gitea: Git with a cup of tea

Author	SHA1	Message	Date
Chris Lu	ad76487e9d	Fix special characters in admin-generated secret keys (#7994 ) Fixes #7990 The issue was that the Charset constant used for generating secret keys included the '/' character, which is URL-unsafe. When secret keys containing '/' were used in HTTP requests, they would be URL-encoded, causing a mismatch during signature verification. Changes: - Removed '/' from the Charset constant in weed/iam/constants.go - Added TestGenerateSecretAccessKey_URLSafe to verify generated keys don't contain URL-unsafe characters like '/' or '+' This ensures all newly generated secret keys are URL-safe and will work correctly with S3 authentication. Existing keys continue to work.	4 weeks ago
Chris Lu	5469b7c58f	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	2 months ago
chrislu	4a764dbb37	fmt	2 months ago
chrislu	f734b2d4bf	Refactor: Extract common IAM logic into shared weed/iam package (#7747 ) This resolves GitHub issue #7747 by extracting duplicated IAM code into a shared package that both the embedded S3 IAM and standalone IAM use. New shared package (weed/iam/): - constants.go: Common constants (charsets, action strings, error messages) - helpers.go: Shared helper functions (Hash, GenerateRandomString, GenerateAccessKeyId, GenerateSecretAccessKey, StringSlicesEqual, MapToStatementAction, MapToIdentitiesAction, MaskAccessKey) - responses.go: Common IAM response structs (CommonResponse, ListUsersResponse, CreateUserResponse, etc.) - helpers_test.go: Unit tests for shared helpers Updated files: - weed/s3api/s3api_embedded_iam.go: Use type aliases and function wrappers to the shared package, removing ~200 lines of duplicated code - weed/iamapi/iamapi_management_handlers.go: Use shared package for constants and helper functions, removing ~100 lines of duplicated code - weed/iamapi/iamapi_response.go: Re-export types from shared package for backwards compatibility Benefits: - Single source of truth for IAM constants and helpers - Easier maintenance - changes only need to be made in one place - Reduced risk of inconsistencies between embedded and standalone IAM - Better test coverage through shared test suite	2 months ago