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

Re: [libvirt] [PATCH] Add support for firewalld



On Mon, Apr 23, 2012 at 11:11:51PM +0200, Thomas Woerner wrote:
> Add support for firewalld
> 
> * bridge_driver, nwfilter_driver: new dbus filters to get FirewallD1.Reloaded
>   signal and DBus.NameOwnerChanged on org.fedoraproject.FirewallD1
> * iptables, ebtables, nwfilter_ebiptables_driver: use firewall-cmd direct
>   passthrough interface
> ---
>  configure.ac                              |    9 ++++++
>  src/Makefile.am                           |    8 ++---
>  src/network/bridge_driver.c               |   50 +++++++++++++++++++++++++++++
>  src/nwfilter/nwfilter_driver.c            |   49 ++++++++++++++++++++++++++++
>  src/nwfilter/nwfilter_ebiptables_driver.c |   27 ++++++++++++++++
>  src/util/ebtables.c                       |   32 ++++++++++++++++++
>  src/util/iptables.c                       |   25 ++++++++++++---
>  7 files changed, 192 insertions(+), 8 deletions(-)
> 
> diff --git a/configure.ac b/configure.ac
> index 89fe818..41d9371 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -1191,6 +1191,15 @@ AM_CONDITIONAL([HAVE_POLKIT1], [test "x$with_polkit1" = "xyes"])
>  AC_SUBST([POLKIT_CFLAGS])
>  AC_SUBST([POLKIT_LIBS])
>  
> +dnl firewalld
> +AC_ARG_WITH([firewalld],
> +  AC_HELP_STRING([--with-firewalld], [enable firewalld support]))
> +if test "x$with_firewalld" = "xyes" ; then
> +   AC_DEFINE_UNQUOTED([HAVE_FIREWALLD], [1], [whether firewalld support is enabled])
> +fi
> +AM_CONDITIONAL([HAVE_FIREWALLD], [test "x$with_firewalld" = "xyes"])
> +
> +
>  dnl Avahi library
>  AC_ARG_WITH([avahi],
>    AC_HELP_STRING([--with-avahi], [use avahi to advertise remote daemon @<:@default=check@:>@]),

To go along with this, you should also modify the libvirt.spec.in file in
GIT, to add logic to turn on firewalld on Fedora >= 17 + RHEL >= 7 only,
forcably disabling elsewhere.

> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index d82212f..094bbae 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -63,6 +63,11 @@
>  #include "virnetdevbridge.h"
>  #include "virnetdevtap.h"
>  
> +#if HAVE_FIREWALLD
> +#include "virdbus.h"
> +#include "logging.h"
> +#endif

While technically correct, this is overkill - just let them be
included unconditionally.

> +
>  #define NETWORK_PID_DIR LOCALSTATEDIR "/run/libvirt/network"
>  #define NETWORK_STATE_DIR LOCALSTATEDIR "/lib/libvirt/network"
>  
> @@ -253,6 +258,24 @@ networkAutostartConfigs(struct network_driver *driver) {
>      }
>  }
>  
> +#if HAVE_FIREWALLD
> +static DBusHandlerResult
> +firewalld_dbus_filter_bridge(DBusConnection *connection ATTRIBUTE_UNUSED, DBusMessage *message, void *user_data) {
> +    struct network_driver *_driverState = (struct network_driver *) user_data;

This cast is not required, since void * auto-casts to anything in C.

> +
> +    if (dbus_message_is_signal(message, DBUS_INTERFACE_DBUS,
> +                               "NameOwnerChanged") ||
> +        dbus_message_is_signal(message, "org.fedoraproject.FirewallD1",
> +                               "Reloaded"))
> +    {
> +        VIR_DEBUG("Reload in bridge_driver because of firewalld.");
> +        networkReloadIptablesRules(_driverState);
> +    }
> +
> +    return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
> +}
> +#endif
> +
>  /**
>   * networkStartup:
>   *
> @@ -262,6 +285,9 @@ static int
>  networkStartup(int privileged) {
>      uid_t uid = geteuid();
>      char *base = NULL;
> +#ifdef HAVE_FIREWALLD
> +    DBusConnection *sysbus = NULL;
> +#endif
>  
>      if (VIR_ALLOC(driverState) < 0)
>          goto error;
> @@ -326,6 +352,30 @@ networkStartup(int privileged) {
>  
>      networkDriverUnlock(driverState);
>  
> +#ifdef HAVE_FIREWALLD
> +    if (!(sysbus = virDBusGetSystemBus())) {
> +        virErrorPtr err = virGetLastError();
> +        VIR_WARN("DBus not available, disabling firewalld support in bridge_driver: %s", err->message);
> +    } else {
> +        /* add matches for
> +         * NameOwnerChanged on org.freedesktop.DBus for firewalld start/stop
> +         * Reloaded on org.fedoraproject.FirewallD1 for firewalld reload
> +         */
> +        dbus_bus_add_match(sysbus,
> +                           "type='signal'"
> +                           ",interface='"DBUS_INTERFACE_DBUS"'"
> +                           ",member='NameOwnerChanged'"
> +                           ",arg0='org.fedoraproject.FirewallD1'",
> +                           NULL);
> +        dbus_bus_add_match(sysbus,
> +                           "type='signal'"
> +                           ",interface='org.fedoraproject.FirewallD1'"
> +                           ",member='Reloaded'",
> +                           NULL);
> +        dbus_connection_add_filter(sysbus, firewalld_dbus_filter_bridge, (void *)driverState, NULL);

No need to cast to void * here either.

> +    }
> +#endif
> +
>      return 0;
>  
>  out_of_memory:
> diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c
> index ffb4b5d..da9f4a0 100644
> --- a/src/nwfilter/nwfilter_driver.c
> +++ b/src/nwfilter/nwfilter_driver.c
> @@ -27,6 +27,11 @@
>  
>  #include <config.h>
>  
> +#if HAVE_FIREWALLD
> +#include "virdbus.h"
> +#include "logging.h"
> +#endif

As before can make this unconditional

> +
>  #include "internal.h"
>  
>  #include "virterror_internal.h"
> @@ -47,6 +52,8 @@ static virNWFilterDriverStatePtr driverState;
>  
>  static int nwfilterDriverShutdown(void);
>  
> +static int nwfilterDriverReload(void);
> +
>  static void nwfilterDriverLock(virNWFilterDriverStatePtr driver)
>  {
>      virMutexLock(&driver->lock);
> @@ -57,6 +64,22 @@ static void nwfilterDriverUnlock(virNWFilterDriverStatePtr driver)
>  }
>  
>  
> +#if HAVE_FIREWALLD
> +static DBusHandlerResult
> +firewalld_dbus_filter_nwfilter(DBusConnection *connection ATTRIBUTE_UNUSED, DBusMessage *message, void *user_data ATTRIBUTE_UNUSED) {
> +    if (dbus_message_is_signal(message, DBUS_INTERFACE_DBUS,
> +                               "NameOwnerChanged") ||
> +        dbus_message_is_signal(message, "org.fedoraproject.FirewallD1",
> +                               "Reloaded"))
> +    {
> +        VIR_DEBUG("Reload in nwfilter_driver because of firewalld.");
> +        nwfilterDriverReload();
> +    }
> +
> +    return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
> +}
> +#endif
> +
>  /**
>   * virNWFilterStartup:
>   *
> @@ -66,6 +89,32 @@ static int
>  nwfilterDriverStartup(int privileged) {
>      char *base = NULL;
>  
> +#ifdef HAVE_FIREWALLD
> +    DBusConnection *sysbus = NULL;
> +
> +    if (!(sysbus = virDBusGetSystemBus())) {
> +        virErrorPtr err = virGetLastError();
> +        VIR_WARN("DBus not available, disabling firewalld support in nwfilter_driver: %s", err->message);
> +    } else {
> +        /* add matches for
> +         * NameOwnerChanged on org.freedesktop.DBus for firewalld start/stop
> +         * Reloaded on org.fedoraproject.FirewallD1 for firewalld reload
> +         */
> +        dbus_bus_add_match(sysbus,
> +                           "type='signal'"
> +                           ",interface='"DBUS_INTERFACE_DBUS"'"
> +                           ",member='NameOwnerChanged'"
> +                           ",arg0='org.fedoraproject.FirewallD1'",
> +                           NULL);
> +        dbus_bus_add_match(sysbus,
> +                           "type='signal'"
> +                           ",interface='org.fedoraproject.FirewallD1'"
> +                           ",member='Reloaded'",
> +                           NULL);
> +        dbus_connection_add_filter(sysbus, firewalld_dbus_filter_nwfilter, NULL, NULL);
> +    }
> +#endif
> +
>      if (virNWFilterLearnInit() < 0)
>          return -1;
>  
> diff --git a/src/nwfilter/nwfilter_ebiptables_driver.c b/src/nwfilter/nwfilter_ebiptables_driver.c
> index 8e4436f..0cd047a 100644
> --- a/src/nwfilter/nwfilter_ebiptables_driver.c
> +++ b/src/nwfilter/nwfilter_ebiptables_driver.c
> @@ -4051,6 +4051,7 @@ ebiptablesDriverInit(bool privileged)
>  {
>      virBuffer buf = VIR_BUFFER_INITIALIZER;
>      char *errmsg = NULL;
> +    char *firewall_cmd_path = NULL;
>  
>      if (!privileged)
>          return 0;
> @@ -4061,6 +4062,30 @@ ebiptablesDriverInit(bool privileged)
>      gawk_cmd_path = virFindFileInPath("gawk");
>      grep_cmd_path = virFindFileInPath("grep");
>  
> +    firewall_cmd_path = virFindFileInPath("firewall-cmd");
> +    if (firewall_cmd_path) {
> +        virBufferAsprintf(&buf, "IPT=%s\n", firewall_cmd_path);
> +        /* basic probing */
> +        virBufferAsprintf(&buf,
> +                          CMD_DEF("$IPT --state") CMD_SEPARATOR
> +                          CMD_EXEC
> +                          "%s",
> +                          CMD_STOPONERR(1));
> +
> +        if (ebiptablesExecCLI(&buf, NULL, NULL) >= 0) {
> +            VIR_DEBUG("Using firewall-cmd in nwfilter_ebiptables_driver.");
> +            ebtables_cmd_path = NULL;
> +            iptables_cmd_path = NULL;
> +            ip6tables_cmd_path = NULL;
> +            virAsprintf(&ebtables_cmd_path, "%s --direct --passthrough eb", firewall_cmd_path);
> +            virAsprintf(&iptables_cmd_path, "%s --direct --passthrough ipv4", firewall_cmd_path);
> +            virAsprintf(&ip6tables_cmd_path, "%s --direct --passthrough ipv6", firewall_cmd_path);

Need to check for & report OOM in virAsprintf.  I'm surprised we haven't
actually annotated it with ATTRIBUTE_RETURN_CHECK to catch this

> +        }
> +        VIR_FREE(firewall_cmd_path);
> +    }
> +    if (ebtables_cmd_path == NULL || iptables_cmd_path == NULL ||
> +        ip6tables_cmd_path == NULL) {
> +
>      ebtables_cmd_path = virFindFileInPath("ebtables");
>      if (ebtables_cmd_path) {
>          NWFILTER_SET_EBTABLES_SHELLVAR(&buf);
> @@ -4099,6 +4124,8 @@ ebiptablesDriverInit(bool privileged)
>          VIR_WARN("Could not find 'iptables' executable");
>      }
>  
> +    }
> +
>      ip6tables_cmd_path = virFindFileInPath("ip6tables");
>      if (ip6tables_cmd_path) {
>          NWFILTER_SET_IP6TABLES_SHELLVAR(&buf);
> diff --git a/src/util/ebtables.c b/src/util/ebtables.c
> index dcb3eb9..b7773ea 100644
> --- a/src/util/ebtables.c
> +++ b/src/util/ebtables.c
> @@ -176,11 +176,34 @@ ebtablesAddRemoveRule(ebtRules *rules, int action, const char *arg, ...)
>      const char *s;
>      int n, command_idx;
>  
> +#if HAVE_FIREWALLD
> +    int ret;
> +    char *firewall_cmd_path = NULL;
> +    virCommandPtr cmd = NULL;
> +
> +    firewall_cmd_path = virFindFileInPath("firewall-cmd");
> +    if (firewall_cmd_path) {
> +        cmd = virCommandNew(firewall_cmd_path);
> +        virCommandAddArgList(cmd, "--state", NULL);
> +        ret = virCommandRun(cmd, NULL);
> +        if (ret != 0) {
> +            VIR_FREE(firewall_cmd_path);
> +            firewall_cmd_path = NULL;
> +        }
> +        virCommandFree(cmd);
> +    }
> +#endif
> +
>      n = 1 + /* /sbin/ebtables  */
>          2 + /*   --table foo   */
>          2 + /*   --insert bar  */
>          1;  /*   arg           */
>  
> +#if HAVE_FIREWALLD
> +    if (firewall_cmd_path)
> +        n += 3; /* --direct --passthrough eb */
> +#endif
> +
>      va_start(args, arg);
>      while (va_arg(args, const char *))
>          n++;
> @@ -192,6 +215,15 @@ ebtablesAddRemoveRule(ebtRules *rules, int action, const char *arg, ...)
>  
>      n = 0;
>  
> +#if HAVE_FIREWALLD
> +    if (firewall_cmd_path) {
> +        if (!(argv[n++] = strdup(firewall_cmd_path)))
> +            goto error;
> +        argv[n++] = strdup("--direct");
> +        argv[n++] = strdup("--passthrough");
> +        argv[n++] = strdup("eb");

Need to check OOM on all 4 of thoses strdup()'s not just the first

> +    } else
> +#endif
>      if (!(argv[n++] = strdup(EBTABLES_PATH)))
>          goto error;

Not your fault, but this is a nice reminder to us that we need to convert
this code to virCommand.

>  
> diff --git a/src/util/iptables.c b/src/util/iptables.c
> index 3023900..0cb3293 100644
> --- a/src/util/iptables.c
> +++ b/src/util/iptables.c
> @@ -104,11 +104,28 @@ iptablesAddRemoveRule(iptRules *rules, int family, int action,
>  {
>      va_list args;
>      int ret;
> -    virCommandPtr cmd;
> +    virCommandPtr cmd = NULL;
>      const char *s;
> -
> -    cmd = virCommandNew((family == AF_INET6)
> -                        ? IP6TABLES_PATH : IPTABLES_PATH);
> +    char *firewall_cmd_path = NULL;
> +
> +#if HAVE_FIREWALLD
> +    firewall_cmd_path = virFindFileInPath("firewall-cmd");
> +    if (firewall_cmd_path) {
> +        cmd = virCommandNew(firewall_cmd_path);
> +        virCommandAddArgList(cmd, "--state", NULL);
> +        ret = virCommandRun(cmd, NULL);
> +        if (ret == 0) {
> +            cmd = virCommandNew(firewall_cmd_path);
> +            virCommandAddArgList(cmd, "--direct", "--passthrough",
> +                                 (family == AF_INET6) ? "ipv6" : "ipv4", NULL);
> +        } else
> +            cmd = NULL;
> +        VIR_FREE(firewall_cmd_path);
> +    }
> +    if (!cmd)
> +#endif
> +        cmd = virCommandNew((family == AF_INET6)
> +                            ? IP6TABLES_PATH : IPTABLES_PATH);
>  
>      virCommandAddArgList(cmd, "--table", rules->table,
>                           action == ADD ? "--insert" : "--delete",
> -- 



Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|


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