From e46830c2a900f31cf63a2082bb0a327cbc777ec7 Mon Sep 17 00:00:00 2001 From: Greg Harper Date: Mon, 16 Mar 2026 14:33:55 -0700 Subject: [PATCH] Use shared access group for macOS keychain secrets --- codex-rs/Cargo.lock | 4 + codex-rs/cli/Cargo.toml | 1 + codex-rs/cli/src/main.rs | 9 + codex-rs/keyring-store/Cargo.toml | 3 + codex-rs/keyring-store/src/lib.rs | 54 +++++- codex-rs/keyring-store/src/macos.rs | 271 ++++++++++++++++++++++++++++ 6 files changed, 341 insertions(+), 1 deletion(-) create mode 100644 codex-rs/keyring-store/src/macos.rs diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 84f911bd44..7955f942fa 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -1655,6 +1655,7 @@ dependencies = [ "codex-core", "codex-exec", "codex-execpolicy", + "codex-keyring-store", "codex-login", "codex-mcp-server", "codex-protocol", @@ -2101,7 +2102,10 @@ dependencies = [ name = "codex-keyring-store" version = "0.0.0" dependencies = [ + "core-foundation 0.10.1", "keyring", + "security-framework 3.5.1", + "security-framework-sys", "tracing", ] diff --git a/codex-rs/cli/Cargo.toml b/codex-rs/cli/Cargo.toml index a7e88cd1b4..621080c94b 100644 --- a/codex-rs/cli/Cargo.toml +++ b/codex-rs/cli/Cargo.toml @@ -30,6 +30,7 @@ codex-config = { workspace = true } codex-core = { workspace = true } codex-exec = { workspace = true } codex-execpolicy = { workspace = true } +codex-keyring-store = { workspace = true } codex-login = { workspace = true } codex-mcp-server = { workspace = true } codex-protocol = { workspace = true } diff --git a/codex-rs/cli/src/main.rs b/codex-rs/cli/src/main.rs index 05b568b7b7..64c95f2907 100644 --- a/codex-rs/cli/src/main.rs +++ b/codex-rs/cli/src/main.rs @@ -21,6 +21,7 @@ use codex_exec::Cli as ExecCli; use codex_exec::Command as ExecCommand; use codex_exec::ReviewArgs; use codex_execpolicy::ExecPolicyCheckCommand; +use codex_keyring_store::migrate_existing_codex_items_to_access_group; use codex_responses_api_proxy::Args as ResponsesApiProxyArgs; use codex_state::StateRuntime; use codex_state::state_db_path; @@ -601,6 +602,14 @@ async fn cli_main(arg0_paths: Arg0DispatchPaths) -> anyhow::Result<()> { root_config_overrides.raw_overrides.extend(toggle_overrides); let root_remote = remote.remote; + #[cfg(target_os = "macos")] + if let Err(error) = migrate_existing_codex_items_to_access_group() { + tracing::warn!( + error = %error, + "failed to migrate existing Codex keychain items into the shared macOS access group" + ); + } + match subcommand { None => { prepend_config_flags( diff --git a/codex-rs/keyring-store/Cargo.toml b/codex-rs/keyring-store/Cargo.toml index dd3686ee59..bf1399c859 100644 --- a/codex-rs/keyring-store/Cargo.toml +++ b/codex-rs/keyring-store/Cargo.toml @@ -15,7 +15,10 @@ tracing = { workspace = true } keyring = { workspace = true, features = ["linux-native-async-persistent"] } [target.'cfg(target_os = "macos")'.dependencies] +core-foundation = "0.10" keyring = { workspace = true, features = ["apple-native"] } +security-framework = { version = "3", features = ["OSX_10_15"] } +security-framework-sys = "2.15" [target.'cfg(target_os = "windows")'.dependencies] keyring = { workspace = true, features = ["windows-native"] } diff --git a/codex-rs/keyring-store/src/lib.rs b/codex-rs/keyring-store/src/lib.rs index 10dad3a98a..9c404729a8 100644 --- a/codex-rs/keyring-store/src/lib.rs +++ b/codex-rs/keyring-store/src/lib.rs @@ -1,10 +1,26 @@ -use keyring::Entry; use keyring::Error as KeyringError; use std::error::Error; use std::fmt; use std::fmt::Debug; use tracing::trace; +#[cfg(target_os = "macos")] +mod macos; + +#[cfg(not(target_os = "macos"))] +use keyring::Entry; + +#[cfg(not(target_os = "macos"))] +pub fn migrate_existing_codex_items_to_access_group() -> Result<(), CredentialStoreError> { + Ok(()) +} + +#[cfg(target_os = "macos")] +pub fn migrate_existing_codex_items_to_access_group() -> Result<(), CredentialStoreError> { + macos::migrate_existing_codex_items_to_access_group() + .map_err(|error| CredentialStoreError::new(KeyringError::PlatformFailure(Box::new(error)))) +} + #[derive(Debug)] pub enum CredentialStoreError { Other(KeyringError), @@ -51,7 +67,19 @@ pub struct DefaultKeyringStore; impl KeyringStore for DefaultKeyringStore { fn load(&self, service: &str, account: &str) -> Result, CredentialStoreError> { trace!("keyring.load start, service={service}, account={account}"); + #[cfg(target_os = "macos")] + { + macos::load_password(service, account).map_err(|error| { + trace!( + "keyring.load macos native error, service={service}, account={account}, error={error}" + ); + CredentialStoreError::new(KeyringError::PlatformFailure(Box::new(error))) + }) + } + + #[cfg(not(target_os = "macos"))] let entry = Entry::new(service, account).map_err(CredentialStoreError::new)?; + #[cfg(not(target_os = "macos"))] match entry.get_password() { Ok(password) => { trace!("keyring.load success, service={service}, account={account}"); @@ -73,7 +101,19 @@ impl KeyringStore for DefaultKeyringStore { "keyring.save start, service={service}, account={account}, value_len={}", value.len() ); + #[cfg(target_os = "macos")] + { + macos::save_password(service, account, value).map_err(|error| { + trace!( + "keyring.save macos native error, service={service}, account={account}, error={error}" + ); + CredentialStoreError::new(KeyringError::PlatformFailure(Box::new(error))) + }) + } + + #[cfg(not(target_os = "macos"))] let entry = Entry::new(service, account).map_err(CredentialStoreError::new)?; + #[cfg(not(target_os = "macos"))] match entry.set_password(value) { Ok(()) => { trace!("keyring.save success, service={service}, account={account}"); @@ -88,7 +128,19 @@ impl KeyringStore for DefaultKeyringStore { fn delete(&self, service: &str, account: &str) -> Result { trace!("keyring.delete start, service={service}, account={account}"); + #[cfg(target_os = "macos")] + { + macos::delete_password(service, account).map_err(|error| { + trace!( + "keyring.delete macos native error, service={service}, account={account}, error={error}" + ); + CredentialStoreError::new(KeyringError::PlatformFailure(Box::new(error))) + }) + } + + #[cfg(not(target_os = "macos"))] let entry = Entry::new(service, account).map_err(CredentialStoreError::new)?; + #[cfg(not(target_os = "macos"))] match entry.delete_credential() { Ok(()) => { trace!("keyring.delete success, service={service}, account={account}"); diff --git a/codex-rs/keyring-store/src/macos.rs b/codex-rs/keyring-store/src/macos.rs new file mode 100644 index 0000000000..9ce3be7a72 --- /dev/null +++ b/codex-rs/keyring-store/src/macos.rs @@ -0,0 +1,271 @@ +use core_foundation::base::TCFType; +use core_foundation::dictionary::CFDictionary; +use core_foundation::string::CFString; +use security_framework::base::Error as SecurityError; +use security_framework::item::ItemClass; +use security_framework::item::ItemSearchOptions; +use security_framework::item::ItemUpdateOptions; +use security_framework::item::Limit; +use security_framework::item::Location; +use security_framework::item::SearchResult; +use security_framework::item::update_item; +use security_framework::passwords::PasswordOptions; +use security_framework::passwords::delete_generic_password_options; +use security_framework::passwords::generic_password; +use security_framework::passwords::set_generic_password_options; +use security_framework_sys::base::errSecItemNotFound; +use security_framework_sys::item::kSecAttrAccount; +use std::fmt; + +#[derive(Debug)] +pub(crate) struct MacOsAccessGroupError { + details: String, +} + +impl MacOsAccessGroupError { + fn new(details: impl Into) -> Self { + Self { + details: details.into(), + } + } +} + +impl fmt::Display for MacOsAccessGroupError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!( + f, + "failed to access macOS keychain item in shared access group: {}", + self.details + ) + } +} + +impl std::error::Error for MacOsAccessGroupError {} + +const SHARED_ACCESS_GROUP: &str = "2DC432GLL2.com.openai.shared"; +const CODEX_KEYCHAIN_SERVICES: [&str; 3] = ["Codex Auth", "Codex MCP Credentials", "codex"]; + +pub(crate) fn load_password( + service: &str, + account: &str, +) -> Result, MacOsAccessGroupError> { + if let Some(password) = load_shared_password_bytes(service, account)? { + return decode_password(password, service, account).map(Some); + } + + if migrate_password_to_access_group(service, account)? + && let Some(password) = load_shared_password_bytes(service, account)? + { + return decode_password(password, service, account).map(Some); + } + + load_legacy_password_bytes(service, account)? + .map(|password| decode_password(password, service, account)) + .transpose() +} + +pub(crate) fn save_password( + service: &str, + account: &str, + value: &str, +) -> Result<(), MacOsAccessGroupError> { + if load_shared_password_bytes(service, account)?.is_none() { + let _ = migrate_password_to_access_group(service, account)?; + } + + save_shared_password_bytes(service, account, value.as_bytes()) +} + +pub(crate) fn delete_password(service: &str, account: &str) -> Result { + let shared_removed = delete_shared_password(service, account)?; + let legacy_removed = delete_legacy_password(service, account)?; + Ok(shared_removed || legacy_removed) +} + +pub(crate) fn migrate_existing_codex_items_to_access_group() -> Result<(), MacOsAccessGroupError> { + for service in CODEX_KEYCHAIN_SERVICES { + for account in legacy_accounts_for_service(service)? { + let _ = migrate_password_to_access_group(service, &account)?; + } + } + + Ok(()) +} + +fn migrate_password_to_access_group( + service: &str, + account: &str, +) -> Result { + let mut search = ItemSearchOptions::new(); + search + .class(ItemClass::generic_password()) + .service(service) + .account(account); + + let mut update = ItemUpdateOptions::new(); + update + .set_access_group(SHARED_ACCESS_GROUP) + .set_location(Location::DataProtectionKeychain); + + match update_item(&search, &update) { + Ok(()) => Ok(true), + Err(error) if error.code() == errSecItemNotFound => Ok(false), + Err(error) => Err(wrap_security_error( + "move existing generic password into the shared access group", + error, + )), + } +} + +fn legacy_accounts_for_service(service: &str) -> Result, MacOsAccessGroupError> { + let mut search = ItemSearchOptions::new(); + search + .class(ItemClass::generic_password()) + .service(service) + .load_attributes(true) + .limit(Limit::All); + + let results = match search.search() { + Ok(results) => results, + Err(error) if error.code() == errSecItemNotFound => return Ok(Vec::new()), + Err(error) => { + return Err(wrap_security_error( + "search existing generic passwords for Codex service", + error, + )); + } + }; + + let mut accounts = Vec::with_capacity(results.len()); + for result in results { + match result { + SearchResult::Dict(attributes) => accounts.push(account_from_attributes(&attributes)?), + SearchResult::Ref(_) | SearchResult::Data(_) | SearchResult::Other => { + return Err(MacOsAccessGroupError::new( + "search existing generic passwords for Codex service returned an unexpected result", + )); + } + } + } + + Ok(accounts) +} + +fn account_from_attributes(attributes: &CFDictionary) -> Result { + let Some(account_value) = attributes.find(unsafe { kSecAttrAccount }.cast()) else { + return Err(MacOsAccessGroupError::new( + "generic password attributes did not include an account name", + )); + }; + Ok(unsafe { CFString::wrap_under_get_rule((*account_value).cast()) }.to_string()) +} + +fn decode_password( + password: Vec, + service: &str, + account: &str, +) -> Result { + String::from_utf8(password).map_err(|error| { + MacOsAccessGroupError::new(format!( + "decode password for service={service}, account={account}: {error}" + )) + }) +} + +fn load_shared_password_bytes( + service: &str, + account: &str, +) -> Result>, MacOsAccessGroupError> { + load_password_bytes( + shared_password_options(service, account), + "read shared generic password", + ) +} + +fn load_legacy_password_bytes( + service: &str, + account: &str, +) -> Result>, MacOsAccessGroupError> { + load_password_bytes( + legacy_password_options(service, account), + "read legacy generic password", + ) +} + +fn load_password_bytes( + options: PasswordOptions, + action: &str, +) -> Result>, MacOsAccessGroupError> { + match generic_password(options) { + Ok(password) => Ok(Some(password)), + Err(error) if error.code() == errSecItemNotFound => Ok(None), + Err(error) => Err(wrap_security_error(action, error)), + } +} + +fn save_shared_password_bytes( + service: &str, + account: &str, + value: &[u8], +) -> Result<(), MacOsAccessGroupError> { + set_generic_password_options(value, shared_password_options(service, account)) + .map_err(|error| wrap_security_error("write shared generic password", error)) +} + +fn delete_shared_password(service: &str, account: &str) -> Result { + delete_password_options( + shared_password_options(service, account), + "delete shared generic password", + ) +} + +fn delete_legacy_password(service: &str, account: &str) -> Result { + delete_password_options( + legacy_password_options(service, account), + "delete legacy generic password", + ) +} + +fn delete_password_options( + options: PasswordOptions, + action: &str, +) -> Result { + match delete_generic_password_options(options) { + Ok(()) => Ok(true), + Err(error) if error.code() == errSecItemNotFound => Ok(false), + Err(error) => Err(wrap_security_error(action, error)), + } +} + +fn shared_password_options(service: &str, account: &str) -> PasswordOptions { + let mut options = PasswordOptions::new_generic_password(service, account); + options.use_protected_keychain(); + options.set_access_group(SHARED_ACCESS_GROUP); + options +} + +fn legacy_password_options(service: &str, account: &str) -> PasswordOptions { + PasswordOptions::new_generic_password(service, account) +} + +fn wrap_security_error(action: &str, error: SecurityError) -> MacOsAccessGroupError { + MacOsAccessGroupError::new(format!("{action}: {error}")) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn codex_keychain_services_match_stored_secret_services() { + assert_eq!( + CODEX_KEYCHAIN_SERVICES, + ["Codex Auth", "Codex MCP Credentials", "codex"] + ); + } + + #[test] + fn shared_access_group_matches_configured_team_prefix() { + assert_eq!(SHARED_ACCESS_GROUP, "2DC432GLL2.com.openai.shared"); + } +}