From 36e97aba261d5667ef90b5daf67d5770e9f6b05e Mon Sep 17 00:00:00 2001 From: Zhengchao An Date: Fri, 24 Jul 2026 11:32:05 +0800 Subject: [PATCH] fix(security): reject federated STS credentials that collide with the root principal (#5161) A federated OIDC / AssumeRoleWithWebIdentity login derives parent_user from the external identity via session_identity() (username -> email -> sub -> oidc-user-unknown). At IAM request time a temporary credential whose parent_user equals the root access key is treated as owner (see rustfs/src/admin/auth.rs and the rustfs_iam owner resolution used by the policy engine and service-account paths). A federated identity whose display name happens to equal the root access key would therefore be silently granted full owner access on a string match alone. Reject such issuance up front: immediately after parent_user is derived, and before credential generation, set_temp_user, and the site-replication hook, deny the binding when parent_user equals the root access key. Both federated flows funnel through the single DefaultFederatedSessionBinding::bind -> issue_credentials chokepoint, so this covers browser OIDC callback and AssumeRoleWithWebIdentity alike. - rustfs/src/admin/service/federated_identity.rs: issue_credentials resolves the root access key via current_action_credentials() and rejects with FederatedSessionBindingError::InvalidRequest on collision. Added the pure helper parent_user_is_reserved(parent_user, root_access_key) so the collision decision is unit-testable without process globals; the comparison is exact and case-sensitive, mirroring the request-time owner comparison. This is the federated-principal / root isolation pre-work (batch 0A, the immediate issuance-time block) from the OIDC review in rustfs/backlog#1437. Layer 2 (a signature-protected federated credential origin that forces is_owner=false at request time regardless of the parent_user string) is a separate follow-up. Already-issued colliding credentials continue on their existing TTL and should be revoked proactively. Legitimate federated identities are unaffected; only an exact collision with the configured root access key is denied. --- .../src/admin/service/federated_identity.rs | 46 ++++++++++++++++++- 1 file changed, 45 insertions(+), 1 deletion(-) diff --git a/rustfs/src/admin/service/federated_identity.rs b/rustfs/src/admin/service/federated_identity.rs index 1f46144ba..21e2e08ae 100644 --- a/rustfs/src/admin/service/federated_identity.rs +++ b/rustfs/src/admin/service/federated_identity.rs @@ -15,7 +15,7 @@ use super::session_policy::populate_session_policy; use crate::admin::{ handlers::site_replication::site_replication_iam_change_hook, - runtime_sources::{current_ready_iam_handle, current_token_signing_key}, + runtime_sources::{current_action_credentials, current_ready_iam_handle, current_token_signing_key}, }; use rustfs_iam::{ federation::{FederatedSessionBinding, FederatedSessionBindingError, FederatedSessionTransaction}, @@ -156,6 +156,16 @@ fn binding_error_from_s3(error: S3Error) -> FederatedSessionBindingError { } } +/// Returns true when a derived federated `parent_user` collides with the root access key. +/// +/// The comparison mirrors the owner check performed at request time (a temporary credential +/// whose `parent_user` equals the root access key is treated as owner), so a federated login +/// whose `username`/`email`/`sub` happens to equal the root access key must be rejected at +/// issuance rather than silently granted owner. +fn parent_user_is_reserved(parent_user: &str, root_access_key: Option<&str>) -> bool { + matches!(root_access_key, Some(root) if root == parent_user) +} + fn issue_credentials( transaction: &FederatedSessionTransaction, secret: Option<&str>, @@ -169,6 +179,17 @@ fn issue_credentials( token_claims.insert("exp".to_string(), Value::Number(serde_json::Number::from(exp.unix_timestamp()))); let parent_user = claims.session_identity(); + // Fail closed if the derived federated parent collides with the root access key. At IAM + // request time `parent_user == root access key` is treated as owner (see auth.rs and + // rustfs_iam owner resolution), so issuing such a credential would silently grant a + // federated identity full owner access purely because its display name matched root. + // Deny before any credential generation, `set_temp_user`, or site replication. + let root_access_key = current_action_credentials().map(|cred| cred.access_key); + if parent_user_is_reserved(&parent_user, root_access_key.as_deref()) { + return Err(FederatedSessionBindingError::InvalidRequest( + "federated identity resolves to a reserved root principal".to_string(), + )); + } debug!( provider_id = %authorization.provider_id, parent_user = %parent_user, @@ -338,4 +359,27 @@ mod tests { let error = issue_credentials(&transaction, None).expect_err("invalid policy should fail first"); assert!(matches!(error, FederatedSessionBindingError::InvalidRequest(_))); } + + #[test] + fn parent_user_reserved_only_on_root_collision() { + // Collides with the configured root access key -> reserved. + assert!(parent_user_is_reserved("rustfsadmin", Some("rustfsadmin"))); + // A distinct federated identity is never reserved. + assert!(!parent_user_is_reserved("user", Some("rustfsadmin"))); + // Access keys are case-sensitive; a case-different value does not collide. + assert!(!parent_user_is_reserved("RustFsAdmin", Some("rustfsadmin"))); + // Without a resolved root access key there is nothing to collide with. + assert!(!parent_user_is_reserved("rustfsadmin", None)); + } + + #[test] + fn issue_credentials_rejects_root_collision_before_generation() { + // Reuse the collision decision the issuance path applies: a federated identity whose + // derived parent_user equals the root access key must be denied at issuance. + let transaction = transaction(); + let parent_user = transaction.authorization.claims.session_identity(); + assert_eq!(parent_user, "user"); + assert!(parent_user_is_reserved(&parent_user, Some("user"))); + assert!(!parent_user_is_reserved(&parent_user, Some("root"))); + } }