Add support for Secp256k1 signatures (informalsystems#962)

* Show underlying detail in some signature errors * Add support for Secp256k1 signatures * Represent Signature as a wrapper over a byte array * Cleanup * Fix warning when `secp256k1` feature is disabled * Add .changelog entry * Updates to secp256k1 signature support (informalsystems#964) * Add fallible constructor for Signature to facilitate TryFrom<&[u8]> Signed-off-by: Thane Thomson <[email protected]> * Add Ed25519 test vectors from RFC Signed-off-by: Thane Thomson <[email protected]> * Add test vectors for secp256k1 signature validation Signed-off-by: Thane Thomson <[email protected]> Co-authored-by: Thane Thomson <[email protected]>
penso · Sep 1, 2021 · b99c937 · b99c937
1 parent bbad6fa
commit b99c937
Show file tree

Hide file tree

Showing 13 changed files with 424 additions and 131 deletions.
diff --git a/.changelog/unreleased/features/939-secp256k1-signatures.md b/.changelog/unreleased/features/939-secp256k1-signatures.md
@@ -0,0 +1 @@
+- Add support for Secp256k1 signatures ([#939](https://github.com/informalsystems/tendermint-rs/issues/939))
diff --git a/light-client/src/operations/voting_power.rs b/light-client/src/operations/voting_power.rs
@@ -138,12 +138,9 @@ impl VotingPowerCalculator for ProdVotingPowerCalculator {
                 None => continue, // Cannot find matching validator, so we skip the vote
             };
 
-            let signed_vote = SignedVote::new(
-                vote.clone(),
-                signed_header.header.chain_id.clone(),
-                vote.validator_address,
-                vote.signature,
-            );
+            let signed_vote =
+                SignedVote::from_vote(vote.clone(), signed_header.header.chain_id.clone())
+                    .ok_or_else(VerificationError::missing_signature)?;
 
             // Check vote is valid
             let sign_bytes = signed_vote.sign_bytes();
@@ -152,7 +149,7 @@ impl VotingPowerCalculator for ProdVotingPowerCalculator {
                 .is_err()
             {
                 return Err(VerificationError::invalid_signature(
-                    signed_vote.signature().to_bytes(),
+                    signed_vote.signature().as_bytes().to_vec(),
                     Box::new(validator),
                     sign_bytes,
                 ));
@@ -212,7 +209,7 @@ fn non_absent_vote(
         timestamp: Some(timestamp),
         validator_address,
         validator_index,
-        signature: *signature,
+        signature: signature.clone(),
     })
 }
 

diff --git a/light-client/src/predicates/errors.rs b/light-client/src/predicates/errors.rs
@@ -58,14 +58,19 @@ define_error! {
                     e.address)
             },
 
+        MissingSignature
+            | _ | {
+                format_args!("missing signature")
+            },
+
         InvalidSignature
             {
                 signature: Vec<u8>,
                 validator: Box<Validator>,
                 sign_bytes: Vec<u8>,
             }
             | e | {
-                format_args!("Couldn't verify signature `{:?}` with validator `{:?}` on sign_bytes `{:?}`",
+                format_args!("failed to verify signature `{:?}` with validator `{:?}` on sign_bytes `{:?}`",
                     e.signature, e.validator, e.sign_bytes)
             },
 

diff --git a/tendermint/src/block/commit_sig.rs b/tendermint/src/block/commit_sig.rs
@@ -20,7 +20,7 @@ pub enum CommitSig {
         /// Timestamp of vote
         timestamp: Time,
         /// Signature of vote
-        signature: Signature,
+        signature: Option<Signature>,
     },
     /// voted for nil.
     BlockIdFlagNil {
@@ -29,7 +29,7 @@ pub enum CommitSig {
         /// Timestamp of vote
         timestamp: Time,
         /// Signature of vote
-        signature: Signature,
+        signature: Option<Signature>,
     },
 }
 
@@ -79,26 +79,33 @@ impl TryFrom<RawCommitSig> for CommitSig {
                     ));
                 }
             }
+
             if !value.signature.is_empty() {
-                return Err(Error::invalid_signature("empty signature".to_string()));
+                return Err(Error::invalid_signature(
+                    "expected empty signature for absent commitsig".to_string(),
+                ));
             }
+
             return Ok(CommitSig::BlockIdFlagAbsent);
         }
+
         if value.block_id_flag == BlockIdFlag::Commit.to_i32().unwrap() {
             if value.signature.is_empty() {
                 return Err(Error::invalid_signature(
-                    "regular commitsig has no signature".to_string(),
+                    "expected non-empty signature for regular commitsig".to_string(),
                 ));
             }
+
             if value.validator_address.is_empty() {
                 return Err(Error::invalid_validator_address());
             }
+
             let timestamp = value.timestamp.ok_or_else(Error::missing_timestamp)?.into();
 
             return Ok(CommitSig::BlockIdFlagCommit {
                 validator_address: value.validator_address.try_into()?,
                 timestamp,
-                signature: value.signature.try_into()?,
+                signature: Signature::new(value.signature)?,
             });
         }
         if value.block_id_flag == BlockIdFlag::Nil.to_i32().unwrap() {
@@ -113,7 +120,7 @@ impl TryFrom<RawCommitSig> for CommitSig {
             return Ok(CommitSig::BlockIdFlagNil {
                 validator_address: value.validator_address.try_into()?,
                 timestamp: value.timestamp.ok_or_else(Error::missing_timestamp)?.into(),
-                signature: value.signature.try_into()?,
+                signature: Signature::new(value.signature)?,
             });
         }
         Err(Error::block_id_flag())
@@ -137,7 +144,7 @@ impl From<CommitSig> for RawCommitSig {
                 block_id_flag: BlockIdFlag::Nil.to_i32().unwrap(),
                 validator_address: validator_address.into(),
                 timestamp: Some(timestamp.into()),
-                signature: signature.into(),
+                signature: signature.map(|s| s.to_bytes()).unwrap_or_default(),
             },
             CommitSig::BlockIdFlagCommit {
                 validator_address,
@@ -147,7 +154,7 @@ impl From<CommitSig> for RawCommitSig {
                 block_id_flag: BlockIdFlag::Commit.to_i32().unwrap(),
                 validator_address: validator_address.into(),
                 timestamp: Some(timestamp.into()),
-                signature: signature.into(),
+                signature: signature.map(|s| s.to_bytes()).unwrap_or_default(),
             },
         }
     }

diff --git a/tendermint/src/error.rs b/tendermint/src/error.rs
@@ -45,15 +45,18 @@ define_error! {
 
         Protocol
             { detail: String }
-            |_| { format_args!("protocol error") },
+            |e| { format_args!("protocol error: {}", e.detail) },
 
         OutOfRange
             [ DisplayOnly<OutOfRangeError> ]
             |_| { format_args!("value out of range") },
 
+        EmptySignature
+            |_| { format_args!("empty signature") },
+
         SignatureInvalid
             { detail: String }
-            |_| { format_args!("bad signature") },
+            |e| { format_args!("bad signature: {}", e.detail) },
 
         InvalidMessageType
             |_| { format_args!("invalid message type") },

diff --git a/tendermint/src/proposal.rs b/tendermint/src/proposal.rs
@@ -35,7 +35,7 @@ pub struct Proposal {
     /// Timestamp
     pub timestamp: Option<Time>,
     /// Signature
-    pub signature: Signature,
+    pub signature: Option<Signature>,
 }
 
 impl Protobuf<RawProposal> for Proposal {}
@@ -58,7 +58,7 @@ impl TryFrom<RawProposal> for Proposal {
             pol_round,
             block_id: value.block_id.map(TryInto::try_into).transpose()?,
             timestamp: value.timestamp.map(|t| t.into()),
-            signature: value.signature.try_into()?,
+            signature: Signature::new(value.signature)?,
         })
     }
 }
@@ -72,7 +72,7 @@ impl From<Proposal> for RawProposal {
             pol_round: value.pol_round.map_or(-1, Into::into),
             block_id: value.block_id.map(Into::into),
             timestamp: value.timestamp.map(Into::into),
-            signature: value.signature.into(),
+            signature: value.signature.map(|s| s.to_bytes()).unwrap_or_default(),
         }
     }
 }
@@ -150,7 +150,9 @@ mod tests {
                 .unwrap(),
             }),
             timestamp: Some(dt.into()),
-            signature: Signature::Ed25519(Ed25519Signature::new([0; ED25519_SIGNATURE_SIZE])),
+            signature: Some(Signature::from(Ed25519Signature::new(
+                [0; ED25519_SIGNATURE_SIZE],
+            ))),
         };
 
         let mut got = vec![];
@@ -232,7 +234,9 @@ mod tests {
                 .unwrap(),
             }),
             timestamp: Some(dt.into()),
-            signature: Signature::Ed25519(Ed25519Signature::new([0; ED25519_SIGNATURE_SIZE])),
+            signature: Some(Signature::from(Ed25519Signature::new(
+                [0; ED25519_SIGNATURE_SIZE],
+            ))),
         };
 
         let mut got = vec![];
@@ -316,7 +320,9 @@ mod tests {
                 )
                 .unwrap(),
             }),
-            signature: Signature::Ed25519(Ed25519Signature::new([0; ED25519_SIGNATURE_SIZE])),
+            signature: Some(Signature::from(Ed25519Signature::new(
+                [0; ED25519_SIGNATURE_SIZE],
+            ))),
         };
         let want = SignProposalRequest {
             proposal,
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1 @@
		- Add support for Secp256k1 signatures ([#939](https://github.com/informalsystems/tendermint-rs/issues/939))