The whole point of <forward mode='open'/> is to supress libvirt from adding any firewall rules for a network, and someone might want to create a network with no IP address (i.e. they don't want the guests to have connectivity to the host via this interface) and no firewall rules (they don't want any, or they want to add their own). So there's no reason to fail when a network has <forward mode='open'/> and also has no IP address. Kind-of-Resolves: https://gitlab.com/libvirt/libvirt/-/issues/588 Signed-off-by: Laine Stump <laine(a)redhat.com&gt; --- src/conf/network_conf.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 5cf419acf1..3383906c58 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -1789,7 +1789,6 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt, case VIR_NETWORK_FORWARD_ROUTE: case VIR_NETWORK_FORWARD_NAT: - case VIR_NETWORK_FORWARD_OPEN: /* It's pointless to specify L3 forwarding without specifying * the network we're on. */ @@ -1806,7 +1805,9 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt, def->name); return NULL; } + break; + case VIR_NETWORK_FORWARD_OPEN: if (def->forward.type == VIR_NETWORK_FORWARD_OPEN && def->forward.nifs) { /* an open network by definition can't place any restrictions * on what traffic is allowed or where it goes, so specifying -- 2.46.0

The 'open' forward type probably hadn't yet been added when this message was written. Signed-off-by: Laine Stump <laine(a)redhat.com&gt; --- src/conf/network_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 3383906c58..5fadaf35e9 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -1851,7 +1851,7 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt, case VIR_NETWORK_FORWARD_BRIDGE: if (def->delay || stp || def->bridgeZone) { virReportError(VIR_ERR_XML_ERROR, - _("bridge delay/stp/zone options only allowed in route, nat, and isolated mode, not in %1$s (network '%2$s')"), + _("bridge delay/stp/zone options only allowed in open, route, nat, and isolated mode, not in %1$s (network '%2$s')"), virNetworkForwardTypeToString(def->forward.type), def->name); return NULL; -- 2.46.0

The bit of code that sets the firewalld zone was previously a part of the function networkAddFirewallRules(), which is not called for networks with <forward mode='open'/>. Setting the 'libvirt' zone for the bridge device of virtual networks that also add firewall rules is usually necessary in order to get the expected traffic through without modifying firewalld's default zone (which would be a bad idea, because that would affect all the other host interfaces set to the default zone), but in general we would *not* want the bridge device for a mode='open' virtual network to be automatically placed in the "libvirt" zone, a user might want to *explicitly* set some other firewalld zone for mode='open' networks. We enable this by moving the code that sets the firewalld zone into a separate function that is called for all forward modes that use a bridge device created by libvirt (nat, route, isolated, open). If no zone is specified, then the bridge device will be in whatever zone interfaces are put in by default, but if the <bridge> element has a "zone" attribute, then the new bridge device will be placed in the specified zone. NB: This function is only called when the network is started, and *not* when the firewall rules of an active network are reloaded at virtnetworkd restart time, because the firewalld zone of an interface isn't something that gets inadvertantly changed as a part of some other unrelated action (e.g. all iptables rules are cleared by a firewalld restart, including those rules added by libvirt), and so we don't need to be re-setting it all the time. Resolves: https://gitlab.com/libvirt/libvirt/-/issues/215 Signed-off-by: Laine Stump <laine(a)redhat.com&gt; --- src/network/bridge_driver.c | 4 ++ src/network/bridge_driver_linux.c | 61 ++++++++++++++++------------ src/network/bridge_driver_nop.c | 13 ++++++ src/network/bridge_driver_platform.h | 2 + 4 files changed, 54 insertions(+), 26 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 915211d1b5..3504d512a0 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -1957,6 +1957,10 @@ networkStartNetworkVirtual(virNetworkDriverState *driver, if (networkSetIPv6Sysctls(obj) < 0) goto error; + /* set the firewall zone for the bridge device on the host */ + if (networkSetBridgeZone(def) < 0) + goto error; + /* Add "once per network" rules */ if (def->forward.type != VIR_NETWORK_FORWARD_OPEN && networkAddFirewallRules(def, cfg->firewallBackend, &fwRemoval) < 0) { diff --git a/src/network/bridge_driver_linux.c b/src/network/bridge_driver_linux.c index fe7c6e193c..a6203a712e 100644 --- a/src/network/bridge_driver_linux.c +++ b/src/network/bridge_driver_linux.c @@ -333,28 +333,8 @@ int networkCheckRouteCollision(virNetworkDef *def) int -networkAddFirewallRules(virNetworkDef *def, - virFirewallBackend firewallBackend, - virFirewall **fwRemoval) +networkSetBridgeZone(virNetworkDef *def) { - - networkSetupPrivateChains(firewallBackend, false); - - if (errInitV4 && - (virNetworkDefGetIPByIndex(def, AF_INET, 0) || - virNetworkDefGetRouteByIndex(def, AF_INET, 0))) { - virSetError(errInitV4); - return -1; - } - - if (errInitV6 && - (virNetworkDefGetIPByIndex(def, AF_INET6, 0) || - virNetworkDefGetRouteByIndex(def, AF_INET6, 0) || - def->ipv6nogw)) { - virSetError(errInitV6); - return -1; - } - if (def->bridgeZone) { /* if a firewalld zone has been specified, fail/log an error @@ -370,12 +350,14 @@ networkAddFirewallRules(virNetworkDef *def, if (virFirewallDInterfaceSetZone(def->bridge, def->bridgeZone) < 0) return -1; - } else { + } else if (def->forward.type != VIR_NETWORK_FORWARD_OPEN) { - /* if firewalld is active, try to set the "libvirt" zone. This is - * desirable (for consistency) if firewalld is using the iptables - * backend, but is necessary (for basic network connectivity) if - * firewalld is using the nftables backend + /* if firewalld is active, try to set the "libvirt" zone by + * default (forward mode='open' networks have no zone set by + * default, but we honor it if one is specified). This is + * desirable (for consistency) if firewalld is using the + * iptables backend, but is necessary (for basic network + * connectivity) if firewalld is using the nftables backend */ if (virFirewallDIsRegistered() == 0) { @@ -421,6 +403,33 @@ networkAddFirewallRules(virNetworkDef *def, } } + return 0; +} + + +int +networkAddFirewallRules(virNetworkDef *def, + virFirewallBackend firewallBackend, + virFirewall **fwRemoval) +{ + + networkSetupPrivateChains(firewallBackend, false); + + if (errInitV4 && + (virNetworkDefGetIPByIndex(def, AF_INET, 0) || + virNetworkDefGetRouteByIndex(def, AF_INET, 0))) { + virSetError(errInitV4); + return -1; + } + + if (errInitV6 && + (virNetworkDefGetIPByIndex(def, AF_INET6, 0) || + virNetworkDefGetRouteByIndex(def, AF_INET6, 0) || + def->ipv6nogw)) { + virSetError(errInitV6); + return -1; + } + switch (firewallBackend) { case VIR_FIREWALL_BACKEND_NONE: virReportError(VIR_ERR_NO_SUPPORT, "%s", diff --git a/src/network/bridge_driver_nop.c b/src/network/bridge_driver_nop.c index 8bf3367bff..20c7a2a595 100644 --- a/src/network/bridge_driver_nop.c +++ b/src/network/bridge_driver_nop.c @@ -38,6 +38,19 @@ int networkCheckRouteCollision(virNetworkDef *def G_GNUC_UNUSED) return 0; } + +int +networkSetBridgeZone(virNetworkDef *def) +{ + if (def->bridgeZone) { + virReportError(VIR_ERR_NO_SUPPORT, "%s", + _("This platform does not support setting the bridge device zone")); + return -1; + } + return 0; +} + + int networkAddFirewallRules(virNetworkDef *def G_GNUC_UNUSED, virFirewallBackend firewallBackend, virFirewall **fwRemoval G_GNUC_UNUSED) diff --git a/src/network/bridge_driver_platform.h b/src/network/bridge_driver_platform.h index cd2e3fa7b5..02abdc197f 100644 --- a/src/network/bridge_driver_platform.h +++ b/src/network/bridge_driver_platform.h @@ -32,6 +32,8 @@ void networkPostReloadFirewallRules(bool startup); int networkCheckRouteCollision(virNetworkDef *def); +int networkSetBridgeZone(virNetworkDef *def); + int networkAddFirewallRules(virNetworkDef *def, virFirewallBackend firewallBackend, virFirewall **fwRemoval); -- 2.46.0

At the time the version check in this function was written, there were still several supported versions of some distros that were using a version of firewalld too old to support the "rich rule priorities" used by the 'libvirt' zone that we installed for firewalld. Today the newest distro that has a version of firewalld < 0.7.0 is RHEL7/CentOS7, so we can remove the complexity and if the libvirt zone is missing simply say "the libvirt zone is missing". Signed-off-by: Laine Stump <laine(a)redhat.com&gt; --- src/network/bridge_driver_linux.c | 21 +++------------------ 1 file changed, 3 insertions(+), 18 deletions(-) diff --git a/src/network/bridge_driver_linux.c b/src/network/bridge_driver_linux.c index a6203a712e..af758d4f3d 100644 --- a/src/network/bridge_driver_linux.c +++ b/src/network/bridge_driver_linux.c @@ -381,24 +381,9 @@ networkSetBridgeZone(virNetworkDef *def) if (virFirewallDInterfaceSetZone(def->bridge, "libvirt") < 0) return -1; } else { - unsigned long long version; - int vresult = virFirewallDGetVersion(&version); - - if (vresult < 0) - return -1; - - /* Support for nftables backend was added in firewalld - * 0.6.0. Support for rule priorities (required by the - * 'libvirt' zone, which should be installed by a - * libvirt package, *not* by firewalld) was not added - * until firewalld 0.7.0 (unless it was backported). - */ - if (version >= 6000 && - virFirewallDGetBackend() == VIR_FIREWALLD_BACKEND_NFTABLES) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("firewalld is set to use the nftables backend, but the required firewalld 'libvirt' zone is missing. Either set the firewalld backend to 'iptables', or ensure that firewalld has a 'libvirt' zone by upgrading firewalld to a version supporting rule priorities (0.7.0+) and/or rebuilding libvirt with --with-firewalld-zone")); - return -1; - } + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("firewalld can't find the 'libvirt' zone that should have been installed with libvirt")); + return -1; } } } -- 2.46.0

When a bridge device for a virtual network had been placed in a firewalld zone while starting the network, then even after the network is shut down and the bridge device is deleted, its name will show up in the list of interfaces for whichever zone it had been in. Usually this isn't a problem, but in the case of forward mode='open', someone might start the network once with a zone specified, then shut down the network, remove vthe zone from its config, and start it again; in this case the bridge device would come up using the zone from the previous time it was started. The solution to this is to remove the interface from whatever zone it is in as the network is being shut down. There is no downside to doing this, since the device is going to be deleted anyway. Note that forward mode='bridge' uses a bridge device that was created outside of libvirt, and libvirt won't be deleting that bridge, so we take care to not unset the zone in that case. Signed-off-by: Laine Stump <laine(a)redhat.com&gt; --- src/libvirt_private.syms | 1 + src/network/bridge_driver.c | 4 ++++ src/network/bridge_driver_linux.c | 14 ++++++++++++++ src/network/bridge_driver_nop.c | 6 ++++++ src/network/bridge_driver_platform.h | 2 ++ src/util/virfirewalld.c | 23 +++++++++++++++++++++++ src/util/virfirewalld.h | 2 ++ 7 files changed, 52 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index af40e5dca3..f15d16c292 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2451,6 +2451,7 @@ virFirewallDGetPolicies; virFirewallDGetVersion; virFirewallDGetZones; virFirewallDInterfaceSetZone; +virFirewallDInterfaceUnsetZone; virFirewallDIsRegistered; virFirewallDPolicyExists; virFirewallDSynchronize; diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 3504d512a0..e457c3bf5e 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2085,6 +2085,8 @@ networkStartNetworkVirtual(virNetworkDriverState *driver, def->forward.type != VIR_NETWORK_FORWARD_OPEN) networkRemoveFirewallRules(obj); + networkUnsetBridgeZone(def); + virNetworkObjUnrefMacMap(obj); ignore_value(virNetDevBridgeDelete(def->bridge)); @@ -2123,6 +2125,8 @@ networkShutdownNetworkVirtual(virNetworkObj *obj) if (def->forward.type != VIR_NETWORK_FORWARD_OPEN) networkRemoveFirewallRules(obj); + networkUnsetBridgeZone(def); + ignore_value(virNetDevBridgeDelete(def->bridge)); /* See if its still alive and really really kill it */ diff --git a/src/network/bridge_driver_linux.c b/src/network/bridge_driver_linux.c index af758d4f3d..3b3608c085 100644 --- a/src/network/bridge_driver_linux.c +++ b/src/network/bridge_driver_linux.c @@ -392,6 +392,20 @@ networkSetBridgeZone(virNetworkDef *def) } +void +networkUnsetBridgeZone(virNetworkDef *def) +{ + /* If there is a libvirt-managed bridge device remove it from any + * zone it had been placed in as a part of deleting the bridge. + * DO NOT CALL THIS FOR 'bridge' forward mode, since that + * bridge is not managed by libvirt. + */ + if (def->bridge && def->forward.type != VIR_NETWORK_FORWARD_BRIDGE + && virFirewallDIsRegistered() == 0) { + virFirewallDInterfaceUnsetZone(def->bridge); + } +} + int networkAddFirewallRules(virNetworkDef *def, virFirewallBackend firewallBackend, diff --git a/src/network/bridge_driver_nop.c b/src/network/bridge_driver_nop.c index 20c7a2a595..180ff30134 100644 --- a/src/network/bridge_driver_nop.c +++ b/src/network/bridge_driver_nop.c @@ -51,6 +51,12 @@ networkSetBridgeZone(virNetworkDef *def) } +void +networkUnsetBridgeZone(virNetworkDef *def) +{ +} + + int networkAddFirewallRules(virNetworkDef *def G_GNUC_UNUSED, virFirewallBackend firewallBackend, virFirewall **fwRemoval G_GNUC_UNUSED) diff --git a/src/network/bridge_driver_platform.h b/src/network/bridge_driver_platform.h index 02abdc197f..a0291532a1 100644 --- a/src/network/bridge_driver_platform.h +++ b/src/network/bridge_driver_platform.h @@ -38,4 +38,6 @@ int networkAddFirewallRules(virNetworkDef *def, virFirewallBackend firewallBackend, virFirewall **fwRemoval); +void networkUnsetBridgeZone(virNetworkDef *def); + void networkRemoveFirewallRules(virNetworkObj *obj); diff --git a/src/util/virfirewalld.c b/src/util/virfirewalld.c index 827e201dbb..4aec33ac45 100644 --- a/src/util/virfirewalld.c +++ b/src/util/virfirewalld.c @@ -449,6 +449,29 @@ virFirewallDInterfaceSetZone(const char *iface, } +int +virFirewallDInterfaceUnsetZone(const char *iface) +{ + GDBusConnection *sysbus = virGDBusGetSystemBus(); + g_autoptr(GVariant) message = NULL; + + if (!sysbus) + return -1; + + message = g_variant_new("(ss)", "", iface); + + return virGDBusCallMethod(sysbus, + NULL, + NULL, + NULL, + VIR_FIREWALL_FIREWALLD_SERVICE, + "/org/fedoraproject/FirewallD1", + "org.fedoraproject.FirewallD1.zone", + "removeInterface", + message); +} + + void virFirewallDSynchronize(void) { diff --git a/src/util/virfirewalld.h b/src/util/virfirewalld.h index 0e94d3507b..0dbe66d435 100644 --- a/src/util/virfirewalld.h +++ b/src/util/virfirewalld.h @@ -46,4 +46,6 @@ int virFirewallDApplyRule(virFirewallLayer layer, int virFirewallDInterfaceSetZone(const char *iface, const char *zone); +int virFirewallDInterfaceUnsetZone(const char *iface); + void virFirewallDSynchronize(void); -- 2.46.0

On Mon, Sep 16, 2024 at 09:49:44AM +0200, Martin Kletzander wrote: Having said that it would still be beneficial to do some extra clean up to remove the zone if, for example, the bridge disappeared while virtnetworkd was not running (shameless plug of [1] O:-) ) Martin [1] https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/D7...

On 9/16/24 4:06 AM, Martin Kletzander wrote: By definition, def->bridge will be non-NULL if any of those forward modes is set. (this is verified in networkValidate() with the call to networkBridgeNameValidate()) Also it would be impossible to get far enough though networkStartNetworkVirtual() (which is the way networks with all of these modes are started) to set the zone if def->bridge was NULL - it would fail right at the top of the function). Finally, even if it was possible to get that far, it is the bridge device itself's name that is being added to the given zone, so "setting the zone" when the device name is NULL wouldn't do anything (other than generate an error). Isn't it enough to put a call to networkUnsetBridgeZone() in networkShutdownNetworkVirtual()? If so, then you've already done that work for me. :-)