[libvirt] [PATCH V2 2/2] nwfilter: provide basic support for firewalld
Laine Stump
laine at laine.org
Fri Aug 10 06:03:21 UTC 2012
On 08/08/2012 12:00 PM, Stefan Berger wrote:
> This patch provides basic support for using firewalld's firewall-cmd
> rather than then plain eb/ip(6)tables commands.
>
> ---
> src/Makefile.am | 4
> src/conf/nwfilter_conf.h | 1
> src/libvirt_private.syms | 4
> src/nwfilter/nwfilter_driver.c | 172 ++++++++++++++++++++++++++++--
> src/nwfilter/nwfilter_driver.h | 2
> src/nwfilter/nwfilter_ebiptables_driver.c | 144 +++++++++++++++++++++----
> 6 files changed, 292 insertions(+), 35 deletions(-)
>
> Index: libvirt-firewalld/src/Makefile.am
> ===================================================================
> --- libvirt-firewalld.orig/src/Makefile.am
> +++ libvirt-firewalld/src/Makefile.am
> @@ -1149,9 +1149,9 @@ noinst_LTLIBRARIES += libvirt_driver_nwf
> #libvirt_la_BUILT_LIBADD += libvirt_driver_nwfilter.la
> endif
> libvirt_driver_nwfilter_la_CFLAGS = $(LIBPCAP_CFLAGS) \
> - -I$(top_srcdir)/src/conf $(LIBNL_CFLAGS) $(AM_CFLAGS)
> + -I$(top_srcdir)/src/conf $(LIBNL_CFLAGS) $(AM_CFLAGS) $(DBUS_CFLAGS)
> libvirt_driver_nwfilter_la_LDFLAGS = $(LD_AMFLAGS)
> -libvirt_driver_nwfilter_la_LIBADD = $(LIBPCAP_LIBS) $(LIBNL_LIBS)
> +libvirt_driver_nwfilter_la_LIBADD = $(LIBPCAP_LIBS) $(LIBNL_LIBS) $(DBUS_LIBS)
> if WITH_DRIVER_MODULES
> libvirt_driver_nwfilter_la_LIBADD += ../gnulib/lib/libgnu.la
> libvirt_driver_nwfilter_la_LDFLAGS += -module -avoid-version
> Index: libvirt-firewalld/src/nwfilter/nwfilter_driver.c
> ===================================================================
> --- libvirt-firewalld.orig/src/nwfilter/nwfilter_driver.c
> +++ libvirt-firewalld/src/nwfilter/nwfilter_driver.c
> @@ -27,6 +27,9 @@
>
> #include <config.h>
>
> +#include "virdbus.h"
> +#include "logging.h"
> +
> #include "internal.h"
>
> #include "virterror_internal.h"
> @@ -45,10 +48,24 @@
>
> #define VIR_FROM_THIS VIR_FROM_NWFILTER
>
> +#define DBUS_RULE_FWD_NAMEOWNERCHANGED \
> + "type='signal'" \
> + ",interface='"DBUS_INTERFACE_DBUS"'" \
> + ",member='NameOwnerChanged'" \
> + ",arg0='org.fedoraproject.FirewallD1'"
> +
> +#define DBUS_RULE_FWD_RELOADED \
> + "type='signal'" \
> + ",interface='org.fedoraproject.FirewallD1'" \
> + ",member='Reloaded'"
> +
> +
> static virNWFilterDriverStatePtr driverState;
>
> static int nwfilterDriverShutdown(void);
>
> +static int nwfilterDriverReload(void);
> +
> static void nwfilterDriverLock(virNWFilterDriverStatePtr driver)
> {
> virMutexLock(&driver->lock);
> @@ -58,6 +75,89 @@ static void nwfilterDriverUnlock(virNWFi
> virMutexUnlock(&driver->lock);
> }
>
> +#if HAVE_FIREWALLD
Again, either firewalld needs to require WITH_DBUS, or this needs to be
conditional on both HAVE_FIREWALLD and HAVE_DBUS.
> +
> +static DBusHandlerResult
> +nwfilterFirewalldDBusFilter(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;
> +}
> +
> +static void
> +nwfilterDriverRemoveDBusMatches(void)
> +{
> + DBusConnection *sysbus;
> +
> + sysbus = virDBusGetSystemBus();
> + if (sysbus) {
> + dbus_bus_remove_match(sysbus,
> + DBUS_RULE_FWD_NAMEOWNERCHANGED,
> + NULL);
> + dbus_bus_remove_match(sysbus,
> + DBUS_RULE_FWD_RELOADED,
> + NULL);
> + dbus_connection_remove_filter(sysbus, nwfilterFirewalldDBusFilter, NULL);
> + }
> +}
> +
> +/**
> + * virNWFilterDriverInstallDBusMatches
> + *
> + * Startup DBus matches for monitoring the state of firewalld
> + */
> +static int
> +nwfilterDriverInstallDBusMatches(DBusConnection *sysbus)
> +{
> + int ret = 0;
> +
> + if (!sysbus) {
> + ret = -1;
> + } 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,
> + DBUS_RULE_FWD_NAMEOWNERCHANGED,
> + NULL);
> + dbus_bus_add_match(sysbus,
> + DBUS_RULE_FWD_RELOADED,
> + NULL);
> + if (!dbus_connection_add_filter(sysbus, nwfilterFirewalldDBusFilter,
> + NULL, NULL)) {
> + VIR_WARN(("Adding a filter to the DBus connection failed"));
> + nwfilterDriverRemoveDBusMatches();
> + ret = -1;
> + }
> + }
> +
> + return ret;
> +}
> +
> +#else /* HAVE_FIREWALLD */
> +
> +static void
> +nwfilterDriverRemoveDBusMatches(void)
> +{
> +}
> +
> +static int
> +nwfilterDriverInstallDBusMatches(DBusConnection *sysbus ATTRIBUTE_UNUSED)
> +{
> + return 0;
> +}
> +
> +#endif /* HAVE_FIREWALLD */
>
> /**
> * virNWFilterStartup:
> @@ -65,14 +165,24 @@ static void nwfilterDriverUnlock(virNWFi
> * Initialization function for the QEmu daemon
> */
> static int
> -nwfilterDriverStartup(int privileged) {
> +nwfilterDriverStartup(int privileged)
> +{
> char *base = NULL;
> + DBusConnection *sysbus = virDBusGetSystemBus();
> +
> + if (VIR_ALLOC(driverState) < 0)
> + goto alloc_err_exit;
> +
> + if (virMutexInit(&driverState->lock) < 0)
> + goto err_free_driverstate;
> +
> + driverState->watchingFirewallD = (sysbus != NULL);
>
> if (!privileged)
> return 0;
>
> if (virNWFilterIPAddrMapInit() < 0)
> - return -1;
> + goto err_free_driverstate;
> if (virNWFilterLearnInit() < 0)
> goto err_exit_ipaddrmapshutdown;
> if (virNWFilterDHCPSnoopInit() < 0)
> @@ -81,16 +191,26 @@ nwfilterDriverStartup(int privileged) {
> virNWFilterTechDriversInit(privileged);
>
> if (virNWFilterConfLayerInit(virNWFilterDomainFWUpdateCB) < 0)
> - goto conf_init_err;
> -
> - if (VIR_ALLOC(driverState) < 0)
> - goto alloc_err_exit;
> -
> - if (virMutexInit(&driverState->lock) < 0)
> - goto alloc_err_exit;
> + goto err_techdrivers_shutdown;
>
> nwfilterDriverLock(driverState);
>
> + /*
> + * startup the DBus late so we don't get a reload signal while
> + * initializing
> + */
> + if (nwfilterDriverInstallDBusMatches(sysbus) < 0) {
> + VIR_ERROR(_("DBus matches could not be installed. Disabling nwfilter "
> + "driver"));
> + /*
> + * unfortunately this is fatal since virNWFilterTechDriversInit
> + * may have caused the ebiptables driver to use the firewall tool
> + * but now that the watches don't work, we just disable the nwfilter
> + * driver
> + */
> + goto error;
> + }
> +
> if (privileged) {
> if ((base = strdup (SYSCONFDIR "/libvirt")) == NULL)
> goto out_of_memory;
> @@ -124,9 +244,11 @@ error:
> nwfilterDriverShutdown();
>
> alloc_err_exit:
> - virNWFilterConfLayerShutdown();
There's a lot of reorganization here that's not really related to adding
firewalld support, but seems more of a general refactoring. Would it be
possible to split that out in to a separate prerequisite patch so it's
easier to see exactly what's going in for the purpose of supporting
firewalld (and also to make it possible to test the refactoring separate
from adding firewalld support, if it ever comes to that)?
> + return -1;
>
> -conf_init_err:
> + nwfilterDriverUnlock(driverState);
> +
> +err_techdrivers_shutdown:
> virNWFilterTechDriversShutdown();
> virNWFilterDHCPSnoopShutdown();
> err_exit_learnshutdown:
> @@ -134,6 +256,9 @@ err_exit_learnshutdown:
> err_exit_ipaddrmapshutdown:
> virNWFilterIPAddrMapShutdown();
>
> +err_free_driverstate:
> + VIR_FREE(driverState);
> +
> return -1;
> }
>
> @@ -192,6 +317,29 @@ nwfilterDriverActive(void) {
>
> nwfilterDriverLock(driverState);
> ret = driverState->nwfilters.count ? 1 : 0;
> + ret |= driverState->watchingFirewallD;
> + nwfilterDriverUnlock(driverState);
> +
> + return ret;
> +}
> +
> +/**
> + * virNWFilterIsWatchingFirewallD:
> + *
> + * Checks if the nwfilter has the DBus watches for FirewallD installed.
> + *
> + * Returns true if it is watching firewalld, false otherwise
> + */
> +bool
> +virNWFilterDriverIsWatchingFirewallD(void)
> +{
> + bool ret;
> +
> + if (!driverState)
> + return false;
> +
> + nwfilterDriverLock(driverState);
> + ret = driverState->watchingFirewallD;
> nwfilterDriverUnlock(driverState);
>
> return ret;
> @@ -215,6 +363,8 @@ nwfilterDriverShutdown(void) {
>
> nwfilterDriverLock(driverState);
>
> + nwfilterDriverRemoveDBusMatches();
> +
> /* free inactive nwfilters */
> virNWFilterObjListFree(&driverState->nwfilters);
>
> Index: libvirt-firewalld/src/nwfilter/nwfilter_ebiptables_driver.c
> ===================================================================
> --- libvirt-firewalld.orig/src/nwfilter/nwfilter_ebiptables_driver.c
> +++ libvirt-firewalld/src/nwfilter/nwfilter_ebiptables_driver.c
> @@ -36,6 +36,7 @@
> #include "virterror_internal.h"
> #include "domain_conf.h"
> #include "nwfilter_conf.h"
> +#include "nwfilter_driver.h"
> #include "nwfilter_gentech_driver.h"
> #include "nwfilter_ebiptables_driver.h"
> #include "virfile.h"
> @@ -145,11 +146,11 @@ static const char ebiptables_script_set_
> #define NWFILTER_FUNC_SET_IFS ebiptables_script_set_ifs
>
> #define NWFILTER_SET_EBTABLES_SHELLVAR(BUFPTR) \
> - virBufferAsprintf(BUFPTR, "EBT=%s\n", ebtables_cmd_path);
> + virBufferAsprintf(BUFPTR, "EBT=\"%s\"\n", ebtables_cmd_path);
> #define NWFILTER_SET_IPTABLES_SHELLVAR(BUFPTR) \
> - virBufferAsprintf(BUFPTR, "IPT=%s\n", iptables_cmd_path);
> + virBufferAsprintf(BUFPTR, "IPT=\"%s\"\n", iptables_cmd_path);
> #define NWFILTER_SET_IP6TABLES_SHELLVAR(BUFPTR) \
> - virBufferAsprintf(BUFPTR, "IPT=%s\n", ip6tables_cmd_path);
> + virBufferAsprintf(BUFPTR, "IPT=\"%s\"\n", ip6tables_cmd_path);
>
> #define VIRT_IN_CHAIN "libvirt-in"
> #define VIRT_OUT_CHAIN "libvirt-out"
> @@ -4121,23 +4122,101 @@ virNWFilterTechDriver ebiptables_driver
> .removeBasicRules = ebtablesRemoveBasicRules,
> };
>
> -
> +/*
> + * ebiptablesDriverInitWithFirewallD
> + *
> + * Try to use firewall-cmd by testing it once; if it works, have ebtables
> + * and ip6tables commands use firewall-cmd.
> + */
> static int
> -ebiptablesDriverInit(bool privileged)
> +ebiptablesDriverInitWithFirewallD(void)
> {
> virBuffer buf = VIR_BUFFER_INITIALIZER;
> - char *errmsg = NULL;
> + char *firewall_cmd_path;
> + char *output = NULL;
> + int ret = -1;
>
> - if (!privileged)
> - return 0;
> + if (!virNWFilterDriverIsWatchingFirewallD())
> + return -1;
>
> - if (virMutexInit(&execCLIMutex) < 0)
> - return -EINVAL;
> + firewall_cmd_path = virFindFileInPath("firewall-cmd");
>
> - gawk_cmd_path = virFindFileInPath("gawk");
> - grep_cmd_path = virFindFileInPath("grep");
> + if (firewall_cmd_path) {
> + virBufferAsprintf(&buf, "FWC=%s\n", firewall_cmd_path);
> + virBufferAsprintf(&buf,
> + CMD_DEF("$FWC --state") CMD_SEPARATOR
I'm curious why you need to put the path to the command into a shell
variable, and then reference the shell variable instead of just directly
putting the path into the commandline.
> + CMD_EXEC
> + "%s",
> + CMD_STOPONERR(1));
> +
> + if (ebiptablesExecCLI(&buf, NULL, &output) == 0 &&
> + strlen(output) == 0) {
> + VIR_DEBUG("Using firewall-cmd in nwfilter_ebiptables_driver.");
> + ebtables_cmd_path = NULL;
> + iptables_cmd_path = NULL;
> + ip6tables_cmd_path = NULL;
It's not necessary to set these to NULL before calling virAsprintf - it
ignores the initial value, and sets it to NULL on failure.
> +
> + ignore_value(virAsprintf(&ebtables_cmd_path,
> + "%s --direct --passthrough eb",
> + firewall_cmd_path));
> + ignore_value(virAsprintf(&iptables_cmd_path,
> + "%s --direct --passthrough ipv4",
> + firewall_cmd_path));
> + ignore_value(virAsprintf(&ip6tables_cmd_path,
> + "%s --direct --passthrough ipv6",
> + firewall_cmd_path));
> +
> + if (!ebtables_cmd_path || !iptables_cmd_path ||
> + !ip6tables_cmd_path) {
> + virReportOOMError();
> + VIR_FREE(ebtables_cmd_path);
> + VIR_FREE(iptables_cmd_path);
> + VIR_FREE(ip6tables_cmd_path);
> + ret = -1;
> + goto err_exit;
> + }
> + ret = 0;
> + }
> + }
> +
> +err_exit:
> + VIR_FREE(firewall_cmd_path);
> + VIR_FREE(output);
> +
> + return ret;
> +}
>
> +static int
> +ebiptablesDriverInitCLITools(void)
> +{
> ebtables_cmd_path = virFindFileInPath("ebtables");
> + if (!ebtables_cmd_path)
> + VIR_WARN("Could not find 'ebtables' executable");
> +
> + iptables_cmd_path = virFindFileInPath("iptables");
> + if (!iptables_cmd_path)
> + VIR_WARN("Could not find 'iptables' executable");
> +
> + ip6tables_cmd_path = virFindFileInPath("ip6tables");
> + if (!ip6tables_cmd_path)
> + VIR_WARN("Could not find 'ip6tables' executable");
> +
> + return 0;
> +}
> +
> +/*
> + * ebiptablesDriverTestCLITools
> + *
> + * Test the CLI tools. If one is found not to be working, free the buffer
> + * holding its path as a sign that the tool cannot be used.
> + */
> +static int
> +ebiptablesDriverTestCLITools(void)
> +{
> + virBuffer buf = VIR_BUFFER_INITIALIZER;
> + char *errmsg = NULL;
> + int ret = 0;
> +
> if (ebtables_cmd_path) {
> NWFILTER_SET_EBTABLES_SHELLVAR(&buf);
> /* basic probing */
> @@ -4151,12 +4230,10 @@ ebiptablesDriverInit(bool privileged)
> VIR_FREE(ebtables_cmd_path);
> VIR_ERROR(_("Testing of ebtables command failed: %s"),
> errmsg);
> + ret = -1;
> }
> - } else {
> - VIR_WARN("Could not find 'ebtables' executable");
> }
>
> - iptables_cmd_path = virFindFileInPath("iptables");
> if (iptables_cmd_path) {
> NWFILTER_SET_IPTABLES_SHELLVAR(&buf);
>
> @@ -4170,12 +4247,10 @@ ebiptablesDriverInit(bool privileged)
> VIR_FREE(iptables_cmd_path);
> VIR_ERROR(_("Testing of iptables command failed: %s"),
> errmsg);
> + ret = -1;
> }
> - } else {
> - VIR_WARN("Could not find 'iptables' executable");
> }
>
> - ip6tables_cmd_path = virFindFileInPath("ip6tables");
> if (ip6tables_cmd_path) {
> NWFILTER_SET_IP6TABLES_SHELLVAR(&buf);
>
> @@ -4189,11 +4264,38 @@ ebiptablesDriverInit(bool privileged)
> VIR_FREE(ip6tables_cmd_path);
> VIR_ERROR(_("Testing of ip6tables command failed: %s"),
> errmsg);
> + ret = -1;
> }
> - } else {
> - VIR_WARN("Could not find 'ip6tables' executable");
> }
>
> + VIR_FREE(errmsg);
> +
> + return ret;
> +}
> +
> +static int
> +ebiptablesDriverInit(bool privileged)
> +{
> + if (!privileged)
> + return 0;
> +
> + if (virMutexInit(&execCLIMutex) < 0)
> + return -EINVAL;
> +
> + gawk_cmd_path = virFindFileInPath("gawk");
> + grep_cmd_path = virFindFileInPath("grep");
> +
> + /*
> + * check whether we can run with firewalld's tools --
> + * if not, we just fall back to eb/iptables command
> + * line tools.
> + */
> + if (ebiptablesDriverInitWithFirewallD() < 0)
> + ebiptablesDriverInitCLITools();
> +
> + /* make sure tools are available and work */
> + ebiptablesDriverTestCLITools();
> +
> /* ip(6)tables support needs gawk & grep, ebtables doesn't */
> if ((iptables_cmd_path != NULL || ip6tables_cmd_path != NULL) &&
> (!grep_cmd_path || !gawk_cmd_path)) {
> @@ -4203,8 +4305,6 @@ ebiptablesDriverInit(bool privileged)
> VIR_FREE(ip6tables_cmd_path);
> }
>
> - VIR_FREE(errmsg);
> -
> if (!ebtables_cmd_path && !iptables_cmd_path && !ip6tables_cmd_path) {
> VIR_ERROR(_("firewall tools were not found or cannot be used"));
> ebiptablesDriverShutdown();
Just a note again at the bgottom of this file - it looks like there's a
lot of general refactoring here, and that's obscuring what is being done
specifically to support using firewalld's commandline rather than
iptables/ebtables.
> Index: libvirt-firewalld/src/conf/nwfilter_conf.h
> ===================================================================
> --- libvirt-firewalld.orig/src/conf/nwfilter_conf.h
> +++ libvirt-firewalld/src/conf/nwfilter_conf.h
> @@ -563,6 +563,7 @@ struct _virNWFilterDriverState {
> virNWFilterObjList nwfilters;
>
> char *configDir;
> + bool watchingFirewallD;
> };
>
>
> Index: libvirt-firewalld/src/libvirt_private.syms
> ===================================================================
> --- libvirt-firewalld.orig/src/libvirt_private.syms
> +++ libvirt-firewalld/src/libvirt_private.syms
> @@ -891,6 +891,10 @@ virNWFilterIPAddrMapInit;
> virNWFilterIPAddrMapShutdown;
>
>
> +# nwfilter_driver.h
> +virNWFilterDriverIsWatchingFirewallD;
> +
> +
> # nwfilter_params.h
> virNWFilterHashTableCreate;
> virNWFilterHashTableFree;
> Index: libvirt-firewalld/src/nwfilter/nwfilter_driver.h
> ===================================================================
> --- libvirt-firewalld.orig/src/nwfilter/nwfilter_driver.h
> +++ libvirt-firewalld/src/nwfilter/nwfilter_driver.h
> @@ -33,4 +33,6 @@
>
> int nwfilterRegister(void);
>
> +bool virNWFilterDriverIsWatchingFirewallD(void);
> +
> #endif /* __VIR_NWFILTER_DRIVER_H__ */
>
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>
More information about the libvir-list
mailing list