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

Add support for secp256k1 consensus keys #1358

Merged
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
2 changes: 2 additions & 0 deletions .changelog/unreleased/features/1364-secp256k1-keys.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
- `[tendermint]` Add support for secp256k1 consensus keys
([\#1364](https://github.com/informalsystems/tendermint-rs/issues/1364))
3 changes: 3 additions & 0 deletions config/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -33,3 +33,6 @@ url = { version = "2.2" }

[dev-dependencies]
pretty_assertions = "1.3.0"

[features]
secp256k1 = ["tendermint/secp256k1"]
18 changes: 15 additions & 3 deletions config/tests/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -213,17 +213,29 @@ fn node_key_parser() {
);
}

/// Parse an example `priv_validator_key.json` to a `PrivValidatorKey` struct
/// Parse an example `priv_validator_key.ed25519.json` to a `PrivValidatorKey` struct
#[test]
fn priv_validator_json_parser() {
let raw_priv_validator_key = read_fixture("priv_validator_key.json");
fn priv_validator_ed25519_json_parser() {
let raw_priv_validator_key = read_fixture("priv_validator_key.ed25519.json");
let priv_validator_key = PrivValidatorKey::parse_json(raw_priv_validator_key).unwrap();
assert_eq!(
priv_validator_key.consensus_pubkey().public_key().to_hex(),
"F26BF4B2A2E84CEB7A53C3F1AE77408779B20064782FBADBDF0E365959EE4534"
);
}

/// Parse an example `priv_validator_key.secp256k1.json` to a `PrivValidatorKey` struct
#[test]
#[cfg(feature = "secp256k1")]
fn priv_validator_secp256k1_json_parser() {
let raw_priv_validator_key = read_fixture("priv_validator_key.secp256k1.json");
let priv_validator_key = PrivValidatorKey::parse_json(raw_priv_validator_key).unwrap();
assert_eq!(
priv_validator_key.consensus_pubkey().public_key().to_hex(),
"03685D6F62E54D0FACC6AA649E4036BF446043A6BA438B793B085E6C8A92AFCAAE"
);
}

/// Parse an example `config.toml` file to a `TendermintConfig` struct, then
/// serialize it and parse again.
#[test]
Expand Down
11 changes: 11 additions & 0 deletions config/tests/support/config/priv_validator_key.secp256k1.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{
"address": "0799B6E1F13F1ECC7E891765F499A73665F42866",
"pub_key": {
"type": "tendermint/PubKeySecp256k1",
"value": "A2hdb2LlTQ+sxqpknkA2v0RgQ6a6Q4t5OwhebIqSr8qu"
},
"priv_key": {
"type": "tendermint/PrivKeySecp256k1",
"value": "aVmw0npSBEqPU4v6qEZ9o3z5nCpHjicwFDdIBRUbSrM="
}
}
54 changes: 54 additions & 0 deletions tendermint/src/private_key.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
//! Cryptographic private keys

pub use crate::crypto::ed25519::SigningKey as Ed25519;
#[cfg(feature = "secp256k1")]
pub use k256::ecdsa::SigningKey as Secp256k1;

use crate::prelude::*;

#[cfg(feature = "rust-crypto")]
Expand All @@ -14,6 +17,7 @@ use subtle_encoding::{Base64, Encoding};
use zeroize::Zeroizing;

pub const ED25519_KEYPAIR_SIZE: usize = 64;
pub const SECP256K1_KEY_SIZE: usize = 32;

/// Private keys as parsed from configuration files
#[cfg_attr(feature = "rust-crypto", derive(Serialize, Deserialize))]
Expand All @@ -30,6 +34,15 @@ pub enum PrivateKey {
)
)]
Ed25519(Ed25519),

#[cfg(feature = "secp256k1")]
#[cfg_attr(docsrs, doc(cfg(feature = "secp256k1")))]
#[serde(
rename = "tendermint/PrivKeySecp256k1",
serialize_with = "serialize_secp256k1_privkey",
deserialize_with = "deserialize_secp256k1_privkey"
)]
Secp256k1(Secp256k1),
Comment on lines +38 to +45
Copy link
Contributor

Choose a reason for hiding this comment

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

Feature-guarded enum variants bring fragility. I'd rather make the whole thing non-optional, secp256k1 support does not add much in terms of dependencies. But the enum is #[non_exhaustive] so maybe it's OK for now.

Copy link
Contributor Author

@mkaczanowski mkaczanowski Sep 29, 2023

Choose a reason for hiding this comment

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

since the secp256k1 is marked almost everywhere as feature I fell with the same approach. But I agree that conditional enums are not as great. Though rust is pretty good with handling conditional compilation via features.

Therefore I hold no strong opinion here

Copy link
Contributor

Choose a reason for hiding this comment

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

The problem with conditional enum variants is that they make match expressions fail to compile dependent on features. #[non_exhaustive] does a lot to make it less fragile, as you can't have a match that would be exhaustive without certain features and then fail to compile if the features are enabled. But anyone wanting to match on the conditionally-enabled variant would have to enable the corresponding feature. This, perhaps, is no worse than other conditionally defined APIs, so I'm willing to let it go.

Copy link
Contributor

Choose a reason for hiding this comment

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

FYI, I believe the original motivation for feature-gating was at the time libsecp256k1 was being used and it didn’t work with WASM.

We’ve since moved to the pure Rust k256 crate so that isn’t an issue.

I think it would probably make more sense to feature gate all the cryptographic functionality under a single feature rather than just secp256k1, that way people who just want e.g. tendermint-rpc for an API client aren’t pulling in a bunch of crypto deps they aren’t using.

}

impl PrivateKey {
Expand All @@ -38,17 +51,58 @@ impl PrivateKey {
pub fn public_key(&self) -> PublicKey {
match self {
PrivateKey::Ed25519(signing_key) => PublicKey::Ed25519(signing_key.verification_key()),

#[cfg(feature = "secp256k1")]
PrivateKey::Secp256k1(signing_key) => {
PublicKey::Secp256k1(*signing_key.verifying_key())
},
}
}

/// If applicable, borrow the Ed25519 keypair
pub fn ed25519_signing_key(&self) -> Option<&Ed25519> {
match self {
PrivateKey::Ed25519(signing_key) => Some(signing_key),

#[cfg(feature = "secp256k1")]
PrivateKey::Secp256k1(_signing_key) => None,
}
}
}

/// Serialize a Secp256k1 privkey as Base64
#[cfg(feature = "secp256k1")]
fn serialize_secp256k1_privkey<S>(signing_key: &Secp256k1, serializer: S) -> Result<S::Ok, S::Error>
where
S: ser::Serializer,
{
Zeroizing::new(String::from_utf8(Base64::default().encode(signing_key.to_bytes())).unwrap())
.serialize(serializer)
}

/// Deserialize a Secp256k1 privkey from Base64
#[cfg(feature = "secp256k1")]
fn deserialize_secp256k1_privkey<'de, D>(deserializer: D) -> Result<Secp256k1, D::Error>
where
D: de::Deserializer<'de>,
{
use de::Error;
let string = Zeroizing::new(String::deserialize(deserializer)?);
let mut privkey_bytes = Zeroizing::new([0u8; SECP256K1_KEY_SIZE]);
let decoded_len = Base64::default()
.decode_to_slice(string.as_bytes(), &mut *privkey_bytes)
.map_err(D::Error::custom)?;

if decoded_len != SECP256K1_KEY_SIZE {
return Err(D::Error::custom("invalid sepc256k1 privkey size"));
}

let signing_key = Secp256k1::try_from(&privkey_bytes[0..SECP256K1_KEY_SIZE])
.map_err(|_| D::Error::custom("invalid signing key"))?;

Ok(signing_key)
}

/// Serialize an Ed25519 keypair as Base64
#[cfg(feature = "rust-crypto")]
fn serialize_ed25519_keypair<S>(signing_key: &Ed25519, serializer: S) -> Result<S::Ok, S::Error>
Expand Down
5 changes: 4 additions & 1 deletion tendermint/src/public_key.rs
Original file line number Diff line number Diff line change
Expand Up @@ -290,8 +290,11 @@ impl TendermintKey {
#[allow(unreachable_patterns)]
match public_key {
PublicKey::Ed25519(_) => Ok(TendermintKey::AccountKey(public_key)),
#[cfg(feature = "secp256k1")]
PublicKey::Secp256k1(_) => Ok(TendermintKey::AccountKey(public_key)),

_ => Err(Error::invalid_key(
"only ed25519 consensus keys are supported".to_string(),
"only ed25519 or secp256k1 consensus keys are supported".to_string(),
)),
}
}
Expand Down