Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat/os keychain followup #1770

Open
wants to merge 20 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 51 additions & 9 deletions cmd/soroban-cli/src/commands/keys/add.rs
Original file line number Diff line number Diff line change
@@ -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)]
Expand All @@ -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)]
Expand All @@ -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> {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved this here from config/secret.rs - i was seeing an interesting cyclical dependency, and moving this here helped that be a bit less confusing.

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()?;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the moment I only added support for a user to add a new seed phrase to secure storage. I'm happy to make a change so that this supports adding a private key directly as well, but wanted to get some more eyes on this to see if folks thought it was worth the effort.


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)
}
55 changes: 8 additions & 47 deletions cmd/soroban-cli/src/commands/keys/generate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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)]
Expand Down Expand Up @@ -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> {
Expand All @@ -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(())
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was all moved to a secure_store mod

}

#[cfg(test)]
Expand Down
3 changes: 2 additions & 1 deletion cmd/soroban-cli/src/commands/keys/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)?,
};
Expand Down
6 changes: 4 additions & 2 deletions cmd/soroban-cli/src/commands/keys/rm.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
use clap::command;

use crate::commands::global;

use super::super::config::locator;

#[derive(thiserror::Error, Debug)]
Expand All @@ -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)?)
}
}
30 changes: 28 additions & 2 deletions cmd/soroban-cli/src/config/locator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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)]
Expand Down Expand Up @@ -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()?)
}

Expand Down
28 changes: 4 additions & 24 deletions cmd/soroban-cli/src/config/secret.rs
Original file line number Diff line number Diff line change
@@ -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};
Expand All @@ -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)]
Expand All @@ -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)]
Expand Down Expand Up @@ -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::*;
Expand Down
2 changes: 1 addition & 1 deletion cmd/soroban-cli/src/config/sign_with.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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>,

Expand Down
1 change: 1 addition & 0 deletions cmd/soroban-cli/src/print.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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, "📅");
1 change: 1 addition & 0 deletions cmd/soroban-cli/src/signer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
32 changes: 32 additions & 0 deletions cmd/soroban-cli/src/signer/keyring.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,10 @@ impl StellarEntry {
Ok(())
}

pub fn delete_seed_phrase(&self) -> Result<(), Error> {
Ok(self.keyring.delete_credential()?)
}

fn get_seed_phrase(&self) -> Result<SeedPhrase, Error> {
Ok(self.keyring.get_password()?.parse()?)
}
Expand Down Expand Up @@ -144,4 +148,32 @@ mod test {
let sign_tx_env_result = entry.sign_data(tx_xdr.as_bytes(), None);
assert!(sign_tx_env_result.is_ok());
}

#[test]
fn test_delete_seed_phrase() {
set_default_credential_builder(mock::default_credential_builder());

//create a seed phrase
let seed_phrase = crate::config::secret::seed_phrase_from_seed(None).unwrap();

// create a keyring entry and set the seed_phrase
let entry = StellarEntry::new("test").unwrap();
entry.set_seed_phrase(seed_phrase).unwrap();

// assert it is there
let get_seed_phrase_result = entry.get_seed_phrase();
assert!(get_seed_phrase_result.is_ok());

// delete the password
let delete_seed_phrase_result = entry.delete_seed_phrase();
assert!(delete_seed_phrase_result.is_ok());

// confirm the entry is gone
let get_password_result = entry.get_seed_phrase();
assert!(get_password_result.is_err());
assert!(matches!(
get_password_result.unwrap_err(),
Error::Keyring(_)
));
}
}
Loading
Loading