New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LDAP: using ldap_bind_s on Windows with methods #878

Merged
merged 3 commits into from May 23, 2017

Conversation

Projects
None yet
6 participants
@snikulov
Member

snikulov commented Jun 15, 2016

Added handling options --basic, --digest, --ntlm and --negotiate for LDAP protocol with provided user -u on Windows to access Microsoft Active Directory.

By default, if no -u option provided on Windows curl will use auto-negotiation with default domain credentials (same as --negotiate).

@mention-bot

This comment has been minimized.

mention-bot commented Jun 15, 2016

By analyzing the blame information on this pull request, we identified @captain-caveman2k, @bagder and @gknauf to be potential reviewers

@snikulov

This comment has been minimized.

Member

snikulov commented Jun 15, 2016

Depends on #878

lib/ldap.c Outdated
cred.UserLength = strlen(user);
cred.Password = passwd;
cred.PasswordLength = strlen(passwd);
cred.Flags = SEC_WINNT_AUTH_IDENTITY_ANSI;

This comment has been minimized.

@jay

jay Jun 15, 2016

Member

Is doesn't look correct that you are specifying ansi and using strlen when you have a function that takes tchar

This comment has been minimized.

@snikulov

snikulov Jun 15, 2016

Member

yes. perhaps it should be
cred.Flags = sizeof(TCHAR) == sizeof(char) ? SEC_WINNT_AUTH_IDENTITY_ANSI : SEC_WINNT_AUTH_IDENTITY_UNICODE;

What do you think? AFAIR curl does not currently support Windows Unicode. But maybe I'm wrong.

This comment has been minimized.

@snikulov

snikulov Jun 15, 2016

Member

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

This comment has been minimized.

@jay

jay Jun 15, 2016

Member

the doc for ldap_bind_s is weird it says it uses PCHAR but I can see in Winldap.h:

WINLDAPAPI ULONG LDAPAPI ldap_bind_sW( LDAP *ld, __in_opt PWCHAR dn, __in_opt PWCHAR cred, ULONG method );
WINLDAPAPI ULONG LDAPAPI ldap_bind_sA( LDAP *ld, __in_opt PCHAR dn, __in_opt PCHAR cred, ULONG method );

and later

#if LDAP_UNICODE
[...]
#define ldap_bind_s ldap_bind_sW
[...]
#else
[...]
WINLDAPAPI ULONG LDAPAPI ldap_bind_s( LDAP *ld, __in_opt const PCHAR dn, __in_opt const PCHAR cred, ULONG method );
[...]
#endif

I don't know if LDAP_UNICODE is tied to UNICODE but I'd guess so.

curl does not currently support Windows Unicode

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.

@snikulov

snikulov Jun 15, 2016

Member

@jay is checksrc work on Windows?

This comment has been minimized.

@jay

jay Jun 15, 2016

Member

I use Steve's wrapper, it works if perl is in your path. Open a command window at the projects directory and run checksrc. It's a checksrc.bat that wraps the checksrc.pl.

This comment has been minimized.

@snikulov

snikulov Jun 15, 2016

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.

@gvanem

gvanem Jun 16, 2016

Member

Jay> I don't know if LDAP_UNICODE is tied to UNICODE but I'd guess so.

Almost. From <WinLdap.h> in the Win-10 SDK:

//
//  The #define LDAP_UNICODE controls if we map the undecorated calls to
//  their unicode counterparts or just leave them defined as the normal
//  single byte entry points.
//
//  If you want to write a UNICODE enabled application, you'd normally
//  just have UNICODE defined and then we'll default to using all LDAP
//  Unicode calls.
//

#ifndef LDAP_UNICODE
#ifdef UNICODE
#define LDAP_UNICODE 1
#else
#define LDAP_UNICODE 0
#endif
#endif

Hence libcurl (or curl) could do:

#define LDAP_UNICODE 0

regardless of UNICODE =[0|1]. But I fail to see the point of that.

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

AFAICR, a libcurl built with UNICODE=1 links fine. But a curl with the same has problems.

@snikulov

This comment has been minimized.

Member

snikulov commented Jun 15, 2016

@jay Fixed per your comments. Also fixed all checksrc.bat warnings.

lib/ldap.c Outdated
cred.PasswordLength = _tcslen(passwd);
cred.Flags = (sizeof(TCHAR) == sizeof(char)
? SEC_WINNT_AUTH_IDENTITY_ANSI
: SEC_WINNT_AUTH_IDENTITY_UNICODE);

This comment has been minimized.

@jay

jay Jun 16, 2016

Member

checksrc and code style may not cover this but the operator goes on the end if it's over multiple lines, like

a &&
b

longfoo ? longbar :
longbaz;

longfoo ? longbar :
          longbaz; (rare)

longfoo ?
longbar :
longbaz;

in the repo there's also another way with a 2 space indent from the starting
column but i think that's some old style
var = x ?
  y : z;

so you could change it to

    cred.Flags = (sizeof(TCHAR) == sizeof(char)) ?
                 SEC_WINNT_AUTH_IDENTITY_ANSI :
                 SEC_WINNT_AUTH_IDENTITY_UNICODE;

the user != NULL it's preferred just if(user && passwd), though != NULL appears to be accepted. I did a git grep just now and there are numerous instances.

also args can be squashed to two lines

static int Win_ldap_bind(struct connectdata *conn, LDAP *server,
                         TCHAR *user, TCHAR *passwd)

as i mentioned someone else will have to +1. i think it would be worthwhile to explain how this behavior is different from ldap in linux, if it is not obvious ( i don't use ldap)

This comment has been minimized.

@snikulov

snikulov Jun 16, 2016

Member

@jay It should be good if someone will just provide .clang-format rules for this :)
Will update it shortly.

This comment has been minimized.

@bagder

bagder Jun 16, 2016

Member

I tried once, but frankly, clang-format is just as annoying as GNU indent so I gave up... :-/

This comment has been minimized.

@snikulov

snikulov Jun 16, 2016

Member

@bagder it doing good and highly flexible from my point of view.

This comment has been minimized.

@bagder

bagder Jun 16, 2016

Member

if you can produce a clang-format setup that makes it output code in the curl style then I'll be very interested!

@snikulov

This comment has been minimized.

Member

snikulov commented Jun 16, 2016

@jay Fixed and squashed with previous commit.

@captain-caveman2k

This comment has been minimized.

Member

captain-caveman2k commented Jun 16, 2016

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.
@jay

This comment has been minimized.

Member

jay commented Jun 16, 2016

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

no:
(a &&
b)

yes:
(a &&
 b)
@captain-caveman2k

This comment has been minimized.

Member

captain-caveman2k commented Jun 16, 2016

Cheers @jay - I wasn't sure if the test was due to this PR or not but thought it worth mentioning.

A quick search about 0, revealed that designated initialisers were introduced in C99 - section 6.7.8.

Prior to that I used, and still do, "{ 0, }" - I just wouldn't recommend doing "char something[] = {0, };" as I accidentally once did in '90s ;-)

@snikulov

This comment has been minimized.

Member

snikulov commented Jun 17, 2016

@captain-caveman2k Steve, I've updated per your comments.
Could you please review?

If all OK, I'll re-base all changes into single commit.

@snikulov

This comment has been minimized.

Member

snikulov commented Jun 20, 2016

@jay @captain-caveman2k Any other comments, suggestions?

@captain-caveman2k

This comment has been minimized.

Member

captain-caveman2k commented Jun 20, 2016

Cheers @snikulov

My only comments would be:

  • 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.

@snikulov

This comment has been minimized.

Member

snikulov commented Jun 21, 2016

@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.

@jay

This comment has been minimized.

Member

jay commented Jun 21, 2016

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.

@snikulov

This comment has been minimized.

Member

snikulov commented Jun 22, 2016

@jay @captain-caveman2k Just give me a hint where it should be written.

I've updated documentation in #891

snikulov added a commit to snikulov/curl that referenced this pull request Jun 22, 2016

@snikulov

This comment has been minimized.

Member

snikulov commented Jun 24, 2016

@jay @captain-caveman2k @bagder Any updates on this? Thank you.

@captain-caveman2k

This comment has been minimized.

Member

captain-caveman2k commented Jun 26, 2016

@snikulov Sorry I've been a little quiet for the last few days.

Thanks for the documentation additions - I have identified a few other pages that will need updating as well and listed those in your other PR.

Many thanks

@captain-caveman2k

This comment has been minimized.

Member

captain-caveman2k commented Jun 26, 2016

Additionally, did you have any plans to add support for GSSAPI (aka Kerberos v5) authentication as I believe LDAP supports it.

I also opened the following discussion on the mailing lists about using HTTP AUTH for LDAP.

@snikulov

This comment has been minimized.

Member

snikulov commented Jun 27, 2016

@captain-caveman2k Auto-negotiation on Windows will use Snego GSS-API (aka Kerberos v5) authentication by default.
So it's already there.

ldap

@snikulov

This comment has been minimized.

Member

snikulov commented Jul 1, 2016

@bagder @captain-caveman2k @jay Any other comments? I believe I've fixed all of your comments/issues.
Will it be merged soon?

@captain-caveman2k

This comment has been minimized.

Member

captain-caveman2k commented Jul 4, 2016

Hi @snikulov

Many thanks for your continued efforts on this - we still need to tidy the documentation up in the files that I mentioned in #891 but other than that I think we are pretty much ready.

I would like to see a way to force krb5 but that will probably have to wait until I have sorted out the merger of SASL and HTTP authentication methods which I will do after merging your changes.

As for timeframe, I'm sure you'll appreciate we are in the middle of a feature freeze at present so my plan is to add all of this in 7.51 which gives it's the proper testing period it deserves, always to get the documentation right and sort out the authentication mechanism.

@captain-caveman2k

This comment has been minimized.

Member

captain-caveman2k commented Aug 8, 2016

@snikulov I'm currently preparing the merge of this PR (after updating the documentation) and was wondering if we really need the line:

Curl_convert_UTF8_to_tchar((char*)user)

Rather than initiate another call to Curl_convert_UTF8_to_tchar() and a subsequent Curl_unicodefree() can we simply reference cred.User?

@snikulov

This comment has been minimized.

Member

snikulov commented Aug 8, 2016

@captain-caveman2k Was too long to remember, but one thing I've discovered - Curl_create_sspi_identity will split incoming string domain\username into structure members as domain and user which is not suitable for LDAP_AUTH_SIMPLE. Because in this case SSPI structure does not used and plain domain\user should be supplied https://msdn.microsoft.com/en-us/library/aa366156(v=vs.85).aspx.

@snikulov

This comment has been minimized.

Member

snikulov commented Aug 8, 2016

@captain-caveman2k @jay BTW, should I squash this PR into single commit?

@captain-caveman2k

This comment has been minimized.

Member

captain-caveman2k commented Aug 9, 2016

@snikulov No need to squash the PR for me personally as I have it squashed with your document changes along with my own from last night. If you want to squash it for readability of the PR then that is up to you.

@captain-caveman2k

This comment has been minimized.

Member

captain-caveman2k commented Aug 9, 2016

Thanks for the clarification @snikulov - I had forgotten that the domain was part of the user :(

I will probably push the single commit from last night that I have tonight - unless there are any other changes.

@snikulov

This comment has been minimized.

Member

snikulov commented Aug 9, 2016

@captain-caveman2k Steve, I've discovered that WinLDAP will not be found by CMake when OpenSSL enabled (see Appveyor build status to this PR).
Will fix it with another PR I'm currently working on.

@snikulov

This comment has been minimized.

Member

snikulov commented Aug 9, 2016

@captain-caveman2k Steve, one caveat - USE_WINDOWS_SSPI should be defined to build with Curl_create_sspi_identity().
When OpenSSL used on Windows, USE_WINDOWS_SSPI undefined.
Here are options

  1. Disable WinLDAP when OpenSSL used
  2. Wrap all AUTH methods with #ifdef USE_WINDOWS_SSPI except PLAIN.

Currently, I've implemented option 1 in #951 to solve build issue.

@captain-caveman2k

This comment has been minimized.

Member

captain-caveman2k commented Aug 9, 2016

@snikulov I don't mean to tread on your toes so to speak but here is my take on Option 2 - your thoughts would be much appreciated...

This hopefully does the following as well:

  • Will error if CURLAUTH_NTLM_WB and/or CURLAUTH_DIGEST_IE are the only HTTP Auth options or CURLAUTH_NONE is specified
  • Honors curls pre-processor directives for different authentication mechanisms
  • Choses the authentication mechanism based on our preferred order - Negotiate over NTLM and NTLM over Digest and Digest over plain or CURLAUTH_ANY is given for example
  • Supports NTLM and Digest for current user credientials
static int ldap_win_bind(struct connectdata *conn, LDAP *server,
                         const char *user, const char *passwd)
{
  int rc = 0;

#if defined(USE_WINDOWS_SSPI)
  ULONG method = 0;
#if defined(USE_SPNEGO)
  if(conn->data->set.httpauth & CURLAUTH_NEGOTIATE)
    method = LDAP_AUTH_NEGOTIATE;
  else
#endif
#if defined(USE_NTLM)
  if(conn->data->set.httpauth & CURLAUTH_NTLM)
    method = LDAP_AUTH_NTLM;
  else
#endif
#if !defined(CURL_DISABLE_CRYPTO_AUTH)
  if(conn->data->set.httpauth & CURLAUTH_DIGEST)
    method = LDAP_AUTH_DIGEST;
#endif
#endif

  if(user && passwd) {
#if defined(USE_WINDOWS_SSPI)
    if(method) {
      SEC_WINNT_AUTH_IDENTITY cred = { 0, };

      if(!Curl_create_sspi_identity(user, passwd, &cred)) {
        rc = ldap_bind_s(server, NULL, (TCHAR *) &cred, method);

        Curl_sspi_free_identity(&cred);
      }
      else
        rc = LDAP_NO_MEMORY;
    }
    else
#endif
    if(conn->data->set.httpauth & CURLAUTH_BASIC) {
      PCHAR inuser = Curl_convert_UTF8_to_tchar((char *) user);
      PCHAR inpass = Curl_convert_UTF8_to_tchar((char *) passwd);

      rc = ldap_bind_s(server, inuser, inpass, LDAP_AUTH_SIMPLE);

      Curl_unicodefree(inuser);
      Curl_unicodefree(inpass);
    }
    else
      rc = LDAP_AUTH_METHOD_NOT_SUPPORTED;
  }
#if defined(USE_WINDOWS_SSPI)
  else if(method)
    rc = ldap_bind_s(server, NULL, NULL, method);
#endif
  else
    rc = LDAP_INVALID_CREDENTIALS;

  return rc;
}

Plese note this is hot off the press so may be a bit rough and ready and certainly hasn't been tested here although it does compile for both OpenSSL and SSPI here ;-)

@snikulov

This comment has been minimized.

Member

snikulov commented Aug 9, 2016

@captain-caveman2k It's really not worth it to rise function complexity that way.

I wonder when Windows winldap.h start using OpenSSL crypto engine. Posible never.
So I prefer just fix build.

@captain-caveman2k

This comment has been minimized.

Member

captain-caveman2k commented Aug 10, 2016

@snikulov I did wonder that myself ;-)

However, I think we need to address the following problems:

  • 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
@snikulov

This comment has been minimized.

Member

snikulov commented Aug 12, 2016

@captain-caveman2k Sorry, Steve I'm currently busy with my job.
I'll get back ASAP.
If you in a rush - go ahead and update whatever you wish.

@captain-caveman2k

This comment has been minimized.

Member

captain-caveman2k commented Aug 12, 2016

@snikulov I know the feeling, hence my curl duties have been a bit lacking of late - 50 hour weeks plus commuting for a good few months :(

No rush from my point of view - It would have been "nice" to get it into this release but if not then there is always the next release which starts dev in 4 weeks time ;-)

@bagder bagder added the LDAP label Oct 16, 2016

@bagder

This comment has been minimized.

Member

bagder commented Oct 16, 2016

Taking this forward or do we close?

@snikulov

This comment has been minimized.

Member

snikulov commented Oct 17, 2016

@bagder I'll get back to it this week, or next week. We have some issues with build options handling. All other stuff are pretty solid, I think.

@snikulov

This comment has been minimized.

Member

snikulov commented Oct 26, 2016

@captain-caveman2k Hello, Steve. It's been a long time :)
I've finally fixed build issues.
Could you please review?

@snikulov

This comment has been minimized.

Member

snikulov commented May 12, 2017

@bagder Hello Daniel.
Looks like Steve @captain-caveman2k are too busy.
Could you please advise what should I do with this pull request? Thank you.

@bagder

bagder approved these changes May 15, 2017

If you're confident in the changes, I think you can merge this (squashed, right?).

You should also update the associated documentation so that users can actually know about this support!

@snikulov snikulov merged commit f0fe66f into curl:master May 23, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@snikulov

This comment has been minimized.

Member

snikulov commented May 23, 2017

@bagder Merged. Will update docs today.

snikulov added a commit to snikulov/curl that referenced this pull request May 23, 2017

snikulov added a commit that referenced this pull request May 23, 2017

vszakats added a commit to vszakats/curl that referenced this pull request Oct 9, 2018

ldap: show precise ldap call in error message on Windows
Also add a unique but common text ('bind via') to make it
easy to grep this specific failure regardless of platform.

Ref: curl#878
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment