feat [cli]: Add keyring management for OpenAI API keys (#361)

Co-authored-by: Bradley Axen <[email protected]>

.github/workflows/ci.yaml

@@ -18,7 +18,14 @@ jobs:
 
     - name: Install Libs
       run: |
-        sudo apt install libdbus-1-dev pkg-config
+        sudo apt update -y
+        sudo apt install -y libdbus-1-dev libssl-dev gnome-keyring
+
+    - name: Start gnome-keyring
+      # run gnome-keyring with 'foobar' as password for the login keyring
+      # this will create a new login keyring and unlock it
+      # the login password doesn't matter, but the keyring must be unlocked for the tests to work
+      run: gnome-keyring-daemon --components=secrets --daemonize --unlock <<< 'foobar'
 
     - name: Install UV
       run: |

crates/goose-cli/src/profile.rs

@@ -3,7 +3,8 @@ use std::error::Error;
 use std::fs;
 use std::path::PathBuf;
 
-use crate::inputs::get_env_value_or_input;
+use crate::inputs::{get_env_value_or_input, get_user_input};
+use goose::key_manager::{get_keyring_secret, save_to_keyring, KeyRetrievalStrategy};
 use goose::providers::configs::{
     DatabricksAuth, DatabricksProviderConfig, OllamaProviderConfig, OpenAiProviderConfig,
     ProviderConfig,
@@ -91,25 +92,57 @@ pub fn find_existing_profile(name: &str) -> Option<Profile> {
     }
 }
 
+pub fn get_or_set_key(
+    human_readable_name: &str,
+    key_name: &str,
+) -> Result<String, Box<dyn std::error::Error>> {
+    // Try to get existing key first from keyring or environment
+    match get_keyring_secret(key_name, KeyRetrievalStrategy::Both) {
+        Ok(key) => return Ok(key),
+        Err(e) => {
+            eprintln!("{}", e); // Print the error
+        }
+    }
+
+    // If no key found or error occurred, prompt user for input
+    let prompt = format!("Please enter your {}:", human_readable_name);
+    let key_val = get_env_value_or_input(key_name, &prompt, false);
+
+    // Check if user wants to save to the system keyring
+    let resp = get_user_input(
+        "Would you like to save this key to the system keyring? (y/n):",
+        "y",
+    )?;
+    if resp.eq_ignore_ascii_case("y") {
+        match save_to_keyring(key_name, &key_val) {
+            Ok(_) => println!("Successfully saved key to system keyring"),
+            Err(e) => {
+                // Log the error but don't fail - the API key is still usable
+                println!("Warning: Failed to save key to system keyring: {}", e);
+            }
+        }
+    }
+
+    Ok(key_val)
+}
+
 pub fn set_provider_config(provider_name: &str, model: String) -> ProviderConfig {
     match provider_name.to_lowercase().as_str() {
-        PROVIDER_OPEN_AI => ProviderConfig::OpenAi(OpenAiProviderConfig {
-            host: "https://api.openai.com".to_string(),
-            api_key: get_env_value_or_input(
-                "OPENAI_API_KEY",
-                "Please enter your OpenAI API key:",
-                true,
-            ),
-            model,
-            temperature: None,
-            max_tokens: None,
-        }),
+        PROVIDER_OPEN_AI => {
+            let api_key = get_or_set_key("OPENAI_API_KEY", "OPENAI_API_KEY")
+                .expect("Failed to get OpenAI API key");
+            ProviderConfig::OpenAi(OpenAiProviderConfig {
+                host: "https://api.openai.com".to_string(),
+                api_key,
+                model,
+                temperature: None,
+                max_tokens: None,
+            })
+        }
         PROVIDER_DATABRICKS => {
-            let host = get_env_value_or_input(
-                "DATABRICKS_HOST",
-                "Please enter your Databricks host:",
-                false,
-            );
+            let host = get_or_set_key("databricks host url", "DATABRICKS_HOST")
+                .expect("Failed to get databricks host");
+
             ProviderConfig::Databricks(DatabricksProviderConfig {
                 host: host.clone(),
                 // TODO revisit configuration

crates/goose/Cargo.toml

@@ -40,12 +40,15 @@ dotenv = "0.15"
 xcap = "0.0.14"
 libc = "=0.2.164"
 
+keyring = { version = "3.6.1", features = ["apple-native", "windows-native", "sync-secret-service"] }
 
 [dev-dependencies]
 wiremock = "0.6.0"
 mockito = "1.2"
 tempfile = "3.8"
+mockall = "0.11"
 
 [[example]]
 name = "databricks_oauth"
 path = "examples/databricks_oauth.rs"
+

crates/goose/src/key_manager.rs

@@ -0,0 +1,153 @@
+use keyring::Entry;
+use std::env;
+use thiserror::Error;
+
+#[derive(Error, Debug)]
+pub enum KeyManagerError {
+    #[error("Failed to access keyring: {0}")]
+    KeyringAccess(String),
+
+    #[error("Failed to save to keyring: {0}")]
+    KeyringSave(String),
+
+    #[error("Failed to access environment variable: {0}")]
+    EnvVarAccess(String),
+}
+
+impl From<keyring::Error> for KeyManagerError {
+    fn from(err: keyring::Error) -> Self {
+        KeyManagerError::KeyringAccess(err.to_string())
+    }
+}
+
+impl From<env::VarError> for KeyManagerError {
+    fn from(err: env::VarError) -> Self {
+        KeyManagerError::EnvVarAccess(err.to_string())
+    }
+}
+
+#[derive(Debug, Clone, Copy)]
+pub enum KeyRetrievalStrategy {
+    /// Only look in environment variables
+    EnvironmentOnly,
+    /// Only look in system keyring
+    KeyringOnly,
+    /// Try keyring first, then environment variables (default behavior)
+    Both,
+}
+
+impl Default for KeyRetrievalStrategy {
+    fn default() -> Self {
+        Self::Both
+    }
+}
+
+pub fn get_keyring_secret(
+    key_name: &str,
+    strategy: KeyRetrievalStrategy,
+) -> Result<String, Box<dyn std::error::Error>> {
+    let kr = Entry::new("goose", key_name)?;
+    match strategy {
+        KeyRetrievalStrategy::EnvironmentOnly => env::var(key_name)
+            .map_err(|e| Box::new(KeyManagerError::from(e)) as Box<dyn std::error::Error>),
+        KeyRetrievalStrategy::KeyringOnly => kr
+            .get_password()
+            .map_err(|e| Box::new(KeyManagerError::from(e)) as Box<dyn std::error::Error>),
+        KeyRetrievalStrategy::Both => {
+            // Try environment first, then keyring
+            env::var(key_name).or_else(|_| {
+                kr.get_password().map_err(|_| {
+                    Box::new(KeyManagerError::EnvVarAccess(format!(
+                        "Could not find {} key in keyring or environment variables",
+                        key_name
+                    ))) as Box<dyn std::error::Error>
+                })
+            })
+        }
+    }
+}
+
+pub fn save_to_keyring(key_name: &str, key_val: &str) -> std::result::Result<(), KeyManagerError> {
+    let kr = Entry::new("goose", key_name)?;
+    kr.set_password(key_val).map_err(KeyManagerError::from)
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    const TEST_ENV_PREFIX: &str = "GOOSE_TEST_";
+
+    fn cleanup_env(key: &str) {
+        std::env::remove_var(key);
+    }
+
+    fn cleanup_keyring(key: &str) -> Result<(), KeyManagerError> {
+        let kr = Entry::new("goose", key)?;
+        kr.delete_credential().map_err(KeyManagerError::from)
+    }
+
+    #[test]
+    fn test_get_key_environment_only() {
+        let key_name = format!("{}{}", TEST_ENV_PREFIX, "ENV_KEY");
+        std::env::set_var(&key_name, "test_value");
+
+        let result = get_keyring_secret(&key_name, KeyRetrievalStrategy::EnvironmentOnly);
+        assert_eq!(result.unwrap(), "test_value");
+
+        cleanup_env(&key_name);
+    }
+
+    #[test]
+    fn test_get_key_environment_only_missing() {
+        let key_name = format!("{}{}", TEST_ENV_PREFIX, "MISSING_KEY");
+
+        let result = get_keyring_secret(&key_name, KeyRetrievalStrategy::EnvironmentOnly);
+        assert!(result.is_err());
+    }
+
+    #[test]
+    fn test_get_key_keyring_only() {
+        let key_name = format!("{}{}", TEST_ENV_PREFIX, "KEYRING_KEY");
+
+        // First save a value
+        save_to_keyring(&key_name, "test_value").unwrap();
+
+        let result = get_keyring_secret(&key_name, KeyRetrievalStrategy::KeyringOnly);
+        assert_eq!(result.unwrap(), "test_value");
+
+        cleanup_keyring(&key_name).unwrap();
+    }
+
+    #[test]
+    fn test_get_key_both() {
+        let key_name = format!("{}{}", TEST_ENV_PREFIX, "BOTH_KEY");
+
+        // Test environment first
+        std::env::set_var(&key_name, "env_value");
+        let result = get_keyring_secret(&key_name, KeyRetrievalStrategy::Both);
+        assert_eq!(result.unwrap(), "env_value");
+
+        // Test keyring takes precedence
+        save_to_keyring(&key_name, "keyring_value").unwrap();
+        let result = get_keyring_secret(&key_name, KeyRetrievalStrategy::Both);
+        assert_eq!(result.unwrap(), "env_value"); // Environment still takes precedence
+
+        cleanup_env(&key_name);
+        cleanup_keyring(&key_name).unwrap();
+    }
+
+    #[test]
+    fn test_save_to_keyring() {
+        let key_name = format!("{}{}", TEST_ENV_PREFIX, "SAVE_KEY");
+
+        let result = save_to_keyring(&key_name, "test_value");
+        assert!(result.is_ok());
+
+        // Verify the value was saved
+        let kr = Entry::new("goose", &key_name).unwrap();
+        assert_eq!(kr.get_password().unwrap(), "test_value");
+
+        cleanup_keyring(&key_name).unwrap();
+    }
+}

crates/goose/src/lib.rs

@@ -1,6 +1,7 @@
 pub mod agent;
 pub mod developer;
 pub mod errors;
+pub mod key_manager;
 pub mod memory;
 pub mod models;
 pub mod prompt_template;