From f479b515a8ce2353ab583525a029f7e68dad4e5f Mon Sep 17 00:00:00 2001 From: daeMOn Date: Mon, 9 Aug 2021 12:35:01 +0200 Subject: [PATCH] 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 | 6 +- client/keys/add.go | 2 +- client/keys/add_test.go | 47 ++++++++++++- client/keys/export.go | 2 +- client/keys/export_test.go | 139 ++++++++++++++++++++++++------------- client/keys/import.go | 2 +- client/keys/import_test.go | 114 +++++++++++++++++++++++------- 7 files changed, 232 insertions(+), 80 deletions(-) diff --git a/client/context.go b/client/context.go index 1308823c84..9f81451735 100644 --- a/client/context.go +++ b/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 } diff --git a/client/keys/add.go b/client/keys/add.go index 4b2a1a0a55..d9713fd2d5 100644 --- a/client/keys/add.go +++ b/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) } diff --git a/client/keys/add_test.go b/client/keys/add_test.go index 8e6bf7e5cc..2108380588 100644 --- a/client/keys/add_test.go +++ b/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()) +} diff --git a/client/keys/export.go b/client/keys/export.go index 7e9cb88ed6..13491b5e83 100644 --- a/client/keys/export.go +++ b/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) diff --git a/client/keys/export_test.go b/client/keys/export_test.go index 4282d5d29b..a63cf7f9b7 100644 --- a/client/keys/export_test.go +++ b/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() - cmd.Flags().AddFlagSet(Commands("home").PersistentFlags()) - mockIn := testutil.ApplyMockIODiscardOutErr(cmd) - - // Now add a temporary keybase - kbHome := t.TempDir() - - // create a key - kb, err := keyring.New(sdk.KeyringServiceName(), keyring.BackendTest, kbHome, mockIn) - require.NoError(t, err) - t.Cleanup(func() { - kb.Delete("keyname1") // nolint:errcheck - }) - - path := sdk.GetConfig().GetFullBIP44Path() - _, err = kb.NewAccount("keyname1", testutil.TestMnemonic, "", path, hd.Secp256k1) - require.NoError(t, err) - - // Now enter password - args := []string{ - "keyname1", - fmt.Sprintf("--%s=%s", flags.FlagHome, kbHome), - fmt.Sprintf("--%s=%s", flags.FlagKeyringBackend, keyring.BackendTest), + testCases := []struct { + name string + keyringBackend string + extraArgs []string + userInput string + mustFail bool + expectedOutput string + }{ + { + name: "--unsafe only must fail", + keyringBackend: keyring.BackendTest, + extraArgs: []string{"--unsafe"}, + mustFail: true, + }, + { + name: "--unarmored-hex must fail", + keyringBackend: keyring.BackendTest, + extraArgs: []string{"--unarmored-hex"}, + mustFail: true, + }, + { + name: "--unsafe --unarmored-hex fail with no user confirmation", + 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") - cmd.SetArgs(args) - - clientCtx := client.Context{}. - WithKeyringDir(kbHome). - WithKeyring(kb) - ctx := context.WithValue(context.Background(), client.ClientContextKey, &clientCtx) - - require.NoError(t, cmd.ExecuteContext(ctx)) - - argsUnsafeOnly := append(args, "--unsafe") - cmd.SetArgs(argsUnsafeOnly) - require.Error(t, cmd.ExecuteContext(ctx)) - - argsUnarmoredHexOnly := append(args, "--unarmored-hex") - cmd.SetArgs(argsUnarmoredHexOnly) - require.Error(t, cmd.ExecuteContext(ctx)) - - argsUnsafeUnarmoredHex := append(args, "--unsafe", "--unarmored-hex") - cmd.SetArgs(argsUnsafeUnarmoredHex) - require.Error(t, cmd.ExecuteContext(ctx)) - - mockIn, mockOut := testutil.ApplyMockIO(cmd) - mockIn.Reset("y\n") - require.NoError(t, cmd.ExecuteContext(ctx)) - require.Equal(t, "2485e33678db4175dc0ecef2d6e1fc493d4a0d7f7ce83324b6ed70afe77f3485\n", mockOut.String()) + for _, tc := range testCases { + 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), + } + + cmd := ExportKeyCommand() + cmd.Flags().AddFlagSet(Commands("home").PersistentFlags()) + + cmd.SetArgs(append(defaultArgs, tc.extraArgs...)) + mockIn, mockOut := testutil.ApplyMockIO(cmd) + + mockIn.Reset(tc.userInput) + mockInBuf := bufio.NewReader(mockIn) + + // create a key + kb, err := keyring.New(sdk.KeyringServiceName(), tc.keyringBackend, kbHome, bufio.NewReader(mockInBuf)) + require.NoError(t, err) + t.Cleanup(func() { + kb.Delete("keyname1") // nolint:errcheck + }) + + path := sdk.GetConfig().GetFullBIP44Path() + _, err = kb.NewAccount("keyname1", testutil.TestMnemonic, "", path, hd.Secp256k1) + require.NoError(t, err) + + clientCtx := client.Context{}. + WithKeyringDir(kbHome). + WithKeyring(kb). + 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()) + } + }) + } } diff --git a/client/keys/import.go b/client/keys/import.go index 2b7e230654..36662a8dd2 100644 --- a/client/keys/import.go +++ b/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 { diff --git a/client/keys/import_test.go b/client/keys/import_test.go index ea84408c2d..ac05ed567d 100644 --- a/client/keys/import_test.go +++ b/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() - cmd.Flags().AddFlagSet(Commands("home").PersistentFlags()) - mockIn := testutil.ApplyMockIODiscardOutErr(cmd) + testCases := []struct { + name string + 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") - cmd.SetArgs([]string{ - "keyname1", keyfile, - fmt.Sprintf("--%s=%s", flags.FlagKeyringBackend, keyring.BackendTest), - }) - require.NoError(t, cmd.ExecuteContext(ctx)) + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + cmd := ImportKeyCommand() + cmd.Flags().AddFlagSet(Commands("home").PersistentFlags()) + mockIn := testutil.ApplyMockIODiscardOutErr(cmd) + + // 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) + } + }) + } }