feat: opnsense-config make sure file has not changed on remote since loading it before saving it, also fix group member Vec<u8> type was not able to deserialize when more than one member

Cargo.lock

@@ -3871,6 +3871,7 @@ dependencies = [
  "russh-sftp",
  "serde",
  "serde_json",
+ "sha2",
  "thiserror 1.0.69",
  "tokio",
  "tokio-stream",

harmony/src/modules/okd/ipxe.rs

@@ -38,7 +38,9 @@ pub struct OKDIpxeInterpret {
 }
 
 #[async_trait]
-impl<T: Topology + DhcpServer + TftpServer + HttpServer + Router> Interpret<T> for OKDIpxeInterpret {
+impl<T: Topology + DhcpServer + TftpServer + HttpServer + Router> Interpret<T>
+    for OKDIpxeInterpret
+{
     async fn execute(
         &self,
         inventory: &Inventory,
@@ -81,7 +83,6 @@ impl<T: Topology + DhcpServer + TftpServer + HttpServer + Router> Interpret<T> 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<T: Topology + DhcpServer + TftpServer + HttpServer + Router> Interpret<T> 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()))
     }

opnsense-config-xml/src/data/opnsense.rs

@@ -51,6 +51,7 @@ pub struct OPNsense {
 
 impl From<String> 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<String>,
     pub scope: String,
     pub gid: u32,
-    pub member: Vec<u32>,
+    pub member: String,
     #[yaserde(rename = "priv")]
-    pub priv_field: String,
+    pub priv_field: Option<String>,
     pub source_networks: Option<MaybeString>,
 }
 

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

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<dyn ConfigManager>,
     shell: Arc<dyn OPNsenseShell>,
+    hash: String,
 }
 
 impl Serialize for Config {
@@ -36,8 +38,10 @@ impl Config {
         repository: Arc<dyn ConfigManager>,
         shell: Arc<dyn OPNsenseShell>,
     ) -> Result<Self, Error> {
+        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<dyn ConfigManager>) -> Result<OPNsense, Error> {
+    async fn get_opnsense_instance(
+        repository: Arc<dyn ConfigManager>,
+    ) -> 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<String, Error> {
@@ -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::<Vec<_>>()].sort();
-            [config_file_str.lines().collect::<Vec<_>>()].sort();
-            assert_eq!((), ());
+            let mut before = config_file_str.lines().collect::<Vec<_>>();
+            let mut after = serialized.lines().collect::<Vec<_>>();
+            before.sort();
+            after.sort();
+            assert_eq!(before, after);
         }
     }
 

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(&current_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
     }
 }

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<String, Error>;
-    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>;
 }

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<String, Error> {
+    async fn copy_to_live_config(&self, new_config_path: &str) -> Result<String, Error> {
         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(&current_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
+}

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<String, Error> {
         let temp_filename = format!(
-            "/tmp/opnsense-config-tmp-config_{}",
+            "/conf/harmony/opnsense-config-{}",
             SystemTime::now()
                 .duration_since(UNIX_EPOCH)
                 .unwrap()