Re: [libvirt] [PATCH 7/7] Convert polkit code to use DBus API instead of CLI helper

Spawning the pkcheck program every time a permission check is required is hugely expensive on CPU. The pkcheck program is just a dumb wrapper for the DBus API, so rewrite the code to use the DBus API directly. This also simplifies error handling a bit. Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com&gt; --- cfg.mk | 3 + po/POTFILES.in | 1 + src/util/virpolkit.c | 122 ++++++++--------- tests/Makefile.am | 9 +- tests/virpolkittest.c | 360 ++++++++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 429 insertions(+), 66 deletions(-) create mode 100644 tests/virpolkittest.c diff --git a/cfg.mk b/cfg.mk index c7119e6..ed7123b 100644 --- a/cfg.mk +++ b/cfg.mk @@ -1143,3 +1143,6 @@ exclude_file_name_regexp--sc_prohibit_mixed_case_abbreviations = \ exclude_file_name_regexp--sc_prohibit_empty_first_line = \ ^(README|daemon/THREADS\.txt|src/esx/README|docs/library.xen|tests/vmwareverdata/fusion-5.0.3.txt|tests/nodeinfodata/linux-raspberrypi/cpu/offline)$$ + +exclude_file_name_regexp--sc_prohibit_useless_translation = \ + ^tests/virpolkittest.c diff --git a/po/POTFILES.in b/po/POTFILES.in index 1a0b75e..b769dfe 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -236,6 +236,7 @@ src/xenapi/xenapi_utils.c src/xenconfig/xen_common.c src/xenconfig/xen_sxpr.c src/xenconfig/xen_xm.c +tests/virpolkittest.c tools/libvirt-guests.sh.in tools/virsh.c tools/virsh.h diff --git a/src/util/virpolkit.c b/src/util/virpolkit.c index 620bfda..203bd6e 100644 --- a/src/util/virpolkit.c +++ b/src/util/virpolkit.c @@ -60,84 +60,76 @@ int virPolkitCheckAuth(const char *actionid, const char **details, bool allowInteraction) { - int status = -1; - bool authdismissed = 0; - bool supportsuid = 0; - char *pkout = NULL; - virCommandPtr cmd = NULL; + DBusConnection *sysbus; + DBusMessage *reply = NULL; + char **retdetails = NULL; + size_t nretdetails = 0; + int is_authorized; /* var-args requires int not bool */ + int is_challenge; /* var-args requires int not bool */ + bool is_dismissed = false; + size_t i; int ret = -1; - static bool polkitInsecureWarned = false; - - VIR_DEBUG("Checking PID %lld UID %d startTime %llu", - (long long)pid, (int)uid, startTime); - if (startTime == 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Start time is required for polkit auth")); - return -1; - } - - cmd = virCommandNewArgList(PKCHECK_PATH, "--action-id", actionid, NULL); - virCommandSetOutputBuffer(cmd, &pkout); - virCommandSetErrorBuffer(cmd, &pkout); - - virCommandAddArg(cmd, "--process"); -# ifdef PKCHECK_SUPPORTS_UID - supportsuid = 1; -# endif - if (supportsuid) { - virCommandAddArgFormat(cmd, "%lld,%llu,%lu", - (long long)pid, startTime, (unsigned long)uid); - } else { - if (!polkitInsecureWarned) { - VIR_WARN("No support for caller UID with pkcheck. This deployment is known to be insecure."); - polkitInsecureWarned = true; - } - virCommandAddArgFormat(cmd, "%lld,%llu", - (long long)pid, startTime); - } - if (allowInteraction) - virCommandAddArg(cmd, "--allow-user-interaction"); + if (!(sysbus = virDBusGetSystemBus())) + goto cleanup; - while (details && details[0] && details[1]) { - virCommandAddArgList(cmd, "--detail", details[0], details[1], NULL); - details += 2; - } + VIR_INFO("Checking PID %lld running as %d", + (long long) pid, uid); - if (virCommandRun(cmd, &status) < 0) + if (virDBusCallMethod(sysbus, + &reply, + NULL, + "org.freedesktop.PolicyKit1", + "/org/freedesktop/PolicyKit1/Authority", + "org.freedesktop.PolicyKit1.Authority", + "CheckAuthorization", + "(sa{sv})sa&{ss}us", + "unix-process", + 3, + "pid", "u", (unsigned int)pid, + "start-time", "t", startTime, + "uid", "i", (int)uid, + actionid, + virStringListLen(details) / 2, + details, + allowInteraction, + "" /* cancellation ID */) < 0) goto cleanup; - authdismissed = (pkout && strstr(pkout, "dismissed=true")); - if (status != 0) { - char *tmp = virProcessTranslateStatus(status); - VIR_DEBUG("Policy kit denied action %s from pid %lld, uid %d: %s", - actionid, (long long)pid, (int)uid, NULLSTR(tmp)); - VIR_FREE(tmp); - ret = -2; + if (virDBusMessageRead(reply, + "(bba&{ss})", + &is_authorized, + &is_challenge, + &nretdetails, + &retdetails) < 0) goto cleanup; - } - VIR_DEBUG("Policy allowed action %s from pid %lld, uid %d", - actionid, (long long)pid, (int)uid); + for (i = 0; i < (nretdetails / 2); i++) { + if (STREQ(retdetails[(i * 2)], "polkit.dismissed") && + STREQ(retdetails[(i * 2) + 1], "true")) + is_dismissed = true; + } - ret = 0; + VIR_DEBUG("is auth %d is challenge %d", + is_authorized, is_challenge); - cleanup: - if (ret < 0) { - virResetLastError(); - - if (authdismissed) { + if (is_authorized) { + ret = 0; + } else { + ret = -2; + if (is_dismissed) virReportError(VIR_ERR_AUTH_CANCELLED, "%s", - _("authentication cancelled by user")); - } else if (pkout && *pkout) { - virReportError(VIR_ERR_AUTH_FAILED, _("polkit: %s"), pkout); - } else { - virReportError(VIR_ERR_AUTH_FAILED, "%s", _("authentication failed")); - } + _("user cancelled authentication process")); + else if (is_challenge) + virReportError(VIR_ERR_AUTH_FAILED, "%s", + _("no agent is available to authenticate")); + else + virReportError(VIR_ERR_AUTH_FAILED, "%s", + _("access denied by policy")); } - virCommandFree(cmd); - VIR_FREE(pkout); + cleanup: + virStringFreeListCount(retdetails, nretdetails); return ret; } diff --git a/tests/Makefile.am b/tests/Makefile.am index d6c3cfb..9faa1c0 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -197,7 +197,9 @@ endif WITH_LIBVIRTD if WITH_DBUS test_programs += virdbustest \ - virsystemdtest + virpolkittest \ + virsystemdtest \ + $(NULL) endif WITH_DBUS if WITH_SECDRIVER_SELINUX @@ -1008,6 +1010,11 @@ virmockdbus_la_CFLAGS = $(AM_CFLAGS) $(DBUS_CFLAGS) virmockdbus_la_LDFLAGS = -module -avoid-version \ -rpath /evil/libtool/hack/to/force/shared/lib/creation +virpolkittest_SOURCES = \ + virpolkittest.c testutils.h testutils.c +virpolkittest_CFLAGS = $(AM_CFLAGS) $(DBUS_CFLAGS) +virpolkittest_LDADD = $(LDADDS) $(DBUS_LIBS) + virsystemdtest_SOURCES = \ virsystemdtest.c testutils.h testutils.c virsystemdtest_CFLAGS = $(AM_CFLAGS) $(DBUS_CFLAGS) diff --git a/tests/virpolkittest.c b/tests/virpolkittest.c new file mode 100644 index 0000000..4b3d6f0 --- /dev/null +++ b/tests/virpolkittest.c @@ -0,0 +1,360 @@ +/* + * Copyright (C) 2013, 2014 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>;. + * + * Author: Daniel P. Berrange <berrange(a)redhat.com&gt; + */ + +#include <config.h> + +#include "testutils.h" + +#if defined(WITH_DBUS) && defined(__linux__) + +# include <stdlib.h> +# include <dbus/dbus.h> + +# include "virpolkit.h" +# include "virdbus.h" +# include "virlog.h" +# include "virmock.h" +# define VIR_FROM_THIS VIR_FROM_NONE + +VIR_LOG_INIT("tests.systemdtest"); + +/* Some interesting numbers */ +# define THE_PID 1458 +# define THE_TIME 11011000001 +# define THE_UID 1729 + +VIR_MOCK_IMPL_RET_ARGS(dbus_connection_send_with_reply_and_block, + DBusMessage *, + DBusConnection *, connection, + DBusMessage *, message, + int, timeout_milliseconds, + DBusError *, error) +{ + DBusMessage *reply = NULL; + const char *service = dbus_message_get_destination(message); + const char *member = dbus_message_get_member(message); + + VIR_MOCK_IMPL_INIT_REAL(dbus_connection_send_with_reply_and_block);