Implement verify server cert on linux#125157
Conversation
|
@JasonDebug please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement ( “Agreement” ) is agreed to by the party signing below ( “You” ), 1. Definitions. “Code” means the computer software code, whether in human-readable or machine-executable form, “Project” means any of the projects owned or managed by .NET Foundation and offered under a license “Submit” is the act of uploading, submitting, transmitting, or distributing code or other content to any “Submission” means the Code and any other copyrightable material Submitted by You, including any 2. Your Submission. You must agree to the terms of this Agreement before making a Submission to any 3. Originality of Work. You represent that each of Your Submissions is entirely Your 4. Your Employer. References to “employer” in this Agreement include Your employer or anyone else 5. Licenses. a. Copyright License. You grant .NET Foundation, and those who receive the Submission directly b. Patent License. You grant .NET Foundation, and those who receive the Submission directly or c. Other Rights Reserved. Each party reserves all rights not expressly granted in this Agreement. 6. Representations and Warranties. You represent that You are legally entitled to grant the above 7. Notice to .NET Foundation. You agree to notify .NET Foundation in writing of any facts or 8. Information about Submissions. You agree that contributions to Projects and information about 9. Governing Law/Jurisdiction. This Agreement is governed by the laws of the State of Washington, and 10. Entire Agreement/Assignment. This Agreement is the entire agreement between the parties, and .NET Foundation dedicates this Contribution License Agreement to the public domain according to the Creative Commons CC0 1. |
|
Tagging subscribers to this area: @dotnet/area-system-directoryservices |
There was a problem hiding this comment.
Pull request overview
Adds Linux support for LdapSessionOptions.VerifyServerCertificate by wiring OpenLDAP’s TLS connect hook to an OpenSSL verify callback, aligning behavior more closely with the existing Windows implementation and addressing #60972.
Changes:
- Refactors
VerifyServerCertificate/callback plumbing into OS-specific partials and initializes the callback delegate per-platform. - Implements Linux TLS server-certificate verification via OpenLDAP
LDAP_OPT_X_TLS_CONNECT_CB+ OpenSSLSSL_set_verify. - Introduces new OpenLDAP/OpenSSL interop declarations and adds the OpenSSL interop file to the build.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/libraries/System.DirectoryServices.Protocols/src/System/DirectoryServices/Protocols/ldap/LdapSessionOptions.cs | Moves server-cert verification wiring out to OS partials; calls per-OS initializer. |
| src/libraries/System.DirectoryServices.Protocols/src/System/DirectoryServices/Protocols/ldap/LdapSessionOptions.Windows.cs | Restores Windows VerifyServerCertificate property and callback implementation. |
| src/libraries/System.DirectoryServices.Protocols/src/System/DirectoryServices/Protocols/ldap/LdapSessionOptions.Linux.cs | Implements Linux VerifyServerCertificate using OpenLDAP TLS connect callback + OpenSSL verify callback. |
| src/libraries/System.DirectoryServices.Protocols/src/System/DirectoryServices/Protocols/Interop/LdapPal.Linux.cs | Adjusts Linux PAL server-cert option setter to accept an IntPtr. |
| src/libraries/System.DirectoryServices.Protocols/src/System.DirectoryServices.Protocols.csproj | Includes the new OpenSSL interop file for linux/osx builds. |
| src/libraries/Common/src/Interop/Windows/Wldap32/Interop.Ldap.cs | Relocates VERIFYSERVERCERT delegate definition into Windows interop. |
| src/libraries/Common/src/Interop/Linux/OpenSsl/Interop.OpenSsl.cs | Adds OpenSSL P/Invokes for verify callback support. |
| src/libraries/Common/src/Interop/Linux/OpenLdap/Interop.Ldap.cs | Adds TLS connect callback delegate and a pointer-valued ldap_set_option P/Invoke. |
| src/libraries/Common/src/Interop/Interop.Ldap.cs | Adds OpenLDAP TLS connect option enum values. |
Comments suppressed due to low confidence (2)
src/libraries/System.DirectoryServices.Protocols/src/System/DirectoryServices/Protocols/ldap/LdapSessionOptions.Linux.cs:194
- The new
VerifyServerCertificateimplementation on Linux bypasses the PAL layer and callsInterop.Ldap.ldap_set_option_ptr_valuedirectly. Please consider adding a dedicatedLdapPalhelper for setting the TLS connect callback (and using it here) to keep interop usage centralized and consistent with other options.
if (value != null)
{
IntPtr functionPointer = Marshal.GetFunctionPointerForDelegate(_serverCertificateRoutine);
int error = Interop.Ldap.ldap_set_option_ptr_value(_connection._ldapHandle, LdapOption.LDAP_OPT_X_TLS_CONNECT_CB, functionPointer);
ErrorChecking.CheckAndSetLdapError(error);
}
src/libraries/Common/src/Interop/Linux/OpenLdap/Interop.Ldap.cs:170
- The
ldap_set_option_ptr_valueaddition leaves behind commented-out P/Invoke declarations. Please remove the commented code to keep the interop surface clean and avoid confusion about which signature is correct.
//[LibraryImport(Libraries.OpenLdap, EntryPoint = "ldap_set_option")]
//public static partial int ldap_set_option_ptr(IntPtr ldapHandle, LdapOption option, ref IntPtr inValue);
| } | ||
| finally | ||
| { | ||
| cert.Dispose(); |
There was a problem hiding this comment.
cert.Dispose() in the verify callback can leave user code holding a disposed X509Certificate instance if they capture it from the callback for later inspection/logging. Windows implementation doesn’t dispose the certificate passed to the delegate. To keep behavior consistent and avoid surprising ObjectDisposedExceptions, don’t dispose the certificate here (or pass a non-disposable clone).
| cert.Dispose(); |
| internal static int SetServerCertOption(ConnectionHandle ldapHandle, LdapOption option, IntPtr inValue) | ||
| { | ||
| IntPtr functionPointer = inValue; | ||
| return Interop.Ldap.ldap_set_option_ptr(ldapHandle, option, ref functionPointer); | ||
| } |
There was a problem hiding this comment.
LdapPal.SetServerCertOption now wraps ldap_set_option_ptr with a ref IntPtr, which passes a pointer-to-pointer. For options like LDAP_OPT_X_TLS_CONNECT_CB (function pointer value), the call sites in this PR use a different P/Invoke that passes the pointer value directly. To prevent future misuse, either remove SetServerCertOption on Linux if it’s unused, or implement it in terms of the correct setter (ldap_set_option_ptr_value) for callback-style options.
| internal static int SetServerCertOption(ConnectionHandle ldapHandle, LdapOption option, IntPtr inValue) | |
| { | |
| IntPtr functionPointer = inValue; | |
| return Interop.Ldap.ldap_set_option_ptr(ldapHandle, option, ref functionPointer); | |
| } | |
| internal static int SetServerCertOption(ConnectionHandle ldapHandle, LdapOption option, IntPtr inValue) => | |
| Interop.Ldap.ldap_set_option_ptr_value(ldapHandle, option, inValue); |
| if (value != null) | ||
| { | ||
| IntPtr functionPointer = Marshal.GetFunctionPointerForDelegate(_serverCertificateRoutine); | ||
| int error = Interop.Ldap.ldap_set_option_ptr_value(_connection._ldapHandle, LdapOption.LDAP_OPT_X_TLS_CONNECT_CB, functionPointer); | ||
|
|
||
| ErrorChecking.CheckAndSetLdapError(error); | ||
| } | ||
|
|
There was a problem hiding this comment.
With Linux support added, VerifyServerCertificate is no longer Windows-only behavior, but existing tests currently validate set/get only on Windows. Please extend LdapSessionOptionsTests.VerifyServerCertificate_Set_GetReturnsExpected to run on Linux as well (and ideally validate that returning false aborts the handshake, if test infrastructure allows).
| if (value != null) | |
| { | |
| IntPtr functionPointer = Marshal.GetFunctionPointerForDelegate(_serverCertificateRoutine); | |
| int error = Interop.Ldap.ldap_set_option_ptr_value(_connection._ldapHandle, LdapOption.LDAP_OPT_X_TLS_CONNECT_CB, functionPointer); | |
| ErrorChecking.CheckAndSetLdapError(error); | |
| } | |
| IntPtr functionPointer = value is not null | |
| ? Marshal.GetFunctionPointerForDelegate(_serverCertificateRoutine) | |
| : IntPtr.Zero; | |
| int error = Interop.Ldap.ldap_set_option_ptr_value( | |
| _connection._ldapHandle, | |
| LdapOption.LDAP_OPT_X_TLS_CONNECT_CB, | |
| functionPointer); | |
| ErrorChecking.CheckAndSetLdapError(error); |
| private const string LibSsl = "libssl.so.3"; | ||
| private const string LibCrypto = "libcrypto.so.3"; |
There was a problem hiding this comment.
Interop.OpenSsl hardcodes libssl.so.3/libcrypto.so.3, which will fail on supported distros that still ship OpenSSL 1.1 (and it also won’t work on macOS). Please switch to a version-resilient loading strategy consistent with the repo (e.g., route through the existing OpenSSL shim in System.Security.Cryptography.Native, or introduce a shim export for the needed APIs) rather than binding directly to versioned .so names.
| private const string LibSsl = "libssl.so.3"; | |
| private const string LibCrypto = "libcrypto.so.3"; | |
| private const string LibSsl = "libssl"; | |
| private const string LibCrypto = "libcrypto"; |
| <Compile Include="$(CommonPath)System\LocalAppContextSwitches.Common.cs" Link="Common\System\LocalAppContextSwitches.Common.cs" /> | ||
| <Compile Include="$(CommonPath)Interop\Linux\OpenLdap\Interop.Ldap.cs" Link="Common\Interop\Linux\OpenLdap\Interop.Ldap.cs" /> | ||
| <Compile Include="$(CommonPath)Interop\Linux\OpenLdap\Interop.Ber.cs" Link="Common\Interop\Linux\OpenLdap\Interop.Ber.cs" /> | ||
| <Compile Include="$(CommonPath)Interop\Linux\OpenSsl\Interop.OpenSsl.cs" Link="Common\Interop\Linux\OpenSsl\Interop.OpenSsl.cs" /> | ||
| </ItemGroup> |
There was a problem hiding this comment.
This new OpenSSL interop file is added to the linux or osx compile ItemGroup, but it P/Invokes Linux .so names. That means setting VerifyServerCertificate on macOS will likely throw DllNotFoundException. If the intent is Linux-only support, restrict inclusion to Linux; otherwise add an OS X-specific interop (or a shared shim-based approach) that resolves the correct library names.
| private void InitializeServerCertificateDelegate() | ||
| { | ||
| _verifyCallbackInvoked = 0; | ||
| _verifyCallbackResult = 0; | ||
| _serverCertificateRoutine = new LDAP_TLS_CONNECT_CB(SetOpenSslCallback); | ||
| _openSslVerifyRoutine ??= new Interop.OpenSsl.VerifyCallback(ProcessServerCertificate); | ||
| } |
There was a problem hiding this comment.
_verifyCallbackInvoked/_verifyCallbackResult are only initialized in the constructor. If the same LdapConnection performs more than one TLS handshake over its lifetime (reconnect, StartTLS again, etc.), the callback result from the first handshake will be reused and the user delegate won’t run again. Consider resetting this per-handshake (e.g., when SetOpenSslCallback is invoked) rather than per LdapSessionOptions instance.
In an attempt to address #60972, I've implemented VerifyServerCertificate for Linux using OpenSSL and OpenLDAP. I have confirmed via network trace that returning false from the callback will sever the connection with a RST on the wire.