[PATCH] mail_{ssl, auth_http}_module: add support for SSL client certificates

[Maxim Dounin]

Hello!

On Tue, Jan 14, 2014 at 02:41:15PM +0100, Sven Peter wrote:

[...]

> >> @@ -1145,6 +1145,16 @@
> >>     ngx_str_t                  login, passwd;
> >>     ngx_mail_core_srv_conf_t  *cscf;
> >> 
> >> +#if (NGX_MAIL_SSL)
> >> +    ngx_str_t ssl_client_verify;
> >> +    ngx_str_t ssl_client_raw_s_dn;
> >> +    ngx_str_t ssl_client_raw_i_dn;
> >> +    ngx_str_t ssl_client_raw_serial;
> >> +    ngx_str_t ssl_client_s_dn;
> >> +    ngx_str_t ssl_client_i_dn;
> >> +    ngx_str_t ssl_client_serial;
> >> +#endif
> >> +
> > 
> > This diverges from the style used.  Additionally, variable names 
> > seems to be too verbose.
> 
> 
> How does this diverge from the style exactly? Because of the #ifdef?

No, because of placement, indentation of variable names, and use 
of types.  With names unchanged, it should look like this:

    ngx_str_t                  login, passwd;
#if (NGX_MAIL_SSL)
    ngx_str_t                  ssl_client_verify, ssl_client_raw_s_dn,
                               ssl_client_raw_i_dn, ssl_client_raw_serial,
                               ssl_client_s_dn, ssl_client_i_dn,
                               ssl_client_serial;
#endif
    ngx_mail_core_srv_conf_t  *cscf;


> Are the variable names acceptable when then the ssl_client prefix is removed?
> Or is the problem that there are client_raw_ and client_ variables?

With "ssl_client" removed it looks better, but I would probably 
use something like "subject" instead of "s_dn", "issuer" instead 
of "i_dn" and so on.

[...]

> >> +        if (ngx_mail_auth_http_escape(pool, &ssl_client_raw_s_dn, 
> >> +                                      &ssl_client_s_dn) != NGX_OK) {
> >> +            return NULL;
> >> +        }
> >> +
> >> +        if (ngx_mail_auth_http_escape(pool, &ssl_client_raw_i_dn,
> >> +                                      &ssl_client_i_dn) != NGX_OK) {
> >> +            return NULL;
> >> +        }
> >> +
> >> +        if (ngx_mail_auth_http_escape(pool, &ssl_client_raw_serial,
> >> +                                      &ssl_client_serial) != NGX_OK) {
> >> +            return NULL;
> >> +        }
> > 
> > On the other hand, escaping of at least client certificate serial 
> > number looks unneeded.
> 
> Yes, it's unneeded. I'll remove it.

The question is more about whether other parts currently escaped 
in your code really need escaping.  Unless they can contatin CR/LF 
(and I think they can't, even with specially crafter certificates, 
but it needs checking) - it would be good idea to avoid escaping 
altogether.

[...]

> >> +    { ngx_string("on"), NGX_MAIL_SSL_VERIFY_ON },
> >> +    { ngx_string("optional"), NGX_MAIL_SSL_VERIFY_OPTIONAL },
> >> +    { ngx_string("optional_no_ca"), NGX_MAIL_SSL_VERIFY_OPTIONAL_NO_CA },
> > 
> > As noted above, "optional_no_ca" makes no sense without a way to 
> > pass a certificate to some backend.
> 
> I've adapted this code from the http module (which exposes the raw certificate) without
> thinking about the use cases.
> 
> This leaves two options imho:
> 
> 	1) Remove the optional_no_ca entirely and let someone else submit another patch if required :-)
> 	2) Add an additional HTTP header for the raw certificate
> 
> I'd go for the first one. Comments?

I'm ok with both variants.

[...]

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