From: Amos Jeffries Date: Sat, 29 Dec 2018 23:58:49 +0000 (+1300) Subject: Move session tls-options= assignment out of CreateSession() function X-Git-Tag: SQUID_5_0_1~87 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=60fcfadfa5bb84d08fe70775173993ce9b7d2c00;p=thirdparty%2Fsquid.git Move session tls-options= assignment out of CreateSession() function The original implementation utilized OpenSSL inheritence of library-specific options from SSL_CTX* to SSL* structures. This does not work with GnuTLS which requires setting the options on each session object. The workaround of looking up the session options inside CreateSession() was only looking for client connection details. Which is broken when creating server sessions. * Extend Security::CreateSession() to take a PeerOptions reference containing the specific config details to associate with the new session (if any). * Move selection of CachePeer vs tls_outgoing_* options out to Security::CreateClientSession(). * Pass the Any::PortCfg::secure details into the Security::CreateServerSession(). --- diff --git a/src/client_side.cc b/src/client_side.cc index 4aaf8f6d66..0510409760 100644 --- a/src/client_side.cc +++ b/src/client_side.cc @@ -2340,9 +2340,10 @@ httpAccept(const CommAcceptCbParams ¶ms) /// Create TLS connection structure and update fd_table static bool -httpsCreate(const Comm::ConnectionPointer &conn, const Security::ContextPointer &ctx) +httpsCreate(const ConnStateData *connState, const Security::ContextPointer &ctx) { - if (Security::CreateServerSession(ctx, conn, "client https start")) { + const auto conn = connState->clientConnection; + if (Security::CreateServerSession(ctx, conn, connState->port->secure, "client https start")) { debugs(33, 5, "will negotiate TLS on " << conn); return true; } @@ -2527,7 +2528,7 @@ httpsEstablish(ConnStateData *connState, const Security::ContextPointer &ctx) assert(connState); const Comm::ConnectionPointer &details = connState->clientConnection; - if (!ctx || !httpsCreate(details, ctx)) + if (!ctx || !httpsCreate(connState, ctx)) return; typedef CommCbMemFunT TimeoutDialer; @@ -2891,7 +2892,7 @@ ConnStateData::getSslContextDone(Security::ContextPointer &ctx) } } - if (!httpsCreate(clientConnection, ctx)) + if (!httpsCreate(this, ctx)) return; // bumped intercepted conns should already have Config.Timeout.request set @@ -3116,7 +3117,7 @@ ConnStateData::startPeekAndSplice() Security::ContextPointer unConfiguredCTX(Ssl::createSSLContext(port->secure.signingCa.cert, port->secure.signingCa.pkey, port->secure)); fd_table[clientConnection->fd].dynamicTlsContext = unConfiguredCTX; - if (!httpsCreate(clientConnection, unConfiguredCTX)) + if (!httpsCreate(this, unConfiguredCTX)) return; switchedToHttps_ = true; diff --git a/src/security/PeerOptions.cc b/src/security/PeerOptions.cc index fd9e5968b8..deecc5eb0f 100644 --- a/src/security/PeerOptions.cc +++ b/src/security/PeerOptions.cc @@ -518,8 +518,9 @@ Security::PeerOptions::parseOptions() const char *err = nullptr; const char *priorities = str.c_str(); gnutls_priority_t op; - if (gnutls_priority_init(&op, priorities, &err) != GNUTLS_E_SUCCESS) { - fatalf("Unknown TLS option '%s'", err); + int x = gnutls_priority_init(&op, priorities, &err); + if (x != GNUTLS_E_SUCCESS) { + fatalf("(%s) in TLS options '%s'", ErrorString(x), err); } parsedOptions = Security::ParsedOptions(op, [](gnutls_priority_t p) { debugs(83, 5, "gnutls_priority_deinit p=" << (void*)p); @@ -738,6 +739,7 @@ Security::PeerOptions::updateSessionOptions(Security::SessionPointer &s) { parseOptions(); #if USE_OPENSSL + debugs(83, 5, "set OpenSSL options for session=" << s << ", parsedOptions=" << parsedOptions); // XXX: Options already set before (via the context) are not cleared! SSL_set_options(s.get(), parsedOptions); @@ -750,13 +752,13 @@ Security::PeerOptions::updateSessionOptions(Security::SessionPointer &s) static const SBuf defaults("default"); errMsg = defaults; } else { - debugs(83, 5, "set GnuTLS options '" << sslOptions << "' for session=" << s); + debugs(83, 5, "set GnuTLS session=" << s << ", options='" << sslOptions << ":" << tlsMinOptions << "'"); x = gnutls_priority_set(s.get(), parsedOptions.get()); errMsg = sslOptions; } if (x != GNUTLS_E_SUCCESS) { - debugs(83, DBG_IMPORTANT, "ERROR: Failed to set TLS options (" << errMsg << ":" << tlsMinVersion << "). error: " << Security::ErrorString(x)); + debugs(83, DBG_IMPORTANT, "ERROR: session=" << s << " Failed to set TLS options (" << errMsg << ":" << tlsMinVersion << "). error: " << Security::ErrorString(x)); } #endif } diff --git a/src/security/PeerOptions.h b/src/security/PeerOptions.h index da7721b91d..1deeff258f 100644 --- a/src/security/PeerOptions.h +++ b/src/security/PeerOptions.h @@ -105,13 +105,15 @@ protected: template Security::ContextPointer convertContextFromRawPtr(T ctx) const { #if USE_OPENSSL + debugs(83, 5, "SSL_CTX construct, this=" << (void*)ctx); return ContextPointer(ctx, [](SSL_CTX *p) { - debugs(83, 5, "SSL_free ctx=" << (void*)p); + debugs(83, 5, "SSL_CTX destruct, this=" << (void*)p); SSL_CTX_free(p); }); #elif USE_GNUTLS + debugs(83, 5, "gnutls_certificate_credentials construct, this=" << (void*)ctx); return Security::ContextPointer(ctx, [](gnutls_certificate_credentials_t p) { - debugs(83, 5, "gnutls_certificate_free_credentials ctx=" << (void*)p); + debugs(83, 0, "gnutls_certificate_credentials destruct this=" << (void*)p); gnutls_certificate_free_credentials(p); }); #else diff --git a/src/security/Session.cc b/src/security/Session.cc index 9a9d90904c..6698d26457 100644 --- a/src/security/Session.cc +++ b/src/security/Session.cc @@ -106,7 +106,7 @@ Security::NewSessionObject(const Security::ContextPointer &ctx) #endif static bool -CreateSession(const Security::ContextPointer &ctx, const Comm::ConnectionPointer &conn, Security::Io::Type type, const char *squidCtx) +CreateSession(const Security::ContextPointer &ctx, const Comm::ConnectionPointer &conn, Security::PeerOptions &opts, Security::Io::Type type, const char *squidCtx) { if (!Comm::IsConnOpen(conn)) { debugs(83, DBG_IMPORTANT, "Gone connection"); @@ -122,6 +122,7 @@ CreateSession(const Security::ContextPointer &ctx, const Comm::ConnectionPointer if (!session) { errCode = ERR_get_error(); errAction = "failed to allocate handle"; + debugs(83, DBG_IMPORTANT, "TLS error: " << errAction << ": " << Security::ErrorString(errCode)); } #elif USE_GNUTLS gnutls_session_t tmp; @@ -134,6 +135,7 @@ CreateSession(const Security::ContextPointer &ctx, const Comm::ConnectionPointer if (errCode != GNUTLS_E_SUCCESS) { session.reset(); errAction = "failed to initialize session"; + debugs(83, DBG_IMPORTANT, "TLS error: " << errAction << ": " << Security::ErrorString(errCode)); } #endif @@ -148,10 +150,7 @@ CreateSession(const Security::ContextPointer &ctx, const Comm::ConnectionPointer errCode = gnutls_credentials_set(session.get(), GNUTLS_CRD_CERTIFICATE, ctx.get()); if (errCode == GNUTLS_E_SUCCESS) { - if (auto *peer = conn->getPeer()) - peer->secure.updateSessionOptions(session); - else - Security::ProxyOutgoingConfig.updateSessionOptions(session); + opts.updateSessionOptions(session); // NP: GnuTLS does not yet support the BIO operations // this does the equivalent of SSL_set_fd() for now. @@ -184,13 +183,17 @@ CreateSession(const Security::ContextPointer &ctx, const Comm::ConnectionPointer bool Security::CreateClientSession(const Security::ContextPointer &ctx, const Comm::ConnectionPointer &c, const char *squidCtx) { - return CreateSession(ctx, c, Security::Io::BIO_TO_SERVER, squidCtx); + if (!c || !c->getPeer()) + return CreateSession(ctx, c, Security::ProxyOutgoingConfig, Security::Io::BIO_TO_SERVER, squidCtx); + + auto *peer = c->getPeer(); + return CreateSession(ctx, c, peer->secure, Security::Io::BIO_TO_SERVER, squidCtx); } bool -Security::CreateServerSession(const Security::ContextPointer &ctx, const Comm::ConnectionPointer &c, const char *squidCtx) +Security::CreateServerSession(const Security::ContextPointer &ctx, const Comm::ConnectionPointer &c, Security::PeerOptions &o, const char *squidCtx) { - return CreateSession(ctx, c, Security::Io::BIO_TO_CLIENT, squidCtx); + return CreateSession(ctx, c, o, Security::Io::BIO_TO_CLIENT, squidCtx); } void diff --git a/src/security/Session.h b/src/security/Session.h index 723b9b83c0..7ba40d89d7 100644 --- a/src/security/Session.h +++ b/src/security/Session.h @@ -34,9 +34,11 @@ namespace Security { /// On errors, emits DBG_IMPORTANT with details and returns false. bool CreateClientSession(const Security::ContextPointer &, const Comm::ConnectionPointer &, const char *squidCtx); +class PeerOptions; + /// Creates TLS Server connection structure (aka 'session' state) and initializes TLS/SSL I/O (Comm and BIO). /// On errors, emits DBG_IMPORTANT with details and returns false. -bool CreateServerSession(const Security::ContextPointer &, const Comm::ConnectionPointer &, const char *squidCtx); +bool CreateServerSession(const Security::ContextPointer &, const Comm::ConnectionPointer &, Security::PeerOptions &, const char *squidCtx); #if USE_OPENSSL typedef std::shared_ptr SessionPointer; diff --git a/src/tests/stub_libsecurity.cc b/src/tests/stub_libsecurity.cc index 63de368b36..ac0d72c1b4 100644 --- a/src/tests/stub_libsecurity.cc +++ b/src/tests/stub_libsecurity.cc @@ -109,7 +109,7 @@ void Security::ServerOptions::updateContextSessionId(Security::ContextPointer &) #include "security/Session.h" namespace Security { bool CreateClientSession(const Security::ContextPointer &, const Comm::ConnectionPointer &, const char *) STUB_RETVAL(false) -bool CreateServerSession(const Security::ContextPointer &, const Comm::ConnectionPointer &, const char *) STUB_RETVAL(false) +bool CreateServerSession(const Security::ContextPointer &, const Comm::ConnectionPointer &, Security::PeerOptions &, const char *) STUB_RETVAL(false) void SessionSendGoodbye(const Security::SessionPointer &) STUB bool SessionIsResumed(const Security::SessionPointer &) STUB_RETVAL(false) void MaybeGetSessionResumeData(const Security::SessionPointer &, Security::SessionStatePointer &) STUB