[PATCH] SSL support for the mail proxy module

[Maxim Dounin]

Hello!

On Mon, Sep 08, 2014 at 10:19:17AM -0700, Quanah Gibson-Mount wrote:

> --On Friday, August 22, 2014 5:13 PM -0500 Kunal Pariani
> <kpariani at zimbra.com> wrote:
> 
> >
> >
> >Any comments on this yet ?
> 
> Any nginx developers who could comment on this?

Some obvious problems with the patch:

- it's corrupted by author's mail client, and hence can't be 
  applied/tested;

- there are various style violations, like C++-style comments;

- it introduces yet another "SSL without certificate verification" 
  case, which is believed to be bad (similar thing was recently 
  resolved by introducing proxy_ssl_verify in the http proxy 
  module);

Some more comments below.

[...]

> >+    // don't support SSLv2 anymore
> >
> >+    if (ngx_ssl_create(pcf->ssl, NGX_SSL_SSLv3|NGX_SSL_TLSv1, NULL)
> >
> >+            != NGX_OK) {

It is incorrect to support SSLv3 and TLSv1 only.  By default 
NGX_SSL_TLSv1_1 and NGX_SSL_TLSv1_2 should be allowed, too.  It's 
also may be a good idea to make this configurable like in http 
proxy module.

Also, 2 style issues here: "//" comment and incorrectly placed 
"{".

[...]

> >+    if (ngx_ssl_create_connection(pcf->ssl, c,
> >
> >+                                  NGX_SSL_BUFFER|NGX_SSL_CLIENT)
> >
> >+        != NGX_OK)
> >
> >+    {

The NGX_SSL_BUFFER is incorrect here.  It won't currently make any 
difference as the code only uses c->recv() / c->send(), but 
nevertheless mail protocols doesn't assume buffering, and the mail 
module doesn't use NGX_SSL_BUFFER in ngx_ssl_create_connection() 
calls intentionally. 

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