Mailing List Archive

On Thu, Apr 18, 2024 at 09:40:21AM +0100, Graham Leggett via dev wrote:
> Hi all,
>
> The attached patch is a current work in progress patch for httpd-trunk to use the new apr_ldap API that just landing in APR.
>
> The highlights:
>
> - Complete replacement of the previous API.
> - Will work against apr-2 (just landed) and apr-util-1.7 (after backport).
> - Linking to the underlying LDAP API has been removed and is no longer needed.

This design decision seems surprising to me, what does this add? Adding
another abstraction layer to allow runtime selection of the LDAP library
seems like a step backwards (a lot of complexity with no benefit).
Unlike with e.g. a database backend selection users don't care about
picking/configuring among many LDAP libraries.

Regards, Joe

On 18 Apr 2024, at 14:21, Joe Orton <jorton@redhat.com> wrote:

> This design decision seems surprising to me, what does this add? Adding
> another abstraction layer to allow runtime selection of the LDAP library
> seems like a step backwards (a lot of complexity with no benefit).
> Unlike with e.g. a database backend selection users don't care about
> picking/configuring among many LDAP libraries.

Can you read through the following and confirm whether your questions are answered?

https://github.com/apache/apr/blob/trunk/include/apr_ldap.h#L27

If any specific questions aren't answered, can you confirm so I can make these clear in the docs?

Regards,
Graham
--

On 4/18/24 3:21 PM, Joe Orton wrote:
> On Thu, Apr 18, 2024 at 09:40:21AM +0100, Graham Leggett via dev wrote:
>> Hi all,
>>
>> The attached patch is a current work in progress patch for httpd-trunk to use the new apr_ldap API that just landing in APR.
>>
>> The highlights:
>>
>> - Complete replacement of the previous API.
>> - Will work against apr-2 (just landed) and apr-util-1.7 (after backport).
>> - Linking to the underlying LDAP API has been removed and is no longer needed.
>
> This design decision seems surprising to me, what does this add? Adding
> another abstraction layer to allow runtime selection of the LDAP library
> seems like a step backwards (a lot of complexity with no benefit).
> Unlike with e.g. a database backend selection users don't care about
> picking/configuring among many LDAP libraries.

First of all I haven't gone through the patch. Hence I may be off below.

I think the core issue with the LDAP API we have in APR-UTIL 1.x today and that let to the
removal discussions on APR side in 2010 (https://lists.apache.org/thread/pz5c28g0gf248fvpds53zl1f8by9n8n4)
and 2011 (https://lists.apache.org/thread/cslw4gj0jgx3bmr8trz0jtfq5bmv1tfl) is that it is an incomplete
abstraction of the LDAP libraries. mod_authnz_ldap and mod_ldap as consumers of the current API
have LDAP library specific code and need to link directly against the specific LDAP library.
The point back then on APR side was that this incomplete API should be either "completed" or removed from APR
and that its code should move to httpd which should deal then directly with all the gory details of the various
LDAP libraries not just with a subset. This happened on APR trunk side, but not on httpd trunk side.

I agree that it is of limited sense to decide during runtime which LDAP library to use as typically there is
only one available on a particular system. This is substantially different from the situation with databases.
Nevertheless I think it is a good idea to concentrate the LDAP library abstraction in one place (either in httpd
or in APR) and don't continue the partial / mixed model we have today.
If we have further consumers of this API but httpd and APR is willing to have the code APR seems like to be the correct
place for it. If not we could do what we need just in httpd and decide during build time what library we want to build against.

Regards

Rüdiger

On 4/18/24 10:40 AM, Graham Leggett via dev wrote:
> Hi all,
>
> The attached patch is a current work in progress patch for httpd-trunk to use the new apr_ldap API that just landing in APR.
>
> The highlights:
>
> - Complete replacement of the previous API.
> - Will work against apr-2 (just landed) and apr-util-1.7 (after backport).
> - Linking to the underlying LDAP API has been removed and is no longer needed.
> - New LDAP API is based on pool lifetimes.
>
> Still to be done:
>
> - Rebind needs doing from scratch in APR, the old way blocked on some platforms.
> - Nested groups needs reimplementing, as we can now do parallel LDAP queries.

Would it be possible to provide this patch as a PR against trunk on Github? I think that would ease reviewing it.
I am quite aware that this moves some gory detail discussion of that patch away from the dev list here and over
to the PR where it is likely harder to find later. Nevertheless I think that the easier way to review the patch combined with
the tests applied to the PR via Github actions outweigh this drawback.

Regards

R?diger

On 19 Apr 2024, at 09:07, Ruediger Pluem <rpluem@apache.org> wrote:

> Would it be possible to provide this patch as a PR against trunk on Github? I think that would ease reviewing it.
> I am quite aware that this moves some gory detail discussion of that patch away from the dev list here and over
> to the PR where it is likely harder to find later. Nevertheless I think that the easier way to review the patch combined with
> the tests applied to the PR via Github actions outweigh this drawback.

Take a look here:

https://github.com/apache/httpd/pull/438

The key change is that the new apr-ldap API is 100% asynchronous, with helpful patterns to make it easy to use in synchronous code like present httpd. The intention is that future code will allow us to have an LDAP connection controlled by the MPM, with "guest" requests added by httpd requests and removed by pool cleanups. This is useful by anybody, not just httpd.

RFC1823 is 30-ish years old, we're not exposing this old API any more.

https://www.rfc-editor.org/rfc/rfc1823.html

Regards,
Graham
--

On Thu, Apr 18, 2024 at 3:21?PM Joe Orton <jorton@redhat.com> wrote:
>
> On Thu, Apr 18, 2024 at 09:40:21AM +0100, Graham Leggett via dev wrote:
> > Hi all,
> >
> > The attached patch is a current work in progress patch for httpd-trunk to use the new apr_ldap API that just landing in APR.
> >
> > The highlights:
> >
> > - Complete replacement of the previous API.
> > - Will work against apr-2 (just landed) and apr-util-1.7 (after backport).
> > - Linking to the underlying LDAP API has been removed and is no longer needed.
>
> This design decision seems surprising to me, what does this add? Adding
> another abstraction layer to allow runtime selection of the LDAP library
> seems like a step backwards (a lot of complexity with no benefit).
>
> Unlike with e.g. a database backend selection users don't care about
> picking/configuring among many LDAP libraries.

FWIW I tried some time ago to avoid the very same kind of complexity
with apr_crypto vs TLS/SSL library but it was -1'd.
I would prefer direct linking of those libraries too, databases are a
special case which we shouldn't generalize for every library used by
APR (IMHO).


Regards;
Yann.

On 19 Apr 2024, at 09:01, Ruediger Pluem <rpluem@apache.org> wrote:

> First of all I haven't gone through the patch. Hence I may be off below.
>
> I think the core issue with the LDAP API we have in APR-UTIL 1.x today and that let to the
> removal discussions on APR side in 2010 (https://lists.apache.org/thread/pz5c28g0gf248fvpds53zl1f8by9n8n4)
> and 2011 (https://lists.apache.org/thread/cslw4gj0jgx3bmr8trz0jtfq5bmv1tfl) is that it is an incomplete
> abstraction of the LDAP libraries. mod_authnz_ldap and mod_ldap as consumers of the current API
> have LDAP library specific code and need to link directly against the specific LDAP library.

What you've described above is the exact thing that has been fixed.

This is the core of the patch:

https://github.com/apache/httpd/pull/438/files#diff-f309c2fcd650f484a21c930ddde995a646387421b671232f5b643328f7d2da06

diff --git a/modules/aaa/config.m4 b/modules/aaa/config.m4
index 1b59f99f492..b12e03b287d 100644
--- a/modules/aaa/config.m4
+++ b/modules/aaa/config.m4
@@ -49,21 +49,7 @@ APACHE_MODULE(authz_core, core authorization provider vector module, , , yes)

dnl LDAP authentication module. This module has both the authn and authz
dnl modules in one, so as to share the LDAP server config directives.
-APACHE_MODULE(authnz_ldap, LDAP based authentication, , , most, [.
- APACHE_CHECK_APR_HAS_LDAP
- if test "$ac_cv_APR_HAS_LDAP" = "yes" ; then
- if test -z "$apu_config" ; then
- LDAP_LIBS="`$apr_config --ldap-libs`"
- else
- LDAP_LIBS="`$apu_config --ldap-libs`"
- fi
- APR_ADDTO(MOD_AUTHNZ_LDAP_LDADD, [$LDAP_LIBS])
- AC_SUBST(MOD_AUTHNZ_LDAP_LDADD)
- else
- AC_MSG_WARN([apr/apr-util is compiled without ldap support])
- enable_authnz_ldap=no
- fi
-])
+APACHE_MODULE(authnz_ldap, LDAP based authentication, , , most)

dnl FastCGI authorizer interface, supporting authn and authz.
APACHE_MODULE(authnz_fcgi,

LDAP is no longer special. DBD, DBM, SQL, LDAP, all the same.

> I agree that it is of limited sense to decide during runtime which LDAP library to use as typically there is
> only one available on a particular system. This is substantially different from the situation with databases.

This isn't the whole story.

Like databases, we have lots of different drivers with wildly different and incompatible implementations. Unlike databases, the majority of these drivers are based on a C API published as an RFC 30 years ago, and therefore are about 80% common code / clashing symbols. It is the way that it is. The driver specific code involves SSL (which evolved) and SASL binding (which evolved), but the vast majority of the code is still standard.

For this reason it's pointless coming up with five separate drivers for five separate libraries. You can never load two at the same time because clashing symbols. This is true of all code, it has nothing to do with httpd specifically. Someone binds to library A in a module, and then someone else binds to library B in a different module, you are not loading those two modules into the server at the same time, it's just not happening.

A further problem is that the libraries diverged.

Many of the functions in OpenLDAP and Windows have been deprecated, replaced with _ext functions. The LDAP_DEPRECATED define is now gone.

Rebinding for example works totally differently if you're on IBM, or on OpenLDAP. One does not block, but has limited SASL capability. The other does SASL for you, but blocks. Windows expects you to rebind yourself.

SASL is wild west. OpenLDAP does SASL for you, but you have to do an intricate dance between two functions to make it work. Windows says "here you go, have a binary blob, pass it to the Windows specific SASL implementation will you". Again, different approaches, it is what it is.

Non-blocking behaviour is inconsistent. Some functions in some libraries block in some circumstances. You've got to be a masochist if every single consumer of LDAP has to deal with all of these inconsistencies, over and over again.

The "P" in APR stands for portability, and that is exactly what I as a consumer of an LDAP API want.

> The point back then on APR side was that this incomplete API should be either "completed" or removed from APR
> and that its code should move to httpd which should deal then directly with all the gory details of the various
> LDAP libraries not just with a subset. This happened on APR trunk side, but not on httpd trunk side.

What happened in the past was that code was removed from APR without a replacement implementation having been provided first. The expectation was that other people (me) would provide free labour to replace that removed implementation to make the server whole again.

I have been working on this on and off for years, providing custom patched httpd's to get the immediate job done. It has reached the point where I can't be supporting custom patches and this needed finishing.

People have been very helpful reviewing the APR patches, and for that I am very grateful. You go round and round in circles for weeks hitting a problem, solving it, hitting the next problem, solving it. After a while you become blind to the code and fresh eyes are needed.

To vendors who expect me to go back to the drawing board and redo things differently for their benefit, pay me.

Regards,
Graham
--