From ff0039b7aec2658f6b402e091349550211da63b9 Mon Sep 17 00:00:00 2001 From: mikhail-oai Date: Mon, 13 Apr 2026 11:34:46 -0400 Subject: [PATCH] split Windows auth keyring storage --- codex-rs/login/src/auth/storage.rs | 152 ++++++++++++++++++-- codex-rs/login/src/auth/storage_tests.rs | 168 +++++++++++++++++++++++ 2 files changed, 310 insertions(+), 10 deletions(-) diff --git a/codex-rs/login/src/auth/storage.rs b/codex-rs/login/src/auth/storage.rs index 97e801415c..f9b640ae38 100644 --- a/codex-rs/login/src/auth/storage.rs +++ b/codex-rs/login/src/auth/storage.rs @@ -2,6 +2,7 @@ use chrono::DateTime; use chrono::Utc; use serde::Deserialize; use serde::Serialize; +use serde::de::DeserializeOwned; use sha2::Digest; use sha2::Sha256; use std::collections::HashMap; @@ -119,6 +120,14 @@ impl AuthStorageBackend for FileAuthStorage { const KEYRING_SERVICE: &str = "Codex Auth"; +// Const for each field of AuthDotJson, only when used on windows +// if more fields are added to struct, then update these consts +// and windows storage functions. +const AUTH_MODE_FIELD: &str = "auth_mode"; +const OPENAI_API_KEY_FIELD: &str = "openai_api_key"; +const TOKENS_FIELD: &str = "tokens"; +const LAST_REFRESH_FIELD: &str = "last_refresh"; + // turns codex_home path into a stable, short key string fn compute_store_key(codex_home: &Path) -> std::io::Result { let canonical = codex_home @@ -175,19 +184,139 @@ impl KeyringAuthStorage { } } } + + fn field_key(key: &str, field_name: &str) -> String { + format!("{key}|{field_name}") + } + + fn save_windows_auth_to_keyring(&self, key: &str, auth: &AuthDotJson) -> std::io::Result<()> { + let fields = [ + ( + AUTH_MODE_FIELD, + serde_json::to_string(&auth.auth_mode).map_err(std::io::Error::other)?, + ), + ( + OPENAI_API_KEY_FIELD, + serde_json::to_string(&auth.openai_api_key).map_err(std::io::Error::other)?, + ), + ( + TOKENS_FIELD, + serde_json::to_string(&auth.tokens).map_err(std::io::Error::other)?, + ), + ( + LAST_REFRESH_FIELD, + serde_json::to_string(&auth.last_refresh).map_err(std::io::Error::other)?, + ), + ]; + + for (field_name, serialized) in fields { + self.save_to_keyring(&Self::field_key(key, field_name), &serialized)?; + } + + if let Err(err) = self.keyring_store.delete(KEYRING_SERVICE, key) { + warn!("failed to remove legacy CLI auth from keyring: {err}"); + } + Ok(()) + } + + fn load_windows_auth_from_keyring(&self, key: &str) -> std::io::Result> { + fn load_auth_field_from_keyring( + keyring_store: &dyn KeyringStore, + key: &str, + ) -> std::io::Result> + where + T: DeserializeOwned, + { + match 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 field from keyring: {err}" + )) + }) + } + Ok(None) => Ok(None), + Err(error) => Err(std::io::Error::other(format!( + "failed to load CLI auth from keyring: {}", + error.message() + ))), + } + } + + let auth_mode = load_auth_field_from_keyring::>( + self.keyring_store.as_ref(), + &Self::field_key(key, AUTH_MODE_FIELD), + )?; + let openai_api_key = load_auth_field_from_keyring::>( + self.keyring_store.as_ref(), + &Self::field_key(key, OPENAI_API_KEY_FIELD), + )?; + let tokens = load_auth_field_from_keyring::>( + self.keyring_store.as_ref(), + &Self::field_key(key, TOKENS_FIELD), + )?; + let last_refresh = load_auth_field_from_keyring::>>( + self.keyring_store.as_ref(), + &Self::field_key(key, LAST_REFRESH_FIELD), + )?; + + if auth_mode.is_none() + && openai_api_key.is_none() + && tokens.is_none() + && last_refresh.is_none() + { + return Ok(None); + } + + Ok(Some(AuthDotJson { + auth_mode: auth_mode.flatten(), + openai_api_key: openai_api_key.flatten(), + tokens: tokens.flatten(), + last_refresh: last_refresh.flatten(), + })) + } + + fn delete_windows_auth_from_keyring(&self, key: &str) -> std::io::Result { + let field_keys = [ + Self::field_key(key, AUTH_MODE_FIELD), + Self::field_key(key, OPENAI_API_KEY_FIELD), + Self::field_key(key, TOKENS_FIELD), + Self::field_key(key, LAST_REFRESH_FIELD), + key.to_string(), + ]; + let mut removed = false; + + for field_key in field_keys { + removed |= self + .keyring_store + .delete(KEYRING_SERVICE, &field_key) + .map_err(|err| { + std::io::Error::other(format!("failed to delete auth from keyring: {err}")) + })?; + } + + Ok(removed) + } } impl AuthStorageBackend for KeyringAuthStorage { fn load(&self) -> std::io::Result> { let key = compute_store_key(&self.codex_home)?; - self.load_from_keyring(&key) + if cfg!(windows) { + self.load_windows_auth_from_keyring(&key) + } else { + self.load_from_keyring(&key) + } } 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)?; + if cfg!(windows) { + self.save_windows_auth_to_keyring(&key, auth)?; + } else { + let serialized = serde_json::to_string(auth).map_err(std::io::Error::other)?; + self.save_to_keyring(&key, &serialized)?; + } if let Err(err) = delete_file_if_exists(&self.codex_home) { warn!("failed to remove CLI auth fallback file: {err}"); } @@ -196,12 +325,15 @@ 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}")) - })?; + let keyring_removed = if cfg!(windows) { + self.delete_windows_auth_from_keyring(&key)? + } else { + self.keyring_store + .delete(KEYRING_SERVICE, &key) + .map_err(|err| { + std::io::Error::other(format!("failed to delete auth from keyring: {err}")) + })? + }; 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 4bf72c11b9..5f98650459 100644 --- a/codex-rs/login/src/auth/storage_tests.rs +++ b/codex-rs/login/src/auth/storage_tests.rs @@ -126,6 +126,34 @@ where Ok(()) } +fn seed_windows_keyring_with_auth( + mock_keyring: &MockKeyringStore, + key: &str, + auth: &AuthDotJson, +) -> anyhow::Result<()> { + mock_keyring.save( + KEYRING_SERVICE, + &KeyringAuthStorage::field_key(key, AUTH_MODE_FIELD), + &serde_json::to_string(&auth.auth_mode)?, + )?; + mock_keyring.save( + KEYRING_SERVICE, + &KeyringAuthStorage::field_key(key, OPENAI_API_KEY_FIELD), + &serde_json::to_string(&auth.openai_api_key)?, + )?; + mock_keyring.save( + KEYRING_SERVICE, + &KeyringAuthStorage::field_key(key, TOKENS_FIELD), + &serde_json::to_string(&auth.tokens)?, + )?; + mock_keyring.save( + KEYRING_SERVICE, + &KeyringAuthStorage::field_key(key, LAST_REFRESH_FIELD), + &serde_json::to_string(&auth.last_refresh)?, + )?; + Ok(()) +} + fn assert_keyring_saved_auth_and_removed_fallback( mock_keyring: &MockKeyringStore, key: &str, @@ -144,6 +172,44 @@ fn assert_keyring_saved_auth_and_removed_fallback( ); } +fn assert_windows_keyring_saved_auth_and_removed_fallback( + mock_keyring: &MockKeyringStore, + key: &str, + codex_home: &Path, + expected: &AuthDotJson, +) { + let auth_mode_key = KeyringAuthStorage::field_key(key, AUTH_MODE_FIELD); + let openai_api_key_key = KeyringAuthStorage::field_key(key, OPENAI_API_KEY_FIELD); + let tokens_key = KeyringAuthStorage::field_key(key, TOKENS_FIELD); + let last_refresh_key = KeyringAuthStorage::field_key(key, LAST_REFRESH_FIELD); + + assert_eq!( + mock_keyring.saved_value(&auth_mode_key), + Some(serde_json::to_string(&expected.auth_mode).expect("serialize auth_mode")), + ); + assert_eq!( + mock_keyring.saved_value(&openai_api_key_key), + Some(serde_json::to_string(&expected.openai_api_key).expect("serialize openai_api_key")), + ); + assert_eq!( + mock_keyring.saved_value(&tokens_key), + Some(serde_json::to_string(&expected.tokens).expect("serialize tokens")), + ); + assert_eq!( + mock_keyring.saved_value(&last_refresh_key), + Some(serde_json::to_string(&expected.last_refresh).expect("serialize last_refresh")), + ); + assert!( + mock_keyring.saved_value(key).is_none(), + "legacy whole-auth key should be removed after windows keyring save" + ); + let auth_file = get_auth_file(codex_home); + assert!( + !auth_file.exists(), + "fallback auth.json should be removed after keyring save" + ); +} + fn id_token_with_prefix(prefix: &str) -> IdTokenInfo { #[derive(Serialize)] struct Header { @@ -275,6 +341,108 @@ fn keyring_auth_storage_delete_removes_keyring_and_file() -> anyhow::Result<()> Ok(()) } +#[test] +fn keyring_auth_storage_windows_load_combines_split_entries() -> anyhow::Result<()> { + let codex_home = tempdir()?; + let mock_keyring = MockKeyringStore::default(); + let storage = KeyringAuthStorage::new( + codex_home.path().to_path_buf(), + Arc::new(mock_keyring.clone()), + ); + let expected = auth_with_prefix("windows-load"); + let key = compute_store_key(codex_home.path())?; + seed_windows_keyring_with_auth(&mock_keyring, &key, &expected)?; + + let loaded = storage.load_windows_auth_from_keyring(&key)?; + + assert_eq!(Some(expected), loaded); + Ok(()) +} + +#[test] +fn keyring_auth_storage_windows_load_ignores_legacy_entry() -> anyhow::Result<()> { + let codex_home = tempdir()?; + let mock_keyring = MockKeyringStore::default(); + let storage = KeyringAuthStorage::new( + codex_home.path().to_path_buf(), + Arc::new(mock_keyring.clone()), + ); + let expected = auth_with_prefix("windows-legacy"); + let key = compute_store_key(codex_home.path())?; + seed_keyring_with_auth(&mock_keyring, || Ok(key.clone()), &expected)?; + + let loaded = storage.load_windows_auth_from_keyring(&key)?; + + assert_eq!(None, loaded); + Ok(()) +} + +#[test] +fn keyring_auth_storage_windows_save_persists_split_entries() -> anyhow::Result<()> { + let codex_home = tempdir()?; + let mock_keyring = MockKeyringStore::default(); + let storage = KeyringAuthStorage::new( + codex_home.path().to_path_buf(), + Arc::new(mock_keyring.clone()), + ); + let auth_file = get_auth_file(codex_home.path()); + std::fs::write(&auth_file, "stale")?; + let auth = auth_with_prefix("windows-save"); + let key = compute_store_key(codex_home.path())?; + mock_keyring.save(KEYRING_SERVICE, &key, "legacy-auth")?; + + storage.save_windows_auth_to_keyring(&key, &auth)?; + if let Err(err) = delete_file_if_exists(codex_home.path()) { + panic!("failed to remove fallback auth file in test: {err}"); + } + + assert_windows_keyring_saved_auth_and_removed_fallback( + &mock_keyring, + &key, + codex_home.path(), + &auth, + ); + Ok(()) +} + +#[test] +fn keyring_auth_storage_windows_delete_removes_split_entries_and_file() -> anyhow::Result<()> { + let codex_home = tempdir()?; + let mock_keyring = MockKeyringStore::default(); + let storage = KeyringAuthStorage::new( + codex_home.path().to_path_buf(), + Arc::new(mock_keyring.clone()), + ); + let key = compute_store_key(codex_home.path())?; + let auth = auth_with_prefix("windows-delete"); + seed_windows_keyring_with_auth(&mock_keyring, &key, &auth)?; + mock_keyring.save(KEYRING_SERVICE, &key, "legacy-auth")?; + let auth_file = get_auth_file(codex_home.path()); + std::fs::write(&auth_file, "stale")?; + + let removed = storage.delete_windows_auth_from_keyring(&key)?; + let file_removed = delete_file_if_exists(codex_home.path())?; + + assert!(removed, "delete should report removal"); + assert!(file_removed, "fallback auth.json should be removed"); + assert!(!mock_keyring.contains(&KeyringAuthStorage::field_key( + key.as_str(), + AUTH_MODE_FIELD + ))); + assert!(!mock_keyring.contains(&KeyringAuthStorage::field_key( + key.as_str(), + OPENAI_API_KEY_FIELD, + ))); + assert!(!mock_keyring.contains(&KeyringAuthStorage::field_key(key.as_str(), TOKENS_FIELD))); + assert!(!mock_keyring.contains(&KeyringAuthStorage::field_key( + key.as_str(), + LAST_REFRESH_FIELD, + ))); + assert!(!mock_keyring.contains(&key)); + assert!(!auth_file.exists()); + Ok(()) +} + #[test] fn auto_auth_storage_load_prefers_keyring_value() -> anyhow::Result<()> { let codex_home = tempdir()?;