Am 25.05.20 um 08:04 schrieb Gert Doering:
> Hi,
>
> I see the granularity of your patch set as "not right":
>
> On Sun, May 24, 2020 at 01:33:22PM -0700, James Bottomley wrote:
>> Testing engines is problematic, so one of the prerequisites built for
>> the tests is a simple openssl engine that reads a non-standard PEM
>> guarded key. The test is simply can we run a client/server
>> configuration with the usual sample key replaced by an engine key.
>> The trivial engine prints out some operations and we check for these
>> in the log to make sure the engine was used to load the key and that
>> it correctly got the password.
>
> This patch says "add unit tests", but it contains changes to configure.ac
> and OpenVPN code
>
>> configure.ac | 5 +
>> src/openvpn/crypto_openssl.c | 1 +
>
> These two hunks should go to the first patch.
>
> Every patch should be fully testable on its own - so if I apply only
> the first hunk, I should be able to use and test engine keys, without
> having to apply the "add a unit test" patch set.
>
>> diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c
>> index a7569623..34637ebf 100644
>> --- a/src/openvpn/crypto_openssl.c
>> +++ b/src/openvpn/crypto_openssl.c
>> @@ -92,6 +92,7 @@ setup_engine(const char *engine)
>> {
>> ENGINE *e = NULL;
>>
>> + OPENSSL_config(NULL);
>> ENGINE_load_builtin_engines();
>>
>> if (engine)
>
> For that change, I wonder what side effects it might have on existing
> setups. Arne, can you help? Is this "safe"?
> For OpenSSL 1.1 this is actually a NACK since it is depreacted API and we want to wrap those into ifdefs to be able to remove them when we drop support for older version. For OpenSSL 1.1 this is a deprecated function to contrary to other depreacted function like OpenSSL_init() not a noop. But this command even though it does not have a return value or reports erro, can still put errors on the error stack. Reading the general openssl.cnf at engine_setup also feels a bit like the wrong place but since this code is only executed when --engine is present in the config and we will remove that OPENSSL_config in the future anyway (with the ifdef) it a low risk to put it here instead of a more proper place, so I am fine to do here. As a side note, engines are deprecated in OpenSSL 3.0.0 and are replaced by providers. But that will probably take a long time until that is really depreacted and removed, so adding/fixing this for now is okay. Arne
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
