From 2eeaca4cd01b746aba977fff00902128425cba9c Mon Sep 17 00:00:00 2001 From: Hauser Date: Wed, 21 Oct 2026 13:35:35 +0800 Subject: [PATCH] fix(iam): derive bucket location conditions from XML (#8744) * fix(iam): derive bucket location conditions from XML Capture the parsed CreateBucket LocationConstraint at the access boundary and move it into shared authorization conditions independently of signing and server regions. Cover conditional allow/deny across user, session and anonymous requests, including absent and spoofed values. Co-Authored-By: heihutu Co-Authored-By: zhi22915 * refactor(iam): pass bucket location directly to authorization Replace temporary request-extension storage with an explicit owned XML argument to the single authorization core, keeping the public entry point compatible. Remove the marker allocation and typemap insert/remove path. Cover Object Lock and Versioning dependent authorizations with an absent location condition and verify their enabled state through real HTTP. Co-Authored-By: heihutu Co-Authored-By: zhi22915 * test(connect): isolate bounded object probe storage Use an independent temporary volume on Linux shared memory when available so the real endpoint/CLI fixture does not depend on contended CI disk IO. Await server shutdown before dropping the caller-owned storage directory. Retain the production cleanup deadline, measurement budget, assertions, and existing nextest capacity reservation without adding retries. Co-Authored-By: heihutu Co-Authored-By: zhi22915 --------- Co-authored-by: heihutu Co-authored-by: zhi22915 --- .config/e2e-full-selection.txt | 4 +- .config/e2e-smoke-selection.txt | 2 +- .../e2e_test/src/create_bucket_region_test.rs | 316 +++++++++++++++++- rustfs/src/storage/access.rs | 127 ++++++- rustfs/tests/connect_perf_object.rs | 11 + 5 files changed, 441 insertions(+), 19 deletions(-) diff --git a/.config/e2e-full-selection.txt b/.config/e2e-full-selection.txt index 8f0642faf..cdb358bed 100644 --- a/.config/e2e-full-selection.txt +++ b/.config/e2e-full-selection.txt @@ -1,2 +1,2 @@ -sha256-darwin=4582837585bf82ad073d7be7f572755e39d7933f015170f5b58872919b63a528 -sha256-linux=0ffd624e8a62a8818f0062eb85855f7dbc39ad07254f7146585148c121d86aba +sha256-darwin=f801018455712d8767ff6d467f3883036f762936f75435a22de74317274bf90d +sha256-linux=1621c50e25d63f124b411babbf613c04474c5e68864d2e810e50ec892379e91e diff --git a/.config/e2e-smoke-selection.txt b/.config/e2e-smoke-selection.txt index 2692eeb26..70834f420 100644 --- a/.config/e2e-smoke-selection.txt +++ b/.config/e2e-smoke-selection.txt @@ -1 +1 @@ -sha256=84db060be346066398f0bcd580122d5e0059176bec4044af46d947b31f876606 +sha256=eaa140bb7c7fff74c92a0cd191805f7fdee3174eaa6a649a1b807e1e6b497a31 diff --git a/crates/e2e_test/src/create_bucket_region_test.rs b/crates/e2e_test/src/create_bucket_region_test.rs index 8c1a75055..0514b9052 100644 --- a/crates/e2e_test/src/create_bucket_region_test.rs +++ b/crates/e2e_test/src/create_bucket_region_test.rs @@ -23,7 +23,10 @@ #[cfg(test)] mod tests { - use crate::common::{RustFSTestEnvironment, init_logging}; + use crate::common::{ + AdminTransport, RustFSTestEnvironment, admin_add_canned_policy_via, admin_attach_user_policy_via, admin_create_user, + build_test_s3_config, build_test_sts_client, init_logging, local_http_client, signed_s3_request_with_headers, + }; use aws_sdk_s3::Client; use aws_sdk_s3::config::Region; use aws_sdk_s3::error::ProvideErrorMetadata; @@ -194,4 +197,315 @@ mod tests { } Ok(()) } + + async fn location_policy_client( + env: &RustFSTestEnvironment, + user: &str, + statements: serde_json::Value, + ) -> Result> { + let secret = "location-policy-test-secret"; + admin_create_user(env, user, secret).await?; + let policy = serde_json::json!({"Version":"2012-10-17", "Statement":statements}); + admin_add_canned_policy_via( + AdminTransport::Signed, + &env.url, + &env.access_key, + &env.secret_key, + user, + &policy.to_string(), + ) + .await?; + admin_attach_user_policy_via(AdminTransport::Signed, &env.url, &env.access_key, &env.secret_key, user, user).await?; + Ok(Client::from_conf( + build_test_s3_config(&env.url, user, secret, None, "location-policy") + .to_builder() + .region(Region::new("eu-west-1")) + .build(), + )) + } + + #[tokio::test] + async fn test_create_bucket_location_constraint_policy_allow() -> Result<(), Box> { + init_logging(); + let mut env = RustFSTestEnvironment::new().await?; + env.start_rustfs_server(vec!["--region", "eu-west-1"]).await?; + let root = Client::from_conf( + env.create_s3_client() + .config() + .to_builder() + .region(Region::new("eu-west-1")) + .build(), + ); + let configuration = |region: &str| { + CreateBucketConfiguration::builder() + .location_constraint(BucketLocationConstraint::from(region)) + .build() + }; + + for (user, condition, accepts_region, accepts_alias, accepts_missing) in [ + ( + "location-allow", + serde_json::json!({"StringEquals":{"s3:LocationConstraint":"eu-west-1"}}), + true, + false, + false, + ), + ( + "location-alias", + serde_json::json!({"StringEquals":{"s3:LocationConstraint":"EU"}}), + false, + true, + false, + ), + ( + "location-null", + serde_json::json!({"Null":{"s3:LocationConstraint":"true"}}), + false, + false, + true, + ), + ] { + let client = location_policy_client( + &env, + user, + serde_json::json!([{ + "Effect":"Allow", "Action":"s3:CreateBucket", "Resource":"arn:aws:s3:::location-*", "Condition":condition, + }]), + ) + .await?; + for (index, (config, allowed)) in [ + (Some(configuration("eu-west-1")), accepts_region), + (Some(configuration("EU")), accepts_alias), + (None, accepts_missing), + (Some(CreateBucketConfiguration::builder().build()), accepts_missing), + (Some(configuration("")), accepts_missing), + ] + .into_iter() + .enumerate() + { + let bucket = format!("{user}-{index}"); + let result = client + .create_bucket() + .bucket(&bucket) + .set_create_bucket_configuration(config) + .send() + .await; + if allowed { + result.unwrap_or_else(|error| panic!("{bucket} should be allowed: {error:?}")); + root.head_bucket().bucket(&bucket).send().await?; + } else { + let denied = result.expect_err("the requested XML constraint must satisfy the conditional Allow"); + assert_eq!(denied.raw_response().map(|response| response.status().as_u16()), Some(403), "{bucket}"); + assert_eq!( + denied.as_service_error().and_then(ProvideErrorMetadata::code), + Some("AccessDenied"), + "{bucket}" + ); + let absent = root + .head_bucket() + .bucket(&bucket) + .send() + .await + .expect_err("denied bucket must not be created"); + assert_eq!(absent.raw_response().map(|response| response.status().as_u16()), Some(404)); + } + } + } + let object_lock_client = location_policy_client( + &env, + "location-lock", + serde_json::json!([ + {"Effect":"Allow", "Action":"s3:CreateBucket", "Resource":"arn:aws:s3:::location-*", + "Condition":{"StringEquals":{"s3:LocationConstraint":"eu-west-1"}}}, + {"Effect":"Allow", "Action":["s3:PutBucketObjectLockConfiguration", "s3:PutBucketVersioning"], + "Resource":"arn:aws:s3:::location-*", "Condition":{"Null":{"s3:LocationConstraint":"true"}}} + ]), + ) + .await?; + object_lock_client + .create_bucket() + .bucket("location-with-lock") + .create_bucket_configuration(configuration("eu-west-1")) + .object_lock_enabled_for_bucket(true) + .send() + .await + .expect("only CreateBucket receives the XML condition; dependent actions see it absent"); + let object_lock = root + .get_object_lock_configuration() + .bucket("location-with-lock") + .send() + .await?; + assert_eq!( + object_lock + .object_lock_configuration() + .and_then(|configuration| configuration.object_lock_enabled()) + .map(|value| value.as_str()), + Some("Enabled") + ); + let versioning = root.get_bucket_versioning().bucket("location-with-lock").send().await?; + assert_eq!(versioning.status().map(|value| value.as_str()), Some("Enabled")); + + let user = "location-session"; + location_policy_client( + &env, + user, + serde_json::json!([{ + "Effect":"Allow", "Action":"s3:CreateBucket", "Resource":"arn:aws:s3:::location-*" + }]), + ) + .await?; + let session_policy = serde_json::json!({"Version":"2012-10-17", "Statement":[ + {"Effect":"Allow", "Action":"s3:CreateBucket", "Resource":"arn:aws:s3:::location-*"}, + {"Effect":"Deny", "Action":"s3:CreateBucket", "Resource":"arn:aws:s3:::location-*", + "Condition":{"StringEquals":{"s3:LocationConstraint":"eu-west-1"}}} + ]}); + let assumed = build_test_sts_client(&env.url, user, "location-policy-test-secret", None, "location-policy-sts") + .assume_role() + .role_arn("arn:aws:iam::123456789012:role/location-policy") + .role_session_name("location-policy-session") + .policy(session_policy.to_string()) + .send() + .await?; + let credentials = assumed.credentials().expect("AssumeRole returns temporary credentials"); + let session = Client::from_conf( + build_test_s3_config( + &env.url, + credentials.access_key_id(), + credentials.secret_access_key(), + Some(credentials.session_token()), + "location-policy-session", + ) + .to_builder() + .region(Region::new("eu-west-1")) + .build(), + ); + let denied = session + .create_bucket() + .bucket("location-session-denied") + .create_bucket_configuration(configuration("eu-west-1")) + .send() + .await + .expect_err("the STS session's conditional Deny must be enforced"); + assert_eq!(denied.as_service_error().and_then(ProvideErrorMetadata::code), Some("AccessDenied")); + let absent = root + .head_bucket() + .bucket("location-session-denied") + .send() + .await + .expect_err("session denial must not create a bucket"); + assert_eq!(absent.raw_response().map(|response| response.status().as_u16()), Some(404)); + session.create_bucket().bucket("location-session-allowed").send().await?; + Ok(()) + } + + #[tokio::test] + async fn test_create_bucket_location_constraint_policy_deny() -> Result<(), Box> { + init_logging(); + let mut env = RustFSTestEnvironment::new().await?; + env.start_rustfs_server(vec!["--region", "eu-west-1"]).await?; + let root = Client::from_conf( + env.create_s3_client() + .config() + .to_builder() + .region(Region::new("eu-west-1")) + .build(), + ); + let user = "location-deny"; + let client = location_policy_client( + &env, + user, + serde_json::json!([ + {"Effect":"Allow", "Action":"s3:CreateBucket", "Resource":"arn:aws:s3:::location-*"}, + {"Effect":"Deny", "Action":"s3:CreateBucket", "Resource":"arn:aws:s3:::location-*", + "Condition":{"StringEquals":{"s3:LocationConstraint":"eu-west-1"}}}, + ]), + ) + .await?; + let denied = client + .create_bucket() + .bucket("location-deny-matching") + .create_bucket_configuration( + CreateBucketConfiguration::builder() + .location_constraint(BucketLocationConstraint::from("eu-west-1")) + .build(), + ) + .send() + .await + .expect_err("a matching explicit Deny must override the unconditional Allow"); + assert_eq!(denied.as_service_error().and_then(ProvideErrorMetadata::code), Some("AccessDenied")); + assert_eq!(denied.raw_response().map(|response| response.status().as_u16()), Some(403)); + let absent = root + .head_bucket() + .bucket("location-deny-matching") + .send() + .await + .expect_err("denied bucket must not exist"); + assert_eq!(absent.raw_response().map(|response| response.status().as_u16()), Some(404)); + client.create_bucket().bucket("location-deny-missing").send().await?; + + let xml = r#"eu-west-1"#; + let mut headers = http::HeaderMap::new(); + headers.insert("locationconstraint", http::HeaderValue::from_static("us-east-1")); + for (index, body) in [xml.to_owned(), xml.replace("eu-west-1", "eu-west-1")] + .into_iter() + .enumerate() + { + let bucket = format!("location-deny-spoofed-{index}"); + let spoofed = signed_s3_request_with_headers( + http::Method::PUT, + &format!("{}/{bucket}?LocationConstraint=us-east-1", env.url), + Some(body), + Some("application/xml"), + user, + "location-policy-test-secret", + &headers, + ) + .await?; + assert_eq!(spoofed.status(), reqwest::StatusCode::FORBIDDEN); + assert!(spoofed.text().await?.contains("AccessDenied")); + let absent = root + .head_bucket() + .bucket(&bucket) + .send() + .await + .expect_err("spoofed conditions must not create a bucket"); + assert_eq!(absent.raw_response().map(|response| response.status().as_u16()), Some(404)); + } + + let bucket = "location-anonymous"; + root.create_bucket().bucket(bucket).send().await?; + for (effect, match_status, missing_status) in [ + ("Allow", reqwest::StatusCode::CONFLICT, reqwest::StatusCode::FORBIDDEN), + ("Deny", reqwest::StatusCode::FORBIDDEN, reqwest::StatusCode::CONFLICT), + ] { + let mut statements = vec![]; + if effect == "Deny" { + statements.push(serde_json::json!({"Effect":"Allow", "Principal":"*", "Action":"s3:CreateBucket", "Resource":format!("arn:aws:s3:::{bucket}")})); + } + statements.push(serde_json::json!({"Effect":effect, "Principal":"*", "Action":"s3:CreateBucket", "Resource":format!("arn:aws:s3:::{bucket}"), + "Condition":{"StringEquals":{"s3:LocationConstraint":"eu-west-1"}}})); + root.put_bucket_policy() + .bucket(bucket) + .policy(serde_json::json!({"Version":"2012-10-17", "Statement":statements}).to_string()) + .send() + .await?; + let unsigned = local_http_client(); + for (body, expected) in [(xml, match_status), ("", missing_status)] { + let response = unsigned + .put(format!("{}/{bucket}", env.url)) + .header("content-type", "application/xml") + .body(body) + .send() + .await?; + assert_eq!(response.status(), expected, "anonymous {effect}, XML present={}", !body.is_empty()); + let response = response.text().await?; + assert!(response.contains(if expected == reqwest::StatusCode::FORBIDDEN { + "AccessDenied" + } else { + "BucketAlreadyExists" + })); + } + } + Ok(()) + } } diff --git a/rustfs/src/storage/access.rs b/rustfs/src/storage/access.rs index c38b44c47..270c49635 100644 --- a/rustfs/src/storage/access.rs +++ b/rustfs/src/storage/access.rs @@ -884,7 +884,7 @@ fn authorization_conditions( req: &S3Request, cred: &rustfs_credentials::Credentials, version_id: Option<&str>, - region: Option, + location_constraint: Option, remote_addr: Option, client_info: Option<&ClientInfo>, action: Action, @@ -893,7 +893,7 @@ fn authorization_conditions( &req.headers, cred, version_id, - region, + None, remote_addr, req.uri.query(), client_info, @@ -909,6 +909,11 @@ fn authorization_conditions( if internal_object_authorization { return Ok(conditions); } + if action == Action::S3Action(S3Action::CreateBucketAction) + && let Some(location) = location_constraint + { + conditions.insert("LocationConstraint".to_string(), vec![location]); + } merge_list_bucket_query_conditions(action, req.uri.query(), &mut conditions); merge_request_object_tag_conditions(action, &req.headers, &mut conditions)?; if let Some(RequestedObjectLockRetention(retention)) = req.extensions.get::() { @@ -949,6 +954,17 @@ pub(crate) fn set_requested_object_lock_retain_until(req: &mut S3Request, } } +/// Keep the XML value literal for IAM; endpoint aliases are resolved later. +fn requested_bucket_location_constraint(input: &CreateBucketInput) -> Option { + input + .create_bucket_configuration + .as_ref() + .and_then(|configuration| configuration.location_constraint.as_ref()) + .map(BucketLocationConstraint::as_str) + .filter(|location| !location.is_empty()) + .map(str::to_owned) +} + fn set_requested_object_lock_retention(req: &mut S3Request) { let retention = req.input.retention.clone().unwrap_or_default(); set_requested_object_lock_retain_until(req, retention.retain_until_date.as_ref()); @@ -1305,6 +1321,14 @@ impl DenialContext<'_> { /// Authorizes the request based on the action and credentials. pub async fn authorize_request(req: &mut S3Request, action: Action) -> S3Result<()> { + authorize_request_with_location_constraint(req, action, None).await +} + +async fn authorize_request_with_location_constraint( + req: &mut S3Request, + action: Action, + location_constraint: Option, +) -> S3Result<()> { let internal_object_authorization = req.extensions.get::().is_some(); let remote_addr = req.extensions.get::>().and_then(|opt| opt.map(|a| a.0)); let req_info = req_info_ref(req)?; @@ -1330,7 +1354,8 @@ pub async fn authorize_request(req: &mut S3Request, action: Action) -> S3R let default_claims = HashMap::new(); let claims = cred.claims.as_ref().unwrap_or(&default_claims); let client_info = req.extensions.get::(); - let mut conditions = authorization_conditions(req, cred, version_id.as_deref(), None, remote_addr, client_info, action)?; + let mut conditions = + authorization_conditions(req, cred, version_id.as_deref(), location_constraint, remote_addr, client_info, action)?; let action_args = Args { account: &cred.access_key, @@ -1506,7 +1531,7 @@ pub async fn authorize_request(req: &mut S3Request, action: Action) -> S3R req, &default_cred, version_id.as_deref(), - req.region.clone(), + location_constraint, remote_addr, client_info, action, @@ -1930,15 +1955,8 @@ async fn prepare_table_data_plane_list_access(req: &mut S3Request, bucket: let client_info = req.extensions.get::(); let default_cred = rustfs_credentials::Credentials::default(); let condition_cred = cred.as_ref().unwrap_or(&default_cred); - let conditions = authorization_conditions( - req, - condition_cred, - None, - req.region.clone(), - remote_addr, - client_info, - Action::S3Action(action), - )?; + let conditions = + authorization_conditions(req, condition_cred, None, None, remote_addr, client_info, Action::S3Action(action))?; let iam_store = cred.as_ref().map(|_| request_iam_store(req)).transpose()?; let store = table_catalog_store_for_data_plane(req)?; let tables = crate::table_catalog::TableCatalogStore::list_all_tables(&store, bucket) @@ -2292,7 +2310,9 @@ impl S3Access for FS { let req_info = ext_req_info_mut(&mut req.extensions)?; req_info.bucket = Some(req.input.bucket.clone()); - authorize_request(req, Action::S3Action(S3Action::CreateBucketAction)).await?; + let location_constraint = requested_bucket_location_constraint(&req.input); + authorize_request_with_location_constraint(req, Action::S3Action(S3Action::CreateBucketAction), location_constraint) + .await?; if req.input.object_lock_enabled_for_bucket.is_some_and(|v| v) { authorize_request(req, Action::S3Action(S3Action::PutBucketObjectLockConfigurationAction)).await?; @@ -3554,7 +3574,10 @@ mod tests { table_data_plane_content_mutation, table_data_plane_resource_for_request, table_publication_guard_error, validate_post_object_success_controls, versioned_read_action, versioned_tagging_action, }; - use super::{remaining_retention_days, set_requested_object_lock_retain_until, set_requested_object_lock_retention}; + use super::{ + remaining_retention_days, requested_bucket_location_constraint, set_requested_object_lock_retain_until, + set_requested_object_lock_retention, + }; use crate::auth::OBJECT_LOCK_REMAINING_RETENTION_DAYS_CONDITION; use crate::error::ApiError; use crate::storage::storage_api::contract::bucket::{BucketOperations as _, DeleteBucketOptions, MakeBucketOptions}; @@ -4595,6 +4618,80 @@ mod tests { } } + #[test] + fn create_bucket_location_constraint_conditions_use_only_xml() { + let configuration = |region: &str| CreateBucketConfiguration { + location_constraint: Some(BucketLocationConstraint::from(region.to_owned())), + ..Default::default() + }; + for (configuration, expected) in [ + (None, None), + (Some(CreateBucketConfiguration::default()), None), + (Some(configuration("")), None), + (Some(configuration("eu-west-1")), Some("eu-west-1")), + (Some(configuration("EU")), Some("EU")), + (Some(configuration("tenant-region-1")), Some("tenant-region-1")), + ] { + let mut req = build_request( + CreateBucketInput { + bucket: "location-policy-bucket".to_string(), + create_bucket_configuration: configuration, + ..Default::default() + }, + Method::PUT, + ); + req.region = Some("us-east-1".parse().expect("signing region")); + req.uri = Uri::from_static("/location-policy-bucket?LocationConstraint=forged&s3%3ALocationConstraint=forged"); + req.headers.insert("locationconstraint", HeaderValue::from_static("forged")); + let credentials = rustfs_credentials::Credentials { + claims: Some( + [ + ("LocationConstraint".to_string(), serde_json::json!("forged")), + ("ldapLocationConstraint".to_string(), serde_json::json!("forged")), + ] + .into_iter() + .collect(), + ), + ..Default::default() + }; + let location = requested_bucket_location_constraint(&req.input); + let conditions = authorization_conditions( + &req, + &credentials, + None, + location, + None, + None, + Action::S3Action(S3Action::CreateBucketAction), + ) + .expect("CreateBucket conditions"); + assert_eq!( + conditions + .get("LocationConstraint") + .and_then(|values| values.first()) + .map(String::as_str), + expected, + ); + for action in [ + S3Action::PutBucketObjectLockConfigurationAction, + S3Action::PutBucketVersioningAction, + S3Action::GetObjectAction, + ] { + let conditions = authorization_conditions( + &req, + &credentials, + None, + expected.map(str::to_owned), + None, + None, + Action::S3Action(action), + ) + .expect("non-CreateBucket conditions"); + assert!(!conditions.contains_key("LocationConstraint"), "{action:?}"); + } + } + } + fn remaining_retention_days_conditions(req: &S3Request, action: Action) -> HashMap> { let credentials = rustfs_credentials::Credentials::default(); authorization_conditions(req, &credentials, None, None, None, None, action).expect("conditions should build") diff --git a/rustfs/tests/connect_perf_object.rs b/rustfs/tests/connect_perf_object.rs index 292d74634..895df2d63 100644 --- a/rustfs/tests/connect_perf_object.rs +++ b/rustfs/tests/connect_perf_object.rs @@ -831,8 +831,17 @@ async fn real_rustfs_endpoint_and_production_cli_support_bounded_get_and_put_bod Err(err) if err.kind() == std::io::ErrorKind::PermissionDenied => return, Err(err) => panic!("find free port: {err}"), }; + // This functional probe keeps production deadlines while avoiding the + // shared CI disk's fsync latency when Linux shared memory is available. + let storage = if cfg!(target_os = "linux") && Path::new("/dev/shm").is_dir() { + tempfile::tempdir_in("/dev/shm") + } else { + tempfile::tempdir() + } + .expect("isolated object fixture storage"); let server = RustFSServerBuilder::new() .address(format!("127.0.0.1:{port}")) + .volume(storage.path().to_str().expect("object fixture storage path")) .access_key("object-perf-access") .secret_key("object-perf-secret") .build() @@ -968,6 +977,8 @@ async fn real_rustfs_endpoint_and_production_cli_support_bounded_get_and_put_bod .expect("public key") .verify(&signed, &signature) .expect("valid ES256 signature"); + + server.shutdown().await; } fn write_credential(path: &Path, value: &str) {