From f86761fae980022b5664bf4b04d8b5f511e3007a Mon Sep 17 00:00:00 2001 From: LeonWang0735 Date: Mon, 5 Jan 2026 13:07:42 +0800 Subject: [PATCH] fix:allow NotResource-only policies in statement validation (#1364) Co-authored-by: loverustfs --- crates/policy/src/policy.rs | 3 + crates/policy/src/policy/policy.rs | 82 ++++++++++++++++++++++++ crates/policy/src/policy/statement.rs | 39 +++++++++-- crates/policy/tests/policy_is_allowed.rs | 46 +++++++++++++ 4 files changed, 164 insertions(+), 6 deletions(-) diff --git a/crates/policy/src/policy.rs b/crates/policy/src/policy.rs index 0a034a865..f732595f0 100644 --- a/crates/policy/src/policy.rs +++ b/crates/policy/src/policy.rs @@ -51,6 +51,9 @@ pub enum Error { #[error("'Resource' is empty")] NonResource, + #[error("'Resource' and 'NotResource' cannot both be specified in the same statement")] + BothResourceAndNotResource, + #[error("invalid key name: '{0}'")] InvalidKeyName(String), diff --git a/crates/policy/src/policy/policy.rs b/crates/policy/src/policy/policy.rs index 91dbb4c92..1e763dfa6 100644 --- a/crates/policy/src/policy/policy.rs +++ b/crates/policy/src/policy/policy.rs @@ -868,4 +868,86 @@ mod test { Ok(()) } + + #[test] + fn test_statement_with_only_notresource_is_valid() { + // Test: A statement with only NotResource (and no Resource) is valid + let data = r#" +{ + "Version": "2012-10-17", + "Statement": [ + { + "Effect": "Allow", + "Action": ["s3:GetObject"], + "NotResource": ["arn:aws:s3:::mybucket/private/*"] + } + ] +} +"#; + + let result = Policy::parse_config(data.as_bytes()); + assert!(result.is_ok(), "Statement with only NotResource should be valid"); + } + + #[test] + fn test_statement_with_both_resource_and_notresource_is_invalid() { + // Test: A statement with both Resource and NotResource returns BothResourceAndNotResource error + let data = r#" +{ + "Version": "2012-10-17", + "Statement": [ + { + "Effect": "Allow", + "Action": ["s3:GetObject"], + "Resource": ["arn:aws:s3:::mybucket/public/*"], + "NotResource": ["arn:aws:s3:::mybucket/private/*"] + } + ] +} +"#; + + let result = Policy::parse_config(data.as_bytes()); + assert!(result.is_err(), "Statement with both Resource and NotResource should be invalid"); + + // Verify the specific error type + if let Err(e) = result { + let error_msg = format!("{}", e); + assert!( + error_msg.contains("Resource") + && error_msg.contains("NotResource") + && error_msg.contains("cannot both be specified"), + "Error should be BothResourceAndNotResource, got: {}", + error_msg + ); + } + } + + #[test] + fn test_statement_with_neither_resource_nor_notresource_is_invalid() { + // Test: A statement with neither Resource nor NotResource returns NonResource error + let data = r#" +{ + "Version": "2012-10-17", + "Statement": [ + { + "Effect": "Allow", + "Action": ["s3:GetObject"] + } + ] +} +"#; + + let result = Policy::parse_config(data.as_bytes()); + assert!(result.is_err(), "Statement with neither Resource nor NotResource should be invalid"); + + // Verify the specific error type + if let Err(e) = result { + let error_msg = format!("{}", e); + assert!( + error_msg.contains("Resource") && error_msg.contains("empty"), + "Error should be NonResource, got: {}", + error_msg + ); + } + } } diff --git a/crates/policy/src/policy/statement.rs b/crates/policy/src/policy/statement.rs index a27d8528f..6412e1c8e 100644 --- a/crates/policy/src/policy/statement.rs +++ b/crates/policy/src/policy/statement.rs @@ -107,10 +107,26 @@ impl Statement { break 'c self.conditions.evaluate_with_resolver(args.conditions, Some(&resolver)).await; } - if !self - .resources - .is_match_with_resolver(&resource, args.conditions, Some(&resolver)) - .await + if self.resources.is_empty() && self.not_resources.is_empty() && !self.is_admin() && !self.is_sts() { + break 'c false; + } + + if !self.resources.is_empty() + && !self + .resources + .is_match_with_resolver(&resource, args.conditions, Some(&resolver)) + .await + && !self.is_admin() + && !self.is_sts() + { + break 'c false; + } + + if !self.not_resources.is_empty() + && self + .not_resources + .is_match_with_resolver(&resource, args.conditions, Some(&resolver)) + .await && !self.is_admin() && !self.is_sts() { @@ -135,13 +151,19 @@ impl Validator for Statement { return Err(IamError::NonAction.into()); } - if self.resources.is_empty() { + // policy must contain either Resource or NotResource (but not both), and cannot have both empty. + if self.resources.is_empty() && self.not_resources.is_empty() { return Err(IamError::NonResource.into()); } + if !self.resources.is_empty() && !self.not_resources.is_empty() { + return Err(IamError::BothResourceAndNotResource.into()); + } + self.actions.is_valid()?; self.not_actions.is_valid()?; self.resources.is_valid()?; + self.not_resources.is_valid()?; Ok(()) } @@ -228,13 +250,18 @@ impl Validator for BPStatement { return Err(IamError::NonAction.into()); } - if self.resources.is_empty() { + if self.resources.is_empty() && self.not_resources.is_empty() { return Err(IamError::NonResource.into()); } + if !self.resources.is_empty() && !self.not_resources.is_empty() { + return Err(IamError::BothResourceAndNotResource.into()); + } + self.actions.is_valid()?; self.not_actions.is_valid()?; self.resources.is_valid()?; + self.not_resources.is_valid()?; Ok(()) } diff --git a/crates/policy/tests/policy_is_allowed.rs b/crates/policy/tests/policy_is_allowed.rs index 00991a71c..771746417 100644 --- a/crates/policy/tests/policy_is_allowed.rs +++ b/crates/policy/tests/policy_is_allowed.rs @@ -611,6 +611,52 @@ struct ArgsBuilder { } => false; "24" )] +#[test_case( + Policy{ + version: DEFAULT_VERSION.into(), + statements: vec![ + Statement{ + effect: Allow, + actions: ActionSet(vec![rustfs_policy::policy::action::Action::S3Action(GetObjectAction)].into_iter().collect()), + resources: ResourceSet::default(), // Empty Resource + not_resources: ResourceSet(vec!["arn:aws:s3:::mybucket/private/*".try_into().unwrap()].into_iter().collect()), + ..Default::default() + } + ], + ..Default::default() + }, + ArgsBuilder{ + account: "Q3AM3UQ867SPQQA43P2F".into(), + action: "s3:GetObject".into(), + bucket: "mybucket".into(), + object: "public/file.txt".into(), + ..Default::default() + } => true; + "notresource_allows_access_outside_blacklist" +)] +#[test_case( + Policy{ + version: DEFAULT_VERSION.into(), + statements: vec![ + Statement{ + effect: Allow, + actions: ActionSet(vec![rustfs_policy::policy::action::Action::S3Action(GetObjectAction)].into_iter().collect()), + resources: ResourceSet::default(), // Empty Resource + not_resources: ResourceSet(vec!["arn:aws:s3:::mybucket/private/*".try_into().unwrap()].into_iter().collect()), + ..Default::default() + } + ], + ..Default::default() + }, + ArgsBuilder{ + account: "Q3AM3UQ867SPQQA43P2F".into(), + action: "s3:GetObject".into(), + bucket: "mybucket".into(), + object: "private/secret.txt".into(), + ..Default::default() + } => false; + "notresource_denies_access_in_blacklist" +)] fn policy_is_allowed(policy: Policy, args: ArgsBuilder) -> bool { pollster::block_on(policy.is_allowed(&Args { account: &args.account,