On Fri, Nov 08, 2013 at 01:35:47PM +0100, Jakub Hrozek wrote: > On Thu, Nov 07, 2013 at 08:43:35PM +0100, Lukas Slebodnik wrote: > > On (31/10/13 11:12), Lukas Slebodnik wrote: > > >On (15/10/13 15:42), Benjamin Franzke wrote: > > >>Hi Lukas, > > >> > > >> > > >>2013/10/15 Lukas Slebodnik <[email protected]> > > >> > > >>> On (15/10/13 11:47), Benjamin Franzke wrote: > > >>> >Hi, > > >>> > > > >>> >These two patches add missing CFLAGS/LIBS to Makefile.am: > > >>> > > > >>> >[PATCH 1/2] BUILD: Link libsss_ad.so to sasl libs > > >>> >[PATCH 2/2] BUILD: Use OPENLDAP_CFLAGS instead of LDAP_CFLAGS > > >>> ACK to 2nd patch > > >>> > > > >>> >This underlinking was noticed in make check (dlopen-test). > > >>> > > > >>> >Note: > > >>> >It failed for me since my openldap build had no sasl support, > > >>> >which would otherwise have pulled in libsasl2.so. > > >>> >Of course, that support should be in place, but the linking should > > >>> >still > > >>> be > > >>> >fixed. > > >>> > > > >>> >BTW: It would propably be nice to have a configure check whether > > >>> >openldap has sasl support, but it seems that would need a check if > > >>> >ldap_sasl_interactive_bind returns LDAP_NOT_SUPPORTED. > > >>> > > > >>> >Regards, Ben > > >>> > > >>> I have a little problem with the first patch. > > >>> > > >>> > > >>> From d26a15de098df2b42582cda590e184c69f48bb7f Mon Sep 17 00:00:00 2001 > > >>> From: Benjamin Franzke <[email protected]> > > >>> Date: Tue, 15 Oct 2013 10:27:36 +0200 > > >>> Subject: [PATCH 1/2] BUILD: Link libsss_ad.so to sasl libs > > >>> > > >>> This is for the sasl_client_init symbol. > > >>> Introducted in commit fb945a2c. > > >>> --- > > >>> Makefile.am | 2 ++ > > >>> configure.ac | 2 +- > > >>> 2 files changed, 3 insertions(+), 1 deletion(-) > > >>> > > >>> diff --git a/Makefile.am b/Makefile.am > > >>> index > > >>> ff1e71e72d90a6eff658a40e2ca24c1929b31aa5..8c919d40481720e4661a42a188cea2fd179282d4 > > >>> 100644 > > >>> --- a/Makefile.am > > >>> +++ b/Makefile.am > > >>> @@ -1713,12 +1713,14 @@ libsss_ad_la_CFLAGS = \ > > >>> $(AM_CFLAGS) \ > > >>> $(SYSTEMD_LOGIN_CFLAGS) \ > > >>> $(LDAP_CFLAGS) \ > > >>> + $(SASL_CFLAGS) \ > > >>> $(DHASH_CFLAGS) \ > > >>> $(KRB5_CFLAGS) \ > > >>> $(NDR_NBT_CFLAGS) > > >>> libsss_ad_la_LIBADD = \ > > >>> $(SYSTEMD_LOGIN_LIBS) \ > > >>> $(OPENLDAP_LIBS) \ > > >>> + $(SASL_LIBS) \ > > >>> $(DHASH_LIBS) \ > > >>> $(KEYUTILS_LIBS) \ > > >>> $(KRB5_LIBS) \ > > >>> diff --git a/configure.ac b/configure.ac > > >>> index > > >>> d28d55f3b0eb35cc4ecc02ae9b1fafbcb9588dcf..7e3819a86ad94176e95f1ded07b88d25399888de > > >>> 100644 > > >>> --- a/configure.ac > > >>> +++ b/configure.ac > > >>> @@ -260,7 +260,7 @@ fi > > >>> > > >>> AM_CHECK_INOTIFY > > >>> > > >>> -AC_CHECK_HEADERS([sasl/sasl.h],,AC_MSG_ERROR([Could not find SASL > > >>> headers])) > > >>> +PKG_CHECK_MODULES([SASL], [libsasl2], [], [AC_MSG_ERROR([Could not find > > >>> SASL library])]) > > >>> > > >>> pkg-config file needn't be available one some distribution (platforms). > > >>> For example popt-devel on fedora 19 doesn't have pkg-config file. > > >>> So it is better to fallback to AC_CHECK_HEADER. > > >>> > > >>> 1. And it does not mean that openldap was build with sasl support if > > >>> libsasl2 > > >>> is installed on the machine. Detection should more complex. > > >>> > > >> > > >>Yes, thats why the dea to detect whether ldap has sasl support, which is > > >>not easy, as Stephen already said. > > >> > > >>> > > >>> 2. According to man ldap_sasl_interactive_bind_s, we should only link > > >>> with > > >>> library "OpenLDAP LDAP (libldap, -lldap)" and we does not directly > > >>> use > > >>> any sasl function. What kind of distribution do you use? > > >>> > > >> > > >>As written in the commit message, since commit fb945a2c sssd uses sasl > > >>unconditionally: > > >>git grep sasl_client_init > > >>src/providers/ad/ad_init.c: (void)sasl_client_init(ad_sasl_callbacks); > > >> > > >>So if sssd should be build without sasl that'd need to be fixed. > > >> > > >>> > > >>> 3. IIRC, sssd can be build with ldap without sasl support, but ipa > > >>> provider > > >>> will not work. And it should be similar with ad provider. I would > > >>> prefer > > >>> to use conditional build instead of failing if ldap does not support > > >>> sasl. > > >>> Because with your patch, sssd could not be build without sasl library > > >>> and > > >>> someone can use sssd only with ldap provider (and he doesn't care > > >>> about > > >>> sasl) > > >>> > > >> > > >>How did it not fail before, when sasl was not installed? I mean there was > > >>error too.. > > >> > > >I realized sssd cannot be built without header file sasl.h, because > > >we include header file in sdap_async_connection.c > > > src/providers/ldap/sdap_async_connection.c:#include <sasl/sasl.h> > > >But we needn't link ldap_common with sasl library, because sasl functions > > >are not used in module sdap_async_connection.c > > > > > >The last problem is in your patch, that you replaced > > >macro AC_CHECK_HEADERS with pkg-config detection. Sasl pkg-config file > > >needn't be available on some distributions. > > >For example, the latest ubuntu does not have sasl.pc in the package > > >libsasl2-dev. > > > > > >http://packages.ubuntu.com/saucy/amd64/libsasl2-dev/filelist > > > > > >LS > > > > I am attaching patches with more robust detection of SASL, > > because pkg-config file is not available in older versions of > > cysrus sasl <= 2.1.25. (Fedora<=18, RHEL<=6, all versions of ubuntu) > > > > Configure failed with Benjamin's version on RHEL6. > > > > LS > > These patches look good to me and are mostly an incremental improvement > over what Stephen already acked before, so ACK from me.
The above did sound like I did no testing :-) Build went fine on F-20 64bit and in RHEL-6 mock chroot. AD provider started fine and was able to establish a GSSAPI connection. _______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
