From bdd1208fc93355c6f1f4479a6ed4ff265e28f29e Mon Sep 17 00:00:00 2001 From: DFINITYManu <123385598+DFINITYManu@users.noreply.github.com> Date: Wed, 6 Mar 2024 13:09:40 +0100 Subject: [PATCH] Fix firewall rule management. (#230) * Fix typo in proposal to remove firewall rules. * Fix signature of remove command being called. * Batch multiple operations so we can at least remove / modify / add multiple rules in one proposal. Before we do the batching, we sort the rules by modification slot then we reverse the order of the rules list, and then we do the batching. This ensures that the modifications happen in an order that will not conflict between each other (what does it mean "add rule in position 3" if rule in position 2 was removed prior to that?). --- rs/cli/src/cli.rs | 9 +- rs/cli/src/ic_admin.rs | 290 ++++++++++++++++++++++++++++------------- rs/cli/src/main.rs | 8 +- 3 files changed, 214 insertions(+), 93 deletions(-) diff --git a/rs/cli/src/cli.rs b/rs/cli/src/cli.rs index c80444abb..3eb58c243 100644 --- a/rs/cli/src/cli.rs +++ b/rs/cli/src/cli.rs @@ -145,7 +145,14 @@ pub(crate) enum Commands { }, /// Firewall rules - Firewall, + Firewall { + #[clap(long, default_value = Some("Proposal to modify firewall rules"))] + title: Option, + #[clap(long, default_value = None)] + summary: Option, + #[clap(long, default_value = None)] + motivation: Option, + }, } pub(crate) mod subnet { diff --git a/rs/cli/src/ic_admin.rs b/rs/cli/src/ic_admin.rs index f7d53be2a..2719ce164 100644 --- a/rs/cli/src/ic_admin.rs +++ b/rs/cli/src/ic_admin.rs @@ -18,14 +18,16 @@ use log::{error, info, warn}; use prost::Message; use regex::Regex; use reqwest::StatusCode; +use serde::Serialize; use sha2::{Digest, Sha256}; use std::collections::BTreeMap; +use std::fmt; use std::fs::File; use std::io::{Read, Write}; use std::os::unix::fs::PermissionsExt; use std::process::Stdio; use std::time::Duration; -use std::{path::Path, process::Command}; +use std::{fmt::Display, path::Path, process::Command}; use strum::Display; use tempfile::NamedTempFile; @@ -35,6 +37,105 @@ use crate::{cli, defaults}; const MAX_SUMMARY_CHAR_COUNT: usize = 14000; +#[derive(Clone, Serialize, PartialEq)] +enum FirewallRuleModificationType { + Addition, + Update, + Removal, +} + +impl Display for FirewallRuleModificationType { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + match self { + Self::Addition => write!(f, "add"), + Self::Update => write!(f, "update"), + Self::Removal => write!(f, "remove"), + } + } +} + +#[derive(Clone, Serialize)] +struct FirewallRuleModification { + change_type: FirewallRuleModificationType, + rule_being_modified: FirewallRule, + position: usize, +} + +impl FirewallRuleModification { + fn addition(position: usize, rule: FirewallRule) -> Self { + Self { + change_type: FirewallRuleModificationType::Addition, + rule_being_modified: rule, + position, + } + } + fn update(position: usize, rule: FirewallRule) -> Self { + Self { + change_type: FirewallRuleModificationType::Update, + rule_being_modified: rule, + position, + } + } + fn removal(position: usize, rule: FirewallRule) -> Self { + Self { + change_type: FirewallRuleModificationType::Removal, + rule_being_modified: rule, + position, + } + } +} + +struct FirewallRuleModifications { + raw: Vec, +} + +impl FirewallRuleModifications { + fn new() -> Self { + FirewallRuleModifications { raw: vec![] } + } + + fn addition(&mut self, position: usize, rule: FirewallRule) { + self.raw.push(FirewallRuleModification::addition(position, rule)) + } + + fn update(&mut self, position: usize, rule: FirewallRule) { + self.raw.push(FirewallRuleModification::update(position, rule)) + } + + fn removal(&mut self, position: usize, rule: FirewallRule) { + self.raw.push(FirewallRuleModification::removal(position, rule)) + } + + fn reverse_sorted(&self) -> Vec { + let mut sorted = self.raw.to_vec(); + sorted.sort_by(|first, second| first.position.partial_cmp(&second.position).unwrap()); + sorted.reverse(); + sorted + } + + fn reverse_sorted_and_batched(&self) -> Vec<(FirewallRuleModificationType, Vec)> { + let mut batches: Vec<(FirewallRuleModificationType, Vec)> = vec![]; + let mut current_batch: Vec = vec![]; + let mut modtype: Option = None; + for modif in self.reverse_sorted().iter() { + if modtype.is_none() { + modtype = Some(modif.clone().change_type); + } + if modtype.clone().unwrap() == modif.change_type { + current_batch.push(modif.clone()) + } else { + batches.push((current_batch[0].clone().change_type, current_batch)); + current_batch = vec![]; + modtype = Some(modif.clone().change_type); + } + } + if !current_batch.is_empty() { + batches.push((current_batch[0].clone().change_type, current_batch)) + } + batches + } +} + #[derive(Clone)] pub struct IcAdminWrapper { ic_admin: Option, @@ -611,7 +712,12 @@ must be identical, and must match the SHA256 from the payload of the NNS proposa Ok(()) } - pub async fn update_replica_nodes_firewall(&self, network: Network, simulate: bool) -> Result<(), Error> { + pub async fn update_replica_nodes_firewall( + &self, + network: Network, + propose_options: ProposeOptions, + simulate: bool, + ) -> Result<(), Error> { let local_registry_path = local_registry_path(network.clone()); let local_registry = LocalRegistry::new(local_registry_path, Duration::from_secs(10)) .map_err(|e| anyhow::anyhow!("Error in creating local registry instance: {:?}", e))?; @@ -679,112 +785,116 @@ must be identical, and must match the SHA256 from the payload of the NNS proposa let removed_entries: BTreeMap = rules.into_iter().filter(|(key, _)| !edited.contains_key(key)).collect(); - fn pretty_print_diff(change_type: &str, entries: &BTreeMap) { - if entries.is_empty() { - return; - } - let diff = serde_json::to_string_pretty(entries).unwrap(); - info!("Pretty printing diff ----- {}:\n{}", change_type, diff); + let mut mods = FirewallRuleModifications::new(); + for (pos, rule) in added_entries.into_iter() { + mods.addition(pos, rule.clone()); + } + for (pos, rule) in updated_entries.into_iter() { + mods.update(pos, rule.clone()); + } + for (pos, rule) in removed_entries.into_iter() { + mods.removal(pos, rule.clone()); } - pretty_print_diff("added", &added_entries); - pretty_print_diff("updated", &updated_entries); - pretty_print_diff("removed", &removed_entries); - - if added_entries.is_empty() && updated_entries.is_empty() && removed_entries.is_empty() { + let reverse_sorted = mods.reverse_sorted_and_batched(); + if reverse_sorted.is_empty() { info!("No modifications should be made"); return Ok(()); } - - if added_entries.len() + updated_entries.len() + removed_entries.len() > 1 { + let diff = serde_json::to_string_pretty(&reverse_sorted).unwrap(); + info!("Pretty printing diff:\n{}", diff); + /*if reverse_sorted.len() > 1 { return Err(anyhow::anyhow!( - "Cannot apply more than 1 change at a time due to hash changes" + "Cannot currently apply more than 1 change at a time due to hash changes" )); - } + }*/ //TODO: adapt to use set-firewall config so we can modify more than 1 rule at a time - fn submit_proposals( + fn submit_proposal( admin_wrapper: &IcAdminWrapper, - change_type: &str, - rules: BTreeMap, + modifications: Vec, + propose_options: ProposeOptions, simulate: bool, ) -> anyhow::Result<()> { - if rules.is_empty() { - return Ok(()); - } - info!("Proceeding with creating proposals to '{}' rules", change_type); - for (position, rule) in rules { - let mut file = - NamedTempFile::new().map_err(|e| anyhow::anyhow!("Couldn't create temp file: {:?}", e))?; - - let array = vec![rule]; - let serialized = serde_json::to_string(&array).unwrap(); - - file.write_all(serialized.as_bytes()) - .map_err(|e| anyhow::anyhow!("Couldn't write to tempfile: {:?}", e))?; - - let cmd = ProposeCommand::Raw { - command: format!("{}-firewall-rules", change_type), - args: [ - "--test", - "replica_nodes", - file.path().to_str().unwrap(), - position.to_string().as_str(), - "none", - ] - .iter() - .map(|arg| arg.to_string()) - .collect(), - }; - - let output = admin_wrapper - .propose_run(cmd, ProposeOptions::default(), true) - .map_err(|e| { - anyhow::anyhow!("Couldn't execute test for {}-firewall-rules: {:?}", change_type, e) - })?; - - let parsed: serde_json::Value = serde_json::from_str(&output).map_err(|e| { - anyhow::anyhow!( - "Error deserializing --test output while performing '{}': {:?}", - change_type, - e - ) - })?; - let hash = match parsed.get("hash") { - Some(serde_json::Value::String(hash)) => hash, - _ => { - return Err(anyhow::anyhow!( - "Couldn't find string value for key 'hash'. Whole dump:\n{}", - serde_json::to_string_pretty(&parsed).unwrap() - )) - } - }; - info!("Computed hash for firewall rule at position '{}': {}", position, hash); - - let cmd = ProposeCommand::Raw { - command: format!("{}-firewall-rules", change_type), - args: vec![ + let positions = modifications.iter().map(|modif| modif.position).join(","); + let change_type = modifications[0].clone().change_type; + + let mut file = NamedTempFile::new().map_err(|e| anyhow::anyhow!("Couldn't create temp file: {:?}", e))?; + + let test_args = match change_type { + FirewallRuleModificationType::Removal => vec![ + "--test".to_string(), + "replica_nodes".to_string(), + positions.to_string(), + "none".to_string(), + ], + _ => { + let rules = modifications + .iter() + .map(|modif| modif.clone().rule_being_modified) + .collect::>(); + let serialized = serde_json::to_string(&rules).unwrap(); + file.write_all(serialized.as_bytes()) + .map_err(|e| anyhow::anyhow!("Couldn't write to tempfile: {:?}", e))?; + vec![ + "--test".to_string(), "replica_nodes".to_string(), file.path().to_str().unwrap().to_string(), - position.to_string(), - hash.to_string(), - ], - }; - - let options = ProposeOptions { - title: Some(format!("Proposal to {} firewall rule", change_type)), - motivation: Some(format!("Proposal to {} firewall rule", change_type)), - summary: Some(format!("Proposal to {} firewall rule", change_type)), - }; - admin_wrapper.propose_run(cmd, options, simulate)?; - } + positions.to_string(), + "none".to_string(), + ] + } + }; + + let cmd = ProposeCommand::Raw { + command: format!("{}-firewall-rules", change_type), + args: test_args.clone(), + }; + + let output = admin_wrapper + .propose_run(cmd, propose_options.clone(), true) + .map_err(|e| anyhow::anyhow!("Couldn't execute test for {}-firewall-rules: {:?}", change_type, e))?; + + let parsed: serde_json::Value = serde_json::from_str(&output).map_err(|e| { + anyhow::anyhow!( + "Error deserializing --test output while performing '{}': {:?}", + change_type, + e + ) + })?; + let hash = match parsed.get("hash") { + Some(serde_json::Value::String(hash)) => hash, + _ => { + return Err(anyhow::anyhow!( + "Couldn't find string value for key 'hash'. Whole dump:\n{}", + serde_json::to_string_pretty(&parsed).unwrap() + )) + } + }; + info!("Computed hash for firewall rule at position '{}': {}", positions, hash); + + let mut final_args = test_args.clone(); + // Remove --test from head of args. + let _ = final_args.remove(0); + // Add the real hash to args. + let last = final_args.last_mut().unwrap(); + *last = hash.to_string(); + + let cmd = ProposeCommand::Raw { + command: format!("{}-firewall-rules", change_type), + args: final_args, + }; + + admin_wrapper.propose_run(cmd, propose_options.clone(), simulate)?; + Ok(()) } - submit_proposals(self, "add", added_entries, simulate)?; - submit_proposals(self, "update", updated_entries, simulate)?; - submit_proposals(self, "removed", removed_entries, simulate)?; + for (_, mods) in reverse_sorted.into_iter() { + submit_proposal(self, mods, propose_options.clone(), simulate)?; + break; // no more than one rule mod implemented currenty -- FIXME + } Ok(()) } } diff --git a/rs/cli/src/main.rs b/rs/cli/src/main.rs index ed0901104..314a45d81 100644 --- a/rs/cli/src/main.rs +++ b/rs/cli/src/main.rs @@ -293,9 +293,13 @@ async fn main() -> Result<(), anyhow::Error> { registry_dump::dump_registry(path, cli_opts.network, version).await } - cli::Commands::Firewall => { + cli::Commands::Firewall{title, summary, motivation} => { let ic_admin: IcAdminWrapper = cli::Cli::from_opts(&cli_opts, true).await?.into(); - ic_admin.update_replica_nodes_firewall(cli_opts.network, cli_opts.simulate).await + ic_admin.update_replica_nodes_firewall(cli_opts.network, ic_admin::ProposeOptions{ + title: title.clone(), + summary: summary.clone(), + motivation: motivation.clone(), + }, cli_opts.simulate).await } } })