[openssl-dev] [RFC v2 2/2] pem: load engine keys

[James Bottomley]

On Tue, 2016-12-06 at 17:47 +0100, Richard Levitte wrote:
> In message <1481042048.4406.14.camel at HansenPartnership.com> on Tue,
> 06 Dec 2016 08:34:08 -0800, James Bottomley <
> James.Bottomley at HansenPartnership.com> said:
> 
> James.Bottomley> On Tue, 2016-12-06 at 15:12 +0100, Richard Levitte
> wrote:
> James.Bottomley> > In message <
> 1480697558.2410.33.camel at HansenPartnership.com> on Fri,
> James.Bottomley> > 02 Dec 2016 08:52:38 -0800, James Bottomley <
> James.Bottomley> > James.Bottomley at HansenPartnership.com> said:
> James.Bottomley> > 
> James.Bottomley> > When I made that argument, I hadn't thought and
> felt things through
> James.Bottomley> > entirely.  Truth be told, I'm feeling very uneasy
> handing over the
> James.Bottomley> > reading and parsing of the PEM file to an engine. 
> However, handing
> James.Bottomley> > over the decoded data and leaving it to the engine
> to do something
> James.Bottomley> > sensible with it, no issues at all.
> James.Bottomley> 
> James.Bottomley> OK, can I pick on this a bit (I'll look at the store
> stuff later and
> James.Bottomley> reply after I've played with it)?  What is it that
> you imagine handing
> James.Bottomley> to the engine?  Some type of buffer that contains
> the full PEM
> James.Bottomley> representation?
> 
> In my STORE branch, you will find this in
> include/openssl/store_file.h:
> 
>     /*
>      * The try_decode function is called to check if the blob of data
> can
>      * be used by this handler, and if it can, decodes it into a
> supported
>      * OpenSSL and returns a STORE_INFO with the recorded data.
>      * Input:
>      *    pem_name:     If this blob comes from a PEM file, this
> holds
>      *                  the PEM name.  If it comes from another type
> of
>      *                  file, this is NULL.
>      *    blob:         The blob of data to match with what this
> handler
>      *                  can use.
>      *    len:          The length of the blob.
>      *    handler_ctx:  For a handler marked repeatable, this pointer
> can
>      *                  be used to create a context for the handler. 
> IT IS
>      *                  THE HANDLER'S RESPONSIBILITY TO CREATE AND
> DESTROY
>      *                  THIS CONTEXT APPROPRIATELY, i.e. create on
> first call
>      *                  and destroy when about to return NULL.
>      *    password_ui:  Application UI method for getting a password.
>      *    password_ui_data:
>      *                  Application data to be passed to password_ui
> when
>      *                  it's called.
>      * Output:
>      *    a STORE_INFO
>      */
>     typedef STORE_INFO *(*STORE_FILE_try_decode_fn)(const char
> *pem_name,
>                                                     const unsigned
> char *blob,
>                                                     size_t len,
>                                                     void
> **handler_ctx,
>                                                     const UI_METHOD
> *password_ui,
>                                                     void
> *password_ui_data);
> 
> This is the central function that tries to see if it can decode
> whatever's thrown at it.  As you can see, it gets handed the PEM name
> if it's called as part of a PEM read.  The |blob| is the decoded *and
> decrypted* data.
> 
> James.Bottomley> The reason I chose a BIO is that it's the basic
> abstract data handler
> James.Bottomley> for openssl. I can hand a buffer to the engine,
> sure, but I'd need to
> James.Bottomley> transform it to a BIO again (because it would need
> PEM parsing and all
> James.Bottomley> the PEM parsers speak BIOs), so it feels suboptimal.
> 
> With a try_decode function, I really do not see why there's a need to
> deal with the BIO...

OK, I get what you want.  There are two problems with this.  The first
is easy: I'm eventually going to need the BIO header as well because
TPM2 keys are going to need to describe external things that aren't in
the key representation (and even TPM1 keys might need to say who their
parent key is).  However, this is fixable by simply expanding your
prototype above to have const char *pem_name and const char *pem_header
... is that OK?

The next problem is that this is slightly harder simply to insert into
the PEM code.  The BIO parsing is done in PEM_bytes_read_bio() not
 PEM_read_bio_PrivateKey().  The easy way to cope with this would be to
move PEM parsing into the ENGINE_find_engine_load_key() function and
then hand the name, header, blob to the engine.  The disadvantage of
this is that we'll end up pulling the PEM apart twice: once in
ENGINE_find_engine_load_key() and then again in PEM_bytes_read_bio(). 
 Is this OK?  If it is I can code up a v3.  If not, I can think about
how we might integrate this into PEM_bytes_read_bio().

James