network: avoid trying to create global firewall rules if unprivileged

The unprivileged libvirtd does not have permission to create firewall
rules, or bridge devices, or do anything to the host network in
general. Historically we still activate the network driver though and
let the network start API call fail.

The startup code path which reloads firewall rules on active networks
would thus effectively be a no-op when unprivileged as it is impossible
for there to be any active networks

With the change to use a global set of firewall chains, however, we now
have code that is run unconditionally.

Ideally we would not register the network driver at all when
unprivileged, but the entanglement with the virt drivers currently makes
that impractical. As a temporary hack, we just make the firewall reload
into a no-op.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
(cherry picked from commit 5d010c3df6)

m4/virt-storage-rbd.m4

@@ -33,6 +33,7 @@ AC_DEFUN([LIBVIRT_STORAGE_CHECK_RBD], [
       old_LIBS="$LIBS"
       LIBS="$LIBS $LIBRBD_LIBS"
       AC_CHECK_FUNCS([rbd_get_features],[],[LIBRBD_FOUND=no])
+      AC_CHECK_FUNCS([rbd_list2])
       LIBS="$old_LIBS"
     fi
 

src/network/bridge_driver.c

@@ -2108,8 +2108,11 @@ static void
 networkReloadFirewallRules(virNetworkDriverStatePtr driver, bool startup)
 {
     VIR_INFO("Reloading iptables rules");
-    if (networkPreReloadFirewallRules(startup) < 0)
+    /* Ideally we'd not even register the driver when unprivilegd
+     * but until we untangle the virt driver that's not viable */
+    if (!driver->privileged)
         return;
+    networkPreReloadFirewallRules(startup);
     virNetworkObjListForEach(driver->networks,
                              networkReloadFirewallRulesHelper,
                              NULL);

src/network/bridge_driver_linux.c

@@ -35,11 +35,37 @@ VIR_LOG_INIT("network.bridge_driver_linux");
 
 #define PROC_NET_ROUTE "/proc/net/route"
 
-int networkPreReloadFirewallRules(bool startup)
+static virErrorPtr errInitV4;
+static virErrorPtr errInitV6;
+
+void networkPreReloadFirewallRules(bool startup)
 {
-    int ret = iptablesSetupPrivateChains();
-    if (ret < 0)
-        return -1;
+    bool created = false;
+    int rc;
+
+    /* We create global rules upfront as we don't want
+     * the perf hit of conditionally figuring out whether
+     * to create them each time a network is started.
+     *
+     * Any errors here are saved to be reported at time
+     * of starting the network though as that makes them
+     * more likely to be seen by a human
+     */
+    rc = iptablesSetupPrivateChains(VIR_FIREWALL_LAYER_IPV4);
+    if (rc < 0) {
+        errInitV4 = virSaveLastError();
+        virResetLastError();
+    }
+    if (rc)
+        created = true;
+
+    rc = iptablesSetupPrivateChains(VIR_FIREWALL_LAYER_IPV6);
+    if (rc < 0) {
+        errInitV6 = virSaveLastError();
+        virResetLastError();
+    }
+    if (rc)
+        created = true;
 
     /*
      * If this is initial startup, and we just created the
@@ -54,10 +80,8 @@ int networkPreReloadFirewallRules(bool startup)
      * rules will be present. Thus we can safely just tell it
      * to always delete from the builin chain
      */
-    if (startup && ret == 1)
+    if (startup && created)
         iptablesSetDeletePrivate(false);
-
-    return 0;
 }
 
 
@@ -671,6 +695,21 @@ int networkAddFirewallRules(virNetworkDefPtr def)
     virFirewallPtr fw = NULL;
     int ret = -1;
 
+    if (errInitV4 &&
+        (virNetworkDefGetIPByIndex(def, AF_INET, 0) ||
+         virNetworkDefGetRouteByIndex(def, AF_INET, 0))) {
+        virSetError(errInitV4);
+        return -1;
+    }
+
+    if (errInitV6 &&
+        (virNetworkDefGetIPByIndex(def, AF_INET6, 0) ||
+         virNetworkDefGetRouteByIndex(def, AF_INET6, 0) ||
+         def->ipv6nogw)) {
+        virSetError(errInitV6);
+        return -1;
+    }
+
     if (def->bridgeZone) {
 
         /* if a firewalld zone has been specified, fail/log an error

src/network/bridge_driver_nop.c

@@ -19,9 +19,8 @@
 
 #include <config.h>
 
-int networkPreReloadFirewallRules(bool startup ATTRIBUTE_UNUSED)
+void networkPreReloadFirewallRules(bool startup ATTRIBUTE_UNUSED)
 {
-    return 0;
 }
 
 

src/network/bridge_driver_platform.h

@@ -58,7 +58,7 @@ struct _virNetworkDriverState {
 typedef struct _virNetworkDriverState virNetworkDriverState;
 typedef virNetworkDriverState *virNetworkDriverStatePtr;
 
-int networkPreReloadFirewallRules(bool startup);
+void networkPreReloadFirewallRules(bool startup);
 void networkPostReloadFirewallRules(bool startup);
 
 int networkCheckRouteCollision(virNetworkDefPtr def);

src/storage/storage_backend_rbd.c

@@ -565,19 +565,111 @@ volStorageBackendRBDRefreshVolInfo(virStorageVolDefPtr vol,
     return ret;
 }
 
+
+#ifdef HAVE_RBD_LIST2
+static char **
+virStorageBackendRBDGetVolNames(virStorageBackendRBDStatePtr ptr)
+{
+    char **names = NULL;
+    size_t nnames = 0;
+    int rc;
+    rbd_image_spec_t *images = NULL;
+    size_t nimages = 16;
+    size_t i;
+
+    while (true) {
+        if (VIR_ALLOC_N(images, nimages) < 0)
+            goto error;
+
+        rc = rbd_list2(ptr->ioctx, images, &nimages);
+        if (rc >= 0)
+            break;
+        if (rc != -ERANGE) {
+            virReportSystemError(-rc, "%s", _("Unable to list RBD images"));
+            goto error;
+        }
+    }
+
+    if (VIR_ALLOC_N(names, nimages + 1) < 0)
+        goto error;
+    nnames = nimages;
+
+    for (i = 0; i < nimages; i++)
+        VIR_STEAL_PTR(names[i], images->name);
+
+    return names;
+
+ error:
+    virStringListFreeCount(names, nnames);
+    rbd_image_spec_list_cleanup(images, nimages);
+    VIR_FREE(images);
+    return NULL;
+}
+
+#else /* ! HAVE_RBD_LIST2 */
+
+static char **
+virStorageBackendRBDGetVolNames(virStorageBackendRBDStatePtr ptr)
+{
+    char **names = NULL;
+    size_t nnames = 0;
+    int rc;
+    size_t max_size = 1024;
+    VIR_AUTOFREE(char *) namebuf = NULL;
+    const char *name;
+
+    while (true) {
+        if (VIR_ALLOC_N(namebuf, max_size) < 0)
+            goto error;
+
+        rc = rbd_list(ptr->ioctx, namebuf, &max_size);
+        if (rc >= 0)
+            break;
+        if (rc != -ERANGE) {
+            virReportSystemError(-rc, "%s", _("Unable to list RBD images"));
+            goto error;
+        }
+        VIR_FREE(namebuf);
+    }
+
+    for (name = namebuf; name < namebuf + max_size;) {
+        VIR_AUTOFREE(char *) namedup = NULL;
+
+        if (STREQ(name, ""))
+            break;
+
+        if (VIR_STRDUP(namedup, name) < 0)
+            goto error;
+
+        if (VIR_APPEND_ELEMENT(names, nnames, namedup) < 0)
+            goto error;
+
+        name += strlen(name) + 1;
+    }
+
+    if (VIR_EXPAND_N(names, nnames, 1) < 0)
+        goto error;
+
+    return names;
+
+ error:
+    virStringListFreeCount(names, nnames);
+    return NULL;
+}
+#endif /* ! HAVE_RBD_LIST2 */
+
+
 static int
 virStorageBackendRBDRefreshPool(virStoragePoolObjPtr pool)
 {
-    size_t max_size = 1024;
     int ret = -1;
-    int len = -1;
     int r = 0;
-    char *name;
     virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
     virStorageBackendRBDStatePtr ptr = NULL;
     struct rados_cluster_stat_t clusterstat;
     struct rados_pool_stat_t poolstat;
-    VIR_AUTOFREE(char *) names = NULL;
+    char **names = NULL;
+    size_t i;
 
     if (!(ptr = virStorageBackendRBDNewState(pool)))
         goto cleanup;
@@ -602,33 +694,16 @@ virStorageBackendRBDRefreshPool(virStoragePoolObjPtr pool)
               def->source.name, clusterstat.kb, clusterstat.kb_avail,
               poolstat.num_bytes);
 
-    while (true) {
-        if (VIR_ALLOC_N(names, max_size) < 0)
-            goto cleanup;
+    if (!(names = virStorageBackendRBDGetVolNames(ptr)))
+        goto cleanup;
 
-        len = rbd_list(ptr->ioctx, names, &max_size);
-        if (len >= 0)
-            break;
-        if (len != -ERANGE) {
-            VIR_WARN("%s", "A problem occurred while listing RBD images");
-            goto cleanup;
-        }
-        VIR_FREE(names);
-    }
-
-    for (name = names; name < names + max_size;) {
+    for (i = 0; names[i] != NULL; i++) {
         VIR_AUTOPTR(virStorageVolDef) vol = NULL;
 
-        if (STREQ(name, ""))
-            break;
-
         if (VIR_ALLOC(vol) < 0)
             goto cleanup;
 
-        if (VIR_STRDUP(vol->name, name) < 0)
-            goto cleanup;
-
-        name += strlen(name) + 1;
+        VIR_STEAL_PTR(vol->name, names[i]);
 
         r = volStorageBackendRBDRefreshVolInfo(vol, pool, ptr);
 
@@ -661,6 +736,7 @@ virStorageBackendRBDRefreshPool(virStoragePoolObjPtr pool)
     ret = 0;
 
  cleanup:
+    virStringListFree(names);
     virStorageBackendRBDFreeState(&ptr);
     return ret;
 }

src/util/viriptables.c

@@ -127,7 +127,7 @@ iptablesPrivateChainCreate(virFirewallPtr fw,
 
 
 int
-iptablesSetupPrivateChains(void)
+iptablesSetupPrivateChains(virFirewallLayer layer)
 {
     virFirewallPtr fw = NULL;
     int ret = -1;
@@ -143,17 +143,11 @@ iptablesSetupPrivateChains(void)
     };
     bool changed = false;
     iptablesGlobalChainData data[] = {
-        { VIR_FIREWALL_LAYER_IPV4, "filter",
+        { layer, "filter",
           filter_chains, ARRAY_CARDINALITY(filter_chains), &changed },
-        { VIR_FIREWALL_LAYER_IPV4, "nat",
+        { layer, "nat",
           natmangle_chains, ARRAY_CARDINALITY(natmangle_chains), &changed },
-        { VIR_FIREWALL_LAYER_IPV4, "mangle",
-          natmangle_chains, ARRAY_CARDINALITY(natmangle_chains), &changed },
-        { VIR_FIREWALL_LAYER_IPV6, "filter",
-          filter_chains, ARRAY_CARDINALITY(filter_chains), &changed },
-        { VIR_FIREWALL_LAYER_IPV6, "nat",
-          natmangle_chains, ARRAY_CARDINALITY(natmangle_chains), &changed },
-        { VIR_FIREWALL_LAYER_IPV6, "mangle",
+        { layer, "mangle",
           natmangle_chains, ARRAY_CARDINALITY(natmangle_chains), &changed },
     };
     size_t i;

src/util/viriptables.h

@@ -24,7 +24,7 @@
 # include "virsocketaddr.h"
 # include "virfirewall.h"
 
-int              iptablesSetupPrivateChains      (void);
+int              iptablesSetupPrivateChains      (virFirewallLayer layer);
 
 void             iptablesSetDeletePrivate        (bool pvt);