[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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 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 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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]