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.

Sign up for GitHub

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

Compatibility between libldap and wldap32.dll #36949

Closed
wants to merge 1 commit into from
Closed

Compatibility between libldap and wldap32.dll #36949

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 15 additions & 5 deletions ...oryServices.Protocols/src/System/DirectoryServices/Protocols/ldap/LdapConnection.Linux.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using System.Diagnostics;
using System.Net;
using System.Runtime.InteropServices;
using System.Text;

namespace System.DirectoryServices.Protocols
{
Expand All @@ -24,17 +25,26 @@ private int InternalConnectToServer()

private int InternalBind(NetworkCredential tempCredential, SEC_WINNT_AUTH_IDENTITY_EX cred, BindMethod method)
{
int error;
if (tempCredential == null && (AuthType == AuthType.External || AuthType == AuthType.Kerberos))
{
error = BindSasl();
return BindSasl();
Copy link
Member

Choose a reason for hiding this comment

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

Any specific reason why you wanted to have multiple return points? Usually I try to keep single return points when possible to make the control flow more readable and also cause it might make the jitted code slightly faster, so if there is no specific reason I would just keep it with the temp variable.

}
else

if (tempCredential != null)
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we also check if AuthType is basic here?

{
error = Interop.ldap_simple_bind(_ldapHandle, cred.user, cred.password);
var tempDomainName = new StringBuilder(100);
Copy link
Member

Choose a reason for hiding this comment

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

This might be a dangerous assumption, when AuthType is basic that may be fine, but here we may have cases were people try to use credentials in the form user@DOMAIN.COM which will basically be broken by this case, or people could have also already passed in a credential which already was in the form DOMAIN\user which would now be broken as well as we would pass in DOMAIN\DOMAIN\user

Copy link
Author

Choose a reason for hiding this comment

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

Except by choosing AuthType External or Kerberos you run into ldap_simple_bind, which always uses LDAP_AUTH_SIMPLE.
I figure out it by looking at ldap_simple_bind_s and ldap_bind

If people pass credentials in DOMAIN\user form and for some reason the project is moved to a Windows environment they will get Credential Error. It's whats happening now for those environments who isn't joined in the realm.

or people could have also already passed in a credential which already was in the form DOMAIN\user which would now be broken as well as we would pass in DOMAIN\DOMAIN\user

Actually this same situation could happen in Windows, if developer choose Basic and user pass credential as DOMAIN\user. It's concating DOMAIN\DOMAIN\user. Maybe some specific validation whether user already typed domain?

This assumption is a try to minimize impact of diff's between Linux and Windows environments.

if (!string.IsNullOrEmpty(cred.domain))
{
tempDomainName.Append(cred.domain);
tempDomainName.Append('\\');
}

tempDomainName.Append(cred.user);
return Interop.ldap_simple_bind(_ldapHandle, tempDomainName.ToString(), cred.password);
}

return error;
return Interop.ldap_simple_bind(_ldapHandle, cred.user, cred.password);
Copy link
Member

Choose a reason for hiding this comment

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

NIT: extra line at the end of the method


}

private int BindSasl()
Expand Down
22 changes: 21 additions & 1 deletion ...yServices.Protocols/src/System/DirectoryServices/Protocols/ldap/LdapConnection.Windows.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

using System.Diagnostics;
using System.Net;
using System.Text;

namespace System.DirectoryServices.Protocols
{
Expand Down Expand Up @@ -38,6 +39,25 @@ private int InternalConnectToServer()
}

private int InternalBind(NetworkCredential tempCredential, SEC_WINNT_AUTH_IDENTITY_EX cred, BindMethod method)
=> tempCredential == null && AuthType == AuthType.External ? Interop.ldap_bind_s(_ldapHandle, null, null, method) : Interop.ldap_bind_s(_ldapHandle, null, cred, method);
{
if (tempCredential != null && AuthType == AuthType.Basic)
{
var tempDomainName = new StringBuilder(100);
if (!string.IsNullOrEmpty(cred.domain))
{
tempDomainName.Append(cred.domain);
tempDomainName.Append('\\');
}

tempDomainName.Append(cred.user);
return Interop.ldap_simple_bind_s(_ldapHandle, tempDomainName.ToString(), cred.password);
}

if (tempCredential == null && AuthType == AuthType.External)
return Interop.ldap_bind_s(_ldapHandle, null, null, method);

return Interop.ldap_bind_s(_ldapHandle, null, cred, method);
}

}
}
62 changes: 16 additions & 46 deletions ...DirectoryServices.Protocols/src/System/DirectoryServices/Protocols/ldap/LdapConnection.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System.Globalization;
using System.Net;
using System.Collections;
using System.ComponentModel;
Expand All @@ -12,7 +11,6 @@
using System.Xml;
using System.Threading;
using System.Security.Cryptography.X509Certificates;
using System.Diagnostics.CodeAnalysis;

namespace System.DirectoryServices.Protocols
{
Expand Down Expand Up @@ -1100,18 +1098,6 @@ private void BindHelper(NetworkCredential newCredential, bool needSetCredential)
{
error = LdapPal.BindToDirectory(_ldapHandle, null, null);
}
else if (AuthType == AuthType.Basic)
Copy link
Member

Choose a reason for hiding this comment

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

In here we were already doing the append of the domain if authtype was basic, and this was happening in Linux as well so not sure why you moved that into Internal Bind of each platform instead.

Copy link
Author

Choose a reason for hiding this comment

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

I've moved to Internal, because in Linux always seems to use ldap_simple_bind.

BindToDirectory(ConnectionHandle ld, string who, string passwd) => Interop.ldap_simple_bind(ld, who, passwd);

Inside Linux if not External or Kerberos will run the same method above:

if (tempCredential == null && (AuthType == AuthType.External || AuthType == AuthType.Kerberos))
//
else
{
    error = Interop.ldap_simple_bind(_ldapHandle, cred.user, cred.password);
}

So, you don't think it is reasonable to move it into here? If you run out the same method, why not write it once and the same rules? And after all libldap only use AUTH_SIMPLE under the hood, which mean that doesn't matter what AuthType the user have choosed.

These was my decisions behind this code.

{
var tempDomainName = new StringBuilder(100);
if (domainName != null && domainName.Length != 0)
{
tempDomainName.Append(domainName);
tempDomainName.Append('\\');
}

tempDomainName.Append(username);
error = LdapPal.BindToDirectory(_ldapHandle, tempDomainName.ToString(), password);
}
else
{
var cred = new SEC_WINNT_AUTH_IDENTITY_EX()
Expand All @@ -1129,42 +1115,26 @@ private void BindHelper(NetworkCredential newCredential, bool needSetCredential)
if (tempCredential != null)
{
cred.user = username;
cred.userLength = (username == null ? 0 : username.Length);
cred.userLength = username?.Length ?? 0;
cred.domain = domainName;
cred.domainLength = (domainName == null ? 0 : domainName.Length);
cred.domainLength = domainName?.Length ?? 0;
cred.password = password;
cred.passwordLength = (password == null ? 0 : password.Length);
}

BindMethod method = BindMethod.LDAP_AUTH_NEGOTIATE;
switch (AuthType)
{
case AuthType.Negotiate:
method = BindMethod.LDAP_AUTH_NEGOTIATE;
break;
case AuthType.Kerberos:
method = BindMethod.LDAP_AUTH_NEGOTIATE;
break;
case AuthType.Ntlm:
method = BindMethod.LDAP_AUTH_NTLM;
break;
case AuthType.Digest:
method = BindMethod.LDAP_AUTH_DIGEST;
break;
case AuthType.Sicily:
method = BindMethod.LDAP_AUTH_SICILY;
break;
case AuthType.Dpa:
method = BindMethod.LDAP_AUTH_DPA;
break;
case AuthType.Msn:
method = BindMethod.LDAP_AUTH_MSN;
break;
case AuthType.External:
method = BindMethod.LDAP_AUTH_EXTERNAL;
break;
cred.passwordLength = password?.Length ?? 0;
}

var method = AuthType switch
{
AuthType.Negotiate => BindMethod.LDAP_AUTH_NEGOTIATE,
AuthType.Kerberos => BindMethod.LDAP_AUTH_NEGOTIATE,
AuthType.Ntlm => BindMethod.LDAP_AUTH_NTLM,
AuthType.Digest => BindMethod.LDAP_AUTH_DIGEST,
AuthType.Sicily => BindMethod.LDAP_AUTH_SICILY,
AuthType.Dpa => BindMethod.LDAP_AUTH_DPA,
AuthType.Msn => BindMethod.LDAP_AUTH_MSN,
AuthType.External => BindMethod.LDAP_AUTH_EXTERNAL,
_ => BindMethod.LDAP_AUTH_NEGOTIATE
};

error = InternalBind(tempCredential, cred, method);
}

Expand Down