From 57fbdfd4575f44700abcdc270180167c69c36eec Mon Sep 17 00:00:00 2001 From: Jacek Chmielewski Date: Thu, 26 Jun 2025 09:24:37 +0200 Subject: [PATCH 1/2] Avoid reassigning addresses for networks that didn't change during network update --- crates/defguard_core/src/db/models/device.rs | 13 ++++++++++ .../defguard_core/src/db/models/wireguard.rs | 24 ++++++++++++------- crates/defguard_core/src/lib.rs | 2 +- 3 files changed, 29 insertions(+), 10 deletions(-) diff --git a/crates/defguard_core/src/db/models/device.rs b/crates/defguard_core/src/db/models/device.rs index 8122877b7a..be15f9476c 100644 --- a/crates/defguard_core/src/db/models/device.rs +++ b/crates/defguard_core/src/db/models/device.rs @@ -811,6 +811,7 @@ impl Device { /// - `transaction`: Active PostgreSQL connection to check and insert assignments. /// - `network`: The `WireguardNetwork` whose subnets will be assigned. /// - `reserved_ips`: Optional slice of IPs that must not be assigned, even if otherwise free. + /// - `current_ips`: Optional slice of IPs already assigned to the device - won't be reassigned if they are still valid. /// /// # Returns /// @@ -821,6 +822,7 @@ impl Device { transaction: &mut PgConnection, network: &WireguardNetwork, reserved_ips: Option<&[IpAddr]>, + current_ips: Option<&[IpAddr]>, ) -> Result { debug!( "Assiging IP addresses for device: {} in network {}", @@ -835,6 +837,17 @@ impl Device { "Assigning address to device {} in network {} {address}", self.name, network.name, ); + // Don't reassign addresses for networks that didn't change + if let Some(ip) = + current_ips.and_then(|ips| ips.iter().find(|ip| address.contains(**ip))) + { + debug!( + "Skipping reassignment of already assigned valid IP {ip} for device {} in network {} with addresses {:?}", + self.name, network.name, network.address + ); + ips.push(*ip); + continue; + } let mut picked = None; for ip in address { if network diff --git a/crates/defguard_core/src/db/models/wireguard.rs b/crates/defguard_core/src/db/models/wireguard.rs index 6b54ba8ec5..7da00773b8 100644 --- a/crates/defguard_core/src/db/models/wireguard.rs +++ b/crates/defguard_core/src/db/models/wireguard.rs @@ -436,7 +436,7 @@ impl WireguardNetwork { let devices = self.get_allowed_devices(&mut *transaction).await?; for device in devices { device - .assign_next_network_ip(&mut *transaction, self, None) + .assign_next_network_ip(&mut *transaction, self, None, None) .await?; } Ok(()) @@ -454,7 +454,7 @@ impl WireguardNetwork { let allowed_device_ids: Vec = allowed_devices.iter().map(|dev| dev.id).collect(); if allowed_device_ids.contains(&device.id) { let wireguard_network_device = device - .assign_next_network_ip(&mut *transaction, self, reserved_ips) + .assign_next_network_ip(&mut *transaction, self, reserved_ips, None) .await?; Ok(wireguard_network_device) } else { @@ -475,8 +475,8 @@ impl WireguardNetwork { self.address.iter().find(|net| net.contains(addr)).copied() } - /// Works out which devices need to be added, removed, or readdressed - /// based on the list of currently configured devices and the list of devices which should be allowed + /// Works out which devices need to be added, removed, or readdressed based on the list + /// of currently configured devices and the list of devices which should be allowed. async fn process_device_access_changes( &self, transaction: &mut PgConnection, @@ -484,18 +484,24 @@ impl WireguardNetwork { currently_configured_devices: Vec, reserved_ips: Option<&[IpAddr]>, ) -> Result, WireguardNetworkError> { - // Loop through current device configurations; remove no longer allowed, readdress when necessary; remove processed entry from all devices list - // initial list should now contain only devices to be added + // Loop through current device configurations; remove no longer allowed, readdress + // when necessary; remove processed entry from all devices list initial list should + // now contain only devices to be added. let mut events: Vec = Vec::new(); for device_network_config in currently_configured_devices { // Device is allowed and an IP was already assigned if let Some(device) = allowed_devices.remove(&device_network_config.device_id) { - // Network address changed and IP addresses need to be updated + // Network address has changed and IP addresses need to be updated if !self.contains_all(&device_network_config.wireguard_ips) || self.address.len() != device_network_config.wireguard_ips.len() { let wireguard_network_device = device - .assign_next_network_ip(&mut *transaction, self, reserved_ips) + .assign_next_network_ip( + &mut *transaction, + self, + reserved_ips, + Some(&device_network_config.wireguard_ips), + ) .await?; events.push(GatewayEvent::DeviceModified(DeviceInfo { device, @@ -537,7 +543,7 @@ impl WireguardNetwork { // Add configs for new allowed devices for device in allowed_devices.into_values() { let wireguard_network_device = device - .assign_next_network_ip(&mut *transaction, self, reserved_ips) + .assign_next_network_ip(&mut *transaction, self, reserved_ips, None) .await?; events.push(GatewayEvent::DeviceCreated(DeviceInfo { device, diff --git a/crates/defguard_core/src/lib.rs b/crates/defguard_core/src/lib.rs index 57350b6519..b56b4595a3 100644 --- a/crates/defguard_core/src/lib.rs +++ b/crates/defguard_core/src/lib.rs @@ -736,7 +736,7 @@ pub async fn init_dev_env(config: &DefGuardConfig) { .await .expect("Could not save device"); device - .assign_next_network_ip(&mut transaction, &network, None) + .assign_next_network_ip(&mut transaction, &network, None, None) .await .expect("Could not assign IP to device"); } From b62f32d7fc9e85a9aafab6fd3d357d062a7e7c81 Mon Sep 17 00:00:00 2001 From: Jacek Chmielewski Date: Thu, 26 Jun 2025 10:22:21 +0200 Subject: [PATCH 2/2] Add integration test for network address reassignment --- .../tests/integration/wireguard.rs | 116 ++++++++++++++++++ 1 file changed, 116 insertions(+) diff --git a/crates/defguard_core/tests/integration/wireguard.rs b/crates/defguard_core/tests/integration/wireguard.rs index 93e99afb09..d1c8c715bb 100644 --- a/crates/defguard_core/tests/integration/wireguard.rs +++ b/crates/defguard_core/tests/integration/wireguard.rs @@ -1,3 +1,5 @@ +use std::net::{IpAddr, Ipv4Addr, Ipv6Addr}; + use defguard_core::{ db::{ models::{ @@ -275,6 +277,120 @@ async fn test_device(_: PgPoolOptions, options: PgConnectOptions) { assert!(devices.is_empty()); } +#[sqlx::test] +async fn test_network_address_reassignment(_: PgPoolOptions, options: PgConnectOptions) { + let pool = setup_pool(options).await; + + let (client, client_state) = make_test_client(pool).await; + + let auth = Auth::new("admin", "pass123"); + let response = &client.post("/api/v1/auth").json(&auth).send().await; + assert_eq!(response.status(), StatusCode::OK); + + // create network + let network = json!({ + "name": "network", + "address": "10.1.1.1/24", + "port": 55555, + "endpoint": "192.168.4.14", + "allowed_ips": "10.1.1.0/24", + "dns": "1.1.1.1", + "allowed_groups": [], + "mfa_enabled": false, + "keepalive_interval": 25, + "peer_disconnect_threshold": 180, + "acl_enabled": false, + "acl_default_allow": false + }); + let response = client.post("/api/v1/network").json(&network).send().await; + assert_eq!(response.status(), StatusCode::CREATED); + + // network details + let response = client.get("/api/v1/network/1").send().await; + assert_eq!(response.status(), StatusCode::OK); + let network_from_details: WireguardNetwork = response.json().await; + + // create devices + let device = json!({ + "name": "device1", + "wireguard_pubkey": "LQKsT6/3HWKuJmMulH63R8iK+5sI8FyYEL6WDIi6lQU=", + }); + let response = client + .post("/api/v1/device/admin") + .json(&device) + .send() + .await; + assert_eq!(response.status(), StatusCode::CREATED); + let device = json!({ + "name": "device2", + "wireguard_pubkey": "ZqDlG4LQZRO9v57Sd27AHdtTLxegbMp5oVThjYrg21I=", + }); + let response = client + .post("/api/v1/device/admin") + .json(&device) + .send() + .await; + assert_eq!(response.status(), StatusCode::CREATED); + + // ensure IPs were assigned for new devices + let network_devices = WireguardNetworkDevice::find_by_device(&client_state.pool, 1) + .await + .unwrap() + .unwrap(); + assert_eq!( + network_devices[0].wireguard_ips, + vec![IpAddr::V4(Ipv4Addr::new(10, 1, 1, 2))], + ); + let network_devices = WireguardNetworkDevice::find_by_device(&client_state.pool, 2) + .await + .unwrap() + .unwrap(); + assert_eq!( + network_devices[0].wireguard_ips, + vec![IpAddr::V4(Ipv4Addr::new(10, 1, 1, 3))], + ); + + // delete the first device + let response = client.delete("/api/v1/device/1").json(&device).send().await; + assert_eq!(response.status(), StatusCode::OK); + + // modify network addresses + let network = json!({ + "id": network_from_details.id, + "name": "network", + "address": "10.1.1.1/24,fc00::1/112", + "port": 55555, + "endpoint": "192.168.4.14", + "allowed_ips": "10.1.1.0/24", + "dns": "1.1.1.1", + "allowed_groups": [], + "mfa_enabled": false, + "keepalive_interval": 25, + "peer_disconnect_threshold": 180, + "acl_enabled": false, + "acl_default_allow": false + }); + let response = client + .put(format!("/api/v1/network/{}", network_from_details.id)) + .json(&network) + .send() + .await; + assert_eq!(response.status(), StatusCode::OK); + + // ensure IPv4 address wasn't reassigned + let network_devices = WireguardNetworkDevice::find_by_device(&client_state.pool, 2) + .await + .unwrap() + .unwrap(); + assert_eq!( + network_devices[0].wireguard_ips, + vec![ + IpAddr::V4(Ipv4Addr::new(10, 1, 1, 3)), + IpAddr::V6(Ipv6Addr::new(0xfc00, 0, 0, 0, 0, 0, 0, 2)), + ], + ); +} + #[sqlx::test] async fn test_device_permissions(_: PgPoolOptions, options: PgConnectOptions) { let pool = setup_pool(options).await;