Refactor signing module into privval (#784)

Factors response-handling operations from `SignedMsg` into the
`Response` type and removes the (unused) sign method.

This decouples `privval` from response handling and reduces its
responsibilities to canonical message serialization and consensus state
extraction, making it easier to potentially extract into a separate
crate in the future.

Cargo.toml

@@ -45,8 +45,8 @@ subtle-encoding = { version = "0.5", features = ["bech32-preview"] }
 tempfile = "3"
 tendermint = { version = "0.34", features = ["secp256k1"] }
 tendermint-config = "0.34"
-tendermint-proto = "0.34"
 tendermint-p2p = "0.34"
+tendermint-proto = "0.34"
 thiserror = "1"
 url = { version = "2.2.2", features = ["serde"], optional = true }
 uuid = { version = "1", features = ["serde"], optional = true }

src/commands/ledger.rs

@@ -3,7 +3,7 @@
 use crate::{
     chain,
     prelude::*,
-    signing::{SignableMsg, SignedMsgType},
+    privval::{SignableMsg, SignedMsgType},
 };
 use abscissa_core::{Command, Runnable};
 use clap::{Parser, Subcommand};

src/lib.rs

@@ -24,9 +24,9 @@ pub mod error;
 pub mod key_utils;
 pub mod keyring;
 pub mod prelude;
+pub mod privval;
 pub mod rpc;
 pub mod session;
-pub mod signing;
 
 #[cfg(feature = "yubihsm")]
 pub mod yubihsm;

src/signing.rs → src/privval.rs

@@ -1,10 +1,8 @@
-//! Signed message support.
+//! Validator private key operations: signing consensus votes and proposals.
 
-use crate::{error::Error, keyring::signature::Signature, rpc::Response};
 use bytes::{Bytes, BytesMut};
 use prost::{EncodeError, Message as _};
-use signature::Signer;
-use tendermint::{block, chain, consensus, vote, Proposal, Vote};
+use tendermint::{block, chain, consensus, vote, Error, Proposal, Vote};
 use tendermint_proto as proto;
 
 /// Message codes.
@@ -49,18 +47,8 @@ impl SignableMsg {
         }
     }
 
-    /// Sign the given message, returning a response with the signature appended.
-    pub fn sign<S>(self, chain_id: chain::Id, signer: &impl Signer<S>) -> Result<Response, Error>
-    where
-        S: Into<Signature>,
-    {
-        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.
+    /// signature is computed over.
     pub fn signable_bytes(&self, chain_id: chain::Id) -> Result<Bytes, EncodeError> {
         let mut bytes = BytesMut::new();
 
@@ -97,44 +85,6 @@ impl SignableMsg {
         Ok(bytes.into())
     }
 
-    /// Add a signature to this request, returning a response.
-    pub fn add_signature(self, sig: Signature) -> Result<Response, Error> {
-        match self {
-            Self::Proposal(proposal) => {
-                let mut proposal = proto::types::Proposal::from(proposal);
-                proposal.signature = sig.to_vec();
-                Ok(Response::SignedProposal(
-                    proto::privval::SignedProposalResponse {
-                        proposal: Some(proposal),
-                        error: None,
-                    },
-                ))
-            }
-            Self::Vote(vote) => {
-                let mut vote = proto::types::Vote::from(vote);
-                vote.signature = sig.to_vec();
-                Ok(Response::SignedVote(proto::privval::SignedVoteResponse {
-                    vote: Some(vote),
-                    error: None,
-                }))
-            }
-        }
-    }
-
-    /// Build an error response for this request.
-    pub fn error(&self, error: proto::privval::RemoteSignerError) -> Response {
-        match self {
-            Self::Proposal(_) => Response::SignedProposal(proto::privval::SignedProposalResponse {
-                proposal: None,
-                error: Some(error),
-            }),
-            Self::Vote(_) => Response::SignedVote(proto::privval::SignedVoteResponse {
-                vote: None,
-                error: Some(error),
-            }),
-        }
-    }
-
     /// Parse the consensus state from the request.
     pub fn consensus_state(&self) -> consensus::State {
         match self {
@@ -158,15 +108,15 @@ impl SignableMsg {
 }
 
 impl TryFrom<proto::types::Proposal> for SignableMsg {
-    type Error = tendermint::Error;
+    type Error = Error;
 
     fn try_from(proposal: proto::types::Proposal) -> Result<Self, Self::Error> {
         Proposal::try_from(proposal).map(Self::Proposal)
     }
 }
 
 impl TryFrom<proto::types::Vote> for SignableMsg {
-    type Error = tendermint::Error;
+    type Error = Error;
 
     fn try_from(vote: proto::types::Vote) -> Result<Self, Self::Error> {
         Vote::try_from(vote).map(Self::Vote)
@@ -246,25 +196,27 @@ impl TryFrom<SignedMsgCode> for SignedMsgType {
     type Error = Error;
 
     fn try_from(code: SignedMsgCode) -> Result<Self, Self::Error> {
-        Ok(proto::types::SignedMsgType::try_from(code)?.into())
+        proto::types::SignedMsgType::try_from(code)
+            .map(Into::into)
+            .map_err(|e| Error::parse(e.to_string()))
     }
 }
 
 #[cfg(test)]
 mod tests {
     use super::{chain, proto, SignableMsg, SignedMsgType};
-    use chrono::{DateTime, Utc};
+    use tendermint::Time;
 
     fn example_chain_id() -> chain::Id {
         chain::Id::try_from("test_chain_id").unwrap()
     }
 
     fn example_timestamp() -> proto::google::protobuf::Timestamp {
-        let dt = "2023-10-04T10:00:00.000Z".parse::<DateTime<Utc>>().unwrap();
+        let dt = Time::parse_from_rfc3339("2023-10-04T10:00:00.000Z").unwrap();
 
         proto::google::protobuf::Timestamp {
-            seconds: dt.timestamp(),
-            nanos: dt.timestamp_subsec_nanos() as i32,
+            seconds: dt.unix_timestamp(),
+            nanos: 0,
         }
     }
 
@@ -305,7 +257,7 @@ mod tests {
     }
 
     #[test]
-    fn sign_proposal() {
+    fn serialize_canonical_proposal() {
         let signable_msg = SignableMsg::try_from(example_proposal()).unwrap();
         let signable_bytes = signable_msg.signable_bytes(example_chain_id()).unwrap();
         assert_eq!(
@@ -320,7 +272,7 @@ mod tests {
     }
 
     #[test]
-    fn sign_vote() {
+    fn serialize_canonical_vote() {
         let signable_msg = SignableMsg::try_from(example_vote()).unwrap();
         let signable_bytes = signable_msg.signable_bytes(example_chain_id()).unwrap();
         assert_eq!(

src/rpc.rs

@@ -3,7 +3,7 @@
 // TODO: docs for everything
 #![allow(missing_docs)]
 
-use crate::signing::SignableMsg;
+use crate::{keyring::Signature, privval::SignableMsg};
 use prost::Message as _;
 use std::io::Read;
 use tendermint::chain;
@@ -97,7 +97,7 @@ pub enum Response {
 }
 
 impl Response {
-    /// Encode response to bytes
+    /// Encode response to bytes.
     pub fn encode(self) -> Result<Vec<u8>, Error> {
         let mut buf = Vec::new();
         let msg = match self {
@@ -111,6 +111,46 @@ impl Response {
         proto::privval::Message { sum: Some(msg) }.encode_length_delimited(&mut buf)?;
         Ok(buf)
     }
+
+    /// Construct an error response for a given [`SignableMsg`].
+    pub fn error(msg: SignableMsg, error: proto::privval::RemoteSignerError) -> Response {
+        match msg {
+            SignableMsg::Proposal(_) => {
+                Response::SignedProposal(proto::privval::SignedProposalResponse {
+                    proposal: None,
+                    error: Some(error),
+                })
+            }
+            SignableMsg::Vote(_) => Response::SignedVote(proto::privval::SignedVoteResponse {
+                vote: None,
+                error: Some(error),
+            }),
+        }
+    }
+
+    /// Construct a signed response from a [`SignableMsg`] and a [`Signature`].
+    pub fn sign(msg: SignableMsg, sig: Signature) -> Result<Response, Error> {
+        match msg {
+            SignableMsg::Proposal(proposal) => {
+                let mut proposal = proto::types::Proposal::from(proposal);
+                proposal.signature = sig.to_vec();
+                Ok(Response::SignedProposal(
+                    proto::privval::SignedProposalResponse {
+                        proposal: Some(proposal),
+                        error: None,
+                    },
+                ))
+            }
+            SignableMsg::Vote(vote) => {
+                let mut vote = proto::types::Vote::from(vote);
+                vote.signature = sig.to_vec();
+                Ok(Response::SignedVote(proto::privval::SignedVoteResponse {
+                    vote: Some(vote),
+                    error: None,
+                }))
+            }
+        }
+    }
 }
 
 /// Read a message from a Secret Connection

src/session.rs

@@ -6,8 +6,8 @@ use crate::{
     connection::{tcp, unix::UnixConnection, Connection},
     error::{Error, ErrorKind::*},
     prelude::*,
+    privval::SignableMsg,
     rpc::{Request, Response},
-    signing::SignableMsg,
 };
 use std::{os::unix::net::UnixStream, time::Instant};
 use tendermint::{consensus, TendermintKey};
@@ -136,7 +136,7 @@ impl Session {
 
         if let Some(remote_err) = self.update_consensus_state(chain, &signable_msg)? {
             // In the event of double signing we send a response to notify the validator
-            return Ok(signable_msg.error(remote_err));
+            return Ok(Response::error(signable_msg, remote_err));
         }
 
         let to_sign = signable_msg.signable_bytes(self.config.chain_id.clone())?;
@@ -146,7 +146,7 @@ impl Session {
         let signature = chain.keyring.sign(None, &to_sign)?;
 
         self.log_signing_request(&signable_msg, started_at).unwrap();
-        signable_msg.add_signature(signature)
+        Response::sign(signable_msg, signature)
     }
 
     /// If a max block height is configured, ensure the block we're signing

tests/integration.rs

@@ -19,7 +19,7 @@ use tmkms::{
     config::provider::KeyType,
     connection::unix::UnixConnection,
     keyring::ed25519,
-    signing::{SignableMsg, SignedMsgType},
+    privval::{SignableMsg, SignedMsgType},
 };
 
 /// Integration tests for the KMS command-line interface