From 19d3a23a13a3e0496fde3d7bdd64d3742aa57e98 Mon Sep 17 00:00:00 2001 From: GatewayJ <835269233@qq.com> Date: Sat, 21 Mar 2026 22:10:38 +0800 Subject: [PATCH] fix(admin): console self password for STS sessions (#1923) (#2250) Co-authored-by: GatewayJ <8352692332qq.com> --- crates/iam/src/sys.rs | 155 +++++++++++++++++++++++++++--- rustfs/src/admin/auth.rs | 16 ++- rustfs/src/admin/handlers/user.rs | 111 +++++++++++++++++++-- 3 files changed, 261 insertions(+), 21 deletions(-) diff --git a/crates/iam/src/sys.rs b/crates/iam/src/sys.rs index 1fa74e0ed..dac697633 100644 --- a/crates/iam/src/sys.rs +++ b/crates/iam/src/sys.rs @@ -794,6 +794,15 @@ impl IamSys { return combined.is_allowed(args).await; } } + + if args.deny_only { + let combined_policy = Policy::default(); + let (has_session_policy, is_allowed_sp) = is_allowed_by_session_policy(args); + if has_session_policy { + return is_allowed_sp && combined_policy.is_allowed(args).await; + } + return combined_policy.is_allowed(args).await; + } return false; } @@ -803,9 +812,14 @@ impl IamSys { } else { let (a, c) = self.store.merge_policies(&policies.join(",")).await; if a.is_empty() { - return false; + if args.deny_only { + Policy::default() + } else { + return false; + } + } else { + c } - c } }; @@ -1035,17 +1049,20 @@ mod tests { use rustfs_credentials::Credentials; use rustfs_policy::auth::UserIdentity; use rustfs_policy::policy::Args; - use rustfs_policy::policy::action::{Action, S3Action}; + use rustfs_policy::policy::action::{Action, AdminAction, S3Action}; + use serde_json::Value; use std::collections::HashMap; use time::OffsetDateTime; - /// Mock Store that populates cache in load_all so STS temp credentials - /// without args.groups still get group-attached policies via parent user (groups fallback). + /// Mock Store for STS tests: either group-attached policies via parent user, or no IAM policies. #[derive(Clone)] - struct StsGroupsFallbackMockStore; + struct StsTestMockStore { + /// When true, parent user has no groups and no mapped policies (empty `policy_db_get`). + empty_policies: bool, + } #[async_trait::async_trait] - impl Store for StsGroupsFallbackMockStore { + impl Store for StsTestMockStore { fn has_watcher(&self) -> bool { false } @@ -1175,14 +1192,48 @@ mod tests { } async fn load_all(&self, cache: &Cache) -> Result<()> { - const PARENT_USER: &str = "sts-fallback-test-parent"; - const GROUP_NAME: &str = "testgroup"; - let policy_docs = get_default_policyes(); cache .policy_docs .store(Arc::new(CacheEntity::new(policy_docs).update_load_time())); + if self.empty_policies { + const PARENT_USER: &str = "sts-empty-parent-policy-test"; + let creds = Credentials { + access_key: PARENT_USER.to_string(), + secret_key: "longenoughsecret".to_string(), + session_token: String::new(), + expiration: None, + status: "on".to_string(), + parent_user: String::new(), + groups: None, + claims: None, + name: None, + description: None, + }; + let parent_identity = UserIdentity { + version: 1, + credentials: creds, + update_at: Some(OffsetDateTime::now_utc()), + }; + let mut users = HashMap::new(); + users.insert(PARENT_USER.to_string(), parent_identity); + cache.users.store(Arc::new(CacheEntity::new(users).update_load_time())); + + cache.groups.store(Arc::new(CacheEntity::default().update_load_time())); + cache + .group_policies + .store(Arc::new(CacheEntity::default().update_load_time())); + cache.user_policies.store(Arc::new(CacheEntity::default().update_load_time())); + cache.sts_accounts.store(Arc::new(CacheEntity::default().update_load_time())); + cache.sts_policies.store(Arc::new(CacheEntity::default().update_load_time())); + cache.build_user_group_memberships(); + return Ok(()); + } + + const PARENT_USER: &str = "sts-fallback-test-parent"; + const GROUP_NAME: &str = "testgroup"; + let creds = Credentials { access_key: PARENT_USER.to_string(), secret_key: "longenoughsecret".to_string(), @@ -1231,7 +1282,7 @@ mod tests { /// would be denied. #[tokio::test] async fn test_sts_groups_fallback_temp_creds_receive_parent_group_policies() { - let store = StsGroupsFallbackMockStore; + let store = StsTestMockStore { empty_policies: false }; let cache_manager = IamCache::new(store).await; let iam_sys = IamSys::new(cache_manager); @@ -1257,13 +1308,93 @@ mod tests { ); } + /// Regression: `deny_only` with empty IAM policies must still evaluate `sessionPolicy-extracted` + /// so session policy Deny cannot be bypassed (see PR #2250 review). + #[tokio::test] + async fn test_sts_deny_only_session_policy_deny_blocks_when_iam_policies_empty() { + let store = StsTestMockStore { empty_policies: true }; + let cache_manager = IamCache::new(store).await; + let iam_sys = IamSys::new(cache_manager); + + let parent_user = "sts-empty-parent-policy-test"; + let session_policy_json = r#"{ + "Version": "2012-10-17", + "Statement": [ + { + "Effect": "Deny", + "Action": ["admin:CreateUser"], + "Resource": ["arn:aws:s3:::*"] + } + ] +}"#; + let mut claims = HashMap::new(); + claims.insert(SESSION_POLICY_NAME_EXTRACTED.to_string(), Value::String(session_policy_json.to_string())); + let groups: Option> = None; + let args = Args { + account: parent_user, + groups: &groups, + action: Action::AdminAction(AdminAction::CreateUserAdminAction), + bucket: "", + conditions: &HashMap::new(), + is_owner: false, + object: "", + claims: &claims, + deny_only: true, + }; + + let allowed = iam_sys.is_allowed_sts(&args, parent_user).await; + assert!( + !allowed, + "session policy Deny must be evaluated even when IAM policies are empty and deny_only is set" + ); + } + + #[tokio::test] + async fn test_sts_deny_only_session_policy_allow_when_no_deny_on_action() { + let store = StsTestMockStore { empty_policies: true }; + let cache_manager = IamCache::new(store).await; + let iam_sys = IamSys::new(cache_manager); + + let parent_user = "sts-empty-parent-policy-test"; + let session_policy_json = r#"{ + "Version": "2012-10-17", + "Statement": [ + { + "Effect": "Allow", + "Action": ["s3:GetObject"], + "Resource": ["arn:aws:s3:::bucket/*"] + } + ] +}"#; + let mut claims = HashMap::new(); + claims.insert(SESSION_POLICY_NAME_EXTRACTED.to_string(), Value::String(session_policy_json.to_string())); + let groups: Option> = None; + let args = Args { + account: parent_user, + groups: &groups, + action: Action::AdminAction(AdminAction::CreateUserAdminAction), + bucket: "", + conditions: &HashMap::new(), + is_owner: false, + object: "", + claims: &claims, + deny_only: true, + }; + + let allowed = iam_sys.is_allowed_sts(&args, parent_user).await; + assert!( + allowed, + "deny_only with no matching Deny in session policy should still allow self-service-style checks" + ); + } + /// Regression test for cross-node IAM notifications: /// `load_user` must populate user cache, and regular-user mapped policy must be written to /// `user_policies` (not `sts_policies`), otherwise list-users and bucket-scoped user listing /// may miss users on follower nodes. #[tokio::test] async fn test_load_user_notification_populates_user_and_policy_caches() { - let store = StsGroupsFallbackMockStore; + let store = StsTestMockStore { empty_policies: false }; let cache_manager = IamCache::new(store).await; let iam_sys = IamSys::new(cache_manager); diff --git a/rustfs/src/admin/auth.rs b/rustfs/src/admin/auth.rs index c2cfc525a..f9e5e9c1c 100644 --- a/rustfs/src/admin/auth.rs +++ b/rustfs/src/admin/auth.rs @@ -21,6 +21,7 @@ use rustfs_policy::policy::{Args, action::Action}; use s3s::{S3Result, s3_error}; use std::collections::HashMap; use std::sync::Arc; +use tracing::debug; #[derive(Clone)] struct AuthContext<'a> { @@ -70,7 +71,7 @@ async fn check_admin_request_auth( ) -> S3Result<()> { let conditions = get_condition_values(ctx.headers, ctx.cred, None, None, ctx.remote_addr); - if !iam_store + let allowed = iam_store .is_allowed(&Args { account: &ctx.cred.access_key, groups: &ctx.cred.groups, @@ -82,8 +83,17 @@ async fn check_admin_request_auth( bucket, object, }) - .await - { + .await; + + if !allowed { + debug!( + target = "rustfs::admin::auth", + ?action, + deny_only = ctx.deny_only, + is_owner = ctx.is_owner, + signer_access_key = %ctx.cred.access_key, + "IAM is_allowed returned false for admin request (enable DEBUG for rustfs::admin::auth)" + ); return Err(s3_error!(AccessDenied, "Access Denied")); } diff --git a/rustfs/src/admin/handlers/user.rs b/rustfs/src/admin/handlers/user.rs index 283236ea9..bd0af3f8c 100644 --- a/rustfs/src/admin/handlers/user.rs +++ b/rustfs/src/admin/handlers/user.rs @@ -45,7 +45,7 @@ use serde::Deserialize; use serde_urlencoded::from_bytes; use std::io::{Read as _, Write}; use std::{collections::HashMap, io::Cursor, str::from_utf8}; -use tracing::warn; +use tracing::{debug, warn}; use zip::{ZipArchive, ZipWriter, result::ZipError, write::SimpleFileOptions}; #[derive(Debug, Deserialize, Default)] @@ -66,11 +66,50 @@ pub fn register_user_route(r: &mut S3Router) -> std::io::Result< Ok(()) } +/// Returns the IAM parent user identity for a temporary (Console/STS) session credential. +/// +/// Prefers `parent_user` when non-empty; otherwise reads the JWT `parent` claim. +/// Returns [`None`] when neither is available. +fn temp_identity_parent(requester: &Credentials) -> Option<&str> { + if !requester.parent_user.is_empty() { + return Some(requester.parent_user.as_str()); + } + requester + .claims + .as_ref() + .and_then(|c| c.get("parent")) + .and_then(|v| v.as_str()) +} + +/// How the IAM parent identity was resolved for logging (no JWT claim values). +fn parent_identity_source(requester: &Credentials) -> &'static str { + if !requester.parent_user.is_empty() { + "parent_user_field" + } else if requester.claims.as_ref().and_then(|c| c.get("parent")).is_some() { + "jwt_parent" + } else { + "none" + } +} + +/// Returns `true` when admin policy checks should use `deny_only` mode (only explicit **Deny** blocks; +/// absence of **Allow** does not deny). +/// +/// Eligible cases: +/// - **Long-term credentials**: target is the requester's own `access_key` and there is no `parent_user`. +/// - **Console/STS session (temp)**: target equals the IAM user this session represents, resolved via +/// [`temp_identity_parent`] as `parent_user` when set, else the JWT claim `parent` (some stores +/// persist only the token). +/// +/// Service accounts always use full Allow/Deny evaluation. 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() + if requester.is_service_account() { + return false; + } + if requester.is_temp() { + return temp_identity_parent(requester).is_some_and(|p| p == target_access_key); + } + target_access_key == requester.access_key && requester.parent_user.is_empty() } fn should_reject_group_import_name(group_name: &str, group_lookup: &rustfs_iam::error::Error) -> bool { @@ -160,6 +199,21 @@ impl Operation for AddUser { let check_deny_only = should_check_deny_only(ak, &cred); + debug!( + target = "rustfs::admin::handlers::user", + operation = "AddUser", + query_access_key = %ak, + signer_access_key = %cred.access_key, + is_temp = cred.is_temp(), + is_service_account = cred.is_service_account(), + parent_user = %cred.parent_user, + parent_identity_source = %parent_identity_source(&cred), + jwt_parent_claim_present = cred.claims.as_ref().and_then(|c| c.get("parent")).is_some(), + check_deny_only, + is_owner = owner, + "authorization context before validate_admin_request (no secrets)" + ); + // For eligible self operations, only explicit Deny should block the request. validate_admin_request( &req.headers, @@ -420,6 +474,21 @@ impl Operation for GetUserInfo { let check_deny_only = should_check_deny_only(ak, &cred); + debug!( + target = "rustfs::admin::handlers::user", + operation = "GetUserInfo", + query_access_key = %ak, + signer_access_key = %cred.access_key, + is_temp = cred.is_temp(), + is_service_account = cred.is_service_account(), + parent_user = %cred.parent_user, + parent_identity_source = %parent_identity_source(&cred), + jwt_parent_claim_present = cred.claims.as_ref().and_then(|c| c.get("parent")).is_some(), + check_deny_only, + is_owner = owner, + "authorization context before validate_admin_request (no secrets)" + ); + // For eligible self operations, only explicit Deny should block the request. validate_admin_request( &req.headers, @@ -1143,7 +1212,8 @@ mod tests { } #[test] - fn test_should_not_check_deny_only_for_temp_credentials() { + fn test_should_not_check_deny_only_for_temp_without_parent() { + // Session token present but no parent_user — not a well-formed self session for deny_only. let cred = Credentials { access_key: "alice".to_string(), session_token: "temp-token".to_string(), @@ -1152,6 +1222,35 @@ mod tests { assert!(!should_check_deny_only("alice", &cred)); } + #[test] + fn test_should_check_deny_only_for_temp_when_target_matches_parent_user() { + // Console/AssumeRole: signing AK is a session key; IAM user is parent_user. + let cred = Credentials { + access_key: "VV0V3VYJK2PV6EG45X2Y".to_string(), + session_token: "jwt-session-token".to_string(), + parent_user: "1923".to_string(), + ..Default::default() + }; + assert!(should_check_deny_only("1923", &cred)); + assert!(!should_check_deny_only("1924", &cred)); + assert!(!should_check_deny_only("VV0V3VYJK2PV6EG45X2Y", &cred)); + } + + #[test] + fn test_should_check_deny_only_for_temp_when_parent_only_in_jwt_claims() { + // STS identity may omit `parentUser` on disk; `check_key_valid` still fills `claims["parent"]`. + let mut claims = HashMap::new(); + claims.insert("parent".to_string(), Value::String("1923".to_string())); + let cred = Credentials { + access_key: "39KNO04Z34D6T4AGL6E6".to_string(), + session_token: "jwt-session-token".to_string(), + claims: Some(claims), + ..Default::default() + }; + assert!(should_check_deny_only("1923", &cred)); + assert!(!should_check_deny_only("1924", &cred)); + } + #[test] fn test_should_not_check_deny_only_for_service_account_credentials() { let mut claims = HashMap::new();