-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
[release/9.0-staging] Add support for LDAPTLS_CACERTDIR \ TrustedCertificateDirectory #112531
base: release/9.0-staging
Are you sure you want to change the base?
[release/9.0-staging] Add support for LDAPTLS_CACERTDIR \ TrustedCertificateDirectory #112531
Conversation
Note regarding the
|
1 similar comment
Note regarding the
|
Tagging subscribers to this area: @dotnet/area-system-directoryservices, @jay98014 |
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.
Copilot reviewed 5 out of 10 changed files in this pull request and generated 1 comment.
Files not reviewed (5)
- src/libraries/Common/tests/System/DirectoryServices/LDAP.Configuration.xml: Language not supported
- src/libraries/System.DirectoryServices.Protocols/src/Resources/Strings.resx: Language not supported
- src/libraries/System.DirectoryServices.Protocols/src/System/DirectoryServices/Protocols/ldap/LdapConnection.cs: Evaluated as low risk
- src/libraries/Common/src/Interop/Interop.Ldap.cs: Evaluated as low risk
- src/libraries/System.DirectoryServices.Protocols/ref/System.DirectoryServices.Protocols.cs: Evaluated as low risk
Comments suppressed due to low confidence (4)
src/libraries/System.DirectoryServices.Protocols/tests/DirectoryServicesProtocolsTests.cs:709
- Ensure that the new test cases properly cover the new functionality for 'StartNewTlsSessionContext' and 'TrustedCertificatesDirectory'.
[ConditionalFact(nameof(UseTls))]
src/libraries/System.DirectoryServices.Protocols/tests/DirectoryServicesProtocolsTests.cs:732
- Ensure that the new test cases properly cover the new functionality for 'StartNewTlsSessionContext' and 'TrustedCertificatesDirectory'.
[ConditionalFact(nameof(UseTls))]
src/libraries/System.DirectoryServices.Protocols/tests/DirectoryServicesProtocolsTests.cs:744
- Ensure that the new test cases properly cover the new functionality for 'TrustedCertificatesDirectory'.
[ConditionalFact(nameof(LdapConfigurationExists))]
src/libraries/System.DirectoryServices.Protocols/tests/DirectoryServicesProtocolsTests.cs:754
- Ensure that the new test cases properly cover the new functionality for 'StartNewTlsSessionContext'.
[ConditionalFact(nameof(LdapConfigurationExists))]
/// which can be done by using <code>openssl rehash .</code> or <code>c_rehash .</code> in the directory | ||
/// containing the certificate files. | ||
/// </remarks> | ||
/// <exception cref="DirectoryNotFoundException">The directory not exist.</exception> |
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.
Minor grammar issue: 'The directory not exist.' should be 'The directory does not exist.'
/// <exception cref="DirectoryNotFoundException">The directory not exist.</exception> | |
/// <exception cref="DirectoryNotFoundException">The directory does not exist.</exception> |
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
@@ -0,0 +1,67 @@ | |||
<?xml version="1.0" encoding="utf-8"?> | |||
<!-- https://learn.microsoft.com/dotnet/fundamentals/package-validation/diagnostic-ids --> | |||
<Suppressions xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns:xsd="http://www.w3.org/2001/XMLSchema"> |
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.
This is OK for now - but if we wanted to make this more common we could let projects opt out of this setting -
Line 41 in b61c07c
<EnableStrictModeForBaselineValidation>true</EnableStrictModeForBaselineValidation> |
Backport of #111877 to release/9.0-staging
Fixes #104260 for v9.0.x
Backport for v8: #112530
/cc @steveharter @JasonDebug
Customer Impact
Adds two new members to System.DirectoryServices.Protocols.LdapSessionOptions to forward to LDAP APIs on Linux\OSX that help with server certificate validation. On Windows, server certificate validation is supported by setting a callback on
LdapSessionOptions.VerifyServerCertificates()
, but that is not supported on Linux\OSX.Regression
Testing
Several months ago, we provided a package with the proposed fix which was validated by the support engineer and customer requesting this feature. Once it was added to v10, the validation was done again. Also, local validation on Linux Ubuntu was done and verification tests were added.
Automated tests here are not supported with current infrastructure as it requires LDAP server support. Currently this must be manually tested.
Risk
Low - although we normally don't add new APIs in a servicing release, it is safer here since this assembly does not ship inbox with .NET so risk is low for versioning issues.