[PATCH]add proxy_protocol_port variable for rfc6302

[Maxim Dounin]

Hello!

On Tue, Dec 01, 2015 at 08:08:30AM +0900, junpei yoshino wrote:

> # HG changeset patch
> # User Junpei Yoshino <junpei.yoshino at gmail.com>
> # Date 1446723407 -32400
> #      Thu Nov 05 20:36:47 2015 +0900
> # Node ID 59cadccedf402ec325b078cb72a284465639e0fe
> # Parent  4ccb37b04454dec6afb9476d085c06aea00adaa0
> Http: add proxy_protocol_port variable for rfc6302
> 
> Logging source port is recommended in rfc6302.
> use case
>  logging
>  sending information by http request headers

Proper source port logging is something nginx don't currently do 
well in various places related to addresses got from external 
sources, including realip module, X-Forwarded-For parsing, 
[ha]proxy protocol and so on.

Improving port handling in various related areas is something that 
should be done, though it needs to be handled more or less 
consistently in all affected areas.  Providing ports in some 
places but not others can be misleading for users.  And that's why 
the patch isn't yet reviewed - sorry for the delay, but we need 
someone to do the rest of the work.

Below are some comments about the patch itself.

> diff -r 4ccb37b04454 -r 59cadccedf40 src/core/ngx_connection.h
> --- a/src/core/ngx_connection.h Fri Oct 30 21:43:30 2015 +0300
> +++ b/src/core/ngx_connection.h Thu Nov 05 20:36:47 2015 +0900
> @@ -146,6 +146,7 @@
>      ngx_str_t           addr_text;
> 
>      ngx_str_t           proxy_protocol_addr;
> +    ngx_str_t           proxy_protocol_port;

Using a string (which takes 2 pointers) for a 16 bit port value 
seems to be excessive.  It should be possible to just store a 
number instead.

[...]

> @@ -71,8 +71,56 @@
>      ngx_memcpy(c->proxy_protocol_addr.data, addr, len);
>      c->proxy_protocol_addr.len = len;
> 
> +    for ( ;; ) {
> +        if (p == last) {
> +            goto invalid;
> +        }
> +
> +        ch = *p++;
> +
> +        if (ch == ' ') {
> +            break;
> +        }
> +
> +        if (ch != ':' && ch != '.'
> +            && (ch < 'a' || ch > 'f')
> +            && (ch < 'A' || ch > 'F')
> +            && (ch < '0' || ch > '9'))
> +        {
> +            goto invalid;
> +        }
> +    }

This is probably excessive.  Just using a space as a separator 
should be enough, as we aren't using the destination address.

[...]

>      ngx_log_debug1(NGX_LOG_DEBUG_CORE, c->log, 0,
>                     "PROXY protocol address: \"%V\"",
> &c->proxy_protocol_addr);
> +    ngx_log_debug1(NGX_LOG_DEBUG_CORE, c->log, 0,
> +                   "PROXY protocol port: \"%V\"", &c->proxy_protocol_port);

Logging the address and the port at once should be enough.

[...]

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