From 7a83b818b8453c7bc0b0f60cee77164ba476d9b6 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 11:05:20 +0800 Subject: [PATCH] fix(policy): address review feedback from #2018 (#2021) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: 安正超 Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com> --- crates/policy/src/policy/function.rs | 28 ++++++ .../policy/src/policy/function/condition.rs | 91 ++++++++++++++++++- crates/policy/src/policy/principal.rs | 32 ++++++- rustfs/src/storage/access.rs | 7 +- 4 files changed, 152 insertions(+), 6 deletions(-) diff --git a/crates/policy/src/policy/function.rs b/crates/policy/src/policy/function.rs index c568388e2..ab079e51d 100644 --- a/crates/policy/src/policy/function.rs +++ b/crates/policy/src/policy/function.rs @@ -355,6 +355,34 @@ mod tests { }"# => true; "6" )] + #[test_case( + r#"{ + "NumericLessThanEquals": { + "s3:max-keys": "10" + } + }"# => true; "numeric_less_than_equals" + )] + #[test_case( + r#"{ + "DateLessThan": { + "aws:CurrentTime": "2026-01-01T00:00:00Z" + } + }"# => true; "date_less_than" + )] + #[test_case( + r#"{ + "StringLikeIfExists": { + "aws:Referer": "http://www.example.com/*" + } + }"# => true; "string_like_if_exists" + )] + #[test_case( + r#"{ + "ArnLike": { + "aws:SourceArn": "arn:aws:s3:::my-bucket" + } + }"# => true; "arn_like" + )] fn test_de(input: &str) -> bool { serde_json::from_str::(input) .map_err(|e| eprintln!("{e:?}")) diff --git a/crates/policy/src/policy/function/condition.rs b/crates/policy/src/policy/function/condition.rs index ad6271f11..40bf66d14 100644 --- a/crates/policy/src/policy/function/condition.rs +++ b/crates/policy/src/policy/function/condition.rs @@ -78,11 +78,13 @@ impl Condition { "NumericEquals" => Self::NumericEquals(d.next_value()?), "NumericNotEquals" => Self::NumericNotEquals(d.next_value()?), "NumericLessThan" => Self::NumericLessThan(d.next_value()?), + "NumericLessThanEquals" => Self::NumericLessThanEquals(d.next_value()?), "NumericGreaterThan" => Self::NumericGreaterThan(d.next_value()?), "NumericGreaterThanIfExists" => Self::NumericGreaterThanIfExists(d.next_value()?), "NumericGreaterThanEquals" => Self::NumericGreaterThanEquals(d.next_value()?), "DateEquals" => Self::DateEquals(d.next_value()?), "DateNotEquals" => Self::DateNotEquals(d.next_value()?), + "DateLessThan" => Self::DateLessThan(d.next_value()?), "DateLessThanEquals" => Self::DateLessThanEquals(d.next_value()?), "DateGreaterThan" => Self::DateGreaterThan(d.next_value()?), "DateGreaterThanEquals" => Self::DateGreaterThanEquals(d.next_value()?), @@ -131,7 +133,7 @@ impl Condition { pub fn to_key_with_suffix(&self) -> String { match self { - Condition::IfExists(inner) => format!("{}IfExists", inner.to_key()), + Condition::IfExists(inner) => format!("{}IfExists", inner.to_key_with_suffix()), _ => self.to_key().to_owned(), } } @@ -303,6 +305,7 @@ impl PartialEq for Condition { mod tests { use super::*; use crate::policy::function::{ + Functions, func::{FuncKeyValue, InnerFunc}, key::Key, string::StringFuncValue, @@ -384,4 +387,90 @@ mod tests { "StringNotEquals should be true when key is absent" ); } + + #[test] + fn test_to_key_with_suffix_if_exists() { + let inner = make_string_condition("StringEquals", "s3:x-amz-server-side-encryption", "aws:kms"); + let cond = Condition::IfExists(Box::new(inner)); + assert_eq!(cond.to_key_with_suffix(), "StringEqualsIfExists"); + } + + #[test] + fn test_to_key_with_suffix_nested_if_exists() { + // IfExists(IfExists(StringEquals)) must produce a stable, predictable key + let inner = make_string_condition("StringEquals", "s3:x-amz-server-side-encryption", "aws:kms"); + let once = Condition::IfExists(Box::new(inner)); + let twice = Condition::IfExists(Box::new(once)); + assert_eq!(twice.to_key_with_suffix(), "StringEqualsIfExistsIfExists"); + } + + #[test] + fn test_if_exists_serde_round_trip() { + let inner = make_string_condition("StringEquals", "s3:x-amz-server-side-encryption", "aws:kms"); + let cond = Condition::IfExists(Box::new(inner)); + + let functions = Functions { + for_normal: vec![cond], + ..Default::default() + }; + + let json = serde_json::to_string(&functions).unwrap(); + assert_eq!(json, r#"{"StringEqualsIfExists":{"s3:x-amz-server-side-encryption":"aws:kms"}}"#); + + let deserialized: Functions = serde_json::from_str(&json).unwrap(); + assert_eq!(functions, deserialized); + } + + #[test] + fn test_nested_if_exists_serde_round_trip() { + // Verifies that nested IfExists(IfExists(StringEquals)) serializes with the + // correct key and round-trips through serde without data loss. + let inner = make_string_condition("StringEquals", "s3:x-amz-server-side-encryption", "aws:kms"); + let once = Condition::IfExists(Box::new(inner)); + let twice = Condition::IfExists(Box::new(once)); + + let functions = Functions { + for_normal: vec![twice], + ..Default::default() + }; + + let json = serde_json::to_string(&functions).unwrap(); + assert_eq!(json, r#"{"StringEqualsIfExistsIfExists":{"s3:x-amz-server-side-encryption":"aws:kms"}}"#); + + let deserialized: Functions = serde_json::from_str(&json).unwrap(); + assert_eq!(functions, deserialized); + } + + #[tokio::test] + async fn test_if_exists_key_absent_returns_true() { + let inner = make_string_condition("StringEquals", "s3:x-amz-server-side-encryption", "aws:kms"); + let cond = Condition::IfExists(Box::new(inner)); + + let values = HashMap::new(); + assert!( + cond.evaluate_with_resolver(false, &values, None).await, + "IfExists should return true when the key is absent" + ); + } + + #[tokio::test] + async fn test_if_exists_key_present_delegates() { + let inner = make_string_condition("StringEquals", "s3:x-amz-server-side-encryption", "aws:kms"); + let cond = Condition::IfExists(Box::new(inner)); + + 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, + "IfExists should delegate to inner and return 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, + "IfExists should delegate to inner and return false when values differ" + ); + } } diff --git a/crates/policy/src/policy/principal.rs b/crates/policy/src/policy/principal.rs index 6bdef3ff7..adfa0f510 100644 --- a/crates/policy/src/policy/principal.rs +++ b/crates/policy/src/policy/principal.rs @@ -130,9 +130,11 @@ impl Principal { return true; } } - // Service principals (e.g., logging.s3.amazonaws.com) allow internal - // AWS services. Treat them as non-matching for user requests — they - // only apply to service-initiated actions. + for pattern in self.service.iter() { + if wildcard::is_simple_match(pattern, principal) { + return true; + } + } false } } @@ -214,4 +216,28 @@ mod test { let reparsed: Principal = serde_json::from_str(&serialized).expect("Should re-parse"); assert_eq!(principal, reparsed); } + + #[test] + fn test_principal_is_match_service() { + let principal = Principal { + aws: HashSet::new(), + service: HashSet::from(["logging.s3.amazonaws.com".to_string()]), + }; + + assert!(principal.is_match("logging.s3.amazonaws.com")); + assert!(!principal.is_match("replication.s3.amazonaws.com")); + assert!(!principal.is_match("arn:aws:iam::123456789012:root")); + } + + #[test] + fn test_principal_is_match_aws_and_service() { + let principal = Principal { + aws: HashSet::from(["arn:aws:iam::123456789012:root".to_string()]), + service: HashSet::from(["logging.s3.amazonaws.com".to_string()]), + }; + + assert!(principal.is_match("arn:aws:iam::123456789012:root")); + assert!(principal.is_match("logging.s3.amazonaws.com")); + assert!(!principal.is_match("other-principal")); + } } diff --git a/rustfs/src/storage/access.rs b/rustfs/src/storage/access.rs index 5281bd4b5..e307c07b8 100644 --- a/rustfs/src/storage/access.rs +++ b/rustfs/src/storage/access.rs @@ -938,8 +938,11 @@ impl S3Access for FS { /// Checks whether the GetBucketLogging request has accesses to the resources. /// /// This method returns `Ok(())` by default. - async fn get_bucket_logging(&self, _req: &mut S3Request) -> S3Result<()> { - Ok(()) + async fn get_bucket_logging(&self, req: &mut S3Request) -> S3Result<()> { + let req_info = ext_req_info_mut(&mut req.extensions)?; + req_info.bucket = Some(req.input.bucket.clone()); + + authorize_request(req, Action::S3Action(S3Action::GetBucketLoggingAction)).await } /// Checks whether the GetBucketMetricsConfiguration request has accesses to the resources.