Trezor Support #6403

folsen · 2017-08-28T14:49:33Z

This PR addresses #4500 and extends what already exists for Ledgers to work for Trezor devices as well. Among other things it adds

Pin Matrix modal to the UI that pops up when a device is plugged in
RPC method to send requests to the Trezor device (such as pin)
Library code to talk to the Trezor over USB HID

The keepkey PinMatrix modal needs to be the same for Trezor, but we should probably try to keep it general since it can be used for both.

This is a result of much trial-and-error and a couple of dead-ends in how to communicate and wire everything up. Code here is still a bit WIP with lots of debug prints and stuff. The test works though, it is possible to sign a transaction.

This is kind of ugly and needs some cleanup and generalization. I’ve just copy-pasted some things to bring in the trezor wallets. I’ve also had to add a lock to the USB API so that only one thing talks to the USB at once.

We need to be able to get “locked” devices from the frontend to figure out if we’re going to display the PinMatrix or not. Then we need to be able to send a pin to a device.

There’s a bug somewhere here because signing a transaction fails if you take too long to press the confirm button on the device.

As my fork has been merged in.

Turns out the Trezor was adjusting the v part of the signature, and we’re already doing that so it was done twice.

BE-encoded U256 is almost the same as RLP encoded without the size-byte, except for <u8 sized values. What’s really done is BE-encoded U256 and then left-trimmed to the smallest size. Kind of obvious in hindsight.

The device will not repeat a ButtonRequest when you read from it, so you need to have a blocking `read` for whatever amount of time that you want to give the user to click. You could also have a shorter timeout but keep retrying for some amount of time, but it would amount to the same thing.

parity-cla-bot · 2017-08-28T14:49:35Z

It looks like @folsen hasn'signed our Contributor License Agreement, yet.

The purpose of a CLA is to ensure that the guardian of a project's outputs has the necessary ownership or grants of rights over all contributions to allow them to distribute under the chosen licence.
Wikipedia

You can read and sign our full Contributor License Agreement at the following URL: https://cla.parity.io

Once you've signed, plesae reply to this thread with [clabot:check] to prove it.

Many thanks,

Parity Technologies CLA Bot

jacogr · 2017-08-28T15:28:25Z

js/src/jsonrpc/interfaces/parity.js

@@ -360,6 +360,32 @@ export default {
    }
  },

+  keepkey: {


Does not match up with the method exposed (e.g. parity_trezor)

jacogr · 2017-08-28T15:30:34Z

js/src/modals/PinMatrix/pinMatrix.js

+                values={ {
+                  enterPin: (
+                    <span>
+                      Please enter the <i>pin</i> for your { device.manufacturer } hardware wallet


Needs to be FormattedMessage for translation as well (as used lower down)

folsen · 2017-08-28T15:49:27Z

[clabot:check]

parity-cla-bot · 2017-08-28T15:49:29Z

It looks like @folsen signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

debris · 2017-08-28T15:46:30Z

hw/Cargo.toml

 hidapi = { git = "https://github.com/paritytech/hidapi-rs" }
 libusb = { git = "https://github.com/paritytech/libusb-rs" }
 ethkey = { path = "../ethkey" }
+ethcore-util = { path = "../util" }


If the only two imports from util are Bytes and U256 it would be better to avoid it. Currently, importing util brings to many redundant dependencies (like rocksdb), and we want to split it into smaller modules soon.

debris · 2017-08-28T15:47:51Z

hw/src/lib.rs

-use ethkey::{Address, Signature};
-
-pub use ledger::KeyPath;
+use util::{Bytes, U256};


u can write use bigint::bigint::U256; instead

Ah, I didn't know it was there too, will change!

debris · 2017-08-28T15:50:40Z

hw/src/lib.rs

 	/// USB error.
 	Usb(libusb::Error),
 	/// Hardware wallet not found for specified key.
 	KeyNotFound,
 }

-/// Hardware waller information.
+/// This is the transaction info we need to supply to Trezor message. It's more
+/// or less a duplicate of ethcore::transaction::Transaction, but we can't


👍 I like that this is a separate structure. I'm just not sure, why is the hardwallet module a dependency of ethcore? I know, that you are not the author of this, but do you know if it could be easily separated?

Currently ethcore/src/account_provider/mod.rs imports the following from the hardware wallet crate:

use hardware_wallet::{Error as HardwareError, HardwareWalletManager, KeyPath, TransactionInfo};

and it's using it for providing accounts given by hardware wallets as well as signing transactions with hardware wallets (the sign_with_hardware function). I don't think you could remove the dependency without abstracting out both account-fetching and tx-signing to their own crates.

those things probably can be taken out into their own crates anyway. I would suggest making a single crate for account management, which encompasses both software and hardware wallets. obviously not for this PR, but further down the line

debris · 2017-08-28T16:04:44Z

hw/src/trezor/gen/mod.rs

@@ -0,0 +1,2 @@
+pub mod types;
+pub mod messages;


is it possible to generate those files at compile time rather than committing 25k loc? :)

doubling request, this code should be committed only if compile-time generation is absolutely impossible

While not impossible, there is no existing way to do it. Down the line we could write a macro to do some compile-time generation, but it's pretty hard to do without requiring the protobuf C-library from google. An acceptable intermediate solution that I've now pushed is to put all the generated code in the trezor-sys crate and import what we need from there.

gavofyork · 2017-09-11T18:24:24Z

needs resolving. and another review eventually...

tomusdrw · 2017-09-12T08:08:33Z

Sorry, forgot about this. I will submit the fixes in a separate PR.

tomusdrw

Few more grumbles regarding the RPC interface.

tomusdrw · 2017-09-12T08:37:48Z

hw/src/trezor.rs

+	pub fn message(&self, message_type: &TrezorMessageType, device_path: &Option<String>, message: &Option<String>) -> Result<String, Error> {
+		match *message_type {
+			TrezorMessageType::GetDevices => {
+				serde_json::to_string(&self.closed_devices).map_err(Error::SerdeError)


Would really prefer to have a serializable enum return type as well, passing the strings around feels a bit hacky.
TBH since we only have two possible messages why not split it into separate methods?

Yeah that's what I was asking above as well. If we change it to two methods, then getLockedDevices could be lockedHardwareAccountsInfo to match with the hardwareAccountsInfo method and a hardwarePinMatrixAck method (that could live under parity_accounts to be more protected than just getting a list of locked devices).

That would also solve the "enum problem" by removing it altogether. I ended up not doing an enum return type because the workaround for lack of impl Trait felt hackier than returning a string.

If you agree then I think I'll split it into two methods and remove this whole message-passing thing.

Awesome, makes sense!

tomusdrw · 2017-09-12T08:43:06Z

hw/src/trezor.rs

+pub enum TrezorMessageType {
+	/// Get a list of locked devices.
+	#[serde(rename="getDevices")]
+	GetDevices,


GetLockedDevices might be a better name to distinguish it from hardwareAccounts that we already have (and where trezor shows up as well after it's unlocked).

tomusdrw · 2017-09-12T08:44:21Z

hw/src/trezor.rs

+	GetDevices,
+	/// Supply a pin for the Pin Matrix Ack
+	#[serde(rename="pinMatrixAck")]
+	PinMatrixAck,


Since the parameters (device and pin) are required only for this variant why not encode them here in the enum?

tomusdrw · 2017-09-12T08:52:07Z

rpc/src/v1/impls/parity.rs

@@ -151,6 +152,11 @@ impl<C, M, U> Parity for ParityClient<C, M, U> where
 		)
 	}

+	fn trezor(&self, message_type: TrezorMessageType, device_path: Option<String>, message: Option<String>) -> Result<String, Error> {


Should be moved to parity_accounts to prevent unauthorized access.

# Conflicts: # Cargo.lock # hardwareStore.js

It’s split into two more generic endpoints that should be suitable for any hardware wallets with the same behavior to sit behind.

Needed to make tests pass

tomusdrw

Looks good! Few last grumbles regarding lock order.

tomusdrw · 2017-09-14T09:09:19Z

hw/src/ledger.rs

 	}

 	/// Sign transaction data with wallet managing `address`.
 	pub fn sign_transaction(&self, address: &Address, data: &[u8]) -> Result<Signature, Error> {
-		let device = self.devices.iter().find(|d| &d.info.address == address)
-			.ok_or(Error::KeyNotFound)?;
+		let devices = self.devices.read();


Swap the lock order to prevent deadlock.

tomusdrw · 2017-09-14T09:12:34Z

hw/src/trezor.rs

+	pub fn sign_transaction(&self, address: &Address, t_info: &TransactionInfo) -> Result<Signature, Error> {
+		let devices = self.devices.read();
+		let device = devices.iter().find(|d| &d.info.address == address).ok_or(Error::KeyNotFound)?;
+		let usb = self.usb.lock();


Swap the lock order to prevent deadlock.

tomusdrw · 2017-09-14T09:16:19Z

js/src/mobx/hardwareStore.js

+    return this._api.parity
+      .lockedHardwareAccountsInfo()
+      .then((paths) => {
+        this.pinMatrixRequest = paths.map((path) => {


As @jacogr pointed out in one of my requests Mobx may not update the listeners in case the assignment doesn't happen in an action:

@action setPinMatrixRequest= (req) => { this.pinMatrixRequest = req }; [...] .then((paths) => { this.setPinMatrixRequest(paths.map(...)); });

tomusdrw · 2017-09-14T09:19:32Z

js/src/mobx/hardwareStore.js

@@ -55,6 +58,21 @@ export default class HardwareStore {
    }, HW_SCAN_INTERVAL);
  }

+  scanTrezor () {
+    return this._api.parity
+      .lockedHardwareAccountsInfo()


This could be moved to pubsub API, but we can keep it for a separate PR.

folsen added 16 commits August 23, 2017 19:34

Copy modal from keepkey branch and generalize

1c8329d

The keepkey PinMatrix modal needs to be the same for Trezor, but we should probably try to keep it general since it can be used for both.

Add trezor communication code

298d9d8

This is a result of much trial-and-error and a couple of dead-ends in how to communicate and wire everything up. Code here is still a bit WIP with lots of debug prints and stuff. The test works though, it is possible to sign a transaction.

Extend the basic lib to allow Trezor

38b4c1d

This is kind of ugly and needs some cleanup and generalization. I’ve just copy-pasted some things to bring in the trezor wallets. I’ve also had to add a lock to the USB API so that only one thing talks to the USB at once.

Add RPC plumbing needed

f3a902b

We need to be able to get “locked” devices from the frontend to figure out if we’re going to display the PinMatrix or not. Then we need to be able to send a pin to a device.

Add logic to query backend for Trezor and display PinMatrix

aa3f62e

There’s a bug somewhere here because signing a transaction fails if you take too long to press the confirm button on the device.

Change back to paritytech branch

847aa76

As my fork has been merged in.

Converting spaces to tabs, as it should be

3de2d0c

Incorporate correct handling of EIP-155

2e08493

Turns out the Trezor was adjusting the v part of the signature, and we’re already doing that so it was done twice.

Some circular logic here that was incorrect

b5dc34e

BE-encoded U256 is almost the same as RLP encoded without the size-byte, except for <u8 sized values. What’s really done is BE-encoded U256 and then left-trimmed to the smallest size. Kind of obvious in hindsight.

Scan after pin entry to make accepting it faster

2763a2e

Remove ability to cancel pin request

5f20d51

Some slight cleanup

e07c275

Probe for the correct HID Version to determine padding

2677d90

Move the PinMatrix from Accounts to Application

f578cfe

Removing unused dependencies

32cfa2f

folsen added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. M7-ui labels Aug 28, 2017

jacogr reviewed Aug 28, 2017

View reviewed changes

folsen added 2 commits August 28, 2017 18:05

Mistake in copying over stuff from keepkey branch

fc22db3

Simplify FormattedMessage

ec58a6b

debris reviewed Aug 28, 2017

View reviewed changes

5chdn added the B7-releasenotes 📜 Changes should be mentioned in the release notes of the next minor version release. label Aug 29, 2017

folsen added 2 commits August 29, 2017 12:36

Move generated code to external crate

f344bca

Remove ethcore-util dependency

9255964

folsen added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Sep 11, 2017

tomusdrw reviewed Sep 12, 2017

View reviewed changes

tomusdrw added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Sep 12, 2017

folsen added 7 commits September 12, 2017 12:00

Merge branch 'master' into fh-4500-trezor-support

1f5d4f7

# Conflicts: # Cargo.lock # hardwareStore.js

Split the trezor RPC endpoint

56bc44d

It’s split into two more generic endpoints that should be suitable for any hardware wallets with the same behavior to sit behind.

Reflect RPC method split in javascript

7ff4c81

Fix bug with pin entry

7e0853c

Fix deadlock for Ledger

31e64c0

Avoid having a USB lock in just listing locked wallets

5463246

Fix javascript issue (see #6509)

66d9f3e

tomusdrw mentioned this pull request Sep 13, 2017

Fix detecting hardware wallets. #6510

Closed

folsen added 4 commits September 13, 2017 12:58

Replace Mutex with RwLock

8f965e8

Update Ledger test

918fce3

Fix typo causing faulty signatures (sometimes)

3df337d

*Actually* fix tests

203e121

folsen added A0-pleasereview 🤓 Pull request needs code review. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Sep 13, 2017

Update git submodule

e81ccbc

Needed to make tests pass

tomusdrw reviewed Sep 14, 2017

View reviewed changes

folsen added 2 commits September 14, 2017 11:24

Swap line orders to prevent possible deadlock

bfce1a1

Make setPinMatrixRequest an @action

c4d14ad

tomusdrw approved these changes Sep 14, 2017

View reviewed changes

tomusdrw added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Sep 14, 2017

gavofyork merged commit 75b6a31 into master Sep 14, 2017

gavofyork deleted the fh-4500-trezor-support branch September 14, 2017 17:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Trezor Support #6403

Trezor Support #6403

folsen commented Aug 28, 2017

parity-cla-bot commented Aug 28, 2017

jacogr Aug 28, 2017

jacogr Aug 28, 2017

folsen commented Aug 28, 2017

parity-cla-bot commented Aug 28, 2017

debris Aug 28, 2017

debris Aug 28, 2017

folsen Aug 28, 2017

debris Aug 28, 2017

folsen Aug 29, 2017

rphmeier Aug 29, 2017 •

edited

Loading

debris Aug 28, 2017

NikVolf Aug 29, 2017

folsen Aug 29, 2017

gavofyork commented Sep 11, 2017

tomusdrw commented Sep 12, 2017

tomusdrw left a comment

tomusdrw Sep 12, 2017

folsen Sep 12, 2017

tomusdrw Sep 12, 2017

tomusdrw Sep 12, 2017

tomusdrw Sep 12, 2017

tomusdrw Sep 12, 2017

tomusdrw left a comment

tomusdrw Sep 14, 2017

tomusdrw Sep 14, 2017

tomusdrw Sep 14, 2017

tomusdrw Sep 14, 2017

Trezor Support #6403

Trezor Support #6403

Conversation

folsen commented Aug 28, 2017

parity-cla-bot commented Aug 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

folsen commented Aug 28, 2017

parity-cla-bot commented Aug 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rphmeier Aug 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gavofyork commented Sep 11, 2017

tomusdrw commented Sep 12, 2017

tomusdrw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomusdrw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rphmeier Aug 29, 2017 •

edited

Loading