[PATCH] Add ENGINE_init/finish directives around ENGINE_load_private_key.

[Bradley Hess]

Hello Maxim,

Ah, well that explains why a patch like this has never been upstreamed,
even though it exists in a bunch of places on teh interwebz.  Sorry,
I didn't do enough archeology here.

I didn't realize the `init = 1` workaround existed, so thanks for the
pointer there.  However, it would be ideal if users could use OpenSSL's
dynamic engine loading, and avoid authoring an OpenSSL config file.

>From the description in the issue you linked, it looks like the patch was
removed for OpenSSL 1.0.x compatibility.  Would you accept a patch that
supplies the init/finish directives only if the OpenSSL version >= 1.1.0?

At this point many distros have OpenSSL 1.1 and a fixed PKCS #11 engine;
for example, the patch I submitted worked smoothly with OpenSSL 1.1 and the
PKCS #11 engine available on Debian 11, and without any engine config.

Thanks a bunch,
Brad

On Tue, Dec 7, 2021 at 2:28 PM Maxim Dounin <mdounin at mdounin.ru> wrote:

> Hello!
>
> On Tue, Dec 07, 2021 at 11:45:01AM -0500, Bradley Hess wrote:
>
> > # HG changeset patch
> > # User Bradley Hess <bdhess at google.com>
> > # Date 1638894138 18000
> > #      Tue Dec 07 11:22:18 2021 -0500
> > # Node ID c3ec7674556519a9068c4e7a9f6279bbff6c3d31
> > # Parent  a7a77549265ef46f1f0fdb3897f4beabf9e09c40
> > Add ENGINE_init/finish directives around ENGINE_load_private_key.
> >
> > ENGINE_by_id creates a "structural reference" to an engine; for
> > actually performing cryptography, this ought to be a "functional
> > reference" per those two topics in the OpenSSL docs:
> > https://www.openssl.org/docs/man1.1.1/man3/ENGINE_load_private_key.html
> >
> > Unlike the default OpenSSL engine, the OpenSC PKCS #11 engine cannot
> > load a private key if it is not first initialized.
> >
> > diff -r a7a77549265e -r c3ec76745565 src/event/ngx_event_openssl.c
> > --- a/src/event/ngx_event_openssl.c Thu Nov 25 22:02:10 2021 +0300
> > +++ b/src/event/ngx_event_openssl.c Tue Dec 07 11:22:18 2021 -0500
> > @@ -734,16 +734,24 @@
> >              return NULL;
> >          }
> >
> > +        if (!ENGINE_init(engine)) {
> > +            *err = "ENGINE_init() failed";
> > +            ENGINE_free(engine);
> > +            return NULL;
> > +        }
> > +
> >          *last++ = ':';
> >
> >          pkey = ENGINE_load_private_key(engine, (char *) last, 0, 0);
> >
> >          if (pkey == NULL) {
> >              *err = "ENGINE_load_private_key() failed";
> > +            ENGINE_finish(engine);
> >              ENGINE_free(engine);
> >              return NULL;
> >          }
> >
> > +        ENGINE_finish(engine);
> >          ENGINE_free(engine);
> >
> >          return pkey;
>
> Previously, a similar patch was rejected, because it caused an
> immediate segfault for most affected users, see here:
>
> http://mailman.nginx.org/pipermail/nginx-devel/2018-May/011149.html
>
> If you think that things have changed since then, you may want to
> elaborate.
>
> --
> Maxim Dounin
> http://mdounin.ru/
> _______________________________________________
> nginx-devel mailing list
> nginx-devel at nginx.org
> http://mailman.nginx.org/mailman/listinfo/nginx-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.nginx.org/pipermail/nginx-devel/attachments/20211207/01e13fa6/attachment.htm>