[PATCH v2 4/4] sign-file: add explicit engine specification

[PATCH v2 4/4] sign-file: add explicit engine specification

[James Bottomley]

This commit adds an optional -e <engine> argument to sign file.  Now
that we have the explicit engine addition, the original pkcs11 token
implementation can also be merged into this code (using UI methods for
getting the key instead of the engine control command).  To keep the
code functioning the same way (no need to specify the pkcs11 engine if
the key file begins pkcs11:) an explicit check will set the engine to
pkcs11 if a pkcs11 key specifier is detected.

Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 scripts/sign-file.c | 63 ++++++++++++++++++++++++++++++-----------------------
 1 file changed, 36 insertions(+), 27 deletions(-)

diff --git a/scripts/sign-file.c b/scripts/sign-file.c
index de8d9bb5e657..ca45cfc6ca6a 100644
--- a/scripts/sign-file.c
+++ b/scripts/sign-file.c
@@ -71,7 +71,7 @@ static __attribute__((noreturn))
 void format(void)
 {
 	fprintf(stderr,
-		"Usage: scripts/sign-file [-dp] <hash algo> <key> <x509> <module> [<dest>]\n");
+		"Usage: scripts/sign-file [-e engine][-dp] <hash algo> <key> <x509> <module> [<dest>]\n");
 	fprintf(stderr,
 		"       scripts/sign-file -s <raw sig> <hash algo> <x509> <module> [<dest>]\n");
 	exit(2);
@@ -103,6 +103,8 @@ static void display_openssl_errors(const char *f, int l)
 	} while(0)
 
 static const char *key_pass;
+static char *engine;
+
 
 static int pem_pw_cb(char *buf, int len, int w, void *v)
 {
@@ -136,31 +138,51 @@ static int ui_read(UI *ui, UI_STRING *uis)
     return 0;
 }
 
+static EVP_PKEY *read_engine_key(const char *private_key_name, ENGINE *e)
+
+{
+	UI_METHOD *ui;
+	EVP_PKEY *private_key;
+
+	if (!ENGINE_get_load_privkey_function(e))
+		return NULL;
+
+	ui = UI_create_method("sign-file");
+	if (!ui)
+		return NULL;
+
+	UI_method_set_reader(ui, ui_read);
+	private_key = ENGINE_load_private_key(e, private_key_name,
+					      ui, NULL);
+	UI_destroy_method(ui);
+	if (private_key)
+		ERR_clear_error(); /* initial key read failed */
+	return private_key;
+}
+
 static EVP_PKEY *read_private_key(const char *private_key_name)
 {
 	EVP_PKEY *private_key;
+	ENGINE *e;
+
 
 	ENGINE_load_builtin_engines();
 	OPENSSL_config(NULL);
 	ERR_clear_error();
-	if (!strncmp(private_key_name, "pkcs11:", 7)) {
-		ENGINE *e;
+	if (!engine && !strncmp(private_key_name, "pkcs11:", 7))
+		engine = "pkcs11";
 
-		e = ENGINE_by_id("pkcs11");
-		ERR(!e, "Load PKCS#11 ENGINE");
+	if (engine) {
+		e = ENGINE_by_id(engine);
+		ERR(!e, "Load %s ENGINE", engine);
 		if (ENGINE_init(e))
 			ERR_clear_error();
 		else
 			ERR(1, "ENGINE_init");
-		if (key_pass)
-			ERR(!ENGINE_ctrl_cmd_string(e, "PIN", key_pass, 0),
-			    "Set PKCS#11 PIN");
-		private_key = ENGINE_load_private_key(e, private_key_name,
-						      NULL, NULL);
+		private_key = read_engine_key(private_key_name, e);
 		ERR(!private_key, "%s", private_key_name);
 	} else {
 		BIO *b;
-		ENGINE *e;
 
 		b = BIO_new_file(private_key_name, "rb");
 		ERR(!b, "%s", private_key_name);
@@ -168,21 +190,7 @@ static EVP_PKEY *read_private_key(const char *private_key_name)
 						      NULL);
 		for (e = ENGINE_get_first(); !private_key && e != NULL;
 		     e = ENGINE_get_next(e)) {
-			UI_METHOD *ui;
-
-			if (!ENGINE_get_load_privkey_function(e))
-				continue;
-
-			ui = UI_create_method("sign-file");
-			if (!ui)
-				continue;
-
-			UI_method_set_reader(ui, ui_read);
-			private_key = ENGINE_load_private_key(e, private_key_name,
-							      ui, NULL);
-			UI_destroy_method(ui);
-			if (private_key)
-				ERR_clear_error(); /* initial key read failed */
+			private_key = read_engine_key(private_key_name, e);
 		}
 
 		ERR(!private_key, "%s", private_key_name);
@@ -267,7 +275,7 @@ int main(int argc, char **argv)
 #endif
 
 	do {
-		opt = getopt(argc, argv, "sdpk");
+		opt = getopt(argc, argv, "sdpke:");
 		switch (opt) {
 		case 's': raw_sig = true; break;
 		case 'p': save_sig = true; break;
@@ -275,6 +283,7 @@ int main(int argc, char **argv)
 #ifndef USE_PKCS7
 		case 'k': use_keyid = CMS_USE_KEYID; break;
 #endif
+		case 'e': engine = optarg; break;
 		case -1: break;
 		default: format();
 		}
-- 
2.16.4

Re: [PATCH v2 4/4] sign-file: add explicit engine specification

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 702 bytes --]

On Mon, 2018-10-22 at 11:48 +0100, James Bottomley wrote:
> This commit adds an optional -e <engine> argument to sign file.  Now
> that we have the explicit engine addition, the original pkcs11 token
> implementation can also be merged into this code (using UI methods for
> getting the key instead of the engine control command).  To keep the
> code functioning the same way (no need to specify the pkcs11 engine if
> the key file begins pkcs11:) an explicit check will set the engine to
> pkcs11 if a pkcs11 key specifier is detected.
> 
> Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>

Acked-if-you-tested-it-with-PKCS#11-by: David Woodhouse <dwmw@amazon.co.uk>

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5213 bytes --]

Re: [PATCH v2 4/4] sign-file: add explicit engine specification

[Mark J Cox]

I've not tested PKCS11.  For the specific case of Thales/nCipher this 
works the same as my patch and therefore

Tested-by: Mark J Cox <mjc@redhat.com>

Mark

On Mon, 22 Oct 2018, James Bottomley wrote:

> This commit adds an optional -e <engine> argument to sign file.  Now
> that we have the explicit engine addition, the original pkcs11 token
> implementation can also be merged into this code (using UI methods for
> getting the key instead of the engine control command).  To keep the
> code functioning the same way (no need to specify the pkcs11 engine if
> the key file begins pkcs11:) an explicit check will set the engine to
> pkcs11 if a pkcs11 key specifier is detected.
>
> Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> ---
> scripts/sign-file.c | 63 ++++++++++++++++++++++++++++++-----------------------
> 1 file changed, 36 insertions(+), 27 deletions(-)
>
> diff --git a/scripts/sign-file.c b/scripts/sign-file.c
> index de8d9bb5e657..ca45cfc6ca6a 100644
> --- a/scripts/sign-file.c
> +++ b/scripts/sign-file.c
> @@ -71,7 +71,7 @@ static __attribute__((noreturn))
> void format(void)
> {
> 	fprintf(stderr,
> -		"Usage: scripts/sign-file [-dp] <hash algo> <key> <x509> <module> [<dest>]\n");
> +		"Usage: scripts/sign-file [-e engine][-dp] <hash algo> <key> <x509> <module> [<dest>]\n");
> 	fprintf(stderr,
> 		"       scripts/sign-file -s <raw sig> <hash algo> <x509> <module> [<dest>]\n");
> 	exit(2);
> @@ -103,6 +103,8 @@ static void display_openssl_errors(const char *f, int l)
> 	} while(0)
>
> static const char *key_pass;
> +static char *engine;
> +
>
> static int pem_pw_cb(char *buf, int len, int w, void *v)
> {
> @@ -136,31 +138,51 @@ static int ui_read(UI *ui, UI_STRING *uis)
>     return 0;
> }
>
> +static EVP_PKEY *read_engine_key(const char *private_key_name, ENGINE *e)
> +
> +{
> +	UI_METHOD *ui;
> +	EVP_PKEY *private_key;
> +
> +	if (!ENGINE_get_load_privkey_function(e))
> +		return NULL;
> +
> +	ui = UI_create_method("sign-file");
> +	if (!ui)
> +		return NULL;
> +
> +	UI_method_set_reader(ui, ui_read);
> +	private_key = ENGINE_load_private_key(e, private_key_name,
> +					      ui, NULL);
> +	UI_destroy_method(ui);
> +	if (private_key)
> +		ERR_clear_error(); /* initial key read failed */
> +	return private_key;
> +}
> +
> static EVP_PKEY *read_private_key(const char *private_key_name)
> {
> 	EVP_PKEY *private_key;
> +	ENGINE *e;
> +
>
> 	ENGINE_load_builtin_engines();
> 	OPENSSL_config(NULL);
> 	ERR_clear_error();
> -	if (!strncmp(private_key_name, "pkcs11:", 7)) {
> -		ENGINE *e;
> +	if (!engine && !strncmp(private_key_name, "pkcs11:", 7))
> +		engine = "pkcs11";
>
> -		e = ENGINE_by_id("pkcs11");
> -		ERR(!e, "Load PKCS#11 ENGINE");
> +	if (engine) {
> +		e = ENGINE_by_id(engine);
> +		ERR(!e, "Load %s ENGINE", engine);
> 		if (ENGINE_init(e))
> 			ERR_clear_error();
> 		else
> 			ERR(1, "ENGINE_init");
> -		if (key_pass)
> -			ERR(!ENGINE_ctrl_cmd_string(e, "PIN", key_pass, 0),
> -			    "Set PKCS#11 PIN");
> -		private_key = ENGINE_load_private_key(e, private_key_name,
> -						      NULL, NULL);
> +		private_key = read_engine_key(private_key_name, e);
> 		ERR(!private_key, "%s", private_key_name);
> 	} else {
> 		BIO *b;
> -		ENGINE *e;
>
> 		b = BIO_new_file(private_key_name, "rb");
> 		ERR(!b, "%s", private_key_name);
> @@ -168,21 +190,7 @@ static EVP_PKEY *read_private_key(const char *private_key_name)
> 						      NULL);
> 		for (e = ENGINE_get_first(); !private_key && e != NULL;
> 		     e = ENGINE_get_next(e)) {
> -			UI_METHOD *ui;
> -
> -			if (!ENGINE_get_load_privkey_function(e))
> -				continue;
> -
> -			ui = UI_create_method("sign-file");
> -			if (!ui)
> -				continue;
> -
> -			UI_method_set_reader(ui, ui_read);
> -			private_key = ENGINE_load_private_key(e, private_key_name,
> -							      ui, NULL);
> -			UI_destroy_method(ui);
> -			if (private_key)
> -				ERR_clear_error(); /* initial key read failed */
> +			private_key = read_engine_key(private_key_name, e);
> 		}
>
> 		ERR(!private_key, "%s", private_key_name);
> @@ -267,7 +275,7 @@ int main(int argc, char **argv)
> #endif
>
> 	do {
> -		opt = getopt(argc, argv, "sdpk");
> +		opt = getopt(argc, argv, "sdpke:");
> 		switch (opt) {
> 		case 's': raw_sig = true; break;
> 		case 'p': save_sig = true; break;
> @@ -275,6 +283,7 @@ int main(int argc, char **argv)
> #ifndef USE_PKCS7
> 		case 'k': use_keyid = CMS_USE_KEYID; break;
> #endif
> +		case 'e': engine = optarg; break;
> 		case -1: break;
> 		default: format();
> 		}
>

Re: [PATCH v2 4/4] sign-file: add explicit engine specification

[James Bottomley]

On Mon, 2018-10-22 at 12:38 +0100, David Woodhouse wrote:
> On Mon, 2018-10-22 at 11:48 +0100, James Bottomley wrote:
> > This commit adds an optional -e <engine> argument to sign
> > file.  Now that we have the explicit engine addition, the original
> > pkcs11 token implementation can also be merged into this code
> > (using UI methods for getting the key instead of the engine control
> > command).  To keep the code functioning the same way (no need to
> > specify the pkcs11 engine if the key file begins pkcs11:) an
> > explicit check will set the engine to pkcs11 if a pkcs11 key
> > specifier is detected.
> > 
> > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.c
> > om>
> 
> Acked-if-you-tested-it-with-PKCS#11-by: David Woodhouse <dwmw@amazon.
> co.uk>

It was a right royal pain, but I have tested it with softhsm+p11lib
engine and it all (amazingly) seems to work with correct pkcs11 URLs
... just don't ask me to do it again.

James

Re: [PATCH v2 4/4] sign-file: add explicit engine specification

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1813 bytes --]

On Tue, 2018-10-23 at 12:10 +0100, James Bottomley wrote:
> On Mon, 2018-10-22 at 12:38 +0100, David Woodhouse wrote:
> > On Mon, 2018-10-22 at 11:48 +0100, James Bottomley wrote:
> > > This commit adds an optional -e <engine> argument to sign
> > > file.  Now that we have the explicit engine addition, the original
> > > pkcs11 token implementation can also be merged into this code
> > > (using UI methods for getting the key instead of the engine control
> > > command).  To keep the code functioning the same way (no need to
> > > specify the pkcs11 engine if the key file begins pkcs11:) an
> > > explicit check will set the engine to pkcs11 if a pkcs11 key
> > > specifier is detected.
> > > 
> > > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.c
> > > om>
> > 
> > Acked-if-you-tested-it-with-PKCS#11-by: David Woodhouse <dwmw@amazon.
> > co.uk>
> 
> It was a right royal pain, but I have tested it with softhsm+p11lib
> engine and it all (amazingly) seems to work with correct pkcs11 URLs
> ... just don't ask me to do it again.

That shouldn't have been a pain.

The SoftHSM part should been just installing the package then running
the two commands from OpenConnect's tests/Makefile.am that I pointed
you at — 'softhsm2-util --init-token' then 'softhsm2-util --import'.

Then you just install the PKCS#11 engine package, then you should be
able to use sign-tool (or indeed anything else in the distro) with the
RFC7512 URI for the cert you imported, instead of a PEM filename.

If it wasn't as simple as that, please file bugs with your distro. It's
OpenSUSE, right? I don't pay much attention to SUSE but I didn't think
their crypto bits were as out of date and badly maintained as
Debian/Ubuntu — I kind of expected stuff to work properly.

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5213 bytes --]

Re: [PATCH v2 4/4] sign-file: add explicit engine specification

[James Bottomley]

On October 23, 2018 12:37:52 PM GMT, David Woodhouse <dwmw2@infradead.org> wrote:
>On Tue, 2018-10-23 at 12:10 +0100, James Bottomley wrote:
>> On Mon, 2018-10-22 at 12:38 +0100, David Woodhouse wrote:
>> > On Mon, 2018-10-22 at 11:48 +0100, James Bottomley wrote:
>> > > This commit adds an optional -e <engine> argument to sign
>> > > file.  Now that we have the explicit engine addition, the
>original
>> > > pkcs11 token implementation can also be merged into this code
>> > > (using UI methods for getting the key instead of the engine
>control
>> > > command).  To keep the code functioning the same way (no need to
>> > > specify the pkcs11 engine if the key file begins pkcs11:) an
>> > > explicit check will set the engine to pkcs11 if a pkcs11 key
>> > > specifier is detected.
>> > > 
>> > > Signed-off-by: James Bottomley
><James.Bottomley@HansenPartnership.c
>> > > om>
>> > 
>> > Acked-if-you-tested-it-with-PKCS#11-by: David Woodhouse
><dwmw@amazon.
>> > co.uk>
>> 
>> It was a right royal pain, but I have tested it with softhsm+p11lib
>> engine and it all (amazingly) seems to work with correct pkcs11 URLs
>> ... just don't ask me to do it again.
>
>That shouldn't have been a pain.
>
>The SoftHSM part should been just installing the package then running
>the two commands from OpenConnect's tests/Makefile.am that I pointed
>you at — 'softhsm2-util --init-token' then 'softhsm2-util --import'.
>
>Then you just install the PKCS#11 engine package, then you should be
>able to use sign-tool (or indeed anything else in the distro) with the
>RFC7512 URI for the cert you imported, instead of a PEM filename.
>
>If it wasn't as simple as that, please file bugs with your distro. It's
>OpenSUSE, right? I don't pay much attention to SUSE but I didn't think
>their crypto bits were as out of date and badly maintained as
>Debian/Ubuntu — I kind of expected stuff to work properly.

The pain points were opensuse doesn't package softhsm.  Provisioning softhsm according to the man page gives a slot not found error and it's unclear how to pass the pkcs11 module to the engine.  Once I had the source code, I figured it out, but it definitely wasn't easy.

James


-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH v2 4/4] sign-file: add explicit engine specification

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 853 bytes --]

On Tue, 2018-10-23 at 12:42 +0100, James Bottomley wrote:
> The pain points were opensuse doesn't package softhsm.  Provisioning
> softhsm according to the man page gives a slot not found error and
> it's unclear how to pass the pkcs11 module to the engine.  Once I had
> the source code, I figured it out, but it definitely wasn't easy.

You shouldn't have to "pass the pkcs11 module to the engine".

The softhsm 'make install' should install its p11-kit ".module" file so
that it is automatically seen by applications in the system¹.

Including the engine². Because the engine should use p11-kit-proxy.so
as its default provider if you don't specify otherwise, thus pulling in
everything it should.

-- 
dwmw2


¹ https://github.com/opendnssec/SoftHSMv2/pull/233 (2016)
² https://github.com/OpenSC/engine_pkcs11/pull/9 (2015)

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5213 bytes --]