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

Add signfile/verifyfile RPC calls. #1957

Closed
wants to merge 2 commits into from
Closed

Add signfile/verifyfile RPC calls. #1957

wants to merge 2 commits into from

Conversation

dparrish
Copy link

These calls can be used to generate and check signatures for arbitrary
(potentially binary) files, much like signmessage / verifymessage.

The client should hash the file locally then send a uint256 in hex form
as the 2rd parameter to signfile and the 3rd parameter to verifyfile.

The other option would have been for the client to hash the file locally and send it to the server using the signmessage / verifymessage calls, but then the server would be signing a hash of a hash.

I'm new to the bitcoin code, so if there's a better way of doing this, let me know.

The functionality has been verified by one other person on IRC but it still could be wrong.

These calls can be used to generate and check signatures for arbitrary
(potentially binary) files, much like signmessage/verifymessage.

The client should hash the file locally then send a uint256 in hex form
as the 2rd parameter to signfile and the 3rd parameter to verifyfile.
@NaruFGT
Copy link

NaruFGT commented Oct 25, 2012

I'd love to see this functionality in the mainstream client.
Does anybody know the history of the uint256 structure? I've never heard of that type of hash. I know it's used in signmessage and verifymessage but options for other hash types (possibly enumerated with another argument to sign/verify) might help more people trust the signatures.

ifstream file(value.get_str().c_str(), ios::in | ios::binary | ios::ate);
if (!file.is_open())
throw runtime_error(string("Can't read input file ") + value.get_str());
int size = file.tellg();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens, if someone tries to feed a >2GB file to this :)?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was an oversight, I'll make this read chunks and hash as it goes.

@laanwj
Copy link
Member

laanwj commented Oct 25, 2012

I'm not against this functionality per-ce, but I don't like to have it in the core. We already suffer from some feature bloat.

I suggest implementing it as an external tool that hashes your file, then signs the resulting hash using the existing signmessage call. This keeps everything nice and contained.

@NaruFGT
Copy link

NaruFGT commented Oct 25, 2012

It's a core feature and would benefit many transactions, with a fix on file size limitation (loading the entire file into ram isn't good for performance and scalability) this feature really deserves a position in the main branch.
An external tool will lead to lack of addoption and frankly, this feature is far more critical in corperate and business enviroments than a message signature.
Feature bloat is a problem but this isn't alot to maintain.

@laanwj
Copy link
Member

laanwj commented Oct 25, 2012

It's easy to hash a file in a streaming fashion, you certainly don't need to read it into ram.

Also you don't need new RPC server calls for this. You can use the existing signmessage/verifymessage RPCs with the hash of the file (and maybe some added salt) as message. You could even generalize it to multiple files by signing a "manifest" (similar to how JAR files are signed). As an added bonus, you can choose any hash function you want to hash the files. This can all be implemented as client-only feature.

We have the mantra that everything that can be done outside the core, should be done outside the core. The discussion isn't about whether this feature deserves including or not!

an external tool will lead to lack of adoption that depends on how you market it. If you think it will fly in "corporate and business environments" then my advice is to try to sell it for big $$$ 💸

@NaruFGT
Copy link

NaruFGT commented Oct 25, 2012

"Also you don't need new RPC server calls for this."
We actually do, I personally won't trust a hash of a hash. Overloading the signmessage/verifymessage to have an option would be fine, but the feature has to implement the signature hash as a direct hash of the file.

The signmessage/verifymessage can be done outside of the core as well. This feature needs to be included into the core because there needs to be a uniform RPC call defined that other bitcoin clients will adopt.

I don't really know why an additional RPC call is met by a fear of the code maintanance overhead. This is pretty much just a wrapper function for signmessage. It's soo easy to maintain that I can't imagine the justification for NOT including it o.o If it was a security risk, or posed more maintainance, bloat, or other such problems I would understand, but on pius principle just isn't a valid reason to soo swiftly discount the inclusion.

This isn't controversial, why is it being made so?

@sipa
Copy link
Member

sipa commented Oct 25, 2012

There is no security difference between using a hash, or using a hash of a hash (there's at most a 0.66 bit decrease in entropy, which makes brute-forcing a collision about 1.26 times faster on average). Given the fact that we're already using double-SHA256 for POW and blockids and txids, you should start worrying about Bitcoin's crypto in general then too.

@NaruFGT
Copy link

NaruFGT commented Oct 25, 2012

sipa do you have details about the signmessage/verifymessage uint256 structure and the hash function?

Really the point is that it's a messy implementation, and adds calculation overhead. It can and should be avoided, unless there's a reason we should use hash of a hash.

@sipa
Copy link
Member

sipa commented Oct 25, 2012

The hash function is double-SHA256("Bitcoin signed message: " + message). The result of this is stored in a uint256 data type (and unsigned 256-bit integer). It is signed using ECDSA, encoded in a 65-byte structure that allows recovery of the public key. This 65-byte structure is then encoded in base64.

@robbak
Copy link
Contributor

robbak commented Oct 25, 2012

On 25 October 2012 18:36, NaruFGT notifications@github.com wrote:

"Also you don't need new RPC server calls for this."
We actually do, I personally won't trust a hash of a hash. Overloading the
signmessage/verifymessage to have an option would be fine, but the feature
has to implement the signature hash as a direct hash of the file.

The signmessage/verifymessage can be done outside of the core as well.
This feature needs to be included into the core because there needs to be a
uniform RPC call defined that other bitcoin clients will adopt.

I don't really know why an additional RPC call is met by a fear of the
code maintanance overhead. This is pretty much just a wrapper function for
signmessage. It's soo easy to maintain that I can't imagine the
justification for NOT including it o.o If it was a security risk, or posed
more maintainance, bloat, or other such problems I would understand, but on
pius principle just isn't a valid reason to soo swiftly discount the
inclusion.

This isn't controversial, why is it being made so?


Reply to this email directly or view it on GitHubhttps://github.com//pull/1957#issuecomment-9769860.

When I saw all these message signing functions in the bitcoin client, the
first thing I thought is "What is this doing in here?" If one wants to sign
and authenticate message, well, there is PGP for just that.

The fact that there is anything of this kind in there seems like kruft.
IMHO, it should be reduced or removed, not increased and expanded.
If anything could be added, it is a simple "signhash" RPC. Then all this
message and digest signing can be removed from core.

Or am I treading all over somone's barely healed toes?

@laanwj
Copy link
Member

laanwj commented Oct 25, 2012

In addition to that, the whole Merkle tree concept used throughout bitcoin is based on hashes of hashes (of hashes...). Also many code signing approaches rely on hashes of (groups of) hashes, such as the JAR signing I mentioned. Not trusting hashes of hashes is quite irrational. That's ok, we're all paranoid in our own way, but I do recommend learning somewhat more about how Bitcoin works before making judgements on relative security merits of different approaches.

@sipa
Copy link
Member

sipa commented Oct 25, 2012

@robbak The reason for this method was having the ability to sign messages with keys corresponding to Bitcoin addresses, so one can prove ownership of coins, or prove being the sender of a transaction.

I do agree it doesn't need to do more than what it can already - it just has to core functionality, other use cases can be implemented on top of it.

@dparrish
Copy link
Author

This is why I asked if this was the best way! Discussion is good.

I'd still really like to see a "standardized" way of signing files with my bitcoin key, but I agree the API doesn't really need to be any bigger than it is. How about keeping "signfile" and "verifyfile" as client-side calls that generate a hash of the file that's sent to the server for re-hashing and signing. I think something like this belongs in this client, as it's basically the reference implementation of bitcoin, there's not much point signing files in a different way in each different client.

@NaruFGT
Copy link

NaruFGT commented Oct 25, 2012

"It is signed using ECDSA, encoded in a 65-byte structure that allows recovery of the public key. This 65-byte structure is then encoded in base64."
In this case can a file signature be done in openssl simply using the address private key? That would be standard easy to adopt.

@sipa
Copy link
Member

sipa commented Oct 28, 2012

@NaruFGT sorry, there are no standard encodings that have all necessary data for public key recovery from a signature, so we had to invent our own.

@gavinandresen
Copy link
Contributor

NACK.

I agred that this is feature-creep.

I think this would be much better implemented as a little python or shell script shipped in the contrib/ directory that computed a hash for the file and then called signmessage.

@BitcoinPullTester
Copy link

Automatic sanity-testing: FAILED MERGE, see http://jenkins.bluematt.me/pull-tester/a7b0f3829e30d43632e2bf342631d011c6ddb270 for test log.

This pull does not merge cleanly onto current master

@jgarzik
Copy link
Contributor

jgarzik commented Nov 16, 2012

One explicit NAK, and two "OK but not in core" seeming NAKs. Closing.

Perhaps consider an external utility that does this; look a "test_bitcoin" for an example of a non-bitcoind binary that uses core bitcoin data structures.

@jgarzik jgarzik closed this Nov 16, 2012
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants