From 7a957a936ca1fbe0f06871ab2673e7e3a49f7cb5 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Tue, 19 Jun 2012 15:51:49 -0600 Subject: [PATCH] Polished names, comments, and code formatting. --- include/CbDataList.h | 7 ++- src/ClientRequestContext.h | 4 +- src/acl/FilledChecklist.cc | 6 +-- src/acl/FilledChecklist.h | 3 +- src/acl/SslError.cc | 2 +- src/acl/SslErrorData.cc | 5 +-- src/adaptation/icap/ModXact.h | 1 + src/adaptation/icap/Xaction.h | 2 +- src/anyp/PortCfg.h | 1 + src/anyp/ProtocolType.h | 2 +- src/cf.data.depend | 2 +- src/cf.data.pre | 37 ++++++++-------- src/client_side.cc | 83 +++++++++++++++++++---------------- src/client_side.h | 4 ++ src/client_side_reply.h | 8 ++-- src/client_side_request.cc | 9 ++-- src/errorpage.cc | 3 +- src/errorpage.h | 4 +- src/forward.cc | 30 +++++++------ src/globals.h | 2 +- src/ssl/ErrorDetail.cc | 6 +-- src/ssl/ErrorDetail.h | 8 ++-- src/ssl/ServerBump.cc | 4 +- src/ssl/ServerBump.h | 5 ++- src/ssl/certificate_db.cc | 5 +-- src/ssl/crtd_message.h | 1 + src/ssl/ssl_crtd.cc | 1 + src/ssl/support.cc | 35 +++++++-------- src/ssl/support.h | 1 - 29 files changed, 150 insertions(+), 131 deletions(-) diff --git a/include/CbDataList.h b/include/CbDataList.h index 8e221e20e2..2ffa0b9822 100644 --- a/include/CbDataList.h +++ b/include/CbDataList.h @@ -44,10 +44,13 @@ public: CbDataList (C const &); ~CbDataList(); - bool push_back_unique(C const &); + /// If element is already in the list return false. + /// Otherwise, adds the element to the end of the list and returns true. + /// Exists to avoid double iteration of find() and push() combo. + bool push_back_unique(C const &element); bool find(C const &)const; bool findAndTune(C const &); - /// iterates entire list to return the last element holder + /// Iterates the entire list to return the last element holder. CbDataList *tail(); CbDataList *next; C element; diff --git a/src/ClientRequestContext.h b/src/ClientRequestContext.h index a52d88fbdf..74233cebc0 100644 --- a/src/ClientRequestContext.h +++ b/src/ClientRequestContext.h @@ -68,8 +68,8 @@ public: #if USE_SSL bool sslBumpCheckDone; #endif - ErrorState *error; - bool readNextRequest; + ErrorState *error; ///< saved error page for centralized/delayed processing + bool readNextRequest; ///< whether Squid should read after error handling private: CBDATA_CLASS(ClientRequestContext); diff --git a/src/acl/FilledChecklist.cc b/src/acl/FilledChecklist.cc index 6db77825b6..6e625f6acf 100644 --- a/src/acl/FilledChecklist.cc +++ b/src/acl/FilledChecklist.cc @@ -67,7 +67,7 @@ ACLFilledChecklist::ACLFilledChecklist() : snmp_community(NULL), #endif #if USE_SSL - sslErrorList(NULL), + sslErrors(NULL), #endif extacl_entry (NULL), conn_(NULL), @@ -98,7 +98,7 @@ ACLFilledChecklist::~ACLFilledChecklist() cbdataReferenceDone(conn_); #if USE_SSL - cbdataReferenceDone(sslErrorList); + cbdataReferenceDone(sslErrors); #endif debugs(28, 4, HERE << "ACLFilledChecklist destroyed " << this); @@ -183,7 +183,7 @@ ACLFilledChecklist::ACLFilledChecklist(const acl_access *A, HttpRequest *http_re snmp_community(NULL), #endif #if USE_SSL - sslErrorList(NULL), + sslErrors(NULL), #endif extacl_entry (NULL), conn_(NULL), diff --git a/src/acl/FilledChecklist.h b/src/acl/FilledChecklist.h index 2ef86b6c85..5c4b766881 100644 --- a/src/acl/FilledChecklist.h +++ b/src/acl/FilledChecklist.h @@ -66,7 +66,8 @@ public: #endif #if USE_SSL - Ssl::Errors *sslErrorList; + /// SSL [certificate validation] errors in undefined order + Ssl::Errors *sslErrors; #endif ExternalACLEntry *extacl_entry; diff --git a/src/acl/SslError.cc b/src/acl/SslError.cc index c81dc72741..a062c12722 100644 --- a/src/acl/SslError.cc +++ b/src/acl/SslError.cc @@ -11,7 +11,7 @@ int ACLSslErrorStrategy::match (ACLData * &data, ACLFilledChecklist *checklist) { - return data->match (checklist->sslErrorList); + return data->match (checklist->sslErrors); } ACLSslErrorStrategy * diff --git a/src/acl/SslErrorData.cc b/src/acl/SslErrorData.cc index 7933f6d1a8..12f3b88d23 100644 --- a/src/acl/SslErrorData.cc +++ b/src/acl/SslErrorData.cc @@ -24,9 +24,8 @@ ACLSslErrorData::~ACLSslErrorData() bool ACLSslErrorData::match(const Ssl::Errors *toFind) { - const Ssl::Errors * err; - for (err = toFind ; err != NULL; err = err->next ) { - if (values->findAndTune (err->element)) + for (const Ssl::Errors *err = toFind; err; err = err->next ) { + if (values->findAndTune(err->element)) return true; } return false; diff --git a/src/adaptation/icap/ModXact.h b/src/adaptation/icap/ModXact.h index 4bbe0b8573..c5cc0f628c 100644 --- a/src/adaptation/icap/ModXact.h +++ b/src/adaptation/icap/ModXact.h @@ -168,6 +168,7 @@ public: /// record error detail in the virgin request if possible virtual void detailError(int errDetail); + // Icap::Xaction API virtual void clearError(); private: diff --git a/src/adaptation/icap/Xaction.h b/src/adaptation/icap/Xaction.h index 012d98db86..8fb26a4ebd 100644 --- a/src/adaptation/icap/Xaction.h +++ b/src/adaptation/icap/Xaction.h @@ -134,7 +134,7 @@ public: // custom exception handling and end-of-call checks virtual void callException(const std::exception &e); virtual void callEnd(); - // clear the error details on retries/repeats + /// clear stored error details, if any; used for retries/repeats virtual void clearError() {} void dnsLookupDone(const ipcache_addrs *ia); diff --git a/src/anyp/PortCfg.h b/src/anyp/PortCfg.h index 1e22043f0e..032e1dc54c 100644 --- a/src/anyp/PortCfg.h +++ b/src/anyp/PortCfg.h @@ -16,6 +16,7 @@ struct PortCfg { ~PortCfg(); AnyP::PortCfg *clone() const; #if USE_SSL + /// creates, configures, and validates SSL context and related port options void configureSslServerContext(); #endif diff --git a/src/anyp/ProtocolType.h b/src/anyp/ProtocolType.h index bdafd7e8be..4f5ab3b753 100644 --- a/src/anyp/ProtocolType.h +++ b/src/anyp/ProtocolType.h @@ -29,7 +29,7 @@ typedef enum { #endif PROTO_URN, PROTO_WHOIS, - PROTO_INTERNAL, // miss on an internal object such as an icon + PROTO_INTERNAL, PROTO_ICY, PROTO_UNKNOWN, PROTO_MAX diff --git a/src/cf.data.depend b/src/cf.data.depend index 2c93520541..81b9aca438 100644 --- a/src/cf.data.depend +++ b/src/cf.data.depend @@ -69,5 +69,5 @@ wccp2_service wccp2_service_info wordlist sslproxy_ssl_bump acl -sslproxy_cert_sign acl +sslproxy_cert_sign acl sslproxy_cert_adapt acl diff --git a/src/cf.data.pre b/src/cf.data.pre index 66a56fbf60..116ea2ee8d 100644 --- a/src/cf.data.pre +++ b/src/cf.data.pre @@ -881,25 +881,24 @@ DOC_START IFDEF USE_SSL acl aclname ssl_error errorname # match against SSL certificate validation error [fast] - # For valid error names see in @DEFAULT_ERROR_DIR@/templates/error-details.txt template file - # The user aditionaly can use as error name the following error name - # shortcuts: - # [ssl::]certHasExpired: certificate "not after" field is in the past - # [ssl::]certNotYetValid: certificate "not before" field is in the - # future - # [ssl::]certDomainMismatch: The certificate CN domain does not match - # connecting host name - # [ssl::]certUntrusted: The certificate is untrusted because of an - # error says that the certificate issuer is not trusted. - # [ssl::]certSelfSigned: The certificate is self signed # - # The ssl::certHasExpired, ssl::certNotYetValid ssl::certDomainMismatch, - # ssl::certUntrusted and ssl::certSelfSigned also exists as predefined - # acl lists. + # For valid error names see in @DEFAULT_ERROR_DIR@/templates/error-details.txt + # template file. # - # NOTE: The ssl_error acl has effect only when used with - # sslproxy_cert_error, sslproxy_cert_sign and sslproxy_cert_adapt - # access lists. + # The following can be used as shortcuts for certificate properties: + # [ssl::]certHasExpired: the "not after" field is in the past + # [ssl::]certNotYetValid: the "not before" field is in the future + # [ssl::]certUntrusted: The certificate issuer is not to be trusted. + # [ssl::]certSelfSigned: The certificate is self signed. + # [ssl::]certDomainMismatch: The certificate CN domain does not + # match the name the name of the host we are connecting to. + # + # The ssl::certHasExpired, ssl::certNotYetValid, ssl::certDomainMismatch, + # ssl::certUntrusted, and ssl::certSelfSigned can also be used as + # predefined ACLs, just like the 'all' ACL. + # + # NOTE: The ssl_error ACL is only supported with sslproxy_cert_error, + # sslproxy_cert_sign, and sslproxy_cert_adapt options. ENDIF Examples: @@ -1353,7 +1352,7 @@ DOC_START Squid, and treat them as unencrypted HTTP messages, becoming the man-in-the-middle. - The "ssl_bump" option is required to fully enable + The ssl_bump option is required to fully enable bumping of CONNECT requests. Omitting the mode flag causes default forward proxy mode to be used. @@ -1569,7 +1568,7 @@ DOC_START NP: disables authentication and maybe IPv6 on the port. ssl-bump For each intercepted connection allowed by ssl_bump - ACLs, establish a secure connection with the client and with + ACLs, establish a secure connection with the client and with the server, decrypt HTTPS messages as they pass through Squid, and treat them as unencrypted HTTP messages, becoming the man-in-the-middle. diff --git a/src/client_side.cc b/src/client_side.cc index 438ce2e8c3..66913cf3c4 100644 --- a/src/client_side.cc +++ b/src/client_side.cc @@ -2471,40 +2471,45 @@ bool ConnStateData::serveDelayedError(ClientSocketContext *context) return false; assert(sslServerBump->entry); + // Did we create an error entry while processing CONNECT? if (!sslServerBump->entry->isEmpty()) { quitAfterError(http->request); - //Failed? Here we should get the error from conn and send it to client - // The error stored in ConnStateData::bumpFirstEntry, replace the - // ClientHttpRequest store entry with this. + // Get the saved error entry and send it to the client by replacing the + // ClientHttpRequest store entry with it. clientStreamNode *node = context->getClientReplyContext(); clientReplyContext *repContext = dynamic_cast(node->data.getRaw()); - assert (repContext); - debugs(33, 5, "Connection first has failed for " << http->uri << ". Respond with an error"); + assert(repContext); + debugs(33, 5, "Responding with delated error for " << http->uri); repContext->setReplyToStoreEntry(sslServerBump->entry); - /*Save the original request for logging purposes*/ + + // save the original request for logging purposes if (!context->http->al.request) context->http->al.request = HTTPMSGLOCK(http->request); - /*Get the error details from the fake request used to retrieve SSL server certificate*/ + + // Get error details from the fake certificate-peeking request. http->request->detailError(sslServerBump->request->errType, sslServerBump->request->errDetail); 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 (sslServerBump && sslServerBump->serverCert.get()) { + // In bump-server-first mode, we have not necessarily seen the intended + // server name at certificate-peeking time. Check for domain mismatch now, + // when we can extract the intended name from the bumped HTTP request. + if (sslServerBump->serverCert.get()) { HttpRequest *request = http->request; if (!Ssl::checkX509ServerValidity(sslServerBump->serverCert.get(), request->GetHost())) { - debugs(33, 2, "SQUID_X509_V_ERR_DOMAIN_MISMATCH: Certificate does not match domainname " << 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); - check.sslErrorList = new Ssl::Errors(SQUID_X509_V_ERR_DOMAIN_MISMATCH); + check.sslErrors = new Ssl::Errors(SQUID_X509_V_ERR_DOMAIN_MISMATCH); if (Comm::IsConnOpen(pinning.serverConnection)) check.fd(pinning.serverConnection->fd); - bool allowDomainMismatch = (check.fastCheck() == ACCESS_ALLOWED); - delete check.sslErrorList; - check.sslErrorList = NULL; + const bool allowDomainMismatch = + check.fastCheck() == ACCESS_ALLOWED; + delete check.sslErrors; + check.sslErrors = NULL; if (!allowDomainMismatch) { quitAfterError(request); @@ -2513,16 +2518,18 @@ bool ConnStateData::serveDelayedError(ClientSocketContext *context) clientReplyContext *repContext = dynamic_cast(node->data.getRaw()); assert (repContext); - // Fill the server ip address and server hostname for use with ErrorState + // Fill the server IP and hostname for error page generation. 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); - err->src_addr = clientConnection->remote; - Ssl::ErrorDetail *errDetail = new Ssl::ErrorDetail( SQUID_X509_V_ERR_DOMAIN_MISMATCH, sslServerBump->serverCert.get(), NULL); + Ssl::ErrorDetail *errDetail = new Ssl::ErrorDetail( + SQUID_X509_V_ERR_DOMAIN_MISMATCH, + sslServerBump->serverCert.get(), NULL); err->detail = errDetail; - /*Save the original request for logging purposes*/ + // Save the original request for logging purposes. if (!context->http->al.request) context->http->al.request = HTTPMSGLOCK(request); repContext->setReplyToError(request->method, err); @@ -2535,7 +2542,7 @@ bool ConnStateData::serveDelayedError(ClientSocketContext *context) return false; } -#endif //USE_SSL +#endif // USE_SSL static void clientProcessRequest(ConnStateData *conn, HttpParser *hp, ClientSocketContext *context, const HttpRequestMethod& method, HttpVersion http_ver) @@ -3542,9 +3549,9 @@ httpsEstablish(ConnStateData *connState, SSL_CTX *sslContext, Ssl::BumpMode bum if (sslContext && !(ssl = httpsCreate(details, sslContext))) return; - typedef CommCbMemFunT TimeoutDialer; - AsyncCall::Pointer timeoutCall = JobCallback(33, 5, - TimeoutDialer, connState, ConnStateData::requestTimeout); + typedef CommCbMemFunT TimeoutDialer; + AsyncCall::Pointer timeoutCall = JobCallback(33, 5, TimeoutDialer, + connState, ConnStateData::requestTimeout); commSetConnTimeout(details, Config.Timeout.request, timeoutCall); if (ssl) @@ -3570,22 +3577,22 @@ httpsEstablish(ConnStateData *connState, SSL_CTX *sslContext, Ssl::BumpMode bum /** * A callback function to use with the ACLFilledChecklist callback. - * In the case of ACCES_ALLOWED answer initializes an ssl bumped connection, - * else revert the connection to tunnel mode. + * In the case of ACCES_ALLOWED answer initializes a bumped SSL connection, + * else reverts the connection to tunnel mode. */ static void httpsSslBumpAccessCheckDone(allow_t answer, void *data) { ConnStateData *connState = (ConnStateData *) data; - //if connection closed/closing just return. + // if the connection is closed or closing, just return. if (!connState->isOpen()) return; - // Require both a match and a positive mode to work around exceptional + // Require both a match and a positive bump mode to work around exceptional // cases where ACL code may return ACCESS_ALLOWED with zero answer.kind. if (answer == ACCESS_ALLOWED && answer.kind != Ssl::bumpNone) { - debugs(33, 2, HERE << " sslBump done data: " << connState->clientConnection); + debugs(33, 2, HERE << "sslBump done data: " << connState->clientConnection); httpsEstablish(connState, NULL, (Ssl::BumpMode)answer.kind); } else { // fake a CONNECT request to force connState to tunnel @@ -3702,8 +3709,10 @@ void ConnStateData::buildSslCertGenerationParams(Ssl::CertificateProperties &cer return; } - // In the case of error while connecting to secure server, use a fake trusted certificate, - // with no mimicked fields and no adaptation algorithms + // In case of an error while connecting to the secure server, use a fake + // trusted certificate, with no mimicked fields and no adaptation + // algorithms. There is nothing we can mimic so we want to minimize the + // number of warnings the user will have to see to get to the error page. assert(sslServerBump->entry); if (sslServerBump->entry->isEmpty()) { if (X509 *mimicCert = sslServerBump->serverCert.get()) @@ -3712,10 +3721,10 @@ void ConnStateData::buildSslCertGenerationParams(Ssl::CertificateProperties &cer ACLFilledChecklist checklist(NULL, sslServerBump->request, clientConnection != NULL ? clientConnection->rfc931 : dash_str); checklist.conn(this); - checklist.sslErrorList = cbdataReference(sslServerBump->bumpSslErrorNoList); + checklist.sslErrors = cbdataReference(sslServerBump->sslErrors); for (sslproxy_cert_adapt *ca = Config.ssl_client.cert_adapt; ca != NULL; ca = ca->next) { - // If the algorithm already set ignore. + // If the algorithm already set, then ignore it. if ((ca->alg == Ssl::algSetCommonName && certProperties.setCommonName) || (ca->alg == Ssl::algSetValidAfter && certProperties.setValidAfter) || (ca->alg == Ssl::algSetValidBefore && certProperties.setValidBefore) ) @@ -3725,8 +3734,8 @@ void ConnStateData::buildSslCertGenerationParams(Ssl::CertificateProperties &cer const char *alg = Ssl::CertAdaptAlgorithmStr[ca->alg]; const char *param = ca->param; - // if not param defined for Common Name adaptation use hostname from - // the CONNECT request + // For parameterless CN adaptation, use hostname from the + // CONNECT request. if (ca->alg == Ssl::algSetCommonName) { if (!param) param = sslConnectHostOrIp.termedBuf(); @@ -4398,8 +4407,8 @@ ConnStateData::sendControlMsg(HttpControlMsg msg) void ConnStateData::clientPinnedConnectionClosed(const CommCloseCbParams &io) { - // it might be possible for FwdState to repin a failed connection sooner - // than this close callback is called for the failed connection + // FwdState might repin a failed connection sooner than this close + // callback is called for the failed connection. if (pinning.serverConnection == io.conn) { pinning.closeHandler = NULL; // Comm unregisters handlers before calling unpinConnection(); @@ -4416,7 +4425,7 @@ ConnStateData::pinConnection(const Comm::ConnectionPointer &pinServer, HttpReque return; } - unpinConnection(); // closes pinned connection, if any, and resets fields. + unpinConnection(); // closes pinned connection, if any, and resets fields pinning.serverConnection = pinServer; diff --git a/src/client_side.h b/src/client_side.h index d4c34765e6..845bc41278 100644 --- a/src/client_side.h +++ b/src/client_side.h @@ -347,6 +347,10 @@ public: /// 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); + /// Called when the client sends the first request on a bumped connection. + /// Returns false if no [delayed] error should be written to the client. + /// Otherwise, writes the error to the client and returns true. Also checks + /// for SQUID_X509_V_ERR_DOMAIN_MISMATCH on bumped requests. bool serveDelayedError(ClientSocketContext *context); #else bool switchedToHttps() const { return false; } diff --git a/src/client_side_reply.h b/src/client_side_reply.h index 7eac9ea302..26d9d11d05 100644 --- a/src/client_side_reply.h +++ b/src/client_side_reply.h @@ -71,15 +71,17 @@ public: void identifyFoundObject(StoreEntry *entry); int storeOKTransferDone() const; int storeNotOKTransferDone() const; - /// Replaces the store entry with the given and awaiting the client side to read it - void setReplyToStoreEntry(StoreEntry *entry); + /// replaces current response store entry with the given one + void setReplyToStoreEntry(StoreEntry *e); + /// builds error using clientBuildError() and calls setReplyToError() below void setReplyToError(err_type, http_status, const HttpRequestMethod&, char const *, Ip::Address &, HttpRequest *, const char *, #if USE_AUTH Auth::UserRequest::Pointer); #else void * unused); #endif - void setReplyToError(const HttpRequestMethod& method, ErrorState *errstate); + /// creates a store entry for the reply and appends err to it + void setReplyToError(const HttpRequestMethod& method, ErrorState *err); void createStoreEntry(const HttpRequestMethod& m, request_flags flags); void removeStoreReference(store_client ** scp, StoreEntry ** ep); void removeClientStoreReference(store_client **scp, ClientHttpRequest *http); diff --git a/src/client_side_request.cc b/src/client_side_request.cc index 26a74e2668..8fb7dacd66 100644 --- a/src/client_side_request.cc +++ b/src/client_side_request.cc @@ -1278,7 +1278,7 @@ bool ClientRequestContext::sslBumpAccessCheck() { if (http->request->method == METHOD_CONNECT && - !http->request->flags.spoof_client_ip && // is not a fake ssl-bumped request from a https port + !http->request->flags.spoof_client_ip && // not a fake bumped request from an https port Config.accessList.ssl_bump && http->getConn()->port->sslBump) { debugs(85, 5, HERE << "SslBump possible, checking ACL"); @@ -1591,8 +1591,8 @@ ClientHttpRequest::doCallouts() } #if USE_SSL - // We need to check for SSL bump even if the calloutContext->error is set - // because we are handling with different way the error inside SSL-bump + // We need to check for SslBump even if the calloutContext->error is set + // because bumping may require delaying the error until after CONNECT. if (!calloutContext->sslBumpCheckDone) { calloutContext->sslBumpCheckDone = true; if (calloutContext->sslBumpAccessCheck()) @@ -1616,7 +1616,7 @@ ClientHttpRequest::doCallouts() } else #endif { - // send the error to the client + // send the error to the client now clientStreamNode *node = (clientStreamNode *)client_stream.tail->prev->data; clientReplyContext *repContext = dynamic_cast(node->data.getRaw()); assert (repContext); @@ -1628,7 +1628,6 @@ ClientHttpRequest::doCallouts() node = (clientStreamNode *)client_stream.tail->data; clientStreamRead(node, this, node->readBuffer); e->unlock(); - // Stop here. return; } } diff --git a/src/errorpage.cc b/src/errorpage.cc index ef6e715482..0f90ad3509 100644 --- a/src/errorpage.cc +++ b/src/errorpage.cc @@ -1219,7 +1219,8 @@ ErrorState::BuildHttpReply() /* do not memBufClean() or delete the content, it was absorbed by httpBody */ } - /* Make sure error codes get back to the client side for logging and error tracking */ + // Make sure error codes get back to the client side for logging and + // error tracking. if (request) { int edc = ERR_DETAIL_NONE; // error detail code #if USE_SSL diff --git a/src/errorpage.h b/src/errorpage.h index 2861a20da2..5253106860 100644 --- a/src/errorpage.h +++ b/src/errorpage.h @@ -105,9 +105,7 @@ public: */ HttpReply *BuildHttpReply(void); - /** - * Sets the error details - */ + /// set error type-specific detail code void detailError(int detailCode); private: diff --git a/src/forward.cc b/src/forward.cc index cac6531a7f..ff2b36414d 100644 --- a/src/forward.cc +++ b/src/forward.cc @@ -649,10 +649,10 @@ FwdState::negotiateSSL(int fd) if (errFromFailure != NULL) { // The errFromFailure is attached to the ssl object // and will be released when ssl object destroyed. - // Copy errFromFailure to a new Ssl::ErrorDetail object + // Copy errFromFailure to a new Ssl::ErrorDetail object. errDetails = new Ssl::ErrorDetail(*errFromFailure); } else { - // server_cert can be be NULL + // server_cert can be NULL here X509 *server_cert = SSL_get_peer_certificate(ssl); errDetails = new Ssl::ErrorDetail(SQUID_ERR_SSL_HANDSHAKE, server_cert, NULL); X509_free(server_cert); @@ -662,22 +662,21 @@ FwdState::negotiateSSL(int fd) errDetails->setLibError(ssl_lib_error); if (request->clientConnectionManager.valid()) { - // Get the server certificate from ErrorDetail object and store it - // to connection manager + // remember the server certificate from the ErrorDetail object 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))) - serverBump->bumpSslErrorNoList = cbdataReference(errNoList); - } + // remember validation errors, if any + if (Ssl::Errors *errs = static_cast(SSL_get_ex_data(ssl, ssl_ex_index_ssl_errors))) + serverBump->sslErrors = cbdataReference(errs); + } } + // For intercepted connections, set the host name to the server + // certificate CN. Otherwise, we just hope that CONNECT is using + // a user-entered address (a host name or a user-entered IP). const bool isConnectRequest = !request->clientConnectionManager->port->spoof_client_ip && !request->clientConnectionManager->port->intercepted; - // For intercepted connections, set host name to server - // certificate CN. Otherwise, we hope that CONNECT is using - // a user-entered address (a host name or a user-entered IP). if (request->flags.sslPeek && !isConnectRequest) { if (X509 *srvX509 = errDetails->peerCert()) { if (const char *name = Ssl::CommonHostName(srvX509)) { @@ -686,6 +685,7 @@ FwdState::negotiateSSL(int fd) } } } + ErrorState *const anErr = makeConnectingError(ERR_SECURE_CONNECT_FAIL); anErr->xerrno = sysErrNo; anErr->detail = errDetails; @@ -701,11 +701,13 @@ FwdState::negotiateSSL(int fd) } if (request->clientConnectionManager.valid()) { + // remember the server certificate from the ErrorDetail object 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); + // remember validation errors, if any + if (Ssl::Errors *errs = static_cast(SSL_get_ex_data(ssl, ssl_ex_index_ssl_errors))) + serverBump->sslErrors = cbdataReference(errs); } } @@ -776,7 +778,7 @@ FwdState::initiateSSL() SSL_set_ex_data(ssl, ssl_ex_index_server, (void*)hostname); // Use SNI TLS extension only when we connect directly - // to the origin server and we know the server host name + // to the origin server and we know the server host name. if (!hostnameIsIp) Ssl::setClientSNI(ssl, hostname); } diff --git a/src/globals.h b/src/globals.h index 8d8222429e..4463ee9079 100644 --- a/src/globals.h +++ b/src/globals.h @@ -153,7 +153,7 @@ extern "C" { extern int ssl_ex_index_cert_error_check; /* -1 */ extern int ssl_ex_index_ssl_error_detail; /* -1 */ extern int ssl_ex_index_ssl_peeked_cert; /* -1 */ - extern int ssl_ex_index_ssl_error_sslerrno; /* -1 */ + extern int ssl_ex_index_ssl_errors; /* -1 */ extern const char *external_acl_message; /* NULL */ extern int opt_send_signal; /* -1 */ diff --git a/src/ssl/ErrorDetail.cc b/src/ssl/ErrorDetail.cc index 2c8c4090c5..6c36fb511a 100644 --- a/src/ssl/ErrorDetail.cc +++ b/src/ssl/ErrorDetail.cc @@ -109,7 +109,7 @@ static const Ssl::ssl_error_t certSelfSigned[] = {X509_V_ERR_DEPTH_ZERO_SELF_SIG // The list of error name shortcuts for use with ssl_error acls. // The keys without the "ssl::" scope prefix allow shorter error // names within the SSL options scope. This is easier than -// carefully stripping the scope prefix in Ssl::ParseErrorString() +// carefully stripping the scope prefix in Ssl::ParseErrorString(). static SslErrorAlias TheSslErrorShortcutsArray[] = { {"ssl::certHasExpired", hasExpired}, {"certHasExpired", hasExpired}, @@ -124,7 +124,7 @@ static SslErrorAlias TheSslErrorShortcutsArray[] = { {NULL, NULL} }; -//Use std::map to optimize search +// Use std::map to optimize search. typedef std::map SslErrorShortcuts; SslErrorShortcuts TheSslErrorShortcuts; @@ -340,7 +340,7 @@ const char *Ssl::ErrorDetail::err_lib_error() const } /** - * It converts the code to a string value. Supported formating codes are: + * Converts the code to a string value. Supported formating codes are: * * Error meta information: * %err_name: The name of a high-level SSL error (e.g., X509_V_ERR_*) diff --git a/src/ssl/ErrorDetail.h b/src/ssl/ErrorDetail.h index b26a87a66e..9a4cc0ae53 100644 --- a/src/ssl/ErrorDetail.h +++ b/src/ssl/ErrorDetail.h @@ -15,10 +15,10 @@ namespace Ssl { /** \ingroup ServerProtocolSSLAPI - * The Ssl::Errors representation of the error described by "name". - * The result may be a single element of a list of errors, and needs to be + * Converts user-friendly error "name" into an Ssl::Errors list. + * The resulting list may have one or more elements, and needs to be * released by the caller. - * This function also parses numeric arguments. + * This function can handle numeric error numbers as well as names. */ Ssl::Errors *ParseErrorString(const char *name); @@ -59,7 +59,7 @@ public: ssl_error_t errorNo() const {return error_no;} ///Sets the low-level error returned by OpenSSL ERR_get_error() void setLibError(unsigned long lib_err_no) {lib_error_no = lib_err_no;} - ///The peer certificate + /// the peer certificate X509 *peerCert() { return peer_cert.get(); } /// peer or intermediate certificate that failed validation X509 *brokenCert() {return broken_cert.get(); } diff --git a/src/ssl/ServerBump.cc b/src/ssl/ServerBump.cc index bac940f24a..95ba24d1d6 100644 --- a/src/ssl/ServerBump.cc +++ b/src/ssl/ServerBump.cc @@ -18,7 +18,7 @@ CBDATA_NAMESPACED_CLASS_INIT(Ssl, ServerBump); Ssl::ServerBump::ServerBump(HttpRequest *fakeRequest, StoreEntry *e): request(fakeRequest), - bumpSslErrorNoList(NULL) + sslErrors(NULL) { debugs(33, 4, HERE << "will peek at " << request->GetHost() << ':' << request->port); const char *uri = urlCanonical(request); @@ -40,6 +40,6 @@ Ssl::ServerBump::~ServerBump() storeUnregister(sc, entry, this); entry->unlock(); } - cbdataReferenceDone(bumpSslErrorNoList); + cbdataReferenceDone(sslErrors); } diff --git a/src/ssl/ServerBump.h b/src/ssl/ServerBump.h index df62acae61..acaf91b0b1 100644 --- a/src/ssl/ServerBump.h +++ b/src/ssl/ServerBump.h @@ -14,18 +14,19 @@ namespace Ssl /** \ingroup ServerProtocolSSLAPI - * Used to store bump-server-first related informations + * Maintains bump-server-first related information. */ class ServerBump { public: explicit ServerBump(HttpRequest *fakeRequest, StoreEntry *e = NULL); ~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 + Ssl::Errors *sslErrors; ///< SSL [certificate validation] errors private: store_client *sc; ///< dummy client to prevent entry trimming diff --git a/src/ssl/certificate_db.cc b/src/ssl/certificate_db.cc index 708d2f07f0..618f976208 100644 --- a/src/ssl/certificate_db.cc +++ b/src/ssl/certificate_db.cc @@ -229,9 +229,8 @@ bool Ssl::CertificateDb::addCertAndPrivateKey(Ssl::X509_Pointer & cert, Ssl::EVP } row.setValue(cnlSerial, serial_string.c_str()); char ** rrow = TXT_DB_get_by_index(db.get(), cnlSerial, row.getRow()); - // We are creating certificates with unique serial number. If the serial - // number found in the database, means that the certificate already exist - // in the database + // We are creating certificates with unique serial numbers. If the serial + // number is found in the database, the same certificate is already stored. if (rrow != NULL) return true; diff --git a/src/ssl/crtd_message.h b/src/ssl/crtd_message.h index 7891ace376..cd787a2796 100644 --- a/src/ssl/crtd_message.h +++ b/src/ssl/crtd_message.h @@ -64,6 +64,7 @@ public: */ void composeBody(BodyParams const & map, std::string const & other_part); + /// orchestrates entire request parsing bool parseRequest(Ssl::CertificateProperties &, std::string &error); void composeRequest(Ssl::CertificateProperties const &); // throws diff --git a/src/ssl/ssl_crtd.cc b/src/ssl/ssl_crtd.cc index 550d7276eb..9a8abe5830 100644 --- a/src/ssl/ssl_crtd.cc +++ b/src/ssl/ssl_crtd.cc @@ -207,6 +207,7 @@ static bool proccessNewRequest(Ssl::CrtdMessage & request_message, std::string c throw std::runtime_error("Error while parsing the crtd request: " + error); Ssl::CertificateDb db(db_path, max_db_size, fs_block_size); + Ssl::X509_Pointer cert; Ssl::EVP_PKEY_Pointer pkey; std::string &cert_subject = certProperties.dbKey(); diff --git a/src/ssl/support.cc b/src/ssl/support.cc index 011db2ae46..0b7d8c960d 100644 --- a/src/ssl/support.cc +++ b/src/ssl/support.cc @@ -244,7 +244,7 @@ ssl_verify_cb(int ok, X509_STORE_CTX * ctx) } if (ok && peeked_cert) { - /*Check if the already peeked certificate match the new one*/ + // Check whether the already peeked certificate matches the new one. if (X509_cmp(peer_cert, peeked_cert) != 0) { debugs(83, 2, "SQUID_X509_V_ERR_CERT_CHANGE: Certificate " << buffer << " does not match peeked certificate"); ok = 0; @@ -253,17 +253,17 @@ ssl_verify_cb(int ok, X509_STORE_CTX * ctx) } if (!ok) { - Ssl::Errors *errNoList = static_cast(SSL_get_ex_data(ssl, ssl_ex_index_ssl_error_sslerrno)); - if (!errNoList) { - errNoList = new Ssl::Errors(error_no); - if (!SSL_set_ex_data(ssl, ssl_ex_index_ssl_error_sslerrno, (void *)errNoList)) { + Ssl::Errors *errs = static_cast(SSL_get_ex_data(ssl, ssl_ex_index_ssl_errors)); + if (!errs) { + errs = new Ssl::Errors(error_no); + if (!SSL_set_ex_data(ssl, ssl_ex_index_ssl_errors, (void *)errs)) { debugs(83, 2, "Failed to set ssl error_no in ssl_verify_cb: Certificate " << buffer); - delete errNoList; - errNoList = NULL; + delete errs; + errs = NULL; } } - else // Append the err no to the SSL errors lists. - errNoList->push_back_unique(error_no); + else // remember another error number + errs->push_back_unique(error_no); if (const char *err_descr = Ssl::GetErrorDescr(error_no)) debugs(83, 5, err_descr << ": " << buffer); @@ -272,17 +272,16 @@ ssl_verify_cb(int ok, X509_STORE_CTX * ctx) if (check) { ACLFilledChecklist *filledCheck = Filled(check); - assert(filledCheck->sslErrorList == NULL); - filledCheck->sslErrorList = new Ssl::Errors(error_no); + assert(!filledCheck->sslErrors); + filledCheck->sslErrors = new Ssl::Errors(error_no); if (check->fastCheck() == ACCESS_ALLOWED) { debugs(83, 3, "bypassing SSL error " << error_no << " in " << buffer); ok = 1; } else { debugs(83, 5, "confirming SSL error " << error_no); } - // Delete the ssl error list - delete filledCheck->sslErrorList; - filledCheck->sslErrorList = NULL; + delete filledCheck->sslErrors; + filledCheck->sslErrors = NULL; } } @@ -615,11 +614,11 @@ ssl_free_ErrorDetail(void *, void *ptr, CRYPTO_EX_DATA *, } static void -ssl_free_SslErrNoList(void *, void *ptr, CRYPTO_EX_DATA *, +ssl_free_SslErrors(void *, void *ptr, CRYPTO_EX_DATA *, int, long, void *) { - Ssl::Errors *errNo = static_cast (ptr); - delete errNo; + Ssl::Errors *errs = static_cast (ptr); + delete errs; } // "free" function for X509 certificates @@ -671,7 +670,7 @@ ssl_initialize(void) ssl_ex_index_cert_error_check = SSL_get_ex_new_index(0, (void *) "cert_error_check", NULL, &ssl_dupAclChecklist, &ssl_freeAclChecklist); ssl_ex_index_ssl_error_detail = SSL_get_ex_new_index(0, (void *) "ssl_error_detail", NULL, NULL, &ssl_free_ErrorDetail); ssl_ex_index_ssl_peeked_cert = SSL_get_ex_new_index(0, (void *) "ssl_peeked_cert", NULL, NULL, &ssl_free_X509); - ssl_ex_index_ssl_error_sslerrno = SSL_get_ex_new_index(0, (void *) "ssl_error_sslerrno", NULL, NULL, &ssl_free_SslErrNoList); + ssl_ex_index_ssl_errors = SSL_get_ex_new_index(0, (void *) "ssl_errors", NULL, NULL, &ssl_free_SslErrors); } /// \ingroup ServerProtocolSSLInternal diff --git a/src/ssl/support.h b/src/ssl/support.h index 4144042077..76a437d19a 100644 --- a/src/ssl/support.h +++ b/src/ssl/support.h @@ -210,7 +210,6 @@ int asn1timeToString(ASN1_TIME *tm, char *buf, int len); \return true if SNI set false otherwise */ bool setClientSNI(SSL *ssl, const char *fqdn); - } //namespace Ssl #if _SQUID_MSWIN_ -- 2.39.2