Avoiding further (LDAP) stack proliferation in Samba

[Andrew Bartlett]

Thanks metze for your reply,

There is much we agree on, and other things that we have different
perspectives on here.  

My opening thought is that in this, I recognise that I need to be
careful what I wish for.  Broad scale lift-and-shift operations have
been popular in Samba of late, and this is another of them in a way.

At the start, they appear very simple, but as noted here there is much
complexity in the end result, and a very risky transition that needs to
be done by experienced developers and reviewed incredibly carefully.

Even if we didn't have my concerns about the API and implementation of
tldap, as you note in (3) below the "ldap ssl ads" parameter (which is
used enough that we have a WIP patch set to fix it for channel
bindings) and "client ldap sasl wrapping" parameter are not
implemented.

And I do have concerns about tldap.  My view since the inception, which
I think I expressed - I've not checked the archives - is that tldap
should:
 * Use the struct ldb_request, struct ldb_parse_tree, struct ldb_dn,
struct ldb_message and the related structures.  Not because they are
divinely inspired but because they enable a suite of utility functions
that will, as the library becomes mature, be duplicated (like dumping
to LDIF, parsing extended DNs, extracting a GUID etc).

I've re-checked the code more recently, and I'm sorry to see so much
inline ASN.1 parsing.  It really should re-use ldap_message.c for the
ASN.1 parsing, so the ASN.1 encode and decode can be tested (see the
recent tests added in test_ldap_message) and fuzzed (fuzz_ldap_decode).

Why do I raise this again?  Because as noted, lift and shift is risky. 
So if it is ever to be done, it should be done while tldap supports
only one small, optional winbind module.

That is why I appreciate but don't accept the "but this is not
something new" (i.e. it is already in use) argument.  This is is a
significant new use that makes it harder and much less likely that
tldap will ever be overhauled and suggests that tldap is already the
"future ideal" API and implementation.

Of course I now wish I had raised these objections again in 2015, but
we both know that would not have been a good idea. 

But here in 2020 I would be much less opposed to winbindd using tldap
directly if these matters were addressed, which is why I tried to leave
this as a possible way forward in my original mail.  It would mean that
tldap would just be a different API to a common LDAP client lib, not an
standalone creature itself. 

But whatever we do, it must not become an independent, fully featured,
LDAP client stack, it must complement and integrate with what we have. 

Swen and Christof,

All this does however really bring into question if for the nominal
purpose, there is some way to get the timeout handling we need out of
the OpenLDAP libraries under libads in the meantime.

I'm sorry this has become a can of worms.  That happens in Samba
sometimes.  Perhaps start by proposing just your tests, and someone can
look at them in isolation?

Andrew Bartlett

On Fri, 2020-05-22 at 15:02 +0200, Stefan Metzmacher wrote:

> As you know I like the idea of having things implemented just once!
> But as found out in the past this is a lot of work and
> replacing everything at once is often not possible.
> We learnt that we sometimes have to do small steps with intermediate
> states, which we sometimes not like, but at the same time require
> in order to make progress at all.
> 
> There're a lot of things I'd like to see:
> 1. The ldb api should not be used for pure LDAP users,
>    it's bad enough that the strange async model exists at all!
>    We should hope that it's only used for LDAP for command line
>    tools in a sync fashion.
> 2. source3/lib/tldap_gensec_bind.c should use gensec_update_send/recv
> 3. tldap makes use of the "client ldap sasl wrapping" and other
>    options, which are use by source4/libcli/ldap/ and
>    source3/libads/
> 4. We can add some helpers to get/pass 'struct ldb_message' from/to
>    tldap c.
> 5. users of source4/libcli/ldap/ should move to the tldap api
>    - lib/ldb-samba/ldb_ildap.c can become lib/ldb-samba/ldb_tldap.c
> 6. libads should go away it, at least it's low level internals
>    maybe it can be build on top of tldap as a first step in
>    order to avoid a rewrite of all non-winbind users.
> 7. winbindd should avoid libads and only use tldap.
> 
> But the goal of
> https://gitlab.com/samba-team/samba/-/merge_requests/1351
> is moving along with 7.
> 
> And I'm not seeing why we would require 4, 5, 6 before doing 7.
> They would be nice to have, but they tasks for another day.
> 
> BTW: I'm not saying that I'm happy with the current patchset of merge
> request 1351. But that's a different discussion.
> 
> metze
> 
-- 
Andrew Bartlett                       https://samba.org/~abartlet/
Authentication Developer, Samba Team  https://samba.org
Samba Developer, Catalyst IT          
https://catalyst.net.nz/services/samba