feat!: improve signature api #145
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
CjS77
force-pushed
the
sig_refactor
branch
2 times, most recently
from
a58a786 to
701f89c
Compare
There are some footguns in the `SchnorrSignature::sign` method that this PR seeks to mitigate. Firstly, it's not clear in the function docs that the nonce and pubkey are assumed to have been bound by the caller in the challenge. The docs are updated to make this clearer. Secondly, we deprecate the sign method and the utility module's sign method in favour of: - `sign_raw`, which is identical to the current sign method, but with a name that conveys the risk associated with using it. - `sign_message`, which does what many clients might have thought `sign` does, which correctly binds a nonce and public key to the message being signed in the challenge construction. - Matching `verify_*` methods. Tests and docs have been updated to reflect the changes.
The WASM and FFI library now use the domain-separated hashing algorithm to generate challenges for signatures.
The old API took ownership of the secret. This isn't necessary from an API point of view. It's more ergonomic to take a reference. The nonce still gives up ownerhip, since this should never be re-used.
AaronFeickert
requested changes
Nov 3, 2022
src/ffi/keys.rs
Outdated
| let msg = match CStr::from_ptr(msg).to_str() { | ||
| Ok(s) => s, | ||
| _ => return STR_CONV_ERR, | ||
| }; | ||
| let challenge = Blake256::digest(msg.as_bytes()); | ||
| let sig = match RistrettoSchnorr::sign(k, r, &challenge) { | ||
| let e = SchnorrSignature::construct_domain_separated_challenge::<_, Blake256>(&pub_r, &pubkey, msg.as_bytes()); |
There was a problem hiding this comment.
The docstring for this function on line 55 (can't make a review comment on that line directly) should be updated to be consistent (it was wrong before, too):
- The caller provides the private key and public nonce, not the private key and challenge
- The caller must never reuse a nonce with the same private key (and shouldn't reuse it in any other circumstance, since in that case something else has probably gone horribly wrong)
There was a problem hiding this comment.
The caller does not provide a nonce. The nonce argument is mutated to receive the public nonce value. I updated the docs to point this out and changed the argument name to be more specific.
In response to review comment
This commit adds support for providing custom domain separation tags to SchnorrSignature. This is done by including a 3rd generic type to the SchnorrSignature struct definition. The provision is optional. By default, the domain hash separator uses SchnorrSigChallenge as the domain separation tag. The `RsitrettoSchnorr` type alias is updated to include the default domain separation tag, to keep backward compatibility. To provide a custom hasher, update usages of `RistrettoSchnorr` t `RistrettoSchnorrWithDomain`
There were some small inconsistencies between the docstrings and method sigs in the ffi module. The `verify` function also had the sig and nonce being mutable, which is incorrect, since they're input parameters in this case.
Export `RistrettoSchnorrWithDomain` and add the missing docs clippy was complaining about
| /// ``` | ||
| pub type RistrettoSchnorr = SchnorrSignature<RistrettoPublicKey, RistrettoSecretKey>; | ||
| pub type RistrettoSchnorrWithDomain<H> = SchnorrSignature<RistrettoPublicKey, RistrettoSecretKey, H>; |
AaronFeickert
approved these changes
Nov 9, 2022
|
ACK |
This was referenced May 18, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There are some footguns in the
SchnorrSignature::signmethod that thisPR seeks to mitigate.
Firstly, it's not clear in the function docs that the nonce and pubkey
are assumed to have been bound by the caller in the challenge.
The docs are updated to make this clearer.
Secondly, we deprecate the sign method and the utility module's sign
method in favour of:
sign_raw, which is identical to the current sign method, but with aname that conveys the risk associated with using it.
sign_message, which does what many clients might have thoughtsigndoes, which correctly binds a nonce and public key to the message
being signed in the challenge construction.
verify_*methods.Tests and docs have been updated to reflect the changes.
The WASM and FFI library now use the domain-separated hashing algorithm
to generate challenges for signatures.