fix: policy StringNotEquals double negation and delete_objects version mapping (#2015)

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<T: SerializeMap>(&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"
+        );
+    }
+}

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")]

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(())
+    }
 }

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()),

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

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