[PATCH] SSL: Add ENGINE_init() calls before using engines.

[Maxim Dounin]

Hello!

On Fri, Apr 27, 2018 at 11:27:57AM -0400, Anderson Sasaki wrote:

> Hello,
> 
> > > > In my opinion it would be better to have nginx working with engines in
> > > > both scenarios.
> > > > And is not a problem to call ENGINE_init() from multiple places, since
> > > > the API takes care of this case.
> > > 
> > > I'll check these statements in your next patch, but for now it
> > > seems an odd functionality to me, because we have openssl config
> > > and even nginx ssl_engine directive for that.
> > 
> > Note that the "ssl_engine" directive is not to initialize engines,
> > but rather to register an engine as the default one for operations it
> > supports.  It is designed to make it possible to work with various
> > SSL accelerators.
> 
> I believe this patch does not affect the "ssl_engine" directive anymore.
> I removed everything I could to make the patch minimal. And changed the log message to be more specific.
> I will send the other changes in a new thread.
> 
> The patch follows:
> 
> # HG changeset patch
> # User Anderson Toshiyuki Sasaki <ansasaki at redhat.com>
> # Date 1524841096 -7200
> #      Fri Apr 27 16:58:16 2018 +0200
> # Node ID f5b0a791092224ff5d3eaf4da9a95e6018e7235f
> # Parent  46c0c7ef4913011f3f1e073f9ac880b07b1a8154
> SSL: Add ENGINE_init() call before loading key.
> It is necessary to call ENGINE_init() before using an OpenSSL engine
> to get the engine functional reference. Without this, when
> ENGINE_load_private_key() is called, the engine is still unitialized.

In no particular order:

- Should be "SSL: added ..." (no capital letter after a semicolon, 
  prefer past tense).

- An empty line after the summary.

- Please prefer double spacing.

- "uniNItialized" 

> diff -r 46c0c7ef4913 -r f5b0a7910922 src/event/ngx_event_openssl.c
> --- a/src/event/ngx_event_openssl.c	Wed Apr 25 14:57:24 2018 +0300
> +++ b/src/event/ngx_event_openssl.c	Fri Apr 27 16:58:16 2018 +0200
> @@ -527,6 +527,13 @@
>              return NGX_ERROR;
>          }
>  
> +        if (!ENGINE_init(engine)) {

As previously noted at [1], this may need ENGINE_finish() to avoid 
leaking loaded engines.

> +            ngx_ssl_error(NGX_LOG_EMERG, ssl->log, 0,
> +                          "ENGINE_init(engine) failed");

There is no reason to log static string "engine" here.  Consider 
using engine name here.

> +            ENGINE_free(engine);
> +            return NGX_ERROR;
> +        }
> +
>          *last++ = ':';
>  
>          pkey = ENGINE_load_private_key(engine, (char *) last, 0, 0);

[1] http://mailman.nginx.org/pipermail/nginx-devel/2015-June/007081.html

-- 
Maxim Dounin
http://mdounin.ru/