Add an in-memory LDAP server to power S.DS.Protocols tests

[bartonjs]

The LDAP server is very simplified, and only has features that were already being tested when a manual server was specified. Further functionality can be added as it is needed to satisfy tests.

The server supports LDAP, LDAP+STARTTLS, and LDAPS.

[dotnet-policy-service]

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

[bartonjs]

The LDAP server was written by Copilot, and I didn't verify any of it against specs since it's only a test component. I did have opinions on things like "use enums, not magic literal ints", but otherwise let it do whatever it wanted.

The general flow in the DirectoryServicesProtocolsTests file is now

[ConditionalFact(ExternalServerSpecified)]
public void TestAThing()
{
    using (LdapConnection connection = ConnectExternal())
    {
        TestAThing(connection, s_externalConfig.BaseDN);
    }
}

[Fact]
public void TestAThing_Local()
{
    using (LdapTestServer testServer = MakeAServer())
    using (LdapConnection connection = ConnectLocally(testServer))
    {
        TestAThing(connection, testServer.BaseDN);
    }
}

private static void TestAThingCore(LdapConnection connection, string baseDN)
{
    // The body of the using from TestAThing before this change.
}

As such, I highly recommend viewing the diff in that file with whitespace ignored.

[Copilot]

Pull request overview

Adds a simplified, in-memory LDAP server to enable System.DirectoryServices.Protocols tests to run without requiring an externally configured LDAP instance, including support for LDAP, LDAP+STARTTLS, and LDAPS.

Changes:

• Introduces LdapTestServer with basic LDAP request handling and BER/ASN.1 encoding/decoding for the subset of operations used by current tests.
• Updates DirectoryServicesProtocolsTests to run key scenarios against either a configured external server or the new local in-memory server.
• Updates the test project file to compile the new server sources and adds required .NET Framework references for ASN.1 and certificate loading support.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File	Description
src/libraries/System.DirectoryServices.Protocols/tests/TestServer/LdapTestServer.cs	Adds in-memory directory state, listener lifecycle, and basic CRUD/search helpers backing the test LDAP server.
src/libraries/System.DirectoryServices.Protocols/tests/TestServer/LdapTestServer.Protocol.cs	Implements LDAP message framing + protocol operation parsing and response generation, including STARTTLS/LDAPS.
src/libraries/System.DirectoryServices.Protocols/tests/System.DirectoryServices.Protocols.Tests.csproj	Includes the new server sources and adds .NET Framework project references needed by the test server.
src/libraries/System.DirectoryServices.Protocols/tests/DirectoryServicesProtocolsTests.cs	Refactors existing tests into shared “Core” helpers and adds local-server variants plus certificate-callback tests for LDAPS/STARTTLS.

[Copilot]

ReadLdapMessageAsync treats a BER length byte of 0x80 (indefinite length) as numLenBytes == 0 and then proceeds with contentLength == 0, which will mis-frame the message and likely cause downstream ASN.1 parsing failures. Even if LDAP messages are expected to be definite-length, this should be rejected explicitly (e.g., throw/close when numLenBytes == 0) to avoid incorrect reads.

[Copilot]

The sort control parser reads reverseOrder using ReadOctetString(reverseTag) and then inspects the first byte. reverseOrder is an implicitly-tagged BOOLEAN; using the boolean reader (ReadBoolean(reverseTag)) would be correct and avoids relying on OCTET STRING decoding behavior for non-octet types.

Suggested change

	AsnReader boolReader = new AsnReader(sortKeyReader.ReadEncodedValue(), AsnEncodingRules.BER);
	byte[] boolBytes = boolReader.ReadOctetString(reverseTag);
	sortReverse = boolBytes.Length > 0 && boolBytes[0] != 0;
	sortReverse = sortKeyReader.ReadBoolean(reverseTag);

[Copilot]

HandleConnectionAsync only catches IO/cancellation/disposal exceptions. If ASN.1 decoding fails (e.g., malformed/unexpected client message causing AsnContentException) or TLS authentication throws, the exception will escape the fire-and-forget task and can surface as an unobserved task exception during test runs. Consider catching AsnContentException (or a broader Exception) and closing the connection cleanly.

[Copilot]

SslStream instances created for LDAPS/STARTTLS are assigned to stream but never disposed. Even though the underlying socket will be closed when TcpClient is disposed, not disposing SslStream can delay cleanup and may leave handshake/close-notify work unflushed. Consider wrapping the active stream in a using/await using (or ensuring the current SslStream is disposed when replaced/when the loop exits).

[bartonjs]

I expect this iteration to fail tests, by the way, since the LDAP+STARTTLS or LDAPS certificate acceptance callback is only wired up on Windows. I'll condition away those tests after seeing them fail in CI.

[tarekgh]

Thanks for the PR - the approach of an in-memory LDAP test server is the right call here. A few observations beyond what the copilot flagged:

[tarekgh]

Both AcceptLoopAsync() (line 39) and this HandleConnectionAsync() call are fire-and-forget (_ = ...). If either throws an unexpected exception (e.g., something not caught by the three catch clauses in HandleConnectionAsync), it becomes an unobserved Task exception that is silently swallowed. In a test context, this means a server-side bug could go unnoticed and a test could pass while the server is actually broken.

Consider storing these tasks (e.g., in a ConcurrentBag<Task>) and observing/awaiting them in Dispose(), so test failures surface clearly when the server had errors.

[tarekgh]

Dispose() cancels the token and stops the listener, but any in-flight HandleConnectionAsync tasks are orphaned - there is no tracking or awaiting of active connections. If a test disposes the server while a request is mid-flight, the TCP connection gets torn out from under the handler.

Consider tracking active handler tasks and awaiting them (with a short timeout) here, so disposal is clean and any late exceptions are observed.

[tarekgh]

Nit/theoretical: _certificate and _useLdaps are set here before Start() calls _listener.Start() + AcceptLoopAsync(). There is no explicit memory barrier between these writes and the reads in HandleConnectionAsync on the thread-pool. In practice the timing makes a race extremely unlikely, but initializing these in the constructor (or making them readonly and selecting the start mode via a parameter) would be structurally cleaner.

[tarekgh]

BaseDn is hardcoded to dc=test, but no corresponding root entry is ever seeded into _entries. This means a baseObject-scope search on the root DN itself will return zero results. If any test ever does that, it would silently get an empty result set rather than hitting the base entry. Worth either seeding a root entry in the constructor or adding a comment noting this limitation.

[tarekgh]

The filter evaluator only handles AND, OR, NOT, equalityMatch (tag 3), and present (tag 7). Any other filter type - substring (tag 4), greaterOrEqual (tag 5), lessOrEqual (tag 6), approxMatch (tag 8), extensibleMatch (tag 9) - silently returns false, which means matching entries are quietly excluded with no error.

This is fine for the current test set, but it is a subtle trap for anyone adding future tests. Consider either returning an UnwillingToPerform result for unrecognized filter tags, or adding a comment here documenting which filter types are intentionally unsupported.

[tarekgh]

The paging implementation uses a simple byte-offset cookie into the current result list. Since the entry store (_entries) can change between paged search requests (entries added or deleted by other test operations), offsets from a previous page can become stale - potentially skipping or duplicating entries.

This is fine for current tests, but worth a brief comment noting that this paging implementation assumes a stable dataset across pages.

[tarekgh]

The three-method pattern (TestX for external, TestX_Local for local, TestXCore for shared logic) is consistent and clear, but it triples the test surface area with identical wiring boilerplate. Across the file this adds significant churn.

Have you considered a data-driven approach (e.g., [MemberData] yielding connection factories, or a shared helper that runs the same core logic against both connection types)? That could cut the boilerplate substantially while preserving the dual-path coverage. Happy to discuss if you want to keep it explicit for readability - just raising it as an option.

[bartonjs]

I'd been thinking about it during the change, and think the best answer is probably to use inheritance, like we do in other libraries for getting coverage across array and span overloads.

So I'll see what

[ConditionalClass(typeof(DirectoryServicesTestHelpers), nameof(DirectoryServicesTestHelpers.IsWindowsOrLibLdapIsInstalled))]
public sealed class DirectoryServicesProtocolsTests_Local : DirectoryServicesProtocolsTests
{
    private class LocalConnectionState : ConnectionState
    {
        internal LdapTestServer TestServer { get; }
        
        internal LocalConnectionState(LdapConnection connection, LdapTestServer testServer)
            : base(connection)
        {
            TestServer = testServer;
        }

        protected override void Dispose(bool disposing)
        {
            base.Dispose(disposing);

            if (disposing)
            {
                TestServer.Dispose();
            }
        }
    }

    protected override ConnectionState OpenConnection()
    {
        LdapTestServer server = StartLocalServer(out int port);
        LdapConnection connection = GetLocalConnection(port);

        return new LocalConnectionState(connection, server);
    }
}

[ConditionalClass(typeof(DirectoryServicesProtocolsTests), nameof(LdapConfigurationExists))]
public sealed class DirectoryServicesProtocolsTests_External : DirectoryServicesProtocolsTests
{
    protected override ConnectionState OpenConnection()
    {
        return new ConnectionState(GetConnection());
    }
}


public abstract partial class DirectoryServicesProtocolsTests
{
    [Fact]
    public void TestInvalidFilter()
    {
        using (ConnectionState state = OpenConnection())
        {
            LdapConnection connection = state.Connection;
            // rest of code
        }
    }
}

ends up looking like

[tarekgh]

IMO your suggestion is a better design.

[danmoseley]

This doesn't replace the existing "infra" to run tests against real servers, right (mostly containerized ones)
https://github.com/dotnet/runtime/blob/main/src/libraries/Common/tests/System/DirectoryServices/LDAP.Configuration.xml

The big advantages of what you have here are we control it, we could potentially use it to mock error paths and it can run in CI. But the existing infra ensures we work against real servers.

[bartonjs]

This doesn't replace the existing "infra" to run tests against real servers, right

Correct. It's purely additive.