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 cosmos#9566) (cosmos#9821) (cosmos#9881)

## Description

Add a test case to reproduce the issue described in cosmos#9566. The test currently fails, and I've pointed some possible solutions over cosmos#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)

Co-authored-by: daeMOn <flavien.binet@gmail.com>
  • Loading branch information
@mergify @daeMOn63 @JeancarloBarrios
2 people authored and JeancarloBarrios committed Sep 28, 2024
1 parent 4a32ead commit 22ef4b8
Show file tree
Hide file tree
Showing 5 changed files with 57 additions and 187 deletions.
14 changes: 12 additions & 2 deletions client/context.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package client

import (
"bufio"
"context"
"encoding/json"
"fmt"
"io"
Expand Down Expand Up @@ -107,12 +106,23 @@ 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 {
// convert to a bufio.Reader to have a shared buffer between the keyring and the
// Commands, ensuring a read from one advance the read pointer for the other.
// 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
}

// Deprecated: WithJSONCodec returns a copy of the Context with an updated JSONCodec.
func (ctx Context) WithJSONCodec(m codec.JSONCodec) Context {
ctx.JSONCodec = m
// since we are using ctx.Codec everywhere in the SDK, for backward compatibility
// we need to try to set it here as well.
if c, ok := m.(codec.Codec); ok {
ctx.Codec = c
}
return ctx
}

// WithCodec returns a copy of the Context with an updated Codec.
func (ctx Context) WithCodec(m codec.Codec) Context {
ctx.Codec = m
Expand Down
35 changes: 10 additions & 25 deletions client/keys/add_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import (
"github.com/cosmos/cosmos-sdk/testutil"
"github.com/cosmos/cosmos-sdk/testutil/testdata"
sdk "github.com/cosmos/cosmos-sdk/types"
moduletestutil "github.com/cosmos/cosmos-sdk/types/module/testutil"
bip39 "github.com/cosmos/go-bip39"
)

func Test_runAddCmdBasic(t *testing.T) {
Expand All @@ -33,14 +33,7 @@ func Test_runAddCmdBasic(t *testing.T) {
kb, err := keyring.New(sdk.KeyringServiceName(), keyring.BackendTest, kbHome, mockIn, cdc)
require.NoError(t, err)

clientCtx := client.Context{}.
WithKeyringDir(kbHome).
WithInput(mockIn).
WithCodec(cdc).
WithAddressCodec(addresscodec.NewBech32Codec("cosmos")).
WithValidatorAddressCodec(addresscodec.NewBech32Codec("cosmosvaloper")).
WithConsensusAddressCodec(addresscodec.NewBech32Codec("cosmosvalcons"))

clientCtx := client.Context{}.WithKeyringDir(kbHome).WithInput(mockIn)
ctx := context.WithValue(context.Background(), client.ClientContextKey, &clientCtx)

t.Cleanup(func() {
Expand Down Expand Up @@ -348,27 +341,19 @@ func Test_runAddCmdDryRun(t *testing.T) {

func TestAddRecoverFileBackend(t *testing.T) {
cmd := AddKeyCommand()
cmd.Flags().AddFlagSet(Commands().PersistentFlags())
cdc := moduletestutil.MakeTestEncodingConfig(codectestutil.CodecOptions{}).Codec
cmd.Flags().AddFlagSet(Commands("home").PersistentFlags())

mockIn := testutil.ApplyMockIODiscardOutErr(cmd)
kbHome := t.TempDir()

clientCtx := client.Context{}.
WithKeyringDir(kbHome).
WithInput(mockIn).
WithCodec(cdc).
WithAddressCodec(addresscodec.NewBech32Codec("cosmos")).
WithValidatorAddressCodec(addresscodec.NewBech32Codec("cosmosvaloper")).
WithConsensusAddressCodec(addresscodec.NewBech32Codec("cosmosvalcons"))

clientCtx := client.Context{}.WithKeyringDir(kbHome).WithInput(mockIn)
ctx := context.WithValue(context.Background(), client.ClientContextKey, &clientCtx)

cmd.SetArgs([]string{
"keyname1",
fmt.Sprintf("--%s=%s", flags.FlagKeyringDir, kbHome),
fmt.Sprintf("--%s=%s", flags.FlagOutput, flags.OutputFormatText),
fmt.Sprintf("--%s=%s", flags.FlagKeyType, hd.Secp256k1Type),
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),
})
Expand All @@ -384,7 +369,7 @@ func TestAddRecoverFileBackend(t *testing.T) {
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, cdc)
kb, err := keyring.New(sdk.KeyringServiceName(), keyring.BackendFile, kbHome, mockIn)
require.NoError(t, err)

t.Cleanup(func() {
Expand All @@ -393,7 +378,7 @@ func TestAddRecoverFileBackend(t *testing.T) {
})

mockIn.Reset(fmt.Sprintf("%s\n%s\n", keyringPassword, keyringPassword))
k, err := kb.Key("keyname1")
info, err := kb.Key("keyname1")
require.NoError(t, err)
require.Equal(t, "keyname1", k.Name)
require.Equal(t, "keyname1", info.GetName())
}
59 changes: 19 additions & 40 deletions client/keys/export_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,15 +20,13 @@ import (
)

func Test_runExportCmd(t *testing.T) {
cdc := moduletestutil.MakeTestEncodingConfig(codectestutil.CodecOptions{}).Codec
testCases := []struct {
name string
keyringBackend string
extraArgs []string
userInput string
mustFail bool
expectedOutput string
expectedOutputContain string // only valid when expectedOutput is empty
name string
keyringBackend string
extraArgs []string
userInput string
mustFail bool
expectedOutput string
}{
{
name: "--unsafe only must fail",
Expand All @@ -51,33 +49,17 @@ func Test_runExportCmd(t *testing.T) {
expectedOutput: "",
},
{
name: "--unsafe --unarmored-hex --yes success",
keyringBackend: keyring.BackendTest,
extraArgs: []string{"--unsafe", "--unarmored-hex", "--yes"},
userInput: "",
mustFail: false,
expectedOutputContain: "2485e33678db4175dc0ecef2d6e1fc493d4a0d7f7ce83324b6ed70afe77f3485\n",
},
{
name: "--unsafe --unarmored-hex success",
keyringBackend: keyring.BackendTest,
extraArgs: []string{"--unsafe", "--unarmored-hex"},
userInput: "y\n",
mustFail: false,
expectedOutputContain: "2485e33678db4175dc0ecef2d6e1fc493d4a0d7f7ce83324b6ed70afe77f3485\n",
},
{
name: "--unsafe --unarmored-hex --indiscreet success",
name: "--unsafe --unarmored-hex succeed",
keyringBackend: keyring.BackendTest,
extraArgs: []string{"--unsafe", "--unarmored-hex", "--indiscreet"},
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", "--indiscreet"},
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,
Expand All @@ -90,12 +72,12 @@ func Test_runExportCmd(t *testing.T) {
kbHome := t.TempDir()
defaultArgs := []string{
"keyname1",
fmt.Sprintf("--%s=%s", flags.FlagKeyringDir, kbHome),
fmt.Sprintf("--%s=%s", flags.FlagHome, kbHome),
fmt.Sprintf("--%s=%s", flags.FlagKeyringBackend, tc.keyringBackend),
}

cmd := ExportKeyCommand()
cmd.Flags().AddFlagSet(Commands().PersistentFlags())
cmd.Flags().AddFlagSet(Commands("home").PersistentFlags())

cmd.SetArgs(append(defaultArgs, tc.extraArgs...))
mockIn, mockOut := testutil.ApplyMockIO(cmd)
Expand All @@ -104,31 +86,28 @@ func Test_runExportCmd(t *testing.T) {
mockInBuf := bufio.NewReader(mockIn)

// create a key
kb, err := keyring.New(sdk.KeyringServiceName(), tc.keyringBackend, kbHome, bufio.NewReader(mockInBuf), cdc)
kb, err := keyring.New(sdk.KeyringServiceName(), tc.keyringBackend, kbHome, bufio.NewReader(mockInBuf))
require.NoError(t, err)
t.Cleanup(cleanupKeys(t, kb, "keyname1"))
t.Cleanup(func() {
kb.Delete("keyname1") // nolint:errcheck
})

path := sdk.GetFullBIP44Path()
_, err = kb.NewAccount("keyname1", testdata.TestMnemonic, "", path, hd.Secp256k1)
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).
WithCodec(cdc)
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)
if tc.expectedOutput != "" {
require.Equal(t, tc.expectedOutput, mockOut.String())
} else if tc.expectedOutputContain != "" {
require.Contains(t, mockOut.String(), tc.expectedOutputContain)
}
require.Equal(t, tc.expectedOutput, mockOut.String())
}
})
}
Expand Down
4 changes: 0 additions & 4 deletions client/keys/import.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,6 @@ func ImportKeyCommand() *cobra.Command {
if err != nil {
return err
}
name := args[0]
if strings.TrimSpace(name) == "" {
return errors.New("the provided name is invalid or empty after trimming whitespace")
}
buf := bufio.NewReader(clientCtx.Input)

armor, err := os.ReadFile(args[1])
Expand Down
Loading

0 comments on commit 22ef4b8

Please sign in to comment.