[libvirt] [PATCH 5/5] network: allow configuring firewalld zone for virtual network bridge device
John Ferlan
jferlan at redhat.com
Tue Jan 15 18:00:50 UTC 2019
On 1/9/19 9:57 PM, Laine Stump wrote:
> 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 at laine.org>
> ---
> 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(-)
>
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.
> 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
tools) or (e.g. no extra comma)
> + will also be managed using firewalld tools).
> + <span class="since">Since 5.0.0</span>
5.1.0
> + </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
...by default. 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">
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...
> + <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
available. So,
> + for its own packet filtering rules even when the firewalld
> + backend is set to use nftables, but due to the way iptables
nftables. However, due
> + support is implemented in kernels using nftables (iptables
kernel's (or due to the way the kernel implements iptables support)
> + 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 and (e.g. no need for the comma)
> + 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
(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.
> + firewalld zone called "libvirt" (unless configured otherwise
> + using the new <code>zone</code> attribute of the network
> + <code>bridge</code> element).
> + </description>
> + </change>
Order wise, this last part certainly exhibits why this was added with
the zone attribute adjustment above.
> </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" */
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 (
> 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 "
zone '%s'
network '%s',
> + "but firewalld is not active"),
> + def->bridgeZone, def->name);
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
> + 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>
>
More information about the libvir-list
mailing list