On 11/10/23 08:19, Alex Rousskov wrote:
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.
In what way specifically?
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.
The Proof is in cf.data.pre:
"
NAME: follow_x_forwarded_for
...
DEFAULT_IF_NONE: deny all
DEFAULT_DOC: X-Forwarded-For header will be ignored.
"
Removing FOLLOW_X_FORWARDED_FOR will make installations which previously
were not able to run with follow_x_forwarded_for configured would begin
behaving as if "follow_x_forwarded_for deny all" was configured.
That is behaviorally the same as --disable-follow-x-forwarded-for.
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.
I do not see a transition problem here.
As per the above cf.data.pre contents, only installations which already
were using --enable-follow-x-forwarded-for were able to set the related
configuration options. They would not see any change.
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.
IMO that is easily resolved. Just drop the DEFAULT_IF_NONE stanza from
cf.data.pre and modify the if-statement in
ClientRequestContext::clientAccessCheck() to set
http->request->indirect_client_addr whenever
"!Config.accessList.followXFF".
That will avoid any "screwing over" because the "indirect" IP would be
the same as the "direct" IP under the above default
follow_x_forwarded_for behavior.
Even if admin has misconfigured one of those *_indirect_client settings
the implicit/default "follow_x_forwarded_for deny all" makes the correct
IP be used.
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.
Dropping the "DEFAULT_IF_NONE" stanza entirely avoids all the ACL costs
by default, without changing the above behavior analysis that proves IMO
this feature as safe to enable.
Bonus: it would also avoid the ACL cycles currently wasted on
installations where the feature is already enabled but not configured.
Extra bonus: any logic which specifically relies on testing
(!)FOLLOW_X_FORWARDED_FOR can switch to checking
(!)Config.accessList.followXFF instead.
[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.
Unnecessary. For reasons detailed above.
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.
IMO this is optional. A "nice to have" UI behaviour that a lot of
squid.conf settings could do, but do not (yet).
The future ConfigParser redesign project changes are likely to make this
type of check a lot easier to implement in an entirely different way
than it would have to be done right now. So IMO there is no hurry to
rush/require this change.
That work will probably fix a few existing bugs. I support this option.
C) Just drop the DEFAULT_IF_NONE stanza from cf.data.pre and modify the
if-statement in ClientRequestContext::clientAccessCheck() like so:
"
if (!http->request->flags.doneFollowXff()) {
/* we always trust the direct client address for actual use */
http->request->indirect_client_addr = http->request->client_addr;
http->request->indirect_client_addr.port(0);
if (Config.accessList.followXFF &&
http->request->header.has(Http::HdrType::X_FORWARDED_FOR)) {
... current logic except above indirect_client_addr lines ...
}
return;
}
"
HTH
Amos
_______________________________________________
squid-dev mailing list
squid-dev@lists.squid-cache.org
https://lists.squid-cache.org/listinfo/squid-dev
