On 04/26/2011 08:04 PM, Stephen Gallagher wrote: > On Tue, 2011-04-26 at 13:59 -0400, Stephen Gallagher wrote: >> On Tue, 2011-04-26 at 11:01 +0200, Jakub Hrozek wrote: >>> [PATCH 1/2] Require openssl-devel is libcrypto backend is selected >>> >>> I've changed the configure flags a little. The previous --enable-crypto >>> sounded like something you should enable to get the functionality >>> compiled in rather than a selection of alternative crypto back end. >>> >>> https://fedorahosted.org/sssd/ticket/844 >>> >>> [PATCH 2/2] BUILD.txt: Warn that all crypto features are implemented >>> >>> Note that the recommended default is NSS. >> >> >> Nack. >> >> Please add a warning in the configure script as well when using >> libcrypto. No one EVER reads the BUILD.txt. >>
Done. btw I've seen one user that read it (and saw a bug there) :-) >> Have you tested whether functionality works with libcrypto at all? What >> happens when cache_passwords = true? Do we store them plaintext or fail >> entirely? Is it a graceful failure? >> password caching works OK with libcrypto, it stores a hash in the cache file in the same way NSS does. The only feature that does not work at all with libcrypto is password obfuscation. >> I'm honestly not sure there's a good reason to allow the use of >> libcrypto at all at this point. I think we should consider disabling it >> (not removing it) until and unless someone else decides to maintain it. > > I agree, rotten code is bad. I would also like to get rid of the --enable-crypto switch one way or another, because the name if confusing. What about including the first two patches that fix the dependency detection and rename the configure switch and then include another patch (#3 in this case) to unconditionally build just NSS? This way, if someone steps up for libcrypto maintenance or contributes another crypto back end, we'll be able to just revert this one patch and get the fixed infrastructure? > Also, 'make check' fails if --with-crypto=libcrypto is used with: > Running suite(s): sss_crypto > 0%: Checks: 1, Failures: 1, Errors: 0 > ../src/tests/crypto-tests.c:69:F:sss crypto > tests:test_encrypt_decrypt:0: Failure 'ret != EOK' occured > FAIL: crypto-tests > Fixed, I've also discovered that HAVE_NSS is not #defined anywhere so the first testcase never ran. Both issues are fixed in patch #1. If you don't agree with the first two patches and prefer to just get rid of libcrypto, I'd still like to include these two changes.
From 84bb15c5f0447e4416085f611d16804a77b4a58e Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhro...@redhat.com>
Date: Sun, 24 Apr 2011 16:15:04 +0200
Subject: [PATCH 1/3] Require openssl-devel is libcrypto backend is selected
---
configure.ac | 8 ++++++++
src/conf_macros.m4 | 22 ++++++++++++++++++++++
src/external/crypto.m4 | 20 ++++++++------------
src/tests/crypto-tests.c | 19 ++++++++++++++-----
4 files changed, 52 insertions(+), 17 deletions(-)
diff --git a/configure.ac b/configure.ac
index 18aa823..f888466 100644
--- a/configure.ac
+++ b/configure.ac
@@ -95,6 +95,7 @@ WITH_SELINUX
WITH_NSCD
WITH_SEMANAGE
WITH_LIBNL
+WITH_CRYPTO
m4_include([src/external/pkg.m4])
m4_include([src/external/libpopt.m4])
@@ -175,6 +176,13 @@ if test x$HAVE_SYSTEMD_UNIT != x; then
AM_CHECK_SYSTEMD
fi
+if test x$cryptolib = xnss; then
+ AM_CHECK_NSS
+fi
+if test x$cryptolib = xlibcrypto; then
+ AM_CHECK_LIBCRYPTO
+fi
+
AC_CHECK_HEADERS([sys/inotify.h])
AC_CHECK_HEADERS([sasl/sasl.h],,AC_MSG_ERROR([Could not find SASL headers]))
diff --git a/src/conf_macros.m4 b/src/conf_macros.m4
index 273a527..31048d3 100644
--- a/src/conf_macros.m4
+++ b/src/conf_macros.m4
@@ -295,3 +295,25 @@ AC_DEFUN([WITH_LIBNL],
fi
])
+AC_DEFUN([WITH_CRYPTO],
+ [ AC_ARG_WITH([crypto],
+ [AC_HELP_STRING([--with-crypto=CRYPTO_LIB],
+ [The cryptographic library to use (nss|libcrypto). The default is nss.]
+ )
+ ],
+ [],
+ with_crypto=nss
+ )
+
+ cryptolib=""
+ if test x"$with_crypto" != x; then
+ if test x"$with_crypto" = xnss || \
+ test x"$with_crypto" = xlibcrypto; then
+ cryptolib="$with_crypto";
+ else
+ AC_MSG_ERROR([Illegal value -$with_crypto- for option --with-crypto])
+ fi
+ fi
+ AM_CONDITIONAL([HAVE_NSS], [test x"$cryptolib" = xnss])
+ AM_CONDITIONAL([HAVE_LIBCRYPTO], [test x"$cryptolib" = xlibcrypto])
+ ])
diff --git a/src/external/crypto.m4 b/src/external/crypto.m4
index d1bcf40..19a064d 100644
--- a/src/external/crypto.m4
+++ b/src/external/crypto.m4
@@ -1,13 +1,9 @@
-AC_ARG_ENABLE(crypto,
- [ --enable-crypto Use OpenSSL crypto instead of NSS],
- [CRYPTO="$enableval"],
- [CRYPTO="no"]
-)
+AC_DEFUN([AM_CHECK_NSS],
+ [PKG_CHECK_MODULES([NSS],[nss])
+ AC_DEFINE_UNQUOTED(HAVE_NSS, 1, [Build with NSS crypto back end])
+])
-if test x$CRYPTO != xyes; then
- PKG_CHECK_MODULES([NSS],[nss],[have_nss=1],[have_nss=])
-else
- PKG_CHECK_MODULES([CRYPTO],[libcrypto],[have_crypto=1],[have_crypto=])
-fi
-AM_CONDITIONAL([HAVE_NSS], [test x$have_nss != x])
-AM_CONDITIONAL([HAVE_CRYPTO], [test x$have_crypto != x])
+AC_DEFUN([AM_CHECK_LIBCRYPTO],
+ [PKG_CHECK_MODULES([CRYPTO],[libcrypto])
+ AC_DEFINE_UNQUOTED(HAVE_LIBCRYPTO, 1, [Build with libcrypt crypto back end])
+])
diff --git a/src/tests/crypto-tests.c b/src/tests/crypto-tests.c
index f802c11..286bc23 100644
--- a/src/tests/crypto-tests.c
+++ b/src/tests/crypto-tests.c
@@ -55,9 +55,18 @@ START_TEST(test_encrypt_decrypt)
"", /* empty */
NULL}; /* sentinel */
int i;
- char *obfpwd;
- char *ctpwd;
+ char *obfpwd = NULL;
+ char *ctpwd = NULL;
int ret;
+ int expected;
+
+#ifdef HAVE_NSS
+ expected = EOK;
+#elif HAVE_LIBCRYPTO
+ expected = ENOSYS;
+#else
+#error Unknown crypto back end
+#endif
test_ctx = talloc_new(NULL);
fail_if(test_ctx == NULL);
@@ -66,12 +75,12 @@ START_TEST(test_encrypt_decrypt)
for (i=0; password[i]; i++) {
ret = sss_password_encrypt(test_ctx, password[i], strlen(password[i])+1,
AES_256, &obfpwd);
- fail_if(ret != EOK);
+ fail_if(ret != expected);
ret = sss_password_decrypt(test_ctx, obfpwd, &ctpwd);
- fail_if(ret != EOK);
+ fail_if(ret != expected);
- fail_if(strcmp(password[i], ctpwd) != 0);
+ fail_if(ctpwd && strcmp(password[i], ctpwd) != 0);
talloc_free(obfpwd);
talloc_free(ctpwd);
--
1.7.4.4
From 44854c8dd876fb491124b35f9e3ccea4c8e78a6c Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhro...@redhat.com>
Date: Sun, 24 Apr 2011 16:32:12 +0200
Subject: [PATCH 2/3] Warn that some crypto features are implemented in NSS
only
---
BUILD.txt | 7 +++++++
configure.ac | 2 ++
2 files changed, 9 insertions(+), 0 deletions(-)
diff --git a/BUILD.txt b/BUILD.txt
index 8dd3694..5e269c5 100644
--- a/BUILD.txt
+++ b/BUILD.txt
@@ -33,6 +33,13 @@ ding-libs are available in Fedora 14 and later version:
yum install libcollection-devel libdhash-devel libini_config-devel \
libpath_utils-devel libref_array-devel
+Some features, notably password caching, require the presence of a crypto
+library. The default, tested by SSSD upstream, is Mozilla NSS. An alternative
+crypto library can be selected during configure time using the --with-crypto
+switch. Please note that alternative crypto back ends may not provide all
+features - as of this writing, password obfuscation is only supported with the
+NSS back end.
+
How to build:
~~~~~~~~~~~~~
From the root of the source, run:
diff --git a/configure.ac b/configure.ac
index f888466..d71578d 100644
--- a/configure.ac
+++ b/configure.ac
@@ -181,6 +181,8 @@ if test x$cryptolib = xnss; then
fi
if test x$cryptolib = xlibcrypto; then
AM_CHECK_LIBCRYPTO
+ AC_MSG_WARN([libcrypto back end does not implement all the crypto features, \
+notably password obfuscation. Using the NSS backend is recommended.])
fi
AC_CHECK_HEADERS([sys/inotify.h])
--
1.7.4.4
From 90a55d9088b2e1007fbfcef8c527454b8f0c0d5f Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhro...@redhat.com>
Date: Wed, 27 Apr 2011 11:35:20 +0200
Subject: [PATCH 3/3] Disable libcrypto code
---
configure.ac | 11 ++---------
1 files changed, 2 insertions(+), 9 deletions(-)
diff --git a/configure.ac b/configure.ac
index d71578d..ba1c03b 100644
--- a/configure.ac
+++ b/configure.ac
@@ -95,7 +95,6 @@ WITH_SELINUX
WITH_NSCD
WITH_SEMANAGE
WITH_LIBNL
-WITH_CRYPTO
m4_include([src/external/pkg.m4])
m4_include([src/external/libpopt.m4])
@@ -176,14 +175,8 @@ if test x$HAVE_SYSTEMD_UNIT != x; then
AM_CHECK_SYSTEMD
fi
-if test x$cryptolib = xnss; then
- AM_CHECK_NSS
-fi
-if test x$cryptolib = xlibcrypto; then
- AM_CHECK_LIBCRYPTO
- AC_MSG_WARN([libcrypto back end does not implement all the crypto features, \
-notably password obfuscation. Using the NSS backend is recommended.])
-fi
+AM_CHECK_NSS
+AM_CONDITIONAL([HAVE_NSS], [test x"$NSS_CFLAGS" != x])
AC_CHECK_HEADERS([sys/inotify.h])
--
1.7.4.4
signature.asc
Description: OpenPGP digital signature
_______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
