From f0d907d92fb0f4a870c4a1472bfec1c7a5c32fa5 Mon Sep 17 00:00:00 2001 From: Jean-Gabriel Gill-Couture Date: Wed, 3 Sep 2025 17:50:17 -0400 Subject: [PATCH] feat: opnsense-config make sure file has not changed on remote since loading it before saving it, also fix group member Vec type was not able to deserialize when more than one member --- Cargo.lock | 1 + harmony/src/modules/okd/ipxe.rs | 7 ++-- opnsense-config-xml/src/data/opnsense.rs | 7 ++-- opnsense-config/Cargo.toml | 1 + opnsense-config/src/config/config.rs | 31 +++++++++++------ .../src/config/manager/local_file.rs | 13 +++++-- opnsense-config/src/config/manager/mod.rs | 6 ++-- opnsense-config/src/config/manager/ssh.rs | 34 +++++++++++++++---- opnsense-config/src/config/shell/ssh.rs | 2 +- 9 files changed, 74 insertions(+), 28 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index a49e99b..afc3f2e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3871,6 +3871,7 @@ dependencies = [ "russh-sftp", "serde", "serde_json", + "sha2", "thiserror 1.0.69", "tokio", "tokio-stream", diff --git a/harmony/src/modules/okd/ipxe.rs b/harmony/src/modules/okd/ipxe.rs index a4dcd18..c6aa6c1 100644 --- a/harmony/src/modules/okd/ipxe.rs +++ b/harmony/src/modules/okd/ipxe.rs @@ -38,7 +38,9 @@ pub struct OKDIpxeInterpret { } #[async_trait] -impl Interpret for OKDIpxeInterpret { +impl Interpret + for OKDIpxeInterpret +{ async fn execute( &self, inventory: &Inventory, @@ -81,7 +83,6 @@ impl Interpret f folder_to_serve: None, // folder_to_serve: Some(Url::LocalFolder("./data/pxe/okd/http_files/".to_string())), files: vec![ - FileContent { path: FilePath::Relative("boot.ipxe".to_string()), content: BootIpxeTpl { @@ -122,7 +123,7 @@ impl Interpret f Err(e) => return Err(e), }; } - inquire::Confirm::new("Execute the copy : `scp -r data/pxe/okd/http_files/* root@192.168.1.1:/usr/local/http/` and confirm when done to continue").prompt().expect("Prompt error"); + inquire::Confirm::new(&format!("Execute the copy : `scp -r data/pxe/okd/http_files/* root@{}:/usr/local/http/` and confirm when done to continue", HttpServer::get_ip(topology))).prompt().expect("Prompt error"); Ok(Outcome::success("Ipxe installed".to_string())) } diff --git a/opnsense-config-xml/src/data/opnsense.rs b/opnsense-config-xml/src/data/opnsense.rs index 2efbaf3..dc0fded 100644 --- a/opnsense-config-xml/src/data/opnsense.rs +++ b/opnsense-config-xml/src/data/opnsense.rs @@ -51,6 +51,7 @@ pub struct OPNsense { impl From for OPNsense { fn from(content: String) -> Self { + yaserde::de::from_str(&content) .map_err(|e| println!("{}", e)) .expect("OPNSense received invalid string, should be full XML") @@ -267,12 +268,12 @@ pub struct Bogons { #[derive(Default, PartialEq, Debug, YaSerialize, YaDeserialize)] pub struct Group { pub name: String, - pub description: String, + pub description: Option, pub scope: String, pub gid: u32, - pub member: Vec, + pub member: String, #[yaserde(rename = "priv")] - pub priv_field: String, + pub priv_field: Option, pub source_networks: Option, } diff --git a/opnsense-config/Cargo.toml b/opnsense-config/Cargo.toml index e70bc12..0580cb2 100644 --- a/opnsense-config/Cargo.toml +++ b/opnsense-config/Cargo.toml @@ -21,6 +21,7 @@ serde_json = "1.0.133" tokio-util = { version = "0.7.13", features = ["codec"] } tokio-stream = "0.1.17" uuid.workspace = true +sha2 = "0.10.9" [dev-dependencies] pretty_assertions.workspace = true diff --git a/opnsense-config/src/config/config.rs b/opnsense-config/src/config/config.rs index 0a1072c..a4701cb 100644 --- a/opnsense-config/src/config/config.rs +++ b/opnsense-config/src/config/config.rs @@ -1,7 +1,7 @@ use std::sync::Arc; use crate::{ - config::{SshConfigManager, SshCredentials, SshOPNSenseShell}, + config::{check_hash, get_hash, SshConfigManager, SshCredentials, SshOPNSenseShell}, error::Error, modules::{ caddy::CaddyConfig, dhcp_legacy::DhcpConfigLegacyISC, dns::UnboundDnsConfig, @@ -12,6 +12,7 @@ use log::{debug, info, trace, warn}; use opnsense_config_xml::OPNsense; use russh::client; use serde::Serialize; +use sha2::Digest; use super::{ConfigManager, OPNsenseShell}; @@ -20,6 +21,7 @@ pub struct Config { opnsense: OPNsense, repository: Arc, shell: Arc, + hash: String, } impl Serialize for Config { @@ -36,8 +38,10 @@ impl Config { repository: Arc, shell: Arc, ) -> Result { + let (opnsense, hash) = Self::get_opnsense_instance(repository.clone()).await?; Ok(Self { - opnsense: Self::get_opnsense_instance(repository.clone()).await?, + opnsense, + hash, repository, shell, }) @@ -146,7 +150,7 @@ impl Config { async fn reload_config(&mut self) -> Result<(), Error> { info!("Reloading opnsense live config"); - self.opnsense = Self::get_opnsense_instance(self.repository.clone()).await?; + let (opnsense, sha2) = Self::get_opnsense_instance(self.repository.clone()).await?; Ok(()) } @@ -158,14 +162,15 @@ impl Config { /// Save the config to the repository. This method is meant NOT to reload services, only save /// the config to the live file/database and perhaps take a backup when relevant. pub async fn save(&self) -> Result<(), Error> { - self.repository.save_config(&self.opnsense.to_xml()).await + let xml = &self.opnsense.to_xml(); + self.repository.save_config(xml, &self.hash).await } /// Save the configuration and reload all services. Be careful with this one as it will cause /// downtime in many cases, such as a PPPoE renegociation pub async fn apply(&self) -> Result<(), Error> { self.repository - .apply_new_config(&self.opnsense.to_xml()) + .apply_new_config(&self.opnsense.to_xml(), &self.hash) .await } @@ -193,11 +198,14 @@ impl Config { Config::new(manager, shell).await.unwrap() } - async fn get_opnsense_instance(repository: Arc) -> Result { + async fn get_opnsense_instance( + repository: Arc, + ) -> Result<(OPNsense, String), Error> { let xml = repository.load_as_str().await?; trace!("xml {}", xml); - Ok(OPNsense::from(xml)) + let hash = get_hash(&xml); + Ok((OPNsense::from(xml), hash)) } pub async fn run_command(&self, command: &str) -> Result { @@ -227,6 +235,7 @@ mod tests { "src/tests/data/config-full-25.7.xml", "src/tests/data/config-full-25.7-dummy-dnsmasq-options.xml", "src/tests/data/config-25.7-dnsmasq-static-host.xml", + "src/tests/data/config-wk1-20250903.xmlDONOTCOMMIT", ] { let mut test_file_path = PathBuf::from(env!("CARGO_MANIFEST_DIR")); test_file_path.push(path); @@ -248,9 +257,11 @@ mod tests { // Since the order of all fields is not always the same in opnsense config files // I think it is good enough to have exactly the same amount of the same lines - [config_file_str.lines().collect::>()].sort(); - [config_file_str.lines().collect::>()].sort(); - assert_eq!((), ()); + let mut before = config_file_str.lines().collect::>(); + let mut after = serialized.lines().collect::>(); + before.sort(); + after.sort(); + assert_eq!(before, after); } } diff --git a/opnsense-config/src/config/manager/local_file.rs b/opnsense-config/src/config/manager/local_file.rs index 804b74c..e48f284 100644 --- a/opnsense-config/src/config/manager/local_file.rs +++ b/opnsense-config/src/config/manager/local_file.rs @@ -1,3 +1,4 @@ +use crate::config::check_hash; use crate::config::manager::ConfigManager; use crate::error::Error; use async_trait::async_trait; @@ -20,11 +21,17 @@ impl ConfigManager for LocalFileConfigManager { Ok(fs::read_to_string(&self.file_path)?) } - async fn save_config(&self, content: &str) -> Result<(), Error> { + async fn save_config(&self, content: &str, hash: &str) -> Result<(), Error> { + let current_content = self.load_as_str().await?; + if !check_hash(¤t_content, hash) { + return Err(Error::Config(format!( + "OPNSense config file changed since loading it! Hash when loading : {hash}" + ))); + } Ok(fs::write(&self.file_path, content)?) } - async fn apply_new_config(&self, content: &str) -> Result<(), Error> { - self.save_config(content).await + async fn apply_new_config(&self, content: &str, hash: &str) -> Result<(), Error> { + self.save_config(content, hash).await } } diff --git a/opnsense-config/src/config/manager/mod.rs b/opnsense-config/src/config/manager/mod.rs index ad3a6b9..70831a4 100644 --- a/opnsense-config/src/config/manager/mod.rs +++ b/opnsense-config/src/config/manager/mod.rs @@ -9,6 +9,8 @@ use crate::Error; #[async_trait] pub trait ConfigManager: std::fmt::Debug + Send + Sync { async fn load_as_str(&self) -> Result; - async fn save_config(&self, content: &str) -> Result<(), Error>; - async fn apply_new_config(&self, content: &str) -> Result<(), Error>; + /// Save a new version of the config file, making sure that the hash still represents the file + /// currently stored in /conf/config.xml + async fn save_config(&self, content: &str, hash: &str) -> Result<(), Error>; + async fn apply_new_config(&self, content: &str, hash: &str) -> Result<(), Error>; } diff --git a/opnsense-config/src/config/manager/ssh.rs b/opnsense-config/src/config/manager/ssh.rs index fb525ea..6bd6b14 100644 --- a/opnsense-config/src/config/manager/ssh.rs +++ b/opnsense-config/src/config/manager/ssh.rs @@ -3,6 +3,7 @@ use crate::error::Error; use async_trait::async_trait; use log::info; use russh_keys::key::KeyPair; +use sha2::Digest; use std::sync::Arc; #[derive(Debug)] @@ -35,10 +36,10 @@ impl SshConfigManager { .await } - async fn move_to_live_config(&self, new_config_path: &str) -> Result { + async fn copy_to_live_config(&self, new_config_path: &str) -> Result { info!("Overwriting OPNSense /conf/config.xml with {new_config_path}"); self.opnsense_shell - .exec(&format!("mv {new_config_path} /conf/config.xml")) + .exec(&format!("cp {new_config_path} /conf/config.xml")) .await } @@ -56,19 +57,40 @@ impl ConfigManager for SshConfigManager { self.opnsense_shell.exec("cat /conf/config.xml").await } - async fn save_config(&self, content: &str) -> Result<(), Error> { + async fn save_config(&self, content: &str, hash: &str) -> Result<(), Error> { + let current_content = self.load_as_str().await?; + + if !check_hash(¤t_content, hash) { + return Err(Error::Config(format!( + "OPNSense config file changed since loading it! Hash when loading : {hash}" + ))); + } + let temp_filename = self .opnsense_shell .write_content_to_temp_file(content) .await?; self.backup_config_remote().await?; - self.move_to_live_config(&temp_filename).await?; + self.copy_to_live_config(&temp_filename).await?; Ok(()) } - async fn apply_new_config(&self, content: &str) -> Result<(), Error> { - self.save_config(content).await?; + async fn apply_new_config(&self, content: &str, hash: &str) -> Result<(), Error> { + self.save_config(content, &hash).await?; self.reload_all_services().await?; Ok(()) } } + +pub fn get_hash(content: &str) -> String { + let mut hasher = sha2::Sha256::new(); + hasher.update(content.as_bytes()); + let hash_bytes = hasher.finalize(); + let hash_string = format!("{:x}", hash_bytes); + info!("Loaded OPNSense config.xml with hash {hash_string:?}"); + hash_string +} + +pub fn check_hash(content: &str, source_hash: &str) -> bool { + get_hash(content) == source_hash +} diff --git a/opnsense-config/src/config/shell/ssh.rs b/opnsense-config/src/config/shell/ssh.rs index 95069fc..1f82c63 100644 --- a/opnsense-config/src/config/shell/ssh.rs +++ b/opnsense-config/src/config/shell/ssh.rs @@ -39,7 +39,7 @@ impl OPNsenseShell for SshOPNSenseShell { async fn write_content_to_temp_file(&self, content: &str) -> Result { let temp_filename = format!( - "/tmp/opnsense-config-tmp-config_{}", + "/conf/harmony/opnsense-config-{}", SystemTime::now() .duration_since(UNIX_EPOCH) .unwrap()