sign-file: add explicit engine specification

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 <[email protected]>

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 <[email protected]>

---
scripts/sign-file.c | 63 ++++++++++++++++++++++++++++++-----------------------

1 file changed, 36 insertions(+), 27 deletions(-)

scripts/sign-file.c +36 -27

index de8d9bb5e657..ca45cfc6ca6a 100644
@@ -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();
 		}

Quoted 9 lines · 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.

Quoted 2 lines

Signed-off-by James Bottomley <[email protected]

om>

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

Quoted 13 lines · David Woodhouse

Quoted 11 lines · 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 <[email protected]

om>

Quoted 2 lines

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.

Quoted 4 lines · James Bottomley

Quoted 3 lines · David Woodhouse

Quoted 2 lines · James Bottomley

This commit adds an optional -e <engine> argument to sign file. Now that we have the explicit engine addition, the

original

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

pkcs11 token implementation can also be merged into this code (using UI methods for getting the key instead of the engine

control

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

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

<[email protected]

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

om>

Acked-if-you-tested-it-with-PKCS#11-by: David Woodhouse

<dwmw@amazon.

Quoted 5 lines

Quoted 1 line

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.