Re: [squid-dev] Idea: what if SBuf was a Packable?

[Alex Rousskov]

On 2024-03-13 05:25, Francesco Chemolli wrote:


   I spent some time wondering whether we could reduce
code duplication by making SBuf implement the Packable interface,
and replacing SBufStream with PackableStream.


This idea was studied two years ago, including spending many hours on 
discussing virtualization costs and building specific examples to 
illustrate those costs. You have participated.


https://github.com/squid-cache/squid/pull/932

I also suspect that "code duplication" you are wondering about either 
does not exist or is not related to the suggested changes: SBufStream 
code does not duplicate much, and moving its functionality to SBuf will 
_move_ unique code, not REmove it.


If you want to continue in this overall direction, I suggest identifying 
a specific use case (the simplest one you can find) that illustrates 
code duplication that should not be resolved by other means.



HTH,

Alex.



The interface changes seem rather minimal, SBuf already implements
append(const char*, int) and vappendf; the main difference there is
that SBuf returns *this.
These methods are of course virtual, which is a price to pay, but besides
the vtbl size, maybe there is a way to not pay their cost when not used

What do you think?

--
     Francesco

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
https://lists.squid-cache.org/listinfo/squid-dev


___
squid-dev mailing list
squid-dev@lists.squid-cache.org
https://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] RFC: ACL clashes with Windows system entity

[Alex Rousskov]

On 2023-12-06 16:47, Francesco Chemolli wrote:

Hi all,
   I'm looking at improving windows portability, and we have a name 
clash with a Windows system header 
(https://learn.microsoft.com/en-us/windows/win32/api/winnt/ns-winnt-acl)


So.. how to deal with it?
I can see two options: either we lift ACL into the Acl namespace, or we 
need to rename it. ACL is referenced in 106 files, so the change is 
going to be big, so I ask if there are any opinions on how to go about this



We need to rename/move ACL into Acl::Node or similar (as PR 1350 
attempted to do but got bogged down in many out-of-scope changes).


I suggest to do nothing right now: After 20+ recently merged smooth 
reconfiguration PRs, we are getting really close to reference counting 
ACL objects. If PR 1350 is not merged by that time, we can rename that 
class in that upcoming PR because refcounting changes will have to touch 
a lot of the same code...



HTH,

Alex.

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
https://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] RFC: Squid documentation upgrade

[Alex Rousskov]

On 2023-11-15 11:28, Amos Jeffries wrote:

On 12/10/23 03:32, Alex Rousskov wrote:

On 2023-10-11 02:25, Amos Jeffries wrote:

Hi all,

As those familiar with Squid sources will know the documentation of 
Squid is currently spread across various formats. Some custom ones, 
and some very outdated.


So far we have a casual agreement amongst the core dev team to use 
Markdown when reasonably possible. If anyone has issues with that 
please chime in ASAP, otherwise I shall continue under the assumption 
that we are good to make it "official".


FWIW, I (still) agree that we should use Markdown as human-friendly 
text input format, where reasonable.



I am looking at the <https://pandoc.org/> Markdown tools as possible 
replacement of linuxdoc tools to translate automatically the 
following documentation outputs:

  * release notes,
  * squid.conf.documented,
  * the .man / .info / .pdf files,
  * the INSTALL and QUICKSETUP files


I do not know what "translate [squid.conf.documented and .pdf] 
automatically" (to Markdown) means to you in this context,



Markdown is the proposed source format (in the repository).

The files I listed are **currently** auto-generated from a variety of 
different source formats. My proposal is to change the sources to 
generate **from** Markdown to the various output as-needed.


Thank you for this clarification. Let's see whether we can reach a rough 
agreement on what sources to change and how to change them before 
changing anything.



* release notes: I recommend discussing changes to release notes 
maintenance and structure before changing their format. It would be 
nice to address several persistent problems that we often bump into 
when working on release notes PRs.


That is a different topic of discussion. I am specifically *not* 
changing the produced outputs content.


It is the input that worries me, not the output. AFAICT, you _are_ 
proposing changing how release notes are written (i.e. input). My 
recommendation stands.



* squid.conf.documented: The format of that file is Squid 
configuration format, not Markdown.


Well, yes that is the point. I am hoping the text inside COMMENT, and 
DOC sections can use Markdown so we can auto-generate documentation 
files like a squid.conf.8 or the .html more easily with links - without 
making the plain-text form obtuse.


I support changing text inside COMMENT and DOC sections to Markdown, but 
only if that change is accompanied by cf.data.pre restructuring. IIRC, 
migration to Markdown was a part of those restructuring plans/discussions.




We have discussed how cf.data.pre and friends should be
restructured and reformatted. We should finish those discussions
and implement those changes instead.


IIRC we last discussed it back around 2009-ish with Henrick's proposal 
to split it into a set of multiple files. Are you referring to that?


I believe that those discussions did include the idea of splitting 
cf.data.pre, including providing a dedicated file for each squid.conf 
directive. I do not think those discussions were limited to that idea.


One of the problems we need to address during this restructuring is 
referencing, at least at the level of individual directives and major 
syntax concepts. At the end, one directive description should be able to 
refer to another directive or concept, with rendered versions providing 
the corresponding links and indexes.



* .man, .info, .pdf files: Your call (assuming .8 and similar source 
files in squid repository remain unchanged).


Assumption is wrong. What I mean is that the source document gets 
converted to Markdown and we have the tools auto-generate the man.8, 
info, or pdf files from that as needed by the user/distro.


Thank you for clarifying this! I need more information to make a 
decision on this item:


* Do you plan to exclude .8 files that are currently generated from 
other sources (e.g., src/log/DB/log_db_daemon.8 and 
src/security/cert_validators/fake/security_fake_certverify.8)?


* AFAICT, .8 files are written in troff or mdoc. Those formatting 
languages have lots of documentation-focused macros that standard 
Markdown does not support. Is there a variation of Markdown (that you 
plan to use for manual page conversion) that adds documentation-writing 
markup? Or are we going to "dumb down" this documentation due to 
Markdown inability to express certain documentation concepts?



* INSTALL and QUICKSETUP: I support rewriting these and other 
top-level [A-Z]* files in Markdown, where possible. The other 
candidates are CONTRIBUTORS, COPYING, CREDITS, ChangeLog, README, and 
SPONSORS(.list).




COPYING is not on the table since that is the mandatory GPL text not 
under our control.


I am not sure we cannot convert COPYING to valid Markdown without 
violating GPL, but I am not going to argue about it. Glad we appear to 
agree regarding the other top-level [A-Z]* files. FWIW, except for 
ChangeLog and CONTRIBUT

Re: [squid-dev] mirrors with missing files

[Alex Rousskov]

On 2023-11-02 07:59, Stuart Henderson wrote:

On 2023-11-01, Amos Jeffries  wrote:

On 1/11/23 09:59, Alex Rousskov wrote:

On 2023-10-31 15:39, Francesco Chemolli wrote:

Before we can migrate ..., we need to deprecate, cleanup and simplify
a lot.


Do you really, really _need_ to "deprecate, cleanup, and simplify a lot"
in order to stop mirroring tomorrow?! Start doing new minor releases on
GitHub a month from now? FWIW, I have asked for specifics many times,
but am still unaware of any serious obstacles on the way to those goals.



As I have mentioned multiple times ... there is a reason we have Jenkins
building the tarballs.


Please run the following commands:

wget
https://github.com/squid-cache/squid/archive/refs/tags/SQUID_6_3.tar.gz &&
tar -xvf SQUID_6_3.tar.gz &&
./configure &&
make check


That's using github's on-the-fly generated tarballs; it's also possible to
upload a "release artefact" which is the proper project-generated tar.gz.



Hi Stuart,

I hope you have better luck with dismantling these poor excuses and 
FUD! I have been doing that for a long time with no signs of success. 
"Where there is a will, there is a way". We lack the former IMO.



> (and it doesn't preclude _also_ having mirrors)

... or other valid solutions (for other problems, including the alleged 
problem of "USA hosted content").


Alex.



See e.g. the irssi releases, https://github.com/irssi/irssi/releases/tag/1.4.5
(though also notice the ugly way they try to make sure people download the
correct file...)



___
squid-dev mailing list
squid-dev@lists.squid-cache.org
https://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] mirrors with missing files

[Alex Rousskov]

On 2023-11-01 12:05, Amos Jeffries wrote:

On 1/11/23 09:59, Alex Rousskov wrote:

On 2023-10-31 15:39, Francesco Chemolli wrote:
Before we can migrate ..., we need to deprecate, cleanup and simplify 
a lot.


Do you really, really _need_ to "deprecate, cleanup, and simplify a 
lot" in order to stop mirroring tomorrow?! Start doing new minor 
releases on GitHub a month from now? FWIW, I have asked for specifics 
many times, but am still unaware of any serious obstacles on the way 
to those goals.


As I have mentioned multiple times ... there is a reason we have Jenkins 
building the tarballs.


The reasons I have heard multiple times do not preclude disabling 
mirroring tomorrow and doing releases on GitHub a month from now.



AFAICT, there is just lack of shared goals (or shared priorities) 
rather than serious obstacles on the way of achieving those two 
specific goals.


FWIW, one of those goals that we appear not to share is freedom of 
information > ... specifically to have Squid code available to everyone.
FWIW, I support that particular goal (among others). That goal certainly 
does not preclude disabling mirroring tomorrow and doing releases on 
GitHub a month from now.



There exist countries in this world who forbid access to USA hosted 
content. Having squid-cache.org mirrored keeps us largely out of the 
political arean.


We do not need mirrors to make Squid available on non-US hosted servers, 
especially stale, broken, and infested mirrors (i.e. the subject of this 
email thread).


Alex.

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
https://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] mirrors with missing files

[Alex Rousskov]

On 2023-10-31 15:39, Francesco Chemolli wrote:
Before we can migrate ..., we need to 
deprecate, cleanup and simplify a lot.


Do you really, really _need_ to "deprecate, cleanup, and simplify a lot" 
in order to stop mirroring tomorrow?! Start doing new minor releases on 
GitHub a month from now? FWIW, I have asked for specifics many times, 
but am still unaware of any serious obstacles on the way to those goals.


AFAICT, there is just lack of shared goals (or shared priorities) rather 
than serious obstacles on the way of achieving those two specific goals.


Alex.


On 2023-10-31 15:39, Francesco Chemolli wrote:
> Hi all,
> I agree, it would be awesome to rely on a more more than delivery
> network for our website.
> At this time it is complicated. Oh website is large as a result of
> posting mailing list, archives and a lot of release files, patches,
> change sets. Before we can migrate to anything different, we need to
> deprecate, cleanup and simplify a lot.
> Any help would be greatly appreciated
>
> @mobile
>


On Tue, 31 Oct 2023 at 19:32, Alex Rousskov wrote:

On 2023-10-31 06:08, Adam Majer wrote:
 > I've looked at the mirrors posted,
 >
 > http://www.squid-cache.org/Download/mirrors.html
 >
 > and these seem all obsolete.

Thank you for doing this analysis! I have been begging the Project to
drop all mirrors for a long time now. I failed to convince others that
the associated embarrassment and waste of time are just not worth the
benefits. Maybe your observations will be the last straw...


 > I'm not sure, but maybe a more reliable way would be to migrate to
 > github.com <http://github.com> for the releases.

FWIW, I am quite sure, and has been making that suggestion for several
years now. GitHub is far from perfect, but using its release posting
features (correctly) will dramatically decrease our release overheads
and delays. This migration is not trivial, and I have failed to
convince
others that it is worth their time to start migrating.

Alex.

___
squid-dev mailing list
squid-dev@lists.squid-cache.org <mailto:squid-dev@lists.squid-cache.org>
https://lists.squid-cache.org/listinfo/squid-dev
<https://lists.squid-cache.org/listinfo/squid-dev>



___
squid-dev mailing list
squid-dev@lists.squid-cache.org
https://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] mirrors with missing files

[Alex Rousskov]

On 2023-10-31 06:08, Adam Majer wrote:

I've looked at the mirrors posted,

   http://www.squid-cache.org/Download/mirrors.html

and these seem all obsolete.


Thank you for doing this analysis! I have been begging the Project to 
drop all mirrors for a long time now. I failed to convince others that 
the associated embarrassment and waste of time are just not worth the 
benefits. Maybe your observations will be the last straw...



I'm not sure, but maybe a more reliable way would be to migrate to 
github.com for the releases.


FWIW, I am quite sure, and has been making that suggestion for several 
years now. GitHub is far from perfect, but using its release posting 
features (correctly) will dramatically decrease our release overheads 
and delays. This migration is not trivial, and I have failed to convince 
others that it is worth their time to start migrating.


Alex.

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
https://lists.squid-cache.org/listinfo/squid-dev

[squid-dev] RFC: Irreplaceable squidclient features

[Alex Rousskov]

Hello,

Francesco and I would like to remove squidclient tool from Squid so 
that we can divert resources to more important areas[1]. As far as we 
can tell, all essential squidclient functionality can be obtained via 
well-known command-line clients like curl, wget, nc, s_client, etc. For 
example, `mgr:foo` shortcuts can be replaced with URLs like 
`http://localhost:3128/squid-internal-mgr/foo` (in a script or shell 
alias if needed).


If you routinely use squidclient _and_ do not think you can replace it 
with another client, please respond and detail your use case!


We will collect use cases until the end of October 2023. If we then 
remove squidclient as currently planned, then that removal will _not_ 
affect Squid v6 and earlier releases, of course -- those releases will 
keep squidclient.



Thank you,

Alex.
P.S. This email thread is _not_ the right place to discuss squidclient 
_problems_, including Squid Bug #5283[2]. Please focus on squidclient 
features that you use and consider irreplaceable.


[1] https://github.com/squid-cache/squid/pull/1514
[2] https://bugs.squid-cache.org/show_bug.cgi?id=5283
___
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

[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

Re: [squid-dev] RFC: Squid documentation upgrade

[Alex Rousskov]

On 2023-10-11 02:25, Amos Jeffries wrote:

Hi all,

As those familiar with Squid sources will know the documentation of 
Squid is currently spread across various formats. Some custom ones, and 
some very outdated.


So far we have a casual agreement amongst the core dev team to use 
Markdown when reasonably possible. If anyone has issues with that please 
chime in ASAP, otherwise I shall continue under the assumption that we 
are good to make it "official".


FWIW, I (still) agree that we should use Markdown as human-friendly text 
input format, where reasonable.



I am looking at the  Markdown tools as possible 
replacement of linuxdoc tools to translate automatically the following 
documentation outputs:

  * release notes,
  * squid.conf.documented,
  * the .man / .info / .pdf files,
  * the INSTALL and QUICKSETUP files


I do not know what "translate [squid.conf.documented and .pdf] 
automatically" (to Markdown) means to you in this context, especially 
since (in the official squid repository) "linuxdoc tools" are only used 
in release notes context. Reaching a rough agreement on what to change, 
how to change it, and where to change it before changing it may be 
prudent. Here are a few comments to bootstrap that process:


* release notes: I recommend discussing changes to release notes 
maintenance and structure before changing their format. It would be nice 
to address several persistent problems that we often bump into when 
working on release notes PRs.


* squid.conf.documented: The format of that file is Squid configuration 
format, not Markdown. FWIW, I am against changing the entire cf.data.pre 
format to Markdown. I am against keeping cf.data.pre as is but 
translating squid.conf.documented format (or portions thereof) to 
Markdown (especially if that requires changes in squid repository). We 
have discussed how cf.data.pre and friends should be restructured and 
reformatted. We should finish those discussions and implement those 
changes instead.


* .man, .info, .pdf files: Your call (assuming .8 and similar source 
files in squid repository remain unchanged).


* INSTALL and QUICKSETUP: I support rewriting these and other top-level 
[A-Z]* files in Markdown, where possible. The other candidates are 
CONTRIBUTORS, COPYING, CREDITS, ChangeLog, README, and SPONSORS(.list).



HTH,

Alex.


This would impact anyone building Squid packages from our repository 
instead of the formal release bundles. As well as our testing 
infrastructure.


The tools are GPL with sources public in github, so SHOULD be easily 
available everywhere. At the very least it has been available in both 
Debian and RHEL distributions for many years. If this change to Squid 
build requirements is an issue for anyone please speak up.



Cheers
Amos
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
https://lists.squid-cache.org/listinfo/squid-dev


___
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

[Alex Rousskov]

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

Re: [squid-dev] RFC: Transitioning ipcache and fqdncache to ClpMap

[Alex Rousskov]

On 7/11/23 03:35, Francesco Chemolli wrote:

I'd like to start working on transitioning ipcache and fqdncache to 
ClpMap.


Thank you. Please _plan_ to convert them both (as you are already doing 
here!), but then convert them one at a time (to avoid duplicating 
review/modification efforts). I bet we will need at least 6 "minimal" 
PRs here.




There is only one snag,


FWIW, I did not check all the details, but I see two more snags, 
including a very important/difficult one:


2. ipcache.cc code "locks" entries to prevent their removal from the 
cache in certain cases. See ipcache_entry::locks. The corresponding code 
already has serious problems, but the approach itself needs to be 
refactored using RefCount, so that a cache entry can be removed from the 
cache at any time. This is not going to be easy and will require at 
least one dedicated PR, but I believe it is an essential preliminary step.


3. We should also remove ipcache_low and ipcache_high directives before 
the conversion. Those two parameters do not make sense for an 
instant-removal cache: They are not needed for anything useful, create 
performance anomalies, and increase code/configuration complexity. The 
corresponding logic is not supported by ClpMap.



Now back to your snag #1:

the current configuration directives specify a cache 
size in number of entries, where ClpMap specifies a max size.


I can see two ways forward:
1. Add a second max-size parameter to ClpMap, ensuring that it starts 
expunging entries when the maximum capacity in either memory use or 
number of entries is reached


I am not against this option, especially if we deprecate/remove existing 
count-based configuration (ipcache_size and friends) and add byte-based 
configuration to replace it (the directive name is to be determined).



2. guesstimate how many bytes of memory an ipcache/fqdncache uses, and 
convert


I support this option if we deprecate/remove existing counter-based 
configuration (ipcache_size and friends) and add bytes-based 
configuration (the directive name is to be determined). I prefer this 
option in this case because it avoids making ClpMap code more complex 
long-term just to accommodate a short-term deprecation need. The extra 
code complexity is not huge, but cache limit enforcement is tricky, and 
the history of the related ClpMap code suggests that adding a new limit 
vector will take several (likely painful) iterations.



Regardless, I believe that from the user-facing perspective, a solid way 
forward is to give to the ipcache_size and fqdncache_size directives an 
option to specify a max size in entries or, by adding a memory-size 
suffix, in used memory, and possibly deprecate the max-size-in-entries 
option in Squid 7 and retire it in squid 8.


In the vast majority of deployments, the existing count-based limit is 
inferior to the byte-based limit because memory bytes is a real resource 
that admins understand and have to optimize/ration/limit (while the 
number of entries has no direct relationship to anything most admins 
should care about). Thus, long-term, we should provide byte-based 
configuration options.


To keep things simple and to facilitate deprecation, I recommend adding 
two new directives instead of making the existing directives more 
complex. Naming is a separate/secondary issue, but we can model the new 
names based on the two existing primary caches (cache_dir and 
cache_mem).  Alternatively, we can use a more natural(?) word order. 
Here are a few variants (from most to least preferred by me):


* dns_cache and reverse_dns_cache
* cache_dns and cache_dns_reverse
* cache_ip and cache_domain


HTH,

Alex.

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] Squid 5.6 leaking memory when peeking for an origin with an invalid certificate

[Alex Rousskov]

On 3/23/23 19:56, Hamilton Coutinho wrote:

We found a way to reproduce the leak: set up squid to run in intercept 
mode + SSL description + memory pools off (to ease identifying the leak) 
and then generate requests to sites with invalid certs (eg, CA not 
installed), for instance: curl -k https://slscr.update.microsoft.com 


Thanks a lot for sharing those details! Using them, I was able to 
reproduce one memory leak. Please try the following fix:

https://github.com/squid-cache/squid/pull/1346

I do not know whether that PR changes port to v5 cleanly or whether the 
same bug exists in v5. I can only speculate that v5 has a similar leak.




squidclient mgr:mem should show ever increasing HttpRequest instances.


As far as I can tell, the HttpRequest object created 
in ConnStateData::buildFakeRequest() is never freed because its refcount 
 > 0.



Any ideas where an HTTPMSGUNLOCK() might be missing?


Sorry, I do not know. In my master/v7-based tests, HttpRequest objects 
are not leaking. However, I am not testing an intercepting Squid. I 
still have one red flag to investigate, so perhaps I will be able to 
reproduce and fix that HttpRequest leak as well.



HTH,

Alex.



On Wed, Jan 18, 2023 at 11:28 AM Hamilton Coutinho wrote:

Hi Alex,

Thanks for the prompt reply! Thanks also for the clarifications.

Agreed, I just realized the requests seem to be failing
with Http::scServiceUnavailable, so my focus turned
to Security::PeerConnector::sslCrtvdHandleReply() and friends.

Best.

On Wed, Jan 18, 2023 at 11:11 AM Alex Rousskov
mailto:rouss...@measurement-factory.com>> wrote:

On 1/18/23 13:46, Hamilton Coutinho wrote:
 > Hi all,
 >
 > We are observing what seems to be several objects leaking in
the output
 > mgr:mem, to the tune of 10s of 1000s
 >
of HttpRequest, HttpHeaderEntry, Comm::Connection, 
Security::ErrorDetail, cbdata PeekingPeerConnector (31), etc.
 >
 > We dumped a core and managed to find some HttpRequest objects
and they
 > all seem to have failed in the same way, with an
ERR_SECURE_CONNECT_FAIL
 > category, for a site that has a certificate signed by a CA
authority not
 > available to squid.
 >
 > If I would guess, the origin of the problem might be in
 > Ssl::PeekingPeerConnector::checkForPeekAndSpliceMatched():
 >
 >      if (finalAction == Ssl::bumpTerminate) {
 >          bail(new ErrorState(ERR_SECURE_CONNECT_FAIL,
Http::scForbidden,
 > request.getRaw(), al));
 >          clientConn->close();
 >          clientConn = nullptr;
 >
 > Wondering if assigning null to clientConn there would be
premature.


FWIW, that connection pointer reset itself looks OK to me.
ConnStateData
and/or others should have a connection closure handler attached
to the
clientConn descriptor. That handler should be notified by Comm and
initiate cleanup of the objects responsible for client-Squid
communication.

The bail() call above should inform the requestor about the
error/termination and terminate this AsyncJob. That requestor
should
then close the Squid-server connection and clean up associated
state.

While there may be bugs in those "should..." sequences, please
note that
the pasted code is not related to handling of untrusted origin
servers
(unless your ssl_bump rules specifically activate the terminate
action
upon discovering such an origin server). The pasted code is
reacting to
an "ssl_bump terminate" rule matching.


Cheers,

Alex.

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
<mailto:squid-dev@lists.squid-cache.org>
http://lists.squid-cache.org/listinfo/squid-dev
<http://lists.squid-cache.org/listinfo/squid-dev>



-- 
Hamilton




--
Hamilton

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev


___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] RFC: GitHub Projects and Issues

[Alex Rousskov]

On 5/5/23 09:39, Amos Jeffries wrote:

You may (or not) have noticed that recently I have been experimenting 
with GitHub Projects.
Creating a few for the major long-term efforts and assigned a number of 
the open PRs to them.


IMO this looks like it could be a better way to track progress on 
incomplete features or code conversions instead of wiki Feature pages.


I am against using GitHub Projects for projects that lack Squid Project 
consensus. Just like Feature pages, GitHub Projects currently create a 
false impression that the Squid Project has agreed that some activity is 
a good idea, that some details of that activity have been reviewed and 
approved by the Squid Project, and/or that some GitHub PRs match that 
good idea.


I wish you have not started using GitHub Projects before discussing that 
shared Squid Project resource use. Please pause that experiment.



That limitation might be resolved by enabling GitHub Issues for (only) 
feature-enhancement TODO items prior to PR creation.


Enabling GitHub Issues and then rejecting "regular" bug reports 
submitted via GitHub Issues is bad UX. IMO, we should either enable 
GitHub Issues for regular bug reports and other issues that folks expect 
to file via GitHub Issues (and deprecate Bugzilla) or not enable them at 
all.



HTH,

Alex.

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] CI: trunk is stable again

[Alex Rousskov]

On 4/11/23 15:52, Francesco Chemolli wrote:


trunk build tests can now be relied upon again for correctness checks.
We can set them up again as required for merging PRs


Done. PRs based on earlier/broken master snapshots will need to be 
updated to pass these now-required checks, of course.


Thanks a lot for fixing Jenkins tests!

Alex.

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] Latest Clang build errors

[Alex Rousskov]

On 4/2/23 03:30, Francesco Chemolli wrote:


BTW, since GCC v13 has other bugs and has not been officially
"released"
yet, I suggest removing that compiler from the set of required tests
until it matures enough for us to support it efficiently.


That's what Fedora Rawhide ships as default compiler.
We can only do that by stopping to target rawhide for our tests;


I disagree that it is the only alternative. For _example_, if somebody 
wants to spend time on future GCC v13 release compatibility, we can run 
_optional_ Jenkins tests with GCC v13 on Rawhide (of the master branch 
or even PR branches) that they will monitor and post Squid bug fixing 
PRs as needed.


As far as Project policy is concerned, given our scarce resources, I 
think it is best to exclude development compiler versions from the 
official support scope. Doing so does not stop anybody from testing with 
those compilers (even using Project's Jenkins setup) and submitting 
Squid improvements.




I'd like to get more feedback before doing so.


Good luck,

Alex.

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] Latest Clang build errors

[Alex Rousskov]

On 3/28/23 09:27, Amos Jeffries wrote:
Alex, since the whole IPC and SHM system is your design are you able to 
work on fixing the FlexibleArray build errors we are now getting with 
clang v15.



[1] Log excerpt from Jenkins:

...
01:00:06 ../../../../src/ipc/mem/FlexibleArray.h:34:52: error: array subscript -1 is below array bounds of 'int [1]' [-Werror=array-bounds=] 

...> 01:00:06 make[4]: Leaving directory

'/srv/jenkins/workspace/5-pr-test/COMPILER/gcc/OS/fedora-rawhide/label/docker-build-host/btlayer-02-maximus/squid-7.0.0-VCS/_build/sub/src/fs'



AFAICT, the above log is not from a clang build; it is from a GCC build. 
FWIW, I could not find matching clang errors in Jenkins.


With GCC, I can reproduce the -Warray-bounds problem using 
squidcache/buildfarm-fedora-rawhide:latest docker. Thank you, Francesco 
for publishing those! PR 1318 has the proposed fix:

https://github.com/squid-cache/squid/pull/1318


BTW, since GCC v13 has other bugs and has not been officially "released" 
yet, I suggest removing that compiler from the set of required tests 
until it matures enough for us to support it efficiently.



HTH,

Alex.

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] Latest Clang build errors

[Alex Rousskov]

On 3/28/23 09:27, Amos Jeffries wrote:
Alex, since the whole IPC and SHM system is your design are you able to 
work on fixing the FlexibleArray build errors we are now getting with 
clang v15.


Sure, I will work on this. I doubt I will be able to post a fix in the 
next few days due to travel, but I am sure we can figure it out in a 
week or so.


Alex.



[1] Log excerpt from Jenkins:

01:00:06 In file included from ../../../../src/ipc/StoreMap.h:12,
01:00:06  from ../../../../src/fs/rock/RockRebuild.h:17,
01:00:06  from ../../../../src/fs/rock/RockRebuild.cc:15:
01:00:06 In member function 'Item& 
Ipc::Mem::FlexibleArray::operator[](int) [with Item = long 
unsigned int]',
01:00:06 inlined from 'Ipc::StoreMapItems::Item& 
Ipc::StoreMapItems::at(int) [with C = long unsigned int]' at 
../../../../src/ipc/StoreMap.h:136:21,
01:00:06 inlined from 'Rock::LoadingEntry::LoadingEntry(sfileno, 
Rock::LoadingParts&)' at ../../../../src/fs/rock/RockRebuild.cc:199:27:
01:00:06 ../../../../src/ipc/mem/FlexibleArray.h:34:52: error: array 
subscript -1 is below array bounds of 'long unsigned int [1]' 
[-Werror=array-bounds=]
01:00:06    34 | Item  [](const int idx) { return 
items[idx]; }

01:00:06   | ~^
01:00:06 ../../../../src/ipc/mem/FlexibleArray.h: In constructor 
'Rock::LoadingEntry::LoadingEntry(sfileno, Rock::LoadingParts&)':
01:00:06 ../../../../src/ipc/mem/FlexibleArray.h:43:10: note: while 
referencing 'Ipc::Mem::FlexibleArray::items'
01:00:06    43 | Item items[1]; // ensures proper alignment of array 
elements

01:00:06   |  ^
01:00:06 In member function 'Item& 
Ipc::Mem::FlexibleArray::operator[](int) [with Item = unsigned int]',
01:00:06 inlined from 'Ipc::StoreMapItems::Item& 
Ipc::StoreMapItems::at(int) [with C = unsigned int]' at 
../../../../src/ipc/StoreMap.h:136:21,
01:00:06 inlined from 'Rock::LoadingEntry::LoadingEntry(sfileno, 
Rock::LoadingParts&)' at ../../../../src/fs/rock/RockRebuild.cc:200:33:
01:00:06 ../../../../src/ipc/mem/FlexibleArray.h:34:52: error: array 
subscript -1 is below array bounds of 'unsigned int [1]' 
[-Werror=array-bounds=]
01:00:06    34 | Item  [](const int idx) { return 
items[idx]; }

01:00:06   | ~^
01:00:06 ../../../../src/ipc/mem/FlexibleArray.h: In constructor 
'Rock::LoadingEntry::LoadingEntry(sfileno, Rock::LoadingParts&)':
01:00:06 ../../../../src/ipc/mem/FlexibleArray.h:43:10: note: while 
referencing 'Ipc::Mem::FlexibleArray::items'
01:00:06    43 | Item items[1]; // ensures proper alignment of array 
elements

01:00:06   |  ^
01:00:06 In member function 'Item& 
Ipc::Mem::FlexibleArray::operator[](int) [with Item = int]',
01:00:06 inlined from 'Ipc::StoreMapItems::Item& 
Ipc::StoreMapItems::at(int) [with C = int]' at 
../../../../src/ipc/StoreMap.h:136:21,
01:00:06 inlined from 'Rock::LoadingSlot::LoadingSlot(Rock::SlotId, 
Rock::LoadingParts&)' at ../../../../src/fs/rock/RockRebuild.cc:208:27:
01:00:06 ../../../../src/ipc/mem/FlexibleArray.h:34:52: error: array 
subscript -1 is below array bounds of 'int [1]' [-Werror=array-bounds=]
01:00:06    34 | Item  [](const int idx) { return 
items[idx]; }

01:00:06   | ~^
01:00:06 ../../../../src/ipc/mem/FlexibleArray.h: In constructor 
'Rock::LoadingSlot::LoadingSlot(Rock::SlotId, Rock::LoadingParts&)':
01:00:06 ../../../../src/ipc/mem/FlexibleArray.h:43:10: note: while 
referencing 'Ipc::Mem::FlexibleArray::items'
01:00:06    43 | Item items[1]; // ensures proper alignment of array 
elements

01:00:06   |  ^
01:00:06 cc1plus: all warnings being treated as errors
01:00:06 make[4]: *** [Makefile:986: rock/RockRebuild.lo] Error 1
01:00:06 make[4]: Leaving directory 
'/srv/jenkins/workspace/5-pr-test/COMPILER/gcc/OS/fedora-rawhide/label/docker-build-host/btlayer-02-maximus/squid-7.0.0-VCS/_build/sub/src/fs'

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] RFC: policy change for header #includes

[Alex Rousskov]

On 3/8/23 09:12, Amos Jeffries wrote:

On 7/03/2023 10:14 pm, Francesco Chemolli wrote:
I would also complement it with the directive to use 
header  instead of  whenever possible




That we already have. PRs doing the updates welcome.


Agreed on both counts, especially if an updating PR updates all known 
cases rather than just "a few here and there".



>> this could also be automatically enforced

This part is not implemented, and it is not trivial to implement (please 
ping me if you decide to work on that). I would welcome proper enforcement.



Cheers,

Alex.

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] RFC: policy change for header #includes

[Alex Rousskov]

On 3/7/23 00:38, Amos Jeffries wrote:


Current Policy 
https://wiki.squid-cache.org/DeveloperResources/SquidCodingGuidelines#file-include-guidelines:

  4. system C headers (with a .h suffix):
     * mandatory HAVE_FOO_H wrapper



I propose using the C++17 "__has_include()" instead of HAVE_FOO_H 
whenever we can. Which is:

  * all .cc and .cci files
  * any .h files in the src/ and tools/ areas



We should not use __has_include() instead of HAVE_FOO_H for most system 
include files because HAVE_FOO_H essentially means "can compile" while 
__has_include() means "exists". Essentially, __has_include() has the 
same problems that old AC_CHECK_HEADER and AC_CHECK_HEADERS macros had, 
before Autoconf changed them to check whether the header is not just 
present but can be compiled. We should not go back to unreliable 
existence checks IMO!


The semantic difference detailed above is sufficient to avoid the 
proposed change IMO, but I can also mention another significant problem: 
The increased difficulty in deciding when to use __has_include() and 
when HAVE_FOO_H. We already suffer from this problem (a little) in some 
special "Do not use HAVE_X_H for x.h!" cases, but unreliable 
environment-dependent __has_include() checks are likely to make it 
significantly worse IMO.


If we should not use __has_include() instead of HAVE_FOO_H for _most_ 
include files, then we should not use it at all: Code consistency and 
less bickering during reviews are worth the extra ./configure 
changes/time IMO, especially since we do not add new system headers that 
often.



HTH,

Alex.
P.S. To save time, I am not detailing my partial disagreement with the 
pros/cons statements quoted below.




Pros:
  * less configure.ac logic, smaller build logs
  * removes need to do a full bootstrap + re-configure when a 
third-party library or system header is added/remove/changed

  * conversion can be semi-scripted

Cons AFAIK are all about how it cannot be a blanket requirement like 
HAVE_FOO_H is:
  * sometimes headers need to be forbidden include by ./configure logic 
checks

  * C language does not support the __has_include



___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] Drop cache_object protocol support

[Alex Rousskov]

On 1/26/23 04:12, Amos Jeffries wrote:

On 26/01/2023 3:30 am, Alex Rousskov wrote:

On 1/25/23 07:29, Amos Jeffries wrote:

On 25/01/2023 5:34 pm, Alex Rousskov wrote:
IMO, we should not keep any code that is only needed for Squid v3.1 
and earlier. Squid v3.2 and later should http-based cache manager 
access, right? More code always means more maintenance overheads and 
higher change costs. Given our lack of resources, we should start 
ignoring Squid v3 needs.


In sentiment I agree. In practicality we have to cope with "LTS" from 
vendors, and Squid bugs in the manager.


IMO, LTS vendors and old Squid bugs do not prevent us from removing 
cache_object support from cachemgr.cgi: The number of admins that 
match _all_ of the conditions listed below at the same time is 
negligible.



Evidence needed.


It would be nice to back up my reasoning with hard evidence, but I do 
not think it is _required_ in this case. It is impractical to prove 
negligibly low count of X when there is no way to enumerate all 
instances where X might exist. These kind of decisions require a 
judgement call.


Circumstantial evidence does exist -- the Project does not see a 
constant stream of complaints, bug reports, and improvements related to 
cachemgr.cgi despite its many shortcomings. We also do not see a lot of 
complaints about v3.2 or a lot of examples where v3 users cannot 
upgrade. To avoid misunderstanding, I am not claiming that evidence 
proves my point beyond reasonable doubt. It is only partial and 
circumstantial.




We should not spend our resources "coping" with those esoteric cases.

1. Use Squid v3.
2. Use Squid v7.
3. Use cachemgr.cgi to manage both Squid versions.
4. Cannot use cachemgr.cgi from Squid v6.
5. Cannot patch cachemgr.cgi v7 to restore cache_object support.


Declaring groups of users "negligible" or "esoteric" without evidence 
just to push your preferred decision is not a valid line of argument.


First of all, I am not declaring groups of users "negligible" or 
"esoteric". I am speculating about the number of certain use cases and 
classifying those use cases.


I believe my line of argument is valid, even though it is not (and, 
given existing Project limitations, cannot be) backed by hard evidence. 
We will never know how many use cases match all 5 criteria. In fact, we 
will never know how many Squid use cases are out there today!


If you have hard evidence that illustrates that relevant use cases are 
common, please present it. Needless to say, I am not aware of such evidence.



v3.2 has http: but the https:, ftp:, whois:, gopher: schemes were 
broken until late in the v3.5 series backports.

So going by [1] LTS systems still using v3.2 are still a pain.


We can stop that pain any time we want. All it takes is for us to stop 
mentioning v3 releases when making design decisions like this one. We 
_choose_ to prolong that pain and to spend scarce resources on a 
negligible percentage of unimportant use cases instead of spending 
those resources on popular and important use cases.


We are making decisions which affect other peoples lives and employment. 
The "pain" I have been mentioning is _their_ pain, not ours.


When making a decision, we should take into account the "lives and 
employment" and "pains" of all affected people. We cannot make everybody 
happy, and if we try, we are likely to increase the overall level of 
unhappiness. Making (few) v3 users happier at the expense of hurting 
(many more) v5 users is the wrong balance IMO, and that is what we often 
end up doing when it comes to this kind of decisions.



It is appropriate to keep the human aspect in mind for this decision and 
utilize the data we do have available.


Agreed. That is exactly what I am doing.


That data I have says v3.2 and v3.5 are still relevant factors for this 
change.


Their relevance is not being disputed. What we disagree on is whether 
those (real) v3 user needs are enough to hurt other (also real) users.



The longer we wait on removal from the CGI and CLI tools (only) the 
more seamless it goes. So I am inclined to be very conservative on 
the tools capability removal and proactive on ensuring they can cope 
with the squid capability loss.


And since there is no way to actually measure "more seamless" or to 
prove that we are wasting resources on rare unimportant use cases, we 
end up doing what we do best -- increasing and prolonging pain.




We do have at least one measure. I use the count of Vendors distributing 
each problematic release.
Over time their old versions reach EOL, or LTS get updated to new Squid 
versions etc.
We can reasonably expect admin to be using a currently supported version 
of their OS packages.


The proposed measure is just too coarse to be applicable here because 
the proposal does _not_ inconvenience users of problematic releases. In 
fact, the proposal doe

Re: [squid-dev] Drop cache_object protocol support

[Alex Rousskov]

On 1/25/23 07:29, Amos Jeffries wrote:

On 25/01/2023 5:34 pm, Alex Rousskov wrote:

On 1/24/23 20:57, Amos Jeffries wrote:
Blocker #2: The squidclient tool still sends cache_object: scheme 
when given "mgr:" on the CLI. We need to upgrade that first 


Looks like we are in agreement on that.


and allow admin some time to upgrade before removing the scheme 
support in squid itself.


Agreed. Would six months be enough in your opinion? If yes, we may be 
able to remove cache_object support in v6. Otherwise, we can remove 
cache_object support starting with v7 (as far as numbered releases are 
concerned).



v6 will "feature freeze" in 10 days.


With proper cooperation, 10 days is more than enough to remove 
cache_object support, but I am not going to fight for that given your 
resistance.




Early in v7 cycle should be good.


Unless you stop me, I will post a message to squid-users to warn the 
admins that they should not count on Squid instances supporting 
cache_object scheme in v7 releases.



IMO, we should not keep any code that is only needed for Squid v3.1 
and earlier. Squid v3.2 and later should http-based cache manager 
access, right? More code always means more maintenance overheads and 
higher change costs. Given our lack of resources, we should start 
ignoring Squid v3 needs.


In sentiment I agree. In practicality we have to cope with "LTS" from 
vendors, and Squid bugs in the manager.


IMO, LTS vendors and old Squid bugs do not prevent us from removing 
cache_object support from cachemgr.cgi: The number of admins that match 
_all_ of the conditions listed below at the same time is negligible. We 
should not spend our resources "coping" with those esoteric cases.


1. Use Squid v3.
2. Use Squid v7.
3. Use cachemgr.cgi to manage both Squid versions.
4. Cannot use cachemgr.cgi from Squid v6.
5. Cannot patch cachemgr.cgi v7 to restore cache_object support.


v3.2 has http: but the https:, ftp:, whois:, gopher: schemes were broken 
until late in the v3.5 series backports.

So going by [1] LTS systems still using v3.2 are still a pain.


We can stop that pain any time we want. All it takes is for us to stop 
mentioning v3 releases when making design decisions like this one. We 
_choose_ to prolong that pain and to spend scarce resources on a 
negligible percentage of unimportant use cases instead of spending those 
resources on popular and important use cases.



For completeness, that MGR_INDEX regression you fixed a short while ago 
also means some broken v4/v5 releases may be a pain source during the 
transition.


Those releases should not be considered a pain in this context because 
if that bug is actually important, it will be fixed in those releases.



The longer we wait on removal from the CGI and CLI tools (only) the more 
seamless it goes. So I am inclined to be very conservative on the tools 
capability removal and proactive on ensuring they can cope with the 
squid capability loss.


And since there is no way to actually measure "more seamless" or to 
prove that we are wasting resources on rare unimportant use cases, we 
end up doing what we do best -- increasing and prolonging pain.


Alex.

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] Drop cache_object protocol support

[Alex Rousskov]

On 1/24/23 20:57, Amos Jeffries wrote:

Blocker #1:  The cachemgr_passwd directly still needs to be cleanly 
removed, eg replaced by a manager_access ACL based mechanism.


I do not see a relationship: I have not tested it, but the existing 
CacheManager::ParseHeaders() code already extracts authentication 
information from cache manager requests that use "http" scheme AFAICT. 
Can you detail why the cachemgr_passwd directive/code cannot continue to 
work essentially as it works today after cache_object scheme support is 
removed from Squid?



Blocker #2: The squidclient tool still sends cache_object: scheme when 
given "mgr:" on the CLI. We need to upgrade that first 


Looks like we are in agreement on that.


and allow admin 
some time to upgrade before removing the scheme support in squid itself.


Agreed. Would six months be enough in your opinion? If yes, we may be 
able to remove cache_object support in v6. Otherwise, we can remove 
cache_object support starting with v7 (as far as numbered releases are 
concerned).



cachemgr.cgi should already prefer http(s) and only use cache_object 
as a backup.


IMO the CGI tool should stay that way, supporting the scheme for older 
installations even after we drop it from the rest of Squid.


IMO, we should not keep any code that is only needed for Squid v3.1 and 
earlier. Squid v3.2 and later should http-based cache manager access, 
right? More code always means more maintenance overheads and higher 
change costs. Given our lack of resources, we should start ignoring 
Squid v3 needs.


Moreover, I do not see how we can keep that "backup" code while 
supporting newer Squids and Javascript-disabled browsers at the same 
time: AFAICT, when Javascript is disabled (or not working properly), 
that "only as a backup" code will send cache_object requests to modern 
Squids that will no longer support them...


I think we should upgrade that cachemgr.cgi code rather than preserve it 
for Squid v3 needs. However, if you insist, it will stay simply because 
I do not think cachemgr.cgi is worth our time.



Cheers,

Alex.

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] Drop cache_object protocol support

[Alex Rousskov]

On 1/24/23 12:22, Eduard Bagdasaryan wrote:


Today we can query cache manager in two ways:

1. with cache_object:// URL scheme
2. with an HTTP request having the 'squid-internal-mgr' path prefix.

I guess that when (2) was initially added at e37bd29, its implementation 
was somewhat incomplete compared to the old cache_object scheme (e.g., 
it lacked authentication) and both methods existed. Since then, however, 
(2) has been improved and it should be equivalent to (1) by now.  If so, 
can we completely remove the non-standard cache_object scheme support 
from Squid? This would simplify request forwarding logic, including code 
paths where the existing code complexity may result in vulnerability 
issues.



FWIW, I am not aware of any good reason to keep supporting the 
"cache_object" URI scheme.


MgrFieldChars() already calls that scheme deprecated. That special (and 
undocumented?) scheme did cause significant problems in the past. I am 
sure it will continue to cause problems if not removed. Removing it will 
simplify code in several tricky places. There will be some upgrade pains 
for admins, but we will be better off without cache_object long-term IMO.


Needless to say, squidclient and cachemgr.cgi implementations would need 
to be adjusted to use HTTP URLs instead, but I hope those adjustments 
are straightforward.



HTH,

Alex.

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] Squid 5.6 leaking memory when peeking for an origin with an invalid certificate

[Alex Rousskov]

On 1/18/23 13:46, Hamilton Coutinho wrote:

Hi all,

We are observing what seems to be several objects leaking in the output 
mgr:mem, to the tune of 10s of 1000s 
of HttpRequest, HttpHeaderEntry, Comm::Connection, Security::ErrorDetail, cbdata PeekingPeerConnector (31), etc.


We dumped a core and managed to find some HttpRequest objects and they 
all seem to have failed in the same way, with an ERR_SECURE_CONNECT_FAIL 
category, for a site that has a certificate signed by a CA authority not 
available to squid.


If I would guess, the origin of the problem might be in 
Ssl::PeekingPeerConnector::checkForPeekAndSpliceMatched():


     if (finalAction == Ssl::bumpTerminate) {
         bail(new ErrorState(ERR_SECURE_CONNECT_FAIL, Http::scForbidden, 
request.getRaw(), al));

         clientConn->close();
         clientConn = nullptr;

Wondering if assigning null to clientConn there would be premature.



FWIW, that connection pointer reset itself looks OK to me. ConnStateData 
and/or others should have a connection closure handler attached to the 
clientConn descriptor. That handler should be notified by Comm and 
initiate cleanup of the objects responsible for client-Squid communication.


The bail() call above should inform the requestor about the 
error/termination and terminate this AsyncJob. That requestor should 
then close the Squid-server connection and clean up associated state.


While there may be bugs in those "should..." sequences, please note that 
the pasted code is not related to handling of untrusted origin servers 
(unless your ssl_bump rules specifically activate the terminate action 
upon discovering such an origin server). The pasted code is reacting to 
an "ssl_bump terminate" rule matching.



Cheers,

Alex.

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] Possible bug with file-descriptor parameter in configure of squid-6.0.0-20221210-r71f62e86e

[Alex Rousskov]

On 12/23/22 16:29, infant vinay wrote:

I am trying to compile squid release squid-6.0.0-20221210-r71f62e86e 

I am using the same configure option I have used for at least 2+ years 
now which includes the  --with-filedescriptors=4096 option in it. The 
options are further down below.


Until the most recent version of this release in Sep 2022 I have never 
seen any errors during configure


However in the Dec 2022 release, I see the following error

checking for main in -lmalloc... no
checking for library containing log... -lm
*configure: error: --with-filedescriptors expects a numeric argument*


This is a Squid bug. Please see the following PR for a proposed fix:
https://github.com/squid-cache/squid/pull/1217


HTH,

Alex.



Configure option used is

./configure --prefix=/opt/squid --build=x86_64-pc-linux-gnu 
--host=x86_64-pc-linux-gnu --enable-removal-policies=heap,lru 
--enable-ssl --enable--storeio=aufs,diskd,ufs,rock --enable-diskio 
--with-aio --with-default-user=squid --with-pthreads 
--disable-arch-native --disable-ipv6 --enable-xmalloc-statistics 
--enable-delay-pools --disable-snmp *--with-filedescriptors=4096*


CFLAGS and CXXFLAGS set are

-O3 -march=znver2 -pthread


I have ensured that the ulimit -n for the root user I use for compile is 
set to 8192 and I am not sure what else to do.


squid-6.0.0-20221210-r71f62e86e]# ulimit -n
8192

OS is Fedora Core rawhide (v38) - as I said this has worked until the 
previous squid version released in Sep 2022 on the same OS and same 
hardware.


Attaching the running version of squid info here

  squid-6.0.0-20221210-r71f62e86e]# /opt/squid/sbin/squid -v
Squid Cache: Version 6.0.0-20220905-r9358e99f9
Service Name: squid
configure options:  '--prefix=/opt/squid' '--build=x86_64-pc-linux-gnu' 
'--host=x86_64-pc-linux-gnu' '--enable-removal-policies=heap,lru' 
'--enable-ssl' '--enable--storeio=aufs,diskd,ufs,rock' '--enable-diskio' 
'--with-aio' '--with-default-user=squid' '--with-pthreads' 
'--disable-arch-native' '--disable-ipv6' '--enable-xmalloc-statistics' 
'--with-filedescriptors=4096' '--enable-delay-pools' '--disable-snmp' 
'build_alias=x86_64-pc-linux-gnu' 'host_alias=x86_64-pc-linux-gnu' 
'CFLAGS=-O3 -march=znver2 -pthread' 'CXXFLAGS=-O3 -march=znver2 
-pthread' --enable-ltdl-convenience



I am not sure what else you need. Please do ask if you need. I will 
attach the config.log file here too if that helps you.


Thanks for verifying if this is a bug. If there is an error from my 
side, I am ok to be pointed out by you as to what is wrong.


- Infant V Patrick



___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev


___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] RFC: Reject repeated same-name annotations

[Alex Rousskov]

On 12/15/22 17:27, ngtech1...@gmail.com wrote:


I must admit that I didn't understand it enough to make sense on what
specific scenario for example it will affect.


Here is a configuration example:

  # Let's mark certain allowed transactions as green and hot:
  acl markCertain annotate_transaction color=green t=hot color=grn
  http_access allow certainSlowAclHere markAsGreen

  # And then use 10.0.0.1 for those certain transactions:
  acl markedAsGreen note color green
  tcp_outgoing_address 10.0.0.1 markedAsGreen

The above will not use 10.0.0.1 for transactions that should use that 
outgoing address because somebody accidentally overwritten the color on 
the annotate_transaction line. We are actually coloring matching 
transactions "grn" instead of "green" now, and the above 
tcp_outgoing_address rule does not match.



Such errors a more difficult to spot when the offending acl line is in a 
different configuration file than the correct acl line. They are even 
more difficult to spot when the problem is hidden in a helper response!




I assumed that what would happen is that if ... a single response
will contain multiple notes with the same key these will be
appended.


No, they will not be appended. Or, to be more precise, they may not be 
appended in every case. The actual results vary depending on the 
annotation source, Squid version, etc. What happens is currently an 
undefined behavior.




From what I understood until now a single helper that will respond
with multiple note_1=v note_1=v Will trigger a fatal error


No, bad helper responses will not trigger fatal errors. They will 
trigger non-fatal ERROR messages.


Only misconfigurations (e.g., the above squid.conf example) will be fatal.



However, if multiple helpers will send both each in it's turn a
note_1=v these will be appended.


IIRC, annotations from earlier helper responses will be overwritten by 
annotations in the later ones. However, this RFC is _not_ about multiple 
responses, so let's not lose our focus, even if my recollection is 
wrong. :-)




I agree that the result should be predictable however if logs can
help to trace the issue I believe it's predicted enough to not say
about the current situation "un-predictable".


Currently, ALL,1 cache.log is silent about same-name annotations in many 
cases. Debugging cache.log does not count, of course (and it would be 
rather difficult to triage these problems in a busy debugging cache.log 
anyway). As far as Squid administration is concerned, the biggest 
trouble is not in figuring out where the problem is. It is knowing that 
there is a problem (e.g., that users that should be blocked are actually 
allowed when everybody seems "happy").



Also, the value of this RFC is not just in making Squid safer. It also 
helps with enhancing Squid code, adding features. Right now, if Bob 
tries to add a feature involving annotations, Alice may shoot it down 
because the new code does not handle same-name annotations the same way 
as the code Alice happens to know about (while Bob is copying another 
piece of code that handles similar situation differently or inventing 
his own thing).



We need to fix this mess from both development and administration point 
of view.



Hope this clarifies,

Alex.



-Original Message-----
From: squid-dev  On Behalf Of Alex 
Rousskov
Sent: Thursday, 15 December 2022 23:30
To: Squid Developers 
Subject: [squid-dev] RFC: Reject repeated same-name annotations

Hello,

  I propose to adjust Squid code to reject repeated same-name
annotations from each and every source that supplies annotations:

* "note" directive
* adaptation_meta directive
* annotate_transaction ACL [1]
* annotate_client ACL [1]
* adaptation services responses (eCAP and ICAP)
* helper responses

If this RFC is approved: A configuration that contains a directive with
repeated same-name annotations will be rejected with a fatal ERROR[2]. A
helper or service response that contains repeated same-name annotations
will trigger a non-fatal (to Squid or transaction) cache.log ERROR[2].


Currently, Squid treats repeated same-name annotations inconsistently.
Depending on the annotation source, Squid processing code may

* use the first same-name annotation and ignore repetitions
* use the last same-name annotation and ignore repetitions
* use all same-name annotations, honoring repetitions

These inconsistencies make it difficult to improve/enhance/optimize
Squid code, while Squid ignorance hides misconfigurations and
helper/service implementation bugs, including problems that may be
related to access controls and other sensitive matters.


Any objections or better ideas?


Thank you,

Alex.

[1] In this context, we are talking about same-name annotations
mentioned in the corresponding ACL _configuration_ (i.e. all "acl"
directives with a given ACL name). A repeated _computation_ of
annotate_foo ACL will continue to dea

[squid-dev] RFC: Reject repeated same-name annotations

[Alex Rousskov]

Hello,

I propose to adjust Squid code to reject repeated same-name 
annotations from each and every source that supplies annotations:


* "note" directive
* adaptation_meta directive
* annotate_transaction ACL [1]
* annotate_client ACL [1]
* adaptation services responses (eCAP and ICAP)
* helper responses

If this RFC is approved: A configuration that contains a directive with 
repeated same-name annotations will be rejected with a fatal ERROR[2]. A 
helper or service response that contains repeated same-name annotations 
will trigger a non-fatal (to Squid or transaction) cache.log ERROR[2].



Currently, Squid treats repeated same-name annotations inconsistently. 
Depending on the annotation source, Squid processing code may


* use the first same-name annotation and ignore repetitions
* use the last same-name annotation and ignore repetitions
* use all same-name annotations, honoring repetitions

These inconsistencies make it difficult to improve/enhance/optimize 
Squid code, while Squid ignorance hides misconfigurations and 
helper/service implementation bugs, including problems that may be 
related to access controls and other sensitive matters.



Any objections or better ideas?


Thank you,

Alex.

[1] In this context, we are talking about same-name annotations 
mentioned in the corresponding ACL _configuration_ (i.e. all "acl" 
directives with a given ACL name). A repeated _computation_ of 
annotate_foo ACL will continue to deal with same-name annotations as 
documented -- a "name+=value" configuration will continue to append 
values to the existing same-name annotation, while a "name=value" 
configuration will continue to overwrite any existing same-name annotation.


[2] Repeated same-name annotations that all have identical _values_ will 
be flagged with a WARNING instead. Some overly simplistic configuration 
generators, complicated configurations build from many include files, 
and dumb helpers/services might generate repeated same-everything 
annotations. Since such repetitions can be _safely_ ignored (honoring 
just one name=value pair among all the identical ones), we do not have 
to reject the configuration or log an ERROR because of them.

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] RFC: Switch to C++17

[Alex Rousskov]

On 12/5/22 06:18, Amos Jeffries wrote:

I support the switch.


Great, I will start working on a PR. If anybody reading this will be 
seriously inconvenienced by future Squid v6 requiring C++17, please 
speak up!




Caveat details below...



On Sun, 4 Dec 2022 at 16:18, Alex Rousskov wrote:
    * GCC v5 supports most C++17 features.
    * GCC v8 supports all relevant C++17 features.
    * Clang v5 supports nearly all C++17 features.
    * Clang v8 supports all relevant C++17 features.



non-RHEL distros shipping Squid are almost all providing at lease GCC 
v8. In many cases even their recently refreshed (Nov 2022) LTS have it.


The RHEL family distros (CentOS 7 in particular, but also many others) 
LTS versions only provide LLVM/clang v7 from the C++17 compilers.



I believe the key here is not what distros ship by default, but how 
difficult it is, for an old distro user, to get the compiler Squid 
needs. For example, AFAICT, RHEL users are supposed to use Red Hat 
Developer Toolset or GCC Toolset to get modern compilers.



It has been a while since I had any info about the RedHat developer 
toolset. Do you know if GCC 8+ or LLVM/clang 8+ are in there? if so, 
then bump to v8 for both compilers and full speed ahead :-)


AFAICT[1], Red Hat Developer Toolset "delivers the latest stable GCC 
version" for RHEL7. For example, Toolset v10 has GCC v10[2].


RHEL8 has GCC Toolset with characteristics similar to Red Hat Developer 
Toolset for RHEL7. For example, GCC Toolset v10 provides GCC v10[3], 
while GCC Toolset v12 provides GCC v12[4].


[1] https://developers.redhat.com/products/developertoolset/overview

[2] 
https://access.redhat.com/documentation/en-us/red_hat_developer_toolset/10/html/user_guide/chap-red_hat_developer_toolset#sect-Red_Hat_Developer_Toolset-About


[3] 
https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/8/html/developing_c_and_cpp_applications_in_rhel_8/additional-toolsets-for-development_developing-applications#tools-and-versions-provided-by-gcc-toolset-10_gcc-toolset-10


[4] 
https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/8/html/developing_c_and_cpp_applications_in_rhel_8/additional-toolsets-for-development_developing-applications#tools-and-versions-provided-by-gcc-toolset-12_gcc-toolset-12



Thank you,

Alex.

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

[squid-dev] RFC: Switch to C++17

[Alex Rousskov]

Hello,

I propose that we switch master/v6 from C++11 to C++17: Modern 
environments support C++17 well. We are wasting significant amounts of 
time on emulating such basic C++17 features as std::optional. We are 
writing worse code than we can because we lack access to such basic 
C++14 and C++17 features as


* function return type deduction (auto)
* generic lambdas (auto arguments)
* relaxed constexpr restrictions
* std::make_unique
* std::integer_sequence
* std::quoted
* Nested namespace definitions
* [[fallthrough]], [[maybe_unused]], and [[nodiscard]]
* fold expressions
* auto [a, b] = getTwoReturnValues();
* inline variables
* std::any
* std::variant
* std::byte

If we do not switch now, then we would have to wait about a year for the 
next such opportunity because we should not introduce such a big 
difference between master and the upcoming unstable v6 branch.



C++17 is supported by popular modern compilers and stable distros. Squid 
master branch should target those IMO. Even old environments can install 
the necessary modern compilers (e.g., RHEL5 users can get them via Red 
Hat Developer Toolset).


* GCC v5 supports most C++17 features.
* GCC v8 supports all relevant C++17 features.
* Clang v5 supports nearly all C++17 features.
* Clang v8 supports all relevant C++17 features.

* Ubuntu 20.04 LTS ships with GCC v9 and clang v10.

* https://gcc.gnu.org/projects/cxx-status.html#cxx17
* 
https://gcc.gnu.org/onlinedocs/libstdc++/manual/status.html#status.iso.2017

* https://clang.llvm.org/cxx_status.html#cxx17
* https://en.cppreference.com/w/cpp/compiler_support/17


Switching to just C++14 would be better than nothing, but it will not 
give us several C++17 features that we already waste serious time on 
emulating/avoiding (e.g., std::optional). We should not switch to C++20 
yet because modern stable compilers still have some C++20 support holes.



I can volunteer the corresponding PR.


Any objections to switching master/v6 to C++17?


Thank you,

Alex.
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] RFC: Semaphore CI to GitHub Actions migration

[Alex Rousskov]

On 10/22/22 04:17, Amos Jeffries wrote:

On 20/10/22 03:25, Alex Rousskov wrote: >> 3. Build tests: Semaphore CI uses 
Ubuntu 14.04. GitHub Actions uses
Ubuntu 22.04. Semaphore CI has fewer build dependencies installed. 
GitHub Actions do not provide Ubuntu 14.04 runners[1].


Plan: I will keep Semaphore CI build tests and make the GitHub Actions 
tests required. When Semaphore CI build tests start failing (e.g., 
because dependency repositories stop working "as is"), or when we stop 
supporting that old environment, I will disable those tests.



FYI the "start failing" condition was met some time ago.


I disagree: Sempahore CI build tests are clearly not failing now.


Ubuntu 14.04 provides GCC 4.8.6 which has a broken/incomplete STL 
prohibiting use of several important C++11 features (at least 
std::chrono, maybe also std::regex, and static analysis).


Squid unconditionally uses std::chrono already. When we start using 
std::regex, we may need to stop using Sempahore CI build tests (if they 
are still running at that time and start failing due to std::regex 
addition), but that likely future is compatible with the above plan.



Previous discussions have focused on CentOS 7 which requires the GCC 
4.8.5 and has (had?) a more difficult upgrade path than Ubuntu.


Sempahore CI build tests should not determine the set of language 
features Squid uses. They have ever determined that IMO. They certainly 
will not determine that going forward since we all agree that we can 
stop running them when they start failing (due to the age of Semaphore 
CI environment).



Cheers,

Alex.
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

[squid-dev] RFC: Semaphore CI to GitHub Actions migration

[Alex Rousskov]

Hello,

I plan to gradually turn Semaphore CI testing off and make GitHub 
Actions required. We should not babysit the same tests in two setups. 
Here is the current status of CI tests with regard to Semaphore and 
GitHub Actions together with the corresponding planned actions:


1. Functionality tests: Essentially the same set of tests. Semaphore CI 
has one extra/old test but it is disabled for master/v6 code. The 
proxy-collapsed-forwarding test often fails on Semaphore, requiring a 
manual restart of all tests. The busy-restart test usually fails on 
GitHub Actions, but those failures are currently ignored.


Plan: I will leave the busy-restart test running on Semaphore CI until 
we find a way to make it stable in GitHub Actions environment. I will 
turn off the other Semaphore CI functionality tests and make the GitHub 
Actions ones required.



2. Source maintenance tests: The same set of tests. GitHub Actions have 
the right Astyle version, so the formatting test actually works on 
GitHub (but its results are currently ignored on both platforms).


Plan: I will turn off Semaphore CI source maintenance tests and make the 
GitHub Actions ones required instead. Formatting test results will still 
be ignored (that is a separate decision/change/action out of this thread 
scope; let's not discuss it here).



3. Build tests: Semaphore CI uses Ubuntu 14.04. GitHub Actions uses 
Ubuntu 22.04. Semaphore CI has fewer build dependencies installed. 
GitHub Actions do not provide Ubuntu 14.04 runners[1].


Plan: I will keep Semaphore CI build tests and make the GitHub Actions 
tests required. When Semaphore CI build tests start failing (e.g., 
because dependency repositories stop working "as is"), or when we stop 
supporting that old environment, I will disable those tests.



If you have any objections or better ideas about gradually moving away 
from Semaphore CI, please discuss.



Thank you,

Alex.

[1]  GitHub provides Ubuntu 18.04 runners, but they are deprecated, will 
purposefully _fail_ according to GitHub schedule, and will be removed in 
December. We should not use them. Details at 
https://github.blog/changelog/2022-08-09-github-actions-the-ubuntu-18-04-actions-runner-image-is-being-deprecated-and-will-be-removed-by-12-1-22/

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] security_file_certgen protocol

[Alex Rousskov]

On 9/22/22 10:03, ngtech1...@gmail.com wrote:


I am trying to write a service like security_file_certgen as a daemon that will 
be communicated  via a TCP or UNIX Socket.
However, it’s a bit hard for me now to grasp the STDIN/STDOUT protocol of 
security_file_certgen.
I remember vaguely that it involves reading from some string (else then new 
lines) to another and then sends back
to stdout a certificate string.

So what are the parts of the request object and what are the parts of the 
response object?
If I will grasp it I will be able to model it in a single ruby script.

I know this is not the first time I am asking about this and it’s harder for me 
that I forget such simple things.
I will be thankful for any help with this.


The basic protocol syntax is documented at 
https://wiki.squid-cache.org/Features/AddonHelpers#SSL_certificate_generation


Beyond that, there is source code and actual traffic that you can 
analyze, of course, but there is no comprehensive documentation AFAICT.


Please note that Squid workers already communicate with these helpers 
via TCP or UNIX sockets. The helpers just do not know that because a 
forked intermediary process remaps those sockets to helper stdin/stdout 
descriptors. See `git grep -1 define.IPC_STREAM` and dup2() in ipcCreate().



HTH,

Alex.
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] Proposal: switch to always-build for some currently optional features

[Alex Rousskov]

On 9/20/22 02:34, Francesco Chemolli wrote:


I agree that modules that can always be built, should be. Such modules
should have no guarding #ifdefs. I think this is the set of modules
that
your proposal is targeting, but please correct me if I am wrong. FWIW,
this design stems from an even more general/fundamental rule of thumb:
Do not add unnecessary #ifdefs.


I agree. We could also work on better isolation, by restricting 
#ifdef-guarded areas
to specific delegate classes, and then using c++ features (e.g. if 
constexpr, eventually) to

disable the callsites.


Yes, of course, but I would start with modules/features that require no 
significant refactoring. I am not sure what the best candidates are, but 
I would evaluate ident, cache digests, htcp, and wccp first (these are 
from your own list of candidates).


For example, a feature like unlinkd (also on your list) would require 
adding a default-disabled configuration option (unlinkd_enable, similar 
to pinger_enable) or introducing support for a special program location 
spelling like "none". Adding either would break existing configurations 
that willingly run unlinkd. There are probably a few features/modules 
that do _not_ require such disruptive configuration changes. I would 
start with those.




However, there is a precondition: Any always-built optional feature
with
a potentially significant performance impact or a controversial side
effect should be disabled by default (via squid.conf). Satisfying this
precondition will require code changes.



Yes, I agree.


Glad we are on the same page!

I recommend giving Amos more time to leave feedback before spending time 
on code changes, but if we are all in agreement, then I am looking 
forward to PRs reducing the number of unnecessary #ifdefs. To minimize 
overheads, please use one PR per module/feature and avoid opening 
multiple concurrent PRs (at least until it is clear that we are on the 
same page regarding the overall approach to the corresponding code 
modifications).



Thank you,

Alex.
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] Proposal: switch to always-build for some currently optional features

[Alex Rousskov]

On 9/19/22 09:28, Francesco Chemolli wrote:

there is a bunch of features that are currently gated at compile 
time: among others, I see:



- adaptation (icap, ecap)
- authentication
- ident
- delay pools
- cache digests
- htcp
- cache digests
- wccp
- unlinkd

I'd like to propose that we switch to always-build them.



We would gain:



- code clarity

> - ease of development

The above two items do not fully apply to features that depend on 
external libraries (which may be absent): eCAP, some authentication 
modules, and possibly others. Their code and related development 
overheads will remain largely unchanged. I suspect that you actually did 
not want to include optional modules with external dependencies in your 
proposal, but please clarify.




- test coverage


To be more precise, we would gain reduction of feature _combinations_ 
that should be tested (which is a significant gain!). Basic code 
coverage by tests would remain unchanged because nearly any test can 
enable (and test) all features that can be built.




- feature uniformity across builds


Yes, fewer features that can be enabled/disabled at build time helps 
with support.




We would lose:
- slightly longer build time
- larger binaries


And:

- Larger attack surface if we always build modules like ESI. This can be 
partially mitigated by making sure we default-disable them. This is one 
of the reasons for the precondition at the end of my email.


- Some loss of performance. For example, the cache digests module, when 
enabled, builds cache digests by default (from squid.conf point of 
view). Similarly, ESI parses applicable content. There are probably also 
non-trivial slow ACL-driven checks that a module may bring in by default 
(from squid.conf point of view) if enabled.




Opinions?


I agree that modules that can always be built, should be. Such modules 
should have no guarding #ifdefs. I think this is the set of modules that 
your proposal is targeting, but please correct me if I am wrong. FWIW, 
this design stems from an even more general/fundamental rule of thumb: 
Do not add unnecessary #ifdefs.


However, there is a precondition: Any always-built optional feature with 
a potentially significant performance impact or a controversial side 
effect should be disabled by default (via squid.conf). Satisfying this 
precondition will require code changes.



Cheers,

Alex.
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] RFC submodule repositories

[Alex Rousskov]

On 8/1/22 22:32, Amos Jeffries wrote:

On 1/08/22 03:09, Alex Rousskov wrote:

On 7/31/22 00:29, Amos Jeffries wrote:
When PR #937 merges we will have the ability to shuffle old helpers 
into a separate repository that users can import as a submodule to 
build with their Squid as-needed.



What will we optimize by introducing such a significant
complication as git submodules?



( So every change to Squid has to be justified as an optimization now. 
Right... )


The verb "optimize" is generally defined as "to make the best or most 
efficient use". In computing, that definition is relaxed to mean any 
improvement (not necessarily the "best"). Both definitions would work in 
this context, but I used "optimize" in the latter ("to improve") sense. 
I hope we can all agree that Squid changes should improve Squid.



We "optimize" future code maintenance workload with the ability to drop 
outdated helpers which are still required by a small group of users but 
no longer want to be actively developed by the core dev team.


N.B. Unless something else changes, simply moving "dropped" helper code 
to a submodule would require reviewing dropped helper update PRs in an 
aggregate "take it or leave it" mode. No dropped helper code change 
requests would be appropriate for such "updated dropped helper H to 
version V" pull requests. I do not know whether that will improve core 
developer lives, but it is likely to require making more judgement calls 
than we have to make today, and jusdgement calls can be especially 
controversial/painful.


How would moving "dropped" helper code to a submodule help core 
developers? AFAICT, just moving a dropped helper into a submodule 
creates more work for core developers because they now have one more 
thing to worry about. Git submodules (and git in general!) have very 
little impact on making "./bootstrap.sh && ./configure && make && sudo 
make install" produce the desired outcome -- with or without submodules, 
we still need to make the whole thing build and work correctly. Thus, 
any improvements must be rooted in some other, yet unspecified Project 
changes besides moving some helper code into submodules. What other 
changes will make that extra core developer work worth it?


In other words, it is possible that the RFC associates "git submodules" 
with some unspecified changes outside the Squid git repository 
structure[1] that RFC readers cannot guess. Identifying any important 
changes the RFC associates with git submodules may help us make progress.


[1] For example, one common submodule use case is having different 
users/permissions for different parts of the Project -- essentially 
having multiple core developer teams with varying levels of access to 
critical Project resources, but all collaborating under one Project 
roof. It is often impractical to achieve that kind of access controls 
within one git/GitHub repository. I am not saying this example applies 
to this RFC -- I am just illustrating how submodules may facilitate 
important changes outside of low-level git mechanisms themselves.



Case in point being CERN who still require our bundled LanManager 
helper(s) for some internal needs. That requires a lot of deprecated and 
rarely touched code being maintained for only one (albeit important) 
user case.
  That code could all be shuffled to a separate repository outside the 
official Squid release, but maintained by developers that support CERN 
needs.


Sure, but we clearly do not need git submodules to "shuffle LanManager
helper(s) code to a separate repository". Why submodules? The RFC 
implies, without elaborating, that submodules are the right solution to 
some problem. Several RFC reviewers correctly point out that submodules 
are associated with significant "cons". I am asking what the "pros" are. 
It is a very reasonable request IMO.


Agreeing on the _problem_ definition may also help shorten the number of 
iterations required to reach consensus. What do we really mean when we 
say that "dropped" helpers are "not actively developed by core 
developers"? To me, the lack of core developer involvement implies that 
PRs against Squid master may break "dropped" helpers and that "dropped" 
helper needs are assigned much lower weight compared to "native" Squid 
improvement needs, but others are likely to interpret that phrase 
differently.



What (if any) updates do we need to make to Anubis and other 
infrastructure so support git submodules ?


That answer probably depends, in part, on what support guarantees we 
are going to issue for those submodules and on what our submodule 
update policy will be.


IMO we should maintain at least one helper officially as a submodule to 
ensure the git submodule mechanisms remain viable for d

Re: [squid-dev] RFC submodule repositories

[Alex Rousskov]

On 7/31/22 00:29, Amos Jeffries wrote:
When PR #937 merges we will have the ability to shuffle old helpers into 
a separate repository that users can import as a submodule to build with 
their Squid as-needed.


In my experience, git submodules are a powerful feature that is rather 
difficult to use correctly. Some of submodule aspects do not feel 
"natural" to many humans. I am pretty sure that proper introduction of 
submodules will require a lot of our time and nerve cells. What will we 
optimize by introducing such a significant complication as git submodules?



What (if any) updates do we need to make to Anubis and other 
infrastructure so support git submodules ?


That answer probably depends, in part, on what support guarantees we are 
going to issue for those submodules and on what our submodule update 
policy will be. Integrating two or more git repositories together is a 
complicated issue...


Alex.
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

[squid-dev] RFC: Class section/member _order_

[Alex Rousskov]

Hello,

Amos and I disagreed[1] regarding the existing guidelines for 
section/member order in C++ class declarations. To resolve that 
disagreement, this email proposes the order for future use.


-

1. Class "sections" order (by member access specifiers): public, 
protected, private. Each section, if present, declared once. Omit 
sections that would be empty. Rationale: List most commonly reused, most 
important things fist. In this context, those are public class 
interfaces. The private section is the least important implementation 
detail as far as the notion of a C++ class is concerned.



2. Within each section, the recommended member order is defined below. 
Rationale: Group similar things together to facilitate searching and 
highlight differences. List things most likely to be reused (by other 
class members) and most important/influential things higher.


* friendship declarations
* type and aliases declarations; nested classes/structs/unions
* static member functions
* constructors and assignment operators
* destructors (just one until C++20)
* other/regular non-static member functions except overrides
* overrides (see item 3 below)
* static data members
* non-static data members


3. Overrides are a special case where we do not expect member 
descriptions but do expect a reference to the corresponding API as 
sketched below. Overrides are grouped by the (direct or indirect) parent 
that _introduced_ the corresponding API method(s) (i.e. the parent class 
that declared the virtual method but could _not_ use an override keyword 
in that declaration). Rationale: Provide API context and facilitate 
searching for member descriptions without chasing overrides through parents.


/* Baz API */
overrides for Baz-introduced methods (excluding destructors)

/* Bar API */
overrides for Bar-introduced methods (excluding destructors)


4. Caveats

The above rules are not meant to force authors to include any access 
specifiers or members that the code does not actually need (except the 
"private" specifier should be mentioned explicitly in class declarations 
that have only private members -- do not rely on the class default 
access specifier being "private").


Squid has some legacy code that forces CBDATA-related declarations to be 
hoisted to the very top of the class, into the "unnamed" section. This 
is an exception to the above rules. Eventually, we will stop doing that, 
but we should continue doing that for consistency sake until the 
offending CBDATA macros are gone.


Like any style rules, these rules are not comprehensive. If your use 
case is not explicitly covered, then look around for similar Squid code 
and try to be consistent and/or reasonable.


-

Most of the above rules are arbitrary but common in the industry[2,3].

[1] https://github.com/squid-cache/squid/pull/1067#discussion_r889864925
[2] https://stackoverflow.com/q/308581
[3] https://google.github.io/styleguide/cppguide.html#Declaration_Order


HTH,

Alex.
P.S. This is my Nth attempt to get through the mailing list filter. You 
may have received a pretty much the same message as an attachment to the 
previous mailing list post. Sorry about the noise!

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

[squid-dev] RFC: Class section/member order

[Alex Rousskov]

Spam detection software, running on the system "master.squid-cache.org",
has identified this incoming email as possible spam.  The original
message has been attached to this so you can view it or label
similar future email.  If you have any questions, see
the administrator of that system for details.

Content preview:  Hello, Amos and I disagreed[1] regarding the existing 
guidelines
   for section/member order in C++ class declarations. To resolve that 
disagreement,
   this email proposes the order for future use. - 

Content analysis details:   (105.0 points, 5.0 required)

 pts rule name  description
 -- --
 105 SQUID_ML_SPAM_01   No description available.
 0.0 UNPARSEABLE_RELAY  Informational: message has unparseable relay
lines
-0.0 T_SCC_BODY_TEXT_LINE   No description available.


--- Begin Message ---

Hello,

Amos and I disagreed[1] regarding the existing guidelines for 
section/member order in C++ class declarations. To resolve that 
disagreement, this email proposes the order for future use.


-

1. Class "sections" order (by member access specifiers): public, 
protected, private. Each section, if present, declared once. Omit 
sections that would be empty. Rationale: List most commonly reused, most 
important things fist. In this context, those are public class 
interfaces. The private section is the least important implementation 
detail as far as the notion of a C++ class is concerned.



2. Within each section, the recommended member order is defined below. 
Rationale: Group similar things together to facilitate searching and 
highlight differences. List things most likely to be reused (by other 
class members) and most important/influential things higher.


* friendship declarations
* type and aliases declarations; nested classes/structs/unions
* static member functions
* constructors and assignment operators
* destructors (just one until C++20)
* other/regular non-static member functions except overrides
* overrides (see item 3 below)
* static data members
* non-static data members


3. Overrides are a special case where we do not expect member 
descriptions but do expect a reference to the corresponding API as 
sketched below. Overrides are grouped by the (direct or indirect) parent 
that _introduced_ the corresponding API method(s) (i.e. the parent class 
that declared the virtual method but could _not_ use an override keyword 
in that declaration). Rationale: Provide API context and facilitate 
searching for member descriptions without chasing overrides through parents.


/* Baz API */
overrides for Baz-introduced methods (excluding destructors)

/* Bar API */
overrides for Bar-introduced methods (excluding destructors)


4. Caveats

The above rules are not meant to force authors to include any access 
specifiers or members that the code does not actually need (except the 
"private" specifier should be mentioned explicitly in class declarations 
that have only private members -- do not rely on the class default 
access specifier being "private").


Squid has some legacy code that forces CBDATA-related declarations to be 
hoisted to the very top of the class, into the "unnamed" section. This 
is an exception to the above rules. Eventually, we will stop doing that, 
but we should continue doing that for consistency sake until the 
offending CBDATA macros are gone.


Like any style rules, these rules are not comprehensive. If your use 
case is not explicitly covered, then look around for similar Squid code 
and try to be consistent and/or reasonable.


-

Most of the above rules are arbitrary but common in the industry[2,3].

[1] https://github.com/squid-cache/squid/pull/1067#discussion_r889864925
[2] https://stackoverflow.com/q/308581
[3] https://google.github.io/styleguide/cppguide.html#Declaration_Order


HTH,

Alex.
--- End Message ---
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] PR backlog

[Alex Rousskov]

On 6/7/22 17:53, Francesco Chemolli wrote:

This is exactly what I meant and what I see happening.
I'd like us to concentrate on clearing as much as possible the PR 
backlog to capitalise on the work many have put in there, prioritising 
that over new feature work for a few days.


I am not sure, but I sense that you are not satisfied with the list of 
the PRs that I said I am working on. If that is the case, you may want 
to clarify what you think I should be doing instead. FWIW, I thought 
that working on the listed PRs will reduce the number of PRs and, hence, 
reduce "backlog of open PRs" which, I thought, was what your 15-day 
sprint suggestion was about...



> Could we prioritise reviewing and merging PR#694?

As you know, that is one of the PRs on top of my list. I do not think I 
can "prioritize" it any more than I already am. I even offered to 
implement some of the required changes! Again, if you think I should be 
doing something else instead, please discuss!



Thank you,

Alex.



On Mon, Jun 6, 2022 at 6:20 PM Alex Rousskov wrote:

On 6/6/22 03:34, Francesco Chemolli wrote:

 >     we have quite a big backlog of open PRs
 >
(https://github.com/squid-cache/squid/pulls?page=1=is%3Apr+is%3Aopen
<https://github.com/squid-cache/squid/pulls?page=1=is%3Apr+is%3Aopen>).
How
 > about doing a 15-days sprint and clearing it or at least trimming it
 > significantly?

I am happy to participate in any way you find useful! Please let me
know
how I can help.

FWIW, I know of a handful of PRs that need my attention (e.g., #1067,
#980, #694, #755, #736). I am already working on some of them (and will
resume working on others ASAP).

Alex.
___
squid-dev mailing list
squid-dev@lists.squid-cache.org <mailto:squid-dev@lists.squid-cache.org>
http://lists.squid-cache.org/listinfo/squid-dev
<http://lists.squid-cache.org/listinfo/squid-dev>



--
     Francesco


___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] PR backlog

[Alex Rousskov]

On 6/6/22 03:34, Francesco Chemolli wrote:

    we have quite a big backlog of open PRs 
(https://github.com/squid-cache/squid/pulls?page=1=is%3Apr+is%3Aopen). How 
about doing a 15-days sprint and clearing it or at least trimming it 
significantly?


I am happy to participate in any way you find useful! Please let me know 
how I can help.


FWIW, I know of a handful of PRs that need my attention (e.g., #1067, 
#980, #694, #755, #736). I am already working on some of them (and will 
resume working on others ASAP).


Alex.
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] [squid-users] squid-6.0.0-20220412-rb706999c1 cannot be built

[Alex Rousskov]

On 5/1/22 18:27, Eliezer Croitoru wrote:


From my tests this issue with the latest daily autogenerated sources package

is the same:
http://master.squid-cache.org/Versions/v6/squid-6.0.0-20220501-re899e0c27.ta
r.bz2
AclRegs.cc:165:50: error: unused parameter 'name' [-Werror=unused-parameter]
RegisterMaker("clientside_mark", [](TypeName name)->ACL* { return new
Acl::ConnMark; });


Please test https://github.com/squid-cache/squid/pull/1042


Thank you,

Alex.



  ~^~~~
AclRegs.cc: In lambda function:
AclRegs.cc:166:57: error: unused parameter 'name' [-Werror=unused-parameter]
  RegisterMaker("client_connection_mark", [](TypeName name)->ACL* {
return new Acl::ConnMark; });
~^~~~
g++ -DHAVE_CONFIG_H -DDEFAULT_CONFIG_FILE=\"/etc/squid/squid.conf\"
-DDEFAULT_SQUID_DATA_DIR=\"/usr/share/squid\"
-DDEFAULT_SQUID_CONFIG_DIR=\"/etc/squid\"   -I.. -I../include -I../lib
-I../src -I../include
-I../libltdl -I../src -I../libltdl  -I/usr/include/libxml2  -Wextra
-Wno-unused-private-field -Wimplicit-fallthrough=2 -Wpointer-arith
-Wwrite-strings -Wcomments -Wshadow -Wmissing-declarations
-Woverloaded-virtual -Werror -pipe -D_REENTRANT -I/usr/include/libxml2
-I/usr/include/p11-kit-1   -O2 -g -pipe -Wall -Werror=format-security
-Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -fexceptions
-fstack-protector-strong -grecord-gcc-switches
-specs=/usr/lib/rpm/redhat/redhat-hardened-cc1
-specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -mtune=generic
-fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection -fPIC
-c -o DelayConfig.o DelayConfig.cc
g++ -DHAVE_CONFIG_H -DDEFAULT_CONFIG_FILE=\"/etc/squid/squid.conf\"
-DDEFAULT_SQUID_DATA_DIR=\"/usr/share/squid\"
-DDEFAULT_SQUID_CONFIG_DIR=\"/etc/squid\"   -I.. -I../include -I../lib
-I../src -I../include   -I../libltdl -I../src -I../libltdl
-I/usr/include/libxml2  -Wextra -Wno-unused-private-field
-Wimplicit-fallthrough=2 -Wpointer-arith -Wwrite-strings -Wcomments -Wshadow
-Wmissing-declarations -Woverloaded-virtual -Werror -pipe -D_REENTRANT
-I/usr/include/libxml2  -I/usr/include/p11-kit-1   -O2 -g -pipe -Wall
-Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS
-fexceptions -fstack-protector-strong -grecord-gcc-switches
-specs=/usr/lib/rpm/redhat/redhat-hardened-cc1
-specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -mtune=generic
-fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection -fPIC
-c -o DelayPool.o DelayPool.cc
g++ -DHAVE_CONFIG_H -DDEFAULT_CONFIG_FILE=\"/etc/squid/squid.conf\"
-DDEFAULT_SQUID_DATA_DIR=\"/usr/share/squid\"
-DDEFAULT_SQUID_CONFIG_DIR=\"/etc/squid\"   -I.. -I../include -I../lib
-I../src -I../include   -I../libltdl -I../src -I../libltdl
-I/usr/include/libxml2  -Wextra -Wno-unused-private-field
-Wimplicit-fallthrough=2 -Wpointer-arith -Wwrite-strings -Wcomments -Wshadow
-Wmissing-declarations -Woverloaded-virtual -Werror -pipe -D_REENTRANT
-I/usr/include/libxml2  -I/usr/include/p11-kit-1   -O2 -g -pipe -Wall
-Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS
-fexceptions -fstack-protector-strong -grecord-gcc-switches
-specs=/usr/lib/rpm/redhat/redhat-hardened-cc1
-specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -mtune=generic
-fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection -fPIC
-c -o DelaySpec.o DelaySpec.cc
At global scope:
cc1plus: error: unrecognized command line option '-Wno-unused-private-field'
[-Werror]
cc1plus: all warnings being treated as errors

## END

I will try to publish my podman build later on.


Eliezer Croitoru
NgTech, Tech Support
Mobile: +972-5-28704261
Email: ngtech1...@gmail.com

-Original Message-
From: squid-users  On Behalf Of
Amos Jeffries
Sent: Monday, May 2, 2022 00:32
To: squid-us...@lists.squid-cache.org
Subject: Re: [squid-users] squid-6.0.0-20220412-rb706999c1 cannot be built

On 2/05/22 07:55, Eliezer Croitoru wrote:

I have tried to build couple RPMs for the V6 beta but found that the
current daily autogenerated releases cannot be built.

Is there any specific git commit I should try to use?



There is a new daily tarball out now. can you try wit that one please?


Also, squid-dev for beta and experimental code issues.


Cheers
Amos
___
squid-users mailing list
squid-us...@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-users

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev


___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] Can't build on Ubuntu Jammy

[Alex Rousskov]

On 4/23/22 10:38, Francesco Chemolli wrote:

Hi all,
   I did a trial build run on Jammy 
(https://build.squid-cache.org/job/anybranch-anyarch-matrix/COMPILER=gcc,OS=ubuntu-jammy,label=amd64-linux/5/console 
); 
it fails with


../../../../src/security/forward.h: In function 'void 
Security::DH_free_cpp(DH*)':
../../../../src/security/LockingPointer.h:34:21: error: 'void DH_free(DH*)' is 
deprecated: Since OpenSSL 3.0 [-Werror=deprecated-declarations]
34 | function(a); \



Squid does not support OpenSSL v3 yet:
https://github.com/squid-cache/squid/pull/694

Alex.

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] Squid-Cache statistics reporting project

[Alex Rousskov]

On 4/20/22 18:34, Eliezer Croitoru wrote:

In the past I wrote about a project that will include Squid statistics 
reporting.


The main goal is to gather from the project users using a script a set 
of cache-mgr pages in specific intervals.


The simplest way to do so is to run a script that will use either a 
token and will upload the files to an api/webdav/sftp or via email and a 
whitelist of emails.


I would like to RFC this specific idea.


Just to avoid misunderstanding: If "this idea" refers to offering your 
script to Squid users that want to participate in your project, then you 
do not need a squid-dev blessing for doing that because that idea does 
not require any Squid modifications.


If you are proposing Squid modifications, then please detail those 
modifications. I hope it does not come to Squid duplicating crontab and 
sendmail functionality :-).



I can offer to write a cache-mgr to yaml/json converter script that will 
create a singe json/yaml file that will contain all the details of the 
instance.


As a personal project, that converter sounds good to me! FWIW, I have 
heard of 3rd party tools[1] that parse Squid cache manager output, but I 
do not know how suitable they are for what you want to achieve. The best 
output format would depend on how you plan to post-process data, but 
once you get something that follows strict grammar, it should be fairly 
easy to convert to other formats as needed. I would just keep the 
converter script output as simple and strict as possible to facilitate 
further conversions and input in various post-processing tools.


[1] https://github.com/boynux/squid-exporter

As an official Squid project, I think it would be much better to finish 
converting cache manager code to produce YAML output natively than to 
develop and maintain an official converter script (while still working 
on that native YAML conversion).



This option will significantly help the project to grasp a little bit 
about the usage of squid around the world and to get a glimpse into the 
unknown.


Personally, I am worried that glimpses based on a few 
volunteer-submitted samples are more likely to mislead than to correctly 
guide Squid development, but that speculation cannot be proven.



Cheers,

Alex.
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] ERR_CONFLICT_HOST for HTTP CONNECT request on port 80

[Alex Rousskov]

On 3/4/22 03:25, YFone Ling wrote:

I am here just try to understand how the squid determines host conflicts 
for a simple http connect proxy request?


The complete answer to your question is large/complicated and 
Squid-version dependent, but, AFAICT, there are no conflicts in the 
simple CONNECT request you have shared. Either the Squid in question is 
buggy or something else is going on (that is not visible in the output 
you have shared).


Are you absolutely sure the CONNECT request looks exactly like the one 
you have copy-pasted? How do you observe that CONNECT request?


Can you reproduce this exact problem using, say, "nc" or "telnet" as a 
proxy client (no TLS)?


Normally, proxies that accept CONNECT requests do not listen on or 
intercept port 80. Normally, CONNECT requests do not target port 80 
either. Are you sure you are supposed to send a CONNECT request to port 
80 and target an origin server port 80?



What do the WiFi providers tell you when you complain to _them_? Can 
they get you in touch with the technical people responsible for their 
Squids?


Alex.




On Thu, Mar 3, 2022 at 6:28 PM Eliezer Croitoru > wrote:


I am not sure if it’s for Squid-dev but anyway to clear out the
doubts I would suggest attaching the squid.conf
and remember to remove any sensitive data.

__ __

Eliezer

__ __



Eliezer Croitoru

NgTech, Tech Support

Mobile: +972-5-28704261

Email: ngtech1...@gmail.com 

__ __

*From:* squid-dev mailto:squid-dev-boun...@lists.squid-cache.org>> *On Behalf Of
*YFone Ling
*Sent:* Thursday, March 3, 2022 22:55
*To:* squid-dev@lists.squid-cache.org

*Subject:* [squid-dev] ERR_CONFLICT_HOST for HTTP CONNECT request on
port 80

__ __

My application sends  HTTP CONNECT requests to a HTTP proxy port 80,
but gets a squid ERR_CONFLICT_HOST error page.

__ __

Is the following code really working as the comments pointed out
"ignore them" since the following if condition is
"http->request->method != Http::METHOD_CONNECT"

and the rest has been blocked by error page
"repContext->setReplyToError(ERR_CONFLICT_HOST, Http::scConflict,"?

__ __

Does "ignore them" mean block them? 

void



ClientRequestContext::hostHeaderVerifyFailed(const char *A, const
char *B)



{



// IP address validation for Host: failed. Admin wants to ignore
them.



// NP: we do not yet handle CONNECT tunnels well, so ignore for them



if (!Config.onoff.hostStrictVerify && http->request->method !=
Http::METHOD_CONNECT) {



debugs(85, 3, "SECURITY ALERT: Host header forgery detected on " <<
http->getConn()->clientConnection <<



"(" << A << "does not match " << B << ") on URL: " <<
http->request->effectiveRequestUri());



__ __

__ __

How does the squid get "hostHeaderVerifyFailed" for a normal HTTP
CONNECT request to a HTTP Proxy as simple as below?

__ __

CONNECT www.zscaler.com:80  HTTP/1.1

Host: www.zscaler.com:80 

User-Agent: Windows Microsoft Windows 10 Enterprise ZTunnel/1.0

Proxy-Connection: keep-alive

Connection: keep-alive

__ __

HTTP/1.1 409 Conflict

Server: squid

Mime-Version: 1.0

Date: Tue, 22 Feb 2022 20:59:42 GMT

Content-Type: text/html;charset=utf-8

Content-Length: 2072

X-Squid-Error: ERR_CONFLICT_HOST 0

Vary: Accept-Language

Content-Language: en

X-Cache: MISS from 3

Via: 1.1 3 (squid)

Connection: keep-alive

__ __





ERROR

The requested URL could not be retrieved





__ __



The following error was encountered while trying to retrieve
the URL: http://www.zscaler.com:80>">www.zscaler.com:80


..

__ __

__ __

__ __

Thank you for any help on the understanding!

__ __

Paul Ling


___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev


___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] RFC: protocols in Squid

[Alex Rousskov]

On 1/30/22 6:56 AM, Amos Jeffries wrote:
> Attached is first draft of a map for the transactions possible by
> protocols supported (and "should be") by Squid.
> 
> 
> In this diagram transactions are split into three types:
> 
>  1) switching - Protocol A ending initiates Protocol B.
> 
>  2) nested - Protocol B carries Protocol C in its message payload.
> 
>  3) fetch - Protocol B initiates Protocol D to fetch some data used by
> Protocol B.
> 
> Note: to simplify the number of arrows surrounding "HTTP" I have split
> out the CONNECT tunnel as a separate thing.
> 
> 
> Am posting this as RFC to see of anyone has additions of transactions I
> missed. Or suggestions on how better to document it.

Who is the target audience for this work? What are the primary goals of
this documentation effort? It is difficult to suggest improvements
without understanding who we are talking to and what we want them to
learn or do with that information...

FWIW, I personally do not find the chosen illustration approach useful
for documenting the relationship between various transactions or
protocols in Squid:

* At a high level, if the target audience are folks who do not know much
about Squid but are familiar with many network protocols, then the
picture can be morphed into a useful introductory illustration of the
protocols supported by Squid. However, most of the effort on
switch/nest/fetch documentation would be lost or misplaced in the
process -- there are much simpler/better ways to illustrate which
protocols are supported where, of course.

* At a high level, if the target audience are folks that want to do
serious Squid development, then the picture effectively obscures or
hides such critical architectural concepts as transaction layering and
grouping/cooperating. Illustrating those concepts needs a very different
approach IMO.

* At a low level, correctly interpreting or even tracing some of the
depicted individual relationships is already quite difficult for a human
like me, and when all the [should be] supported relationships are
included, the interesting parts may become an intractable spaghetti of
colored lines. I suspect it would be easier and a lot more useful to
just say (in text form!) what transport protocols are used by which
to-Squid and from-Squid higher-level transactions.


> Next step is to write up which object(s) or function implements each of
> these in Squid.

Developing a high-level documentation of primary transaction-related
class relationships may be very useful for establishing common
terminology and resolving refactoring conflicts. Such a document would
be based on about ~10 classes and rely heavily on the embedded class
documentation (instead of repeating or substituting it). I can draft
such a document if you think it may be useful.

At the level of "objects or functions", it would take a lot of time to
write and review such detailed function-level documentation. Maintaining
it would be costly as well, especially given the fact that a lot of that
code should be seriously refactored! This is your call, but I think your
personal energy and talents are better spent elsewhere, and the Project
as a whole should not undertake such a costly initiative with murky
outcomes.


Cheers,

Alex.
P.S. FWIW, I do not know what the various
circles/diamonds/hexagons/ovals mean on this particular diagram. Many
different standards and common notations are recycling those basic
shapes, of course.
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] v5.4 backports

[Alex Rousskov]

On 1/24/22 3:26 PM, Amos Jeffries wrote:
> On 21/01/22 08:20, Alex Rousskov wrote:
>> On 1/18/22 5:31 AM, Amos Jeffries wrote:
>>> The following changes accepted into v6 are also eligible for v5 but have
>>> issues preventing me scheduling them.
>>>
>>>
>>> This has conflicts I need some assistance resolving. So will not being
>>> doing the backport myself. If you are interested please open a PR
>>> against v5 branch for the working backport before Feb 1st.
>>>
>>>   * Bug #5090: Must(!request->pinnedConnection()) violation (#930)
>>>    squid-6-15bde30c33e47a72650ef17766719a5fc7abee4c
>>
>> The above fix relies on a Bug #5055 fix (i.e. master/v6 commit 2b6b1bc
>> mentioned below as a special case).
>>
> 
> Okay. I will try again for a later release.
>>
>>>   * Bug #5055: FATAL FwdState::noteDestinationsEnd exception: opening
>>> (#877)
>>>    squid-6-2b6b1bcb8650095c99a1916f5964305484af7ef0
>>
>> Waiting for a base commit SHA from you.
>>
>>
>>> and; Fix FATAL ServiceRep::putConnection exception: theBusyConns > 0
>>> (#939)
>>>    squid-6-a8ac892bab446ac11f816edec53306256bad4de7
>>
>> This fixes a bug in the Bug #5055 fix AFAICT, so waiting on the
>> master/v6 commit 2b6b1bc backport (i.e. the previous bullet).


> FYI; As of right now the v5 HEAD should be fine to base the PR on.

Please see https://github.com/squid-cache/squid/pull/967

I will leave it as a Draft PR until I have a chance to run more tests.
Please test what you can as well.


HTH,

Alex.
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] RFC: Adding a new line to a regex

[Alex Rousskov]

TLDR: Solutions #4 (substitute_logformat_codes) and #6 (STL regex) are
still leading. The next key question is: Do we want this feature to be
the last drop that brings STL regex support to Squid? If yes, #6 wins
IMO. Two followup questions at the end of the email, below details...


On 1/21/22 8:59 PM, Amos Jeffries wrote:
> On 22/01/22 08:36, Alex Rousskov wrote:
>> TLDR: I am adding solution #6 into the mix based on Amos email (#5 was
>> taken by Eduard). Amos needs to clarify why he thinks that Squid master
>> branch cannot accept STL-based regexes "now". After that, we can decide
>> whether #6 remains a viable candidate. Details below.
>>
>>
>> On 1/21/22 12:42 PM, Amos Jeffries wrote:
>>> On 20/01/22 10:32, Alex Rousskov wrote:
>>>> We have a use case where a regex in squid.conf should contain/match
>>>> a new line [...] This email discusses the problem and proposes how
>>>> to add a new line (and other special characters) to regexes found
>>>> in squid.conf and such.
>>
>>
>>> With the current mix of squid.conf parsers this RFC seems irrelevant
>>> to me.
>>
>> I do not understand the relationship between "the current mix of
>> squid.conf parsers" and this RFC relevance. This RFC is relevant because
>> it is about a practical solution to a real problem facing real Squid
>> admins.
>>
> 
> Sentence #2 of the RFC explicitly states that admin needs are not
> relevant "I do not know whether there are similar use
> cases with the existing squid.conf regex directives"

There is no connection between that sentence and admin needs:

* Admin needs are not limited to the existing squid.conf regex directives.

* It is possible that similar needs do exist for the existing squid.conf
regex directives. I just do not know it. This existence is not required
for the RFC to be relevant (because of the first bullet).


> The same sentence delimits RFC scope as: "adding a _new_ directive that
> will need such support."

Correct.


> That means the syntax defining how the regex pattern is configured does
> not yet exist. It is not necessary for the developer to design their
> _new_ UI syntax in a way that exposes admin to this problem in the first
> place. Simply design the

The above paragraph was cut short, but I agree with its beginning: It is
not necessary. In fact, I would say that hitting the problem we are
trying to solve would be a very bad idea! However, our definition of
"way that exposes" may differ.


>> Whether Squid has one parser or ten, good ones or bad ones, is relevant
>> to how the solution is implemented/integrated with Squid, of course, but
>> that is already a part of the analysis on this thread.

> Very relevant.

Glad we agree.


> RFC cites "squid.conf preprocessor and parameter parser
> use/strip all new lines" as a problem.
>
> I point out that this behaviour depends on *which* config parser is
> chosen to be used by the (again _new_) directive. It should be an
> implementation detail for the dev, not design consideration for this RFC.

The behavior I described is rooted in the _preprocessor_ behavior, so
one cannot work around it by inventing a new directive parser. By the
time we get to the directive parser, it is already too late -- the new
lines are already stripped.

Finally, even if we modify the preprocessor to preserve new lines in
some special cases, preserving real new lines is actually not a good
solution at all! That solution results in very awkward-looking
configuration statements. The code would not care, of course, but humans
writing those statements will. All of the proposed solutions are better IMO.


> I must state that I do not see much in the say of squid.conf syntax
> discussion in the RFC text. It seems to focus a lot on syntax inside the
> regex pattern.

... which is obviously a part of the encompassing squid.conf syntax, but
it does not really matter -- if you prefer to think of this RFC as
discussing regex pattern syntax, you can do that and still make correct
decisions AFAICT.


> IMO regex is such a complicated situation that we should avoid having
> special things inside or on top of its syntax. That is a recipe for
> admin pain.

I agree, we should, but we also should solve the problem this RFC is
addressing. The problem obviously has acceptable solutions. There is no
reason to say "regex are too complicated" and walk away. We can make
things better here by finding the best solution we can find.


>> 6. Use STL regex features that support \n and similar escape sequences
>> Pros: Supports much more than just advanced escape sequences!
>> Pros: The new syntax is easy to document by referencing library docs.
> 
> Pro:

Re: [squid-dev] RFC: Adding a new line to a regex

[Alex Rousskov]

TLDR: I am adding solution #6 into the mix based on Amos email (#5 was
taken by Eduard). Amos needs to clarify why he thinks that Squid master
branch cannot accept STL-based regexes "now". After that, we can decide
whether #6 remains a viable candidate. Details below.


On 1/21/22 12:42 PM, Amos Jeffries wrote:
> On 20/01/22 10:32, Alex Rousskov wrote:
>> We have a use case where a regex in squid.conf should contain/match
>> a new line [...] This email discusses the problem and proposes how
>> to add a new line (and other special characters) to regexes found
>> in squid.conf and such.


> With the current mix of squid.conf parsers this RFC seems irrelevant to me.

I do not understand the relationship between "the current mix of
squid.conf parsers" and this RFC relevance. This RFC is relevant because
it is about a practical solution to a real problem facing real Squid admins.

Whether Squid has one parser or ten, good ones or bad ones, is relevant
to how the solution is implemented/integrated with Squid, of course, but
that is already a part of the analysis on this thread.


> The developer designing a new directive also writes the parse_*()
> function that processes the config file line. All they have to do is
> avoid using the parser functions which implicitly do the problematic
> behaviour.

Concerns regarding the overall quality of Squid configuration syntax and
upgrade paths expand the reach of this problem far beyond a single new
directive, but let's assume, for the sake of the argument, that all we
care about is a new parsing function. Now we need to decide what syntax
that parsing function will use. This RFC is about that decision.


> The fact that there is logic imposing this problem at all is a bug to
> be resolved. But that is something for a different RFC.

FWIW, I do not know which logic/bug you are talking about here.


> There was a plan from 2014 (re-attempted by Christos 2016) to migrate
> Squid from the GNURegex dependency to more flexible C++11 regex library
> which supports many regex languages. With that plan the UI would only
> need an option flag or pattern prefix to specify which language a
> pattern uses.

I agree that one of the solutions worth considering is to use a regex
library that supports different regex syntax. So here is the
corresponding entry for solution based on C++ STL regex:

6. Use STL regex features that support \n and similar escape sequences
Pros: Supports much more than just advanced escape sequences!
Pros: The new syntax is easy to document by referencing library docs.
Cons: Requires serious changes to the internal regex support in Squid.
Cons: Miserable STL regex performance in some environments[1,2]?
Cons: Converting old regexes requires (complex) automation.
Cons: Requires dropping GCC v4.8 support.
Cons: Amos thinks Squid cannot support STL regex until 2024.

[1] See, for example, the following Reddit thread, ignoring comments
about GCC v4.8 and similar noise. The table in the second link is
representative of these performance concerns and there are similar
instability claims:
https://www.reddit.com/r/cpp/comments/e16s1m/what_is_wrong_with_stdregex
https://www.reddit.com/r/cpp/comments/e16s1m/what_is_wrong_with_stdregex/f94g2ny/

[2] STL does not allow us to define a custom allocator for its regexes.
Various STL implementations have various hidden workarounds, but we will
be at their (varying) mercy.



> That plan was put on hold due to feature-incomplete GCC 4.8 versions
> being distributed by CentOS 7 and RHEL needing to build Squid.

... and serious/substantiated performance concerns[1]. They may have
been addressed by STL implementations since then, but my quick check and
the impossibility of solving [2] without breaking ABI suggest that at
least some of these issues still remain.


> One Core Developer (you Alex) has repeatedly expressed a strong opinion
> veto'ing the addition/removal of features to Squid-6 while they are
> still officially supported by a small set of "officially supported"
> Vendors. RHEL and CentOS being in that set.

Sorry, I have no idea what you are talking about.

> When combined, those two design limitations mean the C++11 regex library
> cannot be implemented in a Squid released prior to June 2024.

Until you clarify what relevant addition/removals I have repeatedly
vetoed in this context, I cannot validate this assertion. Given how many
times you have completely misrepresented and misinterpreted my
statements (usually without providing usable references to them), it is
likely that this is just one of those pointless time wasting attacks.
Please stop doing that.


>> Unfortunately, squid.conf syntax lacks a similar general mechanism.

> This is not a property of squid.conf design choices. It is an artifact
> of the GNURegex language.

It is the other way around. C/C++ programs can supply LF charac

Re: [squid-dev] RFC: Adding a new line to a regex

[Alex Rousskov]

On 1/21/22 12:16 PM, Amos Jeffries wrote:
> On 21/01/22 07:27, Eduard Bagdasaryan wrote:
>> I would concur with Alex that (4) is preferable: It does not break old
>> configurations, re-uses existing mechanisms and allows to apply it
>> only when/where required. I have one more option for your
>> consideration: escaping with a backtick (e.g., `n) instead of a
>> backslash. This approach is used, e.g., in PowerShell.
>>
>> 5a. Recognize just `n escape sequence in squid.conf regexes.
>>
>> 5b. Recognize all '`'-based escape sequences in squid.conf regexes.
>>
>> Pros:  Easier upgrade: backtick is rare in regular expressions
>> (compared to '%' or '/'), probably there is no need to convert old
>> regexes at all.
>> Pros:  Simplicity: no double-escaping is required (as in (1b)).
>> Cons: Though it should be straightforward to specify common escape
>> sequences, such as `n, `r or `t, we still need to devise a way of
>> providing arbitrary character (i.e., its code) in this way.


> You are mixing up different features offered by several of the *many*
> different languages people call "regex".

Yes, to solve the problem, we should be (and are) considering both
applicable features from other languages/libraries and our own/bespoke
suggestions.


> Squid regex patterns are written in GNU Regular Expression language.
> None of those commonly expected things are features in that ancient
> language.

We cannot modify POSIX regex syntax, and we are not trying to do that.
We can modify how regexes are configured in squid.conf. We need a
solution that works with the existing POSIX regex syntax or on top of
it. Various solutions have been posted and analyzed.

If you want to help, please correct significant analysis mistakes (if
any) and/or add new option(s) to the list, for similar consideration.


Thank you,

Alex.
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] v5.4 backports

[Alex Rousskov]

On 1/18/22 5:31 AM, Amos Jeffries wrote:
> The following changes accepted into v6 are also eligible for v5 but have
> issues preventing me scheduling them.
> 
> 
> This has conflicts I need some assistance resolving. So will not being
> doing the backport myself. If you are interested please open a PR
> against v5 branch for the working backport before Feb 1st.
> 
>  * Bug #5090: Must(!request->pinnedConnection()) violation (#930)
>   squid-6-15bde30c33e47a72650ef17766719a5fc7abee4c

The above fix relies on a Bug #5055 fix (i.e. master/v6 commit 2b6b1bc
mentioned below as a special case).


> The following just need bugzilla IDs. If interested in getting a
> backport please open the bug report with useful details (ie document the
> user-visible behaviour) and then ping me.
> 
>  * Properly track (and mark) truncated store entries (#909)
>    squid-6-ba3fe8d9bc8d35c4b04cecf30cfc7288c57e685c

https://bugs.squid-cache.org/show_bug.cgi?id=5187

>  * Fix reconfiguration leaking tls-cert=... memory (#911)
>    squid-6-b05c195415169b684b6037f306feead45ee9de4e

https://bugs.squid-cache.org/show_bug.cgi?id=5188


>  * Preserve configured order of intermediate CA certificate chain (#956)
>    squid-6-166fb918211b76a0e79eb07967f4d092f74ea18d

https://bugs.squid-cache.org/show_bug.cgi?id=5190

Please do not misinterpret the creation of the above bug reports as an
implicit support for the idea of spending time on creating such bug
reports. I think our time is best spent elsewhere.



>  * Bug #5055: FATAL FwdState::noteDestinationsEnd exception: opening (#877)
>   squid-6-2b6b1bcb8650095c99a1916f5964305484af7ef0

Waiting for a base commit SHA from you.


> and; Fix FATAL ServiceRep::putConnection exception: theBusyConns > 0 (#939)
>   squid-6-a8ac892bab446ac11f816edec53306256bad4de7

This fixes a bug in the Bug #5055 fix AFAICT, so waiting on the
master/v6 commit 2b6b1bc backport (i.e. the previous bullet).


HTH,

Alex.
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

[squid-dev] RFC: Adding a new line to a regex

[Alex Rousskov]

Hello,

We have a use case where a regex in squid.conf should contain/match
a new line (i.e. ASCII LF). I do not know whether there are similar use
cases with the existing squid.conf regex directives, but that is not
important because we are adding a _new_ directive that will need such
support. This email discusses the problem and proposes how to add a new
line (and other special characters) to regexes found in squid.conf and such.

Programming languages usually have standard mechanisms for adding
special characters to strings from which regexes are compiled. We all
know that "a\nb" uses LF byte in the C++ string literal. Other bytes can
be added as well: https://en.cppreference.com/w/cpp/language/escape

Unfortunately, squid.conf syntax lacks a similar general mechanism. In
most cases, it is possible to work around that limitation by entering
special symbols directly. However, that trick causes various headaches
and does not work for new lines at all because squid.conf preprocessor
and parameter parser use/strip all new lines; the code compiling the
regular expression will simply not see any.

In POSIX regex(7), the two-character \n escape sequence is referring to
the ASCII character 'n', not the new line/LF character, so entering \n
(two characters) into a squid.conf regex value will not work if one
wants to match ASCII LF.

There are many options for adding this functionality to regexes used in
_new_ squid.conf contexts (i.e. contexts aware of this enhancement).
Here is a fairly representative sample:

1a. Recognize just \n escape sequence in squid.conf regexes
   Pros: Simple.
   Cons: Converting old regexes[1] requires careful checking[2].
   Cons: Cannot detect typos in escape sequences. \r is accepted.
   Cons: Cannot address other, similar use cases (e.g., ASCII CR).

1b. Recognize all C escape sequences in squid.conf regexes
   Pros: Can detect typos -- unsupported escape sequences.
   Cons: Poor readability: Double-escaping of all for-regex backslashes!
   Cons: Converting old regexes requires non-trivial automation.


2a. Recognize %byte{n} logformat-like sequence in squid.conf regexes
   Pros: Simple.
   Cons: Converting old regexes[1] requires careful checking[3].
   Cons: Cannot detect typos in logformat-like sequences.
   Cons: Does not support other advanced use cases (e.g., %tr).

2b. Recognize %byte{n} and logformat sequences in squid.conf regexes
   Pros: Can detect typos -- unsupported logformat sequences.
   Cons: The need to escape % in regexes will surprise admins.
   Cons: Converting old regexes requires (simple) automation.


3. Use composition to combine regexes and some special strings:
   regex1 + "\n" + regex2
   or
   regex1 + %byte{10} + regex2
   Pros: Old regexes can be safely used without any conversions.
   Cons: Requires new, complex composition expressions/syntax.
   Cons: A bit difficult to read.
   Cons: Requires a lot of development.


4. Use 2b but only when regex is given to a special function:
   substitute_logformat_codes(regex)
   Pros: Old regexes can be safely used without any conversions.
   Pros: New regexes do not need to escape % (by default).
   Pros: Extendable to old regex configuration contexts.
   Pros: Extendable to non-regex configuration contexts.
   Pros: Reusing the existing parameters(...)-like call syntax.
   Cons: A bit more difficult to read than 1a or 2a.
   Cons: Duplicates "quoted string" approach in some directives[4].
   Cons: Requires arguing about the new function name :-).


Given all the pros and cons, I think we should use option 4 above.

Do you see any better options?


Thank you,

Alex.

[1] We are still talking about new configuration contexts here, but we
should still be thinking about bringing old regexes into new contexts
and, eventually, possibly even about upgrading old contexts to support
the new feature. Neither is required or urgent, but it would be nice to
eventually end up with a uniform regex approach, of course.

[2] In most cases, no old regexes will contain the \n sequence because
it means nothing to the regex compiler. A few exceptional regexes can be
edited manually. Automated conversion will be required only in some rare
cases.

[3] Essentially the same as [2] above: Old regexes are unlikely to
contain the %byte 5-character sequence (or whatever we end up calling
that special sequence -- we are polishing a PR that adds %byte support
to logformat).

[4] Several existing squid.conf directives interpret "quoted values"
specially, substituting embedded logformat %codes. Arguably, the
explicit function call mechanism is better because there is less
confusion regarding which context supports it and which does not. And we
probably should not "quote regexes" because many old regexes contain
double quotes already.
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] squid 5.3 crash

[Alex Rousskov]

On 12/28/21 1:17 AM, Dmitry Melekhov wrote:

> Testing squid 5.3 on Ubuntu 20.04.

> 2021/12/28 09:58:01 kid1| assertion failed: Read.cc:61:
> "Comm::IsConnOpen(conn)"

> Is this assertion fail known problem?

Impossible to say for sure without a stack trace, unfortunately -- many
different code paths lead to that low level assertion -- but this could
be bug #5056 or a related problem, as triaged at:

https://bugs.squid-cache.org/show_bug.cgi?id=5056#c1


HTH,

Alex.
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] What os/cpu platforms do we want to target as a project?

[Alex Rousskov]

On 12/26/21 8:36 PM, Amos Jeffries wrote:
> On 27/12/21 10:11, Alex Rousskov wrote:
>> On 12/26/21 10:30 AM, Francesco Chemolli wrote:
>>> On Sun, Dec 5, 2021 at 10:05 PM Alex Rousskov wrote:
>>>> If we manage to and agree on what platforms to "support" and
>>>> on removing code dedicated to unsupported platforms, great! If
>>>> we fail, I would like to propose an alternative scheme for the
>>>> removal of platform-specific (or any other) code from the
>>>> official master branch:
>>>> 
>>>> A PR dedicated to code removal can be merged if it satisfies
>>>> the following criteria:
>>>> 
>>>> 1. Two positive votes from core developers. 2. No negative
>>>> votes. 3. Voting lasted for 30+ calendar days. 4. The removal
>>>> PR is announced in a PR-dedicated squid-users post. This
>>>> announcement resets the 30+ day timer.

>>> How about instead something along the lines of:
>>> 1. announce on squid-users about intent to remove support for a platform
>>> 2. wait for objections for 15 days
>>> 3. PR following standard merge procedure

>> My proposal is trying to solve the specific problem that recent PRs
>> (attempting to remove some code) have faced: The reviewer asked _why_ we
>> are removing code, and the author closed the PR instead of developing
>> consensus regarding the correct answer to that question[1]. My proposal
>> establishes a special track for code removal PRs so that they do not
>> have to answer the (otherwise standard) "why" question.


> [1] is about removal of a trivial piece of code that has no harm leaving
> in place.
> 
> As I stated in followup what I regard as reasonable justification *was*
> given up front. If that was not enough for agreement to merge it was not
> worth the jumping through hoops in the first place. Let alone creating a
> whole new bureaucratic policy and management process.
> 
> As I said in other discussions, we already have a deprecation+removal
> process that works fine and matches the industry standard practices when
> a code change is even suspected to matter to anyone at all. That process
> gives far more than just 30 days to inform any interested community
> members and does not limit the notification channels.
> 
> If anyone picks up the code removal in [1] again they should follow that
> process 


I do not think they should. IMO, that (poorly working)
deprecation+removal process should not be used for explicit OSF/1
support removal (among other things). They should use the regular PR
review process (if we do not come up with something better by then).


> since that code is now obviously of some importance to reviewer Alex.

FWIW, I still[2] support removal of explicit OSF/1 support. The reason
we had this discussion is not some special importance of that explicit
OSF/1 support code. The goal is to lower the overhead of future PRs
similar to the one you closed _because_ that overhead was too high.


Hope this clarifies,

Alex.

[1] https://github.com/squid-cache/squid/pull/942#issuecomment-986208860
[2] https://github.com/squid-cache/squid/pull/942#issuecomment-986055422
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] What os/cpu platforms do we want to target as a project?

[Alex Rousskov]

On 12/27/21 4:41 AM, Francesco Chemolli wrote:
> On Sun, Dec 26, 2021 at 10:58 PM Alex Rousskov wrote:
>>
>> On 12/26/21 10:30 AM, Francesco Chemolli wrote:
>>> On Sun, Dec 5, 2021 at 10:05 PM Alex Rousskov wrote:
>>>> On 12/5/21 4:44 AM, Francesco Chemolli wrote:
>>>>> I would recommend that we support as main targets:
>>>>> - Linux on x64, arm64, arm32 and, if we can, MIPS
>>>>> - FreeBSD, OpenBSD on x64
>>>>
>>>>> As best-effort:
>>>>> - Windows on x64, with the aim of eventually promoting to primary target
>>>>> - Darwin on x64 and maybe eventually arm64
>>>>> - NetBSD on x64

First of all, thank you for making good progress on this important thread.


> I'm a bit more reluctant with downgrading FreeBSD and (once stable) 
> OpenBSD. They ship Squid as part of their ports, and while the 
> developer community may not be very large, I'd expect the user 
> community to be a bit more prevalent. This would also prevent the
> risk of the project becoming a Linux monoculture.

I share the overall sentiment, but I think all of the above concerns can
continue to be adequately addressed with FreeBSD/OpenBSD being assigned
to the second tier (i.e. the "best-effort" tier you have proposed). IMO,
it makes little sense to name two tiers and then essentially treat the
second named tier as "all other".

The second tier, as proposed, will bring our attention to any
FreeBSD/OpenBSD regressions and, in most cases, we will address them
(because they are usually easy to address). IMO, this level of support
is more than adequate given our lack of resources and relative
unpopularity of those platforms.

Perhaps this difference in approaches is rooted in our current (and
newly discovered!) disagreement on when best-effort/tier-2 results must
be reported (as discussed further below).


> My angle on that is that having arm32 will help us not become a
> 64-bit monoculture, which could bring in regressions in low-level
> data structures. At this stage in the embedded world, as far as I'm
> aware, there are x86-32, arm32, MIPS and RISC-V. Of these arm32 is
> the most prevalent and therefore my preferred choice. It also helps
> that we have a test environment for it already, courtesy of a couple
> of raspberry pi 3.

Same here: I share the sentiment but consider all these arguments
appropriate for tier-2; they are an overkill for tier-1. Squid can
survive without x86-32, arm32, MIPS, and RISC-V support. Yes, we do not
want to accidentally abandon support for those environments, without a
compelling reason, but that is exactly why we are adding tier-2 -- to
eliminate such accidents: Thanks to tier-2 presence, the decision to
regress support for those platforms (in some specific PR scope) will be
deliberate, _not_ accidental.



>>> Once achieved this status on any of these platforms,
>>> regressions on it are release blockers.
>>
>> I do not know what you mean by "release" exactly, but we should not add
>> rules that block schedule-based branching points and major releases[1].
>> [1] https://wiki.squid-cache.org/ReleaseSchedule

Just to avoid misunderstanding, my comments/opinions about minor
releases below apply to the text below. My sentence quoted above is not
about minor releases; it is about major releases; major releases occur
on a fixed schedule; they are not governed by release maintenance rules
that are largely in Amos hands today. Moreover, since no PR can be
merged if it breaks tier-1 support, it is impossible to have a tier-1
regression unless we change the test environment without fixing Squid
first (which we should not do, especially for tier-1 platforms!).


>> As for other/minor releases, I am not against this rule if the
>> maintainer is happy about it.

> Amos, what do you think?

I would like to adjust my comment: I think this additional formal rule
is not needed because we should just let the maintainer decide what
maintenance rules work best (and adjust them as needed), but I am not
against adding that formal rule if Amos wants to add it to his current
set of rules.


>> AFAICT, the primary costs of keeping a platform on a best-effort list
>> are CI testing time and CI maintenance overheads. I suggest capping that
>> best-effort list to keep the total CI test time for one PR commit under,
>> say, 4 hours and granting CI maintenance team a veto on adding (but not
>> removing) platforms. IIRC, we have discussed PR commit testing time a
>> few months ago; if we agreed on another number of hours there, let's
>> take that as a cap.
> 
> The best-effort model supports testing best-effort platforms
> asynchronously, no need to cap there. Regressions would be noted and
> hopefully dealt with i

Re: [squid-dev] What os/cpu platforms do we want to target as a project?

[Alex Rousskov]

On 12/26/21 10:30 AM, Francesco Chemolli wrote:
> On Sun, Dec 5, 2021 at 10:05 PM Alex Rousskov wrote:
>> On 12/5/21 4:44 AM, Francesco Chemolli wrote:
>>> I would recommend that we support as main targets:
>>> - Linux on x64, arm64, arm32 and, if we can, MIPS
>>> - FreeBSD, OpenBSD on x64
>>
>>> As best-effort:
>>> - Windows on x64, with the aim of eventually promoting to primary target
>>> - Darwin on x64 and maybe eventually arm64
>>> - NetBSD on x64
>>
>> What does "support as main targets" and "support as best-effort" mean?

> "main targets" to me means that any regression in build or unit tests
> on these platforms is PR-blocking. We aim to deliver a working build
> out of the box on these environments, with no need of maintainer
> patches.

I am OK with that definition if you remove "in build or unit tests". Any
known Squid regression affecting the "main" environment should block the
PR introducing that regression IMO. I see no need to limit this to
"build and unit tests" regressions. Are you OK with that definition
simplification?

I do not think MIPS, FreeBSD, and OpenBSD should be on the "main" list:
There are just too few users and contributors on those platforms right
now to grant them primary status AFAICT. I may be missing some important
signals, of course, but I would downgrade them to best-effort.

I am not sure about arm32, for similar reasons, but perhaps there is a
strong demand for "embedded" Squid that I do not know about (and that
does not materialize in a stream of PRs because Squid already works well
in those environments??)? How can we tell?


> Once achieved this status on any of these platforms,
> regressions on it are release blockers.

I do not know what you mean by "release" exactly, but we should not add
rules that block schedule-based branching points and major releases[1].
As for other/minor releases, I am not against this rule if the
maintainer is happy about it.

[1] https://wiki.squid-cache.org/ReleaseSchedule



> "support as best effort" means that we keep these environment as build
> options, and test builds on them. Regressions on build or unit tests
> on these environments are not PR-blocking but we strongly encourage
> the PR owner to fix these platforms with followup PRs. We aim to
> deliver an out-of-the-box build on these platforms but failure to do
> so or regressions are not a release blocker

OK. If we drop the vague parts, "best effort" simply means that CI
covers these platforms (but they are not "main" environments and, hence,
the corresponding CI failures alone are not sufficient to block PRs).

AFAICT, the primary costs of keeping a platform on a best-effort list
are CI testing time and CI maintenance overheads. I suggest capping that
best-effort list to keep the total CI test time for one PR commit under,
say, 4 hours and granting CI maintenance team a veto on adding (but not
removing) platforms. IIRC, we have discussed PR commit testing time a
few months ago; if we agreed on another number of hours there, let's
take that as a cap.


>>> If anyone in the community wants to support/maintain extra OSes or
>>> architectures, they're very welcome to do so
>>
>> By "welcome", do you mean something like "we will accept high-quality
>> PRs adding such extra support to Squid"? Or "we will consider sharing
>> references to your work, but please do not ask us to merge your
>> OS-specific changes into the official Squid code"? Something else? This
>> bit is critical for understanding the real effect of this proposal IMO.
> 
> Fair point. I'd go for the former. If someone has an environment they
> want to support and do so without adding complexity to the code base,
> it's a good policy to enable that. The question is "how much
> complexity is it healthy to impose on other platforms to support
> niche/obsolete ones?"
> While setting an objective bar is hard to impossible, this is my
> attempt to set one for the most active group of developers

I agree regarding the level of difficulty in defining that bar.

Unfortunately, I do not think "we will accept high-quality PRs adding
such extra support to Squid" is something we should run with. We simply
do not have the resources to support some new environments, even if PRs
introducing them are perfect in every respect. For example, a perfect PR
introducing (explicit) LibreSSL support should be blocked IMO because we
do not have enough resources to properly handle the two already
(unfortunately) accepted TLS libraries (OpenSSL and GnuTLS).

However, we can drop this part of your proposal in hope to get to the
consensus on the other, arguably more important parts: The definitions
of three support levels (main, best-effort, and other). We can come back
to this part of the discussion later/separately, armed with established
and tested support levels.


Cheers,

Alex.
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] What os/cpu platforms do we want to target as a project?

[Alex Rousskov]

On 12/26/21 10:30 AM, Francesco Chemolli wrote:
> On Sun, Dec 5, 2021 at 10:05 PM Alex Rousskov wrote:
>> If we manage to and agree on what platforms to "support" and on removing
>> code dedicated to unsupported platforms, great! If we fail, I would like
>> to propose an alternative scheme for the removal of platform-specific
>> (or any other) code from the official master branch:
>>
>> A PR dedicated to code removal can be merged if it satisfies the
>> following criteria:
>>
>> 1. Two positive votes from core developers.
>> 2. No negative votes.
>> 3. Voting lasted for 30+ calendar days.
>> 4. The removal PR is announced in a PR-dedicated squid-users post.
>>This announcement resets the 30+ day timer.

> How about instead something along the lines of:
> 1. announce on squid-users about intent to remove support for a platform
> 2. wait for objections for 15 days
> 3. PR following standard merge procedure


My proposal is trying to solve the specific problem that recent PRs
(attempting to remove some code) have faced: The reviewer asked _why_ we
are removing code, and the author closed the PR instead of developing
consensus regarding the correct answer to that question[1]. My proposal
establishes a special track for code removal PRs so that they do not
have to answer the (otherwise standard) "why" question.

Your proposal changes nothing as far as that problem is concerned. Thus,
unless that past experience has altered our understanding of the
problem, we are likely to face the same problem again!

Needless to say, getting squid-users feedback is nice -- both proposals
share that feature. However, even an enthusiastic squid-users support
for X removal cannot replace the answer to the "why" question. This is
why I am proposing to change the rules to remove that question for some PRs.


Hope this clarifies,

Alex.

[1] https://github.com/squid-cache/squid/pull/942#issuecomment-986208860
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] Squid does not accept WCCP of Cisco router since CVE 2021-28116

[Alex Rousskov]

On 12/5/21 6:11 PM, Andrej Mikus wrote:

> I would like to find some information about wccp servers (routers,
> firewalls, etc) that are officially supported and therefore tested for
> compatibility.

IIRC, there are no such servers/etc. WCCP code quality is low, the code
has been neglected for a long time, and the changes we recently had to
do for CVE 2021-28116 took a very long time, were unfinished and
essentially untested because, in part, those looking for testers could
not get anybody to test the changes and report the results back to us.


> Is there any way to get in touch with the developper responsible for the
> security patch and request his comments?

You are using the right channel for that. I was one of the developers
that were forced to work on code changes for CVE 2021-28116, but I am
not sure I would consider myself "responsible for the patch" (it depends
on your definition of "responsible"). The advisory says the bug was
fixed by Amos; Amos is on this mailing list.


> I do not have access to other
> Cisco hardware, and I would like to know if the update was confirmed
> working for example against a CSR1000v.

I do not think that update was confirmed as working against any WCCP
server. If you are using WCCP, you are relying on a long-neglected
feature. There is no proper support for WCCP code in Squid today IMO.


Alex.
P.S. Squid side of CVE 2021-28116 is at
https://github.com/squid-cache/squid/security/advisories/GHSA-rgf3-9v3p-qp82



> - Forwarded message from amk <1952...@bugs.launchpad.net> -
> 
> Date: Sun, 05 Dec 2021 22:21:51 -
> From: amk <1952...@bugs.launchpad.net>
> To: launch...@mikus.sk
> Subject: [Bug 1952158] Re: squid does not accept WCCP of Cisco router since 
> 3.5.27-1ubuntu1.12
> 
> 4.13-10ubuntu5 in 21.10 and 5.2-1ubuntu1 in jammy are failing as well,
> with debug log different when compared to version 3 involved here:
> 
> 2021/12/05 19:58:41.705 kid1| 80,6| wccp2.cc(1580) wccp2HereIam: 
> wccp2HereIam: Called
> 2021/12/05 19:58:41.705 kid1| 80,5| wccp2.cc(1599) wccp2HereIam: 
> wccp2HereIam: sending to service id 0
> 2021/12/05 19:58:41.705 kid1| 80,3| wccp2.cc(1630) wccp2HereIam: Sending 
> HereIam packet size 144
> 2021/12/05 19:58:41.707 kid1| 80,6| wccp2.cc(1202) wccp2HandleUdp: 
> wccp2HandleUdp: Called.
> 2021/12/05 19:58:41.707 kid1| 80,3| wccp2.cc(1226) wccp2HandleUdp: Incoming 
> WCCPv2 I_SEE_YOU length 128.
> 2021/12/05 19:58:41.707 kid1| ERROR: Ignoring WCCPv2 message: duplicate 
> security definition
> exception location: wccp2.cc(1249) wccp2HandleUdp
> 
> This looks like a problem with squid itself, the packet does not have
> duplicate security definition. In the code at http://www.squid-
> cache.org/Doc/code/wccp2_8cc_source.html I miss some debug output in the
> loop processing the packet /* Go through the data structure */ so would
> need to rebuild the package or to involve debugger.
> 
> I was not able to find any documentation of squid listing
> supported/tested wccp servers but at this point this looks like an issue
> to be reported upstream. There is no reason to consider wccp packets
> from IOS 15.8(3)M2 invalid.
> 

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] What os/cpu platforms do we want to target as a project?

[Alex Rousskov]

On 12/5/21 4:44 AM, Francesco Chemolli wrote:

> I would recommend that we support as main targets:
> - Linux on x64, arm64, arm32 and, if we can, MIPS
> - FreeBSD, OpenBSD on x64

> As best-effort:
> - Windows on x64, with the aim of eventually promoting to primary target
> - Darwin on x64 and maybe eventually arm64
> - NetBSD on x64

What does "support as main targets" and "support as best-effort" mean?

Without defining/detailing these two terms, it is impossible to properly
evaluate this proposal IMO...


> If anyone in the community wants to support/maintain extra OSes or
> architectures, they're very welcome to do so

By "welcome", do you mean something like "we will accept high-quality
PRs adding such extra support to Squid"? Or "we will consider sharing
references to your work, but please do not ask us to merge your
OS-specific changes into the official Squid code"? Something else? This
bit is critical for understanding the real effect of this proposal IMO.



If we manage to and agree on what platforms to "support" and on removing
code dedicated to unsupported platforms, great! If we fail, I would like
to propose an alternative scheme for the removal of platform-specific
(or any other) code from the official master branch:

A PR dedicated to code removal can be merged if it satisfies the
following criteria:

1. Two positive votes from core developers.
2. No negative votes.
3. Voting lasted for 30+ calendar days.
4. The removal PR is announced in a PR-dedicated squid-users post.
   This announcement resets the 30+ day timer.

The author and reviewers do not have to justify the removal and the
votes if the PR author elects to use this special code removal
procedure. We simply trust they have good reasons to remove, support
removal, or block it.

A PR that does not satisfy the above criteria may still be merged under
the existing/regular PR merging rules. This special procedure does not
change those primary rules. It simply grants us the ability to avoid
difficult justifications and associated policy discussions in exchange
for longer wait times and more votes/exposure, under the assumption that
when it comes to simple removal, we just want to avoid (to the extent
possible) removing something that is still in significant use.

I am not sure we need rule #4, but it is not very difficult to post a
GitHub link and a PR description to squid-users, so the extra protection
that rule offers may be worth the posting overheads.

I am OK with increasing the 30 day waiting period if others think we
should wait longer for possible objections.


Thank you,

Alex.
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] What os/cpu platforms do we want to target as a project?

[Alex Rousskov]

On 12/5/21 7:00 AM, Amos Jeffries wrote:
> On 5/12/21 22:44, Francesco Chemolli wrote:
>> The rationale is that we should focus our attention as a project where
>> the majority of our userbase is, where users mean "people who build
>> and run squid".

> I do not accept that a few hacks for old OS are causing pain to core
> developers simply by bitrotting in untouched code.

Squid code that should be removed (from any reasonable person point of
view) may cause significant pain. I (and, I bet, others) have wasted a
lot of time dealing with such code. Old OSes and platforms are not be
the _biggest_ contributor here, but they are a contributor, and I hope
that this thread will result in a scheme that is applicable to a much
wider set of code snippets than those OS hacks alone.


> Removing things because the majority of users are not using it would
> mean we become a Linux-only project and thus actively push people out of
> the community unnecessarily. There are active benefits gained in the
> form of better code, bug detection, and community inclusiveness from
> maintaining portable code.

Nobody questions the real benefits of supporting non-Linux platforms
AFAICT. The questions usually revolve around the benefits of a given
OS-specific code snippet -- do they outweigh the drawbacks associated
with it? It is impossible to correctly answer such questions in general,
especially in the context of a starving Project. We may have to paint
with a much broader pragmatic brush to arrive at imperfect but
reusable/generic solutions that we can afford.


> IMO specific machine architectures are not much concern to "us". The
> toolchains and vendors take care of that part so long as we provide
> half-decent code.

I would go one step further and claim that any OS-specific Squid code
that exists to accomodate OS Foo idiosyncrasies is the responsibility of
the folks working on OS Foo-based distributions. It should not be the
responsibility of the Squid project. Ideally, we should not have such
code in Squid. This is what distro patches/adjustments are for.

Alex.
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] RFC: Categorize level-0/1 messages

[Alex Rousskov]

On 12/5/21 8:06 AM, Amos Jeffries wrote:
> On 21/10/21 16:16, Alex Rousskov wrote:
>> On 10/20/21 3:14 PM, Amos Jeffries wrote:
>>> On 21/10/21 4:22 am, Alex Rousskov wrote:
>>>> To facilitate automatic monitoring of Squid cache.logs, I suggest to
>>>> adjust Squid code to divide all level-0/1 messages into two major
>>>> categories -- "problem messages" and "status messages"[0]:
>>
>>> We already have a published categorization design which (when/if used)
>>> solves the problem(s) you are describing. Unfortunately that design has
>>> not been followed by all authors and conversion of old code to it has
>>> not been done.
>>
>>> Please focus your project on making Squid actually use the system of
>>> debugs line labels. The labels are documented at:
>>>    https://wiki.squid-cache.org/SquidFaq/SquidLogs#Squid_Error_Messages
>>
>> AFAICT, the partial classification in that wiki table is an opinion on
>> how things could be designed, and that opinion does not reflect Project
>> consensus.

> The wiki was written from observation of how the message labels are/were
> being used in the code. As such it reflects the defacto consensus of
> everyone ever authoring code that used one of the labels.

[ N.B. I am worried that this (mostly irrelevant IMO) part of the
discussion risks ruining the shaky agreement we have reached on the
important parts of the RFC, but I am also worried about
misrepresentation of the wiki table status. I will respond here, but
please move any future discussion about that table status (if you decide
to continue them) to a different email thread. ]

AFAICT, the wiki table in question does not accurately reflect Squid
code and does not constitute Project consensus on how things could be
designed, regardless of what observations led to that table creation.
The creative process of writing a classification table (based on code
observations) naturally allows for misinterpretations, mistakes, and
other problems. One cannot claim consensus on the _result_ on the
grounds that they have started with code observations.


>> FWIW, I cannot use that wiki table for labeling messages, but
>> I do not want to hijack this RFC thread for that table review.

> You our text below contradicts the "cannot" statement by describing how
> the two definitions fit together and offer to use the wiki table labels
> for problem category.

I cannot use the wiki table for deciding how to label a given message
(because of the problems with the table definitions that I would rather
not review here), but the primary labels we use (and should continue to
use!) are naturally found in that table. There is no contradiction here.


>> * The wiki table talks about FATAL, ERROR, and WARNING messages. These
>> labels match the RFC "problem messages" category. This match covers all
>> of the remaining cache.log messages except for 10 debugs() detailed
>> below. Thus, so far, there is a usable match on nearly all current
>> level-0/1 messages. Excellent!

> Thus my request that you use the wiki definitions to categorize the
> unlabeled and fix any detected labeling mistakes.

While I cannot use those wiki definitions (because of the problems with
the table that I would rather not review here), it is not a big deal as
far as this RFC is concerned because I do not have to use or violate
those definitions to implement the vast majority of the proposed changes
-- those changes are orthogonal to the wiki table and its definitions.

If somebody finds a table violation introduced by the RFC PR, then we
will either undo the corresponding PR change, change the label used by
the PR, or fix the table, but my goal is to minimize the number of such
cases because they are likely to waste a lot of time on difficult
discussions about poorly defined concepts.



>> * The wiki table also uses three "SECURITY ..." labels. The RFC does not
>> recognize those labels as special. I find their definitions in the wiki
>> table unusable/impractical, and you naturally think otherwise, but the
>> situation is not as bad as it may seem at the first glance:
>>
>> - "SECURITY ERROR" is used once to report a coding _bug_. That single
>> use case does not match the wiki table SECURITY ERROR description. We
>> should be able to rephrase that single message so that does it not
>> contradict the wiki table and the RFC.
>>
>> - "SECURITY ALERT" is used 6 times. Most or all of those cases are a
>> poor match for the SECURITY ALERT description in the wiki table IMHO. I
>> hope we can find a way to rephrase those 6 cases to avoid conflicts.
>>
>> - "SECURITY NOTICE" is used 3 times. Two

Re: [squid-dev] request for change handling hostStrictVerify

[Alex Rousskov]

On 11/2/21 4:25 AM, k...@sudo-i.net wrote:
> 
> On Monday, November 01, 2021 14:58 GMT, Alex Rousskov
>  wrote:
>  
>> On 11/1/21 3:59 AM, k...@sudo-i.net wrote:
>> > On Saturday, October 30, 2021 01:14 GMT, Alex Rousskov wrote:
>> >> >> AFAICT, in the majority of deployments, the mismatch between the
>> >> >> intended IP address and the SNI/Host header can be correctly handled
>> >> >> automatically and without creating serious problems for the
>> user. Squid
>> >> >> already does the right thing in some cases. Somebody should
>> carefully
>> >> >> expand that coverage to intercepted traffic. Frankly, I am somewhat
>> >> >> surprised nobody has done that yet given the number of complaints!
>>
>> > Not sure what do you mean with "Somebody should carefully expand that
>> > coverage to intercepted traffic"?
>>
>> I meant that somebody should create a high-quality pull request that
>> modifies Squid source code to properly address the problem you, AFAICT,
>> are suffering from. There is already code that handles similar
>> situations correctly.


> I will try to implement it.

Please note that implementing correct changes in this area may be
difficult, especially if you are not familiar with relevant Squid code
and the invariants it is trying to uphold. Please do not expect the
changes to be officially accepted just because they "work" in your use case.

It is your call whether or how to approach this task, but if you find
yourself modifying a lot of Squid code, especially code that seems
mysterious, you might want to pause and sketch a solution for the
discussion here on squid-dev or via a Draft PR on GitHub.


HTH,

Alex.
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] request for change handling hostStrictVerify

[Alex Rousskov]

On 11/1/21 3:59 AM, k...@sudo-i.net wrote:
> On Saturday, October 30, 2021 01:14 GMT, Alex Rousskov wrote:
>> >> AFAICT, in the majority of deployments, the mismatch between the
>> >> intended IP address and the SNI/Host header can be correctly handled
>> >> automatically and without creating serious problems for the user. Squid
>> >> already does the right thing in some cases. Somebody should carefully
>> >> expand that coverage to intercepted traffic. Frankly, I am somewhat
>> >> surprised nobody has done that yet given the number of complaints!

> Not sure what do you mean with "Somebody should carefully expand that
> coverage to intercepted traffic"?

I meant that somebody should create a high-quality pull request that
modifies Squid source code to properly address the problem you, AFAICT,
are suffering from. There is already code that handles similar
situations correctly.

Alex.
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] request for change handling hostStrictVerify

[Alex Rousskov]

On 10/29/21 8:37 PM, Amos Jeffries wrote:
> On 30/10/21 11:09, Alex Rousskov wrote:
>> On 10/26/21 5:46 PM, k...@sudo-i.net wrote:
>>
>>> - Squid enforces the Client to use SNI
>>> - Squid lookup IP for SNI (DNS resolution).
>>> - Squid forces the client to go to the resolved IP
>>
>> AFAICT, the above strategy is in conflict with the "SECURITY NOTE"
>> paragraph in host_verify_strict documentation: If Squid strays from the
>> intended IP using client-supplied destination info, then malicious
>> applets will escape browser IP-based protections. Also, SNI obfuscation
>> or encryption may make this strategy ineffective or short-lived.
>>
>> AFAICT, in the majority of deployments, the mismatch between the
>> intended IP address and the SNI/Host header can be correctly handled
>> automatically and without creating serious problems for the user. Squid
>> already does the right thing in some cases. Somebody should carefully
>> expand that coverage to intercepted traffic. Frankly, I am somewhat
>> surprised nobody has done that yet given the number of complaints!

> IIRC the "right thing" as defined by TLS for SNI verification is that it
> be the same as the host/domain name from the wrapper protocol (i.e. the
> Host header / URL domain from HTTPS messages). Since Squid uses the SNI
> at step2 as Host value it already gets checked against the intercepted IP


Just to avoid misunderstanding, my email was _not_ about SNI
verification. I was talking about solving the problem this thread is
devoted to (and a specific solution proposed in the opening email on the
thread).

Alex.
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] request for change handling hostStrictVerify

[Alex Rousskov]

On 10/26/21 5:46 PM, k...@sudo-i.net wrote:

> - Squid enforces the Client to use SNI
> - Squid lookup IP for SNI (DNS resolution).
> - Squid forces the client to go to the resolved IP

AFAICT, the above strategy is in conflict with the "SECURITY NOTE"
paragraph in host_verify_strict documentation: If Squid strays from the
intended IP using client-supplied destination info, then malicious
applets will escape browser IP-based protections. Also, SNI obfuscation
or encryption may make this strategy ineffective or short-lived.

AFAICT, in the majority of deployments, the mismatch between the
intended IP address and the SNI/Host header can be correctly handled
automatically and without creating serious problems for the user. Squid
already does the right thing in some cases. Somebody should carefully
expand that coverage to intercepted traffic. Frankly, I am somewhat
surprised nobody has done that yet given the number of complaints!


HTH,

Alex.
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] Alternate origin server selection

[Alex Rousskov]

On 10/29/21 9:57 AM, Steve Hill wrote:

> Ok, I've gone back and looked over my old debug logs.  It appears what
> was actually happening was:
> 
> - Client sends "CONNECT www.google.com:443".
> - Connection with TLS made to forcesafesearch.google.com.
> - Client sends "GET / HTTP/1.1\r\nHost: www.google.com"
> - Squid runs the peer selector to find peers for www.google.com (i.e.
> the host contained in the GET request).
> - It finds the appropriate pinned connection:
> client_side.cc(3872) borrowPinnedConnection: conn28
> local=81.187.83.66:52488 remote=216.239.38.120:443 HIER_DIRECT FD 18
> flags=1
> - Squid then logs:
>   FwdState.cc(472) fail: ERR_ZERO_SIZE_OBJECT "Bad Gateway"
>   https://www.google.com/
>   FwdState.cc(484) fail: pconn race happened
>   FwdState.cc(494) fail: zero reply on pinned connection

The above looks like a persistent connection race, way after SslBump
bumped the connections. This kind of races seem unrelated to everything
we have discussed on this thread so far, but I may be missing something.


> Unfortunately, I cannot reproduce this problem now.

Well, if you want to reproduce it, you probably can do that using a
custom origin server, but I do not see the point: There is nothing
particularly "wrong" in the above sequence AFAICT; races happen.
However, it is possible that the above log is lying or misleading, and
there was actually a different problem, a problem related to previous
discussions, but it manifested itself as ERR_ZERO_SIZE_OBJECT "pconn
race happened" diagnostic.


> I can remove the unpinning code and submit a new pull request, which now
> works ok for me.  But I'm very wary that I did originally have problems
> with that which I can no longer reproduce.

Unfortunately, I did not have a chance to study the rejected pull
request before it was rejected, so I cannot advise you on whether
subtracting something from that pull request will result in changes that
should be, in principle, accepted.

I suggest to imagine that the rejected pull request did not exist
(instead of using it as a starting/reference point) and then describe
the problem and the proposed solution from scratch. You can do that here
or via a new pull request. The best avenue may depend on whether your
code changes alter some key Squid principles. If they do, it might be
best to get an agreement on those changes here, but GitHub discussions
have better access to code and better formatting abilities.


Cheers,

Alex.
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] Alternate origin server selection

[Alex Rousskov]

On 10/28/21 12:39 PM, Steve Hill wrote:
> On 28/10/2021 16:41, Alex Rousskov wrote:
>> AFAICT, the primary obstacle here is that Squid pins the connection
>> while obtaining the origin server certificate.

> Well, I can't see why Squid needs the origin certificate - it should be
> able to make a decision off the SNI before connecting to the origin server.

Squid does not "need" any of this, of course. Configuration and/or bugs
force Squid to do what it does. If your decision-making process does not
involve the certificate, then you should be able to rewrite the fake
CONNECT request during SslBump step2, without (or before) telling Squid
to stare at the certificate (and pin the resulting connection).

There are bugs in this area, including bugs that may prevent certain
CONNECT adaptations from happening. We are fixing one of those bugs
right now. For details, you can see an unpolished/unofficial pull
request at https://github.com/measurement-factory/squid/pull/108


> I didn't seem to be able to make the decision prior to the connection
> being pinned though.  I'm not sure why - I will go back and investigate
> further.

Sounds like a plan!


Cheers,

Alex.
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] Alternate origin server selection

[Alex Rousskov]

On 10/28/21 9:24 AM, Steve Hill wrote:

> For transparently proxied traffic, the client makes a connection to
> www.google.com's IP address, which Squid intercepts.  Squid must then
> SSL-peek the request to figure out that it is connecting to
> www.google.com.  The onward connection can then be redirected to the
> virtual IP.

IIRC, Google has recommended (to my surprise) something like that as
well, for environments where DNS modifications are inappropriate and
bumping is possible. I cannot find that recommendation now, unfortunately.


> There is code to do this:
>   https://github.com/squid-cache/squid/pull/924
> This allows an external ACL to record an alt-host note, or an ICAP
> server to return an X-Alt-Host header, specifying a new origin server to
> connect to.
>
> The pull request was rejected, as it adds CVE-2009-0801 vulnerabilities.
> 
> I'm hoping for some guidance on the best way to achieve this.


While I disagree with some of the assertions made in that PR review and
the unilateral approach to closing that PR, I hope we can find a
positive way forward here. Your use case deserves Squid support IMO.


AFAICT, the primary obstacle here is that Squid pins the connection
while obtaining the origin server certificate. Please confirm that
without that origin server certificate you cannot make the decision
whether to redirect the HTTP request. In other words, the
client-intended destination IP address and the client-supplied SNI are
not sufficient to correctly determine whether the connection must be
bumped (in relevant cases).

Also, if you need the server certificate indeed, please confirm that
when the origin server uses TLS v1.3, the proposed scheme will have to
fully bump the Squid-server connection before redirecting the request.
Just staring at the TLS ServerHello will not be enough because the
origin certificate comes later, in the encrypted records.


I hope to suggest the right solution based on your feedback.


Thank you,

Alex.
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] RFC: Categorize level-0/1 messages

[Alex Rousskov]

On 10/20/21 3:14 PM, Amos Jeffries wrote:
> On 21/10/21 4:22 am, Alex Rousskov wrote:
>> To facilitate automatic monitoring of Squid cache.logs, I suggest to
>> adjust Squid code to divide all level-0/1 messages into two major
>> categories -- "problem messages" and "status messages"[0]:


> We already have a published categorization design which (when/if used)
> solves the problem(s) you are describing. Unfortunately that design has
> not been followed by all authors and conversion of old code to it has
> not been done.

> Please focus your project on making Squid actually use the system of
> debugs line labels. The labels are documented at:
>   https://wiki.squid-cache.org/SquidFaq/SquidLogs#Squid_Error_Messages

AFAICT, the partial classification in that wiki table is an opinion on
how things could be designed, and that opinion does not reflect Project
consensus. FWIW, I cannot use that wiki table for labeling messages, but
I do not want to hijack this RFC thread for that table review.

Fortunately, there are many similarities between the wiki table and this
RFC that we can and should capitalize on instead:

* While the wiki table is silent about the majority of existing
cache.log messages, most of the messages it is silent about probably
belong to the "status messages" category proposed by this RFC. This
assumption gives a usable match between the wiki table and the RFC for
about half of the existing level-0/1 cache.log messages. Great!

* The wiki table talks about FATAL, ERROR, and WARNING messages. These
labels match the RFC "problem messages" category. This match covers all
of the remaining cache.log messages except for 10 debugs() detailed
below. Thus, so far, there is a usable match on nearly all current
level-0/1 messages. Excellent!

* The wiki table also uses three "SECURITY ..." labels. The RFC does not
recognize those labels as special. I find their definitions in the wiki
table unusable/impractical, and you naturally think otherwise, but the
situation is not as bad as it may seem at the first glance:

- "SECURITY ERROR" is used once to report a coding _bug_. That single
use case does not match the wiki table SECURITY ERROR description. We
should be able to rephrase that single message so that does it not
contradict the wiki table and the RFC.

- "SECURITY ALERT" is used 6 times. Most or all of those cases are a
poor match for the SECURITY ALERT description in the wiki table IMHO. I
hope we can find a way to rephrase those 6 cases to avoid conflicts.

- "SECURITY NOTICE" is used 3 times. Two of those use cases can be
simply removed by removing the long-deprecated and increasingly poorly
supported SslBump features. I do not see why we should keep the third
message/feature, but if it must be kept, we may be able to rephrase it.

If we cannot reach an agreement regarding these 10 special messages, we
can leave them as is for now, and come back to them when we find a way
to agree on how/whether to assign additional labels to some messages.


Thus, there are no significant conflicts between the RFC and the table!
We strongly disagree how labels should be defined, but I do not think we
have to agree on those details to make progress here. We only need to
agree that (those 10 SECURITY messages aside) the RFC-driven message
categorization projects should adjust (the easily adjustable) messages
about Squid problems to use three standard labels: FATAL, ERROR, and
WARNING. Can we do just that and set aside the other disagreements for
another time?

If there are serious disagreements whether a specific debugs() is an
ERROR or WARNING, we can leave those specific messages intact until we
find a way to reach consensus. I hope there will be very few such
messages if we use the three labels from the RFC and do our best to
avoid controversial changes.


> What we do not have in that design is clarity on which labels are shown
> at what level.

In hope to make progress, I strongly suggest to _ignore_ the difference
between level 0 and level 1 for now. We are just too far apart on that
topic to reach consensus AFAICT. The vast majority of messages that
RFC-driven projects should touch (and, if really needed, _all_ such
messages!) can be left at their current level, avoiding this problem.



> I have one worry about you taking this on right now. PR 574 has not been
> resolved and merged yet, but many of the debugs() messages you are going
> to be touching in here should be converted to thrown exceptions - which
> ones and what exception type is used has some dependency on how that PR
> turns out.

If the RFC PRs are merged first, Factory will help resolve conflicts in
the PR 574 branch. While resolving boring conflicts is certainly
annoying, this is not really a big deal in this case, and both projects
are worth the pain.

Alternatively, I can offer to massage PR 5

[squid-dev] RFC: Categorize level-0/1 messages

[Alex Rousskov]

Hello,

Nobody likes to be awaken at night by an urgent call from NOC about
some boring Squid cache.log message the NOC folks have not seen before
(or miss a critical message that was ignored by the monitoring system).
To facilitate automatic monitoring of Squid cache.logs, I suggest to
adjust Squid code to divide all level-0/1 messages into two major
categories -- "problem messages" and "status messages"[0]:

* Problem messages are defined as those that start with one of the three
well-known prefixes: FATAL:, ERROR:, and WARNING:. These are the
messages that most admins may want to be notified about (by default[1])
and these standardized prefixes make setting up reliable automated
notifications straightforward.

* Status messages are all other messages. Most admins do not want to be
notified about normal Squid state changes and progress reports (by
default[2]). These status messages are still valuable in triage so they
are _not_ going away[3].


Today, Squid does not support the problem/status message classification
well. To reach the above state, we will need to adjust many messages so
that they fall into the right category. However, my analysis of the
existing level-0/1 messages shows that it is doable to correctly
classify most of them without a lot of tedious work (all numbers and
prefix strings below are approximate and abridged for clarity of the
presentation):

* About 40% of messages (~700) already have "obvious" prefixes: BUG:,
BUG [0-9]*:, ERROR:, WARNING:, and FATAL:. We will just need to adjust
~20 existing BUG messages to move them into one of the three proposed
major categories (while still being clearly identified as Squid bugs, of
course).

* About 15% of messages (~300) can be easily found and adjusted using
their prefixes (after excluding the "obvious" messages above). Here is a
representative sample of those prefixes: SECURITY NOTICE, Whoops!, XXX:,
UPGRADE:, CRITICAL, Error, ERROR, Error, error, ALERT, NOTICE, WARNING!,
WARNING OVERRIDE, Warning:, Bug, Failed, Stop, Startup:, Shutdown:,
FATAL Shutdown, avoiding, suspending, DNS error, bug, cannot, can't,
could not, couldn't, bad, unable, malformed, unsupported, not found,
missing, broken, unexpected, invalid, corrupt, obsolete, unrecognised,
and unknown. Again, there is valuable information in many of these
existing prefixes, and all valuable information will be preserved (after
the standardized prefix). Some of these messages may be demoted to
debugging level 2.

* The remaining 45% of messages (~800) may remain as is during the
initial conversion. Many of them are genuine status/progress messages
with prefixes like these: Creating, Processing, Adding, Accepting,
Configuring, Sending, Making, Rebuilding, Skipping, Beginning, Starting,
Initializing, Installing, Indexing, Loading, Preparing, Killing,
Stopping, Completed, Indexing, Loading, Killing, Stopping, Finished,
Removing, Closing, Shutting. There are also "squid -k parse" messages
that are easy to find automatically if somebody wants to classify them
properly. Most other messages can be adjusted as/if they get modified or
if we discover that they are frequent/important enough to warrant a
dedicated adjustment.

If there are no objections or better ideas, Factory will work on a few
PRs that adjust the existing level-0/1 messages according to the above
classification, in the rough order of existing message categories/kinds
discussed in the three bullets above.


Thank you,

Alex.

[0] The spelling of these two category names is unimportant. If you can
suggest better category names, great, but let's focus on the category
definitions.

[1] No default will satisfy everybody, and we already have the
cache_log_message directive that can control the visibility and volume
of individual messages. However, manually setting those parameters for
every level-0/1 message is impractical -- we have more than 1600 such
messages! This RFC makes a reasonable _default_ treatment possible.

[2] Admins can, of course, configure their log monitoring scripts to
alert them of certain status messages if they consider those messages
important. Again, this RFC is about facilitating reasonable _default_
treatment.

[3] We could give status messages a unique prefix as well (e.g., INFO:)
but such a prefix is not necessary to easily distinguish them _and_
adding a prefix would create a lot more painful code changes, so I think
we should stay away from that idea.
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] bizarre build behaviour from PoolingAllocator on OpenBSD/clang

[Alex Rousskov]

On 8/16/21 10:44 AM, Alex Rousskov wrote:
> On 8/16/21 7:29 AM, Stuart Henderson wrote:
> 
>> -c .../src/log/access_log.cc -fPIC -DPIC -o .libs/access_log.o
>> In file included from .../src/log/access_log.cc:12:
>> In file included from .../src/AccessLogEntry.h:19:
>> In file included from .../src/HttpHeader.h:13:
>> In file included from .../src/base/LookupTable.h:15:
> 
>> /usr/include/c++/v1/unordered_map:857:5: error: static_assert failed
>> due to requirement 'is_same> long>, std::__1::pair>::value' "Invalid
>> allocator::value_type"
> 
>> static_assert((is_same> allocator_type::value_type>::value),
>> ^  
>> ~~~
> 
> Yes, that could be the smoking gun missing from the earlier error log,
> thank you, Stuart!
> 
> Francesco, can you reproduce the same false is_same<> result using a toy
> example? You might not even need Squid-specific types.
> 
> If you can reproduce it, is the result valid/expected or is that a clang
> bug?


BTW, since LookupTable.h was mentioned in the above error, I think it
might be important to mention that the unordered_map declaration in that
file (lookupTable_t) is probably buggy because it is missing the custom
equality operator. I have recently discovered that bug but did not have
the time to properly report or address it.

BTW, LookupTable.h is unrelated to PoolingAllocator -- lookupTable_t
does not pool (yet?).


HTH,

Alex.
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] bizarre build behaviour from PoolingAllocator on OpenBSD/clang

[Alex Rousskov]

On 8/16/21 7:29 AM, Stuart Henderson wrote:

> -c .../src/log/access_log.cc -fPIC -DPIC -o .libs/access_log.o
> In file included from .../src/log/access_log.cc:12:
> In file included from .../src/AccessLogEntry.h:19:
> In file included from .../src/HttpHeader.h:13:
> In file included from .../src/base/LookupTable.h:15:

> /usr/include/c++/v1/unordered_map:857:5: error: static_assert failed
> due to requirement 'is_same long>, std::__1::pair>::value' "Invalid
> allocator::value_type"

> static_assert((is_same allocator_type::value_type>::value),
> ^  
> ~~~

Yes, that could be the smoking gun missing from the earlier error log,
thank you, Stuart!

Francesco, can you reproduce the same false is_same<> result using a toy
example? You might not even need Squid-specific types.

If you can reproduce it, is the result valid/expected or is that a clang
bug?

Alex.
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] Coding Style updates

[Alex Rousskov]

On 8/15/21 9:07 PM, Amos Jeffries wrote:

> The existence of such a style requirement on Factory developers, and
> thus need for Squid code to match it for ease of future bug fixing, was
> given to me as a reason for ICAP and eCAP feature code staying in the
> Factory supplied one-line format despite the remainder of Squid code
> back then using two-line.

The above interpretation does not match reality. I continue to urge you
to stop describing things that you do not have enough information about
to describe accurately, especially things that relate to other people.
Statistically speaking, the probability of success for such dangerous
attempts has been close to zero (and there is no actual need to describe
those things to advance your arguments).



> So, between your two responses I gather that there will be no push-back
> on a PR adding enforcement of two-line function/method definitions by
> astyle 3.1.

There is push back, of course -- any large format changes need to
justify the expenses they necessarily bring. Whether that push back is
enough to block that future PR depends, AFAICT, on Astyle formatting
results and the timing of migration to auto results. As previously
discussed, we do not want to reformat the same large set of lines twice
in a short succession.


> It appears to be one of the policy rules not copied over to that page
> from the Squid-2 page.

The fact that the alleged official formatting rules are not actually
documented anywhere explains some of the inconsistencies and makes
making future rule setting easier IMO.

Alex.
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] bizarre build behaviour from PoolingAllocator on OpenBSD/clang

[Alex Rousskov]

On 8/15/21 2:51 PM, Francesco Chemolli wrote:
> Hi all,
>   I'm looking into OpenBSD compatibility for trunk, and there's a
> strange behaviour at build time on  OpenBSD (6.9) / clang (10.0.1)
> 
> When building src/log/access.log.cc, build fails with these errors:
> 
> -- begin quote ---
> static_assert((is_same allocator_type::value_type>::value),
> ^
> ~~~

Are some compiler messages missing in the above quote? It feels like the
above line is not directly related to the lines below, but perhaps I am
misinterpreting the messy output.

> I'm puzzled: I wouldn't expect this to be an OS-specific failure.

We cannot be sure it is OS-specific, but STL implementation is full of
hacks and optimizations that may be OS- or environment-specific. It is
also possible that there is a conflict between GCC and clang
installation; we have seen those before and they manifest in weird ways.

It is strange that the error below mentions "unordered_map" when it
should say "std::unordered_map" or similar.

Are you building with clang standard library or GCC standard library (I
believe -stdlib controls that, but it is not my area of expertise)? If
you are building with GCC stdlib, perhaps try building with
clang-provided stdlib?

I wish I could be more helpful,

Alex.


> access_log.cc:66:26: note: in instantiation of template class
> 'std::__1::unordered_map std::__1::hash, std::__1::equal_to,
> PoolingAllocator > >'
> requested here
> static HeaderValueCounts TheViaCounts;
>  ^
> access_log.cc:460:25: error: type 'HeaderValueCounts' (aka
> 'unordered_map, equal_to,
> PoolingAllocator > >') does not provide
> a subscript operator
> ++TheForwardedCounts[key];
>   ~~^~~~
> access_log.cc:467:24: error: invalid range expression of type 'const
> std::__1::unordered_map std::__1::hash, std::__1::equal_to,
> PoolingAllocator > >'; no
> viable 'begin' function available
> for (const auto  : counts)
>^ ~~
> /usr/include/c++/v1/initializer_list:99:1: note: candidate template
> ignored: could not match 'initializer_list' against 'unordered_map'
> begin(initializer_list<_Ep> __il) _NOEXCEPT
> ^
> /usr/include/c++/v1/iterator:1753:1: note: candidate template ignored:
> could not match '_Tp [_Np]' against 'const
> std::__1::unordered_map std::__1::hash, std::__1::equal_to,
> PoolingAllocator > >'
> begin(_Tp (&__array)[_Np])
> ^
> /usr/include/c++/v1/iterator:1771:1: note: candidate template ignored:
> substitution failure [with _Cp = const std::__1::unordered_map unsigned long long, std::__1::hash, std::__1::equal_to,
> PoolingAllocator > >]: no
> member named 'begin' in 'std::__1::unordered_map long, std::__1::hash, std::__1::equal_to,
> PoolingAllocator > >'
> begin(_Cp& __c) -> decltype(__c.begin())
> ^   ~
> /usr/include/c++/v1/iterator:1779:1: note: candidate template ignored:
> substitution failure [with _Cp = std::__1::unordered_map unsigned long long, std::__1::hash, std::__1::equal_to,
> PoolingAllocator > >]: no
> member named 'begin' in 'std::__1::unordered_map long, std::__1::hash, std::__1::equal_to,
> PoolingAllocator > >'
> begin(const _Cp& __c) -> decltype(__c.begin())
> ^ ~
> access_log.cc:489:24: error: no member named 'clear' in
> 'std::__1::unordered_map std::__1::hash, std::__1::equal_to,
> PoolingAllocator > >'
> TheForwardedCounts.clear();
> ~~ ^
> -- end quote ---
> 
> But then, removing the reference to PoolingAllocator in the definition
> of HeaderValueCounts, everything works.
> """
> using HeaderValueCounts = std::unordered_map std::hash, std::equal_to >;
> """
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] Coding Style updates

[Alex Rousskov]

On 8/12/21 8:31 PM, Amos Jeffries wrote:

> I am aware that Factory ... prefers the one-line style.

Factory does not prefer the one-line style.



> If we don't have agreement on a change I will
> implement enforcement of the existing style policy.

I cannot find any existing/official rules regarding single- or
multi-line function definitions in [1]. Where are they currently stated?

[1] https://wiki.squid-cache.org/SquidCodingGuidelines

Whether we should (adopt, if it has not been already, and) enforce a
particular style depends, in part, on how well Astyle supports that
style. The quality of that support will probably become clear from the
corresponding PR, and we can decide what to do at PR review time.

Alex.
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] Coding Style updates

[Alex Rousskov]

On 8/12/21 12:42 AM, Amos Jeffries wrote:

> 1) return type on separate line from function definition.
> 
> Current style requirement:
> 
>   template<...>
>   void
>   foo(...)
>   {
>     ...
>   }
> 
> AFAIK, this based on GNU project style preferences from the far past
> when Squid had a tight relationship with GNU. Today we have a conflict
> between Factory code coming in with the alternative same-line style
> and/or various developers being confused by the mix of code they are
> touching.

AFAIK, Factory developers try to follow the official style (while
keeping the old code consistent), just like all the other developers
(should) do. They make mistakes just like all other developers do. It is
unfortunate that you felt it is necessary to single out a group of
developers in a negative way (that is completely irrelevant to your
actual proposal).


> IMO; it is easier to work with the one-line style when command-line
> editing, and irrelevant when using more advanced tools.

FWIW, I do not find it easier to use one-line style when using
command-line tools. In fact, the opposite is probably true in my experience.


> As such I propose converting Squid to the same-line style:
> 
>   template<...>
>   void foo(...)
>   {
>     ...
>   }
> 
> 
> Opinions?

One-line stile increases horizontal wrapping, especially when the return
type is complex/long and there are additional markers like "static",
"inline", and "[[ noreturn ]]".

One line approach itself is inconsistent because template<> is on the
other line(s).

Searching for a function name with unknown-to-searcher or complex return
type becomes a bit more difficult when using "git log -L" and similar tools.

In many cases, function parameters should be on multiple lines as well.
Their alignment would look worse if the function name does not start the
line.

Most function definitions will have "auto" return type after we upgrade
to C++14. Whether that makes one-line style more or less attractive is
debatable, but it is an important factor in delaying related changes.

Eventually, we may be able to automatically remove explicit return types
using one of the clang-tidy tools, but such a tool does not exist yet
(modernize-use-trailing-return-type comes close but is not it).

In summary, I recommend delaying this decision until C++14. However, if
others insist on changing the format and changing it now, then I will
not block these changes, assuming newer astyle produces reasonable results.


Can new astyle support multiline formatting? If not, _that_ could be a
strong argument for changing the style.


> 2) braces on one-liner conditional blocks
> 
> Current code style is a bit confused. We have it as a style to use, with
> exceptions for some old macros etc where the lack of braces causes
> compile issues or bugs.
> 
> Personally I prefer the non-braced style. But it seems far safer to
> automate the always-brace style and not have those special exceptions.
> 
> Opinions?

I also prefer the non-braced style for simple expressions.

Perhaps there is a way to automatically wrap complex ones, for some
reasonable definition of "complex"?

Alex.
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] Page render issue at whitelisting

[Alex Rousskov]

On 7/20/21 2:03 PM, Bisma usman wrote:
> Hi , my name is BISMA i have issue related to squid whitelisting.
> 
> I have configured the whitelisting in conf file , e.g i whitelisted
> facebook.com , when i open facebook in browser it
> opens facebook but page dosent load css or CDN domains some images
> dont cosplay some text get large , page loads improper and when i check
> network tab of browser it shows that some external links facebook
> requires to load images and css and js like 1aplha.imgfb.com
> bla bla , and proxy blocks them .
> 
> How can i allow such things in whitelist as i use .facebook but it
> doesn't work , im attaching my conf file in attachments.

Hi Bisma,

The squid-dev list is dedicated to Squid software development. For
free Squid _administration_ support, please repost your question to the
squid-users mailing list:
http://www.squid-cache.org/Support/mailing-lists.html#squid-users

When you repost to squid-users, please indicate whether you are OK with
allowing _all_ accesses to sites that facebook site uses, regardless of
whether a particular request came from a client that needed the
requested resource to finish rendering a page loaded from facebook.com.


HTH,

Alex.


___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] Squid Features

[Alex Rousskov]

On 7/20/21 9:09 AM, mitesh.pa...@amul.coop wrote:

> I am Squid proxy user… Need to know more on its security part.


Hi Mitesh,

Security is a vast topic. You may get better responses from this
Squid development mailing list if you rephrase your email so that it
becomes a specific question about Squid development.

For free Squid administration support, please post to the squid-users
mailing list instead (but I would recommend rephrasing that posting to
be more specific as well):
http://www.squid-cache.org/Support/mailing-lists.html#squid-users


HTH,

Alex.
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] [squid-users] Squid modification to only read client SNI without bumping.

[Alex Rousskov]

On 6/8/21 7:36 AM, squ...@treenet.co.nz wrote:

> The way I think to approach it though is to start with the
> configuration parser.

That starting point does not compute for me. We do need to agree on how
to configure this feature, but parsing any resulting Squid configuration
ought to be very straightforward. Perhaps you have meant "TLS
ClientHello parser", but Squid already has that.


> A simple peek-splice/terminate TLS traffic flow
> should not need certificates setup by admin.

Squid already does not generate/use certificates for splicing or
terminating connections. In splice-or-terminate use cases, the
certificates come into play only when delivery _errors_. A feature to
prevent bumping for error delivery (and remove any configuration
requirements for CA certificate) should be welcomed IMO.

Please drop squid-users if responding to this email.

Alex.
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] Compilling squid

[Alex Rousskov]

On 5/29/21 2:58 PM, phenom252...@yandex.ru wrote:

> Hello, please help with building squid with ssl support for filtering
> https in transparent mode. I am assembling from sources on ubuntu server
> 18.20 version. The problem is that reading on your site, I do not
> understand what dependencies are needed to build a transparent squid in
> each version. I search for articles on the Internet, but this does not
> give much, since the articles may be outdated and when assembling, a
> mountain of errors simply pours in. The question is where is the list of
> dependencies for the assembly and what else is needed. Sorry for the
> possibly stupid questions, I'm just learning to work with a squid and I
> don't understand some points, but I really want to learn. Thank you for
> your help.

IIRC, the following packages are sufficient to build Squid on relatively
modern AWS servers, but I am not sure whether they match Ubuntu server
18.20 needs specifically:

> build-essential
> g++
> gdb
> automake
> autoconf
> autoconf-archive
> libtool
> libtool-bin
> libssl1.0-dev
> zlib1g-dev
> openssl
> libsystemd-dev
> pkg-config


> ./configure --enable-ssl-crtd --with-openssl ...

I suggest that you fix basic build errors before trying to add more
dependencies for interception. I do not know exactly what libraries you
need for interception support on Ubuntu, and the answer may depend on
what kind of interception you want to use. Others on this mailing list
may help with that.


HTH,

Alex.
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] Strategy about build farm nodes

[Alex Rousskov]

On 5/17/21 3:32 PM, Francesco Chemolli wrote:
> On Mon, May 17, 2021 at 8:32 PM Alex Rousskov wrote:
> 
> On 5/17/21 2:17 AM, Francesco Chemolli wrote:
> > $ make all push
> 
> Does that "make push" command automatically switch Jenkins CI to using
> the new/pushed containers? Or is that a separate manual step?

> Right now that's automatic.
> I'm pondering the best way to have a staging step; Docker supports
> versioning of images but we're using that feature to implement
> multiarch. In order to rely on that I'll need to change the naming
> conventions we rely on.

I think you should be free to use whatever versioning/naming scheme you
think works best for supporting safe Jenkins CI upgrades.

Ideally, the new docker images (and any other changes) should be tested
(against official branches) _before_ the official tests are switched to
use them. I hope you find a way to implement that without a lot of work.
If you need to tap into Foundation resources, please say so!


> The overarching objectives are:
> - focus on where most users probably are
> - allow iterating quickly giving some signal on tracked branches
> - allow landing quickly with more signal
> - be slow but exhaustive on every other platform we can cover. Do not
> block landing new code on less common or harder to test on platforms,
> but still rely on devs to own their changes there, too.

What exactly do you mean by "own their changes there"? The rest is
general/vague enough to allow for many interpretations I can agree with,
but that last bit seems rather specific, so I wanted to ask...


> I think that full round of PR tests (i.e. one initial set plus one
> staging set) should not exceed ~24 hours, _including_ any wait/queuing
> time.

> Is everyone okay with such a slow turnaround?

Well, you should not overload the question by calling a 24-hour delay
for an all-green signal slow :-).

It is all relative, of course: Most Squid PRs take a lot longer to
review and fix. There are open source projects where PRs wait for a week
or more, so 24 hours for a full round (and usually just an hour or two
for the first signal!) would feel very fast for them. Etc.

Yes, 24 hours is not "interactive", but it is acceptable for the vast
majority of PRs AFAICT, and you are providing dockers for those who want
to test in a more interactive mode.


> I think you are proposing to make some tests optional. Which ones?
> 
> 
> Not the tests, but the platforms.

From my test results consumer point of view, they are
[platform-specific] [build] tests.


> Things like gentoo, fedora rawhide, freebsd, openbsd.
> Testing is slower there, so results will lag and we need to batch,
> running a full test suite every week or so

OK, so you propose to make slow Jenkins platform-specific tests (i.e.
Jenkins tests on platforms where tests run slowly today) optional and
those platforms are gentoo, fedora rawhide, freebsd, openbsd. Got it!

What happens when such a slow test fails and the PR author ignores the
failure of that optional test and/or the PR is already closed? The
answer to that question is the key to evaluating any proposal that
declares any tests optional IMO...


> FWIW, I do not think reducing the number of #ifdefs will solve our
> primary CI problems.


> Each test build is right now 4 full builds and test suites:
> autodetected, minimal, maximum, everytthing-in-except-for-auth

And all of that takes less than 10 minutes on decent hardware which we
can rent or buy! To me, it feels like we are creating an unsolvable
problem here (i.e. testing a lot of things quickly on a raspberry Pi)
and then trying extra hard to solve that unsolvable problem. We should
either stop testing a lot of things or we should use appropriate
resources for testing a lot of things quickly.

Yes, we can reduce testing time by removing #ifdefs, but that is a lot
of work and does not really scale. #ifdefs should be removed primarily
for other reasons.


Cheers,

Alex.
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] Strategy about build farm nodes

[Alex Rousskov]

On 5/17/21 2:17 AM, Francesco Chemolli wrote:

> Our Linux environments are docker containers on amd64, armv7l and arm64.
> On a roughly monthly cadence, I pull from our dockerfiles repo
> (https://github.com/kinkie/dockerfiles) and
> $ make all push

Does that "make push" command automatically switch Jenkins CI to using
the new/pushed containers? Or is that a separate manual step?


>>> What I would place on each individual dev is the case where a PR breaks
>>> something in the trunk-matrix,trunk-arm32-matrix, trunk-arm64-matrix,
>>> trunk-openbsd-matrix, trunk-freebsd-matrix builds

>> I think you are saying that
>> some CI tests called trunk-matrix, trunk-arm32-matrix,
>> trunk-arm64-matrix, trunk-openbsd-matrix, trunk-freebsd-matrix should be
>> classified as _required_.

> That is how I read the statement too.

... but it sounds like that is _not_ what you (Francesco) is actually
proposing because you are essentialy saying "that ideal would be
impractical" in the paragraph below. Assuming that you are not attacking
your own proposal, that means Amos and I do not know what your proposal
is -- we both guessed incorrectly...


> In a word of infinite resources and very efficient testing, sure.
> But in a space where a single os/compiler combo takes 2hrs on Linux and
> 4hrs on Freebsd or openbsd, and a full 5-pr-test takes 6 hours end to
> end, we need to optimize or making any of these requirements blocking
> would make these times get 4+ times larger (a full trunk-matrix takes
> just about a day on amd64, 2 days on arm64), and the complaint would
> then be that development or release is slowed down by the amount of
> testing done.

FWIW, I think that full round of PR tests (i.e. one initial set plus one
staging set) should not exceed ~24 hours, _including_ any wait/queuing
time. This kind of delay should still allow for reasonable progress with
PRs AFAICT. This includes any release PRs, of course. If we exceed these
limits (or whatever limits we can agree on), then we should add testing
resources and/or drop tests.


> My proposal aims to test/land the PR on the systems where we can be
> efficient and that are relevant, and fix any remaining after-the-fact
> issues with followup, PRs, that remain a soft requirement for the dev
> introducing the change.

Here, I think you are proposing to make some tests optional. Which ones?


> For instance: we currently have a lot of different build-time
> configurations meant to save core memory in runtime environments. Is it
> maybe time to revisit this decision and move these checks to run time?

Sure, quality proposals for removing #ifdefs should be welcomed, one
(group of) #ifdefs at a time. We can warn squid-users in advance in case
somebody wants to provide evidence of harm.


> Unfortunately, one of the problems we have is that we're running blind.
> We don't know what configurations our users deploy; we can only assume
> and that makes this conversation much more susceptible to opinions and
> harder to build consensus on

Yes, we are blind, but I doubt we should care much about actual
configuration in this specific context. If we can remove an #ifdef
without serious negative consequences, we should remove it. We can avoid
long discussions by allowing anybody with concrete evidence of problems
to block any particular removal. I bet that conservative approach would
still allow for the removal of many #ifdefs.


FWIW, I do not think reducing the number of #ifdefs will solve our
primary CI problems. I believe we should reduce the number of platforms
we test on and then use Foundation resources to speed up and improve the
remaining tests.


HTH,

Alex.
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] Strategy about build farm nodes

[Alex Rousskov]

On 5/16/21 10:19 PM, squ...@treenet.co.nz wrote:
> On 2021-05-17 11:56, Alex Rousskov wrote:
>> On 5/16/21 3:31 AM, Amos Jeffries wrote:
>>> On 4/05/21 2:29 am, Alex Rousskov wrote:
>>>> The principles I have proposed allow upgrades that do not violate key
>>>> invariants. For example, if a proposed upgrade would break master, then
>>>> master has to be changed _before_ that upgrade actually happens, not
>>>> after. Upgrades must not break master.
>>>
>>> So ... a node is added/upgraded. It runs and builds master fine. Then
>>> added to the matrices some of the PRs start failing.
>>
>> It is easy to misunderstand what is going on because there is no good
>> visualization of complex PR-master-Jenkins_nodes-Jenkins_failures
>> relationships. Several kinds of PR test failures are possible. I will
>> describe the two most relevant to your email:
>>
>> * PR test failures due to problems introduced by PRs should be welcomed
>> at any time.
> 
> Strawman here. This is both general statement and not relevant to CI
> changes or design(s) we are discussing.

The first bullet (out of the two bullets that are meant to be
interpreted together, in their context) is not even close to being a
strawman argument by any definition I can find. Neither is the
combination of those two bullets.


>> CI improvements are allowed to find new bugs in open PRs.

> IMO the crux is that word "new". CI improvements very rarely find new
> bugs. What it actually finds and intentionally so is *existing bugs* the
> old CI config wrongly ignored.

Here, I used "new" to mean bugs that had not been found by the previous
CI version (i.e. "newly discovered" or "new to us"). These bugs existed
before and after CI improvements, of course -- neither master nor the PR
has changed -- but we did not know about them.


>> * PR test failures due to the existing master code are not welcomed.

> That is not as black/white as the statement above implies. There are
> some master branch bugs we don't want to block PRs merging, and there
> are some (rarely) we absolutely do not want any PRs to change master
> until fixed.

Yes, master bugs should not affect PR merging in the vast majority of
cases, but that is not what this bullet is about at all!

This bullet is about (a certain kind of) PR test failures. Hopefully, we
do not need to revisit the debate whether PRs with failed tests should
be merged. They should not be merged, which is exactly why PR test
failures caused by the combination of CI changes and the existing master
code are not welcomed -- they block progress of an innocent PR.


>> They represent a CI failure.
> 
> IMO this is absolutely false. The whole point of improving CI is to find
> those "existing" bugs which the previous CI config wrong missed.

Your second sentence is correct, but it does not make my statement
false. CI should achieve several goals. Finding bugs (i.e. blocking
buggy PRs) is one of them. Merging (i.e. not blocking) PRs that should
be merged is another. And there are a few more goals, of course. The
problem described in my second bullet represents a CI failure to reach
one of the key CI goals or a failure to maintain a critical CI
invariant. It is a CI failure rather than a PR failure (the latter is
covered by the first bullet).


> e.g. v4+ currently do not build on Windows. We know this, but the
> current CI testing does not show it. Upgrading the CI to include a test
> for Windows is not a "CI failure".

If such an upgrade would result in blocking PRs that do not touch
Windows code, then that upgrade would be a CI failure. Or a failure to
properly upgrade CI.


>> In these cases, if the latest master code
>> is tested with the same test after the problematic CI change, then that
>> master test will fail. Nothing a PR can do in this situation can fix
>> this kind of failure because it is not PR changes that are causing the
>> failure -- CI changes broke the master branch,
> 
> Ah. "broke the master branch" is a bit excessive. master is not broken
> any more or less than it already was.

If you can suggest a better short phrase to describe the problem, please
do so! Until then, I will have to continue to use "breaking master" (in
this context) simply for the lack of a better phrase. I tried to explain
what that phrase means to avoid misunderstanding. I cannot find a better
way to compress that explanation into a single phrase.


> What is *actually* broken is the CI test results.

One can easily argue that CI test results are actually "correct" in this
case -- they correctly discover a bug in the code that wants to become
the next master. The problem is in the assignment of responsibili

Re: [squid-dev] Strategy about build farm nodes

[Alex Rousskov]

On 5/16/21 3:31 AM, Amos Jeffries wrote:
> On 4/05/21 2:29 am, Alex Rousskov wrote:
>> On 5/3/21 12:41 AM, Francesco Chemolli wrote:
>>> - we want our QA environment to match what users will use. For this
>>> reason, it is not sensible that we just stop upgrading our QA nodes,
>>
>> I see flaws in reasoning, but I do agree with the conclusion -- yes, we
>> should upgrade QA nodes. Nobody has proposed a ban on upgrades AFAICT!
>>
>> The principles I have proposed allow upgrades that do not violate key
>> invariants. For example, if a proposed upgrade would break master, then
>> master has to be changed _before_ that upgrade actually happens, not
>> after. Upgrades must not break master.
> 
> So ... a node is added/upgraded. It runs and builds master fine. Then
> added to the matrices some of the PRs start failing.

It is easy to misunderstand what is going on because there is no good
visualization of complex PR-master-Jenkins_nodes-Jenkins_failures
relationships. Several kinds of PR test failures are possible. I will
describe the two most relevant to your email:

* PR test failures due to problems introduced by PRs should be welcomed
at any time. CI improvements are allowed to find new bugs in open PRs.
Such findings, even when discovered at the "last minute", should be seen
as an overall positive event or progress -- our CI was able to identify
a problem before it got officially accepted! I do not recall anybody
complaining about such failures recently.

* PR test failures due to the existing master code are not welcomed.
They represent a CI failure. In these cases, if the latest master code
is tested with the same test after the problematic CI change, then that
master test will fail. Nothing a PR can do in this situation can fix
this kind of failure because it is not PR changes that are causing the
failure -- CI changes broke the master branch, not just the PR! This
kind of failures are the responsibility of CI administrators, and PR
authors should complain about them, especially when there are no signs
of CI administrators aware of and working on addressing the problem.

A good example of a failure of the second kind a -Wrange-loop-construct
error in a PR that does not touch any range loops (Jenkins conveniently
deleted the actual failed test, but my GitHub comment and PR contents
may be enough to restore what happened):
https://github.com/squid-cache/squid/pull/806#issuecomment-827924821


[I am skipping the exaggerations (about other people exaggerations) that
were, AFAICT, driven by mis-classifying the failures of the second kind
as regular PR failures of the first kind.]


>> What this means in terms of sysadmin steps for doing upgrades is up to
>> you. You are doing the hard work here, so you can optimize it the way
>> that works best for _you_. If really necessary, I would not even object
>> to trial upgrades (that may break master for an hour or two) as long as
>> you monitor the results and undo the breaking changes quickly and
>> proactively (without relying on my pleas to fix Jenkins to detect
>> breakages). I do not know what is feasible and what the best options
>> are, but, again, it is up to _you_ how to optimize this (while observing
>> the invariants).


> Uhm. Respectfully, from my perspective the above paragraph conflicts
> directly with actions taken.

My paragraph is not really about any taken actions so there cannot be
any such conflict AFAICT.


> From what I can tell kinkie (as sysadmin) *has* been making a new node
> and testing it first. Not just against master but the main branches and
> most active PRs before adding it for the *post-merge* matrix testing
> snapshot production.

The above summary does not match the (second kind of) PR test failures
that I have observed and asked Francesco to address. Those failures were
triggered by the latest master code, not PR changes. No PR changes would
have fixed those test failures. In fact, an "empty" PR that introduces
no code changes at all would have failed as well. See above for one
recent example.


>   But still threads like this one with complaints appear.

This squid-dev thread did not contain any complaints AFAICT. Francesco
is trying to get consensus on how to handle CI upgrades/changes. He is
doing the right thing and nobody is complaining about it.


> I understand there is some specific pain you have encountered to trigger
> the complaint. Can we get down to documenting as exactly as possible
> what the particular pain was?

Well, I apparently do not know what you call "complaint", so I cannot
answer this exact question, but if you are looking for a recent PR test
failure that triggered this squid-dev thread, then please see the GitHub
link above.



> Test designs that do not fit into our merge and release process seque

Re: [squid-dev] Strategy about build farm nodes

[Alex Rousskov]

On 5/3/21 12:41 AM, Francesco Chemolli wrote:
> - we want our QA environment to match what users will use. For this
> reason, it is not sensible that we just stop upgrading our QA nodes, 

I see flaws in reasoning, but I do agree with the conclusion -- yes, we
should upgrade QA nodes. Nobody has proposed a ban on upgrades AFAICT!

The principles I have proposed allow upgrades that do not violate key
invariants. For example, if a proposed upgrade would break master, then
master has to be changed _before_ that upgrade actually happens, not
after. Upgrades must not break master.

What this means in terms of sysadmin steps for doing upgrades is up to
you. You are doing the hard work here, so you can optimize it the way
that works best for _you_. If really necessary, I would not even object
to trial upgrades (that may break master for an hour or two) as long as
you monitor the results and undo the breaking changes quickly and
proactively (without relying on my pleas to fix Jenkins to detect
breakages). I do not know what is feasible and what the best options
are, but, again, it is up to _you_ how to optimize this (while observing
the invariants).


> - I believe we should define four tiers of runtime environments, and
> reflect these in our test setup:

>  1. current and stable (e.g. ubuntu-latest-lts).
>  2. current (e.g. fedora 34)
>  3. bleeding edge
>  4. everything else - this includes freebsd and openbsd

I doubt this classification is important to anybody _outside_ this
discussion, so I am OK with whatever classification you propose to
satisfy your internal needs.


> I believe we should focus on the first two tiers for our merge workflow,
> but then expect devs to fix any breakages in the third and fourth tiers
> if caused by their PR,

FWIW, I do not understand what "focus" implies in this statement, and
why developers should _not_ "fix any breakages" revealed by the tests in
the first two tiers.

The rules I have in mind use two natural tiers:

* If a PR cannot pass a required CI test, that PR has to change before
it can be merged.

* If a PR cannot pass an optional CI test, it is up to PR author and
reviewers to decide what to do next.

These are very simple rules that do not require developer knowledge of
any complex test node tiers that we might define/use internally.

Needless to say, the rules assume that the tests themselves are correct.
If not, the broken tests need to be fixed (by the Squid Project) before
the first bullet/rule above can be meaningfully applied (the second one
is flexible enough to allow PR author and reviewers to ignore optional
test failures).


> Breakages due to changes in nodes (e.g. introducing a new distro
> version) would be on me and would not stop the merge workflow.

What you do internally to _avoid_ breakage is up to you, but the primary
goal is to _prevent_ CI breakage (rather than to keep CI nodes "up to
date"!). If CI is broken, then it is the responsibility of the Squid
Project to fix CI ASAP. Thank you for assuming that responsibility as
far as Jenkins tests are concerned.

There are many ways to break CI and detect those breakages, of course,
but if master cannot pass required tests after a CI change, then the
change broke CI.


> What I would place on each individual dev is the case where a PR breaks
> something in the trunk-matrix,trunk-arm32-matrix, trunk-arm64-matrix,
> trunk-openbsd-matrix, trunk-freebsd-matrix builds, even if the 5-pr-test
> and 5-pr-auto builds fail to detect the breakage because it happens on a
> unstable or old platform. 

This feels a bit out of topic for me, but I think you are saying that
some CI tests called trunk-matrix, trunk-arm32-matrix,
trunk-arm64-matrix, trunk-openbsd-matrix, trunk-freebsd-matrix should be
classified as _required_. In other words, a PR must pass those CI tests
before it can be merged. Is that the situation today? Or are you
proposing some changes to the list of required CI tests? What are those
changes?


Thank you,

Alex.
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] Strategy about build farm nodes

[Alex Rousskov]

On 4/28/21 5:12 PM, Amos Jeffries wrote:
> I'm not sure why this is so controversial still. We have already been
> over these and have a policy from last time:

Apparently, the recollections of what was agreed upon, if anything,
during that "last time" differ. If you can provide a pointer to that
"last time" agreement, please do so.


> * dev PR submissions use the volatile 5-pr-test, after test approval by
> anyone in QA. Check against unstable OS nodes, as many as possible.
> Kinkie adds/removes from that set as upgrades fix or break at CI end of
> things.

I do not know how to interpret the last sentence correctly, but, IMO, we
should not add or change nodes if doing so breaks master tests. AFAICT
from PR 806 discussion[1], Francesco thinks that it is not a big deal to
do so. The current discussion is meant to resolve that disagreement.

[1] https://github.com/squid-cache/squid/pull/806#issuecomment-827937563


> * anubis auto branch tested by curated set of LTS stable nodes only.

FWIW, the above list and the original list by Francesco appears to focus
on distro stability, popularity, and other factors that are largely
irrelevant to the disagreement at hand. The disagreement is whether it
is OK to break master (and, hence, all PR) tests by changing CI. It does
not matter whether that CI change comes from an upgrade of an "LTS
stable node", "unstable node", or some other source IMO. Breaking
changes should not be allowed (in the CI environments under our
control). If they slip through despite careful monitoring for change
effects, the breaking CI changes should be reverted.

Alex.
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] Strategy about build farm nodes

[Alex Rousskov]

On 4/28/21 1:45 AM, Francesco Chemolli wrote:

>   I'm moving here the discussion from PR #806 about what strategy to
> have for CI tests, looking for an agreement.

> We have 3 classes of tests ni our CI farm
> (https://build.squid-cache.org/)

> - PR staging tests, triggered by commit hooks on GitHub (possibly with
> human approval)

>    the job is 5-pr-auto (misnamed, it tests trunk with the PR applied).

FYI: The "auto" branch is "master branch + PR". When PR is merged,
master becomes identical to the auto branch. The name "auto" is fairly
common in CI AFAICT. In Anubis (and relatively widespread/common)
terminology[1], the words "staged" and "staging" mean what you describe
as "trunk with the PR applied" below.

[1]
https://github.com/measurement-factory/anubis/blob/master/README.md#pull-request-merging-algorithm

AFAICT, this class should _follow_ the 5-pr-test class (described below)
because a PR is not staged until it is approved and passes 5-pr-test checks.


>    To be successful, it needs to successfully compile and unit-test all
> combinations of clang and gcc on the latest stable version of the most
> popular linux distros, at this time centos-8, debian-testing,
> fedora-33, opensuse-leap, ubuntu-focal


> - PR build tests run , after a PR is approved, also triggered by GitHub
>   the job is 5-pr-test

AFAICT, this job tests the "GitHub-generated merge commit for the PR
branch". That commit may become stale if master advances while there
were no PR branch commits. Unlike the GitHub-generated merge commit, the
"auto" branch (mentioned above) is kept in sync with master by Anubis.

We do not merge the GitHub-generated merge commit into master.

If 5-pr-test requires a PR approval, then something probably went wrong
somewhere. Testing PR branch or the GitHub-generated merge commit should
not require a PR approval. Perhaps you meant the "OK to test" _testing_
approval. That testing approval is indeed required and expected for
testing GitHub-generated merge commits on vulnerable Jenkins.


>   To be successful, it needs to compile and unit-test all combinations
> of clang and gcc on LTS and most recent versions of most popular linux
> distros, at this time: centos-7,
> debian-stable, debian-unstable, fedora-32, fedora-34, ubuntu-bionic, 
> ubuntu-hirsute

> - full-scale build tests (jobs: trunk-matrix, trunk-arm32-matrix,
> trunk-arm64-matrix, trunk-freebsd13-clang-matrix, trunk-openbsd-matrix)
>   these test everything we can test, including bleeding edge distros
> such as getntoo, rawhide, tumbleweed, and the latest compilers. 

Please clarify what branches/commits these "full-scale build tests" test.

Ideally, all this should be documented somewhere on Squid wiki!


> Failing
> these will not block a PR from mergeing, but there is an expectation
> that build regressions will be fixed

If we expect a fix, then the failure has to block something important,
like the responsible PR (ideally), the next numbered release, or all
PRs. What does this failure block today, if anything?


> The change in policy since last week

There was no change in any Project policies AFAIK. You have changed
Jenkins setup, but that is not a change in policy. It is likely that
there is no policy at all right now -- just ad hoc decisions by
sysadmins. It would be a stretch to call that current approach a
"policy", but even if we do call it that, then that "policy" has not
"changed" :-).

Alternatively, Jenkins configuration changes have violated a Project
policy (one cannot _change_ a Project policy unilaterally; one can only
violate it). I prefer the "no policy" version above though :-).


> is that the PR-blocking builds used
> to depend on unstabledistros (fedora-rawhide and opensuse-tumbleweed),
> I've removed that today as part of the discussion on PR #806
> This policy would allow keeping stable distros uptodate and matching
> expected user experience, while not introducing instability that would
> come in from the unstable distros
> One distro that's notably missing is centos-stream, this is due to
> technical issues with getting a good docker image for it, when available
> I'll add it

> Would this work as a general policy?

I am not sure what exact general policy you are proposing -- please
extract it from the lengthy prose above (removing the ever-changing
low-level specifics to the extent possible, to arrive at something truly
"general"). FWIW, if the fleshed out proposal violates the existing "do
not break master" principle below, then I do not think it will work
well. I suspect that a focus on vague optimization targets like
"expected user experience" will not allow us to formulate a usable
policy we can all agree on, but I would be happy to be proven wrong.


I can propose the following. Perhaps knowing my state of mind will help
you finalize your proposal.

We already have a "do not break master" principle:

* Master always passes all current development-related tests.


I propose to add the following 

Re: [squid-dev] Forcing interception(transparent) mode, disabling NS lookups, and 'secretly' forwarding connections

[Alex Rousskov]

On 4/4/21 7:06 PM, Joshua Rogers wrote:
> I ended up finding a solution.
> 
> http->uri in the ConnStateData::parseHttpRequest function can simply be
> rewritten to be http://localhost:80/ . You can
> also manually set COMM_INTERCEPTION a little bit before that.

You may also be able to use a cache_peer or URL rewriter for this.

Alex.


> On Sun, Apr 4, 2021 at 11:31 PM Joshua Rogers wrote:
> 
> Hi all,
> 
> I have an extremely specific question about manipulating Squid to
> run to in a very specific way.
> Due to the complexity, I will gladly accept a "not possible" answer.
> 
> I need to make the following changes to Squid, or somehow manipulate
> Squid to:
> 
> 1) Always consider a client intercepted. This would mean
> that COMM_INTERCEPTION is set for every client no matter what.
> 
> 2) NS lookups for domains needs to be disabled or simply return
> localhost. I have considered setting AI_NUMERICHOST flag for every
> getaddrinfo() call, but I don't think this function is used for
> making connections to a website (gethostbyname seems to be used in
> some places).
> 
> 3) Every outward connection Squid makes needs to be forwarded to
> localhost on a specific port.
> 
> 
> Why do I need to do this?
> I have a large set of files which contain HTTP requests (headers
> included).
> I have a large set of files which contain HTTP responses (headers
> included).
> 
> I would like to loop the HTTP requests, sending each request to
> Squid, and then I would like to run a loop, responding with each of
> the HTTP responses I have stored. The idea is to get a nice code
> coverage report which I can use for research later on.
> 
> Like I said, this is a bit complicated, but I thought I would act,
> perhaps somebody has an interesting idea how to do this :-). It will
> most certainly require me to somehow defile the source code, but
> this is just temporary and is nothing to do with a production service.
> 
> Happy to hear any ideas.
> 
> Cheers,
> Josh

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] Squid 5.0.5 - compilation errors

[Alex Rousskov]

On 2/10/21 5:32 PM, Lubos Uhliarik wrote:
> cp: cannot create regular file 'tests/stub_debug.cc': No such file or 
> directory

If you are using parallel make, then this is probably bug #5060:
https://bugs.squid-cache.org/show_bug.cgi?id=5060

Alex.
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] effective acl for tcp_outgoing_address

[Alex Rousskov]

On 1/19/21 11:35 PM, Hideyuki Kawai wrote:

> I would like to map ip address based on username which is required proxy_auth 
> (kerberos).
> Because, based on the squid docs 
> (http://www.squid-cache.org/Doc/config/tcp_outgoing_address/), 
> tcp_outgoing_address can map based on "username".
> However, such acl types (ident, proxy_auth) are slow type. 
> (http://www.squid-cache.org/Doc/config/acl/)
> So, it seems also can not use to cp_outgoing_address (I confused.)

You should be discussing this on squid-users, not squid-dev.


I hear several questions in your email:

Q1: Are ident and proxy_auth ACLs slow?

Answer: They are slow when the transaction first encounters them because
the first encounter triggers various (slow/asynchronous) authentication
procedures.

However, IIRC, these ACLs become fast when the same transaction has been
 successfully authenticated already -- post-authentication, Squid just
needs to check whether the already authenticated user matches the name
in the ACL parameter. That check can be performed instantaneously.

You may see the first authentication-related rule to have a special
REQUIRED keyword instead of a specific user name. Once that rule is
(slowly) evaluated for a transaction, that transaction can treat
specific authentication ACLs as "fast".


Q2: How to use a slow ACL with a directive that only supports fast ACLs?

Answer: Do not use a slow ACL with a directive that only supports fast
ACLs. Instead, use a slow ACL with an earlier directive that supports
slow ACLs _and_ remember the result of that earlier evaluation. The
remember the result of any ACL evaluation, use annotate_transaction or
annotate_client ACLs. To recall the that result, use a note ACL.

Every transaction in Squid goes through a series of directives. In most
cases, if your directive DF cannot handle slow ACLs, there is an earlier
directive DS that can:

  * directive1
  * directive2
  ...
  * http_access
  ...
  * tcp_outgoing_address
  ...
  * directive30
  ...

In many cases, http_access is a directive that is suitable for slow ACL
evaluation. YMMV.


Q3: Would not evaluating an ACL in the "wrong" directive change
transaction processing?

Answer: By itself, ACL evaluation does not trigger directive
application. The directive is applied only if all first-level ACLs in
the directive rule match. If necessary, this can be used to successfully
evaluate an ACL without triggering the directive application.

For example, the following http_access rule will never match, but, if it
is reached (i.e. if http_access is evaluated but all previous rules, if
any, do not match), it will annotate the transaction (or the client) as
transaction (or client) that should be using an specific outgoing address.

  acl userIsBob proxy_auth Bob
  acl markToUseAddressX annotate_client address=x
  http_access deny userIsBob markToUseAddressX !all

  acl markedToUseAddressX note address x
  tcp_outgoing_address x markedToUseAddressX


If you need to do many annotations, then you can either create many
http_access rules or, using basic boolean logic and all-of and any-of
ACLs, it is possible to group all those annotations into one top-level ACL:

  http_access deny annotateAsNeeded !all

Again, nothing is denied here.


HTH,

Alex.



> 
> -Original Message-
> From: Alex Rousskov  
> Sent: Thursday, January 14, 2021 11:25 PM
> To: squid-dev@lists.squid-cache.org
> Cc: Hideyuki Kawai（川井秀行） 
> Subject: Re: [squid-dev] effective acl for tcp_outgoing_address
> 
> On 1/13/21 7:47 PM, Hideyuki Kawai wrote:
> 
>> 1. "external_acl" can not use on tcp_outgoing_address. Because the 
>> external_acl type is slow. My understanding is correct?
> 
> 
> Yes, your understanding is correct. There are cases where a slow ACL "usually 
> works" with a tcp_outgoing_address directive due to ACL caching side effects, 
> and there are many examples on the web abusing those side effects, but you 
> should not rely on such accidents when using modern Squid versions.
> 
> 
>> 2. If yes, how to solve my requirement?
> 
> Use an annotation approach instead. The "note" ACL is fast, and the external 
> ACL helper can annotate transactions (and connections) in modern Squids. The 
> only difficulty with this approach is to find a directive that satisfies all 
> of the conditions below:
> 
> 1. supports slow ACLs
> 2. evaluated after the info needed by the external ACL helper is known 3. 
> evaluated before tcp_outgoing_address
> 
> In many cases, http_access is such a directive, but YMMV.
> 
> 
> HTH,
> 
> Alex.
> P.S. FWIW, I can agree with one Eliezer statement on this thread: This thread 
> belongs to squid-users, not squid-dev.
> 

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] effective acl for tcp_outgoing_address

[Alex Rousskov]

On 1/13/21 7:47 PM, Hideyuki Kawai wrote:

> 1. "external_acl" can not use on tcp_outgoing_address. Because the
> external_acl type is slow. My understanding is correct?


Yes, your understanding is correct. There are cases where a slow ACL
"usually works" with a tcp_outgoing_address directive due to ACL caching
side effects, and there are many examples on the web abusing those side
effects, but you should not rely on such accidents when using modern
Squid versions.


> 2. If yes, how to solve my requirement?

Use an annotation approach instead. The "note" ACL is fast, and the
external ACL helper can annotate transactions (and connections) in
modern Squids. The only difficulty with this approach is to find a
directive that satisfies all of the conditions below:

1. supports slow ACLs
2. evaluated after the info needed by the external ACL helper is known
3. evaluated before tcp_outgoing_address

In many cases, http_access is such a directive, but YMMV.


HTH,

Alex.
P.S. FWIW, I can agree with one Eliezer statement on this thread: This
thread belongs to squid-users, not squid-dev.
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] [squid-users] Host header forgery detected on domain: mobile.pipe.aria.microsoft.com

[Alex Rousskov]

On 1/6/21 10:26 PM, Eliezer Croitoru wrote:

> The main issue now is the extensive logging.

Please note that excessive logging may be the main issue for you and
_some_ Squid admins. For many, the main issue is denied transactions.
For some, it is cache misses. For some, it is a combination of things.
These facts make any single simple solution inapplicable to some use cases.

For example, you can trivially decrease the number (or, with a few more
code lines, frequency) of these messages, but it will not help with the
other problems I mentioned. FWIW, Factory is working on making all
level-0/1 messages configurable that way, but we need more time to
finish that project.

This does not mean that any simple partial solution is going to be
rejected, but please be very careful/specific in defining the problem
when proposing (or asking for) a solution.


> Should we continue this on Squid-dev?

IMO, if you are going to discuss the problem and possible
functionality-level solutions, then squid-users may be the best place
for that. If you are going to discuss code changes and similar
developer-level issues, use squid-dev.

Alex.


> -Original Message-
> From: Alex Rousskov  
> Sent: Wednesday, January 6, 2021 10:42 PM
> To: squid-us...@lists.squid-cache.org
> Cc: Eliezer Croitoru 
> Subject: Re: [squid-users] Host header forgery detected on domain: 
> mobile.pipe.aria.microsoft.com
> 
> On 1/6/21 2:49 PM, Eliezer Croitoru wrote:
> 
>> I am trying to think about the right solution for the next issue:
>> SECURITY ALERT: Host header forgery detected on conn18767
>> local=52.114.32.24:443 remote=192.168.189.52:65107 FD 15 flags=33 (local IP
>> does not match any domain IP)
> 
> As you know, this has been discussed many times on this list before,
> including recently[1]. I doubt anything has changed since then.
> 
> [1]
> http://lists.squid-cache.org/pipermail/squid-users/2020-November/022912.html
> 
> 
>> All of the hosts use the same DNS service in the LAN however for some reason
>> both squid and the client are resolving different addresses
>> in a period of  10  Seconds.
> 
> The "however for some reason" part feels misleading to me -- what you
> observe is the direct, expected consequence of the low-TTL environment
> you have described. There is no contradiction or uncertainty here AFAICT.
> 
> 
>> The solution I am thinking is to force a minimum of 60 seconds caching using
>> dnsmasq or another caching service.
> 
> FTR: Increasing DNS response TTL will reduce the number/probability of
> false positives in forged Host header detection. No more. No less.
> 
> 
>> Can we teach (theoretically) squid a way to look at these short TTLs as
>> something to decide by an ACL?
> 
> Yes, it is possible. There is positive_dns_ttl already which specifies
> an upper bound. One could add a similar positive_dns_ttl_min option that
> would specify the lower bound. Like virtually any Squid directive, it
> can be made conditional on ACLs.
> 
> IMO, violating DNS TTLs is not the right solution for this problem
> though. See my response on the [1] thread for a better medium-term solution.
> 
> 
> HTH,
> 
> Alex.
> 
> ___
> squid-users mailing list
> squid-us...@lists.squid-cache.org
> http://lists.squid-cache.org/listinfo/squid-users
> 

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] File descriptor leak at ICAP reqmod rewrites of CONNECT requests

[Alex Rousskov]

On 12/10/20 3:33 PM, Alexey Sergin wrote:

> - Squid writes to cache.log a message like "kick abandoning <>";

These messages indicate a Squid bug, most likely in REQMOD request
satisfaction implementation specific to CONNECT use cases. The messages
are not prefixed with a "BUG" label, but they should be.


> - Squid does not close the file descriptor used for http client connection.

Yes, that is a likely side effect of the above-mentioned bug.


> client_side.cc. Closing clientConnection right after
> "debugs(<>abandoning<>)" fixes the leak.

> Is it ok to always close() clientConnection when "abandoning" thing
> happens? 

> Are there any known scenarios where this close() would be
> inappropriate?

Unknown. Such a closure (alone) is not a proper fix. If well-tested, it
may be considered to be a good-enough workaround, but no more than that.

What is currently missing (at least) is understanding of what is going
on. The correct fix, whatever it is, would be determined by that
understanding.

In general, responding with a 403 does not invalidate the client
connection, so it does not have to be closed. Said that, I am sure there
are use cases where such closure would be a good idea. I would not be
surprised if Squid closes the connection after denying regular requests.


> Could you please give me some advice on a better/proper fix, if close()
> at "abandoning" time is wrong?

Unfortunately, I cannot. Somebody needs to investigate the problem and
identify the bug in ConnStateData::kick() logic (or elsewhere).

Alex.
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] forwarded_for based on acls

[Alex Rousskov]

On 11/3/20 5:58 AM, Eliezer Croitor wrote:

> I believe that the `forwarded_for` and the `via` config should be
> converted to an ACL style one.

Sure, (optional) ACL support in forwarded_for and via directives is an
improvement worth accepting. It should be straightforward to implement
as long as support is limited to fast ACLs. There are many similar
examples in the existing code.

This addition can (and must) be done in a backward compatible way. It
can (and, IMO, should) be done in a forward compatible way by requiring
an "if" keyword before the ACLs (we are using such an approach for the
upcoming tls_key_log feature detailed at
http://lists.squid-cache.org/pipermail/squid-dev/2020-July/009605.html).


Cheers,

Alex.
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] rfc1738.c

[Alex Rousskov]

On 10/29/20 7:41 AM, Amos Jeffries wrote:

> The latest Squid have AnyP::Uri::Encode() whic uses a caller provided
> buffer.

Just to avoid misunderstanding: AnyP::Uri::Encode() uses a
caller-provided buffer as _input_. So does rfc1738*_escape(), of course.
That kind of input is unavoidable and is not really the issue that
prompted my earlier comments about rfc1738_do_escape() dangers.

The primary in-scope problem with rfc1738*_escape() is the _output_
interface. If those functions cannot be completely removed, they should
return an SBuf. The exact best input interface will depend on the
callers, but it will always remain a "caller provided buffer".


HTH,

Alex.
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] rfc1738.c

[Alex Rousskov]

On 10/29/20 7:17 AM, Damian Wojslaw wrote:

> It was mentioned that rfc1738_do_escape could use changing so it doesn't
> return static buffer.

Yes. Most likely, rfc1738 family of functions should return an SBuf, but
refactoring that may require a serious effort, on several layers. This
is not the easiest Squid XXX to fix by far!

However, please do not misinterpret my comment as a NACK. Just be
prepared to move rfc1738 code into src/ where it belongs and work on
refactoring the API to remove those ugly macros and rework a
difficult-to-grok set of positive and negative flags.


> I'm also interested in getting my hands in cachegr.cc, as it was
> described as neglected in the PR.

If there may be consensus that cachemgr.cc should be removed, then
consider confirming that consensus (here) and removing cachemgr. Doing
so may be a lot easier than fixing rfc1738_do_escape().


Thank you,

Alex.
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] Jenkins situation

[Alex Rousskov]

On 8/4/20 9:26 PM, Amos Jeffries wrote:

> With the recent Jenkins randomly failing builds due to git pull / fetch
> failures I am having to selectively disable the PR Jenkins block on PR
> merging for some hrs.
> 
> Please do not mark any PRs with "M-cleared-for-merge" until further
> notice. I will do this myself with coordination on Jenkins results. You
> can use the "waiting-for-committer" instead if necessary.

I will cooperate until the end of August, but I consider the suggested
procedure excessively restrictive and not precedent-setting.

If Jenkins build testing does not become stable enough by the end of
August, we should make it optional (until it becomes stable again).


@kinkie, I do not know enough about the root cause of Jenkins failures
to suggest any specific fixes, but, if you have not already, please
consider (temporary) removing some of the Jenkins tests/nodes _if_ doing
so would bring the overall Jenkins stability to acceptable levels. IMHO,
it is better to have one working build test node than ten frequently
failing ones.


Thank you,

Alex.
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] RFC: tls_key_log: report TLS pre-master secrets, other key material

[Alex Rousskov]

On 7/30/20 6:28 AM, Amos Jeffries wrote:
>> On 7/15/20 3:14 PM, Alex Rousskov wrote:
>>> I propose to add a new tls_key_log directive to record TLS
>>> pre-master secret (and related encryption details) for to- and
>>> from-Squid TLS connections. This very useful triage feature is common
>>> for browsers and some networking tools. Wireshark supports it[1]. You
>>> might know it as SSLKEYLOGFILE. It has been requested by several Squid
>>> admins. A draft documentation of the proposed directive is at the end of
>>> this email.
>>>
>>> [1] https://wiki.wireshark.org/TLS#Using_the_.28Pre.29-Master-Secret
>>>
>>> If you have any feature scope adjustments, implementation wishes, or
>>> objections to this feature going in, please let me know!


> Two design points:
> 
> 1) It seems to me these bits are part of the handshake. So would come in
> either as members/args of the %handshake logformat macros (some not yet
> implemented) or as secondary %handshake_foo macros in the style %cert_*
> macros use.

Sure, if somebody wants to add a new logformat %code to log secrets,
they should do that. It would not be a good solution for the problems
tls_key_log is solving, but it could be useful for other reasons, of
course. That addition will probably be able to reuse some of the
tls_key_log code as well.

As for reusing %handshake for such logging, I am not sure: Connection
secrets cannot be a part of the plain text handshake (for obvious
reasons). Logging encrypted secrets does not help. If somebody wants to
add plain text secrets logging via a new %handshake parameter, they
should double check whether all the secrets are truly a part of the
encrypted handshake -- I suspect that the handshake actually ends sooner
and/or contains secret _derivatives_.

At any rate, these details are all outside the tls_key_log scope. We do
not need to investigate or agree on them right now AFAICT. If I missed
your point, please clarify.


> 2) Please do use the logging logic implemented for access_log, just with
> the next directive as list of log outputs to write at the appropriate
> logging trigger time.

Sorry, I do not understand what the above paragraph means. The term
"logging logic implemented for access_log" is so broad that I cannot
tell what you are trying to subtract or add to the proposed tls_key_log
directive interface and/or its implementation. If this is already
covered by the specific discussion below, then just skip to that!


> I accept the reasoning for not using access_log directive. This will
> need a new log directive with different times when it triggers output
> written there. 

Yes.


> However (most of) the implementation logic of access_log
> should be usable for this new output.

I see no reason to repeat access_log interface mistakes (e.g., the
special "none" destination) and no need to support some of the
powerful/complex access_log features in tls_key_log (e.g., logformat),
but perhaps you are not asking for any of that. Please be more specific
if this is not covered by the discussion below.


>>> tls_key_log  if ...

> Please allow extension points for options and modules:
> 
>   tls_key_log stdio: [options] if ...

We will requite the ugly "stdio:" suffix to avoid arguing about it.

Future addition of optional parameters was already supported; there were
just no such options until you requested rotate=N below.


>>> At most one log file is supported at this time. Repeated tls_key_log
>>> directives are treated as fatal configuration errors. By default, no
>>> log is created or updated.

> With ACL support it seems reasonable to support multiple logs. We should
> be able to re-use (with minor change to pass the list of log outputs to
> the function) the logic access_log has for writing to a list of outputs.

I agree that supporting multiple tls_key_log is reasonable. I would
leave that feature for a future seamless improvement -- that is why
repeated tls_key_log directives are treated as a fatal configuration
error for now.

The existing multi-log logic for access_logs is confusing for admins and
has serious implementation bugs. I do not think we should model the new
feature on that interface or code. However, we do not need to agree on
this aspect. It can and should be decided later, driven, in part, by
real use cases for multilog support.


>>> This log is rotated based on the logfile_rotate settings.

> Please don't use solely that directive. The new directive should have a
> rotate=N option of its own. Only using the global directive as a default
> if that option is unset.

Will add to avoid arguing about it.


Thank you,

Alex.
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] RFC: tls_key_log: report TLS pre-master secrets, other key material

[Alex Rousskov]

On 7/15/20 3:14 PM, Alex Rousskov wrote:

> I propose to add a new tls_key_log directive to record TLS
> pre-master secret (and related encryption details) for to- and
> from-Squid TLS connections. This very useful triage feature is common
> for browsers and some networking tools. Wireshark supports it[1]. You
> might know it as SSLKEYLOGFILE. It has been requested by several Squid
> admins. A draft documentation of the proposed directive is at the end of
> this email.
> 
> [1] https://wiki.wireshark.org/TLS#Using_the_.28Pre.29-Master-Secret
> 
> If you have any feature scope adjustments, implementation wishes, or
> objections to this feature going in, please let me know!


FYI: Factory is starting to implement this feature.

Alex.


> FTR, we have considered providing similar support by adding new
> logformat %code(s) to the existing access_log. While doing so would
> reduce and simplify code changes, we ultimately rejected that design
> because the combination of the following factors renders it inferior and
> insufficient on several levels:
> 
> 1. Some TLS connections are reflected in access logs a long time
>after they are established (e.g., when the connection serves
>a long CONNECT tunnel). During triage, admins may need the
>ability to decipher a live connection much sooner.
> 
> 2. Some TLS connections are never reflected in access logs at all
>(e.g., when Squid opens but does not use an outgoing TLS connection
>or crashes when parsing the first request on an incoming one). Info
>gaps often create triage suspicions: Did we drop something else?
> 
> 3. A single access_log record may correspond to many from-Squid
>connections, especially when Squid retries peer failures. Logging
>keys for all these connections would require accumulating the keys in
>the master transaction and then dumping them as a part of a new
>%code. Adding (and dumping) repeated ALE fields is awkward.
> 
> 4. Manually creating ACLs to limit access log records to the first
>transaction on a connection would be a must for most deployments
>using this feature. Doing so is far from trivial and related
>configuration errors are difficult to triage. We could add a new
>ACL type for this purpose, but even that is tricky because a
>single master transaction may have many associated connections.
>And logging secrets for every transaction creates too much noise.
> 
> 5. Configuration flexibility offered by logformat is likely to
>remain largely unused by the new feature because tools like
>Wireshark _automatically_ find relevant records when deciphering
>captured traffic. Augmenting these logs with other transaction
>details (typical for access log uses) would be mostly useless.
> 
> 6. New %codes would be awkward to use in a regular access log because
>they may expand into a variable number of lines, going against the
>traditional line-oriented, "fixed" format access log use.
> 
> While some of the above items have workarounds, a few do not, and the
> whole combination looks rather grim/unfriendly. We should attack this
> problem from the other end -- a new simple configuration dedicated to
> this useful feature.
> 
> We propose to structure this new directive so that it is easy to add
> advanced access_log-like features later if needed (while reusing the
> corresponding access_log code). For example, if users find that they
> want to maintain multiple TLS key logs or augment log records with
> connection details, we can add that support by borrowing access_log
> options and code without backward compatibility concerns. The new
> required "if" keyword in front of the ACL list allows for seamless
> addition of new directive options in the future.
> 
> 
> Cheers,
> 
> Alex.
> -- draft squid.conf directive documentation 
> 
> tls_key_log
> 
> Configures whether and where Squid records pre-master secret and
> related encryption details for TLS connections accepted or established
> by Squid. These connections include connections accepted at
> https_port, TLS connections opened to origin servers/cache_peers/ICAP
> services, and TLS tunnels bumped by Squid using the SslBump feature.
> This log (a.k.a. SSLKEYLOGFILE) is meant for triage with traffic
> inspection tools like Wireshark.
> 
> tls_key_log  if ...
> 
> WARNING: This log allows anybody to decrypt the corresponding
> encrypted TLS connections, both in-flight and postmortem.
> 
> At most one log file is supported at this time. Repeated tls_key_log
> directives are treated as fatal configuration errors. By default, no
> log is created or updated.
> 
> If the log file does not exist

[squid-dev] RFC: tls_key_log: report TLS pre-master secrets, other key material

[Alex Rousskov]

Hello,

I propose to add a new tls_key_log directive to record TLS
pre-master secret (and related encryption details) for to- and
from-Squid TLS connections. This very useful triage feature is common
for browsers and some networking tools. Wireshark supports it[1]. You
might know it as SSLKEYLOGFILE. It has been requested by several Squid
admins. A draft documentation of the proposed directive is at the end of
this email.

[1] https://wiki.wireshark.org/TLS#Using_the_.28Pre.29-Master-Secret

If you have any feature scope adjustments, implementation wishes, or
objections to this feature going in, please let me know!


FTR, we have considered providing similar support by adding new
logformat %code(s) to the existing access_log. While doing so would
reduce and simplify code changes, we ultimately rejected that design
because the combination of the following factors renders it inferior and
insufficient on several levels:

1. Some TLS connections are reflected in access logs a long time
   after they are established (e.g., when the connection serves
   a long CONNECT tunnel). During triage, admins may need the
   ability to decipher a live connection much sooner.

2. Some TLS connections are never reflected in access logs at all
   (e.g., when Squid opens but does not use an outgoing TLS connection
   or crashes when parsing the first request on an incoming one). Info
   gaps often create triage suspicions: Did we drop something else?

3. A single access_log record may correspond to many from-Squid
   connections, especially when Squid retries peer failures. Logging
   keys for all these connections would require accumulating the keys in
   the master transaction and then dumping them as a part of a new
   %code. Adding (and dumping) repeated ALE fields is awkward.

4. Manually creating ACLs to limit access log records to the first
   transaction on a connection would be a must for most deployments
   using this feature. Doing so is far from trivial and related
   configuration errors are difficult to triage. We could add a new
   ACL type for this purpose, but even that is tricky because a
   single master transaction may have many associated connections.
   And logging secrets for every transaction creates too much noise.

5. Configuration flexibility offered by logformat is likely to
   remain largely unused by the new feature because tools like
   Wireshark _automatically_ find relevant records when deciphering
   captured traffic. Augmenting these logs with other transaction
   details (typical for access log uses) would be mostly useless.

6. New %codes would be awkward to use in a regular access log because
   they may expand into a variable number of lines, going against the
   traditional line-oriented, "fixed" format access log use.

While some of the above items have workarounds, a few do not, and the
whole combination looks rather grim/unfriendly. We should attack this
problem from the other end -- a new simple configuration dedicated to
this useful feature.

We propose to structure this new directive so that it is easy to add
advanced access_log-like features later if needed (while reusing the
corresponding access_log code). For example, if users find that they
want to maintain multiple TLS key logs or augment log records with
connection details, we can add that support by borrowing access_log
options and code without backward compatibility concerns. The new
required "if" keyword in front of the ACL list allows for seamless
addition of new directive options in the future.


Cheers,

Alex.
-- draft squid.conf directive documentation 

tls_key_log

Configures whether and where Squid records pre-master secret and
related encryption details for TLS connections accepted or established
by Squid. These connections include connections accepted at
https_port, TLS connections opened to origin servers/cache_peers/ICAP
services, and TLS tunnels bumped by Squid using the SslBump feature.
This log (a.k.a. SSLKEYLOGFILE) is meant for triage with traffic
inspection tools like Wireshark.

tls_key_log  if ...

WARNING: This log allows anybody to decrypt the corresponding
encrypted TLS connections, both in-flight and postmortem.

At most one log file is supported at this time. Repeated tls_key_log
directives are treated as fatal configuration errors. By default, no
log is created or updated.

If the log file does not exist, Squid creates it. Otherwise, Squid
appends an existing log file.

The directive is consulted whenever a TLS connection is accepted or
established by Squid. TLS connections that fail the handshake may be
logged if Squid got enough information to form a log record. A record
is logged only if all of the configured ACLs match.

Squid does not buffer these log records -- the worker blocks until
each record is written. File system buffering may speed things up, but
consider placing this triage log in a memory-based partition.

This log is rotated based on the logfile_rotate