Reminder and caution
*Change Process* Most changes (bug fixes and minor, commonsense feature adds) do not require review. Developers are encouraged to request review for: - Large changes affecting many files - Changes to interfaces - Changes that commit APR to one option out of an exclusive set - Any change the developer wants a second (or Nth) opinion on Anyone may retroactively cause someone's change to be reviewed by requesting review after it was committed, and at their discretion may revert the change until it is approved. The above process is called "Commit Then Review" (or CTR). As of this time, the group does not see a need to ever use a "Review Then Commit" (RTC) process.
Re: svn commit: r1833359 - in /apr/apr/trunk: CHANGES CMakeLists.txt build.conf build/crypto.m4 crypto/apr_crypto.c crypto/apr_crypto_prng.c include/apr.h.in include/apr_crypto.h test/testcrypto.c thr
On Tue, Jul 24, 2018 at 10:12 AM, Joe Orton wrote: > On Tue, Jul 24, 2018 at 02:25:57PM +0200, Yann Ylavic wrote: > > On Tue, Jul 24, 2018 at 11:43 AM, Graham Leggett > wrote: > > > 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. > > It's much worse than simply being pointless, pointless DSO abstractions > make dependency management both more complex and more fragile for > distributors. It is a total misfeature in both the crypto and LDAP > cases. When I have a direct linkage (i.e. ELF DT_NEEDED tags): > > mod_ldap -> libaprldap.so -> libldap.so > mod_whatever -> libapr.so -> libssl.so > > my (RPM/.deb/whatever) package manager can detect library dependencies > and reflect that in package dependencies to ensure system consistency. > > When you stick a DSO abstraction in the middle that goes away, and now I > have to track the dependencies manually (i.e. it's now fragile), even > though I know that 100% of users of mod_ldap CAN ONLY and MUST use > exactly the same libldap.so as if they had directly linked against > -lldap. Rinse and repeat for libcrypto. > > This is obviously completely different to db-style APIs where users > *actually care* because their runtime environments are completely > different (pgsql/mysql/...), hence run-time selection (and > configuration) is well matched by a DSO-based abstraction. > While this is a valid argument, as a distributor, you present the option of installing multiple DB providers, multiple crypto providers, etc. Due to the flexible nature of this API, they can pick, say, mariadb, and have only the single db provider loaded by libldl. Were APR to insist on 1:1 providers, the user could not switch to using postgresql, or could only do so by mapping both .so provider stacks into memory. Based on the way most modules are architected, they are completely abstract. The underlying mod_authn_dbd doesn't care about which provider is loaded. Only the libaprmodule-1.so does. The only fragile piece is aprldap-1.so, which completely exposes which ldap provider was chosen to the consuming application. This is why it was evicted from apr-2. If that is ever revisited and made entirely abstract, it would be welcome back into the fold. Yes, the manual tracking of dependency chains is a headache, but not insurmountable, and gives the users and packagers additional choices.
Re: svn commit: r1836519 - /apr/apr/trunk/strings/apr_strings.c
On Tue, Jul 24, 2018 at 12:53 AM, William A Rowe Jr wrote:
> I'm concerned that you've made a specific assumption of 2's compliment int.
> Nothing in the spec or real world assures us of this. Intel x86 is 2's
> compliment, but this is a bad assumption.
I doubt we support 1s complement archs but if so, something like this
would work:
static const char xtoa_digits[] = "9876543210123456789";
static const int xtoa_middle = (sizeof(xtoa_digits) - 1) / 2;
APR_DECLARE(char *) apr_itoa(apr_pool_t *p, int n)
{
const int BUFFER_SIZE = sizeof(int) * 3 + 2;
char *buf = apr_palloc(p, BUFFER_SIZE);
char *start = buf + BUFFER_SIZE - 1;
int negative = (n < 0);
*start = 0;
do {
int tmp = n;
n /= 10;
*--start = xtoa_digits[xtoa_middle + tmp - n * 10];
} while (n);
if (negative) {
*--start = '-';
}
return start;
}
Same for apr_ltoa() or apr_off_t_toa() (with the corresponding type
for "tmp"), no sign assumption.
Re: svn commit: r1833421 - in /apr/apr/trunk: ./ build/ crypto/ include/ include/private/ test/
(I can't quote easily with the html indentation and must edit your parts, so please use plain text) On Tue, Jul 24, 2018 at 4:12 PM, Graham Leggett wrote: > On 24 Jul 2018, at 15:26, Yann Ylavic wrote: > >>> Having looked at this in more detail, I am -1 on this change on the >>> following basis: >>> >>> - We create a hard link between our crypto libraries and APR itself, >>> which both breaks and renders pointless the existing DSO mechanism, >>> and makes APR significantly more expensive and less attractive to >>> use. > >> This is being discussed in the other, so I won't my arguments here. > > Just to clarify, I’m -1 (veto). So I have to convince you (let's try again :p ) or revert, this is it? I'd like to ear what others think though... >> I agree that APR shouldn't de-init *but* when openssl is unloaded, >> which APR can't control, even with DSO. > > APR controls the loading and unloading of APR DSOs, there is nothing > stopping us making the DSO unload a noop when broken versions of openssl are > present. But de-init may be required/desired when APR loads (e.g.) openssl for its own use no? What if the user wants openssl (< 1.1) cleanup when the program stops (possibly that'd clear some memory or alike)? >> What matters more is that APR shouldn't init either unless it is the >> one loading openssl (dlopen() is refcounted!), and that's user's >> knowledge still, not APR’s. > > The apr_crypto_nss driver supports a “noinit” option that does exactly this, > nothing stops us adding the same to apr_crypto_openssl. Please note that apr_crypto_lib_init() takes parameters as argument too. >> The latest openssl is certainly a noop when init is called multiple >> times, but earlier versions leak all over the place when this happens. >> The openssl team tried hard to de-init/re-init(), but finally failed >> to find a portable way to do it and reintroduced some static variables >> which prevent it, including in the latest/master code. >> Unless we want to take (and fail) the same way, we have no other >> choice than let the user decide when to init and de-init, ie. with our >> usual given pool's lifetime. > > What our choices are must be confirmed by the openssl team. I;ve asked on > the openssl-users list and am waiting for confirmation. > > If openssl v1.1.0 is still broken, we just noop the DSO unload - however see > below. We must take multiple openssl versions into account, not only the latest one. If you want an example of what needs to be done for openssl loading/unloading to work and/or not leak accross versions, the history of httpd's ssl_hook_pre_config() and ssl_cleanup_pre_config() are worth a look too. >> I see apr_crypto_lib_init() as the initial call to control openssl >> (de-)init for users of APR and openssl (or another crypto lib handled >> by APR) in the same program. > > This function is functionally identical to the apr_crypto_make() function. You probably mean crypto_init() but yes, same arguments and code moved from there to apr_crypto_lib_init() is on purpose. > > What seems to be a more sensible approach is a general mechanism that allows > the caller to tell APR what external libraries have been loaded. > > In other words, don’t try and initialise openssl, just make a note that the > caller has told us that openssl has been used externally FYI. > > Later on, should we load our DSO, we are in a position to bypass the init > step and to to noop the unload step and avoid any problems. I fail to see how it's better than not calling apr_crypto_lib_init(), and that doesn't seem contradictory with my proposal either. Pass that option to apr_crypto_get_driver() if it shouldn't init the lib because your code already does it, or call apr_crypto_get_driver() without the option because you want APR to handle its own openssl stuff, or call apr_crypto_lib_init() explicitely because you also use openssl but don't want to handle all of its init oddnesses. All these uses cases look interesting to me. Regards, Yann.
Re: svn commit: r1833359 - in /apr/apr/trunk: CHANGES CMakeLists.txt build.conf build/crypto.m4 crypto/apr_crypto.c crypto/apr_crypto_prng.c include/apr.h.in include/apr_crypto.h test/testcrypto.c thr
On Tue, Jul 24, 2018 at 02:25:57PM +0200, Yann Ylavic wrote: > On Tue, Jul 24, 2018 at 11:43 AM, Graham Leggett wrote: > > 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. It's much worse than simply being pointless, pointless DSO abstractions make dependency management both more complex and more fragile for distributors. It is a total misfeature in both the crypto and LDAP cases. When I have a direct linkage (i.e. ELF DT_NEEDED tags): mod_ldap -> libaprldap.so -> libldap.so mod_whatever -> libapr.so -> libssl.so my (RPM/.deb/whatever) package manager can detect library dependencies and reflect that in package dependencies to ensure system consistency. When you stick a DSO abstraction in the middle that goes away, and now I have to track the dependencies manually (i.e. it's now fragile), even though I know that 100% of users of mod_ldap CAN ONLY and MUST use exactly the same libldap.so as if they had directly linked against -lldap. Rinse and repeat for libcrypto. This is obviously completely different to db-style APIs where users *actually care* because their runtime environments are completely different (pgsql/mysql/...), hence run-time selection (and configuration) is well matched by a DSO-based abstraction.
Re: svn commit: r1833421 - in /apr/apr/trunk: ./ build/ crypto/ include/ include/private/ test/
On 24 Jul 2018, at 15:26, Yann Ylavic wrote: >> Having looked at this in more detail, I am -1 on this change on the >> following basis: >> >> - We create a hard link between our crypto libraries and APR itself, >> which both breaks and renders pointless the existing DSO mechanism, >> and makes APR significantly more expensive and less attractive to >> use. > > This is being discussed in the other, so I won't my arguments here. Just to clarify, I’m -1 (veto). > I agree that APR shouldn't de-init *but* when openssl is unloaded, > which APR can't control, even with DSO. APR controls the loading and unloading of APR DSOs, there is nothing stopping us making the DSO unload a noop when broken versions of openssl are present. > What matters more is that APR shouldn't init either unless it is the > one loading openssl (dlopen() is refcounted!), and that's user's > knowledge still, not APR’s. The apr_crypto_nss driver supports a “noinit” option that does exactly this, nothing stops us adding the same to apr_crypto_openssl. > The latest openssl is certainly a noop when init is called multiple > times, but earlier versions leak all over the place when this happens. > The openssl team tried hard to de-init/re-init(), but finally failed > to find a portable way to do it and reintroduced some static variables > which prevent it, including in the latest/master code. > Unless we want to take (and fail) the same way, we have no other > choice than let the user decide when to init and de-init, ie. with our > usual given pool's lifetime. What our choices are must be confirmed by the openssl team. I;ve asked on the openssl-users list and am waiting for confirmation. If openssl v1.1.0 is still broken, we just noop the DSO unload - however see below. > I see apr_crypto_lib_init() as the initial call to control openssl > (de-)init for users of APR and openssl (or another crypto lib handled > by APR) in the same program. This function is functionally identical to the apr_crypto_make() function. What seems to be a more sensible approach is a general mechanism that allows the caller to tell APR what external libraries have been loaded. In other words, don’t try and initialise openssl, just make a note that the caller has told us that openssl has been used externally FYI. Later on, should we load our DSO, we are in a position to bypass the init step and to to noop the unload step and avoid any problems. I’m sure this problem isn’t limited to openssl, but any library we DSO to. Regards, Graham —
Re: svn commit: r1833421 - in /apr/apr/trunk: ./ build/ crypto/ include/ include/private/ test/
On Tue, Jul 24, 2018 at 2:10 PM, Graham Leggett wrote:
> On 12 Jun 2018, at 22:11, yla...@apache.org wrote:
>
>> Author: ylavic
>> Date: Tue Jun 12 20:11:09 2018
>> New Revision: 1833421
>>
>> URL: http://svn.apache.org/viewvc?rev=1833421=rev
>> Log:
>> apr_crypto: follow up to r1833359.
>>
>> Link underlying crypto libraries (openssl, nss, and commoncrypto) with libapr
>> when the corresponding --with is configured, and allow to initialize,
>> terminate
>> or check whether initialized with respectively them with
>> apr_crypto_lib_init(),
>> apr_crypto_lib_term() or apr_crypto_lib_is_initialized().
>>
>> This allows for users to control the (un)initialization of those libraries,
>> notably when they are also used independently in the user code and that doing
>> this multiple times can cause leaks or unexpected behaviour.
>>
>> The initialization code is moved from
>> "apr_crypto_{openssl,nss,commoncrypto}.c"
>> where previously loaded dynamically (DSO) to "apr_crypto_internal.c" which is
>> linked with libapr.
>>
>> Now apr_crypto_prng_init() can make sure the underlying crypto lib is ready.
>
> Having looked at this in more detail, I am -1 on this change on the
> following basis:
>
> - We create a hard link between our crypto libraries and APR itself,
> which both breaks and renders pointless the existing DSO mechanism,
> and makes APR significantly more expensive and less attractive to
> use.
This is being discussed in the other, so I won't my arguments here.
>
> - This doesn’t fix the problem for OpenSSL. The problem is related to
> legacy OpenSSL’s inability to de-init and re-init, which google shows
> me is a universal problem suffered by other libraries, not just us.
> The fix appears to be to not de-init the library in the DSO, but I;ve
> asked the openssl user’s list to be sure. In other words, this
> attempt to init is unnecessary, we should actually not de-init.
I agree that APR shouldn't de-init *but* when openssl is unloaded,
which APR can't control, even with DSO.
What matters more is that APR shouldn't init either unless it is the
one loading openssl (dlopen() is refcounted!), and that's user's
knowledge still, not APR's.
The latest openssl is certainly a noop when init is called multiple
times, but earlier versions leak all over the place when this happens.
The openssl team tried hard to de-init/re-init(), but finally failed
to find a portable way to do it and reintroduced some static variables
which prevent it, including in the latest/master code.
Unless we want to take (and fail) the same way, we have no other
choice than let the user decide when to init and de-init, ie. with our
usual given pool's lifetime.
I see apr_crypto_lib_init() as the initial call to control openssl
(de-)init for users of APR and openssl (or another crypto lib handled
by APR) in the same program.
>
> - This change affects NSS, which doesn’t suffer from this problem as
> NSS supports proper reference counting and multiple inits. Touching
> the NSS code is unnecessary.
So it shouldn't hurt, we aim to provide portable code, same user code
regardless of the underlying system/lib right?
>
> - This problem appears to have been fixed in openssl v1.1.0, where no
> initialisation is required at all. I have asked on openssl-users for
> clarification on this.
> https://www.openssl.org/docs/man1.1.0/crypto/OPENSSL_init_crypto.html
This isn't the only version supported by OpenSSL, distros nor APR (you
even added compatibility code for 0.9.8b in one of your latests
commits...).
I think we should provide a mechanism that works for as much
(maintained) versions as possible, which seems to be the case here.
>
> To be clear, I am only referring to this change here, this is
> unrelated to the PRNG patch which is a separate discussion.
Agreed, thanks.
Regards,
Yann.
Re: svn commit: r1833359 - in /apr/apr/trunk: CHANGES CMakeLists.txt build.conf build/crypto.m4 crypto/apr_crypto.c crypto/apr_crypto_prng.c include/apr.h.in include/apr_crypto.h test/testcrypto.c thr
On Tue, Jul 24, 2018 at 11:43 AM, Graham Leggett wrote: > On 23 Jul 2018, at 01:10, Yann Ylavic 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
Re: svn commit: r1833421 - in /apr/apr/trunk: ./ build/ crypto/ include/ include/private/ test/
On 12 Jun 2018, at 22:11, yla...@apache.org wrote:
> Author: ylavic
> Date: Tue Jun 12 20:11:09 2018
> New Revision: 1833421
>
> URL: http://svn.apache.org/viewvc?rev=1833421=rev
> Log:
> apr_crypto: follow up to r1833359.
>
> Link underlying crypto libraries (openssl, nss, and commoncrypto) with libapr
> when the corresponding --with is configured, and allow to initialize,
> terminate
> or check whether initialized with respectively them with
> apr_crypto_lib_init(),
> apr_crypto_lib_term() or apr_crypto_lib_is_initialized().
>
> This allows for users to control the (un)initialization of those libraries,
> notably when they are also used independently in the user code and that doing
> this multiple times can cause leaks or unexpected behaviour.
>
> The initialization code is moved from
> "apr_crypto_{openssl,nss,commoncrypto}.c"
> where previously loaded dynamically (DSO) to "apr_crypto_internal.c" which is
> linked with libapr.
>
> Now apr_crypto_prng_init() can make sure the underlying crypto lib is ready.
Having looked at this in more detail, I am -1 on this change on the following
basis:
- We create a hard link between our crypto libraries and APR itself, which both
breaks and renders pointless the existing DSO mechanism, and makes APR
significantly more expensive and less attractive to use.
- This doesn’t fix the problem for OpenSSL. The problem is related to legacy
OpenSSL’s inability to de-init and re-init, which google shows me is a
universal problem suffered by other libraries, not just us. The fix appears to
be to not de-init the library in the DSO, but I;ve asked the openssl user’s
list to be sure. In other words, this attempt to init is unnecessary, we should
actually not de-init.
- This change affects NSS, which doesn’t suffer from this problem as NSS
supports proper reference counting and multiple inits. Touching the NSS code is
unnecessary.
- This problem appears to have been fixed in openssl v1.1.0, where no
initialisation is required at all. I have asked on openssl-users for
clarification on this.
https://www.openssl.org/docs/man1.1.0/crypto/OPENSSL_init_crypto.html
To be clear, I am only referring to this change here, this is unrelated to the
PRNG patch which is a separate discussion.
Regards,
Graham
—
buildbot success in on apr-x64-macosx-trunk
The Buildbot has detected a restored build on builder apr-x64-macosx-trunk while building . Full details are available at: https://ci.apache.org/builders/apr-x64-macosx-trunk/builds/132 Buildbot URL: https://ci.apache.org/ Buildslave for this Build: svn-x64-macosx-dgvrs Build Reason: The AnyBranchScheduler scheduler named 'on-apr-commit' triggered this build Build Source Stamp: [branch apr/apr/trunk] 1836539 Blamelist: ylavic Build succeeded! Sincerely, -The Buildbot
Re: svn commit: r1836439 [2/3] - in /apr/apr/trunk: ./ crypto/ include/ include/private/ test/
On 23 Jul 2018, at 01:26, Yann Ylavic wrote:
> On Sun, Jul 22, 2018 at 3:11 PM, wrote:
>> --- apr/apr/trunk/crypto/apr_crypto_openssl.c (original)
>> +++ apr/apr/trunk/crypto/apr_crypto_openssl.c Sun Jul 22 13:11:32 2018
>>
>> @@ -106,16 +143,87 @@ static apr_status_t crypto_error(const a
>> */
>> static apr_status_t crypto_shutdown(void)
>> {
>> -return apr_crypto_lib_term("openssl");
>> +ERR_free_strings();
>> +EVP_cleanup();
>> +ENGINE_cleanup();
>> +return APR_SUCCESS;
>> +}
>
> So you reverted this...
>
>> +
>> +static apr_status_t crypto_shutdown_helper(void *data)
>> +{
>> +return crypto_shutdown();
>> }
>>
>> /**
>> * Initialise the crypto library and perform one time initialisation.
>> */
>> static apr_status_t crypto_init(apr_pool_t *pool, const char *params,
>> -const apu_err_t **result)
>> +const apu_err_t **result)
>> +{
>> +#if APR_USE_OPENSSL_PRE_1_1_API
>> +(void)CRYPTO_malloc_init();
>> +#else
>> +OPENSSL_malloc_init();
>> +#endif
>> +ERR_load_crypto_strings();
>> +/* SSL_load_error_strings(); */
>> +OpenSSL_add_all_algorithms();
>> +ENGINE_load_builtin_engines();
>> +ENGINE_register_all_complete();
>> +
>> +apr_pool_cleanup_register(pool, pool, crypto_shutdown_helper,
>> +apr_pool_cleanup_null);
>> +
>> +return APR_SUCCESS;
>> +}
>> -return apr_crypto_lib_init("openssl", params, result, pool);
>
> ... and this (including for the other libs), because?
Because it was an accident. I missed this one when merging.
Let me take a look at this tomorrow.
> The goal was to allow the initialization of the crypto libs
> independently from DSOs (openssl, for instance, does not play very
> well there).
>
> In httpd there is (at least) APR, the httpd core, mod_ssl and mod_md
> that can all use the crypto library, how is each one supposed to
> initialize it? I thought it was nice for APR to provide a single
> initializer, once for all, with the lifetime of the given pool (as
> usual)…
I've raised this issue before, but there was little interest in solving it -
glad to see this has got some traction.
I believe libraries like NSS are reference counted and initialise themselves
sensibly. Not sure about the behaviour of openssl, however the openssl people
should be able to explain what people should be doing.
Regards,
Graham
—
Re: svn commit: r1833359 - in /apr/apr/trunk: CHANGES CMakeLists.txt build.conf build/crypto.m4 crypto/apr_crypto.c crypto/apr_crypto_prng.c include/apr.h.in include/apr_crypto.h test/testcrypto.c thr
On 23 Jul 2018, at 01:10, Yann Ylavic 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
—
