[PATCH] support other engines for module signing

[PATCH] support other engines for module signing

[Mark J Cox]

Allow sign-file to use any available OpenSSL engine, not limited
to PKCS-11 by using "enginename:keyname" syntax.  We have to do a
special case for pkcs11 key name passing.

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

--- sign-file.c-orig	2018-10-17 10:53:13.726064233 +0100
+++ sign-file.c	2018-10-17 11:25:29.286285382 +0100
@@ -135,24 +135,30 @@ static int pem_pw_cb(char *buf, int len,
  static EVP_PKEY *read_private_key(const char *private_key_name)
  {
  	EVP_PKEY *private_key;
+	char *enginecolon;

-	if (!strncmp(private_key_name, "pkcs11:", 7)) {
+	if ((enginecolon = strchr(private_key_name,':'))) {
  		ENGINE *e;
+		char *keyname = enginecolon+1;
+		char *enginename = strndup(private_key_name, enginecolon-private_key_name);

  		ENGINE_load_builtin_engines();
  		drain_openssl_errors();
-		e = ENGINE_by_id("pkcs11");
-		ERR(!e, "Load PKCS#11 ENGINE");
+		e = ENGINE_by_id(enginename);
+		ERR(!e, "Load ENGINE %s", enginename);
  		if (ENGINE_init(e))
  			drain_openssl_errors();
  		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,
+			    "Set ENGINE PIN");
+		/* if the engine is pkcs11: then keep that bit in keyname */
+		if (!strncmp(private_key_name, "pkcs11:", 7))
+		    keyname = (char *)private_key_name;
+		private_key = ENGINE_load_private_key(e, keyname,
  						      NULL, NULL);
-		ERR(!private_key, "%s", private_key_name);
+		ERR(!private_key, "%s", keyname);
  	} else {
  		BIO *b;

Re: [PATCH] support other engines for module signing

[David Woodhouse]

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

On Wed, 2018-10-17 at 11:43 +0100, Mark J Cox wrote:
> Allow sign-file to use any available OpenSSL engine, not limited
> to PKCS-11 by using "enginename:keyname" syntax.  We have to do a
> special case for pkcs11 key name passing.

Ick, no.

That is not "enginename:keyname" syntax. It is recognising a PKCS#11
URI of the form defined in RFC7512, and invoking the appropriate ENGINE
for it. If you want to handle other keys, then make it transparent —
either by using the OpenSSL STORE API... or by switching to GnuTLS
which makes a lot of this easier.

Please do not introduce other non-standard forms of referring to keys.

http://david.woodhou.se/draft-woodhouse-cert-best-practice.html


Which engine were you planning to use?

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

Re: [PATCH] support other engines for module signing

[James Bottomley]

On Wed, 2018-10-17 at 11:43 +0100, Mark J Cox wrote:
> Allow sign-file to use any available OpenSSL engine, not limited
> to PKCS-11 by using "enginename:keyname" syntax.  We have to do a
> special case for pkcs11 key name passing.

There's actually already a proposal for this which David (Howells) has
been ignoring:

https://marc.info/?l=linux-keyrings&m\x151845297302654&w=2

It tries to use the correct UI callbacks, which yours is missing.

James

Re: [PATCH] support other engines for module signing

[David Howells]

James Bottomley <James.Bottomley@HansenPartnership.com> wrote:

> > Allow sign-file to use any available OpenSSL engine, not limited
> > to PKCS-11 by using "enginename:keyname" syntax.  We have to do a
> > special case for pkcs11 key name passing.
> 
> There's actually already a proposal for this which David (Howells) has
> been ignoring:

Not so much ignoring as it just keeps getting buried.

> https://marc.info/?l=linux-keyrings&m\x151845297302654&w=2
> 
> It tries to use the correct UI callbacks, which yours is missing.

If this works for Mark and Dave, then I could take this instead.

David

Re: [PATCH] support other engines for module signing

[James Bottomley]

On Wed, 2018-10-17 at 15:40 +0100, David Howells wrote:
> James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> 
> > > Allow sign-file to use any available OpenSSL engine, not limited
> > > to PKCS-11 by using "enginename:keyname" syntax.  We have to do a
> > > special case for pkcs11 key name passing.
> > 
> > There's actually already a proposal for this which David (Howells)
> > has
> > been ignoring:
> 
> Not so much ignoring as it just keeps getting buried.

Understood.  What I really need is my patch testing by someone at Red
Hat: the pkcs11 token you use looks highly non-standard so someone
needs to check that adding generic engine support doesn't break it.

> > https://marc.info/?l=linux-keyrings&m\x151845297302654&w=2
> > 
> > It tries to use the correct UI callbacks, which yours is missing.
> 
> If this works for Mark and Dave, then I could take this instead.

Hey, now I've got your attention, there's this one as well.  It's a bit
trivial but it is converting to a known API:

https://marc.info/?l=linux-keyrings&m\x151845291102622&w=2

James

Re: [PATCH] support other engines for module signing

[David Woodhouse]

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

On Wed, 2018-10-17 at 07:43 -0700, James Bottomley wrote:
> On Wed, 2018-10-17 at 15:40 +0100, David Howells wrote:
> > James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> > 
> > > > Allow sign-file to use any available OpenSSL engine, not limited
> > > > to PKCS-11 by using "enginename:keyname" syntax.  We have to do a
> > > > special case for pkcs11 key name passing.
> > > 
> > > There's actually already a proposal for this which David (Howells)
> > > has
> > > been ignoring:
> > 
> > Not so much ignoring as it just keeps getting buried.
> 
> Understood.  What I really need is my patch testing by someone at Red
> Hat: the pkcs11 token you use looks highly non-standard so someone
> needs to check that adding generic engine support doesn't break it.

Que?

Which PKCS#11 token are you talking about? Or do you mean the PKCS#11
engine, which is the normal one from https://github.com/OpenSC/libp11

I think it does support using the UI for callbacks. Your trick of
passing through to pem_pw_cb should probably work fine, as long as
there's only ever one password. It should be trivial to test using
SoftHSM or any other soft token, even if you have no actual hardware-
based PKCS#11 tokens.

> > > https://marc.info/?l=linux-keyrings&m=151845297302654&w=2
> > > 
> > > It tries to use the correct UI callbacks, which yours is missing.
> > 
> > If this works for Mark and Dave, then I could take this instead.

AFAICT you're still only ever loading the "pkcs11" engine there. Is
there another patch I missed?



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

Re: [PATCH] support other engines for module signing

[James Bottomley]

On Wed, 2018-10-17 at 08:05 -0700, David Woodhouse wrote:
> On Wed, 2018-10-17 at 07:43 -0700, James Bottomley wrote:
> > On Wed, 2018-10-17 at 15:40 +0100, David Howells wrote:
> > > James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> > > 
> > > > > Allow sign-file to use any available OpenSSL engine, not
> > > > > limited
> > > > > to PKCS-11 by using "enginename:keyname" syntax.  We have to
> > > > > do a
> > > > > special case for pkcs11 key name passing.
> > > > 
> > > > There's actually already a proposal for this which David
> > > > (Howells)
> > > > has
> > > > been ignoring:
> > > 
> > > Not so much ignoring as it just keeps getting buried.
> > 
> > Understood.  What I really need is my patch testing by someone at
> > Red
> > Hat: the pkcs11 token you use looks highly non-standard so someone
> > needs to check that adding generic engine support doesn't break it.
> 
> Que?
> 
> Which PKCS#11 token are you talking about? Or do you mean the PKCS#11
> engine, which is the normal one from https://github.com/OpenSC/libp11
> 
> I think it does support using the UI for callbacks. Your trick of
> passing through to pem_pw_cb should probably work fine, as long as
> there's only ever one password. It should be trivial to test using
> SoftHSM or any other soft token, even if you have no actual hardware-
> based PKCS#11 tokens.

It didn't look like a normal one.  In fact it doesn't use UI callbacks
at all: it passes the key password in with an engine command.  That's
why I left it specifically cased out in the file.  Now if it does
actually work normally, the special casing can be removed.

> > > > https://marc.info/?l=linux-keyrings&m\x151845297302654&w=2
> > > > 
> > > > It tries to use the correct UI callbacks, which yours is
> > > > missing.
> > > 
> > > If this works for Mark and Dave, then I could take this instead.
> 
> AFAICT you're still only ever loading the "pkcs11" engine there. Is
> there another patch I missed?

No, the requirement seemed to be to add the engine to the openssl
config file.  There's no reason why an additional command parameter
can't be added, though.

James

Re: [PATCH] support other engines for module signing

[David Woodhouse]

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

On Wed, 2018-10-17 at 08:10 -0700, James Bottomley wrote:
> On Wed, 2018-10-17 at 08:05 -0700, David Woodhouse wrote:
> > On Wed, 2018-10-17 at 07:43 -0700, James Bottomley wrote:
> > > On Wed, 2018-10-17 at 15:40 +0100, David Howells wrote:
> > > > James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> > > > 
> > > > > > Allow sign-file to use any available OpenSSL engine, not
> > > > > > limited
> > > > > > to PKCS-11 by using "enginename:keyname" syntax.  We have
> > > > > > to
> > > > > > do a
> > > > > > special case for pkcs11 key name passing.
> > > > > 
> > > > > There's actually already a proposal for this which David
> > > > > (Howells)
> > > > > has
> > > > > been ignoring:
> > > > 
> > > > Not so much ignoring as it just keeps getting buried.
> > > 
> > > Understood.  What I really need is my patch testing by someone at
> > > Red
> > > Hat: the pkcs11 token you use looks highly non-standard so
> > > someone
> > > needs to check that adding generic engine support doesn't break
> > > it.
> > 
> > Que?
> > 
> > Which PKCS#11 token are you talking about? Or do you mean the
> > PKCS#11
> > engine, which is the normal one from 
> > https://github.com/OpenSC/libp11
> > 
> > I think it does support using the UI for callbacks. Your trick of
> > passing through to pem_pw_cb should probably work fine, as long as
> > there's only ever one password. It should be trivial to test using
> > SoftHSM or any other soft token, even if you have no actual
> > hardware-
> > based PKCS#11 tokens.
> 
> It didn't look like a normal one.  In fact it doesn't use UI callbacks
> at all: it passes the key password in with an engine command.  That's
> why I left it specifically cased out in the file.  Now if it does
> actually work normally, the special casing can be removed.

It looks just like yours. It uses the UI callbacks, but you can bypass
those by providing the password in advance with an ENGINE_ctrl if you
want.

> No, the requirement seemed to be to add the engine to the openssl
> config file.  There's no reason why an additional command parameter
> can't be added, though.

But ENGINE_load_private_key() needs to be used with a specific ENGINE,
doesn't it? Unless we're actually using the new STORE API, then
referencing the privkey by a name that the engine has registered with
the STORE code — either a URI scheme, or an ASN.1 parser that gets
invoked by the file_loader.

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

Re: [PATCH] support other engines for module signing

[James Bottomley]

On Wed, 2018-10-17 at 08:18 -0700, David Woodhouse wrote:
> On Wed, 2018-10-17 at 08:10 -0700, James Bottomley wrote:
> > On Wed, 2018-10-17 at 08:05 -0700, David Woodhouse wrote:
> > > On Wed, 2018-10-17 at 07:43 -0700, James Bottomley wrote:
> > > > On Wed, 2018-10-17 at 15:40 +0100, David Howells wrote:
> > > > > James Bottomley <James.Bottomley@HansenPartnership.com>
> > > > > wrote:
> > > > > 
> > > > > > > Allow sign-file to use any available OpenSSL engine, not
> > > > > > > limited
> > > > > > > to PKCS-11 by using "enginename:keyname" syntax.  We have
> > > > > > > to
> > > > > > > do a
> > > > > > > special case for pkcs11 key name passing.
> > > > > > 
> > > > > > There's actually already a proposal for this which David
> > > > > > (Howells)
> > > > > > has
> > > > > > been ignoring:
> > > > > 
> > > > > Not so much ignoring as it just keeps getting buried.
> > > > 
> > > > Understood.  What I really need is my patch testing by someone
> > > > at
> > > > Red
> > > > Hat: the pkcs11 token you use looks highly non-standard so
> > > > someone
> > > > needs to check that adding generic engine support doesn't break
> > > > it.
> > > 
> > > Que?
> > > 
> > > Which PKCS#11 token are you talking about? Or do you mean the
> > > PKCS#11
> > > engine, which is the normal one from 
> > > https://github.com/OpenSC/libp11
> > > 
> > > I think it does support using the UI for callbacks. Your trick of
> > > passing through to pem_pw_cb should probably work fine, as long
> > > as there's only ever one password. It should be trivial to test
> > > using SoftHSM or any other soft token, even if you have no actual
> > > hardware-based PKCS#11 tokens.
> > 
> > It didn't look like a normal one.  In fact it doesn't use UI
> > callbacks at all: it passes the key password in with an engine
> > command.  That's why I left it specifically cased out in the
> > file.  Now if it does actually work normally, the special casing
> > can be removed.
> 
> It looks just like yours. It uses the UI callbacks, but you can
> bypass those by providing the password in advance with an ENGINE_ctrl
> if you want.

Are we talking about the same thing?  This is the current RH code:

		ENGINE *e;

		e = ENGINE_by_id("pkcs11");
		ERR(!e, "Load PKCS#11 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);
		ERR(!private_key, "%s", private_key_name);

It uses an engine control command to load the key password not a UI. 
Now if it will work with a UI, I can fold the cases.  Can someone tell
me if it does work with a UI?

> > No, the requirement seemed to be to add the engine to the openssl
> > config file.  There's no reason why an additional command parameter
> > can't be added, though.
> 
> But ENGINE_load_private_key() needs to be used with a specific
> ENGINE, doesn't it?

Actually no.  One engine can't load another engine's key (or doesn't
understand another engine's URI) so you can iterate over all engines
until you get one that succeeds.  That was the principle of the
original "integrate engines into the main load paths" patch years ago. 
Even the engine lockout machinery isn't invoked until you actually have
a loaded key, so the path is safe.

To add a command specifier, you try that engine (and only that engine)
but the fallback behaviour would be to try them all because that's how
the token works today.  I told Mark I'd be happy to code this.

>  Unless we're actually using the new STORE API, then referencing the
> privkey by a name that the engine has registered with the STORE code
> — either a URI scheme, or an ASN.1 parser that gets invoked by the
> file_loader.

Tag recognition is useful but not required.  The only requirement is
that the engine be able to recognise something that's not its own key
an fail without any other implications.

James

Re: [PATCH] support other engines for module signing

[David Woodhouse]

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

On Wed, 2018-10-17 at 08:28 -0700, James Bottomley wrote:
> On Wed, 2018-10-17 at 08:18 -0700, David Woodhouse wrote:
> > It looks just like yours. It uses the UI callbacks, but you can
> > bypass those by providing the password in advance with an ENGINE_ctrl
> > if you want.
> 
> Are we talking about the same thing?  This is the current RH code:

Why are you calling this "RH code"? What does Red Hat have to do with
it? This is code in the upstream kernel, that I added (IIRC).

> 		ENGINE *e;
> 
> 		e = ENGINE_by_id("pkcs11");
> 		ERR(!e, "Load PKCS#11 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);
> 		ERR(!private_key, "%s", private_key_name);
> 
> It uses an engine control command to load the key password not a UI. 

James, that "PIN" ENGINE_ctrl is basically identical to what we need to
do with your TPM2 engine to provide a parent key password (which your
engine currently can't use a UI for ☹). It's hardly a strange and
unusual concept.

> Now if it will work with a UI, I can fold the cases.  Can someone tell
> me if it does work with a UI?

 $ git clone github.com:OpenSC/libp11
 $ cd libp11
 $ git grep UI_

... probably! You can easily test it.

> > > No, the requirement seemed to be to add the engine to the openssl
> > > config file.  There's no reason why an additional command parameter
> > > can't be added, though.
> > 
> > But ENGINE_load_private_key() needs to be used with a specific
> > ENGINE, doesn't it?
> 
> Actually no.  One engine can't load another engine's key (or doesn't
> understand another engine's URI) so you can iterate over all engines
> until you get one that succeeds.  That was the principle of the
> original "integrate engines into the main load paths" patch years ago. 
> Even the engine lockout machinery isn't invoked until you actually have
> a loaded key, so the path is safe.

Ah, OK... so we try ENGINE_load_private_key(NULL, private_key_name, …)?

> To add a command specifier, you try that engine (and only that engine)
> but the fallback behaviour would be to try them all because that's how
> the token works today.  I told Mark I'd be happy to code this.
> 
> >  Unless we're actually using the new STORE API, then referencing the
> > privkey by a name that the engine has registered with the STORE code
> > — either a URI scheme, or an ASN.1 parser that gets invoked by the
> > file_loader.
> 
> Tag recognition is useful but not required.  The only requirement is
> that the engine be able to recognise something that's not its own key
> an fail without any other implications.

Makes sense.


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

Re: [PATCH] support other engines for module signing

[David Woodhouse]

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

On Wed, 2018-10-17 at 08:48 -0700, David Woodhouse wrote:
> 
>  $ git clone github.com:OpenSC/libp11
>  $ cd libp11
>  $ git grep UI_
> 
> ... probably! You can easily test it.
> 

If you build openconnect and run 'make check' it populates a SoftHSM
token with its test key and uses that for auth. Both with and without
passwords. And possibly with the password ĂŻ (U+0102 U+017b) in both
UTF-8 and ISO8859-2 locales just for ultimate torture testing, or maybe
I only did that for file passwords... :)

You could trivially then try running 'sign-file' with the same SoftHSM
token to test.

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

Re: [PATCH] support other engines for module signing

[James Bottomley]

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

On Wed, 2018-10-17 at 08:48 -0700, David Woodhouse wrote:
> On Wed, 2018-10-17 at 08:28 -0700, James Bottomley wrote:
> > On Wed, 2018-10-17 at 08:18 -0700, David Woodhouse wrote:
> > > It looks just like yours. It uses the UI callbacks, but you can
> > > bypass those by providing the password in advance with an
> > > ENGINE_ctrl
> > > if you want.
> > 
> > Are we talking about the same thing?  This is the current RH code:
> 
> Why are you calling this "RH code"? What does Red Hat have to do with
> it? This is code in the upstream kernel, that I added (IIRC).

At the time it was added, RH was the only company doing pkcs11 based
module signing, so I assumed it was a RH token.

> > 		ENGINE *e;
> > 
> > 		e = ENGINE_by_id("pkcs11");
> > 		ERR(!e, "Load PKCS#11 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);
> > 		ERR(!private_key, "%s", private_key_name);
> > 
> > It uses an engine control command to load the key password not a
> > UI. 
> 
> James, that "PIN" ENGINE_ctrl is basically identical to what we need
> to do with your TPM2 engine to provide a parent key password (which
> your engine currently can't use a UI for ☹). It's hardly a strange
> and unusual concept.

OK, but this isn't the parent it's the key password, so what I think
you're saying is that all pkcs11 tokens can take either engine ctrl PIN
or a UI method to provide the password, so I can proceed with folding
the pkcs11 case into the UI engine path.  That's what I'll do.

James

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH] support other engines for module signing

[David Woodhouse]

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

On Wed, 2018-10-17 at 09:41 -0700, James Bottomley wrote:
> At the time it was added, RH was the only company doing pkcs11 based
> module signing, so I assumed it was a RH token.

I wasn't aware that Red Hat were using it.

> 
> OK, but this isn't the parent it's the key password, so what I think
> you're saying is that all pkcs11 tokens can take either engine ctrl PIN
> or a UI method to provide the password, so I can proceed with folding
> the pkcs11 case into the UI engine path.  That's what I'll do.

Seems reasonable. Don't forget to test it.

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