Skip to content
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

Ledger hardware wallet integration #8068

Merged
merged 8 commits into from Feb 7, 2020

Conversation

@CriesofCarrots
Copy link
Contributor

CriesofCarrots commented Jan 31, 2020

Problem

Solana users need to be able to manage their keys securely. In particular, they need a secure way to generate keys for genesis accounts. Enter hardware wallets.
Ledger app development is in progress (https://github.com/solana-labs/ledger-app-solana). This work is an initial implementation of CLI and keygen client integration.

Summary of Changes

  • Add hardware wallet module. Currently only Ledger is supported, but basic utilities and traits are designed to be extensible to other hardware wallets (eg Trezor) in the future.
  • Integrate Ledger into solana-keygen pubkey subcommand. Enables selection between multiple wallets. Hardware wallet use examples:
solana-keygen pubkey
E13eqyhs8uJvXtKfmTkt82tiEk1hbz3R2E7GgbMtEYBH
solana-keygen pubkey --derivation-path 1/12345
3Hafvf4Sqzj2DxxZJgTH6NBFCofBJzkfPXxymdPDh47b

Work Remaining

Integrate into solana-cli:

  • Make wallet load and selection generic (currently specific to keygen)
  • Enable hardware wallets in solana cli: address, airdrop, balance, create-address-with-seed
  • Enable hardware wallet choice in cli config

Other:

  • Rework hidapi -> use udev backend
  • Ship udev rules with ledger module -- Added ledger-udev bin, which must be run as sudo on linux
  • Update various constants and configuration based on ledger app development, esp error codes
@CriesofCarrots

This comment has been minimized.

Copy link
Contributor Author

CriesofCarrots commented Jan 31, 2020

@garious , I haven't yet found a unique identifier for the Ledger without querying the device. Each device does report a serial number, but both the Ledgers I currently have report the same number ("0001"), so I'm guessing that's not being set in manufacturing.
Any other thoughts?

@garious

This comment has been minimized.

Copy link
Member

garious commented Jan 31, 2020

solana-keygen pubkey hw

I don't like the hw argument. Instead, I'd like there to be some way to override the path in solana get's keypair. Maybe something like:

$ solana set --keypair usb://<name>
$ solana-keygen pubkey
E13eqyhs8uJvXtKfmTkt82tiEk1hbz3R2E7GgbMtEYBH

And also, that extra output should only be there if the user passes a --verbose option.

Regarding that syntax: usb://<name>, we'll need to think of something better. I've googled a bit for precedent, but nothing promising so far.

solana-keygen pubkey hw 1/12345

That's neat, but how about an option instead, --derivation-path=1/12345. Easy to imagine folks might never use that, and instead look for a simpler option --derivation-index that only accepts a number.

@CriesofCarrots

This comment has been minimized.

Copy link
Contributor Author

CriesofCarrots commented Jan 31, 2020

I don't like the hw argument. Instead, I'd like there to be some way to override the path in solana get's keypair.

Yes, I plan to implement something in the solana-cli config, as noted in the pr description. However, currently solana-keygen does not use/read the solana-cli config. This is a similar issue to that noted here: #7304. The CLI config is just for the cli. It is easy enough to have solana-keygen read that file. Is it enough that solana-cli is the only module that sets it?

And also, that extra output should only be there if the user passes a --verbose option.

Sure

That's neat, but how about an option instead, --derivation-path=1/12345. Easy to imagine folks might never use that, and instead look for a simpler option --derivation-index that only accepts a number.

Do you think the explicit --derivation-whatever args are necessary? If you just want that simpler option, you can do solana-keygen pubkey hw 123 right now.

@CriesofCarrots

This comment has been minimized.

Copy link
Contributor Author

CriesofCarrots commented Jan 31, 2020

usb://<name>, we'll need to think of something better. I've googled a bit for precedent, but nothing promising so far.

@garious usb://<manufacturer>/<model>/<base_pubkey>?
eg for this Nano S: usb://ledger/nano_s/E13eqyhs8uJvXtKfmTkt82tiEk1hbz3R2E7GgbMtEYBH

There is precedent for vendor_id:product_id, but that's pretty opaque.

@garious

This comment has been minimized.

Copy link
Member

garious commented Jan 31, 2020

I like usb://<manufacturer>/<model>/<base_pubkey>, but hyphenate the spaces instead of the underscore, since URLs generally use hyphens. Also, make it match such that: usb://ledger is sufficient if only one ledger device is connected, and usb://ledger/nano-s is sufficient if one Nano S and one Nano X is connected. Only look for the pubkey if it is given or if there are multiple Nano S's connected.

@garious

This comment has been minimized.

Copy link
Member

garious commented Feb 1, 2020

Regarding the argument or option, I think optional things should use options. I don't like the idea of modes, where if the keypair points to a USB, you're in "2 argument" mode. There's a reasonable default path, 0.

Cargo.toml Outdated Show resolved Hide resolved
@CriesofCarrots CriesofCarrots force-pushed the CriesofCarrots:ledger-int branch from d7d5ac0 to cd4f39a Feb 4, 2020
@CriesofCarrots

This comment has been minimized.

Copy link
Contributor Author

CriesofCarrots commented Feb 4, 2020

@garious , I think this is quite a bit closer. Current flow:

$ solana config set --keypair usb://<manufacturer>/<?model>/<?pubkey>/<?derivation-path>
$ solana-keygen pubkey
E13eqyhs8uJvXtKfmTkt82tiEk1hbz3R2E7GgbMtEYBH

// `usb://` path passed explicitly will override the cli config 
$ solana-keygen pubkey usb://ledger/nano-s/5YAHsQHFg6mfr4vhEPvbj1CWtXRDV9fKSV5U3y4hC1Ui
5YAHsQHFg6mfr4vhEPvbj1CWtXRDV9fKSV5U3y4hC1Ui

// derivation-path passed explicitly will override the cli config
$ solana-keygen pubkey --derivation-path 1/2
8vCmhV1XmrJfBYsYKPdE3Dyx6D2pSiTuTMhdrXRm3Veu

If the remote-wallet path (set in config or passed explicitly) matches more than one device (eg. usb://ledger/nano-s when you have 2 Nano S ledgers plugged in), the cli will prompt you to select between those devices.

@CriesofCarrots CriesofCarrots force-pushed the CriesofCarrots:ledger-int branch 5 times, most recently from c2f9eee to d650cac Feb 4, 2020
@CriesofCarrots

This comment has been minimized.

Copy link
Contributor Author

CriesofCarrots commented Feb 4, 2020

Here's example output from the solana-cli implementation:

$ solana balance --verbose

Keypair: usb://ledger/nano-s/E13eqyhs8uJvXtKfmTkt82tiEk1hbz3R2E7GgbMtEYBH/1
Pubkey: 9Uh4C9RokY8TQ592x9WEkp1J2kCMBt3iBX4whWEgDcCc
RPC Endpoint: http://testnet.solana.com:8899
9 SOL
@garious

This comment has been minimized.

Copy link
Member

garious commented Feb 4, 2020

Is that a derivation path at the end of the keypair URI?

@garious

This comment has been minimized.

Copy link
Member

garious commented Feb 4, 2020

Un that example, what is the out of solana config get?

@CriesofCarrots

This comment has been minimized.

Copy link
Contributor Author

CriesofCarrots commented Feb 4, 2020

Is that a derivation path at the end of the keypair URI?

Yes. Perhaps it should be stored in a different config setting(?), but it definitely seems useful to be able to set the specific pubkey for a whole batch of actions. Like solana-keygen, usb:// path or --derivation-path passed explicitly will override the cli config.

In that example, what is the out of solana config get?

Currently:

$ solana config get

Wallet Config: ~/.config/solana/cli/config.yml
* url: http://testnet.solana.com:8899
* keypair: usb://ledger/nano-s/E13eqyhs8uJvXtKfmTkt82tiEk1hbz3R2E7GgbMtEYBH/1
@CriesofCarrots CriesofCarrots force-pushed the CriesofCarrots:ledger-int branch from 45c6f43 to d650cac Feb 4, 2020
@garious

This comment has been minimized.

Copy link
Member

garious commented Feb 4, 2020

Sounds good to have the derivation path. Just make sure there's lots of unit tests. Things get super weird because a missing --derivation-path means different things in different contexts. Consider:

  1. File path, no --derivation-path
  2. File path, with --derivation-path
  3. USB path, no --derivation-path
  4. USB path, with --derivation-path

The awkward ones is the difference between #1 and #3. If a file path, then no derivation path means that you want the public key for the keypair in that file. If a USB path, then no derivation path means you want the public key at a default derivation path "0".

In any case, most important is that given the same seed phrase, #2 and #4 result in the same public key.

@garious

This comment has been minimized.

Copy link
Member

garious commented Feb 4, 2020

Regarding the verbose output, too many inconsistencies between solana config get and solana balance --verbose. The former says "Wallet Config" when it should say "config path", it uses lowercase field names whereas solana-balance uses uppercase. Both have lousy field names. "url" is too vague. "keypair" is imprecise. And one is a bulleted list, the other isn't. All this can be fixed in a separate PR, but should be cleaned up sooner than later.

@CriesofCarrots

This comment has been minimized.

Copy link
Contributor Author

CriesofCarrots commented Feb 4, 2020

Regarding the verbose output, too many inconsistencies between solana config get and solana balance --verbose. The former says "Wallet Config" when it should say "config path", it uses lowercase field names whereas solana-balance uses uppercase. Both have lousy field names. "url" is too vague. "keypair" is imprecise.

Fair! Predates this pr, so I'll fix separately and rebase.

@CriesofCarrots CriesofCarrots force-pushed the CriesofCarrots:ledger-int branch from d650cac to 88dc91e Feb 6, 2020

pub(crate) fn pubkey(&self) -> Result<Pubkey, Box<dyn std::error::Error>> {
if let Some(path) = &self.keypair_path {
if path.starts_with("usb://") {

This comment has been minimized.

Copy link
@garious

garious Feb 6, 2020

Member

This expression is repeated a bunch of times and written as those USB is the one special case. If other protocols are added, the filepath will be the syntactic special case. Better would be to:

match parse_keypair_path()?  {
    KeypairUrl::Filepath(keypair_path) => ...,
    KeypairUrl::Usb(stuff_after_usb_colon_slash_slash) => ...,
}

This comment has been minimized.

Copy link
@CriesofCarrots

CriesofCarrots Feb 7, 2020

Author Contributor

This is a great suggestion. However, I think it is closely related to the CLI refactor that needs to happen to handle various types of signers (present and not), so I'd like to handle it as part of that work.

This comment has been minimized.

Copy link
@garious

garious Feb 7, 2020

Member

sounds good

@CriesofCarrots CriesofCarrots force-pushed the CriesofCarrots:ledger-int branch from 88dc91e to 39c9959 Feb 6, 2020
@CriesofCarrots CriesofCarrots force-pushed the CriesofCarrots:ledger-int branch from 39c9959 to e06f755 Feb 7, 2020
@CriesofCarrots CriesofCarrots force-pushed the CriesofCarrots:ledger-int branch from e06f755 to 8e31707 Feb 7, 2020
@CriesofCarrots CriesofCarrots marked this pull request as ready for review Feb 7, 2020
@garious
garious approved these changes Feb 7, 2020
@codecov

This comment has been minimized.

Copy link

codecov bot commented Feb 7, 2020

Codecov Report

Merging #8068 into master will decrease coverage by 0.4%.
The diff coverage is 29.2%.

@@           Coverage Diff            @@
##           master   #8068     +/-   ##
========================================
- Coverage    81.8%   81.4%   -0.5%     
========================================
  Files         248     250      +2     
  Lines       53533   53988    +455     
========================================
+ Hits        43833   43980    +147     
- Misses       9700   10008    +308
@CriesofCarrots CriesofCarrots added the v0.23 label Feb 7, 2020
@CriesofCarrots CriesofCarrots merged commit ed0c1d3 into solana-labs:master Feb 7, 2020
13 checks passed
13 checks passed
Rule: v0.23 backport (backport) Backports have been created
Details
Summary 2 rules match and 2 potential rules
Details
buildkite/solana Build #18653 passed (1 hour, 23 minutes, 13 seconds)
Details
buildkite/solana/bench Passed (14 minutes, 22 seconds)
Details
buildkite/solana/checks Passed (2 minutes, 34 seconds)
Details
buildkite/solana/coverage Passed (18 minutes, 12 seconds)
Details
buildkite/solana/local-cluster Passed (23 minutes, 37 seconds)
Details
buildkite/solana/move Passed (4 minutes, 36 seconds)
Details
buildkite/solana/pipeline-upload Passed (2 seconds)
Details
buildkite/solana/shellcheck Passed (27 seconds)
Details
buildkite/solana/stable Passed (37 minutes, 46 seconds)
Details
buildkite/solana/stable-perf Passed (14 minutes, 12 seconds)
Details
ci-gate Pull Request accepted for CI pipeline
mergify bot pushed a commit that referenced this pull request Feb 7, 2020
* Initial remote wallet module

* Add clap derivation tooling

* Add remote-wallet path apis

* Implement remote-wallet in solana-keygen

* Implement remote-wallet in cli for read-only pubkey usage

* Linux: Use udev backend; add udev rules tool

* Ignore Ledger live test

* Cli api adjustments

(cherry picked from commit ed0c1d3)

# Conflicts:
#	Cargo.lock
#	clap-utils/Cargo.toml
#	cli/Cargo.toml
#	keygen/Cargo.toml
CriesofCarrots added a commit that referenced this pull request Feb 7, 2020
* Initial remote wallet module

* Add clap derivation tooling

* Add remote-wallet path apis

* Implement remote-wallet in solana-keygen

* Implement remote-wallet in cli for read-only pubkey usage

* Linux: Use udev backend; add udev rules tool

* Ignore Ledger live test

* Cli api adjustments

(cherry picked from commit ed0c1d3)
solana-grimes pushed a commit that referenced this pull request Feb 7, 2020
automerge
@CriesofCarrots CriesofCarrots deleted the CriesofCarrots:ledger-int branch Feb 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.