Merge branch 'macos-delete-ifscope-routes' into main

4 files changed, 213 insertions, 222 deletions

diff --git a/talpid-routing/Cargo.toml b/talpid-routing/Cargo.toml
index 2231829475..50bd4c1477 100644
--- a/talpid-routing/Cargo.toml
+++ b/talpid-routing/Cargo.toml
@@ -14,7 +14,7 @@ err-derive = { workspace = true }
 futures = "0.3.15"
 ipnetwork = "0.16"
 log = { workspace = true }
-tokio = { workspace = true, features = ["process", "rt-multi-thread", "net"] }
+tokio = { workspace = true, features = ["process", "rt-multi-thread", "net", "io-util", "time"] }
 
 [target.'cfg(not(target_os="android"))'.dependencies]
 talpid-types = { path = "../talpid-types" }
diff --git a/talpid-routing/src/unix/macos/data.rs b/talpid-routing/src/unix/macos/data.rs
index 16981ee5f6..f0be4186bc 100644
--- a/talpid-routing/src/unix/macos/data.rs
+++ b/talpid-routing/src/unix/macos/data.rs
@@ -321,6 +321,11 @@ impl RouteMessage {
         self.interface_index
     }
 
+    pub fn set_interface_index(mut self, index: u16) -> Self {
+        self.interface_index = index;
+        self
+    }
+
     pub fn interface_address(&self) -> Option<IpAddr> {
         self.get_address(&AddressFlag::RTA_IFA)
     }
diff --git a/talpid-routing/src/unix/macos/interface.rs b/talpid-routing/src/unix/macos/interface.rs
index 761c0f870b..85b76377d4 100644
--- a/talpid-routing/src/unix/macos/interface.rs
+++ b/talpid-routing/src/unix/macos/interface.rs
@@ -1,21 +1,30 @@
 use ipnetwork::IpNetwork;
-use libc::{if_indextoname, IFNAMSIZ};
-use nix::net::if_::{if_nametoindex, InterfaceFlags};
+use nix::{
+    net::if_::{if_nametoindex, InterfaceFlags},
+    sys::socket::{AddressFamily, SockaddrLike, SockaddrStorage},
+};
 use std::{
-    ffi::{CStr, CString},
+    collections::BTreeMap,
     io,
-    net::{Ipv4Addr, Ipv6Addr},
+    net::{IpAddr, Ipv4Addr, Ipv6Addr},
 };
+
 use system_configuration::{
-    core_foundation::string::CFString,
-    network_configuration::{SCNetworkService, SCNetworkSet},
+    core_foundation::{
+        base::{CFType, TCFType, ToVoid},
+        dictionary::CFDictionary,
+        string::CFString,
+    },
+    dynamic_store::SCDynamicStoreBuilder,
+    network_configuration::SCNetworkSet,
     preferences::SCPreferences,
+    sys::schema_definitions::{
+        kSCDynamicStorePropNetPrimaryInterface, kSCPropInterfaceName, kSCPropNetIPv4Router,
+        kSCPropNetIPv6Router,
+    },
 };
 
-use super::{
-    data::{Destination, RouteMessage},
-    watch::RoutingTable,
-};
+use super::data::{Destination, RouteMessage};
 
 #[derive(Debug, PartialEq, Clone, Copy)]
 pub enum Family {
@@ -41,48 +50,6 @@ impl From<Family> for IpNetwork {
     }
 }
 
-/// Retrieve the current unscoped default route. That is the only default route that does not have
-/// the IF_SCOPE flag set, if such a route exists.
-///
-/// # Note
-///
-/// For some reason, the socket sometimes returns a route with the IF_SCOPE flag set, if there also
-/// exists a scoped route for the same interface. This does not occur if there is no unscoped route,
-/// so we can still rely on it.
-pub async fn get_unscoped_default_route(
-    routing_table: &mut RoutingTable,
-    family: Family,
-) -> Option<RouteMessage> {
-    let mut msg = RouteMessage::new_route(Destination::Network(IpNetwork::from(family)));
-    msg = msg.set_gateway_route(true);
-
-    let route = routing_table
-        .get_route(&msg)
-        .await
-        .unwrap_or_else(|error| {
-            log::error!("Failed to retrieve unscoped default route: {error}");
-            None
-        })?;
-
-    let idx = u32::from(route.interface_index());
-    if idx != 0 {
-        let mut ifname = [0u8; IFNAMSIZ];
-
-        // SAFETY: The buffer is large to contain any interface name.
-        if !unsafe { if_indextoname(idx, ifname.as_mut_ptr() as _) }.is_null() {
-            let ifname = CStr::from_bytes_until_nul(&ifname).unwrap();
-            let name = ifname.to_str().expect("expected ascii");
-
-            // Ignore the unscoped route if its interface is not "active"
-            if !is_active_interface(name, family).unwrap_or(true) {
-                return None;
-            }
-        }
-    }
-
-    Some(route)
-}
-
 /// Retrieve the best current default route. That is the first scoped default route, ordered by
 /// network service order, and with interfaces filtered out if they do not have valid IP addresses
 /// assigned.
@@ -90,56 +57,113 @@ pub async fn get_unscoped_default_route(
 /// # Note
 ///
 /// The tunnel interface is not even listed in the service order, so it will be skipped.
-pub async fn get_best_default_route(
-    routing_table: &mut RoutingTable,
-    family: Family,
-) -> Option<RouteMessage> {
-    let mut msg = RouteMessage::new_route(Destination::Network(IpNetwork::from(family)));
-    msg = msg.set_gateway_route(true);
-
-    for iface in network_service_order() {
-        let iface_bytes = match CString::new(iface.as_bytes()) {
-            Ok(name) => name,
-            Err(error) => {
-                log::error!("Invalid interface name: {iface}, {error}");
-                continue;
-            }
-        };
-
-        // Get interface ID
-        let index = match if_nametoindex(iface_bytes.as_c_str()) {
-            Ok(index) => index,
-            Err(_error) => {
-                continue;
-            }
+pub fn get_best_default_route(family: Family) -> Option<RouteMessage> {
+    for iface in network_service_order(family) {
+        let Ok(index) = if_nametoindex(iface.name.as_str()) else {
+            continue;
         };
 
         // Request ifscoped default route for this interface
-        let route_msg = msg.clone().set_ifscope(u16::try_from(index).unwrap());
-        if let Ok(Some(route)) = routing_table.get_route(&route_msg).await {
-            if is_active_interface(&iface, family).unwrap_or(true) {
-                return Some(route);
-            }
+        let msg = RouteMessage::new_route(Destination::Network(IpNetwork::from(family)))
+            .set_gateway_addr(iface.router_ip)
+            .set_interface_index(u16::try_from(index).unwrap());
+        let active = is_active_interface(&iface.name, family).unwrap_or_else(|error| {
+            log::error!("is_active_interface() returned an error for interface \"{}\", assuming active. Error: {error}", iface.name);
+            true
+        });
+        if active {
+            return Some(msg);
         }
     }
 
     None
 }
 
-fn network_service_order() -> Vec<String> {
+/// Return a map from interface name to link addresses (AF_LINK)
+pub fn get_interface_link_addresses() -> io::Result<BTreeMap<String, SockaddrStorage>> {
+    let mut gateway_link_addrs = BTreeMap::new();
+    let addrs = nix::ifaddrs::getifaddrs()?;
+    for addr in addrs.into_iter() {
+        if addr.address.and_then(|addr| addr.family()) != Some(AddressFamily::Link) {
+            continue;
+        }
+        gateway_link_addrs.insert(addr.interface_name, addr.address.unwrap());
+    }
+    Ok(gateway_link_addrs)
+}
+
+struct NetworkServiceDetails {
+    name: String,
+    router_ip: IpAddr,
+}
+
+fn network_service_order(family: Family) -> Vec<NetworkServiceDetails> {
     let prefs = SCPreferences::default(&CFString::new("talpid-routing"));
-    let services = SCNetworkService::get_services(&prefs);
     let set = SCNetworkSet::new(&prefs);
     let service_order = set.service_order();
+    let store = SCDynamicStoreBuilder::new("talpid-routing").build();
+
+    let global_dict = if family == Family::V4 {
+        "State:/Network/Global/IPv4"
+    } else {
+        "State:/Network/Global/IPv6"
+    };
+    let global_dict = store
+        .get(CFString::new(global_dict))
+        .and_then(|v| v.downcast_into::<CFDictionary>());
+    let primary_interface = if let Some(ref dict) = global_dict {
+        dict.find(unsafe { kSCDynamicStorePropNetPrimaryInterface }.to_void())
+            .map(|s| unsafe { CFType::wrap_under_get_rule(*s) })
+            .and_then(|s| s.downcast::<CFString>())
+            .map(|s| s.to_string())
+    } else {
+        None
+    };
+
+    let router_key = if family == Family::V4 {
+        unsafe { kSCPropNetIPv4Router.to_void() }
+    } else {
+        unsafe { kSCPropNetIPv6Router.to_void() }
+    };
 
     service_order
         .iter()
         .filter_map(|service_id| {
-            services
-                .iter()
-                .find(|service| service.id().as_ref() == Some(&*service_id))
-                .and_then(|service| service.network_interface()?.bsd_name())
-                .map(|cf_name| cf_name.to_string())
+            let service_id_s = service_id.to_string();
+            let key = if family == Family::V4 {
+                format!("State:/Network/Service/{service_id_s}/IPv4")
+            } else {
+                format!("State:/Network/Service/{service_id_s}/IPv6")
+            };
+
+            let ip_dict = store
+                .get(CFString::new(&key))
+                .and_then(|v| v.downcast_into::<CFDictionary>())?;
+            let name = ip_dict
+                .find(unsafe { kSCPropInterfaceName }.to_void())
+                .map(|s| unsafe { CFType::wrap_under_get_rule(*s) })
+                .and_then(|s| s.downcast::<CFString>())
+                .map(|s| s.to_string())?;
+            let router_ip = ip_dict
+                .find(router_key)
+                .map(|s| unsafe { CFType::wrap_under_get_rule(*s) })
+                .and_then(|s| s.downcast::<CFString>())
+                .and_then(|ip| ip.to_string().parse().ok())
+                .or_else(|| {
+                    if Some(&name) != primary_interface.as_ref() {
+                        return None;
+                    }
+                    let Some(ref dict) = global_dict else {
+                        return None;
+                    };
+                    // Sometimes only the primary interface contains the router IPv6 addr
+                    dict.find(router_key)
+                        .map(|s| unsafe { CFType::wrap_under_get_rule(*s) })
+                        .and_then(|s| s.downcast::<CFString>())
+                        .and_then(|ip| ip.to_string().parse().ok())
+                })?;
+
+            Some(NetworkServiceDetails { name, router_ip })
         })
         .collect::<Vec<_>>()
 }
diff --git a/talpid-routing/src/unix/macos/mod.rs b/talpid-routing/src/unix/macos/mod.rs
index 7aceeba78f..8cca3594f9 100644
--- a/talpid-routing/src/unix/macos/mod.rs
+++ b/talpid-routing/src/unix/macos/mod.rs
@@ -6,12 +6,12 @@ use futures::{
     stream::{FusedStream, StreamExt},
 };
 use ipnetwork::IpNetwork;
-use nix::sys::socket::{AddressFamily, SockaddrLike, SockaddrStorage};
+use std::sync::Weak;
 use std::{
     collections::{BTreeMap, HashSet},
+    pin::Pin,
     time::Duration,
 };
-use std::{pin::Pin, sync::Weak};
 use talpid_types::ErrorExt;
 use watch::RoutingTable;
 
@@ -43,15 +43,11 @@ pub enum Error {
 
     /// Failed to fetch link addresses
     #[error(display = "Failed to fetch link addresses")]
-    FetchLinkAddresses(nix::Error),
+    FetchLinkAddresses(#[error(source)] std::io::Error),
 
     /// Received message isn't valid
     #[error(display = "Invalid data")]
     InvalidData(data::Error),
-
-    /// Restoring unscoped default routes
-    #[error(display = "Restoring unscoped default routes")]
-    RestoringUnscopedRoutes,
 }
 
 /// Convenience macro to get the current default route. Macro because I don't want to borrow `self`
@@ -123,20 +119,20 @@ impl RouteManagerImpl {
         // Initialize default routes
         // NOTE: This isn't race-free, as we're not listening for route changes before initializing
         self.update_best_default_route(interface::Family::V4)
-            .await
             .unwrap_or_else(|error| {
                 log::error!(
                     "{}",
                     error.display_chain_with_msg("Failed to get initial default v4 route")
                 );
+                false
             });
         self.update_best_default_route(interface::Family::V6)
-            .await
             .unwrap_or_else(|error| {
                 log::error!(
                     "{}",
                     error.display_chain_with_msg("Failed to get initial default v6 route")
                 );
+                false
             });
 
         let mut completion_tx = None;
@@ -198,23 +194,14 @@ impl RouteManagerImpl {
 
                         Some(RouteManagerCommand::AddRoutes(routes, tx)) => {
                             if !self.check_default_routes_restored.is_terminated() {
-                                // Give it some time to recover, but not too much
-                                if !self.try_restore_default_routes().await {
-                                    let _ = tokio::time::timeout(
-                                        Duration::from_millis(500),
-                                        self.check_default_routes_restored.next(),
-                                    ).await;
-
-                                    if !self.try_restore_default_routes().await {
-                                        log::warn!("Unscoped routes were not restored");
-                                        let _ = tx.send(Err(Error::RestoringUnscopedRoutes));
-                                        continue;
-                                    }
-                                }
+                                log::debug!("Cancelling restoration of default routes");
                                 self.check_default_routes_restored = Box::pin(futures::stream::pending());
-                                log::debug!("Unscoped routes were already restored");
                             }
 
+                            // Reset known best route
+                            let _ = self.update_best_default_route(interface::Family::V4);
+                            let _ = self.update_best_default_route(interface::Family::V6);
+
                             log::debug!("Adding routes: {routes:?}");
                             let _ = tx.send(self.add_required_routes(routes).await);
                         }
@@ -261,7 +248,8 @@ impl RouteManagerImpl {
         }
 
         // Map all interfaces to their link addresses
-        let interface_link_addrs = get_interface_link_addresses()?;
+        let interface_link_addrs =
+            interface::get_interface_link_addresses().map_err(Error::FetchLinkAddresses)?;
 
         // Add routes not using the default interface
         for route in routes_to_apply {
@@ -313,8 +301,7 @@ impl RouteManagerImpl {
     ) {
         match message {
             Ok(RouteSocketMessage::DeleteRoute(route)) => {
-                // Forget about applied route, if relevant. This is simply prevent ourselves from
-                // deleting it later.
+                // Forget about applied route, if relevant
                 match RouteDestination::try_from(&route).map_err(Error::InvalidData) {
                     Ok(destination) => {
                         self.applied_routes.remove(&destination);
@@ -351,10 +338,14 @@ impl RouteManagerImpl {
     ///   server. The gateway of the relay route is set to the first interface in the network
     ///   service order that has a working ifscoped default route.
     async fn refresh_routes(&mut self) -> Result<()> {
-        self.update_best_default_route(interface::Family::V4)
-            .await?;
-        self.update_best_default_route(interface::Family::V6)
-            .await?;
+        self.update_best_default_route(interface::Family::V4)?;
+        self.update_best_default_route(interface::Family::V6)?;
+
+        // Remove any existing ifscope route that we've added
+        self.remove_applied_routes(|route| {
+            route.is_ifscope() && route.is_default().unwrap_or(false)
+        })
+        .await;
 
         // Substitute route with a tunnel route
         self.apply_tunnel_default_route().await?;
@@ -367,46 +358,31 @@ impl RouteManagerImpl {
     /// subscribers. The "best routes" are used by the tunnel device to send packets to the VPN
     /// relay.
     ///
-    /// If there is a tunnel device, the "best route" is the first ifscope default route found,
-    /// ordered after network service order (after filtering out interfaces without valid IP
-    /// addresses).
+    /// The "best route" is determined by the first interface in the network service order that has
+    /// a valid IP address and gateway.
     ///
-    /// If there is no tunnel device, the "best route" is the unscoped default route, whatever it
-    /// is.
-    async fn update_best_default_route(&mut self, family: interface::Family) -> Result<()> {
-        let use_scoped_route = (family == interface::Family::V4
-            && self.v4_tunnel_default_route.is_some())
-            || (family == interface::Family::V6 && self.v6_tunnel_default_route.is_some());
+    /// On success, the function returns whether the previously known best default changed.
+    fn update_best_default_route(&mut self, family: interface::Family) -> Result<bool> {
+        let best_route = interface::get_best_default_route(family);
+        let current_route = get_current_best_default_route!(self, family);
 
-        let best_route = if use_scoped_route {
-            interface::get_best_default_route(&mut self.routing_table, family).await
-        } else {
-            interface::get_unscoped_default_route(&mut self.routing_table, family).await
-        };
         log::trace!("Best route ({family:?}): {best_route:?}");
-
-        let default_route = get_current_best_default_route!(self, family);
-
-        if default_route == &best_route {
-            log::trace!("Default route ({family:?}) is unchanged");
-            return Ok(());
+        if best_route == *current_route {
+            return Ok(false);
         }
 
-        let old_route = std::mem::replace(default_route, best_route);
-
-        log::debug!(
-            "Default route change ({family:?}): interface {} -> {}",
-            old_route.map(|r| r.interface_index()).unwrap_or(0),
-            default_route
-                .as_ref()
-                .map(|r| r.interface_index())
-                .unwrap_or(0),
-        );
+        let old_pair = current_route
+            .as_ref()
+            .map(|r| (r.interface_index(), r.gateway_ip()));
+        let new_pair = best_route
+            .as_ref()
+            .map(|r| (r.interface_index(), r.gateway_ip()));
+        log::debug!("Best default route changed from {old_pair:?} to {new_pair:?}");
+        let _ = std::mem::replace(current_route, best_route);
 
-        let changed = default_route.is_some();
+        let changed = current_route.is_some();
         self.notify_default_route_listeners(family, changed);
-
-        Ok(())
+        Ok(true)
     }
 
     fn notify_default_route_listeners(&mut self, family: interface::Family, changed: bool) {
@@ -456,7 +432,7 @@ impl RouteManagerImpl {
             };
 
             // Replace the default route with an ifscope route
-            self.set_default_route_ifscope(family, true).await?;
+            self.replace_with_scoped_route(family).await?;
 
             // Make sure there is really no other unscoped default route
             let mut msg = RouteMessage::new_route(IpNetwork::from(family).into());
@@ -529,45 +505,18 @@ impl RouteManagerImpl {
         Ok(())
     }
 
-    /// Replace a known default route with an ifscope route, if should_be_ifscoped is true.
-    /// If should_be_ifscoped is false, the route is replaced with a non-ifscoped default route
-    /// instead.
-    async fn set_default_route_ifscope(
-        &mut self,
-        family: interface::Family,
-        should_be_ifscoped: bool,
-    ) -> Result<()> {
+    /// Replace a known default route with an ifscope route.
+    async fn replace_with_scoped_route(&mut self, family: interface::Family) -> Result<()> {
         let Some(default_route) = get_current_best_default_route!(self, family) else {
             return Ok(());
         };
 
-        if default_route.is_ifscope() == should_be_ifscoped {
-            return Ok(());
-        }
-
-        log::trace!("Setting non-ifscope: {default_route:?}");
-
-        let interface_index = if should_be_ifscoped {
-            let interface_index = default_route.interface_index();
-            if interface_index == 0 {
-                log::error!("Cannot find interface index of default interface");
-            }
-            interface_index
-        } else {
-            0
-        };
+        let interface_index = default_route.interface_index();
         let new_route = default_route.clone().set_ifscope(interface_index);
-        let old_route = std::mem::replace(default_route, new_route);
 
-        self.routing_table
-            .delete_route(&old_route)
-            .await
-            .map_err(Error::DeleteRoute)?;
+        log::trace!("Setting ifscope: {new_route:?}");
 
-        self.routing_table
-            .add_route(default_route)
-            .await
-            .map_err(Error::AddRoute)
+        self.add_route_with_record(new_route).await
     }
 
     async fn add_route_with_record(&mut self, route: RouteMessage) -> Result<()> {
@@ -583,17 +532,7 @@ impl RouteManagerImpl {
     }
 
     async fn cleanup_routes(&mut self) -> Result<()> {
-        // Remove all applied routes. This includes default destination routes
-        let old_routes = std::mem::take(&mut self.applied_routes);
-        for (_dest, route) in old_routes.into_iter() {
-            log::trace!("Removing route: {route:?}");
-            match self.routing_table.delete_route(&route).await {
-                Ok(_) | Err(watch::Error::RouteNotFound) | Err(watch::Error::Unreachable) => (),
-                Err(err) => {
-                    log::error!("Failed to remove relay route: {err:?}");
-                }
-            }
-        }
+        self.remove_applied_routes(|_| true).await;
 
         // We have already removed the applied default routes
         self.v4_tunnel_default_route = None;
@@ -608,6 +547,29 @@ impl RouteManagerImpl {
         Ok(())
     }
 
+    /// Remove all applied routes for which `filter` returns true
+    async fn remove_applied_routes(&mut self, filter: impl Fn(&RouteMessage) -> bool) {
+        let mut deleted_routes = vec![];
+
+        self.applied_routes.retain(|_dest, route| {
+            if filter(route) {
+                deleted_routes.push(route.clone());
+                return false;
+            }
+            true
+        });
+
+        for route in deleted_routes {
+            log::trace!("Removing route: {route:?}");
+            match self.routing_table.delete_route(&route).await {
+                Ok(_) | Err(watch::Error::RouteNotFound) | Err(watch::Error::Unreachable) => (),
+                Err(err) => {
+                    log::error!("Failed to remove relay route: {err:?}");
+                }
+            }
+        }
+    }
+
     /// FIXME: Hack. Restoring the default routes during cleanup sometimes fails, so repeatedly try
     /// until we have restored unscoped default routes. This function produces a timer for
     /// exponential backoff.
@@ -639,44 +601,44 @@ impl RouteManagerImpl {
     /// Add back unscoped default route for the given `family`, if it is still missing. This
     /// function returns true when no route had to be added.
     async fn restore_default_route(&mut self, family: interface::Family) -> bool {
-        let current_route = get_current_best_default_route!(self, family);
-        let message = RouteMessage::new_route(IpNetwork::from(family).into());
-        if matches!(self.routing_table.get_route(&message).await, Ok(Some(_))) {
+        let Some(desired_default_route) = interface::get_best_default_route(family) else {
             return true;
-        }
-
-        let new_route = interface::get_best_default_route(&mut self.routing_table, family).await;
-        let old_route = std::mem::replace(current_route, new_route);
-        let notify = &old_route != current_route;
+        };
 
-        let done = if let Some(route) = current_route {
-            *route = route.clone().set_ifscope(0);
-            if let Err(error) = self.routing_table.add_route(route).await {
-                log::trace!("Failed to add non-ifscope {family} route: {error}");
+        let current_default_route = RouteMessage::new_route(IpNetwork::from(family).into());
+        if let Ok(Some(current_default)) =
+            self.routing_table.get_route(&current_default_route).await
+        {
+            // We're done if the route we're looking for is already here
+            if route_matches_interface(Some(&current_default), Some(&desired_default_route)) {
+                return true;
             }
-            false
-        } else {
-            true
+            let _ = self
+                .routing_table
+                .delete_route(&current_default_route)
+                .await;
         };
 
-        if notify {
-            let changed = current_route.is_some();
-            self.notify_default_route_listeners(family, changed);
+        if let Err(error) = self.routing_table.add_route(&desired_default_route).await {
+            log::trace!("Failed to add unscoped default {family} route: {error}");
         }
 
-        done
+        self.update_trigger.trigger();
+
+        false
     }
 }
 
-/// Return a map from interface name to link addresses (AF_LINK)
-fn get_interface_link_addresses() -> Result<BTreeMap<String, SockaddrStorage>> {
-    let mut gateway_link_addrs = BTreeMap::new();
-    let addrs = nix::ifaddrs::getifaddrs().map_err(Error::FetchLinkAddresses)?;
-    for addr in addrs.into_iter() {
-        if addr.address.and_then(|addr| addr.family()) != Some(AddressFamily::Link) {
-            continue;
+fn route_matches_interface(
+    default_route: Option<&RouteMessage>,
+    interface_route: Option<&RouteMessage>,
+) -> bool {
+    match (default_route, interface_route) {
+        (Some(default_route), Some(interface_route)) => {
+            default_route.gateway_ip() == interface_route.gateway_ip()
+                && default_route.interface_index() == interface_route.interface_index()
         }
-        gateway_link_addrs.insert(addr.interface_name, addr.address.unwrap());
+        (None, None) => true,
+        _ => false,
     }
-    Ok(gateway_link_addrs)
 }