[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); + + if (STREQ(service, "org.freedesktop.PolicyKit1") && + STREQ(member, "CheckAuthorization")) { + char *type; + char *pidkey; + unsigned int pidval; + char *timekey; + unsigned long long timeval; + char *uidkey; + int uidval; + char *actionid; + char **details; + size_t detailslen; + int allowInteraction; + char *cancellationId; + const char **retdetails = NULL; + size_t retdetailslen = 0; + const char *retdetailscancelled[] = { + "polkit.dismissed", "true", + }; + int is_authorized = 1; + int is_challenge = 0; + + if (virDBusMessageRead(message, + "(sa{sv})sa&{ss}us", + &type, + 3, + &pidkey, "u", &pidval, + &timekey, "t", &timeval, + &uidkey, "i", &uidval, + &actionid, + &detailslen, + &details, + &allowInteraction, + &cancellationId) < 0) + goto error; + + if (STREQ(actionid, "org.libvirt.test.success")) { + is_authorized = 1; + is_challenge = 0; + } else if (STREQ(actionid, "org.libvirt.test.challenge")) { + is_authorized = 0; + is_challenge = 1; + } else if (STREQ(actionid, "org.libvirt.test.cancelled")) { + is_authorized = 0; + is_challenge = 0; + retdetails = retdetailscancelled; + retdetailslen = ARRAY_CARDINALITY(retdetailscancelled) / 2; + } else if (STREQ(actionid, "org.libvirt.test.details")) { + size_t i; + is_authorized = 0; + is_challenge = 0; + for (i = 0; i < detailslen / 2; i++) { + if (STREQ(details[i * 2], + "org.libvirt.test.person") && + STREQ(details[(i * 2) + 1], + "Fred")) { + is_authorized = 1; + is_challenge = 0; + } + } + } else { + is_authorized = 0; + is_challenge = 0; + } + + VIR_FREE(type); + VIR_FREE(pidkey); + VIR_FREE(timekey); + VIR_FREE(uidkey); + VIR_FREE(actionid); + VIR_FREE(cancellationId); + virStringFreeListCount(details, detailslen); + + if (virDBusCreateReply(&reply, + "(bba&{ss})", + is_authorized, + is_challenge, + retdetailslen, + retdetails) < 0) + goto error; + } else { + reply = dbus_message_new(DBUS_MESSAGE_TYPE_METHOD_RETURN); + } + + return reply; + + error: + dbus_message_unref(reply); + return NULL; +} + + + +static int testPolkitAuthSuccess(const void *opaque ATTRIBUTE_UNUSED) +{ + int ret = -1; + + if (virPolkitCheckAuth("org.libvirt.test.success", + THE_PID, + THE_TIME, + THE_UID, + NULL, + true) < 0) + goto cleanup; + + ret = 0; + + cleanup: + return ret; +} + + +static int testPolkitAuthDenied(const void *opaque ATTRIBUTE_UNUSED) +{ + int ret = -1; + int rv; + virErrorPtr err; + + rv = virPolkitCheckAuth("org.libvirt.test.deny", + THE_PID, + THE_TIME, + THE_UID, + NULL, + true); + + if (rv == 0) { + fprintf(stderr, "Unexpected auth success\n"); + goto cleanup; + } else if (rv != -2) { + goto cleanup; + } + + err = virGetLastError(); + if (!err || !strstr(err->message, + _("access denied by policy"))) { + fprintf(stderr, "Incorrect error response\n"); + goto cleanup; + } + + ret = 0; + + cleanup: + return ret; +} + + +static int testPolkitAuthChallenge(const void *opaque ATTRIBUTE_UNUSED) +{ + int ret = -1; + int rv; + virErrorPtr err; + + rv = virPolkitCheckAuth("org.libvirt.test.challenge", + THE_PID, + THE_TIME, + THE_UID, + NULL, + true); + + if (rv == 0) { + fprintf(stderr, "Unexpected auth success\n"); + goto cleanup; + } else if (rv != -2) { + goto cleanup; + } + + err = virGetLastError(); + if (!err || !strstr(err->message, + _("no agent is available to authenticate"))) { + fprintf(stderr, "Incorrect error response\n"); + goto cleanup; + } + + ret = 0; + + cleanup: + return ret; +} + + +static int testPolkitAuthCancelled(const void *opaque ATTRIBUTE_UNUSED) +{ + int ret = -1; + int rv; + virErrorPtr err; + + rv = virPolkitCheckAuth("org.libvirt.test.cancelled", + THE_PID, + THE_TIME, + THE_UID, + NULL, + true); + + if (rv == 0) { + fprintf(stderr, "Unexpected auth success\n"); + goto cleanup; + } else if (rv != -2) { + goto cleanup; + } + + err = virGetLastError(); + if (!err || !strstr(err->message, + _("user cancelled authentication process"))) { + fprintf(stderr, "Incorrect error response\n"); + goto cleanup; + } + + ret = 0; + + cleanup: + return ret; +} + + +static int testPolkitAuthDetailsSuccess(const void *opaque ATTRIBUTE_UNUSED) +{ + int ret = -1; + const char *details[] = { + "org.libvirt.test.person", "Fred", + NULL, + }; + + if (virPolkitCheckAuth("org.libvirt.test.details", + THE_PID, + THE_TIME, + THE_UID, + details, + true) < 0) + goto cleanup; + + ret = 0; + + cleanup: + return ret; +} + + +static int testPolkitAuthDetailsDenied(const void *opaque ATTRIBUTE_UNUSED) +{ + int ret = -1; + int rv; + virErrorPtr err; + const char *details[] = { + "org.libvirt.test.person", "Joe", + NULL, + }; + + rv = virPolkitCheckAuth("org.libvirt.test.details", + THE_PID, + THE_TIME, + THE_UID, + details, + true); + + if (rv == 0) { + fprintf(stderr, "Unexpected auth success\n"); + goto cleanup; + } else if (rv != -2) { + goto cleanup; + } + + err = virGetLastError(); + if (!err || !strstr(err->message, + _("access denied by policy"))) { + fprintf(stderr, "Incorrect error response\n"); + goto cleanup; + } + + ret = 0; + + cleanup: + return ret; +} + + +static int +mymain(void) +{ + int ret = 0; + + if (virtTestRun("Polkit auth success ", testPolkitAuthSuccess, NULL) < 0) + ret = -1; + if (virtTestRun("Polkit auth deny ", testPolkitAuthDenied, NULL) < 0) + ret = -1; + if (virtTestRun("Polkit auth challenge ", testPolkitAuthChallenge, NULL) < 0) + ret = -1; + if (virtTestRun("Polkit auth cancel ", testPolkitAuthCancelled, NULL) < 0) + ret = -1; + if (virtTestRun("Polkit auth details success ", testPolkitAuthDetailsSuccess, NULL) < 0) + ret = -1; + if (virtTestRun("Polkit auth details deny ", testPolkitAuthDetailsDenied, NULL) < 0) + ret = -1; + + return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; +} + +VIRT_TEST_MAIN_PRELOAD(mymain, abs_builddir "/.libs/virmockdbus.so") + +#else /* ! (WITH_DBUS && __linux__) */ +int +main(void) +{ + return EXIT_AM_SKIP; +} +#endif /* ! WITH_DBUS */ -- 1.9.3