wallet: factory does not propagate passphrase

[jeefave]

No description provided.

[jeefave]

@pinheadmz minor update to allow the wallet to generate the master key based of the {mnemonic, passphrase}

[pinheadmz]

Thanks for contributing, @jeefave.

First of all, nits: Pull Requests should really include tests and the CI has failed on a lint error :-/ There's some notes in https://github.com/handshake-org/hsd/blob/master/CONTRIBUTING.md about this.

So, the patch itself: What is your goal/use-case here?

Sadly, hsd is a bit confusing about passphrase and password...

From the user's perspective the option called "passphrase" is the string used to encrypt the wallet's master private key, and is required for all wallet-signing functions. The user's option called "password" in the code is immediately cast to a new variable called passphrase as it is defined in BIP39, i.e. an extra string added to the mnemonic seed phrase before the wallet's master private key is derived.

The BIP39 "passphrase" is never used again by the wallet, and the user doesn't ever need to enter it again. In fact, the user's wallet will still be unencrypted until they actually set a "passphrase" 😠 . I think this is why the BIP39 "passphrase" was never really implemented in bcoin/hsd -- we just use the encryption method for security instead of the obfuscated key derivation method. by the way, the BIP39 passphrase method has been criticized because ANYTHING you enter will be a valid private key. The encryption-with-passphrase model ensures that if the user enters the wrong passphrase, they will get an error, not a totally unexpectedly different wallet!

SO -- maybe you used a different wallet that used the BIP39 "passphrase" and you want to import that into hsd. If that's your use case it can be added to hsd but I found a slightly different method than yours:

diff --git a/lib/hd/hd.js b/lib/hd/hd.js
index 554b1f10c..7b1976741 100644
--- a/lib/hd/hd.js
+++ b/lib/hd/hd.js
@@ -61,8 +61,8 @@ HD.fromSeed = function fromSeed(options) {
  * @returns {HDPrivateKey}
  */

-HD.fromMnemonic = function fromMnemonic(options) {
-  return HDPrivateKey.fromMnemonic(options);
+HD.fromMnemonic = function fromMnemonic(options, passphrase) {
+  return HDPrivateKey.fromMnemonic(options, passphrase);
 };

 /**
diff --git a/lib/wallet/http.js b/lib/wallet/http.js
index b3862088b..a3111760d 100644
--- a/lib/wallet/http.js
+++ b/lib/wallet/http.js
@@ -299,6 +299,7 @@ class HTTP extends Server {
         m: valid.u32('m'),
         n: valid.u32('n'),
         passphrase: valid.str('passphrase'),
+        password: valid.str('password'),
         master: master,
         mnemonic: mnemonic,
         accountKey: accountKey,

With this patch, the "password" (which is really a BIP39 "passphrase") is passed as an option to the wallet and the HD derivation function without disrupting any existing code paths.

After applying this patch I created two wallet with the same mnemonic but adding the "password" to the second. You can see these wallets have the same mnemonic, same entropy, but different master private keys. Both wallets can spend funds without requiring the user to enter anything secret at all.

--> hsw-cli --id=bip39-3 master
{
  "encrypted": false,
  "key": {
    "xprivkey": "rprvKE8qsHtkmUxUSR5EThrMuT1X3PDVspR7JcJGMbLEwQiCh48GMn82QxFYXcN61P1fgSJ6tLsC4e9mZvBYwaCq5HgPyEcKep8sxkmTevgqPboX"
  },
  "mnemonic": {
    "bits": 256,
    "language": "english",
    "entropy": "371cfff85252c6204d94e287dcba14c72c9880ffa7fbb6458b4e8ec94fd36fe7",
    "phrase": "damage tree wrap pigeon cluster awake curve ordinary march total lunch mixture sister document write youth rent clump squirrel budget neutral trust sauce video"
  }
}
--> hsw-cli --id=bip39-5 master
{
  "encrypted": false,
  "key": {
    "xprivkey": "rprvKE8qsHtkmUxUSNzgN8evWRC5CoUMBa964LRqH6Amysa2nxknk9JeYQok4kUbzFRiRsur8nCa1AjDD9T559LmJRZ4hGHXa9fNA1ZFJvrCtkhp"
  },
  "mnemonic": {
    "bits": 256,
    "language": "english",
    "entropy": "371cfff85252c6204d94e287dcba14c72c9880ffa7fbb6458b4e8ec94fd36fe7",
    "phrase": "damage tree wrap pigeon cluster awake curve ordinary march total lunch mixture sister document write youth rent clump squirrel budget neutral trust sauce video"
  }
}

[jeefave]

@pinheadmz Thank you for your reviewing the PR!

The goal is to increase the security of a multisig cold wallet by supplying a passphrase during the signature flow.
I updated the PR with the changes you suggested. It seems to solve the use case perfectly.

Please let me know your thoughts.

[pinheadmz]

@jeefave I don't think that's what this will do, though. And if that is what you want it to do, your test should reflect that!

Just to be clear: we are talking about the BIP39 "extra seed word" style passphrase here. In this implementation, it is only used when a wallet is created and NEVER AGAIN (even during signing).

If a user encrypts their wallet (this also applies to multisig) with the existing "paspshrase" option, that passphrase will be required during signing, because the master private key is encrypted.

If this is confusing or I'm misunderstanding you, we can chat on telegram: https://t.me/hns_tech

[coveralls]

Pull Request Test Coverage Report for Build 900258127

• 3 of 3 (100.0%) changed or added relevant lines in 2 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.05%) to 60.123%

---

Totals
Change from base Build 863907438:	0.05%
Covered Lines:	19867
Relevant Lines:	30796

---

💛 - Coveralls

[jeefave]

@pinheadmz please let me know if any changes are required

[pinheadmz]

@jeefave OK sorry about this, it's my fault but when I suggested "bip39-passphrase" as the option name I was thinking of the CLI commands, where dashes are used. In the code we use camel case (and the bcfg package handles this for us). So I think we should apply this diff: https://gist.github.com/pinheadmz/3aacf83e8ee16d0956bf7107b918bf34

You could then open a second PR to hs-client with this addition:

diff --git a/bin/hsw-cli b/bin/hsw-cli
index cdee472..9f31a86 100755
--- a/bin/hsw-cli
+++ b/bin/hsw-cli
@@ -136,6 +136,7 @@ class CLI {
       n: this.config.uint('n'),
       witness: this.config.bool('witness'),
       passphrase: this.config.str('passphrase'),
+      bip39Passphrase: this.config.str('bip39Passphrase'),
       watchOnly: this.config.has('key') ? true : this.config.bool('watch'),
       accountKey: this.config.str('key')
     };

...and then hsw-cli mkwallet --bip39-passphrase=<...> becomes a thing.

I applied this patch to hs-client locally and tested with this bash script: https://gist.github.com/pinheadmz/75e8c8d79dc5a9c18c00a545c4a03b07

...which proves the two passphrase options don't interfere and do work and that bip39-passphrase is only required once when creating the wallet:

9638
 -- 
Should have different xprv
original
"rprvKE8qsHtkmUxUSPzGa3ykeLJb249aGWYdZrUKwkgKf5m67q6u2kR6ZLQcGE142NpANm64csMHWtNq9uYcf17uSSMiSbNtyJzzhiQ1f27R5XJu"
bip39
"rprvKE8qsHtkmUxUSQcTBanzG6eD1TWgAQNsco3QTf9gXb7WYFaK7fCtzonWpka57A28ybhRyeEEXmtoU367X2eDDSBU5zKCgssY59hc4A1uEhbi"
 -- 
Should have same address
original
"rs1q4rvs9pp9496qawp2zyqpz3s90fjfk362q92vq8"
encrypted
"rs1q4rvs9pp9496qawp2zyqpz3s90fjfk362q92vq8"
Should have same address, but different than original & encrypted
bip39
"rs1q9c4jawxtqsytlasm6ve8cltk97gytj65mlez04"
bip39encrypted
"rs1q9c4jawxtqsytlasm6ve8cltk97gytj65mlez04"
 -- 
Should send without error
"ce2a54c1b4a0152ca4285670ef5d727c476ac355db370d0faa56033df3245398"
"0fc280062510008eecacea5176720fe1a5a45f7e6a270f567be4495653460c4b"
Should require wallet-unlocking passphrase
Error: No passphrase.
    at WalletClient.request (/Users/matthewzipkin/Desktop/work/hs-client/node_modules/bcurl/lib/client.js:233:19)
    at processTicksAndRejections (internal/process/task_queues.js:97:5)
    at async CLI.sendTX (/Users/matthewzipkin/Desktop/work/hs-client/bin/hsw-cli:349:16)
    at async CLI.handleWallet (/Users/matthewzipkin/Desktop/work/hs-client/bin/hsw-cli:637:9)
    at async /Users/matthewzipkin/Desktop/work/hs-client/bin/hsw-cli:690:3
Error: No passphrase.
    at WalletClient.request (/Users/matthewzipkin/Desktop/work/hs-client/node_modules/bcurl/lib/client.js:233:19)
    at processTicksAndRejections (internal/process/task_queues.js:97:5)
    at async CLI.sendTX (/Users/matthewzipkin/Desktop/work/hs-client/bin/hsw-cli:349:16)
    at async CLI.handleWallet (/Users/matthewzipkin/Desktop/work/hs-client/bin/hsw-cli:637:9)
    at async /Users/matthewzipkin/Desktop/work/hs-client/bin/hsw-cli:690:3
"17b0a3e62ecc8620efc83b46b0802127a9d0926e7cc8a81bc79c1c19c8e26b66"
"1cdb49763b8abf1c43f938396678976a87309d9199a4e3602fe0d5616e0462c2"
Stopping.

Besides the option name I think everything is right. It's weird that fromMnemonic() used to be overloaded and accept an options object - so I'm glad we're cleaning that up. The only side effect of that I could find is here:

hsd/lib/hd/hd.js

Lines 114 to 115 in c861e4f

	if (options && typeof options === 'object')
	return HD.fromMnemonic(options);

I'm not sure we need to worry about it though, HD.from() is kind of a crappy function anyway, and technically a Mnemonic would pass the typeof === "object" test, so.

[pinheadmz]

@jeefave I'm hijacking this PR to get landed, will merge to master at: 6d237a0