A couple places in the docs didn't get updated when the forward mode "open" was added. Signed-off-by: Laine Stump <laine(a)laine.org&gt; --- docs/formatnetwork.html.in | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in index 363a72bbc9..156cfae4ec 100644 --- a/docs/formatnetwork.html.in +++ b/docs/formatnetwork.html.in @@ -107,13 +107,13 @@ may also be connected to the LAN. When defining a new network with a <code>&lt;forward&gt;</code> mode of - "nat" or "route" (or an isolated network with + "nat", "route", or "open" (or an isolated network with no <code>&lt;forward&gt;</code> element), libvirt will automatically generate a unique name for the bridge device if none is given, and this name will be permanently stored in the network configuration so that that the same name will be used every time the network is started. For these types of networks - (nat, routed, and isolated), a bridge name beginning with the + (nat, route, open, and isolated), a bridge name beginning with the prefix "virbr" is recommended (and that is what is auto-generated), but not enforced. Attribute <code>stp</code> specifies if Spanning Tree Protocol -- 2.20.1

Since I'm going to be adding at least one more firewalld-specific function, this seems like a good time to separate the code that's unique to firewalld from the more-generic "firewall" file. Signed-off-by: Laine Stump <laine(a)laine.org&gt; --- include/libvirt/virterror.h | 1 + src/libvirt_private.syms | 3 + src/util/Makefile.inc.am | 2 + src/util/virerror.c | 1 + src/util/virfirewall.c | 86 ++---------------------- src/util/virfirewalld.c | 128 ++++++++++++++++++++++++++++++++++++ src/util/virfirewalld.h | 33 ++++++++++ src/util/virfirewallpriv.h | 2 - tests/virfirewalltest.c | 1 + 9 files changed, 173 insertions(+), 84 deletions(-) create mode 100644 src/util/virfirewalld.c create mode 100644 src/util/virfirewalld.h diff --git a/include/libvirt/virterror.h b/include/libvirt/virterror.h index fbbe2d5624..3c19ff5e2e 100644 --- a/include/libvirt/virterror.h +++ b/include/libvirt/virterror.h @@ -131,6 +131,7 @@ typedef enum { VIR_FROM_PERF = 65, /* Error from perf */ VIR_FROM_LIBSSH = 66, /* Error from libssh connection transport */ VIR_FROM_RESCTRL = 67, /* Error from resource control */ + VIR_FROM_FIREWALLD = 68, /* Error from firewalld */ # ifdef VIR_ENUM_SENTINELS VIR_ERR_DOMAIN_LAST diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c3d6306809..583868f422 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1918,6 +1918,9 @@ virFirewallSetLockOverride; virFirewallStartRollback; virFirewallStartTransaction; +# util/virfirewalld.h +virFirewallDApplyRule; +virFirewallDStatus; # util/virfirmware.h virFirmwareFreeList; diff --git a/src/util/Makefile.inc.am b/src/util/Makefile.inc.am index 4295babac3..0295a1c7d0 100644 --- a/src/util/Makefile.inc.am +++ b/src/util/Makefile.inc.am @@ -64,6 +64,8 @@ UTIL_SOURCES = \ util/virfirewall.c \ util/virfirewall.h \ util/virfirewallpriv.h \ + util/virfirewalld.c \ + util/virfirewalld.h \ util/virfirmware.c \ util/virfirmware.h \ util/virgettext.c \ diff --git a/src/util/virerror.c b/src/util/virerror.c index 61b47d2be0..ae1efa72d8 100644 --- a/src/util/virerror.c +++ b/src/util/virerror.c @@ -138,6 +138,7 @@ VIR_ENUM_IMPL(virErrorDomain, VIR_ERR_DOMAIN_LAST, "Perf", /* 65 */ "Libssh transport layer", "Resource control", + "FirewallD", ) diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c index 5a0cf95a44..d5d647fcc4 100644 --- a/src/util/virfirewall.c +++ b/src/util/virfirewall.c @@ -24,12 +24,12 @@ #define LIBVIRT_VIRFIREWALLPRIV_H_ALLOW #include "virfirewallpriv.h" +#include "virfirewalld.h" #include "virerror.h" #include "virutil.h" #include "virstring.h" #include "vircommand.h" #include "virlog.h" -#include "virdbus.h" #include "virfile.h" #include "virthread.h" @@ -46,11 +46,6 @@ VIR_ENUM_IMPL(virFirewallLayerCommand, VIR_FIREWALL_LAYER_LAST, IPTABLES_PATH, IP6TABLES_PATH); -VIR_ENUM_DECL(virFirewallLayerFirewallD) -VIR_ENUM_IMPL(virFirewallLayerFirewallD, VIR_FIREWALL_LAYER_LAST, - "eb", "ipv4", "ipv6") - - struct _virFirewallRule { virFirewallLayer layer; @@ -152,7 +147,7 @@ virFirewallValidateBackend(virFirewallBackend backend) VIR_DEBUG("Validating backend %d", backend); if (backend == VIR_FIREWALL_BACKEND_AUTOMATIC || backend == VIR_FIREWALL_BACKEND_FIREWALLD) { - int rv = virDBusIsServiceRegistered(VIR_FIREWALL_FIREWALLD_SERVICE); + int rv = virFirewallDStatus(); VIR_DEBUG("Firewalld is registered ? %d", rv); if (rv < 0) { @@ -712,81 +707,8 @@ virFirewallApplyRuleFirewallD(virFirewallRulePtr rule, bool ignoreErrors, char **output) { - const char *ipv = virFirewallLayerFirewallDTypeToString(rule->layer); - DBusConnection *sysbus = virDBusGetSystemBus(); - DBusMessage *reply = NULL; - virError error; - int ret = -1; - - if (!sysbus) - return -1; - - memset(&error, 0, sizeof(error)); - - if (!ipv) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unknown firewall layer %d"), - rule->layer); - goto cleanup; - } - - if (virDBusCallMethod(sysbus, - &reply, - &error, - VIR_FIREWALL_FIREWALLD_SERVICE, - "/org/fedoraproject/FirewallD1", - "org.fedoraproject.FirewallD1.direct", - "passthrough", - "sa&s", - ipv, - (int)rule->argsLen, - rule->args) < 0) - goto cleanup; - - if (error.level == VIR_ERR_ERROR) { - /* - * As of firewalld-0.3.9.3-1.fc20.noarch the name and - * message fields in the error look like - * - * name="org.freedesktop.DBus.Python.dbus.exceptions.DBusException" - * message="COMMAND_FAILED: '/sbin/iptables --table filter --delete - * INPUT --in-interface virbr0 --protocol udp --destination-port 53 - * --jump ACCEPT' failed: iptables: Bad rule (does a matching rule - * exist in that chain?)." - * - * We'd like to only ignore DBus errors precisely related to the failure - * of iptables/ebtables commands. A well designed DBus interface would - * return specific named exceptions not the top level generic python dbus - * exception name. With this current scheme our only option is todo a - * sub-string match for 'COMMAND_FAILED' on the message. eg like - * - * if (ignoreErrors && - * STREQ(error.name, - * "org.freedesktop.DBus.Python.dbus.exceptions.DBusException") && - * STRPREFIX(error.message, "COMMAND_FAILED")) - * ... - * - * But this risks our error detecting code being broken if firewalld changes - * ever alter the message string, so we're avoiding doing that. - */ - if (ignoreErrors) { - VIR_DEBUG("Ignoring error '%s': '%s'", - error.str1, error.message); - } else { - virReportErrorObject(&error); - goto cleanup; - } - } else { - if (virDBusMessageRead(reply, "s", output) < 0) - goto cleanup; - } - - ret = 0; - - cleanup: - virResetError(&error); - virDBusMessageUnref(reply); - return ret; + /* wrapper necessary because virFirewallRule is a private struct */ + return virFirewallDApplyRule(rule->layer, rule->args, rule->argsLen, ignoreErrors, output); } static int diff --git a/src/util/virfirewalld.c b/src/util/virfirewalld.c new file mode 100644 index 0000000000..0dc2b3de08 --- /dev/null +++ b/src/util/virfirewalld.c @@ -0,0 +1,128 @@ +/* + * virfirewalld.c: support for firewalld (https://firewalld.org) + * + * Copyright (C) 2019 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>;. + */ + +#include <config.h> + +#include <stdarg.h> + +#include "virfirewall.h" +#include "virfirewalld.h" +#include "virerror.h" +#include "virutil.h" +#include "virlog.h" +#include "virdbus.h" + +#define VIR_FROM_THIS VIR_FROM_FIREWALLD + +VIR_LOG_INIT("util.firewalld"); + +VIR_ENUM_DECL(virFirewallLayerFirewallD) +VIR_ENUM_IMPL(virFirewallLayerFirewallD, VIR_FIREWALL_LAYER_LAST, + "eb", "ipv4", "ipv6") + +int +virFirewallDStatus(void) +{ + return virDBusIsServiceRegistered(VIR_FIREWALL_FIREWALLD_SERVICE); +} + + +int +virFirewallDApplyRule(virFirewallLayer layer, + char **args, size_t argsLen, + bool ignoreErrors, + char **output) +{ + const char *ipv = virFirewallLayerFirewallDTypeToString(layer); + DBusConnection *sysbus = virDBusGetSystemBus(); + DBusMessage *reply = NULL; + virError error; + int ret = -1; + + if (!sysbus) + return -1; + + memset(&error, 0, sizeof(error)); + + if (!ipv) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unknown firewall layer %d"), + layer); + goto cleanup; + } + + if (virDBusCallMethod(sysbus, + &reply, + &error, + VIR_FIREWALL_FIREWALLD_SERVICE, + "/org/fedoraproject/FirewallD1", + "org.fedoraproject.FirewallD1.direct", + "passthrough", + "sa&s", + ipv, + (int)argsLen, + args) < 0) + goto cleanup; + + if (error.level == VIR_ERR_ERROR) { + /* + * As of firewalld-0.3.9.3-1.fc20.noarch the name and + * message fields in the error look like + * + * name="org.freedesktop.DBus.Python.dbus.exceptions.DBusException" + * message="COMMAND_FAILED: '/sbin/iptables --table filter --delete + * INPUT --in-interface virbr0 --protocol udp --destination-port 53 + * --jump ACCEPT' failed: iptables: Bad rule (does a matching rule + * exist in that chain?)." + * + * We'd like to only ignore DBus errors precisely related to the failure + * of iptables/ebtables commands. A well designed DBus interface would + * return specific named exceptions not the top level generic python dbus + * exception name. With this current scheme our only option is todo a + * sub-string match for 'COMMAND_FAILED' on the message. eg like + * + * if (ignoreErrors && + * STREQ(error.name, + * "org.freedesktop.DBus.Python.dbus.exceptions.DBusException") && + * STRPREFIX(error.message, "COMMAND_FAILED")) + * ... + * + * But this risks our error detecting code being broken if firewalld changes + * ever alter the message string, so we're avoiding doing that. + */ + if (ignoreErrors) { + VIR_DEBUG("Ignoring error '%s': '%s'", + error.str1, error.message); + } else { + virReportErrorObject(&error); + goto cleanup; + } + } else { + if (virDBusMessageRead(reply, "s", output) < 0) + goto cleanup; + } + + ret = 0; + + cleanup: + virResetError(&error); + virDBusMessageUnref(reply); + return ret; +} diff --git a/src/util/virfirewalld.h b/src/util/virfirewalld.h new file mode 100644 index 0000000000..c1c929399a --- /dev/null +++ b/src/util/virfirewalld.h @@ -0,0 +1,33 @@ +/* + * virfirewalld.h: support for firewalld (https://firewalld.org) + * + * Copyright (C) 2019 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>;. + */ + +#ifndef LIBVIRT_VIRFIREWALLD_H +# define LIBVIRT_VIRFIREWALLD_H + +# define VIR_FIREWALL_FIREWALLD_SERVICE "org.fedoraproject.FirewallD1" + +int virFirewallDStatus(void); + +int virFirewallDApplyRule(virFirewallLayer layer, + char **args, size_t argsLen, + bool ignoreErrors, + char **output); + +#endif /* LIBVIRT_VIRFIREWALLD_H */ diff --git a/src/util/virfirewallpriv.h b/src/util/virfirewallpriv.h index efa94a7da4..7c31d0680d 100644 --- a/src/util/virfirewallpriv.h +++ b/src/util/virfirewallpriv.h @@ -27,8 +27,6 @@ # include "virfirewall.h" -# define VIR_FIREWALL_FIREWALLD_SERVICE "org.fedoraproject.FirewallD1" - typedef enum { VIR_FIREWALL_BACKEND_AUTOMATIC, VIR_FIREWALL_BACKEND_DIRECT, diff --git a/tests/virfirewalltest.c b/tests/virfirewalltest.c index 63b9ced820..573ab1f9cd 100644 --- a/tests/virfirewalltest.c +++ b/tests/virfirewalltest.c @@ -27,6 +27,7 @@ # include "vircommandpriv.h" # define LIBVIRT_VIRFIREWALLPRIV_H_ALLOW # include "virfirewallpriv.h" +# include "virfirewalld.h" # include "virmock.h" # define LIBVIRT_VIRDBUSPRIV_H_ALLOW # include "virdbuspriv.h" -- 2.20.1

On 1/9/19 9:57 PM, Laine Stump wrote: [...] I was also looking as a learning opportunity, but I see Dan is reviewing as well... Still I'll point out what I have in any case. oddly enough I had similar thoughts regarding that moved hunk with the (error.level == VIR_ERR_ERROR) checking... Just a syntax/spacing NIT here about 2 blank lines seems to be the norm between groupings. So need one before and one after. [...] John

On Wed, Jan 09, 2019 at 09:57:36PM -0500, Laine Stump wrote: [..] The valid priority range is [-32768, 32767]. You may want to change this to 32767 to make sure it's the lowest precedence possible. Although, since libvirt completely controls this zone it won't matter unless libvirt or the user adds other rich rules.

On 1/15/19 11:39 AM, Daniel P. Berrangé wrote: Depending on the firewalld version won't give us correct results, since the patches to support rule priorities could be / will be backported to older firewalld versions on some distros. Of course we could make failure to set the zone to "libvirt" a fatal error - that is really the most reliable proxy we have for the question "Does this firewalld support rule priorities?" That's a bit draconian though, since it means that "new" libvirt will fail to start networks on all distros that haven't yet updated their firewalld version (or backported the necessary patches). In particular, if they haven't updated firewalld, but they're still using the iptables backend anyway, we shouldn't be whining (although for consistency we should still set the zone to "libvirt" when possible, even if the backend is iptables,) Yep, Yeah, the examples I saw had everything in a single line, and that made me think maybe something in firewalld requires it to be a single line, so I avoided breaking it up. I guess I *could* go to the horrible great trouble of actually trying it out with line breaks and see if it's accepted :-) 1) see the comment above about the unreliability of version checks. 2) The version / build of firewalld present at build time is really irrelevant. As a matter of fact, we currently don't require firewalld to be present *at all* at build time (or at runtime - we just use it if it's there and active, otherwise we go directly to iptables/ebtables). 3) I actually *want* the bug reported to distro maintainers, to push them to update their firewalld sooner rather than later. On the other hand, it's much nicer if the error is reported only when someone tries to configure things in a manner that won't work, i.e. "backend==nftables, but firewalld doesn't support rule priorities". So here's an idea - how about if we do this: Once when libvirtd is started:   retrieve a list of zones and check for presence of the libvirt zone. Save this somewhere. Each time a network is started: if (libvirt zone doesn't exist) {     if (firewalldbackend == nftables) {        Error "Hey you!! Change the firewalld backend to nftables or update firewalld!!"     } } else {     put the bridge in the libvirt zone (regardless of nftables/iptables) } This way if firewalld is too old to support rich rule priorities, but the backend is iptables, the network silently functions as it always has (i.e. in the default zone). *AND* as long as firewalld is new enough to support rule priorities, behavior will be consistent no matter what the backend is set to. But if a distro or user tries to set the nftables backend (meaning that networking can't work without rule priorities) and firewalld doesn't support rule priorities, *then* it fails to start the network and logs an error. (in the meantime, we haven't relied on version number checks, and a system with old firewalld can still build a libvirt that supports new firewalld) Does this sound reasonable?

On 1/9/19 9:57 PM, Laine Stump wrote: When the commit message is longer/larger than the patch ;-) /me wonders how much of the above can or even should be described in https://libvirt.org/firewall.html. For those that really know, understand, care, etc. about firewalls getting to understand what role libvirt plays in things could be very helpful. Besides you have so much to say now and before you forget it again... Should this have a with_firewalld like others below? Of course I find/note the conditional with_firewalld check around line 84 and then the unconditional setting around line 131 to be odd, but for consistency... So something is causing me to wonder if we have to do 'anything' here with respect to file security like other %attr's? John [...]

Since we're setting the zone anyway, it will be useful to allow setting a different (custom) zone for each network. This will be done by adding a "zone" attribute to the "bridge" element, e.g.: ... <bridge name='virbr0' zone='myzone'/> ... If a zone is specified in the config and it can't be honored, this will be an error. If no zone is specified and the default zone ("libvirt") can't be set, it will be ignored. (BTW, Federico Simoncelli proposed a similar patch 5 1.2 years (!!) ago, but I misunderstood its usefulness at first, and by the time I did we were both too busy to revisit it. libvirt's code has changed so much in the intervening time that it was simpler to just rewrite from scratch) Signed-off-by: Laine Stump <laine(a)laine.org&gt; --- docs/formatnetwork.html.in | 17 +++++++++ docs/news.xml | 40 ++++++++++++++++++++++ docs/schemas/basictypes.rng | 6 ++++ docs/schemas/network.rng | 6 ++++ src/conf/network_conf.c | 14 ++++++-- src/conf/network_conf.h | 1 + src/network/bridge_driver_linux.c | 30 ++++++++++++---- tests/networkxml2xmlin/routed-network.xml | 2 +- tests/networkxml2xmlout/routed-network.xml | 2 +- 9 files changed, 107 insertions(+), 11 deletions(-) diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in index 156cfae4ec..4aecdfe8e0 100644 --- a/docs/formatnetwork.html.in +++ b/docs/formatnetwork.html.in @@ -152,6 +152,23 @@ <span class="since">Since 1.2.11, requires kernel 3.17 or newer</span> </p> + + <p> + The optional <code>zone</code> attribute of + the <code>bridge</code> element is used to specify + the <a href="https://firewalld.org">firewalld</a> + zone for the bridge of a network with <code>forward</code> + mode of "nat", "route", "open", or one with + no <code>forward</code> specified. By default, the bridges + of all virtual networks with these forward modes are placed + in the firewalld zone named "libvirt", which permits + incoming DNS, DHCP, TFTP, and SSH to the host from guests on + the network. This behavior can be changed either by + modifying the libvirt zone (using firewalld management + tools), or by placing the network in a different zone (which + will also be managed using firewalld tools). + <span class="since">Since 5.0.0</span> + </p> </dd> <dt><code>mtu</code></dt> diff --git a/docs/news.xml b/docs/news.xml index 8c608cdc36..d894821ed5 100644 --- a/docs/news.xml +++ b/docs/news.xml @@ -58,6 +58,19 @@ trunking configuration. </description> </change> + <change> + <summary> + network: support setting a firewalld "zone" for all virtual network bridges + </summary> + <description> + All libvirt virtual networks with bridges managed by libvirt + (i.e. those with <code>forward</code> mode of "nat", + "route", "open", or no forward mode) will now be placed in a + special firewalld zone called "libvirt" by default, and the + zone of any network bridge can be changed using the <code>zone</code> + attribute of the network's <code>bridge</code> element. + </description> + </change> </section> <section title="Removed features"> <change> @@ -123,6 +136,33 @@ </change> </section> <section title="Bug fixes"> + <change> + <summary> + network: fix virtual networks on systems using firewalld+nftables + </summary> + <description> + Because of the transitional state of firewalld's new support + for nftables, not all iptables features required by libvirt + are yet available, so libvirt must continue to use iptables + for its own packet filtering rules even when the firewalld + backend is set to use nftables, but due to the way iptables + support is implemented in kernels using nftables (iptables + rules are converted to nftables rules and processed in a + separate hook from the native nftables rules), guest + networking was broken on hosts with firewalld configured to + use nftables as the backend. This has been fixed by putting + libvirt-managed bridges in their own firewalld zone, so that + guest traffic can be forwarded beyond the host, and so that + host services can be exposed to guests on the virtual + network without opening up those same services to the rest + of the physical network. This means that host access from + virtual machines is no longer controlled by the firewalld + default zone (usually "public"), but rather by the new + firewalld zone called "libvirt" (unless configured otherwise + using the new <code>zone</code> attribute of the network + <code>bridge</code> element). + </description> + </change> </section> </release> <release version="v4.10.0" date="2018-12-03"> diff --git a/docs/schemas/basictypes.rng b/docs/schemas/basictypes.rng index 9a63720ff7..9b3dcad4a5 100644 --- a/docs/schemas/basictypes.rng +++ b/docs/schemas/basictypes.rng @@ -279,6 +279,12 @@ </data> </define> + <define name="zoneName"> + <data type="string"> + <param name="pattern">[a-zA-Z0-9_\-]+</param> + </data> + </define> + <define name="filePath"> <data type="string"> <param name="pattern">.+</param> diff --git a/docs/schemas/network.rng b/docs/schemas/network.rng index f37c422bf3..2a6e3358fd 100644 --- a/docs/schemas/network.rng +++ b/docs/schemas/network.rng @@ -58,6 +58,12 @@ </attribute> </optional> + <optional> + <attribute name="zone"> + <ref name="zoneName"/> + </attribute> + </optional> + <optional> <attribute name="stp"> <ref name="virOnOff"/> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index e035d8aba7..b09cb1dae2 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -203,6 +203,7 @@ virNetworkDefFree(virNetworkDefPtr def) VIR_FREE(def->name); VIR_FREE(def->bridge); + VIR_FREE(def->bridgeZone); VIR_FREE(def->domain); virNetworkForwardDefClear(&def->forward); @@ -1684,6 +1685,7 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) /* Parse bridge information */ def->bridge = virXPathString("string(./bridge[1]/@name)", ctxt); + def->bridgeZone = virXPathString("string(./bridge[1]/@zone)", ctxt); stp = virXPathString("string(./bridge[1]/@stp)", ctxt); def->stp = (stp && STREQ(stp, "off")) ? false : true; @@ -1920,6 +1922,13 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) def->name); goto error; } + if (def->bridgeZone) { + virReportError(VIR_ERR_XML_ERROR, + _("bridge zone not allowed in %s mode (network '%s')"), + virNetworkForwardTypeToString(def->forward.type), + def->name); + goto error; + } if (def->macTableManager) { virReportError(VIR_ERR_XML_ERROR, _("bridge macTableManager setting not allowed " @@ -1931,9 +1940,9 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) ATTRIBUTE_FALLTHROUGH; case VIR_NETWORK_FORWARD_BRIDGE: - if (def->delay || stp) { + if (def->delay || stp || def->bridgeZone) { virReportError(VIR_ERR_XML_ERROR, - _("bridge delay/stp options only allowed in " + _("bridge delay/stp/zone options only allowed in " "route, nat, and isolated mode, not in %s " "(network '%s')"), virNetworkForwardTypeToString(def->forward.type), @@ -2508,6 +2517,7 @@ virNetworkDefFormatBuf(virBufferPtr buf, if (hasbridge || def->bridge || def->macTableManager) { virBufferAddLit(buf, "<bridge"); virBufferEscapeString(buf, " name='%s'", def->bridge); + virBufferEscapeString(buf, " zone='%s'", def->bridgeZone); if (hasbridge) virBufferAsprintf(buf, " stp='%s' delay='%ld'", def->stp ? "on" : "off", def->delay); diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index c630674300..69ee8d7f2a 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -235,6 +235,7 @@ struct _virNetworkDef { int connections; /* # of guest interfaces connected to this network */ char *bridge; /* Name of bridge device */ + char *bridgeZone; /* name of firewalld zone for bridge (default "libvirt" */ int macTableManager; /* enum virNetworkBridgeMACTableManager */ char *domain; int domainLocalOnly; /* enum virTristateBool: yes disables dns forwarding */ diff --git a/src/network/bridge_driver_linux.c b/src/network/bridge_driver_linux.c index a32f19bcf0..8c585594d1 100644 --- a/src/network/bridge_driver_linux.c +++ b/src/network/bridge_driver_linux.c @@ -639,13 +639,29 @@ int networkAddFirewallRules(virNetworkDefPtr def) virFirewallPtr fw = NULL; int ret = -1; - - /* if firewalld is active, try to set the default "libvirt" zone, - * but ignore failure, since the version of firewalld on the host - * may have failed to load the libvirt zone - */ - if (virFirewallDStatus() >= 0) - ignore_value(virFirewallDInterfaceSetZone(def->bridge, "libvirt")); + if (!def->bridgeZone) { + /* if firewalld is active and no zone has been explicitly set + * in the config, try to set the default "libvirt" zone, but + * ignore failure, since the version of firewalld on the host + * may have failed to load the libvirt zone + */ + if (virFirewallDStatus() >= 0) + ignore_value(virFirewallDInterfaceSetZone(def->bridge, "libvirt")); + + } else { + /* if a zone has been specified, fail/log an error if we can't + * honor it + */ + if (virFirewallDStatus() < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("zone %s requested for network %s " + "but firewalld is not active"), + def->bridgeZone, def->name); + goto cleanup; + } + if (virFirewallDInterfaceSetZone(def->bridge, def->bridgeZone) < 0) + goto cleanup; + } fw = virFirewallNew(); diff --git a/tests/networkxml2xmlin/routed-network.xml b/tests/networkxml2xmlin/routed-network.xml index ab5e15b1f6..fce01df132 100644 --- a/tests/networkxml2xmlin/routed-network.xml +++ b/tests/networkxml2xmlin/routed-network.xml @@ -1,7 +1,7 @@ <network> <name>local</name> <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid> - <bridge name="virbr1"/> + <bridge name="virbr1" zone="myzone"/> <mac address='12:34:56:78:9A:BC'/> <forward mode="route" dev="eth1"/> <ip address="192.168.122.1" netmask="255.255.255.0"> diff --git a/tests/networkxml2xmlout/routed-network.xml b/tests/networkxml2xmlout/routed-network.xml index 81abf06e9f..2e13cf4ffa 100644 --- a/tests/networkxml2xmlout/routed-network.xml +++ b/tests/networkxml2xmlout/routed-network.xml @@ -4,7 +4,7 @@ <forward dev='eth1' mode='route'> <interface dev='eth1'/> </forward> - <bridge name='virbr1' stp='on' delay='0'/> + <bridge name='virbr1' zone='myzone' stp='on' delay='0'/> <mac address='12:34:56:78:9a:bc'/> <ip address='192.168.122.1' netmask='255.255.255.0'> </ip> -- 2.20.1

On 1/9/19 9:57 PM, Laine Stump wrote: The news.xml would be in it's own/last patch rather than included in with the others. Whether you split out the two entries into separate patches in the "correct order" is up to you. Having them in one patch at the end is also fine. I think the splitting just makes it easier to determine at a glance whether changes are at least described in the news.xml file. Perhaps makes the end of a release search through git history easier to weed out commit series that did adjust release notes and thus easier to find those that didn't and update appropriately. tools) or (e.g. no extra comma) 5.1.0 ...by default. The ... Really, "just" a bug fix. It's perhaps more a new feature since we're adding a new zone file and altering libvirt.spec.in... Certainly at least an improvement to have filters working properly w/ nftables. FWIW: Rather than 3 really, long sentences... available. So, nftables. However, due kernel's (or due to the way the kernel implements iptables support) host and (e.g. no need for the comma) (usually "public"). Instead virtual machine access will now be controlled by the new "libvirt" firewalld zone, unless the virtual network bridge is configured to use a specific zone file. NB: I don't think using <code></code> around zone and bridge is necessary here since it's used above when defining/describing the virtual network bridge change. Order wise, this last part certainly exhibits why this was added with the zone attribute adjustment above. nit: while "libvirt" is the de facto default, it's not placed into the XML so that on output "bridge='libvirt'" gets listed.... It's an implementation detail described in formatnetwork.html.in. Still if you keep the above comment close the ( zone '%s' network '%s', zone is for bridge for network right? e.g. shouldn't def->bridge be printed as well? or alternatively; otherwise, using example below it's "zone 'myzone' requested for network 'local', but firewalld is not active" John