From b747e9817e4737e13a59ae618e4df3328cd04815 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=AE=89=E6=AD=A3=E8=B6=85?= Date: Tue, 28 Apr 2026 22:09:42 +0800 Subject: [PATCH] fix(policy): preserve gateway ListBucket resources (#2710) --- crates/policy/src/policy/function.rs | 10 ++++- .../policy/src/policy/function/condition.rs | 34 ++++++++++++++++ crates/policy/src/policy/function/func.rs | 6 ++- crates/policy/src/policy/policy.rs | 39 ++++++++++++++++++- crates/policy/src/policy/statement.rs | 26 +++++++------ 5 files changed, 101 insertions(+), 14 deletions(-) diff --git a/crates/policy/src/policy/function.rs b/crates/policy/src/policy/function.rs index ab079e51d..2a598c398 100644 --- a/crates/policy/src/policy/function.rs +++ b/crates/policy/src/policy/function.rs @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -use crate::policy::function::condition::Condition; +use crate::policy::function::{condition::Condition, key_name::KeyName}; use crate::policy::variables::PolicyVariableResolver; use serde::ser::SerializeMap; use serde::{Deserialize, Serialize, Serializer, de}; @@ -71,6 +71,14 @@ impl Functions { pub fn is_empty(&self) -> bool { self.for_all_values.is_empty() && self.for_any_value.is_empty() && self.for_normal.is_empty() } + + pub fn references_key_name(&self, key_name: &KeyName) -> bool { + self.for_any_value + .iter() + .chain(self.for_all_values.iter()) + .chain(self.for_normal.iter()) + .any(|condition| condition.references_key_name(key_name)) + } } impl Serialize for Functions { diff --git a/crates/policy/src/policy/function/condition.rs b/crates/policy/src/policy/function/condition.rs index 40bf66d14..53244944d 100644 --- a/crates/policy/src/policy/function/condition.rs +++ b/crates/policy/src/policy/function/condition.rs @@ -19,6 +19,7 @@ use serde::ser::SerializeMap; use std::collections::HashMap; use time::OffsetDateTime; +use super::key_name::KeyName; use super::{addr::AddrFunc, binary::BinaryFunc, bool_null::BoolFunc, date::DateFunc, number::NumberFunc, string::StringFunc}; #[derive(Clone, Deserialize, Debug)] @@ -171,6 +172,39 @@ impl Condition { } } + pub fn references_key_name(&self, key_name: &KeyName) -> bool { + use Condition::*; + match self { + StringEquals(s) + | StringNotEquals(s) + | StringEqualsIgnoreCase(s) + | StringNotEqualsIgnoreCase(s) + | StringLike(s) + | StringNotLike(s) + | ArnLike(s) + | ArnNotLike(s) + | ArnEquals(s) + | ArnNotEquals(s) => s.contains_key_name(key_name), + BinaryEquals(s) => s.contains_key_name(key_name), + IpAddress(s) | NotIpAddress(s) => s.contains_key_name(key_name), + Null(s) | Bool(s) => s.contains_key_name(key_name), + NumericEquals(s) + | NumericNotEquals(s) + | NumericLessThan(s) + | NumericLessThanEquals(s) + | NumericGreaterThan(s) + | NumericGreaterThanIfExists(s) + | NumericGreaterThanEquals(s) => s.contains_key_name(key_name), + DateEquals(s) + | DateNotEquals(s) + | DateLessThan(s) + | DateLessThanEquals(s) + | DateGreaterThan(s) + | DateGreaterThanEquals(s) => s.contains_key_name(key_name), + IfExists(inner) => inner.references_key_name(key_name), + } + } + pub fn evaluate_with_resolver<'a>( &'a self, for_all: bool, diff --git a/crates/policy/src/policy/function/func.rs b/crates/policy/src/policy/function/func.rs index 1b75c77c8..d088b6444 100644 --- a/crates/policy/src/policy/function/func.rs +++ b/crates/policy/src/policy/function/func.rs @@ -19,7 +19,7 @@ use serde::{ de::{self, Visitor}, }; -use super::key::Key; +use super::{key::Key, key_name::KeyName}; #[derive(PartialEq, Eq, Debug)] pub struct InnerFunc(pub(crate) Vec>); @@ -49,6 +49,10 @@ impl InnerFunc { pub fn key_names(&self) -> impl Iterator + '_ { self.0.iter().map(|kv| kv.key.name()) } + + pub fn contains_key_name(&self, key_name: &KeyName) -> bool { + self.0.iter().any(|kv| kv.key.is(key_name)) + } } impl Serialize for InnerFunc { diff --git a/crates/policy/src/policy/policy.rs b/crates/policy/src/policy/policy.rs index 9519c3a91..ce33efabb 100644 --- a/crates/policy/src/policy/policy.rs +++ b/crates/policy/src/policy/policy.rs @@ -938,7 +938,8 @@ mod test { }"#, )?; - let conditions = HashMap::new(); + let mut conditions = HashMap::new(); + conditions.insert("prefix".to_string(), vec!["home/alice/projects/".to_string()]); let claims = HashMap::new(); let args = Args { account: "polaris-session", @@ -960,6 +961,42 @@ mod test { Ok(()) } + #[tokio::test] + async fn test_bucket_policy_gateway_prefix_uses_object_resource_when_condition_missing() -> Result<()> { + let bucket_policy: BucketPolicy = serde_json::from_str( + r#"{ + "Version": "2012-10-17", + "Statement": [ + { + "Effect": "Allow", + "Principal": {"AWS": "*"}, + "Action": ["s3:ListBucket"], + "Resource": ["arn:aws:s3:::polaris-test-bucket/home/alice/*"] + } + ] +}"#, + )?; + + let mut conditions = HashMap::new(); + conditions.insert("prefix".to_string(), vec!["home/alice/projects/".to_string()]); + let args = BucketPolicyArgs { + account: "polaris-session", + groups: &None, + action: Action::S3Action(crate::policy::action::S3Action::ListBucketAction), + bucket: "polaris-test-bucket", + conditions: &conditions, + is_owner: false, + object: "home/alice/projects/", + }; + + assert!( + bucket_policy.is_allowed(&args).await, + "Bucket policy ListBucket without an s3:prefix condition should continue matching prefix-scoped resources via args.object" + ); + + Ok(()) + } + #[tokio::test] async fn test_aws_username_policy_variable() -> Result<()> { let data = r#" diff --git a/crates/policy/src/policy/statement.rs b/crates/policy/src/policy/statement.rs index e45ff0d07..6498b55c0 100644 --- a/crates/policy/src/policy/statement.rs +++ b/crates/policy/src/policy/statement.rs @@ -15,6 +15,7 @@ use super::{ ActionSet, Args, BucketPolicyArgs, Effect, Error as IamError, Functions, ID, Principal, ResourceSet, Validator, action::{Action, S3Action}, + function::key_name::{KeyName, S3KeyName}, variables::{VariableContext, VariableResolver}, }; use crate::error::{Error, Result}; @@ -56,20 +57,13 @@ pub(crate) fn variable_resolver_for_policy_args(args: &Args<'_>) -> VariableReso VariableResolver::new(context) } -const LIST_BUCKET_PREFIX_CONDITION_KEY: &str = "prefix"; - -fn build_resource( - action: &Action, - bucket: &str, - object: &str, - conditions: &std::collections::HashMap>, -) -> String { +fn build_resource(action: &Action, bucket: &str, object: &str, bucket_resource_only: bool) -> String { let bucket_resource_only = matches!( action, Action::S3Action( S3Action::ListBucketAction | S3Action::ListBucketVersionsAction | S3Action::ListBucketMultipartUploadsAction ) - ) && conditions.contains_key(LIST_BUCKET_PREFIX_CONDITION_KEY); + ) && bucket_resource_only; let mut resource = String::from(bucket); if bucket_resource_only || object.is_empty() { @@ -122,7 +116,12 @@ impl Statement { return false; } - let resource = build_resource(&args.action, args.bucket, args.object, args.conditions); + let resource = build_resource( + &args.action, + args.bucket, + args.object, + self.conditions.references_key_name(&KeyName::S3(S3KeyName::S3Prefix)), + ); if self.is_kms() && (resource == "/" || self.resources.is_empty()) { return true; @@ -249,7 +248,12 @@ impl BPStatement { return false; } - let resource = build_resource(&args.action, args.bucket, args.object, args.conditions); + let resource = build_resource( + &args.action, + args.bucket, + args.object, + self.conditions.references_key_name(&KeyName::S3(S3KeyName::S3Prefix)), + ); if !self.resources.is_empty() && !self.resources.is_match(&resource, args.conditions).await { return false;