Skip to contentSkip to conversation

sign-file: add generic built-in engine key support

PATCH In review James Bottomley 3 participants 7 messages Oct 22, 2018 – Oct 27, 2018

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH v2 3/4 sign-file: add generic built-in engine key support

The current engine code only supports a non-standard pkcs11 engine module. Add code to support any standard engine key module, but leave the non-standard code alone because it would likely fail to function with the correct UI_method of collecting the password.

Note that in order to use built-in engines, you have to have the engine included in the openssl.cnf file

---
 scripts/sign-file.c | 39 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 37 insertions(+), 2 deletions(-)
scripts/sign-file.c +37 -2
index 49f1cf456254..de8d9bb5e657 100644
@@ -28,6 +28,7 @@
 #include <openssl/pem.h>
 #include <openssl/err.h>
 #include <openssl/engine.h>
+#include <openssl/conf.h>
 
 /*
  * Use CMS if we have openssl-1.0.0 or newer available - otherwise we have to
@@ -122,15 +123,29 @@ static int pem_pw_cb(char *buf, int len, int w, void *v)
 	return pwlen;
 }
 
+static int ui_read(UI *ui, UI_STRING *uis)
+{
+    if (UI_get_string_type(uis) = UIT_PROMPT) {
+        char password[64];
+
+        pem_pw_cb(password, sizeof(password), 0, NULL);
+        UI_set_result(ui, uis, password);
+
+        return 1;
+    }
+    return 0;
+}
+
 static EVP_PKEY *read_private_key(const char *private_key_name)
 {
 	EVP_PKEY *private_key;
 
+	ENGINE_load_builtin_engines();
+	OPENSSL_config(NULL);
+	ERR_clear_error();
 	if (!strncmp(private_key_name, "pkcs11:", 7)) {
 		ENGINE *e;
 
-		ENGINE_load_builtin_engines();
-		ERR_clear_error();
 		e = ENGINE_by_id("pkcs11");
 		ERR(!e, "Load PKCS#11 ENGINE");
 		if (ENGINE_init(e))
@@ -145,11 +160,31 @@ static EVP_PKEY *read_private_key(const char *private_key_name)
 		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);
 		private_key = PEM_read_bio_PrivateKey(b, NULL, pem_pw_cb,
 						      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 */
+		}
+
 		ERR(!private_key, "%s", private_key_name);
 		BIO_free(b);
 	}
Signature · 1 line
2.16.4
maintainer Acked-by on v2 3/4
#1 lore raw
Quoted 4 lines · James Bottomley

The current engine code only supports a non-standard pkcs11 engine module. Add code to support any standard engine key module, but leave the non-standard code alone because it would likely fail to function with the correct UI_method of collecting the password.

As discussed, that commit message isn't right. There's nothing really "non-standard" about what the PKCS#11 module does. By allowing the passcode to be provided in advance via an ENGINE_ctrl it's doing the same thing that a bunch of other engines, even your own, are doing.

Please fix it to just describe what it's actually doing — providing a UI_METHOD to automatically fill in the passcode, instead of relying on the "PIN" ENGINE_ctrl.

Quoted 4 lines

Note that in order to use built-in engines, you have to have the engine included in the openssl.cnf file

With the commit message fixed:

.... although it would be nicer if it actually did something useful in the case where its ui_read() method is called and it doesn't have a password to give. Perhaps in fact, don't set up the UI_METHOD at all if you don't have a password?

#2 lore raw
In reply to James Bottomley

Needs revision; OPENSSL_config() is deprecated in OpenSSL 1.1.0+

Mark

Quoted 90 lines · James Bottomley

The current engine code only supports a non-standard pkcs11 engine module. Add code to support any standard engine key module, but leave the non-standard code alone because it would likely fail to function with the correct UI_method of collecting the password.

Note that in order to use built-in engines, you have to have the engine included in the openssl.cnf file

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

1 file changed, 37 insertions(+), 2 deletions(-)
scripts/sign-file.c +37 -2
index 49f1cf456254..de8d9bb5e657 100644
@@ -28,6 +28,7 @@
 #include <openssl/pem.h>
 #include <openssl/err.h>
 #include <openssl/engine.h>
+#include <openssl/conf.h>
 
 /*
 * Use CMS if we have openssl-1.0.0 or newer available - otherwise we have to
@@ -122,15 +123,29 @@ static int pem_pw_cb(char *buf, int len, int w, void *v)
 	return pwlen;
 }
 
+static int ui_read(UI *ui, UI_STRING *uis)
+{
+    if (UI_get_string_type(uis) = UIT_PROMPT) {
+        char password[64];
+
+        pem_pw_cb(password, sizeof(password), 0, NULL);
+        UI_set_result(ui, uis, password);
+
+        return 1;
+    }
+    return 0;
+}
+
 static EVP_PKEY *read_private_key(const char *private_key_name)
 {
 	EVP_PKEY *private_key;
 
+	ENGINE_load_builtin_engines();
+	OPENSSL_config(NULL);
+	ERR_clear_error();
 	if (!strncmp(private_key_name, "pkcs11:", 7)) {
 		ENGINE *e;
 
-		ENGINE_load_builtin_engines();
-		ERR_clear_error();
 		e = ENGINE_by_id("pkcs11");
 		ERR(!e, "Load PKCS#11 ENGINE");
 		if (ENGINE_init(e))
@@ -145,11 +160,31 @@ static EVP_PKEY *read_private_key(const char *private_key_name)
 		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);
 		private_key = PEM_read_bio_PrivateKey(b, NULL, pem_pw_cb,
 						      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 */
+		}
+
 		ERR(!private_key, "%s", private_key_name);
 		BIO_free(b);
 	}
#3 lore raw
In reply to James Bottomley
Quoted 1 line · Mark J Cox

Needs revision; OPENSSL_config() is deprecated in OpenSSL 1.1.0+

It's deprecated but still functional. I have a todo to find out what its replacement is.

James

#4 lore raw
In reply to James Bottomley
Quoted 5 lines · Mark J Cox
Quoted 1 line

Needs revision; OPENSSL_config() is deprecated in OpenSSL 1.1.0+

It's deprecated but still functional. I have a todo to find out what its replacement is.

CONF_modules_load(). The patch really ought to be updated and not drop deprecation warnings as many OS now include OpenSSL 1.1.0+

Mark

#5 lore raw
In reply to James Bottomley
Quoted 1 line · Mark J Cox

Needs revision; OPENSSL_config() is deprecated in OpenSSL 1.1.0+

This doesn't look pleasant: apps/openssl.c now seems to have replaced this with an open coded equvalent.

The man page says to replace it with OPENSSL_init_crypto() which might work, or it looks like I might be able to replace it with

CONF_modules_load_file(NULL, NULL, 0);

James

#6 lore raw
In reply to James Bottomley
Quoted 9 lines · Mark J Cox
Quoted 6 lines · Mark J Cox
Quoted 1 line

Needs revision; OPENSSL_config() is deprecated in OpenSSL 1.1.0+

It's deprecated but still functional. I have a todo to find out what
its replacement is.

CONF_modules_load(). The patch really ought to be updated and not drop deprecation warnings as many OS now include OpenSSL 1.1.0+

Actually, it doesn't seem to be ... CONF_modules_load() seems to be another API screw up. If you look at how openssl/apps/apps.c does it, it's a rather nasty open coding of OPENSSL_config(NULL).

It looks like the new replacement is OPENSSL_init_crypto() with a flag.
Unfortunately this one is 1.1.0 only, so I'll have to make the whole
thing #ifdef hell.

James

---

scripts/sign-file.c +10 -1
index ca45cfc6ca6a..2de66ced9575 100644
@@ -165,10 +165,11 @@ static EVP_PKEY *read_private_key(const char *private_key_name)
 	EVP_PKEY *private_key;
 	ENGINE *e;
 
-
+#if OPENSSL_VERSION_NUMBER < 0x10100000
 	ENGINE_load_builtin_engines();
 	OPENSSL_config(NULL);
 	ERR_clear_error();
+#endif
 	if (!engine && !strncmp(private_key_name, "pkcs11:", 7))
 		engine = "pkcs11";
 
@@ -190,6 +191,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)) {
+		  printf("ENGINE: %s\n", ENGINE_get_name(e));
 			private_key = read_engine_key(private_key_name, e);
 		}
 
@@ -262,9 +264,14 @@ int main(int argc, char **argv)
 	X509 *x509;
 	BIO *bd, *bm;
 	int opt, n;
+#if OPENSSL_VERSION_NUMBER < 0x10100000
 	OpenSSL_add_all_algorithms();
 	ERR_load_crypto_strings();
 	ERR_clear_error();
+#else
+	OPENSSL_init_crypto(OPENSSL_INIT_ENGINE_ALL_BUILTIN
+			    | OPENSSL_INIT_LOAD_CONFIG, NULL);
+#endif
 
 	key_pass = getenv("KBUILD_SIGN_PIN");
 
@@ -332,7 +339,9 @@ int main(int argc, char **argv)
 		x509 = read_x509(x509_name);
 
 		/* Digest the module data. */
+#if OPENSSL_VERSION_NUMBER < 0x10100000
 		OpenSSL_add_all_digests();
+#endif
 		display_openssl_errors(__FILE__, __LINE__);
 		digest_algo = EVP_get_digestbyname(hash_algo);
 		ERR(!digest_algo, "EVP_get_digestbyname");