From 041c65a34fbc10e30beb911b6642e8798e1d3918 Mon Sep 17 00:00:00 2001 From: Jacek Chmielewski Date: Mon, 3 Aug 2026 10:50:34 +0200 Subject: [PATCH 01/13] posture check authenticates the device with polling token --- .../src/enterprise/posture/tests.rs | 2 + .../src/grpc/proxy/client_mfa.rs | 219 ++++++++++++++++++ proto | 2 +- 3 files changed, 222 insertions(+), 1 deletion(-) diff --git a/crates/defguard_core/src/enterprise/posture/tests.rs b/crates/defguard_core/src/enterprise/posture/tests.rs index 19c6b3ebb..0b1127a09 100644 --- a/crates/defguard_core/src/enterprise/posture/tests.rs +++ b/crates/defguard_core/src/enterprise/posture/tests.rs @@ -122,6 +122,8 @@ fn make_request(location_id: Id, data: Option) -> DevicePostu location_id, pubkey: "testpubkey".to_owned(), device_posture_data: data, + // `validate_posture` does not authenticate; the token is checked by the caller. + token: None, } } diff --git a/crates/defguard_core/src/grpc/proxy/client_mfa.rs b/crates/defguard_core/src/grpc/proxy/client_mfa.rs index 2cab09137..627ff9edf 100644 --- a/crates/defguard_core/src/grpc/proxy/client_mfa.rs +++ b/crates/defguard_core/src/grpc/proxy/client_mfa.rs @@ -12,6 +12,7 @@ use defguard_common::{ models::{ BiometricAuth, BiometricChallenge, Device, User, WireguardNetwork, device::{DeviceNetworkInfo, WireguardNetworkDevice}, + polling_token::PollingToken, vpn_client_session::{VpnClientMfaMethod, VpnClientSession, VpnClientSessionState}, wireguard::LocationMfaMode, }, @@ -226,6 +227,9 @@ impl ClientMfaServer { location_id: location.id, pubkey: request.pubkey.clone(), device_posture_data: request.posture_data.clone(), + // Only used to reach `validate_posture`, which ignores the token. This request is + // never dispatched, and the MFA flow authenticates the caller by its own means. + token: None, }; let posture_result = match validate_posture(&self.pool, &posture_request).await { Ok(result) => result, @@ -877,6 +881,30 @@ impl ClientMfaServer { request.pubkey, request.location_id ); + // Authenticate the caller before touching anything else. + // Validated first so that an unauthenticated caller cannot use the error codes below to + // probe which locations exist or which public keys are enrolled. + let Some(token) = request.token.as_deref().filter(|token| !token.is_empty()) else { + error!( + "Posture check: missing polling token for pubkey {}", + request.pubkey + ); + return Err(Status::unauthenticated("missing token")); + }; + let polling_token = PollingToken::find(&self.pool, token) + .await + .map_err(|err| { + error!("Posture check: failed to look up polling token: {err}"); + Status::internal("unexpected error") + })? + .ok_or_else(|| { + error!( + "Posture check: unknown polling token for claimed pubkey {}", + request.pubkey + ); + Status::unauthenticated("invalid token") + })?; + // Look up location, device, and user. let Ok(Some(location)) = WireguardNetwork::find_by_id(&self.pool, request.location_id).await @@ -900,6 +928,16 @@ impl ClientMfaServer { return Err(Status::invalid_argument("device not found")); }; + // Make sure caller owns the device. + if polling_token.device_id != device.id { + error!( + "Posture check: polling token belongs to device {} but request claims pubkey {} \ + (device {})", + polling_token.device_id, request.pubkey, device.id + ); + return Err(Status::unauthenticated("token does not match device")); + } + if !location.has_postures(&self.pool).await.map_err(|err| { error!("Posture check: failed to fetch postures for location {location}: {err}"); Status::internal("unexpected error") @@ -1165,6 +1203,7 @@ mod tests { models::{ Device, DeviceType, User, WireguardNetwork, device::WireguardNetworkDevice, + polling_token::PollingToken, settings::initialize_current_settings, vpn_client_session::{VpnClientMfaMethod, VpnClientSession, VpnClientSessionState}, wireguard::{LocationMfaMode, ServiceLocationMode}, @@ -1213,6 +1252,7 @@ mod tests { let user = create_user(&pool).await; let device = create_device(&pool, user.id).await; attach_device_to_location(&pool, location.id, device.id).await; + let token = create_polling_token(&pool, device.id).await; let (mut server, _event_rx, mut gateway_rx) = make_server(pool.clone()); let outcome = server @@ -1220,6 +1260,7 @@ mod tests { location_id: location.id, pubkey: device.wireguard_pubkey.clone(), device_posture_data: Some(passing_linux_posture_data()), + token: Some(token.clone()), }) .await .expect("posture check should pass"); @@ -1289,6 +1330,7 @@ mod tests { .save(&pool) .await .expect("failed to create previous posture session"); + let token = create_polling_token(&pool, device.id).await; let (mut server, mut event_rx, mut gateway_rx) = make_server(pool.clone()); server @@ -1296,6 +1338,7 @@ mod tests { location_id: location.id, pubkey: device.wireguard_pubkey.clone(), device_posture_data: Some(passing_linux_posture_data()), + token: Some(token.clone()), }) .await .expect("replacement posture check should pass"); @@ -1350,6 +1393,165 @@ mod tests { assert_eq!(old_session.state, VpnClientSessionState::Disconnected); } + /// A caller with no token must be refused. Without this, knowing a device's public key is + /// enough to mint a preshared key for it. + #[sqlx::test] + async fn test_posture_check_requires_a_token(_: PgPoolOptions, options: PgConnectOptions) { + set_enterprise_license(); + let pool = setup_pool(options).await; + initialize_current_settings(&pool) + .await + .expect("failed to init settings"); + let location = create_non_mfa_location(&pool).await; + save_linux_posture_policy(&pool, location.id).await; + let user = create_user(&pool).await; + let device = create_device(&pool, user.id).await; + attach_device_to_location(&pool, location.id, device.id).await; + let (mut server, _, mut gateway_rx) = make_server(pool.clone()); + + for token in [None, Some(String::new())] { + let err = server + .handle_posture_check(DevicePostureCheckRequest { + location_id: location.id, + pubkey: device.wireguard_pubkey.clone(), + device_posture_data: Some(passing_linux_posture_data()), + token, + }) + .await; + let err = match err { + Ok(_) => panic!("posture check without a token must be refused"), + Err(err) => err, + }; + assert_eq!(err.code(), Code::Unauthenticated); + } + + // No session may be created and the gateway must not be touched. + assert!( + VpnClientSession::get_all_active_device_sessions_in_location( + &pool, + location.id, + device.id + ) + .await + .expect("failed to query sessions") + .is_empty() + ); + assert!(gateway_rx.try_recv().is_err()); + } + + /// An unknown token must be refused, so tokens cannot be guessed or replayed after rotation. + #[sqlx::test] + async fn test_posture_check_rejects_unknown_token(_: PgPoolOptions, options: PgConnectOptions) { + set_enterprise_license(); + let pool = setup_pool(options).await; + initialize_current_settings(&pool) + .await + .expect("failed to init settings"); + let location = create_non_mfa_location(&pool).await; + save_linux_posture_policy(&pool, location.id).await; + let user = create_user(&pool).await; + let device = create_device(&pool, user.id).await; + attach_device_to_location(&pool, location.id, device.id).await; + let (mut server, _, _) = make_server(pool); + + let err = server + .handle_posture_check(DevicePostureCheckRequest { + location_id: location.id, + pubkey: device.wireguard_pubkey.clone(), + device_posture_data: Some(passing_linux_posture_data()), + token: Some("not-a-real-token".to_owned()), + }) + .await; + let err = match err { + Ok(_) => panic!("posture check with an unknown token must be refused"), + Err(err) => err, + }; + + assert_eq!(err.code(), Code::Unauthenticated); + } + + /// Regression test for the session-hijack denial of service: holding a valid token for *one* + /// device must not allow authorizing — and thereby superseding the live session of — another. + #[sqlx::test] + async fn test_posture_check_rejects_token_belonging_to_another_device( + _: PgPoolOptions, + options: PgConnectOptions, + ) { + set_enterprise_license(); + let pool = setup_pool(options).await; + initialize_current_settings(&pool) + .await + .expect("failed to init settings"); + let location = create_non_mfa_location(&pool).await; + save_linux_posture_policy(&pool, location.id).await; + let user = create_user(&pool).await; + + let victim = create_device(&pool, user.id).await; + attach_device_to_location(&pool, location.id, victim.id).await; + + // The attacker is a legitimately enrolled device with a token of its own. + let attacker = Device::new( + "attacker-device".to_owned(), + "attacker-pubkey".to_owned(), + user.id, + DeviceType::User, + None, + true, + ) + .save(&pool) + .await + .expect("failed to create attacker device"); + let attacker_token = create_polling_token(&pool, attacker.id).await; + + // The victim holds a live session. + let mut victim_session = VpnClientSession::new( + location.id, + user.id, + victim.id, + Some(Utc::now().naive_utc()), + None, + ); + victim_session.preshared_key = Some("victim-psk".to_owned()); + victim_session.state = VpnClientSessionState::Connected; + let victim_session = victim_session + .save(&pool) + .await + .expect("failed to create victim session"); + + let (mut server, _, mut gateway_rx) = make_server(pool.clone()); + + // Attacker presents its own valid token but claims the victim's public key. + let err = server + .handle_posture_check(DevicePostureCheckRequest { + location_id: location.id, + pubkey: victim.wireguard_pubkey.clone(), + device_posture_data: Some(passing_linux_posture_data()), + token: Some(attacker_token), + }) + .await; + let err = match err { + Ok(_) => panic!("a token from another device must not authorize this one"), + Err(err) => err, + }; + assert_eq!(err.code(), Code::Unauthenticated); + + // The victim's session must survive untouched, and the gateway must see nothing. + let victim_session = VpnClientSession::find_by_id(&pool, victim_session.id) + .await + .expect("failed to reload victim session") + .expect("victim session should still exist"); + assert_eq!(victim_session.state, VpnClientSessionState::Connected); + assert_eq!( + victim_session.preshared_key.as_deref(), + Some("victim-psk"), + "the victim's preshared key must not have been rotated" + ); + assert!( + gateway_rx.try_recv().is_err(), + "no peer delete or re-create may be sent to the gateway" + ); + } + #[sqlx::test] async fn test_posture_check_rejects_mfa_enabled_location( _: PgPoolOptions, @@ -1357,6 +1559,10 @@ mod tests { ) { let pool = setup_pool(options).await; let location = create_mfa_location(&pool).await; + // A valid token is needed to get past authentication and reach the check under test. + let user = create_user(&pool).await; + let device = create_device(&pool, user.id).await; + let token = create_polling_token(&pool, device.id).await; let (mut server, _, _) = make_server(pool); let err = match server @@ -1364,6 +1570,7 @@ mod tests { location_id: location.id, pubkey: "irrelevant".to_owned(), device_posture_data: None, + token: Some(token), }) .await { @@ -1384,6 +1591,7 @@ mod tests { let user = create_user(&pool).await; let device = create_device(&pool, user.id).await; attach_device_to_location(&pool, location.id, device.id).await; + let token = create_polling_token(&pool, device.id).await; let (mut server, _, _) = make_server(pool); let err = match server @@ -1391,6 +1599,7 @@ mod tests { location_id: location.id, pubkey: device.wireguard_pubkey, device_posture_data: None, + token: Some(token), }) .await { @@ -1589,6 +1798,16 @@ mod tests { .expect("failed to create device") } + /// Issues a polling token for a device, as enrollment does. Posture checks require one to + /// authenticate the caller. + async fn create_polling_token(pool: &PgPool, device_id: Id) -> String { + PollingToken::new(device_id) + .save(pool) + .await + .expect("failed to create polling token") + .token + } + #[sqlx::test] async fn test_create_new_mfa_session_disconnects_previous_active_session( _: PgPoolOptions, diff --git a/proto b/proto index cbb798774..394b9dd04 160000 --- a/proto +++ b/proto @@ -1 +1 @@ -Subproject commit cbb798774a48e77940de33b8d6df7dae519541a4 +Subproject commit 394b9dd04b4e3f082326c2cec068b02ebbfdbd56 From a31e5ca4bba3b87d7c75e27a5411d96af397a6be Mon Sep 17 00:00:00 2001 From: Jacek Chmielewski Date: Tue, 4 Aug 2026 07:42:34 +0200 Subject: [PATCH 02/13] service-location-posture-checks proto --- proto | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/proto b/proto index 394b9dd04..b3fa07822 160000 --- a/proto +++ b/proto @@ -1 +1 @@ -Subproject commit 394b9dd04b4e3f082326c2cec068b02ebbfdbd56 +Subproject commit b3fa078221293983d7d8af9d1de834308653b95b From 589c317154ad49671bea0b552975fa64061b9a99 Mon Sep 17 00:00:00 2001 From: Jacek Chmielewski Date: Tue, 4 Aug 2026 08:25:32 +0200 Subject: [PATCH 03/13] allow service locations with posture checks --- .../src/enterprise/handlers/device_posture.rs | 16 - .../defguard_core/src/handlers/wireguard.rs | 47 ++- .../tests/integration/api/device_posture.rs | 58 ++- .../tests/integration/api/wireguard.rs | 388 +++++++++++++++++- 4 files changed, 468 insertions(+), 41 deletions(-) diff --git a/crates/defguard_core/src/enterprise/handlers/device_posture.rs b/crates/defguard_core/src/enterprise/handlers/device_posture.rs index fde1efb32..372527cab 100644 --- a/crates/defguard_core/src/enterprise/handlers/device_posture.rs +++ b/crates/defguard_core/src/enterprise/handlers/device_posture.rs @@ -1163,12 +1163,6 @@ pub async fn set_postures_for_location( .await? .ok_or_else(|| WebError::ObjectNotFound(format!("Location {location_id} not found")))?; - if location.is_service_location() && !data.postures.is_empty() { - return Err(WebError::BadRequest( - "Posture checks cannot be assigned to service locations".to_owned(), - )); - } - let mut tx = appstate.pool.begin().await?; let old_postures = DevicePostureLocation::find_by_location(&mut *tx, location_id).await?; let result = @@ -1234,16 +1228,6 @@ pub async fn set_locations_for_posture( WebError::ObjectNotFound(format!("Device posture check {posture_id} not found")) })?; - for location_id in &data.locations { - if let Some(location) = WireguardNetwork::find_by_id(&appstate.pool, *location_id).await? - && location.is_service_location() - { - return Err(WebError::BadRequest( - "Posture checks cannot be assigned to service locations".to_owned(), - )); - } - } - let mut tx = appstate.pool.begin().await?; let old_locations = DevicePostureLocation::find_by_posture(&mut *tx, posture_id).await?; let result = diff --git a/crates/defguard_core/src/handlers/wireguard.rs b/crates/defguard_core/src/handlers/wireguard.rs index 9c0b33bca..5e01dc8d2 100644 --- a/crates/defguard_core/src/handlers/wireguard.rs +++ b/crates/defguard_core/src/handlers/wireguard.rs @@ -139,6 +139,20 @@ impl WireguardNetworkData { Ok(()) } + /// Rejects service-location mode combined with location MFA: core cannot serve it and the + /// client cannot represent it (`Location::is_service_location()` requires MFA disabled). + pub(crate) fn validate_service_location_mfa(&self) -> Result<(), WebError> { + if self.service_location_mode == ServiceLocationMode::Disabled + || self.location_mfa_mode == LocationMfaMode::Disabled + { + return Ok(()); + } + + Err(WebError::BadRequest( + "Service location mode cannot be combined with location MFA".into(), + )) + } + pub(crate) fn validate_allowed_groups(&self) -> Result<(), WebError> { if self.allow_all_groups || !self.allowed_groups.is_empty() { return Ok(()); @@ -233,6 +247,7 @@ pub(crate) async fn create_network( data.validate_peer_disconnect_threshold()?; data.validate_location_mfa_mode(&appstate.pool).await?; + data.validate_service_location_mfa()?; data.validate_allowed_groups()?; let allowed_ips = data.parse_allowed_ips(); @@ -362,6 +377,7 @@ pub(crate) async fn modify_network( data.validate_peer_disconnect_threshold()?; data.validate_location_mfa_mode(&appstate.pool).await?; + data.validate_service_location_mfa()?; data.validate_allowed_groups()?; let network = find_network(network_id, &appstate.pool).await?; @@ -385,21 +401,34 @@ pub(crate) async fn modify_network( network.acl_enabled = data.acl_enabled; network.acl_default_allow = data.acl_default_allow; network.allowed_ips_from_acl = data.allowed_ips_from_acl; - network.service_location_mode = if data.location_mfa_mode == LocationMfaMode::Disabled { - data.service_location_mode - } else { - warn!( - "Disabling service location mode for location {} because location MFA is enabled", - network.name - ); - ServiceLocationMode::Disabled - }; + network.service_location_mode = data.service_location_mode; network.location_mfa_mode = data.location_mfa_mode; network.save(&mut *transaction).await?; network .set_allowed_groups(&mut transaction, &data.allowed_groups) .await?; + + // assign posture checks + // NOTE: this must happen before the allowed peers list is computed, since the peer list + // depends on whether the location has any posture checks assigned + if let Some(ref posture_checks) = data.posture_checks { + debug!("Assigning posture checks {posture_checks:?} to {network}"); + if !has_enterprise_access(Some(LicenseFeature::DevicePosture)) && !posture_checks.is_empty() + { + error!( + "Cannot assign posture checks to location {network}: Enterprise license required." + ); + return Ok(WebError::Forbidden( + "Cannot assign posture checks to location: Enterprise license required.", + ) + .into()); + } + DevicePostureLocation::set_for_location(&mut transaction, network.id, posture_checks) + .await?; + info!("Assigned posture checks {posture_checks:?} to location {network}"); + } + let _events = sync_location_allowed_devices(&network, &mut transaction, None).await?; let peers = get_location_allowed_peers(&network, &mut transaction).await?; diff --git a/crates/defguard_core/tests/integration/api/device_posture.rs b/crates/defguard_core/tests/integration/api/device_posture.rs index 3f802fc55..7fea42faf 100644 --- a/crates/defguard_core/tests/integration/api/device_posture.rs +++ b/crates/defguard_core/tests/integration/api/device_posture.rs @@ -1303,7 +1303,7 @@ async fn make_service_location(client: &TestClient, name: &str) -> i64 { } #[sqlx::test] -async fn test_set_postures_for_service_location_rejected( +async fn test_set_postures_for_service_location_allowed( _: PgPoolOptions, options: PgConnectOptions, ) { @@ -1319,7 +1319,7 @@ async fn test_set_postures_for_service_location_rejected( .await; client.drain_all_events(); - // assigning posture checks to a service location is rejected + // assigning posture checks to a service location is allowed let response = client .put(format!("/api/v1/network/{service_location_id}/postures")) .json(&AssignPosturesData { @@ -1327,16 +1327,24 @@ async fn test_set_postures_for_service_location_rejected( }) .send() .await; - assert_eq!(response.status(), StatusCode::BAD_REQUEST); - client.assert_event_queue_is_empty(); + assert_eq!(response.status(), StatusCode::OK); + let result: Vec = response.json().await; + assert_eq!(result, vec![posture.id]); + + let events = client.drain_all_events(); + assert_eq!(events.len(), 1); + assert!(matches!( + events[0].0, + ApiEventType::LocationPosturesAssigned { .. } + )); - // nothing was assigned to the posture + // the assignment is visible on the posture let response = client .get(format!("/api/v1/device-posture/{}", posture.id)) .send() .await; let fetched: ApiDevicePosture = response.json().await; - assert!(fetched.locations.is_empty()); + assert_eq!(fetched.locations, vec![service_location_id]); // clearing (empty list) is still allowed on a service location let response = client @@ -1347,10 +1355,18 @@ async fn test_set_postures_for_service_location_rejected( .send() .await; assert_eq!(response.status(), StatusCode::OK); + client.drain_all_events(); + + let response = client + .get(format!("/api/v1/device-posture/{}", posture.id)) + .send() + .await; + let fetched: ApiDevicePosture = response.json().await; + assert!(fetched.locations.is_empty()); } #[sqlx::test] -async fn test_set_locations_for_posture_rejects_service_location( +async fn test_set_locations_for_posture_allows_service_location( _: PgPoolOptions, options: PgConnectOptions, ) { @@ -1369,7 +1385,7 @@ async fn test_set_locations_for_posture_rejects_service_location( .await; client.drain_all_events(); - // assigning a service location to a posture is rejected + // assigning a service location to a posture is allowed let response = client .put(format!("/api/v1/device-posture/{}/locations", posture.id)) .json(&AssignLocationsData { @@ -1377,10 +1393,18 @@ async fn test_set_locations_for_posture_rejects_service_location( }) .send() .await; - assert_eq!(response.status(), StatusCode::BAD_REQUEST); - client.assert_event_queue_is_empty(); + assert_eq!(response.status(), StatusCode::OK); + let result: Vec = response.json().await; + assert_eq!(result, vec![service_location_id]); - // a mix containing a service location is rejected too — nothing is assigned + let events = client.drain_all_events(); + assert_eq!(events.len(), 1); + assert!(matches!( + events[0].0, + ApiEventType::DevicePostureLocationsAssigned { .. } + )); + + // a mix containing a service location is accepted too let response = client .put(format!("/api/v1/device-posture/{}/locations", posture.id)) .json(&AssignLocationsData { @@ -1388,15 +1412,21 @@ async fn test_set_locations_for_posture_rejects_service_location( }) .send() .await; - assert_eq!(response.status(), StatusCode::BAD_REQUEST); - client.assert_event_queue_is_empty(); + assert_eq!(response.status(), StatusCode::OK); + let result: Vec = response.json().await; + assert_eq!(result.len(), 2); + assert!(result.contains(®ular_location_id)); + assert!(result.contains(&service_location_id)); + client.drain_all_events(); let response = client .get(format!("/api/v1/device-posture/{}", posture.id)) .send() .await; let fetched: ApiDevicePosture = response.json().await; - assert!(fetched.locations.is_empty()); + assert_eq!(fetched.locations.len(), 2); + assert!(fetched.locations.contains(®ular_location_id)); + assert!(fetched.locations.contains(&service_location_id)); // assigning only regular locations still works let response = client diff --git a/crates/defguard_core/tests/integration/api/wireguard.rs b/crates/defguard_core/tests/integration/api/wireguard.rs index e242d6487..db40d5edc 100644 --- a/crates/defguard_core/tests/integration/api/wireguard.rs +++ b/crates/defguard_core/tests/integration/api/wireguard.rs @@ -34,8 +34,8 @@ use serde_json::json; use sqlx::postgres::{PgConnectOptions, PgPoolOptions}; use super::common::{ - authenticate_admin, exceed_enterprise_limits, fetch_user_details, make_network, - make_test_client, setup_pool, + authenticate_admin, client::TestClient, exceed_enterprise_limits, fetch_user_details, + make_network, make_test_client, setup_pool, }; const INVALID_MFA_PEER_DISCONNECT_THRESHOLD: i32 = 119; @@ -348,6 +348,390 @@ async fn test_create_network_with_posture_checks_requires_enterprise_license( })); } +/// Build a location payload with overridable name, address and mode fields. +/// `posture_checks` is intentionally absent — add it explicitly where it matters. +fn location_payload( + name: &str, + address: &str, + location_mfa_mode: &str, + service_location_mode: &str, +) -> serde_json::Value { + json!({ + "name": name, + "address": address, + "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": ["admin"], + "allow_all_groups": false, + "keepalive_interval": 25, + "peer_disconnect_threshold": 300, + "acl_enabled": false, + "acl_default_allow": false, + "allowed_ips_from_acl": false, + "location_mfa_mode": location_mfa_mode, + "service_location_mode": service_location_mode + }) +} + +/// Create a posture check and return its ID. +async fn make_posture_check(client: &TestClient, name: &str) -> i64 { + let response = client + .post("/api/v1/device-posture") + .json(&json!({ + "name": name, + "description": null, + "min_desktop_client_version": null, + "min_mobile_client_version": null, + "allow_prerelease_client": false, + "os_rules": [] + })) + .send() + .await; + assert_eq!(response.status(), StatusCode::CREATED); + let posture: serde_json::Value = response.json().await; + posture["id"].as_i64().unwrap() +} + +/// Fetch the posture checks assigned to a location. +async fn fetch_location_postures(client: &TestClient, location_id: i64) -> Vec { + let response = client + .get(format!("/api/v1/network/{location_id}")) + .send() + .await; + assert_eq!(response.status(), StatusCode::OK); + let network: serde_json::Value = response.json().await; + serde_json::from_value(network["posture_checks"].clone()).unwrap() +} + +#[sqlx::test] +async fn test_create_network_rejects_service_location_with_mfa( + _: PgPoolOptions, + options: PgConnectOptions, +) { + let pool = setup_pool(options).await; + let (mut client, _client_state) = make_test_client(pool).await; + authenticate_admin(&mut client).await; + set_enterprise_license(); + + for service_location_mode in ["prelogon", "alwayson"] { + let response = client + .post("/api/v1/network") + .json(&location_payload( + "mfa-service-location", + "10.1.1.1/24", + "internal", + service_location_mode, + )) + .send() + .await; + assert_eq!( + response.status(), + StatusCode::BAD_REQUEST, + "MFA + service location mode {service_location_mode} must be rejected" + ); + } + + // MFA without service location mode is fine + let response = client + .post("/api/v1/network") + .json(&location_payload( + "mfa-only", + "10.1.1.1/24", + "internal", + "disabled", + )) + .send() + .await; + assert_eq!(response.status(), StatusCode::CREATED); + + // service location mode without MFA is fine + let response = client + .post("/api/v1/network") + .json(&location_payload( + "service-location-only", + "10.2.2.1/24", + "disabled", + "prelogon", + )) + .send() + .await; + assert_eq!(response.status(), StatusCode::CREATED); +} + +#[sqlx::test] +async fn test_modify_network_rejects_service_location_with_mfa( + _: PgPoolOptions, + options: PgConnectOptions, +) { + let pool = setup_pool(options).await; + let (mut client, _client_state) = make_test_client(pool).await; + authenticate_admin(&mut client).await; + set_enterprise_license(); + + let response = client + .post("/api/v1/network") + .json(&location_payload( + "location", + "10.1.1.1/24", + "disabled", + "disabled", + )) + .send() + .await; + assert_eq!(response.status(), StatusCode::CREATED); + let location: WireguardNetwork = response.json().await; + + for service_location_mode in ["prelogon", "alwayson"] { + let response = client + .put(format!("/api/v1/network/{}", location.id)) + .json(&location_payload( + "location", + "10.1.1.1/24", + "internal", + service_location_mode, + )) + .send() + .await; + assert_eq!( + response.status(), + StatusCode::BAD_REQUEST, + "MFA + service location mode {service_location_mode} must be rejected" + ); + } + + // the rejected combination was not persisted in any form + let response = client + .get(format!("/api/v1/network/{}", location.id)) + .send() + .await; + let fetched: WireguardNetwork = response.json().await; + assert_eq!(fetched.location_mfa_mode, LocationMfaMode::Disabled); + assert_eq!(fetched.service_location_mode, ServiceLocationMode::Disabled); + + // enabling service location mode alone is accepted and persisted + let response = client + .put(format!("/api/v1/network/{}", location.id)) + .json(&location_payload( + "location", + "10.1.1.1/24", + "disabled", + "prelogon", + )) + .send() + .await; + assert_eq!(response.status(), StatusCode::OK); + let modified: WireguardNetwork = response.json().await; + assert_eq!( + modified.service_location_mode, + ServiceLocationMode::PreLogon + ); +} + +#[sqlx::test] +async fn test_modify_network_with_posture_checks_assigns_postures( + _: PgPoolOptions, + options: PgConnectOptions, +) { + let pool = setup_pool(options).await; + let (mut client, _client_state) = make_test_client(pool).await; + authenticate_admin(&mut client).await; + set_enterprise_license(); + + let posture_1 = make_posture_check(&client, "Posture 1").await; + let posture_2 = make_posture_check(&client, "Posture 2").await; + + let response = client + .post("/api/v1/network") + .json(&location_payload( + "location", + "10.1.1.1/24", + "disabled", + "disabled", + )) + .send() + .await; + assert_eq!(response.status(), StatusCode::CREATED); + let location: WireguardNetwork = response.json().await; + assert!( + fetch_location_postures(&client, location.id) + .await + .is_empty() + ); + + // assign both postures through the location payload + let mut payload = location_payload("location", "10.1.1.1/24", "disabled", "disabled"); + payload["posture_checks"] = json!([posture_1, posture_2]); + let response = client + .put(format!("/api/v1/network/{}", location.id)) + .json(&payload) + .send() + .await; + assert_eq!(response.status(), StatusCode::OK); + + let assigned = fetch_location_postures(&client, location.id).await; + assert_eq!(assigned.len(), 2); + assert!(assigned.contains(&posture_1)); + assert!(assigned.contains(&posture_2)); + + // an explicit list replaces the previous assignment + payload["posture_checks"] = json!([posture_2]); + let response = client + .put(format!("/api/v1/network/{}", location.id)) + .json(&payload) + .send() + .await; + assert_eq!(response.status(), StatusCode::OK); + assert_eq!( + fetch_location_postures(&client, location.id).await, + vec![posture_2] + ); + + // an explicit empty list clears the assignment + payload["posture_checks"] = json!([]); + let response = client + .put(format!("/api/v1/network/{}", location.id)) + .json(&payload) + .send() + .await; + assert_eq!(response.status(), StatusCode::OK); + assert!( + fetch_location_postures(&client, location.id) + .await + .is_empty() + ); +} + +#[sqlx::test] +async fn test_modify_network_without_posture_checks_keeps_assignments( + _: PgPoolOptions, + options: PgConnectOptions, +) { + let pool = setup_pool(options).await; + let (mut client, _client_state) = make_test_client(pool).await; + authenticate_admin(&mut client).await; + set_enterprise_license(); + + let posture = make_posture_check(&client, "Posture").await; + + let mut payload = location_payload("location", "10.1.1.1/24", "disabled", "disabled"); + payload["posture_checks"] = json!([posture]); + let response = client.post("/api/v1/network").json(&payload).send().await; + assert_eq!(response.status(), StatusCode::CREATED); + let location: WireguardNetwork = response.json().await; + assert_eq!( + fetch_location_postures(&client, location.id).await, + vec![posture] + ); + + // a payload with the field omitted must leave the assignment alone + let response = client + .put(format!("/api/v1/network/{}", location.id)) + .json(&location_payload( + "renamed-location", + "10.1.1.1/24", + "disabled", + "disabled", + )) + .send() + .await; + assert_eq!(response.status(), StatusCode::OK); + let modified: WireguardNetwork = response.json().await; + assert_eq!(modified.name, "renamed-location"); + assert_eq!( + fetch_location_postures(&client, location.id).await, + vec![posture] + ); + + // an explicit `null` behaves the same way + let mut payload = location_payload("location", "10.1.1.1/24", "disabled", "disabled"); + payload["posture_checks"] = json!(null); + let response = client + .put(format!("/api/v1/network/{}", location.id)) + .json(&payload) + .send() + .await; + assert_eq!(response.status(), StatusCode::OK); + assert_eq!( + fetch_location_postures(&client, location.id).await, + vec![posture] + ); +} + +#[sqlx::test] +async fn test_posture_checks_allowed_on_service_locations( + _: PgPoolOptions, + options: PgConnectOptions, +) { + let pool = setup_pool(options).await; + let (mut client, _client_state) = make_test_client(pool).await; + authenticate_admin(&mut client).await; + set_enterprise_license(); + + let posture = make_posture_check(&client, "Posture").await; + + // create path: a service location may carry posture checks + let mut payload = location_payload("service-location", "10.1.1.1/24", "disabled", "prelogon"); + payload["posture_checks"] = json!([posture]); + let response = client.post("/api/v1/network").json(&payload).send().await; + assert_eq!(response.status(), StatusCode::CREATED); + let service_location: WireguardNetwork = response.json().await; + assert_eq!( + service_location.service_location_mode, + ServiceLocationMode::PreLogon + ); + assert_eq!( + fetch_location_postures(&client, service_location.id).await, + vec![posture] + ); + + // modify path: turning a posture-carrying regular location into a service + // location keeps its posture checks + let mut payload = location_payload("regular-location", "10.2.2.1/24", "disabled", "disabled"); + payload["posture_checks"] = json!([posture]); + let response = client.post("/api/v1/network").json(&payload).send().await; + assert_eq!(response.status(), StatusCode::CREATED); + let location: WireguardNetwork = response.json().await; + + let response = client + .put(format!("/api/v1/network/{}", location.id)) + .json(&location_payload( + "regular-location", + "10.2.2.1/24", + "disabled", + "alwayson", + )) + .send() + .await; + assert_eq!(response.status(), StatusCode::OK); + let modified: WireguardNetwork = response.json().await; + assert_eq!( + modified.service_location_mode, + ServiceLocationMode::AlwaysOn + ); + assert_eq!( + fetch_location_postures(&client, location.id).await, + vec![posture] + ); + + // modify path: posture checks can be assigned to an existing service location + let mut payload = location_payload("service-location", "10.1.1.1/24", "disabled", "prelogon"); + payload["posture_checks"] = json!([posture]); + let response = client + .put(format!("/api/v1/network/{}", service_location.id)) + .json(&payload) + .send() + .await; + assert_eq!(response.status(), StatusCode::OK); + assert_eq!( + fetch_location_postures(&client, service_location.id).await, + vec![posture] + ); +} + #[sqlx::test] async fn test_location_mfa_mode_validation_create(_: PgPoolOptions, options: PgConnectOptions) { let pool = setup_pool(options).await; From ec3f03258213ed767884ee3c7febfea1bf424414 Mon Sep 17 00:00:00 2001 From: Jacek Chmielewski Date: Tue, 4 Aug 2026 09:25:49 +0200 Subject: [PATCH 04/13] posture check for location with no PCs assigned passes with empty PSK; add missing log events --- .../src/grpc/proxy/client_mfa.rs | 437 +++++++++++++++--- crates/defguard_proxy_manager/src/handler.rs | 7 +- 2 files changed, 381 insertions(+), 63 deletions(-) diff --git a/crates/defguard_core/src/grpc/proxy/client_mfa.rs b/crates/defguard_core/src/grpc/proxy/client_mfa.rs index 627ff9edf..ccddc4a11 100644 --- a/crates/defguard_core/src/grpc/proxy/client_mfa.rs +++ b/crates/defguard_core/src/grpc/proxy/client_mfa.rs @@ -872,9 +872,14 @@ impl ClientMfaServer { /// with a generated preshared key. Returns a typed outcome so the caller can /// map it to the appropriate `CoreResponse` payload without needing to know about /// session internals. + /// + /// A location with no postures assigned is approved with an *empty* preshared key and no + /// session, since its peers are handed to the gateway without one - see the `has_postures` + /// check below. pub async fn handle_posture_check( &mut self, request: DevicePostureCheckRequest, + info: Option, ) -> Result { debug!( "Handling posture check for device pubkey={} location_id={}", @@ -938,17 +943,6 @@ impl ClientMfaServer { return Err(Status::unauthenticated("token does not match device")); } - if !location.has_postures(&self.pool).await.map_err(|err| { - error!("Posture check: failed to fetch postures for location {location}: {err}"); - Status::internal("unexpected error") - })? { - error!( - "Posture check: location {location} has no postures defined but device {} requested posture check", - device.wireguard_pubkey - ); - return Err(Status::invalid_argument("location does not use postures")); - } - let Ok(Some(user)) = User::find_by_id(&self.pool, device.user_id).await else { error!("Posture check: user {} not found", device.user_id); return Err(Status::internal("user not found")); @@ -974,6 +968,24 @@ impl ClientMfaServer { })?; Self::validate_location_access(&self.pool, &location, &user_info).await?; + // If location has no postures assigned, approve the posture check returning empty string as PSK. + // This way the client can recover on it's own if the admin unassigns PCs from a location and the client + // didn't get the config yet. Matters especially for service locations where the client UI may not be + // running and therefore config is not being polled. + if !location.has_postures(&self.pool).await.map_err(|err| { + error!("Posture check: failed to fetch postures for location {location}: {err}"); + Status::internal("unexpected error") + })? { + info!( + "Posture check: location {location} has no postures assigned, approving device {} \ + with an empty preshared key without creating a session", + device.wireguard_pubkey + ); + return Ok(PostureCheckOutcome::Approved { + preshared_key: String::new(), + }); + } + // Evaluate posture. let posture_result = match validate_posture(&self.pool, &request).await { Ok(result) => result, @@ -987,12 +999,42 @@ impl ClientMfaServer { } }; + let (ip, _user_agent) = parse_client_ip_agent(&info).map_err(Status::internal)?; + let context = + BidiRequestContext::new(user.id, user.username.clone(), ip, device.name.clone()); + // Posture check failed - return payload with reasons if let PostureResult::Fail(reasons) = posture_result { - let failed_checks = reasons.iter().map(ToString::to_string).collect(); + let failed_checks = reasons.iter().map(ToString::to_string).collect::>(); + if let Err(err) = self.emit_event(BidiStreamEvent { + context, + event: BidiStreamEventType::DesktopClientMfa(Box::new( + DesktopClientMfaEvent::PostureCheckFailed { + device: device.clone(), + location: location.clone(), + device_posture_data: request.device_posture_data.clone(), + failed_checks: failed_checks.clone(), + }, + )), + }) { + error!("Failed to emit DevicePostureCheckFailed event: {err}"); + } return Ok(PostureCheckOutcome::Rejected { failed_checks }); } + if let Err(err) = self.emit_event(BidiStreamEvent { + context, + event: BidiStreamEventType::DesktopClientMfa(Box::new( + DesktopClientMfaEvent::PostureCheckPassed { + device: device.clone(), + location: location.clone(), + device_posture_data: request.device_posture_data.clone(), + }, + )), + }) { + error!("Failed to emit DevicePostureCheckPassed event: {err}"); + } + // Posture check succeeded - create a vpn session let key = WireguardNetwork::genkey(); @@ -1210,8 +1252,11 @@ mod tests { }, setup_pool, }; - use defguard_proto::enterprise::posture::{ - BoolCheck, DevicePostureCheckRequest, DevicePostureData, bool_check, + use defguard_proto::{ + enterprise::posture::{ + BoolCheck, DevicePostureCheckRequest, DevicePostureData, bool_check, + }, + proxy::DeviceInfo, }; use ipnetwork::IpNetwork; use sqlx::{ @@ -1236,6 +1281,16 @@ mod tests { const REPLACEMENT_MFA_PRESHARED_KEY: &str = "replacement-mfa-psk"; const NEW_MFA_PRESHARED_KEY: &str = "new-psk"; + const DEVICE_INFO_IP: &str = "10.0.0.7"; + + /// The `DeviceInfo` the proxy attaches to every bidi request; audit events are built from it. + fn device_info() -> Option { + Some(DeviceInfo { + ip_address: DEVICE_INFO_IP.to_owned(), + user_agent: Some("defguard-client/1.6.0".to_owned()), + ..Default::default() + }) + } #[sqlx::test] async fn test_posture_check_success_emits_vpn_session_authorized_event( @@ -1256,12 +1311,15 @@ mod tests { let (mut server, _event_rx, mut gateway_rx) = make_server(pool.clone()); let outcome = server - .handle_posture_check(DevicePostureCheckRequest { - location_id: location.id, - pubkey: device.wireguard_pubkey.clone(), - device_posture_data: Some(passing_linux_posture_data()), - token: Some(token.clone()), - }) + .handle_posture_check( + DevicePostureCheckRequest { + location_id: location.id, + pubkey: device.wireguard_pubkey.clone(), + device_posture_data: Some(passing_linux_posture_data()), + token: Some(token.clone()), + }, + device_info(), + ) .await .expect("posture check should pass"); let preshared_key = match outcome { @@ -1334,12 +1392,15 @@ mod tests { let (mut server, mut event_rx, mut gateway_rx) = make_server(pool.clone()); server - .handle_posture_check(DevicePostureCheckRequest { - location_id: location.id, - pubkey: device.wireguard_pubkey.clone(), - device_posture_data: Some(passing_linux_posture_data()), - token: Some(token.clone()), - }) + .handle_posture_check( + DevicePostureCheckRequest { + location_id: location.id, + pubkey: device.wireguard_pubkey.clone(), + device_posture_data: Some(passing_linux_posture_data()), + token: Some(token.clone()), + }, + device_info(), + ) .await .expect("replacement posture check should pass"); @@ -1365,6 +1426,18 @@ mod tests { other => panic!("unexpected gateway event: {other:?}"), } + // the passing posture evaluation is audited first + let event = event_rx + .try_recv() + .expect("expected posture check passed audit event"); + match event.event { + BidiStreamEventType::DesktopClientMfa(event) => match *event { + DesktopClientMfaEvent::PostureCheckPassed { .. } => {} + other => panic!("unexpected bidi event: {other:?}"), + }, + other => panic!("unexpected bidi stream event type: {other:?}"), + } + // replacing a connected posture-only session emits the unified session // superseded audit event, flagged as a non-MFA session let event = event_rx @@ -1411,12 +1484,15 @@ mod tests { for token in [None, Some(String::new())] { let err = server - .handle_posture_check(DevicePostureCheckRequest { - location_id: location.id, - pubkey: device.wireguard_pubkey.clone(), - device_posture_data: Some(passing_linux_posture_data()), - token, - }) + .handle_posture_check( + DevicePostureCheckRequest { + location_id: location.id, + pubkey: device.wireguard_pubkey.clone(), + device_posture_data: Some(passing_linux_posture_data()), + token, + }, + device_info(), + ) .await; let err = match err { Ok(_) => panic!("posture check without a token must be refused"), @@ -1455,12 +1531,15 @@ mod tests { let (mut server, _, _) = make_server(pool); let err = server - .handle_posture_check(DevicePostureCheckRequest { - location_id: location.id, - pubkey: device.wireguard_pubkey.clone(), - device_posture_data: Some(passing_linux_posture_data()), - token: Some("not-a-real-token".to_owned()), - }) + .handle_posture_check( + DevicePostureCheckRequest { + location_id: location.id, + pubkey: device.wireguard_pubkey.clone(), + device_posture_data: Some(passing_linux_posture_data()), + token: Some("not-a-real-token".to_owned()), + }, + device_info(), + ) .await; let err = match err { Ok(_) => panic!("posture check with an unknown token must be refused"), @@ -1522,12 +1601,15 @@ mod tests { // Attacker presents its own valid token but claims the victim's public key. let err = server - .handle_posture_check(DevicePostureCheckRequest { - location_id: location.id, - pubkey: victim.wireguard_pubkey.clone(), - device_posture_data: Some(passing_linux_posture_data()), - token: Some(attacker_token), - }) + .handle_posture_check( + DevicePostureCheckRequest { + location_id: location.id, + pubkey: victim.wireguard_pubkey.clone(), + device_posture_data: Some(passing_linux_posture_data()), + token: Some(attacker_token), + }, + device_info(), + ) .await; let err = match err { Ok(_) => panic!("a token from another device must not authorize this one"), @@ -1566,12 +1648,15 @@ mod tests { let (mut server, _, _) = make_server(pool); let err = match server - .handle_posture_check(DevicePostureCheckRequest { - location_id: location.id, - pubkey: "irrelevant".to_owned(), - device_posture_data: None, - token: Some(token), - }) + .handle_posture_check( + DevicePostureCheckRequest { + location_id: location.id, + pubkey: "irrelevant".to_owned(), + device_posture_data: None, + token: Some(token), + }, + device_info(), + ) .await { Ok(_) => panic!("MFA-enabled location should reject posture-only flow"), @@ -1581,33 +1666,261 @@ mod tests { assert_eq!(err.code(), Code::InvalidArgument); } + /// A location with no postures assigned hands its peers to the gateway without a preshared + /// key, so the only answer that lets a client connect is an empty one. Approving instead of + /// erroring is what allows a service location whose cached config still demands a posture check + /// to recover after an admin unassigns the last posture. #[sqlx::test] - async fn test_posture_check_rejects_location_without_postures( + async fn test_posture_check_without_postures_approves_with_empty_preshared_key( _: PgPoolOptions, options: PgConnectOptions, ) { + set_enterprise_license(); let pool = setup_pool(options).await; + initialize_current_settings(&pool) + .await + .expect("failed to init settings"); let location = create_non_mfa_location(&pool).await; let user = create_user(&pool).await; let device = create_device(&pool, user.id).await; attach_device_to_location(&pool, location.id, device.id).await; let token = create_polling_token(&pool, device.id).await; - let (mut server, _, _) = make_server(pool); + let (mut server, _event_rx, mut gateway_rx) = make_server(pool.clone()); - let err = match server - .handle_posture_check(DevicePostureCheckRequest { - location_id: location.id, - pubkey: device.wireguard_pubkey, - device_posture_data: None, - token: Some(token), - }) + let outcome = server + .handle_posture_check( + DevicePostureCheckRequest { + location_id: location.id, + pubkey: device.wireguard_pubkey.clone(), + device_posture_data: None, + token: Some(token), + }, + device_info(), + ) + .await + .expect("location without postures should be approved"); + + match outcome { + super::PostureCheckOutcome::Approved { preshared_key } => assert!( + preshared_key.is_empty(), + "a location without postures must not hand out a preshared key" + ), + super::PostureCheckOutcome::Rejected { failed_checks } => { + panic!("posture check unexpectedly failed: {failed_checks:?}") + } + } + + // No session may be created and the gateway must not be touched. + assert!( + VpnClientSession::get_all_active_device_sessions_in_location( + &pool, + location.id, + device.id + ) + .await + .expect("failed to query sessions") + .is_empty(), + "no VPN session may be created when a location has no postures" + ); + assert!( + gateway_rx.try_recv().is_err(), + "no gateway command may be sent when a location has no postures" + ); + } + + /// The empty-preshared-key approval must not outrank the access checks: deactivating a user has + /// to stop their devices from getting anything that reads as approval, even on a location with + /// no postures where the approval grants nothing by itself. + #[sqlx::test] + async fn test_posture_check_without_postures_still_rejects_inactive_user( + _: PgPoolOptions, + options: PgConnectOptions, + ) { + set_enterprise_license(); + let pool = setup_pool(options).await; + initialize_current_settings(&pool) + .await + .expect("failed to init settings"); + let location = create_non_mfa_location(&pool).await; + let mut user = create_user(&pool).await; + user.is_active = false; + user.save(&pool).await.expect("failed to deactivate user"); + let device = create_device(&pool, user.id).await; + attach_device_to_location(&pool, location.id, device.id).await; + let token = create_polling_token(&pool, device.id).await; + let (mut server, _event_rx, _gateway_rx) = make_server(pool.clone()); + + let status = match server + .handle_posture_check( + DevicePostureCheckRequest { + location_id: location.id, + pubkey: device.wireguard_pubkey.clone(), + device_posture_data: None, + token: Some(token), + }, + device_info(), + ) .await { - Ok(_) => panic!("location without postures should reject posture-only flow"), - Err(err) => err, + Ok(super::PostureCheckOutcome::Approved { .. }) => { + panic!("an inactive user must not be approved, even without postures") + } + Ok(super::PostureCheckOutcome::Rejected { .. }) => { + panic!("expected an inactive-user error, not a posture rejection") + } + Err(status) => status, }; + assert_eq!(status.code(), tonic::Code::InvalidArgument); + assert_eq!(status.message(), "user is inactive"); + } - assert_eq!(err.code(), Code::InvalidArgument); + /// A passing posture evaluation must be auditable, so an operator can see that a headless + /// service location connected and why. + #[sqlx::test] + async fn test_posture_check_pass_emits_posture_check_passed_event( + _: PgPoolOptions, + options: PgConnectOptions, + ) { + set_enterprise_license(); + let pool = setup_pool(options).await; + initialize_current_settings(&pool) + .await + .expect("failed to init settings"); + let location = create_non_mfa_location(&pool).await; + save_linux_posture_policy(&pool, location.id).await; + let user = create_user(&pool).await; + let device = create_device(&pool, user.id).await; + attach_device_to_location(&pool, location.id, device.id).await; + let token = create_polling_token(&pool, device.id).await; + let (mut server, mut event_rx, _gateway_rx) = make_server(pool.clone()); + + let posture_data = passing_linux_posture_data(); + match server + .handle_posture_check( + DevicePostureCheckRequest { + location_id: location.id, + pubkey: device.wireguard_pubkey.clone(), + device_posture_data: Some(posture_data.clone()), + token: Some(token), + }, + device_info(), + ) + .await + .expect("posture check should pass") + { + super::PostureCheckOutcome::Approved { preshared_key } => { + assert!(!preshared_key.is_empty()); + } + super::PostureCheckOutcome::Rejected { failed_checks } => { + panic!("posture check unexpectedly failed: {failed_checks:?}") + } + } + + let event = event_rx + .try_recv() + .expect("expected posture check passed audit event"); + match event.event { + BidiStreamEventType::DesktopClientMfa(event) => match *event { + DesktopClientMfaEvent::PostureCheckPassed { + device: event_device, + location: event_location, + device_posture_data, + } => { + assert_eq!(event_device.id, device.id); + assert_eq!(event_location.id, location.id); + assert_eq!(device_posture_data, Some(posture_data)); + } + other => panic!("unexpected bidi event: {other:?}"), + }, + other => panic!("unexpected bidi stream event type: {other:?}"), + } + assert_eq!(event.context.user_id, user.id); + assert_eq!(event.context.username, user.username); + assert_eq!(event.context.ip, Some(DEVICE_INFO_IP.parse().unwrap())); + } + + /// A failing posture evaluation must be auditable too - on a service location this is the only + /// way to find out why the tunnel never came up. + #[sqlx::test] + async fn test_posture_check_failure_emits_posture_check_failed_event( + _: PgPoolOptions, + options: PgConnectOptions, + ) { + set_enterprise_license(); + let pool = setup_pool(options).await; + initialize_current_settings(&pool) + .await + .expect("failed to init settings"); + let location = create_non_mfa_location(&pool).await; + save_linux_posture_policy(&pool, location.id).await; + let user = create_user(&pool).await; + let device = create_device(&pool, user.id).await; + attach_device_to_location(&pool, location.id, device.id).await; + let token = create_polling_token(&pool, device.id).await; + let (mut server, mut event_rx, mut gateway_rx) = make_server(pool.clone()); + + // the policy requires disk encryption + let posture_data = DevicePostureData { + disk_encryption: Some(BoolCheck { + result: Some(bool_check::Result::Value(false)), + }), + ..passing_linux_posture_data() + }; + let rejected_checks = match server + .handle_posture_check( + DevicePostureCheckRequest { + location_id: location.id, + pubkey: device.wireguard_pubkey.clone(), + device_posture_data: Some(posture_data.clone()), + token: Some(token), + }, + device_info(), + ) + .await + .expect("posture check should complete") + { + super::PostureCheckOutcome::Approved { .. } => { + panic!("posture check with unencrypted disk should be rejected") + } + super::PostureCheckOutcome::Rejected { failed_checks } => failed_checks, + }; + assert!(!rejected_checks.is_empty()); + + let event = event_rx + .try_recv() + .expect("expected posture check failed audit event"); + match event.event { + BidiStreamEventType::DesktopClientMfa(event) => match *event { + DesktopClientMfaEvent::PostureCheckFailed { + device: event_device, + location: event_location, + device_posture_data, + failed_checks, + } => { + assert_eq!(event_device.id, device.id); + assert_eq!(event_location.id, location.id); + assert_eq!(device_posture_data, Some(posture_data)); + assert_eq!(failed_checks, rejected_checks); + } + other => panic!("unexpected bidi event: {other:?}"), + }, + other => panic!("unexpected bidi stream event type: {other:?}"), + } + assert_eq!(event.context.user_id, user.id); + assert_eq!(event.context.username, user.username); + + // a rejected check must not authorize anything + assert!(gateway_rx.try_recv().is_err()); + assert!( + VpnClientSession::get_all_active_device_sessions_in_location( + &pool, + location.id, + device.id + ) + .await + .expect("failed to query sessions") + .is_empty() + ); } #[sqlx::test] diff --git a/crates/defguard_proxy_manager/src/handler.rs b/crates/defguard_proxy_manager/src/handler.rs index be795dd39..656626c4c 100644 --- a/crates/defguard_proxy_manager/src/handler.rs +++ b/crates/defguard_proxy_manager/src/handler.rs @@ -1048,7 +1048,12 @@ impl ProxyHandler { None } Some(core_request::Payload::DevicePostureCheck(request)) => { - match self.services.client_mfa.handle_posture_check(request).await { + match self + .services + .client_mfa + .handle_posture_check(request, received.device_info) + .await + { Ok(PostureCheckOutcome::Approved { preshared_key }) => { Some(core_response::Payload::DevicePostureCheck( DevicePostureCheckResponse { preshared_key }, From 18c1fb11d35d57d23e1a8c31470f60c4de3da02c Mon Sep 17 00:00:00 2001 From: Jacek Chmielewski Date: Wed, 5 Aug 2026 08:31:31 +0200 Subject: [PATCH 05/13] keepalive >= 1; onlock postures for service locations on frontend --- .../defguard_core/src/handlers/wireguard.rs | 14 +++++ .../tests/integration/api/wireguard.rs | 57 +++++++++++++++++++ web/messages/en/form.json | 1 + web/messages/en/location.json | 3 +- .../pages/AddLocationPage/AddLocationPage.tsx | 1 - .../steps/AddLocationAccessStep.tsx | 5 +- .../steps/AddLocationFirewallStep.tsx | 6 +- .../steps/AddLocationNetworkStep.tsx | 2 + .../EditLocationPage/EditLocationPage.tsx | 43 ++++---------- .../pages/PostureChecksPage/postureChecks.ts | 21 +++---- 10 files changed, 96 insertions(+), 57 deletions(-) diff --git a/crates/defguard_core/src/handlers/wireguard.rs b/crates/defguard_core/src/handlers/wireguard.rs index 5e01dc8d2..5df52984c 100644 --- a/crates/defguard_core/src/handlers/wireguard.rs +++ b/crates/defguard_core/src/handlers/wireguard.rs @@ -153,6 +153,18 @@ impl WireguardNetworkData { )) } + /// Rejects a zero (or negative) keepalive interval to prevent idle service locations + /// from disconnecting. + pub(crate) fn validate_keepalive_interval(&self) -> Result<(), WebError> { + if self.keepalive_interval >= 1 { + return Ok(()); + } + + Err(WebError::BadRequest( + "keepalive_interval must be at least 1".into(), + )) + } + pub(crate) fn validate_allowed_groups(&self) -> Result<(), WebError> { if self.allow_all_groups || !self.allowed_groups.is_empty() { return Ok(()); @@ -248,6 +260,7 @@ pub(crate) async fn create_network( data.validate_peer_disconnect_threshold()?; data.validate_location_mfa_mode(&appstate.pool).await?; data.validate_service_location_mfa()?; + data.validate_keepalive_interval()?; data.validate_allowed_groups()?; let allowed_ips = data.parse_allowed_ips(); @@ -378,6 +391,7 @@ pub(crate) async fn modify_network( data.validate_peer_disconnect_threshold()?; data.validate_location_mfa_mode(&appstate.pool).await?; data.validate_service_location_mfa()?; + data.validate_keepalive_interval()?; data.validate_allowed_groups()?; let network = find_network(network_id, &appstate.pool).await?; diff --git a/crates/defguard_core/tests/integration/api/wireguard.rs b/crates/defguard_core/tests/integration/api/wireguard.rs index db40d5edc..6c743704b 100644 --- a/crates/defguard_core/tests/integration/api/wireguard.rs +++ b/crates/defguard_core/tests/integration/api/wireguard.rs @@ -462,6 +462,63 @@ async fn test_create_network_rejects_service_location_with_mfa( assert_eq!(response.status(), StatusCode::CREATED); } +/// A zero keepalive stops `last_handshake` from ever advancing on an idle tunnel, which would make +/// the posture health check re-authorize forever (D6/R7). The web forms block it, but an API caller +/// bypasses them entirely, so core has to reject it too. +#[sqlx::test] +async fn test_network_rejects_zero_keepalive_interval(_: PgPoolOptions, options: PgConnectOptions) { + let pool = setup_pool(options).await; + let (mut client, _client_state) = make_test_client(pool).await; + authenticate_admin(&mut client).await; + set_enterprise_license(); + + let mut payload = location_payload("zero-keepalive", "10.1.1.1/24", "disabled", "disabled"); + payload["keepalive_interval"] = json!(0); + let response = client.post("/api/v1/network").json(&payload).send().await; + assert_eq!( + response.status(), + StatusCode::BAD_REQUEST, + "keepalive_interval 0 must be rejected on create" + ); + + // A valid location, so the same rule can be checked on the modify path. + let response = client + .post("/api/v1/network") + .json(&location_payload( + "good-keepalive", + "10.2.2.1/24", + "disabled", + "disabled", + )) + .send() + .await; + assert_eq!(response.status(), StatusCode::CREATED); + let created: serde_json::Value = response.json().await; + let location_id = created["id"].as_i64().unwrap(); + + let mut payload = location_payload("good-keepalive", "10.2.2.1/24", "disabled", "disabled"); + payload["keepalive_interval"] = json!(0); + let response = client + .put(format!("/api/v1/network/{location_id}")) + .json(&payload) + .send() + .await; + assert_eq!( + response.status(), + StatusCode::BAD_REQUEST, + "keepalive_interval 0 must be rejected on modify" + ); + + // 1 is the floor, not a rejected edge. + payload["keepalive_interval"] = json!(1); + let response = client + .put(format!("/api/v1/network/{location_id}")) + .json(&payload) + .send() + .await; + assert_eq!(response.status(), StatusCode::OK); +} + #[sqlx::test] async fn test_modify_network_rejects_service_location_with_mfa( _: PgPoolOptions, diff --git a/web/messages/en/form.json b/web/messages/en/form.json index d5ffc350f..eee1b19f8 100644 --- a/web/messages/en/form.json +++ b/web/messages/en/form.json @@ -6,6 +6,7 @@ "form_error_file_contents": "File content is not valid", "form_error_ip_or_domain": "Only valid IP or domain is allowed", "form_error_port_max": "Port exceeds maximum value", + "form_error_keepalive_min": "Keep alive interval must be at least 1 second", "form_error_len": "Required length {length}", "form_error_name_reserved": "Name already taken", "form_error_email": "Enter valid email", diff --git a/web/messages/en/location.json b/web/messages/en/location.json index 5f988a052..4539e84f0 100644 --- a/web/messages/en/location.json +++ b/web/messages/en/location.json @@ -98,10 +98,9 @@ "location_mfa_option_internal": "Internal MFA", "location_mfa_option_external": "External MFA", "location_mfa_service_location_warning": "MFA can't be enabled for service locations. To enforce MFA, select the Regular location type.", - "location_posture_service_location_warning": "Postures can't be enabled for service locations. To enforce posture checks, select the Regular location type.", "location_edit_section_location_type": "Location type", + "location_posture_prelogon_windows_only": "Pre-logon service locations are supported on Windows only, so posture checks for this location will be enforced on Windows clients only.", "location_service_mode_mfa_warning": "MFA-protected locations can't be set as service locations. Disable MFA to use a service location type.", - "location_service_mode_postures_warning": "Locations with assigned posture checks can't be set as service locations. Remove posture checks to use a service location type.", "location_access_section_label": "Location Access", "location_access_selected_group_count_one": "+{count} group", "location_access_selected_group_count_other": "+{count} groups", diff --git a/web/src/pages/AddLocationPage/AddLocationPage.tsx b/web/src/pages/AddLocationPage/AddLocationPage.tsx index d253f4b1e..5ef5dd36d 100644 --- a/web/src/pages/AddLocationPage/AddLocationPage.tsx +++ b/web/src/pages/AddLocationPage/AddLocationPage.tsx @@ -91,7 +91,6 @@ export const AddLocationPage = () => { id: AddLocationPageStep.PostureCheck, order: 6, label: m.add_location_step_posture_check_label(), - hidden: locationType === 'service', description: m.add_location_step_posture_check_description(), }, firewall: { diff --git a/web/src/pages/AddLocationPage/steps/AddLocationAccessStep.tsx b/web/src/pages/AddLocationPage/steps/AddLocationAccessStep.tsx index be5807ebb..28c138bc6 100644 --- a/web/src/pages/AddLocationPage/steps/AddLocationAccessStep.tsx +++ b/web/src/pages/AddLocationPage/steps/AddLocationAccessStep.tsx @@ -95,10 +95,7 @@ export const AddLocationAccessStep = () => { } saveChanges(selected, allowAllGroups); useAddLocationStore.setState({ - activeStep: - locationType === 'service' - ? AddLocationPageStep.Firewall - : AddLocationPageStep.PostureCheck, + activeStep: AddLocationPageStep.PostureCheck, }); }} /> diff --git a/web/src/pages/AddLocationPage/steps/AddLocationFirewallStep.tsx b/web/src/pages/AddLocationPage/steps/AddLocationFirewallStep.tsx index a164e4bba..41fc65c02 100644 --- a/web/src/pages/AddLocationPage/steps/AddLocationFirewallStep.tsx +++ b/web/src/pages/AddLocationPage/steps/AddLocationFirewallStep.tsx @@ -26,7 +26,6 @@ import { useAddLocationStore } from '../useAddLocationStore'; type Choice = 'disable' | 'enabled-allowed' | 'enabled-denied'; export const AddLocationFirewallStep = () => { - const locationType = useAddLocationStore((s) => s.locationType); const [state, setState] = useState('disable'); const [showGateway, setShowGateway] = useState(true); const navigate = useNavigate(); @@ -150,10 +149,7 @@ export const AddLocationFirewallStep = () => { onClick={() => { saveChanges(state); useAddLocationStore.setState({ - activeStep: - locationType === 'service' - ? AddLocationPageStep.AccessControl - : AddLocationPageStep.PostureCheck, + activeStep: AddLocationPageStep.PostureCheck, }); }} /> diff --git a/web/src/pages/AddLocationPage/steps/AddLocationNetworkStep.tsx b/web/src/pages/AddLocationPage/steps/AddLocationNetworkStep.tsx index a551041a4..a7824d167 100644 --- a/web/src/pages/AddLocationPage/steps/AddLocationNetworkStep.tsx +++ b/web/src/pages/AddLocationPage/steps/AddLocationNetworkStep.tsx @@ -14,6 +14,8 @@ import { useAddLocationStore } from '../useAddLocationStore'; const formSchema = z.object({ keepalive_interval: z .number(m.form_error_required()) + // Keepalive is mandatory to prevent idle service locations from disconnecting + .min(1, m.form_error_keepalive_min()) .max(65535, m.form_error_port_max()), mtu: z.number(m.form_error_required()).min(72).max(0xffffffff), fwmark: z.number(m.form_error_required()).min(0).max(0xffffffff), diff --git a/web/src/pages/EditLocationPage/EditLocationPage.tsx b/web/src/pages/EditLocationPage/EditLocationPage.tsx index e8b85640f..5ba52fce0 100644 --- a/web/src/pages/EditLocationPage/EditLocationPage.tsx +++ b/web/src/pages/EditLocationPage/EditLocationPage.tsx @@ -154,6 +154,8 @@ const formSchema = z peer_disconnect_threshold: z.number().nullable(), keepalive_interval: z .number(m.form_error_required()) + // Keepalive is mandatory to prevent idle service locations from disconnecting + .min(1, m.form_error_keepalive_min()) .max(65535, m.form_error_port_max()), mtu: z.number(m.form_error_required()).min(72).max(0xffffffff), fwmark: z.number(m.form_error_required()).min(0).max(0xffffffff), @@ -812,13 +814,6 @@ const EditLocationForm = ({ location }: { location: NetworkLocation }) => { text={m.location_service_mode_mfa_warning()} /> )} - {postureChecksSectionState.hasAssignedPostureChecks && ( - - )} { @@ -945,18 +928,14 @@ const EditLocationForm = ({ location }: { location: NetworkLocation }) => { )} - - s.values.service_location_mode !== LocationServiceMode.Disabled - } - > - {(isServiceLocation) => ( + s.values.service_location_mode}> + {(serviceLocationMode) => ( <> - {isServiceLocation && ( + {serviceLocationMode === LocationServiceMode.Prelogon && ( )} { editIcon={IconKind.Edit} toggleValue={false} counterText={() => ''} - disabled={isServiceLocation} onSelectionChange={(values) => { setLocationPostures({ postures: values.filter( @@ -1020,7 +998,6 @@ const EditLocationForm = ({ location }: { location: NetworkLocation }) => { loading={isUpdatingLocationPostures} text={m.posture_checks_wizard_title()} onClick={openPostureChecksSelection} - disabled={isServiceLocation} /> )} {postureChecksSectionState.showLockedButton && ( diff --git a/web/src/pages/PostureChecksPage/postureChecks.ts b/web/src/pages/PostureChecksPage/postureChecks.ts index c1d834082..71cdd39e3 100644 --- a/web/src/pages/PostureChecksPage/postureChecks.ts +++ b/web/src/pages/PostureChecksPage/postureChecks.ts @@ -1,10 +1,9 @@ import { m } from '../../paraglide/messages'; import api from '../../shared/api/api'; -import { - type ApiDevicePosture, - type ApiDevicePostureOsRule, - LocationServiceMode, - type NetworkLocation, +import type { + ApiDevicePosture, + ApiDevicePostureOsRule, + NetworkLocation, } from '../../shared/api/types'; import type { SelectionOption } from '../../shared/components/SelectionSection/type'; import type { TableFilterMessages } from '../../shared/defguard-ui/components/table/types'; @@ -365,11 +364,9 @@ export const filterPostureChecks = (rows: PostureCheckRow[], search: string) => }; export const buildFilteredLocationOptions = (locations: NetworkLocation[]) => { - return locations - .filter((location) => location.service_location_mode === LocationServiceMode.Disabled) - .map((loc) => ({ - id: loc.id, - label: loc.name, - searchFields: [loc.name, ...loc.address], - })); + return locations.map((loc) => ({ + id: loc.id, + label: loc.name, + searchFields: [loc.name, ...loc.address], + })); }; From e2b5796d22b6a8db2f09b47a685b168bdcc213cc Mon Sep 17 00:00:00 2001 From: Jacek Chmielewski Date: Thu, 6 Aug 2026 11:47:45 +0200 Subject: [PATCH 06/13] update protos --- proto | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/proto b/proto index 3fff8be2f..6867db7bc 160000 --- a/proto +++ b/proto @@ -1 +1 @@ -Subproject commit 3fff8be2fb0ed135dd234d35dc347d90d6466207 +Subproject commit 6867db7bc454023df5fb0d7d15d3482aed5e936d From d433ec9bf25954c472330263d3f15f43b9089fce Mon Sep 17 00:00:00 2001 From: Jacek Chmielewski Date: Thu, 6 Aug 2026 11:49:33 +0200 Subject: [PATCH 07/13] improve comments, remove unnecessary warning --- .../src/grpc/proxy/client_mfa.rs | 4 +- .../EditLocationPage/EditLocationPage.tsx | 149 ++++++++---------- 2 files changed, 65 insertions(+), 88 deletions(-) diff --git a/crates/defguard_core/src/grpc/proxy/client_mfa.rs b/crates/defguard_core/src/grpc/proxy/client_mfa.rs index 69ccbf1d9..b113c0547 100644 --- a/crates/defguard_core/src/grpc/proxy/client_mfa.rs +++ b/crates/defguard_core/src/grpc/proxy/client_mfa.rs @@ -873,8 +873,7 @@ impl ClientMfaServer { /// session internals. /// /// A location with no postures assigned is approved with an *empty* preshared key and no - /// session, since its peers are handed to the gateway without one - see the `has_postures` - /// check below. + /// session, since its peers are handed to the gateway without one. pub async fn handle_posture_check( &mut self, request: DevicePostureCheckRequest, @@ -985,7 +984,6 @@ impl ClientMfaServer { }); } - // Evaluate posture. Use values already validated above rather than the untrusted request. let posture_result = match validate_posture( &self.pool, location.id, diff --git a/web/src/pages/EditLocationPage/EditLocationPage.tsx b/web/src/pages/EditLocationPage/EditLocationPage.tsx index 5ba52fce0..df08df160 100644 --- a/web/src/pages/EditLocationPage/EditLocationPage.tsx +++ b/web/src/pages/EditLocationPage/EditLocationPage.tsx @@ -928,92 +928,71 @@ const EditLocationForm = ({ location }: { location: NetworkLocation }) => { )} - s.values.service_location_mode}> - {(serviceLocationMode) => ( - <> - {serviceLocationMode === LocationServiceMode.Prelogon && ( - - )} - - {postureChecksSectionState.showEmptyState && ( -
- -

- {m.location_posture_checks_empty_state_before_link()}{' '} - - {m.cmp_nav_item_posture_checks()} - {' '} - {m.location_posture_checks_empty_state_after_link()} -

-
- )} - {postureChecksSectionState.showAssignedPostureChecks && ( -
- postureCheck.id), - ) - } - modalTitle={m.location_posture_checks_select()} - editText={m.location_posture_checks_edit()} - editIcon={IconKind.Edit} - toggleValue={false} - counterText={() => ''} - onSelectionChange={(values) => { - setLocationPostures({ - postures: values.filter( - (value): value is number => typeof value === 'number', - ), - }); - }} - onToggleChange={() => {}} - selectionCustomItemRender={renderPostureCheckSelectionItem} - selectionModalProps={{ - contentClassName: 'posture-check-assignment-modal', - enableDividers: true, - itemGap: 12, - searchPlaceholder: m.controls_search(), - visibleItemsLimit: 6, - }} - /> -
- )} - {postureChecksSectionState.showAssignButton && ( -