[ Bug ] Network adapters not properly cleaned up on unexpected shutdown, causing connectivity issues

[ithan]

We've identified a potential issue with DefGuard that can cause network connectivity problems for users. The problem stems from improper cleanup of network adapters when the application is closed unexpectedly (e.g., during a system shutdown). * This happens on windows 11.

Note: This issue report is based on my understanding as a user of the application. I'm not a Rust developer, nor do I have extensive experience with network adapters. The following is my speculation on the root cause and potential solutions based on observed behavior. Technical details may need verification.

Current behavior

1. A user connects to the VPN using the DefGuard client.
2. If the user shuts down their computer without properly disconnecting from DefGuard, the network traffic remains redirected to the VPN adapter.
3. Upon restarting the computer, the VPN adapter is not enabled and has no internet access, but traffic is still being redirected to it.
4. This results in the user being unable to access work-related web pages or other network resources.
5. In some cases, re-enabling DefGuard resolves the issue, but often it creates a new adapter, leaving the old one in place and still inaccessible.
6. The only reliable fix is for the user to manually disable the orphaned adapter.

Proposed solution

1. Implement a startup check that scans for and identifies any DefGuard-related network adapters, regardless of what's in the in-memory cache.
2. Enhance the close_all_connections function to handle both cached connections and any discovered orphaned adapters.

Cache-related issue

The application currently uses an in-memory cache to store active connections:

pub struct AppState {
    pub db: Arc<Mutex<Option<DbPool>>>,
    pub active_connections: Arc<Mutex<Vec<ActiveConnection>>>,
    // ... other fields
}

pub fn get_connections(&self) -> Vec<ActiveConnection> {
    self.active_connections
        .lock()
        .expect("Failed to lock active connections mutex")
        .clone()
}

This in-memory storage is volatile and doesn't persist across application restarts. As a result, when the application starts up after an unexpected shutdown, it has no record of the previously active connections.

Why simply calling close_all_connections on startup wouldn't work

The current close_all_connections function only operates on the connections stored in the in-memory cache:

pub async fn close_all_connections(&self) -> Result<(), crate::error::Error> {
    info!("Closing all active connections...");
    let active_connections = self.get_connections();
    // ... rest of the function
}

If this function were called on startup, it would be working with an empty cache and wouldn't be able to identify or clean up any orphaned adapters from previous sessions.

To address this issue, there is a need to implement the proposed solution, which involves scanning for actual network adapters on the system, rather than relying solely on the in-memory cache.

[rongten]

I have been bitten as well from this. It is problematic if it happens to a remote user.

[aleex1848]

We also have these kind of problems with client v1.2.0

[t-aleksander]

@ithan @rongten @aleex1848 we've released a pre-release version of the windows client that should properly detect and show already established connetions in the system on its start, you can try it out and see if that solves the issue for you: https://github.com/DefGuard/client/releases/tag/v1.2.2-alpha2 (this is a windows only feature currently).

[teon]

1.2.3 is out 🫡

[teon]

1.2.3 is out 🫡