From 441ef754c02f587b624b7eb80fea5bfd6675636d 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/8] 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 1b97198558498a321c3636e92514b5fb39be74dd 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/8] 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 d1c207f433266058fa79a8bb825c2eddc0126009 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/8] 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 080ec3aab3e6a7f1e2488c036bd56ef823203805 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/8] 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 6f24cded57803a8311d4c860e3e8a3eda787bca4 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/8] 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 b1a125746331aa0cfd3eb83e1f0769e4629633e5 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/8] 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 829447d1f..74b648643 100644 --- a/FULL_HELP_DOCS.md +++ b/FULL_HELP_DOCS.md @@ -959,6 +959,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 61849ea0e0aaddc1420bb3b0233bfe6f3fbf4a8b 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/8] 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)); + } + }, } } From 3acc3d0a3885cfc02ed63b209d11cfc6033f9792 Mon Sep 17 00:00:00 2001 From: Elizabeth Engelman <4752801+elizabethengelman@users.noreply.github.com> Date: Thu, 12 Dec 2024 15:19:12 -0500 Subject: [PATCH 8/8] Update doc comments for secure store --- FULL_HELP_DOCS.md | 10 ++++++---- cmd/soroban-cli/src/commands/keys/mod.rs | 4 +++- cmd/soroban-cli/src/config/secret.rs | 4 ++-- cmd/soroban-cli/src/config/sign_with.rs | 2 +- 4 files changed, 12 insertions(+), 8 deletions(-) diff --git a/FULL_HELP_DOCS.md b/FULL_HELP_DOCS.md index 74b648643..16ed660f9 100644 --- a/FULL_HELP_DOCS.md +++ b/FULL_HELP_DOCS.md @@ -959,8 +959,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 +* `--secure-store` — Add using a key saved in a secure store entry. Requires the entry name to be provided with `--entry_name` +* `--entry-name ` — Name of the secure store entry, to be used with `--secure_store` * `--global` — Use global config * `--config-dir ` — Location of config directory, default is "." @@ -1008,7 +1008,9 @@ Fund an identity on a test network ## `stellar keys generate` -Generate a new identity with a seed phrase, currently 12 words +Generate a new identity with a seed phrase, currently 12 words. + +The identity's secret can be stored in a config file (default), in an OS-specific secure store, or be printed out to the console. **Usage:** `stellar keys generate [OPTIONS] ` @@ -2016,7 +2018,7 @@ Sign a transaction envelope appending the signature to the envelope ###### **Options:** -* `--sign-with-key ` — Sign with a local key. Can be an identity (--sign-with-key alice), a secret key (--sign-with-key SC36…), or a seed phrase (--sign-with-key "kite urban…"). If using seed phrase, `--hd-path` defaults to the `0` path +* `--sign-with-key ` — Sign with a local key or a key saved in OS's secure storage. Can be an identity (--sign-with-key alice), a secret key (--sign-with-key SC36…), or a seed phrase (--sign-with-key "kite urban…"). If using seed phrase, `--hd-path` defaults to the `0` path * `--hd-path ` — If using a seed phrase to sign, sets which hierarchical deterministic path to use, e.g. `m/44'/148'/{hd_path}`. Example: `--hd-path 1`. Default: `0` * `--sign-with-lab` — Sign with https://lab.stellar.org * `--rpc-url ` — RPC server endpoint diff --git a/cmd/soroban-cli/src/commands/keys/mod.rs b/cmd/soroban-cli/src/commands/keys/mod.rs index 4d44c081f..ab0884a1b 100644 --- a/cmd/soroban-cli/src/commands/keys/mod.rs +++ b/cmd/soroban-cli/src/commands/keys/mod.rs @@ -21,7 +21,9 @@ pub enum Cmd { /// Fund an identity on a test network Fund(fund::Cmd), - /// Generate a new identity with a seed phrase, currently 12 words + /// Generate a new identity with a seed phrase, currently 12 words. + /// + /// The identity's secret can be stored in a config file (default), in an OS-specific secure store, or be printed out to the console. Generate(generate::Cmd), /// List identities diff --git a/cmd/soroban-cli/src/config/secret.rs b/cmd/soroban-cli/src/config/secret.rs index 8f0ee8673..1e89097ad 100644 --- a/cmd/soroban-cli/src/config/secret.rs +++ b/cmd/soroban-cli/src/config/secret.rs @@ -45,7 +45,7 @@ pub struct Args { #[arg(long, conflicts_with = "secret_key", conflicts_with = "secure_store")] pub seed_phrase: bool, - /// Add using secure store entry + /// Add using a key saved in a secure store entry. Requires the entry name to be provided with `--entry_name` #[arg( long, requires = "entry_name", @@ -54,7 +54,7 @@ pub struct Args { )] pub secure_store: bool, - /// Name of the secure store entry + /// Name of the secure store entry, to be used with `--secure_store` #[arg(long, requires = "secure_store")] pub entry_name: Option, } diff --git a/cmd/soroban-cli/src/config/sign_with.rs b/cmd/soroban-cli/src/config/sign_with.rs index 475013bc8..40512fc48 100644 --- a/cmd/soroban-cli/src/config/sign_with.rs +++ b/cmd/soroban-cli/src/config/sign_with.rs @@ -34,7 +34,7 @@ pub enum Error { #[derive(Debug, clap::Args, Clone, Default)] #[group(skip)] pub struct Args { - /// Sign with a local key. Can be an identity (--sign-with-key alice), a secret key (--sign-with-key SC36…), or a seed phrase (--sign-with-key "kite urban…"). If using seed phrase, `--hd-path` defaults to the `0` path. + /// Sign with a local key or a key saved in OS's secure storage. Can be an identity (--sign-with-key alice), a secret key (--sign-with-key SC36…), or a seed phrase (--sign-with-key "kite urban…"). If using seed phrase, `--hd-path` defaults to the `0` path. #[arg(long, env = "STELLAR_SIGN_WITH_KEY")] pub sign_with_key: Option,