crypto_sha512crypt.c is a clone of nss_sha512crypt.c with the exception that all usage of NSS and related libraries has been switched to libcrypto. I renamed nss_sha512crypt.h to sha512crypt.h since it is common to both crypto_sha512crypt.c and nss_sha512crypt.c. Note that the random number generator is not seeded manually and thus relies on seeding done automatically by libcrypto. On some systems without /dev/urandom seeding may not be performed. See http://www.openssl.org/docs/crypto/RAND_add.html. Signed-off-by: George McCollister georgem@novatech-llc.com --- server/Makefile.am | 28 ++- server/configure.ac | 14 +- server/db/sysdb_ops.c | 2 +- server/responder/pam/pam_LOCAL_domain.c | 2 +- server/util/crypto_sha512crypt.c | 380 +++++++++++++++++++++++++++++++ server/util/nss_sha512crypt.h | 4 - server/util/sha512crypt.h | 4 + 7 files changed, 423 insertions(+), 11 deletions(-) create mode 100644 server/util/crypto_sha512crypt.c delete mode 100644 server/util/nss_sha512crypt.h create mode 100644 server/util/sha512crypt.h
On Fri, Jan 29, 2010 at 04:45:39PM -0600, George McCollister wrote:
crypto_sha512crypt.c is a clone of nss_sha512crypt.c with the exception that all usage of NSS and related libraries has been switched to libcrypto. I renamed nss_sha512crypt.h to sha512crypt.h since it is common to both crypto_sha512crypt.c and nss_sha512crypt.c. Note that the random number generator is not seeded manually and thus relies on seeding done automatically by libcrypto. On some systems without /dev/urandom seeding may not be performed. See http://www.openssl.org/docs/crypto/RAND_add.html. Signed-off-by: George McCollister georgem@novatech-llc.com
Hi George,
thank you for this contribution. I really like the idea of making the crypto library configurable. Nevertheless I have some comments about the patch.
- please use the output of 'git format-patch' or similar so that the commit message and your name and email is include in the patch
- Makefile.am: I would prefer to have one if-else block like
if HAVE_NSS SSS_CRYPT_CFLAGS = $(NSS_CFLAGS) SSS_CRYPT_LIBS = $(NSS_LIBS) else SSS_CRYPT_CFLAGS = $(CRYPTO_CFLAGS) SSS_CRYPT_LIBS = $(CRYPTO_LIBS) endfi
and use SSS_CRYPT_CFLAGS and SSS_CRYPT_LIBS where needed. This way we only need to touch this block if we want to add support for other crypto libraries.
- configure.ac: can you move your changes to say external/crypto.m4 and only add the m4_include statement to configure.ac?
- instead of cloning nss_sha512crypt.c I would prefer to introduce generic replacement function like we do for missing LDAP or Kerberos calls (see util/sss_ldap.c or util/sss_krb5.c). This would mean we have a file like util/sss_crypt.c where calls like sss_crypt_init(), sss_hash_ctx_init(), sss_hash_begin(), sss_hash_update() etc. nss_sha512crypt.c would be renamed to sss_sha512crypt.c and the NSS calls would to substituted be the replacement calls.
It might be a bit tricky to define a suitable set of replacement calls so I would agree to add crypto_sha512crypt.c in the current state and add an enhancement bug to trac to create the replacement calls.
- it would be nice to have a test to see if the calculated hashes are the ones we are expecting and if they are compatible to glibc hashes. This is not strictly related to your patch so I'm volunteering to add the test if you do not find the time to do it.
bye, Sumit
server/Makefile.am | 28 ++- server/configure.ac | 14 +- server/db/sysdb_ops.c | 2 +- server/responder/pam/pam_LOCAL_domain.c | 2 +- server/util/crypto_sha512crypt.c | 380 +++++++++++++++++++++++++++++++ server/util/nss_sha512crypt.h | 4 - server/util/sha512crypt.h | 4 + 7 files changed, 423 insertions(+), 11 deletions(-) create mode 100644 server/util/crypto_sha512crypt.c delete mode 100644 server/util/nss_sha512crypt.h create mode 100644 server/util/sha512crypt.h
diff --git a/server/Makefile.am b/server/Makefile.am index 7ba7ffa..0b96cca 100644 --- a/server/Makefile.am +++ b/server/Makefile.am @@ -102,12 +102,22 @@ endif
noinst_LTLIBRARIES = \ libsss_crypt.la
+if HAVE_NSS libsss_crypt_la_SOURCES = \ util/nss_sha512crypt.c libsss_crypt_la_CPPFLAGS = \ $(NSS_CFLAGS) libsss_crypt_la_LIBADD = \ $(NSS_LIBS) +else +libsss_crypt_la_SOURCES = \
- util/crypto_sha512crypt.c
+libsss_crypt_la_CPPFLAGS = \
- $(CRYPTO_CFLAGS)
+libsss_crypt_la_LIBADD = \
- $(CRYPTO_LIBS)
+endif
if BUILD_PYTHON_BINDINGS pyexec_LTLIBRARIES = \ @@ -243,8 +253,13 @@ SSSD_LIBS = \ $(COLLECTION_LIBS) \ $(DHASH_LIBS) \ $(REPLACE_LIBS) \
- libsss_crypt.la \
- $(NSS_LIBS)
- libsss_crypt.la
+if HAVE_NSS
- SSSD_LIBS += $(NSS_LIBS)
+else
- SSSD_LIBS += $(CRYPTO_LIBS)
+endif
PYTHON_BINDINGS_LIBS = \ $(TALLOC_LIBS) \ @@ -254,9 +269,14 @@ PYTHON_BINDINGS_LIBS = \ $(DBUS_LIBS) \ $(REPLACE_LIBS) \ $(PCRE_LIBS) \
- $(NSS_LIBS) \ libsss_crypt.la
+if HAVE_NSS
- PYTHON_BINDINGS_LIBS += $(NSS_LIBS)
+else
- PYTHON_BINDINGS_LIBS += $(CRYPTO_LIBS)
+endif
TOOLS_LIBS = \ $(TALLOC_LIBS) \ $(TEVENT_LIBS) \ @@ -276,7 +296,7 @@ endif
dist_noinst_HEADERS = \ monitor/monitor.h \
- util/nss_sha512crypt.h \
- util/sha512crypt.h \ util/dlinklist.h \ util/util.h \ util/strtonum.h \
diff --git a/server/configure.ac b/server/configure.ac index 75c07da..d1cce47 100644 --- a/server/configure.ac +++ b/server/configure.ac @@ -77,6 +77,12 @@ m4_include([external/python.m4]) m4_include([external/selinux.m4]) m4_include([util/signal.m4])
+AC_ARG_ENABLE(crypto,
- [ --enable-crypto Use OpenSSL crypto instead of NSS],
- [CRYPTO="$enableval"],
- [CRYPTO="no"]
+)
PKG_CHECK_MODULES([DBUS],[dbus-1]) dnl if test -n "`$PKG_CONFIG --modversion dbus-1 | grep '^0.'`" ; then if ! $PKG_CONFIG --atleast-version 1.0.0 dbus-1; then @@ -93,7 +99,13 @@ if test x$has_dbus != xno; then LDFLAGS="$SAFE_LDFLAGS" fi
-PKG_CHECK_MODULES([NSS],[nss]) +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])
if test x$HAVE_MANPAGES != x; then CHECK_XML_TOOLS diff --git a/server/db/sysdb_ops.c b/server/db/sysdb_ops.c index 36b5867..38686ae 100644 --- a/server/db/sysdb_ops.c +++ b/server/db/sysdb_ops.c @@ -21,7 +21,7 @@
#include "util/util.h" #include "db/sysdb_private.h" -#include "util/nss_sha512crypt.h" +#include "util/sha512crypt.h" #include <time.h>
static int add_string(struct ldb_message *msg, int flags, diff --git a/server/responder/pam/pam_LOCAL_domain.c b/server/responder/pam/pam_LOCAL_domain.c index 9d3738c..6fa42a4 100644 --- a/server/responder/pam/pam_LOCAL_domain.c +++ b/server/responder/pam/pam_LOCAL_domain.c @@ -24,7 +24,7 @@
#include "util/util.h" #include "db/sysdb.h" -#include "util/nss_sha512crypt.h" +#include "util/sha512crypt.h" #include "providers/data_provider.h" #include "responder/pam/pamsrv.h"
diff --git a/server/util/crypto_sha512crypt.c b/server/util/crypto_sha512crypt.c new file mode 100644 index 0000000..6acd654 --- /dev/null +++ b/server/util/crypto_sha512crypt.c @@ -0,0 +1,380 @@ +/* This file is based on the work of Ulrich Drepper
- (http://people.redhat.com/drepper/SHA-crypt.txt). I have replaced the
- included SHA512 implementation by calls to NSS
- Sumit Bose sbose@redhat.com
- */
+/* SHA512-based Unix crypt implementation.
- Released into the Public Domain by Ulrich Drepper drepper@redhat.com. */
+#define _GNU_SOURCE +#include <endian.h> +#include <errno.h> +#include <limits.h> +#include <stdbool.h> +#include <stdint.h> +#include <stdio.h> +#include <stdlib.h> +#include <string.h> +#include <sys/param.h> +#include <sys/types.h>
+#include "util/util.h"
+#include <openssl/evp.h> +#include <openssl/rand.h>
+/* Define our magic string to mark salt for SHA512 "encryption" replacement. */ +const char sha512_salt_prefix[] = "$6$"; +#define SALT_PREF_SIZE (sizeof(sha512_salt_prefix) - 1)
+/* Prefix for optional rounds specification. */ +const char sha512_rounds_prefix[] = "rounds="; +#define ROUNDS_SIZE (sizeof(sha512_rounds_prefix) - 1)
+#define SALT_LEN_MAX 16 +#define ROUNDS_DEFAULT 5000 +#define ROUNDS_MIN 1000 +#define ROUNDS_MAX 999999999
+/* Table with characters for base64 transformation. */ +const char b64t[64] =
- "./0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz";
+/* base64 conversion function */ +static inline void b64_from_24bit(char **dest, size_t *len, size_t n,
uint8_t b2, uint8_t b1, uint8_t b0)
+{
- uint32_t w;
- size_t i;
- if (*len < n) n = *len;
- w = (b2 << 16) | (b1 << 8) | b0;
- for (i = 0; i < n; i++) {
(*dest)[i] = b64t[w & 0x3f];
w >>= 6;
- }
- *len -= i;
- *dest += i;
+}
+#define PTR_2_INT(x) ((x) - ((__typeof__ (x)) NULL)) +#define ALIGN64 __alignof__(uint64_t)
+static int sha512_crypt_r(const char *key,
const char *salt,
char *buffer, size_t buflen)
+{
- unsigned char temp_result[64] __attribute__((__aligned__(ALIGN64)));
- unsigned char alt_result[64] __attribute__((__aligned__(ALIGN64)));
- size_t rounds = ROUNDS_DEFAULT;
- bool rounds_custom = false;
- EVP_MD_CTX alt_ctx;
- EVP_MD_CTX ctx;
- size_t salt_len;
- size_t key_len;
- size_t cnt;
- char *copied_salt = NULL;
- char *copied_key = NULL;
- char *p_bytes = NULL;
- char *s_bytes = NULL;
- int p1, p2, p3, pt, n;
- unsigned int part;
- char *cp, *tmp;
- int ret;
- /* Find beginning of salt string. The prefix should normally always be
* present. Just in case it is not. */
- if (strncmp(salt, sha512_salt_prefix, SALT_PREF_SIZE) == 0) {
/* Skip salt prefix. */
salt += SALT_PREF_SIZE;
- }
- if (strncmp(salt, sha512_rounds_prefix, ROUNDS_SIZE) == 0) {
unsigned long int srounds;
const char *num;
char *endp;
num = salt + ROUNDS_SIZE;
srounds = strtoul(num, &endp, 10);
if (*endp == '$') {
salt = endp + 1;
if (srounds < ROUNDS_MIN) srounds = ROUNDS_MIN;
if (srounds > ROUNDS_MAX) srounds = ROUNDS_MAX;
rounds = srounds;
rounds_custom = true;
}
- }
- salt_len = MIN(strcspn(salt, "$"), SALT_LEN_MAX);
- key_len = strlen(key);
- if ((PTR_2_INT(key) % ALIGN64) != 0) {
tmp = (char *)alloca(key_len + ALIGN64);
key = copied_key = memcpy(tmp + ALIGN64 - PTR_2_INT(tmp) % ALIGN64, key, key_len);
- }
- if (PTR_2_INT(salt) % ALIGN64 != 0) {
tmp = (char *)alloca(salt_len + ALIGN64);
salt = copied_salt = memcpy(tmp + ALIGN64 - PTR_2_INT(tmp) % ALIGN64, salt, salt_len);
- }
- EVP_MD_CTX_init(&ctx);
- EVP_MD_CTX_init(&alt_ctx);
- /* Prepare for the real work. */
- if (!EVP_DigestInit_ex(&ctx, EVP_sha512(), NULL)) {
ret = EIO;
goto done;
- }
- /* Add the key string. */
- EVP_DigestUpdate(&ctx, (const unsigned char *)key, key_len);
- /* The last part is the salt string. This must be at most 16
* characters and it ends at the first `$' character (for
* compatibility with existing implementations). */
- EVP_DigestUpdate(&ctx, (const unsigned char *)salt, salt_len);
- /* Compute alternate SHA512 sum with input KEY, SALT, and KEY.
* The final result will be added to the first context. */
- if (!EVP_DigestInit_ex(&alt_ctx, EVP_sha512(), NULL)) {
ret = EIO;
goto done;
- }
- /* Add key. */
- EVP_DigestUpdate(&alt_ctx, (const unsigned char *)key, key_len);
- /* Add salt. */
- EVP_DigestUpdate(&alt_ctx, (const unsigned char *)salt, salt_len);
- /* Add key again. */
- EVP_DigestUpdate(&alt_ctx, (const unsigned char *)key, key_len);
- /* Now get result of this (64 bytes) and add it to the other context. */
- EVP_DigestFinal_ex(&alt_ctx, alt_result, &part);
- /* Add for any character in the key one byte of the alternate sum. */
- for (cnt = key_len; cnt > 64; cnt -= 64) {
EVP_DigestUpdate(&ctx, alt_result, 64);
- }
- EVP_DigestUpdate(&ctx, alt_result, cnt);
- /* Take the binary representation of the length of the key and for every
* 1 add the alternate sum, for every 0 the key. */
- for (cnt = key_len; cnt > 0; cnt >>= 1) {
if ((cnt & 1) != 0) {
EVP_DigestUpdate(&ctx, alt_result, 64);
} else {
EVP_DigestUpdate(&ctx, (const unsigned char *)key, key_len);
}
- }
- /* Create intermediate result. */
- EVP_DigestFinal_ex(&ctx, alt_result, &part);
- /* Start computation of P byte sequence. */
- if (!EVP_DigestInit_ex(&alt_ctx, EVP_sha512(), NULL)) {
ret = EIO;
goto done;
- }
- /* For every character in the password add the entire password. */
- for (cnt = 0; cnt < key_len; cnt++) {
EVP_DigestUpdate(&alt_ctx, (const unsigned char *)key, key_len);
- }
- /* Finish the digest. */
- EVP_DigestFinal_ex(&alt_ctx, temp_result, &part);
- /* Create byte sequence P. */
- cp = p_bytes = alloca(key_len);
- for (cnt = key_len; cnt >= 64; cnt -= 64) {
cp = mempcpy(cp, temp_result, 64);
- }
- memcpy(cp, temp_result, cnt);
- /* Start computation of S byte sequence. */
- if (!EVP_DigestInit_ex(&alt_ctx, EVP_sha512(), NULL)) {
ret = EIO;
goto done;
- }
- /* For every character in the password add the entire salt. */
- for (cnt = 0; cnt < 16 + alt_result[0]; cnt++) {
EVP_DigestUpdate(&alt_ctx, (const unsigned char *)salt, salt_len);
- }
- /* Finish the digest. */
- EVP_DigestFinal_ex(&alt_ctx, temp_result, &part);
- /* Create byte sequence S. */
- cp = s_bytes = alloca(salt_len);
- for (cnt = salt_len; cnt >= 64; cnt -= 64) {
cp = mempcpy(cp, temp_result, 64);
- }
- memcpy(cp, temp_result, cnt);
- /* Repeatedly run the collected hash value through SHA512 to burn CPU cycles. */
- for (cnt = 0; cnt < rounds; cnt++) {
if (!EVP_DigestInit_ex(&ctx, EVP_sha512(), NULL)) {
ret = EIO;
goto done;
}
/* Add key or last result. */
if ((cnt & 1) != 0) {
EVP_DigestUpdate(&ctx, (const unsigned char *)p_bytes, key_len);
} else {
EVP_DigestUpdate(&ctx, alt_result, 64);
}
/* Add salt for numbers not divisible by 3. */
if (cnt % 3 != 0) {
EVP_DigestUpdate(&ctx, (const unsigned char *)s_bytes, salt_len);
}
/* Add key for numbers not divisible by 7. */
if (cnt % 7 != 0) {
EVP_DigestUpdate(&ctx, (const unsigned char *)p_bytes, key_len);
}
/* Add key or last result. */
if ((cnt & 1) != 0) {
EVP_DigestUpdate(&ctx, alt_result, 64);
} else {
EVP_DigestUpdate(&ctx, (const unsigned char *)p_bytes, key_len);
}
/* Create intermediate result. */
EVP_DigestFinal_ex(&ctx, alt_result, &part);
- }
- /* Now we can construct the result string.
* It consists of three parts. */
- if (buflen <= SALT_PREF_SIZE) {
ret = ERANGE;
goto done;
- }
- cp = __stpncpy(buffer, sha512_salt_prefix, SALT_PREF_SIZE);
- buflen -= SALT_PREF_SIZE;
- if (rounds_custom) {
n = snprintf(cp, buflen, "%s%zu$",
sha512_rounds_prefix, rounds);
if (n < 0 || n >= buflen) {
ret = ERANGE;
goto done;
}
cp += n;
buflen -= n;
- }
- if (buflen <= salt_len + 1) {
ret = ERANGE;
goto done;
- }
- cp = __stpncpy(cp, salt, salt_len);
- *cp++ = '$';
- buflen -= salt_len + 1;
- /* fuzzyfill the base 64 string */
- p1 = 0;
- p2 = 21;
- p3 = 42;
- for (n = 0; n < 21; n++) {
b64_from_24bit(&cp, &buflen, 4, alt_result[p1], alt_result[p2], alt_result[p3]);
if (buflen == 0) {
ret = ERANGE;
goto done;
}
pt = p1;
p1 = p2 + 1;
p2 = p3 + 1;
p3 = pt + 1;
- }
- /* 64th and last byte */
- b64_from_24bit(&cp, &buflen, 2, 0, 0, alt_result[p3]);
- if (buflen == 0) {
ret = ERANGE;
goto done;
- }
- *cp = '\0';
- ret = EOK;
+done:
- /* Clear the buffer for the intermediate result so that people attaching
* to processes or reading core dumps cannot get any information. We do it
* in this way to clear correct_words[] inside the SHA512 implementation
* as well. */
- EVP_MD_CTX_cleanup(&ctx);
- EVP_MD_CTX_cleanup(&alt_ctx);
- if (p_bytes) memset(p_bytes, '\0', key_len);
- if (s_bytes) memset(s_bytes, '\0', salt_len);
- if (copied_key) memset(copied_key, '\0', key_len);
- if (copied_salt) memset(copied_salt, '\0', salt_len);
- memset(temp_result, '\0', sizeof(temp_result));
- return ret;
+}
+int s3crypt_sha512(TALLOC_CTX *memctx,
const char *key, const char *salt, char **_hash)
+{
- char *hash;
- int hlen = (sizeof (sha512_salt_prefix) - 1
+ sizeof (sha512_rounds_prefix) + 9 + 1
+ strlen (salt) + 1 + 86 + 1);
- int ret;
- hash = talloc_size(memctx, hlen);
- if (!hash) return ENOMEM;
- ret = sha512_crypt_r(key, salt, hash, hlen);
- if (ret) return ret;
- *_hash = hash;
- return ret;
+}
+#define SALT_RAND_LEN 12
+int s3crypt_gen_salt(TALLOC_CTX *memctx, char **_salt) +{
- uint8_t rb[SALT_RAND_LEN];
- char *salt, *cp;
- size_t slen;
- int ret;
- salt = talloc_size(memctx, SALT_LEN_MAX + 1);
- if (!salt) {
return ENOMEM;
- }
- ret = RAND_bytes(rb, SALT_RAND_LEN);
- if (ret == 0) {
return EIO;
- }
- slen = SALT_LEN_MAX;
- cp = salt;
- b64_from_24bit(&cp, &slen, 4, rb[0], rb[1], rb[2]);
- b64_from_24bit(&cp, &slen, 4, rb[3], rb[4], rb[5]);
- b64_from_24bit(&cp, &slen, 4, rb[6], rb[7], rb[8]);
- b64_from_24bit(&cp, &slen, 4, rb[9], rb[10], rb[11]);
- *cp = '\0';
- *_salt = salt;
- return EOK;
+}
diff --git a/server/util/nss_sha512crypt.h b/server/util/nss_sha512crypt.h deleted file mode 100644 index 5512c5d..0000000 --- a/server/util/nss_sha512crypt.h +++ /dev/null @@ -1,4 +0,0 @@
-int s3crypt_sha512(TALLOC_CTX *mmectx,
const char *key, const char *salt, char **_hash);
-int s3crypt_gen_salt(TALLOC_CTX *memctx, char **_salt); diff --git a/server/util/sha512crypt.h b/server/util/sha512crypt.h new file mode 100644 index 0000000..5512c5d --- /dev/null +++ b/server/util/sha512crypt.h @@ -0,0 +1,4 @@
+int s3crypt_sha512(TALLOC_CTX *mmectx,
const char *key, const char *salt, char **_hash);
+int s3crypt_gen_salt(TALLOC_CTX *memctx, char **_salt);
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
Sumit,
I've attached a new patch made with git format-patch instead of git send-email. I have made your suggested changes to Makefile.am. I moved my changes out of server/configure.ac and into server/external/crypto.m4.
I didn't introduce any generic replacement functions for sss_crypt although I agree that would be a good idea. I didn't make an automated test to check that the hashes are correct although I did run a build using libcrypto with an LDAP cache that had been created with by a version using nss and I was able to login using the cached password (I'm not certain if that would run through this code).
Let me know if there is anything else you want me to change or test.
Thanks, George McCollister
On 02/01/2010 04:45 AM, Sumit Bose wrote:
On Fri, Jan 29, 2010 at 04:45:39PM -0600, George McCollister wrote:
crypto_sha512crypt.c is a clone of nss_sha512crypt.c with the exception that all usage of NSS and related libraries has been switched to libcrypto. I renamed nss_sha512crypt.h to sha512crypt.h since it is common to both crypto_sha512crypt.c and nss_sha512crypt.c. Note that the random number generator is not seeded manually and thus relies on seeding done automatically by libcrypto. On some systems without /dev/urandom seeding may not be performed. See http://www.openssl.org/docs/crypto/RAND_add.html. Signed-off-by: George McCollister georgem@novatech-llc.com
Hi George,
thank you for this contribution. I really like the idea of making the crypto library configurable. Nevertheless I have some comments about the patch.
please use the output of 'git format-patch' or similar so that the commit message and your name and email is include in the patch
Makefile.am: I would prefer to have one if-else block like
if HAVE_NSS SSS_CRYPT_CFLAGS = $(NSS_CFLAGS) SSS_CRYPT_LIBS = $(NSS_LIBS) else SSS_CRYPT_CFLAGS = $(CRYPTO_CFLAGS) SSS_CRYPT_LIBS = $(CRYPTO_LIBS) endfi
and use SSS_CRYPT_CFLAGS and SSS_CRYPT_LIBS where needed. This way we only need to touch this block if we want to add support for other crypto libraries.
configure.ac: can you move your changes to say external/crypto.m4 and only add the m4_include statement to configure.ac?
instead of cloning nss_sha512crypt.c I would prefer to introduce generic replacement function like we do for missing LDAP or Kerberos calls (see util/sss_ldap.c or util/sss_krb5.c). This would mean we have a file like util/sss_crypt.c where calls like sss_crypt_init(), sss_hash_ctx_init(), sss_hash_begin(), sss_hash_update() etc. nss_sha512crypt.c would be renamed to sss_sha512crypt.c and the NSS calls would to substituted be the replacement calls.
It might be a bit tricky to define a suitable set of replacement calls so I would agree to add crypto_sha512crypt.c in the current state and add an enhancement bug to trac to create the replacement calls.
it would be nice to have a test to see if the calculated hashes are the ones we are expecting and if they are compatible to glibc hashes. This is not strictly related to your patch so I'm volunteering to add the test if you do not find the time to do it.
bye, Sumit
On Wed, Feb 03, 2010 at 11:42:13AM -0600, George McCollister wrote:
Sumit,
I've attached a new patch made with git format-patch instead of git send-email. I have made your suggested changes to Makefile.am. I moved my changes out of server/configure.ac and into server/external/crypto.m4.
I didn't introduce any generic replacement functions for sss_crypt although I agree that would be a good idea. I didn't make an automated test to check that the hashes are correct although I did run a build using libcrypto with an LDAP cache that had been created with by a version using nss and I was able to login using the cached password (I'm not certain if that would run through this code).
ok, I will add a test with a later patch.
Let me know if there is anything else you want me to change or test.
just some minor nitpicks:
- please reformat the commit message to use multiple lines not longer than 80 characters
- can you rename SSS_SOURCES to SSS_CRYPT_SOURCES
- can you update the comment at the beginning of crypto_sha512crypt.c so that it is clear that this file uses calls from libcrypto and not from libnss3
- please fix the following whitespace issues:
Applying: Added option to use libcrypto instead of NSS. /tmp/sssd/.git/rebase-apply/patch:117: trailing whitespace. [ --enable-crypto Use OpenSSL crypto instead of NSS], /tmp/sssd/.git/rebase-apply/patch:271: trailing whitespace.
/tmp/sssd/.git/rebase-apply/patch:292: trailing whitespace. * The final result will be added to the first context. */ /tmp/sssd/.git/rebase-apply/patch:147: new blank line at EOF. + warning: 4 lines add whitespace errors.
Otherwise I'm fine.
bye, Sumit
Thanks, George McCollister
On 02/01/2010 04:45 AM, Sumit Bose wrote:
On Fri, Jan 29, 2010 at 04:45:39PM -0600, George McCollister wrote:
crypto_sha512crypt.c is a clone of nss_sha512crypt.c with the exception that all usage of NSS and related libraries has been switched to libcrypto. I renamed nss_sha512crypt.h to sha512crypt.h since it is common to both crypto_sha512crypt.c and nss_sha512crypt.c. Note that the random number generator is not seeded manually and thus relies on seeding done automatically by libcrypto. On some systems without /dev/urandom seeding may not be performed. See http://www.openssl.org/docs/crypto/RAND_add.html. Signed-off-by: George McCollister georgem@novatech-llc.com
Hi George,
thank you for this contribution. I really like the idea of making the crypto library configurable. Nevertheless I have some comments about the patch.
please use the output of 'git format-patch' or similar so that the commit message and your name and email is include in the patch
Makefile.am: I would prefer to have one if-else block like
if HAVE_NSS SSS_CRYPT_CFLAGS = $(NSS_CFLAGS) SSS_CRYPT_LIBS = $(NSS_LIBS) else SSS_CRYPT_CFLAGS = $(CRYPTO_CFLAGS) SSS_CRYPT_LIBS = $(CRYPTO_LIBS) endfi
and use SSS_CRYPT_CFLAGS and SSS_CRYPT_LIBS where needed. This way we only need to touch this block if we want to add support for other crypto libraries.
configure.ac: can you move your changes to say external/crypto.m4 and only add the m4_include statement to configure.ac?
instead of cloning nss_sha512crypt.c I would prefer to introduce generic replacement function like we do for missing LDAP or Kerberos calls (see util/sss_ldap.c or util/sss_krb5.c). This would mean we have a file like util/sss_crypt.c where calls like sss_crypt_init(), sss_hash_ctx_init(), sss_hash_begin(), sss_hash_update() etc. nss_sha512crypt.c would be renamed to sss_sha512crypt.c and the NSS calls would to substituted be the replacement calls.
It might be a bit tricky to define a suitable set of replacement calls so I would agree to add crypto_sha512crypt.c in the current state and add an enhancement bug to trac to create the replacement calls.
it would be nice to have a test to see if the calculated hashes are the ones we are expecting and if they are compatible to glibc hashes. This is not strictly related to your patch so I'm volunteering to add the test if you do not find the time to do it.
bye, Sumit
Sumit,
I've attached a new version of the patch with the changes you suggested.
just some minor nitpicks:
- please reformat the commit message to use multiple lines not longer than 80 characters
Fixed.
- can you rename SSS_SOURCES to SSS_CRYPT_SOURCES
Thats what I meant to do in the first place. Fixed.
- can you update the comment at the beginning of crypto_sha512crypt.c so that it is clear that this file uses calls from libcrypto and not from libnss3
Done.
- please fix the following whitespace issues:
Applying: Added option to use libcrypto instead of NSS. /tmp/sssd/.git/rebase-apply/patch:117: trailing whitespace. [ --enable-crypto Use OpenSSL crypto instead of NSS], /tmp/sssd/.git/rebase-apply/patch:271: trailing whitespace.
/tmp/sssd/.git/rebase-apply/patch:292: trailing whitespace. * The final result will be added to the first context. */ /tmp/sssd/.git/rebase-apply/patch:147: new blank line at EOF.
warning: 4 lines add whitespace errors.
I ran the last version of the patch through git apply and I only received 3 errors. I fixed everything I could find and now git apply is giving me no errors.
Cheers, George McCollister
On Thu, Feb 04, 2010 at 11:37:57AM -0600, George McCollister wrote:
Sumit,
I've attached a new version of the patch with the changes you suggested.
just some minor nitpicks:
- please reformat the commit message to use multiple lines not longer than 80 characters
Fixed.
- can you rename SSS_SOURCES to SSS_CRYPT_SOURCES
Thats what I meant to do in the first place. Fixed.
- can you update the comment at the beginning of crypto_sha512crypt.c so that it is clear that this file uses calls from libcrypto and not from libnss3
Done.
- please fix the following whitespace issues:
Applying: Added option to use libcrypto instead of NSS. /tmp/sssd/.git/rebase-apply/patch:117: trailing whitespace. [ --enable-crypto Use OpenSSL crypto instead of NSS], /tmp/sssd/.git/rebase-apply/patch:271: trailing whitespace.
/tmp/sssd/.git/rebase-apply/patch:292: trailing whitespace. * The final result will be added to the first context. */ /tmp/sssd/.git/rebase-apply/patch:147: new blank line at EOF.
warning: 4 lines add whitespace errors.
I ran the last version of the patch through git apply and I only received 3 errors. I fixed everything I could find and now git apply is giving me no errors.
Cheers, George McCollister
ACK
bye, Sumit
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 02/05/2010 08:19 AM, Sumit Bose wrote:
ACK
Pushed to master.
- -- Stephen Gallagher RHCE 804006346421761
Delivering value year after year. Red Hat ranks #1 in value among software vendors. http://www.redhat.com/promo/vendor/
sssd-devel@lists.fedorahosted.org