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

[for review] TREZOR: initial integration proposal #4241

Merged
merged 1 commit into from Nov 4, 2018

Conversation

Projects
None yet
8 participants
@ph4r05
Copy link
Contributor

ph4r05 commented Aug 10, 2018

Hi,

in this PR I would love to discuss current TREZOR integration approach in the similar manner Ledger did.

As TREZOR has some differences from Ledger we took a bit different approach when designing the protocols. The main idea is the monero-wallet-cli <-> Trezor communication is similar to the hot-wallet <-> cold wallet communication.

I believe that by choosing a bit more high-level approach in the protocol design we could easily add more advanced features, such as multisigs later during development. I also see security analysis of the protocol simpler as there are only a few well-defined roundtrips and in case of transaction signing. The transaction is incrementally built by Trezor completely. Moreover, the design is not new as the cold wallet features are already implemented in the code thus we can reuse existing features and design.

I tried to document transaction signing protocol on the following link.
https://github.com/ph4r05/monero-trezor-doc

Trezor has already PR merged implementing the Trezor side of the protocol:
trezor/trezor-core#293

Original integration approach was to have a python wallet that mimics monero-wallet-cli interface which is implemented here: https://github.com/ph4r05/monero-agent
The monero-agent starts monero-wallet-rpc under the hood and uses it as a watch-only component for building transaction construction data.

However, it would be quite nice to increase users comfort of the original monero-wallet-cli by integrating TREZOR with monero code directly.

If you would like to test the code with Trezor emulator I can post more detailed information.

Thanks for feedback!


Updates

  • Trezor firmware v2.0.9 does not correctly encrypt short payment ID in outgoing transactions. This firmware version should not be used for sending transactions with short payment IDs and integrated addresses (more than 95 characters). No funds are lost but a recipient of your payment will have problems pairing the payment (e.g., exchanges). Please update the firmware as soon as it becomes available. More technical information here: trezor/trezor-core#426. The current master checks the firmware version in the monero-wallet-cli and blocks short payment ID transactions if the firmware is <= 2.0.9.

Compilation

The Trezor support requires python3 and protobuf library to compile. If all requirements are met the cmake log will contain:

-- Trezor support enabled

If it is not the case, there could be some missing dependency.

If you are having an issue building the Trezor support make sure the all submodules are up to date

git submodule update --init --recursive

And install the python3 and protobuf dependencies:

sudo apt-get install protobuf-compiler libprotobuf-dev  libusb-1.0-0-dev  # ubuntu
brew install protobuf  libusb  # OSX

Usage info

Wallet restore

The first step is to create a wallet file using the device (restore).

./monero-wallet-cli --hw-device Trezor --generate-from-device ~/wallet_file --restore-height 10000 --log-file ~/monero.log  --log-level 3

On the first start, the monero-wallet-cli asks you to export a view-only key from the Trezor to the wallet key file. After this step, the monero wallet can perform the blockchain scanning effectively. (Note: in this version, we did not implement view key scanning on the device as it is slow and and bit impractical).

It is preferable to use --restore-height option to speed up the sync process.

Key images sync (restore used wallet)

If you are recovering wallet that spent some inputs already you also need to perform key image sync. Trezor implements cold wallet signing protocol thus the same logic applies here as if you were using cold wallet on an offline host. Monero wallet does not have the spend-key so it cannot compute key images and discover whether you spent the incoming funds or not. Thus if you are getting double spend errors you need to perform key image sync.

Key image sync is performed with a command hw_key_images_sync from the wallet console. You need to confirm this action on the host. It may take a while.

Using existing restored wallet file

In order to use the wallet restored from the device you can use:

./monero-wallet-cli --wallet ~/wallet_file --log-file ~/monero.log --log-level 3

If there is any problem take a look at the log file for more detailed information and a problem cause.

The integration can be tested both with the Trezor emulator and real Trezor T device for which you need to install the Trezor Bridge. Just follow the instructions on https://trezor.io/start/

Troubleshooting

Logging

It is always better to increase the log level and to inspect log files as there are more concrete reasons of failure. Logs also show detected Trezor devices.

Start the monero-wallet-cli with the following --log-file ~/monero.log --log-level 3 and inspect the last log lines.

Restart Trezor bridge

After each error it is better to kill trezor bridge so it releases all sessions. sudo pkill trezord. It restarts automatically.

@ph4r05

This comment has been minimized.

Copy link
Contributor Author

ph4r05 commented Aug 10, 2018

I will refactor linking dependencies in device_trezor.
I need to rethink the design to get rid of the cyclic linking dependencies.

@ph4r05 ph4r05 force-pushed the ph4r05:master-trezor branch from 6133a77 to 8fb0aff Aug 10, 2018

@ph4r05

This comment has been minimized.

Copy link
Contributor Author

ph4r05 commented Aug 10, 2018

ok I have it fixed, will commit soon

@ph4r05 ph4r05 force-pushed the ph4r05:master-trezor branch 2 times, most recently from b46ed48 to 60bf1d3 Aug 12, 2018

@ph4r05 ph4r05 changed the title [DO NOT MERGE] TREZOR: initial integration proposal [for review] TREZOR: initial integration proposal Aug 13, 2018

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor

moneromooo-monero commented Aug 14, 2018

That's a huge amount of stuff, it'd be nice to start by PRing self contained building blocks like the poly1305 code first.
Also: why an extra JSON lib?

@ph4r05 ph4r05 force-pushed the ph4r05:master-trezor branch from 60bf1d3 to c32eedd Aug 14, 2018

@ph4r05

This comment has been minimized.

Copy link
Contributor Author

ph4r05 commented Aug 14, 2018

@moneromooo-monero thanks for the reply.

So far I can see only 3 basic building blocks, logically separable to separate PRs:

  • poly1305 as you mentioned
  • small patch to add incremental API to Keccak
  • small patch to add incremental API to Chacha

This PR may seem large but the vast majority of the PR are generated protobuf messages in src/device_trezor/trezor/messages with 47476 lines of code (93%), the whole src/device_trezor has 51038 lines of code, so there are only 3562 lines of non-generated code for the TREZOR.

There are also some small additions to wallet2, simplewallet, account etc. mainly adding support for another device (TREZOR) and few minor things, such as cold signing protocol. These are IMO good to keep in the same PR as the device_trezor.

JSON library - this PR was meant as a prototype to align some expectations, not for direct merge. Thus I used a single-file header-only JSON library which is very easy to work with (rapidjson is a bit more complicated). The one I used is also used in the general TREZOR C++ client library. But to make it clearer now I refactored the PR to use rapidjson.

So 3 separate PR (keccak, chacha, poly1305) and then this one?

Thanks!

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor

moneromooo-monero commented Aug 14, 2018

A bit subjective, but my own opinion is that a good split is one first PR with the chacha/keccak/poly1305, either one or three commits.

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor

moneromooo-monero commented Aug 14, 2018

Also, we depend on libsodium nowadays, so maybe poly1305 can be used from there ?

@ph4r05

This comment has been minimized.

Copy link
Contributor Author

ph4r05 commented Aug 14, 2018

I will check what I can use from libsodium. Thanks

@ph4r05 ph4r05 force-pushed the ph4r05:master-trezor branch from c32eedd to 7e0b70a Aug 14, 2018

@ph4r05

This comment has been minimized.

Copy link
Contributor Author

ph4r05 commented Aug 14, 2018

OK I've removed incremental chacha API and poly1305 as I can use libsodium crypto_aead_chacha20poly1305_ietf_decrypt

@ph4r05 ph4r05 force-pushed the ph4r05:master-trezor branch from 7e0b70a to d5a0fe9 Aug 14, 2018

@ph4r05

This comment has been minimized.

Copy link
Contributor Author

ph4r05 commented Aug 14, 2018

Incremental Keccak was added also separately. I will remove it from this commit after 4259 gets merged.

#4259

@ph4r05 ph4r05 force-pushed the ph4r05:master-trezor branch 9 times, most recently from 7d76441 to 2aa2af2 Aug 16, 2018

@ph4r05 ph4r05 force-pushed the ph4r05:master-trezor branch 4 times, most recently from 0898c05 to e0680f7 Aug 23, 2018

@ph4r05

This comment has been minimized.

Copy link
Contributor Author

ph4r05 commented Aug 23, 2018

Part of this PR separated to a new one:

#4299

@ph4r05 ph4r05 force-pushed the ph4r05:master-trezor branch 2 times, most recently from 6db1a21 to 8417c37 Oct 30, 2018

@ph4r05

This comment has been minimized.

Copy link
Contributor Author

ph4r05 commented Oct 31, 2018

Soo I think this PR is more or less complete, I wouldn't add more features to this one.
Another comments, reviews? :)

@prusnak

This comment has been minimized.

Copy link

prusnak commented Oct 31, 2018

I confirm that commit 8417c37 works perfectly (both receiving and sending).

Show resolved Hide resolved src/simplewallet/simplewallet.cpp Outdated
Show resolved Hide resolved src/simplewallet/simplewallet.cpp Outdated
@mariodian

This comment has been minimized.

Copy link

mariodian commented Oct 31, 2018

I confirm that commit 8417c37 works perfectly (both receiving and sending).

Is there any quick tutorial on how to test it? Simply connecting Trezor and monero-wallet-cli --generate-from-device does nothing (it says ledger is not found).

@ph4r05

This comment has been minimized.

Copy link
Contributor Author

ph4r05 commented Oct 31, 2018

Is there any quick tutorial on how to test it? Simply connecting Trezor and monero-wallet-cli --generate-from-device does nothing (it says ledger is not found).

I've added a simple tutorial to the PR description.

@ph4r05 ph4r05 force-pushed the ph4r05:master-trezor branch from 8417c37 to 016cd25 Oct 31, 2018

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor

moneromooo-monero commented Oct 31, 2018

The commit message seems to have some long history of all wip commits, is that intended ?

Also, that's where any tutorial should be, since it'll be in git. Nobody will find the PR on github when that gets merged.

@ph4r05

This comment has been minimized.

Copy link
Contributor Author

ph4r05 commented Oct 31, 2018

The commit message seems to have some long history of all wip commits, is that intended ?

I keep it here to keep track of squashed commits. I can remove it after we are done with the review if that is OK for you.

@mariodian

This comment has been minimized.

Copy link

mariodian commented Nov 1, 2018

I've added a simple tutorial to the PR description.

Thanks! Sorry I somehow overlooked it and was looking all over your repos.

@mariodian

This comment has been minimized.

Copy link

mariodian commented Nov 1, 2018

Doesn't work for me.

I've installed the latest trezor bridge (2.0.24) from https://beta-wallet.trezor.io/#/bridge and unlocked the Trezor, but when I ran ./monero-wallet-cli --hw-device Trezor --generate-from-device trezor --restore-height=1695219 --log-level 3 --trusted-daemon --daemon-address my.remote.node:port --daemon-login user:pass it gives me the following error:

2018-11-01 02:47:47.034      0x11b2bb5c0        INFO    msgwriter       src/common/scoped_message_writer.h:102  Monero 'Beryllium Bullet' (v0.13.0.0-master-016cd25d)
2018-11-01 02:47:47.034      0x11b2bb5c0        INFO    wallet.wallet2  src/wallet/wallet_args.cpp:207  Setting log level = 3
2018-11-01 02:47:47.034      0x11b2bb5c0        INFO    wallet.wallet2  src/wallet/wallet_args.cpp:210  Logging to: ./monero-wallet-cli.log
2018-11-01 02:47:47.034      0x11b2bb5c0        INFO    msgwriter       src/common/scoped_message_writer.h:102  Logging to ./monero-wallet-cli.log
2018-11-01 02:47:51.805      0x11b2bb5c0        INFO    wallet.wallet2  src/wallet/wallet2.cpp:6320     ringdb path set to /Users/mariodian/.shared-ringdb
2018-11-01 02:47:51.832      0x11b2bb5c0        ERROR   default src/device/device.cpp:86        **Device not found in registry: 'Trezor'.** Known devices:
2018-11-01 02:47:51.832      0x11b2bb5c0        ERROR   default src/device/device.cpp:88         - default
2018-11-01 02:47:51.832      0x11b2bb5c0        ERROR   msgwriter       src/common/scoped_message_writer.h:102  Error: failed to generate new wallet: device not found: Trezor
2018-11-01 02:47:51.832      0x11b2bb5c0        ERROR   wallet.simplewallet     src/simplewallet/simplewallet.cpp:3364  account creation failed
2018-11-01 02:47:51.832      0x11b2bb5c0        ERROR   wallet.simplewallet     src/simplewallet/simplewallet.cpp:8290  Failed to initialize wallet
2018-11-01 02:47:51.832      0x11b2bb5c0        DEBUG   net     contrib/epee/include/net/net_helper.h:513       Problems at cancel: Bad file descriptor
2018-11-01 02:47:51.833      0x11b2bb5c0        DEBUG   net     contrib/epee/include/net/net_helper.h:516       Problems at shutdown: Bad file descriptor

Am I missing something? I'm using the original Trezor cable which works with the web wallet with no issues.

@ph4r05

This comment has been minimized.

Copy link
Contributor Author

ph4r05 commented Nov 1, 2018

Doesn't work for me.

It seems the Trezor support is not built by the log. When building the Monero you have to have the following lines in the cmake log

-- Trezor protobuf messages regenerated 
-- Trezor support enabled

If it is not the case, please provide the build log. There could be some missing dependency.

Make sure the all submodules are up to date

git submodule update --init --recursive

And install the python3 and protobuf dependencies:

sudo apt-get install protobuf-compiler libprotobuf-dev
@mariodian

This comment has been minimized.

Copy link

mariodian commented Nov 1, 2018

@ph4r05 thanks! Installing protobuf worked. Btw I was compiling on MacOS so the command was brew install protobuf in my case.

@mariodian

This comment has been minimized.

Copy link

mariodian commented Nov 1, 2018

Is it a bug that Trezor passphrase can only be typed on the device not the host or is it not implemented yet?

@ph4r05

This comment has been minimized.

Copy link
Contributor Author

ph4r05 commented Nov 1, 2018

Is it a bug that Trezor passphrase can only be typed on the device not the host or is it not implemented yet?

In the first PR the passphrase can be entered only on the Trezor. In the following PRs this will be addressed.

@ph4r05 ph4r05 force-pushed the ph4r05:master-trezor branch from 016cd25 to c1df8cc Nov 1, 2018

@ph4r05

This comment has been minimized.

Copy link
Contributor Author

ph4r05 commented Nov 1, 2018

@moneromooo-monero big kudos for the review!

I've just changed the commit message so it does not contain long history of WIP commit messages.

@ph4r05 ph4r05 force-pushed the ph4r05:master-trezor branch from c1df8cc to 0aec5cf Nov 2, 2018

@ph4r05

This comment has been minimized.

Copy link
Contributor Author

ph4r05 commented Nov 2, 2018

Rebased

@ph4r05 ph4r05 force-pushed the ph4r05:master-trezor branch from 0aec5cf to 29ffb6b Nov 2, 2018

@fluffypony
Copy link
Collaborator

fluffypony left a comment

Reviewed

@fluffypony fluffypony merged commit 29ffb6b into monero-project:master Nov 4, 2018

6 of 10 checks passed

buildbot/monero-linux-armv7 Build done.
Details
buildbot/monero-static-ubuntu-i686 Build done.
Details
buildbot/monero-static-win32 Build done.
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
buildbot/monero-linux-armv8 Build done.
Details
buildbot/monero-static-osx-10.11 Build done.
Details
buildbot/monero-static-osx-10.12 Build done.
Details
buildbot/monero-static-osx-10.13 Build done.
Details
buildbot/monero-static-ubuntu-amd64 Build done.
Details
buildbot/monero-static-win64 Build done.
Details

fluffypony added a commit that referenced this pull request Nov 4, 2018

Merge pull request #4241
29ffb6b device/trezor: trezor support added (Dusan Klinec)
@ph4r05

This comment has been minimized.

Copy link
Contributor Author

ph4r05 commented Nov 4, 2018

Wow, thanks @fluffypony @moneromooo-monero for the review & merge!

I am now preparing another PR which will add integration tests & GUI support.

@ph4r05 ph4r05 referenced this pull request Nov 6, 2018

Open

[WIP] Trezor integration roadmap #4810

19 of 29 tasks complete
@ph4r05

This comment has been minimized.

Copy link
Contributor Author

ph4r05 commented Nov 28, 2018

Update regarding the firmware v2.0.9 added to the PR description.

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