Daniel P. Berrangé wrote:
> There are two scenarios identified after the recent firewall backend
> selection was introduced, which result in libvirtd failing to startup
> due to an inability to find either iptables/nftables
>
> - On Linux if running unprivileged with $PATH lacking the dir
> containing iptables/nftables
> - On non-Linux where iptables/nftables never existed
>
> In the former case, it is preferrable to restore the behaviour whereby
> the driver starts successfully. Users will get an error reported when
> attempting to start any virtual network, due to the lack of permissions
> needed to create bridge devices. This makes the missing firewall backend
> irrelevant.
>
> In the latter case, the network driver calls the 'nop' platform
> implementation which does not attempt to implement any firewall logic,
> just allowing the network to start without firewall rules.
>
> To solve this are number of changes are required
>
> * Introduce VIR_FIREWALL_BACKEND_NONE, which does nothing except
> report a fatal error from virFirewallApply(). This code path
> is unreachable, since we'll never create a virFirewall
> object with with VIR_FIREWALL_BACKEND_NONE, so the error reporting
> is just a sanity check.
>
> * Ignore the compile time backend defaults and assume use of
> the 'none' backend if running unprivileged.
>
> This fixes the first regression, avoiding the failure to start
> libvirtd on Linux in unprivileged context, instead allowing use
> of the driver and expecting a permission denied when creating a
> bridge.
>
> * Reject the use of compile time backend defaults no non-Linux
> and hardcode the 'none' backend. The non-Linux platforms have
> no firewall implementation at all currently, so there's no
> reason to permit the use of 'firewall_backend_priority'
> meson option.
>
> This fixes the second regression, avoiding the failure to start
> libvirtd on non-Linux hosts due to non-existant Linux binaries.
>
> * Change the Linux platform backend to raise an error if the
> firewall backend is 'none'. Again this code path is unreachable
> by default since we'll fail to create the bridge before getting
> here, but if someone modified network.conf to request the 'none'
> backend, this will stop further progress.
>
> * Change the nop platform backend to raise an error if the
> firewall backend is 'iptables' or 'nftables'. Again this code
> path is unreachable, since we should already have failed to
> find the iptables/nftables binaries on non-Linux hosts, so
> this is just a sanity check.
>
> * 'none' is not permited as a value in 'firewall_backend_priority'
> meson option, since it is conceptually meaningless to ask for
> that on Linux.
Thanks for the quick fix! You were faster than me.
Minor fixups:
--- a/src/network/bridge_driver_nop.c
+++ b/src/network/bridge_driver_nop.c
@@ -19,6 +19,8 @@
#include <config.h>
+#define VIR_FROM_THIS VIR_FROM_NETWORK
+
void networkPreReloadFirewallRules(virNetworkDriverState *driver G_GNUC_UNUSED,
bool startup G_GNUC_UNUSED,
bool force G_GNUC_UNUSED)
@@ -45,10 +47,10 @@ int networkAddFirewallRules(virNetworkDef *def G_GNUC_UNUSED,
* ought to fail to find the required binaries when loading,
* so this is just a sanity check
*/
- if (firewallBackend != VIR_FIREWALL_NONE) {
- virReportError(VIR_ERR_NO_SUPPORT, "%s",
+ if (firewallBackend != VIR_FIREWALL_BACKEND_NONE) {
+ virReportError(VIR_ERR_NO_SUPPORT,
_("Firewall backend '%s' not available on this platform"),
- virFirewallBackendTypeToSTring(firewallBackend));
+ virFirewallBackendTypeToString(firewallBackend));
return -1;
}
return 0;
I briefly run-time tested that and it works for me.
> Signed-off-by: Daniel P. Berrangé <berrange@xxxxxxxxxx>
> ---
> 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 | 13 ++++++++++++-
> src/util/virfirewall.c | 6 ++++++
> src/util/virfirewall.h | 1 +
> 7 files changed, 63 insertions(+), 14 deletions(-)
>
> diff --git a/meson.build b/meson.build
> index 5c7cd7ec2e..2e8b87280d 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -1647,15 +1647,27 @@ if not get_option('driver_network').disabled() and conf.has('WITH_LIBVIRTD')
> conf.set('WITH_NETWORK', 1)
>
> 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
>
> - conf.set('FIREWALL_BACKEND_PRIORITY_0', 'VIR_FIREWALL_BACKEND_' + firewall_backend_priority[0].to_upper())
> - conf.set('FIREWALL_BACKEND_PRIORITY_1', 'VIR_FIREWALL_BACKEND_' + firewall_backend_priority[1].to_upper())
> - conf.set('FIREWALL_BACKEND_PRIORITY_NUM', firewall_backend_priority.length())
> + backends = []
> + foreach backend: firewall_backend_priority
> + backend = 'VIR_FIREWALL_BACKEND_' + backend.to_upper()
> + backends += backend
> + endforeach
> +
> + conf.set('FIREWALL_BACKENDS', ', '.join(backends))
> elif get_option('driver_network').enabled()
> error('libvirtd must be enabled to build the network driver')
> endif
> diff --git a/meson_options.txt b/meson_options.txt
> index 50d71427cb..2d440c63d8 100644
> --- a/meson_options.txt
> +++ b/meson_options.txt
> @@ -117,7 +117,7 @@ option('dtrace', type: 'feature', value: 'auto', description: 'use dtrace for st
> option('firewalld', type: 'feature', value: 'auto', description: 'firewalld support')
> # dep:firewalld
> option('firewalld_zone', type: 'feature', value: 'auto', description: 'whether to install firewalld libvirt zone')
> -option('firewall_backend_priority', type: 'array', choices: ['nftables', 'iptables'], description: 'order in which to try firewall backends')
> +option('firewall_backend_priority', type: 'array', choices: ['nftables', 'iptables'], value: [], description: 'order in which to try firewall backends')
> option('host_validate', type: 'feature', value: 'auto', description: 'build virt-host-validate')
> option('init_script', type: 'combo', choices: ['systemd', 'openrc', 'check', 'none'], value: 'check', description: 'Style of init script to install')
> option('loader_nvram', type: 'string', value: '', description: 'Pass list of pairs of <loader>:<nvram> paths. Both pairs and list items are separated by a colon.')
> diff --git a/src/network/bridge_driver_conf.c b/src/network/bridge_driver_conf.c
> index e2f3613a41..9da5e790b7 100644
> --- a/src/network/bridge_driver_conf.c
> +++ b/src/network/bridge_driver_conf.c
> @@ -61,6 +61,7 @@ networkGetDnsmasqCaps(virNetworkDriverState *driver)
>
> static int
> virNetworkLoadDriverConfig(virNetworkDriverConfig *cfg G_GNUC_UNUSED,
> + bool privileged,
> const char *filename)
> {
> g_autoptr(virConf) conf = NULL;
> @@ -68,13 +69,17 @@ virNetworkLoadDriverConfig(virNetworkDriverConfig *cfg G_GNUC_UNUSED,
> bool fwBackendSelected = false;
> size_t i;
> int fwBackends[] = {
> - FIREWALL_BACKEND_PRIORITY_0,
> - FIREWALL_BACKEND_PRIORITY_1,
> + FIREWALL_BACKENDS
> };
> - G_STATIC_ASSERT(G_N_ELEMENTS(fwBackends) == VIR_FIREWALL_BACKEND_LAST);
> - G_STATIC_ASSERT(G_N_ELEMENTS(fwBackends) == FIREWALL_BACKEND_PRIORITY_NUM);
> + G_STATIC_ASSERT(G_N_ELEMENTS(fwBackends) > 0 &&
> + G_N_ELEMENTS(fwBackends) <= VIR_FIREWALL_BACKEND_LAST);
> int nFwBackends = G_N_ELEMENTS(fwBackends);
>
> + if (!privileged) {
> + fwBackends[0] = VIR_FIREWALL_BACKEND_NONE;
> + nFwBackends = 1;
> + }
> +
> if (access(filename, R_OK) == 0) {
>
> conf = virConfReadFile(filename, 0);
> @@ -104,6 +109,10 @@ virNetworkLoadDriverConfig(virNetworkDriverConfig *cfg G_GNUC_UNUSED,
> for (i = 0; i < nFwBackends && !fwBackendSelected; i++) {
>
> switch ((virFirewallBackend)fwBackends[i]) {
> + case VIR_FIREWALL_BACKEND_NONE:
> + fwBackendSelected = true;
> + break;
> +
> case VIR_FIREWALL_BACKEND_IPTABLES: {
> g_autofree char *iptablesInPath = virFindFileInPath(IPTABLES);
>
> @@ -187,7 +196,7 @@ virNetworkDriverConfigNew(bool privileged)
>
> configfile = g_strconcat(configdir, "/network.conf", NULL);
>
> - if (virNetworkLoadDriverConfig(cfg, configfile) < 0)
> + if (virNetworkLoadDriverConfig(cfg, privileged, configfile) < 0)
> return NULL;
>
> if (g_mkdir_with_parents(cfg->stateDir, 0777) < 0) {
> diff --git a/src/network/bridge_driver_linux.c b/src/network/bridge_driver_linux.c
> index 35e6bd1154..fe7c6e193c 100644
> --- a/src/network/bridge_driver_linux.c
> +++ b/src/network/bridge_driver_linux.c
> @@ -47,6 +47,11 @@ networkFirewallSetupPrivateChains(virFirewallBackend backend,
> virFirewallLayer layer)
> {
> switch (backend) {
> + case VIR_FIREWALL_BACKEND_NONE:
> + virReportError(VIR_ERR_NO_SUPPORT, "%s",
> + _("No firewall backend is available"));
> + return -1;
> +
> case VIR_FIREWALL_BACKEND_IPTABLES:
> return iptablesSetupPrivateChains(layer);
>
> @@ -417,6 +422,11 @@ networkAddFirewallRules(virNetworkDef *def,
> }
>
> switch (firewallBackend) {
> + case VIR_FIREWALL_BACKEND_NONE:
> + virReportError(VIR_ERR_NO_SUPPORT, "%s",
> + _("No firewall backend is available"));
> + return -1;
> +
> case VIR_FIREWALL_BACKEND_IPTABLES:
> return iptablesAddFirewallRules(def, fwRemoval);
>
> diff --git a/src/network/bridge_driver_nop.c b/src/network/bridge_driver_nop.c
> index 537b9234f8..7797be1ba8 100644
> --- a/src/network/bridge_driver_nop.c
> +++ b/src/network/bridge_driver_nop.c
> @@ -37,9 +37,20 @@ int networkCheckRouteCollision(virNetworkDef *def G_GNUC_UNUSED)
> }
>
> int networkAddFirewallRules(virNetworkDef *def G_GNUC_UNUSED,
> - virFirewallBackend firewallBackend G_GNUC_UNUSED,
> + virFirewallBackend firewallBackend,
> virFirewall **fwRemoval G_GNUC_UNUSED)
> {
> + /*
> + * Shouldn't be possible, since virNetworkLoadDriverConfig
> + * ought to fail to find the required binaries when loading,
> + * so this is just a sanity check
> + */
> + if (firewallBackend != VIR_FIREWALL_NONE) {
> + virReportError(VIR_ERR_NO_SUPPORT, "%s",
> + _("Firewall backend '%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 2219506b18..d374f54b64 100644
> --- a/src/util/virfirewall.c
> +++ b/src/util/virfirewall.c
> @@ -37,6 +37,7 @@ VIR_LOG_INIT("util.firewall");
>
> VIR_ENUM_IMPL(virFirewallBackend,
> VIR_FIREWALL_BACKEND_LAST,
> + "none",
> "iptables",
> "nftables");
>
> @@ -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;
> +
> case VIR_FIREWALL_BACKEND_IPTABLES:
> if (virFirewallCmdIptablesApply(firewall, fwCmd, &output) < 0)
> return -1;
> diff --git a/src/util/virfirewall.h b/src/util/virfirewall.h
> index 302a6a4e5b..bce51259d2 100644
> --- a/src/util/virfirewall.h
> +++ b/src/util/virfirewall.h
> @@ -44,6 +44,7 @@ typedef enum {
> } virFirewallLayer;
>
> typedef enum {
> + VIR_FIREWALL_BACKEND_NONE, /* Always fails */
> VIR_FIREWALL_BACKEND_IPTABLES,
> VIR_FIREWALL_BACKEND_NFTABLES,
>
> --
> 2.45.1
