From d474a40580e03e0a319e51b95bdc659066013754 Mon Sep 17 00:00:00 2001 From: Svetlin Ralchev Date: Tue, 6 Oct 2026 11:09:59 +0400 Subject: [PATCH 1/2] feat: spinners, summaries and colors; keep secrets off the terminal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Output now uses console and indicatif, all on stderr: - a spinner while a secret is fetched from its provider - one-line summaries, e.g. "✓ Loaded 5 secrets from work (3 from the keychain, 2 from 1password)" and "✓ Added 2 SSH keys to ssh-agent (expire in 8h)" - colored errors, warnings and debug lines - colored marks in `doctor`, and states and headings in `status` and `profile show` Nothing is animated or colored when the output isn't a terminal, with -q, or with NO_COLOR. On a terminal without the shell integration, `load` and `unload` now handle SSH keys as usual and skip only the variables, which they can't set and won't print: they fail with a hint instead of refusing everything. --- Cargo.lock | 61 ++++++++++ Cargo.toml | 2 + README.md | 2 +- src/app/args.rs | 72 ++++++----- src/app/exec.rs | 287 ++++++++++++++++++++++++++++++++++++-------- src/log.rs | 119 +++++++++++++++++- src/main.rs | 18 ++- src/vault/loader.rs | 32 ++++- 8 files changed, 487 insertions(+), 106 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index cfecf33..ed86fe9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -427,6 +427,18 @@ dependencies = [ "crossbeam-utils", ] +[[package]] +name = "console" +version = "0.16.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e96a4956774c13c126a8b5af4daa79384f4d826534c95a02d76afb39e2ab64e3" +dependencies = [ + "encode_unicode", + "libc", + "unicode-width", + "windows-sys", +] + [[package]] name = "const-oid" version = "0.9.6" @@ -651,6 +663,12 @@ dependencies = [ "zeroize", ] +[[package]] +name = "encode_unicode" +version = "1.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "34aa73646ffb006b8f5147f3dc182bd4bcb190227ce861fc4a4844bf8e3cb2c0" + [[package]] name = "endi" version = "1.1.1" @@ -923,6 +941,19 @@ dependencies = [ "hashbrown", ] +[[package]] +name = "indicatif" +version = "0.18.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9433806cd6b4ec1aba79c021c7e4c58fb4c3b9977c085062e611ac929998fb0c" +dependencies = [ + "console", + "portable-atomic", + "unicode-width", + "unit-prefix", + "web-time", +] + [[package]] name = "indoc" version = "2.0.7" @@ -992,6 +1023,8 @@ dependencies = [ "assert_cmd", "clap", "clap_complete", + "console", + "indicatif", "indoc", "keyring-core", "libc", @@ -1301,6 +1334,12 @@ dependencies = [ "windows-sys", ] +[[package]] +name = "portable-atomic" +version = "1.15.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "05c8b63e8d9609db387f0324918f81d68fe27748f084ef092fb35954d0539a85" + [[package]] name = "ppv-lite86" version = "0.2.21" @@ -1883,6 +1922,18 @@ version = "1.0.26" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "d245f478577f809a851594d02313b640fb437e0bb33866753cff937863096954" +[[package]] +name = "unicode-width" +version = "0.2.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b4ac048d71ede7ee76d585517add45da530660ef4390e49b098733c6e897f254" + +[[package]] +name = "unit-prefix" +version = "0.5.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "81e544489bf3d8ef66c953931f56617f423cd4b5494be343d9b9d3dda037b9a3" + [[package]] name = "unsafe-libyaml" version = "0.2.11" @@ -1972,6 +2023,16 @@ dependencies = [ "unicode-ident", ] +[[package]] +name = "web-time" +version = "1.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5a6580f308b1fad9207618087a65c04e7a10bc77e02c8e84e9b00dd4b12fa0bb" +dependencies = [ + "js-sys", + "wasm-bindgen", +] + [[package]] name = "windows-link" version = "0.2.1" diff --git a/Cargo.toml b/Cargo.toml index 040403c..01d5cb9 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -18,6 +18,8 @@ pkg-fmt = "bin" anyhow = "1.0.104" clap = { version = "4.6.7", features = ["derive", "env", "string"] } clap_complete = "4.6.11" +console = "0.16" +indicatif = "0.18" keyring-core = "1.0.0" libc = "0.2.190" serde = { version = "1.0.229", features = ["derive"] } diff --git a/README.md b/README.md index 13e78d8..ff9dd60 100644 --- a/README.md +++ b/README.md @@ -177,7 +177,7 @@ keysafe exec -p work -- terraform plan # run one command with the secrets; file keysafe export -p work --format json # the secrets as a JSON object ``` -Without the shell integration, for example in scripts, `load` and `export` print `export` statements to evaluate yourself, since a program can't change the environment of the shell that started it: +Without the shell integration, `load` and `export` print `export` statements to evaluate yourself, since a program can't change the environment of the shell that started it. On a terminal, `load` and `unload` never print them, so secrets don't end up on screen: they still add or remove SSH keys, and say how to set up the integration for the variables. ```bash eval "$(keysafe export -p work)" diff --git a/src/app/args.rs b/src/app/args.rs index e91a638..d93bda5 100644 --- a/src/app/args.rs +++ b/src/app/args.rs @@ -432,7 +432,7 @@ impl Display for ExportFormat { } /// OutputArgs holds the flags shared by subcommands that emit secrets. -#[derive(Debug, Default, Args)] +#[derive(Debug, Clone, Default, Args)] pub struct OutputArgs { /// Output format for the exported secrets. /// Auto-detected from $SHELL if not provided. @@ -456,6 +456,10 @@ pub struct OutputArgs { /// Set by the script that `init` prints; not meant to be set by hand. #[arg(long, env = "KEYSAFE_EVAL", hide = true)] pub eval: Option, + + /// Whether stdout is a terminal. Set by `main`, not a command-line flag. + #[arg(skip)] + pub terminal: bool, } impl OutputArgs { @@ -465,20 +469,22 @@ impl OutputArgs { self.eval.is_some() && self.format.is_none() } - /// Fails when the shell statements of `command` would only be printed on a terminal: - /// without the shell integration and without an explicit --format, they would show - /// secret values on screen without changing the shell. - pub fn check_destination(&self, command: &str, stdout_is_terminal: bool) -> anyhow::Result<()> { - if stdout_is_terminal && self.eval.is_none() && self.format.is_none() { - let (shell, rc) = match self.export_format() { - ExportFormat::Bash => ("bash", "~/.bashrc"), - _ => ("zsh", "~/.zshrc"), - }; - anyhow::bail!( - "`{command}` changes your shell through the shell integration, which isn't active here; add `eval \"$(keysafe init {shell})\"` to {rc}, or run `eval \"$(keysafe {command})\"`" - ); - } - Ok(()) + /// Returns true when shell statements can't reach the shell: stdout is a terminal, the + /// shell integration isn't active and no format was requested, so printing them would + /// only show secret values on screen. + pub fn statements_stranded(&self) -> bool { + self.terminal && self.eval.is_none() && self.format.is_none() + } + + /// Explains how to make `command` change the shell. + pub fn integration_hint(&self, command: &str) -> String { + let (shell, rc) = match self.export_format() { + ExportFormat::Bash => ("bash", "~/.bashrc"), + _ => ("zsh", "~/.zshrc"), + }; + format!( + "add `eval \"$(keysafe init {shell})\"` to {rc}, or run `eval \"$(keysafe {command})\"`" + ) } /// Get the export format: the explicit one, the integration's shell, or $SHELL detection. @@ -870,33 +876,25 @@ mod tests { } #[test] - fn statements_are_not_printed_on_a_terminal_without_the_integration() { - let plain = OutputArgs { - format: None, - eval: None, + fn statements_are_stranded_on_a_terminal_without_the_integration() { + let terminal = OutputArgs { + terminal: true, ..Default::default() }; - let err = plain - .check_destination("load", true) - .unwrap_err() - .to_string(); - assert!( - err.starts_with("`load` changes your shell through the shell integration"), - "{err}" - ); + assert!(terminal.statements_stranded()); - // Piped or evaluated, under the integration, or with an explicit format: printed - assert!(plain.check_destination("load", false).is_ok()); - let integrated = OutputArgs { + // Piped or evaluated, under the integration, or with an explicit format: they arrive + assert!(!OutputArgs::default().statements_stranded()); + assert!(!OutputArgs { eval: Some(Shell::Zsh), - ..Default::default() - }; - assert!(integrated.check_destination("load", true).is_ok()); - let explicit = OutputArgs { + ..terminal.clone() + } + .statements_stranded()); + assert!(!OutputArgs { format: Some(ExportFormat::Zsh), - ..Default::default() - }; - assert!(explicit.check_destination("unload", true).is_ok()); + ..terminal + } + .statements_stranded()); } #[test] diff --git a/src/app/exec.rs b/src/app/exec.rs index 6c19c7c..f1c23ba 100644 --- a/src/app/exec.rs +++ b/src/app/exec.rs @@ -1,5 +1,5 @@ use crate::app::args::*; -use crate::log::{info, warn}; +use crate::log::{count, info, styled, success, warn}; use crate::vault::*; use anyhow::{bail, Context, Result}; use clap::{builder::PossibleValuesParser, CommandFactory}; @@ -114,18 +114,41 @@ impl LoadCommand { }; let runtime = RuntimeDir::new(args.output.runtime_dir.clone()); - // Export the environment and file secrets - let variables = secrets.iter().copied().filter(|s| s.is_variable()); - let (variables, mut failed) = - self.loader - .resolve(&runtime, account, variables, Source::Any(args.refresh)); + // Export the environment and file secrets, unless the statements would only be shown + // on a terminal: then the variables can't be set, and secrets must not be printed + let selected: Vec<&Secret> = secrets + .iter() + .copied() + .filter(|s| s.is_variable()) + .collect(); + let stranded = args.output.statements_stranded(); + let (variables, mut failed) = if stranded { + (Vec::new(), 0) + } else { + self.loader.resolve( + &runtime, + account, + selected.iter().copied(), + Source::Any(args.refresh), + ) + }; let format = args.output.export_format(); write_runtime_dir(&mut self.writer, format, &runtime, args.output.is_eval())?; write_variables(&mut self.writer, format, &variables)?; if !variables.is_empty() { - info(format!( - "loaded {} environment and file secret(s)", - variables.len() + let (cached, fetched) = self.loader.counts(); + let source = match (cached, fetched) { + (0, _) => format!("from {}", account.provider), + (_, 0) => "from the keychain".to_string(), + _ => format!( + "{cached} from the keychain, {fetched} from {}", + account.provider + ), + }; + success(format!( + "Loaded {} from {} ({source})", + count(variables.len(), "secret"), + account.name )); } @@ -137,6 +160,15 @@ impl LoadCommand { .collect(); failed += self.add_keys(account, &keys, args); + // SSH keys don't need the shell; variables do + if stranded && !selected.is_empty() { + bail!( + "{} not set: the shell integration isn't active here; {}", + count(selected.len(), "variable"), + args.output.integration_hint("load") + ); + } + // Record a fully loaded profile, so its cached secrets are exported in new shells if args.names.is_empty() { self.loader.cache.save(account)?; @@ -191,14 +223,18 @@ impl LoadCommand { } if added > 0 { - info(format!( - "added {added} SSH key(s) with {} expiration", - args.expiration + let expiry = lifetime + .map(duration) + .unwrap_or_else(|| args.expiration.clone()); + success(format!( + "Added {} to ssh-agent (expire in {expiry})", + count(added, "SSH key") )); } if kept > 0 { info(format!( - "{kept} SSH key(s) already in the agent (use --refresh to reset their expiration)" + "{} already in ssh-agent (use --refresh to reset the expiration)", + count(kept, "SSH key") )); } @@ -241,17 +277,21 @@ impl UnloadCommand { bail!("unload prints shell statements; use --format zsh or --format bash"); } - // Unset the variables, deleting the files of file secrets keysafe wrote + // Unset the variables, deleting the files of file secrets keysafe wrote. Without a way + // to reach the shell, the variables stay, and so do the files they point to. let variables: Vec<&Secret> = secrets .iter() .copied() .filter(|s| s.is_variable()) .collect(); - for secret in &variables { - if secret.kind == SecretKind::File { - self.remove_file(profile, secret); + let stranded = args.output.statements_stranded(); + if !stranded { + for secret in &variables { + if secret.kind == SecretKind::File { + self.remove_file(profile, secret); + } + writeln!(self.writer, "unset {}", secret.name)?; } - writeln!(self.writer, "unset {}", secret.name)?; } // Remove the SSH keys keysafe added from the agent @@ -262,16 +302,32 @@ impl UnloadCommand { .collect(); let (removed, failed) = self.remove_keys(profile, &keys); + // SSH keys don't need the shell; variables do + if stranded && !variables.is_empty() { + if removed > 0 { + success(format!( + "Removed {} from ssh-agent", + count(removed, "SSH key") + )); + } + bail!( + "{} not unset: the shell integration isn't active here; {}", + count(variables.len(), "variable"), + args.output.integration_hint("unload") + ); + } + // An unloaded profile is no longer exported in new shells; its cache stays if args.names.is_empty() { self.cache.forget(&profile.name)?; } - info(format!( - "unloaded {} variable(s) and {removed} SSH key(s) of profile '{}'", - variables.len(), - profile.name - )); + let unloaded = match (variables.len(), removed) { + (n, 0) => count(n, "secret"), + (0, k) => count(k, "SSH key"), + (n, k) => format!("{} and {}", count(n, "secret"), count(k, "SSH key")), + }; + success(format!("Unloaded {unloaded} from {}", profile.name)); finish(failed) } @@ -638,10 +694,15 @@ impl StatusCommand { }; match args.shell { - Some(shell) => writeln!(self.writer, "Shell integration: active ({shell})")?, + Some(shell) => writeln!( + self.writer, + "Shell integration: {}", + styled(format!("active ({shell})")).green() + )?, None => writeln!( self.writer, - "Shell integration: not active (add `eval \"$(keysafe init zsh)\"` to ~/.zshrc, or `init bash` to ~/.bashrc)" + "Shell integration: {} (add `eval \"$(keysafe init zsh)\"` to ~/.zshrc, or `init bash` to ~/.bashrc)", + styled("not active").yellow() )?, } writeln!(self.writer, "Config: {}", args.parent.config.display())?; @@ -664,7 +725,12 @@ impl StatusCommand { } else { "" }; - writeln!(self.writer, "Profile: {}{default}", profile.name)?; + writeln!( + self.writer, + "{} {}{default}", + styled("Profile:").bold(), + styled(&profile.name).bold() + )?; let exported = self.cache.loaded(&profile.name)?.is_some(); writeln!( self.writer, @@ -685,11 +751,13 @@ impl StatusCommand { for secret in variables { let state = match self.environment.get(&secret.name) { Some(value) if !value.is_empty() => match secret.kind { - SecretKind::File if Path::new(value).exists() => "set (file)", - SecretKind::File => "set, but the file is missing", - _ => "set", + SecretKind::File if Path::new(value).exists() => { + styled("set (file)").green() + } + SecretKind::File => styled("set, but the file is missing").yellow(), + _ => styled("set").green(), }, - _ => "not set", + _ => styled("not set").dim(), }; writeln!(self.writer, " {: "SSH agent not running".to_string(), + None => styled("SSH agent not running".to_string()).yellow(), Some(present) => records .iter() .filter(|r| r.profile == profile.name && r.name == key.name) .find(|r| present.contains(&r.fingerprint)) .map(|r| match r.expires.checked_sub(self.now) { Some(left) if left > 0 => { - format!("in agent, expires in {}", duration(left)) + styled(format!("in agent, expires in {}", duration(left))) + .green() } - _ => "in agent".to_string(), + _ => styled("in agent".to_string()).green(), }) - .unwrap_or_else(|| "not in agent".to_string()), + .unwrap_or_else(|| styled("not in agent".to_string()).dim()), }; writeln!(self.writer, " {: Result<()> { - writeln!(self.writer, "{} {label}: {detail}", check.mark())?; + let mark = styled(check.mark()); + let mark = match check { + Check::Ok => mark.green(), + Check::Warning => mark.yellow(), + Check::Failure => mark.red(), + }; + writeln!( + self.writer, + "{} {}: {detail}", + mark.bold(), + styled(label).bold() + )?; Ok(()) } @@ -1023,7 +1101,12 @@ impl ProfileShowCommand { } else { "" }; - writeln!(self.writer, "Profile: {}{default}", account.name)?; + writeln!( + self.writer, + "{} {}{default}", + styled("Profile:").bold(), + styled(&account.name).bold() + )?; match &account.provider { Provider::OnePassword { account: Some(name), @@ -1064,9 +1147,10 @@ impl ProfileClearCommand { let config = Config::read_from_file(&args.parent.config)?; let account = config.profile(&args.profile)?; - let count = self.cache.clear(account)?; - info(format!( - "cleared {count} cached secret(s) for profile '{}'", + let cleared = self.cache.clear(account)?; + success(format!( + "Cleared {} of {}", + count(cleared, "cached secret"), account.name )); Ok(()) @@ -1175,7 +1259,7 @@ mod tests { OutputArgs { format: Some(format), runtime_dir: Some(self.runtime_dir()), - eval: None, + ..Default::default() } } @@ -1192,10 +1276,7 @@ mod tests { } fn loader(&self, client: MockSecretClient) -> Loader { - Loader { - client: Box::new(client), - cache: self.cache(), - } + Loader::new(Box::new(client), self.cache()) } fn value(&self, profile: &str, name: &str) -> Option { @@ -2515,6 +2596,110 @@ mod tests { Ok(()) } + /// Output settings for a terminal without the shell integration. + fn terminal_output() -> OutputArgs { + OutputArgs { + terminal: true, + ..Default::default() + } + } + + #[test] + fn load_on_a_terminal_adds_keys_but_never_prints_secrets() -> Result<()> { + let fixture = Fixture::new() + .cached("personal", "GITHUB_TOKEN", "brown-fox") + .cached("personal", "my-key", &TEST_KEY); + let mut agent = agent(vec![]); + agent.expect_add().times(1).returning(|_, _| Ok(())); + let writer = Writer::new(); + let mut cmd = LoadCommand { + writer: Box::new(writer.clone()), + loader: fixture.loader(offline()), + agent: Box::new(agent), + }; + + let result = cmd.execute(&LoadCommandArgs { + output: terminal_output(), + ..load_args(&fixture, &["GITHUB_TOKEN", "my-key"], false) + }); + + let err = result.unwrap_err().to_string(); + assert!( + err.starts_with( + "1 variable not set: the shell integration isn't active here; add `eval" + ), + "{err}" + ); + assert_eq!(writer.contents(), ""); + // Only the SSH key was read; the variable wasn't even fetched + assert_eq!(cmd.loader.counts(), (1, 0)); + Ok(()) + } + + #[test] + fn load_on_a_terminal_works_for_ssh_keys_alone() -> Result<()> { + let fixture = Fixture::new().cached("personal", "my-key", &TEST_KEY); + let mut agent = agent(vec![]); + agent.expect_add().times(1).returning(|_, _| Ok(())); + let mut cmd = LoadCommand { + writer: Box::new(Writer::new()), + loader: fixture.loader(offline()), + agent: Box::new(agent), + }; + + cmd.execute(&LoadCommandArgs { + output: terminal_output(), + ..load_args(&fixture, &["my-key"], false) + }) + } + + #[test] + fn unload_on_a_terminal_removes_keys_but_keeps_variables() -> Result<()> { + let fixture = Fixture::new().loaded("personal", "env:GITHUB_TOKEN\n"); + let file = RuntimeDir::new(Some(fixture.runtime_dir())).write( + "personal", + "GCP_CREDENTIALS", + "{}", + )?; + let key = fingerprint(&TEST_KEY)?; + fixture.cache().record_keys( + &[KeyRecord { + profile: "personal".into(), + name: "my-key".into(), + fingerprint: key.clone(), + public_key: public_key(&TEST_KEY)?, + expires: u64::MAX, + }], + 0, + )?; + let mut agent = agent(vec![key]); + agent.expect_remove().times(1).returning(|_| Ok(())); + let writer = Writer::new(); + let mut cmd = UnloadCommand { + writer: Box::new(writer.clone()), + ..unload( + &fixture, + agent, + &[("GCP_CREDENTIALS", &file.to_string_lossy())], + ) + }; + + let result = cmd.execute(&UnloadCommandArgs { + output: terminal_output(), + ..unload_args(&fixture, &[]) + }); + + assert!(result + .unwrap_err() + .to_string() + .starts_with("2 variables not unset: the shell integration isn't active here")); + assert_eq!(writer.contents(), ""); + assert!(file.exists()); + assert!(fixture.cache().loaded("personal")?.is_some()); + assert_eq!(fixture.cache().keys()?, vec![]); + Ok(()) + } + #[test] fn profile_clear_deletes_cached_secrets() -> Result<()> { let fixture = Fixture::new() diff --git a/src/log.rs b/src/log.rs index d9da9ef..b04f397 100644 --- a/src/log.rs +++ b/src/log.rs @@ -1,5 +1,9 @@ +use console::{style, StyledObject, Term}; +use indicatif::{ProgressBar, ProgressStyle}; use std::fmt::Display; -use std::sync::atomic::{AtomicU8, Ordering}; +use std::sync::atomic::{AtomicBool, AtomicU8, Ordering}; +use std::sync::Mutex; +use std::time::Duration; /// How much keysafe prints to stderr. #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)] @@ -14,31 +18,138 @@ pub enum Level { static LEVEL: AtomicU8 = AtomicU8::new(Level::Normal as u8); +/// Whether output on stdout is styled. Off unless `main` turns it on for a terminal, so +/// output written elsewhere, like in tests, stays plain. +static STYLE_STDOUT: AtomicBool = AtomicBool::new(false); + +/// The spinner currently shown on stderr, if any. Messages are printed above it. +static SPINNER: Mutex> = Mutex::new(None); + /// Sets how much keysafe prints to stderr. pub fn set_level(level: Level) { LEVEL.store(level as u8, Ordering::Relaxed); } +/// Sets whether output on stdout is styled. +pub fn set_style_stdout(enabled: bool) { + STYLE_STDOUT.store(enabled, Ordering::Relaxed); +} + /// Returns true if messages of `level` are printed. fn enabled(level: Level) -> bool { LEVEL.load(Ordering::Relaxed) >= level as u8 } +/// Returns `value` styled for stdout, when stdout styling is on. +pub fn styled(value: D) -> StyledObject { + style(value).force_styling(STYLE_STDOUT.load(Ordering::Relaxed)) +} + +/// Prints a line to stderr, above the spinner if one is shown. +fn print(line: String) { + match SPINNER.lock().unwrap_or_else(|e| e.into_inner()).as_ref() { + Some(spinner) => spinner.println(line), + None => eprintln!("{line}"), + } +} + +/// Prints an error to stderr. +pub fn error(message: impl Display) { + print(format!( + "keysafe: {} {message}", + style("error:").red().bold().for_stderr() + )); +} + /// Prints a warning to stderr. pub fn warn(message: impl Display) { - eprintln!("keysafe: warning: {message}"); + print(format!( + "keysafe: {} {message}", + style("warning:").yellow().bold().for_stderr() + )); } /// Prints an informational message to stderr, unless quiet. pub fn info(message: impl Display) { if enabled(Level::Normal) { - eprintln!("keysafe: {message}"); + print(format!("keysafe: {message}")); + } +} + +/// Prints the outcome of an action to stderr, unless quiet. +pub fn success(message: impl Display) { + if enabled(Level::Normal) { + print(format!( + "{} {message}", + style("✓").green().bold().for_stderr() + )); } } /// Prints a detail for debugging to stderr, if verbose. Never pass secret values. pub fn debug(message: impl Display) { if enabled(Level::Verbose) { - eprintln!("keysafe: debug: {message}"); + print( + style(format!("keysafe: debug: {message}")) + .dim() + .for_stderr() + .to_string(), + ); + } +} + +/// Spinner shows a message on stderr while something slow runs, until it is dropped. +/// +/// Nothing is shown when stderr is not a terminal or keysafe runs quietly. +pub struct Spinner(Option); + +/// Shows `message` with a spinner on stderr until the returned [`Spinner`] is dropped. +pub fn spinner(message: impl Into) -> Spinner { + if !enabled(Level::Normal) || !Term::stderr().is_term() { + return Spinner(None); + } + + let bar = ProgressBar::new_spinner(); + if let Ok(template) = ProgressStyle::with_template("{spinner:.cyan} {msg}") { + bar.set_style(template); + } + bar.set_message(message.into()); + bar.enable_steady_tick(Duration::from_millis(80)); + *SPINNER.lock().unwrap_or_else(|e| e.into_inner()) = Some(bar.clone()); + Spinner(Some(bar)) +} + +impl Drop for Spinner { + fn drop(&mut self) { + if let Some(bar) = self.0.take() { + bar.finish_and_clear(); + *SPINNER.lock().unwrap_or_else(|e| e.into_inner()) = None; + } + } +} + +/// Returns `count` with `noun`, pluralized by appending `s` (e.g. "1 secret", "3 secrets"). +pub fn count(count: usize, noun: &str) -> String { + if count == 1 { + format!("1 {noun}") + } else { + format!("{count} {noun}s") + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn count_pluralizes() { + assert_eq!(count(0, "secret"), "0 secrets"); + assert_eq!(count(1, "SSH key"), "1 SSH key"); + assert_eq!(count(3, "SSH key"), "3 SSH keys"); + } + + #[test] + fn styled_is_plain_unless_enabled() { + assert_eq!(styled("✓").green().to_string(), "✓"); } } diff --git a/src/main.rs b/src/main.rs index d921410..95c938a 100644 --- a/src/main.rs +++ b/src/main.rs @@ -25,6 +25,7 @@ fn main() -> ExitCode { std::env::remove_var("KEYSAFE_RUNTIME_DIR"); std::env::remove_var("KEYSAFE_SHELL"); + log::set_style_stdout(console::colors_enabled()); let parent = program.command.parent_mut(); log::set_level(match (parent.quiet, parent.verbose) { (true, _) => log::Level::Quiet, @@ -62,7 +63,7 @@ fn main() -> ExitCode { match run(program) { Ok(code) => code, Err(err) => { - eprintln!("keysafe: error: {err:#}"); + log::error(format!("{err:#}")); ExitCode::FAILURE } } @@ -70,9 +71,8 @@ fn main() -> ExitCode { fn run(program: Program) -> Result { match program.command { - ProgramCommand::Load(args) => { - args.output - .check_destination("load", std::io::stdout().is_terminal())?; + ProgramCommand::Load(mut args) => { + args.output.terminal = std::io::stdout().is_terminal(); let writer = statements(&args.output); let loader = loader(&args.parent); let agent = Box::new(Agent::new()); @@ -83,9 +83,8 @@ fn run(program: Program) -> Result { }; command.execute(&args)? } - ProgramCommand::Unload(args) => { - args.output - .check_destination("unload", std::io::stdout().is_terminal())?; + ProgramCommand::Unload(mut args) => { + args.output.terminal = std::io::stdout().is_terminal(); let writer = statements(&args.output); let cache = cache(&args.parent); let agent = Box::new(Agent::new()); @@ -225,10 +224,7 @@ fn cache(parent: &ProgramArgs) -> Cache { /// Returns a loader reading from the keychain cache and the 1Password CLI. fn loader(parent: &ProgramArgs) -> Loader { - Loader { - client: Box::new(Client::new()), - cache: cache(parent), - } + Loader::new(Box::new(Client::new()), cache(parent)) } /// Maps the exit status of a child process to our own, shell style. diff --git a/src/vault/loader.rs b/src/vault/loader.rs index 4101fc4..feff264 100644 --- a/src/vault/loader.rs +++ b/src/vault/loader.rs @@ -1,6 +1,7 @@ use anyhow::Result; +use std::cell::Cell; -use crate::log::{debug, warn}; +use crate::log::{debug, spinner, warn}; use crate::vault::{ fingerprint, public_key, Cache, KeyAgent, Profile, RuntimeDir, Secret, SecretClient, SecretKind, }; @@ -48,9 +49,28 @@ pub struct Loader { pub client: Box, /// Cache holding previously fetched secrets. pub cache: Cache, + /// Number of secrets read from the cache. + cached: Cell, + /// Number of secrets fetched from their provider. + fetched: Cell, } impl Loader { + /// Creates a loader fetching with `client` and caching in `cache`. + pub fn new(client: Box, cache: Cache) -> Self { + Self { + client, + cache, + cached: Cell::new(0), + fetched: Cell::new(0), + } + } + + /// Returns how many secrets were read from the cache and fetched from their provider. + pub fn counts(&self) -> (usize, usize) { + (self.cached.get(), self.fetched.get()) + } + /// Returns the value of `secret`: from the cache unless `refresh` is set, otherwise /// from 1Password, caching the fetched value. pub fn load(&self, account: &Profile, secret: &Secret, refresh: bool) -> Result { @@ -58,6 +78,7 @@ impl Loader { match self.cache.get(&account.name, &secret.name) { Ok(Some(value)) => { debug(format!("'{}' from the keychain", secret.name)); + self.cached.set(self.cached.get() + 1); return Ok(value); } Ok(None) => {} @@ -69,7 +90,14 @@ impl Loader { "'{}' from {} ({})", secret.name, account.provider, secret.path )); - let value = self.client.read(&account.provider, &secret.path)?; + let value = { + let _spinner = spinner(format!( + "Fetching {} from {}…", + secret.name, account.provider + )); + self.client.read(&account.provider, &secret.path)? + }; + self.fetched.set(self.fetched.get() + 1); // A failed cache write only costs a 1Password round trip next time. if let Err(err) = self.cache.set(&account.name, &secret.name, &value) { warn(format!("{err:#}")); From 2d85ab3cf615e5e09a48a53060bc09e6cf9d1368 Mon Sep 17 00:00:00 2001 From: Svetlin Ralchev Date: Tue, 6 Oct 2026 11:20:16 +0400 Subject: [PATCH 2/2] refactor: report where secrets came from instead of counting in the loader `Loader` counted cached and fetched secrets in `Cell`s, mutable state behind `&self` that also counted SSH keys the summary doesn't report. Now `load` returns the value with its origin (keychain or provider), and `resolve` returns the variables with how many came from where and how many failed. The loader keeps no state. --- src/app/exec.rs | 84 ++++++++++++++++++++++++---------- src/vault/loader.rs | 109 +++++++++++++++++++++++++++++--------------- 2 files changed, 131 insertions(+), 62 deletions(-) diff --git a/src/app/exec.rs b/src/app/exec.rs index f1c23ba..a08d439 100644 --- a/src/app/exec.rs +++ b/src/app/exec.rs @@ -72,13 +72,9 @@ fn write_runtime_dir( /// Resolves the cached env and file secrets of `account` from the keychain only, if the /// profile was loaded before. Only secrets that are still configured are exported, and their /// kind always comes from the current config. -fn resolve_cached( - loader: &Loader, - runtime: &RuntimeDir, - account: &Profile, -) -> Result<(Vec, usize)> { +fn resolve_cached(loader: &Loader, runtime: &RuntimeDir, account: &Profile) -> Result { let Some(names) = loader.cache.loaded(&account.name)? else { - return Ok((Vec::new(), 0)); + return Ok(Resolved::default()); }; let secrets = account @@ -122,8 +118,8 @@ impl LoadCommand { .filter(|s| s.is_variable()) .collect(); let stranded = args.output.statements_stranded(); - let (variables, mut failed) = if stranded { - (Vec::new(), 0) + let resolved = if stranded { + Resolved::default() } else { self.loader.resolve( &runtime, @@ -134,23 +130,23 @@ impl LoadCommand { }; let format = args.output.export_format(); write_runtime_dir(&mut self.writer, format, &runtime, args.output.is_eval())?; - write_variables(&mut self.writer, format, &variables)?; - if !variables.is_empty() { - let (cached, fetched) = self.loader.counts(); - let source = match (cached, fetched) { + write_variables(&mut self.writer, format, &resolved.variables)?; + if !resolved.variables.is_empty() { + let source = match (resolved.cached, resolved.fetched) { (0, _) => format!("from {}", account.provider), (_, 0) => "from the keychain".to_string(), - _ => format!( + (cached, fetched) => format!( "{cached} from the keychain, {fetched} from {}", account.provider ), }; success(format!( "Loaded {} from {} ({source})", - count(variables.len(), "secret"), + count(resolved.variables.len(), "secret"), account.name )); } + let mut failed = resolved.failed; // Add the SSH keys to the agent let keys: Vec<&Secret> = secrets @@ -420,8 +416,8 @@ impl ReadCommand { ); } - let value = self.loader.load(account, secret, args.refresh)?; - writeln!(self.writer, "{value}")?; + let loaded = self.loader.load(account, secret, args.refresh)?; + writeln!(self.writer, "{}", loaded.value)?; Ok(()) } } @@ -448,7 +444,7 @@ impl ExportCommand { let mut variables = Vec::new(); let mut failed = 0; for account in accounts { - let (mut resolved, count) = if args.cached { + let mut resolved = if args.cached { resolve_cached(&self.loader, &runtime, account)? } else { let secrets = account.secrets.iter().filter(|s| s.is_variable()); @@ -458,8 +454,8 @@ impl ExportCommand { self.loader.cache.save(account)?; resolved }; - variables.append(&mut resolved); - failed += count; + variables.append(&mut resolved.variables); + failed += resolved.failed; } let format = args.output.export_format(); @@ -565,7 +561,7 @@ impl InitCommand { let mut variables = Vec::new(); for account in &config.profiles { match resolve_cached(&self.loader, &runtime, account) { - Ok((mut resolved, _)) => variables.append(&mut resolved), + Ok(mut resolved) => variables.append(&mut resolved.variables), Err(err) => warn(format!("{err:#}")), } } @@ -596,9 +592,11 @@ impl ExecCommand { let runtime = RuntimeDir::new(Some(dir.path().to_path_buf())); let secrets = account.secrets.iter().filter(|s| s.is_variable()); - let (variables, failed) = - self.loader - .resolve(&runtime, account, secrets, Source::Any(args.refresh)); + let Resolved { + variables, failed, .. + } = self + .loader + .resolve(&runtime, account, secrets, Source::Any(args.refresh)); // Do not run the command with a partial environment finish(failed)?; @@ -2631,8 +2629,6 @@ mod tests { "{err}" ); assert_eq!(writer.contents(), ""); - // Only the SSH key was read; the variable wasn't even fetched - assert_eq!(cmd.loader.counts(), (1, 0)); Ok(()) } @@ -2700,6 +2696,44 @@ mod tests { Ok(()) } + #[test] + fn resolve_reports_where_values_came_from() -> Result<()> { + let fixture = Fixture::new().cached("personal", "GITHUB_TOKEN", "a"); + let mut client = MockSecretClient::new(); + expect_read(&mut client, "op://Personal/GCP/credentials", "{}"); + client + .expect_read() + .withf(|_, p| p == "op://Infra/Prod/API_KEY") + .returning(|_, _| Err(anyhow::anyhow!("oh no"))); + let loader = fixture.loader(client); + let config = Config::read_from_file(&fixture.parent().config)?; + let personal = config.profile("personal")?; + let runtime = RuntimeDir::new(Some(fixture.runtime_dir())); + + let resolved = loader.resolve( + &runtime, + personal, + personal.secrets.iter().filter(|s| s.is_variable()), + Source::Any(false), + ); + assert_eq!( + (resolved.cached, resolved.fetched, resolved.failed), + (1, 1, 0) + ); + assert_eq!(resolved.variables.len(), 2); + + let work = config.profile("work")?; + let resolved = loader.resolve(&runtime, work, &work.secrets, Source::Any(false)); + assert_eq!( + resolved, + Resolved { + failed: 1, + ..Default::default() + } + ); + Ok(()) + } + #[test] fn profile_clear_deletes_cached_secrets() -> Result<()> { let fixture = Fixture::new() diff --git a/src/vault/loader.rs b/src/vault/loader.rs index feff264..d6980d1 100644 --- a/src/vault/loader.rs +++ b/src/vault/loader.rs @@ -1,5 +1,4 @@ use anyhow::Result; -use std::cell::Cell; use crate::log::{debug, spinner, warn}; use crate::vault::{ @@ -43,43 +42,62 @@ pub enum KeyOutcome { Present, } +/// Where a secret's value came from. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Origin { + /// The keychain cache. + Cache, + /// The profile's provider, e.g. 1Password. + Provider, +} + +/// A secret's value and where it came from. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct Loaded { + /// The secret's value. + pub value: String, + /// Where the value came from. + pub origin: Origin, +} + +/// The variables [`Loader::resolve`] produced, and how they were obtained. +#[derive(Debug, Default, PartialEq, Eq)] +pub struct Resolved { + /// The variables of the secrets that were resolved. + pub variables: Vec, + /// How many of them came from the keychain cache. + pub cached: usize, + /// How many of them were fetched from the provider. + pub fetched: usize, + /// How many secrets failed; each failure was reported. + pub failed: usize, +} + /// Loader resolves secret values from the keychain cache or 1Password. pub struct Loader { /// Client used to fetch secrets from 1Password. pub client: Box, /// Cache holding previously fetched secrets. pub cache: Cache, - /// Number of secrets read from the cache. - cached: Cell, - /// Number of secrets fetched from their provider. - fetched: Cell, } impl Loader { /// Creates a loader fetching with `client` and caching in `cache`. pub fn new(client: Box, cache: Cache) -> Self { - Self { - client, - cache, - cached: Cell::new(0), - fetched: Cell::new(0), - } - } - - /// Returns how many secrets were read from the cache and fetched from their provider. - pub fn counts(&self) -> (usize, usize) { - (self.cached.get(), self.fetched.get()) + Self { client, cache } } /// Returns the value of `secret`: from the cache unless `refresh` is set, otherwise /// from 1Password, caching the fetched value. - pub fn load(&self, account: &Profile, secret: &Secret, refresh: bool) -> Result { + pub fn load(&self, account: &Profile, secret: &Secret, refresh: bool) -> Result { if !refresh { match self.cache.get(&account.name, &secret.name) { Ok(Some(value)) => { debug(format!("'{}' from the keychain", secret.name)); - self.cached.set(self.cached.get() + 1); - return Ok(value); + return Ok(Loaded { + value, + origin: Origin::Cache, + }); } Ok(None) => {} Err(err) => warn(format!("{err:#}; fetching it from 1Password")), @@ -97,12 +115,14 @@ impl Loader { )); self.client.read(&account.provider, &secret.path)? }; - self.fetched.set(self.fetched.get() + 1); // A failed cache write only costs a 1Password round trip next time. if let Err(err) = self.cache.set(&account.name, &secret.name, &value) { warn(format!("{err:#}")); } - Ok(value) + Ok(Loaded { + value, + origin: Origin::Provider, + }) } /// Resolves the variables of the given env and file secrets, writing file secrets to @@ -113,36 +133,51 @@ impl Loader { account: &Profile, secrets: impl IntoIterator, source: Source, - ) -> (Vec, usize) { - let mut variables = Vec::new(); - let mut failed = 0; + ) -> Resolved { + let mut resolved = Resolved::default(); for secret in secrets { let result = match source { - Source::Cache => self.cache.get(&account.name, &secret.name), + Source::Cache => self.cache.get(&account.name, &secret.name).map(|value| { + value.map(|value| Loaded { + value, + origin: Origin::Cache, + }) + }), Source::Any(refresh) => self.load(account, secret, refresh).map(Some), } - .and_then(|value| match (value, secret.kind) { - (Some(value), SecretKind::File) => runtime - .write(&account.name, &secret.name, &value) - .map(|path| Some(path.to_string_lossy().into_owned())), - (value, _) => Ok(value), + .and_then(|loaded| match (loaded, secret.kind) { + (Some(loaded), SecretKind::File) => runtime + .write(&account.name, &secret.name, &loaded.value) + .map(|path| { + Some(Loaded { + value: path.to_string_lossy().into_owned(), + origin: loaded.origin, + }) + }), + (loaded, _) => Ok(loaded), }); match result { - Ok(Some(value)) => variables.push(Variable { - key: secret.name.clone(), - value, - }), + Ok(Some(loaded)) => { + match loaded.origin { + Origin::Cache => resolved.cached += 1, + Origin::Provider => resolved.fetched += 1, + } + resolved.variables.push(Variable { + key: secret.name.clone(), + value: loaded.value, + }); + } Ok(None) => {} Err(err) => { warn(format!("failed to load '{}': {err:#}", secret.name)); - failed += 1; + resolved.failed += 1; } } } - (variables, failed) + resolved } /// Adds the SSH key `secret` to the agent, unless the agent already holds it and @@ -156,7 +191,7 @@ impl Loader { lifetime: &str, refresh: bool, ) -> Result { - let key = self.load(account, secret, refresh)?; + let key = self.load(account, secret, refresh)?.value; let added = fingerprint(&key) .ok()