From c7c318280fe308237335b8d52dedc875445864a4 Mon Sep 17 00:00:00 2001 From: Cooper Gamble Date: Wed, 17 Jun 2026 06:07:11 +0000 Subject: [PATCH] [codex] close workload identity review gaps [ci changed_files] --- codex-rs/Cargo.lock | 1 + codex-rs/core/src/session/mod.rs | 7 +- codex-rs/core/src/session/session.rs | 6 +- codex-rs/core/src/session/tests.rs | 111 ++++++++++++++---- codex-rs/core/src/session/turn.rs | 6 +- .../model-provider/src/models_endpoint.rs | 8 +- codex-rs/model-provider/src/provider.rs | 51 +++++++- codex-rs/workload-identity-spiffe/src/lib.rs | 2 +- codex-rs/workload-identity/Cargo.toml | 5 +- codex-rs/workload-identity/src/source.rs | 52 +++++--- .../workload-identity/src/source_tests.rs | 31 +++++ 11 files changed, 233 insertions(+), 47 deletions(-) diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 44ab7a4d05..fe4f747d1e 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -4418,6 +4418,7 @@ name = "codex-workload-identity" version = "0.0.0" dependencies = [ "anyhow", + "libc", "pretty_assertions", "reqwest 0.12.28", "schemars 0.8.22", diff --git a/codex-rs/core/src/session/mod.rs b/codex-rs/core/src/session/mod.rs index ef547fdb59..82e3c30800 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -2998,7 +2998,12 @@ impl Session { let recommended_plugin_candidates = if crate::tools::spec_plan::tool_suggest_enabled( turn_context, ) { - match self.services.auth_manager.auth().await { + let auth = if turn_context.config.model_provider.requires_openai_auth { + self.services.auth_manager.auth().await + } else { + Ok(self.services.auth_manager.auth_cached()) + }; + match auth { Ok(auth) => { let plugins_config = turn_context.config.plugins_config_input(); self.services diff --git a/codex-rs/core/src/session/session.rs b/codex-rs/core/src/session/session.rs index 3e08a8934d..7e4b9ecf50 100644 --- a/codex-rs/core/src/session/session.rs +++ b/codex-rs/core/src/session/session.rs @@ -604,7 +604,11 @@ impl Session { let mcp_manager_for_mcp = Arc::clone(&mcp_manager); let mcp_thread_init_for_startup = &mcp_thread_init; let auth_and_mcp_fut = async move { - let auth = auth_manager_clone.auth_result().await?; + let auth = if config_for_mcp.model_provider.requires_openai_auth { + auth_manager_clone.auth_result().await? + } else { + auth_manager_clone.auth_cached() + }; let mcp_config = mcp_manager_for_mcp .runtime_config_for_thread(&config_for_mcp, mcp_thread_init_for_startup) .await; diff --git a/codex-rs/core/src/session/tests.rs b/codex-rs/core/src/session/tests.rs index 84c4a38887..0f798bce51 100644 --- a/codex-rs/core/src/session/tests.rs +++ b/codex-rs/core/src/session/tests.rs @@ -29,6 +29,9 @@ use core_test_support::test_codex::local_selections; use codex_features::Feature; use codex_login::CodexAuth; +use codex_login::ExternalAuth; +use codex_login::ExternalAuthFuture; +use codex_login::ExternalAuthRefreshContext; use codex_model_provider_info::ModelProviderInfo; use codex_models_manager::bundled_models_response; use codex_models_manager::model_info; @@ -81,6 +84,7 @@ use crate::tools::registry::ToolExecutor; use crate::tools::router::ToolCallSource; use crate::turn_diff_tracker::TurnDiffTracker; use codex_app_server_protocol::AppInfo; +use codex_app_server_protocol::AuthMode; use codex_app_server_protocol::McpElicitationSchema; use codex_config::config_toml::ConfigToml; use codex_config::config_toml::ProjectConfig; @@ -4767,18 +4771,33 @@ async fn absolute_cwd_update_with_turn_environment_is_allowed() { assert_eq!(turn_context.environments.turn_environments.len(), 1); } -#[tokio::test] -async fn session_new_fails_when_zsh_fork_enabled_without_packaged_zsh() { - let codex_home = tempfile::tempdir().expect("create temp dir"); - let mut config = build_test_config(codex_home.path()).await; - config - .features - .enable(Feature::ShellZshFork) - .expect("test config should allow shell_zsh_fork"); - config.zsh_path = None; - let config = Arc::new(config); +struct FailingRequiredExternalAuth; - let auth_manager = AuthManager::from_auth_for_testing(CodexAuth::from_api_key("Test API Key")); +impl ExternalAuth for FailingRequiredExternalAuth { + fn auth_mode(&self) -> AuthMode { + AuthMode::Chatgpt + } + + fn requires_successful_resolution(&self) -> bool { + true + } + + fn resolve(&self) -> ExternalAuthFuture<'_, Option> { + Box::pin(async { Err(std::io::Error::other("required external auth failed")) }) + } + + fn refresh( + &self, + _context: ExternalAuthRefreshContext, + ) -> ExternalAuthFuture<'_, codex_login::ExternalAuthTokens> { + Box::pin(async { Err(std::io::Error::other("required external auth failed")) }) + } +} + +async fn start_test_session( + config: Arc, + auth_manager: Arc, +) -> anyhow::Result> { let models_manager = models_manager_with_provider( config.codex_home.to_path_buf(), auth_manager.clone(), @@ -4787,17 +4806,16 @@ async fn session_new_fails_when_zsh_fork_enabled_without_packaged_zsh() { let model = get_model_offline_for_tests(config.model.as_deref()); let model_info = construct_model_info_offline_for_tests(model.as_str(), &config.to_models_manager_config()); - let collaboration_mode = CollaborationMode { - mode: ModeKind::Default, - settings: Settings { - model, - reasoning_effort: config.model_reasoning_effort.clone(), - developer_instructions: None, - }, - }; let session_configuration = SessionConfiguration { provider: config.model_provider.clone(), - collaboration_mode, + collaboration_mode: CollaborationMode { + mode: ModeKind::Default, + settings: Settings { + model, + reasoning_effort: config.model_reasoning_effort.clone(), + developer_instructions: None, + }, + }, model_reasoning_summary: config.model_reasoning_summary, developer_instructions: config.developer_instructions.clone(), loaded_agents_md: None, @@ -4827,7 +4845,6 @@ async fn session_new_fails_when_zsh_fork_enabled_without_packaged_zsh() { dynamic_tools: Vec::new(), user_shell_override: None, }; - let (tx_event, _rx_event) = async_channel::unbounded(); let (agent_status_tx, _agent_status_rx) = watch::channel(AgentStatus::PendingInit); let plugins_manager = Arc::new(PluginsManager::new(config.codex_home.to_path_buf())); @@ -4837,7 +4854,7 @@ async fn session_new_fails_when_zsh_fork_enabled_without_packaged_zsh() { /*bundled_skills_enabled*/ true, )); let environment_manager = Arc::new(EnvironmentManager::default_for_tests()); - let result = Session::new( + Session::new( session_configuration, Arc::clone(&config), /*user_instructions*/ None, @@ -4866,7 +4883,22 @@ async fn session_new_fails_when_zsh_fork_enabled_without_packaged_zsh() { /*attestation_provider*/ None, Some(config.multi_agent_version_from_features()), ) - .await; + .await +} + +#[tokio::test] +async fn session_new_fails_when_zsh_fork_enabled_without_packaged_zsh() { + let codex_home = tempfile::tempdir().expect("create temp dir"); + let mut config = build_test_config(codex_home.path()).await; + config + .features + .enable(Feature::ShellZshFork) + .expect("test config should allow shell_zsh_fork"); + config.zsh_path = None; + let config = Arc::new(config); + + let auth_manager = AuthManager::from_auth_for_testing(CodexAuth::from_api_key("Test API Key")); + let result = start_test_session(config, auth_manager).await; let err = match result { Ok(_) => panic!("expected startup to fail"), @@ -4876,6 +4908,39 @@ async fn session_new_fails_when_zsh_fork_enabled_without_packaged_zsh() { assert!(msg.contains("zsh fork feature enabled, but no packaged zsh fork is available")); } +#[tokio::test] +async fn session_new_skips_required_wif_for_unauthenticated_provider() { + let codex_home = tempfile::tempdir().expect("create temp dir"); + let mut config = build_test_config(codex_home.path()).await; + config.model_provider.requires_openai_auth = false; + let auth_manager = + AuthManager::shared_from_config(&config, /*enable_codex_api_key_env*/ false).await; + assert_eq!(auth_manager.auth_cached(), None); + auth_manager.set_external_auth(Arc::new(FailingRequiredExternalAuth)); + + let _session = start_test_session(Arc::new(config), auth_manager) + .await + .expect("unauthenticated provider should not resolve WIF during session startup"); +} + +#[tokio::test] +async fn session_new_propagates_required_wif_failure_for_authenticated_provider() { + let codex_home = tempfile::tempdir().expect("create temp dir"); + let config = build_test_config(codex_home.path()).await; + assert!(config.model_provider.requires_openai_auth); + let auth_manager = + AuthManager::shared_from_config(&config, /*enable_codex_api_key_env*/ false).await; + assert_eq!(auth_manager.auth_cached(), None); + auth_manager.set_external_auth(Arc::new(FailingRequiredExternalAuth)); + + let error = match start_test_session(Arc::new(config), auth_manager).await { + Ok(_) => panic!("authenticated provider should require successful WIF resolution"), + Err(error) => error, + }; + + assert!(error.to_string().contains("required external auth failed")); +} + // todo: use online model info pub(crate) async fn make_session_and_context() -> (Session, TurnContext) { let (tx_event, _rx_event) = async_channel::unbounded(); diff --git a/codex-rs/core/src/session/turn.rs b/codex-rs/core/src/session/turn.rs index ce0e1a05bc..1ad11c06c5 100644 --- a/codex-rs/core/src/session/turn.rs +++ b/codex-rs/core/src/session/turn.rs @@ -1192,7 +1192,11 @@ pub(crate) async fn built_tools( }; let tool_suggest_is_enabled = tool_suggest_enabled(turn_context); let auth = if tool_suggest_is_enabled { - sess.services.auth_manager.auth().await? + if turn_context.config.model_provider.requires_openai_auth { + sess.services.auth_manager.auth().await? + } else { + sess.services.auth_manager.auth_cached() + } } else { None }; diff --git a/codex-rs/model-provider/src/models_endpoint.rs b/codex-rs/model-provider/src/models_endpoint.rs index 891b37da7a..5dbef4a725 100644 --- a/codex-rs/model-provider/src/models_endpoint.rs +++ b/codex-rs/model-provider/src/models_endpoint.rs @@ -51,7 +51,13 @@ impl OpenAiModelsEndpoint { async fn auth(&self) -> CoreResult> { match self.auth_manager.as_ref() { - Some(auth_manager) => Ok(auth_manager.auth().await?), + Some(auth_manager) + if self.provider_info.requires_openai_auth + || self.provider_info.has_command_auth() => + { + Ok(auth_manager.auth().await?) + } + Some(auth_manager) => Ok(auth_manager.auth_cached()), None => Ok(None), } } diff --git a/codex-rs/model-provider/src/provider.rs b/codex-rs/model-provider/src/provider.rs index de5380caf8..55cba78ae5 100644 --- a/codex-rs/model-provider/src/provider.rs +++ b/codex-rs/model-provider/src/provider.rs @@ -225,14 +225,19 @@ impl ModelProvider for ConfiguredModelProvider { fn supports_attestation(&self) -> bool { self.auth_manager .as_ref() - .and_then(|auth_manager| auth_manager.auth_cached()) - .is_some_and(|auth| auth.is_chatgpt_auth()) + .and_then(|auth_manager| auth_manager.get_api_auth_mode()) + .is_some_and(codex_models_manager::AuthMode::has_chatgpt_account) } fn auth(&self) -> ModelProviderFuture<'_, codex_protocol::error::Result>> { Box::pin(async move { match self.auth_manager.as_ref() { - Some(auth_manager) => Ok(auth_manager.auth().await?), + Some(auth_manager) + if self.info.requires_openai_auth || self.info.has_command_auth() => + { + Ok(auth_manager.auth().await?) + } + Some(auth_manager) => Ok(auth_manager.auth_cached()), None => Ok(None), } }) @@ -371,6 +376,16 @@ mod tests { } } + fn required_external_auth_manager_without_cached_auth() -> Arc { + let auth_manager = AuthManager::external_bearer_only( + provider_info_with_command_auth() + .auth + .expect("command auth should be configured"), + ); + auth_manager.set_external_auth(Arc::new(FailingRequiredExternalAuth)); + auth_manager + } + fn test_codex_home() -> std::path::PathBuf { std::env::temp_dir().join(format!("codex-model-provider-test-{}", std::process::id())) } @@ -411,6 +426,36 @@ mod tests { assert!(provider.api_auth().await.is_err()); } + #[tokio::test] + async fn unauthenticated_provider_does_not_resolve_required_openai_auth() { + let auth_manager = required_external_auth_manager_without_cached_auth(); + let provider = create_model_provider( + provider_for("https://example.test/v1".to_string()), + Some(auth_manager), + ); + + assert_eq!(provider.auth().await.unwrap(), None); + assert!( + provider + .api_auth() + .await + .unwrap() + .to_auth_headers() + .is_empty() + ); + } + + #[test] + fn required_external_chatgpt_auth_supports_attestation_before_resolution() { + let auth_manager = required_external_auth_manager_without_cached_auth(); + let provider = create_model_provider( + ModelProviderInfo::create_openai_provider(/*base_url*/ None), + Some(auth_manager), + ); + + assert!(provider.supports_attestation()); + } + fn remote_model(slug: &str) -> ModelInfo { serde_json::from_value(json!({ "slug": slug, diff --git a/codex-rs/workload-identity-spiffe/src/lib.rs b/codex-rs/workload-identity-spiffe/src/lib.rs index 6025ff6798..155afd0d6a 100644 --- a/codex-rs/workload-identity-spiffe/src/lib.rs +++ b/codex-rs/workload-identity-spiffe/src/lib.rs @@ -83,7 +83,7 @@ mod tests { async fn rejects_tcp_endpoint() { let source = SpiffeSubjectTokenProvider::new( Some("tcp://127.0.0.1:8081".to_string()), - None, + /*spiffe_id*/ None, "openai-audience".to_string(), ); diff --git a/codex-rs/workload-identity/Cargo.toml b/codex-rs/workload-identity/Cargo.toml index 74ff3ab995..116bf86e42 100644 --- a/codex-rs/workload-identity/Cargo.toml +++ b/codex-rs/workload-identity/Cargo.toml @@ -16,11 +16,14 @@ thiserror = { workspace = true } tokio = { workspace = true, features = ["fs", "io-util", "sync"] } url = { workspace = true } +[target.'cfg(unix)'.dependencies] +libc = { workspace = true } + [dev-dependencies] anyhow = { workspace = true } pretty_assertions = { workspace = true } tempfile = { workspace = true } -tokio = { workspace = true, features = ["macros", "rt-multi-thread"] } +tokio = { workspace = true, features = ["macros", "rt-multi-thread", "time"] } wiremock = { workspace = true } [lib] diff --git a/codex-rs/workload-identity/src/source.rs b/codex-rs/workload-identity/src/source.rs index 4ab287a67b..0cb0254345 100644 --- a/codex-rs/workload-identity/src/source.rs +++ b/codex-rs/workload-identity/src/source.rs @@ -152,13 +152,28 @@ impl FileSubjectTokenSource { impl SubjectTokenProvider for FileSubjectTokenSource { async fn subject_token(&self) -> Result { - let file = tokio::fs::File::open(&self.path).await.map_err(|error| { - SubjectTokenError::ReadFile { + let metadata = + tokio::fs::metadata(&self.path) + .await + .map_err(|error| SubjectTokenError::ReadFile { + provider: self.source, + path: self.path.clone(), + kind: error.kind(), + })?; + validate_file_metadata(self.source, &self.path, &metadata)?; + + let mut options = tokio::fs::OpenOptions::new(); + options.read(true); + #[cfg(unix)] + options.custom_flags(libc::O_NONBLOCK); + let file = options + .open(&self.path) + .await + .map_err(|error| SubjectTokenError::ReadFile { provider: self.source, path: self.path.clone(), kind: error.kind(), - } - })?; + })?; let metadata = file .metadata() .await @@ -167,17 +182,7 @@ impl SubjectTokenProvider for FileSubjectTokenSource { path: self.path.clone(), kind: error.kind(), })?; - if !metadata.is_file() { - return Err(SubjectTokenError::NotAFile { - provider: self.source, - path: self.path.clone(), - }); - } - if metadata.len() > MAX_SUBJECT_TOKEN_BYTES as u64 { - return Err(SubjectTokenError::TooLarge { - provider: self.source, - }); - } + validate_file_metadata(self.source, &self.path, &metadata)?; let mut bytes = Vec::new(); file.take(MAX_SUBJECT_TOKEN_BYTES as u64 + 1) .read_to_end(&mut bytes) @@ -201,6 +206,23 @@ impl SubjectTokenProvider for FileSubjectTokenSource { } } +fn validate_file_metadata( + source: &'static str, + path: &std::path::Path, + metadata: &std::fs::Metadata, +) -> Result<(), SubjectTokenError> { + if !metadata.is_file() { + return Err(SubjectTokenError::NotAFile { + provider: source, + path: path.to_path_buf(), + }); + } + if metadata.len() > MAX_SUBJECT_TOKEN_BYTES as u64 { + return Err(SubjectTokenError::TooLarge { provider: source }); + } + Ok(()) +} + #[derive(Clone, Debug)] pub struct UnavailableSubjectTokenSource { source: &'static str, diff --git a/codex-rs/workload-identity/src/source_tests.rs b/codex-rs/workload-identity/src/source_tests.rs index 63565cc6be..02b566f346 100644 --- a/codex-rs/workload-identity/src/source_tests.rs +++ b/codex-rs/workload-identity/src/source_tests.rs @@ -1,4 +1,6 @@ use pretty_assertions::assert_eq; +#[cfg(unix)] +use std::time::Duration; use super::*; @@ -15,6 +17,35 @@ async fn file_source_rereads_rotated_token() -> anyhow::Result<()> { Ok(()) } +#[cfg(unix)] +#[tokio::test] +async fn file_source_rejects_fifo_without_blocking() -> anyhow::Result<()> { + use std::ffi::CString; + use std::os::unix::ffi::OsStrExt; + + let temp_dir = tempfile::tempdir()?; + let path = temp_dir.path().join("subject-token"); + let c_path = CString::new(path.as_os_str().as_bytes())?; + // SAFETY: `c_path` is a valid, nul-terminated path and `mkfifo` does not + // retain the pointer after the call. + let result = unsafe { libc::mkfifo(c_path.as_ptr(), 0o600) }; + assert_eq!(result, 0); + let source = FileSubjectTokenSource::new(path.clone()); + + let result = tokio::time::timeout(Duration::from_secs(1), source.subject_token()) + .await + .expect("FIFO credential validation should not block"); + + assert!(matches!( + result, + Err(SubjectTokenError::NotAFile { + provider: "file", + path: error_path, + }) if error_path == path + )); + Ok(()) +} + #[tokio::test] async fn environment_source_captures_startup_value() -> anyhow::Result<()> { const VARIABLE: &str = "CODEX_WIF_SOURCE_CAPTURE_TEST";