From 0215b5c6cd69b6a57fae787156bbf13c8458c16f Mon Sep 17 00:00:00 2001 From: Federico Kunze <31522760+fedekunze@users.noreply.github.com> Date: Thu, 11 Jun 2020 16:30:42 +0200 Subject: [PATCH] client: fix signing algorithm (#6405) * crypto/keyring: fix signing algorithm * client: tests * minor fixes * changelog * address @alexanderbez comments * Update crypto/keyring/keyring.go * Update crypto/keyring/signing_algorithms.go * Update crypto/keyring/keyring.go * Update crypto/keyring/signing_algorithms.go Co-authored-by: Alessio Treglia * fix test Co-authored-by: Alessio Treglia --- CHANGELOG.md | 1 + client/keys/add.go | 5 ++-- client/keys/add_ledger_test.go | 13 +++++++--- client/keys/add_test.go | 9 +++++-- crypto/keyring/keyring.go | 29 +++++++++++++++------- crypto/keyring/signing_algorithms.go | 30 +++++++++++++++++------ crypto/keyring/signing_algorithms_test.go | 20 +++++++++------ 7 files changed, 75 insertions(+), 32 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 706c4d556..544d5e061 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -160,6 +160,7 @@ be used to retrieve the actual proposal `Content`. Also the `NewMsgSubmitProposa ### Bug Fixes +* (client) [\#6402](https://github.com/cosmos/cosmos-sdk/issues/6402) Fix `keys add` `--algo` flag which only worked for Tendermint's `secp256k1` default key signing algorithm. * (x/bank) [\#6283](https://github.com/cosmos/cosmos-sdk/pull/6283) Create account if recipient does not exist on handing `MsgMultiSend`. * (x/distribution) [\#6210](https://github.com/cosmos/cosmos-sdk/pull/6210) Register `MsgFundCommunityPool` in distribution amino codec. * (x/staking) [\#6061](https://github.com/cosmos/cosmos-sdk/pull/6061) Allow a validator to immediately unjail when no signing info is present due to diff --git a/client/keys/add.go b/client/keys/add.go index 1159148ed..529fcf410 100644 --- a/client/keys/add.go +++ b/client/keys/add.go @@ -120,9 +120,10 @@ func RunAddCmd(cmd *cobra.Command, args []string, kb keyring.Keyring, inBuf *buf interactive := viper.GetBool(flagInteractive) showMnemonic := !viper.GetBool(flagNoBackup) - algo, err := keyring.NewSigningAlgoFromString(viper.GetString(flagKeyAlgo)) + keyringAlgos, _ := kb.SupportedAlgorithms() + algo, err := keyring.NewSigningAlgoFromString(viper.GetString(flagKeyAlgo), keyringAlgos) if err != nil { - algo = hd.Secp256k1 + return err } if !viper.GetBool(flags.FlagDryRun) { diff --git a/client/keys/add_ledger_test.go b/client/keys/add_ledger_test.go index dc78c778a..e19db3dd9 100644 --- a/client/keys/add_ledger_test.go +++ b/client/keys/add_ledger_test.go @@ -11,6 +11,7 @@ import ( "github.com/tendermint/tendermint/libs/cli" "github.com/cosmos/cosmos-sdk/client/flags" + "github.com/cosmos/cosmos-sdk/crypto/hd" "github.com/cosmos/cosmos-sdk/crypto/keyring" "github.com/cosmos/cosmos-sdk/tests" sdk "github.com/cosmos/cosmos-sdk/types" @@ -45,8 +46,10 @@ func Test_runAddCmdLedgerWithCustomCoinType(t *testing.T) { viper.Set(flagIndex, "0") viper.Set(flagCoinType, "330") - /// Test Text + // Test Text viper.Set(cli.OutputFlag, OutputFormatText) + // set algo flag value to the default + viper.Set(flagKeyAlgo, string(hd.Secp256k1Type)) // Now enter password mockIn, _, _ := tests.ApplyMockIO(cmd) mockIn.Reset("test1234\ntest1234\n") @@ -57,7 +60,7 @@ func Test_runAddCmdLedgerWithCustomCoinType(t *testing.T) { require.NoError(t, err) require.NotNil(t, kb) t.Cleanup(func() { - kb.Delete("keyname1") + _ = kb.Delete("keyname1") }) mockIn.Reset("test1234\n") key1, err := kb.Key("keyname1") @@ -89,8 +92,10 @@ func Test_runAddCmdLedger(t *testing.T) { viper.Set(flags.FlagHome, kbHome) viper.Set(flags.FlagUseLedger, true) - /// Test Text + // Test Text viper.Set(cli.OutputFlag, OutputFormatText) + // set algo flag value to the default + viper.Set(flagKeyAlgo, string(hd.Secp256k1Type)) // Now enter password mockIn.Reset("test1234\ntest1234\n") viper.Set(flagCoinType, sdk.CoinType) @@ -101,7 +106,7 @@ func Test_runAddCmdLedger(t *testing.T) { require.NoError(t, err) require.NotNil(t, kb) t.Cleanup(func() { - kb.Delete("keyname1") + _ = kb.Delete("keyname1") }) mockIn.Reset("test1234\n") key1, err := kb.Key("keyname1") diff --git a/client/keys/add_test.go b/client/keys/add_test.go index 5776aa8b0..d5532ef3d 100644 --- a/client/keys/add_test.go +++ b/client/keys/add_test.go @@ -9,6 +9,7 @@ import ( "github.com/tendermint/tendermint/libs/cli" "github.com/cosmos/cosmos-sdk/client/flags" + "github.com/cosmos/cosmos-sdk/crypto/hd" "github.com/cosmos/cosmos-sdk/crypto/keyring" "github.com/cosmos/cosmos-sdk/tests" sdk "github.com/cosmos/cosmos-sdk/types" @@ -17,6 +18,7 @@ import ( func Test_runAddCmdBasic(t *testing.T) { cmd := AddKeyCommand() require.NotNil(t, cmd) + mockIn, _, _ := tests.ApplyMockIO(cmd) kbHome, kbCleanUp := tests.NewTestCaseDir(t) @@ -27,11 +29,14 @@ func Test_runAddCmdBasic(t *testing.T) { viper.Set(flags.FlagUseLedger, false) mockIn.Reset("y\n") + // set algo flag value to the default + viper.Set(flagKeyAlgo, string(hd.Secp256k1Type)) + kb, err := keyring.New(sdk.KeyringServiceName(), viper.GetString(flags.FlagKeyringBackend), kbHome, mockIn) require.NoError(t, err) t.Cleanup(func() { - kb.Delete("keyname1") // nolint:errcheck - kb.Delete("keyname2") // nolint:errcheck + _ = kb.Delete("keyname1") + _ = kb.Delete("keyname2") }) require.NoError(t, runAddCmd(cmd, []string{"keyname1"})) diff --git a/crypto/keyring/keyring.go b/crypto/keyring/keyring.go index 846bd5546..7074bb04c 100644 --- a/crypto/keyring/keyring.go +++ b/crypto/keyring/keyring.go @@ -14,18 +14,18 @@ import ( "github.com/99designs/keyring" "github.com/cosmos/go-bip39" "github.com/pkg/errors" - "github.com/tendermint/crypto/bcrypt" tmcrypto "github.com/tendermint/tendermint/crypto" + cryptoamino "github.com/tendermint/tendermint/crypto/encoding/amino" "github.com/cosmos/cosmos-sdk/client/input" "github.com/cosmos/cosmos-sdk/crypto" - cryptoAmino "github.com/cosmos/cosmos-sdk/crypto/codec" "github.com/cosmos/cosmos-sdk/crypto/hd" sdk "github.com/cosmos/cosmos-sdk/types" sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" ) +// Backend options for Keyring const ( BackendFile = "file" BackendOS = "os" @@ -51,6 +51,9 @@ type Keyring interface { // List all keys. List() ([]Info, error) + // Supported signing algorithms for Keyring and Ledger respectively. + SupportedAlgorithms() (SigningAlgoList, SigningAlgoList) + // Key and KeyByAddress return keys by uid and address respectively. Key(uid string) (Info, error) KeyByAddress(address sdk.Address) (Info, error) @@ -114,9 +117,11 @@ type Exporter interface { // Option overrides keyring configuration options. type Option func(options *Options) -//Options define the options of the Keyring +// Options define the options of the Keyring. type Options struct { - SupportedAlgos SigningAlgoList + // supported signing algorithms for keyring + SupportedAlgos SigningAlgoList + // supported signing algorithms for Ledger SupportedAlgosLedger SigningAlgoList } @@ -127,7 +132,7 @@ func NewInMemory(opts ...Option) Keyring { return newKeystore(keyring.NewArrayKeyring(nil), opts...) } -// NewKeyring creates a new instance of a keyring. +// New creates a new instance of a keyring. // Keyring ptions can be applied when generating the new instance. // Available backends are "os", "file", "kwallet", "memory", "pass", "test". func New( @@ -233,7 +238,7 @@ func (ks keystore) ExportPrivateKeyObject(uid string) (tmcrypto.PrivKey, error) return nil, err } - priv, err = cryptoAmino.PrivKeyFromBytes([]byte(linfo.PrivKeyArmor)) + priv, err = cryptoamino.PrivKeyFromBytes([]byte(linfo.PrivKeyArmor)) if err != nil { return nil, err } @@ -282,7 +287,7 @@ func (ks keystore) ImportPubKey(uid string, armor string) error { return err } - pubKey, err := cryptoAmino.PubKeyFromBytes(pubBytes) + pubKey, err := cryptoamino.PubKeyFromBytes(pubBytes) if err != nil { return err } @@ -309,7 +314,7 @@ func (ks keystore) Sign(uid string, msg []byte) ([]byte, tmcrypto.PubKey, error) return nil, nil, fmt.Errorf("private key not available") } - priv, err = cryptoAmino.PrivKeyFromBytes([]byte(i.PrivKeyArmor)) + priv, err = cryptoamino.PrivKeyFromBytes([]byte(i.PrivKeyArmor)) if err != nil { return nil, nil, err } @@ -518,6 +523,12 @@ func (ks keystore) Key(uid string) (Info, error) { return unmarshalInfo(bs.Data) } +// SupportedAlgorithms returns the keystore Options' supported signing algorithm. +// for the keyring and Ledger. +func (ks keystore) SupportedAlgorithms() (SigningAlgoList, SigningAlgoList) { + return ks.options.SupportedAlgos, ks.options.SupportedAlgosLedger +} + // SignWithLedger signs a binary message with the ledger device referenced by an Info object // and returns the signed bytes and the public key. It returns an error if the device could // not be queried or it returned an error. @@ -690,7 +701,7 @@ func (ks keystore) writeInfo(info Info) error { exists, err := ks.existsInDb(info) if exists { - return fmt.Errorf("public key already exist in keybase") + return errors.New("public key already exist in keybase") } if err != nil { diff --git a/crypto/keyring/signing_algorithms.go b/crypto/keyring/signing_algorithms.go index 425f49eb8..920dd7b55 100644 --- a/crypto/keyring/signing_algorithms.go +++ b/crypto/keyring/signing_algorithms.go @@ -2,28 +2,34 @@ package keyring import ( "fmt" + "strings" "github.com/cosmos/cosmos-sdk/crypto/hd" ) +// SignatureAlgo defines the interface for a keyring supported algorithm. type SignatureAlgo interface { Name() hd.PubKeyType Derive() hd.DeriveFn Generate() hd.GenerateFn } -func NewSigningAlgoFromString(str string) (SignatureAlgo, error) { - if str != string(hd.Secp256k1.Name()) { - return nil, fmt.Errorf("provided algorithm `%s` is not supported", str) +// NewSigningAlgoFromString creates a supported SignatureAlgo. +func NewSigningAlgoFromString(str string, algoList SigningAlgoList) (SignatureAlgo, error) { + for _, algo := range algoList { + if str == string(algo.Name()) { + return algo, nil + } } - - return hd.Secp256k1, nil + return nil, fmt.Errorf("provided algorithm %q is not supported", str) } +// SigningAlgoList is a slice of signature algorithms type SigningAlgoList []SignatureAlgo -func (l SigningAlgoList) Contains(algo SignatureAlgo) bool { - for _, cAlgo := range l { +// Contains returns true if the SigningAlgoList the given SignatureAlgo. +func (sal SigningAlgoList) Contains(algo SignatureAlgo) bool { + for _, cAlgo := range sal { if cAlgo.Name() == algo.Name() { return true } @@ -31,3 +37,13 @@ func (l SigningAlgoList) Contains(algo SignatureAlgo) bool { return false } + +// String returns a comma separated string of the signature algorithm names in the list. +func (sal SigningAlgoList) String() string { + names := make([]string, len(sal)) + for i := range sal { + names[i] = string(sal[i].Name()) + } + + return strings.Join(names, ",") +} diff --git a/crypto/keyring/signing_algorithms_test.go b/crypto/keyring/signing_algorithms_test.go index 1293249b0..14283b910 100644 --- a/crypto/keyring/signing_algorithms_test.go +++ b/crypto/keyring/signing_algorithms_test.go @@ -4,7 +4,6 @@ import ( "fmt" "testing" - "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/cosmos/cosmos-sdk/crypto/hd" @@ -30,13 +29,15 @@ func TestNewSigningAlgoByString(t *testing.T) { "notsupportedalgo", false, nil, - fmt.Errorf("provided algorithm `notsupportedalgo` is not supported"), + fmt.Errorf("provided algorithm \"notsupportedalgo\" is not supported"), }, } + + list := SigningAlgoList{hd.Secp256k1} for _, tt := range tests { tt := tt t.Run(tt.name, func(t *testing.T) { - algorithm, err := NewSigningAlgoFromString(tt.algoStr) + algorithm, err := NewSigningAlgoFromString(tt.algoStr, list) if tt.isSupported { require.Equal(t, hd.Secp256k1, algorithm) } else { @@ -47,12 +48,15 @@ func TestNewSigningAlgoByString(t *testing.T) { } func TestAltSigningAlgoList_Contains(t *testing.T) { - list := SigningAlgoList{ - hd.Secp256k1, - } + list := SigningAlgoList{hd.Secp256k1} - assert.True(t, list.Contains(hd.Secp256k1)) - assert.False(t, list.Contains(notSupportedAlgo{})) + require.True(t, list.Contains(hd.Secp256k1)) + require.False(t, list.Contains(notSupportedAlgo{})) +} + +func TestAltSigningAlgoList_String(t *testing.T) { + list := SigningAlgoList{hd.Secp256k1, notSupportedAlgo{}} + require.Equal(t, fmt.Sprintf("%s,notSupported", string(hd.Secp256k1Type)), list.String()) } type notSupportedAlgo struct {