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@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/
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel