[PATCH] Added $realip_add_x_forwarded_for

[J Carter]

Hello,

Thanks for the reply.

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

> Hello!
> 
> On Sun, May 14, 2023 at 04:51:58AM +0100, J Carter wrote:
> 
> > # HG changeset patch
> > # User jordanc.carter at 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).