In some use cases we don't want the virtual network's DNS to only listen to the vnet interface. Adding a publiclyAccessible attribute to the dns element in the configuration allows the DNS to listen to all interfaces. It simply disables the bind-dynamic option of dnsmasq for the network. --- docs/formatnetwork.html.in | 11 +++++++++++ docs/schemas/network.rng | 15 ++++++++++----- src/conf/network_conf.c | 6 ++++++ src/conf/network_conf.h | 1 + src/network/bridge_driver.c | 4 +++- tests/networkxml2confdata/nat-network-dns-hosts.conf | 1 - tests/networkxml2confdata/nat-network-dns-hosts.xml | 2 +- 7 files changed, 32 insertions(+), 8 deletions(-) diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in index 6abed8f..8e43658 100644 --- a/docs/formatnetwork.html.in +++ b/docs/formatnetwork.html.in @@ -851,6 +851,17 @@ DNS server. </p> + <p> + The dns element + can have an optional <code>publiclyAccessible</code> + attribute <span class="since">Since 1.2.17</span>. + If <code>publiclyAccessible</code> is "yes", then the DNS server + will handle requests for all interfaces. + If <code>publiclyAccessible</code> is not set or "no", the DNS + server will only handle requests for the interface of the virtual + network. + </p> + Currently supported sub-elements of <code>&lt;dns&gt;</code> are: <dl> <dt><code>forwarder</code></dt> diff --git a/docs/schemas/network.rng b/docs/schemas/network.rng index 4edb6eb..f989625 100644 --- a/docs/schemas/network.rng +++ b/docs/schemas/network.rng @@ -244,12 +244,17 @@ and other features in the <dns> element --> <optional> <element name="dns"> - <optional> - <attribute name="forwardPlainNames"> - <ref name="virYesNo"/> - </attribute> - </optional> <interleave> + <optional> + <attribute name="forwardPlainNames"> + <ref name="virYesNo"/> + </attribute> + </optional> + <optional> + <attribute name="publiclyAccessible"> + <ref name="virYesNo"/> + </attribute> + </optional>

<zeroOrMore> <element name="forwarder"> <attribute name="addr"><ref name="ipAddr"/></attribute> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index f4a9df0..99bac6d 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -1309,9 +1309,14 @@ virNetworkDNSDefParseXML(const char *networkName, size_t i; int ret = -1; xmlNodePtr save = ctxt->node; + char *publiclyAccessible = NULL; ctxt->node = node; + publiclyAccessible = virXPathString("string(./@publiclyAccessible)", ctxt); + if (publiclyAccessible) + def->publiclyAccessible = virTristateBoolTypeFromString(publiclyAccessible); + forwardPlainNames = virXPathString("string(./@forwardPlainNames)", ctxt); if (forwardPlainNames) { def->forwardPlainNames = virTristateBoolTypeFromString(forwardPlainNames); @@ -1410,6 +1415,7 @@ virNetworkDNSDefParseXML(const char *networkName, ret = 0; cleanup: + VIR_FREE(publiclyAccessible); VIR_FREE(forwardPlainNames); VIR_FREE(fwdNodes); VIR_FREE(hostNodes); diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index f69d999..f555b6b 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -136,6 +136,7 @@ struct _virNetworkDNSDef { virNetworkDNSSrvDefPtr srvs; size_t nfwds; char **forwarders; + int publiclyAccessible; /* enum virTristateBool */ }; typedef struct _virNetworkIpDef virNetworkIpDef; diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index d195085..c39b1a5 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -996,8 +996,10 @@ networkDnsmasqConfContents(virNetworkObjPtr network, * other than one of the virtual guests connected directly to * this network). This was added in response to CVE 2012-3411. */ + if (network->def->dns.publiclyAccessible != VIR_TRISTATE_BOOL_YES) + virBufferAddLit(&configbuf, + "bind-dynamic\n"); virBufferAsprintf(&configbuf, - "bind-dynamic\n" "interface=%s\n", network->def->bridge); } else { diff --git a/tests/networkxml2confdata/nat-network-dns-hosts.conf b/tests/networkxml2confdata/nat-network-dns-hosts.conf index 021316f..759a9e9 100644 --- a/tests/networkxml2confdata/nat-network-dns-hosts.conf +++ b/tests/networkxml2confdata/nat-network-dns-hosts.conf @@ -10,6 +10,5 @@ expand-hosts domain-needed local=// except-interface=lo -bind-dynamic interface=virbr0 addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts diff --git a/tests/networkxml2confdata/nat-network-dns-hosts.xml b/tests/networkxml2confdata/nat-network-dns-hosts.xml index 9add456..969dfa5 100644 --- a/tests/networkxml2confdata/nat-network-dns-hosts.xml +++ b/tests/networkxml2confdata/nat-network-dns-hosts.xml @@ -4,7 +4,7 @@ <forward dev='eth0' mode='nat'/> <bridge name='virbr0' stp='on' delay='0'/> <domain name="example.com"/> - <dns forwardPlainNames='no'> + <dns forwardPlainNames='no' publiclyAccessible='yes'> <host ip='192.168.122.1'> <hostname>host</hostname> <hostname>gateway</hostname>

On 06/01/2015 07:54 AM, Cédric Bosdonnat wrote: > In some use cases we don't want the virtual network's DNS to only > listen to the vnet interface. Adding a publiclyAccessible attribute

> to the dns element in the configuration allows the DNS to listen to > all interfaces. > > It simply disables the bind-dynamic option of dnsmasq for the network. > --- > docs/formatnetwork.html.in | 11 +++++++++++ > docs/schemas/network.rng | 15 ++++++++++----- > src/conf/network_conf.c | 6 ++++++ > src/conf/network_conf.h | 1 + > src/network/bridge_driver.c | 4 +++- > tests/networkxml2confdata/nat-network-dns-hosts.conf | 1 - > tests/networkxml2confdata/nat-network-dns-hosts.xml | 2 +- > 7 files changed, 32 insertions(+), 8 deletions(-) > > diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in > index 6abed8f..8e43658 100644 > --- a/docs/formatnetwork.html.in > +++ b/docs/formatnetwork.html.in > @@ -851,6 +851,17 @@ > DNS server. > </p> > > + <p> > + The dns element > + can have an optional <code>publiclyAccessible</code> > + attribute <span class="since">Since 1.2.17</span>. > + If <code>publiclyAccessible</code> is "yes", then the DNS server > + will handle requests for all interfaces. > + If <code>publiclyAccessible</code> is not set or "no", the DNS > + server will only handle requests for the interface of the virtual > + network. > + </p> > + > Currently supported sub-elements of <code>&lt;dns&gt;</code> are: > <dl> > <dt><code>forwarder</code></dt> > diff --git a/docs/schemas/network.rng b/docs/schemas/network.rng > index 4edb6eb..f989625 100644 > --- a/docs/schemas/network.rng > +++ b/docs/schemas/network.rng > @@ -244,12 +244,17 @@ > and other features in the <dns> element --> > <optional> > <element name="dns"> > - <optional> > - <attribute name="forwardPlainNames"> > - <ref name="virYesNo"/> > - </attribute> > - </optional> > <interleave> > + <optional> > + <attribute name="forwardPlainNames"> > + <ref name="virYesNo"/> > + </attribute> > + </optional> > + <optional> > + <attribute name="publiclyAccessible"> > + <ref name="virYesNo"/> > + </attribute> > + </optional> Moving the attributes inside the <interleave> had me looking through other .rng's... I'm no expert, but had thought they really only mattered for <element>'s

> <zeroOrMore> > <element name="forwarder"> > <attribute name="addr"><ref name="ipAddr"/></attribute> > diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c > index f4a9df0..99bac6d 100644 > --- a/src/conf/network_conf.c > +++ b/src/conf/network_conf.c > @@ -1309,9 +1309,14 @@ virNetworkDNSDefParseXML(const char *networkName, > size_t i; > int ret = -1; > xmlNodePtr save = ctxt->node; > + char *publiclyAccessible = NULL; > > ctxt->node = node; > > + publiclyAccessible = virXPathString("string(./@publiclyAccessible)", ctxt); > + if (publiclyAccessible) > + def->publiclyAccessible = virTristateBoolTypeFromString(publiclyAccessible); > + > forwardPlainNames = virXPathString("string(./@forwardPlainNames)", ctxt); > if (forwardPlainNames) { > def->forwardPlainNames = virTristateBoolTypeFromString(forwardPlainNames); > @@ -1410,6 +1415,7 @@ virNetworkDNSDefParseXML(const char *networkName, > > ret = 0; > cleanup: > + VIR_FREE(publiclyAccessible); > VIR_FREE(forwardPlainNames); > VIR_FREE(fwdNodes); > VIR_FREE(hostNodes); > diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h > index f69d999..f555b6b 100644 > --- a/src/conf/network_conf.h > +++ b/src/conf/network_conf.h > @@ -136,6 +136,7 @@ struct _virNetworkDNSDef { > virNetworkDNSSrvDefPtr srvs; > size_t nfwds; > char **forwarders; > + int publiclyAccessible; /* enum virTristateBool */ > }; > > typedef struct _virNetworkIpDef virNetworkIpDef; > diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c > index d195085..c39b1a5 100644 > --- a/src/network/bridge_driver.c > +++ b/src/network/bridge_driver.c > @@ -996,8 +996,10 @@ networkDnsmasqConfContents(virNetworkObjPtr network, > * other than one of the virtual guests connected directly to > * this network). This was added in response to CVE 2012-3411. > */ > + if (network->def->dns.publiclyAccessible != VIR_TRISTATE_BOOL_YES) > + virBufferAddLit(&configbuf, > + "bind-dynamic\n"); > virBufferAsprintf(&configbuf, > - "bind-dynamic\n" > "interface=%s\n", > network->def->bridge); > } else { > diff --git a/tests/networkxml2confdata/nat-network-dns-hosts.conf b/tests/networkxml2confdata/nat-network-dns-hosts.conf > index 021316f..759a9e9 100644 > --- a/tests/networkxml2confdata/nat-network-dns-hosts.conf > +++ b/tests/networkxml2confdata/nat-network-dns-hosts.conf > @@ -10,6 +10,5 @@ expand-hosts > domain-needed > local=// > except-interface=lo > -bind-dynamic > interface=virbr0 > addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts > diff --git a/tests/networkxml2confdata/nat-network-dns-hosts.xml b/tests/networkxml2confdata/nat-network-dns-hosts.xml > index 9add456..969dfa5 100644 > --- a/tests/networkxml2confdata/nat-network-dns-hosts.xml > +++ b/tests/networkxml2confdata/nat-network-dns-hosts.xml > @@ -4,7 +4,7 @@ > <forward dev='eth0' mode='nat'/> > <bridge name='virbr0' stp='on' delay='0'/> > <domain name="example.com"/> > - <dns forwardPlainNames='no'> > + <dns forwardPlainNames='no' publiclyAccessible='yes'> > <host ip='192.168.122.1'> > <hostname>host</hostname> > <hostname>gateway</hostname> > Rather than change an existing test, a new test or two should be created... One that specifically states 'yes' and possibly one that has 'no' keeping the existing one with nothing provided to be sure that works as well. I don't mind doing that for you, but also I figure by bumping this perhaps Laine will take a look too since he usually responds to most of the network related patches anyway... It seems fine to me though.

On 06/10/2015 03:56 PM, John Ferlan wrote: > > On 06/01/2015 07:54 AM, Cédric Bosdonnat wrote: >> In some use cases we don't want the virtual network's DNS to only >> listen to the vnet interface. Adding a publiclyAccessible attribute :-) Really, that name was only intended as a placeholder! I was hoping you (or someone else) would be able to find something shorter/simpler. Lacking that, I guess this is a reasonable name though.

>> to the dns element in the configuration allows the DNS to listen to >> all interfaces. >> >> It simply disables the bind-dynamic option of dnsmasq for the network. >> --- >> docs/formatnetwork.html.in | 11 +++++++++++ >> docs/schemas/network.rng | 15 ++++++++++----- >> src/conf/network_conf.c | 6 ++++++ >> src/conf/network_conf.h | 1 + >> src/network/bridge_driver.c | 4 +++- >> tests/networkxml2confdata/nat-network-dns-hosts.conf | 1 - >> tests/networkxml2confdata/nat-network-dns-hosts.xml | 2 +- >> 7 files changed, 32 insertions(+), 8 deletions(-) >> >> diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in >> index 6abed8f..8e43658 100644 >> --- a/docs/formatnetwork.html.in >> +++ b/docs/formatnetwork.html.in >> @@ -851,6 +851,17 @@ >> DNS server. >> </p> >> >> + <p> >> + The dns element >> + can have an optional <code>publiclyAccessible</code> >> + attribute <span class="since">Since 1.2.17</span>. >> + If <code>publiclyAccessible</code> is "yes", then the DNS server >> + will handle requests for all interfaces. >> + If <code>publiclyAccessible</code> is not set or "no", the DNS >> + server will only handle requests for the interface of the virtual >> + network. >> + </p> >> + >> Currently supported sub-elements of <code>&lt;dns&gt;</code> are: >> <dl> >> <dt><code>forwarder</code></dt> >> diff --git a/docs/schemas/network.rng b/docs/schemas/network.rng >> index 4edb6eb..f989625 100644 >> --- a/docs/schemas/network.rng >> +++ b/docs/schemas/network.rng >> @@ -244,12 +244,17 @@ >> and other features in the <dns> element --> >> <optional> >> <element name="dns"> >> - <optional> >> - <attribute name="forwardPlainNames"> >> - <ref name="virYesNo"/> >> - </attribute> >> - </optional> >> <interleave> >> + <optional> >> + <attribute name="forwardPlainNames"> >> + <ref name="virYesNo"/> >> + </attribute> >> + </optional> >> + <optional> >> + <attribute name="publiclyAccessible"> >> + <ref name="virYesNo"/> >> + </attribute> >> + </optional> > Moving the attributes inside the <interleave> had me looking through > other .rng's... I'm no expert, but had thought they really only mattered > for <element>'s I'm not an expert either, but you are correct :-)

>> <zeroOrMore> >> <element name="forwarder"> >> <attribute name="addr"><ref name="ipAddr"/></attribute> >> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c >> index f4a9df0..99bac6d 100644 >> --- a/src/conf/network_conf.c >> +++ b/src/conf/network_conf.c >> @@ -1309,9 +1309,14 @@ virNetworkDNSDefParseXML(const char *networkName, >> size_t i; >> int ret = -1; >> xmlNodePtr save = ctxt->node; >> + char *publiclyAccessible = NULL; >> >> ctxt->node = node; >> >> + publiclyAccessible = virXPathString("string(./@publiclyAccessible)", ctxt); >> + if (publiclyAccessible) >> + def->publiclyAccessible = virTristateBoolTypeFromString(publiclyAccessible); >> + >> forwardPlainNames = virXPathString("string(./@forwardPlainNames)", ctxt); >> if (forwardPlainNames) { >> def->forwardPlainNames = virTristateBoolTypeFromString(forwardPlainNames); >> @@ -1410,6 +1415,7 @@ virNetworkDNSDefParseXML(const char *networkName, >> >> ret = 0; >> cleanup: >> + VIR_FREE(publiclyAccessible); >> VIR_FREE(forwardPlainNames); >> VIR_FREE(fwdNodes); >> VIR_FREE(hostNodes); >> diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h >> index f69d999..f555b6b 100644 >> --- a/src/conf/network_conf.h >> +++ b/src/conf/network_conf.h >> @@ -136,6 +136,7 @@ struct _virNetworkDNSDef { >> virNetworkDNSSrvDefPtr srvs; >> size_t nfwds; >> char **forwarders; >> + int publiclyAccessible; /* enum virTristateBool */ >> }; >> >> typedef struct _virNetworkIpDef virNetworkIpDef; >> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c >> index d195085..c39b1a5 100644 >> --- a/src/network/bridge_driver.c >> +++ b/src/network/bridge_driver.c >> @@ -996,8 +996,10 @@ networkDnsmasqConfContents(virNetworkObjPtr network, >> * other than one of the virtual guests connected directly to >> * this network). This was added in response to CVE 2012-3411. >> */ >> + if (network->def->dns.publiclyAccessible != VIR_TRISTATE_BOOL_YES) >> + virBufferAddLit(&configbuf, >> + "bind-dynamic\n"); >> virBufferAsprintf(&configbuf, >> - "bind-dynamic\n" >> "interface=%s\n", >> network->def->bridge); >> } else { >> diff --git a/tests/networkxml2confdata/nat-network-dns-hosts.conf b/tests/networkxml2confdata/nat-network-dns-hosts.conf >> index 021316f..759a9e9 100644 >> --- a/tests/networkxml2confdata/nat-network-dns-hosts.conf >> +++ b/tests/networkxml2confdata/nat-network-dns-hosts.conf >> @@ -10,6 +10,5 @@ expand-hosts >> domain-needed >> local=// >> except-interface=lo >> -bind-dynamic >> interface=virbr0 >> addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts >> diff --git a/tests/networkxml2confdata/nat-network-dns-hosts.xml b/tests/networkxml2confdata/nat-network-dns-hosts.xml >> index 9add456..969dfa5 100644 >> --- a/tests/networkxml2confdata/nat-network-dns-hosts.xml >> +++ b/tests/networkxml2confdata/nat-network-dns-hosts.xml >> @@ -4,7 +4,7 @@ >> <forward dev='eth0' mode='nat'/> >> <bridge name='virbr0' stp='on' delay='0'/> >> <domain name="example.com"/> >> - <dns forwardPlainNames='no'> >> + <dns forwardPlainNames='no' publiclyAccessible='yes'> >> <host ip='192.168.122.1'> >> <hostname>host</hostname> >> <hostname>gateway</hostname> >> > Rather than change an existing test, a new test or two should be > created... One that specifically states 'yes' and possibly one that has > 'no' keeping the existing one with nothing provided to be sure that > works as well.

> I don't mind doing that for you, but also I figure by bumping this > perhaps Laine will take a look too since he usually responds to most of > the network related patches anyway... It seems fine to me though. This is better than the previously proposed "bindDynamic" option (or whatever it was called; I just remember it was something dnsmasq-specific). However, the patch doesn't put "bind-interfaces" in place of "bind-dynamic", and unless I'm remembering incorrectly, removing bind-dynamic without adding "bind-interfaces" will cause multiple dnsmasq instances to conflict with each other - when neither option is used, dnsmasq will create a single socket listening on the wildcard address, then discard packets not intended for the IP address/interface; if any application listens on a given port using the wildcard address, no other application will be able to listen on that port on *any* interface/address. Here's an email that describes the differences: http://lists.thekelleys.org.uk/pipermail/dnsmasq-discuss/2012q4/006525.html Beyond that, I don't think that the name/description of this new option ("publiclyAccessible") really fits the original problem that I believe prompted the creation of the patch. As I understand it, the original problem was that LXC guests connected to a libvirt network were unable to get their IP address via dhcp (and probably weren't able to use libvirt's DNS server) because the packets were identified as arriving on the host side of the guest's veth interface rather than on the libvirt network's bridge device (the previous patch for this said something about vlans - was it only a problem in the case of vlans?).

I don't have a problem pushing this patch (after modifying it to use bind-interfaces when bind-dynamic isn't used, adding some new unit tests as John described, and copious amounts of regression testing, e.g. multiple simultaneous networks with a mixture of publiclyAccessible='yes|no', running a systemwide instance of dnsmasq with bind-dynamic//bind-interfaces at the same time, routed and nated networks, qemu and lxc guests with/without dhcp, etc).

But I think that we should still try to find a solution to the original problem that doesn't involve opening up the DNS and DHCP servers to receiving packets that arrived on *any* interface. (Cedric - perhaps you could provide a config for something that doesn't work so that I can be sure I'm understanding exactly what is the problem).

On 06/16/2015 09:08 AM, Cedric Bosdonnat wrote: > Hi Laine, > > On Thu, 2015-06-11 at 13:37 -0400, Laine Stump wrote: >> On 06/10/2015 03:56 PM, John Ferlan wrote: >>> On 06/01/2015 07:54 AM, Cédric Bosdonnat wrote: >>>> In some use cases we don't want the virtual network's DNS to only >>>> listen to the vnet interface. Adding a publiclyAccessible attribute >> :-) Really, that name was only intended as a placeholder! I was hoping >> you (or someone else) would be able to find something shorter/simpler. >> Lacking that, I guess this is a reasonable name though. > Given the other names around that didn't shock me, but it's surely not a > good habit to introduce such lengthy names ;) Well, "forwardPlainNames" was another invention of mine, and one which I'm not proud of, but I gave fair published notice that I would accept other suggestions, and still couldn't come up with something better. The trick is in getting the balance between short/cryptic and long/unambiguously descriptive right. The worst outcome is to have something short and cryptic that could easily be misunderstood to mean something else. Do you think "public" is specific enough? Or might that possibly be confused with some other intent?

>>>> to the dns element in the configuration allows the DNS to listen to >>>> all interfaces. >>>> >>>> It simply disables the bind-dynamic option of dnsmasq for the network. >>>> --- >>>> docs/formatnetwork.html.in | 11 +++++++++++ >>>> docs/schemas/network.rng | 15 ++++++++++----- >>>> src/conf/network_conf.c | 6 ++++++ >>>> src/conf/network_conf.h | 1 + >>>> src/network/bridge_driver.c | 4 +++- >>>> tests/networkxml2confdata/nat-network-dns-hosts.conf | 1 - >>>> tests/networkxml2confdata/nat-network-dns-hosts.xml | 2 +- >>>> 7 files changed, 32 insertions(+), 8 deletions(-) >>>> >>>> diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in >>>> index 6abed8f..8e43658 100644 >>>> --- a/docs/formatnetwork.html.in >>>> +++ b/docs/formatnetwork.html.in >>>> @@ -851,6 +851,17 @@ >>>> DNS server. >>>> </p> >>>> >>>> + <p> >>>> + The dns element >>>> + can have an optional <code>publiclyAccessible</code> >>>> + attribute <span class="since">Since 1.2.17</span>. >>>> + If <code>publiclyAccessible</code> is "yes", then the DNS server >>>> + will handle requests for all interfaces. >>>> + If <code>publiclyAccessible</code> is not set or "no", the DNS >>>> + server will only handle requests for the interface of the virtual >>>> + network. >>>> + </p> >>>> + >>>> Currently supported sub-elements of <code>&lt;dns&gt;</code> are: >>>> <dl> >>>> <dt><code>forwarder</code></dt> >>>> diff --git a/docs/schemas/network.rng b/docs/schemas/network.rng >>>> index 4edb6eb..f989625 100644 >>>> --- a/docs/schemas/network.rng >>>> +++ b/docs/schemas/network.rng >>>> @@ -244,12 +244,17 @@ >>>> and other features in the <dns> element --> >>>> <optional> >>>> <element name="dns"> >>>> - <optional> >>>> - <attribute name="forwardPlainNames"> >>>> - <ref name="virYesNo"/> >>>> - </attribute> >>>> - </optional> >>>> <interleave> >>>> + <optional> >>>> + <attribute name="forwardPlainNames"> >>>> + <ref name="virYesNo"/> >>>> + </attribute> >>>> + </optional> >>>> + <optional> >>>> + <attribute name="publiclyAccessible"> >>>> + <ref name="virYesNo"/> >>>> + </attribute> >>>> + </optional> >>> Moving the attributes inside the <interleave> had me looking through >>> other .rng's... I'm no expert, but had thought they really only mattered >>> for <element>'s >> I'm not an expert either, but you are correct :-) > I'm fixing that one. > >>>> <zeroOrMore> >>>> <element name="forwarder"> >>>> <attribute name="addr"><ref name="ipAddr"/></attribute> >>>> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c >>>> index f4a9df0..99bac6d 100644 >>>> --- a/src/conf/network_conf.c >>>> +++ b/src/conf/network_conf.c >>>> @@ -1309,9 +1309,14 @@ virNetworkDNSDefParseXML(const char *networkName, >>>> size_t i; >>>> int ret = -1; >>>> xmlNodePtr save = ctxt->node; >>>> + char *publiclyAccessible = NULL; >>>> >>>> ctxt->node = node; >>>> >>>> + publiclyAccessible = virXPathString("string(./@publiclyAccessible)", ctxt); >>>> + if (publiclyAccessible) >>>> + def->publiclyAccessible = virTristateBoolTypeFromString(publiclyAccessible); >>>> + >>>> forwardPlainNames = virXPathString("string(./@forwardPlainNames)", ctxt); >>>> if (forwardPlainNames) { >>>> def->forwardPlainNames = virTristateBoolTypeFromString(forwardPlainNames); >>>> @@ -1410,6 +1415,7 @@ virNetworkDNSDefParseXML(const char *networkName, >>>> >>>> ret = 0; >>>> cleanup: >>>> + VIR_FREE(publiclyAccessible); >>>> VIR_FREE(forwardPlainNames); >>>> VIR_FREE(fwdNodes); >>>> VIR_FREE(hostNodes); >>>> diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h >>>> index f69d999..f555b6b 100644 >>>> --- a/src/conf/network_conf.h >>>> +++ b/src/conf/network_conf.h >>>> @@ -136,6 +136,7 @@ struct _virNetworkDNSDef { >>>> virNetworkDNSSrvDefPtr srvs; >>>> size_t nfwds; >>>> char **forwarders; >>>> + int publiclyAccessible; /* enum virTristateBool */ >>>> }; >>>> >>>> typedef struct _virNetworkIpDef virNetworkIpDef; >>>> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c >>>> index d195085..c39b1a5 100644 >>>> --- a/src/network/bridge_driver.c >>>> +++ b/src/network/bridge_driver.c >>>> @@ -996,8 +996,10 @@ networkDnsmasqConfContents(virNetworkObjPtr network, >>>> * other than one of the virtual guests connected directly to >>>> * this network). This was added in response to CVE 2012-3411. >>>> */ >>>> + if (network->def->dns.publiclyAccessible != VIR_TRISTATE_BOOL_YES) >>>> + virBufferAddLit(&configbuf, >>>> + "bind-dynamic\n"); >>>> virBufferAsprintf(&configbuf, >>>> - "bind-dynamic\n" >>>> "interface=%s\n", >>>> network->def->bridge); >>>> } else { >>>> diff --git a/tests/networkxml2confdata/nat-network-dns-hosts.conf b/tests/networkxml2confdata/nat-network-dns-hosts.conf >>>> index 021316f..759a9e9 100644 >>>> --- a/tests/networkxml2confdata/nat-network-dns-hosts.conf >>>> +++ b/tests/networkxml2confdata/nat-network-dns-hosts.conf >>>> @@ -10,6 +10,5 @@ expand-hosts >>>> domain-needed >>>> local=// >>>> except-interface=lo >>>> -bind-dynamic >>>> interface=virbr0 >>>> addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts >>>> diff --git a/tests/networkxml2confdata/nat-network-dns-hosts.xml b/tests/networkxml2confdata/nat-network-dns-hosts.xml >>>> index 9add456..969dfa5 100644 >>>> --- a/tests/networkxml2confdata/nat-network-dns-hosts.xml >>>> +++ b/tests/networkxml2confdata/nat-network-dns-hosts.xml >>>> @@ -4,7 +4,7 @@ >>>> <forward dev='eth0' mode='nat'/> >>>> <bridge name='virbr0' stp='on' delay='0'/> >>>> <domain name="example.com"/> >>>> - <dns forwardPlainNames='no'> >>>> + <dns forwardPlainNames='no' publiclyAccessible='yes'> >>>> <host ip='192.168.122.1'> >>>> <hostname>host</hostname> >>>> <hostname>gateway</hostname> >>>> >>> Rather than change an existing test, a new test or two should be >>> created... One that specifically states 'yes' and possibly one that has >>> 'no' keeping the existing one with nothing provided to be sure that >>> works as well. > Ok, will do. > >>> I don't mind doing that for you, but also I figure by bumping this >>> perhaps Laine will take a look too since he usually responds to most of >>> the network related patches anyway... It seems fine to me though. >> This is better than the previously proposed "bindDynamic" option (or >> whatever it was called; I just remember it was something dnsmasq-specific). >> >> However, the patch doesn't put "bind-interfaces" in place of >> "bind-dynamic", and unless I'm remembering incorrectly, removing >> bind-dynamic without adding "bind-interfaces" will cause multiple >> dnsmasq instances to conflict with each other - when neither option is >> used, dnsmasq will create a single socket listening on the wildcard >> address, then discard packets not intended for the IP address/interface; >> if any application listens on a given port using the wildcard address, >> no other application will be able to listen on that port on *any* >> interface/address. Here's an email that describes the differences: >> >> >> http://lists.thekelleys.org.uk/pipermail/dnsmasq-discuss/2012q4/006525.html >> >> Beyond that, I don't think that the name/description of this new option >> ("publiclyAccessible") really fits the original problem that I believe >> prompted the creation of the patch. As I understand it, the original >> problem was that LXC guests connected to a libvirt network were unable >> to get their IP address via dhcp (and probably weren't able to use >> libvirt's DNS server) because the packets were identified as arriving on >> the host side of the guest's veth interface rather than on the libvirt >> network's bridge device (the previous patch for this said something >> about vlans - was it only a problem in the case of vlans?). > The problem comes with people setting VLANs on top of the libvirt > network. Then the DHCP / DNS requests are arriving on veth0.x instead of > veth0... and our dnsmasq doesn't take these requests. It sounds almost like a misuse of the libvirt network, and if that's the case then I don't really care about a separate patch that fixes just that problem - if you're already hacking up the network outside the scope of libvirt (in a way that's not supported by libvirt), then it's reasonable you should need to hack it up a bit more to make it work. (after reading your description at the end, yes it is an abuse of libvirt's networks. See down at the bottom for my full opinion) >> I don't have a problem pushing this patch (after modifying it to use >> bind-interfaces when bind-dynamic isn't used, adding some new unit tests >> as John described, and copious amounts of regression testing, e.g. >> multiple simultaneous networks with a mixture of >> publiclyAccessible='yes|no', running a systemwide instance of dnsmasq >> with bind-dynamic//bind-interfaces at the same time, routed and nated >> networks, qemu and lxc guests with/without dhcp, etc). > Can all those tests be automated in our test suites? No, I'm talking about tests that should be run at least once manually by you prior to considering this "done". Any ACK given on the patch by me would be under the assumption that you had run such tests locally. If you want to add them to one of the automated test suites, that would be even better (are people still running the libvirt-tck?)

> Apart from the > configuration tests mentioned by John, those you mention really look > like integration tests to be done somewhere else. In order to detect future regressions at a later time, yes. To be sure that your new feature works properly and doesn't cause regressions in other features when it is used, they should at least be setup and run manually when the feature is added.

> >> But I think that >> we should still try to find a solution to the original problem that >> doesn't involve opening up the DNS and DHCP servers to receiving packets >> that arrived on *any* interface. (Cedric - perhaps you could provide a >> config for something that doesn't work so that I can be sure I'm >> understanding exactly what is the problem). > To reproduce the problem: > > * On the host, create a virbr0.300 vlan interface, assign it an IP > * Create a guest using the default network (or the one creating > virbr0) > * In the guest, create an eth0.300 vlan device. > > In this setup all DHCP and DNS requests won't be handled by virbr0's > dnsmasq, as it doesn't listen to the virbr0.* interfaces. > > After some more tests, I found out that we can have this in the dnsmasq > config: > > interface=virbr0 > interface=virbr0.* > > Even with bind-dynamic, this would make the dnsmasq listen on the vlans > of the bridge. The problems I see with such an approach to fix the vlan > problem are: > * some people / tools don't use the dev.vlanid naming convention, so > it wouldn't work with names like vlan0 > * I guess we can imaging people setting interfaces named like > virbr0.hacked to match the wildcard, even though it's not a VLAN. > > Any thoughts on that? Actually after seeing this description, I think it's something that libvirt doesn't need to try and explicitly fix. People who do this are already hacking around with libvirt's network in a way which wasn't intended, so I don't think it's necessary/desirable to make it work by default, or even to provide a separate specific option to make it work - if someone wants to do this, they deserve what they get, and requiring them to use bind-interfaces instead of bind-dynamic (i.e. to turn on "publiclyAccessible") is perfectly acceptable.

I would suggest maybe coming up with a patch to properly support this config in libvirt's network config, but it's unclear to me what is the advantage of creating a vlan on a network that isn't connected to anywhere else at L2 anyway - it just creates a need to configure a vlan interface in the guest (which you may or may not trust the guest to do), and ends up only serving to isolate one group of guests connected to that network from another group of guests connected to that network (as long as the guests themselves choose to honor the isolation! If the guests want though, they can easily get around this). It seems to me that a better idea would be to just create a separate libvirt network (i.e. a separate bridge device) for each group of guests and forget about burdening everyone with the extra task of setting up a vlan interface.