From 878da562addb99d724d2c13958453b429f245dea Mon Sep 17 00:00:00 2001 From: Cooper Gamble Date: Wed, 17 Jun 2026 03:55:10 +0000 Subject: [PATCH] [codex] close workload identity review gaps [ci changed_files] --- codex-rs/Cargo.lock | 1 + .../request_processors/account_processor.rs | 11 +-- codex-rs/cloud-tasks/src/util.rs | 19 ++-- codex-rs/login/Cargo.toml | 1 + codex-rs/login/src/auth/auth_tests.rs | 42 ++++++++- codex-rs/login/src/auth/manager.rs | 25 ++++- codex-rs/login/src/auth/workload_identity.rs | 12 ++- codex-rs/process-hardening/src/lib.rs | 93 +++++++++++++++---- 8 files changed, 158 insertions(+), 46 deletions(-) diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 80e3920730..44ab7a4d05 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -3246,6 +3246,7 @@ dependencies = [ "codex-keyring-store", "codex-model-provider-info", "codex-otel", + "codex-process-hardening", "codex-protocol", "codex-secrets", "codex-terminal-detection", diff --git a/codex-rs/app-server/src/request_processors/account_processor.rs b/codex-rs/app-server/src/request_processors/account_processor.rs index d8c62fef63..3041d3ae46 100644 --- a/codex-rs/app-server/src/request_processors/account_processor.rs +++ b/codex-rs/app-server/src/request_processors/account_processor.rs @@ -780,12 +780,11 @@ impl AccountRequestProcessor { requires_openai_auth: Some(false), } } else { - let auth = if do_refresh { - Ok(self.auth_manager.auth_cached()) - } else { - self.auth_manager.auth().await - } - .map_err(|err| internal_error(format!("failed to resolve auth: {err}")))?; + let auth = self + .auth_manager + .auth() + .await + .map_err(|err| internal_error(format!("failed to resolve auth: {err}")))?; match auth { Some(auth) => { let permanent_refresh_failure = diff --git a/codex-rs/cloud-tasks/src/util.rs b/codex-rs/cloud-tasks/src/util.rs index 2e0c61c516..840587404a 100644 --- a/codex-rs/cloud-tasks/src/util.rs +++ b/codex-rs/cloud-tasks/src/util.rs @@ -2,6 +2,7 @@ use chrono::DateTime; use chrono::Local; use chrono::Utc; use reqwest::header::HeaderMap; +use std::sync::Arc; use codex_core::config::Config; use codex_login::AuthManager; @@ -41,19 +42,13 @@ pub fn normalize_base_url(input: &str) -> String { base_url } -pub async fn load_auth_manager(chatgpt_base_url: Option) -> Option { +pub async fn load_auth_manager(chatgpt_base_url: Option) -> Option> { // TODO: pass in cli overrides once cloud tasks properly support them. - let config = Config::load_with_cli_overrides(Vec::new()).await.ok()?; - Some( - AuthManager::new( - config.codex_home.to_path_buf(), - /*enable_codex_api_key_env*/ false, - config.cli_auth_credentials_store_mode, - chatgpt_base_url.or(Some(config.chatgpt_base_url.clone())), - config.auth_keyring_backend_kind(), - ) - .await, - ) + let mut config = Config::load_with_cli_overrides(Vec::new()).await.ok()?; + if let Some(chatgpt_base_url) = chatgpt_base_url { + config.chatgpt_base_url = chatgpt_base_url; + } + Some(AuthManager::shared_from_config(&config, /*enable_codex_api_key_env*/ false).await) } /// Build headers for ChatGPT-backed requests: `User-Agent`, optional `Authorization`, diff --git a/codex-rs/login/Cargo.toml b/codex-rs/login/Cargo.toml index c8a6abdb22..92c0b28919 100644 --- a/codex-rs/login/Cargo.toml +++ b/codex-rs/login/Cargo.toml @@ -30,6 +30,7 @@ codex-config = { workspace = true } codex-keyring-store = { workspace = true } codex-model-provider-info = { workspace = true } codex-otel = { workspace = true } +codex-process-hardening = { workspace = true } codex-protocol = { workspace = true } codex-secrets = { workspace = true } codex-terminal-detection = { workspace = true } diff --git a/codex-rs/login/src/auth/auth_tests.rs b/codex-rs/login/src/auth/auth_tests.rs index 856dcfad75..a2b72db74d 100644 --- a/codex-rs/login/src/auth/auth_tests.rs +++ b/codex-rs/login/src/auth/auth_tests.rs @@ -643,27 +643,54 @@ async fn external_chatgpt_auth_resolves_and_refreshes_without_writing_auth_file( ) .await; manager.set_external_auth(external.clone()); + let mut auth_changes = manager.auth_change_receiver(); - assert_eq!(manager.auth_mode(), Some(AuthMode::Chatgpt)); - assert_eq!( - manager.get_api_auth_mode(), - Some(AuthMode::ChatgptAuthTokens) - ); + assert_eq!(manager.auth_mode(), None); + assert_eq!(manager.get_api_auth_mode(), None); let auth = manager .auth() .await .expect("external auth resolution should succeed") .expect("external auth should resolve"); + assert!( + auth_changes + .has_changed() + .expect("auth watch should be open") + ); + drop(auth_changes.borrow_and_update()); assert!(manager.is_external_chatgpt_auth_active()); + assert_eq!(manager.auth_mode(), Some(AuthMode::Chatgpt)); + assert_eq!( + manager.get_api_auth_mode(), + Some(AuthMode::ChatgptAuthTokens) + ); assert_eq!(auth.get_token().unwrap(), token); assert_eq!(auth.get_account_id().as_deref(), Some(WORKSPACE_ID_ALLOWED)); assert!(!get_auth_file(codex_home.path()).exists()); + let repeated = manager + .auth() + .await + .expect("repeated external auth resolution should succeed") + .expect("repeated external auth should resolve"); + assert_eq!(repeated.get_token().unwrap(), token); + assert!( + !auth_changes + .has_changed() + .expect("auth watch should be open"), + "unchanged external tokens must not publish an auth change" + ); + manager .refresh_token() .await .expect("external auth refresh"); assert_eq!(external.refresh_count.load(Ordering::Relaxed), 1); + assert!( + auth_changes + .has_changed() + .expect("auth watch should be open") + ); assert_eq!( manager.auth_cached().unwrap().get_token().unwrap(), refreshed_token @@ -731,6 +758,11 @@ async fn auth_result_preserves_external_auth_resolution_errors() { .await; manager.set_external_auth(Arc::new(FailingExternalChatgptAuth)); + assert_eq!(manager.auth_mode(), Some(AuthMode::Chatgpt)); + assert_eq!( + manager.get_api_auth_mode(), + Some(ApiAuthMode::ChatgptAuthTokens) + ); let error = manager .auth_result() .await diff --git a/codex-rs/login/src/auth/manager.rs b/codex-rs/login/src/auth/manager.rs index 06d8e69c04..762f1c2835 100644 --- a/codex-rs/login/src/auth/manager.rs +++ b/codex-rs/login/src/auth/manager.rs @@ -1787,7 +1787,15 @@ impl AuthManager { (ApiAuthMode::ApiKey, ApiAuthMode::ApiKey) => a.api_key() == b.api_key(), (ApiAuthMode::Chatgpt, ApiAuthMode::Chatgpt) | (ApiAuthMode::ChatgptAuthTokens, ApiAuthMode::ChatgptAuthTokens) => { - a.get_current_auth_json() == b.get_current_auth_json() + let mut a = a.get_current_auth_json(); + let mut b = b.get_current_auth_json(); + if let Some(a) = a.as_mut() { + a.last_refresh = None; + } + if let Some(b) = b.as_mut() { + b.last_refresh = None; + } + a == b } (ApiAuthMode::AgentIdentity, ApiAuthMode::AgentIdentity) => match (a, b) { (CodexAuth::AgentIdentity(a), CodexAuth::AgentIdentity(b)) => { @@ -1973,9 +1981,18 @@ impl AuthManager { } fn external_auth_mode(&self) -> Option { - self.external_auth() - .as_ref() - .map(|external_auth| external_auth.auth_mode()) + let external_auth = self.external_auth()?; + let mode = external_auth.auth_mode(); + if mode == AuthMode::Chatgpt + && !external_auth.requires_successful_resolution() + && !self + .auth_cached() + .as_ref() + .is_some_and(CodexAuth::is_external_chatgpt_tokens) + { + return None; + } + Some(mode) } fn has_external_api_key_auth(&self) -> bool { diff --git a/codex-rs/login/src/auth/workload_identity.rs b/codex-rs/login/src/auth/workload_identity.rs index 955c45ccf8..b2540f9c11 100644 --- a/codex-rs/login/src/auth/workload_identity.rs +++ b/codex-rs/login/src/auth/workload_identity.rs @@ -8,14 +8,24 @@ use super::ExternalAuthTokens; pub(super) struct WorkloadIdentityExternalAuth { client: ConfiguredWorkloadIdentityClient, + process_isolation_error: Option, } impl WorkloadIdentityExternalAuth { pub(super) fn new(client: ConfiguredWorkloadIdentityClient) -> Self { - Self { client } + let process_isolation_error = codex_process_hardening::disable_process_inspection() + .err() + .map(|error| format!("workload identity process isolation failed: {error}")); + Self { + client, + process_isolation_error, + } } async fn tokens(&self, force_refresh: bool) -> std::io::Result { + if let Some(error) = self.process_isolation_error.as_ref() { + return Err(std::io::Error::other(error.clone())); + } let token = if force_refresh { self.client.refresh().await } else { diff --git a/codex-rs/process-hardening/src/lib.rs b/codex-rs/process-hardening/src/lib.rs index f500e15d41..8adee5c777 100644 --- a/codex-rs/process-hardening/src/lib.rs +++ b/codex-rs/process-hardening/src/lib.rs @@ -3,6 +3,18 @@ use std::ffi::OsString; #[cfg(unix)] use std::os::unix::ffi::OsStrExt; +#[cfg(any(target_os = "linux", target_os = "android", target_os = "macos"))] +use std::sync::OnceLock; + +#[cfg(any(target_os = "linux", target_os = "android", target_os = "macos"))] +#[derive(Clone, Copy)] +enum ProcessInspectionHardening { + Applied, + Failed(Option), +} + +#[cfg(any(target_os = "linux", target_os = "android", target_os = "macos"))] +static PROCESS_INSPECTION_HARDENING: OnceLock = OnceLock::new(); /// This is designed to be called pre-main() (using `#[ctor::ctor]`) to perform /// various process hardening steps, such as @@ -43,12 +55,8 @@ const SET_RLIMIT_CORE_FAILED_EXIT_CODE: i32 = 7; #[cfg(any(target_os = "linux", target_os = "android"))] pub(crate) fn pre_main_hardening_linux() { // Disable ptrace attach / mark process non-dumpable. - let ret_code = unsafe { libc::prctl(libc::PR_SET_DUMPABLE, 0, 0, 0, 0) }; - if ret_code != 0 { - eprintln!( - "ERROR: prctl(PR_SET_DUMPABLE, 0) failed: {}", - std::io::Error::last_os_error() - ); + if let Err(error) = disable_process_inspection() { + eprintln!("ERROR: prctl(PR_SET_DUMPABLE, 0) failed: {error}"); std::process::exit(PRCTL_FAILED_EXIT_CODE); } @@ -63,12 +71,7 @@ pub(crate) fn pre_main_hardening_linux() { /// Mark the current Linux process non-dumpable so same-user processes cannot attach with ptrace. #[cfg(target_os = "linux")] pub fn disable_process_dumping() -> std::io::Result<()> { - let ret_code = unsafe { libc::prctl(libc::PR_SET_DUMPABLE, 0, 0, 0, 0) }; - if ret_code == 0 { - Ok(()) - } else { - Err(std::io::Error::last_os_error()) - } + disable_process_inspection() } #[cfg(any(target_os = "freebsd", target_os = "openbsd"))] @@ -82,12 +85,8 @@ pub(crate) fn pre_main_hardening_bsd() { #[cfg(target_os = "macos")] pub(crate) fn pre_main_hardening_macos() { // Prevent debuggers from attaching to this process. - let ret_code = unsafe { libc::ptrace(libc::PT_DENY_ATTACH, 0, std::ptr::null_mut(), 0) }; - if ret_code == -1 { - eprintln!( - "ERROR: ptrace(PT_DENY_ATTACH) failed: {}", - std::io::Error::last_os_error() - ); + if let Err(error) = disable_process_inspection() { + eprintln!("ERROR: ptrace(PT_DENY_ATTACH) failed: {error}"); std::process::exit(PTRACE_DENY_ATTACH_FAILED_EXIT_CODE); } @@ -121,6 +120,55 @@ pub(crate) fn pre_main_hardening_windows() { // TODO(mbolin): Perform the appropriate configuration for Windows. } +/// Prevent same-user child processes from inspecting this process's environment or memory. +/// +/// This is idempotent because auth managers can be rebuilt as configuration changes. Callers that +/// hold in-memory credentials should fail closed when the current platform cannot provide this +/// boundary. +#[cfg(any(target_os = "linux", target_os = "android", target_os = "macos"))] +pub fn disable_process_inspection() -> std::io::Result<()> { + match *PROCESS_INSPECTION_HARDENING.get_or_init(apply_process_inspection_hardening) { + ProcessInspectionHardening::Applied => Ok(()), + ProcessInspectionHardening::Failed(Some(error)) => { + Err(std::io::Error::from_raw_os_error(error)) + } + ProcessInspectionHardening::Failed(None) => Err(std::io::Error::other( + "failed to disable process inspection", + )), + } +} + +#[cfg(not(any(target_os = "linux", target_os = "android", target_os = "macos")))] +pub fn disable_process_inspection() -> std::io::Result<()> { + Err(std::io::Error::new( + std::io::ErrorKind::Unsupported, + "process inspection hardening is unsupported on this platform", + )) +} + +#[cfg(any(target_os = "linux", target_os = "android", target_os = "macos"))] +fn apply_process_inspection_hardening() -> ProcessInspectionHardening { + #[cfg(any(target_os = "linux", target_os = "android"))] + { + let result = unsafe { libc::prctl(libc::PR_SET_DUMPABLE, 0, 0, 0, 0) }; + if result == 0 { + ProcessInspectionHardening::Applied + } else { + ProcessInspectionHardening::Failed(std::io::Error::last_os_error().raw_os_error()) + } + } + + #[cfg(target_os = "macos")] + { + let result = unsafe { libc::ptrace(libc::PT_DENY_ATTACH, 0, std::ptr::null_mut(), 0) }; + if result == 0 { + ProcessInspectionHardening::Applied + } else { + ProcessInspectionHardening::Failed(std::io::Error::last_os_error().raw_os_error()) + } + } +} + #[cfg(unix)] fn remove_env_vars_with_prefix(prefix: &[u8]) { for key in env_keys_with_prefix(std::env::vars_os(), prefix) { @@ -190,4 +238,13 @@ mod tests { assert_eq!(keys.len(), 1); assert_eq!(keys[0].as_os_str(), ld_test_var); } + + #[cfg(target_os = "linux")] + #[test] + fn disable_process_inspection_marks_process_non_dumpable() { + disable_process_inspection().expect("process inspection hardening should succeed"); + + let dumpable = unsafe { libc::prctl(libc::PR_GET_DUMPABLE) }; + assert_eq!(dumpable, 0); + } }