diff --git a/FULL_HELP_DOCS.md b/FULL_HELP_DOCS.md index e8ce7fb2fe..311df98d2f 100644 --- a/FULL_HELP_DOCS.md +++ b/FULL_HELP_DOCS.md @@ -1137,6 +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.. +- `--ledger` — Derive the address from a connected Ledger hardware wallet at `m/44'/148'/N'`, where `N` defaults to 0 and can be set with `--hd-path`. Persists the derived public key (and `--hd-path`, when provided) so later commands work without the device - `--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, which `hd_path` to derive the key at. Persisted on the identity so later commands derive the same account without re-passing the flag. Not valid with `--public-key` or a raw secret key diff --git a/cmd/soroban-cli/src/commands/keys/add.rs b/cmd/soroban-cli/src/commands/keys/add.rs index 720d886ae0..dd6b4c6f97 100644 --- a/cmd/soroban-cli/src/commands/keys/add.rs +++ b/cmd/soroban-cli/src/commands/keys/add.rs @@ -7,10 +7,10 @@ use crate::{ config::{ address::KeyName, key, locator, - secret::{self, Secret}, + secret::{self, HardwareKind, Secret}, }, print::Print, - signer::secure_store, + signer::{ledger, secure_store}, }; #[derive(thiserror::Error, Debug)] @@ -28,6 +28,9 @@ pub enum Error { #[error(transparent)] SeedPhrase(#[from] sep5::error::Error), + #[error(transparent)] + Ledger(#[from] ledger::Error), + #[error("secret input error")] PasswordRead, @@ -42,6 +45,9 @@ pub enum Error { #[error("--hd-path is not valid with a secret key; secret keys cannot be derived")] HdPathNotSupportedForSecretKey, + + #[error("--hd-path {0} is out of range for a Ledger account index")] + HdPathOutOfRange(usize), } #[derive(Debug, clap::Parser, Clone)] @@ -61,10 +67,23 @@ pub struct Cmd { long, conflicts_with = "seed_phrase", conflicts_with = "secret_key", - conflicts_with = "hd_path" + conflicts_with = "hd_path", + conflicts_with = "ledger" )] pub public_key: Option, + /// Derive the address from a connected Ledger hardware wallet at + /// `m/44'/148'/N'`, where `N` defaults to 0 and can be set with + /// `--hd-path`. Persists the derived public key (and `--hd-path`, + /// when provided) so later commands work without the device. + #[arg( + long, + conflicts_with = "secret_key", + conflicts_with = "seed_phrase", + conflicts_with = "secure_store" + )] + pub ledger: bool, + /// Overwrite existing identity if it already exists. When combined with /// --secure-store, also replaces the existing Secure Store entry. #[arg(long)] @@ -79,7 +98,7 @@ pub struct Cmd { } impl Cmd { - pub fn run(&self, global_args: &global::Args) -> Result<(), Error> { + pub async fn run(&self, global_args: &global::Args) -> Result<(), Error> { let print = Print::new(global_args.quiet); if self.config_locator.read_identity(&self.name).is_ok() { @@ -92,6 +111,8 @@ impl Cmd { let key = if let Some(key) = self.public_key.as_ref() { key::Key::parse_public_only(key)? + } else if self.ledger { + self.derive_ledger_secret().await?.into() } else { self.read_secret(&print)?.into() }; @@ -103,6 +124,17 @@ impl Cmd { Ok(()) } + async fn derive_ledger_secret(&self) -> Result { + let raw = self.hd_path.unwrap_or(0); + let index: u32 = raw.try_into().map_err(|_| Error::HdPathOutOfRange(raw))?; + let public_key = ledger::new(index).await?.public_key().await?; + Ok(Secret::Ledger { + hardware: HardwareKind::Ledger, + public_key: public_key.to_string(), + hd_path: self.hd_path, + }) + } + fn read_secret(&self, print: &Print) -> Result { if self.secrets.secure_store { if std::env::var("STELLAR_SECRET_KEY").is_ok() { @@ -207,6 +239,7 @@ mod tests { }, config_locator: locator.clone(), public_key: None, + ledger: false, overwrite: false, hd_path: None, }; @@ -286,61 +319,107 @@ mod tests { } #[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()); + fn test_clap_accepts_ledger_with_hd_path() { + use clap::Parser; + + let cmd = Cmd::try_parse_from(["add", "test_name", "--ledger", "--hd-path", "5"]) + .expect("--ledger + --hd-path must parse"); + assert!(cmd.ledger); + assert_eq!(cmd.hd_path, Some(5)); + } + + #[test] + fn test_clap_rejects_ledger_with_public_key() { + use clap::Parser; + + let err = Cmd::try_parse_from(["add", "test_name", "--ledger", "--public-key", PUBLIC_KEY]) + .expect_err("clap must reject --ledger + --public-key"); + assert_eq!(err.kind(), clap::error::ErrorKind::ArgumentConflict); + } + + #[test] + fn test_clap_rejects_ledger_with_secret_key() { + use clap::Parser; + + let err = Cmd::try_parse_from(["add", "test_name", "--ledger", "--secret-key"]) + .expect_err("clap must reject --ledger + --secret-key"); + assert_eq!(err.kind(), clap::error::ErrorKind::ArgumentConflict); + } + + #[test] + fn test_clap_rejects_ledger_with_seed_phrase() { + use clap::Parser; + + let err = Cmd::try_parse_from(["add", "test_name", "--ledger", "--seed-phrase"]) + .expect_err("clap must reject --ledger + --seed-phrase"); + assert_eq!(err.kind(), clap::error::ErrorKind::ArgumentConflict); } #[test] - fn public_key_flag_accepts_public_key() { + fn test_clap_rejects_ledger_with_secure_store() { + use clap::Parser; + + let err = Cmd::try_parse_from(["add", "test_name", "--ledger", "--secure-store"]) + .expect_err("clap must reject --ledger + --secure-store"); + assert_eq!(err.kind(), clap::error::ErrorKind::ArgumentConflict); + } + + #[tokio::test] + async 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()).await.is_ok()); + } + + #[tokio::test] + async fn public_key_flag_accepts_public_key() { let (_tmp, locator, mut cmd) = set_up_test(); cmd.public_key = Some(PUBLIC_KEY.to_string()); - cmd.run(&global_args()).unwrap(); + cmd.run(&global_args()).await.unwrap(); let stored = locator.read_identity("test_name").unwrap(); assert!(matches!(stored, Key::PublicKey(_))); } - #[test] - fn public_key_flag_accepts_muxed_account() { + #[tokio::test] + async fn public_key_flag_accepts_muxed_account() { let (_tmp, locator, mut cmd) = set_up_test(); cmd.public_key = Some(MUXED_ACCOUNT.to_string()); - cmd.run(&global_args()).unwrap(); + cmd.run(&global_args()).await.unwrap(); let stored = locator.read_identity("test_name").unwrap(); assert!(matches!(stored, Key::MuxedAccount(_))); } - #[test] - fn public_key_flag_rejects_secret_key() { + #[tokio::test] + async fn public_key_flag_rejects_secret_key() { let (_tmp, locator, mut cmd) = set_up_test(); cmd.public_key = Some(SECRET_KEY.to_string()); - let err = cmd.run(&global_args()).unwrap_err(); + let err = cmd.run(&global_args()).await.unwrap_err(); assert!(matches!(err, Error::Key(key_mod::Error::PublicKeyExpected))); assert!(locator.read_identity("test_name").is_err()); } - #[test] - fn public_key_flag_rejects_seed_phrase() { + #[tokio::test] + async fn public_key_flag_rejects_seed_phrase() { let (_tmp, locator, mut cmd) = set_up_test(); cmd.public_key = Some(SEED_PHRASE.to_string()); - let err = cmd.run(&global_args()).unwrap_err(); + let err = cmd.run(&global_args()).await.unwrap_err(); assert!(matches!(err, Error::Key(key_mod::Error::PublicKeyExpected))); assert!(locator.read_identity("test_name").is_err()); } - #[test] - fn public_key_flag_rejects_ledger() { + #[tokio::test] + async fn public_key_flag_rejects_ledger() { let (_tmp, locator, mut cmd) = set_up_test(); cmd.public_key = Some("ledger".to_string()); - let err = cmd.run(&global_args()).unwrap_err(); - assert!(matches!(err, Error::Key(key_mod::Error::PublicKeyExpected))); + let err = cmd.run(&global_args()).await.unwrap_err(); + assert!(matches!(err, Error::Key(key_mod::Error::Parse))); assert!(locator.read_identity("test_name").is_err()); } - #[test] - fn public_key_flag_rejects_secure_store() { + #[tokio::test] + async fn public_key_flag_rejects_secure_store() { let (_tmp, locator, mut cmd) = set_up_test(); cmd.public_key = Some("secure_store:org.stellar.cli-alice".to_string()); - let err = cmd.run(&global_args()).unwrap_err(); + let err = cmd.run(&global_args()).await.unwrap_err(); assert!(matches!(err, Error::Key(key_mod::Error::PublicKeyExpected))); assert!(locator.read_identity("test_name").is_err()); } diff --git a/cmd/soroban-cli/src/commands/keys/mod.rs b/cmd/soroban-cli/src/commands/keys/mod.rs index 390d4139bc..dcbb6305d4 100644 --- a/cmd/soroban-cli/src/commands/keys/mod.rs +++ b/cmd/soroban-cli/src/commands/keys/mod.rs @@ -79,7 +79,7 @@ pub enum Error { impl Cmd { pub async fn run(&self, global_args: &global::Args) -> Result<(), Error> { match self { - Cmd::Add(cmd) => cmd.run(global_args)?, + Cmd::Add(cmd) => cmd.run(global_args).await?, Cmd::PublicKey(cmd) => cmd.run().await?, Cmd::Fund(cmd) => cmd.run(global_args).await?, Cmd::Generate(cmd) => cmd.run(global_args).await?, diff --git a/cmd/soroban-cli/src/commands/message/verify.rs b/cmd/soroban-cli/src/commands/message/verify.rs index abc04f6452..3eb5f665a8 100644 --- a/cmd/soroban-cli/src/commands/message/verify.rs +++ b/cmd/soroban-cli/src/commands/message/verify.rs @@ -312,23 +312,6 @@ mod tests { )); } - #[test] - fn test_verify_rejects_ledger_as_public_key() { - let cmd = super::Cmd { - message: Some("Hello, World!".to_string()), - base64: false, - signature: "fO5dbYhXUhBMhe6kId/cuVq/AfEnHRHEvsP8vXh03M1uLpi5e46yO2Q8rEBzu3feXQewcQE5GArp88u6ePK6BA==".to_string(), - public_key: "ledger".to_string(), - hd_path: None, - locator: setup_locator(), - }; - let err = cmd.run(&global_args()).unwrap_err(); - assert!(matches!( - err, - Error::Key(crate::config::key::Error::PublicKeyExpected) - )); - } - #[test] fn test_verify_rejects_secure_store_as_public_key() { let cmd = super::Cmd { diff --git a/cmd/soroban-cli/src/config/address.rs b/cmd/soroban-cli/src/config/address.rs index 9d78fc07ea..39e95b6b8b 100644 --- a/cmd/soroban-cli/src/config/address.rs +++ b/cmd/soroban-cli/src/config/address.rs @@ -47,8 +47,6 @@ pub enum Error { InvalidKeyNameCharacters(String), #[error("Invalid key name: {0}\n keys cannot exceed 250 characters")] InvalidKeyNameLength(String), - #[error("Invalid key name: {0}\n keys cannot be the word \"ledger\"")] - InvalidKeyName(String), #[error(transparent)] Name(#[from] utils::Error), } @@ -107,9 +105,6 @@ impl std::str::FromStr for KeyName { utils::Error::InvalidNameLength(s) => Error::InvalidKeyNameLength(s), utils::Error::InvalidNameCharacters(s) => Error::InvalidKeyNameCharacters(s), })?; - if s == "ledger" { - return Err(Error::InvalidKeyName(s.to_string())); - } Ok(KeyName(s.to_string())) } } diff --git a/cmd/soroban-cli/src/config/key.rs b/cmd/soroban-cli/src/config/key.rs index 811d990c3d..f3da43be29 100644 --- a/cmd/soroban-cli/src/config/key.rs +++ b/cmd/soroban-cli/src/config/key.rs @@ -227,7 +227,7 @@ mod test { #[test] fn parse_public_only_rejects_ledger() { let err = Key::parse_public_only("ledger").unwrap_err(); - assert!(matches!(err, Error::PublicKeyExpected)); + assert!(matches!(err, Error::Parse)); } #[test] diff --git a/cmd/soroban-cli/src/config/secret.rs b/cmd/soroban-cli/src/config/secret.rs index a9be4dd6ae..12d942508e 100644 --- a/cmd/soroban-cli/src/config/secret.rs +++ b/cmd/soroban-cli/src/config/secret.rs @@ -32,6 +32,12 @@ pub enum Error { SecureStoreDoesNotRevealSecretKey, #[error(transparent)] Ledger(#[from] signer::ledger::Error), + #[error( + "--hd-path {requested} does not match the path stored on this Ledger identity ({cached})" + )] + LedgerHdPathMismatch { cached: usize, requested: usize }, + #[error("--hd-path {0} is out of range for a Ledger account index")] + HdPathOutOfRange(usize), } #[derive(Debug, clap::Args, Clone)] @@ -69,7 +75,17 @@ pub enum Secret { #[serde(default, skip_serializing_if = "Option::is_none")] hd_path: Option, }, - Ledger, + // Hardware-wallet identity. The required `hardware` field tags the device + // kind (currently only `ledger`) and disambiguates this variant under + // `untagged`; future wallets can introduce new `HardwareKind` values + // without a new Secret variant. The cached `public_key` lets address and + // hint lookups succeed without the device being connected. + Ledger { + hardware: HardwareKind, + public_key: String, + #[serde(default, skip_serializing_if = "Option::is_none")] + hd_path: Option, + }, SecureStore { entry_name: String, // Cached public key derived from the secure-store entry. Lets us answer @@ -82,6 +98,12 @@ pub enum Secret { }, } +#[derive(Clone, Copy, Debug, Serialize, Deserialize, PartialEq, Eq)] +#[serde(rename_all = "lowercase")] +pub enum HardwareKind { + Ledger, +} + impl FromStr for Secret { type Err = Error; @@ -95,8 +117,6 @@ impl FromStr for Secret { seed_phrase: s.to_string(), hd_path: None, }) - } else if s == "ledger" { - Ok(Secret::Ledger) } else if s.starts_with(secure_store::ENTRY_PREFIX) { Ok(Secret::SecureStore { entry_name: s.to_string(), @@ -145,7 +165,7 @@ impl Secret { .private() .0, )?, - Secret::Ledger => panic!("Ledger does not reveal secret key"), + Secret::Ledger { .. } => return Err(Error::LedgerDoesNotRevealSecretKey), Secret::SecureStore { .. } => { return Err(Error::SecureStoreDoesNotRevealSecretKey); } @@ -153,22 +173,37 @@ impl Secret { } pub fn public_key(&self, index: Option) -> Result { - if let Secret::SecureStore { - entry_name, - public_key, - hd_path, - } = self - { - let effective = index.or(*hd_path); - if let Some(cached) = cached_public_key(public_key.as_deref(), *hd_path, effective) { - return Ok(cached); + match self { + Secret::SecureStore { + entry_name, + public_key, + hd_path, + } => { + 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, effective)?) + } + Secret::Ledger { + public_key, + hd_path: cached_hd_path, + .. + } => { + let cached = cached_hd_path.unwrap_or_default(); + let requested = index.unwrap_or(cached); + if cached != requested { + return Err(Error::LedgerHdPathMismatch { cached, requested }); + } + Ok(PublicKey::from_string(public_key)?) + } + _ => { + let key = self.key_pair(index)?; + Ok(stellar_strkey::ed25519::PublicKey::from_payload( + key.verifying_key().as_bytes(), + )?) } - Ok(secure_store::get_public_key(entry_name, effective)?) - } else { - let key = self.key_pair(index)?; - Ok(stellar_strkey::ed25519::PublicKey::from_payload( - key.verifying_key().as_bytes(), - )?) } } @@ -178,11 +213,15 @@ impl Secret { let key = self.key_pair(hd_path)?; SignerKind::Local(LocalKey { key }) } - Secret::Ledger => { - let hd_path: u32 = hd_path - .unwrap_or_default() + Secret::Ledger { + hardware: HardwareKind::Ledger, + hd_path: cached_hd_path, + .. + } => { + let effective = hd_path.or(*cached_hd_path).unwrap_or_default(); + let hd_path: u32 = effective .try_into() - .expect("usize bigger than u32"); + .map_err(|_| Error::HdPathOutOfRange(effective))?; SignerKind::Ledger(ledger::new(hd_path).await?) } Secret::SecureStore { @@ -432,4 +471,111 @@ mod tests { assert_eq!(pk.to_string(), TEST_PUBLIC_KEY); assert_eq!(sk.to_string(), TEST_SECRET_KEY); } + + #[test] + fn test_ledger_toml_round_trip_with_hd_path() { + let secret = Secret::Ledger { + hardware: HardwareKind::Ledger, + public_key: TEST_PUBLIC_KEY.to_string(), + hd_path: Some(5), + }; + let serialized = toml::to_string(&secret).unwrap(); + assert!( + serialized.contains("hardware = \"ledger\""), + "expected `hardware = \"ledger\"` tag in TOML, got: {serialized}" + ); + assert!( + serialized.contains("public_key"), + "expected public_key field in TOML, got: {serialized}" + ); + 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_ledger_toml_omits_hd_path_when_none() { + let secret = Secret::Ledger { + hardware: HardwareKind::Ledger, + public_key: TEST_PUBLIC_KEY.to_string(), + hd_path: None, + }; + let serialized = toml::to_string(&secret).unwrap(); + assert!( + !serialized.contains("hd_path"), + "expected no hd_path field in TOML when None, got: {serialized}" + ); + let parsed: Secret = toml::from_str(&serialized).unwrap(); + assert_eq!(secret, parsed); + } + + #[test] + fn test_ledger_public_key_returns_cached_without_device() { + // No emulator/device available in this test; the cached public_key + // must be returned directly without attempting to query the device. + let secret = Secret::Ledger { + hardware: HardwareKind::Ledger, + public_key: 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_ledger_public_key_rejects_mismatched_hd_path() { + // Caller asks for a different account index than the one cached on + // disk; returning the cached key would leak the wrong address. + let secret = Secret::Ledger { + hardware: HardwareKind::Ledger, + public_key: TEST_PUBLIC_KEY.to_string(), + hd_path: Some(5), + }; + assert!(matches!( + secret.public_key(Some(7)).unwrap_err(), + Error::LedgerHdPathMismatch { + cached: 5, + requested: 7 + }, + )); + } + + #[test] + fn test_ledger_public_key_treats_none_and_zero_as_equivalent() { + let secret = Secret::Ledger { + hardware: HardwareKind::Ledger, + public_key: TEST_PUBLIC_KEY.to_string(), + hd_path: None, + }; + assert_eq!( + secret.public_key(Some(0)).unwrap().to_string(), + TEST_PUBLIC_KEY + ); + } + + #[test] + fn test_ledger_private_key_is_rejected() { + let secret = Secret::Ledger { + hardware: HardwareKind::Ledger, + public_key: TEST_PUBLIC_KEY.to_string(), + hd_path: None, + }; + assert!(matches!( + secret.private_key(None).unwrap_err(), + Error::LedgerDoesNotRevealSecretKey, + )); + } + + #[test] + fn test_ledger_toml_does_not_collide_with_secure_store() { + // SecureStore TOMLs (entry_name + optional cached public_key) must not + // be mis-deserialized as Ledger now that Ledger also carries public_key. + let toml_str = "entry_name = \"secure_store:org.stellar.cli-alice\"\n\ + public_key = \"GAREAZZQWHOCBJS236KIE3AWYBVFLSBK7E5UW3ICI3TCRWQKT5LNLCEZ\"\n"; + let secret: Secret = toml::from_str(toml_str).unwrap(); + assert!(matches!(secret, Secret::SecureStore { .. })); + } }