Compatibility between libldap and wldap32.dll #36949
Conversation
… is available. Better compatibility between libldap and wldap32.dll
| @@ -1100,18 +1098,6 @@ private void BindHelper(NetworkCredential newCredential, bool needSetCredential) | |||
| { | |||
| error = LdapPal.BindToDirectory(_ldapHandle, null, null); | |||
| } | |||
| else if (AuthType == AuthType.Basic) | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| { | ||
| error = Interop.ldap_simple_bind(_ldapHandle, cred.user, cred.password); | ||
| var tempDomainName = new StringBuilder(100); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 (tempCredential == null && (AuthType == AuthType.External || AuthType == AuthType.Kerberos)) | ||
| { | ||
| error = BindSasl(); | ||
| return BindSasl(); |
There was a problem hiding this comment.
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.
| } | ||
|
|
||
| return error; | ||
| return Interop.ldap_simple_bind(_ldapHandle, cred.user, cred.password); |
There was a problem hiding this comment.
NIT: extra line at the end of the method
| } | ||
| else | ||
|
|
||
| if (tempCredential != null) |
There was a problem hiding this comment.
Shouldn't we also check if AuthType is basic here?
|
Sorry that this PR kind of got lost through the cracks. Can you please rebase the PR as well as add unit tests that use different types of credentials? I know that most of them would require to test against an actual server, but it would be good if you could do that on this test class: https://github.com/dotnet/runtime/blob/master/src/libraries/System.DirectoryServices.Protocols/tests/DirectoryServicesProtocolsTests.cs which today only runs against an actual server when configured, that way we can run the tests and make sure that the code in this PR won't break an existing AuthType and credentials input combo. |
|
@joperezr given no response from the original author, can you go ahead and close this and recreate a PR cherry-picking his commit if you think this is a valuable fix? |
|
I agree, let's go ahead and close this one and I'll add more details on the issue and a pointer to this PR in case anybody wants to pick it up in the future and add the remaining tests that are still missing from here. |
ghost
locked as resolved and limited conversation to collaborators
Under windows environment isn't necessary to inform domain name at LDAP Connection. But at Linux is.
For a better compatibility between environments, concat Domain Name at Username in Linux environment.
Also remove Basic validation from LdapConnection common environment, since it will have specific validation in both environments and better readability.
#36947