From 38c1e215529601497c7fa6ad6837e9d945fedfa9 Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Tue, 10 Aug 2021 13:56:41 +0200 Subject: [PATCH] fix: file keyring fails to add/import/export keys when input is not stdin (fix #9566) (backport #9821) (#9880) * 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. (cherry picked from commit f479b515a8ce2353ab583525a029f7e68dad4e5f) # Conflicts: # client/keys/add.go # client/keys/add_test.go # client/keys/export_test.go * fix: merge conflict backporting pr-9821 (#9888) Co-authored-by: daeMOn --- client/context.go | 6 +- client/keys/add_test.go | 3 +- client/keys/export_test.go | 143 +++++++++++++++++++++++-------------- client/keys/import_test.go | 118 ++++++++++++++++++++++-------- 4 files changed, 185 insertions(+), 85 deletions(-) diff --git a/client/context.go b/client/context.go index cacdb6ee96..44ccbc259c 100644 --- a/client/context.go +++ b/client/context.go @@ -1,6 +1,7 @@ package client import ( + "bufio" "encoding/json" "io" "os" @@ -60,7 +61,10 @@ func (ctx Context) WithKeyring(k keyring.Keyring) 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_test.go b/client/keys/add_test.go index 758457acc8..6c8af3867d 100644 --- a/client/keys/add_test.go +++ b/client/keys/add_test.go @@ -1,7 +1,6 @@ package keys import ( - "bufio" "context" "fmt" "testing" @@ -124,7 +123,7 @@ func TestAddRecoverFileBackend(t *testing.T) { mockIn := testutil.ApplyMockIODiscardOutErr(cmd) kbHome := t.TempDir() - clientCtx := client.Context{}.WithKeyringDir(kbHome).WithInput(bufio.NewReader(mockIn)) + clientCtx := client.Context{}.WithKeyringDir(kbHome).WithInput(mockIn) ctx := context.WithValue(context.Background(), client.ClientContextKey, &clientCtx) cmd.SetArgs([]string{ diff --git a/client/keys/export_test.go b/client/keys/export_test.go index 66d990b682..7b07bbc6db 100644 --- a/client/keys/export_test.go +++ b/client/keys/export_test.go @@ -1,7 +1,7 @@ package keys import ( - "bytes" + "bufio" "context" "fmt" "testing" @@ -18,58 +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, nil) - require.NoError(t, err) - t.Cleanup(func() { - kb.Delete("keyname1") // nolint:errcheck - }) - - path := sdk.GetConfig().GetFullFundraiserPath() - _, 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). - WithInput(mockIn) - 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)) - - mockOut := bytes.NewBufferString("") - cmd.SetOut(mockOut) - cmd.SetErr(mockOut) - 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().GetFullFundraiserPath() + _, 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_test.go b/client/keys/import_test.go index b12e20e9c1..ac05ed567d 100644 --- a/client/keys/import_test.go +++ b/client/keys/import_test.go @@ -1,10 +1,10 @@ package keys import ( - "bufio" "context" "fmt" "io/ioutil" + "os" "path/filepath" "testing" @@ -18,26 +18,50 @@ import ( ) func Test_runImportCmd(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(), keyring.BackendTest, kbHome, nil) - - clientCtx := client.Context{}. - WithKeyringDir(kbHome). - WithKeyring(kb). - WithInput(bufio.NewReader(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") + 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, + }, + } + armoredKey := `-----BEGIN TENDERMINT PRIVATE KEY----- salt: A790BB721D1C094260EA84F5E5B72289 kdf: bcrypt @@ -47,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) + } + }) + } }