That's a tricky one. Apparently it's possible to build curl with UNICODE, and that may enable some features otherwise not available. This is why some functions use TCHARs.

And yes, should be _tcslen instead of strlen. Will fix it shortly.

Also change if ( to if(, run checksrc please.

As to whether this is a good idea to add I don't know since I don't use LDAP protocol, so I think any +1 should come from someone else. How is the behavior different from Linux with LDAP after this change?

This comment has been minimized.

edited

Author
Member

How is the behavior different from Linux with LDAP after this change?

Linux behavior doesn't affected with this change, but Windows curl now able to request Microsoft AD. Basic AUTH almost always disabled on Windows, so now without user/password curl will perform auto-negotiation.curl --ntlm -u user:pass ldap://.... will connect with other user credentials using NTLM auth.

This comment has been minimized.

edited

Many thanks for you efforts here @snikulov - it looks good. My only comments would be:

Could you use Curl_create_sspi_identity() from curl_sspi.c rather than populate the credentials structure yourself.

May not be required if you use the above but I would recommend initialising the credentials handle with { 0, } rather than { 0 } as I believe, and anyone please correct me if I am wrong, that the latter is C99 onwards.

I would recommend using lower case for the function name and prefixing it with ldap to indicate it is a local static function - for example ldap_win_bind() or similar. I am aware that Curl_ldap() doesn't quite following the more modern naming convention we have and I should really fix that up ;-)

The appveyor and travis-ci tests seem to be failing - which of course may not be related to this PR.

This comment has been minimized.

Steve tests are failing because of #881 not this afaik. The 0, thing sounds familiar, I don't know if it's in the standard but I recall working on a different project C++ whatever compiler we were using warned about {0} and made us do {0,}. Sergei one more thing we align columns when the parentheses continues over multiple lines like

This comment has been minimized.

edited

There is no need for the comparison of CURLE_OK against Curl_create_sspi_identity() - if(Curl_create_sspi_identity()) would suffice.

I would probable combine that with the if user && passwords to be if(user && password && Curl_create_sspi_identity()) to lessen the number of indents ;-)

Curl's --help needs updating to indicate that --basic, --digest and --ntlm now support LDAP rather than just HTTP.

Will you be updating the man pages for curl (--basic, --digest and --ntlm ) as well as the libcurl documentation for CURLOPT_HTTPAUTH?

I do have some reservations about using --ntlm, --digest and --negotiate aka CURLOPT_HTTPAUTH for the LDAP authentication which I'd like to discuss with the team. For example the default authentication selection may not be appropriate to LDAP. Also, should we try and use this option for other SASL based (not just LDAP) authentication protocols - such as IMAP, POP3 and SMTP? Currently we have the --options support there but we could use HTTP auth for this as well - and make it more generic?? I have so far resisted this as I do aim to bring AUTH URL options to HTTP at some point but I am open to the idea.

This comment has been minimized.

@captain-caveman2k I've updated code, but not documentation. I suggesting create another PR against documentation. Only one thing is not clear to me. This code is Windows specific. I'm not sure whether or not OpenLDAP uses such options.

This comment has been minimized.

I'll echo Steve's point about the doc, I think if this makes behavior changes (which it appears to) then it should come with documentation. As far as the way your code is written in the most recent commit I have no objection. The issue of how to reconcile this with Linux LDAP should be addressed I suspect. As mentioned I'm abstaining from a vote due to my lack of experience with this protocol, so I'm going to otherwise stay out of this.

This comment has been minimized.

edited

Selecting CURLAUTH_NTLM_WB and/or CURLAUTH_DIGEST_IE as the only auth options or using CURLAUTH_NONE will cause Negotiate to be chosen

If CURL_DISABLE_CRYPTO_AUTH is defined then Digest should not be used, if USE_NTLM is not used then NTLM should not be usable and if USE_SPNEGO is not defined then Negotiate should not be used

In addition to this it would be really nice to:

Use the currently logged in user credentials when Digest and NTLM are specified and not only for Negotiate

Use the best authentication mechanism when CURLAUTH_BASIC | CURLAUTH_DIGEST | CURLAUTH_NTLM | CURLAUTH_NEGOTIATE like we do with HTTP and the email protocols - with the PR as it is Basic will be chosen

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.