On Wed, May 11, 2022 at 11:41:52AM -0400, Eric Garver wrote: > Convert the existing behavior into policies. Has this split of .zone vs .policy been something firewalld always supported, or is it a "new" feature for some value of "new" ?

Essentially wonder if this has any historical back compat implications for libvirt, given the platforms we target (2 most recent major releases of all distros, so RHEL >= 8 and equiv).

> > This commit has no functional changes. > > Signed-off-by: Eric Garver <eric(a)garver.life&gt; > --- > src/network/libvirt-nat-out.policy | 12 ++++++++++++ > src/network/libvirt-to-host.policy | 20 ++++++++++++++++++++ > src/network/libvirt.zone | 23 +++++------------------ > src/network/meson.build | 10 ++++++++++ > 4 files changed, 47 insertions(+), 18 deletions(-) > create mode 100644 src/network/libvirt-nat-out.policy > create mode 100644 src/network/libvirt-to-host.policy > > diff --git a/src/network/libvirt-nat-out.policy b/src/network/libvirt-nat-out.policy > new file mode 100644 > index 000000000000..7d1cf6dfb4c4 > --- /dev/null > +++ b/src/network/libvirt-nat-out.policy > @@ -0,0 +1,12 @@ > +<?xml version="1.0" encoding="utf-8"?> > +<policy target="ACCEPT"> > + <short>libvirt-nat-out</short> > + > + <description> > + This policy is used to allow NAT virtual machine traffic to the > + rest of the network. > + </description> > + > + <ingress-zone name="libvirt" /> > + <egress-zone name="ANY" /> > +</policy> > diff --git a/src/network/libvirt-to-host.policy b/src/network/libvirt-to-host.policy > new file mode 100644 > index 000000000000..045b35d58d0d > --- /dev/null > +++ b/src/network/libvirt-to-host.policy > @@ -0,0 +1,20 @@ > +<?xml version="1.0" encoding="utf-8"?> > +<policy target="REJECT"> > + <short>libvirt-to-host</short> > + > + <description> > + This policy is used to filter traffic from virtual machines to the > + host. > + </description> > + > + <ingress-zone name="libvirt" /> > + <egress-zone name="HOST" /> > + > + <protocol value='icmp'/> > + <protocol value='ipv6-icmp'/> > + <service name='dhcp'/> > + <service name='dhcpv6'/> > + <service name='dns'/> > + <service name='ssh'/> > + <service name='tftp'/> > +</policy> > diff --git a/src/network/libvirt.zone b/src/network/libvirt.zone > index b1e84b52ecc9..4c5639d8a84f 100644 > --- a/src/network/libvirt.zone > +++ b/src/network/libvirt.zone > @@ -1,25 +1,12 @@ > <?xml version="1.0" encoding="utf-8"?> > -<zone target="ACCEPT"> > +<zone> > <short>libvirt</short> > > <description> > - The default policy of "ACCEPT" allows all packets to/from > - interfaces in the zone to be forwarded, while the (*low priority*) > - reject rule blocks any traffic destined for the host, except those > - services explicitly listed (that list can be modified as required > - by the local admin). This zone is intended to be used only by > - libvirt virtual networks - libvirt will add the bridge devices for > - all new virtual networks to this zone by default. > + This zone is intended to be used only by libvirt virtual networks - > + libvirt will add the bridge devices for all new virtual networks to > + this zone by default. > </description> > > -<rule priority='32767'> > - <reject/> > -</rule> > -<protocol value='icmp'/> > -<protocol value='ipv6-icmp'/> > -<service name='dhcp'/> > -<service name='dhcpv6'/> > -<service name='dns'/> > -<service name='ssh'/> > -<service name='tftp'/> > + <forward /> > </zone> > diff --git a/src/network/meson.build b/src/network/meson.build > index b5eff0c3ab6b..3dd342639a46 100644 > --- a/src/network/meson.build > +++ b/src/network/meson.build > @@ -100,5 +100,15 @@ if conf.has('WITH_NETWORK') > install_dir: prefix / 'lib' / 'firewalld' / 'zones', > rename: [ 'libvirt.xml' ], > ) > + install_data( > + 'libvirt-to-host.policy', > + install_dir: prefix / 'lib' / 'firewalld' / 'policies', > + rename: [ 'libvirt-to-host.xml' ], > + ) > + install_data( > + 'libvirt-nat-out.policy', > + install_dir: prefix / 'lib' / 'firewalld' / 'policies', > + rename: [ 'libvirt-nat-out.xml' ], > + ) > endif > endif > -- > 2.33.0 > With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

On 5/12/22 12:53 PM, Eric Garver wrote: > On Wed, May 11, 2022 at 05:15:25PM +0100, Daniel P. Berrangé wrote: > > On Wed, May 11, 2022 at 11:41:52AM -0400, Eric Garver wrote: > > > Convert the existing behavior into policies. > > > > Has this split of .zone vs .policy been something firewalld > > always supported, or is it a "new" feature for some value > > of "new" ? > > Policies are new in firewalld-0.9.0. > https://firewalld.org/2020/09/policy-objects-introduction > > Policies supplement zones. They do not split or replace them. > > > Essentially wonder if this has any historical back compat > > implications for libvirt, given the platforms we target > > (2 most recent major releases of all distros, so RHEL >= 8 > > and equiv). > > The original zone definition requires firewalld >= 0.7.0. So the > versions we need to worry about with this change are 0.7.z through > 0.8.z. > > At least these distributions (probably non-exhaustive list) have a > firewalld version in that range: > > Ubuntu: > - focal (20.04 LTS) has 0.8.2 > - this is 3 major releases ago, but 2 LTS releases ago Okay, so until we drop support for Ubuntu 20.04 I guess we need to maintain the existing "standalone" libvirt zone (i.e. doesn't use a policy file, and assumes the implied passage of inbound traffic due to the "default accept" behavior of the zone).

We still have a (very outdated and now pointless! note to self: send a patch to remove it!) firewalld version check for a "firewalld < 0.6.0 with nftables backend", so we can easily add a check for firewalld < 0.9.0, use the standalone single libvirt zone in that case, and use separate libvirt-(nat|routed|isolated?) zones when >= 0.9.0. When Ubuntu 20.04 drops from our supported distros, then we can just remove that check along with the standalone "libvirt" zone.

The only quirk here is that anyone who has copied the existing libvirt zone into /etc/firewalld/zones will be surprised when their modified zone is no longer used (as a result of us switching the default zone from "libvirt" to "libvirt-routed" or "libvirt-nat"). If only I'd had the foresight to install separately-named zones from the start :-/ (of course that would have *it's own* set of problems in the case of a zonefile update like this, so I guess I shouldn't beat myself up too much).

Oh, but wait! I just realized - won't an older firewalld complain about a zonefile that references a policy even if no interfaces ever use that zone?

If so, then the packaging for Ubuntu 20.04 may need to only install the old zone file, which would render all the version-check stuff outlined above to be pointless. An associated problem is that the files that create the ubuntu packages are not a part of libvirt's git tree, so making this all work would require updating the out-of-tree build config for a two year old distro (not that they would ever release an official new libvirt version for it, but if we say we support it, that implies that somebody somewhere might want to build an updated libvirt for it). (sigh. Why is nothing ever just simple and straightforward?...) > > -- > > The below distributions should be "good to go": > > RHEL/Fedora: > - RHEL-8 and RHEL-9 have >= 0.9.0. > - f34 and later have >= 0.9.0. > > Debian: > - stable (11, bullseye) has 0.9.2. > - oldstable (10, buster) has 0.6.3 > - defaults to iptables backend [1] so even the original zone is not > necessary > > Ubuntu: > - jammy (22.04 LTS) has 1.1.1 > - impish (21.10) has 0.9.3 repeat sigh. I will never not be annoyed by childish sounding version names :-/ version.numbers++ (/me hangs head in embarrassment at the Fedora "Beefy Miracle" debacle) > > SUSE: > - 15 SP4 has 0.9.3 > - 12 SP5 has 0.4.3.3 (too old to care) > > Note: I didn't investigate rolling release distributions, e.g. Arch, > Gentoo I think by definition those *should* just have the latest version (or close) of every package, shouldn't they?

> > [1]: https://salsa.debian.org/utopia-team/firewalld/-/blob/17fc3126d6eab159f6c... > > > > > > > This commit has no functional changes. > > > > > > Signed-off-by: Eric Garver <eric(a)garver.life&gt; > > > --- > > > src/network/libvirt-nat-out.policy | 12 ++++++++++++ > > > src/network/libvirt-to-host.policy | 20 ++++++++++++++++++++ > > > src/network/libvirt.zone | 23 +++++------------------ > > > src/network/meson.build | 10 ++++++++++ > > > 4 files changed, 47 insertions(+), 18 deletions(-) > > > create mode 100644 src/network/libvirt-nat-out.policy > > > create mode 100644 src/network/libvirt-to-host.policy > > > > > > diff --git a/src/network/libvirt-nat-out.policy b/src/network/libvirt-nat-out.policy > > > new file mode 100644 > > > index 000000000000..7d1cf6dfb4c4 > > > --- /dev/null > > > +++ b/src/network/libvirt-nat-out.policy > > > @@ -0,0 +1,12 @@ > > > +<?xml version="1.0" encoding="utf-8"?> > > > +<policy target="ACCEPT"> > > > + <short>libvirt-nat-out</short> > > > + > > > + <description> > > > + This policy is used to allow NAT virtual machine traffic to the > > > + rest of the network. > > > + </description> > > > + > > > + <ingress-zone name="libvirt" /> > > > + <egress-zone name="ANY" /> > > > +</policy> > > > diff --git a/src/network/libvirt-to-host.policy b/src/network/libvirt-to-host.policy > > > new file mode 100644 > > > index 000000000000..045b35d58d0d > > > --- /dev/null > > > +++ b/src/network/libvirt-to-host.policy > > > @@ -0,0 +1,20 @@ > > > +<?xml version="1.0" encoding="utf-8"?> > > > +<policy target="REJECT"> > > > + <short>libvirt-to-host</short> > > > + > > > + <description> > > > + This policy is used to filter traffic from virtual machines to the > > > + host. > > > + </description> > > > + > > > + <ingress-zone name="libvirt" /> > > > + <egress-zone name="HOST" /> > > > + > > > + <protocol value='icmp'/> > > > + <protocol value='ipv6-icmp'/> > > > + <service name='dhcp'/> > > > + <service name='dhcpv6'/> > > > + <service name='dns'/> > > > + <service name='ssh'/> > > > + <service name='tftp'/> > > > +</policy> > > > diff --git a/src/network/libvirt.zone b/src/network/libvirt.zone > > > index b1e84b52ecc9..4c5639d8a84f 100644 > > > --- a/src/network/libvirt.zone > > > +++ b/src/network/libvirt.zone > > > @@ -1,25 +1,12 @@ > > > <?xml version="1.0" encoding="utf-8"?> > > > -<zone target="ACCEPT"> > > > +<zone> > > > <short>libvirt</short> > > > <description> > > > - The default policy of "ACCEPT" allows all packets to/from > > > - interfaces in the zone to be forwarded, while the (*low priority*) > > > - reject rule blocks any traffic destined for the host, except those > > > - services explicitly listed (that list can be modified as required > > > - by the local admin). This zone is intended to be used only by > > > - libvirt virtual networks - libvirt will add the bridge devices for > > > - all new virtual networks to this zone by default. > > > + This zone is intended to be used only by libvirt virtual networks - > > > + libvirt will add the bridge devices for all new virtual networks to > > > + this zone by default. > > > </description> > > > -<rule priority='32767'> > > > - <reject/> > > > -</rule> > > > -<protocol value='icmp'/> > > > -<protocol value='ipv6-icmp'/> > > > -<service name='dhcp'/> > > > -<service name='dhcpv6'/> > > > -<service name='dns'/> > > > -<service name='ssh'/> > > > -<service name='tftp'/> > > > + <forward /> > > > </zone> > > > diff --git a/src/network/meson.build b/src/network/meson.build > > > index b5eff0c3ab6b..3dd342639a46 100644 > > > --- a/src/network/meson.build > > > +++ b/src/network/meson.build > > > @@ -100,5 +100,15 @@ if conf.has('WITH_NETWORK') > > > install_dir: prefix / 'lib' / 'firewalld' / 'zones', > > > rename: [ 'libvirt.xml' ], > > > ) > > > + install_data( > > > + 'libvirt-to-host.policy', > > > + install_dir: prefix / 'lib' / 'firewalld' / 'policies', > > > + rename: [ 'libvirt-to-host.xml' ], > > > + ) > > > + install_data( > > > + 'libvirt-nat-out.policy', > > > + install_dir: prefix / 'lib' / 'firewalld' / 'policies', > > > + rename: [ 'libvirt-nat-out.xml' ], > > > + ) > > > endif > > > endif > > > -- > > > 2.33.0 > > > > > > > With regards, > > Daniel > > -- > > |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| > > |: https://libvirt.org -o- https://fstop138.berrange.com :| > > |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| > > >

On Wed, May 11, 2022 at 11:41:52AM -0400, Eric Garver wrote: > Convert the existing behavior into policies. > > This commit has no functional changes. > > Signed-off-by: Eric Garver <eric(a)garver.life&gt; > --- > src/network/libvirt-nat-out.policy | 12 ++++++++++++ > src/network/libvirt-to-host.policy | 20 ++++++++++++++++++++ > src/network/libvirt.zone | 23 +++++------------------ > src/network/meson.build | 10 ++++++++++ > 4 files changed, 47 insertions(+), 18 deletions(-) > create mode 100644 src/network/libvirt-nat-out.policy > create mode 100644 src/network/libvirt-to-host.policy > > diff --git a/src/network/libvirt-nat-out.policy b/src/network/libvirt-nat-out.policy > new file mode 100644 > index 000000000000..7d1cf6dfb4c4 > --- /dev/null > +++ b/src/network/libvirt-nat-out.policy > @@ -0,0 +1,12 @@ > +<?xml version="1.0" encoding="utf-8"?> > +<policy target="ACCEPT"> > + <short>libvirt-nat-out</short> > + > + <description> > + This policy is used to allow NAT virtual machine traffic to the > + rest of the network. > + </description> > + > + <ingress-zone name="libvirt" /> > + <egress-zone name="ANY" /> > +</policy> The 'libvirt' zone contains the host bridge device (virbr0, IP address 192.168.122.1). The VMs are implicitly in 'libvirt' zone because their TAP devices are members of virbr0. So this policy allows traffic originating in the VMs to forwarded to any other zone. IIUC, it would also allow traffic originating from the host via the virbr0 device to forward to any other zone. In practice the only traffic originating on the host from virbr0, should be traffic destined for the guest.

Is it possible to force traffic on host destined for the LAN to originate in virbr0, and forward with NAT, instead of just originating in eth0 and get onto the LAN without forwarding ? If it is possible, do we actually care ?

> diff --git a/src/network/libvirt-to-host.policy b/src/network/libvirt-to-host.policy > new file mode 100644 > index 000000000000..045b35d58d0d > --- /dev/null > +++ b/src/network/libvirt-to-host.policy > @@ -0,0 +1,20 @@ > +<?xml version="1.0" encoding="utf-8"?> > +<policy target="REJECT"> > + <short>libvirt-to-host</short> > + > + <description> > + This policy is used to filter traffic from virtual machines to the > + host. > + </description> > + > + <ingress-zone name="libvirt" /> > + <egress-zone name="HOST" /> > + > + <protocol value='icmp'/> > + <protocol value='ipv6-icmp'/> > + <service name='dhcp'/> > + <service name='dhcpv6'/> > + <service name='dns'/> > + <service name='ssh'/> > + <service name='tftp'/> > +</policy> So this applies to traffic originating in the VM, and destined for the host. I'm fuzzy on exactly what "HOST" expands to in our context.

IIUC 'policy' rules apply to traffic transitting between zones, but we shouldn't have any zone transits involved.

virbr0 is in the 'libvirt' zones and VM TAP devs are part of virbr0. So traffic VM -> host should be entirely within the libvirt zone, never transitting zones.

Does this use of "HOST", for example allow traffic from the VMs to connect to the host via its public IP address on eth0, instead of via its private IP on virbr0 which is what we want ?

> diff --git a/src/network/libvirt.zone b/src/network/libvirt.zone > index b1e84b52ecc9..4c5639d8a84f 100644 > --- a/src/network/libvirt.zone > +++ b/src/network/libvirt.zone > @@ -1,25 +1,12 @@ > <?xml version="1.0" encoding="utf-8"?> > -<zone target="ACCEPT"> > +<zone> > <short>libvirt</short> > > <description> > - The default policy of "ACCEPT" allows all packets to/from > - interfaces in the zone to be forwarded, while the (*low priority*) > - reject rule blocks any traffic destined for the host, except those > - services explicitly listed (that list can be modified as required > - by the local admin). This zone is intended to be used only by > - libvirt virtual networks - libvirt will add the bridge devices for > - all new virtual networks to this zone by default. > + This zone is intended to be used only by libvirt virtual networks - > + libvirt will add the bridge devices for all new virtual networks to > + this zone by default. > </description> > > -<rule priority='32767'> > - <reject/> > -</rule> > -<protocol value='icmp'/> > -<protocol value='ipv6-icmp'/> > -<service name='dhcp'/> > -<service name='dhcpv6'/> > -<service name='dns'/> > -<service name='ssh'/> > -<service name='tftp'/> > + <forward /> What does '<forward/>' do - i'm not actually finding it mentioned in the zone docs at

https://firewalld.org/documentation/man-pages/firewalld.zone.html > </zone> With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

On Wed, May 11, 2022 at 11:41:54AM -0400, Eric Garver wrote: > Signed-off-by: Eric Garver <eric(a)garver.life&gt; > --- > src/network/libvirt-routed-in.policy | 11 +++++++++++ > src/network/libvirt-routed-out.policy | 12 ++++++++++++ > src/network/meson.build | 10 ++++++++++ > 3 files changed, 33 insertions(+) > create mode 100644 src/network/libvirt-routed-in.policy > create mode 100644 src/network/libvirt-routed-out.policy > > diff --git a/src/network/libvirt-routed-in.policy b/src/network/libvirt-routed-in.policy > new file mode 100644 > index 000000000000..baf8822d747c > --- /dev/null > +++ b/src/network/libvirt-routed-in.policy > @@ -0,0 +1,11 @@ > +<?xml version="1.0" encoding="utf-8"?> > +<policy target="ACCEPT"> > + <short>libvirt-routed-out</short> > + > + <description> > + This policy is used to allow routed traffic to the virtual machines. > + </description> > + > + <ingress-zone name="ANY" /> > + <egress-zone name="libvirt-routed" /> > +</policy> Same as the NAT version of the policy so makes sense. > diff --git a/src/network/libvirt-routed-out.policy b/src/network/libvirt-routed-out.policy > new file mode 100644 > index 000000000000..efa0030569d6 > --- /dev/null > +++ b/src/network/libvirt-routed-out.policy > @@ -0,0 +1,12 @@ > +<?xml version="1.0" encoding="utf-8"?> > +<policy target="ACCEPT"> > + <short>libvirt-routed-out</short> > + > + <description> > + This policy is used to allow routed virtual machine traffic to the rest of > + the network. > + </description> > + > + <ingress-zone name="libvirt-routed" /> > + <egress-zone name="ANY" /> > +</policy> This is much more permissive than what I expected. Doesn't this allow the VMs to have unrestricted access to anything on the host ?

At a libvirt POV, the NAT and routed zones should be identical, with the only difference being whether masquerading is applied.

In terms of VM -> host, we still only want to allow the small set of services, dns, dhcp, ssh AFAIK.

With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

Signed-off-by: Eric Garver <eric(a)garver.life&gt; --- src/network/libvirt-routed-out.policy | 12 ++++++++++++ src/network/meson.build | 5 +++++ 2 files changed, 17 insertions(+) create mode 100644 src/network/libvirt-routed-out.policy

diff --git a/src/network/libvirt-routed-out.policy b/src/network/libvirt-routed-out.policy new file mode 100644 index 000000000000..efa0030569d6 --- /dev/null +++ b/src/network/libvirt-routed-out.policy @@ -0,0 +1,12 @@ +<?xml version="1.0" encoding="utf-8"?> +<policy target="ACCEPT"> + <short>libvirt-routed-out</short> + + <description> + This policy is used to allow routed virtual machine traffic to the rest of + the network. + </description> + + <ingress-zone name="libvirt-routed" /> + <egress-zone name="ANY" /> +</policy> diff --git a/src/network/meson.build b/src/network/meson.build index cd52e2a54c28..36d9b51a2cf9 100644 --- a/src/network/meson.build +++ b/src/network/meson.build @@ -115,5 +115,10 @@ if conf.has('WITH_NETWORK') install_dir: prefix / 'lib' / 'firewalld' / 'policies', rename: [ 'libvirt-nat-out.xml' ], ) + install_data( + 'libvirt-routed-out.policy', + install_dir: prefix / 'lib' / 'firewalld' / 'policies', + rename: [ 'libvirt-routed-out.xml' ], + ) endif endif -- 2.33.0