From 07410521d71bc35002aaae443707b7e787fa773f Mon Sep 17 00:00:00 2001 From: Hunter Bown Date: Fri, 8 May 2026 17:20:07 -0500 Subject: [PATCH] fix(auth): default to file-backed secret store --- crates/cli/src/lib.rs | 34 +++++---- crates/config/src/lib.rs | 9 ++- crates/secrets/src/lib.rs | 150 +++++++++++++++++++++++++++++++++----- 3 files changed, 154 insertions(+), 39 deletions(-) diff --git a/crates/cli/src/lib.rs b/crates/cli/src/lib.rs index 24937c1a..75dce61a 100644 --- a/crates/cli/src/lib.rs +++ b/crates/cli/src/lib.rs @@ -256,7 +256,7 @@ enum AuthCommand { #[arg(long, value_enum)] provider: ProviderArg, }, - /// Delete a provider's key from config and keyring storage. + /// Delete a provider's key from config and secret-store storage. Clear { #[arg(long, value_enum)] provider: ProviderArg, @@ -556,14 +556,14 @@ fn resolve_runtime_for_dispatch_with_secrets( match store.save() { Ok(()) => { eprintln!( - "info: recovered API key from OS keyring and saved it to {}", + "info: recovered API key from secret store and saved it to {}", store.path().display() ); resolved.api_key_source = Some(RuntimeApiKeySource::ConfigFile); } Err(err) => { eprintln!( - "warning: recovered API key from OS keyring but failed to save {}: {err}", + "warning: recovered API key from secret store but failed to save {}: {err}", store.path().display() ); } @@ -806,7 +806,7 @@ fn auth_status_lines(store: &ConfigStore, secrets: &Secrets) -> Vec { let active_source = if config_key.is_some() { "config" } else if keyring_key.is_some() { - "keyring" + "secret store" } else if env_key.is_some() { "env" } else { @@ -832,14 +832,14 @@ fn auth_status_lines(store: &ConfigStore, secrets: &Secrets) -> Vec { vec![ format!("provider: {}", provider.as_str()), format!("active source: {active_label}"), - "lookup order: config -> keyring -> env".to_string(), + "lookup order: config -> secret store -> env".to_string(), format!( "config file: {} ({})", store.path().display(), source_status(config_key, "missing") ), format!( - "keyring: {} ({})", + "secret store: {} ({})", secrets.backend_name(), source_status(keyring_key.as_deref(), "missing") ), @@ -929,7 +929,7 @@ fn run_auth_command_with_secrets( let source = if in_file { Some("config-file") } else if in_keyring { - Some("keyring") + Some("secret-store") } else if in_env { Some("env") } else { @@ -947,11 +947,11 @@ fn run_auth_command_with_secrets( clear_provider_api_key_from_config(store, provider); clear_provider_api_key_from_keyring(secrets, provider); store.save()?; - println!("cleared API key for {slot} from config and keyring"); + println!("cleared API key for {slot} from config and secret store"); Ok(()) } AuthCommand::List => { - println!("provider config keyring env active"); + println!("provider config store env active"); let active_provider = store.config.provider; for provider in PROVIDER_LIST { let slot = provider_slot(provider); @@ -962,7 +962,7 @@ fn run_auth_command_with_secrets( let active = if file { "config" } else if keyring == Some(true) { - "keyring" + "store" } else if env { "env" } else { @@ -1012,8 +1012,8 @@ fn prompt_api_key(slot: &str) -> Result { Ok(key) } -/// Move plaintext keys from config.toml into an explicit platform credential -/// store. Hidden in v0.8.8 because the normal setup path is config/env only. +/// Move plaintext keys from config.toml into the configured secret store. +/// Hidden in v0.8.8 because the normal setup path is config/env only. fn run_auth_migrate(store: &mut ConfigStore, secrets: &Secrets, dry_run: bool) -> Result<()> { let mut migrated: Vec<(ProviderKind, &'static str)> = Vec::new(); let mut warnings: Vec = Vec::new(); @@ -1042,7 +1042,9 @@ fn run_auth_migrate(store: &mut ConfigStore, secrets: &Secrets, dry_run: bool) - migrated.push((provider, slot)); continue; } else if let Err(err) = secrets.set(slot, &value) { - warnings.push(format!("skipped {slot}: failed to write to keyring: {err}")); + warnings.push(format!( + "skipped {slot}: failed to write to secret store: {err}" + )); continue; } if !dry_run { @@ -1060,7 +1062,7 @@ fn run_auth_migrate(store: &mut ConfigStore, secrets: &Secrets, dry_run: bool) - .context("failed to write updated config.toml")?; } - println!("keyring backend: {}", secrets.backend_name()); + println!("secret store backend: {}", secrets.backend_name()); if migrated.is_empty() { println!("nothing to migrate (config.toml has no plaintext api_key entries)"); } else { @@ -2273,10 +2275,10 @@ mod tests { assert!(output.contains("provider: deepseek")); assert!(output.contains("active source: config (last4: ...3333)")); - assert!(output.contains("lookup order: config -> keyring -> env")); + assert!(output.contains("lookup order: config -> secret store -> env")); assert!(output.contains("config file: ")); assert!(output.contains("set, last4: ...3333")); - assert!(output.contains("keyring: in-memory (test) (set, last4: ...2222)")); + assert!(output.contains("secret store: in-memory (test) (set, last4: ...2222)")); assert!(output.contains("env var: DEEPSEEK_API_KEY (set, last4: ...1111)")); assert!(!output.contains("sk-config-3333")); assert!(!output.contains("sk-keyring-2222")); diff --git a/crates/config/src/lib.rs b/crates/config/src/lib.rs index ee744a4d..4cc2cfba 100644 --- a/crates/config/src/lib.rs +++ b/crates/config/src/lib.rs @@ -835,7 +835,8 @@ impl ConfigToml { /// /// This method keeps library callers prompt-free: CLI flag → config file /// → environment. Call `resolve_runtime_options_with_secrets` when a - /// user-facing dispatcher should recover OS-keyring credentials. + /// user-facing dispatcher should recover credentials from the configured + /// secret store. #[must_use] pub fn resolve_runtime_options(&self, cli: &CliRuntimeOverrides) -> ResolvedRuntimeOptions { let no_keyring = Secrets::new(std::sync::Arc::new( @@ -846,7 +847,7 @@ impl ConfigToml { /// Resolve runtime options using an explicit secrets façade. /// - /// API-key precedence is **CLI flag → config-file → keyring → environment**. + /// API-key precedence is **CLI flag → config-file → secret store → environment**. #[must_use] pub fn resolve_runtime_options_with_secrets( &self, @@ -869,8 +870,8 @@ impl ConfigToml { // CLI flag wins outright. Otherwise: config-file → injected secrets/env. // This makes `deepseek auth set` a reliable fix even when the user's // shell still exports an old key. When the file is empty, the injected - // secrets façade recovers older OS-keyring credentials before falling - // back to ambient env. + // secrets façade recovers configured secret-store credentials before + // falling back to ambient env. let from_file = provider_cfg.api_key.clone().or(root_deepseek_api_key); let (api_key, api_key_source) = if let Some(value) = cli.api_key.clone() { (Some(value), Some(RuntimeApiKeySource::Cli)) diff --git a/crates/secrets/src/lib.rs b/crates/secrets/src/lib.rs index c9148c46..38990dbc 100644 --- a/crates/secrets/src/lib.rs +++ b/crates/secrets/src/lib.rs @@ -1,14 +1,13 @@ //! Secret storage for DeepSeek API keys. //! //! Provides a small abstraction (`KeyringStore`) plus a default -//! implementation backed by the OS keyring (`DefaultKeyringStore`), -//! a file-based fallback for headless or unsupported platforms -//! (`FileKeyringStore`), and an in-memory store for tests +//! file-based implementation (`FileKeyringStore`), an opt-in OS keyring +//! implementation (`DefaultKeyringStore`), and an in-memory store for tests //! (`InMemoryKeyringStore`). //! -//! Higher-level lookup through [`Secrets::resolve`] checks the keyring first +//! Higher-level lookup through [`Secrets::resolve`] checks the secret store first //! and falls back to environment variables. Config-file precedence lives in the -//! config crate so user-facing commands can keep `config -> keyring -> env` +//! config crate so user-facing commands can keep `config -> secret store -> env` //! explicit at the call site. #![deny(missing_docs)] @@ -23,6 +22,9 @@ use thiserror::Error; /// Default OS keychain service name. macOS users can verify entries with /// `security find-generic-password -s deepseek -a `. pub const DEFAULT_SERVICE: &str = "deepseek"; +/// Select the secret storage backend. Supported values are `file` (default) +/// and `system`/`keyring` for the OS credential store. +pub const SECRET_BACKEND_ENV: &str = "DEEPSEEK_SECRET_BACKEND"; /// Errors that may arise from a [`KeyringStore`] backend. #[derive(Debug, Error)] @@ -61,9 +63,10 @@ pub trait KeyringStore: Send + Sync { } /// OS keyring backend (macOS Keychain, Windows Credential Manager, -/// Linux Secret Service / kwallet). On platforms without a configured -/// native keyring dependency, probing this backend returns an unsupported -/// error so [`Secrets::auto_detect`] can fall back to [`FileKeyringStore`]. +/// Linux Secret Service / kwallet). This backend is opt-in through +/// [`SECRET_BACKEND_ENV`]. On platforms without a configured native +/// keyring dependency, probing this backend returns an unsupported error so +/// [`Secrets::auto_detect`] can fall back to [`FileKeyringStore`]. #[derive(Debug, Clone)] pub struct DefaultKeyringStore { /// Keyring service name (defaults to [`DEFAULT_SERVICE`]). @@ -349,17 +352,35 @@ impl KeyringStore for FileKeyringStore { } } +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum SecretBackendSelection { + File, + System, + Unknown, +} + +fn secret_backend_selection(value: Option<&str>) -> SecretBackendSelection { + match value.map(str::trim).filter(|value| !value.is_empty()) { + None => SecretBackendSelection::File, + Some(value) => match value.to_ascii_lowercase().as_str() { + "file" | "local" | "json" => SecretBackendSelection::File, + "system" | "keyring" | "os" | "os-keyring" => SecretBackendSelection::System, + _ => SecretBackendSelection::Unknown, + }, + } +} + /// High-level façade combining a [`KeyringStore`] with environment /// variable fallbacks. /// -/// Lookup precedence: **keyring → env → none**. Callers that also have +/// Lookup precedence: **secret store → env → none**. Callers that also have /// a TOML config layer must wire that themselves at the very end of /// the chain. #[derive(Clone)] pub struct Secrets { /// Underlying secret store. pub store: Arc, - /// Owner identifier within the keyring (typically "deepseek"); the + /// Owner identifier within the secret store (typically "deepseek"); the /// `key` parameter passed to `resolve` is mapped to a slot in the /// store as-is, while envs are looked up by canonical name. service: String, @@ -368,7 +389,7 @@ pub struct Secrets { /// Source layer that provided a resolved secret. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum SecretSource { - /// The configured keyring backend returned the secret. + /// The configured secret-store backend returned the secret. Keyring, /// A process environment variable returned the secret. Env, @@ -393,11 +414,50 @@ impl Secrets { } } - /// Construct the platform-appropriate default backend. On platforms - /// where an OS keyring backend is reachable this returns - /// [`DefaultKeyringStore`]; otherwise it falls back to - /// [`FileKeyringStore`] under `~/.deepseek/secrets/`. + /// Construct the default backend. The prompt-free default is + /// [`FileKeyringStore`] under `~/.deepseek/secrets/`. Set + /// [`SECRET_BACKEND_ENV`] to `system` or `keyring` to opt into the OS + /// credential store. pub fn auto_detect() -> Self { + match secret_backend_selection(std::env::var(SECRET_BACKEND_ENV).ok().as_deref()) { + SecretBackendSelection::File => Self::file_backed_default(), + SecretBackendSelection::Unknown => { + tracing::warn!( + "{SECRET_BACKEND_ENV} has an unsupported value; using file-backed secret store" + ); + Self::file_backed_default() + } + SecretBackendSelection::System => { + let default_store = DefaultKeyringStore::default(); + match default_store.probe() { + Ok(()) => Self::new(Arc::new(default_store)), + Err(err) => { + tracing::warn!( + "OS keyring unavailable ({err}); falling back to file-backed secret store" + ); + Self::file_backed_default() + } + } + } + } + } + + fn file_backed_default() -> Self { + let path = FileKeyringStore::default_path() + .unwrap_or_else(|_| PathBuf::from(".deepseek-secrets.json")); + Self::new(Arc::new(FileKeyringStore::new(path))) + } + + /// Construct the file-backed default backend directly. + #[must_use] + pub fn file_backed() -> Self { + Self::file_backed_default() + } + + /// Construct the opt-in OS credential backend, falling back to the + /// file-backed store when the platform backend is unavailable. + #[must_use] + pub fn system_keyring() -> Self { let default_store = DefaultKeyringStore::default(); match default_store.probe() { Ok(()) => Self::new(Arc::new(default_store)), @@ -405,9 +465,7 @@ impl Secrets { tracing::warn!( "OS keyring unavailable ({err}); falling back to file-backed secret store" ); - let path = FileKeyringStore::default_path() - .unwrap_or_else(|_| PathBuf::from(".deepseek-secrets.json")); - Self::new(Arc::new(FileKeyringStore::new(path))) + Self::file_backed_default() } } } @@ -418,7 +476,7 @@ impl Secrets { self.store.backend_name() } - /// Resolve a secret with `keyring → env → none` precedence. + /// Resolve a secret with `secret store → env → none` precedence. /// /// `name` is the canonical provider name (`"deepseek"`, /// `"openrouter"`, `"novita"`, `"nvidia"`/`"nvidia-nim"`, `"openai"`). @@ -512,6 +570,7 @@ mod tests { "VLLM_API_KEY", "OLLAMA_API_KEY", "OPENAI_API_KEY", + SECRET_BACKEND_ENV, ] { // Safety: tests serialise on env_lock(); the broader // workspace has the same pattern in `crates/config`. @@ -519,6 +578,59 @@ mod tests { } } + #[test] + fn backend_selection_defaults_to_file() { + assert_eq!(secret_backend_selection(None), SecretBackendSelection::File); + assert_eq!( + secret_backend_selection(Some("")), + SecretBackendSelection::File + ); + assert_eq!( + secret_backend_selection(Some(" file ")), + SecretBackendSelection::File + ); + } + + #[test] + fn backend_selection_accepts_explicit_system_keyring() { + assert_eq!( + secret_backend_selection(Some("system")), + SecretBackendSelection::System + ); + assert_eq!( + secret_backend_selection(Some("keyring")), + SecretBackendSelection::System + ); + assert_eq!( + secret_backend_selection(Some("os-keyring")), + SecretBackendSelection::System + ); + } + + #[test] + fn auto_detect_is_file_backed_by_default() { + let _lock = env_lock(); + clear_known_envs(); + + let secrets = Secrets::auto_detect(); + + assert_eq!(secrets.backend_name(), "file-based (~/.deepseek/secrets/)"); + } + + #[test] + fn auto_detect_honors_explicit_file_backend() { + let _lock = env_lock(); + clear_known_envs(); + // Safety: env mutation guarded by env_lock(). + unsafe { std::env::set_var(SECRET_BACKEND_ENV, "local") }; + + let secrets = Secrets::auto_detect(); + + assert_eq!(secrets.backend_name(), "file-based (~/.deepseek/secrets/)"); + // Safety: env mutation guarded by env_lock(). + unsafe { std::env::remove_var(SECRET_BACKEND_ENV) }; + } + #[test] fn in_memory_store_round_trips() { let store = InMemoryKeyringStore::new();