diff --git a/PENDING.md b/PENDING.md index bf8ca309a..e8c6f7bdc 100644 --- a/PENDING.md +++ b/PENDING.md @@ -43,6 +43,7 @@ BUG FIXES * Gaia REST API * Gaia CLI + * [\#3586](https://github.com/cosmos/cosmos-sdk/pull/3586) Incomplete ledger derivation paths in keybase * Gaia * [\#3585] Fix setting the tx hash in `NewResponseFormatBroadcastTxCommit`. @@ -50,4 +51,4 @@ BUG FIXES * SDK * [\#3582](https://github.com/cosmos/cosmos-sdk/pull/3582) Running `make test_unit was failing due to a missing tag -* Tendermint \ No newline at end of file +* Tendermint diff --git a/client/keys/add.go b/client/keys/add.go index d9dfe985f..47b9a19a1 100644 --- a/client/keys/add.go +++ b/client/keys/add.go @@ -77,7 +77,7 @@ the flag --nosort is set. cmd.Flags().Bool(flagNoBackup, false, "Don't print out seed phrase (if others are watching the terminal)") cmd.Flags().Bool(flagDryRun, false, "Perform action, but don't add key to local keystore") cmd.Flags().Uint32(flagAccount, 0, "Account number for HD derivation") - cmd.Flags().Uint32(flagIndex, 0, "Index number for HD derivation") + cmd.Flags().Uint32(flagIndex, 0, "Address index number for HD derivation") return cmd } diff --git a/client/keys/add_ledger_test.go b/client/keys/add_ledger_test.go new file mode 100644 index 000000000..d830fdfd7 --- /dev/null +++ b/client/keys/add_ledger_test.go @@ -0,0 +1,55 @@ +//+build ledger,test_ledger_mock + +package keys + +import ( + "bufio" + "strings" + "testing" + + "github.com/cosmos/cosmos-sdk/crypto/keys" + sdk "github.com/cosmos/cosmos-sdk/types" + + "github.com/spf13/viper" + "github.com/tendermint/tendermint/libs/cli" + + "github.com/cosmos/cosmos-sdk/tests" + + "github.com/cosmos/cosmos-sdk/client" + + "github.com/stretchr/testify/assert" +) + +func Test_runAddCmdLedger(t *testing.T) { + cmd := addKeyCommand() + assert.NotNil(t, cmd) + + // Prepare a keybase + kbHome, kbCleanUp := tests.NewTestCaseDir(t) + assert.NotNil(t, kbHome) + defer kbCleanUp() + viper.Set(cli.HomeFlag, kbHome) + viper.Set(client.FlagUseLedger, true) + + /// Test Text + viper.Set(cli.OutputFlag, OutputFormatText) + // Now enter password + cleanUp1 := client.OverrideStdin(bufio.NewReader(strings.NewReader("test1234\ntest1234\n"))) + defer cleanUp1() + err := runAddCmd(cmd, []string{"keyname1"}) + assert.NoError(t, err) + + // Now check that it has been stored properly + kb, err := NewKeyBaseFromHomeFlag() + assert.NoError(t, err) + assert.NotNil(t, kb) + key1, err := kb.Get("keyname1") + assert.NoError(t, err) + assert.NotNil(t, key1) + + assert.Equal(t, "keyname1", key1.GetName()) + assert.Equal(t, keys.TypeLedger, key1.GetType()) + assert.Equal(t, + "cosmospub1addwnpepqd87l8xhcnrrtzxnkql7k55ph8fr9jarf4hn6udwukfprlalu8lgw0urza0", + sdk.MustBech32ifyAccPub(key1.GetPubKey())) +} diff --git a/client/keys/add_test.go b/client/keys/add_test.go index f20ba27b1..bca21393a 100644 --- a/client/keys/add_test.go +++ b/client/keys/add_test.go @@ -23,10 +23,6 @@ func Test_runAddCmdBasic(t *testing.T) { cmd := addKeyCommand() assert.NotNil(t, cmd) - // Missing input (enter password) - err := runAddCmd(cmd, []string{"keyname"}) - assert.EqualError(t, err, "EOF") - // Prepare a keybase kbHome, kbCleanUp := tests.NewTestCaseDir(t) assert.NotNil(t, kbHome) @@ -38,7 +34,7 @@ func Test_runAddCmdBasic(t *testing.T) { // Now enter password cleanUp1 := client.OverrideStdin(bufio.NewReader(strings.NewReader("test1234\ntest1234\n"))) defer cleanUp1() - err = runAddCmd(cmd, []string{"keyname1"}) + err := runAddCmd(cmd, []string{"keyname1"}) assert.NoError(t, err) /// Test Text - Replace? >> FAIL diff --git a/crypto/keys/codec.go b/crypto/keys/codec.go index f6c1a013d..737836f91 100644 --- a/crypto/keys/codec.go +++ b/crypto/keys/codec.go @@ -1,10 +1,9 @@ package keys import ( - amino "github.com/tendermint/go-amino" + "github.com/cosmos/cosmos-sdk/crypto/keys/hd" + "github.com/tendermint/go-amino" "github.com/tendermint/tendermint/crypto/encoding/amino" - - ccrypto "github.com/cosmos/cosmos-sdk/crypto" ) var cdc = amino.NewCodec() @@ -12,8 +11,7 @@ var cdc = amino.NewCodec() func init() { cryptoAmino.RegisterAmino(cdc) cdc.RegisterInterface((*Info)(nil), nil) - cdc.RegisterConcrete(ccrypto.PrivKeyLedgerSecp256k1{}, - "tendermint/PrivKeyLedgerSecp256k1", nil) + cdc.RegisterConcrete(hd.BIP44Params{}, "crypto/keys/hd/BIP44Params", nil) cdc.RegisterConcrete(localInfo{}, "crypto/keys/localInfo", nil) cdc.RegisterConcrete(ledgerInfo{}, "crypto/keys/ledgerInfo", nil) cdc.RegisterConcrete(offlineInfo{}, "crypto/keys/offlineInfo", nil) diff --git a/crypto/keys/hd/hdpath.go b/crypto/keys/hd/hdpath.go index 050b0a39e..5c1ad0359 100644 --- a/crypto/keys/hd/hdpath.go +++ b/crypto/keys/hd/hdpath.go @@ -33,25 +33,25 @@ const ( // BIP44Params wraps BIP 44 params (5 level BIP 32 path). // To receive a canonical string representation ala -// m / purpose' / coin_type' / account' / change / address_index +// m / purpose' / coinType' / account' / change / addressIndex // call String() on a BIP44Params instance. type BIP44Params struct { - purpose uint32 - coinType uint32 - account uint32 - change bool - addressIdx uint32 + Purpose uint32 `json:"purpose"` + CoinType uint32 `json:"coinType"` + Account uint32 `json:"account"` + Change bool `json:"change"` + AddressIndex uint32 `json:"addressIndex"` } // NewParams creates a BIP 44 parameter object from the params: -// m / purpose' / coin_type' / account' / change / address_index +// m / purpose' / coinType' / account' / change / addressIndex func NewParams(purpose, coinType, account uint32, change bool, addressIdx uint32) *BIP44Params { return &BIP44Params{ - purpose: purpose, - coinType: coinType, - account: account, - change: change, - addressIdx: addressIdx, + Purpose: purpose, + CoinType: coinType, + Account: account, + Change: change, + AddressIndex: addressIdx, } } @@ -105,11 +105,11 @@ func NewParamsFromPath(path string) (*BIP44Params, error) { } return &BIP44Params{ - purpose: purpose, - coinType: coinType, - account: account, - change: change > 0, - addressIdx: addressIdx, + Purpose: purpose, + CoinType: coinType, + Account: account, + Change: change > 0, + AddressIndex: addressIdx, }, nil } @@ -139,32 +139,32 @@ func NewFundraiserParams(account uint32, addressIdx uint32) *BIP44Params { // DerivationPath returns the BIP44 fields as an array. func (p BIP44Params) DerivationPath() []uint32 { change := uint32(0) - if p.change { + if p.Change { change = 1 } return []uint32{ - p.purpose, - p.coinType, - p.account, + p.Purpose, + p.CoinType, + p.Account, change, - p.addressIdx, + p.AddressIndex, } } func (p BIP44Params) String() string { var changeStr string - if p.change { + if p.Change { changeStr = "1" } else { changeStr = "0" } - // m / purpose' / coin_type' / account' / change / address_index + // m / Purpose' / coin_type' / Account' / Change / address_index return fmt.Sprintf("%d'/%d'/%d'/%s/%d", - p.purpose, - p.coinType, - p.account, + p.Purpose, + p.CoinType, + p.Account, changeStr, - p.addressIdx) + p.AddressIndex) } // ComputeMastersFromSeed returns the master public key, master secret, and chain code in hex. diff --git a/crypto/keys/keybase.go b/crypto/keys/keybase.go index 1a7b06d16..5d911ea61 100644 --- a/crypto/keys/keybase.go +++ b/crypto/keys/keybase.go @@ -284,9 +284,9 @@ func (kb dbKeybase) ExportPrivateKeyObject(name string, passphrase string) (tmcr return nil, err } case ledgerInfo: - return nil, errors.New("Only works on local private keys") + return nil, errors.New("only works on local private keys") case offlineInfo: - return nil, errors.New("Only works on local private keys") + return nil, errors.New("only works on local private keys") } return priv, nil } @@ -428,7 +428,8 @@ func (kb dbKeybase) writeOfflineKey(name string, pub tmcrypto.PubKey) Info { func (kb dbKeybase) writeInfo(name string, info Info) { // write the info by key key := infoKey(name) - kb.db.SetSync(key, writeInfo(info)) + serializedInfo := writeInfo(info) + kb.db.SetSync(key, serializedInfo) // store a pointer to the infokey by address for fast lookup kb.db.SetSync(addrKey(info.GetAddress()), key) } diff --git a/crypto/keys/keybase_test.go b/crypto/keys/keybase_test.go index f5c214a37..6ccbcfb2a 100644 --- a/crypto/keys/keybase_test.go +++ b/crypto/keys/keybase_test.go @@ -68,19 +68,34 @@ func TestCreateLedger(t *testing.T) { // test_cover does not compile some dependencies so ledger is disabled // test_unit may add a ledger mock // both cases are acceptable - ledger, err := kb.CreateLedger("some_account", Secp256k1, 0, 1) + ledger, err := kb.CreateLedger("some_account", Secp256k1, 3, 1) if err != nil { assert.Error(t, err) assert.Equal(t, "ledger nano S: support for ledger devices is not available in this executable", err.Error()) assert.Nil(t, ledger) - } else { - // The mock is available, check that the address is correct - pubKey := ledger.GetPubKey() - addr, err := sdk.Bech32ifyAccPub(pubKey) - assert.NoError(t, err) - assert.Equal(t, "cosmospub1addwnpepqfsdqjr68h7wjg5wacksmqaypasnra232fkgu5sxdlnlu8j22ztxvlqvd65", addr) + t.Skip("ledger nano S: support for ledger devices is not available in this executable") + return } + + // The mock is available, check that the address is correct + pubKey := ledger.GetPubKey() + pk, err := sdk.Bech32ifyAccPub(pubKey) + assert.NoError(t, err) + assert.Equal(t, "cosmospub1addwnpepqdszcr95mrqqs8lw099aa9h8h906zmet22pmwe9vquzcgvnm93eqygufdlv", pk) + + // Check that restoring the key gets the same results + restoredKey, err := kb.Get("some_account") + assert.NotNil(t, restoredKey) + assert.Equal(t, "some_account", restoredKey.GetName()) + assert.Equal(t, TypeLedger, restoredKey.GetType()) + pubKey = restoredKey.GetPubKey() + pk, err = sdk.Bech32ifyAccPub(pubKey) + assert.Equal(t, "cosmospub1addwnpepqdszcr95mrqqs8lw099aa9h8h906zmet22pmwe9vquzcgvnm93eqygufdlv", pk) + + linfo := restoredKey.(ledgerInfo) + assert.Equal(t, "44'/118'/3'/0/1", linfo.GetPath().String()) + } // TestKeyManagement makes sure we can manipulate these keys well diff --git a/crypto/keys/types.go b/crypto/keys/types.go index 52ef88b4b..8a4ff7fc2 100644 --- a/crypto/keys/types.go +++ b/crypto/keys/types.go @@ -150,6 +150,10 @@ func (i ledgerInfo) GetAddress() types.AccAddress { return i.PubKey.Address().Bytes() } +func (i ledgerInfo) GetPath() hd.BIP44Params { + return i.Path +} + // offlineInfo is the public information about an offline key type offlineInfo struct { Name string `json:"name"` diff --git a/crypto/keys/types_test.go b/crypto/keys/types_test.go new file mode 100644 index 000000000..621395b0c --- /dev/null +++ b/crypto/keys/types_test.go @@ -0,0 +1,40 @@ +package keys + +import ( + "encoding/hex" + "testing" + + "github.com/cosmos/cosmos-sdk/crypto/keys/hd" + "github.com/cosmos/cosmos-sdk/types" + "github.com/stretchr/testify/assert" + "github.com/tendermint/tendermint/crypto/secp256k1" +) + +func Test_writeReadLedgerInfo(t *testing.T) { + var tmpKey secp256k1.PubKeySecp256k1 + bz, _ := hex.DecodeString("035AD6810A47F073553FF30D2FCC7E0D3B1C0B74B61A1AAA2582344037151E143A") + copy(tmpKey[:], bz) + + lInfo := ledgerInfo{ + "some_name", + tmpKey, + *hd.NewFundraiserParams(5, 1)} + assert.Equal(t, TypeLedger, lInfo.GetType()) + assert.Equal(t, "44'/118'/5'/0/1", lInfo.GetPath().String()) + assert.Equal(t, + "cosmospub1addwnpepqddddqg2glc8x4fl7vxjlnr7p5a3czm5kcdp4239sg6yqdc4rc2r5wmxv8p", + types.MustBech32ifyAccPub(lInfo.GetPubKey())) + + // Serialize and restore + serialized := writeInfo(lInfo) + restoredInfo, err := readInfo(serialized) + assert.NoError(t, err) + assert.NotNil(t, restoredInfo) + + // Check both keys match + assert.Equal(t, lInfo.GetName(), restoredInfo.GetName()) + assert.Equal(t, lInfo.GetType(), restoredInfo.GetType()) + assert.Equal(t, lInfo.GetPubKey(), restoredInfo.GetPubKey()) + + assert.Equal(t, lInfo.GetPath(), restoredInfo.(ledgerInfo).GetPath()) +} diff --git a/crypto/ledger_test.go b/crypto/ledger_test.go index 6418a5732..ea2bc9681 100644 --- a/crypto/ledger_test.go +++ b/crypto/ledger_test.go @@ -79,7 +79,7 @@ func TestPublicKeyHDPath(t *testing.T) { // Store and restore serializedPk := priv.Bytes() require.NotNil(t, serializedPk) - require.Equal(t, 44, len(serializedPk)) + require.True(t, len(serializedPk) >= 50) privKeys[i] = priv }