Re: [Openvpn-devel] [PATCH v5 2/2] Add unit tests for engine keys
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
Re: [Openvpn-devel] [PATCH v5 2/2] Add unit tests for engine keys
Hi,
On Mon, May 25, 2020 at 07:36:11AM -0700, James Bottomley wrote:
> > > configure.ac | 5 +
> > > src/openvpn/crypto_openssl.c | 1 +
> >
> > These two hunks should go to the first patch.
>
> The configure.ac one adds the test Makefile ... it can't move. The
> other pieces figure out what the dynamic extension is, which is only
> used by the test, so I think everything in the configure.ac change is
> in the right place.
Yeah, I misread the part about
+AM_CONDITIONAL([OPENSSL_ENGINE], [test "${have_openssl_engine}" = "yes"])
and assumed it's needed for the #ifdef in the openvpn code.
> The second is a bit more problematic: Right at the moment openvpn is
> blind to local openssl configuration files. To make the test work, it
> needs to be able to add an engine, which requires a local file, so it's
> only function is to enable the test, but it does add to openvpn
> capabilities, I suppose.
I think it should go to the first patch - so "all changes to openvpn
behaviour" are caused by this patch, and the test patch will only add
testing, not change behaviour.
On the actual patch itself, I do not understand enough of the engine
stuff to fully ACK it. Arne or Steffan understand OpenSSL better.
gert
--
"If was one thing all people took for granted, was conviction that if you
feed honest figures into a computer, honest figures come out. Never doubted
it myself till I met a computer with a sense of humor."
Robert A. Heinlein, The Moon is a Harsh Mistress
Gert Doering - Munich, Germany g...@greenie.muc.de
signature.asc
Description: PGP signature
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v5 2/2] Add unit tests for engine keys
On Mon, 2020-05-25 at 08:04 +0200, Gert Doering wrote:
> 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.
The configure.ac one adds the test Makefile ... it can't move. The
other pieces figure out what the dynamic extension is, which is only
used by the test, so I think everything in the configure.ac change is
in the right place.
The second is a bit more problematic: Right at the moment openvpn is
blind to local openssl configuration files. To make the test work, it
needs to be able to add an engine, which requires a local file, so it's
only function is to enable the test, but it does add to openvpn
capabilities, I suppose.
James
> 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"?
>
> gert
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
Re: [Openvpn-devel] [PATCH v5 2/2] Add unit tests for engine keys
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
Re: [Openvpn-devel] [PATCH v5 2/2] Add unit tests for engine keys
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"?
gert
--
"If was one thing all people took for granted, was conviction that if you
feed honest figures into a computer, honest figures come out. Never doubted
it myself till I met a computer with a sense of humor."
Robert A. Heinlein, The Moon is a Harsh Mistress
Gert Doering - Munich, Germany g...@greenie.muc.de
signature.asc
Description: PGP signature
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH v5 2/2] Add unit tests for engine keys
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.
Signed-off-by: James Bottomley
---
v5: do not hard code dynamic library extension into openssl.cnf (MacOS)
v4: add OPENSSL_config(NULL) so debian checks will work
v3: added this patch
---
configure.ac | 5 +
src/openvpn/crypto_openssl.c | 1 +
tests/unit_tests/Makefile.am | 3 +
tests/unit_tests/engine-key/Makefile.am | 24 +
.../engine-key/check_engine_keys.sh | 30 ++
tests/unit_tests/engine-key/libtestengine.c | 101 ++
tests/unit_tests/engine-key/openssl.cnf.in| 12 +++
7 files changed, 176 insertions(+)
create mode 100644 tests/unit_tests/engine-key/Makefile.am
create mode 100755 tests/unit_tests/engine-key/check_engine_keys.sh
create mode 100644 tests/unit_tests/engine-key/libtestengine.c
create mode 100644 tests/unit_tests/engine-key/openssl.cnf.in
diff --git a/configure.ac b/configure.ac
index 273a8d1b..92d4eeba 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1387,6 +1387,10 @@ AM_CONDITIONAL([GIT_CHECKOUT], [test "${GIT_CHECKOUT}" =
"yes"])
AM_CONDITIONAL([ENABLE_PLUGIN_AUTH_PAM], [test "${enable_plugin_auth_pam}" =
"yes"])
AM_CONDITIONAL([ENABLE_PLUGIN_DOWN_ROOT], [test "${enable_plugin_down_root}" =
"yes"])
AM_CONDITIONAL([HAVE_LD_WRAP_SUPPORT], [test "${have_ld_wrap_support}" =
"yes"])
+AM_CONDITIONAL([OPENSSL_ENGINE], [test "${have_openssl_engine}" = "yes"])
+
+shrext=$shrext_cmds
+AC_SUBST([shrext])
sampledir="\$(docdir)/sample"
AC_SUBST([plugindir])
@@ -1448,6 +1452,7 @@ AC_CONFIG_FILES([
tests/unit_tests/openvpn/Makefile
tests/unit_tests/plugins/Makefile
tests/unit_tests/plugins/auth-pam/Makefile
+ tests/unit_tests/engine-key/Makefile
sample/Makefile
])
AC_CONFIG_FILES([tests/t_client.sh], [chmod +x tests/t_client.sh])
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)
diff --git a/tests/unit_tests/Makefile.am b/tests/unit_tests/Makefile.am
index 33fefaac..f27cd90f 100644
--- a/tests/unit_tests/Makefile.am
+++ b/tests/unit_tests/Makefile.am
@@ -2,4 +2,7 @@ AUTOMAKE_OPTIONS = foreign
if ENABLE_UNITTESTS
SUBDIRS = example_test openvpn plugins
+if OPENSSL_ENGINE
+SUBDIRS += engine-key
+endif
endif
diff --git a/tests/unit_tests/engine-key/Makefile.am
b/tests/unit_tests/engine-key/Makefile.am
new file mode 100644
index ..05f56bfd
--- /dev/null
+++ b/tests/unit_tests/engine-key/Makefile.am
@@ -0,0 +1,24 @@
+AUTOMAKE_OPTIONS = foreign
+
+check_LTLIBRARIES = libtestengine.la
+conffiles = openssl.cnf
+
+TESTS_ENVIRONMENT = srcdir="$(abs_srcdir)"; \
+ builddir="$(abs_builddir)"; \
+ top_builddir="$(top_builddir)"; \
+ top_srcdir="$(top_srcdir)"; \
+ export srcdir builddir top_builddir top_srcdir;
+
+TESTS = check_engine_keys.sh
+check_engine_keys.sh: $(conffiles)
+
+clean-local:
+ rm -f $(conffiles)
+
+$(builddir)/%.cnf: $(srcdir)/%.cnf.in
+ sed 's/SHREXT/@shrext@/' < $< > $@
+
+libtestengine_la_SOURCES = libtestengine.c
+libtestengine_la_LDFLAGS = @TEST_LDFLAGS@ -rpath /lib -avoid-version -module
-shared -export-dynamic
+libtestengine_la_CFLAGS = @TEST_CFLAGS@ -I$(openvpn_srcdir) -I$(compat_srcdir)
+
diff --git a/tests/unit_tests/engine-key/check_engine_keys.sh
b/tests/unit_tests/engine-key/check_engine_keys.sh
new file mode 100755
index ..e0c9d7b0
--- /dev/null
+++ b/tests/unit_tests/engine-key/check_engine_keys.sh
@@ -0,0 +1,30 @@
+#!/bin/sh
+
+OPENSSL_CONF="${builddir}/openssl.cnf"
+export OPENSSL_CONF
+
+password='AT3S4PASSWD'
+
+key="${builddir}/client.key"
+pwdfile="${builddir}/passwd"
+
+# create an engine key for us
+sed 's/PRIVATE KEY/TEST ENGINE KEY/' <
${top_srcdir}/sample/sample-keys/client.key > ${key}
+echo "$password" > $pwdfile
+
+# note here we've induced a mismatch in the client key and the server
+# cert which openvpn should report and die. Check that it does. Note
+# also that this mismatch depends on openssl not openvpn, so it is
+# somewhat fragile
+${top_builddir}/src/openvpn/openvpn --cd ${top_srcdir}/sample --config
sample-config-files/loopback-server --engine testengine --key ${key} --askpass
$pwdfile > log.txt 2>&1
+
+# first off check we died because of a key mismatch. If this doesn't
+# pass, suspect openssl of
