Bug 4223: fixed retries of failed re-forwardable transactions (#211)
This change fixes Store to delay writing (and, therefore, sharing) of
re-triable errors (e.g., 504 Gateway Timeout responses) to clients:
once we start sharing the response with a client, we cannot re-try the
transaction. Since ENTRY_FWD_HDR_WAIT flag purpose is to delay response
sharing with Store clients, this patch fixes and clarifies its usage:
1. Removes unconditional clearing from startWriting().
2. Adds a conditional clearing to StoreEntry::write().
3. Sets it only for may-be-rejected responses.
(2) adds ENTRY_FWD_HDR_WAIT clearing to detect responses that filled the
entire read buffer and must be shared now with the clients because
they can no longer be retried with the current re-forwarding mechanisms
(which rely on completing the bad response transaction first) and will
get stuck. Such large re-triable error responses (>16KB with default
read_ahead_gap) should be uncommon. They were getting stuck prior to
master r12501.1.48. That revision started clearing ENTRY_FWD_HDR_WAIT in
StoreEntry startWriting() unconditionally, allowing all errors to be
sent to Store clients without a delay, and effectively disabling
retries.
Mgr::Forwarder was always setting ENTRY_FWD_HDR_WAIT, probably mimicking
similarly aggressive FwdState behavior that we are now removing. Since
the forwarder never rewrites the entry content, it should not need to
set that flag. The forwarder and associated Server classes must not
communicate with the mgr client during the client-Squid connection
descriptor handoff to Coordinator, but ENTRY_FWD_HDR_WAIT is not the
right mechanism to block such Squid-client communication. The flag
does not work well for this purpose because those Server classes may
(and do?) substitute the "blocked" StoreEntry with another one to
write an error message to the client.
Also moved ENTRY_FWD_HDR_WAIT clearance from many StoreEntry::complete()
callers to that method itself. StoreEntry::complete() is meant to be the
last call when forming the entry. Any post-complete entry modifications
such as retries are prohibited.
Amos Jeffries [Tue, 5 Jun 2018 06:11:29 +0000 (06:11 +0000)]
Bug 4831: filter chain certificates for validity when loading (#187)
51e09c08a5e6c582e7d93af99a8f2cfcb14ea9e6 adding
GnuTLS support required splitting the way
certificate chains were loaded. This resulted in the
leaf certificate being added twice at the prefix of a
chain in the serverHello.
It turns out that some recipients validate strictly that the
chain delivered by a serverHello does not contain extra
certificates and reject the handshake if they do.
This patch implements the XXX about filtering certificates
for chain sequence order and self-sign properties, added
in the initial PR. Resolving the bug 4831 regression and also
reporting failures at startup/reconfigure for admins.
Also, add debug display of certificate names for simpler
detection and administrative fix when loaded files fail
these tests.
Bug 4855: re-enable querying private entries for HTCP/ICP (#214)
This was broken since 4310f8b: HTCP/ICP misused Store as a storage of
private entries during queries (e.g., see
neighborsUdpPing()/neighborsUdpAck()). A smarter HTCP/ICP
implementation would maintain its own StoreEntry cache for this purpose
(just like the existing queried_keys array for cache keys). However,
fixing this is beyond this issue scope.
Amos Jeffries [Tue, 22 May 2018 12:55:35 +0000 (12:55 +0000)]
Bug 4707: purge tool does not obey --sysconfdir= build option (#210)
The purge tool was still using DEFAULT_SQUID_CONF macro from
before it was bundled with Squid, which had no connection to our
./configure script.
Update it to the DEFAULT_CONFIG_FILE macro used by other Squid
binaries and fix some existing issues with that macro's use by
binaries outside 'squid'.
Amos Jeffries [Sun, 20 May 2018 15:46:50 +0000 (15:46 +0000)]
Bug 4843 pt2: squidclient refactoring for GCC-8 (#208)
Replace fixed size buffers for mime header block and additional
custom headers. This fixes long standing issues with buffer
overflow from large custom header values which have become a
hard error in GCC-8.
Also improve snprintf() URL buffer limit handling and const
correctness for Transport::Write().
Bug 4811: supply AccessLogEntry (ALE) for more fast ACL checks. (#182)
Supplying ALE for fast ACL checks allows those checks to use ACLs that
assemble values from logformat %codes. Today, such ACLs are limited to
misplaced external ACLs (that should not be used with "fast"
directives!), but it is likely that fast ACLs like annotate_client will
eventually require ALE.
The "has" ACL documentation promises ALE for every transaction, but our
code does not deliver on that promise. This change fixes a dozen of
easy cases where ALE was available nearby. Also a non-trivial
cache_peer_access case was fixed, which proved to be more complex
because of the significant call depth of the peerAllowedToUse() check,
which is a known design problem of its own.
More cases need fixing, and the whole concept of ALE probably needs to
be revised because logformat %code expansion is needed in the
increasing number of contexts that have nothing to do with access
logging.
Also fixed triggering of (probably pointless) level-1 warnings:
* ALE missing adapted HttpRequest object
* ALE missing URL
With fix applied, any ACLChecklist with ALE synchronizes it at
'pre-check' stage without logging level-1 warnings. Warnings are
triggered only if for some reason this 'pre-check' synchronization was
bypassed.
Amos Jeffries [Sun, 13 May 2018 06:57:41 +0000 (06:57 +0000)]
Bug 4843 pt1: ext_edirectory_userip_acl refactoring for GCC-8 (#204)
Proposed changes to this helper to fix strcat / strncat buffer
overread / overflow issues.
The approach takes three parts:
* adds a makeHexString function to replace many for-loops
catenating bits of strings together with hex conversion into a
second buffer. Replacing with a snprintf() and buffer overflow
handling.
* a copy of Ip::Address::lookupHostIp to convert the input
string into IP address binary format, then generate the hex
string using the above new hex function instead of looped
sub-string concatenations across several buffers.
This removes all the "00" and "0000" strncat() calls and
allows far simpler code even with added buffer overflow
handling.
* replace multiple string part concatenations with a few simpler
calls to snprintf() for all the search_ip buffer constructions.
Adding buffer overflow handling as needed for the new calls.
huaraz [Sun, 6 May 2018 16:06:42 +0000 (16:06 +0000)]
Bug 4042: ext_kerberos_ldap_group: add -P principal option (#195)
Added a -P principal option to ext_kerberos_ldap_group to
select a principal from the keytab overwriting the automated
method which may make it more responsive.
Bug 4845: NegotiateSsl crash on aborting transaction (#201)
Security::PeerConnector::NegotiateSsl() might be called after the
Security::PeerConnector object is gone. This race condition is present
on both regular SSL and SslBump code paths, but sightings are rare.
This bug shares the underlying cause (and the solution) with bug 3505.
TODO: Adjust Comm::SetSelect() API to prevent future bugs like this.
Amos Jeffries [Sat, 5 May 2018 14:42:12 +0000 (14:42 +0000)]
Bug 4847 pt1: regression in proxy_auth ACL flags (#191)
r15058 "Support for --long-acl-options" in Squid-4.0.21
unintentionally removed the proxy_auth ACL support for -i/+i
flags. See bug report for details.
Fix proxy_auth ACL -i and +i flags no longer working by copying
RegexData flags registration, since ACLs for UserData all use
the same names and meanings.
Add documentation to indicate that ident and ext_user ACLs do
support -i/+i just like proxy_auth ACLs.
TODO: fix server_cert_fingerprint ACL which is still broken.
Amos Jeffries [Thu, 3 May 2018 15:52:04 +0000 (15:52 +0000)]
Bug 4852: regression in deny_info %R macro (#193)
SBuf::c_str() produces a temporary c-string which is not
guaranteed to survive, and does not survive as long as required
to print the deny_info URL. The HttpRequest::url path SBuf has
a much longer lifetime, so use a const reference to it instead.
Do not abuse argv[0] to supply roles and IDs to SMP kids (#176)
Use a newly added "--kid role-ID" command line option instead. Just like
argv[0], the new option is not meant for direct/human use.
This change allows exec(3)-wrapping tools like Valgrind to work with SMP
Squid: When launching kid processes, Valgrind does not pass Squid-formed
argv[0] to kid processes, breaking old kid role and ID detection code.
This change does not alter argv[0] of Squid processes. There is nothing
wrong with Squid-formed argv[0] values for Squid kids.
Also added a CommandLine class to support command line parsing without
code duplication. Squid needs to handle the new --kid option way before
the old mainParseOptions() handles the other options. The new class also
encapsulates argv manipulations, reducing main.cc pollution.
Avoid ssl/helper.cc "ssl_crtd" assertions on reconfiguration (#186)
Reconfiguration process consists of mainReconfigureStart() and
mainReconfigureFinish() steps separated by at least one main loop
iteration. Clearing a Squid global variable in mainReconfigureStart()
creates two problems for transactions that were started before
reconfiguration:
1. Transactions accessing that global _during_ reconfiguration loop
iteration(s) may be confused by the variable sudden disappearance.
2. Transactions accessing that global _after_ mainReconfigureFinish()
may be confused by the variable disappearance if reconfiguration
resulted in the global variable becoming nil.
To remove the first problem for ssl_crtd, external_acl, and redirecting
helpers, all of them are now reconfigured "instantly", during
mainReconfigureFinish().
To prevent crashes due to the second problem, Squid now generates helper
errors if the disappeared ssl_crtd or external_acl helpers are accessed
after reconfiguration. The admin is warned about such problems via
level-1 cache.log ERROR messages.
The second problem cannot be fully solved without storing (refcounted)
configuration globals inside each transaction that uses them. Such
serious changes are outside this small assertion-fixing project scope.
Reliable timestamp information is often critical for triage. We can use
the existing debugs() interface to add timestamps to FATAL messages. The
affected code already calls such risky functions as
storeDirWriteCleanLogs() so calling debugs() instead of printing
directly to files/syslog should not make things worse.
FATAL messages that were also logged to syslog (at LOG_ALERT level) are
still logged to syslog (at that same level, but now with the usual
Squid-generated prefix). Such syslog alerts can now be easily triggered
via a new ForceAlert() API.
Also treat segmentation faults, bus errors, and other signal-based
sudden deaths the same as most other FATAL errors -- log them to syslog.
Alex Rousskov [Thu, 12 Apr 2018 22:12:39 +0000 (22:12 +0000)]
Fixed Transient reader locking broken by 4310f8b (#161)
The closeForWriting() call comment is correct -- we must keep the lock,
but the optional argument was accidentally lost (when undoing the failed
attempt to remove all long-term transient locks in the feature branch).
Also polished stale comments (which led to the above bug discovery!).
Also polished addEntry() aspects that I have missed in 4310f8b.
The fixed leak was accompanied by these cache.log errors:
ERROR: worker I/O push queue for ... overflow: ...
I/O queue overflows during disk read requests log the same error but do
not leak memory. Repeated overflows during disk write requests could
eventually exhaust IPC shared memory:
ERROR: ... exception: run out of shared memory pages for IPC I/O
With IPC memory exhausted due to leaks, rock disk I/O stops forever.
Amos Jeffries [Thu, 1 Mar 2018 23:48:28 +0000 (12:48 +1300)]
Use va_copy() on all platforms; fixed a dangerous low-level bug (#160)
To improve cross-compilation support and to simplify code, rely on C++11
cstdarg header instead of ./configure-time va_copy() detection.
Using ./configure-time detection for va_copy() is dangerous because when
it does not work (e.g., during a poorly configured cross-compilation
attempt), Squid may crash if va_copy() was needed but was not detected.
See also: Bug 4821 and bug 753.
Also found and fixed a low-level bug: StoreEntry::vappendf() was not
using va_copy() because store.cc lacked VA_COPY #defines. The affected
code (900+ callers!) is used for cache manager responses and Gopher
gateway response compilation. If any of those calls required a buffer
larger than 4KB, the lack of those va_copy() calls could lead to crashes
and/or data corruption issues on platforms where va_copy() is required.
Alexander Gozman [Fri, 16 Feb 2018 10:52:58 +0000 (13:52 +0300)]
Fix clientside_mark and client port logging in TPROXY mode (#150)
The clientside_mark ACL was not working with TPROXY because a
conntrack query could not find connmark without a true client port.
Ip::Intercept::Lookup() must return true client address, but its
TproxyTransparent() component was reseting the client port. We should
use zero port when we compute the source address for the Squid-to-peer
connection instead.
squidadm [Fri, 16 Feb 2018 22:04:09 +0000 (11:04 +1300)]
TLS: GnuTLS implementation for listening ports and client connections (#81) (#152)
Move the http_port cert= and key= options logic to libsecurity and add GnuTLS implementation for PEM file loading. Also adds some extra debugging to clarify listening port initialization problems with the PEM files.
Enable most of the http(s)_port listening socket logic to always build except where OpenSSL-specific dependency still exists. It may seem reasonable to leave it optionally excluded for minimal builds, however a minimal proxy that does not support HTTPS in any way is increasingly useless in the modern web so preference is given to building the generic TLS related code. This also simplifies the required testing to detect code portability issues.
GnuTLS implementation is added for https_port configured with static cert=/key= parameters and the resulting TLS handshake behaviour. Squid built with GnuTLS can now act as useful parent proxies behind a SSL-Bump'ing frontend or for other clients which require a TLS explicit proxy.
Also fixes the definitions for the CertPointer and PrivateKeyPointer.
Bug 4505: SMP caches sometimes do not purge entries (#46)
When Squid finds a requested entry in the memory cache, it does not
check whether the same entry is also stored in a cache_dir. The
StoreEntry object may become associated with its store entry in the
memory cache but not with its store entry on disk. This inconsistency
causes two known problems:
1. Squid may needlessly swap out the memory hit to disk, either
overwriting an existing (and identical) disk entry or, worse,
creating a duplicate entry on another disk. In the second case, the
two disk entries are not synchronized and may eventually start to
differ if one of them is removed or updated.
2. Squid may not delete a stale disk entry when needed, violating
various HTTP MUSTs, and eventually serving stale [disk] cache entries
to clients.
Another purging problem is not caused by the above inconsistency:
3. A DELETE request or equivalent may come for the entry which is still
locked for writing. Squid fails to get a lock for such an entry (in
order to purge it) and the entry remains in disk and/or memory cache.
To solve the first two problems:
* StoreEntry::mayStartSwapout() now avoids needless swapouts by checking
whether StoreEntry was fully loaded, is being loaded, or could have
been loaded from disk. To be able to reject swapouts in the last case,
we now require that the newer (disk) entries explicitly delete their
older variants instead of relying on the Store to overwrite the older
(unlocked) variant. That explicit delete should already be happening
in higher-level code (that knows which entry is newer and must mark
any stale entries for deletion anyway).
To fix problem #3:
* A new Store::Controller::evictIfFound(key) method purges (or marks for
deletion if purging is impossible) all the matching store entries,
without loading the StoreEntry information from stores. Avoiding
StoreEntry creation reduces waste of resources (the StoreEntry object
would have to be deleted anyway) _and_ allows us to mark being-created
entries (that are locked for writing and, hence, cannot be loaded into
a StoreEntry object).
XXX: SMP cache purges may continue to malfunction when the Transients
table is missing. Currently, Transients are created only when the
collapsed_forwarding is on. After Squid bug 4579 is fixed, every public
StoreEntry will have the corresponding Transients entry and vice versa,
extending these fixes to all SMP environments.
Note that even if Squid properly avoids storing duplicate disk entries,
some cache_dir manipulations by humans and Squid crashes may lead to
such duplicates being present. This patch leaves dealing with potential
duplicates out of scope except it guarantees that if an entry is
deleted, then all [possible] duplicates are deleted as well.
Fixing the above problems required (and/or benefited from) many related
improvements, including some Store API changes. It is impractical to
detail each change here, but several are highlighted below.
To propagate DELETEs across workers, every public StoreEntry now has a
Transients entry.
Prevented concurrent cache readers from aborting when their entry is
release()d. Unlike abort, release should not affect current readers.
Fixed store.log code to avoid "Bug: Missing MemObject::storeId value".
Removed Transients extras used to initialize MemObject StoreID/method in
StoreEntry objects created by Transients::get() for collapsed requests.
Controlled::get() and related Controller APIs do not _require_ setting
those MemObject details: get() methods for all cache stores return
StoreEntry objects without them (because entry basics lack Store ID and
request method). The caller is responsible for cache key collision
detection. Controlled::get() parameters could include Store ID and
request method for early cache key collision detection, but adding a
StoreQuery class and improving collision detection code is outside this
project scope (and requires many changes).
Found more cases where release() should not prevent sharing.
Remaining cases need further analysis as discussed in master 39fe14b2.
Greatly simplified UFS store rebuilding, possibly fixing subtle bug(s).
Clarified RELEASE_REQUEST flag meaning, becoming 'a private StoreEntry
which can't become public anymore'. Refactored the related code,
combining two related notions: 'a private entry' and 'an entry marked
for removal'.
Do not abort collapsed StoreEntries during syncing just because the
corresponding being stored shared entry was marked for deletion. Abort
them if the shared entry has been also aborted.
Added StoreEntry helper methods to prevent direct manipulation of
individual disk-related data members (swap_dirn, swap_filen, and
swap_status). These methods help keep these related data members in a
coherent state and minimize code duplication.
Conflicts:
src/MemObject.cc
Fix 889fc47 for SSL bumping with an authentication type other than the Basic (#104)
Commit 889fc47 was made to fix issue with Basic authentication and SSL bumping. But after this commit we can no longer properly use http_access with proxy_auth/proxy_auth_regex ACL because that type of ACL always return 1(match) regardless of the conditions in the rules.
Use the caches authentication results (if any) instead of a fixed 1(match) result.
Alex Rousskov [Tue, 23 Jan 2018 21:08:02 +0000 (14:08 -0700)]
Fixed store.cc "!mem_obj" assertion via peerDigestRequest (#134)
Broken by commit 76d61119 which (correctly) made createMeObject() assert
but missed one case where the old code should have been converted to
call the new ensureMemObject() instead.
peerDigestRequest() is called every 5 minutes, triggered by the
peerDigestCheck event. Most calls find the old digest entry that has the
same method and URIs.
Amos Jeffries [Sat, 20 Jan 2018 04:54:16 +0000 (17:54 +1300)]
Fixed Ip::Address copying (#126)
Explicit copy construction was slow and unnecessary.
Explicit copy assignment mishandled self copying and was unnecessary.
The remaining memcpy() calls mishandled self copying.
There are no known cases of Ip::Address self copying.
Amos Jeffries [Thu, 18 Jan 2018 20:54:50 +0000 (09:54 +1300)]
ESI: remove custom parser (#128)
Alex Rousskov:
let's consider removing the custom ESI parser from Squid. It is of
terrible quality and "nobody" is testing ESI code when things change. Is
the CVE risk worth supporting few platforms that do not have the right
parser libraries?
Andrey [Tue, 16 Jan 2018 23:28:59 +0000 (02:28 +0300)]
Added clientside_mark ACL for checking CONNMARK (#111)
Matches CONNMARK of accepted connections. Takes into account
clientside_mark and qos_flows mark changes (because Squid-set marks are
cached by Squid in conn->nfmark). Ignores 3rd-party marks set after
Squid has accepted the connection from a client (because Squid never
re-queries the connection to update/sync conn->nfmark).
Also added a debugs()-friendly API to print hex values.
Amos Jeffries [Mon, 15 Jan 2018 18:59:37 +0000 (07:59 +1300)]
Bug 3911: clang -fsanitize warnings (#125)
Fixes warnings from clang when -fsanitize is used. Many of these are also part of the bug 4738 issues.
error: private field 'callback' is not used [-Werror,-Wunused-private-field]
error: private field 'cbdata' is not used [-Werror,-Wunused-private-field]
error: private field 'IO' is not used [-Werror,-Wunused-private-field]
error: variable 'wccp2_router_id_element' is not needed and
will not be emitted [-Werror,-Wunneeded-internal-declaration]
We cannot set these warnings as default options yet because the STUB code intentionally does not use any private class members, so it would error on every unit test.
* Convert Store::LocalSearch to C++ initialization
* DiskThreadsDiskFile::IO is unused after setting by the constructor
Also, take the opportunity to redo the construct using C++11 initialization
* Remove currently unused wccp2_router_id_element
This resolves clang warnings until the WCCP redesign is completed.
Alex Rousskov [Fri, 19 Jan 2018 03:27:01 +0000 (03:27 +0000)]
Report exception locations and exception-related polish (#119)
Without location, many exceptions look identical: A growing number of
Must(entry != NULL) and Must(request) complicate triage. The location
info was already stored in TextException but was not reported.
Reporting exception location on a separate line makes admin-visible
FATAL/ERROR/WARNING messages easier to comprehend, and their primary
text becomes more "stable", which is good for documentation. Also, some
future exceptions will probably report multiple details, possibly even
context details collected as a low-level exception bubbles up to its
high-level handling/reporting code.
Also simplified/optimized TextException:
* TextException now reuses std::runtime_error message memory management
code, including its CoW optimizations/guarantees.
* Debug and TextException code now share the source location reporting
code (including Squid build prefix elision) in base/Here.{cc,h}.
Also simplified and polished SBuf-related exceptions, removing a few:
* Removed InvalidParamException as unused.
* Replaced SBufTooBigException with generic exceptions.
SBufTooBigException was misused (by SBuf::plength) and not useful. No
need to create a whole class just to parameterize an object!
* Replaced OutOfBoundsException with a generic exception.
OutOfBoundsException was not very useful (see SBufTooBigException). It
was used by one test case, that did not justify adding a whole class.
Also added SWALLOW_EXCEPTIONS() API to protect any code that may throw
unwanted exceptions. Reworked a few destructors after Must() changes
made it easier for GCC v6 to detect (and warn about) throwing code:
* Polished Ipc::Forwarder cleanup sequence. For Forwarders, I see no
reason to split/duplicate swanSong() functionality via a cleanup()
method. The swanSong() API exists so that job destructors do not need
to make confusing virtual method calls!
* Hid the AsyncJob destructor because all jobs should be "automatically"
deleted by the internal job code that guarantees a swanSong() call.
* Removed a bad (pair-less) StoreEntry::unregisterAbort() call from
Mgr::Forwarder destructor, possibly left behind in or around 51ea090.
* Removed ctor/dtor entrance debugging from the classes affected by the
"throwing destructor issue". AsyncJob covers that debugging need.
Dan Searle [Fri, 19 Jan 2018 01:37:16 +0000 (01:37 +0000)]
Bug 4631: security_file_certgen helper without disk cache (#95)
* disable the certificate DB disk cache if -s and -M command line options are omitted.
E.g. with this you can change squid.conf from:
sslcrtd_program security_file_certgen -s /var/lib/ssl_db -M 32MB
...to...
sslcrtd_program security_file_certgen
...and it will operate without the disk cache, generating certs fresh every time.
* Remove Ssl::CertificateDb::IsEnabledDiskStore()
Make the CertificateDb temporary objects dynamically allocated instead.
* Do command line checks in main() not the CertificateDb object.
This avoids a risky constructor exception and simplifies validity testing of parameters.
* Update man(8) documentation
The helper version is now 1.1. A minor version bump since it is being kept compatible with
installations using 1.0 properly but new feature available.
Also simplify the command line SYNOPSIS and incomplete mention of sslcrtd_* squid.conf directives.
This change started as a %<Hs fix described in the first bullet but
subsequent testing exposed the bug described in the second bullet,
fixing which resulted in other related fixes/improvements:
* Fixed %<Hs for received CONNECT errors: Correctly store the response
status code for %<Hs logging (instead of misplacing it in the %>Hs
location that was later overwritten with the correct to-client value).
* Fixed %<pt and %<tt for received CONNECT errors: Squid tunneling code
was missing message I/O timing maintenance required for those %codes.
* Probably fixed %<bs logging bug on forwarding retries: Squid did not
clear the (bodyBytesRead) counter between retries.
* Possibly improved %<Hs, %<pt, %<tt, %<bs logging on SslBump errors:
request->hier member did not copy all the sslServerBump->request->hier
details when generating an error response in
ConnStateData::serveDelayedError().
* Probably fixed a server.all.kbytes_out and server.other.kbytes_out
statistics bug: Squid missed written CONNECT request header bytes.
Also improved HierarchyLogEntry-related code: Reduced code duplication,
removed unnecessary destructor, and described the class. Removed
peer_http_request_sent timestamp because it ought to be equal to the
last request write time (now peer_last_write_).
TODO: Relay (expected) peer CONNECT error responses to users (instead of
hiding them behind a Squid-generated ERR_CONNECT_FAIL) and support %<h.
Amos Jeffries [Fri, 3 Nov 2017 05:38:40 +0000 (18:38 +1300)]
Bug 4679: User names not sent to url_rewrite_program (#78)
Add accessors to AccessLogEntry for retrieving IDENT and External ACL user
labels in a consistent way. Use these accessors for all log and logformat
outputs.
NP: does not hide/remove the original cache.* members due to direct use
remaining in some code locations that cannot yet be avoided.
Alex Rousskov [Thu, 28 Dec 2017 16:35:32 +0000 (09:35 -0700)]
Bug 2378: Duplicates in selected peer destinations (#112)
Duplicates in FwdServers lead to excessive peer connection retries, skew
in round-robin peer selection, and probably other problems.
This bug was fixed in 2008 but that v2 fix was never ported to v3. This
fix includes a bug 2408 fix for the original (bug 2378) fix, although I
adjusted bug 2408 logic to explicitly reject duplicate PINNED
destinations and to clarify why PINNED connection handling is "special".
I also centralized and improved peerAddFwdServer-related debugging,
removing duplicated and slightly inconsistent code.
squidadm [Thu, 4 Jan 2018 13:52:55 +0000 (02:52 +1300)]
Automatically revive hopeless kids on reconfigure and after a timeout. (#117)
Squid declares kids with "repeated, frequent failures" as hopeless.
Hopeless kids were not automatically restarted. In the absence of
automated recovery, admins were forced to restart the whole Squid
instance (after fixing the underlying problem that led to those kid
failures). Squid instance restarts hurt users.
In many cases, the underlying kid-killing problem is temporary, and
Squid can eventually fully recover without any admin involvement.
Squid now automatically restarts a hopeless kid after a configurable
downtime (a new hopeless_kid_revival_delay directive with a 60 minute
default value).
Also restart all hopeless kids upon receiving a reconfiguration signal.
Also avoid sending signals to non-running kids, fixing an old minor bug.
Squid FTP server dying because of an unhandled exception. (#102)
Related message in cache.log:
FATAL: Dying from an exception handling failure; exception: reply
Unfortunately, Squid does not report the exact place where the exception
was thrown, however the most possible reason is a "Must(reply)" failure inside
Ftp::Server::writeErrorReply.
Do not download remote certificate for issuer X if the received server
certificate (signed by X) can be validated using a locally available CA
certificate. According to our tests, a typical browser does not follow
'CA Issuers' references to download 'missing' certificates when the
browser can validate the origin server certificate (or its chain) using
a local CA certificate. Avoiding unnecessary validations and downloads
not only saves time, but can prevent validation failures as well!
Alex Rousskov [Fri, 17 Nov 2017 16:30:09 +0000 (09:30 -0700)]
Relay peer CONNECT error status line and headers to users (#80)
Automated agents and human users (or their support staff!) often benefit
from knowing what went wrong. Dropping such details is a bad default.
For example, automation may rely on receiving the original status code.
Our CVE-2015-5400 fix (74f35ca) was too aggressive -- it hid all peer
errors behind a generic 502 (Bad Gateway) response. Pass-through peer
authentication errors were later (971003b) exposed again, but our CVE
fix intent was _not_ to hide _any_ peer errors in the first place! The
intent was to close the connection after delivering the error response.
Hiding peer errors was an (unfortunate) implementation choice.
It could be argued that some peer errors should not be relayed, but
since Squid successfully relayed all peer errors prior to 74f35ca and
continues to relay all non-CONNECT peer errors today, discriminating
peer errors is a separate (and possibly unnecessary) feature.
Ideally, Squid should mangle and relay the whole error message (instead
of sending small original headers). Squid should also relay 1xx control
messages while waiting for the final response. Unfortunately, doing so
properly, without reopening CVE-2015-5400 or duplicating a lot of
complex code, is a huge project. This small change fixes the most acute
manifestation of the "hiding errors from users" problem. The rest is a
long-term TODO.
Bug 2821: Ignore Content-Range in non-206 responses (#77)
Squid used to honor Content-Range header in HTTP 200 OK (and possibly
other non-206) responses, truncating (and possibly enlarging) some
response bodies. RFC 7233 declares Content-Range meaningless for
standard HTTP status codes other than 206 and 416. Squid now relays
meaningless Content-Range as is, without using its value.
Why not just strip a meaningless Content-Range header? Squid does not
really know whether it is the status code or the header that is "wrong".
Let the client figure it out while the server remains responsible.
Also ignore Content-Range in 416 (Range Not Satisfiable) responses
because that header does not apply to the response body.
Also fixed body corruption of (unlikely) multipart 206 responses to
single-part Range requests. Valid multipart responses carry no
Content-Range (in the primary header), which confused Squid.
Amos Jeffries [Thu, 2 Nov 2017 08:14:54 +0000 (21:14 +1300)]
Move TLS/SSL http_port config values to libsecurity (#51)
These are most of the minor shuffling prerequisite for the proposal to allow generate-host-certificates to set a CA filename. These are required in libsecurity in order to prevent circular dependencies between libsecurity, libssl and libanyp.
Also contains some improvements to how configuration errors are displayed for these affected settings and some bugs fixed where the configured values were handled incorrectly.
Bug 4718: Support filling raw buffer space of shared SBufs (#64)
SBuf::forceSize() requires exclusive SBuf ownership but its precursor
SBuf::rawSpace() method does not guarantee exclusivity. The pair of
calls may result in SBuf::forceSize() throwing for no good reason.
New SBuf API provides a new pair of raw buffer appending calls that
reduces the number of false negatives.
This change may alleviate bug 4718 symptoms but does not address its
core problem (which is still unconfirmed).
This bug was probably caused by Bug 2833 feature/fix (1a210de).
The primary fix here is limited to clientReplyContext::processExpired():
Collapsed forwarding code must ensure StoreEntry::mem_obj existence. It
was missing for cache hits purged from (or never admitted into) the
memory cache. Most storeClientListAdd() callers either have similar code
or call storeCreateEntry() which also creates StoreEntry::mem_obj.
Also avoided clobbering known StoreEntry URIs/method in some cases. The
known effect of this change is fixed store.log URI and method fields
when a hit transaction did not match the stored entry exactly (e.g., a
HEAD hit for a GET cached entry), but this improvement may have even
more important consequences: The original method is used by possibly
still-running entry filling code (e.g., determining the end of the
incoming response, validating the entry length, finding vary markers,
etc.). Changing the method affects those actions, essentially corrupting
the entry state. The same argument may apply to store ID and log URI.
We even tried to make URIs/method constant, but that is impractical w/o
addressing an XXX in MemStore::get(), which is outside this issue scope.
To facilitate that future fix, the code now distinguishes these cases:
* createMemObject(void): Buggy callers that create a new memory object
but do not know what URIs/method the hosting StoreEntry was based on.
Once these callers are fixed, we can make the URIs/method constant.
* createMemObject(trio): Callers that create a new memory object with
URIs/method that match the hosting StoreEntry.
* ensureMemObject(trio): Callers that are not sure whether StoreEntry
has a memory object but have URIs/method to create one if needed.
Fix SSL certificate cache refresh and collision handling (#40)
SslBump was ignoring some origin server certificate changes or differences,
incorrectly using the previously cached fake certificate (mimicking
now-stale properties or properties of a slightly different certificate).
Also, Squid was not detecting key collisions inside certificate caches.
On-disk certificate cache fixes:
Use the original certificate signature instead of the certificate
subject as part of the key. Using signatures reduces certificate key
collisions to deliberate attacks and woefully misconfigured origins,
and makes any mishandled attacks a lot less dangerous because the
attacking origin server certificate cannot by trusted by a properly
configured Squid and cannot be used for encryption by an attacker.
We have considered using certificate digests instead of signatures.
Digests would further reduce the attack surface to copies of public
certificates (as if the origin server was woefully misconfigured).
However, unlike the origin-supplied signatures, digests require
(expensive) computation in Squid, and implemented collision handling
should make any signature-based attacks unappealing. Signatures won
on performance grounds.
Other key components remain the same: NotValidAfter, NotValidBefore,
forced common name, non-default signing algorithm, and signing hash.
Store the original server certificate in the cache (together with
the generated certificate) for reliable key collision detection.
Upon detecting key collisions, ignore and replace the existing cache
entry with a freshly computed one. This change is required to
prevent an attacker from tricking Squid into hitting a cached
impersonating certificate when talking to a legitimate origin.
In-memory SSL context cache fixes:
Use the original server certificate (in ASN.1 form) as a part of the
cache key, to completely eliminate cache key collisions.
Other related improvements:
Make the LruMap keys template parameters.
Polish Ssl::CertificateDb class member names to match Squid coding
style. Rename some functions parameters to better match their
meaning.
Replace Ssl::CertificateProperties::dbKey() with:
Ssl::OnDiskCertificateDbKey() in ssl/gadgets.cc for
on-disk key generation by the ssl_crtd helper;
Ssl::InRamCertificateDbKey() in ssl/support.cc for
in-memory binary keys generation by the SSL context memory cache.
Optimization: Added Ssl::BIO_new_SBuf(SBuf*) for OpenSSL to write
directly into SBuf objects.
Alex Rousskov [Tue, 22 Aug 2017 01:09:23 +0000 (19:09 -0600)]
Do not die silently when dying early. (#43)
Report (to stderr) various problems (e.g., unhandled exceptions) that
may occur very early in Squid lifetime, before stderr-logging is forced
by SquidMain() and way before proper logging is configured by the first
_db_init() call.
To enable such early reporting, we started with a trivial change:
-FILE *debug_log = NULL;
+FILE *debug_log = stderr;
... but realized that debug_log may not be assigned early enough! The
resulting (larger) changes ensure that we can log (to stderr if
necessary) as soon as stderr itself is initialized. They also polish
related logging code, including localization of stderr checks and
elimination of double-closure during log rotation on Windows.
These reporting changes do not bypass or eliminate any failures.
Alex Rousskov [Sun, 6 Aug 2017 00:20:40 +0000 (18:20 -0600)]
Fixed, changed addresses in README. Made README look better on Github.
Why not add README.md? Not enough reasons to warrant info duplication:
Markdown is not particularly helpful for rendering a trivial list of
references, and Github already renders HTTP links appropriately.
Why not move README to README.md? Many tools and console humans still
look for README rather than README.md.
Why not use Markdown in README? Github does not render such markup.
TODO: Consider removing detailed distribution terms at the bottom
because "everybody" knows what GPLv2 basically means, and we already
tell the reader where to find the exact licensing terms.
Garri Djavadyan [Tue, 1 Aug 2017 00:03:18 +0000 (18:03 -0600)]
Bug 4648: Squid ignores object revalidation for HTTPS scheme
Squid skips object revalidation for HTTPS scheme and, hence, does not
honor a reload_into_ims option (among other settings).
TODO: Add an httpLike() method or function to detect all HTTP-like
schemes instead of comparing with AnyP::PROTO_HTTP directly. There are
20+ candidates for similar bugs: git grep '[!=]= AnyP::PROTO_HTTP'.
Bug 1961 extra: Convert the URL::parse method API to take const URI strings
The input buffer is no longer truncated when overly long. All callers have
been checked that they handle the bool false return value in ways that do
not rely on that truncation.
Callers that were making non-const copies of buffers specifically for the
parsing stage are altered not to do so. This allows a few data copies and
allocations to be removed entirely, or delayed to remove from error handling
paths.
While checking all the callers of Http::FromUrl several places were found to
be using the "raw" URL string before the parsing and validation was done. The
simplest in src/mime.cc is already applied to v5 r15234. A more complicated
redesign in src/store_digest.cc is included here for review. One other marked
with an "XXX: polluting ..." note.
Also, added several TODO's to mark code where class URL needs to be used when
the parser is a bit more efficient.
Also, removed a leftover definition of already removed urlParse() function.
projects / thirdparty / squid.git / log
