[libvirt] [PATCHv2 2/2] network: add netmask to dhcp range of dnsmasq conf file for IPv4

[Laine Stump]

On 2/20/19 4:10 PM, John Ferlan wrote:
>
> On 2/18/19 6:21 PM, Laine Stump wrote:
>> dnsmasq documentation says that the *IPv4* prefix/network
>> address/broadcast address sent to dhcp clients will be automatically
>> determined by dnsmasq by looking at the interface it's listening on,
>> so the original libvirt code did not add a netmask to the dnsmasq
>> commandline (or later, the dnsmasq conf file).
>>
>> For *IPv6* however, dnsmasq apparently cannot automatically determine
>> the prefix (functionally the same as a netmask), and it must be
>> explicitly provided in the conf file (as a part of the dhcp-range
>> option). So many years after IPv4 DHCP support had been added, when
>> IPv6 dhcp support was added the prefix was included at the end of the
>> dhcp-range setting, but only for IPv6.
>>
>> Recently a user reported (privately, because they suspected a possible
>> security implication, which turned out to be unfounded) a bug on a
>> host where one of the interfaces was a superset of the libvirt network
>> where dhcp is needed (e.g., the host's ethernet is 10.0.0.20/8, and
>> the libvirt network is 10.10.0.1/24). For some reason dnsmasq was
>> supplying the netmask for the /8 network to clients requesting an
>> address on the /24 interface.
>>
>> This seems like a bug in dnsmasq, but even if/when it gets fixed
>> there, it looks like there is no harm in just always adding the
>> netmask to all IPv4 dhcp-range options similar to how prefix is added
>> to all IPv6 dhcp-range options.
>>
>> Signed-off-by: Laine Stump <laine at laine.org>
>> ---
>>   src/network/bridge_driver.c                   | 27 +++++++++++++++----
>>   .../dhcp6-nat-network.conf                    |  2 +-
>>   .../networkxml2confdata/isolated-network.conf |  2 +-
>>   .../nat-network-dns-srv-record-minimal.conf   |  2 +-
>>   .../nat-network-dns-srv-record.conf           |  2 +-
>>   .../nat-network-dns-txt-record.conf           |  2 +-
>>   .../networkxml2confdata/nat-network-mtu.conf  |  2 +-
>>   .../nat-network-name-with-quotes.conf         |  2 +-
>>   tests/networkxml2confdata/nat-network.conf    |  2 +-
>>   .../networkxml2confdata/netboot-network.conf  |  2 +-
>>   .../netboot-proxy-network.conf                |  2 +-
>>   .../networkxml2confdata/ptr-domains-auto.conf |  2 +-
>>   12 files changed, 33 insertions(+), 16 deletions(-)
>>
>> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
>> index 6d80818e40..9fa902896b 100644
>> --- a/src/network/bridge_driver.c
>> +++ b/src/network/bridge_driver.c
>> @@ -1320,11 +1320,28 @@ networkDnsmasqConfContents(virNetworkObjPtr obj,
>>                   !(eaddr = virSocketAddrFormat(&ipdef->ranges[r].end)))
>>                   goto cleanup;
>>   
>> -            virBufferAsprintf(&configbuf, "dhcp-range=%s,%s",
>> -                              saddr, eaddr);
>> -            if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET6))
>> -                virBufferAsprintf(&configbuf, ",%d", prefix);
>> -            virBufferAddLit(&configbuf, "\n");
>> +            if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET6)) {
>> +               virBufferAsprintf(&configbuf, "dhcp-range=%s,%s,%d\n",
>> +                                 saddr, eaddr, prefix);
>> +            } else {
>> +                /* IPv4 - dnsmasq requires a netmask rather than prefix */
>> +                virSocketAddr netmask;
> Does it matter if this isn't initialized? e.g. "= { 0 };"


None of the other instances of virSocketAddr are initialized to 0, and 
the intent is that the utility functions should fill in all fields that 
are relevant. I'd rather not be the one responsible for starting a cargo 
cult of explicitly initializing them when it's not necessary :-)


>
>> +                char *netmaskStr;
>      VIR_AUTOFREE(char *) netmaskStr = NULL;
>
>> +
>> +                if (virSocketAddrPrefixToNetmask(prefix, &netmask, AF_INET) < 0) {
>> +                    virReportError(VIR_ERR_INTERNAL_ERROR,
>> +                                   _("Failed to translate bridge '%s' "
>> +                                     "prefix %d to netmask"),
>> +                                   def->bridge, prefix);
>> +                    goto cleanup;
>> +                }
>> +
>> +                if (!(netmaskStr = virSocketAddrFormat(&netmask)))
>> +                    goto cleanup;
>> +                virBufferAsprintf(&configbuf, "dhcp-range=%s,%s,%s\n",
>> +                                  saddr, eaddr, netmaskStr);
>> +                VIR_FREE(netmaskStr);
> w/ VIR_AUTOFREE, this isn't necessary
>
> I know bridge_driver doesn't use it yet, but doesn't harm to start.


Yeah, I'll do that before I push.


>
> Reviewed-by: John Ferlan <jferlan at redhat.com>
>
> John
>
> [...]
>