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;
_______________________________________________
squid-dev mailing list
squid-dev@lists.squid-cache.org
https://lists.squid-cache.org/listinfo/squid-dev
