Replace `signatory` with `ed25519-dalek` and `k256` crates #522

tony-iqlusion · 2020-08-13T03:23:44Z

Native support for the signature traits used by the signatory* was recently added to ed25519-dalek in v1.0.0-pre.4, and the k256 crate recently added a native pure Rust implementation of ECDSA/secp256k1.

Together these changes eliminate the need for the signatory* wrapper crates. Additionally, k256 eliminates the need for the rust-secp256k1 C-based library, which has been problematic with e.g. WASM (#391).

This commit eliminates all signatory dependencies and uses ed25519-dalek and k256 directly instead. Both of these were already dependencies, so it's not adding any additional dependencies, but instead removes several of them.

tony-iqlusion · 2020-08-13T03:25:51Z

tendermint/src/amino_types/ed25519.rs

-        let raw_pk: [u8; PUBLIC_KEY_SIZE] = [
-            0x79, 0xce, 0xd, 0xe0, 0x43, 0x33, 0x4a, 0xec, 0xe0, 0x8b, 0x7b, 0xb5, 0x61, 0xbc,
-            0xe7, 0xc1, 0xd4, 0x69, 0xc3, 0x44, 0x26, 0xec, 0xef, 0xc0, 0x72, 0xa, 0x52, 0x4d,
-            0x37, 0x32, 0xef, 0xed,
+        let raw_pk: [u8; PUBLIC_KEY_LENGTH] = [
+            0xaf, 0xf3, 0x94, 0xc5, 0xb7, 0x5c, 0xfb, 0xd, 0xd9, 0x28, 0xe5, 0x8a, 0x92, 0xdd,
+            0x76, 0x55, 0x2b, 0x2e, 0x8d, 0x19, 0x6f, 0xe9, 0x12, 0x14, 0x50, 0x80, 0x6b, 0xd0,
+            0xd9, 0x3f, 0xd0, 0xcb,


Ed(wards)25519 point decompression was failing for this? (the old implementation didn't attempt to decompress points)

I need to double check compatibility with the go-code here.

I assume it's OK to check with the upcoming 0.34 / master instead of 0.33 (cc @brapse @ebuchman).

OK, I've tested against a few freshly generated pub-keys and they all pass! I think that raw_pk wasn't generated using ed25519.GenPrivKey().PubKey() and we didn't notice before as the old lib didn't attempt to decompress points.

tony-iqlusion · 2020-08-13T03:27:13Z

tendermint/src/lib.rs

@@ -4,6 +4,7 @@
 //! blockchain networks, including chain information types, secret connections,
 //! and remote procedure calls (JSONRPC).

+#![cfg_attr(docsrs, feature(doc_cfg))]


I wired up doc_cfg, which allows you to document which Cargo features need to be enabled to use certain types/methods in rustdoc.

Rendered example:

We should add this to all our other crates too.

tony-iqlusion · 2020-08-13T03:28:10Z

tendermint/src/private_key.rs

-/// Ed25519 keypairs
-#[derive(Zeroize)]
-#[zeroize(drop)]
-pub struct Ed25519Keypair([u8; ED25519_KEYPAIR_SIZE]);


I was able to replace this with ed25519-dalek's own ed25519_dalek::Keypair type, which validates keys.

I think this is the only reason why we were seeing: https://github.com/informalsystems/tendermint-rs/pull/522/files#r469674391

Yeah, this type previously didn't check that the associated point would decompress correctly (or was computed correctly from the Ed25519 "seed" in the keypair)

tony-iqlusion · 2020-08-13T03:31:35Z

tendermint/src/validator.rs

-    pub fn verify_signature(&self, sign_bytes: &[u8], signature: &[u8]) -> bool {
-        if let Some(pk) = &self.pub_key.ed25519() {
-            let verifier = Ed25519Verifier::from(pk);
-            if let Ok(sig) = ed25519::Signature::from_bytes(signature) {
-                return verifier.verify(sign_bytes, &sig).is_ok();
-            }
-        }
-        false
+    pub fn verify_signature(&self, sign_bytes: &[u8], signature: &Signature) -> Result<(), Error> {


I made signature typed here, and changed it from returning a bool to a Result. I very strongly suggest doing this for a few reasons:

Result is #[must_use], so it significantly lowers the risk of ignoring signature validation by making most cases of it a compile-time error

Result provides better type safety. Here's an example of a real-world signature validation bug which resulted from the use of combinators + bool instead of Result: https://github.com/libp2p/rust-libp2p/pull/1127/files

Thanks for this one!

tony-iqlusion · 2020-08-13T03:32:52Z

tendermint/src/validator.rs

        let v3 = make_validator(
-            "EB6B732C4BD86B5FA3F3BC3DB688DA0ED182A7411F81C2D405506B298FC19E52",
+            "76A2B3F5CBB567F0D689D9DF7155FC89A4C878F040D7A5BB85FF68B74D253FC7",
            770_561_664_770_006_272,
        );


This is another example of an Ed(wards)25519 point which failed to validate. It looks like it might've been munged slightly from the similar looking point below (L284).

It looks like these vectors were tested against the Go implementation, so they need to be rechecked for compatibility.

I'll recheck them during the weekend.

I suspect this is a similar problem as above and the key was simply not valid. On the other hand I'm surprised that the other validator keys here (v1, v2) are valid 🤔 As we probably generated them in the same way. From looking at the decompress code in the rust lib I don't see any discrepancies though.

I would assume that both libraries are properly tested against many test-vectors? https://ed25519.cr.yp.to/python/sign.input (true for the go lib not sure about the rust impl)

A simple way to confirm that this pub-key would also fail the go-implementation is to feed that old bytes into the internal package edwards25519:

var A edwards25519.ExtendedGroupElement var publicKeyBytes [32]byte copy(publicKeyBytes[:], publicKey) asster.False(t, A.FromBytes(&publicKeyBytes))

I'll give that a try shortly.

OK, so this key won't work in the go implementation either. Tested via:

func TestKey(t *testing.T) { publicKey, err := hex.DecodeString("EB6B732C4BD86B5FA3F3BC3DB688DA0ED182A7411F81C2D405506B298FC19E52") if err != nil { t.Fatalf("%v", err) } var A edwards25519.ExtendedGroupElement var publicKeyBytes [32]byte copy(publicKeyBytes[:], publicKey) if A.FromBytes(&publicKeyBytes) { panic("You shall not pass") } else { t.Log("OK") } }

And same for the other case. I have no idea where the previous two test-vectors originated from but they never were valid pub-keys and could not have been generated with the equivalent go-code.

What was the take away from this?

We should make sure that the test-vectors we use are generated properly, i.e. they don't just satisfy the properties needed for the test-case (here: 32 random bytes did the job) but rather they should be valid domain objects (here: a valid pub-key) - unless of course we want to test that an invalid case.

tony-iqlusion · 2020-08-13T03:33:26Z

testgen/src/validator.rs

@@ -32,7 +36,7 @@ impl Validator {
    set_option!(proposer_priority, i64);

    /// Get a signer from this validator companion.
-    pub fn get_signer(&self) -> Result<Ed25519Signer, SimpleError> {
+    pub fn get_signer(&self) -> Result<Ed25519Keypair, SimpleError> {


Perhaps this should be renamed to:

Suggested change

pub fn get_signer(&self) -> Result<Ed25519Keypair, SimpleError> {

pub fn get_keypair(&self) -> Result<Ed25519Keypair, SimpleError> {

tony-iqlusion · 2020-08-13T03:33:57Z

testgen/src/validator.rs

-        let public_key = try_with!(signer.public_key(), "failed to get public key");
-        let verifier = Ed25519Verifier::from(&public_key);
-        Ok(verifier)
+    pub fn get_verifier(&self) -> Result<Ed25519PublicKey, SimpleError> {


Likewise, perhaps rename this to:

Suggested change

pub fn get_verifier(&self) -> Result<Ed25519PublicKey, SimpleError> {

pub fn get_verifier(&self) -> Result<Ed25519Verifier, SimpleError> {

tony-iqlusion · 2020-08-13T03:40:02Z

tendermint/src/public_key.rs

    /// Secp256k1 keys
    #[cfg(feature = "secp256k1")]
+    #[cfg_attr(docsrs, doc(cfg(feature = "secp256k1")))]
    #[serde(
        rename = "tendermint/PubKeySecp256k1",
        serialize_with = "serialize_secp256k1_base64",
        deserialize_with = "deserialize_secp256k1_base64"
    )]
-    Secp256k1(secp256k1::PublicKey),
+    Secp256k1(Secp256k1PublicKey),


If you want to keep this gated under a secp256k1 feature, it might make sense to make pub enum PublicKey a #[non_exhaustive] enum. Otherwise clients might assume that variants do or do not exist depending on what features are enabled, and if they try to exhaustively match, they'll break if things don't match up to their expectations.

Or, perhaps the (lightweight, pure Rust) k256 crate is a less onerous dependency than signatory-secp256k1 (C library wrapper) to the point that the secp256k1 feature can be removed.

All for the enum with non_exhaustive - how much would it take to remove the secp256k1 feature?

Remove it as a feature, or remove all secp256k1-related code? I think the k256 crate is lightweight enough it doesn't really need to be feature-gated anymore unless there's some other reason for that (e.g. the eventual goal is deprecation).

For what it's worth, the KMS still relies on it for the transaction signing feature.

tony-iqlusion · 2020-08-13T03:41:33Z

The clippy failures seem unrelated?

xla

Dope! Reviewed mostly for style, consistency and structure.

tendermint/Cargo.toml

tendermint/src/public_key.rs

tendermint/src/account.rs

xla · 2020-08-14T08:43:09Z

tendermint/src/public_key.rs

    /// Secp256k1 keys
    #[cfg(feature = "secp256k1")]
+    #[cfg_attr(docsrs, doc(cfg(feature = "secp256k1")))]
    #[serde(
        rename = "tendermint/PubKeySecp256k1",
        serialize_with = "serialize_secp256k1_base64",
        deserialize_with = "deserialize_secp256k1_base64"
    )]
-    Secp256k1(secp256k1::PublicKey),
+    Secp256k1(Secp256k1PublicKey),


All for the enum with non_exhaustive - how much would it take to remove the secp256k1 feature?

xla · 2020-08-14T08:46:04Z

tendermint/src/validator.rs

-    pub fn verify_signature(&self, sign_bytes: &[u8], signature: &[u8]) -> bool {
-        if let Some(pk) = &self.pub_key.ed25519() {
-            let verifier = Ed25519Verifier::from(pk);
-            if let Ok(sig) = ed25519::Signature::from_bytes(signature) {
-                return verifier.verify(sign_bytes, &sig).is_ok();
-            }
-        }
-        false
+    pub fn verify_signature(&self, sign_bytes: &[u8], signature: &Signature) -> Result<(), Error> {


Thanks for this one!

xla · 2020-08-14T08:47:27Z

testgen/src/validator.rs

@@ -32,7 +36,7 @@ impl Validator {
    set_option!(proposer_priority, i64);

    /// Get a signer from this validator companion.
-    pub fn get_signer(&self) -> Result<Ed25519Signer, SimpleError> {
+    pub fn get_signer(&self) -> Result<Ed25519Keypair, SimpleError> {


xla · 2020-08-14T08:47:36Z

testgen/src/validator.rs

-        let public_key = try_with!(signer.public_key(), "failed to get public key");
-        let verifier = Ed25519Verifier::from(&public_key);
-        Ok(verifier)
+    pub fn get_verifier(&self) -> Result<Ed25519PublicKey, SimpleError> {


tony-iqlusion · 2020-08-18T01:25:03Z

@liamsi @xla hopefully I've addressed all of the line notes.

The major remaining concern to me is the point decompression-related issues. I'll try to verify the behaviors on both Rust and Go, as if there's an actual discrepancy there that sounds potentially consensus-critical.

liamsi · 2020-08-18T10:38:52Z

Yeah, it's a bit weird as the go library also does point decompression as far as I remember. And the test vectors surely were generated using the go implementation (I wish we documented how exactly these were generated). I guess as long as we can "roundtrip" pubkeys between the go and rust implementation we should be fine.

A while ago @ValarDragon suggested to cache these decompressions and add a verification method directly on the uncompressed pk as an optimization: tendermint/tendermint#1784. But as far as I know this never materialized.

liamsi · 2020-08-18T10:57:51Z

Actually, we did document were these test-vectors are coming from, besides, how the raw bytes came about:

tendermint-rs/tendermint/src/amino_types/ed25519.rs

Lines 106 to 110 in 48a11d1

    
           //  var pubKey [32]byte 
        
           //	copy(pubKey[:],[]byte{0x79, 0xce, 0xd, 0xe0, 0x43, 0x33, 0x4a, 0xec, 0xe0, 0x8b, 0x7b, 
        
           //  0xb5, 0x61, 0xbc, 0xe7, 0xc1, 
        
           //	0xd4, 0x69, 0xc3, 0x44, 0x26, 0xec, 0xef, 0xc0, 0x72, 0xa, 0x52, 0x4d, 0x37, 0x32, 0xef, 
        
           // 0xed})

Note that the go code does not do any checks on these bytes but I would be surprised that the existing test-vectors were not generated from a priv-key. In that case the pubkey should have been compressed, as this is called while generating a key(pair): https://github.com/golang/crypto/blob/123391ffb6de907695e1066dc40c1ff09322aeb6/ed25519/internal/edwards25519/edwards25519.go#L708

My suggestion is to generate a fresh key pair in go and use this as a new test-vector (maybe this is what you did anyways?) and verify everything works as expected.

liamsi · 2020-08-18T11:20:36Z

https://github.com/informalsystems/tendermint-rs/pull/522/files#r472102039
The bytes that were used were not generated using github.com/tendermint/tendermint/crypto/ed25519.ed25519.GenPrivKey()

This is not a compatibility issue!

liamsi

Thanks @tony-iqlusion 👍 I'll try to confirm that the pubkeys that fail to decompress will also fail in the go version and then we are good to merge.

liamsi · 2020-08-18T11:23:36Z

tendermint/Cargo.toml

 futures = "0.3"
+k256 = { version = "0.4", optional = true, features = ["ecdsa"] }


I didn't look into k256 but tendermint-rs doesn't really use secp256k1. Hence this is not critical from tendermint's POV.

liamsi · 2020-08-18T11:25:58Z

tendermint/src/lib.rs

@@ -4,6 +4,7 @@
 //! blockchain networks, including chain information types, secret connections,
 //! and remote procedure calls (JSONRPC).

+#![cfg_attr(docsrs, feature(doc_cfg))]


We should add this to all our other crates too.

liamsi · 2020-08-18T11:28:24Z

tendermint/src/private_key.rs

+    #[serde(
+        rename = "tendermint/PrivKeyEd25519",
+        serialize_with = "serialize_ed25519_keypair",
+        deserialize_with = "deserialize_ed25519_keypair"
+    )]


Just curious: why did you decide to go with the [de]serialize_with attribute instead of the impl [DE]Serialize for ... on the type as previously?

The Ed25519 struct this enum variant wraps is now from a different crate (ed25519_dalek::Keypair), but the serde serialization logic is intended to match priv_validator.json (or the bare Base64 encoded format used by the KMS)

liamsi · 2020-08-18T11:36:10Z

tendermint/src/private_key.rs

-/// Ed25519 keypairs
-#[derive(Zeroize)]
-#[zeroize(drop)]
-pub struct Ed25519Keypair([u8; ED25519_KEYPAIR_SIZE]);


I think this is the only reason why we were seeing: https://github.com/informalsystems/tendermint-rs/pull/522/files#r469674391

liamsi · 2020-08-18T12:11:12Z

tendermint/src/validator.rs

        let v3 = make_validator(
-            "EB6B732C4BD86B5FA3F3BC3DB688DA0ED182A7411F81C2D405506B298FC19E52",
+            "76A2B3F5CBB567F0D689D9DF7155FC89A4C878F040D7A5BB85FF68B74D253FC7",
            770_561_664_770_006_272,
        );


I suspect this is a similar problem as above and the key was simply not valid. On the other hand I'm surprised that the other validator keys here (v1, v2) are valid 🤔 As we probably generated them in the same way. From looking at the decompress code in the rust lib I don't see any discrepancies though.

I would assume that both libraries are properly tested against many test-vectors? https://ed25519.cr.yp.to/python/sign.input (true for the go lib not sure about the rust impl)

A simple way to confirm that this pub-key would also fail the go-implementation is to feed that old bytes into the internal package edwards25519:

var A edwards25519.ExtendedGroupElement var publicKeyBytes [32]byte copy(publicKeyBytes[:], publicKey) asster.False(t, A.FromBytes(&publicKeyBytes))

I'll give that a try shortly.

liamsi

LGTM 👍

tony-iqlusion · 2020-08-18T13:13:39Z

@xla looks like this is blocked on your suggested changes, but I think they've all been done.

Any ideas on the clippy error? Will it block merging?

Native support for the `signature` traits used by the `signatory*` was recently added to `ed25519-dalek` in v1.0.0-pre.4, and the `k256` crate recently added a native pure Rust implementation of ECDSA/secp256k1. Together these changes eliminate the need for the `signatory*` wrapper crates. Additionally, `k256` eliminates the need for the rust-secp256k1 C-based library, which has been problematic with e.g. WASM (#391). This commit eliminates all `signatory` dependencies and uses `ed25519-dalek` and `k256` directly instead. Both of these were already dependencies, so it's not adding any additional dependencies, but instead removes several of them.

tony-iqlusion · 2020-08-18T15:29:16Z

After some investigation the clippy error seems to be related to the enum variants the PublicKey type wraps.

Specifically the VerificationError::InvalidSignature variant stores the entire Validator struct, which I guess is now large enough to push it over the clippy size limit with the changes to the PublicKey type.

I threw it in a Box, and clippy now passes locally.

xla

🛂 ♻️ ◻️ 🆔

tony-iqlusion commented Aug 13, 2020

View reviewed changes

tarcieri requested a review from liamsi August 13, 2020 03:34

tony-iqlusion commented Aug 13, 2020

View reviewed changes

xla added critical dependencies Pull requests that update a dependency file enhancement New feature or request labels Aug 14, 2020

xla suggested changes Aug 14, 2020

View reviewed changes

tarcieri force-pushed the replace-signatory-with-ed25519-dalek-and-k256 branch 4 times, most recently from 5f10d14 to 67466e7 Compare August 18, 2020 01:23

liamsi reviewed Aug 18, 2020

View reviewed changes

liamsi previously approved these changes Aug 18, 2020

View reviewed changes

tarcieri dismissed liamsi’s stale review via 8cfe9a4 August 18, 2020 15:29

tarcieri force-pushed the replace-signatory-with-ed25519-dalek-and-k256 branch from 67466e7 to 8cfe9a4 Compare August 18, 2020 15:29

liamsi requested a review from xla August 18, 2020 15:56

liamsi approved these changes Aug 18, 2020

View reviewed changes

xla approved these changes Aug 18, 2020

View reviewed changes

brapse merged commit 43b80a5 into master Aug 18, 2020

brapse deleted the replace-signatory-with-ed25519-dalek-and-k256 branch August 18, 2020 16:39

liamsi added the breaking label Aug 19, 2020

ebuchman mentioned this pull request Sep 20, 2020

Update to tendermint-rs v0.16 informalsystems/hermes#243

Closed

	pub fn get_signer(&self) -> Result<Ed25519Keypair, SimpleError> {
	pub fn get_keypair(&self) -> Result<Ed25519Keypair, SimpleError> {

	pub fn get_verifier(&self) -> Result<Ed25519PublicKey, SimpleError> {
	pub fn get_verifier(&self) -> Result<Ed25519Verifier, SimpleError> {

		futures = "0.3"
		k256 = { version = "0.4", optional = true, features = ["ecdsa"] }

Replace signatory with ed25519-dalek and k256 crates #522

Replace signatory with ed25519-dalek and k256 crates #522

Conversation

tony-iqlusion commented Aug 13, 2020

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

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

liamsi Aug 18, 2020 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tarcieri Aug 17, 2020 • edited Loading

Choose a reason for hiding this comment

tony-iqlusion commented Aug 13, 2020

xla left a 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 commented Aug 18, 2020

liamsi commented Aug 18, 2020

liamsi commented Aug 18, 2020

liamsi commented Aug 18, 2020

liamsi left a 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

Choose a reason for hiding this comment

liamsi left a comment

Choose a reason for hiding this comment

tony-iqlusion commented Aug 18, 2020

tony-iqlusion commented Aug 18, 2020 • edited Loading

xla left a comment

Choose a reason for hiding this comment

Replace `signatory` with `ed25519-dalek` and `k256` crates #522

Replace `signatory` with `ed25519-dalek` and `k256` crates #522

liamsi Aug 18, 2020 •

edited

Loading

tarcieri Aug 17, 2020 •

edited

Loading

tony-iqlusion commented Aug 18, 2020 •

edited

Loading