[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]