On 23 Jul 2018, at 01:10, Yann Ylavic <ylavic....@gmail.com> wrote:
>>> +/* XXX: APU_HAVE_CRYPTO_PRNG shoudn't be defined! */
>>> +#error apr_crypto_prng implemented with OpenSSL only for now
>>
>> The layout of the code goes against the established structure of the
>> apr_crypto API, all of this openssl specific code should go into
>> crypto/apr_crypto_openssl.c.
>
> No thanks, this is APR crypto *PRNG* API, no DSO, no block cipher, no
> PBKDF2, no HMAC, no MD2/4/5 (really?)..
> It needs a stream cipher with a quite specific interface, no
> _init/_encrypt_/_finish each time a keystream is to be generated, no
> mutiple indirections, no allocation once the initial context is
> created (i.e. it never fails after apr_crypto_prng_init(), which is
> nice for a PRNG).
> I know how to do this with openssl, not with the other libs yet but I
> guess it should be as simple.
> I don't see why we'd need all the crypto
> block/passphrase/digest/hmac/whatever complexity for the 5 lines of
> the above function.
I’m not seeing that this functionality being PRNG, or the apparent simplicity
of it being relevant in any way.
We’ve created a hard link between APR and OpenSSL, in a system where we already
have a clean DSO based system to interface with crypto and other libraries.
Remember this is the Apache Portable Runtime, and currently there is nothing
portable about the PRNG implementation.
This needs to be changed to follow our existing convention.
To be clear I totally support the addition of the PRNG code, it’s just the
breaking of established conventions that we need to fix.
>> We shouldn’t be ignoring the caller’s choice of crypto library and
>> then hard coding these calls to openssl, especially on platforms like
>> Linux where openssl might be installed by default. Platforms like
>> MacOS where openssl is deprecated would also be a problem.
>
> Without openssl, the CPRNG is not configured/available, it's that
> simple and no different than other APR parts.
> We don't ignore anything, but simply implement it only with openssl for now.
> When cprng_stream_ctx_bytes() is implemented with other libraries, the
> CPRNG becomes available with that libraries.
That’s not how it works, no.
Throughout APR we have a long established convention where “expensive”
libraries are bound to APR through DSOs. All the SQL database drivers are like
this, LDAP is like this, and so is crypto.
Now, an “expensive” openssl library, and very soon an “expensive” NSS library
are going to be tightly bound to APR itself. This part is broken and needs
fixing.
Remember OpenSSL is deprecated on MacOS (thus commoncrypto), and entirely
missing on Windows (thus a potential future Windows specific driver), so
binding to openssl is not portable.
>> The apr_crypto_block_encrypt_init / apr_crypto_block_encrypt /
>> apr_crypto_block_encrypt_finish functions already implement the above
>> for you, so they could be used instead.
>
> No, the CPRNG does not use a block cipher, and does not allocate data
> for encryption (at least with openssl, provided we don't use
> _init/_encrypt/_finish), and is constant time with chacha20 and with
> AES256-CTR if aesni instructions are available on the CPU (provided we
> use the simple functions self-contained in "apr_crypto_prng.c". These
> are not negligeable points for a PRNG…
Again, not seeing what the problem is here.
Either use what’s there, or if it’s not there add it, or alternatively add a
dedicated function to apr_crypto_internal.h for whatever PRNG call you need and
then do whatever library specific thing you need to do in that function as
you’ve described above.
Binding to crypto libraries is a solved problem, what we’ve done here is
unsolve that problem.
>> The tests keep segfaulting for me in apr-trunk and apr-util v1.7, I
>> think this code needs more tuning to get it right before it’s
>> backported to apr_util v1.7.
>
> Could you please be more specific? Is it what you fixed in r1836438
> ("Make sure we don't segfault if the PRNG does not initialise") or
> does it still segfault for you?
Still segfaults for me on apr-trunk (it originally didn’t build). I have not
yet had a chance to investigate this, I needed to get the test suite to the
point where it just fails and didn’t crash so I can get work done.
Regards,
Graham
—
