From: Alex Rousskov Date: Wed, 1 Feb 2012 05:13:24 +0000 (-0700) Subject: Replaced PROTO_SSL_PEEK with request_flags::sslPeek and disabled server SNI X-Git-Tag: BumpSslServerFirst.take04~2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=2c065fc8a4f92da5702c5273e937e6442e1f678e;p=thirdparty%2Fsquid.git Replaced PROTO_SSL_PEEK with request_flags::sslPeek and disabled server SNI for bump-server-first connections. While PROTO_SSL_PEEK was a safer design option because requests with the "wrong" protocol scheme would be less likely to leave Squid, it required all error-generation code to replace the protocol with PROTO_HTTPS so that error make more sense to end users. We no longer have to do that. The server-side SNI for bump-server-first connections has to be disabled because bump-server-first code does not yet know the true intended server name (even for those CONNECT requests that have server name, it would be a little risky to use CONNECT info for SNI). That name could be eventually obtained from the client before we peek at the server certificate but that work is outside this project scope. --- diff --git a/src/anyp/ProtocolType.h b/src/anyp/ProtocolType.h index a3b4b2c610..7cedcb019f 100644 --- a/src/anyp/ProtocolType.h +++ b/src/anyp/ProtocolType.h @@ -29,9 +29,6 @@ typedef enum { PROTO_WHOIS, PROTO_INTERNAL, // miss on an internal object such as an icon PROTO_ICY, -#if USE_SSL - PROTO_SSL_PEEK, // an internal request to peek at an HTTPS server -#endif PROTO_UNKNOWN, PROTO_MAX } ProtocolType; diff --git a/src/forward.cc b/src/forward.cc index 587c72d8a6..5ed3cc6a99 100644 --- a/src/forward.cc +++ b/src/forward.cc @@ -338,7 +338,7 @@ FwdState::startConnectionOrFail() fail(anErr); } // else use actual error from last connection attempt #if USE_SSL - if (request->protocol == AnyP::PROTO_SSL_PEEK && request->clientConnectionManager.valid()) { + if (request->flags.sslPeek && request->clientConnectionManager.valid()) { errorAppendEntry(entry, err); // will free err err = NULL; CallJobHere1(17, 4, request->clientConnectionManager, ConnStateData, @@ -674,22 +674,16 @@ FwdState::negotiateSSL(int fd) request->clientConnectionManager->setBumpSslErrorList(errNoList); } - HttpRequest *fakeRequest = NULL; - if (request->protocol == AnyP::PROTO_SSL_PEEK && srvX509 != NULL) { - // Create a fake request, with HTTPS protocol and host name the CN name from - // server certificate if exist, to provide a more user friendly URL on error page - fakeRequest = request->clone(); - fakeRequest->protocol = AnyP::PROTO_HTTPS; - safe_free(fakeRequest->canonical); // force re-build url canonical - const char *name = Ssl::CommonHostName(srvX509); - if (name) - fakeRequest->SetHost(name); - - debugs(83, 3, HERE << "Created a fake request for " << - urlCanonical(fakeRequest) << " with " << - fakeRequest->GetHost() << " hostname"); + if (request->flags.sslPeek) { + // If possible, set host name to server certificate CN. + if (srvX509) { + if (const char *name = Ssl::CommonHostName(srvX509)) { + request->SetHost(name); + debugs(83, 3, HERE << "reset request host: " << name); + } + } } - ErrorState *const anErr = makeConnectingError(ERR_SECURE_CONNECT_FAIL, fakeRequest); + ErrorState *const anErr = makeConnectingError(ERR_SECURE_CONNECT_FAIL); anErr->xerrno = sysErrNo; anErr->detail = errDetails; fail(anErr); @@ -765,13 +759,17 @@ FwdState::initiateSSL() SSL_set_session(ssl, peer->sslSession); } else { - if (request->protocol != AnyP::PROTO_SSL_PEEK) - SSL_set_ex_data(ssl, ssl_ex_index_server, (void*)request->GetHost()); - // else we do not have the ssl server name yet, but only its IP address. - - // We need to set SNI TLS extension only in the case we are - // connecting direct to origin server - Ssl::setClientSNI(ssl, request->GetHost()); + // While we are peeking at the certificate, we do not know the server + // name that the client will request (after interception or CONNECT). + if (!request->flags.sslPeek) { + const char *hostname = request->GetHost(); + 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 + if (!request->GetHostIsNumeric()) + Ssl::setClientSNI(ssl, hostname); + } } // Create the ACL check list now, while we have access to more info. @@ -827,8 +825,7 @@ FwdState::connectDone(const Comm::ConnectionPointer &conn, comm_err_t status, in #if USE_SSL if ((serverConnection()->getPeer() && serverConnection()->getPeer()->use_ssl) || - (!serverConnection()->getPeer() && - (request->protocol == AnyP::PROTO_HTTPS || request->protocol == AnyP::PROTO_SSL_PEEK))) { + (!serverConnection()->getPeer() && request->protocol == AnyP::PROTO_HTTPS)) { initiateSSL(); return; } @@ -1033,7 +1030,7 @@ FwdState::dispatch() #endif #if USE_SSL - if (request->protocol == AnyP::PROTO_SSL_PEEK) { + if (request->flags.sslPeek) { CallJobHere1(17, 4, request->clientConnectionManager, ConnStateData, ConnStateData::httpsPeeked, serverConnection()); unregister(serverConn); // async call owns it now @@ -1048,6 +1045,7 @@ FwdState::dispatch() request->peer_domain = serverConnection()->getPeer()->domain; httpStart(this); } else { + assert(!request->flags.sslPeek); request->peer_login = NULL; request->peer_domain = NULL; @@ -1075,10 +1073,6 @@ FwdState::dispatch() case AnyP::PROTO_INTERNAL: -#if USE_SSL - case AnyP::PROTO_SSL_PEEK: -#endif - case AnyP::PROTO_URN: fatal_dump("Should never get here"); break; @@ -1172,10 +1166,10 @@ FwdState::reforward() * proxy-revalidate, must-revalidate or s-maxage Cache-Control directive. */ ErrorState * -FwdState::makeConnectingError(const err_type type, HttpRequest *useRequest) const +FwdState::makeConnectingError(const err_type type) const { return errorCon(type, request->flags.need_validation ? - HTTP_GATEWAY_TIMEOUT : HTTP_SERVICE_UNAVAILABLE, useRequest != NULL? useRequest : request); + HTTP_GATEWAY_TIMEOUT : HTTP_SERVICE_UNAVAILABLE, request); } static void diff --git a/src/forward.h b/src/forward.h index 843bc039d3..505eff5df8 100644 --- a/src/forward.h +++ b/src/forward.h @@ -74,7 +74,7 @@ private: void doneWithRetries(); void completed(); void retryOrBail(); - ErrorState *makeConnectingError(const err_type type, HttpRequest *useRequest = NULL) const; + ErrorState *makeConnectingError(const err_type type) const; static void RegisterWithCacheManager(void); public: diff --git a/src/ssl/ServerPeeker.cc b/src/ssl/ServerPeeker.cc index 7003654405..641bdd3245 100644 --- a/src/ssl/ServerPeeker.cc +++ b/src/ssl/ServerPeeker.cc @@ -24,10 +24,10 @@ Ssl::ServerPeeker::ServerPeeker(ConnStateData *anInitiator, request(new HttpRequest) { debugs(33, 4, HERE << "will peek at " << host << ':' << port); - + request->flags.sslPeek = 1; request->SetHost(host); request->port = port; - request->protocol = AnyP::PROTO_SSL_PEEK; + request->protocol = AnyP::PROTO_HTTPS; request->clientConnectionManager = initiator; const char *uri = urlCanonical(request); entry = storeCreateEntry(uri, uri, request->flags, request->method); diff --git a/src/structs.h b/src/structs.h index 6aaadf29b2..8ee346b4a3 100644 --- a/src/structs.h +++ b/src/structs.h @@ -1009,7 +1009,7 @@ struct _iostats { struct request_flags { - request_flags(): range(0),nocache(0),ims(0),auth(0),cachable(0),hierarchical(0),loopdetect(0),proxy_keepalive(0),proxying(0),refresh(0),redirected(0),need_validation(0),fail_on_validation_err(0),stale_if_hit(0),accelerated(0),ignore_cc(0),intercepted(0),spoof_client_ip(0),internal(0),internalclient(0),must_keepalive(0),chunked_reply(0),stream_error(0),sslBumped(0),destinationIPLookedUp_(0) { + request_flags(): range(0),nocache(0),ims(0),auth(0),cachable(0),hierarchical(0),loopdetect(0),proxy_keepalive(0),proxying(0),refresh(0),redirected(0),need_validation(0),fail_on_validation_err(0),stale_if_hit(0),accelerated(0),ignore_cc(0),intercepted(0),spoof_client_ip(0),internal(0),internalclient(0),must_keepalive(0),chunked_reply(0),stream_error(0),sslPeek(0),sslBumped(0),destinationIPLookedUp_(0) { #if USE_HTTP_VIOLATIONS nocache_hack = 0; #endif @@ -1051,6 +1051,7 @@ unsigned int proxying: unsigned int no_direct:1; /* Deny direct forwarding unless overriden by always_direct. Used in accelerator mode */ unsigned int chunked_reply:1; /**< Reply with chunked transfer encoding */ unsigned int stream_error:1; /**< Whether stream error has occured */ + unsigned int sslPeek:1; ///< internal ssl-bump request to get server cert unsigned int sslBumped:1; /**< ssl-bumped request*/ // When adding new flags, please update cloneAdaptationImmune() as needed.