ESI feature has a number of bugs and security vulnerabilities.
It is also rarely used and a survey of active community members
has not revealed a need to keep maintaining this code.
Alex Rousskov [Fri, 20 Sep 2024 09:35:04 +0000 (09:35 +0000)]
Use ERR_ACCESS_DENIED for HTTP 403 (Forbidden) errors (#1899)
... when request authentication fails. Do not use
ERR_CACHE_ACCESS_DENIED for those "permanent" errors.
Default ERR_CACHE_ACCESS_DENIED is meant for cases where the user is
likely to eventually gain access (e.g., by supplying credentials). Its
default text says "not currently allowed... until you have authenticated
yourself". When the error page was added in 1998 commit cb69b4c7 it was
only used for HTTP 407 errors. The same logic was preserved when that
code was refactored in 1999 commit 1cfdbcf0, but exceptions started to
creep in, perhaps accidentally, since 2011 when HTTP 403 case was added
in commit 2f1431ea that introduced USE_AUTH macro. 2011 commit 21512911
added a similar "not possible to authenticate" SslBump case.
Other HTTP 403 (Forbidden) cases already use ERR_ACCESS_DENIED or a
similar "permanent" error (e.g., ERR_FORWARDING_DENIED or ERR_TOO_BIG).
It is still possible to customize the returned error page via deny_info.
The replaced assertion was wrong because a stale entry may be aborted
while we are revalidating it. The exact real-world conditions that
triggered this assertion are unknown, but many conditions lead to
aborted entries. The assertion can be triggered in lab tests using a hit
transaction that collapses on a miss transaction that runs into storage
problems (e.g., a memory cache that ran out of usable space).
Also adjusted cacheHit() to avoid similar problems. We have not
reproduced them, but no code maintains the asserted invariant on the
cacheHit() path either. Moreover, async-called cacheHit() initiates
processExpired() that leads to problematic sendClientOldEntry() call, so
seeing an aborted StoreEntry at cacheHit() time is probably a matter of
sufficient concurrency levels and asynchronous callback delays.
When establishing a TLS connection to an origin server _through_ a
cache_peer, Security::CreateClientSession() used cache_peer's
Security::PeerOptions instead of global ProxyOutgoingConfig (i.e.
tls_outgoing_options). Used cache_peer's PeerOptions are unrelated to
the (tunneled) TLS connection in question (and are currently empty
because Squid does not support TLS inside TLS -- the cache_peer accepts
plain HTTP connections).
This TLS context:options mismatch exists in both OpenSSL and GnuTLS
builds, but it currently does not affect OpenSSL builds where
CreateSession() gets TLS options from its Security::Context parameter
rather than its (unused) Security::PeerOptions parameter.
The mismatch exists on both PeekingPeerConnector/SslBump and
BlindPeerConnector code paths.
This minimal change pairs TLS context with its TLS options. Long-term,
the added FuturePeerContext shim (that stores references to independent
context and options objects) should be replaced with a PeerContext class
that encapsulates those two objects. We may also be able to avoid
re-computing GnuTLS context from options and to simplify code by
preventing PeerConnector construction in Squid builds that do not
support TLS. That refactoring should be done separately because it
triggers many changes unrelated to Bug 5293.
Also removed updateSessionOptions() from
PeekingPeerConnector::initialize() because Security::CreateSession(),
called by our parent initialize() method, already sets session options.
It is easier to remove that call/code than keep it up to date.
Security::BlindPeerConnector does not updateSessionOptions().
After a ReadNow() call, the buffer length must not exceed accumulation
limits (e.g., client_request_buffer_max_size). SBuf::reserve() alone
cannot reliably enforce those limits because it does not decrease SBuf
space; various SBuf manipulations may lead to excessive SBuf space. When
filled by ReadNow(), that space exceeds the limit.
This change uses documented CommIoCbParams::size trick to limit how much
Comm::ReadNow() may read, obeying SQUID_TCP_SO_RCVBUF (server-to-Squid)
and client_request_buffer_max_size (client-to-Squid) accumulation limit.
Alex Rousskov [Thu, 5 Sep 2024 17:46:20 +0000 (17:46 +0000)]
Bug 5417: An empty annotation value does not match (#1896)
Helpers may return annotations with empty values:
OK team_=""
A note ACL may be configured to match an annotation with an empty value:
configuration_includes_quoted_values on
acl emptyTeam note team_ ""
However, that emptyTeam ACL did not match the above helper annotation:
* AppendTokens() split an empty annotation value into an empty vector
instead of a vector with a single empty entry. That "never match an
empty value received from the helper" bug was probably introduced in
2017 commit 75d47340 when it replaced an "always try to match an empty
value, even when it was not received from the helper" bug in
ACLNoteStrategy::matchNotes().
* ACLStringData::match(SBuf v) never matched an empty value v. That bug
was probably introduced in 2015 commit 76ee67ac that mistook a nil
c-string pointer for an empty c-string.
Alex Rousskov [Sun, 1 Sep 2024 00:39:34 +0000 (00:39 +0000)]
Bug 5405: Large uploads fill request buffer and die (#1887)
maybeMakeSpaceAvailable: request buffer full
ReadNow: ... size 0, retval 0, errno 0
terminateAll: 1/1 after ERR_CLIENT_GONE/WITH_CLIENT
%Ss=TCP_MISS_ABORTED
This bug is triggered by a combination of the following two conditions:
* HTTP client upload fills Squid request buffer faster than it is
drained by an origin server, cache_peer, or REQMOD service. The buffer
accumulates 576 KB (default 512 KB client_request_buffer_max_size + 64
KB internal "pipe" buffer).
* The affected server or service consumes a few bytes after the critical
accumulation is reached. In other words, the bug cannot be triggered
if nothing is consumed after the first condition above is met.
Comm::ReadNow() must not be called with a full buffer: Related
FD_READ_METHOD() code cannot distinguish "received EOF" from "had no
buffer space" outcomes. Server::readSomeData() tried to prevent such
calls, but the corresponding check had two problems:
* The check had an unsigned integer underflow bug[^1] that made it
ineffective when inBuf length exceeded Config.maxRequestBufferSize.
That length could exceed the limit due to reconfiguration and when
inBuf space size first grew outside of maybeMakeSpaceAvailable()
protections (e.g., during an inBuf.c_str() call) and then got filled
with newly read data. That growth started happening after 2020 commit 1dfbca06 optimized SBuf::cow() to merge leading and trailing space.
Prior to that commit, Bug 5405 could probably only affect Squid
reconfigurations that lower client_request_buffer_max_size.
* The check was separated from the ReadNow() call it was meant to
protect. While ConnStateData was waiting for the socket to become
ready for reading, various asynchronous events could alter inBuf or
Config.maxRequestBufferSize.
This change fixes both problems.
This change also fixes Squid Bug 5214.
[^1]: That underflow bug was probably introduced in 2015 commit 4d1376d7
while trying to emulate the original "do not read less than two bytes"
ConnStateData::In::maybeMakeSpaceAvailable() condition. That condition
itself looks like a leftover from manual zero-terminated input buffer
days that ended with 2014 commit e7287625. It is now removed.
Alex Rousskov [Wed, 28 Aug 2024 21:01:15 +0000 (21:01 +0000)]
Reject config with unknown directives before committing to it (#1897)
Ideally, we want to reject configurations with unknown directives before
applying any configuration changes that correspond to known directives,
but current apply-as-you-parse architecture makes that impractical.
Pending smooth reconfiguration refactoring will make that possible, but
we can make a step towards that ideal future now.
Rejecting bad configurations before calling configDoConfigure() reduces
the set of configuration errors that Squid can detect in one execution
(because configDoConfigure() error-checking code is not reached), but
that small reduction is a lesser evil compared to running
configDoConfigure() with a clearly broken config, especially when we are
going to kill Squid anyway. While many legacy parse_foo() functions do
apply significant changes before configDoConfigure(), we cannot easily
prevent that (for now). We can easily prevent configDoConfigure().
Use git to extract default build-info (when enabled) (#1892)
Have configure option --enable-build-info[=yes]
refer to git instead of bazaar to extract
information about what is being built to include
in the output of `squid -v`
Alex Rousskov [Wed, 21 Aug 2024 18:51:29 +0000 (18:51 +0000)]
Fix Comm::TcpAcceptor::status() reporting of listening address (#1891)
Buggy "static" caching optimization led to TcpAcceptor X reporting
listening address of TcpAcceptor Y or an incorrect 0.0.0.0 address.
TcpAcceptor status is visible in mgr:jobs and level-5+ debugs() output.
Side effect: For a listening port configured without an explicit IP
address, the listening address part of status() is now reported using
"[::]" rather than "0.0.0.0" string.
Alex Rousskov [Wed, 21 Aug 2024 07:09:15 +0000 (07:09 +0000)]
Optimization: Do not zero-terminate Server::inBuf (#1889)
... when storing unencoded request body bytes in ConnStateData::bodyPipe
Calling SBuf::c_str() is unnecessary in this context because
BodyPipe::append() does not need/expect a zero-terminated buffer. The
call is relatively expensive when the previous Comm::ReadNow() was able
to fill the entire inBuf trailing space, which is a common occurrence
because Server::maybeMakeSpaceAvailable() uses CLIENT_REQ_BUF_SZ as
idealSpace size, and CLIENT_REQ_BUF_SZ is hard-coded to just 4096 bytes.
The call was added in 2014 commit e7287625 that converted inBuf to SBuf.
Alex Rousskov [Mon, 19 Aug 2024 17:20:06 +0000 (17:20 +0000)]
Fixed and redefined total peering time (%<tt) (#1828)
This change adjusts `%[http::]<tt` definition to include peer selection
stage (with any relevant DNS lookups). That stage runs in parallel with
connection establishment attempts, so excluding portions of that stage
made interpreting logged delays more difficult (and code more complex).
Also documented how cache refresh (TCP_REFRESH_MODIFIED) affects `%<tt`.
Fetching a new version of the stale object creates a second FwdState
object, effectively repeating peer selection activities and request
forwarding attempts. `%<tt` includes the time spent by those activities.
The following three bugs were fixed (at least):
* Squid logged dash `%<tt` values for pinned connections (e.g.,
connections bumped at SslBump step 2 or 3) because Squid did not call
HierarchyLogEntry::startPeerClock() for pinned connections.
* Logged `%<tt` values for connections spliced at SslBump step 2 or 3
included TCP connection establishment time and TLS Server Hello
fetching time (if any) but were missing the rest of Squid-peer
communication (i.e. when Squid was tunneling bytes to and from the
peer) because Squid stopped counting after calling switchToTunnel().
* Squid logged dash `%<tt` values for external_acl_type and most other
directives that support logformat codes because ALE::hier data member
is unset until access logging, when prepareLogWithRequestDetails()
copies HttpRequest::hier to ALE. This is a short-term fix. A proper
one deserves a dedicated change (that should also address similar
concerns for other HierarchyLogEntry-derived %codes).
Amos Jeffries [Mon, 12 Aug 2024 14:26:55 +0000 (14:26 +0000)]
Fix performance regressions with fastCheck() result copying (#1883)
Detected by Coverity. CID 1616162: Performance inefficiencies
(AUTO_CAUSES_COPY). Using the 'auto' keyword without an '&' causes the
copy of an object of type Acl::Answer.
This change fixes a few manually picked cases in sources that are not
normally compiled on our CI nodes. There are probably more missing
declarations, but we do not know how to find them automatically.
Also removed a few seemingly unused functions and declarations.
Protect ACLFilledChecklist heap allocations from leaking (#1870)
Non-blocking ACL checks follow this code pattern:
const auto ch = new ACLFilledChecklist(...);
fillWithInformation(ch); // may throw
ch->nonBlockingCheck(&throwingCallback); // may delete ch or throw!
// ch may be a dangling raw pointer here
The checklist object is leaked if an exception is thrown by
fillWithInformation() (and the code it calls) or by nonBlockingCheck()
(and the code it calls, including, in some cases, throwingCallback()).
Use std::unique_ptr to avoid such leaks.
Remove peerDigestFetchStop() and peerDigestFetchAbort() wrappers (#1869)
These wrappers were left in recent commit f036532 to reduce noise. Now
we always call finishAndDeleteFetch() directly (instead of sometimes
going through those wrappers). The new function name helps reduce the
number of use-after-free bugs in this code.
This flag became effectively unused in 2010 commit cfd66529 when
copyFDFlags() -- the only function checking the flag -- became unused.
That unused function was removed a bit later in commit 5ae21d99.
Supporting logging to a file complicates upgrading helper code to use
debugs() because DebugFile code calls commSetCloseOnExec(), and our
comm/libminimal does not currently provide a functioning implementation
for that API: The existing implementation is an unconditional assert.
To save development time while upgrading helpers, we are dropping this
feature. It can probably be emulated using shell redirection tricks.
Alex Rousskov [Mon, 5 Aug 2024 19:30:08 +0000 (19:30 +0000)]
Extend in-use ACLs lifetime across reconfiguration (#1826)
When Squid is reconfigured, all previously created Acl::Node objects
become unusable[^1], resulting in ACLChecklist::resumeNonBlockingCheck()
ACCESS_DUNNO outcomes. ACCESS_DUNNO leads to Squid applying the affected
directive "default" action rather than the configured one (e.g., an
http_access denies a transaction that should be allowed or miss_access
allows a transaction that should be denied). These unwanted and
surprising effects make hot reconfiguration unreliable. They are also
difficult to discover and triage.
An Acl::Node object should last as long as the object/scope that saved
that ACL for reuse. This change applies this (re)configuration principle
to ACLChecklist::accessList by converting that raw Acl::Tree pointer to
use reference counting. That change requires reference counting of
individual Acl::Node objects and related types. Several TODOs and XXXs
asked for reference counted ACLs, but more than 50 explicit raw ACL
pointers in Squid code make that transition difficult:
* Config.accessList.foo and similar items used raw Acl::Tree pointers to
objects destroyed during reconfiguration by aclDestroyAcls(). They now
point to refcounted Acl::Trees that ACLChecklist and other
configuration-using code can safely store across reconfigurations. The
outer Config.accessList.foo pointer is still raw because Squid
invasive RefCount smart pointers cannot be stored directly in various
configuration objects without significant out-of-scope refactoring.
* Acl::Node::next items formed an invasive list of raw ACL pointers
(rooted in Config.aclList). The list was used to FindByName() and also
to prepareForUse() explicitly named ACLs. Reconfiguration destroyed
listed nodes via aclDestroyAcls(). Acl::Node::next is now gone.
* RegisteredAcls std::set stored raw ACL pointers to destroy all ACL
objects (named and internally-generated) during reconfiguration, after
all the free_foo() calls, in aclDestroyAcls(). It is now gone.
* InnerNode::nodes std::vector stored raw pointers to
internally-generated Acl::Node objects. They are now refcounted.
The remaining raw Acl::Node pointers are not actually _stored_ across
async calls. They are used to pass ACLs around. Some of them should be
replaced with references (where it is known that the ACL must exist and
is not expected to be stored), but that is a different polishing issue.
For now, we just make sure that no raw Acl::Node pointers are stored
across async calls (because any stored raw ACL pointer may be
invalidated by reconfiguration).
Extending lifetime of in-use ACLs is necessary but not sufficient to
make Squid reconfiguration reliable. We need to preserve more in-use
resources...
[^1]: Prior to these changes, reconfiguration made Acl::Node objects
unusable because their C++ destructors were called. Cbdata locks may
have kept underlying object memory, but any linked resources freed by
destructors were gone, and the destructed objects could not be accessed.
gmake[3]: Entering directory .../libltdl
.../cfgaux/missing: line 85: aclocal-1.16: command not found
gmake[3]: *** [Makefile:561: .././../libltdl/aclocal.m4] Error 127
During bootstrap.sh run, libtoolize copies prepackaged configure and
Makefile.in files into our libltdl directory:
* libltdl/configure from libtool v2.4 has aclocal version set to 1.16;
* libltdl/Makefile.in from libtool v2.4 uses configure-set aclocal
version to build aclocal.m4
Thus, libltdl/Makefile (generated from libltdl/Makefile.in above) runs
aclocal-1.16 if "make" needs to build libltdl/aclocal.m4.
Normally, "make" does not need to build libltdl/aclocal.m4 because that
file was created by libtoolize. However, libtool v2.4 is packaged with
(generated by packaging folks) libltdl/Makefile.in that makes
libltdl/aclocal.m4 target dependent on files in libltld/../m4 directory.
Squid does not have that ./m4 directory, so "make" attempts to
re-generate libltdl/aclocal.m4. When it does, it uses aclocal-1.16.
Our bootstrap.sh generated new ./configure but preserved copied
libltdl/configure with its aclocal version set to 1.16. In other words,
our bootstrap.sh did not bootstrap libltdl sub-project. In build
environments without aclocal-1.16, Squid build failed.
Several solutions or workarounds have been tried or considered:
* Adjust bootstrap.sh to bootstrap libltdl (this change). 2008 attempt
to do that was reverted in commit bfd6b6a9 with "better to fix libtool
installation" rationale. Another potential argument against this
option is that packages should be bootstrapped by their distributors,
not "users". We are not distributing libtool, but this is a gray area
because we do distribute files that libtoolize creates. Finally,
libtool itself does not provide a bootstrapping script and does not
explicitly recommend bootstrapping in documentation.
* "Fix libtool installation". We failed to find a good way to do that on
MacOS (without building and installing newer libtool from sources).
* Place m4 files where libtool v2.4 expects to find them. That change
fixes MacOS builds that use automake v1.17, but breaks Gentoo builds
because Gentoo libtool installs a buggy libltdl/Makefile.in that must
be regenerated by automake before it can work. Fixing m4 files
location prevents that regeneration.
We picked the first option despite its drawbacks because the third
option did not work on Gentoo, and asking Squid developers to install
libtool from sources (i.e. the second option) felt like a greater evil.
This old problem was exposed by recently introduced CI MacOS tests that
started to fail when MacOS brew updated automake package from v1.16
without the corresponding libtoolize package changes.
Also work around what seems to be a libtool packaging bug affecting
MacOS/brew environments, including GitHub Actions runners we use for CI:
That workaround will be removed after libtool package is fixed.
Also removed a single-iteration "for dir" loop with several stale hacks
from bootstrap.sh: With only two directories to bootstrap and with a
directory-specific mkdir command, source comments, and progress
messages, it is best to unroll that loop.
Alex Rousskov [Tue, 16 Jul 2024 01:38:59 +0000 (01:38 +0000)]
Enable EDNS for DNS A queries and reverse IPv4 lookups (#1864)
This change brings Squid code in sync with existing dns_packet_max
directive documentation and allows admins to enable a useful performance
optimization for still very popular IPv4-related DNS queries.
An enabled dns_packet_max feature may now break Squid compatibility with
more buggy nameservers (that appeared to "work" before), but we should
not penalize many Squid deployments (or complicate configuration and
spend a lot of development time) to accommodate a few exceptional ones,
at least until such heroic efforts are proven to be necessary.
Alex Rousskov [Mon, 15 Jul 2024 21:59:54 +0000 (21:59 +0000)]
Do not drop queued TCP DNS queries when queuing a new one (#1863)
Since 2005 commit 032785bf, previously queued serialized DNS query
(waiting for a TCP connection to the nameserver or for a write on an
already established TCP connection) was erased every time a new query
was serialized and appended to the `vc->queue` MemBuf. Thus, at most one
TCP query was submitted when the nameserver became available.
Non-serialized versions of erased queries remained on the DNS lru_list
and would eventually be retried or timeout. This change restores
conditional MemBuf initialization that was replaced with an
unconditional MemBuf reset in that 2005 commit.
It would take more than 5 hours of 1000/s unique request load for
100-byte DNS queries to overflow a 2GB MemBuf in a properly functioning
Squid, but this change does not rely on MEM_BUF_MAX_SIZE always being
large enough. New code prevents MemBuf::grow() assertions and informs
the admin about dropped queries that may indicate a Squid bug or a
persistent problem with nameserver communication.
Alex Rousskov [Thu, 11 Jul 2024 20:04:06 +0000 (20:04 +0000)]
Be more careful when updating nameservers global (#1862)
Due to I/O delays and timeouts, DNS nsvc objects may be deleted after
the `nameservers` global pointing to them was cleared and then populated
with new ns objects. Thus, nsvc destructor should check that nsvc and
`nameservers` states are still in sync before manipulating the latter.
DNS code should use similar validation in a few other places, but they
are all about read-only debugging that requires a rather noisy cleanup.
Alex Rousskov [Wed, 10 Jul 2024 10:30:33 +0000 (10:30 +0000)]
Recover after failing to open a TCP connection to DNS server (#1861)
ERROR: Failed to connect to nameserver 127.0.0.1 using TCP.
After failing to establish a TCP connection to a DNS server, all DNS
queries that needed a TCP connection to that DNS server would timeout
because the nsvc object representing TCP connectivity got stuck in a
"queuing new queries but too busy to send any right now" state. Such
timeouts typically lead to HTTP 503 ERR_DNS_FAIL responses. This bug was
introduced when Comm closure handler registration was moved/delayed in
2010 commit cfd66529.
With this change, the affected nsvc object is destroyed, and Squid
attempts to open another TCP connection to the DNS server (when needed).
The original query is typically retried (subject to dns_timeout and
dns_retransmit_interval idiosyncrasies).
XXX: This fix increases the surface of reconfiguration and shutdown
problems documented in nsvc class destructor XXX.
This change fixes how cbdata is used for managing CachePeer::digest
lifetime. Prior to these changes, cbdata was (ab)used as a reference
counting mechanism: CachePeer::digest object could outlive CachePeer
(effectively its creator), necessitating complex "is it time to delete
this digest?" cleanup logic. Now, CachePeer is an exclusive digest owner
that no longer locks/unlocks its digest field; it just creates/deletes
the object. Digest fetching code no longer needs to cleanup the digest.
"CachePeer::digest is always valid" invariant simplifies digest fetching
code and, hopefully, reduces the probability of bugs similar to Bug 5329
fixed in minimal commit 4657405 (that promised this refactoring).
Removed effectively unused acl_checklist members (#1860)
Four classes had problematic acl_checklist data members:
* SnmpRequest does not use acl_checklist at all.
* ClientRequestContext and Adaptation::AccessCheck do not use
acl_checklist beyond checklist creation/configuration code. A local
variable works better for creation/configuration, and having a data
member complicates upcoming checklist creation code improvements.
* PeerSelector creates/configures a checklist and uses acl_checklist
during destruction, but the latter code is marked as a Squid BUG and
is itself buggy: Checklist objects are cbdata-protected. They must
have one owner at any time. Starting with a nonBlockingCheck() call,
that owner is the checklist object itself. That owner destructs the
checklist object (i.e. "this"). If that Squid BUG code could be
reached, Squid would delete the same object twice. There are no known
ways to trigger that bug.
Alex Rousskov [Sun, 7 Jul 2024 03:03:00 +0000 (03:03 +0000)]
Fix Tokenizer::int64() parsing of "0" when guessing base (#1842)
Known bug victims in current code were tcp_outgoing_mark,
mark_client_packet, clientside_mark, and mark_client_connection
directives as well as client_connection_mark and (deprecated)
clientside_mark ACLs if they were configured to match a zero mark using
"0" or "0/..." syntax:
Alex Rousskov [Tue, 2 Jul 2024 17:35:14 +0000 (17:35 +0000)]
Restrict squid.conf preprocessor space characters to SP and HT (#1858)
Here, "preprocessor space" refers to a portion of squid.conf grammar
that covers preprocessor directives (e.g., `if` and `include`
statements) and separation of a directive name from directive
parameters. This change replaces all known xisspace() uses in
preprocessor code with code that only recognizes SP and HT as space
characters.
Prior to this change, Squid preprocessor also classified as space ASCII
VT and FF characters as well as any other locale-specific characters
classified as space by isspace(3).
Squid preprocessor still delimits input lines using LF and terminates
each read line at the first CR or LF character (if any), whichever comes
earlier. This behavior precludes CR and LF characters from reaching
individual directive line parsers. However, directive parameter values
loaded by ConfigParser when honoring "quoted filename" or
parameters(filename) syntax are (still) tokenized using w_space which
includes an ASCII CR character. Thus, a "foo_CR_bar" 9-character
sequence is still interpreted as a single "foo_" token by the
preprocessor and as two tokens ("foo_" and "_bar") by
ConfigParser::strtokFile().
After preprocessing, directive parameters are (still) parsed by parsers
specific to that directive type. Many of those parsers are based on
ConfigParser::TokenParse() that effectively uses the same "SP and HT
only" logic for inlined directive parameters. However, some of those
parsers define "space" differently (and may use non-space-like
characters for token delimiters). For example, response_delay_pool still
indirectly uses isspace(3) to parse integer parameters, and the
following "until eol" directives still use xisspace() to skip space
before their parameter value: debug_options, err_html_text,
mail_program, sslcrtd_program, and sslcrtvalidator_program.
More than 100 uses of xisspace() and indirect uses of isspace(3) remain
in Squid code. Most of those use cases are in configuration parsing
code. Restricting all relevant use cases one-by-one may not be
practical. On the other hand, restricting all configuration input lines
to prohibit VT, FF, CR, and locale-specific characters classified as
space by isspace(3) will break rare configs that use those characters in
places like URL paths and regexes.
Due to inconsistencies highlighted above, there is no consensus
regarding this change among Squid developers. This experiment may need
to be reverted or further grammar changes may need to be implemented
based on testing and deployment experience.
Alex Rousskov [Thu, 27 Jun 2024 22:12:40 +0000 (22:12 +0000)]
CI: Upgrade GitHub upload-artifact action to v4 (#1852)
The following artifacts were uploaded using a version of
actions/upload-artifact that is scheduled for deprecation:
"build-logs-macos-14-Object", "build-logs-ubuntu-22.04-Object",
"test-logs".
This change finishes GitHub actions upgrade started in recent commit 8805c474 and removes all remaining GitHub Actions upgrade warnings.
Upgrading to actions/upload-artifact@v4 is not trivial because the new
action version uses "immutable (unless deleted)" artifacts: One GitHub
Actions job cannot add artifacts to a container created by another job.
We now add build layer dimension to the container name created inside
"build-tests" matrix, so that all layer logs are uploaded, with one
artifact container per matrix job.
Also fixed the compiler component in build-tests artifact names: Recent
commit 5a7811af intended to add "compiler dimension to avoid name
clashes" but only added a constant "-Object" suffix, resulting in log
name clashes on Ubuntu runners that test using two compilers.
CI: Use packages from GitHub Actions MacOS runner base-image (#1850)
openldap 2.6.8 is already installed and up-to-date.
openssl@3 3.3.1 is already installed and up-to-date.
Do not "brew install" openldap and openssl packages to avoid GitHub
Actions warnings. If GitHub stops pre-installing these packages, then
MacOS tests will fail because Squid is not compatible with MacOS's
native frameworks for LDAP and OpenSSL.
Scaffolding for YAML-formatted cache manager reports (#1784)
The only YAML-compliant report currently using new code is mgr:pconn.
Non-YAML non-aggregated reports in non-SMP instances are now framed
using "by kid0 {...}" wrappers (for `squid -N`) or "by kid1 {...}"
wrappers (for instances having one worker process and no rock diskers).
This change makes YAML and non-YAML report framing more similar, but may
affect existing report parsing automation.
Also fixed Content-Type value computation for SMP reports: They were
missing ";charset=utf-8" suffix. Broken since 2014 commit 8088f8d0,
possibly due to code duplication created by 2010 commit 8822ebee.
Moved FunActionCreator and ClassActionCreator classes into
src/mgr/Registration.cc because no other code needs to know about them.
This is especially valuable for function-based actions because they do
not need to know about Mgr::FunAction (or even Mgr::Action).
Alex Rousskov [Mon, 24 Jun 2024 17:30:06 +0000 (17:30 +0000)]
Fix parallel "make check": testHeaders for same-basename files (#1846)
header-test: ok - src/ipc/forward.h
header-test: ok - src/ipc/ReadWriteLock.h
cc1plus: fatal error: forward.hdrtest.cc: No such file or directory
header-test: not ok - src/ipc/mem/forward.h
In a parallel build, testHeaders often tests header files using multiple
concurrent "make" processes. When testing same-basename files (e.g.,
src/ipc/forward.h and src/ipc/mem/forward.h), one "make" process may
delete the file generated by another process (leading to random false
"make check" failures like the one shown above) or even substitute bad
file contents with valid one (leading to false positives).
Clashes among temporary generated files can be avoided using unique
temporary directories, but those are difficult to create in a portable
way. Instead, we use "make" PID to guarantee file name uniqueness across
parallel make processes.
Alex Rousskov [Sat, 22 Jun 2024 02:49:41 +0000 (02:49 +0000)]
CI: Upgrade GitHub Setup Node and CodeQL actions to Node 20 (#1845)
These major action version upgrades are recommended by GitHub that is
deprecating Node 16: Node 16 stopped receiving security updates in
September 2023. Node 20 will reach that state around April 2026.
* actions/setup-node@v4: The primary difference is that Setup Node
action v4 uses Node 20, while v3 uses Node 16. Also, Setup Node v4
adds support for arm64 Windows and other secondary improvements.
* github/codeql-action@v3: The only difference is that CodeQL Action v3
runs on Node 20, while CodeQL Action v2 runs on Node 16.
This change is necessary but not sufficient to address current GitHub
Actions upgrade warnings. We also need to upgrade upload-artifact@v3,
but that upgrade has side effects that deserve a dedicate change.
Since 2010 commit 7de94c8c, only worker processes made RegisterAction()
calls associated with the affected cache manager reports. Coordinator
process needs these registrations as well because it uses Mgr::Action
code while iterating report-generating kids[^1].
When fixed Coordinator asks a disker process for an Action report, that
process must recognize the action as well (even when disker has no
information to supply) to avoid triggering similar Action lookup BUGs.
[^1]: Coordinator mostly needs Mgr::Action for stats aggregation, but
even non-aggregating actions (like the fixed three) need registered
Mgr::ActionProfile objects for Coordinator to know whether to aggregate.
Alex Rousskov [Fri, 14 Jun 2024 16:15:16 +0000 (16:15 +0000)]
Fix reporting of unrecognized directives (#1841)
When Squid does not recognize a directive name, it reports the problem
and proceeds to parse the remaining configuration lines. When parsing is
complete, Squid quits if it has encountered such directives. This change
preserves this overall behavior while fixing related reporting problems.
Fixed problems depend on whether Squid discovered an unrecognized
directive at startup or during reconfiguration.
Startup configuration case was missing both an ERROR classification and
a FATAL message, resulting in a somewhat mysterious death:
Processing Configuration File: squid.conf (depth 0)
squid.conf(7): unrecognized: 'http_accesses'
Starting Authentication on port [::]:4443
Disabling Authentication on port [::]:4443 (interception enabled)
If other non-fatal ERRORs were present, that startup death reporting
became more problematic because admins would naturally (but incorrectly)
assume that Squid died due to those irrelevant ERRORs:
Reconfiguration case was worse because Squid clearly attributed failure
to the wrong (innocent and default) directive that was not even present
in the configuration file (among other problems like saying "null"):
Squid now reports unrecognized directives using the standardized ERROR
prefix and prints correct FATAL message in both cases:
Processing Configuration File: squid.conf (depth 0)
ERROR: unrecognized directive near 'http_accesses'
directive location: squid.conf(7)
FATAL: reconfiguration failure: Found 1 unrecognized directive(s)
exception location: cache_cf.cc(610) Parse
advice: Run 'squid -k parse' and check for ERRORs.
Squid Cache (Version 7.0.0-VCS): Terminated abnormally.
Also reduced the number of nested try/catch blocks, simplifying
configuration code. However, this change is just a small step forward.
More fatal (re)configuration error handling improvements are needed,
including removing unwanted and noisy fatal() side effects and possibly
recognizing other kinds of errors that should not immediately terminate
configuration parsing.
Allocate fast-checking ACLFilledChecklists on stack (#1835)
There is no need for dynamic allocation risks and performance overheads
in these simple "immediate fastCheck()" cases.
This change converts all dynamically allocated checklists that use only
fastCheck() calls except one: Security::PeerConnector::initialize() has
to allocate its checklist dynamically to pass it to SSL_set_ex_data().
Alex Rousskov [Thu, 6 Jun 2024 07:24:52 +0000 (07:24 +0000)]
Do not die when parsing obsolete log_access and log_icap (#1832)
All obsolete directives except for log_access and log_icap do not result
in Squid instance death today. Treating those two directives specially
is not necessary but does complicate reconfiguration improvements a bit.
Future refactoring might add support for treating (some or all) obsolete
directives as fatal configuration errors, but we can simplify for now.
Alex Rousskov [Wed, 5 Jun 2024 18:55:06 +0000 (18:55 +0000)]
Improve ErrorState debugging (#1768)
In triage, it is often useful to know that ErrorState was created _when_
it was created rather than waiting for errorAppendEntry() or errorSend()
debugging, especially if those error handling stages are not reached.
Also report HTTP status code of the error response (when available).
TrieNode::add() incorrectly computed an offset of an internal data
structure, resulting in out-of-bounds memory accesses that could cause
corruption or crashes.
This bug was discovered and detailed by Joshua Rogers at
https://megamansec.github.io/Squid-Security-Audit/esi-underflow.html
where it was filed as "Buffer Underflow in ESI".
Ident protocol (RFC 931 obsoleted by RFC 1413) has been considered
seriously insecure and broken since at least 2009 when SANS issued an
update recommending its removal from all networks. Squid Ident
implementation suffered from assertions (since 2021 commit e227da8) and
memory leaks (since 2015 commit fbbea66). Ident implementation design
increased ACLChecklist::goAsync() design complexity.
An external ACL helper can be written to perform Ident transactions.
Configurations using ident/ident_regex ACLs, %ui logformat codes, %IDENT
external_acl_type format code, or ident_lookup_access/ident_timeout
directives are now rejected, leading to fatal startup failures:
To avoid inconveniencing admins that do _not_ use Ident features, access
logs with "common" and "combined" logformats now always receive a dash
in the position of what used to be a %ui record field.
annotate_client and annotate_transaction ACLs must always match (#1820)
WARNING: markAsTunneled ACL is used in context without an HTTP
request. Assuming mismatch.
Our annotate_client and annotate_transaction ACLs are documented as
"always matching", and some existing Squid configurations rely on that
invariant. Both ACLs did not match when they lacked access to the
current transaction information because their requiresRequest() method
returned true; annotate_client ACL also did not match after the
client-to-Squid connection was gone and when the transaction was not
associated with a client-to-Squid connection.
This change makes ACL code conform to documentation. Squid still warns
the admin if an ACL cannot annotate. Such warnings may indicate s Squid
bug or misconfiguration, but mismatching in those cases causes more harm
because it makes it impossible for the admin to rely on the primary
matching invariant. "One reliable invariant plus one unreliable side
effect" is the lesser evil than "two unreliable effects".
# the following must deny even if it cannot annotate
http_access deny markAsDenied
# the following might not log denied traffic (with a prior warning)
access_log syslog:daemon.err markedAsDenied
Also fixed annotate_client to annotate the current transaction even
after ConnStateData destruction. Such annotations may happen when, for
example, Squid continues a large download after the HTTP client is gone.
Alex Rousskov [Mon, 20 May 2024 14:50:19 +0000 (14:50 +0000)]
Docs: REQUIRED in ident_regex, proxy_auth_regex, ext_user_regex (#1818)
The three ACLs were documented as matching any username when configured
with a parameter spelled "REQUIRED". Neither actually treated that
parameter specially -- all interpreted it as an ordinary regex.
This dangerous documentation bug was introduced in 2000 commit 145cf92
that added ident_regex and proxy_auth_regex support. It was then
duplicated in 2003 commit abb929f that added ext_user_regex support.
This minimal documentation fix does not imply that these ACLs should not
treat REQUIRED values specially. Enabling such special treatment
requires significant code changes, especially if we want to do that well
and without duplicating the corresponding code.
Fix build with clang v18 [-Wvla-cxx-extension] (#1813)
src/fs/rock/RockRebuild.cc:356:17: error: variable length arrays
in C++ are a Clang extension [-Werror,-Wvla-cxx-extension]
char hdrBuf[SwapDir::HeaderSize];
note: initializer of 'HeaderSize' is unknown
Ensure ACLChecklist::markFinished() is called (#1809)
For example, ACLChecklist::resumeNonBlockingCheck() missed a
markFinished() call when prepNonBlocking() returned false after a
reconfiguration. Missing markFinished() calls result in the last
evaluated ACL name being unset in nonBlockingCheck() callbacks.
CI: Add clang to GitHub Actions build-tests matrix (#1812)
Add "compiler" dimension to build-tests matrix.
Do not test layer-04-noauth-everything to partially compensate for the
new matrix dimension repeating other layer tests 3 times.
Change build-tests artifacts name pattern to include compiler dimension
to avoid name clashes that would result in logs being overwritten and
lost. Also use target environment instead of runner environment in
anticipation of adding container-based tests where the two differ.
Maintenance: No ptr copy in future CredentialsCache::insert()s (#1807)
Currently, the insert() method calls implicitly convert raw "this"
pointers to RefCount pointers. That self-registration code raises red
flags and may eventually be refactored. If it is refactored, insert()
calls are likely to start using RefCount pointers as parameters. This
change allows those future calls to avoid RefCount pointer copies. This
change does not affect current calls performance.
This change was triggered by Coverity CID 1554665: Unnecessary object
copies can affect performance (COPY_INSTEAD_OF_MOVE). However, the
insert() method is still calling RefCount copy assignment operator
rather than its (cheaper) move assignment operator. Safely removing that
overhead is only possible by reintroducing future parameter copying
overhead described above _and_ using an explicit std::move() call that
we are trying to avoid in general code. It is arguably not worth it.
Alex Rousskov [Fri, 3 May 2024 02:00:08 +0000 (02:00 +0000)]
Bug 5322: Do not leak HttpReply when checking http_reply_access (#1764)
... as well as response_delay_pool and send_hit directives.
auto check = clientAclChecklistCreate(...); // sets check->reply
check->reply = reply; // self-assignment does nothing
HTTPMSGLOCK(check->reply); // an unwanted/extra lock
When ACLFilledChecklist::reply is already set to X, resetting it to X
should not change HttpReply lock count, but some manual locking code did
not check that "already set" precondition and over-locked reply objects
set to ClientHttpRequest::al::reply by clientAclChecklistFill().
Current known leaks were probably introduced in 2021 commit e227da8 that
started supplying HttpReply to ACLChecklist in clientAclChecklistFill().
The added code locked the reply object correctly, but it was
incompatible with unconditional manual locks in three existing indirect
clientAclChecklistFill() callers (calling clientAclChecklistCreate()).
This change removes all known similar leaks and improves
ACLFilledChecklist API to prevent future similar leaks.
Alex Rousskov [Sat, 13 Apr 2024 08:15:00 +0000 (08:15 +0000)]
Bug 5352: Do not get stuck when RESPMOD is slower than read(2) (#1777)
... RESPMOD BodyPipe buffer becomes full ...
maybeMakeSpaceAvailable: will not read up to 0
The AsyncCall Client::noteDelayAwareReadChance constructed
... RESPMOD consumes some buffered virgin body data ...
entering BodyProducer::noteMoreBodySpaceAvailable
leaving BodyProducer::noteMoreBodySpaceAvailable
When RESPMOD does not empty its adaptation BodyPipe buffer fast enough,
readReply() may eventually fill that buffer and call delayRead(),
anticipating a noteDelayAwareReadChance() callback from Store or Server
delay pools. That callback never happens if Store and Server are not
getting any data -- they do not even start working until RESPMOD service
starts releasing adapted/echoed response back to Squid! Meanwhile, our
flags.do_next_read (cleared by readReply() caller) remains false.
When/if RESPMOD service eventually frees some BodyPipe buffer space,
triggering noteMoreBodySpaceAvailable() notification, nothing changes
because maybeReadVirginBody() quits when flags.do_next_read is false.
noteMoreBodySpaceAvailable() could not just make flags.do_next_read true
because that flag may be false for a variety of other/permanent reasons.
Instead, we replaced that one-size-fits-all flag with more specific
checks so that reading can resume if it is safe to resume it. This
change addresses a couple of flag-related XXXs.
The bug was introduced in 2023 commit 50c5af88. Prior that that change,
delayRead() was not called when RESPMOD BodyPipe buffer became full
because maybeMakeSpaceAvailable() returned false in that case, blocking
maybeReadVirginBody() from triggering readReply() via Comm::Read(). We
missed flags.do_next_read dependency and that Store-specific delayRead()
cannot be used to wait for adaptation buffer space to become available.
XXX: To reduce risks, this change duplicates a part of
calcBufferSpaceToReserve() logic. Removing that duplication requires
significant (and risky) refactoring of several related methods.
Alex Rousskov [Mon, 8 Apr 2024 22:06:02 +0000 (22:06 +0000)]
Upgrade Acl::Node::name to SBuf; remove AclMatchedName global (#1766)
AclMatchedName global has been localized into a regular Acl::Answer data
member (Acl::Answer maintains the result of ACLChecklist evaluations).
This long overdue change resolves an old TODO and XXXs, paving the way
for Acl::Node reference counting.
No significant functionality changes are expected, but it is possible
that some deny_info configurations will now be handled better in
reconfiguration corner cases (because Squid no longer forgets the name
of the last checked ACL when a slow ACL check crosses reconfiguration
barrier).
Most of these changes are performance-neutral or -positive because they
eliminate or reduce memory allocations and associated name copying (and
more reduction will become possible after upgrading squid.conf parsers
to use SBuf). This change adds SBuf object copies when Acl::Answer is
propagated to ACLCB callbacks, but those read-only copies are cheap.
Also renamed and polished aclGetDenyInfoPage() because we had to update
its parameter type (to supply the last evaluated ACL). All callers were
also supplying the same first argument (that is unlikely to change in
the foreseeable future); that argument is now gone. We did not fix the
redirect_allowed name and debugs(): Fixing that behavior deserves a
dedicated change.
Also polished legacy aclIsProxyAuth() profile and description because we
have to change the parameter type (to supply the last evaluated ACL).
Also removed 63-character aclname parameter limit for acl directives.
Alex Rousskov [Wed, 3 Apr 2024 09:25:55 +0000 (09:25 +0000)]
Fix const-correctness of ACLHTTPHeaderData::match() parameter (#1771)
ACLHTTPHeaderData::match() required a pointer to non-const HttpHeader
but does not (and should not) modify the supplied HttpHeader.
Also removed support for nil HttpHeader in that method. All callers
already require HttpHeader presence. If that changes, it is the _caller_
that should decide what HttpHeader absence means (match, mismatch,
exception/dunno, etc.); ACLHTTPHeaderData does not have enough
information to make the right choice and, hence, should not be asked to
choose.
Also polished related #includes to remove unnecessary ones.
Alex Rousskov [Tue, 2 Apr 2024 20:37:31 +0000 (20:37 +0000)]
Do not blame cache_peer for CONNECT errors (#1772)
ERROR: Connection to [such-and-such-cache_peer] failed
TCP_TUNNEL/503 CONNECT nxdomain.test:443 FIRSTUP_PARENT
Squid does not alert an admin about (and decrease health level of) a
cache_peer that responded with an error to a GET request. Just like GET
responses from a cache_peer, CONNECT responses may (and often do!)
reflect client or origin server failures. We should not penalize
cache_peers (and alert admins) until we can distinguish these frequent
client/origin failures from (relatively rare) cache_peer problems. This
change absolves cache_peers of CONNECT problems, restoring parity with
GETs and restoring v4 behavior changed (probably by accident) in v5.
Also removed Http::StatusCode parameter from failure notification
functions because it became essentially unused after the primary
Http::Tunneler changes. Tunneler was the only source of status code
information that (in some cases) used received HTTP response to compute
that status code. All other cases extracted that status code from
Squid-generated errors. Those errors were arguably never meant to supply
status code information for "this failure is not our fault" decision,
and they do not supply 4xx status codes driving that decision.
### Problem evolution
2019 commit f5e1794 effectively started blaming cache_peer for all
FwdState CONNECT errors. That functionality change was probably
accidental, likely influenced by the names of noteConnectFailure() and
peerConnectFailed() functions that abbreviated "Connection", making the
functions look as applicable to CONNECT failures. Prior to that commit,
the functions were never used for CONNECT errors. After it, FwdState
started calling peerConnectFailed() for all CONNECT failures.
In 2020 commit 25b0ce4, TunnelStateData started blaming cache_peers as
well (by moving that FwdState-only error handling code into Tunneler).
The same "accidental functionality change" speculations apply here.
In 2022 commit 022dbab, we made an exception for 4xx CONNECT errors as
folks deploying newer code started complaining about cache_peers getting
blamed for client-caused errors (e.g., HTTP 403 Forbidden replies). We
did not realize that the blaming code itself was an unwanted accident.
Now we are getting complaints about cache_peers getting blamed for 502
and 503 CONNECT errors caused by, for example, domain names without IPs:
As these CONNECT error responses are propagated from parent to child
caches, every child cache in the chain logs ERRORs and every cache_peer
in the chain gets its health counter decreased!
Class constructor and destructor sources should not manually duplicate
class names in debugs() statements. Many existing classes do the right
thing, but find-alive.pl still insists on a manually added class name,
forcing its users to add a colon: `find-alive.pl MemBlob:`.
We now accommodate both legacy/manual and modern/auto use cases.
When FwdState decides to re-forward a request, it forgets the original
response[^1] but does not create an error object. Since commit e2bbd3b,
FwdState::noteDestinationsEnd() correctly assumed that we only idly wait
for additional destinations after encountering a problem, but
incorrectly asserted that past problems imply error object existence.
Now Squid generates an HTTP 502 (Bad Gateway) response while setting
%err_code/%err_detail to ERR_CANNOT_FORWARD/REFORWARD_TO_NONE.
TunnelStateData::noteDestinationsEnd() code is similar, but it probably
does not suffer from the same bug because an error object is created
before every retryOrBail() call, and there is no re-forwarding code that
forgets an HTTP error response without creating an error. Those
invariants are not well tracked, and this change mimics FwdState changes
in TunnelStateData just in case and to keep the two methods in sync. In
those hypothetical cases, ERR_CANNOT_FORWARD/RETRY_TO_NONE is logged.
[^1]: Long-term we probably want to preserve that original response so
that we do not have to replace it with a generated error, but doing so
requires significant refactoring and is outside this minimal fix scope.
Alex Rousskov [Wed, 27 Mar 2024 18:41:41 +0000 (18:41 +0000)]
CI: Require source-maintenance.sh application before merging PRs (#1760)
By default, fixing PR-introduced code formatting problems should be that
PR responsibility. We did not enforced formatting because we did not
have CI automation to support that enforcement, and also because it was
tedious even for core developer to maintain the right astyle version.
Neither excuse remains valid these days.
In most development environments, it is not very difficult to install
Astyle v3.1 (and other Squid source maintenance tools). Our GitHub
Actions configuration file (.github/default.yaml) has a recipe. In
exceptional cases, including cases where a pull request was not
formatted because a different astyle version was installed, other
developers can (re)format PR sources (proactively or at author request).
Also fixed wrong error message when running source-maintenance.sh fails.
projects / thirdparty / squid.git / log
