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

Drop on-disk keybase in favor of keyring #5180

Merged
merged 34 commits into from
Nov 14, 2019
Merged

Conversation

alessio
Copy link
Contributor

@alessio alessio commented Oct 11, 2019

Rationale for this work is articulated in https://github.com/cosmos/cosmos-sdk/blob/master/docs/architecture/adr-006-secret-store-replacement.md

Breaking changes:

  • The following calls were removed from the client package and
    its subpackages:
    • GetKeyInfo()
    • GetPassphrase()
    • NewKeybaseFrom*()
    • ReadPassphraseFromStdin()
  • GetFromFields() now takes a io.Reader as argument.
  • NewTxBuilderFromCLI() constructor now takes a io.Reader, needed
    to initialise the user session's keyring.
  • The keys update command is no longer used and shall be removed in
    a future release.
  • server.GenerateSaveCoinKey() signature changed

CLIContext's new constructors are provided:

  • NewCLIContextWithInputAndFrom()
  • NewCLIContextWithInput()

CLIContext now encapsulates a io.Reader and provides a
WithInput(io.Reader) method to override the initial
configuration.

Thanks: @poldsam for the original patch #4754

  • Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
  • Wrote tests
  • Updated relevant documentation (docs/)
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the github PR explorer

For Admin Use:

  • Added appropriate labels to PR (ex. wip, ready-for-review, docs)
  • Reviewers Assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

client/keys/utils.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Oct 11, 2019

Codecov Report

Merging #5180 into master will decrease coverage by 0.01%.
The diff coverage is 36.25%.

@@            Coverage Diff             @@
##           master    #5180      +/-   ##
==========================================
- Coverage   54.72%   54.71%   -0.02%     
==========================================
  Files         307      307              
  Lines       18304    18290      -14     
==========================================
- Hits        10017    10007      -10     
- Misses       7501     7507       +6     
+ Partials      786      776      -10

@alessio alessio marked this pull request as ready for review October 11, 2019 11:13
@alessio alessio requested review from jleni and zmanian October 11, 2019 11:13
@fedekunze fedekunze added R4R T: State Machine Breaking State machine breaking changes (impacts consensus). labels Oct 11, 2019
@alessio alessio mentioned this pull request Oct 11, 2019
5 tasks

func isRunningOnServer() bool {
backends := keyring.AvailableBackends()
return len(backends) == 2 && backends[1] == keyring.BackendType("file")
Copy link
Contributor

Choose a reason for hiding this comment

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

What does running on server mean here?

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's a misnomer, I copied it from the original PR and am happy to rename it. It attempts to determine whether the test is running on a developer machine or in a CI container.

@alessio
Copy link
Contributor Author

alessio commented Oct 12, 2019

This certainly breaks API and ABI. How comes that breaks the state machine too? @fedekunze

Alessio Treglia added 7 commits October 13, 2019 15:57
```
$ gaiacli keys update --help
Command "update" is deprecated, it takes no effect with the new keyring
based backend and is provided only for backward compatibility with the
legacy LevelDB based backend.
Refer to your operating system's manual to learn how to change your
keyring's password.

Change the password used to protect private key

Usage:
  gaiacli keys update <name> [flags]

Flags:
  -h, --help   help for update

Global Flags:
      --chain-id string   Chain ID of tendermint node
  -e, --encoding string   Binary encoding (hex|b64|btc) (default "hex")
      --home string       directory for config and data (default "/home/alessio/.gaiacli")
  -o, --output string     Output format (text|json) (default "text")
      --trace             print out full stack trace on errors
```
@alexanderbez alexanderbez removed the T: State Machine Breaking State machine breaking changes (impacts consensus). label Oct 14, 2019
@alexanderbez alexanderbez added the C:Keys Keybase, KMS and HSMs label Oct 16, 2019
Copy link
Collaborator

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

utACK

@fedekunze
Copy link
Collaborator

FYI, I'll merge this by EOD. I'll wait for more reviews until then

@tac0turtle
Copy link
Member

tac0turtle commented Nov 14, 2019

bump merge me 😄

@fedekunze your days are quite long 😉

Copy link
Collaborator

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

tested ACK

@fedekunze fedekunze merged commit d4c831e into master Nov 14, 2019
@fedekunze fedekunze deleted the alessio/switch-to-keyring branch November 14, 2019 14:17
@andrecronje
Copy link

https://github.com/cosmos/cosmos-sdk/blame/c9370840bff5c363370d0ec0f95c482a05db5410/x/auth/client/utils/tx.go#L97

txBytes, err := txBldr.BuildAndSign(fromName, client.DefaultKeyPass, msgs)

BuildAndSign uses client.DefaultKeyPass instead of user input

@alessio
Copy link
Contributor Author

alessio commented Nov 20, 2019

@andrecronje BuildAndSign() could even pass an empty passphrase to Sign(), it wouldn't matter as it then calls MakeSignature() that in turn uses a Keybase interface instance to sign. If the keybase instance is a keyring-backed one, passphrase is discarded.

@alessio
Copy link
Contributor Author

alessio commented Nov 20, 2019

I've decided to keep the passphrase around even if it's empty in order to avoid the Keybase interface. Whether the passphrase is actually used depends on the keybase implementation used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:Keys Keybase, KMS and HSMs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants