fix(iam): address PR 1875 review issues for OIDC STS flows (#1969)

crates/iam/src/oidc.rs

@@ -30,7 +30,12 @@ use std::borrow::Cow;
 use std::collections::HashMap;
 use std::future::Future;
 use std::pin::Pin;
-use tracing::{error, info};
+use std::sync::RwLock;
+use std::time::{Duration as StdDuration, Instant};
+use tracing::{error, info, warn};
+use url::Url;
+
+const OIDC_JWKS_REFRESH_INTERVAL: StdDuration = StdDuration::from_secs(24 * 60 * 60);
 
 // ---- HTTP Client Adapter ----
 
@@ -139,8 +144,16 @@ pub struct OidcClaims {
 /// a `CoreClient` because the crate uses type-state generics that make storing
 /// the configured client in a HashMap impractical. The client is reconstructed
 /// on-the-fly from metadata when needed.
+#[derive(Clone)]
 struct ProviderState {
     metadata: CoreProviderMetadata,
+    discovered_at: Instant,
+}
+
+impl ProviderState {
+    fn is_stale(&self) -> bool {
+        self.discovered_at.elapsed() >= OIDC_JWKS_REFRESH_INTERVAL
+    }
 }
 
 // ---- Core OIDC system ----
@@ -148,7 +161,7 @@ struct ProviderState {
 /// Global OIDC manager for all configured providers.
 pub struct OidcSys {
     configs: HashMap<String, OidcProviderConfig>,
-    provider_states: HashMap<String, ProviderState>,
+    provider_states: RwLock<HashMap<String, ProviderState>>,
     state_store: OidcStateStore,
     http_client: ReqwestHttpClient,
 }
@@ -181,7 +194,7 @@ impl OidcSys {
 
         Ok(Self {
             configs,
-            provider_states,
+            provider_states: RwLock::new(provider_states),
             state_store: OidcStateStore::new(),
             http_client,
         })
@@ -191,7 +204,7 @@ impl OidcSys {
     pub fn empty() -> Self {
         Self {
             configs: HashMap::new(),
-            provider_states: HashMap::new(),
+            provider_states: RwLock::new(HashMap::new()),
             state_store: OidcStateStore::new(),
             http_client: ReqwestHttpClient(reqwest::Client::new()),
         }
@@ -224,12 +237,12 @@ impl OidcSys {
             .configs
             .get(provider_id)
             .ok_or_else(|| format!("unknown OIDC provider: {provider_id}"))?;
-        let state = self
-            .provider_states
-            .get(provider_id)
-            .ok_or_else(|| format!("provider not discovered: {provider_id}"))?;
+        let state = self.ensure_provider_state(provider_id, config).await?;
+
+        let (pkce_challenge, pkce_verifier) = PkceCodeChallenge::new_random_sha256();
+
+        let redirect = RedirectUrl::new(redirect_uri.to_string()).map_err(|e| format!("invalid redirect URI: {e}"))?;
 
-        // Construct CoreClient on-the-fly (avoids type-state storage issues)
         let client = CoreClient::from_provider_metadata(
             state.metadata.clone(),
             ClientId::new(config.client_id.clone()),
@@ -237,10 +250,6 @@ impl OidcSys {
         )
         .set_auth_type(AuthType::RequestBody);
 
-        let (pkce_challenge, pkce_verifier) = PkceCodeChallenge::new_random_sha256();
-
-        let redirect = RedirectUrl::new(redirect_uri.to_string()).map_err(|e| format!("invalid redirect URI: {e}"))?;
-
         let mut auth_req =
             client.authorize_url(CoreAuthenticationFlow::AuthorizationCode, CsrfToken::new_random, Nonce::new_random);
         auth_req = auth_req.set_redirect_uri(Cow::Owned(redirect));
@@ -287,10 +296,7 @@ impl OidcSys {
             .configs
             .get(&session.provider_id)
             .ok_or_else(|| format!("unknown provider: {}", session.provider_id))?;
-        let provider_state = self
-            .provider_states
-            .get(&session.provider_id)
-            .ok_or_else(|| format!("provider not discovered: {}", session.provider_id))?;
+        let provider_state = self.get_provider_state(&session.provider_id)?;
 
         // Construct CoreClient on-the-fly with JWKS from discovery
         let client = CoreClient::from_provider_metadata(
@@ -319,16 +325,36 @@ impl OidcSys {
             .ok_or_else(|| "no id_token in token response".to_string())?;
 
         let verifier = client.id_token_verifier();
-        let _verified_claims = id_token
-            .claims(&verifier, &Nonce::new(session.nonce.clone()))
-            .map_err(|e| format!("ID token verification failed: {e}"))?;
+        let verified = id_token.claims(&verifier, &Nonce::new(session.nonce.clone()));
+        if let Err(e) = verified {
+            let refreshed_state = self
+                .refresh_provider_state(&session.provider_id, config)
+                .await
+                .map_err(|refresh_err| {
+                    format!("ID token verification failed: {e}; failed to refresh provider metadata: {refresh_err}")
+                })?;
+
+            warn!(
+                "OIDC provider '{}' JWKS metadata refreshed and verification retried after failure",
+                session.provider_id
+            );
+
+            let client = CoreClient::from_provider_metadata(
+                refreshed_state.metadata,
+                ClientId::new(config.client_id.clone()),
+                config.client_secret.as_ref().map(|s| ClientSecret::new(s.clone())),
+            )
+            .set_auth_type(AuthType::RequestBody);
+
+            let verifier = client.id_token_verifier();
+            id_token
+                .claims(&verifier, &Nonce::new(session.nonce.clone()))
+                .map_err(|retry_err| format!("ID token verification failed after JWKS refresh: {retry_err}"))?;
+        }
 
         // Extract raw claims from the verified JWT for custom claim support
         // (the crate verifies signature/expiry/nonce; we decode payload for non-standard claims)
-        let raw_jwt = serde_json::to_value(id_token)
-            .ok()
-            .and_then(|v| v.as_str().map(String::from))
-            .unwrap_or_default();
+        let raw_jwt = id_token.to_string();
         let raw = decode_jwt_payload(&raw_jwt);
 
         let claims = OidcClaims {
@@ -412,10 +438,12 @@ impl OidcSys {
             .ok_or_else(|| "JWT missing 'iss' claim".to_string())?;
 
         // Find matching provider by issuer
-        let (provider_id, config, state) = self
+        let (provider_id, config, mut state) = self
             .find_provider_by_issuer(issuer)
             .ok_or_else(|| format!("no OIDC provider configured for issuer: {issuer}"))?;
 
+        state = self.ensure_provider_state_if_stale(&provider_id, &config, &state).await?;
+
         // Reconstruct CoreClient from provider metadata
         let client = CoreClient::from_provider_metadata(
             state.metadata.clone(),
@@ -432,9 +460,25 @@ impl OidcSys {
         // Verify the token (signature, issuer, audience, expiry) — skip nonce
         // (nonce is only required for the authorization code flow)
         let verifier = client.id_token_verifier();
-        let _verified = id_token
-            .claims(&verifier, |_: Option<&Nonce>| Ok(()))
-            .map_err(|e| format!("ID token verification failed: {e}"))?;
+        if let Err(e) = id_token.claims(&verifier, |_: Option<&Nonce>| Ok(())) {
+            state = self
+                .refresh_provider_state(&provider_id, &config)
+                .await
+                .map_err(|refresh_err| {
+                    format!("ID token verification failed: {e}; failed to refresh provider metadata: {refresh_err}")
+                })?;
+
+            let client = CoreClient::from_provider_metadata(
+                state.metadata,
+                ClientId::new(config.client_id.clone()),
+                config.client_secret.as_ref().map(|s| ClientSecret::new(s.clone())),
+            )
+            .set_auth_type(AuthType::RequestBody);
+            let verifier = client.id_token_verifier();
+            id_token
+                .claims(&verifier, |_: Option<&Nonce>| Ok(()))
+                .map_err(|retry_err| format!("ID token verification failed after JWKS refresh: {retry_err}"))?;
+        }
 
         // Extract claims using the provider's claim configuration
         let claims = OidcClaims {
@@ -449,20 +493,85 @@ impl OidcSys {
     }
 
     /// Find a provider whose discovered issuer matches the given JWT issuer string.
-    fn find_provider_by_issuer(&self, issuer: &str) -> Option<(&str, &OidcProviderConfig, &ProviderState)> {
-        let issuer_normalized = issuer.trim_end_matches('/');
-        for (id, state) in &self.provider_states {
+    fn find_provider_by_issuer(&self, issuer: &str) -> Option<(String, OidcProviderConfig, ProviderState)> {
+        let (issuer_scheme, issuer_host, issuer_port, issuer_path) = normalize_issuer(issuer)?;
+        let map = self
+            .provider_states
+            .read()
+            .map_err(|e| format!("provider state lock poisoned: {e}"))
+            .ok()?;
+        for (id, state) in map.iter() {
             let provider_issuer = state.metadata.issuer().as_str();
-            let provider_normalized = provider_issuer.trim_end_matches('/');
-            if issuer_normalized == provider_normalized
+            let Some((provider_scheme, provider_host, provider_port, provider_path)) = normalize_issuer(provider_issuer) else {
+                continue;
+            };
+
+            if issuer_scheme == provider_scheme
+                && issuer_host == provider_host
+                && issuer_port == provider_port
+                && issuer_path == provider_path
                 && let Some(config) = self.configs.get(id)
             {
-                return Some((id, config, state));
+                return Some((id.clone(), config.clone(), state.clone()));
             }
         }
         None
     }
 
+    fn get_provider_state(&self, provider_id: &str) -> Result<ProviderState, String> {
+        self.provider_states
+            .read()
+            .map_err(|e| format!("provider state lock poisoned: {e}"))?
+            .get(provider_id)
+            .cloned()
+            .ok_or_else(|| format!("provider not discovered: {provider_id}"))
+    }
+
+    async fn refresh_provider_state(&self, provider_id: &str, config: &OidcProviderConfig) -> Result<ProviderState, String> {
+        let state = Self::discover_provider(config, &self.http_client).await?;
+        let mut map = self.provider_states.write().map_err(|e| {
+            let msg = e.to_string();
+            format!("provider state lock poisoned: {msg}")
+        })?;
+        map.insert(provider_id.to_string(), state.clone());
+
+        Ok(state)
+    }
+
+    async fn ensure_provider_state(&self, provider_id: &str, config: &OidcProviderConfig) -> Result<ProviderState, String> {
+        let state = self.get_provider_state(provider_id)?;
+        if state.is_stale() {
+            self.refresh_provider_state(provider_id, config).await.or_else(|refresh_err| {
+                warn!(
+                    "OIDC provider '{}' JWKS metadata refresh skipped due to transient network issue: {}",
+                    provider_id, refresh_err
+                );
+                Ok(state)
+            })
+        } else {
+            Ok(state)
+        }
+    }
+
+    async fn ensure_provider_state_if_stale(
+        &self,
+        provider_id: &str,
+        config: &OidcProviderConfig,
+        state: &ProviderState,
+    ) -> Result<ProviderState, String> {
+        if state.is_stale() {
+            self.refresh_provider_state(provider_id, config).await.or_else(|refresh_err| {
+                warn!(
+                    "OIDC provider '{}' JWKS metadata refresh skipped due to transient network issue: {}",
+                    provider_id, refresh_err
+                );
+                Ok(state.clone())
+            })
+        } else {
+            Ok(state.clone())
+        }
+    }
+
     /// Get the state store (used by HTTP handlers).
     pub fn state_store(&self) -> &OidcStateStore {
         &self.state_store
@@ -611,18 +720,8 @@ impl OidcSys {
     /// `discover_async` fetches the discovery document and JWKS in one step.
     async fn discover_provider(config: &OidcProviderConfig, http_client: &ReqwestHttpClient) -> Result<ProviderState, String> {
         // The openidconnect crate expects the issuer URL (base), not the
-        // .well-known/openid-configuration URL. Strip the suffix if present.
-        let issuer_str = config
-            .config_url
-            .strip_suffix("/.well-known/openid-configuration")
-            .unwrap_or(&config.config_url);
-
-        // Ensure trailing slash for correct URL joining in the crate
-        let issuer_str = if issuer_str.ends_with('/') {
-            issuer_str.to_string()
-        } else {
-            format!("{issuer_str}/")
-        };
+        // .well-known/openid-configuration URL.
+        let issuer_str = normalize_config_url(&config.config_url)?;
 
         let issuer_url = IssuerUrl::new(issuer_str).map_err(|e| format!("invalid issuer URL: {e}"))?;
 
@@ -630,12 +729,63 @@ impl OidcSys {
             .await
             .map_err(|e| format!("discovery failed: {e}"))?;
 
-        Ok(ProviderState { metadata })
+        Ok(ProviderState {
+            metadata,
+            discovered_at: Instant::now(),
+        })
     }
 }
 
 // --- Helper functions ---
 
+fn normalize_issuer(raw: &str) -> Option<(String, String, u16, String)> {
+    let parsed = Url::parse(raw).ok()?;
+    if parsed.scheme() != "http" && parsed.scheme() != "https" {
+        return None;
+    }
+
+    let host = parsed.host_str()?.to_ascii_lowercase();
+    let port = parsed.port_or_known_default()?;
+    let normalized_path = {
+        let path = parsed.path().trim_end_matches('/').to_string();
+        if path.is_empty() { "/".to_string() } else { path }
+    };
+
+    Some((parsed.scheme().to_string(), host, port, normalized_path))
+}
+
+fn normalize_config_url(config_url: &str) -> Result<String, String> {
+    let config_url = config_url.trim();
+    let url = Url::parse(config_url).map_err(|e| format!("invalid config_url: {e}"))?;
+    if url.scheme() != "http" && url.scheme() != "https" {
+        return Err(format!("invalid config_url scheme: {}", url.scheme()));
+    }
+    let host = url.host_str().ok_or_else(|| "config_url missing host".to_string())?;
+    let path = url.path().trim_end_matches('/');
+
+    if path.contains("/.well-known/") && !path.ends_with("/.well-known/openid-configuration") {
+        return Err("config_url uses an unsupported .well-known discovery URL".into());
+    }
+
+    let normalized_path = path.strip_suffix("/.well-known/openid-configuration").unwrap_or(path);
+
+    let mut issuer = format!("{}://{host}", url.scheme());
+    if let Some(port) = url.port() {
+        issuer.push(':');
+        issuer.push_str(&port.to_string());
+    }
+
+    if !normalized_path.is_empty() {
+        issuer.push_str(normalized_path);
+    }
+
+    if !issuer.ends_with('/') {
+        issuer.push('/');
+    }
+
+    Ok(issuer)
+}
+
 /// Decode the payload section of a JWT without validation (token must already be verified).
 pub(crate) fn decode_jwt_payload(token: &str) -> HashMap<String, serde_json::Value> {
     let parts: Vec<&str> = token.split('.').collect();
@@ -722,6 +872,40 @@ mod tests {
         assert_eq!(claims.get("email").and_then(|v| v.as_str()), Some("user@example.com"));
     }
 
+    #[test]
+    fn test_normalize_issuer_matches() {
+        let lhs = normalize_issuer("https://idp.example.com/.well-known/openid-configuration/").unwrap();
+        let rhs = normalize_issuer("https://idp.example.com/.well-known/openid-configuration").unwrap();
+        assert_eq!(lhs, rhs);
+        assert_eq!(
+            lhs,
+            (
+                "https".to_string(),
+                "idp.example.com".to_string(),
+                443,
+                "/.well-known/openid-configuration".to_string()
+            )
+        );
+    }
+
+    #[test]
+    fn test_normalize_config_url() {
+        assert_eq!(
+            normalize_config_url("https://idp.example.com/.well-known/openid-configuration").unwrap(),
+            "https://idp.example.com/"
+        );
+        assert_eq!(
+            normalize_config_url("https://idp.example.com/.well-known/openid-configuration/").unwrap(),
+            "https://idp.example.com/"
+        );
+        assert_eq!(
+            normalize_config_url("https://idp.example.com/custom/realm").unwrap(),
+            "https://idp.example.com/custom/realm/"
+        );
+        assert!(normalize_config_url("https://idp.example.com/.well-known/invalid").is_err());
+        assert!(normalize_config_url("gopher://idp.example.com").is_err());
+    }
+
     #[test]
     fn test_decode_jwt_payload_invalid() {
         assert!(decode_jwt_payload("not-a-jwt").is_empty());
@@ -810,7 +994,7 @@ mod tests {
         }
         OidcSys {
             configs: config_map,
-            provider_states: HashMap::new(),
+            provider_states: RwLock::new(HashMap::new()),
             state_store: OidcStateStore::new(),
             http_client: ReqwestHttpClient(reqwest::Client::new()),
         }

crates/iam/src/oidc_state.rs

@@ -13,7 +13,15 @@
 // limitations under the License.
 
 use moka::future::Cache;
-use std::time::Duration;
+use std::sync::Arc;
+use std::sync::atomic::{AtomicU64, Ordering};
+use std::time::{Duration, SystemTime, UNIX_EPOCH};
+use tracing::warn;
+
+const OIDC_STATE_CAPACITY: u64 = 10_000;
+const OIDC_STATE_CAPACITY_WARNING: u64 = 9_000;
+const OIDC_STATE_CAPACITY_CRITICAL: u64 = 10_000;
+const OIDC_STATE_CAPACITY_LOG_INTERVAL_SECS: u64 = 60;
 
 /// Stores the PKCE verifier and nonce for an in-flight OIDC authorization flow.
 #[derive(Debug, Clone)]
@@ -29,20 +37,55 @@ pub struct OidcAuthSession {
 #[derive(Clone)]
 pub struct OidcStateStore {
     cache: Cache<String, OidcAuthSession>,
+    last_capacity_log_at: Arc<AtomicU64>,
 }
 
 impl OidcStateStore {
     pub fn new() -> Self {
         let cache = Cache::builder()
-            .max_capacity(10_000)
+            .max_capacity(OIDC_STATE_CAPACITY)
             .time_to_live(Duration::from_secs(300)) // 5 minute TTL
             .build();
-        Self { cache }
+        Self {
+            cache,
+            last_capacity_log_at: Arc::new(AtomicU64::new(0)),
+        }
     }
 
     /// Store a new auth session keyed by the OAuth2 `state` parameter.
     pub async fn insert(&self, state: String, session: OidcAuthSession) {
         self.cache.insert(state, session).await;
+        let size = self.cache.entry_count();
+
+        if !Self::should_log_capacity_warning(&self.last_capacity_log_at) {
+            return;
+        }
+
+        if size >= OIDC_STATE_CAPACITY_CRITICAL {
+            self.cache.run_pending_tasks().await;
+            warn!("OIDC state store reached configured capacity ({size}/{OIDC_STATE_CAPACITY})");
+            return;
+        }
+
+        if size >= OIDC_STATE_CAPACITY_WARNING {
+            self.cache.run_pending_tasks().await;
+            warn!("OIDC state store approaching capacity ({size}/{OIDC_STATE_CAPACITY})");
+        }
+    }
+
+    fn now_unix_secs() -> u64 {
+        SystemTime::now().duration_since(UNIX_EPOCH).map_or(0, |d| d.as_secs())
+    }
+
+    fn should_log_capacity_warning(last_log_at: &AtomicU64) -> bool {
+        let now = Self::now_unix_secs();
+        let last = last_log_at.load(Ordering::Acquire);
+        if now.saturating_sub(last) < OIDC_STATE_CAPACITY_LOG_INTERVAL_SECS {
+            return false;
+        }
+        last_log_at
+            .compare_exchange(last, now, Ordering::AcqRel, Ordering::Acquire)
+            .is_ok()
     }
 
     /// Retrieve and remove an auth session (single-use). Returns None if expired or not found.

crates/iam/src/sys.rs

@@ -778,6 +778,9 @@ impl<T: Store> IamSys<T> {
                 use rustfs_policy::policy::default::DEFAULT_POLICIES;
                 let mut resolved = Vec::new();
                 for policy_name in claim_policies.split(',').map(|s| s.trim()).filter(|s| !s.is_empty()) {
+                    if !Self::is_safe_claim_policy_name(policy_name) {
+                        continue;
+                    }
                     for (name, p) in DEFAULT_POLICIES.iter() {
                         if *name == policy_name {
                             resolved.push(p.clone());
@@ -827,6 +830,10 @@ impl<T: Store> IamSys<T> {
         is_owner || combined_policy.is_allowed(args).await
     }
 
+    fn is_safe_claim_policy_name(policy: &str) -> bool {
+        !policy.is_empty() && policy.chars().all(|c| c.is_ascii_alphanumeric() || c == '_' || c == '-')
+    }
+
     pub async fn is_allowed_service_account(&self, args: &Args<'_>, parent_user: &str) -> bool {
         let Some(p) = args.claims.get("parent") else {
             return false;