cmd/bnscli: use mnemonic to generate private key #923

husio · 2019-07-29T13:31:47Z

Instead of generating a random private key use algorithm described in
https://github.com/iov-one/iov-core/blob/master/docs/address-derivation-v1.md
to create a mnemonic and later use that mnemonic to create a private
key.

resolve #855

Tests are missing.

🗒️ Use https://iov-one.github.io/token-finder/ for testing

Instead of generating a random private key use algorithm described in https://github.com/iov-one/iov-core/blob/master/docs/address-derivation-v1.md to create a mnemonic and later use that mnemonic to create a private key. resolve #855 Tests are missing.

webmaster128

🐎

cmd/bnscli/cmd_key.go

alpe

Good start but from an UX point I would prefer the key generation to create and add the mnemonic and derived key to the storage instead than to pipe another command.
Looks like you want to cut a corner but not sure if the mnemonic command is of any use without key generation. We can have an import and export command instead that deal with the other processes.

husio · 2019-07-30T09:33:04Z

Good start but from an UX point I would prefer the key generation to create and add the mnemonic and derived key to the storage instead than to pipe another command.

I did this on purpose. I think it makes sense as each operation produce a result that is important on its own. Those commands are not always expected to be piped. I think the use cases are for example:

Generate a new mnemonic
Generate a key for my mnemonic using path m/44'/234'/0'
Generate a key for my mnemonic using path m/44'/234'/1'

I don't think it is a valid usage if you do the following:

$ bnscli mnemonic | bnscli keygen

You have generated a private key, but you lost the mnemonic which I understand is much more important than the private key.

Using separate commands enforce the user to make an important decision of where to store the mnemonic. It must be in a safe place that I do not want to guess. Otherwise, I would have to choose to store it in a file and decide on the location. In the current implementation, you can pipe it to an encryption program before writing to a file. You can use the same program of your choice (ie Vim 😂) to decode it before sending to bnscli keygen 💪

Does this make sense? 😃

husio · 2019-07-30T09:50:38Z

I think all is in place. Please review again.

codecov-io · 2019-07-30T09:52:22Z

Codecov Report

Merging #923 into master will decrease coverage by 0.2%.
The diff coverage is 29.6%.

@@           Coverage Diff            @@
##           master    #923     +/-   ##
========================================
- Coverage    65.1%   64.8%   -0.3%     
========================================
  Files         176     176             
  Lines       10174   10242     +68     
========================================
+ Hits         6627    6646     +19     
- Misses       2801    2847     +46     
- Partials      746     749      +3

Impacted Files	Coverage Δ
cmd/bnscli/main.go	`0% <ø> (ø)`	⬆️
cmd/bnscli/common.go	`61.4% <0%> (-5.7%)`	⬇️
cmd/bnscli/cmd_key.go	`18.2% <33.3%> (+18.2%)`	⬆️
cmd/bnscli/cmd_query.go	`0% <0%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d5aada8...d94e1c2. Read the comment docs.

webmaster128

Good job

webmaster128 · 2019-07-30T10:02:50Z

cmd/bnscli/cmd_key.go

+// path.
+func keygen(mnemonic, derivationPath string) (ed25519.PrivateKey, error) {
+	if !bip39.IsMnemonicValid(string(mnemonic)) {
+		return nil, errors.New("invalid mnemonic")


Can we test this validation for

valid mnemonic 12 words

valid mnemonic 15 words

valid mnemonic 18 words

valid mnemonic 21 words

valid mnemonic 24 words

invalid: mnemonic with wrong checksum (change last word to some other workd from the workdlist)

invalid: additional whitespace around valid mnemonnic

invalid: mnenomic that is valid in a language other than English

The code processing the mnemonic is not ours and usually I would argue that we should not test it. But here it is 4b19844

invalid: mnemonic with wrong checksum (change last word to some other workd from the workdlist)

I do not understand this case, so I did not include it in the tests.

The code processing the mnemonic is not ours and usually I would argue that we should not test it.

We don't need to test every single code branch of that external library but we should at least cover our integration of it as well as the bahaviour we expect from the API.

I do not understand this case, so I did not include it in the tests.

I'm glad we talk about it: A BIP39 mnemonic is not just a list of 12, 15, 18, 21 or 24 words from a 2048 element wordlist. It is encoded data plus a checksum. In a 12 word mnemonic for example, 11.64 words are arbitrary data and 0.36 words are a checksum.

e.g.

valid: crack turtle seminar height entire subway motion rail pass seat violin scene invalid (first word changed): turtle turtle seminar height entire subway motion rail pass seat violin scene invalid (last word changed): crack turtle seminar height entire subway motion rail pass seat violin violin

It seems as the implementation is not complete.
https://github.com/tyler-smith/go-bip39/blob/master/bip39.go#L275-L294

It ignores whitespace by using string.Fields to extract words. There is an issue for this Mnemonic whitespace separator deviates from reference implementation tyler-smith/go-bip39#29

It ensures that the number of words is in the right amount

It ensures that all words belong to the dictionary

It does not check the checksum IsMnemonicValid does not validate checksum tyler-smith/go-bip39#32

I think the best is to create a pull request with a fix.

I think the best is to create a pull request with a fix.

I agree. Good that we checked that ;)

Regarding 1.: the behaviour is fair indeed. We should just be explicit about it since not every implementations wirks that way. In this case we should have a test that verifies that mnemonic and menonic+whitespace leads to the same keypair.

webmaster128 · 2019-07-31T09:20:02Z

cmd/bnscli/cmd_key_test.go

+		},
+		"valid mnemonic 24 words": {
+			mnemonic: "usage mountain noodle inspire distance lyrics caution wait mansion never announce biology squirrel guess key gain belt same matrix chase mom beyond model toy",
+			wantErr:  false,


The last mnemonic is valid but the cut ones (15–21 words) are not due to invalid checksum. How can those tests pass?

webmaster128 · 2019-07-31T09:21:39Z

cmd/bnscli/cmd_key_test.go

+					never announce biology
+			`,
+			wantErr: false,
+		},


Why is the additional whitespace test passing? When you add additional whitespace to a mnemonic, you get a different kypair/address

husio · 2019-07-31T12:22:45Z

@webmaster128 please check again. I did not manage to fix the bip39.IsMnemonicValid function but I have found out that another function does better validation fd82cc9#diff-6569bb577f21a71b8e731cb7dc0f2b24R96

This bip39 is .... could be improved. It is also not updated/fixed anymore and broadly used which is very surprising. If I had 2 days of time I would for sure write it from scratch myself to have a proper implementation.

webmaster128 · 2019-07-31T12:37:21Z

@husio nice, looks good. Feel free to create a ticket for writing such a module. In this area a single bit can make the different between being rich and not having access to tokens at all.

The only thing I am missing right now is a test that shows two mnemonics return the same keypair even if they differ in whitespace.

husio · 2019-07-31T12:50:00Z

Done in d370993

webmaster128

nice

husio · 2019-07-31T13:51:56Z

Mnemonic must be normalized before being used, so I have extended the validation to not allow any whitespace separator. To avoid issues, only a single space word separator is allowed.

alpe · 2019-08-01T08:22:00Z

cmd/bnscli/clitests/keygen.test

 # Use a custom key path just in case the host is using original one.
 tempdir=`mktemp -d`
 keypath=$tempdir/key.priv
 export BNSCLI_PRIV_KEY=$keypath

-bnscli keygen
+bnscli keygen < $mnemonic
+rm $mnemonic


req: please update the comments below as key derivation is deterministic.

alpe · 2019-08-01T08:26:54Z

cmd/bnscli/cmd_key.go

@@ -25,6 +32,7 @@ created. This command fails if the private key file already exists.
 	var (
 		keyPathFl = fl.String("key", env("BNSCLI_PRIV_KEY", os.Getenv("HOME")+"/.bnsd.priv.key"),
 			"Path to the private key file that transaction should be signed with. You can use BNSCLI_PRIV_KEY environment variable to set it.")
+		pathFl = fl.String("path", "m/44'/234'/0'", "Derivation path as described in BIP-44.")


pp: please add default path to the description so that it can be copied and modified knowing our exact format and numbers if required.

Default is always printed as part of the help message.

-path string Derivation path as described in BIP-44. (default "m/44'/234'/0'")

alpe · 2019-08-01T08:30:22Z

cmd/bnscli/cmd_key.go

+// words as this is important during seed creation.
+func validateMnemonic(mnemonic string) error {
+	// A lazy way to check that words are exactly single space separated.
+	expected := strings.Join(strings.Fields(mnemonic), " ")


Works for whitespace. Is upper/lower case still considered a problem here?

I think validation is case-sensitive as it has a fixed set of words that it is using to map each word to a numeric value. Using non lowercase letter would not return a match in the index.

alpe · 2019-08-01T08:41:22Z

cmd/bnscli/cmd_key.go

@@ -66,6 +126,7 @@ Print out a hex-address associated with your private key.
 	var (
 		keyPathFl = fl.String("key", env("BNSCLI_PRIV_KEY", os.Getenv("HOME")+"/.bnsd.priv.key"),
 			"Path to the private key file that transaction should be signed with. You can use BNSCLI_PRIV_KEY environment variable to set it.")
+		bechPrefixFl = fl.String("bp", "iov", "Bech32 prefix.")


req: please add the 2 formats we have for tiov testnet and iov as default to the description.

What do you mean? You can have only one default value.

alpe · 2019-08-01T08:48:58Z

cmd/bnscli/cmd_key.go

+// https://github.com/iov-one/iov-core/blob/8846fed17443766a9ad9c908c3d7fc9d205e02ef/docs/address-derivation-v1.md#deriving-addresses-from-keypairs
+func toBech32(prefix string, pubkey []byte) ([]byte, error) {
+	data := append([]byte("sigs/ed25519/"), pubkey...)
+	hash := sha256.Sum256(data)


pp: hash[:20] is the same as weave.NewCondition("sigs", "ed25519", pubkey).Address((). Please use existing logic instead for this sensible part.

alpe · 2019-08-01T08:51:57Z

cmd/bnscli/common.go

+	return ioutil.ReadAll(input)
+}
+
+var errNoPipe = errors.New("no data piped")


req: Please move const and vars before the methods using them.

husio requested review from alpe and webmaster128 July 29, 2019 13:31

webmaster128 reviewed Jul 29, 2019

View reviewed changes

cmd/bnscli/cmd_key.go Outdated Show resolved Hide resolved

cmd/bnscli/cmd_key.go Outdated Show resolved Hide resolved

cmd/bnscli/cmd_key.go Outdated Show resolved Hide resolved

alpe reviewed Jul 29, 2019

View reviewed changes

husio added 2 commits July 29, 2019 16:12

bnscli: validate mnemonic before using

ef5fb9f

update bnscli key management

88f43b2

husio marked this pull request as ready for review July 30, 2019 09:50

CHANGELOG

93f3406

webmaster128 reviewed Jul 30, 2019

View reviewed changes

bnscli: mnemonic tests

4b19844

webmaster128 reviewed Jul 31, 2019

View reviewed changes

bnscli: improve mnemonic validation

fd82cc9

ensure mnemonic whitespace does not influence output

d370993

webmaster128 approved these changes Jul 31, 2019

View reviewed changes

husio mentioned this pull request Jul 31, 2019

Implement BIP-39 #929

Closed

3 tasks

more bip-39 package issues solved

0094cae

fix bnscli tests

d94e1c2

husio merged commit 788afb0 into master Aug 1, 2019

husio deleted the secure_key_management_issue_855 branch August 1, 2019 07:40

alpe reviewed Aug 1, 2019

View reviewed changes

lrettig mentioned this pull request Jul 11, 2023

Whitespace in input mnemonic produces different seed spacemeshos/smcli#58

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmd/bnscli: use mnemonic to generate private key #923

cmd/bnscli: use mnemonic to generate private key #923

husio commented Jul 29, 2019 •

edited

webmaster128 left a comment

alpe left a comment

husio commented Jul 30, 2019 •

edited

husio commented Jul 30, 2019

codecov-io commented Jul 30, 2019 •

edited

webmaster128 left a comment

webmaster128 Jul 30, 2019

husio Jul 31, 2019 •

edited

webmaster128 Jul 31, 2019

husio Jul 31, 2019 •

edited

webmaster128 Jul 31, 2019 •

edited

webmaster128 Jul 31, 2019

webmaster128 Jul 31, 2019

husio commented Jul 31, 2019

webmaster128 commented Jul 31, 2019

husio commented Jul 31, 2019

webmaster128 left a comment

husio commented Jul 31, 2019

alpe Aug 1, 2019

alpe Aug 1, 2019

husio Aug 1, 2019

alpe Aug 1, 2019

husio Aug 1, 2019

alpe Aug 1, 2019

husio Aug 1, 2019

alpe Aug 1, 2019

alpe Aug 1, 2019

cmd/bnscli: use mnemonic to generate private key #923

cmd/bnscli: use mnemonic to generate private key #923

Conversation

husio commented Jul 29, 2019 • edited

webmaster128 left a comment

Choose a reason for hiding this comment

alpe left a comment

Choose a reason for hiding this comment

husio commented Jul 30, 2019 • edited

husio commented Jul 30, 2019

codecov-io commented Jul 30, 2019 • edited

Codecov Report

webmaster128 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

husio Jul 31, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

husio Jul 31, 2019 • edited

Choose a reason for hiding this comment

webmaster128 Jul 31, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

husio commented Jul 31, 2019

webmaster128 commented Jul 31, 2019

husio commented Jul 31, 2019

webmaster128 left a comment

Choose a reason for hiding this comment

husio commented Jul 31, 2019

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

husio commented Jul 29, 2019 •

edited

husio commented Jul 30, 2019 •

edited

codecov-io commented Jul 30, 2019 •

edited

husio Jul 31, 2019 •

edited

husio Jul 31, 2019 •

edited

webmaster128 Jul 31, 2019 •

edited