[PATCH] Add proxy_protocol option to mail listener

[Maxim Dounin]

Hello!

On Tue, Jul 18, 2017 at 12:06:09PM +0200, Kees Bos wrote:

> # HG changeset patch
> # User Kees Bos <cornelis.bos at gmail.com>
> # Date 1500371531 0
> #      Tue Jul 18 09:52:11 2017 +0000
> # Node ID 8dd6050ca6858d9bea139067611ca5c69cfe8f18
> # Parent  e3723f2a11b7ec1c196d59c331739bc21d9d9afd
> Add proxy_protocol option to mail listener
> 
> Add support for the mail handlers. This enables the use of an upstream
> loadbalancer/proxy (like haproxy) that connects with the proxy protocol.
> 
> The original ip (as exposed with the proxy protocol) will be used as
> parameter for the 'Client-IP' in the authentication call and as address
> in the XCLIENT call.
> 
> Optionally (if set), the real ips from the client that are using the
> proxy protocol can be restricted with "set_real_ip_from".

This approach looks unsafe and counter-intuitive.

Instead, address should be changed if and only if there is 
set_real_ip_from and it lists a particular client address, much 
like it is done in http and stream modules.

> 
> Example config:
> mail {
>     server_name mail.example.com;
>     auth_http   localhost:9000/;
> 
>     server {
>         listen 143 proxy_protocol;
>         protocol imap;
>     }

That is, only parsing of PROXY protocol header should happen here.

> 
>     server {
>         listen 25 proxy_protocol;
>         protocol smtp;
>         set_real_ip_from 127.0.0.0/8;
>         set_real_ip_from ::/128;

And here we can change client's address if a connection was from 
listed addresses.

We may also consider sending the information from the header in 
separate auth_http headers (something like Proxy-Protocol-IP, 
Proxy-Protocol-Port?) regardless of set_real_ip_from.  But clearly 
this should be a separate header from Client-IP to make it 
possible for auth_http script to decide if this information should 
be trusted or not.

(There are also multiple style issues in the code.  Some are 
outlined below, though I haven't focused on this as the code logic 
is to be changed anyway.  Most of the comments apply to more than 
one place.)

[...]

> --- a/src/mail/ngx_mail_auth_http_module.c      Mon Jul 17 17:23:51 2017 +0300
> +++ b/src/mail/ngx_mail_auth_http_module.c      Tue Jul 18 09:52:11 2017 +0000
> @@ -1142,6 +1142,7 @@
>      ngx_mail_ssl_conf_t       *sslcf;
>  #endif
>      ngx_mail_core_srv_conf_t  *cscf;
> +    ngx_str_t                 *client_addr;

Style: variables should be sorted from shortest type to longest.  
Moreover, there are other ngx_str_t variables, so it should be 
added to the already existing list instead.

> --- a/src/mail/ngx_mail_handler.c       Mon Jul 17 17:23:51 2017 +0300
> +++ b/src/mail/ngx_mail_handler.c       Tue Jul 18 09:52:11 2017 +0000
> @@ -12,13 +12,14 @@
>  
>  
>  static void ngx_mail_init_session(ngx_connection_t *c);
> -
> +static void ngx_mail_proxy_protocol_handler(ngx_event_t *rev);
>  #if (NGX_MAIL_SSL)

The whitespace change looks wrong.  SSL-related functions are 
listed separately for a reason.

[...]

> -    ngx_log_error(NGX_LOG_INFO, c->log, 0, "*%uA client %*s connected to %V",
> -                  c->number, len, text, s->addr_text);

Removing a logging right after a connection is established might 
be a bad idea.  Instead, it might be a better option to introduce 
additional logging if / when the address is changed.

> +    if (s->proxy_protocol) {
> +        c->log->action = "reading PROXY protocol";
> +        ngx_add_timer(c->read, cscf->timeout);
> +        c->read->handler = ngx_mail_proxy_protocol_handler;
> +        if (ngx_handle_read_event(c->read, 0) != NGX_OK) {
> +            ngx_mail_close_connection(c);
> +        }
> +        return;
> +    }

Style: this clearly needs more empty lines.  You may want to take 
a look at the relevant code at the src/stream/ngx_stream_handler.c 
for an example.

[...]

> +    ngx_log_error(NGX_LOG_INFO, c->log, 0,
> +        "*%uA client %*s (real %*s:%d) connected to %V",
> +        c->number, len, text,
> +        c->proxy_protocol_addr.len, c->proxy_protocol_addr.data,
> +        c->proxy_protocol_port, s->addr_text);

Style: indentation is wrong.  Instead, continuation lines should 
be aligned to the first function argument instead.

[...]

> +static void *ngx_mail_realip_create_srv_conf(ngx_conf_t *cf);
> +static char *ngx_mail_realip_merge_srv_conf(ngx_conf_t *cf, void *parent,
> +    void *child);
> +//static ngx_int_t ngx_mail_realip_init(ngx_conf_t *cf);

No C99-style comments please.

[...]

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