derefAliases doesn't seem to do anything

[matthiasvegh]

As per https://github.com/twisted/ldaptor/blob/42ce8595409a34c97d04eba5598909a3b825bdb8/ldaptor/interfaces.py#L240, I'd expect if derefAliases is set to 3 (LDAP_DEREF_derefAlways) then the alias should be resolved instead of returning the alias.

Visual inspection of search() https://github.com/twisted/ldaptor/blob/42ce8595409a34c97d04eba5598909a3b825bdb8/ldaptor/entryhelpers.py#L238 seems to suggest that the derefAliases is not actually used

I've installed ldaptor from pip with pip install git+https://github.com/twisted/ldaptor.git@42ce8595409a34c97d04eba5598909a3b825bdb8

I'm using ldaptor as an ldap server, based on https://github.com/twisted/ldaptor/blob/42ce8595409a34c97d04eba5598909a3b825bdb8/ldaptor/protocols/ldap/ldapserver.py

Is this a bug? I probably could work around the issue by patching _onSearchGotBase and doing the dereferencing there, do you have any recommendations?

[cwaldbieser]

I believe you are correct. I do not see how the local variable derefAliases is ever used.

The SearchByTreeWalkingMixin.search() in entryhelpers.py creates iterator to walk the directory subtree specified by the search (base, onelevel, or subtree). It collects the matching nodes.

Dereferencing aliases seems like it would have to happen as you retrieved each result. I'd say you'd need to wrap the local matchCallback function so that each result was dereferenced.

RFC4511 [1] states that you need to check for dereference looping. RFC4512 [2] does say that "An alias entry shall have no subordinates, so that an alias entry is always a leaf entry.", so that might help simplify tracking aliases.

[1] https://tools.ietf.org/html/rfc4511#section-4.5.1 [2] https://tools.ietf.org/html/rfc4512#section-2.6

[matthiasvegh]

Not wanting to patch ldaptor (although that would probably be a better approach), I patched ldapserver.py's _onSearchGotBase callback

This is what I came up with:

def _onSearchGotBase(...):
    def _sendEntry(...):
        ...
    def _handleAlias(entry, aliasedObjectNames):
            aliasedObjectName, = aliasedObjectNames
            root = interfaces.IConnectedLDAPEntry(self.factory)
            deferred = root.lookup(aliasedObjectName)

            def _handleAliasResolve(newBase):
                deferred = newBase.search(filterObject=request.filter,
                                          attributes=request.attributes,
                                          scope=request.scope,
                                          derefAliases=request.derefAliases,
                                          sizeLimit=request.sizeLimit,
                                          timeLimit=request.timeLimit,
                                          typesOnly=request.typesOnly)
                deferred.addCallback(_handleEntries)
                return deferred

            deferred.addCallback(_handleAliasResolve)
            return deferred

    def _handleEntries(entries):
        pendingAliasResolutions = []
        for entry in entries:
            aliasedObjectName = entry.get('aliasedObjectName')
            if (aliasedObjectName is None or request.derefAliases ==
                    pureldap.LDAP_DEREF_neverDerefAliases):
                _sendEntryToClient(entry)
            else:
                deferred = _handleAlias(entry, aliasedObjectName)
                pendingAliasResolutions.append(deferred)
        if pendingAliasResolutions:
            return twisted.internet.defer.DeferredList(
                pendingAliasResolutions)
    deferred = base.search(...)
    deferred.addCallback(_handleEntries)
    # ... implementation unchanged from here

This seems to work, but has the problem of copying the .search(...) call. It also is entirely recursive, so aliases resolving to aliases should also work, but does not prevent infinite loops in the case of alias looping

In truth, I would expect this sort of logic to be a part of IConnectedLDAPEntry.search instead of at _onSearchGotBase, would you be interested in a pull request?

[cwaldbieser]

I'm considering it. A couple things:

1. Checking for aliases at this level would mean that you wouldn't need to implement the checking for each back end (e.g. in-memory and file system back ends). That is great!

2. The methods you are referencing don't seem to match what I'm seeing in master. It might be better if you submit a PR so I can view the diffs. Also, if we want to merge, a PR will make it simple.

[matthiasvegh]

Regarding your second point I thought I'd show you the diff before making the PR: https://github.com/twisted/ldaptor/compare/master...matthiasvegh:alias

As for your first: I'm not sure this sort of code is specific to ldap lookups, afterall, alias resolution is relevant in other operations like modify and add. In any event, there is still the case of alias loops to be considered, and precise semantics of the request.derefAliases (currently I'm assuming an all-or-nothing approach).

[cwaldbieser]

So after thinking a bit, there is something about this design that bothers me. In order to dereference aliases, the original call to search needs to not use the callback parameter. This means that search results need to all be gathered together on the server instead of being sent to the client as they are discovered.

This isn't only a technical limitation of the current interface. RFC4511 section 4.5.1.3 [1] states "Servers SHOULD eliminate duplicate entries that arise due to alias dereferencing while searching." That means you need to at least track all the result DNs and not return a single DN to the client more than once. I know that your code doesn't do this, but that is not what concerns me.

My concern is that without the change, the results are sent to the client as they are received, but with the change, the results are all collected on the server first, regardless of whether or not there are any aliased objects or whether the client has expressed a desire to dereference aliased objects. For small directories or result sets, this is probably not an issue. For a large result set, this could have a negative impact on the server.

I'm looking deeper into the code to see if my concerns have any real basis in fact. Sorry it is taking so long.

[1] https://tools.ietf.org/html/rfc4511#section-4.5.1

[cwaldbieser]

So after poking around in the code a bit, I discovered that the current directory implementations in ldaptor have some issues of their own[1]. In short, my concerns are valid, though probably not for ldiftree or inmemory which seem to return all their results all at once, anyway.

I don't think I want to merge without addressing these issues (though maybe Bret or others will weigh in with their opinions):

1. If the client doesn't request alias dereferencing, the entire list of results shouldn't be collected on the server.
2. If alias dereferencing produces a loop, that ought to be handled somehow.
3. If alias dereferencing produces a duplicate, that entry should only be included only once in the results.

A recursive solution also makes me wonder about hitting a recursion limit. I have a nagging feeling we should be looking for an iterative solution.

[1] https://github.com/twisted/ldaptor/issues/62

[matthiasvegh]

Don't worry about it taking too long, alias resolution is no small feature. I agree that my code should not be merged in its current state, for reasons you mentioned, but also because search seems to have this parameter that doesn't do anything. If alias resolution is to be implemented, it should be done at the point where it can be requested, not a layer above it.

If there are no loops, a recursive solution will only go as deep as the aliases take it, so carrying a list of visited dns and bailing out with loopDetected if a dn is visited twice seems reasonable. With that said, a time/size limit may be requested by the client, and should have a server imposed maximum, so that should put a stop to the search if it goes out of control, even if it is recursive.

With that said, ldap.com seems to suggest that servers may return partial results even if a limit was exceeded, so storing everything server side whilst alias resolution is ongoing may not be ideal if clients are interested in partial results.

Perhaps the following behavior would solve both issues: when a result is found and it turns out not be an alias, we should register it as being sent, send it (thereby easing the load on the server and giving the client something to play with in the meantime), and continue with the other results. If the result is an alias, then we check if we have exceeded any size/time/recursion/iteration limit or whether we have visited this node before. If not, resolution continues. If at any point we attempt to send a result we already have, then that could be ignored. I'm not entirely familiar with the innards of ldaptor, but perhaps reply could be a good place to keep track of what results we have sent out. When the ldap request is finished, this register could be cleared.