Skip to content

Commit

Permalink
Fix firewall rule management. (#230)
Browse files Browse the repository at this point in the history 
* 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?).
  • Loading branch information
@DFINITYManu
DFINITYManu authored Mar 6, 2024
1 parent ed7142b commit bdd1208
Show file tree
Hide file tree
Showing 3 changed files with 214 additions and 93 deletions.
9 changes: 8 additions & 1 deletion rs/cli/src/cli.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,14 @@ pub(crate) enum Commands {
},

/// Firewall rules
Firewall,
Firewall {
#[clap(long, default_value = Some("Proposal to modify firewall rules"))]
title: Option<String>,
#[clap(long, default_value = None)]
summary: Option<String>,
#[clap(long, default_value = None)]
motivation: Option<String>,
},
}

pub(crate) mod subnet {
Expand Down
290 changes: 200 additions & 90 deletions rs/cli/src/ic_admin.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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<FirewallRuleModification>,
}

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<FirewallRuleModification> {
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<FirewallRuleModification>)> {
let mut batches: Vec<(FirewallRuleModificationType, Vec<FirewallRuleModification>)> = vec![];
let mut current_batch: Vec<FirewallRuleModification> = vec![];
let mut modtype: Option<FirewallRuleModificationType> = 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<String>,
Expand Down Expand Up @@ -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))?;
Expand Down Expand Up @@ -679,112 +785,116 @@ must be identical, and must match the SHA256 from the payload of the NNS proposa
let removed_entries: BTreeMap<usize, &FirewallRule> =
rules.into_iter().filter(|(key, _)| !edited.contains_key(key)).collect();

fn pretty_print_diff(change_type: &str, entries: &BTreeMap<usize, &FirewallRule>) {
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<usize, &FirewallRule>,
modifications: Vec<FirewallRuleModification>,
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::<Vec<_>>();
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(())
}
}
Expand Down
8 changes: 6 additions & 2 deletions rs/cli/src/main.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}
})
Expand Down

0 comments on commit bdd1208

Please sign in to comment.