-
Notifications
You must be signed in to change notification settings - Fork 5k
Fix LdapConnection with AuthType.External #113238
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
Conversation
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.
PR Overview
This PR fixes the issue with LdapConnection when using AuthType.External by updating the LDAP bind call. Key changes include:
- Replacing Unsafe.NullRef with IntPtr.Zero for external authentication.
- Adding a new Interop.Ldap.ldap_bind_s overload to support the updated external binding.
Reviewed Changes
File | Description |
---|---|
src/libraries/System.DirectoryServices.Protocols/src/System/DirectoryServices/Protocols/ldap/LdapConnection.Windows.cs | Updated the bind call to pass IntPtr.Zero when tempCredential is null and AuthType is External. |
src/libraries/Common/src/Interop/Windows/Wldap32/Interop.Ldap.cs | Introduced a new overload of ldap_bind_s accepting an IntPtr unused parameter for external authentication. |
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
src/libraries/System.DirectoryServices.Protocols/src/System/DirectoryServices/Protocols/ldap/LdapConnection.Windows.cs:65
- Verify that using IntPtr.Zero instead of Unsafe.NullRef<SEC_WINNT_AUTH_IDENTITY_EX>() correctly signals a null credential for external binding, ensuring the expected behavior is maintained.
=> tempCredential == null && AuthType == AuthType.External ? Interop.Ldap.ldap_bind_s(_ldapHandle, null, IntPtr.Zero, method) : Interop.Ldap.ldap_bind_s(_ldapHandle, null, cred, method);
src/libraries/Common/src/Interop/Windows/Wldap32/Interop.Ldap.cs:17
- Double-check that the new ldap_bind_s overload with an IntPtr parameter harmonizes with the existing binding scenarios and does not introduce overload resolution issues.
[LibraryImport(Libraries.Wldap32, EntryPoint = "ldap_bind_sW", StringMarshalling = StringMarshalling.Utf16)]
@@ -62,6 +62,6 @@ private int InternalConnectToServer() | |||
} | |||
|
|||
private int InternalBind(NetworkCredential tempCredential, SEC_WINNT_AUTH_IDENTITY_EX cred, BindMethod method) | |||
=> tempCredential == null && AuthType == AuthType.External ? Interop.Ldap.ldap_bind_s(_ldapHandle, null, Unsafe.NullRef<SEC_WINNT_AUTH_IDENTITY_EX>(), method) : Interop.Ldap.ldap_bind_s(_ldapHandle, null, cred, method); | |||
=> tempCredential == null && AuthType == AuthType.External ? Interop.Ldap.ldap_bind_s(_ldapHandle, null, IntPtr.Zero, method) : Interop.Ldap.ldap_bind_s(_ldapHandle, null, cred, method); |
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.
A recent comment in the issue says that calling ldap_sasl_bind_s
(instead of ldap_bind_s
) may be the correct method to call for AuthType.External
which I assume we pass "EXTERNAL" in for the AuthMechanism
parameter.
Linux uses ldap_sasl_interactive_bind
for External but that is not supported in winldap.
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 this is the case where we were calling the wrong ldap bind method for AuthType.External
on Windows, then it has been that way since it was authored, and seemingly not used (successfully) by anyone.
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.
Having a look back at the .NETFramework code for this, it has a similar codepath:
if ((tempCredential == null) && (AuthType == AuthType.External))
{
error = Wldap32.ldap_bind_s(ldapHandle, null, null, method);
}
else
{
error = Wldap32.ldap_bind_s(ldapHandle, null, cred, method);
}
With ldap_bind_s
defined as
[DllImport("wldap32.dll", CallingConvention=CallingConvention.Cdecl, EntryPoint="ldap_bind_sW", CharSet=CharSet.Unicode)]
public static extern int ldap_bind_s([In]ConnectionHandle ldapHandle, string dn, SEC_WINNT_AUTH_IDENTITY_EX credentials, BindMethod method);
So it does seem to be equivalent to the .NETFramework code.
I did a scan over sources internally and I do see some usage of this that looks similar to the original report - so I assume it does work.
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.
Looks good; at least gets us back to earlier behavior.
Fix #113154
Details in issue