diff --git a/crates/goose-cli/src/commands/configure.rs b/crates/goose-cli/src/commands/configure.rs index a112f18f6c83..d5ae3f88e3c6 100644 --- a/crates/goose-cli/src/commands/configure.rs +++ b/crates/goose-cli/src/commands/configure.rs @@ -430,6 +430,7 @@ fn select_model_from_list( fn try_store_secret(config: &Config, key_name: &str, value: String) -> anyhow::Result { match config.set_secret(key_name, &value) { Ok(_) => Ok(true), + Err(ConfigError::FallbackToFileStorage) => Ok(true), Err(e) => { cliclack::outro(style(format!( "Failed to store {} securely: {}. Please ensure your system's secure storage is accessible. Alternatively you can run with GOOSE_DISABLE_KEYRING=true or set the key in your environment variables", @@ -500,7 +501,6 @@ pub async fn configure_provider_dialog() -> anyhow::Result { } } None => { - // No env var, check config/secret storage let existing: Result = if key.secret { config.get_secret(&key.name) } else { @@ -565,7 +565,9 @@ pub async fn configure_provider_dialog() -> anyhow::Result { }; if key.secret { - config.set_secret(&key.name, &value)?; + if !try_store_secret(config, &key.name, value)? { + return Ok(false); + } } else { config.set_param(&key.name, &value)?; } @@ -734,7 +736,7 @@ fn prompt_extension_name(placeholder: &str) -> anyhow::Result { } fn collect_env_vars() -> anyhow::Result<(HashMap, Vec)> { - let mut envs = HashMap::new(); + let envs = HashMap::new(); let mut env_keys = Vec::new(); let config = Config::global(); @@ -751,12 +753,10 @@ fn collect_env_vars() -> anyhow::Result<(HashMap, Vec)> .mask('▪') .interact()?; - match config.set_secret(&key, &value) { - Ok(_) => env_keys.push(key), - Err(_) => { - envs.insert(key, value); - } + if !try_store_secret(config, &key, value)? { + return Err(anyhow::anyhow!("Failed to store secret")); } + env_keys.push(key); if !cliclack::confirm("Add another environment variable?").interact()? { break; diff --git a/crates/goose-cli/src/recipes/recipe.rs b/crates/goose-cli/src/recipes/recipe.rs index ff722657289a..6d0fa2f47e31 100644 --- a/crates/goose-cli/src/recipes/recipe.rs +++ b/crates/goose-cli/src/recipes/recipe.rs @@ -97,8 +97,15 @@ pub fn collect_missing_secrets(requirements: &[SecretRequirement]) -> Result<()> .unwrap_or_else(|_| String::new()); if !value.trim().is_empty() { - config.set_secret(&req.key, &value)?; - println!("✅ Secret stored securely for {}", req.extension_name); + if let Err(e) = config.set_secret(&req.key, &value) { + println!("⚠️ Failed to store secret in secure storage: {}. Secret available for this session only.", e); + println!( + " Consider setting {} as an environment variable for future use.", + req.key + ); + } else { + println!("✅ Secret stored securely for {}", req.extension_name); + } } else { println!("⏭️ Skipped {} for {}", req.key, req.extension_name); } diff --git a/crates/goose/src/config/base.rs b/crates/goose/src/config/base.rs index 90e21a28d7e6..de57ff2bab1c 100644 --- a/crates/goose/src/config/base.rs +++ b/crates/goose/src/config/base.rs @@ -33,6 +33,8 @@ pub enum ConfigError { KeyringError(String), #[error("Failed to lock config file: {0}")] LockError(String), + #[error("Secret stored using file-based fallback")] + FallbackToFileStorage, } impl From for ConfigError { @@ -540,30 +542,25 @@ impl Config { pub fn all_secrets(&self) -> Result, ConfigError> { match &self.secrets { SecretStorage::Keyring { service } => { - let entry = Entry::new(service, KEYRING_USERNAME)?; + let result = + self.handle_keyring_operation(|entry| entry.get_password(), service, None); - match entry.get_password() { + match result { 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())), - } - } - 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(ConfigError::FallbackToFileStorage) => self.fallback_to_file_storage(), + Err(ConfigError::KeyringError(msg)) + if msg.contains("No entry found") + || msg.contains("No matching entry found") => + { + Ok(HashMap::new()) } - } else { - Ok(HashMap::new()) + Err(e) => Err(e), } } + SecretStorage::File { path } => self.read_secrets_from_file(path), } } @@ -778,8 +775,11 @@ impl Config { 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)?; + self.handle_keyring_operation( + |entry| entry.set_password(&json_value), + service, + Some(&values), + )?; } SecretStorage::File { path } => { let yaml_value = serde_yaml::to_string(&values)?; @@ -809,8 +809,11 @@ impl Config { 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)?; + self.handle_keyring_operation( + |entry| entry.set_password(&json_value), + service, + Some(&values), + )?; } SecretStorage::File { path } => { let yaml_value = serde_yaml::to_string(&values)?; @@ -819,6 +822,97 @@ impl Config { }; Ok(()) } + + /// Read secrets from a YAML file + fn read_secrets_from_file(&self, path: &Path) -> Result, ConfigError> { + 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()), + } + } else { + Ok(HashMap::new()) + } + } + + /// Get the path to the secrets storage file + fn secrets_file_path() -> PathBuf { + Paths::config_dir().join("secrets.yaml") + } + + /// Perform fallback to file storage when keyring is unavailable + fn fallback_to_file_storage(&self) -> Result, ConfigError> { + let path = Self::secrets_file_path(); + self.read_secrets_from_file(&path) + } + + /// Write secrets to file storage (used for fallback) + fn write_secrets_to_file(&self, values: &HashMap) -> Result<(), ConfigError> { + std::fs::create_dir_all(Paths::config_dir())?; + let path = Self::secrets_file_path(); + let yaml_value = serde_yaml::to_string(values)?; + std::fs::write(path, yaml_value)?; + Ok(()) + } + + /// Check if an error string indicates a keyring availability issue that should trigger fallback + fn is_keyring_availability_error(&self, error_str: &str) -> bool { + error_str.contains("keyring") + || error_str.contains("DBus error") + || error_str.contains("org.freedesktop.secrets") + || error_str.contains("couldn't access platform secure storage") + } + + /// Get a keyring entry for the specified service + fn get_keyring_entry(service: &str) -> Result { + Entry::new(service, KEYRING_USERNAME) + } + + /// Handle keyring errors with automatic fallback to file storage + fn handle_keyring_fallback_error( + &self, + keyring_err: &keyring::Error, + fallback_values: Option<&HashMap>, + ) -> Result { + if self.is_keyring_availability_error(&keyring_err.to_string()) { + std::env::set_var("GOOSE_DISABLE_KEYRING", "1"); + tracing::warn!("Keyring unavailable. Using file storage for secrets."); + + if let Some(values) = fallback_values { + self.write_secrets_to_file(values)?; + Err(ConfigError::FallbackToFileStorage) + } else { + Err(ConfigError::FallbackToFileStorage) + } + } else { + Err(ConfigError::KeyringError(keyring_err.to_string())) + } + } + + /// Handle keyring operation with automatic fallback to file storage + fn handle_keyring_operation( + &self, + operation: impl FnOnce(keyring::Entry) -> Result, + service: &str, + fallback_values: Option<&HashMap>, + ) -> Result { + // Try to get the keyring entry and perform the operation + let entry = match Self::get_keyring_entry(service) { + Ok(entry) => entry, + Err(keyring_err) => { + return self.handle_keyring_fallback_error(&keyring_err, fallback_values); + } + }; + + // Perform the operation + match operation(entry) { + Ok(result) => Ok(result), + Err(keyring_err) => self.handle_keyring_fallback_error(&keyring_err, fallback_values), + } + } } config_value!(CLAUDE_CODE_COMMAND, OsString, "claude"); @@ -878,7 +972,6 @@ mod tests { use super::*; use serial_test::serial; use tempfile::NamedTempFile; - #[test] fn test_basic_config() -> Result<(), ConfigError> { let config = new_test_config();