Ledger key path rework (#8453)

automerge

Cargo.lock

@@ -4390,6 +4390,7 @@ dependencies = [
  "semver 0.9.0 (registry+https://github.com/rust-lang/crates.io-index)",
  "solana-sdk 1.1.0",
  "thiserror 1.0.11 (registry+https://github.com/rust-lang/crates.io-index)",
+ "url 2.1.1 (registry+https://github.com/rust-lang/crates.io-index)",
 ]
 
 [[package]]

clap-utils/src/input_parsers.rs

@@ -119,8 +119,8 @@ pub fn derivation_of(matches: &ArgMatches<'_>, name: &str) -> Option<DerivationP
     matches.value_of(name).map(|derivation_str| {
         let derivation_str = derivation_str.replace("'", "");
         let mut parts = derivation_str.split('/');
-        let account = parts.next().map(|account| account.parse::<u16>().unwrap());
+        let account = parts.next().map(|account| account.parse::<u32>().unwrap());
-        let change = parts.next().map(|change| change.parse::<u16>().unwrap());
+        let change = parts.next().map(|change| change.parse::<u32>().unwrap());
         DerivationPath { account, change }
     })
 }

clap-utils/src/input_validators.rs

@@ -142,7 +142,7 @@ pub fn is_derivation(value: String) -> Result<(), String> {
     let mut parts = value.split('/');
     let account = parts.next().unwrap();
     account
-        .parse::<u16>()
+        .parse::<u32>()
         .map_err(|e| {
             format!(
                 "Unable to parse derivation, provided: {}, err: {:?}",
@@ -151,7 +151,7 @@ pub fn is_derivation(value: String) -> Result<(), String> {
         })
         .and_then(|_| {
             if let Some(change) = parts.next() {
-                change.parse::<u16>().map_err(|e| {
+                change.parse::<u32>().map_err(|e| {
                     format!(
                         "Unable to parse derivation, provided: {}, err: {:?}",
                         change, e
@@ -172,11 +172,12 @@ mod tests {
     fn test_is_derivation() {
         assert_eq!(is_derivation("2".to_string()), Ok(()));
         assert_eq!(is_derivation("0".to_string()), Ok(()));
+        assert_eq!(is_derivation("65537".to_string()), Ok(()));
         assert_eq!(is_derivation("0/2".to_string()), Ok(()));
         assert_eq!(is_derivation("0'/2'".to_string()), Ok(()));
         assert!(is_derivation("a".to_string()).is_err());
-        assert!(is_derivation("65537".to_string()).is_err());
+        assert!(is_derivation("4294967296".to_string()).is_err());
         assert!(is_derivation("a/b".to_string()).is_err());
-        assert!(is_derivation("0/65537".to_string()).is_err());
+        assert!(is_derivation("0/4294967296".to_string()).is_err());
     }
 }

clap-utils/src/keypair.rs

@@ -39,7 +39,7 @@ pub fn parse_keypair_path(path: &str) -> KeypairUrl {
     } else if path == ASK_KEYWORD {
         KeypairUrl::Ask
     } else if path.starts_with("usb://") {
-        KeypairUrl::Usb(path.split_at(6).1.to_string())
+        KeypairUrl::Usb(path.to_string())
     } else if let Ok(pubkey) = Pubkey::from_str(path) {
         KeypairUrl::Pubkey(pubkey)
     } else {

remote-wallet/Cargo.toml

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

remote-wallet/src/ledger.rs

@@ -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;
@@ -373,11 +375,11 @@ fn extend_and_serialize(derivation_path: &DerivationPath) -> Vec<u8> {
     let mut concat_derivation = vec![byte];
     concat_derivation.extend_from_slice(&SOL_DERIVATION_PATH_BE);
     if let Some(account) = derivation_path.account {
-        concat_derivation.extend_from_slice(&[0x80, 0]);
+        let hardened_account = account | HARDENED_BIT;
-        concat_derivation.extend_from_slice(&account.to_be_bytes());
+        concat_derivation.extend_from_slice(&hardened_account.to_be_bytes());
         if let Some(change) = derivation_path.change {
-            concat_derivation.extend_from_slice(&[0x80, 0]);
+            let hardened_change = change | HARDENED_BIT;
-            concat_derivation.extend_from_slice(&change.to_be_bytes());
+            concat_derivation.extend_from_slice(&hardened_change.to_be_bytes());
         }
     }
     concat_derivation

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;
@@ -211,45 +212,68 @@ pub struct RemoteWalletInfo {
 }
 
 impl RemoteWalletInfo {
-    pub fn parse_path(mut path: String) -> Result<(Self, DerivationPath), RemoteWalletError> {
+    pub fn parse_path(path: String) -> Result<(Self, DerivationPath), RemoteWalletError> {
-        if path.ends_with('/') {
+        let wallet_path = Url::parse(&path).map_err(|e| {
-            path.pop();
+            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 = wallet_path.host_str().unwrap().to_string();
-        wallet_info.manufacturer = manufacturer.to_string();
+
-        wallet_info.model = parts.next().unwrap_or("").to_string();
+        if let Some(wallet_id) = wallet_path.path_segments().map(|c| c.collect::<Vec<_>>()) {
-        wallet_info.pubkey = parts
+            wallet_info.model = wallet_id[0].to_string();
-            .next()
+            if wallet_id.len() > 1 {
-            .and_then(|pubkey_str| Pubkey::from_str(pubkey_str).ok())
+                wallet_info.pubkey = Pubkey::from_str(wallet_id[1]).map_err(|e| {
-            .unwrap_or_default();
+                    RemoteWalletError::InvalidDerivationPath(format!(
+                        "pubkey from_str error: {:?}",
+                        e
+                    ))
+                })?;
+            }
+        }
 
         let mut derivation_path = DerivationPath::default();
-        if let Some(purpose) = parts.next() {
+        let mut query_pairs = wallet_path.query_pairs();
-            if purpose.replace("'", "") != "44" {
+        if query_pairs.count() > 0 {
-                return Err(RemoteWalletError::InvalidDerivationPath(format!(
+            for _ in 0..query_pairs.count() {
-                    "Incorrect purpose number, found: {}, must be 44",
+                if let Some(mut pair) = query_pairs.next() {
-                    purpose
+                    if pair.0 == "key" {
-                )));
+                        let key_path = pair.1.to_mut();
-            }
+                        let _key_path = key_path.clone();
-            if let Some(coin) = parts.next() {
+                        if key_path.ends_with('/') {
-                if coin.replace("'", "") != "501" {
+                            key_path.pop();
-                    return Err(RemoteWalletError::InvalidDerivationPath(format!(
+                        }
-                        "Incorrect coin number, found: {}, must be 501",
+                        let mut parts = key_path.split('/');
-                        coin
+                        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 query_pairs.next().is_some() {
+                    return Err(RemoteWalletError::InvalidDerivationPath(
+                        "invalid query string, extra fields not supported".to_string(),
+                    ));
                 }
-                derivation_path.account = parts
-                    .next()
-                    .and_then(|account| account.replace("'", "").parse::<u16>().ok());
-                derivation_path.change = parts
-                    .next()
-                    .and_then(|change| change.replace("'", "").parse::<u16>().ok());
-            } else {
-                return Err(RemoteWalletError::InvalidDerivationPath(
-                    "Derivation path too short, missing coin number".to_string(),
-                ));
             }
         }
         Ok((wallet_info, derivation_path))
@@ -273,8 +297,8 @@ impl RemoteWalletInfo {
 
 #[derive(Default, PartialEq, Clone)]
 pub struct DerivationPath {
-    pub account: Option<u16>,
+    pub account: Option<u32>,
-    pub change: Option<u16>,
+    pub change: Option<u32>,
 }
 
 impl fmt::Debug for DerivationPath {
@@ -323,22 +347,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();
+            RemoteWalletInfo::parse_path(format!("usb://ledger/nano-s/{:?}?key=1/2", pubkey))
-        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!("ledger/nano-s/{:?}/44'/501'/1'/2'", pubkey))
                 .unwrap();
         assert!(wallet_info.matches(&RemoteWalletInfo {
             model: "nano-s".to_string(),
@@ -353,13 +362,118 @@ mod tests {
                 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,
+            }
+        );
 
-        assert!(
+        // Test that wallet id need not be complete for key derivation to work
-            RemoteWalletInfo::parse_path(format!("ledger/nano-s/{:?}/43/501/1/2", pubkey)).is_err()
+        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,
+            }
         );
-        assert!(
+        let (wallet_info, derivation_path) =
-            RemoteWalletInfo::parse_path(format!("ledger/nano-s/{:?}/44/500/1/2", pubkey)).is_err()
+            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("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("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]