Hello!
On Tue, Jul 18, 2017 at 12:06:09PM +0200, Kees Bos wrote:
> # HG changeset patch
> # User Kees Bos <cornelis....@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/
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel
