diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 51a6cd08cc..2c23dbb2e9 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -3189,6 +3189,7 @@ dependencies = [ "codex-model-provider-info", "codex-otel", "codex-protocol", + "codex-secrets", "codex-terminal-detection", "codex-utils-template", "core_test_support", diff --git a/codex-rs/login/Cargo.toml b/codex-rs/login/Cargo.toml index e914fa0c7a..d790db674f 100644 --- a/codex-rs/login/Cargo.toml +++ b/codex-rs/login/Cargo.toml @@ -19,6 +19,7 @@ codex-keyring-store = { workspace = true } codex-model-provider-info = { workspace = true } codex-otel = { workspace = true } codex-protocol = { workspace = true } +codex-secrets = { workspace = true } codex-terminal-detection = { workspace = true } codex-utils-template = { workspace = true } once_cell = { workspace = true } diff --git a/codex-rs/login/src/auth/storage.rs b/codex-rs/login/src/auth/storage.rs index d0e606a8cc..2add9d52eb 100644 --- a/codex-rs/login/src/auth/storage.rs +++ b/codex-rs/login/src/auth/storage.rs @@ -26,6 +26,9 @@ use codex_config::types::AuthCredentialsStoreMode; use codex_keyring_store::DefaultKeyringStore; use codex_keyring_store::KeyringStore; use codex_protocol::account::PlanType as AccountPlanType; +use codex_secrets::LocalSecretsBackend; +use codex_secrets::SecretName; +use codex_secrets::SecretScope; use once_cell::sync::Lazy; /// Expected structure for $CODEX_HOME/auth.json. @@ -160,7 +163,10 @@ impl AuthStorageBackend for FileAuthStorage { } } -const KEYRING_SERVICE: &str = "Codex Auth"; +static CLI_AUTH_SECRET_NAME: Lazy = Lazy::new(|| match SecretName::new("CLI_AUTH") { + Ok(name) => name, + Err(err) => unreachable!("CLI_AUTH should be a valid secret name: {err}"), +}); // turns codex_home path into a stable, short key string fn compute_store_key(codex_home: &Path) -> std::io::Result { @@ -179,58 +185,40 @@ fn compute_store_key(codex_home: &Path) -> std::io::Result { #[derive(Clone, Debug)] struct KeyringAuthStorage { codex_home: PathBuf, - keyring_store: Arc, + secrets_backend: LocalSecretsBackend, } impl KeyringAuthStorage { fn new(codex_home: PathBuf, keyring_store: Arc) -> Self { + let secrets_backend = LocalSecretsBackend::new(codex_home.clone(), keyring_store); Self { codex_home, - keyring_store, - } - } - - fn load_from_keyring(&self, key: &str) -> std::io::Result> { - match self.keyring_store.load(KEYRING_SERVICE, key) { - Ok(Some(serialized)) => serde_json::from_str(&serialized).map(Some).map_err(|err| { - std::io::Error::other(format!( - "failed to deserialize CLI auth from keyring: {err}" - )) - }), - Ok(None) => Ok(None), - Err(error) => Err(std::io::Error::other(format!( - "failed to load CLI auth from keyring: {}", - error.message() - ))), - } - } - - fn save_to_keyring(&self, key: &str, value: &str) -> std::io::Result<()> { - match self.keyring_store.save(KEYRING_SERVICE, key, value) { - Ok(()) => Ok(()), - Err(error) => { - let message = format!( - "failed to write OAuth tokens to keyring: {}", - error.message() - ); - warn!("{message}"); - Err(std::io::Error::other(message)) - } + secrets_backend, } } } impl AuthStorageBackend for KeyringAuthStorage { fn load(&self) -> std::io::Result> { - let key = compute_store_key(&self.codex_home)?; - self.load_from_keyring(&key) + match self + .secrets_backend + .get(&SecretScope::Global, &CLI_AUTH_SECRET_NAME) + .map_err(std::io::Error::other)? + { + Some(serialized) => serde_json::from_str(&serialized).map(Some).map_err(|err| { + std::io::Error::other(format!( + "failed to deserialize CLI auth from encrypted auth storage: {err}" + )) + }), + None => Ok(None), + } } fn save(&self, auth: &AuthDotJson) -> std::io::Result<()> { - let key = compute_store_key(&self.codex_home)?; - // Simpler error mapping per style: prefer method reference over closure let serialized = serde_json::to_string(auth).map_err(std::io::Error::other)?; - self.save_to_keyring(&key, &serialized)?; + self.secrets_backend + .set(&SecretScope::Global, &CLI_AUTH_SECRET_NAME, &serialized) + .map_err(std::io::Error::other)?; if let Err(err) = delete_file_if_exists(&self.codex_home) { warn!("failed to remove CLI auth fallback file: {err}"); } @@ -238,13 +226,10 @@ impl AuthStorageBackend for KeyringAuthStorage { } fn delete(&self) -> std::io::Result { - let key = compute_store_key(&self.codex_home)?; let keyring_removed = self - .keyring_store - .delete(KEYRING_SERVICE, &key) - .map_err(|err| { - std::io::Error::other(format!("failed to delete auth from keyring: {err}")) - })?; + .secrets_backend + .delete(&SecretScope::Global, &CLI_AUTH_SECRET_NAME) + .map_err(std::io::Error::other)?; let file_removed = delete_file_if_exists(&self.codex_home)?; Ok(keyring_removed || file_removed) } diff --git a/codex-rs/login/src/auth/storage_tests.rs b/codex-rs/login/src/auth/storage_tests.rs index debe0e18d3..f81fc73354 100644 --- a/codex-rs/login/src/auth/storage_tests.rs +++ b/codex-rs/login/src/auth/storage_tests.rs @@ -2,6 +2,8 @@ use super::*; use crate::token_data::IdTokenInfo; use anyhow::Context; use base64::Engine; +use codex_secrets::LocalSecretsBackend; +use codex_secrets::SecretScope; use pretty_assertions::assert_eq; use serde_json::json; use tempfile::tempdir; @@ -185,51 +187,75 @@ fn ephemeral_storage_save_load_delete_is_in_memory_only() -> anyhow::Result<()> Ok(()) } -fn seed_keyring_and_fallback_auth_file_for_delete( +fn seed_keyring_and_fallback_auth_file_for_delete( mock_keyring: &MockKeyringStore, codex_home: &Path, - compute_key: F, -) -> anyhow::Result<(String, PathBuf)> -where - F: FnOnce() -> std::io::Result, -{ - let key = compute_key()?; - mock_keyring.save(KEYRING_SERVICE, &key, "{}")?; + auth: &AuthDotJson, +) -> anyhow::Result { + let backend = + LocalSecretsBackend::new(codex_home.to_path_buf(), Arc::new(mock_keyring.clone())); + backend.set( + &SecretScope::Global, + &CLI_AUTH_SECRET_NAME, + &serde_json::to_string(auth)?, + )?; let auth_file = get_auth_file(codex_home); std::fs::write(&auth_file, "stale")?; - Ok((key, auth_file)) + Ok(auth_file) } -fn seed_keyring_with_auth( +fn seed_keyring_with_auth( mock_keyring: &MockKeyringStore, - compute_key: F, + codex_home: &Path, auth: &AuthDotJson, -) -> anyhow::Result<()> -where - F: FnOnce() -> std::io::Result, -{ - let key = compute_key()?; - let serialized = serde_json::to_string(auth)?; - mock_keyring.save(KEYRING_SERVICE, &key, &serialized)?; +) -> anyhow::Result<()> { + let backend = + LocalSecretsBackend::new(codex_home.to_path_buf(), Arc::new(mock_keyring.clone())); + backend.set( + &SecretScope::Global, + &CLI_AUTH_SECRET_NAME, + &serde_json::to_string(auth)?, + )?; Ok(()) } fn assert_keyring_saved_auth_and_removed_fallback( mock_keyring: &MockKeyringStore, - key: &str, codex_home: &Path, expected: &AuthDotJson, -) { - let saved_value = mock_keyring - .saved_value(key) - .expect("keyring entry should exist"); - let expected_serialized = serde_json::to_string(expected).expect("serialize expected auth"); +) -> anyhow::Result<()> { + let backend = + LocalSecretsBackend::new(codex_home.to_path_buf(), Arc::new(mock_keyring.clone())); + let saved_value = backend + .get(&SecretScope::Global, &CLI_AUTH_SECRET_NAME)? + .context("encrypted auth entry should exist")?; + let expected_serialized = serde_json::to_string(expected)?; assert_eq!(saved_value, expected_serialized); + let old_key = compute_store_key(codex_home)?; + assert!( + mock_keyring.saved_value(&old_key).is_none(), + "legacy keyring auth entry should not be used" + ); + let secrets_key = compute_secrets_keyring_account(codex_home)?; + assert!( + mock_keyring.saved_value(&secrets_key).is_some(), + "secrets backend should persist an encryption passphrase in the keyring" + ); + assert!(encrypted_auth_file(codex_home).exists()); let auth_file = get_auth_file(codex_home); assert!( !auth_file.exists(), "fallback auth.json should be removed after keyring save" ); + Ok(()) +} + +fn encrypted_auth_file(codex_home: &Path) -> PathBuf { + codex_home.join("secrets").join("local.age") +} + +fn compute_secrets_keyring_account(codex_home: &Path) -> anyhow::Result { + Ok(compute_store_key(codex_home)?.replacen("cli|", "secrets|", 1)) } fn id_token_with_prefix(prefix: &str) -> IdTokenInfo { @@ -298,11 +324,7 @@ fn keyring_auth_storage_load_returns_deserialized_auth() -> anyhow::Result<()> { agent_identity: None, personal_access_token: None, }; - seed_keyring_with_auth( - &mock_keyring, - || compute_store_key(codex_home.path()), - &expected, - )?; + seed_keyring_with_auth(&mock_keyring, codex_home.path(), &expected)?; let loaded = storage.load()?; assert_eq!(Some(expected), loaded); @@ -345,8 +367,7 @@ fn keyring_auth_storage_save_persists_and_removes_fallback_file() -> anyhow::Res storage.save(&auth)?; - let key = compute_store_key(codex_home.path())?; - assert_keyring_saved_auth_and_removed_fallback(&mock_keyring, &key, codex_home.path(), &auth); + assert_keyring_saved_auth_and_removed_fallback(&mock_keyring, codex_home.path(), &auth)?; Ok(()) } @@ -358,18 +379,14 @@ fn keyring_auth_storage_delete_removes_keyring_and_file() -> anyhow::Result<()> codex_home.path().to_path_buf(), Arc::new(mock_keyring.clone()), ); - let (key, auth_file) = - seed_keyring_and_fallback_auth_file_for_delete(&mock_keyring, codex_home.path(), || { - compute_store_key(codex_home.path()) - })?; + let auth = auth_with_prefix("to-delete"); + let auth_file = + seed_keyring_and_fallback_auth_file_for_delete(&mock_keyring, codex_home.path(), &auth)?; let removed = storage.delete()?; assert!(removed, "delete should report removal"); - assert!( - !mock_keyring.contains(&key), - "keyring entry should be removed" - ); + assert_eq!(storage.load()?, None, "encrypted auth should be removed"); assert!( !auth_file.exists(), "fallback auth.json should be removed after keyring delete" @@ -386,11 +403,7 @@ fn auto_auth_storage_load_prefers_keyring_value() -> anyhow::Result<()> { Arc::new(mock_keyring.clone()), ); let keyring_auth = auth_with_prefix("keyring"); - seed_keyring_with_auth( - &mock_keyring, - || compute_store_key(codex_home.path()), - &keyring_auth, - )?; + seed_keyring_with_auth(&mock_keyring, codex_home.path(), &keyring_auth)?; let file_auth = auth_with_prefix("file"); storage.file_storage.save(&file_auth)?; @@ -422,7 +435,10 @@ fn auto_auth_storage_load_falls_back_when_keyring_errors() -> anyhow::Result<()> codex_home.path().to_path_buf(), Arc::new(mock_keyring.clone()), ); - let key = compute_store_key(codex_home.path())?; + let key = compute_secrets_keyring_account(codex_home.path())?; + + let encrypted = auth_with_prefix("encrypted"); + seed_keyring_with_auth(&mock_keyring, codex_home.path(), &encrypted)?; mock_keyring.set_error(&key, KeyringError::Invalid("error".into(), "load".into())); let expected = auth_with_prefix("fallback"); @@ -441,20 +457,13 @@ fn auto_auth_storage_save_prefers_keyring() -> anyhow::Result<()> { codex_home.path().to_path_buf(), Arc::new(mock_keyring.clone()), ); - let key = compute_store_key(codex_home.path())?; - let stale = auth_with_prefix("stale"); storage.file_storage.save(&stale)?; let expected = auth_with_prefix("to-save"); storage.save(&expected)?; - assert_keyring_saved_auth_and_removed_fallback( - &mock_keyring, - &key, - codex_home.path(), - &expected, - ); + assert_keyring_saved_auth_and_removed_fallback(&mock_keyring, codex_home.path(), &expected)?; Ok(()) } @@ -466,7 +475,7 @@ fn auto_auth_storage_save_falls_back_when_keyring_errors() -> anyhow::Result<()> codex_home.path().to_path_buf(), Arc::new(mock_keyring.clone()), ); - let key = compute_store_key(codex_home.path())?; + let key = compute_secrets_keyring_account(codex_home.path())?; mock_keyring.set_error(&key, KeyringError::Invalid("error".into(), "save".into())); let auth = auth_with_prefix("fallback"); @@ -497,18 +506,14 @@ fn auto_auth_storage_delete_removes_keyring_and_file() -> anyhow::Result<()> { codex_home.path().to_path_buf(), Arc::new(mock_keyring.clone()), ); - let (key, auth_file) = - seed_keyring_and_fallback_auth_file_for_delete(&mock_keyring, codex_home.path(), || { - compute_store_key(codex_home.path()) - })?; + let auth = auth_with_prefix("to-delete"); + let auth_file = + seed_keyring_and_fallback_auth_file_for_delete(&mock_keyring, codex_home.path(), &auth)?; let removed = storage.delete()?; assert!(removed, "delete should report removal"); - assert!( - !mock_keyring.contains(&key), - "keyring entry should be removed" - ); + assert_eq!(storage.load()?, None, "encrypted auth should be removed"); assert!( !auth_file.exists(), "fallback auth.json should be removed after delete"