From fd968f579f83e15df47f9a5a71a43bb5e177d4d6 Mon Sep 17 00:00:00 2001 From: Zaki Manian Date: Mon, 4 Mar 2019 09:19:06 -0500 Subject: [PATCH 01/18] Work in progress of tracking the state of what we most recently signed so regressions don't trigger a double sign --- src/last_sign_state.rs | 90 ++++++++++++++++++++++++++++++++++++++++++ src/lib.rs | 1 + 2 files changed, 91 insertions(+) create mode 100644 src/last_sign_state.rs diff --git a/src/last_sign_state.rs b/src/last_sign_state.rs new file mode 100644 index 0000000..d4cda8c --- /dev/null +++ b/src/last_sign_state.rs @@ -0,0 +1,90 @@ +use std::{ + path::Path, + fs::File, + io::prelude::*, +}; +use tendermint::chain; +use serde_json; +use abscissa::Error; + + +#[derive(Serialize,Deserialize)] +struct LastSignData{ + pub height: i64, + pub round: i64, + pub step : i8, + pub signature: String, + pub signbytes: String, +} + +pub struct LastSignState{ + data: LastSignData, + file: File, + chain_id: chain::Id +} + +/// Error type +#[derive(Debug)] +pub struct LastSignError(Error); + +#[derive(Copy, Clone, Eq, PartialEq, Debug, Fail)] +pub enum LastSignErrorKind{ + #[fail(display = "height regression")] + HeightRegression, + #[fail(display = "step regression")] + StepRegression, + #[fail(display = "round regression")] + RoundRegression, +} + +impl From> for LastSignError { + fn from(other: Error) -> Self { + LastSignError(other) + } +} + +impl LastSignState{ + pub fn init_state(&mut self, path: &Path) -> std::io::Result<()>{ + if path.exists(){ + return Ok(()); + } + self.file = File::create(self.filename())?; + self.file.write_all(serde_json::to_string(&self.data).unwrap().as_ref())?; + self.file.sync_all()?; + return Ok(()); + } + + pub fn load_state(&mut self, path:&Path) -> std::io::Result<()>{ + self.file = File::open(path)?; + let mut contents = String::new(); + self.file.read_to_string(&mut contents)?; + self.data = serde_json::from_str(&contents).unwrap(); + return Ok(()); + } + + pub fn filename(&self) -> String{ + self.chain_id.as_str().to_owned() + "_validator_state.json" + } + + pub fn check_and_update_hrs(&mut self, height: i64, round:i64, step:i8)->Result<(),LastSignError>{ + if height < self.data.height{ + fail!(HeightRegression, "last height:{} new height:{}", self.data.height, height); + } + if height == self.data.height { + if round < self.data.round{ + fail!(RoundRegression, "round regression at height:{} last round:{} new round:{}",height, self.data.round, round) + } + if round == self.data.round{ + if step < self.data.step{ + fail!(StepRegression, "round regression at height:{} round:{} last step:{} new step:{}",height,round, self.data.step, step) + } + } + } + self.data.height = height; + self.data.round = round; + self.data.step = step; + Ok(()) + } + + +} \ No newline at end of file diff --git a/src/lib.rs b/src/lib.rs index 4a24fb4..fd803d2 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -33,6 +33,7 @@ mod keyring; mod rpc; mod session; mod unix_connection; +mod last_sign_state; #[cfg(feature = "yubihsm")] mod yubihsm; From 4bb0c03da7687eb07a79656adac9f5650a5a461f Mon Sep 17 00:00:00 2001 From: Zaki Manian Date: Mon, 4 Mar 2019 09:45:56 -0500 Subject: [PATCH 02/18] Cargo fmt --- src/last_sign_state.rs | 95 +++++++++++++++++++++++++----------------- src/lib.rs | 2 +- 2 files changed, 57 insertions(+), 40 deletions(-) diff --git a/src/last_sign_state.rs b/src/last_sign_state.rs index d4cda8c..2ecacc9 100644 --- a/src/last_sign_state.rs +++ b/src/last_sign_state.rs @@ -1,26 +1,21 @@ -use std::{ - path::Path, - fs::File, - io::prelude::*, -}; -use tendermint::chain; -use serde_json; use abscissa::Error; +use serde_json; +use std::{fs::File, io::prelude::*, path::Path}; +use tendermint::chain; - -#[derive(Serialize,Deserialize)] -struct LastSignData{ +#[derive(Serialize, Deserialize)] +struct LastSignData { pub height: i64, pub round: i64, - pub step : i8, + pub step: i8, pub signature: String, - pub signbytes: String, + pub signbytes: String, } -pub struct LastSignState{ +pub struct LastSignState { data: LastSignData, file: File, - chain_id: chain::Id + chain_id: chain::Id, } /// Error type @@ -28,7 +23,7 @@ pub struct LastSignState{ pub struct LastSignError(Error); #[derive(Copy, Clone, Eq, PartialEq, Debug, Fail)] -pub enum LastSignErrorKind{ +pub enum LastSignErrorKind { #[fail(display = "height regression")] HeightRegression, #[fail(display = "step regression")] @@ -43,48 +38,70 @@ impl From> for LastSignError { } } -impl LastSignState{ - pub fn init_state(&mut self, path: &Path) -> std::io::Result<()>{ - if path.exists(){ +impl LastSignState { + pub fn init_state(&mut self, path: &Path) -> std::io::Result<()> { + if path.exists() { return Ok(()); } self.file = File::create(self.filename())?; - self.file.write_all(serde_json::to_string(&self.data).unwrap().as_ref())?; + self.file + .write_all(serde_json::to_string(&self.data).unwrap().as_ref())?; self.file.sync_all()?; return Ok(()); } - pub fn load_state(&mut self, path:&Path) -> std::io::Result<()>{ + pub fn load_state(&mut self, path: &Path) -> std::io::Result<()> { self.file = File::open(path)?; let mut contents = String::new(); self.file.read_to_string(&mut contents)?; - self.data = serde_json::from_str(&contents).unwrap(); - return Ok(()); + self.data = serde_json::from_str(&contents).unwrap(); + return Ok(()); } - - pub fn filename(&self) -> String{ + + pub fn filename(&self) -> String { self.chain_id.as_str().to_owned() + "_validator_state.json" } - pub fn check_and_update_hrs(&mut self, height: i64, round:i64, step:i8)->Result<(),LastSignError>{ - if height < self.data.height{ - fail!(HeightRegression, "last height:{} new height:{}", self.data.height, height); - } - if height == self.data.height { - if round < self.data.round{ - fail!(RoundRegression, "round regression at height:{} last round:{} new round:{}",height, self.data.round, round) + pub fn check_and_update_hrs( + &mut self, + height: i64, + round: i64, + step: i8, + ) -> Result<(), LastSignError> { + if height < self.data.height { + fail!( + HeightRegression, + "last height:{} new height:{}", + self.data.height, + height + ); + } + if height == self.data.height { + if round < self.data.round { + fail!( + RoundRegression, + "round regression at height:{} last round:{} new round:{}", + height, + self.data.round, + round + ) } - if round == self.data.round{ - if step < self.data.step{ - fail!(StepRegression, "round regression at height:{} round:{} last step:{} new step:{}",height,round, self.data.step, step) + if round == self.data.round { + if step < self.data.step { + fail!( + StepRegression, + "round regression at height:{} round:{} last step:{} new step:{}", + height, + round, + self.data.step, + step + ) } } - } + } self.data.height = height; self.data.round = round; self.data.step = step; Ok(()) - } - - -} \ No newline at end of file + } +} diff --git a/src/lib.rs b/src/lib.rs index fd803d2..d05a550 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -30,10 +30,10 @@ mod client; mod commands; mod config; mod keyring; +mod last_sign_state; mod rpc; mod session; mod unix_connection; -mod last_sign_state; #[cfg(feature = "yubihsm")] mod yubihsm; From 455179b3c5053705baf47b7dfc8ff04126f5ec11 Mon Sep 17 00:00:00 2001 From: Zaki Manian Date: Tue, 5 Mar 2019 21:46:40 -0500 Subject: [PATCH 03/18] Add double sign detection test --- src/last_sign_state.rs | 79 +++++++++++++++++++++++++++++++++++++----- 1 file changed, 70 insertions(+), 9 deletions(-) diff --git a/src/last_sign_state.rs b/src/last_sign_state.rs index 2ecacc9..859a5d1 100644 --- a/src/last_sign_state.rs +++ b/src/last_sign_state.rs @@ -1,15 +1,15 @@ + use abscissa::Error; use serde_json; use std::{fs::File, io::prelude::*, path::Path}; -use tendermint::chain; +use tendermint::{chain, block}; #[derive(Serialize, Deserialize)] struct LastSignData { pub height: i64, pub round: i64, pub step: i8, - pub signature: String, - pub signbytes: String, + pub block_id: Option, } pub struct LastSignState { @@ -30,6 +30,8 @@ pub enum LastSignErrorKind { StepRegression, #[fail(display = "round regression")] RoundRegression, + #[fail(display = "invalid block id")] + DoubleSign, } impl From> for LastSignError { @@ -49,7 +51,6 @@ impl LastSignState { self.file.sync_all()?; return Ok(()); } - pub fn load_state(&mut self, path: &Path) -> std::io::Result<()> { self.file = File::open(path)?; let mut contents = String::new(); @@ -57,20 +58,20 @@ impl LastSignState { self.data = serde_json::from_str(&contents).unwrap(); return Ok(()); } - pub fn filename(&self) -> String { self.chain_id.as_str().to_owned() + "_validator_state.json" } - + pub fn check_and_update_hrs( &mut self, height: i64, round: i64, step: i8, + block_id:Option, ) -> Result<(), LastSignError> { if height < self.data.height { fail!( - HeightRegression, + LastSignErrorKind::HeightRegression, "last height:{} new height:{}", self.data.height, height @@ -79,7 +80,7 @@ impl LastSignState { if height == self.data.height { if round < self.data.round { fail!( - RoundRegression, + LastSignErrorKind::RoundRegression, "round regression at height:{} last round:{} new round:{}", height, self.data.round, @@ -87,9 +88,10 @@ impl LastSignState { ) } if round == self.data.round { + if step < self.data.step { fail!( - StepRegression, + LastSignErrorKind::StepRegression, "round regression at height:{} round:{} last step:{} new step:{}", height, round, @@ -97,11 +99,70 @@ impl LastSignState { step ) } + + if block_id != None && self.data.block_id != None && self.data.block_id != block_id { + fail!( + LastSignErrorKind::DoubleSign, + "Attempting to sign a second proposal at height:{} round:{} step:{} old block id:{} new block {}", + height, + round, + step, + self.data.block_id.clone().unwrap(), + block_id.unwrap() + ) + } } } self.data.height = height; self.data.round = round; self.data.step = step; + self.data.block_id = block_id; Ok(()) } } +#[cfg(test)] +mod tests { + use tendermint::{chain, block}; + use std::str::FromStr; + use super::*; + + const EXAMPLE_BLOCK_ID:&str ="26C0A41F3243C6BCD7AD2DFF8A8D83A71D29D307B5326C227F734A1A512FE47D"; + + const EXAMPLE_DOUBLE_SIGN_BLOCK_ID:&str = "2470A41F3243C6BCD7AD2DFF8A8D83A71D29D307B5326C227F734A1A512FE47D"; + + +#[test] +fn hrs_test(){ + let mut last_sign_state = LastSignState{ + data:LastSignData{ + height: 1, + round: 1, + step: 0, + block_id:None, + }, + file: File::create("/tmp/tmp_state.json").unwrap(), + chain_id:"example-chain".parse::().unwrap() + }; + assert_eq!(last_sign_state.check_and_update_hrs(2,0,0,None).unwrap(),()) +} + +#[test] +fn hrs_test_double_sign(){ + let mut last_sign_state = LastSignState{ + data:LastSignData{ + height: 1, + round: 1, + step: 0, + block_id:Some(block::Id::from_str(EXAMPLE_BLOCK_ID).unwrap()), + }, + file: File::create("/tmp/tmp_state.json").unwrap(), + chain_id:"example-chain".parse::().unwrap() + }; + let double_sign_block= block::Id::from_str(EXAMPLE_DOUBLE_SIGN_BLOCK_ID).unwrap(); + let err = last_sign_state.check_and_update_hrs(1,1,1,Some(double_sign_block)); + + let double_sign_error = LastSignErrorKind::DoubleSign; + + assert_eq!(err.expect_err("Expect Double Sign error").0.kind(),&double_sign_error) +} +} \ No newline at end of file From 11a00f2c0bffbdb341be966295bc97a91b28a677 Mon Sep 17 00:00:00 2001 From: Zaki Manian Date: Tue, 5 Mar 2019 23:19:43 -0500 Subject: [PATCH 04/18] Better Constructor --- src/last_sign_state.rs | 140 ++++++++++++++++++++++++----------------- 1 file changed, 82 insertions(+), 58 deletions(-) diff --git a/src/last_sign_state.rs b/src/last_sign_state.rs index 859a5d1..55d7645 100644 --- a/src/last_sign_state.rs +++ b/src/last_sign_state.rs @@ -1,17 +1,23 @@ - use abscissa::Error; use serde_json; use std::{fs::File, io::prelude::*, path::Path}; -use tendermint::{chain, block}; +use tendermint::{block, chain}; #[derive(Serialize, Deserialize)] struct LastSignData { pub height: i64, pub round: i64, pub step: i8, - pub block_id: Option, + pub block_id: Option, } +const EMPTY_DATA: LastSignData = LastSignData { + height: 0, + round: 0, + step: 0, + block_id: None, +}; + pub struct LastSignState { data: LastSignData, file: File, @@ -41,33 +47,44 @@ impl From> for LastSignError { } impl LastSignState { - pub fn init_state(&mut self, path: &Path) -> std::io::Result<()> { - if path.exists() { - return Ok(()); + pub fn load_state(path: &Path, chain_id: chain::Id) -> std::io::Result { + if !path.exists() { + let mut lst = LastSignState { + data: EMPTY_DATA, + file: File::create(path)?, + chain_id: chain_id, + }; + lst.sync_to_disk()?; + return Ok(lst); } - self.file = File::create(self.filename())?; - self.file - .write_all(serde_json::to_string(&self.data).unwrap().as_ref())?; - self.file.sync_all()?; - return Ok(()); - } - pub fn load_state(&mut self, path: &Path) -> std::io::Result<()> { - self.file = File::open(path)?; + let mut lst = LastSignState { + data: EMPTY_DATA, + file: File::open(path)?, + chain_id: chain_id, + }; + let mut contents = String::new(); - self.file.read_to_string(&mut contents)?; - self.data = serde_json::from_str(&contents).unwrap(); - return Ok(()); + lst.file.read_to_string(&mut contents)?; + lst.data = serde_json::from_str(&contents).unwrap(); + return Ok(lst); } pub fn filename(&self) -> String { self.chain_id.as_str().to_owned() + "_validator_state.json" } - + + pub fn sync_to_disk(&mut self) -> std::io::Result<()> { + self.file + .write_all(serde_json::to_string(&self.data).unwrap().as_ref())?; + self.file.sync_all()?; + return Ok(()); + } + pub fn check_and_update_hrs( &mut self, height: i64, round: i64, step: i8, - block_id:Option, + block_id: Option, ) -> Result<(), LastSignError> { if height < self.data.height { fail!( @@ -88,7 +105,6 @@ impl LastSignState { ) } if round == self.data.round { - if step < self.data.step { fail!( LastSignErrorKind::StepRegression, @@ -100,7 +116,8 @@ impl LastSignState { ) } - if block_id != None && self.data.block_id != None && self.data.block_id != block_id { + if block_id != None && self.data.block_id != None && self.data.block_id != block_id + { fail!( LastSignErrorKind::DoubleSign, "Attempting to sign a second proposal at height:{} round:{} step:{} old block id:{} new block {}", @@ -122,47 +139,54 @@ impl LastSignState { } #[cfg(test)] mod tests { - use tendermint::{chain, block}; - use std::str::FromStr; use super::*; + use std::str::FromStr; + use tendermint::{block, chain}; - const EXAMPLE_BLOCK_ID:&str ="26C0A41F3243C6BCD7AD2DFF8A8D83A71D29D307B5326C227F734A1A512FE47D"; + const EXAMPLE_BLOCK_ID: &str = + "26C0A41F3243C6BCD7AD2DFF8A8D83A71D29D307B5326C227F734A1A512FE47D"; - const EXAMPLE_DOUBLE_SIGN_BLOCK_ID:&str = "2470A41F3243C6BCD7AD2DFF8A8D83A71D29D307B5326C227F734A1A512FE47D"; + const EXAMPLE_DOUBLE_SIGN_BLOCK_ID: &str = + "2470A41F3243C6BCD7AD2DFF8A8D83A71D29D307B5326C227F734A1A512FE47D"; + #[test] + fn hrs_test() { + let mut last_sign_state = LastSignState { + data: LastSignData { + height: 1, + round: 1, + step: 0, + block_id: None, + }, + file: File::create("/tmp/tmp_state.json").unwrap(), + chain_id: "example-chain".parse::().unwrap(), + }; + assert_eq!( + last_sign_state.check_and_update_hrs(2, 0, 0, None).unwrap(), + () + ) + } -#[test] -fn hrs_test(){ - let mut last_sign_state = LastSignState{ - data:LastSignData{ - height: 1, - round: 1, - step: 0, - block_id:None, - }, - file: File::create("/tmp/tmp_state.json").unwrap(), - chain_id:"example-chain".parse::().unwrap() - }; - assert_eq!(last_sign_state.check_and_update_hrs(2,0,0,None).unwrap(),()) -} + #[test] + fn hrs_test_double_sign() { + let mut last_sign_state = LastSignState { + data: LastSignData { + height: 1, + round: 1, + step: 0, + block_id: Some(block::Id::from_str(EXAMPLE_BLOCK_ID).unwrap()), + }, + file: File::create("/tmp/tmp_state.json").unwrap(), + chain_id: "example-chain".parse::().unwrap(), + }; + let double_sign_block = block::Id::from_str(EXAMPLE_DOUBLE_SIGN_BLOCK_ID).unwrap(); + let err = last_sign_state.check_and_update_hrs(1, 1, 1, Some(double_sign_block)); -#[test] -fn hrs_test_double_sign(){ - let mut last_sign_state = LastSignState{ - data:LastSignData{ - height: 1, - round: 1, - step: 0, - block_id:Some(block::Id::from_str(EXAMPLE_BLOCK_ID).unwrap()), - }, - file: File::create("/tmp/tmp_state.json").unwrap(), - chain_id:"example-chain".parse::().unwrap() - }; - let double_sign_block= block::Id::from_str(EXAMPLE_DOUBLE_SIGN_BLOCK_ID).unwrap(); - let err = last_sign_state.check_and_update_hrs(1,1,1,Some(double_sign_block)); - - let double_sign_error = LastSignErrorKind::DoubleSign; - - assert_eq!(err.expect_err("Expect Double Sign error").0.kind(),&double_sign_error) + let double_sign_error = LastSignErrorKind::DoubleSign; + + assert_eq!( + err.expect_err("Expect Double Sign error").0.kind(), + &double_sign_error + ) + } } -} \ No newline at end of file From 89204d7f5c6ce750e517659444e2b0f4f5d6d2fc Mon Sep 17 00:00:00 2001 From: Zaki Manian Date: Wed, 6 Mar 2019 09:33:28 -0500 Subject: [PATCH 05/18] Introduce state checking --- src/session.rs | 8 ++++++++ tendermint-rs/src/amino_types/proposal.rs | 22 +++++++++++++++++++++- tendermint-rs/src/amino_types/signature.rs | 11 +++++++++-- tendermint-rs/src/amino_types/vote.rs | 22 +++++++++++++++++++++- 4 files changed, 59 insertions(+), 4 deletions(-) diff --git a/src/session.rs b/src/session.rs index fabd0cf..5ade36a 100644 --- a/src/session.rs +++ b/src/session.rs @@ -22,6 +22,7 @@ use tendermint::{ use crate::{ error::KmsError, keyring::KeyRing, + last_sign_state::LastSignState, prost::Message, rpc::{Request, Response, TendermintRequest}, unix_connection::UnixConnection, @@ -34,6 +35,9 @@ pub struct Session { /// TCP connection to a validator node connection: Connection, + + /// Stateful double sign defense that is synced to disk + last_sign_state: LastSignState, } impl Session> { @@ -50,10 +54,12 @@ impl Session> { let signer = Ed25519Signer::from(secret_connection_key); let public_key = SecretConnectionKey::from(ed25519::public_key(&signer)?); let connection = SecretConnection::new(socket, &public_key, &signer)?; + let last_sign_state = LastSignState::load_state(Path::new(chain_id.as_ref()), chain_id)?; Ok(Self { chain_id, connection, + last_sign_state, }) } } @@ -68,10 +74,12 @@ impl Session> { let socket = UnixStream::connect(socket_path)?; let connection = UnixConnection::new(socket); + let last_sign_state = LastSignState::load_state(Path::new(chain_id.as_ref()), chain_id)?; Ok(Self { chain_id, connection, + last_sign_state, }) } } diff --git a/tendermint-rs/src/amino_types/proposal.rs b/tendermint-rs/src/amino_types/proposal.rs index 5211b12..e8fa97e 100644 --- a/tendermint-rs/src/amino_types/proposal.rs +++ b/tendermint-rs/src/amino_types/proposal.rs @@ -1,10 +1,11 @@ use super::{ block_id::{BlockId, CanonicalBlockId, CanonicalPartSetHeader}, remote_error::RemoteError, - signature::{SignableMsg, SignedMsgType}, + signature::{ConsensusState, SignableMsg, SignedMsgType}, time::TimeMsg, validate::{ConsensusMessage, ValidationError, ValidationErrorKind::*}, }; +use crate::block::ParseId; use crate::{block, chain, error::Error}; use bytes::BufMut; use prost::{EncodeError, Message}; @@ -129,6 +130,25 @@ impl SignableMsg for SignProposalRequest { None => Err(MissingConsensusMessage.into()), } } + fn consensus_state(&self) -> Option { + match self.proposal { + Some(ref p) => Some(ConsensusState { + height: p.height, + round: p.round, + step: 1, //TODO check if correct steo + block_id: { + match p.block_id { + Some(ref b) => match b.parse_block_id() { + Ok(id) => Some(id), + Err(_) => None, + }, + None => None, + } + }, + }), + None => None, + } + } } impl ConsensusMessage for Proposal { diff --git a/tendermint-rs/src/amino_types/signature.rs b/tendermint-rs/src/amino_types/signature.rs index 004ff11..068bd3e 100644 --- a/tendermint-rs/src/amino_types/signature.rs +++ b/tendermint-rs/src/amino_types/signature.rs @@ -1,5 +1,5 @@ use super::validate::ValidationError; -use crate::chain; +use crate::{block, chain}; use bytes::BufMut; use prost::{DecodeError, EncodeError}; use signatory::ed25519; @@ -15,8 +15,15 @@ pub trait SignableMsg { /// Set the Ed25519 signature on the underlying message fn set_signature(&mut self, sig: &ed25519::Signature); - fn validate(&self) -> Result<(), ValidationError>; + fn consensus_state(&self) -> Option; +} + +pub struct ConsensusState { + pub height: i64, + pub round: i64, + pub step: i8, + pub block_id: Option, } /// Signed message types. This follows: diff --git a/tendermint-rs/src/amino_types/vote.rs b/tendermint-rs/src/amino_types/vote.rs index 3ddfa06..32cdb37 100644 --- a/tendermint-rs/src/amino_types/vote.rs +++ b/tendermint-rs/src/amino_types/vote.rs @@ -1,11 +1,12 @@ use super::{ block_id::{BlockId, CanonicalBlockId, CanonicalPartSetHeader}, remote_error::RemoteError, - signature::SignableMsg, + signature::{ConsensusState, SignableMsg}, time::TimeMsg, validate::{ConsensusMessage, ValidationError, ValidationErrorKind::*}, SignedMsgType, }; +use crate::block::ParseId; use crate::{block, chain, error::Error}; use bytes::BufMut; use prost::{error::EncodeError, Message}; @@ -150,6 +151,25 @@ impl SignableMsg for SignVoteRequest { None => Err(MissingConsensusMessage.into()), } } + fn consensus_state(&self) -> Option { + match self.vote { + Some(ref v) => Some(ConsensusState { + height: v.height, + round: v.round, + step: 5, //To Do check if correct. + block_id: { + match v.block_id { + Some(ref b) => match b.parse_block_id() { + Ok(id) => Some(id), + Err(_) => None, + }, + None => None, + } + }, + }), + None => None, + } + } } impl ConsensusMessage for Vote { From 80a30fbeb83e0fd99d0f4ac4dc67384a2ee4b2d9 Mon Sep 17 00:00:00 2001 From: Zaki Manian Date: Wed, 6 Mar 2019 11:02:27 -0500 Subject: [PATCH 06/18] Integrate double sign protection into session --- src/error.rs | 11 +++++++++++ src/last_sign_state.rs | 26 +++++++++++++++++--------- src/session.rs | 15 +++++++++++++++ tendermint-rs/src/amino_types/mod.rs | 2 +- 4 files changed, 44 insertions(+), 10 deletions(-) diff --git a/src/error.rs b/src/error.rs index 7e41f0e..a2cb553 100644 --- a/src/error.rs +++ b/src/error.rs @@ -1,5 +1,6 @@ // Error types +use crate::last_sign_state; use crate::prost; use abscissa::Error; use signatory; @@ -77,6 +78,10 @@ pub enum KmsErrorKind { /// Verification operation failed #[fail(display = "verification failed")] VerificationError, + + /// Signature invalid + #[fail(display = "attempted double sign")] + DoubleSign, } impl Display for KmsError { @@ -145,3 +150,9 @@ impl From for KmsError { err!(KmsErrorKind::InvalidMessageError, other).into() } } + +impl From for KmsError { + fn from(other: last_sign_state::LastSignError) -> Self { + err!(KmsErrorKind::DoubleSign, other).into() + } +} diff --git a/src/last_sign_state.rs b/src/last_sign_state.rs index 55d7645..8f36d9f 100644 --- a/src/last_sign_state.rs +++ b/src/last_sign_state.rs @@ -1,6 +1,11 @@ use abscissa::Error; use serde_json; -use std::{fs::File, io::prelude::*, path::Path}; +use std::{ + fmt::{self, Display}, + fs::File, + io::prelude::*, + path::Path, +}; use tendermint::{block, chain}; #[derive(Serialize, Deserialize)] @@ -21,7 +26,7 @@ const EMPTY_DATA: LastSignData = LastSignData { pub struct LastSignState { data: LastSignData, file: File, - chain_id: chain::Id, + _chain_id: chain::Id, } /// Error type @@ -46,13 +51,19 @@ impl From> for LastSignError { } } +impl Display for LastSignError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + self.0.fmt(f) + } +} + impl LastSignState { pub fn load_state(path: &Path, chain_id: chain::Id) -> std::io::Result { if !path.exists() { let mut lst = LastSignState { data: EMPTY_DATA, file: File::create(path)?, - chain_id: chain_id, + _chain_id: chain_id, }; lst.sync_to_disk()?; return Ok(lst); @@ -60,7 +71,7 @@ impl LastSignState { let mut lst = LastSignState { data: EMPTY_DATA, file: File::open(path)?, - chain_id: chain_id, + _chain_id: chain_id, }; let mut contents = String::new(); @@ -68,9 +79,6 @@ impl LastSignState { lst.data = serde_json::from_str(&contents).unwrap(); return Ok(lst); } - pub fn filename(&self) -> String { - self.chain_id.as_str().to_owned() + "_validator_state.json" - } pub fn sync_to_disk(&mut self) -> std::io::Result<()> { self.file @@ -159,7 +167,7 @@ mod tests { block_id: None, }, file: File::create("/tmp/tmp_state.json").unwrap(), - chain_id: "example-chain".parse::().unwrap(), + _chain_id: "example-chain".parse::().unwrap(), }; assert_eq!( last_sign_state.check_and_update_hrs(2, 0, 0, None).unwrap(), @@ -177,7 +185,7 @@ mod tests { block_id: Some(block::Id::from_str(EXAMPLE_BLOCK_ID).unwrap()), }, file: File::create("/tmp/tmp_state.json").unwrap(), - chain_id: "example-chain".parse::().unwrap(), + _chain_id: "example-chain".parse::().unwrap(), }; let double_sign_block = block::Id::from_str(EXAMPLE_DOUBLE_SIGN_BLOCK_ID).unwrap(); let err = last_sign_state.check_and_update_hrs(1, 1, 1, Some(double_sign_block)); diff --git a/src/session.rs b/src/session.rs index 5ade36a..452d3f7 100644 --- a/src/session.rs +++ b/src/session.rs @@ -128,6 +128,21 @@ where fn sign(&mut self, mut request: T) -> Result { request.validate()?; + match request.consensus_state() { + Some(cs) => match self.last_sign_state.check_and_update_hrs( + cs.height, + cs.round, + cs.step, + cs.block_id, + ) { + Ok(()) => (), + Err(e) => { + debug! {"Double sign event: {}",e} + return Err(KmsError::from(e)); + } + }, + None => (), + } let mut to_sign = vec![]; request.sign_bytes(self.chain_id, &mut to_sign)?; diff --git a/tendermint-rs/src/amino_types/mod.rs b/tendermint-rs/src/amino_types/mod.rs index 282cdf4..c509398 100644 --- a/tendermint-rs/src/amino_types/mod.rs +++ b/tendermint-rs/src/amino_types/mod.rs @@ -21,7 +21,7 @@ pub use self::{ proposal::{SignProposalRequest, SignedProposalResponse, AMINO_NAME as PROPOSAL_AMINO_NAME}, remote_error::RemoteError, secret_connection::AuthSigMessage, - signature::{SignableMsg, SignedMsgType}, + signature::{ConsensusState, SignableMsg, SignedMsgType}, time::TimeMsg, validate::ConsensusMessage, vote::{SignVoteRequest, SignedVoteResponse, AMINO_NAME as VOTE_AMINO_NAME}, From 6e0126567e3f54633432c78b38738d92639f10a1 Mon Sep 17 00:00:00 2001 From: Zaki Manian Date: Wed, 6 Mar 2019 11:09:55 -0500 Subject: [PATCH 07/18] Fix consensus step --- tendermint-rs/src/amino_types/proposal.rs | 2 +- tendermint-rs/src/amino_types/vote.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tendermint-rs/src/amino_types/proposal.rs b/tendermint-rs/src/amino_types/proposal.rs index e8fa97e..357dde0 100644 --- a/tendermint-rs/src/amino_types/proposal.rs +++ b/tendermint-rs/src/amino_types/proposal.rs @@ -135,7 +135,7 @@ impl SignableMsg for SignProposalRequest { Some(ref p) => Some(ConsensusState { height: p.height, round: p.round, - step: 1, //TODO check if correct steo + step: 3, block_id: { match p.block_id { Some(ref b) => match b.parse_block_id() { diff --git a/tendermint-rs/src/amino_types/vote.rs b/tendermint-rs/src/amino_types/vote.rs index 32cdb37..66c5b79 100644 --- a/tendermint-rs/src/amino_types/vote.rs +++ b/tendermint-rs/src/amino_types/vote.rs @@ -156,7 +156,7 @@ impl SignableMsg for SignVoteRequest { Some(ref v) => Some(ConsensusState { height: v.height, round: v.round, - step: 5, //To Do check if correct. + step: 6, block_id: { match v.block_id { Some(ref b) => match b.parse_block_id() { From a798ec42d2e82a439c995376b681ea977258fc65 Mon Sep 17 00:00:00 2001 From: Zaki Manian Date: Wed, 6 Mar 2019 11:13:44 -0500 Subject: [PATCH 08/18] Fix formatting --- tendermint-rs/src/amino_types/proposal.rs | 2 +- tendermint-rs/src/amino_types/vote.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tendermint-rs/src/amino_types/proposal.rs b/tendermint-rs/src/amino_types/proposal.rs index 357dde0..b64977b 100644 --- a/tendermint-rs/src/amino_types/proposal.rs +++ b/tendermint-rs/src/amino_types/proposal.rs @@ -135,7 +135,7 @@ impl SignableMsg for SignProposalRequest { Some(ref p) => Some(ConsensusState { height: p.height, round: p.round, - step: 3, + step: 3, block_id: { match p.block_id { Some(ref b) => match b.parse_block_id() { diff --git a/tendermint-rs/src/amino_types/vote.rs b/tendermint-rs/src/amino_types/vote.rs index 66c5b79..bfc041b 100644 --- a/tendermint-rs/src/amino_types/vote.rs +++ b/tendermint-rs/src/amino_types/vote.rs @@ -156,7 +156,7 @@ impl SignableMsg for SignVoteRequest { Some(ref v) => Some(ConsensusState { height: v.height, round: v.round, - step: 6, + step: 6, block_id: { match v.block_id { Some(ref b) => match b.parse_block_id() { From 12644bc9c65db70a7c298e0665a93decdbf92f89 Mon Sep 17 00:00:00 2001 From: Zaki Manian Date: Wed, 6 Mar 2019 11:23:37 -0500 Subject: [PATCH 09/18] Improve the filepaths Sync to disk on every signature --- src/session.rs | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/session.rs b/src/session.rs index 452d3f7..db12aa8 100644 --- a/src/session.rs +++ b/src/session.rs @@ -54,7 +54,10 @@ impl Session> { let signer = Ed25519Signer::from(secret_connection_key); let public_key = SecretConnectionKey::from(ed25519::public_key(&signer)?); let connection = SecretConnection::new(socket, &public_key, &signer)?; - let last_sign_state = LastSignState::load_state(Path::new(chain_id.as_ref()), chain_id)?; + let last_sign_state = LastSignState::load_state( + Path::new(&(chain_id.as_ref().to_owned() + "_priv_validator_state.json")), + chain_id, + )?; Ok(Self { chain_id, @@ -74,7 +77,10 @@ impl Session> { let socket = UnixStream::connect(socket_path)?; let connection = UnixConnection::new(socket); - let last_sign_state = LastSignState::load_state(Path::new(chain_id.as_ref()), chain_id)?; + let last_sign_state = LastSignState::load_state( + Path::new(&(chain_id.as_ref().to_owned() + "_priv_validator_state.json")), + chain_id, + )?; Ok(Self { chain_id, @@ -143,6 +149,7 @@ where }, None => (), } + self.last_sign_state.sync_to_disk()?; let mut to_sign = vec![]; request.sign_bytes(self.chain_id, &mut to_sign)?; From 252afe8cfcef9947ef83ede83339efdb7d00e7c2 Mon Sep 17 00:00:00 2001 From: Zaki Manian Date: Wed, 6 Mar 2019 11:32:12 -0500 Subject: [PATCH 10/18] Please clippy --- src/last_sign_state.rs | 4 ++-- src/session.rs | 7 +++---- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/src/last_sign_state.rs b/src/last_sign_state.rs index 8f36d9f..70272fb 100644 --- a/src/last_sign_state.rs +++ b/src/last_sign_state.rs @@ -77,14 +77,14 @@ impl LastSignState { let mut contents = String::new(); lst.file.read_to_string(&mut contents)?; lst.data = serde_json::from_str(&contents).unwrap(); - return Ok(lst); + Ok(lst) } pub fn sync_to_disk(&mut self) -> std::io::Result<()> { self.file .write_all(serde_json::to_string(&self.data).unwrap().as_ref())?; self.file.sync_all()?; - return Ok(()); + Ok(()) } pub fn check_and_update_hrs( diff --git a/src/session.rs b/src/session.rs index db12aa8..1ab5972 100644 --- a/src/session.rs +++ b/src/session.rs @@ -134,8 +134,8 @@ where fn sign(&mut self, mut request: T) -> Result { request.validate()?; - match request.consensus_state() { - Some(cs) => match self.last_sign_state.check_and_update_hrs( + if let Some(cs) = request.consensus_state() { + match self.last_sign_state.check_and_update_hrs( cs.height, cs.round, cs.step, @@ -146,8 +146,7 @@ where debug! {"Double sign event: {}",e} return Err(KmsError::from(e)); } - }, - None => (), + } } self.last_sign_state.sync_to_disk()?; let mut to_sign = vec![]; From ad05e72800f2a90f374ed57ef91a54d5d8c7f9e3 Mon Sep 17 00:00:00 2001 From: Ismail Khoffi Date: Wed, 6 Mar 2019 13:09:28 -0500 Subject: [PATCH 11/18] Update src/last_sign_state.rs Co-Authored-By: zmanian --- src/last_sign_state.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/last_sign_state.rs b/src/last_sign_state.rs index 70272fb..f6037e3 100644 --- a/src/last_sign_state.rs +++ b/src/last_sign_state.rs @@ -76,7 +76,7 @@ impl LastSignState { let mut contents = String::new(); lst.file.read_to_string(&mut contents)?; - lst.data = serde_json::from_str(&contents).unwrap(); + lst.data = serde_json::from_str(&contents)?; Ok(lst) } From 74f6c5aa20838cc0588904cb9ad5944719ba2d77 Mon Sep 17 00:00:00 2001 From: Ismail Khoffi Date: Wed, 6 Mar 2019 19:54:20 +0100 Subject: [PATCH 12/18] fix tests: file wasn't opened with write option but we were trying to write --- src/last_sign_state.rs | 6 +++--- src/session.rs | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/last_sign_state.rs b/src/last_sign_state.rs index f6037e3..a3d1759 100644 --- a/src/last_sign_state.rs +++ b/src/last_sign_state.rs @@ -2,7 +2,7 @@ use abscissa::Error; use serde_json; use std::{ fmt::{self, Display}, - fs::File, + fs::{File, OpenOptions}, io::prelude::*, path::Path, }; @@ -70,7 +70,7 @@ impl LastSignState { } let mut lst = LastSignState { data: EMPTY_DATA, - file: File::open(path)?, + file: OpenOptions::new().read(true).write(true).open(path)?, _chain_id: chain_id, }; @@ -82,7 +82,7 @@ impl LastSignState { pub fn sync_to_disk(&mut self) -> std::io::Result<()> { self.file - .write_all(serde_json::to_string(&self.data).unwrap().as_ref())?; + .write_all(serde_json::to_string(&self.data)?.as_ref())?; self.file.sync_all()?; Ok(()) } diff --git a/src/session.rs b/src/session.rs index 1ab5972..c791383 100644 --- a/src/session.rs +++ b/src/session.rs @@ -55,7 +55,7 @@ impl Session> { let public_key = SecretConnectionKey::from(ed25519::public_key(&signer)?); let connection = SecretConnection::new(socket, &public_key, &signer)?; let last_sign_state = LastSignState::load_state( - Path::new(&(chain_id.as_ref().to_owned() + "_priv_validator_state.json")), + Path::new(&(chain_id.to_string() + "_priv_validator_state.json")), chain_id, )?; From 73a1ac554a7fbe7bc1e3052567192553d135d8f8 Mon Sep 17 00:00:00 2001 From: Zaki Manian Date: Thu, 7 Mar 2019 09:43:10 -0500 Subject: [PATCH 13/18] Remove the round regression error --- tests/integration.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration.rs b/tests/integration.rs index b4586a5..983502c 100644 --- a/tests/integration.rs +++ b/tests/integration.rs @@ -291,7 +291,7 @@ fn test_handle_and_sign_proposal() { let proposal = amino_types::proposal::Proposal { msg_type: amino_types::SignedMsgType::Proposal.to_u32(), height: 12345, - round: 23456, + round: 1, timestamp: Some(t), pol_round: -1, block_id: None, From bc8fcebad333cfa433563d93e8d7fbd3abb091b5 Mon Sep 17 00:00:00 2001 From: Zaki Manian Date: Thu, 7 Mar 2019 10:41:21 -0500 Subject: [PATCH 14/18] Delete the state file at begining of the run --- tests/integration.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/integration.rs b/tests/integration.rs index 983502c..427e958 100644 --- a/tests/integration.rs +++ b/tests/integration.rs @@ -8,6 +8,7 @@ use rand::Rng; use signatory::{ed25519, encoding::Identity, Decode, Signature}; use signatory_dalek::{Ed25519Signer, Ed25519Verifier}; use std::{ + fs, io::{self, Cursor, Read, Write}, net::{TcpListener, TcpStream}, os::unix::net::{UnixListener, UnixStream}, @@ -204,6 +205,11 @@ impl ProtocolTester { where F: FnOnce(ProtocolTester), { + + //delete a state file if present + fs::remove_file("test_chain_id_priv_validator_state.json").unwrap(); + + let tcp_device = KmsProcess::create_tcp(); let tcp_connection = tcp_device.create_connection(); let unix_device = KmsProcess::create_unix(); From c229ea4e13e8dd38e0e753c6f682158fd5eebd7e Mon Sep 17 00:00:00 2001 From: Zaki Manian Date: Thu, 7 Mar 2019 10:52:44 -0500 Subject: [PATCH 15/18] fix formatting --- tests/integration.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/integration.rs b/tests/integration.rs index 427e958..9838db0 100644 --- a/tests/integration.rs +++ b/tests/integration.rs @@ -205,11 +205,9 @@ impl ProtocolTester { where F: FnOnce(ProtocolTester), { - //delete a state file if present fs::remove_file("test_chain_id_priv_validator_state.json").unwrap(); - let tcp_device = KmsProcess::create_tcp(); let tcp_connection = tcp_device.create_connection(); let unix_device = KmsProcess::create_unix(); From 9f54d31b8ee45c2b7dc1493819f67a87576132a1 Mon Sep 17 00:00:00 2001 From: Zaki Manian Date: Thu, 7 Mar 2019 10:54:16 -0500 Subject: [PATCH 16/18] Clean up tests at then end --- tests/integration.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/integration.rs b/tests/integration.rs index 9838db0..538ed78 100644 --- a/tests/integration.rs +++ b/tests/integration.rs @@ -205,8 +205,6 @@ impl ProtocolTester { where F: FnOnce(ProtocolTester), { - //delete a state file if present - fs::remove_file("test_chain_id_priv_validator_state.json").unwrap(); let tcp_device = KmsProcess::create_tcp(); let tcp_connection = tcp_device.create_connection(); @@ -226,6 +224,8 @@ impl Drop for ProtocolTester { fn drop(&mut self) { self.tcp_device.process.kill().unwrap(); self.unix_device.process.kill().unwrap(); + fs::remove_file("test_chain_id_priv_validator_state.json").unwrap(); + } } From 5e9a2f7e187157d104f191a26bc6cbc70f780428 Mon Sep 17 00:00:00 2001 From: Zaki Manian Date: Thu, 7 Mar 2019 10:54:40 -0500 Subject: [PATCH 17/18] Format --- tests/integration.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/integration.rs b/tests/integration.rs index 538ed78..4fda8da 100644 --- a/tests/integration.rs +++ b/tests/integration.rs @@ -205,7 +205,6 @@ impl ProtocolTester { where F: FnOnce(ProtocolTester), { - let tcp_device = KmsProcess::create_tcp(); let tcp_connection = tcp_device.create_connection(); let unix_device = KmsProcess::create_unix(); @@ -225,7 +224,6 @@ impl Drop for ProtocolTester { self.tcp_device.process.kill().unwrap(); self.unix_device.process.kill().unwrap(); fs::remove_file("test_chain_id_priv_validator_state.json").unwrap(); - } } From 8927d0f5369df9df91bbb542bb6c8ff2f498c5d1 Mon Sep 17 00:00:00 2001 From: Zaki Manian Date: Thu, 7 Mar 2019 11:45:22 -0500 Subject: [PATCH 18/18] Run integration tests sequentially to prevent race conditions --- .circleci/config.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 5ccd4ee..13d0c93 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -41,7 +41,7 @@ jobs: command: | rustc --version cargo --version - cargo test --all --features "default softsign yubihsm yubihsm-mock" + cargo test --all --features "default softsign yubihsm yubihsm-mock" -- --test-threads 1 - run: name: audit command: |