Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: check multisig key list to prevent unexpected key deletion #737

Merged
merged 11 commits into from
Oct 21, 2022
Merged

fix: check multisig key list to prevent unexpected key deletion #737

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
fe3a2a1
fix: check multisig key list to prevent unexpected key deletion
tkxkd0159 Oct 18, 2022
df13c19
add CHANGELOG
tkxkd0159 Oct 18, 2022
96bb379
use map instead of array to reduce time complexity
tkxkd0159 Oct 19, 2022
277f7c1
Merge branch 'main' into add01
tkxkd0159 Oct 19, 2022
30ad6d0
fix: reject multisig key addition about duplicate name
tkxkd0159 Oct 19, 2022
de7a8bb
Merge branch 'main' into add01
tkxkd0159 Oct 19, 2022
b3333a1
Merge branch 'main' into add01
tkxkd0159 Oct 19, 2022
db6be20
test: add multisig unit test
tkxkd0159 Oct 19, 2022
096e3b0
fix: optimize flag replacing logic
tkxkd0159 Oct 19, 2022
8910f5d
replace custom iteration with keyring built-in function
tkxkd0159 Oct 21, 2022
f5683ee
add test for multisig threshold
tkxkd0159 Oct 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,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
Expand Down
45 changes: 34 additions & 11 deletions client/keys/add.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
tkxkd0159 marked this conversation as resolved.
Show resolved Hide resolved

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)
Expand All @@ -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 {
tkxkd0159 marked this conversation as resolved.
Show resolved Hide resolved
return err
}

err = verifyMultisigTarget(kb, multisigKeys, name)
if err != nil {
return err
}
}

if dryRun, _ := cmd.Flags().GetBool(flags.FlagDryRun); dryRun {
// use in memory keybase
Expand All @@ -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)
Expand Down Expand Up @@ -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 {
zemyblue marked this conversation as resolved.
Show resolved Hide resolved
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
}
87 changes: 81 additions & 6 deletions client/keys/add_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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")
Expand Down Expand Up @@ -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))
Expand All @@ -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",
Expand All @@ -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
Expand Down