Hello!
On Tue, Dec 01, 2015 at 08:08:30AM +0900, junpei yoshino wrote:
> # HG changeset patch
> # User Junpei Yoshino <junpei.yosh...@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@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel
