From bc41e567a5645c1278ebefa6a3a317a51a21531c Mon Sep 17 00:00:00 2001 From: GatewayJ <835269233@qq.com> Date: Sat, 1 Aug 2026 18:10:37 +0800 Subject: [PATCH] fix(oidc): warn on request-header redirect fallback (#5561) * fix(oidc): warn on request-header redirect fallback * test(oidc): cover startup warning publication --- rustfs/src/runtime_sources.rs | 164 +++++++++++++++++++++++++++++++++- 1 file changed, 161 insertions(+), 3 deletions(-) diff --git a/rustfs/src/runtime_sources.rs b/rustfs/src/runtime_sources.rs index 1b8394686..90877f48a 100644 --- a/rustfs/src/runtime_sources.rs +++ b/rustfs/src/runtime_sources.rs @@ -13,7 +13,14 @@ // limitations under the License. use crate::app::context; +use rustfs_config::ENV_RUSTFS_BROWSER_REDIRECT_URL; +use rustfs_iam::federation::FederatedIdentityService; use std::sync::Arc; +use tracing::warn; + +const LOG_COMPONENT_MAIN: &str = "main"; +const LOG_SUBSYSTEM_AUTH: &str = "auth"; +const EVENT_OIDC_BROWSER_REDIRECT_FALLBACK: &str = "oidc_browser_redirect_fallback"; pub(crate) use context::{ AppContext, NotifyInterface, ServerContextSlot, default_notify_interface as fallback_notify_interface, @@ -22,9 +29,9 @@ pub(crate) use context::{ default_s3select_db_interface as fallback_s3select_db_interface, default_scanner_metrics_interface as fallback_scanner_metrics_interface, default_server_config_interface as fallback_server_config_interface, - default_storage_class_interface as fallback_storage_class_interface, publish_federated_identity_service, - publish_server_config, publish_storage_class_config, resolve_action_credentials as current_action_credentials, - resolve_boot_time as current_boot_time, resolve_bucket_metadata_handle as current_bucket_metadata_handle, + default_storage_class_interface as fallback_storage_class_interface, publish_server_config, publish_storage_class_config, + resolve_action_credentials as current_action_credentials, resolve_boot_time as current_boot_time, + resolve_bucket_metadata_handle as current_bucket_metadata_handle, resolve_bucket_monitor_handle as current_bucket_monitor_handle, resolve_buffer_config as current_buffer_config, resolve_daily_tier_stats as current_daily_tier_stats, resolve_deployment_id as current_deployment_id, resolve_encryption_service as current_encryption_service, resolve_endpoints_handle as current_endpoints_handle, @@ -52,6 +59,24 @@ pub(crate) use context::{ resolve_tier_config_handle as current_tier_config_handle, resolve_token_signing_key as current_token_signing_key, }; +pub(crate) fn publish_federated_identity_service(service: Arc) -> bool { + let browser_redirect_url = rustfs_utils::get_env_opt_str(ENV_RUSTFS_BROWSER_REDIRECT_URL); + if service.has_providers() && browser_redirect_url.as_deref().is_none_or(|value| value.trim().is_empty()) { + warn!( + event = EVENT_OIDC_BROWSER_REDIRECT_FALLBACK, + component = LOG_COMPONENT_MAIN, + subsystem = LOG_SUBSYSTEM_AUTH, + state = "fallback_enabled", + configuration = ENV_RUSTFS_BROWSER_REDIRECT_URL, + fallback_source = "request_host_and_forwarded_proto", + reason = "trusted_console_origin_not_configured", + "OIDC browser redirect fallback enabled" + ); + } + + context::publish_federated_identity_service(service) +} + #[cfg(test)] pub(crate) fn set_test_outbound_tls_generation(generation: u64) { context::set_test_outbound_tls_generation(generation); @@ -68,3 +93,136 @@ pub(crate) use context::{IamInterface, KmsInterface, NotificationSystemInterface pub(crate) fn current_app_context() -> Option> { context::get_global_app_context() } + +#[cfg(test)] +mod tests { + use super::*; + use rustfs_iam::{ + federation::{ + FederatedAuthorization, FederatedCodeExchange, FederatedIdentityProvider, FederatedIdentityRegistry, + Result as FederationResult, + }, + oidc::{OidcProviderConfig, OidcProviderSummary}, + }; + use std::{ + io::{self, Write}, + sync::Mutex, + }; + use tracing_subscriber::{Registry, fmt::MakeWriter, layer::SubscriberExt}; + + struct TestProvider { + has_providers: bool, + } + + #[async_trait::async_trait] + impl FederatedIdentityProvider for TestProvider { + fn has_providers(&self) -> bool { + self.has_providers + } + + fn list_providers(&self) -> Vec { + Vec::new() + } + + fn list_visible_providers(&self) -> Vec { + Vec::new() + } + + fn provider_config(&self, _id: &str) -> Option<&OidcProviderConfig> { + None + } + + async fn authorize_url( + &self, + _provider_id: &str, + _redirect_uri: &str, + _redirect_after: Option, + ) -> FederationResult { + unreachable!("startup publication test does not authorize") + } + + async fn exchange_code(&self, _state: &str, _code: &str, _redirect_uri: &str) -> FederationResult { + unreachable!("startup publication test does not exchange codes") + } + + async fn verify_web_identity_token(&self, _jwt: &str) -> FederationResult { + unreachable!("startup publication test does not verify tokens") + } + + async fn create_logout_token(&self, _provider_id: &str, _id_token: &str) -> FederationResult { + unreachable!("startup publication test does not create logout tokens") + } + + async fn build_logout_url( + &self, + _logout_token: &str, + _post_logout_redirect_uri: &str, + ) -> FederationResult> { + unreachable!("startup publication test does not build logout URLs") + } + } + + #[derive(Clone, Default)] + struct CapturedLogs(Arc>>); + + struct CapturedLogWriter(Arc>>); + + impl Write for CapturedLogWriter { + fn write(&mut self, buf: &[u8]) -> io::Result { + self.0.lock().expect("captured log lock").extend_from_slice(buf); + Ok(buf.len()) + } + + fn flush(&mut self) -> io::Result<()> { + Ok(()) + } + } + + impl<'writer> MakeWriter<'writer> for CapturedLogs { + type Writer = CapturedLogWriter; + + fn make_writer(&'writer self) -> Self::Writer { + CapturedLogWriter(self.0.clone()) + } + } + + fn test_service(has_providers: bool) -> Arc { + let provider = Arc::new(TestProvider { has_providers }); + Arc::new(FederatedIdentityService::new(FederatedIdentityRegistry::new(provider))) + } + + fn capture_startup_publication(has_providers: bool, browser_redirect_url: Option<&str>) -> String { + temp_env::with_var(ENV_RUSTFS_BROWSER_REDIRECT_URL, browser_redirect_url, || { + let logs = CapturedLogs::default(); + let captured = logs.0.clone(); + let subscriber = Registry::default().with( + tracing_subscriber::fmt::layer() + .without_time() + .with_target(false) + .with_level(true) + .with_ansi(false) + .with_writer(logs), + ); + + tracing::subscriber::with_default(subscriber, || { + assert!(publish_federated_identity_service(test_service(has_providers))); + }); + + String::from_utf8(captured.lock().expect("captured log lock").clone()).expect("captured logs must be UTF-8") + }) + } + + #[test] + fn oidc_startup_publication_warns_only_for_request_header_fallback() { + let output = capture_startup_publication(true, None); + + assert!(output.contains("WARN"), "{output}"); + assert!(output.contains(EVENT_OIDC_BROWSER_REDIRECT_FALLBACK), "{output}"); + assert!(output.contains(ENV_RUSTFS_BROWSER_REDIRECT_URL), "{output}"); + assert!(output.contains("request_host_and_forwarded_proto"), "{output}"); + assert!(output.contains("trusted_console_origin_not_configured"), "{output}"); + assert!(capture_startup_publication(false, None).is_empty()); + assert!(capture_startup_publication(true, Some("https://console.example.com")).is_empty()); + assert!(!capture_startup_publication(true, Some(" ")).is_empty()); + } +}