Feat/os keychain followup (#1770)

* Remove secure store entry when removing identity

* Factor out a secure_store mod for code reuse

* Allow for adding a seed phrase key and storing in secure store

---------

Co-authored-by: Willem Wyndham <willem@ahalabs.dev>

FULL_HELP_DOCS.md

@@ -973,7 +973,7 @@ Create and manage identities including keys and addresses
 * `add` — Add a new identity (keypair, ledger, OS specific secure store)
 * `public-key` — Given an identity return its address (public key)
 * `fund` — Fund an identity on a test network
-* `generate` — Generate a new identity using a 24-word seed phrase
+* `generate` — Generate a new identity using a 24-word seed phrase The seed phrase can be stored in a config file (default) or in an OS-specific secure store
 * `ls` — List identities
 * `rm` — Remove an identity
 * `secret` — Output an identity's secret key
@@ -995,6 +995,7 @@ Add a new identity (keypair, ledger, OS specific secure store)
 
 * `--secret-key` — (deprecated) Enter secret (S) key when prompted
 * `--seed-phrase` — (deprecated) Enter key using 12-24 word seed phrase
+* `--secure-store` — Save the new key in secure store. This only supports seed phrases for now
 * `--global` — Use global config
 * `--config-dir <CONFIG_DIR>` — Location of config directory, default is "."
 * `--public-key <PUBLIC_KEY>` — Add a public key, ed25519, or muxed account, e.g. G1.., M2..
@@ -1043,7 +1044,7 @@ Fund an identity on a test network
 
 ## `stellar keys generate`
 
-Generate a new identity using a 24-word seed phrase
+Generate a new identity using a 24-word seed phrase The seed phrase can be stored in a config file (default) or in an OS-specific secure store
 
 **Usage:** `stellar keys generate [OPTIONS] <NAME>`
 
@@ -2166,7 +2167,7 @@ Sign a transaction envelope appending the signature to the envelope
 
 ###### **Options:**
 
-* `--sign-with-key <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_KEY>` — Sign with a local key or key saved in OS 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 <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_URL>` — RPC server endpoint

cmd/soroban-cli/src/commands/keys/add.rs

@@ -1,14 +1,17 @@
+use std::io::Write;
+
 use clap::command;
+use sep5::SeedPhrase;
 
 use crate::{
     commands::global,
     config::{
         address::KeyName,
-        key::{self, Key},
-        locator,
+        key, locator,
         secret::{self, Secret},
     },
     print::Print,
+    signer::secure_store,
 };
 
 #[derive(thiserror::Error, Debug)]
@@ -19,6 +22,15 @@ pub enum Error {
     Key(#[from] key::Error),
     #[error(transparent)]
     Config(#[from] locator::Error),
+
+    #[error(transparent)]
+    SecureStore(#[from] secure_store::Error),
+
+    #[error(transparent)]
+    SeedPhrase(#[from] sep5::error::Error),
+
+    #[error("secret input error")]
+    PasswordRead,
 }
 
 #[derive(Debug, clap::Parser, Clone)]
@@ -40,26 +52,56 @@ pub struct Cmd {
 
 impl Cmd {
     pub fn run(&self, global_args: &global::Args) -> Result<(), Error> {
+        let print = Print::new(global_args.quiet);
         let key = if let Some(key) = self.public_key.as_ref() {
             key.parse()?
         } else {
-            self.secrets.read_secret()?.into()
+            self.read_secret(&print)?.into()
         };
 
-        let print = Print::new(global_args.quiet);
         let path = self.config_locator.write_key(&self.name, &key)?;
 
-        if let Key::Secret(Secret::SeedPhrase { seed_phrase }) = key {
-            if seed_phrase.split_whitespace().count() < 24 {
+        print.checkln(format!("Key saved with alias {:?} in {path:?}", self.name));
+
+        Ok(())
+    }
+
+    fn read_secret(&self, print: &Print) -> Result<Secret, Error> {
+        if let Ok(secret_key) = std::env::var("SOROBAN_SECRET_KEY") {
+            Ok(Secret::SecretKey { secret_key })
+        } else if self.secrets.secure_store {
+            let prompt = "Type a 12/24 word seed phrase:";
+            let secret_key = read_password(print, prompt)?;
+            if secret_key.split_whitespace().count() < 24 {
                 print.warnln("The provided seed phrase lacks sufficient entropy and should be avoided. Using a 24-word seed phrase is a safer option.".to_string());
                 print.warnln(
                     "To generate a new key, use the `stellar keys generate` command.".to_string(),
                 );
             }
-        }
 
-        print.checkln(format!("Key saved with alias {:?} in {path:?}", self.name));
+            let seed_phrase: SeedPhrase = secret_key.parse()?;
 
-        Ok(())
+            Ok(secure_store::save_secret(print, &self.name, seed_phrase)?)
+        } else {
+            let prompt = "Type a secret key or 12/24 word seed phrase:";
+            let secret_key = read_password(print, prompt)?;
+            let secret = secret_key.parse()?;
+            if let Secret::SeedPhrase { seed_phrase } = &secret {
+                if seed_phrase.split_whitespace().count() < 24 {
+                    print.warnln("The provided seed phrase lacks sufficient entropy and should be avoided. Using a 24-word seed phrase is a safer option.".to_string());
+                    print.warnln(
+                        "To generate a new key, use the `stellar keys generate` command."
+                            .to_string(),
+                    );
+                }
+            }
+            Ok(secret)
+        }
     }
 }
+
+fn read_password(print: &Print, prompt: &str) -> Result<String, Error> {
+    print.arrowln(prompt);
+    std::io::stdout().flush().map_err(|_| Error::PasswordRead)?;
+    rpassword::read_password().map_err(|_| Error::PasswordRead)
+}

cmd/soroban-cli/src/commands/keys/generate.rs

@@ -6,12 +6,7 @@ use super::super::config::{
     secret::{self, Secret},
 };
 
-use crate::{
-    commands::global,
-    config::address::KeyName,
-    print::Print,
-    signer::keyring::{self, StellarEntry},
-};
+use crate::{commands::global, config::address::KeyName, print::Print, signer::secure_store};
 
 #[derive(thiserror::Error, Debug)]
 pub enum Error {
@@ -28,7 +23,7 @@ pub enum Error {
     IdentityAlreadyExists(String),
 
     #[error(transparent)]
-    Keyring(#[from] keyring::Error),
+    SecureStore(#[from] secure_store::Error),
 }
 
 #[derive(Debug, clap::Parser, Clone)]
@@ -125,29 +120,13 @@ impl Cmd {
     fn secret(&self, print: &Print) -> Result<Secret, Error> {
         let seed_phrase = self.seed_phrase()?;
         if self.secure_store {
-            // secure_store:org.stellar.cli:<key name>
-            let entry_name_with_prefix = format!(
-                "{}{}-{}",
-                keyring::SECURE_STORE_ENTRY_PREFIX,
-                keyring::SECURE_STORE_ENTRY_SERVICE,
-                self.name
-            );
-
-            //checking that the entry name is valid before writing to the secure store
-            let secret: Secret = entry_name_with_prefix.parse()?;
-
-            if let Secret::SecureStore { entry_name } = &secret {
-                Self::write_to_secure_store(entry_name, seed_phrase, print)?;
-            }
-
-            return Ok(secret);
-        }
-        let secret: Secret = seed_phrase.into();
-        Ok(if self.as_secret {
-            secret.private_key(self.hd_path)?.into()
+            Ok(secure_store::save_secret(print, &self.name, seed_phrase)?)
+        } else if self.as_secret {
+            let secret: Secret = seed_phrase.into();
+            Ok(secret.private_key(self.hd_path)?.into())
         } else {
-            secret
-        })
+            Ok(seed_phrase.into())
+        }
     }
 
     fn seed_phrase(&self) -> Result<SeedPhrase, Error> {
@@ -157,24 +136,6 @@ impl Cmd {
             secret::seed_phrase_from_seed(self.seed.as_deref())
         }?)
     }
-
-    fn write_to_secure_store(
-        entry_name: &String,
-        seed_phrase: SeedPhrase,
-        print: &Print,
-    ) -> Result<(), Error> {
-        print.infoln(format!("Writing to secure store: {entry_name}"));
-        let entry = StellarEntry::new(entry_name)?;
-        if let Ok(key) = entry.get_public_key(None) {
-            print.warnln(format!("A key for {entry_name} already exists in your operating system's secure store: {key}"));
-        } else {
-            print.infoln(format!(
-                "Saving a new key to your operating system's secure store: {entry_name}"
-            ));
-            entry.set_seed_phrase(seed_phrase)?;
-        }
-        Ok(())
-    }
 }
 
 #[cfg(test)]

cmd/soroban-cli/src/commands/keys/mod.rs

@@ -23,6 +23,7 @@ pub enum Cmd {
     Fund(fund::Cmd),
 
     /// Generate a new identity using a 24-word seed phrase
+    /// The seed phrase can be stored in a config file (default) or in an OS-specific secure store.
     Generate(generate::Cmd),
 
     /// List identities
@@ -76,7 +77,7 @@ impl Cmd {
             Cmd::Fund(cmd) => cmd.run(global_args).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::Secret(cmd) => cmd.run()?,
             Cmd::Default(cmd) => cmd.run(global_args)?,
         };

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)?)
     }
 }

cmd/soroban-cli/src/config/locator.rs

@@ -12,7 +12,13 @@ use std::{
 };
 use stellar_strkey::{Contract, DecodeError};
 
-use crate::{commands::HEADING_GLOBAL, utils::find_config_dir, xdr, Pwd};
+use crate::{
+    commands::{global, HEADING_GLOBAL},
+    print::Print,
+    signer::{self, keyring::StellarEntry},
+    utils::find_config_dir,
+    xdr, Pwd,
+};
 
 use super::{
     alias,
@@ -88,6 +94,8 @@ pub enum Error {
     ContractAliasCannotOverlapWithKey(String),
     #[error("Key cannot {0} cannot overlap with contract alias")]
     KeyCannotOverlapWithContractAlias(String),
+    #[error(transparent)]
+    Keyring(#[from] signer::keyring::Error),
     #[error("Only private keys and seed phrases are supported for getting private keys {0}")]
     SecretKeyOnly(String),
     #[error(transparent)]
@@ -288,7 +296,25 @@ 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 print = Print::new(global_args.quiet);
+        let identity = self.read_identity(name)?;
+
+        if let Key::Secret(Secret::SecureStore { entry_name }) = identity {
+            let entry = StellarEntry::new(&entry_name)?;
+            match entry.delete_seed_phrase() {
+                Ok(()) => {}
+                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));
+                    }
+                },
+            }
+        }
+
         KeyType::Identity.remove(name, &self.config_dir()?)
     }
 

cmd/soroban-cli/src/config/secret.rs

@@ -1,6 +1,6 @@
 use clap::arg;
 use serde::{Deserialize, Serialize};
-use std::{io::Write, str::FromStr};
+use std::str::FromStr;
 
 use sep5::SeedPhrase;
 use stellar_strkey::ed25519::{PrivateKey, PublicKey};
@@ -15,10 +15,6 @@ use super::key::Key;
 
 #[derive(thiserror::Error, Debug)]
 pub enum Error {
-    // #[error("seed_phrase must be 12 words long, found {len}")]
-    // InvalidSeedPhrase { len: usize },
-    #[error("secret input error")]
-    PasswordRead,
     #[error(transparent)]
     Secret(#[from] stellar_strkey::DecodeError),
     #[error(transparent)]
@@ -44,20 +40,9 @@ pub struct Args {
     /// (deprecated) Enter key using 12-24 word seed phrase
     #[arg(long)]
     pub seed_phrase: bool,
-}
-
-impl Args {
-    pub fn read_secret(&self) -> Result<Secret, Error> {
-        if let Ok(secret_key) = std::env::var("SOROBAN_SECRET_KEY") {
-            Ok(Secret::SecretKey { secret_key })
-        } else {
-            println!("Type a secret key or 12/24 word seed phrase:");
-            let secret_key = read_password()?;
-            secret_key
-                .parse()
-                .map_err(|_| Error::InvalidSecretOrSeedPhrase)
-        }
-    }
+    /// Save the new key in secure store. This only supports seed phrases for now.
+    #[arg(long)]
+    pub secure_store: bool,
 }
 
 #[derive(Clone, Debug, Serialize, Deserialize, PartialEq, Eq)]
@@ -179,11 +164,6 @@ pub fn test_seed_phrase() -> Result<SeedPhrase, Error> {
     Ok("0000000000000000".parse()?)
 }
 
-fn read_password() -> Result<String, Error> {
-    std::io::stdout().flush().map_err(|_| Error::PasswordRead)?;
-    rpassword::read_password().map_err(|_| Error::PasswordRead)
-}
-
 #[cfg(test)]
 mod tests {
     use super::*;

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 key saved in OS 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<String>,
 

cmd/soroban-cli/src/print.rs

@@ -106,5 +106,6 @@ create_print_functions!(save, saveln, "💾");
 create_print_functions!(search, searchln, "🔎");
 create_print_functions!(warn, warnln, "⚠️");
 create_print_functions!(exclaim, exclaimln, "❗️");
+create_print_functions!(arrow, arrowln, "➡️");
 create_print_functions!(log, logln, "📔");
 create_print_functions!(event, eventln, "📅");

cmd/soroban-cli/src/signer.rs

@@ -13,6 +13,7 @@ use crate::xdr::{
 use crate::{config::network::Network, print::Print, utils::transaction_hash};
 
 pub mod keyring;
+pub mod secure_store;
 
 #[derive(thiserror::Error, Debug)]
 pub enum Error {