[PATCH]add proxy_protocol_port variable for rfc6302

[junpei yoshino]

hello

thank you for reply and background.

> but we need someone to do the rest of the work.

Could I contribute it?
At first, I will revise this patch along your review.


On Fri, Dec 4, 2015 at 4:03 AM, Maxim Dounin <mdounin at mdounin.ru> wrote:
> 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/
>
> _______________________________________________
> nginx-devel mailing list
> nginx-devel at nginx.org
> http://mailman.nginx.org/mailman/listinfo/nginx-devel



-- 
junpei.yoshino at gmail.com