[libvirt] [PATCH] network: Let domains be restricted to local DNS

[Josh Stone]

On 12/02/2014 08:06 AM, Laine Stump wrote:
> On 12/01/2014 01:50 PM, Josh Stone wrote:
>> On 12/01/2014 05:18 AM, Martin Kletzander wrote:
>>> On Mon, Dec 01, 2014 at 07:49:32AM -0500, Laine Stump wrote:
>>>> On 11/30/2014 04:06 PM, Martin Kletzander wrote:
>>>>> On Thu, Nov 20, 2014 at 06:46:41PM -0800, Josh Stone wrote:
>>>>>> This adds a new "localOnly" attribute on the domain element of the
>>>>>> network xml.  With this set to "yes", DNS requests under that domain
>>>>>> will only be resolved by libvirt's dnsmasq, never forwarded upstream.
>>>>>>
>>>>>> This was how it worked before commit f69a6b987d616, and I found that
>>>>>> functionality useful.  For example, I have my host's NetworkManager
>>>>>> dnsmasq configured to forward that domain to libvirt's dnsmasq, so I can
>>>>>> easily resolve guest names from outside.  But if libvirt's dnsmasq
>>>>>> doesn't know a name and forwards it to the host, I'd get an endless
>>>>>> forwarding loop.  Now I can set localOnly="yes" to prevent the loop.
>>>>>>
>>>>> I've found 2 things in this patch I'm not sure about:
>>>>>
>>>>> 1) According to the documentation about --local= parameter, it says:
>>>>>    "Setting this flag does not suppress reading of /etc/resolv.conf,
>>>>>    use -R to do that."  Shouldn't "no-resolv" be added as the option
>>>>>    you want in the config file?
>>>> No, "-R" (aka "no-resolv") tells dnsmasq to not use the servers listed
>>>> in /etc/resolv.conf for *anything*. In this case, all that is wanted is
>>>> to resolve requests _within the configured domain_ only locally, and not
>>>> forward those requests upstream. Requests for any other domain _should_
>>>> be forwarded to whatever server is listed in /etc/resolv.conf. Based on
>>>> a discussion with Josh, I'm certain that this is the behavior he wants
>>>> (and it makes sense to have it available).
>>>>
>>> OK, I just misunderstood this part, sorry for that.
>> Right, I want to keep its own domain local, but go out otherwise.
>>
>>>>> 2) If your answer to the previous question is "NO", which might very
>>>>>    well be the case, for example if you don't want to forward that
>>>>>    one particular domain only (and I misunderstood the commit
>>>>>    message), I think you should just use the ",local" subparameter of
>>>>>    the domain parameter (domain=example.com,local).
>>>> Interesting idea - this requires specifying the IP/prefix of the network
>>>> in the option along with domain (which I suppose isn't a problem), and
>>>> has the same effect as adding local=/domain.com/ as well as the reverse
>>>> resolution, which is a nice plus. However, the prefix for the network
>>>> must be 8, 16, or 24 for this to work (because of the way that
>>>> *.in-addr.arpa records are defined), so we can't do this all the time.
>>>> The question is then - do we write in a special case to do it this way
>>>> when the prefix is one of the right lengths, or just not do it at all?
>>>> Having it "magically happen" for some configs and not others could lead
>>>> to confusion, but having reverse resolution when possible would be very
>>>> useful (for example, some sshd setups have a long delay if the
>>>> reverse-resolve of the client's IP doesn't return something other than
>>>> "not found"; a pointless piece of security theater, but still fairly
>>>> common).
>>>>
>>> Looking at the documentation again, I understand it a bit more yet
>>> again.  Let's assume people use normal domain names.  Libvirt can use
>>> the IP address and netmask from the 'ip' element (if known) and
>>> construct the domain parameter for the config file:
>>>
>>>   domain=virt,192.168.13.0/24,local
>>>
>>> If there's no IP address known, then it could be constructed as
>>> follows:
>>>
>>>   domain=virt
>>>   local=/virt/
>>>
>>> What do you say?
> 
> Yeah, that's what I was suggesting as a possibility above (but wondering
> if it was worth the ambiguity), but according to Josh's testing, there
> is at least one version of dnsmasq that implements
> "domain=blah,x.x.x.x/y,local" incorrectly, so I guess we can't use it,
> at least not by itself.
> 
>>>
>>>
>>> Currently this is possible to achieve with the following in the XML:
>>>
>>>   <domain name='virt,192.168.13.0/24,local'/>
>>>
>>> But that's just plain ugly ;-)
>> Interesting.  If this works, and libvirt promises not to take it away
>> from me later, then it would be enough to satisfy my itch. :)
> 
> I'm actually glad that it didn't work, as even the presence of this
> email could create a precedent that we don't want to exist.
> 
> As a rule, libvirt does not like multiple attributes in a single string;
> if multiple attributes are needed, we try to represent them separately
> in the XML.
> 
> In other words, don't count on the above XML working. As a matter of
> fact, assume that it will be eliminated.

Fair enough, I'll ignore this possibility.

>> However, it doesn't actually seem to do the right thing.  The option
>> does survive from XML to the dnsmasq conf, but it doesn't seem to
>> actually behave as local.  When I manually tweak the conf and restart
>> libvirt's dnsmasq myself, it seems that having separate "domain=foo" and
>> "local=/foo/" terminates properly, but having a oneliner
>> "domain=foo,192.168.122.0/24,local" is still forwarding to the host.
>> Which then forwards back to libvirt, and I get my loop. :(
>>
>> This is dnsmasq-2.72-3.fc21.x86_64.  Maybe dnsmasq only only treats it
>> local when coming from that subnet?  But it's documented as equivalent
>> to local=/foo/, so maybe dnsmasq has a bug here.  Did you try it?
> 
> From the other mail it sounds like you discovered a bug in dnsmasq. But
> libvirt will anyway want to work even with buggy versions of dnsmasq, so
> perhaps localOnly should continue to add local=/foo/, but also add
> ",${ip}/$prefix,local" to the domain option *if* the prefix is 8, 16, or
> 24. I would say this can/should be done in a separate patch (which would
> update the test cases and the docs accordingly).

Once you have to have the local=/domain/ option anyway, the only other
thing you really get is rDNS.  Maybe <ip> or its <dhcp> should have
their own localOnly attribute to restrict the in-addr.arpa separately?

> Back to the code of your patch - the only issue I see with it is what
> John caught (missing the addition to virNetworkDefFormatBuf(), and using
> virTriStateBool(To|From)String rather than comparing to "yes"). Other
> than that, it would have been nice if this config could be in the <dns>
> element, but <domain> is at the top level of network because it is used
> by both dns and dhcp, and this new attribute is most closely associated
> with the domain name, so that's not really possible.

I'll send an update for tristate comparisons and formatting.

Thanks,
Josh