Skip to content
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

snikulov
Copy link
Member

@snikulov 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
Copy link

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

@snikulov
Copy link
Member Author

Depends on #878

cred.UserLength = strlen(user);
cred.Password = passwd;
cred.PasswordLength = strlen(passwd);
cred.Flags = SEC_WINNT_AUTH_IDENTITY_ANSI;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@jay jay Jun 15, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jay is checksrc work on Windows?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@snikulov snikulov Jun 15, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member Author

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

cred.PasswordLength = _tcslen(passwd);
cred.Flags = (sizeof(TCHAR) == sizeof(char)
? SEC_WINNT_AUTH_IDENTITY_ANSI
: SEC_WINNT_AUTH_IDENTITY_UNICODE);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@snikulov
Copy link
Member Author

@jay Fixed and squashed with previous commit.

@captain-caveman2k
Copy link
Contributor

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
Copy link
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
Copy link
Contributor

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
Copy link
Member Author

@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 snikulov force-pushed the win_ldap_bind branch 3 times, most recently from c01679f to 227ca5a Compare June 20, 2016 11:33
@snikulov
Copy link
Member Author

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

@captain-caveman2k
Copy link
Contributor

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
Copy link
Member Author

@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
Copy link
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
Copy link
Member Author

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
Copy link
Member Author

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

@captain-caveman2k
Copy link
Contributor

@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
Copy link
Contributor

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
Copy link
Member Author

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

ldap

@snikulov
Copy link
Member Author

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?

@snikulov
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Contributor

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
Copy link
Member Author

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
Copy link
Contributor

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
Copy link
Member Author

@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
Copy link
Contributor

@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
Copy link
Member

bagder commented Oct 16, 2016

Taking this forward or do we close?

@snikulov
Copy link
Member Author

@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 snikulov force-pushed the win_ldap_bind branch 2 times, most recently from 25d30a4 to 6df1e27 Compare October 26, 2016 20:54
@snikulov
Copy link
Member Author

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

@snikulov
Copy link
Member Author

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

Copy link
Member

@bagder bagder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@snikulov
Copy link
Member Author

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
Also add a unique but common text ('bind via') to make it
easy to grep this specific failure regardless of platform.

Ref: curl#878
@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

Successfully merging this pull request may close these issues.

None yet

6 participants