<div dir="ltr"><div>Hello, <br><br></div>I've just seen that my previous mail was sent with wrong mail address, and without mail copies. <br><div><div><div class="gmail_extra">I answer on the thread with the good one, and add some clarifications.<br>
<br>
</div><div class="gmail_extra"><br><div class="gmail_quote">2014-01-28 15:18 GMT+01:00 Maxim Dounin <span dir="ltr"><<a href="mailto:mdounin@mdounin.ru" target="_blank">mdounin@mdounin.ru</a>></span>:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Hello!<br>
<div><br>
On Sat, Jan 25, 2014 at 09:47:09AM +0100, Filipe da Silva wrote:<br>
<br>
> # HG changeset patch<br>
> # User Franck Levionnois <flevionnois at <a href="http://gmail.com" target="_blank">gmail.com</a>><br>
> # Date 1390577176 -3600<br>
> # Fri Jan 24 16:26:16 2014 +0100<br>
> # Node ID 9dc48eeb8e5cb022676dbbe56e3435d20e822ab3<br>
> # Parent a387ce36744aa36b50e8171dbf01ef716748327e<br>
> Mail: added support for SSL client certificate.<br>
><br>
> Add support for SSL Mutual Authentification like in HTTP module.<br>
><br>
> Added mail configuration directives (like http):<br>
> ssl_verify_client, ssl_verify_depth, ssl_client_certificate, ssl_trusted_certificate, ssl_crl<br>
><br>
> Added headers:<br>
> Auth-Certificate, Auth-Certificate-Verify, Auth-Issuer-DN, Auth-Subject-DN, Auth-Subject-Serial<br>
<br>
</div>Please limit commit logs line lengths to 80 chars, much like in<br>
the code.<br>
<div><br>
><br>
> diff -r a387ce36744a -r 9dc48eeb8e5c src/mail/ngx_mail_auth_http_module.c<br>
> --- a/src/mail/ngx_mail_auth_http_module.c Thu Jan 23 22:09:59 2014 +0900<br>
> +++ b/src/mail/ngx_mail_auth_http_module.c Fri Jan 24 16:26:16 2014 +0100<br>
> @@ -1135,6 +1135,35 @@ ngx_mail_auth_http_dummy_handler(ngx_eve<br>
> "mail auth http dummy handler");<br>
> }<br>
><br>
> +#if (NGX_MAIL_SSL)<br>
> +<br>
> +static ngx_int_t<br>
<br>
</div>After Filipe's cleanup it looks much better than what was<br>
originally submitted by Franck (thanks Filipe!), but there are<br>
still lots of style problems.<br>
<div><br>
> +ngx_ssl_get_certificate_oneline(ngx_connection_t *c, ngx_pool_t *pool,<br>
> + ngx_str_t *b64_cert)<br>
> +{<br>
> + ngx_str_t pem_cert;<br>
> + if (ngx_ssl_get_raw_certificate(c, pool, &pem_cert) != NGX_OK) {<br>
> + return NGX_ERROR;<br>
> + }<br>
> +<br>
> + if (pem_cert.len == 0) {<br>
> + b64_cert->len = 0;<br>
> + return NGX_OK;<br>
> + }<br>
> +<br>
> + b64_cert->len = ngx_base64_encoded_length(pem_cert.len);<br>
> + b64_cert->data = ngx_palloc(pool, b64_cert->len);<br>
> + if (b64_cert->data == NULL) {<br>
> + b64_cert->len = 0;<br>
> + return NGX_ERROR;<br>
> + }<br>
> + ngx_encode_base64(b64_cert, &pem_cert);<br>
<br>
</div>Using a raw certificate escaped as other other Auth-* headers may<br>
be a better idea than inventing another method to pass things.<br>
Base64 encoding of base64 encoded data looks especially strange.<br>
:)<br></blockquote><div><br>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) <br></div><div><br>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.<br>
<span id="result_box" class="" lang="en"><span class="">Although i would have</span> <span class="">preferred not to have</span> <span class="">the</span> <span class="">header</span><span>s, i</span> <span class="">think i can</span> <span class="">do with it,</span> <span class="">if you</span> <span class="">think this is</span> <span class="">better</span> <span class="">than adding a</span> <span class="">third</span> <span class="">function</span> to get <span class="">ssl client certificate.</span></span><br>
<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
[...]<br>
<div><br>
> +#if (NGX_MAIL_SSL)<br>
> + if (s->connection->ssl) {<br>
> + if (ngx_ssl_get_client_verify(s->connection, pool,<br>
> + &client_verify) != NGX_OK) {<br>
> + return NULL;<br>
> + }<br>
<br>
</div>The "client_" prefix looks unneeded.<br>
<div><br>
> +<br>
> + if (ngx_ssl_get_subject_dn(s->connection, pool,<br>
> + &client_subject) != NGX_OK) {<br>
> + return NULL;<br>
> + }<br>
> +<br>
> + if (ngx_ssl_get_issuer_dn(s->connection, pool,<br>
> + &client_issuer) != NGX_OK) {<br>
> + return NULL;<br>
> + }<br>
> +<br>
> + if (ngx_ssl_get_serial_number(s->connection, pool,<br>
> + &client_serial) != NGX_OK) {<br>
> + return NULL;<br>
> + }<br>
<br>
</div>One of questions left open during Sven Peter's patch review was<br>
whether subject/issuer can contain CR/LF and require escaping.<br>
The code here suggests they can't. I would like to know if it was<br>
actually checked.<br>
<br>
It would be also cool to get Sven's review of the code (and/or his<br>
own patch improved instead if he don't happy with one from<br>
Franck). Added Sven to Cc.<br>
<br></blockquote><div> </div><div>Subject and Issuer DN may contains special chars but "X509_NAME_oneline" function escapes every chars outside " " -> "~"
range (in ASCII table).<br>This is the function used by "ngx_ssl_get_subject_dn" and "ngx_ssl_get_issuer_dn" to get the DN<br>This is a sample output from the function of DN with carriage returns :<br>
Issuer: /C=FR/ST=Some-State \x0D\x0A\x0D\x0A\x0D\x0Atest/<div>L=Paris/OU=An\x0D\x0Aign/CN=Autorite de certification<br><br>Even if i've never seen Distinguished names with carriage returns, i haven't seen such limitation in RFC 3280 / X500.<br>
RFC 2253 shows a sample of distinguished name with carriage return. </div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
[...]<br>
<span><font color="#888888"><br>
--<br>
Maxim Dounin<br>
<a href="http://nginx.org/" target="_blank">http://nginx.org/</a><br>
<br>
_______________________________________________<br>
nginx-devel mailing list<br>
<a href="mailto:nginx-devel@nginx.org" target="_blank">nginx-devel@nginx.org</a><br>
<a href="http://mailman.nginx.org/mailman/listinfo/nginx-devel" target="_blank">http://mailman.nginx.org/mailman/listinfo/nginx-devel</a><br>
</font></span></blockquote></div><br>Kind regards.<br></div><div class="gmail_extra">Franck.<br></div></div></div></div>