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.