[PATCH 0/8] network: firewalld: native support for NAT/routed

[PATCH] Add vms cli tool to the...

[PATCH 0/7] Couple of...

Eric Garver

Thursday, 10 November 2022 Thu, 10 Nov '22

10:31 a.m.

This series further improves the firewalld backend by converting to a fully native implementation for NAT and routed networks. That is, there are no iptables rules added by libvirt when the running firewalld is 0.9.0 or later. The major advantage is that firewalld users can use firewall-cmd to filter the VM traffic and apply their own policies. When firewalld < 0.9.0 is present only the "libvirt" zone will be used. The new "libvirt-nat" and "libvirt-routed" zones are not used. This maintains compatibility for older distributions (e.g. Ubuntu 20.04). Patch 1 is a bug fix for my previous series to avoid a bogus error log. Patches 2-3 converts the routed network to native firewalld. Patches 4-8 converts the NAT network to native firewalld. It also introduces the "libvirt-nat" zone. Eric Garver (8): util: virFirewallDGetPolicies: gracefully handle older firewalld network: firewalld: add networkAddHybridFirewallDRules() network: firewalld: use native routed networks util: add virFirewallDSourceSetZone() util: add virFirewallDApplyPolicyRichRules() network: firewalld: add zone for NAT networks network: firewalld: add policies for NAT networks network: firewalld: use native NAT networks libvirt.spec.in | 2 + src/libvirt_private.syms | 2 + src/network/bridge_driver_linux.c | 193 ++++++++++++++++++++--------- src/network/libvirt-nat-out.policy | 13 ++ src/network/libvirt-nat.zone | 10 ++ src/network/libvirt-to-host.policy | 1 + src/network/meson.build | 10 ++ src/util/virfirewalld.c | 79 +++++++++++- src/util/virfirewalld.h | 6 + 9 files changed, 258 insertions(+), 58 deletions(-) create mode 100644 src/network/libvirt-nat-out.policy create mode 100644 src/network/libvirt-nat.zone -- 2.37.3

Show replies by date

Eric Garver

Thursday, 10 November Thu, 10 Nov

10:31 a.m.

New subject: [PATCH 1/8] util: virFirewallDGetPolicies: gracefully handle older firewalld

If the running firewalld doesn't support getPolicies() then we fallback to the "libvirt" zone. Throwing an error log is excessive since we gracefully fallback. Avoids these logs: error : virGDBusCallMethod:242 : error from service: \ GDBus.Error:org.freedesktop.DBus.Error.UnknownMethod Fixes: ab56f84976e0 ("util: add virFirewallDGetPolicies()") Signed-off-by: Eric Garver <eric(a)garver.life> --- src/util/virfirewalld.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/util/virfirewalld.c b/src/util/virfirewalld.c index ad879164c3a8..d11e974cc2d5 100644 --- a/src/util/virfirewalld.c +++ b/src/util/virfirewalld.c @@ -240,6 +240,7 @@ virFirewallDGetPolicies(char ***policies, size_t *npolicies) GDBusConnection *sysbus = virGDBusGetSystemBus(); g_autoptr(GVariant) reply = NULL; g_autoptr(GVariant) array = NULL; + g_autoptr(virError) error = NULL; *npolicies = 0; *policies = NULL; @@ -247,10 +248,12 @@ virFirewallDGetPolicies(char ***policies, size_t *npolicies) if (!sysbus) return -1; + error = g_new0(virError, 1); + if (virGDBusCallMethod(sysbus, &reply, G_VARIANT_TYPE("(as)"), - NULL, + error, VIR_FIREWALL_FIREWALLD_SERVICE, "/org/fedoraproject/FirewallD1", "org.fedoraproject.FirewallD1.policy", @@ -258,6 +261,12 @@ virFirewallDGetPolicies(char ***policies, size_t *npolicies) NULL) < 0) return -1; + if (error->level == VIR_ERR_ERROR) { + if (!virGDBusErrorIsUnknownMethod(error)) + virReportErrorObject(error); + return -1; + } + g_variant_get(reply, "(@as)", &array); *policies = g_variant_dup_strv(array, npolicies); -- 2.37.3

Eric Garver

10:31 a.m.

New subject: [PATCH 2/8] network: firewalld: add networkAddHybridFirewallDRules()

This factors out the firewalld pieces of the iptables + firewalld backend. Signed-off-by: Eric Garver <eric(a)garver.life> --- src/network/bridge_driver_linux.c | 117 ++++++++++++++++-------------- 1 file changed, 61 insertions(+), 56 deletions(-) diff --git a/src/network/bridge_driver_linux.c b/src/network/bridge_driver_linux.c index d9597d91beed..88a8e9c5fa27 100644 --- a/src/network/bridge_driver_linux.c +++ b/src/network/bridge_driver_linux.c @@ -801,6 +801,58 @@ networkRemoveIPSpecificFirewallRules(virFirewall *fw, } +static int +networkAddHybridFirewallDRules(virNetworkDef *def) +{ + /* 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 the "libvirt" zone exists, then set it. If not, and + * if firewalld is using the nftables backend, then we + * need to log an error because the combination of + * nftables + default zone means that traffic cannot be + * forwarded (and even DHCP and DNS from guest to host + * will probably no be permitted by the default zone + */ + if (virFirewallDZoneExists("libvirt")) { + if (virFirewallDInterfaceSetZone(def->bridge, "libvirt") < 0) + return -1; + } else { + unsigned 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; + } + } + + return 0; +} + + /* Add all rules for all ip addresses (and general rules) on a network */ int networkAddFirewallRules(virNetworkDef *def) { @@ -842,62 +894,15 @@ int networkAddFirewallRules(virNetworkDef *def) if (virFirewallDInterfaceSetZone(def->bridge, def->bridgeZone) < 0) return -1; - } else { - - /* 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 (virFirewallDIsRegistered() == 0) { - - /* if the "libvirt" zone exists, then set it. If not, and - * if firewalld is using the nftables backend, then we - * need to log an error because the combination of - * nftables + default zone means that traffic cannot be - * forwarded (and even DHCP and DNS from guest to host - * will probably no be permitted by the default zone - * - * Routed networks use a different zone and policy which we also - * need to verify exist. Probing for the policy guarantees the - * running firewalld has support for policies (firewalld >= 0.9.0). - */ - if (def->forward.type == VIR_NETWORK_FORWARD_ROUTE && - virFirewallDPolicyExists("libvirt-routed-out") && - virFirewallDZoneExists("libvirt-routed")) { - if (virFirewallDInterfaceSetZone(def->bridge, "libvirt-routed") < 0) - return -1; - } else if (virFirewallDZoneExists("libvirt")) { - if (virFirewallDInterfaceSetZone(def->bridge, "libvirt") < 0) - return -1; - } else { - unsigned 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; - } - } + } else if (virFirewallDIsRegistered() == 0) { + if (def->forward.type == VIR_NETWORK_FORWARD_ROUTE && + virFirewallDPolicyExists("libvirt-routed-out") && + virFirewallDZoneExists("libvirt-routed")) { + if (virFirewallDInterfaceSetZone(def->bridge, "libvirt-routed") < 0) + return -1; + } else { + if (networkAddHybridFirewallDRules(def) < 0) + return -1; } } -- 2.37.3

Michal Prívozník

Tuesday, 15 November Tue, 15 Nov

4:21 a.m.

New subject: [PATCH 2/8] network: firewalld: add networkAddHybridFirewallDRules()

On 11/10/22 17:31, Eric Garver wrote:

...

I know you wanted this to be plain code movement, but since you're touching this error message you can apply our coding style rule [1] and unbreak the message onto one, long line. All error messages are exempt from the 80 chars rule, because it's easier to git grep them. And on a similar note, per out deprecation policy [2], firewalld-0.6.0 or older is ancient history, thus this version check can be dropped (in a separate commit of course).

...

+ return -1; + } + } + + return 0; +} + +

1: https://libvirt.org/coding-style.html#error-message-format 2: https://libvirt.org/platforms.html#linux-freebsd-and-macos Michal

Eric Garver

4:06 p.m.

New subject: [PATCH 2/8] network: firewalld: add networkAddHybridFirewallDRules()

On Tue, Nov 15, 2022 at 11:21:43AM +0100, Michal Prívozník wrote:

...

On 11/10/22 17:31, Eric Garver wrote: > This factors out the firewalld pieces of the iptables + firewalld > backend. > > Signed-off-by: Eric Garver <eric(a)garver.life> > --- > src/network/bridge_driver_linux.c | 117 ++++++++++++++++-------------- > 1 file changed, 61 insertions(+), 56 deletions(-) > > diff --git a/src/network/bridge_driver_linux.c b/src/network/bridge_driver_linux.c > index d9597d91beed..88a8e9c5fa27 100644 > --- a/src/network/bridge_driver_linux.c > +++ b/src/network/bridge_driver_linux.c > @@ -801,6 +801,58 @@ networkRemoveIPSpecificFirewallRules(virFirewall *fw, > } > > > +static int > +networkAddHybridFirewallDRules(virNetworkDef *def) > +{ > + /* 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 the "libvirt" zone exists, then set it. If not, and > + * if firewalld is using the nftables backend, then we > + * need to log an error because the combination of > + * nftables + default zone means that traffic cannot be > + * forwarded (and even DHCP and DNS from guest to host > + * will probably no be permitted by the default zone > + */ > + if (virFirewallDZoneExists("libvirt")) { > + if (virFirewallDInterfaceSetZone(def->bridge, "libvirt") < 0) > + return -1; > + } else { > + unsigned 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")); I know you wanted this to be plain code movement, but since you're touching this error message you can apply our coding style rule [1] and unbreak the message onto one, long line. All error messages are exempt from the 80 chars rule, because it's easier to git grep them. And on a similar note, per out deprecation policy [2], firewalld-0.6.0 or older is ancient history, thus this version check can be dropped (in a separate commit of course).

ACK. I'll drop the whole else block in the next version.

...

> + return -1; > + } > + } > + > + return 0; > +} > + > + 1: https://libvirt.org/coding-style.html#error-message-format 2: https://libvirt.org/platforms.html#linux-freebsd-and-macos Michal

Eric Garver

Thursday, 10 November Thu, 10 Nov

10:31 a.m.

New subject: [PATCH 3/8] network: firewalld: use native routed networks

The firewalld backend for routed networks can now use a native implementation. The hybrid of iptables + firewalld is no longer necessary. When full native firewalld is in use there are zero iptables rules add by libvirt. This is accomplished by returning early in networkAddFirewallRules() and avoiding calls to networkSetupPrivateChains(). Signed-off-by: Eric Garver <eric(a)garver.life> --- src/network/bridge_driver_linux.c | 51 +++++++++++++++++++++++++------ 1 file changed, 42 insertions(+), 9 deletions(-) diff --git a/src/network/bridge_driver_linux.c b/src/network/bridge_driver_linux.c index 88a8e9c5fa27..42f098ff1f9b 100644 --- a/src/network/bridge_driver_linux.c +++ b/src/network/bridge_driver_linux.c @@ -133,6 +133,21 @@ networkHasRunningNetworksWithFW(virNetworkDriverState *driver) } +static bool +networkUseOnlyFirewallDRules(void) +{ + if (virFirewallDIsRegistered() < 0) + return false; + + if (virFirewallDPolicyExists("libvirt-routed-out") && + virFirewallDZoneExists("libvirt-routed")) { + return true; + } + + return false; +} + + void networkPreReloadFirewallRules(virNetworkDriverState *driver, bool startup G_GNUC_UNUSED, @@ -172,6 +187,9 @@ networkPreReloadFirewallRules(virNetworkDriverState *driver, return; } + if (!chainInitDone && networkUseOnlyFirewallDRules()) + return; + ignore_value(virOnce(&createdOnce, networkSetupPrivateChains)); } } @@ -801,6 +819,18 @@ networkRemoveIPSpecificFirewallRules(virFirewall *fw, } +static int +networkAddOnlyFirewallDRules(virNetworkDef *def) +{ + if (def->forward.type == VIR_NETWORK_FORWARD_ROUTE) { + if (virFirewallDInterfaceSetZone(def->bridge, "libvirt-routed") < 0) + return -1; + } + + return 0; +} + + static int networkAddHybridFirewallDRules(virNetworkDef *def) { @@ -860,6 +890,11 @@ int networkAddFirewallRules(virNetworkDef *def) virNetworkIPDef *ipdef; g_autoptr(virFirewall) fw = virFirewallNew(); + if (!def->bridgeZone && networkUseOnlyFirewallDRules() && + def->forward.type == VIR_NETWORK_FORWARD_ROUTE) { + return networkAddOnlyFirewallDRules(def); + } + if (virOnce(&createdOnce, networkSetupPrivateChains) < 0) return -1; @@ -895,15 +930,8 @@ int networkAddFirewallRules(virNetworkDef *def) return -1; } else if (virFirewallDIsRegistered() == 0) { - if (def->forward.type == VIR_NETWORK_FORWARD_ROUTE && - virFirewallDPolicyExists("libvirt-routed-out") && - virFirewallDZoneExists("libvirt-routed")) { - if (virFirewallDInterfaceSetZone(def->bridge, "libvirt-routed") < 0) - return -1; - } else { - if (networkAddHybridFirewallDRules(def) < 0) - return -1; - } + if (networkAddHybridFirewallDRules(def) < 0) + return -1; } virFirewallStartTransaction(fw, 0); @@ -940,6 +968,11 @@ void networkRemoveFirewallRules(virNetworkDef *def) virNetworkIPDef *ipdef; g_autoptr(virFirewall) fw = virFirewallNew(); + if (!def->bridgeZone && networkUseOnlyFirewallDRules() && + def->forward.type == VIR_NETWORK_FORWARD_ROUTE) { + return; + } + virFirewallStartTransaction(fw, VIR_FIREWALL_TRANSACTION_IGNORE_ERRORS); networkRemoveChecksumFirewallRules(fw, def); -- 2.37.3

Daniel P. Berrangé

Tuesday, 15 November Tue, 15 Nov

10:34 a.m.

New subject: [PATCH 3/8] network: firewalld: use native routed networks

On Thu, Nov 10, 2022 at 11:31:47AM -0500, Eric Garver wrote:

...

This logic doesn't work well in the upgrade scenario. Consider that we have existing running virtual networks, and we upgrade libvirt in-place on the host. During virtnetworkd startup, we tear down old firwall rules and create the new ones. Except that we need to teardown the old iptables rules, and this skips that because it decided we need to use firewalld instead. So we're left with dangling iptables rules on upgrade

...

virFirewallStartTransaction(fw, VIR_FIREWALL_TRANSACTION_IGNORE_ERRORS); networkRemoveChecksumFirewallRules(fw, def);

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

Laine Stump

12:47 p.m.

New subject: [PATCH 3/8] network: firewalld: use native routed networks

On 11/15/22 11:34 AM, Daniel P. Berrangé wrote:

...

On Thu, Nov 10, 2022 at 11:31:47AM -0500, Eric Garver wrote: > The firewalld backend for routed networks can now use a native > implementation. The hybrid of iptables + firewalld is no longer > necessary. When full native firewalld is in use there are zero iptables > rules add by libvirt. > > This is accomplished by returning early in networkAddFirewallRules() and > avoiding calls to networkSetupPrivateChains(). > > Signed-off-by: Eric Garver <eric(a)garver.life> > --- > src/network/bridge_driver_linux.c | 51 +++++++++++++++++++++++++------ > 1 file changed, 42 insertions(+), 9 deletions(-) > > diff --git a/src/network/bridge_driver_linux.c b/src/network/bridge_driver_linux.c > index 88a8e9c5fa27..42f098ff1f9b 100644 > --- a/src/network/bridge_driver_linux.c > +++ b/src/network/bridge_driver_linux.c > @@ -133,6 +133,21 @@ networkHasRunningNetworksWithFW(virNetworkDriverState *driver) > } > > > +static bool > +networkUseOnlyFirewallDRules(void) > +{ > + if (virFirewallDIsRegistered() < 0) > + return false; > + > + if (virFirewallDPolicyExists("libvirt-routed-out") && > + virFirewallDZoneExists("libvirt-routed")) { > + return true; > + } > + > + return false; > +} > + > + > void > networkPreReloadFirewallRules(virNetworkDriverState *driver, > bool startup G_GNUC_UNUSED, > @@ -172,6 +187,9 @@ networkPreReloadFirewallRules(virNetworkDriverState *driver, > return; > } > > + if (!chainInitDone && networkUseOnlyFirewallDRules()) > + return; > + > ignore_value(virOnce(&createdOnce, networkSetupPrivateChains)); > } > } > @@ -801,6 +819,18 @@ networkRemoveIPSpecificFirewallRules(virFirewall *fw, > } > > > +static int > +networkAddOnlyFirewallDRules(virNetworkDef *def) > +{ > + if (def->forward.type == VIR_NETWORK_FORWARD_ROUTE) { > + if (virFirewallDInterfaceSetZone(def->bridge, "libvirt-routed") < 0) > + return -1; > + } > + > + return 0; > +} > + > + > static int > networkAddHybridFirewallDRules(virNetworkDef *def) > { > @@ -860,6 +890,11 @@ int networkAddFirewallRules(virNetworkDef *def) > virNetworkIPDef *ipdef; > g_autoptr(virFirewall) fw = virFirewallNew(); > > + if (!def->bridgeZone && networkUseOnlyFirewallDRules() && > + def->forward.type == VIR_NETWORK_FORWARD_ROUTE) { > + return networkAddOnlyFirewallDRules(def); > + } > + > if (virOnce(&createdOnce, networkSetupPrivateChains) < 0) > return -1; > > @@ -895,15 +930,8 @@ int networkAddFirewallRules(virNetworkDef *def) > return -1; > > } else if (virFirewallDIsRegistered() == 0) { > - if (def->forward.type == VIR_NETWORK_FORWARD_ROUTE && > - virFirewallDPolicyExists("libvirt-routed-out") && > - virFirewallDZoneExists("libvirt-routed")) { > - if (virFirewallDInterfaceSetZone(def->bridge, "libvirt-routed") < 0) > - return -1; > - } else { > - if (networkAddHybridFirewallDRules(def) < 0) > - return -1; > - } > + if (networkAddHybridFirewallDRules(def) < 0) > + return -1; > } > > virFirewallStartTransaction(fw, 0); > @@ -940,6 +968,11 @@ void networkRemoveFirewallRules(virNetworkDef *def) > virNetworkIPDef *ipdef; > g_autoptr(virFirewall) fw = virFirewallNew(); > > + if (!def->bridgeZone && networkUseOnlyFirewallDRules() && > + def->forward.type == VIR_NETWORK_FORWARD_ROUTE) { > + return; > + } > + This logic doesn't work well in the upgrade scenario. Consider that we have existing running virtual networks, and we upgrade libvirt in-place on the host. During virtnetworkd startup, we tear down old firwall rules and create the new ones. Except that we need to teardown the old iptables rules, and this skips that because it decided we need to use firewalld instead. So we're left with dangling iptables rules on upgrade

libvirt has a longstanding problem that each time it starts, it assumes that all the active networks had been started with firewall rules exactly matching the rules that the *current* libvirt version would add if it had started the network. This usually is okay; in the past it was only a problem when someone changed the rulelist added for each network, which historically has only happened a few times (and in those cases we just told people to restart their host if they wanted everything to be exactly correct after the upgrade). This is just another instance of that same problem. I have a patchset to add a "native nftables" backend that I've been alternately working on and abandoning for several months, and in that patchset I save the entire ruleset that was added for each network into that network's status XML. Then when the daemon is restarted, the rules that need to be removed (or, more correctly, the commands that need to be run in order to remove the rules) is read from the status XML for the network rather than just being blindly assumed. This permits switching from iptables to nftables backend without requiring a reboot to get it right, and also makes sure that if a future update to libvirt changes the ruleset, we will properly remove the old rules during the update. Coincidentally, this same code will fix the problem with a restart after switching to a pure firewalld backend (there will be a list of "firewall commands", but that list will be empty[*]) [*] It has to be an empty list rather than no list at all because the absence of a list of necessary commands in the status XML must be recognized as "previous libvirt didn't save the list of commands - assume that we currently have the rules that would have been added by a 'pre-epoch' libvirt". Anyway, I am soonish planning to rebase my nftables-backend patches, and see how Eric's patches and mine can mutually feed off each other (I need to rethink where I draw the abstraction/API line for the functions that each backend must provide).

Eric Garver

Thursday, 10 November Thu, 10 Nov

10:31 a.m.

New subject: [PATCH 4/8] util: add virFirewallDSourceSetZone()

Signed-off-by: Eric Garver <eric(a)garver.life> --- src/libvirt_private.syms | 1 + src/util/virfirewalld.c | 24 ++++++++++++++++++++++++ src/util/virfirewalld.h | 2 ++ 3 files changed, 27 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 97ff2a43e48a..c5882c535210 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2366,6 +2366,7 @@ virFirewallDGetZones; virFirewallDInterfaceSetZone; virFirewallDIsRegistered; virFirewallDPolicyExists; +virFirewallDSourceSetZone; virFirewallDSynchronize; virFirewallDZoneExists; diff --git a/src/util/virfirewalld.c b/src/util/virfirewalld.c index d11e974cc2d5..07f9cdd1e485 100644 --- a/src/util/virfirewalld.c +++ b/src/util/virfirewalld.c @@ -451,6 +451,30 @@ virFirewallDInterfaceSetZone(const char *iface, } +int +virFirewallDSourceSetZone(const char *source, + const char *zone) +{ + GDBusConnection *sysbus = virGDBusGetSystemBus(); + g_autoptr(GVariant) message = NULL; + + if (!sysbus) + return -1; + + message = g_variant_new("(ss)", zone, source); + + return virGDBusCallMethod(sysbus, + NULL, + NULL, + NULL, + VIR_FIREWALL_FIREWALLD_SERVICE, + "/org/fedoraproject/FirewallD1", + "org.fedoraproject.FirewallD1.zone", + "changeZoneOfSource", + message); +} + + void virFirewallDSynchronize(void) { diff --git a/src/util/virfirewalld.h b/src/util/virfirewalld.h index fa4c9e702ccb..11aad7786dfb 100644 --- a/src/util/virfirewalld.h +++ b/src/util/virfirewalld.h @@ -43,5 +43,7 @@ int virFirewallDApplyRule(virFirewallLayer layer, int virFirewallDInterfaceSetZone(const char *iface, const char *zone); +int virFirewallDSourceSetZone(const char *source, + const char *zone); void virFirewallDSynchronize(void); -- 2.37.3

Eric Garver

10:31 a.m.

New subject: [PATCH 5/8] util: add virFirewallDApplyPolicyRichRules()

Signed-off-by: Eric Garver <eric(a)garver.life> --- src/libvirt_private.syms | 1 + src/util/virfirewalld.c | 44 ++++++++++++++++++++++++++++++++++++++++ src/util/virfirewalld.h | 4 ++++ 3 files changed, 49 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c5882c535210..8fddb9aad11b 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2358,6 +2358,7 @@ virFirewallStartTransaction; # util/virfirewalld.h +virFirewallDApplyPolicyRichRules; virFirewallDApplyRule; virFirewallDGetBackend; virFirewallDGetPolicies; diff --git a/src/util/virfirewalld.c b/src/util/virfirewalld.c index 07f9cdd1e485..9b3c1d84c48f 100644 --- a/src/util/virfirewalld.c +++ b/src/util/virfirewalld.c @@ -426,6 +426,50 @@ virFirewallDApplyRule(virFirewallLayer layer, return 0; } +/** + * virFirewallDApplyPolicyRichRules: + * @policy: which policy to apply rules to + * @rules: rules to apply, array of strings + * @rules_count: number of rules in rules array + * + * Returns 0 on success, non-zero on failure + */ +int +virFirewallDApplyPolicyRichRules(const char *policy, + const char **rules, + size_t rules_count) +{ + GDBusConnection *sysbus = virGDBusGetSystemBus(); + g_autoptr(GVariant) message = NULL; + GVariant *array = NULL; + GVariantBuilder builder; + size_t i; + + if (!sysbus) + return -1; + + g_variant_builder_init(&builder, G_VARIANT_TYPE_STRING_ARRAY); + for (i = 0; i < rules_count; i++) { + g_variant_builder_add(&builder, "s", rules[i]); + } + array = g_variant_builder_end(&builder); + + g_variant_builder_init(&builder, G_VARIANT_TYPE_VARDICT); + g_variant_builder_add(&builder, "{sv}", "rich_rules", array); + + message = g_variant_new("(sa{sv})", policy, &builder); + + return virGDBusCallMethod(sysbus, + NULL, + NULL, + NULL, + VIR_FIREWALL_FIREWALLD_SERVICE, + "/org/fedoraproject/FirewallD1", + "org.fedoraproject.FirewallD1.policy", + "setPolicySettings", + message); +} + int virFirewallDInterfaceSetZone(const char *iface, diff --git a/src/util/virfirewalld.h b/src/util/virfirewalld.h index 11aad7786dfb..9ff4e02e1d59 100644 --- a/src/util/virfirewalld.h +++ b/src/util/virfirewalld.h @@ -40,6 +40,10 @@ int virFirewallDApplyRule(virFirewallLayer layer, char **args, size_t argsLen, bool ignoreErrors, char **output); +int virFirewallDApplyPolicyRichRules(const char *policy, + const char **rules, + size_t rules_count); + int virFirewallDInterfaceSetZone(const char *iface, const char *zone); -- 2.37.3

Eric Garver

10:31 a.m.

New subject: [PATCH 6/8] network: firewalld: add zone for NAT networks

This zone will be used for the NAT network by default. Note that this zone definition omits "forward" aka intra-zone forwarding, because it requires firewalld >= 0.9.0. Signed-off-by: Eric Garver <eric(a)garver.life> --- libvirt.spec.in | 1 + src/network/libvirt-nat.zone | 10 ++++++++++ src/network/meson.build | 5 +++++ 3 files changed, 16 insertions(+) create mode 100644 src/network/libvirt-nat.zone diff --git a/libvirt.spec.in b/libvirt.spec.in index ac5bf7b8653c..6537b9385a0e 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -1920,6 +1920,7 @@ exit 0 %if %{with_firewalld_zone} %{_prefix}/lib/firewalld/zones/libvirt.xml +%{_prefix}/lib/firewalld/zones/libvirt-nat.xml %{_prefix}/lib/firewalld/zones/libvirt-routed.xml %{_prefix}/lib/firewalld/policies/libvirt-routed-in.xml %{_prefix}/lib/firewalld/policies/libvirt-routed-out.xml diff --git a/src/network/libvirt-nat.zone b/src/network/libvirt-nat.zone new file mode 100644 index 000000000000..6ebffb189a56 --- /dev/null +++ b/src/network/libvirt-nat.zone @@ -0,0 +1,10 @@ +<?xml version="1.0" encoding="utf-8"?> +<zone> + <short>libvirt-nat</short> + + <description> + This zone is intended to be used only by NAT libvirt virtual networks - + libvirt will add the bridge devices for all new virtual networks to this + zone by default. + </description> +</zone> diff --git a/src/network/meson.build b/src/network/meson.build index d266bb225a64..fa18cbb8ff62 100644 --- a/src/network/meson.build +++ b/src/network/meson.build @@ -101,6 +101,11 @@ if conf.has('WITH_NETWORK') install_dir: prefix / 'lib' / 'firewalld' / 'zones', rename: [ 'libvirt.xml' ], ) + install_data( + 'libvirt-nat.zone', + install_dir: prefix / 'lib' / 'firewalld' / 'zones', + rename: [ 'libvirt-nat.xml' ], + ) install_data( 'libvirt-routed.zone', install_dir: prefix / 'lib' / 'firewalld' / 'zones', -- 2.37.3

Michal Prívozník

Tuesday, 15 November Tue, 15 Nov

4:21 a.m.

New subject: [PATCH 6/8] network: firewalld: add zone for NAT networks

On 11/10/22 17:31, Eric Garver wrote:

...

This zone will be used for the NAT network by default. Note that this zone definition omits "forward" aka intra-zone forwarding, because it requires firewalld >= 0.9.0.

Yeah, looks like we still aim to support Ubuntu 22.04 which has firewalld-0.8.2 :(

...

Signed-off-by: Eric Garver <eric(a)garver.life> --- libvirt.spec.in | 1 + src/network/libvirt-nat.zone | 10 ++++++++++ src/network/meson.build | 5 +++++ 3 files changed, 16 insertions(+) create mode 100644 src/network/libvirt-nat.zone diff --git a/libvirt.spec.in b/libvirt.spec.in index ac5bf7b8653c..6537b9385a0e 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -1920,6 +1920,7 @@ exit 0 %if %{with_firewalld_zone} %{_prefix}/lib/firewalld/zones/libvirt.xml +%{_prefix}/lib/firewalld/zones/libvirt-nat.xml %{_prefix}/lib/firewalld/zones/libvirt-routed.xml %{_prefix}/lib/firewalld/policies/libvirt-routed-in.xml %{_prefix}/lib/firewalld/policies/libvirt-routed-out.xml diff --git a/src/network/libvirt-nat.zone b/src/network/libvirt-nat.zone new file mode 100644 index 000000000000..6ebffb189a56 --- /dev/null +++ b/src/network/libvirt-nat.zone @@ -0,0 +1,10 @@ +<?xml version="1.0" encoding="utf-8"?> +<zone> + <short>libvirt-nat</short> + + <description> + This zone is intended to be used only by NAT libvirt virtual networks - + libvirt will add the bridge devices for all new virtual networks to this + zone by default. + </description> +</zone> diff --git a/src/network/meson.build b/src/network/meson.build index d266bb225a64..fa18cbb8ff62 100644 --- a/src/network/meson.build +++ b/src/network/meson.build @@ -101,6 +101,11 @@ if conf.has('WITH_NETWORK') install_dir: prefix / 'lib' / 'firewalld' / 'zones', rename: [ 'libvirt.xml' ], ) + install_data( + 'libvirt-nat.zone', + install_dir: prefix / 'lib' / 'firewalld' / 'zones', + rename: [ 'libvirt-nat.xml' ], + ) install_data( 'libvirt-routed.zone', install_dir: prefix / 'lib' / 'firewalld' / 'zones',

Michal

Eric Garver

Thursday, 10 November Thu, 10 Nov

10:31 a.m.

New subject: [PATCH 7/8] network: firewalld: add policies for NAT networks

Signed-off-by: Eric Garver <eric(a)garver.life> --- libvirt.spec.in | 1 + src/network/libvirt-nat-out.policy | 13 +++++++++++++ src/network/libvirt-to-host.policy | 1 + src/network/meson.build | 5 +++++ 4 files changed, 20 insertions(+) create mode 100644 src/network/libvirt-nat-out.policy diff --git a/libvirt.spec.in b/libvirt.spec.in index 6537b9385a0e..6a852d726e55 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -1922,6 +1922,7 @@ exit 0 %{_prefix}/lib/firewalld/zones/libvirt.xml %{_prefix}/lib/firewalld/zones/libvirt-nat.xml %{_prefix}/lib/firewalld/zones/libvirt-routed.xml +%{_prefix}/lib/firewalld/policies/libvirt-nat-out.xml %{_prefix}/lib/firewalld/policies/libvirt-routed-in.xml %{_prefix}/lib/firewalld/policies/libvirt-routed-out.xml %{_prefix}/lib/firewalld/policies/libvirt-to-host.xml diff --git a/src/network/libvirt-nat-out.policy b/src/network/libvirt-nat-out.policy new file mode 100644 index 000000000000..ed19be90c751 --- /dev/null +++ b/src/network/libvirt-nat-out.policy @@ -0,0 +1,13 @@ +<?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-nat" /> + <egress-zone name="ANY" /> + <masquerade /> +</policy> diff --git a/src/network/libvirt-to-host.policy b/src/network/libvirt-to-host.policy index b20aecaf4249..a22952ea1c95 100644 --- a/src/network/libvirt-to-host.policy +++ b/src/network/libvirt-to-host.policy @@ -7,6 +7,7 @@ host. </description> + <ingress-zone name="libvirt-nat" /> <ingress-zone name="libvirt-routed" /> <egress-zone name="HOST" /> diff --git a/src/network/meson.build b/src/network/meson.build index fa18cbb8ff62..34f336fa222e 100644 --- a/src/network/meson.build +++ b/src/network/meson.build @@ -116,6 +116,11 @@ if conf.has('WITH_NETWORK') 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' ], + ) install_data( 'libvirt-routed-out.policy', install_dir: prefix / 'lib' / 'firewalld' / 'policies', -- 2.37.3

Eric Garver

10:31 a.m.

New subject: [PATCH 8/8] network: firewalld: use native NAT networks

Use the new "libvirt-nat" zone for native NAT networks. The "libvirt" zone is still in use, but only to handle DHCP packets. Those won't be dispatched to the "libvirt-zone" because said zone is using sources (instead of interfaces). DHCP packets don't have a valid source address. The use of "libvirt" zone is necessary due to a Linux < 5.5 limitation in which nftables iifname cannot be matched in postrouting hook (i.e. masquerade). In the future, when we can assume Linux 5.5+, we can further improve this by attaching interfaces to the "libvirt-nat" zone instead of using sources. Thus making the "libvirt" zone unnecessary. Signed-off-by: Eric Garver <eric(a)garver.life> --- src/network/bridge_driver_linux.c | 55 +++++++++++++++++++++++++++---- 1 file changed, 48 insertions(+), 7 deletions(-) diff --git a/src/network/bridge_driver_linux.c b/src/network/bridge_driver_linux.c index 42f098ff1f9b..d6c7d378f5f7 100644 --- a/src/network/bridge_driver_linux.c +++ b/src/network/bridge_driver_linux.c @@ -140,7 +140,10 @@ networkUseOnlyFirewallDRules(void) return false; if (virFirewallDPolicyExists("libvirt-routed-out") && - virFirewallDZoneExists("libvirt-routed")) { + virFirewallDZoneExists("libvirt-routed") && + virFirewallDPolicyExists("libvirt-nat-out") && + virFirewallDZoneExists("libvirt-nat") && + virFirewallDZoneExists("libvirt")) { return true; } @@ -825,6 +828,48 @@ networkAddOnlyFirewallDRules(virNetworkDef *def) if (def->forward.type == VIR_NETWORK_FORWARD_ROUTE) { if (virFirewallDInterfaceSetZone(def->bridge, "libvirt-routed") < 0) return -1; + } else if (def->forward.type == VIR_NETWORK_FORWARD_NAT) { + virNetworkIPDef *ipdef; + size_t i; + + /* The initial DHCP packets won't be dispatched to the + * libvirt-nat zone because they don't yet have an IP address. + * The libvirt-nat zone needs to use sources instead of + * interfaces because kernels < 5.5 do not support matching + * iifname in postrouting. + * + * As a workaround, add the interface to the libvirt zone. This + * will allow dhcp to function. Afterwards packets will go to + * the libvirt-nat zone. + */ + if (virFirewallDInterfaceSetZone(def->bridge, "libvirt") < 0) + return -1; + + for (i = 0; + (ipdef = virNetworkDefGetIPByIndex(def, AF_UNSPEC, i)); + i++) { + int prefix = virNetworkIPDefPrefix(ipdef); + g_autofree char *networkstr = NULL; + + if (!(networkstr = virSocketAddrFormatWithPrefix(&ipdef->address, prefix, true))) + return -1; + + if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET) || + def->forward.natIPv6 == VIR_TRISTATE_BOOL_YES) { + if (virFirewallDSourceSetZone(networkstr, "libvirt-nat") < 0) + return -1; + if (def->forward.natIPv6 == VIR_TRISTATE_BOOL_YES) { + const char *rich_rules[] = {"rule family=ipv6 masquerade"}; + size_t rich_rules_count = sizeof(rich_rules) / sizeof(rich_rules[0]); + + if (virFirewallDApplyPolicyRichRules("libvirt-nat-out", rich_rules, rich_rules_count) < 0) + return -1; + } + } else if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET6)) { + if (virFirewallDSourceSetZone(networkstr, "libvirt-routed") < 0) + return -1; + } + } } return 0; @@ -890,10 +935,8 @@ int networkAddFirewallRules(virNetworkDef *def) virNetworkIPDef *ipdef; g_autoptr(virFirewall) fw = virFirewallNew(); - if (!def->bridgeZone && networkUseOnlyFirewallDRules() && - def->forward.type == VIR_NETWORK_FORWARD_ROUTE) { + if (!def->bridgeZone && networkUseOnlyFirewallDRules()) return networkAddOnlyFirewallDRules(def); - } if (virOnce(&createdOnce, networkSetupPrivateChains) < 0) return -1; @@ -968,10 +1011,8 @@ void networkRemoveFirewallRules(virNetworkDef *def) virNetworkIPDef *ipdef; g_autoptr(virFirewall) fw = virFirewallNew(); - if (!def->bridgeZone && networkUseOnlyFirewallDRules() && - def->forward.type == VIR_NETWORK_FORWARD_ROUTE) { + if (!def->bridgeZone && networkUseOnlyFirewallDRules()) return; - } virFirewallStartTransaction(fw, VIR_FIREWALL_TRANSACTION_IGNORE_ERRORS); networkRemoveChecksumFirewallRules(fw, def); -- 2.37.3

Michal Prívozník

Tuesday, 15 November Tue, 15 Nov

4:21 a.m.

On 11/10/22 17:31, Eric Garver wrote:

...

Patches look good to me. You have my: Reviewed-by: Michal Privoznik <mprivozn(a)redhat.com> but I'll wait a bit for Laine, if he wants to express his opinion. Michal

Laine Stump

10:03 a.m.

On 11/15/22 5:21 AM, Michal Prívozník wrote:

...

On 11/10/22 17:31, Eric Garver wrote: > This series further improves the firewalld backend by converting to a > fully native implementation for NAT and routed networks. That is, there > are no iptables rules added by libvirt when the running firewalld is > 0.9.0 or later. > > The major advantage is that firewalld users can use firewall-cmd to > filter the VM traffic and apply their own policies. > > When firewalld < 0.9.0 is present only the "libvirt" zone will be used. > The new "libvirt-nat" and "libvirt-routed" zones are not used. This > maintains compatibility for older distributions (e.g. Ubuntu 20.04). > > Patch 1 is a bug fix for my previous series to avoid a bogus error log. > > Patches 2-3 converts the routed network to native firewalld. > > Patches 4-8 converts the NAT network to native firewalld. It also > introduces the "libvirt-nat" zone. > > Eric Garver (8): > util: virFirewallDGetPolicies: gracefully handle older firewalld > network: firewalld: add networkAddHybridFirewallDRules() > network: firewalld: use native routed networks > util: add virFirewallDSourceSetZone() > util: add virFirewallDApplyPolicyRichRules() > network: firewalld: add zone for NAT networks > network: firewalld: add policies for NAT networks > network: firewalld: use native NAT networks > > libvirt.spec.in | 2 + > src/libvirt_private.syms | 2 + > src/network/bridge_driver_linux.c | 193 ++++++++++++++++++++--------- > src/network/libvirt-nat-out.policy | 13 ++ > src/network/libvirt-nat.zone | 10 ++ > src/network/libvirt-to-host.policy | 1 + > src/network/meson.build | 10 ++ > src/util/virfirewalld.c | 79 +++++++++++- > src/util/virfirewalld.h | 6 + > 9 files changed, 258 insertions(+), 58 deletions(-) > create mode 100644 src/network/libvirt-nat-out.policy > create mode 100644 src/network/libvirt-nat.zone > Patches look good to me. You have my: Reviewed-by: Michal Privoznik <mprivozn(a)redhat.com> but I'll wait a bit for Laine, if he wants to express his opinion.

This series has been on my list of things I need to get to since it arrived, but I've been purposefully not responding in order to avoid distracting my brain from something else I'm working on that is more urgent (supporting passt as a guest interface connection mode). I have pending stuff (in-process on and off for many months now) that adds a separate (configurable) backend for raw nftables that this firewalld-backend mode needs to mesh with. In particular, I don't think it's safe to automatically switch to using a pure firewalld backend any time firewalld is running, because behavior isn't exactly the same as the standard iptables backend (the first example that comes to mind is those horrible dhcp checksum munging rules that are added by libvirt's iptables backend). Probably most of the patches in this series will be untouched by mine, or should be prerequisites to mine, but some will need to be re-jiggered to use my conf-file option and to deal with my other reorganizations. I'll look at it in more detail as soon as I have a first version of passt patches posted, which I'm hoping will happen sometime this week. So please don't push these patches (yet).

Eric Garver

4:16 p.m.

On Tue, Nov 15, 2022 at 11:03:21AM -0500, Laine Stump wrote:

...

On 11/15/22 5:21 AM, Michal Prívozník wrote: > On 11/10/22 17:31, Eric Garver wrote: > > This series further improves the firewalld backend by converting to a > > fully native implementation for NAT and routed networks. That is, there > > are no iptables rules added by libvirt when the running firewalld is > > 0.9.0 or later. > > > > The major advantage is that firewalld users can use firewall-cmd to > > filter the VM traffic and apply their own policies. > > > > When firewalld < 0.9.0 is present only the "libvirt" zone will be used. > > The new "libvirt-nat" and "libvirt-routed" zones are not used. This > > maintains compatibility for older distributions (e.g. Ubuntu 20.04). > > > > Patch 1 is a bug fix for my previous series to avoid a bogus error log. > > > > Patches 2-3 converts the routed network to native firewalld. > > > > Patches 4-8 converts the NAT network to native firewalld. It also > > introduces the "libvirt-nat" zone. > > > > Eric Garver (8): > > util: virFirewallDGetPolicies: gracefully handle older firewalld > > network: firewalld: add networkAddHybridFirewallDRules() > > network: firewalld: use native routed networks > > util: add virFirewallDSourceSetZone() > > util: add virFirewallDApplyPolicyRichRules() > > network: firewalld: add zone for NAT networks > > network: firewalld: add policies for NAT networks > > network: firewalld: use native NAT networks > > > > libvirt.spec.in | 2 + > > src/libvirt_private.syms | 2 + > > src/network/bridge_driver_linux.c | 193 ++++++++++++++++++++--------- > > src/network/libvirt-nat-out.policy | 13 ++ > > src/network/libvirt-nat.zone | 10 ++ > > src/network/libvirt-to-host.policy | 1 + > > src/network/meson.build | 10 ++ > > src/util/virfirewalld.c | 79 +++++++++++- > > src/util/virfirewalld.h | 6 + > > 9 files changed, 258 insertions(+), 58 deletions(-) > > create mode 100644 src/network/libvirt-nat-out.policy > > create mode 100644 src/network/libvirt-nat.zone > > > > Patches look good to me. You have my: > > Reviewed-by: Michal Privoznik <mprivozn(a)redhat.com> > > but I'll wait a bit for Laine, if he wants to express his opinion. This series has been on my list of things I need to get to since it arrived, but I've been purposefully not responding in order to avoid distracting my brain from something else I'm working on that is more urgent (supporting passt as a guest interface connection mode). I have pending stuff (in-process on and off for many months now) that adds a separate (configurable) backend for raw nftables that this firewalld-backend mode needs to mesh with. In particular, I don't think it's safe to automatically switch to using a pure firewalld backend any time firewalld is running, because behavior isn't exactly the same as the standard iptables backend (the first example that comes to mind is those horrible dhcp checksum munging rules that are added by libvirt's iptables backend). Probably most of the patches in this series will be untouched by mine, or should be prerequisites to mine, but some will need to be re-jiggered to use my conf-file option and to deal with my other reorganizations. I'll look at it in more detail as soon as I have a first version of passt patches posted, which I'm hoping will happen sometime this week. So please don't push these patches (yet).

Please take the first patch now. I can resend individually if you'd like. The rest we can sort out and re-spin after your series. Thanks. E.

Michal Prívozník

Wednesday, 16 November Wed, 16 Nov

2:40 a.m.

On 11/15/22 23:16, Eric Garver wrote:

...

On Tue, Nov 15, 2022 at 11:03:21AM -0500, Laine Stump wrote: > On 11/15/22 5:21 AM, Michal Prívozník wrote: >> On 11/10/22 17:31, Eric Garver wrote: >>> This series further improves the firewalld backend by converting to a >>> fully native implementation for NAT and routed networks. That is, there >>> are no iptables rules added by libvirt when the running firewalld is >>> 0.9.0 or later. >>> >>> The major advantage is that firewalld users can use firewall-cmd to >>> filter the VM traffic and apply their own policies. >>> >>> When firewalld < 0.9.0 is present only the "libvirt" zone will be used. >>> The new "libvirt-nat" and "libvirt-routed" zones are not used. This >>> maintains compatibility for older distributions (e.g. Ubuntu 20.04). >>> >>> Patch 1 is a bug fix for my previous series to avoid a bogus error log. >>> >>> Patches 2-3 converts the routed network to native firewalld. >>> >>> Patches 4-8 converts the NAT network to native firewalld. It also >>> introduces the "libvirt-nat" zone. >>> >>> Eric Garver (8): >>> util: virFirewallDGetPolicies: gracefully handle older firewalld >>> network: firewalld: add networkAddHybridFirewallDRules() >>> network: firewalld: use native routed networks >>> util: add virFirewallDSourceSetZone() >>> util: add virFirewallDApplyPolicyRichRules() >>> network: firewalld: add zone for NAT networks >>> network: firewalld: add policies for NAT networks >>> network: firewalld: use native NAT networks >>> >>> libvirt.spec.in | 2 + >>> src/libvirt_private.syms | 2 + >>> src/network/bridge_driver_linux.c | 193 ++++++++++++++++++++--------- >>> src/network/libvirt-nat-out.policy | 13 ++ >>> src/network/libvirt-nat.zone | 10 ++ >>> src/network/libvirt-to-host.policy | 1 + >>> src/network/meson.build | 10 ++ >>> src/util/virfirewalld.c | 79 +++++++++++- >>> src/util/virfirewalld.h | 6 + >>> 9 files changed, 258 insertions(+), 58 deletions(-) >>> create mode 100644 src/network/libvirt-nat-out.policy >>> create mode 100644 src/network/libvirt-nat.zone >>> >> >> Patches look good to me. You have my: >> >> Reviewed-by: Michal Privoznik <mprivozn(a)redhat.com> >> >> but I'll wait a bit for Laine, if he wants to express his opinion. > > This series has been on my list of things I need to get to since it arrived, > but I've been purposefully not responding in order to avoid distracting my > brain from something else I'm working on that is more urgent (supporting > passt as a guest interface connection mode). > > I have pending stuff (in-process on and off for many months now) that adds a > separate (configurable) backend for raw nftables that this firewalld-backend > mode needs to mesh with. In particular, I don't think it's safe to > automatically switch to using a pure firewalld backend any time firewalld is > running, because behavior isn't exactly the same as the standard iptables > backend (the first example that comes to mind is those horrible dhcp > checksum munging rules that are added by libvirt's iptables backend). > > Probably most of the patches in this series will be untouched by mine, or > should be prerequisites to mine, but some will need to be re-jiggered to use > my conf-file option and to deal with my other reorganizations. I'll look at > it in more detail as soon as I have a first version of passt patches posted, > which I'm hoping will happen sometime this week. > > So please don't push these patches (yet). Please take the first patch now. I can resend individually if you'd like. The rest we can sort out and re-spin after your series.

Yeah, the first patch is independent of the rest so unless there's any objection from Laine or Dan I'll push it later today. Michal

Daniel P. Berrangé

3:11 a.m.

On Wed, Nov 16, 2022 at 09:40:41AM +0100, Michal Prívozník wrote:

...

On 11/15/22 23:16, Eric Garver wrote: > On Tue, Nov 15, 2022 at 11:03:21AM -0500, Laine Stump wrote: >> On 11/15/22 5:21 AM, Michal Prívozník wrote: >>> On 11/10/22 17:31, Eric Garver wrote: >>>> This series further improves the firewalld backend by converting to a >>>> fully native implementation for NAT and routed networks. That is, there >>>> are no iptables rules added by libvirt when the running firewalld is >>>> 0.9.0 or later. >>>> >>>> The major advantage is that firewalld users can use firewall-cmd to >>>> filter the VM traffic and apply their own policies. >>>> >>>> When firewalld < 0.9.0 is present only the "libvirt" zone will be used. >>>> The new "libvirt-nat" and "libvirt-routed" zones are not used. This >>>> maintains compatibility for older distributions (e.g. Ubuntu 20.04). >>>> >>>> Patch 1 is a bug fix for my previous series to avoid a bogus error log. >>>> >>>> Patches 2-3 converts the routed network to native firewalld. >>>> >>>> Patches 4-8 converts the NAT network to native firewalld. It also >>>> introduces the "libvirt-nat" zone. >>>> >>>> Eric Garver (8): >>>> util: virFirewallDGetPolicies: gracefully handle older firewalld >>>> network: firewalld: add networkAddHybridFirewallDRules() >>>> network: firewalld: use native routed networks >>>> util: add virFirewallDSourceSetZone() >>>> util: add virFirewallDApplyPolicyRichRules() >>>> network: firewalld: add zone for NAT networks >>>> network: firewalld: add policies for NAT networks >>>> network: firewalld: use native NAT networks >>>> >>>> libvirt.spec.in | 2 + >>>> src/libvirt_private.syms | 2 + >>>> src/network/bridge_driver_linux.c | 193 ++++++++++++++++++++--------- >>>> src/network/libvirt-nat-out.policy | 13 ++ >>>> src/network/libvirt-nat.zone | 10 ++ >>>> src/network/libvirt-to-host.policy | 1 + >>>> src/network/meson.build | 10 ++ >>>> src/util/virfirewalld.c | 79 +++++++++++- >>>> src/util/virfirewalld.h | 6 + >>>> 9 files changed, 258 insertions(+), 58 deletions(-) >>>> create mode 100644 src/network/libvirt-nat-out.policy >>>> create mode 100644 src/network/libvirt-nat.zone >>>> >>> >>> Patches look good to me. You have my: >>> >>> Reviewed-by: Michal Privoznik <mprivozn(a)redhat.com> >>> >>> but I'll wait a bit for Laine, if he wants to express his opinion. >> >> This series has been on my list of things I need to get to since it arrived, >> but I've been purposefully not responding in order to avoid distracting my >> brain from something else I'm working on that is more urgent (supporting >> passt as a guest interface connection mode). >> >> I have pending stuff (in-process on and off for many months now) that adds a >> separate (configurable) backend for raw nftables that this firewalld-backend >> mode needs to mesh with. In particular, I don't think it's safe to >> automatically switch to using a pure firewalld backend any time firewalld is >> running, because behavior isn't exactly the same as the standard iptables >> backend (the first example that comes to mind is those horrible dhcp >> checksum munging rules that are added by libvirt's iptables backend). >> >> Probably most of the patches in this series will be untouched by mine, or >> should be prerequisites to mine, but some will need to be re-jiggered to use >> my conf-file option and to deal with my other reorganizations. I'll look at >> it in more detail as soon as I have a first version of passt patches posted, >> which I'm hoping will happen sometime this week. >> >> So please don't push these patches (yet). > > Please take the first patch now. I can resend individually if you'd > like. > > The rest we can sort out and re-spin after your series. Yeah, the first patch is independent of the rest so unless there's any objection from Laine or Dan I'll push it later today.

Yes, it looks fine. 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 :|

Laine Stump

8:32 a.m.

On 11/16/22 4:11 AM, Daniel P. Berrangé wrote:

...

On Wed, Nov 16, 2022 at 09:40:41AM +0100, Michal Prívozník wrote: > On 11/15/22 23:16, Eric Garver wrote: >> On Tue, Nov 15, 2022 at 11:03:21AM -0500, Laine Stump wrote: >>> On 11/15/22 5:21 AM, Michal Prívozník wrote: >>>> On 11/10/22 17:31, Eric Garver wrote: >>>>> This series further improves the firewalld backend by converting to a >>>>> fully native implementation for NAT and routed networks. That is, there >>>>> are no iptables rules added by libvirt when the running firewalld is >>>>> 0.9.0 or later. >>>>> >>>>> The major advantage is that firewalld users can use firewall-cmd to >>>>> filter the VM traffic and apply their own policies. >>>>> >>>>> When firewalld < 0.9.0 is present only the "libvirt" zone will be used. >>>>> The new "libvirt-nat" and "libvirt-routed" zones are not used. This >>>>> maintains compatibility for older distributions (e.g. Ubuntu 20.04). >>>>> >>>>> Patch 1 is a bug fix for my previous series to avoid a bogus error log. >>>>> >>>>> Patches 2-3 converts the routed network to native firewalld. >>>>> >>>>> Patches 4-8 converts the NAT network to native firewalld. It also >>>>> introduces the "libvirt-nat" zone. >>>>> >>>>> Eric Garver (8): >>>>> util: virFirewallDGetPolicies: gracefully handle older firewalld >>>>> network: firewalld: add networkAddHybridFirewallDRules() >>>>> network: firewalld: use native routed networks >>>>> util: add virFirewallDSourceSetZone() >>>>> util: add virFirewallDApplyPolicyRichRules() >>>>> network: firewalld: add zone for NAT networks >>>>> network: firewalld: add policies for NAT networks >>>>> network: firewalld: use native NAT networks >>>>> >>>>> libvirt.spec.in | 2 + >>>>> src/libvirt_private.syms | 2 + >>>>> src/network/bridge_driver_linux.c | 193 ++++++++++++++++++++--------- >>>>> src/network/libvirt-nat-out.policy | 13 ++ >>>>> src/network/libvirt-nat.zone | 10 ++ >>>>> src/network/libvirt-to-host.policy | 1 + >>>>> src/network/meson.build | 10 ++ >>>>> src/util/virfirewalld.c | 79 +++++++++++- >>>>> src/util/virfirewalld.h | 6 + >>>>> 9 files changed, 258 insertions(+), 58 deletions(-) >>>>> create mode 100644 src/network/libvirt-nat-out.policy >>>>> create mode 100644 src/network/libvirt-nat.zone >>>>> >>>> >>>> Patches look good to me. You have my: >>>> >>>> Reviewed-by: Michal Privoznik <mprivozn(a)redhat.com> >>>> >>>> but I'll wait a bit for Laine, if he wants to express his opinion. >>> >>> This series has been on my list of things I need to get to since it arrived, >>> but I've been purposefully not responding in order to avoid distracting my >>> brain from something else I'm working on that is more urgent (supporting >>> passt as a guest interface connection mode). >>> >>> I have pending stuff (in-process on and off for many months now) that adds a >>> separate (configurable) backend for raw nftables that this firewalld-backend >>> mode needs to mesh with. In particular, I don't think it's safe to >>> automatically switch to using a pure firewalld backend any time firewalld is >>> running, because behavior isn't exactly the same as the standard iptables >>> backend (the first example that comes to mind is those horrible dhcp >>> checksum munging rules that are added by libvirt's iptables backend). >>> >>> Probably most of the patches in this series will be untouched by mine, or >>> should be prerequisites to mine, but some will need to be re-jiggered to use >>> my conf-file option and to deal with my other reorganizations. I'll look at >>> it in more detail as soon as I have a first version of passt patches posted, >>> which I'm hoping will happen sometime this week. >>> >>> So please don't push these patches (yet). >> >> Please take the first patch now. I can resend individually if you'd >> like. >> >> The rest we can sort out and re-spin after your series. > > Yeah, the first patch is independent of the rest so unless there's any > objection from Laine or Dan I'll push it later today. Yes, it looks fine.

Yep, okay with me too.

Daniel P. Berrangé

Tuesday, 15 November Tue, 15 Nov

11:55 a.m.

On Thu, Nov 10, 2022 at 11:31:44AM -0500, Eric Garver wrote:

...

Testing this I'm noticing problematic behaviour even with the existing iptables impl. Specifically, if you have 2 different virtual networks, VMs on the distinct virtual networks are not supposed to be able to talk to each other. And yet, even with the existing iptables impl this is not blocked, and I'm wondering if this is a consequence of the 'iptables' impl being switched to nft. With this pure firewalld impl, I'm not sure how we can stop this cross-network traffic, given that all the virtual network sget put in the same 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 :|

Laine Stump

12:33 p.m.

On 11/15/22 12:55 PM, Daniel P. Berrangé wrote:

...

On Thu, Nov 10, 2022 at 11:31:44AM -0500, Eric Garver wrote: > This series further improves the firewalld backend by converting to a > fully native implementation for NAT and routed networks. That is, there > are no iptables rules added by libvirt when the running firewalld is > 0.9.0 or later. > > The major advantage is that firewalld users can use firewall-cmd to > filter the VM traffic and apply their own policies. > > When firewalld < 0.9.0 is present only the "libvirt" zone will be used. > The new "libvirt-nat" and "libvirt-routed" zones are not used. This > maintains compatibility for older distributions (e.g. Ubuntu 20.04). Testing this I'm noticing problematic behaviour even with the existing iptables impl. Specifically, if you have 2 different virtual networks, VMs on the distinct virtual networks are not supposed to be able to talk to each other. And yet, even with the existing iptables impl this is not blocked, and I'm wondering if this is a consequence of the 'iptables' impl being switched to nft.

Between two routed networks it should allow traffic, but not between two NATed networks, or from routed to NAT. Unless I crossed a wire in my testing setup, I had tested this before pushing Eric's last patches (which fixed incoming traffic to routed networks). I'll check it again.

...

With this pure firewalld impl, I'm not sure how we can stop this cross-network traffic, given that all the virtual network sget put in the same zone.

Interesting point. Yeah, can't have that.

Eric Garver

4:20 p.m.

On Tue, Nov 15, 2022 at 01:33:28PM -0500, Laine Stump wrote:

...

On 11/15/22 12:55 PM, Daniel P. Berrangé wrote: > On Thu, Nov 10, 2022 at 11:31:44AM -0500, Eric Garver wrote: > > This series further improves the firewalld backend by converting to a > > fully native implementation for NAT and routed networks. That is, there > > are no iptables rules added by libvirt when the running firewalld is > > 0.9.0 or later. > > > > The major advantage is that firewalld users can use firewall-cmd to > > filter the VM traffic and apply their own policies. > > > > When firewalld < 0.9.0 is present only the "libvirt" zone will be used. > > The new "libvirt-nat" and "libvirt-routed" zones are not used. This > > maintains compatibility for older distributions (e.g. Ubuntu 20.04). > > Testing this I'm noticing problematic behaviour even with the > existing iptables impl. > > Specifically, if you have 2 different virtual networks, VMs on > the distinct virtual networks are not supposed to be able to > talk to each other. And yet, even with the existing iptables > impl this is not blocked, and I'm wondering if this is a > consequence of the 'iptables' impl being switched to nft. Between two routed networks it should allow traffic, but not between two NATed networks, or from routed to NAT. Unless I crossed a wire in my testing setup, I had tested this before pushing Eric's last patches (which fixed incoming traffic to routed networks). I'll check it again.

If it's not currently being blocked then it can be, e.g. adding a policy to reject "libvirt-nat" --> "libvirt-routed".

...

> With this pure firewalld impl, I'm not sure how we can stop this > cross-network traffic, given that all the virtual network sget > put in the same zone. Interesting point. Yeah, can't have that.

728

days inactive

734

days old

devel@lists.libvirt.org

Manage subscription

22 comments

5 participants

tags (0)

participants (5)

Daniel P. Berrangé
Eric Garver
Eric Garver
Laine Stump
Michal Prívozník

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

2009

2008

2007

2006

2005

[PATCH 0/8] network: firewalld: native support for NAT/routed