fix(admin): keep site region out of empty IDP comparison (#7015)

`local_idp_settings` stamped the site region into the reported OpenID settings whenever the federated identity service was published, which it is even with OpenID disabled and no provider configured. The add preflight compares those settings verbatim, so two sites in different regions could never be paired: `replicate add` failed with `IDP settings mismatch` while both sites reported an identical, empty `identity_openid` config. Report the empty OpenID settings when no provider is configured, so the region only qualifies real provider identities, and name the diverging field in the rejection instead of emitting a bare mismatch. Scalar values are echoed; nested objects and credential-derived leaves are reported by presence only. Fixes #7003
2026-09-06 12:09:12 +00:00 · 2026-09-01 22:41:24 +08:00
parent 6a8a8a1eaf
commit cebe57a2f0
1 changed files with 231 additions and 30 deletions
@@ -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<String> {
+    // 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<String> {
+        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<String> {
+        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() { "<root>" } 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 {