From 7e4a98d4b61f93da26f72891c4b12f5549b7ad5a Mon Sep 17 00:00:00 2001 From: Prem Pillai Date: Thu, 10 Jul 2025 16:41:15 +1000 Subject: [PATCH] refactor: abstract keyring logic to better enable DI (#3262) --- Cargo.lock | 1 + Justfile | 37 +- crates/goose-mcp/Cargo.toml | 1 + crates/goose-mcp/src/google_drive/mod.rs | 4 + .../goose-mcp/src/google_drive/oauth_pkce.rs | 16 +- crates/goose-mcp/src/google_drive/storage.rs | 98 +++-- crates/goose/src/config/base.rs | 311 +++++++++++----- crates/goose/src/keyring/factory.rs | 345 ++++++++++++++++++ crates/goose/src/keyring/file.rs | 306 ++++++++++++++++ crates/goose/src/keyring/mock.rs | 71 ++++ crates/goose/src/keyring/mod.rs | 66 ++++ crates/goose/src/keyring/system.rs | 44 +++ crates/goose/src/lib.rs | 1 + crates/goose/src/providers/factory.rs | 188 +++++----- 14 files changed, 1278 insertions(+), 211 deletions(-) create mode 100644 crates/goose/src/keyring/factory.rs create mode 100644 crates/goose/src/keyring/file.rs create mode 100644 crates/goose/src/keyring/mock.rs create mode 100644 crates/goose/src/keyring/mod.rs create mode 100644 crates/goose/src/keyring/system.rs diff --git a/Cargo.lock b/Cargo.lock index 9541e0b7..cadd579d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3629,6 +3629,7 @@ dependencies = [ "google-docs1", "google-drive3", "google-sheets4", + "goose", "http-body-util", "hyper 1.6.0", "ignore", diff --git a/Justfile b/Justfile index 9641cf28..91091d43 100644 --- a/Justfile +++ b/Justfile @@ -177,6 +177,33 @@ run-server: @echo "Running server..." cargo run -p goose-server +# Run tests across all crates with keyring disabled +test: + @echo "Running tests with keyring disabled..." + GOOSE_DISABLE_KEYRING=1 cargo test --workspace + +# Run linting checks across all crates +lint: + @echo "Running linting checks..." + cargo clippy --workspace --all-features -- -D warnings + +# Run comprehensive linting checks (includes tests, examples, benchmarks) +lint-all: + @echo "Running comprehensive linting checks..." + cargo clippy --workspace --all-targets --all-features -- -D warnings + +# Format code across all crates +format: + @echo "Formatting code..." + cargo fmt --all + +# Quality assurance: format, lint, and test everything +qa: + @echo "Running quality assurance: format, lint, and test..." + @just format + @just lint + @just test + # make GUI with latest binary lint-ui: cd ui/desktop && npm run lint:check @@ -355,16 +382,16 @@ set windows-shell := ["powershell.exe", "-NoLogo", "-Command"] ### Build the core code ### profile = --release or "" for debug ### allparam = OR/AND/ANY/NONE --workspace --all-features --all-targets -win-bld profile allparam: +win-bld profile allparam: cargo run {{profile}} -p goose-server --bin generate_schema cargo build {{profile}} {{allparam}} ### Build just debug -win-bld-dbg: +win-bld-dbg: just win-bld " " " " ### Build debug and test, examples,... -win-bld-dbg-all: +win-bld-dbg-all: just win-bld " " "--workspace --all-targets --all-features" ### Build just release @@ -427,8 +454,8 @@ win-total-rls *allparam: just win-bld-rls{{allparam}} just win-run-rls -### Build and run the Kotlin example with -### auto-generated bindings for goose-llm +### Build and run the Kotlin example with +### auto-generated bindings for goose-llm kotlin-example: # Build Rust dylib and generate Kotlin bindings cargo build -p goose-llm diff --git a/crates/goose-mcp/Cargo.toml b/crates/goose-mcp/Cargo.toml index b6b77d01..6b76d875 100644 --- a/crates/goose-mcp/Cargo.toml +++ b/crates/goose-mcp/Cargo.toml @@ -13,6 +13,7 @@ workspace = true [dependencies] mcp-core = { path = "../mcp-core" } mcp-server = { path = "../mcp-server" } +goose = { path = "../goose" } anyhow = "1.0.94" tokio = { version = "1", features = ["full"] } tracing = "0.1" diff --git a/crates/goose-mcp/src/google_drive/mod.rs b/crates/goose-mcp/src/google_drive/mod.rs index 1f1aeae7..cf0cf0f4 100644 --- a/crates/goose-mcp/src/google_drive/mod.rs +++ b/crates/goose-mcp/src/google_drive/mod.rs @@ -161,12 +161,16 @@ impl GoogleDriveRouter { Err(_) => false, }; + // Use factory to create keyring backend consistently + let keyring = goose::keyring::create_default_keyring(); + // Create a credentials manager for storing tokens securely let credentials_manager = Arc::new(CredentialsManager::new( credentials_path.clone(), fallback_to_disk, KEYCHAIN_SERVICE.to_string(), KEYCHAIN_USERNAME.to_string(), + keyring, )); // Read the OAuth credentials from the keyfile diff --git a/crates/goose-mcp/src/google_drive/oauth_pkce.rs b/crates/goose-mcp/src/google_drive/oauth_pkce.rs index 47708b95..e65a2520 100644 --- a/crates/goose-mcp/src/google_drive/oauth_pkce.rs +++ b/crates/goose-mcp/src/google_drive/oauth_pkce.rs @@ -193,10 +193,10 @@ impl PkceOAuth2Client { }; // Store updated token data - self.credentials_manager - .write_credentials(&token_data) - .map(|_| debug!("Successfully stored token data")) - .unwrap_or_else(|e| error!("Failed to store token data: {}", e)); + match self.credentials_manager.write_credentials(&token_data) { + Ok(_) => debug!("Successfully stored token data"), + Err(e) => error!("Failed to store token data: {}", e), + } } else { debug!("No refresh token provided in OAuth flow response"); } @@ -248,10 +248,10 @@ impl PkceOAuth2Client { }; // Store updated token data - self.credentials_manager - .write_credentials(&token_data) - .map(|_| debug!("Successfully stored token data")) - .unwrap_or_else(|e| error!("Failed to store token data: {}", e)); + match self.credentials_manager.write_credentials(&token_data) { + Ok(_) => debug!("Successfully stored token data"), + Err(e) => error!("Failed to store token data: {}", e), + } Ok(access_token) } diff --git a/crates/goose-mcp/src/google_drive/storage.rs b/crates/goose-mcp/src/google_drive/storage.rs index 8e8f3c08..73156b62 100644 --- a/crates/goose-mcp/src/google_drive/storage.rs +++ b/crates/goose-mcp/src/google_drive/storage.rs @@ -1,16 +1,17 @@ use anyhow::Result; -use keyring::Entry; +use goose::keyring::{KeyringBackend, KeyringError}; use serde::{de::DeserializeOwned, Serialize}; use std::fs; use std::path::Path; +use std::sync::Arc; use thiserror::Error; use tracing::{debug, error, warn}; #[allow(dead_code)] #[derive(Error, Debug)] pub enum StorageError { - #[error("Failed to access keychain: {0}")] - KeyringError(#[from] keyring::Error), + #[error("Failed to access keyring: {0}")] + KeyringError(String), #[error("Failed to access file system: {0}")] FileSystemError(#[from] std::io::Error), #[error("No credentials found")] @@ -21,6 +22,15 @@ pub enum StorageError { SerializationError(#[from] serde_json::Error), } +impl From for StorageError { + fn from(err: KeyringError) -> Self { + match err { + KeyringError::NotFound { .. } => StorageError::NotFound, + _ => StorageError::KeyringError(err.to_string()), + } + } +} + /// CredentialsManager handles secure storage of OAuth credentials. /// It attempts to store credentials in the system keychain first, /// with fallback to file system storage if keychain access fails and fallback is enabled. @@ -29,6 +39,7 @@ pub struct CredentialsManager { fallback_to_disk: bool, keychain_service: String, keychain_username: String, + keyring: Arc, } impl CredentialsManager { @@ -37,12 +48,14 @@ impl CredentialsManager { fallback_to_disk: bool, keychain_service: String, keychain_username: String, + keyring: Arc, ) -> Self { Self { credentials_path, fallback_to_disk, keychain_service, keychain_username, + keyring, } } @@ -53,17 +66,19 @@ impl CredentialsManager { /// /// # Type Parameters /// - /// * `T` - The type to deserialize the credentials into. Must implement `serde::de::DeserializeOwned`. + /// * `T` - The type to deserialize to. Must implement `serde::DeserializeOwned`. /// /// # Returns /// - /// * `Ok(T)` - The deserialized credentials + /// * `Ok(T)` - The deserialized data /// * `Err(StorageError)` - If reading or deserialization fails /// /// # Examples /// /// ```no_run /// # use goose_mcp::google_drive::storage::CredentialsManager; + /// # use goose::keyring::SystemKeyringBackend; + /// # use std::sync::Arc; /// use serde::{Serialize, Deserialize}; /// /// #[derive(Serialize, Deserialize)] @@ -73,34 +88,45 @@ impl CredentialsManager { /// expiry: u64, /// } /// + /// # fn example() -> Result<(), Box> { + /// let keyring = Arc::new(SystemKeyringBackend); /// let manager = CredentialsManager::new( /// String::from("/path/to/credentials.json"), /// true, // fallback to disk if keychain fails /// String::from("test_service"), - /// String::from("test_user") + /// String::from("test_user"), + /// keyring /// ); /// match manager.read_credentials::() { - /// Ok(token) => println!("Token expires at: {}", token.expiry), + /// Ok(token) => println!("Access token: {}", token.access_token), /// Err(e) => eprintln!("Failed to read token: {}", e), /// } + /// # Ok(()) + /// # } /// ``` pub fn read_credentials(&self) -> Result where T: DeserializeOwned, { - let json_str = Entry::new(&self.keychain_service, &self.keychain_username) - .and_then(|entry| entry.get_password()) + let json_str = self + .keyring + .get_password(&self.keychain_service, &self.keychain_username) .inspect(|_| { debug!("Successfully read credentials from keychain"); }) .or_else(|e| { if self.fallback_to_disk { - debug!("Falling back to file system due to keyring error: {}", e); + warn!("Falling back to file system due to keyring error: {}", e); self.read_from_file() } else { - match e { - keyring::Error::NoEntry => Err(StorageError::NotFound), - _ => Err(StorageError::KeyringError(e)), + // Convert anyhow::Error back to our error type + if let Some(keyring_err) = e.downcast_ref::() { + match keyring_err { + KeyringError::NotFound { .. } => Err(StorageError::NotFound), + _ => Err(StorageError::KeyringError(e.to_string())), + } + } else { + Err(StorageError::KeyringError(e.to_string())) } } })?; @@ -149,6 +175,8 @@ impl CredentialsManager { /// /// ```no_run /// # use goose_mcp::google_drive::storage::CredentialsManager; + /// # use goose::keyring::SystemKeyringBackend; + /// # use std::sync::Arc; /// use serde::{Serialize, Deserialize}; /// /// #[derive(Serialize, Deserialize)] @@ -158,21 +186,26 @@ impl CredentialsManager { /// expiry: u64, /// } /// + /// # fn example() -> Result<(), Box> { /// let token = OAuthToken { /// access_token: String::from("access_token_value"), /// refresh_token: String::from("refresh_token_value"), /// expiry: 1672531200, // Unix timestamp /// }; /// + /// let keyring = Arc::new(SystemKeyringBackend); /// let manager = CredentialsManager::new( /// String::from("/path/to/credentials.json"), /// true, // fallback to disk if keychain fails /// String::from("test_service"), - /// String::from("test_user") + /// String::from("test_user"), + /// keyring /// ); /// if let Err(e) = manager.write_credentials(&token) { /// eprintln!("Failed to write token: {}", e); /// } + /// # Ok(()) + /// # } /// ``` pub fn write_credentials(&self, content: &T) -> Result<(), StorageError> where @@ -180,8 +213,8 @@ impl CredentialsManager { { let json_str = serde_json::to_string(content).map_err(StorageError::SerializationError)?; - Entry::new(&self.keychain_service, &self.keychain_username) - .and_then(|entry| entry.set_password(&json_str)) + self.keyring + .set_password(&self.keychain_service, &self.keychain_username, &json_str) .inspect(|_| { debug!("Successfully wrote credentials to keychain"); }) @@ -190,13 +223,14 @@ impl CredentialsManager { warn!("Falling back to file system due to keyring error: {}", e); self.write_to_file(&json_str) } else { - Err(StorageError::KeyringError(e)) + Err(StorageError::KeyringError(e.to_string())) } }) } fn write_to_file(&self, content: &str) -> Result<(), StorageError> { let path = Path::new(&self.credentials_path); + if let Some(parent) = path.parent() { if !parent.exists() { match fs::create_dir_all(parent) { @@ -229,6 +263,7 @@ impl Clone for CredentialsManager { fallback_to_disk: self.fallback_to_disk, keychain_service: self.keychain_service.clone(), keychain_username: self.keychain_username.clone(), + keyring: self.keyring.clone(), } } } @@ -236,6 +271,7 @@ impl Clone for CredentialsManager { #[cfg(test)] mod tests { use super::*; + use goose::keyring::MockKeyringBackend; use serde::{Deserialize, Serialize}; use tempfile::tempdir; @@ -263,25 +299,31 @@ mod tests { let cred_path = temp_dir.path().join("test_credentials.json"); let cred_path_str = cred_path.to_str().unwrap().to_string(); + // Create a mock keyring backend + let keyring = Arc::new(MockKeyringBackend::new()); + // Create a credentials manager with fallback enabled - // Using a unique service name to ensure keychain operation fails let manager = CredentialsManager::new( cred_path_str, true, // fallback to disk "test_service".to_string(), "test_user".to_string(), + keyring, ); // Test credentials to store let creds = TestCredentials::new(); - // Write should write to keychain + // Write should succeed with mock keyring let write_result = manager.write_credentials(&creds); - assert!(write_result.is_ok(), "Write should succeed with fallback"); + assert!( + write_result.is_ok(), + "Write should succeed with mock keyring" + ); - // Read should read from keychain + // Read should succeed with mock keyring let read_result = manager.read_credentials::(); - assert!(read_result.is_ok(), "Read should succeed with fallback"); + assert!(read_result.is_ok(), "Read should succeed with mock keyring"); // Verify the read credentials match what we wrote assert_eq!( @@ -298,21 +340,29 @@ mod tests { let cred_path = temp_dir.path().join("nonexistent_credentials.json"); let cred_path_str = cred_path.to_str().unwrap().to_string(); + // Create a mock keyring backend (empty by default) + let keyring = Arc::new(MockKeyringBackend::new()); + // Create a credentials manager with fallback disabled let manager = CredentialsManager::new( cred_path_str, false, // no fallback to disk "test_service_that_should_not_exist".to_string(), "test_user_no_fallback".to_string(), + keyring, ); - // Read should fail with NotFound or KeyringError depending on the system + // Read should fail with NotFound since mock keyring is empty and no fallback let read_result = manager.read_credentials::(); println!("{:?}", read_result); assert!( read_result.is_err(), "Read should fail when credentials don't exist" ); + assert!( + matches!(read_result.unwrap_err(), StorageError::NotFound), + "Should return NotFound error" + ); } #[test] @@ -327,11 +377,13 @@ mod tests { fn test_file_system_error_handling() { // Test handling of file system errors by using an invalid path let invalid_path = String::from("/nonexistent_directory/credentials.json"); + let keyring = Arc::new(MockKeyringBackend::new()); let manager = CredentialsManager::new( invalid_path, true, "test_service".to_string(), "test_user".to_string(), + keyring, ); // Create test credentials diff --git a/crates/goose/src/config/base.rs b/crates/goose/src/config/base.rs index eabdaf4d..143d2845 100644 --- a/crates/goose/src/config/base.rs +++ b/crates/goose/src/config/base.rs @@ -1,6 +1,8 @@ +use crate::keyring::{ + create_default_keyring, create_keyring_with_file_path, FileKeyringBackend, KeyringBackend, +}; use etcetera::{choose_app_strategy, AppStrategy, AppStrategyArgs}; use fs2::FileExt; -use keyring::Entry; use once_cell::sync::{Lazy, OnceCell}; use serde::Deserialize; use serde_json::Value; @@ -9,6 +11,7 @@ use std::env; use std::fs::OpenOptions; use std::io::Write; use std::path::{Path, PathBuf}; +use std::sync::Arc; use thiserror::Error; pub static APP_STRATEGY: Lazy = Lazy::new(|| AppStrategyArgs { @@ -19,6 +22,7 @@ pub static APP_STRATEGY: Lazy = Lazy::new(|| AppStrategyArgs { const KEYRING_SERVICE: &str = "goose"; const KEYRING_USERNAME: &str = "secrets"; +const SECRETS_FILE_NAME: &str = "secrets.yaml"; #[cfg(test)] const TEST_KEYRING_SERVICE: &str = "goose-test"; @@ -105,12 +109,8 @@ impl From for ConfigError { /// For Goose-specific configuration, consider prefixing with "goose_" to avoid conflicts. pub struct Config { config_path: PathBuf, - secrets: SecretStorage, -} - -enum SecretStorage { - Keyring { service: String }, - File { path: PathBuf }, + keyring: Arc, + keyring_service: String, } // Global instance @@ -118,6 +118,19 @@ static GLOBAL_CONFIG: OnceCell = OnceCell::new(); impl Default for Config { fn default() -> Self { + let config_dir = choose_app_strategy(APP_STRATEGY.clone()) + .expect("goose requires a home dir") + .config_dir(); + + // Use factory with custom file path to maintain same behavior + let keyring = create_keyring_with_file_path(config_dir.join(SECRETS_FILE_NAME)); + Self::with_keyring(keyring) + } +} + +impl Config { + /// Create a new configuration instance with a custom keyring backend + pub fn with_keyring(keyring: Arc) -> Self { // choose_app_strategy().config_dir() // - macOS/Linux: ~/.config/goose/ // - Windows: ~\AppData\Roaming\Block\goose\config\ @@ -129,22 +142,13 @@ impl Default for Config { let config_path = config_dir.join("config.yaml"); - let secrets = match env::var("GOOSE_DISABLE_KEYRING") { - Ok(_) => SecretStorage::File { - path: config_dir.join("secrets.yaml"), - }, - Err(_) => SecretStorage::Keyring { - service: KEYRING_SERVICE.to_string(), - }, - }; Config { config_path, - secrets, + keyring, + keyring_service: KEYRING_SERVICE.to_string(), } } -} -impl Config { /// Get the global configuration instance. /// /// This will initialize the configuration with the default path (~/.config/goose/config.yaml) @@ -160,9 +164,8 @@ impl Config { pub fn new>(config_path: P, service: &str) -> Result { Ok(Config { config_path: config_path.as_ref().to_path_buf(), - secrets: SecretStorage::Keyring { - service: service.to_string(), - }, + keyring: create_default_keyring(), + keyring_service: service.to_string(), }) } @@ -176,9 +179,8 @@ impl Config { ) -> Result { Ok(Config { config_path: config_path.as_ref().to_path_buf(), - secrets: SecretStorage::File { - path: secrets_path.as_ref().to_path_buf(), - }, + keyring: Arc::new(FileKeyringBackend::new(secrets_path.as_ref().to_path_buf())), + keyring_service: KEYRING_SERVICE.to_string(), }) } @@ -476,32 +478,23 @@ impl Config { Ok(()) } - // Load current secrets from the keyring pub fn load_secrets(&self) -> Result, ConfigError> { - match &self.secrets { - SecretStorage::Keyring { service } => { - let entry = Entry::new(service, KEYRING_USERNAME)?; - - match entry.get_password() { - Ok(content) => { - let values: HashMap = serde_json::from_str(&content)?; - Ok(values) - } - Err(keyring::Error::NoEntry) => Ok(HashMap::new()), - Err(e) => Err(ConfigError::KeyringError(e.to_string())), - } + match self + .keyring + .get_password(&self.keyring_service, KEYRING_USERNAME) + { + Ok(content) => { + let values: HashMap = serde_json::from_str(&content)?; + Ok(values) } - SecretStorage::File { path } => { - if path.exists() { - let file_content = std::fs::read_to_string(path)?; - let yaml_value: serde_yaml::Value = serde_yaml::from_str(&file_content)?; - let json_value: Value = serde_json::to_value(yaml_value)?; - match json_value { - Value::Object(map) => Ok(map.into_iter().collect()), - _ => Ok(HashMap::new()), + Err(e) => { + if let Some(keyring_err) = e.downcast_ref::() { + match keyring_err { + crate::keyring::KeyringError::NotFound { .. } => Ok(HashMap::new()), + _ => Err(ConfigError::KeyringError(e.to_string())), } } else { - Ok(HashMap::new()) + Err(ConfigError::KeyringError(e.to_string())) } } } @@ -653,18 +646,10 @@ impl Config { pub fn set_secret(&self, key: &str, value: Value) -> Result<(), ConfigError> { let mut values = self.load_secrets()?; values.insert(key.to_string(), value); - - match &self.secrets { - SecretStorage::Keyring { service } => { - let json_value = serde_json::to_string(&values)?; - let entry = Entry::new(service, KEYRING_USERNAME)?; - entry.set_password(&json_value)?; - } - SecretStorage::File { path } => { - let yaml_value = serde_yaml::to_string(&values)?; - std::fs::write(path, yaml_value)?; - } - }; + let json_value = serde_json::to_string(&values)?; + self.keyring + .set_password(&self.keyring_service, KEYRING_USERNAME, &json_value) + .map_err(|e| ConfigError::KeyringError(e.to_string()))?; Ok(()) } @@ -681,18 +666,10 @@ impl Config { pub fn delete_secret(&self, key: &str) -> Result<(), ConfigError> { let mut values = self.load_secrets()?; values.remove(key); - - match &self.secrets { - SecretStorage::Keyring { service } => { - let json_value = serde_json::to_string(&values)?; - let entry = Entry::new(service, KEYRING_USERNAME)?; - entry.set_password(&json_value)?; - } - SecretStorage::File { path } => { - let yaml_value = serde_yaml::to_string(&values)?; - std::fs::write(path, yaml_value)?; - } - }; + let json_value = serde_json::to_string(&values)?; + self.keyring + .set_password(&self.keyring_service, KEYRING_USERNAME, &json_value) + .map_err(|e| ConfigError::KeyringError(e.to_string()))?; Ok(()) } } @@ -754,18 +731,8 @@ pub fn load_init_config_from_workspace() -> Result, Confi #[cfg(test)] mod tests { use super::*; - use serial_test::serial; use tempfile::NamedTempFile; - fn cleanup_keyring() -> Result<(), ConfigError> { - let entry = Entry::new(TEST_KEYRING_SERVICE, KEYRING_USERNAME)?; - match entry.delete_credential() { - Ok(_) => Ok(()), - Err(keyring::Error::NoEntry) => Ok(()), - Err(e) => Err(ConfigError::KeyringError(e.to_string())), - } - } - #[test] fn test_basic_config() -> Result<(), ConfigError> { let temp_file = NamedTempFile::new().unwrap(); @@ -876,11 +843,12 @@ mod tests { } #[test] - #[serial] fn test_secret_management() -> Result<(), ConfigError> { - cleanup_keyring()?; - let temp_file = NamedTempFile::new().unwrap(); - let config = Config::new(temp_file.path(), TEST_KEYRING_SERVICE)?; + use crate::keyring::MockKeyringBackend; + + let _temp_file = NamedTempFile::new().unwrap(); + let mock_keyring = Arc::new(MockKeyringBackend::new()); + let config = Config::with_keyring(mock_keyring); // Test setting and getting a simple secret config.set_secret("api_key", Value::String("secret123".to_string()))?; @@ -898,16 +866,55 @@ mod tests { let result: Result = config.get_secret("api_key"); assert!(matches!(result, Err(ConfigError::NotFound(_)))); - cleanup_keyring()?; Ok(()) } #[test] - #[serial] + fn test_secret_management_with_mock() -> Result<(), ConfigError> { + use crate::keyring::MockKeyringBackend; + + // Save and remove GOOSE_DISABLE_KEYRING to ensure we use the mock keyring + let saved_disable = env::var("GOOSE_DISABLE_KEYRING").ok(); + env::remove_var("GOOSE_DISABLE_KEYRING"); + + let mock_keyring = Arc::new(MockKeyringBackend::new()); + let config = Config::with_keyring(mock_keyring.clone()); + + // Test setting and getting a simple secret + config.set_secret("api_key", Value::String("secret123".to_string()))?; + let value: String = config.get_secret("api_key")?; + assert_eq!(value, "secret123"); + + // Verify it's in the mock keyring + assert!(mock_keyring.contains("goose", "secrets")); + + // Test environment variable override + std::env::set_var("API_KEY", "env_secret"); + let value: String = config.get_secret("api_key")?; + assert_eq!(value, "env_secret"); + std::env::remove_var("API_KEY"); + + // Test deleting a secret + config.delete_secret("api_key")?; + let result: Result = config.get_secret("api_key"); + assert!(matches!(result, Err(ConfigError::NotFound(_)))); + + // Restore GOOSE_DISABLE_KEYRING + match saved_disable { + Some(val) => env::set_var("GOOSE_DISABLE_KEYRING", val), + None => env::remove_var("GOOSE_DISABLE_KEYRING"), + } + + Ok(()) + } + + #[test] fn test_multiple_secrets() -> Result<(), ConfigError> { - cleanup_keyring()?; - let temp_file = NamedTempFile::new().unwrap(); - let config = Config::new(temp_file.path(), TEST_KEYRING_SERVICE)?; + use crate::keyring::MockKeyringBackend; + + let _temp_file = NamedTempFile::new().unwrap(); + let mock_keyring = Arc::new(MockKeyringBackend::new()); + let config = Config::with_keyring(mock_keyring); // Set multiple secrets config.set_secret("key1", Value::String("secret1".to_string()))?; @@ -928,7 +935,131 @@ mod tests { assert!(matches!(result1, Err(ConfigError::NotFound(_)))); assert_eq!(value2, "secret2"); - cleanup_keyring()?; + Ok(()) + } + + #[test] + fn test_multiple_secrets_with_mock() -> Result<(), ConfigError> { + use crate::keyring::MockKeyringBackend; + + // Save and remove GOOSE_DISABLE_KEYRING to ensure we use the mock keyring + let saved_disable = env::var("GOOSE_DISABLE_KEYRING").ok(); + env::remove_var("GOOSE_DISABLE_KEYRING"); + + let mock_keyring = Arc::new(MockKeyringBackend::new()); + let config = Config::with_keyring(mock_keyring.clone()); + + // Set multiple secrets + config.set_secret("key1", Value::String("secret1".to_string()))?; + config.set_secret("key2", Value::String("secret2".to_string()))?; + + // Verify both exist + let value1: String = config.get_secret("key1")?; + let value2: String = config.get_secret("key2")?; + assert_eq!(value1, "secret1"); + assert_eq!(value2, "secret2"); + + // Delete one secret + config.delete_secret("key1")?; + + // Verify key1 is gone but key2 remains + let result1: Result = config.get_secret("key1"); + let value2: String = config.get_secret("key2")?; + assert!(matches!(result1, Err(ConfigError::NotFound(_)))); + assert_eq!(value2, "secret2"); + + // Restore GOOSE_DISABLE_KEYRING + match saved_disable { + Some(val) => env::set_var("GOOSE_DISABLE_KEYRING", val), + None => env::remove_var("GOOSE_DISABLE_KEYRING"), + } + + Ok(()) + } + + #[test] + fn test_keyring_error_propagation() -> Result<(), ConfigError> { + use crate::keyring::{KeyringBackend, KeyringError}; + + // Create a failing keyring that returns backend errors + struct FailingKeyring; + + impl KeyringBackend for FailingKeyring { + fn get_password(&self, _: &str, _: &str) -> anyhow::Result { + Err(KeyringError::Backend("Keyring service unavailable".to_string()).into()) + } + fn set_password(&self, _: &str, _: &str, _: &str) -> anyhow::Result<()> { + Err(KeyringError::Backend("Keyring service unavailable".to_string()).into()) + } + fn delete_password(&self, _: &str, _: &str) -> anyhow::Result<()> { + Err(KeyringError::Backend("Keyring service unavailable".to_string()).into()) + } + } + + // Save and remove GOOSE_DISABLE_KEYRING to ensure we use the failing keyring + let saved_disable = env::var("GOOSE_DISABLE_KEYRING").ok(); + env::remove_var("GOOSE_DISABLE_KEYRING"); + + let failing_keyring = Arc::new(FailingKeyring); + let config = Config::with_keyring(failing_keyring); + + // This should return an error, not an empty HashMap + let result = config.load_secrets(); + assert!(result.is_err()); + assert!(result + .unwrap_err() + .to_string() + .contains("Keyring service unavailable")); + + // Restore GOOSE_DISABLE_KEYRING + match saved_disable { + Some(val) => env::set_var("GOOSE_DISABLE_KEYRING", val), + None => env::remove_var("GOOSE_DISABLE_KEYRING"), + } + + Ok(()) + } + + #[test] + fn test_keyring_not_found_returns_empty() -> Result<(), ConfigError> { + use crate::keyring::{KeyringBackend, KeyringError}; + + // Create a keyring that always returns NotFound + struct NotFoundKeyring; + + impl KeyringBackend for NotFoundKeyring { + fn get_password(&self, service: &str, username: &str) -> anyhow::Result { + Err(KeyringError::NotFound { + service: service.to_string(), + username: username.to_string(), + } + .into()) + } + fn set_password(&self, _: &str, _: &str, _: &str) -> anyhow::Result<()> { + Ok(()) + } + fn delete_password(&self, _: &str, _: &str) -> anyhow::Result<()> { + Ok(()) + } + } + + // Save and remove GOOSE_DISABLE_KEYRING to ensure we use the not-found keyring + let saved_disable = env::var("GOOSE_DISABLE_KEYRING").ok(); + env::remove_var("GOOSE_DISABLE_KEYRING"); + + let not_found_keyring = Arc::new(NotFoundKeyring); + let config = Config::with_keyring(not_found_keyring); + + // This should return an empty HashMap, not an error + let result = config.load_secrets()?; + assert_eq!(result.len(), 0); + + // Restore GOOSE_DISABLE_KEYRING + match saved_disable { + Some(val) => env::set_var("GOOSE_DISABLE_KEYRING", val), + None => env::remove_var("GOOSE_DISABLE_KEYRING"), + } + Ok(()) } diff --git a/crates/goose/src/keyring/factory.rs b/crates/goose/src/keyring/factory.rs new file mode 100644 index 00000000..545d566c --- /dev/null +++ b/crates/goose/src/keyring/factory.rs @@ -0,0 +1,345 @@ +#[cfg(test)] +use super::KeyringError; +use super::{FileKeyringBackend, KeyringBackend, MockKeyringBackend, SystemKeyringBackend}; +use etcetera::AppStrategy; +use std::path::PathBuf; +use std::sync::Arc; +use std::{env, path::Path}; + +/// Factory for creating keyring backends with environment-based defaults. +/// +/// This factory provides a consistent way to create keyring backends across +/// the entire codebase, handling environment variable detection and providing +/// sensible defaults based on the execution context. +/// +/// # Environment Variable Priority +/// +/// The factory checks environment variables in this order: +/// 1. `GOOSE_USE_MOCK_KEYRING` (highest priority) - Forces mock backend for testing +/// 2. `GOOSE_DISABLE_KEYRING` - Forces file-based backend for production without OS keyring +/// 3. Default behavior - Uses system keyring for production +pub struct KeyringFactory; + +impl KeyringFactory { + /// Create a keyring backend using environment-based defaults. + /// + /// This is the most common use case - let the factory decide which backend + /// to use based on environment variables and execution context. + /// + /// # Examples + /// + /// ```rust,no_run + /// use goose::keyring::KeyringFactory; + /// + /// let keyring = KeyringFactory::create_default(); + /// keyring.set_password("service", "user", "password").unwrap(); + /// ``` + pub fn create_default() -> Arc { + Self::create_with_config(DefaultKeyringConfig::new()) + } + + /// Create a keyring backend with custom configuration. + /// + /// This allows overriding default behavior such as specifying a custom + /// file path for the file-based backend. + /// + /// # Examples + /// + /// ```rust,no_run + /// use goose::keyring::{KeyringFactory, DefaultKeyringConfig}; + /// use std::path::PathBuf; + /// + /// let config = DefaultKeyringConfig::new() + /// .with_file_path(PathBuf::from("/custom/path/secrets.yaml")); + /// let keyring = KeyringFactory::create_with_config(config); + /// ``` + pub fn create_with_config(config: DefaultKeyringConfig) -> Arc { + // Priority 1: Mock keyring for testing + if Self::is_env_var_truthy("GOOSE_USE_MOCK_KEYRING") { + return Arc::new(MockKeyringBackend::new()); + } + + // Priority 2: File-based keyring when system keyring disabled + if Self::is_env_var_truthy("GOOSE_DISABLE_KEYRING") { + let file_path = config + .file_path + .unwrap_or_else(|| Self::default_config_dir().join("secrets.yaml")); + return Arc::new(FileKeyringBackend::new(file_path)); + } + + // Priority 3: System keyring (default) + Arc::new(SystemKeyringBackend) + } + + /// Check if an environment variable is set to a truthy value. + /// + /// This matches the same logic used throughout the goose codebase for + /// consistency in environment variable interpretation. + /// + /// Truthy values: "1", "true", "yes", "on" (case insensitive) + /// Falsy values: "0", "false", "no", "off", "" or unset + fn is_env_var_truthy(var_name: &str) -> bool { + match env::var(var_name) { + Ok(value) => { + let normalized = value.trim().to_lowercase(); + matches!(normalized.as_str(), "1" | "true" | "yes" | "on") + } + Err(_) => false, + } + } + + /// Get the default configuration directory using the same logic as Config. + /// + /// This ensures consistency with the rest of the goose configuration system. + fn default_config_dir() -> PathBuf { + use etcetera::{choose_app_strategy, AppStrategyArgs}; + + let strategy = AppStrategyArgs { + top_level_domain: "Block".to_string(), + author: "Block".to_string(), + app_name: "goose".to_string(), + }; + + choose_app_strategy(strategy) + .expect("goose requires a home dir") + .config_dir() + } +} + +/// Configuration options for keyring factory creation. +/// +/// This struct allows customizing the behavior of the keyring factory +/// without breaking the simple default use case. +#[derive(Default)] +pub struct DefaultKeyringConfig { + /// Optional custom file path for FileKeyringBackend. + /// + /// If not provided, defaults to `{config_dir}/secrets.yaml` + pub file_path: Option, +} + +impl DefaultKeyringConfig { + /// Create a new configuration with default values. + pub fn new() -> Self { + Default::default() + } + + /// Set a custom file path for the file-based keyring backend. + /// + /// This path will be used when `GOOSE_DISABLE_KEYRING` is set but + /// `GOOSE_USE_MOCK_KEYRING` is not set. + pub fn with_file_path>(mut self, path: P) -> Self { + self.file_path = Some(path.as_ref().to_path_buf()); + self + } +} + +#[cfg(test)] +mod tests { + use super::*; + use serial_test::serial; + use std::env; + use tempfile::tempdir; + + /// Test utility for managing environment variables in tests. + /// + /// This ensures that environment variables are properly restored + /// after each test, preventing test interference. + struct EnvGuard { + key: String, + original_value: Option, + } + + impl EnvGuard { + fn set(key: &str, value: &str) -> Self { + let original_value = env::var(key).ok(); + env::set_var(key, value); + EnvGuard { + key: key.to_string(), + original_value, + } + } + + fn remove(key: &str) -> Self { + let original_value = env::var(key).ok(); + env::remove_var(key); + EnvGuard { + key: key.to_string(), + original_value, + } + } + } + + impl Drop for EnvGuard { + fn drop(&mut self) { + match &self.original_value { + Some(value) => env::set_var(&self.key, value), + None => env::remove_var(&self.key), + } + } + } + + #[test] + #[serial] + fn test_mock_keyring_priority() { + let _guard1 = EnvGuard::set("GOOSE_USE_MOCK_KEYRING", "true"); + let _guard2 = EnvGuard::set("GOOSE_DISABLE_KEYRING", "true"); + + let keyring = KeyringFactory::create_default(); + + // Should use MockKeyringBackend despite GOOSE_DISABLE_KEYRING being set + // We can verify this by checking that a non-existent key returns NotFound + match keyring.get_password("test_service", "test_user") { + Err(e) => { + if let Some(keyring_err) = e.downcast_ref::() { + assert!(matches!(keyring_err, KeyringError::NotFound { .. })); + } + } + Ok(_) => panic!("Mock keyring should return NotFound for non-existent keys"), + } + } + + #[test] + #[serial] + fn test_file_keyring_when_disabled() { + let _guard1 = EnvGuard::remove("GOOSE_USE_MOCK_KEYRING"); + let _guard2 = EnvGuard::set("GOOSE_DISABLE_KEYRING", "true"); + + let temp_dir = tempdir().unwrap(); + let keyring = KeyringFactory::create_with_config( + DefaultKeyringConfig::new().with_file_path(temp_dir.path().join("test_secrets.yaml")), + ); + + // Use unique service/user names to avoid conflicts + let service = format!("service_{}", std::process::id()); + let user = format!("user_{}", std::process::id()); + + // Verify it creates a FileKeyringBackend by testing file operations + keyring.set_password(&service, &user, "password").unwrap(); + assert_eq!(keyring.get_password(&service, &user).unwrap(), "password"); + + // Verify the file was actually created + assert!(temp_dir.path().join("test_secrets.yaml").exists()); + } + + #[test] + #[serial] + fn test_system_keyring_default() { + // For this test, we'll use mock keyring to avoid OS keyring popups + // but verify the logic by ensuring that when neither override is set, + // the factory doesn't choose the file backend + let _guard1 = EnvGuard::set("GOOSE_USE_MOCK_KEYRING", "true"); + let _guard2 = EnvGuard::remove("GOOSE_DISABLE_KEYRING"); + + let keyring = KeyringFactory::create_default(); + + // Verify it creates a MockKeyringBackend (since we forced it for testing) + // The main thing we're testing is that the logic flow is correct + match keyring.get_password("non_existent_service", "non_existent_user") { + Err(e) => { + if let Some(keyring_err) = e.downcast_ref::() { + assert!(matches!(keyring_err, KeyringError::NotFound { .. })); + } + } + Ok(_) => panic!("Mock keyring should return NotFound for non-existent keys"), + } + } + + #[test] + #[serial] + fn test_default_config_with_custom_path() { + let _guard1 = EnvGuard::remove("GOOSE_USE_MOCK_KEYRING"); + let _guard2 = EnvGuard::set("GOOSE_DISABLE_KEYRING", "true"); + + let temp_dir = tempdir().unwrap(); + let custom_path = temp_dir.path().join("custom_secrets.yaml"); + + let config = DefaultKeyringConfig::new().with_file_path(&custom_path); + let keyring = KeyringFactory::create_with_config(config); + + // Use unique service/user names to avoid conflicts + let service = format!("test_service_{}", std::process::id()); + let user = format!("test_user_{}", std::process::id()); + + // Test that the custom path is used + keyring + .set_password(&service, &user, "test_password") + .unwrap(); + + // Verify the custom file was created + assert!(custom_path.exists()); + + // Verify we can retrieve the password + assert_eq!( + keyring.get_password(&service, &user).unwrap(), + "test_password" + ); + } + + #[test] + #[serial] + fn test_env_var_truthy_values() { + // Test truthy values + for value in [ + "1", "true", "TRUE", "yes", "YES", "on", "ON", " true ", "True", + ] { + let _guard = EnvGuard::set("TEST_TRUTHY_VAR", value); + assert!( + KeyringFactory::is_env_var_truthy("TEST_TRUTHY_VAR"), + "Value '{}' should be truthy", + value + ); + } + + // Test falsy values + for value in [ + "0", "false", "FALSE", "no", "NO", "off", "OFF", "", " ", "random", + ] { + let _guard = EnvGuard::set("TEST_TRUTHY_VAR", value); + assert!( + !KeyringFactory::is_env_var_truthy("TEST_TRUTHY_VAR"), + "Value '{}' should be falsy", + value + ); + } + + // Test unset variable + let _guard = EnvGuard::remove("TEST_TRUTHY_VAR"); + assert!( + !KeyringFactory::is_env_var_truthy("TEST_TRUTHY_VAR"), + "Unset variable should be falsy" + ); + } + + #[test] + #[serial] + fn test_factory_consistency_with_existing_is_env_var_truthy() { + // This test ensures our factory's is_env_var_truthy matches the existing implementation + // We'll test a few key values to ensure consistency + + let test_cases = [ + ("1", true), + ("true", true), + ("TRUE", true), + ("yes", true), + ("on", true), + ("0", false), + ("false", false), + ("no", false), + ("off", false), + ("", false), + ("random", false), + ]; + + for (value, expected) in test_cases { + let _guard = EnvGuard::set("TEST_CONSISTENCY_VAR", value); + assert_eq!( + KeyringFactory::is_env_var_truthy("TEST_CONSISTENCY_VAR"), + expected, + "Value '{}' should be {}", + value, + if expected { "truthy" } else { "falsy" } + ); + } + } +} diff --git a/crates/goose/src/keyring/file.rs b/crates/goose/src/keyring/file.rs new file mode 100644 index 00000000..e6a80069 --- /dev/null +++ b/crates/goose/src/keyring/file.rs @@ -0,0 +1,306 @@ +use super::{KeyringBackend, KeyringError}; +use anyhow::Result; +use serde_json::Value; +use std::collections::HashMap; +use std::path::PathBuf; + +pub struct FileKeyringBackend { + secrets_path: PathBuf, +} + +impl FileKeyringBackend { + pub fn new(secrets_path: PathBuf) -> Self { + Self { secrets_path } + } + + fn load_all_secrets(&self) -> Result> { + if self.secrets_path.exists() { + let file_content = std::fs::read_to_string(&self.secrets_path)?; + let yaml_value: serde_yaml::Value = serde_yaml::from_str(&file_content)?; + let json_value: Value = serde_json::to_value(yaml_value)?; + match json_value { + Value::Object(map) => { + let mut result = HashMap::new(); + + // Check if this is the new format (has "goose:secrets" key) + let service_key = Self::make_key("goose", "secrets"); + if let Some(service_value) = map.get(&service_key) { + // New format: decode JSON from the service key + if let Some(json_str) = service_value.as_str() { + if let Ok(secrets_map) = + serde_json::from_str::>(json_str) + { + for (key, value) in secrets_map { + if let Some(string_value) = value.as_str() { + result.insert(key, string_value.to_string()); + } else { + result.insert(key, serde_json::to_string(&value)?); + } + } + return Ok(result); + } + } + } + + // Legacy format: direct key-value mapping (read-only) + for (key, value) in &map { + if let Some(string_value) = value.as_str() { + result.insert(key.clone(), string_value.to_string()); + } else { + result.insert(key.clone(), serde_json::to_string(&value)?); + } + } + + Ok(result) + } + _ => Ok(HashMap::new()), + } + } else { + Ok(HashMap::new()) + } + } + + fn save_all_secrets(&self, secrets: &HashMap) -> Result<()> { + // Convert strings back to appropriate JSON values + let mut json_map = serde_json::Map::new(); + for (key, value) in secrets { + // Try to parse as JSON first, fall back to string + let json_value = + serde_json::from_str(value).unwrap_or_else(|_| Value::String(value.clone())); + json_map.insert(key.clone(), json_value); + } + + let yaml_value = serde_yaml::to_string(&json_map)?; + + // Ensure parent directory exists + if let Some(parent) = self.secrets_path.parent() { + std::fs::create_dir_all(parent)?; + } + + std::fs::write(&self.secrets_path, yaml_value)?; + Ok(()) + } + + fn make_key(service: &str, username: &str) -> String { + format!("{}:{}", service, username) + } +} + +impl KeyringBackend for FileKeyringBackend { + fn get_password(&self, service: &str, username: &str) -> Result { + let key = Self::make_key(service, username); + let secrets = self.load_all_secrets()?; + + secrets.get(&key).cloned().ok_or_else(|| { + KeyringError::NotFound { + service: service.to_string(), + username: username.to_string(), + } + .into() + }) + } + + fn set_password(&self, service: &str, username: &str, password: &str) -> Result<()> { + let key = Self::make_key(service, username); + let mut secrets = self.load_all_secrets()?; + secrets.insert(key, password.to_string()); + self.save_all_secrets(&secrets)?; + Ok(()) + } + + fn delete_password(&self, service: &str, username: &str) -> Result<()> { + let key = Self::make_key(service, username); + let mut secrets = self.load_all_secrets()?; + secrets.remove(&key); + self.save_all_secrets(&secrets)?; + Ok(()) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use tempfile::NamedTempFile; + + #[test] + fn test_basic_operations() -> Result<()> { + let temp_file = NamedTempFile::new().unwrap(); + let backend = FileKeyringBackend::new(temp_file.path().to_path_buf()); + + // Test setting a password + backend.set_password("test_service", "test_user", "test_password")?; + + // Test getting the password + let password = backend.get_password("test_service", "test_user")?; + assert_eq!(password, "test_password"); + + // Test deleting the password + backend.delete_password("test_service", "test_user")?; + + // Test that getting deleted password returns NotFound error + let result = backend.get_password("test_service", "test_user"); + assert!(result.is_err()); + if let Err(e) = result { + if let Some(keyring_err) = e.downcast_ref::() { + assert!(matches!(keyring_err, KeyringError::NotFound { .. })); + } else { + panic!("Expected KeyringError::NotFound, got: {:?}", e); + } + } + + Ok(()) + } + + #[test] + fn test_multiple_services() -> Result<()> { + let temp_file = NamedTempFile::new().unwrap(); + let backend = FileKeyringBackend::new(temp_file.path().to_path_buf()); + + // Set passwords for different services + backend.set_password("service1", "user1", "password1")?; + backend.set_password("service2", "user2", "password2")?; + backend.set_password("service1", "user2", "password3")?; + + // Verify all passwords can be retrieved correctly + assert_eq!(backend.get_password("service1", "user1")?, "password1"); + assert_eq!(backend.get_password("service2", "user2")?, "password2"); + assert_eq!(backend.get_password("service1", "user2")?, "password3"); + + // Delete one password and verify others remain + backend.delete_password("service1", "user1")?; + assert!(backend.get_password("service1", "user1").is_err()); + assert_eq!(backend.get_password("service2", "user2")?, "password2"); + assert_eq!(backend.get_password("service1", "user2")?, "password3"); + + Ok(()) + } + + #[test] + fn test_password_update() -> Result<()> { + let temp_file = NamedTempFile::new().unwrap(); + let backend = FileKeyringBackend::new(temp_file.path().to_path_buf()); + + // Set initial password + backend.set_password("service", "user", "old_password")?; + assert_eq!(backend.get_password("service", "user")?, "old_password"); + + // Update password + backend.set_password("service", "user", "new_password")?; + assert_eq!(backend.get_password("service", "user")?, "new_password"); + + Ok(()) + } + + #[test] + fn test_nonexistent_file() -> Result<()> { + let temp_file = NamedTempFile::new().unwrap(); + let file_path = temp_file.path().to_path_buf(); + drop(temp_file); // Delete the file + + let backend = FileKeyringBackend::new(file_path); + + // Getting from non-existent file should return NotFound + let result = backend.get_password("service", "user"); + assert!(result.is_err()); + if let Err(e) = result { + if let Some(keyring_err) = e.downcast_ref::() { + assert!(matches!(keyring_err, KeyringError::NotFound { .. })); + } + } + + // Setting should create the file + backend.set_password("service", "user", "password")?; + assert_eq!(backend.get_password("service", "user")?, "password"); + + Ok(()) + } + + #[test] + fn test_empty_password() -> Result<()> { + let temp_file = NamedTempFile::new().unwrap(); + let backend = FileKeyringBackend::new(temp_file.path().to_path_buf()); + + // Test setting and getting empty password + backend.set_password("service", "user", "")?; + assert_eq!(backend.get_password("service", "user")?, ""); + + Ok(()) + } + + #[test] + fn test_special_characters_in_credentials() -> Result<()> { + let temp_file = NamedTempFile::new().unwrap(); + let backend = FileKeyringBackend::new(temp_file.path().to_path_buf()); + + // Test with special characters in service, user, and password + let service = "service-with-dashes_and_underscores.and.dots"; + let user = "user@domain.com"; + let password = "password with spaces & special chars: !@#$%^&*()"; + + backend.set_password(service, user, password)?; + assert_eq!(backend.get_password(service, user)?, password); + + Ok(()) + } + + #[test] + fn test_json_content_in_password() -> Result<()> { + let temp_file = NamedTempFile::new().unwrap(); + let backend = FileKeyringBackend::new(temp_file.path().to_path_buf()); + + // Test storing JSON content as password + let json_password = + r#"{"access_token":"abc123","refresh_token":"def456","expires_in":3600}"#; + backend.set_password("oauth_service", "user", json_password)?; + + let retrieved = backend.get_password("oauth_service", "user")?; + + // Parse both as JSON to compare content regardless of key order + let original: serde_json::Value = serde_json::from_str(json_password).unwrap(); + let retrieved_parsed: serde_json::Value = serde_json::from_str(&retrieved).unwrap(); + assert_eq!(original, retrieved_parsed); + + Ok(()) + } + + #[test] + fn test_delete_nonexistent_password() -> Result<()> { + let temp_file = NamedTempFile::new().unwrap(); + let backend = FileKeyringBackend::new(temp_file.path().to_path_buf()); + + // Deleting non-existent password should not error (idempotent) + backend.delete_password("nonexistent_service", "nonexistent_user")?; + + Ok(()) + } + + #[test] + fn test_legacy_format_compatibility() -> Result<()> { + let temp_file = NamedTempFile::new().unwrap(); + + // Write a legacy format secrets.yaml file + let legacy_content = r#"openai_api_key: sk-abc123 +anthropic_api_key: ant-def456 +complex_config: + nested: value + number: 42 +"#; + std::fs::write(temp_file.path(), legacy_content)?; + + // Load with FileKeyringBackend - should read legacy format + let backend = FileKeyringBackend::new(temp_file.path().to_path_buf()); + + // Load secrets should work with legacy format (read-only) + let secrets = backend.load_all_secrets()?; + assert_eq!(secrets.get("openai_api_key").unwrap(), "sk-abc123"); + assert_eq!(secrets.get("anthropic_api_key").unwrap(), "ant-def456"); + assert!(secrets.get("complex_config").unwrap().contains("nested")); + + // Verify the original file format is preserved (no auto-migration) + let file_content = std::fs::read_to_string(temp_file.path())?; + assert!(file_content.contains("openai_api_key: sk-abc123")); + assert!(!file_content.contains("goose:secrets")); + + Ok(()) + } +} diff --git a/crates/goose/src/keyring/mock.rs b/crates/goose/src/keyring/mock.rs new file mode 100644 index 00000000..3792ea57 --- /dev/null +++ b/crates/goose/src/keyring/mock.rs @@ -0,0 +1,71 @@ +use super::{KeyringBackend, KeyringError}; +use anyhow::Result; +use std::collections::HashMap; +use std::sync::{Arc, RwLock}; + +#[derive(Clone, Default)] +pub struct MockKeyringBackend { + storage: Arc>>, +} + +impl MockKeyringBackend { + pub fn new() -> Self { + Self::default() + } + + pub fn clear(&self) { + self.storage + .write() + .expect("Mock keyring lock poisoned") + .clear(); + } + + pub fn contains(&self, service: &str, username: &str) -> bool { + let key = format!("{}:{}", service, username); + self.storage + .read() + .expect("Mock keyring lock poisoned") + .contains_key(&key) + } + + fn make_key(service: &str, username: &str) -> String { + format!("{}:{}", service, username) + } +} + +impl KeyringBackend for MockKeyringBackend { + fn get_password(&self, service: &str, username: &str) -> Result { + let key = Self::make_key(service, username); + let storage = self + .storage + .read() + .map_err(|e| KeyringError::Backend(format!("Mock keyring lock poisoned: {}", e)))?; + + storage + .get(&key) + .cloned() + .ok_or_else(|| KeyringError::NotFound { + service: service.to_string(), + username: username.to_string(), + }) + .map_err(anyhow::Error::from) + } + + fn set_password(&self, service: &str, username: &str, password: &str) -> Result<()> { + let key = Self::make_key(service, username); + self.storage + .write() + .map_err(|e| KeyringError::Backend(format!("Mock keyring lock poisoned: {}", e)))? + .insert(key, password.to_string()); + Ok(()) + } + + fn delete_password(&self, service: &str, username: &str) -> Result<()> { + let key = Self::make_key(service, username); + self.storage + .write() + .map_err(|e| KeyringError::Backend(format!("Mock keyring lock poisoned: {}", e)))? + .remove(&key); + Ok(()) + } +} diff --git a/crates/goose/src/keyring/mod.rs b/crates/goose/src/keyring/mod.rs new file mode 100644 index 00000000..5a611769 --- /dev/null +++ b/crates/goose/src/keyring/mod.rs @@ -0,0 +1,66 @@ +use anyhow::Result; + +pub trait KeyringBackend: Send + Sync { + fn get_password(&self, service: &str, username: &str) -> Result; + fn set_password(&self, service: &str, username: &str, password: &str) -> Result<()>; + fn delete_password(&self, service: &str, username: &str) -> Result<()>; +} + +#[derive(Debug, thiserror::Error)] +pub enum KeyringError { + #[error("No entry found for service '{service}' user '{username}'")] + NotFound { service: String, username: String }, + #[error("Access denied to keyring")] + AccessDenied, + #[error("Keyring backend error: {0}")] + Backend(String), +} + +pub mod factory; +pub mod file; +pub mod mock; +pub mod system; + +pub use factory::{DefaultKeyringConfig, KeyringFactory}; +pub use file::FileKeyringBackend; +pub use mock::MockKeyringBackend; +pub use system::SystemKeyringBackend; + +/// Convenience function for creating a keyring backend with environment-based defaults. +/// +/// This is the most common use case - it automatically selects the appropriate +/// keyring backend based on environment variables: +/// - `GOOSE_USE_MOCK_KEYRING=true` → MockKeyringBackend (for testing) +/// - `GOOSE_DISABLE_KEYRING=true` → FileKeyringBackend (for systems without keyring) +/// - Default → SystemKeyringBackend (for normal operation) +/// +/// # Examples +/// +/// ```rust,no_run +/// use goose::keyring::create_default_keyring; +/// +/// let keyring = create_default_keyring(); +/// keyring.set_password("service", "user", "password").unwrap(); +/// ``` +pub fn create_default_keyring() -> std::sync::Arc { + KeyringFactory::create_default() +} + +/// Convenience function for creating a keyring backend with a custom file path. +/// +/// This is useful when you need to store secrets in a specific location +/// while still respecting the environment variable hierarchy. +/// +/// # Examples +/// +/// ```rust,no_run +/// use goose::keyring::create_keyring_with_file_path; +/// use std::path::PathBuf; +/// +/// let keyring = create_keyring_with_file_path(PathBuf::from("/custom/secrets.yaml")); +/// ``` +pub fn create_keyring_with_file_path( + file_path: std::path::PathBuf, +) -> std::sync::Arc { + KeyringFactory::create_with_config(DefaultKeyringConfig::new().with_file_path(file_path)) +} diff --git a/crates/goose/src/keyring/system.rs b/crates/goose/src/keyring/system.rs new file mode 100644 index 00000000..4274bc58 --- /dev/null +++ b/crates/goose/src/keyring/system.rs @@ -0,0 +1,44 @@ +use super::{KeyringBackend, KeyringError}; +use anyhow::Result; +use keyring::Entry; + +pub struct SystemKeyringBackend; + +impl KeyringBackend for SystemKeyringBackend { + fn get_password(&self, service: &str, username: &str) -> Result { + let entry = + Entry::new(service, username).map_err(|e| KeyringError::Backend(e.to_string()))?; + + entry + .get_password() + .map_err(|e| match e { + keyring::Error::NoEntry => KeyringError::NotFound { + service: service.to_string(), + username: username.to_string(), + }, + _ => KeyringError::Backend(e.to_string()), + }) + .map_err(anyhow::Error::from) + } + + fn set_password(&self, service: &str, username: &str, password: &str) -> Result<()> { + let entry = + Entry::new(service, username).map_err(|e| KeyringError::Backend(e.to_string()))?; + + entry + .set_password(password) + .map_err(|e| KeyringError::Backend(e.to_string())) + .map_err(anyhow::Error::from) + } + + fn delete_password(&self, service: &str, username: &str) -> Result<()> { + let entry = + Entry::new(service, username).map_err(|e| KeyringError::Backend(e.to_string()))?; + + match entry.delete_credential() { + Ok(()) => Ok(()), + Err(keyring::Error::NoEntry) => Ok(()), // Already deleted is fine + Err(e) => Err(anyhow::Error::from(KeyringError::Backend(e.to_string()))), + } + } +} diff --git a/crates/goose/src/lib.rs b/crates/goose/src/lib.rs index 83c4934d..597e5f21 100644 --- a/crates/goose/src/lib.rs +++ b/crates/goose/src/lib.rs @@ -1,6 +1,7 @@ pub mod agents; pub mod config; pub mod context_mgmt; +pub mod keyring; pub mod message; pub mod model; pub mod permission; diff --git a/crates/goose/src/providers/factory.rs b/crates/goose/src/providers/factory.rs index 6c6f0f9b..e0a48ad4 100644 --- a/crates/goose/src/providers/factory.rs +++ b/crates/goose/src/providers/factory.rs @@ -180,6 +180,39 @@ mod tests { use mcp_core::{content::TextContent, Role}; use std::env; + /// Helper to save and restore environment variables for testing + struct EnvVarGuard { + saved_vars: Vec<(&'static str, Option)>, + } + + impl EnvVarGuard { + fn new(var_names: &[&'static str]) -> Self { + let saved_vars = var_names + .iter() + .map(|&name| (name, env::var(name).ok())) + .collect(); + Self { saved_vars } + } + + fn clear_all(&self) { + for (key, _) in &self.saved_vars { + env::remove_var(key); + } + } + } + + impl Drop for EnvVarGuard { + fn drop(&mut self) { + // Restore all env vars + for (key, value) in &self.saved_vars { + match value { + Some(val) => env::set_var(key, val), + None => env::remove_var(key), + } + } + } + } + #[allow(dead_code)] #[derive(Clone)] struct MockTestProvider { @@ -230,10 +263,17 @@ mod tests { #[test] fn test_create_lead_worker_provider() { - // Save current env vars - let saved_lead = env::var("GOOSE_LEAD_MODEL").ok(); - let saved_provider = env::var("GOOSE_LEAD_PROVIDER").ok(); - let saved_turns = env::var("GOOSE_LEAD_TURNS").ok(); + let _guard = EnvVarGuard::new(&[ + "GOOSE_LEAD_MODEL", + "GOOSE_LEAD_PROVIDER", + "GOOSE_LEAD_TURNS", + "OPENAI_API_KEY", + "ANTHROPIC_API_KEY", + ]); + + // Set fake API keys to avoid keychain access + env::set_var("OPENAI_API_KEY", "fake-test-key"); + env::set_var("ANTHROPIC_API_KEY", "fake-test-key"); // Test with basic lead model configuration env::set_var("GOOSE_LEAD_MODEL", "gpt-4o"); @@ -241,16 +281,21 @@ mod tests { // This will try to create a lead/worker provider let result = create("openai", ModelConfig::new("gpt-4o-mini".to_string())); - // The creation might succeed or fail depending on API keys, but we can verify the logic path + // Since we provided fake API keys, the creation should proceed to the provider instantiation + // It may still fail at the provider level, but we should not get keychain-related errors match result { Ok(_) => { - // If it succeeds, it means we created a lead/worker provider successfully - // This would happen if API keys are available in the test environment + // If it succeeds with fake keys, the logic worked } Err(error) => { - // If it fails, it should be due to missing API keys, confirming we tried to create providers - let error_msg = error.to_string(); - assert!(error_msg.contains("OPENAI_API_KEY") || error_msg.contains("secret")); + // Should not fail due to missing secrets, but may fail due to invalid fake keys + let error_msg = error.to_string().to_lowercase(); + // Ensure it's not a keychain/secret-related error + assert!( + !error_msg.contains("not found") || // Configuration not found + error_msg.contains("invalid") || // Invalid API key format + error_msg.contains("unauthorized") // API validation failed + ); } } @@ -259,44 +304,27 @@ mod tests { env::set_var("GOOSE_LEAD_TURNS", "5"); let _result = create("openai", ModelConfig::new("gpt-4o-mini".to_string())); - // Similar validation as above - will fail due to missing API keys but confirms the logic + // Similar validation as above - confirms the logic path - // Restore env vars - match saved_lead { - Some(val) => env::set_var("GOOSE_LEAD_MODEL", val), - None => env::remove_var("GOOSE_LEAD_MODEL"), - } - match saved_provider { - Some(val) => env::set_var("GOOSE_LEAD_PROVIDER", val), - None => env::remove_var("GOOSE_LEAD_PROVIDER"), - } - match saved_turns { - Some(val) => env::set_var("GOOSE_LEAD_TURNS", val), - None => env::remove_var("GOOSE_LEAD_TURNS"), - } + // EnvVarGuard will automatically restore all env vars when dropped } #[test] fn test_lead_model_env_vars_with_defaults() { - // Save current env vars - let saved_vars = [ - ("GOOSE_LEAD_MODEL", env::var("GOOSE_LEAD_MODEL").ok()), - ("GOOSE_LEAD_PROVIDER", env::var("GOOSE_LEAD_PROVIDER").ok()), - ("GOOSE_LEAD_TURNS", env::var("GOOSE_LEAD_TURNS").ok()), - ( - "GOOSE_LEAD_FAILURE_THRESHOLD", - env::var("GOOSE_LEAD_FAILURE_THRESHOLD").ok(), - ), - ( - "GOOSE_LEAD_FALLBACK_TURNS", - env::var("GOOSE_LEAD_FALLBACK_TURNS").ok(), - ), - ]; + let guard = EnvVarGuard::new(&[ + "GOOSE_LEAD_MODEL", + "GOOSE_LEAD_PROVIDER", + "GOOSE_LEAD_TURNS", + "GOOSE_LEAD_FAILURE_THRESHOLD", + "GOOSE_LEAD_FALLBACK_TURNS", + "OPENAI_API_KEY", + ]); // Clear all lead env vars - for (key, _) in &saved_vars { - env::remove_var(key); - } + guard.clear_all(); + + // Set fake API key to avoid keychain access + env::set_var("OPENAI_API_KEY", "fake-test-key"); // Set only the required lead model env::set_var("GOOSE_LEAD_MODEL", "grok-3"); @@ -304,15 +332,21 @@ mod tests { // This should use defaults for all other values let result = create("openai", ModelConfig::new("gpt-4o-mini".to_string())); - // Should attempt to create lead/worker provider (will fail due to missing API keys but confirms logic) + // Should attempt to create lead/worker provider with fake keys match result { Ok(_) => { - // Success means we have API keys and created the provider + // Success means the logic path worked } Err(error) => { - // Should fail due to missing API keys, confirming we tried to create providers - let error_msg = error.to_string(); - assert!(error_msg.contains("OPENAI_API_KEY") || error_msg.contains("secret")); + // Should not fail due to missing secrets since we provided fake keys + let error_msg = error.to_string().to_lowercase(); + // Allow various provider-level failures but not keychain issues + assert!( + error_msg.contains("invalid") + || error_msg.contains("unauthorized") + || error_msg.contains("model") + || !error_msg.contains("not found") + ); } } @@ -324,63 +358,47 @@ mod tests { let _result = create("openai", ModelConfig::new("gpt-4o-mini".to_string())); // Should still attempt to create lead/worker provider with custom settings - // Restore all env vars - for (key, value) in saved_vars { - match value { - Some(val) => env::set_var(key, val), - None => env::remove_var(key), - } - } + // EnvVarGuard will automatically restore all env vars when dropped } #[test] fn test_create_regular_provider_without_lead_config() { - // Save current env vars - let saved_lead = env::var("GOOSE_LEAD_MODEL").ok(); - let saved_provider = env::var("GOOSE_LEAD_PROVIDER").ok(); - let saved_turns = env::var("GOOSE_LEAD_TURNS").ok(); - let saved_threshold = env::var("GOOSE_LEAD_FAILURE_THRESHOLD").ok(); - let saved_fallback = env::var("GOOSE_LEAD_FALLBACK_TURNS").ok(); + let guard = EnvVarGuard::new(&[ + "GOOSE_LEAD_MODEL", + "GOOSE_LEAD_PROVIDER", + "GOOSE_LEAD_TURNS", + "GOOSE_LEAD_FAILURE_THRESHOLD", + "GOOSE_LEAD_FALLBACK_TURNS", + "OPENAI_API_KEY", + ]); // Ensure all GOOSE_LEAD_* variables are not set - env::remove_var("GOOSE_LEAD_MODEL"); - env::remove_var("GOOSE_LEAD_PROVIDER"); - env::remove_var("GOOSE_LEAD_TURNS"); - env::remove_var("GOOSE_LEAD_FAILURE_THRESHOLD"); - env::remove_var("GOOSE_LEAD_FALLBACK_TURNS"); + guard.clear_all(); + + // Set fake API key to avoid keychain access + env::set_var("OPENAI_API_KEY", "fake-test-key"); // This should try to create a regular provider let result = create("openai", ModelConfig::new("gpt-4o-mini".to_string())); - // The creation might succeed or fail depending on API keys + // The creation should proceed with fake API key match result { Ok(_) => { // If it succeeds, it means we created a regular provider successfully - // This would happen if API keys are available in the test environment } Err(error) => { - // If it fails, it should be due to missing API keys - let error_msg = error.to_string(); - assert!(error_msg.contains("OPENAI_API_KEY") || error_msg.contains("secret")); + // Should not fail due to missing secrets since we provided fake key + let error_msg = error.to_string().to_lowercase(); + // Allow provider-level failures but not keychain issues + assert!( + error_msg.contains("invalid") + || error_msg.contains("unauthorized") + || !error_msg.contains("not found") + ); } } - // Restore env vars - if let Some(val) = saved_lead { - env::set_var("GOOSE_LEAD_MODEL", val); - } - if let Some(val) = saved_provider { - env::set_var("GOOSE_LEAD_PROVIDER", val); - } - if let Some(val) = saved_turns { - env::set_var("GOOSE_LEAD_TURNS", val); - } - if let Some(val) = saved_threshold { - env::set_var("GOOSE_LEAD_FAILURE_THRESHOLD", val); - } - if let Some(val) = saved_fallback { - env::set_var("GOOSE_LEAD_FALLBACK_TURNS", val); - } + // EnvVarGuard will automatically restore all env vars when dropped } #[test]