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?

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.

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

Basically,

SSL_CTX_set_ecdh_auto(ssl->ctx, 1);

is equivalent to:

SSL_CTX_set_options(ssl->ctx, SSL_OP_SINGLE_ECDH_USE);
SSL_CTX_set_tmp_ecdh(ssl->ctx, ecdh);

Having said that, I don't mind re-adding it, if it's going to make you happier.

Best regards,
Piotr Sikora

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