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 a3b4b2c6108da36001ee079c319934ac1310948a..7cedcb019f669bd1b98b4cba2f7f7845015c05d0 100644 (file)
--- 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 587c72d8a6b6eb3ab18b373e49c573e703081664..5ed3cc6a9989d21b019e85fe6be25642f19472a9 100644 (file)
--- 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 843bc039d380a863bcd4adddafea9da017d05097..505eff5df844e51469c6d03dcfc8a4df3c91d342 100644 (file)
--- 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 70036544059e627ca88e7a1bb788ce8be53d0b8b..641bdd3245e06f46124a85b8a310eb58efd656c8 100644 (file)
--- 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 6aaadf29b2e4ca5fd51290b8743b03704727c688..8ee346b4a3927cb1b6c70e65d6499080669119c3 100644 (file)
--- 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.