tkmind/tkmind_go
1
0
Fork 0
Code Issues Pull Requests Actions 5 Packages Projects Releases Wiki Activity

fix: prevent keychain requests during cargo test (#6219)

Browse Source 
Signed-off-by: Adrian Cole <adrian@tetrate.io>
This commit is contained in:
Adrian Cole
2025-12-22 10:37:30 +08:00
committed by GitHub
parent 02a3a1ead1
commit 5942e9c4b2
5 changed files with 148 additions and 77 deletions
Download Patch File Download Diff File Expand all files Collapse all files
crates/goose/src/config/base.rs
+125 -64
View File
@@ -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<HashMap<String, String>, ConfigError> {
let use_env = env::var(primary.to_uppercase()).is_ok();
let get_value = |key: &str| -> Result<String, ConfigError> {
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<String, ConfigError> = 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<String, ConfigError> = 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::<serde_yaml::Value>(&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()
}
}
crates/goose/src/config/signup_tetrate/tests.rs
+2 -1
View File
@@ -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();
crates/goose/src/providers/factory.rs
+9
View File
@@ -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 {
crates/goose/src/providers/litellm.rs
+7 -7
View File
@@ -30,19 +30,19 @@ pub struct LiteLLMProvider {
impl LiteLLMProvider {
pub async fn from_env(model: ModelConfig) -> Result<Self> {
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<HashMap<String, String>> = config
.get_secret("LITELLM_CUSTOM_HEADERS")
.or_else(|_| config.get_param("LITELLM_CUSTOM_HEADERS"))
.ok()
let custom_headers: Option<HashMap<String, String>> = secrets
.get("LITELLM_CUSTOM_HEADERS")
.cloned()
.map(parse_custom_headers);
let timeout_secs: u64 = config.get_param("LITELLM_TIMEOUT").unwrap_or(600);
crates/goose/src/providers/openai.rs
+5 -5
View File
@@ -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<String> = config.get_param("OPENAI_ORGANIZATION").ok();
let project: Option<String> = config.get_param("OPENAI_PROJECT").ok();
let custom_headers: Option<HashMap<String, String>> = config
.get_secret("OPENAI_CUSTOM_HEADERS")
.or_else(|_| config.get_param("OPENAI_CUSTOM_HEADERS"))
.ok()
let custom_headers: Option<HashMap<String, String>> = secrets
.get("OPENAI_CUSTOM_HEADERS")
.cloned()
.map(parse_custom_headers);
let timeout_secs: u64 = config.get_param("OPENAI_TIMEOUT").unwrap_or(600);