Skip to content

Commit

Permalink
chore(windows): document Windows network_changes behavior (#4130)
Browse files Browse the repository at this point in the history
Ref #3429 

It doesn't report DNS changes, but I added a proof of concept for how we
could do that.
  • Loading branch information
ReactorScram committed Mar 13, 2024
1 parent 6a7d8c8 commit 4c77aae
Show file tree
Hide file tree
Showing 6 changed files with 84 additions and 3 deletions.
2 changes: 2 additions & 0 deletions rust/gui-client/src-tauri/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,8 @@ features = [
"Win32_System_JobObjects",
# Needed to check process ID of named pipe clients
"Win32_System_Pipes",
# Needed to listen for system DNS changes
"Win32_System_Registry",
"Win32_System_Threading",
# For deep_link module
"Win32_System_SystemServices",
Expand Down
2 changes: 2 additions & 0 deletions rust/gui-client/src-tauri/src/client/debug_commands.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ use anyhow::Result;
pub enum Cmd {
CheckForUpdates,
Crash,
DnsChanges,
Hostname,
NetworkChanges,
}
Expand All @@ -16,6 +17,7 @@ pub fn run(cmd: Cmd) -> Result<()> {
match cmd {
Cmd::CheckForUpdates => check_for_updates(),
Cmd::Crash => crash(),
Cmd::DnsChanges => client::network_changes::run_dns_debug(),
Cmd::Hostname => hostname(),
Cmd::NetworkChanges => client::network_changes::run_debug(),
}
Expand Down
2 changes: 1 addition & 1 deletion rust/gui-client/src-tauri/src/client/network_changes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,4 +10,4 @@ mod imp;
#[path = "network_changes/windows.rs"]
mod imp;

pub(crate) use imp::{check_internet, run_debug, Worker};
pub(crate) use imp::{check_internet, run_debug, run_dns_debug, Worker};
5 changes: 5 additions & 0 deletions rust/gui-client/src-tauri/src/client/network_changes/linux.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,11 @@ use anyhow::Result;
#[derive(thiserror::Error, Debug)]
pub(crate) enum Error {}

pub(crate) fn run_dns_debug() -> Result<()> {
tracing::warn!("network_changes not implemented yet on Linux");
Ok(())
}

pub(crate) fn run_debug() -> Result<()> {
tracing::warn!("network_changes not implemented yet on Linux");
Ok(())
Expand Down
4 changes: 4 additions & 0 deletions rust/gui-client/src-tauri/src/client/network_changes/macos.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,10 @@ pub(crate) fn run_debug() -> Result<()> {
unimplemented!()
}

pub(crate) fn run_dns_debug() -> Result<()> {
unimplemented!()
}

pub(crate) fn check_internet() -> Result<bool> {
tracing::error!("This is not the real macOS client, so `network_changes` is not implemented");
Ok(true)
Expand Down
72 changes: 70 additions & 2 deletions rust/gui-client/src-tauri/src/client/network_changes/windows.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,28 @@
//! A module for getting callbacks from Windows when we gain / lose Internet connectivity
//!
//! # Changes detected / not detected
//!
//! Tested manually one time each, with `RUST_LOG=info cargo run -p firezone-gui-client -- debug network-changes`
//!
//! Some network changes will fire multiple events with `have_internet = true` in a row.
//! Others will report `have_internet = false` and then `have_internet = true` once they reconnect.
//!
//! We could attempt to listen for DNS changes by subscribing to changes in the Windows Registry: <https://stackoverflow.com/a/64482724>
//!
//! - Manually changing DNS servers on Wi-Fi, not detected
//!
//! - Wi-Fi-only, enable Airplane Mode, <1 second
//! - Disable Airplane Mode, return to Wi-Fi, <5 seconds
//! - Wi-Fi-only, disable Wi-Fi, <1 second
//! - Wi-Fi-only, enable Wi-Fi, <5 seconds
//! - Switching to hotspot Wi-Fi from a phone, instant (once Windows connects)
//! - Stopping the phone's hotspot and switching back to home Wi-Fi, instant (once Windows connects)
//! - On Wi-Fi, connect Ethernet, <4 seconds
//! - On Ethernet and Wi-Fi, disconnect Ethernet, not detected
//! - On Ethernet, Wi-Fi enabled but not connected, disconnect Ethernet, <2 seconds
//! - On Wi-Fi, WLAN loses Internet, 1 minute (Windows doesn't figure it out immediately)
//! - On Wi-Fi, WLAN regains Internet, 6 seconds (Some of that is the AP)
//!
//! # Latency
//!
//! 2 or 3 seconds for the user clicking "Connect" or "Disconnect" on Wi-Fi,
Expand Down Expand Up @@ -43,7 +66,7 @@ use windows::{
INetworkEvents, INetworkEvents_Impl, INetworkListManager, NetworkListManager,
NLM_CONNECTIVITY, NLM_NETWORK_PROPERTY_CHANGE,
},
System::Com,
System::{Com, Registry},
},
};

Expand Down Expand Up @@ -79,7 +102,7 @@ pub(crate) fn run_debug() -> Result<()> {
Ok((Com::APTTYPE_MTA, Com::APTTYPEQUALIFIER_NONE))
);

let rt = Runtime::new().unwrap();
let rt = Runtime::new()?;

tracing::info!("Listening for network events...");

Expand All @@ -98,6 +121,51 @@ pub(crate) fn run_debug() -> Result<()> {
})
}

/// Runs a debug subcommand that listens to the registry for DNS changes
///
/// This actually listens to the entire IPv4 key, so it will have lots of false positives,
/// including when connlib changes anything on the Firezone tunnel.
/// It will often fire multiple events in quick succession.
pub(crate) fn run_dns_debug() -> Result<()> {
tracing_subscriber::fmt::init();
tracing::info!("Listening for network events...");

// TODO: Use WaitForMultipleObjects or whatever to async await both IPv4 and IPv6 DNS changes

let hklm = winreg::RegKey::predef(winreg::enums::HKEY_LOCAL_MACHINE);
let path = std::path::Path::new("SYSTEM")
.join("CurrentControlSet")
.join("Services")
.join("Tcpip")
.join("Parameters")
.join("Interfaces");
let key = hklm.open_subkey_with_flags(path, winreg::enums::KEY_NOTIFY)?;
let key_handle = Registry::HKEY(key.raw_handle());
let notify_flags = Registry::REG_NOTIFY_CHANGE_NAME | Registry::REG_NOTIFY_CHANGE_LAST_SET;

loop {
// SAFETY: No buffers or pointers involved, just the handle to the reg key.
unsafe {
Registry::RegNotifyChangeKeyValue(key_handle, true, notify_flags, None, false);
}

// TODO: It's possible we could miss an event here:
//
// - We call `RegNotifyChangeKeyValue`
// - Some un-important change happens and spuriously wake up
// - We notify connlib
// - Connlib does nothing since the change didn't matter
// - An important change happens but our thread hasn't looped over yet
// - We call `RegNotifyChangeKeyValue` again, having missed the second change
//
// Switching to async may offer a way to close this gap, since we can re-register
// the notify before notifying connlib. Then the second change should cause us to
// immediately re-notify connlib.

tracing::info!("Something changed.");
}
}

/// 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.
Expand Down

0 comments on commit 4c77aae

Please sign in to comment.