From fe884eabfc573473a7ff2b848b3b92223f82615c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=AE=89=E6=AD=A3=E8=B6=85?= Date: Sun, 1 Mar 2026 02:21:13 +0800 Subject: [PATCH] fix(s3): improve S3 API compatibility for versioning, SSE, and policy (#2013) --- rustfs/src/app/bucket_usecase.rs | 19 +++++++++++--- rustfs/src/app/multipart_usecase.rs | 33 +++++++++++++++--------- rustfs/src/app/object_usecase.rs | 13 +++++++--- rustfs/src/storage/access.rs | 11 +++++--- rustfs/src/storage/sse.rs | 14 +++++++++- scripts/s3-tests/implemented_tests.txt | 9 ++++++- scripts/s3-tests/unimplemented_tests.txt | 7 +---- 7 files changed, 75 insertions(+), 31 deletions(-) diff --git a/rustfs/src/app/bucket_usecase.rs b/rustfs/src/app/bucket_usecase.rs index 9d614fa59..cdf6259f0 100644 --- a/rustfs/src/app/bucket_usecase.rs +++ b/rustfs/src/app/bucket_usecase.rs @@ -168,17 +168,28 @@ impl DefaultBucketUsecase { counter!("rustfs_create_bucket_total").increment(1); - store + let make_result = store .make_bucket( &bucket, &MakeBucketOptions { - force_create: false, // TODO: force support + force_create: false, lock_enabled: object_lock_enabled_for_bucket.is_some_and(|v| v), ..Default::default() }, ) - .await - .map_err(ApiError::from)?; + .await; + + match make_result { + Ok(()) => {} + Err(StorageError::BucketExists(_)) => { + // Per S3 spec: CreateBucket for a bucket you already own returns 200 OK + let output = CreateBucketOutput::default(); + let result = Ok(S3Response::new(output)); + let _ = helper.complete(&result); + return result; + } + Err(e) => return Err(ApiError::from(e).into()), + } let owner = default_owner(); let mut stored_acl = stored_acl_from_grant_headers( diff --git a/rustfs/src/app/multipart_usecase.rs b/rustfs/src/app/multipart_usecase.rs index e77cb53ee..9474ae10a 100644 --- a/rustfs/src/app/multipart_usecase.rs +++ b/rustfs/src/app/multipart_usecase.rs @@ -625,18 +625,27 @@ impl DefaultMultipartUsecase { return Err(ApiError::from(StorageError::other(format!("add_checksum error={err:?}"))).into()); } - let server_side_encryption = fi - .user_defined - .get("x-amz-server-side-encryption") - .map(|s| { - ServerSideEncryption::from_str(s) - .map_err(|e| ApiError::from(StorageError::other(format!("Invalid server-side encryption: {e}")))) - }) - .transpose()?; - let ssekms_key_id = fi - .user_defined - .get("x-amz-server-side-encryption-aws-kms-key-id") - .map(|s| s.to_string()); + let has_ssec = sse_customer_algorithm.is_some(); + // When SSE-C headers are present, skip managed-encryption metadata to avoid + // false conflict: the bucket default SSE config stored in multipart metadata + // should not block a legitimate SSE-C upload part. + let (server_side_encryption, ssekms_key_id) = if has_ssec { + (None, None) + } else { + let sse = fi + .user_defined + .get("x-amz-server-side-encryption") + .map(|s| { + ServerSideEncryption::from_str(s) + .map_err(|e| ApiError::from(StorageError::other(format!("Invalid server-side encryption: {e}")))) + }) + .transpose()?; + let key_id = fi + .user_defined + .get("x-amz-server-side-encryption-aws-kms-key-id") + .map(|s| s.to_string()); + (sse, key_id) + }; let part_key = fi.user_defined.get("x-rustfs-encryption-key").cloned(); let part_nonce = fi.user_defined.get("x-rustfs-encryption-iv").cloned(); let encryption_request = EncryptionRequest { diff --git a/rustfs/src/app/object_usecase.rs b/rustfs/src/app/object_usecase.rs index 0912d0127..d6ace5f65 100644 --- a/rustfs/src/app/object_usecase.rs +++ b/rustfs/src/app/object_usecase.rs @@ -550,14 +550,21 @@ impl DefaultObjectUsecase { // Fast in-memory update for immediate quota consistency rustfs_ecstore::data_usage::increment_bucket_usage_memory(&bucket, obj_info.size as u64).await; - let put_version = obj_info.version_id.map(|v| v.to_string()); + let raw_version = obj_info.version_id.map(|v| v.to_string()); helper = helper.object(obj_info.clone()); - if let Some(version_id) = &put_version { + if let Some(version_id) = &raw_version { helper = helper.version_id(version_id.clone()); } - Self::spawn_cache_invalidation(bucket.clone(), key.clone(), put_version.clone()); + Self::spawn_cache_invalidation(bucket.clone(), key.clone(), raw_version.clone()); + + // Per S3 spec: only return VersionId when versioning is Enabled (not Suspended or default) + let put_version = if BucketVersioningSys::prefix_enabled(&bucket, &key).await { + raw_version + } else { + None + }; let e_tag = obj_info.etag.clone().map(|etag| to_s3s_etag(&etag)); diff --git a/rustfs/src/storage/access.rs b/rustfs/src/storage/access.rs index 88acd5068..9024e839e 100644 --- a/rustfs/src/storage/access.rs +++ b/rustfs/src/storage/access.rs @@ -628,11 +628,14 @@ impl S3Access for FS { } /// Checks whether the CreateMultipartUpload request has accesses to the resources. - /// - /// This method returns `Ok(())` by default. - async fn create_multipart_upload(&self, _req: &mut S3Request) -> S3Result<()> { + async fn create_multipart_upload(&self, req: &mut S3Request) -> S3Result<()> { license_check().map_err(|er| s3_error!(AccessDenied, "{:?}", er.to_string()))?; - Ok(()) + + let req_info = ext_req_info_mut(&mut req.extensions)?; + req_info.bucket = Some(req.input.bucket.clone()); + req_info.object = Some(req.input.key.clone()); + + authorize_request(req, Action::S3Action(S3Action::PutObjectAction)).await } /// Checks whether the DeleteBucket request has accesses to the resources. diff --git a/rustfs/src/storage/sse.rs b/rustfs/src/storage/sse.rs index 679b2b4df..4e23f920f 100644 --- a/rustfs/src/storage/sse.rs +++ b/rustfs/src/storage/sse.rs @@ -204,7 +204,19 @@ async fn prepare_sse_configuration( })) } else if let Err(e) = bucket_sse_config_result { match e { - Error::ConfigNotFound => Ok(None), + Error::ConfigNotFound => { + // The bucket has no SSE config. If the user explicitly requested + // aws:kms, we must honor that — return the explicit SSE header so + // downstream logic can try (and fail if KMS is unavailable). + if let Some(sse) = server_side_encryption { + Ok(Some(SseConfiguration { + effective_sse: sse, + effective_kms_key_id: ssekms_key_id, + })) + } else { + Ok(None) + } + } _ => Err(ApiError::from(e)), } } else { diff --git a/scripts/s3-tests/implemented_tests.txt b/scripts/s3-tests/implemented_tests.txt index 6f590cfd3..eaaa982b1 100644 --- a/scripts/s3-tests/implemented_tests.txt +++ b/scripts/s3-tests/implemented_tests.txt @@ -20,7 +20,10 @@ # - SSE-C: Server-side encryption with customer-provided keys # - Object ownership: Bucket ownership controls # -# Total: 224 tests +# - SSE-KMS: KMS-related edge cases +# - Bucket Policy: Multipart upload authorization +# +# Total: 228 tests test_basic_key_count test_bucket_create_naming_bad_short_one @@ -169,7 +172,9 @@ test_put_obj_enc_conflict_c_kms test_put_obj_enc_conflict_s3_kms test_put_obj_enc_conflict_bad_enc_kms test_sse_kms_not_declared +test_sse_kms_no_key test_sse_kms_read_declare +test_encryption_sse_c_multipart_bad_download # ListObjectsV2 delimiter and encoding tests test_bucket_list_encoding_basic @@ -207,6 +212,7 @@ test_bucket_policy_acl test_bucketv2_policy_acl test_bucket_policy_another_bucket test_bucket_policy_allow_notprincipal +test_bucket_policy_multipart test_bucket_policy_put_obj_acl test_object_presigned_put_object_with_acl test_object_put_acl_mtime @@ -255,6 +261,7 @@ test_versioned_concurrent_object_create_and_remove test_versioned_concurrent_object_create_concurrent_remove test_versioned_object_acl test_versioning_bucket_create_suspend +test_versioning_bucket_atomic_upload_return_version_id test_versioning_bucket_multipart_upload_return_version_id test_versioning_multi_object_delete test_versioning_multi_object_delete_with_marker diff --git a/scripts/s3-tests/unimplemented_tests.txt b/scripts/s3-tests/unimplemented_tests.txt index 0d44dd075..3afe5216d 100644 --- a/scripts/s3-tests/unimplemented_tests.txt +++ b/scripts/s3-tests/unimplemented_tests.txt @@ -6,18 +6,16 @@ # # Unimplemented features: # - Bucket Logging: Access logging -# - SSE-KMS: Partial SSE-KMS edge cases # - Object Lock: Enable after create # - Checksum: Full checksum validation # - Conditional writes: If-Match/If-None-Match for writes # - Bucket Ownership Controls -# Failed tests (21) +# Failed tests (17) test_bucket_create_delete_bucket_ownership test_bucket_logging_owner test_bucket_policy_put_obj_kms_s3 test_bucket_policy_put_obj_s3_kms -test_encryption_sse_c_multipart_bad_download test_object_checksum_crc64nvme test_object_checksum_sha256 test_object_lock_put_obj_lock_enable_after_create @@ -27,8 +25,6 @@ test_put_bucket_logging_errors test_put_bucket_logging_permissions test_put_bucket_logging_policy_wildcard test_rm_bucket_logging -test_sse_kms_no_key -test_versioning_bucket_atomic_upload_return_version_id test_versioning_concurrent_multi_object_delete # Skipped tests (require IAM account or multiple storage classes) @@ -52,6 +48,5 @@ test_atomic_write_8mb # Tests with known issues (need further investigation) test_bucket_policy_different_tenant -test_bucket_policy_multipart test_bucket_policy_put_obj_grant test_bucket_policy_tenanted_bucket