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
