fix: file keyring fails to add/import/export keys when input is not stdin (fix #9566) (#9821)

## Description

Add a test case to reproduce the issue described in #9566. The test currently fails, and I've pointed some possible solutions over https://github.com/cosmos/cosmos-sdk/issues/9566#issuecomment-889281861. But I feel this needs more works in order to provide a more robust solution. I'll keep poking at better options, but taking any pointers if anyone has ideas.

client/context.go

@@ -1,6 +1,7 @@
 package client
 
 import (
+	"bufio"
 	"encoding/json"
 	"io"
 	"os"
@@ -68,7 +69,10 @@ func (ctx Context) WithKeyringOptions(opts ...keyring.Option) Context {
 
 // WithInput returns a copy of the context with an updated input.
 func (ctx Context) WithInput(r io.Reader) Context {
-	ctx.Input = r
+	// convert to a bufio.Reader to have a shared buffer between the keyring and the
+	// the Commands, ensuring a read from one advance the read pointer for the other.
+	// see https://github.com/cosmos/cosmos-sdk/issues/9566.
+	ctx.Input = bufio.NewReader(r)
 	return ctx
 }
 

client/keys/add.go

@@ -82,12 +82,12 @@ Example:
 }
 
 func runAddCmdPrepare(cmd *cobra.Command, args []string) error {
-	buf := bufio.NewReader(cmd.InOrStdin())
 	clientCtx, err := client.GetClientQueryContext(cmd)
 	if err != nil {
 		return err
 	}
 
+	buf := bufio.NewReader(clientCtx.Input)
 	return runAddCmd(clientCtx, cmd, args, buf)
 }
 

client/keys/add_test.go

@@ -18,6 +18,7 @@ import (
 	"github.com/cosmos/cosmos-sdk/simapp"
 	"github.com/cosmos/cosmos-sdk/testutil"
 	sdk "github.com/cosmos/cosmos-sdk/types"
+	bip39 "github.com/cosmos/go-bip39"
 )
 
 func Test_runAddCmdBasic(t *testing.T) {
@@ -30,7 +31,7 @@ func Test_runAddCmdBasic(t *testing.T) {
 	kb, err := keyring.New(sdk.KeyringServiceName(), keyring.BackendTest, kbHome, mockIn)
 	require.NoError(t, err)
 
-	clientCtx := client.Context{}.WithKeyringDir(kbHome)
+	clientCtx := client.Context{}.WithKeyringDir(kbHome).WithInput(mockIn)
 	ctx := context.WithValue(context.Background(), client.ClientContextKey, &clientCtx)
 
 	t.Cleanup(func() {
@@ -227,3 +228,47 @@ func Test_runAddCmdDryRun(t *testing.T) {
 		})
 	}
 }
+
+func TestAddRecoverFileBackend(t *testing.T) {
+	cmd := AddKeyCommand()
+	cmd.Flags().AddFlagSet(Commands("home").PersistentFlags())
+
+	mockIn := testutil.ApplyMockIODiscardOutErr(cmd)
+	kbHome := t.TempDir()
+
+	clientCtx := client.Context{}.WithKeyringDir(kbHome).WithInput(mockIn)
+	ctx := context.WithValue(context.Background(), client.ClientContextKey, &clientCtx)
+
+	cmd.SetArgs([]string{
+		"keyname1",
+		fmt.Sprintf("--%s=%s", flags.FlagHome, kbHome),
+		fmt.Sprintf("--%s=%s", cli.OutputFlag, OutputFormatText),
+		fmt.Sprintf("--%s=%s", flags.FlagKeyAlgorithm, string(hd.Secp256k1Type)),
+		fmt.Sprintf("--%s=%s", flags.FlagKeyringBackend, keyring.BackendFile),
+		fmt.Sprintf("--%s", flagRecover),
+	})
+
+	keyringPassword := "12345678"
+
+	entropySeed, err := bip39.NewEntropy(mnemonicEntropySize)
+	require.NoError(t, err)
+
+	mnemonic, err := bip39.NewMnemonic(entropySeed)
+	require.NoError(t, err)
+
+	mockIn.Reset(fmt.Sprintf("%s\n%s\n%s\n", mnemonic, keyringPassword, keyringPassword))
+	require.NoError(t, cmd.ExecuteContext(ctx))
+
+	kb, err := keyring.New(sdk.KeyringServiceName(), keyring.BackendFile, kbHome, mockIn)
+	require.NoError(t, err)
+
+	t.Cleanup(func() {
+		mockIn.Reset(fmt.Sprintf("%s\n%s\n", keyringPassword, keyringPassword))
+		_ = kb.Delete("keyname1")
+	})
+
+	mockIn.Reset(fmt.Sprintf("%s\n%s\n", keyringPassword, keyringPassword))
+	info, err := kb.Key("keyname1")
+	require.NoError(t, err)
+	require.Equal(t, "keyname1", info.GetName())
+}

client/keys/export.go

@@ -31,11 +31,11 @@ FULLY AWARE OF THE RISKS. If you are unsure, you may want to do some research
 and export your keys in ASCII-armored encrypted format.`,
 		Args: cobra.ExactArgs(1),
 		RunE: func(cmd *cobra.Command, args []string) error {
-			buf := bufio.NewReader(cmd.InOrStdin())
 			clientCtx, err := client.GetClientQueryContext(cmd)
 			if err != nil {
 				return err
 			}
+			buf := bufio.NewReader(clientCtx.Input)
 			unarmored, _ := cmd.Flags().GetBool(flagUnarmoredHex)
 			unsafe, _ := cmd.Flags().GetBool(flagUnsafe)
 

client/keys/export_test.go

@@ -1,6 +1,7 @@
 package keys
 
 import (
+	"bufio"
 	"context"
 	"fmt"
 	"testing"
@@ -17,55 +18,95 @@ import (
 )
 
 func Test_runExportCmd(t *testing.T) {
-	cmd := ExportKeyCommand()
+	testCases := []struct {
-	cmd.Flags().AddFlagSet(Commands("home").PersistentFlags())
+		name           string
-	mockIn := testutil.ApplyMockIODiscardOutErr(cmd)
+		keyringBackend string
-
+		extraArgs      []string
-	// Now add a temporary keybase
+		userInput      string
-	kbHome := t.TempDir()
+		mustFail       bool
-
+		expectedOutput string
-	// create a key
+	}{
-	kb, err := keyring.New(sdk.KeyringServiceName(), keyring.BackendTest, kbHome, mockIn)
+		{
-	require.NoError(t, err)
+			name:           "--unsafe only must fail",
-	t.Cleanup(func() {
+			keyringBackend: keyring.BackendTest,
-		kb.Delete("keyname1") // nolint:errcheck
+			extraArgs:      []string{"--unsafe"},
-	})
+			mustFail:       true,
-
+		},
-	path := sdk.GetConfig().GetFullBIP44Path()
+		{
-	_, err = kb.NewAccount("keyname1", testutil.TestMnemonic, "", path, hd.Secp256k1)
+			name:           "--unarmored-hex must fail",
-	require.NoError(t, err)
+			keyringBackend: keyring.BackendTest,
-
+			extraArgs:      []string{"--unarmored-hex"},
-	// Now enter password
+			mustFail:       true,
-	args := []string{
+		},
-		"keyname1",
+		{
-		fmt.Sprintf("--%s=%s", flags.FlagHome, kbHome),
+			name:           "--unsafe --unarmored-hex fail with no user confirmation",
-		fmt.Sprintf("--%s=%s", flags.FlagKeyringBackend, keyring.BackendTest),
+			keyringBackend: keyring.BackendTest,
+			extraArgs:      []string{"--unsafe", "--unarmored-hex"},
+			userInput:      "",
+			mustFail:       true,
+			expectedOutput: "",
+		},
+		{
+			name:           "--unsafe --unarmored-hex succeed",
+			keyringBackend: keyring.BackendTest,
+			extraArgs:      []string{"--unsafe", "--unarmored-hex"},
+			userInput:      "y\n",
+			mustFail:       false,
+			expectedOutput: "2485e33678db4175dc0ecef2d6e1fc493d4a0d7f7ce83324b6ed70afe77f3485\n",
+		},
+		{
+			name:           "file keyring backend properly read password and user confirmation",
+			keyringBackend: keyring.BackendFile,
+			extraArgs:      []string{"--unsafe", "--unarmored-hex"},
+			// first 2 pass for creating the key, then unsafe export confirmation, then unlock keyring pass
+			userInput:      "12345678\n12345678\ny\n12345678\n",
+			mustFail:       false,
+			expectedOutput: "2485e33678db4175dc0ecef2d6e1fc493d4a0d7f7ce83324b6ed70afe77f3485\n",
+		},
 	}
 
-	mockIn.Reset("123456789\n123456789\n")
+	for _, tc := range testCases {
-	cmd.SetArgs(args)
+		t.Run(tc.name, func(t *testing.T) {
+			kbHome := t.TempDir()
+			defaultArgs := []string{
+				"keyname1",
+				fmt.Sprintf("--%s=%s", flags.FlagHome, kbHome),
+				fmt.Sprintf("--%s=%s", flags.FlagKeyringBackend, tc.keyringBackend),
+			}
 
-	clientCtx := client.Context{}.
+			cmd := ExportKeyCommand()
-		WithKeyringDir(kbHome).
+			cmd.Flags().AddFlagSet(Commands("home").PersistentFlags())
-		WithKeyring(kb)
-	ctx := context.WithValue(context.Background(), client.ClientContextKey, &clientCtx)
 
-	require.NoError(t, cmd.ExecuteContext(ctx))
+			cmd.SetArgs(append(defaultArgs, tc.extraArgs...))
+			mockIn, mockOut := testutil.ApplyMockIO(cmd)
 
-	argsUnsafeOnly := append(args, "--unsafe")
+			mockIn.Reset(tc.userInput)
-	cmd.SetArgs(argsUnsafeOnly)
+			mockInBuf := bufio.NewReader(mockIn)
-	require.Error(t, cmd.ExecuteContext(ctx))
 
-	argsUnarmoredHexOnly := append(args, "--unarmored-hex")
+			// create a key
-	cmd.SetArgs(argsUnarmoredHexOnly)
+			kb, err := keyring.New(sdk.KeyringServiceName(), tc.keyringBackend, kbHome, bufio.NewReader(mockInBuf))
-	require.Error(t, cmd.ExecuteContext(ctx))
+			require.NoError(t, err)
+			t.Cleanup(func() {
+				kb.Delete("keyname1") // nolint:errcheck
+			})
 
-	argsUnsafeUnarmoredHex := append(args, "--unsafe", "--unarmored-hex")
+			path := sdk.GetConfig().GetFullBIP44Path()
-	cmd.SetArgs(argsUnsafeUnarmoredHex)
+			_, err = kb.NewAccount("keyname1", testutil.TestMnemonic, "", path, hd.Secp256k1)
-	require.Error(t, cmd.ExecuteContext(ctx))
+			require.NoError(t, err)
 
-	mockIn, mockOut := testutil.ApplyMockIO(cmd)
+			clientCtx := client.Context{}.
-	mockIn.Reset("y\n")
+				WithKeyringDir(kbHome).
-	require.NoError(t, cmd.ExecuteContext(ctx))
+				WithKeyring(kb).
-	require.Equal(t, "2485e33678db4175dc0ecef2d6e1fc493d4a0d7f7ce83324b6ed70afe77f3485\n", mockOut.String())
+				WithInput(mockInBuf)
+			ctx := context.WithValue(context.Background(), client.ClientContextKey, &clientCtx)
+
+			err = cmd.ExecuteContext(ctx)
+			if tc.mustFail {
+				require.Error(t, err)
+			} else {
+				require.NoError(t, err)
+				require.Equal(t, tc.expectedOutput, mockOut.String())
+			}
+		})
+	}
 }

client/keys/import.go

@@ -18,11 +18,11 @@ func ImportKeyCommand() *cobra.Command {
 		Long:  "Import a ASCII armored private key into the local keybase.",
 		Args:  cobra.ExactArgs(2),
 		RunE: func(cmd *cobra.Command, args []string) error {
-			buf := bufio.NewReader(cmd.InOrStdin())
 			clientCtx, err := client.GetClientQueryContext(cmd)
 			if err != nil {
 				return err
 			}
+			buf := bufio.NewReader(clientCtx.Input)
 
 			bz, err := ioutil.ReadFile(args[1])
 			if err != nil {

client/keys/import_test.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"fmt"
 	"io/ioutil"
+	"os"
 	"path/filepath"
 	"testing"
 
@@ -17,25 +18,50 @@ import (
 )
 
 func Test_runImportCmd(t *testing.T) {
-	cmd := ImportKeyCommand()
+	testCases := []struct {
-	cmd.Flags().AddFlagSet(Commands("home").PersistentFlags())
+		name           string
-	mockIn := testutil.ApplyMockIODiscardOutErr(cmd)
+		keyringBackend string
+		userInput      string
+		expectError    bool
+	}{
+		{
+			name:           "test backend success",
+			keyringBackend: keyring.BackendTest,
+			// key armor passphrase
+			userInput: "123456789\n",
+		},
+		{
+			name:           "test backend fail with wrong armor pass",
+			keyringBackend: keyring.BackendTest,
+			userInput:      "987654321\n",
+			expectError:    true,
+		},
+		{
+			name:           "file backend success",
+			keyringBackend: keyring.BackendFile,
+			// key armor passphrase + keyring password x2
+			userInput: "123456789\n12345678\n12345678\n",
+		},
+		{
+			name:           "file backend fail with wrong armor pass",
+			keyringBackend: keyring.BackendFile,
+			userInput:      "987654321\n12345678\n12345678\n",
+			expectError:    true,
+		},
+		{
+			name:           "file backend fail with wrong keyring pass",
+			keyringBackend: keyring.BackendFile,
+			userInput:      "123465789\n12345678\n87654321\n",
+			expectError:    true,
+		},
+		{
+			name:           "file backend fail with no keyring pass",
+			keyringBackend: keyring.BackendFile,
+			userInput:      "123465789\n",
+			expectError:    true,
+		},
+	}
 
-	// Now add a temporary keybase
-	kbHome := t.TempDir()
-	kb, err := keyring.New(sdk.KeyringServiceName(), keyring.BackendTest, kbHome, mockIn)
-
-	clientCtx := client.Context{}.
-		WithKeyringDir(kbHome).
-		WithKeyring(kb)
-	ctx := context.WithValue(context.Background(), client.ClientContextKey, &clientCtx)
-
-	require.NoError(t, err)
-	t.Cleanup(func() {
-		kb.Delete("keyname1") // nolint:errcheck
-	})
-
-	keyfile := filepath.Join(kbHome, "key.asc")
 	armoredKey := `-----BEGIN TENDERMINT PRIVATE KEY-----
 salt: A790BB721D1C094260EA84F5E5B72289
 kdf: bcrypt
@@ -45,12 +71,48 @@ HbP+c6JmeJy9JXe2rbbF1QtCX1gLqGcDQPBXiCtFvP7/8wTZtVOPj8vREzhZ9ElO
 =f3l4
 -----END TENDERMINT PRIVATE KEY-----
 `
-	require.NoError(t, ioutil.WriteFile(keyfile, []byte(armoredKey), 0644))
 
-	mockIn.Reset("123456789\n")
+	for _, tc := range testCases {
-	cmd.SetArgs([]string{
+		t.Run(tc.name, func(t *testing.T) {
-		"keyname1", keyfile,
+			cmd := ImportKeyCommand()
-		fmt.Sprintf("--%s=%s", flags.FlagKeyringBackend, keyring.BackendTest),
+			cmd.Flags().AddFlagSet(Commands("home").PersistentFlags())
-	})
+			mockIn := testutil.ApplyMockIODiscardOutErr(cmd)
-	require.NoError(t, cmd.ExecuteContext(ctx))
+
+			// Now add a temporary keybase
+			kbHome := t.TempDir()
+			kb, err := keyring.New(sdk.KeyringServiceName(), tc.keyringBackend, kbHome, nil)
+
+			clientCtx := client.Context{}.
+				WithKeyringDir(kbHome).
+				WithKeyring(kb).
+				WithInput(mockIn)
+			ctx := context.WithValue(context.Background(), client.ClientContextKey, &clientCtx)
+
+			require.NoError(t, err)
+			t.Cleanup(func() {
+				kb.Delete("keyname1") // nolint:errcheck
+			})
+
+			keyfile := filepath.Join(kbHome, "key.asc")
+
+			require.NoError(t, ioutil.WriteFile(keyfile, []byte(armoredKey), 0644))
+
+			defer func() {
+				_ = os.RemoveAll(kbHome)
+			}()
+
+			mockIn.Reset(tc.userInput)
+			cmd.SetArgs([]string{
+				"keyname1", keyfile,
+				fmt.Sprintf("--%s=%s", flags.FlagKeyringBackend, tc.keyringBackend),
+			})
+
+			err = cmd.ExecuteContext(ctx)
+			if tc.expectError {
+				require.Error(t, err)
+			} else {
+				require.NoError(t, err)
+			}
+		})
+	}
 }