fix(admin): console self password for STS sessions (#1923) (#2250)

Co-authored-by: GatewayJ <8352692332qq.com>
2026-07-27 16:48:58 +00:00 · 2026-03-21 22:10:38 +08:00
parent 95850c1bcd
commit 19d3a23a13
3 changed files with 261 additions and 21 deletions
@@ -794,6 +794,15 @@ impl<T: Store> IamSys<T> {
                    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<T: Store> IamSys<T> {
            } 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<Vec<String>> = 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<Vec<String>> = 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);

@@ -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"));
    }

@@ -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<AdminOperation>) -> 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();