From fc80067f1343c48df5b3da3d349c6568553a9a28 Mon Sep 17 00:00:00 2001 From: James Date: Fri, 29 Jul 2022 18:35:02 +0200 Subject: [PATCH 1/5] feature: check signatures on double update --- agents/watcher/src/watcher.rs | 23 +++++++++++++++++++++-- nomad-core/src/types/update.rs | 2 +- 2 files changed, 22 insertions(+), 3 deletions(-) diff --git a/agents/watcher/src/watcher.rs b/agents/watcher/src/watcher.rs index 3d4a58ae..55efca67 100644 --- a/agents/watcher/src/watcher.rs +++ b/agents/watcher/src/watcher.rs @@ -12,7 +12,7 @@ use tokio::{ task::JoinHandle, time::sleep, }; -use tracing::{error, info, info_span, instrument::Instrumented, Instrument}; +use tracing::{error, info, info_span, instrument::Instrumented, warn, Instrument}; use nomad_base::{ cancel_task, AgentCore, BaseError, CachingHome, ConnectionManagers, NomadAgent, NomadDB, @@ -231,7 +231,26 @@ impl UpdateHandler { .expect("!db_get") { Some(existing) => { - if existing.update.new_root != new_root { + let existing_signer = existing.recover(); + let new_signer = update.recover(); + // if a signature verification failed. We consider this not a + // double update + if existing_signer.is_err() || new_signer.is_err() { + warn!( + existing = %existing, + new = %update, + existing_signer = ?existing_signer, + new_signer = ? new_signer, + "Signature verification on update failed" + ); + return Ok(()); + } + + // ensure both new roots are different, and the signer is the + // same + if existing.update.new_root != new_root + && existing_signer.unwrap() == new_signer.unwrap() + { error!( "UpdateHandler detected double update! Existing: {:?}. Double: {:?}.", &existing, &update diff --git a/nomad-core/src/types/update.rs b/nomad-core/src/types/update.rs index a76f2078..8ffe3a16 100644 --- a/nomad-core/src/types/update.rs +++ b/nomad-core/src/types/update.rs @@ -25,7 +25,7 @@ impl std::fmt::Display for Update { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { write!( f, - "Update(domain {} moved from {} to {})", + "Update(domain {} moved from {:?} to {:?})", self.home_domain, self.previous_root, self.new_root ) } From f9b7239630be75f29e4bcc34ba7ef2a50be7c6bc Mon Sep 17 00:00:00 2001 From: James Date: Fri, 29 Jul 2022 18:51:38 +0200 Subject: [PATCH 2/5] feature: ensure watcher agent errors if misconfigured --- agents/watcher/src/watcher.rs | 32 +++++++++++++++++++++++++------- 1 file changed, 25 insertions(+), 7 deletions(-) diff --git a/agents/watcher/src/watcher.rs b/agents/watcher/src/watcher.rs index 55efca67..6ac3d7d8 100644 --- a/agents/watcher/src/watcher.rs +++ b/agents/watcher/src/watcher.rs @@ -1,8 +1,11 @@ use async_trait::async_trait; -use color_eyre::{eyre::bail, Report, Result}; +use color_eyre::{ + eyre::{bail, ensure}, + Report, Result, +}; use thiserror::Error; -use ethers::core::types::H256; +use ethers::{core::types::H256, prelude::H160}; use futures_util::future::{join, join_all, select_all}; use prometheus::{IntGauge, IntGaugeVec}; use std::{collections::HashMap, fmt::Display, sync::Arc, time::Duration}; @@ -206,6 +209,7 @@ pub struct UpdateHandler { rx: mpsc::Receiver, watcher_db: NomadDB, home: Arc, + updater: H160, } impl UpdateHandler { @@ -213,11 +217,13 @@ impl UpdateHandler { rx: mpsc::Receiver, watcher_db: NomadDB, home: Arc, + updater: H160, ) -> Self { Self { rx, watcher_db, home, + updater, } } @@ -246,11 +252,12 @@ impl UpdateHandler { return Ok(()); } + let existing_signer = existing_signer.unwrap(); + let new_signer = new_signer.unwrap(); + // ensure both new roots are different, and the signer is the - // same - if existing.update.new_root != new_root - && existing_signer.unwrap() == new_signer.unwrap() - { + // same. we perform this check in addition + if existing.update.new_root != new_root && existing_signer == new_signer { error!( "UpdateHandler detected double update! Existing: {:?}. Double: {:?}.", &existing, &update @@ -287,6 +294,14 @@ impl UpdateHandler { let update = update.unwrap(); let old_root = update.update.previous_root; + // This check may appear redundant with the check in + // `check_double_update` that signers match, however, + // this is + ensure!( + update.verify(self.updater).is_ok(), + "Handling update signed by another updater. This agent is misconfigured" + ); + if old_root == self.home.committed_root().await? { // It is okay if tx reverts let _ = self.home.update(&update).await; @@ -374,9 +389,10 @@ impl Watcher { let updates_inspected_for_double = self.updates_inspected_for_double.clone(); tokio::spawn(async move { + let updater = home.updater().await?; // Spawn update handler let (tx, rx) = mpsc::channel(200); - let handler = UpdateHandler::new(rx, watcher_db, home.clone()).spawn(); + let handler = UpdateHandler::new(rx, watcher_db, home.clone(), updater.into()).spawn(); // For each replica, spawn polling and history syncing tasks info!("Spawning replica watch and sync tasks..."); @@ -907,6 +923,7 @@ mod test { "1111111111111111111111111111111111111111111111111111111111111111" .parse() .unwrap(); + let updater = signer.address(); let first_root = H256::from([1; 32]); let second_root = H256::from([2; 32]); @@ -976,6 +993,7 @@ mod test { rx, watcher_db: nomad_db, home, + updater, }; handler From 7ac9c853345f0001095633d8fd1b34e8730796a6 Mon Sep 17 00:00:00 2001 From: James Date: Fri, 29 Jul 2022 18:52:34 +0200 Subject: [PATCH 3/5] chore: changelog --- agents/watcher/CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/agents/watcher/CHANGELOG.md b/agents/watcher/CHANGELOG.md index e7e8bbf0..3dd39a17 100644 --- a/agents/watcher/CHANGELOG.md +++ b/agents/watcher/CHANGELOG.md @@ -2,6 +2,9 @@ ### Unreleased +- update handler now errors if incoming updates have an unexpected updater +- double-update routine now checks that both updates are signed by the same + updater - Add English description to XCM error log, change to use `Display` ### agents@1.1.0 From b7c619bb2c9f0440d2e925185606539c894d8e54 Mon Sep 17 00:00:00 2001 From: James Date: Fri, 29 Jul 2022 18:56:25 +0200 Subject: [PATCH 4/5] fix: better error message --- agents/watcher/src/watcher.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/agents/watcher/src/watcher.rs b/agents/watcher/src/watcher.rs index 6ac3d7d8..001f809a 100644 --- a/agents/watcher/src/watcher.rs +++ b/agents/watcher/src/watcher.rs @@ -299,7 +299,7 @@ impl UpdateHandler { // this is ensure!( update.verify(self.updater).is_ok(), - "Handling update signed by another updater. This agent is misconfigured" + "Handling update signed by another updater. Hint: This agent may misconfigured, or the updater may have rotated while this agent was running" ); if old_root == self.home.committed_root().await? { From 72cae4414ddd2b64f3437875556ba39471338866 Mon Sep 17 00:00:00 2001 From: James Date: Sun, 31 Jul 2022 17:20:25 +0200 Subject: [PATCH 5/5] fix: avoid double logging message in processor --- agents/processor/src/processor.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/agents/processor/src/processor.rs b/agents/processor/src/processor.rs index 35afd071..73db8963 100644 --- a/agents/processor/src/processor.rs +++ b/agents/processor/src/processor.rs @@ -225,7 +225,7 @@ impl Replica { Ok(Flow::Advance) } - #[instrument(err, level = "info", skip(self), fields(self = %self, domain = message.message.destination, nonce = message.message.nonce, leaf_index = message.leaf_index, leaf = ?message.message.to_leaf()))] + #[instrument(err, level = "info", skip(self, message), fields(self = %self, domain = message.message.destination, nonce = message.message.nonce, leaf_index = message.leaf_index, leaf = ?message.message.to_leaf()))] /// Dispatch a message for processing. If the message is already proven, process only. async fn process(&self, message: CommittedMessage, proof: NomadProof) -> Result<()> { use nomad_core::Replica;