Changing macOS dns monitor implementation to consider all settings

3 files changed, 159 insertions, 176 deletions

diff --git a/Cargo.lock b/Cargo.lock
index b518ba0a83..2ce8627195 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -159,20 +159,17 @@ dependencies = [
 
 [[package]]
 name = "core-foundation"
-version = "0.5.1"
+version = "0.6.2"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 dependencies = [
- "core-foundation-sys 0.5.1 (registry+https://github.com/rust-lang/crates.io-index)",
+ "core-foundation-sys 0.6.2 (registry+https://github.com/rust-lang/crates.io-index)",
  "libc 0.2.43 (registry+https://github.com/rust-lang/crates.io-index)",
 ]
 
 [[package]]
 name = "core-foundation-sys"
-version = "0.5.1"
+version = "0.6.2"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-dependencies = [
- "libc 0.2.43 (registry+https://github.com/rust-lang/crates.io-index)",
-]
 
 [[package]]
 name = "crossbeam-deque"
@@ -1618,19 +1615,20 @@ dependencies = [
 
 [[package]]
 name = "system-configuration"
-version = "0.1.0"
+version = "0.2.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 dependencies = [
- "core-foundation 0.5.1 (registry+https://github.com/rust-lang/crates.io-index)",
- "system-configuration-sys 0.1.0 (registry+https://github.com/rust-lang/crates.io-index)",
+ "core-foundation 0.6.2 (registry+https://github.com/rust-lang/crates.io-index)",
+ "system-configuration-sys 0.2.0 (registry+https://github.com/rust-lang/crates.io-index)",
 ]
 
 [[package]]
 name = "system-configuration-sys"
-version = "0.1.0"
+version = "0.2.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 dependencies = [
- "core-foundation-sys 0.5.1 (registry+https://github.com/rust-lang/crates.io-index)",
+ "core-foundation-sys 0.6.2 (registry+https://github.com/rust-lang/crates.io-index)",
+ "libc 0.2.43 (registry+https://github.com/rust-lang/crates.io-index)",
 ]
 
 [[package]]
@@ -1643,7 +1641,6 @@ name = "talpid-core"
 version = "0.1.0"
 dependencies = [
  "atty 0.2.11 (registry+https://github.com/rust-lang/crates.io-index)",
- "core-foundation 0.5.1 (registry+https://github.com/rust-lang/crates.io-index)",
  "dbus 0.6.2 (registry+https://github.com/rust-lang/crates.io-index)",
  "duct 0.11.0 (registry+https://github.com/rust-lang/crates.io-index)",
  "error-chain 0.12.0 (registry+https://github.com/rust-lang/crates.io-index)",
@@ -1663,7 +1660,7 @@ dependencies = [
  "pfctl 0.2.0 (registry+https://github.com/rust-lang/crates.io-index)",
  "resolv-conf 0.6.1 (registry+https://github.com/rust-lang/crates.io-index)",
  "shell-escape 0.1.4 (registry+https://github.com/rust-lang/crates.io-index)",
- "system-configuration 0.1.0 (registry+https://github.com/rust-lang/crates.io-index)",
+ "system-configuration 0.2.0 (registry+https://github.com/rust-lang/crates.io-index)",
  "talpid-ipc 0.1.0",
  "talpid-types 0.1.0",
  "tempfile 3.0.3 (registry+https://github.com/rust-lang/crates.io-index)",
@@ -2201,8 +2198,8 @@ dependencies = [
 "checksum clap 2.32.0 (registry+https://github.com/rust-lang/crates.io-index)" = "b957d88f4b6a63b9d70d5f454ac8011819c6efa7727858f458ab71c756ce2d3e"
 "checksum cloudabi 0.0.3 (registry+https://github.com/rust-lang/crates.io-index)" = "ddfc5b9aa5d4507acaf872de71051dfd0e309860e88966e1051e462a077aac4f"
 "checksum colored 1.6.1 (registry+https://github.com/rust-lang/crates.io-index)" = "dc0a60679001b62fb628c4da80e574b9645ab4646056d7c9018885efffe45533"
-"checksum core-foundation 0.5.1 (registry+https://github.com/rust-lang/crates.io-index)" = "286e0b41c3a20da26536c6000a280585d519fd07b3956b43aed8a79e9edce980"
-"checksum core-foundation-sys 0.5.1 (registry+https://github.com/rust-lang/crates.io-index)" = "716c271e8613ace48344f723b60b900a93150271e5be206212d052bbc0883efa"
+"checksum core-foundation 0.6.2 (registry+https://github.com/rust-lang/crates.io-index)" = "58667b9a618a37ea8c4c4cb5298703e5dfadcd3698c79f54fc43e6a2e94733ea"
+"checksum core-foundation-sys 0.6.2 (registry+https://github.com/rust-lang/crates.io-index)" = "e7ca8a5221364ef15ce201e8ed2f609fc312682a8f4e0e3d4aa5879764e0fa3b"
 "checksum crossbeam-deque 0.3.1 (registry+https://github.com/rust-lang/crates.io-index)" = "fe8153ef04a7594ded05b427ffad46ddeaf22e63fd48d42b3e1e3bb4db07cae7"
 "checksum crossbeam-epoch 0.4.3 (registry+https://github.com/rust-lang/crates.io-index)" = "2af0e75710d6181e234c8ecc79f14a97907850a541b13b0be1dd10992f2e4620"
 "checksum crossbeam-utils 0.3.2 (registry+https://github.com/rust-lang/crates.io-index)" = "d636a8b3bcc1b409d7ffd3facef8f21dcb4009626adbd0c5e6c4305c07253c7b"
@@ -2345,8 +2342,8 @@ dependencies = [
 "checksum syn 0.14.7 (registry+https://github.com/rust-lang/crates.io-index)" = "e2e13df71f29f9440b50261a5882c86eac334f1badb3134ec26f0de2f1418e44"
 "checksum synom 0.11.3 (registry+https://github.com/rust-lang/crates.io-index)" = "a393066ed9010ebaed60b9eafa373d4b1baac186dd7e008555b0f702b51945b6"
 "checksum synstructure 0.9.0 (registry+https://github.com/rust-lang/crates.io-index)" = "85bb9b7550d063ea184027c9b8c20ac167cd36d3e06b3a40bceb9d746dc1a7b7"
-"checksum system-configuration 0.1.0 (registry+https://github.com/rust-lang/crates.io-index)" = "2646789845add5fa0adcbe7684cb89509ae98c404284471bf4f9faf995d88a58"
-"checksum system-configuration-sys 0.1.0 (registry+https://github.com/rust-lang/crates.io-index)" = "7d8b463ff8bb4585b46e3e23f44dd41b3f52d0ad09b6b9cf03aae55c74d74cff"
+"checksum system-configuration 0.2.0 (registry+https://github.com/rust-lang/crates.io-index)" = "8ce91c23e42859a5b8a7da032096d34f776512239e89de6c7bec817d6089cf9c"
+"checksum system-configuration-sys 0.2.0 (registry+https://github.com/rust-lang/crates.io-index)" = "114702b56796b5cf377b6ae266a319fa687edd4d9f18f4d5658193e65dcaf006"
 "checksum take 0.1.0 (registry+https://github.com/rust-lang/crates.io-index)" = "b157868d8ac1f56b64604539990685fa7611d8fa9e5476cf0c02cf34d32917c5"
 "checksum tempfile 3.0.3 (registry+https://github.com/rust-lang/crates.io-index)" = "c4b103c6d08d323b92ff42c8ce62abcd83ca8efa7fd5bf7927efefec75f58c76"
 "checksum termcolor 1.0.1 (registry+https://github.com/rust-lang/crates.io-index)" = "722426c4a0539da2c4ffd9b419d90ad540b4cff4a053be9069c908d4d07e2836"
diff --git a/talpid-core/Cargo.toml b/talpid-core/Cargo.toml
index b0f047ef52..50e5f86c0b 100644
--- a/talpid-core/Cargo.toml
+++ b/talpid-core/Cargo.toml
@@ -39,8 +39,7 @@ which = "2.0"
 
 [target.'cfg(target_os = "macos")'.dependencies]
 pfctl = "0.2"
-system-configuration = "0.1"
-core-foundation = "0.5"
+system-configuration = "0.2"
 tokio-core = "0.1"
 
 [target.'cfg(windows)'.dependencies]
diff --git a/talpid-core/src/security/macos/dns.rs b/talpid-core/src/security/macos/dns.rs
index 1fdc000776..f75ab69e4f 100644
--- a/talpid-core/src/security/macos/dns.rs
+++ b/talpid-core/src/security/macos/dns.rs
@@ -1,20 +1,21 @@
-extern crate core_foundation;
 extern crate system_configuration;
 
-use self::core_foundation::{
-    array::CFArray,
-    base::{CFType, TCFType},
-    dictionary::{CFDictionary, CFMutableDictionary},
-    propertylist::CFPropertyList,
-    runloop::{kCFRunLoopCommonModes, CFRunLoop},
-    string::CFString,
-};
-use self::system_configuration::dynamic_store::{
-    SCDynamicStore, SCDynamicStoreBuilder, SCDynamicStoreCallBackContext,
+use self::system_configuration::{
+    core_foundation::{
+        array::CFArray,
+        base::{CFType, TCFType, ToVoid},
+        dictionary::{CFDictionary, CFMutableDictionary},
+        propertylist::CFPropertyList,
+        runloop::{kCFRunLoopCommonModes, CFRunLoop},
+        string::CFString,
+    },
+    dynamic_store::{SCDynamicStore, SCDynamicStoreBuilder, SCDynamicStoreCallBackContext},
+    sys::schema_definitions::kSCPropNetDNSServerAddresses,
 };
 use error_chain::ChainedError;
 use std::{
     collections::HashMap,
+    fmt,
     sync::{mpsc, Arc, Mutex},
     thread,
 };
@@ -34,24 +35,84 @@ type DnsServer = String;
 
 struct State {
     /// The settings this monitor is currently enforcing as active settings.
-    desired_dns: DnsSettings,
+    dns_settings: DnsSettings,
     /// The backup of all DNS settings. These are being applied back on reset.
     backup: HashMap<ServicePath, Option<DnsSettings>>,
 }
 
 /// Holds the configuration for one service.
 #[derive(Debug, Eq, PartialEq)]
-struct DnsSettings {
-    server_addresses: Vec<DnsServer>,
-    search_domains: Vec<String>,
-}
+struct DnsSettings(CFDictionary);
+
+unsafe impl Send for DnsSettings {}
 
 impl DnsSettings {
-    pub fn from_server_addresses(server_addresses: Vec<DnsServer>) -> Self {
-        DnsSettings {
-            server_addresses,
-            search_domains: Vec::new(),
+    pub fn from_server_addresses(server_addresses: &[DnsServer]) -> Self {
+        let mut mut_dict = CFMutableDictionary::new();
+        if !server_addresses.is_empty() {
+            let cf_string_servers: Vec<CFString> =
+                server_addresses.iter().map(|s| CFString::new(s)).collect();
+            let server_addresses_value = CFArray::from_CFTypes(&cf_string_servers).into_untyped();
+            let server_addresses_key =
+                unsafe { CFString::wrap_under_get_rule(kSCPropNetDNSServerAddresses) };
+            mut_dict.add(
+                &server_addresses_key.to_void(),
+                &server_addresses_value.to_void(),
+            );
+        }
+        let dict = unsafe { CFDictionary::wrap_under_get_rule(mut_dict.as_concrete_TypeRef()) };
+        DnsSettings(dict)
+    }
+
+    /// Get DNS settings for a given service path. Returns `None` If the path does not exist.
+    pub fn load<S: Into<CFString>>(store: &SCDynamicStore, path: S) -> Option<Self> {
+        let dict = store
+            .get(path)
+            .and_then(CFPropertyList::downcast_into::<CFDictionary>)?;
+        Some(DnsSettings(dict))
+    }
+
+    /// Set the dynamic store entry at `path` to a dictionary these DNS settings.
+    pub fn save<S: Into<CFString> + fmt::Display>(
+        &self,
+        store: &SCDynamicStore,
+        path: S,
+    ) -> Result<()> {
+        trace!(
+            "Setting DNS to [{}] for {}",
+            self.server_addresses().join(", "),
+            path.to_string()
+        );
+        if store.set(path, self.0.clone()) {
+            Ok(())
+        } else {
+            bail!(ErrorKind::SettingDnsFailed)
+        }
+    }
+
+    pub fn server_addresses(&self) -> Vec<String> {
+        self.0
+            .find(unsafe { kSCPropNetDNSServerAddresses }.to_void())
+            .map(|array_ptr| unsafe { CFType::wrap_under_get_rule(*array_ptr) })
+            .and_then(|array| array.downcast::<CFArray>())
+            .and_then(Self::parse_cf_array_to_strings)
+            .unwrap_or(Vec::new())
+    }
+
+    /// Parses a CFArray into a Rust vector of Rust strings, if the array contains CFString
+    /// instances only, otherwise `None` is returned.
+    fn parse_cf_array_to_strings(array: CFArray) -> Option<Vec<String>> {
+        let mut strings = Vec::new();
+        for item_ptr in array.iter() {
+            let item = unsafe { CFType::wrap_under_get_rule(*item_ptr) };
+            if let Some(string) = item.downcast::<CFString>() {
+                strings.push(string.to_string());
+            } else {
+                error!("DNS server entry is not a string: {:?}", item);
+                return None;
+            };
         }
+        Some(strings)
     }
 }
 
@@ -59,7 +120,7 @@ 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
+    /// When it's `Some(state)` we are actively making sure `state.dns_settings` is configured
     /// on all network interfaces.
     state: Arc<Mutex<Option<State>>>,
 }
@@ -95,29 +156,29 @@ impl DnsMonitor {
     }
 
     pub fn set_dns(&self, servers: Vec<DnsServer>) -> Result<()> {
-        let settings = DnsSettings::from_server_addresses(servers);
+        let settings = DnsSettings::from_server_addresses(&servers);
         let mut state_lock = self.state.lock().unwrap();
         *state_lock = Some(match state_lock.take() {
             None => {
                 let backup = read_all_dns(&self.store);
                 trace!("Backup of DNS settings: {:#?}", backup);
-                debug!("Setting DNS to [{}]", settings.server_addresses.join(", "));
+                debug!("Setting DNS to [{}]", servers.join(", "));
                 for service_path in backup.keys() {
-                    set_dns(&self.store, CFString::new(service_path), &settings)?;
+                    settings.save(&self.store, service_path.as_str())?;
                 }
                 State {
-                    desired_dns: settings,
+                    dns_settings: settings,
                     backup,
                 }
             }
             Some(state) => {
-                if settings != state.desired_dns {
-                    debug!("Changing DNS to [{}]", settings.server_addresses.join(", "));
+                if servers != state.dns_settings.server_addresses() {
+                    debug!("Changing DNS to [{}]", servers.join(", "));
                     for service_path in state.backup.keys() {
-                        set_dns(&self.store, CFString::new(service_path), &settings)?;
+                        settings.save(&self.store, service_path.as_str())?;
                     }
                     State {
-                        desired_dns: settings,
+                        dns_settings: settings,
                         backup: state.backup,
                     }
                 } else {
@@ -133,9 +194,10 @@ impl DnsMonitor {
     pub fn reset(&self) -> Result<()> {
         let mut state_lock = self.state.lock().unwrap();
         if let Some(state) = state_lock.take() {
+            trace!("Restoring DNS settings to: {:#?}", state.backup);
             for (service_path, settings) in state.backup {
                 if let Some(settings) = settings {
-                    set_dns(&self.store, CFString::new(&service_path), &settings)?;
+                    settings.save(&self.store, service_path.as_str())?;
                 } else {
                     debug!("Removing DNS for {}", service_path);
                     if !self.store.remove(CFString::new(&service_path)) {
@@ -188,104 +250,64 @@ fn dns_change_callback(
     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 in changed_keys.iter() {
-                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(settings) => {
-                        if settings != state.desired_dns {
-                            debug!(
-                                "Detected DNS changed to [{}] for {}",
-                                settings.server_addresses.join(", "),
-                                *path
-                            );
-                            state.backup.insert(path.to_string(), Some(settings));
-                            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" entry, also set the corresponding "setup" entry.
-                    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))?;
-                    }
-                }
+            if let Err(e) = dns_change_callback_internal(store, changed_keys, state) {
+                error!("{}", e.display_chain());
             }
         }
     }
-    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, settings: &DnsSettings) -> Result<()> {
-    trace!(
-        "Setting DNS to [{}], with search domains [{}] for {}",
-        settings.server_addresses.join(", "),
-        settings.search_domains.join(", "),
-        path
-    );
-    let dns_dictionary = CFMutableDictionary::new();
-    if !settings.server_addresses.is_empty() {
-        let server_addresses_key = CFString::from_static_string("ServerAddresses");
-        let cf_string_servers: Vec<CFString> = settings
-            .server_addresses
-            .iter()
-            .map(|s| CFString::new(s))
-            .collect();
-        let server_addresses_value = CFArray::from_CFTypes(&cf_string_servers);
-        dns_dictionary.add2(&server_addresses_key, &server_addresses_value);
-    }
-    if !settings.search_domains.is_empty() {
-        let search_domains_key = CFString::from_static_string("SearchDomains");
-        let cf_string_search_domains: Vec<CFString> = settings
-            .search_domains
-            .iter()
-            .map(|s| CFString::new(s))
-            .collect();
-        let search_domains_value = CFArray::from_CFTypes(&cf_string_search_domains);
-        dns_dictionary.add2(&search_domains_key, &search_domains_value);
-    }
-    let dns_dictionary = dns_dictionary
-        .into_CFType()
-        .downcast_into::<CFDictionary>()
-        .unwrap();
-
-    if store.set(path, dns_dictionary) {
-        Ok(())
-    } else {
-        bail!(ErrorKind::SettingDnsFailed)
+fn dns_change_callback_internal(
+    store: SCDynamicStore,
+    changed_keys: CFArray<CFString>,
+    state: &mut State,
+) -> Result<()> {
+    for path in &changed_keys {
+        let should_set_dns = match DnsSettings::load(&store, path.clone()) {
+            None => {
+                debug!("Detected DNS removed for {}", *path);
+                state.backup.insert(path.to_string(), None);
+                true
+            }
+            Some(new_settings) => {
+                if new_settings != state.dns_settings {
+                    debug!("Detected DNS change for {}", *path);
+                    state.backup.insert(path.to_string(), Some(new_settings));
+                    true
+                } else {
+                    trace!("Ignoring DNS change since it's equal to desired DNS");
+                    false
+                }
+            }
+        };
+        if should_set_dns {
+            state
+                .dns_settings
+                .save(&store, path.clone())
+                .chain_err(|| format!("Failed changing DNS for {}", *path))?;
+            // If we changed a "state" entry, also set the corresponding "setup" entry.
+            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,
+                        DnsSettings::load(&store, setup_path.clone()),
+                    );
+                }
+                state
+                    .dns_settings
+                    .save(&store, setup_path.clone())
+                    .chain_err(|| format!("Failed changing DNS for {}", setup_path))?;
+            }
+        }
     }
+    Ok(())
 }
 
 /// Read all existing DNS settings and return them.
@@ -296,9 +318,11 @@ fn read_all_dns(store: &SCDynamicStore) -> HashMap<ServicePath, Option<DnsSettin
         for state_path in paths.iter() {
             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.clone()));
-            backup.insert(setup_path_str, read_dns(store, setup_path));
+            backup.insert(state_path_str, DnsSettings::load(store, state_path.clone()));
+            backup.insert(
+                setup_path_str.clone(),
+                DnsSettings::load(store, setup_path_str.as_ref()),
+            );
         }
     }
     // Backup all "setup" DNS not already covered
@@ -306,7 +330,7 @@ fn read_all_dns(store: &SCDynamicStore) -> HashMap<ServicePath, Option<DnsSettin
         for setup_path in paths.iter() {
             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.clone()));
+                backup.insert(setup_path_str, DnsSettings::load(store, setup_path.clone()));
             }
         }
     }
@@ -320,40 +344,3 @@ fn state_to_setup_path(state_path: &str) -> Option<String> {
         None
     }
 }
-
-/// Get DNS settings for a given service path. Returns `None` If the path does not exist.
-fn read_dns(store: &SCDynamicStore, path: CFString) -> Option<DnsSettings> {
-    let dictionary = store
-        .get(path)
-        .and_then(CFPropertyList::downcast_into::<CFDictionary>)?;
-    let search_domains = get_dictionary_value(&dictionary, "SearchDomains").unwrap_or(Vec::new());
-    let server_addresses =
-        get_dictionary_value(&dictionary, "ServerAddresses").unwrap_or(Vec::new());
-    Some(DnsSettings {
-        server_addresses,
-        search_domains,
-    })
-}
-
-fn get_dictionary_value(dictionary: &CFDictionary, key: &'static str) -> Option<Vec<String>> {
-    let array = dictionary
-        .find2(&CFString::from_static_string(key))
-        .map(|array_ptr| unsafe { CFType::wrap_under_get_rule(array_ptr) })?;
-    let cast_array = array.downcast::<CFArray<CFType>>()?;
-    Some(parse_cf_array_to_strings(cast_array)?)
-}
-
-/// Parses a CFArray into a Rust vector of Rust strings, if the array contains CFString instances
-/// only, otherwise `None` is returned.
-fn parse_cf_array_to_strings(array: CFArray<CFType>) -> Option<Vec<String>> {
-    let mut strings = Vec::new();
-    for item in array.iter() {
-        if let Some(string) = item.downcast::<CFString>() {
-            strings.push(string.to_string());
-        } else {
-            error!("DNS server entry is not a string: {:?}", item);
-            return None;
-        };
-    }
-    Some(strings)
-}