From b73059dcf2f499d956431aeeac72ce3aaa945ebc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=AE=89=E6=AD=A3=E8=B6=85?= Date: Thu, 5 Mar 2026 15:47:21 +0800 Subject: [PATCH] fix(admin): allow non-consoleAdmin self password update (#2082) --- crates/policy/src/policy/policy.rs | 56 +++++++++++++------- rustfs/src/admin/handlers/user.rs | 84 ++++++++++++++++++++++++++---- 2 files changed, 113 insertions(+), 27 deletions(-) diff --git a/crates/policy/src/policy/policy.rs b/crates/policy/src/policy/policy.rs index 01f786cc8..e7ed0c993 100644 --- a/crates/policy/src/policy/policy.rs +++ b/crates/policy/src/policy/policy.rs @@ -70,13 +70,15 @@ impl Policy { } } - // Owner has all permissions - if args.is_owner { + // DenyOnly mode: only validate explicit Deny statements. + // If no Deny matched above, allow the request. + if args.deny_only { return true; } - if args.deny_only { - return false; + // Owner has all permissions + if args.is_owner { + return true; } // Check Allow statements @@ -603,7 +605,7 @@ mod test { } #[tokio::test] - async fn test_deny_only_security_fix() -> Result<()> { + async fn test_deny_only_checks_only_deny_statements() -> Result<()> { let data = r#" { "Version": "2012-10-17", @@ -621,7 +623,8 @@ mod test { let conditions = HashMap::new(); let claims = HashMap::new(); - // Test with deny_only=true but no matching Allow statement + // deny_only=true should allow if no Deny statement matches, + // even when no Allow statement matches. let args_deny_only = Args { account: "testuser", groups: &None, @@ -631,16 +634,15 @@ mod test { is_owner: false, object: "test.txt", claims: &claims, - deny_only: true, // Should NOT automatically allow + deny_only: true, }; - // Should return false because deny_only=true, regardless of whether there's a matching Allow statement assert!( - !policy.is_allowed(&args_deny_only).await, - "deny_only should return false when deny_only=true, regardless of Allow statements" + policy.is_allowed(&args_deny_only).await, + "deny_only should allow when no Deny statement matches" ); - // Test with deny_only=true and matching Allow statement + // deny_only=true should also allow when action matches Allow statement. let args_deny_only_allowed = Args { account: "testuser", groups: &None, @@ -653,13 +655,12 @@ mod test { deny_only: true, }; - // Should return false because deny_only=true prevents checking Allow statements (unless is_owner=true) assert!( - !policy.is_allowed(&args_deny_only_allowed).await, - "deny_only should return false even with matching Allow statement" + policy.is_allowed(&args_deny_only_allowed).await, + "deny_only should allow when no Deny statement matches, even if Allow exists" ); - // Test with deny_only=false (normal case) + // Normal policy evaluation remains unchanged when deny_only=false. let args_normal = Args { account: "testuser", groups: &None, @@ -678,21 +679,40 @@ mod test { "normal policy evaluation should allow with matching Allow statement" ); + // Explicit Deny must still block request in deny_only mode. + let deny_policy_data = r#" +{ + "Version": "2012-10-17", + "Statement": [ + { + "Effect": "Deny", + "Action": ["s3:PutObject"], + "Resource": ["arn:aws:s3:::bucket2/*"] + } + ] +} +"#; + let deny_policy = Policy::parse_config(deny_policy_data.as_bytes())?; + assert!( + !deny_policy.is_allowed(&args_deny_only).await, + "deny_only should reject request when an explicit Deny matches" + ); + let args_owner_deny_only = Args { account: "testuser", groups: &None, action: Action::S3Action(crate::policy::action::S3Action::PutObjectAction), bucket: "bucket2", conditions: &conditions, - is_owner: true, // Owner has all permissions + is_owner: true, object: "test.txt", claims: &claims, - deny_only: true, // Even with deny_only=true, owner should be allowed + deny_only: true, }; assert!( policy.is_allowed(&args_owner_deny_only).await, - "owner should retain all permissions even when deny_only=true" + "deny_only should allow when no Deny statement matches, including owner requests" ); Ok(()) diff --git a/rustfs/src/admin/handlers/user.rs b/rustfs/src/admin/handlers/user.rs index 1bab138c0..a9b5183fc 100644 --- a/rustfs/src/admin/handlers/user.rs +++ b/rustfs/src/admin/handlers/user.rs @@ -25,7 +25,7 @@ use crate::{ use http::{HeaderMap, StatusCode}; use matchit::Params; use rustfs_config::{MAX_ADMIN_REQUEST_BODY_SIZE, MAX_IAM_IMPORT_SIZE}; -use rustfs_credentials::get_global_action_cred; +use rustfs_credentials::{Credentials, get_global_action_cred}; use rustfs_iam::{ store::{GroupInfo, MappedPolicy, UserType}, sys::NewServiceAccountOpts, @@ -66,6 +66,13 @@ pub fn register_user_route(r: &mut S3Router) -> std::io::Result< Ok(()) } +fn should_check_deny_only(target_access_key: &str, requester: &Credentials) -> bool { + target_access_key == requester.access_key + && requester.parent_user.is_empty() + && !requester.is_temp() + && !requester.is_service_account() +} + pub struct AddUser {} #[async_trait::async_trait] impl Operation for AddUser { @@ -134,14 +141,14 @@ impl Operation for AddUser { return Err(s3_error!(InvalidArgument, "access key is not utf8")); } - // Security fix: Always require explicit Allow permission for CreateUser - // Do not use deny_only to bypass permission checks, even when creating for self - // This ensures consistent security semantics and prevents privilege escalation + let check_deny_only = should_check_deny_only(ak, &cred); + + // For eligible self operations, only explicit Deny should block the request. validate_admin_request( &req.headers, &cred, owner, - false, // Always require explicit Allow permission + check_deny_only, vec![Action::AdminAction(AdminAction::CreateUserAdminAction)], req.extensions.get::>().and_then(|opt| opt.map(|a| a.0)), ) @@ -393,14 +400,14 @@ impl Operation for GetUserInfo { let (cred, owner) = check_key_valid(get_session_token(&req.uri, &req.headers).unwrap_or_default(), &input_cred.access_key).await?; - // Security fix: Always require explicit Allow permission for GetUser - // Users should have explicit GetUser permission to view account information - // This ensures consistent security semantics across all admin operations + let check_deny_only = should_check_deny_only(ak, &cred); + + // For eligible self operations, only explicit Deny should block the request. validate_admin_request( &req.headers, &cred, owner, - false, // Always require explicit Allow permission + check_deny_only, vec![Action::AdminAction(AdminAction::GetUserAdminAction)], req.extensions.get::>().and_then(|opt| opt.map(|a| a.0)), ) @@ -1054,3 +1061,62 @@ impl Operation for ImportIam { Ok(S3Response::with_headers((StatusCode::OK, Body::from(body)), header)) } } + +#[cfg(test)] +mod tests { + use super::should_check_deny_only; + use rustfs_credentials::{Credentials, IAM_POLICY_CLAIM_NAME_SA}; + use serde_json::Value; + use std::collections::HashMap; + + #[test] + fn test_should_check_deny_only_for_regular_self_request() { + let cred = Credentials { + access_key: "alice".to_string(), + ..Default::default() + }; + assert!(should_check_deny_only("alice", &cred)); + } + + #[test] + fn test_should_not_check_deny_only_for_other_user_request() { + let cred = Credentials { + access_key: "bob".to_string(), + ..Default::default() + }; + assert!(!should_check_deny_only("alice", &cred)); + } + + #[test] + fn test_should_not_check_deny_only_for_temp_credentials() { + let cred = Credentials { + access_key: "alice".to_string(), + session_token: "temp-token".to_string(), + ..Default::default() + }; + assert!(!should_check_deny_only("alice", &cred)); + } + + #[test] + fn test_should_not_check_deny_only_for_service_account_credentials() { + let mut claims = HashMap::new(); + claims.insert(IAM_POLICY_CLAIM_NAME_SA.to_string(), Value::String("policy".to_string())); + let cred = Credentials { + access_key: "alice".to_string(), + parent_user: "parent-user".to_string(), + claims: Some(claims), + ..Default::default() + }; + assert!(!should_check_deny_only("alice", &cred)); + } + + #[test] + fn test_should_not_check_deny_only_when_parent_user_present() { + let cred = Credentials { + access_key: "alice".to_string(), + parent_user: "parent-user".to_string(), + ..Default::default() + }; + assert!(!should_check_deny_only("alice", &cred)); + } +}