Fix #41618 Correct marshalling of SortKey objects on Linux

[lscorcia]

The SortKey objects are marshalled to pointers and then passed to native functions. Under Windows, the native LDAP functions use Unicode strings, while under Linux they use ANSI - this breaks the use of sort controls under Linux. This PR ensures the correct StructLayout CharSet for each platform.

Fixes #41618 .

[ghost]

Tagging subscribers to this area: @dotnet/area-system-directoryservices, @jay98014
See info in area-owners.md if you want to be subscribed.

Issue Details

---

The SortKey objects are marshalled to pointers and then passed to native functions. Under Windows, the native LDAP functions use Unicode strings, while under Linux they use ANSI - this breaks the use of sort controls under Linux. This PR ensures the correct StructLayout CharSet for each platform.

Author:	lscorcia
Assignees:	-
Labels:	area-System.DirectoryServices, community-contribution
Milestone:	-

[dnfadmin]

All CLA requirements met.

[lscorcia]

Hello @joperezr , what do you think about this?

[joperezr]

Thanks a lot for the contribution @lscorcia, I'm very sorry that it took a bit for me to take a look at this.

[joperezr]

@lscorcia would it be possible to also add a functional test which performs a search and uses the sort control here: https://github.com/dotnet/runtime/blob/main/src/libraries/System.DirectoryServices.Protocols/tests/DirectoryServicesProtocolsTests.cs ? Those tests don't run in our builds, but we run them manually every now and then against a real LdapServer so it would be good to get protection against this to ensure it works as expected in all platforms.

[joperezr]

The changes LGTM, but I'm adding @bartonjs to take a look first because he is much more familiar with PInvoke marshalling in general, but also because SortKey is a public type and even though I believe that changing the StructLayout isn't a breaking change, I want to make sure we double check.

[joperezr]

I see, yeah that makes sense, I think it is fine to create an internal struct (that uses the appropriate charset depending on the platform) which is then used for PInvoking is better and we should leave SortKey as is, and just use it to copy the values into that internal struct. @lscorcia would you be able to adjust this PR to do what is suggested?

[lscorcia]

Thanks for the suggestions! @bartonjs notes make sense and I'd like to give them a shot, it may not look good but I should be able to do it. I also wrote the additional test you mentioned, but despite a few hours spent on it I still have not been able to run it. I always have matching issues between CoreCLR, the libraries and the test assembly. I will spend some more time on it tomorrow.

[lscorcia]

So, I spent way too much time trying to run the DirectoryServicesProtocolsTests and I have never been able to run them successfully under Linux. I am now wondering if it is a problem of my machine, or if they don't currently pass under Linux.

This is what I did:

sudo docker run -p 1389:1389 -e ROOT_USER_DN='cn=admin,dc=example,dc=com' -e BASE_DN='dc=example,dc=com' -e ROOT_PASSWORD=password  -d openidentityplatform/opendj

export LDAP_TEST_SERVER_INDEX=0
git clean -xdf
./build.sh clr+libs -rc Release
cd src/libraries/System.DirectoryServices.Protocols/tests
pushd ../src && dotnet build && popd && dotnet build /t:test /p:XUnitOptions="-class System.DirectoryServices.Protocols.Tests.DirectoryServicesProtocolsTests"

When compilation ends, all existing tests fail with the following error:

System.DirectoryServices.Protocols.Tests.DirectoryServicesProtocolsTests.TestAddAndModifyAttribute [FAIL]
  /home/luca/dotnet-runtime/src/libraries/System.DirectoryServices.Protocols/src/System/DirectoryServices/Protocols/ldap/LdapConnection.cs(1168,0): at System.DirectoryServices.Protocols.LdapConnection.BindHelper(NetworkCredential newCredential, Boolean needSetCredential)
  /home/luca/dotnet-runtime/src/libraries/System.DirectoryServices.Protocols/src/System/DirectoryServices/Protocols/ldap/LdapConnection.cs(1025,0): at System.DirectoryServices.Protocols.LdapConnection.Bind()
  /home/luca/dotnet-runtime/src/libraries/System.DirectoryServices.Protocols/tests/DirectoryServicesProtocolsTests.cs(685,0): at System.DirectoryServices.Protocols.Tests.DirectoryServicesProtocolsTests.GetConnection()
  /home/luca/dotnet-runtime/src/libraries/System.DirectoryServices.Protocols/tests/DirectoryServicesProtocolsTests.cs(124,0): at System.DirectoryServices.Protocols.Tests.DirectoryServicesProtocolsTests.TestAddAndModifyAttribute()
  System.DirectoryServices.Protocols.LdapException : An encoding error occurred.

I attached a debugger to the existing test, but at a glance nothing seems off. The error is triggered here, before any network interaction:

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

Line 124 in 58b8de9

return Interop.Ldap.ldap_sasl_bind(ld, who, Interop.LDAP_SASL_SIMPLE, passwordBerval, IntPtr.Zero, IntPtr.Zero, IntPtr.Zero);

I verified the parameters and they are correct, ld is not IntPtr.Zero, who contains the bind username and the password berVal is assigned the correct byte array. The mechanism is LDAP_SASL_SIMPLE (null) and the rest of the params are IntPtr.Zero.

I also tried rebasing my code on top of the current main as some things about marshalling have changed but the output is just the same. Can anyone confirm that those tests are actually passing as of today?

[joperezr]

@lscorcia you are right, I'm able to repro the issue. It also seems to repro with other setups as well, as I am seeing this happening with the slapd openldap server as well. Let's open an issue to get that fixed and it is fine if you don't add a functional test on this PR for now, we can add this later as part of the fix of that issue.

[joperezr]

I'm still testing, but is looking like what broke this was #64279 changes cc: @jkoritzinsky

[jkoritzinsky]

Yes, some of the marshalling changes in #64279 affect S.DS.Protocols. Runtime marshalling is disabled in that assembly, so defaults for non-generated P/Invokes are changed. However, I don't think there should be many changes that would affect this particular change in a way that isn't a clear MarshalDirectiveException.

[joperezr]

@lscorcia the fix for the encoding issue is now checked in. Could you please rebase your PR and try writing your test again? I have validated that the tests pass after following the steps you outlined above.

[lscorcia]

I added the new test which now passes, but I still have to introduce the non-public struct that was suggested above. Shouldn't take too long, will take a look at it tomorrow. Meanwhile, any idea about how to name it?

[lscorcia]

@joperezr I pushed the updated version containing the suggested changes. Test failures seem unrelated.

[lscorcia]

Rebased again to trigger a new test run since the previous one failed due to issues unrelated to this PR.

[lscorcia]

Test failures seem unrelated to this PR.

[joperezr]

I'm sorry that I haven't looked at this yet @lscorcia. I will look at this today.

[joperezr]

Left three minor things to fix, but other than that looks good. @bartonjs do you mind taking one last look?

[joperezr]

@lscorcia thank you so much for addressing all of the comments. One last thing, I'm trying to run your new test against one of the containers we have in our Ldap.Configuration.xml (the slapd with ssl one) and I'm getting an error:

[xUnit.net 00:00:01.12]     System.DirectoryServices.Protocols.Tests.DirectoryServicesProtocolsTests.TestSortedSearch [FAIL]
  Failed System.DirectoryServices.Protocols.Tests.DirectoryServicesProtocolsTests.TestSortedSearch [194 ms]
  Error Message:
   System.DirectoryServices.Protocols.DirectoryOperationException : The server does not support the control. The control is critical. critical extension is not recognized
  Stack Trace:
     at System.DirectoryServices.Protocols.LdapConnection.ConstructResponseAsync(Int32 messageId, LdapOperation operation, ResultAll resultType, TimeSpan requestTimeOut, Boolean exceptionOnTimeOut, Boolean sync) in /home/joepr25/runtime/src/libraries/System.DirectoryServices.Protocols/src/System/DirectoryServices/Protocols/ldap/LdapConnection.cs:line 1576
   at System.DirectoryServices.Protocols.LdapConnection.SendRequest(DirectoryRequest request, TimeSpan requestTimeout) in /home/joepr25/runtime/src/libraries/System.DirectoryServices.Protocols/src/System/DirectoryServices/Protocols/ldap/LdapConnection.cs:line 273
   at System.DirectoryServices.Protocols.LdapConnection.SendRequest(DirectoryRequest request) in /home/joepr25/runtime/src/libraries/System.DirectoryServices.Protocols/src/System/DirectoryServices/Protocols/ldap/LdapConnection.cs:line 216
   at System.DirectoryServices.Protocols.Tests.DirectoryServicesProtocolsTests.TestSortedSearch() in /home/joepr25/runtime/src/libraries/System.DirectoryServices.Protocols/tests/DirectoryServicesProtocolsTests.cs:line 571

Which server did you try your test against?

[joperezr]

Seems like OpenDJ server works just fine. It would be great if we can add a Skip Attribute to your test that ensures SortKey is supported on the target server by looking into the LdapConfiguration index that was set.

[lscorcia]

Yes, I tested it with OpenDJ. In order to use server side sorting with slapd one would have to enable the corresponding overlay (https://www.systutorials.com/docs/linux/man/5-slapo-sssvlv/), and even then in its default schema the uid attribute is not sortable I think. This is a bit overkill for a simple test, so it's probably better to just skip the test if the control is not supported.

What I plan to do: add a new element named SupportsServerSideSort to LDAP.Configuration.xml, expose it as a property in LdapConfiguration.cs and define a new IsServerSideSupported property to DirectoryServicesTests.cs. This in turn would be used to conditionally disable the test. Both Active Directory and OpenDJ should support it so I would set the new element to True for those two configs.

Is this plan ok for you?

[joperezr]

That sounds perfect. Yeah I wasn't suggesting that you fixed the slapd server so it supported sorting, the only thing I want is that if folks want to manually run the tests against a real server using our Ldap.Configuration.xml provided ones, that they won't see a test failure if they pick slapd one. I'm totally fine with them getting a test skipped if on slapd, and then have it running if they are on opendj

[joperezr]

Thank you so much for fixing this @lscorcia!

[joperezr]

Merging since failures are unrelated.

[lscorcia]

Thanks for your help and support! 🍻