From f6ad78f56004e7e9dc9f696510725e2c551ed757 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. (cherry picked from commit f479b515a8ce2353ab583525a029f7e68dad4e5f) # Conflicts: # client/keys/add.go # client/keys/add_test.go # client/keys/export_test.go --- client/context.go | 6 +- client/keys/add.go | 6 +- client/keys/add_test.go | 160 ++++++++++++++++++++++++++++++++++++- client/keys/export.go | 2 +- client/keys/export_test.go | 119 +++++++++++++++++++++------ client/keys/import.go | 2 +- client/keys/import_test.go | 114 ++++++++++++++++++++------ 7 files changed, 352 insertions(+), 57 deletions(-) diff --git a/client/context.go b/client/context.go index cacdb6ee961c..44ccbc259cd5 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.go b/client/keys/add.go index 20c142c777b3..6f847a7fe4f8 100644 --- a/client/keys/add.go +++ b/client/keys/add.go @@ -83,13 +83,17 @@ the flag --nosort is set. } func runAddCmdPrepare(cmd *cobra.Command, args []string) error { - buf := bufio.NewReader(cmd.InOrStdin()) clientCtx, err := client.GetClientQueryContext(cmd) if err != nil { return err } +<<<<<<< HEAD return RunAddCmd(clientCtx, cmd, args, buf) +======= + buf := bufio.NewReader(clientCtx.Input) + return runAddCmd(clientCtx, cmd, args, buf) +>>>>>>> f479b515a (fix: file keyring fails to add/import/export keys when input is not stdin (fix #9566) (#9821)) } /* diff --git a/client/keys/add_test.go b/client/keys/add_test.go index aa6f68876a2e..4e513884189b 100644 --- a/client/keys/add_test.go +++ b/client/keys/add_test.go @@ -15,6 +15,7 @@ import ( "github.com/cosmos/cosmos-sdk/crypto/keyring" "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) { @@ -27,7 +28,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() { @@ -114,3 +115,160 @@ func Test_runAddCmdBasic(t *testing.T) { mockIn.Reset("\n" + password + "\n" + "fail" + "\n") require.Error(t, cmd.ExecuteContext(ctx)) } +<<<<<<< HEAD +======= + +func Test_runAddCmdDryRun(t *testing.T) { + pubkey1 := `{"@type":"/cosmos.crypto.secp256k1.PubKey","key":"AtObiFVE4s+9+RX5SP8TN9r2mxpoaT4eGj9CJfK7VRzN"}` + pubkey2 := `{"@type":"/cosmos.crypto.secp256k1.PubKey","key":"A/se1vkqgdQ7VJQCM4mxN+L+ciGhnnJ4XYsQCRBMrdRi"}` + + testData := []struct { + name string + args []string + added bool + }{ + { + name: "account is added", + args: []string{ + "testkey", + fmt.Sprintf("--%s=%s", flags.FlagDryRun, "false"), + }, + added: true, + }, + { + name: "account is not added with dry run", + args: []string{ + "testkey", + fmt.Sprintf("--%s=%s", flags.FlagDryRun, "true"), + }, + added: false, + }, + { + name: "multisig account is added", + args: []string{ + "testkey", + fmt.Sprintf("--%s=%s", flags.FlagDryRun, "false"), + fmt.Sprintf("--%s=%s", flagMultisig, "subkey"), + }, + added: true, + }, + { + name: "multisig account is not added with dry run", + args: []string{ + "testkey", + fmt.Sprintf("--%s=%s", flags.FlagDryRun, "true"), + fmt.Sprintf("--%s=%s", flagMultisig, "subkey"), + }, + added: false, + }, + { + name: "pubkey account is added", + args: []string{ + "testkey", + fmt.Sprintf("--%s=%s", flags.FlagDryRun, "false"), + fmt.Sprintf("--%s=%s", FlagPublicKey, pubkey1), + }, + added: true, + }, + { + name: "pubkey account is not added with dry run", + args: []string{ + "testkey", + fmt.Sprintf("--%s=%s", flags.FlagDryRun, "true"), + fmt.Sprintf("--%s=%s", FlagPublicKey, pubkey2), + }, + added: false, + }, + } + for _, tt := range testData { + tt := tt + t.Run(tt.name, func(t *testing.T) { + cmd := AddKeyCommand() + cmd.Flags().AddFlagSet(Commands("home").PersistentFlags()) + + kbHome := t.TempDir() + mockIn := testutil.ApplyMockIODiscardOutErr(cmd) + kb, err := keyring.New(sdk.KeyringServiceName(), keyring.BackendTest, kbHome, mockIn) + require.NoError(t, err) + + appCodec := simapp.MakeTestEncodingConfig().Codec + clientCtx := client.Context{}. + WithCodec(appCodec). + WithKeyringDir(kbHome). + WithKeyring(kb) + ctx := context.WithValue(context.Background(), client.ClientContextKey, &clientCtx) + + path := sdk.GetConfig().GetFullBIP44Path() + _, err = kb.NewAccount("subkey", testutil.TestMnemonic, "", path, hd.Secp256k1) + require.NoError(t, err) + + t.Cleanup(func() { + _ = kb.Delete("subkey") + }) + + b := bytes.NewBufferString("") + cmd.SetOut(b) + + cmd.SetArgs(tt.args) + require.NoError(t, cmd.ExecuteContext(ctx)) + + if tt.added { + _, err = kb.Key("testkey") + require.NoError(t, err) + + out, err := ioutil.ReadAll(b) + require.NoError(t, err) + require.Contains(t, string(out), "name: testkey") + } else { + _, err = kb.Key("testkey") + require.Error(t, err) + require.Equal(t, "testkey.info: key not found", err.Error()) + } + }) + } +} + +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()) +} +>>>>>>> f479b515a (fix: file keyring fails to add/import/export keys when input is not stdin (fix #9566) (#9821)) diff --git a/client/keys/export.go b/client/keys/export.go index 7e9cb88ed633..13491b5e839a 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 b207a71bb159..3508db5bf0f5 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,6 +18,7 @@ import ( ) func Test_runExportCmd(t *testing.T) { +<<<<<<< HEAD cmd := ExportKeyCommand() cmd.Flags().AddFlagSet(Commands("home").PersistentFlags()) mockIn := testutil.ApplyMockIODiscardOutErr(cmd) @@ -40,32 +42,97 @@ func Test_runExportCmd(t *testing.T) { "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", + }, +>>>>>>> f479b515a (fix: file keyring fails to add/import/export keys when input is not stdin (fix #9566) (#9821)) } - 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 2b7e230654ce..36662a8dd2e6 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 ea84408c2df5..ac05ed567daa 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) + } + }) + } }