From 6b99ab3a57f1de05e7a8ebcf81cd3c14b99f6cfe Mon Sep 17 00:00:00 2001 From: Tyera Eulberg Date: Tue, 25 Feb 2020 17:41:21 -0700 Subject: [PATCH] Ledger key path rework (#8453) automerge --- Cargo.lock | 1 + clap-utils/src/input_parsers.rs | 4 +- clap-utils/src/input_validators.rs | 9 +- clap-utils/src/keypair.rs | 2 +- remote-wallet/Cargo.toml | 1 + remote-wallet/src/ledger.rs | 10 +- remote-wallet/src/remote_wallet.rs | 226 ++++++++++++++++++++++------- 7 files changed, 186 insertions(+), 67 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index a6eddc43d9..c6b9a412a3 100644 --- a/Cargo.lock +++ b/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]] diff --git a/clap-utils/src/input_parsers.rs b/clap-utils/src/input_parsers.rs index 3451ab675a..9504b79866 100644 --- a/clap-utils/src/input_parsers.rs +++ b/clap-utils/src/input_parsers.rs @@ -119,8 +119,8 @@ pub fn derivation_of(matches: &ArgMatches<'_>, name: &str) -> Option().unwrap()); - let change = parts.next().map(|change| change.parse::().unwrap()); + let account = parts.next().map(|account| account.parse::().unwrap()); + let change = parts.next().map(|change| change.parse::().unwrap()); DerivationPath { account, change } }) } diff --git a/clap-utils/src/input_validators.rs b/clap-utils/src/input_validators.rs index 5864d95c68..0419abc058 100644 --- a/clap-utils/src/input_validators.rs +++ b/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::() + .parse::() .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::().map_err(|e| { + change.parse::().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()); } } diff --git a/clap-utils/src/keypair.rs b/clap-utils/src/keypair.rs index 24db6c29e1..5015389623 100644 --- a/clap-utils/src/keypair.rs +++ b/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 { diff --git a/remote-wallet/Cargo.toml b/remote-wallet/Cargo.toml index f679c3c1db..68fc295f54 100644 --- a/remote-wallet/Cargo.toml +++ b/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"] diff --git a/remote-wallet/src/ledger.rs b/remote-wallet/src/ledger.rs index f96665cc94..3e008f82d3 100644 --- a/remote-wallet/src/ledger.rs +++ b/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 { 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]); - concat_derivation.extend_from_slice(&account.to_be_bytes()); + let hardened_account = account | HARDENED_BIT; + concat_derivation.extend_from_slice(&hardened_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()); + let hardened_change = change | HARDENED_BIT; + concat_derivation.extend_from_slice(&hardened_change.to_be_bytes()); } } concat_derivation diff --git a/remote-wallet/src/remote_wallet.rs b/remote-wallet/src/remote_wallet.rs index e6f2a3723f..e9ccdea24f 100644 --- a/remote-wallet/src/remote_wallet.rs +++ b/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> { - 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::>()) { + 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::().ok()); + derivation_path.change = parts + .next() + .and_then(|change| change.replace("'", "").parse::().ok()); + if parts.next().is_some() { + return Err(RemoteWalletError::InvalidDerivationPath(format!( + "key path `{}` too deep, only / 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::().ok()); - derivation_path.change = parts - .next() - .and_then(|change| change.replace("'", "").parse::().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, - pub change: Option, + pub account: Option, + pub change: Option, } 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(); - 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)) + RemoteWalletInfo::parse_path(format!("usb://ledger/nano-s/{:?}?key=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!( - RemoteWalletInfo::parse_path(format!("ledger/nano-s/{:?}/43/501/1/2", pubkey)).is_err() + // 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, + } ); - assert!( - RemoteWalletInfo::parse_path(format!("ledger/nano-s/{:?}/44/500/1/2", pubkey)).is_err() + 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("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]