diff --git a/rustfs/src/admin/handlers/site_replication.rs b/rustfs/src/admin/handlers/site_replication.rs index bca762fb3..a9dda62fc 100644 --- a/rustfs/src/admin/handlers/site_replication.rs +++ b/rustfs/src/admin/handlers/site_replication.rs @@ -61,7 +61,7 @@ use rustfs_iam::sys::{ }; use rustfs_madmin::{ BucketBandwidth, GroupStatus, IDPSettings, InProgressMetric, InQueueMetric, LDAPConfigSettings, LDAPSettings, - OpenIDProviderSettings, PeerInfo, PeerSite, QStat, ReplProxyMetric, ReplicateAddStatus, ReplicateEditStatus, + OpenIDProviderSettings, OpenIDSettings, PeerInfo, PeerSite, QStat, ReplProxyMetric, ReplicateAddStatus, ReplicateEditStatus, ReplicateRemoveStatus, ResyncBucketStatus, SITE_REPL_API_VERSION, SR_IAM_ITEM_STS_ACC, SR_IAM_ITEM_STS_ACC_LEGACY, SRBucketInfo, SRBucketMeta, SRBucketStatsSummary, SRGroupInfo, SRGroupStatsSummary, SRIAMItem, SRIAMUser, SRILMExpiryStatsSummary, SRInfo, SRMetric, SRMetricsSummary, SRPeerError, SRPeerJoinReq, SRPendingOperation, SRPolicyMapping, @@ -1079,6 +1079,62 @@ async fn add_preflight_infos( .collect() } +/// The first field at which a peer's reported IDP settings diverge from the +/// local ones, named so an operator can act on the rejection instead of +/// re-deriving the comparison (rustfs#7003). Values are reported except for +/// credential-derived leaves, which are named but never echoed. +fn idp_settings_difference(local: &serde_json::Value, peer: &serde_json::Value) -> Option { + // Only scalars are echoed: a nested object may carry credential-derived + // leaves whose own field name never reaches `path`. + fn scalar(value: &serde_json::Value) -> Option { + match value { + serde_json::Value::String(text) => Some(text.clone()), + serde_json::Value::Object(_) | serde_json::Value::Array(_) => None, + other => Some(other.to_string()), + } + } + + fn presence(value: &serde_json::Value) -> &'static str { + if value.is_null() { "absent" } else { "set" } + } + + fn walk(path: &str, local: &serde_json::Value, peer: &serde_json::Value) -> Option { + if local == peer { + return None; + } + + if let (serde_json::Value::Object(local_fields), serde_json::Value::Object(peer_fields)) = (local, peer) { + let keys: BTreeSet<&String> = local_fields.keys().chain(peer_fields.keys()).collect(); + for key in keys { + let child_path = if path.is_empty() { + key.to_string() + } else { + format!("{path}.{key}") + }; + let difference = walk( + &child_path, + local_fields.get(key).unwrap_or(&serde_json::Value::Null), + peer_fields.get(key).unwrap_or(&serde_json::Value::Null), + ); + if difference.is_some() { + return difference; + } + } + } + + let field = if path.is_empty() { "" } else { path }; + if field.to_ascii_lowercase().contains("secret") { + return Some(format!("`{field}` differs (values redacted)")); + } + match (scalar(local), scalar(peer)) { + (Some(local), Some(peer)) => Some(format!("`{field}` differs (local `{local}`, peer `{peer}`)")), + _ => Some(format!("`{field}` differs (local {}, peer {})", presence(local), presence(peer))), + } + } + + walk("", local, peer) +} + fn validate_add_preflight_topology(infos: &[SiteReplicationAddPreflightInfo], local_peer: &PeerInfo) -> S3Result<()> { let mut deployment_ids = HashSet::new(); let mut local_seen = false; @@ -1121,8 +1177,12 @@ fn validate_add_preflight_topology(infos: &[SiteReplicationAddPreflightInfo], lo )); }; for info in infos { - if &info.idp_settings != local_idp { - return Err(s3_error!(InvalidRequest, "IDP settings mismatch for site `{}`", info.endpoint)); + if let Some(difference) = idp_settings_difference(local_idp, &info.idp_settings) { + return Err(s3_error!( + InvalidRequest, + "IDP settings mismatch for site `{}`: {difference}", + info.endpoint + )); } } @@ -1916,36 +1976,64 @@ async fn bootstrap_existing_metadata_after_add( errors } +/// The OpenID half of the IDP settings a site reports to its site-replication +/// peers, built from the providers this site has configured (in +/// `list_providers` order) and the site's own region. +fn open_id_settings(providers: Vec<(String, OpenIDProviderSettings)>, region: String) -> OpenIDSettings { + // `region` qualifies the provider identities, not the site: a site + // without OpenID must report the empty settings in every region, or the + // peer comparison in `validate_add_preflight_topology` would reject two + // sites whose IDP configuration is identically absent (rustfs#7003). + if providers.is_empty() { + return OpenIDSettings::default(); + } + + let mut settings = OpenIDSettings { + enabled: true, + region, + ..Default::default() + }; + + for (provider_id, provider_settings) in providers { + let claim_provider_unset = settings.claim_provider.client_id.is_empty() + && settings.claim_provider.claim_name.is_empty() + && settings.claim_provider.role_policy.is_empty() + && settings.claim_provider.hashed_client_secret.is_empty(); + + if provider_id == "default" || claim_provider_unset { + settings.claim_provider = provider_settings; + } else { + settings.roles.insert(provider_id, provider_settings); + } + } + + settings +} + fn local_idp_settings() -> IDPSettings { let mut settings = IDPSettings::default(); if let Some(federation) = current_federated_identity_service() { - let providers = federation.list_providers(); - settings.open_id.enabled = !providers.is_empty(); - settings.open_id.region = current_region().map(|region| region.to_string()).unwrap_or_default(); - - for provider in providers { - let Some(config) = federation.get_provider_config(&provider.provider_id) else { - continue; - }; - let provider_settings = OpenIDProviderSettings { - claim_name: config.claim_name.clone(), - claim_userinfo_enabled: false, - role_policy: config.role_policy.clone(), - client_id: config.client_id.clone(), - hashed_client_secret: hash_client_secret(config.client_secret.as_deref()), - }; - - let claim_provider_unset = settings.open_id.claim_provider.client_id.is_empty() - && settings.open_id.claim_provider.claim_name.is_empty() - && settings.open_id.claim_provider.role_policy.is_empty() - && settings.open_id.claim_provider.hashed_client_secret.is_empty(); - - if provider.provider_id == "default" || claim_provider_unset { - settings.open_id.claim_provider = provider_settings.clone(); - } else { - settings.open_id.roles.insert(provider.provider_id.clone(), provider_settings); - } - } + // A listed provider whose config cannot be resolved contributes + // nothing to the peer comparison, so it is dropped here rather than + // inside the settings builder. + let providers = federation + .list_providers() + .into_iter() + .filter_map(|provider| { + let config = federation.get_provider_config(&provider.provider_id)?; + Some(( + provider.provider_id.clone(), + OpenIDProviderSettings { + claim_name: config.claim_name.clone(), + claim_userinfo_enabled: false, + role_policy: config.role_policy.clone(), + client_id: config.client_id.clone(), + hashed_client_secret: hash_client_secret(config.client_secret.as_deref()), + }, + )) + }) + .collect(); + settings.open_id = open_id_settings(providers, current_region().map(|region| region.to_string()).unwrap_or_default()); } let (ldap, ldap_configs) = load_ldap_idp_settings(); @@ -9636,6 +9724,119 @@ mod tests { assert!(err.to_string().contains("must include the local deployment")); } + fn openid_provider(client_id: &str) -> OpenIDProviderSettings { + OpenIDProviderSettings { + claim_name: "groups".to_string(), + claim_userinfo_enabled: false, + role_policy: "readwrite".to_string(), + client_id: client_id.to_string(), + hashed_client_secret: "hashed".to_string(), + } + } + + // 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. + #[test] + fn test_open_id_settings_omits_region_without_providers() { + let settings = open_id_settings(Vec::new(), "eu-site-1".to_string()); + + assert!(!settings.enabled); + assert_eq!(settings.region, ""); + assert!(settings.roles.is_empty()); + assert_eq!( + serde_json::to_value(&settings).expect("serialize OpenID settings"), + serde_json::to_value(OpenIDSettings::default()).expect("serialize default OpenID settings"), + "a site without OpenID providers must report the same settings in every region" + ); + } + + #[test] + fn test_open_id_settings_keeps_region_with_providers() { + let settings = open_id_settings(vec![("default".to_string(), openid_provider("client-a"))], "eu-site-1".to_string()); + + assert!(settings.enabled); + assert_eq!(settings.region, "eu-site-1"); + assert_eq!(settings.claim_provider.client_id, "client-a"); + } + + // A whole provider object present on one side only must not spill its + // credential-derived leaves into the rejection message. + #[test] + fn test_validate_add_preflight_topology_omits_nested_values_in_idp_diff() { + let local_peer = PeerInfo { + deployment_id: "local-dep".to_string(), + ..peer("local", "https://local.example.com") + }; + let mut local = preflight_site("local", "https://local.example.com", "local-dep", 0); + local.idp_settings = serde_json::json!({ + "OpenID": {"Enabled": true, "ClaimProvider": {"ClientID": "client-a", "HashedClientSecret": "local-hash"}}, + }); + let mut remote = preflight_site("remote", "https://remote.example.com", "remote-dep", 0); + remote.idp_settings = serde_json::json!({"OpenID": {"Enabled": true}}); + let infos = vec![local, remote]; + + let err = validate_add_preflight_topology(&infos, &local_peer).expect_err("IDP mismatch should fail"); + let message = err.to_string(); + + assert!(message.contains("OpenID.ClaimProvider"), "{message}"); + assert!(!message.contains("local-hash"), "{message}"); + } + + // rustfs#7003: a bare "IDP settings mismatch" leaves the operator with no + // way to tell which element diverged. + #[test] + fn test_validate_add_preflight_topology_reports_differing_idp_field() { + let local_peer = PeerInfo { + deployment_id: "local-dep".to_string(), + ..peer("local", "https://local.example.com") + }; + let mut local = preflight_site("local", "https://local.example.com", "local-dep", 0); + local.idp_settings = serde_json::json!({ + "LDAP": {"IsLDAPEnabled": false}, + "OpenID": {"Enabled": false, "Region": "eu-site-1"}, + }); + let mut remote = preflight_site("remote", "https://remote.example.com", "remote-dep", 0); + remote.idp_settings = serde_json::json!({ + "LDAP": {"IsLDAPEnabled": false}, + "OpenID": {"Enabled": false, "Region": "eu-site-2"}, + }); + let infos = vec![local, remote]; + + let err = validate_add_preflight_topology(&infos, &local_peer).expect_err("IDP mismatch should fail"); + let message = err.to_string(); + + assert!(message.contains("OpenID.Region"), "{message}"); + assert!(message.contains("eu-site-1"), "{message}"); + assert!(message.contains("eu-site-2"), "{message}"); + } + + // Hashed client secrets are still credential-derived material: name the + // field that diverged, never its value. + #[test] + fn test_validate_add_preflight_topology_redacts_secret_values_in_idp_diff() { + let local_peer = PeerInfo { + deployment_id: "local-dep".to_string(), + ..peer("local", "https://local.example.com") + }; + let mut local = preflight_site("local", "https://local.example.com", "local-dep", 0); + local.idp_settings = serde_json::json!({ + "OpenID": {"ClaimProvider": {"HashedClientSecret": "local-hash"}}, + }); + let mut remote = preflight_site("remote", "https://remote.example.com", "remote-dep", 0); + remote.idp_settings = serde_json::json!({ + "OpenID": {"ClaimProvider": {"HashedClientSecret": "remote-hash"}}, + }); + let infos = vec![local, remote]; + + let err = validate_add_preflight_topology(&infos, &local_peer).expect_err("IDP mismatch should fail"); + let message = err.to_string(); + + assert!(message.contains("OpenID.ClaimProvider.HashedClientSecret"), "{message}"); + assert!(!message.contains("local-hash"), "{message}"); + assert!(!message.contains("remote-hash"), "{message}"); + } + #[test] fn test_validate_add_preflight_topology_rejects_idp_mismatch() { let local_peer = PeerInfo {