Alex Rousskov [Thu, 8 Mar 2012 01:50:04 +0000 (18:50 -0700)]
Do not assert if we fail to compose ssl_crtd request. Do blocking generation.
Users report assertions when OpenSSL fails to write a true server certificate
to to memory. Since that certificate is received from a 3rd party, we should
not assert that it is writeable. Besides, OpenSSL may have limitations/bugs
even if dealing with valid certificates.
If we fail to componse a request, we now try the good old blocking in-process
certificate generation.
Currently, it is not known what exactly causes OpenSSL to fail as we are
unable to trigger the assertion in a controlled test.
Bug fix: ssl_crtd crashes when accessing HTTPS sites with a domain name exceeding 64 characters
Squid tries to generate a certificate for long domain names, which is not
possible.
According to RFC 5280 (Section A.1), the common name length in a certificate
can be at most 64 characters. Therefore it is not possible to generate a valid
certificate with the above domain name as common name.
This patch does not allow use of common names longer than 64 bytes in
setCommonName adaptation algorithm. Also In the case the openssl fails to
read subject name from mimicking certificate does not set any subject to
generated certification. (currently ssl_crtd crashes).
Simplify Ssl::ServerPeeker and renamed it to Ssl::ServerBump
Currently, Ssl::ServerPeeker is a job that does virtually nothing and leaks on
SSL connection establishment errors. This patch convert this class convert this
class into a non-job class which ConnStateData use to maintain SSL
certificate peeking state.
The Ssl::ServerPeeker class renamed to Ssl::ServerBump.
ConnStateData::bumpErrorEntry and ConnStateData::bumpSslErrorNoList and
ConnStateData::bumpServerCert now are members to the this class.
Bug fix: The '%I' formating code in error page shows '[unknown]' in the case of SQUID_X509_V_ERR_DOMAIN_MISMATCH error
Update the server ip address and server hostname in HttpRequest::hier object
in the case of SQUID_X509_V_ERR_DOMAIN_MISMATCH error to be used for %I field
of the generated error page
Bug fix: Current serial number generation code does not produce a stable serial number for self-signed certificates
Squid always send the signing certificate to the ssl_crtd daemon even for
self-signed certificates because the signing certificate may used for cert
adaptation algorithms. The ssl_crtd currently ignore the signing certificate
in the case of self-signed certificates. This is has as result to
use a random number as serial number of generated certificate.
This patch also use 0 as serial number of the temporary intermediate certificate
used to generate the final serial number of the certificate, in the case of
signing certificate is not given.
Alex Rousskov [Wed, 29 Feb 2012 06:32:14 +0000 (23:32 -0700)]
Better helper-to-Squid buffer size management.
The minimum buffer size is reduced from 8KB to 4KB after a squid-dev
discussion to prevent wasting of "several hundred KB of unused permanent
memory on some installations".
We now increase the buffer if we cannot parse the helper response message.
The maximum buffer size is now 32KB. This should be enough for all known
helper responses.
We now warn if the read buffer reaches its capacity and kill the offending
helper explicitly. An increase in maximum buffer capacity to 32KB should make
such events rare.
Motivation: ssl_crtd helper may produce responses exceeding 9907 bytes in size
(and possibly much larger if multiple chained certificates need to be returned
to Squid). The old helper.cc code would fill the read buffer completely,
schedule a read for zero bytes, receive zero bytes, declare an EOF condition,
and close the stream (which kills ssl_crtd). Due to insufficient information
logged, the observable symptoms were pretty much the same as if ssl_crtd
closed the stream first, indicating a ssl_crtd bug.
The "trimMemory for unswappable objects" fix (trunk r11969,r11970) exposed
ENTRY_SPECIAL objects such as icons to memory trimming which they cannot
handle because Squid does not reload the missing parts of special objects.
Further testing showed that a special object may be cached in shared RAM
and/or might even be purged from all caches completely. This may explain why
icons eventually "disappear" in some environments.
We now treat special objects as not belonging to memory or disk Stores and do
not ask those Stores to manage them. This separation needs more work, but it
passes basic tests, and it is the right step towards creating a proper storage
dedicated to those objects.
Alex Rousskov [Tue, 28 Feb 2012 23:45:23 +0000 (16:45 -0700)]
Better helper-to-Squid buffer size management.
The minimum buffer size is reduced from 8KB to 4KB after a squid-dev
discussion to prevent wasting of "several hundred KB of unused permanent
memory on some installations".
We now increase the buffer if we cannot parse the helper response message.
The maximum buffer size is now 32KB. This should be enough for all known
helper responses.
We now warn if the read buffer reaches its capacity and kill the offending
helper explicitly. An increase in maximum buffer capacity to 32KB should make
such events rare.
Motivation: ssl_crtd helper may produce responses exceeding 9907 bytes in size
(and possibly much larger if multiple chained certificates need to be returned
to Squid). The old helper.cc code would fill the read buffer completely,
schedule a read for zero bytes, receive zero bytes, declare an EOF condition,
and close the stream (which kills ssl_crtd). Due to insufficient information
logged, the observable symptoms were pretty much the same as if ssl_crtd
closed the stream first, indicating a ssl_crtd bug.
HONDA Hirofumi [Tue, 28 Feb 2012 17:52:21 +0000 (10:52 -0700)]
Bug 3502: client timeout uses server-side read_timeout, not request_timeout
I have also adjusted request_timeout description in squid.conf to clarify that
request_timeout applies to receiving complete HTTP request headers and not
just the first header byte. We reset the connection timeout to
clientLifetimeTimeout after parsing request headers.
https_port was correctly using Config.Timeout.request already.
Alex Rousskov [Tue, 28 Feb 2012 00:30:47 +0000 (17:30 -0700)]
Bug 3497: Bad ssl_crtd db size file causes infinite loop.
The db size file may become empty when Squid runs out of disk space. Ignoring
db size reading errors led to bogus db sizes used as looping condition. This
fix honors reading errors and also terminates the loop when no more
certificates can be removed. Both errors and removal failure are fatal to
ssl_crtd.
A positive side-effect of this fix is one less call to the relatively
expensive file-reading size()/readSize() methods under normal conditions.
I also removed "minimum db size" check because it did not seem to be in sync
with other ssl_crtd parameters such as fs block size and because its overall
purpose was unclear. The check was also removed by the original bug reporter.
TODO: Remaining problems include: ssl_crtd should not exit just because it
cannot write something to disk. A proper reporting/debugging API is missing.
Guy Helmer [Tue, 28 Feb 2012 00:22:38 +0000 (17:22 -0700)]
Bug 3497: Bad ssl_crtd db size file causes infinite loop.
The db size file may become empty when Squid runs out of disk space. Ignoring
db size reading errors led to bogus db sizes used as looping condition. This
fix honors reading errors and also terminates the loop when no more
certificates can be removed. Both errors and removal failure are fatal to
ssl_crtd.
A positive side-effect of this fix is one less call to the relatively
expensive file-reading size()/readSize() methods under normal conditions.
I also removed "minimum db size" check because it did not seem to be in sync
with other ssl_crtd parameters such as fs block size and because its overall
purpose was unclear. The check was also removed by the original bug reporter.
TODO: Remaining problems include: ssl_crtd should not exit just because it
cannot write something to disk. A proper reporting/debugging API is missing.
When there is an error and we know the intended server name from CONNECT
request, we should use that name for the CN in the fake certificate instead
of mimicking the received true server certificate CN.
Alex Rousskov [Sat, 25 Feb 2012 16:44:36 +0000 (09:44 -0700)]
Mark requests on re-pinned connections to avoid them being pconnPush()ed
causing "fd_table[conn->fd].halfClosedReader != NULL" comm assertions later.
Forward.cc comments imply that request->flags.pinned is set by ConnStateData
but that is a lie. The flag is set by forward.cc itself. It was set for PINNED
peers having a valid pinned connection only. When we retry a pinned pconn
race, we still have a PINNED peer but the failed connection prevents us from
setting the flag. If we successfuly re-pin later, we must set the flag.
request->flags.pinned essentially means "the connection is or should be
pinned".
Alex Rousskov [Fri, 24 Feb 2012 22:29:40 +0000 (15:29 -0700)]
Ssl::ServerPeeker must become a store_client to prevent [error] entry trimming
We do not need a store client during the certificate peeking stage because we
do not send the error to the client and only accumulate what is being written
to the store. However, if there is no store client then Store will trim the
entry and we will hit a
setCommonName sets CN, but that is not enough when the fake certificate containsalternative names (which Squid mimics): The browser does not accept the
resulting fake certificate because it does not contains at least one alternativename that matches the domain name.
This patch stop mimicking of "Certificate Subject Alt Name" extensions when the
setCommonName adaptation algorithm used
If a certificate verification error is honored by Squid (i.e., a Squid error
page is returned to the user), the user gets a browser warning and then, after
ignoring the warning, the squid error message (about the same kind of problem). This happens because the fake certificate mimics details of the true server
certificate, which is "broken".
When Squid serves its own error page it is no longer trying to hide its presencefrom the user. Thus, it is probably better to serve all Squid-generated SSL
handshake and validation error responses with a trusted certificate containing
minimal details, like we already do for the DNS_FAIL errors.
This patch fixing the above problem.
NOTE: The fix will not work for domain mismatch errors. We are checking
for domain mismatch after we have establish the SSL connection with the web
client , and after we got the http request, because we need the HTTP request
Host header to check for SSL domain mismatch errors.
- Handle the case the signing certificate changed. The ssl_crtd daemon
must drop cached certificates which has signed with older signing
certificates and generate new one using the current signing certificate
- We need to generate certificates with different serial numbers, if the
signing certificate has changes in any way, even if the certificates has
exactly the same fields.
To achieve this we are firstly generating a temporary fake certificate with
serial number the hash digest of signing certificates public key. The digest
of the temporary fake certificate used as serial key to the final certificate
- Bug fix: A cached certificate which has adaptated with one or more algorithms
(setNotAfter, setNotBefore, setCommonName etc) did not used and always a new
certificate generated. This patch fixes this bug.
Notes:
- Ssl::ssl_match_certificates replaced with Ssl::certificateMatchesProperties
function which checks if a given certificate matches given properties:
Checks if the certificate signed with current signing certificate, and
check if mimicked certificate matches the given certificate.
- The Ssl::CertificateDb::purgeCert method added to delete a certificate from
database.
Two different certificates of the same fake Issuer must have the same serial
number. Otherwise, Firefox and possibly others will display a
sec_error_reused_issuer_and_serial error. Similarly, the same two certificates
should have the same serial number, even if generated on different
non-communicating (but identically configured) Squid boxes.
To produce unique serial numbers a temporary fake certificate with serial number
zero created, and its fingerprint used as the serial number of the final fake
certificate.
The old Ssl::CertificateDb code which was responsible to produce a serial number
for generated certificates removed.
Bug fix: sslpassword_program for ssl-bump http ports
Currently the sslpassword_program configuration parameter does not work
for encrypted certificate keys on ssl-bump enabled http ports, and user
always asked to give the SSL key password.
Alex Rousskov [Mon, 20 Feb 2012 21:05:28 +0000 (14:05 -0700)]
Use broken instead of a peer certificate info for error detail formatting.
This change has no visible effect when the peer certificate is a broken one.
Eventually, we may have to add more formatting codes to give admin access to
both peer and broken certificate details, but it may be difficult to use such
two sets of formatting codes on one static error page that does not "know"
whether the peer certificate is broken.
Alex Rousskov [Mon, 20 Feb 2012 19:32:58 +0000 (12:32 -0700)]
Retry requests that failed due to a persistent connection race
instead of replying with ERR_ZERO_SIZE_OBJECT "Bad Gateway".
Trunk r12050 code contains a portion of these changes. The rest is code
to re-pin a bump-server-first connection if a bumped pinned pconn fails
due to a race. We use a new canRePin flag to mark connections that can
be repinned and assume that pinned connections unrelated to SslBump
cannot be repinned.
Alex Rousskov [Mon, 20 Feb 2012 19:10:54 +0000 (12:10 -0700)]
Retry requests that failed due to a persistent connection race
instead of replying with ERR_ZERO_SIZE_OBJECT "Bad Gateway".
The ERR_ZERO_SIZE_OBJECT errors were visible to the client when the
destination had only one address because serverDestinations.shift()
made the list of destination empty and startConnectionOrFail() failed.
When FwdState starts to use a pinned connection, the connection is treated as
an idle persistent connection as far as race detection is concerned.
Currently, pinned connections cannot be reopened, repinned, and retried after
a pconn race. This will change when server-side bumped connections become
pinned.
It felt wrong that a failed serverConn may remain set while we are opening a
new connection so I set it to NULL after a squid-dev discussion indicating
that doing so should be safe.
We also now reset the local port number to zero in case it was set to the
actual source port by ConnOpener or other code working with the previous
connection to the same serverDestinations[0] address, although simple tests
worked (and showed changing source port) without this reset.
Bug fix: sslpassword_program for ssl-bump http ports
Currently the sslpassword_program configuration parameter does not work
for encrypted certificate keys on ssl-bump enabled http ports, and user
always asked to give the SSL key password.
Changes to make fake certificates "stable" and "unique".
This patch modify bump-server-first branch to:
- use configured trusted CA private key for generating all fake certificates,
including trusted, untrusted, and self-signed fake certificates.
- use untrusted CA certificate which be deterministically derived from
the trusted CA certificate to both reduce configuration effort (compared to
a configuration option) and to generate identical untrusted CA certificates
given identical Squid configurations.
Alex Rousskov [Fri, 10 Feb 2012 00:32:44 +0000 (17:32 -0700)]
Do not cache partially loaded entries in shared mem cache (and then serve them)
When handling a conditional request, Squid may load the beginning of a cached
object from disk, realize that the client has the same fresh copy, and respond
with 304 Not Modified. After that, Squid was checking whether the partially
loaded object should be kept in shared memory cache (if enabled). There were
no checks preventing memory caching of the partially loaded object.
Later, partially cached objects were served to clients, resulting in truncated
responses. I believe this happens because shared memory cache does not keep
all the StoreEntry data (just like a disk cache does not do that) so the fact
that only a part of the object was available was lost.
Alex Rousskov [Fri, 10 Feb 2012 00:01:17 +0000 (17:01 -0700)]
Do not swap out swapped out objects.
I noticed that sometimes Squid would start swapping out an entry that was
recently loaded from disk and was still on disk. That wastes disk
resources (at best).
The old StoreEntry::mayStartSwapOut() code assumed that when swap_status is
not SWAPOUT_NONE it is SWAPOUT_WRITING, but SWAPOUT_WRITING is impossible
after recent StoreEntry::swapOut() modifications because mayStartSwapOut() is
only called when we are not swappingOut() already. SWAPOUT_DONE is possible.
Amos Jeffries [Thu, 9 Feb 2012 13:27:51 +0000 (06:27 -0700)]
Drop dead code in reply parsing
This code has not been used/needed in some time. It can die.
It is also no clear why it existed in the first place. The RFC is not
mentioned by number and RFC 2068/2616 only talk about tolerance for
whitespace before request lines, not replies.
projects / thirdparty / squid.git / log
