LDAP module requirements?

9 years ago by come.bernigaud@opensides.be — view source

unread

Hello,

The code of the LDAP module have some ifdefs like LDAP_API_FEATURE_X_OPENLDAP to test if we are building against openldap or something else.
Is there an exaustive list of the LDAP libs PHP is supposed to be buildable against?

The following page suggest it should always be openldap, is that the case?
http://php.net/manual/en/ldap.requirements.php
(if so, could ifdefs of LDAP_API_FEATURE_X_OPENLDAP be safely removed?)

What is the oldest openldap version supposed to be supported by each PHP version?

Côme

9 years ago by Kalle Sommer Nielsen — view source

unread

Hi Côme

2015-06-18 14:40 GMT+02:00 Côme BERNIGAUD come.bernigaud@opensides.be:

What is the oldest openldap version supposed to be supported by each PHP
version?

Seeing as you are one of the only people that I can re-call in recent
time that are working on ext/ldap I guess that choice would be up to
you. I have CC'ed the other listed extension maintainers of ext/ldap
(from EXTENSIONS in the php-src root).

Let me know if you wish to be added to the list in case you do not
have karma yourself.

--
regards,

Kalle Sommer Nielsen
kalle@php.net

9 years ago by come.bernigaud@opensides.be — view source

unread

(from EXTENSIONS in the php-src root).

Let me know if you wish to be added to the list in case you do not
have karma yourself.

Yes please, I’m pretty I don’t have the karma to do so.

Côme

9 years ago by Kalle Sommer Nielsen — view source

unread

Hi Côme

2015-06-18 16:25 GMT+02:00 Côme BERNIGAUD come.bernigaud@opensides.be:

Yes please, I’m pretty I don’t have the karma to do so.

Done: http://git.php.net/?p=php-src.git;a=commitdiff;h=d9d1948396285583a915018fe9310fad998876ca

--
regards,

Kalle Sommer Nielsen
kalle@php.net

9 years ago by come.bernigaud@opensides.be — view source

unread

Seeing as you are one of the only people that I can re-call in recent
time that are working on ext/ldap I guess that choice would be up to
you. I have CC'ed the other listed extension maintainers of ext/ldap
(from EXTENSIONS in the php-src root).

In that case I think I’ll go with openldap >= 2.4 required, as I intend to work on PHP 5.6 and openldap 2.4 was out in 2007, people using PHP >= 5.6 should have an openldap >= 2.4.
This will allow us to avoid all these ifdefs.

If anyone think this is wrong please speak up.

Is there a place where such requirements are supposed to be listed?

Côme

9 years ago by Andreas Heigl — view source

unread

Hi Côme

Am 22.06.15 um 11:36 schrieb Côme BERNIGAUD:

Seeing as you are one of the only people that I can re-call in recent
time that are working on ext/ldap I guess that choice would be up to
you. I have CC'ed the other listed extension maintainers of ext/ldap
(from EXTENSIONS in the php-src root).

In that case I think I’ll go with openldap >= 2.4 required, as I intend
to work on PHP 5.6 and openldap 2.4 was out in 2007, people using PHP >=
5.6 should have an openldap >= 2.4.

As 5.5 went into security-updates-only recently and the 2.4 is supported
by most of the distros out there (only checked debian, but they are
AFAIK the slowest in adapting) that should be fine.

This will allow us to avoid all these ifdefs.

If anyone think this is wrong please speak up.

Is there a place where such requirements are supposed to be listed?

I'd expect an appropriate information at least at
http://php.net/manual/en/ldap.requirements.php

I can do that if you'd like ;)

Cheers

Andreas

                                                          ,,,
                                                         (o o)

+---------------------------------------------------------ooO-(_)-Ooo-+
| Andreas Heigl |
| mailto:andreas@heigl.org N 50°22'59.5" E 08°23'58" |
| http://andreas.heigl.org http://hei.gl/wiFKy7 |
+---------------------------------------------------------------------+
| http://hei.gl/root-ca |
+---------------------------------------------------------------------+

9 years ago by Kalle Sommer Nielsen — view source

unread

Hi Côme

2015-06-22 11:36 GMT+02:00 Côme BERNIGAUD come.bernigaud@opensides.be:

Is there a place where such requirements are supposed to be listed?

Make sure to note that in any related files that should be in ext/ldap
(some extensions supply a document for such information), the
documentation (phpdoc in svn) and NEWS (something along the lines of
"The LDAP extension now requires openldap 2.4+").

You might want to consult Frenc (5.6 RM for extensive new features to
the 5.6 branch) (cc'd).

--
regards,

Kalle Sommer Nielsen
kalle@php.net

9 years ago by Rainer Jung — view source

unread

Hi,

Am 18.06.2015 um 14:40 schrieb Côme BERNIGAUD:

Hello,

The code of the LDAP module have some ifdefs like
LDAP_API_FEATURE_X_OPENLDAP to test if we are building against openldap
or something else.
Is there an exaustive list of the LDAP libs PHP is supposed to be
buildable against?

The following page suggest it should always be openldap, is that the case?
http://php.net/manual/en/ldap.requirements.php
(if so, could ifdefs of LDAP_API_FEATURE_X_OPENLDAP be safely removed?)

What is the oldest openldap version supposed to be supported by each PHP
version?

being late to this discussion: I do ship PHP with ldap extension on
Solaris using the Solaris ldap client libs. This worked well until and
including 5.6.11. It is broken starting with 5.6.12, because now
OpenLDAP is assumed and all or some compatibility ifdefs have been removed.

Is their any interest to keep compatibility with Solaris ldap? If not,
would it be possible to keep it for 5.6 and not drop it in a minor
release? I saw, that the docs mention OpenLDAp as a requirement, but it
worked for years very well with Solaris ldap client.

The build for 5.6 currently issues the following warnings due to new
incompatibilties:

.../ext/ldap/ldap.c: In function 'ldap_control_find':
.../ext/ldap/ldap.c:77:3: warning: implicit declaration of function
'ldap_find_control' [-Wimplicit-function-declaration]
return ldap_find_control(oid, ctrls);
^
.../ext/ldap/ldap.c:77:3: warning: return makes pointer from integer
without a cast
.../ext/ldap/ldap.c: In function 'zif_ldap_connect':
/shared/build/autobuild/workdirs/20150810_145109/bld/php56/ext/ldap/ldap.c:370:3:
warning: implicit declaration of function 'ldap_initialize'
[-Wimplicit-function-declaration]
rc = ldap_initialize(&ldap, url);
^
.../ext/ldap/ldap.c: In function 'zif_ldap_explode_dn':
.../ext/ldap/ldap.c:1308:2: warning: implicit declaration of function
'ldap_memvfree' [-Wimplicit-function-declaration]
ldap_memvfree((void **)ldap_value);
^
.../ext/ldap/ldap.c: In function 'zif_ldap_set_rebind_proc':
.../ext/ldap/ldap.c:2563:34: warning: passing argument 2 of
'ldap_set_rebind_proc' from incompatible pointer type
ldap_set_rebind_proc(ld->link, _ldap_rebind_proc, (void ) link);
^
In file included from .../ext/ldap/php_ldap.h:30:0,
from .../ext/ldap/ldap.c:45:
/usr/include/ldap.h:1205:26: note: expected 'int ()(struct LDAP *, char
**, char **, int *, int, void )' but argument is of type 'int
()(struct LDAP *, const char *, ber_tag_t, ber_int_t, void *)'
LDAP_API(void) LDAP_CALL ldap_set_rebind_proc(LDAP *ld,
^
Regards,

Rainer

9 years ago by Andreas Heigl — view source

unread

Hi Rainer.

Am 10.08.2015 um 21:48 schrieb Rainer Jung rainer.jung@kippdata.de:

Hi,

Am 18.06.2015 um 14:40 schrieb Côme BERNIGAUD:
Hello,

The code of the LDAP module have some ifdefs like
LDAP_API_FEATURE_X_OPENLDAP to test if we are building against openldap
or something else.
Is there an exaustive list of the LDAP libs PHP is supposed to be
buildable against?

The following page suggest it should always be openldap, is that the case?
http://php.net/manual/en/ldap.requirements.php
(if so, could ifdefs of LDAP_API_FEATURE_X_OPENLDAP be safely removed?)

What is the oldest openldap version supposed to be supported by each PHP
version?

being late to this discussion: I do ship PHP with ldap extension on Solaris using the Solaris ldap client libs. This worked well until and including 5.6.11. It is broken starting with 5.6.12, because now OpenLDAP is assumed and all or some compatibility ifdefs have been removed.

Is their any interest to keep compatibility with Solaris ldap? If not, would it be possible to keep it for 5.6 and not drop it in a minor release? I saw, that the docs mention OpenLDAp as a requirement, but it worked for years very well with Solaris ldap client.

The build for 5.6 currently issues the following warnings due to new incompatibilties:

.../ext/ldap/ldap.c: In function 'ldap_control_find':
.../ext/ldap/ldap.c:77:3: warning: implicit declaration of function 'ldap_find_control' [-Wimplicit-function-declaration]
return ldap_find_control(oid, ctrls);
^
.../ext/ldap/ldap.c:77:3: warning: return makes pointer from integer without a cast
.../ext/ldap/ldap.c: In function 'zif_ldap_connect':
/shared/build/autobuild/workdirs/20150810_145109/bld/php56/ext/ldap/ldap.c:370:3: warning: implicit declaration of function 'ldap_initialize' [-Wimplicit-function-declaration]
rc = ldap_initialize(&ldap, url);
^
.../ext/ldap/ldap.c: In function 'zif_ldap_explode_dn':
.../ext/ldap/ldap.c:1308:2: warning: implicit declaration of function 'ldap_memvfree' [-Wimplicit-function-declaration]
ldap_memvfree((void **)ldap_value);
^
.../ext/ldap/ldap.c: In function 'zif_ldap_set_rebind_proc':
.../ext/ldap/ldap.c:2563:34: warning: passing argument 2 of 'ldap_set_rebind_proc' from incompatible pointer type
ldap_set_rebind_proc(ld->link, _ldap_rebind_proc, (void ) link);
^
In file included from .../ext/ldap/php_ldap.h:30:0,
from .../ext/ldap/ldap.c:45:
/usr/include/ldap.h:1205:26: note: expected 'int ()(struct LDAP *, char **, char **, int *, int, void )' but argument is of type 'int ()(struct LDAP *, const char *, ber_tag_t, ber_int_t, void *)'
LDAP_API(void) LDAP_CALL ldap_set_rebind_proc(LDAP *ld,
^

Sorry to hear that the build against the Solaris LDAP client fails nowmduentonthe cleanup. Come asked a long while ago whether there are any other libs used. And as the dicumentation states that the build should link against OpenLDAP we went ahead and removed those ifdefs.

Can you give us a link to the Solaris LDAP lib? We'll then check what's possible and what not. The reason behind the cleanup (and therefore removal of ifdefs) was that we wanted to optimize PHPs ldap-functions and remove those calls to long-deprecated functions in the OpenLDAP library. Sonwe'll have to check what's possible with the Solaris lib.

Out of interest: What is for you the benefit of using the Solaris-ldap lib instead of the OpenLDAP lib? I'd like to get a feeling for the issue!

Cheers

Andreas

Andreas Heigl
andreas@heigl.org

9 years ago by Rainer Jung — view source

unread

Hi Andreas and all,

Am 10.08.2015 um 22:15 schrieb Andreas Heigl:

Hi Rainer.

Am 10.08.2015 um 21:48 schrieb Rainer Jung rainer.jung@kippdata.de:

Hi,

Am 18.06.2015 um 14:40 schrieb Côme BERNIGAUD:
Hello,

The code of the LDAP module have some ifdefs like
LDAP_API_FEATURE_X_OPENLDAP to test if we are building against openldap
or something else.
Is there an exaustive list of the LDAP libs PHP is supposed to be
buildable against?

The following page suggest it should always be openldap, is that the case?
http://php.net/manual/en/ldap.requirements.php
(if so, could ifdefs of LDAP_API_FEATURE_X_OPENLDAP be safely removed?)

What is the oldest openldap version supposed to be supported by each PHP
version?

being late to this discussion: I do ship PHP with ldap extension on Solaris using the Solaris ldap client libs. This worked well until and including 5.6.11. It is broken starting with 5.6.12, because now OpenLDAP is assumed and all or some compatibility ifdefs have been removed.

Is their any interest to keep compatibility with Solaris ldap? If not, would it be possible to keep it for 5.6 and not drop it in a minor release? I saw, that the docs mention OpenLDAp as a requirement, but it worked for years very well with Solaris ldap client.

The build for 5.6 currently issues the following warnings due to new incompatibilties:

.../ext/ldap/ldap.c: In function 'ldap_control_find':
.../ext/ldap/ldap.c:77:3: warning: implicit declaration of function 'ldap_find_control' [-Wimplicit-function-declaration]
return ldap_find_control(oid, ctrls);
^
.../ext/ldap/ldap.c:77:3: warning: return makes pointer from integer without a cast
.../ext/ldap/ldap.c: In function 'zif_ldap_connect':
/shared/build/autobuild/workdirs/20150810_145109/bld/php56/ext/ldap/ldap.c:370:3: warning: implicit declaration of function 'ldap_initialize' [-Wimplicit-function-declaration]
rc = ldap_initialize(&ldap, url);
^
.../ext/ldap/ldap.c: In function 'zif_ldap_explode_dn':
.../ext/ldap/ldap.c:1308:2: warning: implicit declaration of function 'ldap_memvfree' [-Wimplicit-function-declaration]
ldap_memvfree((void **)ldap_value);
^
.../ext/ldap/ldap.c: In function 'zif_ldap_set_rebind_proc':
.../ext/ldap/ldap.c:2563:34: warning: passing argument 2 of 'ldap_set_rebind_proc' from incompatible pointer type
ldap_set_rebind_proc(ld->link, _ldap_rebind_proc, (void ) link);
^
In file included from .../ext/ldap/php_ldap.h:30:0,
from .../ext/ldap/ldap.c:45:
/usr/include/ldap.h:1205:26: note: expected 'int ()(struct LDAP *, char **, char **, int *, int, void )' but argument is of type 'int ()(struct LDAP *, const char *, ber_tag_t, ber_int_t, void *)'
LDAP_API(void) LDAP_CALL ldap_set_rebind_proc(LDAP *ld,
^

Sorry to hear that the build against the Solaris LDAP client fails nowmduentonthe cleanup. Come asked a long while ago whether there are any other libs used. And as the dicumentation states that the build should link against OpenLDAP we went ahead and removed those ifdefs.

Yeah I should have noticed the discussion when it happened 2 months ago.

Can you give us a link to the Solaris LDAP lib? We'll then check what's possible and what not. The reason behind the cleanup (and therefore removal of ifdefs) was that we wanted to optimize PHPs ldap-functions and remove those calls to long-deprecated functions in the OpenLDAP library. Sonwe'll have to check what's possible with the Solaris lib.

The Solaris LDAP client is not open source. It has slapd as a common
historical origin just like the Netscape LDAP variant. The current
problems should be mostly around the above four compiler warnings. I can
test any patches you want me to test.

The man pages documenting the interface functions are readable under

http://docs.oracle.com/cd/E19253-01/816-5170/6mbb5es45/index.html

The introduction page with a list of all interfaces is

http://docs.oracle.com/cd/E19253-01/816-5170/ldap-3ldap/index.html

I found a publicly available ldap.h which is apart from non-functional
whitespace changes identical to the one on our system:

https://java.net/projects/solaris/sources/on-src/content/usr/src/head/ldap.h

Out of interest: What is for you the benefit of using the Solaris-ldap lib instead of the OpenLDAP lib? I'd like to get a feeling for the issue!

This is about providing PHP for the Solaris platform. On that platform
the Solaris ldap client is always installed. In theory you could remove
it but most enterprise installations use it one way or another, so you
can't get rid of it.

If PHP would only support the OpenLDAP client, we would have to bundle
that client together with PHP to make the PHP ldap extension work. This
would be feasible in principle, but there will be practical problems.
For instance if PHP is running inside the Apache web server (mod_php,
not FPM) and we activate the ldap extension, the Apache process will
load two ldap client libraries. The platform one, because it will be
used e.g. to resolve users etc. and the OpenLDAP one, because the PHP
extension is linked against it. Both will define common symbols with
incompatible implementations, so we would have to play dirty tricks to
make sure, that the httpd binary resolves all ldap symbols in the
platform libs, and the PHP ldap extension resolves the same symbols in
the OpenLDAP lib. It is doable, but one would typically prefer to stick
to the platform libs.

Thanks for your consideration.

Rainer

9 years ago by come.bernigaud@opensides.be — view source

unread

The current problems should be mostly around the above four compiler warnings. I can test any patches you want me to test.

Can you test including the attached file in ext/ldap/ldap.c, and not defining HAVE_3ARG_SETREBINDPROC and LDAP_CONTROL_PAGEDRESULTS ?

9 years ago by Rainer Jung — view source

unread

Hi Côme,

Am 11.08.2015 um 16:58 schrieb Côme BERNIGAUD:

The current problems should be mostly around the above four compiler
warnings. I can test any patches you want me to test.

Can you test including the attached file in ext/ldap/ldap.c, and not
defining HAVE_3ARG_SETREBINDPROC and LDAP_CONTROL_PAGEDRESULTS ?

thanks for the patch proposal. There's a few problems with it and I will
come back to you with an alternative proposal soon. For the moment:

the definition of ldap_control_find in terms of at the top of ldap.c
currently is protected by

#if !defined(HAVE_LDAP_CONTROL_FIND)

It should be

#if defined(LDAP_CONTROL_PAGEDRESULTS) && !defined(HAVE_LDAP_CONTROL_FIND)

That's not related to your patch.

void ldap_memvfree( void **value )
{
ldap_value_free(value);
}

it should probably be

ldap_value_free((char **)value);

otherwise we get compiler warnings.

instead of ldap_open() we can use ldap_init() (recommended).

using ldap_open() or ldap_init() with a url does not work, we have to
use host and port.

the use of ldap_sasl_bind_s() in PHP_FUNCTION(ldap_bind) does not
work, without setting "LDAP_OPT_PROTOCOL_VERSION" to LDAP_VERSION3 using
ldap_set_option(). Maybe it works for OpenLDAP, but Netscape and Solaris
document it must be set and indeed return LDAP_NOT_SUPPORTED immediately
otherwise. But since in PHP_FUNCTION(ldap_bind) no SASL functionality is
actually being used, I propose to use ldap_simple_bind_s() instead of
ldap_sasl_bind_s(). It is not deprecated and using it actually
simplifies the code a bit.

As I said, I'll come up with a patch proposal soon.

The patch should also help for other LDAP implementations than OpenLDAP
or Solaris.

Regards,

Rainer

9 years ago by Andreas Heigl — view source

unread

Hi Rainer.

Am 12.08.2015 um 13:00 schrieb Rainer Jung rainer.jung@kippdata.de:

Hi Côme,

Am 11.08.2015 um 16:58 schrieb Côme BERNIGAUD:

The current problems should be mostly around the above four compiler
warnings. I can test any patches you want me to test.

Can you test including the attached file in ext/ldap/ldap.c, and not
defining HAVE_3ARG_SETREBINDPROC and LDAP_CONTROL_PAGEDRESULTS ?

thanks for the patch proposal. There's a few problems with it and I will come back to you with an alternative proposal soon. For the moment:

the definition of ldap_control_find in terms of at the top of ldap.c currently is protected by

#if !defined(HAVE_LDAP_CONTROL_FIND)

It should be

#if defined(LDAP_CONTROL_PAGEDRESULTS) && !defined(HAVE_LDAP_CONTROL_FIND)

That's not related to your patch.

in

void ldap_memvfree( void **value )
{
ldap_value_free(value);
}

it should probably be

ldap_value_free((char **)value);

otherwise we get compiler warnings.

instead of ldap_open() we can use ldap_init() (recommended).

using ldap_open() or ldap_init() with a url does not work, we have to use host and port.

the use of ldap_sasl_bind_s() in PHP_FUNCTION(ldap_bind) does not work, without setting "LDAP_OPT_PROTOCOL_VERSION" to LDAP_VERSION3 using ldap_set_option(). Maybe it works for OpenLDAP, but Netscape and Solaris document it must be set and indeed return LDAP_NOT_SUPPORTED immediately otherwise. But since in PHP_FUNCTION(ldap_bind) no SASL functionality is actually being used, I propose to use ldap_simple_bind_s() instead of ldap_sasl_bind_s(). It is not deprecated and using it actually simplifies the code a bit.

As I said, I'll come up with a patch proposal soon.

The patch should also help for other LDAP implementations than OpenLDAP or Solaris.

The main question here is whether we want PHP to support other implementations. The documentation clearly states that OpenLDAP shall be used and even Oracle states on an official site (I don't actually have the URL at hand but it has been in an email to this thread) that PHPs LDAP extension has to be built against OpenLDAP.

Come has done a great job to clean up the LDAP code to start implementing long awaited features and I'm not really sure whether we should give that up for maintaining compatibility with some edge-case usages. So I'm very interested in your patch to see how we can rech both goals!

In the meantime you can still use the "old" ext/ldap for the newer PHP-branch as it should compile without an issue. The "newer" ext/ldap just removed stuff ;)

Cheers

Andreas

Regards,

Rainer

9 years ago by Rainer Jung — view source

unread

Am 12.08.2015 um 13:17 schrieb Andreas Heigl:

Hi Rainer.

Am 12.08.2015 um 13:00 schrieb Rainer Jung rainer.jung@kippdata.de:

Hi Côme,

Am 11.08.2015 um 16:58 schrieb Côme BERNIGAUD:

The current problems should be mostly around the above four compiler
warnings. I can test any patches you want me to test.

Can you test including the attached file in ext/ldap/ldap.c, and not
defining HAVE_3ARG_SETREBINDPROC and LDAP_CONTROL_PAGEDRESULTS ?

thanks for the patch proposal. There's a few problems with it and I will come back to you with an alternative proposal soon. For the moment:

the definition of ldap_control_find in terms of at the top of ldap.c currently is protected by

#if !defined(HAVE_LDAP_CONTROL_FIND)

It should be

#if defined(LDAP_CONTROL_PAGEDRESULTS) && !defined(HAVE_LDAP_CONTROL_FIND)

That's not related to your patch.

in

void ldap_memvfree( void **value )
{
ldap_value_free(value);
}

it should probably be

ldap_value_free((char **)value);

otherwise we get compiler warnings.

instead of ldap_open() we can use ldap_init() (recommended).

using ldap_open() or ldap_init() with a url does not work, we have to use host and port.

the use of ldap_sasl_bind_s() in PHP_FUNCTION(ldap_bind) does not work, without setting "LDAP_OPT_PROTOCOL_VERSION" to LDAP_VERSION3 using ldap_set_option(). Maybe it works for OpenLDAP, but Netscape and Solaris document it must be set and indeed return LDAP_NOT_SUPPORTED immediately otherwise. But since in PHP_FUNCTION(ldap_bind) no SASL functionality is actually being used, I propose to use ldap_simple_bind_s() instead of ldap_sasl_bind_s(). It is not deprecated and using it actually simplifies the code a bit.

As I said, I'll come up with a patch proposal soon.

The patch should also help for other LDAP implementations than OpenLDAP or Solaris.

The main question here is whether we want PHP to support other implementations. The documentation clearly states that OpenLDAP shall be used and even Oracle states on an official site (I don't actually have the URL at hand but it has been in an email to this thread) that PHPs LDAP extension has to be built against OpenLDAP.

That's you (project) call. You are doing the work, you decide. I
wouldn't rely on Oracle for a judgement ;)

Also note that the code and m4 macros have quite a few places where they
react on other LDAP SDKs, like Windows, Oracle (not sun) and I think
also Netware. But those are not really strong arguments for multi-SDK
support. You can purge those places and the argument is gone. I really
think it depends on your goals.

But you should consider 5.6 a stable release. Maybe I'm the only guy on
the planet haven't build the ldap extension against non-OpenLDAP, but I
doubt it. 7.0 is in front of the doors and compatibility is much less an
issue there, maybe combined with a NEWS entry strengthening, that you
now really demand OpenLDAP and throw an error, if you can't find it
during configure.

Come has done a great job to clean up the LDAP code to start implementing long awaited features and I'm not really sure whether we should give that up for maintaining compatibility with some edge-case usages. So I'm very interested in your patch to see how we can rech both goals!

Agreed, and much of the cleanup wouldn't have to be undone to make the
result still compatible with more SDKs. For instance all of the new use
of the "_ext" variants of functions is nice and OK also for other LDAP
implementations.

I'll attach my current patch. See below for some comments on the patch.

In the meantime you can still use the "old" ext/ldap for the newer PHP-branch as it should compile without an issue. The "newer" ext/ldap just removed stuff ;)

Sure, that's the workaround I would use if you decide to require OpenLDAP.

Concerning the patch:

I switched from

#ifndef HAVE_LDAP_CONTROL_FIND

#if defined(LDAP_CONTROL_PAGEDRESULTS) && !defined(HAVE_LDAP_CONTROL_FIND)

because the contents of the ifndef-block are only used in the
LDAP_CONTROL_PAGEDRESULTS case and the block needs ldap_find_control,
which isn't available in the general case.

I added a definition of ldap_memvfree() in terms of ldap_value_free()
similar to Come's proposal, but surrounded it by #if
!defined(LDAP_API_FEATURE_X_OPENLDAP). This define is set in OpenLDAP
since about 1998.

I replaced all

#if defined(HAVE_3ARG_SETREBINDPROC)

with

#if defined(LDAP_API_FEATURE_X_OPENLDAP) && defined(HAVE_3ARG_SETREBINDPROC)

Reasoning: most LDAP impls have a 3 arg ldap_set_rebind_proc(), but the
callback function argument has no standardized signature. The different
impls use different arguments in the callback and using them would need
different code. You can't simply abstract away the differences. So for
now the existing code only makes definite sense for OpenLDAP. For other
SDKs it might need to vary.

I protected the use of ldap_initialize() by

#ifdef LDAP_API_FEATURE_X_OPENLDAP

and switch back to ldap_init(host, port) with a slightly different type
of success check for other impls. The original idea of Come to define
ldap_initialize(&ldap, url) in terms of ldap_init(host, port) does not
work, because ldap_init() doesn't take URLs.

I protected the use of ldap_sasl_bind_s(..., ..., LDAP_SASL_SIMPLE,
...) by

#ifdef LDAP_API_FEATURE_X_OPENLDAP

and switch back to ldap_simple_bind_s() for the other impls. This is
because the sasl call for instance for the Solaris impl fails, if you do
not explicitly set the protocol version on ld->link to LDAP_VERSION3
using ldap_set_option(). Other impls have different behavior. Since we
here only want to do a simple bind, I used the old style call in the non
OpenLDAP case.

All of these changes should also be OK for other LDAP impls than
OpenLDAP and Solaris. I don't see reasons, why they currently shouldn't
be able to use the patched code without further adjustments.

Regards,

Rainer

9 years ago by Andreas Heigl — view source

unread

Hi Rainer.

Am 13.08.15 um 16:39 schrieb Rainer Jung:

Am 12.08.2015 um 13:17 schrieb Andreas Heigl:

Hi Rainer.

Am 12.08.2015 um 13:00 schrieb Rainer Jung rainer.jung@kippdata.de:

Hi Côme,

Am 11.08.2015 um 16:58 schrieb Côme BERNIGAUD:

The current problems should be mostly around the above four compiler
warnings. I can test any patches you want me to test.

Can you test including the attached file in ext/ldap/ldap.c, and not
defining HAVE_3ARG_SETREBINDPROC and LDAP_CONTROL_PAGEDRESULTS ?

thanks for the patch proposal. There's a few problems with it and I
will come back to you with an alternative proposal soon. For the moment:

the definition of ldap_control_find in terms of at the top of
ldap.c currently is protected by

#if !defined(HAVE_LDAP_CONTROL_FIND)

It should be

#if defined(LDAP_CONTROL_PAGEDRESULTS) &&
!defined(HAVE_LDAP_CONTROL_FIND)

That's not related to your patch.

in

void ldap_memvfree( void **value )
{
ldap_value_free(value);
}

it should probably be

ldap_value_free((char **)value);

otherwise we get compiler warnings.

instead of ldap_open() we can use ldap_init() (recommended).

using ldap_open() or ldap_init() with a url does not work, we have
to use host and port.

the use of ldap_sasl_bind_s() in PHP_FUNCTION(ldap_bind) does not
work, without setting "LDAP_OPT_PROTOCOL_VERSION" to LDAP_VERSION3
using ldap_set_option(). Maybe it works for OpenLDAP, but Netscape
and Solaris document it must be set and indeed return
LDAP_NOT_SUPPORTED immediately otherwise. But since in
PHP_FUNCTION(ldap_bind) no SASL functionality is actually being used,
I propose to use ldap_simple_bind_s() instead of ldap_sasl_bind_s().
It is not deprecated and using it actually simplifies the code a bit.

As I said, I'll come up with a patch proposal soon.

The patch should also help for other LDAP implementations than
OpenLDAP or Solaris.

The main question here is whether we want PHP to support other
implementations. The documentation clearly states that OpenLDAP shall
be used and even Oracle states on an official site (I don't actually
have the URL at hand but it has been in an email to this thread) that
PHPs LDAP extension has to be built against OpenLDAP.

That's you (project) call. You are doing the work, you decide. I
wouldn't rely on Oracle for a judgement ;)

We should all decide (somehow) ;) And thanks for the warning about Oracle ;)

Also note that the code and m4 macros have quite a few places where they
react on other LDAP SDKs, like Windows, Oracle (not sun) and I think
also Netware. But those are not really strong arguments for multi-SDK
support. You can purge those places and the argument is gone. I really
think it depends on your goals.

But you should consider 5.6 a stable release. Maybe I'm the only guy on
the planet haven't build the ldap extension against non-OpenLDAP, but I
doubt it. 7.0 is in front of the doors and compatibility is much less an
issue there, maybe combined with a NEWS entry strengthening, that you
now really demand OpenLDAP and throw an error, if you can't find it
during configure.

You are actually right on that one. We should keep BC in the 5.6
release. That was the reason for the call to what other libs are used
where no one stepped up for others and therefore there wasn't any BC
expected.... :(

@Côme, can we fix that in the 5.6 branch somehow?

Come has done a great job to clean up the LDAP code to start
implementing long awaited features and I'm not really sure whether we
should give that up for maintaining compatibility with some edge-case
usages. So I'm very interested in your patch to see how we can rech
both goals!

Agreed, and much of the cleanup wouldn't have to be undone to make the
result still compatible with more SDKs. For instance all of the new use
of the "_ext" variants of functions is nice and OK also for other LDAP
implementations.

That's great! I'm not that deep into the C-business (yet) therefore I'll
need a day or two to digest the patch. I'd just love to see the
LDAP-extension move towards better implementation of such things as
paginated and/or serversided sorted results.

Thanks for your input and contribution!

Cheers

Andreas

--
,,,
(o o)
+---------------------------------------------------------ooO-(_)-Ooo-+
| Andreas Heigl |
| mailto:andreas@heigl.org N 50°22'59.5" E 08°23'58" |
| http://andreas.heigl.org http://hei.gl/wiFKy7 |
+---------------------------------------------------------------------+
| http://hei.gl/root-ca |
+---------------------------------------------------------------------+

9 years ago by Rainer Jung — view source

unread

I checked the latest 5.6 release and it looks fine to me. Thanks for
incorporating the patch!

Rainer

Am 13.08.2015 um 17:32 schrieb Andreas Heigl:

Hi Rainer.

Am 13.08.15 um 16:39 schrieb Rainer Jung:

Am 12.08.2015 um 13:17 schrieb Andreas Heigl:

Hi Rainer.

Am 12.08.2015 um 13:00 schrieb Rainer Jung rainer.jung@kippdata.de:

Hi Côme,

Am 11.08.2015 um 16:58 schrieb Côme BERNIGAUD:

The current problems should be mostly around the above four compiler
warnings. I can test any patches you want me to test.

Can you test including the attached file in ext/ldap/ldap.c, and not
defining HAVE_3ARG_SETREBINDPROC and LDAP_CONTROL_PAGEDRESULTS ?

thanks for the patch proposal. There's a few problems with it and I
will come back to you with an alternative proposal soon. For the moment:

the definition of ldap_control_find in terms of at the top of
ldap.c currently is protected by

#if !defined(HAVE_LDAP_CONTROL_FIND)

It should be

#if defined(LDAP_CONTROL_PAGEDRESULTS) &&
!defined(HAVE_LDAP_CONTROL_FIND)

That's not related to your patch.

in

void ldap_memvfree( void **value )
{
ldap_value_free(value);
}

it should probably be

ldap_value_free((char **)value);

otherwise we get compiler warnings.

instead of ldap_open() we can use ldap_init() (recommended).

using ldap_open() or ldap_init() with a url does not work, we have
to use host and port.

the use of ldap_sasl_bind_s() in PHP_FUNCTION(ldap_bind) does not
work, without setting "LDAP_OPT_PROTOCOL_VERSION" to LDAP_VERSION3
using ldap_set_option(). Maybe it works for OpenLDAP, but Netscape
and Solaris document it must be set and indeed return
LDAP_NOT_SUPPORTED immediately otherwise. But since in
PHP_FUNCTION(ldap_bind) no SASL functionality is actually being used,
I propose to use ldap_simple_bind_s() instead of ldap_sasl_bind_s().
It is not deprecated and using it actually simplifies the code a bit.

As I said, I'll come up with a patch proposal soon.

The patch should also help for other LDAP implementations than
OpenLDAP or Solaris.

The main question here is whether we want PHP to support other
implementations. The documentation clearly states that OpenLDAP shall
be used and even Oracle states on an official site (I don't actually
have the URL at hand but it has been in an email to this thread) that
PHPs LDAP extension has to be built against OpenLDAP.

That's you (project) call. You are doing the work, you decide. I
wouldn't rely on Oracle for a judgement ;)

We should all decide (somehow) ;) And thanks for the warning about Oracle ;)

Also note that the code and m4 macros have quite a few places where they
react on other LDAP SDKs, like Windows, Oracle (not sun) and I think
also Netware. But those are not really strong arguments for multi-SDK
support. You can purge those places and the argument is gone. I really
think it depends on your goals.

But you should consider 5.6 a stable release. Maybe I'm the only guy on
the planet haven't build the ldap extension against non-OpenLDAP, but I
doubt it. 7.0 is in front of the doors and compatibility is much less an
issue there, maybe combined with a NEWS entry strengthening, that you
now really demand OpenLDAP and throw an error, if you can't find it
during configure.

You are actually right on that one. We should keep BC in the 5.6
release. That was the reason for the call to what other libs are used
where no one stepped up for others and therefore there wasn't any BC
expected.... :(

@Côme, can we fix that in the 5.6 branch somehow?

Come has done a great job to clean up the LDAP code to start
implementing long awaited features and I'm not really sure whether we
should give that up for maintaining compatibility with some edge-case
usages. So I'm very interested in your patch to see how we can rech
both goals!

Agreed, and much of the cleanup wouldn't have to be undone to make the
result still compatible with more SDKs. For instance all of the new use
of the "_ext" variants of functions is nice and OK also for other LDAP
implementations.

That's great! I'm not that deep into the C-business (yet) therefore I'll
need a day or two to digest the patch. I'd just love to see the
LDAP-extension move towards better implementation of such things as
paginated and/or serversided sorted results.

Thanks for your input and contribution!

Cheers

Andreas