Re: [squid-dev] [PATCH] Log SSL Cryptography Parameters

[Francesco Chemolli]

On 10 Dec 2015, at 18:36, Christos Tsantilas  wrote:

> This patch adds the following formatting codes:

LGTM. +1.

Kinkie
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] [RFC] Fix shared memory initialization, cleanup. Ensure its usability.

[Amos Jeffries]

On 11/12/2015 5:06 a.m., Alex Rousskov wrote:
> On 12/10/2015 01:53 AM, Amos Jeffries wrote:
>> On 10/12/2015 1:24 p.m., Alex Rousskov wrote:
>>> On 12/09/2015 04:24 PM, Amos Jeffries wrote:
 On 10/12/2015 10:49 a.m., Alex Rousskov wrote:
>>> Questions: Should we add a configuration directive to control whether
>>> mlock(2) is called? If yes, should mlock(2) be called by default?
>>> Should mlock(2) failures be fatal?
> 
>> My answers are no, yes, yes. Since this is a startup thing squid.conf
>> post-processing is probably too late.
> 
> 
>> It makes them "maybe", no, yes.
> 
> 
>> Though I am still not convinced exactly that squid.conf is the right
>> place. As I said before:
>> "
>> In general if a setting is not something that can have a reasonable
>> temporary default for a while, then be switched around in realtime. Then
>> squid.conf is not a good place for it.
> 
> Yes, you said that, but (a) your precondition still does not quite match
> the situation at hand:
> 
> * The "perform mlock" setting _has_ a reasonable default (i.e., yes, lock),
> 
> * that default can be overwritten to "do not lock" if the admin does not
> want to lock, and
> 
> * the setting can even be switched around during reconfiguration (after
> somebody adds support for reconfigurable shared storage, although the
> change from no-lock to lock can be supported earlier);
> 
> and (b) I disagree with the overall decision logic you are proposing:
> IMO, command line options should be limited to these two areas:
> 
> i. controlling Squid behavior before squid.conf is parsed.
> ii. controlling another Squid instance (for legacy reasons).
> 
> Memory locking controls do not belong to the command line. And yes, many
> current command line options violate the above, but that does not make
> those violations good ideas and does not mean we should add more of them.
> 
> 
>> I would make it mandatory on "-k check" (but not -k parse). Optional
>> through a command line parameter on all other runs.
> 
> 
> I have two problems with the -k check approach:
> 
> 1. -k check is documented to parse configuration and send a signal to
> the already running Squid. To implement what you are proposing would
> require also initializing Squid storage. Initializing Squid storage when
> another Squid instance is running is either impossible (because it will
> conflict with the existing instance) or would require a highly
> customized code (to work around those conflicts) that will not fully
> test what a freshly started Squid would experience. Finally, locking
> memory on -k check may also swap out or even crash the running Squid
> instance!
> 
> 2. Since most folks do not run -k check, they will not be aware that
> their Squid or OS is misconfigured and will suffer from the obscure
> effects of that misconfiguration such as SIGBUS and segmentation fault
> deaths.
> 
> Problem #2 affects the "Optional through a command line parameter on all
> other runs" part as well.
> 
> 
>> The other (default / normal) runs having it OFF by default, since the -k
>> check should have confirmed already that it is going to work.
> 
> Yes, but I do not think we should use -kcheck and can rely on -kcheck as
> discussed in #1 and #2 above. If we want to avoid these SIGBUS problems
> in old and new configurations, we should lock by default. I see no other
> way.
> 
> We may, of course, decide that SIGBUS problems are not significant
> enough to be worth avoiding, but it feels wrong to ship a proxy that can
> easily crash in its default configuration just because we decided to
> avoid a check preventing such crashes. I would rather ship a proxy that
> starts slow when explicitly configured to use lots of shared memory.
> 

Okay. Good points, and some very good ones.


So it comes down to doing it your way and we will have to come up with
something to explain to confused admin why their proxies are now slow.

If we really have no choice in the matter (and you make a good case for
that being so). Then requiring an answer up front on what we will say is
not useful, just blocking progress. It is hard to pick any answer or
explanation in advance of knowing how bad the problem is (if at all),
and I dont think we should spend time trying to.

So is there anything else you need to discuss? or shall we wind up this
thread and wait to see what appears for audit?

Amos
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] [RFC] Fix shared memory initialization, cleanup. Ensure its usability.

[Amos Jeffries]

On 10/12/2015 1:24 p.m., Alex Rousskov wrote:
> On 12/09/2015 04:24 PM, Amos Jeffries wrote:
>> On 10/12/2015 10:49 a.m., Alex Rousskov wrote:
> Questions: Should we add a configuration directive to control whether
> mlock(2) is called? If yes, should mlock(2) be called by default?
> Should mlock(2) failures be fatal?
> 
 My answers are no, yes, yes. Since this is a startup thing squid.conf
 post-processing is probably too late.
> 
> 
>>> Shared memory is not allocated until _after_ we parse squid.conf.
> 
> 
>> Ah. Okay.
> 
> 
> Does this understanding change your no/yes/yes answers?
> 

It makes them "maybe", no, yes.

Though I am still not convinced exactly that squid.conf is the right
place. As I said before:
"
In general if a setting is not something that can have a reasonable
temporary default for a while, then be switched around in realtime. Then
squid.conf is not a good place for it.

I would make it mandatory on "-k check" (but not -k parse). Optional
through a command line parameter on all other runs. (Remember we have
long-args possible now so no need to squeeze it into a gap.)
"

Which is *not* doing mlock by default (retaining current behaviour). The
slowdown then only happens on -k check, which is the manual test to
check for problems. If mlock has a problem, a FATAL debugs (maybe exit
too, but could keep going to find other errors) during the test run is
very appropriate.

The other (default / normal) runs having it OFF by default, since the -k
check should have confirmed already that it is going to work. Unless the
admin explicitly sets the config / command line option to ON. In which
case complaints of slowdown are unjustified, and they can be told to
disable again.


> My plan was to tell them to disable mlock() (via a new squid.conf
> directive) if all of the items below apply to them:
> 
> (a) mlock() is successful now
> (b) they are reasonably sure no environment or configuration changes
> will make mlock() unsuccessful (if it is re-enabled) without them
> knowing about the problem
> (c) mlock() slows things down too much in their setup
> (d) they want to trade faster startup for micro delays during runtime
> 

Which seems to me all the things a -k check execution run will be
testing for naturally. So if mlock errors are fatal on -k check, they
get alerted to fix it before the running process is signalled to restart
with the new config.

Amos

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

[squid-dev] Ntlm helper child exceeds limit

[manojmaybe]

I have used squid 3.3 proxy server with NTLM authentication. I have use the
squid config file for  maximum of 3000 children.

auth_param ntlm children 3000 startup=0 idle=0

but when traffic is very high ,found that more than 3000 ntlm helper
children is active (used ps command to get the number of ntlm child process)
and helper child are going to be crash and showing error cache log


read(net): Connection reset by peer
read(net): Connection reset by peer
read(net): Connection reset by peer
read(net): Connection reset by peer
read(net): Connection reset by peer
2015/12/09 15:01:36 kid2| ERROR: NTLM Authentication Helper '0x160fa948'
crashed!.
2015/12/09 15:01:36 kid2| ERROR: NTLM Authentication validating user. Error
returned 'BH Internal error'
2015/12/09 15:01:36 kid2| WARNING: ntlmauthenticator #1 exited
2015/12/09 15:01:36 kid2| Too few ntlmauthenticator processes are running
(need 1/300)
2015/12/09 15:01:36 kid2| Starting new helpers
2015/12/09 15:01:36 kid2| helperOpenServers: Starting 1/300 'netcat'
processes
2015/12/09 15:01:36 kid1| ERROR: NTLM Authentication Helper '0x1615d048'
crashed!.
2015/12/09 15:01:36 kid1| ERROR: NTLM Authentication validating user. Error
returned 'BH Internal error'
2015/12/09 15:01:36 kid1| WARNING: ntlmauthenticator #1 exited
2015/12/09 15:01:36 kid1| Too few ntlmauthenticator processes are running
(need 1/300)
2015/12/09 15:01:36 kid1| Starting new helpers
2015/12/09 15:01:36 kid1| helperOpenServers: Starting 1/300 'netcat'
processes
2015/12/09 15:01:36 kid2| ERROR: NTLM Authentication Helper '0x162b7dd8'
crashed!.
2015/12/09 15:01:36 kid2| ERROR: NTLM Authentication validating user. Error
returned 'BH Internal error'
2015/12/09 15:01:36 kid2| WARNING: ntlmauthenticator #1 exited
2015/12/09 15:01:36 kid2| Too few ntlmauthenticator processes are running
(need 1/300)
2015/12/09 15:01:36 kid2| Starting new helpers
2015/12/09 15:01:36 kid2| helperOpenServers: Starting 1/300 'netcat'
processes
2015/12/09 15:01:36 kid1| ERROR: NTLM Authentication Helper '0x15562898'
crashed!.
2015/12/09 15:01:36 kid1| ERROR: NTLM Authentication validating user. Error
returned 'BH Internal error'
2015/12/09 15:01:36 kid1| WARNING: ntlmauthenticator #1 exited
2015/12/09 15:01:36 kid1| Too few ntlmauthenticator processes are running
(need 1/300)
2015/12/09 15:01:36 kid1| Starting new helpers
2015/12/09 15:01:36 kid1| helperOpenServers: Starting 1/300 'netcat'
processes
2015/12/09 15:01:36 kid2| ERROR: NTLM Authentication Helper '0x168abdf8'
crashed!.
2015/12/09 15:01:36 kid2| ERROR: NTLM Authentication validating user. Error
returned 'BH Internal error'
2015/12/09 15:01:36 kid2| WARNING: ntlmauthenticator #1 exited
2015/12/09 15:01:36 kid2| Too few ntlmauthenticator processes are running
(need 1/300)
2015/12/09 15:01:36 kid2| Starting new helpers
2015/12/09 15:01:36 kid2| helperOpenServers: Starting 1/300 'netcat'
processes
2015/12/09 15:01:36 kid1| ERROR: NTLM Authentication Helper '0x15727ba8'
crashed!.
2015/12/09 15:01:36 kid1| ERROR: NTLM Authentication validating user. Error
returned 'BH Internal error'
2015/12/09 15:01:36 kid1| WARNING: ntlmauthenticator #1 exited
2015/12/09 15:01:36 kid1| Too few ntlmauthenticator processes are running
(need 1/300)
2015/12/09 15:01:36 kid1| Starting new helpers



--
View this message in context: 
http://squid-web-proxy-cache.1019090.n4.nabble.com/Ntlm-helper-child-exceeds-limit-tp4675091.html
Sent from the Squid - Development mailing list archive at Nabble.com.
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] [RFC] Fix shared memory initialization, cleanup. Ensure its usability.

[Alex Rousskov]

On 12/10/2015 11:25 AM, Francesco Chemolli wrote:
> 
> Well, slow in starting up, not in operating, hopefully.

Yes, I expect runtime performance to negligibly(?) improve during warmup
(on correctly configured deployments) due to pre-allocation of all
shared memory used during runtime.


> And it is actually pretty detectable: let’s choose an arbitrary size
> of the shared segment and output a warning message to cache.log. This
> won’t help with the speed, but it might help with the confusion..

Excellent idea! And we can improve it by warning if the combined
mlock(2) _delay_ exceeds some hard-coded value.

I wonder what that warning should say to avoid a [usually wrong]
implication that memory locking should be turned off. How about
something like this:

"""
2015/12/09 11:24:51| Total mlock() delay exceeded 5.3 seconds. See
shared_memory_locking in squid.conf.documented.
"""

We can set level-1 reporting threshold at 5 or even 10 seconds. We do
not need to add a shared_memory_locking parameter to disable this
reporting (while still locking), do we?


Thank you,

Alex.

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] [RFC] Fix shared memory initialization, cleanup. Ensure its usability.

[Kinkie]

On Thu, Dec 10, 2015 at 10:07 PM, Alex Rousskov
 wrote:
> On 12/10/2015 11:25 AM, Francesco Chemolli wrote:
>>
>> Well, slow in starting up, not in operating, hopefully.
>
> Yes, I expect runtime performance to negligibly(?) improve during warmup
> (on correctly configured deployments) due to pre-allocation of all
> shared memory used during runtime.
>
>
>> And it is actually pretty detectable: let’s choose an arbitrary size
>> of the shared segment and output a warning message to cache.log. This
>> won’t help with the speed, but it might help with the confusion..
>
> Excellent idea! And we can improve it by warning if the combined
> mlock(2) _delay_ exceeds some hard-coded value.
>
> I wonder what that warning should say to avoid a [usually wrong]
> implication that memory locking should be turned off. How about
> something like this:
>
> """
> 2015/12/09 11:24:51| Total mlock() delay exceeded 5.3 seconds. See
> shared_memory_locking in squid.conf.documented.
> """
>
> We can set level-1 reporting threshold at 5 or even 10 seconds. We do
> not need to add a shared_memory_locking parameter to disable this
> reporting (while still locking), do we?

I was thinking more of something _before_ the delay:
e.g. if shared memory is > 100mb, then
"locking XXX mbytes of shared memory. On some systems this may require
a long time. Squid is not dead, it's just thinking"

and then when it's done "shared memory locking of X Mb done in Y
seconds. This may be normal depending on the size of the shared memory
area".

-- 
Francesco
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] Ntlm helper child exceeds limit

[Amos Jeffries]

On 10/12/2015 10:42 p.m., manojmaybe wrote:
> I have used squid 3.3 proxy server with NTLM authentication. I have use the
> squid config file for  maximum of 3000 children.

Please upgrade your Squid. 3.5 is the current stable release.

> 
> auth_param ntlm children 3000 startup=0 idle=0
> 
> but when traffic is very high ,found that more than 3000 ntlm helper
> children is active (used ps command to get the number of ntlm child process)
> and helper child are going to be crash and showing error cache log
> 

If you have so many users that you need 3K NTLM helpers then NTLM is
probably the worst thing you could be doing for authentication.

It takes a relatively long time to authenticate a single client
connection, and they are statefully locked the entire time so no other
visitors can use them to authenticate. That causes huge memory resource
consumption in the proxy. And the system has limits on how many AD
connections can be made simulteneously. Winbindd itself starts failing
after 255 concurrent logins, Samba is a bit higher but not 3K.


> 
> read(net): Connection reset by peer
> read(net): Connection reset by peer
> read(net): Connection reset by peer
> read(net): Connection reset by peer
> read(net): Connection reset by peer

Where is that coming from? Not Squid.

> 2015/12/09 15:01:36 kid2| ERROR: NTLM Authentication Helper '0x160fa948'
> crashed!.
> 2015/12/09 15:01:36 kid2| ERROR: NTLM Authentication validating user. Error
> returned 'BH Internal error'
> 2015/12/09 15:01:36 kid2| WARNING: ntlmauthenticator #1 exited
> 2015/12/09 15:01:36 kid2| Too few ntlmauthenticator processes are running
> (need 1/300)
> 2015/12/09 15:01:36 kid2| Starting new helpers
> 2015/12/09 15:01:36 kid2| helperOpenServers: Starting 1/300 'netcat'
> processes

Does netcat speak NTLM protocol? or Squid helper protocol for that matter?

AFAIK the netcat tool has a habit of closing connections, but only
half-way. Squid helpers MUST NOT close any of their connections to Squid
unless they are aborting and shutting down.

netcat may also be diverting and hiding critical error messages produced
by whatever the real authenticator is on stderr.

Amos

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] [RFC] Fix shared memory initialization, cleanup. Ensure its usability.

[Francesco Chemolli]

On 10 Dec 2015, at 19:23, Amos Jeffries  wrote:

> Okay. Good points, and some very good ones.
> 
> 
> So it comes down to doing it your way and we will have to come up with
> something to explain to confused admin why their proxies are now slow.

Well, slow in starting up, not in operating, hopefully.
And it is actually pretty detectable: let’s choose an arbitrary size of the 
shared segment and output a warning message to cache.log. This won’t help with 
the speed, but it might help with the confusion..

Kinkie
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] [PATCH] %ssl::

[Francesco Chemolli]

On 10 Dec 2015, at 18:04, Christos Tsantilas  wrote:

> This is a complement patch for "%ssl:: applied to trunk as r14343.

LGTM. +1.

Kinkie
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

[squid-dev] [PATCH] Log SSL Cryptography Parameters

[Christos Tsantilas]

This patch adds the following formatting codes:

  %ssl::>negotiated_version
  The SSL version of the client-to-Squid connection.

  %ssl::received_hello_version
  The SSL version of the Hello message received from SSL client

  %ssl::received_supported_version
  The maximum SSL version supported by the the SSL client.

  %ssl::cipher
  The negotiated cipher of the client-to-Squid connection.

  %ssl::These are useful for statistics collection, security reviews, and 
reviews prior to adjusting the list of the allowed SSL protocols and 
ciphers.


This is a Measurement Factory project
Log SSL Cryptography Parameters

This patch adds the following formatting codes:
  %ssl::>negotiated_version  The SSL version of the client-to-Squid connection.
  %ssl::received_hello_version The SSL version of the Hello message received
from SSL client
  %ssl::received_supported_version The maximum SSL version supported by the
the SSL client.
  %ssl::cipher   The negotiated cipher of the client-to-Squid connection.
  %ssl::cert_issuer
 The Issuer field of the received client
 SSL certificate or a dash ('-') if Squid has
 received an invalid/malformed certificate or
 no certificate at all. Consider encoding the
 logged value because Issuer often has spaces.
 
 		ssl::negotiated_version The negotiated SSL version of the
+client-to-Squid connection.
+
+		%ssl::received_hello_version The SSL version of the Hello
+message received from SSL client.
+
+		%ssl::received_supported_version The maximum SSL version
+supported by the SSL client.
+
+		%ssl::negotiated_cipher The negotiated cipher of the
+client-to-Squid connection.
+
+		%ssl::clientConnection->tlsNegotiations()->fillWith(ssl);
 
 client_cert = SSL_get_peer_certificate(ssl);
 
 if (client_cert != NULL) {
 debugs(83, 3, "clientNegotiateSSL: FD " << fd <<
" client certificate: subject: " <<
X509_NAME_oneline(X509_get_subject_name(client_cert), 0, 0));
 
 debugs(83, 3, "clientNegotiateSSL: FD " << fd <<
" client certificate: issuer: " <<
X509_NAME_oneline(X509_get_issuer_name(client_cert), 0, 0));
 
 X509_free(client_cert);
 } else {
 debugs(83, 5, "clientNegotiateSSL: FD " << fd <<
" has no certificate.");
 }
 
 #if defined(TLSEXT_NAMETYPE_host_name)
 if (!conn->serverBump()) {
@@ -3874,41 +3874,41 @@
 
 int ret = 0;
 if ((ret = Squid_SSL_accept(conn, clientPeekAndSpliceSSL)) < 0)
 debugs(83, 2, "SSL_accept failed.");
 
 BIO *b = SSL_get_rbio(ssl);
 assert(b);
 Ssl::ClientBio *bio = static_cast(b->ptr);
 if (ret < 0) {
 const err_type err = bio->noSslClient() ? ERR_PROTOCOL_UNKNOWN : ERR_SECURE_ACCEPT_FAIL;
 if (!conn->spliceOnError(err))
 conn->clientConnection->close();
 return;
 }
 
 if (bio->rBufData().contentSize() > 0)
 conn->receivedFirstByte();
 
 if (bio->gotHello()) {
 if (conn->serverBump()) {
-Ssl::Bio::sslFeatures const  = bio->getFeatures();
+Ssl::Bio::sslFeatures const  = bio->receivedHelloFeatures();
 if (!features.serverName.isEmpty()) {
 conn->serverBump()->clientSni = features.serverName;
 conn->resetSslCommonName(features.serverName.c_str());
 }
 }
 
 debugs(83, 5, "I got hello. Start forwarding the request!!! ");
 Comm::SetSelect(fd, COMM_SELECT_READ, NULL, NULL, 0);
 Comm::SetSelect(fd, COMM_SELECT_WRITE, NULL, NULL, 0);
 conn->startPeekAndSpliceDone();
 return;
 }
 }
 
 void ConnStateData::startPeekAndSplice()
 {
 // will call httpsPeeked() with certificate and connection, eventually
 auto unConfiguredCTX = Ssl::createSSLContext(port->signingCert, port->signPkey, *port);
 fd_table[clientConnection->fd].dynamicSslContext = unConfiguredCTX;
 
@@ -3951,40 +3951,44 @@
 bumpAction = (Ssl::BumpMode)answer.kind;
 } else
 bumpAction = Ssl::bumpSplice;
 
 connState->serverBump()->act.step2 = bumpAction;
 connState->sslBumpMode = bumpAction;
 
 if (bumpAction == Ssl::bumpTerminate) {
 connState->clientConnection->close();
 } else if (bumpAction != Ssl::bumpSplice) {
 connState->startPeekAndSpliceDone();
 } else
 connState->splice();
 }
 
 void
 ConnStateData::splice()
 {
 //Normally we can splice here, because we just got client hello message
 auto ssl = fd_table[clientConnection->fd].ssl;
+
+//retrieve received SSL client information
+clientConnection->tlsNegotiations()->fillWith(ssl);
+
 BIO *b = SSL_get_rbio(ssl);
 Ssl::ClientBio *bio = static_cast(b->ptr);
 MemBuf const  = bio->rBufData();
 debugs(83,5, "Bio for  " << clientConnection << " read " << rbuf.contentSize() << " helo bytes");
 // 

[squid-dev] [PATCH] FwdState should retry connect to the next ip after a Ssl::PeerConnector failure

[Christos Tsantilas]

When the Ssl::PeerConnector fails to establish an SSL connection 
FwdState does not retry to connect to the next destination server ip 
address, but instead returns an error.


This is a Measurement Factory project
FwdState should retry connect to the next ip after a Ssl::PeerConnector failure

When the Ssl::PeerConnector fails to establish an SSL connection FwdState does
not retry to connect to the next destination server ip address, but instead
returns an error.

This is a Measurement Factory project

=== modified file 'src/FwdState.cc'
--- src/FwdState.cc	2015-11-30 10:53:23 +
+++ src/FwdState.cc	2015-12-10 15:37:09 +
@@ -663,42 +663,40 @@
 void
 FwdState::connectDone(const Comm::ConnectionPointer , Comm::Flag status, int xerrno)
 {
 if (status != Comm::OK) {
 ErrorState *const anErr = makeConnectingError(ERR_CONNECT_FAIL);
 anErr->xerrno = xerrno;
 fail(anErr);
 
 /* it might have been a timeout with a partially open link */
 if (conn != NULL) {
 if (conn->getPeer())
 peerConnectFailed(conn->getPeer());
 
 conn->close();
 }
 retryOrBail();
 return;
 }
 
 serverConn = conn;
-flags.connected_okay = true;
-
 debugs(17, 3, HERE << serverConnection() << ": '" << entry->url() << "'" );
 
 comm_add_close_handler(serverConnection()->fd, fwdServerClosedWrapper, this);
 
 if (serverConnection()->getPeer())
 peerConnectSucceded(serverConnection()->getPeer());
 
 #if USE_OPENSSL
 if (!request->flags.pinned) {
 const CachePeer *p = serverConnection()->getPeer();
 const bool peerWantsTls = p && p->secure.encryptTransport;
 // userWillTlsToPeerForUs assumes CONNECT == HTTPS
 const bool userWillTlsToPeerForUs = p && p->options.originserver &&
 request->method == Http::METHOD_CONNECT;
 const bool needTlsToPeer = peerWantsTls && !userWillTlsToPeerForUs;
 const bool needTlsToOrigin = !p && request->url.getScheme() == AnyP::PROTO_HTTPS;
 if (needTlsToPeer || needTlsToOrigin || request->flags.sslPeek) {
 HttpRequest::Pointer requestPointer = request;
 AsyncCall::Pointer callback = asyncCall(17,4,
 "FwdState::ConnectedToPeer",
@@ -710,48 +708,51 @@
 connector = new Ssl::PeekingPeerConnector(requestPointer, serverConnection(), clientConn, callback, sslNegotiationTimeout);
 else
 connector = new Ssl::BlindPeerConnector(requestPointer, serverConnection(), callback, sslNegotiationTimeout);
 AsyncJob::Start(connector); // will call our callback
 return;
 }
 }
 #endif
 
 // if not encrypting just run the post-connect actions
 Security::EncryptorAnswer nil;
 connectedToPeer(nil);
 }
 
 void
 FwdState::connectedToPeer(Security::EncryptorAnswer )
 {
 if (ErrorState *error = answer.error.get()) {
 fail(error);
 answer.error.clear(); // preserve error for errorSendComplete()
-self = NULL;
+if (CachePeer *p = serverConnection()->getPeer())
+peerConnectFailed(p);
+retryOrBail();
 return;
 }
 
 // should reach ConnStateData before the dispatched Client job starts
 CallJobHere1(17, 4, request->clientConnectionManager, ConnStateData,
  ConnStateData::notePeerConnection, serverConnection());
 
+flags.connected_okay = true;
 dispatch();
 }
 
 void
 FwdState::connectTimeout(int fd)
 {
 debugs(17, 2, "fwdConnectTimeout: FD " << fd << ": '" << entry->url() << "'" );
 assert(serverDestinations[0] != NULL);
 assert(fd == serverDestinations[0]->fd);
 
 if (entry->isEmpty()) {
 ErrorState *anErr = new ErrorState(ERR_CONNECT_FAIL, Http::scGatewayTimeout, request);
 anErr->xerrno = ETIMEDOUT;
 fail(anErr);
 
 /* This marks the peer DOWN ... */
 if (serverDestinations[0]->getPeer())
 peerConnectFailed(serverDestinations[0]->getPeer());
 }
 

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] [RFC] Fix shared memory initialization, cleanup. Ensure its usability.

[Alex Rousskov]

On 12/10/2015 01:53 AM, Amos Jeffries wrote:
> On 10/12/2015 1:24 p.m., Alex Rousskov wrote:
>> On 12/09/2015 04:24 PM, Amos Jeffries wrote:
>>> On 10/12/2015 10:49 a.m., Alex Rousskov wrote:
>> Questions: Should we add a configuration directive to control whether
>> mlock(2) is called? If yes, should mlock(2) be called by default?
>> Should mlock(2) failures be fatal?

> My answers are no, yes, yes. Since this is a startup thing squid.conf
> post-processing is probably too late.


> It makes them "maybe", no, yes.


> Though I am still not convinced exactly that squid.conf is the right
> place. As I said before:
> "
> In general if a setting is not something that can have a reasonable
> temporary default for a while, then be switched around in realtime. Then
> squid.conf is not a good place for it.

Yes, you said that, but (a) your precondition still does not quite match
the situation at hand:

* The "perform mlock" setting _has_ a reasonable default (i.e., yes, lock),

* that default can be overwritten to "do not lock" if the admin does not
want to lock, and

* the setting can even be switched around during reconfiguration (after
somebody adds support for reconfigurable shared storage, although the
change from no-lock to lock can be supported earlier);

and (b) I disagree with the overall decision logic you are proposing:
IMO, command line options should be limited to these two areas:

i. controlling Squid behavior before squid.conf is parsed.
ii. controlling another Squid instance (for legacy reasons).

Memory locking controls do not belong to the command line. And yes, many
current command line options violate the above, but that does not make
those violations good ideas and does not mean we should add more of them.


> I would make it mandatory on "-k check" (but not -k parse). Optional
> through a command line parameter on all other runs.


I have two problems with the -k check approach:

1. -k check is documented to parse configuration and send a signal to
the already running Squid. To implement what you are proposing would
require also initializing Squid storage. Initializing Squid storage when
another Squid instance is running is either impossible (because it will
conflict with the existing instance) or would require a highly
customized code (to work around those conflicts) that will not fully
test what a freshly started Squid would experience. Finally, locking
memory on -k check may also swap out or even crash the running Squid
instance!

2. Since most folks do not run -k check, they will not be aware that
their Squid or OS is misconfigured and will suffer from the obscure
effects of that misconfiguration such as SIGBUS and segmentation fault
deaths.

Problem #2 affects the "Optional through a command line parameter on all
other runs" part as well.


> The other (default / normal) runs having it OFF by default, since the -k
> check should have confirmed already that it is going to work.

Yes, but I do not think we should use -kcheck and can rely on -kcheck as
discussed in #1 and #2 above. If we want to avoid these SIGBUS problems
in old and new configurations, we should lock by default. I see no other
way.

We may, of course, decide that SIGBUS problems are not significant
enough to be worth avoiding, but it feels wrong to ship a proxy that can
easily crash in its default configuration just because we decided to
avoid a check preventing such crashes. I would rather ship a proxy that
starts slow when explicitly configured to use lots of shared memory.


Thank you,

Alex.
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

[squid-dev] [PATCH] Avoid memory leaks when a certificate validator is used with SslBump

[Christos Tsantilas]

When a certificate validator was used, sslCrtvdHandleReplyWrapper 
delivered validator response directly to the Ssl::PeerConnector job 
using job's Ssl::CertValidationHelper::CVHCB callback. If that 
synchronous call happened to be the last job call, then 
Ssl::PeerConnector::done() would become true for the job, as it should, 
but nobody would notice that the PeerConnector job object should be 
deleted, and the object would leak.


This fix converts CVHCB into an async job call to avoid direct, 
unprotected job calls in this context.


This is a Measurement Factory project.
Avoid memory leaks when a certificate validator is used with SslBump

When a certificate validator was used, sslCrtvdHandleReplyWrapper delivered
validator response directly to the Ssl::PeerConnector job using job's
Ssl::CertValidationHelper::CVHCB callback. If that synchronous call happened
to be the last job call, then Ssl::PeerConnector::done() would become true
for the job, as it should, but nobody would notice that the PeerConnector
job object should be deleted, and the object would leak.

This fix converts CVHCB into an async job call to avoid direct, unprotected
job calls in this context.

This is a Measurement Factory project.

=== modified file 'src/ssl/PeerConnector.cc'
--- src/ssl/PeerConnector.cc	2015-11-28 03:00:35 +
+++ src/ssl/PeerConnector.cc	2015-11-30 18:06:46 +
@@ -169,41 +169,42 @@
 Ssl::PeerConnector::sslFinalized()
 {
 if (Ssl::TheConfig.ssl_crt_validator && useCertValidator_) {
 const int fd = serverConnection()->fd;
 SSL *ssl = fd_table[fd].ssl;
 
 Ssl::CertValidationRequest validationRequest;
 // WARNING: Currently we do not use any locking for any of the
 // members of the Ssl::CertValidationRequest class. In this code the
 // Ssl::CertValidationRequest object used only to pass data to
 // Ssl::CertValidationHelper::submit method.
 validationRequest.ssl = ssl;
 validationRequest.domainName = request->url.host();
 if (Ssl::CertErrors *errs = static_cast(SSL_get_ex_data(ssl, ssl_ex_index_ssl_errors)))
 // validationRequest disappears on return so no need to cbdataReference
 validationRequest.errors = errs;
 else
 validationRequest.errors = NULL;
 try {
 debugs(83, 5, "Sending SSL certificate for validation to ssl_crtvd.");
-Ssl::CertValidationHelper::GetInstance()->sslSubmit(validationRequest, sslCrtvdHandleReplyWrapper, this);
+AsyncCall::Pointer call = asyncCall(83,5, "Ssl::PeerConnector::sslCrtvdHandleReply", Ssl::CertValidationHelper::CbDialer(this, ::PeerConnector::sslCrtvdHandleReply, nullptr));
+Ssl::CertValidationHelper::GetInstance()->sslSubmit(validationRequest, call);
 return false;
 } catch (const std::exception ) {
 debugs(83, DBG_IMPORTANT, "ERROR: Failed to compose ssl_crtvd " <<
"request for " << validationRequest.domainName <<
" certificate: " << e.what() << "; will now block to " <<
"validate that certificate.");
 // fall through to do blocking in-process generation.
 ErrorState *anErr = new ErrorState(ERR_GATEWAY_FAILURE, Http::scInternalServerError, request.getRaw());
 
 noteNegotiationDone(anErr);
 bail(anErr);
 serverConn->close();
 return true;
 }
 }
 
 noteNegotiationDone(NULL);
 return true;
 }
 
@@ -292,61 +293,56 @@
 }
 
 Ssl::BumpMode
 Ssl::PeekingPeerConnector::checkForPeekAndSpliceGuess() const
 {
 if (const ConnStateData *csd = request->clientConnectionManager.valid()) {
 const Ssl::BumpMode currentMode = csd->sslBumpMode;
 if (currentMode == Ssl::bumpStare) {
 debugs(83,5, "default to bumping after staring");
 return Ssl::bumpBump;
 }
 debugs(83,5, "default to splicing after " << currentMode);
 } else {
 debugs(83,3, "default to splicing due to missing info");
 }
 
 return Ssl::bumpSplice;
 }
 
 void
-Ssl::PeerConnector::sslCrtvdHandleReplyWrapper(void *data, Ssl::CertValidationResponse const )
+Ssl::PeerConnector::sslCrtvdHandleReply(Ssl::CertValidationResponse::Pointer validationResponse)
 {
-Ssl::PeerConnector *connector = (Ssl::PeerConnector *)(data);
-connector->sslCrtvdHandleReply(validationResponse);
-}
+Must(validationResponse != NULL);
 
-void
-Ssl::PeerConnector::sslCrtvdHandleReply(Ssl::CertValidationResponse const )
-{
 Ssl::CertErrors *errs = NULL;
 Ssl::ErrorDetail *errDetails = NULL;
 bool validatorFailed = false;
 if (!Comm::IsConnOpen(serverConnection())) {
 return;
 }
 
-debugs(83,5, request->url.host() << " cert validation result: " << validationResponse.resultCode);
+debugs(83,5, request->url.host() << " cert validation result: " << validationResponse->resultCode);