[squid-dev] RFC: make FOLLOW_X_FORWARDED_FOR unconditional

[Alex Rousskov]

On 2023-10-11 03:15, Amos Jeffries wrote:
> 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?

I believe my original email have provided specific problems. Your 
response assessed some of those specific problems are "easily resolved". 
Thus, AFAICT, you know at least of some specific problems and do not 
need me to answer this particular question.


>> 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.

If you exclude performance from "behavior", then, yes, 
follow_x_forwarded_for will be behaviorally the same as with 
--disable-follow-x-forwarded-for.

However, even if we ignore performance, follow_x_forwarded_for is not 
the whole story because, as I said, there are _other_ affected 
squid.conf directives[1] that will become turned on by default if we 
remove FOLLOW_X_FORWARDED_FOR guard. Any correct proof must account for 
those directives code as well. The above proof does not.


>> 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.

My concern is (still) about installations that were using 
--disable-follow-x-forwarded for. Those installations will see a change 
[because they will get a bunch of options and the corresponding code 
that gets enabled by default in their environment after the upgrade].


> 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".

I agree that this DEFAULT_IF_NONE should be dropped during this 
conversion (or in a separate minimal PR; BTW, all "DEFAULT_IF_NONE: deny 
all" should probably be removed, with the corresponding code adjusted as 
needed).

I disagree that the above change alone is an acceptable solution. Among 
other things mentioned in my original response, we should refactor so 
that callers cannot use indirect_client_addr when it was not set, and so 
that indirect_client_addr is only set when an follow_x_forwarded_for 
match has allowed it to be set.

Most likely, access to the client address should be protected (wrapped 
in a parameterized accessor method). This protection is probably a good 
idea regardless, but if Francesco does not want to implement (B), then 
this kind of protection may become an essential part of the proof.


> 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.

Setting indirect_client_addr to an address other than the indirect 
client address is an obvious code quality problem that we should avoid. 
Relying on indirect_client_addr being set (to any value) by code X, when 
we know that code X is not reached in some cases is another problem we 
should avoid in this refactoring.


>> [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.


>> 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).

I agree that a misconfiguration warning is an optional improvement. It 
is very easy to implement though, and I would encourage Francesco to 
start in that direction (i.e. the direction of a good solution) rather 
than starting by adding more hacks and workarounds.

Overall, a PR that enables configuration options that should not be 
enabled is much worse than a PR does does not have that flaw, even if 
both PRs result in the same "behavior" of then-current Squid code due to 
workarounds in the worse PR. This is especially true when the problem 
was foreseen before the start of the development and has a 
straightforward solution, with several positive side-effects.


> 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;
>      }
> "

See above for (C) analysis. Let's do this right instead of adding 
another hack on top of an already problematic (but currently optional!) 
XFF code.


Thank you,

Alex.