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