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

Switch secret store to the keyring secret store #4754

Closed
wants to merge 120 commits into from

Conversation

poldsam
Copy link
Contributor

@poldsam poldsam commented Jul 21, 2019

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)

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

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

Choose a reason for hiding this comment

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

undeclared name: signed (from typecheck)

client/keys/add.go Outdated Show resolved Hide resolved
client/keys/migrate.go Outdated Show resolved Hide resolved
types/rest/rest_test.go Outdated Show resolved Hide resolved
x/distribution/client/common/common_test.go Outdated Show resolved Hide resolved
}

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

Choose a reason for hiding this comment

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

undeclared name: signed (from typecheck)

client/keys/add.go Outdated Show resolved Hide resolved
client/keys/migrate.go Outdated Show resolved Hide resolved
types/rest/rest_test.go Outdated Show resolved Hide resolved
x/distribution/client/common/common_test.go Outdated Show resolved Hide resolved
crypto/keys/keybase_keyring.go Outdated Show resolved Hide resolved
crypto/keys/keybase_keyring.go Outdated Show resolved Hide resolved
crypto/keys/keybase_keyring.go Outdated Show resolved Hide resolved
var keyhash []byte
keyhashStored := false

if _, err := os.Stat(lkb.dir + "/keyhash"); err == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

x/nft/client/cli/tx.go Outdated Show resolved Hide resolved

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

Choose a reason for hiding this comment

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

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

algo := Secp256k1

n1, n2, n3 := "some dude", "a dudette", "dude-ish"
p1, p2, p3 := "1234", "foobar", "foobar"
Copy link
Contributor

Choose a reason for hiding this comment

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

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

types/rest/rest_test.go Outdated Show resolved Hide resolved
x/distribution/client/common/common_test.go Outdated Show resolved Hide resolved
crypto/keys/lazy_keybase_keyring.go Outdated Show resolved Hide resolved
crypto/keys/keybase_keyring.go Outdated Show resolved Hide resolved
var keyhash []byte
keyhashStored := false

if _, err := os.Stat(lkb.dir + "/keyhash"); err == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

x/nft/client/cli/tx.go Outdated Show resolved Hide resolved
crypto/keys/keybase_keyring.go Outdated Show resolved Hide resolved

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

Choose a reason for hiding this comment

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

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

algo := Secp256k1

n1, n2, n3 := "some dude", "a dudette", "dude-ish"
p1, p2, p3 := "1234", "foobar", "foobar"
Copy link
Contributor

Choose a reason for hiding this comment

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

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

types/rest/rest_test.go Outdated Show resolved Hide resolved
x/distribution/client/common/common_test.go Outdated Show resolved Hide resolved
crypto/keys/lazy_keybase_keyring.go Outdated Show resolved Hide resolved
poldsam and others added 2 commits September 5, 2019 16:46
Co-Authored-By: Bot from GolangCI <42910462+golangcibot@users.noreply.github.com>
Co-Authored-By: Bot from GolangCI <42910462+golangcibot@users.noreply.github.com>
crypto/keys/keybase_keyring.go Outdated Show resolved Hide resolved
var keyhash []byte
keyhashStored := false

if _, err := os.Stat(lkb.dir + "/keyhash"); err == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

File is not goimports-ed (from goimports)

Suggested change
return
return

algo := Secp256k1

n1, n2, n3 := "some dude", "a dudette", "dude-ish"
p1, p2, p3 := "1234", "foobar", "foobar"
Copy link
Contributor

Choose a reason for hiding this comment

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

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


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

Choose a reason for hiding this comment

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

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

types/rest/rest_test.go Outdated Show resolved Hide resolved
x/distribution/client/common/common_test.go Outdated Show resolved Hide resolved
crypto/keys/lazy_keybase_keyring.go Outdated Show resolved Hide resolved
crypto/keys/keybase_keyring.go Outdated Show resolved Hide resolved
var keyhash []byte
keyhashStored := false

if _, err := os.Stat(lkb.dir + "/keyhash"); err == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

File is not goimports-ed (from goimports)

Suggested change
return
return


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

Choose a reason for hiding this comment

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

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

algo := Secp256k1

n1, n2, n3 := "some dude", "a dudette", "dude-ish"
p1, p2, p3 := "1234", "foobar", "foobar"
Copy link
Contributor

Choose a reason for hiding this comment

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

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

types/rest/rest_test.go Outdated Show resolved Hide resolved
x/distribution/client/common/common_test.go Outdated Show resolved Hide resolved
crypto/keys/lazy_keybase_keyring.go Outdated Show resolved Hide resolved
crypto/keys/keybase_keyring.go Outdated Show resolved Hide resolved
var keyhash []byte
keyhashStored := false

if _, err := os.Stat(lkb.dir + "/keyhash"); err == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

Suggested change
return
return


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

Choose a reason for hiding this comment

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

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

algo := Secp256k1

n1, n2, n3 := "some dude", "a dudette", "dude-ish"
p1, p2, p3 := "1234", "foobar", "foobar"
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@@ -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())
Copy link
Contributor

Choose a reason for hiding this comment

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

undeclared name: bufio (from typecheck)

types/rest/rest_test.go Outdated Show resolved Hide resolved
x/distribution/client/common/common_test.go Outdated Show resolved Hide resolved
crypto/keys/lazy_keybase_keyring.go Outdated Show resolved Hide resolved
buf := bufio.NewReader(os.Stdin)
_, err = fmt.Fprintf(os.Stderr, "\nEnter Amino-encoded signature:\n")
if err != nil {
return
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Suggested change
return
return

crypto/keys/keybase_keyring.go Outdated Show resolved Hide resolved
var keyhash []byte
keyhashStored := false

if _, err := os.Stat(lkb.dir + "/keyhash"); err == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

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


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

Choose a reason for hiding this comment

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

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

algo := Secp256k1

n1, n2, n3 := "some dude", "a dudette", "dude-ish"
p1, p2, p3 := "1234", "foobar", "foobar"
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@@ -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())
Copy link
Contributor

Choose a reason for hiding this comment

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

undeclared name: bufio (from typecheck)

types/rest/rest_test.go Outdated Show resolved Hide resolved
x/distribution/client/common/common_test.go Outdated Show resolved Hide resolved
crypto/keys/lazy_keybase_keyring.go Outdated Show resolved Hide resolved
var keyhash []byte
keyhashStored := false

if _, err := os.Stat(lkb.dir + "/keyhash"); err == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

Suggested change
return
return

algo := Secp256k1

n1, n2, n3 := "some dude", "a dudette", "dude-ish"
p1, p2, p3 := "1234", "foobar", "foobar"
Copy link
Contributor

Choose a reason for hiding this comment

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

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


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

Choose a reason for hiding this comment

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

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

@@ -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())
Copy link
Contributor

Choose a reason for hiding this comment

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

undeclared name: bufio (from typecheck)

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

Choose a reason for hiding this comment

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

"os" imported but not used (from typecheck)

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

import (
"os"
Copy link
Contributor

Choose a reason for hiding this comment

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

"os" imported but not used (from typecheck)

alessio pushed a commit that referenced this pull request Sep 11, 2019
This chunk is extracted from @poldsam's original PR:
- #4754
@alessio
Copy link
Contributor

alessio commented Sep 11, 2019

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

alessio pushed a commit that referenced this pull request Sep 21, 2019
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
@alessio alessio mentioned this pull request Sep 24, 2019
5 tasks
alexanderbez pushed a commit that referenced this pull request Sep 30, 2019
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
Copy link

stale bot commented Oct 11, 2019

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 stale bot added the wontfix label Oct 11, 2019
@stale
Copy link

stale bot commented Nov 10, 2019

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 stale bot added the stale label Nov 10, 2019
@stale stale bot closed this Nov 19, 2019
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.

6 participants