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/

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