From 4c857b6e6a0e82226f0a91d69735afb8c97cfb8f Mon Sep 17 00:00:00 2001 From: Nikola Milosavljevic <73236646+NikolaMilosa@users.noreply.github.com> Date: Tue, 10 Sep 2024 12:12:31 +0200 Subject: [PATCH] refactor: `IcAdminRequirement` -> `AuthRequirement` (#895) --- .github/workflows/main.yaml | 7 +- rs/cli/src/auth.rs | 135 ++++++--- rs/cli/src/commands/api_boundary_nodes/add.rs | 8 +- rs/cli/src/commands/api_boundary_nodes/mod.rs | 6 +- .../src/commands/api_boundary_nodes/remove.rs | 8 +- .../src/commands/api_boundary_nodes/update.rs | 8 +- rs/cli/src/commands/completions.rs | 4 +- rs/cli/src/commands/der_to_principal.rs | 6 +- rs/cli/src/commands/firewall.rs | 8 +- rs/cli/src/commands/get.rs | 8 +- rs/cli/src/commands/heal.rs | 8 +- rs/cli/src/commands/hostos/mod.rs | 6 +- rs/cli/src/commands/hostos/rollout.rs | 8 +- .../hostos/rollout_from_node_group.rs | 8 +- rs/cli/src/commands/mod.rs | 44 ++- rs/cli/src/commands/neuron/balance.rs | 10 +- rs/cli/src/commands/neuron/mod.rs | 6 +- rs/cli/src/commands/neuron/refresh.rs | 6 +- rs/cli/src/commands/neuron/top_up.rs | 8 +- rs/cli/src/commands/node_metrics.rs | 6 +- rs/cli/src/commands/nodes/mod.rs | 6 +- rs/cli/src/commands/nodes/remove.rs | 8 +- rs/cli/src/commands/proposals/analyze.rs | 8 +- rs/cli/src/commands/proposals/filter.rs | 6 +- rs/cli/src/commands/proposals/get.rs | 6 +- rs/cli/src/commands/proposals/list.rs | 6 +- rs/cli/src/commands/proposals/mod.rs | 6 +- rs/cli/src/commands/proposals/pending.rs | 6 +- rs/cli/src/commands/propose.rs | 8 +- rs/cli/src/commands/qualify/execute.rs | 12 +- rs/cli/src/commands/qualify/list.rs | 9 +- rs/cli/src/commands/qualify/mod.rs | 7 +- rs/cli/src/commands/registry.rs | 6 +- rs/cli/src/commands/subnet/create.rs | 8 +- rs/cli/src/commands/subnet/deploy.rs | 8 +- rs/cli/src/commands/subnet/mod.rs | 6 +- rs/cli/src/commands/subnet/replace.rs | 8 +- rs/cli/src/commands/subnet/rescue.rs | 8 +- rs/cli/src/commands/subnet/resize.rs | 8 +- rs/cli/src/commands/subnet/whatif.rs | 8 +- .../src/commands/update_authorized_subnets.rs | 8 +- .../src/commands/update_unassigned_nodes.rs | 15 +- rs/cli/src/commands/upgrade.rs | 6 +- rs/cli/src/commands/version/mod.rs | 6 +- .../src/commands/version/revise/guest_os.rs | 8 +- rs/cli/src/commands/version/revise/host_os.rs | 8 +- rs/cli/src/commands/version/revise/mod.rs | 7 +- rs/cli/src/commands/vote.rs | 8 +- rs/cli/src/ctx.rs | 222 ++++++--------- rs/cli/src/ic_admin.rs | 6 +- .../qualification/ensure_blessed_versions.rs | 1 + .../qualification/retire_blessed_versions.rs | 1 + rs/cli/src/qualification/upgrade_subnets.rs | 2 + rs/cli/src/unit_tests/ctx_init.rs | 268 +++++++++++++++++- .../src/unit_tests/update_unassigned_nodes.rs | 21 +- rs/cli/src/unit_tests/version.rs | 2 + rs/qualifier/src/qualify_util.rs | 5 +- 57 files changed, 651 insertions(+), 399 deletions(-) diff --git a/.github/workflows/main.yaml b/.github/workflows/main.yaml index fca33d1df..1ff9920eb 100644 --- a/.github/workflows/main.yaml +++ b/.github/workflows/main.yaml @@ -43,7 +43,12 @@ jobs: with: GITHUB_TOKEN: "${{ secrets.GIX_CREATE_PR_PAT }}" - name: "🚀 Testing" - run: bazel test --spawn_strategy=local ... + env: + STAGING_PRIVATE_KEY_PEM: "${{ secrets.STAGING_PRIVATE_KEY_PEM }}" + run: | + mkdir -p ~/.config/dfx/identity/bootstrap-super-leader/ + echo $STAGING_PRIVATE_KEY_PEM > ~/.config/dfx/identity/bootstrap-super-leader/identity.pem + bazel test ... --spawn_strategy=local --test_env=HOME=/home/runner # We don't need the linear-jira build and test step for now # - name: "🚀 Build and Test Linear-Jira with Bazel" diff --git a/rs/cli/src/auth.rs b/rs/cli/src/auth.rs index cb51b35df..6dfe1fbc4 100644 --- a/rs/cli/src/auth.rs +++ b/rs/cli/src/auth.rs @@ -2,7 +2,7 @@ use std::path::PathBuf; use std::str::FromStr; use anyhow::anyhow; -use clio::InputPath; +use clio::{ClioPath, InputPath}; use cryptoki::object::AttributeInfo; use cryptoki::session::Session; use cryptoki::{ @@ -20,17 +20,17 @@ use log::{debug, info, warn}; use secrecy::SecretString; use std::sync::Mutex; -use crate::commands::{AuthOpts, HsmOpts, HsmParams}; +use crate::commands::{AuthOpts, AuthRequirement, HsmOpts, HsmParams}; -#[derive(Clone, Debug)] +#[derive(Clone, Debug, Eq, PartialEq)] pub struct Neuron { pub auth: Auth, pub neuron_id: u64, pub include_proposer: bool, } -static RELEASE_AUTOMATION_DEFAULT_PRIVATE_KEY_PEM: &str = ".config/dfx/identity/release-automation/identity.pem"; // Relative to the home directory -const RELEASE_AUTOMATION_NEURON_ID: u64 = 80; +pub const STAGING_NEURON_ID: u64 = 49; +pub const STAGING_KEY_PATH_FROM_HOME: &str = ".config/dfx/identity/bootstrap-super-leader/identity.pem"; // As per fn str_to_key_id(s: &str) in ic-canisters/.../parallel_hardware_identity.rs, // the representation of key ID that the canister client wants is a sequence of @@ -43,6 +43,82 @@ pub fn hsm_key_id_to_string(s: u8) -> String { } impl Neuron { + pub(crate) async fn from_opts_and_req( + auth_opts: AuthOpts, + requirement: AuthRequirement, + network: &Network, + neuron_id: Option, + ) -> anyhow::Result { + let (neuron_id, auth_opts) = if network.name == "staging" { + let staging_known_path = PathBuf::from_str(&std::env::var("HOME").unwrap()) + // Must be a valid path + .unwrap() + .join(STAGING_KEY_PATH_FROM_HOME); + + match neuron_id { + Some(n) => (Some(n), auth_opts), + None => ( + Some(STAGING_NEURON_ID), + match Auth::pem(staging_known_path.clone()).await { + Ok(_) => AuthOpts { + private_key_pem: Some(InputPath::new(ClioPath::new(staging_known_path).unwrap()).unwrap()), + hsm_opts: HsmOpts { + hsm_pin: None, + hsm_params: HsmParams { + hsm_slot: None, + hsm_key_id: None, + }, + }, + }, + Err(e) => match requirement { + // If there is an error but auth is not needed + // just send what we have since it won't be + // checked anyway + AuthRequirement::Anonymous => auth_opts, + _ => anyhow::bail!("Failed to find staging auth: {:?}", e), + }, + }, + ), + } + } else { + (neuron_id, auth_opts) + }; + + Self::from_opts_and_req_inner(auth_opts, requirement, network, neuron_id).await + } + + async fn from_opts_and_req_inner( + auth_opts: AuthOpts, + requirement: AuthRequirement, + network: &Network, + neuron_id: Option, + ) -> anyhow::Result { + match requirement { + AuthRequirement::Anonymous => Ok(Self { + auth: Auth::Anonymous, + neuron_id: 0, + include_proposer: false, + }), + AuthRequirement::Signer => Ok(Self { + auth: Auth::from_auth_opts(auth_opts).await?, + neuron_id: 0, + include_proposer: false, + }), + AuthRequirement::Neuron => Ok({ + let auth = Auth::from_auth_opts(auth_opts).await?; + Self { + neuron_id: match neuron_id { + Some(n) => n, + None => auth.auto_detect_neuron_id(network.nns_urls.clone()).await?, + }, + auth, + include_proposer: true, + } + }), + } + } + + #[allow(dead_code)] pub async fn new(auth: Auth, neuron_id: Option, network: &Network, include_proposer: bool) -> anyhow::Result { let neuron_id = match neuron_id { Some(n) => n, @@ -67,19 +143,6 @@ impl Neuron { vec![] } - // FIXME: there should be no unchecked anything. - // Caller must be able to bubble up the error of the file not existing there. - pub fn automation_neuron_unchecked() -> Self { - debug!("Constructing neuron using the release automation private key"); - Self { - auth: Auth::Keyfile { - path: dirs::home_dir().unwrap().join(RELEASE_AUTOMATION_DEFAULT_PRIVATE_KEY_PEM), - }, - neuron_id: RELEASE_AUTOMATION_NEURON_ID, - include_proposer: true, - } - } - pub fn anonymous_neuron() -> Self { debug!("Constructing anonymous neuron (ID 0)"); Self { @@ -90,7 +153,7 @@ impl Neuron { } } -#[derive(Clone, Debug)] +#[derive(Clone, Debug, Eq, PartialEq)] pub enum Auth { Hsm { pin: String, slot: u64, key_id: u8 }, Keyfile { path: PathBuf }, @@ -158,6 +221,7 @@ impl Auth { } } + /// If it is called it is expected to retrieve an Auth of type Hsm or fail fn detect_hsm_auth(maybe_pin: Option, maybe_slot: Option, maybe_key_id: Option) -> anyhow::Result { if maybe_slot.is_none() && maybe_key_id.is_none() { debug!("Scanning hardware security module devices"); @@ -206,31 +270,26 @@ impl Auth { }; let memo_key: String = format!("hsm-{}-{}", info.slot_description(), info.manufacturer_id()); let pin = Auth::get_or_prompt_pin_checked_for_slot(&session, maybe_pin, slot, token_info, &memo_key)?; - let detected = Some(Auth::Hsm { + let detected = Auth::Hsm { pin, slot: slot.id(), key_id, - }); + }; info!("Using key ID {} of hardware security module in slot {}", key_id, slot); - return Ok(detected.map_or(Auth::Anonymous, |a| a)); + return Ok(detected); } } - if maybe_slot.is_none() && maybe_key_id.is_none() && maybe_pin.is_none() { - info!("No hardware security module detected -- falling back to anonymous operations"); - } else { - return Err(anyhow!( - "No hardware security module detected{}{}", - match maybe_slot { - None => "".to_string(), - Some(slot) => format!(" in slot {}", slot), - }, - match maybe_key_id { - None => "".to_string(), - Some(key_id) => format!(" that contains a key ID {}", key_id), - } - )); - } - Ok(Auth::Anonymous) + Err(anyhow!( + "No hardware security module detected{}{}", + match maybe_slot { + None => "".to_string(), + Some(slot) => format!(" in slot {}", slot), + }, + match maybe_key_id { + None => "".to_string(), + Some(key_id) => format!(" that contains a key ID {}", key_id), + } + )) } /// Finds the key ID in a slot. If a key ID is specified, diff --git a/rs/cli/src/commands/api_boundary_nodes/add.rs b/rs/cli/src/commands/api_boundary_nodes/add.rs index 2f04a0f3f..b23a1c967 100644 --- a/rs/cli/src/commands/api_boundary_nodes/add.rs +++ b/rs/cli/src/commands/api_boundary_nodes/add.rs @@ -2,7 +2,7 @@ use clap::Args; use ic_types::PrincipalId; use crate::{ - commands::{ExecutableCommand, IcAdminRequirement}, + commands::{AuthRequirement, ExecutableCommand}, ic_admin::{self}, }; @@ -22,12 +22,12 @@ pub struct Add { } impl ExecutableCommand for Add { - fn require_ic_admin(&self) -> IcAdminRequirement { - IcAdminRequirement::Detect + fn require_auth(&self) -> AuthRequirement { + AuthRequirement::Neuron } async fn execute(&self, ctx: crate::ctx::DreContext) -> anyhow::Result<()> { - let ic_admin = ctx.ic_admin(); + let ic_admin = ctx.ic_admin().await?; ic_admin .propose_run( diff --git a/rs/cli/src/commands/api_boundary_nodes/mod.rs b/rs/cli/src/commands/api_boundary_nodes/mod.rs index b53cc10f8..d576e3924 100644 --- a/rs/cli/src/commands/api_boundary_nodes/mod.rs +++ b/rs/cli/src/commands/api_boundary_nodes/mod.rs @@ -3,7 +3,7 @@ use clap::Args; use remove::Remove; use update::Update; -use super::{impl_executable_command_for_enums, ExecutableCommand, IcAdminRequirement}; +use super::{impl_executable_command_for_enums, AuthRequirement, ExecutableCommand}; mod add; mod remove; @@ -18,8 +18,8 @@ pub struct ApiBoundaryNodes { impl_executable_command_for_enums! { Add, Update, Remove } impl ExecutableCommand for ApiBoundaryNodes { - fn require_ic_admin(&self) -> IcAdminRequirement { - self.subcommand.require_ic_admin() + fn require_auth(&self) -> AuthRequirement { + self.subcommand.require_auth() } async fn execute(&self, ctx: crate::ctx::DreContext) -> anyhow::Result<()> { diff --git a/rs/cli/src/commands/api_boundary_nodes/remove.rs b/rs/cli/src/commands/api_boundary_nodes/remove.rs index ce54e8900..d3274af46 100644 --- a/rs/cli/src/commands/api_boundary_nodes/remove.rs +++ b/rs/cli/src/commands/api_boundary_nodes/remove.rs @@ -2,7 +2,7 @@ use clap::Args; use ic_types::PrincipalId; use crate::{ - commands::{ExecutableCommand, IcAdminRequirement}, + commands::{AuthRequirement, ExecutableCommand}, ic_admin::{self}, }; @@ -18,12 +18,12 @@ pub struct Remove { } impl ExecutableCommand for Remove { - fn require_ic_admin(&self) -> IcAdminRequirement { - IcAdminRequirement::Detect + fn require_auth(&self) -> AuthRequirement { + AuthRequirement::Neuron } async fn execute(&self, ctx: crate::ctx::DreContext) -> anyhow::Result<()> { - let ic_admin = ctx.ic_admin(); + let ic_admin = ctx.ic_admin().await?; ic_admin .propose_run( ic_admin::ProposeCommand::RemoveApiBoundaryNodes { nodes: self.nodes.to_vec() }, diff --git a/rs/cli/src/commands/api_boundary_nodes/update.rs b/rs/cli/src/commands/api_boundary_nodes/update.rs index 00b8389e3..191344300 100644 --- a/rs/cli/src/commands/api_boundary_nodes/update.rs +++ b/rs/cli/src/commands/api_boundary_nodes/update.rs @@ -2,7 +2,7 @@ use clap::Args; use ic_types::PrincipalId; use crate::{ - commands::{ExecutableCommand, IcAdminRequirement}, + commands::{AuthRequirement, ExecutableCommand}, ic_admin::{self}, }; @@ -21,12 +21,12 @@ pub struct Update { } impl ExecutableCommand for Update { - fn require_ic_admin(&self) -> IcAdminRequirement { - IcAdminRequirement::Detect + fn require_auth(&self) -> AuthRequirement { + AuthRequirement::Neuron } async fn execute(&self, ctx: crate::ctx::DreContext) -> anyhow::Result<()> { - let ic_admin = ctx.ic_admin(); + let ic_admin = ctx.ic_admin().await?; ic_admin .propose_run( diff --git a/rs/cli/src/commands/completions.rs b/rs/cli/src/commands/completions.rs index 43a0fd445..1902b7069 100644 --- a/rs/cli/src/commands/completions.rs +++ b/rs/cli/src/commands/completions.rs @@ -10,8 +10,8 @@ pub struct Completions { } impl ExecutableCommand for Completions { - fn require_ic_admin(&self) -> super::IcAdminRequirement { - super::IcAdminRequirement::None + fn require_auth(&self) -> super::AuthRequirement { + super::AuthRequirement::Anonymous } fn validate(&self, _cmd: &mut clap::Command) {} diff --git a/rs/cli/src/commands/der_to_principal.rs b/rs/cli/src/commands/der_to_principal.rs index 3fa5c2891..836ffcd15 100644 --- a/rs/cli/src/commands/der_to_principal.rs +++ b/rs/cli/src/commands/der_to_principal.rs @@ -2,7 +2,7 @@ use std::path::PathBuf; use clap::Args; -use super::{ExecutableCommand, IcAdminRequirement}; +use super::{AuthRequirement, ExecutableCommand}; #[derive(Args, Debug)] pub struct DerToPrincipal { @@ -11,8 +11,8 @@ pub struct DerToPrincipal { } impl ExecutableCommand for DerToPrincipal { - fn require_ic_admin(&self) -> IcAdminRequirement { - IcAdminRequirement::None + fn require_auth(&self) -> AuthRequirement { + AuthRequirement::Neuron } async fn execute(&self, _ctx: crate::ctx::DreContext) -> anyhow::Result<()> { diff --git a/rs/cli/src/commands/firewall.rs b/rs/cli/src/commands/firewall.rs index 9182d27d6..997045210 100644 --- a/rs/cli/src/commands/firewall.rs +++ b/rs/cli/src/commands/firewall.rs @@ -15,7 +15,7 @@ use tempfile::NamedTempFile; use crate::ic_admin::{IcAdmin, ProposeCommand, ProposeOptions}; -use super::{ExecutableCommand, IcAdminRequirement}; +use super::{AuthRequirement, ExecutableCommand}; #[derive(Args, Debug)] pub struct Firewall { @@ -31,8 +31,8 @@ pub struct Firewall { } impl ExecutableCommand for Firewall { - fn require_ic_admin(&self) -> IcAdminRequirement { - IcAdminRequirement::Detect + fn require_auth(&self) -> AuthRequirement { + AuthRequirement::Neuron } async fn execute(&self, ctx: crate::ctx::DreContext) -> anyhow::Result<()> { @@ -99,7 +99,7 @@ impl ExecutableCommand for Firewall { match reverse_sorted.into_iter().last() { Some((_, mods)) => { Self::submit_proposal( - ctx.ic_admin(), + ctx.ic_admin().await?, mods, ProposeOptions { title: self.title.clone(), diff --git a/rs/cli/src/commands/get.rs b/rs/cli/src/commands/get.rs index 74c83509e..57dccd064 100644 --- a/rs/cli/src/commands/get.rs +++ b/rs/cli/src/commands/get.rs @@ -1,6 +1,6 @@ use clap::Args; -use super::{ExecutableCommand, IcAdminRequirement}; +use super::{AuthRequirement, ExecutableCommand}; #[derive(Args, Debug)] pub struct Get { @@ -10,12 +10,12 @@ pub struct Get { } impl ExecutableCommand for Get { - fn require_ic_admin(&self) -> IcAdminRequirement { - IcAdminRequirement::Anonymous + fn require_auth(&self) -> AuthRequirement { + AuthRequirement::Anonymous } async fn execute(&self, ctx: crate::ctx::DreContext) -> anyhow::Result<()> { - let ic_admin = ctx.ic_admin(); + let ic_admin = ctx.ic_admin().await?; let _ = ic_admin.run_passthrough_get(&self.args, false).await?; Ok(()) diff --git a/rs/cli/src/commands/heal.rs b/rs/cli/src/commands/heal.rs index c595fa28b..61955a502 100644 --- a/rs/cli/src/commands/heal.rs +++ b/rs/cli/src/commands/heal.rs @@ -1,17 +1,17 @@ use clap::Args; -use super::{ExecutableCommand, IcAdminRequirement}; +use super::{AuthRequirement, ExecutableCommand}; #[derive(Args, Debug)] pub struct Heal {} impl ExecutableCommand for Heal { - fn require_ic_admin(&self) -> IcAdminRequirement { - IcAdminRequirement::Detect + fn require_auth(&self) -> AuthRequirement { + AuthRequirement::Neuron } async fn execute(&self, ctx: crate::ctx::DreContext) -> anyhow::Result<()> { - let runner = ctx.runner().await; + let runner = ctx.runner().await?; runner.network_heal(ctx.forum_post_link()).await } diff --git a/rs/cli/src/commands/hostos/mod.rs b/rs/cli/src/commands/hostos/mod.rs index a7a79b7bd..d40274b62 100644 --- a/rs/cli/src/commands/hostos/mod.rs +++ b/rs/cli/src/commands/hostos/mod.rs @@ -2,7 +2,7 @@ use clap::Args; use rollout::Rollout; use rollout_from_node_group::RolloutFromNodeGroup; -use super::{ExecutableCommand, IcAdminRequirement}; +use super::{AuthRequirement, ExecutableCommand}; mod rollout; pub mod rollout_from_node_group; @@ -16,8 +16,8 @@ pub struct HostOs { super::impl_executable_command_for_enums! { Rollout, RolloutFromNodeGroup } impl ExecutableCommand for HostOs { - fn require_ic_admin(&self) -> IcAdminRequirement { - self.subcommand.require_ic_admin() + fn require_auth(&self) -> AuthRequirement { + self.subcommand.require_auth() } async fn execute(&self, ctx: crate::ctx::DreContext) -> anyhow::Result<()> { diff --git a/rs/cli/src/commands/hostos/rollout.rs b/rs/cli/src/commands/hostos/rollout.rs index 0ff3670c0..307ea0c87 100644 --- a/rs/cli/src/commands/hostos/rollout.rs +++ b/rs/cli/src/commands/hostos/rollout.rs @@ -1,7 +1,7 @@ use clap::Args; use ic_types::PrincipalId; -use crate::commands::{ExecutableCommand, IcAdminRequirement}; +use crate::commands::{AuthRequirement, ExecutableCommand}; #[derive(Args, Debug)] pub struct Rollout { @@ -15,12 +15,12 @@ pub struct Rollout { } impl ExecutableCommand for Rollout { - fn require_ic_admin(&self) -> IcAdminRequirement { - IcAdminRequirement::Detect + fn require_auth(&self) -> AuthRequirement { + AuthRequirement::Neuron } async fn execute(&self, ctx: crate::ctx::DreContext) -> anyhow::Result<()> { - let runner = ctx.runner().await; + let runner = ctx.runner().await?; runner .hostos_rollout(self.nodes.clone(), &self.version, None, ctx.forum_post_link()) .await diff --git a/rs/cli/src/commands/hostos/rollout_from_node_group.rs b/rs/cli/src/commands/hostos/rollout_from_node_group.rs index 7a15dc21a..3d33e358e 100644 --- a/rs/cli/src/commands/hostos/rollout_from_node_group.rs +++ b/rs/cli/src/commands/hostos/rollout_from_node_group.rs @@ -3,7 +3,7 @@ use std::str::FromStr; use clap::{Args, ValueEnum}; use crate::{ - commands::{ExecutableCommand, IcAdminRequirement}, + commands::{AuthRequirement, ExecutableCommand}, operations::hostos_rollout::{NodeGroupUpdate, NumberOfNodes}, }; @@ -76,13 +76,13 @@ supported values are absolute numbers (10) or percentage (10%)"# } impl ExecutableCommand for RolloutFromNodeGroup { - fn require_ic_admin(&self) -> IcAdminRequirement { - IcAdminRequirement::Detect + fn require_auth(&self) -> AuthRequirement { + AuthRequirement::Neuron } async fn execute(&self, ctx: crate::ctx::DreContext) -> anyhow::Result<()> { let update_group = NodeGroupUpdate::new(self.assignment, self.owner, NumberOfNodes::from_str(&self.nodes_in_group)?); - let runner = ctx.runner().await; + let runner = ctx.runner().await?; if let Some((nodes_to_update, summary)) = runner .hostos_rollout_nodes(update_group, &self.version, &self.only, &self.exclude) .await? diff --git a/rs/cli/src/commands/mod.rs b/rs/cli/src/commands/mod.rs index ac58e5640..5729e64fe 100644 --- a/rs/cli/src/commands/mod.rs +++ b/rs/cli/src/commands/mod.rs @@ -1,3 +1,4 @@ +use std::path::PathBuf; use std::{collections::BTreeMap, str::FromStr}; use crate::commands::subnet::Subnet; @@ -12,7 +13,7 @@ use firewall::Firewall; use get::Get; use heal::Heal; use hostos::HostOs; -use ic_management_types::{MinNakamotoCoefficients, Network, NodeFeature}; +use ic_management_types::{MinNakamotoCoefficients, NodeFeature}; use neuron::Neuron; use node_metrics::NodeMetrics; use nodes::Nodes; @@ -28,8 +29,6 @@ use url::Url; use version::Version; use vote::Vote; -use crate::auth::Neuron as AuthNeuron; - pub(crate) mod api_boundary_nodes; pub(crate) mod completions; pub(crate) mod der_to_principal; @@ -98,7 +97,7 @@ pub(crate) struct HsmOpts { #[derive(ClapArgs, Debug, Clone)] #[group(multiple = false)] /// Authentication arguments -pub(crate) struct AuthOpts { +pub struct AuthOpts { /// Path to private key file (in PEM format) #[clap( long, @@ -111,6 +110,23 @@ pub(crate) struct AuthOpts { pub(crate) hsm_opts: HsmOpts, } +impl TryFrom for AuthOpts { + type Error = anyhow::Error; + + fn try_from(value: PathBuf) -> std::result::Result { + Ok(AuthOpts { + private_key_pem: Some(InputPath::new(ClioPath::new(value)?)?), + hsm_opts: HsmOpts { + hsm_pin: None, + hsm_params: HsmParams { + hsm_slot: None, + hsm_key_id: None, + }, + }, + }) + } +} + #[derive(Parser, Debug)] #[clap(version = env!("CARGO_PKG_VERSION"), about, author)] pub(crate) struct Args { @@ -177,8 +193,8 @@ The argument is mandatory for testnets, and is optional for mainnet and staging" // Do not use outside of DRE CLI. // You can run your command by directly instantiating it. impl ExecutableCommand for Args { - fn require_ic_admin(&self) -> IcAdminRequirement { - self.subcommands.require_ic_admin() + fn require_auth(&self) -> AuthRequirement { + self.subcommands.require_auth() } async fn execute(&self, ctx: DreContext) -> anyhow::Result<()> { @@ -201,9 +217,9 @@ macro_rules! impl_executable_command_for_enums { )*} impl ExecutableCommand for Subcommands { - fn require_ic_admin(&self) -> IcAdminRequirement { + fn require_auth(&self) -> AuthRequirement { match &self { - $(Subcommands::$var(variant) => variant.require_ic_admin(),)* + $(Subcommands::$var(variant) => variant.require_auth(),)* } } @@ -226,7 +242,7 @@ pub(crate) use impl_executable_command_for_enums; impl_executable_command_for_enums! { DerToPrincipal, Heal, Subnet, Get, Propose, UpdateUnassignedNodes, Version, NodeMetrics, HostOs, Nodes, ApiBoundaryNodes, Vote, Registry, Firewall, Upgrade, Proposals, Completions, Qualify, UpdateAuthorizedSubnets, Neuron } pub trait ExecutableCommand { - fn require_ic_admin(&self) -> IcAdminRequirement; + fn require_auth(&self) -> AuthRequirement; fn validate(&self, cmd: &mut Command); @@ -301,11 +317,11 @@ pub trait ExecutableCommand { } } -pub enum IcAdminRequirement { - None, - Anonymous, // for get commands - Detect, // detect the neuron - OverridableBy { network: Network, neuron: AuthNeuron }, // eg automation which we know where is placed +#[derive(Clone)] +pub enum AuthRequirement { + Anonymous, // for get commands + Signer, // just authentication details used for signing + Neuron, // Signer + neuron_id used for proposals } #[derive(Debug, Display, Clone)] diff --git a/rs/cli/src/commands/neuron/balance.rs b/rs/cli/src/commands/neuron/balance.rs index e858ec03c..7b5a83721 100644 --- a/rs/cli/src/commands/neuron/balance.rs +++ b/rs/cli/src/commands/neuron/balance.rs @@ -11,10 +11,10 @@ pub struct Balance { } impl ExecutableCommand for Balance { - fn require_ic_admin(&self) -> crate::commands::IcAdminRequirement { + fn require_auth(&self) -> crate::commands::AuthRequirement { match &self.neuron { - Some(_) => crate::commands::IcAdminRequirement::None, - None => crate::commands::IcAdminRequirement::Detect, + Some(_) => crate::commands::AuthRequirement::Anonymous, + None => crate::commands::AuthRequirement::Neuron, } } @@ -22,9 +22,7 @@ impl ExecutableCommand for Balance { async fn execute(&self, ctx: crate::ctx::DreContext) -> anyhow::Result<()> { let governance = GovernanceCanisterWrapper::from(ctx.create_ic_agent_canister_client(None)?); - let neuron_info = governance - .get_neuron_info(self.neuron.unwrap_or_else(|| ctx.ic_admin().neuron().neuron_id)) - .await?; + let neuron_info = governance.get_neuron_info(self.neuron.unwrap_or_else(|| ctx.neuron().neuron_id)).await?; println!("{}", neuron_info.stake_e8s / 10_u64.pow(8)); Ok(()) diff --git a/rs/cli/src/commands/neuron/mod.rs b/rs/cli/src/commands/neuron/mod.rs index 2f72da54f..e923230d0 100644 --- a/rs/cli/src/commands/neuron/mod.rs +++ b/rs/cli/src/commands/neuron/mod.rs @@ -1,6 +1,6 @@ use clap::Args; -use super::{impl_executable_command_for_enums, ExecutableCommand, IcAdminRequirement}; +use super::{impl_executable_command_for_enums, AuthRequirement, ExecutableCommand}; use crate::commands::neuron::balance::Balance; use crate::commands::neuron::refresh::Refresh; use crate::commands::neuron::top_up::TopUp; @@ -18,8 +18,8 @@ pub struct Neuron { impl_executable_command_for_enums! { Balance, TopUp, Refresh } impl ExecutableCommand for Neuron { - fn require_ic_admin(&self) -> IcAdminRequirement { - self.subcommand.require_ic_admin() + fn require_auth(&self) -> AuthRequirement { + self.subcommand.require_auth() } fn validate(&self, cmd: &mut Command) { diff --git a/rs/cli/src/commands/neuron/refresh.rs b/rs/cli/src/commands/neuron/refresh.rs index 5388b638e..b239c9f5b 100644 --- a/rs/cli/src/commands/neuron/refresh.rs +++ b/rs/cli/src/commands/neuron/refresh.rs @@ -7,8 +7,8 @@ use crate::commands::ExecutableCommand; pub struct Refresh {} impl ExecutableCommand for Refresh { - fn require_ic_admin(&self) -> crate::commands::IcAdminRequirement { - crate::commands::IcAdminRequirement::Detect + fn require_auth(&self) -> crate::commands::AuthRequirement { + crate::commands::AuthRequirement::Neuron } fn validate(&self, _cmd: &mut clap::Command) {} @@ -16,7 +16,7 @@ impl ExecutableCommand for Refresh { async fn execute(&self, ctx: crate::ctx::DreContext) -> anyhow::Result<()> { let governance_canister = GovernanceCanisterWrapper::from(ctx.create_ic_agent_canister_client(None)?); - let resp = governance_canister.refresh_neuron(ctx.ic_admin().neuron().neuron_id).await?; + let resp = governance_canister.refresh_neuron(ctx.neuron().neuron_id).await?; println!("{:?}", resp); Ok(()) diff --git a/rs/cli/src/commands/neuron/top_up.rs b/rs/cli/src/commands/neuron/top_up.rs index 0f1d7ca79..4481e639c 100644 --- a/rs/cli/src/commands/neuron/top_up.rs +++ b/rs/cli/src/commands/neuron/top_up.rs @@ -8,22 +8,22 @@ use crate::commands::ExecutableCommand; pub struct TopUp {} impl ExecutableCommand for TopUp { - fn require_ic_admin(&self) -> crate::commands::IcAdminRequirement { - crate::commands::IcAdminRequirement::Detect + fn require_auth(&self) -> crate::commands::AuthRequirement { + crate::commands::AuthRequirement::Neuron } fn validate(&self, _cmd: &mut clap::Command) {} async fn execute(&self, ctx: crate::ctx::DreContext) -> anyhow::Result<()> { let governance = GovernanceCanisterWrapper::from(ctx.create_ic_agent_canister_client(None)?); - let full_neuron = governance.get_full_neuron(ctx.ic_admin().neuron().neuron_id).await?; + let full_neuron = governance.get_full_neuron(ctx.neuron().neuron_id).await?; let account_hex = full_neuron.account.iter().map(|byte| format!("{:02x}", byte)).join(""); println!("Please request ICP in the #icp-to-go slack channel:"); println!( "> Hi! Can I please get XX ICPs on the account address `{}` for neuron ID {} in order to be able to submit more NNS proposals. Thank you\n", account_hex, - ctx.ic_admin().neuron().neuron_id + ctx.neuron().neuron_id ); println!("You can check balance by running `dre neuron balance`"); diff --git a/rs/cli/src/commands/node_metrics.rs b/rs/cli/src/commands/node_metrics.rs index b1424a71e..b002b4323 100644 --- a/rs/cli/src/commands/node_metrics.rs +++ b/rs/cli/src/commands/node_metrics.rs @@ -15,7 +15,7 @@ use ic_types::{CanisterId, PrincipalId}; use itertools::Itertools; use log::{info, warn}; -use super::{ExecutableCommand, IcAdminRequirement}; +use super::{AuthRequirement, ExecutableCommand}; type CLINodeMetrics = BTreeMap>; @@ -125,8 +125,8 @@ impl NodeMetrics { } impl ExecutableCommand for NodeMetrics { - fn require_ic_admin(&self) -> IcAdminRequirement { - IcAdminRequirement::Detect + fn require_auth(&self) -> AuthRequirement { + AuthRequirement::Signer } async fn execute(&self, ctx: crate::ctx::DreContext) -> anyhow::Result<()> { diff --git a/rs/cli/src/commands/nodes/mod.rs b/rs/cli/src/commands/nodes/mod.rs index ae8e93bed..af5b43ead 100644 --- a/rs/cli/src/commands/nodes/mod.rs +++ b/rs/cli/src/commands/nodes/mod.rs @@ -1,7 +1,7 @@ use clap::Args; use remove::Remove; -use super::{impl_executable_command_for_enums, ExecutableCommand, IcAdminRequirement}; +use super::{impl_executable_command_for_enums, AuthRequirement, ExecutableCommand}; mod remove; @@ -13,8 +13,8 @@ pub struct Nodes { impl_executable_command_for_enums! { Remove } impl ExecutableCommand for Nodes { - fn require_ic_admin(&self) -> IcAdminRequirement { - self.subcommand.require_ic_admin() + fn require_auth(&self) -> AuthRequirement { + self.subcommand.require_auth() } async fn execute(&self, ctx: crate::ctx::DreContext) -> anyhow::Result<()> { diff --git a/rs/cli/src/commands/nodes/remove.rs b/rs/cli/src/commands/nodes/remove.rs index 8e049af4f..76edfcdec 100644 --- a/rs/cli/src/commands/nodes/remove.rs +++ b/rs/cli/src/commands/nodes/remove.rs @@ -1,7 +1,7 @@ use clap::{error::ErrorKind, Args}; use decentralization::subnets::NodesRemover; -use crate::commands::{ExecutableCommand, IcAdminRequirement}; +use crate::commands::{AuthRequirement, ExecutableCommand}; #[derive(Args, Debug)] pub struct Remove { @@ -26,12 +26,12 @@ pub struct Remove { } impl ExecutableCommand for Remove { - fn require_ic_admin(&self) -> IcAdminRequirement { - IcAdminRequirement::Detect + fn require_auth(&self) -> AuthRequirement { + AuthRequirement::Neuron } async fn execute(&self, ctx: crate::ctx::DreContext) -> anyhow::Result<()> { - let runner = ctx.runner().await; + let runner = ctx.runner().await?; runner .remove_nodes(NodesRemover { no_auto: self.no_auto, diff --git a/rs/cli/src/commands/proposals/analyze.rs b/rs/cli/src/commands/proposals/analyze.rs index 4f2bfdb43..a70ea7c61 100644 --- a/rs/cli/src/commands/proposals/analyze.rs +++ b/rs/cli/src/commands/proposals/analyze.rs @@ -4,7 +4,7 @@ use ic_management_types::filter_map_nns_function_proposals; use ic_nns_governance::pb::v1::ProposalStatus; use registry_canister::mutations::do_change_subnet_membership::ChangeSubnetMembershipPayload; -use crate::commands::{ExecutableCommand, IcAdminRequirement}; +use crate::commands::{AuthRequirement, ExecutableCommand}; #[derive(Args, Debug)] pub struct Analyze { @@ -13,8 +13,8 @@ pub struct Analyze { } impl ExecutableCommand for Analyze { - fn require_ic_admin(&self) -> IcAdminRequirement { - IcAdminRequirement::Anonymous + fn require_auth(&self) -> AuthRequirement { + AuthRequirement::Anonymous } async fn execute(&self, ctx: crate::ctx::DreContext) -> anyhow::Result<()> { @@ -30,7 +30,7 @@ impl ExecutableCommand for Analyze { )); } - let runner = ctx.runner().await; + let runner = ctx.runner().await?; match filter_map_nns_function_proposals::(&[proposal]).first() { Some((_, change_membership)) => runner.decentralization_change(change_membership, None).await, diff --git a/rs/cli/src/commands/proposals/filter.rs b/rs/cli/src/commands/proposals/filter.rs index 36ffd7922..8b6801be7 100644 --- a/rs/cli/src/commands/proposals/filter.rs +++ b/rs/cli/src/commands/proposals/filter.rs @@ -10,7 +10,7 @@ use std::fmt::Display; use clap::ValueEnum; use ic_nns_governance::pb::v1::{ListProposalInfo, ProposalStatus as ProposalStatusUpstream, Topic as TopicUpstream}; -use crate::commands::{proposals::Proposal, ExecutableCommand, IcAdminRequirement}; +use crate::commands::{proposals::Proposal, AuthRequirement, ExecutableCommand}; #[derive(Args, Debug)] pub struct Filter { /// Limit on the number of \[ProposalInfo\] to return. If value greater than @@ -221,8 +221,8 @@ impl From for Topic { } impl ExecutableCommand for Filter { - fn require_ic_admin(&self) -> IcAdminRequirement { - IcAdminRequirement::None + fn require_auth(&self) -> AuthRequirement { + AuthRequirement::Anonymous } async fn execute(&self, ctx: crate::ctx::DreContext) -> anyhow::Result<()> { diff --git a/rs/cli/src/commands/proposals/get.rs b/rs/cli/src/commands/proposals/get.rs index ed5ec6e1d..e51729513 100644 --- a/rs/cli/src/commands/proposals/get.rs +++ b/rs/cli/src/commands/proposals/get.rs @@ -1,7 +1,7 @@ use clap::Args; use ic_canisters::governance::GovernanceCanisterWrapper; -use crate::commands::{ExecutableCommand, IcAdminRequirement}; +use crate::commands::{AuthRequirement, ExecutableCommand}; #[derive(Args, Debug)] pub struct Get { @@ -10,8 +10,8 @@ pub struct Get { } impl ExecutableCommand for Get { - fn require_ic_admin(&self) -> IcAdminRequirement { - IcAdminRequirement::None + fn require_auth(&self) -> AuthRequirement { + AuthRequirement::Anonymous } async fn execute(&self, ctx: crate::ctx::DreContext) -> anyhow::Result<()> { diff --git a/rs/cli/src/commands/proposals/list.rs b/rs/cli/src/commands/proposals/list.rs index 34bc087d0..44152ffab 100644 --- a/rs/cli/src/commands/proposals/list.rs +++ b/rs/cli/src/commands/proposals/list.rs @@ -4,7 +4,7 @@ use ic_nns_common::pb::v1::ProposalId; use ic_nns_governance::pb::v1::ListProposalInfo; use itertools::Itertools; -use crate::commands::{ExecutableCommand, IcAdminRequirement}; +use crate::commands::{AuthRequirement, ExecutableCommand}; use super::Proposal; @@ -59,8 +59,8 @@ pub struct List { } impl ExecutableCommand for List { - fn require_ic_admin(&self) -> IcAdminRequirement { - IcAdminRequirement::None + fn require_auth(&self) -> AuthRequirement { + AuthRequirement::Anonymous } async fn execute(&self, ctx: crate::ctx::DreContext) -> anyhow::Result<()> { diff --git a/rs/cli/src/commands/proposals/mod.rs b/rs/cli/src/commands/proposals/mod.rs index 689ec2481..00dd67a6f 100644 --- a/rs/cli/src/commands/proposals/mod.rs +++ b/rs/cli/src/commands/proposals/mod.rs @@ -47,7 +47,7 @@ use registry_canister::mutations::{ }; use serde::{Deserialize, Serialize}; -use super::{impl_executable_command_for_enums, ExecutableCommand, IcAdminRequirement}; +use super::{impl_executable_command_for_enums, AuthRequirement, ExecutableCommand}; mod analyze; mod filter; @@ -65,8 +65,8 @@ pub struct Proposals { impl_executable_command_for_enums! { Pending, Get, Analyze, Filter, List } impl ExecutableCommand for Proposals { - fn require_ic_admin(&self) -> IcAdminRequirement { - self.subcommand.require_ic_admin() + fn require_auth(&self) -> AuthRequirement { + self.subcommand.require_auth() } async fn execute(&self, ctx: crate::ctx::DreContext) -> anyhow::Result<()> { diff --git a/rs/cli/src/commands/proposals/pending.rs b/rs/cli/src/commands/proposals/pending.rs index b33d96d59..9947409cb 100644 --- a/rs/cli/src/commands/proposals/pending.rs +++ b/rs/cli/src/commands/proposals/pending.rs @@ -1,14 +1,14 @@ use clap::Args; use ic_canisters::governance::GovernanceCanisterWrapper; -use crate::commands::{ExecutableCommand, IcAdminRequirement}; +use crate::commands::{AuthRequirement, ExecutableCommand}; #[derive(Args, Debug)] pub struct Pending {} impl ExecutableCommand for Pending { - fn require_ic_admin(&self) -> IcAdminRequirement { - IcAdminRequirement::None + fn require_auth(&self) -> AuthRequirement { + AuthRequirement::Anonymous } async fn execute(&self, ctx: crate::ctx::DreContext) -> anyhow::Result<()> { diff --git a/rs/cli/src/commands/propose.rs b/rs/cli/src/commands/propose.rs index c79401927..32e79a602 100644 --- a/rs/cli/src/commands/propose.rs +++ b/rs/cli/src/commands/propose.rs @@ -1,6 +1,6 @@ use clap::Args; -use super::{ExecutableCommand, IcAdminRequirement}; +use super::{AuthRequirement, ExecutableCommand}; #[derive(Args, Debug)] pub struct Propose { @@ -10,12 +10,12 @@ pub struct Propose { } impl ExecutableCommand for Propose { - fn require_ic_admin(&self) -> IcAdminRequirement { - IcAdminRequirement::Detect + fn require_auth(&self) -> AuthRequirement { + AuthRequirement::Neuron } async fn execute(&self, ctx: crate::ctx::DreContext) -> anyhow::Result<()> { - let ic_admin = ctx.ic_admin(); + let ic_admin = ctx.ic_admin().await?; ic_admin.run_passthrough_propose(&self.args).await?; Ok(()) diff --git a/rs/cli/src/commands/qualify/execute.rs b/rs/cli/src/commands/qualify/execute.rs index abaa2c6ec..7b8895e94 100644 --- a/rs/cli/src/commands/qualify/execute.rs +++ b/rs/cli/src/commands/qualify/execute.rs @@ -4,11 +4,7 @@ use clap::Args; use ic_management_types::Network; use serde_json::Value; -use crate::{ - commands::{ExecutableCommand, IcAdminRequirement}, - ic_admin::IcAdmin, - qualification::QualificationExecutorBuilder, -}; +use crate::{commands::ExecutableCommand, ic_admin::IcAdmin, qualification::QualificationExecutorBuilder}; #[derive(Args, Debug)] pub struct Execute { @@ -44,8 +40,8 @@ pub struct Execute { } impl ExecutableCommand for Execute { - fn require_ic_admin(&self) -> crate::commands::IcAdminRequirement { - IcAdminRequirement::Detect + fn require_auth(&self) -> crate::commands::AuthRequirement { + crate::commands::AuthRequirement::Neuron } fn validate(&self, cmd: &mut clap::Command) { @@ -66,7 +62,7 @@ impl ExecutableCommand for Execute { let from_version = match &self.from_version { Some(v) => v.to_string(), None => { - let anonymous_admin_wrapper_for_mainnet = ctx.readonly_ic_admin_for_other_network(Network::mainnet_unchecked().unwrap()); + let anonymous_admin_wrapper_for_mainnet = ctx.readonly_ic_admin_for_other_network(Network::mainnet_unchecked().unwrap()).await?; let subnets = ctx.registry().await.subnets().await?; let nns_subnet_id = subnets.keys().next().unwrap(); diff --git a/rs/cli/src/commands/qualify/list.rs b/rs/cli/src/commands/qualify/list.rs index 3f343a87e..48dc8671b 100644 --- a/rs/cli/src/commands/qualify/list.rs +++ b/rs/cli/src/commands/qualify/list.rs @@ -1,9 +1,6 @@ use clap::Args; -use crate::{ - commands::{ExecutableCommand, IcAdminRequirement}, - qualification::QualificationExecutorBuilder, -}; +use crate::{commands::ExecutableCommand, qualification::QualificationExecutorBuilder}; #[derive(Args, Debug)] pub struct List { @@ -14,8 +11,8 @@ pub struct List { } impl ExecutableCommand for List { - fn require_ic_admin(&self) -> crate::commands::IcAdminRequirement { - IcAdminRequirement::None + fn require_auth(&self) -> crate::commands::AuthRequirement { + crate::commands::AuthRequirement::Anonymous } fn validate(&self, _cmd: &mut clap::Command) {} diff --git a/rs/cli/src/commands/qualify/mod.rs b/rs/cli/src/commands/qualify/mod.rs index 8ad2b212a..e652caf71 100644 --- a/rs/cli/src/commands/qualify/mod.rs +++ b/rs/cli/src/commands/qualify/mod.rs @@ -1,9 +1,8 @@ -use crate::commands::IcAdminRequirement; use clap::Args; use execute::Execute; use list::List; -use super::{impl_executable_command_for_enums, ExecutableCommand}; +use super::{impl_executable_command_for_enums, AuthRequirement, ExecutableCommand}; pub mod execute; mod list; @@ -17,8 +16,8 @@ pub struct Qualify { impl_executable_command_for_enums! { List, Execute } impl ExecutableCommand for Qualify { - fn require_ic_admin(&self) -> super::IcAdminRequirement { - self.subcommand.require_ic_admin() + fn require_auth(&self) -> AuthRequirement { + self.subcommand.require_auth() } fn validate(&self, cmd: &mut clap::Command) { diff --git a/rs/cli/src/commands/registry.rs b/rs/cli/src/commands/registry.rs index 28b62cdb3..04c337b8f 100644 --- a/rs/cli/src/commands/registry.rs +++ b/rs/cli/src/commands/registry.rs @@ -22,7 +22,7 @@ use serde::Serialize; use crate::ctx::DreContext; -use super::{ExecutableCommand, IcAdminRequirement}; +use super::{AuthRequirement, ExecutableCommand}; #[derive(Args, Debug)] pub struct Registry { @@ -36,8 +36,8 @@ pub struct Registry { } impl ExecutableCommand for Registry { - fn require_ic_admin(&self) -> IcAdminRequirement { - IcAdminRequirement::None + fn require_auth(&self) -> AuthRequirement { + AuthRequirement::Anonymous } async fn execute(&self, ctx: DreContext) -> anyhow::Result<()> { diff --git a/rs/cli/src/commands/subnet/create.rs b/rs/cli/src/commands/subnet/create.rs index 36c7922df..4cbb92911 100644 --- a/rs/cli/src/commands/subnet/create.rs +++ b/rs/cli/src/commands/subnet/create.rs @@ -2,7 +2,7 @@ use clap::{error::ErrorKind, Args}; use ic_management_types::requests::SubnetCreateRequest; use ic_types::PrincipalId; -use crate::commands::{ExecutableCommand, IcAdminRequirement}; +use crate::commands::{AuthRequirement, ExecutableCommand}; #[derive(Args, Debug)] pub struct Create { @@ -44,12 +44,12 @@ regardless of the decentralization coefficients"#)] } impl ExecutableCommand for Create { - fn require_ic_admin(&self) -> IcAdminRequirement { - IcAdminRequirement::Detect + fn require_auth(&self) -> AuthRequirement { + AuthRequirement::Neuron } async fn execute(&self, ctx: crate::ctx::DreContext) -> anyhow::Result<()> { - let runner = ctx.runner().await; + let runner = ctx.runner().await?; let motivation = match &self.motivation { Some(m) => m, None if self.help_other_args => &"help for options".to_string(), diff --git a/rs/cli/src/commands/subnet/deploy.rs b/rs/cli/src/commands/subnet/deploy.rs index 0bba6c639..15fd70556 100644 --- a/rs/cli/src/commands/subnet/deploy.rs +++ b/rs/cli/src/commands/subnet/deploy.rs @@ -1,7 +1,7 @@ use clap::Args; use ic_types::PrincipalId; -use crate::commands::{ExecutableCommand, IcAdminRequirement}; +use crate::commands::{AuthRequirement, ExecutableCommand}; #[derive(Args, Debug)] pub struct Deploy { @@ -15,12 +15,12 @@ pub struct Deploy { } impl ExecutableCommand for Deploy { - fn require_ic_admin(&self) -> IcAdminRequirement { - IcAdminRequirement::Detect + fn require_auth(&self) -> AuthRequirement { + AuthRequirement::Neuron } async fn execute(&self, ctx: crate::ctx::DreContext) -> anyhow::Result<()> { - let runner = ctx.runner().await; + let runner = ctx.runner().await?; runner.deploy(&self.id, &self.version, ctx.forum_post_link()).await } diff --git a/rs/cli/src/commands/subnet/mod.rs b/rs/cli/src/commands/subnet/mod.rs index 813ee958f..06bafdca9 100644 --- a/rs/cli/src/commands/subnet/mod.rs +++ b/rs/cli/src/commands/subnet/mod.rs @@ -6,7 +6,7 @@ use rescue::Rescue; use resize::Resize; use whatif::WhatifDecentralization; -use super::{impl_executable_command_for_enums, ExecutableCommand, IcAdminRequirement}; +use super::{impl_executable_command_for_enums, AuthRequirement, ExecutableCommand}; mod create; mod deploy; @@ -24,8 +24,8 @@ pub struct Subnet { impl_executable_command_for_enums! { WhatifDecentralization, Deploy, Replace, Resize, Create, Rescue } impl ExecutableCommand for Subnet { - fn require_ic_admin(&self) -> IcAdminRequirement { - self.subcommand.require_ic_admin() + fn require_auth(&self) -> AuthRequirement { + self.subcommand.require_auth() } async fn execute(&self, ctx: crate::ctx::DreContext) -> anyhow::Result<()> { diff --git a/rs/cli/src/commands/subnet/replace.rs b/rs/cli/src/commands/subnet/replace.rs index 678e58f07..eb6ea0fe2 100644 --- a/rs/cli/src/commands/subnet/replace.rs +++ b/rs/cli/src/commands/subnet/replace.rs @@ -2,7 +2,7 @@ use clap::{error::ErrorKind, Args}; use ic_types::PrincipalId; use crate::{ - commands::{ExecutableCommand, IcAdminRequirement}, + commands::{AuthRequirement, ExecutableCommand}, subnet_manager::SubnetTarget, }; @@ -46,8 +46,8 @@ pub struct Replace { } impl ExecutableCommand for Replace { - fn require_ic_admin(&self) -> IcAdminRequirement { - IcAdminRequirement::Detect + fn require_auth(&self) -> AuthRequirement { + AuthRequirement::Neuron } async fn execute(&self, ctx: crate::ctx::DreContext) -> anyhow::Result<()> { @@ -70,7 +70,7 @@ impl ExecutableCommand for Replace { ) .await?; - let runner = ctx.runner().await; + let runner = ctx.runner().await?; runner.propose_subnet_change(subnet_change_response, ctx.forum_post_link()).await } diff --git a/rs/cli/src/commands/subnet/rescue.rs b/rs/cli/src/commands/subnet/rescue.rs index 709710843..1e20b394c 100644 --- a/rs/cli/src/commands/subnet/rescue.rs +++ b/rs/cli/src/commands/subnet/rescue.rs @@ -1,7 +1,7 @@ use clap::Args; use ic_types::PrincipalId; -use crate::commands::{ExecutableCommand, IcAdminRequirement}; +use crate::commands::{AuthRequirement, ExecutableCommand}; #[derive(Args, Debug)] pub struct Rescue { @@ -15,12 +15,12 @@ pub struct Rescue { } impl ExecutableCommand for Rescue { - fn require_ic_admin(&self) -> IcAdminRequirement { - IcAdminRequirement::Detect + fn require_auth(&self) -> AuthRequirement { + AuthRequirement::Neuron } async fn execute(&self, ctx: crate::ctx::DreContext) -> anyhow::Result<()> { - let runner = ctx.runner().await; + let runner = ctx.runner().await?; runner.subnet_rescue(&self.id, self.keep_nodes.clone(), ctx.forum_post_link()).await } diff --git a/rs/cli/src/commands/subnet/resize.rs b/rs/cli/src/commands/subnet/resize.rs index c5de1e6c8..0a50c7d40 100644 --- a/rs/cli/src/commands/subnet/resize.rs +++ b/rs/cli/src/commands/subnet/resize.rs @@ -2,7 +2,7 @@ use clap::Args; use ic_management_types::requests::SubnetResizeRequest; use ic_types::PrincipalId; -use crate::commands::{ExecutableCommand, IcAdminRequirement}; +use crate::commands::{AuthRequirement, ExecutableCommand}; #[derive(Args, Debug)] pub struct Resize { @@ -37,12 +37,12 @@ pub struct Resize { } impl ExecutableCommand for Resize { - fn require_ic_admin(&self) -> IcAdminRequirement { - IcAdminRequirement::Detect + fn require_auth(&self) -> AuthRequirement { + AuthRequirement::Neuron } async fn execute(&self, ctx: crate::ctx::DreContext) -> anyhow::Result<()> { - let runner = ctx.runner().await; + let runner = ctx.runner().await?; runner .subnet_resize( SubnetResizeRequest { diff --git a/rs/cli/src/commands/subnet/whatif.rs b/rs/cli/src/commands/subnet/whatif.rs index a2fb0be71..24bad9700 100644 --- a/rs/cli/src/commands/subnet/whatif.rs +++ b/rs/cli/src/commands/subnet/whatif.rs @@ -2,7 +2,7 @@ use clap::Args; use ic_types::PrincipalId; use registry_canister::mutations::do_change_subnet_membership::ChangeSubnetMembershipPayload; -use crate::commands::{ExecutableCommand, IcAdminRequirement}; +use crate::commands::{AuthRequirement, ExecutableCommand}; #[derive(Args, Debug)] #[clap(visible_aliases = &["analyze", "analyze-decentralization", "decentralization", "whatif", "what-if"])] @@ -24,12 +24,12 @@ pub struct WhatifDecentralization { } impl ExecutableCommand for WhatifDecentralization { - fn require_ic_admin(&self) -> IcAdminRequirement { - IcAdminRequirement::Anonymous + fn require_auth(&self) -> AuthRequirement { + AuthRequirement::Anonymous } async fn execute(&self, ctx: crate::ctx::DreContext) -> anyhow::Result<()> { - let runner = ctx.runner().await; + let runner = ctx.runner().await?; let change_membership = ChangeSubnetMembershipPayload { subnet_id: self.subnet_id, diff --git a/rs/cli/src/commands/update_authorized_subnets.rs b/rs/cli/src/commands/update_authorized_subnets.rs index 57fa8c4b7..af1b1fb40 100644 --- a/rs/cli/src/commands/update_authorized_subnets.rs +++ b/rs/cli/src/commands/update_authorized_subnets.rs @@ -9,7 +9,7 @@ use log::info; use crate::ic_admin::{ProposeCommand, ProposeOptions}; -use super::ExecutableCommand; +use super::{AuthRequirement, ExecutableCommand}; const DEFAULT_CANISTER_LIMIT: u64 = 60_000; const DEFAULT_STATE_SIZE_BYTES_LIMIT: u64 = 322_122_547_200; // 300GB @@ -32,8 +32,8 @@ pub struct UpdateAuthorizedSubnets { } impl ExecutableCommand for UpdateAuthorizedSubnets { - fn require_ic_admin(&self) -> super::IcAdminRequirement { - super::IcAdminRequirement::Detect + fn require_auth(&self) -> AuthRequirement { + super::AuthRequirement::Neuron } fn validate(&self, cmd: &mut clap::Command) { @@ -95,7 +95,7 @@ impl ExecutableCommand for UpdateAuthorizedSubnets { .cloned() .collect(); - let ic_admin = ctx.ic_admin(); + let ic_admin = ctx.ic_admin().await?; ic_admin .propose_run( ProposeCommand::SetAuthorizedSubnetworks { subnets: authorized }, diff --git a/rs/cli/src/commands/update_unassigned_nodes.rs b/rs/cli/src/commands/update_unassigned_nodes.rs index 8be9bd96b..6aad5c2b3 100644 --- a/rs/cli/src/commands/update_unassigned_nodes.rs +++ b/rs/cli/src/commands/update_unassigned_nodes.rs @@ -2,12 +2,9 @@ use std::str::FromStr; use clap::Args; use ic_canisters::registry::RegistryCanisterWrapper; -use ic_management_types::Network; use ic_types::PrincipalId; -use crate::auth::Neuron; - -use super::{ExecutableCommand, IcAdminRequirement}; +use super::{AuthRequirement, ExecutableCommand}; #[derive(Args, Debug)] pub struct UpdateUnassignedNodes { @@ -17,16 +14,11 @@ pub struct UpdateUnassignedNodes { } impl ExecutableCommand for UpdateUnassignedNodes { - fn require_ic_admin(&self) -> IcAdminRequirement { - IcAdminRequirement::OverridableBy { - network: Network::mainnet_unchecked().unwrap(), - neuron: Neuron::automation_neuron_unchecked(), - } + fn require_auth(&self) -> AuthRequirement { + AuthRequirement::Neuron } async fn execute(&self, ctx: crate::ctx::DreContext) -> anyhow::Result<()> { - let runner = ctx.runner().await; - let nns_subnet_id = match &self.nns_subnet_id { Some(n) => n.to_owned(), None => { @@ -41,6 +33,7 @@ impl ExecutableCommand for UpdateUnassignedNodes { } }; + let runner = ctx.runner().await?; runner .update_unassigned_nodes(&PrincipalId::from_str(&nns_subnet_id)?, ctx.forum_post_link()) .await diff --git a/rs/cli/src/commands/upgrade.rs b/rs/cli/src/commands/upgrade.rs index d20257cb9..f00c1fca3 100644 --- a/rs/cli/src/commands/upgrade.rs +++ b/rs/cli/src/commands/upgrade.rs @@ -4,7 +4,7 @@ use regex::Regex; use serde_json::Value; use tokio::task::JoinHandle; -use super::{ExecutableCommand, IcAdminRequirement}; +use super::{AuthRequirement, ExecutableCommand}; #[derive(Args, Debug, Default)] pub struct Upgrade { @@ -147,8 +147,8 @@ pub enum UpdateStatus { } impl ExecutableCommand for Upgrade { - fn require_ic_admin(&self) -> IcAdminRequirement { - IcAdminRequirement::None + fn require_auth(&self) -> AuthRequirement { + AuthRequirement::Anonymous } async fn execute(&self, _ctx: crate::ctx::DreContext) -> anyhow::Result<()> { diff --git a/rs/cli/src/commands/version/mod.rs b/rs/cli/src/commands/version/mod.rs index b00b35741..d8c214f4a 100644 --- a/rs/cli/src/commands/version/mod.rs +++ b/rs/cli/src/commands/version/mod.rs @@ -1,6 +1,6 @@ use clap::Args; -use super::{impl_executable_command_for_enums, ExecutableCommand, IcAdminRequirement}; +use super::{impl_executable_command_for_enums, AuthRequirement, ExecutableCommand}; use crate::commands::version::revise::ReviseElectedVersions; pub(crate) mod revise; @@ -14,8 +14,8 @@ pub struct Version { impl_executable_command_for_enums! { ReviseElectedVersions } impl ExecutableCommand for Version { - fn require_ic_admin(&self) -> IcAdminRequirement { - self.subcommand.require_ic_admin() + fn require_auth(&self) -> AuthRequirement { + self.subcommand.require_auth() } async fn execute(&self, ctx: crate::ctx::DreContext) -> anyhow::Result<()> { diff --git a/rs/cli/src/commands/version/revise/guest_os.rs b/rs/cli/src/commands/version/revise/guest_os.rs index 7a8dbf9be..76259d8ef 100644 --- a/rs/cli/src/commands/version/revise/guest_os.rs +++ b/rs/cli/src/commands/version/revise/guest_os.rs @@ -1,6 +1,6 @@ use clap::Args; -use crate::commands::{ExecutableCommand, IcAdminRequirement}; +use crate::commands::{AuthRequirement, ExecutableCommand}; #[derive(Debug, Args)] pub struct GuestOs { @@ -22,12 +22,12 @@ pub struct GuestOs { } impl ExecutableCommand for GuestOs { - fn require_ic_admin(&self) -> IcAdminRequirement { - IcAdminRequirement::Detect + fn require_auth(&self) -> AuthRequirement { + AuthRequirement::Neuron } async fn execute(&self, ctx: crate::ctx::DreContext) -> anyhow::Result<()> { - let runner = ctx.runner().await; + let runner = ctx.runner().await?; runner .do_revise_elected_replica_versions( &ic_management_types::Artifact::GuestOs, diff --git a/rs/cli/src/commands/version/revise/host_os.rs b/rs/cli/src/commands/version/revise/host_os.rs index ef52dfa41..5b5fbe2dd 100644 --- a/rs/cli/src/commands/version/revise/host_os.rs +++ b/rs/cli/src/commands/version/revise/host_os.rs @@ -1,6 +1,6 @@ use clap::Args; -use crate::commands::{ExecutableCommand, IcAdminRequirement}; +use crate::commands::{AuthRequirement, ExecutableCommand}; #[derive(Debug, Args)] pub struct HostOs { @@ -22,12 +22,12 @@ pub struct HostOs { } impl ExecutableCommand for HostOs { - fn require_ic_admin(&self) -> IcAdminRequirement { - IcAdminRequirement::Detect + fn require_auth(&self) -> AuthRequirement { + AuthRequirement::Neuron } async fn execute(&self, ctx: crate::ctx::DreContext) -> anyhow::Result<()> { - let runner = ctx.runner().await; + let runner = ctx.runner().await?; runner .do_revise_elected_replica_versions( &ic_management_types::Artifact::HostOs, diff --git a/rs/cli/src/commands/version/revise/mod.rs b/rs/cli/src/commands/version/revise/mod.rs index 260caf1fa..87a50661e 100644 --- a/rs/cli/src/commands/version/revise/mod.rs +++ b/rs/cli/src/commands/version/revise/mod.rs @@ -1,6 +1,7 @@ -use super::{impl_executable_command_for_enums, ExecutableCommand, IcAdminRequirement}; +use super::{impl_executable_command_for_enums, ExecutableCommand}; use crate::commands::version::revise::guest_os::GuestOs; use crate::commands::version::revise::host_os::HostOs; +use crate::commands::AuthRequirement; use clap::Args; use ic_management_types::Artifact; @@ -25,8 +26,8 @@ impl From for Artifact { } impl ExecutableCommand for ReviseElectedVersions { - fn require_ic_admin(&self) -> IcAdminRequirement { - self.subcommand.require_ic_admin() + fn require_auth(&self) -> AuthRequirement { + self.subcommand.require_auth() } async fn execute(&self, ctx: crate::ctx::DreContext) -> anyhow::Result<()> { diff --git a/rs/cli/src/commands/vote.rs b/rs/cli/src/commands/vote.rs index 4fcc4bfaf..64bcc15b0 100644 --- a/rs/cli/src/commands/vote.rs +++ b/rs/cli/src/commands/vote.rs @@ -7,7 +7,7 @@ use ic_nns_governance::pb::v1::ProposalInfo; use log::info; use spinners::{Spinner, Spinners}; -use super::{ExecutableCommand, IcAdminRequirement}; +use super::{AuthRequirement, ExecutableCommand}; use crate::desktop_notify::DesktopNotifier; #[derive(Args, Debug)] @@ -42,8 +42,8 @@ pub struct Vote { } impl ExecutableCommand for Vote { - fn require_ic_admin(&self) -> IcAdminRequirement { - IcAdminRequirement::Detect + fn require_auth(&self) -> AuthRequirement { + AuthRequirement::Neuron } async fn execute(&self, ctx: crate::ctx::DreContext) -> anyhow::Result<()> { @@ -85,7 +85,7 @@ impl ExecutableCommand for Vote { let prop_id = proposal.id.unwrap().id; if !ctx.is_dry_run() { - let response = match client.register_vote(ctx.ic_admin().neuron().neuron_id, proposal.id.unwrap().id).await { + let response = match client.register_vote(ctx.neuron().neuron_id, proposal.id.unwrap().id).await { Ok(response) => format!("Voted successfully: {}", response), Err(e) => { DesktopNotifier::send_critical( diff --git a/rs/cli/src/ctx.rs b/rs/cli/src/ctx.rs index f2987e050..7576b0123 100644 --- a/rs/cli/src/ctx.rs +++ b/rs/cli/src/ctx.rs @@ -1,8 +1,6 @@ use std::{ cell::RefCell, - path::PathBuf, rc::Rc, - str::FromStr, sync::{Arc, Mutex}, time::Duration, }; @@ -21,41 +19,42 @@ use url::Url; use crate::{ artifact_downloader::{ArtifactDownloader, ArtifactDownloaderImpl}, - auth::{Auth, Neuron}, - commands::{Args, ExecutableCommand, IcAdminRequirement, IcAdminVersion}, + auth::Neuron, + commands::{Args, AuthOpts, AuthRequirement, ExecutableCommand, IcAdminVersion}, ic_admin::{download_ic_admin, should_update_ic_admin, IcAdmin, IcAdminImpl, FALLBACK_IC_ADMIN_VERSION}, runner::Runner, subnet_manager::SubnetManager, }; -const STAGING_NEURON_ID: u64 = 49; #[derive(Clone)] pub struct DreContext { network: Network, registry: RefCell>>, - ic_admin: Option>, + ic_admin: RefCell>>, runner: RefCell>>, ic_repo: RefCell>>, proposal_agent: Arc, verbose_runner: bool, skip_sync: bool, - ic_admin_path: Option, forum_post_link: Option, dry_run: bool, artifact_downloader: Arc, + neuron: Neuron, + proceed_without_confirmation: bool, + version: IcAdminVersion, } impl DreContext { pub async fn new( network: String, nns_urls: Vec, - auth: Auth, + auth: AuthOpts, neuron_id: Option, verbose: bool, no_sync: bool, yes: bool, dry_run: bool, - ic_admin_requirement: IcAdminRequirement, + auth_requirement: AuthRequirement, forum_post_link: Option, ic_admin_version: IcAdminVersion, ) -> anyhow::Result { @@ -66,42 +65,23 @@ impl DreContext { true => Network::new_unchecked(network.clone(), &nns_urls)?, }; - // Overrides of neuron ID and private key PEM file for staging. - // Appropriate fallbacks take place when options are missing. - // I personally don't think this should be here, but more refactoring - // needs to take place for this code to reach its final destination. - let (neuron_id, auth_opts) = if network.name == "staging" { - let staging_path = PathBuf::from_str(&std::env::var("HOME")?)?.join(".config/dfx/identity/bootstrap-super-leader/identity.pem"); - ( - neuron_id.or(Some(STAGING_NEURON_ID)), - match (&auth, Auth::pem(staging_path).await) { - // There is no private key PEM specified, this is staging, the user - // did not specify HSM options, and the default staging path exists, - // so we use the default staging path. - (Auth::Anonymous, Ok(staging_pem_auth)) => staging_pem_auth, - _ => auth.clone(), - }, - ) - } else { - (neuron_id, auth.clone()) - }; - - let (ic_admin, ic_admin_path) = - Self::init_ic_admin(&network, neuron_id, auth_opts, yes, dry_run, ic_admin_requirement, ic_admin_version).await?; + let neuron = Neuron::from_opts_and_req(auth, auth_requirement, &network, neuron_id).await?; Ok(Self { proposal_agent: Arc::new(ProposalAgentImpl::new(&network.nns_urls)), network, registry: RefCell::new(None), - ic_admin, + ic_admin: RefCell::new(None), runner: RefCell::new(None), verbose_runner: verbose, skip_sync: no_sync, - ic_admin_path, forum_post_link: forum_post_link.clone(), ic_repo: RefCell::new(None), dry_run, artifact_downloader: Arc::new(ArtifactDownloaderImpl {}) as Arc, + neuron, + proceed_without_confirmation: yes, + version: ic_admin_version, }) } @@ -109,99 +89,19 @@ impl DreContext { Self::new( args.network.clone(), args.nns_urls.clone(), - match args.subcommands.require_ic_admin() { - IcAdminRequirement::None | IcAdminRequirement::Anonymous => Auth::Anonymous, - IcAdminRequirement::Detect | IcAdminRequirement::OverridableBy { network: _, neuron: _ } => { - Auth::from_auth_opts(args.auth_opts.clone()).await? - } - }, + args.auth_opts.clone(), args.neuron_id, args.verbose, args.no_sync, args.yes, args.dry_run, - args.subcommands.require_ic_admin(), + args.subcommands.require_auth(), args.forum_post_link.clone(), args.ic_admin_version.clone(), ) .await } - async fn init_ic_admin( - network: &Network, - neuron_id: Option, - auth: Auth, - proceed_without_confirmation: bool, - dry_run: bool, - requirement: IcAdminRequirement, - version: IcAdminVersion, - ) -> anyhow::Result<(Option>, Option)> { - if let IcAdminRequirement::None = requirement { - return Ok((None, None)); - } - let neuron = match requirement { - IcAdminRequirement::Anonymous | IcAdminRequirement::None => Neuron { - auth: crate::auth::Auth::Anonymous, - neuron_id: 0, - include_proposer: false, - }, - IcAdminRequirement::Detect => Neuron::new(auth, neuron_id, network, true).await?, - IcAdminRequirement::OverridableBy { - network: accepted_network, - neuron, - } => { - let maybe_neuron = Neuron::new(auth, neuron_id, network, true).await; - - match maybe_neuron { - Ok(n) => n, - Err(_) if accepted_network == *network => neuron, - Err(e) => return Err(e), - } - } - }; - - let ic_admin_path = match version { - IcAdminVersion::FromGovernance => match should_update_ic_admin()? { - (true, _) => { - let govn_canister_version = governance_canister_version(network.get_nns_urls()).await?; - debug!( - "Using ic-admin matching the version of governance canister, version: {}", - govn_canister_version.stringified_hash - ); - download_ic_admin(match govn_canister_version.stringified_hash.as_str() { - // Some testnets could have this version setup if deployed - // from HEAD of the branch they are created from - "0000000000000000000000000000000000000000" => None, - v => Some(v.to_owned()), - }) - .await? - } - (false, s) => { - debug!("Using cached ic-admin matching the version of governance canister, path: {}", s); - s - } - }, - IcAdminVersion::Fallback => { - debug!("Using default ic-admin, version: {}", FALLBACK_IC_ADMIN_VERSION); - download_ic_admin(None).await? - } - IcAdminVersion::Strict(ver) => { - debug!("Using ic-admin specified via args: {}", ver); - download_ic_admin(Some(ver)).await? - } - }; - - let ic_admin = Some(Arc::new(IcAdminImpl::new( - network.clone(), - Some(ic_admin_path.clone()), - proceed_without_confirmation, - neuron, - dry_run, - )) as Arc); - - Ok((ic_admin, Some(ic_admin_path))) - } - pub async fn registry(&self) -> Arc { if let Some(reg) = self.registry.borrow().as_ref() { return reg.clone(); @@ -235,22 +135,70 @@ impl DreContext { /// Uses `ic_agent::Agent` pub fn create_ic_agent_canister_client(&self, lock: Option>) -> anyhow::Result { - let urls = self.network.get_nns_urls().to_vec(); - match &self.ic_admin { - Some(a) => a.neuron().auth.create_canister_client(urls, lock), - None => IcAgentCanisterClient::from_anonymous(urls.first().expect("Should have at least one NNS url").clone()), - } + self.neuron.auth.create_canister_client(self.network.get_nns_urls().to_vec(), lock) } - pub fn ic_admin(&self) -> Arc { - match &self.ic_admin { - Some(a) => a.clone(), - None => panic!("This command is not configured to use ic admin"), + pub async fn ic_admin(&self) -> anyhow::Result> { + if let Some(a) = self.ic_admin.borrow().as_ref() { + return Ok(a.clone()); } + + let ic_admin_path = match &self.version { + IcAdminVersion::FromGovernance => match should_update_ic_admin()? { + (true, _) => { + let govn_canister_version = governance_canister_version(self.network().get_nns_urls()).await?; + debug!( + "Using ic-admin matching the version of governance canister, version: {}", + govn_canister_version.stringified_hash + ); + download_ic_admin(match govn_canister_version.stringified_hash.as_str() { + // Some testnets could have this version setup if deployed + // from HEAD of the branch they are created from + "0000000000000000000000000000000000000000" => None, + v => Some(v.to_owned()), + }) + .await? + } + (false, s) => { + debug!("Using cached ic-admin matching the version of governance canister, path: {}", s); + s + } + }, + IcAdminVersion::Fallback => { + debug!("Using default ic-admin, version: {}", FALLBACK_IC_ADMIN_VERSION); + download_ic_admin(None).await? + } + IcAdminVersion::Strict(ver) => { + debug!("Using ic-admin specified via args: {}", ver); + download_ic_admin(Some(ver.to_string())).await? + } + }; + + let ic_admin = Arc::new(IcAdminImpl::new( + self.network().clone(), + Some(ic_admin_path.clone()), + self.proceed_without_confirmation, + self.neuron(), + self.dry_run, + )) as Arc; + + *self.ic_admin.borrow_mut() = Some(ic_admin.clone()); + Ok(ic_admin) } - pub fn readonly_ic_admin_for_other_network(&self, network: Network) -> impl IcAdmin { - IcAdminImpl::new(network, self.ic_admin_path.clone(), true, Neuron::anonymous_neuron(), false) + pub fn neuron(&self) -> Neuron { + self.neuron.clone() + } + + pub async fn readonly_ic_admin_for_other_network(&self, network: Network) -> anyhow::Result { + let ic_admin = self.ic_admin().await?; + Ok(IcAdminImpl::new( + network, + ic_admin.ic_admin_path(), + true, + Neuron::anonymous_neuron(), + false, + )) } pub async fn subnet_manager(&self) -> SubnetManager { @@ -263,13 +211,13 @@ impl DreContext { self.proposal_agent.clone() } - pub async fn runner(&self) -> Rc { + pub async fn runner(&self) -> anyhow::Result> { if let Some(r) = self.runner.borrow().as_ref() { - return r.clone(); + return Ok(r.clone()); } let runner = Rc::new(Runner::new( - self.ic_admin(), + self.ic_admin().await?, self.registry().await, self.network().clone(), self.proposals_agent(), @@ -278,17 +226,12 @@ impl DreContext { self.artifact_downloader.clone(), )); *self.runner.borrow_mut() = Some(runner.clone()); - runner + Ok(runner) } pub fn forum_post_link(&self) -> Option { self.forum_post_link.clone() } - - #[cfg(test)] - pub fn ic_admin_path(&self) -> Option { - self.ic_admin_path.clone() - } } #[cfg(test)] @@ -299,12 +242,13 @@ pub mod tests { use ic_management_backend::{lazy_git::LazyGit, lazy_registry::LazyRegistry, proposal::ProposalAgent}; use ic_management_types::Network; - use crate::{artifact_downloader::ArtifactDownloader, ic_admin::IcAdmin}; + use crate::{artifact_downloader::ArtifactDownloader, auth::Neuron, ic_admin::IcAdmin}; use super::DreContext; pub fn get_mocked_ctx( network: Network, + neuron: Neuron, registry: Arc, ic_admin: Arc, git: Arc, @@ -314,16 +258,18 @@ pub mod tests { DreContext { network, registry: RefCell::new(Some(registry)), - ic_admin: Some(ic_admin), + ic_admin: RefCell::new(Some(ic_admin)), runner: RefCell::new(None), ic_repo: RefCell::new(Some(git)), proposal_agent, verbose_runner: true, skip_sync: false, - ic_admin_path: None, forum_post_link: None, dry_run: true, artifact_downloader, + neuron, + proceed_without_confirmation: true, + version: crate::commands::IcAdminVersion::Strict("Shouldn't reach this because of mock".to_string()), } } } diff --git a/rs/cli/src/ic_admin.rs b/rs/cli/src/ic_admin.rs index d7a7b59c0..74bb368a3 100644 --- a/rs/cli/src/ic_admin.rs +++ b/rs/cli/src/ic_admin.rs @@ -22,7 +22,7 @@ const MAX_SUMMARY_CHAR_COUNT: usize = 29000; #[automock] pub trait IcAdmin: Send + Sync { - fn neuron(&self) -> Neuron; + fn ic_admin_path(&self) -> Option; fn propose_run(&self, cmd: ProposeCommand, opts: ProposeOptions) -> BoxFuture<'_, anyhow::Result>; @@ -45,8 +45,8 @@ pub struct IcAdminImpl { } impl IcAdmin for IcAdminImpl { - fn neuron(&self) -> Neuron { - self.neuron.clone() + fn ic_admin_path(&self) -> Option { + self.ic_admin_bin_path.clone() } fn propose_run(&self, cmd: ProposeCommand, opts: ProposeOptions) -> BoxFuture<'_, anyhow::Result> { diff --git a/rs/cli/src/qualification/ensure_blessed_versions.rs b/rs/cli/src/qualification/ensure_blessed_versions.rs index 767e86b7b..36679bcd5 100644 --- a/rs/cli/src/qualification/ensure_blessed_versions.rs +++ b/rs/cli/src/qualification/ensure_blessed_versions.rs @@ -35,6 +35,7 @@ impl Step for EnsureBlessedRevisions { let place_proposal = || async { ctx.dre_ctx() .ic_admin() + .await? .propose_run( ProposeCommand::ReviseElectedVersions { release_artifact: ic_management_types::Artifact::GuestOs, diff --git a/rs/cli/src/qualification/retire_blessed_versions.rs b/rs/cli/src/qualification/retire_blessed_versions.rs index 5303b276a..e275da3cd 100644 --- a/rs/cli/src/qualification/retire_blessed_versions.rs +++ b/rs/cli/src/qualification/retire_blessed_versions.rs @@ -37,6 +37,7 @@ impl Step for RetireBlessedVersions { let place_proposal = || async { ctx.dre_ctx() .ic_admin() + .await? .propose_run( ProposeCommand::ReviseElectedVersions { release_artifact: ic_management_types::Artifact::GuestOs, diff --git a/rs/cli/src/qualification/upgrade_subnets.rs b/rs/cli/src/qualification/upgrade_subnets.rs index e3fd0e428..2d4f5cebc 100644 --- a/rs/cli/src/qualification/upgrade_subnets.rs +++ b/rs/cli/src/qualification/upgrade_subnets.rs @@ -90,6 +90,7 @@ impl Step for UpgradeSubnets { let place_proposal = || async { ctx.dre_ctx() .ic_admin() + .await? .propose_run( ProposeCommand::DeployGuestosToAllSubnetNodes { subnet: subnet.principal, @@ -133,6 +134,7 @@ impl Step for UpgradeSubnets { let place_proposal = || async { ctx.dre_ctx() .ic_admin() + .await? .propose_run( ProposeCommand::DeployGuestosToAllUnassignedNodes { replica_version: self.to_version.clone(), diff --git a/rs/cli/src/unit_tests/ctx_init.rs b/rs/cli/src/unit_tests/ctx_init.rs index 1277d6aa7..825607e88 100644 --- a/rs/cli/src/unit_tests/ctx_init.rs +++ b/rs/cli/src/unit_tests/ctx_init.rs @@ -1,8 +1,13 @@ -use std::path::PathBuf; +use std::{path::PathBuf, str::FromStr}; -use crate::auth::Auth; +use crate::{ + auth::{Auth, Neuron, STAGING_KEY_PATH_FROM_HOME, STAGING_NEURON_ID}, + commands::{AuthOpts, AuthRequirement, HsmOpts}, +}; +use clio::{ClioPath, InputPath}; use ic_canisters::governance::governance_canister_version; use ic_management_types::Network; +use itertools::Itertools; use crate::{commands::IcAdminVersion, ctx::DreContext, ic_admin::FALLBACK_IC_ADMIN_VERSION}; @@ -22,27 +27,36 @@ async fn get_context(network: &Network, version: IcAdminVersion) -> anyhow::Resu DreContext::new( network.name.clone(), network.nns_urls.clone(), - Auth::Anonymous, + AuthOpts { + private_key_pem: None, + hsm_opts: crate::commands::HsmOpts { + hsm_pin: None, + hsm_params: crate::commands::HsmParams { + hsm_slot: None, + hsm_key_id: None, + }, + }, + }, None, false, true, false, true, - crate::commands::IcAdminRequirement::Detect, + crate::commands::AuthRequirement::Anonymous, None, version, ) .await } -struct TestScenario<'a> { +struct AdminVersionTestScenario<'a> { name: &'static str, version: IcAdminVersion, should_delete_status_file: bool, should_contain: Option<&'a str>, } -impl<'a> TestScenario<'a> { +impl<'a> AdminVersionTestScenario<'a> { fn new(name: &'static str) -> Self { Self { name, @@ -78,18 +92,18 @@ async fn init_tests_ic_admin_version() { let governance_version = governance_canister_version(&mainnet.nns_urls).await.unwrap(); let tests = &[ - TestScenario::new("match governance canister") + AdminVersionTestScenario::new("match governance canister") .delete_status_file() .should_contain(&governance_version.stringified_hash), - TestScenario::new("use default version") + AdminVersionTestScenario::new("use default version") .delete_status_file() .version(IcAdminVersion::Fallback) .should_contain(FALLBACK_IC_ADMIN_VERSION), - TestScenario::new("existing version on s3") + AdminVersionTestScenario::new("existing version on s3") .delete_status_file() .version(IcAdminVersion::Strict(version_on_s3.to_string())) .should_contain(version_on_s3), - TestScenario::new("random version not present on s3").version(IcAdminVersion::Strict("random-version".to_string())), + AdminVersionTestScenario::new("random version not present on s3").version(IcAdminVersion::Strict("random-version".to_string())), ]; for test in tests { @@ -109,7 +123,9 @@ async fn init_tests_ic_admin_version() { ); let ctx = maybe_ctx.unwrap(); - let ic_admin_path = ctx.ic_admin_path().unwrap(); + let ic_admin_path = ctx.ic_admin().await; + assert!(ic_admin_path.is_ok()); + let ic_admin_path = ic_admin_path.unwrap().ic_admin_path().unwrap_or_default(); assert!( ic_admin_path.contains(ver), "Test `{}`: ic_admin_path `{}`, expected version `{}`", @@ -119,11 +135,20 @@ async fn init_tests_ic_admin_version() { ) } else { assert!( - maybe_ctx.is_err(), - "Test `{}`: expected error but got ok with version: {}", + maybe_ctx.is_ok(), + "Test `{}`: expected ok for ctx but got err: {:?}", test.name, - maybe_ctx.unwrap().ic_admin_path().unwrap() + maybe_ctx.err().unwrap() ); + + let ctx = maybe_ctx.unwrap(); + let maybe_ic_admin = ctx.ic_admin().await; + assert!( + maybe_ic_admin.is_err(), + "Test `{}`: expected err for ic-admin but got ok with path: {}", + test.name, + maybe_ic_admin.unwrap().ic_admin_path().unwrap_or_default() + ) } if test.should_delete_status_file { @@ -135,3 +160,218 @@ async fn init_tests_ic_admin_version() { } } } + +async fn get_ctx_for_neuron_test( + auth: AuthOpts, + neuron_id: Option, + requirement: AuthRequirement, + network: String, +) -> anyhow::Result { + DreContext::new( + network, + vec![], + auth, + neuron_id, + true, + false, + false, + true, + requirement, + None, + IcAdminVersion::Strict("Shouldn't get to here".to_string()), + ) + .await +} + +struct NeuronAuthTestScenarion<'a> { + name: &'a str, + neuron_id: Option, + private_key_pem: Option, + hsm_pin: Option, + hsm_key_id: Option, + hsm_slot: Option, + requirement: AuthRequirement, + network: String, + want: anyhow::Result, +} + +// Must be left here until we add HSM simulator +#[allow(dead_code)] +impl<'a> NeuronAuthTestScenarion<'a> { + fn new(name: &'a str) -> Self { + Self { + name, + neuron_id: None, + private_key_pem: None, + hsm_pin: None, + hsm_key_id: None, + hsm_slot: None, + requirement: AuthRequirement::Anonymous, + network: "".to_string(), + want: Ok(Neuron::anonymous_neuron()), + } + } + + fn with_neuron_id(self, neuron_id: u64) -> Self { + Self { + neuron_id: Some(neuron_id), + ..self + } + } + + fn with_private_key(self, private_key_path: String) -> Self { + Self { + private_key_pem: Some(private_key_path), + ..self + } + } + + fn with_pin(self, hsm_pin: &'a str) -> Self { + Self { + hsm_pin: Some(hsm_pin.to_string()), + ..self + } + } + + fn with_key_id(self, hsm_key_id: u8) -> Self { + Self { + hsm_key_id: Some(hsm_key_id), + ..self + } + } + + fn with_slot(self, hsm_slot: u64) -> Self { + Self { + hsm_slot: Some(hsm_slot), + ..self + } + } + + fn when_requirement(self, auth: AuthRequirement) -> Self { + Self { requirement: auth, ..self } + } + + fn with_network(self, network: &'a str) -> Self { + Self { + network: network.to_string(), + ..self + } + } + + fn want(self, neuron: anyhow::Result) -> Self { + Self { want: neuron, ..self } + } + + async fn get_neuron(&self) -> anyhow::Result { + get_ctx_for_neuron_test( + AuthOpts { + private_key_pem: self + .private_key_pem + .as_ref() + .map(|path| InputPath::new(ClioPath::new(path).unwrap()).unwrap()), + hsm_opts: HsmOpts { + hsm_pin: self.hsm_pin.clone(), + hsm_params: crate::commands::HsmParams { + hsm_slot: self.hsm_slot, + hsm_key_id: self.hsm_key_id, + }, + }, + }, + self.neuron_id, + self.requirement.clone(), + self.network.clone(), + ) + .await + .map(|ctx| ctx.neuron()) + } +} + +fn get_staging_key_path() -> PathBuf { + PathBuf::from_str(&std::env::var("HOME").unwrap()) + .unwrap() + .join(STAGING_KEY_PATH_FROM_HOME) +} + +fn ensure_testing_pem(name: &str) -> PathBuf { + let path = PathBuf::from_str(&std::env::var("HOME").unwrap()) + .unwrap() + .join(format!(".config/dfx/identity/{}/identity.pem", name)); + + let parent = path.parent().unwrap(); + if !parent.exists() { + std::fs::create_dir_all(parent).unwrap() + } + + if !path.exists() { + std::fs::write(&path, "Some private key").unwrap(); + } + path +} + +#[tokio::test] +async fn init_test_neuron_and_auth() { + let scenarios = &[ + // Successful scenarios + // + // Should use the known neuron for staging + // If run in CI it will require the key to be there + NeuronAuthTestScenarion::new("Staging signer") + .with_network("staging") + .want(Ok(Neuron { + auth: Auth::Keyfile { + path: get_staging_key_path(), + }, + ..Neuron::anonymous_neuron() + })) + .when_requirement(AuthRequirement::Signer), + NeuronAuthTestScenarion::new("Staging anonymous") + .with_network("staging") + .want(Ok(Neuron::anonymous_neuron())) + .when_requirement(AuthRequirement::Anonymous), + NeuronAuthTestScenarion::new("Staging neuron") + .with_network("staging") + .want(Ok(Neuron { + auth: Auth::Keyfile { + path: get_staging_key_path(), + }, + neuron_id: STAGING_NEURON_ID, + include_proposer: true, + })) + .when_requirement(AuthRequirement::Neuron), + // Failing scenarios + // + NeuronAuthTestScenarion::new("Detecting neuron id for random private key") + .with_network("mainnet") + .with_private_key(ensure_testing_pem("testing").to_str().unwrap().to_string()) + .want(Err(anyhow::anyhow!("Will not be able to detect neuron id"))) + .when_requirement(AuthRequirement::Neuron), + ]; + + let mut outcomes = vec![]; + for test in scenarios { + let got = test.get_neuron().await; + outcomes.push(( + test.name, + format!("{:?}", test.want), + format!("{:?}", got), + (match (&test.want, got) { + (Ok(want), Ok(got)) => want.eq(&got), + (Ok(_), Err(_)) => false, + (Err(_), Ok(_)) => false, + (Err(_), Err(_)) => true, + }), + )) + } + + assert!( + outcomes.iter().map(|(_, _, _, is_successful)| is_successful).all(|o| *o), + "{}", + outcomes + .iter() + .filter_map(|(name, wanted, got, is_successful)| match is_successful { + true => None, + false => Some(format!("Test `{}` failed:\nWanted:\n\t{}\nGot:\n\t{}\n", name, wanted, got)), + }) + .join("\n\n") + ) +} diff --git a/rs/cli/src/unit_tests/update_unassigned_nodes.rs b/rs/cli/src/unit_tests/update_unassigned_nodes.rs index d25b20a4a..10bdae562 100644 --- a/rs/cli/src/unit_tests/update_unassigned_nodes.rs +++ b/rs/cli/src/unit_tests/update_unassigned_nodes.rs @@ -10,12 +10,6 @@ use ic_types::PrincipalId; use crate::ctx::tests::get_mocked_ctx; -fn anonymous_admin() -> MockIcAdmin { - let mut ic_admin = MockIcAdmin::new(); - ic_admin.expect_neuron().returning(|| Neuron::anonymous_neuron()); - ic_admin -} - fn registry_with_subnets(subnets: Vec) -> MockLazyRegistry { let mut registry = MockLazyRegistry::new(); @@ -31,8 +25,7 @@ fn registry_with_subnets(subnets: Vec) -> MockLazyRegistry { #[tokio::test] async fn should_skip_update_same_version_nns_not_provided() { - let mut ic_admin = anonymous_admin(); - + let mut ic_admin = MockIcAdmin::new(); let principal = PrincipalId::from_str("tdb26-jop6k-aogll-7ltgs-eruif-6kk7m-qpktf-gdiqx-mxtrf-vb5e6-eqe").unwrap(); let mut registry = registry_with_subnets(vec![Subnet { @@ -50,8 +43,9 @@ async fn should_skip_update_same_version_nns_not_provided() { let ctx = get_mocked_ctx( Network::mainnet_unchecked().unwrap(), + Neuron::anonymous_neuron(), Arc::new(registry), - Arc::new(ic_admin), + Arc::new(MockIcAdmin::new()), Arc::new(MockLazyGit::new()), Arc::new(MockProposalAgent::new()), Arc::new(MockArtifactDownloader::new()), @@ -64,7 +58,7 @@ async fn should_skip_update_same_version_nns_not_provided() { #[tokio::test] async fn should_skip_update_same_version_nns_provided() { - let mut ic_admin = anonymous_admin(); + let mut ic_admin = MockIcAdmin::new(); let principal = PrincipalId::new_anonymous(); @@ -83,6 +77,7 @@ async fn should_skip_update_same_version_nns_provided() { let ctx = get_mocked_ctx( Network::mainnet_unchecked().unwrap(), + Neuron::anonymous_neuron(), Arc::new(registry), Arc::new(ic_admin), Arc::new(MockLazyGit::new()), @@ -99,7 +94,7 @@ async fn should_skip_update_same_version_nns_provided() { #[tokio::test] async fn should_update_unassigned_nodes() { - let mut ic_admin = anonymous_admin(); + let mut ic_admin = MockIcAdmin::new(); let principal = PrincipalId::new_anonymous(); @@ -121,6 +116,7 @@ async fn should_update_unassigned_nodes() { let ctx = get_mocked_ctx( Network::mainnet_unchecked().unwrap(), + Neuron::anonymous_neuron(), Arc::new(registry), Arc::new(ic_admin), Arc::new(MockLazyGit::new()), @@ -137,7 +133,7 @@ async fn should_update_unassigned_nodes() { #[tokio::test] async fn should_fail_nns_not_found() { - let mut ic_admin = anonymous_admin(); + let mut ic_admin = MockIcAdmin::new(); let principal = PrincipalId::new_subnet_test_id(0); let other_principal = PrincipalId::new_subnet_test_id(1); @@ -157,6 +153,7 @@ async fn should_fail_nns_not_found() { let ctx = get_mocked_ctx( Network::mainnet_unchecked().unwrap(), + Neuron::anonymous_neuron(), Arc::new(registry), Arc::new(ic_admin), Arc::new(MockLazyGit::new()), diff --git a/rs/cli/src/unit_tests/version.rs b/rs/cli/src/unit_tests/version.rs index 4fb21c872..02f98defe 100644 --- a/rs/cli/src/unit_tests/version.rs +++ b/rs/cli/src/unit_tests/version.rs @@ -10,6 +10,7 @@ use itertools::Itertools; use crate::{ artifact_downloader::MockArtifactDownloader, + auth::Neuron, commands::ExecutableCommand, ctx::tests::get_mocked_ctx, ic_admin::{MockIcAdmin, ProposeCommand, ProposeOptions}, @@ -64,6 +65,7 @@ async fn guest_os_elect_version_tests() { let ctx = get_mocked_ctx( Network::mainnet_unchecked().unwrap(), + Neuron::anonymous_neuron(), Arc::new(registry), Arc::new(ic_admin), Arc::new(git), diff --git a/rs/qualifier/src/qualify_util.rs b/rs/qualifier/src/qualify_util.rs index 9b2f18020..449400b03 100644 --- a/rs/qualifier/src/qualify_util.rs +++ b/rs/qualifier/src/qualify_util.rs @@ -2,7 +2,6 @@ use std::{path::PathBuf, str::FromStr}; use anyhow::Error; use dre::{ - auth::Auth, commands::{qualify::execute::Execute, ExecutableCommand}, ctx::DreContext, }; @@ -77,13 +76,13 @@ pub async fn qualify( let ctx = DreContext::new( network_name.to_string(), config.nns_urls, - Auth::pem(private_key_pem).await?, + private_key_pem.try_into()?, Some(neuron_id), false, false, true, false, - cmd.require_ic_admin(), + cmd.require_auth(), None, dre::commands::IcAdminVersion::FromGovernance, )