v0.23: backport cli refactoring and remote-wallet signing integration (#8487)

* CLI: dynamic signing reboot (#8384)

* Add keypair_util_from_path helper

* Cli: impl config.keypair as a trait object

* SDK: Add Debug and PartialEq for dyn Signer

* ClapUtils: Arg parsing from pubkey+signers to Presigner

* Impl Signers for &dyn Signer collections

* CLI: Add helper for getting signers from args

* CLI: Replace SigningAuthority with Signer trait-objs

* CLI: Drop disused signers command field

* CLI: Drop redundant tests

* Add clap validator that handles all current signer types

* clap_utils: Factor Presigner resolution to helper

* SDK: `From` for boxing Signer implementors to trait objects

* SDK: Derive `Clone` for `Presigner`

* Remove panic

* Cli: dedup signers in transfer for remote-wallet ergonomics

* Update docs vis-a-vis ASK changes

* Cli: update transaction types to use new dynamic-signer methods

* CLI: Fix tests No. 1

what to do about write_keypair outstanding

* Work around `CliConfig`'s signer not necessarily being a `Keypair`

* CLI: Fix tests No. 2

* Remove unused arg

* Remove unused methods

* Move offline arg constants upstream

* Make cli signing fallible

Co-authored-by: Trent Nelson <trent.a.b.nelson@gmail.com>

* Reinstate `create-stale-account` w/ seed test (#8401)

automerge

* CLI: collect and deduplicate signers (#8398)

* Rename (keypair util is not a thing)

* Add method to generate_unique_signers

* Cli: refactor signer handling and remote-wallet init

* Fixup unit tests

* Fixup intergation tests

* Update keypair path print statement

* Remove &None

* Use deterministic key in test

* Retain storage-account as index

* Make signer index-handling less brittle

* Cache pubkey on RemoteKeypair::new

* Make signer_of consistent + return pubkey

* Remove &matches double references

* Nonce authorities need special handling

* Make solana root key accessible on Ledger (#8421)

* Use 44/501 key as ledger id

* Add error codes

* Ledger key path rework (#8453)

automerge

* Ledger hardware wallet docs (#8472)

* Update protocol documentation

* Correct app-version command const

* Rough initial Ledger docs

* Add more docs

* Cleanup

* Add remote-wallet to docs TOC

Co-authored-by: Greg Fitzgerald <greg@solana.com>

* Add flag to confirm key on device

Co-authored-by: Trent Nelson <trent.a.b.nelson@gmail.com>
Co-authored-by: Greg Fitzgerald <greg@solana.com>

remote-wallet/Cargo.toml

@@ -17,6 +17,7 @@ parking_lot = "0.7"
 semver = "0.9"
 solana-sdk = { path = "../sdk", version = "0.23.7" }
 thiserror = "1.0"
+url = "2.1.1"
 
 [features]
 default = ["linux-static-hidraw"]

remote-wallet/src/ledger.rs

@@ -1,5 +1,5 @@
 use crate::remote_wallet::{
-    initialize_wallet_manager, DerivationPath, RemoteWallet, RemoteWalletError, RemoteWalletInfo,
+    DerivationPath, RemoteWallet, RemoteWalletError, RemoteWalletInfo, RemoteWalletManager,
 };
 use dialoguer::{theme::ColorfulTheme, Select};
 use log::*;
@@ -7,6 +7,8 @@ use semver::Version as FirmwareVersion;
 use solana_sdk::{pubkey::Pubkey, signature::Signature};
 use std::{cmp::min, fmt, sync::Arc};
 
+const HARDENED_BIT: u32 = 1 << 31;
+
 const APDU_TAG: u8 = 0x05;
 const APDU_CLA: u8 = 0xe0;
 const APDU_PAYLOAD_HEADER_LEN: usize = 8;
@@ -41,7 +43,7 @@ const HID_PREFIX_ZERO: usize = 0;
 
 mod commands {
     #[allow(dead_code)]
-    pub const GET_APP_CONFIGURATION: u8 = 0x06;
+    pub const GET_APP_CONFIGURATION: u8 = 0x01;
     pub const GET_PUBKEY: u8 = 0x02;
     pub const SIGN_MESSAGE: u8 = 0x03;
 }
@@ -74,7 +76,7 @@ impl LedgerWallet {
     //		* APDU_INS				(1 byte)
     //		* APDU_P1				(1 byte)
     //		* APDU_P2				(1 byte)
-    //		* APDU_LENGTH				(1 byte)
+    //		* APDU_LENGTH				(2 bytes)
     //		* APDU_Payload				(Variable)
     //
     fn write(&self, command: u8, p1: u8, p2: u8, data: &[u8]) -> Result<(), RemoteWalletError> {
@@ -139,11 +141,6 @@ impl LedgerWallet {
     //		* Payload				(Optional)
     //
     // Payload
-    //		* APDU Total Length			(2 bytes big endian)
-    //		* APDU_CLA				(1 byte)
-    //		* APDU_INS				(1 byte)
-    //		* APDU_P1				(1 byte)
-    //		* APDU_P2				(1 byte)
     //		* APDU_LENGTH				(1 byte)
     //		* APDU_Payload				(Variable)
     //
@@ -193,6 +190,10 @@ impl LedgerWallet {
         match status {
             // These need to be aligned with solana Ledger app error codes, and clippy allowance removed
             0x6700 => Err(RemoteWalletError::Protocol("Incorrect length")),
+            0x6802 => Err(RemoteWalletError::Protocol("Invalid parameter")),
+            0x6803 => Err(RemoteWalletError::Protocol(
+                "Overflow: message longer than MAX_MESSAGE_LENGTH",
+            )),
             0x6982 => Err(RemoteWalletError::Protocol(
                 "Security status not satisfied (Canceled by user)",
             )),
@@ -260,7 +261,7 @@ impl RemoteWallet for LedgerWallet {
             .serial_number
             .clone()
             .unwrap_or_else(|| "Unknown".to_owned());
-        self.get_pubkey(&DerivationPath::default())
+        self.get_pubkey(&DerivationPath::default(), false)
             .map(|pubkey| RemoteWalletInfo {
                 model,
                 manufacturer,
@@ -269,12 +270,16 @@ impl RemoteWallet for LedgerWallet {
             })
     }
 
-    fn get_pubkey(&self, derivation_path: &DerivationPath) -> Result<Pubkey, RemoteWalletError> {
+    fn get_pubkey(
+        &self,
+        derivation_path: &DerivationPath,
+        confirm_key: bool,
+    ) -> Result<Pubkey, RemoteWalletError> {
         let derivation_path = extend_and_serialize(derivation_path);
 
         let key = self.send_apdu(
             commands::GET_PUBKEY,
-            0, // In the naive implementation, default request is for no device confirmation
+            if confirm_key { 1 } else { 0 },
             0,
             &derivation_path,
         )?;
@@ -361,16 +366,20 @@ pub fn is_valid_ledger(vendor_id: u16, product_id: u16) -> bool {
 fn extend_and_serialize(derivation_path: &DerivationPath) -> Vec<u8> {
     let byte = if derivation_path.change.is_some() {
         4
-    } else {
+    } else if derivation_path.account.is_some() {
         3
+    } else {
+        2
     };
     let mut concat_derivation = vec![byte];
     concat_derivation.extend_from_slice(&SOL_DERIVATION_PATH_BE);
-    concat_derivation.extend_from_slice(&[0x80, 0]);
-    concat_derivation.extend_from_slice(&derivation_path.account.to_be_bytes());
-    if let Some(change) = derivation_path.change {
-        concat_derivation.extend_from_slice(&[0x80, 0]);
-        concat_derivation.extend_from_slice(&change.to_be_bytes());
+    if let Some(account) = derivation_path.account {
+        let hardened_account = account | HARDENED_BIT;
+        concat_derivation.extend_from_slice(&hardened_account.to_be_bytes());
+        if let Some(change) = derivation_path.change {
+            let hardened_change = change | HARDENED_BIT;
+            concat_derivation.extend_from_slice(&hardened_change.to_be_bytes());
+        }
     }
     concat_derivation
 }
@@ -378,9 +387,8 @@ fn extend_and_serialize(derivation_path: &DerivationPath) -> Vec<u8> {
 /// Choose a Ledger wallet based on matching info fields
 pub fn get_ledger_from_info(
     info: RemoteWalletInfo,
+    wallet_manager: &RemoteWalletManager,
 ) -> Result<Arc<LedgerWallet>, RemoteWalletError> {
-    let wallet_manager = initialize_wallet_manager();
-    let _device_count = wallet_manager.update_devices()?;
     let devices = wallet_manager.list_devices();
     let (pubkeys, device_paths): (Vec<Pubkey>, Vec<String>) = devices
         .iter()

remote-wallet/src/remote_keypair.rs

@@ -1,7 +1,8 @@
 use crate::{
     ledger::get_ledger_from_info,
     remote_wallet::{
-        DerivationPath, RemoteWallet, RemoteWalletError, RemoteWalletInfo, RemoteWalletType,
+        DerivationPath, RemoteWallet, RemoteWalletError, RemoteWalletInfo, RemoteWalletManager,
+        RemoteWalletType,
     },
 };
 use solana_sdk::{
@@ -12,24 +13,30 @@ use solana_sdk::{
 pub struct RemoteKeypair {
     pub wallet_type: RemoteWalletType,
     pub derivation_path: DerivationPath,
+    pub pubkey: Pubkey,
 }
 
 impl RemoteKeypair {
-    pub fn new(wallet_type: RemoteWalletType, derivation_path: DerivationPath) -> Self {
-        Self {
+    pub fn new(
+        wallet_type: RemoteWalletType,
+        derivation_path: DerivationPath,
+        confirm_key: bool,
+    ) -> Result<Self, RemoteWalletError> {
+        let pubkey = match &wallet_type {
+            RemoteWalletType::Ledger(wallet) => wallet.get_pubkey(&derivation_path, confirm_key)?,
+        };
+
+        Ok(Self {
             wallet_type,
             derivation_path,
-        }
+            pubkey,
+        })
     }
 }
 
 impl Signer for RemoteKeypair {
     fn try_pubkey(&self) -> Result<Pubkey, SignerError> {
-        match &self.wallet_type {
-            RemoteWalletType::Ledger(wallet) => wallet
-                .get_pubkey(&self.derivation_path)
-                .map_err(|e| e.into()),
-        }
+        Ok(self.pubkey)
     }
 
     fn try_sign_message(&self, message: &[u8]) -> Result<Signature, SignerError> {
@@ -44,17 +51,20 @@ impl Signer for RemoteKeypair {
 pub fn generate_remote_keypair(
     path: String,
     explicit_derivation_path: Option<DerivationPath>,
+    wallet_manager: &RemoteWalletManager,
+    confirm_key: bool,
 ) -> Result<RemoteKeypair, RemoteWalletError> {
     let (remote_wallet_info, mut derivation_path) = RemoteWalletInfo::parse_path(path)?;
     if let Some(derivation) = explicit_derivation_path {
         derivation_path = derivation;
     }
     if remote_wallet_info.manufacturer == "ledger" {
-        let ledger = get_ledger_from_info(remote_wallet_info)?;
-        Ok(RemoteKeypair {
-            wallet_type: RemoteWalletType::Ledger(ledger),
+        let ledger = get_ledger_from_info(remote_wallet_info, wallet_manager)?;
+        Ok(RemoteKeypair::new(
+            RemoteWalletType::Ledger(ledger),
             derivation_path,
-        })
+            confirm_key,
+        )?)
     } else {
         Err(RemoteWalletError::DeviceTypeMismatch)
     }

remote-wallet/src/remote_wallet.rs

@@ -12,6 +12,7 @@ use std::{
     time::{Duration, Instant},
 };
 use thiserror::Error;
+use url::Url;
 
 const HID_GLOBAL_USAGE_PAGE: u16 = 0xFF00;
 const HID_USB_DEVICE_CLASS: u8 = 0;
@@ -173,7 +174,11 @@ pub trait RemoteWallet {
     ) -> Result<RemoteWalletInfo, RemoteWalletError>;
 
     /// Get solana pubkey from a RemoteWallet
-    fn get_pubkey(&self, derivation_path: &DerivationPath) -> Result<Pubkey, RemoteWalletError>;
+    fn get_pubkey(
+        &self,
+        derivation_path: &DerivationPath,
+        confirm_key: bool,
+    ) -> Result<Pubkey, RemoteWalletError>;
 
     /// Sign transaction data with wallet managing pubkey at derivation path m/44'/501'/<account>'/<change>'.
     fn sign_message(
@@ -211,45 +216,68 @@ pub struct RemoteWalletInfo {
 }
 
 impl RemoteWalletInfo {
-    pub fn parse_path(mut path: String) -> Result<(Self, DerivationPath), RemoteWalletError> {
-        if path.ends_with('/') {
-            path.pop();
+    pub fn parse_path(path: String) -> Result<(Self, DerivationPath), RemoteWalletError> {
+        let wallet_path = Url::parse(&path).map_err(|e| {
+            RemoteWalletError::InvalidDerivationPath(format!("parse error: {:?}", e))
+        })?;
+
+        if wallet_path.host_str().is_none() {
+            return Err(RemoteWalletError::InvalidDerivationPath(
+                "missing remote wallet type".to_string(),
+            ));
         }
-        let mut parts = path.split('/');
+
         let mut wallet_info = RemoteWalletInfo::default();
-        let manufacturer = parts.next().unwrap();
-        wallet_info.manufacturer = manufacturer.to_string();
-        wallet_info.model = parts.next().unwrap_or("").to_string();
-        wallet_info.pubkey = parts
-            .next()
-            .and_then(|pubkey_str| Pubkey::from_str(pubkey_str).ok())
-            .unwrap_or_default();
+        wallet_info.manufacturer = wallet_path.host_str().unwrap().to_string();
+
+        if let Some(wallet_id) = wallet_path.path_segments().map(|c| c.collect::<Vec<_>>()) {
+            wallet_info.model = wallet_id[0].to_string();
+            if wallet_id.len() > 1 {
+                wallet_info.pubkey = Pubkey::from_str(wallet_id[1]).map_err(|e| {
+                    RemoteWalletError::InvalidDerivationPath(format!(
+                        "pubkey from_str error: {:?}",
+                        e
+                    ))
+                })?;
+            }
+        }
 
         let mut derivation_path = DerivationPath::default();
-        if let Some(purpose) = parts.next() {
-            if purpose.replace("'", "") != "44" {
-                return Err(RemoteWalletError::InvalidDerivationPath(format!(
-                    "Incorrect purpose number, found: {}, must be 44",
-                    purpose
-                )));
-            }
-            if let Some(coin) = parts.next() {
-                if coin.replace("'", "") != "501" {
-                    return Err(RemoteWalletError::InvalidDerivationPath(format!(
-                        "Incorrect coin number, found: {}, must be 501",
-                        coin
-                    )));
+        let mut query_pairs = wallet_path.query_pairs();
+        if query_pairs.count() > 0 {
+            for _ in 0..query_pairs.count() {
+                if let Some(mut pair) = query_pairs.next() {
+                    if pair.0 == "key" {
+                        let key_path = pair.1.to_mut();
+                        let _key_path = key_path.clone();
+                        if key_path.ends_with('/') {
+                            key_path.pop();
+                        }
+                        let mut parts = key_path.split('/');
+                        derivation_path.account = parts
+                            .next()
+                            .and_then(|account| account.replace("'", "").parse::<u32>().ok());
+                        derivation_path.change = parts
+                            .next()
+                            .and_then(|change| change.replace("'", "").parse::<u32>().ok());
+                        if parts.next().is_some() {
+                            return Err(RemoteWalletError::InvalidDerivationPath(format!(
+                                "key path `{}` too deep, only <account>/<change> supported",
+                                _key_path
+                            )));
+                        }
+                    } else {
+                        return Err(RemoteWalletError::InvalidDerivationPath(format!(
+                            "invalid query string `{}={}`, only `key` supported",
+                            pair.0, pair.1
+                        )));
+                    }
                 }
-                if let Some(account) = parts.next() {
-                    derivation_path.account = account.replace("'", "").parse::<u16>().unwrap();
-                    derivation_path.change = parts
-                        .next()
-                        .and_then(|change| change.replace("'", "").parse::<u16>().ok());
+                if query_pairs.next().is_some() {
+                    return Err(RemoteWalletError::InvalidDerivationPath(
+                        "invalid query string, extra fields not supported".to_string(),
+                    ));
                 }
-            } else {
-                return Err(RemoteWalletError::InvalidDerivationPath(
-                    "Derivation path too short, missing coin number".to_string(),
-                ));
             }
         }
         Ok((wallet_info, derivation_path))
@@ -273,18 +301,23 @@ impl RemoteWalletInfo {
 
 #[derive(Default, PartialEq, Clone)]
 pub struct DerivationPath {
-    pub account: u16,
-    pub change: Option<u16>,
+    pub account: Option<u32>,
+    pub change: Option<u32>,
 }
 
 impl fmt::Debug for DerivationPath {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        let account = if let Some(account) = self.account {
+            format!("/{:?}'", account)
+        } else {
+            "".to_string()
+        };
         let change = if let Some(change) = self.change {
             format!("/{:?}'", change)
         } else {
             "".to_string()
         };
-        write!(f, "m/44'/501'/{:?}'{}", self.account, change)
+        write!(f, "m/44'/501'{}{}", account, change)
     }
 }
 
@@ -294,9 +327,20 @@ pub fn is_valid_hid_device(usage_page: u16, interface_number: i32) -> bool {
 }
 
 /// Helper to initialize hidapi and RemoteWalletManager
-pub fn initialize_wallet_manager() -> Arc<RemoteWalletManager> {
-    let hidapi = Arc::new(Mutex::new(hidapi::HidApi::new().unwrap()));
-    RemoteWalletManager::new(hidapi)
+pub fn initialize_wallet_manager() -> Result<Arc<RemoteWalletManager>, RemoteWalletError> {
+    let hidapi = Arc::new(Mutex::new(hidapi::HidApi::new()?));
+    Ok(RemoteWalletManager::new(hidapi))
+}
+
+pub fn maybe_wallet_manager() -> Result<Option<Arc<RemoteWalletManager>>, RemoteWalletError> {
+    let wallet_manager = initialize_wallet_manager()?;
+    let device_count = wallet_manager.update_devices()?;
+    if device_count > 0 {
+        Ok(Some(wallet_manager))
+    } else {
+        drop(wallet_manager);
+        Ok(None)
+    }
 }
 
 #[cfg(test)]
@@ -307,22 +351,7 @@ mod tests {
     fn test_parse_path() {
         let pubkey = Pubkey::new_rand();
         let (wallet_info, derivation_path) =
-            RemoteWalletInfo::parse_path(format!("ledger/nano-s/{:?}/44/501/1/2", pubkey)).unwrap();
-        assert!(wallet_info.matches(&RemoteWalletInfo {
-            model: "nano-s".to_string(),
-            manufacturer: "ledger".to_string(),
-            serial: "".to_string(),
-            pubkey,
-        }));
-        assert_eq!(
-            derivation_path,
-            DerivationPath {
-                account: 1,
-                change: Some(2),
-            }
-        );
-        let (wallet_info, derivation_path) =
-            RemoteWalletInfo::parse_path(format!("ledger/nano-s/{:?}/44'/501'/1'/2'", pubkey))
+            RemoteWalletInfo::parse_path(format!("usb://ledger/nano-s/{:?}?key=1/2", pubkey))
                 .unwrap();
         assert!(wallet_info.matches(&RemoteWalletInfo {
             model: "nano-s".to_string(),
@@ -333,17 +362,122 @@ mod tests {
         assert_eq!(
             derivation_path,
             DerivationPath {
-                account: 1,
+                account: Some(1),
+                change: Some(2),
+            }
+        );
+        let (wallet_info, derivation_path) =
+            RemoteWalletInfo::parse_path(format!("usb://ledger/nano-s/{:?}?key=1'/2'", pubkey))
+                .unwrap();
+        assert!(wallet_info.matches(&RemoteWalletInfo {
+            model: "nano-s".to_string(),
+            manufacturer: "ledger".to_string(),
+            serial: "".to_string(),
+            pubkey,
+        }));
+        assert_eq!(
+            derivation_path,
+            DerivationPath {
+                account: Some(1),
+                change: Some(2),
+            }
+        );
+        let (wallet_info, derivation_path) =
+            RemoteWalletInfo::parse_path(format!("usb://ledger/nano-s/{:?}?key=1\'/2\'", pubkey))
+                .unwrap();
+        assert!(wallet_info.matches(&RemoteWalletInfo {
+            model: "nano-s".to_string(),
+            manufacturer: "ledger".to_string(),
+            serial: "".to_string(),
+            pubkey,
+        }));
+        assert_eq!(
+            derivation_path,
+            DerivationPath {
+                account: Some(1),
+                change: Some(2),
+            }
+        );
+        let (wallet_info, derivation_path) =
+            RemoteWalletInfo::parse_path(format!("usb://ledger/nano-s/{:?}?key=1/2/", pubkey))
+                .unwrap();
+        assert!(wallet_info.matches(&RemoteWalletInfo {
+            model: "nano-s".to_string(),
+            manufacturer: "ledger".to_string(),
+            serial: "".to_string(),
+            pubkey,
+        }));
+        assert_eq!(
+            derivation_path,
+            DerivationPath {
+                account: Some(1),
+                change: Some(2),
+            }
+        );
+        let (wallet_info, derivation_path) =
+            RemoteWalletInfo::parse_path(format!("usb://ledger/nano-s/{:?}?key=1/", pubkey))
+                .unwrap();
+        assert!(wallet_info.matches(&RemoteWalletInfo {
+            model: "nano-s".to_string(),
+            manufacturer: "ledger".to_string(),
+            serial: "".to_string(),
+            pubkey,
+        }));
+        assert_eq!(
+            derivation_path,
+            DerivationPath {
+                account: Some(1),
+                change: None,
+            }
+        );
+
+        // Test that wallet id need not be complete for key derivation to work
+        let (wallet_info, derivation_path) =
+            RemoteWalletInfo::parse_path("usb://ledger/nano-s?key=1".to_string()).unwrap();
+        assert!(wallet_info.matches(&RemoteWalletInfo {
+            model: "nano-s".to_string(),
+            manufacturer: "ledger".to_string(),
+            serial: "".to_string(),
+            pubkey: Pubkey::default(),
+        }));
+        assert_eq!(
+            derivation_path,
+            DerivationPath {
+                account: Some(1),
+                change: None,
+            }
+        );
+        let (wallet_info, derivation_path) =
+            RemoteWalletInfo::parse_path("usb://ledger/?key=1/2".to_string()).unwrap();
+        assert!(wallet_info.matches(&RemoteWalletInfo {
+            model: "".to_string(),
+            manufacturer: "ledger".to_string(),
+            serial: "".to_string(),
+            pubkey: Pubkey::default(),
+        }));
+        assert_eq!(
+            derivation_path,
+            DerivationPath {
+                account: Some(1),
                 change: Some(2),
             }
         );
 
+        // Failure cases
         assert!(
-            RemoteWalletInfo::parse_path(format!("ledger/nano-s/{:?}/43/501/1/2", pubkey)).is_err()
+            RemoteWalletInfo::parse_path("usb://ledger/nano-s/bad-pubkey?key=1/2".to_string())
+                .is_err()
         );
+        assert!(RemoteWalletInfo::parse_path("usb://?key=1/2".to_string()).is_err());
+        assert!(RemoteWalletInfo::parse_path("usb:/ledger?key=1/2".to_string()).is_err());
+        assert!(RemoteWalletInfo::parse_path("ledger?key=1/2".to_string()).is_err());
+        assert!(RemoteWalletInfo::parse_path("usb://ledger?key=1/2/3".to_string()).is_err());
+        // Other query strings cause an error
         assert!(
-            RemoteWalletInfo::parse_path(format!("ledger/nano-s/{:?}/44/500/1/2", pubkey)).is_err()
+            RemoteWalletInfo::parse_path("usb://ledger/?key=1/2&test=other".to_string()).is_err()
         );
+        assert!(RemoteWalletInfo::parse_path("usb://ledger/?Key=1/2".to_string()).is_err());
+        assert!(RemoteWalletInfo::parse_path("usb://ledger/?test=other".to_string()).is_err());
     }
 
     #[test]