fix(s3): return proper HTTP 400 for SSE-C validation errors (#1998)

2026-08-06 13:27:43 +00:00 · 2026-02-28 10:24:46 +08:00
parent af6c32efac
commit a24cbbb7a6
6 changed files with 395 additions and 85 deletions
@@ -0,0 +1,144 @@
+# S3 Compatibility Fix Workflow
+
+Step-by-step guide for identifying and fixing S3 API compatibility issues in RustFS.
+
+## Prerequisites
+
+- Rust toolchain installed (`cargo`, `rustc`)
+- Python 3 for running ceph s3-tests
+- Access to upstream remote (`git remote add upstream https://github.com/rustfs/rustfs.git`)
+- Familiarity with the [run.sh](./run.sh) test runner
+
+## Workflow
+
+### Step 1: Sync with upstream
+
+```bash
+git checkout main
+git fetch upstream
+git merge upstream/main
+```
+
+### Step 2: Select candidate tests
+
+Pick ~20 tests from `unimplemented_tests.txt` and write them to `selected_tests.txt`:
+
+```bash
+TESTEXPR=$(scripts/s3-tests/build_testexpr.sh selected_tests.txt) \
+  DEPLOY_MODE=build MAXFAIL=0 ./scripts/s3-tests/run.sh
+```
+
+Or run them directly:
+
+```bash
+TESTEXPR="test_foo or test_bar" DEPLOY_MODE=build MAXFAIL=0 ./scripts/s3-tests/run.sh
+```
+
+Review `artifacts/s3tests-single/pytest.log` for results.
+
+### Step 3: Triage failures
+
+From the test report, classify each failure:
+
+| Category | Action |
+|----------|--------|
+| Feature not implemented (e.g., returns `501 Not Implemented`) | Skip — do not attempt |
+| Incorrect HTTP status code or response body | Good candidate for a fix |
+| Teardown/cleanup issue (test passes but cleanup fails) | Good candidate — usually simple |
+| Complex multi-feature dependency | Defer to a later iteration |
+
+Pick the **simplest** failure to fix. "Simplest" means: fewest code paths affected, clearest expected behavior, closest to existing implementation.
+
+### Step 4: Deep analysis
+
+Before writing any code:
+
+1. **Read the test source** in `s3-tests/s3tests/functional/test_s3.py` to understand exactly what the test expects.
+2. **Read the S3 API specification** for the operation being tested.
+3. **Search the RustFS codebase** for the handler that serves this operation.
+4. **Compare with MinIO** (`github.com/minio/minio`) — find the equivalent handler and see how it handles the same edge case. This is critical because RustFS was ported from MinIO's Go code to Rust.
+5. **Identify the root cause** — is it a missing header, wrong status code, incorrect XML response, logic bug, etc.?
+6. **Document your findings** before making changes.
+
+### Step 5: Create a fix branch
+
+```bash
+git checkout main
+git checkout -b fix/s3-compat-<short-description>
+```
+
+One branch per fix. Never combine unrelated fixes in a single branch.
+
+### Step 6: Write tests first (when applicable)
+
+If the fix involves logic changes, add unit tests before modifying the production code:
+
+- Co-locate tests with their module (`#[cfg(test)] mod tests { ... }`)
+- Use descriptive test names: `test_<operation>_<scenario>_<expected_outcome>`
+- Cover both the happy path and the edge case being fixed
+
+### Step 7: Implement the fix
+
+Guidelines:
+
+- **Reuse existing abstractions** — do not duplicate logic that already exists in helper functions or shared modules.
+- **Follow s3s conventions** — RustFS's S3 layer is built on the `s3s` crate. Respect its traits, error types, and request/response patterns.
+- **Name things clearly** — variable names, function names, and types should be self-explanatory.
+- **Add comments only where intent is non-obvious** — explain *why*, not *what*.
+- **Do not introduce security holes** — validate inputs, check permissions, handle errors properly.
+- **Do not use `unwrap()` or `expect()` in production code** — use proper error handling with `Result` and `?`.
+
+### Step 8: Verify the fix
+
+Run the specific test(s) you fixed:
+
+```bash
+TESTEXPR="test_the_fixed_test" DEPLOY_MODE=build ./scripts/s3-tests/run.sh
+```
+
+Then run the full implemented test suite to confirm no regressions:
+
+```bash
+./scripts/s3-tests/run.sh
+```
+
+### Step 9: Run quality checks
+
+```bash
+make pre-commit
+```
+
+This runs:
+
+- `cargo fmt --all --check`
+- `cargo clippy --all-targets --all-features -- -D warnings`
+- `cargo test --workspace --exclude e2e_test`
+
+All three must pass before committing.
+
+### Step 10: Commit and prepare PR
+
+```bash
+git add -A
+git commit -m "fix(s3): <concise description of the fix>"
+```
+
+Write a PR description following `.github/pull_request_template.md`. The description must:
+
+- Be written in English
+- Use plain, natural language (no emoji, no marketing speak)
+- Explain what was wrong, why, and how it was fixed
+- Reference the specific s3-tests that now pass
+
+### Step 11: Update test lists
+
+Move the now-passing test(s) from `unimplemented_tests.txt` to `implemented_tests.txt`. Update the test count comment in `implemented_tests.txt`.
+
+## Important Rules
+
+1. **One branch, one fix** — never mix unrelated changes.
+2. **Analyze before coding** — understand the root cause thoroughly before writing a fix.
+3. **No shotgun debugging** — do not blindly try different return codes or response shapes hoping to pass the test.
+4. **Every change must be justified** — if you cannot explain why a line changed, do not change it.
+5. **Compare with MinIO** — when in doubt about the correct behavior, check MinIO's source code for the equivalent logic.
+6. **Security first** — never skip input validation, permission checks, or error handling for the sake of passing a test.
@@ -17,7 +17,10 @@
 # - Metadata: User-defined metadata
 # - Conditional GET: If-Match, If-None-Match, If-Modified-Since
 #
-# Total: 123 tests
+# - SSE-C: Server-side encryption with customer-provided keys
+# - Object ownership: Bucket ownership controls
+#
+# Total: 159 tests

 test_basic_key_count
 test_bucket_create_naming_bad_short_one
@@ -149,3 +152,51 @@ test_set_multipart_tagging
 test_upload_part_copy_percent_encoded_key
 test_api_error_from_storage_error_mappings
 test_get_object_torrent
+
+# SSE-C encryption tests
+test_encryption_sse_c_method_head
+test_encryption_sse_c_present
+test_encryption_sse_c_other_key
+
+# ListObjectsV2 delimiter and encoding tests
+test_bucket_list_encoding_basic
+test_bucket_listv2_delimiter_alt
+test_bucket_listv2_delimiter_basic
+test_bucket_listv2_delimiter_dot
+test_bucket_listv2_delimiter_empty
+test_bucket_listv2_delimiter_none
+test_bucket_listv2_delimiter_not_exist
+test_bucket_listv2_delimiter_percentage
+test_bucket_listv2_delimiter_prefix_ends_with_delimiter
+test_bucket_listv2_delimiter_unreadable
+test_bucket_listv2_delimiter_whitespace
+test_bucket_listv2_encoding_basic
+
+# Multipart and tagging tests
+test_abort_multipart_upload
+test_multipart_resend_first_finishes_last
+test_set_bucket_tagging
+test_put_excess_key_tags
+test_put_excess_tags
+test_put_excess_val_tags
+
+# PublicAccessBlock tests
+test_put_get_delete_public_block
+test_put_public_block
+test_block_public_policy
+test_block_public_policy_with_principal
+test_get_public_block_deny_bucket_policy
+test_get_undefined_public_block
+
+# Bucket policy tests
+test_bucketv2_policy
+test_bucket_policy_acl
+test_bucketv2_policy_acl
+test_bucket_policy_another_bucket
+test_bucket_policy_allow_notprincipal
+test_bucket_policy_put_obj_acl
+test_object_presigned_put_object_with_acl
+test_object_put_acl_mtime
+
+# Object ownership
+test_create_bucket_no_ownership_controls
@@ -16,9 +16,6 @@
 # - STS: Security Token Service
 # - Checksum: Full checksum validation
 # - Conditional writes: If-Match/If-None-Match for writes
-# - Object ownership: BucketOwnerEnforced/Preferred
-#
-# Total: all unimplemented S3 feature tests listed below (keep this comment in sync with the list)

 test_bucket_create_delete_bucket_ownership
 test_bucket_logging_owner
@@ -32,12 +29,9 @@ test_delete_bucket_encryption_kms
 test_delete_bucket_encryption_s3
 test_encryption_key_no_sse_c
 test_encryption_sse_c_invalid_md5
-test_encryption_sse_c_method_head
 test_encryption_sse_c_multipart_bad_download
 test_encryption_sse_c_no_key
 test_encryption_sse_c_no_md5
-test_encryption_sse_c_other_key
-test_encryption_sse_c_present
 test_get_bucket_encryption_kms
 test_get_bucket_encryption_s3
 test_get_versioned_object_attributes
@@ -104,21 +98,6 @@ test_versioning_obj_plain_null_version_overwrite_suspended
 test_versioning_obj_plain_null_version_removal
 test_versioning_obj_suspend_versions

-# Teardown issues (list_object_versions on non-versioned buckets)
-# These tests pass but have cleanup issues with list_object_versions
-test_bucket_list_encoding_basic
-test_bucket_listv2_delimiter_alt
-test_bucket_listv2_delimiter_basic
-test_bucket_listv2_delimiter_dot
-test_bucket_listv2_delimiter_empty
-test_bucket_listv2_delimiter_none
-test_bucket_listv2_delimiter_not_exist
-test_bucket_listv2_delimiter_percentage
-test_bucket_listv2_delimiter_prefix_ends_with_delimiter
-test_bucket_listv2_delimiter_unreadable
-test_bucket_listv2_delimiter_whitespace
-test_bucket_listv2_encoding_basic
-
 # Checksum and atomic write tests (require x-amz-checksum-* support)
 test_atomic_dual_write_1mb
 test_atomic_dual_write_4mb
@@ -130,50 +109,13 @@ test_atomic_read_8mb
 test_atomic_write_1mb
 test_atomic_write_4mb
 test_atomic_write_8mb
-test_set_bucket_tagging

-# Tests with implementation issues (need investigation)
-test_bucket_policy_acl
+# Tests with known issues (need further investigation)
 test_bucket_policy_different_tenant
-test_bucketv2_policy_acl
-test_multipart_resend_first_finishes_last
-
-# Multipart abort and policy issues
-test_abort_multipart_upload
 test_bucket_policy_multipart
-
-# Tests with prefix conflicts or ACL/tenant dependencies
-test_bucket_policy
-test_bucket_policy_allow_notprincipal
-test_bucket_policy_another_bucket
-test_bucket_policy_put_obj_acl
 test_bucket_policy_put_obj_grant
 test_bucket_policy_tenanted_bucket
-test_bucketv2_policy
-test_object_presigned_put_object_with_acl
 test_object_presigned_put_object_with_acl_tenant
-test_object_put_acl_mtime

-# ACL-dependent tests (PutBucketAcl not implemented)
-test_block_public_object_canned_acls
-test_block_public_put_bucket_acls
-test_get_authpublic_acl_bucket_policy_status
-test_get_nonpublicpolicy_acl_bucket_policy_status
-test_get_public_acl_bucket_policy_status
-test_get_publicpolicy_acl_bucket_policy_status
-test_ignore_public_acls
-
-# PublicAccessBlock and tag validation tests
-test_block_public_policy
-test_block_public_policy_with_principal
-test_get_public_block_deny_bucket_policy
-test_get_undefined_public_block
-test_put_excess_key_tags
-test_put_excess_tags
-test_put_excess_val_tags
-test_put_get_delete_public_block
-test_put_public_block
-
-# Object attributes and torrent tests
-test_create_bucket_no_ownership_controls
+# Object attributes
 test_get_checksum_object_attributes