Branch: master

add-ec-vacuum

add_fasthttp_client

add_remote_storage

adding-message-queue-integration-tests

adjust-fsck-cutoff-default

also-delete-parent-directory-if-empty

avoid_releasing_temp_file_on_write

changing-to-zap

collect-public-metrics

copilot/fix-helm-chart-installation

copilot/fix-s3-object-tagging-issue

copilot/make-renew-interval-configurable

copilot/make-renew-interval-configurable-again

copilot/sub-pr-7677

create-table-snapshot-api-design

data_query_pushdown

dependabot/maven/other/java/client/com.google.protobuf-protobuf-java-3.25.5

dependabot/maven/other/java/examples/org.apache.hadoop-hadoop-common-3.4.0

detect-and-plan-ec-tasks

do-not-retry-if-error-is-NotFound

ec-disk-type-support

enhance-erasure-coding

fasthttp

feature/mini-port-detection

feature/modernize-s3-tests

filer1_maintenance_branch

fix-GetObjectLockConfigurationHandler

fix-bucket-name-case-7910

fix-mount-http-parallelism

fix-mount-read-throughput-7504

fix-pr-7909

fix-s3-object-tagging-issue-7589

fix-sts-session-token-7941

fix-versioning-listing-only

ftp

gh-pages

improve-fuse-mount

improve-fuse-mount2

logrus

master

message_send

mount2

mq-subscribe

mq2

nfs-cookie-prefix-list-fixes

optimize-delete-lookups

original_weed_mount

pr-7412

raft-dual-write

random_access_file

refactor-needle-read-operations

refactor-volume-write

remote_overlay

remove-implicit-directory-handling

revert-5134-patch-1

revert-5819-patch-1

revert-6434-bugfix-missing-s3-audit

s3-remote-cache-singleflight

s3-select

sub

tcp_read

test-reverting-lock-table

test_udp

testing

testing-sdx-generation

tikv

track-mount-e2e

upgrade-versions-to-4.00

volume_buffered_writes

worker-execute-ec-tasks

0.72

0.72.release

0.73

0.74

0.75

0.76

0.77

0.90

0.91

0.92

0.93

0.94

0.95

0.96

0.97

0.98

0.99

1.00

1.01

1.02

1.03

1.04

1.05

1.06

1.07

1.08

1.09

1.10

1.11

1.12

1.14

1.15

1.16

1.17

1.18

1.19

1.20

1.21

1.22

1.23

1.24

1.25

1.26

1.27

1.28

1.29

1.30

1.31

1.32

1.33

1.34

1.35

1.36

1.37

1.38

1.40

1.41

1.42

1.43

1.44

1.45

1.46

1.47

1.48

1.49

1.50

1.51

1.52

1.53

1.54

1.55

1.56

1.57

1.58

1.59

1.60

1.61

1.61RC

1.62

1.63

1.64

1.65

1.66

1.67

1.68

1.69

1.70

1.71

1.72

1.73

1.74

1.75

1.76

1.77

1.78

1.79

1.80

1.81

1.82

1.83

1.84

1.85

1.86

1.87

1.88

1.90

1.91

1.92

1.93

1.94

1.95

1.96

1.97

1.98

1.99

1;70

2.00

2.01

2.02

2.03

2.04

2.05

2.06

2.07

2.08

2.09

2.10

2.11

2.12

2.13

2.14

2.15

2.16

2.17

2.18

2.19

2.20

2.21

2.22

2.23

2.24

2.25

2.26

2.27

2.28

2.29

2.30

2.31

2.32

2.33

2.34

2.35

2.36

2.37

2.38

2.39

2.40

2.41

2.42

2.43

2.47

2.48

2.49

2.50

2.51

2.52

2.53

2.54

2.55

2.56

2.57

2.58

2.59

2.60

2.61

2.62

2.63

2.64

2.65

2.66

2.67

2.68

2.69

2.70

2.71

2.72

2.73

2.74

2.75

2.76

2.77

2.78

2.79

2.80

2.81

2.82

2.83

2.84

2.85

2.86

2.87

2.88

2.89

2.90

2.91

2.92

2.93

2.94

2.95

2.96

2.97

2.98

2.99

3.00

3.01

3.02

3.03

3.04

3.05

3.06

3.07

3.08

3.09

3.10

3.11

3.12

3.13

3.14

3.15

3.16

3.18

3.19

3.20

3.21

3.22

3.23

3.24

3.25

3.26

3.27

3.28

3.29

3.30

3.31

3.32

3.33

3.34

3.35

3.36

3.37

3.38

3.39

3.40

3.41

3.42

3.43

3.44

3.45

3.46

3.47

3.48

3.50

3.51

3.52

3.53

3.54

3.55

3.56

3.57

3.58

3.59

3.60

3.61

3.62

3.63

3.64

3.65

3.66

3.67

3.68

3.69

3.71

3.72

3.73

3.74

3.75

3.76

3.77

3.78

3.79

3.80

3.81

3.82

3.83

3.84

3.85

3.86

3.87

3.88

3.89

3.90

3.91

3.92

3.93

3.94

3.95

3.96

3.97

3.98

3.99

4.00

4.01

4.02

4.03

4.04

4.05

dev

helm-3.65.1

v0.69

v0.70beta

v3.33

Branches Tags

${ item.name }

Create tag ${ searchTerm }

Create branch ${ searchTerm }

from 'master'

${ noResults }

Commit Graph

770 Commits (master)

Author	SHA1	Message	Date
Chris Lu	1ea6b0c0d9	cleanup: deduplicate environment variable credential loading Previously, `weed mini` logic duplicated the credential loading process by creating a temporary IAM config file from environment variables. `auth_credentials.go` also had fallback logic to load these variables. This change: 1. Updates `auth_credentials.go` to *always* check for and merge AWS environment variable credentials (`AWS_ACCESS_KEY_ID`, etc.) into the identity list. This ensures they are available regardless of whether other configurations (static file or filer) are loaded. 2. Removes the redundant file creation logic from `weed/command/mini.go`. 3. Updates `weed mini` user messages to accurately reflect that credentials are loaded from environment variables in-memory. This results in a cleaner implementation where `weed/s3api` manages all credential loading logic, and `weed mini` simply relies on it.	18 hours ago
Chris Lu	7f1182472a	fix: enable dual loading of static and dynamic IAM configuration Refactored `NewIdentityAccessManagementWithStore` to remove mutual exclusivity between static (file-based) and dynamic (filer-based) configuration loading. Previously, if a static config configuration was present (including the legacy `IamConfig` option used by `weed mini`), it prevented loading users from the filer. Now, the system loads the static configuration first (if present), and then *always* attempts to merge in the dynamic configuration from the filer. This ensures that: 1. Static users (e.g. from `weed mini` env vars or `-s3.config`) are loaded and protected. 2. Dynamic users (e.g. created via Admin UI and stored in Filer) are also loaded and available.	18 hours ago
Chris Lu	451b897d56	fix: support loading static config from IamConfig option for mini mode `weed mini` sets the `-s3.iam.config` flag instead of `-s3.config`, which populates `S3ApiServerOption.IamConfig`. Previously, `NewIdentityAccessManagementWithStore` only checked `option.Config`. This caused `weed mini` generated credentials (written to a temp file passed via IamConfig) to be ignored, breaking S3 access in mini mode even when environment variables were provided. This change ensures we try to load the configuration from `IamConfig` if `Config` is empty, restoring functionality for `weed mini`.	18 hours ago
Chris Lu	48ded6b965	fix: allow environment variable fallback when filer config is empty Fixed regression where AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY environment variables were not being loaded as fallback credentials. The issue was that configLoaded was set to true when filer call succeeded, even if it returned an empty configuration. This blocked the environment variable fallback logic. Now only set configLoaded = true when we actually have loaded identities, allowing env vars to work correctly in mini mode and other scenarios where filer config is empty.	18 hours ago
Chris Lu	4e835a1d81	fix(s3api): ensure S3 configuration persistence and refactor authorization tests (#7989) * fix(s3api): ensure static config file takes precedence over dynamic updates When a static S3 configuration file is provided, avoid overwriting the configuration from dynamic filer updates. This ensures the documented "Highest Priority" for the configuration file is respected. * refactor(s3api): implement merge-based static config with immutable identities Static identities from config file are now immutable and protected from dynamic updates. Dynamic identities (from admin panel) can be added and updated without affecting static entries. - Track identity names loaded from static config file - Implement merge logic that preserves static identities - Allow dynamic identities to be added or updated - Remove blanket block on config file updates * fix: address PR review comments for static config merge logic Critical Bugs: - Fix existingIdx always-false condition causing duplicate identities - Fix race condition in static config initialization (move useStaticConfig inside mutex) Security & Robustness: - Add nil identity check in VerifyActionPermission to fail closed - Mask access keys in STS validation logs to avoid exposing credentials - Add nil guard for s3a.iam in subscription handler Test Improvements: - Add authCalled tracking to MockIAMIntegration for explicit verification - Lower log level for static config messages to reduce noise * fix: prevent duplicates and race conditions in merge logic Data Integrity: - Prevent service account credential duplicates on repeated merges - Clean up stale accessKeyIdent entries when replacing identities - Check existing credentials before appending Concurrency Safety: - Add synchronization to IsStaticConfig method Test Improvements: - Add mux route vars for proper GetBucketAndObject extraction - Add STS session token header to trigger correct auth path	19 hours ago
Chris Lu	abfa64456b	Fix STS authorization in streaming/chunked uploads (#7988) * Fix STS authorization in streaming/chunked uploads During streaming/chunked uploads (SigV4 streaming), authorization happens twice: 1. Initial authorization in authRequestWithAuthType() - works correctly 2. Second authorization in verifyV4Signature() - was failing for STS The issue was that verifyV4Signature() only used identity.canDo() for permission checks, which always denies STS identities (they have empty Actions). This bypassed IAM authorization completely. This commit makes verifyV4Signature() IAM-aware by adding the same fallback logic used in authRequestWithAuthType(): - Traditional identities (with Actions) use legacy canDo() check - STS/JWT identities (empty Actions) fall back to IAM authorization Fixes: https://github.com/seaweedfs/seaweedfs/pull/7986#issuecomment-3723196038 * Add comprehensive unit tests for STS authorization in streaming uploads Created test suite to verify that verifyV4Signature properly handles STS identities by falling back to IAM authorization when shouldCheckPermissions is true. Tests cover: - STS identities with IAM integration (allow and deny cases) - STS identities without IAM integration (should deny) - Traditional identities with Actions (canDo check) - Permission check bypass when shouldCheckPermissions=false - Specific streaming upload scenario from bug report - Action determination based on HTTP method All tests pass successfully. * Refactor authorization logic to avoid duplication Centralized the authorization logic into IdentityAccessManagement.VerifyActionPermission. Updated auth_signature_v4.go and auth_credentials.go to use this new helper. Updated tests to clarify that they mirror the centralized logic. * Refactor tests to use VerifyActionPermission directly Introduced IAMIntegration interface to facilitate mocking of internal IAM integration logic. Updated IdentityAccessManagement to use the interface. Updated tests to directy call VerifyActionPermission using a mocked IAM integration, eliminating duplicated logic in tests. * fix(s3api): ensure static config file takes precedence and refactor tests - Track if configuration was loaded from a static file using `useStaticConfig`. - Ignore filer-based IAM updates when a static configuration is in use to respect "Highest Priority" rule. - Refactor `TestVerifyV4SignatureWithSTSIdentity` to use `VerifyActionPermission` directly. - Fix typed nil interface panic in authorization test.	21 hours ago
Chris Lu	217d8b9e0e	Fix: ListObjectVersions delimiter support (#7987) * Fix: Add delimiter support to ListObjectVersions with proper truncation - Implemented delimiter support to group keys into CommonPrefixes - Fixed critical truncation bug: now merges versions and common prefixes into single sorted list before truncation - Ensures total items never exceed MaxKeys (prevents infinite pagination loops) - Properly sets NextKeyMarker and NextVersionIdMarker for pagination - Added integration tests in test/s3/versioning/s3_versioning_delimiter_test.go - Verified behavior matches S3 API specification * Fix: Add delimiter support to ListObjectVersions with proper truncation - Implemented delimiter support to group keys into CommonPrefixes - Fixed critical truncation bug: now merges versions and common prefixes before truncation - Added safety guard for maxKeys=0 to prevent panics - Condensed verbose comments for better readability - Added robust Go integration tests with nil checks for AWS SDK pointers - Verified behavior matches S3 API specification - Resolved compilation error in integration tests - Refined pagination comments and ensured exclusive KeyMarker behavior - Refactored listObjectVersions into helper methods for better maintainability	1 day ago
Chris Lu	4ba89bf73b	adjust log level	1 day ago
Chris Lu	5a3aade445	less logs	1 day ago
promalert	9012069bd7	chore: execute goimports to format the code (#7983) * chore: execute goimports to format the code Signed-off-by: promalert <promalert@outlook.com> * goimports -w . --------- Signed-off-by: promalert <promalert@outlook.com> Co-authored-by: Chris Lu <chris.lu@gmail.com>	2 days ago
Chris Lu	6432019d08	Fix STS identity authorization by populating PolicyNames (#7985) (#7986) * Fix STS identity authorization by populating PolicyNames (#7985) This commit fixes GitHub issue #7985 where STS-assumed identities received empty identity.Actions, causing all S3 operations to be denied even when the role had valid IAM policies attached. Changes: 1. Populate PolicyNames field from sessionInfo.Policies in validateSTSSessionToken() to enable IAM-based authorization for STS identities 2. Fix bucket+objectKey path construction in canDo() method to include proper slash separator between bucket and object key 3. Add comprehensive test suite to validate the fix and prevent regression The fix ensures that STS-assumed identities are properly authorized through the IAM path when iamIntegration is available, allowing roles with valid IAM policies to perform S3 operations as expected. * Update STS identity tests to be more rigorous and use actual implementation path * Fix regression in canDo() path concatenation The previous fix blindly added a slash separator, which caused double slashes when objectKey already started with a slash (common in existing tests and some code paths). This broke TestCanDo and TestObjectLevelListPermissions. This commit updates the logic to only add the slash separator if objectKey is not empty and does not already start with a slash. This fixes the regressions while maintaining the fix for issue #7985. * Refactor STS identity tests: extract helpers and simplify redundant logic - Extracted setupTestSTSService and newTestIdentity helper functions - Removed redundant if-else verification blocks that were already covered by assertions - Cleaned up test cases to improve maintainability as suggested in code review. * Add canDo() verification to STS identity tests Address code review suggestion: verify that identities with empty Actions correctly return false for canDo() checks, which confirms the behavior that forces authorization to fall back to the IAM path. * Simplify TestCanDoPathConstruction variable names Rename expectedPath to fullPath and simplify logging/assertion logic based on code review feedback. * Refactor path construction and logging in canDo() - Compute fullPath early and use it for logging to prevent double slashes - Update TestCanDoPathConstruction to use robust path verification - Add test case for objectKey with leading slash to ensure correct handling	2 days ago
Chris Lu	e67973dc53	Support Policy Attachment for Object Store Users (#7981) * Implement Policy Attachment support for Object Store Users - Added policy_names field to iam.proto and regenerated protos. - Updated S3 API and IAM integration to support direct policy evaluation for users. - Enhanced Admin UI to allow attaching policies to users via modals. - Renamed 'policies' to 'policy_names' to clarify that it stores identifiers. - Fixed syntax error in user_management.go. * Fix policy dropdown not populating The API returns {policies: [...]} but JavaScript was treating response as direct array. Updated loadPolicies() to correctly access data.policies property. * Add null safety checks for policy dropdowns Added checks to prevent "undefined" errors when: - Policy select elements don't exist - Policy dropdowns haven't loaded yet - User is being edited before policies are loaded * Fix policy dropdown by using correct JSON field name JSON response has lowercase 'name' field but JavaScript was accessing 'Name'. Changed policy.Name to policy.name to match the IAMPolicy JSON structure. * Fix policy names not being saved on user update Changed condition from len(req.PolicyNames) > 0 to req.PolicyNames != nil to ensure policy names are always updated when present in the request, even if it's an empty array (to allow clearing policies). * Add debug logging for policy names update flow Added console.log in frontend and glog in backend to trace policy_names data through the update process. * Temporarily disable auto-reload for debugging Commented out window.location.reload() so console logs are visible when updating a user. * Add detailed debug logging and alert for policy selection Added console.log for each step and an alert to show policy_names value to help diagnose why it's not being included in the request. * Regenerate templ files for object_store_users Ran templ generate to ensure _templ.go files are up to date with the latest .templ changes including debug logging. * Remove debug logging and restore normal functionality Cleaned up temporary debug code (console.log and alert statements) and re-enabled automatic page reload after user update. * Add step-by-step alert debugging for policy update Added 5 alert checkpoints to trace policy data through the update flow: 1. Check if policiesSelect element exists 2. Show selected policy values 3. Show userData.policy_names 4. Show full request body 5. Confirm server response Temporarily disabled auto-reload to see alerts. * Add version check alert on page load Added alert on DOMContentLoaded to verify new JavaScript is being executed and not cached by the browser. * Compile templates using make Ran make to compile all template files and install the weed binary. * Add button click detection and make handleUpdateUser global - Added inline alert on button click to verify click is detected - Made handleUpdateUser a window-level function to ensure it's accessible - Added alert at start of handleUpdateUser function * Fix handleUpdateUser scope issue - remove duplicate definition Removed duplicate function definition that was inside DOMContentLoaded. Now handleUpdateUser is defined only once in global scope (line 383) making it accessible when button onclick fires. * Remove all duplicate handleUpdateUser definitions Now handleUpdateUser is defined only once at the very top of the script block (line 352), before DOMContentLoaded, ensuring it's available when the button onclick fires. * Add function existence check and error catching Added alerts to check if handleUpdateUser is defined and wrapped the function call in try-catch to capture any JavaScript errors. Also added console.log statements to verify function definition. * Simplify handleUpdateUser to non-async for testing Removed async/await and added early return to test if function can be called at all. This will help identify if async is causing the issue. * Add cache-control headers to prevent browser caching Added no-cache headers to ShowObjectStoreUsers handler to prevent aggressive browser caching of inline JavaScript in the HTML page. * Fix syntax error - make handleUpdateUser async Changed function back to async to fix 'await is only valid in async functions' error. The cache-control headers are working - browser is now loading new code. * Update version check to v3 to verify cache busting Changed version alert to 'v3 - WITH EARLY RETURN' to confirm the new code with early return statement is being loaded. * Remove all debug code - clean implementation Removed all alerts, console.logs, and test code. Implemented clean policy update functionality with proper error handling. * Add ETag header for cache-busting and update walkthrough * Fix policy pre-selection in Edit User modal - Updated admin.js editUser function to pre-select policies - Root cause: duplicate editUser in admin.js overwrote inline version - Added policy pre-selection logic to match inline template - Verified working in browser: policies now pre-select correctly * Fix policy persistence in handleUpdateUser - Added policy_names field to userData payload in handleUpdateUser - Policies were being lost because handleUpdateUser only sent email and actions - Now collects selected policies from editPolicies dropdown - Verified working: policies persist correctly across updates * Fix XSS vulnerability in access keys display - Escape HTML in access key display using escapeHtml utility - Replace inline onclick handlers with data attributes - Add event delegation for delete access key buttons - Prevents script injection via malicious access key values * Fix additional XSS vulnerabilities in user details display - Escape HTML in actions badges (line 626) - Escape HTML in policy_names badges (line 636) - Prevents script injection via malicious action or policy names * Fix XSS vulnerability in loadPolicies function - Replace innerHTML string concatenation with DOM API - Use createElement and textContent for safe policy name insertion - Prevents script injection via malicious policy names - Apply same pattern to both create and edit select elements * Remove debug logging from UpdateObjectStoreUser - Removed glog.V(0) debug statements - Clean up temporary debugging code before production * Remove duplicate handleUpdateUser function - Removed inline handleUpdateUser that duplicated admin.js logic - Removed debug console.log statement - admin.js version is now the single source of truth - Eliminates maintenance burden of keeping two versions in sync * Refine user management and address code review feedback - Preserve PolicyNames in UpdateUserPolicies - Allow clearing actions in UpdateObjectStoreUser by checking for nil - Remove version comment from object_store_users.templ - Refactor loadPolicies for DRYness using cloneNode while keeping DOM API security * IAM Authorization for Static Access Keys * verified XSS Fixes in Templates * fix div	3 days ago
Chris Lu	d75162370c	Fix trust policy wildcard principal handling (#7970) * Fix trust policy wildcard principal handling This change fixes the trust policy validation to properly support AWS-standard wildcard principals like {"Federated": "*"}. Previously, the evaluatePrincipalValue() function would check for context existence before evaluating wildcards, causing wildcard principals to fail when the context key didn't exist. This forced users to use the plain "*" workaround instead of the more specific {"Federated": "*"} format. Changes: - Modified evaluatePrincipalValue() to check for "*" FIRST before validating against context - Added support for wildcards in principal arrays - Added comprehensive tests for wildcard principal handling - All existing tests continue to pass (no regressions) This matches AWS IAM behavior where "*" in a principal field means "allow any value" without requiring context validation. Fixes: https://github.com/seaweedfs/seaweedfs/issues/7917 * Refactor: Move Principal matching to PolicyEngine This refactoring consolidates all policy evaluation logic into the PolicyEngine, improving code organization and eliminating duplication. Changes: - Added matchesPrincipal() and evaluatePrincipalValue() to PolicyEngine - Added EvaluateTrustPolicy() method for direct trust policy evaluation - Updated statementMatches() to check Principal field when present - Made resource matching optional (trust policies don't have Resources) - Simplified evaluateTrustPolicy() in iam_manager.go to delegate to PolicyEngine - Removed ~170 lines of duplicate code from iam_manager.go Benefits: - Single source of truth for all policy evaluation - Better code reusability and maintainability - Consistent evaluation rules for all policy types - Easier to test and debug All tests pass with no regressions. * Make PolicyEngine AWS-compatible and add unit tests Changes: 1. AWS-Compatible Context Keys: - Changed "seaweed:FederatedProvider" -> "aws:FederatedProvider" - Changed "seaweed:AWSPrincipal" -> "aws:PrincipalArn" - Changed "seaweed:ServicePrincipal" -> "aws:PrincipalServiceName" - This ensures 100% AWS compatibility for trust policies 2. Added Comprehensive Unit Tests: - TestPrincipalMatching: 8 test cases for Principal matching - TestEvaluatePrincipalValue: 7 test cases for value evaluation - TestTrustPolicyEvaluation: 6 test cases for trust policy evaluation - TestGetPrincipalContextKey: 4 test cases for context key mapping - Total: 25 new unit tests for PolicyEngine All tests pass: - Policy engine tests: 54 passed - Integration tests: 9 passed - Total: 63 tests passing * Update context keys to standard AWS/OIDC formats Replaced remaining seaweed: context keys with standard AWS and OIDC keys to ensure 100% compatibility with AWS IAM policies. Mappings: - seaweed:TokenIssuer -> oidc:iss - seaweed:Issuer -> oidc:iss - seaweed:Subject -> oidc:sub - seaweed:SourceIP -> aws:SourceIp Also updated unit tests to reflect these changes. All 63 tests pass successfully. * Add advanced policy tests for variable substitution and conditions Added comprehensive tests inspired by AWS IAM patterns: - TestPolicyVariableSubstitution: Tests ${oidc:sub} variable in resources - TestConditionWithNumericComparison: Tests sts:DurationSeconds condition - TestMultipleConditionOperators: Tests combining StringEquals and StringLike Results: - TestMultipleConditionOperators: ✅ All 3 subtests pass - Other tests reveal need for sts:DurationSeconds context population These tests validate the PolicyEngine's ability to handle complex AWS-compatible policy scenarios. * Fix federated provider context and add DurationSeconds support Changes: - Use iss claim as aws:FederatedProvider (AWS standard) - Add sts:DurationSeconds to trust policy evaluation context - TestPolicyVariableSubstitution now passes ✅ Remaining work: - TestConditionWithNumericComparison partially works (1/3 pass) - Need to investigate NumericLessThanEquals evaluation * Update trust policies to use issuer URL for AWS compatibility Changed trust policy from using provider name ("test-oidc") to using the issuer URL ("https://test-issuer.com") to match AWS standard behavior where aws:FederatedProvider contains the OIDC issuer URL. Test Results: - 10/12 test suites passing - TestFullOIDCWorkflow: ✅ All subtests pass - TestPolicyEnforcement: ✅ All subtests pass - TestSessionExpiration: ✅ Pass - TestPolicyVariableSubstitution: ✅ Pass - TestMultipleConditionOperators: ✅ All subtests pass Remaining work: - TestConditionWithNumericComparison needs investigation - One subtest in TestTrustPolicyValidation needs fix * Fix S3 API tests for AWS compatibility Updated all S3 API tests to use AWS-compatible context keys and trust policy principals: Changes: - seaweed:SourceIP → aws:SourceIp (IP-based conditions) - Federated: "test-oidc" → "https://test-issuer.com" (trust policies) Test Results: - TestS3EndToEndWithJWT: ✅ All 13 subtests pass - TestIPBasedPolicyEnforcement: ✅ All 3 subtests pass This ensures policies are 100% AWS-compatible and portable. * Fix ValidateTrustPolicy for AWS compatibility Updated ValidateTrustPolicy method to check for: - OIDC: issuer URL ("https://test-issuer.com") - LDAP: provider name ("test-ldap") - Wildcard: "*" Test Results: - TestTrustPolicyValidation: ✅ All 3 subtests pass This ensures trust policy validation uses the same AWS-compatible principals as the PolicyEngine. * Fix multipart and presigned URL tests for AWS compatibility Updated trust policies in: - s3_multipart_iam_test.go - s3_presigned_url_iam_test.go Changed "Federated": "test-oidc" → "https://test-issuer.com" Test Results: - TestMultipartIAMValidation: ✅ All 7 subtests pass - TestPresignedURLIAMValidation: ✅ All 4 subtests pass - TestPresignedURLGeneration: ✅ All 4 subtests pass - TestPresignedURLExpiration: ✅ All 4 subtests pass - TestPresignedURLSecurityPolicy: ✅ All 4 subtests pass All S3 API tests now use AWS-compatible trust policies. * Fix numeric condition evaluation and trust policy validation interface Major updates to ensure robust AWS-compatible policy evaluation: 1. **Policy Engine**: Added support for `int` and `int64` types in `evaluateNumericCondition`, fixing issues where raw numbers in policy documents caused evaluation failures. 2. **Trust Policy Validation**: Updated `TrustPolicyValidator` interface and `STSService` to propagate `DurationSeconds` correctly during the double-validation flow (Validation -> STS -> Validation callback). 3. **IAM Manager**: Updated implementation to match the new interface and correctly pass `sts:DurationSeconds` context key. Test Results: - TestConditionWithNumericComparison: ✅ All 3 subtests pass - All IAM and S3 integration tests pass (100%) This resolves the final edge case with DurationSeconds numeric conditions. * Fix MockTrustPolicyValidator interface and unreachable code warnings Updates: 1. Updated MockTrustPolicyValidator.ValidateTrustPolicyForWebIdentity to match new interface signature with durationSeconds parameter 2. Removed unreachable code after infinite loops in filer_backup.go and filer_meta_backup.go to satisfy linter Test Results: - All STS tests pass ✅ - Build warnings resolved ✅ * Refactor matchesPrincipal to consolidate array handling logic Consolidated duplicated logic for []interface{} and []string types by converting them to a unified []interface{} upfront. * Fix malformed AWS docs URL in iam_manager.go comment * dup * Enhance IAM integration tests with negative cases and interface array support Added test cases to TestTrustPolicyWildcardPrincipal to: 1. Verify rejection of roles when principal context does not match (negative test) 2. Verify support for principal arrays as []interface{} (simulating JSON unmarshaled roles) * Fix syntax errors in filer_backup and filer_meta_backup Restored missing closing braces for for-loops and re-added return statements. The previous attempt to remove unreachable code accidentally broke the function structure. Build now passes successfully.	4 days ago
Chris Lu	383c2e3b41	fix: handle range requests on empty objects (size=0) (#7963) * fix: handle range requests on empty objects (size=0) Range requests on empty objects were incorrectly being rejected with: 'invalid range start for ...: 0 >= 0' The validation logic used 'startOffset >= totalSize' which failed when both were 0, incorrectly rejecting valid range requests like bytes=0-1535 on 0-byte files. Fix: Added special case handling before validation to properly return 416 Range Not Satisfiable for any range request on an empty object, per RFC 7233. Fixed at two locations (lines 873 and 1154) in s3api_object_handlers.go * refactor: return 404 for directory objects, not 416 Per S3 semantics, GET requests on directory paths (without trailing "/") should return 404 Not Found, not try to serve them as objects. Updated fix to: 1. Check if entry.IsDirectory and return 404 (S3-compliant) 2. Only return 416 for true empty files (size=0, not directory) This matches AWS S3 behavior where directories don't exist as objects unless they're explicit directory markers ending with "/". * reduce repeated info * refactor: move directory check before range branching This ensures that any Range header (including suffix ranges like bytes=-N) on a directory path (without trailing slash) returns 404 (ErrNoSuchKey) instead of potentially returning 416 or attempting to serve as an object. Applied to both streamFromVolumeServers and streamFromVolumeServersWithSSE. * refactoring	5 days ago
Robert Schade	de3df211d7	store S3 storage class in extended atrributes #7961 (#7962) * store S3 storage class in extended atrributes #7961 * canonical * remove issue reference --------- Co-authored-by: Robert Schade <robert.schade@uni-paderborn.de> Co-authored-by: Chris Lu <chris.lu@gmail.com>	5 days ago
Chris Lu	0647bc24d5	s3api: fix authentication bypass and potential SIGSEGV (Issue #7912) (#7954) * s3api: fix authentication bypass and potential SIGSEGV * s3api: improve security tests with positive cases and nil identity guards * s3api: fix secondary authentication bypass in AuthSignatureOnly * s3api: refactor account loading and refine security tests based on review feedback * s3api: refine security tests with realistic signature failures * Update weed/s3api/auth_security_test.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>	6 days ago
Chris Lu	23fc3f2621	Fix AWS SDK Signature V4 with STS credentials (issue #7941) (#7944) * Add documentation for issue #7941 fix * ensure auth * rm FIX_ISSUE_7941.md * Integrate STS session token validation into V4 signature verification - Check for X-Amz-Security-Token header in verifyV4Signature - Call validateSTSSessionToken for STS requests - Skip regular access key lookup and expiration check for STS sessions * Fix variable scoping in verifyV4Signature for STS session token validation * Add ErrExpiredToken error for better AWS S3 compatibility with STS session tokens * Support STS session token in query parameters for presigned URLs * Fix nil pointer dereference in validateSTSSessionToken * Enhance STS token validation with detailed error diagnostics and logging * Fix missing credentials in STSSessionClaims.ToSessionInfo() * test: Add comprehensive STS session claims validation tests - TestSTSSessionClaimsToSessionInfo: Validates basic claims conversion - TestSTSSessionClaimsToSessionInfoCredentialGeneration: Verifies credential generation - TestSTSSessionClaimsToSessionInfoPreservesAllFields: Ensures all fields are preserved - TestSTSSessionClaimsToSessionInfoEmptyFields: Tests handling of empty/nil fields - TestSTSSessionClaimsToSessionInfoCredentialExpiration: Validates expiration handling All tests pass with proper timing tolerance for credential generation. * perf: Reuse CredentialGenerator instance for STS session claims Optimize ToSessionInfo() to reuse a package-level defaultCredentialGenerator instead of allocating a new CredentialGenerator on every call. This reduces allocation overhead since this method is called frequently during signature verification (potentially once per request). The CredentialGenerator is stateless and deterministic, making it safe to reuse across concurrent calls without synchronization. * refactor: Surface credential generation errors and remove sensitive logging Two improvements to error handling and security: 1. weed/iam/sts/session_claims.go: - Add logging for credential generation failures in ToSessionInfo() - Wrap errors with context (session ID) to aid debugging - Use glog.Warningf() to surface errors instead of silently swallowing them - Add fmt import for error wrapping 2. weed/s3api/auth_signature_v4.go: - Remove debug logging of actual access key IDs (glog.V(2) call) - Security improvement: avoid exposing sensitive access keys even at debug level - Keep warning-level logging that shows only count of available keys This ensures credential generation failures are observable while protecting sensitive authentication material from logs. * test: Verify deterministic credential generation in session claims tests Update TestSTSSessionClaimsToSessionInfoCredentialGeneration to properly verify deterministic credential generation: - Remove misleading comment about 'randomness' - parts of credentials ARE deterministic - Add assertions that AccessKeyId is identical for same SessionId (hash-based, deterministic) - Add assertions that SessionToken is identical for same SessionId (hash-based, deterministic) - Verify Expiration matches when SessionId is identical - Document that SecretAccessKey is NOT deterministic (uses random.Read) - Truncate expiresAt to second precision to avoid timing issues This test now properly verifies that the deterministic components of credential generation work correctly while acknowledging the cryptographic randomness of the secret access key. * test(sts): Assert credentials expiration relative to now in credential expiration tests Replace wallclock assertions comparing tc.expiresAt to time.Now() (which only verified test setup) with assertions that check sessionInfo.Credentials.Expiration relative to time.Now(), thus exercising the code under test. Include clarifying comment for intent. * feat(sts): Add IsExpired helpers and use them in expiration tests - Add Credentials.IsExpired() and SessionInfo.IsExpired() in new file session_helpers.go. - Update TestSTSSessionClaimsToSessionInfoCredentialExpiration to use helpers for clearer intent. * test: revert test-only IsExpired helpers; restore direct expiration assertions Remove session_helpers.go and update TestSTSSessionClaimsToSessionInfoCredentialExpiration to assert against sessionInfo.Credentials.Expiration directly as requested by reviewer., * fix(s3api): restore error return when access key not found Critical fix: The previous cleanup of sensitive logging inadvertently removed the error return statement when access key lookup fails. This caused the code to continue and call isCredentialExpired() on nil pointer, crashing the server. This explains EOF errors in CORS tests - server was panicking on requests with invalid keys. * fix(sts): make secret access key deterministic based on sessionId CRITICAL FIX: The secret access key was being randomly generated, causing signature verification failures when the same session token was used twice: 1. AssumeRoleWithWebIdentity generates random secret key X 2. Client signs request using secret key X 3. Server validates token, regenerates credentials via ToSessionInfo() 4. ToSessionInfo() calls generateSecretAccessKey(), which generates random key Y 5. Server tries to verify signature using key Y, but signature was made with X 6. Signature verification fails (SignatureDoesNotMatch) Solution: Make generateSecretAccessKey() deterministic by using SHA256 hash of 'secret-key:' + sessionId, just like generateAccessKeyId() already does. This ensures: - AssumeRoleWithWebIdentity generates deterministic secret key from sessionId - ToSessionInfo() regenerates the same secret key from the same sessionId - Client signature verification succeeds because keys match Fixes: AWS SDK v2 CORS tests failing with 'ExpiredToken' errors Affected files: - weed/iam/sts/token_utils.go: Updated generateSecretAccessKey() signature and implementation to be deterministic - Updated GenerateTemporaryCredentials() to pass sessionId parameter Tests: All 54 STS tests pass with this fix * test(sts): add comprehensive secret key determinism test coverage Updated tests to verify that secret access keys are now deterministic: 1. Updated TestSTSSessionClaimsToSessionInfoCredentialGeneration: - Changed comment from 'NOT deterministic' to 'NOW deterministic' - Added assertion that same sessionId produces identical secret key - Explains why this is critical for signature verification 2. Added TestSecretAccessKeyDeterminism (new dedicated test): - Verifies secret key is identical across multiple calls with same sessionId - Verifies access key ID and session token are also identical - Verifies different sessionIds produce different credentials - Includes detailed comments explaining why determinism is critical These tests ensure that the STS implementation correctly regenerates deterministic credentials during signature verification. Without determinism, signature verification would always fail because the server would use different secret keys than the client used to sign. * refactor(sts): add explicit zero-time expiration handling Improved defensive programming in IsExpired() methods: 1. Credentials.IsExpired(): - Added explicit check for zero-time expiration (time.Time{}) - Treats uninitialized credentials as expired - Prevents accidentally treating uninitialized creds as valid 2. SessionInfo.IsExpired(): - Added same explicit zero-time check - Treats uninitialized sessions as expired - Protects against bugs where sessions might not be properly initialized This is important because time.Now().After(time.Time{}) returns true, but explicitly checking for zero time makes the intent clear and helps catch initialization bugs during code review and debugging. * refactor(sts): remove unused IsExpired() helper functions The session_helpers.go file contained two unused IsExpired() methods: - Credentials.IsExpired() - SessionInfo.IsExpired() These were never called anywhere in the codebase. The actual expiration checks use: - isCredentialExpired() in weed/s3api/auth_credentials.go (S3 auth) - Direct time.Now().After() checks Removing unused code improves code clarity and reduces maintenance burden. * fix(auth): pass STS session token to IAM authorization for V4 signature auth CRITICAL FIX: Session tokens were not being passed to the authorization check when using AWS Signature V4 authentication with STS credentials. The bug: 1. AWS SDK sends request with X-Amz-Security-Token header (V4 signature) 2. validateSTSSessionToken validates the token, creates Identity with PrincipalArn 3. authorizeWithIAM only checked X-SeaweedFS-Session-Token (JWT auth header) 4. Since it was empty, fell into 'static V4' branch which set SessionToken = '' 5. AuthorizeAction returned ErrAccessDenied because SessionToken was empty The fix (in authorizeWithIAM): - Check X-SeaweedFS-Session-Token first (JWT auth) - If empty, fallback to X-Amz-Security-Token header (V4 STS auth) - If still empty, check X-Amz-Security-Token query param (presigned URLs) - When session token is found with PrincipalArn, use 'STS V4 signature' path - Only use 'static V4' path when there's no session token This ensures: - JWT Bearer auth with session tokens works (existing path) - STS V4 signature auth with session tokens works (new path) - Static V4 signature auth without session tokens works (existing path) Logging updated to distinguish: - 'JWT-based IAM authorization' - 'STS V4 signature IAM authorization' (new) - 'static V4 signature IAM authorization' (clarified) * test(s3api): add comprehensive STS session token authorization test coverage Added new test file auth_sts_v4_test.go with comprehensive tests for the STS session token authorization fix: 1. TestAuthorizeWithIAMSessionTokenExtraction: - Verifies X-SeaweedFS-Session-Token is extracted from JWT auth headers - Verifies X-Amz-Security-Token is extracted from V4 STS auth headers - Verifies X-Amz-Security-Token is extracted from query parameters (presigned URLs) - Verifies JWT tokens take precedence when both are present - Regression test for the bug where V4 STS tokens were not being passed to authorization 2. TestSTSSessionTokenIntoCredentials: - Verifies STS credentials have all required fields (AccessKeyId, SecretAccessKey, SessionToken) - Verifies deterministic generation from sessionId (same sessionId = same credentials) - Verifies different sessionIds produce different credentials - Critical for signature verification: same session must regenerate same secret key 3. TestActionConstantsForV4Auth: - Verifies S3 action constants are available for authorization checks - Ensures ACTION_READ, ACTION_WRITE, etc. are properly defined These tests ensure that: - V4 Signature auth with STS tokens properly extracts and uses session tokens - Session tokens are prioritized correctly (JWT > X-Amz-Security-Token header > query param) - STS credentials are deterministically generated for signature verification - The fix for passing STS session tokens to authorization is properly covered All 3 test functions pass (6 test cases total). * refactor(s3api): improve code quality and performance - Rename authorization path constants to avoid conflict with existing authType enum - Replace nested if/else with clean switch statement in authorizeWithIAM() - Add determineIAMAuthPath() helper for clearer intent and testability - Optimize key counting in auth_signature_v4.go: remove unnecessary slice allocation - Fix timing assertion in session_claims_test.go: use WithinDuration for symmetric tolerance These changes improve code readability, maintainability, and performance while maintaining full backward compatibility and test coverage. * refactor(s3api): use typed iamAuthPath for authorization path constants - Define iamAuthPath as a named string type (similar to existing authType enum) - Update constants to use explicit type: iamAuthPathJWT, iamAuthPathSTS_V4, etc. - Update determineIAMAuthPath() to return typed iamAuthPath - Improves type safety and prevents accidental string value misuse	6 days ago
Chris Lu	4d4b2e2d4a	add debug messages	7 days ago
Chris Lu	f2373f9e8d	fix: directory incorrectly listed as object in S3 ListObjects (#7939) * fix: directory incorrectly listed as object in S3 ListObjects Regular directories (without MIME type) were only added to CommonPrefixes when delimiter was exactly '/'. This caused directories to be silently skipped for other delimiter values. Changed the condition from 'delimiter == "/"' to 'delimiter != ""' to ensure directories are correctly added to CommonPrefixes for any delimiter. Fixes issue where directories like 'data/file.vhd' were being returned as objects instead of prefixes in ListObjects responses. * fix: complete the directory listing fix for all delimiters Address reviewer feedback: - Changed doListFilerEntries line 549 from 'delimiter != "/"' to 'delimiter == ""' This ensures directories are yielded to the callback for ANY delimiter, not just "/" - Parameterized test to verify fix works with multiple delimiters (/, _, :) The previous fix only addressed line 260 but line 549 was still causing recursion for non-"/" delimiters, preventing directories from being added to CommonPrefixes. * docs: update test comment to reflect multiple delimiters Address reviewer feedback - clarify that the test verifies behavior for any non-empty delimiter, not just '/'. * docs: clarify test comment with delimiter examples Add specific examples of delimiters ('/', '_', ':') to make it clear that the test verifies behavior with multiple delimiter types. * fix: revert line 549 to original logic, only line 260 needed changing The fix for directories being listed as objects only required changing line 260 from 'delimiter == "/"' to 'delimiter != ""'. Line 549 should remain as 'delimiter != "/"' to allow recursion for delimiters that don't exist in paths (e.g., delimiter=z for paths like b/a/c). This is correct S3 behavior. Updated test to only verify delimiter="/" since other delimiters should recurse into directories to find actual files. * docs: clarify test scope in directory listing test	7 days ago
Chris Lu	0f786cf0d2	Fix S3 list objects marker adjustment for delimiters (#7938)	1 week ago
Chris Lu	fca0a38435	Update s3api_object_handlers.go	1 week ago
Chris Lu	e3db95e0c1	Fix: Route unauthenticated specific STS requests to STS handler correctly (#7920) * Fix STS Access Denied for AssumeRoleWithWebIdentity (Issue #7917) * Fix logging regression: ensure IAM status is logged even if STS is enabled * Address PR feedback: fix duplicate log, clarify comments, add comprehensive routing tests * Add edge case test: authenticated STS action routes to IAM (auth precedence)	1 week ago
Chris Lu	b034cf188e	Fix: trim prefix slash in ListObjectVersionsHandler (#7919) * Fix: trim prefix slash in ListObjectVersionsHandler * Add test for ListObjectVersions prefix handling Test validates that prefix normalization works correctly with and without leading slashes, ensuring the fix for /Veeam/Archive/ style prefixes. * Simplify prefix test to validate normalization logic The test now validates that the prefix normalization (TrimPrefix) works correctly and that normalized prefixes match paths as expected. This is a focused unit test that validates the core fix without requiring complex mocking of the filer client. * Enhance prefix test with full matchesPrefixFilter logic Added test cases for directory traversal including: - Directory matching with trailing slash - canDescend logic for recursive directory search - Full simulation of matchesPrefixFilter behavior This provides more comprehensive coverage of the prefix normalization fix and ensures it works correctly for both files and directories.	1 week ago
Chris Lu	7a18c3a16f	Fix critical authentication bypass vulnerability (#7912) (#7915) * Fix critical authentication bypass vulnerability (#7912) The isRequestPostPolicySignatureV4() function was incorrectly returning true for ANY POST request with multipart/form-data content type, causing all such requests to bypass authentication in authRequest(). This allowed unauthenticated access to S3 API endpoints, as reported in issue #7912 where any credentials (or no credentials) were accepted. The fix removes isRequestPostPolicySignatureV4() entirely, preventing authTypePostPolicy from ever being set. PostPolicy signature verification is still properly handled in PostPolicyBucketHandler via doesPolicySignatureMatch(). Fixes #7912 * add AuthPostPolicy * refactor * Optimizing Auth Credentials * Update auth_credentials.go * Update auth_credentials.go	1 week ago
Chris Lu	808205e38f	s3: implement Bucket Owner Enforced for object ownership (#7913) * s3: implement Bucket Owner Enforced for object ownership Objects uploaded by service accounts (or any user) are now owned by the bucket owner when the bucket has BucketOwnerEnforced ownership policy (the modern AWS default since April 2023). This provides a more intuitive ownership model where users expect objects created by their service accounts to be owned by themselves. - Modified setObjectOwnerFromRequest to check bucket ObjectOwnership - When BucketOwnerEnforced: use bucket owner's account ID - When ObjectWriter: use uploader's account ID (backward compatible) * s3: add nil check and fix ownership logic hole - Add nil check for bucketRegistry before calling GetBucketMetadata - Fix logic hole where objects could be created without owner when BucketOwnerEnforced is set but bucket owner is nil - Refactor to ensure objects always have an owner by falling back to uploader when bucket owner is unavailable - Improve logging to distinguish between different fallback scenarios Addresses code review feedback from Gemini on PR #7913 * s3: add comprehensive tests for object ownership logic Add unit tests for setObjectOwnerFromRequest covering: - BucketOwnerEnforced: uses bucket owner - ObjectWriter: uses uploader - BucketOwnerPreferred: uses uploader - Nil owner fallback scenarios - Bucket metadata errors - Nil bucketRegistry - Empty account ID handling All 8 test cases pass, verifying correct ownership assignment in all scenarios including edge cases.	2 weeks ago
Chris Lu	ae9a943ef6	IAM: Add Service Account Support (#7744) (#7901) * iam: add ServiceAccount protobuf schema Add ServiceAccount message type to iam.proto with support for: - Unique ID and parent user linkage - Optional expiration timestamp - Separate credentials (access key/secret) - Action restrictions (subset of parent) - Enable/disable status This is the first step toward implementing issue #7744 (IAM Service Account Support). * iam: add service account response types Add IAM API response types for service account operations: - ServiceAccountInfo struct for marshaling account details - CreateServiceAccountResponse - DeleteServiceAccountResponse - ListServiceAccountsResponse - GetServiceAccountResponse - UpdateServiceAccountResponse Also add type aliases in iamapi package for backwards compatibility. Part of issue #7744 (IAM Service Account Support). * iam: implement service account API handlers Add CRUD operations for service accounts: - CreateServiceAccount: Creates service account with ABIA key prefix - DeleteServiceAccount: Removes service account and parent linkage - ListServiceAccounts: Lists all or filtered by parent user - GetServiceAccount: Retrieves service account details - UpdateServiceAccount: Modifies status, description, expiration Service accounts inherit parent user's actions by default and support optional expiration timestamps. Part of issue #7744 (IAM Service Account Support). * sts: add AssumeRoleWithWebIdentity HTTP endpoint Add STS API HTTP endpoint for AWS SDK compatibility: - Create s3api_sts.go with HTTP handlers matching AWS STS spec - Support AssumeRoleWithWebIdentity action with JWT token - Return XML response with temporary credentials (AccessKeyId, SecretAccessKey, SessionToken) matching AWS format - Register STS route at POST /?Action=AssumeRoleWithWebIdentity This enables AWS SDKs (boto3, AWS CLI, etc.) to obtain temporary S3 credentials using OIDC/JWT tokens. Part of issue #7744 (IAM Service Account Support). * test: add service account and STS integration tests Add integration tests for new IAM features: s3_service_account_test.go: - TestServiceAccountLifecycle: Create, Get, List, Update, Delete - TestServiceAccountValidation: Error handling for missing params s3_sts_test.go: - TestAssumeRoleWithWebIdentityValidation: Parameter validation - TestAssumeRoleWithWebIdentityWithMockJWT: JWT token handling Tests skip gracefully when SeaweedFS is not running or when IAM features are not configured. Part of issue #7744 (IAM Service Account Support). * iam: address code review comments - Add constants for service account ID and key lengths - Use strconv.ParseInt instead of fmt.Sscanf for better error handling - Allow clearing descriptions by checking key existence in url.Values - Replace magic numbers (12, 20, 40) with named constants Addresses review comments from gemini-code-assist[bot] * test: add proper error handling in service account tests Use require.NoError(t, err) for io.ReadAll and xml.Unmarshal to prevent silent failures and ensure test reliability. Addresses review comment from gemini-code-assist[bot] * test: add proper error handling in STS tests Use require.NoError(t, err) for io.ReadAll and xml.Unmarshal to prevent silent failures and ensure test reliability. Repeated this fix throughout the file. Addresses review comment from gemini-code-assist[bot] in PR #7901. * iam: address additional code review comments - Specific error code mapping for STS service errors - Distinguish between Sender and Receiver error types in STS responses - Add nil checks for credentials in List/GetServiceAccount - Validate expiration date is in the future - Improve integration test error messages (include response body) - Add credential verification step in service account tests Addresses remaining review comments from gemini-code-assist[bot] across multiple files. * iam: fix shared slice reference in service account creation Copy parent's actions to create an independent slice for the service account instead of sharing the underlying array. This prevents unexpected mutations when the parent's actions are modified later. Addresses review comment from coderabbitai[bot] in PR #7901. * iam: remove duplicate unused constant Removed redundant iamServiceAccountKeyPrefix as ServiceAccountKeyPrefix is already defined and used. Addresses remaining cleanup task. * sts: document limitation of string-based error mapping Added TODO comment explaining that the current string-based error mapping approach is fragile and should be replaced with typed errors from the STS service in a future refactoring. This addresses the architectural concern raised in code review while deferring the actual implementation to a separate PR to avoid scope creep in the current service account feature addition. * iam: fix remaining review issues - Add future-date validation for expiration in UpdateServiceAccount - Reorder tests so credential verification happens before deletion - Fix compilation error by using correct JWT generation methods Addresses final review comments from coderabbitai[bot]. * iam: fix service account access key length The access key IDs were incorrectly generated with 24 characters instead of the AWS-standard 20 characters. This was caused by generating 20 random characters and then prepending the 4-character ABIA prefix. Fixed by subtracting the prefix length from AccessKeyLength, so the final key is: ABIA (4 chars) + random (16 chars) = 20 chars total. This ensures compatibility with S3 clients that validate key length. * test: add comprehensive service account security tests Added comprehensive integration tests for service account functionality: - TestServiceAccountS3Access: Verify SA credentials work for S3 operations - TestServiceAccountExpiration: Test expiration date validation and enforcement - TestServiceAccountInheritedPermissions: Verify parent-child relationship - TestServiceAccountAccessKeyFormat: Validate AWS-compatible key format (ABIA prefix, 20 char length) These tests ensure SeaweedFS service accounts are compatible with AWS conventions and provide robust security coverage. * iam: remove unused UserAccessKeyPrefix constant Code cleanup to remove unused constants. * iam: remove unused iamCommonResponse type alias Code cleanup to remove unused type aliases. * iam: restore and use UserAccessKeyPrefix constant Restored UserAccessKeyPrefix constant and updated s3api tests to use it instead of hardcoded strings for better maintainability and consistency. * test: improve error handling in service account security tests Added explicit error checking for io.ReadAll and xml.Unmarshal in TestServiceAccountExpiration to ensure failures are reported correctly and cleanup is performed only when appropriate. Also added logging for failed responses. * test: use t.Cleanup for reliable resource cleanup Replaced defer with t.Cleanup to ensure service account cleanup runs even when require.NoError fails. Also switched from manual error checking to require.NoError for more idiomatic testify usage. * iam: add CreatedBy field and optimize identity lookups - Added createdBy parameter to CreateServiceAccount to track who created each service account - Extract creator identity from request context using GetIdentityNameFromContext - Populate created_by field in ServiceAccount protobuf - Added findIdentityByName helper function to optimize identity lookups - Replaced nested loops with O(n) helper function calls in CreateServiceAccount and DeleteServiceAccount This addresses code review feedback for better auditing and performance. * iam: prevent user deletion when service accounts exist Following AWS IAM behavior, prevent deletion of users that have active service accounts. This ensures explicit cleanup and prevents orphaned service account resources with invalid ParentUser references. Users must delete all associated service accounts before deleting the parent user, providing safer resource management. * sts: enhance TODO with typed error implementation guidance Updated TODO comment with detailed implementation approach for replacing string-based error matching with typed errors using errors.Is(). This provides a clear roadmap for a follow-up PR to improve error handling robustness and maintainability. * iam: add operational limits for service account creation Added AWS IAM-compatible safeguards to prevent resource exhaustion: - Maximum 100 service accounts per user (LimitExceededException) - Maximum 1000 character description length (InvalidInputException) These limits prevent accidental or malicious resource exhaustion while not impacting legitimate use cases. * iam: add missing operational limit constants Added MaxServiceAccountsPerUser and MaxDescriptionLength constants that were referenced in the previous commit but not defined. * iam: enforce service account expiration during authentication CRITICAL SECURITY FIX: Expired service account credentials were not being rejected during authentication, allowing continued access after expiration. Changes: - Added Expiration field to Credential struct - Populate expiration when loading service accounts from configuration - Check expiration in all authentication paths (V2 and V4 signatures) - Return ErrExpiredToken for expired credentials This ensures expired service accounts are properly rejected at authentication time, matching AWS IAM behavior and preventing unauthorized access. * iam: fix error code for expired service account credentials Use ErrAccessDenied instead of non-existent ErrExpiredToken for expired service account credentials. This provides appropriate access denial for expired credentials while maintaining AWS-compatible error responses. * iam: fix remaining ErrExpiredToken references Replace all remaining instances of non-existent ErrExpiredToken with ErrAccessDenied for expired service account credentials. * iam: apply AWS-standard key format to user access keys Updated CreateAccessKey to generate AWS-standard 20-character access keys with AKIA prefix for regular users, matching the format used for service accounts. This ensures consistency across all access key types and full AWS compatibility. - Access keys: AKIA + 16 random chars = 20 total (was 21 chars, no prefix) - Secret keys: 40 random chars (was 42, now matches AWS standard) - Uses AccessKeyLength and UserAccessKeyPrefix constants * sts: replace fragile string-based error matching with typed errors Implemented robust error handling using typed errors and errors.Is() instead of fragile strings.Contains() matching. This decouples the HTTP layer from service implementation details and prevents errors from being miscategorized if error messages change. Changes: - Added typed error variables to weed/iam/sts/constants.go: * ErrTypedTokenExpired * ErrTypedInvalidToken * ErrTypedInvalidIssuer * ErrTypedInvalidAudience * ErrTypedMissingClaims - Updated STS service to wrap provider authentication errors with typed errors - Replaced strings.Contains() with errors.Is() in HTTP layer for error checking - Removed TODO comment as the improvement is now implemented This makes error handling more maintainable and reliable. * sts: eliminate all string-based error matching with provider-level typed errors Completed the typed error implementation by adding provider-level typed errors and updating provider implementations to return them. This eliminates ALL fragile string matching throughout the entire error handling stack. Changes: - Added typed error definitions to weed/iam/providers/errors.go: * ErrProviderTokenExpired * ErrProviderInvalidToken * ErrProviderInvalidIssuer * ErrProviderInvalidAudience * ErrProviderMissingClaims - Updated OIDC provider to wrap JWT validation errors with typed provider errors - Replaced strings.Contains() with errors.Is() in STS service for error mapping - Complete error chain: Provider -> STS -> HTTP layer, all using errors.Is() This provides: - Reliable error classification independent of error message content - Type-safe error checking throughout the stack - No order-dependent string matching - Maintainable error handling that won't break with message changes * oidc: use jwt.ErrTokenExpired instead of string matching Replaced the last remaining string-based error check with the JWT library's exported typed error. This makes the error detection independent of error message content and more robust against library updates. Changed from: strings.Contains(errMsg, "expired") To: errors.Is(err, jwt.ErrTokenExpired) This completes the elimination of ALL string-based error matching throughout the entire authentication stack. * iam: add description length validation to UpdateServiceAccount Fixed inconsistency where UpdateServiceAccount didn't validate description length against MaxDescriptionLength, allowing operational limits to be bypassed during updates. Now validates that updated descriptions don't exceed 1000 characters, matching the validation in CreateServiceAccount. * iam: refactor expiration check into helper method Extracted duplicated credential expiration check logic into a helper method to reduce code duplication and improve maintainability. Added Credential.isCredentialExpired() method and replaced 5 instances of inline expiration checks across auth_signature_v2.go and auth_signature_v4.go. * iam: address critical Copilot security and consistency feedback Fixed three critical issues identified by Copilot code review: 1. SECURITY: Prevent loading disabled service account credentials - Added check to skip disabled service accounts during credential loading - Disabled accounts can no longer authenticate 2. Add DurationSeconds validation for STS AssumeRoleWithWebIdentity - Enforce AWS-compatible range: 900-43200 seconds (15 min - 12 hours) - Returns proper error for out-of-range values 3. Fix expiration update consistency in UpdateServiceAccount - Added key existence check like Description field - Allows explicit clearing of expiration by setting to empty string - Distinguishes between "not updating" and "clearing expiration" * sts: remove unused durationSecondsStr variable Fixed build error from unused variable after refactoring duration parsing. * iam: address remaining Copilot feedback and remove dead code Completed remaining Copilot code review items: 1. Remove unused getPermission() method (dead code) - Method was defined but never called anywhere 2. Improve slice modification safety in DeleteServiceAccount - Replaced append-with-slice-operations with filter pattern - Avoids potential issues from mutating slice during iteration 3. Fix route registration order - Moved STS route registration BEFORE IAM route - Prevents IAM route from intercepting STS requests - More specific route (with query parameter) now registered first * iam: improve expiration validation and test cleanup robustness Addressed additional Copilot feedback: 1. Make expiration validation more explicit - Added explicit check for negative values - Added comment clarifying that 0 is allowed to clear expiration - Improves code readability and intent 2. Fix test cleanup order in s3_service_account_test.go - Track created service accounts in a slice - Delete all service accounts before deleting parent user - Prevents DeleteConflictException during cleanup - More robust cleanup even if test fails mid-execution Note: s3_service_account_security_test.go already had correct cleanup order due to LIFO defer execution. * test: remove redundant variable assignments Removed duplicate assignments of createdSAId, createdAccessKeyId, and createdSecretAccessKey on lines 148-150 that were already assigned on lines 132-134.	2 weeks ago
Chris Lu	2b529e310d	s3: Add SOSAPI support for Veeam integration (#7899) * s3api: Add SOSAPI core implementation and tests Implement Smart Object Storage API (SOSAPI) support for Veeam integration. - Add s3api_sosapi.go with XML structures and handlers for system.xml and capacity.xml - Implement virtual object detection and dynamic XML generation - Add capacity retrieval via gRPC (to be optimized in follow-up) - Include comprehensive unit tests covering detection, XML generation, and edge cases This enables Veeam Backup & Replication to discover SeaweedFS capabilities and capacity. * s3api: Integrate SOSAPI handlers into GetObject and HeadObject Add early interception for SOSAPI virtual objects in GetObjectHandler and HeadObjectHandler. - Check for SOSAPI objects (.system-*/system.xml, .system-*/capacity.xml) before normal processing - Delegate to handleSOSAPIGetObject and handleSOSAPIHeadObject when detected - Ensures virtual objects are served without hitting storage layer * s3api: Allow anonymous access to SOSAPI virtual objects Enable discovery of SOSAPI capabilities without requiring credentials. - Modify AuthWithPublicRead to bypass auth for SOSAPI objects if bucket exists - Supports Veeam's initial discovery phase before full IAM setup - Validates bucket existence to prevent information disclosure * s3api: Fix SOSAPI capacity retrieval to use proper master connection Fix gRPC error by connecting directly to master servers instead of through filer. - Use pb.WithOneOfGrpcMasterClients with s3a.option.Masters - Matches pattern used in bucket_size_metrics.go - Resolves "unknown service master_pb.Seaweed" error - Gracefully handles missing master configuration * Merge origin/master and implement robust SOSAPI capacity logic - Resolved merge conflict in s3api_sosapi.go - Replaced global Statistics RPC with VolumeList (topology) for accurate bucket-specific 'Used' calculation - Added bucket quota support (report quota as Capacity if set) - Implemented cluster-wide capacity calculation from topology when no quota - Added unit tests for topology capacity and usage calculations * s3api: Remove anonymous access to SOSAPI virtual objects Reverts the implicit public access for system.xml and capacity.xml. Requests to these objects now require standard S3 authentication, unless the bucket has a public-read policy. * s3api: Refactor SOSAPI handlers to use http.ServeContent - Consolidate handleSOSAPIGetObject and handleSOSAPIHeadObject into serveSOSAPI - Use http.ServeContent for standard Range, HEAD, and ETag handling - Remove manual range request handler and reduce code duplication * s3api: Unify SOSAPI request handling - Replaced handleSOSAPIGetObject and handleSOSAPIHeadObject with single HandleSOSAPI function - Updated call sites in s3api_object_handlers.go - Simplifies logic and ensures consistent handling for both GET and HEAD requests via http.ServeContent * s3api: Restore distinct SOSAPI GET/HEAD handlers - Reverted unified handler to enforce distinct behavior for GET and HEAD - GET: Supports Range requests via http.ServeContent - HEAD: Explicitly ignores Range requests (matches MinIO behavior) and writes headers only * s3api: Refactor SOSAPI handlers to eliminate duplication - Extracted shared content generation logic into generateSOSAPIContent helper - handleSOSAPIGetObject: Uses http.ServeContent (supports Range requests) - handleSOSAPIHeadObject: Manually sets headers (no Range, no body) - Maintains distinct behavior while following DRY principle * s3api: Remove low-value SOSAPI tests Removed tests that validate standard library behavior or trivial constant checks: - TestSOSAPIConstants (string prefix/suffix checks) - TestSystemInfoXMLRootElement (redundant with TestGenerateSystemXML) - TestSOSAPIXMLContentType (tests httptest, not our code) - TestHTTPTimeFormat (tests standard library) - TestCapacityInfoXMLStruct (tests Go's XML marshaling) Kept tests that validate actual business logic and edge cases. * s3api: Use consistent S3-compliant error responses in SOSAPI Replaced http.Error() with s3err.WriteErrorResponse() for internal errors to ensure all SOSAPI errors return S3-compliant XML instead of plain text. * s3api: Return error when no masters configured for SOSAPI capacity Changed getCapacityInfo to return an error instead of silently returning zero capacity when no master servers are configured. This helps surface configuration issues rather than masking them. * s3api: Use collection name with FilerGroup prefix for SOSAPI capacity Fixed collectBucketUsageFromTopology to use s3a.getCollectionName(bucket) instead of raw bucket name. This ensures collection comparisons match actual volume collection names when FilerGroup prefix is configured. * s3api: Apply PR review feedback for SOSAPI - Renamed `bucket` parameter to `collectionName` in collectBucketUsageFromTopology for clarity - Changed error checks from `==` to `errors.Is()` for better wrapped error handling - Added `errors` import * s3api: Avoid variable shadowing in SOSAPI capacity retrieval Refactored `getCapacityInfo` to use distinct variable names for errors to improve code clarity and avoid unintentional shadowing of the return parameter.	2 weeks ago
Chris Lu	e8baeb3616	s3api: Allow anonymous access to SOSAPI virtual objects Enable discovery of SOSAPI capabilities without requiring credentials. - Modify AuthWithPublicRead to bypass auth for SOSAPI objects if bucket exists - Supports Veeam's initial discovery phase before full IAM setup - Validates bucket existence to prevent information disclosure	2 weeks ago
Chris Lu	a757ef77b1	s3api: Integrate SOSAPI handlers into GetObject and HeadObject Add early interception for SOSAPI virtual objects in GetObjectHandler and HeadObjectHandler. - Check for SOSAPI objects (.system-*/system.xml, .system-*/capacity.xml) before normal processing - Delegate to handleSOSAPIGetObject and handleSOSAPIHeadObject when detected - Ensures virtual objects are served without hitting storage layer	2 weeks ago
Chris Lu	fba67ce0f0	s3api: Add SOSAPI core implementation and tests Implement Smart Object Storage API (SOSAPI) support for Veeam integration. - Add s3api_sosapi.go with XML structures and handlers for system.xml and capacity.xml - Implement virtual object detection and dynamic XML generation - Add capacity retrieval via gRPC (to be optimized in follow-up) - Include comprehensive unit tests covering detection, XML generation, and edge cases This enables Veeam Backup & Replication to discover SeaweedFS capabilities and capacity.	2 weeks ago
Chris Lu	ef20873c31	S3: Fix Content-Encoding header not preserved (#7894) (#7895) * S3: Fix Content-Encoding header not preserved (#7894) The Content-Encoding header was not being returned in S3 GET/HEAD responses because it wasn't being stored in metadata during PUT operations. Root cause: The putToFiler function only stored a hardcoded list of standard HTTP headers (Cache-Control, Expires, Content-Disposition) but was missing Content-Encoding and Content-Language. Fix: Added Content-Encoding and Content-Language to the list of standard headers that are stored in entry.Extended during PUT operations. This matches the behavior of ParseS3Metadata (used for multipart uploads) and ensures consistency across all S3 operations. Fixes #7894 * Update s3api_object_handlers_put.go	2 weeks ago
Chris Lu	8d6bcddf60	Add S3 volume encryption support with -s3.encryptVolumeData flag (#7890) * Add S3 volume encryption support with -s3.encryptVolumeData flag This change adds volume-level encryption support for S3 uploads, similar to the existing -filer.encryptVolumeData option. Each chunk is encrypted with its own auto-generated CipherKey when the flag is enabled. Changes: - Add -s3.encryptVolumeData flag to weed s3, weed server, and weed mini - Wire Cipher option through S3ApiServer and ChunkedUploadOption - Add integration tests for multi-chunk range reads with encryption - Tests verify encryption works across chunk boundaries Usage: weed s3 -encryptVolumeData weed server -s3 -s3.encryptVolumeData weed mini -s3.encryptVolumeData Integration tests: go test -v -tags=integration -timeout 5m ./test/s3/sse/... * Add GitHub Actions CI for S3 volume encryption tests - Add test-volume-encryption target to Makefile that starts server with -s3.encryptVolumeData - Add s3-volume-encryption job to GitHub Actions workflow - Tests run with integration build tag and 10m timeout - Server logs uploaded on failure for debugging * Fix S3 client credentials to use environment variables The test was using hardcoded credentials "any"/"any" but the Makefile sets AWS_ACCESS_KEY_ID/AWS_SECRET_ACCESS_KEY to "some_access_key1"/ "some_secret_key1". Updated getS3Client() to read from environment variables with fallback to "any"/"any" for manual testing. * Change bucket creation errors from skip to fatal Tests should fail, not skip, when bucket creation fails. This ensures that credential mismatches and other configuration issues are caught rather than silently skipped. * Make copy and multipart test jobs fail instead of succeed Changed exit 0 to exit 1 for s3-sse-copy-operations and s3-sse-multipart jobs. These jobs document known limitations but should fail to ensure the issues are tracked and addressed, not silently ignored. * Hardcode S3 credentials to match Makefile Changed from environment variables to hardcoded credentials "some_access_key1"/"some_secret_key1" to match the Makefile configuration. This ensures tests work reliably. * fix Double Encryption * fix Chunk Size Mismatch * Added IsCompressed * is gzipped * fix copying * only perform HEAD request when len(cipherKey) > 0 * Revert "Make copy and multipart test jobs fail instead of succeed" This reverts commit bc34a7eb3c. * fix security vulnerability * fix security * Update s3api_object_handlers_copy.go * Update s3api_object_handlers_copy.go * jwt to get content length	2 weeks ago
Chris Lu	82dac3df03	s3: do not persist multi part "Response-Content-Disposition" in request header (#7887) * fix: support standard HTTP headers in S3 multipart upload * fix(s3api): validate standard HTTP headers correctly and avoid persisting Response-Content-Disposition --------- Co-authored-by: steve.wei <coderushing@gmail.com>	2 weeks ago
steve.wei	f07ba2c5aa	fix: support standard HTTP headers in S3 multipart upload (#7884) Co-authored-by: Chris Lu <chris.lu@gmail.com>	2 weeks ago
Chris Lu	2f6aa98221	Refactor: Replace removeDuplicateSlashes with NormalizeObjectKey (#7873) * Replace removeDuplicateSlashes with NormalizeObjectKey Use s3_constants.NormalizeObjectKey instead of removeDuplicateSlashes in most places for consistency. NormalizeObjectKey handles both duplicate slash removal and ensures the path starts with '/', providing more complete normalization. * Fix double slash issues after NormalizeObjectKey After using NormalizeObjectKey, object keys have a leading '/'. This commit ensures: - getVersionedObjectDir strips leading slash before concatenation - getEntry calls receive names without leading slash - String concatenation with '/' doesn't create '//' paths This prevents path construction errors like: /buckets/bucket//object (wrong) /buckets/bucket/object (correct) * ensure object key leading "/" * fix compilation * fix: Strip leading slash from object keys in S3 API responses After introducing NormalizeObjectKey, all internal object keys have a leading slash. However, S3 API responses must return keys without leading slashes to match AWS S3 behavior. Fixed in three functions: - addVersion: Strip slash for version list entries - processRegularFile: Strip slash for regular file entries - processExplicitDirectory: Strip slash for directory entries This ensures ListObjectVersions and similar APIs return keys like 'bar' instead of '/bar', matching S3 API specifications. * fix: Normalize keyMarker for consistent pagination comparison The S3 API provides keyMarker without a leading slash (e.g., 'object-001'), but after introducing NormalizeObjectKey, all internal object keys have leading slashes (e.g., '/object-001'). When comparing keyMarker < normalizedObjectKey in shouldSkipObjectForMarker, the ASCII value of '/' (47) is less than 'o' (111), causing all objects to be incorrectly skipped during pagination. This resulted in page 2 and beyond returning 0 results. Fix: Normalize the keyMarker when creating versionCollector so comparisons work correctly with normalized object keys. Fixes pagination tests: - TestVersioningPaginationOver1000Versions - TestVersioningPaginationMultipleObjectsManyVersions * refactor: Change NormalizeObjectKey to return keys without leading slash BREAKING STRATEGY CHANGE: Previously, NormalizeObjectKey added a leading slash to all object keys, which required stripping it when returning keys to S3 API clients and caused complexity in marker normalization for pagination. NEW STRATEGY: - NormalizeObjectKey now returns keys WITHOUT leading slash (e.g., 'foo/bar' not '/foo/bar') - This matches the S3 API format directly - All path concatenations now explicitly add '/' between bucket and object - No need to strip slashes in responses or normalize markers Changes: 1. Modified NormalizeObjectKey to strip leading slash instead of adding it 2. Fixed all path concatenations to use: - BucketsPath + '/' + bucket + '/' + object instead of: - BucketsPath + '/' + bucket + object 3. Reverted response key stripping in: - addVersion() - processRegularFile() - processExplicitDirectory() 4. Reverted keyMarker normalization in findVersionsRecursively() 5. Updated matchesPrefixFilter() to work with keys without leading slash 6. Fixed paths in handlers: - s3api_object_handlers.go (GetObject, HeadObject, cacheRemoteObjectForStreaming) - s3api_object_handlers_postpolicy.go - s3api_object_handlers_tagging.go - s3api_object_handlers_acl.go - s3api_version_id.go (getVersionedObjectDir, getVersionIdFormat) - s3api_object_versioning.go (getObjectVersionList, updateLatestVersionAfterDeletion) All versioning tests pass including pagination stress tests. * adjust format * Update post policy tests to match new NormalizeObjectKey behavior - Update TestPostPolicyKeyNormalization to expect keys without leading slashes - Update TestNormalizeObjectKey to expect keys without leading slashes - Update TestPostPolicyFilenameSubstitution to expect keys without leading slashes - Update path construction in tests to use new pattern: BucketsPath + '/' + bucket + '/' + object * Fix ListObjectVersions prefix filtering Remove leading slash addition to prefix parameter to allow correct filtering of .versions directories when listing object versions with a specific prefix. The prefix parameter should match entry paths relative to bucket root. Adding a leading slash was breaking the prefix filter for paginated requests. Fixes pagination issue where second page returned 0 versions instead of continuing with remaining versions. * no leading slash * Fix urlEscapeObject to add leading slash for filer paths NormalizeObjectKey now returns keys without leading slashes to match S3 API format. However, urlEscapeObject is used for filer paths which require leading slashes. Add leading slash back after normalization to ensure filer paths are correct. Fixes TestS3ApiServer_toFilerPath test failures. * adjust tests * normalize * Fix: Normalize prefixes and markers in LIST operations using NormalizeObjectKey Ensure consistent key normalization across all S3 operations (GET, PUT, LIST). Previously, LIST operations were not applying the same normalization rules (handling backslashes, duplicate slashes, leading slashes) as GET/PUT operations. Changes: - Updated normalizePrefixMarker() to call NormalizeObjectKey for both prefix and marker - This ensures prefixes with leading slashes, backslashes, or duplicate slashes are handled consistently with how object keys are normalized - Fixes Parquet test failures where pads.write_dataset creates implicit directory structures that couldn't be discovered by subsequent LIST operations - Added TestPrefixNormalizationInList and TestListPrefixConsistency tests All existing LIST tests continue to pass with the normalization improvements. * Add debugging logging to LIST operations to track prefix normalization * Fix: Remove leading slash addition from GetPrefix to work with NormalizeObjectKey The NormalizeObjectKey function removes leading slashes to match S3 API format (e.g., 'foo/bar' not '/foo/bar'). However, GetPrefix was adding a leading slash back, which caused LIST operations to fail with incorrect path handling. Now GetPrefix only normalizes duplicate slashes without adding a leading slash, which allows NormalizeObjectKey changes to work correctly for S3 LIST operations. All Parquet integration tests now pass (20/20). * Fix: Handle object paths without leading slash in checkDirectoryObject NormalizeObjectKey() removes the leading slash to match S3 API format. However, checkDirectoryObject() was assuming the object path has a leading slash when processing directory markers (paths ending with '/'). Now we ensure the object has a leading slash before processing it for filer operations. Fixes implicit directory marker test (explicit_dir/) while keeping Parquet integration tests passing (20/20). All tests pass: - Implicit directory tests: 6/6 - Parquet integration tests: 20/20 * Fix: Handle explicit directory markers with trailing slashes Explicit directory markers created with put_object(Key='dir/', ...) are stored in the filer with the trailing slash as part of the name. The checkDirectoryObject() function now checks for both: 1. Explicit directories: lookup with trailing slash preserved (e.g., 'explicit_dir/') 2. Implicit directories: lookup without trailing slash (e.g., 'implicit_dir') This ensures both types of directory markers are properly recognized. All tests pass: - Implicit directory tests: 6/6 (including explicit directory marker test) - Parquet integration tests: 20/20 * Fix: Preserve trailing slash in NormalizeObjectKey NormalizeObjectKey now preserves trailing slashes when normalizing object keys. This is important for explicit directory markers like 'explicit_dir/' which rely on the trailing slash to be recognized as directory objects. The normalization process: 1. Notes if trailing slash was present 2. Removes duplicate slashes and converts backslashes 3. Removes leading slash for S3 API format 4. Restores trailing slash if it was in the original This ensures explicit directory markers created with put_object(Key='dir/', ...) are properly normalized and can be looked up by their exact name. All tests pass: - Implicit directory tests: 6/6 - Parquet integration tests: 20/20 * clean object * Fix: Don't restore trailing slash if result is empty When normalizing paths that are only slashes (e.g., '///', '/'), the function should return an empty string, not a single slash. The fix ensures we only restore the trailing slash if the result is non-empty. This fixes the 'just_slashes' test case: - Input: '///' - Expected: '' - Previous: '/' - Fixed: '' All tests now pass: - Unit tests: TestNormalizeObjectKey (13/13) - Implicit directory tests: 6/6 - Parquet integration tests: 20/20 * prefixEndsOnDelimiter * Update s3api_object_handlers_list.go * Update s3api_object_handlers_list.go * handle create directory	2 weeks ago
Chris Lu	014027f75a	Fix: Support object tagging in versioned buckets (Issue #7868) (#7871) * Fix: Support object tagging in versioned buckets (Issue #7868) This fix addresses the issue where setting tags on files in versioned buckets would fail with 'filer: no entry is found in filer store' error. Changes: - Updated GetObjectTaggingHandler to check versioning status and retrieve correct object versions - Updated PutObjectTaggingHandler to properly locate and update tags on versioned objects - Updated DeleteObjectTaggingHandler to delete tags from versioned objects - Added proper handling for both specific versions and latest versions - Added distinction between null versions (pre-versioning objects) and versioned objects The fix follows the same versioning-aware pattern already implemented in ACL handlers. Tests: - Added comprehensive test suite for tagging operations on versioned buckets - Tests cover PUT, GET, and DELETE tagging operations on specific versions and latest versions - Tests verify tag isolation between different versions of the same object * Fix: Ensure consistent directory path construction in tagging handlers Changed directory path construction to match the pattern used in ACL handlers: - Added missing '/' before object path when constructing .versions directory path - This ensures compatibility with the filer's expected path structure - Applied to both PutObjectTaggingHandler and DeleteObjectTaggingHandler * Revert: Remove redundant slash in path construction - object already has leading slash from NormalizeObjectKey * Fix: Remove redundant slashes in versioning path construction across handlers - getVersionedObjectDir: object already starts with '/', no need for extra '/' - ACL handlers: same pattern, fix both PutObjectAcl locations - Ensures consistent path construction with object parameter normalization * fix test compilation * Add: Comprehensive ACL tests for versioned and non-versioned buckets - Added s3_acl_versioning_test.go with 5 test cases covering: * GetObjectAcl on versioned buckets * GetObjectAcl on specific versions * PutObjectAcl on versioned buckets * PutObjectAcl on specific versions * Independent ACL management across versions These tests were missing and would have caught the path construction issues we just fixed in the ACL handler. Tests validate that ACL operations work correctly on both versioned and non-versioned objects. * Fix: Correct tagging versioning test file formatting * fix: Update AWS SDK endpoint config and improve cleanup to handle delete markers - Replace deprecated EndpointResolverWithOptions with BaseEndpoint in AWS SDK v2 client configuration - Update cleanupTestBucket to properly delete both object versions and delete markers - Apply changes to both ACL and tagging test files for consistency * Fix S3 multi-delete for versioned objects The bug was in getVersionedObjectDir() which was constructing paths without a slash between the bucket and object key: BEFORE (WRONG): /buckets/mybucket{key}.versions AFTER (FIXED): /buckets/mybucket/{key}/.versions This caused version deletions to claim success but not actually delete files, breaking S3 compatibility tests: - test_versioning_multi_object_delete - test_versioning_multi_object_delete_with_marker - test_versioning_concurrent_multi_object_delete - test_object_lock_multi_delete_object_with_retention Added comprehensive test that reproduces the issue and verifies the fix. * Remove emojis from test output	2 weeks ago
Chris Lu	26acebdef1	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)	2 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 weeks ago
Chris Lu	1261e93ef2	fix: comprehensive go vet error fixes and add CI enforcement (#7861) * fix: use keyed fields in struct literals - Replace unsafe reflect.StringHeader/SliceHeader with safe unsafe.String/Slice (weed/query/sqltypes/unsafe.go) - Add field names to Type_ScalarType struct literals (weed/mq/schema/schema_builder.go) - Add Duration field name to FlexibleDuration struct literals across test files - Add field names to bson.D struct literals (weed/filer/mongodb/mongodb_store_kv.go) Fixes go vet warnings about unkeyed struct literals. * fix: remove unreachable code - Remove unreachable return statements after infinite for loops - Remove unreachable code after if/else blocks where all paths return - Simplify recursive logic by removing unnecessary for loop (inode_to_path.go) - Fix Type_ScalarType literal to use enum value directly (schema_builder.go) - Call onCompletionFn on stream error (subscribe_session.go) Files fixed: - weed/query/sqltypes/unsafe.go - weed/mq/schema/schema_builder.go - weed/mq/client/sub_client/connect_to_sub_coordinator.go - weed/filer/redis3/ItemList.go - weed/mq/client/agent_client/subscribe_session.go - weed/mq/broker/broker_grpc_pub_balancer.go - weed/mount/inode_to_path.go - weed/util/skiplist/name_list.go * fix: avoid copying lock values in protobuf messages - Use proto.Merge() instead of direct assignment to avoid copying sync.Mutex in S3ApiConfiguration (iamapi_server.go) - Add explicit comments noting that channel-received values are already copies before taking addresses (volume_grpc_client_to_master.go) The protobuf messages contain sync.Mutex fields from the message state, which should not be copied. Using proto.Merge() properly merges messages without copying the embedded mutex. * fix: correct byte array size for uint32 bit shift operations The generateAccountId() function only needs 4 bytes to create a uint32 value. Changed from allocating 8 bytes to 4 bytes to match the actual usage. This fixes go vet warning about shifting 8-bit values (bytes) by more than 8 bits. * fix: ensure context cancellation on all error paths In broker_client_subscribe.go, ensure subscriberCancel() is called on all error return paths: - When stream creation fails - When partition assignment fails - When sending initialization message fails This prevents context leaks when an error occurs during subscriber creation. * fix: ensure subscriberCancel called for CreateFreshSubscriber stream.Send error Ensure subscriberCancel() is called when stream.Send fails in CreateFreshSubscriber. * ci: add go vet step to prevent future lint regressions - Add go vet step to GitHub Actions workflow - Filter known protobuf lock warnings (MessageState sync.Mutex) These are expected in generated protobuf code and are safe - Prevents accumulation of go vet errors in future PRs - Step runs before build to catch issues early * fix: resolve remaining syntax and logic errors in vet fixes - Fixed syntax errors in filer_sync.go caused by missing closing braces - Added missing closing brace for if block and function - Synchronized fixes to match previous commits on branch * fix: add missing return statements to daemon functions - Add 'return false' after infinite loops in filer_backup.go and filer_meta_backup.go - Satisfies declared bool return type signatures - Maintains consistency with other daemon functions (runMaster, runFilerSynchronize, runWorker) - While unreachable, explicitly declares the return satisfies function signature contract * fix: add nil check for onCompletionFn in SubscribeMessageRecord - Check if onCompletionFn is not nil before calling it - Prevents potential panic if nil function is passed - Matches pattern used in other callback functions * docs: clarify unreachable return statements in daemon functions - Add comments documenting that return statements satisfy function signature - Explains that these returns follow infinite loops and are unreachable - Improves code clarity for future maintainers	2 weeks ago
Chris Lu	289ec5e2f5	Fix SeaweedFS S3 bucket extended attributes handling (#7854) * refactor: Convert versioning to three-state string model matching AWS S3 - Change VersioningEnabled bool to VersioningStatus string in S3Bucket struct - Add GetVersioningStatus() function returning empty string (never enabled), 'Enabled', or 'Suspended' - Update StoreVersioningInExtended() to delete key instead of setting 'Suspended' - Ensures Admin UI and S3 API use consistent versioning state representation * fix: Add validation for bucket quota and Object Lock configuration - Prevent buckets with quota enabled but size=0 (validation check) - Fix Object Lock mode handling to only pass mode when setDefaultRetention is true - Ensures proper extended attribute storage for Object Lock configuration - Matches AWS S3 behavior for Object Lock setup * feat: Handle versioned objects in bucket details view - Recognize .versions directories as versioned objects in listBucketObjects() - Extract size and mtime from extended attribute metadata (ExtLatestVersionSizeKey, ExtLatestVersionMtimeKey) - Add length validation (8 bytes) before parsing extended attribute byte arrays - Update GetBucketDetails() and GetS3Buckets() to use new GetVersioningStatus() - Properly display versioned objects without .versions suffix in bucket details * ui: Update bucket management UI to show three-state versioning and Object Lock - Change versioning display from binary (Enabled/Disabled) to three-state (Not configured/Enabled/Suspended) - Update Object Lock display to show 'Not configured' instead of 'Disabled' - Fix bucket details modal to use bucket.versioning_status instead of bucket.versioning_enabled - Update displayBucketDetails() JavaScript to handle three versioning states * chore: Regenerate template code for bucket UI changes - Generated from updated s3_buckets.templ - Reflects three-state versioning and Object Lock UI improvements	3 weeks ago
Chris Lu	1d0361d936	Fix: Eliminate duplicate versioned objects in S3 list operations (#7850) * Fix: Eliminate duplicate versioned objects in S3 list operations - Move versioned directory processing outside of pagination loop to process only once - Add deduplication during .versions directory collection phase - Fix directory handling to not add directories to results in recursive mode - Directly add versioned entries to contents array instead of using callback Fixes issue where AWS S3 list operations returned duplicated versioned objects (e.g., 1000 duplicate entries from 4 unique objects). Now correctly returns only the unique logical entries without duplication. Verified with: aws s3api list-objects --endpoint-url http://localhost:8333 --bucket pm-itatiaiucu-01 Returns exactly 4 entries (ClientInfo.xml and Repository from 2 Veeam backup folders) * Refactor: Process .versions directories immediately when encountered Instead of collecting .versions directories and processing them after the pagination loop, process them immediately when encountered during traversal. Benefits: - Simpler code: removed versionedDirEntry struct and collection array - More efficient: no need to store and iterate through collected entries - Same O(V) complexity but with less memory overhead - Clearer logic: processing happens in one pass during traversal Since each .versions directory is only visited once during recursive traversal (we never traverse into them), there's no need for deferred processing or deduplication. * Add comprehensive tests for versioned objects list - TestListObjectsWithVersionedObjects: Tests listing with various delimiters - TestVersionedObjectsNoDuplication: Core test validating no 250x duplication - TestVersionedObjectsWithDeleteMarker: Tests delete marker filtering - TestVersionedObjectsMaxKeys: Tests pagination with versioned objects - TestVersionsDirectoryNotTraversed: Ensures .versions never traversed - Fix existing test signature to match updated doListFilerEntries * style: Fix formatting alignment in versioned objects tests * perf: Optimize path extraction using string indexing Replace multiple strings.Split/Join calls with efficient strings.Index slicing to extract bucket-relative path from directory string. Reduces unnecessary allocations and improves performance in versioned objects listing path construction. * refactor: Address code review feedback from Gemini Code Assist 1. Fix misleading comment about versioned directory processing location. Versioned directories are processed immediately in doListFilerEntries, not deferred to ListObjectsV1Handler. 2. Simplify path extraction logic using explicit bucket path construction instead of index-based string slicing for better readability and maintainability. 3. Add clarifying comment to test callback explaining why production logic is duplicated - necessary because listFilerEntries is not easily testable with filer client injection. * fmt * refactor: Address code review feedback from Copilot - Fix misleading comment about versioned directory processing location (note that processing happens within doListFilerEntries, not at top level) - Add maxKeys validation checks in all test callbacks for consistency - Add maxKeys check before calling eachEntryFn for versioned objects - Improve test documentation to clarify testing approach and avoid apologetic tone * refactor: Address code review feedback from Gemini Code Assist - Remove redundant maxKeys check before eachEntryFn call on line 541 (the loop already checks maxKeys <= 0 at line 502, ensuring quota exists) - Fix pagination pattern consistency in all test callbacks - TestVersionedObjectsNoDuplication: Use cursor.maxKeys <= 0 check and decrement - TestVersionedObjectsWithDeleteMarker: Use cursor.maxKeys <= 0 check and decrement - TestVersionsDirectoryNotTraversed: Use cursor.maxKeys <= 0 check and decrement - Ensures consistent pagination logic across all callbacks matching production behavior * refactor: Address code review suggestions for code quality - Adjust log verbosity from V(5) to V(4) for file additions to reduce noise while maintaining useful debug output during troubleshooting - Remove unused isRecursive parameter from doListFilerEntries function signature and all call sites (not used for any logic decisions) - Consolidate redundant comments about versioned directory handling to reduce documentation duplication These changes improve code maintainability and clarity. * fmt * refactor: Add pagination test and optimize stream processing - Add comprehensive test validation to TestVersionedObjectsMaxKeys that verifies truncation is correctly set when maxKeys is exhausted with more entries available, ensuring proper pagination state - Optimize stream processing in doListFilerEntries by using 'break' instead of 'continue' when quota is exhausted (cursor.maxKeys <= 0) This avoids receiving and discarding entries from the stream when we've already reached the requested limit, improving efficiency	3 weeks ago
Chris Lu	f67ba35f4a	Make lock_manager.RenewInterval configurable in LiveLock (#7830) * Make lock_manager.RenewInterval configurable in LiveLock - Add renewInterval field to LiveLock struct - Modify StartLongLivedLock to accept renewInterval parameter - Update all call sites to pass lock_manager.RenewInterval - Default to lock_manager.RenewInterval if zero is passed * S3 metrics: reduce collection interval to half of bucketSizeMetricsInterval Since S3 metrics collection is not critical, check more frequently but only collect when holding the distributed lock. This allows faster detection of any issues while avoiding overhead on non-leader instances. * Remove unused lock_manager import from bucket_size_metrics.go * Refactor: Make lockTTL the primary parameter, derive renewInterval from it Instead of configurable renew interval, lockTTL is now the input parameter. The renewal interval is automatically derived as lockTTL / 2, ensuring that locks are renewed well before expiration. Changes: - Replace renewInterval parameter with lockTTL - Rename LiveLock.renewInterval field to lockTTL - Calculate renewInterval as lockTTL / 2 inside the goroutine - Update all call sites to pass lockTTL values - Simplify sleep logic to use consistent renewInterval for both states This approach is more intuitive and guarantees safe renewal windows. * When locked, renew more aggressively to actively keep the lock When holding the lock, sleep for renewInterval/2 to renew more frequently. When seeking the lock, sleep for renewInterval to retry with normal frequency. This ensures we actively maintain lock ownership while being less aggressive when competing for the lock. * Simplify: use consistent renewInterval for all lock states Since renewInterval is already lockTTL / 2, there's no need to differentiate between locked and unlocked states. Both use the same interval for consistency. * Adjust sleep intervals for different lock states - Locked instances sleep for renewInterval (lockTTL/2) to renew the lock - Unlocked instances sleep for 5*renewInterval (2.5*lockTTL) to retry acquisition less frequently	3 weeks ago
Chris Lu	f63d9ad390	s3api: fix bucket-root listing w/ delimiter (#7827) * s3api: fix bucket-root listing w/ delimiter * test: improve mock robustness for bucket-root listing test - Make testListEntriesStream implement interface explicitly without embedding - Add prefix filtering logic to testFilerClient to simulate real filer behavior - Special-case prefix='/' to not filter for bucket root compatibility - Add required imports for metadata and strings packages This addresses review comments about test mock brittleness and accuracy. * test: add clarifying comment for mock filtering behavior Add detailed comment explaining which ListEntriesRequest parameters are implemented (Prefix) vs ignored (Limit, StartFromFileName, etc.) in the test mock to improve code documentation and future maintenance. * logging * less logs * less check if already locked	3 weeks ago
chrislu	4a764dbb37	fmt	3 weeks ago
Chris Lu	134fd6a1ae	fix: S3 remote storage cold-cache read fails with 'size reported but no content available' (#7817) fix: S3 remote storage cold-cache read fails with 'size reported but no content available' (#7815) When a remote-only entry's initial caching attempt times out or fails, streamFromVolumeServers() now detects this case and retries caching synchronously before streaming, similar to how the filer server handles remote-only entries. Changes: - Modified streamFromVolumeServers() to check entry.IsInRemoteOnly() before treating missing chunks as a data integrity error - Added doCacheRemoteObject() as the core caching function (calls filer gRPC) - Added buildRemoteObjectPath() helper to reduce code duplication - Refactored cacheRemoteObjectWithDedup() and cacheRemoteObjectForStreaming() to reuse the shared functions - Added integration tests for remote storage scenarios Fixes https://github.com/seaweedfs/seaweedfs/issues/7815	3 weeks ago
Chris Lu	bccef78082	fix: reduce N+1 queries in S3 versioned object list operations (#7814) * fix: achieve single-scan efficiency for S3 versioned object listing When listing objects in a versioning-enabled bucket, the original code triggered multiple getEntry calls per versioned object (up to 12 with retries), causing excessive 'find' operations visible in Grafana and leading to high memory usage. This fix achieves single-scan efficiency by caching list metadata (size, ETag, mtime, owner) directly in the .versions directory: 1. Add new Extended keys for caching list metadata in .versions dir 2. Update upload/copy/multipart paths to cache metadata when creating versions 3. Update getLatestVersionEntryFromDirectoryEntry to use cached metadata (zero getEntry calls when cache is available) 4. Update updateLatestVersionAfterDeletion to maintain cache consistency Performance improvement for N versioned objects: - Before: N×1 to N×12 find operations per list request - After: 0 extra find operations (all metadata from single scan) This matches the efficiency of normal (non-versioned) object listing. * Update s3api_object_versioning.go * s3api: fix ETag handling for versioned objects and simplify delete marker creation - Add Md5 attribute to synthetic logicalEntry for single-part uploads to ensure filer.ETag() returns correct value in ListObjects response - Simplify delete marker creation by initializing entry directly in mkFile callback - Add bytes and encoding/hex imports for ETag parsing * s3api: preserve default attributes in delete marker mkFile callback Only modify Mtime field instead of replacing the entire Attributes struct, preserving default values like Crtime, FileMode, Uid, and Gid that mkFile initializes. * s3api: fix ETag handling in newListEntry for multipart uploads Prioritize ExtETagKey from Extended attributes before falling back to filer.ETag(). This properly handles multipart upload ETags (format: md5-parts) for versioned objects, where the synthetic entry has cached ETag metadata but no chunks to calculate from. * s3api: reduce code duplication in delete marker creation Extract deleteMarkerExtended map to be reused in both mkFile callback and deleteMarkerEntry construction. * test: add multipart upload versioning tests for ETag verification Add tests to verify that multipart uploaded objects in versioned buckets have correct ETags when listed: - TestMultipartUploadVersioningListETag: Basic multipart upload with 2 parts - TestMultipartUploadMultipleVersionsListETag: Multiple multipart versions - TestMixedSingleAndMultipartVersionsListETag: Mix of single-part and multipart These tests cover a bug where synthetic entries for versioned objects didn't include proper ETag handling for multipart uploads. * test: add delete marker test for multipart uploaded versioned objects TestMultipartUploadDeleteMarkerListBehavior verifies: - Delete marker creation hides object from ListObjectsV2 - ListObjectVersions shows both version and delete marker - Version ETag (multipart format) is preserved after delete marker - Object can be accessed by version ID after delete marker - Removing delete marker restores object visibility * refactor: address code review feedback - test: use assert.ElementsMatch for ETag verification (more idiomatic) - s3api: optimize newListEntry ETag logic (check ExtETagKey first) - s3api: fix edge case in ETag parsing (>= 2 instead of > 2) * s3api: prevent stale cached metadata and preserve existing extended attrs - setCachedListMetadata: clear old cached keys before setting new values to prevent stale data when new version lacks certain fields (e.g., owner) - createDeleteMarker: merge extended attributes instead of overwriting to preserve any existing metadata on the entry * s3api: extract clearCachedVersionMetadata to reduce code duplication - clearCachedVersionMetadata: clears only metadata fields (size, mtime, etag, owner, deleteMarker) - clearCachedListMetadata: now reuses clearCachedVersionMetadata + clears ID/filename - setCachedListMetadata: uses clearCachedVersionMetadata (not clearCachedListMetadata because caller has already set ID/filename) * s3api: share timestamp between version entry and cache entry Capture versionMtime once before mkFile and reuse for both: - versionEntry.Attributes.Mtime in the mkFile callback - versionEntryForCache.Attributes.Mtime for list caching This keeps list vs. HEAD LastModified timestamps aligned. * s3api: remove amzAccountId variable shadowing in multipart upload Extract amzAccountId before mkFile callback and reuse in both places, similar to how versionMtime is handled. Avoids confusion from redeclaring the same variable.	3 weeks ago
Chris Lu	414cda4215	fix: S3 versioning memory leak in ListObjectVersions pagination (#7813) * fix: S3 versioning memory leak in ListObjectVersions pagination This commit fixes a memory leak issue in S3 versioning buckets where ListObjectVersions with pagination (key-marker set) would collect ALL versions in the bucket before filtering, causing O(N) memory usage. Root cause: - When keyMarker was set, maxCollect was set to 0 (unlimited) - This caused findVersionsRecursively to traverse the entire bucket - All versions were collected into memory, sorted, then filtered Fix: - Updated findVersionsRecursively to accept keyMarker and versionIdMarker - Skips objects/versions before the marker during recursion (not after) - Always respects maxCollect limit (never unlimited) - Memory usage is now O(maxKeys) instead of O(total versions) Refactoring: - Introduced versionCollector struct to encapsulate collection state - Extracted helper methods for cleaner, more testable code: - matchesPrefixFilter: prefix matching logic - shouldSkipObjectForMarker: keyMarker filtering - shouldSkipVersionForMarker: versionIdMarker filtering - processVersionsDirectory: .versions directory handling - processExplicitDirectory: S3 directory object handling - processRegularFile: pre-versioning file handling - collectVersions: main recursive collection loop - processDirectory: directory entry dispatch This reduces the high QPS on 'find' and 'prefixList' operations by skipping irrelevant objects during traversal. Fixes customer-reported memory leak with high find/prefixList QPS in Grafana for S3 versioning buckets. * s3: infer version ID format from ExtLatestVersionIdKey metadata Simplified version format detection: - Removed ExtVersionIdFormatKey - no longer needed - getVersionIdFormat() now infers format from ExtLatestVersionIdKey - Uses isNewFormatVersionId() to check if latest version uses inverted format This approach is simpler because: - ExtLatestVersionIdKey is already stored in .versions directory metadata - No need for separate format metadata field - Format is naturally determined by the existing version IDs	3 weeks ago
Chris Lu	99a2e79efc	fix: authenticate before parsing form in IAM API (#7803) fix: authenticate before parsing form in IAM API (#7802) The AuthIam middleware was calling ParseForm() before AuthSignatureOnly(), which consumed the request body before signature verification could hash it. For IAM requests (service != 's3'), the signature verification needs to hash the request body. When ParseForm() was called first, the body was already consumed, resulting in an empty body hash and SignatureDoesNotMatch error. The fix moves authentication before form parsing. The streamHashRequestBody function preserves the body after reading, so ParseForm() works correctly after authentication. Fixes #7802	3 weeks ago
Chris Lu	a77b145590	fix: ListBuckets returns empty for users with bucket-specific permissions (#7799) * fix: ListBuckets returns empty for users with bucket-specific permissions (#7796) The ListBucketsHandler was using sequential AND logic where ownership check happened before permission check. If a user had 'List:bucketname' permission but didn't own the bucket (different AmzIdentityId or missing owner metadata), the bucket was filtered out before the permission check could run. Changed to OR logic: a bucket is now visible if the user owns it OR has explicit permission to list it. This allows users with bucket-specific permissions like 'List:geoserver' to see buckets they have access to, even if they don't own them. Changes: - Modified ListBucketsHandler to check both ownership and permission, including bucket if either check passes - Renamed isBucketVisibleToIdentity to isBucketOwnedByIdentity for clarity - Added comprehensive tests in TestListBucketsIssue7796 Fixes #7796 * address review comments: optimize permission check and add integration test - Skip permission check if user is already the owner (performance optimization) - Add integration test that simulates the complete handler filtering logic to verify the combination of ownership OR permission check works correctly * add visibility assertions to each sub-test for self-contained verification Each sub-test now verifies the final outcome using isOwner || canList logic, making tests more robust and independently verifiable.	3 weeks ago
G-OD	504b258258	s3: fix remote object not caching (#7790) * s3: fix remote object not caching * s3: address review comments for remote object caching - Fix leading slash in object name by using strings.TrimPrefix - Return cached entry from CacheRemoteObjectToLocalCluster to get updated local chunk locations - Reuse existing helper function instead of inline gRPC call * s3/filer: add singleflight deduplication for remote object caching - Add singleflight.Group to FilerServer to deduplicate concurrent cache operations - Wrap CacheRemoteObjectToLocalCluster with singleflight to ensure only one caching operation runs per object when multiple clients request the same file - Add early-return check for already-cached objects - S3 API calls filer gRPC with timeout and graceful fallback on error - Clear negative bucket cache when bucket is created via weed shell - Add integration tests for remote cache with singleflight deduplication This benefits all clients (S3, HTTP, Hadoop) accessing remote-mounted objects by preventing redundant cache operations and improving concurrent access performance. Fixes: https://github.com/seaweedfs/seaweedfs/discussions/7599 * fix: data race in concurrent remote object caching - Add mutex to protect chunks slice from concurrent append - Add mutex to protect fetchAndWriteErr from concurrent read/write - Fix incorrect error check (was checking assignResult.Error instead of parseErr) - Rename inner variable to avoid shadowing fetchAndWriteErr * fix: address code review comments - Remove duplicate remote caching block in GetObjectHandler, keep only singleflight version - Add mutex protection for concurrent chunk slice and error access (data race fix) - Use lazy initialization for S3 client in tests to avoid panic during package load - Fix markdown linting: add language specifier to code fence, blank lines around tables - Add 'all' target to Makefile as alias for test-with-server - Remove unused 'util' import * style: remove emojis from test files * fix: add defensive checks and sort chunks by offset - Add nil check and type assertion check for singleflight result - Sort chunks by offset after concurrent fetching to maintain file order * fix: improve test diagnostics and path normalization - runWeedShell now returns error for better test diagnostics - Add all targets to .PHONY in Makefile (logs-primary, logs-remote, health) - Strip leading slash from normalizedObject to avoid double slashes in path --------- Co-authored-by: chrislu <chris.lu@gmail.com> Co-authored-by: Chris Lu <chrislusf@users.noreply.github.com>	3 weeks ago