[PATCH] Mail: added support for SSL client certificate

[Maxim Dounin]

Hello!

On Sat, Jan 25, 2014 at 09:47:09AM +0100, Filipe da Silva wrote:

> # HG changeset patch
> # User Franck Levionnois <flevionnois at gmail.com>
> # Date 1390577176 -3600
> #      Fri Jan 24 16:26:16 2014 +0100
> # Node ID 9dc48eeb8e5cb022676dbbe56e3435d20e822ab3
> # Parent  a387ce36744aa36b50e8171dbf01ef716748327e
> Mail: added support for SSL client certificate.
> 
> Add support for SSL Mutual Authentification like in HTTP module.
> 
> Added mail configuration directives (like http):
> ssl_verify_client, ssl_verify_depth,  ssl_client_certificate, ssl_trusted_certificate, ssl_crl
> 
> Added headers:
> Auth-Certificate, Auth-Certificate-Verify, Auth-Issuer-DN, Auth-Subject-DN, Auth-Subject-Serial

Please limit commit logs line lengths to 80 chars, much like in 
the code.

> 
> diff -r a387ce36744a -r 9dc48eeb8e5c src/mail/ngx_mail_auth_http_module.c
> --- a/src/mail/ngx_mail_auth_http_module.c	Thu Jan 23 22:09:59 2014 +0900
> +++ b/src/mail/ngx_mail_auth_http_module.c	Fri Jan 24 16:26:16 2014 +0100
> @@ -1135,6 +1135,35 @@ ngx_mail_auth_http_dummy_handler(ngx_eve
>                     "mail auth http dummy handler");
>  }
>  
> +#if (NGX_MAIL_SSL)
> +
> +static ngx_int_t

After Filipe's cleanup it looks much better than what was 
originally submitted by Franck (thanks Filipe!), but there are 
still lots of style problems.

> +ngx_ssl_get_certificate_oneline(ngx_connection_t *c, ngx_pool_t *pool,
> +                                ngx_str_t *b64_cert)
> +{
> +    ngx_str_t   pem_cert;
> +    if (ngx_ssl_get_raw_certificate(c, pool, &pem_cert) != NGX_OK) {
> +        return NGX_ERROR;
> +    }
> +
> +    if (pem_cert.len == 0) {
> +        b64_cert->len = 0;
> +        return NGX_OK;
> +    }
> +
> +    b64_cert->len = ngx_base64_encoded_length(pem_cert.len);
> +    b64_cert->data = ngx_palloc(pool, b64_cert->len);
> +    if (b64_cert->data == NULL) {
> +        b64_cert->len = 0;
> +        return NGX_ERROR;
> +    }
> +    ngx_encode_base64(b64_cert, &pem_cert);

Using a raw certificate escaped as other other Auth-* headers may 
be a better idea than inventing another method to pass things.  
Base64 encoding of base64 encoded data looks especially strange.  
:)

[...]

> +#if (NGX_MAIL_SSL)
> +    if (s->connection->ssl) {
> +        if (ngx_ssl_get_client_verify(s->connection, pool,
> +                                      &client_verify) != NGX_OK) {
> +            return NULL;
> +        }

The "client_" prefix looks unneeded.

> +
> +        if (ngx_ssl_get_subject_dn(s->connection, pool,
> +                                   &client_subject) != NGX_OK) {
> +            return NULL;
> +        }
> +
> +        if (ngx_ssl_get_issuer_dn(s->connection, pool,
> +                                  &client_issuer) != NGX_OK) {
> +            return NULL;
> +        }
> +
> +        if (ngx_ssl_get_serial_number(s->connection, pool,
> +                                      &client_serial) != NGX_OK) {
> +            return NULL;
> +        }

One of questions left open during Sven Peter's patch review was 
whether subject/issuer can contain CR/LF and require escaping.  
The code here suggests they can't.  I would like to know if it was 
actually checked.

It would be also cool to get Sven's review of the code (and/or his 
own patch improved instead if he don't happy with one from 
Franck).  Added Sven to Cc.

[...]

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