Hello,

Thanks for the reply.

On Mon, 22 May 2023 15:48:23 +0300
Maxim Dounin <mdounin@mdounin.ru> wrote:

> Hello!
>
> On Sun, May 14, 2023 at 04:51:58AM +0100, J Carter wrote:
>
> > # HG changeset patch
> > # User jordanc.carter@outlook.com
> > # Date 1684035158 -3600
> > # Sun May 14 04:32:38 2023 +0100
> > # Node ID dad6e472ee0d97a738b117f6480987ef135c9e7f
> > # Parent b71e69247483631bd8fc79a47cc32b762625b1fb
> > Added $realip_add_x_forwarded_for
> >
> > Resolves Ticket #2127.
> >
> > Duplicates the functionality of proxy_add_x_forwarded_for, except
> > the true source ip is appended and not the remote address extracted
> > by the real IP module.
> >
> > In practice this is proxy_add_x_forwarded_for but
> > $realip_remote_addr is used and not $remote_addr.
> >
> > This follows the same convention as $realip_remote_addr and
> > $real_ip_remote_port, in that it is a drop in replacement for
> > $proxy_add_x_forwarded_for that can be used in contexts that both do
> > and do not have the real_ip directives, with the same results.
> >
> > An example configuration:
> >
> > server {
> > listen 80;
> > real_ip_header X-Forwarded-For;
> > set_real_ip_from 127.0.0.1;
> >
> > location / {
> > proxy_set_header X-Forwarded-For
> > x; proxy_set_header Remote $remote_addr;
> > proxy_pass http://127.0.0.1:8080;
> > }
> > }
>
> Thanks for the patch, but I can't say I like the idea of
> introducing yet another variable and asking users to change it
> manually.

Good point, it should be possible to merge the two.

> This is essentially equivalent to using
>
> proxy_set_header X-Forwarded-For "$http_x_forwarded_for,
> $realip_remote_addr";
>
> as the ticket suggests.
>

Well yes, but this proxy_set_header example would only be valid if
$http_x_forwarded_for always exists as a request header,
otherwise you'd have a hanging comma at the start.

It'd need a map to handle that if it were sometimes present and
sometimes not. I imagine avoiding such a map is the reason the
regular proxy_add_x_forwarded_for directive also exists.

> Also, it is an open question if $realip_remote_addr should be
> used, or X-Forwarded-For should be left unmodified if remote addr
> was set from X-Forwarded-For.

Leaving it unmodified does seem undesirable, as it means omitting a
proxy hop($realip_remote_addr) from the x-forwarded-for chain.

> The realip module instructs nginx
> to use the address as obtained from the header, and not using it
> for some purposes looks at least questionable.

I believe this would be the only valid exception to that, given that
value of $realip_add_x_forwarded_for is only ever going to be to
overwrite x-forwarded-for with proxy_set_header for the next hop proxy
or backend app to utilize. It's quite contained.

Also it does seem more sensible than the resulting x-forwarded-for
value shown in the ticket, which would look like nonsense to any
upstream consumer of that value that wishes to analyze the whole chain.

The proxy_add_x_forwarded_for's value in the ticket isn't in
the spirit of the header's purpose either, which is to preserve
addresses of the client & chain of proxies.

>
> Also, it seems incorrect to use $realip_remote_addr (or keep
> X-Forwarded-For unmodified) if remote addr was set from other
> sources, such as PROXY protocol headers.
>
> Overall, current behaviour might actually be optimal.
>
> [...]
>

This is a good point, although perhaps adding both
$remote_addr and $realip_remote_addr to the
x-forwarded-for chain would be better behavior for the other sources
(especially proxy_protocol).

It'd need some additional checks to ensure
no duplications are introduced (if the x-forwarded-for header already
exists).
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel