hardware_wallet/Ledger `Sign messages` + some refactoring (#8868)

* getting started

* getting started

* signing personal messages works and refactoring

* Refactor `Ledger`

* Make `Ledger Manager` only visible inside the hardwallet-crate
* Refactor `send_apdu` with separate functions for read and write

* Add support for signing messages through `ethcore`

* Trezor modify update_devices and some error msgs

* nits

Cargo.lock

@@ -1192,6 +1192,7 @@ dependencies = [
  "parking_lot 0.5.4 (registry+https://github.com/rust-lang/crates.io-index)",
  "protobuf 1.5.1 (registry+https://github.com/rust-lang/crates.io-index)",
  "rustc-hex 1.0.0 (registry+https://github.com/rust-lang/crates.io-index)",
+ "semver 0.9.0 (registry+https://github.com/rust-lang/crates.io-index)",
  "trezor-sys 1.0.0 (git+https://github.com/paritytech/trezor-sys)",
 ]
 

ethcore/src/account_provider/mod.rs

@@ -41,7 +41,7 @@ pub use ethstore::{Derivation, IndexDerivation, KeyFile};
 enum Unlock {
 	/// If account is unlocked temporarily, it should be locked after first usage.
 	OneTime,
-	/// Account unlocked permantently can always sign message.
+	/// Account unlocked permanently can always sign message.
 	/// Use with caution.
 	Perm,
 	/// Account unlocked with a timeout
@@ -280,10 +280,10 @@ impl AccountProvider {
 	pub fn accounts(&self) -> Result<Vec<Address>, Error> {
 		let accounts = self.sstore.accounts()?;
 		Ok(accounts
-		   .into_iter()
+			.into_iter()
-		   .map(|a| a.address)
+			.map(|a| a.address)
-		   .filter(|address| !self.blacklisted_accounts.contains(address))
+			.filter(|address| !self.blacklisted_accounts.contains(address))
-		   .collect()
+			.collect()
 		)
 	}
 
@@ -495,7 +495,7 @@ impl AccountProvider {
 		self.address_book.write().set_meta(account, meta)
 	}
 
-	/// Removes and address from the addressbook
+	/// Removes and address from the address book
 	pub fn remove_address(&self, addr: Address) {
 		self.address_book.write().remove(addr)
 	}
@@ -585,7 +585,7 @@ impl AccountProvider {
 	fn unlock_account(&self, address: Address, password: String, unlock: Unlock) -> Result<(), Error> {
 		let account = self.sstore.account_ref(&address)?;
 
-		// check if account is already unlocked pernamently, if it is, do nothing
+		// check if account is already unlocked permanently, if it is, do nothing
 		let mut unlocked = self.unlocked.write();
 		if let Some(data) = unlocked.get(&account) {
 			if let Unlock::Perm = data.unlock {
@@ -809,8 +809,17 @@ impl AccountProvider {
 			.map_err(Into::into)
 	}
 
+	/// Sign message with hardware wallet.
+	pub fn sign_message_with_hardware(&self, address: &Address, message: &[u8]) -> Result<Signature, SignError> {
+		match self.hardware_store.as_ref().map(|s| s.sign_message(address, message)) {
+			None | Some(Err(HardwareError::KeyNotFound)) => Err(SignError::NotFound),
+			Some(Err(e)) => Err(From::from(e)),
+			Some(Ok(s)) => Ok(s),
+		}
+	}
+
 	/// Sign transaction with hardware wallet.
-	pub fn sign_with_hardware(&self, address: Address, transaction: &Transaction, chain_id: Option<u64>, rlp_encoded_transaction: &[u8]) -> Result<Signature, SignError> {
+	pub fn sign_transaction_with_hardware(&self, address: &Address, transaction: &Transaction, chain_id: Option<u64>, rlp_encoded_transaction: &[u8]) -> Result<Signature, SignError> {
 		let t_info = TransactionInfo {
 			nonce: transaction.nonce,
 			gas_price: transaction.gas_price,

hw/Cargo.toml

@@ -15,6 +15,7 @@ libusb = { git = "https://github.com/paritytech/libusb-rs" }
 trezor-sys = { git = "https://github.com/paritytech/trezor-sys" }
 ethkey = { path = "../ethkey" }
 ethereum-types = "0.3"
+semver = "0.9"
 
 [dev-dependencies]
 rustc-hex = "1.0"

hw/src/lib.rs

@@ -25,7 +25,9 @@ extern crate hidapi;
 extern crate libusb;
 extern crate parking_lot;
 extern crate protobuf;
+extern crate semver;
 extern crate trezor_sys;
+
 #[macro_use] extern crate log;
 #[cfg(test)] extern crate rustc_hex;
 
@@ -35,13 +37,12 @@ mod trezor;
 use ethkey::{Address, Signature};
 
 use parking_lot::Mutex;
-use std::fmt;
+use std::{fmt, time::Duration};
-use std::sync::Arc;
+use std::sync::{Arc, atomic, atomic::AtomicBool};
-use std::sync::atomic;
-use std::sync::atomic::AtomicBool;
 use ethereum_types::U256;
 
 const USB_DEVICE_CLASS_DEVICE: u8 = 0;
+const POLLING_DURATION: Duration = Duration::from_millis(500);
 
 #[derive(Debug)]
 pub struct Device {
@@ -57,13 +58,13 @@ pub trait Wallet<'a> {
 
 	/// Sign transaction data with wallet managing `address`.
 	fn sign_transaction(&self, address: &Address, transaction: Self::Transaction) -> Result<Signature, Self::Error>;
-
+	
 	/// Set key derivation path for a chain.
 	fn set_key_path(&self, key_path: KeyPath);
 
 	/// Re-populate device list
 	/// Note, this assumes all devices are iterated over and updated
-	fn update_devices(&self) -> Result<usize, Self::Error>;
+	fn update_devices(&self, device_direction: DeviceDirection) -> Result<usize, Self::Error>;
 
 	/// Read device info
 	fn read_device(&self, usb: &hidapi::HidApi, dev_info: &hidapi::HidDeviceInfo) -> Result<Device, Self::Error>;
@@ -185,6 +186,22 @@ impl From<libusb::Error> for Error {
 	}
 }
 
+/// Specifies the direction of the `HardwareWallet` i.e, whether it arrived or left
+#[derive(Debug, Copy, Clone)]
+pub enum DeviceDirection {
+    Arrived,
+    Left,
+}
+
+impl fmt::Display for DeviceDirection {
+	fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+		match self {
+			DeviceDirection::Arrived => write!(f, "arrived"),
+			DeviceDirection::Left => write!(f, "left"),
+		}
+	}
+}
+
 /// Hardware wallet management interface.
 pub struct HardwareWalletManager {
 	exiting: Arc<AtomicBool>,
@@ -239,6 +256,17 @@ impl HardwareWalletManager {
 		}
 	}
 
+	/// Sign a message with the wallet (only supported by Ledger)
+	pub fn sign_message(&self, address: &Address, msg: &[u8]) -> Result<Signature, Error> {
+		if self.ledger.get_wallet(address).is_some() {
+			Ok(self.ledger.sign_message(address, msg)?)
+		} else if self.trezor.get_wallet(address).is_some() {
+			Err(Error::TrezorDevice(trezor::Error::NoSigningMessage))
+		} else {
+			Err(Error::KeyNotFound)
+		}
+	}
+
 	/// Sign transaction data with wallet managing `address`.
 	pub fn sign_transaction(&self, address: &Address, t_info: &TransactionInfo, encoded_transaction: &[u8]) -> Result<Signature, Error> {
 		if self.ledger.get_wallet(address).is_some() {

hw/src/trezor.rs

@@ -19,23 +19,20 @@
 //! and https://github.com/trezor/trezor-common/blob/master/protob/protocol.md
 //! for protocol details.
 
-use super::{WalletInfo, TransactionInfo, KeyPath, Wallet, Device, USB_DEVICE_CLASS_DEVICE};
+use super::{DeviceDirection, WalletInfo, TransactionInfo, KeyPath, Wallet, Device, USB_DEVICE_CLASS_DEVICE, POLLING_DURATION};
 
 use std::cmp::{min, max};
-use std::fmt;
+use std::sync::{atomic, atomic::AtomicBool, Arc, Weak};
-use std::sync::{Arc, Weak};
-use std::sync::atomic;
-use std::sync::atomic::AtomicBool;
 use std::time::{Duration, Instant};
-use std::thread;
+use std::{fmt, thread};
 
 use ethereum_types::{U256, H256, Address};
 use ethkey::Signature;
 use hidapi;
 use libusb;
 use parking_lot::{Mutex, RwLock};
-use protobuf;
 use protobuf::{Message, ProtobufEnum};
+use protobuf;
 
 use trezor_sys::messages::{EthereumAddress, PinMatrixAck, MessageType, EthereumTxRequest, EthereumSignTx, EthereumGetAddress, EthereumTxAck, ButtonAck};
 
@@ -62,6 +59,12 @@ pub enum Error {
 	BadMessageType,
 	/// Trying to read from a closed device at the given path
 	LockedDevice(String),
+	/// Signing messages are not supported by Trezor
+	NoSigningMessage,
+	/// No device arrived
+	NoDeviceArrived,
+	/// No device left
+	NoDeviceLeft,
 }
 
 impl fmt::Display for Error {
@@ -73,6 +76,9 @@ impl fmt::Display for Error {
 			Error::UserCancel => write!(f, "Operation has been cancelled"),
 			Error::BadMessageType => write!(f, "Bad Message Type in RPC call"),
 			Error::LockedDevice(ref s) => write!(f, "Device is locked, needs PIN to perform operations: {}", s),
+			Error::NoSigningMessage=> write!(f, "Signing messages are not supported by Trezor"),
+			Error::NoDeviceArrived => write!(f, "No device arrived"),
+			Error::NoDeviceLeft=> write!(f, "No device left"),
 		}
 	}
 }
@@ -89,8 +95,8 @@ impl From<protobuf::ProtobufError> for Error {
 	}
 }
 
-/// Ledger device manager
+/// Trezor device manager
-pub struct Manager {
+pub (crate) struct Manager {
 	usb: Arc<Mutex<hidapi::HidApi>>,
 	devices: RwLock<Vec<Device>>,
 	locked_devices: RwLock<Vec<String>>,
@@ -127,12 +133,12 @@ impl Manager {
 		thread::Builder::new()
 			.name("hw_wallet_trezor".to_string())
 			.spawn(move || {
-				if let Err(e) = m.update_devices() {
+				if let Err(e) = m.update_devices(DeviceDirection::Arrived) {
 					debug!(target: "hw", "Trezor couldn't connect at startup, error: {}", e);
 				}
 				loop {
 					usb_context.handle_events(Some(Duration::from_millis(500)))
-							   .unwrap_or_else(|e| debug!(target: "hw", "Trezor event handler error: {}", e));
+						.unwrap_or_else(|e| debug!(target: "hw", "Trezor event handler error: {}", e));
 					if exiting.load(atomic::Ordering::Acquire) {
 						break;
 					}
@@ -160,7 +166,7 @@ impl Manager {
 
 			}
 		};
-		self.update_devices()?;
+		self.update_devices(DeviceDirection::Arrived)?;
 		unlocked
 	}
 
@@ -325,52 +331,57 @@ impl <'a>Wallet<'a> for Manager {
 		*self.key_path.write() = key_path;
 	}
 
-	fn update_devices(&self) -> Result<usize, Error> {
+	fn update_devices(&self, device_direction: DeviceDirection) -> Result<usize, Error> {
 		let mut usb = self.usb.lock();
 		usb.refresh_devices();
 		let devices = usb.devices();
-		let mut new_devices = Vec::new();
+		let num_prev_devices = self.devices.read().len();
-		let mut locked_devices = Vec::new();
-		let mut error = None;
-		for usb_device in devices {
-			let is_trezor = usb_device.vendor_id == TREZOR_VID;
-			let is_supported_product = TREZOR_PIDS.contains(&usb_device.product_id);
-			let is_valid = usb_device.usage_page == 0xFF00 || usb_device.interface_number == 0;
 
-			trace!(
+		let detected_devices = devices.iter()
-				"Checking device: {:?}, trezor: {:?}, prod: {:?}, valid: {:?}",
+			.filter(|&d| {
-				usb_device,
+				let is_trezor = d.vendor_id == TREZOR_VID;
-				is_trezor,
+				let is_supported_product = TREZOR_PIDS.contains(&d.product_id);
-				is_supported_product,
+				let is_valid = d.usage_page == 0xFF00 || d.interface_number == 0;
-				is_valid,
+
-			);
+				is_trezor && is_supported_product && is_valid
-			if !is_trezor || !is_supported_product || !is_valid {
+			})
-				continue;
+			.fold(Vec::new(), |mut v, d| {
-			}
+				match self.read_device(&usb, &d) {
-			match self.read_device(&usb, &usb_device) {
+					Ok(info) => {
-				Ok(device) => new_devices.push(device),
+						trace!(target: "hw", "Found device: {:?}", info);
-				Err(Error::LockedDevice(path)) => locked_devices.push(path.to_string()),
+						v.push(info);
-				Err(e) => {
+					}
-					warn!("Error reading device: {:?}", e);
+					Err(e) => trace!(target: "hw", "Error reading device info: {}", e),
-					error = Some(e);
+				};
+				v
+			});
+		
+		let num_curr_devices = detected_devices.len();
+		*self.devices.write() = detected_devices;
+
+		match device_direction {
+			DeviceDirection::Arrived => {
+				if num_curr_devices > num_prev_devices {
+					Ok(num_curr_devices - num_prev_devices)
+				} else {
+					Err(Error::NoDeviceArrived)
+				}
+			}
+			DeviceDirection::Left => {
+				if num_prev_devices > num_curr_devices {
+					Ok(num_prev_devices- num_curr_devices)
+				} else {
+					Err(Error::NoDeviceLeft)
 				}
 			}
-		}
-		let count = new_devices.len();
-		trace!("Got devices: {:?}, closed: {:?}", new_devices, locked_devices);
-		*self.devices.write() = new_devices;
-		*self.locked_devices.write() = locked_devices;
-		match error {
-			Some(e) => Err(e),
-			None => Ok(count),
 		}
 	}
 
 	fn read_device(&self, usb: &hidapi::HidApi, dev_info: &hidapi::HidDeviceInfo) -> Result<Device, Error> {
 		let handle = self.open_path(|| usb.open_path(&dev_info.path))?;
-		let manufacturer = dev_info.manufacturer_string.clone().unwrap_or("Unknown".to_owned());
+		let manufacturer = dev_info.manufacturer_string.clone().unwrap_or_else(|| "Unknown".to_owned());
-		let name = dev_info.product_string.clone().unwrap_or("Unknown".to_owned());
+		let name = dev_info.product_string.clone().unwrap_or_else(|| "Unknown".to_owned());
-		let serial = dev_info.serial_number.clone().unwrap_or("Unknown".to_owned());
+		let serial = dev_info.serial_number.clone().unwrap_or_else(|| "Unknown".to_owned());
 		match self.get_address(&handle) {
 			Ok(Some(addr)) => {
 				Ok(Device {
@@ -428,10 +439,11 @@ impl <'a>Wallet<'a> for Manager {
 }
 
 // Try to connect to the device using polling in at most the time specified by the `timeout`
-fn try_connect_polling(trezor: Arc<Manager>, duration: Duration) -> bool {
+fn try_connect_polling(trezor: Arc<Manager>, duration: &Duration, dir: DeviceDirection) -> bool {
 	let start_time = Instant::now();
-	while start_time.elapsed() <= duration {
+	while start_time.elapsed() <= *duration {
-		if let Ok(_) = trezor.update_devices() {
+		if let Ok(num_devices) = trezor.update_devices(dir) {
+			trace!(target: "hw", "{} Trezor devices {}", num_devices, dir);
 			return true
 		}
 	}
@@ -458,8 +470,8 @@ impl libusb::Hotplug for EventHandler {
 	fn device_arrived(&mut self, _device: libusb::Device) {
 		debug!(target: "hw", "Trezor V1 arrived");
 		if let Some(trezor) = self.trezor.upgrade() {
-			if try_connect_polling(trezor, Duration::from_millis(500)) != true {
+			if try_connect_polling(trezor, &POLLING_DURATION, DeviceDirection::Arrived) != true {
-				debug!(target: "hw", "Ledger connect timeout");
+				trace!(target: "hw", "No Trezor connected");
 			}
 		}
 	}
@@ -467,8 +479,8 @@ impl libusb::Hotplug for EventHandler {
 	fn device_left(&mut self, _device: libusb::Device) {
 		debug!(target: "hw", "Trezor V1 left");
 		if let Some(trezor) = self.trezor.upgrade() {
-			if try_connect_polling(trezor, Duration::from_millis(500)) != true {
+			if try_connect_polling(trezor, &POLLING_DURATION, DeviceDirection::Left) != true {
-				debug!(target: "hw", "Ledger disconnect timeout");
+				trace!(target: "hw", "No Trezor disconnected");
 			}
 		}
 	}
@@ -481,11 +493,14 @@ impl libusb::Hotplug for EventHandler {
 fn test_signature() {
 	use ethereum_types::{H160, H256, U256};
 
-	let hidapi = Arc::new(Mutex::new(hidapi::HidApi::new().unwrap()));
+	let manager = Manager::new(
-	let manager = Manager::new(hidapi.clone(), Arc::new(AtomicBool::new(false))).unwrap();
+		Arc::new(Mutex::new(hidapi::HidApi::new().expect("HidApi"))),
+		Arc::new(AtomicBool::new(false))
+	).expect("HardwareWalletManager");
+	
 	let addr: Address = H160::from("some_addr");
 
-	assert_eq!(try_connect_polling(manager.clone(), Duration::from_millis(500)), true);
+	assert_eq!(try_connect_polling(manager.clone(), &POLLING_DURATION, DeviceDirection::Arrived), true);
 
 	let t_info = TransactionInfo {
 		nonce: U256::from(1),

rpc/src/v1/helpers/dispatch.rs

@@ -674,9 +674,16 @@ pub fn execute<D: Dispatcher + 'static>(
 		},
 		ConfirmationPayload::EthSignMessage(address, data) => {
 			if accounts.is_hardware_address(&address) {
-				return Box::new(future::err(errors::unsupported("Signing via hardware wallets is not supported.", None)));
+				let signature = accounts.sign_message_with_hardware(&address, &data)
-			}
+					.map(|s| H520(s.into_electrum()))
+					.map(RpcH520::from)
+					.map(ConfirmationResponse::Signature)
+					// TODO: is this correct? I guess the `token` is the wallet in this context
+					.map(WithToken::No)
+					.map_err(|e| errors::account("Error signing message with hardware_wallet", e));
 
+				return Box::new(future::done(signature));
+			}
 			let hash = eth_data_hash(data);
 			let res = signature(&accounts, address, hash, pass)
 				.map(|result| result
@@ -720,7 +727,7 @@ fn hardware_signature(accounts: &AccountProvider, address: Address, t: Transacti
 
 	let mut stream = rlp::RlpStream::new();
 	t.rlp_append_unsigned_transaction(&mut stream, chain_id);
-	let signature = accounts.sign_with_hardware(address, &t, chain_id, &stream.as_raw())
+	let signature = accounts.sign_transaction_with_hardware(&address, &t, chain_id, &stream.as_raw())
 		.map_err(|e| {
 			debug!(target: "miner", "Error signing transaction with hardware wallet: {}", e);
 			errors::account("Error signing transaction with hardware wallet", e)