Merge branch 'backup-macos-search-domains'

4 files changed, 181 insertions, 150 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 5b2c14fa20..a535fc7714 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -34,6 +34,9 @@ Line wrap the file at 100 chars.                                              Th
 - Pick new random relay for each reconnect attempt instead of just retrying with the same one.
 - Disable GPU acceleration on Linux to fix App on Ubuntu 14.04 and other older distributions.
 
+#### macOS
+- Correctly backup and restore search domains and other DNS settings.
+
 
 ## [2018.4] - 2018-10-16
 ### Fixed
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 6ecc9cfbd0..f75ab69e4f 100644
--- a/talpid-core/src/security/macos/dns.rs
+++ b/talpid-core/src/security/macos/dns.rs
@@ -1,22 +1,24 @@
-extern crate core_foundation;
 extern crate system_configuration;
 
-use self::core_foundation::array::CFArray;
-use self::core_foundation::base::{CFType, TCFType};
-use self::core_foundation::dictionary::CFDictionary;
-use self::core_foundation::propertylist::CFPropertyList;
-use self::core_foundation::runloop::{kCFRunLoopCommonModes, CFRunLoop};
-use self::core_foundation::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;
-use std::sync::{mpsc, Arc, Mutex};
-use std::thread;
+use std::{
+    collections::HashMap,
+    fmt,
+    sync::{mpsc, Arc, Mutex},
+    thread,
+};
 
 error_chain! {
     errors {
@@ -32,15 +34,93 @@ type ServicePath = String;
 type DnsServer = String;
 
 struct State {
-    desired_dns: Vec<DnsServer>,
-    backup: HashMap<ServicePath, Option<Vec<DnsServer>>>,
+    /// The settings this monitor is currently enforcing as active settings.
+    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(CFDictionary);
+
+unsafe impl Send for DnsSettings {}
+
+impl DnsSettings {
+    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)
+    }
 }
 
 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>>>,
 }
@@ -76,27 +156,29 @@ impl DnsMonitor {
     }
 
     pub fn set_dns(&self, servers: Vec<DnsServer>) -> Result<()> {
+        let settings = DnsSettings::from_server_addresses(&servers);
         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);
+                trace!("Backup of DNS settings: {:#?}", backup);
+                debug!("Setting DNS to [{}]", servers.join(", "));
                 for service_path in backup.keys() {
-                    set_dns(&self.store, CFString::new(service_path), &servers)?;
+                    settings.save(&self.store, service_path.as_str())?;
                 }
                 State {
-                    desired_dns: servers,
+                    dns_settings: settings,
                     backup,
                 }
             }
             Some(state) => {
-                if servers != state.desired_dns {
+                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), &servers)?;
+                        settings.save(&self.store, service_path.as_str())?;
                     }
                     State {
-                        desired_dns: servers,
+                        dns_settings: settings,
                         backup: state.backup,
                     }
                 } else {
@@ -112,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() {
-            for (service_path, servers) in state.backup {
-                if let Some(servers) = servers {
-                    set_dns(&self.store, CFString::new(&service_path), &servers)?;
+            trace!("Restoring DNS settings to: {:#?}", state.backup);
+            for (service_path, settings) in state.backup {
+                if let Some(settings) = settings {
+                    settings.save(&self.store, service_path.as_str())?;
                 } else {
                     debug!("Removing DNS for {}", service_path);
                     if !self.store.remove(CFString::new(&service_path)) {
@@ -167,94 +250,79 @@ 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(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))?;
-                    }
-                }
+            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, 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)
+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.
-fn read_all_dns(store: &SCDynamicStore) -> HashMap<ServicePath, Option<Vec<DnsServer>>> {
+fn read_all_dns(store: &SCDynamicStore) -> HashMap<ServicePath, Option<DnsSettings>> {
     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 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
@@ -262,7 +330,7 @@ fn read_all_dns(store: &SCDynamicStore) -> HashMap<ServicePath, Option<Vec<DnsSe
         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()));
             }
         }
     }
@@ -276,39 +344,3 @@ fn state_to_setup_path(state_path: &str) -> Option<String> {
         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(CFPropertyList::downcast_into::<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| {
-            if let Some(array) = addresses.downcast::<CFArray<CFType>>() {
-                parse_cf_array_to_strings(array)
-            } else {
-                error!("DNS ServerAddresess is not an array: {:?}", addresses);
-                None
-            }
-        })
-}
-
-/// 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)
-}