[PATCH 0/5] gssd: add the GSSAPI acceptor name to the info passed in downcall

[PATCH 0/5] gssd: add the GSSAPI acceptor name to the info passed in downcall

[Jeff Layton]

Recently, I started a mailing list thread about some authentication
failures that I was seeing on the callback channel when krb5 was in use.

After a bit of discussion we determined that the right way to fix it
was to save off the GSSAPI acceptor name used in the SETCLIENT call,
and then ensure that the same principal is used in callback requests.

This patchset is the userland portion of that change. It basically
just adds the acceptor name to the downcall, immediately following
the context token. Older kernel will just ignore this data, so this
should be safe.

There is also a companion kernel patchset that will allow the kernel
to save off this info for later usage.

Jeff Layton (5):
  gssd: handle malloc failure appropriately in do_downcall
  gssd: make do_downcall a void return
  gssd: move hostbased name routines into separate file
  gssd: add new routine for generating a hostbased principal in a
    gss_buffer_t
  gssd: scrape the acceptor name out of the context

 utils/gssd/Makefile.am    |   2 +
 utils/gssd/gss_names.c    | 138 ++++++++++++++++++++++++++++++++++++++++++++++
 utils/gssd/gss_names.h    |  36 ++++++++++++
 utils/gssd/gssd_proc.c    |  52 +++++++++++------
 utils/gssd/svcgssd_proc.c |  66 +---------------------
 5 files changed, 213 insertions(+), 81 deletions(-)
 create mode 100644 utils/gssd/gss_names.c
 create mode 100644 utils/gssd/gss_names.h

-- 
1.9.0

[PATCH 1/5] gssd: handle malloc failure appropriately in do_downcall

[Jeff Layton]

...and get rid of some pointless NULL ptr checks.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 utils/gssd/gssd_proc.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
index 33cfeb2afd2e..5f7fb32c41b5 100644
--- a/utils/gssd/gssd_proc.c
+++ b/utils/gssd/gssd_proc.c
@@ -694,6 +694,9 @@ do_downcall(int k5_fd, uid_t uid, struct authgss_private_data *pd,
 		sizeof(pd->pd_ctx_hndl.length) + pd->pd_ctx_hndl.length +
 		sizeof(context_token->length) + context_token->length;
 	p = buf = malloc(buf_size);
+	if (!buf)
+		goto out_err;
+
 	end = buf + buf_size;
 
 	/* context_timeout set by -t option overrides context lifetime */
@@ -706,10 +709,10 @@ do_downcall(int k5_fd, uid_t uid, struct authgss_private_data *pd,
 	if (write_buffer(&p, end, context_token)) goto out_err;
 
 	if (write(k5_fd, buf, p - buf) < p - buf) goto out_err;
-	if (buf) free(buf);
+	free(buf);
 	return 0;
 out_err:
-	if (buf) free(buf);
+	free(buf);
 	printerr(1, "Failed to write downcall!\n");
 	return -1;
 }
-- 
1.9.0

[PATCH 2/5] gssd: make do_downcall a void return

[Jeff Layton]

...since its return code is ignored anyway.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 utils/gssd/gssd_proc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
index 5f7fb32c41b5..7387cce010cf 100644
--- a/utils/gssd/gssd_proc.c
+++ b/utils/gssd/gssd_proc.c
@@ -681,7 +681,7 @@ parse_enctypes(char *enctypes)
 	return 0;
 }
 
-static int
+static void
 do_downcall(int k5_fd, uid_t uid, struct authgss_private_data *pd,
 	    gss_buffer_desc *context_token, OM_uint32 lifetime_rec)
 {
@@ -710,11 +710,11 @@ do_downcall(int k5_fd, uid_t uid, struct authgss_private_data *pd,
 
 	if (write(k5_fd, buf, p - buf) < p - buf) goto out_err;
 	free(buf);
-	return 0;
+	return;
 out_err:
 	free(buf);
 	printerr(1, "Failed to write downcall!\n");
-	return -1;
+	return;
 }
 
 static int
-- 
1.9.0

[PATCH 3/5] gssd: move hostbased name routines into separate file

[Jeff Layton]

In a later patch, we'll need gssd to call into this code as well as
svcgssd. Move it into a common file that both can link in.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 utils/gssd/Makefile.am    |   2 +
 utils/gssd/gss_names.c    | 123 ++++++++++++++++++++++++++++++++++++++++++++++
 utils/gssd/gss_names.h    |  34 +++++++++++++
 utils/gssd/svcgssd_proc.c |  66 +------------------------
 4 files changed, 160 insertions(+), 65 deletions(-)
 create mode 100644 utils/gssd/gss_names.c
 create mode 100644 utils/gssd/gss_names.h

diff --git a/utils/gssd/Makefile.am b/utils/gssd/Makefile.am
index a9a3e42de19d..af597918e8e0 100644
--- a/utils/gssd/Makefile.am
+++ b/utils/gssd/Makefile.am
@@ -18,11 +18,13 @@ COMMON_SRCS = \
 	context_lucid.c \
 	gss_util.c \
 	gss_oids.c \
+	gss_names.c \
 	err_util.c \
 	\
 	context.h \
 	err_util.h \
 	gss_oids.h \
+	gss_names.h \
 	gss_util.h
 
 gssd_SOURCES = \
diff --git a/utils/gssd/gss_names.c b/utils/gssd/gss_names.c
new file mode 100644
index 000000000000..aa61e4d7a851
--- /dev/null
+++ b/utils/gssd/gss_names.c
@@ -0,0 +1,123 @@
+/*
+  Copyright (c) 2000 The Regents of the University of Michigan.
+  All rights reserved.
+
+  Copyright (c) 2002 Bruce Fields <bfields@UMICH.EDU>
+
+  Redistribution and use in source and binary forms, with or without
+  modification, are permitted provided that the following conditions
+  are met:
+
+  1. Redistributions of source code must retain the above copyright
+     notice, this list of conditions and the following disclaimer.
+  2. Redistributions in binary form must reproduce the above copyright
+     notice, this list of conditions and the following disclaimer in the
+     documentation and/or other materials provided with the distribution.
+  3. Neither the name of the University nor the names of its
+     contributors may be used to endorse or promote products derived
+     from this software without specific prior written permission.
+
+  THIS SOFTWARE IS PROVIDED ``AS IS'' AND ANY EXPRESS OR IMPLIED
+  WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF
+  MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
+  DISCLAIMED. IN NO EVENT SHALL THE REGENTS OR CONTRIBUTORS BE LIABLE
+  FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+  CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+  SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
+  BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
+  LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING
+  NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
+  SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+*/
+
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif	/* HAVE_CONFIG_H */
+
+#include <sys/param.h>
+#include <sys/stat.h>
+#include <rpc/rpc.h>
+
+#include <pwd.h>
+#include <stdio.h>
+#include <unistd.h>
+#include <ctype.h>
+#include <string.h>
+#include <fcntl.h>
+#include <errno.h>
+#include <nfsidmap.h>
+#include <nfslib.h>
+#include <time.h>
+
+#include "svcgssd.h"
+#include "gss_util.h"
+#include "err_util.h"
+#include "context.h"
+#include "misc.h"
+#include "gss_oids.h"
+#include "svcgssd_krb5.h"
+
+static int
+get_krb5_hostbased_name(gss_buffer_desc *name, char **hostbased_name)
+{
+	char *p, *sname = NULL;
+	if (strchr(name->value, '@') && strchr(name->value, '/')) {
+		if ((sname = calloc(name->length, 1)) == NULL) {
+			printerr(0, "ERROR: get_krb5_hostbased_name failed "
+				 "to allocate %d bytes\n", name->length);
+			return -1;
+		}
+		/* read in name and instance and replace '/' with '@' */
+		sscanf(name->value, "%[^@]", sname);
+		p = strrchr(sname, '/');
+		if (p == NULL) {    /* The '@' preceeded the '/' */
+			free(sname);
+			return -1;
+		}
+		*p = '@';
+	}
+	*hostbased_name = sname;
+	return 0;
+}
+
+int
+get_hostbased_client_name(gss_name_t client_name, gss_OID mech,
+			  char **hostbased_name)
+{
+	u_int32_t	maj_stat, min_stat;
+	gss_buffer_desc	name;
+	gss_OID		name_type = GSS_C_NO_OID;
+	char		*cname;
+	int		res = -1;
+
+	*hostbased_name = NULL;	    /* preset in case we fail */
+
+	/* Get the client's gss authenticated name */
+	maj_stat = gss_display_name(&min_stat, client_name, &name, &name_type);
+	if (maj_stat != GSS_S_COMPLETE) {
+		pgsserr("get_hostbased_client_name: gss_display_name",
+			maj_stat, min_stat, mech);
+		goto out_err;
+	}
+	if (name.length >= 0xffff) {	    /* don't overflow */
+		printerr(0, "ERROR: get_hostbased_client_name: "
+			 "received gss_name is too long (%d bytes)\n",
+			 name.length);
+		goto out_rel_buf;
+	}
+
+	/* For Kerberos, transform the NT_KRB5_PRINCIPAL name to
+	 * an NT_HOSTBASED_SERVICE name */
+	if (g_OID_equal(&krb5oid, mech)) {
+		if (get_krb5_hostbased_name(&name, &cname) == 0)
+			*hostbased_name = cname;
+	} else {
+		printerr(1, "WARNING: unknown/unsupport mech OID\n");
+	}
+
+	res = 0;
+out_rel_buf:
+	gss_release_buffer(&min_stat, &name);
+out_err:
+	return res;
+}
diff --git a/utils/gssd/gss_names.h b/utils/gssd/gss_names.h
new file mode 100644
index 000000000000..1d5f49c1c1d2
--- /dev/null
+++ b/utils/gssd/gss_names.h
@@ -0,0 +1,34 @@
+/*
+  Copyright (c) 2000 The Regents of the University of Michigan.
+  All rights reserved.
+
+  Copyright (c) 2002 Bruce Fields <bfields@UMICH.EDU>
+
+  Redistribution and use in source and binary forms, with or without
+  modification, are permitted provided that the following conditions
+  are met:
+
+  1. Redistributions of source code must retain the above copyright
+     notice, this list of conditions and the following disclaimer.
+  2. Redistributions in binary form must reproduce the above copyright
+     notice, this list of conditions and the following disclaimer in the
+     documentation and/or other materials provided with the distribution.
+  3. Neither the name of the University nor the names of its
+     contributors may be used to endorse or promote products derived
+     from this software without specific prior written permission.
+
+  THIS SOFTWARE IS PROVIDED ``AS IS'' AND ANY EXPRESS OR IMPLIED
+  WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF
+  MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
+  DISCLAIMED. IN NO EVENT SHALL THE REGENTS OR CONTRIBUTORS BE LIABLE
+  FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+  CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+  SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
+  BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
+  LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING
+  NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
+  SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+*/
+
+extern int get_hostbased_client_name(gss_name_t client_name, gss_OID mech,
+					char **hostbased_name);
diff --git a/utils/gssd/svcgssd_proc.c b/utils/gssd/svcgssd_proc.c
index 3757d5191041..5bdb438650d4 100644
--- a/utils/gssd/svcgssd_proc.c
+++ b/utils/gssd/svcgssd_proc.c
@@ -59,6 +59,7 @@
 #include "misc.h"
 #include "gss_oids.h"
 #include "svcgssd_krb5.h"
+#include "gss_names.h"
 
 extern char * mech2file(gss_OID mech);
 #define SVCGSSD_CONTEXT_CHANNEL "/proc/net/rpc/auth.rpcsec.context/channel"
@@ -315,71 +316,6 @@ print_hexl(const char *description, unsigned char *cp, int length)
 }
 #endif
 
-static int
-get_krb5_hostbased_name (gss_buffer_desc *name, char **hostbased_name)
-{
-	char *p, *sname = NULL;
-	if (strchr(name->value, '@') && strchr(name->value, '/')) {
-		if ((sname = calloc(name->length, 1)) == NULL) {
-			printerr(0, "ERROR: get_krb5_hostbased_name failed "
-				 "to allocate %d bytes\n", name->length);
-			return -1;
-		}
-		/* read in name and instance and replace '/' with '@' */
-		sscanf(name->value, "%[^@]", sname);
-		p = strrchr(sname, '/');
-		if (p == NULL) {    /* The '@' preceeded the '/' */
-			free(sname);
-			return -1;
-		}
-		*p = '@';
-	}
-	*hostbased_name = sname;
-	return 0;
-}
-
-static int
-get_hostbased_client_name(gss_name_t client_name, gss_OID mech,
-			  char **hostbased_name)
-{
-	u_int32_t	maj_stat, min_stat;
-	gss_buffer_desc	name;
-	gss_OID		name_type = GSS_C_NO_OID;
-	char		*cname;
-	int		res = -1;
-
-	*hostbased_name = NULL;	    /* preset in case we fail */
-
-	/* Get the client's gss authenticated name */
-	maj_stat = gss_display_name(&min_stat, client_name, &name, &name_type);
-	if (maj_stat != GSS_S_COMPLETE) {
-		pgsserr("get_hostbased_client_name: gss_display_name",
-			maj_stat, min_stat, mech);
-		goto out_err;
-	}
-	if (name.length >= 0xffff) {	    /* don't overflow */
-		printerr(0, "ERROR: get_hostbased_client_name: "
-			 "received gss_name is too long (%d bytes)\n",
-			 name.length);
-		goto out_rel_buf;
-	}
-
-	/* For Kerberos, transform the NT_KRB5_PRINCIPAL name to
-	 * an NT_HOSTBASED_SERVICE name */
-	if (g_OID_equal(&krb5oid, mech)) {
-		if (get_krb5_hostbased_name(&name, &cname) == 0)
-			*hostbased_name = cname;
-	} else {
-		printerr(1, "WARNING: unknown/unsupport mech OID\n");
-	}
-
-	res = 0;
-out_rel_buf:
-	gss_release_buffer(&min_stat, &name);
-out_err:
-	return res;
-}
-
 void
 handle_nullreq(FILE *f) {
 	/* XXX initialize to a random integer to reduce chances of unnecessary
-- 
1.9.0

[PATCH 4/5] gssd: add new routine for generating a hostbased principal in a gss_buffer_t

[Jeff Layton]

We'll need a gss_buffer_t to pass to the downcall marshalling code.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 utils/gssd/gss_names.c | 15 +++++++++++++++
 utils/gssd/gss_names.h |  2 ++
 2 files changed, 17 insertions(+)

diff --git a/utils/gssd/gss_names.c b/utils/gssd/gss_names.c
index aa61e4d7a851..047069de1755 100644
--- a/utils/gssd/gss_names.c
+++ b/utils/gssd/gss_names.c
@@ -121,3 +121,18 @@ out_rel_buf:
 out_err:
 	return res;
 }
+
+void
+get_hostbased_client_buffer(gss_name_t client_name, gss_OID mech,
+			    gss_buffer_t buf)
+{
+	char *hname;
+
+	if (!get_hostbased_client_name(client_name, mech, &hname)) {
+		buf->length = strlen(hname) + 1;
+		buf->value = hname;
+	} else {
+		buf->length = 0;
+		buf->value = NULL;
+	}
+}
diff --git a/utils/gssd/gss_names.h b/utils/gssd/gss_names.h
index 1d5f49c1c1d2..ce182f7985f3 100644
--- a/utils/gssd/gss_names.h
+++ b/utils/gssd/gss_names.h
@@ -32,3 +32,5 @@
 
 extern int get_hostbased_client_name(gss_name_t client_name, gss_OID mech,
 					char **hostbased_name);
+extern void get_hostbased_client_buffer(gss_name_t client_name,
+			gss_OID mech, gss_buffer_t buf);
-- 
1.9.0

[PATCH 5/5] gssd: scrape the acceptor name out of the context

[Jeff Layton]

...and pass it to the kernel in the downcall. Legacy kernels will just
ignore the extra data, but with a proposed kernel patch the kernel will
grab this info and use it to verify requests on the v4.0 callback
channel.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 utils/gssd/gssd_proc.c | 39 ++++++++++++++++++++++++++++-----------
 1 file changed, 28 insertions(+), 11 deletions(-)

diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
index 7387cce010cf..d95e39416c28 100644
--- a/utils/gssd/gssd_proc.c
+++ b/utils/gssd/gssd_proc.c
@@ -77,6 +77,7 @@
 #include "context.h"
 #include "nfsrpc.h"
 #include "nfslib.h"
+#include "gss_names.h"
 
 /*
  * pollarray:
@@ -683,16 +684,19 @@ parse_enctypes(char *enctypes)
 
 static void
 do_downcall(int k5_fd, uid_t uid, struct authgss_private_data *pd,
-	    gss_buffer_desc *context_token, OM_uint32 lifetime_rec)
+	    gss_buffer_desc *context_token, OM_uint32 lifetime_rec,
+	    gss_buffer_desc *acceptor)
 {
 	char    *buf = NULL, *p = NULL, *end = NULL;
 	unsigned int timeout = context_timeout;
 	unsigned int buf_size = 0;
 
-	printerr(1, "doing downcall lifetime_rec %u\n", lifetime_rec);
+	printerr(1, "doing downcall: lifetime_rec=%u acceptor=%.*s\n",
+		lifetime_rec, acceptor->length, acceptor->value);
 	buf_size = sizeof(uid) + sizeof(timeout) + sizeof(pd->pd_seq_win) +
 		sizeof(pd->pd_ctx_hndl.length) + pd->pd_ctx_hndl.length +
-		sizeof(context_token->length) + context_token->length;
+		sizeof(context_token->length) + context_token->length +
+		acceptor->length;
 	p = buf = malloc(buf_size);
 	if (!buf)
 		goto out_err;
@@ -707,6 +711,8 @@ do_downcall(int k5_fd, uid_t uid, struct authgss_private_data *pd,
 	if (WRITE_BYTES(&p, end, pd->pd_seq_win)) goto out_err;
 	if (write_buffer(&p, end, &pd->pd_ctx_hndl)) goto out_err;
 	if (write_buffer(&p, end, context_token)) goto out_err;
+	if (acceptor->length > 0 &&
+	    write_buffer(&p, end, acceptor)) goto out_err;
 
 	if (write(k5_fd, buf, p - buf) < p - buf) goto out_err;
 	free(buf);
@@ -1034,6 +1040,9 @@ process_krb5_upcall(struct clnt_info *clp, uid_t uid, int fd, char *tgtname,
 	gss_cred_id_t		gss_cred;
 	OM_uint32		maj_stat, min_stat, lifetime_rec;
 	pid_t			pid;
+	gss_name_t		gacceptor;
+	gss_OID			mech;
+	gss_buffer_desc		acceptor  = {0};
 
 	pid = fork();
 	switch(pid) {
@@ -1174,15 +1183,22 @@ process_krb5_upcall(struct clnt_info *clp, uid_t uid, int fd, char *tgtname,
 		goto out_return_error;
 	}
 
-	/* Grab the context lifetime to pass to the kernel. lifetime_rec
-	 * is set to zero on error */
-	maj_stat = gss_inquire_context(&min_stat, pd.pd_ctx, NULL, NULL,
-				       &lifetime_rec, NULL, NULL, NULL, NULL);
+	maj_stat = gss_inquire_context(&min_stat, pd.pd_ctx, NULL, &gacceptor,
+				       &lifetime_rec, &mech, NULL, NULL, NULL);
 
-	if (maj_stat)
-		printerr(1, "WARNING: Failed to inquire context for lifetme "
-			    "maj_stat %u\n", maj_stat);
+	if (maj_stat != GSS_S_COMPLETE) {
+		printerr(1, "WARNING: Failed to inquire context "
+			    "maj_stat (0x%x)\n", maj_stat);
+	} else {
+		get_hostbased_client_buffer(gacceptor, mech, &acceptor);
+		gss_release_name(&min_stat, &gacceptor);
+	}
 
+	/*
+	 * The serialization can mean turning the ctx into a lucid context. If
+	 * that happens then the original ctx is no longer valid, so we mustn't
+	 * try to use if after this point.
+	 */
 	if (serialize_context_for_kernel(&pd.pd_ctx, &token, &krb5oid, NULL)) {
 		printerr(0, "WARNING: Failed to serialize krb5 context for "
 			    "user with uid %d for server %s\n",
@@ -1190,9 +1206,10 @@ process_krb5_upcall(struct clnt_info *clp, uid_t uid, int fd, char *tgtname,
 		goto out_return_error;
 	}
 
-	do_downcall(fd, uid, &pd, &token, lifetime_rec);
+	do_downcall(fd, uid, &pd, &token, lifetime_rec, &acceptor);
 
 out:
+	gss_release_buffer(&min_stat, &acceptor);
 	if (token.value)
 		free(token.value);
 #ifdef HAVE_AUTHGSS_FREE_PRIVATE_DATA
-- 
1.9.0

Re: [PATCH 5/5] gssd: scrape the acceptor name out of the context

[Jeff Layton]

On Thu, 10 Apr 2014 16:31:03 -0400
Jeff Layton <jlayton@redhat.com> wrote:

> ...and pass it to the kernel in the downcall. Legacy kernels will just
> ignore the extra data, but with a proposed kernel patch the kernel will
> grab this info and use it to verify requests on the v4.0 callback
> channel.
> 
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
>  utils/gssd/gssd_proc.c | 39 ++++++++++++++++++++++++++++-----------
>  1 file changed, 28 insertions(+), 11 deletions(-)
> 
> diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
> index 7387cce010cf..d95e39416c28 100644
> --- a/utils/gssd/gssd_proc.c
> +++ b/utils/gssd/gssd_proc.c
> @@ -77,6 +77,7 @@
>  #include "context.h"
>  #include "nfsrpc.h"
>  #include "nfslib.h"
> +#include "gss_names.h"
>  
>  /*
>   * pollarray:
> @@ -683,16 +684,19 @@ parse_enctypes(char *enctypes)
>  
>  static void
>  do_downcall(int k5_fd, uid_t uid, struct authgss_private_data *pd,
> -	    gss_buffer_desc *context_token, OM_uint32 lifetime_rec)
> +	    gss_buffer_desc *context_token, OM_uint32 lifetime_rec,
> +	    gss_buffer_desc *acceptor)
>  {
>  	char    *buf = NULL, *p = NULL, *end = NULL;
>  	unsigned int timeout = context_timeout;
>  	unsigned int buf_size = 0;
>  
> -	printerr(1, "doing downcall lifetime_rec %u\n", lifetime_rec);
> +	printerr(1, "doing downcall: lifetime_rec=%u acceptor=%.*s\n",
> +		lifetime_rec, acceptor->length, acceptor->value);
>  	buf_size = sizeof(uid) + sizeof(timeout) + sizeof(pd->pd_seq_win) +
>  		sizeof(pd->pd_ctx_hndl.length) + pd->pd_ctx_hndl.length +
> -		sizeof(context_token->length) + context_token->length;
> +		sizeof(context_token->length) + context_token->length +
> +		acceptor->length;
>  	p = buf = malloc(buf_size);
>  	if (!buf)
>  		goto out_err;
> @@ -707,6 +711,8 @@ do_downcall(int k5_fd, uid_t uid, struct authgss_private_data *pd,
>  	if (WRITE_BYTES(&p, end, pd->pd_seq_win)) goto out_err;
>  	if (write_buffer(&p, end, &pd->pd_ctx_hndl)) goto out_err;
>  	if (write_buffer(&p, end, context_token)) goto out_err;
> +	if (acceptor->length > 0 &&
> +	    write_buffer(&p, end, acceptor)) goto out_err;

I think I'll need to make a minor change to this patch. In the event
that the acceptor has zero length, then I think we still want to add
the length to the downcall. That will allow us to extend the downcall
format again in the future if needed.

The only thing I'll need to do to fix that is to remove the
"acceptor->length > 0" check above. I'll do that and send a v2 patch
once I've given everyone a chance to comment on the set.

>  
>  	if (write(k5_fd, buf, p - buf) < p - buf) goto out_err;
>  	free(buf);
> @@ -1034,6 +1040,9 @@ process_krb5_upcall(struct clnt_info *clp, uid_t uid, int fd, char *tgtname,
>  	gss_cred_id_t		gss_cred;
>  	OM_uint32		maj_stat, min_stat, lifetime_rec;
>  	pid_t			pid;
> +	gss_name_t		gacceptor;
> +	gss_OID			mech;
> +	gss_buffer_desc		acceptor  = {0};
>  
>  	pid = fork();
>  	switch(pid) {
> @@ -1174,15 +1183,22 @@ process_krb5_upcall(struct clnt_info *clp, uid_t uid, int fd, char *tgtname,
>  		goto out_return_error;
>  	}
>  
> -	/* Grab the context lifetime to pass to the kernel. lifetime_rec
> -	 * is set to zero on error */
> -	maj_stat = gss_inquire_context(&min_stat, pd.pd_ctx, NULL, NULL,
> -				       &lifetime_rec, NULL, NULL, NULL, NULL);
> +	maj_stat = gss_inquire_context(&min_stat, pd.pd_ctx, NULL, &gacceptor,
> +				       &lifetime_rec, &mech, NULL, NULL, NULL);
>  
> -	if (maj_stat)
> -		printerr(1, "WARNING: Failed to inquire context for lifetme "
> -			    "maj_stat %u\n", maj_stat);
> +	if (maj_stat != GSS_S_COMPLETE) {
> +		printerr(1, "WARNING: Failed to inquire context "
> +			    "maj_stat (0x%x)\n", maj_stat);
> +	} else {
> +		get_hostbased_client_buffer(gacceptor, mech, &acceptor);
> +		gss_release_name(&min_stat, &gacceptor);
> +	}
>  
> +	/*
> +	 * The serialization can mean turning the ctx into a lucid context. If
> +	 * that happens then the original ctx is no longer valid, so we mustn't
> +	 * try to use if after this point.
> +	 */
>  	if (serialize_context_for_kernel(&pd.pd_ctx, &token, &krb5oid, NULL)) {
>  		printerr(0, "WARNING: Failed to serialize krb5 context for "
>  			    "user with uid %d for server %s\n",
> @@ -1190,9 +1206,10 @@ process_krb5_upcall(struct clnt_info *clp, uid_t uid, int fd, char *tgtname,
>  		goto out_return_error;
>  	}
>  
> -	do_downcall(fd, uid, &pd, &token, lifetime_rec);
> +	do_downcall(fd, uid, &pd, &token, lifetime_rec, &acceptor);
>  
>  out:
> +	gss_release_buffer(&min_stat, &acceptor);
>  	if (token.value)
>  		free(token.value);
>  #ifdef HAVE_AUTHGSS_FREE_PRIVATE_DATA


-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH 5/5] gssd: scrape the acceptor name out of the context

[Steve Dickson]

Hey Jeff,

Just a couple nit.... 

On 04/10/2014 04:31 PM, Jeff Layton wrote:
> ...and pass it to the kernel in the downcall. Legacy kernels will just
> ignore the extra data, but with a proposed kernel patch the kernel will
> grab this info and use it to verify requests on the v4.0 callback
> channel.
> 
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
>  utils/gssd/gssd_proc.c | 39 ++++++++++++++++++++++++++++-----------
>  1 file changed, 28 insertions(+), 11 deletions(-)
> 
> diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
> index 7387cce010cf..d95e39416c28 100644
> --- a/utils/gssd/gssd_proc.c
> +++ b/utils/gssd/gssd_proc.c
> @@ -77,6 +77,7 @@
>  #include "context.h"
>  #include "nfsrpc.h"
>  #include "nfslib.h"
> +#include "gss_names.h"
>  
>  /*
>   * pollarray:
> @@ -683,16 +684,19 @@ parse_enctypes(char *enctypes)
>  
>  static void
>  do_downcall(int k5_fd, uid_t uid, struct authgss_private_data *pd,
> -	    gss_buffer_desc *context_token, OM_uint32 lifetime_rec)
> +	    gss_buffer_desc *context_token, OM_uint32 lifetime_rec,
> +	    gss_buffer_desc *acceptor)
>  {
>  	char    *buf = NULL, *p = NULL, *end = NULL;
>  	unsigned int timeout = context_timeout;
>  	unsigned int buf_size = 0;
>  
> -	printerr(1, "doing downcall lifetime_rec %u\n", lifetime_rec);
> +	printerr(1, "doing downcall: lifetime_rec=%u acceptor=%.*s\n",
> +		lifetime_rec, acceptor->length, acceptor->value);
>  	buf_size = sizeof(uid) + sizeof(timeout) + sizeof(pd->pd_seq_win) +
>  		sizeof(pd->pd_ctx_hndl.length) + pd->pd_ctx_hndl.length +
> -		sizeof(context_token->length) + context_token->length;
> +		sizeof(context_token->length) + context_token->length +
> +		acceptor->length;
>  	p = buf = malloc(buf_size);
>  	if (!buf)
>  		goto out_err;
> @@ -707,6 +711,8 @@ do_downcall(int k5_fd, uid_t uid, struct authgss_private_data *pd,
>  	if (WRITE_BYTES(&p, end, pd->pd_seq_win)) goto out_err;
>  	if (write_buffer(&p, end, &pd->pd_ctx_hndl)) goto out_err;
>  	if (write_buffer(&p, end, context_token)) goto out_err;
> +	if (acceptor->length > 0 &&
> +	    write_buffer(&p, end, acceptor)) goto out_err;
>  
>  	if (write(k5_fd, buf, p - buf) < p - buf) goto out_err;
>  	free(buf);
> @@ -1034,6 +1040,9 @@ process_krb5_upcall(struct clnt_info *clp, uid_t uid, int fd, char *tgtname,
>  	gss_cred_id_t		gss_cred;
>  	OM_uint32		maj_stat, min_stat, lifetime_rec;
>  	pid_t			pid;
> +	gss_name_t		gacceptor;
> +	gss_OID			mech;
> +	gss_buffer_desc		acceptor  = {0};
>  
>  	pid = fork();
>  	switch(pid) {
> @@ -1174,15 +1183,22 @@ process_krb5_upcall(struct clnt_info *clp, uid_t uid, int fd, char *tgtname,
>  		goto out_return_error;
>  	}
>  
> -	/* Grab the context lifetime to pass to the kernel. lifetime_rec
> -	 * is set to zero on error */
Why get rid of this comment instead of updating it? 

> -	maj_stat = gss_inquire_context(&min_stat, pd.pd_ctx, NULL, NULL,
> -				       &lifetime_rec, NULL, NULL, NULL, NULL);
> +	maj_stat = gss_inquire_context(&min_stat, pd.pd_ctx, NULL, &gacceptor,
> +				       &lifetime_rec, &mech, NULL, NULL, NULL);
>  
> -	if (maj_stat)
> -		printerr(1, "WARNING: Failed to inquire context for lifetme "
> -			    "maj_stat %u\n", maj_stat);
> +	if (maj_stat != GSS_S_COMPLETE) {
> +		printerr(1, "WARNING: Failed to inquire context "
> +			    "maj_stat (0x%x)\n", maj_stat);
> +	} else {
> +		get_hostbased_client_buffer(gacceptor, mech, &acceptor);
> +		gss_release_name(&min_stat, &gacceptor);
> +	}
>  
> +	/*
> +	 * The serialization can mean turning the ctx into a lucid context. If
> +	 * that happens then the original ctx is no longer valid, so we mustn't
> +	 * try to use if after this point.
I'm not sure what you are trying to say here... 

steved.

> +	 */
>  	if (serialize_context_for_kernel(&pd.pd_ctx, &token, &krb5oid, NULL)) {
>  		printerr(0, "WARNING: Failed to serialize krb5 context for "
>  			    "user with uid %d for server %s\n",
> @@ -1190,9 +1206,10 @@ process_krb5_upcall(struct clnt_info *clp, uid_t uid, int fd, char *tgtname,
>  		goto out_return_error;
>  	}
>  
> -	do_downcall(fd, uid, &pd, &token, lifetime_rec);
> +	do_downcall(fd, uid, &pd, &token, lifetime_rec, &acceptor);
>  
>  out:
> +	gss_release_buffer(&min_stat, &acceptor);
>  	if (token.value)
>  		free(token.value);
>  #ifdef HAVE_AUTHGSS_FREE_PRIVATE_DATA
> 

Re: [PATCH 5/5] gssd: scrape the acceptor name out of the context

[Jeff Layton]

On Mon, 14 Apr 2014 11:07:37 -0400
Steve Dickson <SteveD@redhat.com> wrote:

> Hey Jeff,
> 
> Just a couple nit.... 
> 
> On 04/10/2014 04:31 PM, Jeff Layton wrote:
> > ...and pass it to the kernel in the downcall. Legacy kernels will just
> > ignore the extra data, but with a proposed kernel patch the kernel will
> > grab this info and use it to verify requests on the v4.0 callback
> > channel.
> > 
> > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > ---
> >  utils/gssd/gssd_proc.c | 39 ++++++++++++++++++++++++++++-----------
> >  1 file changed, 28 insertions(+), 11 deletions(-)
> > 
> > diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
> > index 7387cce010cf..d95e39416c28 100644
> > --- a/utils/gssd/gssd_proc.c
> > +++ b/utils/gssd/gssd_proc.c
> > @@ -77,6 +77,7 @@
> >  #include "context.h"
> >  #include "nfsrpc.h"
> >  #include "nfslib.h"
> > +#include "gss_names.h"
> >  
> >  /*
> >   * pollarray:
> > @@ -683,16 +684,19 @@ parse_enctypes(char *enctypes)
> >  
> >  static void
> >  do_downcall(int k5_fd, uid_t uid, struct authgss_private_data *pd,
> > -	    gss_buffer_desc *context_token, OM_uint32 lifetime_rec)
> > +	    gss_buffer_desc *context_token, OM_uint32 lifetime_rec,
> > +	    gss_buffer_desc *acceptor)
> >  {
> >  	char    *buf = NULL, *p = NULL, *end = NULL;
> >  	unsigned int timeout = context_timeout;
> >  	unsigned int buf_size = 0;
> >  
> > -	printerr(1, "doing downcall lifetime_rec %u\n", lifetime_rec);
> > +	printerr(1, "doing downcall: lifetime_rec=%u acceptor=%.*s\n",
> > +		lifetime_rec, acceptor->length, acceptor->value);
> >  	buf_size = sizeof(uid) + sizeof(timeout) + sizeof(pd->pd_seq_win) +
> >  		sizeof(pd->pd_ctx_hndl.length) + pd->pd_ctx_hndl.length +
> > -		sizeof(context_token->length) + context_token->length;
> > +		sizeof(context_token->length) + context_token->length +
> > +		acceptor->length;
> >  	p = buf = malloc(buf_size);
> >  	if (!buf)
> >  		goto out_err;
> > @@ -707,6 +711,8 @@ do_downcall(int k5_fd, uid_t uid, struct authgss_private_data *pd,
> >  	if (WRITE_BYTES(&p, end, pd->pd_seq_win)) goto out_err;
> >  	if (write_buffer(&p, end, &pd->pd_ctx_hndl)) goto out_err;
> >  	if (write_buffer(&p, end, context_token)) goto out_err;
> > +	if (acceptor->length > 0 &&
> > +	    write_buffer(&p, end, acceptor)) goto out_err;
> >  
> >  	if (write(k5_fd, buf, p - buf) < p - buf) goto out_err;
> >  	free(buf);
> > @@ -1034,6 +1040,9 @@ process_krb5_upcall(struct clnt_info *clp, uid_t uid, int fd, char *tgtname,
> >  	gss_cred_id_t		gss_cred;
> >  	OM_uint32		maj_stat, min_stat, lifetime_rec;
> >  	pid_t			pid;
> > +	gss_name_t		gacceptor;
> > +	gss_OID			mech;
> > +	gss_buffer_desc		acceptor  = {0};
> >  
> >  	pid = fork();
> >  	switch(pid) {
> > @@ -1174,15 +1183,22 @@ process_krb5_upcall(struct clnt_info *clp, uid_t uid, int fd, char *tgtname,
> >  		goto out_return_error;
> >  	}
> >  
> > -	/* Grab the context lifetime to pass to the kernel. lifetime_rec
> > -	 * is set to zero on error */
> Why get rid of this comment instead of updating it? 
> 

I suppose I could do that. Would you prefer I respin and update that
comment instead?

> > -	maj_stat = gss_inquire_context(&min_stat, pd.pd_ctx, NULL, NULL,
> > -				       &lifetime_rec, NULL, NULL, NULL, NULL);
> > +	maj_stat = gss_inquire_context(&min_stat, pd.pd_ctx, NULL, &gacceptor,
> > +				       &lifetime_rec, &mech, NULL, NULL, NULL);
> >  
> > -	if (maj_stat)
> > -		printerr(1, "WARNING: Failed to inquire context for lifetme "
> > -			    "maj_stat %u\n", maj_stat);
> > +	if (maj_stat != GSS_S_COMPLETE) {
> > +		printerr(1, "WARNING: Failed to inquire context "
> > +			    "maj_stat (0x%x)\n", maj_stat);
> > +	} else {
> > +		get_hostbased_client_buffer(gacceptor, mech, &acceptor);
> > +		gss_release_name(&min_stat, &gacceptor);
> > +	}
> >  
> > +	/*
> > +	 * The serialization can mean turning the ctx into a lucid context. If
> > +	 * that happens then the original ctx is no longer valid, so we mustn't
> > +	 * try to use if after this point.
> I'm not sure what you are trying to say here... 
> 
> steved.
> 

Just a semi-helpful context for posterity...

On my initial attempt, I tried to change the code around to grab the
lifetime and acceptor out of the context in do_downcall. That didn't
work, because once you call serialize_context_for_kernel, the GSS
context is turned into a "lucid context". At that point, things like
gss_inquire_context no longer work on it.

IOW, once you call serialize_context_for_kernel, you can't count on
being able to do anything useful with pd.pd_ctx at all.


> > +	 */
> >  	if (serialize_context_for_kernel(&pd.pd_ctx, &token, &krb5oid, NULL)) {
> >  		printerr(0, "WARNING: Failed to serialize krb5 context for "
> >  			    "user with uid %d for server %s\n",
> > @@ -1190,9 +1206,10 @@ process_krb5_upcall(struct clnt_info *clp, uid_t uid, int fd, char *tgtname,
> >  		goto out_return_error;
> >  	}
> >  
> > -	do_downcall(fd, uid, &pd, &token, lifetime_rec);
> > +	do_downcall(fd, uid, &pd, &token, lifetime_rec, &acceptor);
> >  
> >  out:
> > +	gss_release_buffer(&min_stat, &acceptor);
> >  	if (token.value)
> >  		free(token.value);
> >  #ifdef HAVE_AUTHGSS_FREE_PRIVATE_DATA
> > 



-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH 5/5] gssd: scrape the acceptor name out of the context

[Steve Dickson]

On 04/14/2014 11:48 AM, Jeff Layton wrote:
> On Mon, 14 Apr 2014 11:07:37 -0400
> Steve Dickson <SteveD@redhat.com> wrote:
> 
>> Hey Jeff,
>>
>> Just a couple nit.... 
>>
>> On 04/10/2014 04:31 PM, Jeff Layton wrote:
>>> ...and pass it to the kernel in the downcall. Legacy kernels will just
>>> ignore the extra data, but with a proposed kernel patch the kernel will
>>> grab this info and use it to verify requests on the v4.0 callback
>>> channel.
>>>
>>> Signed-off-by: Jeff Layton <jlayton@redhat.com>
>>> ---
>>>  utils/gssd/gssd_proc.c | 39 ++++++++++++++++++++++++++++-----------
>>>  1 file changed, 28 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
>>> index 7387cce010cf..d95e39416c28 100644
>>> --- a/utils/gssd/gssd_proc.c
>>> +++ b/utils/gssd/gssd_proc.c
>>> @@ -77,6 +77,7 @@
>>>  #include "context.h"
>>>  #include "nfsrpc.h"
>>>  #include "nfslib.h"
>>> +#include "gss_names.h"
>>>  
>>>  /*
>>>   * pollarray:
>>> @@ -683,16 +684,19 @@ parse_enctypes(char *enctypes)
>>>  
>>>  static void
>>>  do_downcall(int k5_fd, uid_t uid, struct authgss_private_data *pd,
>>> -	    gss_buffer_desc *context_token, OM_uint32 lifetime_rec)
>>> +	    gss_buffer_desc *context_token, OM_uint32 lifetime_rec,
>>> +	    gss_buffer_desc *acceptor)
>>>  {
>>>  	char    *buf = NULL, *p = NULL, *end = NULL;
>>>  	unsigned int timeout = context_timeout;
>>>  	unsigned int buf_size = 0;
>>>  
>>> -	printerr(1, "doing downcall lifetime_rec %u\n", lifetime_rec);
>>> +	printerr(1, "doing downcall: lifetime_rec=%u acceptor=%.*s\n",
>>> +		lifetime_rec, acceptor->length, acceptor->value);
>>>  	buf_size = sizeof(uid) + sizeof(timeout) + sizeof(pd->pd_seq_win) +
>>>  		sizeof(pd->pd_ctx_hndl.length) + pd->pd_ctx_hndl.length +
>>> -		sizeof(context_token->length) + context_token->length;
>>> +		sizeof(context_token->length) + context_token->length +
>>> +		acceptor->length;
>>>  	p = buf = malloc(buf_size);
>>>  	if (!buf)
>>>  		goto out_err;
>>> @@ -707,6 +711,8 @@ do_downcall(int k5_fd, uid_t uid, struct authgss_private_data *pd,
>>>  	if (WRITE_BYTES(&p, end, pd->pd_seq_win)) goto out_err;
>>>  	if (write_buffer(&p, end, &pd->pd_ctx_hndl)) goto out_err;
>>>  	if (write_buffer(&p, end, context_token)) goto out_err;
>>> +	if (acceptor->length > 0 &&
>>> +	    write_buffer(&p, end, acceptor)) goto out_err;
>>>  
>>>  	if (write(k5_fd, buf, p - buf) < p - buf) goto out_err;
>>>  	free(buf);
>>> @@ -1034,6 +1040,9 @@ process_krb5_upcall(struct clnt_info *clp, uid_t uid, int fd, char *tgtname,
>>>  	gss_cred_id_t		gss_cred;
>>>  	OM_uint32		maj_stat, min_stat, lifetime_rec;
>>>  	pid_t			pid;
>>> +	gss_name_t		gacceptor;
>>> +	gss_OID			mech;
>>> +	gss_buffer_desc		acceptor  = {0};
>>>  
>>>  	pid = fork();
>>>  	switch(pid) {
>>> @@ -1174,15 +1183,22 @@ process_krb5_upcall(struct clnt_info *clp, uid_t uid, int fd, char *tgtname,
>>>  		goto out_return_error;
>>>  	}
>>>  
>>> -	/* Grab the context lifetime to pass to the kernel. lifetime_rec
>>> -	 * is set to zero on error */
>> Why get rid of this comment instead of updating it? 
>>
> 
> I suppose I could do that. Would you prefer I respin and update that
> comment instead?
If you would not mind.... I may not get to it for a while... I'm
going to be traveling this week...

thanks!

steved.

> 
>>> -	maj_stat = gss_inquire_context(&min_stat, pd.pd_ctx, NULL, NULL,
>>> -				       &lifetime_rec, NULL, NULL, NULL, NULL);
>>> +	maj_stat = gss_inquire_context(&min_stat, pd.pd_ctx, NULL, &gacceptor,
>>> +				       &lifetime_rec, &mech, NULL, NULL, NULL);
>>>  
>>> -	if (maj_stat)
>>> -		printerr(1, "WARNING: Failed to inquire context for lifetme "
>>> -			    "maj_stat %u\n", maj_stat);
>>> +	if (maj_stat != GSS_S_COMPLETE) {
>>> +		printerr(1, "WARNING: Failed to inquire context "
>>> +			    "maj_stat (0x%x)\n", maj_stat);
>>> +	} else {
>>> +		get_hostbased_client_buffer(gacceptor, mech, &acceptor);
>>> +		gss_release_name(&min_stat, &gacceptor);
>>> +	}
>>>  
>>> +	/*
>>> +	 * The serialization can mean turning the ctx into a lucid context. If
>>> +	 * that happens then the original ctx is no longer valid, so we mustn't
>>> +	 * try to use if after this point.
>> I'm not sure what you are trying to say here... 
>>
>> steved.
>>
> 
> Just a semi-helpful context for posterity...
> 
> On my initial attempt, I tried to change the code around to grab the
> lifetime and acceptor out of the context in do_downcall. That didn't
> work, because once you call serialize_context_for_kernel, the GSS
> context is turned into a "lucid context". At that point, things like
> gss_inquire_context no longer work on it.
> 
> IOW, once you call serialize_context_for_kernel, you can't count on
> being able to do anything useful with pd.pd_ctx at all.
> 
> 
>>> +	 */
>>>  	if (serialize_context_for_kernel(&pd.pd_ctx, &token, &krb5oid, NULL)) {
>>>  		printerr(0, "WARNING: Failed to serialize krb5 context for "
>>>  			    "user with uid %d for server %s\n",
>>> @@ -1190,9 +1206,10 @@ process_krb5_upcall(struct clnt_info *clp, uid_t uid, int fd, char *tgtname,
>>>  		goto out_return_error;
>>>  	}
>>>  
>>> -	do_downcall(fd, uid, &pd, &token, lifetime_rec);
>>> +	do_downcall(fd, uid, &pd, &token, lifetime_rec, &acceptor);
>>>  
>>>  out:
>>> +	gss_release_buffer(&min_stat, &acceptor);
>>>  	if (token.value)
>>>  		free(token.value);
>>>  #ifdef HAVE_AUTHGSS_FREE_PRIVATE_DATA
>>>
> 
> 
> 

Re: [PATCH 5/5] gssd: scrape the acceptor name out of the context

[Simo Sorce]

On Mon, 2014-04-14 at 11:48 -0400, Jeff Layton wrote:
> On Mon, 14 Apr 2014 11:07:37 -0400
> Steve Dickson <SteveD@redhat.com> wrote:
> 
> > Hey Jeff,
> > 
> > Just a couple nit.... 
> > 
> > On 04/10/2014 04:31 PM, Jeff Layton wrote:
> > > ...and pass it to the kernel in the downcall. Legacy kernels will just
> > > ignore the extra data, but with a proposed kernel patch the kernel will
> > > grab this info and use it to verify requests on the v4.0 callback
> > > channel.
> > > 
> > > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > > ---
> > >  utils/gssd/gssd_proc.c | 39 ++++++++++++++++++++++++++++-----------
> > >  1 file changed, 28 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
> > > index 7387cce010cf..d95e39416c28 100644
> > > --- a/utils/gssd/gssd_proc.c
> > > +++ b/utils/gssd/gssd_proc.c
> > > @@ -77,6 +77,7 @@
> > >  #include "context.h"
> > >  #include "nfsrpc.h"
> > >  #include "nfslib.h"
> > > +#include "gss_names.h"
> > >  
> > >  /*
> > >   * pollarray:
> > > @@ -683,16 +684,19 @@ parse_enctypes(char *enctypes)
> > >  
> > >  static void
> > >  do_downcall(int k5_fd, uid_t uid, struct authgss_private_data *pd,
> > > -	    gss_buffer_desc *context_token, OM_uint32 lifetime_rec)
> > > +	    gss_buffer_desc *context_token, OM_uint32 lifetime_rec,
> > > +	    gss_buffer_desc *acceptor)
> > >  {
> > >  	char    *buf = NULL, *p = NULL, *end = NULL;
> > >  	unsigned int timeout = context_timeout;
> > >  	unsigned int buf_size = 0;
> > >  
> > > -	printerr(1, "doing downcall lifetime_rec %u\n", lifetime_rec);
> > > +	printerr(1, "doing downcall: lifetime_rec=%u acceptor=%.*s\n",
> > > +		lifetime_rec, acceptor->length, acceptor->value);
> > >  	buf_size = sizeof(uid) + sizeof(timeout) + sizeof(pd->pd_seq_win) +
> > >  		sizeof(pd->pd_ctx_hndl.length) + pd->pd_ctx_hndl.length +
> > > -		sizeof(context_token->length) + context_token->length;
> > > +		sizeof(context_token->length) + context_token->length +
> > > +		acceptor->length;
> > >  	p = buf = malloc(buf_size);
> > >  	if (!buf)
> > >  		goto out_err;
> > > @@ -707,6 +711,8 @@ do_downcall(int k5_fd, uid_t uid, struct authgss_private_data *pd,
> > >  	if (WRITE_BYTES(&p, end, pd->pd_seq_win)) goto out_err;
> > >  	if (write_buffer(&p, end, &pd->pd_ctx_hndl)) goto out_err;
> > >  	if (write_buffer(&p, end, context_token)) goto out_err;
> > > +	if (acceptor->length > 0 &&
> > > +	    write_buffer(&p, end, acceptor)) goto out_err;
> > >  
> > >  	if (write(k5_fd, buf, p - buf) < p - buf) goto out_err;
> > >  	free(buf);
> > > @@ -1034,6 +1040,9 @@ process_krb5_upcall(struct clnt_info *clp, uid_t uid, int fd, char *tgtname,
> > >  	gss_cred_id_t		gss_cred;
> > >  	OM_uint32		maj_stat, min_stat, lifetime_rec;
> > >  	pid_t			pid;
> > > +	gss_name_t		gacceptor;
> > > +	gss_OID			mech;
> > > +	gss_buffer_desc		acceptor  = {0};
> > >  
> > >  	pid = fork();
> > >  	switch(pid) {
> > > @@ -1174,15 +1183,22 @@ process_krb5_upcall(struct clnt_info *clp, uid_t uid, int fd, char *tgtname,
> > >  		goto out_return_error;
> > >  	}
> > >  
> > > -	/* Grab the context lifetime to pass to the kernel. lifetime_rec
> > > -	 * is set to zero on error */
> > Why get rid of this comment instead of updating it? 
> > 
> 
> I suppose I could do that. Would you prefer I respin and update that
> comment instead?
> 
> > > -	maj_stat = gss_inquire_context(&min_stat, pd.pd_ctx, NULL, NULL,
> > > -				       &lifetime_rec, NULL, NULL, NULL, NULL);
> > > +	maj_stat = gss_inquire_context(&min_stat, pd.pd_ctx, NULL, &gacceptor,
> > > +				       &lifetime_rec, &mech, NULL, NULL, NULL);
> > >  
> > > -	if (maj_stat)
> > > -		printerr(1, "WARNING: Failed to inquire context for lifetme "
> > > -			    "maj_stat %u\n", maj_stat);
> > > +	if (maj_stat != GSS_S_COMPLETE) {
> > > +		printerr(1, "WARNING: Failed to inquire context "
> > > +			    "maj_stat (0x%x)\n", maj_stat);
> > > +	} else {
> > > +		get_hostbased_client_buffer(gacceptor, mech, &acceptor);
> > > +		gss_release_name(&min_stat, &gacceptor);
> > > +	}
> > >  
> > > +	/*
> > > +	 * The serialization can mean turning the ctx into a lucid context. If
> > > +	 * that happens then the original ctx is no longer valid, so we mustn't
> > > +	 * try to use if after this point.
> > I'm not sure what you are trying to say here... 
> > 
> > steved.
> > 
> 
> Just a semi-helpful context for posterity...
> 
> On my initial attempt, I tried to change the code around to grab the
> lifetime and acceptor out of the context in do_downcall. That didn't
> work, because once you call serialize_context_for_kernel, the GSS
> context is turned into a "lucid context". At that point, things like
> gss_inquire_context no longer work on it.
> 
> IOW, once you call serialize_context_for_kernel, you can't count on
> being able to do anything useful with pd.pd_ctx at all.

This is on purpose, when you export the lucid context, the original
context is destroyed, this is to avoid having 2 parties with a context
for the same connection as the sequence numbers would get messed up and
communication fail if 2 parties tried to use them at the same time
anyway.

Simo.

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

Re: [PATCH 5/5] gssd: scrape the acceptor name out of the context

[Jeff Layton]

On Mon, 14 Apr 2014 14:36:34 -0400
Simo Sorce <simo@redhat.com> wrote:

> On Mon, 2014-04-14 at 11:48 -0400, Jeff Layton wrote:
> > On Mon, 14 Apr 2014 11:07:37 -0400
> > Steve Dickson <SteveD@redhat.com> wrote:
> > 
> > > Hey Jeff,
> > > 
> > > Just a couple nit.... 
> > > 
> > > On 04/10/2014 04:31 PM, Jeff Layton wrote:
> > > > ...and pass it to the kernel in the downcall. Legacy kernels will just
> > > > ignore the extra data, but with a proposed kernel patch the kernel will
> > > > grab this info and use it to verify requests on the v4.0 callback
> > > > channel.
> > > > 
> > > > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > > > ---
> > > >  utils/gssd/gssd_proc.c | 39 ++++++++++++++++++++++++++++-----------
> > > >  1 file changed, 28 insertions(+), 11 deletions(-)
> > > > 
> > > > diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
> > > > index 7387cce010cf..d95e39416c28 100644
> > > > --- a/utils/gssd/gssd_proc.c
> > > > +++ b/utils/gssd/gssd_proc.c
> > > > @@ -77,6 +77,7 @@
> > > >  #include "context.h"
> > > >  #include "nfsrpc.h"
> > > >  #include "nfslib.h"
> > > > +#include "gss_names.h"
> > > >  
> > > >  /*
> > > >   * pollarray:
> > > > @@ -683,16 +684,19 @@ parse_enctypes(char *enctypes)
> > > >  
> > > >  static void
> > > >  do_downcall(int k5_fd, uid_t uid, struct authgss_private_data *pd,
> > > > -	    gss_buffer_desc *context_token, OM_uint32 lifetime_rec)
> > > > +	    gss_buffer_desc *context_token, OM_uint32 lifetime_rec,
> > > > +	    gss_buffer_desc *acceptor)
> > > >  {
> > > >  	char    *buf = NULL, *p = NULL, *end = NULL;
> > > >  	unsigned int timeout = context_timeout;
> > > >  	unsigned int buf_size = 0;
> > > >  
> > > > -	printerr(1, "doing downcall lifetime_rec %u\n", lifetime_rec);
> > > > +	printerr(1, "doing downcall: lifetime_rec=%u acceptor=%.*s\n",
> > > > +		lifetime_rec, acceptor->length, acceptor->value);
> > > >  	buf_size = sizeof(uid) + sizeof(timeout) + sizeof(pd->pd_seq_win) +
> > > >  		sizeof(pd->pd_ctx_hndl.length) + pd->pd_ctx_hndl.length +
> > > > -		sizeof(context_token->length) + context_token->length;
> > > > +		sizeof(context_token->length) + context_token->length +
> > > > +		acceptor->length;
> > > >  	p = buf = malloc(buf_size);
> > > >  	if (!buf)
> > > >  		goto out_err;
> > > > @@ -707,6 +711,8 @@ do_downcall(int k5_fd, uid_t uid, struct authgss_private_data *pd,
> > > >  	if (WRITE_BYTES(&p, end, pd->pd_seq_win)) goto out_err;
> > > >  	if (write_buffer(&p, end, &pd->pd_ctx_hndl)) goto out_err;
> > > >  	if (write_buffer(&p, end, context_token)) goto out_err;
> > > > +	if (acceptor->length > 0 &&
> > > > +	    write_buffer(&p, end, acceptor)) goto out_err;
> > > >  
> > > >  	if (write(k5_fd, buf, p - buf) < p - buf) goto out_err;
> > > >  	free(buf);
> > > > @@ -1034,6 +1040,9 @@ process_krb5_upcall(struct clnt_info *clp, uid_t uid, int fd, char *tgtname,
> > > >  	gss_cred_id_t		gss_cred;
> > > >  	OM_uint32		maj_stat, min_stat, lifetime_rec;
> > > >  	pid_t			pid;
> > > > +	gss_name_t		gacceptor;
> > > > +	gss_OID			mech;
> > > > +	gss_buffer_desc		acceptor  = {0};
> > > >  
> > > >  	pid = fork();
> > > >  	switch(pid) {
> > > > @@ -1174,15 +1183,22 @@ process_krb5_upcall(struct clnt_info *clp, uid_t uid, int fd, char *tgtname,
> > > >  		goto out_return_error;
> > > >  	}
> > > >  
> > > > -	/* Grab the context lifetime to pass to the kernel. lifetime_rec
> > > > -	 * is set to zero on error */
> > > Why get rid of this comment instead of updating it? 
> > > 
> > 
> > I suppose I could do that. Would you prefer I respin and update that
> > comment instead?
> > 
> > > > -	maj_stat = gss_inquire_context(&min_stat, pd.pd_ctx, NULL, NULL,
> > > > -				       &lifetime_rec, NULL, NULL, NULL, NULL);
> > > > +	maj_stat = gss_inquire_context(&min_stat, pd.pd_ctx, NULL, &gacceptor,
> > > > +				       &lifetime_rec, &mech, NULL, NULL, NULL);
> > > >  
> > > > -	if (maj_stat)
> > > > -		printerr(1, "WARNING: Failed to inquire context for lifetme "
> > > > -			    "maj_stat %u\n", maj_stat);
> > > > +	if (maj_stat != GSS_S_COMPLETE) {
> > > > +		printerr(1, "WARNING: Failed to inquire context "
> > > > +			    "maj_stat (0x%x)\n", maj_stat);
> > > > +	} else {
> > > > +		get_hostbased_client_buffer(gacceptor, mech, &acceptor);
> > > > +		gss_release_name(&min_stat, &gacceptor);
> > > > +	}
> > > >  
> > > > +	/*
> > > > +	 * The serialization can mean turning the ctx into a lucid context. If
> > > > +	 * that happens then the original ctx is no longer valid, so we mustn't
> > > > +	 * try to use if after this point.
> > > I'm not sure what you are trying to say here... 
> > > 
> > > steved.
> > > 
> > 
> > Just a semi-helpful context for posterity...
> > 
> > On my initial attempt, I tried to change the code around to grab the
> > lifetime and acceptor out of the context in do_downcall. That didn't
> > work, because once you call serialize_context_for_kernel, the GSS
> > context is turned into a "lucid context". At that point, things like
> > gss_inquire_context no longer work on it.
> > 
> > IOW, once you call serialize_context_for_kernel, you can't count on
> > being able to do anything useful with pd.pd_ctx at all.
> 
> This is on purpose, when you export the lucid context, the original
> context is destroyed, this is to avoid having 2 parties with a context
> for the same connection as the sequence numbers would get messed up and
> communication fail if 2 parties tried to use them at the same time
> anyway.
> 

Yes, I gathered that it's intentional behavior. You get back an error
that basically means that the context handle is no longer valid when
you try to use it. It's just not obvious that that's the case at this
point in the code, so I figured it warranted a comment.

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH 5/5] gssd: scrape the acceptor name out of the context

[Simo Sorce]

On Mon, 2014-04-14 at 14:44 -0400, Jeff Layton wrote:
> On Mon, 14 Apr 2014 14:36:34 -0400
> Simo Sorce <simo@redhat.com> wrote:
> 
> > On Mon, 2014-04-14 at 11:48 -0400, Jeff Layton wrote:
> > > On Mon, 14 Apr 2014 11:07:37 -0400
> > > Steve Dickson <SteveD@redhat.com> wrote:
> > > 
> > > > Hey Jeff,
> > > > 
> > > > Just a couple nit.... 
> > > > 
> > > > On 04/10/2014 04:31 PM, Jeff Layton wrote:
> > > > > ...and pass it to the kernel in the downcall. Legacy kernels will just
> > > > > ignore the extra data, but with a proposed kernel patch the kernel will
> > > > > grab this info and use it to verify requests on the v4.0 callback
> > > > > channel.
> > > > > 
> > > > > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > > > > ---
> > > > >  utils/gssd/gssd_proc.c | 39 ++++++++++++++++++++++++++++-----------
> > > > >  1 file changed, 28 insertions(+), 11 deletions(-)
> > > > > 
> > > > > diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
> > > > > index 7387cce010cf..d95e39416c28 100644
> > > > > --- a/utils/gssd/gssd_proc.c
> > > > > +++ b/utils/gssd/gssd_proc.c
> > > > > @@ -77,6 +77,7 @@
> > > > >  #include "context.h"
> > > > >  #include "nfsrpc.h"
> > > > >  #include "nfslib.h"
> > > > > +#include "gss_names.h"
> > > > >  
> > > > >  /*
> > > > >   * pollarray:
> > > > > @@ -683,16 +684,19 @@ parse_enctypes(char *enctypes)
> > > > >  
> > > > >  static void
> > > > >  do_downcall(int k5_fd, uid_t uid, struct authgss_private_data *pd,
> > > > > -	    gss_buffer_desc *context_token, OM_uint32 lifetime_rec)
> > > > > +	    gss_buffer_desc *context_token, OM_uint32 lifetime_rec,
> > > > > +	    gss_buffer_desc *acceptor)
> > > > >  {
> > > > >  	char    *buf = NULL, *p = NULL, *end = NULL;
> > > > >  	unsigned int timeout = context_timeout;
> > > > >  	unsigned int buf_size = 0;
> > > > >  
> > > > > -	printerr(1, "doing downcall lifetime_rec %u\n", lifetime_rec);
> > > > > +	printerr(1, "doing downcall: lifetime_rec=%u acceptor=%.*s\n",
> > > > > +		lifetime_rec, acceptor->length, acceptor->value);
> > > > >  	buf_size = sizeof(uid) + sizeof(timeout) + sizeof(pd->pd_seq_win) +
> > > > >  		sizeof(pd->pd_ctx_hndl.length) + pd->pd_ctx_hndl.length +
> > > > > -		sizeof(context_token->length) + context_token->length;
> > > > > +		sizeof(context_token->length) + context_token->length +
> > > > > +		acceptor->length;
> > > > >  	p = buf = malloc(buf_size);
> > > > >  	if (!buf)
> > > > >  		goto out_err;
> > > > > @@ -707,6 +711,8 @@ do_downcall(int k5_fd, uid_t uid, struct authgss_private_data *pd,
> > > > >  	if (WRITE_BYTES(&p, end, pd->pd_seq_win)) goto out_err;
> > > > >  	if (write_buffer(&p, end, &pd->pd_ctx_hndl)) goto out_err;
> > > > >  	if (write_buffer(&p, end, context_token)) goto out_err;
> > > > > +	if (acceptor->length > 0 &&
> > > > > +	    write_buffer(&p, end, acceptor)) goto out_err;
> > > > >  
> > > > >  	if (write(k5_fd, buf, p - buf) < p - buf) goto out_err;
> > > > >  	free(buf);
> > > > > @@ -1034,6 +1040,9 @@ process_krb5_upcall(struct clnt_info *clp, uid_t uid, int fd, char *tgtname,
> > > > >  	gss_cred_id_t		gss_cred;
> > > > >  	OM_uint32		maj_stat, min_stat, lifetime_rec;
> > > > >  	pid_t			pid;
> > > > > +	gss_name_t		gacceptor;
> > > > > +	gss_OID			mech;
> > > > > +	gss_buffer_desc		acceptor  = {0};
> > > > >  
> > > > >  	pid = fork();
> > > > >  	switch(pid) {
> > > > > @@ -1174,15 +1183,22 @@ process_krb5_upcall(struct clnt_info *clp, uid_t uid, int fd, char *tgtname,
> > > > >  		goto out_return_error;
> > > > >  	}
> > > > >  
> > > > > -	/* Grab the context lifetime to pass to the kernel. lifetime_rec
> > > > > -	 * is set to zero on error */
> > > > Why get rid of this comment instead of updating it? 
> > > > 
> > > 
> > > I suppose I could do that. Would you prefer I respin and update that
> > > comment instead?
> > > 
> > > > > -	maj_stat = gss_inquire_context(&min_stat, pd.pd_ctx, NULL, NULL,
> > > > > -				       &lifetime_rec, NULL, NULL, NULL, NULL);
> > > > > +	maj_stat = gss_inquire_context(&min_stat, pd.pd_ctx, NULL, &gacceptor,
> > > > > +				       &lifetime_rec, &mech, NULL, NULL, NULL);
> > > > >  
> > > > > -	if (maj_stat)
> > > > > -		printerr(1, "WARNING: Failed to inquire context for lifetme "
> > > > > -			    "maj_stat %u\n", maj_stat);
> > > > > +	if (maj_stat != GSS_S_COMPLETE) {
> > > > > +		printerr(1, "WARNING: Failed to inquire context "
> > > > > +			    "maj_stat (0x%x)\n", maj_stat);
> > > > > +	} else {
> > > > > +		get_hostbased_client_buffer(gacceptor, mech, &acceptor);
> > > > > +		gss_release_name(&min_stat, &gacceptor);
> > > > > +	}
> > > > >  
> > > > > +	/*
> > > > > +	 * The serialization can mean turning the ctx into a lucid context. If
> > > > > +	 * that happens then the original ctx is no longer valid, so we mustn't
> > > > > +	 * try to use if after this point.
> > > > I'm not sure what you are trying to say here... 
> > > > 
> > > > steved.
> > > > 
> > > 
> > > Just a semi-helpful context for posterity...
> > > 
> > > On my initial attempt, I tried to change the code around to grab the
> > > lifetime and acceptor out of the context in do_downcall. That didn't
> > > work, because once you call serialize_context_for_kernel, the GSS
> > > context is turned into a "lucid context". At that point, things like
> > > gss_inquire_context no longer work on it.
> > > 
> > > IOW, once you call serialize_context_for_kernel, you can't count on
> > > being able to do anything useful with pd.pd_ctx at all.
> > 
> > This is on purpose, when you export the lucid context, the original
> > context is destroyed, this is to avoid having 2 parties with a context
> > for the same connection as the sequence numbers would get messed up and
> > communication fail if 2 parties tried to use them at the same time
> > anyway.
> > 
> 
> Yes, I gathered that it's intentional behavior. You get back an error
> that basically means that the context handle is no longer valid when
> you try to use it. It's just not obvious that that's the case at this
> point in the code, so I figured it warranted a comment.

Yes it is a good thing, or people that are unaware of the export
semantic may get confused.

Simo.

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