Skip to content

Drop Mozilla LDAP SDK detection, update --with-ldap code #1710

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
yadij wants to merge 1 commit into from
Closed

Drop Mozilla LDAP SDK detection, update --with-ldap code #1710

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 9 additions & 13 deletions acinclude/ldap.m4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,9 @@ AC_DEFUN([SQUID_LDAP_TEST],[
LIBS="$LIBLDAP_PATH $LIBLDAP_LIBS $LIBPTHREADS"
CPPFLAGS="-DLDAP_DEPRECATED=1 -DLDAP_REFERRALS $CPPFLAGS"
AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[
# if HAVE_LDAP_H
# include <ldap.h>
# elif HAVE_MOZLDAP_LDAP_H
# include <mozldap/ldap.h>
# endif
#if HAVE_LDAP_H
#include <ldap.h>
#endif
]],[[$2]])
],[
squid_cv_$1=1
Expand All @@ -37,12 +35,12 @@ AC_DEFUN([SQUID_LDAP_TEST_RUN],[
LIBS="$LIBLDAP_PATH $LIBLDAP_LIBS $LIBPTHREADS"
CPPFLAGS="-DLDAP_DEPRECATED=1 -DLDAP_REFERRALS $CPPFLAGS"
AC_RUN_IFELSE([AC_LANG_PROGRAM([[
# if HAVE_LDAP_H
# include <ldap.h>
# elif HAVE_MOZLDAP_LDAP_H
# include <mozldap/ldap.h>
# endif
# include <string.h>
#if HAVE_LDAP_H
#include <ldap.h>
#endif
#if HAVE_STRING_H
#include <string.h>
#endif
]],[[$2]])
],[
m4_translit([squid_cv_$1],[-+. ],[____])=1
Expand All @@ -61,7 +59,6 @@ dnl find the LDAP library vendor and define relevant HAVE_(vendor name) macro
AC_DEFUN([SQUID_LDAP_CHECK_VENDOR],[
SQUID_LDAP_TEST_RUN([OpenLDAP],[return strcmp(LDAP_VENDOR_NAME,"OpenLDAP")])
SQUID_LDAP_TEST_RUN([Sun LDAP SDK],[return strcmp(LDAP_VENDOR_NAME,"Sun Microsystems Inc.")])
SQUID_LDAP_TEST_RUN([Mozilla LDAP SDK],[return strcmp(LDAP_VENDOR_NAME,"mozilla.org")])
])

dnl check whether the LDAP library(s) provide the needed API and types
Expand Down Expand Up @@ -99,5 +96,4 @@ AC_DEFUN([SQUID_CHECK_LDAP_API],[
AC_SEARCH_LIBS([ldap_start_tls_s],[$LIBLDAP_NAMES],[
AC_DEFINE(HAVE_LDAP_START_TLS_S,1,[Define to 1 if you have ldap_start_tls_s])
])
SQUID_STATE_ROLLBACK(squid_ldap_state)
])
34 changes: 10 additions & 24 deletions configure.ac
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1219,33 +1219,19 @@ SQUID_CHECK_LIB_WORKS(gss,[
])

SQUID_AUTO_LIB(ldap,[LDAP],[LIBLDAP])
dnl On MinGW set Windows LDAP libraries using -lwldap32
AS_IF([test "x$with_ldap" != "xno" -a "$squid_host_os" = "mingw"],[
LIBLDAP_LIBS="$LIBLDAP_LIBS -lwldap32"
Copy link
Contributor

@rousskov rousskov Mar 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feels wrong to change LIBLDAP_LIBS that was set by admin. Should this be conditional on LIBLDAP_LIBS being empty?

Copy link
Contributor Author

@yadij yadij Mar 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe, for now I am leaving the logic inside this case as-was. Testing changes to it need some testing on MinGW which we are not quite up to doing (yet)

Copy link
Contributor

@rousskov rousskov Mar 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That excuse itself is very weak, but when combined with the fact that this PR does change the condition, it becomes invalid.

AFAICT, the PRs in this series routinely introduce untested changes. That is just the nature of this work, given our current CI limitations. IMO, we should not apply "need some testing before I modify this further" logic when dealing with changes that appear to violate basic rules. Given the two evils, I recommend increasing the risk of breaking MinGW build instead of committing such violations. These changes should be moving us forward as far as code quality is concerned. We can fix marginal builds later, as needed.

Comment on lines +1222 to +1224
Copy link
Contributor

@rousskov rousskov Mar 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we have to place this special code outside of SQUID_CHECK_LIB_WORKS()?

Copy link
Contributor Author

@yadij yadij Mar 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To extract it for now outside the other logic. I would like to later (n the MinGW work) identify whether it is actually needed, or if we should have a completely separate library option+check for the Windows LDAP.

Copy link
Contributor

@rousskov rousskov Mar 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alex: Why do we have to place this special code outside of SQUID_CHECK_LIB_WORKS()?

Amos: To extract it for now outside the other logic

That sentence does not appear to answer the "why" question. It essentially restates the fact accepted by the question.

Amos: I would like to later identify whether it is actually needed, or if we should have a completely separate library option+check for the Windows LDAP.

I see no reason to move this special code to accomplish the stated "identify the need" and associated possible refactoring goals. AFAICT, the move itself contradicts the intent behind SQUID_CHECK_LIB_WORKS() approach. We should not move unless there is a very compelling argument for doing so. No such argument has been provided AFAICT.

])
SQUID_CHECK_LIB_WORKS(ldap,[
dnl On MinGW OpenLDAP is not available, so LDAP helpers can be linked
dnl only with Windows LDAP libraries using -lwldap32
AS_IF([test "$squid_host_os" = "mingw"],[
LIBLDAP_LIBS="-lwldap32"
],[
SQUID_STATE_SAVE(squid_ldap_state)
LIBS="$LIBLDAP_PATH $LIBPTHREADS $LIBS"
PKG_CHECK_MODULES([LIBLDAP],[ldap],[],[
AC_CHECK_LIB(lber, ber_init, [LIBLBER="-llber"])
AC_CHECK_LIB(ldap, ldap_init, [LIBLDAP_LIBS="-lldap $LIBLBER"])
dnl if no ldap lib found check for mozilla version
AS_IF([test "x$ac_cv_lib_ldap_ldap_init" != "xyes"],[
SQUID_STATE_SAVE(squid_ldap_mozilla)
LIBS="$LIBLDAP_PATH $LIBPTHREADS"
AC_CHECK_LIB(ldap60, ldap_init, [LIBLDAP_LIBS="-lldap60 $LIBLBER"])
LIBS="$LIBLDAP_PATH $LIBLDAP_LIBS $LIBPTHREADS"
AC_CHECK_LIB(prldap60, prldap_init, [LIBLDAP_LIBS="-lprldap60 $LIBLDAP_LIBS"])
LIBS="$LIBLDAP_PATH $LIBLDAP_LIBS $LIBPTHREADS"
AC_CHECK_LIB(ssldap60, ldapssl_init, [LIBLDAP_LIBS="-lssldap60 $LIBLDAP_LIBS"])
SQUID_STATE_ROLLBACK(squid_ldap_mozilla)
])
])
AC_CHECK_HEADERS(ldap.h lber.h)
AC_CHECK_HEADERS(mozldap/ldap.h)
SQUID_STATE_SAVE(squid_ldap_state)
LIBS="$LIBLDAP_PATH $LIBPTHREADS $LIBS"
PKG_CHECK_MODULES([LIBLDAP],[ldap],[:],[:])
AS_IF([test "x$LIBLDAP_LIBS" != "x"],[
AC_CHECK_HEADERS(ldap.h lber.h winldap.h)
SQUID_CHECK_LDAP_API
])
SQUID_STATE_ROLLBACK(squid_ldap_state)
])

SQUID_AUTO_LIB(systemd,[systemd API for start-up notification],[LIBSYSTEMD])
Expand Down
2 changes: 1 addition & 1 deletion doc/manuals/manuals.pot
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -772,7 +772,7 @@ msgstr ""
#: src/acl/external/kerberos_ldap_group/ext_kerberos_ldap_group_acl.8:80
msgid ""
"The SSL certificate database can be set via the environment variable "
"SSL_CERTDBPATH (default /etc/certs) (Sun and Mozilla LDAP SDK)."
"SSL_CERTDBPATH (default /etc/certs) (Sun LDAP SDK)."
msgstr ""

#. type: Plain text
Expand Down
3 changes: 3 additions & 0 deletions doc/release-notes/release-7.sgml.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,9 @@ This section gives an account of those changes in three categories:
<p>The ESI feature is now disabled by default.
Use <em>--enable-esi</em> if needed.

<tag>--with-ldap</tag>
<p>No longer builds with Mozilla LDAP SDK.

</descrip>
</p>

Expand Down
2 changes: 1 addition & 1 deletion src/acl/external/LDAP_group/required.m4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,4 @@
## Please see the COPYING and CONTRIBUTORS files for details.
##

AC_CHECK_HEADERS([ldap.h winldap.h],[BUILD_HELPER="LDAP_group"])
AS_IF([test "x$LIBLDAP_LIBS" != "x"],[BUILD_HELPER="LDAP_group"])
2 changes: 1 addition & 1 deletion src/acl/external/eDirectory_userip/required.m4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,4 @@
## Please see the COPYING and CONTRIBUTORS files for details.
##

AC_CHECK_HEADERS([ldap.h winldap.h],[BUILD_HELPER="eDirectory_userip"])
AS_IF([test "x$LIBLDAP_LIBS" != "x"],[BUILD_HELPER="eDirectory_userip"])
2 changes: 1 addition & 1 deletion src/acl/external/kerberos_ldap_group/ext_kerberos_ldap_group_acl.8
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ Use SSL for the LDAP connection.
.IP
The CA certificate file can be set via the environment variable TLS_CACERTFILE (default /etc/ssl/certs/cert.pem) (OpenLDAP).
.IP
The SSL certificate database can be set via the environment variable SSL_CERTDBPATH (default /etc/certs) (Sun and Mozilla LDAP SDK).
The SSL certificate database can be set via the environment variable SSL_CERTDBPATH (default /etc/certs) (Sun LDAP SDK).
.if !'po4a'hide' .TP 12
.if !'po4a'hide' .B \-a
Allow SSL without certificate verification.
Expand Down
3 changes: 0 additions & 3 deletions src/acl/external/kerberos_ldap_group/support.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,6 @@
#if HAVE_LDAP_H
#include <ldap.h>
#endif
#if HAVE_MOZLDAP_LDAP_H
#include <mozldap/ldap.h>
#endif

struct gdstruct {
char *group;
Expand Down
3 changes: 2 additions & 1 deletion src/acl/external/kerberos_ldap_group/support_ldap.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,8 @@ size_t get_bin_attributes(LDAP * ld, LDAPMessage * res,
int search_group_tree(struct main_args *margs, LDAP * ld, char *bindp,
char *ldap_group, char *group, int depth);

#if HAVE_SUN_LDAP_SDK || HAVE_MOZILLA_LDAP_SDK
#if HAVE_SUN_LDAP_SDK

#if HAVE_LDAP_REBINDPROC_CALLBACK

#if HAVE_SASL_H || HAVE_SASL_SASL_H || HAVE_SASL_DARWIN
Expand Down
2 changes: 1 addition & 1 deletion src/auth/basic/LDAP/required.m4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,4 @@
## Please see the COPYING and CONTRIBUTORS files for details.
##

AC_CHECK_HEADERS([ldap.h winldap.h],[BUILD_HELPER="LDAP"])
AS_IF([test "x$LIBLDAP_LIBS" != "x"],[BUILD_HELPER="LDAP"])
2 changes: 1 addition & 1 deletion src/auth/digest/eDirectory/required.m4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,4 @@
## Please see the COPYING and CONTRIBUTORS files for details.
##

AC_CHECK_HEADERS([ldap.h winldap.h],[BUILD_HELPER="eDirectory"])
AS_IF([test "x$LIBLDAP_LIBS" != "x"],[BUILD_HELPER="eDirectory"])