[libvirt] [PATCH 0/3] Drop old Policy-Kit support
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
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
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
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
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
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
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
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
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
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 :|
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 :|
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.
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 :|
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
participants (6)
-
Andrea Bolognani -
Daniel P. Berrangé -
Jim Fehlig -
Jiri Denemark -
Ján Tomko -
Peter Krempa