From e091f0d4b8a524a6527b07eae12780356ce2d26c Mon Sep 17 00:00:00 2001 From: DJO <790521+Alenar@users.noreply.github.com> Date: Tue, 26 Nov 2024 11:29:36 +0100 Subject: [PATCH 01/20] feat(client-lib): scaffold cache system for certificates validation As a `unstable` feature --- mithril-client/src/certificate_client.rs | 14 ++++++++++++++ mithril-client/src/client.rs | 21 +++++++++++++++++++++ 2 files changed, 35 insertions(+) diff --git a/mithril-client/src/certificate_client.rs b/mithril-client/src/certificate_client.rs index de33a81cf82..83f54ea5a77 100644 --- a/mithril-client/src/certificate_client.rs +++ b/mithril-client/src/certificate_client.rs @@ -82,6 +82,8 @@ pub struct CertificateClient { aggregator_client: Arc, retriever: Arc, verifier: Arc, + #[cfg(feature = "unstable")] + _verifier_cache: Option>, } /// API that defines how to validate certificates. @@ -93,11 +95,19 @@ pub trait CertificateVerifier: Sync + Send { async fn verify_chain(&self, certificate: &MithrilCertificate) -> MithrilResult<()>; } +/// API that defines how to cache certificates validation results. +#[cfg(feature = "unstable")] +#[cfg_attr(test, mockall::automock)] +#[cfg_attr(target_family = "wasm", async_trait(?Send))] +#[cfg_attr(not(target_family = "wasm"), async_trait)] +pub trait CertificateVerifierCache: Sync + Send {} + impl CertificateClient { /// Constructs a new `CertificateClient`. pub fn new( aggregator_client: Arc, verifier: Arc, + #[cfg(feature = "unstable")] verifier_cache: Option>, logger: Logger, ) -> Self { let logger = logger.new_with_component_name::(); @@ -110,6 +120,8 @@ impl CertificateClient { aggregator_client, retriever, verifier, + #[cfg(feature = "unstable")] + _verifier_cache: verifier_cache, } } @@ -307,6 +319,8 @@ mod tests { CertificateClient::new( aggregator_client, verifier.unwrap_or(Arc::new(MockCertificateVerifier::new())), + #[cfg(feature = "unstable")] + None, test_utils::test_logger(), ) } diff --git a/mithril-client/src/client.rs b/mithril-client/src/client.rs index 6f52dab99d2..42d40359398 100644 --- a/mithril-client/src/client.rs +++ b/mithril-client/src/client.rs @@ -10,6 +10,8 @@ use mithril_common::api_version::APIVersionProvider; use crate::aggregator_client::{AggregatorClient, AggregatorHTTPClient}; use crate::cardano_stake_distribution_client::CardanoStakeDistributionClient; use crate::cardano_transaction_client::CardanoTransactionClient; +#[cfg(feature = "unstable")] +use crate::certificate_client::CertificateVerifierCache; use crate::certificate_client::{ CertificateClient, CertificateVerifier, MithrilCertificateVerifier, }; @@ -94,6 +96,8 @@ pub struct ClientBuilder { genesis_verification_key: String, aggregator_client: Option>, certificate_verifier: Option>, + #[cfg(feature = "unstable")] + certificate_verifier_cache: Option>, #[cfg(feature = "fs")] snapshot_downloader: Option>, logger: Option, @@ -110,6 +114,8 @@ impl ClientBuilder { genesis_verification_key: genesis_verification_key.to_string(), aggregator_client: None, certificate_verifier: None, + #[cfg(feature = "unstable")] + certificate_verifier_cache: None, #[cfg(feature = "fs")] snapshot_downloader: None, logger: None, @@ -128,6 +134,8 @@ impl ClientBuilder { genesis_verification_key: genesis_verification_key.to_string(), aggregator_client: None, certificate_verifier: None, + #[cfg(feature = "unstable")] + certificate_verifier_cache: None, #[cfg(feature = "fs")] snapshot_downloader: None, logger: None, @@ -197,6 +205,8 @@ impl ClientBuilder { let certificate_client = Arc::new(CertificateClient::new( aggregator_client.clone(), certificate_verifier, + #[cfg(feature = "unstable")] + self.certificate_verifier_cache, logger.clone(), )); @@ -243,6 +253,17 @@ impl ClientBuilder { self } + cfg_unstable! { + /// Set the [CertificateVerifierCache] that will be used to cache certificate validation results. + pub fn with_certificate_verifier_cache( + mut self, + certificate_verifier_cache: Arc, + ) -> ClientBuilder { + self.certificate_verifier_cache = Some(certificate_verifier_cache); + self + } + } + cfg_fs! { /// Set the [SnapshotDownloader] that will be used to download snapshots. pub fn with_snapshot_downloader( From ac0e65ba23eaf818cb1fff6dbbfad55a97e6acd1 Mon Sep 17 00:00:00 2001 From: DJO <790521+Alenar@users.noreply.github.com> Date: Tue, 26 Nov 2024 13:04:45 +0100 Subject: [PATCH 02/20] refactor(client-cli): promote `certificate_client` module to directory To split its code in more manageable part. --- mithril-client/src/certificate_client.rs | 533 ------------------ .../src/certificate_client/fetch.rs | 224 ++++++++ mithril-client/src/certificate_client/mod.rs | 128 +++++ .../src/certificate_client/verify.rs | 236 ++++++++ .../src/certificate_client/verify_cache.rs | 10 + 5 files changed, 598 insertions(+), 533 deletions(-) delete mode 100644 mithril-client/src/certificate_client.rs create mode 100644 mithril-client/src/certificate_client/fetch.rs create mode 100644 mithril-client/src/certificate_client/mod.rs create mode 100644 mithril-client/src/certificate_client/verify.rs create mode 100644 mithril-client/src/certificate_client/verify_cache.rs diff --git a/mithril-client/src/certificate_client.rs b/mithril-client/src/certificate_client.rs deleted file mode 100644 index 83f54ea5a77..00000000000 --- a/mithril-client/src/certificate_client.rs +++ /dev/null @@ -1,533 +0,0 @@ -//! A client which retrieves and validates certificates from an Aggregator. -//! -//! In order to do so it defines a [CertificateClient] exposes the following features: -//! - [get][CertificateClient::get]: get a certificate data from its hash -//! - [list][CertificateClient::list]: get the list of available certificates -//! - [verify_chain][CertificateClient::verify_chain]: verify a certificate chain -//! -//! # Get a certificate -//! -//! To get a certificate using the [ClientBuilder][crate::client::ClientBuilder]. -//! -//! ```no_run -//! # async fn run() -> mithril_client::MithrilResult<()> { -//! use mithril_client::ClientBuilder; -//! -//! let client = ClientBuilder::aggregator("YOUR_AGGREGATOR_ENDPOINT", "YOUR_GENESIS_VERIFICATION_KEY").build()?; -//! let certificate = client.certificate().get("CERTIFICATE_HASH").await?.unwrap(); -//! -//! println!("Certificate hash={}, signed_message={}", certificate.hash, certificate.signed_message); -//! # Ok(()) -//! # } -//! ``` -//! -//! # List available certificates -//! -//! To list available certificates using the [ClientBuilder][crate::client::ClientBuilder]. -//! -//! ```no_run -//! # async fn run() -> mithril_client::MithrilResult<()> { -//! use mithril_client::ClientBuilder; -//! -//! let client = mithril_client::ClientBuilder::aggregator("YOUR_AGGREGATOR_ENDPOINT", "YOUR_GENESIS_VERIFICATION_KEY").build()?; -//! let certificates = client.certificate().list().await?; -//! -//! for certificate in certificates { -//! println!("Certificate hash={}, signed_message={}", certificate.hash, certificate.signed_message); -//! } -//! # Ok(()) -//! # } -//! ``` -//! -//! # Validate a certificate chain -//! -//! To validate a certificate using the [ClientBuilder][crate::client::ClientBuilder]. -//! -//! ```no_run -//! # async fn run() -> mithril_client::MithrilResult<()> { -//! use mithril_client::ClientBuilder; -//! -//! let client = ClientBuilder::aggregator("YOUR_AGGREGATOR_ENDPOINT", "YOUR_GENESIS_VERIFICATION_KEY").build()?; -//! let certificate = client.certificate().verify_chain("CERTIFICATE_HASH").await?; -//! -//! println!("Chain of Certificate (hash: {}) is valid", certificate.hash); -//! # Ok(()) -//! # } -//! ``` - -use std::sync::Arc; - -use anyhow::{anyhow, Context}; -use async_trait::async_trait; -use slog::{crit, Logger}; - -use mithril_common::{ - certificate_chain::{ - CertificateRetriever, CertificateRetrieverError, - CertificateVerifier as CommonCertificateVerifier, - MithrilCertificateVerifier as CommonMithrilCertificateVerifier, - }, - crypto_helper::ProtocolGenesisVerificationKey, - entities::Certificate, - logging::LoggerExtensions, - messages::CertificateMessage, -}; - -use crate::aggregator_client::{AggregatorClient, AggregatorClientError, AggregatorRequest}; -use crate::feedback::{FeedbackSender, MithrilEvent}; -use crate::{MithrilCertificate, MithrilCertificateListItem, MithrilResult}; - -/// Aggregator client for the Certificate -pub struct CertificateClient { - aggregator_client: Arc, - retriever: Arc, - verifier: Arc, - #[cfg(feature = "unstable")] - _verifier_cache: Option>, -} - -/// API that defines how to validate certificates. -#[cfg_attr(test, mockall::automock)] -#[cfg_attr(target_family = "wasm", async_trait(?Send))] -#[cfg_attr(not(target_family = "wasm"), async_trait)] -pub trait CertificateVerifier: Sync + Send { - /// Validate the chain starting with the given certificate. - async fn verify_chain(&self, certificate: &MithrilCertificate) -> MithrilResult<()>; -} - -/// API that defines how to cache certificates validation results. -#[cfg(feature = "unstable")] -#[cfg_attr(test, mockall::automock)] -#[cfg_attr(target_family = "wasm", async_trait(?Send))] -#[cfg_attr(not(target_family = "wasm"), async_trait)] -pub trait CertificateVerifierCache: Sync + Send {} - -impl CertificateClient { - /// Constructs a new `CertificateClient`. - pub fn new( - aggregator_client: Arc, - verifier: Arc, - #[cfg(feature = "unstable")] verifier_cache: Option>, - logger: Logger, - ) -> Self { - let logger = logger.new_with_component_name::(); - let retriever = Arc::new(InternalCertificateRetriever { - aggregator_client: aggregator_client.clone(), - logger, - }); - - Self { - aggregator_client, - retriever, - verifier, - #[cfg(feature = "unstable")] - _verifier_cache: verifier_cache, - } - } - - /// Fetch a list of certificates - pub async fn list(&self) -> MithrilResult> { - let response = self - .aggregator_client - .get_content(AggregatorRequest::ListCertificates) - .await - .with_context(|| "CertificateClient can not get the certificate list")?; - let items = serde_json::from_str::>(&response) - .with_context(|| "CertificateClient can not deserialize certificate list")?; - - Ok(items) - } - - /// Get a single certificate full information from the aggregator. - pub async fn get(&self, certificate_hash: &str) -> MithrilResult> { - self.retriever.get(certificate_hash).await - } - - /// Validate the chain starting with the certificate with given `certificate_hash`, return the certificate if - /// the chain is valid. - /// - /// This method will fail if no certificate exists for the given `certificate_hash`. - pub async fn verify_chain(&self, certificate_hash: &str) -> MithrilResult { - let certificate = self.retriever.get(certificate_hash).await?.ok_or(anyhow!( - "No certificate exist for hash '{certificate_hash}'" - ))?; - - self.verifier - .verify_chain(&certificate) - .await - .with_context(|| { - format!("Certificate chain of certificate '{certificate_hash}' is invalid") - })?; - - Ok(certificate) - } -} - -/// Internal type to implement the [InternalCertificateRetriever] trait and avoid a circular -/// dependency between the [CertificateClient] and the [CommonMithrilCertificateVerifier] that need -/// a [CertificateRetriever] as a dependency. -struct InternalCertificateRetriever { - aggregator_client: Arc, - logger: Logger, -} - -impl InternalCertificateRetriever { - async fn get(&self, certificate_hash: &str) -> MithrilResult> { - let response = self - .aggregator_client - .get_content(AggregatorRequest::GetCertificate { - hash: certificate_hash.to_string(), - }) - .await; - - match response { - Err(AggregatorClientError::RemoteServerLogical(_)) => Ok(None), - Err(e) => Err(e.into()), - Ok(response) => { - let message = - serde_json::from_str::(&response).inspect_err(|e| { - crit!( - self.logger, "Could not create certificate from API message"; - "error" => e.to_string(), - "raw_message" => response - ); - })?; - - Ok(Some(message)) - } - } - } -} - -/// Implementation of a [CertificateVerifier] that can send feedbacks using -/// the [feedback][crate::feedback] mechanism. -pub struct MithrilCertificateVerifier { - internal_verifier: Arc, - genesis_verification_key: ProtocolGenesisVerificationKey, - feedback_sender: FeedbackSender, -} - -impl MithrilCertificateVerifier { - /// Constructs a new `MithrilCertificateVerifier`. - pub fn new( - aggregator_client: Arc, - genesis_verification_key: &str, - feedback_sender: FeedbackSender, - logger: Logger, - ) -> MithrilResult { - let logger = logger.new_with_component_name::(); - let retriever = Arc::new(InternalCertificateRetriever { - aggregator_client: aggregator_client.clone(), - logger: logger.clone(), - }); - let internal_verifier = Arc::new(CommonMithrilCertificateVerifier::new( - logger, - retriever.clone(), - )); - let genesis_verification_key = - ProtocolGenesisVerificationKey::try_from(genesis_verification_key) - .with_context(|| "Invalid genesis verification key")?; - - Ok(Self { - internal_verifier, - genesis_verification_key, - feedback_sender, - }) - } -} - -#[cfg_attr(target_family = "wasm", async_trait(?Send))] -#[cfg_attr(not(target_family = "wasm"), async_trait)] -impl CertificateVerifier for MithrilCertificateVerifier { - async fn verify_chain(&self, certificate: &MithrilCertificate) -> MithrilResult<()> { - // Todo: move most of this code in the `mithril_common` verifier by defining - // a new `verify_chain` method that take a callback called when a certificate is - // validated. - let certificate_chain_validation_id = MithrilEvent::new_certificate_chain_validation_id(); - self.feedback_sender - .send_event(MithrilEvent::CertificateChainValidationStarted { - certificate_chain_validation_id: certificate_chain_validation_id.clone(), - }) - .await; - - let mut current_certificate = certificate.clone().try_into()?; - loop { - let previous_or_none = self - .internal_verifier - .verify_certificate(¤t_certificate, &self.genesis_verification_key) - .await?; - - self.feedback_sender - .send_event(MithrilEvent::CertificateValidated { - certificate_hash: current_certificate.hash.clone(), - certificate_chain_validation_id: certificate_chain_validation_id.clone(), - }) - .await; - - match previous_or_none { - Some(previous_certificate) => current_certificate = previous_certificate, - None => break, - } - } - - self.feedback_sender - .send_event(MithrilEvent::CertificateChainValidated { - certificate_chain_validation_id, - }) - .await; - - Ok(()) - } -} - -#[cfg_attr(target_family = "wasm", async_trait(?Send))] -#[cfg_attr(not(target_family = "wasm"), async_trait)] -impl CertificateRetriever for InternalCertificateRetriever { - async fn get_certificate_details( - &self, - certificate_hash: &str, - ) -> Result { - self.get(certificate_hash) - .await - .map_err(CertificateRetrieverError)? - .map(|message| message.try_into()) - .transpose() - .map_err(CertificateRetrieverError)? - .ok_or(CertificateRetrieverError(anyhow!(format!( - "Certificate does not exist: '{}'", - certificate_hash - )))) - } -} - -#[cfg(test)] -mod tests { - use mithril_common::crypto_helper::tests_setup::setup_certificate_chain; - use mithril_common::test_utils::fake_data; - use mockall::predicate::eq; - - use crate::aggregator_client::MockAggregatorHTTPClient; - use crate::feedback::StackFeedbackReceiver; - use crate::test_utils; - - use super::*; - - fn build_client( - aggregator_client: Arc, - verifier: Option>, - ) -> CertificateClient { - CertificateClient::new( - aggregator_client, - verifier.unwrap_or(Arc::new(MockCertificateVerifier::new())), - #[cfg(feature = "unstable")] - None, - test_utils::test_logger(), - ) - } - - #[tokio::test] - async fn get_certificate_list() { - let expected = vec![ - MithrilCertificateListItem { - hash: "cert-hash-123".to_string(), - ..MithrilCertificateListItem::dummy() - }, - MithrilCertificateListItem { - hash: "cert-hash-456".to_string(), - ..MithrilCertificateListItem::dummy() - }, - ]; - let message = expected.clone(); - let mut aggregator_client = MockAggregatorHTTPClient::new(); - aggregator_client - .expect_get_content() - .return_once(move |_| Ok(serde_json::to_string(&message).unwrap())); - let certificate_client = build_client(Arc::new(aggregator_client), None); - let items = certificate_client.list().await.unwrap(); - - assert_eq!(expected, items); - } - - #[tokio::test] - async fn get_certificate_empty_list() { - let mut aggregator_client = MockAggregatorHTTPClient::new(); - aggregator_client - .expect_get_content() - .return_once(move |_| { - Ok(serde_json::to_string::>(&vec![]).unwrap()) - }); - let certificate_client = build_client(Arc::new(aggregator_client), None); - let items = certificate_client.list().await.unwrap(); - - assert!(items.is_empty()); - } - - #[tokio::test] - async fn test_show_ok_some() { - let mut aggregator_client = MockAggregatorHTTPClient::new(); - let certificate_hash = "cert-hash-123".to_string(); - let certificate = fake_data::certificate(certificate_hash.clone()); - let expected_certificate = certificate.clone(); - aggregator_client - .expect_get_content() - .return_once(move |_| { - let message: CertificateMessage = certificate.try_into().unwrap(); - Ok(serde_json::to_string(&message).unwrap()) - }) - .times(1); - - let certificate_client = build_client(Arc::new(aggregator_client), None); - let cert = certificate_client - .get("cert-hash-123") - .await - .unwrap() - .expect("The certificate should be found") - .try_into() - .unwrap(); - - assert_eq!(expected_certificate, cert); - } - - #[tokio::test] - async fn test_show_ok_none() { - let mut aggregator_client = MockAggregatorHTTPClient::new(); - aggregator_client - .expect_get_content() - .return_once(move |_| { - Err(AggregatorClientError::RemoteServerLogical(anyhow!( - "an error" - ))) - }) - .times(1); - - let certificate_client = build_client(Arc::new(aggregator_client), None); - assert!(certificate_client - .get("cert-hash-123") - .await - .unwrap() - .is_none()); - } - - #[tokio::test] - async fn test_show_ko() { - let mut aggregator_client = MockAggregatorHTTPClient::new(); - aggregator_client - .expect_get_content() - .return_once(move |_| { - Err(AggregatorClientError::RemoteServerTechnical(anyhow!( - "an error" - ))) - }) - .times(1); - - let certificate_client = build_client(Arc::new(aggregator_client), None); - certificate_client - .get("cert-hash-123") - .await - .expect_err("The certificate client should fail here."); - } - - #[tokio::test] - async fn validating_chain_send_feedbacks() { - let (chain, verifier) = setup_certificate_chain(3, 1); - let verification_key: String = verifier.to_verification_key().try_into().unwrap(); - let mut aggregator_client = MockAggregatorHTTPClient::new(); - let last_certificate_hash = chain.first().unwrap().hash.clone(); - - for certificate in chain.clone() { - let hash = certificate.hash.clone(); - let message = serde_json::to_string( - &TryInto::::try_into(certificate).unwrap(), - ) - .unwrap(); - aggregator_client - .expect_get_content() - .with(eq(AggregatorRequest::GetCertificate { hash })) - .returning(move |_| Ok(message.to_owned())); - } - - let aggregator_client = Arc::new(aggregator_client); - let feedback_receiver = Arc::new(StackFeedbackReceiver::new()); - let certificate_client = build_client( - aggregator_client.clone(), - Some(Arc::new( - MithrilCertificateVerifier::new( - aggregator_client, - &verification_key, - FeedbackSender::new(&[feedback_receiver.clone()]), - test_utils::test_logger(), - ) - .unwrap(), - )), - ); - - certificate_client - .verify_chain(&last_certificate_hash) - .await - .expect("Chain validation should succeed"); - - let actual = feedback_receiver.stacked_events(); - let id = actual[0].event_id(); - - let expected = { - let mut vec = vec![MithrilEvent::CertificateChainValidationStarted { - certificate_chain_validation_id: id.to_string(), - }]; - vec.extend( - chain - .into_iter() - .map(|c| MithrilEvent::CertificateValidated { - certificate_chain_validation_id: id.to_string(), - certificate_hash: c.hash, - }), - ); - vec.push(MithrilEvent::CertificateChainValidated { - certificate_chain_validation_id: id.to_string(), - }); - vec - }; - - assert_eq!(actual, expected); - } - - #[tokio::test] - async fn verify_chain_return_certificate_with_given_hash() { - let (chain, verifier) = setup_certificate_chain(3, 1); - let verification_key: String = verifier.to_verification_key().try_into().unwrap(); - let mut aggregator_client = MockAggregatorHTTPClient::new(); - let last_certificate_hash = chain.first().unwrap().hash.clone(); - - for certificate in chain.clone() { - let hash = certificate.hash.clone(); - let message = serde_json::to_string( - &TryInto::::try_into(certificate).unwrap(), - ) - .unwrap(); - aggregator_client - .expect_get_content() - .with(eq(AggregatorRequest::GetCertificate { hash })) - .returning(move |_| Ok(message.to_owned())); - } - - let aggregator_client = Arc::new(aggregator_client); - let certificate_client = build_client( - aggregator_client.clone(), - Some(Arc::new( - MithrilCertificateVerifier::new( - aggregator_client, - &verification_key, - FeedbackSender::new(&[]), - test_utils::test_logger(), - ) - .unwrap(), - )), - ); - - let certificate = certificate_client - .verify_chain(&last_certificate_hash) - .await - .expect("Chain validation should succeed"); - - assert_eq!(certificate.hash, last_certificate_hash); - } -} diff --git a/mithril-client/src/certificate_client/fetch.rs b/mithril-client/src/certificate_client/fetch.rs new file mode 100644 index 00000000000..498a0d9ba6c --- /dev/null +++ b/mithril-client/src/certificate_client/fetch.rs @@ -0,0 +1,224 @@ +use anyhow::{anyhow, Context}; +use async_trait::async_trait; +use slog::{crit, Logger}; +use std::sync::Arc; + +use mithril_common::certificate_chain::{CertificateRetriever, CertificateRetrieverError}; +use mithril_common::entities::Certificate; +use mithril_common::messages::CertificateMessage; + +use crate::aggregator_client::{AggregatorClient, AggregatorClientError, AggregatorRequest}; +use crate::certificate_client::CertificateClient; +use crate::{MithrilCertificate, MithrilCertificateListItem, MithrilResult}; + +impl CertificateClient { + /// Fetch a list of certificates + pub async fn list(&self) -> MithrilResult> { + let response = self + .aggregator_client + .get_content(AggregatorRequest::ListCertificates) + .await + .with_context(|| "CertificateClient can not get the certificate list")?; + let items = serde_json::from_str::>(&response) + .with_context(|| "CertificateClient can not deserialize certificate list")?; + + Ok(items) + } + + /// Get a single certificate full information from the aggregator. + pub async fn get(&self, certificate_hash: &str) -> MithrilResult> { + self.retriever.get(certificate_hash).await + } +} + +/// Internal type to implement the [InternalCertificateRetriever] trait and avoid a circular +/// dependency between the [CertificateClient] and the [CommonMithrilCertificateVerifier] that need +/// a [CertificateRetriever] as a dependency. +pub(super) struct InternalCertificateRetriever { + aggregator_client: Arc, + logger: Logger, +} + +impl InternalCertificateRetriever { + pub(super) fn new( + aggregator_client: Arc, + logger: Logger, + ) -> InternalCertificateRetriever { + InternalCertificateRetriever { + aggregator_client, + logger, + } + } + + pub(super) async fn get( + &self, + certificate_hash: &str, + ) -> MithrilResult> { + let response = self + .aggregator_client + .get_content(AggregatorRequest::GetCertificate { + hash: certificate_hash.to_string(), + }) + .await; + + match response { + Err(AggregatorClientError::RemoteServerLogical(_)) => Ok(None), + Err(e) => Err(e.into()), + Ok(response) => { + let message = + serde_json::from_str::(&response).inspect_err(|e| { + crit!( + self.logger, "Could not create certificate from API message"; + "error" => e.to_string(), + "raw_message" => response + ); + })?; + + Ok(Some(message)) + } + } + } +} + +#[cfg_attr(target_family = "wasm", async_trait(?Send))] +#[cfg_attr(not(target_family = "wasm"), async_trait)] +impl CertificateRetriever for InternalCertificateRetriever { + async fn get_certificate_details( + &self, + certificate_hash: &str, + ) -> Result { + self.get(certificate_hash) + .await + .map_err(CertificateRetrieverError)? + .map(|message| message.try_into()) + .transpose() + .map_err(CertificateRetrieverError)? + .ok_or(CertificateRetrieverError(anyhow!(format!( + "Certificate does not exist: '{}'", + certificate_hash + )))) + } +} + +#[cfg(test)] +mod tests { + use mithril_common::test_utils::fake_data; + + use crate::aggregator_client::MockAggregatorHTTPClient; + use crate::certificate_client::MockCertificateVerifier; + use crate::test_utils; + + use super::*; + + fn build_client(aggregator_client: Arc) -> CertificateClient { + CertificateClient::new( + aggregator_client, + Arc::new(MockCertificateVerifier::new()), + #[cfg(feature = "unstable")] + None, + test_utils::test_logger(), + ) + } + + #[tokio::test] + async fn get_certificate_list() { + let expected = vec![ + MithrilCertificateListItem { + hash: "cert-hash-123".to_string(), + ..MithrilCertificateListItem::dummy() + }, + MithrilCertificateListItem { + hash: "cert-hash-456".to_string(), + ..MithrilCertificateListItem::dummy() + }, + ]; + let message = expected.clone(); + let mut aggregator_client = MockAggregatorHTTPClient::new(); + aggregator_client + .expect_get_content() + .return_once(move |_| Ok(serde_json::to_string(&message).unwrap())); + let certificate_client = build_client(Arc::new(aggregator_client)); + let items = certificate_client.list().await.unwrap(); + + assert_eq!(expected, items); + } + + #[tokio::test] + async fn get_certificate_empty_list() { + let mut aggregator_client = MockAggregatorHTTPClient::new(); + aggregator_client + .expect_get_content() + .return_once(move |_| { + Ok(serde_json::to_string::>(&vec![]).unwrap()) + }); + let certificate_client = build_client(Arc::new(aggregator_client)); + let items = certificate_client.list().await.unwrap(); + + assert!(items.is_empty()); + } + + #[tokio::test] + async fn test_show_ok_some() { + let mut aggregator_client = MockAggregatorHTTPClient::new(); + let certificate_hash = "cert-hash-123".to_string(); + let certificate = fake_data::certificate(certificate_hash.clone()); + let expected_certificate = certificate.clone(); + aggregator_client + .expect_get_content() + .return_once(move |_| { + let message: CertificateMessage = certificate.try_into().unwrap(); + Ok(serde_json::to_string(&message).unwrap()) + }) + .times(1); + + let certificate_client = build_client(Arc::new(aggregator_client)); + let cert = certificate_client + .get("cert-hash-123") + .await + .unwrap() + .expect("The certificate should be found") + .try_into() + .unwrap(); + + assert_eq!(expected_certificate, cert); + } + + #[tokio::test] + async fn test_show_ok_none() { + let mut aggregator_client = MockAggregatorHTTPClient::new(); + aggregator_client + .expect_get_content() + .return_once(move |_| { + Err(AggregatorClientError::RemoteServerLogical(anyhow!( + "an error" + ))) + }) + .times(1); + + let certificate_client = build_client(Arc::new(aggregator_client)); + assert!(certificate_client + .get("cert-hash-123") + .await + .unwrap() + .is_none()); + } + + #[tokio::test] + async fn test_show_ko() { + let mut aggregator_client = MockAggregatorHTTPClient::new(); + aggregator_client + .expect_get_content() + .return_once(move |_| { + Err(AggregatorClientError::RemoteServerTechnical(anyhow!( + "an error" + ))) + }) + .times(1); + + let certificate_client = build_client(Arc::new(aggregator_client)); + certificate_client + .get("cert-hash-123") + .await + .expect_err("The certificate client should fail here."); + } +} diff --git a/mithril-client/src/certificate_client/mod.rs b/mithril-client/src/certificate_client/mod.rs new file mode 100644 index 00000000000..6e790bd2888 --- /dev/null +++ b/mithril-client/src/certificate_client/mod.rs @@ -0,0 +1,128 @@ +//! A client which retrieves and validates certificates from an Aggregator. +//! +//! In order to do so it defines a [CertificateClient] exposes the following features: +//! - [get][CertificateClient::get]: get a certificate data from its hash +//! - [list][CertificateClient::list]: get the list of available certificates +//! - [verify_chain][CertificateClient::verify_chain]: verify a certificate chain +//! +//! # Get a certificate +//! +//! To get a certificate using the [ClientBuilder][crate::client::ClientBuilder]. +//! +//! ```no_run +//! # async fn run() -> mithril_client::MithrilResult<()> { +//! use mithril_client::ClientBuilder; +//! +//! let client = ClientBuilder::aggregator("YOUR_AGGREGATOR_ENDPOINT", "YOUR_GENESIS_VERIFICATION_KEY").build()?; +//! let certificate = client.certificate().get("CERTIFICATE_HASH").await?.unwrap(); +//! +//! println!("Certificate hash={}, signed_message={}", certificate.hash, certificate.signed_message); +//! # Ok(()) +//! # } +//! ``` +//! +//! # List available certificates +//! +//! To list available certificates using the [ClientBuilder][crate::client::ClientBuilder]. +//! +//! ```no_run +//! # async fn run() -> mithril_client::MithrilResult<()> { +//! use mithril_client::ClientBuilder; +//! +//! let client = mithril_client::ClientBuilder::aggregator("YOUR_AGGREGATOR_ENDPOINT", "YOUR_GENESIS_VERIFICATION_KEY").build()?; +//! let certificates = client.certificate().list().await?; +//! +//! for certificate in certificates { +//! println!("Certificate hash={}, signed_message={}", certificate.hash, certificate.signed_message); +//! } +//! # Ok(()) +//! # } +//! ``` +//! +//! # Validate a certificate chain +//! +//! To validate a certificate using the [ClientBuilder][crate::client::ClientBuilder]. +//! +//! ```no_run +//! # async fn run() -> mithril_client::MithrilResult<()> { +//! use mithril_client::ClientBuilder; +//! +//! let client = ClientBuilder::aggregator("YOUR_AGGREGATOR_ENDPOINT", "YOUR_GENESIS_VERIFICATION_KEY").build()?; +//! let certificate = client.certificate().verify_chain("CERTIFICATE_HASH").await?; +//! +//! println!("Chain of Certificate (hash: {}) is valid", certificate.hash); +//! # Ok(()) +//! # } +//! ``` + +mod fetch; +mod verify; +#[cfg(feature = "unstable")] +mod verify_cache; + +use fetch::*; +pub use verify::*; +#[cfg(feature = "unstable")] +pub use verify_cache::*; + +use crate::aggregator_client::AggregatorClient; +use mithril_common::logging::LoggerExtensions; +use std::sync::Arc; + +/// Aggregator client for the Certificate +pub struct CertificateClient { + aggregator_client: Arc, + retriever: Arc, + verifier: Arc, + #[cfg(feature = "unstable")] + _verifier_cache: Option>, +} + +impl CertificateClient { + /// Constructs a new `CertificateClient`. + pub fn new( + aggregator_client: Arc, + verifier: Arc, + #[cfg(feature = "unstable")] verifier_cache: Option>, + logger: slog::Logger, + ) -> Self { + let logger = logger.new_with_component_name::(); + let retriever = Arc::new(InternalCertificateRetriever::new( + aggregator_client.clone(), + logger, + )); + + Self { + aggregator_client, + retriever, + verifier, + #[cfg(feature = "unstable")] + _verifier_cache: verifier_cache, + } + } +} + +#[cfg(test)] +pub mod tests_utils { + use mockall::predicate::eq; + + use mithril_common::entities::Certificate; + use mithril_common::messages::CertificateMessage; + + use crate::aggregator_client::{AggregatorRequest, MockAggregatorHTTPClient}; + + impl MockAggregatorHTTPClient { + pub fn expect_certificate_chain(&mut self, certificate_chain: Vec) { + for certificate in certificate_chain { + let hash = certificate.hash.clone(); + let message = serde_json::to_string( + &TryInto::::try_into(certificate).unwrap(), + ) + .unwrap(); + self.expect_get_content() + .with(eq(AggregatorRequest::GetCertificate { hash })) + .returning(move |_| Ok(message.to_owned())); + } + } + } +} diff --git a/mithril-client/src/certificate_client/verify.rs b/mithril-client/src/certificate_client/verify.rs new file mode 100644 index 00000000000..f73427c6ab6 --- /dev/null +++ b/mithril-client/src/certificate_client/verify.rs @@ -0,0 +1,236 @@ +use anyhow::{anyhow, Context}; +use async_trait::async_trait; +use slog::Logger; +use std::sync::Arc; + +use mithril_common::{ + certificate_chain::{ + CertificateVerifier as CommonCertificateVerifier, + MithrilCertificateVerifier as CommonMithrilCertificateVerifier, + }, + crypto_helper::ProtocolGenesisVerificationKey, + logging::LoggerExtensions, +}; + +use crate::aggregator_client::AggregatorClient; +use crate::certificate_client::fetch::InternalCertificateRetriever; +use crate::certificate_client::CertificateClient; +use crate::feedback::{FeedbackSender, MithrilEvent}; +use crate::{MithrilCertificate, MithrilResult}; + +/// API that defines how to validate certificates. +#[cfg_attr(test, mockall::automock)] +#[cfg_attr(target_family = "wasm", async_trait(?Send))] +#[cfg_attr(not(target_family = "wasm"), async_trait)] +pub trait CertificateVerifier: Sync + Send { + /// Validate the chain starting with the given certificate. + async fn verify_chain(&self, certificate: &MithrilCertificate) -> MithrilResult<()>; +} + +impl CertificateClient { + /// Validate the chain starting with the certificate with given `certificate_hash`, return the certificate if + /// the chain is valid. + /// + /// This method will fail if no certificate exists for the given `certificate_hash`. + pub async fn verify_chain(&self, certificate_hash: &str) -> MithrilResult { + let certificate = self.retriever.get(certificate_hash).await?.ok_or(anyhow!( + "No certificate exist for hash '{certificate_hash}'" + ))?; + + self.verifier + .verify_chain(&certificate) + .await + .with_context(|| { + format!("Certificate chain of certificate '{certificate_hash}' is invalid") + })?; + + Ok(certificate) + } +} + +/// Implementation of a [CertificateVerifier] that can send feedbacks using +/// the [feedback][crate::feedback] mechanism. +pub struct MithrilCertificateVerifier { + internal_verifier: Arc, + genesis_verification_key: ProtocolGenesisVerificationKey, + feedback_sender: FeedbackSender, +} + +impl MithrilCertificateVerifier { + /// Constructs a new `MithrilCertificateVerifier`. + pub fn new( + aggregator_client: Arc, + genesis_verification_key: &str, + feedback_sender: FeedbackSender, + logger: Logger, + ) -> MithrilResult { + let logger = logger.new_with_component_name::(); + let retriever = Arc::new(InternalCertificateRetriever::new( + aggregator_client, + logger.clone(), + )); + let internal_verifier = Arc::new(CommonMithrilCertificateVerifier::new( + logger, + retriever.clone(), + )); + let genesis_verification_key = + ProtocolGenesisVerificationKey::try_from(genesis_verification_key) + .with_context(|| "Invalid genesis verification key")?; + + Ok(Self { + internal_verifier, + genesis_verification_key, + feedback_sender, + }) + } +} + +#[cfg_attr(target_family = "wasm", async_trait(?Send))] +#[cfg_attr(not(target_family = "wasm"), async_trait)] +impl CertificateVerifier for MithrilCertificateVerifier { + async fn verify_chain(&self, certificate: &MithrilCertificate) -> MithrilResult<()> { + // Todo: move most of this code in the `mithril_common` verifier by defining + // a new `verify_chain` method that take a callback called when a certificate is + // validated. + let certificate_chain_validation_id = MithrilEvent::new_certificate_chain_validation_id(); + self.feedback_sender + .send_event(MithrilEvent::CertificateChainValidationStarted { + certificate_chain_validation_id: certificate_chain_validation_id.clone(), + }) + .await; + + let mut current_certificate = certificate.clone().try_into()?; + loop { + let previous_or_none = self + .internal_verifier + .verify_certificate(¤t_certificate, &self.genesis_verification_key) + .await?; + + self.feedback_sender + .send_event(MithrilEvent::CertificateValidated { + certificate_hash: current_certificate.hash.clone(), + certificate_chain_validation_id: certificate_chain_validation_id.clone(), + }) + .await; + + match previous_or_none { + Some(previous_certificate) => current_certificate = previous_certificate, + None => break, + } + } + + self.feedback_sender + .send_event(MithrilEvent::CertificateChainValidated { + certificate_chain_validation_id, + }) + .await; + + Ok(()) + } +} + +#[cfg(test)] +mod tests { + use mithril_common::crypto_helper::tests_setup::setup_certificate_chain; + + use crate::aggregator_client::MockAggregatorHTTPClient; + use crate::feedback::StackFeedbackReceiver; + use crate::test_utils; + + use super::*; + + fn build_client( + aggregator_client: Arc, + verifier: Arc, + ) -> CertificateClient { + CertificateClient::new( + aggregator_client, + verifier, + #[cfg(feature = "unstable")] + None, + test_utils::test_logger(), + ) + } + + #[tokio::test] + async fn validating_chain_send_feedbacks() { + let (chain, verifier) = setup_certificate_chain(3, 1); + let verification_key: String = verifier.to_verification_key().try_into().unwrap(); + let mut aggregator_client = MockAggregatorHTTPClient::new(); + aggregator_client.expect_certificate_chain(chain.clone()); + let last_certificate_hash = chain.first().unwrap().hash.clone(); + + let aggregator_client = Arc::new(aggregator_client); + let feedback_receiver = Arc::new(StackFeedbackReceiver::new()); + let certificate_client = build_client( + aggregator_client.clone(), + Arc::new( + MithrilCertificateVerifier::new( + aggregator_client, + &verification_key, + FeedbackSender::new(&[feedback_receiver.clone()]), + test_utils::test_logger(), + ) + .unwrap(), + ), + ); + + certificate_client + .verify_chain(&last_certificate_hash) + .await + .expect("Chain validation should succeed"); + + let actual = feedback_receiver.stacked_events(); + let id = actual[0].event_id(); + + let expected = { + let mut vec = vec![MithrilEvent::CertificateChainValidationStarted { + certificate_chain_validation_id: id.to_string(), + }]; + vec.extend( + chain + .into_iter() + .map(|c| MithrilEvent::CertificateValidated { + certificate_chain_validation_id: id.to_string(), + certificate_hash: c.hash, + }), + ); + vec.push(MithrilEvent::CertificateChainValidated { + certificate_chain_validation_id: id.to_string(), + }); + vec + }; + + assert_eq!(actual, expected); + } + + #[tokio::test] + async fn verify_chain_return_certificate_with_given_hash() { + let (chain, verifier) = setup_certificate_chain(3, 1); + let verification_key: String = verifier.to_verification_key().try_into().unwrap(); + let mut aggregator_client = MockAggregatorHTTPClient::new(); + aggregator_client.expect_certificate_chain(chain.clone()); + let last_certificate_hash = chain.first().unwrap().hash.clone(); + + let aggregator_client = Arc::new(aggregator_client); + let certificate_client = build_client( + aggregator_client.clone(), + Arc::new( + MithrilCertificateVerifier::new( + aggregator_client, + &verification_key, + FeedbackSender::new(&[]), + test_utils::test_logger(), + ) + .unwrap(), + ), + ); + + let certificate = certificate_client + .verify_chain(&last_certificate_hash) + .await + .expect("Chain validation should succeed"); + + assert_eq!(certificate.hash, last_certificate_hash); + } +} diff --git a/mithril-client/src/certificate_client/verify_cache.rs b/mithril-client/src/certificate_client/verify_cache.rs new file mode 100644 index 00000000000..ce67ed5af0b --- /dev/null +++ b/mithril-client/src/certificate_client/verify_cache.rs @@ -0,0 +1,10 @@ +use async_trait::async_trait; + +/// API that defines how to cache certificates validation results. +#[cfg_attr(test, mockall::automock)] +#[cfg_attr(target_family = "wasm", async_trait(?Send))] +#[cfg_attr(not(target_family = "wasm"), async_trait)] +pub trait CertificateVerifierCache: Sync + Send {} + +#[cfg(test)] +mod tests {} From b5a953d01b172a6c5b717db3f2844f898b23ec93 Mon Sep 17 00:00:00 2001 From: DJO <790521+Alenar@users.noreply.github.com> Date: Wed, 27 Nov 2024 12:33:10 +0100 Subject: [PATCH 03/20] refactor(client-lib): implement the certificate client only once By using private static function so behavior are still implemented in splitted modules. --- mithril-client/src/certificate_client/api.rs | 75 +++++++++++++++++++ .../src/certificate_client/fetch.rs | 37 ++++----- mithril-client/src/certificate_client/mod.rs | 44 +---------- .../src/certificate_client/verify.rs | 44 +++++------ .../src/certificate_client/verify_cache.rs | 8 -- 5 files changed, 117 insertions(+), 91 deletions(-) create mode 100644 mithril-client/src/certificate_client/api.rs diff --git a/mithril-client/src/certificate_client/api.rs b/mithril-client/src/certificate_client/api.rs new file mode 100644 index 00000000000..690a5fbf515 --- /dev/null +++ b/mithril-client/src/certificate_client/api.rs @@ -0,0 +1,75 @@ +use async_trait::async_trait; +use mithril_common::logging::LoggerExtensions; +use std::sync::Arc; + +use crate::aggregator_client::AggregatorClient; +use crate::certificate_client::fetch::InternalCertificateRetriever; +use crate::certificate_client::{fetch, verify}; +use crate::{MithrilCertificate, MithrilCertificateListItem, MithrilResult}; + +/// Aggregator client for the Certificate +pub struct CertificateClient { + pub(super) aggregator_client: Arc, + pub(super) retriever: Arc, + pub(super) verifier: Arc, + #[cfg(feature = "unstable")] + pub(super) _verifier_cache: Option>, +} + +impl CertificateClient { + /// Constructs a new `CertificateClient`. + pub fn new( + aggregator_client: Arc, + verifier: Arc, + #[cfg(feature = "unstable")] verifier_cache: Option>, + logger: slog::Logger, + ) -> Self { + let logger = logger.new_with_component_name::(); + let retriever = Arc::new(InternalCertificateRetriever::new( + aggregator_client.clone(), + logger, + )); + + Self { + aggregator_client, + retriever, + verifier, + #[cfg(feature = "unstable")] + _verifier_cache: verifier_cache, + } + } + + /// Fetch a list of certificates + pub async fn list(&self) -> MithrilResult> { + fetch::list(self).await + } + + /// Get a single certificate full information from the aggregator. + pub async fn get(&self, certificate_hash: &str) -> MithrilResult> { + fetch::get(self, certificate_hash).await + } + + /// Validate the chain starting with the certificate with given `certificate_hash`, return the certificate if + /// the chain is valid. + /// + /// This method will fail if no certificate exists for the given `certificate_hash`. + pub async fn verify_chain(&self, certificate_hash: &str) -> MithrilResult { + verify::verify_chain(self, certificate_hash).await + } +} + +/// API that defines how to validate certificates. +#[cfg_attr(test, mockall::automock)] +#[cfg_attr(target_family = "wasm", async_trait(?Send))] +#[cfg_attr(not(target_family = "wasm"), async_trait)] +pub trait CertificateVerifier: Sync + Send { + /// Validate the chain starting with the given certificate. + async fn verify_chain(&self, certificate: &MithrilCertificate) -> MithrilResult<()>; +} + +#[cfg(feature = "unstable")] +/// API that defines how to cache certificates validation results. +#[cfg_attr(test, mockall::automock)] +#[cfg_attr(target_family = "wasm", async_trait(?Send))] +#[cfg_attr(not(target_family = "wasm"), async_trait)] +pub trait CertificateVerifierCache: Sync + Send {} diff --git a/mithril-client/src/certificate_client/fetch.rs b/mithril-client/src/certificate_client/fetch.rs index 498a0d9ba6c..e5e6a512155 100644 --- a/mithril-client/src/certificate_client/fetch.rs +++ b/mithril-client/src/certificate_client/fetch.rs @@ -11,24 +11,27 @@ use crate::aggregator_client::{AggregatorClient, AggregatorClientError, Aggregat use crate::certificate_client::CertificateClient; use crate::{MithrilCertificate, MithrilCertificateListItem, MithrilResult}; -impl CertificateClient { - /// Fetch a list of certificates - pub async fn list(&self) -> MithrilResult> { - let response = self - .aggregator_client - .get_content(AggregatorRequest::ListCertificates) - .await - .with_context(|| "CertificateClient can not get the certificate list")?; - let items = serde_json::from_str::>(&response) - .with_context(|| "CertificateClient can not deserialize certificate list")?; - - Ok(items) - } +#[inline] +pub(super) async fn list( + client: &CertificateClient, +) -> MithrilResult> { + let response = client + .aggregator_client + .get_content(AggregatorRequest::ListCertificates) + .await + .with_context(|| "CertificateClient can not get the certificate list")?; + let items = serde_json::from_str::>(&response) + .with_context(|| "CertificateClient can not deserialize certificate list")?; + + Ok(items) +} - /// Get a single certificate full information from the aggregator. - pub async fn get(&self, certificate_hash: &str) -> MithrilResult> { - self.retriever.get(certificate_hash).await - } +#[inline] +pub(super) async fn get( + client: &CertificateClient, + certificate_hash: &str, +) -> MithrilResult> { + client.retriever.get(certificate_hash).await } /// Internal type to implement the [InternalCertificateRetriever] trait and avoid a circular diff --git a/mithril-client/src/certificate_client/mod.rs b/mithril-client/src/certificate_client/mod.rs index 6e790bd2888..6ecc4ba26eb 100644 --- a/mithril-client/src/certificate_client/mod.rs +++ b/mithril-client/src/certificate_client/mod.rs @@ -55,52 +55,14 @@ //! # } //! ``` +mod api; mod fetch; mod verify; #[cfg(feature = "unstable")] mod verify_cache; -use fetch::*; -pub use verify::*; -#[cfg(feature = "unstable")] -pub use verify_cache::*; - -use crate::aggregator_client::AggregatorClient; -use mithril_common::logging::LoggerExtensions; -use std::sync::Arc; - -/// Aggregator client for the Certificate -pub struct CertificateClient { - aggregator_client: Arc, - retriever: Arc, - verifier: Arc, - #[cfg(feature = "unstable")] - _verifier_cache: Option>, -} - -impl CertificateClient { - /// Constructs a new `CertificateClient`. - pub fn new( - aggregator_client: Arc, - verifier: Arc, - #[cfg(feature = "unstable")] verifier_cache: Option>, - logger: slog::Logger, - ) -> Self { - let logger = logger.new_with_component_name::(); - let retriever = Arc::new(InternalCertificateRetriever::new( - aggregator_client.clone(), - logger, - )); - - Self { - aggregator_client, - retriever, - verifier, - #[cfg(feature = "unstable")] - _verifier_cache: verifier_cache, - } - } -} +pub use api::*; +pub use verify::MithrilCertificateVerifier; #[cfg(test)] pub mod tests_utils { diff --git a/mithril-client/src/certificate_client/verify.rs b/mithril-client/src/certificate_client/verify.rs index f73427c6ab6..e76cc2b8203 100644 --- a/mithril-client/src/certificate_client/verify.rs +++ b/mithril-client/src/certificate_client/verify.rs @@ -14,38 +14,32 @@ use mithril_common::{ use crate::aggregator_client::AggregatorClient; use crate::certificate_client::fetch::InternalCertificateRetriever; -use crate::certificate_client::CertificateClient; +use crate::certificate_client::{CertificateClient, CertificateVerifier}; use crate::feedback::{FeedbackSender, MithrilEvent}; use crate::{MithrilCertificate, MithrilResult}; -/// API that defines how to validate certificates. -#[cfg_attr(test, mockall::automock)] -#[cfg_attr(target_family = "wasm", async_trait(?Send))] -#[cfg_attr(not(target_family = "wasm"), async_trait)] -pub trait CertificateVerifier: Sync + Send { - /// Validate the chain starting with the given certificate. - async fn verify_chain(&self, certificate: &MithrilCertificate) -> MithrilResult<()>; -} - -impl CertificateClient { - /// Validate the chain starting with the certificate with given `certificate_hash`, return the certificate if - /// the chain is valid. - /// - /// This method will fail if no certificate exists for the given `certificate_hash`. - pub async fn verify_chain(&self, certificate_hash: &str) -> MithrilResult { - let certificate = self.retriever.get(certificate_hash).await?.ok_or(anyhow!( +#[inline] +pub(super) async fn verify_chain( + client: &CertificateClient, + certificate_hash: &str, +) -> MithrilResult { + let certificate = client + .retriever + .get(certificate_hash) + .await? + .ok_or(anyhow!( "No certificate exist for hash '{certificate_hash}'" ))?; - self.verifier - .verify_chain(&certificate) - .await - .with_context(|| { - format!("Certificate chain of certificate '{certificate_hash}' is invalid") - })?; + client + .verifier + .verify_chain(&certificate) + .await + .with_context(|| { + format!("Certificate chain of certificate '{certificate_hash}' is invalid") + })?; - Ok(certificate) - } + Ok(certificate) } /// Implementation of a [CertificateVerifier] that can send feedbacks using diff --git a/mithril-client/src/certificate_client/verify_cache.rs b/mithril-client/src/certificate_client/verify_cache.rs index ce67ed5af0b..5c6016f6a14 100644 --- a/mithril-client/src/certificate_client/verify_cache.rs +++ b/mithril-client/src/certificate_client/verify_cache.rs @@ -1,10 +1,2 @@ -use async_trait::async_trait; - -/// API that defines how to cache certificates validation results. -#[cfg_attr(test, mockall::automock)] -#[cfg_attr(target_family = "wasm", async_trait(?Send))] -#[cfg_attr(not(target_family = "wasm"), async_trait)] -pub trait CertificateVerifierCache: Sync + Send {} - #[cfg(test)] mod tests {} From bc38aa4abe9b66d3c17b173f01774500c653173a Mon Sep 17 00:00:00 2001 From: DJO <790521+Alenar@users.noreply.github.com> Date: Wed, 27 Nov 2024 15:56:30 +0100 Subject: [PATCH 04/20] refactor(client-cli): move cache instance directly to the verifier As it will be its only consumer, the client should not have to use it directly. --- mithril-client/src/certificate_client/api.rs | 5 ----- .../src/certificate_client/fetch.rs | 2 -- .../src/certificate_client/verify.rs | 19 ++++++++++++------- .../src/certificate_client/verify_cache.rs | 2 -- .../certificate_client/verify_cache/mod.rs | 1 + mithril-client/src/client.rs | 4 ++-- 6 files changed, 15 insertions(+), 18 deletions(-) delete mode 100644 mithril-client/src/certificate_client/verify_cache.rs create mode 100644 mithril-client/src/certificate_client/verify_cache/mod.rs diff --git a/mithril-client/src/certificate_client/api.rs b/mithril-client/src/certificate_client/api.rs index 690a5fbf515..38db6f1419a 100644 --- a/mithril-client/src/certificate_client/api.rs +++ b/mithril-client/src/certificate_client/api.rs @@ -12,8 +12,6 @@ pub struct CertificateClient { pub(super) aggregator_client: Arc, pub(super) retriever: Arc, pub(super) verifier: Arc, - #[cfg(feature = "unstable")] - pub(super) _verifier_cache: Option>, } impl CertificateClient { @@ -21,7 +19,6 @@ impl CertificateClient { pub fn new( aggregator_client: Arc, verifier: Arc, - #[cfg(feature = "unstable")] verifier_cache: Option>, logger: slog::Logger, ) -> Self { let logger = logger.new_with_component_name::(); @@ -34,8 +31,6 @@ impl CertificateClient { aggregator_client, retriever, verifier, - #[cfg(feature = "unstable")] - _verifier_cache: verifier_cache, } } diff --git a/mithril-client/src/certificate_client/fetch.rs b/mithril-client/src/certificate_client/fetch.rs index e5e6a512155..0b9baee2785 100644 --- a/mithril-client/src/certificate_client/fetch.rs +++ b/mithril-client/src/certificate_client/fetch.rs @@ -117,8 +117,6 @@ mod tests { CertificateClient::new( aggregator_client, Arc::new(MockCertificateVerifier::new()), - #[cfg(feature = "unstable")] - None, test_utils::test_logger(), ) } diff --git a/mithril-client/src/certificate_client/verify.rs b/mithril-client/src/certificate_client/verify.rs index e76cc2b8203..85f422002e6 100644 --- a/mithril-client/src/certificate_client/verify.rs +++ b/mithril-client/src/certificate_client/verify.rs @@ -14,6 +14,8 @@ use mithril_common::{ use crate::aggregator_client::AggregatorClient; use crate::certificate_client::fetch::InternalCertificateRetriever; +#[cfg(feature = "unstable")] +use crate::certificate_client::CertificateVerifierCache; use crate::certificate_client::{CertificateClient, CertificateVerifier}; use crate::feedback::{FeedbackSender, MithrilEvent}; use crate::{MithrilCertificate, MithrilResult}; @@ -48,6 +50,8 @@ pub struct MithrilCertificateVerifier { internal_verifier: Arc, genesis_verification_key: ProtocolGenesisVerificationKey, feedback_sender: FeedbackSender, + #[cfg(feature = "unstable")] + _verifier_cache: Option>, } impl MithrilCertificateVerifier { @@ -56,6 +60,7 @@ impl MithrilCertificateVerifier { aggregator_client: Arc, genesis_verification_key: &str, feedback_sender: FeedbackSender, + #[cfg(feature = "unstable")] verifier_cache: Option>, logger: Logger, ) -> MithrilResult { let logger = logger.new_with_component_name::(); @@ -75,6 +80,8 @@ impl MithrilCertificateVerifier { internal_verifier, genesis_verification_key, feedback_sender, + #[cfg(feature = "unstable")] + _verifier_cache: verifier_cache, }) } } @@ -137,13 +144,7 @@ mod tests { aggregator_client: Arc, verifier: Arc, ) -> CertificateClient { - CertificateClient::new( - aggregator_client, - verifier, - #[cfg(feature = "unstable")] - None, - test_utils::test_logger(), - ) + CertificateClient::new(aggregator_client, verifier, test_utils::test_logger()) } #[tokio::test] @@ -163,6 +164,8 @@ mod tests { aggregator_client, &verification_key, FeedbackSender::new(&[feedback_receiver.clone()]), + #[cfg(feature = "unstable")] + None, test_utils::test_logger(), ) .unwrap(), @@ -214,6 +217,8 @@ mod tests { aggregator_client, &verification_key, FeedbackSender::new(&[]), + #[cfg(feature = "unstable")] + None, test_utils::test_logger(), ) .unwrap(), diff --git a/mithril-client/src/certificate_client/verify_cache.rs b/mithril-client/src/certificate_client/verify_cache.rs deleted file mode 100644 index 5c6016f6a14..00000000000 --- a/mithril-client/src/certificate_client/verify_cache.rs +++ /dev/null @@ -1,2 +0,0 @@ -#[cfg(test)] -mod tests {} diff --git a/mithril-client/src/certificate_client/verify_cache/mod.rs b/mithril-client/src/certificate_client/verify_cache/mod.rs new file mode 100644 index 00000000000..8b137891791 --- /dev/null +++ b/mithril-client/src/certificate_client/verify_cache/mod.rs @@ -0,0 +1 @@ + diff --git a/mithril-client/src/client.rs b/mithril-client/src/client.rs index 42d40359398..7d39e71ae71 100644 --- a/mithril-client/src/client.rs +++ b/mithril-client/src/client.rs @@ -196,6 +196,8 @@ impl ClientBuilder { aggregator_client.clone(), &self.genesis_verification_key, feedback_sender.clone(), + #[cfg(feature = "unstable")] + self.certificate_verifier_cache, logger.clone(), ) .with_context(|| "Building certificate verifier failed")?, @@ -205,8 +207,6 @@ impl ClientBuilder { let certificate_client = Arc::new(CertificateClient::new( aggregator_client.clone(), certificate_verifier, - #[cfg(feature = "unstable")] - self.certificate_verifier_cache, logger.clone(), )); From 9cdf019f48c1202de74340abdb43685acdb40977 Mon Sep 17 00:00:00 2001 From: DJO <790521+Alenar@users.noreply.github.com> Date: Wed, 27 Nov 2024 18:31:31 +0100 Subject: [PATCH 05/20] feat(client-lib): define verifier cache api and a simple in memory implementation --- mithril-client/src/certificate_client/api.rs | 15 +- mithril-client/src/certificate_client/mod.rs | 6 +- .../verify_cache/memory_cache.rs | 253 ++++++++++++++++++ .../certificate_client/verify_cache/mod.rs | 2 + 4 files changed, 273 insertions(+), 3 deletions(-) create mode 100644 mithril-client/src/certificate_client/verify_cache/memory_cache.rs diff --git a/mithril-client/src/certificate_client/api.rs b/mithril-client/src/certificate_client/api.rs index 38db6f1419a..4e8343d0177 100644 --- a/mithril-client/src/certificate_client/api.rs +++ b/mithril-client/src/certificate_client/api.rs @@ -67,4 +67,17 @@ pub trait CertificateVerifier: Sync + Send { #[cfg_attr(test, mockall::automock)] #[cfg_attr(target_family = "wasm", async_trait(?Send))] #[cfg_attr(not(target_family = "wasm"), async_trait)] -pub trait CertificateVerifierCache: Sync + Send {} +pub trait CertificateVerifierCache: Sync + Send { + /// Store a validated certificate hash and its parent hash in the cache. + async fn store_validated_certificate( + &self, + certificate_hash: &str, + previous_certificate_hash: &str, + ) -> MithrilResult<()>; + + /// Get the previous hash of the certificate with the given hash if available in the cache. + async fn get_previous_hash(&self, certificate_hash: &str) -> MithrilResult>; + + /// Reset the stored values + async fn reset(&self) -> MithrilResult<()>; +} diff --git a/mithril-client/src/certificate_client/mod.rs b/mithril-client/src/certificate_client/mod.rs index 6ecc4ba26eb..449b5b001c3 100644 --- a/mithril-client/src/certificate_client/mod.rs +++ b/mithril-client/src/certificate_client/mod.rs @@ -63,9 +63,11 @@ mod verify_cache; pub use api::*; pub use verify::MithrilCertificateVerifier; +#[cfg(feature = "unstable")] +pub use verify_cache::MemoryCertificateVerifierCache; #[cfg(test)] -pub mod tests_utils { +pub(crate) mod tests_utils { use mockall::predicate::eq; use mithril_common::entities::Certificate; @@ -74,7 +76,7 @@ pub mod tests_utils { use crate::aggregator_client::{AggregatorRequest, MockAggregatorHTTPClient}; impl MockAggregatorHTTPClient { - pub fn expect_certificate_chain(&mut self, certificate_chain: Vec) { + pub(crate) fn expect_certificate_chain(&mut self, certificate_chain: Vec) { for certificate in certificate_chain { let hash = certificate.hash.clone(); let message = serde_json::to_string( diff --git a/mithril-client/src/certificate_client/verify_cache/memory_cache.rs b/mithril-client/src/certificate_client/verify_cache/memory_cache.rs new file mode 100644 index 00000000000..11834095ff0 --- /dev/null +++ b/mithril-client/src/certificate_client/verify_cache/memory_cache.rs @@ -0,0 +1,253 @@ +use async_trait::async_trait; +use std::collections::HashMap; +use tokio::sync::RwLock; + +use mithril_common::entities::Certificate; + +use crate::certificate_client::CertificateVerifierCache; +use crate::MithrilResult; + +pub type CertificateHash = str; +pub type PreviousCertificateHash = str; + +/// A in-memory cache for the certificate verifier. +#[derive(Default)] +pub struct MemoryCertificateVerifierCache { + /// Hashmap of certificate hash and their parent hash. + cache: RwLock>, +} + +impl MemoryCertificateVerifierCache { + #[cfg(test)] + async fn content(&self) -> HashMap { + self.cache.read().await.clone() + } +} + +impl From<&[(&CertificateHash, &PreviousCertificateHash)]> for MemoryCertificateVerifierCache { + fn from(slice: &[(&CertificateHash, &PreviousCertificateHash)]) -> Self { + MemoryCertificateVerifierCache { + cache: RwLock::new( + slice + .iter() + .map(|(k, v)| (k.to_string(), v.to_string())) + .collect(), + ), + } + } +} + +impl<'a> FromIterator<(&'a CertificateHash, &'a PreviousCertificateHash)> + for MemoryCertificateVerifierCache +{ + fn from_iter>( + iter: T, + ) -> Self { + MemoryCertificateVerifierCache { + cache: RwLock::new( + iter.into_iter() + .map(|(k, v)| (k.to_string(), v.to_string())) + .collect(), + ), + } + } +} + +impl<'a> FromIterator<&'a Certificate> for MemoryCertificateVerifierCache { + fn from_iter>(iter: T) -> Self { + MemoryCertificateVerifierCache { + cache: RwLock::new( + iter.into_iter() + .map(|cert| (cert.hash.clone(), cert.previous_hash.clone())) + .collect(), + ), + } + } +} + +#[cfg_attr(target_family = "wasm", async_trait(?Send))] +#[cfg_attr(not(target_family = "wasm"), async_trait)] +impl CertificateVerifierCache for MemoryCertificateVerifierCache { + async fn store_validated_certificate( + &self, + certificate_hash: &CertificateHash, + previous_certificate_hash: &PreviousCertificateHash, + ) -> MithrilResult<()> { + // todo: should we raise an error if an empty string is given for previous_certificate_hash ? (or any other kind of validation) + let mut cache = self.cache.write().await; + cache.insert( + certificate_hash.to_string(), + previous_certificate_hash.to_string(), + ); + Ok(()) + } + + async fn get_previous_hash( + &self, + certificate_hash: &CertificateHash, + ) -> MithrilResult> { + let cache = self.cache.read().await; + Ok(cache.get(certificate_hash).cloned()) + } + + async fn reset(&self) -> MithrilResult<()> { + let mut cache = self.cache.write().await; + cache.clear(); + Ok(()) + } +} + +#[cfg(test)] +mod tests { + use mithril_common::test_utils::fake_data; + + use super::*; + + #[tokio::test] + async fn from_str_iterator() { + let cache = + MemoryCertificateVerifierCache::from_iter([("first", "one"), ("second", "two")]); + + assert_eq!( + HashMap::from_iter([ + ("first".to_string(), "one".to_string()), + ("second".to_string(), "two".to_string()) + ]), + cache.content().await + ); + } + + #[tokio::test] + async fn from_certificate_iterator() { + let chain = vec![ + Certificate { + previous_hash: "first_parent".to_string(), + ..fake_data::certificate("first") + }, + Certificate { + previous_hash: "second_parent".to_string(), + ..fake_data::certificate("second") + }, + ]; + let cache = MemoryCertificateVerifierCache::from_iter(&chain); + + assert_eq!( + HashMap::from_iter([ + ("first".to_string(), "first_parent".to_string()), + ("second".to_string(), "second_parent".to_string()) + ]), + cache.content().await + ); + } + + mod store_validated_certificate { + use super::*; + + #[tokio::test] + async fn store_in_empty_cache() { + let cache = MemoryCertificateVerifierCache::default(); + cache + .store_validated_certificate("hash", "parent") + .await + .unwrap(); + + assert_eq!( + HashMap::from_iter([("hash".to_string(), "parent".to_string()),]), + cache.content().await + ); + } + + #[tokio::test] + async fn store_new_hash_push_new_key_at_end_and_dont_alter_existing_values() { + let cache = MemoryCertificateVerifierCache::from_iter([ + ("existing_hash", "existing_parent"), + ("another_hash", "another_parent"), + ]); + cache + .store_validated_certificate("new_hash", "new_parent") + .await + .unwrap(); + + assert_eq!( + HashMap::from_iter([ + ("existing_hash".to_string(), "existing_parent".to_string()), + ("another_hash".to_string(), "another_parent".to_string()), + ("new_hash".to_string(), "new_parent".to_string()), + ]), + cache.content().await + ); + } + + #[tokio::test] + async fn storing_same_hash_update_parent_hash() { + let cache = MemoryCertificateVerifierCache::from_iter([ + ("hash", "first_parent"), + ("another_hash", "another_parent"), + ]); + cache + .store_validated_certificate("hash", "updated_parent") + .await + .unwrap(); + + assert_eq!( + HashMap::from_iter([ + ("hash".to_string(), "updated_parent".to_string()), + ("another_hash".to_string(), "another_parent".to_string()) + ]), + cache.content().await + ); + } + } + + mod get_previous_hash { + use super::*; + + #[tokio::test] + async fn get_previous_hash_when_key_exists() { + let cache = MemoryCertificateVerifierCache::from_iter([ + ("hash", "parent"), + ("another_hash", "another_parent"), + ]); + + assert_eq!( + Some("parent".to_string()), + cache.get_previous_hash("hash").await.unwrap() + ); + } + + #[tokio::test] + async fn get_previous_hash_return_none_if_not_found() { + let cache = MemoryCertificateVerifierCache::from_iter([ + ("hash", "parent"), + ("another_hash", "another_parent"), + ]); + + assert_eq!(None, cache.get_previous_hash("not_found").await.unwrap()); + } + } + + mod reset { + use super::*; + + #[tokio::test] + async fn reset_empty_cache_dont_raise_error() { + let cache = MemoryCertificateVerifierCache::default(); + + cache.reset().await.unwrap(); + + assert_eq!(HashMap::new(), cache.content().await); + } + + #[tokio::test] + async fn reset_not_empty_cache() { + let cache = MemoryCertificateVerifierCache::from_iter([ + ("hash", "parent"), + ("another_hash", "another_parent"), + ]); + + cache.reset().await.unwrap(); + + assert_eq!(HashMap::new(), cache.content().await); + } + } +} diff --git a/mithril-client/src/certificate_client/verify_cache/mod.rs b/mithril-client/src/certificate_client/verify_cache/mod.rs index 8b137891791..6a4fdfba0c1 100644 --- a/mithril-client/src/certificate_client/verify_cache/mod.rs +++ b/mithril-client/src/certificate_client/verify_cache/mod.rs @@ -1 +1,3 @@ +mod memory_cache; +pub use memory_cache::*; From 2f207f28743a66cf029636be2a83894a6d663d35 Mon Sep 17 00:00:00 2001 From: DJO <790521+Alenar@users.noreply.github.com> Date: Wed, 27 Nov 2024 18:40:12 +0100 Subject: [PATCH 06/20] feat(client-lib): add a feedback event for validated certificate from cache --- examples/client-snapshot/src/main.rs | 10 +++++++++ .../src/utils/feedback_receiver.rs | 10 +++++++++ mithril-client/src/feedback.rs | 21 +++++++++++++++++++ 3 files changed, 41 insertions(+) diff --git a/examples/client-snapshot/src/main.rs b/examples/client-snapshot/src/main.rs index 3b76b6c0730..3b9fc68da7f 100644 --- a/examples/client-snapshot/src/main.rs +++ b/examples/client-snapshot/src/main.rs @@ -171,6 +171,16 @@ impl FeedbackReceiver for IndicatifFeedbackReceiver { progress_bar.inc(1); } } + MithrilEvent::CertificateValidatedFromCache { + certificate_chain_validation_id: _, + certificate_hash, + } => { + let certificate_validation_pb = self.certificate_validation_pb.read().await; + if let Some(progress_bar) = certificate_validation_pb.as_ref() { + progress_bar.set_message(format!("Cached '{certificate_hash}'")); + progress_bar.inc(1); + } + } MithrilEvent::CertificateChainValidated { certificate_chain_validation_id: _, } => { diff --git a/mithril-client-cli/src/utils/feedback_receiver.rs b/mithril-client-cli/src/utils/feedback_receiver.rs index b9f9874cf05..63d1fc59a5a 100644 --- a/mithril-client-cli/src/utils/feedback_receiver.rs +++ b/mithril-client-cli/src/utils/feedback_receiver.rs @@ -92,6 +92,16 @@ impl FeedbackReceiver for IndicatifFeedbackReceiver { progress_bar.inc(1); } } + MithrilEvent::CertificateValidatedFromCache { + certificate_chain_validation_id: _, + certificate_hash, + } => { + let certificate_validation_pb = self.certificate_validation_pb.read().await; + if let Some(progress_bar) = certificate_validation_pb.as_ref() { + progress_bar.set_message(format!("Cached '{certificate_hash}'")); + progress_bar.inc(1); + } + } MithrilEvent::CertificateChainValidated { certificate_chain_validation_id: _, } => { diff --git a/mithril-client/src/feedback.rs b/mithril-client/src/feedback.rs index f81c8cee2a4..ed8452a1697 100644 --- a/mithril-client/src/feedback.rs +++ b/mithril-client/src/feedback.rs @@ -98,6 +98,13 @@ pub enum MithrilEvent { /// The validated certificate hash certificate_hash: String, }, + /// A individual certificate of a chain have been validated. + CertificateValidatedFromCache { + /// Unique identifier used to track this specific certificate chain validation + certificate_chain_validation_id: String, + /// The validated certificate hash + certificate_hash: String, + }, /// The whole certificate chain is valid. CertificateChainValidated { /// Unique identifier used to track this specific certificate chain validation @@ -129,6 +136,10 @@ impl MithrilEvent { certificate_chain_validation_id, .. } => certificate_chain_validation_id, + MithrilEvent::CertificateValidatedFromCache { + certificate_chain_validation_id, + .. + } => certificate_chain_validation_id, MithrilEvent::CertificateChainValidated { certificate_chain_validation_id, } => certificate_chain_validation_id, @@ -226,6 +237,16 @@ impl FeedbackReceiver for SlogFeedbackReceiver { "certificate_chain_validation_id" => certificate_chain_validation_id, ); } + MithrilEvent::CertificateValidatedFromCache { + certificate_hash, + certificate_chain_validation_id, + } => { + info!( + self.logger, "Cached"; + "certificate_hash" => certificate_hash, + "certificate_chain_validation_id" => certificate_chain_validation_id, + ); + } MithrilEvent::CertificateChainValidated { certificate_chain_validation_id, } => { From 54e83b123f5b60572b6175dbcb2d76398c3ee3bb Mon Sep 17 00:00:00 2001 From: DJO <790521+Alenar@users.noreply.github.com> Date: Fri, 29 Nov 2024 17:57:20 +0100 Subject: [PATCH 07/20] test(client-lib): use a builder pattern to simplify certificate client scenario --- .../src/certificate_client/fetch.rs | 93 +++++++++---------- mithril-client/src/certificate_client/mod.rs | 81 +++++++++++++++- .../src/certificate_client/verify.rs | 67 ++++--------- 3 files changed, 142 insertions(+), 99 deletions(-) diff --git a/mithril-client/src/certificate_client/fetch.rs b/mithril-client/src/certificate_client/fetch.rs index 0b9baee2785..8edd8834b1b 100644 --- a/mithril-client/src/certificate_client/fetch.rs +++ b/mithril-client/src/certificate_client/fetch.rs @@ -107,20 +107,10 @@ impl CertificateRetriever for InternalCertificateRetriever { mod tests { use mithril_common::test_utils::fake_data; - use crate::aggregator_client::MockAggregatorHTTPClient; - use crate::certificate_client::MockCertificateVerifier; - use crate::test_utils; + use crate::certificate_client::tests_utils::CertificateClientTestBuilder; use super::*; - fn build_client(aggregator_client: Arc) -> CertificateClient { - CertificateClient::new( - aggregator_client, - Arc::new(MockCertificateVerifier::new()), - test_utils::test_logger(), - ) - } - #[tokio::test] async fn get_certificate_list() { let expected = vec![ @@ -134,11 +124,12 @@ mod tests { }, ]; let message = expected.clone(); - let mut aggregator_client = MockAggregatorHTTPClient::new(); - aggregator_client - .expect_get_content() - .return_once(move |_| Ok(serde_json::to_string(&message).unwrap())); - let certificate_client = build_client(Arc::new(aggregator_client)); + let certificate_client = CertificateClientTestBuilder::default() + .config_aggregator_client_mock(|mock| { + mock.expect_get_content() + .return_once(move |_| Ok(serde_json::to_string(&message).unwrap())); + }) + .build(); let items = certificate_client.list().await.unwrap(); assert_eq!(expected, items); @@ -146,13 +137,13 @@ mod tests { #[tokio::test] async fn get_certificate_empty_list() { - let mut aggregator_client = MockAggregatorHTTPClient::new(); - aggregator_client - .expect_get_content() - .return_once(move |_| { - Ok(serde_json::to_string::>(&vec![]).unwrap()) - }); - let certificate_client = build_client(Arc::new(aggregator_client)); + let certificate_client = CertificateClientTestBuilder::default() + .config_aggregator_client_mock(|mock| { + mock.expect_get_content().return_once(move |_| { + Ok(serde_json::to_string::>(&vec![]).unwrap()) + }); + }) + .build(); let items = certificate_client.list().await.unwrap(); assert!(items.is_empty()); @@ -160,19 +151,21 @@ mod tests { #[tokio::test] async fn test_show_ok_some() { - let mut aggregator_client = MockAggregatorHTTPClient::new(); let certificate_hash = "cert-hash-123".to_string(); let certificate = fake_data::certificate(certificate_hash.clone()); let expected_certificate = certificate.clone(); - aggregator_client - .expect_get_content() - .return_once(move |_| { - let message: CertificateMessage = certificate.try_into().unwrap(); - Ok(serde_json::to_string(&message).unwrap()) + + let certificate_client = CertificateClientTestBuilder::default() + .config_aggregator_client_mock(|mock| { + mock.expect_get_content() + .return_once(move |_| { + let message: CertificateMessage = certificate.try_into().unwrap(); + Ok(serde_json::to_string(&message).unwrap()) + }) + .times(1); }) - .times(1); + .build(); - let certificate_client = build_client(Arc::new(aggregator_client)); let cert = certificate_client .get("cert-hash-123") .await @@ -186,17 +179,18 @@ mod tests { #[tokio::test] async fn test_show_ok_none() { - let mut aggregator_client = MockAggregatorHTTPClient::new(); - aggregator_client - .expect_get_content() - .return_once(move |_| { - Err(AggregatorClientError::RemoteServerLogical(anyhow!( - "an error" - ))) + let certificate_client = CertificateClientTestBuilder::default() + .config_aggregator_client_mock(|mock| { + mock.expect_get_content() + .return_once(move |_| { + Err(AggregatorClientError::RemoteServerLogical(anyhow!( + "an error" + ))) + }) + .times(1); }) - .times(1); + .build(); - let certificate_client = build_client(Arc::new(aggregator_client)); assert!(certificate_client .get("cert-hash-123") .await @@ -206,17 +200,18 @@ mod tests { #[tokio::test] async fn test_show_ko() { - let mut aggregator_client = MockAggregatorHTTPClient::new(); - aggregator_client - .expect_get_content() - .return_once(move |_| { - Err(AggregatorClientError::RemoteServerTechnical(anyhow!( - "an error" - ))) + let certificate_client = CertificateClientTestBuilder::default() + .config_aggregator_client_mock(|mock| { + mock.expect_get_content() + .return_once(move |_| { + Err(AggregatorClientError::RemoteServerTechnical(anyhow!( + "an error" + ))) + }) + .times(1); }) - .times(1); + .build(); - let certificate_client = build_client(Arc::new(aggregator_client)); certificate_client .get("cert-hash-123") .await diff --git a/mithril-client/src/certificate_client/mod.rs b/mithril-client/src/certificate_client/mod.rs index 449b5b001c3..d7f6dbd461f 100644 --- a/mithril-client/src/certificate_client/mod.rs +++ b/mithril-client/src/certificate_client/mod.rs @@ -68,12 +68,88 @@ pub use verify_cache::MemoryCertificateVerifierCache; #[cfg(test)] pub(crate) mod tests_utils { - use mockall::predicate::eq; - + use mithril_common::crypto_helper::ProtocolGenesisVerificationKey; use mithril_common::entities::Certificate; use mithril_common::messages::CertificateMessage; + use mockall::predicate::eq; + use std::sync::Arc; use crate::aggregator_client::{AggregatorRequest, MockAggregatorHTTPClient}; + use crate::feedback::{FeedbackReceiver, FeedbackSender}; + use crate::test_utils; + + use super::*; + + #[derive(Default)] + pub(crate) struct CertificateClientTestBuilder { + aggregator_client: MockAggregatorHTTPClient, + genesis_verification_key: Option, + feedback_receivers: Vec>, + #[cfg(feature = "unstable")] + verifier_cache: Option>, + } + + impl CertificateClientTestBuilder { + pub fn config_aggregator_client_mock( + mut self, + config: impl FnOnce(&mut MockAggregatorHTTPClient), + ) -> Self { + config(&mut self.aggregator_client); + self + } + + pub fn with_genesis_verification_key( + mut self, + genesis_verification_key: ProtocolGenesisVerificationKey, + ) -> Self { + self.genesis_verification_key = Some(genesis_verification_key.try_into().unwrap()); + self + } + + pub fn add_feedback_receiver( + mut self, + feedback_receiver: Arc, + ) -> Self { + self.feedback_receivers.push(feedback_receiver); + self + } + + #[cfg(feature = "unstable")] + pub fn with_verifier_cache( + mut self, + verifier_cache: Arc, + ) -> Self { + self.verifier_cache = Some(verifier_cache); + self + } + + /// Builds a new [CertificateClient] with the given configuration. + /// + /// If no genesis verification key is provided, a [MockCertificateVerifier] will be used, + /// else a [MithrilCertificateVerifier] will be used. + pub fn build(self) -> CertificateClient { + let logger = test_utils::test_logger(); + let aggregator_client = Arc::new(self.aggregator_client); + + let certificate_verifier: Arc = + match self.genesis_verification_key { + None => Arc::new(MockCertificateVerifier::new()), + Some(genesis_verification_key) => Arc::new( + MithrilCertificateVerifier::new( + aggregator_client.clone(), + &genesis_verification_key, + FeedbackSender::new(&self.feedback_receivers), + #[cfg(feature = "unstable")] + self.verifier_cache, + logger.clone(), + ) + .unwrap(), + ), + }; + + CertificateClient::new(aggregator_client.clone(), certificate_verifier, logger) + } + } impl MockAggregatorHTTPClient { pub(crate) fn expect_certificate_chain(&mut self, certificate_chain: Vec) { @@ -85,6 +161,7 @@ pub(crate) mod tests_utils { .unwrap(); self.expect_get_content() .with(eq(AggregatorRequest::GetCertificate { hash })) + .once() .returning(move |_| Ok(message.to_owned())); } } diff --git a/mithril-client/src/certificate_client/verify.rs b/mithril-client/src/certificate_client/verify.rs index 85f422002e6..5655df10d74 100644 --- a/mithril-client/src/certificate_client/verify.rs +++ b/mithril-client/src/certificate_client/verify.rs @@ -132,45 +132,27 @@ impl CertificateVerifier for MithrilCertificateVerifier { #[cfg(test)] mod tests { - use mithril_common::crypto_helper::tests_setup::setup_certificate_chain; + use mithril_common::test_utils::CertificateChainBuilder; - use crate::aggregator_client::MockAggregatorHTTPClient; + use crate::certificate_client::tests_utils::CertificateClientTestBuilder; use crate::feedback::StackFeedbackReceiver; - use crate::test_utils; use super::*; - fn build_client( - aggregator_client: Arc, - verifier: Arc, - ) -> CertificateClient { - CertificateClient::new(aggregator_client, verifier, test_utils::test_logger()) - } - #[tokio::test] async fn validating_chain_send_feedbacks() { - let (chain, verifier) = setup_certificate_chain(3, 1); - let verification_key: String = verifier.to_verification_key().try_into().unwrap(); - let mut aggregator_client = MockAggregatorHTTPClient::new(); - aggregator_client.expect_certificate_chain(chain.clone()); + let (chain, verifier) = CertificateChainBuilder::new() + .with_total_certificates(3) + .with_certificates_per_epoch(1) + .build(); let last_certificate_hash = chain.first().unwrap().hash.clone(); - let aggregator_client = Arc::new(aggregator_client); let feedback_receiver = Arc::new(StackFeedbackReceiver::new()); - let certificate_client = build_client( - aggregator_client.clone(), - Arc::new( - MithrilCertificateVerifier::new( - aggregator_client, - &verification_key, - FeedbackSender::new(&[feedback_receiver.clone()]), - #[cfg(feature = "unstable")] - None, - test_utils::test_logger(), - ) - .unwrap(), - ), - ); + let certificate_client = CertificateClientTestBuilder::default() + .config_aggregator_client_mock(|mock| mock.expect_certificate_chain(chain.clone())) + .with_genesis_verification_key(verifier.to_verification_key()) + .add_feedback_receiver(feedback_receiver.clone()) + .build(); certificate_client .verify_chain(&last_certificate_hash) @@ -203,27 +185,16 @@ mod tests { #[tokio::test] async fn verify_chain_return_certificate_with_given_hash() { - let (chain, verifier) = setup_certificate_chain(3, 1); - let verification_key: String = verifier.to_verification_key().try_into().unwrap(); - let mut aggregator_client = MockAggregatorHTTPClient::new(); - aggregator_client.expect_certificate_chain(chain.clone()); + let (chain, verifier) = CertificateChainBuilder::new() + .with_total_certificates(3) + .with_certificates_per_epoch(1) + .build(); let last_certificate_hash = chain.first().unwrap().hash.clone(); - let aggregator_client = Arc::new(aggregator_client); - let certificate_client = build_client( - aggregator_client.clone(), - Arc::new( - MithrilCertificateVerifier::new( - aggregator_client, - &verification_key, - FeedbackSender::new(&[]), - #[cfg(feature = "unstable")] - None, - test_utils::test_logger(), - ) - .unwrap(), - ), - ); + let certificate_client = CertificateClientTestBuilder::default() + .config_aggregator_client_mock(|mock| mock.expect_certificate_chain(chain.clone())) + .with_genesis_verification_key(verifier.to_verification_key()) + .build(); let certificate = certificate_client .verify_chain(&last_certificate_hash) From 1f14231c22a7eb36ec4f2f2610bf21c768966c2f Mon Sep 17 00:00:00 2001 From: DJO <790521+Alenar@users.noreply.github.com> Date: Wed, 4 Dec 2024 12:01:28 +0100 Subject: [PATCH 08/20] test(client-lib): enable `trace` level logs in debug builds And keep them at max level `warn` in release build (to keep the previous default behavior, see: https://docs.rs/slog/latest/slog/#notable-details). --- mithril-client/Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mithril-client/Cargo.toml b/mithril-client/Cargo.toml index c95fedc7c94..fb3d45c5dc4 100644 --- a/mithril-client/Cargo.toml +++ b/mithril-client/Cargo.toml @@ -41,7 +41,7 @@ reqwest = { version = "0.12.9", default-features = false, features = [ semver = "1.0.23" serde = { version = "1.0.215", features = ["derive"] } serde_json = "1.0.133" -slog = "2.7.0" +slog = { version = "2.7.0", features = ["max_level_trace", "release_max_level_warn"] } strum = { version = "0.26.3", features = ["derive"] } tar = { version = "0.4.43", optional = true } thiserror = "2.0.6" From f1f5720fa99c37dd7d6305bbb21a9081558dadf7 Mon Sep 17 00:00:00 2001 From: DJO <790521+Alenar@users.noreply.github.com> Date: Wed, 4 Dec 2024 18:15:09 +0100 Subject: [PATCH 09/20] feat(client-cli): use cache system when validating certificates --- .../src/certificate_client/verify.rs | 326 ++++++++++++++++-- 1 file changed, 306 insertions(+), 20 deletions(-) diff --git a/mithril-client/src/certificate_client/verify.rs b/mithril-client/src/certificate_client/verify.rs index 5655df10d74..7446f4f84d8 100644 --- a/mithril-client/src/certificate_client/verify.rs +++ b/mithril-client/src/certificate_client/verify.rs @@ -1,14 +1,15 @@ use anyhow::{anyhow, Context}; use async_trait::async_trait; -use slog::Logger; +use slog::{trace, Logger}; use std::sync::Arc; use mithril_common::{ certificate_chain::{ - CertificateVerifier as CommonCertificateVerifier, + CertificateRetriever, CertificateVerifier as CommonCertificateVerifier, MithrilCertificateVerifier as CommonMithrilCertificateVerifier, }, crypto_helper::ProtocolGenesisVerificationKey, + entities::Certificate, logging::LoggerExtensions, }; @@ -47,11 +48,13 @@ pub(super) async fn verify_chain( /// Implementation of a [CertificateVerifier] that can send feedbacks using /// the [feedback][crate::feedback] mechanism. pub struct MithrilCertificateVerifier { + retriever: Arc, internal_verifier: Arc, genesis_verification_key: ProtocolGenesisVerificationKey, feedback_sender: FeedbackSender, #[cfg(feature = "unstable")] - _verifier_cache: Option>, + verifier_cache: Option>, + logger: Logger, } impl MithrilCertificateVerifier { @@ -69,7 +72,7 @@ impl MithrilCertificateVerifier { logger.clone(), )); let internal_verifier = Arc::new(CommonMithrilCertificateVerifier::new( - logger, + logger.clone(), retriever.clone(), )); let genesis_verification_key = @@ -77,13 +80,106 @@ impl MithrilCertificateVerifier { .with_context(|| "Invalid genesis verification key")?; Ok(Self { + retriever, internal_verifier, genesis_verification_key, feedback_sender, #[cfg(feature = "unstable")] - _verifier_cache: verifier_cache, + verifier_cache, + logger, }) } + + #[cfg(feature = "unstable")] + async fn fetch_cached_previous_hash(&self, hash: &str) -> MithrilResult> { + if let Some(cache) = self.verifier_cache.as_ref() { + Ok(cache.get_previous_hash(hash).await?) + } else { + Ok(None) + } + } + + #[cfg(not(feature = "unstable"))] + async fn fetch_cached_previous_hash(&self, _hash: &str) -> MithrilResult> { + Ok(None) + } + + async fn verify_one( + &self, + certificate_chain_validation_id: &str, + certificate: CertificateToVerify, + ) -> MithrilResult> { + trace!(self.logger, "Validating certificate"; "hash" => certificate.hash(), "previous_hash" => certificate.hash()); + if let Some(previous_hash) = self.fetch_cached_previous_hash(certificate.hash()).await? { + trace!(self.logger, "Certificate validated from cache"; "hash" => certificate.hash(), "previous_hash" => &previous_hash); + self.feedback_sender + .send_event(MithrilEvent::CertificateValidatedFromCache { + certificate_hash: certificate.hash().to_owned(), + certificate_chain_validation_id: certificate_chain_validation_id.to_string(), + }) + .await; + + Ok(Some(CertificateToVerify::ToDownload { + hash: previous_hash, + })) + } else { + self.verify_not_cached_certificate(certificate_chain_validation_id, certificate) + .await + } + } + + async fn verify_not_cached_certificate( + &self, + certificate_chain_validation_id: &str, + certificate: CertificateToVerify, + ) -> MithrilResult> { + let certificate = match certificate { + CertificateToVerify::Downloaded { certificate } => certificate, + CertificateToVerify::ToDownload { hash } => { + self.retriever.get_certificate_details(&hash).await? + } + }; + + let previous_certificate = self + .internal_verifier + .verify_certificate(&certificate, &self.genesis_verification_key) + .await?; + + #[cfg(feature = "unstable")] + if let Some(cache) = self.verifier_cache.as_ref() { + if !certificate.is_genesis() { + cache + .store_validated_certificate(&certificate.hash, &certificate.previous_hash) + .await?; + } + } + + trace!(self.logger, "Certificate validated"; "hash" => &certificate.hash, "previous_hash" => &certificate.previous_hash); + self.feedback_sender + .send_event(MithrilEvent::CertificateValidated { + certificate_hash: certificate.hash, + certificate_chain_validation_id: certificate_chain_validation_id.to_string(), + }) + .await; + + Ok(previous_certificate.map(|cert| CertificateToVerify::Downloaded { certificate: cert })) + } +} + +enum CertificateToVerify { + /// The certificate is already downloaded. + Downloaded { certificate: Certificate }, + /// The certificate is not downloaded yet (since its parent was cached). + ToDownload { hash: String }, +} + +impl CertificateToVerify { + fn hash(&self) -> &str { + match self { + CertificateToVerify::Downloaded { certificate } => &certificate.hash, + CertificateToVerify::ToDownload { hash } => hash, + } + } } #[cfg_attr(target_family = "wasm", async_trait(?Send))] @@ -100,23 +196,24 @@ impl CertificateVerifier for MithrilCertificateVerifier { }) .await; - let mut current_certificate = certificate.clone().try_into()?; - loop { - let previous_or_none = self - .internal_verifier - .verify_certificate(¤t_certificate, &self.genesis_verification_key) - .await?; - - self.feedback_sender - .send_event(MithrilEvent::CertificateValidated { - certificate_hash: current_certificate.hash.clone(), - certificate_chain_validation_id: certificate_chain_validation_id.clone(), - }) - .await; + // always validate the given certificate even if it was cached + let mut current_certificate = self + .verify_not_cached_certificate( + &certificate_chain_validation_id, + CertificateToVerify::Downloaded { + certificate: certificate.clone().try_into()?, + }, + ) + .await?; - match previous_or_none { - Some(previous_certificate) => current_certificate = previous_certificate, + loop { + match current_certificate { None => break, + Some(next) => { + current_certificate = self + .verify_one(&certificate_chain_validation_id, next) + .await? + } } } @@ -203,4 +300,193 @@ mod tests { assert_eq!(certificate.hash, last_certificate_hash); } + + #[cfg(feature = "unstable")] + mod cache { + use mockall::predicate::eq; + + use crate::aggregator_client::MockAggregatorHTTPClient; + use crate::certificate_client::verify_cache::MemoryCertificateVerifierCache; + use crate::certificate_client::MockCertificateVerifierCache; + use crate::test_utils; + + use super::*; + + fn build_verifier_with_cache( + aggregator_client_mock_config: impl FnOnce(&mut MockAggregatorHTTPClient), + genesis_verification_key: ProtocolGenesisVerificationKey, + cache: Arc, + ) -> MithrilCertificateVerifier { + let mut aggregator_client = MockAggregatorHTTPClient::new(); + aggregator_client_mock_config(&mut aggregator_client); + let genesis_verification_key: String = genesis_verification_key.try_into().unwrap(); + + MithrilCertificateVerifier::new( + Arc::new(aggregator_client), + &genesis_verification_key, + FeedbackSender::new(&[]), + Some(cache), + test_utils::test_logger(), + ) + .unwrap() + } + + #[tokio::test] + async fn genesis_certificates_verification_result_is_not_cached() { + let (chain, verifier) = CertificateChainBuilder::new() + .with_total_certificates(1) + .with_certificates_per_epoch(1) + .build(); + let genesis_certificate = chain.last().unwrap(); + assert!(genesis_certificate.is_genesis()); + + let cache = Arc::new(MemoryCertificateVerifierCache::default()); + let verifier = build_verifier_with_cache( + |_mock| {}, + verifier.to_verification_key(), + cache.clone(), + ); + + verifier + .verify_one( + "certificate_chain_validation_id", + CertificateToVerify::Downloaded { + certificate: genesis_certificate.clone(), + }, + ) + .await + .unwrap(); + + assert_eq!( + cache + .get_previous_hash(&genesis_certificate.hash) + .await + .unwrap(), + None + ); + } + + #[tokio::test] + async fn non_genesis_certificates_verification_result_is_cached() { + let (chain, verifier) = CertificateChainBuilder::new() + .with_total_certificates(2) + .with_certificates_per_epoch(1) + .build(); + let certificate = chain.first().unwrap(); + let genesis_certificate = chain.last().unwrap(); + assert!(!certificate.is_genesis()); + + let cache = Arc::new(MemoryCertificateVerifierCache::default()); + let verifier = build_verifier_with_cache( + |mock| mock.expect_certificate_chain(vec![genesis_certificate.clone()]), + verifier.to_verification_key(), + cache.clone(), + ); + + verifier + .verify_one( + "certificate_chain_validation_id", + CertificateToVerify::Downloaded { + certificate: certificate.clone(), + }, + ) + .await + .unwrap(); + + assert_eq!( + cache.get_previous_hash(&certificate.hash).await.unwrap(), + Some(certificate.previous_hash.clone()) + ); + } + + #[tokio::test] + async fn verification_of_first_certificate_of_a_chain_should_always_fetch_it_from_network() + { + let (chain, verifier) = CertificateChainBuilder::new() + .with_total_certificates(2) + .with_certificates_per_epoch(1) + .build(); + let first_certificate = chain.first().unwrap(); + + let cache = Arc::new(MemoryCertificateVerifierCache::from_iter(&vec![ + first_certificate.clone(), + ])); + let certificate_client = CertificateClientTestBuilder::default() + .config_aggregator_client_mock(|mock| { + // Expect to first certificate to be fetched from the network + mock.expect_certificate_chain(chain.clone()); + }) + .with_genesis_verification_key(verifier.to_verification_key()) + .with_verifier_cache(cache.clone()) + .build(); + + certificate_client + .verify_chain(&first_certificate.hash) + .await + .unwrap(); + } + + #[tokio::test] + async fn verification_of_first_certificate_of_a_chain_should_not_use_cache() { + let (chain, verifier) = CertificateChainBuilder::new() + .with_total_certificates(2) + .with_certificates_per_epoch(1) + .build(); + let first_certificate = chain.first().unwrap(); + let genesis_certificate = chain.last().unwrap(); + assert!(genesis_certificate.is_genesis()); + + let mut cache = MockCertificateVerifierCache::new(); + cache.expect_get_previous_hash().returning(|_| Ok(None)); + cache + .expect_get_previous_hash() + .with(eq(first_certificate.hash.clone())) + .never(); + cache + .expect_store_validated_certificate() + .returning(|_, _| Ok(())); + + let certificate_client = CertificateClientTestBuilder::default() + .config_aggregator_client_mock(|mock| { + mock.expect_certificate_chain(chain.clone()); + }) + .with_genesis_verification_key(verifier.to_verification_key()) + .with_verifier_cache(Arc::new(cache)) + .build(); + + certificate_client + .verify_chain(&first_certificate.hash) + .await + .unwrap(); + } + + #[tokio::test] + async fn verify_chain_return_certificate_with_cache() { + let (chain, verifier) = CertificateChainBuilder::new() + .with_total_certificates(5) + .with_certificates_per_epoch(1) + .build(); + let last_certificate_hash = chain.first().unwrap().hash.clone(); + + // All certificates are cached except the last and the genesis (we always fetch the both) + let cache = MemoryCertificateVerifierCache::from_iter(&chain[1..4]); + + let certificate_client = CertificateClientTestBuilder::default() + .config_aggregator_client_mock(|mock| { + mock.expect_certificate_chain( + [chain[0..2].to_vec(), vec![chain.last().unwrap().clone()]].concat(), + ) + }) + .with_genesis_verification_key(verifier.to_verification_key()) + .with_verifier_cache(Arc::new(cache)) + .build(); + + let certificate = certificate_client + .verify_chain(&last_certificate_hash) + .await + .unwrap(); + + assert_eq!(certificate.hash, last_certificate_hash); + } + } } From ffa62afaed8f8aa1483021a5c01108ca84f43c8f Mon Sep 17 00:00:00 2001 From: DJO <790521+Alenar@users.noreply.github.com> Date: Wed, 4 Dec 2024 19:02:51 +0100 Subject: [PATCH 10/20] feat(client-lib): add expiration to cache support to `MemoryCertificateVerifierCache` --- .../src/certificate_client/verify.rs | 15 +- .../verify_cache/memory_cache.rs | 234 ++++++++++++------ 2 files changed, 168 insertions(+), 81 deletions(-) diff --git a/mithril-client/src/certificate_client/verify.rs b/mithril-client/src/certificate_client/verify.rs index 7446f4f84d8..97d3abd26a9 100644 --- a/mithril-client/src/certificate_client/verify.rs +++ b/mithril-client/src/certificate_client/verify.rs @@ -303,6 +303,7 @@ mod tests { #[cfg(feature = "unstable")] mod cache { + use chrono::TimeDelta; use mockall::predicate::eq; use crate::aggregator_client::MockAggregatorHTTPClient; @@ -340,7 +341,7 @@ mod tests { let genesis_certificate = chain.last().unwrap(); assert!(genesis_certificate.is_genesis()); - let cache = Arc::new(MemoryCertificateVerifierCache::default()); + let cache = Arc::new(MemoryCertificateVerifierCache::new(TimeDelta::hours(1))); let verifier = build_verifier_with_cache( |_mock| {}, verifier.to_verification_key(), @@ -376,7 +377,7 @@ mod tests { let genesis_certificate = chain.last().unwrap(); assert!(!certificate.is_genesis()); - let cache = Arc::new(MemoryCertificateVerifierCache::default()); + let cache = Arc::new(MemoryCertificateVerifierCache::new(TimeDelta::hours(1))); let verifier = build_verifier_with_cache( |mock| mock.expect_certificate_chain(vec![genesis_certificate.clone()]), verifier.to_verification_key(), @@ -408,9 +409,10 @@ mod tests { .build(); let first_certificate = chain.first().unwrap(); - let cache = Arc::new(MemoryCertificateVerifierCache::from_iter(&vec![ - first_certificate.clone(), - ])); + let cache = Arc::new( + MemoryCertificateVerifierCache::new(TimeDelta::hours(3)) + .with_items_from_chain(&vec![first_certificate.clone()]), + ); let certificate_client = CertificateClientTestBuilder::default() .config_aggregator_client_mock(|mock| { // Expect to first certificate to be fetched from the network @@ -469,7 +471,8 @@ mod tests { let last_certificate_hash = chain.first().unwrap().hash.clone(); // All certificates are cached except the last and the genesis (we always fetch the both) - let cache = MemoryCertificateVerifierCache::from_iter(&chain[1..4]); + let cache = MemoryCertificateVerifierCache::new(TimeDelta::hours(3)) + .with_items_from_chain(&chain[1..4]); let certificate_client = CertificateClientTestBuilder::default() .config_aggregator_client_mock(|mock| { diff --git a/mithril-client/src/certificate_client/verify_cache/memory_cache.rs b/mithril-client/src/certificate_client/verify_cache/memory_cache.rs index 11834095ff0..70c0583fed1 100644 --- a/mithril-client/src/certificate_client/verify_cache/memory_cache.rs +++ b/mithril-client/src/certificate_client/verify_cache/memory_cache.rs @@ -1,9 +1,9 @@ use async_trait::async_trait; +use chrono::{DateTime, TimeDelta, Utc}; use std::collections::HashMap; +use std::ops::Add; use tokio::sync::RwLock; -use mithril_common::entities::Certificate; - use crate::certificate_client::CertificateVerifierCache; use crate::MithrilResult; @@ -11,57 +11,46 @@ pub type CertificateHash = str; pub type PreviousCertificateHash = str; /// A in-memory cache for the certificate verifier. -#[derive(Default)] pub struct MemoryCertificateVerifierCache { - /// Hashmap of certificate hash and their parent hash. - cache: RwLock>, + expiration_delay: TimeDelta, + cache: RwLock>, } -impl MemoryCertificateVerifierCache { - #[cfg(test)] - async fn content(&self) -> HashMap { - self.cache.read().await.clone() - } +#[derive(Debug, PartialEq, Eq, Clone)] +struct CachedCertificate { + previous_hash: String, + expire_at: DateTime, } -impl From<&[(&CertificateHash, &PreviousCertificateHash)]> for MemoryCertificateVerifierCache { - fn from(slice: &[(&CertificateHash, &PreviousCertificateHash)]) -> Self { - MemoryCertificateVerifierCache { - cache: RwLock::new( - slice - .iter() - .map(|(k, v)| (k.to_string(), v.to_string())) - .collect(), - ), +impl CachedCertificate { + fn new>( + previous_hash: TPreviousHash, + expire_at: DateTime, + ) -> Self { + CachedCertificate { + previous_hash: previous_hash.into(), + expire_at, } } } -impl<'a> FromIterator<(&'a CertificateHash, &'a PreviousCertificateHash)> - for MemoryCertificateVerifierCache -{ - fn from_iter>( - iter: T, - ) -> Self { +impl MemoryCertificateVerifierCache { + /// `MemoryCertificateVerifierCache` factory + pub fn new(expiration_delay: TimeDelta) -> Self { MemoryCertificateVerifierCache { - cache: RwLock::new( - iter.into_iter() - .map(|(k, v)| (k.to_string(), v.to_string())) - .collect(), - ), + expiration_delay, + cache: RwLock::new(HashMap::new()), } } -} -impl<'a> FromIterator<&'a Certificate> for MemoryCertificateVerifierCache { - fn from_iter>(iter: T) -> Self { - MemoryCertificateVerifierCache { - cache: RwLock::new( - iter.into_iter() - .map(|cert| (cert.hash.clone(), cert.previous_hash.clone())) - .collect(), - ), - } + /// Get the number of elements in the cache + pub async fn len(&self) -> usize { + self.cache.read().await.len() + } + + /// Return true if the cache is empty + pub async fn is_empty(&self) -> bool { + self.cache.read().await.is_empty() } } @@ -77,7 +66,10 @@ impl CertificateVerifierCache for MemoryCertificateVerifierCache { let mut cache = self.cache.write().await; cache.insert( certificate_hash.to_string(), - previous_certificate_hash.to_string(), + CachedCertificate::new( + previous_certificate_hash, + Utc::now().add(self.expiration_delay), + ), ); Ok(()) } @@ -87,7 +79,10 @@ impl CertificateVerifierCache for MemoryCertificateVerifierCache { certificate_hash: &CertificateHash, ) -> MithrilResult> { let cache = self.cache.read().await; - Ok(cache.get(certificate_hash).cloned()) + Ok(cache + .get(certificate_hash) + .filter(|cached| cached.expire_at >= Utc::now()) + .map(|cached| cached.previous_hash.clone())) } async fn reset(&self) -> MithrilResult<()> { @@ -97,16 +92,86 @@ impl CertificateVerifierCache for MemoryCertificateVerifierCache { } } +#[cfg(test)] +pub(crate) mod test_tools { + use mithril_common::entities::Certificate; + + use super::*; + + impl MemoryCertificateVerifierCache { + /// `Test only` Populate the cache with the given hash and previous hash + pub(crate) fn with_items<'a, T>(mut self, key_values: T) -> Self + where + T: IntoIterator, + { + let expire_at = Utc::now() + self.expiration_delay; + self.cache = RwLock::new( + key_values + .into_iter() + .map(|(k, v)| (k.to_string(), CachedCertificate::new(v, expire_at))) + .collect(), + ); + self + } + + /// `Test only` Populate the cache with the given hash and previous hash from given certificates + pub(crate) fn with_items_from_chain<'a, T>(self, chain: T) -> Self + where + T: IntoIterator, + { + self.with_items( + chain + .into_iter() + .map(|cert| (cert.hash.as_str(), cert.previous_hash.as_str())), + ) + } + + /// `Test only` Return the content of the cache (without the expiration date) + pub(crate) async fn content(&self) -> HashMap { + self.cache + .read() + .await + .iter() + .map(|(hash, cached)| (hash.clone(), cached.previous_hash.clone())) + .collect() + } + + /// `Test only` Overwrite the expiration date of an entry the given certificate hash. + /// + /// panic if the key is not found + pub(crate) async fn overwrite_expiration_date( + &self, + certificate_hash: &CertificateHash, + expire_at: DateTime, + ) { + let mut cache = self.cache.write().await; + cache + .get_mut(certificate_hash) + .expect("Key not found") + .expire_at = expire_at; + } + + /// `Test only` Get the cached value for the given certificate hash + pub(super) async fn get_cached_value( + &self, + certificate_hash: &CertificateHash, + ) -> Option { + self.cache.read().await.get(certificate_hash).cloned() + } + } +} + #[cfg(test)] mod tests { + use mithril_common::entities::Certificate; use mithril_common::test_utils::fake_data; use super::*; #[tokio::test] async fn from_str_iterator() { - let cache = - MemoryCertificateVerifierCache::from_iter([("first", "one"), ("second", "two")]); + let cache = MemoryCertificateVerifierCache::new(TimeDelta::hours(1)) + .with_items([("first", "one"), ("second", "two")]); assert_eq!( HashMap::from_iter([ @@ -129,7 +194,8 @@ mod tests { ..fake_data::certificate("second") }, ]; - let cache = MemoryCertificateVerifierCache::from_iter(&chain); + let cache = + MemoryCertificateVerifierCache::new(TimeDelta::hours(1)).with_items_from_chain(&chain); assert_eq!( HashMap::from_iter([ @@ -144,22 +210,28 @@ mod tests { use super::*; #[tokio::test] - async fn store_in_empty_cache() { - let cache = MemoryCertificateVerifierCache::default(); + async fn store_in_empty_cache_add_new_item_that_expire_after_parametrized_delay() { + let expiration_delay = TimeDelta::hours(1); + let now = Utc::now(); + let cache = MemoryCertificateVerifierCache::new(expiration_delay); cache .store_validated_certificate("hash", "parent") .await .unwrap(); - assert_eq!( - HashMap::from_iter([("hash".to_string(), "parent".to_string()),]), - cache.content().await - ); + let cached = cache + .get_cached_value("hash") + .await + .expect("Cache should have been populated"); + + assert_eq!(1, cache.len().await); + assert_eq!("parent", cached.previous_hash); + assert!(cached.expire_at - now >= expiration_delay); } #[tokio::test] async fn store_new_hash_push_new_key_at_end_and_dont_alter_existing_values() { - let cache = MemoryCertificateVerifierCache::from_iter([ + let cache = MemoryCertificateVerifierCache::new(TimeDelta::hours(1)).with_items([ ("existing_hash", "existing_parent"), ("another_hash", "another_parent"), ]); @@ -179,23 +251,30 @@ mod tests { } #[tokio::test] - async fn storing_same_hash_update_parent_hash() { - let cache = MemoryCertificateVerifierCache::from_iter([ - ("hash", "first_parent"), - ("another_hash", "another_parent"), - ]); + async fn storing_same_hash_update_parent_hash_and_expiration_time() { + let expiration_delay = TimeDelta::days(2); + let now = Utc::now(); + let cache = MemoryCertificateVerifierCache::new(expiration_delay) + .with_items([("hash", "first_parent"), ("another_hash", "another_parent")]); + cache .store_validated_certificate("hash", "updated_parent") .await .unwrap(); + let updated_value = cache + .get_cached_value("hash") + .await + .expect("Cache should have been populated"); + + assert_eq!(2, cache.len().await); assert_eq!( - HashMap::from_iter([ - ("hash".to_string(), "updated_parent".to_string()), - ("another_hash".to_string(), "another_parent".to_string()) - ]), - cache.content().await + Some("another_parent".to_string()), + cache.get_previous_hash("another_hash").await.unwrap(), + "Existing but not updated value should not have been altered" ); + assert_eq!("updated_parent", updated_value.previous_hash); + assert!(updated_value.expire_at - now >= expiration_delay); } } @@ -204,10 +283,8 @@ mod tests { #[tokio::test] async fn get_previous_hash_when_key_exists() { - let cache = MemoryCertificateVerifierCache::from_iter([ - ("hash", "parent"), - ("another_hash", "another_parent"), - ]); + let cache = MemoryCertificateVerifierCache::new(TimeDelta::hours(1)) + .with_items([("hash", "parent"), ("another_hash", "another_parent")]); assert_eq!( Some("parent".to_string()), @@ -217,13 +294,22 @@ mod tests { #[tokio::test] async fn get_previous_hash_return_none_if_not_found() { - let cache = MemoryCertificateVerifierCache::from_iter([ - ("hash", "parent"), - ("another_hash", "another_parent"), - ]); + let cache = MemoryCertificateVerifierCache::new(TimeDelta::hours(1)) + .with_items([("hash", "parent"), ("another_hash", "another_parent")]); assert_eq!(None, cache.get_previous_hash("not_found").await.unwrap()); } + + #[tokio::test] + async fn get_expired_previous_hash_return_none() { + let cache = MemoryCertificateVerifierCache::new(TimeDelta::hours(1)) + .with_items([("hash", "parent")]); + cache + .overwrite_expiration_date("hash", Utc::now() - TimeDelta::days(5)) + .await; + + assert_eq!(None, cache.get_previous_hash("hash").await.unwrap()); + } } mod reset { @@ -231,7 +317,7 @@ mod tests { #[tokio::test] async fn reset_empty_cache_dont_raise_error() { - let cache = MemoryCertificateVerifierCache::default(); + let cache = MemoryCertificateVerifierCache::new(TimeDelta::hours(1)); cache.reset().await.unwrap(); @@ -240,10 +326,8 @@ mod tests { #[tokio::test] async fn reset_not_empty_cache() { - let cache = MemoryCertificateVerifierCache::from_iter([ - ("hash", "parent"), - ("another_hash", "another_parent"), - ]); + let cache = MemoryCertificateVerifierCache::new(TimeDelta::hours(1)) + .with_items([("hash", "parent"), ("another_hash", "another_parent")]); cache.reset().await.unwrap(); From b8f6cf96b29020110de79a4a188a479c81b81970 Mon Sep 17 00:00:00 2001 From: DJO <790521+Alenar@users.noreply.github.com> Date: Mon, 9 Dec 2024 18:52:43 +0100 Subject: [PATCH 11/20] feat(client-wasm): add a local storage base certificate verifier cache --- Cargo.lock | 3 + mithril-client-wasm/Cargo.toml | 5 +- .../src/certificate_verification_cache.rs | 441 ++++++++++++++++++ mithril-client-wasm/src/lib.rs | 6 +- 4 files changed, 451 insertions(+), 4 deletions(-) create mode 100644 mithril-client-wasm/src/certificate_verification_cache.rs diff --git a/Cargo.lock b/Cargo.lock index ee49fef20c0..212013be233 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3729,12 +3729,15 @@ dependencies = [ name = "mithril-client-wasm" version = "0.7.2" dependencies = [ + "anyhow", "async-trait", + "chrono", "futures", "mithril-build-script", "mithril-client", "serde", "serde-wasm-bindgen", + "serde_json", "wasm-bindgen", "wasm-bindgen-futures", "wasm-bindgen-test", diff --git a/mithril-client-wasm/Cargo.toml b/mithril-client-wasm/Cargo.toml index aacb529584c..148d2f909d6 100644 --- a/mithril-client-wasm/Cargo.toml +++ b/mithril-client-wasm/Cargo.toml @@ -13,14 +13,17 @@ categories = ["cryptography"] crate-type = ["cdylib"] [dependencies] +anyhow = "1.0.94" async-trait = "0.1.83" +chrono = { version = "0.4.38", features = ["serde"] } futures = "0.3.31" mithril-client = { path = "../mithril-client", features = ["unstable"] } serde = { version = "1.0.215", features = ["derive"] } serde-wasm-bindgen = "0.6.5" +serde_json = "1.0.132" wasm-bindgen = "0.2.99" wasm-bindgen-futures = "0.4.49" -web-sys = { version = "0.3.76", features = ["BroadcastChannel"] } +web-sys = { version = "0.3.76", features = ["BroadcastChannel", "Storage", "Window"] } [dev-dependencies] wasm-bindgen-test = "0.3.49" diff --git a/mithril-client-wasm/src/certificate_verification_cache.rs b/mithril-client-wasm/src/certificate_verification_cache.rs new file mode 100644 index 00000000000..44bdea6b369 --- /dev/null +++ b/mithril-client-wasm/src/certificate_verification_cache.rs @@ -0,0 +1,441 @@ +use anyhow::{anyhow, Context}; +use async_trait::async_trait; +use chrono::{DateTime, TimeDelta, Utc}; +use std::ops::Add; +use web_sys::{window, Storage}; + +use mithril_client::certificate_client::CertificateVerifierCache; +use mithril_client::MithrilResult; + +pub type CertificateHash = str; +pub type PreviousCertificateHash = str; + +/// Browser local-storage based cache for the certificate verifier. +/// +/// Note : as this cache is based on the browser local storage, it can only be used in a browser +/// (it is not compatible with nodejs or other non-browser environment). +pub struct LocalStorageCertificateVerifierCache { + cache_key_prefix: String, + expiration_delay: TimeDelta, +} + +#[derive(Debug, PartialEq, Eq, Clone, serde::Serialize, serde::Deserialize)] +struct CachedCertificate { + previous_hash: String, + expire_at: DateTime, +} + +impl CachedCertificate { + fn new>( + previous_hash: TPreviousHash, + expire_at: DateTime, + ) -> Self { + CachedCertificate { + previous_hash: previous_hash.into(), + expire_at, + } + } +} + +impl LocalStorageCertificateVerifierCache { + /// `LocalStorageCertificateVerifierCache` factory + pub fn new(cache_key_prefix_seed: &str, expiration_delay: TimeDelta) -> Self { + const CACHE_KEY_BASE_PREFIX: &'static str = "certificate_cache"; + + LocalStorageCertificateVerifierCache { + cache_key_prefix: format!("{CACHE_KEY_BASE_PREFIX}_{cache_key_prefix_seed}_"), + expiration_delay, + } + } + + fn push( + &self, + certificate_hash: &CertificateHash, + previous_certificate_hash: &PreviousCertificateHash, + expire_at: DateTime, + ) -> MithrilResult<()> { + if let Some(storage) = open_local_storage()? { + let key = self.cache_key(certificate_hash); + storage + .set_item( + &key, + &serde_json::to_string(&CachedCertificate::new( + previous_certificate_hash, + expire_at, + )) + .map_err(|err| anyhow!("Error serializing cache: {err:?}"))?, + ) + .map_err(|err| anyhow!("Error storing key `{key}` in local storage: {err:?}"))?; + } + Ok(()) + } + + fn parse_cached_certificate(value: String) -> MithrilResult { + serde_json::from_str(&value) + .map_err(|err| anyhow!("Error deserializing cached certificate: {err:?}")) + } + + fn cache_key(&self, certificate_hash: &CertificateHash) -> String { + format!("{}{}", self.cache_key_prefix, certificate_hash) + } +} + +fn open_local_storage() -> MithrilResult> { + let window = window() + .with_context(|| "No window object")? + .local_storage() + .map_err(|err| anyhow!("Error accessing local storage: {err:?}"))?; + Ok(window) +} + +#[cfg_attr(target_family = "wasm", async_trait(?Send))] +#[cfg_attr(not(target_family = "wasm"), async_trait)] +impl CertificateVerifierCache for LocalStorageCertificateVerifierCache { + async fn store_validated_certificate( + &self, + certificate_hash: &CertificateHash, + previous_certificate_hash: &PreviousCertificateHash, + ) -> MithrilResult<()> { + self.push( + certificate_hash, + previous_certificate_hash, + Utc::now().add(self.expiration_delay), + )?; + Ok(()) + } + + async fn get_previous_hash( + &self, + certificate_hash: &CertificateHash, + ) -> MithrilResult> { + match open_local_storage()? { + None => return Ok(None), + Some(storage) => { + let key = self.cache_key(certificate_hash); + match storage.get_item(&key).map_err(|err| { + anyhow!("Error accessing key `{key}` from local storage: {err:?}") + })? { + Some(value) => { + let cached = Self::parse_cached_certificate(value)?; + if Utc::now() >= cached.expire_at { + Ok(None) + } else { + Ok(Some(cached.previous_hash)) + } + } + None => Ok(None), + } + } + } + } + + async fn reset(&self) -> MithrilResult<()> { + if let Some(storage) = open_local_storage()? { + let len = storage + .length() + .map_err(|err| anyhow!("Error accessing local storage length: {err:?}"))?; + let mut key_to_remove = vec![]; + + for i in 0..len { + match storage.key(i).map_err(|err| { + anyhow!("Error accessing key index `{i}` from local storage: {err:?}") + })? { + Some(key) if key.starts_with(&self.cache_key_prefix) => key_to_remove.push(key), + _ => continue, + } + } + + for key in key_to_remove { + storage.remove_item(&key).map_err(|err| { + anyhow!("Error removing key `{key}` from local storage: {err:?}") + })?; + } + } + + Ok(()) + } +} + +#[cfg(test)] +pub(crate) mod test_tools { + use std::collections::HashMap; + + use super::*; + + /// `Test only` Return the raw content of the local storage + pub(crate) fn local_storage_content() -> HashMap { + let storage = open_local_storage().unwrap().unwrap(); + let len = storage.length().unwrap(); + let mut content = HashMap::new(); + + for i in 0..len { + let key = storage.key(i).unwrap().unwrap(); + let value = storage.get_item(&key).unwrap().unwrap(); + content.insert(key, value); + } + + content + } + + impl LocalStorageCertificateVerifierCache { + /// `Test only` Return the number of items in the cache + pub(crate) fn len(&self) -> usize { + local_storage_content() + .into_iter() + .filter(|(k, _v)| k.starts_with(&self.cache_key_prefix)) + .count() + } + + /// `Test only` Populate the cache with the given hash and previous hash + pub(crate) fn with_items<'a, T>(self, key_values: T) -> Self + where + T: IntoIterator, + { + let expire_at = Utc::now() + self.expiration_delay; + for (k, v) in key_values { + self.push(k, v, expire_at).unwrap(); + } + self + } + + /// `Test only` Return the content of the cache (without the expiration date) + pub(crate) fn content(&self) -> HashMap { + local_storage_content() + .into_iter() + .filter(|(k, _v)| k.starts_with(&self.cache_key_prefix)) + .map(|(k, v)| { + ( + k.trim_start_matches(&self.cache_key_prefix).to_string(), + Self::parse_cached_certificate(v).unwrap().previous_hash, + ) + }) + .collect() + } + + /// `Test only` Overwrite the expiration date of an entry the given certificate hash. + /// + /// panic if the key is not found + pub(crate) fn overwrite_expiration_date( + &self, + certificate_hash: &CertificateHash, + expire_at: DateTime, + ) { + let storage = open_local_storage().unwrap().unwrap(); + let key = self.cache_key(certificate_hash); + let existing_value = Self::parse_cached_certificate( + storage.get_item(&key).unwrap().expect("Key not found"), + ) + .unwrap(); + storage + .set_item( + &key, + &serde_json::to_string(&CachedCertificate::new( + &existing_value.previous_hash, + expire_at, + )) + .unwrap(), + ) + .unwrap(); + } + + /// `Test only` Get the cached value for the given certificate hash + pub(super) fn get_cached_value( + &self, + certificate_hash: &CertificateHash, + ) -> Option { + let storage = open_local_storage().unwrap().unwrap(); + storage + .get_item(&self.cache_key(certificate_hash)) + .unwrap() + .map(Self::parse_cached_certificate) + .transpose() + .unwrap() + } + } +} + +#[cfg(test)] +mod tests { + use std::collections::HashMap; + use wasm_bindgen_test::*; + + use super::{test_tools::*, *}; + + // Note: as those tests are using local storage, they MUST be run in a browser as node doesn't + // have support for local storage. + wasm_bindgen_test_configure!(run_in_browser); + + #[wasm_bindgen_test] + async fn from_str_iterator() { + let cache = + LocalStorageCertificateVerifierCache::new("from_str_iterator", TimeDelta::hours(1)) + .with_items([("first", "one"), ("second", "two")]); + + assert_eq!( + HashMap::from_iter([ + ("first".to_string(), "one".to_string()), + ("second".to_string(), "two".to_string()) + ]), + cache.content() + ); + } + + mod store_validated_certificate { + use super::*; + + #[wasm_bindgen_test] + async fn store_in_empty_cache_add_new_item_that_expire_after_parametrized_delay() { + let expiration_delay = TimeDelta::hours(1); + let now = Utc::now(); + let cache = LocalStorageCertificateVerifierCache::new( + "store_in_empty_cache_add_new_item_that_expire_after_parametrized_delay", + expiration_delay, + ); + cache + .store_validated_certificate("hash", "parent") + .await + .unwrap(); + + let cached = cache + .get_cached_value("hash") + .expect("Cache should have been populated"); + + assert_eq!(1, cache.len()); + assert_eq!("parent", cached.previous_hash); + assert!(cached.expire_at - now >= expiration_delay); + } + + #[wasm_bindgen_test] + async fn store_new_hash_push_new_key_at_end_and_dont_alter_existing_values() { + let cache = LocalStorageCertificateVerifierCache::new( + "store_new_hash_push_new_key_at_end_and_dont_alter_existing_values", + TimeDelta::hours(1), + ) + .with_items([ + ("existing_hash", "existing_parent"), + ("another_hash", "another_parent"), + ]); + cache + .store_validated_certificate("new_hash", "new_parent") + .await + .unwrap(); + + assert_eq!( + HashMap::from_iter([ + ("existing_hash".to_string(), "existing_parent".to_string()), + ("another_hash".to_string(), "another_parent".to_string()), + ("new_hash".to_string(), "new_parent".to_string()), + ]), + cache.content() + ); + } + + #[wasm_bindgen_test] + async fn storing_same_hash_update_parent_hash_and_expiration_time() { + let expiration_delay = TimeDelta::days(2); + let now = Utc::now(); + let cache = LocalStorageCertificateVerifierCache::new( + "storing_same_hash_update_parent_hash_and_expiration_time", + expiration_delay, + ) + .with_items([("hash", "first_parent"), ("another_hash", "another_parent")]); + + cache + .store_validated_certificate("hash", "updated_parent") + .await + .unwrap(); + + let updated_value = cache + .get_cached_value("hash") + .expect("Cache should have been populated"); + + assert_eq!(2, cache.len()); + assert_eq!( + Some("another_parent".to_string()), + cache.get_previous_hash("another_hash").await.unwrap(), + "Existing but not updated value should not have been altered, content: {:#?}, now: {:?}", + local_storage_content(), now, + ); + assert_eq!("updated_parent", updated_value.previous_hash); + assert!(updated_value.expire_at - now >= expiration_delay); + } + } + + mod get_previous_hash { + use super::*; + + #[wasm_bindgen_test] + async fn get_previous_hash_when_key_exists() { + let cache = LocalStorageCertificateVerifierCache::new( + "get_previous_hash_when_key_exists", + TimeDelta::hours(1), + ) + .with_items([("hash", "parent"), ("another_hash", "another_parent")]); + + assert_eq!( + Some("parent".to_string()), + cache.get_previous_hash("hash").await.unwrap() + ); + } + + #[wasm_bindgen_test] + async fn get_previous_hash_return_none_if_not_found() { + let cache = LocalStorageCertificateVerifierCache::new( + "get_previous_hash_return_none_if_not_found", + TimeDelta::hours(1), + ) + .with_items([("hash", "parent"), ("another_hash", "another_parent")]); + + assert_eq!(None, cache.get_previous_hash("not_found").await.unwrap()); + } + + #[wasm_bindgen_test] + async fn get_expired_previous_hash_return_none() { + let cache = LocalStorageCertificateVerifierCache::new( + "get_expired_previous_hash_return_none", + TimeDelta::hours(1), + ) + .with_items([("hash", "parent")]); + cache.overwrite_expiration_date("hash", Utc::now() - TimeDelta::days(5)); + + assert_eq!(None, cache.get_previous_hash("hash").await.unwrap()); + } + } + + mod reset { + use super::*; + + #[wasm_bindgen_test] + async fn reset_empty_cache_dont_raise_error() { + let cache = LocalStorageCertificateVerifierCache::new( + "reset_empty_cache_dont_raise_error", + TimeDelta::hours(1), + ); + + cache.reset().await.unwrap(); + + assert_eq!(HashMap::new(), cache.content()); + } + + #[wasm_bindgen_test] + async fn reset_not_empty_cache() { + let cache = LocalStorageCertificateVerifierCache::new( + "reset_not_empty_cache", + TimeDelta::hours(1), + ) + .with_items([("hash", "parent"), ("another_hash", "another_parent")]); + let storage = open_local_storage().unwrap().unwrap(); + storage + .set_item("key_from_another_component", "another_value") + .unwrap(); + + cache.reset().await.unwrap(); + + assert_eq!(HashMap::new(), cache.content()); + assert_eq!( + Some(&"another_value".to_string()), + local_storage_content().get("key_from_another_component") + ); + } + } +} diff --git a/mithril-client-wasm/src/lib.rs b/mithril-client-wasm/src/lib.rs index ce4b7280f3a..3a29a7b07bd 100644 --- a/mithril-client-wasm/src/lib.rs +++ b/mithril-client-wasm/src/lib.rs @@ -2,11 +2,11 @@ #![cfg(target_family = "wasm")] #![cfg_attr(target_family = "wasm", warn(missing_docs))] +mod certificate_verification_cache; mod client_wasm; - -pub use client_wasm::MithrilClient; - #[cfg(test)] mod test_data; +pub use client_wasm::MithrilClient; + pub(crate) type WasmResult = Result; From eb6c90c704a80c90764dba2bf2d5da8ed0113d07 Mon Sep 17 00:00:00 2001 From: DJO <790521+Alenar@users.noreply.github.com> Date: Tue, 10 Dec 2024 16:41:42 +0100 Subject: [PATCH 12/20] feat(client-wasm): add option that enable certificate verification cache With a warning reminding that this feature is experimental. --- mithril-client-wasm/Cargo.toml | 2 +- mithril-client-wasm/src/client_wasm.rs | 48 ++++++++++++++++++++++++-- mithril-client/src/client.rs | 12 +++++++ 3 files changed, 58 insertions(+), 4 deletions(-) diff --git a/mithril-client-wasm/Cargo.toml b/mithril-client-wasm/Cargo.toml index 148d2f909d6..70345353a4e 100644 --- a/mithril-client-wasm/Cargo.toml +++ b/mithril-client-wasm/Cargo.toml @@ -23,7 +23,7 @@ serde-wasm-bindgen = "0.6.5" serde_json = "1.0.132" wasm-bindgen = "0.2.99" wasm-bindgen-futures = "0.4.49" -web-sys = { version = "0.3.76", features = ["BroadcastChannel", "Storage", "Window"] } +web-sys = { version = "0.3.76", features = ["BroadcastChannel", "console", "Storage", "Window"] } [dev-dependencies] wasm-bindgen-test = "0.3.49" diff --git a/mithril-client-wasm/src/client_wasm.rs b/mithril-client-wasm/src/client_wasm.rs index e0946a492d3..1427c597786 100644 --- a/mithril-client-wasm/src/client_wasm.rs +++ b/mithril-client-wasm/src/client_wasm.rs @@ -1,15 +1,18 @@ use async_trait::async_trait; +use chrono::TimeDelta; use serde::Serialize; use std::sync::Arc; use wasm_bindgen::prelude::*; use mithril_client::{ + certificate_client::CertificateVerifierCache, common::Epoch, feedback::{FeedbackReceiver, MithrilEvent}, CardanoTransactionsProofs, Client, ClientBuilder, ClientOptions, MessageBuilder, MithrilCertificate, }; +use crate::certificate_verification_cache::LocalStorageCertificateVerifierCache; use crate::WasmResult; macro_rules! allow_unstable_dead_code { @@ -89,9 +92,22 @@ impl MithrilClient { .unwrap() }; let unstable = client_options.unstable; - let client = ClientBuilder::aggregator(aggregator_endpoint, genesis_verification_key) - .add_feedback_receiver(feedback_receiver) - .with_options(client_options) + let enable_certificate_chain_verification_cache = + client_options.enable_certificate_chain_verification_cache; + let mut client_builder = + ClientBuilder::aggregator(aggregator_endpoint, genesis_verification_key) + .add_feedback_receiver(feedback_receiver) + .with_options(client_options); + + if unstable && enable_certificate_chain_verification_cache { + if let Some(cache) = + Self::build_certifier_cache(aggregator_endpoint, TimeDelta::weeks(1)) + { + client_builder = client_builder.with_certificate_verifier_cache(cache); + } + } + + let client = client_builder .build() .map_err(|err| format!("{err:?}")) .unwrap(); @@ -99,6 +115,32 @@ impl MithrilClient { MithrilClient { client, unstable } } + fn build_certifier_cache( + aggregator_endpoint: &str, + expiration_delay: TimeDelta, + ) -> Option> { + if web_sys::window().is_none() { + web_sys::console::warn_1( + &"Can't enable certificate chain verification cache: window object is not available\ + (are you running in a browser environment ?)" + .into(), + ); + return None; + } + + web_sys::console::warn_1( + &"Certificate chain verification cache is enabled.\n\ + This feature is experimental and will be heavily modified or removed in the \ + future as its security implications are not fully understood." + .into(), + ); + + Some(Arc::new(LocalStorageCertificateVerifierCache::new( + aggregator_endpoint, + expiration_delay, + ))) + } + /// Call the client to get a snapshot from a digest #[wasm_bindgen] pub async fn get_snapshot(&self, digest: &str) -> WasmResult { diff --git a/mithril-client/src/client.rs b/mithril-client/src/client.rs index 7d39e71ae71..59597bf95f7 100644 --- a/mithril-client/src/client.rs +++ b/mithril-client/src/client.rs @@ -32,6 +32,16 @@ pub struct ClientOptions { #[cfg(target_family = "wasm")] #[cfg_attr(target_family = "wasm", serde(default))] pub unstable: bool, + + /// Whether to enable certificate chain verification caching in the WASM client. + /// + /// `unstable` must be set to `true` for this option to have any effect. + /// + /// IMPORTANT: This feature is experimental and will be heavily modified or removed in the + /// future as its security implications are not fully understood. + #[cfg(target_family = "wasm")] + #[cfg_attr(target_family = "wasm", serde(default))] + pub enable_certificate_chain_verification_cache: bool, } impl ClientOptions { @@ -41,6 +51,8 @@ impl ClientOptions { http_headers, #[cfg(target_family = "wasm")] unstable: false, + #[cfg(target_family = "wasm")] + enable_certificate_chain_verification_cache: false, } } From 622ae0d29ce61b8e07ee5918d7dfb551cb2532d4 Mon Sep 17 00:00:00 2001 From: DJO <790521+Alenar@users.noreply.github.com> Date: Tue, 10 Dec 2024 16:43:47 +0100 Subject: [PATCH 13/20] feat(explorer): enable certificate verifier cache --- .../VerifyCertificate/CertificateVerifier.js | 13 +++++++++++-- .../VerifyCertificate/VerifyCertificateModal.js | 5 ++++- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/mithril-explorer/src/components/VerifyCertificate/CertificateVerifier.js b/mithril-explorer/src/components/VerifyCertificate/CertificateVerifier.js index 2b6db60e148..c75a96353fc 100644 --- a/mithril-explorer/src/components/VerifyCertificate/CertificateVerifier.js +++ b/mithril-explorer/src/components/VerifyCertificate/CertificateVerifier.js @@ -19,6 +19,7 @@ export const certificateValidationSteps = { const certificateChainValidationEvents = { started: "CertificateChainValidationStarted", certificateValidated: "CertificateValidated", + certificateValidatedFromCache: "CertificateValidatedFromCache", done: "CertificateChainValidated", }; @@ -113,7 +114,11 @@ export default function CertificateVerifier({ break; case certificateChainValidationEvents.certificateValidated: position = eventPosition.inTable; - message = { certificateHash: event.payload.certificate_hash }; + message = { certificateHash: event.payload.certificate_hash, cached: false }; + break; + case certificateChainValidationEvents.certificateValidatedFromCache: + position = eventPosition.inTable; + message = { certificateHash: event.payload.certificate_hash, cached: true }; break; case certificateChainValidationEvents.done: message = ( @@ -192,7 +197,11 @@ export default function CertificateVerifier({ /> - + {evt.message.cached ? ( + + ) : ( + + )} ))} diff --git a/mithril-explorer/src/components/VerifyCertificate/VerifyCertificateModal.js b/mithril-explorer/src/components/VerifyCertificate/VerifyCertificateModal.js index ebae7a9079f..3acbfb5c8ab 100644 --- a/mithril-explorer/src/components/VerifyCertificate/VerifyCertificateModal.js +++ b/mithril-explorer/src/components/VerifyCertificate/VerifyCertificateModal.js @@ -30,7 +30,10 @@ export default function VerifyCertificateModal({ show, onClose, certificateHash async function init(aggregator, certificateHash) { const genesisVerificationKey = await fetchGenesisVerificationKey(aggregator); - const client = new MithrilClient(aggregator, genesisVerificationKey); + const client = new MithrilClient(aggregator, genesisVerificationKey, { + unstable: true, + enable_certificate_chain_verification_cache: true, + }); const certificate = await client.get_mithril_certificate(certificateHash); setClient(client); From cf9f7f9e1cd3021b189651b6f7642dd21e3afebc Mon Sep 17 00:00:00 2001 From: DJO <790521+Alenar@users.noreply.github.com> Date: Tue, 10 Dec 2024 18:58:28 +0100 Subject: [PATCH 14/20] feat(explorer): add reset of the certificate verifier --- mithril-client-wasm/src/client_wasm.rs | 41 ++++++++++++++----- .../VerifyCertificate/CertificateVerifier.js | 14 +++++++ .../VerifyCertificateModal.js | 4 +- 3 files changed, 47 insertions(+), 12 deletions(-) diff --git a/mithril-client-wasm/src/client_wasm.rs b/mithril-client-wasm/src/client_wasm.rs index 1427c597786..b5936632ba7 100644 --- a/mithril-client-wasm/src/client_wasm.rs +++ b/mithril-client-wasm/src/client_wasm.rs @@ -69,7 +69,7 @@ impl From for MithrilEventWasm { #[wasm_bindgen(getter_with_clone)] pub struct MithrilClient { client: Client, - + certificate_verifier_cache: Option>, unstable: bool, } @@ -99,20 +99,27 @@ impl MithrilClient { .add_feedback_receiver(feedback_receiver) .with_options(client_options); - if unstable && enable_certificate_chain_verification_cache { - if let Some(cache) = - Self::build_certifier_cache(aggregator_endpoint, TimeDelta::weeks(1)) - { - client_builder = client_builder.with_certificate_verifier_cache(cache); + let certificate_verifier_cache = if unstable && enable_certificate_chain_verification_cache + { + let cache = Self::build_certifier_cache(aggregator_endpoint, TimeDelta::weeks(1)); + if let Some(cache) = &cache { + client_builder = client_builder.with_certificate_verifier_cache(cache.clone()); } - } + cache + } else { + None + }; let client = client_builder .build() .map_err(|err| format!("{err:?}")) .unwrap(); - MithrilClient { client, unstable } + MithrilClient { + client, + certificate_verifier_cache, + unstable, + } } fn build_certifier_cache( @@ -409,6 +416,18 @@ impl MithrilClient { Ok(serde_wasm_bindgen::to_value(&result)?) } + + /// `unstable` Reset the certificate verifier cache if enabled + #[wasm_bindgen] + pub async fn reset_certificate_verifier_cache(&self) -> Result<(), JsValue> { + self.guard_unstable()?; + + if let Some(cache) = self.certificate_verifier_cache.as_ref() { + cache.reset().await.map_err(|err| format!("{err:?}"))?; + } + + Ok(()) + } } allow_unstable_dead_code! { @@ -444,8 +463,7 @@ mod tests { const FAKE_AGGREGATOR_IP: &str = "127.0.0.1"; const FAKE_AGGREGATOR_PORT: &str = "8000"; - fn get_mithril_client(unstable: bool) -> MithrilClient { - let options = ClientOptions::new(None).with_unstable_features(unstable); + fn get_mithril_client(options: ClientOptions) -> MithrilClient { let options_js_value = serde_wasm_bindgen::to_value(&options).unwrap(); MithrilClient::new( &format!( @@ -458,7 +476,8 @@ mod tests { } fn get_mithril_client_stable() -> MithrilClient { - get_mithril_client(false) + let options = ClientOptions::new(None).with_unstable_features(false); + get_mithril_client(options) } wasm_bindgen_test_configure!(run_in_browser); diff --git a/mithril-explorer/src/components/VerifyCertificate/CertificateVerifier.js b/mithril-explorer/src/components/VerifyCertificate/CertificateVerifier.js index c75a96353fc..9e9de3e5f42 100644 --- a/mithril-explorer/src/components/VerifyCertificate/CertificateVerifier.js +++ b/mithril-explorer/src/components/VerifyCertificate/CertificateVerifier.js @@ -52,6 +52,7 @@ export default function CertificateVerifier({ certificate, hideSpinner = false, showCertificateLinks = false, + certificateChainVerificationCacheEnabled = true, onStepChange = (step) => {}, onChainValidationError = (error) => {}, onCertificateClick = (hash) => {}, @@ -138,6 +139,10 @@ export default function CertificateVerifier({ ]); } + async function onCacheResetClick() { + await client.reset_certificate_verifier_cache(); + } + return ( <> {Object.entries(certificate).length > 0 && ( @@ -212,6 +217,15 @@ export default function CertificateVerifier({ .map((evt) => (
{evt.message}
))} + {certificateChainVerificationCacheEnabled && + currentStep === certificateValidationSteps.done && ( + <> + Cache enabled:{" "} + + reset cache + + + )} {validationError !== undefined && ( diff --git a/mithril-explorer/src/components/VerifyCertificate/VerifyCertificateModal.js b/mithril-explorer/src/components/VerifyCertificate/VerifyCertificateModal.js index 3acbfb5c8ab..6310a758c36 100644 --- a/mithril-explorer/src/components/VerifyCertificate/VerifyCertificateModal.js +++ b/mithril-explorer/src/components/VerifyCertificate/VerifyCertificateModal.js @@ -11,6 +11,7 @@ export default function VerifyCertificateModal({ show, onClose, certificateHash const [showLoadingWarning, setShowLoadingWarning] = useState(false); const [client, setClient] = useState(undefined); const [certificate, setCertificate] = useState(undefined); + const enableCertificateChainVerificationCache = true; useEffect(() => { if (show) { @@ -32,7 +33,7 @@ export default function VerifyCertificateModal({ show, onClose, certificateHash const genesisVerificationKey = await fetchGenesisVerificationKey(aggregator); const client = new MithrilClient(aggregator, genesisVerificationKey, { unstable: true, - enable_certificate_chain_verification_cache: true, + enable_certificate_chain_verification_cache: enableCertificateChainVerificationCache, }); const certificate = await client.get_mithril_certificate(certificateHash); @@ -72,6 +73,7 @@ export default function VerifyCertificateModal({ show, onClose, certificateHash setLoading(step === certificateValidationSteps.validationInProgress) } From be52d6507411a864cbb23972d010efe491f44453 Mon Sep 17 00:00:00 2001 From: DJO <790521+Alenar@users.noreply.github.com> Date: Tue, 10 Dec 2024 19:43:16 +0100 Subject: [PATCH 15/20] feat(client-lib+client-wasm): certificate verifier cache duration can now be set at the client level With a default duration of one week --- mithril-client-wasm/src/client_wasm.rs | 17 ++++++++++------- mithril-client/src/client.rs | 23 ++++++++++++++++++++++- 2 files changed, 32 insertions(+), 8 deletions(-) diff --git a/mithril-client-wasm/src/client_wasm.rs b/mithril-client-wasm/src/client_wasm.rs index b5936632ba7..cfc07e8be8d 100644 --- a/mithril-client-wasm/src/client_wasm.rs +++ b/mithril-client-wasm/src/client_wasm.rs @@ -91,17 +91,20 @@ impl MithrilClient { .map_err(|err| format!("Failed to parse options: {err:?}")) .unwrap() }; - let unstable = client_options.unstable; - let enable_certificate_chain_verification_cache = - client_options.enable_certificate_chain_verification_cache; let mut client_builder = ClientBuilder::aggregator(aggregator_endpoint, genesis_verification_key) .add_feedback_receiver(feedback_receiver) - .with_options(client_options); + .with_options(client_options.clone()); - let certificate_verifier_cache = if unstable && enable_certificate_chain_verification_cache + let certificate_verifier_cache = if client_options.unstable + && client_options.enable_certificate_chain_verification_cache { - let cache = Self::build_certifier_cache(aggregator_endpoint, TimeDelta::weeks(1)); + let cache = Self::build_certifier_cache( + aggregator_endpoint, + TimeDelta::seconds( + client_options.certificate_chain_verification_cache_duration_in_seconds as i64, + ), + ); if let Some(cache) = &cache { client_builder = client_builder.with_certificate_verifier_cache(cache.clone()); } @@ -118,7 +121,7 @@ impl MithrilClient { MithrilClient { client, certificate_verifier_cache, - unstable, + unstable: client_options.unstable, } } diff --git a/mithril-client/src/client.rs b/mithril-client/src/client.rs index 59597bf95f7..8ea18b59c40 100644 --- a/mithril-client/src/client.rs +++ b/mithril-client/src/client.rs @@ -22,8 +22,13 @@ use crate::snapshot_client::SnapshotClient; use crate::snapshot_downloader::{HttpSnapshotDownloader, SnapshotDownloader}; use crate::MithrilResult; +#[cfg(target_family = "wasm")] +const fn certificate_chain_verification_cache_duration_in_seconds_default() -> u32 { + 604800 +} + /// Options that can be used to configure the client. -#[derive(Debug, Default, Serialize, Deserialize)] +#[derive(Debug, Clone, Default, Serialize, Deserialize)] pub struct ClientOptions { /// HTTP headers to include in the client requests. pub http_headers: Option>, @@ -42,6 +47,19 @@ pub struct ClientOptions { #[cfg(target_family = "wasm")] #[cfg_attr(target_family = "wasm", serde(default))] pub enable_certificate_chain_verification_cache: bool, + + /// Duration in seconds of certificate chain verification cache in the WASM client. + /// + /// Default to one week (604800 seconds). + /// + /// `enable_certificate_chain_verification_cache` and `unstable` must both be set to `true` + /// for this option to have any effect. + #[cfg(target_family = "wasm")] + #[cfg_attr( + target_family = "wasm", + serde(default = "certificate_chain_verification_cache_duration_in_seconds_default") + )] + pub certificate_chain_verification_cache_duration_in_seconds: u32, } impl ClientOptions { @@ -53,6 +71,9 @@ impl ClientOptions { unstable: false, #[cfg(target_family = "wasm")] enable_certificate_chain_verification_cache: false, + #[cfg(target_family = "wasm")] + certificate_chain_verification_cache_duration_in_seconds: + certificate_chain_verification_cache_duration_in_seconds_default(), } } From a93a40f21ab0011ac9591c1c6d38f4f8db4d868a Mon Sep 17 00:00:00 2001 From: DJO <790521+Alenar@users.noreply.github.com> Date: Wed, 18 Dec 2024 11:07:44 +0100 Subject: [PATCH 16/20] chore: adjustments following reviews - Rename new event from `CertificateValidatedFromCache` to `CertificateFetchedFromCache` - Better naming for the cache default duration in client (to make it explicit that the number is one week) - Make some tests more clear with better naming and more assertion - Change `ClientBuilder::with_certificate_verifier_cache` to take an option, allowing to disable existing cache and simplifying usage in the wasm library. - Simplify local storage acquisition in `LocalStorageCertificateVerifierCache` by raising an error if it's missing instead of asking consumer to handle an option (it should not be used if not available anyway). - Adjusted some comments and logs --- examples/client-snapshot/src/main.rs | 2 +- .../src/utils/feedback_receiver.rs | 2 +- .../src/certificate_verification_cache.rs | 120 +++++++++--------- mithril-client-wasm/src/client_wasm.rs | 24 ++-- .../src/certificate_client/verify.rs | 4 +- .../verify_cache/memory_cache.rs | 16 +-- mithril-client/src/client.rs | 19 ++- mithril-client/src/feedback.rs | 12 +- .../VerifyCertificate/CertificateVerifier.js | 4 +- 9 files changed, 95 insertions(+), 108 deletions(-) diff --git a/examples/client-snapshot/src/main.rs b/examples/client-snapshot/src/main.rs index 3b9fc68da7f..64a94a5a978 100644 --- a/examples/client-snapshot/src/main.rs +++ b/examples/client-snapshot/src/main.rs @@ -171,7 +171,7 @@ impl FeedbackReceiver for IndicatifFeedbackReceiver { progress_bar.inc(1); } } - MithrilEvent::CertificateValidatedFromCache { + MithrilEvent::CertificateFetchedFromCache { certificate_chain_validation_id: _, certificate_hash, } => { diff --git a/mithril-client-cli/src/utils/feedback_receiver.rs b/mithril-client-cli/src/utils/feedback_receiver.rs index 63d1fc59a5a..ef515c19a8c 100644 --- a/mithril-client-cli/src/utils/feedback_receiver.rs +++ b/mithril-client-cli/src/utils/feedback_receiver.rs @@ -92,7 +92,7 @@ impl FeedbackReceiver for IndicatifFeedbackReceiver { progress_bar.inc(1); } } - MithrilEvent::CertificateValidatedFromCache { + MithrilEvent::CertificateFetchedFromCache { certificate_chain_validation_id: _, certificate_hash, } => { diff --git a/mithril-client-wasm/src/certificate_verification_cache.rs b/mithril-client-wasm/src/certificate_verification_cache.rs index 44bdea6b369..bf85e7084d9 100644 --- a/mithril-client-wasm/src/certificate_verification_cache.rs +++ b/mithril-client-wasm/src/certificate_verification_cache.rs @@ -54,19 +54,18 @@ impl LocalStorageCertificateVerifierCache { previous_certificate_hash: &PreviousCertificateHash, expire_at: DateTime, ) -> MithrilResult<()> { - if let Some(storage) = open_local_storage()? { - let key = self.cache_key(certificate_hash); - storage - .set_item( - &key, - &serde_json::to_string(&CachedCertificate::new( - previous_certificate_hash, - expire_at, - )) - .map_err(|err| anyhow!("Error serializing cache: {err:?}"))?, - ) - .map_err(|err| anyhow!("Error storing key `{key}` in local storage: {err:?}"))?; - } + let key = self.cache_key(certificate_hash); + open_local_storage()? + .set_item( + &key, + &serde_json::to_string(&CachedCertificate::new( + previous_certificate_hash, + expire_at, + )) + .map_err(|err| anyhow!("Error serializing cache: {err:?}"))?, + ) + .map_err(|err| anyhow!("Error storing key `{key}` in local storage: {err:?}"))?; + Ok(()) } @@ -80,11 +79,12 @@ impl LocalStorageCertificateVerifierCache { } } -fn open_local_storage() -> MithrilResult> { +fn open_local_storage() -> MithrilResult { let window = window() .with_context(|| "No window object")? .local_storage() - .map_err(|err| anyhow!("Error accessing local storage: {err:?}"))?; + .map_err(|err| anyhow!("Error accessing local storage: {err:?}"))? + .with_context(|| "No local storage object")?; Ok(window) } @@ -108,50 +108,45 @@ impl CertificateVerifierCache for LocalStorageCertificateVerifierCache { &self, certificate_hash: &CertificateHash, ) -> MithrilResult> { - match open_local_storage()? { - None => return Ok(None), - Some(storage) => { - let key = self.cache_key(certificate_hash); - match storage.get_item(&key).map_err(|err| { - anyhow!("Error accessing key `{key}` from local storage: {err:?}") - })? { - Some(value) => { - let cached = Self::parse_cached_certificate(value)?; - if Utc::now() >= cached.expire_at { - Ok(None) - } else { - Ok(Some(cached.previous_hash)) - } - } - None => Ok(None), + let key = self.cache_key(certificate_hash); + match open_local_storage()? + .get_item(&key) + .map_err(|err| anyhow!("Error accessing key `{key}` from local storage: {err:?}"))? + { + Some(value) => { + let cached = Self::parse_cached_certificate(value)?; + if Utc::now() >= cached.expire_at { + Ok(None) + } else { + Ok(Some(cached.previous_hash)) } } + None => Ok(None), } } async fn reset(&self) -> MithrilResult<()> { - if let Some(storage) = open_local_storage()? { - let len = storage - .length() - .map_err(|err| anyhow!("Error accessing local storage length: {err:?}"))?; - let mut key_to_remove = vec![]; - - for i in 0..len { - match storage.key(i).map_err(|err| { - anyhow!("Error accessing key index `{i}` from local storage: {err:?}") - })? { - Some(key) if key.starts_with(&self.cache_key_prefix) => key_to_remove.push(key), - _ => continue, - } - } + let storage = open_local_storage()?; + let len = storage + .length() + .map_err(|err| anyhow!("Error accessing local storage length: {err:?}"))?; + let mut key_to_remove = vec![]; - for key in key_to_remove { - storage.remove_item(&key).map_err(|err| { - anyhow!("Error removing key `{key}` from local storage: {err:?}") - })?; + for i in 0..len { + match storage.key(i).map_err(|err| { + anyhow!("Error accessing key index `{i}` from local storage: {err:?}") + })? { + Some(key) if key.starts_with(&self.cache_key_prefix) => key_to_remove.push(key), + _ => continue, } } + for key in key_to_remove { + storage + .remove_item(&key) + .map_err(|err| anyhow!("Error removing key `{key}` from local storage: {err:?}"))?; + } + Ok(()) } } @@ -164,7 +159,7 @@ pub(crate) mod test_tools { /// `Test only` Return the raw content of the local storage pub(crate) fn local_storage_content() -> HashMap { - let storage = open_local_storage().unwrap().unwrap(); + let storage = open_local_storage().unwrap(); let len = storage.length().unwrap(); let mut content = HashMap::new(); @@ -220,7 +215,7 @@ pub(crate) mod test_tools { certificate_hash: &CertificateHash, expire_at: DateTime, ) { - let storage = open_local_storage().unwrap().unwrap(); + let storage = open_local_storage().unwrap(); let key = self.cache_key(certificate_hash); let existing_value = Self::parse_cached_certificate( storage.get_item(&key).unwrap().expect("Key not found"), @@ -243,7 +238,7 @@ pub(crate) mod test_tools { &self, certificate_hash: &CertificateHash, ) -> Option { - let storage = open_local_storage().unwrap().unwrap(); + let storage = open_local_storage().unwrap(); storage .get_item(&self.cache_key(certificate_hash)) .unwrap() @@ -286,7 +281,7 @@ mod tests { #[wasm_bindgen_test] async fn store_in_empty_cache_add_new_item_that_expire_after_parametrized_delay() { let expiration_delay = TimeDelta::hours(1); - let now = Utc::now(); + let start_time = Utc::now(); let cache = LocalStorageCertificateVerifierCache::new( "store_in_empty_cache_add_new_item_that_expire_after_parametrized_delay", expiration_delay, @@ -302,7 +297,7 @@ mod tests { assert_eq!(1, cache.len()); assert_eq!("parent", cached.previous_hash); - assert!(cached.expire_at - now >= expiration_delay); + assert!(cached.expire_at - start_time >= expiration_delay); } #[wasm_bindgen_test] @@ -333,31 +328,32 @@ mod tests { #[wasm_bindgen_test] async fn storing_same_hash_update_parent_hash_and_expiration_time() { let expiration_delay = TimeDelta::days(2); - let now = Utc::now(); + let start_time = Utc::now(); let cache = LocalStorageCertificateVerifierCache::new( "storing_same_hash_update_parent_hash_and_expiration_time", expiration_delay, ) .with_items([("hash", "first_parent"), ("another_hash", "another_parent")]); + let initial_value = cache.get_cached_value("hash").unwrap(); + cache .store_validated_certificate("hash", "updated_parent") .await .unwrap(); - let updated_value = cache - .get_cached_value("hash") - .expect("Cache should have been populated"); + let updated_value = cache.get_cached_value("hash").unwrap(); assert_eq!(2, cache.len()); assert_eq!( Some("another_parent".to_string()), cache.get_previous_hash("another_hash").await.unwrap(), - "Existing but not updated value should not have been altered, content: {:#?}, now: {:?}", - local_storage_content(), now, + "Existing but not updated value should not have been altered, content: {:#?}, start_time: {:?}", + local_storage_content(), start_time, ); assert_eq!("updated_parent", updated_value.previous_hash); - assert!(updated_value.expire_at - now >= expiration_delay); + assert_ne!(initial_value, updated_value); + assert!(updated_value.expire_at - start_time >= expiration_delay); } } @@ -424,7 +420,7 @@ mod tests { TimeDelta::hours(1), ) .with_items([("hash", "parent"), ("another_hash", "another_parent")]); - let storage = open_local_storage().unwrap().unwrap(); + let storage = open_local_storage().unwrap(); storage .set_item("key_from_another_component", "another_value") .unwrap(); diff --git a/mithril-client-wasm/src/client_wasm.rs b/mithril-client-wasm/src/client_wasm.rs index cfc07e8be8d..a64e256720d 100644 --- a/mithril-client-wasm/src/client_wasm.rs +++ b/mithril-client-wasm/src/client_wasm.rs @@ -91,29 +91,24 @@ impl MithrilClient { .map_err(|err| format!("Failed to parse options: {err:?}")) .unwrap() }; - let mut client_builder = - ClientBuilder::aggregator(aggregator_endpoint, genesis_verification_key) - .add_feedback_receiver(feedback_receiver) - .with_options(client_options.clone()); let certificate_verifier_cache = if client_options.unstable && client_options.enable_certificate_chain_verification_cache { - let cache = Self::build_certifier_cache( + Self::build_certifier_cache( aggregator_endpoint, TimeDelta::seconds( client_options.certificate_chain_verification_cache_duration_in_seconds as i64, ), - ); - if let Some(cache) = &cache { - client_builder = client_builder.with_certificate_verifier_cache(cache.clone()); - } - cache + ) } else { None }; - let client = client_builder + let client = ClientBuilder::aggregator(aggregator_endpoint, genesis_verification_key) + .add_feedback_receiver(feedback_receiver) + .with_options(client_options.clone()) + .with_certificate_verifier_cache(certificate_verifier_cache.clone()) .build() .map_err(|err| format!("{err:?}")) .unwrap(); @@ -132,16 +127,15 @@ impl MithrilClient { if web_sys::window().is_none() { web_sys::console::warn_1( &"Can't enable certificate chain verification cache: window object is not available\ - (are you running in a browser environment ?)" + (are you running in a browser environment?)" .into(), ); return None; } web_sys::console::warn_1( - &"Certificate chain verification cache is enabled.\n\ - This feature is experimental and will be heavily modified or removed in the \ - future as its security implications are not fully understood." + &"Danger: the certificate chain verification cache is enabled.\n\ + This feature is highly experimental and insecure, and it must not be used in production." .into(), ); diff --git a/mithril-client/src/certificate_client/verify.rs b/mithril-client/src/certificate_client/verify.rs index 97d3abd26a9..04fbeb3de8e 100644 --- a/mithril-client/src/certificate_client/verify.rs +++ b/mithril-client/src/certificate_client/verify.rs @@ -111,9 +111,9 @@ impl MithrilCertificateVerifier { ) -> MithrilResult> { trace!(self.logger, "Validating certificate"; "hash" => certificate.hash(), "previous_hash" => certificate.hash()); if let Some(previous_hash) = self.fetch_cached_previous_hash(certificate.hash()).await? { - trace!(self.logger, "Certificate validated from cache"; "hash" => certificate.hash(), "previous_hash" => &previous_hash); + trace!(self.logger, "Certificate fetched from cache"; "hash" => certificate.hash(), "previous_hash" => &previous_hash); self.feedback_sender - .send_event(MithrilEvent::CertificateValidatedFromCache { + .send_event(MithrilEvent::CertificateFetchedFromCache { certificate_hash: certificate.hash().to_owned(), certificate_chain_validation_id: certificate_chain_validation_id.to_string(), }) diff --git a/mithril-client/src/certificate_client/verify_cache/memory_cache.rs b/mithril-client/src/certificate_client/verify_cache/memory_cache.rs index 70c0583fed1..2db890fbdaf 100644 --- a/mithril-client/src/certificate_client/verify_cache/memory_cache.rs +++ b/mithril-client/src/certificate_client/verify_cache/memory_cache.rs @@ -212,7 +212,7 @@ mod tests { #[tokio::test] async fn store_in_empty_cache_add_new_item_that_expire_after_parametrized_delay() { let expiration_delay = TimeDelta::hours(1); - let now = Utc::now(); + let start_time = Utc::now(); let cache = MemoryCertificateVerifierCache::new(expiration_delay); cache .store_validated_certificate("hash", "parent") @@ -226,7 +226,7 @@ mod tests { assert_eq!(1, cache.len().await); assert_eq!("parent", cached.previous_hash); - assert!(cached.expire_at - now >= expiration_delay); + assert!(cached.expire_at - start_time >= expiration_delay); } #[tokio::test] @@ -253,19 +253,18 @@ mod tests { #[tokio::test] async fn storing_same_hash_update_parent_hash_and_expiration_time() { let expiration_delay = TimeDelta::days(2); - let now = Utc::now(); + let start_time = Utc::now(); let cache = MemoryCertificateVerifierCache::new(expiration_delay) .with_items([("hash", "first_parent"), ("another_hash", "another_parent")]); + let initial_value = cache.get_cached_value("hash").await.unwrap(); + cache .store_validated_certificate("hash", "updated_parent") .await .unwrap(); - let updated_value = cache - .get_cached_value("hash") - .await - .expect("Cache should have been populated"); + let updated_value = cache.get_cached_value("hash").await.unwrap(); assert_eq!(2, cache.len().await); assert_eq!( @@ -273,8 +272,9 @@ mod tests { cache.get_previous_hash("another_hash").await.unwrap(), "Existing but not updated value should not have been altered" ); + assert_ne!(initial_value, updated_value); assert_eq!("updated_parent", updated_value.previous_hash); - assert!(updated_value.expire_at - now >= expiration_delay); + assert!(updated_value.expire_at - start_time >= expiration_delay); } } diff --git a/mithril-client/src/client.rs b/mithril-client/src/client.rs index 8ea18b59c40..b093778104a 100644 --- a/mithril-client/src/client.rs +++ b/mithril-client/src/client.rs @@ -23,7 +23,7 @@ use crate::snapshot_downloader::{HttpSnapshotDownloader, SnapshotDownloader}; use crate::MithrilResult; #[cfg(target_family = "wasm")] -const fn certificate_chain_verification_cache_duration_in_seconds_default() -> u32 { +const fn one_week_in_seconds() -> u32 { 604800 } @@ -42,8 +42,7 @@ pub struct ClientOptions { /// /// `unstable` must be set to `true` for this option to have any effect. /// - /// IMPORTANT: This feature is experimental and will be heavily modified or removed in the - /// future as its security implications are not fully understood. + /// DANGER: This feature is highly experimental and insecure, and it must not be used in production #[cfg(target_family = "wasm")] #[cfg_attr(target_family = "wasm", serde(default))] pub enable_certificate_chain_verification_cache: bool, @@ -55,10 +54,7 @@ pub struct ClientOptions { /// `enable_certificate_chain_verification_cache` and `unstable` must both be set to `true` /// for this option to have any effect. #[cfg(target_family = "wasm")] - #[cfg_attr( - target_family = "wasm", - serde(default = "certificate_chain_verification_cache_duration_in_seconds_default") - )] + #[cfg_attr(target_family = "wasm", serde(default = "one_week_in_seconds"))] pub certificate_chain_verification_cache_duration_in_seconds: u32, } @@ -72,8 +68,7 @@ impl ClientOptions { #[cfg(target_family = "wasm")] enable_certificate_chain_verification_cache: false, #[cfg(target_family = "wasm")] - certificate_chain_verification_cache_duration_in_seconds: - certificate_chain_verification_cache_duration_in_seconds_default(), + certificate_chain_verification_cache_duration_in_seconds: one_week_in_seconds(), } } @@ -288,11 +283,13 @@ impl ClientBuilder { cfg_unstable! { /// Set the [CertificateVerifierCache] that will be used to cache certificate validation results. + /// + /// Passing a `None` value will disable the cache if any was previously set. pub fn with_certificate_verifier_cache( mut self, - certificate_verifier_cache: Arc, + certificate_verifier_cache: Option>, ) -> ClientBuilder { - self.certificate_verifier_cache = Some(certificate_verifier_cache); + self.certificate_verifier_cache = certificate_verifier_cache; self } } diff --git a/mithril-client/src/feedback.rs b/mithril-client/src/feedback.rs index ed8452a1697..ab5d4b9f0a2 100644 --- a/mithril-client/src/feedback.rs +++ b/mithril-client/src/feedback.rs @@ -91,18 +91,18 @@ pub enum MithrilEvent { /// Unique identifier used to track this specific certificate chain validation certificate_chain_validation_id: String, }, - /// A individual certificate of a chain have been validated. + /// An individual certificate of a chain have been validated. CertificateValidated { /// Unique identifier used to track this specific certificate chain validation certificate_chain_validation_id: String, /// The validated certificate hash certificate_hash: String, }, - /// A individual certificate of a chain have been validated. - CertificateValidatedFromCache { + /// An individual certificate of a chain have been fetched from the cache. + CertificateFetchedFromCache { /// Unique identifier used to track this specific certificate chain validation certificate_chain_validation_id: String, - /// The validated certificate hash + /// The fetched certificate hash certificate_hash: String, }, /// The whole certificate chain is valid. @@ -136,7 +136,7 @@ impl MithrilEvent { certificate_chain_validation_id, .. } => certificate_chain_validation_id, - MithrilEvent::CertificateValidatedFromCache { + MithrilEvent::CertificateFetchedFromCache { certificate_chain_validation_id, .. } => certificate_chain_validation_id, @@ -237,7 +237,7 @@ impl FeedbackReceiver for SlogFeedbackReceiver { "certificate_chain_validation_id" => certificate_chain_validation_id, ); } - MithrilEvent::CertificateValidatedFromCache { + MithrilEvent::CertificateFetchedFromCache { certificate_hash, certificate_chain_validation_id, } => { diff --git a/mithril-explorer/src/components/VerifyCertificate/CertificateVerifier.js b/mithril-explorer/src/components/VerifyCertificate/CertificateVerifier.js index 9e9de3e5f42..eb05c39c38c 100644 --- a/mithril-explorer/src/components/VerifyCertificate/CertificateVerifier.js +++ b/mithril-explorer/src/components/VerifyCertificate/CertificateVerifier.js @@ -19,7 +19,7 @@ export const certificateValidationSteps = { const certificateChainValidationEvents = { started: "CertificateChainValidationStarted", certificateValidated: "CertificateValidated", - certificateValidatedFromCache: "CertificateValidatedFromCache", + certificateFetchedFromCache: "CertificateFetchedFromCache", done: "CertificateChainValidated", }; @@ -117,7 +117,7 @@ export default function CertificateVerifier({ position = eventPosition.inTable; message = { certificateHash: event.payload.certificate_hash, cached: false }; break; - case certificateChainValidationEvents.certificateValidatedFromCache: + case certificateChainValidationEvents.certificateFetchedFromCache: position = eventPosition.inTable; message = { certificateHash: event.payload.certificate_hash, cached: true }; break; From baa1cb6847750dd67711d2a1935379453c0c40a1 Mon Sep 17 00:00:00 2001 From: DJO <790521+Alenar@users.noreply.github.com> Date: Wed, 18 Dec 2024 15:42:22 +0100 Subject: [PATCH 17/20] test(common): allow to chain certificates to direct predecessor when using certificate chain builder --- .../test_utils/certificate_chain_builder.rs | 132 +++++++++++++++--- mithril-common/src/test_utils/mod.rs | 4 +- 2 files changed, 114 insertions(+), 22 deletions(-) diff --git a/mithril-common/src/test_utils/certificate_chain_builder.rs b/mithril-common/src/test_utils/certificate_chain_builder.rs index 9a5fc7f9952..5650704fc08 100644 --- a/mithril-common/src/test_utils/certificate_chain_builder.rs +++ b/mithril-common/src/test_utils/certificate_chain_builder.rs @@ -82,6 +82,20 @@ impl<'a> CertificateChainBuilderContext<'a> { } } +/// Chaining method to use when building a certificate chain with the [CertificateChainBuilder]. For tests only. +#[derive(Debug, Clone, Copy, PartialEq, Default)] +pub enum CertificateChainingMethod { + /// `default` Chain certificates to the 'master' certificate of the epoch or if it's the 'master' + /// certificate, chain it to the 'master' certificate of the previous epoch. + /// + /// The 'master' certificate of an epoch is the first certificate of the epoch. + #[default] + ToMasterCertificate, + + /// Chain certificates sequentially. + Sequential, +} + /// A builder for creating a certificate chain. For tests only. /// /// # Simple example usage for building a fully valid certificate chain @@ -149,6 +163,7 @@ pub struct CertificateChainBuilder<'a> { total_signers_per_epoch_processor: &'a TotalSignersPerEpochProcessorFunc, genesis_certificate_processor: &'a GenesisCertificateProcessorFunc, standard_certificate_processor: &'a StandardCertificateProcessorFunc, + certificate_chaining_method: CertificateChainingMethod, } impl<'a> CertificateChainBuilder<'a> { @@ -166,6 +181,7 @@ impl<'a> CertificateChainBuilder<'a> { total_signers_per_epoch_processor: &|epoch| min(2 + *epoch as usize, 5), genesis_certificate_processor: &|certificate, _, _| certificate, standard_certificate_processor: &|certificate, _| certificate, + certificate_chaining_method: Default::default(), } } @@ -220,6 +236,16 @@ impl<'a> CertificateChainBuilder<'a> { self } + /// Set the chaining method to use when building the certificate chain. + pub fn with_certificate_chaining_method( + mut self, + certificate_chaining_method: CertificateChainingMethod, + ) -> Self { + self.certificate_chaining_method = certificate_chaining_method; + + self + } + /// Build the certificate chain. pub fn build(self) -> (Vec, ProtocolGenesisVerifier) { let (genesis_signer, genesis_verifier) = CertificateChainBuilder::setup_genesis(); @@ -438,26 +464,31 @@ impl<'a> CertificateChainBuilder<'a> { certificate: &Certificate, certificates_chained: &'b [Certificate], ) -> Option<&'b Certificate> { - let is_certificate_first_of_epoch = certificates_chained - .last() - .map(|c| c.epoch != certificate.epoch) - .unwrap_or(true); - - certificates_chained - .iter() - .rev() - .filter(|c| { - if is_certificate_first_of_epoch { - // The previous certificate of the first certificate of an epoch - // is the first certificate of the previous epoch - c.epoch == certificate.epoch.previous().unwrap() - } else { - // The previous certificate of not the first certificate of an epoch - // is the first certificate of the epoch - c.epoch == certificate.epoch - } - }) - .last() + match self.certificate_chaining_method { + CertificateChainingMethod::ToMasterCertificate => { + let is_certificate_first_of_epoch = certificates_chained + .last() + .map(|c| c.epoch != certificate.epoch) + .unwrap_or(true); + + certificates_chained + .iter() + .rev() + .filter(|c| { + if is_certificate_first_of_epoch { + // The previous certificate of the first certificate of an epoch + // is the first certificate of the previous epoch + c.epoch == certificate.epoch.previous().unwrap() + } else { + // The previous certificate of not the first certificate of an epoch + // is the first certificate of the epoch + c.epoch == certificate.epoch + } + }) + .last() + } + CertificateChainingMethod::Sequential => certificates_chained.last(), + } } // Returns the chained certificates in reverse order @@ -788,7 +819,7 @@ mod test { } #[test] - fn builds_certificate_chain_correctly_chained() { + fn builds_certificate_chain_chained_by_default_to_master_certificates() { fn create_fake_certificate(epoch: Epoch, index_in_epoch: u64) -> Certificate { Certificate { epoch, @@ -845,6 +876,65 @@ mod test { ); } + #[test] + fn builds_certificate_chain_chained_sequentially() { + fn create_fake_certificate(epoch: Epoch, index_in_epoch: u64) -> Certificate { + Certificate { + epoch, + signed_message: format!("certificate-{}-{index_in_epoch}", *epoch), + ..fake_data::certificate("cert-fake".to_string()) + } + } + + let certificates = vec![ + create_fake_certificate(Epoch(1), 1), + create_fake_certificate(Epoch(2), 1), + create_fake_certificate(Epoch(2), 2), + create_fake_certificate(Epoch(3), 1), + create_fake_certificate(Epoch(4), 1), + create_fake_certificate(Epoch(4), 2), + create_fake_certificate(Epoch(4), 3), + ]; + + let mut certificates_chained = CertificateChainBuilder::default() + .with_certificate_chaining_method(CertificateChainingMethod::Sequential) + .compute_chained_certificates(certificates); + certificates_chained.reverse(); + + let certificate_chained_1_1 = &certificates_chained[0]; + let certificate_chained_2_1 = &certificates_chained[1]; + let certificate_chained_2_2 = &certificates_chained[2]; + let certificate_chained_3_1 = &certificates_chained[3]; + let certificate_chained_4_1 = &certificates_chained[4]; + let certificate_chained_4_2 = &certificates_chained[5]; + let certificate_chained_4_3 = &certificates_chained[6]; + assert_eq!("", certificate_chained_1_1.previous_hash); + assert_eq!( + certificate_chained_2_1.previous_hash, + certificate_chained_1_1.hash + ); + assert_eq!( + certificate_chained_2_2.previous_hash, + certificate_chained_2_1.hash + ); + assert_eq!( + certificate_chained_3_1.previous_hash, + certificate_chained_2_2.hash + ); + assert_eq!( + certificate_chained_4_1.previous_hash, + certificate_chained_3_1.hash + ); + assert_eq!( + certificate_chained_4_2.previous_hash, + certificate_chained_4_1.hash + ); + assert_eq!( + certificate_chained_4_3.previous_hash, + certificate_chained_4_2.hash + ); + } + #[test] fn builds_certificate_chain_with_alteration_on_genesis_certificate() { let (certificates, _) = CertificateChainBuilder::new() diff --git a/mithril-common/src/test_utils/mod.rs b/mithril-common/src/test_utils/mod.rs index 2a0de475583..baadb5da5eb 100644 --- a/mithril-common/src/test_utils/mod.rs +++ b/mithril-common/src/test_utils/mod.rs @@ -25,7 +25,9 @@ mod temp_dir; pub mod test_http_server; pub use cardano_transactions_builder::CardanoTransactionsBuilder; -pub use certificate_chain_builder::{CertificateChainBuilder, CertificateChainBuilderContext}; +pub use certificate_chain_builder::{ + CertificateChainBuilder, CertificateChainBuilderContext, CertificateChainingMethod, +}; pub use fixture_builder::{MithrilFixtureBuilder, StakeDistributionGenerationMethod}; pub use mithril_fixture::{MithrilFixture, SignerFixture}; pub use temp_dir::*; From 51b3641071ca867d258d1f39170e66363356a7b6 Mon Sep 17 00:00:00 2001 From: DJO <790521+Alenar@users.noreply.github.com> Date: Wed, 18 Dec 2024 18:44:30 +0100 Subject: [PATCH 18/20] feat(client-lib): enhanced security check when verifying with cache enabled By not using the cache until we cross an epoch boundary to make sure that the avk in certificate to check was included in a certificate from the previous epoch. --- .../src/certificate_client/verify.rs | 135 ++++++++++++------ 1 file changed, 92 insertions(+), 43 deletions(-) diff --git a/mithril-client/src/certificate_client/verify.rs b/mithril-client/src/certificate_client/verify.rs index 04fbeb3de8e..ffaf66652b1 100644 --- a/mithril-client/src/certificate_client/verify.rs +++ b/mithril-client/src/certificate_client/verify.rs @@ -104,7 +104,7 @@ impl MithrilCertificateVerifier { Ok(None) } - async fn verify_one( + async fn verify_with_cache_enabled( &self, certificate_chain_validation_id: &str, certificate: CertificateToVerify, @@ -123,23 +123,25 @@ impl MithrilCertificateVerifier { hash: previous_hash, })) } else { - self.verify_not_cached_certificate(certificate_chain_validation_id, certificate) - .await + let certificate = match certificate { + CertificateToVerify::Downloaded { certificate } => certificate, + CertificateToVerify::ToDownload { hash } => { + self.retriever.get_certificate_details(&hash).await? + } + }; + + let previous_certificate = self + .verify_without_cache(certificate_chain_validation_id, certificate) + .await?; + Ok(previous_certificate.map(Into::into)) } } - async fn verify_not_cached_certificate( + async fn verify_without_cache( &self, certificate_chain_validation_id: &str, - certificate: CertificateToVerify, - ) -> MithrilResult> { - let certificate = match certificate { - CertificateToVerify::Downloaded { certificate } => certificate, - CertificateToVerify::ToDownload { hash } => { - self.retriever.get_certificate_details(&hash).await? - } - }; - + certificate: Certificate, + ) -> MithrilResult> { let previous_certificate = self .internal_verifier .verify_certificate(&certificate, &self.genesis_verification_key) @@ -162,7 +164,7 @@ impl MithrilCertificateVerifier { }) .await; - Ok(previous_certificate.map(|cert| CertificateToVerify::Downloaded { certificate: cert })) + Ok(previous_certificate) } } @@ -182,6 +184,12 @@ impl CertificateToVerify { } } +impl From for CertificateToVerify { + fn from(value: Certificate) -> Self { + Self::Downloaded { certificate: value } + } +} + #[cfg_attr(target_family = "wasm", async_trait(?Send))] #[cfg_attr(not(target_family = "wasm"), async_trait)] impl CertificateVerifier for MithrilCertificateVerifier { @@ -196,22 +204,36 @@ impl CertificateVerifier for MithrilCertificateVerifier { }) .await; - // always validate the given certificate even if it was cached - let mut current_certificate = self - .verify_not_cached_certificate( - &certificate_chain_validation_id, - CertificateToVerify::Downloaded { - certificate: certificate.clone().try_into()?, - }, - ) - .await?; + // Validate certificates without cache until we cross an epoch boundary + // This is necessary to ensure that the AVK chaining is correct + let start_epoch = certificate.epoch; + let mut current_certificate: Option = Some(certificate.clone().try_into()?); + loop { + match current_certificate { + None => break, + Some(next) => { + current_certificate = self + .verify_without_cache(&certificate_chain_validation_id, next) + .await?; + + let has_crossed_epoch_boundary = current_certificate + .as_ref() + .is_some_and(|c| c.epoch != start_epoch); + if has_crossed_epoch_boundary { + break; + } + } + } + } + let mut current_certificate: Option = + current_certificate.map(Into::into); loop { match current_certificate { None => break, Some(next) => { current_certificate = self - .verify_one(&certificate_chain_validation_id, next) + .verify_with_cache_enabled(&certificate_chain_validation_id, next) .await? } } @@ -304,6 +326,7 @@ mod tests { #[cfg(feature = "unstable")] mod cache { use chrono::TimeDelta; + use mithril_common::test_utils::CertificateChainingMethod; use mockall::predicate::eq; use crate::aggregator_client::MockAggregatorHTTPClient; @@ -349,7 +372,7 @@ mod tests { ); verifier - .verify_one( + .verify_with_cache_enabled( "certificate_chain_validation_id", CertificateToVerify::Downloaded { certificate: genesis_certificate.clone(), @@ -385,7 +408,7 @@ mod tests { ); verifier - .verify_one( + .verify_with_cache_enabled( "certificate_chain_validation_id", CertificateToVerify::Downloaded { certificate: certificate.clone(), @@ -429,31 +452,57 @@ mod tests { } #[tokio::test] - async fn verification_of_first_certificate_of_a_chain_should_not_use_cache() { + async fn verification_of_certificates_should_not_use_cache_until_crossing_an_epoch_boundary( + ) { + // Scenario: + // | Certificate | epoch | Parent | Can use cache to | Should be fully | + // | | | | get parent hash | Verified | + // |------------:|------:|---------------:|------------------|-----------------| + // | n°6 | 3 | n°5 | No | Yes | + // | n°5 | 3 | n°4 | No | Yes | + // | n°4 | 2 | n°3 | Yes | Yes | + // | n°3 | 2 | n°2 | Yes | No | + // | n°2 | 2 | n°1 | Yes | No | + // | n°1 | 1 | None (genesis) | Yes | Yes | let (chain, verifier) = CertificateChainBuilder::new() - .with_total_certificates(2) - .with_certificates_per_epoch(1) + .with_total_certificates(6) + .with_certificates_per_epoch(3) + .with_certificate_chaining_method(CertificateChainingMethod::Sequential) .build(); + let first_certificate = chain.first().unwrap(); let genesis_certificate = chain.last().unwrap(); assert!(genesis_certificate.is_genesis()); - let mut cache = MockCertificateVerifierCache::new(); - cache.expect_get_previous_hash().returning(|_| Ok(None)); - cache - .expect_get_previous_hash() - .with(eq(first_certificate.hash.clone())) - .never(); - cache - .expect_store_validated_certificate() - .returning(|_, _| Ok(())); + let certificates_that_must_be_fully_verified = + [chain[..3].to_vec(), vec![genesis_certificate.clone()]].concat(); + let certificates_which_parents_can_be_fetched_from_cache = chain[2..5].to_vec(); + + let cache = { + let mut mock = MockCertificateVerifierCache::new(); + + for certificate in certificates_which_parents_can_be_fetched_from_cache { + let previous_hash = certificate.previous_hash.clone(); + mock.expect_get_previous_hash() + .with(eq(certificate.hash.clone())) + .return_once(|_| Ok(Some(previous_hash))) + .once(); + } + mock.expect_get_previous_hash() + .with(eq(genesis_certificate.hash.clone())) + .returning(|_| Ok(None)); + mock.expect_store_validated_certificate() + .returning(|_, _| Ok(())); + + Arc::new(mock) + }; let certificate_client = CertificateClientTestBuilder::default() .config_aggregator_client_mock(|mock| { - mock.expect_certificate_chain(chain.clone()); + mock.expect_certificate_chain(certificates_that_must_be_fully_verified); }) .with_genesis_verification_key(verifier.to_verification_key()) - .with_verifier_cache(Arc::new(cache)) + .with_verifier_cache(cache) .build(); certificate_client @@ -470,14 +519,14 @@ mod tests { .build(); let last_certificate_hash = chain.first().unwrap().hash.clone(); - // All certificates are cached except the last and the genesis (we always fetch the both) + // All certificates are cached except the last two (to cross an epoch boundary) and the genesis let cache = MemoryCertificateVerifierCache::new(TimeDelta::hours(3)) - .with_items_from_chain(&chain[1..4]); + .with_items_from_chain(&chain[2..4]); let certificate_client = CertificateClientTestBuilder::default() .config_aggregator_client_mock(|mock| { mock.expect_certificate_chain( - [chain[0..2].to_vec(), vec![chain.last().unwrap().clone()]].concat(), + [chain[0..3].to_vec(), vec![chain.last().unwrap().clone()]].concat(), ) }) .with_genesis_verification_key(verifier.to_verification_key()) From 3ff980cc13d789112190106c660dfdbfe2cccd3e Mon Sep 17 00:00:00 2001 From: DJO <790521+Alenar@users.noreply.github.com> Date: Thu, 19 Dec 2024 11:47:09 +0100 Subject: [PATCH 19/20] feat(explorer): only enable certificate verification cache in unstable builds --- .../VerifyCertificate/CertificateVerifier.js | 19 +++++++++---------- .../VerifyCertificateModal.js | 17 +++++++++++------ 2 files changed, 20 insertions(+), 16 deletions(-) diff --git a/mithril-explorer/src/components/VerifyCertificate/CertificateVerifier.js b/mithril-explorer/src/components/VerifyCertificate/CertificateVerifier.js index eb05c39c38c..01d67b714d3 100644 --- a/mithril-explorer/src/components/VerifyCertificate/CertificateVerifier.js +++ b/mithril-explorer/src/components/VerifyCertificate/CertificateVerifier.js @@ -52,7 +52,7 @@ export default function CertificateVerifier({ certificate, hideSpinner = false, showCertificateLinks = false, - certificateChainVerificationCacheEnabled = true, + isCacheEnabled = false, onStepChange = (step) => {}, onChainValidationError = (error) => {}, onCertificateClick = (hash) => {}, @@ -217,15 +217,14 @@ export default function CertificateVerifier({ .map((evt) => (
{evt.message}
))} - {certificateChainVerificationCacheEnabled && - currentStep === certificateValidationSteps.done && ( - <> - Cache enabled:{" "} - - reset cache - - - )} + {isCacheEnabled && currentStep === certificateValidationSteps.done && ( + <> + Cache enabled:{" "} + + reset cache + + + )} {validationError !== undefined && ( diff --git a/mithril-explorer/src/components/VerifyCertificate/VerifyCertificateModal.js b/mithril-explorer/src/components/VerifyCertificate/VerifyCertificateModal.js index 6310a758c36..4b261fc8732 100644 --- a/mithril-explorer/src/components/VerifyCertificate/VerifyCertificateModal.js +++ b/mithril-explorer/src/components/VerifyCertificate/VerifyCertificateModal.js @@ -11,7 +11,7 @@ export default function VerifyCertificateModal({ show, onClose, certificateHash const [showLoadingWarning, setShowLoadingWarning] = useState(false); const [client, setClient] = useState(undefined); const [certificate, setCertificate] = useState(undefined); - const enableCertificateChainVerificationCache = true; + const [isCacheEnabled, setIsCacheEnabled] = useState(false); useEffect(() => { if (show) { @@ -31,14 +31,19 @@ export default function VerifyCertificateModal({ show, onClose, certificateHash async function init(aggregator, certificateHash) { const genesisVerificationKey = await fetchGenesisVerificationKey(aggregator); - const client = new MithrilClient(aggregator, genesisVerificationKey, { - unstable: true, - enable_certificate_chain_verification_cache: enableCertificateChainVerificationCache, - }); + const isCacheEnabled = process.env.UNSTABLE === true; + const client_options = process.env.UNSTABLE + ? { + unstable: true, + enable_certificate_chain_verification_cache: isCacheEnabled, + } + : {}; + const client = new MithrilClient(aggregator, genesisVerificationKey, client_options); const certificate = await client.get_mithril_certificate(certificateHash); setClient(client); setCertificate(certificate); + setIsCacheEnabled(isCacheEnabled); } function handleModalClose() { @@ -73,7 +78,7 @@ export default function VerifyCertificateModal({ show, onClose, certificateHash setLoading(step === certificateValidationSteps.validationInProgress) } From 052de8f30e737fd9d6ba819d29c2b29c4a48ced5 Mon Sep 17 00:00:00 2001 From: DJO <790521+Alenar@users.noreply.github.com> Date: Thu, 19 Dec 2024 16:19:38 +0100 Subject: [PATCH 20/20] chore: upgrade crate versions * mithril-client-cli from `0.10.5` to `0.10.6` * mithril-client-wasm from `0.7.2` to `0.7.3` * mithril-client from `0.10.4` to `0.10.5` * mithril-common from `0.4.97` to `0.4.98` * [example] client-snapshot from `0.1.21` to `0.1.22` * [js] mithril-client-wasm from `0.7.2` to `0.7.3` * [js] mithril-explorer from `0.7.23` to `0.7.24` --- Cargo.lock | 10 +++++----- examples/client-snapshot/Cargo.toml | 2 +- examples/client-wasm-nodejs/package-lock.json | 2 +- examples/client-wasm-web/package-lock.json | 2 +- mithril-client-cli/Cargo.toml | 2 +- mithril-client-wasm/Cargo.toml | 2 +- mithril-client-wasm/ci-test/package-lock.json | 2 +- mithril-client-wasm/package.json | 2 +- mithril-client/Cargo.toml | 2 +- mithril-common/Cargo.toml | 2 +- mithril-explorer/package-lock.json | 11 ++++++----- mithril-explorer/package.json | 2 +- 12 files changed, 21 insertions(+), 20 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 212013be233..622dc7e38ae 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -991,7 +991,7 @@ dependencies = [ [[package]] name = "client-snapshot" -version = "0.1.21" +version = "0.1.22" dependencies = [ "anyhow", "async-trait", @@ -3666,7 +3666,7 @@ dependencies = [ [[package]] name = "mithril-client" -version = "0.10.4" +version = "0.10.5" dependencies = [ "anyhow", "async-recursion", @@ -3698,7 +3698,7 @@ dependencies = [ [[package]] name = "mithril-client-cli" -version = "0.10.5" +version = "0.10.6" dependencies = [ "anyhow", "async-trait", @@ -3727,7 +3727,7 @@ dependencies = [ [[package]] name = "mithril-client-wasm" -version = "0.7.2" +version = "0.7.3" dependencies = [ "anyhow", "async-trait", @@ -3746,7 +3746,7 @@ dependencies = [ [[package]] name = "mithril-common" -version = "0.4.97" +version = "0.4.98" dependencies = [ "anyhow", "async-trait", diff --git a/examples/client-snapshot/Cargo.toml b/examples/client-snapshot/Cargo.toml index 616209b6314..bebd268c3dd 100644 --- a/examples/client-snapshot/Cargo.toml +++ b/examples/client-snapshot/Cargo.toml @@ -1,7 +1,7 @@ [package] name = "client-snapshot" description = "Mithril client snapshot example" -version = "0.1.21" +version = "0.1.22" authors = ["dev@iohk.io", "mithril-dev@iohk.io"] documentation = "https://mithril.network/doc" edition = "2021" diff --git a/examples/client-wasm-nodejs/package-lock.json b/examples/client-wasm-nodejs/package-lock.json index f55ebb68521..bd297d1dd16 100644 --- a/examples/client-wasm-nodejs/package-lock.json +++ b/examples/client-wasm-nodejs/package-lock.json @@ -16,7 +16,7 @@ }, "../../mithril-client-wasm": { "name": "@mithril-dev/mithril-client-wasm", - "version": "0.7.1", + "version": "0.7.3", "license": "Apache-2.0" }, "node_modules/@mithril-dev/mithril-client-wasm": { diff --git a/examples/client-wasm-web/package-lock.json b/examples/client-wasm-web/package-lock.json index fce143d5b0a..1e6b8dbaeb7 100644 --- a/examples/client-wasm-web/package-lock.json +++ b/examples/client-wasm-web/package-lock.json @@ -20,7 +20,7 @@ }, "../../mithril-client-wasm": { "name": "@mithril-dev/mithril-client-wasm", - "version": "0.7.1", + "version": "0.7.3", "license": "Apache-2.0" }, "node_modules/@discoveryjs/json-ext": { diff --git a/mithril-client-cli/Cargo.toml b/mithril-client-cli/Cargo.toml index 7ac0d8853b0..6f6834f3d9c 100644 --- a/mithril-client-cli/Cargo.toml +++ b/mithril-client-cli/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "mithril-client-cli" -version = "0.10.5" +version = "0.10.6" description = "A Mithril Client" authors = { workspace = true } edition = { workspace = true } diff --git a/mithril-client-wasm/Cargo.toml b/mithril-client-wasm/Cargo.toml index 70345353a4e..bda0cee095d 100644 --- a/mithril-client-wasm/Cargo.toml +++ b/mithril-client-wasm/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "mithril-client-wasm" -version = "0.7.2" +version = "0.7.3" description = "Mithril client WASM" authors = { workspace = true } edition = { workspace = true } diff --git a/mithril-client-wasm/ci-test/package-lock.json b/mithril-client-wasm/ci-test/package-lock.json index e1ecc779f3b..88f16107717 100644 --- a/mithril-client-wasm/ci-test/package-lock.json +++ b/mithril-client-wasm/ci-test/package-lock.json @@ -21,7 +21,7 @@ }, "..": { "name": "@mithril-dev/mithril-client-wasm", - "version": "0.7.2", + "version": "0.7.3", "license": "Apache-2.0" }, "node_modules/@discoveryjs/json-ext": { diff --git a/mithril-client-wasm/package.json b/mithril-client-wasm/package.json index c318ee8a74b..b9600e8105f 100644 --- a/mithril-client-wasm/package.json +++ b/mithril-client-wasm/package.json @@ -1,6 +1,6 @@ { "name": "@mithril-dev/mithril-client-wasm", - "version": "0.7.2", + "version": "0.7.3", "description": "Mithril client WASM", "license": "Apache-2.0", "collaborators": [ diff --git a/mithril-client/Cargo.toml b/mithril-client/Cargo.toml index fb3d45c5dc4..080296e9e33 100644 --- a/mithril-client/Cargo.toml +++ b/mithril-client/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "mithril-client" -version = "0.10.4" +version = "0.10.5" description = "Mithril client library" authors = { workspace = true } edition = { workspace = true } diff --git a/mithril-common/Cargo.toml b/mithril-common/Cargo.toml index 460cfc49609..613e403a91a 100644 --- a/mithril-common/Cargo.toml +++ b/mithril-common/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "mithril-common" -version = "0.4.97" +version = "0.4.98" description = "Common types, interfaces, and utilities for Mithril nodes." authors = { workspace = true } edition = { workspace = true } diff --git a/mithril-explorer/package-lock.json b/mithril-explorer/package-lock.json index 96036bbefdc..05aafec36d6 100644 --- a/mithril-explorer/package-lock.json +++ b/mithril-explorer/package-lock.json @@ -1,12 +1,12 @@ { "name": "mithril-explorer", - "version": "0.7.23", + "version": "0.7.24", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "mithril-explorer", - "version": "0.7.23", + "version": "0.7.24", "dependencies": { "@mithril-dev/mithril-client-wasm": "file:../mithril-client-wasm/dist/web", "@popperjs/core": "^2.11.8", @@ -36,7 +36,7 @@ }, "../mithril-client-wasm/dist/web": { "name": "mithril-client-wasm", - "version": "0.7.2", + "version": "0.7.3", "license": "Apache-2.0" }, "node_modules/@aashutoshrathi/word-wrap": { @@ -7693,14 +7693,15 @@ "license": "MIT" }, "node_modules/nanoid": { - "version": "3.3.7", + "version": "3.3.8", + "resolved": "https://registry.npmjs.org/nanoid/-/nanoid-3.3.8.tgz", + "integrity": "sha512-WNLf5Sd8oZxOm+TzppcYk8gVOgP+l58xNy58D0nbUnOxOWRWvlcCV4kUF7ltmI6PsrLl/BgKEyS4mqsGChFN0w==", "funding": [ { "type": "github", "url": "https://github.com/sponsors/ai" } ], - "license": "MIT", "bin": { "nanoid": "bin/nanoid.cjs" }, diff --git a/mithril-explorer/package.json b/mithril-explorer/package.json index b82cfd213db..6f85cf84274 100644 --- a/mithril-explorer/package.json +++ b/mithril-explorer/package.json @@ -1,6 +1,6 @@ { "name": "mithril-explorer", - "version": "0.7.23", + "version": "0.7.24", "private": true, "scripts": { "dev": "next dev",