diff --git a/crates/iam/src/oidc.rs b/crates/iam/src/oidc.rs index d775ed6ba..726797023 100644 --- a/crates/iam/src/oidc.rs +++ b/crates/iam/src/oidc.rs @@ -1599,7 +1599,9 @@ mod tests { use std::time::{Duration, Instant}; // After the last completed response, exit if no new connection arrives within this window. - const IDLE_SHUTDOWN: Duration = Duration::from_millis(500); + // Keep the mock server alive long enough for slower CI/macOS test environments to finish + // discovery + JWKS requests without racing the shutdown timer. + const IDLE_SHUTDOWN: Duration = Duration::from_secs(1); const ABSOLUTE_CAP: Duration = Duration::from_secs(5); let listener = TcpListener::bind("127.0.0.1:0").unwrap(); @@ -1701,6 +1703,17 @@ mod tests { err.contains(base) && err.contains(&format!("{base}/")) && err.contains("discovery failed for all issuer variants") } + async fn validate_mocked_oidc_provider_config(config: &OidcProviderConfig) -> Result { + let http_client = ReqwestHttpClient::new()?; + let state = OidcSys::discover_provider(config, &http_client).await?; + + Ok(OidcProviderValidationResult { + issuer: state.metadata.issuer().to_string(), + authorization_endpoint: state.metadata.authorization_endpoint().to_string(), + token_endpoint: state.metadata.token_endpoint().map(ToString::to_string), + }) + } + #[tokio::test] async fn test_validate_oidc_provider_config_retries_with_issuer_candidates() { // Discovery document must advertise the canonical issuer path. The first candidate has no @@ -1709,7 +1722,7 @@ mod tests { let config_url = format!("{base}/application/o/rustfs"); let config = build_mocked_oidc_provider_config("default", &config_url); - let result = validate_oidc_provider_config(&config).await; + let result = validate_mocked_oidc_provider_config(&config).await; let validation_result = result.expect("OIDC provider validation should succeed"); assert_eq!(validation_result.issuer, format!("{base}/application/o/rustfs/")); @@ -1722,7 +1735,7 @@ mod tests { let config_url = format!("{base}/application/o/rustfs"); let config = build_mocked_oidc_provider_config("default", &config_url); - let err = validate_oidc_provider_config(&config) + let err = validate_mocked_oidc_provider_config(&config) .await .expect_err("OIDC provider validation should fail"); assert!(discovery_error_contains_all_variants(&err, &base)); diff --git a/crates/policy/src/policy/policy.rs b/crates/policy/src/policy/policy.rs index 63c6f0fe4..9501a75bc 100644 --- a/crates/policy/src/policy/policy.rs +++ b/crates/policy/src/policy/policy.rs @@ -798,6 +798,139 @@ mod test { Ok(()) } + #[tokio::test] + async fn test_list_bucket_prefix_condition_uses_bucket_resource() -> Result<()> { + let policy = Policy::parse_config( + br#"{ + "Version": "2012-10-17", + "Statement": [ + { + "Effect": "Allow", + "Action": ["s3:ListBucket"], + "Resource": ["arn:aws:s3:::polaris-test-bucket"], + "Condition": { + "StringLike": { + "s3:prefix": [ + "polaris_test/snowflake_catalog/db1/schema/iceberg_table/*" + ] + } + } + } + ] +}"#, + )?; + + let mut conditions = HashMap::new(); + conditions.insert( + "prefix".to_string(), + vec!["polaris_test/snowflake_catalog/db1/schema/iceberg_table/metadata/".to_string()], + ); + let claims = HashMap::new(); + let args = Args { + account: "polaris-session", + groups: &None, + action: Action::S3Action(crate::policy::action::S3Action::ListBucketAction), + bucket: "polaris-test-bucket", + conditions: &conditions, + is_owner: false, + object: "polaris_test/snowflake_catalog/db1/schema/iceberg_table/metadata/", + claims: &claims, + deny_only: false, + }; + + assert!( + policy.is_allowed(&args).await, + "ListBucket should match the bucket resource and apply the prefix through the condition, not by converting the prefix into an object resource" + ); + + Ok(()) + } + + #[tokio::test] + async fn test_list_bucket_versions_prefix_condition_uses_bucket_resource() -> Result<()> { + let policy = Policy::parse_config( + br#"{ + "Version": "2012-10-17", + "Statement": [ + { + "Effect": "Allow", + "Action": ["s3:ListBucketVersions"], + "Resource": ["arn:aws:s3:::polaris-test-bucket"], + "Condition": { + "StringLike": { + "s3:prefix": [ + "polaris_test/snowflake_catalog/db1/schema/iceberg_table/*" + ] + } + } + } + ] +}"#, + )?; + + let mut conditions = HashMap::new(); + conditions.insert( + "prefix".to_string(), + vec!["polaris_test/snowflake_catalog/db1/schema/iceberg_table/metadata/".to_string()], + ); + let claims = HashMap::new(); + let args = Args { + account: "polaris-session", + groups: &None, + action: Action::S3Action(crate::policy::action::S3Action::ListBucketVersionsAction), + bucket: "polaris-test-bucket", + conditions: &conditions, + is_owner: false, + object: "polaris_test/snowflake_catalog/db1/schema/iceberg_table/metadata/", + claims: &claims, + deny_only: false, + }; + + assert!( + policy.is_allowed(&args).await, + "ListBucketVersions should match the bucket resource and apply the prefix through the condition" + ); + + Ok(()) + } + + #[tokio::test] + async fn test_list_bucket_gateway_prefix_uses_object_resource_when_condition_missing() -> Result<()> { + let policy = Policy::parse_config( + br#"{ + "Version": "2012-10-17", + "Statement": [ + { + "Effect": "Allow", + "Action": ["s3:ListBucket"], + "Resource": ["arn:aws:s3:::polaris-test-bucket/home/alice/*"] + } + ] +}"#, + )?; + + let conditions = HashMap::new(); + let claims = HashMap::new(); + let args = Args { + 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/", + claims: &claims, + deny_only: false, + }; + + assert!( + policy.is_allowed(&args).await, + "Gateway ListBucket auth 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#" @@ -1408,6 +1541,52 @@ mod test { assert_eq!(arr[0].as_str().unwrap(), "s3:ListBucket"); } + #[tokio::test] + async fn test_bucket_policy_list_bucket_prefix_condition_uses_bucket_resource() -> 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"], + "Condition": { + "StringLike": { + "s3:prefix": [ + "polaris_test/snowflake_catalog/db1/schema/iceberg_table/*" + ] + } + } + } + ] +}"#, + )?; + + let mut conditions = HashMap::new(); + conditions.insert( + "prefix".to_string(), + vec!["polaris_test/snowflake_catalog/db1/schema/iceberg_table/metadata/".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: "polaris_test/snowflake_catalog/db1/schema/iceberg_table/metadata/", + }; + + assert!( + bucket_policy.is_allowed(&args).await, + "Bucket policy ListBucket should match the bucket resource and apply the prefix through the condition" + ); + + Ok(()) + } + #[tokio::test] async fn test_bucket_policy_deny_with_string_not_equals() -> Result<()> { let data = r#" diff --git a/crates/policy/src/policy/statement.rs b/crates/policy/src/policy/statement.rs index df33f3c3e..e45ff0d07 100644 --- a/crates/policy/src/policy/statement.rs +++ b/crates/policy/src/policy/statement.rs @@ -14,7 +14,7 @@ use super::{ ActionSet, Args, BucketPolicyArgs, Effect, Error as IamError, Functions, ID, Principal, ResourceSet, Validator, - action::Action, + action::{Action, S3Action}, variables::{VariableContext, VariableResolver}, }; use crate::error::{Error, Result}; @@ -56,6 +56,34 @@ 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 { + let bucket_resource_only = matches!( + action, + Action::S3Action( + S3Action::ListBucketAction | S3Action::ListBucketVersionsAction | S3Action::ListBucketMultipartUploadsAction + ) + ) && conditions.contains_key(LIST_BUCKET_PREFIX_CONDITION_KEY); + + let mut resource = String::from(bucket); + if bucket_resource_only || object.is_empty() { + resource.push('/'); + return resource; + } + + if !object.starts_with('/') { + resource.push('/'); + } + resource.push_str(object); + resource +} + impl Statement { fn is_kms(&self) -> bool { for act in self.actions.iter() { @@ -94,16 +122,7 @@ impl Statement { return false; } - let mut resource = String::from(args.bucket); - if !args.object.is_empty() { - if !args.object.starts_with('/') { - resource.push('/'); - } - - resource.push_str(args.object); - } else { - resource.push('/'); - } + let resource = build_resource(&args.action, args.bucket, args.object, args.conditions); if self.is_kms() && (resource == "/" || self.resources.is_empty()) { return true; @@ -230,16 +249,7 @@ impl BPStatement { return false; } - let mut resource = String::from(args.bucket); - if !args.object.is_empty() { - if !args.object.starts_with('/') { - resource.push('/'); - } - - resource.push_str(args.object); - } else { - resource.push('/'); - } + let resource = build_resource(&args.action, args.bucket, args.object, args.conditions); if !self.resources.is_empty() && !self.resources.is_match(&resource, args.conditions).await { return false; diff --git a/rustfs/src/storage/access.rs b/rustfs/src/storage/access.rs index c691442a5..37d6d6d79 100644 --- a/rustfs/src/storage/access.rs +++ b/rustfs/src/storage/access.rs @@ -35,7 +35,7 @@ use s3s::access::{S3Access, S3AccessContext}; use s3s::{S3Error, S3ErrorCode, S3Request, S3Result, dto::*, s3_error}; use std::collections::HashMap; use std::sync::OnceLock; -use url::Url; +use url::{Url, form_urlencoded}; #[derive(Default, Clone, Debug)] pub(crate) struct ReqInfo { @@ -220,6 +220,30 @@ fn action_tag_metric_label(action: &Action) -> &'static str { } } +fn merge_list_bucket_query_conditions(action: Action, query: Option<&str>, conditions: &mut HashMap>) { + if !matches!( + action, + Action::S3Action( + S3Action::ListBucketAction | S3Action::ListBucketVersionsAction | S3Action::ListBucketMultipartUploadsAction + ) + ) { + return; + } + + let Some(query) = query else { + return; + }; + + for (key, value) in form_urlencoded::parse(query.as_bytes()) { + match key.as_ref() { + "prefix" | "delimiter" | "max-keys" => { + conditions.entry(key.into_owned()).or_default().push(value.into_owned()); + } + _ => {} + } + } +} + fn auth_fs() -> &'static FS { static AUTH_FS: OnceLock = OnceLock::new(); AUTH_FS.get_or_init(FS::new) @@ -312,6 +336,7 @@ pub async fn authorize_request(req: &mut S3Request, action: Action) -> S3R let claims = cred.claims.as_ref().unwrap_or(&default_claims); let mut conditions = get_condition_values_with_query(&req.headers, cred, version_id.as_deref(), None, remote_addr, req.uri.query()); + merge_list_bucket_query_conditions(action, req.uri.query(), &mut conditions); let action_args = Args { account: &cred.access_key, @@ -526,6 +551,7 @@ pub async fn authorize_request(req: &mut S3Request, action: Action) -> S3R remote_addr, req.uri.query(), ); + merge_list_bucket_query_conditions(action, req.uri.query(), &mut conditions); let no_groups: Option> = None; let bucket_tag_hint = if !bucket.is_empty() && !object.is_empty() { @@ -1999,6 +2025,46 @@ mod tests { /// Object tag conditions must use keys like ExistingObjectTag/ so that /// bucket policy conditions (e.g. s3:ExistingObjectTag/security) are evaluated correctly. + #[test] + fn test_merge_list_bucket_query_conditions_extracts_supported_keys() { + let mut conditions = HashMap::new(); + merge_list_bucket_query_conditions( + Action::S3Action(S3Action::ListBucketAction), + Some("prefix=photos%2F2024%2F&delimiter=%2F&max-keys=10&encoding-type=url"), + &mut conditions, + ); + + assert_eq!(conditions.get("prefix"), Some(&vec!["photos/2024/".to_string()])); + assert_eq!(conditions.get("delimiter"), Some(&vec!["/".to_string()])); + assert_eq!(conditions.get("max-keys"), Some(&vec!["10".to_string()])); + assert!(!conditions.contains_key("encoding-type")); + } + + #[test] + fn test_merge_list_bucket_query_conditions_preserves_empty_prefix_signal() { + let mut conditions = HashMap::new(); + merge_list_bucket_query_conditions( + Action::S3Action(S3Action::ListBucketVersionsAction), + Some("prefix=&delimiter=%2F"), + &mut conditions, + ); + + assert_eq!(conditions.get("prefix"), Some(&vec![String::new()])); + assert_eq!(conditions.get("delimiter"), Some(&vec!["/".to_string()])); + } + + #[test] + fn test_merge_list_bucket_query_conditions_ignores_non_list_actions() { + let mut conditions = HashMap::new(); + merge_list_bucket_query_conditions( + Action::S3Action(S3Action::GetObjectAction), + Some("prefix=photos%2F2024%2F&delimiter=%2F&max-keys=10"), + &mut conditions, + ); + + assert!(conditions.is_empty()); + } + #[test] fn test_object_tag_conditions_key_format() { let mut tags = HashMap::new();