Re: [libvirt] [PATCH 3/3] Do not check for pkcheck
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
Re: [libvirt] [PATCH 3/3] Do not check for pkcheck
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 signature.asc Description: Digital signature -- 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
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
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
--
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
All we need is DBus. Signed-off-by: Ján Tomko --- 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
