Switch secret store to the keyring secret store

.gitignore

@@ -47,3 +47,4 @@ dependency-graph.png
 *.aux
 *.out
 *.synctex.gz
+.clog.yml

CHANGELOG.md

@@ -39,9 +39,15 @@ Ref: https://keepachangelog.com/en/1.0.0/
 
 ### API Breaking Changes
 
-* (store) [\#4748](https://github.com/cosmos/cosmos-sdk/pull/4748) The `CommitMultiStore` interface
-now requires a `SetInterBlockCache` method. Applications that do not wish to support this can simply
-have this method perform a no-op.
+* (sdk) [\#4754](https://github.com/cosmos/cosmos-sdk/issues/4754) Switching back-end to new secret store using library [Keyring](https://github.com/99designs/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
+  * Security audit recommendation was to use the OS secret store 
+  * 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` 
+  * Signing transactions is allowed on the legacy store if you pass in the legacy secret store flag
+  * Running the tests locally may require entering your user password to access your keystore large amount of times (if anyone has a workaround, please leave a comment, thanks) 
 * (modules) [\#4665](https://github.com/cosmos/cosmos-sdk/issues/4665) Refactored `x/gov` module structure and dev-UX:
   * Prepare for module spec integration
   * Update gov keys to use big endian encoding instead of little endian

client/alias.go

@@ -63,8 +63,6 @@ const (
 )
 
 var (
-	// functions aliases
-	NewCLIContextWithFrom              = context.NewCLIContextWithFrom
 	NewCLIContext                      = context.NewCLIContext
 	GetFromFields                      = context.GetFromFields
 	ErrInvalidAccount                  = context.ErrInvalidAccount

client/context/context.go

@@ -1,6 +1,7 @@
 package context
 
 import (
+	"bufio"
 	"bytes"
 	"fmt"
 	"io"
@@ -36,6 +37,7 @@ type CLIContext struct {
 	Client        rpcclient.Client
 	Keybase       cryptokeys.Keybase
 	Output        io.Writer
+	Input         io.Reader
 	OutputFormat  string
 	Height        int64
 	NodeURI       string
@@ -51,22 +53,17 @@ type CLIContext struct {
 	FromName      string
 	Indent        bool
 	SkipConfirm   bool
+	SecretStore   bool
 }
 
 // NewCLIContextWithFrom returns a new initialized CLIContext with parameters from the
 // command line using Viper. It takes a key name or address and populates the FromName and
 // FromAddress field accordingly.
-func NewCLIContextWithFrom(from string) CLIContext {
+func NewCLIContext() CLIContext {
 	var nodeURI string
 	var rpc rpcclient.Client
 
 	genOnly := viper.GetBool(flags.FlagGenerateOnly)
-	fromAddress, fromName, err := GetFromFields(from, genOnly)
-	if err != nil {
-		fmt.Printf("failed to get from fields: %v", err)
-		os.Exit(1)
-	}
-
 	if !genOnly {
 		nodeURI = viper.GetString(flags.FlagNode)
 		if nodeURI != "" {
@@ -80,7 +77,7 @@ func NewCLIContextWithFrom(from string) CLIContext {
 		verifierHome = viper.GetString(flags.FlagHome)
 	}
 
-	return CLIContext{
+	return CLIContext{ //assign value to new boolean var based on parsing the flag
 		Client:        rpc,
 		Output:        os.Stdout,
 		NodeURI:       nodeURI,
@@ -93,17 +90,12 @@ func NewCLIContextWithFrom(from string) CLIContext {
 		Verifier:      verifier,
 		Simulate:      viper.GetBool(flags.FlagDryRun),
 		GenerateOnly:  genOnly,
-		FromAddress:   fromAddress,
-		FromName:      fromName,
 		Indent:        viper.GetBool(flags.FlagIndentResponse),
 		SkipConfirm:   viper.GetBool(flags.FlagSkipConfirmation),
+		SecretStore:   viper.GetBool(flags.FlagLegacy),
 	}
 }
 
-// NewCLIContext returns a new initialized CLIContext with parameters from the
-// command line using Viper.
-func NewCLIContext() CLIContext { return NewCLIContextWithFrom(viper.GetString(flags.FlagFrom)) }
-
 func createVerifier() tmlite.Verifier {
 	trustNodeDefined := viper.IsSet(flags.FlagTrustNode)
 	if !trustNodeDefined {
@@ -238,6 +230,29 @@ func (ctx CLIContext) WithBroadcastMode(mode string) CLIContext {
 	return ctx
 }
 
+// WithInput returns a copy of the context with an updated input.
+func (ctx CLIContext) WithInput(input io.Reader) CLIContext {
+	ctx.Input = bufio.NewReader(input)
+	return ctx
+
+}
+
+// WithSecretStore returns a copy of the context with an updated SecretStore flag.
+func (ctx CLIContext) WithSecretStore() CLIContext {
+	ctx.SecretStore = viper.GetBool(flags.FlagLegacy)
+
+	if ctx.SecretStore {
+		var err error
+		ctx.Keybase, err = keys.NewKeyBaseFromHomeFlag()
+		if err != nil {
+			panic(err)
+		}
+	} else {
+		ctx.Keybase = keys.NewKeyringKeybase(ctx.Input) //if flag is set, add flag to struct, then boolean variable.
+	}
+	return ctx
+}
+
 // PrintOutput prints output while respecting output and indent flags
 // NOTE: pass in marshalled structs that have been unmarshaled
 // because this function will panic on marshaling errors
@@ -264,14 +279,32 @@ func (ctx CLIContext) PrintOutput(toPrint fmt.Stringer) (err error) {
 	return
 }
 
+// WithFromFields returns a copy of the context with an updated FromName and FromAddres flag.
+func (ctx CLIContext) WithFromFields() CLIContext {
+	from := viper.GetString(flags.FlagFrom)
+
+	fromAddress, fromName, err := GetFromFields(from, ctx.GenerateOnly, ctx.Input)
+
+	if err != nil {
+		panic(err)
+	}
+
+	ctx.FromAddress = fromAddress
+	ctx.FromName = fromName
+	return ctx
+
+}
+
 // GetFromFields returns a from account address and Keybase name given either
 // an address or key name. If genOnly is true, only a valid Bech32 cosmos
 // address is returned.
-func GetFromFields(from string, genOnly bool) (sdk.AccAddress, string, error) {
+func GetFromFields(from string, genOnly bool, input io.Reader) (sdk.AccAddress, string, error) {
 	if from == "" {
 		return nil, "", nil
 	}
 
+	legacySecretStore := viper.GetBool(flags.FlagLegacy)
+
 	if genOnly {
 		addr, err := sdk.AccAddressFromBech32(from)
 		if err != nil {
@@ -281,11 +314,16 @@ func GetFromFields(from string, genOnly bool) (sdk.AccAddress, string, error) {
 		return addr, "", nil
 	}
 
-	keybase, err := keys.NewKeyBaseFromHomeFlag()
-	if err != nil {
-		return nil, "", err
+	var keybase cryptokeys.Keybase
+	if legacySecretStore {
+		var err error
+		keybase, err = keys.NewKeyBaseFromHomeFlag()
+		if err != nil {
+			return nil, "", err
+		}
+	} else {
+		keybase = keys.NewKeyringKeybase(input) //if flag is set, add flag to struct, then boolean variable.
 	}
-
 	var info cryptokeys.Info
 	if addr, err := sdk.AccAddressFromBech32(from); err == nil {
 		info, err = keybase.GetByAddress(addr)

client/flags/flags.go

@@ -54,6 +54,7 @@ const (
 	FlagRPCWriteTimeout    = "write-timeout"
 	FlagOutputDocument     = "output-document" // inspired by wget -O
 	FlagSkipConfirmation   = "yes"
+	FlagLegacy             = "legacy"
 )
 
 // LineBreak can be included in a command list to provide a blank line
@@ -99,6 +100,7 @@ func PostCommands(cmds ...*cobra.Command) []*cobra.Command {
 		c.Flags().Bool(FlagDryRun, false, "ignore the --gas flag and perform a simulation of a transaction, but don't broadcast it")
 		c.Flags().Bool(FlagGenerateOnly, false, "Build an unsigned transaction and write it to STDOUT (when enabled, the local Keybase is not accessible and the node operates offline)")
 		c.Flags().BoolP(FlagSkipConfirmation, "y", false, "Skip tx broadcasting prompt confirmation")
+		c.Flags().Bool(FlagLegacy, false, "Use legacy secret store")
 
 		// --gas can accept integers and "simulate"
 		c.Flags().Var(&GasFlagVar, "gas", fmt.Sprintf(

client/keys/add.go

@@ -72,6 +72,7 @@ the flag --nosort is set.
 	cmd.Flags().Uint32(flagAccount, 0, "Account number for HD derivation")
 	cmd.Flags().Uint32(flagIndex, 0, "Address index number for HD derivation")
 	cmd.Flags().Bool(flags.FlagIndentResponse, false, "Add indent to JSON response")
+	cmd.Flags().Bool(flags.FlagLegacy, false, "Use legacy secret store")
 	return cmd
 }
 
@@ -101,11 +102,15 @@ func runAddCmd(cmd *cobra.Command, args []string) error {
 		kb = keys.NewInMemory()
 		encryptPassword = DefaultKeyPass
 	} else {
-		kb, err = NewKeyBaseFromHomeFlag()
-		if err != nil {
-			return err
+		if viper.GetBool(flags.FlagLegacy) {
+			cmd.PrintErrf("IMPORTANT: using deprecated secret store. This will be removed in a future release.")
+			kb, err = NewKeyBaseFromHomeFlag()
+			if err != nil {
+				return err
+			}
+		} else {
+			kb = NewKeyringKeybase(inBuf)
 		}
-
 		_, err = kb.Get(name)
 		if err == nil {
 			// account exists, ask for user confirmation
@@ -152,13 +157,15 @@ func runAddCmd(cmd *cobra.Command, args []string) error {
 		}
 
 		// ask for a password when generating a local key
-		if viper.GetString(FlagPublicKey) == "" && !viper.GetBool(flags.FlagUseLedger) {
+		if viper.GetString(FlagPublicKey) == "" && !viper.GetBool(flags.FlagUseLedger) && viper.GetBool(flags.FlagLegacy) {
 			encryptPassword, err = input.GetCheckPassword(
 				"Enter a passphrase to encrypt your key to disk:",
 				"Repeat the passphrase:", inBuf)
 			if err != nil {
 				return err
 			}
+		} else {
+			encryptPassword = DefaultKeyPass
 		}
 	}
 

client/keys/add_ledger_test.go

@@ -89,8 +89,11 @@ func Test_runAddCmdLedger(t *testing.T) {
 	assert.NoError(t, runAddCmd(cmd, []string{"keyname1"}))
 
 	// Now check that it has been stored properly
-	kb, err := NewKeyBaseFromHomeFlag()
-	assert.NoError(t, err)
+	kb := NewKeyringKeybase(mockIn)
+	defer func() {
+		kb.Delete("keyname1", "", false)
+	}()
+
 	assert.NotNil(t, kb)
 	key1, err := kb.Get("keyname1")
 	assert.NoError(t, err)

client/keys/add_test.go

@@ -1,8 +1,10 @@
 package keys
 
 import (
+	"fmt"
 	"testing"
 
+	"github.com/99designs/keyring"
 	"github.com/spf13/viper"
 	"github.com/stretchr/testify/assert"
 
@@ -13,8 +15,17 @@ import (
 )
 
 func Test_runAddCmdBasic(t *testing.T) {
+
+	backends := keyring.AvailableBackends()
+
+	runningOnServer := false
+
+	if len(backends) == 2 && backends[1] == keyring.BackendType("file") {
+		runningOnServer = true
+	}
 	cmd := addKeyCommand()
 	assert.NotNil(t, cmd)
+
 	mockIn, _, _ := tests.ApplyMockIO(cmd)
 
 	kbHome, kbCleanUp := tests.NewTestCaseDir(t)
@@ -24,25 +35,52 @@ func Test_runAddCmdBasic(t *testing.T) {
 
 	viper.Set(cli.OutputFlag, OutputFormatText)
 
-	mockIn.Reset("test1234\ntest1234\n")
+	if runningOnServer {
+		mockIn.Reset("testpass1\ntestpass1\n")
+
+	} else {
+		mockIn.Reset("y\n")
+		kb := NewKeyringKeybase(mockIn)
+		defer func() {
+			kb.Delete("keyname1", "", false)
+			kb.Delete("keyname2", "", false)
+		}()
+	}
 	err := runAddCmd(cmd, []string{"keyname1"})
 	assert.NoError(t, err)
 
 	viper.Set(cli.OutputFlag, OutputFormatText)
 
-	mockIn.Reset("test1234\ntest1234\n")
+	if runningOnServer {
+		mockIn.Reset("testpass1\nN\n")
+
+	} else {
+		mockIn.Reset("N\n")
+	}
 	err = runAddCmd(cmd, []string{"keyname1"})
+	fmt.Println(err)
 	assert.Error(t, err)
 
 	viper.Set(cli.OutputFlag, OutputFormatText)
 
-	mockIn.Reset("y\ntest1234\ntest1234\n")
+	if runningOnServer {
+		mockIn.Reset("testpass1\ny\ntestpass1\n")
+
+	} else {
+		mockIn.Reset("y\n")
+	}
 	err = runAddCmd(cmd, []string{"keyname1"})
 	assert.NoError(t, err)
 
 	viper.Set(cli.OutputFlag, OutputFormatJSON)
 
-	mockIn.Reset("test1234\ntest1234\n")
+	if runningOnServer {
+		mockIn.Reset("testpass1\n")
+
+	} else {
+		mockIn.Reset("y\n")
+	}
 	err = runAddCmd(cmd, []string{"keyname2"})
 	assert.NoError(t, err)
+
 }