diff --git a/crates/defguard_common/src/db/models/device.rs b/crates/defguard_common/src/db/models/device.rs index e1a143032..a5831c64d 100644 --- a/crates/defguard_common/src/db/models/device.rs +++ b/crates/defguard_common/src/db/models/device.rs @@ -1,4 +1,4 @@ -use std::{fmt, net::IpAddr}; +use std::{collections::HashSet, fmt, net::IpAddr}; use base64::{Engine, prelude::BASE64_STANDARD}; use chrono::{NaiveDate, NaiveDateTime, Timelike, Utc}; @@ -843,6 +843,7 @@ impl Device { &self, transaction: &mut PgConnection, network: &WireguardNetwork, + used_ips: &HashSet, reserved_ips: Option<&[IpAddr]>, current_ips: Option<&[IpAddr]>, ) -> Result { @@ -872,15 +873,16 @@ impl Device { } let mut picked = None; for ip in address { - if network - .can_assign_ips(transaction, &[ip], Some(self.id)) - .await - .is_ok() - && !reserved.contains(&ip) - { - picked = Some(ip); - break; + if ip == address.network() || ip == address.broadcast() || ip == address.ip() { + continue; } + + if used_ips.contains(&ip) || reserved.contains(&ip) { + continue; + } + + picked = Some(ip); + break; } // Return error if no address can be assigned @@ -1129,6 +1131,265 @@ mod test { assert!(device.is_err()); } + /// Test that assign_next_network_ip correctly preserves or reassigns device IPs + /// when a network's address list changes. + /// Initial network: 10.0.0.0/8, 123.10.0.0/16, 123.123.123.0/24 + /// Device IPs: 10.0.0.234, 123.10.33.44, 123.123.123.52 + /// New network: 10.0.0.0/16, 123.12.0.0/16, 123.123.0.0/16 + /// Expected: + /// - 10.0.0.234 KEPT (still within 10.0.0.0/16) + /// - 123.10.33.44 CHANGED (not within 123.12.0.0/16) + /// - 123.123.123.52 KEPT (still within 123.123.0.0/16) + #[sqlx::test] + async fn test_assign_next_network_ip_preserves_matching_subnets( + _: PgPoolOptions, + options: PgConnectOptions, + ) { + let pool = setup_pool(options).await; + + let mut network = WireguardNetwork::default(); + network + .try_set_address("10.0.0.1/8,123.10.0.1/16,123.123.123.1/24") + .unwrap(); + let network = network.save(&pool).await.unwrap(); + + let user = User::new( + "testuser", + Some("password"), + "Tester", + "Test", + "test@test.com", + None, + ) + .save(&pool) + .await + .unwrap(); + + let device = Device::new( + "dev1".into(), + "key1".into(), + user.id, + DeviceType::User, + None, + true, + ) + .save(&pool) + .await + .unwrap(); + + let ip = IpAddr::from_str("10.0.0.234").unwrap(); + let ip2 = IpAddr::from_str("123.10.33.44").unwrap(); + let ip3 = IpAddr::from_str("123.123.123.52").unwrap(); + let initial_ips = vec![ip, ip2, ip3]; + + let mut conn = pool.acquire().await.unwrap(); + WireguardNetworkDevice::new(network.id, device.id, initial_ips.clone()) + .insert(&mut *conn) + .await + .unwrap(); + + let mut updated_network = network.clone(); + updated_network.address = vec![ + "10.0.0.0/16".parse::().unwrap(), + "123.12.0.0/16".parse::().unwrap(), + "123.123.0.0/16".parse::().unwrap(), + ]; + updated_network.save(&mut *conn).await.unwrap(); + + let used_ips = updated_network + .all_used_ips_for_network(&mut conn) + .await + .unwrap(); + + let result = device + .assign_next_network_ip( + &mut conn, + &updated_network, + &used_ips, + None, + Some(&initial_ips), + ) + .await + .unwrap(); + + let new_ips = &result.wireguard_ips; + assert_eq!(new_ips.len(), 3, "should have one IP per subnet"); + + assert!( + new_ips.contains(&ip), + "10.0.0.234 should be kept – it is still within 10.0.0.0/16; got {new_ips:?}" + ); + + assert!( + !new_ips.contains(&ip2), + "123.10.33.44 should be reassigned – not within 123.12.0.0/16; got {new_ips:?}" + ); + let network: IpNetwork = "123.12.0.0/16".parse().unwrap(); + assert!( + new_ips.iter().any(|ip| network.contains(*ip)), + "a new IP within 123.12.0.0/16 should be assigned; got {new_ips:?}" + ); + + assert!( + new_ips.contains(&ip3), + "123.123.123.52 should be kept – it is still within 123.123.0.0/16; got {new_ips:?}" + ); + } + /// Initial: 10.0.0.0/8 | 10.1.0.5 + /// Modified: 10.0.0.0/16 | 10.1.0.5 should be replaced with a 10.0.x.x address + #[sqlx::test] + async fn test_assign_next_network_ip_subnet_narrowed( + _: PgPoolOptions, + options: PgConnectOptions, + ) { + let pool = setup_pool(options).await; + + let mut network = WireguardNetwork::default(); + network.try_set_address("10.0.0.1/8").unwrap(); + let network = network.save(&pool).await.unwrap(); + + let user = User::new( + "testuser", + Some("password"), + "Tester", + "Test", + "test@test.com", + None, + ) + .save(&pool) + .await + .unwrap(); + + let device = Device::new( + "dev1".into(), + "key1".into(), + user.id, + DeviceType::User, + None, + true, + ) + .save(&pool) + .await + .unwrap(); + + let ip = IpAddr::from_str("10.1.0.5").unwrap(); + let initial_ips = vec![ip]; + + let mut conn = pool.acquire().await.unwrap(); + WireguardNetworkDevice::new(network.id, device.id, initial_ips.clone()) + .insert(&mut *conn) + .await + .unwrap(); + + let mut updated_network = network.clone(); + updated_network.address = vec!["10.0.0.0/16".parse::().unwrap()]; + updated_network.save(&mut *conn).await.unwrap(); + + let used_ips = updated_network + .all_used_ips_for_network(&mut conn) + .await + .unwrap(); + + let result = device + .assign_next_network_ip( + &mut conn, + &updated_network, + &used_ips, + None, + Some(&initial_ips), + ) + .await + .unwrap(); + + let new_ips = &result.wireguard_ips; + assert_eq!(new_ips.len(), 1, "should have one IP per subnet"); + + assert!( + !new_ips.contains(&ip), + "10.1.0.5 should be reassigned – outside narrowed 10.0.0.0/16; got {new_ips:?}" + ); + let narrowed_net: IpNetwork = "10.0.0.0/16".parse().unwrap(); + assert!( + new_ips.iter().all(|ip| narrowed_net.contains(*ip)), + "new IP must be within 10.0.0.0/16; got {new_ips:?}" + ); + } + + /// Initial: 123.123.123.0/24 | 123.123.123.254 + /// Modified: 123.123.0.0/16 | 123.123.123.254 still fits + #[sqlx::test] + async fn test_assign_next_network_ip_still_valid_after_widening( + _: PgPoolOptions, + options: PgConnectOptions, + ) { + let pool = setup_pool(options).await; + + let mut network = WireguardNetwork::default(); + network.try_set_address("123.123.123.1/24").unwrap(); + let network = network.save(&pool).await.unwrap(); + + let user = User::new( + "testuser", + Some("password"), + "Tester", + "Test", + "test@test.com", + None, + ) + .save(&pool) + .await + .unwrap(); + + let device = Device::new( + "dev1".into(), + "key1".into(), + user.id, + DeviceType::User, + None, + true, + ) + .save(&pool) + .await + .unwrap(); + + let ip = IpAddr::from_str("123.123.123.254").unwrap(); + let initial_ips = vec![ip]; + + let mut conn = pool.acquire().await.unwrap(); + WireguardNetworkDevice::new(network.id, device.id, initial_ips.clone()) + .insert(&mut *conn) + .await + .unwrap(); + + let mut updated_network = network.clone(); + updated_network.address = vec!["123.123.0.0/16".parse::().unwrap()]; + updated_network.save(&mut *conn).await.unwrap(); + + let used_ips = updated_network + .all_used_ips_for_network(&mut conn) + .await + .unwrap(); + + let result = device + .assign_next_network_ip( + &mut conn, + &updated_network, + &used_ips, + None, + Some(&initial_ips), + ) + .await + .unwrap(); + + let new_ips = &result.wireguard_ips; + assert_eq!(new_ips.len(), 1, "should have one IP per subnet"); + + assert!( + new_ips.contains(&ip), + "123.123.123.254 should be preserved – still within widened 123.123.0.0/16; got {new_ips:?}" + ); + } + #[test] fn test_pubkey_validation() { let invalid_test_key = "invalid_key"; diff --git a/crates/defguard_common/src/db/models/wireguard.rs b/crates/defguard_common/src/db/models/wireguard.rs index 4ca4998c5..0f687ea7f 100644 --- a/crates/defguard_common/src/db/models/wireguard.rs +++ b/crates/defguard_common/src/db/models/wireguard.rs @@ -1,5 +1,5 @@ use std::{ - collections::HashMap, + collections::{HashMap, HashSet}, fmt::{self, Display}, iter::zip, net::{IpAddr, Ipv4Addr}, @@ -438,10 +438,11 @@ impl WireguardNetwork { "Assigning IPs in network {} for all existing devices ", self ); + let used_ips = self.all_used_ips_for_network(&mut *transaction).await?; let devices = self.get_allowed_devices(&mut *transaction).await?; for device in devices { device - .assign_next_network_ip(&mut *transaction, self, None, None) + .assign_next_network_ip(&mut *transaction, self, &used_ips, None, None) .await?; } Ok(()) @@ -457,9 +458,11 @@ impl WireguardNetwork { info!("Assigning IP in network {self} for {device}"); let allowed_devices = self.get_allowed_devices(&mut *transaction).await?; let allowed_device_ids: Vec = allowed_devices.iter().map(|dev| dev.id).collect(); + let used_ips = self.all_used_ips_for_network(&mut *transaction).await?; + if allowed_device_ids.contains(&device.id) { let wireguard_network_device = device - .assign_next_network_ip(&mut *transaction, self, reserved_ips, None) + .assign_next_network_ip(&mut *transaction, self, &used_ips, reserved_ips, None) .await?; Ok(wireguard_network_device) } else { @@ -533,9 +536,7 @@ impl WireguardNetwork { // split into separate stats for each device let mut device_stats: HashMap> = stats.into_iter().fold(HashMap::new(), |mut acc, item| { - acc.entry(item.device_id) - .or_insert_with(Vec::new) - .push(item); + acc.entry(item.device_id).or_default().push(item); acc }); @@ -1354,6 +1355,20 @@ impl WireguardNetwork { .fetch_all(executor) .await } + + /// Obtain all used ips for network + pub async fn all_used_ips_for_network( + &self, + transaction: &mut PgConnection, + ) -> Result, SqlxError> { + let all_devices = + WireguardNetworkDevice::all_for_network(&mut *transaction, self.id).await?; + let used_ips: HashSet = all_devices + .into_iter() + .flat_map(|device| device.wireguard_ips) + .collect(); + Ok(used_ips) + } } // [`IpNetwork`] does not implement [`Default`] diff --git a/crates/defguard_core/src/handlers/wireguard.rs b/crates/defguard_core/src/handlers/wireguard.rs index 3a8adda26..fa6d02b9a 100644 --- a/crates/defguard_core/src/handlers/wireguard.rs +++ b/crates/defguard_core/src/handlers/wireguard.rs @@ -328,17 +328,6 @@ pub(crate) async fn modify_network( let before = network.clone(); let new_addresses = data.parse_addresses()?; - // Block network address changes if any device is assigned to the network - if before.address != new_addresses - && WireguardNetworkDevice::has_devices_in_network(&appstate.pool, network_id).await? - { - return Err(WebError::BadRequest( - "Cannot change network address while devices are assigned to this network. \ - Remove all devices first." - .into(), - )); - } - network.address = new_addresses; network.allowed_ips = data.parse_allowed_ips(); network.name = data.name; diff --git a/crates/defguard_core/src/lib.rs b/crates/defguard_core/src/lib.rs index bc6ae24ca..849e119b7 100644 --- a/crates/defguard_core/src/lib.rs +++ b/crates/defguard_core/src/lib.rs @@ -738,7 +738,10 @@ pub async fn init_dev_env(config: &DefGuardConfig) { .await .expect("Could not save network") }; - + let used_ips = network + .all_used_ips_for_network(&mut transaction) + .await + .expect("Failed to query used ip's from database"); if Device::find_by_pubkey( &mut *transaction, "gQYL5eMeFDj0R+lpC7oZyIl0/sNVmQDC6ckP7husZjc=", @@ -762,7 +765,7 @@ pub async fn init_dev_env(config: &DefGuardConfig) { .await .expect("Could not save device"); device - .assign_next_network_ip(&mut transaction, &network, None, None) + .assign_next_network_ip(&mut transaction, &network, &used_ips, None, None) .await .expect("Could not assign IP to device"); } diff --git a/crates/defguard_core/src/location_management/mod.rs b/crates/defguard_core/src/location_management/mod.rs index 400be93f6..51bac5237 100644 --- a/crates/defguard_core/src/location_management/mod.rs +++ b/crates/defguard_core/src/location_management/mod.rs @@ -165,6 +165,7 @@ pub async fn process_device_access_changes( // 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 used_ips = location.all_used_ips_for_network(&mut *transaction).await?; let mut events: Vec = Vec::new(); for device_network_config in currently_configured_devices { // Device is allowed and an IP was already assigned @@ -177,6 +178,7 @@ pub async fn process_device_access_changes( .assign_next_network_ip( &mut *transaction, location, + &used_ips, reserved_ips, Some(&device_network_config.wireguard_ips), ) @@ -217,11 +219,10 @@ pub async fn process_device_access_changes( } } } - // Add configs for new allowed devices for device in allowed_devices.into_values() { let wireguard_network_device = device - .assign_next_network_ip(&mut *transaction, location, reserved_ips, None) + .assign_next_network_ip(&mut *transaction, location, &used_ips, reserved_ips, None) .await?; events.push(GatewayEvent::DeviceCreated(DeviceInfo { device, diff --git a/crates/defguard_core/tests/integration/api/wireguard.rs b/crates/defguard_core/tests/integration/api/wireguard.rs index ec2591b82..c29353019 100644 --- a/crates/defguard_core/tests/integration/api/wireguard.rs +++ b/crates/defguard_core/tests/integration/api/wireguard.rs @@ -533,7 +533,7 @@ async fn test_network_address_reassignment(_: PgPoolOptions, options: PgConnectO vec![IpAddr::V4(Ipv4Addr::new(10, 1, 1, 3))], ); - // trying to modify network addresses while devices exist should fail + // trying to modify network addresses while devices exist shouldn't fail let network = json!({ "id": network_id, "name": "network", @@ -557,7 +557,7 @@ async fn test_network_address_reassignment(_: PgPoolOptions, options: PgConnectO .json(&network) .send() .await; - assert_eq!(response.status(), StatusCode::BAD_REQUEST); + assert_eq!(response.status(), StatusCode::OK); // delete both devices let response = client @@ -571,14 +571,6 @@ async fn test_network_address_reassignment(_: PgPoolOptions, options: PgConnectO .await; assert_eq!(response.status(), StatusCode::OK); - // now modify network addresses should succeed - let response = client - .put(format!("/api/v1/network/{network_id}")) - .json(&network) - .send() - .await; - assert_eq!(response.status(), StatusCode::OK); - // re-create a device and verify it gets IPs in both subnets let device = json!({ "name": "device3", @@ -920,85 +912,3 @@ async fn test_network_size_validation(_: PgPoolOptions, options: PgConnectOption .await; assert_eq!(response.status(), StatusCode::BAD_REQUEST); } - -/// Test that modifying a network's address is blocked when any devices are assigned. -/// Also verifies that non-address modifications still succeed. -#[sqlx::test] -async fn test_modify_network_blocked_by_devices(_: 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 response = make_network(&client, "network").await; - let network: WireguardNetwork = response.json().await; - - // create a device for the admin user — it gets auto-assigned to the network - 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); - - // try to modify the network address — should be rejected because a device exists - let modified = json!({ - "name": "network", - "address": "10.2.2.1/24", - "port": 55555, - "endpoint": "192.168.4.14", - "allowed_ips": "10.2.2.0/24", - "dns": "1.1.1.1", - "mtu": 1420, - "fwmark": 0, - "allowed_groups": [], - "keepalive_interval": 25, - "peer_disconnect_threshold": 300, - "acl_enabled": false, - "acl_default_allow": false, - "location_mfa_mode": "disabled", - "service_location_mode": "disabled" - }); - let response = client - .put(format!("/api/v1/network/{}", network.id)) - .json(&modified) - .send() - .await; - assert_eq!(response.status(), StatusCode::BAD_REQUEST); - - let body: serde_json::Value = response.json().await; - assert!(body["msg"].as_str().is_some()); - - // verify that modifying other fields (not address) still works - let modified_name_only = json!({ - "name": "renamed-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", - "mtu": 1420, - "fwmark": 0, - "allowed_groups": [], - "keepalive_interval": 25, - "peer_disconnect_threshold": 300, - "acl_enabled": false, - "acl_default_allow": false, - "location_mfa_mode": "disabled", - "service_location_mode": "disabled" - }); - let response = client - .put(format!("/api/v1/network/{}", network.id)) - .json(&modified_name_only) - .send() - .await; - assert_eq!(response.status(), StatusCode::OK); -} diff --git a/tools/defguard_generator/src/vpn_session_stats.rs b/tools/defguard_generator/src/vpn_session_stats.rs index a134f6b04..f9ded71e0 100644 --- a/tools/defguard_generator/src/vpn_session_stats.rs +++ b/tools/defguard_generator/src/vpn_session_stats.rs @@ -70,6 +70,7 @@ pub async fn generate_vpn_session_stats( let devices = prepare_user_devices(&pool, &mut rng, &user, config.devices_per_user as usize).await?; + let used_ips = location.all_used_ips_for_network(&mut *transaction).await?; // assign devices to the network if not already assigned for device in &devices { if WireguardNetworkDevice::find(&mut *transaction, device.id, location.id) @@ -81,7 +82,7 @@ pub async fn generate_vpn_session_stats( device.name, location.name ); device - .assign_next_network_ip(&mut transaction, &location, None, None) + .assign_next_network_ip(&mut transaction, &location, &used_ips, None, None) .await?; } else { info!( diff --git a/web/messages/en/location.json b/web/messages/en/location.json index cd02cae66..b26fc64ed 100644 --- a/web/messages/en/location.json +++ b/web/messages/en/location.json @@ -8,5 +8,5 @@ "add_location_mfa_external_content": "Requires configuring an external identity provider in the settings, such as Google, Microsoft Entra ID, Okta, or JumpCloud.", "location_delete_success": "Location deleted", "location_delete_failed": "Failed to delete location", "location_edit_failed": "Failed to update location", - "location_edit_failed_has_devices": "Gateway VPN IP address and netmask can’t be changed while devices are active on this network. To proceed, remove all devices from the current network first." + "location_edit_addresses_rewrite_warning": "Changing the Gateway VPN IP address or netmask will automatically reassign any device addresses that fall outside the new network range to a randomly selected available IP address." } diff --git a/web/src/pages/EditLocationPage/EditLocationPage.tsx b/web/src/pages/EditLocationPage/EditLocationPage.tsx index 86804963f..d32dfc683 100644 --- a/web/src/pages/EditLocationPage/EditLocationPage.tsx +++ b/web/src/pages/EditLocationPage/EditLocationPage.tsx @@ -190,9 +190,9 @@ const EditLocationForm = ({ location }: { location: NetworkLocation }) => { const defaultValues = useMemo( (): FormFields => ({ name: location.name, - address: location.address.join(', '), + address: location.address.join(','), allowed_groups: location.allowed_groups, - allowed_ips: location.allowed_ips.join(', '), + allowed_ips: location.allowed_ips.join(','), dns: location.dns, endpoint: location.endpoint, keepalive_interval: location.keepalive_interval, @@ -260,18 +260,14 @@ const EditLocationForm = ({ location }: { location: NetworkLocation }) => { )} {(field) => ( - + )}