From 473094dd350754507621d67bebf3f083fe5999ee Mon Sep 17 00:00:00 2001 From: Franz-Stefan Preiss Date: Tue, 20 Feb 2024 20:50:49 +0000 Subject: [PATCH] feat(crypto): CRP-2382 construct vault directly in the crypto component --- .../src/builder/mod.rs | 60 +++++--------- .../crypto_service_provider/src/lib.rs | 79 ++---------------- .../crypto_service_provider/src/vault/mod.rs | 83 +++++++++++++++++++ .../src/common/test_utils/crypto_component.rs | 12 +-- rs/crypto/src/keygen/tests.rs | 19 ++--- rs/crypto/src/lib.rs | 24 ++++-- rs/crypto/temp_crypto/src/lib.rs | 3 +- rs/crypto/tests/canister_threshold_sigs.rs | 4 +- 8 files changed, 151 insertions(+), 133 deletions(-) diff --git a/rs/crypto/internal/crypto_service_provider/src/builder/mod.rs b/rs/crypto/internal/crypto_service_provider/src/builder/mod.rs index 36879d7f97a..03feb92d80c 100644 --- a/rs/crypto/internal/crypto_service_provider/src/builder/mod.rs +++ b/rs/crypto/internal/crypto_service_provider/src/builder/mod.rs @@ -1,19 +1,15 @@ use super::*; -pub struct CspBuilder { - vault: Box Arc>, +pub struct CspBuilder { + vault: Box Arc>, logger: ReplicaLogger, metrics: Arc, } -impl CspBuilder { - pub fn with_vault(self, vault: I) -> CspBuilder - where - I: VaultIntoArc + 'static, - W: CspVault + 'static, - { +impl CspBuilder { + pub fn with_vault(self, vault: I) -> CspBuilder { CspBuilder { - vault: Box::new(|| vault.into_arc()), + vault: Box::new(|| vault.into()), logger: self.logger, metrics: self.metrics, } @@ -29,61 +25,47 @@ impl CspBuilder { } impl Csp { - pub fn builder( + pub fn builder( vault: I, logger: ReplicaLogger, metrics: Arc, - ) -> CspBuilder - where - I: VaultIntoArc + 'static, - V: CspVault + 'static, - { + ) -> CspBuilder { CspBuilder { - vault: Box::new(|| vault.into_arc()), + vault: Box::new(|| vault.into()), logger, metrics, } } } -pub trait VaultIntoArc { - type Item; - - fn into_arc(self) -> Arc; +pub trait IntoVaultArc { + fn into(self) -> Arc; } -impl VaultIntoArc for Arc { - type Item = V; - - fn into_arc(self) -> Arc { +impl IntoVaultArc for Arc { + fn into(self) -> Arc { self } } -impl VaultIntoArc for V { - type Item = V; - - fn into_arc(self) -> Arc { +impl IntoVaultArc for V { + fn into(self) -> Arc { Arc::new(self) } } +impl IntoVaultArc for Arc { + fn into(self) -> Arc { + self + } +} + #[cfg(test)] mod test_utils { use super::*; - use crate::public_key_store::temp_pubkey_store::TempPublicKeyStore; - use crate::secret_key_store::temp_secret_key_store::TempSecretKeyStore; - use ic_crypto_test_utils_reproducible_rng::ReproducibleRng; impl Csp { - pub fn builder_for_test() -> CspBuilder< - LocalCspVault< - ReproducibleRng, - TempSecretKeyStore, - TempSecretKeyStore, - TempPublicKeyStore, - >, - > { + pub fn builder_for_test() -> CspBuilder { CspBuilder { vault: Box::new(|| LocalCspVault::builder_for_test().build_into_arc()), logger: no_op_logger(), diff --git a/rs/crypto/internal/crypto_service_provider/src/lib.rs b/rs/crypto/internal/crypto_service_provider/src/lib.rs index 422d4abe40a..3b4a715014f 100644 --- a/rs/crypto/internal/crypto_service_provider/src/lib.rs +++ b/rs/crypto/internal/crypto_service_provider/src/lib.rs @@ -22,7 +22,7 @@ pub mod vault; pub use crate::vault::api::TlsHandshakeCspVault; pub use crate::vault::local_csp_vault::LocalCspVault; pub use crate::vault::remote_csp_vault::run_csp_vault_server; -use crate::vault::remote_csp_vault::RemoteCspVault; +pub use crate::vault::remote_csp_vault::RemoteCspVault; use crate::api::{ CspIDkgProtocol, CspKeyGenerator, CspPublicAndSecretKeyStoreChecker, CspPublicKeyStore, @@ -34,15 +34,13 @@ use crate::types::{CspPublicKey, ExternalPublicKeys}; use crate::vault::api::{ CspPublicKeyStoreError, CspVault, PksAndSksContainsErrors, ValidatePksAndSksError, }; -use ic_adapter_metrics::AdapterMetrics; -use ic_config::crypto::{CryptoConfig, CspVaultType}; +use ic_config::crypto::CryptoConfig; use ic_crypto_internal_logmon::metrics::CryptoMetrics; use ic_crypto_node_key_validation::ValidNodePublicKeys; -use ic_logger::{info, new_logger, replica_logger::no_op_logger, ReplicaLogger}; +use ic_logger::{new_logger, replica_logger::no_op_logger, ReplicaLogger}; use ic_types::crypto::CurrentNodePublicKeys; use key_id::KeyId; use parking_lot::{RwLock, RwLockReadGuard, RwLockWriteGuard}; -use std::path::{Path, PathBuf}; use std::sync::Arc; use std::time::Instant; @@ -163,74 +161,15 @@ impl Csp { tokio_runtime_handle: Option, logger: Option, metrics: Arc, - ) -> Self { - match &config.csp_vault_type { - CspVaultType::InReplica => Self::new_with_in_replica_vault(config, logger, metrics), - CspVaultType::UnixSocket { - logic: logic_socket_path, - metrics: metrics_socket_path, - } => Self::new_with_unix_socket_vault( - logic_socket_path, - metrics_socket_path.clone(), - tokio_runtime_handle.expect("missing tokio runtime handle"), - config, - logger, - metrics, - ), - } - } - - fn new_with_in_replica_vault( - config: &CryptoConfig, - logger: Option, - metrics: Arc, ) -> Self { let logger = logger.unwrap_or_else(no_op_logger); - info!( - logger, - "Proceeding with an in-replica csp_vault, CryptoConfig: {:?}", config - ); - let csp_vault = - LocalCspVault::new_in_dir(&config.crypto_root, metrics.clone(), new_logger!(&logger)); - Csp::builder(csp_vault, logger, metrics).build() - } - - fn new_with_unix_socket_vault( - socket_path: &Path, - metrics_socket_path: Option, - rt_handle: tokio::runtime::Handle, - config: &CryptoConfig, - logger: Option, - metrics: Arc, - ) -> Self { - let logger = logger.unwrap_or_else(no_op_logger); - info!( - logger, - "Proceeding with a remote csp_vault, CryptoConfig: {:?}", config - ); - if let (Some(metrics_uds_path), Some(global_metrics)) = - (metrics_socket_path, metrics.metrics_registry()) - { - global_metrics.register_adapter(AdapterMetrics::new( - "cryptocsp", - metrics_uds_path, - rt_handle.clone(), - )); - } - - let csp_vault = RemoteCspVault::new( - socket_path, - rt_handle, + let vault = vault::vault_from_config( + config, + tokio_runtime_handle, new_logger!(&logger), - metrics.clone(), - ) - .unwrap_or_else(|e| { - panic!( - "Could not connect to CspVault at socket {:?}: {:?}", - socket_path, e - ) - }); - Csp::builder(csp_vault, logger, metrics).build() + Arc::clone(&metrics), + ); + Csp::builder(vault, logger, metrics).build() } } diff --git a/rs/crypto/internal/crypto_service_provider/src/vault/mod.rs b/rs/crypto/internal/crypto_service_provider/src/vault/mod.rs index b00840ca86e..fe656a176f6 100644 --- a/rs/crypto/internal/crypto_service_provider/src/vault/mod.rs +++ b/rs/crypto/internal/crypto_service_provider/src/vault/mod.rs @@ -1,9 +1,18 @@ +use self::api::CspVault; +use self::local_csp_vault::ProdLocalCspVault; +use self::remote_csp_vault::RemoteCspVault; use crate::key_id::KeyIdInstantiationError; use crate::vault::api::{ CspBasicSignatureError, CspBasicSignatureKeygenError, CspMultiSignatureError, CspMultiSignatureKeygenError, CspSecretKeyStoreContainsError, }; +use ic_adapter_metrics::AdapterMetrics; +use ic_config::crypto::{CryptoConfig, CspVaultType}; +use ic_crypto_internal_logmon::metrics::CryptoMetrics; +use ic_logger::{info, ReplicaLogger}; use ic_types::crypto::CryptoError; +use std::path::{Path, PathBuf}; +use std::sync::Arc; pub mod api; pub mod local_csp_vault; @@ -11,6 +20,80 @@ pub mod remote_csp_vault; #[cfg(test)] pub mod test_utils; +/// Creates a production-grade crypto vault. +/// +/// If the `config`'s vault type is `UnixSocket`, a `tokio_runtime_handle` +/// is provided, which is then used for the `async`hronous communication +/// with the vault via RPC. +/// +/// # Panics +/// Panics if the `config`'s vault type is `UnixSocket` and +/// `tokio_runtime_handle` is `None`. +pub fn vault_from_config( + config: &CryptoConfig, + tokio_runtime_handle: Option, + logger: ReplicaLogger, + metrics: Arc, +) -> Arc { + match &config.csp_vault_type { + CspVaultType::InReplica => in_replica_vault(config, logger, metrics), + CspVaultType::UnixSocket { + logic: logic_socket_path, + metrics: metrics_socket_path, + } => unix_socket_vault( + logic_socket_path, + metrics_socket_path.as_ref(), + tokio_runtime_handle.expect("missing tokio runtime handle"), + config, + logger, + metrics, + ), + } +} + +fn in_replica_vault( + config: &CryptoConfig, + logger: ReplicaLogger, + metrics: Arc, +) -> Arc { + info!( + logger, + "Proceeding with an in-replica csp_vault, CryptoConfig: {:?}", config + ); + let vault = ProdLocalCspVault::new_in_dir(&config.crypto_root, metrics, logger); + Arc::new(vault) +} + +fn unix_socket_vault( + socket_path: &Path, + metrics_socket_path: Option<&PathBuf>, + rt_handle: tokio::runtime::Handle, + config: &CryptoConfig, + logger: ReplicaLogger, + metrics: Arc, +) -> Arc { + info!( + logger, + "Proceeding with a remote csp_vault, CryptoConfig: {:?}", config + ); + if let (Some(metrics_uds_path), Some(global_metrics)) = + (metrics_socket_path, metrics.metrics_registry()) + { + global_metrics.register_adapter(AdapterMetrics::new( + "cryptocsp", + metrics_uds_path.clone(), + rt_handle.clone(), + )); + } + let vault = RemoteCspVault::new(socket_path, rt_handle, logger, metrics).unwrap_or_else(|e| { + panic!( + "Could not connect to CspVault at socket {:?}: {:?}", + socket_path, e + ) + }); + Arc::new(vault) +} + impl From for CryptoError { fn from(e: CspBasicSignatureError) -> CryptoError { match e { diff --git a/rs/crypto/src/common/test_utils/crypto_component.rs b/rs/crypto/src/common/test_utils/crypto_component.rs index 02ae9a337dd..83baa79925e 100644 --- a/rs/crypto/src/common/test_utils/crypto_component.rs +++ b/rs/crypto/src/common/test_utils/crypto_component.rs @@ -1,6 +1,7 @@ use crate::CryptoComponentImpl; -use ic_crypto_internal_csp::CryptoServiceProvider; use ic_crypto_internal_logmon::metrics::CryptoMetrics; +use ic_crypto_test_utils_csp::MockAllCryptoServiceProvider; +use ic_crypto_test_utils_local_csp_vault::MockLocalCspVault; use ic_interfaces_registry::RegistryClient; use ic_logger::replica_logger::no_op_logger; use ic_types_test_utils::ids::node_test_id; @@ -8,12 +9,13 @@ use std::sync::Arc; const NODE_ID: u64 = 42; -pub fn crypto_component_with_csp( - csp: C, +pub fn crypto_component_with_csp( + csp: MockAllCryptoServiceProvider, registry_client: Arc, -) -> CryptoComponentImpl { - CryptoComponentImpl::new_with_csp_and_fake_node_id( +) -> CryptoComponentImpl { + CryptoComponentImpl::new_for_test( csp, + Arc::new(MockLocalCspVault::new()), no_op_logger(), registry_client, node_test_id(NODE_ID), diff --git a/rs/crypto/src/keygen/tests.rs b/rs/crypto/src/keygen/tests.rs index 133a282ae88..014ffaee20a 100644 --- a/rs/crypto/src/keygen/tests.rs +++ b/rs/crypto/src/keygen/tests.rs @@ -1,6 +1,7 @@ #![allow(clippy::unwrap_used)] use super::*; +use crate::common::test_utils::crypto_component::crypto_component_with_csp; use assert_matches::assert_matches; use ic_base_types::SubnetId; use ic_base_types::{NodeId, PrincipalId}; @@ -20,6 +21,7 @@ use ic_crypto_test_utils_keys::public_keys::{ valid_idkg_dealing_encryption_public_key, valid_node_signing_public_key, valid_tls_certificate_and_validation_time, }; +use ic_crypto_test_utils_local_csp_vault::MockLocalCspVault; use ic_crypto_test_utils_metrics::assertions::MetricsObservationsAssert; use ic_interfaces::crypto::KeyManager; use ic_interfaces_registry::RegistryClient; @@ -1272,7 +1274,6 @@ mod rotate_idkg_dealing_encryption_keys { fn should_return_transient_error_if_key_mismatch_then_latest_rotation_too_recent_with_retry() { use ic_crypto_test_utils_csp::MockAllCryptoServiceProvider; use ic_interfaces::crypto::KeyManager; - use ic_logger::replica_logger::no_op_logger; use ic_protobuf::registry::subnet::v1::SubnetListRecord; use ic_registry_keys::{make_subnet_list_record_key, make_subnet_record_key}; @@ -1339,8 +1340,9 @@ mod rotate_idkg_dealing_encryption_keys { .expect("Failed to add subnet list record key"); let time_source = FastForwardTimeSource::new(); - let crypto_component = CryptoComponentImpl::new_with_csp_and_fake_node_id( + let crypto_component = CryptoComponentImpl::new_for_test( csp, + Arc::new(MockLocalCspVault::new()), no_op_logger(), registry_client.clone(), node_id(), @@ -1368,7 +1370,6 @@ mod rotate_idkg_dealing_encryption_keys { fn should_return_transient_error_if_key_mismatch_then_rotate_with_retry() { use ic_crypto_test_utils_csp::MockAllCryptoServiceProvider; use ic_interfaces::crypto::KeyManager; - use ic_logger::replica_logger::no_op_logger; use ic_protobuf::registry::subnet::v1::SubnetListRecord; use ic_registry_keys::{make_subnet_list_record_key, make_subnet_record_key}; @@ -1438,14 +1439,7 @@ mod rotate_idkg_dealing_encryption_keys { ) .expect("Failed to add subnet list record key"); - let crypto_component = CryptoComponentImpl::new_with_csp_and_fake_node_id( - csp, - no_op_logger(), - registry_client.clone(), - node_id(), - Arc::new(CryptoMetrics::none()), - None, - ); + let crypto_component = crypto_component_with_csp(csp, registry_client.clone()); registry_client.reload(); let result = crypto_component.rotate_idkg_dealing_encryption_keys(REGISTRY_VERSION_1); @@ -1958,8 +1952,9 @@ impl SetupBuilder { ); let time_source = FastForwardTimeSource::new(); - let crypto = CryptoComponentImpl::new_with_csp_and_fake_node_id( + let crypto = CryptoComponentImpl::new_for_test( mock_csp, + Arc::new(MockLocalCspVault::new()), self.logger.unwrap_or_else(no_op_logger), Arc::clone(®istry_client), node_id(), diff --git a/rs/crypto/src/lib.rs b/rs/crypto/src/lib.rs index 5cdfa8a6305..9ccb4bcb5e7 100644 --- a/rs/crypto/src/lib.rs +++ b/rs/crypto/src/lib.rs @@ -17,6 +17,7 @@ mod keygen; mod sign; mod tls; +use ic_crypto_internal_csp::vault::api::CspVault; pub use sign::{ get_tecdsa_master_public_key, retrieve_mega_public_key_from_registry, MegaKeyFromRegistryError, }; @@ -24,6 +25,7 @@ pub use sign::{ use crate::sign::ThresholdSigDataStoreImpl; use ic_config::crypto::CryptoConfig; use ic_crypto_internal_csp::api::CspPublicKeyStore; +use ic_crypto_internal_csp::vault::vault_from_config; use ic_crypto_internal_csp::{CryptoServiceProvider, Csp}; use ic_crypto_internal_logmon::metrics::CryptoMetrics; use ic_crypto_utils_basic_sig::conversions::derive_node_id; @@ -53,6 +55,7 @@ pub type CryptoComponent = CryptoComponentImpl; /// handshakes. pub struct CryptoComponentImpl { lockable_threshold_sig_data_store: LockableThresholdSigDataStore, + _vault: Arc, csp: C, registry_client: Arc, // The node id of the node that instantiated this crypto component. @@ -90,10 +93,13 @@ impl LockableThresholdSigDataStore { } } +/// Methods required for testing. Ideally, this block would be `#[test]` code, +/// but this is not possible as the methods are required outside of the crate. impl CryptoComponentImpl { /// Creates a crypto component using the given `csp` and fake `node_id`. - pub fn new_with_csp_and_fake_node_id( + pub fn new_for_test( csp: C, + vault: Arc, logger: ReplicaLogger, registry_client: Arc, node_id: NodeId, @@ -103,6 +109,7 @@ impl CryptoComponentImpl { CryptoComponentImpl { lockable_threshold_sig_data_store: LockableThresholdSigDataStore::new(), csp, + _vault: vault, registry_client, node_id, logger: new_logger!(&logger), @@ -184,12 +191,18 @@ impl CryptoComponentImpl { metrics_registry: Option<&MetricsRegistry>, ) -> Self { let metrics = Arc::new(CryptoMetrics::new(metrics_registry)); - let csp = Csp::new( + let vault = vault_from_config( config, tokio_runtime_handle, - Some(new_logger!(&logger)), + new_logger!(&logger), Arc::clone(&metrics), ); + let csp = Csp::builder( + Arc::clone(&vault), + new_logger!(&logger), + Arc::clone(&metrics), + ) + .build(); let node_pks = csp .current_node_public_keys() .expect("Failed to retrieve node public keys"); @@ -203,11 +216,12 @@ impl CryptoComponentImpl { let crypto_component = CryptoComponentImpl { lockable_threshold_sig_data_store: LockableThresholdSigDataStore::new(), csp, + _vault: vault, registry_client, node_id, - logger: new_logger!(&logger), + time_source: Arc::new(CurrentSystemTimeSource::new(new_logger!(&logger))), + logger, metrics, - time_source: Arc::new(CurrentSystemTimeSource::new(logger)), }; crypto_component.collect_and_store_key_count_metrics(latest_registry_version); crypto_component diff --git a/rs/crypto/temp_crypto/src/lib.rs b/rs/crypto/temp_crypto/src/lib.rs index a619e818050..bf1d45feaf7 100644 --- a/rs/crypto/temp_crypto/src/lib.rs +++ b/rs/crypto/temp_crypto/src/lib.rs @@ -412,8 +412,9 @@ pub mod internal { fake_registry_client as Arc }); - let crypto_component = CryptoComponent::new_with_csp_and_fake_node_id( + let crypto_component = CryptoComponent::new_for_test( csp, + local_vault, logger, registry_client, node_id, diff --git a/rs/crypto/tests/canister_threshold_sigs.rs b/rs/crypto/tests/canister_threshold_sigs.rs index ae7575f995c..89ce765dc4f 100644 --- a/rs/crypto/tests/canister_threshold_sigs.rs +++ b/rs/crypto/tests/canister_threshold_sigs.rs @@ -18,6 +18,7 @@ use ic_crypto_test_utils_canister_threshold_sigs::{ CanisterThresholdSigTestEnvironment, IntoBuilder, }; use ic_crypto_test_utils_canister_threshold_sigs::{setup_masked_random_params, IDkgParticipants}; +use ic_crypto_test_utils_local_csp_vault::MockLocalCspVault; use ic_crypto_test_utils_reproducible_rng::{reproducible_rng, ReproducibleRng}; use ic_interfaces::crypto::{IDkgProtocol, ThresholdEcdsaSigVerifier, ThresholdEcdsaSigner}; use ic_types::crypto::canister_threshold_sig::error::{ @@ -3474,8 +3475,9 @@ mod verify_dealing_private { let metrics = MetricsRegistry::new(); let crypto_metrics = Arc::new(CryptoMetrics::new(Some(&metrics))); let time_source = None; - let crypto = CryptoComponentImpl::new_with_csp_and_fake_node_id( + let crypto = CryptoComponentImpl::new_for_test( csp, + Arc::new(MockLocalCspVault::new()), logger, registry_client, node_id,