Merge branch 'set-dns-on-macos'

6 files changed, 393 insertions, 117 deletions

diff --git a/talpid-core/src/firewall/macos/dns.rs b/talpid-core/src/firewall/macos/dns.rs
new file mode 100644
index 0000000000..abf12353ab
--- /dev/null
+++ b/talpid-core/src/firewall/macos/dns.rs
@@ -0,0 +1,316 @@
+extern crate core_foundation;
+extern crate system_configuration;
+
+use self::core_foundation::array::{CFArray, CFArrayRef};
+use self::core_foundation::base::{CFType, TCFType};
+use self::core_foundation::dictionary::CFDictionary;
+use self::core_foundation::runloop::{CFRunLoop, kCFRunLoopCommonModes};
+use self::core_foundation::string::{CFString, CFStringRef};
+
+use self::system_configuration::dynamic_store::{SCDynamicStore, SCDynamicStoreBuilder,
+                                                SCDynamicStoreCallBackContext};
+
+use error_chain::ChainedError;
+
+use std::collections::HashMap;
+use std::sync::{mpsc, Arc, Mutex};
+use std::thread;
+
+error_chain! {
+    errors {
+        SettingDnsFailed { description("Error while setting DNS servers") }
+        DynamicStoreInitError { description("Failed to initialize dynamic store") }
+    }
+}
+
+const STATE_PATH_PATTERN: &str = "State:/Network/Service/.*/DNS";
+const SETUP_PATH_PATTERN: &str = "Setup:/Network/Service/.*/DNS";
+
+type ServicePath = String;
+type DnsServer = String;
+
+struct State {
+    desired_dns: Vec<DnsServer>,
+    backup: HashMap<ServicePath, Option<Vec<DnsServer>>>,
+}
+
+pub struct DnsMonitor {
+    store: SCDynamicStore,
+
+    /// The current DNS injection state. If this is `None` it means we are not injecting any DNS.
+    /// When it's `Some(state)` we are actively making sure `state.desired_dns` is configured
+    /// on all network interfaces.
+    state: Arc<Mutex<Option<State>>>,
+}
+
+impl DnsMonitor {
+    /// Creates and returns a new `DnsMonitor`. This spawns a background thread that will monitor
+    /// DNS settings for all network interfaces. If any changes occur it will instantly reset
+    /// the DNS settings for that interface back to the last server list set to this instance
+    /// with `set_dns`.
+    pub fn new() -> Result<Self> {
+        let state = Arc::new(Mutex::new(None));
+        Self::spawn(state.clone())?;
+        Ok(DnsMonitor {
+            store: SCDynamicStoreBuilder::new("mullvad-dns").build(),
+            state,
+        })
+    }
+
+    /// Spawns the background thread running the CoreFoundation main loop and monitors the system
+    /// for DNS changes.
+    fn spawn(state: Arc<Mutex<Option<State>>>) -> Result<()> {
+        let (result_tx, result_rx) = mpsc::channel();
+        thread::spawn(move || match create_dynamic_store(state) {
+            Ok(store) => {
+                result_tx.send(Ok(())).unwrap();
+                run_dynamic_store_runloop(store);
+                // TODO(linus): This is critical. Improve later by sending error signal to Daemon
+                error!("Core Foundation main loop exited! It should run forever");
+            }
+            Err(e) => result_tx.send(Err(e)).unwrap(),
+        });
+        result_rx.recv().unwrap()
+    }
+
+    pub fn set_dns(&self, servers: Vec<DnsServer>) -> Result<()> {
+        let mut state_lock = self.state.lock().unwrap();
+        *state_lock = Some(match state_lock.take() {
+            None => {
+                debug!("Setting DNS to [{}]", servers.join(", "));
+                let backup = read_all_dns(&self.store);
+                for service_path in backup.keys() {
+                    set_dns(&self.store, CFString::new(service_path), &servers)?;
+                }
+                State {
+                    desired_dns: servers,
+                    backup,
+                }
+            }
+            Some(state) => if servers != state.desired_dns {
+                debug!("Changing DNS to [{}]", servers.join(", "));
+                for service_path in state.backup.keys() {
+                    set_dns(&self.store, CFString::new(service_path), &servers)?;
+                }
+                State {
+                    desired_dns: servers,
+                    backup: state.backup,
+                }
+            } else {
+                debug!("No change, new DNS same as the one already set");
+                state
+            },
+        });
+        Ok(())
+    }
+
+    /// Reset all DNS settings to the latest backed up values.
+    pub fn reset(&self) -> Result<()> {
+        let mut state_lock = self.state.lock().unwrap();
+        if let Some(state) = state_lock.take() {
+            for (service_path, servers) in state.backup {
+                if let Some(servers) = servers {
+                    set_dns(&self.store, CFString::new(&service_path), &servers)?;
+                } else {
+                    debug!("Removing DNS for {}", service_path);
+                    if !self.store.remove(CFString::new(&service_path)) {
+                        bail!(ErrorKind::SettingDnsFailed);
+                    }
+                }
+            }
+        }
+        Ok(())
+    }
+}
+
+/// Creates a `SCDynamicStore` that watches all network interfaces for changes to the DNS settings.
+fn create_dynamic_store(state: Arc<Mutex<Option<State>>>) -> Result<SCDynamicStore> {
+    let callback_context = SCDynamicStoreCallBackContext {
+        callout: dns_change_callback,
+        info: state,
+    };
+
+    let store = SCDynamicStoreBuilder::new("mullvad-dns-monitor")
+        .callback_context(callback_context)
+        .build();
+
+    let watch_keys: CFArray<CFString> = CFArray::from_CFTypes(&[]);
+    let watch_patterns = CFArray::from_CFTypes(&[
+        CFString::new(STATE_PATH_PATTERN),
+        CFString::new(SETUP_PATH_PATTERN),
+    ]);
+
+    if store.set_notification_keys(&watch_keys, &watch_patterns) {
+        trace!("Registered for dynamic store notifications");
+        Ok(store)
+    } else {
+        bail!(ErrorKind::DynamicStoreInitError)
+    }
+}
+
+fn run_dynamic_store_runloop(store: SCDynamicStore) {
+    let run_loop_source = store.create_run_loop_source();
+    CFRunLoop::get_current().add_source(&run_loop_source, unsafe { kCFRunLoopCommonModes });
+
+    trace!("Entering CFRunLoop");
+    CFRunLoop::run_current();
+}
+
+/// This function is called by the Core Foundation event loop when there is a change to one or more
+/// watched dynamic store values. In our case we watch all DNS settings.
+fn dns_change_callback(
+    store: SCDynamicStore,
+    changed_keys: CFArray<CFString>,
+    state: &mut Arc<Mutex<Option<State>>>,
+) {
+    if let Err(e) = dns_change_callback_internal(store, changed_keys, state) {
+        error!("{}", e.display_chain());
+    }
+}
+
+fn dns_change_callback_internal(
+    store: SCDynamicStore,
+    changed_keys: CFArray<CFString>,
+    state: &mut Arc<Mutex<Option<State>>>,
+) -> Result<()> {
+    let mut state_lock = state.lock().unwrap();
+    match *state_lock {
+        None => {
+            trace!("Not injecting DNS at this time");
+        }
+        Some(ref mut state) => for path_ptr in changed_keys.as_untyped().iter() {
+            let path = unsafe { CFString::wrap_under_get_rule(path_ptr as CFStringRef) };
+            let should_set_dns = match read_dns(&store, path.clone()) {
+                None => {
+                    debug!("Detected DNS removed for {}", path);
+                    state.backup.insert(path.to_string(), None);
+                    true
+                }
+                Some(servers) => if servers != state.desired_dns {
+                    debug!(
+                        "Detected DNS changed to [{}] for {}",
+                        servers.join(", "),
+                        path
+                    );
+                    state.backup.insert(path.to_string(), Some(servers));
+                    true
+                } else {
+                    false
+                },
+            };
+            if should_set_dns {
+                set_dns(&store, path.clone(), &state.desired_dns)
+                    .chain_err(|| format!("Failed changing DNS for {}", path))?;
+                // If we changed a state DNS, also set the corresponding setup DNS.
+                if let Some(setup_path_str) = state_to_setup_path(&path.to_string()) {
+                    let setup_path = CFString::new(&setup_path_str);
+                    if !state.backup.contains_key(&setup_path_str) {
+                        state
+                            .backup
+                            .insert(setup_path_str, read_dns(&store, setup_path.clone()));
+                    }
+                    set_dns(&store, setup_path.clone(), &state.desired_dns)
+                        .chain_err(|| format!("Failed changing DNS for {}", setup_path))?;
+                }
+            }
+        },
+    }
+    Ok(())
+}
+
+/// Set the dynamic store entry at `path` to a dictionary with the given servers under the
+/// "ServerAddresses" key.
+fn set_dns(store: &SCDynamicStore, path: CFString, servers: &[DnsServer]) -> Result<()> {
+    debug!("Setting DNS to [{}] for {}", servers.join(", "), path);
+    let server_addresses_key = CFString::from_static_string("ServerAddresses");
+
+    let cf_string_servers: Vec<CFString> = servers.iter().map(|s| CFString::new(s)).collect();
+    let server_addresses_value = CFArray::from_CFTypes(&cf_string_servers);
+
+    let dns_dictionary =
+        CFDictionary::from_CFType_pairs(&[(server_addresses_key, server_addresses_value)]);
+
+    if store.set(path, &dns_dictionary) {
+        Ok(())
+    } else {
+        bail!(ErrorKind::SettingDnsFailed)
+    }
+}
+
+/// Read all existing DNS settings and return them.
+fn read_all_dns(store: &SCDynamicStore) -> HashMap<ServicePath, Option<Vec<DnsServer>>> {
+    let mut backup = HashMap::new();
+    // Backup all "state" DNS, and all corresponding "setup" DNS even if they don't exist
+    if let Some(paths) = store.get_keys(STATE_PATH_PATTERN) {
+        for path_ptr in paths.as_untyped().iter() {
+            let state_path = unsafe { CFString::wrap_under_get_rule(path_ptr as CFStringRef) };
+            let state_path_str = state_path.to_string();
+            let setup_path_str = state_to_setup_path(&state_path_str).unwrap();
+            let setup_path = CFString::new(&setup_path_str);
+            backup.insert(state_path_str, read_dns(store, state_path));
+            backup.insert(setup_path_str, read_dns(store, setup_path));
+        }
+    }
+    // Backup all "setup" DNS not already covered
+    if let Some(paths) = store.get_keys(SETUP_PATH_PATTERN) {
+        for path_ptr in paths.as_untyped().iter() {
+            let setup_path = unsafe { CFString::wrap_under_get_rule(path_ptr as CFStringRef) };
+            let setup_path_str = setup_path.to_string();
+            if !backup.contains_key(&setup_path_str) {
+                backup.insert(setup_path_str, read_dns(store, setup_path));
+            }
+        }
+    }
+    backup
+}
+
+fn state_to_setup_path(state_path: &str) -> Option<String> {
+    if state_path.starts_with("State:/") {
+        Some(state_path.replacen("State:/", "Setup:/", 1))
+    } else {
+        None
+    }
+}
+
+/// Get DNS settings for a given dynamic store path. Returns `None` If the path does not exist
+/// or does not contain the expected format.
+fn read_dns(store: &SCDynamicStore, path: CFString) -> Option<Vec<DnsServer>> {
+    store
+        .get(path.clone())
+        .and_then(|property_list| property_list.downcast::<_, CFDictionary>())
+        .and_then(|dictionary| {
+            dictionary
+                .find2(&CFString::from_static_string("ServerAddresses"))
+                .map(|array_ptr| unsafe {
+                    CFType::wrap_under_get_rule(array_ptr)
+                })
+        })
+        .and_then(|addresses: CFType| {
+            if addresses.instance_of::<_, CFArray>() {
+                let addresses_array = unsafe {
+                    CFArray::wrap_under_get_rule(addresses.as_concrete_TypeRef() as CFArrayRef)
+                };
+                parse_cf_string_array(addresses_array)
+            } else {
+                error!("DNS settings is not an array: {:?}", addresses);
+                None
+            }
+        })
+}
+
+/// Parses a CFArray into a Rust vector of Rust strings, if the array contains CFString instances,
+/// otherwise `None` is returned.
+fn parse_cf_string_array(array: CFArray) -> Option<Vec<String>> {
+    let mut strings = Vec::new();
+    for string_ptr in array.iter() {
+        let cf_type = unsafe { CFType::wrap_under_get_rule(string_ptr) };
+        if cf_type.instance_of::<_, CFString>() {
+            let address = unsafe { CFString::wrap_under_get_rule(string_ptr as CFStringRef) };
+            strings.push(address.to_string());
+        } else {
+            error!("DNS server entry is not a string: {:?}", cf_type);
+            return None;
+        };
+    }
+    Some(strings)
+}
diff --git a/talpid-core/src/firewall/macos.rs b/talpid-core/src/firewall/macos/mod.rs
index fdef40ed9e..9065a607d6 100644
--- a/talpid-core/src/firewall/macos.rs
+++ b/talpid-core/src/firewall/macos/mod.rs
@@ -1,27 +1,34 @@
-extern crate socket_relay;
+extern crate pfctl;
 extern crate tokio_core;
 
 use super::{Firewall, SecurityPolicy};
-use pfctl;
 
-use std::net::{IpAddr, Ipv4Addr, SocketAddr};
-use std::sync::mpsc;
-use std::thread;
+use std::net::Ipv4Addr;
 
-use self::socket_relay::udp::{Relay, RelayCloseHandle};
 use talpid_types::net;
-use tunnel::TunnelMetadata;
 
-// alias used to instantiate firewall implementation
+mod dns;
+
+use self::dns::DnsMonitor;
+
+error_chain! {
+    links {
+        PfCtl(self::pfctl::Error, self::pfctl::ErrorKind) #[doc = "PF error"];
+        DnsMonitor(self::dns::Error, self::dns::ErrorKind) #[doc = "DNS error"];
+    }
+}
+
+/// alias used to instantiate firewall implementation
 pub type ConcreteFirewall = PacketFilter;
-pub use pfctl::{Error, ErrorKind, Result, ResultExt};
 
 const ANCHOR_NAME: &'static str = "mullvad";
 
+/// The macOS firewall implementation. Acting as converter between the `Firewall` trait API
+/// and actual PF firewall rules and other protective measures to keep the `SecurityPolicy`.
 pub struct PacketFilter {
     pf: pfctl::PfCtl,
     pf_was_enabled: Option<bool>,
-    dns_proxy_close_handle: Option<RelayCloseHandle>,
+    dns_monitor: DnsMonitor,
 }
 
 impl Firewall<Error> for PacketFilter {
@@ -29,7 +36,7 @@ impl Firewall<Error> for PacketFilter {
         Ok(PacketFilter {
             pf: pfctl::PfCtl::new()?,
             pf_was_enabled: None,
-            dns_proxy_close_handle: None,
+            dns_monitor: DnsMonitor::new()?,
         })
     }
 
@@ -40,11 +47,11 @@ impl Firewall<Error> for PacketFilter {
     }
 
     fn reset_policy(&mut self) -> Result<()> {
-        self.stop_dns_proxy();
         vec![
             self.remove_rules(),
             self.remove_anchor(),
             self.restore_state(),
+            self.restore_dns(),
         ].into_iter()
             .collect::<Result<Vec<_>>>()
             .map(|_| ())
@@ -54,15 +61,12 @@ impl Firewall<Error> for PacketFilter {
 impl PacketFilter {
     fn set_rules(&mut self, policy: SecurityPolicy) -> Result<()> {
         let mut new_filter_rules = vec![];
-        let mut new_redirect_rules = vec![];
 
         new_filter_rules.append(&mut Self::get_allow_loopback_rules()?);
         new_filter_rules.append(&mut Self::get_allow_dhcp_rules()?);
 
-        let (mut policy_filter_rules, mut policy_redirect_rules) =
-            self.get_policy_specific_rules(policy)?;
+        let mut policy_filter_rules = self.get_policy_specific_rules(policy)?;
         new_filter_rules.append(&mut policy_filter_rules);
-        new_redirect_rules.append(&mut policy_redirect_rules);
 
         let drop_all_rule = pfctl::FilterRuleBuilder::default()
             .action(pfctl::FilterRuleAction::Drop)
@@ -72,64 +76,59 @@ impl PacketFilter {
 
         let mut anchor_change = pfctl::AnchorChange::new();
         anchor_change.set_filter_rules(new_filter_rules);
-        anchor_change.set_redirect_rules(new_redirect_rules);
-        self.pf.set_rules(ANCHOR_NAME, anchor_change)
+        Ok(self.pf.set_rules(ANCHOR_NAME, anchor_change)?)
     }
 
     fn get_policy_specific_rules(
         &mut self,
         policy: SecurityPolicy,
-    ) -> Result<(Vec<pfctl::FilterRule>, Vec<pfctl::RedirectRule>)> {
+    ) -> Result<Vec<pfctl::FilterRule>> {
         match policy {
             SecurityPolicy::Connecting(relay_endpoint) => {
-                self.stop_dns_proxy();
-                Ok((vec![Self::get_allow_relay_rule(relay_endpoint)?], vec![]))
+                Ok(vec![Self::get_allow_relay_rule(relay_endpoint)?])
             }
             SecurityPolicy::Connected(relay_endpoint, tunnel) => {
-                let dns_proxy_listen_addr = self.start_dns_proxy(&tunnel)?;
+                self.dns_monitor.set_dns(vec![tunnel.gateway.to_string()])?;
 
-                let allow_dns_to_relay_rule = pfctl::FilterRuleBuilder::default()
+                let allow_tcp_dns_to_relay_rule = pfctl::FilterRuleBuilder::default()
                     .action(pfctl::FilterRuleAction::Pass)
                     .direction(pfctl::Direction::Out)
                     .quick(true)
                     .interface(&tunnel.interface)
-                    .proto(pfctl::Proto::Udp)
+                    .proto(pfctl::Proto::Tcp)
                     .to(pfctl::Endpoint::new(tunnel.gateway, 53))
                     .build()?;
-                let reroute_dns_rule = pfctl::FilterRuleBuilder::default()
+                let allow_udp_dns_to_relay_rule = pfctl::FilterRuleBuilder::default()
                     .action(pfctl::FilterRuleAction::Pass)
                     .direction(pfctl::Direction::Out)
                     .quick(true)
-                    .route(pfctl::Route::route_to(pfctl::Interface::from("lo0")))
+                    .interface(&tunnel.interface)
                     .proto(pfctl::Proto::Udp)
-                    .to(pfctl::Port::from(53))
+                    .to(pfctl::Endpoint::new(tunnel.gateway, 53))
                     .build()?;
-                let block_all_other_dns_rule = pfctl::FilterRuleBuilder::default()
+                let block_tcp_dns_rule = pfctl::FilterRuleBuilder::default()
                     .action(pfctl::FilterRuleAction::Drop)
                     .direction(pfctl::Direction::Out)
                     .quick(true)
                     .proto(pfctl::Proto::Tcp)
                     .to(pfctl::Port::from(53))
                     .build()?;
-
-                let dns_redirect_rule = pfctl::RedirectRuleBuilder::default()
-                    .action(pfctl::RedirectRuleAction::Redirect)
-                    .interface("lo0")
+                let block_udp_dns_rule = pfctl::FilterRuleBuilder::default()
+                    .action(pfctl::FilterRuleAction::Drop)
+                    .direction(pfctl::Direction::Out)
+                    .quick(true)
                     .proto(pfctl::Proto::Udp)
                     .to(pfctl::Port::from(53))
-                    .redirect_to(dns_proxy_listen_addr)
                     .build()?;
 
-                Ok((
-                    vec![
-                        allow_dns_to_relay_rule,
-                        reroute_dns_rule,
-                        block_all_other_dns_rule,
-                        Self::get_allow_relay_rule(relay_endpoint)?,
-                        Self::get_allow_tunnel_rule(tunnel.interface.as_str())?,
-                    ],
-                    vec![dns_redirect_rule],
-                ))
+                Ok(vec![
+                    allow_tcp_dns_to_relay_rule,
+                    allow_udp_dns_to_relay_rule,
+                    block_tcp_dns_rule,
+                    block_udp_dns_rule,
+                    Self::get_allow_relay_rule(relay_endpoint)?,
+                    Self::get_allow_tunnel_rule(tunnel.interface.as_str())?,
+                ])
             }
         }
     }
@@ -137,7 +136,7 @@ impl PacketFilter {
     fn get_allow_relay_rule(relay_endpoint: net::Endpoint) -> Result<pfctl::FilterRule> {
         let pfctl_proto = as_pfctl_proto(relay_endpoint.protocol);
 
-        pfctl::FilterRuleBuilder::default()
+        Ok(pfctl::FilterRuleBuilder::default()
             .action(pfctl::FilterRuleAction::Pass)
             .direction(pfctl::Direction::Out)
             .to(relay_endpoint.address)
@@ -145,17 +144,17 @@ impl PacketFilter {
             .keep_state(pfctl::StatePolicy::Keep)
             .tcp_flags(Self::get_tcp_flags())
             .quick(true)
-            .build()
+            .build()?)
     }
 
     fn get_allow_tunnel_rule(tunnel_interface: &str) -> Result<pfctl::FilterRule> {
-        pfctl::FilterRuleBuilder::default()
+        Ok(pfctl::FilterRuleBuilder::default()
             .action(pfctl::FilterRuleAction::Pass)
             .interface(tunnel_interface)
             .keep_state(pfctl::StatePolicy::Keep)
             .tcp_flags(Self::get_tcp_flags())
             .quick(true)
-            .build()
+            .build()?)
     }
 
     fn get_allow_loopback_rules() -> Result<Vec<pfctl::FilterRule>> {
@@ -200,49 +199,43 @@ impl PacketFilter {
 
     fn remove_rules(&mut self) -> Result<()> {
         // remove_anchor() does not deactivate active rules
-        self.pf.flush_rules(ANCHOR_NAME, pfctl::RulesetKind::Filter)
+        Ok(self.pf
+            .flush_rules(ANCHOR_NAME, pfctl::RulesetKind::Filter)?)
     }
 
     fn enable(&mut self) -> Result<()> {
         if self.pf_was_enabled.is_none() {
             self.pf_was_enabled = Some(self.pf.is_enabled()?);
         }
-        self.pf.try_enable()
+        Ok(self.pf.try_enable()?)
     }
 
     fn restore_state(&mut self) -> Result<()> {
         match self.pf_was_enabled.take() {
-            Some(true) => self.pf.try_enable(),
-            Some(false) => self.pf.try_disable(),
+            Some(true) => Ok(self.pf.try_enable()?),
+            Some(false) => Ok(self.pf.try_disable()?),
             None => Ok(()),
         }
     }
 
+    fn restore_dns(&self) -> Result<()> {
+        Ok(self.dns_monitor.reset()?)
+    }
+
     fn add_anchor(&mut self) -> Result<()> {
         self.pf
             .try_add_anchor(ANCHOR_NAME, pfctl::AnchorKind::Filter)?;
         self.pf
-            .try_add_anchor(ANCHOR_NAME, pfctl::AnchorKind::Redirect)
+            .try_add_anchor(ANCHOR_NAME, pfctl::AnchorKind::Redirect)?;
+        Ok(())
     }
 
     fn remove_anchor(&mut self) -> Result<()> {
         self.pf
             .try_remove_anchor(ANCHOR_NAME, pfctl::AnchorKind::Filter)?;
         self.pf
-            .try_remove_anchor(ANCHOR_NAME, pfctl::AnchorKind::Redirect)
-    }
-
-    fn start_dns_proxy(&mut self, tunnel: &TunnelMetadata) -> Result<SocketAddr> {
-        self.stop_dns_proxy();
-        let (listen_addr, close_handle) = spawn_dns_proxy(tunnel.ip, tunnel.gateway)?;
-        self.dns_proxy_close_handle = Some(close_handle);
-        Ok(listen_addr)
-    }
-
-    fn stop_dns_proxy(&mut self) {
-        if let Some(close_handle) = self.dns_proxy_close_handle.take() {
-            close_handle.close();
-        }
+            .try_remove_anchor(ANCHOR_NAME, pfctl::AnchorKind::Redirect)?;
+        Ok(())
     }
 }
 
@@ -252,43 +245,3 @@ fn as_pfctl_proto(protocol: net::TransportProtocol) -> pfctl::Proto {
         net::TransportProtocol::Tcp => pfctl::Proto::Tcp,
     }
 }
-
-fn spawn_dns_proxy(
-    tunnel_ip: Ipv4Addr,
-    tunnel_gateway: Ipv4Addr,
-) -> Result<(SocketAddr, RelayCloseHandle)> {
-    let (tx, rx) = mpsc::channel();
-    thread::spawn(move || {
-        match spawn_dns_proxy_helper(tunnel_ip, tunnel_gateway) {
-            Ok((mut core, relay)) => {
-                tx.send(Ok((relay.listen_addr(), relay.close_handle())))
-                    .unwrap();
-                match core.run(relay) {
-                    Err(e) => error!("DNS proxy died with an error: {}", e),
-                    Ok(_) => info!("DNS proxy exiting"),
-                }
-            }
-            Err(e) => {
-                tx.send(Err(e)).unwrap();
-            }
-        }
-    });
-    rx.recv().unwrap()
-}
-
-fn spawn_dns_proxy_helper(
-    tunnel_ip: Ipv4Addr,
-    tunnel_gateway: Ipv4Addr,
-) -> Result<(tokio_core::reactor::Core, Relay)> {
-    let core = tokio_core::reactor::Core::new().chain_err(|| "Unable to init Tokio event loop")?;
-
-    let relay = Relay::new(
-        "127.0.0.1:0".parse().unwrap(),
-        IpAddr::V4(tunnel_ip),
-        SocketAddr::from((tunnel_gateway, 53)),
-        core.handle(),
-    ).chain_err(|| "Unable to create DNS proxy socket relay")?;
-    info!("DNS proxy listening on {}", relay.listen_addr());
-
-    Ok((core, relay))
-}
diff --git a/talpid-core/src/firewall/mod.rs b/talpid-core/src/firewall/mod.rs
index 6ac63e2493..17ba65b88d 100644
--- a/talpid-core/src/firewall/mod.rs
+++ b/talpid-core/src/firewall/mod.rs
@@ -1,16 +1,24 @@
 use talpid_types::net::Endpoint;
 
+/// macOS implementation of the firewall/security policy enforcer.
 #[cfg(target_os = "macos")]
-#[path = "macos.rs"]
-mod imp;
+pub mod macos;
+#[cfg(target_os = "macos")]
+use self::macos as imp;
 
+/// Linux implementation of the firewall/security policy enforcer.
+#[cfg(all(unix, not(target_os = "macos")))]
+pub mod unix;
 #[cfg(all(unix, not(target_os = "macos")))]
-#[path = "unix.rs"]
-mod imp;
+use self::unix as imp;
 
+/// Windows implementation of the firewall/security policy enforcer.
+#[cfg(windows)]
+pub mod windows;
 #[cfg(windows)]
-#[path = "windows.rs"]
-mod imp;
+use self::windows as imp;
+
+
 
 error_chain!{
     errors {
diff --git a/talpid-core/src/firewall/unix.rs b/talpid-core/src/firewall/unix.rs
index 7550c3c051..b39013e3c0 100644
--- a/talpid-core/src/firewall/unix.rs
+++ b/talpid-core/src/firewall/unix.rs
@@ -1,10 +1,11 @@
 use super::{Firewall, SecurityPolicy};
 
-// alias used to instantiate firewall implementation
+/// alias used to instantiate firewall implementation
 pub type ConcreteFirewall = Netfilter;
 
 error_chain!{}
 
+/// The Linux implementation for the `Firewall` trait.
 pub struct Netfilter;
 impl Firewall<Error> for Netfilter {
     fn new() -> Result<Self> {
diff --git a/talpid-core/src/firewall/windows.rs b/talpid-core/src/firewall/windows.rs
index 3405ba12c0..bae506fdc5 100644
--- a/talpid-core/src/firewall/windows.rs
+++ b/talpid-core/src/firewall/windows.rs
@@ -1,10 +1,11 @@
 use super::{Firewall, SecurityPolicy};
 
-// alias used to instantiate firewall implementation
+/// alias used to instantiate firewall implementation
 pub type ConcreteFirewall = WindowsFirewall;
 
 error_chain!{}
 
+/// The Windows implementation for the `Firewall` trait.
 pub struct WindowsFirewall;
 impl Firewall<Error> for WindowsFirewall {
     fn new() -> Result<Self> {
diff --git a/talpid-core/src/lib.rs b/talpid-core/src/lib.rs
index 1af5587def..fc85584267 100644
--- a/talpid-core/src/lib.rs
+++ b/talpid-core/src/lib.rs
@@ -29,9 +29,6 @@ extern crate openvpn_plugin;
 extern crate talpid_ipc;
 extern crate talpid_types;
 
-#[cfg(target_os = "macos")]
-extern crate pfctl;
-
 /// Working with processes.
 pub mod process;