Create signmessagewithprivkey rpc #7953

pull achow101 wants to merge 2 commits into bitcoin:master from achow101:signmessagewithprivkey changing 3 files +84 −0

achow101 commented at 2:34 AM on April 27, 2016: member

Since no one else did...

This adds a new RPC which does not rely on the wallet to sign messages. Continuation of #7899
achow101 force-pushed on Apr 27, 2016

Create signmessagewithprivkey rpc

New rpc 'signmessagewithprivkey' which takes a private key to sign a message without using the wallet.

f90efbfeef

in src/rpc/misc.cpp:None in 12c5fb94c3 outdated

 370 | +{
 371 | +    if (fHelp || params.size() != 2)
 372 | +        throw runtime_error(
 373 | +            "signmessage \"privkey\" \"message\"\n"
 374 | +            "\nSign a message with the private key of an address"
 375 | +            + HelpRequiringPassphrase() + "\n"

paveljanik commented at 5:07 AM on April 27, 2016:

You do not need HelpRequiringPassphrase here. It is unavailable in no-wallet builds (see https://travis-ci.org/bitcoin/bitcoin/jobs/126001935#L1864).

achow101 commented at 11:37 AM on April 27, 2016:

Fixed

in src/rpc/misc.cpp:None in 12c5fb94c3 outdated

 365 | @@ -366,6 +366,49 @@ UniValue verifymessage(const UniValue& params, bool fHelp)
 366 |      return (pubkey.GetID() == keyID);
 367 |  }
 368 |  
 369 | +UniValue signmessagewithprivkey(const UniValue& params, bool fHelp)
 370 | +{
 371 | +    if (fHelp || params.size() != 2)
 372 | +        throw runtime_error(
 373 | +            "signmessage \"privkey\" \"message\"\n"

paveljanik commented at 5:17 AM on April 27, 2016:

This has to be signmessagewithprivkey instead of signmessage.

achow101 commented at 11:37 AM on April 27, 2016:

Fixed

achow101 force-pushed on Apr 27, 2016
laanwj added the label RPC/REST/ZMQ on Apr 27, 2016
laanwj commented at 12:00 PM on April 27, 2016: member

utACK https://github.com/bitcoin/bitcoin/pull/7953/commits/f90efbfeef9cd733aa7590fdf581ff82b999c4b5
in src/rpc/misc.cpp:None in f90efbfeef outdated
```
 404 | +    vector<unsigned char> vchSig;
 405 | +    if (!key.SignCompact(ss.GetHash(), vchSig))
 406 | +        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Sign failed");
 407 | +
 408 | +    return EncodeBase64(&vchSig[0], vchSig.size());
 409 | +}
```
jonasschnelli commented at 12:05 PM on April 27, 2016:

IMO strPrivkey, vchSecret and params[0] should be mem-cleansed before the function returns. I guess key RAII -cleans itself.
laanwj commented at 12:50 PM on April 27, 2016:

vchSecret is ok - CBitcoinSecret (derived from CBase58Data) uses a zero_after_free allocator

strPrivKey could be a SecureString, I guess.

However, params[] is a const parameter vector passed in, provided by a third-party library. I don't think this issue can be solved in general (except by never passing private keys in the API in the first place). The http server, JSON parsing, the RPC mechanism leaves the parameters all over memory. We have a similar problem in importprivkey. No need to solve in this pull.
jonasschnelli commented at 12:54 PM on April 27, 2016:

Agree. Its just a nitpick (I forgot to pre-fix it with "nit:"). As long as we are dealing with privatekeys in bitcoin-core there will always be memory containing sensitive stuff.
jonasschnelli commented at 12:06 PM on April 27, 2016: contributor

utACK f90efbfeef9cd733aa7590fdf581ff82b999c4b5
paveljanik commented at 12:17 PM on April 27, 2016: contributor

ACK https://github.com/bitcoin/bitcoin/commit/f90efbfeef9cd733aa7590fdf581ff82b999c4b5 @jonasschnelli Right. We should also recommend using -stdin for this when document this new RPC in release notes.
paveljanik commented at 3:58 PM on April 27, 2016: contributor

Can you please write some test for this new RPC?
achow101 commented at 7:20 PM on April 27, 2016: member

@paveljanik I've added a test. I think I did it correctly.
in qa/rpc-tests/signmessages.py:None in 69e826378f outdated
```
  25 | +        privKey = 'cUeKHd5orzT3mz8P9pxyREHfsWtVfgsfDjiZZBcjUBAaGk1BTj7N'
  26 | +        address = 'mpLQjfK79b7CCV4VMJWEWAj5Mpx8Up5zxB'
  27 | +        try:
  28 | +            signature = self.nodes[0].signmessagewithprivkey(privKey, message)
  29 | +        except JSONRPCException as e:
  30 | +            assert("Invalid private key" not in e.error['message'])
```
MarcoFalke commented at 7:24 PM on April 27, 2016:

All of those seem dead code, so you might as well remove them?
MarcoFalke commented at 7:28 PM on April 27, 2016:

What I mean is: If you don't expect an exception, don't catch it. The whole purpose of the test framework is to fail on any unexpected exception.

Other than that, you may want to try this syntax:

try: something_that_fails() assert(False) except: assert(thing_on_failure())

or

try: something_that_fails() except: assert(thing_on_failure()) else: assert(False)
achow101 commented at 7:30 PM on April 27, 2016:

Ok, I'll remove them. I should probably add a failure condition, right?

MarcoFalke commented at 1:12 PM on April 28, 2016:

Right, if you feel like adding a case that should fail to check if it fails properly, you are very welcome to do so.
achow101 force-pushed on Apr 27, 2016

in qa/rpc-tests/signmessages.py:None in 6220f3fb9e outdated

  32 | +        # Test the signing with an address with wallet
  33 | +        address = self.nodes[0].getnewaddress()
  34 | +        signature = self.nodes[0].signmessage(address, message)
  35 | +
  36 | +        # Verify the message
  37 | +        self.nodes[0].verifymessage(address, signature, message)

laanwj commented at 1:03 PM on April 28, 2016:

You should assert the result from verifymessage

achow101 commented at 1:32 PM on April 28, 2016:

done

achow101 force-pushed on Apr 28, 2016
achow101 force-pushed on Apr 28, 2016
achow101 force-pushed on Apr 28, 2016

Test for signing messages

New rpc test for signing and verifying messages.

7db0ecb90c

jonasschnelli commented at 12:44 PM on April 29, 2016: contributor

Tested ACK. Binaries: https://bitcoin.jonasschnelli.ch/pulls/7953/
laanwj merged this on May 5, 2016
laanwj closed this on May 5, 2016
laanwj referenced this in commit 0630353323 on May 5, 2016
achow101 deleted the branch on Oct 29, 2016
codablock referenced this in commit 8af9840ad5 on Sep 16, 2017
codablock referenced this in commit 6f5f195876 on Sep 19, 2017
codablock referenced this in commit 90b00cfc66 on Dec 21, 2017
DrahtBot locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#7678 TODO for release notes 0.13.0 #7899 optionally provide privkey to signmessage RPC