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

Replace signatory with ed25519-dalek and k256 crates #522

Merged
merged 1 commit into from
Aug 18, 2020
Merged
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
15 changes: 9 additions & 6 deletions light-client/src/operations/voting_power.rs
Original file line number Diff line number Diff line change
Expand Up @@ -146,10 +146,13 @@ impl VotingPowerCalculator for ProdVotingPowerCalculator {

// Check vote is valid
let sign_bytes = signed_vote.sign_bytes();
if !validator.verify_signature(&sign_bytes, signed_vote.signature()) {
if validator
.verify_signature(&sign_bytes, signed_vote.signature())
.is_err()
{
bail!(VerificationError::InvalidSignature {
signature: signed_vote.signature().to_vec(),
validator,
signature: signed_vote.signature().to_bytes(),
validator: Box::new(validator),
sign_bytes,
});
}
Expand Down Expand Up @@ -186,14 +189,14 @@ fn non_absent_vote(commit_sig: &CommitSig, validator_index: u64, commit: &Commit
} => (
*validator_address,
*timestamp,
signature.clone(),
signature,
Some(commit.block_id.clone()),
),
CommitSig::BlockIDFlagNil {
validator_address,
timestamp,
signature,
} => (*validator_address, *timestamp, signature.clone(), None),
} => (*validator_address, *timestamp, signature, None),
};

Some(Vote {
Expand All @@ -204,7 +207,7 @@ fn non_absent_vote(commit_sig: &CommitSig, validator_index: u64, commit: &Commit
timestamp,
validator_address,
validator_index,
signature,
signature: *signature,
})
}

Expand Down
2 changes: 1 addition & 1 deletion light-client/src/predicates/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ pub enum VerificationError {
/// Signature as a byte array
signature: Vec<u8>,
/// Validator which provided the signature
validator: Validator,
validator: Box<Validator>,
/// Bytes which were signed
sign_bytes: Vec<u8>,
},
Expand Down
13 changes: 6 additions & 7 deletions tendermint/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -27,16 +27,17 @@ authors = [

[package.metadata.docs.rs]
all-features = true

[badges]
codecov = { repository = "..."}
rustdoc-args = ["--cfg", "docsrs"]

[dependencies]
anomaly = "0.2"
async-trait = "0.1"
bytes = "0.5"
chrono = { version = "0.4", features = ["serde"] }
ed25519 = "1"
ed25519-dalek = { version = "= 1.0.0-pre.4", features = ["serde"] }
futures = "0.3"
k256 = { version = "0.4", optional = true, features = ["ecdsa"] }
Copy link
Member

Choose a reason for hiding this comment

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

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

once_cell = "1.3"
prost-amino = "0.6"
prost-amino-derive = "0.6"
Expand All @@ -45,9 +46,7 @@ serde_json = "1"
serde_bytes = "0.11"
serde_repr = "0.1"
sha2 = { version = "0.9", default-features = false }
signatory = { version = "0.20", features = ["ed25519", "ecdsa"] }
signatory-dalek = "0.20"
signatory-secp256k1 = { version = "0.20", optional = true }
signature = "1.2"
subtle = "2"
subtle-encoding = { version = "0.5", features = ["bech32-preview"] }
tai64 = { version = "3", features = ["chrono"] }
Expand All @@ -61,4 +60,4 @@ tendermint-rpc = { path = "../rpc", features = [ "client" ] }
tokio = { version = "0.2", features = [ "macros" ] }

[features]
secp256k1 = ["signatory-secp256k1", "ripemd160"]
secp256k1 = ["k256", "ripemd160"]
33 changes: 18 additions & 15 deletions tendermint/src/account.rs
Original file line number Diff line number Diff line change
@@ -1,20 +1,25 @@
//! Tendermint accounts

use crate::error::{Error, Kind};
#[cfg(feature = "secp256k1")]
use ripemd160::Ripemd160;
use crate::{
error::{Error, Kind},
public_key::Ed25519,
};

use serde::{de, Deserialize, Deserializer, Serialize, Serializer};
use sha2::{Digest, Sha256};
#[cfg(feature = "secp256k1")]
use signatory::ecdsa::curve::secp256k1;
use signatory::ed25519;
use std::{
convert::TryInto,
fmt::{self, Debug, Display},
str::FromStr,
};
use subtle::{self, ConstantTimeEq};
use subtle_encoding::hex;

#[cfg(feature = "secp256k1")]
use crate::public_key::Secp256k1;
#[cfg(feature = "secp256k1")]
use ripemd160::Ripemd160;

/// Size of an account ID in bytes
pub const LENGTH: usize = 20;

Expand Down Expand Up @@ -64,8 +69,8 @@ impl Debug for Id {

// RIPEMD160(SHA256(pk))
#[cfg(feature = "secp256k1")]
impl From<secp256k1::PublicKey> for Id {
fn from(pk: secp256k1::PublicKey) -> Id {
impl From<Secp256k1> for Id {
fn from(pk: Secp256k1) -> Id {
let sha_digest = Sha256::digest(pk.as_bytes());
let ripemd_digest = Ripemd160::digest(&sha_digest[..]);
let mut bytes = [0u8; LENGTH];
Expand All @@ -75,12 +80,10 @@ impl From<secp256k1::PublicKey> for Id {
}

// SHA256(pk)[:20]
impl From<ed25519::PublicKey> for Id {
fn from(pk: ed25519::PublicKey) -> Id {
impl From<Ed25519> for Id {
fn from(pk: Ed25519) -> Id {
let digest = Sha256::digest(pk.as_bytes());
let mut bytes = [0u8; LENGTH];
bytes.copy_from_slice(&digest[..LENGTH]);
Id(bytes)
Id(digest[..LENGTH].try_into().unwrap())
tony-iqlusion marked this conversation as resolved.
Show resolved Hide resolved
}
}

Expand Down Expand Up @@ -141,7 +144,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::PublicKey::from_bytes(pubkey_bytes).unwrap();
let pubkey = Ed25519::from_bytes(pubkey_bytes).unwrap();
let id = Id::from(pubkey);

assert_eq!(id_bytes.ct_eq(&id).unwrap_u8(), 1);
Expand All @@ -160,7 +163,7 @@ mod tests {
let id_bytes = Id::from_str(id_hex).expect("expected id_hex to decode properly");

// get id for pubkey
let pubkey = secp256k1::PublicKey::from_bytes(pubkey_bytes).unwrap();
let pubkey = Secp256k1::from_bytes(pubkey_bytes).unwrap();
let id = Id::from(pubkey);

assert_eq!(id_bytes.ct_eq(&id).unwrap_u8(), 1);
Expand Down
43 changes: 26 additions & 17 deletions tendermint/src/amino_types/ed25519.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,13 @@
use super::compute_prefix;
use crate::public_key::PublicKey;
use crate::{
error,
public_key::{Ed25519, PublicKey},
Error,
};
use anomaly::format_err;
use once_cell::sync::Lazy;
use prost_amino_derive::Message;
use signatory::ed25519::PUBLIC_KEY_SIZE;
use std::convert::TryFrom;

// Note:On the golang side this is generic in the sense that it could everything that implements
// github.com/tendermint/tendermint/crypto.PubKey
Expand All @@ -24,13 +29,15 @@ pub struct PubKeyResponse {
#[amino_name = "tendermint/remotesigner/PubKeyRequest"]
pub struct PubKeyRequest {}

impl From<PubKeyResponse> for PublicKey {
impl TryFrom<PubKeyResponse> for PublicKey {
type Error = Error;

// This does not check if the underlying pub_key_ed25519 has the right size.
// The caller needs to make sure that this is actually the case.
fn from(response: PubKeyResponse) -> PublicKey {
let mut public_key = [0u8; PUBLIC_KEY_SIZE];
public_key.copy_from_slice(response.pub_key_ed25519.as_ref());
PublicKey::Ed25519(signatory::ed25519::PublicKey::new(public_key))
fn try_from(response: PubKeyResponse) -> Result<PublicKey, Error> {
Ed25519::from_bytes(&response.pub_key_ed25519)
.map(Into::into)
.map_err(|_| format_err!(error::Kind::InvalidKey, "malformed Ed25519 key").into())
}
}

Expand All @@ -49,7 +56,9 @@ impl From<PublicKey> for PubKeyResponse {
#[cfg(test)]
mod tests {
use super::*;
use ed25519_dalek::PUBLIC_KEY_LENGTH;
use prost_amino::Message;
use std::convert::TryInto;

#[test]
fn test_empty_pubkey_msg() {
Expand Down Expand Up @@ -140,21 +149,21 @@ mod tests {

#[test]
fn test_into() {
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,
Comment on lines -143 to +155
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link
Member

Choose a reason for hiding this comment

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

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.

];
let want = PublicKey::Ed25519(signatory::ed25519::PublicKey::new(raw_pk));
let want = PublicKey::Ed25519(Ed25519::from_bytes(&raw_pk).unwrap());
let pk = PubKeyResponse {
pub_key_ed25519: vec![
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,
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,
],
};
let orig = pk.clone();
let got: PublicKey = pk.into();
let got: PublicKey = pk.try_into().unwrap();

assert_eq!(got, want);

Expand All @@ -170,6 +179,6 @@ mod tests {
pub_key_ed25519: vec![],
};
// we expect this to panic:
let _got: PublicKey = empty_msg.into();
let _got: PublicKey = empty_msg.try_into().unwrap();
}
}
1 change: 0 additions & 1 deletion tendermint/src/amino_types/proposal.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ use bytes::BufMut;
use once_cell::sync::Lazy;
use prost_amino::{EncodeError, Message};
use prost_amino_derive::Message;
use signatory::ed25519;
use std::convert::TryFrom;

#[derive(Clone, PartialEq, Message)]
Expand Down
1 change: 0 additions & 1 deletion tendermint/src/amino_types/signature.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ use super::validate;
use crate::{chain, consensus};
use bytes::BufMut;
use prost_amino::{DecodeError, EncodeError};
use signatory::ed25519;

/// Amino messages which are signable within a Tendermint network
pub trait SignableMsg {
Expand Down
1 change: 0 additions & 1 deletion tendermint/src/amino_types/vote.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ use bytes::BufMut;
use once_cell::sync::Lazy;
use prost_amino::{error::EncodeError, Message};
use prost_amino_derive::Message;
use signatory::ed25519;
use std::convert::TryFrom;

const VALIDATOR_ADDR_SIZE: usize = 20;
Expand Down
7 changes: 4 additions & 3 deletions tendermint/src/config/node_key.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,10 @@

use crate::{
error::{Error, Kind},
node,
private_key::PrivateKey,
public_key::PublicKey,
};
use crate::{node, public_key::PublicKey};
use anomaly::format_err;
use serde::{Deserialize, Serialize};
use std::{fs, path::Path};
Expand Down Expand Up @@ -42,15 +43,15 @@ impl NodeKey {
/// Get the public key for this keypair
pub fn public_key(&self) -> PublicKey {
match &self.priv_key {
PrivateKey::Ed25519(key) => key.public_key(),
PrivateKey::Ed25519(keypair) => keypair.public.into(),
}
}

/// Get node ID for this keypair
pub fn node_id(&self) -> node::Id {
#[allow(unreachable_patterns)]
match &self.public_key() {
PublicKey::Ed25519(key) => node::Id::from(*key),
PublicKey::Ed25519(pubkey) => node::Id::from(*pubkey),
_ => unreachable!(),
}
}
Expand Down
5 changes: 3 additions & 2 deletions tendermint/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
//! blockchain networks, including chain information types, secret connections,
//! and remote procedure calls (JSONRPC).

#![cfg_attr(docsrs, feature(doc_cfg))]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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:

Screen Shot 2020-08-12 at 8 26 45 PM

Copy link
Member

Choose a reason for hiding this comment

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

We should add this to all our other crates too.

#![deny(
warnings,
missing_docs,
Expand Down Expand Up @@ -50,17 +51,17 @@ pub mod vote;
#[cfg(test)]
mod test;

pub use crate::genesis::Genesis;
pub use crate::{
block::Block,
error::{Error, Kind},
genesis::Genesis,
hash::Hash,
moniker::Moniker,
private_key::PrivateKey,
public_key::{PublicKey, TendermintKey},
signature::Signature,
time::Time,
timeout::Timeout,
version::Version,
vote::Vote,
};
pub use private_key::PrivateKey;
11 changes: 7 additions & 4 deletions tendermint/src/node/id.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
//! Tendermint node IDs

use crate::error::{Error, Kind};
use crate::{
error::{Error, Kind},
public_key::Ed25519,
};

use serde::{de, Deserialize, Deserializer, Serialize, Serializer};
use sha2::{Digest, Sha256};
use signatory::ed25519;
use std::{
fmt::{self, Debug, Display},
str::FromStr,
Expand Down Expand Up @@ -58,8 +61,8 @@ impl Debug for Id {
}
}

impl From<ed25519::PublicKey> for Id {
fn from(pk: ed25519::PublicKey) -> Id {
impl From<Ed25519> for Id {
fn from(pk: Ed25519) -> Id {
let digest = Sha256::digest(pk.as_bytes());
let mut bytes = [0u8; LENGTH];
bytes.copy_from_slice(&digest[..LENGTH]);
Expand Down
Loading