[Openvpn-devel,v6,3/3] Add unit tests for engine keys

Message ID 20200528225920.6983-4-James.Bottomley@HansenPartnership.com
State Superseded
Headers show
Series
  • add support for engine keys
Related show

Commit Message

James Bottomley May 28, 2020, 10:59 p.m.
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 <James.Bottomley@HansenPartnership.com>

---
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 +
 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 +++
 6 files changed, 175 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

Comments

Arne Schwabe June 5, 2020, 11:31 a.m. | #1
Am 29.05.20 um 00:59 schrieb James Bottomley:
> 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 <James.Bottomley@HansenPartnership.com>
> 
> ---
> 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 +
>  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 +++
>  6 files changed, 175 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/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 00000000..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 00000000..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 returning different messages and update the
> +# test accordingly
> +grep -q 'X509_check_private_key:key values mismatch' log.txt || { echo "Key mismatch not detected"; exit 1; }
> +
> +# now look for the engine prints (these are under our control)
> +grep -q 'ENGINE: engine_init called' log.txt || { echo "Engine initialization not detected"; exit 1; }
> +grep -q 'ENGINE: engine_load_key called' log.txt || { echo "Key was not loaded from engine"; exit 1; }
> +grep -q "ENGINE: engine_load_key got password ${password}" log.txt || { echo "Key password was not retrieved by the engine"; exit 1; }
> +exit 0
> diff --git a/tests/unit_tests/engine-key/libtestengine.c b/tests/unit_tests/engine-key/libtestengine.c
> new file mode 100644
> index 00000000..46ec1e33
> --- /dev/null
> +++ b/tests/unit_tests/engine-key/libtestengine.c
> @@ -0,0 +1,101 @@
> +#include <string.h>
> +#include <openssl/engine.h>
> +#include <openssl/evp.h>
> +#include <openssl/pem.h>
> +
> +static char *engine_id = "testengine";
> +static char *engine_name = "Engine for testing openvpn engine key support";
> +
> +static int is_initialized = 0;
> +
> +static int engine_init(ENGINE *e)
> +{
> +	is_initialized = 1;
> +	fprintf(stderr, "ENGINE: engine_init called\n");
> +	return 1;
> +}
> +
> +static int engine_finish(ENGINE *e)
> +{
> +	fprintf(stderr, "ENGINE: engine_finsh called\n");
> +	is_initialized = 0;
> +	return 1;
> +}
> +
> +static EVP_PKEY *engine_load_key(ENGINE *e, const char *key_id,
> +				 UI_METHOD *ui_method, void *cb_data)
> +{
> +	BIO *b;
> +	EVP_PKEY *pkey;
> +	PKCS8_PRIV_KEY_INFO *p8inf;
> +	UI *ui;
> +	char auth[256];
> +
> +	fprintf(stderr, "ENGINE: engine_load_key called\n");
> +
> +	if (!is_initialized) {
> +		fprintf(stderr, "Load Key called without correct initialization\n");
> +		return NULL;
> +	}
> +	b = BIO_new_file(key_id, "r");
> +	if (!b) {
> +		fprintf(stderr, "File %s does not exist or cannot be read\n", key_id);
> +		return 0;
> +	}
> +	/* Basically read an EVP_PKEY private key file with different
> +	 * PEM guards --- we are a test engine */
> +	p8inf = PEM_ASN1_read_bio((d2i_of_void *)d2i_PKCS8_PRIV_KEY_INFO,
> +				 "TEST ENGINE KEY", b,
> +				 NULL, NULL, NULL);
> +	BIO_free(b);
> +	if (!p8inf) {
> +		fprintf(stderr, "Failed to read engine private key\n");
> +		return NULL;
> +	}
> +	pkey = EVP_PKCS82PKEY(p8inf);
> +
> +	/* now we have a private key, pretend it had a password
> +	 * this verifies the password makes it through openvpn OK */
> +	ui = UI_new();
> +
> +	if (ui_method)
> +		UI_set_method(ui, ui_method);
> +
> +	UI_add_user_data(ui, cb_data);
> +
> +	if (UI_add_input_string(ui, "enter test engine key",
> +				UI_INPUT_FLAG_DEFAULT_PWD,
> +				auth, 0, sizeof(auth)) == 0) {
> +		fprintf(stderr, "UI_add_input_string failed\n");
> +		goto out;
> +	}
> +
> +	if (UI_process(ui)) {
> +		fprintf(stderr, "UI_process failed\n");
> +		goto out;
> +	}
> +
> +	fprintf(stderr, "ENGINE: engine_load_key got password %s\n", auth);
> +
> + out:
> +	UI_free(ui);
> +
> +	return pkey;
> +}
> +
> +
> +static int engine_bind_fn(ENGINE *e, const char *id)
> +{
> +	if (id && strcmp(id, engine_id) != 0)
> +		return 0;
> +	if (!ENGINE_set_id(e, engine_id) ||
> +	    !ENGINE_set_name(e, engine_name) ||
> +	    !ENGINE_set_init_function(e, engine_init) ||
> +	    !ENGINE_set_finish_function(e, engine_finish) ||
> +	    !ENGINE_set_load_privkey_function(e, engine_load_key))
> +		return 0;
> +	return 1;
> +}
> +
> +IMPLEMENT_DYNAMIC_CHECK_FN()
> +IMPLEMENT_DYNAMIC_BIND_FN(engine_bind_fn)
> diff --git a/tests/unit_tests/engine-key/openssl.cnf.in b/tests/unit_tests/engine-key/openssl.cnf.in
> new file mode 100644
> index 00000000..93edd483
> --- /dev/null
> +++ b/tests/unit_tests/engine-key/openssl.cnf.in
> @@ -0,0 +1,12 @@
> +HOME		= .
> +openssl_conf	= openssl_init
> +
> +[req]
> +[openssl_init]
> +engines		= engines_section
> +
> +[engines_section]
> +testengine	= testengine_section
> +
> +[testengine_section]
> +dynamic_path	= $ENV::srcdir/.libs/libtestengineSHREXT
> 

Somehow this turns into a autoconf/automake nightmare:

[testengine_section]
dynamic_path	= $ENV::srcdir/.libs/libtestengine`test .odule = .yes
SHREXTSHREXT echo .so || echo .dylib`

is what I end up in the openssl.cnf

The generated Makefile already has:

$(builddir)/%.cnf: $(srcdir)/%.cnf.in
 	sed 's/SHREXT/`test .$module = .yes && echo .so || echo .dylib`/' < $<
> $@

Maybe instead of using that use magic useing two different section like
testengineso and testenginedylib and then makeing the engine name in the
script dynamic?

Also for some reason the makefile still builds it now as .so instead of
.dylib and if I manually put the  full path in the openssl.cnf the test
works:

dynamic_path	=
/Users/arne/oss/openvpn-ossl/tests/unit_tests/engine-key/.libs/libtestengine.so

Any idea? I can also help to get this mess resolve but maybe you have a
quick idea what is wrong.

Arne
James Bottomley June 5, 2020, 4:28 p.m. | #2
On Fri, 2020-06-05 at 13:31 +0200, Arne Schwabe wrote:
[...]
> Somehow this turns into a autoconf/automake nightmare:

Heh, got to say autoconf is a bit of a nightmare for its more esoteric
features because the docs usually don't cover them and you end up
having to take the opinion of the internet, which is somewhat, er,
diverse.

> [testengine_section]
> dynamic_path	= $ENV::srcdir/.libs/libtestengine`test .odule =
> .yes SHREXTSHREXT echo .so || echo .dylib`
> 
> is what I end up in the openssl.cnf
> 
> The generated Makefile already has:
> 
> $(builddir)/%.cnf: $(srcdir)/%.cnf.in
>  	sed 's/SHREXT/`test .$module = .yes && echo .so || echo
> .dylib`/' < $<
> > $@

Hm, so SHREXT is dynamic on MAC platforms.  If I replace the single
quote with double, that should allow the shell expansion.

> Maybe instead of using that use magic useing two different section
> like testengineso and testenginedylib and then makeing the engine
> name in the script dynamic?

I'll investigate, but openssl has a fairly tight binding between the
name of the dynamic object and the name of the engine.  It's very
difficult (but not I perhaps impossible) to have an engine name
different from the DSO name.

> Also for some reason the makefile still builds it now as .so instead
> of .dylib

That's likely a libtool flag issue.  If you update this in Makefile.am:

libtestengine_la_LDFLAGS = -rpath /lib -avoid-version -module -shared -export-dynamic

does it build a .dylib?

>  and if I manually put the  full path in the openssl.cnf the
> test works:
> 
> dynamic_path	=
> /Users/arne/oss/openvpn-ossl/tests/unit_tests/engine-
> key/.libs/libtestengine.so
> 
> Any idea? I can also help to get this mess resolve but maybe you have
> a quick idea what is wrong.

My money would be on the idea that environment specifiers in openssl
conf files are only supported on UNIX.  I'll get it to do a full path
substitution on build to fix this.

James
James Bottomley June 5, 2020, 5:19 p.m. | #3
On Fri, 2020-06-05 at 09:28 -0700, James Bottomley wrote:
> On Fri, 2020-06-05 at 13:31 +0200, Arne Schwabe wrote:
> [...]
> > Somehow this turns into a autoconf/automake nightmare:
> 
> Heh, got to say autoconf is a bit of a nightmare for its more
> esoteric features because the docs usually don't cover them and you
> end up having to take the opinion of the internet, which is somewhat,
> er, diverse.
> 
> > [testengine_section]
> > dynamic_path	= $ENV::srcdir/.libs/libtestengine`test .odule
> > = .yes SHREXTSHREXT echo .so || echo .dylib`
> > 
> > is what I end up in the openssl.cnf
> > 
> > The generated Makefile already has:
> > 
> > $(builddir)/%.cnf: $(srcdir)/%.cnf.in
> >  	sed 's/SHREXT/`test .$module = .yes && echo .so || echo
> > .dylib`/' < $< $@
> 
> Hm, so SHREXT is dynamic on MAC platforms.  If I replace the single
> quote with double, that should allow the shell expansion.

The internet thinks the fix is to add eval to the hunk in configure.ac:

eval shrext=$shrext_cmds
AC_SUBST([shrext])

I tried it out and it seems to work.

> > Maybe instead of using that use magic useing two different section
> > like testengineso and testenginedylib and then makeing the engine
> > name in the script dynamic?
> 
> I'll investigate, but openssl has a fairly tight binding between the
> name of the dynamic object and the name of the engine.  It's very
> difficult (but not I perhaps impossible) to have an engine name
> different from the DSO name.
> 
> > Also for some reason the makefile still builds it now as .so
> > instead
> > of .dylib
> 
> That's likely a libtool flag issue.  If you update this in
> Makefile.am:
> 
> libtestengine_la_LDFLAGS = -rpath /lib -avoid-version -module -shared
> -export-dynamic
> 
> does it build a .dylib?

So if this works, with the eval above and full path substitution in the
openssl.cnf.in I think I might have a solution.

James
Arne Schwabe June 5, 2020, 10:48 p.m. | #4
Am 05.06.20 um 19:19 schrieb James Bottomley:
> On Fri, 2020-06-05 at 09:28 -0700, James Bottomley wrote:
>> On Fri, 2020-06-05 at 13:31 +0200, Arne Schwabe wrote:
>> [...]
>>> Somehow this turns into a autoconf/automake nightmare:
>>
>> Heh, got to say autoconf is a bit of a nightmare for its more
>> esoteric features because the docs usually don't cover them and you
>> end up having to take the opinion of the internet, which is somewhat,
>> er, diverse.
>>
>>> [testengine_section]
>>> dynamic_path	= $ENV::srcdir/.libs/libtestengine`test .odule
>>> = .yes SHREXTSHREXT echo .so || echo .dylib`
>>>
>>> is what I end up in the openssl.cnf
>>>
>>> The generated Makefile already has:
>>>
>>> $(builddir)/%.cnf: $(srcdir)/%.cnf.in
>>>  	sed 's/SHREXT/`test .$module = .yes && echo .so || echo
>>> .dylib`/' < $< $@
>>
>> Hm, so SHREXT is dynamic on MAC platforms.  If I replace the single
>> quote with double, that should allow the shell expansion.
> 
> The internet thinks the fix is to add eval to the hunk in configure.ac:
> 
> eval shrext=$shrext_cmds
> AC_SUBST([shrext])
> 
> I tried it out and it seems to work.

[testengine_section]
dynamic_path	= $ENV::srcdir/.libs/libtestengine.dylib

Yes, works here.

> 
>>> Maybe instead of using that use magic useing two different section
>>> like testengineso and testenginedylib and then makeing the engine
>>> name in the script dynamic?
>>
>> I'll investigate, but openssl has a fairly tight binding between the
>> name of the dynamic object and the name of the engine.  It's very
>> difficult (but not I perhaps impossible) to have an engine name
>> different from the DSO name.
>>
>>> Also for some reason the makefile still builds it now as .so
>>> instead
>>> of .dylib
>>
>> That's likely a libtool flag issue.  If you update this in
>> Makefile.am:
>>
>> libtestengine_la_LDFLAGS = -rpath /lib -avoid-version -module -shared
>> -export-dynamic
>>
>> does it build a .dylib?
> 
> So if this works, with the eval above and full path substitution in the
> openssl.cnf.in I think I might have a solution.

Unfortunately not. Still creates the .so file.  If everything fails we
might just do a if mac then cp from .so to .dylib as part of the test
run. It is ugly but might be better than to wait forever on this patch.

Arne
Gert Doering June 6, 2020, 8:41 a.m. | #5
Hi,

On Sat, Jun 06, 2020 at 12:48:36AM +0200, Arne Schwabe wrote:
> > So if this works, with the eval above and full path substitution in the
> > openssl.cnf.in I think I might have a solution.
> 
> Unfortunately not. Still creates the .so file.  If everything fails we
> might just do a if mac then cp from .so to .dylib as part of the test
> run. It is ugly but might be better than to wait forever on this patch.

Naive question - can we not just use a .so on Mac?  Like, consistent,
build .so, request .so from openssl.cnf?

(Haven't dug into shlib weirdness on MacOS yet.  But if you want, I can
tell stories about shlib on AIX.  Which will make you ask for lots of 
alcoholic beverages so you can forget again...)

gert
Arne Schwabe June 6, 2020, 10:43 a.m. | #6
Am 06.06.20 um 10:41 schrieb Gert Doering:
> Hi,
> 
> On Sat, Jun 06, 2020 at 12:48:36AM +0200, Arne Schwabe wrote:
>>> So if this works, with the eval above and full path substitution in the
>>> openssl.cnf.in I think I might have a solution.
>>
>> Unfortunately not. Still creates the .so file.  If everything fails we
>> might just do a if mac then cp from .so to .dylib as part of the test
>> run. It is ugly but might be better than to wait forever on this patch.
> 
> Naive question - can we not just use a .so on Mac?  Like, consistent,
> build .so, request .so from openssl.cnf?
> 
> (Haven't dug into shlib weirdness on MacOS yet.  But if you want, I can
> tell stories about shlib on AIX.  Which will make you ask for lots of 
> alcoholic beverages so you can forget again...)

I thought I tested that and it didn't work but I tested it again and
that also works.

Arne
Gert Doering June 6, 2020, 11:28 a.m. | #7
Hi,

On Sat, Jun 06, 2020 at 12:43:10PM +0200, Arne Schwabe wrote:
> > Naive question - can we not just use a .so on Mac?  Like, consistent,
> > build .so, request .so from openssl.cnf?
> 
> I thought I tested that and it didn't work but I tested it again and
> that also works.

Then let's just do that, and simplifiy the test stuff :-)

(Building a "production quality" OpenSSL engine would need to do what
the platform wants, but for "this is really just to test the OpenVPN code",
I think "less autoconf hoops is better")

gert
James Bottomley June 6, 2020, 3:22 p.m. | #8
On Sat, 2020-06-06 at 13:28 +0200, Gert Doering wrote:
> Hi,
> 
> On Sat, Jun 06, 2020 at 12:43:10PM +0200, Arne Schwabe wrote:
> > > Naive question - can we not just use a .so on Mac?  Like,
> > > consistent, build .so, request .so from openssl.cnf?
> > 
> > I thought I tested that and it didn't work but I tested it again
> > and that also works.
> 
> Then let's just do that, and simplifiy the test stuff :-)
> 
> (Building a "production quality" OpenSSL engine would need to do what
> the platform wants, but for "this is really just to test the OpenVPN
> code", I think "less autoconf hoops is better")

Well, I am intellectually curious since I maintain the TPM engine for
openssl.  But Macs don't have TPMs so no-one is ever going to complain
...

The only remaining problem is the lack of environment variable support
in MAC openssl which I'll fix by using the absolute path.  If it works
I think below is the replacement patch.

James

---8>8>8><8<8<8----

From: James Bottomley <James.Bottomley@HansenPartnership.com>
Subject: [PATCH v7 3/3] 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 <James.Bottomley@HansenPartnership.com>

---
v7: add absolute path instead of env variable in openssl.cnf (MacOS)
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                                  |   6 ++
 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 +++
 6 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..3e8a541d 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1387,6 +1387,11 @@ 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"])
+
+module=yes
+eval shrext=$shrext_cmds
+AC_SUBST([shrext])
 
 sampledir="\$(docdir)/sample"
 AC_SUBST([plugindir])
@@ -1448,6 +1453,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/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 00000000..e8a473a8
--- /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@|;s|ABSBUILDDIR|$(abs_builddir)|" < $< > $@
+
+libtestengine_la_SOURCES = libtestengine.c
+libtestengine_la_LDFLAGS = @TEST_LDFLAGS@ -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 00000000..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 returning different messages and update the
+# test accordingly
+grep -q 'X509_check_private_key:key values mismatch' log.txt || { echo "Key mismatch not detected"; exit 1; }
+
+# now look for the engine prints (these are under our control)
+grep -q 'ENGINE: engine_init called' log.txt || { echo "Engine initialization not detected"; exit 1; }
+grep -q 'ENGINE: engine_load_key called' log.txt || { echo "Key was not loaded from engine"; exit 1; }
+grep -q "ENGINE: engine_load_key got password ${password}" log.txt || { echo "Key password was not retrieved by the engine"; exit 1; }
+exit 0
diff --git a/tests/unit_tests/engine-key/libtestengine.c b/tests/unit_tests/engine-key/libtestengine.c
new file mode 100644
index 00000000..46ec1e33
--- /dev/null
+++ b/tests/unit_tests/engine-key/libtestengine.c
@@ -0,0 +1,101 @@
+#include <string.h>
+#include <openssl/engine.h>
+#include <openssl/evp.h>
+#include <openssl/pem.h>
+
+static char *engine_id = "testengine";
+static char *engine_name = "Engine for testing openvpn engine key support";
+
+static int is_initialized = 0;
+
+static int engine_init(ENGINE *e)
+{
+	is_initialized = 1;
+	fprintf(stderr, "ENGINE: engine_init called\n");
+	return 1;
+}
+
+static int engine_finish(ENGINE *e)
+{
+	fprintf(stderr, "ENGINE: engine_finsh called\n");
+	is_initialized = 0;
+	return 1;
+}
+
+static EVP_PKEY *engine_load_key(ENGINE *e, const char *key_id,
+				 UI_METHOD *ui_method, void *cb_data)
+{
+	BIO *b;
+	EVP_PKEY *pkey;
+	PKCS8_PRIV_KEY_INFO *p8inf;
+	UI *ui;
+	char auth[256];
+
+	fprintf(stderr, "ENGINE: engine_load_key called\n");
+
+	if (!is_initialized) {
+		fprintf(stderr, "Load Key called without correct initialization\n");
+		return NULL;
+	}
+	b = BIO_new_file(key_id, "r");
+	if (!b) {
+		fprintf(stderr, "File %s does not exist or cannot be read\n", key_id);
+		return 0;
+	}
+	/* Basically read an EVP_PKEY private key file with different
+	 * PEM guards --- we are a test engine */
+	p8inf = PEM_ASN1_read_bio((d2i_of_void *)d2i_PKCS8_PRIV_KEY_INFO,
+				 "TEST ENGINE KEY", b,
+				 NULL, NULL, NULL);
+	BIO_free(b);
+	if (!p8inf) {
+		fprintf(stderr, "Failed to read engine private key\n");
+		return NULL;
+	}
+	pkey = EVP_PKCS82PKEY(p8inf);
+
+	/* now we have a private key, pretend it had a password
+	 * this verifies the password makes it through openvpn OK */
+	ui = UI_new();
+
+	if (ui_method)
+		UI_set_method(ui, ui_method);
+
+	UI_add_user_data(ui, cb_data);
+
+	if (UI_add_input_string(ui, "enter test engine key",
+				UI_INPUT_FLAG_DEFAULT_PWD,
+				auth, 0, sizeof(auth)) == 0) {
+		fprintf(stderr, "UI_add_input_string failed\n");
+		goto out;
+	}
+
+	if (UI_process(ui)) {
+		fprintf(stderr, "UI_process failed\n");
+		goto out;
+	}
+
+	fprintf(stderr, "ENGINE: engine_load_key got password %s\n", auth);
+
+ out:
+	UI_free(ui);
+
+	return pkey;
+}
+
+
+static int engine_bind_fn(ENGINE *e, const char *id)
+{
+	if (id && strcmp(id, engine_id) != 0)
+		return 0;
+	if (!ENGINE_set_id(e, engine_id) ||
+	    !ENGINE_set_name(e, engine_name) ||
+	    !ENGINE_set_init_function(e, engine_init) ||
+	    !ENGINE_set_finish_function(e, engine_finish) ||
+	    !ENGINE_set_load_privkey_function(e, engine_load_key))
+		return 0;
+	return 1;
+}
+
+IMPLEMENT_DYNAMIC_CHECK_FN()
+IMPLEMENT_DYNAMIC_BIND_FN(engine_bind_fn)
diff --git a/tests/unit_tests/engine-key/openssl.cnf.in b/tests/unit_tests/engine-key/openssl.cnf.in
new file mode 100644
index 00000000..40d328ce
--- /dev/null
+++ b/tests/unit_tests/engine-key/openssl.cnf.in
@@ -0,0 +1,12 @@
+HOME		= .
+openssl_conf	= openssl_init
+
+[req]
+[openssl_init]
+engines		= engines_section
+
+[engines_section]
+testengine	= testengine_section
+
+[testengine_section]
+dynamic_path	= ABSBUILDDIR/.libs/libtestengineSHREXT
Arne Schwabe June 6, 2020, 4:20 p.m. | #9
Am 06.06.20 um 17:22 schrieb James Bottomley:
> On Sat, 2020-06-06 at 13:28 +0200, Gert Doering wrote:
>> Hi,
>>
>> On Sat, Jun 06, 2020 at 12:43:10PM +0200, Arne Schwabe wrote:
>>>> Naive question - can we not just use a .so on Mac?  Like,
>>>> consistent, build .so, request .so from openssl.cnf?
>>>
>>> I thought I tested that and it didn't work but I tested it again
>>> and that also works.
>>
>> Then let's just do that, and simplifiy the test stuff :-)
>>
>> (Building a "production quality" OpenSSL engine would need to do what
>> the platform wants, but for "this is really just to test the OpenVPN
>> code", I think "less autoconf hoops is better")
> 
> Well, I am intellectually curious since I maintain the TPM engine for
> openssl.  But Macs don't have TPMs so no-one is ever going to complain
> ...
> 
> The only remaining problem is the lack of environment variable support
> in MAC openssl which I'll fix by using the absolute path.  If it works
> I think below is the replacement patch.
> 

Looks more like one step forward, two back:
openssl.cnf absolute path looks good:

dynamic_path	=
/Users/arne/oss/openvpn-ossl/tests/unit_tests/engine-key/.libs/libtestengine.so

But now it has .so there. Alos the .libs directory now has neither .so
nor .dylib


engine-key% find .
.
./libtestengine_la-libtestengine.o
./Makefile
./libtestengine.la
./libtestengine_la-libtestengine.lo
./openssl.cnf
./log.txt
./.deps
./.deps/libtestengine_la-libtestengine.Plo
./.libs
./.libs/libtestengine_la-libtestengine.o
./.libs/libtestengine.la
./.libs/libtestengine.a
./passwd
./client.key


(This is an out of tree build so only generated files here)

Make log:

make check
[...]
Making check in engine-key
Makefile:451: .deps/libtestengine_la-libtestengine.Plo: No such file or
directory
/Applications/Xcode.app/Contents/Developer/usr/bin/make  libtestengine.la
/bin/sh ../../../libtool  --tag=CC   --mode=compile clang
-DHAVE_CONFIG_H -I.
-I../../../../openvpn-git/tests/unit_tests/engine-key -I../../..
-I../../../include    -I/usr/local/opt/openssl@1.1/include
-I../../../../openvpn-git/include
-I/usr/local/Cellar/cmocka/1.1.5/include -I -I -Wall
-Wno-unused-parameter -Wno-unused-function -O2 -g -Wall -std=c99 -MT
libtestengine_la-libtestengine.lo -MD -MP -MF
.deps/libtestengine_la-libtestengine.Tpo -c -o
libtestengine_la-libtestengine.lo `test -f 'libtestengine.c' || echo
'../../../../openvpn-git/tests/unit_tests/engine-key/'`libtestengine.c
libtool: compile:  clang -DHAVE_CONFIG_H -I.
-I../../../../openvpn-git/tests/unit_tests/engine-key -I../../..
-I../../../include -I/usr/local/opt/openssl@1.1/include
-I../../../../openvpn-git/include
-I/usr/local/Cellar/cmocka/1.1.5/include -I -I -Wall
-Wno-unused-parameter -Wno-unused-function -O2 -g -Wall -std=c99 -MT
libtestengine_la-libtestengine.lo -MD -MP -MF
.deps/libtestengine_la-libtestengine.Tpo -c
../../../../openvpn-git/tests/unit_tests/engine-key/libtestengine.c
-fno-common -DPIC -o .libs/libtestengine_la-libtestengine.o
libtool: compile:  clang -DHAVE_CONFIG_H -I.
-I../../../../openvpn-git/tests/unit_tests/engine-key -I../../..
-I../../../include -I/usr/local/opt/openssl@1.1/include
-I../../../../openvpn-git/include
-I/usr/local/Cellar/cmocka/1.1.5/include -I -I -Wall
-Wno-unused-parameter -Wno-unused-function -O2 -g -Wall -std=c99 -MT
libtestengine_la-libtestengine.lo -MD -MP -MF
.deps/libtestengine_la-libtestengine.Tpo -c
../../../../openvpn-git/tests/unit_tests/engine-key/libtestengine.c -o
libtestengine_la-libtestengine.o >/dev/null 2>&1
mv -f .deps/libtestengine_la-libtestengine.Tpo
.deps/libtestengine_la-libtestengine.Plo
/bin/sh ../../../libtool  --tag=CC   --mode=link clang
-I/usr/local/opt/openssl@1.1/include   -I../../../../openvpn-git/include
-I/usr/local/Cellar/cmocka/1.1.5/include -I -I -Wall
-Wno-unused-parameter -Wno-unused-function -O2 -g -Wall -std=c99
-L/usr/local/opt/openssl@1.1/lib -lcrypto -lssl
-L/usr/local/Cellar/cmocka/1.1.5/lib -lcmocka -avoid-version -module
-shared -export-dynamic  -o libtestengine.la
libtestengine_la-libtestengine.lo  -lresolv
libtool: link: ar cru .libs/libtestengine.a
.libs/libtestengine_la-libtestengine.o
libtool: link: ranlib .libs/libtestengine.a
libtool: link: ( cd ".libs" && rm -f "libtestengine.la" && ln -s
"../libtestengine.la" "libtestengine.la" )
/Applications/Xcode.app/Contents/Developer/usr/bin/make  check-TESTS
sed
"s|SHREXT|.so|;s|ABSBUILDDIR|/Users/arne/oss/openvpn-ossl/tests/unit_tests/engine-key|"
< ../../../../openvpn-git/tests/unit_tests/engine-key/openssl.cnf.in >
openssl.cnf
Gert Doering June 6, 2020, 6:16 p.m. | #10
Hi,

On Sat, Jun 06, 2020 at 08:22:51AM -0700, James Bottomley wrote:
> The only remaining problem is the lack of environment variable support
> in MAC openssl which I'll fix by using the absolute path.  If it works
> I think below is the replacement patch.

That patch seems to go the "complicated" route, not the "easy" one...?

> +eval shrext=$shrext_cmds
> +AC_SUBST([shrext])

... I hoped we can get rid of this again...

And maybe then we can just have "libtestengine.so" target in Makefile?

> +[testengine_section]
> +dynamic_path	= ABSBUILDDIR/.libs/libtestengineSHREXT

And no more SHREXT here either...

gert
James Bottomley June 6, 2020, 7:10 p.m. | #11
On Sat, 2020-06-06 at 20:16 +0200, Gert Doering wrote:
> Hi,
> 
> On Sat, Jun 06, 2020 at 08:22:51AM -0700, James Bottomley wrote:
> > The only remaining problem is the lack of environment variable
> > support
> > in MAC openssl which I'll fix by using the absolute path.  If it
> > works
> > I think below is the replacement patch.
> 
> That patch seems to go the "complicated" route, not the "easy"
> one...?
> 
> > +eval shrext=$shrext_cmds
> > +AC_SUBST([shrext])
> 
> ... I hoped we can get rid of this again...

In theory I can get around this using the libtool -shrext .so on the
command line.  Someone would have to check on windows that this is
acceptable because they really like .dll as the extension.  If it works
on windows then the below patch should do it.

James

> And maybe then we can just have "libtestengine.so" target in
> Makefile?
> 
> > +[testengine_section]
> > +dynamic_path	= ABSBUILDDIR/.libs/libtestengineSHREXT
> 
> And no more SHREXT here either...
> 
> gert

---8>8>8><8<8<8---
From 5e1fb464d4669882d11f52d314e7830d556c8f12 Mon Sep 17 00:00:00 2001
From: James Bottomley <James.Bottomley@HansenPartnership.com>
Date: Wed, 24 Jan 2018 17:14:19 -0800
Subject: [PATCH] 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 <James.Bottomley@HansenPartnership.com>

---
v6: add absolute path instead of env variable in openssl.cnf (MacOS)
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                                  |   2 +
 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 +++
 6 files changed, 172 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..53b7a967 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1387,6 +1387,7 @@ 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"])
 
 sampledir="\$(docdir)/sample"
 AC_SUBST([plugindir])
@@ -1448,6 +1449,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/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 00000000..857459ed
--- /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|ABSBUILDDIR|$(abs_builddir)|" < $< > $@
+
+libtestengine_la_SOURCES = libtestengine.c
+libtestengine_la_LDFLAGS = @TEST_LDFLAGS@ -rpath /lib -shrext .so
+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 00000000..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 returning different messages and update the
+# test accordingly
+grep -q 'X509_check_private_key:key values mismatch' log.txt || { echo "Key mismatch not detected"; exit 1; }
+
+# now look for the engine prints (these are under our control)
+grep -q 'ENGINE: engine_init called' log.txt || { echo "Engine initialization not detected"; exit 1; }
+grep -q 'ENGINE: engine_load_key called' log.txt || { echo "Key was not loaded from engine"; exit 1; }
+grep -q "ENGINE: engine_load_key got password ${password}" log.txt || { echo "Key password was not retrieved by the engine"; exit 1; }
+exit 0
diff --git a/tests/unit_tests/engine-key/libtestengine.c b/tests/unit_tests/engine-key/libtestengine.c
new file mode 100644
index 00000000..46ec1e33
--- /dev/null
+++ b/tests/unit_tests/engine-key/libtestengine.c
@@ -0,0 +1,101 @@
+#include <string.h>
+#include <openssl/engine.h>
+#include <openssl/evp.h>
+#include <openssl/pem.h>
+
+static char *engine_id = "testengine";
+static char *engine_name = "Engine for testing openvpn engine key support";
+
+static int is_initialized = 0;
+
+static int engine_init(ENGINE *e)
+{
+	is_initialized = 1;
+	fprintf(stderr, "ENGINE: engine_init called\n");
+	return 1;
+}
+
+static int engine_finish(ENGINE *e)
+{
+	fprintf(stderr, "ENGINE: engine_finsh called\n");
+	is_initialized = 0;
+	return 1;
+}
+
+static EVP_PKEY *engine_load_key(ENGINE *e, const char *key_id,
+				 UI_METHOD *ui_method, void *cb_data)
+{
+	BIO *b;
+	EVP_PKEY *pkey;
+	PKCS8_PRIV_KEY_INFO *p8inf;
+	UI *ui;
+	char auth[256];
+
+	fprintf(stderr, "ENGINE: engine_load_key called\n");
+
+	if (!is_initialized) {
+		fprintf(stderr, "Load Key called without correct initialization\n");
+		return NULL;
+	}
+	b = BIO_new_file(key_id, "r");
+	if (!b) {
+		fprintf(stderr, "File %s does not exist or cannot be read\n", key_id);
+		return 0;
+	}
+	/* Basically read an EVP_PKEY private key file with different
+	 * PEM guards --- we are a test engine */
+	p8inf = PEM_ASN1_read_bio((d2i_of_void *)d2i_PKCS8_PRIV_KEY_INFO,
+				 "TEST ENGINE KEY", b,
+				 NULL, NULL, NULL);
+	BIO_free(b);
+	if (!p8inf) {
+		fprintf(stderr, "Failed to read engine private key\n");
+		return NULL;
+	}
+	pkey = EVP_PKCS82PKEY(p8inf);
+
+	/* now we have a private key, pretend it had a password
+	 * this verifies the password makes it through openvpn OK */
+	ui = UI_new();
+
+	if (ui_method)
+		UI_set_method(ui, ui_method);
+
+	UI_add_user_data(ui, cb_data);
+
+	if (UI_add_input_string(ui, "enter test engine key",
+				UI_INPUT_FLAG_DEFAULT_PWD,
+				auth, 0, sizeof(auth)) == 0) {
+		fprintf(stderr, "UI_add_input_string failed\n");
+		goto out;
+	}
+
+	if (UI_process(ui)) {
+		fprintf(stderr, "UI_process failed\n");
+		goto out;
+	}
+
+	fprintf(stderr, "ENGINE: engine_load_key got password %s\n", auth);
+
+ out:
+	UI_free(ui);
+
+	return pkey;
+}
+
+
+static int engine_bind_fn(ENGINE *e, const char *id)
+{
+	if (id && strcmp(id, engine_id) != 0)
+		return 0;
+	if (!ENGINE_set_id(e, engine_id) ||
+	    !ENGINE_set_name(e, engine_name) ||
+	    !ENGINE_set_init_function(e, engine_init) ||
+	    !ENGINE_set_finish_function(e, engine_finish) ||
+	    !ENGINE_set_load_privkey_function(e, engine_load_key))
+		return 0;
+	return 1;
+}
+
+IMPLEMENT_DYNAMIC_CHECK_FN()
+IMPLEMENT_DYNAMIC_BIND_FN(engine_bind_fn)
diff --git a/tests/unit_tests/engine-key/openssl.cnf.in b/tests/unit_tests/engine-key/openssl.cnf.in
new file mode 100644
index 00000000..5eda9fa9
--- /dev/null
+++ b/tests/unit_tests/engine-key/openssl.cnf.in
@@ -0,0 +1,12 @@
+HOME		= .
+openssl_conf	= openssl_init
+
+[req]
+[openssl_init]
+engines		= engines_section
+
+[engines_section]
+testengine	= testengine_section
+
+[testengine_section]
+dynamic_path	= ABSBUILDDIR/.libs/libtestengine.so
Gert Doering June 6, 2020, 7:46 p.m. | #12
Hi,

On Sat, Jun 06, 2020 at 12:10:47PM -0700, James Bottomley wrote:
> > > +eval shrext=$shrext_cmds
> > > +AC_SUBST([shrext])
> > 
> > ... I hoped we can get rid of this again...
> 
> In theory I can get around this using the libtool -shrext .so on the
> command line.  Someone would have to check on windows that this is
> acceptable because they really like .dll as the extension.  If it works
> on windows then the below patch should do it.

Oh, Windows.

Production builds for windows are done with MinGW on Ubuntu (cross-builds) 
and we do not run any unit tests there.

Lev does run MSVC builds, but he's not using "make", so I'm not sure he
ever runs "make check"...

What would be needed to actually run this test on windows?  Or any of our
tests?  "MinGW/Cygwin on windows"?  Has anyone built with that since XP?
(My first openvpn.exe was built with MinGW/Cygwin, but that was painful)

gert
Gert Doering June 21, 2020, 8:53 a.m. | #13
Hi,

On Thu, May 28, 2020 at 03:59:20PM -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.
> 
> Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> 
> ---
> 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
> ---

So, how can we continue with this one?  The engine key code is in, the
unit test code not.

Arne, James, can we converge on something here?

I would like something "as simple as possible", aka "little configure magic"
- if that means "MacOS engine is built and loaded as .so and not .dynlib,
but the test passes" I would prefer it over "more configure.ac magic" -
but if this is what it takes, I'm fine as well.

gert
James Bottomley June 21, 2020, 3:10 p.m. | #14
On Sun, 2020-06-21 at 10:53 +0200, Gert Doering wrote:
> Hi,
> 
> On Thu, May 28, 2020 at 03:59:20PM -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.
> > 
> > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.c
> > om>
> > 
> > ---
> > 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
> > ---
> 
> So, how can we continue with this one?  The engine key code is in,
> the unit test code not.
> 
> Arne, James, can we converge on something here?

Could someone just test the proposed updated v6 patch on a Mac?

https://sourceforge.net/p/openvpn/mailman/message/37031113/

  If it works then we have something that functions on both platforms
and we don't need to worry about windows until someone runs unit tests
there.

> I would like something "as simple as possible", aka "little configure
> magic" - if that means "MacOS engine is built and loaded as .so and
> not .dynlib, but the test passes" I would prefer it over "more
> configure.ac magic"
> -
> but if this is what it takes, I'm fine as well.

Right, that's what the patch in the url does: uses .so on both mac and
linux.

James
Gert Doering June 22, 2020, 4:23 p.m. | #15
Hi,

On Sun, Jun 21, 2020 at 08:10:34AM -0700, James Bottomley wrote:
> > Arne, James, can we converge on something here?
> 
> Could someone just test the proposed updated v6 patch on a Mac?
> 
> https://sourceforge.net/p/openvpn/mailman/message/37031113/

Took Arne and me half a day ("the macos build slave did not have cmocka,
and it went downhill from there") but I can now confirm that it works
and passes the tests.

This is on a Mojave MacMini with "brew" openssl 1.1.1c and your patch
"v6":

Making check in engine-key
/Applications/Xcode.app/Contents/Developer/usr/bin/make  libtestengine.la
...
/bin/sh ../../../libtool  --tag=CC   --mode=link gcc -I/usr/local/Cellar/openssl@1.1/1.1.1c/include   -I../../../../openvpn.git/include -I/usr/local/Cellar/cmocka/1.1.5/include -I -I -Wall -Wno-unused-parameter -Wno-unused-function -g -O2 -std=c99 -L/usr/local/Cellar/openssl@1.1/1.1.1c/lib -lcrypto -lssl   -L/usr/local/Cellar/cmocka/1.1.5/lib -lcmocka -rpath /lib -shrext .so  -o libtestengine.la  libtestengine_la-libtestengine.lo  -lresolv 
libtool: link: gcc -dynamiclib -Wl,-undefined -Wl,dynamic_lookup -o .libs/libtestengine.0.so  .libs/libtestengine_la-libtestengine.o   -L/usr/local/Cellar/openssl@1.1/1.1.1c/lib -lcrypto -lssl -L/usr/local/Cellar/cmocka/1.1.5/lib -lcmocka -lresolv  -g -O2   -install_name  /lib/libtestengine.0.so -compatibility_version 1 -current_version 1.0 -Wl,-single_module
libtool: link: (cd ".libs" && rm -f "libtestengine.so" && ln -s "libtestengine.0.so" "libtestengine.so")
libtool: link: ar cru .libs/libtestengine.a  libtestengine_la-libtestengine.o
libtool: link: ranlib .libs/libtestengine.a
libtool: link: ( cd ".libs" && rm -f "libtestengine.la" && ln -s "../libtestengine.la" "libtestengine.la" )
/Applications/Xcode.app/Contents/Developer/usr/bin/make  check-TESTS
sed "s|ABSBUILDDIR|/Users/gert/openvpn-build-mojave/tests/unit_tests/engine-key|" < ../../../../openvpn.git/tests/unit_tests/engine-key/openssl.cnf.in > openssl.cnf
PASS: check_engine_keys.sh
=============
1 test passed
=============

(I'm not sure where it is actually *calling* the "check_engine_keys.sh",
but it says "test passed!" and I see ".so" up there)

[..]
> Right, that's what the patch in the url does: uses .so on both mac and
> linux.

I got all confused with your "v6" patch and your "v7" patch, which did
other things, and but did not have a "v6" in the history :-)

*This* patch definitely looks like it's doing the right thing on MacOS.

gert
James Bottomley June 22, 2020, 5:06 p.m. | #16
On Mon, 2020-06-22 at 18:23 +0200, Gert Doering wrote:
> Hi,
> 
> On Sun, Jun 21, 2020 at 08:10:34AM -0700, James Bottomley wrote:
> > > Arne, James, can we converge on something here?
> > 
> > Could someone just test the proposed updated v6 patch on a Mac?
> > 
> > https://sourceforge.net/p/openvpn/mailman/message/37031113/
> 
> Took Arne and me half a day ("the macos build slave did not have
> cmocka, and it went downhill from there") but I can now confirm that
> it works and passes the tests.
> 
> This is on a Mojave MacMini with "brew" openssl 1.1.1c and your patch
> "v6":
> 
> Making check in engine-key
> /Applications/Xcode.app/Contents/Developer/usr/bin/make  libtestengin
> e.la
> ...
> /bin/sh ../../../libtool  --tag=CC   --mode=link gcc -I/usr/local/Cel
> lar/openssl@1.1/1.1.1c/include   -I../../../../openvpn.git/include
> -I/usr/local/Cellar/cmocka/1.1.5/include -I -I -Wall -Wno-unused-
> parameter -Wno-unused-function -g -O2 -std=c99 -L/usr/local/Cellar/op
> enssl@1.1/1.1.1c/lib -lcrypto -lssl   -
> L/usr/local/Cellar/cmocka/1.1.5/lib -lcmocka -rpath /lib -shrext
> .so  -o libtestengine.la  libtestengine_la-libtestengine.lo  -
> lresolv 
> libtool: link: gcc -dynamiclib -Wl,-undefined -Wl,dynamic_lookup -o
> .libs/libtestengine.0.so  .libs/libtestengine_la-libtestengine.o   -L
> /usr/local/Cellar/openssl@1.1/1.1.1c/lib -lcrypto -lssl
> -L/usr/local/Cellar/cmocka/1.1.5/lib -lcmocka -lresolv  -g -O2   -
> install_name  /lib/libtestengine.0.so -compatibility_version 1
> -current_version 1.0 -Wl,-single_module
> libtool: link: (cd ".libs" && rm -f "libtestengine.so" && ln -s
> "libtestengine.0.so" "libtestengine.so")
> libtool: link: ar cru .libs/libtestengine.a  libtestengine_la-
> libtestengine.o
> libtool: link: ranlib .libs/libtestengine.a
> libtool: link: ( cd ".libs" && rm -f "libtestengine.la" && ln -s
> "../libtestengine.la" "libtestengine.la" )
> /Applications/Xcode.app/Contents/Developer/usr/bin/make  check-TESTS
> sed "s|ABSBUILDDIR|/Users/gert/openvpn-build-
> mojave/tests/unit_tests/engine-key|" <
> ../../../../openvpn.git/tests/unit_tests/engine-key/openssl.cnf.in >
> openssl.cnf
> PASS: check_engine_keys.sh
> =============
> 1 test passed
> =============
> 
> (I'm not sure where it is actually *calling* the
> "check_engine_keys.sh",
> but it says "test passed!" and I see ".so" up there)

Yes, it's using the standard test runner, so if it says PASS/FAIL:
<script> it definitely called <script>

> [..]
> > Right, that's what the patch in the url does: uses .so on both mac
> > and linux.
> 
> I got all confused with your "v6" patch and your "v7" patch, which
> did other things, and but did not have a "v6" in the history :-)
> 
> *This* patch definitely looks like it's doing the right thing on
> MacOS.

Great, thanks ... I'll rewrap it and submit as a separate patch email
with cover letter.

James
Gert Doering June 22, 2020, 5:28 p.m. | #17
Hi,

On Mon, Jun 22, 2020 at 10:06:44AM -0700, James Bottomley wrote:
> > [..]
> > > Right, that's what the patch in the url does: uses .so on both mac
> > > and linux.
> > 
> > I got all confused with your "v6" patch and your "v7" patch, which
> > did other things, and but did not have a "v6" in the history :-)
> > 
> > *This* patch definitely looks like it's doing the right thing on
> > MacOS.
> 
> Great, thanks ... I'll rewrap it and submit as a separate patch email
> with cover letter.

I was about to just merge what I have, but it exploded on me on FreeBSD
now - and I can't really figure out why (yet)...

libtool: link: (cd ".libs" && rm -f "libtestengine.so.0" && ln -s "libtestengine.so.0.0.0" "libtestengine.so.0")
libtool: link: (cd ".libs" && rm -f "libtestengine.so" && ln -s "libtestengine.so.0.0.0" "libtestengine.so")
libtool: link: ar cru .libs/libtestengine.a  libtestengine_la-libtestengine.o
libtool: link: ranlib .libs/libtestengine.a
libtool: link: ( cd ".libs" && rm -f "libtestengine.la" && ln -s "../libtestengine.la" "libtestengine.la" )
make  check-TESTS
make[5]: don't know how to make openssl.cnf. Stop

make[5]: stopped in /home/gert/src/openvpn-maint/test-build-master-fbsd/tests/unit_tests/engine-key


it seems as if there is an implicit rule "somewhere" that Linux make has
and FreeBSD make not how to build openssl.cnf from $(srcdir)/openssl.cnf.in
- on Linux, when I remove "openssl.cnf" from the target dir and re-run
"make check", I get

make[5]: Entering directory '/rhome/gert/src/openvpn-maint/test-build-master-ossl111/tests/unit_tests/engine-key'
sed "s|ABSBUILDDIR|/rhome/gert/src/openvpn-maint/test-build-master-ossl111/tests/unit_tests/engine-key|" < ../../../../openvpn/tests/unit_tests/engine-key/openssl.cnf.in > openssl.cnf
PASS: check_engine_keys.sh


Ah.  If I run "gmake check", it also passes on FreeBSD...

gmake[5]: Entering directory '/home/gert/src/openvpn-maint/test-build-master-fbsd/tests/unit_tests/engine-key'
sed "s|ABSBUILDDIR|/home/gert/src/openvpn-maint/test-build-master-fbsd/tests/unit_tests/engine-key|" < ../../../../openvpn/tests/unit_tests/engine-key/openssl.cnf.in > openssl.cnf
PASS: check_engine_keys.sh
=============
1 test passed
=============

... so the engine part *itself* is fine, just the "openssl.cnf from .in"
doesn't work with BSD make...


We need a v7 (or v8?) :-(

gert
James Bottomley June 22, 2020, 6:28 p.m. | #18
On Mon, 2020-06-22 at 19:28 +0200, Gert Doering wrote:
> Hi,
> 
> On Mon, Jun 22, 2020 at 10:06:44AM -0700, James Bottomley wrote:
> > > [..]
> > > > Right, that's what the patch in the url does: uses .so on both
> > > > mac
> > > > and linux.
> > > 
> > > I got all confused with your "v6" patch and your "v7" patch,
> > > which
> > > did other things, and but did not have a "v6" in the history :-)
> > > 
> > > *This* patch definitely looks like it's doing the right thing on
> > > MacOS.
> > 
> > Great, thanks ... I'll rewrap it and submit as a separate patch
> > email
> > with cover letter.
> 
> I was about to just merge what I have, but it exploded on me on
> FreeBSD
> now - and I can't really figure out why (yet)...
> 
> libtool: link: (cd ".libs" && rm -f "libtestengine.so.0" && ln -s
> "libtestengine.so.0.0.0" "libtestengine.so.0")
> libtool: link: (cd ".libs" && rm -f "libtestengine.so" && ln -s
> "libtestengine.so.0.0.0" "libtestengine.so")
> libtool: link: ar cru .libs/libtestengine.a  libtestengine_la-
> libtestengine.o
> libtool: link: ranlib .libs/libtestengine.a
> libtool: link: ( cd ".libs" && rm -f "libtestengine.la" && ln -s
> "../libtestengine.la" "libtestengine.la" )
> make  check-TESTS
> make[5]: don't know how to make openssl.cnf. Stop
> 
> make[5]: stopped in /home/gert/src/openvpn-maint/test-build-master-
> fbsd/tests/unit_tests/engine-key
> 
> 
> it seems as if there is an implicit rule "somewhere" that Linux make
> has
> and FreeBSD make not how to build openssl.cnf from
> $(srcdir)/openssl.cnf.in
> - on Linux, when I remove "openssl.cnf" from the target dir and re-
> run
> "make check", I get
> 
> make[5]: Entering directory '/rhome/gert/src/openvpn-maint/test-
> build-master-ossl111/tests/unit_tests/engine-key'
> sed "s|ABSBUILDDIR|/rhome/gert/src/openvpn-maint/test-build-master-
> ossl111/tests/unit_tests/engine-key|" <
> ../../../../openvpn/tests/unit_tests/engine-key/openssl.cnf.in >
> openssl.cnf
> PASS: check_engine_keys.sh
> 
> 
> Ah.  If I run "gmake check", it also passes on FreeBSD...
> 
> gmake[5]: Entering directory '/home/gert/src/openvpn-maint/test-
> build-master-fbsd/tests/unit_tests/engine-key'
> sed "s|ABSBUILDDIR|/home/gert/src/openvpn-maint/test-build-master-
> fbsd/tests/unit_tests/engine-key|" <
> ../../../../openvpn/tests/unit_tests/engine-key/openssl.cnf.in >
> openssl.cnf
> PASS: check_engine_keys.sh
> =============
> 1 test passed
> =============
> 
> ... so the engine part *itself* is fine, just the "openssl.cnf from
> .in" doesn't work with BSD make...
> 
> 
> We need a v7 (or v8?) :-(

That will be my fault.  I assumed automake always ran with gnu make, so
the .cnf rule is in gnu make syntax at the bottom of (Makefile.am):

$(builddir)/%.cnf: $(srcdir)/%.cnf.in
	sed "s|ABSBUILDDIR|$(abs_builddir)|" < $< > $@


If I just spell everything out and don't use the percent wildcard, it
should work even on legacy make.

James
Gert Doering June 22, 2020, 6:40 p.m. | #19
Hi,

On Mon, Jun 22, 2020 at 11:28:16AM -0700, James Bottomley wrote:
> That will be my fault.  I assumed automake always ran with gnu make, 

No :-)   (and let's not start a gnu make vs bsd make vs. cmake vs. ant
discussion now :-) ).

Specifically, we run automake on linux systems before doing release
tarballs, so the target host doesn't even *need* automake installed -
but if you want, automake runs just fine on BSD, producing "standard
makefiles" that work fine with BSD make.

> so the .cnf rule is in gnu make syntax at the bottom of (Makefile.am):
> 
> $(builddir)/%.cnf: $(srcdir)/%.cnf.in
> 	sed "s|ABSBUILDDIR|$(abs_builddir)|" < $< > $@
> 
> 
> If I just spell everything out and don't use the percent wildcard, it
> should work even on legacy make.

Ah, there it is :) - I think "non-GNU" make doesn't handle these in
combination with paths.

"man make" on FreeBSD says:

     The $@ and $< variables are more or less universally portable, as is the
     $(MAKE) variable.  Basic use of suffix rules (for files only in the
     current directory, not trying to chain transformations together, etc.) is
     also reasonably portable.

so

$(builddir)/openvpn.cnf: $(srcdir)/openvpn.cnf.in
	sed "s|ABSBUILDDIR|$(abs_builddir)|" < $< > $@

should work.

gert
Gert Doering June 24, 2020, 8:11 a.m. | #20
Hi,

On Mon, Jun 22, 2020 at 08:40:15PM +0200, Gert Doering wrote:
> so
> 
> $(builddir)/openvpn.cnf: $(srcdir)/openvpn.cnf.in
> 	sed "s|ABSBUILDDIR|$(abs_builddir)|" < $< > $@
> 
> should work.

It doesn't :-(

It worked for my tests of that patch, because the openssl.cnf generated
with "gmake test" was still in that directory - but today I created new
build directories to test --enable-small, and it doesn't - seems it
is not understanding $(builddir)/openvpn.cnf: as destination, nor does
"< $<" work.

What works is this:

openssl.cnf: $(srcdir)/openssl.cnf.in
        sed "s|ABSBUILDDIR|$(abs_builddir)|" < $(srcdir)/openssl.cnf.in > $@

(tested both for clean in-tree and out-of-tree builds)

Can we have a new (LAST!!!) patch, please...

gert

Patch

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/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 00000000..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 00000000..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 returning different messages and update the
+# test accordingly
+grep -q 'X509_check_private_key:key values mismatch' log.txt || { echo "Key mismatch not detected"; exit 1; }
+
+# now look for the engine prints (these are under our control)
+grep -q 'ENGINE: engine_init called' log.txt || { echo "Engine initialization not detected"; exit 1; }
+grep -q 'ENGINE: engine_load_key called' log.txt || { echo "Key was not loaded from engine"; exit 1; }
+grep -q "ENGINE: engine_load_key got password ${password}" log.txt || { echo "Key password was not retrieved by the engine"; exit 1; }
+exit 0
diff --git a/tests/unit_tests/engine-key/libtestengine.c b/tests/unit_tests/engine-key/libtestengine.c
new file mode 100644
index 00000000..46ec1e33
--- /dev/null
+++ b/tests/unit_tests/engine-key/libtestengine.c
@@ -0,0 +1,101 @@ 
+#include <string.h>
+#include <openssl/engine.h>
+#include <openssl/evp.h>
+#include <openssl/pem.h>
+
+static char *engine_id = "testengine";
+static char *engine_name = "Engine for testing openvpn engine key support";
+
+static int is_initialized = 0;
+
+static int engine_init(ENGINE *e)
+{
+	is_initialized = 1;
+	fprintf(stderr, "ENGINE: engine_init called\n");
+	return 1;
+}
+
+static int engine_finish(ENGINE *e)
+{
+	fprintf(stderr, "ENGINE: engine_finsh called\n");
+	is_initialized = 0;
+	return 1;
+}
+
+static EVP_PKEY *engine_load_key(ENGINE *e, const char *key_id,
+				 UI_METHOD *ui_method, void *cb_data)
+{
+	BIO *b;
+	EVP_PKEY *pkey;
+	PKCS8_PRIV_KEY_INFO *p8inf;
+	UI *ui;
+	char auth[256];
+
+	fprintf(stderr, "ENGINE: engine_load_key called\n");
+
+	if (!is_initialized) {
+		fprintf(stderr, "Load Key called without correct initialization\n");
+		return NULL;
+	}
+	b = BIO_new_file(key_id, "r");
+	if (!b) {
+		fprintf(stderr, "File %s does not exist or cannot be read\n", key_id);
+		return 0;
+	}
+	/* Basically read an EVP_PKEY private key file with different
+	 * PEM guards --- we are a test engine */
+	p8inf = PEM_ASN1_read_bio((d2i_of_void *)d2i_PKCS8_PRIV_KEY_INFO,
+				 "TEST ENGINE KEY", b,
+				 NULL, NULL, NULL);
+	BIO_free(b);
+	if (!p8inf) {
+		fprintf(stderr, "Failed to read engine private key\n");
+		return NULL;
+	}
+	pkey = EVP_PKCS82PKEY(p8inf);
+
+	/* now we have a private key, pretend it had a password
+	 * this verifies the password makes it through openvpn OK */
+	ui = UI_new();
+
+	if (ui_method)
+		UI_set_method(ui, ui_method);
+
+	UI_add_user_data(ui, cb_data);
+
+	if (UI_add_input_string(ui, "enter test engine key",
+				UI_INPUT_FLAG_DEFAULT_PWD,
+				auth, 0, sizeof(auth)) == 0) {
+		fprintf(stderr, "UI_add_input_string failed\n");
+		goto out;
+	}
+
+	if (UI_process(ui)) {
+		fprintf(stderr, "UI_process failed\n");
+		goto out;
+	}
+
+	fprintf(stderr, "ENGINE: engine_load_key got password %s\n", auth);
+
+ out:
+	UI_free(ui);
+
+	return pkey;
+}
+
+
+static int engine_bind_fn(ENGINE *e, const char *id)
+{
+	if (id && strcmp(id, engine_id) != 0)
+		return 0;
+	if (!ENGINE_set_id(e, engine_id) ||
+	    !ENGINE_set_name(e, engine_name) ||
+	    !ENGINE_set_init_function(e, engine_init) ||
+	    !ENGINE_set_finish_function(e, engine_finish) ||
+	    !ENGINE_set_load_privkey_function(e, engine_load_key))
+		return 0;
+	return 1;
+}
+
+IMPLEMENT_DYNAMIC_CHECK_FN()
+IMPLEMENT_DYNAMIC_BIND_FN(engine_bind_fn)
diff --git a/tests/unit_tests/engine-key/openssl.cnf.in b/tests/unit_tests/engine-key/openssl.cnf.in
new file mode 100644
index 00000000..93edd483
--- /dev/null
+++ b/tests/unit_tests/engine-key/openssl.cnf.in
@@ -0,0 +1,12 @@ 
+HOME		= .
+openssl_conf	= openssl_init
+
+[req]
+[openssl_init]
+engines		= engines_section
+
+[engines_section]
+testengine	= testengine_section
+
+[testengine_section]
+dynamic_path	= $ENV::srcdir/.libs/libtestengineSHREXT