* [PATCH 1/2] gssd: Fix bugs in process_krb5_upcall
@ 2012-11-26 22:31 Trond Myklebust
2012-11-26 22:31 ` [PATCH 2/2] gssd: Remove insane sanity checks of the service name Trond Myklebust
2012-11-27 16:05 ` [PATCH 1/2] gssd: Fix bugs in process_krb5_upcall Myklebust, Trond
0 siblings, 2 replies; 6+ messages in thread
From: Trond Myklebust @ 2012-11-26 22:31 UTC (permalink / raw)
To: Steve Dickson; +Cc: Bruce Fields, linux-nfs
The 'tgtname' parameter is the _server_ name, not the service name.
Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
---
utils/gssd/gssd_proc.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
index ec251fa..b79e872 100644
--- a/utils/gssd/gssd_proc.c
+++ b/utils/gssd/gssd_proc.c
@@ -963,10 +963,8 @@ process_krb5_upcall(struct clnt_info *clp, uid_t uid, int fd, char *tgtname,
printerr(1, "handling krb5 upcall (%s)\n", clp->dirname);
if (tgtname) {
- if (clp->servicename) {
- free(clp->servicename);
- clp->servicename = strdup(tgtname);
- }
+ free(clp->servername);
+ clp->servername = strdup(tgtname);
}
token.length = 0;
token.value = NULL;
--
1.7.11.7
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] gssd: Remove insane sanity checks of the service name
2012-11-26 22:31 [PATCH 1/2] gssd: Fix bugs in process_krb5_upcall Trond Myklebust
@ 2012-11-26 22:31 ` Trond Myklebust
2012-11-26 22:37 ` Bruce Fields
2012-11-28 19:53 ` Steve Dickson
2012-11-27 16:05 ` [PATCH 1/2] gssd: Fix bugs in process_krb5_upcall Myklebust, Trond
1 sibling, 2 replies; 6+ messages in thread
From: Trond Myklebust @ 2012-11-26 22:31 UTC (permalink / raw)
To: Steve Dickson; +Cc: Bruce Fields, linux-nfs
Either we trust the info file, or we don't. The current
'checks' only work for the combination 'nfs', '100003' and
a version number between 2 and 4.
The problem is that the callback channel also wants to use
'nfs' in combination with a different program number and
version number.
This patch throws the bogus checks out altogether and lets the
kernel use whatever combination it wants....
Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
---
utils/gssd/gssd_proc.c | 13 +------------
1 file changed, 1 insertion(+), 12 deletions(-)
diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
index b79e872..8c00201 100644
--- a/utils/gssd/gssd_proc.c
+++ b/utils/gssd/gssd_proc.c
@@ -250,21 +250,10 @@ read_service_info(char *info_file_name, char **servicename, char **servername,
if ((p = strstr(buf, "port")) != NULL)
sscanf(p, "port: %127s\n", port);
- /* check service, program, and version */
- if (memcmp(service, "nfs", 3) != 0)
- return -1;
+ /* get program, and version numbers */
*prog = atoi(program + 1); /* skip open paren */
*vers = atoi(version);
- if (strlen(service) == 3 ) {
- if ((*prog != 100003) || ((*vers != 2) && (*vers != 3) &&
- (*vers != 4)))
- goto fail;
- } else if (memcmp(service, "nfs4_cb", 7) == 0) {
- if (*vers != 1)
- goto fail;
- }
-
if (!addrstr_to_sockaddr(addr, address, port))
goto fail;
--
1.7.11.7
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] gssd: Remove insane sanity checks of the service name
2012-11-26 22:31 ` [PATCH 2/2] gssd: Remove insane sanity checks of the service name Trond Myklebust
@ 2012-11-26 22:37 ` Bruce Fields
2012-11-28 19:53 ` Steve Dickson
1 sibling, 0 replies; 6+ messages in thread
From: Bruce Fields @ 2012-11-26 22:37 UTC (permalink / raw)
To: Trond Myklebust; +Cc: Steve Dickson, linux-nfs
On Mon, Nov 26, 2012 at 05:31:21PM -0500, Trond Myklebust wrote:
> Either we trust the info file, or we don't. The current
> 'checks' only work for the combination 'nfs', '100003' and
> a version number between 2 and 4.
> The problem is that the callback channel also wants to use
> 'nfs' in combination with a different program number and
> version number.
>
> This patch throws the bogus checks out altogether and lets the
> kernel use whatever combination it wants....
>
> Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
ACK to both.--b.
> ---
> utils/gssd/gssd_proc.c | 13 +------------
> 1 file changed, 1 insertion(+), 12 deletions(-)
>
> diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
> index b79e872..8c00201 100644
> --- a/utils/gssd/gssd_proc.c
> +++ b/utils/gssd/gssd_proc.c
> @@ -250,21 +250,10 @@ read_service_info(char *info_file_name, char **servicename, char **servername,
> if ((p = strstr(buf, "port")) != NULL)
> sscanf(p, "port: %127s\n", port);
>
> - /* check service, program, and version */
> - if (memcmp(service, "nfs", 3) != 0)
> - return -1;
> + /* get program, and version numbers */
> *prog = atoi(program + 1); /* skip open paren */
> *vers = atoi(version);
>
> - if (strlen(service) == 3 ) {
> - if ((*prog != 100003) || ((*vers != 2) && (*vers != 3) &&
> - (*vers != 4)))
> - goto fail;
> - } else if (memcmp(service, "nfs4_cb", 7) == 0) {
> - if (*vers != 1)
> - goto fail;
> - }
> -
> if (!addrstr_to_sockaddr(addr, address, port))
> goto fail;
>
> --
> 1.7.11.7
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH 1/2] gssd: Fix bugs in process_krb5_upcall
2012-11-26 22:31 [PATCH 1/2] gssd: Fix bugs in process_krb5_upcall Trond Myklebust
2012-11-26 22:31 ` [PATCH 2/2] gssd: Remove insane sanity checks of the service name Trond Myklebust
@ 2012-11-27 16:05 ` Myklebust, Trond
2012-11-27 16:43 ` Steve Dickson
1 sibling, 1 reply; 6+ messages in thread
From: Myklebust, Trond @ 2012-11-27 16:05 UTC (permalink / raw)
To: Myklebust, Trond, Steve Dickson; +Cc: Bruce Fields, linux-nfs@vger.kernel.org
> -----Original Message-----
> From: Trond Myklebust [mailto:Trond.Myklebust@netapp.com]
> Sent: Monday, November 26, 2012 5:31 PM
> To: Steve Dickson
> Cc: Bruce Fields; linux-nfs@vger.kernel.org
> Subject: [PATCH 1/2] gssd: Fix bugs in process_krb5_upcall
>
> The 'tgtname' parameter is the _server_ name, not the service name.
>
> Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
> ---
> utils/gssd/gssd_proc.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c index
> ec251fa..b79e872 100644
> --- a/utils/gssd/gssd_proc.c
> +++ b/utils/gssd/gssd_proc.c
> @@ -963,10 +963,8 @@ process_krb5_upcall(struct clnt_info *clp, uid_t uid,
> int fd, char *tgtname,
> printerr(1, "handling krb5 upcall (%s)\n", clp->dirname);
>
> if (tgtname) {
> - if (clp->servicename) {
> - free(clp->servicename);
> - clp->servicename = strdup(tgtname);
> - }
> + free(clp->servername);
> + clp->servername = strdup(tgtname);
> }
> token.length = 0;
> token.value = NULL;
> --
Sigh... Actually, this isn't right either. The log comment for commit 8b1c7bf5b624c9bc91b41ae577b9fc5c21641705 (rpc: add target field to new upcall) on the Linux client does indeed talk about who we want to authenticate to, but the choice of 'clnt->cl_principal' will actually give us our client hostname.
It turns out that nfs@"client hostname "is indeed the correct machine cred name when we're acting as the client, but when doing _callbacks_, the server has to authenticate using the same principal used by the client in the SETCLIENTID call (See Section 3.4, RFC3530). i.e. the nfs@hostname used does in fact include the NFS client's hostname (not the server's)!
So while PATCH 2/2 is still good, this patch appears to be incorrect and should be dropped for now.
Cheers
Trond
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] gssd: Fix bugs in process_krb5_upcall
2012-11-27 16:05 ` [PATCH 1/2] gssd: Fix bugs in process_krb5_upcall Myklebust, Trond
@ 2012-11-27 16:43 ` Steve Dickson
0 siblings, 0 replies; 6+ messages in thread
From: Steve Dickson @ 2012-11-27 16:43 UTC (permalink / raw)
To: Myklebust, Trond; +Cc: Bruce Fields, linux-nfs@vger.kernel.org
On 27/11/12 11:05, Myklebust, Trond wrote:
>> -----Original Message-----
>> From: Trond Myklebust [mailto:Trond.Myklebust@netapp.com]
>> Sent: Monday, November 26, 2012 5:31 PM
>> To: Steve Dickson
>> Cc: Bruce Fields; linux-nfs@vger.kernel.org
>> Subject: [PATCH 1/2] gssd: Fix bugs in process_krb5_upcall
>>
>> The 'tgtname' parameter is the _server_ name, not the service name.
>>
>> Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
>> ---
>> utils/gssd/gssd_proc.c | 6 ++----
>> 1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c index
>> ec251fa..b79e872 100644
>> --- a/utils/gssd/gssd_proc.c
>> +++ b/utils/gssd/gssd_proc.c
>> @@ -963,10 +963,8 @@ process_krb5_upcall(struct clnt_info *clp, uid_t uid,
>> int fd, char *tgtname,
>> printerr(1, "handling krb5 upcall (%s)\n", clp->dirname);
>>
>> if (tgtname) {
>> - if (clp->servicename) {
>> - free(clp->servicename);
>> - clp->servicename = strdup(tgtname);
>> - }
>> + free(clp->servername);
>> + clp->servername = strdup(tgtname);
>> }
>> token.length = 0;
>> token.value = NULL;
>> --
>
> Sigh... Actually, this isn't right either. The log comment for commit 8b1c7bf5b624c9bc91b41ae577b9fc5c21641705 (rpc: add target field to new upcall) on the Linux client does indeed talk about who we want to authenticate to, but the choice of 'clnt->cl_principal' will actually give us our client hostname.
>
> It turns out that nfs@"client hostname "is indeed the correct machine cred name when we're acting as the client, but when doing _callbacks_, the server has to authenticate using the same principal used by the client in the SETCLIENTID call (See Section 3.4, RFC3530). i.e. the nfs@hostname used does in fact include the NFS client's hostname (not the server's)!
>
> So while PATCH 2/2 is still good, this patch appears to be incorrect and should be dropped for now.
>
Duly noted...
steved.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] gssd: Remove insane sanity checks of the service name
2012-11-26 22:31 ` [PATCH 2/2] gssd: Remove insane sanity checks of the service name Trond Myklebust
2012-11-26 22:37 ` Bruce Fields
@ 2012-11-28 19:53 ` Steve Dickson
1 sibling, 0 replies; 6+ messages in thread
From: Steve Dickson @ 2012-11-28 19:53 UTC (permalink / raw)
To: Trond Myklebust; +Cc: Bruce Fields, linux-nfs
On 26/11/12 17:31, Trond Myklebust wrote:
> Either we trust the info file, or we don't. The current
> 'checks' only work for the combination 'nfs', '100003' and
> a version number between 2 and 4.
> The problem is that the callback channel also wants to use
> 'nfs' in combination with a different program number and
> version number.
>
> This patch throws the bogus checks out altogether and lets the
> kernel use whatever combination it wants....
>
> Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
Committed...
steved.
> ---
> utils/gssd/gssd_proc.c | 13 +------------
> 1 file changed, 1 insertion(+), 12 deletions(-)
>
> diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
> index b79e872..8c00201 100644
> --- a/utils/gssd/gssd_proc.c
> +++ b/utils/gssd/gssd_proc.c
> @@ -250,21 +250,10 @@ read_service_info(char *info_file_name, char **servicename, char **servername,
> if ((p = strstr(buf, "port")) != NULL)
> sscanf(p, "port: %127s\n", port);
>
> - /* check service, program, and version */
> - if (memcmp(service, "nfs", 3) != 0)
> - return -1;
> + /* get program, and version numbers */
> *prog = atoi(program + 1); /* skip open paren */
> *vers = atoi(version);
>
> - if (strlen(service) == 3 ) {
> - if ((*prog != 100003) || ((*vers != 2) && (*vers != 3) &&
> - (*vers != 4)))
> - goto fail;
> - } else if (memcmp(service, "nfs4_cb", 7) == 0) {
> - if (*vers != 1)
> - goto fail;
> - }
> -
> if (!addrstr_to_sockaddr(addr, address, port))
> goto fail;
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-11-28 19:53 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-11-26 22:31 [PATCH 1/2] gssd: Fix bugs in process_krb5_upcall Trond Myklebust
2012-11-26 22:31 ` [PATCH 2/2] gssd: Remove insane sanity checks of the service name Trond Myklebust
2012-11-26 22:37 ` Bruce Fields
2012-11-28 19:53 ` Steve Dickson
2012-11-27 16:05 ` [PATCH 1/2] gssd: Fix bugs in process_krb5_upcall Myklebust, Trond
2012-11-27 16:43 ` Steve Dickson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).