Skip to content

Commit

Permalink
Use ed25519-consensus instead of ed25519-dalek
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
hdevalence committed Dec 8, 2021
1 parent f739e86 commit 0cf605d
Show file tree
Hide file tree
Showing 23 changed files with 117 additions and 107 deletions.
2 changes: 1 addition & 1 deletion config/src/node_key.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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!(),
}
}
Expand Down
4 changes: 2 additions & 2 deletions p2p/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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 = "1.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 }
Expand All @@ -37,7 +37,7 @@ prost = { version = "0.9", 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.3.0", default-features = false }
aead = { version = "0.4.1", default-features = false }
Expand Down
2 changes: 0 additions & 2 deletions p2p/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@

use flex_error::{define_error, DisplayOnly};
use prost::DecodeError;
use signature::Error as SignatureError;

define_error! {
Error {
Expand Down Expand Up @@ -36,7 +35,6 @@ define_error! {
| _ | { "public key missing" },

Signature
[ DisplayOnly<SignatureError> ]
| _ | { "signature error" },

UnsupportedKey
Expand Down
40 changes: 16 additions & 24 deletions p2p/src/secret_connection.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,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;
Expand Down Expand Up @@ -58,7 +57,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>,
}

Expand All @@ -68,15 +67,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.
Expand Down Expand Up @@ -148,9 +147,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 {
Expand Down Expand Up @@ -183,22 +182,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())
}
_ => 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.
Expand Down Expand Up @@ -276,7 +276,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.
Expand Down Expand Up @@ -467,20 +467,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
Expand Down
10 changes: 6 additions & 4 deletions p2p/src/secret_connection/amino_types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@

use crate::error::Error;
use core::convert::TryFrom;
use ed25519_dalek as ed25519;
use prost_derive::Message;
use tendermint_proto as proto;

Expand All @@ -22,13 +21,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(),
}
}
}
Expand Down
15 changes: 7 additions & 8 deletions p2p/src/secret_connection/protocol.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@

use std::convert::TryInto;

use ed25519_dalek as ed25519;
use prost::Message as _;

use x25519_dalek::PublicKey as EphemeralPublic;
Expand Down Expand Up @@ -113,8 +112,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`
Expand All @@ -126,7 +125,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();
Expand Down Expand Up @@ -168,8 +167,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);
Expand All @@ -184,8 +183,8 @@ impl Version {
#[cfg(not(feature = "amino"))]
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")
}
Expand Down
24 changes: 13 additions & 11 deletions p2p/src/secret_connection/public_key.rs
Original file line number Diff line number Diff line change
@@ -1,15 +1,17 @@
//! Secret Connection peer public keys

use ed25519_dalek as ed25519;
use sha2::{digest::Digest, Sha256};
use std::fmt::{self, Display};
use std::{
convert::TryFrom,
fmt::{self, Display},
};
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 {
Expand All @@ -19,14 +21,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),
}
Expand All @@ -53,14 +55,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)
}
}
4 changes: 0 additions & 4 deletions pbt-gen/src/time.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ pub const MAX_NANO_SECS: u32 = 999_999_999u32;
/// let timestamp = pbt_gen::time::min_time().unix_timestamp_nanos();
/// assert!(OffsetDateTime::from_unix_timestamp_nanos(timestamp).is_ok());
/// assert!(OffsetDateTime::from_unix_timestamp_nanos(timestamp - 1).is_err());
///
/// ```
pub fn min_time() -> OffsetDateTime {
Date::MIN.midnight().assume_utc()
Expand Down Expand Up @@ -49,7 +48,6 @@ pub fn max_time() -> OffsetDateTime {
/// Google's well-known [`Timestamp`] protobuf message format.
///
/// [`Timestamp`]: https://developers.google.com/protocol-buffers/docs/reference/google.protobuf#google.protobuf.Timestamp
///
pub const fn min_protobuf_time() -> OffsetDateTime {
datetime!(0001-01-01 00:00:00 UTC)
}
Expand All @@ -58,7 +56,6 @@ pub const fn min_protobuf_time() -> OffsetDateTime {
/// Google's well-known [`Timestamp`] protobuf message format.
///
/// [`Timestamp`]: https://developers.google.com/protocol-buffers/docs/reference/google.protobuf#google.protobuf.Timestamp
///
pub const fn max_protobuf_time() -> OffsetDateTime {
datetime!(9999-12-31 23:59:59.999999999 UTC)
}
Expand Down Expand Up @@ -248,7 +245,6 @@ prop_compose! {
/// Google's well-known [`Timestamp`] protobuf message format.
///
/// [`Timestamp`]: https://developers.google.com/protocol-buffers/docs/reference/google.protobuf#google.protobuf.Timestamp
///
pub fn arb_protobuf_safe_rfc3339_timestamp() -> impl Strategy<Value = String> {
arb_rfc3339_timestamp().prop_filter("timestamp out of protobuf range", |ts| {
let t = OffsetDateTime::parse(ts, &Rfc3339).unwrap();
Expand Down
1 change: 0 additions & 1 deletion proto/src/serializers/timestamp.rs
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,6 @@ pub fn to_rfc3339_nanos(t: OffsetDateTime) -> String {
///
/// [`Display`]: core::fmt::Display
/// [`Debug`]: core::fmt::Debug
///
pub fn fmt_as_rfc3339_nanos(t: OffsetDateTime, f: &mut impl fmt::Write) -> fmt::Result {
let t = t.to_offset(offset!(UTC));
let nanos = t.nanosecond();
Expand Down
2 changes: 1 addition & 1 deletion tendermint/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ crate-type = ["cdylib", "rlib"]
async-trait = { version = "0.1", default-features = false }
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 = "1.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 }
Expand Down
2 changes: 1 addition & 1 deletion tendermint/src/account.rs
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,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);
Expand Down
3 changes: 1 addition & 2 deletions tendermint/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -208,8 +208,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)" },
Expand Down
Loading

0 comments on commit 0cf605d

Please sign in to comment.