R4R: Improve crypto/keys and add `keys mnemonic` and `keys new` commands #2090

ebuchman · 2018-08-20T00:36:51Z

Ref: #2089 and #2091

Various minor cleanup (comments, move mintkey into its own pkg)
Remove bip39 pkg in favour of a fork of the upstream
Remove bcrypt pkg in favour of fork of x/crypto (Change ed25519 and bcrypt to be forked off golang/x/crypto tendermint/tendermint#1959)
Add BIP39 passphrase to KeyBase.Derive
Add gaiacli keys mnemonic and gaiacli keys new commands

Tagging @cwgoes @liamsi so they're aware

Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
Updated all relevant documentation (docs/)
Updated all relevant code comments
Wrote tests
Added entries in PENDING.md that include links to the relevant issue or PR that most accurately describes the change.
Updated cmd/gaia and examples/

For Admin Use:

Added appropriate labels to PR (ex. wip, ready-for-review, docs)
Reviewers Assigned
Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

ebuchman · 2018-08-20T00:39:19Z

some relevant background: tendermint/go-crypto#89 - it looks like the issue has been fixed both in tyler-smith and bartekn. tyler-smith repo is quite active again so we took the commit with the fix before all the new activity.

codecov · 2018-08-20T03:12:38Z

Codecov Report

Merging #2090 into develop will decrease coverage by 2.23%.
The diff coverage is 71.25%.

@@            Coverage Diff             @@
##           develop   #2090      +/-   ##
==========================================
- Coverage    60.13%   57.9%   -2.24%     
==========================================
  Files          152     140      -12     
  Lines         8870    8119     -751     
==========================================
- Hits          5334    4701     -633     
+ Misses        3167    3121      -46     
+ Partials       369     297      -72

ebuchman · 2018-08-20T03:43:07Z

client/keys/new.go

+		}
+
+		algo := keys.Secp256k1               // SigningAlgo(viper.GetString(flagType))
+		path := bip44Params.DerivationPath() // ccrypto.DerivationPath{44, 118, account, 0, index}


Does the ledger enforce BIP44 (ie. it knows the first three fields are hardened) or do we need to pass in hardened values here ?

The go client will harden first three bytes
https://github.com/ZondaX/ledger-goclient/blob/4296ee5701e945f9b3a7dbe51f402e0b9be57259/ledger.go#L245-L248

* add tendermint fork of golang.org/x/crypto * pin some transitive deps

* remove in favour of fork of golang.org/x/crypto/bcrypt at github.com/tendermint/crypto/bcrypt

* use fork cosmos/go-bip39 instead

jaekwon · 2018-08-20T18:30:21Z

It isn't clear in the client what the mnemonic password is vs the encryption password.
Also, we should probably have the user enter these passwords twice to verify...

Kinda think we should just not expose the mnemonic password for now... The KeyBase doesn't support it besides via the Derive function...

mslipper

Will PR in my work now.

mslipper · 2018-08-30T22:46:38Z

client/keys/new.go

+
+	// if we're using ledger, only thing we need is the path.
+	// generate key and we're done.
+	if viper.GetBool(client.FlagUseLedger) {


Could be part of the prompt flow, no? I.e., Do you want to use Ledger [y/n]?

Could be, but since its just boolean (and not really sensitive info) seems fine to leave as a flag

mslipper · 2018-08-30T22:48:07Z

client/keys/new.go

+	// get the mnemonic
+	var mnemonic string
+	if !useDefaults {
+		fmt.Println("> Enter your bip39 mnemonic.")


For all these prompts, I made a helper method in the client package in my PR you can use to prompt the user - client.GetString(). I'll merge that into this branch.

mslipper · 2018-08-30T22:49:56Z

client/keys/mnemonic.go

+			return err
+		}
+		fmt.Println("> Input length:", len(inputEntropy))
+		if len(inputEntropy) < 99 {


Do we want to require this amount of entropy from the user rather than warning them if the provided entropy is weak?

Pull in changes from my PR

rigelrozanski · 2018-09-25T18:20:09Z

sup with this PR is it WIP or R4R? seems inactive

mslipper · 2018-09-25T19:57:14Z

Should be R4R as far as I know.

rigelrozanski

Cool, I fixed the lint here are some superficial comments

client/keys/new.go

rigelrozanski · 2018-09-25T21:06:21Z

crypto/keys/hd/hdpath.go

+	if isHardened(spl[3]) || isHardened(spl[4]) {
+		return nil,
+			fmt.Errorf("fourth and fifth field in path must not be hardened (ie. not contain the suffix ', got %v and %v", spl[3], spl[4])
+	}


all these if statements could be lumped into a switch statement - make it a bit more concise

How's that?

Having all those if statements lined up (which have returns, so they effectively act as chained if-else statements) act identically as a switch statement - I like compactness where it's appropriate but doesn't super matter here

Can you show me what the switch statement would look like? Each of these conditions is on a different piece of the slice, so not clear to me what we could switch on

crypto/keys/mintkey/mintkey.go

client/keys/mnemonic.go

client/input.go

alessio · 2018-09-26T09:44:00Z

client/input.go

+// GetString simply returns the trimmed string output of a given reader.
+func GetString(prompt string, buf *bufio.Reader) (string, error) {
+	if inputIsTty() && prompt != "" {
+		fmt.Println(prompt)


Please redirect the prompt to stderr: fmt.Fprint(os.Stderr, prompt)

alessio · 2018-09-26T09:45:52Z

client/keys/new.go

+// nolint: gocyclo
+func runNewCmd(cmd *cobra.Command, args []string) error {
+
+	if len(args) != 1 || len(args[0]) == 0 {


I'd use cobra.ExactArgs(1)

fedekunze · 2018-10-16T16:15:59Z

From @NodeGuy #1442 (comment):

There's a bug in the current design:
POST /keys a new key with name Chris.
GET /keys/Chris returns the new key.
POST /keys a new key with name seed.
GET /keys/seed does not return the new key, violating the REST abstraction of keys as a collection.
This confused a contract test I wrote and I had to add extra code to work around the bug.

rigelrozanski · 2018-10-16T20:19:10Z

Lol bucky you're getting slashed for leaving this PR open (and almost ready to merge) for 2 months

ebuchman · 2018-10-17T03:02:36Z

Lol bucky you're getting slashed for leaving this PR open (and almost ready to merge) for 2 months

I had till midnight tonight and I totally made it. All comments addressed. Tests fail I think due to unrelated non-determinism ...

ebuchman · 2018-10-17T03:02:53Z

From @NodeGuy #1442 (comment):

There's a bug in the current design:
POST /keys a new key with name Chris.
GET /keys/Chris returns the new key.
POST /keys a new key with name seed.
GET /keys/seed does not return the new key, violating the REST abstraction of keys as a collection.
This confused a contract test I wrote and I had to add extra code to work around the bug.

@fedekunze sounds like this should be a new issue?

ebuchman · 2018-10-17T03:03:14Z

Sorry for the massive PR and for letting it hang so long guys. Thanks for all the nice review.

rigelrozanski · 2018-10-17T05:04:43Z

Yeah test_cover is failing due to lcd non-deterministic test (lcd_test.go:577)

CI:lint failing looks like a go-dep issue which should be addressed b4 merge

Gopkg.lock is out of sync with imports and/or Gopkg.toml. Run `dep check` for details.
input-digest mismatch
Makefile:193: recipe for target 'test_lint' failed

rigelrozanski

looks good - couple linting issues still need to pass before merge - but all my concerns addressed

melekes · 2018-10-17T12:28:55Z

client/input.go

@@ -133,5 +128,6 @@ func readLineFromBuf(buf *bufio.Reader) (string, error) {

 // PrintPrefixed prints a string with > prefixed for use in prompts.
 func PrintPrefixed(msg string) {
-	fmt.Printf("> %s\n", msg)
+	msg = fmt.Sprintf("> %s\n", msg)
+	fmt.Fprint(os.Stderr, msg)


maybe it's better to avoid modifying msg and just do

fmt.Fprint(os.Stderr, fmt.Sprintf("> %s\n", msg))

ebuchman added 6 commits August 19, 2018 20:02

crypto/keys/hd: use btcec to remove dep on tendermint

b1064a5

crypto/keys/bcrypt: improve comment about fork

cc61fa7

crypto/keys/bip39 -> crypto/keys/bip39/fundraiser

115ec48

crypto/keys/bip39: bring in fork of tyler-smith

d9390bf

crypto/keys/hd: update dep

860a1ce

crypto/keys: update deps

bd4876b

ebuchman requested review from cwgoes and rigelrozanski as code owners August 20, 2018 00:36

crypto/keys: move mintkey.go into new crypto/keys/mintkey

930b934

ebuchman added 2 commits August 19, 2018 22:09

crypto/keys/hd: NewParamsFromPath

edc414b

crypto/keys: keybase.Derive takes a bip39 passphrase too

e79e35e

ebuchman force-pushed the bucky/keys branch from cd72d7f to fcd3c86 Compare August 20, 2018 02:45

ebuchman changed the title ~~[WIP] Improve crypto/keys~~ Improve crypto/keys Aug 20, 2018

ebuchman added 2 commits August 19, 2018 22:55

crypto/keys/hd: BIP44Params.DerivationPath

d743d10

gaiacli keys: add commands new and mnemonic

fcd3c86

fix lints

9d6f065

ebuchman commented Aug 20, 2018

View reviewed changes

minor fixes from review

32bc332

ebuchman mentioned this pull request Aug 20, 2018

crypto/keys: Improvements to Keybase #2095

Closed

ebuchman added 2 commits August 20, 2018 01:10

update Gopkg.toml

f761d96

* add tendermint fork of golang.org/x/crypto * pin some transitive deps

crypto/keys/bcrypt: remove

5fa2da1

* remove in favour of fork of golang.org/x/crypto/bcrypt at github.com/tendermint/crypto/bcrypt

ebuchman mentioned this pull request Aug 20, 2018

Change ed25519 and bcrypt to be forked off golang/x/crypto tendermint/tendermint#1959

Closed

ebuchman added 2 commits August 20, 2018 01:25

crypto/keys/bip39: remove completely

27a5815

* use fork cosmos/go-bip39 instead

Gopkg.toml: dont use master

5b2221d

alexanderbez added T: UX wip C:Keys Keybase, KMS and HSMs labels Aug 20, 2018

ebuchman changed the title ~~R4R: Improve crypto/keys and add keys mnemonic and keys new~~ R4R: Improve crypto/keys and add keys mnemonic and keys new commands Aug 30, 2018

ebuchman mentioned this pull request Aug 30, 2018

R4R: Add keys new and keys mnemonic commands #2155

Closed

5 tasks

mslipper suggested changes Aug 30, 2018

View reviewed changes

mslipper and others added 5 commits August 30, 2018 16:00

Pull in changes from my PR

24e8ec1

Merge pull request #2196 from mslipper/bucky/keys

fd8a1ee

Pull in changes from my PR

Merge branch 'develop' into bucky/keys

b22f835

fixes from review

a41c32d

enforce min len for --unsafe-entropy

a5bb443

rigelrozanski added 2 commits September 25, 2018 16:53

Merge remote-tracking branch 'origin/develop' into bucky/keys

f3ec6bc

lint fix

57abdc8

rigelrozanski reviewed Sep 25, 2018

View reviewed changes

alessio reviewed Sep 26, 2018

View reviewed changes

cwgoes mentioned this pull request Oct 12, 2018

Update LCD keys endpoint for crypto/keybase API updates #1442

Closed

Merge branch 'develop' into bucky/keys

82f4def

feedback from review

ac6bceb

rigelrozanski approved these changes Oct 17, 2018

View reviewed changes

ebuchman and others added 2 commits October 17, 2018 12:30

fix dep

71513e5

Merge branch 'develop' into bucky/keys

96c0739

rigelrozanski merged commit 1ee8dee into develop Oct 17, 2018

rigelrozanski deleted the bucky/keys branch October 17, 2018 18:00

melekes reviewed Oct 18, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

R4R: Improve crypto/keys and add `keys mnemonic` and `keys new` commands #2090

R4R: Improve crypto/keys and add `keys mnemonic` and `keys new` commands #2090

ebuchman commented Aug 20, 2018 •

edited by alexanderbez

Loading

ebuchman commented Aug 20, 2018

codecov bot commented Aug 20, 2018 •

edited

Loading

ebuchman Aug 20, 2018

jleni Aug 20, 2018

jaekwon commented Aug 20, 2018

mslipper left a comment

mslipper Aug 30, 2018

ebuchman Sep 22, 2018

mslipper Aug 30, 2018

mslipper Aug 30, 2018

ebuchman Sep 22, 2018

rigelrozanski commented Sep 25, 2018

mslipper commented Sep 25, 2018

rigelrozanski left a comment

rigelrozanski Sep 25, 2018

ebuchman Oct 17, 2018

rigelrozanski Oct 17, 2018

ebuchman Oct 17, 2018

alessio Sep 26, 2018

alessio Sep 26, 2018

fedekunze commented Oct 16, 2018 •

edited

Loading

rigelrozanski commented Oct 16, 2018

ebuchman commented Oct 17, 2018

ebuchman commented Oct 17, 2018

ebuchman commented Oct 17, 2018

rigelrozanski commented Oct 17, 2018 •

edited

Loading

rigelrozanski left a comment

melekes Oct 17, 2018

R4R: Improve crypto/keys and add keys mnemonic and keys new commands #2090

R4R: Improve crypto/keys and add keys mnemonic and keys new commands #2090

Conversation

ebuchman commented Aug 20, 2018 • edited by alexanderbez Loading

ebuchman commented Aug 20, 2018

codecov bot commented Aug 20, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaekwon commented Aug 20, 2018

mslipper 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

rigelrozanski commented Sep 25, 2018

mslipper commented Sep 25, 2018

rigelrozanski 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

fedekunze commented Oct 16, 2018 • edited Loading

rigelrozanski commented Oct 16, 2018

ebuchman commented Oct 17, 2018

ebuchman commented Oct 17, 2018

ebuchman commented Oct 17, 2018

rigelrozanski commented Oct 17, 2018 • edited Loading

rigelrozanski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

R4R: Improve crypto/keys and add `keys mnemonic` and `keys new` commands #2090

R4R: Improve crypto/keys and add `keys mnemonic` and `keys new` commands #2090

ebuchman commented Aug 20, 2018 •

edited by alexanderbez

Loading

codecov bot commented Aug 20, 2018 •

edited

Loading

fedekunze commented Oct 16, 2018 •

edited

Loading

rigelrozanski commented Oct 17, 2018 •

edited

Loading