Reduce the number of `REGISTER_IP_ADDRESSES` calls #4219

dlon · 2022-12-14T10:59:14Z

Follow-up to #4218. This PR makes a couple of changes:

Don't unregister tunnel IP in ST driver when reconnecting
When entering the error state due to being offline, don't send an update.

Most of the time, this should lower the number of IOCTL calls from 2 to 0 when reconnecting, and from 2 to 1 when connecting if the tunnel IP has changed.

There is a small risk that some other network interface will take over the previous tunnel IP. If that happens, excluded connections will be incorrectly redirected to the default interface, though this is unlikely and is irrelevant from a security standpoint. As long as the tunnel IP is correct when there is a tunnel device, leaks are not possible.

This change is

Jontified

Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @dlon)

talpid-core/src/split_tunnel/windows/mod.rs line 115 at r1 (raw file):

    excluded_processes: Arc<RwLock<HashMap<usize, ExcludedProcess>>>,
    _route_change_callback: Option<CallbackHandle>,
    daemon_tx: Weak<mpsc::UnboundedSender<TunnelCommand>>,

Discussed in previous PR

talpid-core/src/split_tunnel/windows/mod.rs line 857 at r1 (raw file):

    }

    fn create_burst_guard(

Perhaps we'd like to create a generic implementation of a burst guard and use it for taplid-routing/src/windows/default_route_monitor.rs as well? I guess the abort handle is here to make cancelling the task a bit quicker? Just dropping the tx would do and would make it a bit simpler but the task would cancel only at the point were rx.next() returns None. Not saying it is better.

talpid-core/src/split_tunnel/windows/mod.rs line 875 at r1 (raw file):

                Ok(None) => {
                    log::warn!("Failed to obtain default route interface address");
                    match address_family {

The only time that get_up_address_for_interface returns Ok(None) here is when the new default interface has lost internet connection?

Previously we'd have cleared the ST drivers IP addresses but now we don't which means that if the old default interface still has connectivity we'll keep using that for the ST. The reason it is impossible for this to leak inside of the tunnel is that the old default interface can not have been the tunnel interface and the tunnel interface can not change to be the old default interface. The reason this is impossible to leak outside of the tunnel is that this is already leaking outside of the tunnel, that's the point.

Am I understanding correctly? If I am then that sounds reasonable in order to avoid having unnecessary calls to the ST driver.

talpid-core/src/tunnel_state_machine/disconnected_state.rs line 55 at r1 (raw file):

    ) {
        if shared_values.block_when_disconnected {
            if let Err(error) = shared_values.split_tunnel.set_tunnel_addresses(None) {

General question here. Why do we reset the split tunnel when block_when_disconnected is set to true? I'm guessing blocking it would not be what we want since only vpn tunnel traffic should be blocked?

talpid-types/src/tunnel.rs line 120 at r1 (raw file):

    #[cfg(target_os = "windows")]
    pub fn prevents_split_tunneling(&self) -> bool {
        matches!(self, Self::SplitTunnelError | Self::IsOffline)

If the error is SplitTunnelError we will not clear the split tunnel IPs, why? I thought this was because we want to be able to disconnect and reconnect to networks without having to spend time clearing and resetting the ST IPs. If there is an ST error isn't that a different scenario?

dlon

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @Jontified)

talpid-core/src/split_tunnel/windows/mod.rs line 115 at r1 (raw file):