Unify local and external keys in keybase interface #117

cwgoes · 2018-06-05T03:04:43Z

Rework PrivKey, Keybase, and Info to work with both locally stored and external keys (offline, HW wallets, etc).

This involves the following API changes:

Return errors on privkey.Sign() and privkey.PubKey(), as we may be querying an external datastore
Add methods to the Keybase interface to track Ledger HD ed25519 keys and offline keys, so that signing by name automatically looks up the right key and prompts the user for a password / communicates with the Ledger / prompts the user to sign offline, as appropriate.

cwgoes · 2018-06-07T00:08:57Z

To-do:

Add way to track a watch-only key by pubkey
Correctly switch on .Sign() based on keytype
Add testcases
Test downstream in Cosmos SDK

cwgoes · 2018-06-07T18:29:17Z

@liamsi Thoughts on the API changes?

liamsi · 2018-06-08T00:46:53Z

The interface changes look good to me! Totally in favour of sign returning an error. I also like changing Info to an interface. I might change the Create to CreateMneomnicKey or simply CreateMnemonic (as the key is clear because of the context/package name/iface name) in #118

liamsi · 2018-06-08T21:00:43Z

keys/keybase.go

+	case ledgerInfo:
+	case offlineInfo:
+		if passphrase != "yes" {
+			return fmt.Errorf("Enter exactly 'yes' to delete the key")


nitpick: Error messages shouldn be capitalized. Ideally, run golint before finalizing the PR.

Addressed. Agreed on golint, unfortunately this repo is very noncompliant at the moment...

Yeah, you can run go-lint only on the files you've modified. That would be great.. Otherwise, don't bother. I'll delete so much code in #116 ... after that nothing is left and it's secure and very compliant ;-)

liamsi

LGTM, left some nitpicks in comments

liamsi · 2018-06-09T06:42:31Z

encode_test.go

 		checkAminoBinary(t, sig1, &sig2, -1) // Siganture size changes for Secp anyways.
 		assert.EqualValues(t, sig1, sig2)
 		checkAminoJSON(t, sig1, &sig3, false) // TODO also check Prefix bytes.
 		assert.EqualValues(t, sig1, sig3)

 		// Check (de/en)codings of PubKeys.
-		pubKey := tc.privKey.PubKey()
+		pubKey, err := tc.privKey.PubKey()
+		assert.Nil(t, err)


Can we move this newly introduced asserts to assert.NoError? It slightly increases the readability of the tests.

All instances changed to assert.NoError.

liamsi · 2018-06-09T06:52:53Z

keys/keybase.go

+		linfo := info.(offlineInfo)
+		fmt.Printf("Bytes to sign:\n%s", msg)
+		buf := bufio.NewReader(os.Stdin)
+		fmt.Printf("\nEnter encoded signature:\n")


Be more specific: encoded in what?

Amino-encoded signature (updated), for now at least.

liamsi · 2018-06-09T06:55:13Z

keys/keybase.go

+		if err != nil {
+			return nil, nil, err
+		}
+		cdc.MustUnmarshalBinary([]byte(signed), sig)


Can you add a comment, that 1) this will block until the user inputs the signature and 2) this assumes that the offline computer is aware of the structure to be signed (TXs?) and properly encodes it in amino. Are we writing the offline-signing software, too? Otherwise, it might be reasonable to assume some other more common encoding. Thoughts?

I agree that requiring Amino is suboptimal, let's track that separately though - #121.

Added a comment about blocking.

Great, thanks! I'll join the discussion in #121 👍

liamsi · 2018-06-09T06:58:37Z

keys/keybase.go

+	case ledgerInfo:
+	case offlineInfo:
+		if passphrase != "yes" {
+			return fmt.Errorf("enter exactly 'yes' to delete the key")


Is 'yes' always a user entered value? Otherwise: "call with seedphrase 'yes' to delete the key".

In the only downstream consumer I know of, it is user-entered. Leaving for now, if in the future we have other consumers we should probably just change this API anyways.

Can this support y also? (Alot of people are probably in the habit of this)

Most bash commands don't delete keys (even just public keys, as in this case) though. IMO better to require that the user enter exactly 'yes'.

liamsi · 2018-06-09T07:02:05Z

keys/keybase.go

 }

-func (kb dbKeybase) writePubKey(pub crypto.PubKey, name string) Info {
+func (kb dbKeybase) writeLocalKey(priv crypto.PrivKey, name, passphrase string) Info {
+	// generate the encrypted privkey


Does this actually generate anything? Proposal:
// encrypt private key using passphrase

Agreed, reworded file so "generate" is only used for actually generating keys.

liamsi · 2018-06-09T07:07:36Z

priv_key.go

-		panic(err)
-	}
-	return bz
+	return cdc.MustMarshalBinaryBare(privKey)


Hmm, unrelated to your code but why do we amino encode these bytes?

Addressed.

liamsi

LGTM

Work-in-progress, doesn't compile yet

ef6ff41

cwgoes requested review from ebuchman and jaekwon as code owners June 5, 2018 03:04

cwgoes mentioned this pull request Jun 5, 2018

Ledger integration cosmos/cosmos-sdk#931

Merged

6 tasks

cwgoes added 3 commits June 6, 2018 05:55

Fix testsuite

0e7fd34

Add offlineInfo representation type

1b8a99b

Return errors on priv.Sign(), priv.PubKey()

03fe129

cwgoes added 3 commits June 7, 2018 03:14

Add CreateLedger, CreateOffline

eabd703

Add switch on .Sign() for Ledger wallets

3bec45c

Add offline signing switch on .Sign()

1d9165b

cwgoes added 2 commits June 7, 2018 20:32

Use MustUnmarshalBinaryBare()

77c827a

Offline key creation/deletion testcase

789ef58

cwgoes mentioned this pull request Jun 7, 2018

Improve Ledger error handling, change PrivKey API #114

Closed

cwgoes self-assigned this Jun 8, 2018

cwgoes added 4 commits June 8, 2018 21:31

Rename Create() => CreateMnemonic()

275e51a

Add confirmation to delete offline/Ledger keys

b2fa7fe

Consolidate writePubKey() => writeOfflineKey()

4d6f98d

Forward compatiblity with Ledger ed25519 support

c7ea3c7

cwgoes force-pushed the cwgoes/unified-ledger-keybase branch 2 times, most recently from d245bae to 84bff8e Compare June 8, 2018 20:18

Update upstream versions

60827af

cwgoes force-pushed the cwgoes/unified-ledger-keybase branch from 84bff8e to 60827af Compare June 8, 2018 20:49

liamsi reviewed Jun 8, 2018

View reviewed changes

cwgoes added 2 commits June 8, 2018 23:05

Lowercase error message

8e77665

Update upstream versions

94d6b8d

cwgoes force-pushed the cwgoes/unified-ledger-keybase branch from 59609f8 to 94d6b8d Compare June 8, 2018 22:16

Non-bare marshal/unmarshal

8688dd2

cwgoes added 3 commits June 9, 2018 04:26

Add human-readable .GetType() function to Info interface

09ecc8f

Add newlines to offlineInfo.Sign()

6ff3e45

Pin to exact upstream version

0b8d74c

cwgoes changed the title ~~WIP: Unify local and external keys in keybase interface~~ Unify local and external keys in keybase interface Jun 9, 2018

Rename CryptoAlgo => SignAlgo

b40a660

liamsi previously requested changes Jun 9, 2018

View reviewed changes

cwgoes added 4 commits June 10, 2018 04:13

assert.Nil(t, err) => assert.NoError(t, err)

abe273d

Specify required encoding

171bdbf

Add clarifying comment

77f96b1

Reworded for clarity

a83c72f

liamsi approved these changes Jun 10, 2018

View reviewed changes

liamsi merged commit c21f67c into develop Jun 10, 2018

cwgoes deleted the cwgoes/unified-ledger-keybase branch June 10, 2018 08:09

cwgoes mentioned this pull request Jun 11, 2018

Change upstream go-crypto PrivKey API and update Amino version to 0.10.0 tendermint/tendermint#1721

Closed

liamsi mentioned this pull request Jun 12, 2018

Don't Panic! #69

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unify local and external keys in keybase interface #117

Unify local and external keys in keybase interface #117

cwgoes commented Jun 5, 2018 •

edited

Loading

cwgoes commented Jun 7, 2018 •

edited

Loading

cwgoes commented Jun 7, 2018

liamsi commented Jun 8, 2018

liamsi Jun 8, 2018

cwgoes Jun 8, 2018

liamsi Jun 8, 2018

liamsi left a comment

liamsi Jun 9, 2018

cwgoes Jun 10, 2018

liamsi Jun 9, 2018

cwgoes Jun 10, 2018

liamsi Jun 9, 2018

cwgoes Jun 10, 2018

liamsi Jun 10, 2018

liamsi Jun 9, 2018

cwgoes Jun 10, 2018

ValarDragon Jun 10, 2018 •

edited

Loading

cwgoes Jun 10, 2018

liamsi Jun 9, 2018

cwgoes Jun 10, 2018

liamsi Jun 9, 2018

cwgoes Jun 10, 2018

liamsi left a comment

Unify local and external keys in keybase interface #117

Unify local and external keys in keybase interface #117

Conversation

cwgoes commented Jun 5, 2018 • edited Loading

cwgoes commented Jun 7, 2018 • edited Loading

cwgoes commented Jun 7, 2018

liamsi commented Jun 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamsi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ValarDragon Jun 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamsi left a comment

Choose a reason for hiding this comment

cwgoes commented Jun 5, 2018 •

edited

Loading

cwgoes commented Jun 7, 2018 •

edited

Loading

ValarDragon Jun 10, 2018 •

edited

Loading