Changeset
.gitignore                                         |   1 -
m4/virt-polkit.m4                                  |  80 +++------------
src/access/Makefile.inc.am                         |   6 +-
src/access/viraccessmanager.c                      |   4 +-
src/libvirt.c                                      |  27 -----
src/remote/Makefile.inc.am                         |  24 +----
src/remote/{libvirtd.policy.in => libvirtd.policy} |   6 +-
src/remote/remote_driver.c                         |  63 ------------
src/util/Makefile.inc.am                           |   2 -
src/util/virpolkit.c                               | 113 +--------------------
tests/Makefile.am                                  |   4 +-
11 files changed, 28 insertions(+), 302 deletions(-)
rename src/remote/{libvirtd.policy.in => libvirtd.policy} (92%)
Git apply log
Switched to a new branch 'cover.1520414902.git.jtomko@redhat.com'
Applying: Remove Policy-Kit support
Applying: Merge WITH_POLKIT1 and WITH_POLKIT
Applying: Do not check for pkcheck
To https://github.com/patchew-project/libvirt
 + d34564a5b...7025efc37 patchew/cover.1520414902.git.jtomko@redhat.com -> patchew/cover.1520414902.git.jtomko@redhat.com (forced update)
Test passed: syntax-check

loading

[libvirt] [PATCH 0/3] Drop old Policy-Kit support
Posted by Ján Tomko, 15 weeks ago
The build WITH_POLKIT0 and -Wunused-label was broken for almost two
years.

Ján Tomko (3):
  Remove Policy-Kit support
  Merge WITH_POLKIT1 and WITH_POLKIT
  Do not check for pkcheck

 .gitignore                                         |   1 -
 m4/virt-polkit.m4                                  |  80 +++------------
 src/access/Makefile.inc.am                         |   6 +-
 src/access/viraccessmanager.c                      |   4 +-
 src/libvirt.c                                      |  27 -----
 src/remote/Makefile.inc.am                         |  24 +----
 src/remote/{libvirtd.policy.in => libvirtd.policy} |   6 +-
 src/remote/remote_driver.c                         |  63 ------------
 src/util/Makefile.inc.am                           |   2 -
 src/util/virpolkit.c                               | 113 +--------------------
 tests/Makefile.am                                  |   4 +-
 11 files changed, 28 insertions(+), 302 deletions(-)
 rename src/remote/{libvirtd.policy.in => libvirtd.policy} (92%)

-- 
2.16.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/3] Drop old Policy-Kit support
Posted by Daniel P. Berrangé, 15 weeks ago
On Wed, Mar 07, 2018 at 10:29:29AM +0100, Ján Tomko wrote:
> The build WITH_POLKIT0 and -Wunused-label was broken for almost two
> years.

Last time we discussed this, SUSE folks said they still had polkit0 on
an actively supported distro. I'd like confirmation they don't still
need it before removing, so copying Jim.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/3] Drop old Policy-Kit support
Posted by Peter Krempa, 15 weeks ago
On Wed, Mar 07, 2018 at 10:15:35 +0000, Daniel Berrange wrote:
> On Wed, Mar 07, 2018 at 10:29:29AM +0100, J�n Tomko wrote:
> > The build WITH_POLKIT0 and -Wunused-label was broken for almost two
> > years.
> 
> Last time we discussed this, SUSE folks said they still had polkit0 on
> an actively supported distro. I'd like confirmation they don't still
> need it before removing, so copying Jim.

Probably not very actively used though since nobody complained that it
was broken for so long.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/3] Drop old Policy-Kit support
Posted by Daniel P. Berrangé, 15 weeks ago
On Wed, Mar 07, 2018 at 12:09:15PM +0100, Peter Krempa wrote:
> On Wed, Mar 07, 2018 at 10:15:35 +0000, Daniel Berrange wrote:
> > On Wed, Mar 07, 2018 at 10:29:29AM +0100, Ján Tomko wrote:
> > > The build WITH_POLKIT0 and -Wunused-label was broken for almost two
> > > years.
> > 
> > Last time we discussed this, SUSE folks said they still had polkit0 on
> > an actively supported distro. I'd like confirmation they don't still
> > need it before removing, so copying Jim.
> 
> Probably not very actively used though since nobody complained that it
> was broken for so long.

Well downstreams don't run with -Werror usually, so fact that -Wunused-label
generated a warning doesn't mean it was broken for downstreams.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/3] Drop old Policy-Kit support
Posted by Jim Fehlig, 15 weeks ago
On 03/07/2018 03:15 AM, Daniel P. Berrangé wrote:
> On Wed, Mar 07, 2018 at 10:29:29AM +0100, Ján Tomko wrote:
>> The build WITH_POLKIT0 and -Wunused-label was broken for almost two
>> years.
> 
> Last time we discussed this, SUSE folks said they still had polkit0 on
> an actively supported distro. I'd like confirmation they don't still
> need it before removing, so copying Jim.

SLES 11 contains polkit0 and is actively supported, but I don't see any reason 
for that old distro to hamstring upstream. The SLES11 target was removed from 
automated builds of libvirt.git quite a while back.

Regards,
Jim

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/3] Remove Policy-Kit support
Posted by Ján Tomko, 15 weeks ago
Policy-Kit has been replaced by polkit (referred to as POLKIT0
and POLKIT1 in our Makefiles).

The last build fix with old Policy-Kit was in May 2013:
commit <442eb2ba> and build with -Wunused-label was broken
since April 2016: commit <8437130>

This includes a partial revert of commit <e1019e9>, which added
an extra step to generating the policy file.

Signed-off-by: Ján Tomko <jtomko@redhat.com>
---
 .gitignore                                         |   1 -
 m4/virt-polkit.m4                                  |  44 +--------
 src/libvirt.c                                      |  27 -----
 src/remote/Makefile.inc.am                         |  24 +----
 src/remote/{libvirtd.policy.in => libvirtd.policy} |   6 +-
 src/remote/remote_driver.c                         |  63 ------------
 src/util/Makefile.inc.am                           |   2 -
 src/util/virpolkit.c                               | 109 +--------------------
 8 files changed, 8 insertions(+), 268 deletions(-)
 rename src/remote/{libvirtd.policy.in => libvirtd.policy} (92%)

diff --git a/.gitignore b/.gitignore
index 2ca7d9776..dd00fc5cc 100644
--- a/.gitignore
+++ b/.gitignore
@@ -135,7 +135,6 @@
 /src/libvirt_lxc
 /src/libvirtd
 /src/libvirtd*.logrotate
-/src/libvirtd.policy
 /src/locking/libxl-lockd.conf
 /src/locking/libxl-sanlock.conf
 /src/locking/lock_daemon_dispatch_stubs.h
diff --git a/m4/virt-polkit.m4 b/m4/virt-polkit.m4
index 7bdbf804d..9426c7d5d 100644
--- a/m4/virt-polkit.m4
+++ b/m4/virt-polkit.m4
@@ -25,12 +25,8 @@ AC_DEFUN([LIBVIRT_ARG_POLKIT], [
 AC_DEFUN([LIBVIRT_CHECK_POLKIT], [
   AC_REQUIRE([LIBVIRT_CHECK_DBUS])
 
-  POLKIT_REQUIRED="0.6"
-  POLKIT_CFLAGS=
-  POLKIT_LIBS=
   PKCHECK_PATH=
 
-  with_polkit0=no
   with_polkit1=no
 
   if test "x$with_polkit" = "xyes" || test "x$with_polkit" = "xcheck"; then
@@ -56,52 +52,14 @@ AC_DEFUN([LIBVIRT_CHECK_POLKIT], [
              [You must install dbus to compile libvirt with polkit-1])
         fi
       fi
-    else
-      dnl Check for old polkit second - library + binary
-      PKG_CHECK_MODULES(POLKIT, polkit-dbus >= $POLKIT_REQUIRED,
-        [with_polkit=yes], [
-        if test "x$with_polkit" = "xcheck" ; then
-           with_polkit=no
-        else
-           AC_MSG_ERROR(
-             [You must install PolicyKit >= $POLKIT_REQUIRED to compile libvirt])
-        fi
-      ])
-      if test "x$with_polkit" = "xyes" ; then
-        AC_DEFINE_UNQUOTED([WITH_POLKIT], 1,
-          [use PolicyKit for UNIX socket access checks])
-        AC_DEFINE_UNQUOTED([WITH_POLKIT0], 1,
-          [use PolicyKit for UNIX socket access checks])
-
-        old_CFLAGS=$CFLAGS
-        old_LIBS=$LIBS
-        CFLAGS="$CFLAGS $POLKIT_CFLAGS"
-        LIBS="$LIBS $POLKIT_LIBS"
-        AC_CHECK_FUNCS([polkit_context_is_caller_authorized])
-        CFLAGS="$old_CFLAGS"
-        LIBS="$old_LIBS"
-
-        AC_PATH_PROG([POLKIT_AUTH], [polkit-auth])
-        if test "x$POLKIT_AUTH" != "x"; then
-          AC_DEFINE_UNQUOTED([POLKIT_AUTH],["$POLKIT_AUTH"],[Location of polkit-auth program])
-        fi
-        with_polkit0="yes"
-      fi
     fi
   fi
 
   AM_CONDITIONAL([WITH_POLKIT], [test "x$with_polkit" = "xyes"])
-  AM_CONDITIONAL([WITH_POLKIT0], [test "x$with_polkit0" = "xyes"])
   AM_CONDITIONAL([WITH_POLKIT1], [test "x$with_polkit1" = "xyes"])
-  AC_SUBST([POLKIT_CFLAGS])
-  AC_SUBST([POLKIT_LIBS])
 ])
 
 AC_DEFUN([LIBVIRT_RESULT_POLKIT], [
-  if test "$with_polkit0" = "yes" ; then
-    msg="$POLKIT_CFLAGS $POLKIT_LIBS (version 0)"
-  else
-    msg="$PKCHECK_PATH (version 1)"
-  fi
+  msg="$PKCHECK_PATH (version 1)"
   LIBVIRT_RESULT([polkit], [$with_polkit], [$msg])
 ])
diff --git a/src/libvirt.c b/src/libvirt.c
index 536d56f0a..b7bcf8022 100644
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -121,28 +121,6 @@ static virSecretDriverPtr virSharedSecretDriver;
 static virNWFilterDriverPtr virSharedNWFilterDriver;
 
 
-#if defined(POLKIT_AUTH)
-static int
-virConnectAuthGainPolkit(const char *privilege)
-{
-    virCommandPtr cmd;
-    int ret = -1;
-
-    if (geteuid() == 0)
-        return 0;
-
-    cmd = virCommandNewArgList(POLKIT_AUTH, "--obtain", privilege, NULL);
-    if (virCommandRun(cmd, NULL) < 0)
-        goto cleanup;
-
-    ret = 0;
- cleanup:
-    virCommandFree(cmd);
-    return ret;
-}
-#endif
-
-
 static int
 virConnectAuthCallbackDefault(virConnectCredentialPtr cred,
                               unsigned int ncred,
@@ -160,16 +138,11 @@ virConnectAuthCallbackDefault(virConnectCredentialPtr cred,
             if (STRNEQ(cred[i].challenge, "PolicyKit"))
                 return -1;
 
-#if defined(POLKIT_AUTH)
-            if (virConnectAuthGainPolkit(cred[i].prompt) < 0)
-                return -1;
-#else
             /*
              * Ignore & carry on. Although we can't auth
              * directly, the user may have authenticated
              * themselves already outside context of libvirt
              */
-#endif
             break;
         }
 
diff --git a/src/remote/Makefile.inc.am b/src/remote/Makefile.inc.am
index a6e8ecabf..12600b8bb 100644
--- a/src/remote/Makefile.inc.am
+++ b/src/remote/Makefile.inc.am
@@ -75,7 +75,7 @@ EXTRA_DIST += \
 	remote/test_libvirtd.aug.in \
 	remote/libvirtd.aug \
 	remote/libvirtd.conf \
-	remote/libvirtd.policy.in \
+	remote/libvirtd.policy \
 	remote/libvirtd.rules \
 	remote/libvirtd.sasl \
 	remote/libvirtd.sysctl \
@@ -120,18 +120,9 @@ conf_DATA += remote/libvirtd.conf
 CLEANFILES += test_libvirtd.aug
 
 if WITH_POLKIT
-if WITH_POLKIT0
-policydir = $(datadir)/PolicyKit/policy
-policyauth = auth_admin_keep_session
-else ! WITH_POLKIT0
 policydir = $(datadir)/polkit-1/actions
-policyauth = auth_admin_keep
-endif ! WITH_POLKIT0
 endif WITH_POLKIT
 
-BUILT_SOURCES += libvirtd.policy
-CLEANFILES += libvirtd.policy
-
 man8_MANS += libvirtd.8
 
 libvirtd_SOURCES = $(LIBVIRTD_SOURCES)
@@ -218,20 +209,17 @@ endif ! WITH_SYSCTL
 if WITH_POLKIT
 install-polkit::
 	$(MKDIR_P) $(DESTDIR)$(policydir)
-	$(INSTALL_DATA) libvirtd.policy $(DESTDIR)$(policydir)/org.libvirt.unix.policy
-if ! WITH_POLKIT0
+	$(INSTALL_DATA) $(srcdir)/remote/libvirtd.policy \
+		$(DESTDIR)$(policydir)/org.libvirt.unix.policy
 	$(MKDIR_P) $(DESTDIR)$(datadir)/polkit-1/rules.d
 	$(INSTALL_DATA) $(srcdir)/remote/libvirtd.rules \
 		$(DESTDIR)$(datadir)/polkit-1/rules.d/50-libvirt.rules
-endif ! WITH_POLKIT0
 
 uninstall-polkit::
 	rm -f $(DESTDIR)$(policydir)/org.libvirt.unix.policy
 	rmdir $(DESTDIR)$(policydir) || :
-if ! WITH_POLKIT0
 	rm -f $(DESTDIR)$(datadir)/polkit-1/rules.d/50-libvirt.rules
 	rmdir $(DESTDIR)$(datadir)/polkit-1/rules.d || :
-endif ! WITH_POLKIT0
 
 else ! WITH_POLKIT
 install-polkit::
@@ -267,12 +255,6 @@ install-sasl:
 uninstall-sasl:
 endif ! WITH_SASL
 
-libvirtd.policy: remote/libvirtd.policy.in $(top_builddir)/config.status
-	$(AM_V_GEN) sed \
-	    -e 's|[@]authaction[@]|$(policyauth)|g' \
-	    < $< > $@-t && \
-	mv $@-t $@
-
 libvirtd.init: remote/libvirtd.init.in $(top_builddir)/config.status
 	$(AM_V_GEN)sed \
 	    -e 's|[@]localstatedir[@]|$(localstatedir)|g' \
diff --git a/src/remote/libvirtd.policy.in b/src/remote/libvirtd.policy
similarity index 92%
rename from src/remote/libvirtd.policy.in
rename to src/remote/libvirtd.policy
index de1aba459..e834d2432 100644
--- a/src/remote/libvirtd.policy.in
+++ b/src/remote/libvirtd.policy
@@ -43,9 +43,9 @@ License along with this library.  If not, see
       <defaults>
         <!-- Any program can use libvirt in read/write mode if they
              provide the root password -->
-        <allow_any>@authaction@</allow_any>
-        <allow_inactive>@authaction@</allow_inactive>
-        <allow_active>@authaction@</allow_active>
+        <allow_any>auth_admin_keep</allow_any>
+        <allow_inactive>auth_admin_keep</allow_inactive>
+        <allow_active>auth_admin_keep</allow_active>
       </defaults>
     </action>
 </policyconfig>
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index 9ea726dc4..bf00e3210 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -4289,64 +4289,6 @@ remoteAuthSASL(virConnectPtr conn, struct private_data *priv,
 #endif /* WITH_SASL */
 
 
-#if WITH_POLKIT0
-/* Perform the PolicyKit0 authentication process */
-static int
-remoteAuthPolkit0(virConnectPtr conn, struct private_data *priv,
-                 virConnectAuthPtr auth)
-{
-    remote_auth_polkit_ret ret;
-    size_t i;
-    int allowcb = 0;
-    virConnectCredential cred = {
-        VIR_CRED_EXTERNAL,
-        conn->flags & VIR_CONNECT_RO ? "org.libvirt.unix.monitor" : "org.libvirt.unix.manage",
-        "PolicyKit",
-        NULL,
-        NULL,
-        0,
-    };
-    VIR_DEBUG("Client initialize PolicyKit-0 authentication");
-
-    /* We only make it here if auth already failed
-     * Ask client to obtain it and check again. */
-    if (auth && auth->cb) {
-        /* Check if the necessary credential type for PolicyKit is supported */
-        for (i = 0; i < auth->ncredtype; i++) {
-            if (auth->credtype[i] == VIR_CRED_EXTERNAL)
-                allowcb = 1;
-        }
-
-        if (allowcb) {
-            VIR_DEBUG("Client run callback for PolicyKit authentication");
-            /* Run the authentication callback */
-            if ((*(auth->cb))(&cred, 1, auth->cbdata) < 0) {
-                virReportError(VIR_ERR_AUTH_FAILED, "%s",
-                               _("Failed to collect auth credentials"));
-                return -1;
-            }
-        } else {
-            VIR_DEBUG("Client auth callback does not support PolicyKit");
-            return -1;
-        }
-    } else {
-        VIR_DEBUG("No auth callback provided");
-        return -1;
-    }
-
-    memset(&ret, 0, sizeof(ret));
-    if (call(conn, priv, 0, REMOTE_PROC_AUTH_POLKIT,
-             (xdrproc_t) xdr_void, (char *)NULL,
-             (xdrproc_t) xdr_remote_auth_polkit_ret, (char *) &ret) != 0) {
-        return -1; /* virError already set by call */
-    }
-
- out:
-    VIR_DEBUG("PolicyKit-0 authentication complete");
-    return 0;
-}
-#endif /* WITH_POLKIT0 */
-
 static int
 remoteAuthPolkit(virConnectPtr conn, struct private_data *priv,
                  virConnectAuthPtr auth ATTRIBUTE_UNUSED)
@@ -4361,11 +4303,6 @@ remoteAuthPolkit(virConnectPtr conn, struct private_data *priv,
         return -1; /* virError already set by call */
     }
 
-#if WITH_POLKIT0
-    if (remoteAuthPolkit0(conn, priv, auth) < 0)
-        return -1;
-#endif /* WITH_POLKIT0 */
-
     VIR_DEBUG("PolicyKit authentication complete");
     return 0;
 }
diff --git a/src/util/Makefile.inc.am b/src/util/Makefile.inc.am
index a91b30dca..3f9d1164b 100644
--- a/src/util/Makefile.inc.am
+++ b/src/util/Makefile.inc.am
@@ -251,7 +251,6 @@ libvirt_util_la_CFLAGS = \
 	$(DBUS_CFLAGS) \
 	$(LDEXP_LIBM) \
 	$(NUMACTL_CFLAGS) \
-	$(POLKIT_CFLAGS) \
 	$(GNUTLS_CFLAGS) \
 	$(ACL_CFLAGS) \
 	$(NULL)
@@ -269,7 +268,6 @@ libvirt_util_la_LIBADD = \
 	$(SECDRIVER_LIBS) \
 	$(NUMACTL_LIBS) \
 	$(ACL_LIBS) \
-	$(POLKIT_LIBS) \
 	$(GNUTLS_LIBS) \
 	$(NULL)
 
diff --git a/src/util/virpolkit.c b/src/util/virpolkit.c
index 4559431ba..2e8660188 100644
--- a/src/util/virpolkit.c
+++ b/src/util/virpolkit.c
@@ -22,11 +22,6 @@
 #include <config.h>
 #include <poll.h>
 
-#if WITH_POLKIT0
-# include <polkit/polkit.h>
-# include <polkit-dbus/polkit-dbus.h>
-#endif
-
 #include "virpolkit.h"
 #include "virerror.h"
 #include "virlog.h"
@@ -211,109 +206,7 @@ virPolkitAgentCreate(void)
 }
 
 
-#elif WITH_POLKIT0
-int virPolkitCheckAuth(const char *actionid,
-                       pid_t pid,
-                       unsigned long long startTime ATTRIBUTE_UNUSED,
-                       uid_t uid,
-                       const char **details,
-                       bool allowInteraction ATTRIBUTE_UNUSED)
-{
-    PolKitCaller *pkcaller = NULL;
-    PolKitAction *pkaction = NULL;
-    PolKitContext *pkcontext = NULL;
-    PolKitError *pkerr = NULL;
-    PolKitResult pkresult;
-    DBusError err;
-    DBusConnection *sysbus;
-    int ret = -1;
-
-    if (details) {
-        virReportError(VIR_ERR_AUTH_FAILED, "%s",
-                       _("Details not supported with polkit v0"));
-        return -1;
-    }
-
-    if (!(sysbus = virDBusGetSystemBus()))
-        goto cleanup;
-
-    VIR_INFO("Checking PID %lld running as %d",
-             (long long) pid, uid);
-    dbus_error_init(&err);
-    if (!(pkcaller = polkit_caller_new_from_pid(sysbus,
-                                                pid, &err))) {
-        VIR_DEBUG("Failed to lookup policy kit caller: %s", err.message);
-        dbus_error_free(&err);
-        goto cleanup;
-    }
-
-    if (!(pkaction = polkit_action_new())) {
-        char ebuf[1024];
-        VIR_DEBUG("Failed to create polkit action %s",
-                  virStrerror(errno, ebuf, sizeof(ebuf)));
-        goto cleanup;
-    }
-    polkit_action_set_action_id(pkaction, actionid);
-
-    if (!(pkcontext = polkit_context_new()) ||
-        !polkit_context_init(pkcontext, &pkerr)) {
-        char ebuf[1024];
-        VIR_DEBUG("Failed to create polkit context %s",
-                  (pkerr ? polkit_error_get_error_message(pkerr)
-                   : virStrerror(errno, ebuf, sizeof(ebuf))));
-        if (pkerr)
-            polkit_error_free(pkerr);
-        dbus_error_free(&err);
-        goto cleanup;
-    }
-
-# if HAVE_POLKIT_CONTEXT_IS_CALLER_AUTHORIZED
-    pkresult = polkit_context_is_caller_authorized(pkcontext,
-                                                   pkaction,
-                                                   pkcaller,
-                                                   0,
-                                                   &pkerr);
-    if (pkerr && polkit_error_is_set(pkerr)) {
-        VIR_DEBUG("Policy kit failed to check authorization %d %s",
-                  polkit_error_get_error_code(pkerr),
-                  polkit_error_get_error_message(pkerr));
-        goto cleanup;
-    }
-# else
-    pkresult = polkit_context_can_caller_do_action(pkcontext,
-                                                   pkaction,
-                                                   pkcaller);
-# endif
-    if (pkresult != POLKIT_RESULT_YES) {
-        VIR_DEBUG("Policy kit denied action %s from pid %lld, uid %d, result: %s",
-                  actionid, (long long) pid, uid,
-                  polkit_result_to_string_representation(pkresult));
-        ret = -2;
-        goto cleanup;
-    }
-
-    VIR_DEBUG("Policy allowed action %s from pid %lld, uid %d",
-              actionid, (long long)pid, (int)uid);
-
-    ret = 0;
-
- cleanup:
-    if (ret < 0) {
-        virResetLastError();
-        virReportError(VIR_ERR_AUTH_FAILED, "%s",
-                       _("authentication failed"));
-    }
-    if (pkcontext)
-        polkit_context_unref(pkcontext);
-    if (pkcaller)
-        polkit_caller_unref(pkcaller);
-    if (pkaction)
-        polkit_action_unref(pkaction);
-    return ret;
-}
-
-
-#else /* ! WITH_POLKIT1 && ! WITH_POLKIT0 */
+#else /* ! WITH_POLKIT1 */
 
 int virPolkitCheckAuth(const char *actionid ATTRIBUTE_UNUSED,
                        pid_t pid ATTRIBUTE_UNUSED,
-- 
2.16.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/3] Remove Policy-Kit support
Posted by Andrea Bolognani, 15 weeks ago
On Wed, 2018-03-07 at 10:29 +0100, Ján Tomko wrote:
> Policy-Kit has been replaced by polkit (referred to as POLKIT0
> and POLKIT1 in our Makefiles).

... referred to, respectively, as ...

[...]
>  if WITH_POLKIT
> -if WITH_POLKIT0
> -policydir = $(datadir)/PolicyKit/policy
> -policyauth = auth_admin_keep_session
> -else ! WITH_POLKIT0
>  policydir = $(datadir)/polkit-1/actions
> -policyauth = auth_admin_keep
> -endif ! WITH_POLKIT0
>  endif WITH_POLKIT
>  
> -BUILT_SOURCES += libvirtd.policy
> -CLEANFILES += libvirtd.policy

[...]
> -libvirtd.policy: remote/libvirtd.policy.in $(top_builddir)/config.status
> -	$(AM_V_GEN) sed \
> -	    -e 's|[@]authaction[@]|$(policyauth)|g' \
> -	    < $< > $@-t && \
> -	mv $@-t $@

[...]
> -        <allow_any>@authaction@</allow_any>
> -        <allow_inactive>@authaction@</allow_inactive>
> -        <allow_active>@authaction@</allow_active>
> +        <allow_any>auth_admin_keep</allow_any>
> +        <allow_inactive>auth_admin_keep</allow_inactive>
> +        <allow_active>auth_admin_keep</allow_active>

The bit about generating the .policy file dynamically is clearly
not needed anymore after you've removed support for the second
polkit version; however, that machinery can just as well be
removed in a follow-up commit, so I'd like you to do that.

The rest looks good, so

  Reviewed-by: Andrea Bolognani <abologna@redhat.com>

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/3] Merge WITH_POLKIT1 and WITH_POLKIT
Posted by Ján Tomko, 15 weeks ago
There is just one polkit now.

Signed-off-by: Ján Tomko <jtomko@redhat.com>
---
 m4/virt-polkit.m4             | 6 ------
 src/access/Makefile.inc.am    | 6 +++---
 src/access/viraccessmanager.c | 4 ++--
 src/util/virpolkit.c          | 6 +++---
 tests/Makefile.am             | 4 ++--
 5 files changed, 10 insertions(+), 16 deletions(-)

diff --git a/m4/virt-polkit.m4 b/m4/virt-polkit.m4
index 9426c7d5d..5c2a3c1e3 100644
--- a/m4/virt-polkit.m4
+++ b/m4/virt-polkit.m4
@@ -27,8 +27,6 @@ AC_DEFUN([LIBVIRT_CHECK_POLKIT], [
 
   PKCHECK_PATH=
 
-  with_polkit1=no
-
   if test "x$with_polkit" = "xyes" || test "x$with_polkit" = "xcheck"; then
     dnl Check for new polkit first. We directly talk over DBus
     dnl but we use existence of pkcheck binary as a sign that
@@ -40,10 +38,7 @@ AC_DEFUN([LIBVIRT_CHECK_POLKIT], [
       if test "x$with_dbus" = "xyes" ; then
         AC_DEFINE_UNQUOTED([WITH_POLKIT], 1,
             [use PolicyKit for UNIX socket access checks])
-        AC_DEFINE_UNQUOTED([WITH_POLKIT1], 1,
-            [use PolicyKit for UNIX socket access checks])
         with_polkit="yes"
-        with_polkit1="yes"
       else
         if test "x$with_polkit" = "xcheck" ; then
           with_polkit=no
@@ -56,7 +51,6 @@ AC_DEFUN([LIBVIRT_CHECK_POLKIT], [
   fi
 
   AM_CONDITIONAL([WITH_POLKIT], [test "x$with_polkit" = "xyes"])
-  AM_CONDITIONAL([WITH_POLKIT1], [test "x$with_polkit1" = "xyes"])
 ])
 
 AC_DEFUN([LIBVIRT_RESULT_POLKIT], [
diff --git a/src/access/Makefile.inc.am b/src/access/Makefile.inc.am
index c68ba5f04..6d57ca1a1 100644
--- a/src/access/Makefile.inc.am
+++ b/src/access/Makefile.inc.am
@@ -64,7 +64,7 @@ $(ACCESS_DRIVER_POLKIT_POLICY): $(srcdir)/access/viraccessperm.h \
     $(srcdir)/access/genpolkit.pl Makefile.am
 	$(AM_V_GEN)$(PERL) $(srcdir)/access/genpolkit.pl < $< > $@ || rm -f $@
 
-if WITH_POLKIT1
+if WITH_POLKIT
 libvirt_driver_access_la_SOURCES += $(ACCESS_DRIVER_POLKIT_SOURCES)
 
 polkitactiondir = $(datadir)/polkit-1/actions
@@ -74,9 +74,9 @@ endif WITH_LIBVIRTD
 
 CLEANFILES += $(ACCESS_DRIVER_POLKIT_POLICY)
 BUILT_SOURCES += $(ACCESS_DRIVER_POLKIT_POLICY)
-else ! WITH_POLKIT1
+else ! WITH_POLKIT
 EXTRA_DIST += $(ACCESS_DRIVER_POLKIT_SOURCES)
-endif ! WITH_POLKIT1
+endif ! WITH_POLKIT
 
 
 BUILT_SOURCES += \
diff --git a/src/access/viraccessmanager.c b/src/access/viraccessmanager.c
index cbfefb9d4..c268ec57f 100644
--- a/src/access/viraccessmanager.c
+++ b/src/access/viraccessmanager.c
@@ -23,7 +23,7 @@
 #include "viraccessmanager.h"
 #include "viraccessdrivernop.h"
 #include "viraccessdriverstack.h"
-#if WITH_POLKIT1
+#if WITH_POLKIT
 # include "viraccessdriverpolkit.h"
 #endif
 #include "viralloc.h"
@@ -112,7 +112,7 @@ static virAccessManagerPtr virAccessManagerNewDriver(virAccessDriverPtr drv)
 
 static virAccessDriverPtr accessDrivers[] = {
     &accessDriverNop,
-#if WITH_POLKIT1
+#if WITH_POLKIT
     &accessDriverPolkit,
 #endif
 };
diff --git a/src/util/virpolkit.c b/src/util/virpolkit.c
index 2e8660188..198439cea 100644
--- a/src/util/virpolkit.c
+++ b/src/util/virpolkit.c
@@ -35,7 +35,7 @@
 
 VIR_LOG_INIT("util.polkit");
 
-#if WITH_POLKIT1
+#if WITH_POLKIT
 
 struct _virPolkitAgent {
     virCommandPtr cmd;
@@ -206,7 +206,7 @@ virPolkitAgentCreate(void)
 }
 
 
-#else /* ! WITH_POLKIT1 */
+#else /* ! WITH_POLKIT */
 
 int virPolkitCheckAuth(const char *actionid ATTRIBUTE_UNUSED,
                        pid_t pid ATTRIBUTE_UNUSED,
@@ -236,4 +236,4 @@ virPolkitAgentCreate(void)
                    _("polkit text authentication agent unavailable"));
     return NULL;
 }
-#endif /* WITH_POLKIT1 */
+#endif /* WITH_POLKIT */
diff --git a/tests/Makefile.am b/tests/Makefile.am
index d794df3e5..6ca34092c 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -250,9 +250,9 @@ test_programs += virdbustest \
                  virsystemdtest \
                  $(NULL)
 test_libraries += virdbusmock.la
-if WITH_POLKIT1
+if WITH_POLKIT
 test_programs += virpolkittest
-endif WITH_POLKIT1
+endif WITH_POLKIT
 endif WITH_DBUS
 
 if WITH_SECDRIVER_SELINUX
-- 
2.16.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/3] Merge WITH_POLKIT1 and WITH_POLKIT
Posted by Andrea Bolognani, 15 weeks ago
On Wed, 2018-03-07 at 10:29 +0100, Ján Tomko wrote:
> There is just one polkit now.
> 
> Signed-off-by: Ján Tomko <jtomko@redhat.com>
> ---
>  m4/virt-polkit.m4             | 6 ------
>  src/access/Makefile.inc.am    | 6 +++---
>  src/access/viraccessmanager.c | 4 ++--
>  src/util/virpolkit.c          | 6 +++---
>  tests/Makefile.am             | 4 ++--
>  5 files changed, 10 insertions(+), 16 deletions(-)

Reviewed-by: Andrea Bolognani <abologna@redhat.com>

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 3/3] Do not check for pkcheck
Posted by Ján Tomko, 15 weeks ago
All we need is DBus.

Signed-off-by: Ján Tomko <jtomko@redhat.com>
---
 m4/virt-polkit.m4 | 30 +++++++++++-------------------
 1 file changed, 11 insertions(+), 19 deletions(-)

diff --git a/m4/virt-polkit.m4 b/m4/virt-polkit.m4
index 5c2a3c1e3..1016df4b3 100644
--- a/m4/virt-polkit.m4
+++ b/m4/virt-polkit.m4
@@ -25,27 +25,19 @@ AC_DEFUN([LIBVIRT_ARG_POLKIT], [
 AC_DEFUN([LIBVIRT_CHECK_POLKIT], [
   AC_REQUIRE([LIBVIRT_CHECK_DBUS])
 
-  PKCHECK_PATH=
-
   if test "x$with_polkit" = "xyes" || test "x$with_polkit" = "xcheck"; then
-    dnl Check for new polkit first. We directly talk over DBus
-    dnl but we use existence of pkcheck binary as a sign that
-    dnl we should prefer polkit-1 over polkit-0, so we check
-    dnl for it even though we don't ultimately use it
-    AC_PATH_PROG([PKCHECK_PATH], [pkcheck], [], [$LIBVIRT_SBIN_PATH])
-    if test "x$PKCHECK_PATH" != "x" ; then
-      dnl Found pkcheck, so ensure dbus-devel is present
-      if test "x$with_dbus" = "xyes" ; then
-        AC_DEFINE_UNQUOTED([WITH_POLKIT], 1,
-            [use PolicyKit for UNIX socket access checks])
-        with_polkit="yes"
+    dnl All we need to talk to polkit is DBus, no need to check
+    dnl for anything else.
+    if test "x$with_dbus" = "xyes" ; then
+      AC_DEFINE_UNQUOTED([WITH_POLKIT], 1,
+          [use PolicyKit for UNIX socket access checks])
+      with_polkit="yes"
+    else
+      if test "x$with_polkit" = "xcheck" ; then
+        with_polkit=no
       else
-        if test "x$with_polkit" = "xcheck" ; then
-          with_polkit=no
-        else
-           AC_MSG_ERROR(
-             [You must install dbus to compile libvirt with polkit-1])
-        fi
+         AC_MSG_ERROR(
+           [You must install dbus to compile libvirt with polkit-1])
       fi
     fi
   fi
-- 
2.16.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/3] Do not check for pkcheck
Posted by Andrea Bolognani, 15 weeks ago
On Wed, 2018-03-07 at 10:29 +0100, Ján Tomko wrote:
[...]
> +    dnl All we need to talk to polkit is DBus, no need to check
> +    dnl for anything else.

The correct name is D-Bus, here and in the commit message.

Also, the second part of the comment ("no need...") doesn't
add any useful information, so just drop it.

Though you dropped $PKCHECK_PATH here, at the end of the file
there's still

  AC_DEFUN([LIBVIRT_RESULT_POLKIT], [
    msg="$PKCHECK_PATH (version 1)"
    LIBVIRT_RESULT([polkit], [$with_polkit], [$msg])
  ])

Drop both setting and using $msg please.

With the above taken care of,

  Reviewed-by: Andrea Bolognani <abologna@redhat.com>

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/3] Do not check for pkcheck
Posted by Jiri Denemark, 13 weeks ago
On Wed, Mar 07, 2018 at 10:29:32 +0100, Ján Tomko wrote:
> All we need is DBus.

Unfortunately, this is wrong. From a compilation/linking POV we really
don't need anything more than D-Bus. But we polkit to actually work, we
need more. Thus we can end up enabling polkit even though it is not
actually installed, which means libvirtd will change default
authentication scheme for UNIX sockets to polkit and it will chmod the
socket to 777. Luckily, this is not a security issue because all
connections will be refused because the daemon will not be able to talk
to polkit, but it's still an unpleasant change of defaults.

Is there really nothing we could check to detect polkit presence or
should we just drop the autodetection (i.e., 'check') capability of
--with-polkit since it's mostly useless now?

Jirka

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/3] Do not check for pkcheck
Posted by Ján Tomko, 13 weeks ago
On Mon, Mar 19, 2018 at 07:47:54PM +0100, Jiri Denemark wrote:
>On Wed, Mar 07, 2018 at 10:29:32 +0100, J�n Tomko wrote:
>> All we need is DBus.
>
>Unfortunately, this is wrong. From a compilation/linking POV we really
>don't need anything more than D-Bus.

Good, we should compile as much code as we can to prevent bitrot.

>But we polkit to actually work, we
>need more. Thus we can end up enabling polkit even though it is not
>actually installed, which means libvirtd will change default
>authentication scheme for UNIX sockets to polkit and it will chmod the
>socket to 777. Luckily, this is not a security issue because all
>connections will be refused because the daemon will not be able to talk
>to polkit, but it's still an unpleasant change of defaults.
>

Same if you have polkit installed but do not bother to use it (which
is IMO more common, although a pre-existing issue).

>Is there really nothing we could check to detect polkit presence or
>should we just drop the autodetection (i.e., 'check') capability of
>--with-polkit since it's mostly useless now?
>

Since it's a runtime dependency, we could check for it at runtime like
we do for systemd, but I did not want to think about the security
implications. I can look into it if someone else is running such a
strange configuration (Peter?)

Alternatively, we could disable the option to compile out polkit,
check for pkcheck at configure time and use that only to enable it by
default.

And of course, IMO all the compile-time autodetection of runtime
dependencies is useless and should be abolished.

Jan

>Jirka
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/3] Do not check for pkcheck
Posted by Daniel P. Berrangé, 13 weeks ago
On Tue, Mar 20, 2018 at 12:27:17PM +0100, Ján Tomko wrote:
> On Mon, Mar 19, 2018 at 07:47:54PM +0100, Jiri Denemark wrote:
> > On Wed, Mar 07, 2018 at 10:29:32 +0100, Ján Tomko wrote:
> > > All we need is DBus.
> > 
> > Unfortunately, this is wrong. From a compilation/linking POV we really
> > don't need anything more than D-Bus.
> 
> Good, we should compile as much code as we can to prevent bitrot.
> 
> > But we polkit to actually work, we
> > need more. Thus we can end up enabling polkit even though it is not
> > actually installed, which means libvirtd will change default
> > authentication scheme for UNIX sockets to polkit and it will chmod the
> > socket to 777. Luckily, this is not a security issue because all
> > connections will be refused because the daemon will not be able to talk
> > to polkit, but it's still an unpleasant change of defaults.
> > 
> 
> Same if you have polkit installed but do not bother to use it (which
> is IMO more common, although a pre-existing issue).
> 
> > Is there really nothing we could check to detect polkit presence or
> > should we just drop the autodetection (i.e., 'check') capability of
> > --with-polkit since it's mostly useless now?
> > 
> 
> Since it's a runtime dependency, we could check for it at runtime like
> we do for systemd, but I did not want to think about the security
> implications. I can look into it if someone else is running such a
> strange configuration (Peter?)
> 
> Alternatively, we could disable the option to compile out polkit,
> check for pkcheck at configure time and use that only to enable it by
> default.
> 
> And of course, IMO all the compile-time autodetection of runtime
> dependencies is useless and should be abolished.

For Linux I think we should expect polkit to be enabled out of the box in
all builds. If someone really wants to disable it then they can pass
--disable-polkit still, and they also have libvirtd.conf config options
to disable it post-biuld.

We should, however, make sure that polkit is disabled when building on
OS-X / Windows / BSD, *even* if dbus is available. I think this is
something we get wrong now that we removed the check for pkcheck in
configure.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list