From 5d9a65df667cf7c16f81b0d3bda939e2336deecd Mon Sep 17 00:00:00 2001 From: Amos Jeffries Date: Mon, 5 Sep 2016 09:29:19 +1200 Subject: [PATCH] GnuTLS: support for TLS session resume (part 2) --- src/CachePeer.cc | 4 -- src/CachePeer.h | 4 +- src/Makefile.am | 2 +- src/adaptation/icap/ServiceRep.cc | 3 -- src/adaptation/icap/ServiceRep.h | 4 +- src/adaptation/icap/Xaction.cc | 20 ++-------- src/client_side.cc | 2 +- src/security/BlindPeerConnector.cc | 15 ++----- src/security/Session.cc | 63 ++++++++++++++++++++++++++++++ src/security/Session.h | 27 +++++++++++++ src/tests/stub_libsecurity.cc | 7 ++++ 11 files changed, 109 insertions(+), 42 deletions(-) diff --git a/src/CachePeer.cc b/src/CachePeer.cc index fac6a3e5ba..964948ad5b 100644 --- a/src/CachePeer.cc +++ b/src/CachePeer.cc @@ -42,7 +42,6 @@ CachePeer::CachePeer() : domain(NULL), #if USE_OPENSSL sslContext(NULL), - sslSession(NULL), #endif front_end_https(0), connection_auth(2 /* auto */) @@ -102,9 +101,6 @@ CachePeer::~CachePeer() #if USE_OPENSSL if (sslContext) SSL_CTX_free(sslContext); - - if (sslSession) - SSL_SESSION_free(sslSession); #endif } diff --git a/src/CachePeer.h b/src/CachePeer.h index 848150df31..67a6302ae3 100644 --- a/src/CachePeer.h +++ b/src/CachePeer.h @@ -184,9 +184,7 @@ public: /// security settings for peer connection Security::PeerOptions secure; Security::ContextPtr sslContext; -#if USE_OPENSSL - SSL_SESSION *sslSession; -#endif + Security::SessionStatePointer sslSession; int front_end_https; int connection_auth; diff --git a/src/Makefile.am b/src/Makefile.am index 11180a0827..7557134021 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -547,8 +547,8 @@ squid_LDADD = \ fs/libfs.la \ DiskIO/libdiskio.la \ comm/libcomm.la \ - $(SSL_LIBS) \ security/libsecurity.la \ + $(SSL_LIBS) \ ipc/libipc.la \ mgr/libmgr.la \ anyp/libanyp.la \ diff --git a/src/adaptation/icap/ServiceRep.cc b/src/adaptation/icap/ServiceRep.cc index 7aec5ca4f2..14996a7fa8 100644 --- a/src/adaptation/icap/ServiceRep.cc +++ b/src/adaptation/icap/ServiceRep.cc @@ -34,9 +34,6 @@ CBDATA_NAMESPACED_CLASS_INIT(Adaptation::Icap, ServiceRep); Adaptation::Icap::ServiceRep::ServiceRep(const ServiceConfigPointer &svcCfg): AsyncJob("Adaptation::Icap::ServiceRep"), Adaptation::Service(svcCfg), sslContext(NULL), -#if USE_OPENSSL - sslSession(NULL), -#endif theOptions(NULL), theOptionsFetcher(0), theLastUpdate(0), theBusyConns(0), theAllWaiters(0), diff --git a/src/adaptation/icap/ServiceRep.h b/src/adaptation/icap/ServiceRep.h index 39e9bf57b0..579bfbd574 100644 --- a/src/adaptation/icap/ServiceRep.h +++ b/src/adaptation/icap/ServiceRep.h @@ -111,9 +111,7 @@ public: // treat these as private, they are for callbacks only virtual void noteAdaptationAnswer(const Answer &answer); Security::ContextPtr sslContext; -#if USE_OPENSSL - SSL_SESSION *sslSession; -#endif + Security::SessionStatePointer sslSession; private: // stores Prepare() callback info diff --git a/src/adaptation/icap/Xaction.cc b/src/adaptation/icap/Xaction.cc index 79ee247440..74b03938e3 100644 --- a/src/adaptation/icap/Xaction.cc +++ b/src/adaptation/icap/Xaction.cc @@ -710,22 +710,18 @@ Ssl::IcapPeerConnector::initialize(Security::SessionPointer &serverSession) if (!Security::PeerConnector::initialize(serverSession)) return false; -#if USE_OPENSSL assert(!icapService->cfg().secure.sslDomain.isEmpty()); +#if USE_OPENSSL SBuf *host = new SBuf(icapService->cfg().secure.sslDomain); SSL_set_ex_data(serverSession.get(), ssl_ex_index_server, host); ACLFilledChecklist *check = static_cast(SSL_get_ex_data(serverSession.get(), ssl_ex_index_cert_error_check)); if (check) check->dst_peer_name = *host; +#endif - if (icapService->sslSession) - SSL_set_session(serverSession.get(), icapService->sslSession); - + Security::SetSessionResumeData(serverSession, icapService->sslSession); return true; -#else - return false; -#endif } void @@ -734,16 +730,8 @@ Ssl::IcapPeerConnector::noteNegotiationDone(ErrorState *error) if (error) return; -#if USE_OPENSSL const int fd = serverConnection()->fd; - auto ssl = fd_table[fd].ssl.get(); - assert(ssl); - if (!SSL_session_reused(ssl)) { - if (icapService->sslSession) - SSL_SESSION_free(icapService->sslSession); - icapService->sslSession = SSL_get1_session(ssl); - } -#endif + Security::MaybeGetSessionResumeData(fd_table[fd].ssl, icapService->sslSession); } void diff --git a/src/client_side.cc b/src/client_side.cc index bcc25d4410..1d83a43fb0 100644 --- a/src/client_side.cc +++ b/src/client_side.cc @@ -2663,7 +2663,7 @@ clientNegotiateSSL(int fd, void *data) return; } - if (SSL_session_reused(ssl)) { + if (Security::SessionIsResumed(fd_table[fd].ssl)) { debugs(83, 2, "clientNegotiateSSL: Session " << SSL_get_session(ssl) << " reused on FD " << fd << " (" << fd_table[fd].ipaddr << ":" << (int)fd_table[fd].remote_port << ")"); } else { diff --git a/src/security/BlindPeerConnector.cc b/src/security/BlindPeerConnector.cc index 54631f3305..578e0e18cd 100644 --- a/src/security/BlindPeerConnector.cc +++ b/src/security/BlindPeerConnector.cc @@ -47,8 +47,7 @@ Security::BlindPeerConnector::initialize(Security::SessionPointer &serverSession SBuf *host = new SBuf(peer->secure.sslDomain); SSL_set_ex_data(serverSession.get(), ssl_ex_index_server, host); - if (peer->sslSession) - SSL_set_session(serverSession.get(), peer->sslSession); + Security::SetSessionResumeData(serverSession, peer->sslSession); } else { SBuf *hostName = new SBuf(request->url.host()); SSL_set_ex_data(serverSession.get(), ssl_ex_index_server, (void*)hostName); @@ -71,15 +70,9 @@ Security::BlindPeerConnector::noteNegotiationDone(ErrorState *error) return; } -#if USE_OPENSSL - const int fd = serverConnection()->fd; - Security::SessionPtr ssl = fd_table[fd].ssl.get(); - if (serverConnection()->getPeer() && !SSL_session_reused(ssl)) { - if (serverConnection()->getPeer()->sslSession) - SSL_SESSION_free(serverConnection()->getPeer()->sslSession); - - serverConnection()->getPeer()->sslSession = SSL_get1_session(ssl); + if (auto *peer = serverConnection()->getPeer()) { + const int fd = serverConnection()->fd; + Security::MaybeGetSessionResumeData(fd_table[fd].ssl, peer->sslSession); } -#endif } diff --git a/src/security/Session.cc b/src/security/Session.cc index 38480c1438..cf717926f7 100644 --- a/src/security/Session.cc +++ b/src/security/Session.cc @@ -6,9 +6,12 @@ * Please see the COPYING and CONTRIBUTORS files for details. */ +/* DEBUG: section 83 TLS session management */ + #include "squid.h" #include "anyp/PortCfg.h" #include "base/RunnersRegistry.h" +#include "Debug.h" #include "ipc/MemMap.h" #include "security/Session.h" #include "SquidConfig.h" @@ -16,6 +19,66 @@ #define SSL_SESSION_ID_SIZE 32 #define SSL_SESSION_MAX_SIZE 10*1024 +bool +Security::SessionIsResumed(const Security::SessionPointer &s) +{ + bool result = false; +#if USE_OPENSSL + result = SSL_session_reused(s.get()) == 1; +#elif USE_GNUTLS + result = gnutls_session_is_resumed(s.get()) != 0; +#endif + debugs(83, 7, "session=" << (void*)s.get() << ", query? answer: " << (result ? 'T' : 'F') ); + return result; +} + +void +Security::MaybeGetSessionResumeData(const Security::SessionPointer &s, Security::SessionStatePointer &data) +{ + if (!SessionIsResumed(s)) { +#if USE_OPENSSL + // nil is valid for SSL_get1_session(), it cannot fail. + data.reset(SSL_get1_session(s.get())); +#elif USE_GNUTLS + gnutls_datum_t *tmp = nullptr; + const auto x = gnutls_session_get_data2(s.get(), tmp); + if (x != GNUTLS_E_SUCCESS) { + debugs(83, 3, "session=" << (void*)s.get() << " error: " << gnutls_strerror(x)); + } + data.reset(tmp); +#endif + debugs(83, 5, "session=" << (void*)s.get() << " data=" << (void*)data.get()); + } else { + debugs(83, 5, "session=" << (void*)s.get() << " data=" << (void*)data.get() << ", do nothing."); + } +} + +void +Security::SetSessionResumeData(const Security::SessionPointer &s, const Security::SessionStatePointer &data) +{ + if (data) { +#if USE_OPENSSL + if (!SSL_set_session(s.get(), data.get())) { + const auto ssl_error = ERR_get_error(); + debugs(83, 3, "session=" << (void*)s.get() << " data=" << (void*)data.get() << + " resume error: " << ERR_error_string(ssl_error, nullptr)); + } +#elif USE_GNUTLS + const auto x = gnutls_session_set_data(s.get(), data->data, data->size); + if (x != GNUTLS_E_SUCCESS) { + debugs(83, 3, "session=" << (void*)s.get() << " data=" << (void*)data.get() << + " resume error: " << gnutls_strerror(x)); + } +#else + // critical because, how did it get here? + debugs(83, DBG_CRITICAL, "no TLS library. session=" << (void*)s.get() << " data=" << (void*)data.get()); +#endif + debugs(83, 5, "session=" << (void*)s.get() << " data=" << (void*)data.get()); + } else { + debugs(83, 5, "session=" << (void*)s.get() << " no resume data"); + } +} + static bool isTlsServer() { diff --git a/src/security/Session.h b/src/security/Session.h index de9c3b785d..e7ce105709 100644 --- a/src/security/Session.h +++ b/src/security/Session.h @@ -9,6 +9,7 @@ #ifndef SQUID_SRC_SECURITY_SESSION_H #define SQUID_SRC_SECURITY_SESSION_H +#include "base/HardFun.h" #include "security/LockingPointer.h" #include @@ -32,6 +33,8 @@ typedef SSL* SessionPtr; CtoCpp1(SSL_free, SSL *); typedef LockingPointer SessionPointer; +typedef std::unique_ptr> SessionStatePointer; + #elif USE_GNUTLS typedef gnutls_session_t SessionPtr; // Locks can be implemented attaching locks counter to gnutls_session_t @@ -40,14 +43,38 @@ typedef gnutls_session_t SessionPtr; CtoCpp1(gnutls_deinit, gnutls_session_t); typedef LockingPointer SessionPointer; +// wrapper function to get around gnutls_free being a typedef +inline void squid_gnutls_free(void *d) {gnutls_free(d);} +typedef std::unique_ptr> SessionStatePointer; + #else // use void* so we can check against NULL typedef void* SessionPtr; CtoCpp1(xfree, SessionPtr); typedef LockingPointer SessionPointer; +typedef std::unique_ptr SessionStatePointer; + #endif +/// whether the session is a resumed one +bool SessionIsResumed(const Security::SessionPointer &); + +/** + * When the session is not a resumed session, retrieve the details needed to + * resume a later connection and store them in 'data'. This may result in 'data' + * becoming a nil Pointer if no details exist or an error occurs. + * + * When the session is already a resumed session, do nothing and leave 'data' + * unhanged. + * XXX: is this latter behaviour always correct? + */ +void MaybeGetSessionResumeData(const Security::SessionPointer &, Security::SessionStatePointer &data); + +/// Set the data for resuming a previous session. +/// Needs to be done before using the SessionPointer for a handshake. +void SetSessionResumeData(const Security::SessionPointer &, const Security::SessionStatePointer &); + } // namespace Security #endif /* SQUID_SRC_SECURITY_SESSION_H */ diff --git a/src/tests/stub_libsecurity.cc b/src/tests/stub_libsecurity.cc index 95f88d2cf6..304d2bb17c 100644 --- a/src/tests/stub_libsecurity.cc +++ b/src/tests/stub_libsecurity.cc @@ -87,3 +87,10 @@ Security::ContextPtr Security::ServerOptions::createBlankContext() const STUB bool Security::ServerOptions::createStaticServerContext(AnyP::PortCfg &) STUB_RETVAL(false) void Security::ServerOptions::updateContextEecdh(Security::ContextPtr &) STUB +#include "security/Session.h" +namespace Security { +bool SessionIsResumed(const Security::SessionPointer &) STUB_RETVAL(false) +void MaybeGetSessionResumeData(const Security::SessionPointer &, Security::SessionStatePointer &) STUB +void SetSessionResumeData(const Security::SessionPointer &, const Security::SessionStatePointer &) STUB +} // namespace Security + -- 2.47.2