diff --git a/Gopkg.lock b/Gopkg.lock index 97fcf6734..9711a8447 100644 --- a/Gopkg.lock +++ b/Gopkg.lock @@ -1,22 +1,6 @@ # This file is autogenerated, do not edit; changes may be undone by the next 'dep ensure'. -[[projects]] - digest = "1:7736fc6da04620727f8f3aa2ced8d77be8e074a302820937aa5993848c769b27" - name = "github.com/ZondaX/hid-go" - packages = ["."] - pruneopts = "UT" - revision = "48b08affede2cea076a3cf13b2e3f72ed262b743" - version = "v0.4.0" - -[[projects]] - digest = "1:1ba351898f7efc68c7c9ff3145b920e478f716b077fdaaf06b967c5d883fa988" - name = "github.com/ZondaX/ledger-go" - packages = ["."] - pruneopts = "UT" - revision = "c3225ab10c2f53397d4aa419a588466493572b22" - version = "v0.4.0" - [[projects]] branch = "master" digest = "1:09a7f74eb6bb3c0f14d8926610c87f569c5cff68e978d30e9a3540aeb626fdf0" @@ -522,12 +506,28 @@ revision = "v0.29.0" [[projects]] - digest = "1:29b886f00694ae7c18c4559a2901f2a057d5a62308ed5eb6cd52b9a31016fb14" + digest = "1:b73f5e117bc7c6e8fc47128f20db48a873324ad5cfeeebfc505e85c58682b5e4" + name = "github.com/zondax/hid" + packages = ["."] + pruneopts = "T" + revision = "302fd402163c34626286195dfa9adac758334acc" + version = "v0.9.0" + +[[projects]] + digest = "1:fca24169988a61ea725d1326de30910d8049fe68bcbc194d28803f9a76dda380" name = "github.com/zondax/ledger-cosmos-go" packages = ["."] pruneopts = "UT" - revision = "71aa0ab6e03d2d320c82bbe13678a36584a5b813" - version = "v0.9.3" + revision = "69fdb8ce5e5b9d9c3b22b9248e117b231d4f06dd" + version = "v0.9.7" + +[[projects]] + digest = "1:f8e4c0b959174a1fa5946b12f1f2ac7ea5651bef20a9e4a8dac55dbffcaa6cd6" + name = "github.com/zondax/ledger-go" + packages = ["."] + pruneopts = "UT" + revision = "69c15f1333a9b6866e5f66096561c7d138894bc5" + version = "v0.8.0" [[projects]] digest = "1:6f6dc6060c4e9ba73cf28aa88f12a69a030d3d19d518ef8e931879eaa099628d" @@ -681,6 +681,7 @@ "github.com/stretchr/testify/assert", "github.com/stretchr/testify/require", "github.com/syndtr/goleveldb/leveldb/opt", + "github.com/tendermint/btcd/btcec", "github.com/tendermint/go-amino", "github.com/tendermint/iavl", "github.com/tendermint/tendermint/abci/server", diff --git a/Gopkg.toml b/Gopkg.toml index 1bf115d61..bb3b016c0 100644 --- a/Gopkg.toml +++ b/Gopkg.toml @@ -44,7 +44,7 @@ [[constraint]] name = "github.com/zondax/ledger-cosmos-go" - version = "=v0.9.3" + version = "=v0.9.7" ## deps without releases: @@ -84,3 +84,6 @@ [prune] go-tests = true unused-packages = true + [[prune.project]] + name = "github.com/zondax/hid" + unused-packages = false diff --git a/Makefile b/Makefile index d583e0c76..0b2c2c76b 100644 --- a/Makefile +++ b/Makefile @@ -144,6 +144,9 @@ test: test_unit test_cli: @go test -p 4 `go list github.com/cosmos/cosmos-sdk/cmd/gaia/cli_test` -tags=cli_test +test_ledger: + @go test `go list github.com/cosmos/cosmos-sdk/crypto` -tags='cgo ledger test_real_ledger' + test_unit: @VERSION=$(VERSION) go test $(PACKAGES_NOSIMULATION) diff --git a/PENDING.md b/PENDING.md index 109e18ab5..296d30d1c 100644 --- a/PENDING.md +++ b/PENDING.md @@ -90,6 +90,7 @@ BUG FIXES - [\#3419](https://github.com/cosmos/cosmos-sdk/pull/3419) Fix `q distr slashes` panic - [\#3453](https://github.com/cosmos/cosmos-sdk/pull/3453) The `rest-server` command didn't respect persistent flags such as `--chain-id` and `--trust-node` if they were passed on the command line. + - [\#3441](https://github.com/cosmos/cosmos-sdk/pull/3431) Improved resource management and connection handling (ledger devices). Fixes issue with DER vs BER signatures. * Gaia * [\#3486](https://github.com/cosmos/cosmos-sdk/pull/3486) Use AmountOf in diff --git a/crypto/ledger_integration_test.go b/crypto/ledger_integration_test.go new file mode 100644 index 000000000..30c66c765 --- /dev/null +++ b/crypto/ledger_integration_test.go @@ -0,0 +1,157 @@ +// +build cgo,ledger,test_real_ledger + +package crypto + +import ( + "fmt" + "testing" + + "github.com/cosmos/cosmos-sdk/crypto/keys/hd" + sdk "github.com/cosmos/cosmos-sdk/types" + "github.com/stretchr/testify/require" + "github.com/tendermint/tendermint/crypto/encoding/amino" + ledger "github.com/zondax/ledger-cosmos-go" +) + +const ( + // These tests expect a ledger device initialized to the following mnemonic + testMnemonic = "equip will roof matter pink blind book anxiety banner elbow sun young" +) + +func TestDiscoverDevice(t *testing.T) { + device, err := discoverLedger() + require.NoError(t, err) + require.NotNil(t, device) + defer device.Close() +} + +func TestDiscoverDeviceTwice(t *testing.T) { + // We expect the second call not to find a device + device, err := discoverLedger() + require.NoError(t, err) + require.NotNil(t, device) + defer device.Close() + + device2, err := discoverLedger() + require.Error(t, err) + require.Equal(t, "no ledger connected", err.Error()) + require.Nil(t, device2) +} + +func TestDiscoverDeviceTwiceClosing(t *testing.T) { + { + device, err := ledger.FindLedgerCosmosUserApp() + require.NoError(t, err) + require.NotNil(t, device) + require.NoError(t, device.Close()) + } + + device2, err := discoverLedger() + require.NoError(t, err) + require.NotNil(t, device2) + require.NoError(t, device2.Close()) +} + +func TestPublicKey(t *testing.T) { + path := *hd.NewFundraiserParams(0, 0) + priv, err := NewPrivKeyLedgerSecp256k1(path) + require.Nil(t, err, "%s", err) + require.NotNil(t, priv) + + pubKeyAddr, err := sdk.Bech32ifyAccPub(priv.PubKey()) + require.NoError(t, err) + require.Equal(t, "cosmospub1addwnpepqd87l8xhcnrrtzxnkql7k55ph8fr9jarf4hn6udwukfprlalu8lgw0urza0", pubKeyAddr, "Is your device using test mnemonic: %s ?", testMnemonic) + require.Equal(t, "5075624b6579536563703235366b317b303334464546394344374334433633353838443342303"+ + "3464542353238314239443233324342413334443646334437314145453539323131464642464531464538377d", + fmt.Sprintf("%x", priv.PubKey())) +} + +func TestPublicKeyHDPath(t *testing.T) { + expectedAnswers := []string{ + "cosmospub1addwnpepqd87l8xhcnrrtzxnkql7k55ph8fr9jarf4hn6udwukfprlalu8lgw0urza0", + "cosmospub1addwnpepqfsdqjr68h7wjg5wacksmqaypasnra232fkgu5sxdlnlu8j22ztxvlqvd65", + "cosmospub1addwnpepqw3xwqun6q43vtgw6p4qspq7srvxhcmvq4jrx5j5ma6xy3r7k6dtxmrkh3d", + "cosmospub1addwnpepqvez9lrp09g8w7gkv42y4yr5p6826cu28ydrhrujv862yf4njmqyyjr4pjs", + "cosmospub1addwnpepq06hw3enfrtmq8n67teytcmtnrgcr0yntmyt25kdukfjkerdc7lqg32rcz7", + "cosmospub1addwnpepqg3trf2gd0s2940nckrxherwqhgmm6xd5h4pcnrh4x7y35h6yafmcpk5qns", + "cosmospub1addwnpepqdm6rjpx6wsref8wjn7ym6ntejet430j4szpngfgc20caz83lu545vuv8hp", + "cosmospub1addwnpepqvdhtjzy2wf44dm03jxsketxc07vzqwvt3vawqqtljgsr9s7jvydjmt66ew", + "cosmospub1addwnpepqwystfpyxwcava7v3t7ndps5xzu6s553wxcxzmmnxevlzvwrlqpzz695nw9", + "cosmospub1addwnpepqw970u6gjqkccg9u3rfj99857wupj2z9fqfzy2w7e5dd7xn7kzzgkgqch0r", + } + + // Check with device + for i := uint32(0); i < 10; i++ { + path := *hd.NewFundraiserParams(0, i) + fmt.Printf("Checking keys at %v\n", path) + + priv, err := NewPrivKeyLedgerSecp256k1(path) + require.Nil(t, err, "%s", err) + require.NotNil(t, priv) + + pubKeyAddr, err := sdk.Bech32ifyAccPub(priv.PubKey()) + require.NoError(t, err) + require.Equal(t, expectedAnswers[i], pubKeyAddr, "Is your device using test mnemonic: %s ?", testMnemonic) + } +} + +func getFakeTx(accountNumber uint32) []byte { + tmp := fmt.Sprintf( + `{"account_number":"%d","chain_id":"1234","fee":{"amount":[{"amount":"150","denom":"atom"}],"gas":"5000"},"memo":"memo","msgs":[[""]],"sequence":"6"}`, + accountNumber) + + return []byte(tmp) +} + +func TestSignaturesHD(t *testing.T) { + for account := uint32(0); account < 100; account += 30 { + msg := getFakeTx(account) + + path := *hd.NewFundraiserParams(account, account/5) + fmt.Printf("Checking signature at %v\n", path) + + priv, err := NewPrivKeyLedgerSecp256k1(path) + require.Nil(t, err, "%s", err) + + pub := priv.PubKey() + sig, err := priv.Sign(msg) + require.Nil(t, err) + + valid := pub.VerifyBytes(msg, sig) + require.True(t, valid, "Is your device using test mnemonic: %s ?", testMnemonic) + } +} + +func TestRealLedgerSecp256k1(t *testing.T) { + msg := getFakeTx(50) + path := *hd.NewFundraiserParams(0, 0) + priv, err := NewPrivKeyLedgerSecp256k1(path) + require.Nil(t, err, "%s", err) + + pub := priv.PubKey() + sig, err := priv.Sign(msg) + require.Nil(t, err) + + valid := pub.VerifyBytes(msg, sig) + require.True(t, valid) + + // now, let's serialize the public key and make sure it still works + bs := priv.PubKey().Bytes() + pub2, err := cryptoAmino.PubKeyFromBytes(bs) + require.Nil(t, err, "%+v", err) + + // make sure we get the same pubkey when we load from disk + require.Equal(t, pub, pub2) + + // signing with the loaded key should match the original pubkey + sig, err = priv.Sign(msg) + require.Nil(t, err) + valid = pub.VerifyBytes(msg, sig) + require.True(t, valid) + + // make sure pubkeys serialize properly as well + bs = pub.Bytes() + bpub, err := cryptoAmino.PubKeyFromBytes(bs) + require.NoError(t, err) + require.Equal(t, pub, bpub) +} diff --git a/crypto/ledger_secp256k1.go b/crypto/ledger_secp256k1.go index 8f5c9ab81..5be8405f5 100644 --- a/crypto/ledger_secp256k1.go +++ b/crypto/ledger_secp256k1.go @@ -2,11 +2,12 @@ package crypto import ( "fmt" + "os" - "github.com/pkg/errors" - - secp256k1 "github.com/btcsuite/btcd/btcec" + "github.com/btcsuite/btcd/btcec" "github.com/cosmos/cosmos-sdk/crypto/keys/hd" + "github.com/pkg/errors" + tmbtcec "github.com/tendermint/btcd/btcec" tmcrypto "github.com/tendermint/tendermint/crypto" tmsecp256k1 "github.com/tendermint/tendermint/crypto/secp256k1" ) @@ -26,6 +27,7 @@ type ( // LedgerSECP256K1 reflects an interface a Ledger API must implement for // the SECP256K1 scheme. LedgerSECP256K1 interface { + Close() error GetPublicKeySECP256K1([]uint32) ([]byte, error) SignSECP256K1([]uint32, []byte) ([]byte, error) } @@ -38,7 +40,6 @@ type ( // ledger attached. CachedPubKey tmcrypto.PubKey Path hd.BIP44Params - ledger LedgerSECP256K1 } ) @@ -48,24 +49,18 @@ type ( // CONTRACT: The ledger device, ledgerDevice, must be loaded and set prior to // any creation of a PrivKeyLedgerSecp256k1. func NewPrivKeyLedgerSecp256k1(path hd.BIP44Params) (tmcrypto.PrivKey, error) { - if discoverLedger == nil { - return nil, errors.New("no Ledger discovery function defined") - } - - device, err := discoverLedger() + device, err := getLedgerDevice() if err != nil { - return nil, errors.Wrap(err, "failed to create PrivKeyLedgerSecp256k1") + return nil, err } + defer warnIfErrors(device.Close) - pkl := &PrivKeyLedgerSecp256k1{Path: path, ledger: device} - - pubKey, err := pkl.getPubKey() + pubKey, err := getPubKey(device, path) if err != nil { return nil, err } - pkl.CachedPubKey = pubKey - return pkl, err + return PrivKeyLedgerSecp256k1{pubKey, path}, nil } // PubKey returns the cached public key. @@ -73,21 +68,27 @@ func (pkl PrivKeyLedgerSecp256k1) PubKey() tmcrypto.PubKey { return pkl.CachedPubKey } +// Sign returns a secp256k1 signature for the corresponding message +func (pkl PrivKeyLedgerSecp256k1) Sign(message []byte) ([]byte, error) { + device, err := getLedgerDevice() + if err != nil { + return nil, err + } + defer warnIfErrors(device.Close) + + return sign(device, pkl, message) +} + // ValidateKey allows us to verify the sanity of a public key after loading it // from disk. func (pkl PrivKeyLedgerSecp256k1) ValidateKey() error { - // getPubKey will return an error if the ledger is not - pub, err := pkl.getPubKey() + device, err := getLedgerDevice() if err != nil { return err } + defer warnIfErrors(device.Close) - // verify this matches cached address - if !pub.Equals(pkl.CachedPubKey) { - return fmt.Errorf("cached key does not match retrieved key") - } - - return nil + return validateKey(device, pkl) } // AssertIsPrivKeyInner implements the PrivKey interface. It performs a no-op. @@ -105,54 +106,89 @@ func (pkl PrivKeyLedgerSecp256k1) Equals(other tmcrypto.PrivKey) bool { if ledger, ok := other.(*PrivKeyLedgerSecp256k1); ok { return pkl.CachedPubKey.Equals(ledger.CachedPubKey) } - return false } +// warnIfErrors wraps a function and writes a warning to stderr. This is required +// to avoid ignoring errors when defer is used. Using defer may result in linter warnings. +func warnIfErrors(f func() error) { + if err := f(); err != nil { + _, _ = fmt.Fprint(os.Stderr, "received error when closing ledger connection", err) + } +} + +func convertDERtoBER(signatureDER []byte) ([]byte, error) { + sigDER, err := btcec.ParseDERSignature(signatureDER[:], btcec.S256()) + if err != nil { + return nil, err + } + sigBER := tmbtcec.Signature{R: sigDER.R, S: sigDER.S} + return sigBER.Serialize(), nil +} + +func getLedgerDevice() (LedgerSECP256K1, error) { + if discoverLedger == nil { + return nil, errors.New("no Ledger discovery function defined") + } + + device, err := discoverLedger() + if err != nil { + return nil, errors.Wrap(err, "ledger nano S") + } + + return device, nil +} + +func validateKey(device LedgerSECP256K1, pkl PrivKeyLedgerSecp256k1) error { + pub, err := getPubKey(device, pkl.Path) + if err != nil { + return err + } + + // verify this matches cached address + if !pub.Equals(pkl.CachedPubKey) { + return fmt.Errorf("cached key does not match retrieved key") + } + + return nil +} + // Sign calls the ledger and stores the PubKey for future use. // // Communication is checked on NewPrivKeyLedger and PrivKeyFromBytes, returning // an error, so this should only trigger if the private key is held in memory // for a while before use. -func (pkl PrivKeyLedgerSecp256k1) Sign(msg []byte) ([]byte, error) { - sig, err := pkl.signLedgerSecp256k1(msg) +func sign(device LedgerSECP256K1, pkl PrivKeyLedgerSecp256k1, msg []byte) ([]byte, error) { + err := validateKey(device, pkl) if err != nil { return nil, err } - return sig, nil + sig, err := device.SignSECP256K1(pkl.Path.DerivationPath(), msg) + if err != nil { + return nil, err + } + + return convertDERtoBER(sig) } // getPubKey reads the pubkey the ledger itself // since this involves IO, it may return an error, which is not exposed // in the PubKey interface, so this function allows better error handling -func (pkl PrivKeyLedgerSecp256k1) getPubKey() (key tmcrypto.PubKey, err error) { - key, err = pkl.pubkeyLedgerSecp256k1() +func getPubKey(device LedgerSECP256K1, path hd.BIP44Params) (tmcrypto.PubKey, error) { + publicKey, err := device.GetPublicKeySECP256K1(path.DerivationPath()) if err != nil { - return key, fmt.Errorf("please open Cosmos app on the Ledger device - error: %v", err) + return nil, fmt.Errorf("please open Cosmos app on the Ledger device - error: %v", err) } - return key, err -} - -func (pkl PrivKeyLedgerSecp256k1) signLedgerSecp256k1(msg []byte) ([]byte, error) { - return pkl.ledger.SignSECP256K1(pkl.Path.DerivationPath(), msg) -} - -func (pkl PrivKeyLedgerSecp256k1) pubkeyLedgerSecp256k1() (pub tmcrypto.PubKey, err error) { - key, err := pkl.ledger.GetPublicKeySECP256K1(pkl.Path.DerivationPath()) - if err != nil { - return nil, fmt.Errorf("error fetching public key: %v", err) - } - - var pk tmsecp256k1.PubKeySecp256k1 - // re-serialize in the 33-byte compressed format - cmp, err := secp256k1.ParsePubKey(key[:], secp256k1.S256()) + cmp, err := btcec.ParsePubKey(publicKey[:], btcec.S256()) if err != nil { return nil, fmt.Errorf("error parsing public key: %v", err) } - copy(pk[:], cmp.SerializeCompressed()) - return pk, nil + var compressedPublicKey tmsecp256k1.PubKeySecp256k1 + copy(compressedPublicKey[:], cmp.SerializeCompressed()) + + return compressedPublicKey, nil } diff --git a/crypto/ledger_test.go b/crypto/ledger_test.go index cf14db74b..cb2c7b325 100644 --- a/crypto/ledger_test.go +++ b/crypto/ledger_test.go @@ -1,66 +1,17 @@ package crypto import ( - "fmt" - "os" "testing" "github.com/cosmos/cosmos-sdk/crypto/keys/hd" - "github.com/stretchr/testify/require" - "github.com/tendermint/tendermint/crypto/encoding/amino" ) -var ledgerEnabledEnv = "TEST_WITH_LEDGER" - -func TestRealLedgerSecp256k1(t *testing.T) { - if os.Getenv(ledgerEnabledEnv) == "" { - t.Skip(fmt.Sprintf("Set '%s' to run code on a real ledger", ledgerEnabledEnv)) - } - msg := []byte("{\"account_number\":\"3\",\"chain_id\":\"1234\",\"fee\":{\"amount\":[{\"amount\":\"150\",\"denom\":\"atom\"}],\"gas\":\"5000\"},\"memo\":\"memo\",\"msgs\":[[\"%s\"]],\"sequence\":\"6\"}") - path := hd.NewParams(44, 60, 0, false, 0) - - priv, err := NewPrivKeyLedgerSecp256k1(*path) - require.Nil(t, err, "%s", err) - - pub := priv.PubKey() - sig, err := priv.Sign(msg) - require.Nil(t, err) - - valid := pub.VerifyBytes(msg, sig) - require.True(t, valid) - - // now, let's serialize the public key and make sure it still works - bs := priv.PubKey().Bytes() - pub2, err := cryptoAmino.PubKeyFromBytes(bs) - require.Nil(t, err, "%+v", err) - - // make sure we get the same pubkey when we load from disk - require.Equal(t, pub, pub2) - - // signing with the loaded key should match the original pubkey - sig, err = priv.Sign(msg) - require.Nil(t, err) - valid = pub.VerifyBytes(msg, sig) - require.True(t, valid) - - // make sure pubkeys serialize properly as well - bs = pub.Bytes() - bpub, err := cryptoAmino.PubKeyFromBytes(bs) - require.NoError(t, err) - require.Equal(t, pub, bpub) -} - -// TestRealLedgerErrorHandling calls. These tests assume -// the ledger is not plugged in.... -func TestRealLedgerErrorHandling(t *testing.T) { - if os.Getenv(ledgerEnabledEnv) != "" { - t.Skip(fmt.Sprintf("Unset '%s' to run code as if without a real Ledger", ledgerEnabledEnv)) - } - +// This tests assume a ledger is not plugged in +func TestLedgerErrorHandling(t *testing.T) { // first, try to generate a key, must return an error // (no panic) - path := hd.NewParams(44, 60, 0, false, 0) - _, err := NewPrivKeyLedgerSecp256k1(*path) + path := *hd.NewParams(44, 555, 0, false, 0) + _, err := NewPrivKeyLedgerSecp256k1(path) require.Error(t, err) }