On Thu, 16 Jan 2014 20:28:27 -0500
Simo Sorce <simo@xxxxxxxxxx> wrote:
> On Thu, 2014-01-16 at 10:47 -0500, Jeff Layton wrote:
> > On Wed, 15 Jan 2014 16:41:34 -0500
> > Simo Sorce <simo@xxxxxxxxxx> wrote:
> >
> > > From 421f66b1cd0b031ef843f7680f463027904b93ca Mon Sep 17 00:00:00 2001
> > > From: Simo Sorce <simo@xxxxxxxxxx>
> > > Date: Wed, 15 Jan 2014 16:01:49 -0500
> > > Subject: [PATCH] Improve first attempt at acquiring GSS credentials
> > >
> > > Since now rpc.gssd is swithing uid before attempting to acquire
> > > credentials, we do not need to pass in the special uid-as-a-string name
> > > to gssapi, because the process is already running under the user's
> > > credentials.
> > >
> > > By making this optional we can fix a class of false negatives where the
> > > user name does not match the actual ccache credentials and the ccache
> > > type used is not one of the only 2 supported explicitly by rpc.gssd by the
> > > fallback trolling done later.
> > >
> > > Signed-off-by: Simo Sorce <simo@xxxxxxxxxx>
> > > ---
> > > utils/gssd/krb5_util.c | 32 ++++++++++++++++++--------------
> > > 1 file changed, 18 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/utils/gssd/krb5_util.c b/utils/gssd/krb5_util.c
> > > index 697d1d2e79db0cc38160ea4772d3af3a9b7d6c21..7db5baf4e4bea75ed7beebd2103afbc291efb641 100644
> > > --- a/utils/gssd/krb5_util.c
> > > +++ b/utils/gssd/krb5_util.c
> > > @@ -1383,24 +1383,28 @@ gssd_acquire_user_cred(uid_t uid, gss_cred_id_t *gss_cred)
> > > {
> > > OM_uint32 maj_stat, min_stat;
> > > gss_buffer_desc name_buf;
> > > - gss_name_t name;
> > > + gss_name_t name = GSS_C_NO_NAME;
> > > char buf[11];
> > > int ret;
> > >
> > > - ret = snprintf(buf, 11, "%u", uid);
> > > - if (ret < 1 || ret > 10) {
> > > - return -1;
> > > - }
> > > - name_buf.value = buf;
> > > - name_buf.length = ret + 1;
> > > + /* the follwing is useful only if change_identity() in
> > > + * process_krb5_upcall() failed to change uids */
> > > + if (getuid() == 0) {
> > > + ret = snprintf(buf, 11, "%u", uid);
> > > + if (ret < 1 || ret > 10) {
> > > + return -1;
> > > + }
> > > + name_buf.value = buf;
> > > + name_buf.length = ret + 1;
> > >
> >
> > If change_identity() fails, then process_krb5_upcall should just give
> > up and do an error downcall, so falling back to using
> > GSS_C_NT_STRING_UID_NAME in that case seems unnecessary.
> >
> > Also, we can end up in here legitimately with uid == 0 if
> > root_uses_machine_creds == 0. So I wonder if we even need the stuff
> > inside this "if (getuid() == 0)" block at all...
>
> I were under the impression that rpc.gssd could still be used without
> doing the fork()/setuid() dance, and I didn't really check if it really
> is conditional.
>
> If it is not and the only case where uid = 0 is when rpc.gssd is
> actually performing the operation on behalf of root, then yeah we can
> simply remove everything in the if branch.
>
> Let me know how you want to proceed.
>
> > Other than that, I'm fine with ripping that junk out.
>
> Ok, so should I sent a patch that just removes, instead of making
> conditional, this chunk of code ?
>
> Simo.
>
Yeah, the fork isn't conditional. It's always done when processing an
upcall now. I think we can just get rid of the if block altogether.
Mind sending a v2 patch that does that?
Thanks,
--
Jeff Layton <jlayton@xxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
