Hello!

On Sun, Apr 06, 2014 at 07:09:41PM -0700, Piotr Sikora wrote:

> Hey Maxim,
>
> > Comments about the patch below, in no particular order:
> >
> > - Suggested code doesn't seem to allow to use the default list of
> > curves, as normally available with just a call to
> > SSL_CTX_set_ecdh_auto(); this seems to be what OpenSSL
> > recommends to use by default, and we may want to follow.
>
> But isn't that the way nginx usually interacts with OpenSSL? i.e.
> always calling SSL_CTX_set_cipher_list(), even with the default
> "DEFAULT" value?

The problem is that there is no equivalent "default" value in case
of curves. But I think we can live with it, at least till OpenSSL
folks will introduce one.

> Also, right now nginx uses NIST P-256 curve. Calling
> SSL_CTX_set_ecdh_auto() without limiting list of supported curves
> enables all of them, with preference for the strongest ones, which
> means that all modern browsers will start using NIST P-521 and clients
> compiled against OpenSSL will start using NIST B-571.
>
> This results in rather significant performance loss:
>
> op op/s
> 256 bit ecdh (nistp256) 0.0002s 4589.7
> 521 bit ecdh (nistp521) 0.0006s 1551.9
> 571 bit ecdh (nistb571) 0.0030s 330.9
>
> and I don't think that it's a good idea to silently change used curve
> for users using default configuration.

Agree, preserving prime256v1 as the default looks reasonable.

> > - Error messages in the ngx_ssl_ecdh_curve() are way off from
> > what's normally used in ngx_event_openssl.c, and probably
> > it's not a good idea to use similar messages in the new code.
>
> Right, my bad.
>
> > - If nginx was compiled with OpenSSL 1.0.2, but used with an
> > older version, things will not work at all; this is not something
> > completely unacceptable, but it's something we may want to
> > avoid.
>
> Will look into it.
>
> > - SSL_CTX_set_options(SSL_OP_SINGLE_ECDH_USE) is not used
> > with OpenSSL 1.0.2, and this looks just wrong.
>
> Well, it just looks wrong ;)
>
> If you dig into OpenSSL's code, you'll notice that this option is
> checked only in 3 places: when setting SSL_CTX_set_tmp_ecdh() /
> SSL_set_tmp_ecdh() to see if we need to generate "persistent"
> temporary key and during ServerKeyExchange phase to see if we need to
> generate ephemeral temporary key. This check consists of 3 conditions
> and first two are fulfilled via SSL_CTX_set_ecdh_auto(), so
> SSL_OP_SINGLE_ECDH_USE is never check when using it.

While the SSL_OP_SINGLE_ECDH_USE may be unneeded with current
OpenSSL code in case of SSL_CTX_set_ecdh_auto(), I would prefer to
keep the option set anyway - as nothing stops OpenSSL from
implementing a key cache for this case.

--
Maxim Dounin
http://nginx.org/

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