refactor(crypto): CRP-1416: remove obsolete CryptoComponentForNonReplicaProcess trait

Author: fspreiss

rs/crypto/src/lib.rs

@@ -26,18 +26,15 @@ use ic_config::crypto::CryptoConfig;
 use ic_crypto_internal_csp::api::CspPublicKeyStore;
 use ic_crypto_internal_csp::{CryptoServiceProvider, Csp};
 use ic_crypto_internal_logmon::metrics::CryptoMetrics;
-use ic_crypto_tls_interfaces::TlsHandshake;
 use ic_crypto_utils_basic_sig::conversions::derive_node_id;
 use ic_crypto_utils_time::CurrentSystemTimeSource;
-use ic_interfaces::crypto::{BasicSigner, KeyManager, ThresholdSigVerifierByPublicKey};
+use ic_interfaces::crypto::KeyManager;
 use ic_interfaces::time_source::TimeSource;
 use ic_interfaces_registry::RegistryClient;
 use ic_logger::{new_logger, ReplicaLogger};
 use ic_metrics::MetricsRegistry;
 use ic_protobuf::registry::crypto::v1::{PublicKey as PublicKeyProto, X509PublicKeyCert};
-use ic_types::consensus::CatchUpContentProtobufBytes;
 use ic_types::crypto::{CryptoError, CryptoResult, KeyPurpose};
-use ic_types::messages::MessageId;
 use ic_types::{NodeId, RegistryVersion};
 use parking_lot::{RwLock, RwLockReadGuard, RwLockWriteGuard};
 use std::fmt;
@@ -51,42 +48,6 @@ pub const THRESHOLD_SIG_DATA_STORE_CAPACITY: usize = ThresholdSigDataStoreImpl::
 /// See the Rust documentation of `CryptoComponentImpl`.
 pub type CryptoComponent = CryptoComponentImpl<Csp>;
 
-/// A crypto component that offers limited functionality and can be used outside
-/// of the replica process.
-///
-/// This is an intermediate solution before crypto runs in a separate process.
-///
-/// This should be used whenever crypto is required on a node, but a
-/// full-fledged `CryptoComponent` is not available. Example use cases are in
-/// separate process such as ic-fe or the orchestrator.
-///
-/// Do not instantiate a CryptoComponent outside of the replica process, since
-/// that may lead to problems with concurrent access to the secret key store.
-/// `CryptoComponentForNonReplicaProcess` guarantees that only methods are
-/// exposed that don't risk running into such concurrency issues, as they do not
-/// modify the secret key store.
-pub trait CryptoComponentForNonReplicaProcess:
-    KeyManager
-    + BasicSigner<MessageId>
-    + ThresholdSigVerifierByPublicKey<CatchUpContentProtobufBytes>
-    + TlsHandshake
-    + Send
-    + Sync // TODO(CRP-606): add API for authenticating registry queries.
-{
-}
-
-// Blanket implementation of `CryptoComponentForNonReplicaProcess` for all types
-// that fulfill the requirements.
-impl<T> CryptoComponentForNonReplicaProcess for T where
-    T: KeyManager
-        + BasicSigner<MessageId>
-        + ThresholdSigVerifierByPublicKey<CatchUpContentProtobufBytes>
-        + TlsHandshake
-        + Send
-        + Sync
-{
-}
-
 /// Allows Internet Computer nodes to perform crypto operations such as
 /// distributed key generation, signing, signature verification, and TLS
 /// handshakes.
@@ -282,54 +243,6 @@ impl CryptoComponentImpl<Csp> {
         }
     }
 
-    /// Creates a crypto component that offers limited functionality and can be
-    /// used outside of the replica process.
-    ///
-    /// Please refer to the trait documentation of
-    /// `CryptoComponentForNonReplicaProcess` for more details.
-    ///
-    /// If the `config`'s vault type is `UnixSocket`, a `tokio_runtime_handle`
-    /// must be provided, which is then used for the `async`hronous
-    /// communication with the vault via RPC for secret key operations. In most
-    /// cases, this is done by calling `tokio::runtime::Handle::block_on` and
-    /// it is the caller's responsibility to ensure that these calls to
-    /// `block_on` do not panic. This can be achieved, for example, by ensuring
-    /// that the crypto component's methods are not themselves called from
-    /// within a call to `block_on` (because calls to `block_on` cannot be
-    /// nested), or by wrapping them with `tokio::task::block_in_place`
-    /// and accepting the performance implications.
-    /// Because the asynchronous communication with the vault happens only for
-    /// secret key operations, for the `CryptoComponentImpl` the concerned
-    /// methods are
-    /// * `KeyManager::check_keys_with_registry`
-    /// * `BasicSigner::sign_basic`
-    ///
-    /// The methods of the `TlsHandshake` trait are unaffected by this.
-    ///
-    /// # NOTE:
-    /// Callers of this method are strongly encouraged to switch from using
-    /// `CryptoComponentForNonReplicaProcess`, to using the full crypto component,
-    /// by calling `new` instead of `new_for_non_replica_process`.
-    ///
-    /// # Panics
-    /// Panics if the `config`'s vault type is `UnixSocket` and
-    /// `tokio_runtime_handle` is `None`.
-    pub fn new_for_non_replica_process(
-        config: &CryptoConfig,
-        tokio_runtime_handle: Option<tokio::runtime::Handle>,
-        registry_client: Arc<dyn RegistryClient>,
-        logger: ReplicaLogger,
-        metrics_registry: Option<&MetricsRegistry>,
-    ) -> impl CryptoComponentForNonReplicaProcess {
-        CryptoComponentImpl::new(
-            config,
-            tokio_runtime_handle,
-            registry_client,
-            logger,
-            metrics_registry,
-        )
-    }
-
     /// Returns the `NodeId` of this crypto component.
     pub fn get_node_id(&self) -> NodeId {
         self.node_id

rs/crypto/tests/integration_test.rs

@@ -9,8 +9,6 @@ use ic_crypto_internal_csp_test_utils::remote_csp_vault::{
     get_temp_file_path, start_new_remote_csp_vault_server_for_test,
 };
 use ic_crypto_internal_tls::generate_tls_key_pair_der;
-use ic_crypto_node_key_generation::generate_node_keys_once;
-use ic_crypto_node_key_validation::ValidNodePublicKeys;
 use ic_crypto_temp_crypto::{EcdsaSubnetConfig, NodeKeysToGenerate, TempCryptoComponent};
 use ic_crypto_test_utils::files::temp_dir;
 use ic_crypto_test_utils_keygen::{add_public_key_to_registry, add_tls_cert_to_registry};
@@ -102,70 +100,6 @@ fn should_not_construct_crypto_component_if_remote_csp_vault_is_missing() {
     );
 }
 
-#[test]
-#[should_panic(expected = "Missing node signing public key")]
-fn should_not_construct_crypto_component_for_non_replica_process_without_keys() {
-    CryptoConfig::run_with_temp_config(|config| {
-        let registry_client = FakeRegistryClient::new(Arc::new(ProtoRegistryDataProvider::new()));
-        let _crypto = CryptoComponent::new_for_non_replica_process(
-            &config,
-            None,
-            Arc::new(registry_client),
-            no_op_logger(),
-            None,
-        );
-    })
-}
-
-#[test]
-fn should_successfully_construct_crypto_component_for_non_replica_process_with_default_config_and_keys(
-) {
-    CryptoConfig::run_with_temp_config(|config| {
-        // Create node keys.
-        let _created_node_pks =
-            generate_node_keys_once(&config, None).expect("error generating node public keys");
-
-        let registry_client = FakeRegistryClient::new(Arc::new(ProtoRegistryDataProvider::new()));
-        let _crypto = CryptoComponent::new_for_non_replica_process(
-            &config,
-            None,
-            Arc::new(registry_client),
-            no_op_logger(),
-            None,
-        );
-    })
-}
-
-#[test]
-fn should_provide_public_keys_via_crypto_for_non_replica_process() {
-    CryptoConfig::run_with_temp_config(|config| {
-        // Create node keys.
-        let created_node_pks =
-            generate_node_keys_once(&config, None).expect("error generating node public keys");
-        let node_id = created_node_pks.node_id();
-
-        let registry_client = FakeRegistryClient::new(Arc::new(ProtoRegistryDataProvider::new()));
-        let crypto = CryptoComponent::new_for_non_replica_process(
-            &config,
-            None,
-            Arc::new(registry_client),
-            no_op_logger(),
-            None,
-        );
-
-        let retrieved_node_pks = ValidNodePublicKeys::try_from(
-            crypto
-                .current_node_public_keys()
-                .expect("Failed to retrieve node public keys"),
-            node_id,
-            ic_types::time::current_time(),
-        )
-        .expect("retrieved keys are not valid");
-
-        assert_eq!(created_node_pks, retrieved_node_pks);
-    })
-}
-
 // TODO(CRP-430): check/improve the test coverage of SKS checks.
 
 #[test]

rs/orchestrator/src/catch_up_package_provider.rs

@@ -34,7 +34,7 @@ use crate::error::{OrchestratorError, OrchestratorResult};
 use crate::registry_helper::RegistryHelper;
 use ic_canister_client::Sender;
 use ic_canister_client::{Agent, HttpClient};
-use ic_crypto::CryptoComponentForNonReplicaProcess;
+use ic_interfaces::crypto::ThresholdSigVerifierByPublicKey;
 use ic_logger::{info, warn, ReplicaLogger};
 use ic_protobuf::registry::node::v1::NodeRecord;
 use ic_protobuf::types::v1 as pb;
@@ -60,7 +60,7 @@ pub(crate) struct CatchUpPackageProvider {
     registry: Arc<RegistryHelper>,
     cup_dir: PathBuf,
     client: HttpClient,
-    crypto: Arc<dyn CryptoComponentForNonReplicaProcess + Send + Sync>,
+    crypto: Arc<dyn ThresholdSigVerifierByPublicKey<CatchUpContentProtobufBytes> + Send + Sync>,
     logger: ReplicaLogger,
     node_id: NodeId,
 }
@@ -70,7 +70,7 @@ impl CatchUpPackageProvider {
     pub(crate) fn new(
         registry: Arc<RegistryHelper>,
         cup_dir: PathBuf,
-        crypto: Arc<dyn CryptoComponentForNonReplicaProcess + Send + Sync>,
+        crypto: Arc<dyn ThresholdSigVerifierByPublicKey<CatchUpContentProtobufBytes> + Send + Sync>,
         logger: ReplicaLogger,
         node_id: NodeId,
     ) -> Self {

rs/orchestrator/src/orchestrator.rs

@@ -11,7 +11,7 @@ use crate::registry_helper::RegistryHelper;
 use crate::ssh_access_manager::SshAccessManager;
 use crate::upgrade::Upgrade;
 use ic_config::metrics::{Config as MetricsConfig, Exporter};
-use ic_crypto::{CryptoComponent, CryptoComponentForNonReplicaProcess};
+use ic_crypto::CryptoComponent;
 use ic_crypto_node_key_generation::{generate_node_keys_once, NodeKeyGenerationError};
 use ic_crypto_tls_interfaces::TlsHandshake;
 use ic_http_endpoints_metrics::MetricsHttpEndpoint;
@@ -152,7 +152,7 @@ impl Orchestrator {
         let crypto_config = config.crypto.clone();
         let c_metrics = metrics_registry.clone();
         let crypto = tokio::task::spawn_blocking(move || {
-            Arc::new(CryptoComponent::new_for_non_replica_process(
+            Arc::new(CryptoComponent::new(
                 &crypto_config,
                 Some(tokio::runtime::Handle::current()),
                 c_registry.get_registry_client(),
@@ -169,7 +169,7 @@ impl Orchestrator {
             &slog_logger,
             &metrics_registry,
             registry.get_registry_client(),
-            crypto.clone(),
+            Arc::clone(&crypto) as _,
         );
         let metrics = Arc::new(metrics);
 
@@ -184,7 +184,7 @@ impl Orchestrator {
             Arc::clone(&registry_client),
             Arc::clone(&metrics),
             node_id,
-            Arc::clone(&crypto) as Arc<dyn CryptoComponentForNonReplicaProcess>,
+            Arc::clone(&crypto) as _,
             registry_local_store.clone(),
         );
 
@@ -198,7 +198,7 @@ impl Orchestrator {
         let cup_provider = Arc::new(CatchUpPackageProvider::new(
             Arc::clone(&registry),
             args.cup_dir.clone(),
-            crypto.clone(),
+            Arc::clone(&crypto) as _,
             logger.clone(),
             node_id,
         ));

rs/orchestrator/src/registration.rs

@@ -14,7 +14,6 @@ use ic_config::{
     transport::TransportConfig,
     Config,
 };
-use ic_crypto::CryptoComponentForNonReplicaProcess;
 use ic_icos_sev::{get_chip_id, SnpError};
 use ic_interfaces::crypto::IDkgKeyRotationResult;
 use ic_interfaces_registry::RegistryClient;
@@ -42,14 +41,28 @@ use url::Url;
 /// we use a 15% time buffer compensating for a potential delay of the previous node.
 const DELAY_COMPENSATION: f64 = 0.85;
 
+pub trait NodeRegistrationCrypto:
+    ic_interfaces::crypto::KeyManager + ic_interfaces::crypto::BasicSigner<MessageId> + Send + Sync
+{
+}
+
+// Blanket implementation of `NodeRegistrationCrypto` for all types that fulfill the requirements.
+impl<T> NodeRegistrationCrypto for T where
+    T: ic_interfaces::crypto::KeyManager
+        + ic_interfaces::crypto::BasicSigner<MessageId>
+        + Send
+        + Sync
+{
+}
+
 /// Subcomponent used to register this node with the provided NNS.
 pub(crate) struct NodeRegistration {
     log: ReplicaLogger,
     node_config: Config,
     registry_client: Arc<dyn RegistryClient>,
     metrics: Arc<OrchestratorMetrics>,
     node_id: NodeId,
-    key_handler: Arc<dyn CryptoComponentForNonReplicaProcess>,
+    key_handler: Arc<dyn NodeRegistrationCrypto>,
     local_store: Arc<dyn LocalStore>,
     signer: Box<dyn Signer>,
 }
@@ -63,7 +76,7 @@ impl NodeRegistration {
         registry_client: Arc<dyn RegistryClient>,
         metrics: Arc<OrchestratorMetrics>,
         node_id: NodeId,
-        key_handler: Arc<dyn CryptoComponentForNonReplicaProcess>,
+        key_handler: Arc<dyn NodeRegistrationCrypto>,
         local_store: Arc<dyn LocalStore>,
     ) -> Self {
         // If we can open a PEM file under the path specified in the replica config,