Add support for Secp256k1 signatures #962

romac · 2021-08-27T12:58:49Z

Close: #939

Add support for verifying Secp256k1 signatures.
Turn the Signature enum into a wrapper over a byte array, so that it can hold signatures of any type. As the Signature is only ever used/useful in conjunction with a PublicKey, the latter is enough to determine which type of signature it holds.
Use Option<Signature> instead of Signature::None in various places.
Write tests using the test vectors provided below in the comments
Add a length check to the Signature constructor

Referenced an issue explaining the need for the change
Updated all relevant documentation in docs
Updated all code comments where relevant
Wrote tests
Added entry in .changelog/

tony-iqlusion · 2021-08-27T13:06:38Z

tendermint/src/signature.rs

-#[derive(Copy, Clone, Debug, PartialEq)]
-#[non_exhaustive]
-pub enum Signature {
-    /// Ed25519 block signature
-    Ed25519(Ed25519Signature),
-    /// No signature present
-    None, /* This could have been implemented as an `Option<>` but then handling it would be
-           * outside the scope of this enum. */
-}
+#[derive(Clone, Debug, PartialEq)]
+pub struct Signature(Vec<u8>);


It seems somewhat problematic to me to discard all information about the signature algorithm.

Based on the TryFrom impl below it looks like this would accept any bytestring as a "signature" except for empty ones, and only tries to validate that the signature is well-formed for a particular algorithm lazily at verification time, which means this type could accept all sorts of potential garbage data as a signature.

This includes signatures that are not the valid size for a particular algorithm, or signatures whose elements are't a valid element of e.g. the scalar field for a particular elliptic curve.

Yeah we tried finding another way around this, but there's no discriminant built into the signatures themselves that allows you to determine what kind of signature it is.

How else would one know this up-front, so that you could validate them beforehand?

The only way I can think of would require changing our serialization approach substantially, such that we can only deserialize signatures from within data structures that provide information about what algorithm was used. This complicates the serialization picture significantly.

I was wondering if there was a multihash (https://github.com/multiformats/multihash) equivalent for signatures.
I haven't been able to find one but it would be nice to have some sort of standardized self-describing signatures.

Self-describing signatures would be amazing, but that change would need to happen in the Tendermint RPC.

At the very least the only two supported signature types are both 64-bytes, so you could check for that.

It would probably be a good idea to eventually fix this upstream in Tendermint, but if it doesn't do that already I agree there's nothing relevant to this PR.

I agree, a 64-byte length check would be good 👍

try using k256::ecdsa::Signature::from_scalars to create signature

pub fn new_secp256k1_signature(value : &[u8]) -> Result<Secp256k1Signature, Error> { if value.len() != 64 { return Err(Kind::InvalidSignatureIdLength.into()); } let (r_bytes, s_bytes) = value.split_at(32); let r = match NonZeroScalar::from_repr(GenericArray::clone_from_slice(r_bytes)) { Some(v) => v, None => return Err(Kind::InvalidSignature.into()), }; let s = match NonZeroScalar::from_repr(GenericArray::clone_from_slice(s_bytes)) { Some(v) => v, None => return Err(Kind::InvalidSignature.into()), }; Secp256k1Signature::from_scalars(r,s).map_err(|_| Kind::InvalidSignature.into()) }

TryFrom<&[u8]> will also accomplish the same thing, and is a bit simpler, e.g.

Secp256k1Signature::try_from(value).map_err(|_| Kind::InvalidSignature.into())

tony-iqlusion · 2021-08-27T13:07:57Z

tendermint/src/public_key.rs

+            PublicKey::Ed25519(pk) => {
+                match ed25519_dalek::Signature::try_from(signature.as_bytes()) {
+                    Ok(sig) => pk.verify(msg, &sig).map_err(|_| {
+                        Error::signature_invalid(
+                            "Ed25519 signature verification failed".to_string(),
+                        )
+                    }),
+                    Err(e) => Err(Error::signature_invalid(format!(
+                        "invalid Ed25519 signature: {}",
+                        e
+                    ))),
+                }
+            }
            #[cfg(feature = "secp256k1")]
-            PublicKey::Secp256k1(_) => Err(Error::invalid_key(
-                "unsupported signature algorithm (ECDSA/secp256k1)".to_string(),
-            )),
+            PublicKey::Secp256k1(pk) => {
+                match k256::ecdsa::Signature::try_from(signature.as_bytes()) {
+                    Ok(sig) => pk.verify(msg, &sig).map_err(|_| {
+                        Error::signature_invalid(
+                            "Secp256k1 signature verification failed".to_string(),
+                        )
+                    }),
+                    Err(e) => Err(Error::signature_invalid(format!(
+                        "invalid Secp256k1 signature: {}",
+                        e
+                    ))),
+                }
+            }


It might be good to add some test vectors for both of these, just to ensure that a valid signature verifies and an invalid signature is rejected.

I can get some if you'd like.

Here are the RFC 8032 test vectors for Ed25519:

https://datatracker.ietf.org/doc/html/rfc8032#section-7.1

Here is an ECDSA/secp256k1 test vector:

https://github.com/RustCrypto/elliptic-curves/blob/master/k256/src/test_vectors/ecdsa.rs

There are quite a few ECDSA/secp256k1 test vectors in the Wycheproof test suite if you want more.

I've tried adding a few test vectors from RFC 8032 and the Wycheproof test suite in #964.

codecov-commenter · 2021-08-27T13:13:49Z

Codecov Report

Merging #962 (d294d9b) into master (bbad6fa) will increase coverage by 0.0%.
The diff coverage is 82.0%.

@@          Coverage Diff           @@
##           master    #962   +/-   ##
======================================
  Coverage    72.0%   72.1%           
======================================
  Files         203     203           
  Lines       16522   16589   +67     
======================================
+ Hits        11912   11969   +57     
- Misses       4610    4620   +10

Impacted Files	Coverage Δ
light-client/src/predicates/errors.rs	`50.0% <ø> (ø)`
tendermint/src/error.rs	`0.0% <ø> (ø)`
tendermint/src/block/commit_sig.rs	`78.7% <63.6%> (+1.1%)`	⬆️
tendermint/src/signature.rs	`56.6% <67.7%> (-3.9%)`	⬇️
tendermint/src/public_key.rs	`69.0% <80.3%> (+3.4%)`	⬆️
tendermint/src/proposal.rs	`89.3% <90.9%> (+0.2%)`	⬆️
light-client/src/operations/voting_power.rs	`97.9% <100.0%> (-0.1%)`	⬇️
tendermint/src/vote.rs	`76.0% <100.0%> (+1.6%)`	⬆️
tendermint/src/vote/sign_vote.rs	`95.3% <100.0%> (+<0.1%)`	⬆️
testgen/src/commit.rs	`90.5% <100.0%> (ø)`
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bbad6fa...d294d9b. Read the comment docs.

tony-iqlusion · 2021-08-27T13:34:11Z

tendermint/src/signature.rs

-           * outside the scope of this enum. */
-}
+#[derive(Clone, Debug, PartialEq)]
+pub struct Signature(Vec<u8>);

 impl Protobuf<Vec<u8>> for Signature {}

 impl TryFrom<Vec<u8>> for Signature {


This is somewhat orthogonal to the changes in this PR, but I'm curious why this is TryFrom<Vec<u8>> instead of TryFrom<&[u8]>. The latter is a lot more flexible since you can use it with anything which can be borrowed as a byte slice (e.g. Bytes)

The TryFrom<Vec<u8>> instance is required by the Protobuf<Vec<u8>> instance, but I agree that we should add a TryFrom<&[u8]> instance as well.

pub trait Protobuf<T: Message + From<Self> + Default> where Self: Sized + Clone + TryFrom<T>, // <--------- <Self as TryFrom<T>>::Error: Display,

thanethomson · 2021-08-31T23:10:35Z

@tony-iqlusion since @romac is on vacation at the moment, I'll make updates to this PR in #964. Once we're happy with #964, we can merge into this PR and subsequently into master.

* Add fallible constructor for Signature to facilitate TryFrom<&[u8]> Signed-off-by: Thane Thomson <connect@thanethomson.com> * Add Ed25519 test vectors from RFC Signed-off-by: Thane Thomson <connect@thanethomson.com> * Add test vectors for secp256k1 signature validation Signed-off-by: Thane Thomson <connect@thanethomson.com>

romac added 6 commits August 25, 2021 13:40

Show underlying detail in some signature errors

b74fb96

Add support for Secp256k1 signatures

6487569

Represent Signature as a wrapper over a byte array

989cfed

Cleanup

950e13f

Fix warning when secp256k1 feature is disabled

04e02d5

Add .changelog entry

6804579

tony-iqlusion reviewed Aug 27, 2021

View reviewed changes

thanethomson mentioned this pull request Aug 31, 2021

Updates to secp256k1 signature support #964

Merged

5 tasks

thanethomson marked this pull request as ready for review September 1, 2021 15:33

thanethomson requested a review from tony-iqlusion September 1, 2021 15:47

tony-iqlusion approved these changes Sep 1, 2021

View reviewed changes

thanethomson merged commit b99c937 into master Sep 1, 2021

thanethomson deleted the romac/939-secp256k1-signatures branch September 1, 2021 16:56

RiccardoM mentioned this pull request Sep 1, 2021

Secp256k1 signature verification (BandChain) informalsystems/hermes#1155

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Secp256k1 signatures #962

Add support for Secp256k1 signatures #962

romac commented Aug 27, 2021 •

edited by thanethomson

Loading

tony-iqlusion Aug 27, 2021 •

edited

Loading

thanethomson Aug 27, 2021

hu55a1n1 Aug 27, 2021

thanethomson Aug 27, 2021

tony-iqlusion Aug 27, 2021

thanethomson Aug 27, 2021

lyqingye Aug 31, 2021

tony-iqlusion Aug 31, 2021 •

edited

Loading

tony-iqlusion Aug 27, 2021

tony-iqlusion Aug 27, 2021

thanethomson Aug 31, 2021

codecov-commenter commented Aug 27, 2021 •

edited

Loading

tony-iqlusion Aug 27, 2021

romac Aug 28, 2021

thanethomson commented Aug 31, 2021

Add support for Secp256k1 signatures #962

Add support for Secp256k1 signatures #962

Conversation

romac commented Aug 27, 2021 • edited by thanethomson Loading

tony-iqlusion Aug 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tony-iqlusion Aug 31, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Aug 27, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thanethomson commented Aug 31, 2021

romac commented Aug 27, 2021 •

edited by thanethomson

Loading

tony-iqlusion Aug 27, 2021 •

edited

Loading

tony-iqlusion Aug 31, 2021 •

edited

Loading

codecov-commenter commented Aug 27, 2021 •

edited

Loading