On Wed, Aug 08, 2007 at 05:22:33AM +0100, Daniel P. Berrange wrote:
> In summary what I really need for virt-manager is
>
> - Always run as non-root
> - Authenticate for local guest management (ie read+write)
Okay, problem understood.
> - libvirtd defines two actions it can check called 'libvirt-local-monitor
> (read only monitoring of state), and 'libvirt-local-manage' (full
> read-write management).
Would the model allow things like being able to manage domains run by the
user, but only monitor domains from someone else. I understand it may not
always be possible to store that information, but if available a finer grained
control based on context would make sense. Things like PolicyKit are being added
among other things for better user switching mechanism, and the domains run
by an user are in my opinion an extension of the user session, and having
per user control would make sense IMHO.
> - Both libvirt-sock and libvirt-sock-ro are mode 0777 (ie all users)
>
> - A connection arrives on the UNIX domain socket either libvirt-sock-ro
> or libvirt-sock
>
> - libvirtd use SO_PEERCRED to get the PID of the client
I'm just wondering a bit about the portability, I remember that for gamin
we had to use at least 2 different authentication mechanism to assert reliably
the identity of the connecting process. Might be worth looking again at
D-Bus code for this.
> - If the connection is on libvirt-sock-ro 'action' is 'libvirt-local-monitor'
> Else if connection is on libvirt-sock 'action' is 'libvirt-local-manage'
>
> - Libvirt asks policy kit if PID is allowed to perform the 'action'
> -> Yes, connection proceeds
> -> No, connection is dropped
>
> NB, having two separate UNIX domain sockets is strictly speaking redundant
> now - the daemon could simply check each 'action' in turn - but keeping the
> separate sockets simplifies the code, because it minimises the diffs when
> PolicyKit is not enabled by the 'configure' script.
Okay, we clearly need to go progressively as PolicyKit being new, we can't
rely on it being there, it may take years to propagate to most of our users.
> How is policy determined by PolicyKit ?
>
> - libvirt RPM provides /usr/share/PolicyKit/policy/libvirt.policy
>
> - This file defines the two actions it supports 'libvirt-local-monitor'
> and 'libvirt-local-manage'.
>
> - For each action we define a default policy for an active desktop
> session, and an inactive desktop session. The sample policy is to
> allow 'libvirt-local-monitor' for any local desktop session, but
> only allow 'libvirt-local-manage' for the active session with the
> additional requirement that the user authenticate with PolicyKit
> using the root password.
>
> This basically replicates the existing security semantics, without requiring
> that we run virt-manager itself as root. This accomplishes the core goal.
Okay, sounds like a good first step,
> If course this doesn't just apply to virt-manager either. This transparently
> 'just works' for virsh too & any other apps using libvirt - eg if policy
> grants 'libvirt-local-manage' then you can do full management whether virsh
> is root or not.
yeah it's very important that those be dealt at the libvirt level, in a
sense the elevation of priviledge needed for virt-manager and the proxy are
just a bad way to get around a lack of finer grained mechanism.
> Finally, PolicyKit is new in Fedora 8 if you wish to try this out. The
[...]
> Oh, finally, finally, finally. PolicyKit is pretty new so I'd understand
> if there's some resistance to including it in an official libvirt release
> at this point. I think the patch is small enough that we could carry it
> as add-on in the Fedora RPMs only to start with untill its proved to be
> maintainable long term - I really need this for Fedora 8 to make virt-manager
> work sanely - ie not need to run as root.
To me it's not a problem as long as users without PolicyKit don't see a
regression. The idea of removing the suid binary of the proxy is to me
sufficient to push it as the default mechanism when DBus/PolicyKit is
available. We just need to be careful, but that doesn't mean being
conservative.
In a nutshell, this sounds cool, I wonder how much finer grained we
can go with PolicyKit and as long as we don't make it a requirement it's
fine to add to the default code base IMHO.
>
> +enum libvirtd_auth {
> + LIBVIRTD_AUTH_NONE,
> + LIBVIRTD_AUTH_TLS,
> +#ifdef HAVE_POLICY_KIT
> + LIBVIRTD_AUTH_POLICYKIT,
> +#endif
> +};
Hum, I would keep the enum present even if it's not detected, the
value won't be used but it should still be defined IMHO.
> void qemudLog(int priority, const char *fmt, ...)
> Index: qemud/libvirtd.policy
> ===================================================================
> RCS file: qemud/libvirtd.policy
> diff -N qemud/libvirtd.policy
> --- /dev/null 1 Jan 1970 00:00:00 -0000
> +++ qemud/libvirtd.policy 8 Aug 2007 04:10:45 -0000
> @@ -0,0 +1,41 @@
> +<!DOCTYPE policyconfig PUBLIC
> + "-//freedesktop//DTD PolicyKit Policy Configuration 1.0//EN"
> + "http://www.freedesktop.org/standards/PolicyKit/1.0/policyconfig.dtd";>
> +
> +<!--
> +Policy definitions for libvirt daemon
> +
> +Copyright (c) 2007 Daniel P. Berrange <berrange@xxxxxxxxxx>
> +
> +libvirt is licensed to you under the GNU Lesser General Public License
> +version 2. See COPYING for details.
> +
> +NOTE: If you make changes to this file, make sure to validate the file
> +using the polkit-policy-file-validate(1) tool. Changes made to this
> +file are instantly applied.
> +-->
> +
> +<policyconfig>
> + <group id="libvirtd">
> + <description>Virtualization</description>
> +
> + <policy id="libvirtd-local-monitor">
> + <description>Monitor local virtualized systems</description>
> + <message>System policy prevents monitoring local virtualized systems</message>
> + <defaults>
> + <allow_inactive>yes</allow_inactive>
> + <allow_active>yes</allow_active>
> + </defaults>
> + </policy>
> +
> + <policy id="libvirtd-local-manage">
> + <description>Manage local virtualized systems</description>
> + <message>System policy prevents managing local virtualized systems</message>
> + <defaults>
> + <allow_inactive>no</allow_inactive>
> + <allow_active>auth_admin</allow_active>
> + </defaults>
> + </policy>
> +
> + </group>
> +</policyconfig>
Cool, more XML, we should actually check that PolicyKit install the
http://www.freedesktop.org/standards/PolicyKit/1.0/policyconfig.dtd DTD in
the XML catalog. I need to install F-8 test1 somewhere !
Okay here is the actual new authentication code,
> - if (!client->tls) {
> + switch (client->auth) {
> +#ifdef HAVE_POLICY_KIT
> + case LIBVIRTD_AUTH_POLICYKIT:
I would keep the case out, add an error if HAVE_POLICY_KIT is not #defined
which should not happen but...
> + {
> + PolKitCaller *pkcaller = NULL;
> + PolKitAction *pkaction = NULL;
> + PolKitContext *pkcontext = NULL;
> + PolKitError *pkerr;
> + PolKitResult pkresult;
> + const char *action = sock->readonly ?
> + "libvirtd-local-monitor" :
> + "libvirtd-local-manage";
> + pid_t callerPid;
> + DBusError err;
> +#ifdef SO_PEERCRED
> + struct ucred cr;
> + unsigned int cr_len = sizeof (cr);
> +
> + if (getsockopt (fd, SOL_SOCKET, SO_PEERCRED, &cr, &cr_len) < 0) {
> + qemudLog(QEMUD_ERR, "Failed to verify client credentials: %s", strerror(errno));
> + close(fd);
> + free(client);
> + return -1;
> + }
> + callerPid = cr.pid;
> +#else
> + /* XXX Many more OS support UNIX socket credentials we could port to ....*/
We should match with DBus code. If PolicyKit is defined, then that mean
DBus is ported there and DBus do various socket creds checks last time I looked
like CMSGCRED (but that's not as easy to use as SO_PEERCRED clearly).
> +#error "UNIX socket credentials not supported/implemeneted on this platform"
> +#endif
> +
> + dbus_error_init(&err);
> + if (!(pkcaller = polkit_caller_new_from_pid(server->sysbus, callerPid, &err))) {
> + qemudLog(QEMUD_ERR, "Failed to lookup policy kit caller: %s", err.message);
> + dbus_error_free(&err);
> + close(fd);
> + free(client);
> + return -1;
> + }
> +
> + if (!(pkaction = polkit_action_new())) {
> + qemudLog(QEMUD_ERR, "Failed to create polkit action %s\n", strerror(errno));
> + polkit_caller_unref(pkcaller);
> + close(fd);
> + free(client);
> + return -1;
> + }
> + polkit_action_set_action_id(pkaction, action);
> +
> + if (!(pkcontext = polkit_context_new()) ||
> + !polkit_context_init(pkcontext, &pkerr)) {
> + qemudLog(QEMUD_ERR, "Failed to create polkit context %s\n",
> + pkerr ? polkit_error_get_error_message(pkerr) : strerror(errno));
> + if (pkerr)
> + polkit_error_free(pkerr);
> + polkit_caller_unref(pkcaller);
> + polkit_action_unref(pkaction);
> + dbus_error_free(&err);
> + close(fd);
> + free(client);
> + return -1;
> + }
> +
> + pkresult = polkit_context_can_caller_do_action(pkcontext, pkaction, pkcaller);
> + polkit_context_unref(pkcontext);
> + polkit_caller_unref(pkcaller);
> + polkit_action_unref(pkaction);
> + if (pkresult != POLKIT_RESULT_YES) {
> + qemudLog(QEMUD_ERR, "Policy kit denied action %s from pid %d, result: %s\n",
> + action, callerPid, polkit_result_to_string_representation(pkresult));
> + close(fd);
> + free(client);
> + return -1;
> + }
> + }
> + /* Allowed by policy kit, so fallthrough to generic setup... */
> +#endif
The availability of the uid can be derived from the pid and even in the
absence of PolicyKit I think we could embbed the same kind of policies as
we do now and enforce them without relying on the filesystem attributes.
One improvement I would like to see is to fallback to abstract sockets,
not mapped on the filesystem, if we don't need the filesystem attributes
to implement the policy, I find cleaner and safer to not map in the fs
anymore (since libvirtd and libvirt are to be upgraded together I think
the disruption would be rather minimal).
Most of the other changes seems related to the tls->auth field change
and looks fine.
>
> dnl Allow to build without Xen, QEMU/KVM, test or remote driver
> AC_ARG_WITH(xen,
> -[ --with-xen add XEN support (on)])
> +[ --with-xen add XEN support (on)],[],[with_xen=yes])
> AC_ARG_WITH(qemu,
> -[ --with-qemu add QEMU/KVM support (on)])
> +[ --with-qemu add QEMU/KVM support (on)],[],[with_qemu=yes])
> AC_ARG_WITH(openvz,
> -[ --with-openvz add OpenVZ support (off)])
> +[ --with-openvz add OpenVZ support (off)],[],[with_openvz=no])
> AC_ARG_WITH(test,
> -[ --with-test add test driver support (on)])
> +[ --with-test add test driver support (on)],[],[with_test=yes])
> AC_ARG_WITH(remote,
> -[ --with-remote add remote driver support (on)])
> +[ --with-remote add remote driver support (on)],[],[with_remote=yes])
>
> dnl
> dnl specific tests to setup DV devel environments with debug etc ...
> @@ -95,7 +97,7 @@ AC_SUBST(STATIC_BINARIES)
> dnl --enable-debug=(yes|no)
> AC_ARG_ENABLE(debug,
> AC_HELP_STRING([--enable-debug=no/yes],
> - [enable debugging output]))
> + [enable debugging output]),[],[enable_debug=no])
> if test x"$enable_debug" = x"yes"; then
> AC_DEFINE(ENABLE_DEBUG, [], [whether debugging is enabled])
> fi
Hum, that's cleanups, commit separately this doesn't need to wait :-)
[...]
> +
> +echo
> +echo "Drivers:"
> +echo " Xen: $with_xen"
> +echo " QEMU: $with_qemu"
> +echo " Test: $with_test"
> +echo " OpenVZ: $with_openvz"
> +echo " Remote: $with_remote"
> +echo
> +echo "General features:"
> +echo " Debug: $enable_debug"
> +if test "$with_remote" = "yes"; then
> + echo
> + echo "Remote driver features:"
and that too :-)
> + echo " PolicyKit: $with_policykit"
> +fi
In general this looks fairly cool to me. This certainly need also:
- documentation
- update of the spec file
before being commited, but it seems this should not impact users not on F8
testing environment, and if it was the case it's best to detect it earlier
than later.
I still have some open questions about finer grained policied based on
UID and owner of the domain when this could be asserted by libvirt. And
if we can do a cleanup of the socket and unmap them from the filesystem
in the process I would find this a good added property.
Sounds excellent to me !
Daniel
--
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard | virtualization library http://libvirt.org/
veillard@xxxxxxxxxx | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list