From 1e538735b30a012bafcdb03688a823a421269352 Mon Sep 17 00:00:00 2001 From: Jayden Lee <41176085+tkxkd0159@users.noreply.github.com> Date: Fri, 21 Oct 2022 15:10:49 +0900 Subject: [PATCH] fix: check multisig key list to prevent unexpected key deletion (#737) * fix: reject multisig key addition about duplicate name * test: add multisig unit test * fix: optimize flag replacing logic * replace custom iteration with keyring built-in function * add test for multisig threshold --- CHANGELOG.md | 1 + client/keys/add.go | 45 +++++++++++++++------ client/keys/add_test.go | 87 ++++++++++++++++++++++++++++++++++++++--- 3 files changed, 116 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c2927d068d..d2594697cc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -116,6 +116,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * (x/foundation) [\#732](https://github.com/line/lbm-sdk/pull/732) add verification on accounts into x/foundation Grants cli * (x/foundation) [\#730](https://github.com/line/lbm-sdk/pull/730) prune stale x/foundation proposals at voting period end * (cli) [\#734](https://github.com/line/lbm-sdk/pull/734) add restrictions on the number of args in the CLIs +* (client) [\#737](https://github.com/line/lbm-sdk/pull/737) check multisig key list to prevent unexpected key deletion ### Breaking Changes * (proto) [\#564](https://github.com/line/lbm-sdk/pull/564) change gRPC path to original cosmos path diff --git a/client/keys/add.go b/client/keys/add.go index cb64b87448..3038a62672 100644 --- a/client/keys/add.go +++ b/client/keys/add.go @@ -8,7 +8,7 @@ import ( "math" "sort" - bip39 "github.com/cosmos/go-bip39" + "github.com/cosmos/go-bip39" "github.com/spf13/cobra" "github.com/line/lbm-sdk/client" @@ -95,15 +95,17 @@ func runAddCmdPrepare(cmd *cobra.Command, args []string) error { /* input - - bip39 mnemonic - - bip39 passphrase - - bip44 path - - local encryption password + - bip39 mnemonic + - bip39 passphrase + - bip44 path + - local encryption password + output - - armor encrypted private key (saved to file) + - armor encrypted private key (saved to file) */ func runAddCmd(ctx client.Context, cmd *cobra.Command, args []string, inBuf *bufio.Reader) error { var err error + var multisigThreshold int name := args[0] interactive, _ := cmd.Flags().GetBool(flagInteractive) @@ -118,6 +120,18 @@ func runAddCmd(ctx client.Context, cmd *cobra.Command, args []string, inBuf *buf if err != nil { return err } + multisigKeys, _ := cmd.Flags().GetStringSlice(flagMultisig) + if len(multisigKeys) != 0 { + multisigThreshold, _ = cmd.Flags().GetInt(flagMultiSigThreshold) + if err = validateMultisigThreshold(multisigThreshold, len(multisigKeys)); err != nil { + return err + } + + err = verifyMultisigTarget(kb, multisigKeys, name) + if err != nil { + return err + } + } if dryRun, _ := cmd.Flags().GetBool(flags.FlagDryRun); dryRun { // use in memory keybase @@ -141,13 +155,8 @@ func runAddCmd(ctx client.Context, cmd *cobra.Command, args []string, inBuf *buf } } - multisigKeys, _ := cmd.Flags().GetStringSlice(flagMultisig) if len(multisigKeys) != 0 { pks := make([]cryptotypes.PubKey, len(multisigKeys)) - multisigThreshold, _ := cmd.Flags().GetInt(flagMultiSigThreshold) - if err := validateMultisigThreshold(multisigThreshold, len(multisigKeys)); err != nil { - return err - } for i, keyname := range multisigKeys { k, err := kb.Key(keyname) @@ -327,3 +336,17 @@ func printCreate(cmd *cobra.Command, info keyring.Info, showMnemonic bool, mnemo return nil } + +func verifyMultisigTarget(kb keyring.Keyring, multisigKeys []string, newkey string) error { + if _, err := kb.Key(newkey); err == nil { + return errors.New("you cannot specify a new key as one of the names of the keys that make up a multisig") + } + + for _, k := range multisigKeys { + if _, err := kb.Key(k); err != nil { + return errors.New("part of the multisig target key does not exist") + } + } + + return nil +} diff --git a/client/keys/add_test.go b/client/keys/add_test.go index a968152985..0f35b42487 100644 --- a/client/keys/add_test.go +++ b/client/keys/add_test.go @@ -7,7 +7,8 @@ import ( "io" "testing" - bip39 "github.com/cosmos/go-bip39" + "github.com/cosmos/go-bip39" + "github.com/spf13/pflag" "github.com/stretchr/testify/require" "github.com/line/ostracon/libs/cli" @@ -32,8 +33,9 @@ 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).WithInput(mockIn) - ctx := context.WithValue(context.Background(), client.ClientContextKey, &clientCtx) + clientCtx := client.Context{}.WithKeyringDir(kbHome).WithInput(mockIn).WithKeyring(kb) + clientCtxPtr := &clientCtx + ctx := context.WithValue(context.Background(), client.ClientContextKey, clientCtxPtr) t.Cleanup(func() { _ = kb.Delete("keyname1") @@ -62,7 +64,6 @@ func Test_runAddCmdBasic(t *testing.T) { }) require.NoError(t, cmd.ExecuteContext(ctx)) - require.Error(t, cmd.ExecuteContext(ctx)) mockIn.Reset("y\n") require.NoError(t, cmd.ExecuteContext(ctx)) @@ -76,7 +77,81 @@ func Test_runAddCmdBasic(t *testing.T) { }) require.NoError(t, cmd.ExecuteContext(ctx)) - require.Error(t, cmd.ExecuteContext(ctx)) + + // In Multisig + tcs := []struct { + args []string + err string + }{ + {[]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.BackendTest), + fmt.Sprintf("--%s=%s", flagMultisig, "keyname1,keyname2"), + }, + "you cannot specify a new key as one of the names of the keys that make up a multisig", + }, + {[]string{ + "keyname-multi", + 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.BackendTest), + fmt.Sprintf("--%s=%s", flagMultisig, "keyname1,keyname11"), + }, + "part of the multisig target key does not exist", + }, + {[]string{ + "keyname-multi", + 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.BackendTest), + fmt.Sprintf("--%s=%s", flagMultisig, "keyname1,keyname2"), + fmt.Sprintf("--%s=%d", flagMultiSigThreshold, 3), + }, + "threshold k of n multisignature", + }, + {[]string{ + "keyname-multi", + 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.BackendTest), + fmt.Sprintf("--%s=%s", flagMultisig, "keyname1,keyname2"), + fmt.Sprintf("--%s=%d", flagMultiSigThreshold, -1), + }, + "threshold must be a positive integer", + }, + {[]string{ + "keyname-multi", + 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.BackendTest), + fmt.Sprintf("--%s=%s", flagMultisig, "keyname1,keyname2"), + fmt.Sprintf("--%s=%d", flagMultiSigThreshold, 2), + }, + "", + }, + } + + for _, tc := range tcs { + cmd.SetArgs(tc.args) + if tc.err != "" { + require.Contains(t, cmd.ExecuteContext(ctx).Error(), tc.err) + } else { + require.NoError(t, cmd.ExecuteContext(ctx)) + } + + cmd.Flags().Visit(func(f *pflag.Flag) { + if f.Name == flagMultisig { + f.Value.(pflag.SliceValue).Replace([]string{}) + } + }) + } cmd.SetArgs([]string{ "keyname5", @@ -85,7 +160,7 @@ func Test_runAddCmdBasic(t *testing.T) { fmt.Sprintf("--%s=%s", cli.OutputFlag, OutputFormatText), fmt.Sprintf("--%s=%s", flags.FlagKeyAlgorithm, string(hd.Secp256k1Type)), }) - + mockIn.Reset("\n") require.NoError(t, cmd.ExecuteContext(ctx)) // In recovery mode