feat(cli): graceful fallback for keyring failures (#5808)
Signed-off-by: sheikhlimon <sheikhlimon404@gmail.com>
This commit is contained in:
@@ -493,6 +493,7 @@ fn select_model_from_list(
|
|||||||
fn try_store_secret(config: &Config, key_name: &str, value: String) -> anyhow::Result<bool> {
|
fn try_store_secret(config: &Config, key_name: &str, value: String) -> anyhow::Result<bool> {
|
||||||
match config.set_secret(key_name, &value) {
|
match config.set_secret(key_name, &value) {
|
||||||
Ok(_) => Ok(true),
|
Ok(_) => Ok(true),
|
||||||
|
Err(ConfigError::FallbackToFileStorage) => Ok(true),
|
||||||
Err(e) => {
|
Err(e) => {
|
||||||
cliclack::outro(style(format!(
|
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",
|
"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",
|
||||||
@@ -563,7 +564,6 @@ pub async fn configure_provider_dialog() -> anyhow::Result<bool> {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
None => {
|
None => {
|
||||||
// No env var, check config/secret storage
|
|
||||||
let existing: Result<String, _> = if key.secret {
|
let existing: Result<String, _> = if key.secret {
|
||||||
config.get_secret(&key.name)
|
config.get_secret(&key.name)
|
||||||
} else {
|
} else {
|
||||||
@@ -628,7 +628,9 @@ pub async fn configure_provider_dialog() -> anyhow::Result<bool> {
|
|||||||
};
|
};
|
||||||
|
|
||||||
if key.secret {
|
if key.secret {
|
||||||
config.set_secret(&key.name, &value)?;
|
if !try_store_secret(config, &key.name, value)? {
|
||||||
|
return Ok(false);
|
||||||
|
}
|
||||||
} else {
|
} else {
|
||||||
config.set_param(&key.name, &value)?;
|
config.set_param(&key.name, &value)?;
|
||||||
}
|
}
|
||||||
@@ -799,7 +801,7 @@ fn prompt_extension_name(placeholder: &str) -> anyhow::Result<String> {
|
|||||||
}
|
}
|
||||||
|
|
||||||
fn collect_env_vars() -> anyhow::Result<(HashMap<String, String>, Vec<String>)> {
|
fn collect_env_vars() -> anyhow::Result<(HashMap<String, String>, Vec<String>)> {
|
||||||
let mut envs = HashMap::new();
|
let envs = HashMap::new();
|
||||||
let mut env_keys = Vec::new();
|
let mut env_keys = Vec::new();
|
||||||
let config = Config::global();
|
let config = Config::global();
|
||||||
|
|
||||||
@@ -816,12 +818,10 @@ fn collect_env_vars() -> anyhow::Result<(HashMap<String, String>, Vec<String>)>
|
|||||||
.mask('▪')
|
.mask('▪')
|
||||||
.interact()?;
|
.interact()?;
|
||||||
|
|
||||||
match config.set_secret(&key, &value) {
|
if !try_store_secret(config, &key, value)? {
|
||||||
Ok(_) => env_keys.push(key),
|
return Err(anyhow::anyhow!("Failed to store secret"));
|
||||||
Err(_) => {
|
|
||||||
envs.insert(key, value);
|
|
||||||
}
|
|
||||||
}
|
}
|
||||||
|
env_keys.push(key);
|
||||||
|
|
||||||
if !cliclack::confirm("Add another environment variable?").interact()? {
|
if !cliclack::confirm("Add another environment variable?").interact()? {
|
||||||
break;
|
break;
|
||||||
|
|||||||
@@ -97,8 +97,15 @@ pub fn collect_missing_secrets(requirements: &[SecretRequirement]) -> Result<()>
|
|||||||
.unwrap_or_else(|_| String::new());
|
.unwrap_or_else(|_| String::new());
|
||||||
|
|
||||||
if !value.trim().is_empty() {
|
if !value.trim().is_empty() {
|
||||||
config.set_secret(&req.key, &value)?;
|
if let Err(e) = config.set_secret(&req.key, &value) {
|
||||||
println!("✅ Secret stored securely for {}", req.extension_name);
|
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 {
|
} else {
|
||||||
println!("⏭️ Skipped {} for {}", req.key, req.extension_name);
|
println!("⏭️ Skipped {} for {}", req.key, req.extension_name);
|
||||||
}
|
}
|
||||||
|
|||||||
+114
-21
@@ -33,6 +33,8 @@ pub enum ConfigError {
|
|||||||
KeyringError(String),
|
KeyringError(String),
|
||||||
#[error("Failed to lock config file: {0}")]
|
#[error("Failed to lock config file: {0}")]
|
||||||
LockError(String),
|
LockError(String),
|
||||||
|
#[error("Secret stored using file-based fallback")]
|
||||||
|
FallbackToFileStorage,
|
||||||
}
|
}
|
||||||
|
|
||||||
impl From<serde_json::Error> for ConfigError {
|
impl From<serde_json::Error> for ConfigError {
|
||||||
@@ -540,30 +542,25 @@ impl Config {
|
|||||||
pub fn all_secrets(&self) -> Result<HashMap<String, Value>, ConfigError> {
|
pub fn all_secrets(&self) -> Result<HashMap<String, Value>, ConfigError> {
|
||||||
match &self.secrets {
|
match &self.secrets {
|
||||||
SecretStorage::Keyring { service } => {
|
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) => {
|
Ok(content) => {
|
||||||
let values: HashMap<String, Value> = serde_json::from_str(&content)?;
|
let values: HashMap<String, Value> = serde_json::from_str(&content)?;
|
||||||
Ok(values)
|
Ok(values)
|
||||||
}
|
}
|
||||||
Err(keyring::Error::NoEntry) => Ok(HashMap::new()),
|
Err(ConfigError::FallbackToFileStorage) => self.fallback_to_file_storage(),
|
||||||
Err(e) => Err(ConfigError::KeyringError(e.to_string())),
|
Err(ConfigError::KeyringError(msg))
|
||||||
}
|
if msg.contains("No entry found")
|
||||||
}
|
|| msg.contains("No matching entry found") =>
|
||||||
SecretStorage::File { path } => {
|
{
|
||||||
if path.exists() {
|
Ok(HashMap::new())
|
||||||
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 {
|
Err(e) => Err(e),
|
||||||
Ok(HashMap::new())
|
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
SecretStorage::File { path } => self.read_secrets_from_file(path),
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -778,8 +775,11 @@ impl Config {
|
|||||||
match &self.secrets {
|
match &self.secrets {
|
||||||
SecretStorage::Keyring { service } => {
|
SecretStorage::Keyring { service } => {
|
||||||
let json_value = serde_json::to_string(&values)?;
|
let json_value = serde_json::to_string(&values)?;
|
||||||
let entry = Entry::new(service, KEYRING_USERNAME)?;
|
self.handle_keyring_operation(
|
||||||
entry.set_password(&json_value)?;
|
|entry| entry.set_password(&json_value),
|
||||||
|
service,
|
||||||
|
Some(&values),
|
||||||
|
)?;
|
||||||
}
|
}
|
||||||
SecretStorage::File { path } => {
|
SecretStorage::File { path } => {
|
||||||
let yaml_value = serde_yaml::to_string(&values)?;
|
let yaml_value = serde_yaml::to_string(&values)?;
|
||||||
@@ -809,8 +809,11 @@ impl Config {
|
|||||||
match &self.secrets {
|
match &self.secrets {
|
||||||
SecretStorage::Keyring { service } => {
|
SecretStorage::Keyring { service } => {
|
||||||
let json_value = serde_json::to_string(&values)?;
|
let json_value = serde_json::to_string(&values)?;
|
||||||
let entry = Entry::new(service, KEYRING_USERNAME)?;
|
self.handle_keyring_operation(
|
||||||
entry.set_password(&json_value)?;
|
|entry| entry.set_password(&json_value),
|
||||||
|
service,
|
||||||
|
Some(&values),
|
||||||
|
)?;
|
||||||
}
|
}
|
||||||
SecretStorage::File { path } => {
|
SecretStorage::File { path } => {
|
||||||
let yaml_value = serde_yaml::to_string(&values)?;
|
let yaml_value = serde_yaml::to_string(&values)?;
|
||||||
@@ -819,6 +822,97 @@ impl Config {
|
|||||||
};
|
};
|
||||||
Ok(())
|
Ok(())
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Read secrets from a YAML file
|
||||||
|
fn read_secrets_from_file(&self, path: &Path) -> Result<HashMap<String, Value>, 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<HashMap<String, Value>, 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<String, Value>) -> 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<keyring::Entry, keyring::Error> {
|
||||||
|
Entry::new(service, KEYRING_USERNAME)
|
||||||
|
}
|
||||||
|
|
||||||
|
/// Handle keyring errors with automatic fallback to file storage
|
||||||
|
fn handle_keyring_fallback_error<T>(
|
||||||
|
&self,
|
||||||
|
keyring_err: &keyring::Error,
|
||||||
|
fallback_values: Option<&HashMap<String, Value>>,
|
||||||
|
) -> Result<T, ConfigError> {
|
||||||
|
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<T>(
|
||||||
|
&self,
|
||||||
|
operation: impl FnOnce(keyring::Entry) -> Result<T, keyring::Error>,
|
||||||
|
service: &str,
|
||||||
|
fallback_values: Option<&HashMap<String, Value>>,
|
||||||
|
) -> Result<T, ConfigError> {
|
||||||
|
// 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");
|
config_value!(CLAUDE_CODE_COMMAND, OsString, "claude");
|
||||||
@@ -878,7 +972,6 @@ mod tests {
|
|||||||
use super::*;
|
use super::*;
|
||||||
use serial_test::serial;
|
use serial_test::serial;
|
||||||
use tempfile::NamedTempFile;
|
use tempfile::NamedTempFile;
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn test_basic_config() -> Result<(), ConfigError> {
|
fn test_basic_config() -> Result<(), ConfigError> {
|
||||||
let config = new_test_config();
|
let config = new_test_config();
|
||||||
|
|||||||
Reference in New Issue
Block a user