diff mbox

Fix crdential sourcing with new setuid behavior in rpc.gssd

Message ID 1389822094.26102.488.camel@willson.li.ssimo.org (mailing list archive)
State New, archived
Headers show

Commit Message

Simo Sorce Jan. 15, 2014, 9:41 p.m. UTC 
The attached patch makes some edge cases work where the name returned by
getpwnam() does not match the data in the credential cache without
having to fall back to trolling the ccaches on the file systems.

It also allow KEYRING (or any other future type) based ccaches work
which otherwise don't as there is not code for trolling any other ccache
type then FILE or DIR.

[Found testing RHEL7 beta with keyring caches into a FreeIPA/RH-Idm
Domain trusting a Windows AD Domain, with a Windows user logged into the
Linux client.]

Simo.

Comments

Jeff Layton Jan. 16, 2014, 3:47 p.m. UTC | #1 
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...

> -	maj_stat = gss_import_name(&min_stat, &name_buf,
> -				   GSS_C_NT_STRING_UID_NAME, &name);
> -	if (maj_stat != GSS_S_COMPLETE) {
> -		if (get_verbosity() > 0)
> -			pgsserr("gss_import_name",
> -				maj_stat, min_stat, &krb5oid);
> -		return -1;
> +		maj_stat = gss_import_name(&min_stat, &name_buf,
> +					   GSS_C_NT_STRING_UID_NAME, &name);
> +		if (maj_stat != GSS_S_COMPLETE) {
> +			if (get_verbosity() > 0)
> +				pgsserr("gss_import_name",
> +					maj_stat, min_stat, &krb5oid);
> +			return -1;
> +		}
>  	}
>  
>  	ret = gssd_acquire_krb5_cred(name, gss_cred);


Other than that, I'm fine with ripping that junk out.
Simo Sorce Jan. 17, 2014, 1:28 a.m. UTC | #2 
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.
Jeff Layton Jan. 17, 2014, 1:49 a.m. UTC | #3 
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,
Simo Sorce Jan. 17, 2014, 4:11 a.m. UTC | #4 
Take 2

Patch 1: simply remove the old code that is not necessary anymore

Bonus patch 2: clean up code, name is not needed anymore as the default
credentials for the impersonated uid are always used.

Simo Sorce (2):
  Improve first attempt at acquiring GSS credentials
  Remove unused parameter

 utils/gssd/krb5_util.c |   28 ++++------------------------
 1 files changed, 4 insertions(+), 24 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jeff Layton Jan. 17, 2014, 11:54 a.m. UTC | #5 
On Thu, 16 Jan 2014 23:11:57 -0500
Simo Sorce <simo@redhat.com> wrote:

> Take 2
> 
> Patch 1: simply remove the old code that is not necessary anymore
> 
> Bonus patch 2: clean up code, name is not needed anymore as the default
> credentials for the impersonated uid are always used.
> 
> Simo Sorce (2):
>   Improve first attempt at acquiring GSS credentials
>   Remove unused parameter
> 
>  utils/gssd/krb5_util.c |   28 ++++------------------------
>  1 files changed, 4 insertions(+), 24 deletions(-)
> 

LGTM

Acked-by: Jeff Layton <jlayton@redhat.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Simo Sorce Jan. 17, 2014, 4:56 p.m. UTC | #6 
Take 3

Patch 1:
 - simply remove the old code that is not necessary anymore
 * removed an additional unused variable in take 3

Bonus patch 2:
 - clean up code, name is not needed anymore as the default
   credentials for the impersonated uid are always used.
 * also stopped passing uid needlessly around in take 3
   Pointed out by Steve, thanks.

Simo Sorce (2):
  Improve first attempt at acquiring GSS credentials
  Remove unused arguments

 utils/gssd/gssd_proc.c |    2 +-
 utils/gssd/krb5_util.c |   32 ++++++--------------------------
 utils/gssd/krb5_util.h |    2 +-
 3 files changed, 8 insertions(+), 28 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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;
 
-	maj_stat = gss_import_name(&min_stat, &name_buf,
-				   GSS_C_NT_STRING_UID_NAME, &name);
-	if (maj_stat != GSS_S_COMPLETE) {
-		if (get_verbosity() > 0)
-			pgsserr("gss_import_name",
-				maj_stat, min_stat, &krb5oid);
-		return -1;
+		maj_stat = gss_import_name(&min_stat, &name_buf,
+					   GSS_C_NT_STRING_UID_NAME, &name);
+		if (maj_stat != GSS_S_COMPLETE) {
+			if (get_verbosity() > 0)
+				pgsserr("gss_import_name",
+					maj_stat, min_stat, &krb5oid);
+			return -1;
+		}
 	}
 
 	ret = gssd_acquire_krb5_cred(name, gss_cred);
-- 
1.8.4.2