[PATCH] SSL: support automatic selection of ECDH temporary key parameters

[Maxim Dounin]

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/