diff --git a/FULL_HELP_DOCS.md b/FULL_HELP_DOCS.md index 08b33aeeb..376a3d8e8 100644 --- a/FULL_HELP_DOCS.md +++ b/FULL_HELP_DOCS.md @@ -1138,7 +1138,7 @@ Add a new identity (keypair, ledger, OS specific secure store) - `--public-key ` — Add a public key, ed25519, or muxed account, e.g. G1.., M2.. - `--overwrite` — Overwrite existing identity if it already exists. When combined with --secure-store, also replaces the existing Secure Store entry -- `--hd-path ` — When importing a seed phrase into the Secure Store, which `hd_path` to derive the key at +- `--hd-path ` — When importing a seed phrase, which `hd_path` to derive the key at. Persisted on the identity (or its Secure Store entry) so later commands derive the same account without re-passing the flag. Not valid with `--public-key` or a raw secret key ###### **Options (Global):** @@ -1207,7 +1207,7 @@ Generate a new identity using a 24-word seed phrase The seed phrase can be store On Mac this uses Keychain, on Windows it is Secure Store Service, and on \*nix platforms it uses a combination of the kernel keyutils and DBus-based Secret Service. -- `--hd-path ` — With `--as-secret` or `--secure-store`, which `hd_path` to derive the key at from the seed phrase +- `--hd-path ` — Which `hd_path` to derive the key at from the seed phrase. Honored across all storage modes: with `--as-secret` it picks which derived key is stored, with `--secure-store` or plain seed-phrase storage it is persisted on the identity so later commands derive the same account without re-passing the flag - `--fund` — Fund generated key pair Default value: `false` diff --git a/cmd/crates/soroban-test/tests/it/integration/hello_world.rs b/cmd/crates/soroban-test/tests/it/integration/hello_world.rs index 3baffe62e..89ea50aeb 100644 --- a/cmd/crates/soroban-test/tests/it/integration/hello_world.rs +++ b/cmd/crates/soroban-test/tests/it/integration/hello_world.rs @@ -71,7 +71,7 @@ async fn invoke_contract() { let dir = sandbox.config_dir(); let seed_phrase = std::fs::read_to_string(dir.join("identity/test.toml")).unwrap(); let s = toml::from_str::(&seed_phrase).unwrap(); - let secret::Secret::SeedPhrase { seed_phrase } = s else { + let secret::Secret::SeedPhrase { seed_phrase, .. } = s else { panic!("Expected seed phrase") }; diff --git a/cmd/crates/soroban-test/tests/it/integration/keys.rs b/cmd/crates/soroban-test/tests/it/integration/keys.rs index f9018dd0e..9ca365675 100644 --- a/cmd/crates/soroban-test/tests/it/integration/keys.rs +++ b/cmd/crates/soroban-test/tests/it/integration/keys.rs @@ -351,6 +351,79 @@ async fn rm_with_force_skips_confirmation() { .failure(); } +// `keys generate --hd-path N` (plain seed-phrase storage) must persist N so +// that later `keys address` calls without `--hd-path` derive at index N rather +// than the default. Guards the user-visible contract from #2538 across CLI +// parsing, identity-file I/O, and key derivation. +#[tokio::test] +async fn hd_path_persists_for_keys_generate() { + let sandbox = &TestEnv::new(); + sandbox + .new_assert_cmd("keys") + .args(["generate", "hd-gen", "--hd-path", "5"]) + .assert() + .success(); + + let address_default = pubkey_for_identity(sandbox, "hd-gen"); + let address_explicit = sandbox + .new_assert_cmd("keys") + .args(["address", "hd-gen", "--hd-path", "5"]) + .assert() + .success() + .stdout_as_str(); + let address_zero = sandbox + .new_assert_cmd("keys") + .args(["address", "hd-gen", "--hd-path", "0"]) + .assert() + .success() + .stdout_as_str(); + + assert_eq!( + address_default, address_explicit, + "expected `keys address hd-gen` (no flag) to derive at the persisted hd_path 5" + ); + assert_ne!( + address_default, address_zero, + "expected hd_path 5 derivation to differ from hd_path 0" + ); +} + +#[tokio::test] +async fn hd_path_persists_for_keys_add_seed_phrase() { + let sandbox = &TestEnv::new(); + let seed_phrase = "aisle reflect depart add safe fury dress artist bronze abuse warrior clap inquiry ask mandate deputy view trade debate flip priority boy depart recipe"; + + sandbox + .new_assert_cmd("keys") + .write_stdin(format!("{seed_phrase}\n")) + .args(["add", "hd-add", "--hd-path", "5"]) + .assert() + .success(); + + let address_default = pubkey_for_identity(sandbox, "hd-add"); + let address_explicit = sandbox + .new_assert_cmd("keys") + .args(["address", "hd-add", "--hd-path", "5"]) + .assert() + .success() + .stdout_as_str(); + let address_zero = sandbox + .new_assert_cmd("keys") + .args(["address", "hd-add", "--hd-path", "0"]) + .assert() + .success() + .stdout_as_str(); + + assert_eq!( + address_default, address_explicit, + "expected `keys address hd-add` (no flag) to derive at the persisted hd_path 5" + ); + assert_ne!( + address_default, address_zero, + "expected hd_path 5 derivation to differ from hd_path 0" + ); +} + #[tokio::test] async fn rm_nonexistent_key() { let sandbox = &TestEnv::new(); diff --git a/cmd/crates/soroban-test/tests/it/util.rs b/cmd/crates/soroban-test/tests/it/util.rs index f33728e86..719ccb5a4 100644 --- a/cmd/crates/soroban-test/tests/it/util.rs +++ b/cmd/crates/soroban-test/tests/it/util.rs @@ -18,6 +18,7 @@ pub fn add_key(dir: &Path, name: &str, kind: SecretKind, data: &str) { let secret = match kind { SecretKind::Seed => Secret::SeedPhrase { seed_phrase: data.to_string(), + hd_path: None, }, SecretKind::Key => Secret::SecretKey { secret_key: data.to_string(), diff --git a/cmd/soroban-cli/src/commands/keys/add.rs b/cmd/soroban-cli/src/commands/keys/add.rs index 4bf205ef4..b79cb2a5e 100644 --- a/cmd/soroban-cli/src/commands/keys/add.rs +++ b/cmd/soroban-cli/src/commands/keys/add.rs @@ -39,6 +39,12 @@ pub enum Error { unset STELLAR_SECRET_KEY or provide a seed phrase instead" )] SecureStoreRequiresSeedPhrase, + + #[error("--hd-path is not valid with a secret key; secret keys cannot be derived")] + HdPathNotSupportedForSecretKey, + + #[error("--hd-path is not valid with --public-key; public keys cannot be derived")] + HdPathNotSupportedForPublicKey, } #[derive(Debug, clap::Parser, Clone)] @@ -62,7 +68,9 @@ pub struct Cmd { #[arg(long)] pub overwrite: bool, - /// When importing a seed phrase into the Secure Store, which `hd_path` to derive the key at. + /// When importing a seed phrase, which `hd_path` to derive the key at. Persisted on + /// the identity (or its Secure Store entry) so later commands derive the same account + /// without re-passing the flag. Not valid with `--public-key` or a raw secret key. #[arg(long)] pub hd_path: Option, } @@ -71,6 +79,10 @@ impl Cmd { pub fn run(&self, global_args: &global::Args) -> Result<(), Error> { let print = Print::new(global_args.quiet); + if self.public_key.is_some() && self.hd_path.is_some() { + return Err(Error::HdPathNotSupportedForPublicKey); + } + if self.config_locator.read_identity(&self.name).is_ok() { if !self.overwrite { return Err(Error::IdentityAlreadyExists(self.name.to_string())); @@ -98,7 +110,7 @@ impl Cmd { return Err(Error::SecureStoreRequiresSeedPhrase); } } else if let Ok(secret_key) = std::env::var("STELLAR_SECRET_KEY") { - return Ok(secret_key.parse()?); + return build_secret(&secret_key, self.hd_path); } if self.secrets.secure_store { @@ -123,8 +135,8 @@ impl Cmd { } 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 { + let secret = build_secret(&secret_key, self.hd_path)?; + 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( @@ -138,6 +150,18 @@ impl Cmd { } } +fn build_secret(input: &str, hd_path: Option) -> Result { + let secret: Secret = input.parse()?; + match (secret, hd_path) { + (Secret::SecretKey { .. }, Some(_)) => Err(Error::HdPathNotSupportedForSecretKey), + (Secret::SeedPhrase { seed_phrase, .. }, hd_path) => Ok(Secret::SeedPhrase { + seed_phrase, + hd_path, + }), + (secret, _) => Ok(secret), + } +} + fn read_password(print: &Print, prompt: &str) -> Result { if std::io::stdin().is_terminal() { // Interactive: prompt and read from TTY @@ -190,6 +214,16 @@ mod tests { (temp_dir, locator, cmd) } + fn cmd_with_public_key( + public_key: &str, + hd_path: Option, + ) -> (tempfile::TempDir, locator::Args, Cmd) { + let (temp_dir, locator, mut cmd) = set_up_test(); + cmd.public_key = Some(public_key.to_string()); + cmd.hd_path = hd_path; + (temp_dir, locator, cmd) + } + fn global_args() -> global::Args { global::Args { quiet: true, @@ -197,6 +231,55 @@ mod tests { } } + #[test] + fn test_build_secret_persists_hd_path_on_seed_phrase() { + let secret = build_secret(SEED_PHRASE, Some(5)).unwrap(); + match secret { + Secret::SeedPhrase { + seed_phrase, + hd_path, + } => { + assert_eq!(seed_phrase, SEED_PHRASE); + assert_eq!(hd_path, Some(5)); + } + other => panic!("expected SeedPhrase variant, got {other:?}"), + } + } + + #[test] + fn test_build_secret_seed_phrase_without_hd_path() { + let secret = build_secret(SEED_PHRASE, None).unwrap(); + match secret { + Secret::SeedPhrase { hd_path, .. } => assert_eq!(hd_path, None), + other => panic!("expected SeedPhrase variant, got {other:?}"), + } + } + + #[test] + fn test_build_secret_rejects_hd_path_with_secret_key() { + let result = build_secret(SECRET_KEY, Some(5)); + assert!(matches!(result, Err(Error::HdPathNotSupportedForSecretKey))); + } + + #[test] + fn test_build_secret_secret_key_without_hd_path() { + let secret = build_secret(SECRET_KEY, None).unwrap(); + assert!(matches!(secret, Secret::SecretKey { .. })); + } + + #[test] + fn test_run_rejects_hd_path_with_public_key() { + let (_tmp, _locator, cmd) = cmd_with_public_key(PUBLIC_KEY, Some(3)); + let result = cmd.run(&global_args()); + assert!(matches!(result, Err(Error::HdPathNotSupportedForPublicKey))); + } + + #[test] + fn test_run_accepts_public_key_without_hd_path() { + let (_tmp, _locator, cmd) = cmd_with_public_key(PUBLIC_KEY, None); + assert!(cmd.run(&global_args()).is_ok()); + } + #[test] fn public_key_flag_accepts_public_key() { let (_tmp, locator, mut cmd) = set_up_test(); diff --git a/cmd/soroban-cli/src/commands/keys/generate.rs b/cmd/soroban-cli/src/commands/keys/generate.rs index a99e0719c..9ef33bacf 100644 --- a/cmd/soroban-cli/src/commands/keys/generate.rs +++ b/cmd/soroban-cli/src/commands/keys/generate.rs @@ -50,7 +50,10 @@ pub struct Cmd { #[command(flatten)] pub config_locator: locator::Args, - /// With `--as-secret` or `--secure-store`, which `hd_path` to derive the key at from the seed phrase. + /// Which `hd_path` to derive the key at from the seed phrase. Honored across all + /// storage modes: with `--as-secret` it picks which derived key is stored, with + /// `--secure-store` or plain seed-phrase storage it is persisted on the identity + /// so later commands derive the same account without re-passing the flag. #[arg(long)] pub hd_path: Option, @@ -127,7 +130,10 @@ impl Cmd { let secret: Secret = seed_phrase.into(); Ok(secret.private_key(self.hd_path)?.into()) } else { - Ok(seed_phrase.into()) + Ok(Secret::SeedPhrase { + seed_phrase: seed_phrase.seed_phrase.into_phrase(), + hd_path: self.hd_path, + }) } } @@ -140,7 +146,7 @@ impl Cmd { mod tests { use crate::config::{address::KeyName, key::Key, secret::Secret}; - fn set_up_test() -> (super::locator::Args, super::Cmd) { + fn set_up_test() -> (super::locator::Args, super::Cmd, tempfile::TempDir) { let temp_dir = tempfile::tempdir().unwrap(); let locator = super::locator::Args { config_dir: Some(temp_dir.path().to_path_buf()), @@ -158,7 +164,7 @@ mod tests { overwrite: false, }; - (locator, cmd) + (locator, cmd, temp_dir) } fn global_args() -> super::global::Args { @@ -170,7 +176,7 @@ mod tests { #[tokio::test] async fn test_storing_secret_as_a_seed_phrase() { - let (test_locator, cmd) = set_up_test(); + let (test_locator, cmd, _temp_dir) = set_up_test(); let global_args = global_args(); let result = cmd.run(&global_args).await; @@ -179,9 +185,26 @@ mod tests { assert!(matches!(identity, Key::Secret(Secret::SeedPhrase { .. }))); } + #[tokio::test] + async fn test_generate_seed_phrase_persists_hd_path() { + let (test_locator, mut cmd, _temp_dir) = set_up_test(); + cmd.hd_path = Some(7); + let global_args = global_args(); + + cmd.run(&global_args).await.unwrap(); + + let identity = test_locator.read_identity("test_name").unwrap(); + match identity { + Key::Secret(Secret::SeedPhrase { hd_path, .. }) => { + assert_eq!(hd_path, Some(7)); + } + other => panic!("expected SeedPhrase variant, got {other:?}"), + } + } + #[tokio::test] async fn test_storing_secret_as_a_secret_key() { - let (test_locator, mut cmd) = set_up_test(); + let (test_locator, mut cmd, _temp_dir) = set_up_test(); cmd.as_secret = true; let global_args = global_args(); @@ -196,7 +219,7 @@ mod tests { async fn test_storing_secret_in_secure_store() { use keyring::{mock, set_default_credential_builder}; set_default_credential_builder(mock::default_credential_builder()); - let (test_locator, mut cmd) = set_up_test(); + let (test_locator, mut cmd, _temp_dir) = set_up_test(); cmd.secure_store = true; let global_args = global_args(); @@ -211,7 +234,7 @@ mod tests { async fn test_generate_secure_store_caches_public_key_on_disk() { use keyring::{mock, set_default_credential_builder}; set_default_credential_builder(mock::default_credential_builder()); - let (test_locator, mut cmd) = set_up_test(); + let (test_locator, mut cmd, _temp_dir) = set_up_test(); cmd.secure_store = true; let global_args = global_args(); @@ -237,7 +260,7 @@ mod tests { #[cfg(not(feature = "additional-libs"))] #[tokio::test] async fn test_storing_in_secure_store_returns_error_when_additional_libs_not_enabled() { - let (test_locator, mut cmd) = set_up_test(); + let (test_locator, mut cmd, _temp_dir) = set_up_test(); cmd.secure_store = true; let global_args = global_args(); diff --git a/cmd/soroban-cli/src/commands/keys/secret.rs b/cmd/soroban-cli/src/commands/keys/secret.rs index 1338307b1..ceb79d886 100644 --- a/cmd/soroban-cli/src/commands/keys/secret.rs +++ b/cmd/soroban-cli/src/commands/keys/secret.rs @@ -49,7 +49,7 @@ impl Cmd { pub fn seed_phrase(&self) -> Result { let key = self.locator.read_identity(&self.name)?; - if let Key::Secret(Secret::SeedPhrase { seed_phrase }) = key { + if let Key::Secret(Secret::SeedPhrase { seed_phrase, .. }) = key { Ok(seed_phrase) } else { Err(Error::UnknownSeedPhrase) diff --git a/cmd/soroban-cli/src/config/key.rs b/cmd/soroban-cli/src/config/key.rs index 5ac65d62a..811d990c3 100644 --- a/cmd/soroban-cli/src/config/key.rs +++ b/cmd/soroban-cli/src/config/key.rs @@ -185,7 +185,10 @@ mod test { #[test] fn secret_seed_phrase() { let seed_phrase = "singer swing mango apple singer swing mango apple singer swing mango apple singer swing mango apple".to_string(); - let secret = Secret::SeedPhrase { seed_phrase }; + let secret = Secret::SeedPhrase { + seed_phrase, + hd_path: None, + }; let key = Key::Secret(secret); round_trip(&key); } diff --git a/cmd/soroban-cli/src/config/secret.rs b/cmd/soroban-cli/src/config/secret.rs index 7ab52712f..582596058 100644 --- a/cmd/soroban-cli/src/config/secret.rs +++ b/cmd/soroban-cli/src/config/secret.rs @@ -62,6 +62,12 @@ pub enum Secret { }, SeedPhrase { seed_phrase: String, + // Persisted derivation index. Lets `--hd-path` set on `keys generate` / + // `keys add` travel with the identity, so later commands derive the + // intended account without re-passing the flag. Optional for backwards + // compatibility with files written before this field existed. + #[serde(default, skip_serializing_if = "Option::is_none")] + hd_path: Option, }, Ledger, SecureStore { @@ -87,6 +93,7 @@ impl FromStr for Secret { } else if sep5::SeedPhrase::from_str(s).is_ok() { Ok(Secret::SeedPhrase { seed_phrase: s.to_string(), + hd_path: None, }) } else if s == "ledger" { Ok(Secret::Ledger) @@ -120,6 +127,7 @@ impl From for Secret { fn from(value: SeedPhrase) -> Self { Secret::SeedPhrase { seed_phrase: value.seed_phrase.into_phrase(), + hd_path: None, } } } @@ -128,9 +136,12 @@ impl Secret { pub fn private_key(&self, index: Option) -> Result { Ok(match self { Secret::SecretKey { secret_key } => PrivateKey::from_string(secret_key)?, - Secret::SeedPhrase { seed_phrase } => PrivateKey::from_payload( + Secret::SeedPhrase { + seed_phrase, + hd_path, + } => PrivateKey::from_payload( &sep5::SeedPhrase::from_str(seed_phrase)? - .from_path_index(index.unwrap_or_default(), None)? + .from_path_index(index.or(*hd_path).unwrap_or_default(), None)? .private() .0, )?, @@ -148,10 +159,11 @@ impl Secret { hd_path, } = self { - if let Some(cached) = cached_public_key(public_key.as_deref(), *hd_path, index) { + let effective = index.or(*hd_path); + if let Some(cached) = cached_public_key(public_key.as_deref(), *hd_path, effective) { return Ok(cached); } - Ok(secure_store::get_public_key(entry_name, index)?) + Ok(secure_store::get_public_key(entry_name, effective)?) } else { let key = self.key_pair(index)?; Ok(stellar_strkey::ed25519::PublicKey::from_payload( @@ -178,11 +190,12 @@ impl Secret { public_key, hd_path: cached_hd_path, } => { + let effective = hd_path.or(*cached_hd_path); let cached_public_key = - cached_public_key(public_key.as_deref(), *cached_hd_path, hd_path); + cached_public_key(public_key.as_deref(), *cached_hd_path, effective); SignerKind::SecureStore(SecureStoreEntry { name: entry_name.clone(), - hd_path, + hd_path: effective, public_key: cached_public_key, }) } @@ -326,6 +339,20 @@ mod tests { assert_eq!(pk.to_string(), TEST_PUBLIC_KEY); } + #[test] + fn test_secure_store_public_key_falls_back_to_persisted_hd_path() { + // Bogus entry_name guarantees a keychain lookup would fail. The cache is + // populated at the persisted hd_path; calling public_key(None) must fall + // back to that hd_path and hit the cache rather than re-deriving at index 0. + let secret = Secret::SecureStore { + entry_name: "secure_store:org.stellar.cli-no-such-entry".to_string(), + public_key: Some(TEST_PUBLIC_KEY.to_string()), + hd_path: Some(5), + }; + let pk = secret.public_key(None).unwrap(); + assert_eq!(pk.to_string(), TEST_PUBLIC_KEY); + } + #[test] fn test_cached_public_key_treats_none_and_zero_as_equal() { // `unwrap_or_default()` is used everywhere else for hd_path, so the @@ -347,4 +374,62 @@ mod tests { assert!(cached_public_key(Some("not-a-public-key"), None, None).is_none()); assert!(cached_public_key(Some(""), None, None).is_none()); } + + #[test] + fn test_seed_phrase_toml_round_trip_with_hd_path() { + let secret = Secret::SeedPhrase { + seed_phrase: TEST_SEED_PHRASE.to_string(), + hd_path: Some(5), + }; + let serialized = toml::to_string(&secret).unwrap(); + assert!( + serialized.contains("hd_path"), + "expected hd_path field in TOML, got: {serialized}" + ); + let parsed: Secret = toml::from_str(&serialized).unwrap(); + assert_eq!(secret, parsed); + } + + #[test] + fn test_seed_phrase_legacy_toml_parses_with_none_hd_path() { + // Identity files written before this feature only contain seed_phrase. + // They must still parse, with hd_path defaulting to None. + let toml_str = format!("seed_phrase = \"{TEST_SEED_PHRASE}\"\n"); + let secret: Secret = toml::from_str(&toml_str).unwrap(); + match secret { + Secret::SeedPhrase { + seed_phrase, + hd_path, + } => { + assert_eq!(seed_phrase, TEST_SEED_PHRASE); + assert_eq!(hd_path, None); + } + other => panic!("expected SeedPhrase variant, got {other:?}"), + } + } + + #[test] + fn test_seed_phrase_uses_persisted_hd_path_when_caller_passes_none() { + // When the caller passes None, the persisted hd_path should drive derivation. + let secret = Secret::SeedPhrase { + seed_phrase: TEST_SEED_PHRASE.to_string(), + hd_path: Some(1), + }; + let pk_at_0 = secret.public_key(Some(0)).unwrap(); + let pk_default = secret.public_key(None).unwrap(); + assert_ne!(pk_at_0.to_string(), pk_default.to_string()); + } + + #[test] + fn test_seed_phrase_caller_hd_path_overrides_persisted() { + // Caller's explicit hd_path argument always wins over the persisted value. + let secret = Secret::SeedPhrase { + seed_phrase: TEST_SEED_PHRASE.to_string(), + hd_path: Some(1), + }; + let pk = secret.public_key(Some(0)).unwrap(); + let sk = secret.private_key(Some(0)).unwrap(); + assert_eq!(pk.to_string(), TEST_PUBLIC_KEY); + assert_eq!(sk.to_string(), TEST_SECRET_KEY); + } }