[libvirt] [PATCH 4/5] network: regain guest network connectivity after firewalld switch to nftables

Tue Jan 15 18:18:13 UTC 2019

On 1/15/19 12:23 PM, John Ferlan wrote:
>
> On 1/9/19 9:57 PM, Laine Stump wrote:
>> From: Laine Stump <laine at redhat.com>
>>
>> In the past (when both libvirt and firewalld used iptables), if either
>> libvirt's rules *OR* firewalld's rules accepted a packet, it would be
>> accepted. This was because libvirt and firewalld rules were processed
>> by the same kernel hook.
>>
>> But now firewalld can use nftables for its backend, while libvirt's
>> firewall rules are still using iptables; iptables rules are still
>> processed, but at a different time during packet processing
>> (i.e. during a different hook) than the firewalld nftables rules. The
>> result is that a packet must be accepted by *BOTH* the libvirt
>> iptables rules *AND* the firewalld nftable rules in order to be
>> accepted.
>>
>> This causes pain because
>>
>> 1) libvirt always adds rules to permit DNS and DHCP (and sometimes
>> TFTP) from guests to the local host. But libvirt's bridges are in
>> firewalld's "default" zone (which is usually the zone called
>> "public"). The public zone allows ssh, but doesn't allow DNS, DHCP, or
>> TFTP. So even though libvirt's rules allow the DHCP and DNS traffic,
>> the firewalld rules dont, thus guests connected to libvirt's bridges
>> can't acquire an IP address from DHCP, nor can they make DNS queries
>> to the DNS server libvirt has setup on the host.
>>
>> 2) firewalld's higher level "rich rules" don't yet have the ability to
>> configure the acceptance of forwarded traffic (traffic that is going
>> somewhere beyond the host), so any traffic that needs to be forwarded
>> is rejected by the public zone's default "reject" policy (which
>> rejects all traffic in the zone not specifically allowed by the rules
>> in the zone, whether that traffic is destined to be forwarded or
>> locally received by the host).
>>
>> libvirt can't send "direct" nftables rules (firewalld only supports
>> that for iptables), so we can't solve this problem by just sending
>> explicit nftables rules instead of explicit iptables rules (which, if
>> it could be done, would place libvirt's rules in the same hook as
>> firewalld's native rules, and thus eliminate the need for packets to
>> be accepted by both libvirt's and firewalld's own rules).
>>
>> However, we can take advantage of a quirk in firewalld zones that have
>> a default policy of "accept" (meaning any packet that doesn't match a
>> specific rule in the zone will be *accepted*) - this default accept will
>> also accept forwarded traffic (not just traffic destined for the host).
>>
>> Putting each network's bridge in a new zone called "libvirt" which has
>> a default policy of accept will allow the forwarded traffic to pass,
>> but the same default accept policy that fixes forwarded traffic also
>> causes *all* traffic from guest to host to be accepted. To solve this
>> new problem, we can take advantage of a new feature in firewalld
>> (currently slated for firewalld-0.7.0) - priorities for rich rules -
>> to add a low priority rule that rejects all local traffic (but leaves
>> alone all forwarded traffic).
>>
>> So, our new zone will start with a list of services that are allowed
>> (dhcp, dns, tftp, and ssh to start, but configurable via any firewalld
>> management application, or direct editing of the zone file in
>> /etc/firewalld/zones/libvirt.xml), followed by a low priority
>> <reject/> rule (to reject all other traffic from guest to host), and
>> finally with a default policy of accept (to allow forwarded traffic)
>>
>> After this patch, any network created by libvirt (when firewalld is
>> enabled) will be added to the zone called "libvirt".
>>
>> HOWEVER, even this could be problematic - since the libvirt zone uses
>> a very new feature in firewalld which might not yet be present in the
>> firewalld package on the host. The best we can do is put the zone file
>> in place, and let firewalld try to load it - if firewalld doesn't
>> support rule priorities, it will fail to load the zone file and log an
>> error. Since libvirtd will also be attempting to set the zone of every
>> new interface to "libvirt", if the libvirt zone failed to load, then
>> the call to set the zone of an interface will also fail; this is
>> acceptable because it's a transient problem, and the failure will help
>> alert the user that they need to also update their firewalld package.
>>
>> NB: This behavior *is* slightly different from behavior of previous
>> libvirt (in the past, libvirt network behavior would be affected by
>> the configuration of firewalld's default zone (usually "public"), but
>> now it is affected only by the "libvirt" zone), and thus almost surely
>> warrants a release note for any distro upgrading to libvirt 5.0 or
>> above. Although it's unfortunate that the behavior has to change, the
>> architecture of multiple hooks makes it impossible to *not* change
>> behavior in some way, and the new behavior is arguably better (since
>> it will now be possible to manage access to the host from virtual
>> machines vs from public interfaces separately).
>>
>> NB2: This patch does not check whether the firewalld backend is
>> nftables or iptables; it behaves identically in either case, which is
>> much less confusing than getting different behavior based on the
>> configuration of some other package (firewalld).
>>
>> NB3: firewalld zones can't normally be added to the runtime config of
>> firewalld, so at package install/upgrade time we have to reload all of
>> the firewalld permanent config for the new zone to be recognized. This
>> is done with a call to "firewall-cmd --reload" during postinstall and
>> postuninstall (for rpm-based distros; non-rpm distros will need to
>> figure out a different method of triggering the reload). In the case
>> that firewalld is inactive, firewall-cmd exits without doing anything
>> (i.e. it doesn't start up firewalld.service if it's not already
>> started).
>>
>> Resolves: https://bugzilla.redhat.com/1638342
>> Creates-and-Resolves: https://bugzilla.redhat.com/1650320
>> Signed-off-by: Laine Stump <laine at laine.org>
>> ---
>>   libvirt.spec.in                   | 16 ++++++++++++++++
>>   src/network/Makefile.inc.am       | 10 +++++++++-
>>   src/network/bridge_driver_linux.c |  9 +++++++++
>>   src/network/libvirt.zone          | 14 ++++++++++++++
>>   4 files changed, 48 insertions(+), 1 deletion(-)
>>   create mode 100644 src/network/libvirt.zone
>>
> When the commit message is longer/larger than the patch ;-)

I end up with a lot of those somehow...

>
> /me wonders how much of the above can or even should be described in
> https://libvirt.org/firewall.html.

Yeah, I should probably update that to include nftables and firewalld zones.

>
> For those that really know, understand, care, etc. about firewalls
> getting to understand what role libvirt plays in things could be very
> helpful. Besides you have so much to say now and before you forget it
> again...
>
>> diff --git a/libvirt.spec.in b/libvirt.spec.in
>> index b04cf53eb8..5217fee6ce 100644
>> --- a/libvirt.spec.in
>> +++ b/libvirt.spec.in
>> @@ -389,6 +389,8 @@ BuildRequires: rpcgen
>>   BuildRequires: libtirpc-devel
>>   %endif
>>   
>> +BuildRequires: firewalld-filesystem
>> +
> Should this have a with_firewalld like others below?

Yeah, it should.

>
> Of course I find/note the conditional with_firewalld check around line
> 84 and then the unconditional setting around line 131 to be odd, but for
> consistency...

Interesting. It looks like that came about because we used to force 
enable firewalld on all fedora and all RHEL >= 7, and when we dropped 
support for RHEL6 it just became unconditional. (The same thing happened 
to with_systemd and with_systemd_macros). It would maybe be better if 
that unconditional line was instead:

%define with_firewalld  0%{!?_without_firewalld:1}

so that it was possible to disable it in the build.

>
>>   Provides: bundled(gnulib)
>>   
>>   %description
>> @@ -1352,6 +1354,16 @@ if [ -f %{_localstatedir}/lib/rpm-state/libvirt/restart ]; then
>>   fi
>>   rm -rf %{_localstatedir}/lib/rpm-state/libvirt || :
>>   
>> +%post daemon-driver-network
>> +%if %{with_firewalld}
>> +    %firewalld_reload
>> +%endif
>> +
>> +%postun daemon-driver-network
>> +%if %{with_firewalld}
>> +    %firewalld_reload
>> +%endif
>> +
>>   %post daemon-config-network
>>   if test $1 -eq 1 && test ! -f %{_sysconfdir}/libvirt/qemu/networks/default.xml ; then
>>       # see if the network used by default network creates a conflict,
>> @@ -1590,6 +1602,10 @@ exit 0
>>   %attr(0755, root, root) %{_libexecdir}/libvirt_leaseshelper
>>   %{_libdir}/%{name}/connection-driver/libvirt_driver_network.so
>>   
>> +%if %{with_firewalld}
>> +%{_prefix}/lib/firewalld/zones/libvirt.xml
>> +%endif
>> +
> So something is causing me to wonder if we have to do 'anything' here
> with respect to file security like other %attr's?

Well, according to the Fedora packaging guidelines, the default 
permissions for a file are 0644, and default owner:group is root:root:

https://docs.fedoraproject.org/en-US/packaging-guidelines/#_file_permissions

That will be the case unless defattr() is used to change the defaults, 
and Cole removed all use of defattr() in commit 90f9193cf.

The other zone files in /usr/lib/firewalld/zones (installed by 
firewalld) have permissions 0644 and are owned by root:root, so I think 
this default is okay. (It looks like we only use attr() to set 
permissions/owner if it needs to be something other than 0644/root:root).

(I know that may sound like I actually know what I'm talking about, but 
it's all the result of just looking up a few things based on a memory of 
defattr() being removed :-)