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