Re: Socket credentials (take 2)

[Elad Efrat <elad%NetBSD.org@localhost>]

Andrew Doran wrote:
> On Sat, Apr 25, 2009 at 03:40:17AM +0300, Elad Efrat wrote:
>
> > A while ago I proposed to add credentials to sockets. Looking back at
> > the thread:
> >
> >     http://mail-index.netbsd.org/tech-kern/2006/07/21/0002.html
> >
> > I see there were no objections, yet the thread just died.
>
> It leaves open questions, e.g. what should happen on accept(). The meaning
> of the credential is somewhat fuzzy.

Looking at other systems, the credentials (or uid/gids) are inherited.
Where credentials are used, a reference is held to the parent's copy.

We can probably do the same, maybe some slight modifications to kauth(9)
if we're interested (kauth_cred_fork() for sockets?) in giving
extensions that attach additional data to credentials apply their own
inheritance policy.

Other than that, we'll be very careful wrt/what we use so_cred for; but
I have no doubt we can make it all work. :)

Do you have anything else in mind that might require attention?

> > Questions:
> >   - Are there any locking considerations present today that weren't
> >     when the diff was made, or are omitted in it?
>
> +       kauth_cred_hold(l->l_proc->p_cred);
> +       so->so_cred = l->l_proc->p_cred;
>
> I know it is an old patch, but I have repeatedly pointed out that you can't
> used p_cred without locking. l_cred should be used where the operation is
> with reference to the caller.
>
> .. and if it's not in the core kernel, kauth_cred_get(), to reduce
> sensitivity to ABI changes.

To my defense I'll say that the diff was created about the same time as
lwp cached credentials ;) but of course, comment noted.

> >   - I think "struct uidinfo" is only a member of "struct socket".
> >     Perhaps, if we put credentials in a socket, we can remove it from
> >         there, remove the "ui_uid" member, and attach it to the socket's
> >         credentials using kauth_cred_setdata()?
>
> The uidinfo is used in performance critical paths.
> What would it look like if it were changed?

If it's used in performance critical paths, we might as well just leave
it there and note that ui_uid should be used for "anything" (as it's
used for hashing internally by uidinfo).

FWIW, it might look like:

        /* The subsystem registers a key for uidinfo. */
        kauth_register_key("uidinfo", &uidinfo_kauth_key);

        /* New uidinfo: */
        struct uidinfo *uidinfo = kmem_zalloc(...);
        kauth_cred_setdata(cred, uidinfo_kauth_key, uidinfo);

        /* Updating uidinfo: */
        struct uidinfo *uidinfo = kauth_cred_getdata(cred,
            uidinfo_kauth_key);
        /* ...update it... */
        kauth_cred_setdata(cred, uidinfo_kauth_key, uidinfo);

Thanks,

-e.