From: Christos Tsantilas Date: Sat, 3 Mar 2012 06:19:04 +0000 (+0200) Subject: Simplify Ssl::ServerPeeker and renamed it to Ssl::ServerBump X-Git-Tag: BumpSslServerFirst.take06~6 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=fd4624d7e193817a763d4474fcab5436512fd945;p=thirdparty%2Fsquid.git 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. --- diff --git a/src/client_side.cc b/src/client_side.cc index f5e88637a5..139185f29f 100644 --- a/src/client_side.cc +++ b/src/client_side.cc @@ -107,6 +107,7 @@ #include "errorpage.h" #include "eui/Config.h" #include "fde.h" +#include "forward.h" #include "HttpHdrContRange.h" #include "HttpReply.h" #include "HttpRequest.h" @@ -124,7 +125,7 @@ #if USE_SSL #include "ssl/context_storage.h" #include "ssl/helper.h" -#include "ssl/ServerPeeker.h" +#include "ssl/ServerBump.h" #include "ssl/support.h" #include "ssl/gadgets.h" #endif @@ -813,11 +814,8 @@ ConnStateData::~ConnStateData() if (bodyPipe != NULL) stopProducingFor(bodyPipe, false); -#if USE_SSL - if (bumpErrorEntry) - bumpErrorEntry->unlock(); - - cbdataReferenceDone(bumpSslErrorNoList); +#if USE_SSL + delete sslServerBump; #endif } @@ -2469,11 +2467,12 @@ ConnStateData::quitAfterError(HttpRequest *request) bool ConnStateData::serveDelayedError(ClientSocketContext *context) { ClientHttpRequest *http = context->http; - StoreEntry *e = bumpServerFirstErrorEntry(); - if (!e) + + if (!sslServerBump) return false; - if (!e->isEmpty()) { + assert(sslServerBump->entry); + if (!sslServerBump->entry->isEmpty()) { quitAfterError(http->request); //Failed? Here we should get the error from conn and send it to client @@ -2483,16 +2482,16 @@ bool ConnStateData::serveDelayedError(ClientSocketContext *context) clientReplyContext *repContext = dynamic_cast(node->data.getRaw()); assert (repContext); debugs(33, 5, "Connection first has failed for " << http->uri << ". Respond with an error"); - repContext->setReplyToStoreEntry(e); + repContext->setReplyToStoreEntry(sslServerBump->entry); context->pullData(); return true; } // We are in ssl-bump first mode. We have not the server connect name when // we connected to server so we have to check certificates subject with our server name - if (X509 *server_cert = getBumpServerCert()) { + if (sslServerBump && sslServerBump->serverCert.get()) { HttpRequest *request = http->request; - if (!Ssl::checkX509ServerValidity(server_cert, request->GetHost())) { + if (!Ssl::checkX509ServerValidity(sslServerBump->serverCert.get(), request->GetHost())) { debugs(33, 2, "SQUID_X509_V_ERR_DOMAIN_MISMATCH: Certificate does not match domainname " << request->GetHost()); ACLFilledChecklist check(Config.ssl_client.cert_error, request, dash_str); @@ -2511,7 +2510,8 @@ bool ConnStateData::serveDelayedError(ClientSocketContext *context) assert (repContext); // Fill the server ip address and server hostname for use with ErrorState - request->hier.note(pinning.serverConnection, request->GetHost()); + HttpRequest::Pointer const & peekerRequest = sslServerBump->request; + request->hier.note(peekerRequest->hier.tcpServer, request->GetHost()); // Create an error object and fill it ErrorState *err = new ErrorState(ERR_SECURE_CONNECT_FAIL, HTTP_SERVICE_UNAVAILABLE, request); @@ -2521,7 +2521,7 @@ bool ConnStateData::serveDelayedError(ClientSocketContext *context) #else err->xerrno = EACCES; #endif - Ssl::ErrorDetail *errDetail = new Ssl::ErrorDetail( SQUID_X509_V_ERR_DOMAIN_MISMATCH, server_cert, NULL); + Ssl::ErrorDetail *errDetail = new Ssl::ErrorDetail( SQUID_X509_V_ERR_DOMAIN_MISMATCH, sslServerBump->serverCert.get(), NULL); err->detail = errDetail; repContext->setReplyToError(request->method, err); assert(context->http->out.offset == 0); @@ -3677,13 +3677,14 @@ void ConnStateData::buildSslCertGenerationParams(Ssl::CertificateProperties &cer certProperties.commonName = sslCommonName.defined() ? sslCommonName.termedBuf() : sslConnectHostOrIp.termedBuf(); // fake certificate adaptation requires bump-server-first mode - if (!bumpServerFirstErrorEntry()) + if (!sslServerBump) return; // In the case of error while connecting to secure server, use a fake trusted certificate, // with no mimicked fields and no adaptation algorithms - if (bumpServerFirstErrorEntry()->isEmpty()) { - if (X509 *mimicCert = bumpServerCert.get()) + assert(sslServerBump->entry); + if (sslServerBump->entry->isEmpty()) { + if (X509 *mimicCert = sslServerBump->serverCert.get()) certProperties.mimicCert.resetAndLock(mimicCert); HttpRequest *fakeRequest = new HttpRequest(); @@ -3696,7 +3697,7 @@ void ConnStateData::buildSslCertGenerationParams(Ssl::CertificateProperties &cer checklist.conn(this); checklist.src_addr = clientConnection->remote; checklist.my_addr = clientConnection->local; - checklist.sslErrorList = cbdataReference(bumpSslErrorNoList); + checklist.sslErrorList = cbdataReference(sslServerBump->bumpSslErrorNoList); for (sslproxy_cert_adapt *ca = Config.ssl_client.cert_adapt; ca != NULL; ca = ca->next) { if (ca->aclList && checklist.fastCheck(ca->aclList) == ACCESS_ALLOWED) { @@ -3729,7 +3730,7 @@ void ConnStateData::buildSslCertGenerationParams(Ssl::CertificateProperties &cer break; } } - } else {// if (!bumpServerFirstErrorEntry()->isEmpty()) + } else {// if (!sslServerBump->entry->isEmpty()) // Use trusted certificate for a Squid-generated error // or the user would have to add a security exception // just to see the error page. We will close the connection @@ -3864,13 +3865,17 @@ ConnStateData::switchToHttps(const char *host, const int port) const bool alwaysBumpServerFirst = true; if (alwaysBumpServerFirst) { - Must(!httpsPeeker.set()); - httpsPeeker = new Ssl::ServerPeeker(this, sslConnectHostOrIp.termedBuf(), port); - bumpErrorEntry = httpsPeeker->storeEntry(); - Must(bumpErrorEntry); - bumpErrorEntry->lock(); + Must(!sslServerBump); + HttpRequest *fakeRequest = new HttpRequest; + fakeRequest->flags.sslPeek = 1; + fakeRequest->SetHost(sslConnectHostOrIp.termedBuf()); + fakeRequest->port = port; + fakeRequest->protocol = AnyP::PROTO_HTTPS; + fakeRequest->clientConnectionManager = this; + sslServerBump = new Ssl::ServerBump(fakeRequest); + // will call httpsPeeked() with certificate and connection, eventually - AsyncJob::Start(httpsPeeker.raw()); + FwdState::fwdStart(clientConnection, sslServerBump->entry, sslServerBump->request); return; } @@ -3881,7 +3886,7 @@ ConnStateData::switchToHttps(const char *host, const int port) void ConnStateData::httpsPeeked(Comm::ConnectionPointer serverConnection) { - Must(httpsPeeker.set()); + Must(sslServerBump != NULL); if (Comm::IsConnOpen(serverConnection)) { SSL *ssl = fd_table[serverConnection->fd].ssl; @@ -3904,13 +3909,10 @@ ConnStateData::httpsPeeked(Comm::ConnectionPointer serverConnection) // only when bumping CONNECT which uses a host name. if (intendedDest.IsAnyAddr()) sslCommonName = sslConnectHostOrIp; - else if (bumpServerCert.get()) - sslCommonName = Ssl::CommonHostName(bumpServerCert.get()); + else if (sslServerBump->serverCert.get()) + sslCommonName = Ssl::CommonHostName(sslServerBump->serverCert.get()); } - if (httpsPeeker.valid()) - httpsPeeker->noteHttpsPeeked(serverConnection); - httpsPeeker.clear(); getSslContextStart(); } @@ -4206,6 +4208,7 @@ ConnStateData::ConnStateData() : AsyncJob("ConnStateData"), #if USE_SSL switchedToHttps_(false), + sslServerBump(NULL), #endif stoppedSending_(NULL), stoppedReceiving_(NULL) diff --git a/src/client_side.h b/src/client_side.h index 78626e1a7b..62a0d2ab9b 100644 --- a/src/client_side.h +++ b/src/client_side.h @@ -165,7 +165,7 @@ private: class ConnectionDetail; #if USE_SSL namespace Ssl { - class ServerPeeker; + class ServerBump; } #endif /** @@ -324,7 +324,7 @@ public: void quitAfterError(HttpRequest *request); // meant to be private #if USE_SSL - /// called by Ssl::ServerPeeker when it is done bumping the server + /// called by FwdState when it is done bumping the server void httpsPeeked(Comm::ConnectionPointer serverConnection); /// Start to create dynamic SSL_CTX for host or uses static port SSL context. @@ -342,11 +342,7 @@ public: void switchToHttps(const char *host, const int port); bool switchedToHttps() const { return switchedToHttps_; } - /// Holds the squid error reply in the case of bump server first error - StoreEntry *bumpServerFirstErrorEntry() const {return bumpErrorEntry;} - void setBumpServerCert(X509 *serverCert) {bumpServerCert.reset(serverCert);} - X509 *getBumpServerCert() {return bumpServerCert.get();} - void setBumpSslErrorList(Ssl::Errors *errNoList) {bumpSslErrorNoList = cbdataReference(errNoList);} + Ssl::ServerBump *serverBump() {return sslServerBump;} /// Fill the certAdaptParams with the required data for certificate adaptation /// and create the key for storing/retrieve the certificate to/from the cache void buildSslCertGenerationParams(Ssl::CertificateProperties &certProperties); @@ -379,11 +375,8 @@ private: String sslCommonName; ///< CN name for SSL certificate generation String sslBumpCertKey; ///< Key to use to store/retrieve generated certificate - /// a job that connects to the HTTPS server to get its SSL certificate - CbcPointer httpsPeeker; - StoreEntry *bumpErrorEntry; - Ssl::X509_Pointer bumpServerCert; - Ssl::Errors *bumpSslErrorNoList; ///< The list of SSL certificate errors which ignored + /// HTTPS server cert. fetching state for bump-ssl-server-first + Ssl::ServerBump *sslServerBump; Ssl::CertSignAlgorithm signAlgorithm; ///< The signing algorithm to use #endif diff --git a/src/forward.cc b/src/forward.cc index 9de8f4c3ac..68b673226e 100644 --- a/src/forward.cc +++ b/src/forward.cc @@ -59,7 +59,7 @@ #if USE_SSL #include "ssl/support.h" #include "ssl/ErrorDetail.h" -#include "ssl/ServerPeeker.h" +#include "ssl/ServerBump.h" #endif static PSC fwdPeerSelectionCompleteWrapper; @@ -670,11 +670,13 @@ FwdState::negotiateSSL(int fd) if (request->clientConnectionManager.valid()) { // Get the server certificate from ErrorDetail object and store it // to connection manager - request->clientConnectionManager->setBumpServerCert(X509_dup(errDetails->peerCert())); + if (Ssl::ServerBump *serverBump = request->clientConnectionManager->serverBump()) { + serverBump->serverCert.resetAndLock(errDetails->peerCert()); // if there is a list of ssl errors, pass it to connection manager - if (Ssl::Errors *errNoList = static_cast(SSL_get_ex_data(ssl, ssl_ex_index_ssl_error_sslerrno))) - request->clientConnectionManager->setBumpSslErrorList(errNoList); + if (Ssl::Errors *errNoList = static_cast(SSL_get_ex_data(ssl, ssl_ex_index_ssl_error_sslerrno))) + serverBump->bumpSslErrorNoList = cbdataReference(errNoList); + } } if (request->flags.sslPeek) { @@ -701,9 +703,12 @@ FwdState::negotiateSSL(int fd) } if (request->clientConnectionManager.valid()) { - request->clientConnectionManager->setBumpServerCert(SSL_get_peer_certificate(ssl)); - if (Ssl::Errors *errNoList = static_cast(SSL_get_ex_data(ssl, ssl_ex_index_ssl_error_sslerrno))) - request->clientConnectionManager->setBumpSslErrorList(errNoList); + if (Ssl::ServerBump *serverBump = request->clientConnectionManager->serverBump()) { + serverBump->serverCert.reset(SSL_get_peer_certificate(ssl)); + + if (Ssl::Errors *errNoList = static_cast(SSL_get_ex_data(ssl, ssl_ex_index_ssl_error_sslerrno))) + serverBump->bumpSslErrorNoList = cbdataReference(errNoList); + } } if (serverConnection()->getPeer() && !SSL_session_reused(ssl)) { @@ -785,7 +790,9 @@ FwdState::initiateSSL() // store peeked cert to check SQUID_X509_V_ERR_CERT_CHANGE X509 *peeked_cert; - if (request->clientConnectionManager.valid() && (peeked_cert = request->clientConnectionManager->getBumpServerCert())) { + if (request->clientConnectionManager.valid() && + request->clientConnectionManager->serverBump() && + (peeked_cert = request->clientConnectionManager->serverBump()->serverCert.get())) { CRYPTO_add(&(peeked_cert->references),1,CRYPTO_LOCK_X509); SSL_set_ex_data(ssl, ssl_ex_index_ssl_peeked_cert, peeked_cert); } diff --git a/src/ssl/Makefile.am b/src/ssl/Makefile.am index 4cde18b11e..519c75f6be 100644 --- a/src/ssl/Makefile.am +++ b/src/ssl/Makefile.am @@ -30,8 +30,8 @@ libsslsquid_la_SOURCES = \ ErrorDetail.h \ ErrorDetailManager.cc \ ErrorDetailManager.h \ - ServerPeeker.cc \ - ServerPeeker.h \ + ServerBump.cc \ + ServerBump.h \ support.cc \ support.h diff --git a/src/ssl/ServerBump.cc b/src/ssl/ServerBump.cc new file mode 100644 index 0000000000..b2f85b3d2a --- /dev/null +++ b/src/ssl/ServerBump.cc @@ -0,0 +1,41 @@ +/* + * $Id$ + * + * DEBUG: section 33 Client-side Routines + * + */ + +#include "squid.h" + +#include "client_side.h" +#include "forward.h" +#include "ssl/ServerBump.h" +#include "Store.h" + + +CBDATA_NAMESPACED_CLASS_INIT(Ssl, ServerBump); + + +Ssl::ServerBump::ServerBump(HttpRequest *fakeRequest): + request(fakeRequest), + bumpSslErrorNoList(NULL) +{ + debugs(33, 4, HERE << "will peek at " << request->GetHost() << ':' << request->port); + const char *uri = urlCanonical(request); + entry = storeCreateEntry(uri, uri, request->flags, request->method); + // We do not need to be a client because the error contents will be used + // later, but an entry without any client will trim all its contents away. + sc = storeClientListAdd(entry, this); +} + +Ssl::ServerBump::~ServerBump() +{ + debugs(33, 4, HERE << "destroying"); + if (entry) { + debugs(33, 4, HERE << *entry); + storeUnregister(sc, entry, this); + entry->unlock(); + } + cbdataReferenceDone(bumpSslErrorNoList); +} + diff --git a/src/ssl/ServerBump.h b/src/ssl/ServerBump.h new file mode 100644 index 0000000000..af41bfcdc2 --- /dev/null +++ b/src/ssl/ServerBump.h @@ -0,0 +1,38 @@ +#ifndef _SQUID_SSL_PEEKER_H +#define _SQUID_SSL_PEEKER_H + +#include "base/AsyncJob.h" +#include "base/CbcPointer.h" +#include "comm/forward.h" +#include "HttpRequest.h" +#include "ip/Address.h" + +class ConnStateData; + +namespace Ssl +{ + +/** + \ingroup ServerProtocolSSLAPI + * Used to store bump-server-first related informations + */ +class ServerBump +{ +public: + explicit ServerBump(HttpRequest *fakeRequest); + ~ServerBump(); + /// faked, minimal request; required by server-side API + HttpRequest::Pointer request; + StoreEntry *entry; ///< for receiving Squid-generated error messages + Ssl::X509_Pointer serverCert; ///< HTTPS server certificate + Ssl::Errors *bumpSslErrorNoList; ///< The list of SSL certificate errors which ignored + +private: + store_client *sc; ///< dummy client to prevent entry trimming + + CBDATA_CLASS2(ServerBump); +}; + +} // namespace Ssl + +#endif diff --git a/src/ssl/ServerPeeker.cc b/src/ssl/ServerPeeker.cc deleted file mode 100644 index ce490c9b0a..0000000000 --- a/src/ssl/ServerPeeker.cc +++ /dev/null @@ -1,71 +0,0 @@ -/* - * $Id$ - * - * DEBUG: section 33 Client-side Routines - * - */ - -#include "squid.h" - -#include "client_side.h" -#include "forward.h" -#include "ssl/ServerPeeker.h" -#include "Store.h" - - -CBDATA_NAMESPACED_CLASS_INIT(Ssl, ServerPeeker); - - -Ssl::ServerPeeker::ServerPeeker(ConnStateData *anInitiator, - const char *host, const int port): - AsyncJob("Ssl::ServerPeeker"), - initiator(anInitiator), - clientConnection(anInitiator->clientConnection), - request(new HttpRequest), - entry(NULL), - sc(NULL) -{ - debugs(33, 4, HERE << "will peek at " << host << ':' << port); - request->flags.sslPeek = 1; - request->SetHost(host); - request->port = port; - request->protocol = AnyP::PROTO_HTTPS; - request->clientConnectionManager = initiator; - const char *uri = urlCanonical(request); - entry = storeCreateEntry(uri, uri, request->flags, request->method); - // We do not need to be a client because the error contents will be used - // later, but an entry without any client will trim all its contents away. - sc = storeClientListAdd(entry, this); -} - -Ssl::ServerPeeker::~ServerPeeker() -{ - if (entry) { - debugs(33, 4, HERE << "stopped peeking via " << *entry); - storeUnregister(sc, entry, this); - entry->unlock(); - } -} - -void -Ssl::ServerPeeker::start() -{ - FwdState::fwdStart(clientConnection, entry, request); -} - -void Ssl::ServerPeeker::noteHttpsPeeked(Comm::ConnectionPointer &serverConnection) -{ - assert(initiator.raw()); - initiator.clear(); // will trigger the end of the job -} - -bool -Ssl::ServerPeeker::doneAll() const -{ - return !initiator.valid() && AsyncJob::doneAll(); -} - -void -Ssl::ServerPeeker::swanSong() -{ -} diff --git a/src/ssl/ServerPeeker.h b/src/ssl/ServerPeeker.h deleted file mode 100644 index 9e44e9f77f..0000000000 --- a/src/ssl/ServerPeeker.h +++ /dev/null @@ -1,60 +0,0 @@ -#ifndef _SQUID_SSL_PEEKER_H -#define _SQUID_SSL_PEEKER_H - -#include "base/AsyncJob.h" -#include "base/CbcPointer.h" -#include "comm/forward.h" -#include "HttpRequest.h" -#include "ip/Address.h" - -class ConnStateData; - -namespace Ssl -{ - -/** - \ingroup ServerProtocolSSLAPI - * A job to facilitate connecting to the HTTPS server to learn its certificate. - * - * The Peeker job calls FwdState::fwdStart(). There are two possible outcomes: - * - * Success: FwdState calls ConnStateData which pins the establihsed connection - * for future bumped HTTP requests (TODO: and stops this job). - * - * Error: FwdState Stores the error (TODO: and this job preserves it for - * for serving to the client in response to the first bumped request). - */ -class ServerPeeker: public AsyncJob -{ -public: - typedef CbcPointer Pointer; - - explicit ServerPeeker(ConnStateData *anInitiator, const char *host, const int port); - - /* AsyncJob API */ - virtual ~ServerPeeker(); - virtual void start(); - virtual bool doneAll() const; - virtual void swanSong(); - StoreEntry *storeEntry() {return entry;} - void noteHttpsPeeked(Comm::ConnectionPointer &serverConnection); - -private: - /// connection manager waiting for peeked server info - CbcPointer initiator; - - /// client-Squid connection which triggered this job - Comm::ConnectionPointer clientConnection; - - /// faked, minimal request; required by server-side API - HttpRequest::Pointer request; - - StoreEntry *entry; ///< for receiving Squid-generated error messages - store_client *sc; ///< dummy client to prevent entry trimming - - CBDATA_CLASS2(ServerPeeker); -}; - -} // namespace Ssl - -#endif