[libvirt] [PATCHv2 1/2] network: only prevent forwarding of DNS requests for unqualified names

[Laine Stump]

In commit f386825 we began adding the options

  --domain-needed
  --local=/$mydomain/

to all dnsmasq commandlines with the stated reason of preventing
forwarding of DNS queries for names that weren't fully qualified
domain names ("FQDN", i.e. a name that included some "."s and a domain
name). This was later changed to

  domain-needed
  local=/$mydomain/

when we moved the options from the dnsmasq commandline to a conf file.

The original patch on the list, and discussion about it, is here:

  https://www.redhat.com/archives/libvir-list/2012-August/msg01594.html

When a domain name isn't specified (mydomain == ""), the addition of
"domain-needed local=//" will prevent forwarding of domain-less
requests to the virtualization host's DNS resolver, but if a domain
*is* specified, the addition of "local=/domain/" will prevent
forwarding of any requests for *qualified* names within that domain
that aren't resolvable by libvirt's dnsmasq itself.

An example of the problems this causes - let's say a network is
defined with:

   <domain name='example.com'/>
   <dhcp>
      ..
      <host mac='52:54:00:11:22:33' ip='1.2.3.4' name='myguest'/>
   </dhcp>

This results in "local=/example.com/" being added to the dnsmasq options.

If a guest requests "myguest" or "myguest.example.com", that will be
resolved by dnsmasq. If the guest asks for "www.example.com", dnsmasq
will not know the answer, but instead of forwarding it to the host, it
will return NOT FOUND to the guest. In most cases that isn't the
behavior an admin is looking for.

A later patch (commit 4f595ba) attempted to remedy this by adding a
"forwardPlainNames" attribute to the <dns> element. The idea was that
if forwardPlainNames='yes' (default is 'no'), we would allow
unresolved names to be forwarded. However, that patch was botched, in
that it only removed the "domain-needed" option when
forwardPlainNames='yes', and left the "local=/mydomain/".

Really we should have been just including the option "--domain-needed
--local=//" (note the lack of domain name) regardless of the
configured domain of the network, so that requests for names without a
domain would be treated as "local to dnsmasq" and not forwarded, but
all others (including those in the network's configured domain) would
be forwarded. We also shouldn't include *either* of those options if
forwardPlainNames='yes'. This patch makes those corrections.

This patch doesn't remedy the fact that default behavior was changed
by the addition of this feature. That will be handled in a subsequent
patch.
---
 src/network/bridge_driver.c                              | 16 ++++++----------
 tests/networkxml2confdata/dhcp6-network.conf             |  4 ++--
 .../networkxml2confdata/nat-network-dns-forwarders.conf  |  2 +-
 tests/networkxml2confdata/nat-network-dns-hosts.conf     |  4 ++--
 tests/networkxml2confdata/netboot-network.conf           |  4 ++--
 tests/networkxml2confdata/netboot-proxy-network.conf     |  4 ++--
 6 files changed, 15 insertions(+), 19 deletions(-)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 3b9b58d..f50795b 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -710,9 +710,6 @@ networkDnsmasqConfContents(virNetworkObjPtr network,
                       "strict-order\n",
                       network->def->name);
 
-    if (!network->def->dns.forwardPlainNames)
-        virBufferAddLit(&configbuf, "domain-needed\n");
-
     if (network->def->dns.forwarders) {
         virBufferAddLit(&configbuf, "no-resolv\n");
         for (i = 0; i < network->def->dns.nfwds; i++) {
@@ -728,14 +725,13 @@ networkDnsmasqConfContents(virNetworkObjPtr network,
                           network->def->domain);
     }
 
-    if (network->def->domain || !network->def->dns.forwardPlainNames) {
-        /* need to specify local even if no domain specified, unless
-         * the config says we should forward "plain" names (i.e. not
-         * fully qualified, no '.' characters)
+    if (!network->def->dns.forwardPlainNames) {
+        virBufferAddLit(&configbuf, "domain-needed\n");
+        /* need to specify local=// whether or not a domain is
+         * specified, unless the config says we should forward "plain"
+         * names (i.e. not fully qualified, no '.' characters)
          */
-        virBufferAsprintf(&configbuf,
-                          "local=/%s/\n",
-                          network->def->domain ? network->def->domain : "");
+        virBufferAddLit(&configbuf, "local=//\n");
     }
 
     if (pidfile)
diff --git a/tests/networkxml2confdata/dhcp6-network.conf b/tests/networkxml2confdata/dhcp6-network.conf
index 5fde07f..92ea2a4 100644
--- a/tests/networkxml2confdata/dhcp6-network.conf
+++ b/tests/networkxml2confdata/dhcp6-network.conf
@@ -5,10 +5,10 @@
 ##
 ## dnsmasq conf file created by libvirt
 strict-order
-domain-needed
 domain=mynet
 expand-hosts
-local=/mynet/
+domain-needed
+local=//
 except-interface=lo
 bind-dynamic
 interface=virbr0
diff --git a/tests/networkxml2confdata/nat-network-dns-forwarders.conf b/tests/networkxml2confdata/nat-network-dns-forwarders.conf
index ebca289..5f95f77 100644
--- a/tests/networkxml2confdata/nat-network-dns-forwarders.conf
+++ b/tests/networkxml2confdata/nat-network-dns-forwarders.conf
@@ -5,10 +5,10 @@
 ##
 ## dnsmasq conf file created by libvirt
 strict-order
-domain-needed
 no-resolv
 server=8.8.8.8
 server=8.8.4.4
+domain-needed
 local=//
 except-interface=lo
 bind-dynamic
diff --git a/tests/networkxml2confdata/nat-network-dns-hosts.conf b/tests/networkxml2confdata/nat-network-dns-hosts.conf
index 2577882..021316f 100644
--- a/tests/networkxml2confdata/nat-network-dns-hosts.conf
+++ b/tests/networkxml2confdata/nat-network-dns-hosts.conf
@@ -5,10 +5,10 @@
 ##
 ## dnsmasq conf file created by libvirt
 strict-order
-domain-needed
 domain=example.com
 expand-hosts
-local=/example.com/
+domain-needed
+local=//
 except-interface=lo
 bind-dynamic
 interface=virbr0
diff --git a/tests/networkxml2confdata/netboot-network.conf b/tests/networkxml2confdata/netboot-network.conf
index b6f3c23..ce33176 100644
--- a/tests/networkxml2confdata/netboot-network.conf
+++ b/tests/networkxml2confdata/netboot-network.conf
@@ -5,10 +5,10 @@
 ##
 ## dnsmasq conf file created by libvirt
 strict-order
-domain-needed
 domain=example.com
 expand-hosts
-local=/example.com/
+domain-needed
+local=//
 except-interface=lo
 bind-interfaces
 listen-address=192.168.122.1
diff --git a/tests/networkxml2confdata/netboot-proxy-network.conf b/tests/networkxml2confdata/netboot-proxy-network.conf
index 1e969fa..f4d3880 100644
--- a/tests/networkxml2confdata/netboot-proxy-network.conf
+++ b/tests/networkxml2confdata/netboot-proxy-network.conf
@@ -5,10 +5,10 @@
 ##
 ## dnsmasq conf file created by libvirt
 strict-order
-domain-needed
 domain=example.com
 expand-hosts
-local=/example.com/
+domain-needed
+local=//
 except-interface=lo
 bind-interfaces
 listen-address=192.168.122.1
-- 
1.8.5.3