merge: branch 'ih/dbus-global-dns'

core: dns: fix the behavior of [global-dns]

https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/2261

man/NetworkManager.conf.xml

@@ -1540,8 +1540,12 @@ managed=1
 
   <refsect1>
     <title><literal>global-dns</literal> section</title>
-    <para>This section specifies DNS settings that are applied
+    <para>This section specifies DNS settings that are applied globally. They
-    globally, in addition to connection-specific ones.</para>
+    override the equivalent options defined in individual connections, making
+    them to be ignored. If a [global-dns-domain-*] section is defined, but this
+    section isn't, an empty [global-dns] section is assumed, thus overwriting
+    connection specific configurations too.
+    </para>
     <para>
       <variablelist>
         <varlistentry>
@@ -1601,6 +1605,10 @@ managed=1
     default domain "*". When the global DNS domains are valid, the
     name servers and domains defined globally override the ones from
     active connections.
+
+    If any global DNS domain is defined but a [global-dns] section isn't,
+    an empty [global-dns] section is assumed, thus overwriting its
+    connection specific configurations too.
     </para>
     <para>
       <variablelist>

src/core/dns/nm-dns-manager.c

@@ -586,7 +586,11 @@ add_dns_domains(GPtrArray            *array,
 }
 
 static void
-merge_one_l3cd(NMResolvConfData *rc, int addr_family, int ifindex, const NML3ConfigData *l3cd)
+merge_one_l3cd(NMResolvConfData     *rc,
+               int                   addr_family,
+               int                   ifindex,
+               const NML3ConfigData *l3cd,
+               gboolean              ignore_searches_and_options)
 {
     char               buf[NM_INET_ADDRSTRLEN + 50];
     gboolean           has_trust_ad;
@@ -624,30 +628,32 @@ merge_one_l3cd(NMResolvConfData *rc, int addr_family, int ifindex, const NML3Con
         add_string_item(rc->nameservers, buf, TRUE);
     }
 
-    add_dns_domains(rc->searches, addr_family, l3cd, FALSE, TRUE);
+    if (!ignore_searches_and_options) {
+        add_dns_domains(rc->searches, addr_family, l3cd, FALSE, TRUE);
 
-    has_trust_ad = FALSE;
+        has_trust_ad = FALSE;
-    strarr       = nm_l3_config_data_get_dns_options(l3cd, addr_family, &num);
+        strarr       = nm_l3_config_data_get_dns_options(l3cd, addr_family, &num);
-    for (i = 0; i < num; i++) {
+        for (i = 0; i < num; i++) {
-        const char *option = strarr[i];
+            const char *option = strarr[i];
 
-        if (nm_streq(option, NM_SETTING_DNS_OPTION_TRUST_AD)) {
+            if (nm_streq(option, NM_SETTING_DNS_OPTION_TRUST_AD)) {
-            has_trust_ad = TRUE;
+                has_trust_ad = TRUE;
-            continue;
+                continue;
+            }
+            add_dns_option_item(rc->options, option);
         }
-        add_dns_option_item(rc->options, option);
-    }
 
-    if (num_nameservers == 0) {
+        if (num_nameservers == 0) {
-        /* If the @l3cd contributes no DNS servers, ignore whether trust-ad is set or unset
+            /* If the @l3cd contributes no DNS servers, ignore whether trust-ad is set or unset
-         * for this @l3cd. */
+             * for this @l3cd. */
-    } else if (has_trust_ad) {
+        } else if (has_trust_ad) {
-        /* We only set has_trust_ad to TRUE, if all IP configs agree (or don't contribute).
+            /* We only set has_trust_ad to TRUE, if all IP configs agree (or don't contribute).
-         * Once set to FALSE, it doesn't get reset. */
+             * Once set to FALSE, it doesn't get reset. */
-        if (rc->has_trust_ad == NM_TERNARY_DEFAULT)
+            if (rc->has_trust_ad == NM_TERNARY_DEFAULT)
-            rc->has_trust_ad = NM_TERNARY_TRUE;
+                rc->has_trust_ad = NM_TERNARY_TRUE;
-    } else
+        } else
-        rc->has_trust_ad = NM_TERNARY_FALSE;
+            rc->has_trust_ad = NM_TERNARY_FALSE;
+    }
 
     if (addr_family == AF_INET) {
         const in_addr_t *nis_servers;
@@ -1231,12 +1237,15 @@ compute_hash(NMDnsManager *self, const NMGlobalDnsConfig *global, guint8 buffer[
 {
     nm_auto_free_checksum GChecksum *sum = NULL;
     NMDnsConfigIPData               *ip_data;
+    gboolean                         has_global_dns_section = FALSE;
 
     sum = g_checksum_new(G_CHECKSUM_SHA1);
     nm_assert(HASH_LEN == g_checksum_type_get_length(G_CHECKSUM_SHA1));
 
-    if (global)
+    if (global) {
         nm_global_dns_config_update_checksum(global, sum);
+        has_global_dns_section = nm_global_dns_has_global_dns_section(global);
+    }
 
     if (!global || !nm_global_dns_config_lookup_domain(global, "*")) {
         const CList *head;
@@ -1248,7 +1257,8 @@ compute_hash(NMDnsManager *self, const NMGlobalDnsConfig *global, guint8 buffer[
             nm_l3_config_data_hash_dns(ip_data->l3cd,
                                        sum,
                                        ip_data->addr_family,
-                                       ip_data->ip_config_type);
+                                       ip_data->ip_config_type,
+                                       has_global_dns_section);
         }
     }
 
@@ -1264,6 +1274,9 @@ merge_global_dns_config(NMResolvConfData *rc, NMGlobalDnsConfig *global_conf)
     const char *const *servers;
     guint              i;
 
+    /* Global config must be processed before connections' config */
+    nm_assert(rc->nameservers->len == 0);
+
     if (!global_conf)
         return FALSE;
 
@@ -1351,12 +1364,17 @@ _collect_resolv_conf_data(NMDnsManager      *self,
             .nis_servers  = g_ptr_array_new(),
             .has_trust_ad = NM_TERNARY_DEFAULT,
     };
+    gboolean has_global_dns_section = FALSE;
 
     priv = NM_DNS_MANAGER_GET_PRIVATE(self);
 
-    if (global_config)
+    if (global_config) {
         merge_global_dns_config(&rc, global_config);
+        has_global_dns_section = nm_global_dns_has_global_dns_section(global_config);
+    }
 
+    /* If global nameservers are defined, no DNS configs are used from connections at all,
+     * including searches and options. */
     if (!global_config || !nm_global_dns_config_lookup_domain(global_config, "*")) {
         nm_auto_str_buf NMStrBuf tmp_strbuf = NM_STR_BUF_INIT(0, FALSE);
         int                      first_prio = 0;
@@ -1390,8 +1408,16 @@ _collect_resolv_conf_data(NMDnsManager      *self,
                   skip ? "<SKIP>" : "",
                   get_nameserver_list(ip_data->addr_family, ip_data->l3cd, &tmp_strbuf));
 
-            if (!skip)
+            if (!skip) {
-                merge_one_l3cd(&rc, ip_data->addr_family, ip_data->data->ifindex, ip_data->l3cd);
+                /* Merge the configs from connections. However, if there was a [global-dns]
+                 * it overwrites searches and options from the connections, thus we only
+                 * merge the nameservers. */
+                merge_one_l3cd(&rc,
+                               ip_data->addr_family,
+                               ip_data->data->ifindex,
+                               ip_data->l3cd,
+                               has_global_dns_section);
+            }
         }
     }
 

src/core/nm-config-data.c

@@ -50,9 +50,9 @@ struct _NMGlobalDnsConfig {
     char           **options;
     GHashTable      *domains;
     const char     **domain_list;
-    gboolean         internal;
     char            *cert_authority;
     NMDnsResolveMode resolve_mode;
+    gboolean         internal;
 };
 
 /*****************************************************************************/
@@ -941,6 +941,14 @@ next:
 
 /*****************************************************************************/
 
+gboolean
+nm_global_dns_has_global_dns_section(const NMGlobalDnsConfig *dns_config)
+{
+    g_return_val_if_fail(dns_config, FALSE);
+
+    return dns_config->searches != NULL || dns_config->options != NULL;
+}
+
 const char *const *
 nm_global_dns_config_get_searches(const NMGlobalDnsConfig *dns_config)
 {
@@ -1236,6 +1244,7 @@ load_global_dns(GKeyFile *keyfile, gboolean internal)
     gs_free char      *cert_authority = NULL;
     gs_free char      *resolve_mode   = NULL;
     NMDnsResolveMode   parsed_resolve_mode;
+    gboolean           has_global_dns_section;
 
     if (internal) {
         group         = NM_CONFIG_KEYFILE_GROUP_INTERN_GLOBAL_DNS;
@@ -1386,6 +1395,22 @@ load_global_dns(GKeyFile *keyfile, gboolean internal)
         return NULL;
     }
 
+    /* Defining [global-dns-domain-*] implies defining [global-dns] too (maybe empty) */
+    if (default_found)
+        has_global_dns_section = TRUE;
+    else
+        has_global_dns_section = g_key_file_has_group(keyfile, group);
+
+    /* If there exist a [global-dns] section, always initialize "searches" and "options" so
+     * they appear in D-Bus. Clients can use this to know if it's defined, so they can know
+     * if DNS configs from connections are relevant or not. */
+    if (has_global_dns_section) {
+        if (!dns_config->searches)
+            dns_config->searches = nm_strv_empty_new();
+        if (!dns_config->options)
+            dns_config->options = nm_strv_empty_new();
+    }
+
     dns_config->internal = internal;
     global_dns_config_seal_domains(dns_config);
     return dns_config;
@@ -1606,17 +1631,6 @@ nm_global_dns_config_from_dbus(const GValue *value, GError **error)
         g_variant_unref(val);
     }
 
-    /* An empty value is valid and clears the internal configuration */
-    if (!nm_global_dns_config_is_empty(dns_config)
-        && !nm_global_dns_config_lookup_domain(dns_config, "*")) {
-        g_set_error_literal(error,
-                            NM_MANAGER_ERROR,
-                            NM_MANAGER_ERROR_FAILED,
-                            "Global DNS configuration is missing the default domain");
-        nm_global_dns_config_free(dns_config);
-        return NULL;
-    }
-
     global_dns_config_seal_domains(dns_config);
     return dns_config;
 }

src/core/nm-config-data.h

@@ -274,6 +274,7 @@ gboolean nm_config_data_is_intern_atomic_group(const NMConfigData *self, const c
 
 GKeyFile *nm_config_data_clone_keyfile_intern(const NMConfigData *self);
 
+gboolean           nm_global_dns_has_global_dns_section(const NMGlobalDnsConfig *dns_config);
 const char *const *nm_global_dns_config_get_searches(const NMGlobalDnsConfig *dns_config);
 const char *const *nm_global_dns_config_get_options(const NMGlobalDnsConfig *dns_config);
 const char *nm_global_dns_config_get_certification_authority(const NMGlobalDnsConfig *dns_config);

src/core/nm-config.c

@@ -18,6 +18,7 @@
 #include "libnm-core-intern/nm-core-internal.h"
 #include "libnm-core-intern/nm-keyfile-internal.h"
 #include "libnm-core-intern/nm-keyfile-utils.h"
+#include "libnm-glib-aux/nm-keyfile-aux.h"
 
 #define DEFAULT_CONFIG_MAIN_FILE     NMCONFDIR "/NetworkManager.conf"
 #define DEFAULT_CONFIG_DIR           NMCONFDIR "/conf.d"
@@ -1046,6 +1047,10 @@ read_config(GKeyFile   *keyfile,
             /* internal groups cannot be set by user configuration. */
             continue;
         }
+
+        if (!g_key_file_has_group(keyfile, group))
+            nm_key_file_add_group(keyfile, group);
+
         keys = g_key_file_get_keys(kf, group, &nkeys, NULL);
         if (!keys)
             continue;
@@ -1639,6 +1644,12 @@ intern_config_read(const char        *filename,
                                  "");
         }
 
+        if (!g_key_file_has_group(keyfile_intern, group)) {
+            nm_key_file_add_group(keyfile_intern, group);
+            if (is_intern)
+                has_intern = TRUE;
+        }
+
         for (k = 0; keys[k]; k++) {
             gs_free char *value_set = NULL;
             const char   *key       = keys[k];
@@ -1823,6 +1834,9 @@ intern_config_write(const char        *filename,
             }
         }
 
+        if (!g_key_file_has_group(keyfile, group))
+            nm_key_file_add_group(keyfile, group);
+
         for (k = 0; keys[k]; k++) {
             const char   *key       = keys[k];
             gs_free char *value_set = NULL;

src/core/nm-l3-config-data.c

@@ -3139,7 +3139,8 @@ void
 nm_l3_config_data_hash_dns(const NML3ConfigData *l3cd,
                            GChecksum            *sum,
                            int                   addr_family,
-                           NMDnsIPConfigType     dns_ip_config_type)
+                           NMDnsIPConfigType     dns_ip_config_type,
+                           gboolean              ignore_searches_and_options)
 {
     guint              i;
     int                val;
@@ -3178,16 +3179,18 @@ nm_l3_config_data_hash_dns(const NML3ConfigData *l3cd,
         empty = FALSE;
     }
 
-    searches = nm_l3_config_data_get_searches(l3cd, addr_family, &num_searches);
+    if (!ignore_searches_and_options) {
-    for (i = 0; i < num_searches; i++) {
+        searches = nm_l3_config_data_get_searches(l3cd, addr_family, &num_searches);
-        g_checksum_update(sum, (const guint8 *) searches[i], strlen(searches[i]));
+        for (i = 0; i < num_searches; i++) {
-        empty = FALSE;
+            g_checksum_update(sum, (const guint8 *) searches[i], strlen(searches[i]));
-    }
+            empty = FALSE;
+        }
 
-    options = nm_l3_config_data_get_dns_options(l3cd, addr_family, &num_options);
+        options = nm_l3_config_data_get_dns_options(l3cd, addr_family, &num_options);
-    for (i = 0; i < num_options; i++) {
+        for (i = 0; i < num_options; i++) {
-        g_checksum_update(sum, (const guint8 *) options[i], strlen(options[i]));
+            g_checksum_update(sum, (const guint8 *) options[i], strlen(options[i]));
-        empty = FALSE;
+            empty = FALSE;
+        }
     }
 
     val = nm_l3_config_data_get_mdns(l3cd);

src/core/nm-l3-config-data.h

@@ -615,6 +615,7 @@ nmtst_l3_config_data_get_best_gateway(const NML3ConfigData *self, int addr_famil
 void nm_l3_config_data_hash_dns(const NML3ConfigData *l3cd,
                                 GChecksum            *sum,
                                 int                   addr_family,
-                                NMDnsIPConfigType     dns_ip_config_type);
+                                NMDnsIPConfigType     dns_ip_config_type,
+                                gboolean              ignore_searches_and_options);
 
 #endif /* __NM_L3_CONFIG_DATA_H__ */

src/core/tests/config/global-dns-empty.conf

@@ -0,0 +1,3 @@
+# Good configuration, an empty global-dns section must be valid
+
+[global-dns]

src/core/tests/config/global-dns-not-set.conf

@@ -0,0 +1,5 @@
+# Good configuration, an empty [global-dns] must be implicitly assumed because a domain is defined
+
+[global-dns-domain-*]
+servers=4.5.6.7
+options=myoption1

src/core/tests/config/test-config.c

@@ -387,7 +387,27 @@ test_config_global_dns(void)
     g_assert(dns);
     g_object_unref(config);
 
-    /* Check that a file with a domain domain, but without a default one gives a NULL configuration */
+    /* Check that a file with an empty global-dns section gives a good configuration.
+     * Check also that searches and options are not NULL, as this is how we expose to
+     * D-Bus that global-dns is defined. */
+    config =
+        setup_config(NULL, TEST_DIR "/global-dns-empty.conf", "", NULL, "/no/such/dir", "", NULL);
+    dns = nm_config_data_get_global_dns_config(nm_config_get_data_orig(config));
+    g_assert(dns);
+    g_assert(nm_global_dns_config_get_searches(dns));
+    g_assert(nm_global_dns_config_get_options(dns));
+    g_object_unref(config);
+
+    /* Check that a file with a domain, but no global-dns, assumes an implicit empty global-dns */
+    config =
+        setup_config(NULL, TEST_DIR "/global-dns-not-set.conf", "", NULL, "/no/such/dir", "", NULL);
+    dns = nm_config_data_get_global_dns_config(nm_config_get_data_orig(config));
+    g_assert(dns);
+    g_assert(nm_global_dns_config_get_searches(dns));
+    g_assert(nm_global_dns_config_get_options(dns));
+    g_object_unref(config);
+
+    /* Check that a file with a domain, but without a default one, gives a NULL configuration */
     config =
         setup_config(NULL, TEST_DIR "/global-dns-invalid.conf", "", NULL, "/no/such/dir", "", NULL);
     dns = nm_config_data_get_global_dns_config(nm_config_get_data_orig(config));