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.
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.
squidadm [Sat, 5 May 2018 11:58:34 +0000 (11:58 +0000)]
Revert incorrect changes in PR189 (#199)
Commit a85f0df5d9226a613c14219a26c53f9a9e6c3a5f was supposed
to be documentation only, but somehow included changes from
another PR branch. Remove so the other PR can be applied with
correct commit details.
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.
Alexander Gozman [Fri, 13 Apr 2018 02:28:57 +0000 (02:28 +0000)]
Reworked packet/connection marking (#170)
The handling of packet and connection marks was odd: The clientside_mark
ACL worked with connection marks, but the directive of the same name
supported only packet marks. Also, clients packet MARK (if set)
overwrote CONNMARK and, as a result, broke ACL checking.
To minimize confusion, connection and packet marks are now separated:
* renamed the clientside_mark ACL to client_connection_mark
* renamed the clientside_mark directive to mark_client_packet
* added a mark_client_connection directive
While the first two points just clarify things, the last one introduces
a new functionality: It allows to set or change clients CONNMARK.
Both clientside_mark ACL and directive are now deprecated.
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.
Support selective CF: collapsed_forwarding_access (#151)
The new directive controls whether individual requests (including
ICP/HTCP and revalidation requests) should participate in collapsed
forwarding. Admins want to limit collapsed forwarding because it carries
significant transaction-specific risks (and benefits!).
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.
Amos Jeffries [Mon, 12 Feb 2018 15:05:25 +0000 (04:05 +1300)]
Fix loading certificates after tls-cert= changes (#144)
* Remove self-signed CA check
This check is not needed when loading the initial cert portion of a PEM file
as it will be performed later when loading the chain and was causing
self-signed CA to be rejected incorrectly.
* Fix a typo in debugs output
* Always generate static context from tls-cert= parameter
... if a cert= is provided. SSL-Bump still (for now) requires a static context as fallback when generate fails.
* Revert tlsAttemptHandshake to Squid_SSL_Accept API
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.
Amos Jeffries [Thu, 1 Feb 2018 09:51:54 +0000 (22:51 +1300)]
TLS: GnuTLS implementation for listening ports and client connections (#81)
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.
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 [Wed, 10 Jan 2018 15:45:43 +0000 (08:45 -0700)]
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.
Amos Jeffries [Sun, 7 Jan 2018 14:31:16 +0000 (03:31 +1300)]
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.
Alex Rousskov [Tue, 2 Jan 2018 16:16:52 +0000 (09:16 -0700)]
Moved peer*(ps_state) functions into ps_state renamed PeerSelector (#113)
No functionality changes intended (other than debug message variations).
Also polished related documentation and debug messages.
Also converted "struct _icp_common_t" into an icp_common_t class, to
make its forward declarations simple. As a side effect, removed
__cplusplus ifdefs, addressing an old TODO.
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.
Amos Jeffries [Fri, 15 Dec 2017 02:50:53 +0000 (15:50 +1300)]
Convert Acl::InnerNode to C++11 for-each loop (#101)
This also fixes a bug in some STL implementations where passing for_each &ACL::prepareForUse
results in the ACL class nil-method explicitly running instead of the child ACL class
virtual method.
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.
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.
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 [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.
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.
Do not check 'configured_once' in mainInitialize(). (#74)
Since mainInitialize() is called only once, there is no need to do this
check. I assume the check became obsolete since trunk r212: before this
revision, mainInitialize() could be called twice from main() with a
similar 'first_time' variable protection.
Fixed reporting of validation errors for downloaded intermediate certs. (#72)
When Squid or its helper could not validate a downloaded intermediate
certificate (or the root certificate), Squid error page contained
'[Not available]' instead of the broken certificate details, and '-1'
instead of depth of the broken certificate in logs.
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.
r15240 broke ipcacheCheckNumeric() because that function's static
storage was no longer reset properly between calls.
This bug is very different from bug 4741, but their symptoms (e.g.,
false "Host header forgery" SECURITY ALERTs) can be the same.
I did not realize that std::vector::resize(n, x) ignores x when the
vector size is already at least n. It is not a reset()-like method. My
tests did not have enough different IP-based URLs to expose this bug.
Security::HandshakeParser::parseServerCertificates builds cert list with nils (#42)
... if squid does not compiled with OpenSSL support.
This patch fixes:
* HandshakeParser::ParseCertificate() to return a Security::Pointer
* HandshakeParser:: parseServerCertificates() to be a no-op if OpenSSL is
not used
* Fix compile error if squid compiled without openssl but with gnutls enabled
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.
Alex Rousskov [Wed, 2 Aug 2017 22:13:27 +0000 (16:13 -0600)]
Automatically revive hopeless kids on reconfigure and after a timeout.
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.
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'.