diff --git a/FULL_HELP_DOCS.md b/FULL_HELP_DOCS.md index 0923939e3..81ed87d4c 100644 --- a/FULL_HELP_DOCS.md +++ b/FULL_HELP_DOCS.md @@ -1137,7 +1137,7 @@ Add a new identity (keypair, ledger, OS specific secure store) This only supports seed phrases for now. - `--public-key ` — Add a public key, ed25519, or muxed account, e.g. G1.., M2.. -- `--overwrite` — Overwrite existing identity if it already exists +- `--overwrite` — Overwrite existing identity if it already exists. When combined with --secure-store, also replaces the existing Secure Store entry ###### **Options (Global):** @@ -1211,7 +1211,7 @@ Generate a new identity using a 24-word seed phrase The seed phrase can be store Default value: `false` -- `--overwrite` — Overwrite existing identity if it already exists +- `--overwrite` — Overwrite existing identity if it already exists. When combined with --secure-store, also replaces the existing Secure Store entry ###### **Options (Global):** diff --git a/cmd/crates/soroban-test/tests/it/integration/secure_store.rs b/cmd/crates/soroban-test/tests/it/integration/secure_store.rs index 5bd6a7611..ac58eba70 100644 --- a/cmd/crates/soroban-test/tests/it/integration/secure_store.rs +++ b/cmd/crates/soroban-test/tests/it/integration/secure_store.rs @@ -74,4 +74,28 @@ async fn secure_store_key_management() { let new_account = client.get_account(&new_address).await.unwrap(); assert_eq!(new_account.balance, starting_balance); + + // generating the same key again without --overwrite must fail + sandbox + .new_assert_cmd("keys") + .args(["generate", secure_key_name, "--secure-store"]) + .assert() + .failure() + .stderr(predicate::str::contains("already exists")); + + // generating the same key again with --overwrite must succeed and replace the entry + sandbox + .new_assert_cmd("keys") + .args(["generate", secure_key_name, "--secure-store", "--overwrite"]) + .assert() + .success(); + + // the address should still be a valid public key (new key was written) + let new_secure_store_address = sandbox + .new_assert_cmd("keys") + .args(["address", secure_key_name]) + .assert() + .success() + .stdout_as_str(); + assert!(new_secure_store_address.starts_with('G')); } diff --git a/cmd/soroban-cli/src/commands/keys/add.rs b/cmd/soroban-cli/src/commands/keys/add.rs index 64aa4271b..5e2593f00 100644 --- a/cmd/soroban-cli/src/commands/keys/add.rs +++ b/cmd/soroban-cli/src/commands/keys/add.rs @@ -57,7 +57,8 @@ pub struct Cmd { #[arg(long, conflicts_with = "seed_phrase", conflicts_with = "secret_key")] pub public_key: Option, - /// Overwrite existing identity if it already exists. + /// Overwrite existing identity if it already exists. When combined with + /// --secure-store, also replaces the existing Secure Store entry. #[arg(long)] pub overwrite: bool, } @@ -108,7 +109,8 @@ impl Cmd { let seed_phrase: SeedPhrase = secret_key.parse()?; - let secret = secure_store::save_secret(print, &self.name, &seed_phrase)?; + let secret = + secure_store::save_secret(print, &self.name, &seed_phrase, self.overwrite)?; Ok(secret.parse()?) } else { let prompt = "Type a secret key or 12/24 word seed phrase:"; diff --git a/cmd/soroban-cli/src/commands/keys/generate.rs b/cmd/soroban-cli/src/commands/keys/generate.rs index bfc450aa5..0cc0441e1 100644 --- a/cmd/soroban-cli/src/commands/keys/generate.rs +++ b/cmd/soroban-cli/src/commands/keys/generate.rs @@ -61,7 +61,8 @@ pub struct Cmd { #[arg(long, default_value = "false")] pub fund: bool, - /// Overwrite existing identity if it already exists. + /// Overwrite existing identity if it already exists. When combined with + /// --secure-store, also replaces the existing Secure Store entry. #[arg(long)] pub overwrite: bool, } @@ -115,7 +116,8 @@ impl Cmd { fn secret(&self, print: &Print) -> Result { let seed_phrase = self.seed_phrase()?; if self.secure_store { - let secret = secure_store::save_secret(print, &self.name, &seed_phrase)?; + let secret = + secure_store::save_secret(print, &self.name, &seed_phrase, self.overwrite)?; Ok(secret.parse()?) } else if self.as_secret { let secret: Secret = seed_phrase.into(); diff --git a/cmd/soroban-cli/src/signer/keyring.rs b/cmd/soroban-cli/src/signer/keyring.rs index 22b3d6884..229a16a86 100644 --- a/cmd/soroban-cli/src/signer/keyring.rs +++ b/cmd/soroban-cli/src/signer/keyring.rs @@ -15,6 +15,9 @@ pub enum Error { #[error("Secure Store keys are not allowed: additional-libs feature must be enabled")] FeatureNotEnabled, + + #[error("An entry for '{0}' already exists in the secure store")] + EntryAlreadyExists(String), } pub struct StellarEntry { @@ -31,19 +34,32 @@ impl StellarEntry { }) } - pub fn write(&self, seed_phrase: SeedPhrase, print: &Print) -> Result<(), Error> { - if let Ok(key) = self.get_public_key(None) { + pub fn write( + &self, + seed_phrase: SeedPhrase, + print: &Print, + overwrite: bool, + ) -> Result<(), Error> { + let exists = match self.keyring.get_password() { + Ok(_) => true, + Err(keyring::Error::NoEntry) => false, + Err(e) => return Err(Error::Keyring(e)), + }; + if exists { + if !overwrite { + return Err(Error::EntryAlreadyExists(self.name.clone())); + } print.warnln(format!( - "A key for {0} already exists in your operating system's secure store: {1}", - self.name, key + "Overwriting existing key in secure store: {0}", + self.name )); } else { print.infoln(format!( "Saving a new key to your operating system's secure store: {0}", self.name )); - self.set_seed_phrase(seed_phrase)?; } + self.set_seed_phrase(seed_phrase)?; Ok(()) } @@ -184,6 +200,58 @@ mod test { assert!(sign_tx_env_result.is_ok()); } + #[test] + fn write_with_overwrite_updates_existing_entry() { + set_default_credential_builder(mock::default_credential_builder()); + + let seed_phrase_1 = + crate::config::secret::seed_phrase_from_seed(Some("0123456789abcdef")).unwrap(); + let seed_phrase_2 = + crate::config::secret::seed_phrase_from_seed(Some("fedcba9876543210")).unwrap(); + + let pubkey_1 = seed_phrase_1.from_path_index(0, None).unwrap().public().0; + let pubkey_2 = seed_phrase_2.from_path_index(0, None).unwrap().public().0; + assert_ne!(pubkey_1, pubkey_2, "test seeds must produce different keys"); + + let entry = StellarEntry::new("test-overwrite").unwrap(); + let print = print::Print::new(true); + + entry.write(seed_phrase_1, &print, false).unwrap(); + assert_eq!(entry.get_public_key(None).unwrap().0, pubkey_1); + + // overwrite=true must replace the entry with the new seed phrase + entry.write(seed_phrase_2, &print, true).unwrap(); + assert_eq!( + entry.get_public_key(None).unwrap().0, + pubkey_2, + "overwrite should have replaced the keyring entry" + ); + } + + #[test] + fn write_without_overwrite_errors_on_existing_entry() { + set_default_credential_builder(mock::default_credential_builder()); + + let seed_phrase_1 = + crate::config::secret::seed_phrase_from_seed(Some("0123456789abcdef")).unwrap(); + let seed_phrase_2 = + crate::config::secret::seed_phrase_from_seed(Some("fedcba9876543210")).unwrap(); + + let entry = StellarEntry::new("test-no-overwrite").unwrap(); + let print = print::Print::new(true); + + entry.write(seed_phrase_1, &print, false).unwrap(); + + // overwrite=false must fail with EntryAlreadyExists when an entry already exists + let err = entry + .write(seed_phrase_2, &print, false) + .expect_err("write without overwrite should fail on existing entry"); + assert!( + matches!(err, Error::EntryAlreadyExists(_)), + "expected EntryAlreadyExists, got {err:?}" + ); + } + #[test] fn test_delete_seed_phrase() { set_default_credential_builder(mock::default_credential_builder()); diff --git a/cmd/soroban-cli/src/signer/secure_store.rs b/cmd/soroban-cli/src/signer/secure_store.rs index d30cf3b50..9951b4833 100644 --- a/cmd/soroban-cli/src/signer/secure_store.rs +++ b/cmd/soroban-cli/src/signer/secure_store.rs @@ -45,12 +45,13 @@ mod secure_store_impl { print: &Print, entry_name: &str, seed_phrase: &SeedPhrase, + overwrite: bool, ) -> Result { // secure_store:org.stellar.cli: let entry_name_with_prefix = format!("{ENTRY_PREFIX}{ENTRY_SERVICE}-{entry_name}"); let entry = StellarEntry::new(&entry_name_with_prefix)?; - entry.write(seed_phrase.clone(), print)?; + entry.write(seed_phrase.clone(), print, overwrite)?; Ok(entry_name_with_prefix) } @@ -81,6 +82,7 @@ mod secure_store_impl { _print: &Print, _entry_name: &str, _seed_phrase: &SeedPhrase, + _overwrite: bool, ) -> Result { Err(Error::FeatureNotEnabled) }