New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

accounts, console, internal: support trezor hardware wallet #14885

Merged
merged 3 commits into from Aug 14, 2017

Conversation

Projects
None yet
10 participants
@karalabe
Member

karalabe commented Aug 1, 2017

This PR adds support for the Trezor hardware wallet. Plain message signing is not yet supported as each wallet has a different spec for the signatures (#14794). I'll add that when an EIP is opened and a consensus is reached on how to sign arbitrary messages.


Opposed to the Ledger, the Trezor is a bit more complicated as it requires a PIN-unlock sent from the communicating machine instead of directly input by the user. As such, when a user plugs in a Trezor, Geth will print the following warning:

WARN [08-10|11:55:19] New wallet appeared, failed to open url=trezor://0003:0007:00 err="trezor: pin needed"

Opening the Trezor from Geth

The Geth console can be used to unlock the Trezor by invoking personal.openWallet(url), which will request the user to enter the shuffled PIN code and send that over to the Trezor for verification:

> personal.openWallet("trezor://0003:0007:00")

Look at the device for number positions

7 | 8 | 9
--+---+--
4 | 5 | 6
--+---+--
1 | 2 | 3

Please enter current PIN: <enter pin>

INFO [08-10|11:58:06] New wallet appeared   url=trezor://0003:0007:00 status="Trezor v1.5.0 'Quack' online"

Opening the Trezor from RPC

The Trezor can also be unlocked via remote RPC methods (to support external UI wallets) by calling personal_openWallet with the parameters [url, pin].

If pin == "", the Trezor is attempted to be opened without a PIN, which will either succeed (if it was already opened before), or it will display a PIN pad on the device and return a trezor: pin needed error. The UI doing the opening should react to this error by displaying a PIN entry UI element, and after reading the code from the user, call personal_openWallet again, with pin set to the user's input corresponding to a keyboard numpad layout (7-8-9 / 4-5-6 / 1-2-3).

Listing the Trezor wallets

The available Trezor (and other) wallets can be listed via personal_listWallets, which will also auto-derive Ethereum accounts which have non-zero balances or nonces:

> personal.listWallets

INFO [08-10|12:07:35] USB wallet discovered new account   url=trezor://0003:0007:00 address=0x7Bf042272737121236dcBC078A218646C94730f4 path=m/44'/60'/0'/0/0 balance=88999999999876 nonce=4
INFO [08-10|12:07:35] USB wallet discovered new account   url=trezor://0003:0007:00 address=0x89B613B665E9EA122EB1D54f3B54195160cAB8fb path=m/44'/60'/0'/0/1 balance=124            nonce=0
INFO [08-10|12:07:35] USB wallet discovered new account   url=trezor://0003:0007:00 address=0x6742064c46Ccc179C877Ea8782b5d5Fe5b033378 path=m/44'/60'/0'/0/2 balance=0              nonce=0

[{
    accounts: [{
        address: "0x7bf042272737121236dcbc078a218646c94730f4",
        url: "trezor://0003:0007:00/m/44'/60'/0'/0/0"
    }, {
        address: "0x89b613b665e9ea122eb1d54f3b54195160cab8fb",
        url: "trezor://0003:0007:00/m/44'/60'/0'/0/1"
    }, {
        address: "0x6742064c46ccc179c877ea8782b5d5fe5b033378",
        url: "trezor://0003:0007:00/m/44'/60'/0'/0/2"
    }],
    status: "Trezor v1.5.0 'Quack' online",
    url: "trezor://0003:0007:00"
}]

@karalabe karalabe added this to the 1.7.0 milestone Aug 9, 2017

Show outdated Hide outdated console/bridge.go
@nagydani

This comment has been minimized.

Show comment
Hide comment
@nagydani

nagydani Aug 10, 2017

Question: What are the events that should trigger adding new addresses to eth.accounts array? Since it is not specified, I do not know whether this implementation is correct or not, but I have my doubts.

Otherwise, the code is tidy and readable and none of the implemented functionality showed any erroneous behavior with the possible exception of adding accounts from the Trezor.

nagydani commented Aug 10, 2017

Question: What are the events that should trigger adding new addresses to eth.accounts array? Since it is not specified, I do not know whether this implementation is correct or not, but I have my doubts.

Otherwise, the code is tidy and readable and none of the implemented functionality showed any erroneous behavior with the possible exception of adding accounts from the Trezor.

@karalabe

This comment has been minimized.

Show comment
Hide comment
@karalabe

karalabe Aug 11, 2017

Member

I'm unsure what you mean by not correct. Accounts are auto-derived from the base path:

// DefaultBaseDerivationPath is the base path from which custom derivation endpoints
// are incremented. As such, the first account will be at m/44'/60'/0'/0, the second
// at m/44'/60'/0'/1, etc.

If an account has a non zero balance or non zero nonce, it will be added to the list of tracked accounts, and the next one is derived too. The account list is refreshed whenever balance changes are detected.

Member

karalabe commented Aug 11, 2017

I'm unsure what you mean by not correct. Accounts are auto-derived from the base path:

// DefaultBaseDerivationPath is the base path from which custom derivation endpoints
// are incremented. As such, the first account will be at m/44'/60'/0'/0, the second
// at m/44'/60'/0'/1, etc.

If an account has a non zero balance or non zero nonce, it will be added to the list of tracked accounts, and the next one is derived too. The account list is refreshed whenever balance changes are detected.

@karalabe

This comment has been minimized.

Show comment
Hide comment
@karalabe

karalabe Aug 11, 2017

Member

@nagydani You can also see the logic in the log I posted in the PR description:

INFO [08-10|12:07:35] USB wallet discovered new account   url=trezor://0003:0007:00 address=0x7Bf042272737121236dcBC078A218646C94730f4 path=m/44'/60'/0'/0/0 balance=88999999999876 nonce=4
INFO [08-10|12:07:35] USB wallet discovered new account   url=trezor://0003:0007:00 address=0x89B613B665E9EA122EB1D54f3B54195160cAB8fb path=m/44'/60'/0'/0/1 balance=124            nonce=0
INFO [08-10|12:07:35] USB wallet discovered new account   url=trezor://0003:0007:00 address=0x6742064c46Ccc179C877Ea8782b5d5Fe5b033378 path=m/44'/60'/0'/0/2 balance=0              nonce=0
Member

karalabe commented Aug 11, 2017

@nagydani You can also see the logic in the log I posted in the PR description:

INFO [08-10|12:07:35] USB wallet discovered new account   url=trezor://0003:0007:00 address=0x7Bf042272737121236dcBC078A218646C94730f4 path=m/44'/60'/0'/0/0 balance=88999999999876 nonce=4
INFO [08-10|12:07:35] USB wallet discovered new account   url=trezor://0003:0007:00 address=0x89B613B665E9EA122EB1D54f3B54195160cAB8fb path=m/44'/60'/0'/0/1 balance=124            nonce=0
INFO [08-10|12:07:35] USB wallet discovered new account   url=trezor://0003:0007:00 address=0x6742064c46Ccc179C877Ea8782b5d5Fe5b033378 path=m/44'/60'/0'/0/2 balance=0              nonce=0
@karalabe

This comment has been minimized.

Show comment
Hide comment
@karalabe

karalabe Aug 11, 2017

Member

Also note, there's a personal.deriveAccount(url, path, keep) method which can derive arbitrary derivation paths on a hardware device identified by the URL and can optionally keep it as a member of the accounts array.

Member

karalabe commented Aug 11, 2017

Also note, there's a personal.deriveAccount(url, path, keep) method which can derive arbitrary derivation paths on a hardware device identified by the URL and can optionally keep it as a member of the accounts array.

@nagydani nagydani self-requested a review Aug 14, 2017

@nagydani

The PR in its present form works with Trezor correctly and I am satisfied with the implementation. Could not verify Ledger support regressions, as I do not have that HW.

@karalabe karalabe merged commit ef0edc6 into ethereum:master Aug 14, 2017

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
commit-message-check/gitcop All commit messages are valid
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@prusnak

This comment has been minimized.

Show comment
Hide comment
@prusnak

prusnak Aug 14, 2017

Contributor

Thank you, Péter!

Contributor

prusnak commented Aug 14, 2017

Thank you, Péter!

@carver

This comment has been minimized.

Show comment
Hide comment
@carver

carver Aug 15, 2017

Contributor

Just to be clear, any "gap" in activity in your accounts will prevent discovery of later accounts, right? Like:

m/44'/60'/0'/0 - activity - discovered
m/44'/60'/0'/1 - activity - discovered
m/44'/60'/0'/2 - no activity - not discovered
m/44'/60'/0'/3 - activity - not discovered

(where "activity" is a nonzero balance or nonce)

This seems reasonable, just wanted to be sure I understood.

Contributor

carver commented Aug 15, 2017

Just to be clear, any "gap" in activity in your accounts will prevent discovery of later accounts, right? Like:

m/44'/60'/0'/0 - activity - discovered
m/44'/60'/0'/1 - activity - discovered
m/44'/60'/0'/2 - no activity - not discovered
m/44'/60'/0'/3 - activity - not discovered

(where "activity" is a nonzero balance or nonce)

This seems reasonable, just wanted to be sure I understood.

@prusnak

This comment has been minimized.

Show comment
Hide comment
@prusnak

prusnak Aug 15, 2017

Contributor
Contributor

prusnak commented Aug 15, 2017

@woodydeck

This comment has been minimized.

Show comment
Hide comment
@woodydeck

woodydeck Dec 27, 2017

On OSX, how do you find the URL? It's not documented anywhere, but maybe it is a bug. The url shown is not correct and/or truncated.

WARN [12-26|20:37:23] New wallet appeared, failed to open url=trezor://IOService:/AppleACPIPl… err="trezor: pin needed"

personal.openWallet("trezor://IOServiceAppleACPIPl") gets you a 'Error: unknown wallet' error.

woodydeck commented Dec 27, 2017

On OSX, how do you find the URL? It's not documented anywhere, but maybe it is a bug. The url shown is not correct and/or truncated.

WARN [12-26|20:37:23] New wallet appeared, failed to open url=trezor://IOService:/AppleACPIPl… err="trezor: pin needed"

personal.openWallet("trezor://IOServiceAppleACPIPl") gets you a 'Error: unknown wallet' error.

@karalabe

This comment has been minimized.

Show comment
Hide comment
@karalabe

karalabe Dec 27, 2017

Member

You can list all the details with personal.listWallets()

Member

karalabe commented Dec 27, 2017

You can list all the details with personal.listWallets()

@woodydeck

This comment has been minimized.

Show comment
Hide comment
@woodydeck

woodydeck Dec 27, 2017

> personal.openWallet("trezor://1111:1111:11")
Error: unknown wallet
> personal.listWallets()
TypeError: 'listWallets' is not a function
    at <anonymous>:1:1

I'm on geth 1.8/go 1.9.2, running latest High Sierra.

woodydeck commented Dec 27, 2017

> personal.openWallet("trezor://1111:1111:11")
Error: unknown wallet
> personal.listWallets()
TypeError: 'listWallets' is not a function
    at <anonymous>:1:1

I'm on geth 1.8/go 1.9.2, running latest High Sierra.

@iinstances

This comment has been minimized.

Show comment
Hide comment
@iinstances

iinstances Jan 7, 2018

$ geth version
Geth
Version: 1.7.3-stable
Architecture: amd64
Protocol Versions: [63 62]
Network Id: 1
Go Version: go1.9
Operating System: linux

$ geth attach 

> personal.openWallet("trezor://0001:000f:00")
TypeError: 'openWallet' is not a function

> personal.listWallets
undefined
> personal.listWallets()
TypeError: 'listWallets' is not a function
    at <anonymous>:1:1

Checked in console mode instead of attach and work fine now.

iinstances commented Jan 7, 2018

$ geth version
Geth
Version: 1.7.3-stable
Architecture: amd64
Protocol Versions: [63 62]
Network Id: 1
Go Version: go1.9
Operating System: linux

$ geth attach 

> personal.openWallet("trezor://0001:000f:00")
TypeError: 'openWallet' is not a function

> personal.listWallets
undefined
> personal.listWallets()
TypeError: 'listWallets' is not a function
    at <anonymous>:1:1

Checked in console mode instead of attach and work fine now.

@woodydeck

This comment has been minimized.

Show comment
Hide comment
@woodydeck

woodydeck Jan 7, 2018

Also, geth automatically hijacks the process, so you can't use it with MEW or the Chrome plugin with geth running, which is annoying because I want to keep Swarm and geth running 24/7.

woodydeck commented Jan 7, 2018

Also, geth automatically hijacks the process, so you can't use it with MEW or the Chrome plugin with geth running, which is annoying because I want to keep Swarm and geth running 24/7.

@tomjohn1028

This comment has been minimized.

Show comment
Hide comment
@tomjohn1028

tomjohn1028 Jan 29, 2018

@woodydeck I'm experiencing the same thing. I'm trying to use a locally running MEW with my local node but because Geth instantly tries to connect no other process can connect with the Trezor. Is there any way around this?

tomjohn1028 commented Jan 29, 2018

@woodydeck I'm experiencing the same thing. I'm trying to use a locally running MEW with my local node but because Geth instantly tries to connect no other process can connect with the Trezor. Is there any way around this?

@ngutman

This comment has been minimized.

Show comment
Hide comment
@ngutman

ngutman Apr 11, 2018

@karalabe I dug around a bit on the geth/trezor integration and I wonder if it's still working (specifically I'm trying to get ethsign to work with trezor which relies on it being accessible in geth). I manage to enter the pin in geth console but I don't understand where and how to enter the passphrase (which is required after entering the pin)

ngutman commented Apr 11, 2018

@karalabe I dug around a bit on the geth/trezor integration and I wonder if it's still working (specifically I'm trying to get ethsign to work with trezor which relies on it being accessible in geth). I manage to enter the pin in geth console but I don't understand where and how to enter the passphrase (which is required after entering the pin)

@karalabe

This comment has been minimized.

Show comment
Hide comment
@karalabe

karalabe Apr 11, 2018

Member

I have not implemented passwords for the Trezor. Didn't know that that was a generally desired use case. Will do when I get to integrating the Trezor T.

Member

karalabe commented Apr 11, 2018

I have not implemented passwords for the Trezor. Didn't know that that was a generally desired use case. Will do when I get to integrating the Trezor T.

@ncsolar

This comment has been minimized.

Show comment
Hide comment
@ncsolar

ncsolar May 2, 2018

@karalabe Excellent to know the plan to implement passwords for the Trezor, thanks!

ncsolar commented May 2, 2018

@karalabe Excellent to know the plan to implement passwords for the Trezor, thanks!

@fjrojasgarcia

This comment has been minimized.

Show comment
Hide comment
@fjrojasgarcia

fjrojasgarcia May 2, 2018

Any plan to implement local Hardware Wallets (Trezor/Ledger) recognition from an attached ws/rpc geth instance?

Is this functionality already implemented?
Would it break some security policy?

IMHO It seems an interesting use case. You could attach to a remote node via ws/rpc (avoiding the consumption of local resources) without losing the ability to continue using your local USB connected Ledger or Trezor from the awesome geth console.

fjrojasgarcia commented May 2, 2018

Any plan to implement local Hardware Wallets (Trezor/Ledger) recognition from an attached ws/rpc geth instance?

Is this functionality already implemented?
Would it break some security policy?

IMHO It seems an interesting use case. You could attach to a remote node via ws/rpc (avoiding the consumption of local resources) without losing the ability to continue using your local USB connected Ledger or Trezor from the awesome geth console.

@fjrojasgarcia

This comment has been minimized.

Show comment
Hide comment
@fjrojasgarcia

fjrojasgarcia May 2, 2018

Added Hardware Wallets support wiki entry at forked repo.

fjrojasgarcia commented May 2, 2018

Added Hardware Wallets support wiki entry at forked repo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment