[libvirt] [PATCH 7/7] Convert polkit code to use DBus API instead of CLI helper
Michal Privoznik
mprivozn at redhat.com
Fri Sep 12 12:20:31 UTC 2014
On 10.09.2014 16:21, Daniel P. Berrange wrote:
> 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 at redhat.com>
> ---
> 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 at redhat.com>
> + */
> +
> +#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);
After your latest patches, you need to update this:
diff --git a/tests/virpolkittest.c b/tests/virpolkittest.c
index 4b3d6f0..bc97529 100644
--- a/tests/virpolkittest.c
+++ b/tests/virpolkittest.c
@@ -51,7 +51,7 @@
VIR_MOCK_IMPL_RET_ARGS(dbus_connection_send_with_reply_and_block,
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);
+ VIR_MOCK_REAL_INIT(dbus_connection_send_with_reply_and_block);
if (STREQ(service, "org.freedesktop.PolicyKit1") &&
STREQ(member, "CheckAuthorization")) {
Michal
More information about the libvir-list
mailing list