On Mon, 20 Jun 2011, Wu, Mandy wrote:

> Hi, I am working on adding NTLM single-sign-on support by using Samba's
> 'winbind' helper ntlm_auth.

Hi, and thanks for your contribution and work on libcurl.

I'll admit I don't quite understand what "NTLM single-sign-on" is and how it
differs from ordinary NTLM etc, but I'll still provide some feedback on the
patch you've shown.

Is there any way we can make test cases for this?

> +#ifdef USE_NTLM_SSO
> + if(1 &&
> +#else
> if(conn->bits.user_passwd &&
> +#endif
> ((data->req.httpcode == 401) ||
> (conn->bits.authneg && data->req.httpcode < 300))) {

This seems like a funny change. If we really can unconditionally do that check
when USE_NTLM_SSO is set then we ought to be able to always do it and then the
check seems pointless. For the other "ticket-based" auth types libcurl already
forces the users to set a "fake" user in order to trigger authentication to
happen and I guess NTLM SSO can too. (The same applies to the same change you
did to the proxy auth code.)

Do all the auth types/test cases work with this change applied?

> else {
> +#ifdef USE_NTLM_SSO
> + /* NTLM single-sign-on, continue please */ ;
> +#else
> authhost->done = TRUE;
> authproxy->done = TRUE;
> return CURLE_OK; /* no authentication with no user or password */
> +#endif

Does this really make sense? Just because libcurl was built to support NTLM
SSO you can skip that code unconditionally?

> + username = getenv("NTLMUSER");
> + if(!username)
> + username = getenv("USER");
> + if(!username)
> + goto done;

I don't think getting info from environment variables like this is a good
library API. What about using conn->user ?

> +{
> + ssize_t size;
> + char buf[1024], *response = NULL;
> +
> + if(write (sso_data->fd_helper, input, strlen (input)) < 0 ||
> + (size = read (sso_data->fd_helper, buf, 1024)) < 5 ) {
> + goto done;
> + }

1024 seems aribtrary picked here. Is there a particualr justfication for this
number? How about using sizeof(buf) instead of 1024 when referring to the size
of the buffer?

> + response = strndup(buf + 3, size - 4);

I'm convinced we'll fall over systems that don't have strndup. I think we
might use the memdup for this instead (currently private in formdata.c).

> + /* Fix me later */
> + conn->sso_data = calloc(1, sizeof(struct ntlmhelper));

It says fix me, but we need to check for malloc failures and bail out nicely
on such.

> + if(!strcasecmp(header, "PW")) {

Curl_raw_equal() is what we use for portable case insensitive comparisons that
should ignore locales.

-- 
  / daniel.haxx.se
-------------------------------------------------------------------
List admin: http://cool.haxx.se/list/listinfo/curl-library
Etiquette:  http://curl.haxx.se/mail/etiquette.html