Changing macOS dns monitor implementation to consider all settings

1 files changed, 144 insertions, 157 deletions

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)
-}