Add support for LDAPTLS_CACERTDIR \ TrustedCertificateDirectory #111877

steveharter · 2025-01-27T23:36:15Z

Note that the two new members here throw PlatformNotSupportedException in the appropriate non-Linux cases. However, this was not carried forward to the many other pre-existing members that only run on Linux or non-Linux. Assuming we want to do this, I will create an issue to tract that work but do not plan to address that work in this PR. We could also remove the support for PlatformNotSupportedException for now in this PR.

dotnet-issue-labeler · 2025-01-27T23:36:23Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

dotnet-issue-labeler · 2025-01-27T23:36:24Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

src/libraries/Common/tests/System/DirectoryServices/LDAP.Configuration.xml

ericstj

Just had a couple questions about testing / input validation.

...ryServices.Protocols/src/System/DirectoryServices/Protocols/ldap/LdapSessionOptions.Linux.cs

src/libraries/System.DirectoryServices.Protocols/tests/DirectoryServicesProtocolsTests.cs

buyaa-n · 2025-01-29T23:11:07Z

src/libraries/System.DirectoryServices.Protocols/ref/System.DirectoryServices.Protocols.cs

+        [System.Runtime.Versioning.UnsupportedOSPlatformAttribute("android")]
+        [System.Runtime.Versioning.UnsupportedOSPlatformAttribute("browser")]
+        [System.Runtime.Versioning.UnsupportedOSPlatformAttribute("ios")]
+        [System.Runtime.Versioning.UnsupportedOSPlatformAttribute("tvos")]


These are redundant, the assembly has <UnsupportedOSPlatforms>browser;android;ios;tvos</UnsupportedOSPlatforms> attribute
https://github.com/dotnet/runtime/blob/f552567173b706812d11579f8059a14a748dab81/src/libraries/System.DirectoryServices.Protocols/Directory.Build.props#L6C1-L6C78

Suggested change

[System.Runtime.Versioning.UnsupportedOSPlatformAttribute("android")]

[System.Runtime.Versioning.UnsupportedOSPlatformAttribute("browser")]

[System.Runtime.Versioning.UnsupportedOSPlatformAttribute("ios")]

[System.Runtime.Versioning.UnsupportedOSPlatformAttribute("tvos")]

Thanks Buyaa! Removed those.

Removed from source but not from ref

Thanks again - done.

I'm still seeing these, @steveharter. I think we can omit the attributes already defined at the assembly level, and just add the one additional windows one here?

Nevermind, for some reason was looking at subset of commits.

I'm assuming the previously excluded browser;android;ios;tvos were because the browser and mobile platforms don't support LDAP \ DirectoryServices.

buyaa-n · 2025-01-29T23:37:29Z

src/libraries/System.DirectoryServices.Protocols/ref/System.DirectoryServices.Protocols.cs

+        [System.Runtime.Versioning.UnsupportedOSPlatformAttribute("ios")]
+        [System.Runtime.Versioning.UnsupportedOSPlatformAttribute("tvos")]
+        [System.Runtime.Versioning.UnsupportedOSPlatformAttribute("windows")]
+        public void StartNewTlsSessionContext() { }
        public bool TcpKeepAlive { get { throw null; } set { } }
        public System.DirectoryServices.Protocols.VerifyServerCertificateCallback VerifyServerCertificate { get { throw null; } set { } }


Not sure if it changed but, we were talking to make VerifyServerCertificate windows only with this change

Suggested change

public System.DirectoryServices.Protocols.VerifyServerCertificateCallback VerifyServerCertificate { get { throw null; } set { } }

[System.Runtime.Versioning.SupportedOSPlatformAttribute("windows")]

public System.DirectoryServices.Protocols.VerifyServerCertificateCallback VerifyServerCertificate { get { throw null; } set { } }

Thanks Buyaa! I'll put up another PR for that (per this PR's description I elected not to do any of that, but VerifyServerCertificateCallback has caused a lot of pain so I will do that one-off).

src/libraries/System.DirectoryServices.Protocols/ref/System.DirectoryServices.Protocols.cs

ericstj · 2025-02-05T20:13:31Z

...ryServices.Protocols/src/System/DirectoryServices/Protocols/ldap/LdapSessionOptions.Linux.cs

+
+            set
+            {
+                if (!Directory.Exists(value))


Do we know if a relative path is OK to pass to LDAP? Will it resolve in the same way that Directory.Exists does?

Yes it seems to work locally. All other path-based APIs support relative paths AFAIK and both Directory.Exists() and LDAP client are based upon the current working directory of the current application (and LDAP client is in-process).

ericstj · 2025-02-05T20:17:23Z

src/libraries/System.DirectoryServices.Protocols/tests/DirectoryServicesProtocolsTests.cs

+        {
+            using (var connection = GetConnection(bind: false))
+            {
+                Assert.Throws<DirectoryNotFoundException>(() => connection.SessionOptions.TrustedCertificatesDirectory = "nonexistent");


What happens if the directory is deleted after our check then user calls StartNewTlsSessionContext, do we care?

We don't care. We would throw a vague exception like "unable to connect to server" based on the underlying LDAP error code.

ericstj

Looks good, just a couple small questions about testing / edge case.

* main: (23 commits) add important remarks to NrbfDecoder (dotnet#111286) docs: fix spelling grammar and missing words in clr-code-guide.md (dotnet#112222) Consider type declaration order in MethodImpls (dotnet#111998) Add a feature flag to not use GVM in Linq Select (dotnet#109978) [cDAC] Implement ISOSDacInterface::GetMethodDescPtrFromIp (dotnet#110755) Restructure JSImport/JSExport generators to share more code and utilize more Microsoft.Interop.SourceGeneration shared code (dotnet#107769) Add more detailed explanations to control-flow RegexOpcode values (dotnet#112170) Add repo-specific condition to labeling workflows (dotnet#112169) Fix bad assembly when a nested exported type is marked via link.xml (dotnet#107945) Make `CalculateAssemblyAction` virtual. (dotnet#112154) JIT: Enable reusing profile-aware DFS trees between phases (dotnet#112198) Add support for LDAPTLS_CACERTDIR \ TrustedCertificateDirectory (dotnet#111877) JIT: Support custom `ClassLayout` instances with GC pointers in them (dotnet#112064) Factor positive lookaheads better into find optimizations (dotnet#112107) Add ImmutableCollectionsMarshal.AsMemory (dotnet#112177) [mono] ILStrip write directly to the output filestream (dotnet#112142) Allow the NativeAOT runtime pack to be specified as the ILC runtime package (dotnet#111876) JIT: some reworking for conditional escape analysis (dotnet#112194) Replace HELPER_METHOD_FRAME with DynamicHelperFrame in patchpoints (dotnet#112025) [Android] Decouple runtime initialization and entry point execution for Android sample (dotnet#111742) ...

steveharter · 2025-02-13T15:47:53Z

/backport to release/8.0-staging
/backport to release/9.0-staging

github-actions · 2025-02-13T15:48:05Z

Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/13311570389

github-actions · 2025-02-13T15:49:47Z

@steveharter backporting to "release/8.0-staging" failed, the patch most likely resulted in conflicts:

$ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patch

Applying: Add support for setting CertificateDirectory
Using index info to reconstruct a base tree...
M	src/libraries/Common/src/Interop/Interop.Ldap.cs
M	src/libraries/Common/tests/System/DirectoryServices/LDAP.Configuration.xml
M	src/libraries/System.DirectoryServices.Protocols/ref/System.DirectoryServices.Protocols.cs
M	src/libraries/System.DirectoryServices.Protocols/src/System/DirectoryServices/Protocols/ldap/LdapConnection.cs
M	src/libraries/System.DirectoryServices.Protocols/tests/DirectoryServicesProtocolsTests.cs
M	src/libraries/System.DirectoryServices.Protocols/tests/DirectoryServicesTestHelpers.cs
Falling back to patching base and 3-way merge...
Auto-merging src/libraries/System.DirectoryServices.Protocols/tests/DirectoryServicesTestHelpers.cs
Auto-merging src/libraries/System.DirectoryServices.Protocols/tests/DirectoryServicesProtocolsTests.cs
CONFLICT (content): Merge conflict in src/libraries/System.DirectoryServices.Protocols/tests/DirectoryServicesProtocolsTests.cs
Auto-merging src/libraries/System.DirectoryServices.Protocols/src/System/DirectoryServices/Protocols/ldap/LdapConnection.cs
Auto-merging src/libraries/System.DirectoryServices.Protocols/ref/System.DirectoryServices.Protocols.cs
Auto-merging src/libraries/Common/tests/System/DirectoryServices/LDAP.Configuration.xml
CONFLICT (content): Merge conflict in src/libraries/Common/tests/System/DirectoryServices/LDAP.Configuration.xml
Auto-merging src/libraries/Common/src/Interop/Interop.Ldap.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config set advice.mergeConflict false"
Patch failed at 0001 Add support for setting CertificateDirectory
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

steveharter · 2025-02-13T15:53:25Z

/backport to release/9.0-staging

github-actions · 2025-02-13T15:53:36Z

Started backporting to release/9.0-staging: https://github.com/dotnet/runtime/actions/runs/13311678524

github-actions · 2025-02-13T15:55:17Z

@steveharter backporting to "release/9.0-staging" failed, the patch most likely resulted in conflicts:

$ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patch

Applying: Add support for setting CertificateDirectory
Using index info to reconstruct a base tree...
M	src/libraries/Common/tests/System/DirectoryServices/LDAP.Configuration.xml
M	src/libraries/System.DirectoryServices.Protocols/ref/System.DirectoryServices.Protocols.cs
M	src/libraries/System.DirectoryServices.Protocols/tests/DirectoryServicesProtocolsTests.cs
Falling back to patching base and 3-way merge...
Auto-merging src/libraries/System.DirectoryServices.Protocols/tests/DirectoryServicesProtocolsTests.cs
Auto-merging src/libraries/System.DirectoryServices.Protocols/ref/System.DirectoryServices.Protocols.cs
Auto-merging src/libraries/Common/tests/System/DirectoryServices/LDAP.Configuration.xml
CONFLICT (content): Merge conflict in src/libraries/Common/tests/System/DirectoryServices/LDAP.Configuration.xml
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config set advice.mergeConflict false"
Patch failed at 0001 Add support for setting CertificateDirectory
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

…et#111877)

steveharter self-assigned this Jan 27, 2025

dotnet-issue-labeler bot added area-System.DirectoryServices new-api-needs-documentation labels Jan 27, 2025

Add support for setting CertificateDirectory

510c98b

steveharter force-pushed the FixIssue-104260 branch from 8b298a5 to 510c98b Compare January 28, 2025 16:34

Update API based on review; update doc

4a4427e

steveharter changed the title ~~Add support for setting CertificateDirectory~~ Add support for LDAPTLS_CACERTDIR \ TrustedCertificateDirectory Jan 28, 2025

steveharter added the os-linux Linux OS (any supported distro) label Jan 28, 2025

steveharter commented Jan 28, 2025

View reviewed changes

src/libraries/Common/tests/System/DirectoryServices/LDAP.Configuration.xml Show resolved Hide resolved

Update test (manually verified on Linux)

51070ed

steveharter requested a review from ericstj January 28, 2025 21:58

steveharter marked this pull request as ready for review January 28, 2025 21:58

build-analysis bot mentioned this pull request Jan 29, 2025

SIGKILL (OOM?) while running LibraryImportGenerator.Tests w/o actionable log messages or artifacts dotnet/dnceng#2496

Open

3 tasks

ericstj reviewed Jan 29, 2025

View reviewed changes

buyaa-n reviewed Jan 29, 2025

View reviewed changes

Address feedback

5c64144

steveharter requested a review from ericstj February 3, 2025 21:34

buyaa-n reviewed Feb 4, 2025

View reviewed changes

src/libraries/System.DirectoryServices.Protocols/ref/System.DirectoryServices.Protocols.cs Outdated Show resolved Hide resolved

Also remove UnsupportedOSPlatformAttribute from ref file

1940a2b

steveharter mentioned this pull request Feb 4, 2025

LdapSessionOptions.ReferralChasing property setter doesn't work on all Linux platforms #112146

Open

Fix a couple test failures

7a11a43

steveharter mentioned this pull request Feb 5, 2025

OSX failure on StringTests.StartsWithNoMatch_StringComparison #112195

Open

ericstj reviewed Feb 5, 2025

View reviewed changes

ericstj approved these changes Feb 5, 2025

View reviewed changes

steveharter merged commit 53a3e33 into dotnet:main Feb 5, 2025
84 of 86 checks passed

steveharter deleted the FixIssue-104260 branch February 5, 2025 21:29

steveharter added a commit to steveharter/runtime that referenced this pull request Feb 13, 2025

Add support for LDAPTLS_CACERTDIR \ TrustedCertificateDirectory (dotn…

a3ac760

…et#111877)

steveharter added a commit to steveharter/runtime that referenced this pull request Feb 13, 2025

Add support for LDAPTLS_CACERTDIR \ TrustedCertificateDirectory (dotn…

69fa3a4

…et#111877)

This was referenced Feb 13, 2025

[release/8.0-staging] Add support for LDAPTLS_CACERTDIR \ TrustedCertificateDirectory #112530

Open

[release/9.0-staging] Add support for LDAPTLS_CACERTDIR \ TrustedCertificateDirectory #112531

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for LDAPTLS_CACERTDIR \ TrustedCertificateDirectory #111877

Add support for LDAPTLS_CACERTDIR \ TrustedCertificateDirectory #111877

steveharter commented Jan 27, 2025 •

edited

Loading

dotnet-issue-labeler bot commented Jan 27, 2025

dotnet-issue-labeler bot commented Jan 27, 2025

ericstj left a comment

buyaa-n Jan 29, 2025

steveharter Feb 3, 2025

buyaa-n Feb 4, 2025

steveharter Feb 4, 2025

ericstj Feb 5, 2025

ericstj Feb 5, 2025

steveharter Feb 5, 2025

buyaa-n Jan 29, 2025

steveharter Feb 3, 2025

ericstj Feb 5, 2025

steveharter Feb 5, 2025

ericstj Feb 5, 2025

steveharter Feb 5, 2025

ericstj left a comment

steveharter commented Feb 13, 2025

github-actions bot commented Feb 13, 2025

github-actions bot commented Feb 13, 2025

steveharter commented Feb 13, 2025

github-actions bot commented Feb 13, 2025

github-actions bot commented Feb 13, 2025

	public System.DirectoryServices.Protocols.VerifyServerCertificateCallback VerifyServerCertificate { get { throw null; } set { } }
	[System.Runtime.Versioning.SupportedOSPlatformAttribute("windows")]
	public System.DirectoryServices.Protocols.VerifyServerCertificateCallback VerifyServerCertificate { get { throw null; } set { } }

Add support for LDAPTLS_CACERTDIR \ TrustedCertificateDirectory #111877

Add support for LDAPTLS_CACERTDIR \ TrustedCertificateDirectory #111877

Conversation

steveharter commented Jan 27, 2025 • edited Loading

dotnet-issue-labeler bot commented Jan 27, 2025

dotnet-issue-labeler bot commented Jan 27, 2025

ericstj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericstj left a comment

Choose a reason for hiding this comment

steveharter commented Feb 13, 2025

github-actions bot commented Feb 13, 2025

github-actions bot commented Feb 13, 2025

steveharter commented Feb 13, 2025

github-actions bot commented Feb 13, 2025

github-actions bot commented Feb 13, 2025

steveharter commented Jan 27, 2025 •

edited

Loading