gsadmin/rustfs
1
0
Fork 0
mirror of https://github.com/rustfs/rustfs.git synced 2026-08-22 04:16:38 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(admin): pass the real RemoteAddr so aws:SourceIp reaches policy evaluation (#6273)

Browse Source 
backlog#1885. Six admin call sites hardcoded `None` for `validate_admin_request`'s `remote_addr`, so `aws:SourceIp` never entered the condition map for those endpoints.

`AddrFunc::evaluate` (crates/policy/src/policy/function/addr.rs:23-41) reads the key with `values.get(...)`; an absent key yields an empty iterator, the inner loop never runs, and the function returns `false`. That flips two policy shapes in opposite directions:

- `Allow` + an IpAddress whitelist stops matching, locking a legitimate admin out of these endpoints.
- `Deny` + an IpAddress blacklist also stops matching, so a source the policy means to block is let through. This one is a bypass, and it is the one nobody would report.

The sites now read the address the way the correct handlers do — `req.extensions.get::<Option<RemoteAddr>>()`, populated from the connection in `server/http.rs`.

A regression test covers both shapes at the policy layer, since that is where the direction is decided. The existing `test_iam_policy_source_ip` only exercised a present key matching or not matching; nothing covered an absent one.

A tree-wide sweep of all 88 `validate_admin_request*` call sites confirms these six were the only ones dropping the address. The issue asked whether more existed beyond the six it had found: they do not. Worth noting that a first pass checked the last argument and reported only three — `_with_bucket` takes `remote_addr` second-to-last — so the sweep is positional.

One caveat for operators, unchanged by this fix: admin authorization does not route the peer address through `crates/trusted-proxies`, so behind a reverse proxy these conditions match the proxy's address, not the client's.

Refs backlog#1885
This commit is contained in:
Zhengchao An
2026-08-19 23:15:34 +08:00
committed by GitHub
parent 1f23fd17b6
commit cc0254d8de
3 changed files with 97 additions and 6 deletions
Download Patch File Download Diff File Expand all files Collapse all files
rustfs/src/admin/handlers/durability.rs
+3 -1
View File
@@ -28,6 +28,7 @@ use crate::admin::storage_api::bucket::metadata::BUCKET_DURABILITY_CONFIG;
use crate::admin::storage_api::bucket::metadata_sys; use crate::admin::storage_api::bucket::metadata_sys;
use crate::auth::{check_key_valid, get_session_token}; use crate::auth::{check_key_valid, get_session_token};
use crate::server::ADMIN_PREFIX; use crate::server::ADMIN_PREFIX;
use crate::server::RemoteAddr;
use hyper::{Method, StatusCode}; use hyper::{Method, StatusCode};
use matchit::Params; use matchit::Params;
use rustfs_policy::policy::action::{Action, AdminAction}; use rustfs_policy::policy::action::{Action, AdminAction};
@@ -126,13 +127,14 @@ async fn authenticate_admin(req: &S3Request<Body>) -> S3Result<()> {
let (cred, owner) = check_key_valid(get_session_token(&req.uri, &req.headers).unwrap_or_default(), &cred.access_key).await?; 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::<Option<RemoteAddr>>().and_then(|opt| opt.map(|a| a.0));
validate_admin_request( validate_admin_request(
&req.headers, &req.headers,
&cred, &cred,
owner, owner,
false, false,
vec![Action::AdminAction(AdminAction::ConfigUpdateAdminAction)], vec![Action::AdminAction(AdminAction::ConfigUpdateAdminAction)],
None, remote_addr,
) )
.await?; .await?;
rustfs/src/admin/handlers/quota.rs
+11 -5
View File
@@ -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::auth::{check_key_valid, get_session_token};
use crate::error::ApiError; use crate::error::ApiError;
use crate::server::ADMIN_PREFIX; use crate::server::ADMIN_PREFIX;
use crate::server::RemoteAddr;
use hyper::{Method, StatusCode}; use hyper::{Method, StatusCode};
use matchit::Params; use matchit::Params;
use rustfs_madmin::{SITE_REPL_API_VERSION, SRBucketMeta}; use rustfs_madmin::{SITE_REPL_API_VERSION, SRBucketMeta};
@@ -264,13 +265,14 @@ impl Operation for SetBucketQuotaHandler {
let (cred, owner) = let (cred, owner) =
check_key_valid(get_session_token(&req.uri, &req.headers).unwrap_or_default(), &cred.access_key).await?; check_key_valid(get_session_token(&req.uri, &req.headers).unwrap_or_default(), &cred.access_key).await?;
let remote_addr = req.extensions.get::<Option<RemoteAddr>>().and_then(|opt| opt.map(|a| a.0));
validate_admin_request( validate_admin_request(
&req.headers, &req.headers,
&cred, &cred,
owner, owner,
false, false,
vec![Action::AdminAction(AdminAction::SetBucketQuotaAdminAction)], vec![Action::AdminAction(AdminAction::SetBucketQuotaAdminAction)],
None, remote_addr,
) )
.await?; .await?;
@@ -393,13 +395,14 @@ impl Operation for GetBucketQuotaHandler {
if bucket.is_empty() { if bucket.is_empty() {
return Err(s3_error!(InvalidRequest, "bucket name is required")); return Err(s3_error!(InvalidRequest, "bucket name is required"));
} }
let remote_addr = req.extensions.get::<Option<RemoteAddr>>().and_then(|opt| opt.map(|a| a.0));
validate_admin_request_with_bucket( validate_admin_request_with_bucket(
&req.headers, &req.headers,
&cred, &cred,
owner, owner,
false, false,
vec![Action::S3Action(S3Action::GetBucketQuotaAction)], vec![Action::S3Action(S3Action::GetBucketQuotaAction)],
None, remote_addr,
&bucket, &bucket,
) )
.await?; .await?;
@@ -461,13 +464,14 @@ impl Operation for ClearBucketQuotaHandler {
let (cred, owner) = let (cred, owner) =
check_key_valid(get_session_token(&req.uri, &req.headers).unwrap_or_default(), &cred.access_key).await?; check_key_valid(get_session_token(&req.uri, &req.headers).unwrap_or_default(), &cred.access_key).await?;
let remote_addr = req.extensions.get::<Option<RemoteAddr>>().and_then(|opt| opt.map(|a| a.0));
validate_admin_request( validate_admin_request(
&req.headers, &req.headers,
&cred, &cred,
owner, owner,
false, false,
vec![Action::AdminAction(AdminAction::SetBucketQuotaAdminAction)], vec![Action::AdminAction(AdminAction::SetBucketQuotaAdminAction)],
None, remote_addr,
) )
.await?; .await?;
@@ -577,13 +581,14 @@ impl Operation for GetBucketQuotaStatsHandler {
return Err(s3_error!(InvalidRequest, "bucket name is required")); return Err(s3_error!(InvalidRequest, "bucket name is required"));
} }
let remote_addr = req.extensions.get::<Option<RemoteAddr>>().and_then(|opt| opt.map(|a| a.0));
validate_admin_request_with_bucket( validate_admin_request_with_bucket(
&req.headers, &req.headers,
&cred, &cred,
owner, owner,
false, false,
vec![Action::S3Action(S3Action::GetBucketQuotaAction)], vec![Action::S3Action(S3Action::GetBucketQuotaAction)],
None, remote_addr,
&bucket, &bucket,
) )
.await?; .await?;
@@ -649,13 +654,14 @@ impl Operation for CheckBucketQuotaHandler {
return Err(s3_error!(InvalidRequest, "bucket name is required")); return Err(s3_error!(InvalidRequest, "bucket name is required"));
} }
let remote_addr = req.extensions.get::<Option<RemoteAddr>>().and_then(|opt| opt.map(|a| a.0));
validate_admin_request_with_bucket( validate_admin_request_with_bucket(
&req.headers, &req.headers,
&cred, &cred,
owner, owner,
false, false,
vec![Action::S3Action(S3Action::GetBucketQuotaAction)], vec![Action::S3Action(S3Action::GetBucketQuotaAction)],
None, remote_addr,
&bucket, &bucket,
) )
.await?; .await?;
rustfs/src/auth.rs
+83
View File
@@ -2210,6 +2210,89 @@ mod tests_policy {
assert!(!policy.is_allowed(&args_fail).await, "IAM Policy should deny non-matching IP"); 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<String, Vec<String>> = 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] #[tokio::test]
async fn test_bucket_policy_source_ip() { async fn test_bucket_policy_source_ip() {
let policy_json = r#"{ let policy_json = r#"{