refactor(orchestrator): [CON-1457] Remove dependency to canister_client in orchestrator (#5104)

The orchestrator still depends on the unmaintained `canister_client`
crate to send update messages to the registry during node registration.
We would like to remove this dependency and instead rely on the
[`ic_agent` crate](https://docs.rs/ic-agent/latest/ic_agent/), which is
better maintained.

This PR replaces the two occurrences of an update call using the
mentioned crate, mostly refactoring the `orchestrator::signer` module to
provide the API that `ic_agent` expects to sign messages. Concretely, it
consists of implementing `ic_agent::Identity` traits functionally
equivalently to `canister_client::Sender`, taking inspiration from the
[trait’s
implementation](https://docs.rs/ic-agent/latest/src/ic_agent/identity/secp256k1.rs.html#75)
for `ic_agent::identity::Secp256k1Identity` compared to the one found in
[`canister_client`](https://github.com/dfinity/ic/blob/528c6848d1afd1377958e6a71e8618e6d44c4665/rs/canister_client/sender/src/lib.rs#L126).

---------

Co-authored-by: IDX GitHub Automation <infra+github-automation@dfinity.org>

Author: pierugo-dfinity

Cargo.lock

@@ -116,7 +116,7 @@ pub enum Sender {
     PrincipalId(PrincipalId),
     /// Signed from the node itself, with its key.
     Node {
-        /// DER encoded public key
+        /// Ed25519 public key
         pub_key: Vec<u8>,
         /// Function that signs the message id
         sign: SignMessageId,

rs/canister_client/sender/src/lib.rs

@@ -18,8 +18,7 @@ rust_library(
     version = "0.9.0",
     deps = [
         # Keep sorted.
-        "//rs/canister_client",
-        "//rs/canister_client/sender",
+        "//packages/ic-ed25519",
         "//rs/config",
         "//rs/consensus",
         "//rs/consensus/dkg",
@@ -56,6 +55,7 @@ rust_library(
         "@crate_index//:hyper",
         "@crate_index//:hyper-rustls",
         "@crate_index//:hyper-util",
+        "@crate_index//:ic-agent",
         "@crate_index//:idna",
         "@crate_index//:nix",
         "@crate_index//:prometheus",

rs/orchestrator/BUILD.bazel

@@ -19,16 +19,16 @@ http-body-util = { workspace = true }
 hyper = { workspace = true }
 hyper-util = { workspace = true }
 hyper-rustls = { workspace = true }
-ic-http-endpoints-async-utils = { path = "../http_endpoints/async_utils" }
-ic-canister-client = { path = "../canister_client" }
-ic-canister-client-sender = { path = "../canister_client/sender" }
+ic-agent = { workspace = true }
 ic-config = { path = "../config" }
 ic-consensus = { path = "../consensus" }
 ic-consensus-dkg = { path = "../consensus/dkg" }
 ic-crypto = { path = "../crypto" }
 ic-crypto-node-key-generation = { path = "../crypto/node_key_generation" }
 ic-crypto-tls-interfaces = { path = "../crypto/tls_interfaces" }
 ic-dashboard = { path = "./dashboard" }
+ic-ed25519 = { path = "../../packages/ic-ed25519" }
+ic-http-endpoints-async-utils = { path = "../http_endpoints/async_utils" }
 ic-http-endpoints-metrics = { path = "../http_endpoints/metrics" }
 ic-http-utils = { path = "../http_utils" }
 ic-image-upgrader = { path = "./image_upgrader" }

rs/orchestrator/Cargo.toml

@@ -2,11 +2,11 @@
 use crate::{
     error::{OrchestratorError, OrchestratorResult},
     metrics::{KeyRotationStatus, OrchestratorMetrics},
-    signer::{Hsm, NodeProviderSigner, Signer},
+    signer::{Hsm, NodeProviderSigner, NodeSender, Signer},
     utils::http_endpoint_to_url,
 };
 use candid::Encode;
-use ic_canister_client::{Agent, Sender};
+use ic_agent::{export::Principal, Agent};
 use ic_config::{
     http_handler::Config as HttpConfig,
     initial_ipv4_config::IPv4Config as InitialIPv4Config,
@@ -149,16 +149,18 @@ impl NodeRegistration {
                     let nns_url = self
                         .get_random_nns_url_from_config()
                         .expect("no NNS urls available");
-                    let agent = Agent::new(nns_url, signer);
+                    let agent = Agent::builder()
+                        .with_url(nns_url)
+                        .with_identity(signer)
+                        .build()
+                        .expect("Failed to create IC agent");
+                    let add_node_encoded = Encode!(&add_node_payload)
+                        .expect("Could not encode payload for the registration request");
+
                     if let Err(e) = agent
-                        .execute_update(
-                            &REGISTRY_CANISTER_ID,
-                            &REGISTRY_CANISTER_ID,
-                            "add_node",
-                            Encode!(&add_node_payload)
-                                .expect("Could not encode payload for the registration request"),
-                            generate_nonce(),
-                        )
+                        .update(&Principal::from(REGISTRY_CANISTER_ID), "add_node")
+                        .with_arg(add_node_encoded)
+                        .call_and_wait()
                         .await
                     {
                         warn!(self.log, "Registration request failed: {}", e);
@@ -440,26 +442,25 @@ impl NodeRegistration {
             })
         };
 
-        let sender = Sender::Node {
-            pub_key: node_pub_key.key_value,
-            sign: Arc::new(sign_cmd),
-        };
-
-        let agent = Agent::new(nns_url.clone(), sender);
+        let signer = NodeSender::new(node_pub_key, Arc::new(sign_cmd))?;
+        let agent = Agent::builder()
+            .with_url(nns_url)
+            .with_identity(signer)
+            .build()
+            .map_err(|e| format!("Failed to create IC agent: {e}"))?;
         let update_node_payload = UpdateNodeDirectlyPayload {
             idkg_dealing_encryption_pk: Some(protobuf_to_vec(idkg_pk)),
         };
+        let update_node_encoded = Encode!(&update_node_payload)
+            .map_err(|e| format!("Could not encode payload for update_node-call: {e}"))?;
 
-        let arguments =
-            Encode!(&update_node_payload).expect("Could not encode payload for update_node-call.");
         agent
-            .execute_update(
-                &REGISTRY_CANISTER_ID,
-                &REGISTRY_CANISTER_ID,
+            .update(
+                &Principal::from(REGISTRY_CANISTER_ID),
                 "update_node_directly",
-                arguments,
-                generate_nonce(),
             )
+            .with_arg(update_node_encoded)
+            .call_and_wait()
             .await
             .map_err(|e| format!("Error when sending register additional key request: {e}"))?;
 
@@ -687,17 +688,6 @@ fn process_domain_name(log: &ReplicaLogger, domain: &str) -> OrchestratorResult<
     Ok(Some(domain.to_string()))
 }
 
-/// Create a nonce to be included with the ingress message sent to the node
-/// handler.
-fn generate_nonce() -> Vec<u8> {
-    SystemTime::now()
-        .duration_since(SystemTime::UNIX_EPOCH)
-        .unwrap()
-        .as_nanos()
-        .to_le_bytes()
-        .to_vec()
-}
-
 fn protobuf_to_vec<M: Message>(entry: M) -> Vec<u8> {
     let mut buf: Vec<u8> = Vec::new();
     entry.encode(&mut buf).expect("This must not fail");

rs/orchestrator/src/registration.rs

@@ -1,19 +1,26 @@
-use ic_canister_client::Sender;
-use ic_canister_client_sender::{Secp256k1KeyPair, SigKeys};
+use ic_agent::{
+    agent::EnvelopeContent, export::Principal, identity::Secp256k1Identity, Identity, Signature,
+};
+use ic_protobuf::registry::crypto::v1::{AlgorithmId, PublicKey};
 use ic_sys::utility_command::{UtilityCommand, UtilityCommandResult};
+use ic_types::messages::MessageId;
 use std::{path::Path, sync::Arc};
 
 /// An abstract message signer interface.
 pub trait Signer: Send + Sync {
     /// Returns the message signer bundle containing the public key and a signing command. This
     /// object is intended to be used with an agent to send messages to IC canisters.
-    fn get(&self) -> UtilityCommandResult<Sender>;
+    fn get(&self) -> UtilityCommandResult<Box<dyn Identity>>;
 }
 
+type SignBytes = Arc<dyn Fn(&[u8]) -> Result<Vec<u8>, Box<dyn std::error::Error>> + Send + Sync>;
+type SignMessageId =
+    Arc<dyn Fn(&MessageId) -> Result<Vec<u8>, Box<dyn std::error::Error>> + Send + Sync>;
+
 pub struct Hsm;
 
 impl Signer for Hsm {
-    fn get(&self) -> UtilityCommandResult<Sender> {
+    fn get(&self) -> UtilityCommandResult<Box<dyn Identity>> {
         UtilityCommand::notify_host("Starting node registration.", 1);
         UtilityCommand::try_to_attach_hsm();
         let pub_key = UtilityCommand::read_public_key(None, None).execute()?;
@@ -27,29 +34,110 @@ impl Signer for Hsm {
             UtilityCommand::try_to_detach_hsm();
             res
         }
-        Ok(Sender::ExternalHsm {
-            pub_key,
+        Ok(Box::new(ExternalHsmSender {
+            der_encoded_pub_key: pub_key,
             sign: Arc::new(get_sign_command),
+        }))
+    }
+}
+
+/// The sender is authenticated via an external HSM device and the signature mechanism is specified
+/// through the provided function reference.
+struct ExternalHsmSender {
+    /// DER encoded public key
+    der_encoded_pub_key: Vec<u8>,
+    /// Function that abstracts the external HSM.
+    sign: SignBytes,
+}
+
+impl Identity for ExternalHsmSender {
+    fn sender(&self) -> Result<Principal, String> {
+        Ok(Principal::self_authenticating(
+            self.der_encoded_pub_key.as_slice(),
+        ))
+    }
+
+    fn public_key(&self) -> Option<Vec<u8>> {
+        Some(self.der_encoded_pub_key.clone())
+    }
+
+    fn sign(&self, content: &EnvelopeContent) -> Result<Signature, String> {
+        let msg = content.to_request_id().signable();
+        let signature =
+            Some((self.sign)(&msg).map_err(|err| format!("Cannot create hsm signature: {err}"))?);
+        let public_key = self.public_key();
+        Ok(Signature {
+            public_key,
+            signature,
+            delegations: None,
         })
     }
 }
 
 pub struct NodeProviderSigner {
-    keypair: Secp256k1KeyPair,
+    identity: Secp256k1Identity,
 }
 
 impl NodeProviderSigner {
     pub fn new(path: &Path) -> Option<Self> {
-        let contents = std::fs::read_to_string(path).ok()?;
-        let keypair = Secp256k1KeyPair::from_pem(&contents).ok()?;
-        Some(Self { keypair })
+        let identity = Secp256k1Identity::from_pem_file(path).ok()?;
+        Some(Self { identity })
     }
 }
 
 impl Signer for NodeProviderSigner {
-    fn get(&self) -> UtilityCommandResult<Sender> {
-        Ok(Sender::SigKeys(SigKeys::EcdsaSecp256k1(
-            self.keypair.clone(),
-        )))
+    fn get(&self) -> UtilityCommandResult<Box<dyn Identity>> {
+        Ok(Box::new(self.identity.clone()))
+    }
+}
+
+/// Signed from the node itself, with its key.
+pub struct NodeSender {
+    /// DER encoded public key
+    der_encoded_pub_key: Vec<u8>,
+    /// Function that signs the message id
+    sign: SignMessageId,
+}
+
+impl NodeSender {
+    pub fn new(pub_key: PublicKey, sign: SignMessageId) -> Result<Self, String> {
+        if pub_key.algorithm() != AlgorithmId::Ed25519 {
+            return Err(format!(
+                "Unsupported algorithm: {}",
+                pub_key.algorithm().as_str_name()
+            ));
+        }
+
+        let der_encoded_pub_key = ic_ed25519::PublicKey::convert_raw_to_der(&pub_key.key_value)
+            .map_err(|err| err.to_string())?;
+
+        Ok(Self {
+            der_encoded_pub_key,
+            sign,
+        })
+    }
+}
+
+impl Identity for NodeSender {
+    fn sender(&self) -> Result<Principal, String> {
+        Ok(Principal::self_authenticating(
+            self.der_encoded_pub_key.as_slice(),
+        ))
+    }
+
+    fn public_key(&self) -> Option<Vec<u8>> {
+        Some(self.der_encoded_pub_key.clone())
+    }
+
+    fn sign(&self, content: &EnvelopeContent) -> Result<Signature, String> {
+        let msg = MessageId::from(*content.to_request_id());
+        let signature =
+            Some((self.sign)(&msg).map_err(|err| format!("Cannot create node signature: {err}"))?);
+        let public_key = self.public_key();
+        Ok(Signature {
+            public_key,
+            signature,
+            delegations: None,
+        })
     }
 }