feat: improve signature api

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.

Cargo.toml

@@ -7,7 +7,7 @@ categories = ["cryptography"]
 homepage = "https://tari.com"
 readme = "README.md"
 license = "BSD-3-Clause"
-version = "0.15.7"
+version = "0.16.0"
 edition = "2018"
 
 [dependencies]

src/hashing.rs

@@ -520,6 +520,7 @@ pub trait DerivedKeyDomain: DomainSeparation {
 #[macro_export]
 macro_rules! hash_domain {
     ($name:ident, $domain:expr, $version: expr) => {
+        /// A hashing domain instance
         #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)]
         pub struct $name;
 

src/ristretto/ristretto_sig.rs

@@ -53,9 +53,8 @@ use crate::{
 ///
 /// #[allow(non_snake_case)]
 /// let (k, P) = get_keypair();
-/// let (r, R) = get_keypair();
-/// let e = Blake256::digest(b"Small Gods");
-/// let sig = RistrettoSchnorr::sign(k, r, &e);
+/// let msg = "Small Gods";
+/// let sig = RistrettoSchnorr::sign_message(k, &msg);
 /// ```
 ///
 /// # Verifying signatures
@@ -72,34 +71,32 @@ use crate::{
 /// # use tari_utilities::ByteArray;
 /// # use digest::Digest;
 ///
-/// # #[allow(non_snake_case)]
-/// let P = RistrettoPublicKey::from_hex(
-///     "74896a30c89186b8194e25f8c1382f8d3081c5a182fb8f8a6d34f27fbefbfc70",
-/// )
-/// .unwrap();
-/// let R = RistrettoPublicKey::from_hex(
-///     "fa14cb581ce5717248444721242e6b195a482d503a853dea4acb513074d8d803",
-/// )
-/// .unwrap();
-/// let s = RistrettoSecretKey::from_hex(
+/// let msg = "Maskerade";
+/// let k = RistrettoSecretKey::from_hex(
 ///     "bd0b253a619310340a4fa2de54cdd212eac7d088ee1dc47e305c3f6cbd020908",
 /// )
 /// .unwrap();
-/// let sig = RistrettoSchnorr::new(R, s);
-/// let e = Blake256::digest(b"Maskerade");
-/// assert!(sig.verify_challenge(&P, &e));
+/// # #[allow(non_snake_case)]
+/// let P = RistrettoPublicKey::from_secret_key(&k);
+/// let sig: SchnorrSignature<RistrettoPublicKey, RistrettoSecretKey> =
+///     SchnorrSignature::sign_message(k, msg).unwrap();
+/// assert!(sig.verify_message(&P, msg));
 /// ```
 pub type RistrettoSchnorr = SchnorrSignature<RistrettoPublicKey, RistrettoSecretKey>;
 
 #[cfg(test)]
 mod test {
     use digest::Digest;
-    use tari_utilities::{hex::from_hex, ByteArray};
+    use tari_utilities::{
+        hex::{from_hex, to_hex, Hex},
+        ByteArray,
+    };
 
     use crate::{
         hash::blake2::Blake256,
         keys::{PublicKey, SecretKey},
         ristretto::{RistrettoPublicKey, RistrettoSchnorr, RistrettoSecretKey},
+        signatures::SchnorrSignature,
     };
 
     #[test]
@@ -112,18 +109,19 @@ mod test {
     /// Create a signature, and then verify it. Also checks that some invalid signatures fail to verify
     #[test]
     #[allow(non_snake_case)]
-    fn sign_and_verify_message() {
+    fn raw_sign_and_verify_challenge() {
         let mut rng = rand::thread_rng();
         let (k, P) = RistrettoPublicKey::random_keypair(&mut rng);
         let (r, R) = RistrettoPublicKey::random_keypair(&mut rng);
+        // Use sign raw, and bind the nonce and public key manually
         let e = Blake256::new()
             .chain(P.as_bytes())
             .chain(R.as_bytes())
             .chain(b"Small Gods")
             .finalize();
         let e_key = RistrettoSecretKey::from_bytes(&e).unwrap();
         let s = &r + &e_key * &k;
-        let sig = RistrettoSchnorr::sign(k, r, &e).unwrap();
+        let sig = RistrettoSchnorr::sign_raw(k, r, &e).unwrap();
         let R_calc = sig.get_public_nonce();
         assert_eq!(R, *R_calc);
         assert_eq!(sig.get_signature(), &s);
@@ -155,9 +153,9 @@ mod test {
             .chain(b"Moving Pictures")
             .finalize();
         // Calculate Alice's signature
-        let s1 = RistrettoSchnorr::sign(k1, r1, &e).unwrap();
+        let s1 = RistrettoSchnorr::sign_raw(k1, r1, &e).unwrap();
         // Calculate Bob's signature
-        let s2 = RistrettoSchnorr::sign(k2, r2, &e).unwrap();
+        let s2 = RistrettoSchnorr::sign_raw(k2, r2, &e).unwrap();
         // Now add the two signatures together
         let s_agg = &s1 + &s2;
         // Check that the multi-sig verifies
@@ -167,11 +165,45 @@ mod test {
     /// Ristretto scalars have a max value 2^255. This test checks that hashed messages above this value can still be
     /// signed as a result of applying modulo arithmetic on the challenge value
     #[test]
+    #[allow(non_snake_case)]
     fn challenge_from_invalid_scalar() {
         let mut rng = rand::thread_rng();
         let m = from_hex("ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff").unwrap();
         let k = RistrettoSecretKey::random(&mut rng);
         let r = RistrettoSecretKey::random(&mut rng);
-        assert!(RistrettoSchnorr::sign(k, r, &m).is_ok());
+        assert!(RistrettoSchnorr::sign_raw(k, r, &m).is_ok());
+    }
+
+    #[test]
+    #[allow(non_snake_case)]
+    fn domain_separated_challenge() {
+        let P =
+            RistrettoPublicKey::from_hex("74896a30c89186b8194e25f8c1382f8d3081c5a182fb8f8a6d34f27fbefbfc70").unwrap();
+        let R =
+            RistrettoPublicKey::from_hex("fa14cb581ce5717248444721242e6b195a482d503a853dea4acb513074d8d803").unwrap();
+        let msg = "Moving Pictures";
+        let hash = SchnorrSignature::construct_domain_separated_challenge::<_, Blake256>(&R, &P, msg);
+        let naiive = Blake256::new()
+            .chain(R.as_bytes())
+            .chain(P.as_bytes())
+            .chain(msg)
+            .finalize()
+            .to_vec();
+        assert_ne!(hash.as_ref(), naiive.as_bytes());
+        assert_eq!(
+            to_hex(hash.as_ref()),
+            "e64d66b31e2b1c81272f5574f41ab2c997114436c2d3706dca1cf947bed60198"
+        );
+    }
+
+    #[test]
+    #[allow(non_snake_case)]
+    fn sign_and_verify_message() {
+        let mut rng = rand::thread_rng();
+        let (k, P) = RistrettoPublicKey::random_keypair(&mut rng);
+        let sig = RistrettoSchnorr::sign_message(k, "Queues are things that happen to other people").unwrap();
+        assert!(sig.verify_message(&P, "Queues are things that happen to other people"));
+        assert!(!sig.verify_message(&P, "Qs are things that happen to other people"));
+        assert!(!sig.verify_message(&(&P + &P), "Queues are things that happen to other people"));
     }
 }

src/ristretto/utils.rs

@@ -29,6 +29,10 @@ pub struct SignatureSet {
 /// # Panics
 ///
 /// The function panics if it cannot generate a suitable signature
+#[deprecated(
+    since = "0.16.0",
+    note = "Use SchnorrSignature::sign_message instead. This method will be removed in v1.0.0"
+)]
 pub fn sign<D: Digest>(
     private_key: &RistrettoSecretKey,
     message: &[u8],
@@ -41,7 +45,7 @@ pub fn sign<D: Digest>(
         .finalize()
         .to_vec();
     let e = RistrettoSecretKey::from_bytes(&message).map_err(|_| SchnorrSignatureError::InvalidChallenge)?;
-    let s = RistrettoSchnorr::sign(private_key.clone(), nonce.clone(), e.as_bytes())?;
+    let s = RistrettoSchnorr::sign_raw(private_key.clone(), nonce.clone(), e.as_bytes())?;
     Ok(SignatureSet {
         nonce,
         public_nonce,

src/signatures/schnorr.rs

@@ -10,11 +10,20 @@ use std::{
     ops::{Add, Mul},
 };
 
+use digest::Digest;
 use serde::{Deserialize, Serialize};
 use tari_utilities::ByteArray;
 use thiserror::Error;
 
-use crate::keys::{PublicKey, SecretKey};
+use crate::{
+    hash::blake2::Blake256,
+    hash_domain,
+    hashing::{DomainSeparatedHash, DomainSeparatedHasher},
+    keys::{PublicKey, SecretKey},
+};
+
+// Define the hashing domain for Schnorr signatures
+hash_domain!(SchnorrSigChallenge, "SchnorrSignature", 1);
 
 /// An error occurred during construction of a SchnorrSignature
 #[derive(Clone, Debug, Error, PartialEq, Eq, Deserialize, Serialize)]
@@ -57,7 +66,31 @@ where
 
     /// Sign a challenge with the given `secret` and private `nonce`. Returns an SchnorrSignatureError if `<K as
     /// ByteArray>::from_bytes(challenge)` returns an error.
+    ///
+    /// WARNING: The public key and nonce are NOT bound to the challenge. This method assumes that the challenge has
+    /// been constructed such that all commitments are already included in the challenge.
+    ///
+    /// Use [`sign_raw`] instead if this is what you want. (This method is a deprecated alias for `sign_raw`).
+    ///
+    /// If you want a simple API that binds the nonce and public key to the message, use [`sign_message`] instead.
+    #[deprecated(
+        since = "0.16.0",
+        note = "This method probably doesn't do what you think it does. Please use `sign_message` or `sign_raw` \
+                instead, depending on your use case. This function will be removed in v1.0.0"
+    )]
     pub fn sign(secret: K, nonce: K, challenge: &[u8]) -> Result<Self, SchnorrSignatureError>
+    where K: Add<Output = K> + Mul<P, Output = P> + Mul<Output = K> {
+        Self::sign_raw(secret, nonce, challenge)
+    }
+
+    /// Sign a challenge with the given `secret` and private `nonce`. Returns an SchnorrSignatureError if `<K as
+    /// ByteArray>::from_bytes(challenge)` returns an error.
+    ///
+    /// WARNING: The public key and nonce are NOT bound to the challenge. This method assumes that the challenge has
+    /// been constructed such that all commitments are already included in the challenge.
+    ///
+    /// If you want a simple API that binds the nonce and public key to the message, use [`sign_message`] instead.
+    pub fn sign_raw(secret: K, nonce: K, challenge: &[u8]) -> Result<Self, SchnorrSignatureError>
     where K: Add<Output = K> + Mul<P, Output = P> + Mul<Output = K> {
         // s = r + e.k
         let e = match K::from_bytes(challenge) {
@@ -70,6 +103,63 @@ where
         Ok(Self::new(public_nonce, s))
     }
 
+    /// Signs a message with the given secret key.
+    ///
+    /// This method correctly binds a nonce and the public key to the signature challenge, using domain-separated
+    /// hashing. The hasher is also opinionated in the sense that Blake2b 256-bit digest is always used.
+    ///
+    /// it is possible to customise the challenge by using [`construct_domain_separated_challenge`] and [`sign_raw`]
+    /// yourself, or even use [`sign_raw`] using a completely custom challenge.
+    pub fn sign_message<B>(secret: K, message: B) -> Result<Self, SchnorrSignatureError>
+    where
+        K: Add<Output = K> + Mul<P, Output = P> + Mul<Output = K>,
+        B: AsRef<[u8]>,
+    {
+        let nonce = K::random(&mut rand::thread_rng());
+        let public_nonce = P::from_secret_key(&nonce);
+        let public_key = P::from_secret_key(&secret);
+        let challenge = Self::construct_domain_separated_challenge::<_, Blake256>(&public_nonce, &public_key, message);
+        Self::sign_raw(secret, nonce, challenge.as_ref())
+    }
+
+    /// Constructs an opinionated challenge hash for the given public nonce, public key and message.
+    ///
+    /// In general, the signature challenge is given by `H(R, P, m)`. Often, plain concatenation is used to construct
+    /// the challenge. In this implementation, the challenge is constructed by means of domain separated hashing
+    /// using the provided digest.
+    ///
+    /// This challenge is used in the [`sign_message`] and [`verify_message`] methods.If you wish to use a custom
+    /// challenge, you can use [`sign_raw`] instead.
+    pub fn construct_domain_separated_challenge<B, D>(
+        public_nonce: &P,
+        public_key: &P,
+        message: B,
+    ) -> DomainSeparatedHash<D>
+    where
+        B: AsRef<[u8]>,
+        D: Digest,
+    {
+        DomainSeparatedHasher::<D, SchnorrSigChallenge>::new_with_label("challenge")
+            .chain(public_nonce.as_bytes())
+            .chain(public_key.as_bytes())
+            .chain(message.as_ref())
+            .finalize()
+    }
+
+    /// Verifies a signature created by the `sign_message` method. The function returns `true` if and only if the
+    /// message was signed by the secret key corresponding to the given public key, and that the challenge was
+    /// constructed using the domain-separation method defined in [`construct_domain_separated_challenge`].
+    pub fn verify_message<'a, B>(&self, public_key: &'a P, message: B) -> bool
+    where
+        for<'b> &'b K: Mul<&'a P, Output = P>,
+        for<'b> &'b P: Add<P, Output = P>,
+        B: AsRef<[u8]>,
+    {
+        let challenge =
+            Self::construct_domain_separated_challenge::<_, Blake256>(&self.public_nonce, public_key, message);
+        self.verify_challenge(public_key, challenge.as_ref())
+    }
+
     /// Returns true if this signature is valid for a public key and challenge, otherwise false. This will always return
     /// false if `<K as ByteArray>::from_bytes(challenge)` returns an error.
     pub fn verify_challenge<'a>(&self, public_key: &'a P, challenge: &[u8]) -> bool