[PATCH v3 0/5] network: firewalld: fix routed network
This series fixes routed networks when a newer firewalld (>= 1.0.0) is present [1]. Firewalld 1.0.0 included a change that disallows implicit forwarding between zones [2]. libvirt was relying on this behavior to allow routed networks to function. Firewalld policies are added. Policies have been supported since firewalld 0.9.0. If the running firewall does not support policies, then it will fallback to the current zone only behavior. My goal is to get libvirt to a fully native firewalld backend; no iptables rules. This series is phase 1 of that effort. The next steps are: 1. introduce a "libvirt-nat" zone and policies - the current "libvirt" zone will become obsolete 2. go full native firewalld, do not use iptables directly - currently a hybrid of iptables + firewalld is used v3: - rebase, retest, resend v2: - keep existing libvirt zone as is - remove "<forward />" in libvirt-routed zone because this feature requires firewalld >= 0.9.0. Has no impact since the added policies allow forwarding libvirt-routed <--> ANY zone (including itself). - add probe for policies: virFirewallDGetPolicies(), virFirewallDPolicyExists() [1]: https://bugzilla.redhat.com/show_bug.cgi?id=2055706 [2]: https://github.com/firewalld/firewalld/issues/177 Eric Garver (5): util: add virFirewallDGetPolicies() util: add virFirewallDPolicyExists() network: firewalld: add zone for routed networks network: firewalld: add policies for routed networks network: firewalld: add support for routed networks src/libvirt_private.syms | 2 + src/network/bridge_driver_linux.c | 11 +++- src/network/libvirt-routed-in.policy | 11 ++++ src/network/libvirt-routed-out.policy | 12 +++++ src/network/libvirt-routed.zone | 10 ++++ src/network/libvirt-to-host.policy | 20 ++++++++ src/network/meson.build | 20 ++++++++ src/util/virfirewalld.c | 72 +++++++++++++++++++++++++++ src/util/virfirewalld.h | 2 + 9 files changed, 159 insertions(+), 1 deletion(-) create mode 100644 src/network/libvirt-routed-in.policy create mode 100644 src/network/libvirt-routed-out.policy create mode 100644 src/network/libvirt-routed.zone create mode 100644 src/network/libvirt-to-host.policy -- 2.35.3
Signed-off-by: Eric Garver <eric@garver.life> --- src/libvirt_private.syms | 1 + src/util/virfirewalld.c | 41 ++++++++++++++++++++++++++++++++++++++++ src/util/virfirewalld.h | 1 + 3 files changed, 43 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 25794bc2f417..32c8bdeb23ee 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2370,6 +2370,7 @@ virFirewallStartTransaction; # util/virfirewalld.h virFirewallDApplyRule; virFirewallDGetBackend; +virFirewallDGetPolicies; virFirewallDGetVersion; virFirewallDGetZones; virFirewallDInterfaceSetZone; diff --git a/src/util/virfirewalld.c b/src/util/virfirewalld.c index c909901833f7..0912508dbc45 100644 --- a/src/util/virfirewalld.c +++ b/src/util/virfirewalld.c @@ -223,6 +223,47 @@ virFirewallDGetZones(char ***zones, size_t *nzones) return 0; } +/** + * virFirewallDGetPolicies: + * @policies: array of char *, each entry is a null-terminated policy name + * @npolicies: number of entries in @policies + * + * Get the number of currently active firewalld policies, and their names + * in an array of null-terminated strings. The memory pointed to by + * @policies will belong to the caller, and must be freed. + * + * Returns 0 on success, -1 (and failure logged) on error + */ +int +virFirewallDGetPolicies(char ***policies, size_t *npolicies) +{ + GDBusConnection *sysbus = virGDBusGetSystemBus(); + g_autoptr(GVariant) reply = NULL; + g_autoptr(GVariant) array = NULL; + + *npolicies = 0; + *policies = NULL; + + if (!sysbus) + return -1; + + if (virGDBusCallMethod(sysbus, + &reply, + G_VARIANT_TYPE("(as)"), + NULL, + VIR_FIREWALL_FIREWALLD_SERVICE, + "/org/fedoraproject/FirewallD1", + "org.fedoraproject.FirewallD1.policy", + "getPolicies", + NULL) < 0) + return -1; + + g_variant_get(reply, "(@as)", &array); + *policies = g_variant_dup_strv(array, npolicies); + + return 0; +} + /** * virFirewallDZoneExists: diff --git a/src/util/virfirewalld.h b/src/util/virfirewalld.h index c396802a2f56..ef05896e2b8b 100644 --- a/src/util/virfirewalld.h +++ b/src/util/virfirewalld.h @@ -33,6 +33,7 @@ int virFirewallDGetVersion(unsigned long *version); int virFirewallDGetBackend(void); int virFirewallDIsRegistered(void); int virFirewallDGetZones(char ***zones, size_t *nzones); +int virFirewallDGetPolicies(char ***policies, size_t *npolicies); bool virFirewallDZoneExists(const char *match); int virFirewallDApplyRule(virFirewallLayer layer, char **args, size_t argsLen, -- 2.35.3
Signed-off-by: Eric Garver <eric@garver.life> --- src/libvirt_private.syms | 1 + src/util/virfirewalld.c | 31 +++++++++++++++++++++++++++++++ src/util/virfirewalld.h | 1 + 3 files changed, 33 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 32c8bdeb23ee..92b6062fabda 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2375,6 +2375,7 @@ virFirewallDGetVersion; virFirewallDGetZones; virFirewallDInterfaceSetZone; virFirewallDIsRegistered; +virFirewallDPolicyExists; virFirewallDSynchronize; virFirewallDZoneExists; diff --git a/src/util/virfirewalld.c b/src/util/virfirewalld.c index 0912508dbc45..ad879164c3a8 100644 --- a/src/util/virfirewalld.c +++ b/src/util/virfirewalld.c @@ -296,6 +296,37 @@ virFirewallDZoneExists(const char *match) } +/** + * virFirewallDPolicyExists: + * @match: name of policy to look for + * + * Returns true if the requested policy exists, or false if it doesn't exist + */ +bool +virFirewallDPolicyExists(const char *match) +{ + size_t npolicies = 0, i; + char **policies = NULL; + bool result = false; + + if (virFirewallDGetPolicies(&policies, &npolicies) < 0) + goto cleanup; + + for (i = 0; i < npolicies; i++) { + if (STREQ_NULLABLE(policies[i], match)) + result = true; + } + + cleanup: + VIR_DEBUG("Requested policy '%s' %s exist", + match, result ? "does" : "doesn't"); + for (i = 0; i < npolicies; i++) + VIR_FREE(policies[i]); + VIR_FREE(policies); + return result; +} + + /** * virFirewallDApplyRule: * @layer: which layer to apply the rule to diff --git a/src/util/virfirewalld.h b/src/util/virfirewalld.h index ef05896e2b8b..fa4c9e702ccb 100644 --- a/src/util/virfirewalld.h +++ b/src/util/virfirewalld.h @@ -35,6 +35,7 @@ int virFirewallDIsRegistered(void); int virFirewallDGetZones(char ***zones, size_t *nzones); int virFirewallDGetPolicies(char ***policies, size_t *npolicies); bool virFirewallDZoneExists(const char *match); +bool virFirewallDPolicyExists(const char *match); int virFirewallDApplyRule(virFirewallLayer layer, char **args, size_t argsLen, bool ignoreErrors, -- 2.35.3
This zone will be used for the routed 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@garver.life> --- src/network/libvirt-routed.zone | 10 ++++++++++ src/network/meson.build | 5 +++++ 2 files changed, 15 insertions(+) create mode 100644 src/network/libvirt-routed.zone diff --git a/src/network/libvirt-routed.zone b/src/network/libvirt-routed.zone new file mode 100644 index 000000000000..ed7dd936a242 --- /dev/null +++ b/src/network/libvirt-routed.zone @@ -0,0 +1,10 @@ +<?xml version="1.0" encoding="utf-8"?> +<zone> + <short>libvirt-routed</short> + + <description> + This zone is intended to be used only by routed 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 395074a0a0a6..dcb31af6448b 100644 --- a/src/network/meson.build +++ b/src/network/meson.build @@ -101,5 +101,10 @@ if conf.has('WITH_NETWORK') install_dir: prefix / 'lib' / 'firewalld' / 'zones', rename: [ 'libvirt.xml' ], ) + install_data( + 'libvirt-routed.zone', + install_dir: prefix / 'lib' / 'firewalld' / 'zones', + rename: [ 'libvirt-routed.xml' ], + ) endif endif -- 2.35.3
Signed-off-by: Eric Garver <eric@garver.life> --- src/network/libvirt-routed-in.policy | 11 +++++++++++ src/network/libvirt-routed-out.policy | 12 ++++++++++++ src/network/libvirt-to-host.policy | 20 ++++++++++++++++++++ src/network/meson.build | 15 +++++++++++++++ 4 files changed, 58 insertions(+) create mode 100644 src/network/libvirt-routed-in.policy create mode 100644 src/network/libvirt-routed-out.policy create mode 100644 src/network/libvirt-to-host.policy diff --git a/src/network/libvirt-routed-in.policy b/src/network/libvirt-routed-in.policy new file mode 100644 index 000000000000..dd691efbb64c --- /dev/null +++ b/src/network/libvirt-routed-in.policy @@ -0,0 +1,11 @@ +<?xml version="1.0" encoding="utf-8"?> +<policy target="ACCEPT"> + <short>libvirt-routed-in</short> + + <description> + This policy is used to allow routed traffic to the virtual machines. + </description> + + <ingress-zone name="ANY" /> + <egress-zone name="libvirt-routed" /> +</policy> diff --git a/src/network/libvirt-routed-out.policy b/src/network/libvirt-routed-out.policy new file mode 100644 index 000000000000..efa0030569d6 --- /dev/null +++ b/src/network/libvirt-routed-out.policy @@ -0,0 +1,12 @@ +<?xml version="1.0" encoding="utf-8"?> +<policy target="ACCEPT"> + <short>libvirt-routed-out</short> + + <description> + This policy is used to allow routed virtual machine traffic to the rest of + the network. + </description> + + <ingress-zone name="libvirt-routed" /> + <egress-zone name="ANY" /> +</policy> diff --git a/src/network/libvirt-to-host.policy b/src/network/libvirt-to-host.policy new file mode 100644 index 000000000000..b20aecaf4249 --- /dev/null +++ b/src/network/libvirt-to-host.policy @@ -0,0 +1,20 @@ +<?xml version="1.0" encoding="utf-8"?> +<policy target="REJECT"> + <short>libvirt-to-host</short> + + <description> + This policy is used to filter traffic from virtual machines to the + host. + </description> + + <ingress-zone name="libvirt-routed" /> + <egress-zone name="HOST" /> + + <protocol value='icmp'/> + <protocol value='ipv6-icmp'/> + <service name='dhcp'/> + <service name='dhcpv6'/> + <service name='dns'/> + <service name='ssh'/> + <service name='tftp'/> +</policy> diff --git a/src/network/meson.build b/src/network/meson.build index dcb31af6448b..26b95367748f 100644 --- a/src/network/meson.build +++ b/src/network/meson.build @@ -106,5 +106,20 @@ if conf.has('WITH_NETWORK') install_dir: prefix / 'lib' / 'firewalld' / 'zones', rename: [ 'libvirt-routed.xml' ], ) + install_data( + 'libvirt-to-host.policy', + install_dir: prefix / 'lib' / 'firewalld' / 'policies', + rename: [ 'libvirt-to-host.xml' ], + ) + install_data( + 'libvirt-routed-out.policy', + install_dir: prefix / 'lib' / 'firewalld' / 'policies', + rename: [ 'libvirt-routed-out.xml' ], + ) + install_data( + 'libvirt-routed-in.policy', + install_dir: prefix / 'lib' / 'firewalld' / 'policies', + rename: [ 'libvirt-routed-in.xml' ], + ) endif endif -- 2.35.3
Signed-off-by: Eric Garver <eric@garver.life> --- src/network/bridge_driver_linux.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/network/bridge_driver_linux.c b/src/network/bridge_driver_linux.c index a0f593b06636..d9597d91beed 100644 --- a/src/network/bridge_driver_linux.c +++ b/src/network/bridge_driver_linux.c @@ -857,8 +857,17 @@ int networkAddFirewallRules(virNetworkDef *def) * 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 (virFirewallDZoneExists("libvirt")) { + 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 { -- 2.35.3
Hey Eric! I *finally* set things up to test this adequately, and it all looks good and is operating properly. The one nit I found with the content of the patches was that the new zone/policy files weren't added to the specfile, so I've done that. I'm now ready to push all the patches, but wanted to put more explanation into this final patch that turns it all on. Does the following sound okay to you?: network: firewalld: allow incoming connections to guests on routed networks Prior to firewalld version 0.9.0, the default action of ACCEPT in the "libvirt" zone (subsequently overridden with a lower priority "REJECT" action) would result in an implicit rule that allowed incoming sessions through the zone; libvirt relied on this implicit rule to permit incoming connections to guests that were connected via a libvirt "routed" network. Starting in firewalld 0.9.0, the rules generated for this same zonefile changed such that incoming sessions through the libvirt zone were no longer allowed, breaking the longstanding convention that they should be allowed (only for routed networks). This patch changes the zone for routed networks from "libvirt" to the newly-added "libvirt-routed" zone so that incoming sessions to guests on routed networks are once again allowed. Resolves: https://bugzilla.redhat.com/2055706 " On 9/22/22 11:13 AM, Eric Garver wrote:
Signed-off-by: Eric Garver <eric@garver.life> --- src/network/bridge_driver_linux.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/src/network/bridge_driver_linux.c b/src/network/bridge_driver_linux.c index a0f593b06636..d9597d91beed 100644 --- a/src/network/bridge_driver_linux.c +++ b/src/network/bridge_driver_linux.c @@ -857,8 +857,17 @@ int networkAddFirewallRules(virNetworkDef *def) * 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 (virFirewallDZoneExists("libvirt")) { + 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 {
On Mon, Oct 24, 2022 at 09:38:17AM -0400, Laine Stump wrote:
Hey Eric!
I *finally* set things up to test this adequately, and it all looks good and is operating properly. The one nit I found with the content of the patches was that the new zone/policy files weren't added to the specfile, so I've done that.
Thanks! Sorry I missed it.
I'm now ready to push all the patches, but wanted to put more explanation into this final patch that turns it all on. Does the following sound okay to you?:
network: firewalld: allow incoming connections to guests on routed networks
Prior to firewalld version 0.9.0, the default action of ACCEPT in the
s/0.9.0/1.0.0/
"libvirt" zone (subsequently overridden with a lower priority "REJECT" action) would result in an implicit rule that allowed incoming sessions through the zone; libvirt relied on this implicit rule to permit incoming connections to guests that were connected via a libvirt "routed" network.
Starting in firewalld 0.9.0, the rules generated for this same
s/0.9.0/1.0.0/
zonefile changed such that incoming sessions through the libvirt zone were no longer allowed, breaking the longstanding convention that they should be allowed (only for routed networks).
The behavior changed in firewalld 1.0.0. However, the new zone, "libvirt-routed", will be active when firewalld >= 0.9.0 is present.
This patch changes the zone for routed networks from "libvirt" to the newly-added "libvirt-routed" zone so that incoming sessions to guests on routed networks are once again allowed.
Resolves: https://bugzilla.redhat.com/2055706
Otherwise, lgtm. Thanks again. Eric.
"
On 9/22/22 11:13 AM, Eric Garver wrote:
Signed-off-by: Eric Garver <eric@garver.life> --- src/network/bridge_driver_linux.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/src/network/bridge_driver_linux.c b/src/network/bridge_driver_linux.c index a0f593b06636..d9597d91beed 100644 --- a/src/network/bridge_driver_linux.c +++ b/src/network/bridge_driver_linux.c @@ -857,8 +857,17 @@ int networkAddFirewallRules(virNetworkDef *def) * 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 (virFirewallDZoneExists("libvirt")) { + 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 {
participants (3)
-
Eric Garver -
Eric Garver -
Laine Stump