From ecfb198ade76b356b4369c2648f374c01243e1af Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20W=C3=B3jcik?= Date: Wed, 29 Jul 2026 15:59:03 +0200 Subject: [PATCH] port disconnect recovery fix from stable --- src/config.rs | 19 +++++++++ src/gateway.rs | 37 +++++++++++++++--- src/gateway_server.rs | 54 +++++++++++++------------- src/tests/mock_wgapi.rs | 86 ++++++++++++++++++++++++++++++++++++++++- src/tests/mod.rs | 1 + src/tests/mtls.rs | 10 ++--- src/tests/recovery.rs | 53 +++++++++++++++++++++++++ 7 files changed, 220 insertions(+), 40 deletions(-) create mode 100644 src/tests/recovery.rs diff --git a/src/config.rs b/src/config.rs index bbc7941d..529baa7c 100644 --- a/src/config.rs +++ b/src/config.rs @@ -23,6 +23,10 @@ fn default_stats_period() -> u64 { 30 } +fn default_core_disconnect_grace_period() -> u64 { + 30 +} + fn default_ifname() -> String { String::from("wg0") } @@ -66,6 +70,20 @@ pub struct Config { #[serde(default = "default_stats_period")] pub stats_period: u64, + /// Seconds to wait after losing the Defguard Core connection before tearing down + /// the WireGuard interface. Bridges transient Core outages (e.g. a container + /// restart) without dropping VPN traffic; if Core reconnects within the window, + /// the teardown is skipped. Set to 0 to tear down immediately. NOTE: during the + /// window peers keep forwarding while Core is unreachable, so revoked or expired + /// sessions retain access until it elapses. + #[arg( + long, + env = "DEFGUARD_CORE_DISCONNECT_GRACE_PERIOD", + default_value = "30" + )] + #[serde(default = "default_core_disconnect_grace_period")] + pub core_disconnect_grace_period: u64, + /// Network interface name (e.g. wg0) #[arg(long, short = 'i', env = "DEFGUARD_IFNAME", default_value = "wg0")] #[serde(default = "default_ifname")] @@ -184,6 +202,7 @@ impl Default for Config { grpc_port: 50066, userspace: false, stats_period: 15, + core_disconnect_grace_period: 30, ifname: "wg0".into(), pidfile: None, use_syslog: false, diff --git a/src/gateway.rs b/src/gateway.rs index 7f857c6f..79daeb0a 100644 --- a/src/gateway.rs +++ b/src/gateway.rs @@ -207,6 +207,12 @@ impl Gateway { self.connected.store(false, Ordering::Relaxed); } + /// Grace period to wait after a Core disconnect before purging the interface. + /// Zero means purge immediately (no grace). + pub(crate) fn disconnect_grace_period(&self) -> Duration { + Duration::from_secs(self.config.core_disconnect_grace_period) + } + // Replace current peer map with a new list of peers. fn replace_peers(&mut self, new_peers: Vec) { debug!("Replacing stored peers with {} new peers", new_peers.len()); @@ -402,6 +408,19 @@ impl Gateway { mask!(new_configuration, private_key) ); + // configure() is the sole owner of interface existence; recreate it if + // it was removed (e.g. by purge during a disconnect). + { + let mut wgapi = self.wgapi.lock().expect("Failed to lock Gateway::wgapi"); + if wgapi.read_interface_data().is_err() { + info!( + "WireGuard interface {} is not present, creating it", + new_configuration.name + ); + wgapi.create_interface()?; + } + } + // check if new configuration is different than current one let new_interface_configuration = new_configuration.clone().into(); @@ -472,12 +491,18 @@ impl Gateway { if UpdateType::try_from(update.update_type) == Ok(UpdateType::Delete) { debug!("Deleting peer {peer_config:?}"); self.peers.remove(&peer_config.pubkey); - if let Err(err) = - self.wgapi.lock().unwrap().remove_peer( - &peer_config.pubkey.as_str().try_into().unwrap_or_default(), - ) - { - error!("Failed to delete peer: {err}"); + match peer_config.pubkey.as_str().try_into() { + Ok(key) => { + if let Err(err) = self.wgapi.lock().unwrap().remove_peer(&key) { + error!("Failed to delete peer: {err}"); + } + } + Err(err) => { + error!( + "Failed to parse public key of peer {} to delete: {err}", + peer_config.pubkey + ); + } } } // UpdateType::Create, UpdateType::Modify diff --git a/src/gateway_server.rs b/src/gateway_server.rs index c6c4e94a..e5795afa 100644 --- a/src/gateway_server.rs +++ b/src/gateway_server.rs @@ -13,15 +13,15 @@ use defguard_version::{ }; use tokio::{ fs::remove_file, + spawn, sync::{mpsc, oneshot}, + time::sleep, }; use tokio_stream::wrappers::UnboundedReceiverStream; use tonic::{Request, Response, Status, Streaming, service::InterceptorLayer, transport::Server}; use tower::ServiceBuilder; use tracing::instrument; -#[cfg(target_os = "linux")] -use crate::enterprise::firewall::api::{FirewallApi, FirewallManagementApi}; use crate::{ CORE_CLIENT_CERT_NAME, GRPC_CA_CERT_NAME, GRPC_CERT_NAME, GRPC_KEY_NAME, VERSION, config::Config, @@ -63,30 +63,6 @@ impl GatewayServer { pub(crate) async fn start(self, config: Config) -> Result<(), GatewayError> { info!("Starting Defguard Gateway version {VERSION} with configuration: {config:?}"); - // Try to create network interface for WireGuard. - // FIXME: check if the interface already exists, or somehow be more clever. - { - #[allow(unused)] - let mut gateway = &mut self.gateway.lock().expect("gateway mutex poison"); - if let Err(err) = gateway - .wgapi - .lock() - .expect("wgapi mutex poison") - .create_interface() - { - warn!( - "Couldn't create network interface {}: {err}. Proceeding anyway.", - config.ifname - ); - } else { - #[cfg(target_os = "linux")] - if !config.disable_firewall_management && config.masquerade { - let mut firewall_api = FirewallApi::new(&config.ifname)?; - firewall_api.setup_nat(config.masquerade, &[])?; - } - } - } - if let Some(post_up) = &config.post_up { debug!("Executing specified POST_UP command: {post_up}"); execute_command(post_up)?; @@ -251,10 +227,32 @@ impl gateway_server::Gateway for GatewayServer { } } info!("Defguard Core gRPC stream has been disconnected: {address}"); - if let Ok(mut gateway) = gateway.lock() { + // Mark disconnected immediately, but delay tearing down the interface by + // the grace period so a transient Core outage (e.g. a container restart) + // doesn't drop VPN traffic. If Core reconnects within the window the purge + // is skipped; otherwise we fail closed. + let grace_period = { + let mut gateway = gateway.lock().expect("Gateway lock poison"); gateway.connected.store(false, Ordering::Relaxed); gateway.client_tx = None; - gateway.purge(); + gateway.disconnect_grace_period() + }; + if grace_period.is_zero() { + gateway.lock().expect("Gateway lock poison").purge(); + } else { + let gateway = Arc::clone(&gateway); + spawn(async move { + sleep(grace_period).await; + let mut gateway = gateway.lock().expect("Gateway lock poison"); + if gateway.client_tx.is_none() { + info!( + "Core still disconnected after {grace_period:?}; purging (fail-closed)" + ); + gateway.purge(); + } else { + debug!("Core reconnected within grace period; skipping purge"); + } + }); } }); diff --git a/src/tests/mock_wgapi.rs b/src/tests/mock_wgapi.rs index 261daac5..e00d79d5 100644 --- a/src/tests/mock_wgapi.rs +++ b/src/tests/mock_wgapi.rs @@ -1,4 +1,7 @@ -use std::net::IpAddr; +use std::{ + net::IpAddr, + sync::atomic::{AtomicBool, Ordering}, +}; use defguard_wireguard_rs::{ InterfaceConfiguration, WireguardInterfaceApi, error::WireguardInterfaceError, host::Host, @@ -64,3 +67,84 @@ impl WireguardInterfaceApi for NullWgApi { Err(WireguardInterfaceError::Interface("test".into())) } } + +/// A mock WireGuard API that tracks whether the interface exists. +/// +/// Used to reproduce the bug where `purge()` removes the interface, then +/// `configure()` tries to configure a non-existent device and gets `NoDevice`. +pub(crate) struct StatefulMockWgApi { + interface_exists: AtomicBool, +} + +impl StatefulMockWgApi { + #[must_use] + pub(crate) fn new() -> Self { + Self { + interface_exists: AtomicBool::new(true), + } + } +} + +impl WireguardInterfaceApi for StatefulMockWgApi { + fn create_interface(&mut self) -> Result<(), WireguardInterfaceError> { + self.interface_exists.store(true, Ordering::Relaxed); + Ok(()) + } + + fn assign_address(&self, _address: &IpAddrMask) -> Result<(), WireguardInterfaceError> { + Ok(()) + } + + fn configure_peer_routing(&self, _peers: &[WgPeer]) -> Result<(), WireguardInterfaceError> { + Ok(()) + } + + fn configure_interface( + &self, + _config: &InterfaceConfiguration, + ) -> Result<(), WireguardInterfaceError> { + if self.interface_exists.load(Ordering::Relaxed) { + Ok(()) + } else { + Err(WireguardInterfaceError::Interface("no such device".into())) + } + } + + #[cfg(not(target_os = "windows"))] + fn remove_interface(&self) -> Result<(), WireguardInterfaceError> { + self.interface_exists.store(false, Ordering::Relaxed); + Ok(()) + } + + #[cfg(target_os = "windows")] + fn remove_interface(&mut self) -> Result<(), WireguardInterfaceError> { + self.interface_exists.store(false, Ordering::Relaxed); + Ok(()) + } + + fn configure_peer(&self, _peer: &WgPeer) -> Result<(), WireguardInterfaceError> { + Ok(()) + } + + fn remove_peer(&self, _peer_pubkey: &Key) -> Result<(), WireguardInterfaceError> { + Ok(()) + } + + fn read_interface_data(&self) -> Result { + if self.interface_exists.load(Ordering::Relaxed) { + Ok(Host::default()) + } else { + Err(WireguardInterfaceError::ReadInterfaceError( + "no such device".into(), + )) + } + } + + fn configure_dns( + &self, + _dns: &[IpAddr], + _search_domains: &[&str], + ) -> Result<(), WireguardInterfaceError> { + Ok(()) + } +} diff --git a/src/tests/mod.rs b/src/tests/mod.rs index d5fb0405..113ec4f5 100644 --- a/src/tests/mod.rs +++ b/src/tests/mod.rs @@ -1,2 +1,3 @@ mod mock_wgapi; mod mtls; +mod recovery; diff --git a/src/tests/mtls.rs b/src/tests/mtls.rs index a85104fa..cde71673 100644 --- a/src/tests/mtls.rs +++ b/src/tests/mtls.rs @@ -189,7 +189,7 @@ async fn call_bidi(client: &mut GatewayClient) -> tonic::Status { /// `start()` must return `Err` immediately when no `TlsConfig` has been set. #[tokio::test] -async fn start_errors_without_tls_config() { +async fn test_start_errors_without_tls_config() { let config = Config::default(); let gateway = build_gateway(&config); let gateway = Arc::new(Mutex::new(gateway)); @@ -213,7 +213,7 @@ async fn start_errors_without_tls_config() { /// but it must NOT be rejected with `Unauthenticated` - that would indicate the mTLS layer /// or serial-pin interceptor wrongly rejected the cert. #[tokio::test] -async fn valid_mtls_client_accepted() { +async fn test_valid_mtls_client_accepted() { init_crypto(); let certs = TestCerts::generate(); let (port, shutdown_tx) = spawn_test_gateway(&certs).await; @@ -242,7 +242,7 @@ async fn valid_mtls_client_accepted() { /// tonic may reject the connection eagerly (at `connect()` time) or lazily /// (on the first RPC). Both outcomes are treated as the expected rejection. #[tokio::test] -async fn no_client_cert_rejected() { +async fn test_no_client_cert_rejected() { init_crypto(); let certs = TestCerts::generate(); let (port, shutdown_tx) = spawn_test_gateway(&certs).await; @@ -271,7 +271,7 @@ async fn no_client_cert_rejected() { /// A client presenting a cert from the correct CA but with the wrong serial must be rejected /// by the serial-pin interceptor with `Unauthenticated`. #[tokio::test] -async fn wrong_serial_rejected() { +async fn test_wrong_serial_rejected() { init_crypto(); let certs = TestCerts::generate(); let (port, shutdown_tx) = spawn_test_gateway(&certs).await; @@ -304,7 +304,7 @@ async fn wrong_serial_rejected() { /// but the rejection must not be `FailedPrecondition`, which would indicate /// the cert bypassed CA verification and reached the gRPC handler. #[tokio::test] -async fn rogue_ca_client_rejected() { +async fn test_rogue_ca_client_rejected() { init_crypto(); let certs = TestCerts::generate(); let (port, shutdown_tx) = spawn_test_gateway(&certs).await; diff --git a/src/tests/recovery.rs b/src/tests/recovery.rs new file mode 100644 index 00000000..dfaa1068 --- /dev/null +++ b/src/tests/recovery.rs @@ -0,0 +1,53 @@ +use super::mock_wgapi::StatefulMockWgApi; +use crate::{config::Config, gateway::Gateway, proto::gateway::Configuration}; + +/// Regression test for the interface recovery bug (#354). +/// +/// After a disconnect, `Gateway::purge()` calls `remove_interface()` which +/// deletes the kernel WireGuard link. Before the fix, the next `configure()` +/// tried to configure a non-existent device and failed; `configure()` now +/// recreates the interface when it is missing, so recovery succeeds. +/// +/// The test: +/// 1. Builds a Gateway with the stateful mock (interface starts as existing) +/// 2. Calls `purge()`, which drives `remove_interface()` -> interface gone +/// 3. Calls `configure()` and asserts it recreates the interface and succeeds +#[test] +fn test_purge_then_configure_recovers_interface() { + let mut config = Config::default(); + config.disable_firewall_management = true; + let mut gateway = Gateway::new(config, StatefulMockWgApi::new()).unwrap(); + + // Verify the interface exists before purge. + assert!( + gateway.wgapi.lock().unwrap().read_interface_data().is_ok(), + "interface should exist initially" + ); + + // Simulate what happens on disconnect: purge removes the interface. + gateway.purge(); + + // After purge, the interface should be gone. + assert!( + gateway.wgapi.lock().unwrap().read_interface_data().is_err(), + "interface should not exist after purge" + ); + + // Simulate reconnect: Core pushes a new configuration. + let new_config = Configuration { + name: "wg0".into(), + private_key: "aBcDeFgHiJkLmNoPqRsTuVwXyZ0123456789abC=".into(), + port: 51820, + peers: vec![], + addresses: vec!["10.0.0.1/24".into()], + firewall_config: None, + mtu: 1420, + fwmark: 0, + }; + + let result = gateway.configure(new_config); + assert!( + result.is_ok(), + "configure should recover the interface and succeed: {result:?}", + ); +}