Sign and verify arbitrary messages with wallet keys - #2467
Conversation
|
Full suite on this branch: Two notes on CI, in case it comes up red here as it did on my other PR:
|
jozanek
left a comment
There was a problem hiding this comment.
A few minor polish items — nothing blocking. Five inline comments below, plus one that spans two test files:
Route-test gaps (one-liners each):
WalletApiRouteSpec: no test for a request missing themessagefield (themessageSigningRequestrejection path);UtilsApiRouteSpec: no test for a non-P2PK address on the verify side (theLeft("address: not a P2PK address, ...")branch is untested).
| } | ||
|
|
||
| def signMessageR: Route = (path("signMessage") & post & messageSigningRequest) { case (message, addressOpt) => | ||
| withWalletOp(_.signMessage(message.getBytes(org.ergoplatform.settings.Constants.StringEncoding), addressOpt)) { |
There was a problem hiding this comment.
Inlining the fully-qualified org.ergoplatform.settings.Constants.StringEncoding here reads awkwardly — it's forced by the existing org.ergoplatform.wallet.Constants import, so an alias would clean it up:
import org.ergoplatform.settings.{Constants => NodeConstants}Also, a few of the new lines here (and in ErgoUtilsApiRoute.verifyMessageR) run past scalafmt's maxColumn = 90. Not CI-enforced and the surrounding code already violates it, but since the hunks are new, worth a formatting pass.
There was a problem hiding this comment.
Done in 82cce94. I split it into two import lines rather than one, because the single-line form is itself 92 columns:
import org.ergoplatform.settings.{Constants => NodeConstants}
import org.ergoplatform.settings.{ErgoSettings, RESTApiSettings}Formatting pass done on this hunk and on verifyMessageR. In verifyMessageR the shortest way under 90 turned out to be pulling the two repeated .left.map(...) shapes into local field / base16 helpers rather than rewrapping each line.
Code lines in the new hunks are now all under 90. A few scaladoc prose lines sit at 91-93: scalafmt 2.3.2 does not reflow comments, and there is no scalafmt plugin in the build so there is no scalafmtCheck task to satisfy either. Happy to hard-wrap those too if you would rather have them uniform.
|
|
||
| /** The byte string actually signed when `message` is signed with `salt` */ | ||
| def wrap(message: Array[Byte], salt: Array[Byte]): Array[Byte] = { | ||
| require(salt.length == SaltLength, s"Salt must be $SaltLength bytes long, got ${salt.length}") |
There was a problem hiding this comment.
wrap throws via require from published library code, which goes against the repo's "avoid throwing in library code" guideline. It's a programmer-error precondition (the node always passes freshSalt()), so I don't think it needs to become a Try — but since ergo-wallet is consumed externally, an explicit @throws[IllegalArgumentException] scaladoc note would make the contract visible to library users.
There was a problem hiding this comment.
Done in c08ef13 — kept as a require, since it is a programmer-error precondition as you say, and made the contract visible:
/**
* ...
* The salt is a precondition rather than something to recover from: callers are expected
* to get it from [[freshSalt]], and a salt of the wrong length would silently change what
* [[unwrap]] reads back.
*
* @throws IllegalArgumentException if `salt` is not [[SaltLength]] bytes long
*/
@throws[IllegalArgumentException]
def wrap(message: Array[Byte], salt: Array[Byte]): Array[Byte] = {While in the file I also rewrapped it to 90 columns, since all of it is new.
| Failure(new Exception(s"The wallet has no secret for $what")) | ||
| } | ||
| case None => | ||
| Failure(new Exception("Wallet is locked")) |
There was a problem hiding this comment.
This branch reports "Wallet is locked", but proverOpt is also None when the wallet was never initialized, so an uninitialized-wallet user gets a misleading 400. Suggest: "Wallet is locked or not initialized".
There was a problem hiding this comment.
Good catch, done in 7554d52:
case None =>
// no prover means either a locked wallet or one which was never initialized
Failure(new Exception("Wallet is locked or not initialized"))| proof: | ||
| description: Base16-encoded signature | ||
| type: string | ||
| example: 'cd07f00d80cd0c9a0d16f9dbba1b4ff5d0e0aa5cb2b1e0ff1e6c0b3f9e4a1a4c' |
There was a problem hiding this comment.
The proof example is 32 bytes of hex, but a real single-ProveDlog signature is 56 bytes. Client authors sometimes size fields from examples — a realistic 56-byte example (e.g. one generated by an actual signMessage call) would prevent that.
There was a problem hiding this comment.
Done in 472c524. The signedMessage example above it was wrong in the same way — it was only the 21-byte prefix, with neither salt nor payload — so rather than hand-write either, I took a matched pair from an actual signMessage call:
signedMessageis 83 bytes:4572676f207369676e6564206d6573736167653a0a("Ergo signed message:\n") ++ 32 bytes of salt ++ the 30 bytes of the example messageproofis the 56-byte signature over exactly those bytes
So the two examples round-trip through /utils/verifyMessage instead of just being the right length. I put the same pair on MessageVerificationRequest, which had no examples at all, and noted the 56 bytes in the proof description.
| { case (p2pk, signedMessage, proof) => | ||
| val isValid = MessageSigning.verify(p2pk.pubkey, signedMessage, proof) | ||
| val message = MessageSigning.unwrap(signedMessage) | ||
| .map(bytes => new String(bytes, Constants.StringEncoding).asJson) |
There was a problem hiding this comment.
unwrap-ed payload bytes are decoded as UTF-8 unconditionally here. For node-produced signatures that's always correct, but a signature produced by an external tool over non-UTF-8 payload bytes would return isValid: true with replacement characters in message. Harmless, just worth a sentence in the OpenAPI description: message is the UTF-8 decoding of the signed payload, and may be lossy if the payload wasn't UTF-8 text.
There was a problem hiding this comment.
Agreed, documented in 472c524. The message description now reads:
The message the signed byte string carries, or null when the byte string is not wrapped for message signing at all. It is the UTF-8 decoding of the signed payload, so a payload which is not UTF-8 text - which a signer following this convention does not produce, but an external one might - comes back lossily decoded. Check
signedMessageitself if the exact bytes matter.
I left a two-line comment at the decoding site too, so the next reader does not have to go to the OpenAPI file to find out why it is unconditional.
isValid is unaffected either way: it is computed over the raw bytes, not over the decoded string.
|
Thanks — all six addressed, replies inline on each thread. The two route-test gaps are covered in b0464e3:
I checked both actually bite rather than trusting that they do: making the missing-message case
|
Closes #1392.
Two endpoints, so that the holder of an address can be proven without moving funds:
POST /wallet/signMessage— needs an unlocked wallet, optionally takes an address of that wallet. Returns the address, the byte string actually signed, and the signature.POST /utils/verifyMessage— needs neither wallet nor blockchain state, so anyone can check a signature. Returns whether it is valid and the message it attests to.Schnorr, not ECDSA
The issue asks for ECDSA. I went with the sigma protocol the node already uses, and would rather argue the point than quietly do the other thing.
ProverInterpreter.signMessageandInterpreter.verifySignatureare already in sigma-state, which the node depends on, and they are the same primitive as a transaction input proof. Adding ECDSA would put a second signature scheme in the node for one feature, and nothing else in the ecosystem - sigma-rust, appkit, wallets - would be able to check the result. On the "k" reuse the issue worries about, and RFC 6979: each sigma proof draws a fresh commitment from a secure source, so the failure mode does not arise.What is signed, and why it is not the message
This is the part most worth a second pair of eyes.
A sigma proof over an arbitrary byte string is the proof which spends a box - both are a Fiat-Shamir transcript over some bytes. If the node signed the caller's bytes verbatim, a caller could hand it the
messageToSignof a transaction spending the wallet's own boxes and receive a proof that makes that transaction valid. So the node signswith 32 fresh random bytes of salt, and returns the byte string it signed. The prefix says what the transcript is for; the salt makes the signed string unpredictable to whoever supplied the message, so it cannot be steered onto a chosen byte string. This is what EIP-0028 (ErgoAuth) already prescribes for wallet applications - the wallet adds its own bytes and reports back what it signed - so a node doing the same keeps the two consistent.
Verification insists on the wrapping too. Accepting an unwrapped byte string would give the separation away, since a transaction input proof would then pass as a message signature.
The convention lives in
ergo-wallet(org.ergoplatform.wallet.crypto.MessageSigning) rather than in the node's API layer, because signer and verifier have to agree on it andergo-walletis the published library other tools use.Tests
ErgoWalletServiceSpec— a signature verifies for its address and for nothing else (other key, altered byte string, other proof all rejected); signing twice gives two different byte strings, both valid; signing needs an unlocked wallet holding the address asked for.ErgoWalletServiceSpec— the attack, defeated: a real transaction is built and signed, itsmessageToSignis then handed tosignMessagedressed up as a message, and the resulting proof does not verify against the transaction. The other direction is covered too: the transaction's own input proof does not pass as a message signature.UtilsApiRouteSpec— the verify route: valid, wrong address, unwrapped byte string, and a request which does not parse.WalletApiRouteSpec— the sign route: default address, chosen address, and a non-P2PK address refused. The stub wallet actor signs for real, so the route test checks a genuine signature.Notes
/scriptor elsewhere if you prefer.signMessagetakes text and encodes it as UTF-8, matching/utils/hash/blake2bwhich also takes a plain string. If a hex-bytes variant is wanted for binary payloads it is a small addition.ergo-wallet, which cross-compiles on 2.11, 2.12 and 2.13; nothing there uses anything newer.