Fix loading certificates after tls-cert= changes (#144)

* Remove self-signed CA check

This check is not needed when loading the initial cert portion of a PEM file
as it will be performed later when loading the chain and was causing
self-signed CA to be rejected incorrectly.

* Fix a typo in debugs output

* Always generate static context from tls-cert= parameter

... if a cert= is provided. SSL-Bump still (for now) requires a static context as fallback when generate fails.

* Revert tlsAttemptHandshake to Squid_SSL_Accept API

* Update const correctness

* Document when initialization is skipped

diff --git a/src/client_side.cc b/src/client_side.cc
index 5a1c799a952035b57a6dd595e4005a3a2de85c8d..e72b692d3f3e41b24fb4c8fead0f706f90b962cc 100644 (file)
--- a/src/client_side.cc
+++ b/src/client_side.cc
@@ -2544,14 +2544,14 @@ httpsCreate(const Comm::ConnectionPointer &conn, const Security::ContextPointer
 
 /**
  *
- * \retval true on success
- * \retval false when needs more data
- * \retval false when an error occurred (and closes the TCP connection)
+ * \retval 1 on success
+ * \retval 0 when needs more data
+ * \retval -1 on error
  */
-static bool
+static int
 tlsAttemptHandshake(ConnStateData *conn, PF *callback)
 {
-    // TODO: maybe throw instead of just closing the TCP connection.
+    // TODO: maybe throw instead of returning -1
     // see https://github.com/squid-cache/squid/pull/81#discussion_r153053278
     int fd = conn->clientConnection->fd;
     auto session = fd_table[fd].ssl.get();
@@ -2561,7 +2561,7 @@ tlsAttemptHandshake(ConnStateData *conn, PF *callback)
 #if USE_OPENSSL
     const auto ret = SSL_accept(session);
     if (ret > 0)
-        return true;
+        return 1;
 
     const int xerrno = errno;
     const auto ssl_error = SSL_get_error(session, ret);
@@ -2570,11 +2570,11 @@ tlsAttemptHandshake(ConnStateData *conn, PF *callback)
 
     case SSL_ERROR_WANT_READ:
         Comm::SetSelect(fd, COMM_SELECT_READ, callback, (callback ? conn : nullptr), 0);
-        return false;
+        return 0;
 
     case SSL_ERROR_WANT_WRITE:
         Comm::SetSelect(fd, COMM_SELECT_WRITE, callback, (callback ? conn : nullptr), 0);
-        return false;
+        return 0;
 
     case SSL_ERROR_SYSCALL:
         if (ret == 0) {
@@ -2599,7 +2599,7 @@ tlsAttemptHandshake(ConnStateData *conn, PF *callback)
 
     const auto x = gnutls_handshake(session);
     if (x == GNUTLS_E_SUCCESS)
-        return true;
+        return 1;
 
     if (gnutls_error_is_fatal(x)) {
         debugs(83, 2, "Error negotiating TLS on " << conn->clientConnection << ": Aborted by client: " << Security::ErrorString(x));
@@ -2607,7 +2607,7 @@ tlsAttemptHandshake(ConnStateData *conn, PF *callback)
     } else if (x == GNUTLS_E_INTERRUPTED || x == GNUTLS_E_AGAIN) {
         const auto ioAction = (gnutls_record_get_direction(session)==0 ? COMM_SELECT_READ : COMM_SELECT_WRITE);
         Comm::SetSelect(fd, ioAction, callback, (callback ? conn : nullptr), 0);
-        return false;
+        return 0;
     }
 
 #else
@@ -2616,8 +2616,7 @@ tlsAttemptHandshake(ConnStateData *conn, PF *callback)
     fatal("FATAL: HTTPS not supported by this Squid.");
 #endif
 
-    conn->clientConnection->close();
-    return false;
+    return -1;
 }
 
 /** negotiate an SSL connection */
@@ -2626,8 +2625,12 @@ clientNegotiateSSL(int fd, void *data)
 {
     ConnStateData *conn = (ConnStateData *)data;
 
-    if (!tlsAttemptHandshake(conn, clientNegotiateSSL))
+    const int ret = tlsAttemptHandshake(conn, clientNegotiateSSL);
+    if (ret <= 0) {
+        if (ret < 0) // An error
+            conn->clientConnection->close();
         return;
+    }
 
     Security::SessionPointer session(fd_table[fd].ssl);
 
@@ -3058,7 +3061,7 @@ void
 ConnStateData::getSslContextDone(Security::ContextPointer &ctx)
 {
     if (port->secure.generateHostCertificates && !ctx) {
-        debugs(33, 2, "Failed to generate TLS cotnext for " << sslConnectHostOrIp);
+        debugs(33, 2, "Failed to generate TLS context for " << sslConnectHostOrIp);
     }
 
     // If generated ssl context = NULL, try to use static ssl context.
@@ -3304,10 +3307,10 @@ ConnStateData::startPeekAndSplice()
     bio->hold(true);
 
     // Here squid should have all of the client hello message so the
-    // tlsAttemptHandshake() should return false;
+    // tlsAttemptHandshake() should return 0.
     // This block exist only to force openSSL parse client hello and detect
     // ERR_SECURE_ACCEPT_FAIL error, which should be checked and splice if required.
-    if (!tlsAttemptHandshake(this, nullptr)) {
+    if (tlsAttemptHandshake(this, nullptr) < 0) {
         debugs(83, 2, "TLS handshake failed.");
         HttpRequest::Pointer request(http ? http->request : nullptr);
         if (!clientTunnelOnError(this, context, request, HttpRequestMethod(), ERR_SECURE_ACCEPT_FAIL))

diff --git a/src/security/KeyData.cc b/src/security/KeyData.cc
index e763ebef892d3e8b9fd85b70a65745ba91c022fc..299f1b205109569c8374970ddda12d3f3c5a7ab7 100644 (file)
--- a/src/security/KeyData.cc
+++ b/src/security/KeyData.cc
@@ -21,6 +21,7 @@ bool
 Security::KeyData::loadX509CertFromFile()
 {
     debugs(83, DBG_IMPORTANT, "Using certificate in " << certFile);
+    cert.reset(); // paranoid: ensure cert is unset
 
 #if USE_OPENSSL
     const char *certFilename = certFile.c_str();
@@ -32,10 +33,7 @@ Security::KeyData::loadX509CertFromFile()
     }
 
     if (X509 *certificate = PEM_read_bio_X509(bio.get(), nullptr, nullptr, nullptr)) {
-        if (X509_check_issued(certificate, certificate) == X509_V_OK)
-            debugs(83, 5, "Certificate is self-signed, will not be chained");
-        else
-            cert.resetWithoutLocking(certificate);
+        cert.resetWithoutLocking(certificate);
     }
 
 #elif USE_GNUTLS
@@ -67,8 +65,6 @@ Security::KeyData::loadX509CertFromFile()
                    debugs(83, 5, "gnutls_x509_crt_deinit cert=" << (void*)p);
                    gnutls_x509_crt_deinit(p);
                });
-    } else {
-        cert.reset(); // paranoid: ensure cert is unset
     }
 
 #else

diff --git a/src/security/ServerOptions.cc b/src/security/ServerOptions.cc
index 46e5625851983ad2984fb686dc74b711dce9821d..6041c41d377219b3d973737e81efea2952e1c5f0 100644 (file)
--- a/src/security/ServerOptions.cc
+++ b/src/security/ServerOptions.cc
@@ -196,11 +196,15 @@ Security::ServerOptions::initServerContexts(AnyP::PortCfg &port)
 
     if (generateHostCertificates) {
         createSigningContexts(port);
+    }
 
-    } else if (!createStaticServerContext(port)) {
+    if (!certs.empty() && !createStaticServerContext(port)) {
         char buf[128];
         fatalf("%s_port %s initialization error", portType, port.s.toUrl(buf, sizeof(buf)));
     }
+
+    // if generate-host-certificates=off and certs is empty, no contexts may be created.
+    // features depending on contexts do their own checks and error messages later.
 }
 
 bool

diff --git a/src/security/ServerOptions.h b/src/security/ServerOptions.h
index fbfc587443476451a5c8d81dc30fce9f04b910a5..1d1198bc40ab8befccdb892973407cdcd40e5a38 100644 (file)
--- a/src/security/ServerOptions.h
+++ b/src/security/ServerOptions.h
@@ -41,7 +41,8 @@ public:
     virtual Security::ContextPointer createBlankContext() const;
     virtual void dumpCfg(Packable *, const char *pfx) const;
 
-    /// initialize all server contexts as-needed
+    /// initialize all server contexts as-needed and load PEM files.
+    /// if none can be created this may do nothing.
     void initServerContexts(AnyP::PortCfg &);
 
     /// update the given TLS security context using squid.conf settings