From: Amos Jeffries Date: Mon, 12 Feb 2018 15:05:25 +0000 (+1300) Subject: Fix loading certificates after tls-cert= changes (#144) X-Git-Tag: M-staged-PR151~7 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=1700fab7ea919f0bb6c6c488af318e322704e895;p=thirdparty%2Fsquid.git 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 5a1c799a95..e72b692d3f 100644 --- 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 e763ebef89..299f1b2051 100644 --- 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 46e5625851..6041c41d37 100644 --- 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 fbfc587443..1d1198bc40 100644 --- 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