keyring's encrypted file backend integration #5355

alessio · 2019-12-03T14:26:22Z

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)

jackzampolin · 2019-12-03T21:21:21Z

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

alessio · 2019-12-04T09:44:13Z

@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

…respectively

…age.

fedekunze

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

client/flags/flags.go

codecov · 2019-12-05T10:40:57Z

Codecov Report

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

@@            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%> (ø)`	⬆️

alexanderbez

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

alessio · 2019-12-05T15:25:08Z

@alexanderbez cosmos/gaia#212

alessio · 2019-12-05T16:12:26Z

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.

@jackzampolin

…nd flag CC @jackzampolin

alessio · 2019-12-08T13:51:33Z

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

alexanderbez

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 · 2019-12-09T17:23:34Z

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

crypto/keys/README.md

alexanderbez · 2019-12-09T18:05:14Z

crypto/keys/README.md

+### 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


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

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

alexanderbez · 2019-12-09T18:06:30Z

crypto/keys/README.md

+
+### NewKeyringFile, NewTestKeyring
+
+Both [NewKeyringFile](https://godoc.org/github.com/cosmos/cosmos-sdk/crypto/keys#NewKeyringFile) and


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

Co-Authored-By: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>

…os-sdk into alessio/keyring-file-backend

alexanderbez

ACK

Use new --keyring-backend flag. See cosmos/cosmos-sdk#5355 for more information.

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

Start working on keyring's encrypted file backend integration

376fe1f

alessio changed the title ~~Start working on keyring's encrypted file backend integration~~ keyring's encrypted file backend integration Dec 3, 2019

Alessio Treglia added 3 commits December 4, 2019 18:19

Store keyring and test keyring files under ~/.appcli/{,test-}keyring …

a97002e

…respectively

Keybase -> keyring migration is in fact an API and ABI breakage break…

32c374c

…age.

Merge branch 'master' into alessio/keyring-file-backend

79f691d

alessio marked this pull request as ready for review December 4, 2019 17:30

alessio requested review from alexanderbez, fedekunze, jackzampolin and rigelrozanski as code owners December 4, 2019 17:30

alessio added the R4R label Dec 4, 2019

alessio requested a review from tac0turtle December 4, 2019 17:32

fedekunze reviewed Dec 4, 2019

View reviewed changes

CHANGELOG.md Show resolved Hide resolved

client/flags/flags.go Outdated Show resolved Hide resolved

Merge branch 'master' into alessio/keyring-file-backend

7a11d4e

Improve keyring docs

14c1252

fedekunze approved these changes Dec 5, 2019

View reviewed changes

Merge branch 'master' into alessio/keyring-file-backend

69bb4af

alexanderbez reviewed Dec 5, 2019

View reviewed changes

alessio mentioned this pull request Dec 5, 2019

Bump SDK master version cosmos/gaia#212

Merged

11 tasks

alessio added WIP and removed R4R labels Dec 5, 2019

alessio requested a review from fedekunze December 5, 2019 16:13

Alessio Treglia added 3 commits December 6, 2019 16:21

Replace COSMOS_SDK_TEST_KEYRING env var with a global --keybase-backe…

411cb42

…nd flag CC @jackzampolin

Merge branch 'master' into alessio/keyring-file-backend

9f0c12e

Merge branch 'master' into alessio/keyring-file-backend

5e963a9

Update

fc190ab

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

Alessio Treglia and others added 2 commits December 9, 2019 12:29

Merge branch 'master' into alessio/keyring-file-backend

c4b3fa6

Merge branch 'master' into alessio/keyring-file-backend

dcd967e

alexanderbez approved these changes Dec 9, 2019

View reviewed changes

Create README.md

5f777bf

Alessio Treglia added 2 commits December 9, 2019 18:25

Typo

810083f

Fixes

7fc84fb

alexanderbez suggested changes Dec 9, 2019

View reviewed changes

Alessio Treglia and others added 4 commits December 9, 2019 19:41

Update crypto/keys/README.md

34ced6d

Co-Authored-By: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>

Update crypto/keys/README.md

c2b248f

Co-Authored-By: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>

Fix NewInMemory description

6b3b8f3

Add info on where keyring files are stored

afbb163

alessio requested a review from alexanderbez December 9, 2019 18:49

alexanderbez and others added 3 commits December 10, 2019 09:35

Merge branch 'master' into alessio/keyring-file-backend

d1cfa57

Merge branch 'alessio/keyring-file-backend' of github.com:cosmos/cosm…

1701751

…os-sdk into alessio/keyring-file-backend

Merge branch 'master' into alessio/keyring-file-backend

d79acf2

alexanderbez approved these changes Dec 11, 2019

View reviewed changes

alexanderbez and others added 2 commits December 10, 2019 23:44

Merge branch 'master' into alessio/keyring-file-backend

81bf4fb

Merge branch 'master' into alessio/keyring-file-backend

24ba280

alessio merged commit 3948600 into master Dec 11, 2019

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

Bump SDK master version (#212)

47eca7b

Use new --keyring-backend flag. See cosmos/cosmos-sdk#5355 for more information.

ethanfrey mentioned this pull request Jan 21, 2020

wasmcli does not works. CosmWasm/wasmd#31

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

keyring's encrypted file backend integration #5355

keyring's encrypted file backend integration #5355

alessio commented Dec 3, 2019 •

edited

Loading

jackzampolin commented Dec 3, 2019

alessio commented Dec 4, 2019

fedekunze left a comment

codecov bot commented Dec 5, 2019 •

edited

Loading

alexanderbez left a comment

alessio commented Dec 5, 2019

alessio commented Dec 5, 2019

alessio commented Dec 8, 2019

alexanderbez left a comment

alessio commented Dec 9, 2019

alexanderbez Dec 9, 2019

alessio Dec 9, 2019

alessio Dec 10, 2019

alexanderbez Dec 9, 2019

alessio Dec 10, 2019

alexanderbez left a comment


		### NewKeyringFile, NewTestKeyring

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

keyring's encrypted file backend integration #5355

keyring's encrypted file backend integration #5355

Conversation

alessio commented Dec 3, 2019 • edited Loading

jackzampolin commented Dec 3, 2019

alessio commented Dec 4, 2019

fedekunze left a comment

Choose a reason for hiding this comment

codecov bot commented Dec 5, 2019 • edited Loading

Codecov Report

alexanderbez left a comment

Choose a reason for hiding this comment

alessio commented Dec 5, 2019

alessio commented Dec 5, 2019

alessio commented Dec 8, 2019

alexanderbez left a comment

Choose a reason for hiding this comment

alessio commented Dec 9, 2019

alexanderbez Dec 9, 2019

Choose a reason for hiding this comment

alessio Dec 9, 2019

Choose a reason for hiding this comment

alessio Dec 10, 2019

Choose a reason for hiding this comment

alexanderbez Dec 9, 2019

Choose a reason for hiding this comment

alessio Dec 10, 2019

Choose a reason for hiding this comment

alexanderbez left a comment

Choose a reason for hiding this comment

alessio commented Dec 3, 2019 •

edited

Loading

codecov bot commented Dec 5, 2019 •

edited

Loading