Hello,

I've just seen that my previous mail was sent with wrong mail address, and without mail copies.
I answer on the thread with the good one, and add some clarifications.


2014-01-28 15:18 GMT+01:00 Maxim Dounin <mdounin@mdounin.ru>:
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.
:)

Base64 encoding of the PEM certificate may looks strange, but it is done for compatibility with other reverse proxy like F5 BigIp. It is also possible to simply remove PEM header / footer and carriage returns (like another reverse proxy) 

The function "ngx_ssl_get_certificate" is about to do the work, but it let headers, and replaces carriage returns by tabulations. Modify this one to remove the headers may have some consequences.
Although i would have preferred not to have the headers, i think i can do with it, if you think this is better than adding a third function to get ssl client certificate.


[...]

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

 
Subject and Issuer DN may contains special chars but "X509_NAME_oneline" function escapes every chars outside " " -> "~" range (in ASCII table).
This is the function used by "ngx_ssl_get_subject_dn" and "ngx_ssl_get_issuer_dn" to get the DN
This is a sample output from the function of DN with carriage returns :
Issuer: /C=FR/ST=Some-State \x0D\x0A\x0D\x0A\x0D\x0Atest/
L=Paris/OU=An\x0D\x0Aign/CN=Autorite de certification

Even if i've never seen Distinguished names with carriage returns, i haven't seen such limitation in RFC 3280 / X500.
RFC 2253 shows a sample of distinguished name with carriage return.
 
[...]

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

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

Kind regards.
Franck.