From fd564b7bbb6a5ef027473f2da2a7ddef4740c3da Mon Sep 17 00:00:00 2001 From: Elizabeth Engelman <4752801+elizabethengelman@users.noreply.github.com> Date: Tue, 26 Nov 2024 15:37:28 -0500 Subject: [PATCH 1/7] Add delete_password fn to StellarEntry --- cmd/soroban-cli/src/signer/keyring.rs | 31 ++++++++++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-) diff --git a/cmd/soroban-cli/src/signer/keyring.rs b/cmd/soroban-cli/src/signer/keyring.rs index b5a63a398..0a07e9c33 100644 --- a/cmd/soroban-cli/src/signer/keyring.rs +++ b/cmd/soroban-cli/src/signer/keyring.rs @@ -35,6 +35,10 @@ impl StellarEntry { Ok(base64.decode(self.keyring.get_password()?)?) } + pub fn delete_password(&self) -> Result<(), Error> { + Ok(self.keyring.delete_credential()?) + } + fn use_key( &self, f: impl FnOnce(ed25519_dalek::SigningKey) -> Result, @@ -116,7 +120,7 @@ mod test { fn test_sign_data() { set_default_credential_builder(mock::default_credential_builder()); - //create a secret + // create a secret let secret = crate::config::secret::Secret::from_seed(None).unwrap(); let key_pair = secret.key_pair(None).unwrap(); @@ -129,4 +133,29 @@ mod test { let sign_tx_env_result = entry.sign_data(tx_xdr.as_bytes()); assert!(sign_tx_env_result.is_ok()); } + + #[test] + fn test_delete_password() { + set_default_credential_builder(mock::default_credential_builder()); + + // add a keyring entry + let entry = StellarEntry::new("test").unwrap(); + entry.set_password("test password".as_bytes()).unwrap(); + + // assert it is there + let get_password_result = entry.get_password(); + assert!(get_password_result.is_ok()); + + // delete the password + let delete_password_result = entry.delete_password(); + assert!(delete_password_result.is_ok()); + + // confirm the entry is gone + let get_password_result = entry.get_password(); + assert!(get_password_result.is_err()); + assert!(matches!( + get_password_result.unwrap_err(), + Error::Keyring(_) + )); + } } From b41dc49bc7e23b1c036c8220a6a4a65738b35ec0 Mon Sep 17 00:00:00 2001 From: Elizabeth Engelman <4752801+elizabethengelman@users.noreply.github.com> Date: Mon, 2 Dec 2024 10:45:05 -0500 Subject: [PATCH 2/7] Remove secure store entry when removing identity --- cmd/soroban-cli/src/config/locator.rs | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/cmd/soroban-cli/src/config/locator.rs b/cmd/soroban-cli/src/config/locator.rs index b6f5c75c1..c33646938 100644 --- a/cmd/soroban-cli/src/config/locator.rs +++ b/cmd/soroban-cli/src/config/locator.rs @@ -12,7 +12,12 @@ use std::{ }; use stellar_strkey::{Contract, DecodeError}; -use crate::{commands::HEADING_GLOBAL, utils::find_config_dir, Pwd}; +use crate::{ + commands::HEADING_GLOBAL, + signer::{self, keyring::StellarEntry}, + utils::find_config_dir, + Pwd, +}; use super::{ alias, @@ -83,6 +88,8 @@ pub enum Error { UpgradeCheckReadFailed { path: PathBuf, error: io::Error }, #[error("Failed to write upgrade check file: {path}: {error}")] UpgradeCheckWriteFailed { path: PathBuf, error: io::Error }, + #[error(transparent)] + Keyring(#[from] signer::keyring::Error), } #[derive(Debug, clap::Args, Default, Clone)] @@ -254,6 +261,22 @@ impl Args { } pub fn remove_identity(&self, name: &str) -> Result<(), Error> { + let identity = self.read_identity(name)?; + match identity { + Secret::SecureStore { entry_name } => { + let entry = StellarEntry::new(&entry_name)?; + let _ = entry.delete_password().map_err(|e| { + if e.to_string() == keyring::Error::NoEntry.to_string() { + println!("This key was already removed from the secure store. Removing the config file"); + return Ok(()); + } else { + Err(Error::Keyring(e)) + } + }); + } + _ => {} + } + KeyType::Identity.remove(name, &self.config_dir()?) } From f3370b49753976a313b9f8b00ae60e7c1cb6bdfd Mon Sep 17 00:00:00 2001 From: Elizabeth Engelman <4752801+elizabethengelman@users.noreply.github.com> Date: Mon, 2 Dec 2024 11:35:23 -0500 Subject: [PATCH 3/7] Refactor/cleanup --- cmd/soroban-cli/src/commands/keys/mod.rs | 2 +- cmd/soroban-cli/src/commands/keys/rm.rs | 6 ++++-- cmd/soroban-cli/src/config/locator.rs | 21 ++++++++++++--------- 3 files changed, 17 insertions(+), 12 deletions(-) diff --git a/cmd/soroban-cli/src/commands/keys/mod.rs b/cmd/soroban-cli/src/commands/keys/mod.rs index b3309fa02..4d44c081f 100644 --- a/cmd/soroban-cli/src/commands/keys/mod.rs +++ b/cmd/soroban-cli/src/commands/keys/mod.rs @@ -75,7 +75,7 @@ impl Cmd { Cmd::Fund(cmd) => cmd.run().await?, Cmd::Generate(cmd) => cmd.run(global_args).await?, Cmd::Ls(cmd) => cmd.run()?, - Cmd::Rm(cmd) => cmd.run()?, + Cmd::Rm(cmd) => cmd.run(global_args)?, Cmd::Show(cmd) => cmd.run()?, Cmd::Default(cmd) => cmd.run(global_args)?, }; diff --git a/cmd/soroban-cli/src/commands/keys/rm.rs b/cmd/soroban-cli/src/commands/keys/rm.rs index df48108d3..c551d05e8 100644 --- a/cmd/soroban-cli/src/commands/keys/rm.rs +++ b/cmd/soroban-cli/src/commands/keys/rm.rs @@ -1,5 +1,7 @@ use clap::command; +use crate::commands::global; + use super::super::config::locator; #[derive(thiserror::Error, Debug)] @@ -19,7 +21,7 @@ pub struct Cmd { } impl Cmd { - pub fn run(&self) -> Result<(), Error> { - Ok(self.config.remove_identity(&self.name)?) + pub fn run(&self, global_args: &global::Args) -> Result<(), Error> { + Ok(self.config.remove_identity(&self.name, global_args)?) } } diff --git a/cmd/soroban-cli/src/config/locator.rs b/cmd/soroban-cli/src/config/locator.rs index c33646938..a0450e5a0 100644 --- a/cmd/soroban-cli/src/config/locator.rs +++ b/cmd/soroban-cli/src/config/locator.rs @@ -13,7 +13,8 @@ use std::{ use stellar_strkey::{Contract, DecodeError}; use crate::{ - commands::HEADING_GLOBAL, + commands::{global, HEADING_GLOBAL}, + print::Print, signer::{self, keyring::StellarEntry}, utils::find_config_dir, Pwd, @@ -260,19 +261,21 @@ impl Args { res } - pub fn remove_identity(&self, name: &str) -> Result<(), Error> { + pub fn remove_identity(&self, name: &str, global_args: &global::Args) -> Result<(), Error> { + let printer = Print::new(global_args.quiet); let identity = self.read_identity(name)?; match identity { Secret::SecureStore { entry_name } => { let entry = StellarEntry::new(&entry_name)?; - let _ = entry.delete_password().map_err(|e| { - if e.to_string() == keyring::Error::NoEntry.to_string() { - println!("This key was already removed from the secure store. Removing the config file"); - return Ok(()); - } else { - Err(Error::Keyring(e)) + match entry.delete_password() { + Ok(_) => {} + Err(e) if e.to_string() == keyring::Error::NoEntry.to_string() => { + printer.infoln("This key was already removed from the secure store. Removing the config file."); } - }); + Err(e) => { + return Err(Error::Keyring(e)); + } + } } _ => {} } From fe02078a5378dca3a45ba34de57897aea5ef45c5 Mon Sep 17 00:00:00 2001 From: Elizabeth Engelman <4752801+elizabethengelman@users.noreply.github.com> Date: Fri, 6 Dec 2024 11:19:25 -0500 Subject: [PATCH 4/7] Support keys add for secure store --- cmd/soroban-cli/src/config/secret.rs | 29 ++++++++++++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-) diff --git a/cmd/soroban-cli/src/config/secret.rs b/cmd/soroban-cli/src/config/secret.rs index cd3bc908a..8f0ee8673 100644 --- a/cmd/soroban-cli/src/config/secret.rs +++ b/cmd/soroban-cli/src/config/secret.rs @@ -38,11 +38,25 @@ pub enum Error { pub struct Args { /// Add using `secret_key` /// Can provide with `SOROBAN_SECRET_KEY` - #[arg(long, conflicts_with = "seed_phrase")] + #[arg(long, conflicts_with = "seed_phrase", conflicts_with = "secure_store")] pub secret_key: bool, + /// Add using 12 word seed phrase to generate `secret_key` - #[arg(long, conflicts_with = "secret_key")] + #[arg(long, conflicts_with = "secret_key", conflicts_with = "secure_store")] pub seed_phrase: bool, + + /// Add using secure store entry + #[arg( + long, + requires = "entry_name", + conflicts_with = "seed_phrase", + conflicts_with = "secret_key" + )] + pub secure_store: bool, + + /// Name of the secure store entry + #[arg(long, requires = "secure_store")] + pub entry_name: Option, } impl Args { @@ -71,6 +85,17 @@ impl Args { .collect::>() .join(" "), }) + } else if self.secure_store { + let entry_name_with_prefix = format!( + "{}{}-{}", + keyring::SECURE_STORE_ENTRY_PREFIX, + keyring::SECURE_STORE_ENTRY_SERVICE, + self.entry_name.as_ref().unwrap() + ); + + Ok(Secret::SecureStore { + entry_name: entry_name_with_prefix, + }) } else { Err(Error::PasswordRead {}) } From 83bb99be3a43ca06dbe687ba8ce69734cb8724b2 Mon Sep 17 00:00:00 2001 From: Elizabeth Engelman <4752801+elizabethengelman@users.noreply.github.com> Date: Fri, 6 Dec 2024 11:48:11 -0500 Subject: [PATCH 5/7] Clippy --- cmd/soroban-cli/src/config/locator.rs | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/cmd/soroban-cli/src/config/locator.rs b/cmd/soroban-cli/src/config/locator.rs index a0450e5a0..4f1e18309 100644 --- a/cmd/soroban-cli/src/config/locator.rs +++ b/cmd/soroban-cli/src/config/locator.rs @@ -264,20 +264,17 @@ impl Args { pub fn remove_identity(&self, name: &str, global_args: &global::Args) -> Result<(), Error> { let printer = Print::new(global_args.quiet); let identity = self.read_identity(name)?; - match identity { - Secret::SecureStore { entry_name } => { - let entry = StellarEntry::new(&entry_name)?; - match entry.delete_password() { - Ok(_) => {} - Err(e) if e.to_string() == keyring::Error::NoEntry.to_string() => { - printer.infoln("This key was already removed from the secure store. Removing the config file."); - } - Err(e) => { - return Err(Error::Keyring(e)); - } + if let Secret::SecureStore { entry_name } = identity { + let entry = StellarEntry::new(&entry_name)?; + match entry.delete_password() { + Ok(()) => {} + Err(e) if e.to_string() == keyring::Error::NoEntry.to_string() => { + printer.infoln("This key was already removed from the secure store. Removing the cli config file."); + } + Err(e) => { + return Err(Error::Keyring(e)); } } - _ => {} } KeyType::Identity.remove(name, &self.config_dir()?) From 42f3f968e07e7607579a5824200f6db72884993c Mon Sep 17 00:00:00 2001 From: Elizabeth Engelman <4752801+elizabethengelman@users.noreply.github.com> Date: Fri, 6 Dec 2024 11:58:03 -0500 Subject: [PATCH 6/7] Generated docs --- FULL_HELP_DOCS.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/FULL_HELP_DOCS.md b/FULL_HELP_DOCS.md index 70e0d19ff..8f50c1cee 100644 --- a/FULL_HELP_DOCS.md +++ b/FULL_HELP_DOCS.md @@ -958,6 +958,8 @@ Add a new identity (keypair, ledger, OS specific secure store) * `--secret-key` — Add using `secret_key` Can provide with `SOROBAN_SECRET_KEY` * `--seed-phrase` — Add using 12 word seed phrase to generate `secret_key` +* `--secure-store` — Add using secure store entry +* `--entry-name ` — Name of the secure store entry * `--global` — Use global config * `--config-dir ` — Location of config directory, default is "." From bf647a6aee89085af78f201c53f4982aba86244e Mon Sep 17 00:00:00 2001 From: Elizabeth Engelman <4752801+elizabethengelman@users.noreply.github.com> Date: Mon, 9 Dec 2024 17:55:26 -0500 Subject: [PATCH 7/7] Address PR feedback --- cmd/soroban-cli/src/config/locator.rs | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/cmd/soroban-cli/src/config/locator.rs b/cmd/soroban-cli/src/config/locator.rs index 4f1e18309..2f6f36f0e 100644 --- a/cmd/soroban-cli/src/config/locator.rs +++ b/cmd/soroban-cli/src/config/locator.rs @@ -262,18 +262,20 @@ impl Args { } pub fn remove_identity(&self, name: &str, global_args: &global::Args) -> Result<(), Error> { - let printer = Print::new(global_args.quiet); + let print = Print::new(global_args.quiet); let identity = self.read_identity(name)?; if let Secret::SecureStore { entry_name } = identity { let entry = StellarEntry::new(&entry_name)?; match entry.delete_password() { Ok(()) => {} - Err(e) if e.to_string() == keyring::Error::NoEntry.to_string() => { - printer.infoln("This key was already removed from the secure store. Removing the cli config file."); - } - Err(e) => { - return Err(Error::Keyring(e)); - } + Err(e) => match e { + signer::keyring::Error::Keyring(keyring::Error::NoEntry) => { + print.infoln("This key was already removed from the secure store. Removing the cli config file."); + } + _ => { + return Err(Error::Keyring(e)); + } + }, } }