Add support for TLS and connectionless LDAP connections on Linux #52904

iinuwa · 2021-05-18T12:41:24Z

May fix #43890.

This PR does a few things for LDAP support in Linux for System.DirectoryServices.Protocols:

Replaces deprecated OpenLDAP methods ldap_init and ldap_simple_bind_s with ldap_create and ldap_sasl_bind_s, respectively.
Fixes a problem with setting the LDAP version on the connection by using IntPtr types rather than int directly.
Adds support for TLS and connectionless LDAP connections.

The first two points are prerequisites for the ultimate goal, which is to add LDAP TLS support to Linux. Let me know if you want me to break this up into multiple PRs.

Testing

I tested this using by running the osixia/openldap image with a TLS listener on 1636, and extracted the self-signed CA to a file:

docker run -p 1389:389 -p 1636:636 --name ldap -d --rm -e LDAP_TLS_VERIFY_CLIENT=never osixia/openldap --loglevel debug
docker exec ldap cat /container/service/slapd/assets/certs/ca.crt > /tmp/ca.crt

I set the LDAP client CA certificate path in /etc/ldap/ldap.conf so OpenLDAP would trust the self-signed certificate:

# /etc/ldap/ldap.conf
#...
TLS_CACERT /tmp/ca.crt

(I'm testing with OpenLDAP 2.10.12 on Ubuntu 20.04, in case paths for configs differ on different distributions.)
I also had to map the short container ID (e.g. 7fb28b4656af) to 127.0.0.1 in /etc/hosts.

Finally, I ran env LDAP_TEST_SERVER_INDEX=4 dotnet test to select the new test configuration I added in LdapConfiguration.xml using TLS, and change the hostname to the short container ID in /etc/hosts to make sure the client validated the server certificate. All tests passed successfully locally, and I could see the successful binds and operations in the container logs.

dnfadmin · 2021-05-18T12:41:38Z

All CLA requirements met.

src/libraries/Common/src/Interop/Interop.Ldap.cs

...ectoryServices.Protocols/src/System/DirectoryServices/Protocols/ldap/LdapConnection.Linux.cs

src/libraries/Common/src/Interop/Linux/OpenLdap/Interop.Ldap.cs

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

...ectoryServices.Protocols/src/System/DirectoryServices/Protocols/ldap/LdapConnection.Linux.cs

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

danmoseley · 2021-05-19T03:30:10Z

...ectoryServices.Protocols/src/System/DirectoryServices/Protocols/ldap/LdapConnection.Linux.cs

+                uris = $"{scheme}:{directoryIdentifier.PortNumber}";
+            }
+
+            return LdapPal.SetStringOption(_ldapHandle, LdapOption.LDAP_OPT_URI, uris);


Is it possible for directoryIdentifier.Servers to, through bad input, contains only servers that are empty strings? If so uris will be null but it appears openldap treats that as return to default. Not sure whether that's what you want, or you want to avoid calling it in that case.
https://git.openldap.org/openldap/openldap/-/blob/master/libraries/libldap/options.c#L655

Yes, it is possible. As you said, OpenLDAP falls back to its default hostname (which can be configured in /etc/ldap/ldap.conf) if the hostname is not set. I intentionally allowed that behavior here too since it would be expected if someone is familiar with OpenLDAP.

I think that's OK... though I don't know how the Windows side handles empty string hostnames. If someone wrote code on Linux, then tried to run on Windows, that could be an issue if Windows doesn't support default hostnames.

danmoseley · 2021-05-19T03:37:40Z

...ectoryServices.Protocols/src/System/DirectoryServices/Protocols/ldap/LdapConnection.Linux.cs

+            LdapDirectoryIdentifier directoryIdentifier = _directoryIdentifier as LdapDirectoryIdentifier;
+            if (directoryIdentifier.Connectionless)
+            {
+                scheme = "cldap://";


do we have any test for connectionless?

No, I wasn't sure how to test it. I think that since connectionless LDAP isn't standardized, it's only implemented in AD (or as far as I can tell, it's not implemented in slapd). I've only got access to one [production] instance of AD, so I didn't want to test there! :)

I imagine that this is currently broken in Linux since there are no provisions anywhere else to configure connectionless. I think that this patch should work for connectionless should work though.

Normally we don't add features we can't test 🙂. But if the feature is simply "given this flag, use this scheme prefix" maybe it's reasonable to add if testing isn't straightforward.

@joperezr preferences?

I agree that having a test would be ideally, but given the context here I think that if we can't come up with a good way to test this it may be reasonable.

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

danmoseley · 2021-05-19T03:50:37Z

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

        {
-            get => throw new PlatformNotSupportedException();
-            set => throw new PlatformNotSupportedException();
+            get => GetPtrValueHelper(LdapOption.LDAP_OPT_VERSION).ToInt32();


Fixes a problem with setting the LDAP version on the connection by using IntPtr types rather than int directly.

Could you say more?

In my testing, I had a problem where the version was not being set properly when using an integer rather than an IntPtr, so it was defaulting to LDAPv2 on my machine and failing to connect. The API calls for an pointer to an int:

LDAP_OPT_PROTOCOL_VERSION
Sets/gets the protocol version. outvalue and invalue must be
int *.

I think this might work now because the value is being cast to an integer pointer on the C side, but from what I understand (which is very little) I think that this can fail on systems with different pointer sizes?

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

danmoseley · 2021-05-19T03:52:35Z

Thanks for the contribution @iinuwa we have not had much time to give this library a lot of attention. It is good to see the improvement.

danmoseley · 2021-05-19T03:54:09Z

cc @blowdart who may be interested.

...ectoryServices.Protocols/src/System/DirectoryServices/Protocols/ldap/LdapConnection.Linux.cs

....DirectoryServices.Protocols/src/System/DirectoryServices/Protocols/Interop/LdapPal.Linux.cs

iinuwa · 2021-05-25T21:20:40Z

I think the last 4 CI checks timed out; do I need to do anything about these?

danmoseley · 2021-05-25T21:42:58Z

Let's try again and see whether it's fixed.

danmoseley · 2021-05-25T21:43:43Z

@buyaa-n @joperezr @krwq as area owners could one of you please review this community PR?

src/libraries/Common/src/Interop/Interop.Ldap.cs

joperezr · 2021-05-25T22:35:33Z

src/libraries/Common/src/Interop/Linux/OpenLdap/Interop.Ldap.cs

@@ -77,8 +79,8 @@ static Ldap()
        [DllImport(Libraries.OpenLdap, EntryPoint = "ldap_initialize", CharSet = CharSet.Ansi, SetLastError = true)]
        public static extern int ldap_initialize(out IntPtr ld, string hostname);

-        [DllImport(Libraries.OpenLdap, EntryPoint = "ldap_init", CharSet = CharSet.Ansi, SetLastError = true)]
-        public static extern IntPtr ldap_init(string hostName, int portNumber);
+        [DllImport(Libraries.OpenLdap, EntryPoint = "ldap_create", CharSet = CharSet.Ansi, SetLastError = true)]


Can you share the difference between ldap_init and ldap_create?

ldap_init and ldap_initialize both call ldap_create internally. ldap_create does the heavy lifting of allocating memory and instantiating global options and whatnot: ldap_init and ldap_initialize are just wrappers to easily specify the server using a hostname and port or an LDAP URI, respectively.

ldap_init has been deprecated by the OpenLDAP developers for a few reasons, one of which is the inability to specify the protocol scheme using ldap_init. Lbraries that are not compiled with #define LDAP_DEPRECATED 1 will not include ldap_init. The OpenLDAP developers recommend using either ldap_create or ldap_initialize to replace ldap_init.

FWIW, we use ldap_create instead of ldap_initialize when setting up an LdapConnection since we don't know the value of the SessionOptions.SecureSocketLayer property when we initialize the OpenLDAP handle.

Makes sense, thanks so much for the explanation. I agree with you that ldap_init is deprecated and we shouldn't be using it any longer. The docs from openldap seem to only reference ldap_initialize though, so I wonder if we should be using that instead of ldap_create. What are your thoughts?

I don't mind either way; it winds up doing the same thing.

What I was trying by using ldap_create over ldap_initialize to do was avoid setting the LDAP_OPT_URI option twice (once during initialization, and again after the SessionOptions is populated). But after reading through the code for ldap_initialize again I've discovered we can accomplish that by passing a null value as the URI. That would eliminate the need to call ldap_create. I can do that in the morning if that's your preference.

The docs from openldap seem to only reference ldap_initialize

(Reading through the mailing list, I think that the developers themselves admit that there's some stuff missing from the manpages. I don't know if ldap_create's absence in the manpage is intentional or not.)

I totally agree that the manpages are not complete, but I do feel a bit better on using the documented API over the undocumented one specially if there are no clear advantages over the other one. I’m not very familiar how they manage contributions, but most dev teams would be more hesitant to make a breaking change to documented APIs than to public but undocumented one.

(even when I don’t really think that any of the two will ever be changed, but might as well be more careful)

....DirectoryServices.Protocols/src/System/DirectoryServices/Protocols/Interop/LdapPal.Linux.cs

joperezr · 2021-05-25T22:46:04Z

Left some small comments but this looks great otherwise! Thanks so much for this great contribution @iinuwa I'm sure a lot of people will be happy with this 😄

joperezr · 2021-05-27T22:17:11Z

FWIW I ran the scenario tests against an ActiveDirectory server from a linux client VM and they all ran just fine and were able to connect 🥳

My only comment left is regarding the use of ldap_initialize over ldap_create but I think other than that this should be ready to go.

In OpenLDAP, ldap_simple_bind_s is deprecated in favor of ldap_sasl_bind_s with the LDAP_SASL_SIMPLE auth method[1][]. Similarly, ldap_init is deprecated in favor of ldap_initialize[2][]. The newer APIs also allows us to specify a URI to use TLS with OpenLDAP. [1]: https://git.openldap.org/openldap/openldap/-/blob/OPENLDAP_REL_ENG_2_4_58/include/ldap.h#L1278 [2]: https://git.openldap.org/openldap/openldap//blob/OPENLDAP_REL_ENG_2_4_58/include/ldap.h#L1513

This commit manually specifies the LDAP URI option during connect (but before binding). This is necessary because in order to know the correct scheme, we need access to SessionOptions, which is not available until after initialization. Finally, it removes the PlatformUnsupportedException from the SessionOptions.SecureSocketLayer property. This makes it possible to use LDAP over TLS and connectionless (UDP) LDAP.

iinuwa · 2021-06-01T15:39:44Z

Sorry for the delay. I made the change to use ldap_initialize instead of ldap_create and fixed some mixed up git history to make sure each commit actually builds. 😜

Windmiller75 · 2021-06-01T22:46:18Z

Hi, I'm having the problem with connecting to LDAP over SSL on Linux/Ubuntu and wondering if there's a fix for this today.
As far as I understand you´re working on it but I don´t understand how to fix it. Regards, Morgan

danmoseley · 2021-06-02T04:29:36Z

@Windmiller75 if this is merged fairly soon it will be released in .NET 6 Preview 6 in a month or so.

iinuwa · 2021-06-03T14:32:43Z

@joperezr I think this is ready to go.

Two tests timed out and the other (Library release test on Linux x64) seems to have crashed: I don't think that it's related to the code though, since other Library tests on Linux passed.

joperezr · 2021-06-03T17:31:08Z

Sorry for the delay. I made the change to use ldap_initialize instead of ldap_create and fixed some mixed up git history to make sure each commit actually builds.

Thanks for making the change! Just a note for future contributions, it's ok if not all commits in your PR build individually, and usually it is good to not rebase in the middle so it is easier to track non-reviewed code vs already reviewed one unless there is a good reason to rebase.

Thanks again for this great contribution, code looks good to me ,I have requeued the few CI runs that were failing due to unrelated issues and I'm locally double-checking against an Active Directory server to make sure all tests pass. After that I'll go ahead and merge 🎆

joperezr · 2021-06-03T17:47:30Z

Tests against real server all passed as expected. Will merge as soon as CI is green

ghost · 2021-06-03T17:48:02Z

Hello @joperezr!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

danmoseley · 2021-06-03T18:42:48Z

Thanks @iinuwa and @joperezr --- you might want to add this to the Preview 6 tracking issue so that it goes in the blog post.

danmoseley · 2021-06-04T18:54:41Z

@iinuwa do you have interest or plans to look at anything else in this area? There's plenty of ideas, some of them likely small:
https://github.com/dotnet/runtime/issues?q=is%3Aopen+is%3Aissue+label%3Aarea-System.DirectoryServices+sort%3Acomments-desc

iinuwa · 2021-06-05T15:32:24Z

@iinuwa do you have interest or plans to look at anything else in this area? There's plenty of ideas, some of them likely small:
https://github.com/dotnet/runtime/issues?q=is%3Aopen+is%3Aissue+label%3Aarea-System.DirectoryServices+sort%3Acomments-desc

@danmoseley Yeah, some of those look interesting. I'd look at the referrals and Kerberos issues on Linux, but I'd need an AD and KDC instance to test I think. (I'd have to do this on my free time, and I only have access to my personal Linux laptop, which doesn't have the resources/license to run a Windows VM.) Do you know of a free and easy way to get that set up?

danmoseley · 2021-06-07T13:06:39Z

@iinuwa I am not aware of free Windows Server images for testing. (http://my.visualstudio.com/downloads I assume is for people with VS licenses). Perhaps there is a way using a free Azure trial but that's likely more than you want to deal with.
If you are doing work that can essentially be tested on Linux and just needs a quick check on Windows (client and server) then perhaps we could do that test when you're done.

Set LDAP version with pointers on Linux

e5c9d1e

dotnet-issue-labeler bot added the area-System.DirectoryServices label May 18, 2021

iinuwa mentioned this pull request May 18, 2021

Add SSL support to LdapSessionOptions on Linux #43890

Closed

jkotas reviewed May 18, 2021

View reviewed changes

src/libraries/Common/src/Interop/Interop.Ldap.cs Outdated Show resolved Hide resolved

hangy reviewed May 18, 2021

View reviewed changes

...ectoryServices.Protocols/src/System/DirectoryServices/Protocols/ldap/LdapConnection.Linux.cs Outdated Show resolved Hide resolved

...ectoryServices.Protocols/src/System/DirectoryServices/Protocols/ldap/LdapConnection.Linux.cs Outdated Show resolved Hide resolved