On Thu, Apr 24, 2008 at 01:48:08PM +0100, John Levon wrote:
>
> In the interests of giving a 'heads-up' I'm posting this patch. It
> implements least-privilege on Solaris. The basic idea is that all
> libvirt clients are forced to go through libvirtd, which verifies a
> particular privilege. virtd itself runs with enough privilege to
> interact with Xen.
This is interesting work. Work around privilege levels / security is
probably one of the most interesting & important aspects for future
libvirt development. On Linux obviously this likely involves SELinux
and its interesting to see that Solaris is also working on a MAC
based on the FLASK model. Any work we can do in the realm of privilege
separation using existing UNIX apis is also useful.
> --- libvirt-0.4.0/qemud/remote.c 2007-12-12 05:30:49.000000000 -0800
> +++ libvirt-new/qemud/remote.c 2008-04-10 12:52:18.059618661 -0700
> @@ -434,6 +434,15 @@ remoteDispatchOpen (struct qemud_server
> flags = args->flags;
> if (client->readonly) flags |= VIR_CONNECT_RO;
>
> +#ifdef __sun
> + /*
> + * On Solaris, all clients are forced to go via virtd. As a result,
> + * virtd must indicate it really does want to connect to the
> + * hypervisor.
> + */
> + name = "xen:///";
> +#endif
> +
I'm pretty sure we can come up with a way todo this without having to
have the #ifdef __sun here. AFAICT, you're trying to stop the remote
driver in the daemon re-entrantly calling back into the daemon. If
so, then we can make this more generic, by just having a global flag
to disable the remote driver entirely when inside the daemon.
> --- libvirt-0.4.0/src/libvirt.c 2007-12-17 13:51:09.000000000 -0800
> +++ libvirt-new/src/libvirt.c 2008-04-16 08:46:28.767087199 -0700
> @@ -34,6 +34,7 @@
>
> #include "uuid.h"
> #include "test.h"
> +#include "xen_internal.h"
> #include "xen_unified.h"
> #include "remote_internal.h"
> #include "qemu_driver.h"
> @@ -202,8 +203,16 @@ virInitialize(void)
> if (qemudRegister() == -1) return -1;
> #endif
> #ifdef WITH_XEN
> + /*
> + * On Solaris, only initialize Xen if we're libvirtd.
> + */
> +#ifdef __sun
> + if (geteuid() != 0 && xenHavePrivilege() &&
> + xenUnifiedRegister () == -1) return -1;
> +#else
> if (xenUnifiedRegister () == -1) return -1;
> #endif
> +#endif
As Daniel suggests, we should write some kind of generic helper function
in the util.c file, so we can isolate the #ifdef __sun in a single place
rather than repeating it.
> --- libvirt-0.4.0/qemud/qemud.c 2007-12-12 05:30:49.000000000 -0800
> +++ libvirt-new/qemud/qemud.c 2008-04-17 17:16:47.075258251 -0700
> @@ -60,6 +60,25 @@
> #include "mdns.h"
> #endif
>
> +#ifdef __sun
> +#include <ucred.h>
> +#include <priv.h>
> +
> +#ifndef PRIV_VIRT_MANAGE
> +#define PRIV_VIRT_MANAGE ((const char *)"virt_manage")
> +#endif
> +
> +#ifndef PRIV_XVM_CONTROL
> +#define PRIV_XVM_CONTROL ((const char *)"xvm_control")
> +#endif
> +
> +#define PU_RESETGROUPS 0x0001 /* Remove supplemental groups */
> +#define PU_CLEARLIMITSET 0x0008 /* L=0 */
> +
> +extern int __init_daemon_priv(int, uid_t, gid_t, ...);
> +
> +#endif
> +
> static int godaemon = 0; /* -d: Be a daemon */
> static int verbose = 0; /* -v: Verbose mode */
> static int timeout = -1; /* -t: Shutdown timeout */
> @@ -668,11 +687,13 @@ static int qemudInitPaths(struct qemud_s
>
> unlink(sockname);
>
> +#ifndef __sun
> if (snprintf (roSockname, maxlen, "%s/run/libvirt/libvirt-sock-ro",
> LOCAL_STATE_DIR) >= maxlen)
> goto snprintf_error;
>
> unlink(roSockname);
> +#endif
>
> if (snprintf(server->logDir, PATH_MAX, "%s/log/libvirt/", LOCAL_STATE_DIR) >= PATH_MAX)
> goto snprintf_error;
> @@ -1033,6 +1054,29 @@ static int qemudDispatchServer(struct qe
> return -1;
> }
>
> +#ifdef __sun
> + {
> + ucred_t *ucred = NULL;
> + const priv_set_t *privs;
> +
> + if (getpeerucred (fd, &ucred) == -1 ||
> + (privs = ucred_getprivset (ucred, PRIV_EFFECTIVE)) == NULL) {
> + if (ucred != NULL)
> + ucred_free (ucred);
> + close (fd);
> + return -1;
> + }
> +
> + if (!priv_ismember (privs, PRIV_VIRT_MANAGE)) {
> + ucred_free (ucred);
> + close (fd);
> + return -1;
> + }
> +
> + ucred_free (ucred);
> + }
> +#endif /* __sun */
> +
After the 0.4.0 release I refactored the damon code to have a generic function for
getting socket peer credentials. So we can move this code into the qemud.c file
in the qemudGetSocketIdentity() method. Its good to have a Solaris impl for this
API.
> /* Disable Nagle. Unix sockets will ignore this. */
> setsockopt (fd, IPPROTO_TCP, TCP_NODELAY, (void *)&no_slow_start,
> sizeof no_slow_start);
> @@ -1864,6 +1908,10 @@ remoteReadConfigFile (struct qemud_serve
> if (auth_unix_rw == REMOTE_AUTH_POLKIT)
> unix_sock_rw_mask = 0777;
> #endif
> +#ifdef __sun
> + unix_sock_rw_mask = 0666;
> +#endif
> +
We can probably use 0666 even on Linux - there's no compelling reason why we
need to have 0777 with execute permission - read & write should be sufficient
I believe.
> +#ifdef __sun
> +static void
> +qemudSetupPrivs (struct qemud_server *server)
> +{
> + chown ("/var/run/libvirt", 60, 60);
> + chmod ("/var/run/libvirt", 0755);
> + chown ("/var/run/libvirt/libvirt-sock", 60, 60);
> + chmod ("/var/run/libvirt/libvirt-sock", 0666);
> + chown (server->logDir, 60, 60);
The thing that concerns me here, is that we're getting a bit of a disconnect
between the config file and the impl. The user can already set these things
via the cofnig file, so hardcoding specific values is not so pleasant. I'm
wondering if its practical to have this privilege separation stuff be toggled
via a config file setting and avoid hardcoding this soo much.
> +/**
> + * xenHavePrivilege()
> + *
> + * Return true if the current process should be able to connect to Xen.
> + */
> +int
> +xenHavePrivilege()
> +{
> +#ifdef __sun
> + return priv_ineffect (PRIV_XVM_CONTROL);
> +#else
> + return getuid () == 0;
> +#endif
> +}
> +
As mentioned earlier, we probably want to move this into the util.c file
and have the privilege name passed in as a parameter.
> +/*
> + * Attempt to access the domain via 'xenconsole', which may have
> + * additional privilege to reach the console.
> + */
> +static int
> +vshXenConsole(int id)
> +{
> + char arg[100];
> + char *argv[3];
> + pid_t pid;
> +
> + if ((pid = fork()) < 0) {
> + return -1;
> + } else if (pid) {
> + int wstat;
> +
> + if (waitpid(pid, &wstat, 0) < 0)
> + return -1;
> +
> + if (WIFSIGNALED(wstat))
> + return -1;
> + if (WIFEXITED(wstat) && WEXITSTATUS(wstat))
> + return -1;
> +
> + return 1;
> + }
> +
> + /* child */
> +
> + if (snprintf(arg, 100, "%d", id) < 0)
> + return -1;
> +
> + argv[0] = "/usr/lib/xen/bin/xenconsole";
> + argv[1] = arg;
> + argv[2] = NULL;
> +
> + if (execv("/usr/lib/xen/bin/xenconsole", argv))
Clearly this has to die. The virsh console command implements exactly
the same functionality as xenconsole without being xen specific.
I'm guessing you're doing this because you need the console in a
separate process from virsh, so it can run with different privileges.
If so, we should probably split 'virsh console' out into a separate
standlone binary/command 'virt-console'. This will let you get the
privilege separation without relying on Xen specific commands.
So in summary, this is interesting work. I've no objections to the general
principle of what the patch is trying to achieve - we'll just need to
iterate over it a few times to try and better isolate the solaris specific
#ifdefs and/or make them more generically applicable and avoid the xen
specific console bits.
It'd be useful for other people's understanding if there was a short doc
giving the 1000ft overview of the different components and their relative
privileges
Regards,
Dan.
--
|: Red Hat, Engineering, Boston -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list