From e9458e6623c7bf8d2909ac2510767e28a3474231 Mon Sep 17 00:00:00 2001 From: Tony Arcieri Date: Fri, 21 Jun 2019 15:02:12 -0700 Subject: [PATCH] Send RemoteSignerError response on double sign (closes #249) Previously double signing would abort the connection. However, there is a semi-valid use case for returning an error message instead: when concurrent validators on the same chain are sending signing messages. This was proposed by @mdyring in #249. Ideally there should be coordination (i.e. between KMS instances) as to which validator is currently active, as this approach depends critically on the KMS's double signing prevention and encourages configurations where multiple validator instances are attempting to sign simultaneously. This runs the risk that a bug in the KMS's double signing detection could be singularly responsible for a double sign event. However, without something like this, it isn't possible for the KMS to service two validators simultaneously, so this seems like an OK start. --- src/chain/state/error.rs | 7 +++ src/rpc.rs | 41 +++++++++----- src/session.rs | 54 ++++++++++++++----- tendermint-rs/src/amino_types/remote_error.rs | 23 ++++++++ 4 files changed, 99 insertions(+), 26 deletions(-) diff --git a/src/chain/state/error.rs b/src/chain/state/error.rs index 8d7bf49..39450ef 100644 --- a/src/chain/state/error.rs +++ b/src/chain/state/error.rs @@ -29,6 +29,13 @@ pub enum StateErrorKind { SyncError, } +impl StateError { + /// Get the kind of error + pub fn kind(&self) -> StateErrorKind { + *self.0.kind() + } +} + impl From> for StateError { fn from(other: Error) -> Self { StateError(other) diff --git a/src/rpc.rs b/src/rpc.rs index 81bfbb5..22a34a3 100644 --- a/src/rpc.rs +++ b/src/rpc.rs @@ -42,8 +42,7 @@ pub enum Response { } pub trait TendermintRequest: SignableMsg { - // TODO(ismail): this should take an error as an argument: - fn build_response(self) -> Response; + fn build_response(self, error: Option) -> Response; } fn compute_prefix(name: &str) -> (Vec) { @@ -113,19 +112,37 @@ impl Request { } impl TendermintRequest for SignVoteRequest { - fn build_response(self) -> Response { - Response::SignedVote(SignedVoteResponse { - vote: self.vote, - err: None, - }) + fn build_response(self, error: Option) -> Response { + let response = if let Some(e) = error { + SignedVoteResponse { + vote: None, + err: Some(e), + } + } else { + SignedVoteResponse { + vote: self.vote, + err: None, + } + }; + + Response::SignedVote(response) } } impl TendermintRequest for SignProposalRequest { - fn build_response(self) -> Response { - Response::SignedProposal(SignedProposalResponse { - proposal: self.proposal, - err: None, - }) + fn build_response(self, error: Option) -> Response { + let response = if let Some(e) = error { + SignedProposalResponse { + proposal: None, + err: Some(e), + } + } else { + SignedProposalResponse { + proposal: self.proposal, + err: None, + } + }; + + Response::SignedProposal(response) } } diff --git a/src/session.rs b/src/session.rs index 6176c25..93d3450 100644 --- a/src/session.rs +++ b/src/session.rs @@ -1,7 +1,7 @@ //! A session with a validator node use crate::{ - chain, + chain::{self, state::StateErrorKind}, error::{Error, ErrorKind::*}, prost::Message, rpc::{Request, Response, TendermintRequest}, @@ -23,13 +23,16 @@ use std::{ }; use subtle::ConstantTimeEq; use tendermint::{ - amino_types::{PingRequest, PingResponse, PubKeyRequest, PubKeyResponse}, + amino_types::{PingRequest, PingResponse, PubKeyRequest, PubKeyResponse, RemoteError}, node, secret_connection::{self, SecretConnection}, }; /// Encrypted session with a validator node pub struct Session { + /// Remote peer location + peer_addr: String, + /// Chain ID for this session chain_id: chain::Id, @@ -50,7 +53,8 @@ impl Session> { port: u16, secret_connection_key: &ed25519::Seed, ) -> Result { - debug!("{}: Connecting to {}:{}...", chain_id, host, port); + let peer_addr = format!("{}:{}", host, port); + debug!("{}: Connecting to {}...", chain_id, &peer_addr); let socket = TcpStream::connect(format!("{}:{}", host, port))?; let signer = Ed25519Signer::from(secret_connection_key); @@ -81,6 +85,7 @@ impl Session> { } Ok(Self { + peer_addr, chain_id, max_height, connection, @@ -95,16 +100,15 @@ impl Session> { max_height: Option, socket_path: &Path, ) -> Result { - debug!( - "{}: Connecting to socket at {}...", - chain_id, - socket_path.to_str().unwrap() - ); + let peer_addr = socket_path.to_str().unwrap().to_owned(); + + debug!("{}: Connecting to socket at {}...", chain_id, &peer_addr); let socket = UnixStream::connect(socket_path)?; let connection = UnixConnection::new(socket); Ok(Self { + peer_addr, chain_id, max_height, connection, @@ -131,7 +135,10 @@ where } let request = Request::read(&mut self.connection)?; - debug!("received request: {:?}", &request); + debug!( + "[{}:{}] received request: {:?}", + &self.chain_id, &self.peer_addr, &request + ); let response = match request { Request::SignProposal(req) => self.sign(req)?, @@ -141,7 +148,10 @@ where Request::ShowPublicKey(ref req) => self.get_public_key(req)?, }; - debug!("sending response: {:?}", &response); + debug!( + "[{}:{}] sending response: {:?}", + &self.chain_id, &self.peer_addr, &response + ); let mut buf = vec![]; @@ -167,7 +177,23 @@ where if let Some(request_state) = request.consensus_state() { // TODO(tarcieri): better handle `PoisonError`? let mut chain_state = chain.state.lock().unwrap(); - chain_state.update_consensus_state(request_state.clone())?; + + if let Err(e) = chain_state.update_consensus_state(request_state.clone()) { + // Report double signing error back to the validator + if e.kind() == StateErrorKind::DoubleSign { + let height = request.height().unwrap(); + + warn!( + "[{}:{}] attempt to double sign at height: {}", + &self.chain_id, &self.peer_addr, height + ); + + let remote_err = RemoteError::double_sign(height); + return Ok(request.build_response(Some(remote_err))); + } else { + return Err(e.into()); + } + } } if let Some(max_height) = self.max_height { @@ -193,7 +219,7 @@ where self.log_signing_request(&request); request.set_signature(&sig); - Ok(request.build_response()) + Ok(request.build_response(None)) } /// Reply to a ping request @@ -225,8 +251,8 @@ where .unwrap_or_else(|| "Unknown".to_owned()); info!( - "[{}] signed {:?} at height: {}", - self.chain_id, msg_type, height + "[{}@{}] signed {:?} at height: {}", + &self.chain_id, &self.peer_addr, msg_type, height ); } } diff --git a/tendermint-rs/src/amino_types/remote_error.rs b/tendermint-rs/src/amino_types/remote_error.rs index d188d75..8c86716 100644 --- a/tendermint-rs/src/amino_types/remote_error.rs +++ b/tendermint-rs/src/amino_types/remote_error.rs @@ -5,3 +5,26 @@ pub struct RemoteError { #[prost(string, tag = "2")] pub description: String, } + +/// Error codes for remote signer failures +// TODO(tarcieri): add these to Tendermint. See corresponding TODO here: +// +#[derive(Copy, Clone, Debug, Eq, PartialEq)] +#[repr(i32)] +pub enum RemoteErrorCode { + /// Generic error code useful when the others don't apply + RemoteSignerError = 1, + + /// Double signing detected + DoubleSignError = 2, +} + +impl RemoteError { + /// Create a new double signing error with the given message + pub fn double_sign(height: i64) -> Self { + RemoteError { + code: RemoteErrorCode::DoubleSignError as i32, + description: format!("double signing requested at height: {}", height), + } + } +}