deps: Use ed25519-consensus instead of ed25519-dalek (informalsystems…

…#1067) * Use ed25519-consensus instead of ed25519-dalek Closes informalsystems#355 (see that issue for more context; `ed25519-consensus` is a fork of `ed25519-zebra` that's Zcash-independent). This change ensures that `tendermint-rs` has the same signature verification as `tendermint`, which uses the validation criteria provided by the Go `ed25519consensus` library. * clippy fixes Co-authored-by: Thane Thomson <[email protected]> * Remove redundant dependency Signed-off-by: Thane Thomson <[email protected]> * cargo fmt Signed-off-by: Thane Thomson <[email protected]> * Add changelog entries Signed-off-by: Thane Thomson <[email protected]> Co-authored-by: Henry de Valence <[email protected]>
penumbra-zone · Dec 6, 2022 · b2e822a · b2e822a
1 parent e9c41de
commit b2e822a
Show file tree

Hide file tree

Showing 21 changed files with 122 additions and 111 deletions.
diff --git a/.changelog/unreleased/dependencies/1046-ed25519-consensus-dep.md b/.changelog/unreleased/dependencies/1046-ed25519-consensus-dep.md
@@ -0,0 +1,3 @@
+- `[tendermint, tendermint-p2p]` Replaced the `ed25519-dalek` dependency with
+  `ed25519-consensus`
+  ([#1046](https://github.com/informalsystems/tendermint-rs/pull/1046))
diff --git a/config/src/node_key.rs b/config/src/node_key.rs
@@ -36,7 +36,7 @@ impl NodeKey {
     pub fn public_key(&self) -> PublicKey {
         #[allow(unreachable_patterns)]
         match &self.priv_key {
-            PrivateKey::Ed25519(keypair) => keypair.public.into(),
+            PrivateKey::Ed25519(signing_key) => PublicKey::Ed25519(signing_key.verification_key()),
             _ => unreachable!(),
         }
     }

diff --git a/p2p/Cargo.toml b/p2p/Cargo.toml
@@ -1,7 +1,7 @@
 [package]
 name        = "tendermint-p2p"
 version     = "0.27.0"
-edition     = "2018"
+edition     = "2021"
 license     = "Apache-2.0"
 repository  = "https://github.com/informalsystems/tendermint-rs"
 homepage    = "https://tendermint.com"
@@ -28,7 +28,7 @@ amino = ["prost-derive"]
 
 [dependencies]
 chacha20poly1305 = { version = "0.8", default-features = false, features = ["reduced-round"] }
-ed25519-dalek = { version = "1", default-features = false }
+ed25519-consensus = { version = "2", default-features = false }
 eyre = { version = "0.6", default-features = false }
 flume = { version = "0.10.7", default-features = false }
 hkdf = { version = "0.10.0", default-features = false }
@@ -37,7 +37,7 @@ prost = { version = "0.11", default-features = false }
 rand_core = { version = "0.5", default-features = false, features = ["std"] }
 sha2 = { version = "0.9", default-features = false }
 subtle = { version = "2", default-features = false }
-x25519-dalek = { version = "1.1", default-features = false }
+x25519-dalek = { version = "1.1", default-features = false, features = ["u64_backend"] }
 zeroize = { version = "1", default-features = false }
 signature = { version = "1", default-features = false }
 aead = { version = "0.4.1", default-features = false }

diff --git a/p2p/src/error.rs b/p2p/src/error.rs
@@ -6,7 +6,6 @@
 
 use flex_error::{define_error, DisplayOnly};
 use prost::DecodeError;
-use signature::Error as SignatureError;
 
 define_error! {
     Error {
@@ -40,7 +39,6 @@ define_error! {
             | _ | { "public key missing" },
 
         Signature
-            [ DisplayOnly<SignatureError> ]
             | _ | { "signature error" },
 
         UnsupportedKey

diff --git a/p2p/src/secret_connection.rs b/p2p/src/secret_connection.rs
@@ -16,7 +16,6 @@ use chacha20poly1305::{
     aead::{generic_array::GenericArray, AeadInPlace, NewAead},
     ChaCha20Poly1305,
 };
-use ed25519_dalek::{self as ed25519, Signer, Verifier};
 use merlin::Transcript;
 use rand_core::OsRng;
 use subtle::ConstantTimeEq;
@@ -61,7 +60,7 @@ pub struct Handshake<S> {
 
 /// `AwaitingEphKey` means we're waiting for the remote ephemeral pubkey.
 pub struct AwaitingEphKey {
-    local_privkey: ed25519::Keypair,
+    local_privkey: ed25519_consensus::SigningKey,
     local_eph_privkey: Option<EphemeralSecret>,
 }
 
@@ -71,15 +70,15 @@ pub struct AwaitingAuthSig {
     kdf: Kdf,
     recv_cipher: ChaCha20Poly1305,
     send_cipher: ChaCha20Poly1305,
-    local_signature: ed25519::Signature,
+    local_signature: ed25519_consensus::Signature,
 }
 
 #[allow(clippy::use_self)]
 impl Handshake<AwaitingEphKey> {
     /// Initiate a handshake.
     #[must_use]
     pub fn new(
-        local_privkey: ed25519::Keypair,
+        local_privkey: ed25519_consensus::SigningKey,
         protocol_version: Version,
     ) -> (Self, EphemeralPublic) {
         // Generate an ephemeral key for perfect forward secrecy.
@@ -151,9 +150,9 @@ impl Handshake<AwaitingEphKey> {
 
         // Sign the challenge bytes for authentication.
         let local_signature = if self.protocol_version.has_transcript() {
-            sign_challenge(&sc_mac, &self.state.local_privkey)?
+            self.state.local_privkey.sign(&sc_mac)
         } else {
-            sign_challenge(&kdf.challenge, &self.state.local_privkey)?
+            self.state.local_privkey.sign(&kdf.challenge)
         };
 
         Ok(Handshake {
@@ -186,22 +185,23 @@ impl Handshake<AwaitingAuthSig> {
 
         let remote_pubkey = match pk_sum {
             proto::crypto::public_key::Sum::Ed25519(ref bytes) => {
-                ed25519::PublicKey::from_bytes(bytes).map_err(Error::signature)
+                ed25519_consensus::VerificationKey::try_from(&bytes[..])
+                    .map_err(|_| Error::signature())
             },
-            proto::crypto::public_key::Sum::Secp256k1(_) => Err(Error::unsupported_key()),
+            _ => Err(Error::unsupported_key()),
         }?;
 
-        let remote_sig =
-            ed25519::Signature::try_from(auth_sig_msg.sig.as_slice()).map_err(Error::signature)?;
+        let remote_sig = ed25519_consensus::Signature::try_from(auth_sig_msg.sig.as_slice())
+            .map_err(|_| Error::signature())?;
 
         if self.protocol_version.has_transcript() {
             remote_pubkey
-                .verify(&self.state.sc_mac, &remote_sig)
-                .map_err(Error::signature)?;
+                .verify(&remote_sig, &self.state.sc_mac)
+                .map_err(|_| Error::signature())?;
         } else {
             remote_pubkey
-                .verify(&self.state.kdf.challenge, &remote_sig)
-                .map_err(Error::signature)?;
+                .verify(&remote_sig, &self.state.kdf.challenge)
+                .map_err(|_| Error::signature())?;
         }
 
         // We've authorized.
@@ -279,7 +279,7 @@ impl<IoHandler: Read + Write + Send + Sync> SecretConnection<IoHandler> {
     /// * if receiving the signature fails
     pub fn new(
         mut io_handler: IoHandler,
-        local_privkey: ed25519::Keypair,
+        local_privkey: ed25519_consensus::SigningKey,
         protocol_version: Version,
     ) -> Result<Self, Error> {
         // Start a handshake process.
@@ -470,20 +470,12 @@ fn share_eph_pubkey<IoHandler: Read + Write + Send + Sync>(
     protocol_version.decode_initial_handshake(&buf)
 }
 
-/// Sign the challenge with the local private key
-fn sign_challenge(
-    challenge: &[u8; 32],
-    local_privkey: &dyn Signer<ed25519::Signature>,
-) -> Result<ed25519::Signature, Error> {
-    local_privkey.try_sign(challenge).map_err(Error::signature)
-}
-
 // TODO(ismail): change from DecodeError to something more generic
 // this can also fail while writing / sending
 fn share_auth_signature<IoHandler: Read + Write + Send + Sync>(
     sc: &mut SecretConnection<IoHandler>,
-    pubkey: &ed25519::PublicKey,
-    local_signature: &ed25519::Signature,
+    pubkey: &ed25519_consensus::VerificationKey,
+    local_signature: &ed25519_consensus::Signature,
 ) -> Result<proto::p2p::AuthSigMessage, Error> {
     let buf = sc
         .protocol_version

diff --git a/p2p/src/secret_connection/amino_types.rs b/p2p/src/secret_connection/amino_types.rs
@@ -1,8 +1,6 @@
 //! Amino types used by Secret Connection
 
 use core::convert::TryFrom;
-
-use ed25519_dalek as ed25519;
 use prost_derive::Message;
 use tendermint_proto as proto;
 
@@ -24,13 +22,16 @@ pub struct AuthSigMessage {
 }
 
 impl AuthSigMessage {
-    pub fn new(pub_key: &ed25519::PublicKey, sig: &ed25519::Signature) -> Self {
+    pub fn new(
+        pub_key: &ed25519_consensus::VerificationKey,
+        sig: &ed25519_consensus::Signature,
+    ) -> Self {
         let mut pub_key_bytes = Vec::from(PUB_KEY_ED25519_AMINO_PREFIX);
-        pub_key_bytes.extend_from_slice(pub_key.as_ref());
+        pub_key_bytes.extend_from_slice(pub_key.as_bytes());
 
         Self {
             pub_key: pub_key_bytes,
-            sig: sig.as_ref().to_vec(),
+            sig: sig.to_bytes().to_vec(),
         }
     }
 }

diff --git a/p2p/src/secret_connection/protocol.rs b/p2p/src/secret_connection/protocol.rs
@@ -2,7 +2,6 @@
 
 use std::convert::TryInto;
 
-use ed25519_dalek as ed25519;
 use prost::Message as _;
 use tendermint_proto as proto;
 use x25519_dalek::PublicKey as EphemeralPublic;
@@ -110,8 +109,8 @@ impl Version {
     #[must_use]
     pub fn encode_auth_signature(
         self,
-        pub_key: &ed25519::PublicKey,
-        signature: &ed25519::Signature,
+        pub_key: &ed25519_consensus::VerificationKey,
+        signature: &ed25519_consensus::Signature,
     ) -> Vec<u8> {
         if self.is_protobuf() {
             // Protobuf `AuthSigMessage`
@@ -123,7 +122,7 @@ impl Version {
 
             let msg = proto::p2p::AuthSigMessage {
                 pub_key: Some(pub_key),
-                sig: signature.as_ref().to_vec(),
+                sig: signature.to_bytes().to_vec(),
             };
 
             let mut buf = Vec::new();
@@ -165,8 +164,8 @@ impl Version {
     #[cfg(feature = "amino")]
     fn encode_auth_signature_amino(
         self,
-        pub_key: &ed25519::PublicKey,
-        signature: &ed25519::Signature,
+        pub_key: &ed25519_consensus::VerificationKey,
+        signature: &ed25519_consensus::Signature,
     ) -> Vec<u8> {
         // Legacy Amino encoded `AuthSigMessage`
         let msg = amino_types::AuthSigMessage::new(pub_key, signature);
@@ -181,8 +180,8 @@ impl Version {
     #[cfg(not(feature = "amino"))]
     const fn encode_auth_signature_amino(
         self,
-        _: &ed25519::PublicKey,
-        _: &ed25519::Signature,
+        _: &ed25519_consensus::VerificationKey,
+        _: &ed25519_consensus::Signature,
     ) -> Vec<u8> {
         panic!("attempted to encode auth signature using amino, but 'amino' feature is not present")
     }

diff --git a/p2p/src/secret_connection/public_key.rs b/p2p/src/secret_connection/public_key.rs
@@ -2,15 +2,14 @@
 
 use std::fmt::{self, Display};
 
-use ed25519_dalek as ed25519;
 use sha2::{digest::Digest, Sha256};
 use tendermint::{error::Error, node};
 
 /// Secret Connection peer public keys (signing, presently Ed25519-only)
 #[derive(Copy, Clone, Debug, Eq, PartialEq)]
 pub enum PublicKey {
     /// Ed25519 Secret Connection Keys
-    Ed25519(ed25519::PublicKey),
+    Ed25519(ed25519_consensus::VerificationKey),
 }
 
 impl PublicKey {
@@ -20,14 +19,14 @@ impl PublicKey {
     ///
     /// * if the bytes given are invalid
     pub fn from_raw_ed25519(bytes: &[u8]) -> Result<Self, Error> {
-        ed25519::PublicKey::from_bytes(bytes)
+        ed25519_consensus::VerificationKey::try_from(bytes)
             .map(Self::Ed25519)
-            .map_err(Error::signature)
+            .map_err(|_| Error::signature())
     }
 
     /// Get Ed25519 public key
     #[must_use]
-    pub const fn ed25519(self) -> Option<ed25519::PublicKey> {
+    pub const fn ed25519(self) -> Option<ed25519_consensus::VerificationKey> {
         match self {
             Self::Ed25519(pk) => Some(pk),
         }
@@ -54,14 +53,14 @@ impl Display for PublicKey {
     }
 }
 
-impl From<&ed25519::Keypair> for PublicKey {
-    fn from(sk: &ed25519::Keypair) -> Self {
-        Self::Ed25519(sk.public)
+impl From<&ed25519_consensus::SigningKey> for PublicKey {
+    fn from(sk: &ed25519_consensus::SigningKey) -> Self {
+        Self::Ed25519(sk.verification_key())
     }
 }
 
-impl From<ed25519::PublicKey> for PublicKey {
-    fn from(pk: ed25519::PublicKey) -> Self {
+impl From<ed25519_consensus::VerificationKey> for PublicKey {
+    fn from(pk: ed25519_consensus::VerificationKey) -> Self {
         Self::Ed25519(pk)
     }
 }
diff --git a/tendermint/Cargo.toml b/tendermint/Cargo.toml
@@ -32,7 +32,7 @@ rustdoc-args = ["--cfg", "docsrs"]
 [dependencies]
 bytes = { version = "1.0", default-features = false, features = ["serde"] }
 ed25519 = { version = "1.3", default-features = false }
-ed25519-dalek = { version = "1", default-features = false, features = ["u64_backend"] }
+ed25519-consensus = { version = "2", default-features = false }
 futures = { version = "0.3", default-features = false }
 num-traits = { version = "0.2", default-features = false }
 once_cell = { version = "1.3", default-features = false }

diff --git a/tendermint/src/account.rs b/tendermint/src/account.rs
@@ -161,7 +161,7 @@ mod tests {
         let id_bytes = Id::from_str(id_hex).expect("expected id_hex to decode properly");
 
         // get id for pubkey
-        let pubkey = Ed25519::from_bytes(pubkey_bytes).unwrap();
+        let pubkey = Ed25519::try_from(&pubkey_bytes[..]).unwrap();
         let id = Id::from(pubkey);
 
         assert_eq!(id_bytes.ct_eq(&id).unwrap_u8(), 1);

diff --git a/tendermint/src/error.rs b/tendermint/src/error.rs
@@ -209,8 +209,7 @@ define_error! {
             |_| { format_args!("subtle encoding error") },
 
         Signature
-            [ DisplayOnly<signature::Error> ]
-            |_| { format_args!("signature error") },
+            |_| { "signature error" },
 
         TrustThresholdTooLarge
             |_| { "trust threshold is too large (must be <= 1)" },

diff --git a/tendermint/src/private_key.rs b/tendermint/src/private_key.rs
@@ -1,11 +1,15 @@
 //! Cryptographic private keys
 
-pub use ed25519_dalek::{Keypair as Ed25519, EXPANDED_SECRET_KEY_LENGTH as ED25519_KEYPAIR_SIZE};
+pub use ed25519_consensus::SigningKey as Ed25519;
+
+use crate::prelude::*;
+use crate::public_key::PublicKey;
+use ed25519_consensus::VerificationKey;
 use serde::{de, ser, Deserialize, Serialize};
 use subtle_encoding::{Base64, Encoding};
 use zeroize::Zeroizing;
 
-use crate::{prelude::*, public_key::PublicKey};
+pub const ED25519_KEYPAIR_SIZE: usize = 64;
 
 /// Private keys as parsed from configuration files
 #[derive(Serialize, Deserialize)]
@@ -25,24 +29,28 @@ impl PrivateKey {
     /// Get the public key associated with this private key
     pub fn public_key(&self) -> PublicKey {
         match self {
-            PrivateKey::Ed25519(private_key) => private_key.public.into(),
+            PrivateKey::Ed25519(signing_key) => PublicKey::Ed25519(signing_key.verification_key()),
         }
     }
 
     /// If applicable, borrow the Ed25519 keypair
-    pub fn ed25519_keypair(&self) -> Option<&Ed25519> {
+    pub fn ed25519_signing_key(&self) -> Option<&Ed25519> {
         match self {
-            PrivateKey::Ed25519(keypair) => Some(keypair),
+            PrivateKey::Ed25519(signing_key) => Some(signing_key),
         }
     }
 }
 
 /// Serialize an Ed25519 keypair as Base64
-fn serialize_ed25519_keypair<S>(keypair: &Ed25519, serializer: S) -> Result<S::Ok, S::Error>
+fn serialize_ed25519_keypair<S>(signing_key: &Ed25519, serializer: S) -> Result<S::Ok, S::Error>
 where
     S: ser::Serializer,
 {
-    let keypair_bytes = Zeroizing::new(keypair.to_bytes());
+    // Tendermint uses a serialization format inherited from Go that includes
+    // a cached copy of the public key as the second half.
+    let mut keypair_bytes = Zeroizing::new([0u8; ED25519_KEYPAIR_SIZE]);
+    keypair_bytes[0..32].copy_from_slice(signing_key.as_bytes());
+    keypair_bytes[32..64].copy_from_slice(signing_key.verification_key().as_bytes());
     Zeroizing::new(String::from_utf8(Base64::default().encode(&keypair_bytes[..])).unwrap())
         .serialize(serializer)
 }
@@ -63,5 +71,17 @@ where
         return Err(D::Error::custom("invalid Ed25519 keypair size"));
     }
 
-    Ed25519::from_bytes(&*keypair_bytes).map_err(D::Error::custom)
+    // Tendermint uses a serialization format inherited from Go that includes a
+    // cached copy of the public key as the second half.  This is somewhat
+    // dangerous, since there's no validation that the two parts are consistent
+    // with each other, so we ignore the second half and just check consistency
+    // with the re-derived data.
+    let signing_key = Ed25519::try_from(&keypair_bytes[0..32])
+        .map_err(|_| D::Error::custom("invalid signing key"))?;
+    let verification_key = VerificationKey::from(&signing_key);
+    if &keypair_bytes[32..64] != verification_key.as_bytes() {
+        return Err(D::Error::custom("keypair mismatch"));
+    }
+
+    Ok(signing_key)
 }