Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use ed25519-consensus instead of ed25519-dalek #1046

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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" },
Comment on lines 37 to 38
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it not be worthwhile to bubble up the original error here? Unfortunately it seems like the ed25519_consensus::Error type doesn't implement std::fmt::Display unless you turn on the std feature.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The motivation for removing the original error was that the code this is replacing used an opaque error type, so there's no additional information conveyed by the original error, and was easier to just remove it rather than patch the ed25519_consensus::Error type to have an extra Display impl. But I could add that Display impl in another release.


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(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this method's even necessary. Why would one want to convert a compile-time error to a runtime error like this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure, I tried to leave the rest of the code untouched as a drop-in change.

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" },
Comment on lines 210 to +211
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to my other comment: should we not find a way here to bubble up the original error?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


TrustThresholdTooLarge
|_| { "trust threshold is too large (must be <= 1)" },
Expand Down
Loading