From e7466eb1ccf37126ca6143d1bcfa5d186e8a04e5 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 03:13:52 +0800 Subject: [PATCH] fix: policy StringNotEquals double negation and delete_objects version mapping (#2015) --- .../policy/src/policy/function/condition.rs | 91 ++++++++++++++++++- crates/policy/src/policy/function/key_name.rs | 15 +++ crates/policy/src/policy/policy.rs | 87 ++++++++++++++++++ rustfs/src/app/object_usecase.rs | 21 ++--- scripts/s3-tests/implemented_tests.txt | 8 +- scripts/s3-tests/unimplemented_tests.txt | 5 +- 6 files changed, 208 insertions(+), 19 deletions(-) diff --git a/crates/policy/src/policy/function/condition.rs b/crates/policy/src/policy/function/condition.rs index 5792f2522..f25b6d07f 100644 --- a/crates/policy/src/policy/function/condition.rs +++ b/crates/policy/src/policy/function/condition.rs @@ -148,7 +148,9 @@ impl Condition { #[inline] pub fn is_negate(&self) -> bool { use Condition::*; - matches!(self, StringNotEquals(_) | StringNotEqualsIgnoreCase(_) | NotIpAddress(_)) + // StringNotEquals/StringNotEqualsIgnoreCase handle negation via the + // `negate` parameter in `evaluate_with_resolver`; do NOT negate again here. + matches!(self, NotIpAddress(_)) } pub fn serialize_map(&self, se: &mut T) -> Result<(), T::Error> { @@ -212,3 +214,90 @@ impl PartialEq for Condition { } } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::policy::function::{ + func::{FuncKeyValue, InnerFunc}, + key::Key, + string::StringFuncValue, + }; + use std::collections::{BTreeSet, HashMap}; + + fn make_string_condition(condition_type: &str, key: &str, value: &str) -> Condition { + let func: StringFunc = InnerFunc(vec![FuncKeyValue { + key: Key { + name: key.try_into().unwrap(), + variable: None, + }, + values: StringFuncValue({ + let mut s = BTreeSet::new(); + s.insert(value.to_string()); + s + }), + }]); + match condition_type { + "StringEquals" => Condition::StringEquals(func), + "StringNotEquals" => Condition::StringNotEquals(func), + "StringNotEqualsIgnoreCase" => Condition::StringNotEqualsIgnoreCase(func), + _ => unreachable!(), + } + } + + #[tokio::test] + async fn test_string_not_equals_no_double_negation() { + let cond = make_string_condition("StringNotEquals", "s3:x-amz-server-side-encryption", "aws:kms"); + + let mut values = HashMap::new(); + values.insert("x-amz-server-side-encryption".to_string(), vec!["AES256".to_string()]); + + // "AES256" != "aws:kms" is true, so StringNotEquals should evaluate to true + assert!( + cond.evaluate_with_resolver(false, &values, None).await, + "StringNotEquals should be true when values differ" + ); + + values.insert("x-amz-server-side-encryption".to_string(), vec!["aws:kms".to_string()]); + + // "aws:kms" != "aws:kms" is false, so StringNotEquals should evaluate to false + assert!( + !cond.evaluate_with_resolver(false, &values, None).await, + "StringNotEquals should be false when values match" + ); + } + + #[tokio::test] + async fn test_string_equals_condition() { + let cond = make_string_condition("StringEquals", "s3:x-amz-server-side-encryption", "aws:kms"); + + let mut values = HashMap::new(); + values.insert("x-amz-server-side-encryption".to_string(), vec!["aws:kms".to_string()]); + + assert!( + cond.evaluate_with_resolver(false, &values, None).await, + "StringEquals should be true when values match" + ); + + values.insert("x-amz-server-side-encryption".to_string(), vec!["AES256".to_string()]); + + assert!( + !cond.evaluate_with_resolver(false, &values, None).await, + "StringEquals should be false when values differ" + ); + } + + #[tokio::test] + async fn test_string_not_equals_absent_key() { + let cond = make_string_condition("StringNotEquals", "s3:x-amz-server-side-encryption", "aws:kms"); + + let values = HashMap::new(); + + // Key absent: rvalues is empty, intersection is empty. + // for_all=false: ivalues.count() > 0 → false. Negated → true. + assert!( + cond.evaluate_with_resolver(false, &values, None).await, + "StringNotEquals should be true when key is absent" + ); + } +} diff --git a/crates/policy/src/policy/function/key_name.rs b/crates/policy/src/policy/function/key_name.rs index c24cd83ee..c628a6fe0 100644 --- a/crates/policy/src/policy/function/key_name.rs +++ b/crates/policy/src/policy/function/key_name.rs @@ -184,6 +184,21 @@ pub enum S3KeyName { #[strum(serialize = "s3:delimiter")] S3Delimiter, + #[strum(serialize = "s3:x-amz-grant-full-control")] + S3XAmzGrantFullControl, + + #[strum(serialize = "s3:x-amz-grant-read")] + S3XAmzGrantRead, + + #[strum(serialize = "s3:x-amz-grant-write")] + S3XAmzGrantWrite, + + #[strum(serialize = "s3:x-amz-grant-read-acp")] + S3XAmzGrantReadAcp, + + #[strum(serialize = "s3:x-amz-grant-write-acp")] + S3XAmzGrantWriteAcp, + #[strum(serialize = "s3:ExistingObjectTag")] S3ExistingObjectTag, #[strum(serialize = "s3:RequestObjectTagKeys")] diff --git a/crates/policy/src/policy/policy.rs b/crates/policy/src/policy/policy.rs index f757529f4..01f786cc8 100644 --- a/crates/policy/src/policy/policy.rs +++ b/crates/policy/src/policy/policy.rs @@ -1244,4 +1244,91 @@ mod test { assert_eq!(arr.len(), 1); assert_eq!(arr[0].as_str().unwrap(), "s3:ListBucket"); } + + #[tokio::test] + async fn test_bucket_policy_deny_with_string_not_equals() -> Result<()> { + let data = r#" +{ + "Version": "2012-10-17", + "Statement": [ + { + "Effect": "Deny", + "Action": "s3:PutObject", + "Principal": {"AWS": "*"}, + "Resource": "arn:aws:s3:::mybucket/*", + "Condition": { + "StringNotEquals": { + "s3:x-amz-server-side-encryption": "aws:kms" + } + } + }, + { + "Effect": "Deny", + "Action": "s3:PutObject", + "Principal": {"AWS": "*"}, + "Resource": "arn:aws:s3:::mybucket/*", + "Condition": { + "Null": { + "s3:x-amz-server-side-encryption": "true" + } + } + } + ] +} +"#; + + let bp: BucketPolicy = serde_json::from_slice(data.as_bytes())?; + + // Request with wrong encryption → should be DENIED (StringNotEquals matches) + let mut cond_wrong_enc = HashMap::new(); + cond_wrong_enc.insert("x-amz-server-side-encryption".to_string(), vec!["AES256".to_string()]); + + let args_wrong = BucketPolicyArgs { + account: "testowner", + groups: &None, + action: Action::S3Action(crate::policy::action::S3Action::PutObjectAction), + bucket: "mybucket", + conditions: &cond_wrong_enc, + is_owner: true, + object: "testobj", + }; + assert!( + !bp.is_allowed(&args_wrong).await, + "Should deny PutObject with AES256 when policy requires aws:kms" + ); + + // Request with correct encryption → should be ALLOWED + let mut cond_correct_enc = HashMap::new(); + cond_correct_enc.insert("x-amz-server-side-encryption".to_string(), vec!["aws:kms".to_string()]); + + let args_correct = BucketPolicyArgs { + account: "testowner", + groups: &None, + action: Action::S3Action(crate::policy::action::S3Action::PutObjectAction), + bucket: "mybucket", + conditions: &cond_correct_enc, + is_owner: true, + object: "testobj", + }; + assert!( + bp.is_allowed(&args_correct).await, + "Should allow PutObject with aws:kms matching the policy" + ); + + // Request with no encryption header → should be DENIED (Null condition matches) + let cond_no_enc = HashMap::new(); + + let args_no_enc = BucketPolicyArgs { + account: "testowner", + groups: &None, + action: Action::S3Action(crate::policy::action::S3Action::PutObjectAction), + bucket: "mybucket", + conditions: &cond_no_enc, + is_owner: true, + object: "testobj", + }; + assert!(!bp.is_allowed(&args_no_enc).await, "Should deny PutObject with no encryption header"); + + Ok(()) + } } diff --git a/rustfs/src/app/object_usecase.rs b/rustfs/src/app/object_usecase.rs index d6ace5f65..64ac42527 100644 --- a/rustfs/src/app/object_usecase.rs +++ b/rustfs/src/app/object_usecase.rs @@ -2391,8 +2391,8 @@ impl DefaultObjectUsecase { let mut delete_results = vec![DeleteResult::default(); delete.objects.len()]; let mut object_to_delete = Vec::new(); - let mut object_to_delete_index = HashMap::new(); - let mut object_sizes = HashMap::new(); + let mut object_to_delete_idx = Vec::new(); + let mut object_sizes = Vec::new(); for (idx, obj_id) in delete.objects.iter().enumerate() { let raw_version_id = obj_id.version_id.clone(); let (version_id, version_uuid) = match normalize_delete_objects_version_id(raw_version_id.clone()) { @@ -2460,7 +2460,7 @@ impl DefaultObjectUsecase { continue; } - object_sizes.insert(object.object_name.clone(), goi.size); + object_sizes.push(goi.size); if is_dir_object(&object.object_name) && object.version_id.is_none() { object.version_id = Some(Uuid::nil()); @@ -2490,7 +2490,7 @@ impl DefaultObjectUsecase { } } - object_to_delete_index.insert(object.object_name.clone(), idx); + object_to_delete_idx.push(idx); object_to_delete.push(object); } @@ -2532,11 +2532,7 @@ impl DefaultObjectUsecase { } for (i, err) in errs.iter().enumerate() { - let obj = dobjs[i].clone(); - - let Some(didx) = object_to_delete_index.get(&obj.object_name) else { - continue; - }; + let didx = object_to_delete_idx[i]; if err.is_none() || err @@ -2546,15 +2542,16 @@ impl DefaultObjectUsecase { if replicate_deletes { dobjs[i].replication_state = Some(object_to_delete[i].replication_state()); } - delete_results[*didx].delete_object = Some(dobjs[i].clone()); - if let Some(&size) = object_sizes.get(&obj.object_name) { + delete_results[didx].delete_object = Some(dobjs[i].clone()); + let size = object_sizes[i]; + if size > 0 { rustfs_ecstore::data_usage::decrement_bucket_usage_memory(&bucket, size as u64).await; } continue; } if let Some(err) = err.clone() { - delete_results[*didx].error = Some(Error { + delete_results[didx].error = Some(Error { code: Some(err.to_string()), key: Some(object_to_delete[i].object_name.clone()), message: Some(err.to_string()), diff --git a/scripts/s3-tests/implemented_tests.txt b/scripts/s3-tests/implemented_tests.txt index eaaa982b1..a7e8af6c8 100644 --- a/scripts/s3-tests/implemented_tests.txt +++ b/scripts/s3-tests/implemented_tests.txt @@ -21,9 +21,10 @@ # - Object ownership: Bucket ownership controls # # - SSE-KMS: KMS-related edge cases -# - Bucket Policy: Multipart upload authorization +# - Bucket Policy: Multipart upload authorization, SSE condition keys +# - Versioning: Concurrent multi-object delete # -# Total: 228 tests +# Total: 231 tests test_basic_key_count test_bucket_create_naming_bad_short_one @@ -214,6 +215,8 @@ test_bucket_policy_another_bucket test_bucket_policy_allow_notprincipal test_bucket_policy_multipart test_bucket_policy_put_obj_acl +test_bucket_policy_put_obj_kms_s3 +test_bucket_policy_put_obj_s3_kms test_object_presigned_put_object_with_acl test_object_put_acl_mtime @@ -263,6 +266,7 @@ 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_concurrent_multi_object_delete test_versioning_multi_object_delete test_versioning_multi_object_delete_with_marker test_versioning_obj_create_read_remove diff --git a/scripts/s3-tests/unimplemented_tests.txt b/scripts/s3-tests/unimplemented_tests.txt index 3afe5216d..6c4f78b9a 100644 --- a/scripts/s3-tests/unimplemented_tests.txt +++ b/scripts/s3-tests/unimplemented_tests.txt @@ -11,11 +11,9 @@ # - Conditional writes: If-Match/If-None-Match for writes # - Bucket Ownership Controls -# Failed tests (17) +# Failed tests (11) 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_object_checksum_crc64nvme test_object_checksum_sha256 test_object_lock_put_obj_lock_enable_after_create @@ -25,7 +23,6 @@ test_put_bucket_logging_errors test_put_bucket_logging_permissions test_put_bucket_logging_policy_wildcard test_rm_bucket_logging -test_versioning_concurrent_multi_object_delete # Skipped tests (require IAM account or multiple storage classes) test_bucket_policy_deny_self_denied_policy