[squid-dev] RFC: make FOLLOW_X_FORWARDED_FOR unconditional

[Amos Jeffries]

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