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
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
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 10/15/2013 05:47 AM, 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
Ack
[PATCH 2/2] BUILD: Use OPENLDAP_CFLAGS instead of LDAP_CFLAGS
Ack. I'm somewhat amazed this hasn't been noticed. It's been like this since 2011.
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.
Right, and I think that would necessitate a live server to test against. Ouch.
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 benjaminfranzke@googlemail.com 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.
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?
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)
LS
Hi Lukas,
2013/10/15 Lukas Slebodnik lslebodn@redhat.com
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 benjaminfranzke@googlemail.com 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.
- 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.
- 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.
- 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..
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On (15/10/13 15:42), Benjamin Franzke wrote:
Hi Lukas,
2013/10/15 Lukas Slebodnik lslebodn@redhat.com
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 benjaminfranzke@googlemail.com 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.
- 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.
- 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);
ahh
I remember this patch, but I still think that we should not fail if sasl is not available. A lot of features are optional in sssd. For example cifs plugin :-) SASL in ad-provider can be the same situation.
But it is nice catch and we should also update spec file, to explicitly require sasl in sssd-ad.
So if sssd should be build without sasl that'd need to be fixed.
- 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 checked mock-build with fedora 19 and normal build in ubuntu 13.04. There was not problem with any test.
LS
On (15/10/13 15:42), Benjamin Franzke wrote:
Hi Lukas,
2013/10/15 Lukas Slebodnik lslebodn@redhat.com
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 benjaminfranzke@googlemail.com 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.
- 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.
- 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.
- 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
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 lslebodn@redhat.com
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 benjaminfranzke@googlemail.com 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.
- 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.
- 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.
- 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
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 lslebodn@redhat.com
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 benjaminfranzke@googlemail.com 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.
- 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.
- 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.
- 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.
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 lslebodn@redhat.com
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 benjaminfranzke@googlemail.com 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.
- 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.
- 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.
- 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.
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 lslebodn@redhat.com
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 benjaminfranzke@googlemail.com 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.
- 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.
- 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.
- 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.
Pushed to master.
On (07/11/13 20:43), 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 lslebodn@redhat.com
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 benjaminfranzke@googlemail.com 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.
- 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.
- 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.
- 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
From 8b4746d7e2d5c53cbe5d29f1ce984a16aefb5a54 Mon Sep 17 00:00:00 2001 From: Lukas Slebodnik lslebodn@redhat.com Date: Tue, 15 Oct 2013 10:27:36 +0200 Subject: [PATCH 1/2] BUILD: Explicitly link libsss_ad.so with sasl libs
If openldap is not built with sasl support libsss_ad.so will not be linked with libsasl2 although sasl_client_init is called by function ad_sasl_initialize.
Makefile.am | 2 ++ configure.ac | 3 +-- src/external/sasl.m4 | 17 +++++++++++++++++ 3 files changed, 20 insertions(+), 2 deletions(-) create mode 100644 src/external/sasl.m4
Could someone push 1st patch also to the 1-11 branch?
LS
On Sat, 2014-05-17 at 19:49 +0200, Lukas Slebodnik wrote:
From 8b4746d7e2d5c53cbe5d29f1ce984a16aefb5a54 Mon Sep 17 00:00:00 2001 From: Lukas Slebodnik lslebodn@redhat.com Date: Tue, 15 Oct 2013 10:27:36 +0200 Subject: [PATCH 1/2] BUILD: Explicitly link libsss_ad.so with sasl libs
If openldap is not built with sasl support libsss_ad.so will not be linked with libsasl2 although sasl_client_init is called by function ad_sasl_initialize.
Makefile.am | 2 ++ configure.ac | 3 +-- src/external/sasl.m4 | 17 +++++++++++++++++ 3 files changed, 20 insertions(+), 2 deletions(-) create mode 100644 src/external/sasl.m4
Could someone push 1st patch also to the 1-11 branch?
LS
Sure, the patch applies and builds well. Pushed as 43975eaed128924e7999757eea1708487178d317
On Tue, 2013-10-15 at 14:39 +0200, Lukas Slebodnik wrote:
- 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)
Lukas, I think it is fine to consider SASL a hard dependency for SSSD, although we shouldn't force it to be cyrus-sasl, it is unlikely openldap will move any time to soon to an alternative library, so just pretending cyrus-sasl is the only one we care about is fine.
That said I agree just checking sasl is installed does not mean openldap has been built with it, so we need better detection for openldap.
Simo.
On (15/10/13 13:07), Simo Sorce wrote:
On Tue, 2013-10-15 at 14:39 +0200, Lukas Slebodnik wrote:
- 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)
Lukas, I think it is fine to consider SASL a hard dependency for SSSD, although we shouldn't force it to be cyrus-sasl, it is unlikely openldap will move any time to soon to an alternative library, so just pretending cyrus-sasl is the only one we care about is fine.
We have optional dependencies in sssd --robust mutexes --pac responder ...
SSSD can be built with openladap without sasl provider. In this case: ldap works ldap + krb5 works ipa doesn't work, error message is written in log file
This is a reason why I think SASL can be optional (but recommend) dependency in sssd. For me, it is overkill to add strong dependency on SASL only for small patch in ad-provider.
That said I agree just checking sasl is installed does not mean openldap has been built with it, so we need better detection for openldap.
I agree, but this is not problem.
LS
sssd-devel@lists.fedorahosted.org
-
Benjamin Franzke -
Jakub Hrozek -
Jakub Hrozek -
Lukas Slebodnik -
Simo Sorce -
Stephen Gallagher