* Re: [PATCH v2 3/4] sign-file: add generic built-in engine key support
2018-10-22 10:46 [PATCH v2 3/4] sign-file: add generic built-in engine key support James Bottomley
@ 2018-10-22 11:38 ` David Woodhouse
2018-10-22 13:51 ` Mark J Cox
` (4 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: David Woodhouse @ 2018-10-22 11:38 UTC (permalink / raw)
To: keyrings
[-- Attachment #1: Type: text/plain, Size: 1302 bytes --]
On Mon, 2018-10-22 at 11:46 +0100, James Bottomley wrote:
> 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.
> Note that in order to use built-in engines, you have to have the
> engine included in the openssl.cnf file
>
> Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
With the commit message fixed:
Acked-by: David Woodhouse <dwmw@amazon.co.uk>
.... 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?
[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5213 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2 3/4] sign-file: add generic built-in engine key support
2018-10-22 10:46 [PATCH v2 3/4] sign-file: add generic built-in engine key support James Bottomley
2018-10-22 11:38 ` David Woodhouse
@ 2018-10-22 13:51 ` Mark J Cox
2018-10-22 13:52 ` James Bottomley
` (3 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Mark J Cox @ 2018-10-22 13:51 UTC (permalink / raw)
To: keyrings
Needs revision; OPENSSL_config() is deprecated in OpenSSL 1.1.0+
Mark
On Mon, 22 Oct 2018, James Bottomley wrote:
> 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
>
> Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> ---
> scripts/sign-file.c | 39 +++++++++++++++++++++++++++++++++++++--
> 1 file changed, 37 insertions(+), 2 deletions(-)
>
> diff --git a/scripts/sign-file.c b/scripts/sign-file.c
> index 49f1cf456254..de8d9bb5e657 100644
> --- a/scripts/sign-file.c
> +++ b/scripts/sign-file.c
> @@ -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);
> }
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2 3/4] sign-file: add generic built-in engine key support
2018-10-22 10:46 [PATCH v2 3/4] sign-file: add generic built-in engine key support James Bottomley
2018-10-22 11:38 ` David Woodhouse
2018-10-22 13:51 ` Mark J Cox
@ 2018-10-22 13:52 ` James Bottomley
2018-10-22 14:08 ` Mark J Cox
` (2 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: James Bottomley @ 2018-10-22 13:52 UTC (permalink / raw)
To: keyrings
On Mon, 2018-10-22 at 14:51 +0100, Mark J Cox wrote:
> 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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2 3/4] sign-file: add generic built-in engine key support
2018-10-22 10:46 [PATCH v2 3/4] sign-file: add generic built-in engine key support James Bottomley
` (2 preceding siblings ...)
2018-10-22 13:52 ` James Bottomley
@ 2018-10-22 14:08 ` Mark J Cox
2018-10-23 14:02 ` James Bottomley
2018-10-27 14:10 ` James Bottomley
5 siblings, 0 replies; 7+ messages in thread
From: Mark J Cox @ 2018-10-22 14:08 UTC (permalink / raw)
To: keyrings
> On Mon, 2018-10-22 at 14:51 +0100, Mark J Cox wrote:
>> 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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2 3/4] sign-file: add generic built-in engine key support
2018-10-22 10:46 [PATCH v2 3/4] sign-file: add generic built-in engine key support James Bottomley
` (3 preceding siblings ...)
2018-10-22 14:08 ` Mark J Cox
@ 2018-10-23 14:02 ` James Bottomley
2018-10-27 14:10 ` James Bottomley
5 siblings, 0 replies; 7+ messages in thread
From: James Bottomley @ 2018-10-23 14:02 UTC (permalink / raw)
To: keyrings
On Mon, 2018-10-22 at 14:51 +0100, Mark J Cox wrote:
> 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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2 3/4] sign-file: add generic built-in engine key support
2018-10-22 10:46 [PATCH v2 3/4] sign-file: add generic built-in engine key support James Bottomley
` (4 preceding siblings ...)
2018-10-23 14:02 ` James Bottomley
@ 2018-10-27 14:10 ` James Bottomley
5 siblings, 0 replies; 7+ messages in thread
From: James Bottomley @ 2018-10-27 14:10 UTC (permalink / raw)
To: keyrings
On Mon, 2018-10-22 at 15:08 +0100, Mark J Cox wrote:
> > On Mon, 2018-10-22 at 14:51 +0100, Mark J Cox wrote:
> > > 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
---
diff --git a/scripts/sign-file.c b/scripts/sign-file.c
index ca45cfc6ca6a..2de66ced9575 100644
--- a/scripts/sign-file.c
+++ b/scripts/sign-file.c
@@ -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");
^ permalink raw reply related [flat|nested] 7+ messages in thread