Return-Path: linux-nfs-owner@vger.kernel.org Received: from mx1.redhat.com ([209.132.183.28]:56613 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754574AbaDNSol (ORCPT ); Mon, 14 Apr 2014 14:44:41 -0400 Date: Mon, 14 Apr 2014 14:44:38 -0400 From: Jeff Layton To: Simo Sorce Cc: Steve Dickson , trond.myklebust@primarydata.com, linux-nfs@vger.kernel.org Subject: Re: [PATCH 5/5] gssd: scrape the acceptor name out of the context Message-ID: <20140414144438.5afa19c9@ipyr.poochiereds.net> In-Reply-To: <1397500594.19767.258.camel@willson.li.ssimo.org> References: <1397161863-29266-1-git-send-email-jlayton@redhat.com> <1397161863-29266-6-git-send-email-jlayton@redhat.com> <534BF9B9.3000707@RedHat.com> <20140414114851.40dcbcfe@ipyr.poochiereds.net> <1397500594.19767.258.camel@willson.li.ssimo.org> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Sender: linux-nfs-owner@vger.kernel.org List-ID: On Mon, 14 Apr 2014 14:36:34 -0400 Simo Sorce wrote: > On Mon, 2014-04-14 at 11:48 -0400, Jeff Layton wrote: > > On Mon, 14 Apr 2014 11:07:37 -0400 > > Steve Dickson 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 > > > > --- > > > > 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