Hello,

Thank you for your feedback.

> > # HG changeset patch
> > # User Anderson Toshiyuki Sasaki <ansasaki@redhat.com>
> > # Date 1524670310 -7200
> > # Wed Apr 25 17:31:50 2018 +0200
> > # Node ID f916a804d526c1acb493c7c4e5c114d947e0eed1
> > # Parent 46c0c7ef4913011f3f1e073f9ac880b07b1a8154
> > SSL: Add ENGINE_init() calls before using engines.
> > It is necessary to call ENGINE_init() before using a OpenSSL engine
> > to get the engine functional reference.
> >
> > diff -r 46c0c7ef4913 -r f916a804d526 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 Wed Apr 25 17:31:50 2018 +0200
> > @@ -527,27 +527,44 @@
> > return NGX_ERROR;
> > }
> >
> > + if (!ENGINE_init(engine)) {
> > + ngx_ssl_error(NGX_LOG_EMERG, ssl->log, 0,
> > + "ENGINE_init(\"%s\") failed", p);
> > + ENGINE_free(engine);
> > + return NGX_ERROR;
> > + }
> > +
> > *last++ = ':';
> >
> > pkey = ENGINE_load_private_key(engine, (char *) last, 0, 0);
> >
> > if (pkey == NULL) {
> > ngx_ssl_error(NGX_LOG_EMERG, ssl->log, 0,
> > - "ENGINE_load_private_key(\"%s\") failed", last);
> > + "ENGINE_load_private_key(\"%s\", %s, %d, %d)
> > failed",
> > + p, last, 0, 0);
> > ENGINE_free(engine);
> > return NGX_ERROR;
> > }
> >
> > - ENGINE_free(engine);
> > + if (!ENGINE_set_default(engine, ENGINE_METHOD_PKEY_METHS)) {
> > + ngx_ssl_error(NGX_LOG_EMERG, ssl->log, 0,
> > + "ENGINE_set_default(\"%s\", %s) failed",
> > + p, "ENGINE_METHOD_PKEY_METHS");
> > + EVP_PKEY_free(pkey);
> > + ENGINE_free(engine);
> > + return NGX_ERROR;
> > + }
>
> Apart from the ENGINE_init() discussion you are having with
> Dmirty, the patch seems to contain various unrelated changes,
> including logging changes and the ENGINE_set_default() call quoted
> above.

I agree that I could avoid changing the log messages. I will remove theses changes from the patch.

>
> If you really think these changes are needed, you may want to
> either submit these changes separately (or document why these
> changes should be in the ENGINE_init() patch in the commit log, if
> you really think these changes should be an integral part of the
> ENGINE_init() patch).

I will separate the changes in different patches.

>
> [...]
>
> > @@ -4215,13 +4232,18 @@
> > return NGX_CONF_ERROR;
> > }
> >
> > - if (ENGINE_set_default(engine, ENGINE_METHOD_ALL) == 0) {
> > - ngx_ssl_error(NGX_LOG_EMERG, cf->log, 0,
> > - "ENGINE_set_default(\"%V\", ENGINE_METHOD_ALL)
> > failed",
> > + if (!ENGINE_init(engine)) {
> > + ngx_ssl_error(NGX_LOG_EMERG, cf->log, 0, "ENGINE_init(\"%V\")
> > failed",
> > &value[1]);
> > -
> > ENGINE_free(engine);
> > -
> > + return NGX_CONF_ERROR;
> > + }
> > +
> > + if (ENGINE_set_default(engine, ENGINE_METHOD_PKEY_METHS) == 0) {
> > + ngx_ssl_error(NGX_LOG_EMERG, cf->log, 0,
> > + "ENGINE_set_default(\"%V\", %s) failed",
> > + &value[1], "ENGINE_METHOD_PKEY_METHS");
> > + ENGINE_free(engine);
> > return NGX_CONF_ERROR;
> > }
>
> Note well that the change in the ENGINE_set_default() arguments
> here seems to be simply wrong.

Here (and above in the other call to ENGINE_set_default()) I set the engine as the default choice only for private key operations.
Otherwise any call to the OpenSSL API will be handled to the engine. This could be the intention or not.
Anyway, I will separate these changes from the essential patch.

Best Regards,
Anderson
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel