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