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

2022-12-05 Thread 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

2022-12-04 Thread 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

2022-10-24 Thread 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

2022-10-19 Thread 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

2022-09-22 Thread 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

2022-09-20 Thread 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

2022-09-19 Thread 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

2022-08-02 Thread 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

2022-07-31 Thread 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_

2022-06-23 Thread 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

2022-06-23 Thread 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

2022-06-08 Thread 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

2022-06-06 Thread 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

2022-05-02 Thread 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

2022-04-23 Thread 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

2022-04-20 Thread 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

2022-03-04 Thread 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

2022-01-30 Thread 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

2022-01-24 Thread 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

2022-01-21 Thread 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

2022-01-21 Thread 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

2022-01-21 Thread 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

2022-01-20 Thread 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

2022-01-19 Thread 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

2021-12-29 Thread 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?

2021-12-27 Thread 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?

2021-12-27 Thread 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?

2021-12-26 Thread 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?

2021-12-26 Thread 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

2021-12-06 Thread 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?

2021-12-05 Thread 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?

2021-12-05 Thread 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

2021-12-05 Thread 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

2021-11-02 Thread 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

2021-11-01 Thread 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

2021-10-29 Thread 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

2021-10-29 Thread 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

2021-10-29 Thread 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

2021-10-28 Thread 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

2021-10-28 Thread 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

2021-10-20 Thread 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

2021-10-20 Thread 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

2021-08-16 Thread 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

2021-08-16 Thread 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

2021-08-16 Thread 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

2021-08-15 Thread 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

2021-08-14 Thread 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

2021-08-12 Thread 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

2021-07-21 Thread 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

2021-07-21 Thread 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.

2021-06-08 Thread 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

2021-06-04 Thread 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

2021-05-17 Thread 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

2021-05-17 Thread 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

2021-05-17 Thread 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

2021-05-16 Thread 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

2021-05-03 Thread 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

2021-04-28 Thread 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

2021-04-28 Thread 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

2021-04-04 Thread 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

2021-02-10 Thread 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

2021-01-20 Thread 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

2021-01-14 Thread 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

2021-01-07 Thread 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

2020-12-11 Thread 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

2020-11-03 Thread 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

2020-10-29 Thread 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

2020-10-29 Thread 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

2020-08-07 Thread 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

2020-07-30 Thread 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

2020-07-29 Thread 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

2020-07-15 Thread 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 

Re: [squid-dev] External ACL Feed, helper?

2020-07-07 Thread Alex Rousskov
On 7/7/20 1:00 PM, Eliezer Croitor wrote:

> let say I have a set of regex for sni which are bypassed or IP addresses
> that are allowed etc...
> Then with an automated update script that will validate that an update is
> possible and required, an update and reconfiguration will be applied.

I do not think it is a good idea to add such a script to the Squid
repository because such a script will have virtually no Squid-specific
code (and a lot of environment/business logic specifics that would be
impossible to properly support in a simple sample script).

Admins can easily script the "git pull && squid -k reconfigure" idea.
There is no point in providing that kind of a sample. I can think of
dozens of enhancements to that idea, but most of them are not about
Squid, and most of them are environment-specific, making them poor
candidate for inclusion in the official Squid repository.


Cheers,

Alex.


> -----Original Message-
> From: Alex Rousskov [mailto:rouss...@measurement-factory.com] 
> Sent: Tuesday, July 7, 2020 4:54 PM
> To: Eliezer Croitor; squid-dev@lists.squid-cache.org
> Subject: Re: [squid-dev] External ACL Feed, helper?
> 
> On 7/7/20 1:08 AM, Eliezer Croitor wrote:
> 
>> I think that many proxy admins would like to have a script that will
>> help them to update their ACLs from a feed.
>>
>> Ie they have a DB or a GIT repository that contains their ACLs data like
>> IP addresses, domain names, sni patterns etc.
> 
> * External ACL updates without Squid reconfiguration is available today.
> 
> * Built-in ACL updates via Squid reconfiguration is available today.
> 
> * Built-in ACL updates without full Squid reconfiguration is planned,
> but it is a relatively complex low-priority project with no ETA.
> Sponsors welcome.
> 
> 
>> Would it be possible to add such helper to the project sources?
> 
> If you are talking about a script that will automatically update an
> external ACL helper configuration file based on DB/git/etc. interaction,
> then I do not think it is a good idea to add such a script to the Squid
> repository because such a script will have virtually no Squid-specific
> code (and a lot of environment/business logic specifics that would be
> impossible to properly support in a simple sample script).
> 
> If you are talking about built-in ACL updates without full Squid
> reconfiguration (i.e. the last bullet above), then such a feature does
> not need an external Squid helper. It needs Squid code enhancements.
> Most likely, it will be triggered by a standard reconfiguration signal
> (but will zero-in on changed ACL parameter files by comparing file
> timestamps).
> 
> 
> Thank you,
> 
> Alex.
> 

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


Re: [squid-dev] External ACL Feed, helper?

2020-07-07 Thread Alex Rousskov
On 7/7/20 1:08 AM, Eliezer Croitor wrote:

> I think that many proxy admins would like to have a script that will
> help them to update their ACLs from a feed.
> 
> Ie they have a DB or a GIT repository that contains their ACLs data like
> IP addresses, domain names, sni patterns etc.

* External ACL updates without Squid reconfiguration is available today.

* Built-in ACL updates via Squid reconfiguration is available today.

* Built-in ACL updates without full Squid reconfiguration is planned,
but it is a relatively complex low-priority project with no ETA.
Sponsors welcome.


> Would it be possible to add such helper to the project sources?

If you are talking about a script that will automatically update an
external ACL helper configuration file based on DB/git/etc. interaction,
then I do not think it is a good idea to add such a script to the Squid
repository because such a script will have virtually no Squid-specific
code (and a lot of environment/business logic specifics that would be
impossible to properly support in a simple sample script).

If you are talking about built-in ACL updates without full Squid
reconfiguration (i.e. the last bullet above), then such a feature does
not need an external Squid helper. It needs Squid code enhancements.
Most likely, it will be triggered by a standard reconfiguration signal
(but will zero-in on changed ACL parameter files by comparing file
timestamps).


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: making TrieNode less memory-hungry

2020-07-01 Thread Alex Rousskov
On 6/19/20 5:13 PM, Francesco Chemolli wrote:

>   I'm looking at the TrieNode code, and while it's super fast, it's
> quite memory-hungry: each node uses 2kb of RAM for the children index
> and any moderately-sized Trie has plenty of nodes. On the upside, it's
> blazing fast.

In Squid, TrieNode is only used be ESI, right? IIRC, ESI code quality is
rather poor, but I do not know whether it ESI was written to optimize
performance. If it was not, then is it a good idea to tune performance
of a library used exclusively by poorly written slowish code?


> How about changing it so that each node only havs as many children as
> the [min_char, max_char] range, using a std::vector and a min_char
> offset? Lookups would still be O(length of key), insertions may require
> shifting the vector if the char being inserted is lower than the current
> min_char, but the memory savings sound promising.

Do ESI users want to trade speed for memory savings? What memory savings
and speed reduction do you anticipate for a typical use case or two?


Thank you,

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


Re: [squid-dev] Proposed focus for Squid-6

2020-07-01 Thread Alex Rousskov
On 6/30/20 6:59 PM, Amos Jeffries wrote:
> I have been asked a few weeks ago about what the "goal for Squid-6" is
> going to be.

What does it mean to claim that "Squid v6 goal is X"? Does "reaching X"
become a precondition for the v6 release? Something else? Our RoadMap
page talks of _features_ (with specific properties) driving release
process, not goals. I assume the two concepts are different, but I do
not know what the term "goal" really means here.

Does the paragraph quoted below represent a complete answer to my
question or just a partial description of what we are trying to define here?

> It just gives people some rough direction to
> consider when struggling with selecting of new work to start.

IMHO, Squid work selection should be primarily driven by
developer-specific factors that usually have little to do with
Project-declared release goals, whatever they are.

Until the meaning of "release goal" is clear to me, I can only comment
on the validity of the proposed goals from a general "What is a good
goal in a software development project?" point of view. Please do not
misinterpret my responses below as an agreement (or disagreement)
regarding adding those items as v6 release goals.

As of now, I see no reason to have release goals other than the already
established practice of tracking TODO features on the RoadMap. I am very
open to changing my position, but that change would require developing a
shared definition of the term "release goal". Hence my question above...


> The last few version we have focused on C++11 optimizations and code
> upgrades. 

I do not think this summary is meaningful or accurate, but it is a
matter of opinion/perspective.


> 0) the ongoing project to clarify OS support and testing.

I would support that project if you replace "clarify" with "define" or
something similarly meaningful/measurable.


> 1) remove features that have been deprecated since Squid-3 days.

This goal needs polishing: All such features? Some features we agree on
(e.g., the two you listed explicitly -- WAIS and ICP)? What determines
that feature F has been deprecated "since v3 days"?


> 2) proposing some next features to be removed ASAP, possibly removing
> them this release.
>  - send-announce removal
>  - SMB_LM helper removal

This needs to be rephrased as a meaningful goal. Perhaps you meant
"Removal of the following features: ..."? If yes, we need to make a
decision for each feature. It is not clear (to me) whether the two
specific examples have something in common here.


> 3) drop (all?) bitrotten code

Even with "all" in place, this is not a valid goal due to the vagueness
of the term "bitrotten code".


> 4) statistic addition to measure feature use. To improve admin ability
> to answer our "are you using this feature" requests.

I think Squid usage reporting would be a very nice feature (at virtually
any granularity) _if_ we have enough admin support to enable such
reporting by default. Perhaps we should ask on squid-users first?


HTH,

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


Re: [squid-dev] RFC: Modernizing sources using clang-tidy

2020-05-31 Thread Alex Rousskov
On 5/30/20 3:22 PM, Amos Jeffries wrote:
> On 20/04/20 2:02 pm, Alex Rousskov wrote:
>> Squid sources contain a lot of poorly written, obsolete, and
>> inconsistent code that (objectively) complicates development and
>> (unfortunately) increases tensions among developers during review.
>>
>> Some of those problems can be solved using tools that modify sources.
>> Clang-tidy is one such tool: https://clang.llvm.org/extra/clang-tidy/
>> It contains 150+ "checks" that can automatically fix some common
>> problems by studying the syntax tree produced by the clang compiler.


> In order to switch we should be looking for a tool that improves over
> the status-quo. Which is both astyle plus the custom scripts.
> 
> All of the above are high-level abstractions that the existing astyle
> tool provides by itself. So no valid reason for changing is visible yet.

Amos, you are mixing up two completely different subjects: modernizing
code using clang-tidy (this thread) and formatting sources using
clang-format (Francesco's effort). In hope to make progress, I am
ignoring most comments about astyle.


>> https://clang.llvm.org/extra/clang-tidy/checks/list.html

>> * performance-...
>>
>> Clang-tidy has a few checks focusing on performance optimizations. The
>> following commit shows a combination of the following four checks:
>> performance-trivially-destructible, performance-unnecessary-value-param,
>> performance-for-range-copy, performance-move-const-arg
>>
>> Diff: https://github.com/measurement-factory/squid/commit/1ae5d7c


> IMO this is something we should have a

Sorry, was this phrase cut prematurely or are you voicing support (in
principle) for applying (some of these) performance-related changes?


>> * modernize-use-nullptr
>>
>> Replaces NULL and 0 constants with C++11 nullptr:
>> https://clang.llvm.org/extra/clang-tidy/checks/modernize-use-nullptr.html
>>
>> While replacing most NULLs is possible with a simple script, this check
>> may be a better solution because it can safely cover more NULL uses and
>> also converts bare 0s which would be impossible using a script.
>>
>> Diff: https://github.com/measurement-factory/squid/commit/4242604

> As I mentioned earlier when kinkie tried this; I do not like this
> particular tool feature. The "upgrade" it does is far too simplistic for
> a style polishing update. If the end goal were simply to eradicate NULL
> - this would be fine. But the goal of using this tool is to ensure a
> good quality formatting style.

... except it is not the goal. modernize-use-nullptr does not change
code formatting.


> A simplistic s/NULL/nullptr/ leaves quite
> a few code lines looking just as ugly as they were with NULL.

And, as I mentioned above, modernize-use-nullptr is not simplistic --
nothing else can detect and replace 0s with nullptr.


> IMO we would be better off going the scripted way to remove the subset
> of cases we can automate and catch the rest with manual edits and in review.

I do not see a point of automating ourselves if there is an existing
automation that works much better than anything we can do ourselves.


> I like a few things the tool does. But so far it seems like something we
> want to run across the code occasionally. eg as a Jenkins test job.

Yes, or a Semaphore CI job, and/or on-demand. Please clarify regarding
performance-* checks above. If we are in agreement regarding
modernize-use-override and at least one more check, then I can start
working on a polished proposal for those checks (at least) and the
overall setup.


Cheers,

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


Re: [squid-dev] cppunit -> googletest / gmock?

2020-05-31 Thread Alex Rousskov
On 5/31/20 3:05 PM, Francesco Chemolli wrote:

> - https://stackoverflow.com/questions/7922289/googletest-vs-cppun
> it-the-facts
> - trivial but it builds up: not generally necessary to have .h and .cc
> for simple cases
> - comparison
> table: https://socialcompare.com/en/comparison/c-unit-testing-framework
> - gmock/gtest is used in broadly-used projects (e.g. chromium) - I'm not
> sure 


> So far I'm basing on documentation, found
> at 
> https://github.com/google/googletest/blob/master/googlemock/docs/cook_book.md 
> .
> I see: 
> - a more structured approach to mocking, and more infrastructure to do it
> - ON_CALL and EXPECT_CALL patterns to define actions and expectations on
> class methods (method called once / called multiple times). This can
> also allow to change the behaviour of a mocked object on different tests
> without having to remock it all
> - matchers on called methods
> - moched methods can different return values depending on arguments, and
> check for complex sequences of calls to methods (call a() with some
> arguments, then either b() or c(), if it's c() must be followe by d() 
> - for object, the concept of "uninteresting calls" that get ignored, and
> can be defaulted

Thank you for summarizing this info!

FWIW, if you and Amos decide to switch to another unit test platform, I
will not object to such a switch, provided the transition is instant. We
do not have enough resources to properly write and maintain unit tests
using one platform IMO. We certainly do not have enough resources for
two platforms.


> TBH I favour unit tests as well.

TBH, I am against test discrimination ;-).

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


Re: [squid-dev] cppunit -> googletest / gmock?

2020-05-31 Thread Alex Rousskov
On 5/30/20 2:36 PM, Amos Jeffries wrote:
> On 31/05/20 5:27 am, Francesco Chemolli wrote:
>> starting from a PR in a conversation with Alex about our current
>> approach to unit testing being painful, I've checked what alternatives
>> would we have and how practical would they be.

FWIW, I would start from the other end -- trying to understand what the
primary problems are. What unit testing problems do we want to solve?


>> - googlemock promises to be vastly superior to our current approach

> Where are you seeing evidence of this?

I too would appreciate a brief/summary answer to this question -- what
are the primary advantages? If this is already listed on the web
somewhere, a reference is enough, of course.


> The main issue we have with cppunit itself is that when a test fails it
> is not clear from the output which assertion occurred, nor why.

FWIW, the above problem is only a minor issue for me.

The main issue for me is the amount of time spent on keeping "make
check" operational compared to the actual benefits of those unit tests.
I speculate that Squid would be in an overall better place today if all
those tests were simply deleted ten years ago.


> IIRC Alex an I have different ideas about the ideal focus of testing;
>  * I prefer the micro-test approach to demonstrate a high quality proof
> of code reliability.
>  * Alex has stated a preference for high level black-box testing of
> behaviour vs design requirements.

The above summary does not accurately reflect my position. And the
juxtaposition of "reliability" and "satisfying design requirements" is
misleading at best.

Any test, including any micro- and macro-test, can be valuable or a
waste of time. The "level" of the test is not what determines its value.
The focus should be on tests that, over the long term, maximize return
on investment. In the ideal world, that approach would result in a
_combination_ of micro/unit/macro/white/black/etc. tests, each applied
to problems where its effectiveness is maximized.

In our current situation, we can discuss, among other things, whether
the continued investment in unit tests is the right thing to do, and
whether that investment (if any) should be conditional on significantly
reducing maintenance overheads. It is a difficult discussion, especially
when there is a lack of agreed upon testing principles and goals.


HTH,

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


Re: [squid-dev] squid master build with alternate openssl fails

2020-05-11 Thread Alex Rousskov
On 5/8/20 5:11 PM, Francesco Chemolli wrote:
> I rebuild our docker
> images once a month to ensure they're fresh with what's in the wild.

FWIW, I think montly update frequency is excessive when there is no
adequate update validation. I speculate that we are spending more time
on faulty build tests than on fixing true build bugs the tests expose.


> To decide how much effort to invest, how prevalent is this situation? On
> Linux I'd expect this to be pretty much a corner case by now, is it not?

Unfortunately, we do not know. Folks running SslBump on older OS
releases may want to build with newer OpenSSL releases. Is that 1% of
Squid deployments? 10%? 20% of those deployments that matter? Unknown.

If it takes more than a few hours to change an existing (or add a new)
test node that uses custom library locations for --with-foo=PATH
options, then I would not do it (for now). You have bigger fish to catch.

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


Re: [squid-dev] squid master build with alternate openssl fails

2020-05-08 Thread Alex Rousskov
On 5/8/20 10:12 AM, Christos Tsantilas wrote:

> Squid master 699ade2d fails to build with an alternate OpenSsl, when the
> "--with-openssl=/path/to/openssl" is used.

Francesco, builds with custom OpenSSL paths are not that uncommon,
especially among SslBump admins. Would you be able to test that kind of
configuration in one of the Jenkins tests? It can be even combined with
other custom-path tests. Or is this too custom/special to warrant an
automated test in your opinion?


> I think that the issue added with the commit 245314010.

I speculate that the bug is related to the disappearance of the
LIBOPENSSL_PATH assignment in that commit. We still use that variable,
but we no longer set it.


Amos, would you be able to fix this?

Thank you,

Alex.



> Example build output:
> g++ -DHAVE_CONFIG_H
> -DDEFAULT_CONFIG_FILE=\"/usr/local/squid3-cvs/etc/squid.conf\"
> -DDEFAULT_SQUID_DATA_DIR=\"/usr/local/squid3-cvs/share\"
> -DDEFAULT_SQUID_CONFIG_DIR=\"/usr/local/squid3-cvs/etc\"   -I..
> -I../include -I../lib -I../src -I../include    -I../src
> -I/usr/include/libxml2 -Wall -Wpointer-arith -Wwrite-strings -Wcomments
> -Wshadow -Woverloaded-virtual -Werror -pipe -D_REENTRANT
> -I/usr/include/libxml2 -m64    -I/usr/include/p11-kit-1  -g -O2
> -march=native -MT CacheDigest.o -MD -MP -MF $depbase.Tpo -c -o
> CacheDigest.o CacheDigest.cc &&\
> mv -f $depbase.Tpo $depbase.Po
> In file included from cache_cf.cc:3427:0:
> cf_parser.cci: In function ‘int parse_line(char*)’:
> cf_parser.cci:1466:20: error: ‘Ssl’ has not been declared
>  parse_eol(::TheConfig.ssl_crtd);
> ...
> 
> 
> My understanding is that squid enables ssl-crtd build (--with-ssl-crtd
> is given) but fails to detect OpenSsl correctly.
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev


[squid-dev] RFC: Modernizing sources using clang-tidy

2020-04-19 Thread Alex Rousskov
Hello,

Squid sources contain a lot of poorly written, obsolete, and
inconsistent code that (objectively) complicates development and
(unfortunately) increases tensions among developers during review.

Some of those problems can be solved using tools that modify sources.
Clang-tidy is one such tool: https://clang.llvm.org/extra/clang-tidy/
It contains 150+ "checks" that can automatically fix some common
problems by studying the syntax tree produced by the clang compiler.
Understanding the code at that level allows clang-tidy to attack
problems that simple scripts cannot touch.

I have not studied most of the clang-tidy checks, but did try a few
listed at the end of this email. You can see the whole list of checks at
https://clang.llvm.org/extra/clang-tidy/checks/list.html


Here are a few pros and cons of using clang-tidy compared to our own
custom scripts:

Pros:

* maintained and improved by others
* can fix problems that our scripts cannot see
* covers a few rules from C++ Core Guidelines and popular Style Guides
* arguably less likely to accidentally screw things up than our scripts

Cons:

* Requires installation of clang, clang-tidy-10, bear. It is not
difficult in a CI environment, but may be too much for occasional
contributors.

* Clang-tidy misses files that do not participate in a specific build
(e.g., misses many compat/ files that are not needed for an actual
build). Applying clang-tidy to all sources will be difficult.

* Clang-tidy misses code lines that do not participate in a specific
build (e.g., lines inside `#if HEADERS_LOG` where HEADERS_LOG was not
defined). Applying clang-tidy to all lines will be impractical.

* Clang-tidy would be difficult to customize or adjust (probably
requires building clang from scratch and writing low-level AST
manipulation code).

* Clang-tidy is relatively slow -- the whole repository scan takes
approximately 15-30 minutes per rule in my limited tests. Combining
rules speeds things up, but it may still be too slow to run during every
PR check on the current CI hardware.

* We do not have any clang-tidy experts on the development team (AFAIK).


I will itemize a few checks that I tried. The "diff" links below show
unpolished and partial changes introduced by the corresponding checks.
If we decide to use clang-tidy in principle, we will need to fine-tune
each check options (at least) to get the most out of the tool.

* modernize-use-override

Adds "override" (and removes "virtual") keywords from class declarations.

This check is very useful not just because "override" helps prevent
difficult-to-detect bugs but because it is very difficult to transition
to using "override" _gradually_ -- some compilers reject class
declarations that have a mixture of with-override and without-override
methods. Moreover, adding override keywords to old class declarations is
rather time-consuming because it is often not obvious (to a human)
whether the class introduces a new interface or overrides and old one.

Diff: https://github.com/measurement-factory/squid/commit/d00d0a8


* performance-...

Clang-tidy has a few checks focusing on performance optimizations. The
following commit shows a combination of the following four checks:
performance-trivially-destructible, performance-unnecessary-value-param,
performance-for-range-copy, performance-move-const-arg

Diff: https://github.com/measurement-factory/squid/commit/1ae5d7c


* modernize-use-nullptr

Replaces NULL and 0 constants with C++11 nullptr:
https://clang.llvm.org/extra/clang-tidy/checks/modernize-use-nullptr.html

While replacing most NULLs is possible with a simple script, this check
may be a better solution because it can safely cover more NULL uses and
also converts bare 0s which would be impossible using a script.

Diff: https://github.com/measurement-factory/squid/commit/4242604


In summary, I think investing in clang-tidy would be worth it because
the tool can address several important problems that we would otherwise
have to leave untreated. It would take some time to agree on a set of
checks and then properly configure/tune each one, but I think it is
doable. I am not sure whether these checks should be applied on each PR
check or periodically, but we can figure it out as we go.

I am not aware of any viable alternatives.

What do you think?


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: clang-format

2020-04-12 Thread Alex Rousskov
On 4/11/20 6:26 PM, Francesco Chemolli wrote:
>   I have made an attempt at running clang-format on the squid sources,
> after applying a configuration which follows as closely as possible
> the project's coding guidelines.
> 
> You can find the output of the exercise at
> https://github.com/kinkie/squid/tree/clang-format ; the configuration
> is at https://github.com/kinkie/squid/blob/clang-format/.clang-format
> .
> 
> Most of the changes are shifting comments and realigning macro blocks.
> What do you think? Is it worthwhile to make this or same variant of
> this an official guideline?


Hi Francesco,

Most likely, you have spent more time researching this issue than
anybody reading your email; your recommendations and especially
justifications behind those recommendations could be the most valuable!
Do _you_ think clang-format advantages are worth the large amount of
conflicts the switch will create across branches? If yes, what are those
advantages, and why are they worth the cost? Can clang-format do much
better than mimicking our current (very limited) Code Style? Etc. Please
try to convert this thread from an opinion poll into a true RFC that it
is supposed to be...


I only looked at a small portion of the diff, but, FWIW, I saw:

* many small improvements (e.g., [1]),
* some small regressions (e.g., [2,3]),
* more complex #include ordering rules (e.g., [4]),
* numerous bad comment movements (e.g., [5]),
* numerous pointless(?) last empty line removals (e.g., [6]).

I suspect some of the problems mentioned above can be reduced via
clang-format configuration adjustments, but I did not study this issue.


Thank you,

Alex.

[1] better tertiary operator spacing near
https://github.com/squid-cache/squid/compare/master...kinkie:clang-format#diff-94b81725fceba313922066c3ce6071a7R19

[2] broken xgetnameinfo() formatting near
https://github.com/squid-cache/squid/compare/master...kinkie:clang-format#diff-31086c4678ad7442b4024e422f6e4351R318

[3] extra ip6_parsenumeric() function name indent near
https://github.com/squid-cache/squid/compare/master...kinkie:clang-format#diff-31086c4678ad7442b4024e422f6e4351R337

[4]
https://github.com/squid-cache/squid/compare/master...kinkie:clang-format#diff-2b432472575e0b46de13556740f65a1aR10

[5] manual comment alignment for related code lines is a Bad Idea, but
even if we do automate all such alignments, Squid code is not written to
help clang identify _related_ lines correctly as illustrated by the
jobs_ member declaration changes near
https://github.com/squid-cache/squid/compare/master...kinkie:clang-format#diff-57b6a8db44fb379395f9be80a4c44c5aR82

[6] Virtually any modified source file can be used as an example, but
here is one (compat/inet_pton.h) where nothing else has changed:
https://github.com/squid-cache/squid/compare/master...kinkie:clang-format#diff-563683e32b22291b9d5622652ec544d9L33

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


Re: [squid-dev] removing cache_diff?

2020-04-07 Thread Alex Rousskov
On 4/6/20 5:00 PM, Francesco Chemolli wrote:

> has anybody used the cache_diff program in the last 10 years?

Not me. If you are going to remove it, I recommend asking on squid-users
as well and giving folks a week to respond.

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: use clang-format?

2020-04-04 Thread Alex Rousskov
On 4/4/20 4:54 AM, Francesco Chemolli wrote:

>    astyle is a bit of PITA, maybe we can replace it with clang-format?
> It seems to me it has more power and flexibility, and its config could
> be stored in the source tree itself.

I would expect clang-format to be overall better than astyle, but
somebody would need to reformat Squid using a reasonable clang
configuration, share the diff, and honestly analyze specific advantages
and disadvantages of using clang before an informed decision can be made.

BTW, astyle configuration can also be stored in the repository AFAICT:
http://astyle.sourceforge.net/astyle.html#_Option_Files

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


Re: [squid-dev] RFC: cacheMatchAcl

2020-04-04 Thread Alex Rousskov
On 4/4/20 2:49 AM, Francesco Chemolli wrote:

> I’m mostly done in a PR replacing the dlink with a std::list but without
> changing the overall design. It does kill a few tens of lines of code
> and is clearer to read tho.

Please note that std::list is usually a lot more expensive than dlink
(i.e., an invasive list) as far as performance is concerned because
std::list does one extra memory allocation for every list element. In
performance-sensitive cases, we should not replace dlink with std::list.


HTH,

Alex.


> On Sat, 4 Apr 2020 at 07:41, Amos Jeffries wrote:
> 
> On 4/04/20 3:34 am, Alex Rousskov wrote:
> > On 4/3/20 7:25 AM, Francesco Chemolli wrote:
> >
> >>   I'm looking at places where to improve things a bit, and I stumbled
> >> across cacheMatchAcl . It tries hard to be generic, but it is
> only ever
> >> used in ACLProxyAuth::matchProxyAuth . Would it make sense to
> just have
> >> a specialised cache for proxyauth?
> >
> > I wonder whether proxy_auth is special in this context:
> >
> > 1. Is proxy_auth cache radically different from other ACL caches
> such as
> > external ACL cache? Or did we just not bother unifying the code
> > supporting these two caches?
> >
> 
> Pretty much yes, we have not done the legwork. Almost every component in
> Squid which deals with externally provided state has some form of ad-hoc
> cache. If we are lucky the use a hash or dlink. One at least uses splay
> (ouch).
> 
> 
> One of my background projects in the effort to empty the PR queue this
> year is to implement a proper CLP Map - specifically for PR 30 instead
> of the LruMap disagreement blocking it. That would be a good container
> to use for all these small state data caches all over Squid - keyed
> access with a dual TTL and LFU (fading) removal mechanism.
> 
> If this ACL cache is not causing issues already we can wait until that
> gets submitted for review.
> 
> 
> > 2. Do some other ACLs cache nothing just because we did not have
> enough
> > time to add the corresponding caching support? Or do proxy_auth and
> > external ACL poses some unique properties that no other ACL
> already has
> > or likely to have in the foreseeable future?
> 
> The only thing special is that cache they use is exclusively accessed by
> them.
> 
> IDENT, ASN, DNS based ACLs also use caches. But those are a bit detached
> from the ACL code itself (eg fqdncache) since other code sometimes
> accesses the cache directly for other uses.
> 
> 
> Amos
> ___
> 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
> 
> -- 
> @mobile
> 
> ___
> 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: cacheMatchAcl

2020-04-03 Thread Alex Rousskov
On 4/3/20 7:25 AM, Francesco Chemolli wrote:

>   I'm looking at places where to improve things a bit, and I stumbled
> across cacheMatchAcl . It tries hard to be generic, but it is only ever
> used in ACLProxyAuth::matchProxyAuth . Would it make sense to just have
> a specialised cache for proxyauth?

I wonder whether proxy_auth is special in this context:

1. Is proxy_auth cache radically different from other ACL caches such as
external ACL cache? Or did we just not bother unifying the code
supporting these two caches?

2. Do some other ACLs cache nothing just because we did not have enough
time to add the corresponding caching support? Or do proxy_auth and
external ACL poses some unique properties that no other ACL already has
or likely to have in the foreseeable future?

The result of this analysis is likely to lead us towards the correct
answer to your question.


Cheers,

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


Re: [squid-dev] squid.conf future

2020-02-29 Thread Alex Rousskov
On 2/29/20 6:07 AM, Francesco Chemolli wrote:
> I like the idea of markdown, if possible.
> It is simple and gets the job done.
> Would processing it be more cumbersome than the alternatives?

If we look at the big picture rather then the next step, then the
answer, IMO, is "no". Many modern projects use markdown. I suspect there
are more modern/supported/quality tools processing markdown than
modern/supported/quality tools processing linuxdoc or similar XML-like
markup.

Moreover, _if_ we want that, we can avoid doing any mardown-to-HTML
conversion ourselves! Services like Github Pages do it for you
automatically. I am not (yet) saying that we should use such a service,
but I think we should at least _consider_ it because such services have
some serious advantages over the current web site integration approach.


Cheers,

Alex.


> On Wed, Feb 26, 2020 at 8:43 PM Alex Rousskov wrote:
> 
> On 2/25/20 1:31 AM, Amos Jeffries wrote:
> 
> > Any suggestions of formats I should look at then?
> 
> I believe cf.data.pre should use two primary formats, each optimized
> specifically for the content it is applied to. The secondary details of
> each format will evolve, but here is where I would start today:
> 
> 1. YAML-like metadata to supply formal details about each directive. We
> already use this today, and I expect no significant changes in the
> immediate future (even though many improvements are possible in this
> area!).
> 
> 2. Markdown for informal documentation inside DOC_START...DOC_END,
> DEFAULT_DOC, and similar text blocks. Preprocessing includes validation
> of internal references (for sure), generation of internal anchors
> (probably), and removal of minimal common indentation (not sure; need to
> experiment/discuss). Other preprocessing actions may be desirable as
> well, of course. An agreement on the lower-level details and a
> non-trivial conversion effort would be required. We can discuss whether
> to do it incrementally or once-and-for-all.
> 
> 
> HTH,
> 
> 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
> 
> 
> 
> -- 
>     Francesco

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


Re: [squid-dev] squid.conf future

2020-02-26 Thread Alex Rousskov
On 2/25/20 1:31 AM, Amos Jeffries wrote:

> Any suggestions of formats I should look at then?

I believe cf.data.pre should use two primary formats, each optimized
specifically for the content it is applied to. The secondary details of
each format will evolve, but here is where I would start today:

1. YAML-like metadata to supply formal details about each directive. We
already use this today, and I expect no significant changes in the
immediate future (even though many improvements are possible in this area!).

2. Markdown for informal documentation inside DOC_START...DOC_END,
DEFAULT_DOC, and similar text blocks. Preprocessing includes validation
of internal references (for sure), generation of internal anchors
(probably), and removal of minimal common indentation (not sure; need to
experiment/discuss). Other preprocessing actions may be desirable as
well, of course. An agreement on the lower-level details and a
non-trivial conversion effort would be required. We can discuss whether
to do it incrementally or once-and-for-all.


HTH,

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


Re: [squid-dev] squid.conf future

2020-02-25 Thread Alex Rousskov
On 2/25/20 1:31 AM, Amos Jeffries wrote:
> On 25/02/20 6:11 am, Alex Rousskov wrote:
>> On 2/24/20 3:11 AM, Amos Jeffries wrote:
>>> For the future I am considering a switch of cf.data.pre to a format like
>>> SGML or XML which we can better generate the website contents from.

>> If you meant something specific by "SGML", please clarify.

> We have the Linuxdoc toolchain already used for release notes
> etc. so long as we have a simple set of rules about the markup used for
> bits that cf_gen needs to pull out for code generation we can use any of
> the more powerful markup in the documentation comment parts.

With all due respect to LDP, LinuxDoc feels like is a dying project
these days -- not a lot of activity and a lot of stale sites. The
current(?) toolchain maintainer said[1] that modern (at the time)
developers prefer DocBook: "DocBook DTD [...] is now a more popular DTD
than LinuxDoc in writing technical software documentation".

That was ... 11 years ago.

[1] https://gitlab.com/agmartin/linuxdoc-tools



>> Automated rendering of squid.conf sources, including web site content
>> generation, should be straightforward with any good source format,
>> including writer-friendly formats. Thus, web site generation is not an
>> important deciding criteria here AFAICT.

> It is an existing use-case for documentation output we need to maintain.

Agreed. My point is _not_ that we do not need to support web site
generation. My point is that any decent tool, including custom scripts,
can generate web sites these days (and, in most cases, do a better job
than what we have today). Thus, we should decide based on other, more
selective factors first.


>> IMO, an ideal markup language for cf.data.pre (or its replacements)
>> would satisfy these draft high-level criteria:
>>
>> 1. Writer-friendly. Proper whitespace, indentation, and other
>> presentation features of the _rendered_ output are the responsibility of
>> renderes, not content writers. Decent _sources_ formatting should be
>> automatically handled by popular modern text editors that developers
>> already use. No torturing humans with counting tags or brackets.

> This nullifies the argument that XML is torturous. Good editing tools
> can handle XML easily.

Good editors can close the current tag for you, but closing tags and
dealing with all the other machine noise is still tedious for most
humans. XML is just not designed to be friendly to human writers (and
readers!). It is like JSON: Yes, one can edit JSON by hand, especially
with a good editor, but that does not make it human-friendly. Both
formats are meant for exchanging information between programs.


>> 2. Expressive enough to define all the squid.conf concepts that we want
>> to keep/support, so that they can be rendered beautifully without hacks.
>> For example, if we agree that those sections are a good idea, then this
>> item includes support for introduction sections that define no
>> configuration options themselves.

> What are you calling squid.conf concepts here?

Everything that may need to be referenced or rendered specially. For
example, directive names, directive parameter lists, individual
parameter documentation, parameter defaults, default parameter
documentation, configuration examples, C++ macro guards, AND prose
elements such as sections, paragraphs, lists, emphasized phrases,
verbatim text, hyperlinks, etc.



>> 3. Supports documentation duplication avoidance so that we do not have
>> to duplicate a lot of text or refer the reader to directive X for
>> details of directive Y functionality.

> The XML idea supports that. I am not sure about SGML.

With XML (and many SGML DTDs), the question is not so much whether it is
_possible_ to support Foo or Bar, but how difficult that support is
going to be (for documentation writers, readers, and tool
developers/admins). I suspect that reusing XML snippets is going to
require custom tooling unless those snippets are isolated into
entities/macros. We can live with that isolation, but a more flexible
"foo.faz documentation is the same as bar.baz documentation (after
replacing baz with faz)" may work a lot better.

N.B. If by "SGML" you mean Linuxdoc DTD, then I am not sure whether it
supports quoting. SGML itself, being a meta-language (compared to XML),
can "support" anything XML can support and a lot more.


> All the other text syntax I'm aware of do not have nice writer-friendly
> referencing. The YAML-like one we currently have is a case in point.

I am not aware of _nice_ referencing in XML either. FWIW, Markdown
referencing is OK. Certainly not nice, just OK. We have no referencing
today in cf.data.pre AFAIK.

Please note that referencing and quoting/reusing content are differen

Re: [squid-dev] squid.conf future

2020-02-24 Thread Alex Rousskov
On 2/24/20 3:11 AM, Amos Jeffries wrote:

> While doing some polish to cf_gen tool (PR #558) I am faced with some
> large code edits to get that tool any more compliant with our current
> guidelines. With that comes the question of whether that more detailed
> work is worth doing at all ...

Probably not. Even PR #558 changes might be going a bit too far (or not
far enough). Ideally, we should agree on key code cleanup principles
before doing such cleanup, to minimize tensions in every such PR.
Cleanup for the sake of cleanup should be done under a general
agreement/consent rather than ad-hoc. I am working on the corresponding
suggestions but need another week or so to post a specific proposal.


> For the future I am considering a switch of cf.data.pre to a format like
> SGML or XML which we can better generate the website contents from.

I do support fixing cf.data.pre-related issues -- they are a well-known
constant (albeit moderate) pain for developers and users alike. However,
using writer-unfriendly formats such as XML is not the best solution
IMO. SGML may be a good fit, but that concept covers such a wide variety
of languages that it is difficult to say anything specific about it in
this context (e.g., both raw XML and wiki-like markups can be valid
SGML!). If you meant something specific by "SGML", please clarify.

Automated rendering of squid.conf sources, including web site content
generation, should be straightforward with any good source format,
including writer-friendly formats. Thus, web site generation is not an
important deciding criteria here AFAICT.

IMO, an ideal markup language for cf.data.pre (or its replacements)
would satisfy these draft high-level criteria:

1. Writer-friendly. Proper whitespace, indentation, and other
presentation features of the _rendered_ output are the responsibility of
renderes, not content writers. Decent _sources_ formatting should be
automatically handled by popular modern text editors that developers
already use. No torturing humans with counting tags or brackets.

2. Expressive enough to define all the squid.conf concepts that we want
to keep/support, so that they can be rendered beautifully without hacks.
For example, if we agree that those sections are a good idea, then this
item includes support for introduction sections that define no
configuration options themselves.

3. Supports documentation duplication avoidance so that we do not have
to duplicate a lot of text or refer the reader to directive X for
details of directive Y functionality.

4. Allows for automated validation of internal cross-references (and
possibly other internal concepts that can be validated). Specification
of these cross-references is covered by item 2.

5. Allows for automated spellchecking without dangerous exceptions.

6. Git-friendly: Adding two new unrelated directives does not lead to
conflicting pull requests.

7. Either already well-known or easy to learn by example (as far as
major used concepts are concerned).

8. Can be easily parsed using programming languages that our renderers
are (going to be) written in (e.g., using existing parser libraries). We
should probably discuss whether these renderers should be (re)written in
some specific languages.

9. Translation-friendly. (I do not know what that entails, but I am sure
that others can detail this reqiurement.)

It is unlikely that we can find a language that fully satisfies all the
criteria, but I hope that we can come close. It is not a new/unusual
problem. Let's not rush into rewrites until we agree on this.


> The main point in favour of these is that we already have infrastructure
> in place for ESI and release notes. It would be less work to re-use one
> of those than integrate a new library or tooling for some other format.

Reusing existing infrastructure is a nice bonus, of course, but I think
that any major format rework should be focusing on optimizing for the
long-term. Any infrastructure changes required to render static content
on a web site seem relatively small to me. (And does not ESI support
injection of any content, not just XML-based?)


Thank you,

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


Re: [squid-dev] handling spaces in path to sources

2020-02-18 Thread Alex Rousskov
On 2/18/20 4:46 AM, Francesco Chemolli wrote:

> TL;DR: it's pointless. Spaces in paths are not safe and do not work.

A single failure on MacOS does not prove much, but, AFAICT, we should
indeed avoid spending time on supporting spaces in bootstrap.sh because
autoconf explicitly prohibits file names with spaces[1]:

```
Autoconf uses shell-script processing extensively, so the file names
that it processes should not contain characters that are special to the
shell. Special characters include space, tab, ...
```

[1]
https://www.gnu.org/savannah-checkouts/gnu/autoconf/manual/autoconf-2.69/html_node/File-System-Conventions.html#File-System-Conventions


Since regular users do not bootstrap, we do not need to make
bootstrap.sh space-safe at least until autoconf/configure becomes
space-safe (or we stop using autoconf and similar space-unsafe tools).


Thank you,

Alex.

> On Tue, Feb 18, 2020 at 8:40 AM Francesco Chemolli wrote:
> 
> Hi all,
>   as a followup to discussion on PR 555, I was looking into safely
> handling source code paths with spaces.
> 
> Turns out worrying is pointless: after doing the needed changes,
> doing a test build with
> builddir = "/Users/kinkie/src/test build with space"
> srcdir = "/Users/kinkie/src/squid dir with space"
> on MacOS catalina with homebrew
> 
> resulted in configure failing with error:
> checking whether build environment is sane... configure: error:
> unsafe srcdir value: '/Users/kinkie/src/squid dir with
> space/test-suite/..'
> 
> If there is no regression with my changes, I'll submit the PR
> anyway, but its benefits will be limited.
> 
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev


Re: [squid-dev] Interested in helping with Squid development

2020-01-28 Thread Alex Rousskov
On 1/27/20 2:11 PM, Mike Rumph wrote:

> I am interested in helping with Squid development.
> I've participated as committer on the Apache HTTP Server project since 2012.
> And I have over 20 years experience with C/C++.


Hello Mike,

Welcome to the Squid Project! Squid does need good C++ developers.
We have a lot more work than the current handful of regulars can carry.
The Apache committer status should qualify you for challenging
development projects, but please note that many areas of Squid code are
a lot less polished than httpd.


> First of all as I get to know the project, I would help with the
> documentation.
> For example, I have already found several grammar errors in the
> following link that I would like to contribute fixes for:
> - https://wiki.squid-cache.org/SquidFaq/ConfiguringSquid 
> 
> Later, I will be interested in contributing source fixes as well.

For wiki fixes, please see the following first step instructions if you
have not already: https://wiki.squid-cache.org/WikiAccountCreation

Another, somewhat similar project would be addressing bug 5021:
http://bugs.squid-cache.org/show_bug.cgi?id=5021


Cheers,

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


Re: [squid-dev] Want to integrate squid github to Jenkins CI

2020-01-22 Thread Alex Rousskov
On 1/21/20 11:30 PM, Justin Michael Schwartzbeck wrote:
> 
> So I guess maybe I need to narrow this down a little bit more. Is there
> some programmatic way that I can get the *latest stable release*
> *version* and *source download link*?

If you want GitHub integration, then you should get all git tags from
the official Squid repository, filter/sort SQUID_X_Y release tags, find
the last tag, and checkout git sources using that tag.

Until the Squid Project starts using GitHub releases, your trigger will
probably have to be any repository modification. Such modifications are
infrequent so the amount of extra tag manipulation work (when the tag of
interest has not changes) will be small.


HTH,

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


Re: [squid-dev] Efficient FD annotations

2020-01-13 Thread Alex Rousskov
On 1/10/20 2:37 AM, Amos Jeffries wrote:
> On 8/01/20 3:39 am, Alex Rousskov wrote:
>> On 1/7/20 1:39 AM, Amos Jeffries wrote:
>>> On 7/01/20 4:28 am, Alex Rousskov wrote:
>>>> For the record: The ideas below are superseded by the concept of the
>>>> code context introduced in commit ccfbe8f, including the
>>>> fde::codeContext field. --Alex
>>
>>> If you want to go that way (replace fde:note with fde:codeContext)
>>
>> I would not replace fde::note with fde::codeContext. I would keep
>> fde::note as a basic indication of the current FD purpose/scope. This
>> can be done cheaply using string literals or constant SBufs.

> In that case, why is this thread being revived?

FWIW, I was not trying to revive this thread. I simply added a reference
to the (official) next generation of the ideas expressed on that old
thread. IIRC, I posted this "closure" in part because PR 270 is open,
and one of my old comments there refers to this thread.


> AIUI your proposals were alternatives to PR 270 - ways to replace the
> fde::note field instead of just updating it to SBuf.

This thread kept the fde::note field and its SBuf conversion introduced
in PR 270. The thread focused on adding transaction information without
the performance sacrifices. Please see the original message for details:
http://lists.squid-cache.org/pipermail/squid-dev/2019-February/009503.html


>>> we are going to have to do a security audit on the values displayed
>>> by the CodeContext objects. That is due to how the fde::note are sent
>>> over the public network in clear-text transactions for
>>> mgr:filedescriptors report.
>>
>> Overall, I doubt such an audit is a good idea -- only the Squid admin
>> can correctly decide whether it is OK to expose transaction information
>> in cache manager responses[1,2].
> 
> see responses below to [1] and [2]. Since you are apparently not seeking
> to replace the mgr:filedescriptors report fde::note field with
> fde::codeContext display - then please take it as a general security
> approach policy for handling that type of change.

Sorry, I do not understand what "it" and "that type of change" refers to
in your last sentence, especially since those references follow a text
that describes what is _not_ going to happen (i.e. "you are apparently
not ..."). My argument was (and is) that an audit without a
configuration option (or an equivalent shared definition of "sensitive"
and "exposure") would be largely impractical or meaningless because the
auditor would not be able to properly identify problems with exposure of
sensitive info.


>> If there is demand for limiting that
>> exposure, I would rather add a configuration directive that would allow
>> the admin to control whether Squid is allowed to report context in cache
>> manager responses, error pages, etc.

> We do not have to default such an option to publish everything on the
> off chance it is safe. Our objective here is to prevent CVEs occuring.
> To achieve that we should be defaulting to elide sensitive details from
> public view unless configured to show them.

I have no problems with the "do not publish" default for areas where the
information may go to a 3rd party unexpectedly (for a reasonable admin).
You started your paragraph with "we do not have to X". I hope I did not
imply anywhere that we have to X.


> The details I am thinking of most prominently here are things like
> credentials and tokens in the auth processing contexts. URL history and
> keys in the SSL-Bump context. Access to Squid internal memory spaces on
> the server. There are probably others we will uncover later. Anything
> that could be reported as a sensitive information leak and assigned a CVE.

I think we are on the same page here although, IIRC, we already report
URLs in fde::notes.


>> [1] Some of the transaction context is already exposed in the current
>> cache manager responses. We may want to add more details or report fewer
>> details, but there is no paradigm shift here.

> "some" renders this argument irrelevant. The leak issues will be
> cropping up about the *new* data if we let that contain anything sensitive.

I do not follow. If we already expose sensitive data in interface X,
then a reasonable admin would be expected to protect that interface and,
hence, we can add similarly sensitive details there with little risk.
The only other reasonable action that I can see is to file a CVE for
what we are already doing, admitting that we have exposed too much (even
though nobody complained [loudly enough] about that old exposure).


>> [2] In some deployment environments, cache manager responses are
>> delivered over 

Re: [squid-dev] Timeouts for abandoned negative reviews

2020-01-10 Thread Alex Rousskov
On 1/10/20 1:59 AM, Amos Jeffries wrote:
> On 9/01/20 11:20 am, Alex Rousskov wrote:
>> Squid GitHub pull requests have the following problem: A core
>> developer can stall PR progress by submitting a negative review and then
>> ignoring the PR (despite others reminding them that the reviewer action
>> is required). Such stalled PRs cannot be merged because our policies
>> strictly prohibit merging of vetoed PRs.

> The "problem" you are describing is what I see as a core principle of
> the review process.

You may have missed the critical "and then..." part of my description. I
did not describe the ability to block a PR. I described the ability to
block and then ignore the PR. I called that "stalling". The ability to
block a PR (a fundamental feature) is very different from stalling (a
dereliction of duty).

[I am omitting the part of your response that discusses basic PR
blocking because this is not what I am talking about.]


>> Collection of meaningful stats is
>> prohibitively expensive, but there are ~50 PRs that were open for 100+
>> days and many of them are not abandoned/irrelevant. Here are some of the
>> worst examples (the stalled day counts below are approximate):
>>
>> * PR 369: stalled for 310 days (and counting)
>>   https://github.com/squid-cache/squid/pull/369
>>
>> * PR 59: 120-480 days, depending on when you think the PR was stalled
>>   https://github.com/squid-cache/squid/pull/59
>>
>> * PR 443: stalled for 100+ days (and counting)
>>   https://github.com/squid-cache/squid/pull/443


> Also, at a fundamental level I object to the categorization of any open
> PRs as "abandoned".

This term does not change the proposal AFAICT, but what term would you
prefer?


> Particular to this proposal I regularly review *all* PRs in a quick scan
> to see where progress can happen

It is good that you know when a PR needs your attention, but please note
that your scans have not solved the problem I am writing about. We need
to solve that problem.

The timeout might solve (or reduce) the problem not because of new
notifications, but because it sheds reviewer load. A reviewer who for
months sees authors pleading to explain what needs to be done but does
not respond is probably overloaded.


>> Stalled PRs may significantly increase development overheads and badly
>> reflect on the Squid Project as a whole. I have heard many complains
>> from contributors frustrated with the lack of Project responsiveness and
>> accountability. Stalled PRs have also been used in attempts to block
>> features from being added to the next numbered release.


> Interesting statement there. Particularly since you and I are
> essentially the only reviewers.
> 
> Are you admitting reason for PR 358 not being approved yet?

You seem to imply something nefarious on my part. Sorry, I do not know
what you are implying exactly, so I will just stick to the facts:

* PR 358 was approved a year ago and was open for about a day. I do not
think it is relevant here.

* If you meant PR 538, then I reviewed it within ~6 days of opening
(before seeing your squid-dev message), and Eduard helped you make
progress with this PR a few days earlier. And the PR is not even
blocked! Clearly, this is not a stalled PR by any stretch of the
imagination.


> I certainly have not done such underhanded politics. When I want to
> block a feature for next release I state so clearly in the PR. Though
> there is scant reason to block anything from merging to master when the
> code is good - no numbered releases come from there.

I am not sure why you are saying the above. You seem to be trying to
prove that you have not done something bad that I have done, but I do
not know what that something is, so I cannot respond in a meaningful way.


>> While 100-day stalling is unheard of in other open source projects I
>> know about, the problem with unresponsive reviewers is not unique to
>> Squid. The best (applicable to the Squid Project) solution that I know
>> of is a timeout:
>>
>> If the reviewer remains unresponsive for N days, their negative review
>> can be dismissed. The counting starts from a well-defined event S, and
>> there are at least two reminder comments addressed at the reviewer (R1
>> days after S and R2 days before the dismissal).
>>
>> Do you think timeouts can solve the problem with stalled PRs if Project
>> contributors do not attempt to game the system? Can you think of a
>> better solution?


> I do not think this will solve stalled PRs.

Why do you think timeouts will not solve stalled PRs? What do you think
will happen if we agree to enable timeouts (other than better
communication you have mentioned)?

Can you think of a solution th

[squid-dev] Timeouts for abandoned negative reviews

2020-01-08 Thread Alex Rousskov
Hello,

Squid GitHub pull requests have the following problem: A core
developer can stall PR progress by submitting a negative review and then
ignoring the PR (despite others reminding them that the reviewer action
is required). Such stalled PRs cannot be merged because our policies
strictly prohibit merging of vetoed PRs.

This problem has affected many PRs. Collection of meaningful stats is
prohibitively expensive, but there are ~50 PRs that were open for 100+
days and many of them are not abandoned/irrelevant. Here are some of the
worst examples (the stalled day counts below are approximate):

* PR 369: stalled for 310 days (and counting)
  https://github.com/squid-cache/squid/pull/369

* PR 59: 120-480 days, depending on when you think the PR was stalled
  https://github.com/squid-cache/squid/pull/59

* PR 443: stalled for 100+ days (and counting)
  https://github.com/squid-cache/squid/pull/443

Stalled PRs may significantly increase development overheads and badly
reflect on the Squid Project as a whole. I have heard many complains
from contributors frustrated with the lack of Project responsiveness and
accountability. Stalled PRs have also been used in attempts to block
features from being added to the next numbered release.

While 100-day stalling is unheard of in other open source projects I
know about, the problem with unresponsive reviewers is not unique to
Squid. The best (applicable to the Squid Project) solution that I know
of is a timeout:

If the reviewer remains unresponsive for N days, their negative review
can be dismissed. The counting starts from a well-defined event S, and
there are at least two reminder comments addressed at the reviewer (R1
days after S and R2 days before the dismissal).

Do you think timeouts can solve the problem with stalled PRs if Project
contributors do not attempt to game the system? Can you think of a
better solution?


We can discuss specific parameter values and minor technical details
separately, but I would post the following rough suggestions for
illustrative purposes:

* N (stalling days after which the veto is dismissed): 30. N should be
small enough to prevent long stalls (keeping in mind that an
irresponsible reviewer can stall the same PR many times) but large
enough to accommodate vacations, sickness, overload, and similar
temporary conditions.

* R1 (days between S and the first "you are stalling!" reminder): 20. R1
should be small enough to avoid rushing re-review but large enough to
indicate a likely problem and avoid noise.

* R2 (days between the second reminder and vote dismissal): 5. R2 should
be small enough to reduce "Oh, I will do it later!" failures to
re-review and to clearly indicate a stalling problem (the PR was stalled
for N-R2 days already) but large enough to be sufficient for a thorough
re-review.

* S (the event that resets the timeout counter): The last time the PR is
assigned to the reviewer. GitHub already logs assignments and generates
a notification email. I do not like to abuse the "PR Assignment" feature
to pass the "who is responsible for the next step" baton, but I cannot
think of a _simpler_ way to implement this. Assignment also makes it
easy to search for PRs that await your action. The reviewer would be
responsible for assigning the PR back to the author (if there are
unaddressed change requests) or to Anubis (if a commit is required).


Thank you,

Alex.
P.S. A similar timeout approach can be applied to PRs stalled by
authors, but those PRs represent a smaller problem and their incorrect
handling results in little harm. We can consider that problem and argue
about appropriate parameters for it later.
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev


Re: [squid-dev] Efficient FD annotations

2020-01-07 Thread Alex Rousskov
On 1/7/20 1:39 AM, Amos Jeffries wrote:
> On 7/01/20 4:28 am, Alex Rousskov wrote:
>> For the record: The ideas below are superseded by the concept of the
>> code context introduced in commit ccfbe8f, including the
>> fde::codeContext field. --Alex

> If you want to go that way (replace fde:note with fde:codeContext)

I would not replace fde::note with fde::codeContext. I would keep
fde::note as a basic indication of the current FD purpose/scope. This
can be done cheaply using string literals or constant SBufs.


> we are going to have to do a security audit on the values displayed
> by the CodeContext objects. That is due to how the fde::note are sent
> over the public network in clear-text transactions for
> mgr:filedescriptors report.

Overall, I doubt such an audit is a good idea -- only the Squid admin
can correctly decide whether it is OK to expose transaction information
in cache manager responses[1,2]. If there is demand for limiting that
exposure, I would rather add a configuration directive that would allow
the admin to control whether Squid is allowed to report context in cache
manager responses, error pages, etc.

Alex.

[1] Some of the transaction context is already exposed in the current
cache manager responses. We may want to add more details or report fewer
details, but there is no paradigm shift here.

[2] In some deployment environments, cache manager responses are
delivered over secure channels.
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev


Re: [squid-dev] Efficient FD annotations

2020-01-06 Thread Alex Rousskov
For the record: The ideas below are superseded by the concept of the
code context introduced in commit ccfbe8f, including the
fde::codeContext field. --Alex


On 2/22/19 12:08 PM, Alex Rousskov wrote:
> In https://github.com/squid-cache/squid/pull/270#discussion_r259316609
> 
> 
>> In src/comm.cc:
>>
>>> @@ -424,7 +424,7 @@ comm_init_opened(const Comm::ConnectionPointer ,
>>  debugs(5, 5, HERE << conn << " is a new socket");
>>  
>>  assert(!isOpen(conn->fd));
>> -fd_open(conn->fd, FD_SOCKET, note);
>> +fd_open(conn->fd, FD_SOCKET, SBuf(note));
> 
> 
>> Alex: I do not think we should introduce this performance regression
>> on a common code path. Better debugging/reporting is just not worth
>> it. I have ideas on how this regression can be avoided (and debugging
>> improved), but I do not want to waste time detailing and discussing
>> them if you do not want to spend a lot more time on this PR.
> 
> 
>> Amos: Since this PR is about fixing the compile error I don't want to
>> complicate it any further. But do want to hear these ideas. Can you
>> post them to squid-dev so we can go over it separately please.
> 
> 
> IMO, the best way to annotate file descriptors (and other objects) for
> debugging/reporting purposes is to store pointers to their current
> owners and/or objects currently responsible for FD processing. Very
> roughly speaking:
> 
> class fde
> {
>   ...
> 
>   /* current FD handling context */
> 
>   /// Comm::Connection this FD belongs to
>   WeakConnectionPointer connection;
> 
>   /// Client that created this FD
>   WeakClientPointer creator;
> 
>   /// Server that accepted this FD
>   WeakServerPointer acceptor;
> 
>   /// generic FD owner/handler
>   /// (to cover exceptional contexts missed above?)
>   WeakFdOwnerPointer owner;
> 
>   /// poor man's generic FD owner/handler/operation description
>   /// (to cover performance-ignorant contexts missed above)
>   SBuf note;
> };
> 
> 
> Copying a pointer is a cheap operation; its performance expense is worth
> providing that extra information to developers and sysadmins. The heavy
> price of interpreting/reporting owner details is only paid when that
> extra information is actually requested/used, allowing us to provide a
> lot more useful details (than a short summary which we can compute and
> stuff into an SBuf every time we manipulate an FD).
> 
> Designing the right set of context pointers requires making difficult
> decisions such as whether the fde class should point to acceptor/creator
> (as shown in the above oversimplified sketch) or the Comm::Connection
> class should do that instead. FWIW, the latter makes more sense to me
> now, but I have not given it enough thought.
> 
> Implementing weak pointers correctly (including efficiently) OR
> convincing ourselves that certain strong pointers are acceptable in this
> context is also a serious challenge.
> 
> 
> IMO, we should be moving towards this design while continuing to provide
> sketchy/stale/truncated/limited FD info without performance regressions.
> 
> Fortunately, the two approaches can co-exist nicely: Want more/better
> details? Invest in the right approach for the context you are interested
> in. Just want to continue to provide the bare minimum? Continue to use
> expensive fixed annotation buffers (without slowing things down further)
> and/or cheap constant SBuf annotations.
> 
> 
> 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 status update and RFI

2019-12-30 Thread Alex Rousskov
On 12/30/19 11:22 AM, Amos Jeffries wrote:
> On 31/12/19 3:01 am, Alex Rousskov wrote:
>> On 12/30/19 4:46 AM, Amos Jeffries wrote:
>>>
>>> The v5 branch will be bumped to master HEAD
>>> commit in a few hours then the documentation update PRs for stage 2 will
>>> proceed.
>>
>> I would wait for all pending v5 changes to be committed to master before
>> pointing v5 to master's HEAD. There is no pressure to commit to master
>> anything that should not be in v5 right now.


> Problem with that plan is that most of your requested "should be in v5"
> list are new features. 

The new features are not the problem.


> We already have enough features to make v5 a release.

... but we should not release v5 without PRs that should be in v5. Doing
so would effectively grant you unchecked powers to block any change from
being released. Unchecked powers are inherently dangerous, and you are
not impartial, rational, and careful enough to allow this unnecessary risk.


> "Just one more feature" is a very slippery slope that we have been
> sliding down for most of this past year already.

A stream of new features is _not_ the reason why v5 branching time has
been sliding. Improper treatment of the already submitted changes is one
of the reasons behind that slide.


> Frequent release -> fewer feature change -> fewer new bugs -> happier
> community. The sequence is simple and well-known.

With even fewer new features, Squid would have been dead or forked long
time ago...

However, we should not even be discussing new feature frequency in this
thread context because feature frequency does not determine release
frequency. If you want to make more frequent releases, please post a
proposal. FWIW, I would be OK with much more frequent releases, even
monthly ones, if they are done right and somebody has a genuine need
that justifies such an overhead.


> Likewise our versioning policy (published since 2008):
> 
>  *10* features for a new major version.
>  Bi-Monthly stable point release.
>  Monthly beta of next version.
>  Daily alpha of development work.

I do not understand why you are citing a policy that nobody follows. Are
you implying that you will start following this policy soon? FWIW, I am
still OK with the above (bad) policy as long as it is consistently
applied and there is protection from you forever blocking the features
you do not like.

I think that branching is an important enough event to deserve
consistent rules and, if those rules are vague or effectively
non-existent, consensus.


> PS. I see you do have a sense of humour. "There is no pressure to commit
> to master". Thanks for the laugh :-). Though its NYE not April 1st.

I actually said "There is no pressure to commit to master anything that
should not be in v5 right now", and that fact was not a joke.

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


  1   2   3   4   5   6   7   8   9   10   >