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

[Maxim Dounin]

Hello!

On Fri, May 18, 2018 at 04:36:53AM -0400, Anderson Sasaki wrote:

> Hello,
>  
> > The patch looks correct to me.  Though it causes a segmentation
> > faults within pkcs11 engine when using such loaded keys at least
> > on Ubuntu 18.04 (OpenSSL 1.1.0g, pkcs11 engine from libp11 0.4.7).
> > Segmentation faults can be reproduced with the test you've sent
> > earlier.
> > 
> > Using an explitic "init = 1" in openssl.conf resolves this, as
> > well as commenting out ENGINE_finish(), so it looks like it cannot
> > handle ENGINE_finish() while certificates loaded from the engine
> > are still in use.
> > 
> > Possible options might be:
> > 
> > - avoid any changes, and require "init = 1" as we effectively do
> >   now;
> > 
> > - add explicit lists of engines initialized, and call
> >   ENGINE_finish() once no longer needed (probably somewhere in
> >   ngx_ssl_cleanup_ctx());
> > 
> > - avoid calling ENGINE_finish() with appropriate explanation of
> >   the problem;
> > 
> > - dig further into what goes on in OpenSSL / pkcs11 engine, and
> >   fix things (might be already resolved in [1]).
> > 
> > [1]
> > https://github.com/OpenSC/libp11/commit/da725ab727342083478150a203a3c80c4551feb4
> 
> The root of the problem is solved in the patch you pointed out 
> above. The libp11-0.4.7 release is missing this 
> EVP_PKEY_set1_engine() call. Without this, the engine is not 
> properly associated with the EVP_PKEY object, preventing the 
> OpenSSL automatic re-initialization of the engine to take place 
> when the key is used.
> 
> With the inclusion of such patch, the ENGINE_finish() can be 
> safely called. As long as the key keeps the structural reference 
> to the engine, it will be re-initialized when needed.
> 
> I've tested in Fedora, where the same problem occurs. Since I am 
> currently a co-maintainer of the engine in Fedora, I can fix it 
> there. But I can't fix it on Ubuntu.

Ok, so it looks like:

- With OpenSSL 1.0.x, it seems that it is not at all possible to 
  correctly use keys loaded from an engine without this engine 
  being referenced elsewhere.  Trying to use "ENGINE_init(); 
  ENGINE_load_private_key(); ENGINE_finish();" will result in 
  a segmentation fault unless the engine is referenced elsewhere 
  (and hence "ENGINE_init()" is not needed).

- With OpenSSL 1.1.x, it is now possible to use 
  EVP_PKEY_set1_engine() to preserve a reference, but neither 
  OpenSSL 1.1.x nor fixed pkcs11 engine is available on
  most platforms right now.

Commiting the patch as is means that we'll change an easily 
fixable "not initialized" error to a segmentation fault for most 
users affected by the patch.  It doesn't look like a good 
solution.

I don't like the idea of not calling ENGINE_finish() either, as 
this approach can eventually result in a reference counter 
overflow and another segmentation fault.

A simpliest solution seems to preserve things as is, at least for 
now, requiring an explicit engine initialization either via 
openssl.conf or via (misuse of) the "ssl_engine" directive.

I've also tried postponing ENGINE_finish() till 
ngx_ssl_cleanup_ctx(), but I can't say I like this approach.  Just 
for the record, patch below:

diff --git a/src/event/ngx_event_openssl.c b/src/event/ngx_event_openssl.c
--- a/src/event/ngx_event_openssl.c
+++ b/src/event/ngx_event_openssl.c
@@ -114,6 +114,7 @@ int  ngx_ssl_certificate_index;
 int  ngx_ssl_next_certificate_index;
 int  ngx_ssl_certificate_name_index;
 int  ngx_ssl_stapling_index;
+int  ngx_ssl_engine_index;
 
 
 ngx_int_t
@@ -225,6 +226,13 @@ ngx_ssl_init(ngx_log_t *log)
 return NGX_ERROR;
 }
 
+ngx_ssl_engine_index = X509_get_ex_new_index(0, NULL, NULL, NULL, NULL);
+
+if (ngx_ssl_engine_index == -1) {
+ngx_ssl_error(NGX_LOG_ALERT, log, 0, "X509_get_ex_new_index() failed");
+return NGX_ERROR;
+}
+
 return NGX_OK;
 }
 
@@ -534,6 +542,16 @@ ngx_ssl_certificate(ngx_conf_t *cf, ngx_
 return NGX_ERROR;
 }
 
+x509 = SSL_CTX_get_ex_data(ssl->ctx, ngx_ssl_certificate_index);
+
+if (X509_set_ex_data(x509, ngx_ssl_engine_index, engine) == 0) {
+ngx_ssl_error(NGX_LOG_EMERG, ssl->log, 0,
+  "X509_set_ex_data() failed");
+ENGINE_finish(engine);
+ENGINE_free(engine);
+return NGX_ERROR;
+}
+
 *last++ = ':';
 
 pkey = ENGINE_load_private_key(engine, (char *) last, 0, 0);
@@ -546,7 +564,6 @@ ngx_ssl_certificate(ngx_conf_t *cf, ngx_
 return NGX_ERROR;
 }
 
-ENGINE_finish(engine);
 ENGINE_free(engine);
 
 if (SSL_CTX_use_PrivateKey(ssl->ctx, pkey) == 0) {
@@ -3154,11 +3171,20 @@ ngx_ssl_cleanup_ctx(void *data)
 {
 ngx_ssl_t  *ssl = data;
 
-X509  *cert, *next;
+X509*cert, *next;
+#ifndef OPENSSL_NO_ENGINE
+ENGINE  *engine;
+#endif
 
 cert = SSL_CTX_get_ex_data(ssl->ctx, ngx_ssl_certificate_index);
 
 while (cert) {

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

[Anderson Sasaki]

Hello,
 
> The patch looks correct to me.  Though it causes a segmentation
> faults within pkcs11 engine when using such loaded keys at least
> on Ubuntu 18.04 (OpenSSL 1.1.0g, pkcs11 engine from libp11 0.4.7).
> Segmentation faults can be reproduced with the test you've sent
> earlier.
> 
> Using an explitic "init = 1" in openssl.conf resolves this, as
> well as commenting out ENGINE_finish(), so it looks like it cannot
> handle ENGINE_finish() while certificates loaded from the engine
> are still in use.
> 
> Possible options might be:
> 
> - avoid any changes, and require "init = 1" as we effectively do
>   now;
> 
> - add explicit lists of engines initialized, and call
>   ENGINE_finish() once no longer needed (probably somewhere in
>   ngx_ssl_cleanup_ctx());
> 
> - avoid calling ENGINE_finish() with appropriate explanation of
>   the problem;
> 
> - dig further into what goes on in OpenSSL / pkcs11 engine, and
>   fix things (might be already resolved in [1]).
> 
> [1]
> https://github.com/OpenSC/libp11/commit/da725ab727342083478150a203a3c80c4551feb4

The root of the problem is solved in the patch you pointed out above. The 
libp11-0.4.7 release is missing this EVP_PKEY_set1_engine() call. Without this, 
the engine is not properly associated with the EVP_PKEY object, preventing the 
OpenSSL automatic re-initialization of the engine to take place when the key is 
used.

With the inclusion of such patch, the ENGINE_finish() can be safely called. As 
long as the key keeps the structural reference to the engine, it will be 
re-initialized when needed.

I've tested in Fedora, where the same problem occurs. Since I am currently a 
co-maintainer of the engine in Fedora, I can fix it there. But I can't fix it 
on Ubuntu.

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

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

[Maxim Dounin]

Hello!

On Thu, May 10, 2018 at 12:42:58PM -0400, Anderson Sasaki wrote:

> Hello,
> 
> Thanks again for the feedback.
> 
> > 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"
> 
> The proposed changes were applied in the new patch version.
> 
> > > 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.
> 
> Added ENGINE_finish() calls.
> 
> > 
> > > +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.
> 
> Logging the engine name instead.
> 
> The patch follows:
> 
> # HG changeset patch
> # User Anderson Toshiyuki Sasaki 
> # Date 1525954250 -7200
> #  Thu May 10 14:10:50 2018 +0200
> # Node ID e7d0387ad229ca47514f346c8b692e6f388d72e1
> # Parent  ceab908790c4eb7cd01c40bd16581ef794222ca5
> SSL: added 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 uninitialized.
> 
> diff -r ceab908790c4 -r e7d0387ad229 src/event/ngx_event_openssl.c
> --- a/src/event/ngx_event_openssl.c   Tue Apr 24 14:04:59 2018 +0300
> +++ b/src/event/ngx_event_openssl.c   Thu May 10 14:10:50 2018 +0200
> @@ -527,6 +527,13 @@
>  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);
> @@ -534,10 +541,12 @@
>  if (pkey == NULL) {
>  ngx_ssl_error(NGX_LOG_EMERG, ssl->log, 0,
>"ENGINE_load_private_key(\"%s\") failed", last);
> +ENGINE_finish(engine);
>  ENGINE_free(engine);
>  return NGX_ERROR;
>  }
>  
> +ENGINE_finish(engine);
>  ENGINE_free(engine);
>  
>  if (SSL_CTX_use_PrivateKey(ssl->ctx, pkey) == 0) {

The patch looks correct to me.  Though it causes a segmentation 
faults within pkcs11 engine when using such loaded keys at least 
on Ubuntu 18.04 (OpenSSL 1.1.0g, pkcs11 engine from libp11 0.4.7).  
Segmentation faults can be reproduced with the test you've sent 
earlier.

Using an explitic "init = 1" in openssl.conf resolves this, as 
well as commenting out ENGINE_finish(), so it looks like it cannot 
handle ENGINE_finish() while certificates loaded from the engine 
are still in use.

Possible options might be:

- avoid any changes, and require "init = 1" as we effectively do 
  now;

- add explicit lists of engines initialized, and call 
  ENGINE_finish() once no longer needed (probably somewhere in 
  ngx_ssl_cleanup_ctx());

- avoid calling ENGINE_finish() with appropriate explanation of 
  the problem; 

- dig further into what goes on in OpenSSL / pkcs11 engine, and 
  fix things (might be already resolved in [1]).

[1] 
https://github.com/OpenSC/libp11/commit/da725ab727342083478150a203a3c80c4551feb4

-- 
Maxim Dounin
http://mdounin.ru/
___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel

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

[Anderson Sasaki]

Hello,

Thanks again for the feedback.

> 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"

The proposed changes were applied in the new patch version.

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

Added ENGINE_finish() calls.

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

Logging the engine name instead.

The patch follows:

# HG changeset patch
# User Anderson Toshiyuki Sasaki 
# Date 1525954250 -7200
#  Thu May 10 14:10:50 2018 +0200
# Node ID e7d0387ad229ca47514f346c8b692e6f388d72e1
# Parent  ceab908790c4eb7cd01c40bd16581ef794222ca5
SSL: added 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 uninitialized.

diff -r ceab908790c4 -r e7d0387ad229 src/event/ngx_event_openssl.c
--- a/src/event/ngx_event_openssl.c Tue Apr 24 14:04:59 2018 +0300
+++ b/src/event/ngx_event_openssl.c Thu May 10 14:10:50 2018 +0200
@@ -527,6 +527,13 @@
 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);
@@ -534,10 +541,12 @@
 if (pkey == NULL) {
 ngx_ssl_error(NGX_LOG_EMERG, ssl->log, 0,
   "ENGINE_load_private_key(\"%s\") failed", last);
+ENGINE_finish(engine);
 ENGINE_free(engine);
 return NGX_ERROR;
 }
 
+ENGINE_finish(engine);
 ENGINE_free(engine);
 
 if (SSL_CTX_use_PrivateKey(ssl->ctx, pkey) == 0) {
___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel

Re: [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 
> # 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

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

[Anderson Sasaki]

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

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)) {
+ngx_ssl_error(NGX_LOG_EMERG, ssl->log, 0,
+  "ENGINE_init(engine) failed");
+ENGINE_free(engine);
+return NGX_ERROR;
+}
+
 *last++ = ':';
 
 pkey = ENGINE_load_private_key(engine, (char *) last, 0, 0);
___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel

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

[Maxim Dounin]

Hello!

On Thu, Apr 26, 2018 at 07:31:37PM +, Пичулин Дмитрий Николаевич wrote:

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

-- 
Maxim Dounin
http://mdounin.ru/
___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel

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

[Пичулин Дмитрий Николаевич]

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

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

[Anderson Sasaki]

Hello,

Thank you for your feedback.

> > # HG changeset patch
> > # User Anderson Toshiyuki Sasaki 
> > # 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",
> >[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",
> > +  [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

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

[Anderson Sasaki]

Hello,

> The original patch was tested on the same setup:
> http://mailman.nginx.org/pipermail/nginx-devel/2014-October/006151.html
> 
> Do you insist that it does not work in the current state?

Yes, the problem is that the automatic initialization only take place for the 
default engines, which have to be configured through the configuration file.
For the engines that are used ad-hoc, the ENGINE_init() have to be called 
explicitly.

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

Re: [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 
> # 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",
>[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",
> +  [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/
___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel

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

[Пичулин Дмитрий Николаевич]

The original patch was tested on the same setup: 
http://mailman.nginx.org/pipermail/nginx-devel/2014-October/006151.html

Do you insist that it does not work in the current state?
___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel

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

[Anderson Sasaki]

Hello,

> Typically engines initialize themselves in bind(), if not, they are
> initialized by openssl.cnf ("default_algorithms"), why use "init = 0" in
> your openssl config and rely this openssl engine stuff to nginx?

Following the OpenSSL documentation, the application is responsible for 
initializing the engines.
Some engines, like the engine_pkcs11, rely on this and expects an explicit call 
to ENGINE_init().
The engines which initialize themselves, as far as I know, are actually doing a 
workaround to avoid the problem with non-compliant applications.

In the specific case of engine_pkcs11, setting the "init" and 
"default_algorithms" in openssl.cnf do not initialize the engine.

It would be interesting for nginx to follow the OpenSSL documentation and be 
compatible with more engines.
For the specific case of the engine_pkcs11, it is interesting to support it 
because it allows using PKCS#11 URIs transparently.
There were efforts in the past ([0], [1], [2]) to improve the support for 
PKCS#11 integration with nginx.

[0] http://mailman.nginx.org/pipermail/nginx-devel/2014-November/006188.html
[1] http://mailman.nginx.org/pipermail/nginx-devel/2015-April/006786.html
[2] http://mailman.nginx.org/pipermail/nginx-devel/2015-June/007074.html

Best regards,
Anderson


___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel

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

[Пичулин Дмитрий Николаевич]

Typically engines initialize themselves in bind(), if not, they are initialized 
by openssl.cnf ("default_algorithms"), why use "init = 0" in your openssl 
config and rely this openssl engine stuff to nginx?
___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel

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

[Anderson Sasaki]

Hello,

Following there is a test using the engine_pkcs11 [0] and softhsm [1].
The key is referenced in the device using PKCS#11 URI [2].

The test was based on an existing test, ssl_engine_keys.t

[0] https://github.com/OpenSC/libp11
[1] https://github.com/opendnssec/SoftHSMv2
[2] https://tools.ietf.org/html/rfc7512

Best regards,
Anderson

# HG changeset patch
# User Anderson Toshiyuki Sasaki 
# Date 1524668496 -7200
#  Wed Apr 25 17:01:36 2018 +0200
# Node ID 84d417fa2dda58b027184ca3e34479e1aa7cbd9c
# Parent  d6daf03478adb5fe7523eab0b87c9372261422d7
Tests: Add a SSL test using PKCS#11 URI.
The test run a nginx instance with ssl enabled using a
PKCS#11 URI to reference a key from a device.

diff -r d6daf03478ad -r 84d417fa2dda ssl_pkcs11_uri.t
--- /dev/null   Thu Jan 01 00:00:00 1970 +
+++ b/ssl_pkcs11_uri.t  Wed Apr 25 17:01:36 2018 +0200
@@ -0,0 +1,172 @@
+#!/usr/bin/perl
+
+# (C) Sergey Kandaurov
+# (C) Nginx, Inc.
+
+# Tests for http ssl module, loading "engine:pkcs11:" keys.
+
+###
+
+use warnings;
+use strict;
+
+use Test::More;
+
+BEGIN { use FindBin; chdir($FindBin::Bin); }
+
+use lib 'lib';
+use Test::Nginx;
+
+###
+
+select STDERR; $| = 1;
+select STDOUT; $| = 1;
+
+plan(skip_all => 'win32') if $^O eq 'MSWin32';
+
+plan(skip_all => 'may not work, leaves coredump')
+   unless $ENV{TEST_NGINX_UNSAFE};
+
+my $t = Test::Nginx->new()->has(qw/http proxy http_ssl/)->has_daemon('openssl')
+   ->has_daemon('softhsm2-util')->has_daemon('pkcs11-tool')->plan(1);
+
+$t->write_file_expand('nginx.conf', <<'EOF');
+
+%%TEST_GLOBALS%%
+
+daemon off;
+
+events {
+}
+
+http {
+%%TEST_GLOBALS_HTTP%%
+
+server {
+listen   127.0.0.1:8081 ssl;
+listen   127.0.0.1:8080;
+server_name  localhost;
+
+ssl_certificate_key 
"engine:pkcs11:pkcs11:token=NginxZero;object=nx_key_0;type=private;pin-value=1234";
+ssl_certificate localhost.crt;
+
+location / {
+# index index.html by default
+}
+location /proxy {
+proxy_pass https://127.0.0.1:8081/;
+}
+}
+}
+
+EOF
+
+# Create a OpenSSL configuration file
+my $module_path = `find /usr -name *libsofthsm*.so 2>/dev/null | head -n 1 | \
+tr -d "\n"`;
+my $dynamic_path = `find /usr -name *pkcs11*.so 2>/dev/null | grep engine | \
+head -n 1 | tr -d "\n"`;
+
+$t->write_file('openssl.conf', <testdir();
+
+# Test if OpenSSL is already configured with the engine pkcs11
+# If not, create a local configuration
+my $openssl_config;
+eval "openssl engine -t pkcs11";
+if ($? == 0) {
+$openssl_config = "";
+} else {
+$openssl_config = "-config $d/openssl.conf";
+}
+
+# Configure SoftHSM to create a local database for the keys
+$t->write_file('softhsm.conf', <>$d/openssl.out 2>&1") == 0
+or exit($?);
+
+   system('pkcs11-tool --module='
+. "$module_path -p 1234 -l -k -d 0 -a nx_key_0 --key-type rsa:1024 "
+   . ">>$d/openssl.out 2>&1") == 0
+or exit($?);
+
+   system('openssl req -x509 -new -engine pkcs11 '
+   . "$openssl_config -subj \"/CN=$name\" "
+   . "-out $d/$name.crt -keyform engine "
+. '-key "pkcs11:token=NginxZero;object=nx_key_0;type=private'
+. ';pin-value=1234" '
+   . ">>$d/openssl.out 2>&1") == 0
+   or exit($?);
+}
+
+$t->run();
+
+$t->write_file('index.html', '');
+
+###
+
+like(http_get('/proxy', socket => get_ssl_socket()), qr/200 OK/, 'https');
+
+###
+#
+sub get_ssl_socket {
+   my $s;
+
+   eval {
+   local $SIG{ALRM} = sub { die "timeout\n" };
+   local $SIG{PIPE} = sub { die "sigpipe\n" };
+   alarm(2);
+   $s = IO::Socket::SSL->new(
+   Proto => 'tcp',
+   PeerAddr => 'localhost:',
+