-
-
Notifications
You must be signed in to change notification settings - Fork 6.3k
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
Conversation
By analyzing the blame information on this pull request, we identified @captain-caveman2k, @bagder and @gknauf to be potential reviewers |
Depends on #878 |
cred.UserLength = strlen(user); | ||
cred.Password = passwd; | ||
cred.PasswordLength = strlen(passwd); | ||
cred.Flags = SEC_WINNT_AUTH_IDENTITY_ANSI; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 cur
l) 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.
@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); |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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... :-/
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
@jay Fixed and squashed with previous commit. |
Many thanks for you efforts here @snikulov - it looks good. My only comments would be:
|
Steve tests are failing because of #881 not this afaik. The
|
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 ;-) |
@captain-caveman2k Steve, I've updated per your comments. If all OK, I'll re-base all changes into single commit. |
c01679f
to
227ca5a
Compare
@jay @captain-caveman2k Any other comments, suggestions? |
Cheers @snikulov My only comments would be:
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. |
@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. |
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. |
@jay @captain-caveman2k Just give me a hint where it should be written. I've updated documentation in #891 |
@jay @captain-caveman2k @bagder Any updates on this? Thank you. |
@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 |
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. |
@captain-caveman2k Auto-negotiation on Windows will use Snego GSS-API (aka Kerberos v5) authentication by default. |
@bagder @captain-caveman2k @jay Any other comments? I believe I've fixed all of your comments/issues. |
@captain-caveman2k Steve, I've discovered that WinLDAP will not be found by CMake when OpenSSL enabled (see Appveyor build status to this PR). |
@captain-caveman2k Steve, one caveat -
Currently, I've implemented option 1 in #951 to solve build issue. |
@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:
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 ;-) |
@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. |
@snikulov I did wonder that myself ;-) However, I think we need to address the following problems:
In addition to this it would be really nice to:
|
@captain-caveman2k Sorry, Steve I'm currently busy with my job. |
@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 ;-) |
Taking this forward or do we close? |
@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. |
25d30a4
to
6df1e27
Compare
@captain-caveman2k Hello, Steve. It's been a long time :) |
6df1e27
to
0d903a6
Compare
0d903a6
to
de51f57
Compare
@bagder Hello Daniel. |
There was a problem hiding this 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!
@bagder Merged. Will update docs today. |
Also add a unique but common text ('bind via') to make it easy to grep this specific failure regardless of platform. Ref: curl#878
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
).