[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 5/5] network: allow configuring firewalld zone for virtual network bridge device



On Wed, Jan 09, 2019 at 09:57:37PM -0500, 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)

Should this paragraph go below the --- ?

> 
> Signed-off-by: Laine Stump <laine 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(-)
> 
> 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>

Either this change belongs in the previous patch, or we should
put both changes in a patch #6 separate from the code.

>      </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
> 
> --
> libvir-list mailing list
> libvir-list redhat com
> https://www.redhat.com/mailman/listinfo/libvir-list

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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]