Avoiding further (LDAP) stack proliferation in Samba

[Jeremy Allison]

On Fri, May 22, 2020 at 03:02:29PM +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.

OK, trying to help square this circle by moving the technical
discussion along a bit :-).

Looks to me like the core of the technical dispute is that
source3/lib/tldap.c is duplicating some of the existing
data structures and marshalling/unmarshalling code that
is already being tested with the fuzzing code that already
exists inside ldb.

Here's a good example:

In tldap:

struct tldap_control {
        const char *oid;
        DATA_BLOB value;
        bool critical;
};

marshalled with tldap_push_controls().

In ldb:

struct ldb_control {
        const char *oid;
        int critical;
        void *data;
};

marshalled with ldap_encode_control().

Both of them are doing the same thing (admittedly it's
better to have a ranged pointer via a DATA_BLOB than
an unbounded pointer void *), but call into different
encoding/decoding libraries which are both based on top
of our struct asn1_data library.

We really should be using common marshalling/unmarshalling
calls for things like this so they can be fuzzed and
tested once.

This is (4) part of Metze's comment above.

I agree that (7) is orthoganal to merging the ldap
client libraries, but not quite. The more code that
gets added to winbindd using the tldap structures and
client definitions, the harder the move to common
marshalling will become.

Is there someone on the Team with an iron in this
fire who has the time and motivation to reduce this
duplication so that eventually moving to common
client libraries is less pain ? It's not going to
be able to be me I'm afraid as I have other commitments,
so feel free to tell me to bugger off :-).

Just guessing that would go a long way towards
reducing the conflict in this area. Swen, Christof,
does that seem reasonable to you ?

Jeremy.