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

feat!: remove default Schnorr functionality #180

Closed

Conversation

AaronFeickert
Copy link
Contributor

@AaronFeickert AaronFeickert commented May 24, 2023

Removes default Schnorr signature functionality by requiring domain separation. Closes #178.

Previously, the Schnorr signature API permitted the use of a default domain separator. This introduces significant risk, as reuse of signatures across contexts can be dangerous. Further, as there are no widely-used standards for Schnorr-type signatures over the Ristretto group, there is no particularly good choice for such a domain separator.

This PR does several things:

  • requires that SchnorrSignature be provided with a domain separator via trait bound, which was previously optional
  • removes RistrettoSchnorrWithDomain
  • redefines RistrettoSchnorr to RistrettoSchnorr<H> to require a domain separator

It also removes all existing WASM and FFI signing and verification functionality for Schnorr signatures, since this previously used the default domain separator and there is no way to make them properly generic. Applications needing Schnorr signatures using WASM or FFI should define specific functions that use the updated types to provide a domain separator.

BREAKING CHANGE: Makes several type changes to SchnorrSignature, RistrettoSchnorr, and RistrettoSchnorrWithDomain that are breaking.

Copy link
Contributor

@SWvheerden SWvheerden left a comment

Choose a reason for hiding this comment

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

Looks good

hansieodendaal
hansieodendaal previously approved these changes May 26, 2023
Copy link
Contributor

@hansieodendaal hansieodendaal left a comment

Choose a reason for hiding this comment

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

I think the changes are good, and agree that the domain should be enforced. I just have some nits about the readability in the test examples, shown below.

ACK

src/ristretto/ristretto_sig.rs Outdated Show resolved Hide resolved
src/ristretto/ristretto_sig.rs Outdated Show resolved Hide resolved
src/ristretto/ristretto_sig.rs Outdated Show resolved Hide resolved
src/ristretto/ristretto_sig.rs Outdated Show resolved Hide resolved
src/ristretto/ristretto_sig.rs Outdated Show resolved Hide resolved
src/ristretto/ristretto_sig.rs Show resolved Hide resolved
src/ristretto/ristretto_sig.rs Show resolved Hide resolved
src/ristretto/ristretto_sig.rs Outdated Show resolved Hide resolved
@CjS77
Copy link
Contributor

CjS77 commented May 26, 2023

I disagree here. As mentioned out in #178, I feel that API ergonomics is important. Newcomers to the library are going to be confused with an API requiring Domain separation and will use a default anyway. Those looking for it will see it in the function signature and will be able to provide their own domains.

@CjS77
Copy link
Contributor

CjS77 commented May 26, 2023

A lot of tests have been removed -- is there a reason for the decrease in coverage?

Copy link
Contributor

@CjS77 CjS77 left a comment

Choose a reason for hiding this comment

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

Need consensus before this can be merged.

@AaronFeickert
Copy link
Contributor Author

I disagree here. As mentioned out in #178, I feel that API ergonomics is important. Newcomers to the library are going to be confused with an API requiring Domain separation and will use a default anyway. Those looking for it will see it in the function signature and will be able to provide their own domains.

I think implementation snafus like this are indicative of the risk of providing "a Schnorr signature". The implementer is always free to ignore the recommendation of distinct domain separation, but this API change at least forces them to make a decision. Coupled with the fact that there is no "default" signature by any common standard, I think it's an annoyance whose safety benefit outweighs the inconvenience.

I understand where you're coming from, but respectfully disagree with the conclusion. Frankly, an implementer who is confused by domain separation should probably take a moment to investigate first.

@AaronFeickert
Copy link
Contributor Author

A lot of tests have been removed -- is there a reason for the decrease in coverage?

Some were specific to FFI or WASM, and naturally went away. An "invalid scalar" test didn't really make sense, and was nixed. A couple of other tests dealt specifically with the default separator, and are no longer relevant or were modified accordingly.

@AaronFeickert
Copy link
Contributor Author

One point to keep in mind is that "raw" signatures, in which the caller is responsible for challenge generation, have no inherent notion of domain separation. Currently, it's probably easiest simply to use the default separator type, since it plays no role in generating the Fiat-Shamir challenge. These are used (for good reason) in several places within the tari codebase,

@AaronFeickert AaronFeickert force-pushed the no-morr-default-schnorr branch from 3f589c1 to 3c0a3dd Compare June 6, 2023 20:40
@AaronFeickert AaronFeickert marked this pull request as draft June 7, 2023 00:24
@AaronFeickert AaronFeickert deleted the no-morr-default-schnorr branch March 23, 2024 20:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enforce domain separation on Schnorr signatures
5 participants