On 1/15/19 12:23 PM, John Ferlan wrote:
On 1/9/19 9:57 PM, Laine Stump wrote:
From: Laine Stump <laine@xxxxxxxxxx>
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@xxxxxxxxx>
---
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 :-)
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
