Re: [libvirt] [PATCH] Solaris least privilege support
On Thu, Jan 15, 2009 at 10:19:58AM +0000, Daniel P. Berrange wrote:
> +#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);
Can move the ucred_free up before priv_ismember() call and thus
avoid the need for the call in the cleanup path.
Nope, privs points into the ucred structure.
Is the chmod of the socket really required for solaris ? We already
I'll double check these.
Also, if this libvirtd is running as UID 60, so the chown really
needed ? We also setgid before creating the socket so that it
gets desired group ownership at time of creation, rather than
having to change it post-create.
At this point in the code (I think) we're still root.
This would appear to make it try to change the socket ownership
and permissions, before we've actually created the socket, which
is much later in the main() method where we call NetworkInit
Hmm, let me re-check.
> +#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
This should not be neccessary if the client end + drivers are
correctly written.
Can you explain a bit more? Why don't we need to rewrite the URI as xen?
If you want Xen to always go via the demon, the only change that
should
be required, is to make xenUnifiedOpen() return VIR_DRV_OPEN_DECLINED.
Hmm, yes you might be right. Let me experiment.
> if (err == 0) {
> - error (in_open ? NULL : conn,
> - VIR_ERR_RPC, _("socket closed unexpectedly"));
> + DEBUG("conn %p: socket closed unexpectedly", conn);
> return -1;
> }
> }
These two I/O methods here have been completely re-written in my
thread patches. Why is removing the error messages required ?
If we try to connect w/o privilege, then the socket is closed straight
after accept() - so it's not longer an RPC error for this to happen.
> + /*
> + * If the connection over a socket failed abruptly, it's probably
> + * due to not having the right privileges.
> + */
> + if (sigpipe)
> + vshError(ctl, TRUE, _("failed to connect (insufficient
privileges?)"));
> +
It will also be seen if the daemon drops the connection due to an
OOM condition, or the max-clients limit being exceeded, so perhaps
a little more detailed message.
Suggestions?
thanks
john