Skip to content

Commit

Permalink
vauth: Removed the need for a separate GSS-API based SPN function
Browse files Browse the repository at this point in the history
  • Loading branch information
@captain-caveman2k
captain-caveman2k committed Apr 3, 2016
1 parent e655ae0 commit 9feb267
Show file tree
Hide file tree
Showing 8 changed files with 37 additions and 39 deletions.
2 changes: 1 addition & 1 deletion lib/vauth/digest.c
View file
Expand Up @@ -415,7 +415,7 @@ CURLcode Curl_auth_create_digest_md5_message(struct SessionHandle *data,
snprintf(&HA1_hex[2 * i], 3, "%02x", digest[i]);

/* Generate our SPN */
spn = Curl_auth_build_spn(service, realm);
spn = Curl_auth_build_spn(service, realm, NULL);
if(!spn)
return CURLE_OUT_OF_MEMORY;

Expand Down
2 changes: 1 addition & 1 deletion lib/vauth/digest_sspi.c
View file
Expand Up @@ -125,7 +125,7 @@ CURLcode Curl_auth_create_digest_md5_message(struct SessionHandle *data,
}

/* Generate our SPN */
spn = Curl_auth_build_spn(service, data->easy_conn->host.name);
spn = Curl_auth_build_spn(service, data->easy_conn->host.name, NULL);
if(!spn) {
free(output_token);
free(input_token);
Expand Down
2 changes: 1 addition & 1 deletion lib/vauth/krb5_gssapi.c
View file
Expand Up @@ -90,7 +90,7 @@ CURLcode Curl_auth_create_gssapi_user_message(struct SessionHandle *data,

if(!krb5->spn) {
/* Generate our SPN */
char *spn = Curl_auth_build_gssapi_spn(service, host);
char *spn = Curl_auth_build_spn(service, NULL, host);
if(!spn)
return CURLE_OUT_OF_MEMORY;

Expand Down
2 changes: 1 addition & 1 deletion lib/vauth/krb5_sspi.c
View file
Expand Up @@ -87,7 +87,7 @@ CURLcode Curl_auth_create_gssapi_user_message(struct SessionHandle *data,

if(!krb5->spn) {
/* Generate our SPN */
krb5->spn = Curl_auth_build_spn(service, host);
krb5->spn = Curl_auth_build_spn(service, host, NULL);
if(!krb5->spn)
return CURLE_OUT_OF_MEMORY;
}
Expand Down
2 changes: 1 addition & 1 deletion lib/vauth/spnego_gssapi.c
View file
Expand Up @@ -89,7 +89,7 @@ CURLcode Curl_auth_decode_spnego_message(struct SessionHandle *data,

if(!nego->spn) {
/* Generate our SPN */
char *spn = Curl_auth_build_gssapi_spn(service, host);
char *spn = Curl_auth_build_spn(service, NULL, host);
if(!spn)
return CURLE_OUT_OF_MEMORY;

Expand Down
2 changes: 1 addition & 1 deletion lib/vauth/spnego_sspi.c
View file
Expand Up @@ -90,7 +90,7 @@ CURLcode Curl_auth_decode_spnego_message(struct SessionHandle *data,

if(!nego->spn) {
/* Generate our SPN */
nego->spn = Curl_auth_build_spn(service, host);
nego->spn = Curl_auth_build_spn(service, host, NULL);
if(!nego->spn)
return CURLE_OUT_OF_MEMORY;
}
Expand Down
54 changes: 27 additions & 27 deletions lib/vauth/vauth.c
View file
Expand Up @@ -35,36 +35,55 @@
/*
* Curl_auth_build_spn()
*
* This is used to build a SPN string in the format service/instance.
* This is used to build a SPN string in the following formats:
*
* service/host@realm (Not currently used)
* service/host (Not used by GSS-API)
* service@realm (Not used by Windows SSPI)
*
* Parameters:
*
* service [in] - The service type such as www, smtp, pop or imap.
* instance [in] - The host name or realm.
* host [in] - The host name.
* realm [in] - The realm.
*
* Returns a pointer to the newly allocated SPN.
*/
#if !defined(USE_WINDOWS_SSPI)
char *Curl_auth_build_spn(const char *service, const char *instance)
char *Curl_auth_build_spn(const char *service, const char *host,

This comment has been minimized.

Sign in to view

Copy link
@michael-o

michael-o Apr 5, 2016

Contributor

It rather confusing passing the host to realm just to have correct GSS-API behavior. If you consider this library general purpose, GSS-API call to service and host will be broken.

This comment has been minimized.

Sign in to view

Copy link
@captain-caveman2k

captain-caveman2k Apr 5, 2016

Author Contributor

I'm not entirely sure I understand your point.

You have previously mentioned that:

a) you weren't happy with my naming of argument variables
b) that the gss-api version of the function wasn't needed whilst it was

I have tried to address those concerns with what resources and time I have. If you are still unhappy with the implementation please feel free to make constructive suggestions or take the time to provide a patch rather than simply criticise from the side lines.

This comment has been minimized.

Sign in to view

Copy link
@michael-o

michael-o Apr 5, 2016

Contributor

You are right, of course. No offense intended. I will have a look at it tomorrow. Maybe I won't come up with a better general purpose idea.

This comment has been minimized.

Sign in to view

Copy link
@michael-o

michael-o Apr 5, 2016

Contributor

I am just looking what the RFC has to say about this: https://tools.ietf.org/html/rfc4120#section-6.2

This comment has been minimized.

Sign in to view

Copy link
@captain-caveman2k

captain-caveman2k Apr 5, 2016

Author Contributor

Its hard not to Michael...

I put a lot of time, effort, passion and even money (I run autobuilds on my own servers, for example, which need to be paid for and now attend fosdem, which I enjoy immensely and try and catch up with other curl contributors when they are in the UK) into my involvement with curl and unfortunately criticism like this really doesn't encourage me to continue.

This comment has been minimized.

Sign in to view

Copy link
@michael-o

michael-o Apr 5, 2016

Contributor

Don't take it personal. I wrote intentionally: "No offense intended." and I mean it like that. I am not diminishing your work or contributions. Just trying to shed some light on it from another direction. I wouldn't review your code if I wouldn't want to help you. Did anyone else offer you to review the code? I guess no one.

This comment has been minimized.

Sign in to view

Copy link
@captain-caveman2k

captain-caveman2k Apr 5, 2016

Author Contributor

Thank you but your "No offense intended" comment was after the initial criticism and quite a number of comments at that - most of which has taken me a couple of hours to go through and respond to. Not only that but it seems more critical using a public forum like this rather than discussing it privately as the whole world is able to see :(

Your assistance is appreciated and as you know I value your opinion as you are one of a handful (roughly) of people involved with curl that take an interest in and have expertise and knowledge of the authentication mechanisms and APIs.

This comment has been minimized.

Sign in to view

Copy link
@michael-o

michael-o Apr 5, 2016

Contributor

Thanks, I hope we are good now. Btw, it took me two days at work to test the recent PR from Isaac and your code. So, you are not the only one investing time into this :)

This comment has been minimized.

Sign in to view

Copy link
@captain-caveman2k

captain-caveman2k Apr 5, 2016

Author Contributor

I know I am not alone - there are thousands of people that have made curl what it is today and mostly done it on a voluntary basis and like I said above your own contribution is much appreciated by me.

I don't expect thanks or gratitude, I do this as much for my own enjoyment as much as anything, and it is great to create/write/be part of something that a lot of people use either directly or indirectly, as Daniel has often written about on his blog.

In some respects, I am also fortunate I am able to dedicate the amount of time I do - at 44 I don't have family commitments and children for example that friends have ;-)

This comment has been minimized.

Sign in to view

Copy link
@michael-o

michael-o Apr 6, 2016

Contributor

I just checked the function in question again and all of it is usages. Frankly, I didn't come up with a better solution unless one splits it in two. It might be worth adding: "Depending on the usecase it might be necessary to swap host and realm depending on the desired format." Thank you anyway, btw I do have commitments ;-)
const char *realm)
{
/* Generate and return our SPN */
return aprintf("%s/%s", service, instance);
char *spn = NULL;

/* Generate our SPN */
if(host && realm)
spn = aprintf("%s/%s@%s", service, host, realm);
else if(host)
spn = aprintf("%s/%s", service, host);
else if(realm)
spn = aprintf("%s@%s", service, realm);

/* Return our newly allocated SPN */
return spn;
}
#else
TCHAR *Curl_auth_build_spn(const char *service, const char *instance)
TCHAR *Curl_auth_build_spn(const char *service, const char *host,
const char *realm)
{
char *utf8_spn = NULL;
TCHAR *tchar_spn = NULL;

(void) realm;

/* Note: We could use DsMakeSPN() or DsClientMakeSpnForTargetServer() rather
than doing this ourselves but the first is only available in Windows XP
and Windows Server 2003 and the latter is only available in Windows 2000
but not Windows95/98/ME or Windows NT4.0 unless the Active Directory
Client Extensions are installed. As such it is far simpler for us to
formulate the SPN instead. */

/* Allocate our UTF8 based SPN */
utf8_spn = aprintf("%s/%s", service, instance);
/* Generate our UTF8 based SPN */
utf8_spn = aprintf("%s/%s", service, host);
if(!utf8_spn) {
return NULL;
}
Expand All @@ -85,22 +104,3 @@ TCHAR *Curl_auth_build_spn(const char *service, const char *instance)
}
#endif /* USE_WINDOWS_SSPI */

#if defined(HAVE_GSSAPI)
/*
* Curl_auth_build_gssapi_spn()
*
* This is used to build a SPN string in the format service@instance.
*
* Parameters:
*
* service [in] - The service type such as www, smtp, pop or imap.
* instance [in] - The host name or realm.
*
* Returns a pointer to the newly allocated SPN.
*/
char *Curl_auth_build_gssapi_spn(const char *service, const char *instance)
{
/* Generate and return our SPN */
return aprintf("%s@%s", service, instance);
}
#endif /* HAVE_GSSAPI */
10 changes: 4 additions & 6 deletions lib/vauth/vauth.h
View file
Expand Up @@ -48,13 +48,11 @@ struct negotiatedata;

/* This is used to build a SPN string */
#if !defined(USE_WINDOWS_SSPI)
char *Curl_auth_build_spn(const char *service, const char *instance);
char *Curl_auth_build_spn(const char *service, const char *host,
const char *realm);
#else
TCHAR *Curl_auth_build_spn(const char *service, const char *instance);
#endif

#if defined(HAVE_GSSAPI)
char *Curl_auth_build_gssapi_spn(const char *service, const char *instance);
TCHAR *Curl_auth_build_spn(const char *service, const char *host,
const char *realm);
#endif

/* This is used to generate a base64 encoded PLAIN cleartext message */
Expand Down

0 comments on commit 9feb267

Please sign in to comment.