[WIP] External signer support (e.g. hardware wallet) #14912

Sjors · 2018-12-10T14:46:02Z

This PR lets bitcoind to call an arbitrary command -signer=<cmd>, e.g. a hardware wallet driver, where it can fetch public keys, ask to display an address, and sign a PSBT.

It's design to work with https://github.com/bitcoin-core/HWI, which supports multiple hardware wallets. Any command with the same arguments and return values will work. It simplifies the manual procedure described here.

Usage is documented in doc/external-signer.md, which also describes what protocol a different signer binary should conform to.

It adds the following RPC methods:

enumeratesigners: asks for a list of signers (e.g. devices) and their master key fingerprint
signerfetchkeys (needs Add getdescriptors command bitcoin-core/HWI#137): asks for descriptors and then fills the keypool (no private keys)
signerdisplayaddress <address>: asks to display an address
signerprocesspsbt <psbt> to send the <psbt> to <cmd> to sign and wait for the result

Usage TL&DR:

clone HWI repo somewhere and launch bitcoind -signer=../HWI/hwi.py
create wallet without private keys: bitcoin-cli createwallet hww true
list hardware devices: bitcoin-cli enumeratesigners
fetch keys from hardware device into the wallet: bitcoin-cli -rpcwallet=hww signerfetchkeys
display address on device, sign transaction: see doc/external-signer.md

For easier review, this builds on the following PRs:

util: add RunCommandParseJSON #15382: add runCommandParseJSON
Descriptor: add GetAddressType() and IsSegWit() #15590 Descriptor: add GetAddressType() and IsSegwit()

Potentially useful followups:

[rpc] signer send and fee bump convenience methods #15876: signer send and bumpfee conveniance methods
Native Descriptor Wallets using DescriptorScriptPubKeyMan #16528: descriptor based wallets (to preserve BIP44/49/84 compatibility with mixed address types)
(automatically) verify (a subset of) keys on the device after import, through message signing

Sjors · 2018-12-15T11:22:06Z

Now that #14491 has been rebased, the hww branch I'm building off should also soon be rebased. At that point I can rebase and make Travis happy. In addition, I'll be able to leverage #14646 to clean up my descriptor related code (I'm currently using string concatenation to build descriptors). I have a few other spring cleaning items on my todo list too.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#16542 (Return more specific errors about invalid descriptors by achow101)
#16539 ([wallet] lower -txmaxfee default from 0.1 to 0.01 BTC by Sjors)
#16528 ([WIP] Native Descriptor Wallets (take 2) by achow101)
#16365 (Log RPC parameters (arguments) if -debug=rpcparams by LarryRuane)
#16341 (Introduce ScriptPubKeyMan interface and use it for key and script management (aka wallet boxes) by achow101)
#16273 (refactor: Remove unused includes by practicalswift)
#15876 ([rpc] signer send and fee bump convenience methods by Sjors)
#15529 (Add Qt programs to msvc build (updated, no code changes) by sipsorcery)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

src/wallet/rpcwallet.h

practicalswift · 2018-12-18T16:20:53Z

src/wallet/externalsigner.cpp

+        std::string fingerprintStr = fingerprint.get_str();
+        // Skip duplicate signer
+        bool duplicate = false;
+        for (ExternalSigner signer : signers) {


ExternalSigner signer shadows UniValue signer :-)

src/wallet/rpcdump.cpp

practicalswift · 2018-12-18T16:24:28Z

src/wallet/rpcdump.cpp

+    UniValue importdata(UniValue::VARR);
+
+    uint64_t keypool_target_size = 0;
+    keypool_target_size = gArgs.GetArg("-keypool", DEFAULT_KEYPOOL_SIZE);


Make signedness changing implicit conversion explicit? Also, remove redundant initialization to zero on the line above? Merge the two lines.

The compiler didn't like it when I initialized using gArgs.GetArg.

practicalswift · 2018-12-18T16:29:39Z

src/util/system.cpp

+{
+    if (strCommand.empty()) return UniValue::VNULL;
+
+    std::array<char, 128> buffer;


Initialize to zero?

practicalswift · 2018-12-18T16:31:09Z

src/wallet/rpcwallet.cpp

+        ExternalSigner *signer = GetSignerForJSONRPCRequest(request, 4, pwallet);
+        if (signer && !bip32derivs) JSONRPCError(RPC_WALLET_ERROR, "Using signer requires bip32derivs argument to be true");
+
+        // Check if signer fingerpint matches any input master key fingerprint


Should be "fingerprint" :-)

practicalswift · 2018-12-18T16:38:09Z

src/util/system.cpp

+
+    std::array<char, 128> buffer;
+    std::string result;
+    std::unique_ptr<FILE, decltype(&pclose)> pipe(popen(strCommand.c_str(), "r"), pclose);


I haven't reviewed this use of popen(…) closer, but please note the recommendations/risks with regards to popen(…) used described in the CERT secure coding guidelines (more specifically rule ENV33-C).

This is straight from stack overflow, so certainly needs more review... I'm surprised how complicated it is to just read some JSON from stdout.

popen and pclose are unix-like only. You can't use them on Windows. IMO you could use boost process instead.

I think we don't depend on boost process.

I know. But this is the simplest way if this is really necessary to call another process. I don't think that any one here know how to use Windows CreateProcess API.

It would be reasonable to call _popen function here on windows or #define popen _popen: https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/system-wsystem.

You do need to be very careful about special characters passed in the command string, but it should be ok if you stick to hex/base64.

_popen would only work in console program. See https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/popen-wpopen. So it doesn't work in Qt.

practicalswift · 2018-12-18T16:38:57Z

src/util/strencodings.cpp

+    return ret;
+}
+
+std::string WriteHdKeypath(const std::vector<uint32_t> keypath)


keypath should be const reference?

practicalswift · 2018-12-18T16:42:48Z

src/wallet/rpcwallet.cpp

+    }
+
+    const std::string command = gArgs.GetArg("-signer", DEFAULT_EXTERNAL_SIGNER);
+    if (command == "") throw JSONRPCError(RPC_WALLET_ERROR, "Error: restart bitcoind with -signer=<cmd>");


Nit: Could use command.empty()? :-)

practicalswift · 2018-12-18T16:43:39Z

src/wallet/externalsigner.cpp

+    const UniValue result = runCommandParseJSON(command + " enumerate");
+    if (!result.isArray())
+        throw ExternalSignerException(strprintf("'%s' received invalid response, expected array of signers", command));
+    for (UniValue signer : result.getValues()) {


Should be const reference? :-)

practicalswift · 2018-12-18T16:43:54Z

src/wallet/externalsigner.cpp

+        const UniValue& fingerprint = find_value(signer, "fingerprint");
+        if (result.isNull())
+            throw ExternalSignerException(strprintf("'%s' received invalid response, missing signer fingerprint", command));
+        std::string fingerprintStr = fingerprint.get_str();


Should be const reference? :-)

practicalswift · 2018-12-18T16:44:29Z

src/wallet/externalsigner.cpp

+        // Skip duplicate signer
+        bool duplicate = false;
+        for (ExternalSigner signer : signers) {
+            if (signer.m_fingerprint.compare(fingerprintStr) == 0) duplicate = true;


Use string equality operator instead of compare :-)

practicalswift · 2018-12-18T16:47:04Z

src/wallet/rpcwallet.cpp

+        for (unsigned int i = 0; i < psbtx.inputs.size(); ++i) {
+            const PSBTInput& input = psbtx.inputs[i];
+            for (auto entry : input.hd_keypaths) {
+                if (signer->m_fingerprint == strprintf("%08x", ReadBE32(entry.second.fingerprint))) match = true;


signer can be nullptr here if the check on L4016 is correct?

practicalswift · 2018-12-18T16:47:38Z

src/wallet/rpcwallet.cpp

+        if (!match) JSONRPCError(RPC_WALLET_ERROR, "Signer fingerprint does not match any of the inputs");
+
+        // Call signer, get result
+        const UniValue signer_result = signer->signTransaction(request.params[0].get_str());


Same here: signer can be nullptr here if the check on L4016 is correct?

practicalswift · 2018-12-18T16:48:58Z

src/util/strencodings.h

@@ -204,6 +204,10 @@ bool ConvertBits(const O& outfn, I it, I end) {
 /** Parse an HD keypaths like "m/7/0'/2000". */
 NODISCARD bool ParseHDKeypath(const std::string& keypath_str, std::vector<uint32_t>& keypath);

+/** Write HD keypaths as strings */
+std::string WriteHdKeypath(const std::vector<uint32_t> keypath);


keypath should be const reference?

practicalswift · 2018-12-18T16:50:23Z

src/wallet/rpcdump.cpp

+    UniValue receive_descriptors = signer->getKeys(receive_desc);
+    if (!receive_descriptors.isArray()) JSONRPCError(RPC_WALLET_ERROR, "Expected an array of receive descriptors");
+    for (const UniValue& descriptor : receive_descriptors.getValues()) {
+        descriptors.push_back(descriptor);


Nit: Could use std::copy instead?

practicalswift · 2018-12-18T16:50:51Z

src/wallet/rpcdump.cpp

+    UniValue change_descriptors = signer->getKeys(change_desc);
+    if (!change_descriptors.isArray()) JSONRPCError(RPC_WALLET_ERROR, "Expected an array of change descriptors");
+    for (const UniValue& descriptor : change_descriptors.getValues()) {
+        descriptors.push_back(descriptor);


Same here: Could you std::copy?

practicalswift · 2018-12-18T16:52:53Z

src/wallet/rpcdump.cpp

+    std::string desc_prefix = "";
+    std::string desc_suffix = "";
+    std::string purpose = "";
+    switch(pwallet->m_default_address_type) {


Missing space before (. Consider running new code through clang-format :-)

practicalswift · 2018-12-18T16:53:15Z

src/wallet/rpcdump.cpp

+    }
+
+
+    switch(pwallet->m_default_change_type) {


Same here: Missing whitespace before (.

src/wallet/rpcwallet.cpp

test/functional/wallet_importmulti.py

Create basic ExternalSigner class with contructor. A Signer(<cmd>) is added to CWallet on load if -signer=<cmd> is set.

Sjors · 2019-08-02T18:46:55Z

Rebased now that #15911 is merged. Dropped a few commits that should have been in #15876 and aren't needed anyway after #15713. Removed the need for the unit test to add private keys to the keypool (closing #15414).

I'll keep this and the convenience methods in #15876 up to date and work on a "boxed" version in a separate PR.

Sjors · 2019-08-03T11:05:21Z

Here's a simple rebase on top of a native descriptor wallet #16528 (benefit: gives access to full BIP44/49/84 address tree): https://github.com/Sjors/bitcoin/tree/2019/08/hww-box

Includes a mock to mimick the HWI interace.

Sjors · 2019-08-04T21:43:57Z

Closing in favor of the native descriptor edition in #16546.

Sjors mentioned this pull request Dec 10, 2018

Hardware wallet support #14145

Closed

Sjors force-pushed the 2018/11/rpc-signer branch from e021812 to a2b018d Compare December 10, 2018 14:56

DrahtBot added the Needs rebase label Dec 10, 2018

laanwj added the Wallet label Dec 11, 2018

Sjors force-pushed the 2018/11/rpc-signer branch from a2b018d to 9ec4aac Compare December 17, 2018 18:06

DrahtBot removed the Needs rebase label Dec 17, 2018