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

make dependency secp256k1 optional, fix #391 #441

Merged
merged 1 commit into from
Jul 13, 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
3 changes: 3 additions & 0 deletions light-client/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -26,3 +26,6 @@ tokio = "0.2.20"
[dev-dependencies]
serde_json = "1.0.51"
gumdrop = "0.8.0"

[features]
secp256k1 = ["tendermint/secp256k1", "tendermint-rpc/secp256k1"]
1 change: 1 addition & 0 deletions rpc/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ all-features = true
[features]
default = []
client = [ "async-tungstenite", "futures", "http", "hyper", "tokio" ]
secp256k1 = ["tendermint/secp256k1"]

[dependencies]
bytes = "0.5"
Expand Down
7 changes: 5 additions & 2 deletions tendermint/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -47,15 +47,18 @@ serde_repr = "0.1"
sha2 = { version = "0.9", default-features = false }
signatory = { version = "0.20", features = ["ed25519", "ecdsa"] }
signatory-dalek = "0.20"
signatory-secp256k1 = "0.20"
signatory-secp256k1 = { version = "0.20", optional = true }
subtle = "2"
subtle-encoding = { version = "0.5", features = ["bech32-preview"] }
tai64 = { version = "3", features = ["chrono"] }
thiserror = "1"
toml = { version = "0.5" }
zeroize = { version = "1.1", features = ["zeroize_derive"] }
ripemd160 = "0.9"
ripemd160 = { version = "0.9", optional = true }

[dev-dependencies]
tendermint-rpc = { path = "../rpc", features = [ "client" ] }
tokio = { version = "0.2", features = [ "macros" ] }

[features]
secp256k1 = ["signatory-secp256k1", "ripemd160"]
7 changes: 6 additions & 1 deletion tendermint/src/account.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,13 @@
//! Tendermint accounts

use crate::error::{Error, Kind};
#[cfg(feature = "secp256k1")]
use ripemd160::Ripemd160;
use serde::{de, Deserialize, Deserializer, Serialize, Serializer};
use sha2::{Digest, Sha256};
use signatory::{ecdsa::curve::secp256k1, ed25519};
#[cfg(feature = "secp256k1")]
use signatory::ecdsa::curve::secp256k1;
use signatory::ed25519;
use std::{
fmt::{self, Debug, Display},
str::FromStr,
Expand Down Expand Up @@ -60,6 +63,7 @@ impl Debug for Id {
}

// RIPEMD160(SHA256(pk))
#[cfg(feature = "secp256k1")]
impl From<secp256k1::PublicKey> for Id {
fn from(pk: secp256k1::PublicKey) -> Id {
let sha_digest = Sha256::digest(pk.as_bytes());
Expand Down Expand Up @@ -144,6 +148,7 @@ mod tests {
}

#[test]
#[cfg(feature = "secp256k1")]
fn test_secp_id() {
// test vector for pubkey and id (address)
let pubkey_hex = "02950E1CDFCB133D6024109FD489F734EEB4502418E538C28481F22BCE276F248C";
Expand Down
1 change: 1 addition & 0 deletions tendermint/src/amino_types/ed25519.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ impl From<PublicKey> for PubKeyResponse {
PublicKey::Ed25519(ref pk) => PubKeyResponse {
pub_key_ed25519: pk.as_bytes().to_vec(),
},
#[cfg(feature = "secp256k1")]
PublicKey::Secp256k1(_) => panic!("secp256k1 PubKeyResponse unimplemented"),
}
}
Expand Down
1 change: 1 addition & 0 deletions tendermint/src/config/node_key.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ impl NodeKey {

/// 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),
_ => unreachable!(),
Expand Down
25 changes: 18 additions & 7 deletions tendermint/src/public_key.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@
use crate::error::{Error, Kind};
use anomaly::fail;
use serde::{de::Error as _, Deserialize, Deserializer, Serialize, Serializer};
use signatory::{ecdsa::curve::secp256k1, ed25519};
#[cfg(feature = "secp256k1")]
Copy link
Member

@liamsi liamsi Jul 13, 2020

Choose a reason for hiding this comment

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

If we moved everything secp-related into a separate file/module, we can likely just annotate it on the mod level instead? Not blocking this PR but probably worth separating this out (if possible)?

use signatory::ecdsa::curve::secp256k1;
use signatory::ed25519;
use std::{
fmt::{self, Display},
ops::Deref,
Expand All @@ -26,6 +28,7 @@ pub enum PublicKey {
Ed25519(ed25519::PublicKey),

/// Secp256k1 keys
#[cfg(feature = "secp256k1")]
#[serde(
rename = "tendermint/PubKeySecp256k1",
serialize_with = "serialize_secp256k1_base64",
Expand All @@ -36,6 +39,7 @@ pub enum PublicKey {

impl PublicKey {
/// From raw secp256k1 public key bytes
#[cfg(feature = "secp256k1")]
pub fn from_raw_secp256k1(bytes: &[u8]) -> Option<PublicKey> {
Some(PublicKey::Secp256k1(secp256k1::PublicKey::from_bytes(
bytes,
Expand All @@ -49,6 +53,7 @@ impl PublicKey {

/// Get Ed25519 public key
pub fn ed25519(self) -> Option<ed25519::PublicKey> {
#[allow(unreachable_patterns)]
Copy link
Member

@romac romac Jul 13, 2020

Choose a reason for hiding this comment

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

I understand that those _ => unreachable!() patterns were here previously to this PR, but just wanted to mention that those seem inherently unsafe to me (as in: may lead to panics). This PR improves the state of things by making them actually unreachable if the secp265k1 feature is not enabled (hence the need for the annotation), but we should perhaps take a closer look at them in a follow-up issue.

match self {
PublicKey::Ed25519(pk) => Some(pk),
_ => None,
Expand All @@ -59,6 +64,7 @@ impl PublicKey {
pub fn as_bytes(self) -> Vec<u8> {
match self {
PublicKey::Ed25519(ref pk) => pk.as_bytes(),
#[cfg(feature = "secp256k1")]
PublicKey::Secp256k1(ref pk) => pk.as_bytes(),
}
.to_vec()
Expand All @@ -73,6 +79,7 @@ impl PublicKey {
key_bytes.extend(pk.as_bytes());
key_bytes
}
#[cfg(feature = "secp256k1")]
PublicKey::Secp256k1(ref pk) => {
let mut key_bytes = vec![0xEB, 0x5A, 0xE9, 0x87, 0x21];
key_bytes.extend(pk.as_bytes());
Expand All @@ -98,6 +105,7 @@ impl From<ed25519::PublicKey> for PublicKey {
}
}

#[cfg(feature = "secp256k1")]
impl From<secp256k1::PublicKey> for PublicKey {
fn from(pk: secp256k1::PublicKey) -> PublicKey {
PublicKey::Secp256k1(pk)
Expand All @@ -118,14 +126,15 @@ impl TendermintKey {
/// Create a new account key from a `PublicKey`
pub fn new_account_key(public_key: PublicKey) -> Result<TendermintKey, Error> {
match public_key {
PublicKey::Ed25519(_) | PublicKey::Secp256k1(_) => {
Ok(TendermintKey::AccountKey(public_key))
}
PublicKey::Ed25519(_) => Ok(TendermintKey::AccountKey(public_key)),
#[cfg(feature = "secp256k1")]
PublicKey::Secp256k1(_) => Ok(TendermintKey::AccountKey(public_key)),
}
}

/// Create a new consensus key from a `PublicKey`
pub fn new_consensus_key(public_key: PublicKey) -> Result<TendermintKey, Error> {
#[allow(unreachable_patterns)]
match public_key {
PublicKey::Ed25519(_) => Ok(TendermintKey::AccountKey(public_key)),
_ => fail!(
Expand Down Expand Up @@ -209,6 +218,7 @@ where
}

/// Serialize the bytes of a secp256k1 ECDSA public key as Base64. Used for serializing JSON
#[cfg(feature = "secp256k1")]
fn serialize_secp256k1_base64<S>(
pk: &secp256k1::PublicKey,
serializer: S,
Expand All @@ -231,6 +241,7 @@ where
ed25519::PublicKey::from_bytes(&bytes).ok_or_else(|| D::Error::custom("invalid ed25519 key"))
}

#[cfg(feature = "secp256k1")]
fn deserialize_secp256k1_base64<'de, D>(
deserializer: D,
) -> Result<signatory::ecdsa::curve::secp256k1::PublicKey, D::Error>
Expand Down Expand Up @@ -265,11 +276,11 @@ mod tests {
);
}

const EXAMPLE_ACCOUNT_KEY: &str =
"02A1633CAFCC01EBFB6D78E39F687A1F0995C62FC95F51EAD10A02EE0BE551B5DC";

#[test]
#[cfg(feature = "secp256k1")]
fn test_account_serialization() {
const EXAMPLE_ACCOUNT_KEY: &str =
"02A1633CAFCC01EBFB6D78E39F687A1F0995C62FC95F51EAD10A02EE0BE551B5DC";
let example_key = TendermintKey::AccountKey(
PublicKey::from_raw_secp256k1(&hex::decode_upper(EXAMPLE_ACCOUNT_KEY).unwrap())
.unwrap(),
Expand Down
1 change: 1 addition & 0 deletions tendermint/src/validator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@ impl From<PublicKey> for account::Id {
fn from(pub_key: PublicKey) -> account::Id {
match pub_key {
PublicKey::Ed25519(pk) => account::Id::from(pk),
#[cfg(feature = "secp256k1")]
PublicKey::Secp256k1(pk) => account::Id::from(pk),
}
}
Expand Down