Avoiding further (LDAP) stack proliferation in Samba

[Stefan Metzmacher]

Hi Andrew,

Am 25.05.20 um 11:44 schrieb 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.

Yes, these are required for a wider usage of tldap.

> 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).

Here I disagree, the low level LDAP client API really doesn't need these!

There can be helpers to construct these on top. In order to
implement/convert ldb_ildap to tldap. Or let other code make use
of the existing and useful utility functions.

> 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).

I think we can find ways to have just just one code path for the ASN.1
part. I'm sure it's possible to refactor or maybe split
ldap_message.c into client and server parts and have the result still
fuzzed. In my view it's just a detail on how we'll do that in the end,
but the key is to have only one code path for encoding/decoding.

> 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. 

I'd say that it should become such a independent, fully featured,
LDAP client stack!

But it should be the only one used for AD related
access and everything else should be build on top.

So that it's not a 'separate independent' stack,
which I hope is your main concern here.

I hope we can agree and these key points to have:

- We should minimize the code paths to encode/decode LDAP pdu's.

- AD related client code should only use the tldap API,
  which does not have a API dependencies to LDB nor OpenLDAP.
  Currently we have ldb_ildap, libads, and tldap.
  We should rewrite ldb_ildap and libads to use tldap.

- We have also smbldap, but it's not used for AD related things,
  only for pdb_ldap and related things like idmap_ldap, net_sam.c.
  It would be possible to base smbldap or its consumers on tldap,
  but the danger to introduce regressions seems to high
  and we better leave it untouched.

In order to get there I'd propose the following steps:

1. Add missing features to tldap, e.g. tls support and
   similar things like using "client ldap sasl wrapping"
   and other options, which are use by source4/libcli/ldap/ and
   source3/libads/ and using gensec_update_send/recv

2. Refactor/consolidate the ASN.1 handling of ldap_message.c
   and tldap.c

3. Avoid using source4/libcli/ldap/ in torture tests.
   Most tests already use the ldb API for ldap tests,
   we can keep those untouched. The tests in
   source4/torture/ldap/{basic,common}.c can be removed
   or replaced using tldap. We need to test LDAPCompare,
   LDAPAbandon and LDAPUnbind.

4. Rewrite ldb_ildap.c to use the tldap api.

5. libads uses LDAPMessage, but that's an opaque structure
   We could use the following steps to remove it from
   the libads API:
   - define ADSMEssage LDAPMessage and convert all callers
     to ADSMessage.
   - Use 'struct ADSMessage { LDAPMessage *msg; }; and
     make sure everything build and works.
   - Then we can convert libads/ldap.c to tldap.

Note the order is not strict. It may turns out that it's
easier to do 3. + 4. before 2.

I hope this strategy would resolve the deadlock we currently have.

Can you agree on that plan?

If so we can start working on this without running into a deadlock again.

Thanks!
metze

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20200624/b0dd6fde/signature.sig>