[PATCH] mail_{ssl, auth_http}_module: add support for SSL client certificates

Sven Peter sven at ha.cki.ng
Tue Jan 14 13:41:15 UTC 2014


Hi,

Thanks for the feedback!

On Jan 14, 2014, at 1:08 PM, Maxim Dounin <mdounin at 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 at nginx.org
> http://mailman.nginx.org/mailman/listinfo/nginx-devel


Cheers,


Sven


More information about the nginx-devel mailing list