From d3a404fbf71a860963bc4c45287bee45f9aa6159 Mon Sep 17 00:00:00 2001 From: "Tony Arcieri (iqlusion)" Date: Fri, 13 Oct 2023 10:33:46 -0700 Subject: [PATCH] Rename and refactor `SignableMsg::signable_bytes` (#780) The previous method name was `sign_bytes`, which as was noted in #273 is unclear to whether the resulting bytes have an appended signature. The new name indicates that the message can be signed, but has not yet been signed. Additionally, this commit does a bit of refactoring to eliminate some duplicated code within the method. Closes #273 --- src/commands/ledger.rs | 5 ++- src/error.rs | 6 ++++ src/keyring/signature.rs | 20 ++++++++--- src/session.rs | 4 +-- src/signing.rs | 77 ++++++++++++++++++++-------------------- tests/integration.rs | 39 ++++++++++---------- 6 files changed, 82 insertions(+), 69 deletions(-) diff --git a/src/commands/ledger.rs b/src/commands/ledger.rs index 8eb751f0..4c80f8ae 100644 --- a/src/commands/ledger.rs +++ b/src/commands/ledger.rs @@ -62,9 +62,8 @@ impl Runnable for InitCommand { }; println!("{vote:?}"); let sign_vote_req = SignableMsg::Vote(vote); - let mut to_sign = vec![]; - sign_vote_req - .sign_bytes(config.validator[0].chain_id.clone(), &mut to_sign) + let to_sign = sign_vote_req + .signable_bytes(config.validator[0].chain_id.clone()) .unwrap(); let _sig = chain.keyring.sign(None, &to_sign).unwrap(); diff --git a/src/error.rs b/src/error.rs index c0e694e3..e09ea3fc 100644 --- a/src/error.rs +++ b/src/error.rs @@ -184,6 +184,12 @@ impl From for Error { } } +impl From for Error { + fn from(other: signature::Error) -> Self { + ErrorKind::CryptoError.context(other).into() + } +} + impl From for Error { fn from(other: tendermint::error::Error) -> Self { ErrorKind::TendermintError.context(other).into() diff --git a/src/keyring/signature.rs b/src/keyring/signature.rs index 5684319a..c101aa43 100644 --- a/src/keyring/signature.rs +++ b/src/keyring/signature.rs @@ -5,19 +5,31 @@ pub use k256::ecdsa; /// Cryptographic signature used for block signing pub enum Signature { + /// ECDSA signature (e.g secp256k1) + Ecdsa(ecdsa::Signature), + /// ED25519 signature Ed25519(ed25519::Signature), - - /// ECDSA signagure (e.g secp256k1) - Ecdsa(ecdsa::Signature), } impl Signature { /// Serialize this signature as a byte vector. pub fn to_vec(&self) -> Vec { match self { - Self::Ed25519(sig) => sig.to_vec(), Self::Ecdsa(sig) => sig.to_vec(), + Self::Ed25519(sig) => sig.to_vec(), } } } + +impl From for Signature { + fn from(sig: ecdsa::Signature) -> Signature { + Self::Ecdsa(sig) + } +} + +impl From for Signature { + fn from(sig: ed25519::Signature) -> Signature { + Self::Ed25519(sig) + } +} diff --git a/src/session.rs b/src/session.rs index 981c9460..1fc115e5 100644 --- a/src/session.rs +++ b/src/session.rs @@ -139,9 +139,7 @@ impl Session { return Ok(signable_msg.error(remote_err)); } - let mut to_sign = vec![]; - signable_msg.sign_bytes(self.config.chain_id.clone(), &mut to_sign)?; - + let to_sign = signable_msg.signable_bytes(self.config.chain_id.clone())?; let started_at = Instant::now(); // TODO(ismail): figure out which key to use here instead of taking the only key diff --git a/src/signing.rs b/src/signing.rs index 7bccb9ba..aa9bcbd4 100644 --- a/src/signing.rs +++ b/src/signing.rs @@ -6,8 +6,9 @@ use crate::{ prelude::{ensure, fail, format_err}, rpc::Response, }; -use bytes::BufMut; +use bytes::{Bytes, BytesMut}; use prost::{EncodeError, Message as _}; +use signature::Signer; use tendermint::{block, chain, consensus}; use tendermint_proto as proto; @@ -98,31 +99,43 @@ impl SignableMsg { Ok(()) } - /// Get the bytes to be signed for a given message. - pub fn sign_bytes( - &self, - chain_id: chain::Id, - sign_bytes: &mut B, - ) -> Result + /// Sign the given message, returning a response with the signature appended. + pub fn sign(self, chain_id: chain::Id, signer: impl Signer) -> Result where - B: BufMut, + S: Into, { + let signable_bytes = self.signable_bytes(chain_id)?; + let signature = signer.try_sign(&signable_bytes)?; + self.add_signature(signature.into()) + } + + /// Get the bytes representing a canonically encoded message over which a + /// signature is to be computed. + pub fn signable_bytes(&self, chain_id: chain::Id) -> Result { + fn canonicalize_block_id( + block_id: &proto::types::BlockId, + ) -> Option { + if block_id.hash.is_empty() { + return None; + } + + Some(proto::types::CanonicalBlockId { + hash: block_id.hash.clone(), + part_set_header: block_id.part_set_header.as_ref().map(|y| { + proto::types::CanonicalPartSetHeader { + total: y.total, + hash: y.hash.clone(), + } + }), + }) + } + + let mut bytes = BytesMut::new(); + match self { Self::Proposal(proposal) => { let proposal = proposal.clone(); - let block_id = match proposal.block_id.as_ref() { - Some(x) if x.hash.is_empty() => None, - Some(x) => Some(proto::types::CanonicalBlockId { - hash: x.hash.clone(), - part_set_header: x.part_set_header.as_ref().map(|y| { - proto::types::CanonicalPartSetHeader { - total: y.total, - hash: y.hash.clone(), - } - }), - }), - None => None, - }; + let block_id = proposal.block_id.as_ref().and_then(canonicalize_block_id); let cp = proto::types::CanonicalProposal { chain_id: chain_id.to_string(), @@ -134,24 +147,11 @@ impl SignableMsg { timestamp: proposal.timestamp.map(Into::into), }; - cp.encode_length_delimited(sign_bytes).unwrap(); - Ok(true) + cp.encode_length_delimited(&mut bytes)?; } Self::Vote(vote) => { let vote = vote.clone(); - let block_id = match vote.block_id.as_ref() { - Some(x) if x.hash.is_empty() => None, - Some(x) => Some(proto::types::CanonicalBlockId { - hash: x.hash.clone(), - part_set_header: x.part_set_header.as_ref().map(|y| { - proto::types::CanonicalPartSetHeader { - total: y.total, - hash: y.hash.clone(), - } - }), - }), - None => None, - }; + let block_id = vote.block_id.as_ref().and_then(canonicalize_block_id); let cv = proto::types::CanonicalVote { r#type: vote.r#type, @@ -161,10 +161,11 @@ impl SignableMsg { timestamp: vote.timestamp.map(Into::into), chain_id: chain_id.to_string(), }; - cv.encode_length_delimited(sign_bytes).unwrap(); - Ok(true) + cv.encode_length_delimited(&mut bytes)?; } } + + Ok(bytes.into()) } /// Add a signature to this request, returning a response. diff --git a/tests/integration.rs b/tests/integration.rs index 7eac38c0..d19ba88f 100644 --- a/tests/integration.rs +++ b/tests/integration.rs @@ -365,28 +365,27 @@ fn handle_and_sign_proposal(key_type: KeyType) { other => panic!("unexpected message type in response: {other:?}"), }; - let mut sign_bytes: Vec = vec![]; - signable_msg - .sign_bytes(chain_id.parse().unwrap(), &mut sign_bytes) + let signable_bytes = signable_msg + .signable_bytes(chain_id.parse().unwrap()) .unwrap(); let prop = response .proposal .expect("proposal should be embedded but none was found"); - let msg: &[u8] = sign_bytes.as_slice(); - let r = match key_type { KeyType::Account => { let signature = k256::ecdsa::Signature::try_from(prop.signature.as_slice()).unwrap(); - test_secp256k1_keypair().1.verify(msg, &signature) + test_secp256k1_keypair() + .1 + .verify(&signable_bytes, &signature) } KeyType::Consensus => { let signature = ed25519::Signature::try_from(prop.signature.as_slice()).unwrap(); test_ed25519_keypair() .verifying_key() - .verify(msg, &signature) + .verify(&signable_bytes, &signature) } }; assert!(r.is_ok()); @@ -449,9 +448,8 @@ fn handle_and_sign_vote(key_type: KeyType) { other => panic!("unexpected message type in response: {other:?}"), }; - let mut sign_bytes: Vec = vec![]; - signable_msg - .sign_bytes(chain_id.parse().unwrap(), &mut sign_bytes) + let signable_bytes = signable_msg + .signable_bytes(chain_id.parse().unwrap()) .unwrap(); let vote_msg: proto::types::Vote = request @@ -461,18 +459,18 @@ fn handle_and_sign_vote(key_type: KeyType) { let sig: Vec = vote_msg.signature; assert_ne!(sig.len(), 0); - let msg: &[u8] = sign_bytes.as_slice(); - let r = match key_type { KeyType::Account => { let signature = k256::ecdsa::Signature::try_from(sig.as_slice()).unwrap(); - test_secp256k1_keypair().1.verify(msg, &signature) + test_secp256k1_keypair() + .1 + .verify(&signable_bytes, &signature) } KeyType::Consensus => { let signature = ed25519::Signature::try_from(sig.as_slice()).unwrap(); test_ed25519_keypair() .verifying_key() - .verify(msg, &signature) + .verify(&signable_bytes, &signature) } }; assert!(r.is_ok()); @@ -537,9 +535,8 @@ fn exceed_max_height(key_type: KeyType) { other => panic!("unexpected message type in response: {other:?}"), }; - let mut sign_bytes: Vec = vec![]; - signable_msg - .sign_bytes(chain_id.parse().unwrap(), &mut sign_bytes) + let signable_bytes = signable_msg + .signable_bytes(chain_id.parse().unwrap()) .unwrap(); let vote_msg = response @@ -549,18 +546,18 @@ fn exceed_max_height(key_type: KeyType) { let sig: Vec = vote_msg.signature; assert_ne!(sig.len(), 0); - let msg: &[u8] = sign_bytes.as_slice(); - let r = match key_type { KeyType::Account => { let signature = k256::ecdsa::Signature::try_from(sig.as_slice()).unwrap(); - test_secp256k1_keypair().1.verify(msg, &signature) + test_secp256k1_keypair() + .1 + .verify(&signable_bytes, &signature) } KeyType::Consensus => { let signature = ed25519::Signature::try_from(sig.as_slice()).unwrap(); test_ed25519_keypair() .verifying_key() - .verify(msg, &signature) + .verify(&signable_bytes, &signature) } }; assert!(r.is_ok());