diff --git a/crates/iam/src/oidc_state.rs b/crates/iam/src/oidc_state.rs index 880ee679a..5cdd962ee 100644 --- a/crates/iam/src/oidc_state.rs +++ b/crates/iam/src/oidc_state.rs @@ -234,6 +234,53 @@ mod tests { assert_eq!(first.is_some() as usize + second.is_some() as usize, 1); } + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn distinct_auth_states_are_consumed_independently_under_concurrent_take() { + let store = OidcStateStore::new(); + for (state, provider_id) in [("state_corp", "corp"), ("state_partner", "partner")] { + store + .insert( + state.to_string(), + OidcAuthSession { + provider_id: provider_id.to_string(), + pkce_verifier: format!("{provider_id}-verifier"), + nonce: format!("{provider_id}-nonce"), + redirect_after: None, + }, + ) + .await; + } + + let barrier = Arc::new(Barrier::new(3)); + let first_store = store.clone(); + let first_barrier = Arc::clone(&barrier); + let first = tokio::spawn(async move { + first_barrier.wait().await; + first_store.take("state_corp").await + }); + let second_store = store.clone(); + let second_barrier = Arc::clone(&barrier); + let second = tokio::spawn(async move { + second_barrier.wait().await; + second_store.take("state_partner").await + }); + + barrier.wait().await; + let first = first + .await + .expect("first state consumer should finish") + .expect("first state should remain available to its consumer"); + let second = second + .await + .expect("second state consumer should finish") + .expect("second state should remain available to its consumer"); + + assert_eq!(first.provider_id, "corp"); + assert_eq!(second.provider_id, "partner"); + assert!(!store.contains("state_corp").await); + assert!(!store.contains("state_partner").await); + } + #[tokio::test] async fn test_logout_state_store_insert_and_take() { let store = OidcStateStore::new(); diff --git a/rustfs/src/admin/handlers/site_replication.rs b/rustfs/src/admin/handlers/site_replication.rs index 09e668fa7..73e4882c4 100644 --- a/rustfs/src/admin/handlers/site_replication.rs +++ b/rustfs/src/admin/handlers/site_replication.rs @@ -11386,6 +11386,36 @@ mod tests { } } + fn ldap_settings_kvs() -> rustfs_config::server_config::KVS { + rustfs_config::server_config::KVS(vec![ + rustfs_config::server_config::KV { + key: "enable".to_string(), + value: "on".to_string(), + hidden_if_empty: false, + }, + rustfs_config::server_config::KV { + key: "user_dn_search_base_dn".to_string(), + value: "ou=people,dc=example,dc=com".to_string(), + hidden_if_empty: false, + }, + rustfs_config::server_config::KV { + key: "user_dn_search_filter".to_string(), + value: "(uid=%s)".to_string(), + hidden_if_empty: false, + }, + rustfs_config::server_config::KV { + key: "group_search_base_dn".to_string(), + value: "ou=groups,dc=example,dc=com".to_string(), + hidden_if_empty: false, + }, + rustfs_config::server_config::KV { + key: "group_search_filter".to_string(), + value: "(&(objectclass=groupOfNames)(member=%s))".to_string(), + hidden_if_empty: false, + }, + ]) + } + // rustfs#7003: the site region is per-site and must not reach the peer // IDP comparison while OpenID is unconfigured, or two sites in different // regions can never be paired even with an identical (empty) IDP config. @@ -11412,6 +11442,66 @@ mod tests { assert_eq!(settings.claim_provider.client_id, "client-a"); } + #[test] + fn idp_settings_preserve_site_replication_wire_shape() { + let (ldap, ldap_configs) = ldap_settings_from_kvs(&ldap_settings_kvs()); + let settings = IDPSettings { + ldap, + ldap_configs, + open_id: open_id_settings( + vec![ + ("default".to_string(), openid_provider("client-a")), + ("corp".to_string(), openid_provider("client-b")), + ], + "eu-site-1".to_string(), + ), + }; + + assert_eq!( + serde_json::to_value(settings).expect("serialize IDP settings"), + serde_json::json!({ + "LDAP": { + "IsLDAPEnabled": true, + "LDAPUserDNSearchBase": "ou=people,dc=example,dc=com", + "LDAPUserDNSearchFilter": "(uid=%s)", + "LDAPGroupSearchBase": "ou=groups,dc=example,dc=com", + "LDAPGroupSearchFilter": "(&(objectclass=groupOfNames)(member=%s))", + }, + "LDAPConfigs": { + "Enabled": true, + "Configs": { + "default": { + "UserDNSearchBase": "ou=people,dc=example,dc=com", + "UserDNSearchFilter": "(uid=%s)", + "GroupSearchBase": "ou=groups,dc=example,dc=com", + "GroupSearchFilter": "(&(objectclass=groupOfNames)(member=%s))", + } + } + }, + "OpenID": { + "Enabled": true, + "Region": "eu-site-1", + "Roles": { + "corp": { + "ClaimName": "groups", + "ClaimUserinfoEnabled": false, + "RolePolicy": "readwrite", + "ClientID": "client-b", + "HashedClientSecret": "hashed", + } + }, + "ClaimProvider": { + "ClaimName": "groups", + "ClaimUserinfoEnabled": false, + "RolePolicy": "readwrite", + "ClientID": "client-a", + "HashedClientSecret": "hashed", + } + } + }) + ); + } + // A whole provider object present on one side only must not spill its // credential-derived leaves into the rejection message. #[test] @@ -14494,33 +14584,7 @@ mod tests { #[test] fn test_ldap_settings_from_kvs_reads_minio_style_keys() { - let kvs = rustfs_config::server_config::KVS(vec![ - rustfs_config::server_config::KV { - key: "enable".to_string(), - value: "on".to_string(), - hidden_if_empty: false, - }, - rustfs_config::server_config::KV { - key: "user_dn_search_base_dn".to_string(), - value: "ou=people,dc=example,dc=com".to_string(), - hidden_if_empty: false, - }, - rustfs_config::server_config::KV { - key: "user_dn_search_filter".to_string(), - value: "(uid=%s)".to_string(), - hidden_if_empty: false, - }, - rustfs_config::server_config::KV { - key: "group_search_base_dn".to_string(), - value: "ou=groups,dc=example,dc=com".to_string(), - hidden_if_empty: false, - }, - rustfs_config::server_config::KV { - key: "group_search_filter".to_string(), - value: "(&(objectclass=groupOfNames)(member=%s))".to_string(), - hidden_if_empty: false, - }, - ]); + let kvs = ldap_settings_kvs(); let (ldap, ldap_configs) = ldap_settings_from_kvs(&kvs); diff --git a/rustfs/src/admin/handlers/sts.rs b/rustfs/src/admin/handlers/sts.rs index 7bc7acaac..054a1d11c 100644 --- a/rustfs/src/admin/handlers/sts.rs +++ b/rustfs/src/admin/handlers/sts.rs @@ -434,15 +434,23 @@ async fn handle_assume_role_with_web_identity(body: AssumeRoleRequest) -> S3Resu let new_cred = &session.credentials; let subject = authorization.claims.session_identity(); - // Build XML response (AssumeRoleWithWebIdentityResponse) - let expiration = new_cred + let xml = assume_role_with_web_identity_response_xml(new_cred, &subject); + + let mut resp = S3Response::new((StatusCode::OK, Body::from(xml.into_bytes()))); + resp.headers + .insert(http::header::CONTENT_TYPE, HeaderValue::from_static("application/xml")); + Ok(resp) +} + +fn assume_role_with_web_identity_response_xml(credentials: &rustfs_credentials::Credentials, subject: &str) -> String { + let expiration = credentials .expiration .unwrap_or_else(|| OffsetDateTime::now_utc().saturating_add(Duration::seconds(3600))); let exp_str = expiration .format(&time::format_description::well_known::Rfc3339) .unwrap_or_default(); - let xml = format!( + format!( r#" @@ -455,17 +463,12 @@ async fn handle_assume_role_with_web_identity(body: AssumeRoleRequest) -> S3Resu {} "#, - xml_escape(&new_cred.access_key), - xml_escape(&new_cred.secret_key), - xml_escape(&new_cred.session_token), + xml_escape(&credentials.access_key), + xml_escape(&credentials.secret_key), + xml_escape(&credentials.session_token), xml_escape(&exp_str), - xml_escape(&subject), - ); - - let mut resp = S3Response::new((StatusCode::OK, Body::from(xml.into_bytes()))); - resp.headers - .insert(http::header::CONTENT_TYPE, HeaderValue::from_static("application/xml")); - Ok(resp) + xml_escape(subject), + ) } /// Escape special XML characters in a string. @@ -561,6 +564,33 @@ mod tests { assert_eq!(xml_escape("it's"), "it's"); } + #[test] + fn web_identity_response_xml_preserves_wire_shape() { + let credentials = rustfs_credentials::Credentials { + access_key: "access + + + + access<key + secret&key + session"token + 1970-01-01T00:00:00Z + + subject'name + +"# + ); + } + #[test] fn test_duration_clamping() { // Simulates the clamping logic from handle_assume_role_with_web_identity diff --git a/rustfs/src/startup_auth.rs b/rustfs/src/startup_auth.rs index e21ff8dfb..9120b1fde 100644 --- a/rustfs/src/startup_auth.rs +++ b/rustfs/src/startup_auth.rs @@ -15,7 +15,7 @@ use rustfs_iam::{ federation::{FederatedIdentityRegistry, FederatedIdentityService, oidc::StandardOidcAdapter}, get_oidc, init_oidc_sys_with_extra_root_ca_provider, - oidc::{OidcExtraRootCaMaterial, OidcExtraRootCaProvider}, + oidc::{OidcExtraRootCaMaterial, OidcExtraRootCaProvider, OidcSys}, }; use std::{ collections::hash_map::DefaultHasher, @@ -56,10 +56,7 @@ pub(crate) async fn init_auth_integrations() -> Result<()> { match init_oidc_sys_with_extra_root_ca_provider(oidc_extra_root_ca_provider()).await { Ok(()) => { if let Some(oidc) = get_oidc() { - let adapter = Arc::new(StandardOidcAdapter::new(oidc)); - let registry = FederatedIdentityRegistry::new(adapter); - let service = Arc::new(FederatedIdentityService::new(registry)); - crate::runtime_sources::publish_federated_identity_service(service); + crate::runtime_sources::publish_federated_identity_service(standard_oidc_service(oidc)); } } Err(e) => { @@ -76,6 +73,12 @@ pub(crate) async fn init_auth_integrations() -> Result<()> { Ok(()) } +fn standard_oidc_service(oidc: Arc) -> Arc { + let adapter = Arc::new(StandardOidcAdapter::new(oidc)); + let registry = FederatedIdentityRegistry::new(adapter); + Arc::new(FederatedIdentityService::new(registry)) +} + pub(crate) fn oidc_extra_root_ca_provider() -> OidcExtraRootCaProvider { OidcExtraRootCaProvider::new(current_oidc_extra_root_ca_material) } @@ -112,3 +115,20 @@ fn oidc_extra_root_ca_generation(outbound_generation: u64, root_ca_pem: Option<& root_ca_pem.hash(&mut hasher); hasher.finish() } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn standard_oidc_service_retains_one_oidc_arc() { + let oidc = Arc::new(OidcSys::empty().expect("empty OIDC configuration should be valid")); + let retained = Arc::clone(&oidc); + + let service = standard_oidc_service(oidc); + + assert_eq!(Arc::strong_count(&retained), 2, "the service should retain one OIDC Arc"); + drop(service); + assert_eq!(Arc::strong_count(&retained), 1, "dropping the service should release its OIDC Arc"); + } +}