[API Proposal]: System.DirectoryServices.Protocols.LdapFilter

[RoadTrain]

Background and motivation

As per discussion at https://github.com/dotnet/runtime/issues/38609#issuecomment-897331400, it was suggested that runtime might benefit from LDAP query escape functionality.

As per RFC 4515, LDAP search filter arguments should be properly escaped. S.DS.P currently lacks such a functionality, which prevents some corner cases to work without consumer implementing such functionality themselves.

Detailed context (copied from https://github.com/dotnet/runtime/issues/38609#issuecomment-897331400):

Another case with escaping is when filter argument (e.g. group CN) contains parentheses, e.g. Some (very) important group. Since parentheses are a part of filter syntax itself, it might be problematic to correctly parse the filter.

(&(objectClass=group)(sAMAccountName=Some (very) important group))

The correct representation, as per RFC, is:

(&(objectClass=group)(sAMAccountName=Some \28very\29 important group))

What's interesting is that the first one works on Windows, but fails on Linux. The second one works on both Windows and Linux, as per my tests.

If that is the case I propose to add the escaping functionality to System.DirectoryServices.Protocols.LdapConnection, so both libraries act the same on the windows as well as on linux.

I think this might benefit from some general escape method which we can call manually when constructing filters, like:

var filter = $"(&(objectClass=group)(sAMAccountName={LdapFilter.Escape("Some (very) important group")}))";

Something along the lines of what I did here: https://github.com/dotnet/aspnetcore/pull/35279/commits/7f72499bb428bc1828d13e14367dc3c69c05b4d7 Is it a good idea to provide such a method as part of core System.DirectoryServices.Protocols functionality?

Originally posted by @RoadTrain in https://github.com/dotnet/runtime/issues/38609#issuecomment-897331400

API Proposal

namespace System.DirectoryServices.Protocols
{
    public static class LdapFilter
    {
        public static string EscapeFilterArgument(string filterArgument);
    }
}

API Usage

// Construct the LDAP filter by correctly escaping filter arguments
var filter = $"(&(objectClass=group)(sAMAccountName={LdapFilter.EscapeFilterArgument("Some (very) important group")}))";
Console.WriteLine(filter);
// Output:
// (&(objectClass=group)(sAMAccountName=Some \28very\29 important group))

Risks

None that I'm aware of.

[ghost]

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

Issue Details

---

### Background and motivation As per discussion at https://github.com/dotnet/runtime/issues/38609#issuecomment-897331400, it was suggested that runtime might benefit from LDAP query escape functionality. As per [RFC 4515](https://tools.ietf.org/search/rfc4515#section-3), LDAP search filter arguments should be properly escaped. S.DS.P currently lacks such a functionality, which prevents some corner cases to work without consumer implementing such functionality themselves. ### Detailed context (copied from linked issue): Another case with escaping is when filter argument (e.g. group CN) contains parentheses, e.g. `Some (very) important group`. Since parentheses are a part of filter syntax itself, it might be problematic to correctly parse the filter. ``` (&(objectClass=group)(sAMAccountName=Some (very) important group)) ``` The correct representation, [as per RFC](https://tools.ietf.org/search/rfc4515#section-3), is: ``` (&(objectClass=group)(sAMAccountName=Some \28very\29 important group)) ``` What's interesting is that the first one works on Windows, but fails on Linux. The second one works on both Windows and Linux, as per my tests. > If that is the case I propose to add the escaping functionality to System.DirectoryServices.Protocols.LdapConnection, so both libraries act the same on the windows as well as on linux. I think this might benefit from some general escape method which we can call manually when constructing filters, like: ``` var filter = $"(&(objectClass=group)(sAMAccountName={LdapFilter.Escape("Some (very) important group")}))"; ``` Something along the lines of what I did here: https://github.com/dotnet/aspnetcore/pull/35279/commits/7f72499bb428bc1828d13e14367dc3c69c05b4d7 Is it a good idea to provide such a method as part of core `System.DirectoryServices.Protocols` functionality? _Originally posted by @RoadTrain in https://github.com/dotnet/runtime/issues/38609#issuecomment-897331400_ ### API Proposal ```C# namespace System.DirectoryServices.Protocols { public static class LdapFilter { public static string EscapeFilterArgument(string filterArgument); } } ``` ### API Usage ```C# // Construct the LDAP filter by correctly escaping filter arguments var filter = $"(&(objectClass=group)(sAMAccountName={LdapFilter.EscapeFilterArgument("Some (very) important group")}))"; Console.WriteLine(filter); // Output: // (&(objectClass=group)(sAMAccountName=Some \28very\29 important group)) ``` ### Risks None that I'm aware of.

Author:	RoadTrain
Assignees:	-
Labels:	`api-suggestion`, `area-System.DirectoryServices`, `untriaged`
Milestone:	-

[RoadTrain]

One thing to consider: the escaping method should only do one thing -- replace reserved characters with their RFC-compliant form. So the user of that method should be aware if the string is already escaped, because double-escaping would result in invalid argument value, i.e. the method cannot trivially be made idempotent.

[joperezr]

This sounds like a great feature, thanks for the proposal. Can you confirm a list of all characters that would need to be escaped for this? Also, we would need to make sure that the RFC-compliant form works for both wldap32 and libldap (openldap) which are the native libraries that we use underneath.

[RoadTrain]

This sounds like a great feature, thanks for the proposal. Can you confirm a list of all characters that would need to be escaped for this?

The RFC lists 5 characters that must be escaped:

The rule ensures that the entire filter string is a valid UTF-8 string and provides that the octets that represent the ASCII characters "*" (ASCII 0x2a), "(" (ASCII 0x28), ")" (ASCII 0x29), "\" (ASCII 0x5c), and NUL (ASCII 0x00) are represented as a backslash "\" (ASCII 0x5c) followed by the two hexadecimal digits representing the value of the encoded octet.

From Richard L. Mueller:

Also, we would need to make sure that the RFC-compliant form works for both wldap32 and libldap (openldap) which are the native libraries that we use underneath.

That would probably require some custom AD setup. In our case I tested probably the most common occurrence -- escaping parentheses. It worked on both Linux (openldap) and Windows (wldap32).

There's a note on this at Technet. An interesting detail is:

Actually, the parentheses only need to be escaped if they are unmatched, as above. If instead the Common Name were "James (Jim) Smith", nothing would need to be escaped. However, any characters, including non-display characters, can be escaped in a similar manner in an LDAP filter.

I believe it's an implementation detail of wldap32, since openldap still needs matched parentheses escaped.

[joperezr]

Great, I guess last thing to consider is whether we want to have this type public, or instead simply make it part of the LdapConnection request implementation detail, where we automatically escape all filters that get sent to us. what are your thoughts around that @RoadTrain? Main thinking behind that being that this way more folks would benefit from the escaping, as opposed to just folks who know about the new API.

[RoadTrain]

@joperezr

Great, I guess last thing to consider is whether we want to have this type public, or instead simply make it part of the LdapConnection request implementation detail, where we automatically escape all filters that get sent to us. what are your thoughts around that @RoadTrain? Main thinking behind that being that this way more folks would benefit from the escaping, as opposed to just folks who know about the new API.

That would be ideal, but the problem is that it would require parsing the entire query, which adds a whole bunch of issues and risks to the generalizability of implementation. For example, for a query like

(&(objectClass=group)(sAMAccountName=John Smith (work)))

we would need to determine where the filter argument (John Smith (work)) starts and ends, then escape only that part. If the argument is John Smith work) instead, we would need to account for unmatched parentheses.

I think this problem is better illustrated by this example: https://stackoverflow.com/a/62193991 Another reply there actually shows an approach that is equivalent to my proposal (except it's in Java :D ): https://stackoverflow.com/a/67284683 . The API is similar: https://nightlies.apache.org/directory/api/1.0.2/apidocs/org/apache/directory/api/ldap/model/filter/FilterEncoder.html#encodeFilterValue-java.lang.String-

I personally don't know how many people use LdapConnection directly, my own use case is actually hidden behind ASP.NET Core APIs (i.e. RBAC authorization on Linux). So presumably, if such a functionality is implemented and used inside ASP.NET Core, at least some customers would benefit indirectly.

[RoadTrain]

Another approach might be to implement some form of an LDAP query builder. Yet another one is parametrized queries. E.g.

var filter = "(&(objectClass=group)(sAMAccountName={0}))";
var searchRequest = new SearchRequest(distinguishedName, filter, new [] {accountName}, SearchScope.Subtree, null);

That would require an additional parameter in the constructor of SearchRequest.

Maybe the best way is to do both via statics:

namespace System.DirectoryServices.Protocols
{
    public static class LdapFilter
    {
        public static string EscapeFilterArgument(string filterArgument);
        public static string Format(string filter, string[] args);
    }
}

[joperezr]

You have a good point regarding it being better to scope it down to a helper method for now, specially since that way it can be used for something else and not just SearchRequest. I think this API is ready for review so I’ll mark it as such.

[joperezr]

One last question here, I was looking for precedence in the framework and found a similar pattern in Regex, which has the static API Regex.Escape for escaping special characters on a regex pattern directly onto the regex class. The question being, would it make sense to add these APIs directly into LdapConnection? That could help a lot with the visibility and discoverability of the API.

[RoadTrain]

One last question here, I was looking for precedence in the framework and found a similar pattern in Regex, which has the static API Regex.Escape for escaping special characters on a regex pattern directly onto the regex class. The question being, would it make sense to add these APIs directly into LdapConnection? That could help a lot with the visibility and discoverability of the API.

Yeah, I think it's a valid approach. One thing to consider is possibly breaking SRP in case of LdapConnection. It feels like Regex is a bit more suited for escape functionality than LdapConnection. So they question really is whether LdapConnection is a good place for this method. I also can probably envision a situation where we might need other escape methods, e.g. for escaping Distinguished Names. I think it's something for the runtime team to decide :) I personally have no strong preferences, but I'm leaning towards a separate static class. Either way will be good, so it's up to you.

[terrajobst]

Video

• Do we need a new type? It seems more sensible to put it on an existing type.
  • SearchRequest
  • LdapConnection / DirectoryConnection
• Is this specific to filters? If not, should we remove the term "Filter" from the method?
• Do we need more a complex concept, such as builder-based approach?

namespace System.DirectoryServices.Protocols
{
    public static class LdapFilter
    {
        public static string EscapeFilterArgument(string filterArgument);
    }
}