Convert Ledger secp256k1 privkey to proto.Message #7872

amaury1093 · 2020-11-10T10:52:56Z

Description

Problem

The SDK primarily manages 2 private key types: secp256k1 and ed25519. Both are defined as proto messages.

However, there's a 3rd private key type, PrivKeyLedgerSecp256k1, that has not been converted to a proto message. This creates asymmetries in our cryptotypes.PrivKey interface, namely, should it extend proto.Message?

cosmos-sdk/crypto/ledger/ledger_secp256k1.go

Lines 40 to 48 in 90e9370

    
           // PrivKeyLedgerSecp256k1 implements PrivKey, calling the ledger nano we 
        
           // cache the PubKey from the first call to use it later. 
        
           PrivKeyLedgerSecp256k1 struct { 
        
           	// CachedPubKey should be private, but we want to encode it via 
        
           	// go-amino so we can view the address later, even without having the 
        
           	// ledger attached. 
        
           	CachedPubKey types.PubKey 
        
           	Path         hd.BIP44Params 
        
           }

Solution

I see two solutions:

cryptotypes.PrivKey does not extend proto.Message. There's actually no reason in the SDK that we require private keys to be proto Messages.
Convert PrivKeyLedgerSecp256k1 to a proto.Message.

I personally see having all our private keys defined as proto.Message a better solution. Even if technically a proto.Message is not needed in the SDK for managing private keys, it still gives us the stability guarantees that come with protobuf. This PR implements solution 2. Moreover, it removes dependency of PrivKeyLedgerSecp256k1 on amino.

Let me know your thoughts.

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards.
Wrote unit and integration tests
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added relevant godoc comments.
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer
Review Codecov Report in the comment section below once CI passes

amaury1093 · 2020-11-10T10:54:25Z

proto/cosmos/crypto/ledger/secp256k1/v1beta1/keys.proto

@@ -0,0 +1,17 @@
+syntax = "proto3";
+package cosmos.crypto.ledger.secp256k1.v1beta1;


because of the cached_pub_key field below, I prefer to keep it as beta for now. imo cached_pub_key should be an implementation detail, and not exposed in proto definitions (but at the same time it's super useful).

codecov · 2020-11-16T12:49:39Z

Codecov Report

Merging #7872 (a503648) into master (ed9fc05) will decrease coverage by 0.06%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master    #7872      +/-   ##
==========================================
- Coverage   61.51%   61.45%   -0.07%     
==========================================
  Files         605      607       +2     
  Lines       34721    34779      +58     
==========================================
+ Hits        21358    21372      +14     
- Misses      11140    11183      +43     
- Partials     2223     2224       +1

Impacted Files	Coverage Δ
client/keys/show.go	`86.51% <0.00%> (ø)`
crypto/hd/hdpath.go	`98.46% <ø> (ø)`
crypto/keys/ed25519/ed25519.go	`67.34% <ø> (ø)`
crypto/keys/secp256k1/secp256k1.go	`86.79% <ø> (ø)`
crypto/ledger/ledger_secp256k1.go	`51.57% <66.66%> (-1.11%)`	⬇️
crypto/keyring/keyring.go	`60.49% <100.00%> (ø)`
crypto/codec/tm.go	`0.00% <0.00%> (ø)`
crypto/codec/amino.go	`81.81% <0.00%> (ø)`
crypto/codec/proto.go	`0.00% <0.00%> (ø)`

crypto/ledger/encode_test.go

amaury1093 · 2020-11-16T12:52:20Z

crypto/ledger/ledger_test.go

 	require.Nil(t, err, "%s", err)
 	require.NotNil(t, priv)

-	require.Equal(t, "eb5ae98721034fef9cd7c4c63588d3b03feb5281b9d232cba34d6f3d71aee59211ffbfe1fe87",
-		fmt.Sprintf("%x", cdc.Amino.MustMarshalBinaryBare(priv.PubKey())),
+	require.Equal(t, "034fef9cd7c4c63588d3b03feb5281b9d232cba34d6f3d71aee59211ffbfe1fe87",


switching from amino to proto removes the first 5 bytes.

amaury1093 · 2020-11-16T12:53:10Z

crypto/ledger/ledger_test.go

@@ -82,7 +83,7 @@ func TestPublicKeyUnsafeHDPath(t *testing.T) {
 		// Store and restore
 		serializedPk := priv.Bytes()
 		require.NotNil(t, serializedPk)
-		require.True(t, len(serializedPk) >= 50)
+		require.True(t, len(serializedPk) >= 40)


switching to proto, the serialized private key seems to be shorter. Why did we need 50 before?

Probably a prefix?

crypto/ledger/encode_test.go

tac0turtle · 2020-11-17T09:27:26Z

does this require changes in the ledger app?

amaury1093 · 2020-11-17T09:33:28Z

does this require changes in the ledger app?

Good question. I didn't change the code ledger<->sdk_priv_key, but i did change sdk_priv_key<->serialized output.

Who's the person maintaining the ledger app?

tac0turtle · 2020-11-17T09:34:52Z

does this require changes in the ledger app?

Good question. I didn't change the code ledger<->sdk_priv_key, but i did change sdk_priv_key<->serialized output.

Who's the person maintaining the ledger app?

@jleni Is the person to ask.

…s-sdk into am-7357-ledgerprivkey

amaury1093 · 2020-11-17T09:54:23Z

crypto/codec/amino_test.go

+	amino.PrintTypes(os.Stdout)
+	// Output:
+	// | Type | Name | Prefix | Length | Notes |
+	// | ---- | ---- | ------ | ----- | ------ |
+	// | PubKey | tendermint/PubKeySr25519 | 0x0DFB1005 | variable |  |


Please note that I explicitly removed amino support for PrivKeyLedgerSecp256k1. I cannot see where it's needed, but if it's needed, I can add it back (in the same way we do for other pubkeys).

github-actions · 2021-01-19T00:44:35Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

amaury1093 added 3 commits November 10, 2020 11:30

Generate cached pub key

bac6879

Remove LedgerPrivKey

87bc583

Fix package name

afc212e

amaury1093 added the C:Crypto label Nov 10, 2020

amaury1093 commented Nov 10, 2020

View reviewed changes

amaury1093 added 3 commits November 10, 2020 11:56

Fix proto lint

37d1c25

Remove amino from ledger

03e8f4d

Fix tests

9160670

Merge branch 'master' into am-7357-ledgerprivkey

0054b5e

amaury1093 commented Nov 16, 2020

View reviewed changes

amaury1093 marked this pull request as ready for review November 16, 2020 12:53

amaury1093 requested review from aaronc, alessio and alexanderbez as code owners November 16, 2020 12:53

amaury1093 requested review from blushi and robert-zaremba November 16, 2020 12:53

robert-zaremba requested changes Nov 17, 2020

View reviewed changes

crypto/ledger/encode_test.go Show resolved Hide resolved

crypto/ledger/encode_test.go Show resolved Hide resolved

amaury1093 added 2 commits November 17, 2020 10:50

Add back ExamplePrintRegisteredTypes

13e0c17

Merge branch 'am-7357-ledgerprivkey' of ssh://github.com/cosmos/cosmo…

8c69484

…s-sdk into am-7357-ledgerprivkey

amaury1093 commented Nov 17, 2020

View reviewed changes

Merge master

a503648

amaury1093 added the C:Keys Keybase, KMS and HSMs label Nov 30, 2020

amaury1093 marked this pull request as draft December 4, 2020 09:43

github-actions bot added the stale label Jan 19, 2021

github-actions bot closed this Jan 26, 2021

alessio deleted the am-7357-ledgerprivkey branch March 14, 2021 01:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert Ledger secp256k1 privkey to proto.Message #7872

Convert Ledger secp256k1 privkey to proto.Message #7872

amaury1093 commented Nov 10, 2020 •

edited

Loading

amaury1093 Nov 10, 2020 •

edited

Loading

codecov bot commented Nov 16, 2020 •

edited

Loading

amaury1093 Nov 16, 2020

amaury1093 Nov 16, 2020

robert-zaremba Nov 17, 2020

tac0turtle commented Nov 17, 2020

amaury1093 commented Nov 17, 2020

tac0turtle commented Nov 17, 2020

amaury1093 Nov 17, 2020 •

edited

Loading

github-actions bot commented Jan 19, 2021

	// PrivKeyLedgerSecp256k1 implements PrivKey, calling the ledger nano we
	// cache the PubKey from the first call to use it later.
	PrivKeyLedgerSecp256k1 struct {
	// CachedPubKey should be private, but we want to encode it via
	// go-amino so we can view the address later, even without having the
	// ledger attached.
	CachedPubKey types.PubKey
	Path hd.BIP44Params
	}

		@@ -0,0 +1,17 @@
		syntax = "proto3";
		package cosmos.crypto.ledger.secp256k1.v1beta1;

Convert Ledger secp256k1 privkey to proto.Message #7872

Convert Ledger secp256k1 privkey to proto.Message #7872

Conversation

amaury1093 commented Nov 10, 2020 • edited Loading

Description

Problem

Solution

amaury1093 Nov 10, 2020 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Nov 16, 2020 • edited Loading

Codecov Report

amaury1093 Nov 16, 2020

Choose a reason for hiding this comment

amaury1093 Nov 16, 2020

Choose a reason for hiding this comment

robert-zaremba Nov 17, 2020

Choose a reason for hiding this comment

tac0turtle commented Nov 17, 2020

amaury1093 commented Nov 17, 2020

tac0turtle commented Nov 17, 2020

amaury1093 Nov 17, 2020 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Jan 19, 2021

amaury1093 commented Nov 10, 2020 •

edited

Loading

amaury1093 Nov 10, 2020 •

edited

Loading

codecov bot commented Nov 16, 2020 •

edited

Loading

amaury1093 Nov 17, 2020 •

edited

Loading