Merge PR #3431: PrivKeyLedgerSecp256k1: lost connection

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"
+  revision = "69fdb8ce5e5b9d9c3b22b9248e117b231d4f06dd"
-  version = "v0.9.3"
+  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",

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

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)
 

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

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)
+}

crypto/ledger_secp256k1.go

@@ -2,11 +2,12 @@ package crypto
 
 import (
 	"fmt"
+	"os"
 
-	"github.com/pkg/errors"
+	"github.com/btcsuite/btcd/btcec"
-
-	secp256k1 "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 {
+	device, err := getLedgerDevice()
-		return nil, errors.New("no Ledger discovery function defined")
-	}
-
-	device, err := discoverLedger()
 	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 := getPubKey(device, path)
-
-	pubKey, err := pkl.getPubKey()
 	if err != nil {
 		return nil, err
 	}
 
-	pkl.CachedPubKey = pubKey
+	return PrivKeyLedgerSecp256k1{pubKey, path}, nil
-	return pkl, err
 }
 
 // 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
+	device, err := getLedgerDevice()
-	pub, err := pkl.getPubKey()
 	if err != nil {
 		return err
 	}
+	defer warnIfErrors(device.Close)
 
-	// verify this matches cached address
+	return validateKey(device, pkl)
-	if !pub.Equals(pkl.CachedPubKey) {
-		return fmt.Errorf("cached key does not match retrieved key")
-	}
-
-	return nil
 }
 
 // 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) {
+func sign(device LedgerSECP256K1, pkl PrivKeyLedgerSecp256k1, msg []byte) ([]byte, error) {
-	sig, err := pkl.signLedgerSecp256k1(msg)
+	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) {
+func getPubKey(device LedgerSECP256K1, path hd.BIP44Params) (tmcrypto.PubKey, error) {
-	key, err = pkl.pubkeyLedgerSecp256k1()
+	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
 }

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"
+// This tests assume a ledger is not plugged in
-
+func TestLedgerErrorHandling(t *testing.T) {
-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))
-	}
-
 	// first, try to generate a key, must return an error
 	// (no panic)
-	path := hd.NewParams(44, 60, 0, false, 0)
+	path := *hd.NewParams(44, 555, 0, false, 0)
-	_, err := NewPrivKeyLedgerSecp256k1(*path)
+	_, err := NewPrivKeyLedgerSecp256k1(path)
 	require.Error(t, err)
 }