[Libvir] [RFC PATCH] Solaris least privilege

Thu Apr 24 14:42:35 UTC 2008

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