Re: [PATCH v2] network: introduce a "none" firewall backend type

On Fri, Jun 14, 2024 at 03:43:53PM GMT, Daniel P. Berrangé wrote: > meson.build | 26 +++++++++++++++++++------- > meson_options.txt | 2 +- > src/network/bridge_driver_conf.c | 19 ++++++++++++++----- > src/network/bridge_driver_linux.c | 10 ++++++++++ > src/network/bridge_driver_nop.c | 15 ++++++++++++++- > src/util/virfirewall.c | 6 ++++++ > src/util/virfirewall.h | 1 + > 7 files changed, 65 insertions(+), 14 deletions(-) The test suite no longer passes after applying this. At the very least, you need to squash in the diff at the bottom of this message.

> firewall_backend_priority = get_option('firewall_backend_priority') > - if (not firewall_backend_priority.contains('nftables') or > - not firewall_backend_priority.contains('iptables') or > - firewall_backend_priority.length() != 2) > - error('invalid value for firewall_backend_priority option') > + if firewall_backend_priority.length() == 0 > + if host_machine.system() == 'linux' > + firewall_backend_priority = ['nftables', 'iptables'] > + else > + # No firewall impl on non-Linux so far, so force 'none' > + # as placeholder > + firewall_backend_priority = ['none'] > + endif > + else > + if host_machine.system() != 'linux' > + error('firewall backend priority only supported on linux hosts') > + endif > endif This implementation allows things such as -Dfirewall_backend_priority=nftables

and -Dfirewall_backend_priority=iptables,iptables At least -Dfirewall_backend_priority=iptables,nftables,iptables will be blocked, but only because it results in a compilation error: meson will happily accept it. Are we okay with that? It's IMO inferior to the much stricter checking that's performed today.

> @@ -815,6 +816,11 @@ virFirewallApplyCmd(virFirewall *firewall, > switch (virFirewallGetBackend(firewall)) { > + case VIR_FIREWALL_BACKEND_NONE: > + virReportError(VIR_ERR_NO_SUPPORT, > + _("Firewall backend is not implemented")); > + return -1; This check (and the other ones you've introduced) are running too late, which leads to this behavior: $ cat default-session.xml <network> <name>default-session</name> <forward mode='nat'/> <bridge name='virbr0' stp='on' delay='0'/> <ip address='192.168.123.1' netmask='255.255.255.0'> <dhcp> <range start='192.168.123.2' end='192.168.123.254'/> </dhcp> </ip> </network> $ virsh -c qemu:///session net-define default-session.xml Network default-session defined from default-session.xml $ virsh -c qemu:///session net-start default-session error: Failed to start network default-session error: error creating bridge interface virbr0: Operation not permitted Since <forward mode='nat'/> requires firewall rules, we shouldn't allow a network that uses it to be defined in the first place.

diff --git a/po/POTFILES b/po/POTFILES index 4bfbb91164..1ed4086d2c 100644 --- a/po/POTFILES +++ b/po/POTFILES @@ -143,6 +143,7 @@ src/lxc/lxc_process.c src/network/bridge_driver.c src/network/bridge_driver_conf.c src/network/bridge_driver_linux.c +src/network/bridge_driver_nop.c src/network/leaseshelper.c src/network/network_iptables.c src/network/network_nftables.c diff --git a/src/network/bridge_driver_nop.c b/src/network/bridge_driver_nop.c index 2114d521d1..323990cf17 100644 --- a/src/network/bridge_driver_nop.c +++ b/src/network/bridge_driver_nop.c @@ -49,8 +49,8 @@ int networkAddFirewallRules(virNetworkDef *def G_GNUC_UNUSED, */ if (firewallBackend != VIR_FIREWALL_BACKEND_NONE) { virReportError(VIR_ERR_NO_SUPPORT, - _("Firewall backend '%s' not available on this platform"), - virFirewallBackendTypeToString(firewallBackend)); + _("Firewall backend '%1$s' not available on this platform"), + virFirewallBackendTypeToSTring(firewallBackend)); return -1; } return 0; diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c index d374f54b64..090dbcdbed 100644 --- a/src/util/virfirewall.c +++ b/src/util/virfirewall.c @@ -817,7 +817,7 @@ virFirewallApplyCmd(virFirewall *firewall, switch (virFirewallGetBackend(firewall)) { case VIR_FIREWALL_BACKEND_NONE: - virReportError(VIR_ERR_NO_SUPPORT, + virReportError(VIR_ERR_NO_SUPPORT, "%s", _("Firewall backend is not implemented")); return -1; -- Andrea Bolognani / Red Hat / Virtualization