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

keyring's encrypted file backend integration #5355

Merged
merged 34 commits into from
Dec 11, 2019

Conversation

alessio
Copy link
Contributor

@alessio alessio commented Dec 3, 2019

Add encrypted file support to keyring implementation.

  • 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)

@alessio alessio changed the title Start working on keyring's encrypted file backend integration keyring's encrypted file backend integration Dec 3, 2019
@jackzampolin
Copy link
Member

If users use the keyring-file option, can they avoid password entry?

@alessio
Copy link
Contributor Author

alessio commented Dec 4, 2019

@jackzampolin no, absolutely not. If users want to avoid inputting password at all they should use the test keyring, i.e. export COSMOS_SDK_TEST_KEYRING = y would do it

@alessio alessio marked this pull request as ready for review December 4, 2019 17:30
@alessio alessio added the R4R label Dec 4, 2019
@alessio alessio requested a review from tac0turtle December 4, 2019 17:32
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.

changes LGTM. I'd like to add the comment you posted on slack somewhere:

Same impl as test keyring w/ the difference that when you export COSMOS_SDK_TEST_KEYRING you enable an on-file, passwordless keyring (for testing only).
If you pass the --keyring-file option to gaiacli (or you set the opt to true in the config file), then you get a CLI only-based keyring. It uses the same backend as the test keyring, though files are stored under a different directory (you don't want to mix&match test keyring keys AND encrypted keyring keys).

CHANGELOG.md Show resolved Hide resolved
client/flags/flags.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Dec 5, 2019

Codecov Report

Merging #5355 into master will increase coverage by 0.03%.
The diff coverage is 68.75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5355      +/-   ##
==========================================
+ Coverage    54.5%   54.53%   +0.03%     
==========================================
  Files         311      311              
  Lines       18721    18740      +19     
==========================================
+ Hits        10203    10219      +16     
- Misses       7740     7741       +1     
- Partials      778      780       +2
Impacted Files Coverage Δ
client/keys/import.go 68.42% <100%> (-1.58%) ⬇️
client/config.go 46.25% <100%> (ø) ⬆️
client/keys/root.go 100% <100%> (ø) ⬆️
client/keys/export.go 65.21% <100%> (-1.45%) ⬇️
client/keys/utils.go 48.61% <62.5%> (+3.15%) ⬆️
crypto/keys/keyring.go 45.55% <63.15%> (+1.89%) ⬆️
client/keys/add.go 39.35% <0%> (ø) ⬆️

Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

Can you open an accompanying PR on Gaia that points to this branch so that I can test this out please?

@alessio alessio mentioned this pull request Dec 5, 2019
11 tasks
@alessio
Copy link
Contributor Author

alessio commented Dec 5, 2019

@alessio
Copy link
Contributor Author

alessio commented Dec 5, 2019

After having a conversation with @jackzampolin, I think it makes more sense and is overall more secure to replace the COSMOS_SDK_TEST_KEYRING environment variable with a command line flag/client's config file option. So please do not merge this for now.

@alessio alessio added WIP and removed R4R labels Dec 5, 2019
@alessio alessio requested a review from fedekunze December 5, 2019 16:13
@alessio
Copy link
Contributor Author

alessio commented Dec 8, 2019

This is now ready for review again. The feature can be tested on gaia via cosmos/gaia#212

@alessio alessio added C:CLI C:Keys Keybase, KMS and HSMs R4R and removed WIP labels Dec 8, 2019
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

ACK. @alessio, I really do think it would be great if we had a README.md under crypto/keys that outlines what the Keybase is, what the Keyring is and the different modes and their details (e.g. where is key material persisted, how is it managed, etc..).

@alessio
Copy link
Contributor Author

alessio commented Dec 9, 2019

Great idea @alexanderbez, I just added a README.md under crypto/keys. @fedekunze please have a look!

Alessio Treglia added 2 commits December 9, 2019 18:25
crypto/keys/README.md Outdated Show resolved Hide resolved
crypto/keys/README.md Outdated Show resolved Hide resolved
### NewInMemory

The [NewInMemory](https://godoc.org/github.com/cosmos/cosmos-sdk/crypto/keys#NewInMemory) constructor returns
an implementation backed by an in-memory LevelDB storage that we've historically used for testing purposes or on-the-fly
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this true? I believed MemDB to be a true in-memory map db -- no LevelDB involved. I could be wrong here though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're definitely right, just double checked. Will amend accordingly, thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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


### NewKeyringFile, NewTestKeyring

Both [NewKeyringFile](https://godoc.org/github.com/cosmos/cosmos-sdk/crypto/keys#NewKeyringFile) and
Copy link
Contributor

Choose a reason for hiding this comment

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

We should state where NewKeyringFile stores key material on disk (i.e. the default path).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alessio alessio requested a review from alexanderbez December 9, 2019 18:49
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

ACK

@alessio alessio merged commit 3948600 into master Dec 11, 2019
@alessio alessio deleted the alessio/keyring-file-backend branch December 11, 2019 09:45
alessio pushed a commit to cosmos/gaia that referenced this pull request Dec 12, 2019
Use new --keyring-backend flag.

See cosmos/cosmos-sdk#5355 for more information.
xiangjianmeng pushed a commit to xiangjianmeng/cosmos-sdk that referenced this pull request Dec 18, 2019
Client commands accept a new `--keyring-backend` option through which users can specify which backend should be used by the new key store:
- os: use OS default credentials storage (default).
- file: use encrypted file-based store.
- test: use password-less key store (highly insecure).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:CLI C:Keys Keybase, KMS and HSMs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants