Re: [libvirt] [PATCH v2 5/7] network: set firewalld zone of bridges to "libvirt" zone when appropriate

On Thu, Jan 31, 2019 at 08:24:56PM -0500, Laine Stump wrote: > From: Laine Stump <laine(a)redhat.com&gt; > > This patch restores broken guest network connectivity after a host > firewalld is switched to using an nftables backend. It does this by > adding libvirt networks' bridge interfaces to the new "libvirt" zone > in firewalld. > > After this patch, the bridge interface of any network created by > libvirt (when firewalld is active) will be added to the firewalld > zone called "libvirt" if it exists (regardless of the firewalld > backend setting). This behavior does *not* depend on whether or not > libvirt has installed the libvirt zone file (set with > "--with[out]-firewalld-zone" during the configure phase of the package > build). > > If the libvirt zone doesn't exist (either because the package was > configured to not install it, or possibly it was installed, but > firewalld doesn't support rule priorities, resulting in a parse > error), the bridge will remain in firewalld's default zone, which > could be innocuous (in the case that the firewalld backend is > iptables, guest networking will still function properly with the > bridge in the default zone), or it could be disastrous (if the > firewalld backend is nftables, we can be assured that guest networking > will fail). In order to be unobtrusive in the former case, and > informative in the latter, when the libvirt zone doesn't exist we > then check the firewalld version to see if it's new enough to support > the nftables backend, and then if the backend is actually set to > nftables, before logging an error (and failing the net-start > operation, since the network couldn't possibly work anyway). > > When the libvirt zone is used, network 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.1 or above. Although it's > unfortunate that we have to deal with a mandatory behavior 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). > > Resolves: https://bugzilla.redhat.com/1638342 > Creates-and-Resolves: https://bugzilla.redhat.com/1650320 > Signed-off-by: Laine Stump <laine(a)laine.org&gt; > --- > > NB: I had considered that it might be useful to cache the results of > checking the list of active zones, the firewalld version, or the > firewalld backend, but in my tests of restarting libvirtd with 100 > active networks, the full startup time (from the beginning of > "systemctl restart libvirtd.service" until successful return from a > subsequent "virsh list") showed 42 seconds and a bit regardless of > whether or not I made those checks. This tells me that the amount of > time to be saved by caching the results of a single call vs calling > once for each network are insignificant relative to everything else > that is being done. Because any cached values would need to be stored > in the network driver state object, and thus require acquiring the > driver-wide lock to update at potentially very different times > (e.g. in the response to a dbus message informing us that firewalld > was restarted, as well as while starting a new network from an API > call) I consider the chance of a bug in my code causing an obscure > deadlock sometime in the future to be a much greater concern than > maybe saving 1/10th of a second out of 42 (and lock contention might > eliminate the gain anyway(), so I have left the code to retrieve the > list of zones once for each network start. > > Changes in V2: > > * split off from Patch 5. This patch only sets the libvirt zone if it > exists (and attempts to somewhat document the behavior in > firewall.html), it doesn't install the libvirt zone. > > * check for existence of libvirt zone before attempting to set it. > > * if libvirt zone doesn't exist, only log an error in the case that > the firewalld version is new enough to have an nftables backend, and > the backend is actually set to nftables. > > > docs/firewall.html.in | 33 +++++++++++++++++++++ > src/network/bridge_driver_linux.c | 48 +++++++++++++++++++++++++++++++ > 2 files changed, 81 insertions(+) > > diff --git a/docs/firewall.html.in b/docs/firewall.html.in > index 0a50687c26..5d584e582e 100644 > --- a/docs/firewall.html.in > +++ b/docs/firewall.html.in > @@ -129,6 +129,39 @@ MASQUERADE all -- * * 192.168.122.0/24 !192.168.122.0/24</pre> > </li> > </ul> > > + <h3><a id="fw-firewalld-and-virtual-network-driver">firewalld and the virtual network driver</a> > + </h3> > + <p> > + If <a href="https://firewalld.org">firewalld</a> is active on > + the host, libvirt will attempt to place the bridge interface of > + a libvirt virtual network into the firewalld zone named > + "libvirt" (thus making all guest->host traffic on that network > + subject to the rules of the "libvirt" zone). This is done > + because, if firewalld is using its nftables backend (available > + since firewalld 0.6.0) the default firewalld zone (which would > + be used if libvirt didn't explicitly set the zone) prevents > + forwarding traffic from guests through the bridge, as well as > + preventing DHCP, DNS, and most other traffic from guests to > + host. The zone named "libvirt" is installed into the firewalld > + configuration by libvirt (not by firewalld), and allows > + forwarded traffic through the bridge as well as DHCP, DNS, TFTP, > + and SSH traffic to the host - depending on firewalld's backend > + this will be implemented via either iptables or nftables > + rules. libvirt's own rules outlined above will *always* be > + iptables rules regardless of which backend is in use by > + firewalld. > + </p> > + <p> > + NB: Prior to libvirt 5.1.0, the firewalld "libvirt" zone did not > + exist, and prior to firewalld 0.7.0 a feature crucial to making > + the "libvirt" zone operate properly (rich rule priority > + settings) was not implemented in firewalld. In cases where one > + or the other of the two packages is missing the necessary > + functionality, it's still possible to have functional guest > + networking by setting the firewalld backend to "iptables" (in > + firewalld prior to 0.6.0, this was the only backend available). > + </p> > + > <h3><a id="fw-network-filter-driver">The network filter driver</a> > </h3> > <p>This driver provides a fully configurable network filtering capability > diff --git a/src/network/bridge_driver_linux.c b/src/network/bridge_driver_linux.c > index e5e48c90f1..9d2e6877ae 100644 > --- a/src/network/bridge_driver_linux.c > +++ b/src/network/bridge_driver_linux.c > @@ -27,6 +27,7 @@ > #include "virstring.h" > #include "virlog.h" > #include "virfirewall.h" > +#include "virfirewalld.h" > > #define VIR_FROM_THIS VIR_FROM_NONE > > @@ -670,6 +671,53 @@ int networkAddFirewallRules(virNetworkDefPtr def) > virFirewallPtr fw = NULL; > int ret = -1; > > + /* if firewalld is active, try to set the "libvirt" zone. This is > + * desirable (for consistency) if firewalld is using the iptables > + * backend, but is necessary (for basic network connectivity) if > + * firewalld is using the nftables backend > + */ > + if (virFirewallDIsRegistered() == 0) { The indentation here is odd - its 4 spaces too much for the surrounding context.

With that fixed Reviewed-by: Daniel P. Berrangé <berrange(a)redhat.com&gt; Regards, Daniel