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.
--
Simo Sorce * Red Hat, Inc * New York
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(-)
On 17/01/14 11:56, Simo Sorce wrote:
> 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 removing this code 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 <[email protected]>
Committed...
steved
> ---
> utils/gssd/krb5_util.c | 24 ++----------------------
> 1 files changed, 2 insertions(+), 22 deletions(-)
>
> diff --git a/utils/gssd/krb5_util.c b/utils/gssd/krb5_util.c
> index 697d1d2e79db0cc38160ea4772d3af3a9b7d6c21..230b909b14d244f832c0b5dd62e600cf8db4f80b 100644
> --- a/utils/gssd/krb5_util.c
> +++ b/utils/gssd/krb5_util.c
> @@ -1381,29 +1381,10 @@ gssd_acquire_krb5_cred(gss_name_t name, gss_cred_id_t *gss_cred)
> int
> 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;
> - char buf[11];
> + OM_uint32 min_stat;
> int ret;
>
> - 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;
> - }
> -
> - ret = gssd_acquire_krb5_cred(name, gss_cred);
> + ret = gssd_acquire_krb5_cred(GSS_C_NO_NAME, gss_cred);
>
> /* force validation of cred to check for expiry */
> if (ret == 0) {
> @@ -1412,7 +1393,6 @@ gssd_acquire_user_cred(uid_t uid, gss_cred_id_t *gss_cred)
> ret = -1;
> }
>
> - maj_stat = gss_release_name(&min_stat, &name);
> return ret;
> }
>
>
On Thu, 16 Jan 2014 23:11:57 -0500
Simo Sorce <[email protected]> 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 <[email protected]>
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 removing this code 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 <[email protected]>
---
utils/gssd/krb5_util.c | 22 +---------------------
1 files changed, 1 insertions(+), 21 deletions(-)
diff --git a/utils/gssd/krb5_util.c b/utils/gssd/krb5_util.c
index 697d1d2e79db0cc38160ea4772d3af3a9b7d6c21..cb64a52f3c9c6e6fdb90e007172ecd43f8e2f458 100644
--- a/utils/gssd/krb5_util.c
+++ b/utils/gssd/krb5_util.c
@@ -1382,28 +1382,9 @@ int
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;
- 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;
-
- 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);
+ ret = gssd_acquire_krb5_cred(GSS_C_NO_NAME, gss_cred);
/* force validation of cred to check for expiry */
if (ret == 0) {
@@ -1412,7 +1393,6 @@ gssd_acquire_user_cred(uid_t uid, gss_cred_id_t *gss_cred)
ret = -1;
}
- maj_stat = gss_release_name(&min_stat, &name);
return ret;
}
--
1.7.1
On Thu, 2014-01-16 at 10:47 -0500, Jeff Layton wrote:
> On Wed, 15 Jan 2014 16:41:34 -0500
> Simo Sorce <[email protected]> wrote:
>
> > From 421f66b1cd0b031ef843f7680f463027904b93ca Mon Sep 17 00:00:00 2001
> > From: Simo Sorce <[email protected]>
> > 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 <[email protected]>
> > ---
> > 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.
--
Simo Sorce * Red Hat, Inc * New York
The name variable is always set to NULL now in all callers, so just
sto passing it around needlessly.
The uid_t variable is not used at all, so chuck it out too.
Signed-off-by: Simo Sorce <[email protected]>
---
utils/gssd/gssd_proc.c | 2 +-
utils/gssd/krb5_util.c | 10 +++++-----
utils/gssd/krb5_util.h | 2 +-
3 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
index 2a6ea97135848eaf601bbed17e5b62cb77bd66b9..33cfeb2afd2ed6c6810dc39548b1a9da8daa8c37 100644
--- a/utils/gssd/gssd_proc.c
+++ b/utils/gssd/gssd_proc.c
@@ -1095,7 +1095,7 @@ process_krb5_upcall(struct clnt_info *clp, uid_t uid, int fd, char *tgtname,
/* Tell krb5 gss which credentials cache to use */
/* Try first to acquire credentials directly via GSSAPI */
- err = gssd_acquire_user_cred(uid, &gss_cred);
+ err = gssd_acquire_user_cred(&gss_cred);
if (!err)
create_resp = create_auth_rpc_client(clp, tgtname, &rpc_clnt, &auth, uid,
AUTHTYPE_KRB5, gss_cred);
diff --git a/utils/gssd/krb5_util.c b/utils/gssd/krb5_util.c
index 230b909b14d244f832c0b5dd62e600cf8db4f80b..208c72bc072bceecc81f591887603e274dc35d9b 100644
--- a/utils/gssd/krb5_util.c
+++ b/utils/gssd/krb5_util.c
@@ -1359,12 +1359,12 @@ gssd_k5_get_default_realm(char **def_realm)
}
static int
-gssd_acquire_krb5_cred(gss_name_t name, gss_cred_id_t *gss_cred)
+gssd_acquire_krb5_cred(gss_cred_id_t *gss_cred)
{
OM_uint32 maj_stat, min_stat;
gss_OID_set_desc desired_mechs = { 1, &krb5oid };
- maj_stat = gss_acquire_cred(&min_stat, name, GSS_C_INDEFINITE,
+ maj_stat = gss_acquire_cred(&min_stat, GSS_C_NO_NAME, GSS_C_INDEFINITE,
&desired_mechs, GSS_C_INITIATE,
gss_cred, NULL, NULL);
@@ -1379,12 +1379,12 @@ gssd_acquire_krb5_cred(gss_name_t name, gss_cred_id_t *gss_cred)
}
int
-gssd_acquire_user_cred(uid_t uid, gss_cred_id_t *gss_cred)
+gssd_acquire_user_cred(gss_cred_id_t *gss_cred)
{
OM_uint32 min_stat;
int ret;
- ret = gssd_acquire_krb5_cred(GSS_C_NO_NAME, gss_cred);
+ ret = gssd_acquire_krb5_cred(gss_cred);
/* force validation of cred to check for expiry */
if (ret == 0) {
@@ -1423,7 +1423,7 @@ limit_krb5_enctypes(struct rpc_gss_sec *sec)
int err = -1;
if (sec->cred == GSS_C_NO_CREDENTIAL) {
- err = gssd_acquire_krb5_cred(GSS_C_NO_NAME, &sec->cred);
+ err = gssd_acquire_krb5_cred(&sec->cred);
if (err)
return -1;
}
diff --git a/utils/gssd/krb5_util.h b/utils/gssd/krb5_util.h
index 3f0723e05cb3fd24b8063941e715010b21db0515..a319588a61253420da1ba7f62124df6b74795eea 100644
--- a/utils/gssd/krb5_util.h
+++ b/utils/gssd/krb5_util.h
@@ -35,7 +35,7 @@ int gssd_refresh_krb5_machine_credential(char *hostname,
char *gssd_k5_err_msg(krb5_context context, krb5_error_code code);
void gssd_k5_get_default_realm(char **def_realm);
-int gssd_acquire_user_cred(uid_t uid, gss_cred_id_t *gss_cred);
+int gssd_acquire_user_cred(gss_cred_id_t *gss_cred);
#ifdef HAVE_SET_ALLOWABLE_ENCTYPES
extern int limit_to_legacy_enctypes;
--
1.7.1
On Thu, 16 Jan 2014 20:28:27 -0500
Simo Sorce <[email protected]> wrote:
> On Thu, 2014-01-16 at 10:47 -0500, Jeff Layton wrote:
> > On Wed, 15 Jan 2014 16:41:34 -0500
> > Simo Sorce <[email protected]> wrote:
> >
> > > From 421f66b1cd0b031ef843f7680f463027904b93ca Mon Sep 17 00:00:00 2001
> > > From: Simo Sorce <[email protected]>
> > > 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 <[email protected]>
> > > ---
> > > 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 <[email protected]>
On 17/01/14 11:56, Simo Sorce wrote:
> The name variable is always set to NULL now in all callers, so just
> sto passing it around needlessly.
> The uid_t variable is not used at all, so chuck it out too.
>
> Signed-off-by: Simo Sorce <[email protected]>
Committed...
steved.
> ---
> utils/gssd/gssd_proc.c | 2 +-
> utils/gssd/krb5_util.c | 10 +++++-----
> utils/gssd/krb5_util.h | 2 +-
> 3 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
> index 2a6ea97135848eaf601bbed17e5b62cb77bd66b9..33cfeb2afd2ed6c6810dc39548b1a9da8daa8c37 100644
> --- a/utils/gssd/gssd_proc.c
> +++ b/utils/gssd/gssd_proc.c
> @@ -1095,7 +1095,7 @@ process_krb5_upcall(struct clnt_info *clp, uid_t uid, int fd, char *tgtname,
>
> /* Tell krb5 gss which credentials cache to use */
> /* Try first to acquire credentials directly via GSSAPI */
> - err = gssd_acquire_user_cred(uid, &gss_cred);
> + err = gssd_acquire_user_cred(&gss_cred);
> if (!err)
> create_resp = create_auth_rpc_client(clp, tgtname, &rpc_clnt, &auth, uid,
> AUTHTYPE_KRB5, gss_cred);
> diff --git a/utils/gssd/krb5_util.c b/utils/gssd/krb5_util.c
> index 230b909b14d244f832c0b5dd62e600cf8db4f80b..208c72bc072bceecc81f591887603e274dc35d9b 100644
> --- a/utils/gssd/krb5_util.c
> +++ b/utils/gssd/krb5_util.c
> @@ -1359,12 +1359,12 @@ gssd_k5_get_default_realm(char **def_realm)
> }
>
> static int
> -gssd_acquire_krb5_cred(gss_name_t name, gss_cred_id_t *gss_cred)
> +gssd_acquire_krb5_cred(gss_cred_id_t *gss_cred)
> {
> OM_uint32 maj_stat, min_stat;
> gss_OID_set_desc desired_mechs = { 1, &krb5oid };
>
> - maj_stat = gss_acquire_cred(&min_stat, name, GSS_C_INDEFINITE,
> + maj_stat = gss_acquire_cred(&min_stat, GSS_C_NO_NAME, GSS_C_INDEFINITE,
> &desired_mechs, GSS_C_INITIATE,
> gss_cred, NULL, NULL);
>
> @@ -1379,12 +1379,12 @@ gssd_acquire_krb5_cred(gss_name_t name, gss_cred_id_t *gss_cred)
> }
>
> int
> -gssd_acquire_user_cred(uid_t uid, gss_cred_id_t *gss_cred)
> +gssd_acquire_user_cred(gss_cred_id_t *gss_cred)
> {
> OM_uint32 min_stat;
> int ret;
>
> - ret = gssd_acquire_krb5_cred(GSS_C_NO_NAME, gss_cred);
> + ret = gssd_acquire_krb5_cred(gss_cred);
>
> /* force validation of cred to check for expiry */
> if (ret == 0) {
> @@ -1423,7 +1423,7 @@ limit_krb5_enctypes(struct rpc_gss_sec *sec)
> int err = -1;
>
> if (sec->cred == GSS_C_NO_CREDENTIAL) {
> - err = gssd_acquire_krb5_cred(GSS_C_NO_NAME, &sec->cred);
> + err = gssd_acquire_krb5_cred(&sec->cred);
> if (err)
> return -1;
> }
> diff --git a/utils/gssd/krb5_util.h b/utils/gssd/krb5_util.h
> index 3f0723e05cb3fd24b8063941e715010b21db0515..a319588a61253420da1ba7f62124df6b74795eea 100644
> --- a/utils/gssd/krb5_util.h
> +++ b/utils/gssd/krb5_util.h
> @@ -35,7 +35,7 @@ int gssd_refresh_krb5_machine_credential(char *hostname,
> char *gssd_k5_err_msg(krb5_context context, krb5_error_code code);
> void gssd_k5_get_default_realm(char **def_realm);
>
> -int gssd_acquire_user_cred(uid_t uid, gss_cred_id_t *gss_cred);
> +int gssd_acquire_user_cred(gss_cred_id_t *gss_cred);
>
> #ifdef HAVE_SET_ALLOWABLE_ENCTYPES
> extern int limit_to_legacy_enctypes;
>
The name variable is always set to NULL now in all callers, so just
sto passing it around needlessly.
Signed-off-by: Simo Sorce <[email protected]>
---
utils/gssd/krb5_util.c | 8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/utils/gssd/krb5_util.c b/utils/gssd/krb5_util.c
index cb64a52f3c9c6e6fdb90e007172ecd43f8e2f458..8d4b0e3593a0bd25e00d41c7d0e29322f5f55d6e 100644
--- a/utils/gssd/krb5_util.c
+++ b/utils/gssd/krb5_util.c
@@ -1359,12 +1359,12 @@ gssd_k5_get_default_realm(char **def_realm)
}
static int
-gssd_acquire_krb5_cred(gss_name_t name, gss_cred_id_t *gss_cred)
+gssd_acquire_krb5_cred(gss_cred_id_t *gss_cred)
{
OM_uint32 maj_stat, min_stat;
gss_OID_set_desc desired_mechs = { 1, &krb5oid };
- maj_stat = gss_acquire_cred(&min_stat, name, GSS_C_INDEFINITE,
+ maj_stat = gss_acquire_cred(&min_stat, GSS_C_NO_NAME, GSS_C_INDEFINITE,
&desired_mechs, GSS_C_INITIATE,
gss_cred, NULL, NULL);
@@ -1384,7 +1384,7 @@ gssd_acquire_user_cred(uid_t uid, gss_cred_id_t *gss_cred)
OM_uint32 maj_stat, min_stat;
int ret;
- ret = gssd_acquire_krb5_cred(GSS_C_NO_NAME, gss_cred);
+ ret = gssd_acquire_krb5_cred(gss_cred);
/* force validation of cred to check for expiry */
if (ret == 0) {
@@ -1423,7 +1423,7 @@ limit_krb5_enctypes(struct rpc_gss_sec *sec)
int err = -1;
if (sec->cred == GSS_C_NO_CREDENTIAL) {
- err = gssd_acquire_krb5_cred(GSS_C_NO_NAME, &sec->cred);
+ err = gssd_acquire_krb5_cred(&sec->cred);
if (err)
return -1;
}
--
1.7.1
On Wed, 15 Jan 2014 16:41:34 -0500
Simo Sorce <[email protected]> wrote:
> From 421f66b1cd0b031ef843f7680f463027904b93ca Mon Sep 17 00:00:00 2001
> From: Simo Sorce <[email protected]>
> 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 <[email protected]>
> ---
> 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.
--
Jeff Layton <[email protected]>
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 removing this code 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 <[email protected]>
---
utils/gssd/krb5_util.c | 24 ++----------------------
1 files changed, 2 insertions(+), 22 deletions(-)
diff --git a/utils/gssd/krb5_util.c b/utils/gssd/krb5_util.c
index 697d1d2e79db0cc38160ea4772d3af3a9b7d6c21..230b909b14d244f832c0b5dd62e600cf8db4f80b 100644
--- a/utils/gssd/krb5_util.c
+++ b/utils/gssd/krb5_util.c
@@ -1381,29 +1381,10 @@ gssd_acquire_krb5_cred(gss_name_t name, gss_cred_id_t *gss_cred)
int
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;
- char buf[11];
+ OM_uint32 min_stat;
int ret;
- 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;
- }
-
- ret = gssd_acquire_krb5_cred(name, gss_cred);
+ ret = gssd_acquire_krb5_cred(GSS_C_NO_NAME, gss_cred);
/* force validation of cred to check for expiry */
if (ret == 0) {
@@ -1412,7 +1393,6 @@ gssd_acquire_user_cred(uid_t uid, gss_cred_id_t *gss_cred)
ret = -1;
}
- maj_stat = gss_release_name(&min_stat, &name);
return ret;
}
--
1.7.1
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(-)