new logic for keyring

Author: celia-oai

codex-rs/Cargo.lock

@@ -45,6 +45,7 @@ reqwest = { workspace = true, features = ["json", "stream"] }
 serde = { workspace = true, features = ["derive"] }
 serde_json = { workspace = true }
 sha1 = { workspace = true }
+sha2 = { workspace = true }
 shlex = { workspace = true }
 similar = { workspace = true }
 strum_macros = { workspace = true }

codex-rs/core/Cargo.toml

@@ -4,6 +4,8 @@ use serde::Deserialize;
 use serde::Serialize;
 #[cfg(test)]
 use serial_test::serial;
+use sha2::Digest;
+use sha2::Sha256;
 use std::env;
 use std::fmt::Debug;
 use std::fs::File;
@@ -17,9 +19,9 @@ use std::path::PathBuf;
 use std::sync::Arc;
 use std::sync::Mutex;
 use std::time::Duration;
+use tracing::warn;
 
 use codex_app_server_protocol::AuthMode;
-use codex_keyring_store::CredentialStoreError;
 use codex_keyring_store::DefaultKeyringStore;
 use codex_keyring_store::KeyringStore;
 use codex_protocol::config_types::ForcedLoginMethod;
@@ -48,6 +50,19 @@ trait AuthStorageBackend: Debug + Send + Sync {
     fn delete(&self) -> std::io::Result<bool>;
 }
 
+fn get_auth_file(codex_home: &Path) -> PathBuf {
+    codex_home.join("auth.json")
+}
+
+fn delete_file_if_exists(codex_home: &Path) -> std::io::Result<bool> {
+    let auth_file = get_auth_file(codex_home);
+    match std::fs::remove_file(&auth_file) {
+        Ok(()) => Ok(true),
+        Err(err) if err.kind() == std::io::ErrorKind::NotFound => Ok(false),
+        Err(err) => Err(err),
+    }
+}
+
 #[derive(Clone, Debug)]
 struct FileAuthStorage {
     codex_home: PathBuf,
@@ -59,7 +74,7 @@ impl FileAuthStorage {
     }
 
     fn load_from_file(&self) -> std::io::Result<Option<AuthDotJson>> {
-        let auth_file = self.get_auth_file(&self.codex_home);
+        let auth_file = get_auth_file(&self.codex_home);
         let auth_dot_json = match self.try_read_auth_json(&auth_file) {
             Ok(auth) => auth,
             Err(err) if err.kind() == std::io::ErrorKind::NotFound => return Ok(None),
@@ -69,16 +84,7 @@ impl FileAuthStorage {
     }
 
     fn save_to_file(&self, auth: &AuthDotJson) -> std::io::Result<()> {
-        self.write_auth_json(&self.get_auth_file(&self.codex_home), auth)
-    }
-
-    fn delete_file_if_exists(&self) -> std::io::Result<bool> {
-        let auth_file = self.get_auth_file(&self.codex_home);
-        match std::fs::remove_file(&auth_file) {
-            Ok(()) => Ok(true),
-            Err(err) if err.kind() == std::io::ErrorKind::NotFound => Ok(false),
-            Err(err) => Err(err),
-        }
+        self.write_auth_json(&get_auth_file(&self.codex_home), auth)
     }
 
     /// Attempt to read and refresh the `auth.json` file in the given `CODEX_HOME` directory.
@@ -92,10 +98,6 @@ impl FileAuthStorage {
         Ok(auth_dot_json)
     }
 
-    fn get_auth_file(&self, codex_home: &Path) -> PathBuf {
-        codex_home.join("auth.json")
-    }
-
     fn write_auth_json(
         &self,
         auth_file: &Path,
@@ -127,66 +129,162 @@ impl AuthStorageBackend for FileAuthStorage {
         self.save_to_file(auth)
     }
     fn delete(&self) -> std::io::Result<bool> {
-        self.delete_file_if_exists()
+        delete_file_if_exists(&self.codex_home)
     }
 }
 
+const KEYRING_SERVICE: &str = "Codex CLI Auth";
+
 #[derive(Clone, Debug)]
 struct KeyringAuthStorage {
     codex_home: PathBuf,
+    keyring_store: Arc<dyn KeyringStore>,
 }
 
 impl KeyringAuthStorage {
-    fn new(codex_home: PathBuf) -> Self {
-        Self { codex_home }
+    fn new(codex_home: PathBuf, keyring_store: Arc<dyn KeyringStore>) -> Self {
+        Self {
+            codex_home,
+            keyring_store,
+        }
+    }
+
+    // turns codex_home path into a stable, short key string
+    fn compute_store_key(&self, codex_home: &Path) -> std::io::Result<String> {
+        let canonical = codex_home
+            .canonicalize()
+            .unwrap_or_else(|_| codex_home.to_path_buf());
+        let path_str = canonical.to_string_lossy();
+        let mut hasher = Sha256::new();
+        hasher.update(path_str.as_bytes());
+        let digest = hasher.finalize();
+        let hex = format!("{digest:x}");
+        let truncated = hex.get(..16).unwrap_or(&hex);
+        Ok(format!("cli|{truncated}"))
+    }
+
+    fn load_from_keyring(&self, key: &str) -> std::io::Result<Option<AuthDotJson>> {
+        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))
+            }
+        }
     }
 }
 
 impl AuthStorageBackend for KeyringAuthStorage {
     fn load(&self) -> std::io::Result<Option<AuthDotJson>> {
-        Ok(None)
+        let key = self.compute_store_key(&self.codex_home)?;
+        self.load_from_keyring(&key)
     }
 
     fn save(&self, auth: &AuthDotJson) -> std::io::Result<()> {
+        let key = self.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 let Err(err) = delete_file_if_exists(&self.codex_home) {
+            warn!("failed to remove CLI auth fallback file: {err}");
+        }
         Ok(())
     }
+
     fn delete(&self) -> std::io::Result<bool> {
-        Ok(false)
+        let key = self.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 file_removed = delete_file_if_exists(&self.codex_home)?;
+        Ok(keyring_removed || file_removed)
     }
 }
 
 #[derive(Clone, Debug)]
 struct AutoAuthStorage {
-    codex_home: PathBuf,
+    keyring_storage: Arc<KeyringAuthStorage>,
+    file_storage: Arc<FileAuthStorage>,
 }
 
 impl AutoAuthStorage {
-    fn new(codex_home: PathBuf) -> Self {
-        Self { codex_home }
+    fn new(codex_home: PathBuf, keyring_store: Arc<dyn KeyringStore>) -> Self {
+        Self {
+            keyring_storage: Arc::new(KeyringAuthStorage::new(codex_home.clone(), keyring_store)),
+            file_storage: Arc::new(FileAuthStorage::new(codex_home.clone())),
+        }
     }
 }
 
 impl AuthStorageBackend for AutoAuthStorage {
     fn load(&self) -> std::io::Result<Option<AuthDotJson>> {
-        Ok(None)
+        match self.keyring_storage.load() {
+            Ok(Some(auth)) => Ok(Some(auth)),
+            Ok(None) => self.file_storage.load(),
+            Err(err) => {
+                warn!("failed to load CLI auth from keyring, falling back to file storage: {err}");
+                self.file_storage.load()
+            }
+        }
     }
 
     fn save(&self, auth: &AuthDotJson) -> std::io::Result<()> {
-        Ok(())
+        match self.keyring_storage.save(auth) {
+            Ok(()) => Ok(()),
+            Err(err) => {
+                warn!("failed to save auth to keyring, falling back to file storage: {err}");
+                return self.file_storage.save(auth);
+            }
+        }
     }
+
     fn delete(&self) -> std::io::Result<bool> {
-        Ok(false)
+        // Keyring storage will delete from disk as well
+        self.keyring_storage.delete()
     }
 }
 
 fn create_auth_storage(
     codex_home: PathBuf,
     mode: AuthCredentialsStoreMode,
+) -> Arc<dyn AuthStorageBackend> {
+    let keyring_store: Arc<dyn KeyringStore> = Arc::new(DefaultKeyringStore);
+    create_auth_storage_with_keyring_store(codex_home, mode, keyring_store)
+}
+
+fn create_auth_storage_with_keyring_store(
+    codex_home: PathBuf,
+    mode: AuthCredentialsStoreMode,
+    keyring_store: Arc<dyn KeyringStore>,
 ) -> Arc<dyn AuthStorageBackend> {
     match mode {
         AuthCredentialsStoreMode::File => Arc::new(FileAuthStorage::new(codex_home)),
-        AuthCredentialsStoreMode::Keyring => Arc::new(KeyringAuthStorage::new(codex_home)),
-        AuthCredentialsStoreMode::Auto => Arc::new(AutoAuthStorage::new(codex_home)),
+        AuthCredentialsStoreMode::Keyring => {
+            Arc::new(KeyringAuthStorage::new(codex_home, keyring_store))
+        }
+        AuthCredentialsStoreMode::Auto => Arc::new(AutoAuthStorage::new(codex_home, keyring_store)),
     }
 }
 
@@ -645,7 +743,7 @@ mod tests {
         )
         .expect("failed to write auth file");
 
-        let file = storage.get_auth_file(codex_home.path());
+        let file = get_auth_file(codex_home.path());
         let auth_dot_json = storage.try_read_auth_json(&file).unwrap();
         storage.write_auth_json(&file, &auth_dot_json).unwrap();
 
@@ -781,8 +879,7 @@ mod tests {
     }
 
     fn write_auth_file(params: AuthFileParams, codex_home: &Path) -> std::io::Result<String> {
-        let storage = FileAuthStorage::new(codex_home.to_path_buf());
-        let auth_file = storage.get_auth_file(codex_home);
+        let auth_file = get_auth_file(codex_home);
         // Create a minimal valid JWT for the id_token field.
         #[derive(Serialize)]
         struct Header {

codex-rs/core/src/auth.rs

@@ -2,6 +2,7 @@ use anyhow::Error as AnyhowError;
 use keyring::Entry;
 use std::error::Error;
 use std::fmt;
+use std::fmt::Debug;
 
 #[derive(Debug)]
 pub struct CredentialStoreError(AnyhowError);
@@ -26,12 +27,13 @@ impl fmt::Display for CredentialStoreError {
 impl Error for CredentialStoreError {}
 
 /// Shared credential store abstraction for keyring-backed implementations.
-pub trait KeyringStore: Send + Sync {
+pub trait KeyringStore: Debug + Send + Sync {
     fn load(&self, service: &str, account: &str) -> Result<Option<String>, CredentialStoreError>;
     fn save(&self, service: &str, account: &str, value: &str) -> Result<(), CredentialStoreError>;
     fn delete(&self, service: &str, account: &str) -> Result<bool, CredentialStoreError>;
 }
 
+#[derive(Debug)]
 pub struct DefaultKeyringStore;
 
 impl KeyringStore for DefaultKeyringStore {
@@ -69,7 +71,7 @@ pub mod testing {
     use std::sync::Arc;
     use std::sync::Mutex;
 
-    #[derive(Default, Clone)]
+    #[derive(Default, Clone, Debug)]
     pub struct MockKeyringStore {
         credentials: Arc<Mutex<HashMap<String, Arc<MockCredential>>>>,
     }