Hello!

On Mon, Jan 13, 2014 at 04:28:25PM +0100, Sven Peter wrote:

> # HG changeset patch
> # User Sven Peter <sven@ha.cki.ng>
> # Date 1389626052 -3600
> # Mon Jan 13 16:14:12 2014 +0100
> # Node ID a444733105e8eb96212f142533e714532a23cddf
> # Parent 4aa64f6950313311e0d322a2af1788edeb7f036c
> mail_{ssl,auth_http}_module: add support for SSL client certificates

Better summary line would be:

Mail: added support for SSL client certificate.

>
> This patch adds support for SSL client certificates to the mail proxy
> capabilities of nginx both for STARTTLS and SSL mode.
> Just like the HTTP SSL module a root CA is defined in the mail section
> of the configuration file. Verification can be optional or mandatory.
> Additionally, the result of the verification is exposed to the
> auth http backend via the SSL-Verify, SSL-Subject-DN, SSL-Issuer-DN
> and SSL-Serial HTTP headers.

It would be good idea to add a list of configuration directives
added.

>
> diff -r 4aa64f695031 -r a444733105e8 src/mail/ngx_mail_auth_http_module.c
> --- a/src/mail/ngx_mail_auth_http_module.c Sat Jan 04 03:32:22 2014 +0400
> +++ b/src/mail/ngx_mail_auth_http_module.c Mon Jan 13 16:14:12 2014 +0100
> @@ -1145,6 +1145,16 @@
> ngx_str_t login, passwd;
> ngx_mail_core_srv_conf_t *cscf;
>
> +#if (NGX_MAIL_SSL)
> + ngx_str_t ssl_client_verify;
> + ngx_str_t ssl_client_raw_s_dn;
> + ngx_str_t ssl_client_raw_i_dn;
> + ngx_str_t ssl_client_raw_serial;
> + ngx_str_t ssl_client_s_dn;
> + ngx_str_t ssl_client_i_dn;
> + ngx_str_t ssl_client_serial;
> +#endif
> +

This diverges from the style used. Additionally, variable names
seems to be too verbose.

> if (ngx_mail_auth_http_escape(pool, &s->login, &login) != NGX_OK) {
> return NULL;
> }
> @@ -1153,6 +1163,51 @@
> return NULL;
> }
>
> +#if (NGX_MAIL_SSL)
> + if (s->connection->ssl) {
> + /* ssl_client_verify comes from nginx itself - no need to escape */

This comment looks obvious.

> + if (ngx_ssl_get_client_verify(s->connection, pool,
> + &ssl_client_verify) != NGX_OK) {
> + return NULL;
> + }
> +
> + if (ngx_ssl_get_subject_dn(s->connection, pool,
> + &ssl_client_raw_s_dn) != NGX_OK) {
> + return NULL;
> + }
> +
> + if (ngx_ssl_get_issuer_dn(s->connection, pool,
> + &ssl_client_raw_i_dn) != NGX_OK) {
> + return NULL;
> + }
> +
> + if (ngx_ssl_get_serial_number(s->connection, pool,
> + &ssl_client_raw_serial) != NGX_OK) {
> + return NULL;
> + }
> +
> + if (ngx_mail_auth_http_escape(pool, &ssl_client_raw_s_dn,
> + &ssl_client_s_dn) != NGX_OK) {
> + return NULL;
> + }
> +
> + if (ngx_mail_auth_http_escape(pool, &ssl_client_raw_i_dn,
> + &ssl_client_i_dn) != NGX_OK) {
> + return NULL;
> + }
> +
> + if (ngx_mail_auth_http_escape(pool, &ssl_client_raw_serial,
> + &ssl_client_serial) != NGX_OK) {
> + return NULL;
> + }

On the other hand, escaping of at least client certificate serial
number looks unneeded.

> + } else {
> + ngx_str_set(&ssl_client_verify, "NONE");
> + ssl_client_i_dn.len = 0;
> + ssl_client_s_dn.len = 0;
> + ssl_client_serial.len = 0;

Using fake values here looks wrong. In http, nginx marks $ssl_*
variables as "not found" for non-ssl connections, which is
essentially equivalent to empty strings, i.e., this contradicts to
the use of "NONE".

> + }
> +#endif
> +
> cscf = ngx_mail_get_module_srv_conf(s, ngx_mail_core_module);
>
> len = sizeof("GET ") - 1 + ahcf->uri.len + sizeof(" HTTP/1.0" CRLF) - 1
> @@ -1173,6 +1228,16 @@
> + sizeof("Auth-SMTP-Helo: ") - 1 + s->smtp_helo.len
> + sizeof("Auth-SMTP-From: ") - 1 + s->smtp_from.len
> + sizeof("Auth-SMTP-To: ") - 1 + s->smtp_to.len
> +#if (NGX_MAIL_SSL)
> + + sizeof("SSL-Verify: ") - 1 + ssl_client_verify.len
> + + sizeof(CRLF) - 1
> + + sizeof("SSL-Subject-DN: ") - 1 + ssl_client_s_dn.len
> + + sizeof(CRLF) - 1
> + + sizeof("SSL-Issuer-DN: ") - 1 + ssl_client_i_dn.len
> + + sizeof(CRLF) - 1
> + + sizeof("SSL-Serial: ") - 1 + ssl_client_serial.len
> + + sizeof(CRLF) - 1
> +#endif

Using common prefix "Auth-" might be a good idea.

> + ahcf->header.len
> + sizeof(CRLF) - 1;
>
> @@ -1255,6 +1320,34 @@
>
> }
>
> +#if (NGX_MAIL_SSL)
> + if (ssl_client_verify.len) {
> + b->last = ngx_cpymem(b->last, "SSL-Verify: ",
> + sizeof("SSL-Verify: ") - 1);
> + b->last = ngx_copy(b->last, ssl_client_verify.data,
> + ssl_client_verify.len);
> + *b->last++ = CR; *b->last++ = LF;
> +
> + b->last = ngx_cpymem(b->last, "SSL-Subject-DN: ",
> + sizeof("SSL-Subject-DN: ") - 1);
> + b->last = ngx_copy(b->last, ssl_client_s_dn.data,
> + ssl_client_s_dn.len);
> + *b->last++ = CR; *b->last++ = LF;
> +
> + b->last = ngx_cpymem(b->last, "SSL-Issuer-DN: ",
> + sizeof("SSL-Issuer-DN: ") - 1);
> + b->last = ngx_copy(b->last, ssl_client_i_dn.data,
> + ssl_client_i_dn.len);
> + *b->last++ = CR; *b->last++ = LF;
> +
> + b->last = ngx_cpymem(b->last, "SSL-Serial: ",
> + sizeof("SSL-Serial: ") - 1);
> + b->last = ngx_copy(b->last, ssl_client_serial.data,
> + ssl_client_serial.len);
> + *b->last++ = CR; *b->last++ = LF;
> + }
> +#endif
> +

I don't think that these headers should be sent if there is no
SSL connection. Any empty headers should be probably ommitted,
too.

> if (ahcf->header.len) {
> b->last = ngx_copy(b->last, ahcf->header.data, ahcf->header.len);
> }
> diff -r 4aa64f695031 -r a444733105e8 src/mail/ngx_mail_handler.c
> --- a/src/mail/ngx_mail_handler.c Sat Jan 04 03:32:22 2014 +0400
> +++ b/src/mail/ngx_mail_handler.c Mon Jan 13 16:14:12 2014 +0100
> @@ -236,11 +236,40 @@
> {
> ngx_mail_session_t *s;
> ngx_mail_core_srv_conf_t *cscf;
> + ngx_mail_ssl_conf_t *sslcf;
>
> if (c->ssl->handshaked) {
>
> s = c->data;
>
> + sslcf = ngx_mail_get_module_srv_conf(s, ngx_mail_ssl_module);
> + if (sslcf->verify != NGX_MAIL_SSL_VERIFY_OFF) {

The use of the != check looks silly. You may want to preserve the
same code as used in http, where ->verify is more or less boolean
with some special true values to differentiate submodes when
verify is used.

> + long rc;
> + rc = SSL_get_verify_result(c->ssl->connection);
> +
> + if (rc != X509_V_OK &&
> + (sslcf->verify != NGX_MAIL_SSL_VERIFY_OPTIONAL_NO_CA && ngx_ssl_verify_error_optional(rc))) {
> + ngx_log_error(NGX_LOG_INFO, c->log, 0,
> + "client SSL certificate verify error: (%l:%s)",
> + rc, X509_verify_cert_error_string(rc));
> + ngx_mail_close_connection(c);
> + return;
> + }

Minor note: a 80+ line here due to use of long names.

Maror problem: you allow "optional_no_ca" here, but this is for
sure not secure due to no certificate passed to a backend.

> +
> + if (sslcf->verify == NGX_MAIL_SSL_VERIFY_ON) {
> + X509 *cert;
> + cert = SSL_get_peer_certificate(c->ssl->connection);
> +
> + if (cert == NULL) {
> + ngx_log_error(NGX_LOG_INFO, c->log, 0,
> + "client sent no required SSL certificate");
> + ngx_mail_close_connection(c);
> + return;
> + }
> + X509_free(cert);
> + }
> + }
> +
> if (s->starttls) {
> cscf = ngx_mail_get_module_srv_conf(s, ngx_mail_core_module);
>
> diff -r 4aa64f695031 -r a444733105e8 src/mail/ngx_mail_ssl_module.c
> --- a/src/mail/ngx_mail_ssl_module.c Sat Jan 04 03:32:22 2014 +0400
> +++ b/src/mail/ngx_mail_ssl_module.c Mon Jan 13 16:14:12 2014 +0100
> @@ -43,6 +43,13 @@
> { ngx_null_string, 0 }
> };
>
> +static ngx_conf_enum_t ngx_mail_ssl_verify[] = {
> + { ngx_string("off"), NGX_MAIL_SSL_VERIFY_OFF },
> + { ngx_string("on"), NGX_MAIL_SSL_VERIFY_ON },
> + { ngx_string("optional"), NGX_MAIL_SSL_VERIFY_OPTIONAL },
> + { ngx_string("optional_no_ca"), NGX_MAIL_SSL_VERIFY_OPTIONAL_NO_CA },

As noted above, "optional_no_ca" makes no sense without a way to
pass a certificate to some backend.

> + { ngx_null_string, 0 }
> +};
>
> static ngx_command_t ngx_mail_ssl_commands[] = {

You may note that previously there were 2 empty lines between
blocks. With your change, there is just 1 empty line.

>
> @@ -130,7 +137,40 @@
> offsetof(ngx_mail_ssl_conf_t, session_timeout),
> NULL },
>
> - ngx_null_command
> + {

The change here indicate you did something wrong with style.

> + ngx_string("ssl_verify_client"),
> + NGX_MAIL_MAIN_CONF|NGX_MAIL_SRV_CONF|NGX_CONF_TAKE1,
> + ngx_conf_set_enum_slot,
> + NGX_MAIL_SRV_CONF_OFFSET,
> + offsetof(ngx_mail_ssl_conf_t, verify),
> + &ngx_mail_ssl_verify
> + },
> + {

The style here is certainly wrong.

> + ngx_string("ssl_verify_depth"),
> + NGX_MAIL_MAIN_CONF|NGX_MAIL_SRV_CONF|NGX_CONF_1MORE,

Hm, "1MORE" is wrong here, should be "TAKE1". Fixed this in http
ssl module.

> + ngx_conf_set_num_slot,
> + NGX_MAIL_SRV_CONF_OFFSET,
> + offsetof(ngx_mail_ssl_conf_t, verify_depth),
> + NULL
> + },
> + {
> + ngx_string("ssl_client_certificate"),
> + 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, client_certificate),
> + NULL
> + },
> + {
> + ngx_string("ssl_trusted_certificate"),
> + 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, trusted_certificate),
> + NULL
> + },
> +
> + ngx_null_command
> };
>
>
> @@ -184,6 +224,8 @@
> * scf->ecdh_curve = { 0, NULL };
> * scf->ciphers = { 0, NULL };
> * scf->shm_zone = NULL;
> + * scf->client_certificate = { 0, NULL };
> + * scf->trusted_certificate = { 0, NULL };
> */
>
> scf->enable = NGX_CONF_UNSET;
> @@ -192,6 +234,8 @@
> scf->builtin_session_cache = NGX_CONF_UNSET;
> scf->session_timeout = NGX_CONF_UNSET;
> scf->session_ticket_keys = NGX_CONF_UNSET_PTR;
> + scf->verify = NGX_CONF_UNSET_UINT;
> + scf->verify_depth = NGX_CONF_UNSET_UINT;
>
> return scf;
> }
> @@ -230,6 +274,11 @@
>
> ngx_conf_merge_str_value(conf->ciphers, prev->ciphers, NGX_DEFAULT_CIPHERS);
>
> + ngx_conf_merge_uint_value(conf->verify, prev->verify, NGX_MAIL_SSL_VERIFY_OFF);
> + ngx_conf_merge_uint_value(conf->verify_depth, prev->verify_depth, 1);
> +
> + ngx_conf_merge_str_value(conf->client_certificate, prev->client_certificate, "");
> + ngx_conf_merge_str_value(conf->trusted_certificate, prev->trusted_certificate, "");
>
> conf->ssl.log = cf->log;
>
> @@ -310,6 +359,21 @@
> return NGX_CONF_ERROR;
> }
>
> + if (conf->verify) {
> + if (conf->client_certificate.len == 0 && conf->verify != NGX_MAIL_SSL_VERIFY_OPTIONAL_NO_CA) {
> + ngx_log_error(NGX_LOG_EMERG, cf->log, 0,
> + "no ssl_client_certificate for ssl_client_verify");
> + return NGX_CONF_ERROR;
> + }
> +
> + if (ngx_ssl_client_certificate(cf, &conf->ssl,
> + &conf->client_certificate,
> + conf->verify_depth)
> + != NGX_OK) {
> + return NGX_CONF_ERROR;
> + }
> + }
> +

This code looks incomplete (and there are style issues). E.g., it
doesn't looks like trusted certificates are loaded at all.

It also lacks ssl_crl support, which is also directly related to
client certificates.

> if (conf->prefer_server_ciphers) {
> SSL_CTX_set_options(conf->ssl.ctx, SSL_OP_CIPHER_SERVER_PREFERENCE);
> }
> diff -r 4aa64f695031 -r a444733105e8 src/mail/ngx_mail_ssl_module.h
> --- a/src/mail/ngx_mail_ssl_module.h Sat Jan 04 03:32:22 2014 +0400
> +++ b/src/mail/ngx_mail_ssl_module.h Mon Jan 13 16:14:12 2014 +0100
> @@ -37,8 +37,14 @@
> ngx_str_t dhparam;
> ngx_str_t ecdh_curve;
>
> + ngx_str_t client_certificate;
> + ngx_str_t trusted_certificate;
> +
> ngx_str_t ciphers;
>
> + ngx_uint_t verify;
> + ngx_uint_t verify_depth;
> +
> ngx_shm_zone_t *shm_zone;
>
> ngx_array_t *session_ticket_keys;
> @@ -47,6 +53,13 @@
> ngx_uint_t line;
> } ngx_mail_ssl_conf_t;
>
> +enum ngx_mail_ssl_verify_enum {
> + NGX_MAIL_SSL_VERIFY_OFF = 0,
> + NGX_MAIL_SSL_VERIFY_ON,
> + NGX_MAIL_SSL_VERIFY_OPTIONAL,
> + NGX_MAIL_SSL_VERIFY_OPTIONAL_NO_CA,
> +};
> +
>
> extern ngx_module_t ngx_mail_ssl_module;

We usually avoid using enum types for enumerated configuration
slots. While questionable, it's currently mostly style.

There are also other style problems here - indentation is wrong,
as well as number of empty lines.

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

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