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
