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 command for converting Byron, Icarus, and Shelley style cardano-address signing keys #1822

Merged
merged 3 commits into from
Sep 15, 2020

Conversation

intricate
Copy link
Contributor

@intricate intricate commented Sep 8, 2020

Closes #1756

Help text output of the new command added by this PR:

Usage: cardano-cli shelley key convert-cardano-address-key (--shelley-payment-key |
                                                                     
                                                                     --shelley-stake-key |
                                                                     
                                                                     --icarus-payment-key |
                                                                     
                                                                     --byron-payment-key)
                                                                   --signing-key-file FILE
                                                                   --out-file FILE
  Convert a cardano-address extended signing key to a corresponding
  Shelley-format key.

Available options:
  --shelley-payment-key    Use a Shelley-era extended payment key.
  --shelley-stake-key      Use a Shelley-era extended stake key.
  --icarus-payment-key     Use a Byron-era extended payment key formatted in the
                           Icarus style.
  --byron-payment-key      Use a Byron-era extended payment key formatted in the
                           deprecated Byron style.
  --signing-key-file FILE  Input filepath of the signing key.
  --out-file FILE          The output file.

@intricate intricate self-assigned this Sep 8, 2020
@intricate intricate marked this pull request as ready for review September 8, 2020 17:37
@intricate intricate force-pushed the intricate/1756-cardano-address-keys branch 2 times, most recently from a0770fa to 8bcae8e Compare September 8, 2020 17:46
pKeyConvertCardanoAddressSigningKey =
KeyConvertCardanoAddressSigningKey
<$> pCardanoAddressKeyType
<*> pSigningKeyFile Input
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps I should use an option parser whose help text specifically denotes that we expect a file containing a Bech32-encoded extended signing key.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems a good idea.

pure $ Left $
FileError fp (CardanoAddressSigningKeyBech32DecodeError err)
Right (_hrPart, _dataPart, bs) ->
pure $ first (FileError fp) (convertBip32SigningKey bs)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a go to see what an ExceptT version would look like:

readBech32Bip32SigningKeyFile
  :: SigningKeyFile
  -> ExceptT (FileError CardanoAddressSigningKeyConversionError) IO Crypto.XPrv
readBech32Bip32SigningKeyFile (SigningKeyFile fp) = do
  str <- readFile fp                                                      & handleIOExceptT (FileIOError fp)
  (_hrPart, _dataPart, bs) <- decodeBech32 (Text.concat $ Text.words str) & except & withExceptT (FileError fp . CardanoAddressSigningKeyBech32DecodeError)
  convertBip32SigningKey bs                                               & except & withExceptT (FileError fp)

The LHS of the & would be the business logic and the RHS of & is all the error type mangling.

Copy link
Contributor

@Jimbo4350 Jimbo4350 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One comment, happy to approve after.

@intricate intricate force-pushed the intricate/1756-cardano-address-keys branch 3 times, most recently from df07253 to a7ab18a Compare September 14, 2020 19:44
@intricate intricate force-pushed the intricate/1756-cardano-address-keys branch from a7ab18a to 289de30 Compare September 14, 2020 22:18
ItnSigningKeyDeserialisationError _sKey ->
-- Sensitive data, such as the signing key, is purposely not included in
-- the error message.
"Error deserialising signing key."
Copy link
Contributor

@newhoggy newhoggy Sep 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious about this concern since presumably the user has access to the key that might be printed anyway.

Might it be worthwhile printing **************suffix to at least make it easier for the user to check if they're using the right key?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be a concern if, for example, the user is redirecting stderr to some log file with more lax permissions than the signing key file itself.

I think it could be useful to print a suffix, but I wonder how many bytes of the signing key would be safe to display.

@Jimbo4350 Jimbo4350 self-requested a review September 15, 2020 10:20
@intricate
Copy link
Contributor Author

bors r+

iohk-bors bot added a commit that referenced this pull request Sep 15, 2020
1822: Add command for converting Byron, Icarus, and Shelley style cardano-address signing keys r=intricate a=intricate

Closes #1756 

Help text output of the new command added by this PR:

```
Usage: cardano-cli shelley key convert-cardano-address-key (--shelley-payment-key |
                                                                     
                                                                     --shelley-stake-key |
                                                                     
                                                                     --icarus-payment-key |
                                                                     
                                                                     --byron-payment-key)
                                                                   --signing-key-file FILE
                                                                   --out-file FILE
  Convert a cardano-address extended signing key to a corresponding
  Shelley-format key.

Available options:
  --shelley-payment-key    Use a Shelley-era extended payment key.
  --shelley-stake-key      Use a Shelley-era extended stake key.
  --icarus-payment-key     Use a Byron-era extended payment key formatted in the
                           Icarus style.
  --byron-payment-key      Use a Byron-era extended payment key formatted in the
                           deprecated Byron style.
  --signing-key-file FILE  Input filepath of the signing key.
  --out-file FILE          The output file.
```

Co-authored-by: Luke Nadur <19835357+intricate@users.noreply.github.com>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Sep 15, 2020

Build failed:

@intricate
Copy link
Contributor Author

bors r+

Copy link
Contributor

@dcoutts dcoutts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks plausible. How do we test this properly?

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Sep 15, 2020

@iohk-bors iohk-bors bot merged commit 10392ba into master Sep 15, 2020
@iohk-bors iohk-bors bot deleted the intricate/1756-cardano-address-keys branch September 15, 2020 14:52
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

Successfully merging this pull request may close these issues.

[FR] - Additional CLI commands for converting cardano-address byron and shelley keys
4 participants