diff --git a/crates/goose/src/config/base.rs b/crates/goose/src/config/base.rs index ddae0fd2..0cff5f85 100644 --- a/crates/goose/src/config/base.rs +++ b/crates/goose/src/config/base.rs @@ -19,9 +19,6 @@ const KEYRING_SERVICE: &str = "goose"; const KEYRING_USERNAME: &str = "secrets"; pub const CONFIG_YAML_NAME: &str = "config.yaml"; -#[cfg(test)] -const TEST_KEYRING_SERVICE: &str = "goose-test"; - #[derive(Error, Debug)] pub enum ConfigError { #[error("Configuration value not found: {0}")] @@ -729,6 +726,31 @@ impl Config { .and_then(|v| Ok(serde_json::from_value(v.clone())?)) } + /// Get secrets. If primary is in env, use env for all keys. Otherwise use secret storage. + pub fn get_secrets( + &self, + primary: &str, + maybe_secret: &[&str], + ) -> Result, ConfigError> { + let use_env = env::var(primary.to_uppercase()).is_ok(); + let get_value = |key: &str| -> Result { + if use_env { + env::var(key.to_uppercase()).map_err(|_| ConfigError::NotFound(key.to_string())) + } else { + self.get_secret(key) + } + }; + + let mut result = HashMap::new(); + result.insert(primary.to_string(), get_value(primary)?); + for &key in maybe_secret { + if let Ok(v) = get_value(key) { + result.insert(key.to_string(), v); + } + } + Ok(result) + } + /// Set a secret value in the system keyring. /// /// This will store the value in a single JSON object in the system keyring, @@ -853,19 +875,9 @@ mod tests { 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(); - let config = Config::new(temp_file.path(), TEST_KEYRING_SERVICE)?; + let config = new_test_config(); // Set a simple string value config.set_param("test_key", "test_value")?; @@ -890,8 +902,7 @@ mod tests { field2: i32, } - let temp_file = NamedTempFile::new().unwrap(); - let config = Config::new(temp_file.path(), TEST_KEYRING_SERVICE)?; + let config = new_test_config(); // Set a complex value config.set_param( @@ -911,8 +922,7 @@ mod tests { #[test] fn test_missing_value() { - let temp_file = NamedTempFile::new().unwrap(); - let config = Config::new(temp_file.path(), TEST_KEYRING_SERVICE).unwrap(); + let config = new_test_config(); let result: Result = config.get_param("nonexistent_key"); assert!(matches!(result, Err(ConfigError::NotFound(_)))); @@ -920,14 +930,15 @@ mod tests { #[test] fn test_yaml_formatting() -> Result<(), ConfigError> { - let temp_file = NamedTempFile::new().unwrap(); - let config = Config::new(temp_file.path(), TEST_KEYRING_SERVICE)?; + let config_file = NamedTempFile::new().unwrap(); + let secrets_file = NamedTempFile::new().unwrap(); + let config = Config::new_with_file_secrets(config_file.path(), secrets_file.path())?; config.set_param("key1", "value1")?; config.set_param("key2", 42)?; // Read the file directly to check YAML formatting - let content = std::fs::read_to_string(temp_file.path())?; + let content = std::fs::read_to_string(config_file.path())?; assert!(content.contains("key1: value1")); assert!(content.contains("key2: 42")); @@ -936,8 +947,7 @@ mod tests { #[test] fn test_value_management() -> Result<(), ConfigError> { - let temp_file = NamedTempFile::new().unwrap(); - let config = Config::new(temp_file.path(), TEST_KEYRING_SERVICE)?; + let config = new_test_config(); config.set_param("test_key", "test_value")?; config.set_param("another_key", 42)?; @@ -953,9 +963,7 @@ mod tests { #[test] fn test_file_based_secrets_management() -> Result<(), ConfigError> { - let config_file = NamedTempFile::new().unwrap(); - let secrets_file = NamedTempFile::new().unwrap(); - let config = Config::new_with_file_secrets(config_file.path(), secrets_file.path())?; + let config = new_test_config(); config.set_secret("key", &"value")?; @@ -973,9 +981,7 @@ 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)?; + let config = new_test_config(); // Test setting and getting a simple secret config.set_secret("api_key", &Value::String("secret123".to_string()))?; @@ -993,16 +999,13 @@ mod tests { let result: Result = config.get_secret("api_key"); assert!(matches!(result, Err(ConfigError::NotFound(_)))); - cleanup_keyring()?; Ok(()) } #[test] #[serial] fn test_multiple_secrets() -> Result<(), ConfigError> { - cleanup_keyring()?; - let temp_file = NamedTempFile::new().unwrap(); - let config = Config::new(temp_file.path(), TEST_KEYRING_SERVICE)?; + let config = new_test_config(); // Set multiple secrets config.set_secret("key1", &Value::String("secret1".to_string()))?; @@ -1023,7 +1026,6 @@ mod tests { assert!(matches!(result1, Err(ConfigError::NotFound(_)))); assert_eq!(value2, "secret2"); - cleanup_keyring()?; Ok(()) } @@ -1032,8 +1034,7 @@ mod tests { use std::sync::{Arc, Barrier, Mutex}; use std::thread; - let temp_file = NamedTempFile::new().unwrap(); - let config = Arc::new(Config::new(temp_file.path(), TEST_KEYRING_SERVICE)?); + let config = Arc::new(new_test_config()); let barrier = Arc::new(Barrier::new(3)); // For 3 concurrent threads let values = Arc::new(Mutex::new(Mapping::new())); let mut handles = vec![]; @@ -1103,8 +1104,9 @@ mod tests { #[test] fn test_config_recovery_from_backup() -> Result<(), ConfigError> { - let temp_file = NamedTempFile::new().unwrap(); - let config = Config::new(temp_file.path(), TEST_KEYRING_SERVICE)?; + let config_file = NamedTempFile::new().unwrap(); + let secrets_file = NamedTempFile::new().unwrap(); + let config = Config::new_with_file_secrets(config_file.path(), secrets_file.path())?; // Create a valid config first config.set_param("key1", "value1")?; @@ -1129,7 +1131,7 @@ mod tests { } // Corrupt the main config file - std::fs::write(temp_file.path(), "invalid: yaml: content: [unclosed")?; + std::fs::write(config_file.path(), "invalid: yaml: content: [unclosed")?; // Try to load values - should recover from backup let recovered_values = config.all_values()?; @@ -1146,11 +1148,12 @@ mod tests { #[test] fn test_config_recovery_creates_fresh_file() -> Result<(), ConfigError> { - let temp_file = NamedTempFile::new().unwrap(); - let config = Config::new(temp_file.path(), TEST_KEYRING_SERVICE)?; + let config_file = NamedTempFile::new().unwrap(); + let secrets_file = NamedTempFile::new().unwrap(); + let config = Config::new_with_file_secrets(config_file.path(), secrets_file.path())?; // Create a corrupted config file with no backup - std::fs::write(temp_file.path(), "invalid: yaml: content: [unclosed")?; + std::fs::write(config_file.path(), "invalid: yaml: content: [unclosed")?; // Try to load values - should create a fresh default config let recovered_values = config.all_values()?; @@ -1159,7 +1162,7 @@ mod tests { assert_eq!(recovered_values.len(), 0); // Verify that a clean config file was written to disk - let file_content = std::fs::read_to_string(temp_file.path())?; + let file_content = std::fs::read_to_string(config_file.path())?; // Should be valid YAML (empty object) let parsed: serde_yaml::Value = serde_yaml::from_str(&file_content)?; @@ -1174,15 +1177,15 @@ mod tests { #[test] fn test_config_file_creation_when_missing() -> Result<(), ConfigError> { - let temp_file = NamedTempFile::new().unwrap(); - let config_path = temp_file.path(); + let config_file = NamedTempFile::new().unwrap(); + let secrets_file = NamedTempFile::new().unwrap(); + let config_path = config_file.path().to_path_buf(); + let config = Config::new_with_file_secrets(&config_path, secrets_file.path())?; // Delete the file to simulate it not existing - std::fs::remove_file(config_path)?; + std::fs::remove_file(&config_path)?; assert!(!config_path.exists()); - let config = Config::new(config_path, TEST_KEYRING_SERVICE)?; - // Try to load values - should create a fresh default config file let values = config.all_values()?; @@ -1193,7 +1196,7 @@ mod tests { assert!(config_path.exists()); // Verify that it's valid YAML - let file_content = std::fs::read_to_string(config_path)?; + let file_content = std::fs::read_to_string(&config_path)?; let parsed: serde_yaml::Value = serde_yaml::from_str(&file_content)?; assert!(parsed.is_mapping()); @@ -1206,9 +1209,10 @@ mod tests { #[test] fn test_config_recovery_from_backup_when_missing() -> Result<(), ConfigError> { - let temp_file = NamedTempFile::new().unwrap(); - let config_path = temp_file.path(); - let config = Config::new(config_path, TEST_KEYRING_SERVICE)?; + let config_file = NamedTempFile::new().unwrap(); + let secrets_file = NamedTempFile::new().unwrap(); + let config_path = config_file.path().to_path_buf(); + let config = Config::new_with_file_secrets(&config_path, secrets_file.path())?; // First, create a config with some data config.set_param("test_key_backup", "backup_value")?; @@ -1223,7 +1227,7 @@ mod tests { assert!(primary_backup.exists(), "Backup should exist after writes"); // Now delete the main config file to simulate it being lost - std::fs::remove_file(config_path)?; + std::fs::remove_file(&config_path)?; assert!(!config_path.exists()); // Try to load values - should recover from backup @@ -1251,19 +1255,20 @@ mod tests { #[test] fn test_atomic_write_prevents_corruption() -> Result<(), ConfigError> { - let temp_file = NamedTempFile::new().unwrap(); - let config = Config::new(temp_file.path(), TEST_KEYRING_SERVICE)?; + let config_file = NamedTempFile::new().unwrap(); + let secrets_file = NamedTempFile::new().unwrap(); + let config = Config::new_with_file_secrets(config_file.path(), secrets_file.path())?; // Set initial values config.set_param("key1", "value1")?; // Verify the config file exists and is valid - assert!(temp_file.path().exists()); - let content = std::fs::read_to_string(temp_file.path())?; + assert!(config_file.path().exists()); + let content = std::fs::read_to_string(config_file.path())?; assert!(serde_yaml::from_str::(&content).is_ok()); // The temp file should not exist after successful write - let temp_path = temp_file.path().with_extension("tmp"); + let temp_path = config_file.path().with_extension("tmp"); assert!(!temp_path.exists(), "Temporary file should be cleaned up"); Ok(()) @@ -1271,8 +1276,7 @@ mod tests { #[test] fn test_backup_rotation() -> Result<(), ConfigError> { - let temp_file = NamedTempFile::new().unwrap(); - let config = Config::new(temp_file.path(), TEST_KEYRING_SERVICE)?; + let config = new_test_config(); // Create multiple versions to test rotation for i in 1..=7 { @@ -1467,8 +1471,7 @@ mod tests { #[test] fn test_env_var_with_config_integration() -> Result<(), ConfigError> { - let temp_file = NamedTempFile::new().unwrap(); - let config = Config::new(temp_file.path(), TEST_KEYRING_SERVICE)?; + let config = new_test_config(); // Test string environment variable (the original issue case) std::env::set_var("PROVIDER", "ANTHROPIC"); @@ -1507,8 +1510,7 @@ mod tests { #[test] fn test_env_var_precedence_over_config_file() -> Result<(), ConfigError> { - let temp_file = NamedTempFile::new().unwrap(); - let config = Config::new(temp_file.path(), TEST_KEYRING_SERVICE)?; + let config = new_test_config(); // Set value in config file config.set_param("test_precedence", "file_value")?; @@ -1529,4 +1531,63 @@ mod tests { Ok(()) } + + #[test] + fn get_secrets_primary_from_env_uses_env_for_secondary() { + temp_env::with_vars( + [ + ("TEST_PRIMARY", Some("primary_env")), + ("TEST_SECONDARY", Some("secondary_env")), + ], + || { + let config = new_test_config(); + let secrets = config + .get_secrets("TEST_PRIMARY", &["TEST_SECONDARY"]) + .unwrap(); + + assert_eq!(secrets["TEST_PRIMARY"], "primary_env"); + assert_eq!(secrets["TEST_SECONDARY"], "secondary_env"); + }, + ); + } + + #[test] + fn get_secrets_primary_from_secret_uses_secret_for_secondary() { + temp_env::with_vars( + [("TEST_PRIMARY", None::<&str>), ("TEST_SECONDARY", None)], + || { + let config = new_test_config(); + config + .set_secret("TEST_PRIMARY", &"primary_secret") + .unwrap(); + config + .set_secret("TEST_SECONDARY", &"secondary_secret") + .unwrap(); + + let secrets = config + .get_secrets("TEST_PRIMARY", &["TEST_SECONDARY"]) + .unwrap(); + + assert_eq!(secrets["TEST_PRIMARY"], "primary_secret"); + assert_eq!(secrets["TEST_SECONDARY"], "secondary_secret"); + }, + ); + } + + #[test] + fn get_secrets_primary_missing_returns_error() { + temp_env::with_vars([("TEST_PRIMARY", None::<&str>)], || { + let config = new_test_config(); + + let result = config.get_secrets("TEST_PRIMARY", &[]); + + assert!(matches!(result, Err(ConfigError::NotFound(_)))); + }); + } + + fn new_test_config() -> Config { + let config_file = NamedTempFile::new().unwrap(); + let secrets_file = NamedTempFile::new().unwrap(); + Config::new_with_file_secrets(config_file.path(), secrets_file.path()).unwrap() + } } diff --git a/crates/goose/src/config/signup_tetrate/tests.rs b/crates/goose/src/config/signup_tetrate/tests.rs index 64f9136b..75124e7f 100644 --- a/crates/goose/src/config/signup_tetrate/tests.rs +++ b/crates/goose/src/config/signup_tetrate/tests.rs @@ -65,7 +65,8 @@ fn test_configure_tetrate() { // Create a test config with temporary paths let temp_dir = TempDir::new().unwrap(); let config_path = temp_dir.path().join("test_config.yaml"); - let config = Config::new(&config_path, "test_service").unwrap(); + let secrets_path = temp_dir.path().join("test_secrets.yaml"); + let config = Config::new_with_file_secrets(&config_path, &secrets_path).unwrap(); // Configure with a test API key let test_key = "test-api-key-123".to_string(); diff --git a/crates/goose/src/providers/factory.rs b/crates/goose/src/providers/factory.rs index 27f6e1f3..11aab4dd 100644 --- a/crates/goose/src/providers/factory.rs +++ b/crates/goose/src/providers/factory.rs @@ -276,12 +276,17 @@ mod tests { #[tokio::test] async fn test_create_lead_worker_provider() { + // Both API keys needed: openai for worker, anthropic for lead (GOOSE_LEAD_PROVIDER=anthropic) let _guard = EnvVarGuard::new(&[ "GOOSE_LEAD_MODEL", "GOOSE_LEAD_PROVIDER", "GOOSE_LEAD_TURNS", + "OPENAI_API_KEY", + "ANTHROPIC_API_KEY", ]); + _guard.set("OPENAI_API_KEY", "fake-openai-no-keyring"); + _guard.set("ANTHROPIC_API_KEY", "fake-anthropic-no-keyring"); _guard.set("GOOSE_LEAD_MODEL", "gpt-4o"); let gpt4mini_config = ModelConfig::new_or_fail("gpt-4o-mini"); @@ -309,8 +314,10 @@ mod tests { "GOOSE_LEAD_TURNS", "GOOSE_LEAD_FAILURE_THRESHOLD", "GOOSE_LEAD_FALLBACK_TURNS", + "OPENAI_API_KEY", ]); + _guard.set("OPENAI_API_KEY", "fake-openai-no-keyring"); _guard.set("GOOSE_LEAD_MODEL", "grok-3"); let result = create("openai", ModelConfig::new_or_fail("gpt-4o-mini")).await; @@ -338,8 +345,10 @@ mod tests { "GOOSE_LEAD_TURNS", "GOOSE_LEAD_FAILURE_THRESHOLD", "GOOSE_LEAD_FALLBACK_TURNS", + "OPENAI_API_KEY", ]); + _guard.set("OPENAI_API_KEY", "fake-openai-no-keyring"); let result = create("openai", ModelConfig::new_or_fail("gpt-4o-mini")).await; match result { diff --git a/crates/goose/src/providers/litellm.rs b/crates/goose/src/providers/litellm.rs index 308740e0..e62fea72 100644 --- a/crates/goose/src/providers/litellm.rs +++ b/crates/goose/src/providers/litellm.rs @@ -30,19 +30,19 @@ pub struct LiteLLMProvider { impl LiteLLMProvider { pub async fn from_env(model: ModelConfig) -> Result { let config = crate::config::Config::global(); - let api_key: String = config - .get_secret("LITELLM_API_KEY") - .unwrap_or_else(|_| String::new()); + let secrets = config + .get_secrets("LITELLM_API_KEY", &["LITELLM_CUSTOM_HEADERS"]) + .unwrap_or_default(); + let api_key = secrets.get("LITELLM_API_KEY").cloned().unwrap_or_default(); let host: String = config .get_param("LITELLM_HOST") .unwrap_or_else(|_| "https://api.litellm.ai".to_string()); let base_path: String = config .get_param("LITELLM_BASE_PATH") .unwrap_or_else(|_| "v1/chat/completions".to_string()); - let custom_headers: Option> = config - .get_secret("LITELLM_CUSTOM_HEADERS") - .or_else(|_| config.get_param("LITELLM_CUSTOM_HEADERS")) - .ok() + let custom_headers: Option> = secrets + .get("LITELLM_CUSTOM_HEADERS") + .cloned() .map(parse_custom_headers); let timeout_secs: u64 = config.get_param("LITELLM_TIMEOUT").unwrap_or(600); diff --git a/crates/goose/src/providers/openai.rs b/crates/goose/src/providers/openai.rs index cf98dcbb..f2cd11a5 100644 --- a/crates/goose/src/providers/openai.rs +++ b/crates/goose/src/providers/openai.rs @@ -67,7 +67,8 @@ impl OpenAiProvider { let model = model.with_fast(OPEN_AI_DEFAULT_FAST_MODEL.to_string()); let config = crate::config::Config::global(); - let api_key: String = config.get_secret("OPENAI_API_KEY")?; + let secrets = config.get_secrets("OPENAI_API_KEY", &["OPENAI_CUSTOM_HEADERS"])?; + let api_key = secrets.get("OPENAI_API_KEY").unwrap().clone(); let host: String = config .get_param("OPENAI_HOST") .unwrap_or_else(|_| "https://api.openai.com".to_string()); @@ -76,10 +77,9 @@ impl OpenAiProvider { .unwrap_or_else(|_| "v1/chat/completions".to_string()); let organization: Option = config.get_param("OPENAI_ORGANIZATION").ok(); let project: Option = config.get_param("OPENAI_PROJECT").ok(); - let custom_headers: Option> = config - .get_secret("OPENAI_CUSTOM_HEADERS") - .or_else(|_| config.get_param("OPENAI_CUSTOM_HEADERS")) - .ok() + let custom_headers: Option> = secrets + .get("OPENAI_CUSTOM_HEADERS") + .cloned() .map(parse_custom_headers); let timeout_secs: u64 = config.get_param("OPENAI_TIMEOUT").unwrap_or(600);