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

[Maxim Dounin]

Hello!

On Wed, Apr 25, 2018 at 11:52:45AM -0400, Anderson Sasaki wrote:

> # HG changeset patch
> # User Anderson Toshiyuki Sasaki <ansasaki at 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.

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).

[...]

> @@ -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.

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