From 509fedf72f06ca10aa5d47a90d14e62f965e5edd Mon Sep 17 00:00:00 2001 From: Svetlin Ralchev Date: Wed, 7 Oct 2026 09:17:57 +0400 Subject: [PATCH 1/2] feat: clean up secrets removed from the config with `profile prune` Removing a secret from config.yml left it behind: `unload` only unset configured secrets, so a removed variable stayed in the shell and a removed SSH key stayed in ssh-agent, and its cached value stayed in the keychain until `profile clear` wiped the whole profile. - `unload` also unsets the secrets recorded as loaded and removes the SSH keys keysafe recorded adding, even if the config no longer names them; `unload ` accepts those names too. - `profile prune ` deletes cached secrets the config no longer names (under keysafe. and op-secrets-) and removes their SSH keys from ssh-agent, keeping the profile loaded. For a profile removed from the config, it deletes everything keysafe kept of it and forgets it. - `doctor` warns about orphaned keychain items, profiles recorded as loaded that the config no longer has, and orphaned keys ssh-agent still holds, and suggests `profile prune`. `load` doesn't prune: the cache is only cleaned when asked. Closes #33 --- README.md | 7 +- src/app/args.rs | 35 ++- src/app/exec.rs | 531 ++++++++++++++++++++++++++++++++++++++------ src/main.rs | 6 + src/vault/cache.rs | 153 ++++++++++++- src/vault/config.rs | 17 +- 6 files changed, 661 insertions(+), 88 deletions(-) diff --git a/README.md b/README.md index df45eeb..410a391 100644 --- a/README.md +++ b/README.md @@ -140,7 +140,7 @@ Commands: exec Execute a command with the secrets of a profile in its environment. status Show what is loaded: secrets in this shell, SSH keys in the agent, exported profiles. doctor Check the setup and say how to fix problems. - profile List, show and clear profiles. + profile List, show, clear and prune profiles. config Create, edit and locate the config file. init Print the shell integration script for zsh or bash. ``` @@ -160,7 +160,7 @@ keysafe load github-work -e 4h # one SSH key Loading a whole profile records it, so its cached secrets are exported in every new shell. Loading individual secrets doesn't. -`unload` undoes `load`: it unsets the variables, deletes the files of file secrets and removes the SSH keys keysafe added from ssh-agent. +`unload` undoes `load`: it unsets the variables, deletes the files of file secrets and removes the SSH keys keysafe added from ssh-agent. That includes secrets loaded before you removed them from the config. ```bash keysafe unload -p work # the whole profile; new shells no longer get it either @@ -200,8 +200,11 @@ keysafe status -p work # one profile keysafe profile list # profile names keysafe profile show work # provider, secrets and whether it's exported in new shells (never values) keysafe profile clear work # delete its cached secrets and forget it was loaded +keysafe profile prune work # delete only what the config no longer names ``` +When you remove a secret from the config, its cached value and any SSH key keysafe added stay behind until you prune them. `keysafe doctor` tells you when there is something to prune, including profiles you removed from the config. Pruning doesn't touch open shells: run `keysafe unload` there. + ## How It Works 1. **Configuration**: profiles map secret names to `op://` references. diff --git a/src/app/args.rs b/src/app/args.rs index d93bda5..011c8e2 100644 --- a/src/app/args.rs +++ b/src/app/args.rs @@ -43,6 +43,9 @@ const SHOW_EXAMPLES: &str = "Examples: keysafe profile show work # one profile"; const CLEAR_EXAMPLES: &str = "Examples: keysafe profile clear work # delete its cached secrets"; +const PRUNE_EXAMPLES: &str = "Examples: + keysafe profile prune work # delete what the config no longer names + keysafe profile prune old # everything of a profile removed from the config"; const INIT_CONFIG_EXAMPLES: &str = "Examples: keysafe config init # create ~/.config/keysafe/config.yml keysafe config init --force # start over"; @@ -191,7 +194,7 @@ pub enum ProgramCommand { name = "unload", after_help = UNLOAD_EXAMPLES, about = "Unload secrets of a profile from the current shell.", - long_about = "Undo `load`: unset the environment variables, delete the files of file secrets, and remove the SSH keys keysafe added from ssh-agent. Without names, the whole profile is unloaded and no longer exported in new shells. Its cached secrets stay; use `profile clear` to delete them. Needs the shell integration (`keysafe init`); otherwise, evaluate the printed statements yourself.", + long_about = "Undo `load`: unset the environment variables, delete the files of file secrets, and remove the SSH keys keysafe added from ssh-agent. Without names, the whole profile is unloaded and no longer exported in new shells, including secrets loaded before they were removed from the config. Its cached secrets stay; use `profile clear` to delete them. Needs the shell integration (`keysafe init`); otherwise, evaluate the printed statements yourself.", next_display_order = 2 )] Unload(UnloadCommandArgs), @@ -246,10 +249,10 @@ pub enum ProgramCommand { )] Doctor(DoctorCommandArgs), - /// List, show and clear profiles. + /// List, show, clear and prune profiles. #[command( name = "profile", - about = "List, show and clear profiles.", + about = "List, show, clear and prune profiles.", next_display_order = 8 )] Profile(ProfileCommandArgs), @@ -288,6 +291,7 @@ impl ProgramCommand { ProfileCommand::List(args) => &mut args.parent, ProfileCommand::Show(args) => &mut args.parent, ProfileCommand::Clear(args) => &mut args.parent, + ProfileCommand::Prune(args) => &mut args.parent, }, Self::Config(args) => match &mut args.command { ConfigCommand::Init(args) => &mut args.parent, @@ -364,6 +368,16 @@ pub enum ProfileCommand { next_display_order = 3 )] Clear(ProfileClearCommandArgs), + + /// Delete what keysafe keeps of secrets removed from the config. + #[command( + name = "prune", + after_help = PRUNE_EXAMPLES, + about = "Delete what keysafe keeps of secrets removed from the config.", + long_about = "Delete the cached secrets of a profile that its config no longer names, and remove the SSH keys keysafe added for them from ssh-agent. The profile stays loaded. For a profile removed from the config, everything keysafe kept of it is deleted. Variables already set in open shells stay; use `unload` there. `keysafe doctor` says when there is something to prune.", + next_display_order = 4 + )] + Prune(ProfilePruneCommandArgs), } /// Shell specifies a shell supported by the shell integration. @@ -697,6 +711,18 @@ pub struct ProfileClearCommandArgs { pub profile: String, } +/// ProfilePruneCommandArgs defines the arguments for the ProfilePruneCommand. +#[derive(Debug, Args)] +pub struct ProfilePruneCommandArgs { + /// Shared global flags. + #[command(flatten)] + pub parent: ProgramArgs, + + /// Profile whose orphaned secrets are deleted. + #[arg(help = "Profile name (may be one removed from the config).")] + pub profile: String, +} + /// ExportCommandArgs defines the arguments for the ExportCommand. #[derive(Debug, Args)] pub struct ExportCommandArgs { @@ -968,8 +994,9 @@ mod tests { }; assert_eq!(args.profile, "work"); - // Clearing needs an explicit profile + // Clearing and pruning need an explicit profile assert!(Program::try_parse_from(["keysafe", "profile", "clear"]).is_err()); + assert!(Program::try_parse_from(["keysafe", "profile", "prune"]).is_err()); } #[test] diff --git a/src/app/exec.rs b/src/app/exec.rs index 064b763..6c7cb1b 100644 --- a/src/app/exec.rs +++ b/src/app/exec.rs @@ -261,42 +261,36 @@ impl UnloadCommand { pub fn execute(&mut self, args: &UnloadCommandArgs) -> Result<()> { let config = Config::read_from_file(&args.parent.config)?; let profile = config.resolve(args.profile.as_deref())?; - let secrets: Vec<&Secret> = if args.names.is_empty() { - profile.secrets.iter().collect() - } else { - args.names - .iter() - .map(|name| profile.secret(name)) - .collect::>()? - }; + let secrets = self.secrets(profile, &args.names)?; if args.output.export_format() == ExportFormat::Json { bail!("unload prints shell statements; use --format zsh or --format bash"); } // 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(); + let mut variables: Vec<&str> = Vec::new(); + for (kind, name) in &secrets { + if *kind != SecretKind::Ssh && !variables.contains(&name.as_str()) { + variables.push(name); + } + } let stranded = args.output.statements_stranded(); if !stranded { - for secret in &variables { - if secret.kind == SecretKind::File { - self.remove_file(profile, secret); + for name in &variables { + if secrets.contains(&(SecretKind::File, name.to_string())) { + self.remove_file(&profile.name, name); } - writeln!(self.writer, "unset {}", secret.name)?; + writeln!(self.writer, "unset {name}")?; } } // Remove the SSH keys keysafe added from the agent - let keys: Vec<&Secret> = secrets + let keys: Vec<&str> = secrets .iter() - .copied() - .filter(|s| s.kind == SecretKind::Ssh) + .filter(|(kind, _)| *kind == SecretKind::Ssh) + .map(|(_, name)| name.as_str()) .collect(); - let (removed, failed) = self.remove_keys(profile, &keys); + let (removed, failed) = remove_keys(&self.cache, self.agent.as_ref(), &profile.name, &keys); // SSH keys don't need the shell; variables do if stranded && !variables.is_empty() { @@ -327,14 +321,55 @@ impl UnloadCommand { finish(failed) } - /// Deletes the file of the file secret `secret`, if its variable points to a file that + /// Returns the kind and name of the secrets to unload: the given `names`, or every secret + /// of `profile`. Besides the configured secrets, these include the secrets recorded as + /// loaded and the SSH keys keysafe added, even if they were removed from the config since. + fn secrets(&self, profile: &Profile, names: &[String]) -> Result> { + let mut known: Vec<(SecretKind, String)> = profile + .secrets + .iter() + .map(|s| (s.kind, s.name.clone())) + .collect(); + known.extend(self.cache.loaded_secrets(&profile.name)?); + match self.cache.keys() { + Ok(records) => known.extend( + records + .into_iter() + .filter(|r| r.profile == profile.name) + .map(|r| (SecretKind::Ssh, r.name)), + ), + Err(err) => warn(format!("{err:#}")), + } + let mut secrets = Vec::new(); + for secret in known { + if !secrets.contains(&secret) { + secrets.push(secret); + } + } + if names.is_empty() { + return Ok(secrets); + } + + let mut selected = Vec::new(); + for name in names { + let matching: Vec<_> = secrets.iter().filter(|(_, n)| n == name).collect(); + if matching.is_empty() { + // Neither configured nor loaded: fails, listing the configured secrets + profile.secret(name)?; + } + selected.extend(matching.into_iter().cloned()); + } + Ok(selected) + } + + /// Deletes the file of the file secret `name`, if its variable points to a file that /// keysafe wrote (`/files//`). - fn remove_file(&self, profile: &Profile, secret: &Secret) { - let Some(value) = self.environment.get(&secret.name) else { + fn remove_file(&self, profile: &str, name: &str) { + let Some(value) = self.environment.get(name) else { return; }; let path = Path::new(value); - let written = Path::new("files").join(&profile.name).join(&secret.name); + let written = Path::new("files").join(profile).join(name); if path.ends_with(&written) { if let Err(err) = std::fs::remove_file(path) { if err.kind() != std::io::ErrorKind::NotFound { @@ -343,55 +378,60 @@ impl UnloadCommand { } } } +} - /// Removes `keys` from the agent, if keysafe added them and the agent still holds them. - /// Returns the number of removed keys and of failures. - fn remove_keys(&self, profile: &Profile, keys: &[&Secret]) -> (usize, usize) { - if keys.is_empty() { - return (0, 0); +/// Removes the SSH keys `names` of `profile` from `agent`, if keysafe added them and the agent +/// still holds them. Returns the number of removed keys and of failures. +fn remove_keys( + cache: &Cache, + agent: &dyn KeyAgent, + profile: &str, + names: &[&str], +) -> (usize, usize) { + if names.is_empty() { + return (0, 0); + } + // Without a running agent, there is nothing to remove + let Ok(present) = agent.fingerprints() else { + return (0, 0); + }; + let records = match cache.keys() { + Ok(records) => records, + Err(err) => { + warn(format!("{err:#}")); + return (0, names.len()); } - // Without a running agent, there is nothing to remove - let Ok(present) = self.agent.fingerprints() else { - return (0, 0); + }; + + let (mut removed, mut failed) = (Vec::new(), 0); + for name in names { + let Some(record) = records + .iter() + .find(|r| r.profile == profile && r.name == *name && present.contains(&r.fingerprint)) + else { + continue; }; - let records = match self.cache.keys() { - Ok(records) => records, + if record.public_key.is_empty() { + warn(format!( + "SSH key '{name}' was added by an older keysafe; remove it with `ssh-add -d`" + )); + continue; + } + match agent.remove(&record.public_key) { + Ok(()) => removed.push(record.fingerprint.clone()), Err(err) => { - warn(format!("{err:#}")); - return (0, keys.len()); - } - }; - - let (mut removed, mut failed) = (Vec::new(), 0); - for key in keys { - let Some(record) = records.iter().find(|r| { - r.profile == profile.name && r.name == key.name && present.contains(&r.fingerprint) - }) else { - continue; - }; - if record.public_key.is_empty() { - warn(format!( - "SSH key '{}' was added by an older keysafe; remove it with `ssh-add -d`", - key.name - )); - continue; - } - match self.agent.remove(&record.public_key) { - Ok(()) => removed.push(record.fingerprint.clone()), - Err(err) => { - warn(format!("failed to remove SSH key '{}': {err:#}", key.name)); - failed += 1; - } + warn(format!("failed to remove SSH key '{name}': {err:#}")); + failed += 1; } } + } - if !removed.is_empty() { - if let Err(err) = self.cache.drop_keys(&removed) { - warn(format!("{err:#}")); - } + if !removed.is_empty() { + if let Err(err) = cache.drop_keys(&removed) { + warn(format!("{err:#}")); } - (removed.len(), failed) } + (removed.len(), failed) } /// Print the value of a secret. @@ -1029,6 +1069,23 @@ impl DoctorCommand { ); self.report(Check::Warning, "Keychain", detail)?; } + + // Secrets cached before they were removed from the config + let configured: Vec<&str> = + profile.secrets.iter().map(|s| s.name.as_str()).collect(); + let Ok(items) = self.cache.orphaned(&profile.name, &configured) else { + continue; + }; + if !items.is_empty() { + let detail = format!( + "{} under {}, no longer in the config: {} (remove them with `keysafe profile prune {}`)", + count(items.len(), "orphaned item"), + Cache::service(&profile.name), + items.join(", "), + profile.name + ); + self.report(Check::Warning, "Keychain", detail)?; + } } } Err(err) => { @@ -1037,17 +1094,67 @@ impl DoctorCommand { } } - // The SSH agent, if a profile has SSH keys + // Profiles loaded before they were removed from the config + if config.is_some() { + let recorded = self.cache.profiles().unwrap_or_default(); + for name in recorded.iter().filter(|name| is_profile_name(name)) { + if !profiles.iter().any(|p| p.name == *name) { + let detail = format!( + "profile '{name}' is no longer in the config (remove what keysafe kept of it with `keysafe profile prune {name}`)" + ); + self.report(Check::Warning, "Cache", detail)?; + } + } + } + + // SSH keys keysafe added before they were removed from the config + let orphaned: Vec = match config { + Some(_) => self + .cache + .keys() + .unwrap_or_default() + .into_iter() + .filter(|r| { + !profiles.iter().any(|p| { + p.name == r.profile + && p.secrets + .iter() + .any(|s| s.kind == SecretKind::Ssh && s.name == r.name) + }) + }) + .collect(), + None => Vec::new(), + }; + + // The SSH agent, if a profile has SSH keys or keysafe added some let has_keys = profiles .iter() .any(|p| p.secrets.iter().any(|s| s.kind == SecretKind::Ssh)); - if has_keys { + if has_keys || !orphaned.is_empty() { match self.agent.fingerprints() { - Ok(keys) => self.report( - Check::Ok, - "SSH agent", - format!("running, {} key(s)", keys.len()), - )?, + Ok(keys) => { + if has_keys { + self.report( + Check::Ok, + "SSH agent", + format!("running, {} key(s)", keys.len()), + )?; + } + let mut held: Vec<&KeyRecord> = orphaned + .iter() + .filter(|r| keys.contains(&r.fingerprint)) + .collect(); + held.sort_by(|a, b| (&a.profile, &a.name).cmp(&(&b.profile, &b.name))); + for record in held { + let detail = format!( + "orphaned key '{}' of {}, no longer in the config (remove it with `keysafe profile prune {}`)", + record.name, record.profile, record.profile + ); + self.report(Check::Warning, "SSH agent", detail)?; + } + } + // An agent that isn't running holds no orphaned keys + Err(_) if !has_keys => {} Err(err) => { failures += 1; self.report(Check::Failure, "SSH agent", format!("{err:#}"))?; @@ -1173,6 +1280,78 @@ impl ProfileClearCommand { } } +/// Delete what keysafe keeps of secrets removed from the config. +pub struct ProfilePruneCommand { + /// Cache the orphaned secrets are deleted from. + pub cache: Cache, + /// Agent the orphaned SSH keys are removed from. + pub agent: Box, +} + +impl ProfilePruneCommand { + /// Execute the ProfilePruneCommand with the provided arguments. + pub fn execute(&mut self, args: &ProfilePruneCommandArgs) -> Result<()> { + let config = Config::read_from_file(&args.parent.config)?; + let name = args.profile.as_str(); + // A profile removed from the config is pruned entirely, if keysafe kept anything of it + let profile = match config.profile(name) { + Ok(profile) => Some(profile), + Err(_) if is_profile_name(name) && self.kept(name)? => None, + Err(err) => return Err(err), + }; + let secrets = profile.map(|p| p.secrets.as_slice()).unwrap_or_default(); + + let configured: Vec<&str> = secrets.iter().map(|s| s.name.as_str()).collect(); + let pruned = self.cache.prune(name, &configured)?; + + let keys: Vec = self + .cache + .keys()? + .into_iter() + .filter(|r| r.profile == name) + .filter(|r| { + !secrets + .iter() + .any(|s| s.kind == SecretKind::Ssh && s.name == r.name) + }) + .map(|r| r.name) + .collect(); + let keys: Vec<&str> = keys.iter().map(String::as_str).collect(); + let (removed, failed) = remove_keys(&self.cache, self.agent.as_ref(), name, &keys); + + if pruned > 0 { + success(format!( + "Pruned {} from {name}", + count(pruned, "orphaned secret") + )); + } + if removed > 0 { + success(format!( + "Removed {} from ssh-agent", + count(removed, "SSH key") + )); + } + let forgotten = profile.is_none() && self.cache.loaded(name)?.is_some(); + if forgotten { + self.cache.forget(name)?; + success(format!("Forgot {name}, which is no longer in the config")); + } + if pruned == 0 && removed == 0 && !forgotten { + info(format!("Nothing to prune in {name}")); + } + finish(failed) + } + + /// Returns true if keysafe kept anything of `profile`: cached secrets, a record that it + /// was loaded, or SSH keys it added. + fn kept(&self, profile: &str) -> Result { + Ok(self.cache.loaded(profile)?.is_some() + || !self.cache.orphaned(profile, &[])?.is_empty() + || !self.cache.legacy_items(profile)?.is_empty() + || self.cache.keys()?.iter().any(|r| r.profile == profile)) + } +} + #[cfg(test)] mod tests { use super::*; @@ -2351,6 +2530,83 @@ mod tests { Ok(()) } + #[test] + fn unload_unsets_secrets_removed_from_the_config() -> Result<()> { + let fixture = Fixture::new().loaded( + "personal", + "env:GITHUB_TOKEN\nenv:OLD_TOKEN\nfile:OLD_FILE\n", + ); + let file = + RuntimeDir::new(Some(fixture.runtime_dir())).write("personal", "OLD_FILE", "{}")?; + let writer = Writer::new(); + let mut cmd = UnloadCommand { + writer: Box::new(writer.clone()), + ..unload( + &fixture, + agent(vec![]), + &[("OLD_FILE", &file.to_string_lossy())], + ) + }; + + cmd.execute(&unload_args(&fixture, &[]))?; + + assert_eq!( + writer.contents(), + "unset GITHUB_TOKEN\nunset GCP_CREDENTIALS\nunset OLD_TOKEN\nunset OLD_FILE\n" + ); + assert!(!file.exists()); + Ok(()) + } + + #[test] + fn unload_names_secrets_removed_from_the_config() -> Result<()> { + let fixture = Fixture::new().loaded("personal", "env:OLD_TOKEN\n"); + let writer = Writer::new(); + let mut cmd = UnloadCommand { + writer: Box::new(writer.clone()), + ..unload(&fixture, MockKeyAgent::new(), &[]) + }; + + cmd.execute(&unload_args(&fixture, &["OLD_TOKEN"]))?; + let unknown = cmd.execute(&unload_args(&fixture, &["NEVER_LOADED"])); + + assert_eq!(writer.contents(), "unset OLD_TOKEN\n"); + assert!(unknown + .unwrap_err() + .to_string() + .starts_with("secret 'NEVER_LOADED' not found in profile 'personal'")); + Ok(()) + } + + #[test] + fn unload_removes_keys_removed_from_the_config() -> Result<()> { + let fixture = Fixture::new(); + let key = fingerprint(&TEST_KEY)?; + let public = public_key(&TEST_KEY)?; + fixture.cache().record_keys( + &[KeyRecord { + profile: "personal".into(), + name: "old-key".into(), + fingerprint: key.clone(), + public_key: public.clone(), + expires: u64::MAX, + }], + 0, + )?; + let mut agent = agent(vec![key]); + agent + .expect_remove() + .withf(move |k| k == public) + .times(1) + .returning(|_| Ok(())); + let mut cmd = unload(&fixture, agent, &[]); + + cmd.execute(&unload_args(&fixture, &[]))?; + + assert_eq!(fixture.cache().keys()?, vec![]); + Ok(()) + } + #[test] fn unload_rejects_json() { let fixture = Fixture::new(); @@ -2771,6 +3027,137 @@ mod tests { Ok(()) } + fn prune_args(fixture: &Fixture, profile: &str) -> ProfilePruneCommandArgs { + ProfilePruneCommandArgs { + parent: fixture.parent(), + profile: profile.into(), + } + } + + #[test] + fn profile_prune_deletes_what_the_config_no_longer_names() -> Result<()> { + let fixture = Fixture::new() + .cached("personal", "GITHUB_TOKEN", "a") + .cached("personal", "OLD_TOKEN", "b") + .loaded("personal", "env:GITHUB_TOKEN\nenv:OLD_TOKEN\n"); + let key = fingerprint(&TEST_KEY)?; + let public = public_key(&TEST_KEY)?; + let record = |name: &str| KeyRecord { + profile: "personal".into(), + name: name.into(), + fingerprint: format!("{key}-{name}"), + public_key: public.clone(), + expires: u64::MAX, + }; + fixture + .cache() + .record_keys(&[record("my-key"), record("old-key")], 0)?; + let mut agent = agent(vec![format!("{key}-my-key"), format!("{key}-old-key")]); + agent.expect_remove().times(1).returning(|_| Ok(())); + let mut cmd = ProfilePruneCommand { + cache: fixture.cache(), + agent: Box::new(agent), + }; + + cmd.execute(&prune_args(&fixture, "personal"))?; + + assert_eq!( + fixture.value("personal", "GITHUB_TOKEN").as_deref(), + Some("a") + ); + assert_eq!(fixture.value("personal", "OLD_TOKEN"), None); + assert_eq!(fixture.cache().keys()?, vec![record("my-key")]); + // The profile stays loaded, and unload still knows about OLD_TOKEN + assert!(fixture.cache().loaded("personal")?.is_some()); + Ok(()) + } + + #[test] + fn profile_prune_deletes_a_profile_removed_from_the_config() -> Result<()> { + let fixture = Fixture::new() + .cached("old", "TOKEN", "a") + .cached("personal", "GITHUB_TOKEN", "b") + .loaded("old", "env:TOKEN\n"); + let mut cmd = ProfilePruneCommand { + cache: fixture.cache(), + agent: Box::new(MockKeyAgent::new()), + }; + + cmd.execute(&prune_args(&fixture, "old"))?; + + assert_eq!(fixture.value("old", "TOKEN"), None); + assert_eq!( + fixture.value("personal", "GITHUB_TOKEN").as_deref(), + Some("b") + ); + assert_eq!(fixture.cache().loaded("old")?, None); + Ok(()) + } + + #[test] + fn profile_prune_rejects_profiles_keysafe_knows_nothing_of() { + let fixture = Fixture::new(); + let mut cmd = ProfilePruneCommand { + cache: fixture.cache(), + agent: Box::new(MockKeyAgent::new()), + }; + + for name in ["typo", "../escape"] { + let result = cmd.execute(&prune_args(&fixture, name)); + assert!(result + .unwrap_err() + .to_string() + .starts_with(&format!("profile '{name}' not found in config"))); + } + } + + #[test] + fn doctor_warns_about_what_the_config_no_longer_names() -> Result<()> { + let fixture = Fixture::new() + .cached("personal", "OLD_TOKEN", "a") + .loaded("old", "env:TOKEN\n"); + fixture.cache().record_keys( + &[ + KeyRecord { + profile: "personal".into(), + name: "old-key".into(), + fingerprint: "SHA256:a".into(), + public_key: "ssh-ed25519 AAAA".into(), + expires: u64::MAX, + }, + KeyRecord { + profile: "personal".into(), + name: "gone-from-agent".into(), + fingerprint: "SHA256:b".into(), + public_key: "ssh-ed25519 BBBB".into(), + expires: u64::MAX, + }, + ], + 0, + )?; + let mut client = MockSecretClient::new(); + client.expect_check().returning(|_| Ok("fine".into())); + let (mut cmd, writer) = doctor(&fixture, client, agent(vec!["SHA256:a".into()])); + + cmd.execute(&DoctorCommandArgs { + parent: fixture.parent(), + shell: Some(Shell::Zsh), + })?; + + let output = writer.contents(); + assert!(output.contains( + "! Keychain: 1 orphaned item under keysafe.personal, no longer in the config: OLD_TOKEN (remove them with `keysafe profile prune personal`)\n" + )); + assert!(output.contains( + "! Cache: profile 'old' is no longer in the config (remove what keysafe kept of it with `keysafe profile prune old`)\n" + )); + assert!(output.contains( + "! SSH agent: orphaned key 'old-key' of personal, no longer in the config (remove it with `keysafe profile prune personal`)\n" + )); + assert!(!output.contains("gone-from-agent")); + Ok(()) + } + #[test] fn profile_clear_deletes_cached_secrets() -> Result<()> { let fixture = Fixture::new() diff --git a/src/main.rs b/src/main.rs index 95c938a..93b8a02 100644 --- a/src/main.rs +++ b/src/main.rs @@ -164,6 +164,12 @@ fn run(program: Program) -> Result { let mut command = ProfileClearCommand { cache }; command.execute(&args)? } + ProfileCommand::Prune(args) => { + let cache = cache(&args.parent); + let agent = Box::new(Agent::new()); + let mut command = ProfilePruneCommand { cache, agent }; + command.execute(&args)? + } }, ProgramCommand::Config(args) => match args.command { ConfigCommand::Init(args) => { diff --git a/src/vault/cache.rs b/src/vault/cache.rs index 0e9bebf..0605b71 100644 --- a/src/vault/cache.rs +++ b/src/vault/cache.rs @@ -7,7 +7,7 @@ use std::{ }; use crate::log::{debug, warn}; -use crate::vault::Profile; +use crate::vault::{Profile, SecretKind}; /// SecretStore persists secret values in a secure credential store. pub trait SecretStore { @@ -211,6 +211,24 @@ impl Cache { /// Returns the secret names recorded for `profile`, or `None` if it was never loaded. pub fn loaded(&self, profile: &str) -> Result>> { + Ok(self + .metadata(profile)? + .map(|lines| lines.into_iter().map(|(_, name)| name).collect())) + } + + /// Returns the secrets recorded for `profile` with their kind, which may no longer match + /// the config. Lines with a kind keysafe doesn't know are skipped. + pub fn loaded_secrets(&self, profile: &str) -> Result> { + Ok(self + .metadata(profile)? + .unwrap_or_default() + .into_iter() + .filter_map(|(kind, name)| Some((kind.parse().ok()?, name))) + .collect()) + } + + /// Returns the `kind:name` lines recorded for `profile`, or `None` if it was never loaded. + fn metadata(&self, profile: &str) -> Result>> { for path in self.metadata_paths(profile) { let data = match std::fs::read_to_string(&path) { Ok(data) => data, @@ -218,18 +236,74 @@ impl Cache { Err(e) => return Err(e).context(format!("failed to read {}", path.display())), }; - let names = data + let lines = data .lines() .map(str::trim) .filter(|line| !line.is_empty() && !line.starts_with('#')) // Parse: kind:name (e.g. "env:GITHUB_TOKEN" or "ssh:github-work") - .filter_map(|line| line.split_once(':').map(|(_, name)| name.to_string())) + .filter_map(|line| line.split_once(':')) + .map(|(kind, name)| (kind.to_string(), name.to_string())) .collect(); - return Ok(Some(names)); + return Ok(Some(lines)); } Ok(None) } + /// Returns the profiles recorded as loaded, whether or not they are still configured. + pub fn profiles(&self) -> Result> { + let mut profiles = Vec::new(); + for dir in std::iter::once(&self.dir).chain(self.legacy_dir.iter()) { + let entries = match std::fs::read_dir(dir) { + Ok(entries) => entries, + Err(e) if e.kind() == ErrorKind::NotFound => continue, + Err(e) => return Err(e).context(format!("failed to read {}", dir.display())), + }; + for entry in entries { + let path = entry + .with_context(|| format!("failed to read {}", dir.display()))? + .path(); + if path.extension().is_some_and(|ext| ext == "metadata") { + if let Some(name) = path.file_stem().and_then(|s| s.to_str()) { + profiles.push(name.to_string()); + } + } + } + } + profiles.sort(); + profiles.dedup(); + Ok(profiles) + } + + /// Returns the secrets cached for `profile` whose names are not in `configured`. + pub fn orphaned(&self, profile: &str, configured: &[&str]) -> Result> { + let mut names = self.store.accounts(&Self::service(profile))?; + names.retain(|name| !configured.contains(&name.as_str())); + Ok(names) + } + + /// Deletes the secrets cached for `profile`, under either service, whose names are not in + /// `configured`. Returns the number of deleted secrets. + pub fn prune(&self, profile: &str, configured: &[&str]) -> Result { + let services = [Self::service(profile), Self::legacy_service(profile)]; + let mut names = Vec::new(); + for service in &services { + names.extend(self.store.accounts(service)?); + } + names.retain(|name| !configured.contains(&name.as_str())); + names.sort(); + names.dedup(); + + let mut count = 0; + for name in &names { + let mut deleted = false; + for service in &services { + deleted |= self.store.delete(service, name)?; + } + count += usize::from(deleted); + } + Ok(count) + } + /// Records every secret of `account` as loaded. pub fn save(&self, account: &Profile) -> Result<()> { std::fs::create_dir_all(&self.dir) @@ -538,6 +612,77 @@ mod tests { assert!(cache.legacy_items("personal").unwrap().is_empty()); } + #[test] + fn prune_deletes_only_items_the_config_no_longer_names() { + let dir = tempfile::tempdir().unwrap(); + let store = MemoryStore::with(&[ + ("keysafe.personal", "GITHUB_TOKEN", "a"), + ("keysafe.personal", "OLD_TOKEN", "b"), + ("op-secrets-personal", "OLDER_TOKEN", "c"), + ("keysafe.work", "OLD_TOKEN", "d"), + ]); + let cache = Cache::new(Box::new(store.clone()), dir.path()); + cache.save(&account()).unwrap(); + let configured = ["GITHUB_TOKEN", "my-key"]; + + // Only the current service is reported, zsh-op's has its own warning + assert_eq!( + cache.orphaned("personal", &configured).unwrap(), + ["OLD_TOKEN"] + ); + assert_eq!(cache.prune("personal", &configured).unwrap(), 2); + + assert_eq!( + store.value("keysafe.personal", "GITHUB_TOKEN").as_deref(), + Some("a") + ); + assert_eq!(store.value("keysafe.personal", "OLD_TOKEN"), None); + assert_eq!(store.value("op-secrets-personal", "OLDER_TOKEN"), None); + assert_eq!( + store.value("keysafe.work", "OLD_TOKEN").as_deref(), + Some("d") + ); + // The profile stays loaded + assert!(cache.loaded("personal").unwrap().is_some()); + } + + #[test] + fn loaded_secrets_keeps_the_recorded_kinds() { + let dir = tempfile::tempdir().unwrap(); + let cache = Cache::new(Box::new(MemoryStore::default()), dir.path()); + assert!(cache.loaded_secrets("personal").unwrap().is_empty()); + std::fs::write( + cache.metadata_path("personal"), + "# Format: kind:name\n\nenv:OLD_TOKEN\nfile:GCP\nssh:old-key\nweird:X\n", + ) + .unwrap(); + + assert_eq!( + cache.loaded_secrets("personal").unwrap(), + [ + (SecretKind::Env, "OLD_TOKEN".to_string()), + (SecretKind::File, "GCP".to_string()), + (SecretKind::Ssh, "old-key".to_string()), + ] + ); + } + + #[test] + fn profiles_lists_current_and_legacy_metadata() { + let dir = tempfile::tempdir().unwrap(); + let legacy = dir.path().join("op"); + std::fs::create_dir(&legacy).unwrap(); + std::fs::write(legacy.join("old.metadata"), "env:A\n").unwrap(); + std::fs::write(legacy.join("notes.txt"), "").unwrap(); + let cache = Cache::new(Box::new(MemoryStore::default()), &dir.path().join("new")) + .with_legacy_dir(Some(legacy)); + assert_eq!(cache.profiles().unwrap(), ["old"]); + + cache.save(&account()).unwrap(); + + assert_eq!(cache.profiles().unwrap(), ["old", "personal"]); + } + #[test] fn service_uses_profile_prefix() { assert_eq!(Cache::service("work"), "keysafe.work"); diff --git a/src/vault/config.rs b/src/vault/config.rs index e400c26..3a4373d 100644 --- a/src/vault/config.rs +++ b/src/vault/config.rs @@ -144,12 +144,7 @@ impl TryFrom for Config { .name .filter(|s| !s.is_empty()) .ok_or_else(|| anyhow!("profile at index {i} missing 'name' field"))?; - // Profile names are used in file paths and keychain service names. - if name.starts_with('.') - || !name - .chars() - .all(|c| c.is_ascii_alphanumeric() || matches!(c, '.' | '_' | '-')) - { + if !is_profile_name(&name) { bail!("profile name '{name}' may only contain letters, digits, '.', '_' and '-'"); } if profiles.iter().any(|a: &Profile| a.name == name) { @@ -225,6 +220,16 @@ impl TryFrom for Config { } } +/// Returns true if `name` can name a profile. Profile names are used in file paths and +/// keychain service names. +pub fn is_profile_name(name: &str) -> bool { + !name.is_empty() + && !name.starts_with('.') + && name + .chars() + .all(|c| c.is_ascii_alphanumeric() || matches!(c, '.' | '_' | '-')) +} + /// Returns true if `name` is a valid shell environment variable name. fn is_variable_name(name: &str) -> bool { let mut chars = name.chars(); From 37ebb31a3028a837a230a54b5da7333b04faaad8 Mon Sep 17 00:00:00 2001 From: Svetlin Ralchev Date: Wed, 7 Oct 2026 09:19:56 +0400 Subject: [PATCH 2/2] fix: point doctor to `profile prune` for zsh-op leftovers `doctor` suggested `profile clear` for items zsh-op left under op-secrets-, which wipes the whole cache, while `profile prune` now removes exactly the leftovers the config no longer names. The warning counts only those items, since configured ones move to keysafe. when they are read, and suggests `profile prune`. The help of `profile clear` and `profile prune` now mentions the other. --- README.md | 2 +- src/app/args.rs | 4 ++-- src/app/exec.rs | 34 +++++++++++++++++++--------------- 3 files changed, 22 insertions(+), 18 deletions(-) diff --git a/README.md b/README.md index 410a391..5bc67bd 100644 --- a/README.md +++ b/README.md @@ -221,7 +221,7 @@ keysafe picks up where the zsh-op plugin left off: - With no config at the new location, `~/.config/op/config.yml` is used, with a hint to move it. - Profiles recorded in `~/.cache/op` count as loaded until you load them again. - Secrets cached under `op-secrets-` move to `keysafe.` the first time they are read; the old items are deleted. -- Items zsh-op cached that keysafe never reads, such as secrets no longer in your config, stay behind. `keysafe doctor` warns about them and `keysafe profile clear ` deletes them. +- Items zsh-op cached that keysafe never reads, such as secrets no longer in your config, stay behind. `keysafe doctor` warns about them and `keysafe profile prune ` deletes them. ## Troubleshooting diff --git a/src/app/args.rs b/src/app/args.rs index 011c8e2..4b8f8fa 100644 --- a/src/app/args.rs +++ b/src/app/args.rs @@ -364,7 +364,7 @@ pub enum ProfileCommand { name = "clear", after_help = CLEAR_EXAMPLES, about = "Clear the cached secrets of a profile.", - long_about = "Delete every cached secret of a profile from the keychain and forget that the profile was loaded.", + long_about = "Delete every cached secret of a profile from the keychain and forget that the profile was loaded. The next `load` fetches every secret from 1Password again. To delete only the secrets the config no longer names, use `profile prune`.", next_display_order = 3 )] Clear(ProfileClearCommandArgs), @@ -374,7 +374,7 @@ pub enum ProfileCommand { name = "prune", after_help = PRUNE_EXAMPLES, about = "Delete what keysafe keeps of secrets removed from the config.", - long_about = "Delete the cached secrets of a profile that its config no longer names, and remove the SSH keys keysafe added for them from ssh-agent. The profile stays loaded. For a profile removed from the config, everything keysafe kept of it is deleted. Variables already set in open shells stay; use `unload` there. `keysafe doctor` says when there is something to prune.", + long_about = "Delete the cached secrets of a profile that its config no longer names, and remove the SSH keys keysafe added for them from ssh-agent. The profile stays loaded. For a profile removed from the config, everything keysafe kept of it is deleted. Variables already set in open shells stay; use `unload` there. `keysafe doctor` says when there is something to prune. To delete every cached secret of a profile, use `profile clear`.", next_display_order = 4 )] Prune(ProfilePruneCommandArgs), diff --git a/src/app/exec.rs b/src/app/exec.rs index 6c7cb1b..2260d3b 100644 --- a/src/app/exec.rs +++ b/src/app/exec.rs @@ -1055,24 +1055,26 @@ impl DoctorCommand { Ok(_) => { self.report(Check::Ok, "Keychain", "reachable")?; - // Secrets zsh-op cached under its own service names for profile in profiles { - let Ok(items) = self.cache.legacy_items(&profile.name) else { - continue; - }; - if !items.is_empty() { - let detail = format!( - "{} left from zsh-op under {} (remove them with `keysafe profile clear {}`)", - count(items.len(), "item"), - Cache::legacy_service(&profile.name), - profile.name - ); - self.report(Check::Warning, "Keychain", detail)?; + let configured: Vec<&str> = + profile.secrets.iter().map(|s| s.name.as_str()).collect(); + + // Secrets zsh-op cached under its own service names. Those still in the + // config move to keysafe's service when they are read. + if let Ok(mut items) = self.cache.legacy_items(&profile.name) { + items.retain(|name| !configured.contains(&name.as_str())); + if !items.is_empty() { + let detail = format!( + "{} left from zsh-op under {}, no longer in the config (remove them with `keysafe profile prune {}`)", + count(items.len(), "item"), + Cache::legacy_service(&profile.name), + profile.name + ); + self.report(Check::Warning, "Keychain", detail)?; + } } // Secrets cached before they were removed from the config - let configured: Vec<&str> = - profile.secrets.iter().map(|s| s.name.as_str()).collect(); let Ok(items) = self.cache.orphaned(&profile.name, &configured) else { continue; }; @@ -2850,6 +2852,8 @@ mod tests { fn doctor_warns_about_items_left_from_zsh_op() -> Result<()> { let fixture = Fixture::new(); fixture.store.set("op-secrets-work", "OLD_TOKEN", "x")?; + // Still configured: moves to keysafe.work when it is read + fixture.store.set("op-secrets-work", "API_KEY", "y")?; let mut client = MockSecretClient::new(); client.expect_check().returning(|_| Ok("fine".into())); let (mut cmd, writer) = doctor(&fixture, client, agent(vec![])); @@ -2860,7 +2864,7 @@ mod tests { })?; assert!(writer.contents().contains( - "! Keychain: 1 item left from zsh-op under op-secrets-work (remove them with `keysafe profile clear work`)\n" + "! Keychain: 1 item left from zsh-op under op-secrets-work, no longer in the config (remove them with `keysafe profile prune work`)\n" )); Ok(()) }