9:42 a.m.
On Thu, Apr 24, 2008 at 03:25:05PM +0100, Daniel P. Berrange wrote:
> --- 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.
That's right. Such a flag sounds sensible to me, I wasn't sure over the
rules of what the daemon can actually use (i.e. private interfaces to
libvirt core).
> --- 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.
I can do that, sure.
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.
But this only returns the UID right? Or are you saying there's a generic
function for "can connect"? If so, we could certainly use that. I don't
see how we could use qemudGetSocketIdentity() (though we could certainly
*implement* it).
> /* 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.
This hunk is enforcing 0666 even without Polkit on Solaris. So I can fix
the 0777 too, but we do need this hunk.
> +#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.
We don't even ship the config file, as there's nothing in it that we
want to make configurable on Solaris. We don't have the same issue that
Linux does, where it needs to define a single set of sources that
applies to whole bunch of disparate environments. We just have the
Solaris ecosystem, and it's always configured and locked down in the
same way.
When there's genuine configuration values that users need for something
enabled on Solaris, it will be in SMF anyway, so will need re-working on
the config backend in libvirt.
> +/**
> + * 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.
Could you explain further how you see this working?
> +/*
> + * Attempt to access the domain via 'xenconsole', which may have
> + * additional privilege to reach the console.
> + */
> +static int
> +vshXenConsole(int id)
Clearly this has to die.
It's not at all clear to me. virsh console is already a wart, and I
don't see the problem in a pragmatic approach here, given it only ever
tries the direct approach as a fallback.
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.
Yes. In particular it needs to run setuid root and:
- get the pts from xenstore (needs PRIV_XVM_CONTROL)
- open the pty (requires all privileges)
- drop all privileges before continuing
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.
This just adds a whole bunch of extra code to no clear advantage?
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
Sure. Just as a warning, it will probably be quite some time before I'm
able to find time to get this updated and in a mergable state.
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
I have a detailed document ready for architecture review. When it comes
up on the ARC (http://www.opensolaris.org/os/community/arc/) I'll
forward it on.
regards,
john