Skip to content

Commit

Permalink
fix: file keyring fails to add/import/export keys when input is not s…
Browse files Browse the repository at this point in the history
…tdin (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 #9566 (comment). 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 f479b51)

# Conflicts:
#	client/keys/add.go
#	client/keys/add_test.go
#	client/keys/export_test.go
  • Loading branch information
daeMOn63 authored and mergify-bot committed Aug 9, 2021
1 parent ed5d165 commit f6ad78f
Show file tree
Hide file tree
Showing 7 changed files with 352 additions and 57 deletions.
6 changes: 5 additions & 1 deletion client/context.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package client

import (
"bufio"
"encoding/json"
"io"
"os"
Expand Down Expand Up @@ -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
}

Expand Down
6 changes: 5 additions & 1 deletion client/keys/add.go
Original file line number Diff line number Diff line change
Expand Up @@ -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))
}

/*
Expand Down
160 changes: 159 additions & 1 deletion client/keys/add_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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() {
Expand Down Expand Up @@ -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))
2 changes: 1 addition & 1 deletion client/keys/export.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
119 changes: 93 additions & 26 deletions client/keys/export_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package keys

import (
"bufio"
"context"
"fmt"
"testing"
Expand All @@ -17,6 +18,7 @@ import (
)

func Test_runExportCmd(t *testing.T) {
<<<<<<< HEAD
cmd := ExportKeyCommand()
cmd.Flags().AddFlagSet(Commands("home").PersistentFlags())
mockIn := testutil.ApplyMockIODiscardOutErr(cmd)
Expand All @@ -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())
}
})
}
}
2 changes: 1 addition & 1 deletion client/keys/import.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
Loading

0 comments on commit f6ad78f

Please sign in to comment.