All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/5] fix nfs server handling of principal names
@ 2015-11-24 17:18 J. Bruce Fields
  2015-11-24 17:18 ` [PATCH 1/5] svcrpc: move some initialization to common code J. Bruce Fields
                   ` (4 more replies)
  0 siblings, 5 replies; 8+ messages in thread
From: J. Bruce Fields @ 2015-11-24 17:18 UTC (permalink / raw)
  To: linux-nfs; +Cc: J. Bruce Fields

From: "J. Bruce Fields" <bfields@redhat.com>

Thanks to James Ralston for helping debug a problem with the server's
handling of kerberos principal names, which could cause 4.1 mount
failures when using Active Directory.

Along the way I noticed another bug and some opportunities for minor
cleanup.

--b.

J. Bruce Fields (5):
  svcrpc: move some initialization to common code
  nfsd: helper for dup of possibly NULL string
  nfsd: minor consolidation of mach_cred handling code
  nfsd: fix unlikely NULL deref in mach_creds_match
  nfsd4: fix gss-proxy 4.1 mounts for some AD principals

 fs/nfsd/nfs4state.c                  | 55 +++++++++++++++++++++++++++---------
 include/linux/sunrpc/svcauth.h       |  9 +++++-
 net/sunrpc/auth_gss/gss_rpc_upcall.c |  3 ++
 net/sunrpc/svcauth.c                 |  2 ++
 net/sunrpc/svcauth_unix.c            |  8 ------
 5 files changed, 55 insertions(+), 22 deletions(-)

-- 
2.5.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 1/5] svcrpc: move some initialization to common code
  2015-11-24 17:18 [PATCH 0/5] fix nfs server handling of principal names J. Bruce Fields
@ 2015-11-24 17:18 ` J. Bruce Fields
  2015-11-24 17:18 ` [PATCH 2/5] nfsd: helper for dup of possibly NULL string J. Bruce Fields
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 8+ messages in thread
From: J. Bruce Fields @ 2015-11-24 17:18 UTC (permalink / raw)
  To: linux-nfs; +Cc: J. Bruce Fields

From: "J. Bruce Fields" <bfields@redhat.com>

Minor cleanup, no change in behavior.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 net/sunrpc/svcauth.c      | 2 ++
 net/sunrpc/svcauth_unix.c | 8 --------
 2 files changed, 2 insertions(+), 8 deletions(-)

diff --git a/net/sunrpc/svcauth.c b/net/sunrpc/svcauth.c
index 79c0f3459b5c..69841db1f533 100644
--- a/net/sunrpc/svcauth.c
+++ b/net/sunrpc/svcauth.c
@@ -55,6 +55,7 @@ svc_authenticate(struct svc_rqst *rqstp, __be32 *authp)
 	spin_unlock(&authtab_lock);
 
 	rqstp->rq_auth_slack = 0;
+	init_svc_cred(&rqstp->rq_cred);
 
 	rqstp->rq_authop = aops;
 	return aops->accept(rqstp, authp);
@@ -63,6 +64,7 @@ EXPORT_SYMBOL_GPL(svc_authenticate);
 
 int svc_set_client(struct svc_rqst *rqstp)
 {
+	rqstp->rq_client = NULL;
 	return rqstp->rq_authop->set_client(rqstp);
 }
 EXPORT_SYMBOL_GPL(svc_set_client);
diff --git a/net/sunrpc/svcauth_unix.c b/net/sunrpc/svcauth_unix.c
index 621ca7b4a155..dfacdc95b3f5 100644
--- a/net/sunrpc/svcauth_unix.c
+++ b/net/sunrpc/svcauth_unix.c
@@ -728,10 +728,6 @@ svcauth_null_accept(struct svc_rqst *rqstp, __be32 *authp)
 	struct kvec	*resv = &rqstp->rq_res.head[0];
 	struct svc_cred	*cred = &rqstp->rq_cred;
 
-	cred->cr_group_info = NULL;
-	cred->cr_principal = NULL;
-	rqstp->rq_client = NULL;
-
 	if (argv->iov_len < 3*4)
 		return SVC_GARBAGE;
 
@@ -794,10 +790,6 @@ svcauth_unix_accept(struct svc_rqst *rqstp, __be32 *authp)
 	u32		slen, i;
 	int		len   = argv->iov_len;
 
-	cred->cr_group_info = NULL;
-	cred->cr_principal = NULL;
-	rqstp->rq_client = NULL;
-
 	if ((len -= 3*4) < 0)
 		return SVC_GARBAGE;
 
-- 
2.5.0


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 2/5] nfsd: helper for dup of possibly NULL string
  2015-11-24 17:18 [PATCH 0/5] fix nfs server handling of principal names J. Bruce Fields
  2015-11-24 17:18 ` [PATCH 1/5] svcrpc: move some initialization to common code J. Bruce Fields
@ 2015-11-24 17:18 ` J. Bruce Fields
  2015-11-24 17:18 ` [PATCH 3/5] nfsd: minor consolidation of mach_cred handling code J. Bruce Fields
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 8+ messages in thread
From: J. Bruce Fields @ 2015-11-24 17:18 UTC (permalink / raw)
  To: linux-nfs; +Cc: J. Bruce Fields

From: "J. Bruce Fields" <bfields@redhat.com>

Technically the initialization in the NULL case isn't even needed as the
only caller already has target zeroed out, but it seems safer to keep
copy_cred generic.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/nfsd/nfs4state.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 6b800b5b8fed..db53df069069 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -1857,15 +1857,24 @@ static void copy_clid(struct nfs4_client *target, struct nfs4_client *source)
 	target->cl_clientid.cl_id = source->cl_clientid.cl_id; 
 }
 
-static int copy_cred(struct svc_cred *target, struct svc_cred *source)
+int strdup_if_nonnull(char **target, char *source)
 {
-	if (source->cr_principal) {
-		target->cr_principal =
-				kstrdup(source->cr_principal, GFP_KERNEL);
-		if (target->cr_principal == NULL)
+	if (source) {
+		*target = kstrdup(source, GFP_KERNEL);
+		if (!*target)
 			return -ENOMEM;
 	} else
-		target->cr_principal = NULL;
+		*target = NULL;
+	return 0;
+}
+
+static int copy_cred(struct svc_cred *target, struct svc_cred *source)
+{
+	int ret;
+
+	ret = strdup_if_nonnull(&target->cr_principal, source->cr_principal);
+	if (ret)
+		return ret;
 	target->cr_flavor = source->cr_flavor;
 	target->cr_uid = source->cr_uid;
 	target->cr_gid = source->cr_gid;
-- 
2.5.0


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 3/5] nfsd: minor consolidation of mach_cred handling code
  2015-11-24 17:18 [PATCH 0/5] fix nfs server handling of principal names J. Bruce Fields
  2015-11-24 17:18 ` [PATCH 1/5] svcrpc: move some initialization to common code J. Bruce Fields
  2015-11-24 17:18 ` [PATCH 2/5] nfsd: helper for dup of possibly NULL string J. Bruce Fields
@ 2015-11-24 17:18 ` J. Bruce Fields
  2015-11-24 17:18 ` [PATCH 4/5] nfsd: fix unlikely NULL deref in mach_creds_match J. Bruce Fields
  2015-11-24 17:18 ` [PATCH 5/5] nfsd4: fix gss-proxy 4.1 mounts for some AD principals J. Bruce Fields
  4 siblings, 0 replies; 8+ messages in thread
From: J. Bruce Fields @ 2015-11-24 17:18 UTC (permalink / raw)
  To: linux-nfs; +Cc: J. Bruce Fields

From: "J. Bruce Fields" <bfields@redhat.com>

Minor cleanup, no change in functionality.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/nfsd/nfs4state.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index db53df069069..5b1be1ab700b 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -2374,10 +2374,17 @@ nfsd4_exchange_id(struct svc_rqst *rqstp,
 	if (exid->flags & ~EXCHGID4_FLAG_MASK_A)
 		return nfserr_inval;
 
+	new = create_client(exid->clname, rqstp, &verf);
+	if (new == NULL)
+		return nfserr_jukebox;
+
 	switch (exid->spa_how) {
 	case SP4_MACH_CRED:
-		if (!svc_rqst_integrity_protected(rqstp))
-			return nfserr_inval;
+		if (!svc_rqst_integrity_protected(rqstp)) {
+			status = nfserr_inval;
+			goto out_nolock;
+		}
+		new->cl_mach_cred = true;
 	case SP4_NONE:
 		break;
 	default:				/* checked by xdr code */
@@ -2386,10 +2393,6 @@ nfsd4_exchange_id(struct svc_rqst *rqstp,
 		return nfserr_encr_alg_unsupp;
 	}
 
-	new = create_client(exid->clname, rqstp, &verf);
-	if (new == NULL)
-		return nfserr_jukebox;
-
 	/* Cases below refer to rfc 5661 section 18.35.4: */
 	spin_lock(&nn->client_lock);
 	conf = find_confirmed_client_by_name(&exid->clname, nn);
@@ -2451,7 +2454,6 @@ out_new:
 			goto out;
 	}
 	new->cl_minorversion = cstate->minorversion;
-	new->cl_mach_cred = (exid->spa_how == SP4_MACH_CRED);
 
 	gen_clid(new, nn);
 	add_to_unconfirmed(new);
@@ -2469,6 +2471,7 @@ out_copy:
 
 out:
 	spin_unlock(&nn->client_lock);
+out_nolock:
 	if (new)
 		expire_client(new);
 	if (unconf)
-- 
2.5.0


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 4/5] nfsd: fix unlikely NULL deref in mach_creds_match
  2015-11-24 17:18 [PATCH 0/5] fix nfs server handling of principal names J. Bruce Fields
                   ` (2 preceding siblings ...)
  2015-11-24 17:18 ` [PATCH 3/5] nfsd: minor consolidation of mach_cred handling code J. Bruce Fields
@ 2015-11-24 17:18 ` J. Bruce Fields
  2015-11-24 17:18 ` [PATCH 5/5] nfsd4: fix gss-proxy 4.1 mounts for some AD principals J. Bruce Fields
  4 siblings, 0 replies; 8+ messages in thread
From: J. Bruce Fields @ 2015-11-24 17:18 UTC (permalink / raw)
  To: linux-nfs; +Cc: J. Bruce Fields

From: "J. Bruce Fields" <bfields@redhat.com>

We really shouldn't allow a client to be created with cl_mach_cred set
unless it also has a principal name.

This also allows us to fail such cases immediately on EXCHANGE_ID as
opposed to waiting and incorrectly returning WRONG_CRED on the following
CREATE_SESSION.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/nfsd/nfs4state.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 5b1be1ab700b..36ad22a15d61 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -2384,6 +2384,15 @@ nfsd4_exchange_id(struct svc_rqst *rqstp,
 			status = nfserr_inval;
 			goto out_nolock;
 		}
+		/*
+		 * Sometimes userspace doesn't give us a principal.
+		 * Which is a bug, really.  Anyway, we can't enforce
+		 * MACH_CRED in that case, better to give up now:
+		 */
+		if (!new->cl_cred.cr_principal) {
+			status = nfserr_serverfault;
+			goto out_nolock;
+		}
 		new->cl_mach_cred = true;
 	case SP4_NONE:
 		break;
-- 
2.5.0


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 5/5] nfsd4: fix gss-proxy 4.1 mounts for some AD principals
  2015-11-24 17:18 [PATCH 0/5] fix nfs server handling of principal names J. Bruce Fields
                   ` (3 preceding siblings ...)
  2015-11-24 17:18 ` [PATCH 4/5] nfsd: fix unlikely NULL deref in mach_creds_match J. Bruce Fields
@ 2015-11-24 17:18 ` J. Bruce Fields
  2015-11-24 18:05   ` Simo Sorce
  4 siblings, 1 reply; 8+ messages in thread
From: J. Bruce Fields @ 2015-11-24 17:18 UTC (permalink / raw)
  To: linux-nfs; +Cc: J. Bruce Fields, Simo Sorce

From: "J. Bruce Fields" <bfields@redhat.com>

The principal name on a gss cred is used to setup the NFSv4.0 callback,
which has to have a client principal name to authenticate to.

That code wants the name to be in the form servicetype@hostname.
rpc.svcgssd passes down such names (and passes down no principal name at
all in the case the principal isn't a service principal).

gss-proxy always passes down the principal name, and passes it down in
the form servicetype/hostname@REALM.  So we've been munging the name
gss-proxy passes down into the format the NFSv4.0 callback code expects,
or throwing away the name if we can't.

Since the introduction of the MACH_CRED enforcement in NFSv4.1, we've
also been using the principal name to verify that certain operations are
done as the same principal as was used on the original EXCHANGE_ID call.

For that application, the original name passed down by gss-proxy is also
useful.

Lack of that name in some cases was causing some kerberized NFSv4.1
mount failures in an Active Directory environment.

This fix only works in the gss-proxy case.  The fix for legacy
rpc.svcgssd would be more involved, and rpc.svcgssd already has other
problems in the AD case.

Reported-and-tested-by: James Ralston <ralston@pobox.com>
Cc: Simo Sorce <simo@redhat.com>
Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/nfsd/nfs4state.c                  | 10 +++++++++-
 include/linux/sunrpc/svcauth.h       |  9 ++++++++-
 net/sunrpc/auth_gss/gss_rpc_upcall.c |  3 +++
 3 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 36ad22a15d61..0e685201f0db 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -1875,6 +1875,10 @@ static int copy_cred(struct svc_cred *target, struct svc_cred *source)
 	ret = strdup_if_nonnull(&target->cr_principal, source->cr_principal);
 	if (ret)
 		return ret;
+	ret = strdup_if_nonnull(&target->cr_raw_principal,
+					source->cr_raw_principal);
+	if (ret)
+		return ret;
 	target->cr_flavor = source->cr_flavor;
 	target->cr_uid = source->cr_uid;
 	target->cr_gid = source->cr_gid;
@@ -1978,6 +1982,9 @@ static bool mach_creds_match(struct nfs4_client *cl, struct svc_rqst *rqstp)
 		return false;
 	if (!svc_rqst_integrity_protected(rqstp))
 		return false;
+	if (cl->cl_cred.cr_raw_principal)
+		return 0 == strcmp(cl->cl_cred.cr_raw_principal,
+						cr->cr_raw_principal);
 	if (!cr->cr_principal)
 		return false;
 	return 0 == strcmp(cl->cl_cred.cr_principal, cr->cr_principal);
@@ -2389,7 +2396,8 @@ nfsd4_exchange_id(struct svc_rqst *rqstp,
 		 * Which is a bug, really.  Anyway, we can't enforce
 		 * MACH_CRED in that case, better to give up now:
 		 */
-		if (!new->cl_cred.cr_principal) {
+		if (!new->cl_cred.cr_principal &&
+					!new->cl_cred.cr_raw_principal) {
 			status = nfserr_serverfault;
 			goto out_nolock;
 		}
diff --git a/include/linux/sunrpc/svcauth.h b/include/linux/sunrpc/svcauth.h
index 8d71d6577459..c00f53a4ccdd 100644
--- a/include/linux/sunrpc/svcauth.h
+++ b/include/linux/sunrpc/svcauth.h
@@ -23,13 +23,19 @@ struct svc_cred {
 	kgid_t			cr_gid;
 	struct group_info	*cr_group_info;
 	u32			cr_flavor; /* pseudoflavor */
-	char			*cr_principal; /* for gss */
+	/* name of form servicetype/hostname@REALM, passed down by
+	 * gss-proxy: */
+	char			*cr_raw_principal;
+	/* name of form servicetype@hostname, passed down by
+	 * rpc.svcgssd, or computed from the above: */
+	char			*cr_principal;
 	struct gss_api_mech	*cr_gss_mech;
 };
 
 static inline void init_svc_cred(struct svc_cred *cred)
 {
 	cred->cr_group_info = NULL;
+	cred->cr_raw_principal = NULL;
 	cred->cr_principal = NULL;
 	cred->cr_gss_mech = NULL;
 }
@@ -38,6 +44,7 @@ static inline void free_svc_cred(struct svc_cred *cred)
 {
 	if (cred->cr_group_info)
 		put_group_info(cred->cr_group_info);
+	kfree(cred->cr_raw_principal);
 	kfree(cred->cr_principal);
 	gss_mech_put(cred->cr_gss_mech);
 	init_svc_cred(cred);
diff --git a/net/sunrpc/auth_gss/gss_rpc_upcall.c b/net/sunrpc/auth_gss/gss_rpc_upcall.c
index 59eeed43eda2..f0c6a8c78a56 100644
--- a/net/sunrpc/auth_gss/gss_rpc_upcall.c
+++ b/net/sunrpc/auth_gss/gss_rpc_upcall.c
@@ -326,6 +326,9 @@ int gssp_accept_sec_context_upcall(struct net *net,
 	if (data->found_creds && client_name.data != NULL) {
 		char *c;
 
+		data->creds.cr_raw_principal = kstrndup(client_name.data,
+						client_name.len, GFP_KERNEL);
+
 		data->creds.cr_principal = kstrndup(client_name.data,
 						client_name.len, GFP_KERNEL);
 		if (data->creds.cr_principal) {
-- 
2.5.0


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH 5/5] nfsd4: fix gss-proxy 4.1 mounts for some AD principals
  2015-11-24 17:18 ` [PATCH 5/5] nfsd4: fix gss-proxy 4.1 mounts for some AD principals J. Bruce Fields
@ 2015-11-24 18:05   ` Simo Sorce
  2015-11-24 18:36     ` J. Bruce Fields
  0 siblings, 1 reply; 8+ messages in thread
From: Simo Sorce @ 2015-11-24 18:05 UTC (permalink / raw)
  To: J. Bruce Fields; +Cc: linux-nfs

On Tue, 2015-11-24 at 12:18 -0500, J. Bruce Fields wrote:
> From: "J. Bruce Fields" <bfields@redhat.com>
> 
> The principal name on a gss cred is used to setup the NFSv4.0 callback,
> which has to have a client principal name to authenticate to.
> 
> That code wants the name to be in the form servicetype@hostname.
> rpc.svcgssd passes down such names (and passes down no principal name at
> all in the case the principal isn't a service principal).
> 
> gss-proxy always passes down the principal name, and passes it down in
> the form servicetype/hostname@REALM.  So we've been munging the name
> gss-proxy passes down into the format the NFSv4.0 callback code expects,
> or throwing away the name if we can't.
> 
> Since the introduction of the MACH_CRED enforcement in NFSv4.1, we've
> also been using the principal name to verify that certain operations are
> done as the same principal as was used on the original EXCHANGE_ID call.
> 
> For that application, the original name passed down by gss-proxy is also
> useful.
> 
> Lack of that name in some cases was causing some kerberized NFSv4.1
> mount failures in an Active Directory environment.
> 
> This fix only works in the gss-proxy case.  The fix for legacy
> rpc.svcgssd would be more involved, and rpc.svcgssd already has other
> problems in the AD case.
> 
> Reported-and-tested-by: James Ralston <ralston@pobox.com>
> Cc: Simo Sorce <simo@redhat.com>

LGTM, feel free to add ack by me.

> Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> ---
>  fs/nfsd/nfs4state.c                  | 10 +++++++++-
>  include/linux/sunrpc/svcauth.h       |  9 ++++++++-
>  net/sunrpc/auth_gss/gss_rpc_upcall.c |  3 +++
>  3 files changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 36ad22a15d61..0e685201f0db 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -1875,6 +1875,10 @@ static int copy_cred(struct svc_cred *target, struct svc_cred *source)
>  	ret = strdup_if_nonnull(&target->cr_principal, source->cr_principal);
>  	if (ret)
>  		return ret;
> +	ret = strdup_if_nonnull(&target->cr_raw_principal,
> +					source->cr_raw_principal);
> +	if (ret)
> +		return ret;
>  	target->cr_flavor = source->cr_flavor;
>  	target->cr_uid = source->cr_uid;
>  	target->cr_gid = source->cr_gid;
> @@ -1978,6 +1982,9 @@ static bool mach_creds_match(struct nfs4_client *cl, struct svc_rqst *rqstp)
>  		return false;
>  	if (!svc_rqst_integrity_protected(rqstp))
>  		return false;
> +	if (cl->cl_cred.cr_raw_principal)
> +		return 0 == strcmp(cl->cl_cred.cr_raw_principal,
> +						cr->cr_raw_principal);
>  	if (!cr->cr_principal)
>  		return false;
>  	return 0 == strcmp(cl->cl_cred.cr_principal, cr->cr_principal);
> @@ -2389,7 +2396,8 @@ nfsd4_exchange_id(struct svc_rqst *rqstp,
>  		 * Which is a bug, really.  Anyway, we can't enforce
>  		 * MACH_CRED in that case, better to give up now:
>  		 */
> -		if (!new->cl_cred.cr_principal) {
> +		if (!new->cl_cred.cr_principal &&
> +					!new->cl_cred.cr_raw_principal) {
>  			status = nfserr_serverfault;
>  			goto out_nolock;
>  		}
> diff --git a/include/linux/sunrpc/svcauth.h b/include/linux/sunrpc/svcauth.h
> index 8d71d6577459..c00f53a4ccdd 100644
> --- a/include/linux/sunrpc/svcauth.h
> +++ b/include/linux/sunrpc/svcauth.h
> @@ -23,13 +23,19 @@ struct svc_cred {
>  	kgid_t			cr_gid;
>  	struct group_info	*cr_group_info;
>  	u32			cr_flavor; /* pseudoflavor */
> -	char			*cr_principal; /* for gss */
> +	/* name of form servicetype/hostname@REALM, passed down by
> +	 * gss-proxy: */
> +	char			*cr_raw_principal;
> +	/* name of form servicetype@hostname, passed down by
> +	 * rpc.svcgssd, or computed from the above: */
> +	char			*cr_principal;
>  	struct gss_api_mech	*cr_gss_mech;
>  };
>  
>  static inline void init_svc_cred(struct svc_cred *cred)
>  {
>  	cred->cr_group_info = NULL;
> +	cred->cr_raw_principal = NULL;
>  	cred->cr_principal = NULL;
>  	cred->cr_gss_mech = NULL;
>  }
> @@ -38,6 +44,7 @@ static inline void free_svc_cred(struct svc_cred *cred)
>  {
>  	if (cred->cr_group_info)
>  		put_group_info(cred->cr_group_info);
> +	kfree(cred->cr_raw_principal);
>  	kfree(cred->cr_principal);
>  	gss_mech_put(cred->cr_gss_mech);
>  	init_svc_cred(cred);
> diff --git a/net/sunrpc/auth_gss/gss_rpc_upcall.c b/net/sunrpc/auth_gss/gss_rpc_upcall.c
> index 59eeed43eda2..f0c6a8c78a56 100644
> --- a/net/sunrpc/auth_gss/gss_rpc_upcall.c
> +++ b/net/sunrpc/auth_gss/gss_rpc_upcall.c
> @@ -326,6 +326,9 @@ int gssp_accept_sec_context_upcall(struct net *net,
>  	if (data->found_creds && client_name.data != NULL) {
>  		char *c;
>  
> +		data->creds.cr_raw_principal = kstrndup(client_name.data,
> +						client_name.len, GFP_KERNEL);
> +
>  		data->creds.cr_principal = kstrndup(client_name.data,
>  						client_name.len, GFP_KERNEL);
>  		if (data->creds.cr_principal) {


-- 
Simo Sorce * Red Hat, Inc * New York


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 5/5] nfsd4: fix gss-proxy 4.1 mounts for some AD principals
  2015-11-24 18:05   ` Simo Sorce
@ 2015-11-24 18:36     ` J. Bruce Fields
  0 siblings, 0 replies; 8+ messages in thread
From: J. Bruce Fields @ 2015-11-24 18:36 UTC (permalink / raw)
  To: Simo Sorce; +Cc: linux-nfs

On Tue, Nov 24, 2015 at 01:05:06PM -0500, Simo Sorce wrote:
> On Tue, 2015-11-24 at 12:18 -0500, J. Bruce Fields wrote:
> > From: "J. Bruce Fields" <bfields@redhat.com>
> > 
> > The principal name on a gss cred is used to setup the NFSv4.0 callback,
> > which has to have a client principal name to authenticate to.
> > 
> > That code wants the name to be in the form servicetype@hostname.
> > rpc.svcgssd passes down such names (and passes down no principal name at
> > all in the case the principal isn't a service principal).
> > 
> > gss-proxy always passes down the principal name, and passes it down in
> > the form servicetype/hostname@REALM.  So we've been munging the name
> > gss-proxy passes down into the format the NFSv4.0 callback code expects,
> > or throwing away the name if we can't.
> > 
> > Since the introduction of the MACH_CRED enforcement in NFSv4.1, we've
> > also been using the principal name to verify that certain operations are
> > done as the same principal as was used on the original EXCHANGE_ID call.
> > 
> > For that application, the original name passed down by gss-proxy is also
> > useful.
> > 
> > Lack of that name in some cases was causing some kerberized NFSv4.1
> > mount failures in an Active Directory environment.
> > 
> > This fix only works in the gss-proxy case.  The fix for legacy
> > rpc.svcgssd would be more involved, and rpc.svcgssd already has other
> > problems in the AD case.
> > 
> > Reported-and-tested-by: James Ralston <ralston@pobox.com>
> > Cc: Simo Sorce <simo@redhat.com>
> 
> LGTM, feel free to add ack by me.

Thanks!--b.

> 
> > Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> > ---
> >  fs/nfsd/nfs4state.c                  | 10 +++++++++-
> >  include/linux/sunrpc/svcauth.h       |  9 ++++++++-
> >  net/sunrpc/auth_gss/gss_rpc_upcall.c |  3 +++
> >  3 files changed, 20 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index 36ad22a15d61..0e685201f0db 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -1875,6 +1875,10 @@ static int copy_cred(struct svc_cred *target, struct svc_cred *source)
> >  	ret = strdup_if_nonnull(&target->cr_principal, source->cr_principal);
> >  	if (ret)
> >  		return ret;
> > +	ret = strdup_if_nonnull(&target->cr_raw_principal,
> > +					source->cr_raw_principal);
> > +	if (ret)
> > +		return ret;
> >  	target->cr_flavor = source->cr_flavor;
> >  	target->cr_uid = source->cr_uid;
> >  	target->cr_gid = source->cr_gid;
> > @@ -1978,6 +1982,9 @@ static bool mach_creds_match(struct nfs4_client *cl, struct svc_rqst *rqstp)
> >  		return false;
> >  	if (!svc_rqst_integrity_protected(rqstp))
> >  		return false;
> > +	if (cl->cl_cred.cr_raw_principal)
> > +		return 0 == strcmp(cl->cl_cred.cr_raw_principal,
> > +						cr->cr_raw_principal);
> >  	if (!cr->cr_principal)
> >  		return false;
> >  	return 0 == strcmp(cl->cl_cred.cr_principal, cr->cr_principal);
> > @@ -2389,7 +2396,8 @@ nfsd4_exchange_id(struct svc_rqst *rqstp,
> >  		 * Which is a bug, really.  Anyway, we can't enforce
> >  		 * MACH_CRED in that case, better to give up now:
> >  		 */
> > -		if (!new->cl_cred.cr_principal) {
> > +		if (!new->cl_cred.cr_principal &&
> > +					!new->cl_cred.cr_raw_principal) {
> >  			status = nfserr_serverfault;
> >  			goto out_nolock;
> >  		}
> > diff --git a/include/linux/sunrpc/svcauth.h b/include/linux/sunrpc/svcauth.h
> > index 8d71d6577459..c00f53a4ccdd 100644
> > --- a/include/linux/sunrpc/svcauth.h
> > +++ b/include/linux/sunrpc/svcauth.h
> > @@ -23,13 +23,19 @@ struct svc_cred {
> >  	kgid_t			cr_gid;
> >  	struct group_info	*cr_group_info;
> >  	u32			cr_flavor; /* pseudoflavor */
> > -	char			*cr_principal; /* for gss */
> > +	/* name of form servicetype/hostname@REALM, passed down by
> > +	 * gss-proxy: */
> > +	char			*cr_raw_principal;
> > +	/* name of form servicetype@hostname, passed down by
> > +	 * rpc.svcgssd, or computed from the above: */
> > +	char			*cr_principal;
> >  	struct gss_api_mech	*cr_gss_mech;
> >  };
> >  
> >  static inline void init_svc_cred(struct svc_cred *cred)
> >  {
> >  	cred->cr_group_info = NULL;
> > +	cred->cr_raw_principal = NULL;
> >  	cred->cr_principal = NULL;
> >  	cred->cr_gss_mech = NULL;
> >  }
> > @@ -38,6 +44,7 @@ static inline void free_svc_cred(struct svc_cred *cred)
> >  {
> >  	if (cred->cr_group_info)
> >  		put_group_info(cred->cr_group_info);
> > +	kfree(cred->cr_raw_principal);
> >  	kfree(cred->cr_principal);
> >  	gss_mech_put(cred->cr_gss_mech);
> >  	init_svc_cred(cred);
> > diff --git a/net/sunrpc/auth_gss/gss_rpc_upcall.c b/net/sunrpc/auth_gss/gss_rpc_upcall.c
> > index 59eeed43eda2..f0c6a8c78a56 100644
> > --- a/net/sunrpc/auth_gss/gss_rpc_upcall.c
> > +++ b/net/sunrpc/auth_gss/gss_rpc_upcall.c
> > @@ -326,6 +326,9 @@ int gssp_accept_sec_context_upcall(struct net *net,
> >  	if (data->found_creds && client_name.data != NULL) {
> >  		char *c;
> >  
> > +		data->creds.cr_raw_principal = kstrndup(client_name.data,
> > +						client_name.len, GFP_KERNEL);
> > +
> >  		data->creds.cr_principal = kstrndup(client_name.data,
> >  						client_name.len, GFP_KERNEL);
> >  		if (data->creds.cr_principal) {
> 
> 
> -- 
> Simo Sorce * Red Hat, Inc * New York
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2015-11-24 18:36 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-11-24 17:18 [PATCH 0/5] fix nfs server handling of principal names J. Bruce Fields
2015-11-24 17:18 ` [PATCH 1/5] svcrpc: move some initialization to common code J. Bruce Fields
2015-11-24 17:18 ` [PATCH 2/5] nfsd: helper for dup of possibly NULL string J. Bruce Fields
2015-11-24 17:18 ` [PATCH 3/5] nfsd: minor consolidation of mach_cred handling code J. Bruce Fields
2015-11-24 17:18 ` [PATCH 4/5] nfsd: fix unlikely NULL deref in mach_creds_match J. Bruce Fields
2015-11-24 17:18 ` [PATCH 5/5] nfsd4: fix gss-proxy 4.1 mounts for some AD principals J. Bruce Fields
2015-11-24 18:05   ` Simo Sorce
2015-11-24 18:36     ` J. Bruce Fields

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.