On Mon, 2020-05-25 at 14:52 +0200, Arne Schwabe wrote:
> 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.OK ... it's replacement is a simple OPENSSL_init_crypto(OPENSSL_INIT_LOAD_CONFIG, NULL); which is easy to #ifdef around. > 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. So if the desire is to make openvpn fully consistent with regard to local configuration files, the statement should be in crypto_lib_init() rather than conditioning the behaviour on setting the engines parameter. I can certainly do that, as a separate patch, but it would have a slightly wider blast radius. > 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. Heh, yes, we're all waiting to see how that works out. James
signature.asc
Description: This is a digitally signed message part
_______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
