diff --git a/src-tauri/Cargo.lock b/src-tauri/Cargo.lock index 554e6d83b..88f24e6fc 100644 --- a/src-tauri/Cargo.lock +++ b/src-tauri/Cargo.lock @@ -1680,7 +1680,9 @@ dependencies = [ "serde_json", "sysinfo", "time", + "tokio", "tonic", + "wiremock", "wmi", ] @@ -1745,6 +1747,7 @@ dependencies = [ "base64 0.22.1", "defguard-client-common", "defguard-client-core", + "defguard-client-posture", "defguard-client-proto", "defguard_wireguard_rs", "known-folders", @@ -1752,6 +1755,7 @@ dependencies = [ "prost", "serde", "serde_json", + "tempfile", "thiserror 2.0.19", "tokio", "windows 0.62.2", diff --git a/src-tauri/client-cli/src/commands/connect.rs b/src-tauri/client-cli/src/commands/connect.rs index 72e16d5cc..92e20ebfc 100644 --- a/src-tauri/client-cli/src/commands/connect.rs +++ b/src-tauri/client-cli/src/commands/connect.rs @@ -159,7 +159,7 @@ pub async fn handle( let psk = authorize_posture_session(location) .await .map_err(|e| CliError::Other(e.to_string()))?; - (location.name.clone(), Some(psk), state.app_config.mtu()) + (location.name.clone(), psk, state.app_config.mtu()) } else { (location.name.clone(), None, state.app_config.mtu()) } diff --git a/src-tauri/client-cli/src/resolve.rs b/src-tauri/client-cli/src/resolve.rs index f05366b0b..8a75cfd8b 100644 --- a/src-tauri/client-cli/src/resolve.rs +++ b/src-tauri/client-cli/src/resolve.rs @@ -19,11 +19,27 @@ pub enum ResolvedTarget { Tunnel(Tunnel), } -/// Resolve a target for the `connect` command. +/// Resolve a target for the `connect` command. Rejects service locations. pub async fn resolve_connect_target( spec: &TargetSpec, pool: &DbPool, ) -> Result { + let target = resolve_user_target(spec, pool).await?; + + if let ResolvedTarget::Location(location) = &target { + if location.is_service_location() { + return Err(CliError::InvalidInput(format!( + "'{}' is a service location and is managed by the defguard service", + location.name + ))); + } + } + + Ok(target) +} + +/// Resolution proper, without the service-location check its callers rely on. +async fn resolve_user_target(spec: &TargetSpec, pool: &DbPool) -> Result { // --id fast path if let Some(id) = spec.id { if spec.tunnel { @@ -250,6 +266,71 @@ mod tests { } } + /// Service locations belong to the daemon: connecting to one would make the app and the daemon + /// supersede each other's session indefinitely. `--name` is safe because + /// `Location::find_by_name` filters them out in SQL; `--id` is not safe. + #[sqlx::test(migrations = "../migrations")] + async fn test_service_location_is_not_resolvable(pool: DbPool) { + let i = sample_instance("acme").save(&pool).await.unwrap(); + let mut location = sample_location("headless", i.id); + location.service_location_mode = ServiceLocationMode::AlwaysOn; + let location = location.save(&pool).await.unwrap(); + + let by_name = TargetSpec { + name: Some("headless".into()), + tunnel: false, + id: None, + instance: None, + }; + // Already filtered in SQL, so this reports "not found" rather than reaching the guard. + let err = expect_err(resolve_connect_target(&by_name, &pool).await); + assert!( + matches!(err, CliError::NotFound(_)), + "expected NotFound for a service location by name, got {err:?}" + ); + + let by_id = TargetSpec { + name: None, + tunnel: false, + id: Some(location.id), + instance: None, + }; + let err = expect_err(resolve_connect_target(&by_id, &pool).await); + assert!( + matches!(err, CliError::InvalidInput(_)), + "expected InvalidInput for a service location by id, got {err:?}" + ); + + // Disconnect resolves through the same function, so it is covered too. + let err = expect_err(resolve_disconnect_target(&by_id, &pool).await); + assert!( + matches!(err, CliError::InvalidInput(_)), + "expected InvalidInput when disconnecting a service location, got {err:?}" + ); + } + + /// A regular location on the same instance must still resolve, so the check above is not simply + /// rejecting everything. + #[sqlx::test(migrations = "../migrations")] + async fn test_regular_location_resolves_alongside_a_service_location(pool: DbPool) { + let i = sample_instance("acme").save(&pool).await.unwrap(); + let mut service = sample_location("headless", i.id); + service.service_location_mode = ServiceLocationMode::AlwaysOn; + service.save(&pool).await.unwrap(); + sample_location("office", i.id).save(&pool).await.unwrap(); + + let spec = TargetSpec { + name: Some("office".into()), + tunnel: false, + id: None, + instance: None, + }; + match expect_ok(resolve_connect_target(&spec, &pool).await) { + ResolvedTarget::Location(l) => assert_eq!(l.name, "office"), + _ => panic!("expected Location"), + } + } + #[sqlx::test(migrations = "../migrations")] async fn test_unique_tunnel_by_name(pool: DbPool) { sample_tunnel("gateway").save(&pool).await.unwrap(); diff --git a/src-tauri/client-proto/build.rs b/src-tauri/client-proto/build.rs index 8d982665f..907b1d917 100644 --- a/src-tauri/client-proto/build.rs +++ b/src-tauri/client-proto/build.rs @@ -2,6 +2,8 @@ fn main() -> Result<(), Box> { println!("cargo:rerun-if-changed=../proto"); tonic_prost_build::configure() + // These types contain sensitive data. + .skip_debug(["SaveServiceLocationsRequest"]) // Enable optional fields. .protoc_arg("--experimental_allow_proto3_optional") // Make sure empty DNS is deserialized correctly as `None`. @@ -12,6 +14,18 @@ fn main() -> Result<(), Box> { ) // Make all messages serde-serializable. .type_attribute(".", "#[derive(serde::Serialize,serde::Deserialize)]") + // `ServiceLocation` is persisted as JSON by the daemon. Tolerate these fields being absent + // in files written by older clients. Deliberately per-field rather than a container-level + // `#[serde(default)]`, so a truncated or corrupt file still fails to deserialize instead of + // quietly becoming "no locations". + .field_attribute( + ".defguard.client.v1.ServiceLocation.network_id", + "#[serde(default)]", + ) + .field_attribute( + ".defguard.client.v1.ServiceLocation.posture_check_required", + "#[serde(default)]", + ) // Use proto defaults for missing fields in enrollment types that // may differ across proxy versions. .type_attribute(".defguard.client_types.AdminInfo", "#[serde(default)]") diff --git a/src-tauri/daemon/src/daemon.rs b/src-tauri/daemon/src/daemon.rs index 254e3160f..4ed68becc 100644 --- a/src-tauri/daemon/src/daemon.rs +++ b/src-tauri/daemon/src/daemon.rs @@ -11,7 +11,7 @@ use std::{fs, path::Path}; use defguard_client_common::dns_borrow; #[cfg(windows)] -use defguard_client_posture::inspector::device_posture_data; +use defguard_client_posture::inspector::{device_posture_data, DiskEncryptionTarget}; use defguard_client_proto::defguard::{ client::v1::{ desktop_daemon_service_server::{DesktopDaemonService, DesktopDaemonServiceServer}, @@ -23,7 +23,7 @@ use defguard_client_proto::defguard::{ }; use defguard_client_service_locations::ServiceLocationError; #[cfg(any(windows, target_os = "linux"))] -use defguard_client_service_locations::ServiceLocationManager; +use defguard_client_service_locations::{ReconcileSignal, ServiceLocationManager}; #[cfg(not(target_os = "macos"))] use defguard_wireguard_rs::Kernel; #[cfg(target_os = "macos")] @@ -57,10 +57,12 @@ pub(super) const DAEMON_SOCKET_PATH: &str = "/var/run/defguard.socket"; #[cfg(target_os = "linux")] pub(super) const DAEMON_SOCKET_GROUP: &str = "defguard"; +/// How often the reconciler brings running tunnels back in line with what is on disk. +/// +/// On Windows this is a backstop, since the watchers wake it on network, logon and resume events. On +/// Linux nothing wakes it, so this is the only trigger and sets the worst-case recovery time. #[cfg(any(windows, target_os = "linux"))] -pub(crate) const SERVICE_LOCATION_CONNECT_RETRY_COUNT: u32 = 5; -#[cfg(any(windows, target_os = "linux"))] -pub(crate) const SERVICE_LOCATION_CONNECT_RETRY_DELAY: Duration = Duration::from_secs(30); +pub(crate) const SERVICE_LOCATION_RECONCILE_INTERVAL: Duration = Duration::from_secs(30); #[derive(Debug, thiserror::Error)] pub enum DaemonError { @@ -217,11 +219,7 @@ impl DesktopDaemonService for DaemonService { self.service_location_manager .write() .unwrap() - .save_service_locations( - service_location.service_locations.as_slice(), - &service_location.instance_id, - &service_location.private_key, - ) + .save_service_locations(&service_location) .map_err(|err| { let msg = format!("Failed to save service locations: {err}"); error!(msg); @@ -238,10 +236,11 @@ impl DesktopDaemonService for DaemonService { _request: tonic::Request<()>, ) -> Result, Status> { warn!( - "Daemon service received a get_posture_data request. Daemon posture requests are only supported on windows systems. Unix systems perform client-side posture checks." + "Received a get_posture_data request. Only Windows needs the service to collect posture \ + data; elsewhere the app evaluates it itself." ); Err(Status::unimplemented( - "Service-side posture checks are only supported on Unix systems", + "Service-side posture checks are not supported on this platform", )) } @@ -544,13 +543,20 @@ impl DesktopDaemonService for DaemonService { Ok(Response::new(ListInterfacesResponse { interfaces })) } + /// Collects this device's posture data on the app's behalf. + /// + /// Windows-only, because only SYSTEM can query the WMI encryption namespace. This answers a + /// *user-initiated* check, hence `ClientDatabase` - inert on Windows, where the probe reports on + /// the system volume regardless, but it states which question is being answered. #[cfg(windows)] async fn get_posture_data( &self, _request: tonic::Request<()>, ) -> Result, Status> { debug!("Get posture data request received"); - Ok(Response::new(device_posture_data())) + Ok(Response::new(device_posture_data( + DiskEncryptionTarget::ClientDatabase, + ))) } } @@ -560,14 +566,14 @@ pub async fn run_server(config: Config) -> anyhow::Result<()> { #[cfg(target_os = "linux")] let service_location_manager = Arc::new(RwLock::new(ServiceLocationManager::init()?)); + // Nothing wakes the reconciler on Linux - there are no network, logon or resume watchers - so + // the tick is its only trigger. #[cfg(target_os = "linux")] - tokio::spawn( - defguard_client_service_locations::connect_service_locations( - service_location_manager.clone(), - SERVICE_LOCATION_CONNECT_RETRY_COUNT, - SERVICE_LOCATION_CONNECT_RETRY_DELAY, - ), - ); + tokio::spawn(defguard_client_service_locations::run_reconciler( + service_location_manager.clone(), + ReconcileSignal::default(), + SERVICE_LOCATION_RECONCILE_INTERVAL, + )); let daemon_service = DaemonService::new( &config, diff --git a/src-tauri/daemon/src/windows.rs b/src-tauri/daemon/src/windows.rs index 17e7aad65..5562f7aea 100644 --- a/src-tauri/daemon/src/windows.rs +++ b/src-tauri/daemon/src/windows.rs @@ -8,15 +8,15 @@ use std::{ use clap::Parser; use defguard_client_service_locations::{ windows::{watch_for_login_logoff, watch_for_network_change}, - ServiceLocationError, ServiceLocationManager, + ReconcileSignal, ServiceLocationError, ServiceLocationManager, }; use tokio::runtime::Runtime; -use tracing::{error, info, warn}; +use tracing::{debug, error, info, warn}; use windows_service::{ define_windows_service, service::{ - ServiceControl, ServiceControlAccept, ServiceExitCode, ServiceState, ServiceStatus, - ServiceType, + PowerEventParam, ServiceControl, ServiceControlAccept, ServiceExitCode, ServiceState, + ServiceStatus, ServiceType, }, service_control_handler::{register, ServiceControlHandlerResult}, service_dispatcher, @@ -24,16 +24,12 @@ use windows_service::{ use crate::{ config::Config, - daemon::{ - run_server, DaemonError, SERVICE_LOCATION_CONNECT_RETRY_COUNT, - SERVICE_LOCATION_CONNECT_RETRY_DELAY, - }, + daemon::{run_server, DaemonError, SERVICE_LOCATION_RECONCILE_INTERVAL}, utils::logging_setup, }; static SERVICE_NAME: &str = "DefguardService"; const SERVICE_TYPE: ServiceType = ServiceType::OWN_PROCESS; -const LOGIN_LOGOFF_MONITORING_RESTART_DELAY_SECS: Duration = Duration::from_secs(5); pub fn run() -> Result<(), windows_service::Error> { // Register generated `ffi_service_main` with the system and start the service, blocking @@ -55,6 +51,12 @@ fn run_service() -> Result<(), DaemonError> { let (shutdown_tx, shutdown_rx) = mpsc::channel::(); let shutdown_tx_server = shutdown_tx.clone(); + // One signal, shared by everything that can notice the world changed. Created here because the + // control handler below is registered before the service location manager exists, and a + // `Notify` remembers a wake that arrives before anyone is listening. + let wake_reconciler = ReconcileSignal::default(); + let wake_on_power_event = wake_reconciler.clone(); + // Define system service event handler that will be receiving service events. let event_handler = move |control_event| -> ServiceControlHandlerResult { match control_event { @@ -68,6 +70,22 @@ fn run_service() -> Result<(), DaemonError> { ServiceControlHandlerResult::NoError } + // Resuming from sleep leaves tunnels that were established before the suspend looking + // alive but no longer passing traffic, so wake the reconciler rather than waiting up to a + // full tick. This arrives here and not through `WTSWaitSystemEvent`, which has no power + // event; the service control handler is the only place a service is told. + ServiceControl::PowerEvent(param) => { + debug!("Received power event: {param:?}"); + if matches!( + param, + PowerEventParam::ResumeAutomatic | PowerEventParam::ResumeSuspend + ) { + info!("Resumed from sleep, waking the service location reconciler"); + wake_on_power_event.notify_one(); + } + ServiceControlHandlerResult::NoError + } + _ => ServiceControlHandlerResult::NotImplemented, } }; @@ -82,7 +100,7 @@ fn run_service() -> Result<(), DaemonError> { status_handle.set_service_status(ServiceStatus { service_type: SERVICE_TYPE, current_state: ServiceState::Running, - controls_accepted: ServiceControlAccept::STOP, + controls_accepted: ServiceControlAccept::STOP | ServiceControlAccept::POWER_EVENT, exit_code: ServiceExitCode::Win32(0), checkpoint: 0, wait_hint: Duration::default(), @@ -118,55 +136,33 @@ fn run_service() -> Result<(), DaemonError> { // NotifyAddrChange syscall does not stall Tokio's async worker threads. // Register it first so no network event can be missed before the watcher is listening; // the retry loop below is the backstop for any event that slips through the startup window. - let service_location_manager_clone = service_location_manager.clone(); + let wake = wake_reconciler.clone(); std::thread::Builder::new() .name("network-change-monitor".to_string()) .spawn(move || { info!("Starting network change monitoring"); - watch_for_network_change(service_location_manager_clone); + watch_for_network_change(wake); error!("Network change monitoring ended unexpectedly."); }) .expect("Failed to spawn network change monitor thread"); - // Spawn the service location auto-connect task with retries. Each attempt skips locations - // that are already connected, so it is safe to call repeatedly. The retry loop handles the - // case where the connection fails initially at startup because the network (e.g. Wi-Fi) is - // not yet available (mainly DNS resolution issues), and serves as a backstop for any - // network events missed by the watcher above. - runtime.spawn( - defguard_client_service_locations::connect_service_locations( - service_location_manager.clone(), - SERVICE_LOCATION_CONNECT_RETRY_COUNT, - SERVICE_LOCATION_CONNECT_RETRY_DELAY, - ), - ); + // Spawn the reconciler. Each pass leaves already-correct locations alone, so waking it is + // always safe. Its tick covers startup before the network is ready - typically DNS not yet + // resolving - and backstops any event the watchers miss. + runtime.spawn(defguard_client_service_locations::run_reconciler( + service_location_manager.clone(), + wake_reconciler.clone(), + SERVICE_LOCATION_RECONCILE_INTERVAL, + )); // Spawn login/logoff monitoring on a dedicated OS thread so the blocking // WTSWaitSystemEvent syscall does not stall Tokio's async worker threads. - let service_location_manager_clone = service_location_manager.clone(); + let wake = wake_reconciler.clone(); std::thread::Builder::new() .name("login-logoff-monitor".to_string()) .spawn(move || { info!("Starting login/logoff event monitoring"); - loop { - match watch_for_login_logoff(service_location_manager_clone.clone()) { - Ok(()) => { - warn!( - "Login/logoff event monitoring ended unexpectedly. Restarting in \ - {LOGIN_LOGOFF_MONITORING_RESTART_DELAY_SECS:?}..." - ); - std::thread::sleep(LOGIN_LOGOFF_MONITORING_RESTART_DELAY_SECS); - } - Err(e) => { - error!( - "Error in login/logoff event monitoring: {e}. Restarting in \ - {LOGIN_LOGOFF_MONITORING_RESTART_DELAY_SECS:?}...", - ); - std::thread::sleep(LOGIN_LOGOFF_MONITORING_RESTART_DELAY_SECS); - info!("Restarting login/logoff event monitoring"); - } - } - } + watch_for_login_logoff(&wake); }) .expect("Failed to spawn login/logoff monitor thread"); diff --git a/src-tauri/enterprise/config-sync/src/commands.rs b/src-tauri/enterprise/config-sync/src/commands.rs index 12d70ab2f..aea680ca5 100644 --- a/src-tauri/enterprise/config-sync/src/commands.rs +++ b/src-tauri/enterprise/config-sync/src/commands.rs @@ -5,10 +5,13 @@ use defguard_client_core::{ connection::daemon_client::DAEMON_CLIENT, database::models::wireguard_keys::WireguardKeys, }; use defguard_client_core::{ - database::models::{ - instance::{ClientTrafficPolicy, Instance}, - location::{infer_mfa_method, Location}, - Id, NoId, + database::{ + models::{ + instance::{ClientTrafficPolicy, Instance}, + location::{infer_mfa_method, Location}, + Id, NoId, + }, + DbPool, }, error::Error, into_location, @@ -45,6 +48,7 @@ pub async fn locations_changed( Ok(db_locations != core_locations) } +/// Applies a fetched configuration to the local database. Returns whether the location set changed. pub async fn do_update_instance( transaction: &mut Transaction<'_, Sqlite>, instance: &mut Instance, @@ -143,8 +147,6 @@ pub async fn do_update_instance( ); } debug!("Finished updating locations for instance {instance}"); - - sync_service_locations(transaction, instance).await?; } else { info!("Locations for instance {instance} didn't change. Not updating them."); } @@ -154,16 +156,17 @@ pub async fn do_update_instance( /// Synchronizes the daemon's persisted service-location state from the current database state. /// -/// This is called after location config changes have been applied locally. It sends all currently -/// persisted service locations for the instance to the daemon, or asks the daemon to delete its -/// service-location state when none remain. -pub async fn sync_service_locations( - transaction: &mut Transaction<'_, Sqlite>, - instance: &Instance, -) -> Result<(), Error> { +/// Sends all currently persisted service locations for the instance to the daemon, or asks the +/// daemon to delete its service-location state when none remain. This is the **only** place that +/// builds a `SaveServiceLocationsRequest`, so every caller pushes the same field set. +/// +/// Takes a pool rather than a transaction deliberately: this performs gRPC calls that can each take +/// seconds, and holding a SQLite write transaction open across them would block every other writer. +/// **Call it after the surrounding transaction has committed**, so what is pushed is committed state +/// and a slow or unavailable daemon cannot roll back the database. +pub async fn sync_service_locations(pool: &DbPool, instance: &Instance) -> Result<(), Error> { let mut service_locations = Vec::new(); - let current_locations = - Location::find_by_instance_id(transaction.as_mut(), instance.id, true).await?; + let current_locations = Location::find_by_instance_id(pool, instance.id, true).await?; for location in current_locations { if location.is_service_location() { debug!( @@ -212,15 +215,18 @@ pub async fn sync_service_locations( #[cfg(not(target_os = "macos"))] { - let private_key = WireguardKeys::find_by_instance_id(transaction.as_mut(), instance.id) + let keys = WireguardKeys::find_by_instance_id(pool, instance.id) .await? - .ok_or(Error::NotFound)? - .prvkey; + .ok_or(Error::NotFound)?; let save_request = SaveServiceLocationsRequest { service_locations: service_locations.clone(), instance_id: instance.uuid.clone(), - private_key, + private_key: keys.prvkey, + proxy_url: instance.proxy_url.clone(), + // The device's own public key, not a remote peer's key. + device_pubkey: keys.pubkey, + token: instance.token.clone(), }; debug!( diff --git a/src-tauri/enterprise/config-sync/src/lib.rs b/src-tauri/enterprise/config-sync/src/lib.rs index 909a308e0..8067873ee 100644 --- a/src-tauri/enterprise/config-sync/src/lib.rs +++ b/src-tauri/enterprise/config-sync/src/lib.rs @@ -20,7 +20,7 @@ use semver::Version; use serde::Serialize; use sqlx::{Sqlite, Transaction}; -use crate::commands::{disable_enterprise_features, do_update_instance}; +use crate::commands::{disable_enterprise_features, do_update_instance, sync_service_locations}; static POLLING_ENDPOINT: &str = "/api/v1/poll"; @@ -250,6 +250,28 @@ pub async fn poll_instances( } transaction.commit().await?; + + // Push to the daemon only after committing: `sync_service_locations` makes gRPC calls that + // would otherwise hold this write transaction open, and a failure here must not undo config + // that is already correct in the database - which would also discard a freshly rotated + // polling token. + // + // Pushed for *every* instance on *every* cycle, not just those the poll changed. That is what + // makes a failed push self-healing: the next cycle simply pushes again, with no record of the + // failure to keep. It costs nothing when nothing changed, because the daemon compares the + // request against what it already has and returns without touching any tunnel. + for instance in &instances { + if let Err(err) = sync_service_locations(pool, instance).await { + // Deliberately not propagated: the database is already committed and correct, and one + // unavailable daemon must not discard the polling outcomes of every other instance. + // The next cycle retries. + error!( + "Failed to push service locations to the daemon for instance {}({}): {err}.", + instance.name, instance.id + ); + } + } + Ok(outcomes) } diff --git a/src-tauri/enterprise/posture/Cargo.toml b/src-tauri/enterprise/posture/Cargo.toml index eac4b9655..d27def6d1 100644 --- a/src-tauri/enterprise/posture/Cargo.toml +++ b/src-tauri/enterprise/posture/Cargo.toml @@ -27,3 +27,7 @@ sysinfo = { version = "0.39", default-features = false, features = ["system"] } sysinfo = { version = "0.39", default-features = false, features = ["system"] } time = { version = "0.3", features = ["formatting", "macros", "serde"] } wmi = { version = "0.18", default-features = false } + +[dev-dependencies] +tokio.workspace = true +wiremock.workspace = true diff --git a/src-tauri/enterprise/posture/src/inspector/linux.rs b/src-tauri/enterprise/posture/src/inspector/linux.rs index eabb86790..86e903352 100644 --- a/src-tauri/enterprise/posture/src/inspector/linux.rs +++ b/src-tauri/enterprise/posture/src/inspector/linux.rs @@ -5,8 +5,6 @@ use std::{ process::Command, }; -use defguard_client_core::database::db_file_path; - use super::UnavailableReason; /// Path to the kernel's mount table for the current process. @@ -40,10 +38,14 @@ struct MountEntry { /// (bcachefs native encryption, fscrypt on ext4/f2fs, eCryptfs) are not detected /// and resolve to `DetectionFailed` - fail-safe: a required posture rule fails /// rather than falsely passing. -pub(super) fn disk_encryption_status() -> Result { - // Resolve the database file and the mount that backs it. - let db_path = db_file_path().ok_or(UnavailableReason::DetectionFailed)?; - let db_path = canonicalize_on_disk(&db_path).ok_or(UnavailableReason::DetectionFailed)?; +/// Reports whether the device stack backing `path` includes an encryption layer. +/// +/// `path` is supplied by the caller rather than derived here, because who is asking changes the +/// answer: a user-initiated check means the partition holding the client database, while the service +/// means `/`. Deriving it internally would silently answer for whichever process happened to call. +pub(super) fn disk_encryption_status(path: &Path) -> Result { + // Resolve the target and the mount that backs it. + let db_path = canonicalize_on_disk(path).ok_or(UnavailableReason::DetectionFailed)?; let mountinfo = read_to_string(MOUNTINFO_PATH).map_err(|_| UnavailableReason::DetectionFailed)?; diff --git a/src-tauri/enterprise/posture/src/inspector/mod.rs b/src-tauri/enterprise/posture/src/inspector/mod.rs index b60209b7d..f7ae2bcb7 100644 --- a/src-tauri/enterprise/posture/src/inspector/mod.rs +++ b/src-tauri/enterprise/posture/src/inspector/mod.rs @@ -15,6 +15,22 @@ use defguard_client_proto::defguard::enterprise::posture::v2::{ }; use sysinfo::System; +/// Which filesystem the disk-encryption check should be evaluated against. +/// +/// Only matters on Linux, whose probe inspects the device stack backing a specific path; Windows and +/// macOS query the system volume regardless. It exists because the two callers legitimately mean +/// different things, and the previous parameterless probe silently answered for whichever process +/// happened to call it. +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub enum DiskEncryptionTarget { + /// The partition backing the client's own database, i.e. the logged-in user's data. What a + /// user-initiated posture check should report on. + ClientDatabase, + /// The root filesystem. What the service reports on: it has no user session, so there is no user + /// database to resolve, and asking for one as root would resolve to root's home directory. + RootFilesystem, +} + /// Returns the operating system name. fn os_name() -> Result { System::name().ok_or(UnavailableReason::DetectionFailed) @@ -49,8 +65,15 @@ fn linux_kernel_version() -> Result { } } -/// Returns the disk encryption status, preferably for the system volume. -fn disk_encryption_status() -> Result { +/// Returns the disk encryption status for `target`. +/// +/// `target` is only consulted on Linux; Windows and macOS report on the system volume whatever is +/// asked for. +fn disk_encryption_status(target: DiskEncryptionTarget) -> Result { + // Only the Linux probe is path-sensitive. + #[cfg(not(target_os = "linux"))] + let _ = target; + #[cfg(target_os = "macos")] { macos::disk_encryption_status() @@ -63,7 +86,12 @@ fn disk_encryption_status() -> Result { #[cfg(target_os = "linux")] { - linux::disk_encryption_status() + let path = match target { + DiskEncryptionTarget::ClientDatabase => defguard_client_core::database::db_file_path() + .ok_or(UnavailableReason::DetectionFailed)?, + DiskEncryptionTarget::RootFilesystem => std::path::PathBuf::from("/"), + }; + linux::disk_encryption_status(&path) } } @@ -118,13 +146,13 @@ fn security_update_age_days() -> Result { } #[must_use] -pub fn device_posture_data() -> DevicePostureData { +pub fn device_posture_data(disk_target: DiskEncryptionTarget) -> DevicePostureData { DevicePostureData { defguard_client_version: PKG_VERSION.to_owned(), os_type: OS.to_string(), os_name: Some(StringCheck::from(os_name())), os_version: Some(StringCheck::from(os_version())), - disk_encryption: Some(BoolCheck::from(disk_encryption_status())), + disk_encryption: Some(BoolCheck::from(disk_encryption_status(disk_target))), antivirus_present: Some(BoolCheck::from(anti_virus_status())), windows_ad_domain_joined: Some(BoolCheck::from(part_of_domain())), windows_security_update_age_days: Some(Int32Check::from(security_update_age_days())), diff --git a/src-tauri/enterprise/posture/src/inspector/tests/ci/linux.rs b/src-tauri/enterprise/posture/src/inspector/tests/ci/linux.rs index 712c2083e..b9bde6579 100644 --- a/src-tauri/enterprise/posture/src/inspector/tests/ci/linux.rs +++ b/src-tauri/enterprise/posture/src/inspector/tests/ci/linux.rs @@ -1,6 +1,8 @@ use std::{fs::read_to_string, process::Command}; -use super::super::super::{disk_encryption_status, linux_kernel_version, os_name, os_version}; +use super::super::super::{ + disk_encryption_status, linux_kernel_version, os_name, os_version, DiskEncryptionTarget, +}; fn expected_kernel_version() -> String { let output = Command::new("uname") @@ -73,7 +75,7 @@ mod setup1 { #[test] #[ignore = "CI posture testing only"] fn test_disk_encryption_status_unencrypted() { - assert!(!disk_encryption_status().unwrap()); + assert!(!disk_encryption_status(DiskEncryptionTarget::ClientDatabase).unwrap()); } } @@ -101,6 +103,6 @@ mod setup2 { #[test] #[ignore = "CI posture testing only"] fn test_disk_encryption_status_encrypted() { - assert!(disk_encryption_status().unwrap()); + assert!(disk_encryption_status(DiskEncryptionTarget::ClientDatabase).unwrap()); } } diff --git a/src-tauri/enterprise/posture/src/inspector/tests/linux.rs b/src-tauri/enterprise/posture/src/inspector/tests/linux.rs index 14357d896..b00b9264a 100644 --- a/src-tauri/enterprise/posture/src/inspector/tests/linux.rs +++ b/src-tauri/enterprise/posture/src/inspector/tests/linux.rs @@ -1,4 +1,4 @@ -use super::super::{disk_encryption_status, os_name, os_version}; +use super::super::{disk_encryption_status, os_name, os_version, DiskEncryptionTarget}; #[test] fn test_os_name() { @@ -13,5 +13,22 @@ fn test_os_version() { #[test] #[ignore = "development machine only"] fn test_disk_encryption() { - assert!(!disk_encryption_status().unwrap()); + assert!(!disk_encryption_status(DiskEncryptionTarget::ClientDatabase).unwrap()); +} + +/// A path that does not exist yet still resolves: `canonicalize_on_disk` walks up to the nearest +/// existing ancestor, which is deliberate, since the client database may not have been created yet. +/// +/// It is also why the probe must be told which path to report on. Handed a path under a data directory +/// that does not exist - which is what resolving the *user's* database as root produces - it does not +/// fail loudly, it quietly answers for an ancestor instead. The answer looks plausible and is about +/// the wrong filesystem. +#[test] +fn test_nonexistent_path_resolves_to_its_nearest_existing_ancestor() { + let missing = std::path::Path::new("/nonexistent-defguard-posture-target/data/db.sqlite"); + assert_eq!( + super::super::linux::disk_encryption_status(missing), + super::super::linux::disk_encryption_status(std::path::Path::new("/")), + "a path whose ancestors are all missing must report the same as `/`" + ); } diff --git a/src-tauri/enterprise/posture/src/lib.rs b/src-tauri/enterprise/posture/src/lib.rs index da9d2d7b8..4e80e73b2 100644 --- a/src-tauri/enterprise/posture/src/lib.rs +++ b/src-tauri/enterprise/posture/src/lib.rs @@ -4,4 +4,4 @@ extern crate log; pub mod inspector; pub mod posture; -pub use posture::{authorize_posture_session, get_posture_data}; +pub use posture::{authorize_posture_session, get_posture_data, request_posture_authorization}; diff --git a/src-tauri/enterprise/posture/src/posture.rs b/src-tauri/enterprise/posture/src/posture.rs index 90d7bb60c..eec088a6e 100644 --- a/src-tauri/enterprise/posture/src/posture.rs +++ b/src-tauri/enterprise/posture/src/posture.rs @@ -15,12 +15,16 @@ use reqwest::{StatusCode, Url}; use serde::Deserialize; #[cfg(not(windows))] -use crate::inspector::device_posture_data; +use crate::inspector::{device_posture_data, DiskEncryptionTarget}; const POSTURE_ENDPOINT: &str = "/api/v1/posture/connect"; -/// Collects device posture data, sends it to the proxy, and returns the runtime preshared key. -pub async fn authorize_posture_session(location: &Location) -> Result { +/// Collects device posture data, sends it to the proxy, and returns the optional runtime preshared +/// key. Core approves without a key when posture checks were removed from the location. +/// +/// The app's entry point: reads the instance, keys and token from the local database. The daemon has +/// no database, so it calls [`request_posture_authorization`] directly with values from its RPC. +pub async fn authorize_posture_session(location: &Location) -> Result, Error> { let instance = Instance::find_by_id(&*DB_POOL, location.instance_id) .await? .ok_or(Error::NotFound)?; @@ -43,20 +47,45 @@ pub async fn authorize_posture_session(location: &Location) -> Result Result, Error> { let request = DevicePostureCheckRequest { - location_id: location.network_id, - pubkey: keys.pubkey, + location_id, + pubkey: device_pubkey, device_posture_data: Some(posture_data), token: Some(token), }; - let proxy_url = Url::parse(&instance.proxy_url) + let url = Url::parse(proxy_url) .map_err(|e| Error::InternalError(format!("Invalid proxy URL: {e}")))? .join(POSTURE_ENDPOINT) .map_err(|e| Error::InternalError(format!("Failed to build posture URL: {e}")))?; - debug!("Sending posture check request to {proxy_url}"); - let response = post_with_headers(proxy_url, &request) + debug!("Sending posture check request to {url}"); + let response = post_with_headers(url, &request) .await .map_err(|e| Error::ServiceUnavailable(e.to_string()))?; @@ -66,8 +95,8 @@ pub async fn authorize_posture_session(location: &Location) -> Result { #[derive(Deserialize)] @@ -79,8 +108,8 @@ pub async fn authorize_posture_session(location: &Location) -> Result) -> Result Result { #[cfg(windows)] { @@ -108,6 +142,6 @@ pub async fn get_posture_data() -> Result { } #[cfg(not(windows))] { - Ok(device_posture_data()) + Ok(device_posture_data(DiskEncryptionTarget::ClientDatabase)) } } diff --git a/src-tauri/enterprise/service-locations/Cargo.toml b/src-tauri/enterprise/service-locations/Cargo.toml index 39af6b739..8fcb07b1f 100644 --- a/src-tauri/enterprise/service-locations/Cargo.toml +++ b/src-tauri/enterprise/service-locations/Cargo.toml @@ -11,6 +11,7 @@ version.workspace = true [dependencies] defguard-client-common = { path = "../../common" } defguard-client-core = { path = "../../core" } +defguard-client-posture = { path = "../posture" } defguard-client-proto = { path = "../../client-proto" } defguard_wireguard_rs.workspace = true base64.workspace = true @@ -21,6 +22,9 @@ serde_json.workspace = true thiserror.workspace = true tokio = { workspace = true, features = ["time"] } +[dev-dependencies] +tempfile.workspace = true + [target.'cfg(windows)'.dependencies] known-folders = "1.4" windows = "0.62" diff --git a/src-tauri/enterprise/service-locations/src/lib.rs b/src-tauri/enterprise/service-locations/src/lib.rs index 311094c23..e9c5df5b9 100644 --- a/src-tauri/enterprise/service-locations/src/lib.rs +++ b/src-tauri/enterprise/service-locations/src/lib.rs @@ -1,8 +1,8 @@ -use std::{collections::HashMap, fmt}; +use std::{collections::HashMap, fmt, fs, path::Path}; #[cfg(any(windows, target_os = "linux"))] use std::{ sync::{Arc, RwLock}, - time::Duration, + time::{Duration, SystemTime}, }; use defguard_client_core::{ @@ -12,13 +12,18 @@ use defguard_client_core::{ }, error::Error as CoreError, }; +#[cfg(any(windows, target_os = "linux"))] +use defguard_client_posture::{ + inspector::{device_posture_data, DiskEncryptionTarget}, + request_posture_authorization, +}; use defguard_client_proto::defguard::client::v1::{ - ServiceLocation, ServiceLocationMode as ProtoServiceLocationMode, + SaveServiceLocationsRequest, ServiceLocation, ServiceLocationMode as ProtoServiceLocationMode, }; use defguard_wireguard_rs::{error::WireguardInterfaceError, WGApi}; -#[cfg(any(windows, target_os = "linux"))] -use log::info; use log::warn; +#[cfg(any(windows, target_os = "linux"))] +use log::{debug, error, info}; use serde::{Deserialize, Serialize}; #[cfg(target_os = "linux")] @@ -58,14 +63,41 @@ pub struct ServiceLocationManager { wgapis: HashMap, // Instance ID: Service locations connected under that instance connected_service_locations: HashMap>, + #[cfg(any(windows, target_os = "linux"))] + configuration_generation: u64, + // (Instance ID, location public key): when its posture session was last approved. + // + // Kept beside `connected_service_locations` rather than folded into it: the alternative meant + // changing that map's element type at every one of its call sites, most of them in Windows code + // that cannot be compiled here. Entries for locations that are no longer connected are pruned + // during the health check, so the two cannot drift apart for long. + #[cfg(any(windows, target_os = "linux"))] + posture_sessions: HashMap<(String, String), SystemTime>, } +/// Current schema version of the on-disk service location JSON file. +/// +/// Files written by older clients predate versioning and deserialize with `schema_version == 0` +/// (see the `#[serde(default)]` on [`ServiceLocationData::schema_version`]). +pub const SERVICE_LOCATION_SCHEMA_VERSION: u32 = 1; + #[allow(dead_code)] #[derive(Serialize, Deserialize)] pub struct ServiceLocationData { pub service_locations: Vec, pub instance_id: String, pub private_key: String, + #[serde(default)] + pub proxy_url: String, + /// The *device's* WireGuard public key (`WireguardKeys.pubkey`), used to identify this device + /// to the proxy. This is **not** [`ServiceLocation::pubkey`], which is the remote peer key. + #[serde(default)] + pub device_pubkey: String, + /// Device polling token, used to authenticate posture requests made by the service. + #[serde(default)] + pub token: Option, + #[serde(default)] + pub schema_version: u32, } #[allow(dead_code)] @@ -81,10 +113,40 @@ impl fmt::Debug for ServiceLocationData { .field("service_locations", &self.service_locations) .field("instance_id", &self.instance_id) .field("private_key", &"***") + .field("proxy_url", &self.proxy_url) + .field("device_pubkey", &self.device_pubkey) + .field("token", &self.token.as_ref().map(|_| "***")) + .field("schema_version", &self.schema_version) .finish() } } +impl ServiceLocationData { + /// Builds the on-disk representation from a daemon save request. + /// + /// `service_locations` is passed separately rather than taken from the request because each + /// platform first filters the requested set down to the modes it supports. + /// + /// Every other field is copied from the request here and nowhere else, so a field added to + /// `SaveServiceLocationsRequest` has exactly one place to be wired in — it cannot be silently + /// dropped on the way to disk by one platform but not the other. + #[must_use] + pub fn from_save_request( + request: &SaveServiceLocationsRequest, + service_locations: Vec, + ) -> Self { + Self { + service_locations, + instance_id: request.instance_id.clone(), + private_key: request.private_key.clone(), + proxy_url: request.proxy_url.clone(), + device_pubkey: request.device_pubkey.clone(), + token: request.token.clone(), + schema_version: SERVICE_LOCATION_SCHEMA_VERSION, + } + } +} + impl fmt::Debug for SingleServiceLocationData { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { f.debug_struct("SingleServiceLocationData") @@ -95,6 +157,20 @@ impl fmt::Debug for SingleServiceLocationData { } } +/// Whether the file at `path` already holds exactly `contents`. +/// +/// This is what makes a save idempotent. Service locations are pushed on **every** poll cycle so a +/// failed push retries without any bookkeeping, which means the overwhelmingly common case is that +/// nothing changed. Saving is not a cheap no-op by default: it ends by disconnecting and +/// reconnecting every tunnel, so a save that proceeded regardless would drop working tunnels at the +/// poll interval forever. Comparing here lets the caller return before any of that. +/// +/// A read failure counts as "differs", so a missing or unreadable file is simply rewritten. +#[must_use] +pub fn is_unchanged_on_disk(path: &Path, contents: &str) -> bool { + fs::read_to_string(path).is_ok_and(|existing| existing == contents) +} + pub fn to_service_location(location: &Location) -> Result { if !location.is_service_location() { warn!("Location {location} is not a service location, so it can't be converted to one."); @@ -129,43 +205,571 @@ pub fn to_service_location(location: &Location) -> Result>, - retry_count: u32, - retry_delay: Duration, -) { - for attempt in 1..=retry_count { - info!("Attempting to auto-connect service locations (attempt {attempt}/{retry_count})"); - match manager.write().unwrap().connect_to_service_locations() { - Ok(true) => { +pub const POSTURE_SESSION_STALE_AFTER: Duration = Duration::from_secs(180); + +/// Whether a posture session needs renewing. +/// +/// The interface is the only honest source here. A location the daemon believes it connected can be +/// dead: while a machine sleeps, core's `peer_disconnect_threshold` elapses and the gateway drops the +/// peer, leaving an interface that looks perfectly healthy and passes nothing. A handshake is the +/// evidence that the far side still has us. +/// +/// `authorized_at` covers the case where no handshake has happened yet, which is normal immediately +/// after connecting and suspicious a few minutes later. It is the one thing here that cannot be +/// recovered from the interface, which is why it has to be remembered. +#[cfg(any(windows, target_os = "linux"))] +#[must_use] +pub fn posture_session_is_stale( + last_handshake: Option, + authorized_at: Option, + now: SystemTime, +) -> bool { + let beyond_threshold = |moment: SystemTime| { + now.duration_since(moment) + .is_ok_and(|elapsed| elapsed > POSTURE_SESSION_STALE_AFTER) + }; + + match (last_handshake, authorized_at) { + // A handshake is the strongest evidence available, so it wins whenever there is one. + (Some(handshake), _) => beyond_threshold(handshake), + // Never handshaken: expected just after connecting, suspicious much later. + (None, Some(authorized)) => beyond_threshold(authorized), + // Neither, so the daemon has no record of authorizing this at all. Renewing is the safe + // reading: at worst it is redundant, whereas assuming health leaves a dead tunnel up. + (None, None) => true, + } +} + +/// A location that cannot be connected until a posture check approves it. +/// +/// Carries everything the request needs, so authorization can happen with no lock held. Note +/// `network_id` is core's id for the location, which is what the posture endpoint expects, and +/// `device_pubkey` is this device's key rather than the remote peer's. +#[cfg(any(windows, target_os = "linux"))] +#[derive(Debug)] +pub struct PostureAuthorizationRequest { + pub instance_id: String, + pub location_pubkey: String, + pub location_name: String, + pub network_id: i64, + pub proxy_url: String, + pub device_pubkey: String, + pub token: Option, + pub configuration_generation: u64, +} + +/// Posture approvals obtained this pass, keyed by (instance id, location public key). +/// +/// An absent map entry means authorization failed and leaves the location alone. A present entry +/// with no key means core approved connecting without a PSK because posture checks were removed. +#[cfg(any(windows, target_os = "linux"))] +pub struct PostureAuthorization { + configuration_generation: u64, + preshared_key: Option, +} + +#[cfg(any(windows, target_os = "linux"))] +pub type PostureAuthorizations = HashMap<(String, String), PostureAuthorization>; + +/// What one reconcile pass should do with a persisted service location. +/// +/// An authorization is present only when posture authorization succeeded during this pass. Its key +/// may be absent when posture checks were removed from the location. +#[cfg(any(windows, target_os = "linux"))] +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum ReconcileAction<'a> { + LeaveConnected, + WaitForAuthorization, + Renew(Option<&'a str>), + Connect(Option<&'a str>), +} + +#[cfg(any(windows, target_os = "linux"))] +#[must_use] +pub(crate) fn reconcile_action( + is_connected: bool, + posture_check_required: bool, + authorization: Option<&PostureAuthorization>, + configuration_generation: u64, +) -> ReconcileAction<'_> { + let authorization = authorization + .filter(|authorization| authorization.configuration_generation == configuration_generation) + .map(|authorization| authorization.preshared_key.as_deref()); + + if is_connected { + return authorization.map_or(ReconcileAction::LeaveConnected, ReconcileAction::Renew); + } + + if posture_check_required && authorization.is_none() { + ReconcileAction::WaitForAuthorization + } else { + ReconcileAction::Connect(authorization.flatten()) + } +} + +/// Obtains a preshared key for each location that needs one. +/// +/// Posture data is collected once per pass rather than once per location: on Windows that is a WMI +/// query, and every location on a machine reports the same posture anyway. +/// +/// Failures are logged and skipped, never propagated. A rejected device and an unreachable proxy are +/// treated alike - the location simply is not connected, and the next pass tries again. +#[cfg(any(windows, target_os = "linux"))] +async fn authorize_pending(pending: Vec) -> PostureAuthorizations { + let mut authorizations = PostureAuthorizations::new(); + if pending.is_empty() { + return authorizations; + } + + debug!( + "{} service location(s) need a posture check before they can be connected", + pending.len() + ); + let posture_data = device_posture_data(DiskEncryptionTarget::RootFilesystem); + + for request in pending { + let Some(token) = request.token.clone().filter(|token| !token.is_empty()) else { + error!( + "Cannot run a posture check for service location '{}': no polling token was stored \ + for its instance. Re-enrolling the device will store one.", + request.location_name + ); + continue; + }; + + match request_posture_authorization( + &request.proxy_url, + request.device_pubkey.clone(), + request.network_id, + token, + posture_data.clone(), + ) + .await + { + Ok(preshared_key) => { info!( - "All service locations connected successfully (attempt {attempt}/{retry_count})" + "Posture check approved for service location '{}'", + request.location_name + ); + authorizations.insert( + (request.instance_id, request.location_pubkey), + PostureAuthorization { + configuration_generation: request.configuration_generation, + preshared_key, + }, ); - break; } + Err(err) => error!( + "Posture check failed for service location '{}': {err}. It will stay disconnected \ + and be retried.", + request.location_name + ), + } + } + + authorizations +} + +#[cfg(any(windows, target_os = "linux"))] +impl ServiceLocationManager { + pub(crate) fn note_configuration_changed(&mut self) { + self.configuration_generation = self.configuration_generation.wrapping_add(1); + } + + /// Forgets posture sessions for locations that are no longer connected. + /// + /// Keeps this map from being a second, drifting source of truth: a location that is removed or + /// disconnected would otherwise leave its timestamp behind forever, and a location reconnected + /// later would inherit it and look healthier than it is. + pub(crate) fn prune_posture_sessions(&mut self) { + self.posture_sessions.retain(|(instance_id, pubkey), _| { + self.connected_service_locations + .get(instance_id) + .is_some_and(|locations| { + locations.iter().any(|location| location.pubkey == *pubkey) + }) + }); + } +} + +/// Signal used to wake the reconciler before its next tick. +/// +/// `notify_one` is callable from synchronous code, which matters because the Windows watchers are +/// plain OS threads wrapping blocking syscalls. A wake that arrives while a pass is already running +/// is remembered rather than dropped, so an event can never be missed by arriving at a bad moment. +#[cfg(any(windows, target_os = "linux"))] +pub type ReconcileSignal = std::sync::Arc; + +/// Brings the running tunnels in line with what is on disk, forever. +/// +/// Replaces a retry loop that gave up permanently after a fixed number of attempts, which left a +/// machine that booted before its network was ready disconnected until the service restarted. This +/// never gives up: every tick it looks at what should be running and fixes the difference. +/// +/// Each pass is idempotent - already-correct locations are left alone - so waking it spuriously +/// costs nothing, and callers are free to wake it whenever something *might* have changed rather +/// than working out whether it did. +/// +/// `wake` is the only way to react faster than `tick`. On Windows it is signalled by the network, +/// logon and resume watchers. **On Linux nothing signals it**, so there the tick is the sole trigger +/// and recovery from any disruption takes up to one interval. +#[cfg(any(windows, target_os = "linux"))] +pub async fn run_reconciler( + manager: Arc>, + wake: ReconcileSignal, + tick: Duration, +) { + info!("Service location reconciler started, reconciling every {tick:?}"); + + loop { + // Authorize first, mutate second. Working out what needs a posture check takes only a read + // guard, the checks themselves are HTTP round trips of up to 5s each and are made with no + // guard at all, and only the final step takes the write guard. + // + // The ordering is enforced rather than merely intended: these are `std` guards, so they are + // `!Send` and holding one across the await below would not compile. + let pending = { + let manager = manager.read().unwrap(); + manager.locations_needing_authorization() + }; + + let authorizations = authorize_pending(pending).await; + + let outcome = { + let mut manager = manager.write().unwrap(); + manager.reconcile(&authorizations) + }; + + match outcome { + Ok(true) => debug!("Service locations reconciled, everything is as it should be"), Ok(false) => warn!( - "Service location auto-connect attempt {attempt}/{retry_count} completed with some \ - failures" + "Service location reconcile pass completed with failures, retrying in {tick:?}" ), Err(err) => { - warn!("Service location auto-connect attempt {attempt}/{retry_count} failed: {err}") + warn!("Service location reconcile pass failed: {err}. Retrying in {tick:?}"); } } - if attempt < retry_count { - tokio::time::sleep(retry_delay).await; + tokio::select! { + () = tokio::time::sleep(tick) => {} + () = wake.notified() => debug!("Service location reconciler woken early by an event"), } } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[cfg(any(windows, target_os = "linux"))] + mod staleness { + use super::*; + + fn ago(seconds: u64) -> SystemTime { + SystemTime::UNIX_EPOCH + Duration::from_secs(1_000_000 - seconds) + } + + fn now() -> SystemTime { + SystemTime::UNIX_EPOCH + Duration::from_secs(1_000_000) + } + + #[test] + fn a_recent_handshake_is_healthy() { + assert!(!posture_session_is_stale( + Some(ago(10)), + Some(ago(10_000)), + now() + )); + } + + /// A handshake outranks `authorized_at`: the far side has stopped answering, and having + /// authorized recently does not make the tunnel work. + #[test] + fn an_old_handshake_is_stale_even_if_just_authorized() { + assert!(posture_session_is_stale( + Some(ago(1_000)), + Some(ago(1)), + now() + )); + } + + /// Expected right after connecting - there has been no traffic to handshake for yet. + #[test] + fn no_handshake_yet_is_healthy_if_authorized_recently() { + assert!(!posture_session_is_stale(None, Some(ago(10)), now())); + } + + /// The suspend case: authorized long ago, never handshaken, so nothing says it works. + #[test] + fn no_handshake_long_after_authorizing_is_stale() { + assert!(posture_session_is_stale(None, Some(ago(1_000)), now())); + } + + /// No record at all. Renewing is redundant at worst; assuming health leaves a dead tunnel up. + #[test] + fn no_evidence_at_all_is_stale() { + assert!(posture_session_is_stale(None, None, now())); + } + + /// A clock that moved backwards must not read as "ancient", which would renew every pass. + #[test] + fn a_handshake_in_the_future_is_not_stale() { + let future = now() + Duration::from_secs(60); + assert!(!posture_session_is_stale(Some(future), None, now())); + } + + #[test] + fn the_threshold_boundary_is_not_yet_stale() { + assert!(!posture_session_is_stale( + Some(now() - POSTURE_SESSION_STALE_AFTER), + None, + now() + )); + } + } + + #[cfg(any(windows, target_os = "linux"))] + mod reconciliation { + use super::*; + + fn authorization( + configuration_generation: u64, + preshared_key: Option<&str>, + ) -> PostureAuthorization { + PostureAuthorization { + configuration_generation, + preshared_key: preshared_key.map(str::to_string), + } + } + + #[test] + fn connected_location_with_fresh_key_is_renewed() { + let authorization = authorization(1, Some("fresh-key")); + assert_eq!( + reconcile_action(true, true, Some(&authorization), 1), + ReconcileAction::Renew(Some("fresh-key")) + ); + } + + #[test] + fn approval_without_a_key_connects_without_a_key() { + let authorization = authorization(1, None); + assert_eq!( + reconcile_action(false, true, Some(&authorization), 1), + ReconcileAction::Connect(None) + ); + } - info!("Service location auto-connect task finished"); + #[test] + fn approval_without_a_key_removes_the_old_key_from_a_connected_location() { + let authorization = authorization(1, None); + assert_eq!( + reconcile_action(true, true, Some(&authorization), 1), + ReconcileAction::Renew(None) + ); + } + + #[test] + fn authorization_failure_keeps_a_posture_location_disconnected() { + assert_eq!( + reconcile_action(false, true, None, 1), + ReconcileAction::WaitForAuthorization + ); + } + + #[test] + fn authorization_from_an_older_configuration_is_discarded() { + let authorization = authorization(1, Some("stale-key")); + assert_eq!( + reconcile_action(false, true, Some(&authorization), 2), + ReconcileAction::WaitForAuthorization + ); + } + } + + /// A save is a no-op only if this comparison is exact: getting it wrong does not merely cost a + /// disk write, it drops and rebuilds every tunnel on the box. + #[test] + fn unchanged_contents_are_detected() { + let dir = tempfile::tempdir().expect("failed to create temp dir"); + let path = dir.path().join("locations.json"); + + assert!( + !is_unchanged_on_disk(&path, "first"), + "a missing file must count as changed" + ); + + fs::write(&path, "first").expect("failed to write"); + assert!( + is_unchanged_on_disk(&path, "first"), + "identical contents must be detected as unchanged" + ); + assert!( + !is_unchanged_on_disk(&path, "second"), + "different contents must be detected as changed" + ); + assert!( + !is_unchanged_on_disk(&path, "first "), + "a trailing-whitespace difference must still count as changed" + ); + } + + /// JSON exactly as written by a client that predates posture checks: no `proxy_url`, + /// `device_pubkey`, `token` or `schema_version` at the top level, and no `network_id` or + /// `posture_check_required` inside the locations. This is the upgrade path. + const LEGACY_JSON: &str = r#"{ + "service_locations": [ + { + "name": "Office", + "address": "10.0.0.2/24", + "pubkey": "remote-peer-pubkey", + "endpoint": "vpn.example.com:51820", + "allowed_ips": "10.0.0.0/24", + "keepalive_interval": 25, + "dns": "10.0.0.1", + "mode": 2 + } + ], + "instance_id": "d3a5b1f0-0000-0000-0000-000000000001", + "private_key": "device-private-key" + }"#; + + #[test] + fn legacy_json_without_new_fields_deserializes_with_defaults() { + let data: ServiceLocationData = serde_json::from_str(LEGACY_JSON) + .expect("legacy service location file must still load"); + + assert_eq!(data.instance_id, "d3a5b1f0-0000-0000-0000-000000000001"); + assert_eq!(data.private_key, "device-private-key"); + assert_eq!(data.proxy_url, ""); + assert_eq!(data.device_pubkey, ""); + assert_eq!(data.token, None); + // 0 marks a file written before schema versioning existed. + assert_eq!(data.schema_version, 0); + + assert_eq!(data.service_locations.len(), 1); + let location = &data.service_locations[0]; + assert_eq!(location.name, "Office"); + assert_eq!(location.pubkey, "remote-peer-pubkey"); + assert_eq!(location.network_id, 0); + assert!(!location.posture_check_required); + } + + #[test] + fn truncated_json_still_fails_to_deserialize() { + // A container-level `#[serde(default)]` on `ServiceLocation` would let malformed entries + // silently vanish; make sure missing required keys are still an error. + let json = r#"{ + "service_locations": [{ "network_id": 7 }], + "instance_id": "id", + "private_key": "key" + }"#; + + assert!(serde_json::from_str::(json).is_err()); + } + + #[test] + fn round_trip_preserves_new_fields() { + let data = ServiceLocationData { + service_locations: vec![ServiceLocation { + name: "Office".into(), + address: "10.0.0.2/24".into(), + pubkey: "remote-peer-pubkey".into(), + endpoint: "vpn.example.com:51820".into(), + allowed_ips: "10.0.0.0/24".into(), + keepalive_interval: 25, + dns: "10.0.0.1".into(), + mode: ProtoServiceLocationMode::AlwaysOn as i32, + network_id: 42, + posture_check_required: true, + }], + instance_id: "instance-uuid".into(), + private_key: "device-private-key".into(), + proxy_url: "https://proxy.example.com".into(), + device_pubkey: "device-public-key".into(), + token: Some("polling-token".into()), + schema_version: SERVICE_LOCATION_SCHEMA_VERSION, + }; + + let json = serde_json::to_string(&data).expect("serialization must succeed"); + let restored: ServiceLocationData = + serde_json::from_str(&json).expect("deserialization must succeed"); + + assert_eq!(restored.proxy_url, "https://proxy.example.com"); + assert_eq!(restored.device_pubkey, "device-public-key"); + assert_eq!(restored.token.as_deref(), Some("polling-token")); + assert_eq!(restored.schema_version, SERVICE_LOCATION_SCHEMA_VERSION); + assert_eq!(restored.service_locations[0].network_id, 42); + assert!(restored.service_locations[0].posture_check_required); + // The remote peer key must not be confused with the device key. + assert_eq!(restored.service_locations[0].pubkey, "remote-peer-pubkey"); + } + + #[test] + fn debug_masks_private_key_and_token() { + let data = ServiceLocationData { + service_locations: Vec::new(), + instance_id: "instance-uuid".into(), + private_key: "super-secret-private-key".into(), + proxy_url: "https://proxy.example.com".into(), + device_pubkey: "device-public-key".into(), + token: Some("super-secret-token".into()), + schema_version: SERVICE_LOCATION_SCHEMA_VERSION, + }; + + let debug = format!("{data:?}"); + assert!(!debug.contains("super-secret-private-key"), "{debug}"); + assert!(!debug.contains("super-secret-token"), "{debug}"); + // Non-secret fields are still visible for diagnostics. + assert!(debug.contains("https://proxy.example.com"), "{debug}"); + assert!(debug.contains("device-public-key"), "{debug}"); + } + + #[test] + fn debug_of_absent_token_is_not_masked_as_present() { + let data = ServiceLocationData { + service_locations: Vec::new(), + instance_id: "instance-uuid".into(), + private_key: "private".into(), + proxy_url: String::new(), + device_pubkey: String::new(), + token: None, + schema_version: SERVICE_LOCATION_SCHEMA_VERSION, + }; + + assert!(format!("{data:?}").contains("token: None")); + } + + #[test] + fn single_service_location_debug_masks_private_key() { + let data = SingleServiceLocationData { + service_location: ServiceLocation { + name: "Office".into(), + address: "10.0.0.2/24".into(), + pubkey: "remote-peer-pubkey".into(), + endpoint: "vpn.example.com:51820".into(), + allowed_ips: "10.0.0.0/24".into(), + keepalive_interval: 25, + dns: "10.0.0.1".into(), + mode: ProtoServiceLocationMode::AlwaysOn as i32, + network_id: 42, + posture_check_required: true, + }, + instance_id: "instance-uuid".into(), + private_key: "super-secret-private-key".into(), + }; + + assert!(!format!("{data:?}").contains("super-secret-private-key")); + } } diff --git a/src-tauri/enterprise/service-locations/src/linux.rs b/src-tauri/enterprise/service-locations/src/linux.rs index e697216f7..618d11f8f 100644 --- a/src-tauri/enterprise/service-locations/src/linux.rs +++ b/src-tauri/enterprise/service-locations/src/linux.rs @@ -5,16 +5,23 @@ use std::{ os::unix::fs::PermissionsExt, path::PathBuf, str::FromStr, + time::SystemTime, }; use defguard_client_common::{dns_borrow, find_free_tcp_port, get_interface_name}; -use defguard_client_proto::defguard::client::v1::{ServiceLocation, ServiceLocationMode}; +use defguard_client_proto::defguard::client::v1::{ + SaveServiceLocationsRequest, ServiceLocation, ServiceLocationMode, +}; use defguard_wireguard_rs::{ key::Key, net::IpAddrMask, peer::Peer, InterfaceConfiguration, WGApi, WireguardInterfaceApi, }; -use log::{debug, error, warn}; +use log::{debug, error, info, warn}; -use crate::{ServiceLocationData, ServiceLocationError, ServiceLocationManager}; +use crate::{ + is_unchanged_on_disk, posture_session_is_stale, reconcile_action, PostureAuthorizationRequest, + PostureAuthorizations, ReconcileAction, ServiceLocationData, ServiceLocationError, + ServiceLocationManager, +}; const DEFGUARD_DIR: &str = "/etc/defguard"; const SERVICE_LOCATIONS_SUBDIR: &str = "service_locations"; @@ -49,6 +56,15 @@ fn remove_created_interface(wgapi: &WGApi, ifname: &str) { } } +fn preshared_key_update(preshared_key: Option<&str>) -> Result { + // Netlink interprets an omitted attribute as "leave unchanged". WireGuard's explicit all-zero + // key removes the PSK from an existing peer. + Ok(match preshared_key { + Some(preshared_key) => Key::from_str(preshared_key)?, + None => Key::default(), + }) +} + impl ServiceLocationManager { pub fn init() -> Result { debug!("Initializing Linux service location storage"); @@ -58,15 +74,20 @@ impl ServiceLocationManager { /// Persists Linux-supported service locations and resets their runtime connection state. /// + /// **Idempotent.** Callers push on every poll cycle without doing their own change detection, so + /// this returns early when the data it would write matches what is already on disk, leaving the + /// running tunnels alone. Only a real change proceeds to the reset loop below. + /// /// Linux supports Always-on service locations only. Unsupported modes are filtered out before - /// storage, stale previously-saved locations are disconnected, and every saved Always-on location - /// is reset. All resets are attempted before returning an aggregate error. + /// storage - and before the comparison, so a PreLogon location does not read as a change on every + /// push. Stale previously-saved locations are disconnected, and every saved Always-on location is + /// reset. All resets are attempted before returning an aggregate error. pub fn save_service_locations( &mut self, - service_locations: &[ServiceLocation], - instance_id: &str, - private_key: &str, + request: &SaveServiceLocationsRequest, ) -> Result<(), ServiceLocationError> { + let instance_id = request.instance_id.as_str(); + let service_locations = request.service_locations.as_slice(); debug!( "Received a request to save {} service location(s) for instance {instance_id}", service_locations.len(), @@ -91,21 +112,36 @@ impl ServiceLocationManager { .map(|location| location.pubkey.clone()) .collect::>(); - let service_location_data = ServiceLocationData { - service_locations: service_locations.clone(), - instance_id: instance_id.to_string(), - private_key: private_key.to_string(), - }; + let service_location_data = + ServiceLocationData::from_save_request(request, service_locations.clone()); ensure_shared_directory()?; let instance_file_path = get_instance_file_path(instance_id); let json = serde_json::to_string_pretty(&service_location_data)?; + // Saving is pushed unconditionally on every poll cycle, so nothing having changed is the + // normal case. Return before the reset loop below, which disconnects and reconnects every + // tunnel: proceeding would drop working tunnels at the poll interval forever. Permissions + // are still reapplied, so a file whose mode drifted is repaired even on this path. + if is_unchanged_on_disk(&instance_file_path, &json) { + debug!( + "Service locations for instance {instance_id} are unchanged, leaving {} and the \ + existing tunnels untouched", + instance_file_path.display() + ); + set_permissions( + &instance_file_path, + fs::Permissions::from_mode(SERVICE_LOCATION_FILE_PERMS), + )?; + return Ok(()); + } + debug!( "Writing service location data to file: {}", instance_file_path.display() ); fs::write(&instance_file_path, json)?; + self.note_configuration_changed(); set_permissions( &instance_file_path, fs::Permissions::from_mode(SERVICE_LOCATION_FILE_PERMS), @@ -119,7 +155,8 @@ impl ServiceLocationManager { let mut reset_failed = false; for location in &service_locations { - if let Err(err) = self.reset_service_location_state(instance_id, location, private_key) + if let Err(err) = + self.reset_service_location_state(instance_id, location, &request.private_key) { warn!( "Failed to reset Linux service location '{}' after saving: {err}", @@ -138,7 +175,12 @@ impl ServiceLocationManager { Ok(()) } - /// Reconnects one Linux always-on service location. + /// Reconnects one Linux always-on service location after its configuration changed. + /// + /// A posture-gated location is only torn down here, not brought back: obtaining a preshared key + /// means an HTTP round trip, and this runs inside the gRPC save handler while the manager write + /// guard is held. The reconciler authorizes and reconnects it on its next pass instead, so the + /// location is down for at most one interval. fn reset_service_location_state( &mut self, instance_id: &str, @@ -151,7 +193,17 @@ impl ServiceLocationManager { ); self.disconnect_service_location(instance_id, &location.pubkey)?; - self.connect_service_location(instance_id, location, private_key)?; + + if location.posture_check_required { + debug!( + "Leaving Linux service location '{}' disconnected: it needs a posture check, which \ + the reconciler will run", + location.name + ); + return Ok(()); + } + + self.connect_service_location(instance_id, location, private_key, None)?; debug!( "Linux service location '{}' state reset successfully", @@ -301,10 +353,15 @@ impl ServiceLocationManager { &mut self, location: &ServiceLocation, private_key: &str, + preshared_key: Option<&str>, ) -> Result<(), ServiceLocationError> { let peer_key = Key::from_str(&location.pubkey)?; let mut peer = Peer::new(peer_key); peer.set_endpoint(&location.endpoint)?; + // Held only by the running interface. It is never written to the service location file, + // which already holds two long-lived secrets, and a session key is reconstructible by + // authorizing again. + peer.preshared_key = preshared_key.map(Key::from_str).transpose()?; peer.persistent_keepalive_interval = location.keepalive_interval.try_into().ok(); for allowed_ip in location.allowed_ips.split(',').map(str::trim) { @@ -375,6 +432,7 @@ impl ServiceLocationManager { instance_id: &str, location: &ServiceLocation, private_key: &str, + preshared_key: Option<&str>, ) -> Result<(), ServiceLocationError> { if self.is_service_location_connected(instance_id, &location.pubkey) { debug!( @@ -396,7 +454,7 @@ impl ServiceLocationManager { return Ok(()); } - self.setup_service_location_interface(location, private_key)?; + self.setup_service_location_interface(location, private_key, preshared_key)?; self.add_connected_service_location(instance_id, location); debug!("Connected Linux service location '{}'", location.name); Ok(()) @@ -406,7 +464,165 @@ impl ServiceLocationManager { /// /// Returns `Ok(true)` when every supported location is connected or already connected, and /// `Ok(false)` when at least one supported location failed so the caller can retry later. - pub fn connect_to_service_locations(&mut self) -> Result { + /// Whether a connected location's posture session has stopped showing signs of life. + /// + /// Reads the handshake from the interface itself, because the daemon's own record of having + /// connected proves nothing: a suspend outlasts core's `peer_disconnect_threshold`, the gateway + /// drops the peer, and the interface carries on looking healthy while passing nothing. + fn posture_session_needs_renewal(&self, instance_id: &str, location_pubkey: &str) -> bool { + let authorized_at = self + .posture_sessions + .get(&(instance_id.to_string(), location_pubkey.to_string())) + .copied(); + let last_handshake = self.read_last_handshake(location_pubkey); + + let stale = posture_session_is_stale(last_handshake, authorized_at, SystemTime::now()); + if stale { + debug!( + "Posture session for peer {location_pubkey} looks stale (last handshake: \ + {last_handshake:?}), it will be renewed" + ); + } + stale + } + + /// Reads the last handshake for a peer from the interface carrying it. + /// + /// `None` means either no interface was found or the peer has never completed a handshake. The + /// staleness rule treats both the same, falling back to when the session was authorized. + fn read_last_handshake(&self, location_pubkey: &str) -> Option { + let ifname = self.find_interface_by_peer_pubkey(location_pubkey)?; + let wgapi = self.wgapis.get(&ifname)?; + let host = wgapi + .read_interface_data() + .inspect_err(|err| { + warn!("Failed to read data for service location interface {ifname}: {err}"); + }) + .ok()?; + let peer_key = Key::from_str(location_pubkey).ok()?; + host.peers.get(&peer_key)?.last_handshake + } + + /// Applies a freshly obtained preshared key to an already-running interface. + /// + /// Uses `configure_peer` rather than rebuilding the interface: on Linux that is a single netlink + /// call carrying the new key, so the tunnel keeps its listen port and the gap in traffic is as + /// short as it can be. + fn reapply_preshared_key( + &mut self, + instance_id: &str, + location: &ServiceLocation, + preshared_key: Option<&str>, + ) -> Result<(), ServiceLocationError> { + let Some(ifname) = self.find_interface_by_peer_pubkey(&location.pubkey) else { + return Err(ServiceLocationError::InterfaceError(format!( + "No interface found for service location '{}' while renewing its posture session", + location.name + ))); + }; + let Some(wgapi) = self.wgapis.get(&ifname) else { + return Err(ServiceLocationError::InterfaceError(format!( + "No WireGuard API for interface {ifname} while renewing a posture session" + ))); + }; + + let mut peer = Peer::new(Key::from_str(&location.pubkey)?); + peer.set_endpoint(&location.endpoint)?; + peer.persistent_keepalive_interval = location.keepalive_interval.try_into().ok(); + peer.preshared_key = Some(preshared_key_update(preshared_key)?); + for allowed_ip in location.allowed_ips.split(',').map(str::trim) { + if allowed_ip.is_empty() { + continue; + } + match IpAddrMask::from_str(allowed_ip) { + Ok(addr) => peer.allowed_ips.push(addr), + Err(err) => error!( + "Error parsing allowed IP {allowed_ip} while renewing service location {}: \ + {err}", + location.name + ), + } + } + + wgapi.configure_peer(&peer)?; + self.record_posture_session(instance_id, &location.pubkey); + info!( + "Renewed the posture session for Linux service location '{}'", + location.name + ); + Ok(()) + } + + /// Notes that a location's posture session was just approved. + fn record_posture_session(&mut self, instance_id: &str, location_pubkey: &str) { + self.posture_sessions.insert( + (instance_id.to_string(), location_pubkey.to_string()), + SystemTime::now(), + ); + } + + /// Brings the running tunnels in line with what is on disk. + /// + /// On Linux that is only ever "connect what is missing": the save path filters out everything but + /// Always-on locations, so nothing persisted here should ever be deliberately down. + pub fn reconcile( + &mut self, + authorizations: &PostureAuthorizations, + ) -> Result { + self.prune_posture_sessions(); + + self.connect_to_service_locations(authorizations) + } + + /// Lists the locations that need a posture check before the next pass can connect them. + /// + /// Read-only, so the caller can hold a read guard briefly, release it, and do the network calls + /// unlocked. Locations that are already connected are excluded: re-authorizing a working tunnel + /// would supersede its session in core for no reason. + pub fn locations_needing_authorization(&self) -> Vec { + let Ok(data) = self.load_service_locations() else { + warn!("Failed to load service locations while looking for posture checks to run"); + return Vec::new(); + }; + + let mut pending = Vec::new(); + for instance_data in data { + for location in instance_data.service_locations { + if !location.posture_check_required + || location.mode != ServiceLocationMode::AlwaysOn as i32 + { + continue; + } + + // A connected location is left alone unless its session has gone stale. Renewing a + // healthy one would supersede it in core for no reason. + if self.is_service_location_connected(&instance_data.instance_id, &location.pubkey) + && !self + .posture_session_needs_renewal(&instance_data.instance_id, &location.pubkey) + { + continue; + } + + pending.push(PostureAuthorizationRequest { + instance_id: instance_data.instance_id.clone(), + location_pubkey: location.pubkey.clone(), + location_name: location.name.clone(), + network_id: location.network_id, + proxy_url: instance_data.proxy_url.clone(), + device_pubkey: instance_data.device_pubkey.clone(), + token: instance_data.token.clone(), + configuration_generation: self.configuration_generation, + }); + } + } + + pending + } + + pub fn connect_to_service_locations( + &mut self, + authorizations: &PostureAuthorizations, + ) -> Result { debug!("Attempting to auto-connect Linux Always-on service locations"); let data = self.load_service_locations()?; @@ -422,25 +638,68 @@ impl ServiceLocationManager { continue; } - if self.is_service_location_connected(&instance_data.instance_id, &location.pubkey) - { - debug!( - "Skipping Linux service location '{}' because it's already connected", - location.name - ); - continue; - } + let authorization = authorizations + .get(&(instance_data.instance_id.clone(), location.pubkey.clone())); + let action = reconcile_action( + self.is_service_location_connected( + &instance_data.instance_id, + &location.pubkey, + ), + location.posture_check_required, + authorization, + self.configuration_generation, + ); - if let Err(err) = self.connect_service_location( - &instance_data.instance_id, - &location, - &instance_data.private_key, - ) { - warn!( - "Failed to setup Linux service location interface for '{}': {err:?}", - location.name - ); - all_connected = false; + match action { + ReconcileAction::LeaveConnected => { + debug!( + "Skipping Linux service location '{}' because it's already connected", + location.name + ); + continue; + } + ReconcileAction::WaitForAuthorization => { + debug!( + "Leaving Linux service location '{}' disconnected: no posture check has \ + approved it yet", + location.name + ); + all_connected = false; + continue; + } + ReconcileAction::Renew(preshared_key) => { + if let Err(err) = self.reapply_preshared_key( + &instance_data.instance_id, + &location, + preshared_key, + ) { + warn!( + "Failed to renew the posture session for '{}': {err}", + location.name + ); + all_connected = false; + } + continue; + } + ReconcileAction::Connect(preshared_key) => { + if let Err(err) = self.connect_service_location( + &instance_data.instance_id, + &location, + &instance_data.private_key, + preshared_key, + ) { + warn!( + "Failed to setup Linux service location interface for '{}': {err:?}", + location.name + ); + all_connected = false; + } else if authorization.is_some() { + self.record_posture_session( + &instance_data.instance_id, + &location.pubkey, + ); + } + } } } } @@ -509,3 +768,13 @@ impl ServiceLocationManager { Ok(Some(serde_json::from_str::(&data)?)) } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn removing_a_preshared_key_emits_an_explicit_zero_key() { + assert_eq!(preshared_key_update(None).unwrap(), Key::default()); + } +} diff --git a/src-tauri/enterprise/service-locations/src/windows.rs b/src-tauri/enterprise/service-locations/src/windows.rs index 3133ef4d5..bea4cb684 100644 --- a/src-tauri/enterprise/service-locations/src/windows.rs +++ b/src-tauri/enterprise/service-locations/src/windows.rs @@ -5,18 +5,19 @@ use std::{ path::PathBuf, result::Result, str::FromStr, - sync::{Arc, RwLock}, thread::sleep, - time::Duration, + time::{Duration, SystemTime}, }; use defguard_client_common::{dns_borrow, find_free_tcp_port, get_interface_name}; -use defguard_client_proto::defguard::client::v1::{ServiceLocation, ServiceLocationMode}; +use defguard_client_proto::defguard::client::v1::{ + SaveServiceLocationsRequest, ServiceLocation, ServiceLocationMode, +}; use defguard_wireguard_rs::{ key::Key, net::IpAddrMask, peer::Peer, InterfaceConfiguration, WGApi, WireguardInterfaceApi, }; use known_folders::get_known_folder_path; -use log::{debug, error, warn}; +use log::{debug, error, info, warn}; use windows::{ core::PSTR, Win32::System::RemoteDesktop::{ @@ -28,7 +29,9 @@ use windows_acl::acl::ACL; use windows_sys::Win32::NetworkManagement::IpHelper::NotifyAddrChange; use crate::{ - ServiceLocationData, ServiceLocationError, ServiceLocationManager, SingleServiceLocationData, + is_unchanged_on_disk, posture_session_is_stale, reconcile_action, PostureAuthorizationRequest, + PostureAuthorizations, ReconcileAction, ReconcileSignal, ServiceLocationData, + ServiceLocationError, ServiceLocationManager, SingleServiceLocationData, }; const LOGIN_LOGOFF_EVENT_RETRY_DELAY_SECS: u64 = 5; @@ -47,11 +50,12 @@ const SERVICE_LOCATIONS_SUBDIR: &str = "service_locations"; /// yet available. When the network comes up and an IP is assigned, this watcher fires and /// retries the connection. /// -/// Note: `NotifyAddrChange` also fires when WireGuard interfaces are created. This is -/// harmless because `connect_to_service_locations` skips already-connected locations. +/// Note: `NotifyAddrChange` also fires when WireGuard interfaces are created. This is harmless +/// because a reconcile pass leaves already-correct locations alone. /// -/// Runs on a dedicated OS thread because `NotifyAddrChange` is a blocking syscall. -pub fn watch_for_network_change(service_location_manager: Arc>) { +/// Runs on a dedicated OS thread because `NotifyAddrChange` is a blocking syscall. It only wakes the +/// reconciler and never touches the manager itself, so tunnel state has a single owner. +pub fn watch_for_network_change(wake: ReconcileSignal) { loop { // NotifyAddrChange blocks until any IP address is added or removed on any interface. // Passing NULL for both handle and overlapped selects the synchronous (blocking) mode. @@ -69,29 +73,21 @@ pub fn watch_for_network_change(service_location_manager: Arc { - debug!("Service location connect attempt after network change completed"); - } - Err(err) => { - warn!("Failed to connect to service locations after network change: {err}"); - } - } + debug!("Waking the service location reconciler after a network change"); + wake.notify_one(); } } -/// Watches for user logon/logoff events and connects/disconnects pre-logon service locations -/// accordingly. +/// Watches for user logon and logoff events and wakes the reconciler. /// -/// Runs on a dedicated OS thread because `WTSWaitSystemEvent` is a blocking syscall. -pub fn watch_for_login_logoff( - service_location_manager: Arc>, -) -> Result<(), ServiceLocationError> { +/// Which event occurred is deliberately not passed on: the reconciler establishes whether a user is +/// logged in for itself, so a logon and a logoff are both simply "look again". That is what lets this +/// thread stay out of the manager entirely. +/// +/// Runs on a dedicated OS thread because `WTSWaitSystemEvent` is a blocking syscall. It never +/// returns: a failed wait is retried after a delay rather than reported, since there is nothing a +/// caller could usefully do about it. +pub fn watch_for_login_logoff(wake: &ReconcileSignal) -> ! { loop { let mut event_flags: u32 = 0; let success = unsafe { @@ -113,19 +109,9 @@ pub fn watch_for_login_logoff( } }; - if event_flags & WTS_EVENT_LOGON != 0 { - debug!("Detected user logon, attempting to auto-disconnect from service locations."); - service_location_manager - .write() - .unwrap() - .disconnect_service_locations(Some(ServiceLocationMode::PreLogon))?; - } - if event_flags & WTS_EVENT_LOGOFF != 0 { - debug!("Detected user logoff, attempting to auto-connect to service locations."); - service_location_manager - .write() - .unwrap() - .connect_to_service_locations()?; + if event_flags & (WTS_EVENT_LOGON | WTS_EVENT_LOGOFF) != 0 { + debug!("Detected a logon or logoff, waking the service location reconciler"); + wake.notify_one(); } } } @@ -138,6 +124,48 @@ fn setup_wgapi(ifname: &str) -> Result { }) } +fn interface_configuration( + location: &ServiceLocation, + private_key: &str, + preshared_key: Option<&str>, + port: u16, +) -> Result { + let mut peer = Peer::new(Key::from_str(&location.pubkey)?); + peer.set_endpoint(&location.endpoint)?; + // Held only by the running interface. It is never written to the service location file, + // which already holds two long-lived secrets, and a session key is reconstructible by + // authorizing again. + peer.preshared_key = preshared_key.map(Key::from_str).transpose()?; + peer.persistent_keepalive_interval = location.keepalive_interval.try_into().ok(); + + for allowed_ip in location.allowed_ips.split(',') { + match IpAddrMask::from_str(allowed_ip) { + Ok(addr) => peer.allowed_ips.push(addr), + Err(err) => error!( + "Error parsing IP address {allowed_ip} while setting up interface for location \ + {location:?}, error details: {err}" + ), + } + } + + let addresses = location + .address + .split(',') + .map(str::trim) + .map(IpAddrMask::from_str) + .collect::, _>>()?; + + Ok(InterfaceConfiguration { + name: location.name.clone(), + prvkey: private_key.to_string(), + addresses, + port, + peers: vec![peer], + mtu: None, + fwmark: None, // TODO: add + }) +} + fn get_shared_directory() -> Result { match get_known_folder_path(known_folders::KnownFolder::ProgramData) { Some(mut path) => { @@ -333,10 +361,7 @@ impl ServiceLocationManager { warn!("Failed to convert path to string for ACL setting"); } - let manager = Self { - wgapis: HashMap::new(), - connected_service_locations: HashMap::new(), - }; + let manager = Self::default(); debug!("ServiceLocationApi initialized successfully"); Ok(manager) @@ -443,6 +468,22 @@ impl ServiceLocationManager { service_location_data.service_location.name ); + // A posture-gated location is only torn down here, not brought back: obtaining a preshared + // key means an HTTP round trip, and this runs inside the gRPC save handler while the manager + // write guard is held. The reconciler authorizes and reconnects it on its next pass instead, + // so the location is down for at most one interval. + if service_location_data + .service_location + .posture_check_required + { + debug!( + "Leaving service location '{}' disconnected: it needs a posture check, which the \ + reconciler will run", + service_location_data.service_location.name + ); + return Ok(()); + } + // We should reconnect only if: // 1. It's an always on location // 2. It's a pre-logon location and the user is not logged in @@ -565,49 +606,14 @@ impl ServiceLocationManager { &mut self, location: &ServiceLocation, private_key: &str, + preshared_key: Option<&str>, ) -> Result<(), ServiceLocationError> { - let peer_key = Key::from_str(&location.pubkey)?; - - let mut peer = Peer::new(peer_key.clone()); - peer.set_endpoint(&location.endpoint)?; - - peer.persistent_keepalive_interval = location.keepalive_interval.try_into().ok(); - - let allowed_ips = location - .allowed_ips - .split(',') - .map(str::to_string) - .collect::>(); - - for allowed_ip in &allowed_ips { - match IpAddrMask::from_str(allowed_ip) { - Ok(addr) => { - peer.allowed_ips.push(addr); - } - Err(err) => { - error!( - "Error parsing IP address {allowed_ip} while setting up interface for \ - location {location:?}, error details: {err}" - ); - } - } - } - - let mut addresses = Vec::new(); - - for address in location.address.split(',') { - addresses.push(IpAddrMask::from_str(address.trim())?); - } - - let config = InterfaceConfiguration { - name: location.name.clone(), - prvkey: private_key.to_string(), - addresses, - port: find_free_tcp_port().unwrap_or(DEFAULT_WIREGUARD_PORT), - peers: vec![peer.clone()], - mtu: None, - fwmark: None, // TODO: add - }; + let config = interface_configuration( + location, + private_key, + preshared_key, + find_free_tcp_port().unwrap_or(DEFAULT_WIREGUARD_PORT), + )?; let ifname = location.name.clone(); let ifname = get_interface_name(&ifname); @@ -672,6 +678,7 @@ impl ServiceLocationManager { self.setup_service_location_interface( &location_data.service_location, &location_data.private_key, + None, )?; self.add_connected_service_location( &location_data.instance_id, @@ -683,9 +690,14 @@ impl ServiceLocationManager { Ok(()) } + /// Disconnects every connected service location in `mode`. + /// + /// Takes the mode directly rather than an `Option` meaning "all modes": the only caller is the + /// reconcile pass tearing down pre-logon locations once a user logs in, and an all-modes teardown + /// has never been asked for. `disconnect_service_locations_by_instance` covers the other case. pub(crate) fn disconnect_service_locations( &mut self, - mode: Option, + mode: ServiceLocationMode, ) -> Result<(), ServiceLocationError> { debug!("Disconnecting service locations with mode: {mode:?}"); @@ -696,16 +708,14 @@ impl ServiceLocationManager { location_pubkey: {}", location.pubkey ); - if let Some(m) = mode { - let location_mode: ServiceLocationMode = location.mode.try_into()?; - if location_mode != m { - debug!( + let location_mode: ServiceLocationMode = location.mode.try_into()?; + if location_mode != mode { + debug!( "Skipping interface {} due to the service location mode doesn't match the \ - requested mode (expected {m:?}, found {:?})", + requested mode (expected {mode:?}, found {:?})", location.name, location.mode ); - continue; - } + continue; } let ifname = get_interface_name(&location.name); @@ -723,15 +733,12 @@ impl ServiceLocationManager { } self.remove_connected_service_locations(|_, location| { - if let Some(m) = mode { - let location_mode: ServiceLocationMode = location - .mode - .try_into() - .unwrap_or(ServiceLocationMode::AlwaysOn); - location_mode == m - } else { - true - } + // An unparseable mode is left in place rather than removed: dropping the record of a + // tunnel that is still up would leak it. + location + .mode + .try_into() + .is_ok_and(|location_mode: ServiceLocationMode| location_mode == mode) })?; debug!("Service locations disconnected."); @@ -744,7 +751,155 @@ impl ServiceLocationManager { /// Returns `Ok(true)` if every location is now connected (either it was already connected or /// it was successfully connected during this call), and `Ok(false)` if at least one location /// failed to connect (indicating that a retry may be worthwhile). - pub fn connect_to_service_locations(&mut self) -> Result { + /// Whether a connected location's posture session has stopped showing signs of life. + /// + /// Reads the handshake from the interface itself, because the daemon's own record of having + /// connected proves nothing: a suspend outlasts core's `peer_disconnect_threshold`, the gateway + /// drops the peer, and the interface carries on looking healthy while passing nothing. + fn posture_session_needs_renewal(&self, instance_id: &str, location: &ServiceLocation) -> bool { + let authorized_at = self + .posture_sessions + .get(&(instance_id.to_string(), location.pubkey.clone())) + .copied(); + let last_handshake = self.read_last_handshake(location); + + let stale = posture_session_is_stale(last_handshake, authorized_at, SystemTime::now()); + if stale { + debug!( + "Posture session for service location '{}' looks stale (last handshake: \ + {last_handshake:?}), it will be renewed", + location.name + ); + } + stale + } + + /// Reads the last handshake for a location from the interface carrying it. + /// + /// Goes through the stored `WGApi` deliberately: on Windows `read_interface_data` needs the very + /// instance that created the adapter, so a freshly built one would fail with `AdapterNotFound`. + fn read_last_handshake(&self, location: &ServiceLocation) -> Option { + let ifname = get_interface_name(&location.name); + let wgapi = self.wgapis.get(&ifname)?; + let host = wgapi + .read_interface_data() + .inspect_err(|err| { + warn!("Failed to read data for service location interface {ifname}: {err}"); + }) + .ok()?; + let peer_key = Key::from_str(&location.pubkey).ok()?; + host.peers.get(&peer_key)?.last_handshake + } + + /// Applies a freshly obtained preshared key to an already-running interface. + /// + /// Reconfigures the whole interface rather than the single peer, because `configure_peer` does + /// nothing on Windows. The tracked API owns the existing adapter, so renewal configures that + /// adapter directly without opening/creating an interface or replacing the API handle. + fn reapply_preshared_key( + &mut self, + instance_id: &str, + location: &ServiceLocation, + private_key: &str, + preshared_key: Option<&str>, + ) -> Result<(), ServiceLocationError> { + let ifname = get_interface_name(&location.name); + let Some(wgapi) = self.wgapis.get(&ifname) else { + return Err(ServiceLocationError::InterfaceError(format!( + "No WireGuard API for interface {ifname} while renewing a posture session" + ))); + }; + let port = wgapi.read_interface_data()?.listen_port; + let config = interface_configuration(location, private_key, preshared_key, port)?; + wgapi.configure_interface(&config)?; + self.record_posture_session(instance_id, &location.pubkey); + info!( + "Renewed the posture session for service location '{}'", + location.name + ); + Ok(()) + } + + /// Notes that a location's posture session was just approved. + fn record_posture_session(&mut self, instance_id: &str, location_pubkey: &str) { + self.posture_sessions.insert( + (instance_id.to_string(), location_pubkey.to_string()), + SystemTime::now(), + ); + } + + /// Brings the running tunnels in line with what is on disk and who is logged in. + /// + /// Both directions, unlike `connect_to_service_locations` alone. Tearing down a pre-logon location + /// once a user logs in used to happen only in the logon event handler, which meant it depended on + /// having observed the event. Deriving it from `is_user_logged_in()` instead makes the pass + /// correct on its own, so the watchers can be reduced to "something happened, look again" and a + /// missed event costs a tick rather than leaving a tunnel up that should be down. + pub fn reconcile( + &mut self, + authorizations: &PostureAuthorizations, + ) -> Result { + self.prune_posture_sessions(); + + if is_user_logged_in() { + debug!("A user is logged in, disconnecting any connected pre-logon service locations"); + self.disconnect_service_locations(ServiceLocationMode::PreLogon)?; + } + + self.connect_to_service_locations(authorizations) + } + + /// Lists the locations that need a posture check before the next pass can connect them. + /// + /// Read-only, so the caller can hold a read guard briefly, release it, and do the network calls + /// unlocked. Excludes anything already connected, and anything that should not be up right now - + /// re-authorizing a working tunnel would supersede its session in core for no reason, and + /// authorizing a pre-logon location while a user is logged in would be wasted work. + pub fn locations_needing_authorization(&self) -> Vec { + let Ok(data) = self.load_service_locations() else { + warn!("Failed to load service locations while looking for posture checks to run"); + return Vec::new(); + }; + + let user_logged_in = is_user_logged_in(); + let mut pending = Vec::new(); + + for instance_data in data { + for location in instance_data.service_locations { + if !location.posture_check_required + || (location.mode == ServiceLocationMode::PreLogon as i32 && user_logged_in) + { + continue; + } + + // A connected location is left alone unless its session has gone stale. Renewing a + // healthy one would supersede it in core for no reason. + if self.is_service_location_connected(&instance_data.instance_id, &location.pubkey) + && !self.posture_session_needs_renewal(&instance_data.instance_id, &location) + { + continue; + } + + pending.push(PostureAuthorizationRequest { + instance_id: instance_data.instance_id.clone(), + location_pubkey: location.pubkey.clone(), + location_name: location.name.clone(), + network_id: location.network_id, + proxy_url: instance_data.proxy_url.clone(), + device_pubkey: instance_data.device_pubkey.clone(), + token: instance_data.token.clone(), + configuration_generation: self.configuration_generation, + }); + } + } + + pending + } + + pub fn connect_to_service_locations( + &mut self, + authorizations: &PostureAuthorizations, + ) -> Result { debug!("Attempting to auto-connect to VPN..."); let data = self.load_service_locations()?; @@ -779,38 +934,86 @@ impl ServiceLocationManager { ); } - if self.is_service_location_connected(&instance_data.instance_id, &location.pubkey) - { - debug!( - "Skipping service location '{}' because it's already connected", - location.name - ); - continue; - } + let authorization = authorizations + .get(&(instance_data.instance_id.clone(), location.pubkey.clone())); + let action = reconcile_action( + self.is_service_location_connected( + &instance_data.instance_id, + &location.pubkey, + ), + location.posture_check_required, + authorization, + self.configuration_generation, + ); - if let Err(err) = - self.setup_service_location_interface(&location, &instance_data.private_key) - { - warn!( - "Failed to setup service location interface for '{}': {err:?}", - location.name - ); - all_connected = false; - continue; - } + match action { + ReconcileAction::LeaveConnected => { + debug!( + "Skipping service location '{}' because it's already connected", + location.name + ); + continue; + } + ReconcileAction::WaitForAuthorization => { + debug!( + "Leaving service location '{}' disconnected: no posture check has \ + approved it yet", + location.name + ); + all_connected = false; + continue; + } + ReconcileAction::Renew(preshared_key) => { + if let Err(err) = self.reapply_preshared_key( + &instance_data.instance_id, + &location, + &instance_data.private_key, + preshared_key, + ) { + warn!( + "Failed to renew the posture session for '{}': {err}", + location.name + ); + all_connected = false; + } + continue; + } + ReconcileAction::Connect(preshared_key) => { + if let Err(err) = self.setup_service_location_interface( + &location, + &instance_data.private_key, + preshared_key, + ) { + warn!( + "Failed to setup service location interface for '{}': {err:?}", + location.name + ); + all_connected = false; + continue; + } - if let Err(err) = - self.add_connected_service_location(&instance_data.instance_id, &location) - { - debug!( - "Failed to persist connected service location after auto-connect: {err:?}" - ); - } + if let Err(err) = self + .add_connected_service_location(&instance_data.instance_id, &location) + { + debug!( + "Failed to persist connected service location after auto-connect: \ + {err:?}" + ); + } - debug!( - "Successfully connected to service location '{}'", - location.name - ); + if authorization.is_some() { + self.record_posture_session( + &instance_data.instance_id, + &location.pubkey, + ); + } + + debug!( + "Successfully connected to service location '{}'", + location.name + ); + } + } } } @@ -819,12 +1022,17 @@ impl ServiceLocationManager { Ok(all_connected) } + /// Persists service locations and resets their runtime connection state. + /// + /// **Idempotent.** Callers push on every poll cycle without doing their own change detection, so + /// this returns early when the data it would write matches what is already on disk, leaving the + /// running tunnels alone. Only a real change proceeds to the reset loop below. pub fn save_service_locations( &mut self, - service_locations: &[ServiceLocation], - instance_id: &str, - private_key: &str, + request: &SaveServiceLocationsRequest, ) -> Result<(), ServiceLocationError> { + let instance_id = request.instance_id.as_str(); + let service_locations = request.service_locations.as_slice(); debug!( "Received a request to save {} service location(s) for instance {instance_id}", service_locations.len(), @@ -847,20 +1055,39 @@ impl ServiceLocationManager { let instance_file_path = get_instance_file_path(instance_id)?; - let service_location_data = ServiceLocationData { - service_locations: service_locations.to_vec(), - instance_id: instance_id.to_string(), - private_key: private_key.to_string(), - }; + let service_location_data = + ServiceLocationData::from_save_request(request, service_locations.to_vec()); let json = serde_json::to_string_pretty(&service_location_data)?; + // Saving is pushed unconditionally on every poll cycle, so nothing having changed is the + // normal case. Return before the reset loop below, which disconnects and reconnects every + // tunnel: proceeding would drop working tunnels at the poll interval forever. ACLs are + // still reapplied, so a file whose ACLs drifted is repaired even on this path. + if is_unchanged_on_disk(&instance_file_path, &json) { + debug!( + "Service locations for instance {instance_id} are unchanged, leaving {} and the \ + existing tunnels untouched", + instance_file_path.display() + ); + if let Some(file_path_str) = instance_file_path.to_str() { + if let Err(err) = set_protected_acls(file_path_str) { + warn!( + "Failed to reapply ACLs on unchanged service location file \ + {file_path_str}: {err}" + ); + } + } + return Ok(()); + } + debug!( "Writing service location data to file: {}", instance_file_path.display() ); fs::write(&instance_file_path, &json)?; + self.note_configuration_changed(); if let Some(file_path_str) = instance_file_path.to_str() { debug!("Setting ACLs on service location file: {file_path_str}"); diff --git a/src-tauri/proto b/src-tauri/proto index 569334098..6867db7bc 160000 --- a/src-tauri/proto +++ b/src-tauri/proto @@ -1 +1 @@ -Subproject commit 569334098f1cd7e81809c3ccd7681acfc18a7491 +Subproject commit 6867db7bc454023df5fb0d7d15d3482aed5e936d diff --git a/src-tauri/src/commands.rs b/src-tauri/src/commands.rs index f6bd7141d..f6812b11a 100644 --- a/src-tauri/src/commands.rs +++ b/src-tauri/src/commands.rs @@ -15,7 +15,7 @@ use defguard_client_core::{ use defguard_client_posture::authorize_posture_session; #[cfg(not(target_os = "macos"))] use defguard_client_proto::defguard::client::v1::{ - DeleteServiceLocationsRequest, RemoveInterfaceRequest, SaveServiceLocationsRequest, + DeleteServiceLocationsRequest, RemoveInterfaceRequest, }; use defguard_client_proto::defguard::{ client_types::{ @@ -26,8 +26,6 @@ use defguard_client_proto::defguard::{ enterprise::posture::v2::DevicePostureData, }; use defguard_client_provisioning::ProvisioningConfig; -#[cfg(not(target_os = "macos"))] -use defguard_client_service_locations::to_service_location; use reqwest::Url; use serde::{Deserialize, Serialize}; use struct_patch::Patch; @@ -61,7 +59,7 @@ use crate::{ global_log_watcher::{spawn_global_log_watcher_task, stop_global_log_watcher_task}, service_log_watcher::stop_log_watcher_task, }, - periodic::config::{do_update_instance, poll_instance_with_events}, + periodic::config::{do_update_instance, poll_instance_with_events, sync_service_locations}, proxy::construct_platform_header, tauri_err_to_app_err, tray::{configure_tray_icon, reload_tray_menu}, @@ -152,11 +150,29 @@ pub async fn connect( "Identified location with ID {location_id} as \"{}\", handling connection.", location.name ); + // A service location is the daemon's to manage: it brings the tunnel up with no user + // session at all. Connecting from here would put two managers on one tunnel, and since + // core keeps a single session per (device, location), each side's authorization + // supersedes the other's preshared key - so the app and the daemon would take turns + // breaking each other's tunnel indefinitely. The same reasoning is why service locations + // are already skipped by `disconnect_locations` and hidden from `all_active_connections` + // and `all_locations`; without this check they were still reachable by id. + if location.is_service_location() { + error!( + "Refusing to connect location {location} from the app: it is a service \ + location, managed by the background service" + ); + return Err(Error::InvalidInput(format!( + "Location \"{}\" is a service location and is managed by the defguard service", + location.name + )) + .into()); + } // Connect-time MFA brings the tunnel up itself (keeping the preshared // key backend-side), so the only preshared key resolved here is for // posture-only locations. let preshared_key = if location.posture_check_required { - Some(authorize_posture_session(&location).await?) + authorize_posture_session(&location).await? } else { None }; @@ -398,6 +414,17 @@ async fn maybe_update_instance_config(location_id: Id, handle: &AppHandle) -> Re }; poll_instance_with_events(&mut transaction, &mut instance, handle).await?; transaction.commit().await?; + + // `do_update_instance` no longer pushes to the daemon itself, so every path that applies a + // fetched config has to do it here, after the commit. + if let Err(err) = sync_service_locations(&DB_POOL, &instance).await { + error!( + "Failed to push service locations to the daemon for instance {instance} after polling \ + its config: {err}. The daemon keeps its previous service-location state until the next \ + successful sync." + ); + } + handle .emit(EventKey::InstanceUpdate.into(), ()) .map_err(tauri_err_to_app_err)?; @@ -474,7 +501,7 @@ pub async fn save_device_config( disconnect_all_tunnels(&handle).await?; } - let locations = push_service_locations(&instance, keys).await?; + let locations = push_service_locations(&instance).await?; handle .emit(EventKey::InstanceUpdate.into(), ()) @@ -489,63 +516,24 @@ pub async fn save_device_config( } #[cfg(target_os = "macos")] -async fn push_service_locations( - _instance: &Instance, - _keys: WireguardKeys, -) -> Result>, Error> { +async fn push_service_locations(_instance: &Instance) -> Result>, Error> { // Nothing here... yet Ok(Vec::new()) } +/// Pushes the instance's service locations to the daemon and returns all of its locations. +/// +/// Delegates to [`sync_service_locations`] rather than building its own request, so the pushed +/// field set cannot drift from the config-sync path. Note this means an instance with no service +/// locations now asks the daemon to clear its state, which the previous inline version skipped - +/// correct on re-enrollment, where stale daemon state would otherwise survive. #[cfg(not(target_os = "macos"))] -async fn push_service_locations( - instance: &Instance, - keys: WireguardKeys, -) -> Result>, Error> { +async fn push_service_locations(instance: &Instance) -> Result>, Error> { let locations = Location::find_by_instance_id(&*DB_POOL, instance.id, true).await?; trace!("Created following locations: {locations:#?}"); - let mut service_locations = Vec::new(); - - for saved_location in &locations { - if saved_location.is_service_location() { - debug!( - "Adding service location {}({}) for instance {}({}) to be saved to the daemon.", - saved_location.name, saved_location.id, instance.name, instance.id, - ); - service_locations.push(to_service_location(saved_location)?); - } - } - - if !service_locations.is_empty() { - let save_request = SaveServiceLocationsRequest { - service_locations: service_locations.clone(), - instance_id: instance.uuid.clone(), - private_key: keys.prvkey, - }; - debug!( - "Saving {} service locations to the daemon for instance {}({}).", - save_request.service_locations.len(), - instance.name, - instance.id, - ); - DAEMON_CLIENT - .clone() - .save_service_locations(save_request) - .await - .map_err(|err| { - error!( - "Error while saving service locations to the daemon for instance {}({}): {err}", - instance.name, instance.id, - ); - Error::InternalError(err.to_string()) - })?; - debug!( - "Saved service locations to the daemon for instance {}({}).", - instance.name, instance.id, - ); - } + sync_service_locations(&DB_POOL, instance).await?; Ok(locations) } @@ -740,6 +728,16 @@ pub async fn update_instance( do_update_instance(&mut transaction, &mut instance, response).await?; transaction.commit().await?; + // After the commit, and unconditionally: `locations_changed` is blind to a `proxy_url` or + // polling-token change, and this is the path a re-enrollment takes (D23). + if let Err(err) = sync_service_locations(&DB_POOL, &instance).await { + error!( + "Failed to push service locations to the daemon for instance {instance} after \ + update: {err}. The daemon keeps its previous service-location state until the next \ + successful sync." + ); + } + if locations_changed { if let Err(err) = app_handle.emit(EventKey::InstanceUpdated.into(), ()) { error!("Failed to emit instance-updated event: {err}"); diff --git a/src-tauri/src/periodic/config.rs b/src-tauri/src/periodic/config.rs index ad7b6ad5f..4c4a3ae7b 100644 --- a/src-tauri/src/periodic/config.rs +++ b/src-tauri/src/periodic/config.rs @@ -5,7 +5,7 @@ use std::{ }; pub use defguard_client_config_sync::commands::{ - disable_enterprise_features, do_update_instance, locations_changed, + disable_enterprise_features, do_update_instance, locations_changed, sync_service_locations, }; use defguard_client_config_sync::{ poll_instance, poll_instances, PollInstanceResult, VersionMismatchPayload,