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

`signMultisigParticipant` & `verifyWithPublicKey` functions added into wallet api #3598

Merged
merged 1 commit into from Jun 8, 2018

Conversation

4 participants
@naughtyfox
Contributor

naughtyfox commented Apr 10, 2018

wallet2::sign function can't work in multisignature wallets

@moneromooo-monero

This generates invalid signatures (assuming multisig is not completely broken).

@naughtyfox

This comment has been minimized.

Contributor

naughtyfox commented Apr 11, 2018

@moneromooo-monero what do you mean? i've checked it.

@moneromooo-monero

This comment has been minimized.

Contributor

moneromooo-monero commented Apr 11, 2018

Your change uses a single key, which is obviously wrong for a multisignature wallet, no ? Am I missing somehting ?

@naughtyfox

This comment has been minimized.

Contributor

naughtyfox commented Apr 11, 2018

In case of multisig we can't use public spend key because we don't have full private spend. But what we do can is to use signer key + private partial spend (Signer = b_partial * G) which is cryptographic dependent.

In multisig wallet the only way to identify the wallet participant is signer key. Hence we can sign message with signer public key + private partial spend and verify only by signer public key.

What i do miss is to make verify function that accepts signer key to verify signature.
Please let me know if you agree with my arguments to let me work further on this feature.

@moneromooo-monero

This comment has been minimized.

Contributor

moneromooo-monero commented Apr 11, 2018

That's pointless, since any one participant will be able to sign something, which defeats the point. Wait for luigi to come up with the right way. I asked long ago, but he's always busy so might take some more time.

@naughtyfox

This comment has been minimized.

Contributor

naughtyfox commented Apr 11, 2018

since any one participant will be able to sign something, which defeats the point

I got what you mean. I assume we sign message with certain multisig wallet participant's personal signature (signatures from different participants are not equal) and you think a message is signed from entire wallet perspective.
We need to define what the wallet signature is - personal participant's or entire wallet's.

@moneromooo-monero

This comment has been minimized.

Contributor

moneromooo-monero commented Apr 11, 2018

Obviously (IMHO) a number of signers equal to the multisig threshold.

@stoffu

This comment has been minimized.

Contributor

stoffu commented Apr 13, 2018

What use case do you have in mind? Why is this needed?

TBH I've been finding the existing sign/verify feature not particularly useful either; it doesn't seem much more useful than popular signature tools like GPG. I'm happy to be educated if I'm missing some important use cases.

@moneromooo-monero

This comment has been minimized.

Contributor

moneromooo-monero commented Apr 13, 2018

I bvelieve onishin wanted to use this to allow miners to set persistent config (such as payout threshold). This depends on just the public identity being the address, and no further information is needed. Instructions can then be checked to have been signed with the address' controller.

@naughtyfox

This comment has been minimized.

Contributor

naughtyfox commented Apr 13, 2018

What use case do you have in mind? Why is this needed?

We assume to use personal wallet participant signature as part of authentication facilities in our multisig service

@moneromooo-monero

This comment has been minimized.

Contributor

moneromooo-monero commented Apr 13, 2018

If you need this (signing by any one participant), then it can be added as another command, which is multisig specific.

@stoffu

This comment has been minimized.

Contributor

stoffu commented Apr 13, 2018

Could you provide a little more detail about the multisig procedure you’re developing? I still don’t see where and why participants would want/need to sign arbitrary messages (other than the exchanged key info).

@naughtyfox

This comment has been minimized.

Contributor

naughtyfox commented Apr 13, 2018

The service provides ability to exchange keys and partial images between multisig participants. And the service needs to identify certain participant somehow. Obviously the most natural way to do this is to use public signer key (that's what i made previous PR #3599 for). And in order to build authentication system upon signer public key we have to prove the sender has corresponding private key - that's why we need this kind of signature.

@moneromooo-monero

This comment has been minimized.

Contributor

moneromooo-monero commented Apr 13, 2018

The export files are already signed.

@naughtyfox

This comment has been minimized.

Contributor

naughtyfox commented Apr 13, 2018

yes, and we want to sign this way our custom messages in service communicating protocol. For example:

{
 “destination_address”: “44AFFq5kSiGBoZ4NMDwYtN18obc8AemS33DBLWs3H7otXft3XjrpDtQGv7SqSsaBYBb98uNbr2VBBEt7f2wfn3RVGQBEP3A”,
 “description”: “monero team donation”,
 “signed_transaction”: “xxxxxxxxxxx”,
 “proposal_id”: 1312,
 “amount”: 1.2352,
 “fee”: 0.378
}
@moneromooo-monero

This comment has been minimized.

Contributor

moneromooo-monero commented Apr 13, 2018

Are you OK with a multisig specific call for this ? I think using the existing sign functionality is dangerous since it would create an expectation that the threshold was met (and there will probably be some code to do this later).

@naughtyfox

This comment has been minimized.

Contributor

naughtyfox commented Apr 13, 2018

When i was implementing this feature i assumed this function works as personal signature. Probably you r right and it's worth to be implemented as separate library and not to be a part of monero...

@moneromooo-monero

This comment has been minimized.

Contributor

moneromooo-monero commented Apr 13, 2018

That is not what I said though.

@stoffu

This comment has been minimized.

Contributor

stoffu commented Apr 14, 2018

I still don’t understand the need for signing arbitrary messages by multisig participants, because the fact that a multisig tx is partially signed by the participants already means that the tx is genuinely approved by the participants.

Can you explain what kind of forgery is possible without this ability?

@naughtyfox

This comment has been minimized.

Contributor

naughtyfox commented Apr 16, 2018

We want to authenticate our users with this signature (+ public signer key) and be sure the message remains unchanged. A message may not contain signed by default data such as transaction or multisig info, it may contain only service data like "synchronization status" or "i agree to sign transaction proposal".

For example, a participant sends message "my synchronization status x/y", adds his signer public key to HTTP header and signs the message with his private signer key. This way the service will it's participant Alice and the message hasn't been changed by third parties.

PS Sorry for late answer

@stoffu

This comment has been minimized.

Contributor

stoffu commented Apr 16, 2018

Thanks for explaining, I'm now OK with adding such a signature tool. However, as it stands now, the change in the sign code will confuse the verify code because the public key against which this signature is to be checked is no longer the wallet public address, but the multisig signer's public key. I.e. one can no longer verify the signature in simplewallet as:

verify <filename> <address> <signature

As such, some more coding is necessary, e.g. introducing a new variant of sign/verify specifically for multisig.

@naughtyfox

This comment has been minimized.

Contributor

naughtyfox commented Apr 16, 2018

Thank you.
I'll add new member functions signMultisigParticipant and verifyMultisigParticipant and renew the PR. Are you ok with these names?

@moneromooo-monero

This comment has been minimized.

Contributor

moneromooo-monero commented Apr 16, 2018

tbh I'd rather you'd keep to the underscore type naming for the wallet2 part (for the GUI it uses that kind of naming).

@naughtyfox naughtyfox force-pushed the naughtyfox:fix-sign-function branch from 989973d to eadeae4 Apr 23, 2018

@naughtyfox naughtyfox changed the title from wallet2::sign fixed to work with multisignature wallets to `signMultisigParticipant` & `verifyWithPublicKey` functions added into wallet api Apr 23, 2018

@naughtyfox naughtyfox force-pushed the naughtyfox:fix-sign-function branch from eadeae4 to fcd01f5 Apr 23, 2018

@naughtyfox

This comment has been minimized.

Contributor

naughtyfox commented Apr 23, 2018

@stoffu @moneromooo-monero Hey guys! I have updated this pull request so please check this out.

When i was implementing verify signature function i decided that it'd be better if it was more common and called it verifyMessageWithPublicKey because we pass a public key either way.
I introduced new magic for this signature - SigPkV1 because despite signature algorithm remains the same we use different keys and i'd like to let client know if he's using wrong verification function.
I checked it works (i had to merge the branch with my previous pull request locally).

@stoffu

stoffu approved these changes Apr 24, 2018

@@ -115,6 +115,8 @@ using namespace cryptonote;
#define STAGENET_SEGREGATION_FORK_HEIGHT 1000000
#define SEGREGATION_FORK_VICINITY 1500 /* blocks */
static const std::string PUB_KEY_SIGNATURE_MAGIC = "SigPkV1";

This comment has been minimized.

@moneromooo-monero

moneromooo-monero Apr 24, 2018

Contributor

How about making this have multisig in the name (or some shortening thereof) for clarity ?

This comment has been minimized.

@naughtyfox

naughtyfox Apr 24, 2018

Contributor

I thought about it, but verification function accepts public key so it potentially maybe a common function and i didn't want to bind it to mulitisig only

This comment has been minimized.

@naughtyfox

naughtyfox Apr 24, 2018

Contributor

Or maybe you mean variable name (not the magic itself)? If so probably it worth to do it...

This comment has been minimized.

@moneromooo-monero

moneromooo-monero Apr 25, 2018

Contributor

I meant the magic. This is multisig specific, and a multisig wallet may be able to sign with proper threshold in the future.

This comment has been minimized.

@naughtyfox

naughtyfox Apr 25, 2018

Contributor

Okay, will do it in a day

bool wallet2::verify_with_public_key(const std::string &data, const crypto::public_key &public_key, const std::string &signature) const
{
if (signature.size() < PUB_KEY_SIGNATURE_MAGIC.size() || signature.substr(0, PUB_KEY_SIGNATURE_MAGIC.size()) != PUB_KEY_SIGNATURE_MAGIC) {
LOG_PRINT_L0("Signature header check error");

This comment has been minimized.

@moneromooo-monero

moneromooo-monero Apr 24, 2018

Contributor

I'd rather new logs use the severity based api (MERROR, MWARNING, etc) as L0 just asumes warning since it can't tell.

This comment has been minimized.

@naughtyfox

naughtyfox Apr 24, 2018

Contributor

valuable notice, will fix it

@naughtyfox naughtyfox force-pushed the naughtyfox:fix-sign-function branch from fcd01f5 to 015df80 Apr 25, 2018

Wallet: added methods to sign and verify arbitrary message with multi…
…sig public signer's key (libwallet & wallet api)

@naughtyfox naughtyfox force-pushed the naughtyfox:fix-sign-function branch from 015df80 to b21bc00 Apr 25, 2018

@naughtyfox

This comment has been minimized.

Contributor

naughtyfox commented Apr 25, 2018

@moneromooo-monero check this out please, i fixed signature magic

@luigi1111 luigi1111 merged commit b21bc00 into monero-project:master Jun 8, 2018

6 of 8 checks passed

buildbot/monero-static-win32 Build done.
Details
buildbot/monero-static-osx-10.12 Build done.
Details
buildbot/monero-linux-armv7 Build done.
Details
buildbot/monero-static-osx-10.11 Build done.
Details
buildbot/monero-static-osx-10.13 Build done.
Details
buildbot/monero-static-ubuntu-amd64 Build done.
Details
buildbot/monero-static-ubuntu-i686 Build done.
Details
buildbot/monero-static-win64 Build done.
Details

luigi1111 added a commit that referenced this pull request Jun 8, 2018

Merge pull request #3598
b21bc00 Wallet: added methods to sign and verify arbitrary message with multisig public signer's key (libwallet & wallet api) (naughtyfox)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment