| Message ID | 1402342401-5640-4-git-send-email-andros@netapp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 2014-06-09 at 15:33 -0400, andros@netapp.com wrote: > From: Andy Adamson <andros@netapp.com> > > The current code returns an RPC_AUTH_GSS pseudoflavor without testing to see > if it is configured properly. If an RPC_AUTH_GSS pseudoflavor fails then the > next SECINFO flavor should be tried. > > Create an rpc_auth, rpc_cred, and initialize the cred (e.g. get a GSS Context) > using the short-lived SECINFO rpc client to test if the use of the RPC_AUTH_GSS > pseudoflavor succeeds. > > Signed-off-by: Andy Adamson <andros@netapp.com> > --- > fs/nfs/nfs4namespace.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 46 insertions(+), 2 deletions(-) > > diff --git a/fs/nfs/nfs4namespace.c b/fs/nfs/nfs4namespace.c > index fd4dcb6..e0a5491 100644 > --- a/fs/nfs/nfs4namespace.c > +++ b/fs/nfs/nfs4namespace.c > @@ -135,6 +135,39 @@ static size_t nfs_parse_server_name(char *string, size_t len, > } > > /** > + * nfs_test_gss - Test client support of pseudoflavor > + * @server: NFS server struct > + * @flavor: RPC_AUTH_GSS pseudoflavor > + */ > + > +static int nfs_test_gss_flavor(struct nfs_server *server, > + rpc_authflavor_t pseudoflavor) > +{ > + struct rpc_auth_create_args auth_args = { > + .pseudoflavor = pseudoflavor, > + }; > + struct rpc_auth *auth; > + struct rpc_cred *rcred; > + const struct cred *cred = current_cred(); > + struct auth_cred acred = { > + .uid = cred->fsuid, > + .gid = cred->fsgid, > + .group_info = get_group_info(((struct cred *)cred)->group_info), > + }; > + > + auth = rpcauth_create(&auth_args, server->client); This call has the side-effect of changing the value of server->client->cl_auth. Not sure that we want that here. > + if (IS_ERR(auth)) > + return -EACCES; > + > + /* This will call cr_init to create a gss context */ > + rcred = rpcauth_lookup_credcache(auth, &acred, 0); Why not call rpcauth_lookupcred() instead of open-coding? Also note that there is a credential refcount leak here (and a group_info refcount leak). > + if (IS_ERR(cred)) > + return -EACCES; > + > + return 0; > +} > + > +/** > * nfs_find_best_sec - Find a security mechanism supported locally > * @server: NFS server struct > * @flavors: List of security tuples returned by SECINFO procedure > @@ -152,21 +185,32 @@ static rpc_authflavor_t nfs_find_best_sec(struct nfs_server *server, > rpc_authflavor_t pseudoflavor; > struct nfs4_secinfo4 *secinfo; > unsigned int i; > + int err = 0; > > for (i = 0; i < flavors->num_flavors; i++) { > + bool gss = false; > + > secinfo = &flavors->flavors[i]; > > switch (secinfo->flavor) { > + case RPC_AUTH_GSS: > + gss = true; > case RPC_AUTH_NULL: > case RPC_AUTH_UNIX: > - case RPC_AUTH_GSS: > pseudoflavor = rpcauth_get_pseudoflavor(secinfo->flavor, > &secinfo->flavor_info); > /* make sure pseudoflavor matches sec= mount opt */ > if (pseudoflavor != RPC_AUTH_MAXFLAVOR && > nfs_auth_info_match(&server->auth_info, > - pseudoflavor)) > + pseudoflavor)) { > + if (gss) { > + err = nfs_test_gss_flavor(server, > + pseudoflavor); > + if (err) /* try the next flavor */ > + continue; > + } > return pseudoflavor; > + } > break; > } > }
On Jun 10, 2014, at 12:21 PM, Trond Myklebust <trond.myklebust@primarydata.com> wrote: > On Mon, 2014-06-09 at 15:33 -0400, andros@netapp.com wrote: >> From: Andy Adamson <andros@netapp.com> >> >> The current code returns an RPC_AUTH_GSS pseudo-flavor without testing to see >> if it is configured properly. If an RPC_AUTH_GSS pseudoflavor fails then the >> next SECINFO flavor should be tried. >> >> Create an rpc_auth, rpc_cred, and initialize the cred (e.g. get a GSS Context) >> using the short-lived SECINFO rpc client to test if the use of the RPC_AUTH_GSS >> pseudoflavor succeeds. >> >> Signed-off-by: Andy Adamson <andros@netapp.com> >> --- >> fs/nfs/nfs4namespace.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++-- >> 1 file changed, 46 insertions(+), 2 deletions(-) >> >> diff --git a/fs/nfs/nfs4namespace.c b/fs/nfs/nfs4namespace.c >> index fd4dcb6..e0a5491 100644 >> --- a/fs/nfs/nfs4namespace.c >> +++ b/fs/nfs/nfs4namespace.c >> @@ -135,6 +135,39 @@ static size_t nfs_parse_server_name(char *string, size_t len, >> } >> >> /** >> + * nfs_test_gss - Test client support of pseudoflavor >> + * @server: NFS server struct >> + * @flavor: RPC_AUTH_GSS pseudoflavor >> + */ >> + >> +static int nfs_test_gss_flavor(struct nfs_server *server, >> + rpc_authflavor_t pseudoflavor) >> +{ >> + struct rpc_auth_create_args auth_args = { >> + .pseudoflavor = pseudoflavor, >> + }; >> + struct rpc_auth *auth; >> + struct rpc_cred *rcred; >> + const struct cred *cred = current_cred(); >> + struct auth_cred acred = { >> + .uid = cred->fsuid, >> + .gid = cred->fsgid, >> + .group_info = get_group_info(((struct cred *)cred)->group_info), >> + }; >> + >> + auth = rpcauth_create(&auth_args, server->client); > > This call has the side-effect of changing the value of > server->client->cl_auth. Not sure that we want that here. I don't see any other interface to get a gss_auth struct to pass to rpcauth_lookupcredcache. If the gss_cred/gss_context creation works, then the cl_auth being set is OK as it would have been set anyway by the callers of nfs4_negotiate_security (nfs4_submount or nfs4_create_sec_client so far) if we simply passed the flavor to those functions to “test” if RPC_AUTH_GSS can be used. But on failure, you’re right, the cl_auth needs to be reaped. I’ll add a call to rpcauth_release() if nfs_test_gss_flavor() fails and set the cl_auth to NULL - and check that it is actually reaped. Since failure means no gss_context was created it is more simple than otherwise. > >> + if (IS_ERR(auth)) >> + return -EACCES; >> + >> + /* This will call cr_init to create a gss context */ >> + rcred = rpcauth_lookup_credcache(auth, &acred, 0); > > Why not call rpcauth_lookupcred() instead of open-coding? I see - it will call rpcauth_lookupcredcache for me (gssand do the put of the group_info as well. Good - I’ll use it. > > Also note that there is a credential refcount leak here I’ll make sure this is addressed Thanks for the review :) —>Andy > (and a > group_info refcount leak). > >> + if (IS_ERR(cred)) >> + return -EACCES; >> + >> + return 0; >> +} >> + >> +/** >> * nfs_find_best_sec - Find a security mechanism supported locally >> * @server: NFS server struct >> * @flavors: List of security tuples returned by SECINFO procedure >> @@ -152,21 +185,32 @@ static rpc_authflavor_t nfs_find_best_sec(struct nfs_server *server, >> rpc_authflavor_t pseudoflavor; >> struct nfs4_secinfo4 *secinfo; >> unsigned int i; >> + int err = 0; >> >> for (i = 0; i < flavors->num_flavors; i++) { >> + bool gss = false; >> + >> secinfo = &flavors->flavors[i]; >> >> switch (secinfo->flavor) { >> + case RPC_AUTH_GSS: >> + gss = true; >> case RPC_AUTH_NULL: >> case RPC_AUTH_UNIX: >> - case RPC_AUTH_GSS: >> pseudoflavor = rpcauth_get_pseudoflavor(secinfo->flavor, >> &secinfo->flavor_info); >> /* make sure pseudoflavor matches sec= mount opt */ >> if (pseudoflavor != RPC_AUTH_MAXFLAVOR && >> nfs_auth_info_match(&server->auth_info, >> - pseudoflavor)) >> + pseudoflavor)) { >> + if (gss) { >> + err = nfs_test_gss_flavor(server, >> + pseudoflavor); >> + if (err) /* try the next flavor */ >> + continue; >> + } >> return pseudoflavor; >> + } >> break; >> } >> } > > -- > Trond Myklebust > Linux NFS client maintainer, PrimaryData > trond.myklebust@primarydata.com -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jun 10, 2014 at 2:38 PM, Adamson, Andy <William.Adamson@netapp.com> wrote: > > On Jun 10, 2014, at 12:21 PM, Trond Myklebust <trond.myklebust@primarydata.com> wrote: > >> On Mon, 2014-06-09 at 15:33 -0400, andros@netapp.com wrote: >>> From: Andy Adamson <andros@netapp.com> >>> >>> The current code returns an RPC_AUTH_GSS pseudo-flavor without testing to see >>> if it is configured properly. If an RPC_AUTH_GSS pseudoflavor fails then the >>> next SECINFO flavor should be tried. >>> >>> Create an rpc_auth, rpc_cred, and initialize the cred (e.g. get a GSS Context) >>> using the short-lived SECINFO rpc client to test if the use of the RPC_AUTH_GSS >>> pseudoflavor succeeds. >>> >>> Signed-off-by: Andy Adamson <andros@netapp.com> >>> --- >>> fs/nfs/nfs4namespace.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++-- >>> 1 file changed, 46 insertions(+), 2 deletions(-) >>> >>> diff --git a/fs/nfs/nfs4namespace.c b/fs/nfs/nfs4namespace.c >>> index fd4dcb6..e0a5491 100644 >>> --- a/fs/nfs/nfs4namespace.c >>> +++ b/fs/nfs/nfs4namespace.c >>> @@ -135,6 +135,39 @@ static size_t nfs_parse_server_name(char *string, size_t len, >>> } >>> >>> /** >>> + * nfs_test_gss - Test client support of pseudoflavor >>> + * @server: NFS server struct >>> + * @flavor: RPC_AUTH_GSS pseudoflavor >>> + */ >>> + >>> +static int nfs_test_gss_flavor(struct nfs_server *server, >>> + rpc_authflavor_t pseudoflavor) >>> +{ >>> + struct rpc_auth_create_args auth_args = { >>> + .pseudoflavor = pseudoflavor, >>> + }; >>> + struct rpc_auth *auth; >>> + struct rpc_cred *rcred; >>> + const struct cred *cred = current_cred(); >>> + struct auth_cred acred = { >>> + .uid = cred->fsuid, >>> + .gid = cred->fsgid, >>> + .group_info = get_group_info(((struct cred *)cred)->group_info), >>> + }; >>> + >>> + auth = rpcauth_create(&auth_args, server->client); >> >> This call has the side-effect of changing the value of >> server->client->cl_auth. Not sure that we want that here. > > I don't see any other interface to get a gss_auth struct to pass to rpcauth_lookupcredcache. > > If the gss_cred/gss_context creation works, then the cl_auth being set is OK as it would have been set anyway by the callers of nfs4_negotiate_security (nfs4_submount or nfs4_create_sec_client so far) if we simply passed the flavor to those functions to “test” if RPC_AUTH_GSS can be used. Looking at the callers, I think I disagree. They are both trying to do lookups, so the WRONGSEC error that we're handling applies to the object being looked up, and not to the parent directory or the struct nfs_server that is being passed as an argument to nfs_find_best_sec().
On Jun 10, 2014, at 3:29 PM, Trond Myklebust <trond.myklebust@primarydata.com> wrote: > On Tue, Jun 10, 2014 at 2:38 PM, Adamson, Andy > <William.Adamson@netapp.com> wrote: >> >> On Jun 10, 2014, at 12:21 PM, Trond Myklebust <trond.myklebust@primarydata.com> wrote: >> >>> On Mon, 2014-06-09 at 15:33 -0400, andros@netapp.com wrote: >>>> From: Andy Adamson <andros@netapp.com> >>>> >>>> The current code returns an RPC_AUTH_GSS pseudo-flavor without testing to see >>>> if it is configured properly. If an RPC_AUTH_GSS pseudoflavor fails then the >>>> next SECINFO flavor should be tried. >>>> >>>> Create an rpc_auth, rpc_cred, and initialize the cred (e.g. get a GSS Context) >>>> using the short-lived SECINFO rpc client to test if the use of the RPC_AUTH_GSS >>>> pseudoflavor succeeds. >>>> >>>> Signed-off-by: Andy Adamson <andros@netapp.com> >>>> --- >>>> fs/nfs/nfs4namespace.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++-- >>>> 1 file changed, 46 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/fs/nfs/nfs4namespace.c b/fs/nfs/nfs4namespace.c >>>> index fd4dcb6..e0a5491 100644 >>>> --- a/fs/nfs/nfs4namespace.c >>>> +++ b/fs/nfs/nfs4namespace.c >>>> @@ -135,6 +135,39 @@ static size_t nfs_parse_server_name(char *string, size_t len, >>>> } >>>> >>>> /** >>>> + * nfs_test_gss - Test client support of pseudoflavor >>>> + * @server: NFS server struct >>>> + * @flavor: RPC_AUTH_GSS pseudoflavor >>>> + */ >>>> + >>>> +static int nfs_test_gss_flavor(struct nfs_server *server, >>>> + rpc_authflavor_t pseudoflavor) >>>> +{ >>>> + struct rpc_auth_create_args auth_args = { >>>> + .pseudoflavor = pseudoflavor, >>>> + }; >>>> + struct rpc_auth *auth; >>>> + struct rpc_cred *rcred; >>>> + const struct cred *cred = current_cred(); >>>> + struct auth_cred acred = { >>>> + .uid = cred->fsuid, >>>> + .gid = cred->fsgid, >>>> + .group_info = get_group_info(((struct cred *)cred)->group_info), >>>> + }; >>>> + >>>> + auth = rpcauth_create(&auth_args, server->client); >>> >>> This call has the side-effect of changing the value of >>> server->client->cl_auth. Not sure that we want that here. >> >> I don't see any other interface to get a gss_auth struct to pass to rpcauth_lookupcredcache. >> >> If the gss_cred/gss_context creation works, then the cl_auth being set is OK as it would have been set anyway by the callers of nfs4_negotiate_security (nfs4_submount or nfs4_create_sec_client so far) if we simply passed the flavor to those functions to “test” if RPC_AUTH_GSS can be used. > > Looking at the callers, I think I disagree. They are both trying to do > lookups, so the WRONGSEC error that we're handling applies to the > object being looked up, and not to the parent directory or the struct > nfs_server that is being passed as an argument to nfs_find_best_sec(). Ah. So I should clone a client to test the RPC_AUTH_GSS SECINFO case… —>Andy > > -- > Trond Myklebust > > Linux NFS client maintainer, PrimaryData > > trond.myklebust@primarydata.com -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/nfs/nfs4namespace.c b/fs/nfs/nfs4namespace.c index fd4dcb6..e0a5491 100644 --- a/fs/nfs/nfs4namespace.c +++ b/fs/nfs/nfs4namespace.c @@ -135,6 +135,39 @@ static size_t nfs_parse_server_name(char *string, size_t len, } /** + * nfs_test_gss - Test client support of pseudoflavor + * @server: NFS server struct + * @flavor: RPC_AUTH_GSS pseudoflavor + */ + +static int nfs_test_gss_flavor(struct nfs_server *server, + rpc_authflavor_t pseudoflavor) +{ + struct rpc_auth_create_args auth_args = { + .pseudoflavor = pseudoflavor, + }; + struct rpc_auth *auth; + struct rpc_cred *rcred; + const struct cred *cred = current_cred(); + struct auth_cred acred = { + .uid = cred->fsuid, + .gid = cred->fsgid, + .group_info = get_group_info(((struct cred *)cred)->group_info), + }; + + auth = rpcauth_create(&auth_args, server->client); + if (IS_ERR(auth)) + return -EACCES; + + /* This will call cr_init to create a gss context */ + rcred = rpcauth_lookup_credcache(auth, &acred, 0); + if (IS_ERR(cred)) + return -EACCES; + + return 0; +} + +/** * nfs_find_best_sec - Find a security mechanism supported locally * @server: NFS server struct * @flavors: List of security tuples returned by SECINFO procedure @@ -152,21 +185,32 @@ static rpc_authflavor_t nfs_find_best_sec(struct nfs_server *server, rpc_authflavor_t pseudoflavor; struct nfs4_secinfo4 *secinfo; unsigned int i; + int err = 0; for (i = 0; i < flavors->num_flavors; i++) { + bool gss = false; + secinfo = &flavors->flavors[i]; switch (secinfo->flavor) { + case RPC_AUTH_GSS: + gss = true; case RPC_AUTH_NULL: case RPC_AUTH_UNIX: - case RPC_AUTH_GSS: pseudoflavor = rpcauth_get_pseudoflavor(secinfo->flavor, &secinfo->flavor_info); /* make sure pseudoflavor matches sec= mount opt */ if (pseudoflavor != RPC_AUTH_MAXFLAVOR && nfs_auth_info_match(&server->auth_info, - pseudoflavor)) + pseudoflavor)) { + if (gss) { + err = nfs_test_gss_flavor(server, + pseudoflavor); + if (err) /* try the next flavor */ + continue; + } return pseudoflavor; + } break; } }