From cc1478adc2dedb39b2c722418a974699f32f24a1 Mon Sep 17 00:00:00 2001 From: Reactor Scram Date: Thu, 25 Jul 2024 10:45:22 -0500 Subject: [PATCH] feat(headless-client/windows): add DNS change / network change listening to the Headless Client (#6022) Note that for GUI Clients, listening is still done by the GUI process, not the IPC service. Yak shave towards #5846. This allows for faster dev cycles since I won't have to compile all the GUI stuff. Some changes in here were extracted from other draft PRs. Changes: - Remove `thiserror` that was never matched on - Don't return the DNS resolvers from the notifier directly, just send a notification and allow the caller to check the resolvers itself if needed - Rename `DnsListener` to `DnsNotifier` - Rename `Worker` to `NetworkNotifier` - remove `unwrap_or_default` when getting resolvers. I don't know why it's there, if there's a good reason then it should be handled inside the function, not in the caller ```[tasklist] ### Tasks - [x] Rename `*Listener` to `*Notifier` - [x] (not needed) ~~Support `/etc/resolv.conf` DNS control method too?~~ ``` --- rust/Cargo.lock | 5 +- rust/bin-shared/Cargo.toml | 13 +++ rust/bin-shared/src/lib.rs | 4 + .../src}/network_changes.rs | 2 +- rust/bin-shared/src/network_changes/linux.rs | 46 ++++++++ .../src}/network_changes/macos.rs | 0 .../src}/network_changes/windows.rs | 109 +++++++----------- rust/gui-client/src-tauri/Cargo.toml | 10 -- rust/gui-client/src-tauri/src/client.rs | 1 - rust/gui-client/src-tauri/src/client/gui.rs | 27 ++--- .../src/client/network_changes/linux.rs | 62 ---------- rust/headless-client/src/standalone.rs | 32 ++++- 12 files changed, 145 insertions(+), 166 deletions(-) rename rust/{gui-client/src-tauri/src/client => bin-shared/src}/network_changes.rs (85%) create mode 100644 rust/bin-shared/src/network_changes/linux.rs rename rust/{gui-client/src-tauri/src/client => bin-shared/src}/network_changes/macos.rs (100%) rename rust/{gui-client/src-tauri/src/client => bin-shared/src}/network_changes/windows.rs (87%) delete mode 100644 rust/gui-client/src-tauri/src/client/network_changes/linux.rs diff --git a/rust/Cargo.lock b/rust/Cargo.lock index 60f7b0fe2..308f5719d 100644 --- a/rust/Cargo.lock +++ b/rust/Cargo.lock @@ -1801,6 +1801,9 @@ dependencies = [ "url", "uuid", "windows 0.57.0", + "windows-core 0.57.0", + "windows-implement 0.57.0", + "winreg 0.52.0", "wintun", ] @@ -1886,8 +1889,6 @@ dependencies = [ "url", "uuid", "windows 0.57.0", - "windows-core 0.57.0", - "windows-implement 0.57.0", "winreg 0.52.0", "wintun", "zip", diff --git a/rust/bin-shared/Cargo.toml b/rust/bin-shared/Cargo.toml index 4c71aadad..bf43efb26 100644 --- a/rust/bin-shared/Cargo.toml +++ b/rust/bin-shared/Cargo.toml @@ -32,15 +32,28 @@ libc = "0.2" known-folders = "1.1.0" ring = "0.17" uuid = { version = "1.10.0", features = ["v4"] } +windows-core = "0.57.0" +windows-implement = "0.57.0" wintun = "0.4.0" +winreg = "0.52.0" [target.'cfg(windows)'.dependencies.windows] version = "0.57.0" features = [ + # For implementing COM interfaces + "implement", "Win32_Foundation", + # For listening for network change events + "Win32_Networking_NetworkListManager", "Win32_NetworkManagement_IpHelper", "Win32_NetworkManagement_Ndis", "Win32_Networking_WinSock", + "Win32_Security", + # COM is needed to listen for network change events + "Win32_System_Com", + # Needed to listen for system DNS changes + "Win32_System_Registry", + "Win32_System_Threading", ] [lints] diff --git a/rust/bin-shared/src/lib.rs b/rust/bin-shared/src/lib.rs index 46d67ad2b..2687c6ec3 100644 --- a/rust/bin-shared/src/lib.rs +++ b/rust/bin-shared/src/lib.rs @@ -1,3 +1,4 @@ +mod network_changes; mod tun_device_manager; #[cfg(target_os = "windows")] @@ -28,6 +29,9 @@ pub const BUNDLE_ID: &str = "dev.firezone.client"; /// Mark for Firezone sockets to prevent routing loops on Linux. pub const FIREZONE_MARK: u32 = 0xfd002021; +#[cfg(any(target_os = "linux", target_os = "windows"))] +pub use network_changes::{DnsNotifier, NetworkNotifier}; + #[cfg(any(target_os = "linux", target_os = "windows"))] pub use tun_device_manager::TunDeviceManager; diff --git a/rust/gui-client/src-tauri/src/client/network_changes.rs b/rust/bin-shared/src/network_changes.rs similarity index 85% rename from rust/gui-client/src-tauri/src/client/network_changes.rs rename to rust/bin-shared/src/network_changes.rs index 39e754729..f0e88a686 100644 --- a/rust/gui-client/src-tauri/src/client/network_changes.rs +++ b/rust/bin-shared/src/network_changes.rs @@ -13,4 +13,4 @@ mod imp; #[allow(clippy::unnecessary_wraps)] mod imp; -pub(crate) use imp::{check_internet, DnsListener, Worker}; +pub use imp::{DnsNotifier, NetworkNotifier}; diff --git a/rust/bin-shared/src/network_changes/linux.rs b/rust/bin-shared/src/network_changes/linux.rs new file mode 100644 index 000000000..26081aeba --- /dev/null +++ b/rust/bin-shared/src/network_changes/linux.rs @@ -0,0 +1,46 @@ +//! Not implemented for Linux yet + +use anyhow::Result; +use tokio::time::Interval; + +pub struct NetworkNotifier {} + +impl NetworkNotifier { + pub fn new() -> Result { + Ok(Self {}) + } + + pub fn close(&mut self) -> Result<()> { + Ok(()) + } + + /// Not implemented on Linux + /// + /// On Windows this returns when we gain or lose Internet. + pub async fn notified(&mut self) { + futures::future::pending().await + } +} + +pub struct DnsNotifier { + interval: Interval, +} + +impl DnsNotifier { + pub fn new() -> Result { + Ok(Self { + interval: create_interval(), + }) + } + + pub async fn notified(&mut self) -> Result<()> { + self.interval.tick().await; + Ok(()) + } +} + +fn create_interval() -> Interval { + let mut interval = tokio::time::interval(std::time::Duration::from_secs(5)); + interval.set_missed_tick_behavior(tokio::time::MissedTickBehavior::Delay); + interval +} diff --git a/rust/gui-client/src-tauri/src/client/network_changes/macos.rs b/rust/bin-shared/src/network_changes/macos.rs similarity index 100% rename from rust/gui-client/src-tauri/src/client/network_changes/macos.rs rename to rust/bin-shared/src/network_changes/macos.rs diff --git a/rust/gui-client/src-tauri/src/client/network_changes/windows.rs b/rust/bin-shared/src/network_changes/windows.rs similarity index 87% rename from rust/gui-client/src-tauri/src/client/network_changes/windows.rs rename to rust/bin-shared/src/network_changes/windows.rs index f39e1a15f..233c429c1 100644 --- a/rust/gui-client/src-tauri/src/client/network_changes/windows.rs +++ b/rust/bin-shared/src/network_changes/windows.rs @@ -64,7 +64,7 @@ //! //! Raymond Chen also explains it on his blog: -use anyhow::Result; +use anyhow::{anyhow, Context as _, Result}; use tokio::sync::mpsc; use windows::{ core::{Interface, Result as WinResult, GUID}, @@ -77,55 +77,22 @@ use windows::{ }, }; -pub(crate) use async_dns::CombinedListener as DnsListener; +pub use async_dns::DnsNotifier; -#[derive(thiserror::Error, Debug)] -pub(crate) enum Error { - #[error("Couldn't initialize COM: {0}")] - ComInitialize(windows::core::Error), - #[error("Couldn't stop worker thread")] - CouldntStopWorkerThread, - #[error("Couldn't creat NetworkListManager")] - CreateNetworkListManager(windows::core::Error), - #[error("Couldn't start listening to network events: {0}")] - Listening(windows::core::Error), - #[error("Couldn't stop listening to network events: {0}")] - Unadvise(windows::core::Error), -} - -/// Returns true if Windows thinks we have Internet access per [IsConnectedToInternet](https://learn.microsoft.com/en-us/windows/win32/api/netlistmgr/nf-netlistmgr-inetworklistmanager-get_isconnectedtointernet) -/// -/// Call this when `Listener` notifies you. -pub fn check_internet() -> Result { - // Retrieving the INetworkListManager takes less than half a millisecond, and this - // makes the lifetimes and Send+Sync much simpler for callers, so just retrieve it - // every single time. - // SAFETY: No lifetime problems. TODO: Could threading be a problem? - // I think in practice we'll never call this from two threads, but what if we did? - // Maybe make it a method on a `!Send + !Sync` guard struct? - let network_list_manager: INetworkListManager = - unsafe { Com::CoCreateInstance(&NetworkListManager, None, Com::CLSCTX_ALL) }?; - // SAFETY: `network_list_manager` isn't shared between threads, and the lifetime - // should be good. - let have_internet = unsafe { network_list_manager.IsConnectedToInternet() }?.as_bool(); - - Ok(have_internet) -} - -/// Worker thread that can be joined explicitly, and joins on Drop -pub(crate) struct Worker { +/// Notifies when we change Wi-Fi networks, change between Wi-Fi and Ethernet, or gain / lose Internet +pub struct NetworkNotifier { inner: Option, rx: mpsc::Receiver<()>, } /// Needed so that `Drop` can consume the oneshot Sender and the thread's JoinHandle struct WorkerInner { - thread: std::thread::JoinHandle>, + thread: std::thread::JoinHandle>, stopper: tokio::sync::oneshot::Sender<()>, } -impl Worker { - pub(crate) fn new() -> Result { +impl NetworkNotifier { + pub fn new() -> Result { let (tx, rx) = mpsc::channel(1); let (stopper, stopper_rx) = tokio::sync::oneshot::channel(); @@ -150,12 +117,12 @@ impl Worker { } /// Same as `drop`, but you can catch errors - pub(crate) fn close(&mut self) -> Result<()> { + pub fn close(&mut self) -> Result<()> { if let Some(inner) = self.inner.take() { inner .stopper .send(()) - .map_err(|_| Error::CouldntStopWorkerThread)?; + .map_err(|_| anyhow!("Couldn't stop `NetworkNotifier` worker thread"))?; match inner.thread.join() { Err(e) => std::panic::resume_unwind(e), Ok(x) => x?, @@ -164,12 +131,12 @@ impl Worker { Ok(()) } - pub(crate) async fn notified(&mut self) { + pub async fn notified(&mut self) { self.rx.recv().await; } } -impl Drop for Worker { +impl Drop for NetworkNotifier { fn drop(&mut self) { self.close() .expect("should be able to close Worker cleanly"); @@ -193,12 +160,12 @@ type PhantomUnsendUnsync = std::marker::PhantomData<*const ()>; impl ComGuard { /// Initialize a "Multi-threaded apartment" so that Windows COM stuff /// can be called, and COM callbacks can work. - pub fn new() -> Result { + pub fn new() -> Result { // SAFETY: Threading shouldn't be a problem since this is meant to initialize // COM per-thread anyway. unsafe { Com::CoInitializeEx(None, Com::COINIT_MULTITHREADED) } .ok() - .map_err(Error::ComInitialize)?; + .context("Failed in `CoInitializeEx`")?; Ok(Self { dropped: false, _unsend_unsync: Default::default(), @@ -219,7 +186,7 @@ impl Drop for ComGuard { } } -/// Listens to network connectivity change eents +/// Listens to network connectivity change events struct Listener<'a> { /// The cookies we get back from `Advise`. Can be None if the owner called `close` /// @@ -253,19 +220,20 @@ impl<'a> Listener<'a> { /// on the same thread as `new` is called on. /// * `tx` - A Sender to notify when Windows detects /// connectivity changes. Some notifications may be spurious. - fn new(com: &'a ComGuard, tx: mpsc::Sender<()>) -> Result { + fn new(com: &'a ComGuard, tx: mpsc::Sender<()>) -> Result { // `windows-rs` automatically releases (de-refs) COM objects on Drop: // https://github.com/microsoft/windows-rs/issues/2123#issuecomment-1293194755 // https://github.com/microsoft/windows-rs/blob/cefdabd15e4a7a7f71b7a2d8b12d5dc148c99adb/crates/samples/windows/wmi/src/main.rs#L22 // SAFETY: TODO let network_list_manager: INetworkListManager = unsafe { Com::CoCreateInstance(&NetworkListManager, None, Com::CLSCTX_ALL) } - .map_err(Error::CreateNetworkListManager)?; - let cpc: Com::IConnectionPointContainer = - network_list_manager.cast().map_err(Error::Listening)?; + .context("Failed in `CoCreateInstance`")?; + let cpc: Com::IConnectionPointContainer = network_list_manager + .cast() + .context("Failed to cast network list manager")?; // SAFETY: TODO - let cxn_point_net = - unsafe { cpc.FindConnectionPoint(&INetworkEvents::IID) }.map_err(Error::Listening)?; + let cxn_point_net = unsafe { cpc.FindConnectionPoint(&INetworkEvents::IID) } + .context("Failed in `FindConnectionPoint`")?; let mut this = Listener { advise_cookie_net: None, @@ -280,8 +248,10 @@ impl<'a> Listener<'a> { // SAFETY: What happens if Windows sends us a network change event while // we're dropping Listener? // Is it safe to Advise on `this` and then immediately move it? - this.advise_cookie_net = - Some(unsafe { this.cxn_point_net.Advise(&callbacks) }.map_err(Error::Listening)?); + this.advise_cookie_net = Some( + unsafe { this.cxn_point_net.Advise(&callbacks) } + .context("Failed to listen for network event callbacks")?, + ); // After we call `Advise`, notify. This should avoid a problem if this happens: // @@ -297,10 +267,11 @@ impl<'a> Listener<'a> { /// Like `drop`, but you can catch errors /// /// Unregisters the network change callbacks - pub fn close(&mut self) -> Result<(), Error> { + pub fn close(&mut self) -> Result<()> { if let Some(cookie) = self.advise_cookie_net.take() { // SAFETY: I don't see any memory safety issues. - unsafe { self.cxn_point_net.Unadvise(cookie) }.map_err(Error::Unadvise)?; + unsafe { self.cxn_point_net.Unadvise(cookie) } + .context("Failed to unadvise connection point")?; tracing::debug!("Unadvised"); } Ok(()) @@ -343,7 +314,8 @@ impl Drop for Callback { mod async_dns { use anyhow::{Context, Result}; - use std::{ffi::c_void, net::IpAddr, ops::Deref, path::Path}; + use futures::FutureExt as _; + use std::{ffi::c_void, ops::Deref, path::Path, pin::pin}; use tokio::sync::mpsc; use windows::Win32::{ Foundation::{CloseHandle, BOOLEAN, HANDLE, INVALID_HANDLE_VALUE}, @@ -379,13 +351,13 @@ mod async_dns { )) } - pub(crate) struct CombinedListener { + pub struct DnsNotifier { listener_4: Listener, listener_6: Listener, } - impl CombinedListener { - pub(crate) fn new() -> Result { + impl DnsNotifier { + pub fn new() -> Result { let (key_ipv4, key_ipv6) = open_network_registry_keys()?; let listener_4 = Listener::new(key_ipv4)?; let listener_6 = Listener::new(key_ipv6)?; @@ -396,15 +368,14 @@ mod async_dns { }) } - pub(crate) async fn notified(&mut self) -> Result> { - tokio::select! { - r = self.listener_4.notified() => r?, - r = self.listener_6.notified() => r?, + pub async fn notified(&mut self) -> Result<()> { + let mut fut_4 = pin!(self.listener_4.notified().fuse()); + let mut fut_6 = pin!(self.listener_6.notified().fuse()); + futures::select! { + r = fut_4 => r?, + r = fut_6 => r?, } - Ok( - firezone_headless_client::dns_control::system_resolvers_for_gui() - .unwrap_or_default(), - ) + Ok(()) } } diff --git a/rust/gui-client/src-tauri/Cargo.toml b/rust/gui-client/src-tauri/Cargo.toml index 98e6e1640..f86ee5543 100644 --- a/rust/gui-client/src-tauri/Cargo.toml +++ b/rust/gui-client/src-tauri/Cargo.toml @@ -66,23 +66,13 @@ nix = { version = "0.28.0", features = ["user"] } [target.'cfg(target_os = "windows")'.dependencies] tauri-winrt-notification = "0.5.0" -windows-core = "0.57.0" -windows-implement = "0.57.0" winreg = "0.52.0" wintun = "0.4.0" [target.'cfg(target_os = "windows")'.dependencies.windows] version = "0.57.0" features = [ - # For implementing COM interfaces - "implement", "Win32_Foundation", - # For listening for network change events - "Win32_Networking_NetworkListManager", - # COM is needed to listen for network change events - "Win32_System_Com", - # Needed to listen for system DNS changes - "Win32_System_Registry", "Win32_System_Threading", ] diff --git a/rust/gui-client/src-tauri/src/client.rs b/rust/gui-client/src-tauri/src/client.rs index 157daf15d..1c31120d0 100644 --- a/rust/gui-client/src-tauri/src/client.rs +++ b/rust/gui-client/src-tauri/src/client.rs @@ -13,7 +13,6 @@ mod elevation; mod gui; mod ipc; mod logging; -mod network_changes; mod settings; mod updates; mod uptime; diff --git a/rust/gui-client/src-tauri/src/client/gui.rs b/rust/gui-client/src-tauri/src/client/gui.rs index 707ed171d..a63b52df9 100644 --- a/rust/gui-client/src-tauri/src/client/gui.rs +++ b/rust/gui-client/src-tauri/src/client/gui.rs @@ -4,12 +4,13 @@ //! The real macOS Client is in `swift/apple` use crate::client::{ - self, about, deep_link, ipc, logging, network_changes, + self, about, deep_link, ipc, logging, settings::{self, AdvancedSettings}, Failure, }; use anyhow::{anyhow, bail, Context, Result}; use connlib_client_shared::callbacks::ResourceDescription; +use firezone_bin_shared::{DnsNotifier, NetworkNotifier}; use firezone_headless_client::IpcServerMsg; use secrecy::{ExposeSecret, SecretString}; use std::{ @@ -796,32 +797,24 @@ async fn run_controller( win.show().context("Couldn't show Welcome window")?; } - let mut have_internet = - network_changes::check_internet().context("Failed initial check for internet")?; - tracing::info!(?have_internet); + let mut com_worker = NetworkNotifier::new().context("Failed to listen for network changes")?; - let mut com_worker = - network_changes::Worker::new().context("Failed to listen for network changes")?; - - let mut dns_listener = network_changes::DnsListener::new()?; + let mut dns_listener = DnsNotifier::new()?; loop { // TODO: Add `ControllerRequest::NetworkChange` and `DnsChange` and replace // `tokio::select!` with a `poll_*` function tokio::select! { () = com_worker.notified() => { - let new_have_internet = network_changes::check_internet().context("Failed to check for internet")?; - if new_have_internet != have_internet { - have_internet = new_have_internet; - if controller.status.connlib_is_up() { - tracing::debug!("Internet up/down changed, calling `Session::reconnect`"); - controller.ipc_client.reconnect().await?; - } + if controller.status.connlib_is_up() { + tracing::debug!("Internet up/down changed, calling `Session::reconnect`"); + controller.ipc_client.reconnect().await?; } }, - resolvers = dns_listener.notified() => { - let resolvers = resolvers?; + result = dns_listener.notified() => { + result?; if controller.status.connlib_is_up() { + let resolvers = firezone_headless_client::dns_control::system_resolvers_for_gui()?; tracing::debug!(?resolvers, "New DNS resolvers, calling `Session::set_dns`"); controller.ipc_client.set_dns(resolvers).await?; } diff --git a/rust/gui-client/src-tauri/src/client/network_changes/linux.rs b/rust/gui-client/src-tauri/src/client/network_changes/linux.rs deleted file mode 100644 index 35fc6043c..000000000 --- a/rust/gui-client/src-tauri/src/client/network_changes/linux.rs +++ /dev/null @@ -1,62 +0,0 @@ -//! Not implemented for Linux yet - -use anyhow::Result; -use firezone_headless_client::dns_control::system_resolvers_for_gui; -use std::net::IpAddr; -use tokio::time::Interval; - -/// TODO: Implement for Linux -pub(crate) fn check_internet() -> Result { - Ok(true) -} - -pub(crate) struct Worker {} - -impl Worker { - pub(crate) fn new() -> Result { - Ok(Self {}) - } - - pub(crate) fn close(&mut self) -> Result<()> { - Ok(()) - } - - /// Not implemented on Linux - /// - /// On Windows this returns when we gain or lose Internet. - pub(crate) async fn notified(&mut self) { - futures::future::pending().await - } -} - -pub(crate) struct DnsListener { - interval: Interval, - last_seen: Vec, -} - -impl DnsListener { - pub(crate) fn new() -> Result { - Ok(Self { - interval: create_interval(), - last_seen: system_resolvers_for_gui().unwrap_or_default(), - }) - } - - pub(crate) async fn notified(&mut self) -> Result> { - loop { - self.interval.tick().await; - tracing::trace!("Checking for DNS changes"); - let new = system_resolvers_for_gui().unwrap_or_default(); - if new != self.last_seen { - self.last_seen.clone_from(&new); - return Ok(new); - } - } - } -} - -fn create_interval() -> Interval { - let mut interval = tokio::time::interval(std::time::Duration::from_secs(5)); - interval.set_missed_tick_behavior(tokio::time::MissedTickBehavior::Delay); - interval -} diff --git a/rust/headless-client/src/standalone.rs b/rust/headless-client/src/standalone.rs index a6576e20d..11c341884 100644 --- a/rust/headless-client/src/standalone.rs +++ b/rust/headless-client/src/standalone.rs @@ -9,7 +9,9 @@ use backoff::ExponentialBackoffBuilder; use clap::Parser; use connlib_client_shared::{file_logger, keypair, ConnectArgs, LoginUrl, Session}; use connlib_shared::get_user_agent; -use firezone_bin_shared::{setup_global_subscriber, TunDeviceManager}; +use firezone_bin_shared::{ + setup_global_subscriber, DnsNotifier, NetworkNotifier, TunDeviceManager, +}; use futures::{FutureExt as _, StreamExt as _}; use phoenix_channel::PhoenixChannel; use secrecy::{Secret, SecretString}; @@ -196,22 +198,38 @@ pub fn run_only_headless_client() -> Result<()> { let mut tun_device = TunDeviceManager::new()?; let mut cb_rx = ReceiverStream::new(cb_rx).fuse(); + let mut dns_notifier = DnsNotifier::new()?; + let mut network_notifier = NetworkNotifier::new()?; + let tun = tun_device.make_tun()?; session.set_tun(Box::new(tun)); - // TODO: DNS should be added dynamically session.set_dns(dns_control::system_resolvers().unwrap_or_default()); loop { + let mut dns_changed = pin!(dns_notifier.notified().fuse()); + let mut network_changed = pin!(network_notifier.notified().fuse()); + let cb = futures::select! { () = terminate => { tracing::info!("Caught SIGINT / SIGTERM / Ctrl+C"); - return Ok(()); + break; }, () = hangup => { tracing::info!("Caught SIGHUP"); session.reconnect(); continue; }, + result = dns_changed => { + result?; + tracing::info!("DNS change, notifying Session"); + session.set_dns(dns_control::system_resolvers()?); + continue; + }, + () = network_changed => { + tracing::info!("Network change, reconnecting Session"); + session.reconnect(); + continue; + }, cb = cb_rx.next() => cb.context("cb_rx unexpectedly ran empty")?, }; @@ -239,11 +257,17 @@ pub fn run_only_headless_client() -> Result<()> { } if cli.exit { tracing::info!("Exiting due to `--exit` CLI flag"); - break Ok(()); + break; } } } } + + if let Err(error) = network_notifier.close() { + tracing::error!(?error, "network listener"); + } + + Ok(()) }); session.disconnect();