Switch secret store to the keyring secret store #4754

poldsam · 2019-07-21T20:13:07Z

ADR: adr-006-secret-store-replacement

Switching back-end to new secret store using library Keyring
- This is intended to provide stronger security guarantees by using the OS built-in secret store instead of the legacy key database which was designed to single-user computers.
- Existing users are expected to migrate their keys using the migrate command:
  - gaiacli keys migrate
- Support for legacy keystore is available through the secret store flag:
  - gaiacli keys add <key_name> --legacy-secret-store
- Signing transactions is enabled on the legacy store if you pass in the legacy secret store flag (legacy-secret-store)
- Running the tests locally may require entering your user password to access your keystore large amount of times
- There will be a follow-up pull request on github.com/cosmos/gaia to fix breakage in gaiacli tests. Gaiacli tests are breaking due to new password entries required when using the file backend for the keyring secret store.

Targeted PR against correct branch (see CONTRIBUTING.md)
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: clog add [section] [stanza] [message]
rereviewed 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)

for keybaseKeyring

… secret store

…t to new secret store

…mos-sdk into kristi/keyringKeybase

golangcibot · 2019-09-05T20:43:17Z

crypto/keys/keybase_keyring.go

+		}
+
+		// Will block until user inputs the signature
+		signed, err = buf.ReadString('\n')


undeclared name: signed (from typecheck)

client/keys/add.go

client/keys/migrate.go

types/rest/rest_test.go

x/distribution/client/common/common_test.go

golangcibot · 2019-09-05T21:12:06Z

crypto/keys/keybase_keyring.go

+		}
+
+		// Will block until user inputs the signature
+		signed, err = buf.ReadString('\n')


undeclared name: signed (from typecheck)

client/keys/add.go

client/keys/migrate.go

types/rest/rest_test.go

x/distribution/client/common/common_test.go

crypto/keys/keybase_keyring.go

golangcibot · 2019-09-05T21:33:49Z

crypto/keys/lazy_keybase_keyring.go

+		var keyhash []byte
+		keyhashStored := false
+
+		if _, err := os.Stat(lkb.dir + "/keyhash"); err == nil {


ifElseChain: rewrite if-else to switch statement (from gocritic)

x/nft/client/cli/tx.go

golangcibot · 2019-09-05T21:33:50Z

crypto/keys/lazy_keybase_keyring_test.go

+
+	algo := Secp256k1
+	n1, n2, n3 := "personal", "business", "other"
+	p1, p2 := "1234", "really-secure!@#$"


string 1234 has 4 occurrences, but such constant nums already exists (from goconst)

golangcibot · 2019-09-05T21:33:50Z

crypto/keys/lazy_keybase_keyring_test.go

+	algo := Secp256k1
+
+	n1, n2, n3 := "some dude", "a dudette", "dude-ish"
+	p1, p2, p3 := "1234", "foobar", "foobar"


string foobar has 3 occurrences, but such constant foobar already exists (from goconst)

types/rest/rest_test.go

x/distribution/client/common/common_test.go

crypto/keys/lazy_keybase_keyring.go

crypto/keys/keybase_keyring.go

golangcibot · 2019-09-05T23:41:15Z

crypto/keys/lazy_keybase_keyring.go

+		var keyhash []byte
+		keyhashStored := false
+
+		if _, err := os.Stat(lkb.dir + "/keyhash"); err == nil {


ifElseChain: rewrite if-else to switch statement (from gocritic)

x/nft/client/cli/tx.go

crypto/keys/keybase_keyring.go

golangcibot · 2019-09-05T23:41:16Z

crypto/keys/lazy_keybase_keyring_test.go

+
+	algo := Secp256k1
+	n1, n2, n3 := "personal", "business", "other"
+	p1, p2 := "1234", "really-secure!@#$"


string 1234 has 4 occurrences, but such constant nums already exists (from goconst)

golangcibot · 2019-09-05T23:41:16Z

crypto/keys/lazy_keybase_keyring_test.go

+	algo := Secp256k1
+
+	n1, n2, n3 := "some dude", "a dudette", "dude-ish"
+	p1, p2, p3 := "1234", "foobar", "foobar"


string foobar has 3 occurrences, but such constant foobar already exists (from goconst)

types/rest/rest_test.go

x/distribution/client/common/common_test.go

crypto/keys/lazy_keybase_keyring.go

Co-Authored-By: Bot from GolangCI <42910462+golangcibot@users.noreply.github.com>

crypto/keys/keybase_keyring.go

golangcibot · 2019-09-05T23:49:00Z

crypto/keys/lazy_keybase_keyring.go

+		var keyhash []byte
+		keyhashStored := false
+
+		if _, err := os.Stat(lkb.dir + "/keyhash"); err == nil {


ifElseChain: rewrite if-else to switch statement (from gocritic)

golangcibot · 2019-09-05T23:49:00Z

crypto/keys/keybase_keyring.go

+		buf := bufio.NewReader(os.Stdin)
+		_, err = fmt.Fprintf(os.Stderr, "\nEnter Amino-encoded signature:\n")
+		if err != nil {
+			return 


File is not goimports-ed (from goimports)

Suggested change

return

return

golangcibot · 2019-09-05T23:49:01Z

crypto/keys/lazy_keybase_keyring_test.go

+	algo := Secp256k1
+
+	n1, n2, n3 := "some dude", "a dudette", "dude-ish"
+	p1, p2, p3 := "1234", "foobar", "foobar"


string foobar has 3 occurrences, but such constant foobar already exists (from goconst)

golangcibot · 2019-09-05T23:49:01Z

crypto/keys/lazy_keybase_keyring_test.go

+
+	algo := Secp256k1
+	n1, n2, n3 := "personal", "business", "other"
+	p1, p2 := "1234", "really-secure!@#$"


string 1234 has 4 occurrences, but such constant nums already exists (from goconst)

types/rest/rest_test.go

x/distribution/client/common/common_test.go

crypto/keys/lazy_keybase_keyring.go

crypto/keys/keybase_keyring.go

golangcibot · 2019-09-05T23:49:09Z

crypto/keys/lazy_keybase_keyring.go

+		var keyhash []byte
+		keyhashStored := false
+
+		if _, err := os.Stat(lkb.dir + "/keyhash"); err == nil {


ifElseChain: rewrite if-else to switch statement (from gocritic)

golangcibot · 2019-09-05T23:49:09Z

crypto/keys/keybase_keyring.go

+		buf := bufio.NewReader(os.Stdin)
+		_, err = fmt.Fprintf(os.Stderr, "\nEnter Amino-encoded signature:\n")
+		if err != nil {
+			return 


File is not goimports-ed (from goimports)

Suggested change

return

return

golangcibot · 2019-09-05T23:49:10Z

crypto/keys/lazy_keybase_keyring_test.go

+
+	algo := Secp256k1
+	n1, n2, n3 := "personal", "business", "other"
+	p1, p2 := "1234", "really-secure!@#$"


string 1234 has 4 occurrences, but such constant nums already exists (from goconst)

golangcibot · 2019-09-05T23:49:10Z

crypto/keys/lazy_keybase_keyring_test.go

+	algo := Secp256k1
+
+	n1, n2, n3 := "some dude", "a dudette", "dude-ish"
+	p1, p2, p3 := "1234", "foobar", "foobar"


string foobar has 3 occurrences, but such constant foobar already exists (from goconst)

types/rest/rest_test.go

x/distribution/client/common/common_test.go

crypto/keys/lazy_keybase_keyring.go

crypto/keys/keybase_keyring.go

golangcibot · 2019-09-05T23:49:43Z

crypto/keys/lazy_keybase_keyring.go

+		var keyhash []byte
+		keyhashStored := false
+
+		if _, err := os.Stat(lkb.dir + "/keyhash"); err == nil {


ifElseChain: rewrite if-else to switch statement (from gocritic)

golangcibot · 2019-09-05T23:49:43Z

crypto/keys/keybase_keyring.go

+		buf := bufio.NewReader(os.Stdin)
+		_, err = fmt.Fprintf(os.Stderr, "\nEnter Amino-encoded signature:\n")
+		if err != nil {
+			return 


File is not gofmt-ed with -s (from gofmt)

Suggested change

return

return

golangcibot · 2019-09-05T23:49:43Z

crypto/keys/lazy_keybase_keyring_test.go

+
+	algo := Secp256k1
+	n1, n2, n3 := "personal", "business", "other"
+	p1, p2 := "1234", "really-secure!@#$"


string 1234 has 4 occurrences, but such constant nums already exists (from goconst)

golangcibot · 2019-09-05T23:49:44Z

crypto/keys/lazy_keybase_keyring_test.go

+	algo := Secp256k1
+
+	n1, n2, n3 := "some dude", "a dudette", "dude-ish"
+	p1, p2, p3 := "1234", "foobar", "foobar"


string foobar has 3 occurrences, but such constant foobar already exists (from goconst)

golangcibot · 2019-09-05T23:49:44Z

x/nft/client/cli/tx.go

@@ -176,7 +179,8 @@ $ %s tx %s burn cripto-kitties d04b98f48e8f8bcc15c6ae5ac050801cd6dcfd428fb5f9e65
 		Args: cobra.ExactArgs(2),
 		RunE: func(cmd *cobra.Command, args []string) error {
 			cliCtx := context.NewCLIContext().WithCodec(cdc)
-			txBldr := authtypes.NewTxBuilderFromCLI().WithTxEncoder(utils.GetTxEncoder(cdc))
+			inBuf := bufio.NewReader(cmd.InOrStdin())


undeclared name: bufio (from typecheck)

types/rest/rest_test.go

x/distribution/client/common/common_test.go

crypto/keys/lazy_keybase_keyring.go

golangcibot · 2019-09-05T23:49:54Z

crypto/keys/keybase_keyring.go

+		buf := bufio.NewReader(os.Stdin)
+		_, err = fmt.Fprintf(os.Stderr, "\nEnter Amino-encoded signature:\n")
+		if err != nil {
+			return 


File is not gofmt-ed with -s (from gofmt)

Suggested change

return

return

crypto/keys/keybase_keyring.go

golangcibot · 2019-09-05T23:49:55Z

crypto/keys/lazy_keybase_keyring.go

+		var keyhash []byte
+		keyhashStored := false
+
+		if _, err := os.Stat(lkb.dir + "/keyhash"); err == nil {


ifElseChain: rewrite if-else to switch statement (from gocritic)

golangcibot · 2019-09-05T23:49:55Z

crypto/keys/lazy_keybase_keyring_test.go

+
+	algo := Secp256k1
+	n1, n2, n3 := "personal", "business", "other"
+	p1, p2 := "1234", "really-secure!@#$"


string 1234 has 4 occurrences, but such constant nums already exists (from goconst)

golangcibot · 2019-09-05T23:49:55Z

crypto/keys/lazy_keybase_keyring_test.go

+	algo := Secp256k1
+
+	n1, n2, n3 := "some dude", "a dudette", "dude-ish"
+	p1, p2, p3 := "1234", "foobar", "foobar"


string foobar has 3 occurrences, but such constant foobar already exists (from goconst)

golangcibot · 2019-09-05T23:49:56Z

x/nft/client/cli/tx.go

@@ -176,7 +179,8 @@ $ %s tx %s burn cripto-kitties d04b98f48e8f8bcc15c6ae5ac050801cd6dcfd428fb5f9e65
 		Args: cobra.ExactArgs(2),
 		RunE: func(cmd *cobra.Command, args []string) error {
 			cliCtx := context.NewCLIContext().WithCodec(cdc)
-			txBldr := authtypes.NewTxBuilderFromCLI().WithTxEncoder(utils.GetTxEncoder(cdc))
+			inBuf := bufio.NewReader(cmd.InOrStdin())


undeclared name: bufio (from typecheck)

types/rest/rest_test.go

x/distribution/client/common/common_test.go

crypto/keys/lazy_keybase_keyring.go

…smos-sdk into kristi/keyringKeybase2

golangcibot · 2019-09-06T00:53:37Z

crypto/keys/lazy_keybase_keyring.go

+		var keyhash []byte
+		keyhashStored := false
+
+		if _, err := os.Stat(lkb.dir + "/keyhash"); err == nil {


ifElseChain: rewrite if-else to switch statement (from gocritic)

golangcibot · 2019-09-06T00:53:37Z

crypto/keys/keybase_keyring.go

+		buf := bufio.NewReader(os.Stdin)
+		_, err = fmt.Fprintf(os.Stderr, "\nEnter Amino-encoded signature:\n")
+		if err != nil {
+			return 


File is not gofmt-ed with -s (from gofmt)

Suggested change

return

return

golangcibot · 2019-09-06T00:53:37Z

crypto/keys/lazy_keybase_keyring_test.go

+	algo := Secp256k1
+
+	n1, n2, n3 := "some dude", "a dudette", "dude-ish"
+	p1, p2, p3 := "1234", "foobar", "foobar"


string foobar has 3 occurrences, but such constant foobar already exists (from goconst)

golangcibot · 2019-09-06T00:53:38Z

crypto/keys/lazy_keybase_keyring_test.go

+
+	algo := Secp256k1
+	n1, n2, n3 := "personal", "business", "other"
+	p1, p2 := "1234", "really-secure!@#$"


string 1234 has 4 occurrences, but such constant nums already exists (from goconst)

golangcibot · 2019-09-06T00:53:38Z

x/nft/client/cli/tx.go

@@ -176,7 +179,8 @@ $ %s tx %s burn cripto-kitties d04b98f48e8f8bcc15c6ae5ac050801cd6dcfd428fb5f9e65
 		Args: cobra.ExactArgs(2),
 		RunE: func(cmd *cobra.Command, args []string) error {
 			cliCtx := context.NewCLIContext().WithCodec(cdc)
-			txBldr := authtypes.NewTxBuilderFromCLI().WithTxEncoder(utils.GetTxEncoder(cdc))
+			inBuf := bufio.NewReader(cmd.InOrStdin())


undeclared name: bufio (from typecheck)

golangcibot · 2019-09-06T00:53:38Z

types/rest/rest_test.go

@@ -7,6 +7,7 @@ import (
 	"io/ioutil"
 	"net/http"
 	"net/http/httptest"
+	"os"


"os" imported but not used (from typecheck)

golangcibot · 2019-09-06T00:53:39Z

x/distribution/client/common/common_test.go

@@ -1,6 +1,7 @@
 package common

 import (
+	"os"


"os" imported but not used (from typecheck)

@poldsam

This chunk is extracted from @poldsam's original PR: - #4754

alessio · 2019-09-11T08:27:14Z

I started to break this down in smaller pieces: #5029

@poldsam

Introduce new Keybase implementation that can leverage operating systems' built-in functionalities to securely store secrets. This chunk is extracted from @poldsam's original PR: - #4754 Thanks: @alexanderbez

Add new command to assist users migrate their keys from the legacy on-disk keybase to the new OS keyring-based implementation. Ref #4754

stale · 2019-10-11T08:46:58Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale · 2019-11-10T09:08:34Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

poldsam and others added 30 commits April 28, 2019 14:15

initial work on keyringKeybase

28859d2

Created a keybase implmentation based on keyring

10fcf0c

Added the lazykey keybase for keyring

aa0441f

First test. Not yet working

3455769

Temporary bandaid fix for testing

75e4272

fixes for keybase tests passed

20d26e3

adding another tests passed

df118b2

adding another tests passed

12cd140

adding another tests passed

040f3a4

adding more tests passed

3e36d53

sorting keys in alphabetical order

7381a97

Merge commit with master

49ef05d

Apply Formating

650ef8b

implementing updated to the keybase interface

630aec0

for keybaseKeyring

implementing interface for the lazy keybase keyring

f82f871

started migration command

c4bf95d

changing variable name to camel case format

43e4d6c

starting migrate command testing

0b9995d

Fix tests for migration

98caf12

Fixed test harnesses

a0e18dd

adding FlagSecretStore to all key commands

ca275a2

adding FlagSecretStore to all key commands

4283079

fixing a typo in FlagSecretStore

3fb144a

adding conditional for FlagSecretStore value; if set to false use new…

4511c5f

… secret store

Fixing testing errors for new keyring keybase

9186212

Fixing update_test to run only on legacy secret store, test irrelevan…

66ba136

…t to new secret store

Merge commit

cbe6b34

Expected fix for the Import issue

54ee067

Merge branch 'kristi/keyringKeybase' of github.com:Proof-Of-Audit/cos…

73f0188

…mos-sdk into kristi/keyringKeybase

Fixing migrate to point to the same keyring service

2ab5885

golangcibot reviewed Sep 5, 2019

View reviewed changes

removing keys

d83f189

golangcibot reviewed Sep 5, 2019

View reviewed changes

Fixing tupos and errors

cda535e

golangcibot reviewed Sep 5, 2019

View reviewed changes

Fixing gobot typeSwitch error

553ef29

golangcibot reviewed Sep 5, 2019

View reviewed changes

poldsam and others added 2 commits September 5, 2019 16:46

Update crypto/keys/keybase_keyring.go

cbb43ab

Co-Authored-By: Bot from GolangCI <42910462+golangcibot@users.noreply.github.com>

Update x/nft/client/cli/tx.go

c408e67

Co-Authored-By: Bot from GolangCI <42910462+golangcibot@users.noreply.github.com>

golangcibot reviewed Sep 5, 2019

View reviewed changes

poldsam added 3 commits September 5, 2019 16:54

Fixing gobot errors

27fa3eb

Fixing gobot errors

d59bfd7

Merge branch 'kristi/keyringKeybase2' of github.com:Proof-Of-Audit/co…

3e1a2f3

…smos-sdk into kristi/keyringKeybase2

golangcibot reviewed Sep 6, 2019

View reviewed changes

jackzampolin assigned alessio Sep 10, 2019

alessio pushed a commit that referenced this pull request Sep 11, 2019

Add support for github.com/99designs/keyring based keybases

f94dd27

This chunk is extracted from @poldsam's original PR: - #4754

alessio mentioned this pull request Sep 11, 2019

Add support for github.com/99designs/keyring-backed keybases #5029

Merged

5 tasks

alessio mentioned this pull request Sep 24, 2019

Add keys migrate command #5097

Merged

5 tasks

alexanderbez pushed a commit that referenced this pull request Sep 30, 2019

Merge PR #5097: Add keys migrate command

3e6562c

Add new command to assist users migrate their keys from the legacy on-disk keybase to the new OS keyring-based implementation. Ref #4754

stale bot added the wontfix label Oct 11, 2019

alessio mentioned this pull request Oct 12, 2019

Drop on-disk keybase in favor of keyring #5180

Merged

5 tasks

stale bot added the stale label Nov 10, 2019

stale bot closed this Nov 19, 2019

Switch secret store to the keyring secret store #4754

Switch secret store to the keyring secret store #4754

Conversation

poldsam commented Jul 21, 2019 • edited by fedekunze Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alessio commented Sep 11, 2019

stale bot commented Oct 11, 2019

stale bot commented Nov 10, 2019

poldsam commented Jul 21, 2019 •

edited by fedekunze

Loading