[libvirt] [firewalld PATCHv3] firewalld PATCH v3
* configura.ac, spec file: firewalld now defaults to enabled, depends on dbus * fixed comment for with_firewalld define * 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 * spec file changed as requested --- configure.ac | 17 ++++++++++++++++ libvirt.spec.in | 11 +++++++++++ src/Makefile.am | 4 ++-- src/network/bridge_driver.c | 47 +++++++++++++++++++++++++++++++++++++++++++++ src/util/ebtables.c | 35 +++++++++++++++++++++++++++++++++ src/util/iptables.c | 21 ++++++++++++++++++-- 6 files changed, 131 insertions(+), 4 deletions(-) diff --git a/configure.ac b/configure.ac index 8a04d91..c742d2f 100644 --- a/configure.ac +++ b/configure.ac @@ -1282,6 +1282,22 @@ 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 @<:@default=check@:>@]), + [], + [with_firewalld=check]) +if test "x$with_firewalld" = "xcheck" ; then + with_firewalld=$with_dbus +fi +if test "x$with_firewalld" == "xyes" ; then + if test "x$with_dbus" != "xyes" ; then + AC_MSG_ERROR([You must have dbus enabled for firewalld support]) + fi + AC_DEFINE_UNQUOTED([HAVE_FIREWALLD], [1], [whether firewalld support is enabled]) +fi +AM_CONDITIONAL([HAVE_FIREWALLD], [test "x$with_firewalld" != "xno"]) + dnl Avahi library AC_ARG_WITH([avahi], AC_HELP_STRING([--with-avahi], [use avahi to advertise remote daemon @<:@default=check@:>@]), @@ -2989,6 +3005,7 @@ AC_MSG_NOTICE([ sanlock: $SANLOCK_CFLAGS $SANLOCK_LIBS]) else AC_MSG_NOTICE([ sanlock: no]) fi +AC_MSG_NOTICE([firewalld: $with_firewalld]) if test "$with_avahi" = "yes" ; then AC_MSG_NOTICE([ avahi: $AVAHI_CFLAGS $AVAHI_LIBS]) else diff --git a/libvirt.spec.in b/libvirt.spec.in index 67b955a..ea2fd88 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -106,6 +106,7 @@ %define with_sanlock 0%{!?_without_sanlock:0} %define with_systemd 0%{!?_without_systemd:0} %define with_numad 0%{!?_without_numad:0} +%define with_firewalld 0%{!?_without_firewalld:0} # Non-server/HV driver defaults which are always enabled %define with_python 0%{!?_without_python:1} @@ -146,6 +147,11 @@ %define with_systemd 1 %endif +# Fedora 18 / RHEL-7 are first where firewalld support is enabled +%if 0%{?fedora} >= 17 || 0%{?rhel} >= 7 +%define with_firewalld 1 +%endif + # RHEL-5 has restricted QEMU to x86_64 only and is too old for LXC %if 0%{?rhel} == 5 %define with_qemu_tcg 0 @@ -1182,6 +1188,10 @@ of recent versions of Linux (and other OSes). %define _without_driver_modules --without-driver-modules %endif +%if %{with_firewalld} +%define _with_firewalld --with-firewalld +%endif + %define when %(date +"%%F-%%T") %define where %(hostname) %define who %{?packager}%{!?packager:Unknown} @@ -1240,6 +1250,7 @@ autoreconf -if %{?_without_audit} \ %{?_without_dtrace} \ %{?_without_driver_modules} \ + %{?_with_firewalld} \ %{with_packager} \ %{with_packager_version} \ --with-qemu-user=%{qemu_user} \ diff --git a/src/Makefile.am b/src/Makefile.am index 79b4e59..76570db 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -989,7 +989,7 @@ libvirt_driver_network_la_SOURCES = libvirt_driver_network_la_LIBADD = libvirt_driver_network_impl.la if WITH_DRIVER_MODULES mod_LTLIBRARIES += libvirt_driver_network.la -libvirt_driver_network_la_LIBADD += ../gnulib/lib/libgnu.la $(LIBNL_LIBS) +libvirt_driver_network_la_LIBADD += ../gnulib/lib/libgnu.la $(LIBNL_LIBS) $(DBUS_LIBS) libvirt_driver_network_la_LDFLAGS = -module -avoid-version $(AM_LDFLAGS) else noinst_LTLIBRARIES += libvirt_driver_network.la @@ -999,7 +999,7 @@ endif libvirt_driver_network_impl_la_CFLAGS = \ $(LIBNL_CFLAGS) \ - -I$(top_srcdir)/src/conf $(AM_CFLAGS) + -I$(top_srcdir)/src/conf $(AM_CFLAGS) $(DBUS_CFLAGS) libvirt_driver_network_impl_la_SOURCES = $(NETWORK_DRIVER_SOURCES) endif EXTRA_DIST += network/default.xml diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index a5046f1..39c0449 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -61,6 +61,7 @@ #include "virnetdev.h" #include "virnetdevbridge.h" #include "virnetdevtap.h" +#include "virdbus.h" #define NETWORK_PID_DIR LOCALSTATEDIR "/run/libvirt/network" #define NETWORK_STATE_DIR LOCALSTATEDIR "/lib/libvirt/network" @@ -248,6 +249,25 @@ 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 = user_data; + + 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: * @@ -256,6 +276,9 @@ networkAutostartConfigs(struct network_driver *driver) { static int networkStartup(int privileged) { char *base = NULL; +#ifdef HAVE_FIREWALLD + DBusConnection *sysbus = NULL; +#endif if (VIR_ALLOC(driverState) < 0) goto error; @@ -322,6 +345,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, driverState, NULL); + } +#endif + return 0; out_of_memory: diff --git a/src/util/ebtables.c b/src/util/ebtables.c index ca056b1..6f4d151 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,18 @@ 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; + if (!(argv[n++] = strdup("--direct"))) + goto error; + if (!(argv[n++] = strdup("--passthrough"))) + goto error; + if (!(argv[n++] = strdup("eb"))) + goto error; + } else +#endif if (!(argv[n++] = strdup(EBTABLES_PATH))) goto error; diff --git a/src/util/iptables.c b/src/util/iptables.c index b23aca9..1378575 100644 --- a/src/util/iptables.c +++ b/src/util/iptables.c @@ -101,9 +101,26 @@ iptablesAddRemoveRule(iptRules *rules, int family, int action, { va_list args; int ret; - virCommandPtr cmd; + virCommandPtr cmd = NULL; const char *s; - +#if HAVE_FIREWALLD + char *firewall_cmd_path = 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) { + 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); -- 1.7.11.2
On Tue, Aug 14, 2012 at 08:59:52PM +0200, Thomas Woerner wrote:
* configura.ac, spec file: firewalld now defaults to enabled, depends on
s/configura/configure/
dbus * fixed comment for with_firewalld define * 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 * spec file changed as requested --- configure.ac | 17 ++++++++++++++++ libvirt.spec.in | 11 +++++++++++ src/Makefile.am | 4 ++-- src/network/bridge_driver.c | 47 +++++++++++++++++++++++++++++++++++++++++++++ src/util/ebtables.c | 35 +++++++++++++++++++++++++++++++++ src/util/iptables.c | 21 ++++++++++++++++++-- 6 files changed, 131 insertions(+), 4 deletions(-)
ACK 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 :|
On 08/14/2012 12:59 PM, Thomas Woerner wrote:
* configura.ac, spec file: firewalld now defaults to enabled, depends on dbus * fixed comment for with_firewalld define * 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 * spec file changed as requested
In spite of Dan's ACK,
+++ 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");
this is rather inefficient - doing a PATH lookup for every call to ebtablesAddRemoveRule. Can we do the lookup just once and remember the result? -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 08/14/2012 02:59 PM, Thomas Woerner wrote:
* configura.ac, spec file: firewalld now defaults to enabled, depends on dbus * fixed comment for with_firewalld define * 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 * spec file changed as requested --- configure.ac | 17 ++++++++++++++++ libvirt.spec.in | 11 +++++++++++ src/Makefile.am | 4 ++-- src/network/bridge_driver.c | 47 +++++++++++++++++++++++++++++++++++++++++++++ src/util/ebtables.c | 35 +++++++++++++++++++++++++++++++++ src/util/iptables.c | 21 ++++++++++++++++++-- 6 files changed, 131 insertions(+), 4 deletions(-)
This isn't ready to push yet: The previous versions of this patch also made changes to the nwfilter driver, but this one doesn't. Did you forget to add those to the commit? This patch is still doing a search for the path of firewall-cmd every time ebtablesAddRemoveRule or iptablesAddRemoveRule is called. That's very wasteful. It should instead just do the search once when libvirtd starts. You should be able to do this with the VIR_ONCE_GLOBAL_INIT() macro and an associated xxxOnceInit() function. Also, I don't see any check for whether or not to use firewall-cmd other than whether or not the binary exists - if firewalld is disabled, will firewall-cmd still succeed? (by just calling iptables directly maybe?) If not, then we need to check if firewalld is enabled at libvirtd start time, and search for/populate (or not) the firewall-cmd path accordingly (this is needed both for util/(eb|ip)tables.c and for nwfilter/nwtilter_ebiptables_driver.c). Finally, in the commit log message, please put a description of what the patch does, rather than what changes have been made since the previous revision of the patch.
On 08/15/2012 11:24 AM, Laine Stump wrote:
On 08/14/2012 02:59 PM, Thomas Woerner wrote:
* configura.ac, spec file: firewalld now defaults to enabled, depends on dbus * fixed comment for with_firewalld define * 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 * spec file changed as requested --- configure.ac | 17 ++++++++++++++++ libvirt.spec.in | 11 +++++++++++ src/Makefile.am | 4 ++-- src/network/bridge_driver.c | 47 +++++++++++++++++++++++++++++++++++++++++++++ src/util/ebtables.c | 35 +++++++++++++++++++++++++++++++++ src/util/iptables.c | 21 ++++++++++++++++++-- 6 files changed, 131 insertions(+), 4 deletions(-) This isn't ready to push yet:
The previous versions of this patch also made changes to the nwfilter driver, but this one doesn't. Did you forget to add those to the commit?
This patch is still doing a search for the path of firewall-cmd every time ebtablesAddRemoveRule or iptablesAddRemoveRule is called. That's very wasteful. It should instead just do the search once when libvirtd starts. You should be able to do this with the VIR_ONCE_GLOBAL_INIT() macro and an associated xxxOnceInit() function.
Also, I don't see any check for whether or not to use firewall-cmd other than whether or not the binary exists - if firewalld is disabled, will firewall-cmd still succeed? (by just calling iptables directly maybe?) If not, then we need to check if firewalld is enabled at libvirtd start time, and search for/populate (or not) the firewall-cmd path accordingly (this is needed both for util/(eb|ip)tables.c and for nwfilter/nwtilter_ebiptables_driver.c).
Finally, in the commit log message, please put a description of what the patch does, rather than what changes have been made since the previous revision of the patch.
Also your name/email address needs to be added to AUTHORs, otherwise "make syntax-check" fails.
participants (4)
-
Daniel P. Berrange -
Eric Blake -
Laine Stump -
Thomas Woerner