Alex Rousskov [Sun, 28 Jun 2015 14:08:31 +0000 (07:08 -0700)]
Do not blindly forward cache peer CONNECT responses.
Squid blindly forwards cache peer CONNECT responses to clients. This
may break things if the peer responds with something like HTTP 403
(Forbidden) and keeps the connection with Squid open:
- The client application issues a CONNECT request.
- Squid forwards this request to a cache peer.
- Cache peer correctly responds back with a "403 Forbidden".
- Squid does not parse cache peer response and
just forwards it as if it was a Squid response to the client.
- The TCP connections are not closed.
At this stage, Squid is unaware that the CONNECT request has failed. All
subsequent requests on the user agent TCP connection are treated as
tunnelled traffic. Squid is forwarding these requests to the peer on the
TCP connection previously used for the 403-ed CONNECT request, without
proper processing. The additional headers which should have been applied
by Squid to these requests are not applied, and the requests are being
forwarded to the cache peer even though the Squid configuration may
state that these requests must go directly to the origin server.
This fixes Squid to parse cache peer responses, and if an error response
found, respond with "502 Bad Gateway" to the client and close the
connections.
Amos Jeffries [Tue, 23 Jun 2015 09:21:06 +0000 (02:21 -0700)]
Use relative-URL in errorpage.css for SN.png
Modern browsers now seem to be accepting relative-URLs, and Squid
global_internal_static non-https:// URLs are working. So we can do this
now without as many failures.
Amos Jeffries [Mon, 22 Jun 2015 11:29:39 +0000 (04:29 -0700)]
Replace GNU atomics and related hacks with C++11 std::atomic
With C++11 atomic support by the stdlib is not optional. This
resolves issues determining whether GNU atomics are available,
operational 32-bit vs 64-bit, or cross-compiling (bug 4224).
Amos Jeffries [Sat, 20 Jun 2015 00:24:24 +0000 (17:24 -0700)]
Fix CONNECT failover to IPv4 after trying broken IPv6 servers
This makes CONNECT tunnel connection attempts obey forward_timeout
and continue retrying instead of aborting with a client error when one
possible server hits a connect_timeout.
Alex Rousskov [Fri, 19 Jun 2015 16:57:30 +0000 (10:57 -0600)]
Fixed segmentation fault when freeing https_port clientca on reconfigure
or exit.
AnyP::PortCfg::clientCA list was double-freed because the SSL context takes
ownership of the STACK_OF(X509_NAME) supplied via SSL_CTX_set_client_CA_list(),
but Squid was not aware of that. Squid now supplies a clone of clientCA.
Amos Jeffries [Fri, 19 Jun 2015 07:13:57 +0000 (00:13 -0700)]
Bug 4269: ignore-must-revalidate broken
ignore-must-revalidate appears to prevent revalidation by disabling
storage of objects with must-revalidate/proxy-revalidate header.
However it was also preventing revalidation of objects cached due to
ignore-private, or the presence of no-cache, s-maxage, and use of auth
credentials.
Remove the violation option entirely.
Also cleanup the documentation of ignore-auth which was removed earlier.
Amos Jeffries [Tue, 9 Jun 2015 06:14:43 +0000 (23:14 -0700)]
Bug 1961 partial: Move HttpRequest host:port to class URL
Moves the host:port authority details into class URL for more
modular URI management. Add URL::authority() member to generate
authority-form URIs from the class URL stored details.
Also, shuffle urlDefaultPort() to AnyP::UriScheme::defaultPort()
Amos Jeffries [Tue, 9 Jun 2015 01:54:56 +0000 (18:54 -0700)]
Parser-NG: Transfer-Encoding:chunked Parser
Remove several performance regressions incurred in earlier Parser-NG
updates by refactoring the class ChunkedCodingParser to a class
Http1::TeChunkedParser which parses an SBuf I/O buffer for chunked
encoding data and (for now) copies the chunk payloads into a MemBuf buffer.
The new class is inherited from Http1::Parser and presents the same API.
Chunk Trailers are now available via the Parser API mimeHeader() method
- although none of the rest of Squid makes use of that data yet. It
implements parsing using a ::Parser::Tokenizer for (nearly) compliant
protocol tokenization. With enumerated states instead of a dynamic
function-pointer chain.
Measurements:
Co-Advisor shows no compliance change.
Polygraph shows approx 1% speed improvement over trunk.
Wrong intialization value for clientReplyContext::headers_sz member
The clientReplyContext::headers_sz member after the trunk patch r14078
initialized to wrong value. In many cases inside clientReplyContext class
this is considered as initialized to "0" and setting to "-1" can cause
problems.
This bug can be caused by certificates does not contain a CN field. In this
case the Ssl::ErrorDetail::cn method may return NULL causing this assertion
somewhere inside Ssl::ErrorDetail::buildDetail method, which expects always
a non NULL value from Ssl::ErrorDetail::cn and similar methods.
This patch try to hardening the Ssl::ErrorDetail error formating functions to
avoid always check for NULL values and also avoid sending wrong information
for various certificate fields in the case of an error while extracting the
information from certificate..
This is a squid Assertion inside ConnStateData::getSslContextDone while setting timeout. The reason is that the ConnStateData::clientConnection connection may
closed while waiting response from ssl_crtd helper.
Amos Jeffries [Tue, 2 Jun 2015 23:27:26 +0000 (16:27 -0700)]
Bug 3875: bad mimeLoadIconFile error handling
Improve the MimeIcon reliability when filesystem I/O errors or others
cause the icon data to not be loadable.
The loading process is re-worked to guarantee that once the
MimeIon::created callback occurs it will result in a valid StoreEntry in
the cache representing the wanted icon.
* If the image can be loaded without any issues it will be placed in
the cache as a 200 response.
* If errors prevent the image being loaded or necessary parameters
(size and mtime) being known a 204 object will be placed into the cache.
NP: There is no clear agreement on 204 being 'the best' status for this
case. 500 Internal Error is also appropriate. I have use 204 since:
* the bug is not in the clients request (eliminating 400, 404, etc),
* a 500 would be revealing details about server internals unnecessarily
often and incur extra complexity creating the error page.
* 204 also avoids needing to send Content-Length, Cache-Control header
and body object (bandwidth saving over 500 status).
NP: This started with just correcting the errno usage, but other bugs
promptly started appearing once I got to seriously testing this load
process. So far it fixes:
* several assertions resulting from StoreEntry being left invalid in
cache limbo beween created hash entries and valid mem_obj data.
* repeated attempts on startup to load absent icons files which dont
exist in the filesystem.
* buffer overfow on misconfigured or corrupt mime.conf file entries
* incorrect debugs messages about file I/O errors
* large error pages delivered when icons not installed (when it does
not assert from the StoreEntry)
This patch allow user_cert and ca_cert ACLs to match arbitrary stand-alone OIDs
(not DN/C/O/CN/L/ST objects or their substrings). For example, should be able to
match certificates that have 1.3.6.1.4.1.1814.3.1.14 OID in the certificate
Subject or Issuer field. Squid configuration would look like this:
Amos Jeffries [Tue, 2 Jun 2015 11:05:22 +0000 (04:05 -0700)]
Convert StoreMap to std::atomic
Implemented copy-constructor and assignment operator for StoreMapSlice
since the default ones used by AtomicWord were not thread safe and
C++11 explicitly deletes them for std::atomic.
Bug3329: The server side pinned connection is not closed properly
in ConnStateData::clientPinnedConnectionClosed CommClose handler.
Squid enters a buggy state when an idle connection pinned to a peer closes:
- The ConnStateData::clientPinnedConnectionRead, the pinned peer
connection read handler, is called with the io.flag set to
Comm::ERR_CLOSING. The read handler does not close the peer
Comm::Connection object. This is correct and expected -- the I/O
handler must exit on ERR_CLOSING without doing anything.
- The ConnStateData::clientPinnedConnectionClosed close handler is called,
but it does not close the peer Comm::Connection object either. Again,
this is correct and expected -- the close handler is not the place to
close a being-closed connection.
- The corresponding fde object is marked as closed (fde::flags.open
is false), but the peer Comm::Connection object is still open
(Comm::Connection.fd >= 0)! From this point on, we have an inconsistency
between the peer Comm::Connection object state and the real world.
- When the ConnStateData::pinning::serverConnection object is later
destroyed (by refcounting), it will try to close its fd. If that fd
is already in use (e.g., by another Comm::Connection), bad things
happen (crashes, segfaults, etc). Otherwise (i.e., if that fd is
not open), comm_close may cry about BUG 3556 (or worse).
To fix this problem, we must not allow Comm::Connections to get out
of sync with fd_table, even when a descriptor is closed without going
through Connection::close(). There are two ways to accomplished that:
* Change Comm to always store Comm::Connections and similar high-level
objects instead of fdes. This is a huge change that has been long on
the TODO list (those "other high-level objects" is on of the primary
obstacles there because not everything with a FD is a Connection).
* Notify Comm::Connections about closure in their closing handlers
(this change). This design relies on every Comm::Connection having
a close handler that notifies it. It may take us some time to reach
that goal, but this change is the first step providing the necessary
API, a known bug fix, and a few preventive changes.
This change:
- Adds a new Comm::Connection::noteClosure() method to inform the
Comm::Connection object that somebody is closing its FD.
- Uses the new method inside ConnStateData::clientPinnedConnectionClosed
handler to inform the ConnStateData::pinning::serverConnection object
that its FD is being closed.
- Replaces comm_close calls which may cause bug #3329 in other places with
Comm::Connection->close() calls.
Initially based on Nathan Hoad research for bug 3329.
Amos Jeffries [Tue, 26 May 2015 17:25:04 +0000 (10:25 -0700)]
Replace Packer object API with Packable API
Majority of thost patch is symbol renaming to unify the
class method names to the Packable API names.
There is effectively no logical change in this patch
despite appearances because it replaces the Packer object
which provides methods which are just wrappers pointing
to static functions which are in turn wrappers pointing
to storage buffer object methods. With direct calls to
those storage object methods (renamed).
We can now interchangebly use MemBuf or StoreEntry objects
with the packInto(Packable *) functions. Or any other
object which inherits and implements the Packable API.
We also gain 0.1% in performance (+2 RPS) by avoiding the
layers of wrapper funcions and Packer object allocate /
deallocate cycles.
Amos Jeffries [Sat, 23 May 2015 05:10:00 +0000 (22:10 -0700)]
Cleanup: remove unnecessary if-conditions
krb5 credentials objects were being checked fro NULL before freeing in
several cases where they should not be. Use assert() instead to enforce
the expected behaviour.
The Adaptation::Icap::Xaction::swanSong may try to use an invalid
Icap::Xaction::cs object (Comm::ConnOpener object) if the Comm::ConnOpener
is already gone (because its job finished) but the Xaction::noteCommConnected
method is not called yet.
This patch makes the Adaptation::Icap::Xaction::cs object a CbcPointer instead
of a raw pointer and checks if the Xaction::cs object is still valid before
using it.
Amos Jeffries [Fri, 22 May 2015 15:43:19 +0000 (08:43 -0700)]
Portability: Define C++11 uniform distributions if missing
Older compilers standard libraries may not contain the uniform
distribution templates now used. But we may be able to use the TR1
distributions instead.
Amos Jeffries [Fri, 22 May 2015 13:59:49 +0000 (06:59 -0700)]
C++11: compiler support is now mandatory
We are now using too many C++11 features to go without proper support in
the compiler. Present ./configure time errors if support for -std=c++11
is missing.
Amos Jeffries [Fri, 22 May 2015 13:33:01 +0000 (06:33 -0700)]
Crypto-NG: update random number generators
C++11 brings with it a set of reasonable quality random number
generators and tools to retrieve values for various ranges and types.
Use those C++11 STL <random> features to replace the use of the
varyingly broken, weak or non-standard functions: rand(), random(),
lrand48(), and drand48().
In the process we gain much faster and higher 'quality' randomness in
the auth nonces and event queue scheduling. And more "even" spread for
the ACL random feature.
Amos Jeffries [Fri, 22 May 2015 09:42:55 +0000 (02:42 -0700)]
Replacement of sslversion=N by tls-min-version=1.N
Overall the default behaviour is changed from enumerating the protocols
wanted. To enumerating and eliminating the unwanted.
* sslversion= / version= parameter is removed from documentation.
* sslversion= code logics is converted from setting the SSL_*_method()
function to setting the ssloptions= masking parameters.
Yes this will open a hole for future libraries use of TLSv1.3. However
that is kind of desirable and if it becomes a problem the
ssloptions=NO_TLSv1_3 should be made available.
* The SSL_*_method() logic is all converted to using the flexible
TLS_*_Method() API when available (OpenSSL 1.1.0) otherwise the
equivalent SSLv23_*_method() API is used.
That API follows the latest specification behaviour: to send a protocol
frame type that any recipient should be able to parse (library decides
which), while only negotiating the protocol type permitted.
* A new option tls-min-version=1.N is added to server connection
directives. It controls *only* the TLS version range.
- http(s)_port directives are not (yet) implemented using
Security::PeerOptions. For now they are left with options= masking to
select protocol support.
- bug in http(s)_port directives version= parameter is fixed. The new
backward compatibility code accepts version=4|5|6 where the existing
code did not despite documentation saying it did.
- SSLv3 is left at the library default unless ssloptions=NO_SSLv3 is used.
* ssloptions= is left alone so anyone can still set the library options
masks to control SSLv3 enable/disable or specific TLS versions higher
than the configured minimum.
Fix "Not enough space to hold server hello message" error message
This patch merges the Ssl::ClientBio and Ssl::ServerBio read buffering code
to the Ssl::Bio::readAndBuffer method and uses the MemBuf::potentialSpaceSize
instead of MemBuf::spaceSize to check space size for SSL hello messages buffer,
to take in account available space after a possible buffer grow.
Amos Jeffries [Mon, 18 May 2015 12:50:03 +0000 (05:50 -0700)]
Prevent unused ssl_crtd helpers being run
The conditions for when to start ssl_crtd helpers was ignoring the
generate-host-certificates=off option. Meaning most ssl-bump installs
were running them needlessly.
Amos Jeffries [Fri, 15 May 2015 12:50:09 +0000 (05:50 -0700)]
Cleanup: Refactor IcmpConfig
Pull the IcmpConfig object out of the global SquidConfig structure and
updates it to processing its own parse logics. Bringing it inline with
the per-component configuration design in SourceLayout and HotConf projects.
This allows us to use SBuf for storing the pinger program details and
avoid valgrind complaints about some malloc.
It will also allow lazy re-starting of the helper to be implemented later.
Alex Dowad [Thu, 14 May 2015 10:24:29 +0000 (03:24 -0700)]
Fix incorrect use of errno in various libcomm.la places
Fix problems with 'errno' in TcpAcceptor::Listen, Comm::HandleRead, and
Comm::HandleWrite. 'errno' is only valid after a standard library function
returns an error. Also, we must avoid calling out to other functions before
recording the value of 'errno', since they might overwrite it.
Alex Dowad [Mon, 11 May 2015 15:31:57 +0000 (08:31 -0700)]
Fix signal.h usage to resolve compiler warning
When included, musl libc's sys/signal.h issues a compiler warning
stating that signal.h should be used directly instead. If gcc is
treating all warnings as errors, this breaks the build.
glibc's sys/signal.h does not contain any definitions; all it does
is include signal.h (indirectly). So directly including signal.h
doesn't break anything with glibc.
Nathan Hoad [Fri, 8 May 2015 19:28:16 +0000 (12:28 -0700)]
Fix missing external ACL helper notes
external ACL helper notes are only added onto the HTTP request that
kicked off the external ACL lookup, and not cached ACL responses.
This means if you set notes from an external ACL that are used for
some processing in other ACLs, or post-processing on logs, things
may be missed.
The comm_connect_addr on connect failures sets the xerrno to 0
and returns Comm::OK. This is causes problems on ConnOpener
class users which believes the connection is established and
it is ready for use.
Amos Jeffries [Fri, 8 May 2015 08:11:53 +0000 (01:11 -0700)]
Docs: shuffle SMP specific options to the top of squid.conf
The workers directive is required to be used before several other
directives. It makes little sense to documents it after the controls
which depend on it.
Make a new config section to contain the SMP specific options.
Amos Jeffries [Fri, 8 May 2015 07:13:35 +0000 (00:13 -0700)]
CacheMgr: display 'client_db off' instead of 0 clients accessing cache
... to clarify why there is no record of even the mgr request happening.
The cleint_db mechanism needs to be enabled and measuring traffci for
any useful client counter value to exist.
Inside IdleConnList::findUseable the IdleConnList::removeAt call can delete
"this" IdleConnList object. The IdleConnList::clearHandlers called imediatelly
after the removeAt method, will try to use the invalid "this" object in
a comm_read_cancel function call, causing this assertion or other similar.
This patch fixes the IdleConnList::findUseable, IdleConnList::pop and
IdleConnList::findAndClose methods to call IdleConnList::clearHandlers before
the IdleConnList::removeAt is called.
The maximum buffer size for holding Server and Client SSL hello messages is only
16k which is not enough hold a Hello message which includes some extensions and
1-2 or more Certificates.
This patch increases the maximum size to 65535 and also adds some checks to
avoid squid crashes in the case the hello messages buffer overflows.
This patch adds support for ICAP services that require SSL/TLS transport
connections. The same options used for the cache_peer directive are used for
the icap_service directive, with similar certificate validation logic.
To mark an ICAP service as "secure", use an "icaps://" service URI scheme when
listing your service via an icap_service directive. The industry is using a
"Secure ICAP" term, and Squid follows that convention, but "icaps" seems more
appropriate for a _scheme_ name.
Squid uses port 11344 for Secure ICAP by default, following another popular
proxy convention. The old 1344 default for plain ICAP ports has not changed.
Technical Details
==================
This patch:
- Splits Ssl::PeerConnector class into Ssl::PeerConnector parent and two kids:
Ssl::BlindPeerConnector, a basic SSL connector for cache_peers, and
Ssl::PeekingPeerConnector, a peek-and-splice SSL connector for HTTP servers.
- Adds a third Ssl::IcapPeerConnector kid to connect to Secure ICAP servers.
- Fixes ErrorState class to avoid crashes on nil ErrorState::request member.
(Ssl::IcapPeerConnector may generate an ErrorState with a nil request).
- Modifies the ACL peername to use the Secure ICAP server name as value while
connecting to an ICAP server. This is useful to make SSL certificate
policies based on ICAP server name. However, this change is undocumented
until we decide whether a dedicated ACL would be better.
This patch changes pconn_lifetime (r13780) to abort only really idle
persistent connections (when they timeout). It removes some "extra" features
(added to pconn_lifetime during the feature review) because they break things
when aggressive timeouts is combined with picky clients. Specifically,
1. Squid closed connections with partially received requests when they
reached pconn_lifetime limit. We should only close _idle_ connections.
2. When connecting to Squid without sending anything for longer than
pconn_lifetime, the connection hangs if the request is sent after the
waiting period.
3. The connection also hangs if the initial request is starting to be
transmitted but then there is a longer pause before the request is
completed.
Most of the above problems are easy to trigger only when using very aggressive
pconn_lifetime settings that the feature was not designed for, but they still
can be considered bugs from admins point of view. Fixes:
* Do not stop reading a partially received request when we are timing out,
to avoid aborting that request.
* Do not set keepalive flag based on the pconn_lifetime timeout. We cannot
predict whether some new request data is going to be read (and reset the
idle timeout clock) before our Connection:close response is sent back.
HTTP clients are supposed to recover from such races, but some apparently
do not, especially if it is their first request on the connection.