fix(windows): ignore network changes from irrelevant networks - #9696
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@jamilbk Could you test this on your Windows machine please? Clients here: https://github.com/firezone/firezone/actions/runs/15924451506 |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the Windows network events handling to ignore network change notifications from networks not classified as wired, wireless, or mobile broadband.
- Replaces the single Firezone network profile check with a collection of ignored network profiles computed from registry values.
- Modifies the callback logic to iterate over the list of ignored networks and conditionally skip processing.
Comments suppressed due to low confidence (1)
rust/bin-shared/src/network_changes/windows.rs:386
- The identifier 'nametypes' is not defined; it appears this should be 'nametype' as retrieved from the registry. Updating this would resolve the variable naming inconsistency.
if !RELEVANT_NAME_TYPES.contains(&nametypes) {
| let nametype = profiles | ||
| .open_subkey(&guid) | ||
| .with_context(|| format!("Failed to open key `{guid}`"))? | ||
| .get_value::<u32, _>("NameType") | ||
| .context("Failed to get name type")?; | ||
|
|
||
| let profile_name = profiles | ||
| .open_subkey(&guid) | ||
| .with_context(|| format!("Failed to open key `{guid}`"))? |
There was a problem hiding this comment.
The registry subkey is opened multiple times for the same GUID to get 'NameType' and 'ProfileName'. Consider opening the subkey once and reusing it to improve code maintainability and efficiency.
| let nametype = profiles | |
| .open_subkey(&guid) | |
| .with_context(|| format!("Failed to open key `{guid}`"))? | |
| .get_value::<u32, _>("NameType") | |
| .context("Failed to get name type")?; | |
| let profile_name = profiles | |
| .open_subkey(&guid) | |
| .with_context(|| format!("Failed to open key `{guid}`"))? | |
| let subkey = profiles | |
| .open_subkey(&guid) | |
| .with_context(|| format!("Failed to open key `{guid}`"))?; | |
| let nametype = subkey | |
| .get_value::<u32, _>("NameType") | |
| .context("Failed to get name type")?; | |
| let profile_name = subkey |
Signed-off-by: Thomas Eizinger <thomas@eizinger.io>
The only alternative I can think of is to use a block list instead of an allow list, i.e. disallow network with name type 53. This also bears the risk that we are not filtering out enough networks and customers still have a degraded UX. Last but not least, the change detection being buggy is not ideal but can be fixed by signing out and in manually and therefore is hopefully not a deal-breaker. |
|
On Apple we check the order of interfaces on the default path (0.0.0.0/0 route essentially). It's been pretty good so far, but I realize Windows is different. So this will essentially check when the interface used for "Internet" has changed? I wonder what Windows considers "changed". Hopefully not too noisy. |
If you know to do this to fix Firezone then it might be a stop gap, but users won't know unfortunately until they complain to their admin and then to us. These are users who were confused about finding Firezone in the task bar to sign in / out remember. I'll give it a spin and try to give it a good looking over. |
No, it has actually nothing to do with what the interface is being used for. This will ignore network change events from interfaces that aren't marked as "Wired", "Wireless" or "Mobile broadband". So for example, change events from VPN connections are ignored. From the customer logs I've based this on, I know that there might be odd local interfaces that also emit these change events. Those are of a different type though so will be ignored with this patch set too. In my testing on the VM, this will work. There, the interface is classified as a "wired" interface.
It is not that easy on Windows unfortunately. The network profile list we get here doesn't give us any information about routes. And even with the routes, we don't necessarily know which interface is being used for traffic (there might be competing routes) unless we recompute the same metrics Windows is (allegedly) using internally. So I think, if we want to be reliable, it is better to react to a few more events here than less. |
I think ultimately, something like this would be a good implementation. But this is not as easy to implement and requires a lot more testing. So considering that, simply ignoring certain types of network whilst still reacting to all events seems to be a good trade-off in terms of implementation complexity and effectiveness. I am open to alternatives though. Once I have my Windows development machine, it should be easy to work on a more targeted implementation. Roaming is unfortunately quite hard test in the VM. |
|
Ok, will give this a spin with its current design sometime today. |
jamilbk
left a comment
There was a problem hiding this comment.
LGTM. Works fine switching between Wired / Wireless.
In order to detect network changes on Windows, we implement the
INetworkEventscallback interface. This callback notifies us every time the connectivity of a certain network changes.Performing a network reset in connlib on any of these changes hurts the user experience as Firezone is booting because it takes a while for this to settle. Firezone itself is making changes to the network so several of these change events happen because Firezone is starting.
The documentation from Microsoft on what possible values the
NameTypeattribute can have is pretty thin but I did manage to find the following values on the Internet:6: Wired network71: Wireless network243: Broadband networkWe assume that the user is connected to the Internet through one of these so we ignore network changes on all other networks.
An alternative approach to reducing the number of false-positive change events would be to react to a narrower list of change events. I discarded this approach because it wasn't clear to me, which of the event types 0 would matter to us and when Windows emits them. I think in order to effectively react to those, we'd have to do more fine granular tracking of which state a network is in and e.g. only trigger a reset if we move from "Disconnected" to e.g. "Subnet connectivity". Windows also differentiates between local, subnet and Internet connectivity, yet in my testing, I've never observed the "Internet" connectivity being emitted.
Hence, it is deemed more robust to just filter out networks based on their type. Firezone itself is of type 53 and is therefore automatically filtered out as well. The risk here is that we don't react to connectivity changes of a network that a customer is relying on. Unfortunately, I don't think there is a better way to find this out other than shipping this change and waiting for reports.