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

Add CLI command for retrieving root private keys from mnemonics #1316

Closed
7 tasks done
Anviking opened this issue Jan 29, 2020 · 12 comments
Closed
7 tasks done

Add CLI command for retrieving root private keys from mnemonics #1316

Anviking opened this issue Jan 29, 2020 · 12 comments
Assignees

Comments

@Anviking
Copy link
Collaborator

Anviking commented Jan 29, 2020

Context

ADP-81

It is currently possible to retrieve the reward account private key of a (shelley) mnemonic.

We want to make it possible to expose the root private key (and later address keys) given a mnemonic. And not just for shelley.

Decision

As a meaningful first step in the story, add command(s) for retrieving the root private key.

Acceptance Criteria


  • We must be able to retrieve the root xprvs of Byron-, Icarus-, Trezor- and ledger-wallets.
  • Keys should be output as hexadecimal strings
  • The output keys should be extended keys
  • The keys may be compatible with jcli

Development

QA

@Anviking
Copy link
Collaborator Author

@piotr-iohk @paweljakubas any thoughts on the following plan:

Extract root xprvs (this ticket)

using same flow as the existing reward-credentials, i.e
Prompts user to enter mnemonic in stdin after executing the command

cardano-wallet-jormungandr mnemonic extract-root-xprv --type shelley
cardano-wallet-jormungandr mnemonic extract-root-xprv --type byron
cardano-wallet-jormungandr mnemonic extract-root-xprv --type icarus
cardano-wallet-jormungandr mnemonic extract-root-xprv --type ledger
cardano-wallet-jormungandr mnemonic extract-root-xprv --type trezor

Extract address keys (not this ticket technically)

# Extracting the 100th address xprv for the mnemonic's default account
cardano-wallet-jormungandr mnemonic extract-address-xprv 100 --type byron

Pros:

  • Easy to use
  • No mnemonics in terminal history (I believe is the idea, = secure)
  • When deriving an address key, there is no intermediary step of having the root private key in the terminal output or on disk (less likely to leak root private key)

Cons:

  • Fairly limited in uses
  • Not that composable. Adding more features might come unnatural.
  • No support for deriving public keys from the mnemonic
  • No support for doing soft derivation (root/account xpub -> address xpub )

@paweljakubas
Copy link
Contributor

I think that the fact that it uses the same flow as the existing reward-credentials is very strong argument in favor. Prompting to enter mnemonics in stdin also seems right - I agree here.

@piotr-iohk
Copy link
Contributor

lgtm.

Question about the part: Extract address keys (not this ticket technically)
Is there a way to get key for a specific address rather than which in line?

I mean like:

cardano-wallet-jormungandr mnemonic extract-address-xprv addr1s0.....ljk38k3la --type byron

I think that would be more useful?

@Anviking
Copy link
Collaborator Author

Anviking commented Jan 31, 2020

That is an interesting idea!

I believe this should be fairly straight-forward to do with random addresses (e.g. byron).

For sequential addresses we'd need to generate a range (say for indices [0, 1000]) of keys, and check if each corresponds to the address.

Maybe that could even help this user brute-force search for his missing seed word.

@Anviking
Copy link
Collaborator Author

Anviking commented Feb 4, 2020

For the record: New requirements from Matthias in ADP-81

For this ticket this means taking the mnemonic words as arguments instead of via a secure getLine.

Regarding "Extract address keys (not this ticket technically)", this will be more generic. From the provided examples:

$ key child --path 44'/1815'/0'/0 5bd164a28...1ae275939
  0d0a62eef...6f2aee621

iohk-bors bot added a commit that referenced this issue Feb 14, 2020
1343: Introduce SomeMnemonic as source of root keys (instead of entropy) r=Anviking a=Anviking

# Issue Number

#1316, preliminary work to unblock #1321 
<!-- Put here a reference to the issue this PR relates to and which requirements it tackles -->


# Overview

- Add SomeMnemonic as return type of fromMnemonic
- Make e.g. unsafeGenerateKeyFromSeed take a SomeMnemonic instead
entropy. This is more similar to `Icarus.generateKeyFromHardwareLedger`
- Add genMnemonic helper in a shared location

# Comments

<!-- Additional comments or screenshots to attach if any -->

<!-- 
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Once created, link this PR to its corresponding ticket
 ✓ Assign the PR to a corresponding milestone
 ✓ Acknowledge any changes required to the Wiki
-->


Co-authored-by: Johannes Lund <johannes.lund@iohk.io>
iohk-bors bot added a commit that referenced this issue Feb 14, 2020
1343: Introduce SomeMnemonic as source of root keys (instead of entropy) r=Anviking a=Anviking

# Issue Number

#1316, preliminary work to unblock #1321 
<!-- Put here a reference to the issue this PR relates to and which requirements it tackles -->


# Overview

- Add SomeMnemonic as return type of fromMnemonic
- Make e.g. unsafeGenerateKeyFromSeed take a SomeMnemonic instead
entropy. This is more similar to `Icarus.generateKeyFromHardwareLedger`
- Add genMnemonic helper in a shared location

# Comments

<!-- Additional comments or screenshots to attach if any -->

<!-- 
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Once created, link this PR to its corresponding ticket
 ✓ Assign the PR to a corresponding milestone
 ✓ Acknowledge any changes required to the Wiki
-->


Co-authored-by: Johannes Lund <johannes.lund@iohk.io>
iohk-bors bot added a commit that referenced this issue Feb 15, 2020
1343: Introduce SomeMnemonic as source of root keys (instead of entropy) r=Anviking a=Anviking

# Issue Number

#1316, preliminary work to unblock #1321 
<!-- Put here a reference to the issue this PR relates to and which requirements it tackles -->


# Overview

- Add SomeMnemonic as return type of fromMnemonic
- Make e.g. unsafeGenerateKeyFromSeed take a SomeMnemonic instead
entropy. This is more similar to `Icarus.generateKeyFromHardwareLedger`
- Add genMnemonic helper in a shared location

# Comments

<!-- Additional comments or screenshots to attach if any -->

<!-- 
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Once created, link this PR to its corresponding ticket
 ✓ Assign the PR to a corresponding milestone
 ✓ Acknowledge any changes required to the Wiki
-->


Co-authored-by: Johannes Lund <johannes.lund@iohk.io>
iohk-bors bot added a commit that referenced this issue Feb 26, 2020
1369: Add unXPrvStripPub (& inverse) that matches jcli r=Anviking a=Anviking

# Issue Number

#1316 

# Overview

- [x] Added
```haskell
unXPrvStripPub :: XPrv -> Either ErrUnXPrvStripPub ByteString
xPrvFromStrippedPubXPrv :: ByteString -> Either ErrXPrvFromStrippedPubXPrv XPrv
```

To convert `XPrv` (`prv <> pub <> cc`) to a 96-byte bytestring of the form `prv <> cc`, and back.

Unusually, _both_ functions may fail. `unXPrvStripPub` fails if the resulting byte string cannot be converted back to the _same_ `XPrv` using `xPrvFromStrippedPubXPrv` (i.e. roundtrip)

- [x] Roundtrip properties.
    - `either roundtrips or fails (if xprv is encrypted)`
    - `(xPrvFromStrippedPubXPrv bs) fails if (BS.length bs) /= 96`
- [x] Integration test verifying that this format works with jcli

# Comments

- We need to convert between 96-byte long hex-encoded bytestrings and
XPrvs when implementing the `key root` and `key child` CLI commands.

Example showing that we cannot `unXPrvStripPub` encrypted XPrvs:
```haskell
λ> bytes = "(\134\242|I\141L\EM\NUL\128\173\252q\191\172\167>f \218\222\167.\136\DC4\216\191\253r8cD8&I\STX;\185&\177\172E\241\185\241\157\226\r\163+\EM\GS\232-\188\250[E^N\129J\158\STX\242s\215\149\142\217;\168P\DLE\159jHO\149'\ENQ\ETX!\129\CAN\140\176\221\DEL'*\DC4=<B\226\134\188!\241\DLEzt\222\199\247U\143\ETB\128,\226Q\"\230\234\"\191\177\250\230\167\n\214X\244z\\" :: BS.ByteString

λ> unXPrvStripPub k
Right "(\134\242|I\141L\EM\NUL\128\173\252q\191\172\167>f \218\222\167.\136\DC4\216\191\253r8cD8&I\STX;\185&\177\172E\241\185\241\157\226\r\163+\EM\GS\232-\188\250[E^N\129J\158\STX\134\188!\241\DLEzt\222\199\247U\143\ETB\128,\226Q\"\230\234\"\191\177\250\230\167\n\214X\244z\\"

λ> unXPrvStripPub $ CC.xPrvChangePass (""::BS.ByteString) ("newpass"::BS.ByteString) k
Left ErrNoRoundtripMismatch
```

<!-- 
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Once created, link this PR to its corresponding ticket
 ✓ Assign the PR to a corresponding milestone
 ✓ Acknowledge any changes required to the Wiki
-->


Co-authored-by: Johannes Lund <johannes.lund@iohk.io>
iohk-bors bot added a commit that referenced this issue Feb 26, 2020
1315: Add shutdown handler for new launcher r=KtorZ a=rvl

Relates to #1314.

# Overview

This adds a simpler way of ensuring clean shutdown of the wallet on windows (and linux), which doesn't require DaedalusIPC. The mechanism will be used by cardano-launcher.

- Adds the shutdown handler thread.
- Needed to rearrange startup functions around a little bit.
- Unit tests and an integration test.

# Comments

[Hydra jobset](https://hydra.iohk.io/jobset/Cardano/cardano-wallet-pr-1315)


1321: Add CLI command for extracting root xprvs r=Anviking a=Anviking

# Issue Number

#1316 


# Overview

<!-- Detail in a few bullet points the work accomplished in this PR -->

- [x] I have added `cardano-wallet-jormungandr key root --type random <mnemonic words>`
- [x] I added unit tests for help-text and actual usage.
- [x] I added a *pending* test making sure keys are compatible with jcli (which would fail)

# Comments

```bash
$ cardano-wallet-jormungandr key --help
Usage: cardano-wallet-jormungandr key COMMAND
  Derive keys from mnemonics.

Available options:
  -h,--help                Show this help text

Available commands:
  root                     Extract root xprv as hex (64 bytes private key + 32
                           bytes chain code)
$ cardano-wallet-jormungandr key root --help
Usage: cardano-wallet-jormungandr key root --type KEYTYPE MNEMONIC_WORDS...
  Extract root xprv as hex (64 bytes private key + 32 bytes chain code)

Available options:
  -h,--help                Show this help text
  --type KEYTYPE           Any of the following:
                             random (Daedalus, 12 words)
                             icarus (15 words)
                             trezor (12, 15, 18, 21, or 24 words)
                             ledger (12, 15, 18, 21, or 24 words)
$ cardano-wallet-jormungandr key root --type random flock advance execute country leader exotic mix twenty six margin orient meat
68a0f29e6bd5d8af7ffd00a55006afa8af6fbdbded07984ddf7fb1c31c66f7460685e5d1016553fccc9724f5ee95dd8d66facd2ac1bb2f6fcd7fa5e53c97a57f50c592fcd18b67bf3393a16184d009fb25450b2de8079f870222874e804584a8
```

<!-- Additional comments or screenshots to attach if any -->

<!-- 
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Once created, link this PR to its corresponding ticket
 ✓ Assign the PR to a corresponding milestone
 ✓ Acknowledge any changes required to the Wiki
-->


Co-authored-by: Rodney Lorrimar <rodney.lorrimar@iohk.io>
Co-authored-by: KtorZ <matthias.benkort@gmail.com>
Co-authored-by: Johannes Lund <johannes.lund@iohk.io>
iohk-bors bot added a commit that referenced this issue Feb 26, 2020
1321: Add CLI command for extracting root xprvs r=Anviking a=Anviking

# Issue Number

#1316 


# Overview

<!-- Detail in a few bullet points the work accomplished in this PR -->

- [x] I have added `cardano-wallet-jormungandr key root --type random <mnemonic words>`
- [x] I added unit tests for help-text and actual usage.
- [x] I added a *pending* test making sure keys are compatible with jcli (which would fail)

# Comments

```bash
$ cardano-wallet-jormungandr key --help
Usage: cardano-wallet-jormungandr key COMMAND
  Derive keys from mnemonics.

Available options:
  -h,--help                Show this help text

Available commands:
  root                     Extract root xprv as hex (64 bytes private key + 32
                           bytes chain code)
$ cardano-wallet-jormungandr key root --help
Usage: cardano-wallet-jormungandr key root --type KEYTYPE MNEMONIC_WORDS...
  Extract root xprv as hex (64 bytes private key + 32 bytes chain code)

Available options:
  -h,--help                Show this help text
  --type KEYTYPE           Any of the following:
                             random (Daedalus, 12 words)
                             icarus (15 words)
                             trezor (12, 15, 18, 21, or 24 words)
                             ledger (12, 15, 18, 21, or 24 words)
$ cardano-wallet-jormungandr key root --type random flock advance execute country leader exotic mix twenty six margin orient meat
68a0f29e6bd5d8af7ffd00a55006afa8af6fbdbded07984ddf7fb1c31c66f7460685e5d1016553fccc9724f5ee95dd8d66facd2ac1bb2f6fcd7fa5e53c97a57f50c592fcd18b67bf3393a16184d009fb25450b2de8079f870222874e804584a8
```

<!-- Additional comments or screenshots to attach if any -->

<!-- 
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Once created, link this PR to its corresponding ticket
 ✓ Assign the PR to a corresponding milestone
 ✓ Acknowledge any changes required to the Wiki
-->


Co-authored-by: Johannes Lund <johannes.lund@iohk.io>
@piotr-iohk
Copy link
Contributor

@Anviking, @KtorZ

  1. Question about: Add CLI command for retrieving root private keys from mnemonics #1316 (comment)

For this ticket this means taking the mnemonic words as arguments instead of via a secure getLine.

Don't you think that's a bit of a security concern? Also it is not consistent with how we handle mnemonics in CLI in other places, e.g.:

  • cardano-wallet-jormungandr mnemonic reward-credentials
  • cardano-wallet-jormungandr wallet create
  1. I see that key option is available in cardano-wallet-jormungandr but not in cardano-wallet-byron. Is that intentional?

@Anviking
Copy link
Collaborator Author

Anviking commented Mar 3, 2020

I see that key option is available in cardano-wallet-jormungandr but not in cardano-wallet-byron. Is that intentional?

Ah. I guess that is not intentional👍

Don't you think that's a bit of a security concern? Also it is not consistent with how we handle mnemonics in CLI in other places, e.g.:

Tbh, yes. I was worried when I tested with a real old Byron mnemonic, and upgraded my shell to have a fish --private feature before doing that.

I think being able to provide the mnemonic as stdin or argument should still be possible. But an interactive "secure option" might be nice. Perhaps marketed as default.

I would also like to support piping between wallet key commands

such that I can more conveniently perform:

cardano-wallet key root | cardano-wallet key child --path "44'/1815'/0'/0" | cardano-wallet key public

without ever having an intermediary private key or mnemonic in the history.

Perhaps we (I) could create a follow-up U/S for this?

@Anviking
Copy link
Collaborator Author

Anviking commented Mar 3, 2020

I should note: I don't really feel confident about what security concerns there are, or how intricate shell details might interplay.

I mostly just know that the interactive prompt way of reading mnemonics avoids putting them in the shell history, and I'm extrapolating from there.

@piotr-iohk
Copy link
Contributor

Re: #1316 (comment)

I think being able to provide the mnemonic as stdin or argument should still be possible. But an interactive "secure option" might be nice. Perhaps marketed as default.

I think that if we think that having mnemonic in arg is a security concern we shouldn't support it. (the only advantage is that such CLI is easier to automate I think, but I don't suppose that is a big advantage over security). Also, I think that we should be consistent in the way we treat mnemonics in CLI, and I think that having it in the form of "interactive CLI" in other CLI commands was the result of previous discussions over security. 🤷‍♂️

I mostly just know that the interactive prompt way of reading mnemonics avoids putting them in the shell history, and I'm extrapolating from there.

I'm going from the same place :).

I would also like to support piping between wallet key commands

Indeed, that sounds like a separate story.

--
Some thoughts about integration tests:
https://github.com/input-output-hk/cardano-wallet/blob/c392526ffdf063290580bc2d108d5ffdfc29bd74/lib/jormungandr/test/integration/Test/Integration/Jormungandr/Scenario/CLI/Keys.hs#L61-L67

I think that these are not integration tests actually, because they do not excercise the system e2e enforcing "integration" of all components in order to get the result (unless I'm missing something).
Nevertheless these are nice roundtrip tests, but I think they should live somewhere along other unit tests.

Having said that, I think there are actually no integration tests for this requirement. Perhaps at least a few scenarios could be handy. (similar as here -> https://github.com/input-output-hk/cardano-wallet/tree/master/lib/core-integration/src/Test/Integration/Scenario/CLI). What do you think? :)

iohk-bors bot added a commit that referenced this issue Mar 4, 2020
1409: Enable `key` cli commands for cardano-wallet-byron executable r=Anviking a=Anviking

# Issue Number

#1316 


# Overview

<!-- Detail in a few bullet points the work accomplished in this PR -->

- [x] enable `key` cli commands for cardano-wallet-byron

# Comments

<!-- Additional comments or screenshots to attach if any -->

<!-- 
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Once created, link this PR to its corresponding ticket
 ✓ Assign the PR to a corresponding milestone
 ✓ Acknowledge any changes required to the Wiki
-->


Co-authored-by: Johannes Lund <johannes.lund@iohk.io>
@Anviking
Copy link
Collaborator Author

Anviking commented Mar 4, 2020

I think that these are not integration tests actually, because they do not excercise the system e2e enforcing "integration" of all components in order to get the result (unless I'm missing something).
Nevertheless these are nice roundtrip tests, but I think they should live somewhere along other unit tests.

They do launch jcli as an external process. I originally added them in core:unit, but @KtorZ told me to move them 😄

Having said that, I think there are actually no integration tests for this requirement. Perhaps at least a few scenarios could be handy

Are these not enough as integration tests?
https://github.com/input-output-hk/cardano-wallet/blob/master/lib/cli/test/unit/Cardano/CLISpec.hs#L640-L667

(They don't actually call a wallet executable, but they result in the full cli handlers being invoked. But I think that is just fine.)

@piotr-iohk
Copy link
Contributor

They do launch jcli as an external process. I originally added them in core:unit, but @KtorZ told me to move them smile

Ok, in my opinion the fact that they use jcli to check the result does not make them integration tests 🤷‍♂️. But, I guess, that's opinion vs. opinion... so, ok. ;)

(They don't actually call a wallet executable, but they result in the full cli handlers being invoked. But I think that is just fine.)

Allright, I'll base on your assessment.

@piotr-iohk
Copy link
Contributor

ok, lgtm.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants