Hello,

Thanks for the review, and the cleanup.

2014-01-28 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)

>
> [...]
>
> > +#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, "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.
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel