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'.
* replace bzr calls with git equivalent
* remove obsolete ROOT and PWD variables (git does not support non-recursive file listing)
* add exceptions to ignore more files caught by git than bzr
Alex Rousskov [Thu, 20 Jul 2017 03:45:59 +0000 (21:45 -0600)]
Made Ip::Address::fromHost() handle nil pointers after fd9c47d. (#25)
No functionality changes expected. Nobody was passing nil pointers to
this code before or after fd9c47d AFAICT, but now that this code is
exposed as a public method, it must handle nil pointers.
Detected by Coverity Scan. Issue 1415049 (FORWARD_NULL).
Alex Rousskov [Wed, 12 Jul 2017 05:04:41 +0000 (23:04 -0600)]
Happy Eyeballs: Deliver DNS resolution results to peer selection ASAP.
To make eyeballs happy, DNS code must deliver each lookup result to the
IP cache and, ultimately, to upper layers of ipcache_nbgethostbyname()
callers. This requires changing two interfaces:
1. between the DNS and the IP cache (the IDNSCB callback);
2. between the IP cache and peer selection code (the IPH callback).
The IDNSCB callback is now called after every usable A and AAAA lookup
instead of waiting for both answers. The IPH callback now has a sister
API for incremental delivery: The Dns::IpReceiver class.
To safely handle incremental delivery of IP addresses to the IP cache, I
upgraded ipcache_addrs from an equivalent of a C POD to a C++ CachedIps
container. The encapsulation allowed me to clearly separate the two IP
cache iteration APIs:
* All IPs (used by, e.g., ACL matching and host verification code) and
* just the "good" IPs (used only for peer selection for now).
CachedIps stores IPs together with their good/bad status in a single
std::vector. Eventually, the CachedIp element may be extended to store
TTL. The following implementation alternatives were considered and
rejected (at least for now) while optimizing for the "a few (and usually
just one), usually good IPs" case:
* Using std::list or std::deque storage would consume more RAM[1] for
the common case of one (or few) IPs per name and slowed down IPs
iteration code.
[1] http://info.prelert.com/blog/stl-container-memory-usage
* Separating IP from its status, like the old code did, would make it
easier to mismatch IP and its status, make it harder to add more
metadata like per-IP TTL, and only save memory when storing many IPs
per name.
The drawback of the selected "all IP-related info in one place" approach
is that we need smart iterators (e.g., the added GoodIpsIterator) or a
visitor API.
I added a new interface class for the incremental notification about
newly found IP addresses (Dns::IpReceiver) instead of adding second
IPH-like function pointer because we cannot safely call cbdata-protected
functions multiple times for the same cbdata object -- only
cbdataReferenceValidDone() dereferences the opaque pointer properly, and
that function cannot be called repeatedly. CbcPointer solves that
problem (but requires a class). Class methods also allow for more
precise notifications, with fewer ifs in the recipient code.
The new IpCacheLookupForwarder class hides the differences between the
old C-style IPH callbacks and the new Dns::IpReceiver. Eventually, we
may be able to move all lookup-specific data/methods into
IpCacheLookupForwarder, away from the IP cache entries where that info
is useless at best.
mgr:ipcache no longer reports "IPcache Entries In Use" but that info is
now available as "cbdata ipcache_entry" row in mgr:mem.
Do not cache IPv6 /etc/hosts addresses when IPv6 support is disabled.
This change simplified code, made it more consistent (we did not cache
AAAA records), and fixed ipcacheCycleAddr() and ipcacheMarkAllGood()
that were clearing supposed-to-be-permanent "bad (IPv6 disabled)" marks.
Also fixed two DNS TTL bugs. Squid now uses minimum TTL among all used
DNS records[2]. Old ipcacheParse() was trying to do the same but:
* could overwrite a zero TTL with a positive value
* took into account TTLs from unused record types (e.g., CNAME).
[2] Subject to *_dns_ttl limits in squid.conf, as before.
Also fixed "delete xstrdup" (i.e., malloc()ed) pointer in bracketed IP
parsing code (now moved to Ip::Address::FromHost()).
Also prohibited duplicate addresses from entering the IP cache. Allowing
duplicates may be useful for various hacks, but the IP cache code
assumes that cached IPs are unique and fails to mark bad repeated IPs.
Also fixed sending Squid Announcements to unsupported/disabled IPv6
addresses discovered via /etc/hosts.
Also slightly optimized dstdomain when dealing with IP-based host names:
The code now skips unnecessary Ip::Address to ipcache_addrs conversion.
This simplification may also help remove the ipcacheCheckNumeric() hack.
The bracketed IP parsing code was moved to Ip::Address::fromHost(). It
still needs a lot of love.
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.
* Protect Squid Client classes from new requests that compete with
ongoing pinned connection use and
* resume dealing with new requests when those Client classes are done
using the pinned connection.
Replaced primary ConnStateData::pinConnection() calls with a pair of
pinBusyConnection() and notePinnedConnectionBecameIdle() calls,
depending on the pinned connection state ("busy" or "idle").
Removed pinConnection() parameters that were not longer used or could be computed from the remaining parameters.
Removed ConnStateData::httpsPeeked() code "hiding" the originating
request and connection peer details while entering the first "idle"
state. The old (trunk r11880.1.6) bump-server-first code used a pair of
NULLs because "Intercepted connections do not have requests at the
connection pinning stage", but that limitation no longer applicable
because Squid always fakes (when intercepting) or parses (a CONNECT)
request now, even during SslBump step1.
The added XXX and TODOs are not directly related to this fix. They
were added to document problems discovered while working on this fix.
In v3.5 code, the same problems manifest as Read.cc
"fd_table[conn->fd].halfClosedReader != NULL" assertions.
Amos Jeffries [Mon, 26 Jun 2017 02:14:42 +0000 (14:14 +1200)]
Bug 1961 partial: move urlParse() into class URL
* rename local variables in urlParse() to avoid symbol
clashes with class URL members and methods.
* move HttpRequest method assignment out to the single caller
which actually needed it. Others all passed in the method
which was already set on the HttpRequest object passed.
* removed now needless HttpRequest parameter of urlParse()
* rename urlParse as a class URL method
* make URL::parseFinish() private
* remove unnecessary CONNECT_PORT define
* add RFC documentation for 'CONNECT' URI handling
* fixed two XXX in URL-rewrite handling doing unnecessary
HttpRequest object creation and destruction cycles on
invalid URL-rewrite helper output.
Alex Rousskov [Mon, 26 Jun 2017 00:10:34 +0000 (18:10 -0600)]
Minimize direct comparisons with ACCESS_ALLOWED and ACCESS_DENIED.
No functionality changes expected.
Added allow_t API to avoid direct comparisons with ACCESS_ALLOWED and
ACCESS_DENIED. Developers using direct comparisons eventually mishandle
exceptional ACCESS_DUNNO and ACCESS_AUTH_REQUIRED cases where neither
"allow" nor "deny" rule matched. The new API cannot fully prevent such
bugs, but should either led the developer to the right choice (usually
.allowed()) or alert the reviewer about an unusual choice (i.e.,
denied()).
The vast majority of checks use allowed(), but we could not eliminate
the remaining denied() cases ("miss_access" and "cache" directives) for
backward compatibility reasons -- previously "working" deployments may
suddenly start blocking cache misses and/or stop caching:
http://lists.squid-cache.org/pipermail/squid-dev/2017-May/008576.html
Alex Rousskov [Sat, 24 Jun 2017 00:01:51 +0000 (18:01 -0600)]
Fixed mgr query handoff from the original recipient to Coordinator.
This bug has already been fixed once, in trunk r11164.1.61, but that fix
was accidentally undone shortly after, during significant cross-branch
merging activity combined with the Forwarder class split. The final
merge importing the associated code (trunk r11730) was buggy.
The bug (explained in r11164.1.61) leads to a race condition between
* Store notifying Server classes about the entry completion (which might
trigger a bogus error message sent to the cache manager client while
Coordinator sends its own valid response on the same connection!) and
* post-cleanup() connection closure handlers of Server classes silently
closing everything (and leaving Coordinator the only responding
process on that shared connection).
The bug probably was not noticed for so long because, evidently, the
latter actions tend to win in the current code.
Andreas Weigel [Sun, 18 Jun 2017 14:26:55 +0000 (02:26 +1200)]
Fix option --foreground to implement expected behavior
... and allow usage of SMP mode with service supervisors that do not work
well with daemons.
Currently, --foreground behavior is counter-intuitive in that the launched
process, while staying in the foreground, forks another "master" process,
which will create additional children (kids), depending on the number of
configured workers/diskers.
Furthermore, sending a SIGINT/SIGTERM signal to this foreground process
terminates it, but leaves all the children running.
The behavior got introduced with v4 rev.14561.
From discussion on squid-dev, the following behavior is implemented:
* -N: The initial process is a master and a worker process.
No kids.
No daemonimization.
* --foreground: The initial process is the master process.
One or more worker kids (depending on workers=N).
No daemonimization.
* neither: The initial process double-forks the master process.
One or more worker kids (depending on workers=N).
Daemonimization.
The Release Notes for v4 were updated to reflect the corrected behavior.
transaction_initiator ACL for detecting various unusual transactions
This ACL is essential in several use cases, including:
* After fetching a missing intermediate certificate, Squid uses the
regular cache (and regular caching rules) to store the response. Squid
deployments that do not want to cache regular traffic need to cache
fetched certificates and only them.
acl fetched_certificate transaction_initiator certificate-fetching
cache allow fetched_certificate
cache deny all
* Many traffic policies and tools assume the existence of an HTTP client
behind every transaction. Internal Squid requests violate that
assumption. Identifying internal requests protects external ACLs, log
analyzers, and other mechanisms from the transactions they mishandle.
The new transaction_initiator ACL classifies transactions based on their
initiator. Currently supported initiators are esi, certificate-fetching,
cache-digest, internal, client, and all. In the future, the same ACL
will be able to identify HTTP/2 push transactions using the "server"
initiator. See src/cf.data.pre for details.
Concurrent identical same-worker security_file_certgen (a.k.a. ssl_crtd)
requests are collapsed: The first such request goes through to one of
the helpers while others wait for that first request to complete,
successfully or otherwise. This optimization helps dealing with flash
crowds that suddenly send a large number of HTTPS requests to a small
group of origin servers.
Two certificate generation requests are considered identical if their
on-the-wire images are identical. This simple and fast approach covers
all certificate generation parameters, including all mimicked
certificate properties, and avoids hash collisions and poisoning.
Compared to collision- or poisoning-sensitive approaches that store raw
certificates and compare their signatures or fingerprints, storing
helper queries costs a few extra KB per pending helper request. That
extra RAM cost is worth the advantages and will be eliminated when
helper code switches from c-strings to SBufs.
ssl::server_name options to control matching logic.
Many popular servers use certificates with several "alternative subject
names" (SubjectAltName). Many of those names are wildcards. For example,
a www.youtube.com certificate currently includes *.google.com and 50+
other subject names, most of which are wildcards.
Often, admins want server_name to match any of the subject names. This
is useful to match any server belonging to a large conglomerate of
companies, all including some *.example.com name in their certificates.
The existing server_name functionality addresses this use case well.
The new ACL options address several other important use cases:
--consensus identifies transactions with a particular server when
server's subject name is also present in certificates used by many other
servers (e.g., matching transactions with a particular Google server but
not with all Youtube servers).
--client-requested allows both (a) SNI-based matching even after
Squid obtains the server certificate and (b) pinpointing a particular
server in a group of different servers all using the same wildcard
certificate (e.g., matching appengine.example.com but not
www.example.com when the certificate for has *.example.com subject).
--server-provided allows matching only after Squid obtains the server
certificate and matches any of the conglomerate parts.
Also this patch fixes squid to log client SNI when client-first bumping mode
is used too.
The old single-letter ACL "flags" code was refactored to support long option
names (with option-specific value types) without significant
per-ACL-object performance/RAM overheads and without creating a global
registry for all possible options. This refactoring (unexpectedly)
resulted in removal of a lot of unreliable static initialization code.
Refactoring fixed ACL flags parsing code that was dangerously misinterpreting
-i and +i flags in several contexts. For example, each of the three cases
below was misinterpreted as if three domains were configured (e.g., "+i",
"-z", and "example.com") on each line instead of one domain ("example.com"):
Alex Rousskov [Fri, 9 Jun 2017 04:38:40 +0000 (22:38 -0600)]
Happy Eyeballs: Use each fully resolved forwarding destination ASAP.
Do not wait for all request forwarding destinations to be resolved. Use
each resolved destination as soon as it is needed. This change does not
improve or affect IPv4/IPv6 handling (Squid still waits for both DNS A
and AAAA answers when resolving a single destination name), but the peer
selection code can now deliver each IP address to the FwdState/tunneling
code without waiting for all other destination names to be DNS-resolved.
TODO: Deliver A and AAAA answers to the peer selection code ASAP.
This change speeds up forwarding in peering environments where peers may
need frequent DNS resolutions but that was not the motivation here.
This change is a step towards a more complete Happy Eyeballs support in
Squid. The general path can be roughly summarized as follows:
1. Squid has already supported: Use parallel A and AAAA queries.
2. This change: ASAP delivery of IPs from peer selection to FwdState.
3. The next step: ASAP delivery of IPs from DNS to peer selection.
4. A separate project may add: Use parallel TCP connections.
Also fixed missing cbdataReferenceDone(psstate->callback_data) calls
in three error handling cases. These cases probably leaked memory.
projects / thirdparty / squid.git / log
