Hello!

On Tue, Jan 04, 2011 at 10:34:07AM -0500, timo2 wrote:

> I was able to add support for elliptic curve cryptography. Nginx has to
> be compiled with Openssl 1.0.0 libraries, though. The patch against
> nginx-0.8.54 is here (sorry I do not know how to add attachment).

You may want to use nginx-devel@ (cc'd) mailing list instead, and
use latest development version (0.9.3) for patches (it's very
close, but just to be sure).

Most serious problem with the patch is it won't compile with older
OpenSSL versions: nginx is currently expected to compile with
OpenSSL 0.9.7+.

As this patch uses OpenSSL 1.0.0+ only functions / data types - it
should be guarded with appropriate preprocessor / configure
checks.

Some minor problems are noted below.

> Basicly, it ads extra configuration parameter ssl_eccurve with
> which one can specify the elliptic curve. If this parameter is
> missing then the default secp224r1 curve is used.

It looks like OpenSSL's s_server as well as Apache defaults to
nistp256 aka secp256r1. Any specific reasons for secp224r1?

> diff -rupN nginx-0.8.54/src/event/ngx_event_openssl.c
> nginx-0.8.54p/src/event/ngx_event_openssl.c
> --- nginx-0.8.54/src/event/ngx_event_openssl.c 2010-07-29
> 12:30:15.000000000 +0300
> +++ nginx-0.8.54p/src/event/ngx_event_openssl.c 2011-01-04
> 17:11:08.000000000 +0200
> @@ -479,6 +479,60 @@ ngx_ssl_dhparam(ngx_conf_t *cf, ngx_ssl_
> return NGX_OK;
> }
>
> +ngx_int_t
> +ngx_ssl_eccurve(ngx_conf_t *cf, ngx_ssl_t *ssl, ngx_str_t
> *named_curve)

Using "name" instead of "named_curve" probably would be better
here.

> +{
> + /*
> + * Elliptic-Curve Diffie-Hellman parameters are either "named
> curves"
> + * from RFC 4492 section 5.1.1, or explicitly described curves
> over
> + * binary fields. OpenSSL only supports the "named curves", which
> provide
> + * maximum interoperability.
> + */
> +
> + EC_KEY *ecdh=NULL;

Any reason why this should be initialized to NULL?... (1)

> + int nid;
> +
> + if (named_curve->len) {

It's probably good idea to reverse this check to be in line with
ngx_ssl_dhparam().

> + nid = OBJ_sn2nid( (const char *) named_curve->data);

Casting to "const" isn't needed, only to "char *". Using space in
"( (" isn't needed as well (style).

> +
> + if (nid == 0) {
> + ngx_ssl_error(NGX_LOG_EMERG, ssl->log, 0, "Unknown curve
> name (%s)\n", named_curve->data);

No need to use "\n". Applies to other places as well.

> +
> + EC_KEY_free(ecdh);

(1) ... and this call EC_KEY_free(), while we are perfectly sure
we don't have anything usefull in ecdh?

> + return NGX_ERROR;
> + }
> +
> + ecdh = EC_KEY_new_by_curve_name(nid);
> +
> + if (ecdh == NULL) {
> + ngx_ssl_error(NGX_LOG_EMERG, ssl->log, 0, "Unable to create
> curve (%s)\n", named_curve->data);
> +
> + EC_KEY_free(ecdh);

The same here. We know for sure ecdh is NULL.

> + return NGX_ERROR;
> + }
> +
> + SSL_CTX_set_tmp_ecdh(ssl->ctx, ecdh);
> +
> + EC_KEY_free(ecdh);
> +
> + return NGX_OK;
> + }
> +
> + ecdh = EC_KEY_new_by_curve_name(NID_secp224r1);
> +
> + if (ecdh == NULL) {
> + ngx_ssl_error(NGX_LOG_EMERG, ssl->log, 0, "Unable to create
> curve (%s)\n", named_curve->data);
> +
> + EC_KEY_free(ecdh);

And here.

> + return NGX_ERROR;
> + }
> +
> + SSL_CTX_set_tmp_ecdh(ssl->ctx, ecdh);
> +
> + EC_KEY_free(ecdh);
> +
> + return NGX_OK;
> +}
>
> ngx_int_t
> ngx_ssl_create_connection(ngx_ssl_t *ssl, ngx_connection_t *c,
> ngx_uint_t flags)
> diff -rupN nginx-0.8.54/src/event/ngx_event_openssl.h
> nginx-0.8.54p/src/event/ngx_event_openssl.h
> --- nginx-0.8.54/src/event/ngx_event_openssl.h 2010-03-03
> 18:23:14.000000000 +0200
> +++ nginx-0.8.54p/src/event/ngx_event_openssl.h 2011-01-04
> 14:57:00.000000000 +0200
> @@ -101,6 +101,7 @@ ngx_int_t ngx_ssl_client_certificate(ngx
> ngx_int_t ngx_ssl_crl(ngx_conf_t *cf, ngx_ssl_t *ssl, ngx_str_t *crl);
> ngx_int_t ngx_ssl_generate_rsa512_key(ngx_ssl_t *ssl);
> ngx_int_t ngx_ssl_dhparam(ngx_conf_t *cf, ngx_ssl_t *ssl, ngx_str_t
> *file);
> +ngx_int_t ngx_ssl_eccurve(ngx_conf_t *cf, ngx_ssl_t *ssl, ngx_str_t
> *named_curve);
> ngx_int_t ngx_ssl_session_cache(ngx_ssl_t *ssl, ngx_str_t *sess_ctx,
> ssize_t builtin_session_cache, ngx_shm_zone_t *shm_zone, time_t
> timeout);
> ngx_int_t ngx_ssl_create_connection(ngx_ssl_t *ssl, ngx_connection_t
> *c,
> diff -rupN nginx-0.8.54/src/http/modules/ngx_http_ssl_module.c
> nginx-0.8.54p/src/http/modules/ngx_http_ssl_module.c
> --- nginx-0.8.54/src/http/modules/ngx_http_ssl_module.c 2010-05-14
> 12:56:37.000000000 +0300
> +++ nginx-0.8.54p/src/http/modules/ngx_http_ssl_module.c 2011-01-04
> 16:06:10.000000000 +0200
> @@ -77,7 +77,14 @@ static ngx_command_t ngx_http_ssl_comma
> NGX_HTTP_SRV_CONF_OFFSET,
> offsetof(ngx_http_ssl_srv_conf_t, dhparam),
> NULL },
> -
> +

Unrelated whitespace change.

> + { ngx_string("ssl_eccurve"),
> + NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_CONF_TAKE1,
> + ngx_conf_set_str_slot,
> + NGX_HTTP_SRV_CONF_OFFSET,
> + offsetof(ngx_http_ssl_srv_conf_t, eccurve),
> + NULL },
> +

Trailing spaces.

> { ngx_string("ssl_protocols"),
> NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_CONF_1MORE,
> ngx_conf_set_bitmask_slot,
> @@ -312,6 +319,7 @@ ngx_http_ssl_create_srv_conf(ngx_conf_t
> * sscf->certificate = { 0, NULL };
> * sscf->certificate_key = { 0, NULL };
> * sscf->dhparam = { 0, NULL };
> + * sscf->eccurve = { 0, NULL };
> * sscf->client_certificate = { 0, NULL };
> * sscf->crl = { 0, NULL };
> * sscf->ciphers = { 0, NULL };
> @@ -356,6 +364,8 @@ ngx_http_ssl_merge_srv_conf(ngx_conf_t *
>
> ngx_conf_merge_str_value(conf->dhparam, prev->dhparam, "");
>
> + ngx_conf_merge_str_value(conf->eccurve, prev->eccurve, "");
> +
> ngx_conf_merge_str_value(conf->client_certificate,
> prev->client_certificate,
> "");
> ngx_conf_merge_str_value(conf->crl, prev->crl, "");
> @@ -473,6 +483,10 @@ ngx_http_ssl_merge_srv_conf(ngx_conf_t *
> return NGX_CONF_ERROR;
> }
>
> + if (ngx_ssl_eccurve(cf, &conf->ssl, &conf->eccurve) != NGX_OK) {
> + return NGX_CONF_ERROR;
> + }
> +
> ngx_conf_merge_value(conf->builtin_session_cache,
> prev->builtin_session_cache,
> NGX_SSL_NONE_SCACHE);
>
> diff -rupN nginx-0.8.54/src/http/modules/ngx_http_ssl_module.h
> nginx-0.8.54p/src/http/modules/ngx_http_ssl_module.h
> --- nginx-0.8.54/src/http/modules/ngx_http_ssl_module.h 2009-07-23
> 15:21:26.000000000 +0300
> +++ nginx-0.8.54p/src/http/modules/ngx_http_ssl_module.h 2011-01-04
> 15:41:00.000000000 +0200
> @@ -32,6 +32,7 @@ typedef struct {
> ngx_str_t certificate;
> ngx_str_t certificate_key;
> ngx_str_t dhparam;
> + ngx_str_t eccurve;
> ngx_str_t client_certificate;
> ngx_str_t crl;
>
> diff -rupN nginx-0.8.54/src/mail/ngx_mail_ssl_module.c
> nginx-0.8.54p/src/mail/ngx_mail_ssl_module.c
> --- nginx-0.8.54/src/mail/ngx_mail_ssl_module.c 2010-05-14
> 12:56:37.000000000 +0300
> +++ nginx-0.8.54p/src/mail/ngx_mail_ssl_module.c 2011-01-04
> 14:55:00.000000000 +0200
> @@ -77,6 +77,13 @@ static ngx_command_t ngx_mail_ssl_comma
> offsetof(ngx_mail_ssl_conf_t, dhparam),
> NULL },
>
> + { ngx_string("ssl_eccurve"),
> + NGX_MAIL_MAIN_CONF|NGX_MAIL_SRV_CONF|NGX_CONF_TAKE1,
> + ngx_conf_set_str_slot,
> + NGX_MAIL_SRV_CONF_OFFSET,
> + offsetof(ngx_mail_ssl_conf_t, eccurve),
> + NULL },
> +

Trailing spaces.

> { ngx_string("ssl_protocols"),
> NGX_MAIL_MAIN_CONF|NGX_MAIL_SRV_CONF|NGX_CONF_1MORE,
> ngx_conf_set_bitmask_slot,
> @@ -163,6 +170,7 @@ ngx_mail_ssl_create_conf(ngx_conf_t *cf)
> * scf->certificate = { 0, NULL };
> * scf->certificate_key = { 0, NULL };
> * scf->dhparam = { 0, NULL };
> + * scf->eccurve = { 0, NULL };
> * scf->ciphers = { 0, NULL };
> * scf->shm_zone = NULL;
> */
> @@ -204,6 +212,8 @@ ngx_mail_ssl_merge_conf(ngx_conf_t *cf,
>
> ngx_conf_merge_str_value(conf->dhparam, prev->dhparam, "");
>
> + ngx_conf_merge_str_value(conf->eccurve, prev->eccurve, "");
> +
> ngx_conf_merge_str_value(conf->ciphers, prev->ciphers,
> NGX_DEFAULT_CIPHERS);
>
>
> @@ -294,6 +304,10 @@ ngx_mail_ssl_merge_conf(ngx_conf_t *cf,
> return NGX_CONF_ERROR;
> }
>
> + if (ngx_ssl_eccurve(cf, &conf->ssl, &conf->eccurve) != NGX_OK) {
> + return NGX_CONF_ERROR;
> + }
> +
> ngx_conf_merge_value(conf->builtin_session_cache,
> prev->builtin_session_cache,
> NGX_SSL_NONE_SCACHE);
>
> diff -rupN nginx-0.8.54/src/mail/ngx_mail_ssl_module.h
> nginx-0.8.54p/src/mail/ngx_mail_ssl_module.h
> --- nginx-0.8.54/src/mail/ngx_mail_ssl_module.h 2008-09-01
> 17:19:01.000000000 +0300
> +++ nginx-0.8.54p/src/mail/ngx_mail_ssl_module.h 2011-01-04
> 14:49:00.000000000 +0200
> @@ -34,6 +34,7 @@ typedef struct {
> ngx_str_t certificate;
> ngx_str_t certificate_key;
> ngx_str_t dhparam;
> + ngx_str_t eccurve;
>
> ngx_str_t ciphers;
>

Otherwise looks good, thank you for your patch. Please resubmit
with fixes to nginx-devel@.

Maxim Dounin

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