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(a)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