Rework firewall exclusion rules

6 files changed, 56 insertions, 149 deletions

diff --git a/talpid-core/src/firewall/macos.rs b/talpid-core/src/firewall/macos.rs
index cc046bb821..0b18cc1a20 100644
--- a/talpid-core/src/firewall/macos.rs
+++ b/talpid-core/src/firewall/macos.rs
@@ -24,7 +24,7 @@ pub struct Firewall {
     pf: pfctl::PfCtl,
     pf_was_enabled: Option<bool>,
     rule_logging: RuleLogging,
-    exclusion_gid: Option<u32>,
+    exclusion_gid: u32,
 }
 
 impl FirewallT for Firewall {
@@ -176,57 +176,53 @@ impl Firewall {
         &self,
         servers: BTreeSet<IpAddr>,
     ) -> Result<Vec<pfctl::FilterRule>> {
-        if let Some(exclusion_gid) = self.exclusion_gid {
-            let allow_upstream_rules = servers
-                .iter()
-                .flat_map(|addr| {
-                    std::array::IntoIter::new([
-                        self.create_rule_builder(FilterRuleAction::Pass)
-                            .direction(pfctl::Direction::Out)
-                            .quick(true)
-                            .proto(pfctl::Proto::Tcp)
-                            .keep_state(pfctl::StatePolicy::Keep)
-                            .tcp_flags(Self::get_tcp_flags())
-                            .to(pfctl::Endpoint::new(*addr, 53))
-                            .group(exclusion_gid)
-                            .build(),
-                        self.create_rule_builder(FilterRuleAction::Pass)
-                            .direction(pfctl::Direction::Out)
-                            .quick(true)
-                            .proto(pfctl::Proto::Udp)
-                            .keep_state(pfctl::StatePolicy::Keep)
-                            .to(pfctl::Endpoint::new(*addr, 53))
-                            .group(exclusion_gid)
-                            .build(),
-                    ])
-                })
-                .collect::<Result<Vec<_>>>();
+        let allow_upstream_rules = servers
+            .iter()
+            .flat_map(|addr| {
+                std::array::IntoIter::new([
+                    self.create_rule_builder(FilterRuleAction::Pass)
+                        .direction(pfctl::Direction::Out)
+                        .quick(true)
+                        .proto(pfctl::Proto::Tcp)
+                        .keep_state(pfctl::StatePolicy::Keep)
+                        .tcp_flags(Self::get_tcp_flags())
+                        .to(pfctl::Endpoint::new(*addr, 53))
+                        .group(self.exclusion_gid)
+                        .build(),
+                    self.create_rule_builder(FilterRuleAction::Pass)
+                        .direction(pfctl::Direction::Out)
+                        .quick(true)
+                        .proto(pfctl::Proto::Udp)
+                        .keep_state(pfctl::StatePolicy::Keep)
+                        .to(pfctl::Endpoint::new(*addr, 53))
+                        .group(self.exclusion_gid)
+                        .build(),
+                ])
+            })
+            .collect::<Result<Vec<_>>>();
 
-            allow_upstream_rules.and_then(|mut rules| {
-                if !rules.is_empty() {
-                    rules.push(
-                        self.create_rule_builder(FilterRuleAction::Pass)
-                            .quick(true)
-                            .proto(pfctl::Proto::Udp)
-                            .to(pfctl::Endpoint::new(Ipv4Addr::LOCALHOST, 53))
-                            .build()?,
-                    );
-                    rules.push(
-                        self.create_rule_builder(FilterRuleAction::Pass)
-                            .quick(true)
-                            .proto(pfctl::Proto::Tcp)
-                            .keep_state(pfctl::StatePolicy::Keep)
-                            .tcp_flags(Self::get_tcp_flags())
-                            .to(pfctl::Endpoint::new(Ipv4Addr::LOCALHOST, 53))
-                            .build()?,
-                    );
-                }
+        allow_upstream_rules.and_then(|mut rules| {
+            if !rules.is_empty() {
+                rules.push(
+                    self.create_rule_builder(FilterRuleAction::Pass)
+                        .quick(true)
+                        .proto(pfctl::Proto::Udp)
+                        .to(pfctl::Endpoint::new(Ipv4Addr::LOCALHOST, 53))
+                        .build()?,
+                );
+                rules.push(
+                    self.create_rule_builder(FilterRuleAction::Pass)
+                        .quick(true)
+                        .proto(pfctl::Proto::Tcp)
+                        .keep_state(pfctl::StatePolicy::Keep)
+                        .tcp_flags(Self::get_tcp_flags())
+                        .to(pfctl::Endpoint::new(Ipv4Addr::LOCALHOST, 53))
+                        .build()?,
+                );
+            }
 
-                Ok(rules)
-            })
-        } else {
-            Ok(vec![])
-        }
+            Ok(rules)
+        })
     }
 
     fn get_allow_dns_rules_when_connected(
diff --git a/talpid-core/src/firewall/mod.rs b/talpid-core/src/firewall/mod.rs
index d981ff027a..814bd405c3 100644
--- a/talpid-core/src/firewall/mod.rs
+++ b/talpid-core/src/firewall/mod.rs
@@ -230,7 +230,7 @@ pub struct FirewallArguments {
     #[cfg(target_os = "macos")]
     /// This argument is required on macOS to know which group's traffic should be excluded, if at
     /// all.
-    pub exclusion_gid: Option<u32>,
+    pub exclusion_gid: u32,
 }
 
 /// State to enter during firewall init.
diff --git a/talpid-core/src/lib.rs b/talpid-core/src/lib.rs
index 38cffec488..73c3293fb4 100644
--- a/talpid-core/src/lib.rs
+++ b/talpid-core/src/lib.rs
@@ -63,10 +63,6 @@ mod mktemp;
 #[cfg(target_os = "linux")]
 mod linux;
 
-/// Misc utilities for the macOS platform.
-#[cfg(target_os = "macos")]
-pub mod macos;
-
 /// A pair of functions to monitor and establish connectivity with ICMP
 pub mod ping_monitor;
 
diff --git a/talpid-core/src/macos.rs b/talpid-core/src/macos.rs
deleted file mode 100644
index 49f9ee6b2f..0000000000
--- a/talpid-core/src/macos.rs
+++ /dev/null
@@ -1,67 +0,0 @@
-use std::{ffi::CStr, io};
-
-/// Returns the GID of the specified group name
-pub fn get_group_id(group_name: &CStr) -> io::Result<u32> {
-    // SAFETY: group_name is a valid CString
-    let group = unsafe { libc::getgrnam(group_name.as_ptr() as *const _) };
-    if group.is_null() {
-        return Err(io::Error::from(io::ErrorKind::NotFound));
-    }
-    // SAFETY: group is not null
-    let gid = unsafe { (*group).gr_gid };
-    Ok(gid)
-}
-
-/// Sets group ID for the current process
-pub fn set_gid(gid: u32) -> io::Result<()> {
-    if unsafe { libc::setgid(gid) } == 0 {
-        Ok(())
-    } else {
-        Err(io::Error::last_os_error())
-    }
-}
-
-const INCREASED_FILEHANDLE_LIMIT: u64 = 1024;
-/// Bump filehandle limit
-pub fn bump_filehandle_limit() {
-    let mut limits = libc::rlimit {
-        rlim_cur: 0,
-        rlim_max: 0,
-    };
-    // SAFETY: `&mut limits` is a valid pointer parameter for the getrlimit syscall
-    let status = unsafe { libc::getrlimit(libc::RLIMIT_NOFILE, &mut limits) };
-    if status != 0 {
-        log::error!(
-            "Failed to get file handle limits: {}-{}",
-            io::Error::from_raw_os_error(status),
-            status
-        );
-        return;
-    }
-
-    // if file handle limit is already big enough, there's no reason to decrease it.
-    if limits.rlim_cur >= INCREASED_FILEHANDLE_LIMIT {
-        return;
-    }
-
-    limits.rlim_cur = INCREASED_FILEHANDLE_LIMIT;
-    // SAFETY: `&limits` is a valid pointer parameter for the getrlimit syscall
-    let status = unsafe { libc::setrlimit(libc::RLIMIT_NOFILE, &limits as *const _) };
-    if status != 0 {
-        log::error!(
-            "Failed to set file handle limit to {}: {}-{}",
-            INCREASED_FILEHANDLE_LIMIT,
-            io::Error::from_raw_os_error(status),
-            status
-        );
-    }
-}
-
-#[cfg(test)]
-#[test]
-fn test_unknown_group() {
-    let unknown_group = CStr::from_bytes_with_nul(b"asdunknown\0").unwrap();
-    let group_err = get_group_id(unknown_group).unwrap_err();
-    assert!(group_err.kind() == io::ErrorKind::NotFound)
-
-}
diff --git a/talpid-core/src/resolver/mod.rs b/talpid-core/src/resolver/mod.rs
index fa026a48f3..6a52d6771f 100644
--- a/talpid-core/src/resolver/mod.rs
+++ b/talpid-core/src/resolver/mod.rs
@@ -61,35 +61,14 @@ pub(crate) async fn start_resolver(sender: TunnelCommandSender) -> Result<Resolv
 }
 
 async fn start_resolver_inner(
-    sender: TunnelCommandSender,
+    tunnel_tx: TunnelCommandSender,
     port: u16,
 ) -> Result<ResolverHandle, Error> {
-    let (tx, rx) = oneshot::channel();
-    std::thread::spawn(move || run_resolver(sender, tx, port));
-    rx.await.map_err(|_| Error::LauncherThreadPanic)?
+    let (resolver, resolver_handle) = FilteringResolver::new(tunnel_tx, port).await?;
+    tokio::spawn(resolver.run());
+    Ok(resolver_handle)
 }
 
-async fn run_resolver(
-    tunnel_tx: TunnelCommandSender,
-    done_tx: oneshot::Sender<Result<ResolverHandle, Error>>,
-    port: u16,
-) {
-    let mut builder = tokio::runtime::Builder::new_multi_thread();
-    builder.enable_all();
-    builder.worker_threads(2);
-    builder.max_blocking_threads(1);
-
-    let rt = builder.build().expect("failed to initialize tokio runtime");
-    match rt.block_on(FilteringResolver::new(tunnel_tx, port)) {
-        Ok((resolver, resolver_handle)) => {
-            let _ = done_tx.send(Ok(resolver_handle));
-            rt.block_on(resolver.run());
-        }
-        Err(err) => {
-            let _ = done_tx.send(Err(err));
-        }
-    }
-}
 /// Resolver errors
 #[derive(err_derive::Error, Debug)]
 #[error(no_from)]
@@ -244,6 +223,7 @@ impl FilteringResolver {
                     match resolver_state {
                         ResolverState::Shutdown => {
                             self.stop_server().await;
+                            self.resolver_state = ResolverState::Shutdown;
                         }
                         running_state => {
                             if self.dns_server.is_none() {
@@ -799,6 +779,8 @@ mod test {
         rt.block_on(async { handle.shutdown().await })
             .expect("failed to make resovler active");
 
+        // macOS takes it sweet time reaping the socket
+        std::thread::sleep(std::time::Duration::from_millis(300));
         UdpSocket::bind(server_sockaddr)
             .expect("Failed to bind to resolver socket addr when it should be unbound");
     }
diff --git a/talpid-core/src/tunnel_state_machine/mod.rs b/talpid-core/src/tunnel_state_machine/mod.rs
index 8b44f39dad..b222a5caa6 100644
--- a/talpid-core/src/tunnel_state_machine/mod.rs
+++ b/talpid-core/src/tunnel_state_machine/mod.rs
@@ -109,7 +109,7 @@ pub async fn spawn(
     state_change_listener: impl Sender<TunnelStateTransition> + Send + 'static,
     offline_state_listener: mpsc::UnboundedSender<bool>,
     shutdown_tx: oneshot::Sender<()>,
-    #[cfg(target_os = "macos")] exclusion_gid: Option<u32>,
+    #[cfg(target_os = "macos")] exclusion_gid: u32,
     #[cfg(target_os = "macos")] enable_resolver: bool,
     #[cfg(target_os = "android")] android_context: AndroidContext,
 ) -> Result<Arc<mpsc::UnboundedSender<TunnelCommand>>, Error> {
@@ -225,7 +225,7 @@ impl TunnelStateMachine {
         log_dir: Option<PathBuf>,
         resource_dir: PathBuf,
         commands_rx: mpsc::UnboundedReceiver<TunnelCommand>,
-        #[cfg(target_os = "macos")] exclusion_gid: Option<u32>,
+        #[cfg(target_os = "macos")] exclusion_gid: u32,
         #[cfg(target_os = "macos")] enable_resolver: bool,
         #[cfg(target_os = "android")] android_context: AndroidContext,
     ) -> Result<Self, Error> {