Convert the existing behavior into policies.

This commit has no functional changes.

Signed-off-by: Eric Garver <eric@garver.life>
---
 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

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@garver.life>
> ---
>  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 :|

I'm adding this text here in hopes that Mimecast no longer thinks this
email is s-p-a-m. My replies are inline below. :)

On Thu, May 12, 2022 at 07:35:03PM +0100, Daniel P. Berrangé wrote:
> 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@garver.life>
> > ---
> >  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.

No. That's OUTPUT traffic and would require a policy with
ingress-zones=HOST.

ANY excludes HOST (INPUT, OUTPUT).

> 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 ?

I don't think it's possible. But I'm not certain.

The traffic should be OUTPUT.

> > 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.

That's correct.

If egress-zones=HOST, then it's equivalent to INPUT.
If ingress-zones=HOST, then it's equivalent to OUTPUT.

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

libvirt zone --> public zone (uplink)

That should occur for the default config as "public" is the default zone
for unassigned traffic.

> 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.

Right. The above policy is libvirt --> HOST. Internally the
services/ports/etc added to a zone are implemented as a policy identical
to above. They're functionally equivalent.

The real reason I moved these rules into a policy is so they could be
common with the routed network.

> 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 ?

I think so, but a quick look at the iptables rules generated by libvirt
look like they also allow it.

Chain INPUT (policy ACCEPT 471K packets, 200M bytes)
 pkts bytes target     prot opt in     out     source               destination         
 472K  200M LIBVIRT_INP  all  --  any    any     anywhere             anywhere            

LIBVIRT_INP is non-terminal, so they would be accepted by the INPUT policy (see
the counters?).

> > 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

It's INTRA-zone forwarding. Equivalent to LIBVIRT_FWX.
https://firewalld.org/2020/04/intra-zone-forwarding

i.e. forward between interfaces in the same zone.

iptables equivalent generated by libvirt:

Chain LIBVIRT_FWX (1 references)
pkts bytes target     prot opt in     out     source destination         
0     0 ACCEPT     all  --  virbr0 virbr0  anywhere  anywhere    


>   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: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@garver.life>
> ---
>  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 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

--

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

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

[1]: https://salsa.debian.org/utopia-team/firewalld/-/blob/17fc3126d6eab159f6c703c7e100345fe3450f97/debian/patches/Switch-firewall-backend-from-nftables-back-to-iptables.patch

> > 
> > This commit has no functional changes.
> > 
> > Signed-off-by: Eric Garver <eric@garver.life>
> > ---
> >  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/17fc3126d6eab159f6c703c7e100345fe3450f97/debian/patches/Switch-firewall-backend-from-nftables-back-to-iptables.patch
> 
>>>
>>> This commit has no functional changes.
>>>
>>> Signed-off-by: Eric Garver <eric@garver.life>
>>> ---
>>>   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 Thu, May 12, 2022 at 01:53:00PM -0400, Laine Stump wrote:
> 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).

Seems like that may be the case.

> 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.

That may work.

I was worried about shipping policy files and the running firewalld not
understanding them, but it would simply not load them!

> 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).

You are right. The user modified config will always be a problem. They
won't get future zone/policy updates - those in /etc/firewalld override
the defaults in /usr/lib/firewalld. This issue exists today without this
series though.

The NAT network could continue to use "libvirt" zone as-is.

The routed network could use the new policies only if firewalld > 0.9.0
by adding the interface to the "libvirt-routed" zone. It would to
fallback to the "libvirt" zone otherwise and routed would still work.

After the distributions catch up, then we could transition the "libvirt"
zone to policies as I've done here. But that's optional.

> 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?

Nope. Zones can't reference a policy. Polices reference zones.

> 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?

Right. I checked Arch and Gentoo. They're both 1.1.1 (the latest
release).

> > 
> > [1]: https://salsa.debian.org/utopia-team/firewalld/-/blob/17fc3126d6eab159f6c703c7e100345fe3450f97/debian/patches/Switch-firewall-backend-from-nftables-back-to-iptables.patch
> > 
> > > > 
> > > > This commit has no functional changes.
> > > > 
> > > > Signed-off-by: Eric Garver <eric@garver.life>
> > > > ---
> > > >   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 :|
> > > 
> > 
>