On Tue, Jul 24, 2018 at 11:43 AM, Graham Leggett <minf...@sharp.fm> wrote:
> On 23 Jul 2018, at 01:10, Yann Ylavic <ylavic....@gmail.com> wrote:
>>
>> 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.
How is DSO harder or softer than linking when --with-crypto (and
--with-openssl or any other crypto lib) is configured?
To me, --with-openssl means link with openssl, just like
--with-libxml2 or --with-expat.
You don't want (or can't have) APR crypto, just don't ./configure it,
otherwise you need a crypto lib on the system in any case, be it
dlopen()ed or linked.
>
> Remember this is the Apache Portable Runtime, and currently there is
> nothing portable about the PRNG implementation.
Do you mean openssl isn't portable?
(BTW, I still struggle on how to do
simple/run-this-cipher-with-this-key crypto with libnss, or even find
a stream cipher with commoncrypto).
>
> This needs to be changed to follow our existing convention.
I think there isn't a single convention in APR, libxml2 is not dlopen()ed right?
>
> 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.
Thanks for your support, but IMHO here a key point is simplicity (as
opposed to complexity) like for any crypto code.
The more complex, the less secure, let's not introduce attack vectors
in our implementation.
>
>>> 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.
That's how it works, though not how *you* want it to work.
>
> 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.
I think this is a mistake (for crypto at least) and that it solves nothing.
As I already said, DSOs don't work better if the external lib is not
installed on the system in the first place (same dependency), and so
will libapr-util.
>
> 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.
I disagree, APR offers crypto but does not want to implement crypto
primitives (rightly), so it depends on a crypto lib for --with-crypto.
They way we link to this lib is irrelevent.
>
> 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.
Come on, openssl is highly portable and exists on all the OSes you are
citing here.
If one wants/needs APR PRNG, it needs openssl which shouldn't be
difficult to achieve.
Once I (or any of us) figure out how to use nss or cc for the PRNG
needs, this will improve (no different than the ENOTIMPLs for the two
ms* API in current apr_crypto code right?).
>
>>> 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.
That's a bit of bikeshedding...
I could implement _init/_encrypt/_finish for the two stream cipher
used by the PRNG, in a separate file (looks overkill with openssl only
for now) or in the crypto driver (could be useful outside the PRNG,
though the "block" terminology doesn't help there). At least with
openssl for now still.
Yet I won't use them for the PRNG because I can do better with
internal/low-level/self-contained code in the PRNG itself (my concerns
are non-failure and speed, which doesn't match allocations for every
crypto operation).
>
> Binding to crypto libraries is a solved problem, what we’ve done here
> is unsolve that problem.
Again, I see no problem with linking to a lib when --with-that-lib is
used. What problem exactly?
Anyway, if this is really an issue for the team, an other option I can
see is to put the PRNG outside of the APR(-util) crypto tree/linking,
i.e. directly in APR "core" as a replacement of apr_random, and
implement CHACHA20 by ourself (which is light code).
>
>>> 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.
Possibly addressed in r1836539, we really need to crypto_init() on the
global pool (as before) since openssl can't cleanup than re-init (no
DSO unload/reload to clears it internals...).
Regards,
Yann.
