Hi,

Thanks for the feedback!

On Jan 14, 2014, at 1:08 PM, Maxim Dounin <mdounin@mdounin.ru> wrote:
>
> Better summary line would be:
>
> Mail: added support for SSL client certificate.

Agreed.

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

Agreed, I'll fix that.

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


How does this diverge from the style exactly? Because of the #ifdef?
Are the variable names acceptable when then the ssl_client prefix is removed?
Or is the problem that there are client_raw_ and client_ variables?

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

Yup, I'll remove it.

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

Yes, it's unneeded. I'll remove it.

>
>> + } 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".

Yup.

>
>> + }
>> +#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.

That indeed sounds like 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.
Agreed.

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

I'll convert the verify field to look just like it does in http then.

>
>> + 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.
Yup, I must've overlooked that line when fixing my 80+ ones.

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

I've adapted this code from the http module (which exposes the raw certificate) without
thinking about the use cases.

This leaves two options imho:

1) Remove the optional_no_ca entirely and let someone else submit another patch if required :-)
2) Add an additional HTTP header for the raw certificate

I'd go for the first one. Comments?

>
>> + { 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.
Ah, yeah. Thanks.

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

Possibly, I'll take a closer look at ngx_ssl_* and the http module again. I might've misunderstood something there.

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

I'll remove the enum and fix the style problems.

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


Cheers,


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