Re: [squid-dev] RFC: make FOLLOW_X_FORWARDED_FOR unconditional
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.
Re: [squid-dev] RFC: make FOLLOW_X_FORWARDED_FOR unconditional
On 12/10/23 01:09, ngtech1...@gmail.com wrote: Hey, Not sure I understood exactly what the proposal is? To remove the ./configure --disable-follow-x-forwarded-for build option. Leaving the feature available to everyone. HTH Amos From Amos response I understand that it will be converted into some ACL which can be configured or not. Right? Eliezer From: squid-dev On Behalf Of Francesco Chemolli Sent: Tuesday, October 10, 2023 19:18 To: Squid Developers Subject: [squid-dev] RFC: make FOLLOW_X_FORWARDED_FOR unconditional Hi all, what if we removed the configure option for FOLLOW_X_FORWARDED_FOR, and made it unconditionally part of Squid? It is on by default, and it is controlled by runtime configuration, removing the compile-time option would ensure we have better testing coverage. What do you think? ___ squid-dev mailing list squid-dev@lists.squid-cache.org https://lists.squid-cache.org/listinfo/squid-dev
Re: [squid-dev] RFC: make FOLLOW_X_FORWARDED_FOR unconditional
Hey, Not sure I understood exactly what the proposal is? >From Amos response I understand that it will be converted into some ACL which >can be configured or not. Right? Eliezer From: squid-dev On Behalf Of Francesco Chemolli Sent: Tuesday, October 10, 2023 19:18 To: Squid Developers Subject: [squid-dev] RFC: make FOLLOW_X_FORWARDED_FOR unconditional Hi all, what if we removed the configure option for FOLLOW_X_FORWARDED_FOR, and made it unconditionally part of Squid? It is on by default, and it is controlled by runtime configuration, removing the compile-time option would ensure we have better testing coverage. What do you think? -- Francesco ___ squid-dev mailing list squid-dev@lists.squid-cache.org https://lists.squid-cache.org/listinfo/squid-dev
Re: [squid-dev] RFC: make FOLLOW_X_FORWARDED_FOR unconditional
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:
Re: [squid-dev] RFC: make FOLLOW_X_FORWARDED_FOR unconditional
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
[squid-dev] RFC: make FOLLOW_X_FORWARDED_FOR unconditional
Hi all, what if we removed the configure option for FOLLOW_X_FORWARDED_FOR, and made it unconditionally part of Squid? It is on by default, and it is controlled by runtime configuration, removing the compile-time option would ensure we have better testing coverage. What do you think? -- Francesco ___ squid-dev mailing list squid-dev@lists.squid-cache.org https://lists.squid-cache.org/listinfo/squid-dev
