Date: Thu, 16 Jan 2014 20:49:49 -0500
From: Jeff Layton <jlayton@redhat.com>
To: Simo Sorce <simo@redhat.com>
Cc: Steve Dickson <SteveD@redhat.com>, linux-nfs <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH] Fix crdential sourcing with new setuid behavior in
 rpc.gssd
Message-ID: <20140116204949.1bea663a@corrin.poochiereds.net>
In-Reply-To: <1389922107.2370.13.camel@willson.li.ssimo.org>
References: <1389822094.26102.488.camel@willson.li.ssimo.org>
	<20140116104707.6b4c11fe@tlielax.poochiereds.net>
	<1389922107.2370.13.camel@willson.li.ssimo.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Sender: linux-nfs-owner@vger.kernel.org

On Thu, 16 Jan 2014 20:28:27 -0500
Simo Sorce <simo@redhat.com> 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@redhat.com> wrote:
> > 
> > > From 421f66b1cd0b031ef843f7680f463027904b93ca Mon Sep 17 00:00:00 2001
> > > From: Simo Sorce <simo@redhat.com>
> > > 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@redhat.com>
> > > ---
> > >  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@redhat.com>