diff --git a/rustfs/src/admin/handlers/durability.rs b/rustfs/src/admin/handlers/durability.rs index 3fc92572e..cba9351a1 100644 --- a/rustfs/src/admin/handlers/durability.rs +++ b/rustfs/src/admin/handlers/durability.rs @@ -28,6 +28,7 @@ use crate::admin::storage_api::bucket::metadata::BUCKET_DURABILITY_CONFIG; use crate::admin::storage_api::bucket::metadata_sys; use crate::auth::{check_key_valid, get_session_token}; use crate::server::ADMIN_PREFIX; +use crate::server::RemoteAddr; use hyper::{Method, StatusCode}; use matchit::Params; use rustfs_policy::policy::action::{Action, AdminAction}; @@ -126,13 +127,14 @@ async fn authenticate_admin(req: &S3Request) -> S3Result<()> { let (cred, owner) = check_key_valid(get_session_token(&req.uri, &req.headers).unwrap_or_default(), &cred.access_key).await?; + let remote_addr = req.extensions.get::>().and_then(|opt| opt.map(|a| a.0)); validate_admin_request( &req.headers, &cred, owner, false, vec![Action::AdminAction(AdminAction::ConfigUpdateAdminAction)], - None, + remote_addr, ) .await?; diff --git a/rustfs/src/admin/handlers/quota.rs b/rustfs/src/admin/handlers/quota.rs index 7dc2621be..8f7f672e9 100644 --- a/rustfs/src/admin/handlers/quota.rs +++ b/rustfs/src/admin/handlers/quota.rs @@ -24,6 +24,7 @@ use crate::admin::storage_api::bucket::quota::{BucketQuota, QuotaError, QuotaOpe use crate::auth::{check_key_valid, get_session_token}; use crate::error::ApiError; use crate::server::ADMIN_PREFIX; +use crate::server::RemoteAddr; use hyper::{Method, StatusCode}; use matchit::Params; use rustfs_madmin::{SITE_REPL_API_VERSION, SRBucketMeta}; @@ -264,13 +265,14 @@ impl Operation for SetBucketQuotaHandler { let (cred, owner) = check_key_valid(get_session_token(&req.uri, &req.headers).unwrap_or_default(), &cred.access_key).await?; + let remote_addr = req.extensions.get::>().and_then(|opt| opt.map(|a| a.0)); validate_admin_request( &req.headers, &cred, owner, false, vec![Action::AdminAction(AdminAction::SetBucketQuotaAdminAction)], - None, + remote_addr, ) .await?; @@ -393,13 +395,14 @@ impl Operation for GetBucketQuotaHandler { if bucket.is_empty() { return Err(s3_error!(InvalidRequest, "bucket name is required")); } + let remote_addr = req.extensions.get::>().and_then(|opt| opt.map(|a| a.0)); validate_admin_request_with_bucket( &req.headers, &cred, owner, false, vec![Action::S3Action(S3Action::GetBucketQuotaAction)], - None, + remote_addr, &bucket, ) .await?; @@ -461,13 +464,14 @@ impl Operation for ClearBucketQuotaHandler { let (cred, owner) = check_key_valid(get_session_token(&req.uri, &req.headers).unwrap_or_default(), &cred.access_key).await?; + let remote_addr = req.extensions.get::>().and_then(|opt| opt.map(|a| a.0)); validate_admin_request( &req.headers, &cred, owner, false, vec![Action::AdminAction(AdminAction::SetBucketQuotaAdminAction)], - None, + remote_addr, ) .await?; @@ -577,13 +581,14 @@ impl Operation for GetBucketQuotaStatsHandler { return Err(s3_error!(InvalidRequest, "bucket name is required")); } + let remote_addr = req.extensions.get::>().and_then(|opt| opt.map(|a| a.0)); validate_admin_request_with_bucket( &req.headers, &cred, owner, false, vec![Action::S3Action(S3Action::GetBucketQuotaAction)], - None, + remote_addr, &bucket, ) .await?; @@ -649,13 +654,14 @@ impl Operation for CheckBucketQuotaHandler { return Err(s3_error!(InvalidRequest, "bucket name is required")); } + let remote_addr = req.extensions.get::>().and_then(|opt| opt.map(|a| a.0)); validate_admin_request_with_bucket( &req.headers, &cred, owner, false, vec![Action::S3Action(S3Action::GetBucketQuotaAction)], - None, + remote_addr, &bucket, ) .await?; diff --git a/rustfs/src/auth.rs b/rustfs/src/auth.rs index fa12a564e..5b019de18 100644 --- a/rustfs/src/auth.rs +++ b/rustfs/src/auth.rs @@ -2210,6 +2210,89 @@ mod tests_policy { assert!(!policy.is_allowed(&args_fail).await, "IAM Policy should deny non-matching IP"); } + /// The failure this issue is about: when `remote_addr` is dropped the + /// `aws:SourceIp` key never reaches the condition map, and `AddrFunc::evaluate` + /// returns `false` for an absent key. That flips two policy shapes in + /// opposite directions, and only one of them looks like a failure + /// (rustfs/backlog#1885). + #[tokio::test] + async fn source_ip_policies_break_in_both_directions_when_the_key_is_missing() { + let allow_from_office = |effect: &str| { + format!( + r#"{{ + "Version": "2012-10-17", + "Statement": [ + {{"Effect": "Allow", "Action": ["admin:ConfigUpdate"], "Resource": ["arn:aws:s3:::*"]}}, + {{ + "Effect": "{effect}", + "Action": ["admin:ConfigUpdate"], + "Resource": ["arn:aws:s3:::*"], + "Condition": {{"IpAddress": {{"aws:SourceIp": "192.168.1.0/24"}}}} + }} + ] + }}"# + ) + }; + + let claims = HashMap::new(); + let groups = None; + let mut with_ip = HashMap::new(); + with_ip.insert("SourceIp".to_string(), vec!["192.168.1.10".to_string()]); + let without_ip: HashMap> = HashMap::new(); + + let args_with_ip = Args { + account: "test-account", + groups: &groups, + action: Action::AdminAction(rustfs_policy::policy::action::AdminAction::ConfigUpdateAdminAction), + bucket: "", + conditions: &with_ip, + is_owner: false, + object: "", + claims: &claims, + deny_only: false, + }; + let args_without_ip = Args { + conditions: &without_ip, + ..args_with_ip + }; + + // Deny + blacklist: the bypass shape. With the key present the deny + // matches and the request is refused; drop the key and the deny stops + // matching, so a source that policy means to block gets through. + let deny_policy: Policy = serde_json::from_str(&allow_from_office("Deny")).expect("deny policy parses"); + assert!( + !deny_policy.is_allowed(&args_with_ip).await, + "a blacklisted source must be refused while aws:SourceIp is present" + ); + assert!( + deny_policy.is_allowed(&args_without_ip).await, + "dropping remote_addr makes the Deny statement unreachable — this is the bypass" + ); + + // Allow + whitelist: the availability shape, and the only one an + // operator would notice, which is why the bypass above went unseen. + let allow_policy: Policy = serde_json::from_str( + r#"{ + "Version": "2012-10-17", + "Statement": [{ + "Effect": "Allow", + "Action": ["admin:ConfigUpdate"], + "Resource": ["arn:aws:s3:::*"], + "Condition": {"IpAddress": {"aws:SourceIp": "192.168.1.0/24"}} + }] + }"#, + ) + .expect("allow policy parses"); + assert!( + allow_policy.is_allowed(&args_with_ip).await, + "a whitelisted source must be allowed while aws:SourceIp is present" + ); + assert!( + !allow_policy.is_allowed(&args_without_ip).await, + "dropping remote_addr locks out a legitimate admin" + ); + } + #[tokio::test] async fn test_bucket_policy_source_ip() { let policy_json = r#"{