[squid-dev] RFC: make FOLLOW_X_FORWARDED_FOR unconditional

[Alex Rousskov]

On 2023-10-10 12:17, Francesco Chemolli wrote:

> what if we removed the configure option for FOLLOW_X_FORWARDED_FOR, and
> made it unconditionally part of Squid?

Some Squid deployments will silently break AFAICT.


> It is on by default,

Here, "it" should be viewed as a combination of FOLLOW_X_FORWARDED_FOR 
and the directives that macro enables (by default). Unfortunately, some 
of those directives are on by default[1]. This implies that, after we 
remove FOLLOW_X_FORWARDED_FOR, a Squid built with 
--disable-follow-x-forwarded-for will start doing something it was not 
doing before, and the behavior changes should be assumed to be 
significant until you can prove otherwise.


> and it is controlled by runtime configuration,

... but not in a good way: Squid code cannot tell whether 
follow_x_forwarded_for is "enabled" beyond checking 
FOLLOW_X_FORWARDED_FOR. It is possible to change that, but changing that 
(and adjusting how other related directives are checked!) requires 
non-trivial work. See (B) below.


> removing the compile-time option would ensure we have better testing 
> coverage.

Indeed. I support removal of this and similar unnecessary compile-time 
options, but because related directives[1] are _on_ by default (in 
--enable-follow-x-forwarded-for and equivalent builds), we should not 
just enable this feature, screwing up folks that disabled it explicitly 
in their builds.

Also, the current implementation of XFF code is fairly expensive -- it 
requires slow ACL checks. If we just remove that FOLLOW_X_FORWARDED_FOR 
guard, we will be adding quite a few wasted CPU cycles to all Squid 
builds that (used to) disable that feature.


[1] squid.conf directives that are enabled by default in 
--enable-follow-x-forwarded-for and equivalent builds include: 
log_uses_indirect_client, adaptation_uses_indirect_client, 
acl_uses_indirect_client, delay_pool_uses_indirect_client.


I see a few possible strategies here:

A) Make ./configure --disable-follow-x-forwarded-for fail. Easy to 
implement but kind of goes against ./configure tradition and leaves a 
backward compatibility hack in Squid code. It also forces admins that do 
_not_ want this feature to disable 4+ directives they do not care about 
-- bad UX. I am against this option.

B) Make all *_uses_indirect_client and similar directives default to off 
unless the configuration contains an explicit follow_x_forwarded_for 
rule. Warn if one of those directives is explicitly on when there are no 
explicit follow_x_forwarded_for rules. This requires non-trivial work. 
That work will probably fix a few existing bugs. I support this option.


HTH,

Alex.


> AccessLogEntry.cc:#if FOLLOW_X_FORWARDED_FOR
> AccessLogEntry.cc-    if (Config.onoff.log_uses_indirect_client && request)
> AccessLogEntry.cc-        log_ip = request->indirect_client_addr;
> AccessLogEntry.cc-    else
> AccessLogEntry.cc-#endif
> AccessLogEntry.cc-        if (tcpClient)
> AccessLogEntry.cc-            log_ip = tcpClient->remote;
> AccessLogEntry.cc-        else
> AccessLogEntry.cc-            log_ip = cache.caddr;