Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 25 commits into
base: master
from

Conversation

Projects
None yet
@Sjors
Copy link
Member

commented Dec 10, 2018

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 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:

  • #15382: add runCommandParseJSON
  • #15911: use wallet default for walletcreatefundedpsbt
  • #15414: add imported private keys to keypool (only needed for tests here)
  • #15590 Descriptor: add GetAddressType() and IsSegwit()

Dependencies, not included in the PR itself:

  • #15457 Check std::system for -[alert|block|wallet]notify

Potentially useful followups:

  • #15487 or #15764: 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 Sjors referenced this pull request Dec 10, 2018

Open

Hardware wallet support #14145

@Sjors Sjors force-pushed the Sjors:2018/11/rpc-signer branch Dec 10, 2018

@laanwj laanwj added the Wallet label Dec 11, 2018

@Sjors

This comment has been minimized.

Copy link
Member Author

commented Dec 15, 2018

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.

See also the wallet meeting notes.

@Sjors Sjors force-pushed the Sjors:2018/11/rpc-signer branch Dec 17, 2018

@DrahtBot DrahtBot removed the Needs rebase label Dec 17, 2018

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Dec 17, 2018

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #15911 (Use wallet RBF default for walletcreatefundedpsbt by Sjors)
  • #15529 (Add Qt programs to msvc build by sipsorcery)
  • #15457 (Check std::system for -[alert|block|wallet]notify by Sjors)

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.

Show resolved Hide resolved src/wallet/rpcwallet.h Outdated
std::string fingerprintStr = fingerprint.get_str();
// Skip duplicate signer
bool duplicate = false;
for (ExternalSigner signer : signers) {

This comment has been minimized.

Copy link
@practicalswift

practicalswift Dec 18, 2018

Member

ExternalSigner signer shadows UniValue signer :-)

Show resolved Hide resolved src/wallet/rpcdump.cpp Outdated
Show resolved Hide resolved src/wallet/rpcdump.cpp Outdated
src/wallet/rpcdump.cpp Outdated
UniValue importdata(UniValue::VARR);

uint64_t keypool_target_size = 0;
keypool_target_size = gArgs.GetArg("-keypool", DEFAULT_KEYPOOL_SIZE);

This comment has been minimized.

Copy link
@practicalswift

practicalswift Dec 18, 2018

Member

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

This comment has been minimized.

Copy link
@Sjors

Sjors Dec 20, 2018

Author Member

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

src/util/system.cpp Outdated
{
if (strCommand.empty()) return UniValue::VNULL;

std::array<char, 128> buffer;

This comment has been minimized.

Copy link
@practicalswift

practicalswift Dec 18, 2018

Member

Initialize to zero?

src/wallet/rpcwallet.cpp Outdated
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

This comment has been minimized.

Copy link
@practicalswift

practicalswift Dec 18, 2018

Member

Should be "fingerprint" :-)

src/util/system.cpp Outdated

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

This comment has been minimized.

Copy link
@practicalswift

practicalswift Dec 18, 2018

Member

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 comment has been minimized.

Copy link
@Sjors

Sjors Dec 20, 2018

Author Member

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.

This comment has been minimized.

Copy link
@ken2812221

ken2812221 Jan 20, 2019

Member

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

This comment has been minimized.

Copy link
@promag

promag Jan 20, 2019

Member

I think we don't depend on boost process.

This comment has been minimized.

Copy link
@ken2812221

ken2812221 Jan 20, 2019

Member

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.

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Jan 22, 2019

Contributor

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.

This comment has been minimized.

Copy link
@ken2812221

ken2812221 Jan 22, 2019

Member

_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.

src/util/strencodings.cpp Outdated
return ret;
}

std::string WriteHdKeypath(const std::vector<uint32_t> keypath)

This comment has been minimized.

Copy link
@practicalswift

practicalswift Dec 18, 2018

Member

keypath should be const reference?

src/wallet/rpcwallet.cpp Outdated
}

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

This comment has been minimized.

Copy link
@practicalswift

practicalswift Dec 18, 2018

Member

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

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()) {

This comment has been minimized.

Copy link
@practicalswift

practicalswift Dec 18, 2018

Member

Should be const reference? :-)

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();

This comment has been minimized.

Copy link
@practicalswift

practicalswift Dec 18, 2018

Member

Should be const reference? :-)

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

This comment has been minimized.

Copy link
@practicalswift

practicalswift Dec 18, 2018

Member

Use string equality operator instead of compare :-)

src/wallet/rpcwallet.cpp Outdated
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;

This comment has been minimized.

Copy link
@practicalswift

practicalswift Dec 18, 2018

Member

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

src/wallet/rpcwallet.cpp Outdated
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());

This comment has been minimized.

Copy link
@practicalswift

practicalswift Dec 18, 2018

Member

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

src/util/strencodings.h Outdated
@@ -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);

This comment has been minimized.

Copy link
@practicalswift

practicalswift Dec 18, 2018

Member

keypath should be const reference?

src/wallet/rpcdump.cpp Outdated
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);

This comment has been minimized.

Copy link
@practicalswift

practicalswift Dec 18, 2018

Member

Nit: Could use std::copy instead?

src/wallet/rpcdump.cpp Outdated
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);

This comment has been minimized.

Copy link
@practicalswift

practicalswift Dec 18, 2018

Member

Same here: Could you std::copy?

src/wallet/rpcdump.cpp Outdated
std::string desc_prefix = "";
std::string desc_suffix = "";
std::string purpose = "";
switch(pwallet->m_default_address_type) {

This comment has been minimized.

Copy link
@practicalswift

practicalswift Dec 18, 2018

Member

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

src/wallet/rpcdump.cpp Outdated
}


switch(pwallet->m_default_change_type) {

This comment has been minimized.

Copy link
@practicalswift

practicalswift Dec 18, 2018

Member

Same here: Missing whitespace before (.

Show resolved Hide resolved src/wallet/rpcwallet.cpp Outdated
Show resolved Hide resolved test/functional/wallet_importmulti.py Outdated

Sjors and others added some commits Feb 13, 2019

Add boost::process
* AppVeyor boost-process vcpkg package.
* Add HAVE_BOOST_PROCESS for MSVC build (bitcoin_config.h)
* Tell Boost linter to ignore it
[wallet] add -signer argument for external signer command
Create basic ExternalSigner class with contructor. A Signer(<cmd>)
is added to CWallet on load if -signer=<cmd> is set.
[test] add external signer test
Includes a mock to mimick the HWI interace.

@Sjors Sjors force-pushed the Sjors:2018/11/rpc-signer branch from 4f9afdd to 654cb83 Jun 7, 2019

Sjors added some commits Feb 15, 2019

@Sjors Sjors force-pushed the Sjors:2018/11/rpc-signer branch from 654cb83 to b540b95 Jun 7, 2019

@Sjors

This comment has been minimized.

Copy link
Member Author

commented Jun 7, 2019

Rebased and dropped signersend from this PR, moved it to #15876 (which also contains fee bump support).

@DrahtBot DrahtBot removed the Needs rebase label Jun 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.