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