[squid-dev] [PATCH] GnuTLS session redo

Amos Jeffries squid3 at treenet.co.nz
Thu Aug 4 05:57:00 UTC 2016


Attached is an updated version of the TLS session resume patch which was
previously reverted due to a crash issue.

Trunk no longer needs the implicit conversion that turned out to be
behind that issue. However, I would still like to get this change tested
by someone with OpenSSL to confirm that there is nothing else hiding.

Cheers
Amos
-------------- next part --------------
=== modified file 'src/CachePeer.cc'
--- src/CachePeer.cc	2016-07-18 12:36:38 +0000
+++ src/CachePeer.cc	2016-08-01 13:05:56 +0000
@@ -25,41 +25,40 @@
     typelist(NULL),
     access(NULL),
     weight(1),
     basetime(0),
 #if USE_CACHE_DIGESTS
     digest(NULL),
     digest_url(NULL),
 #endif
     tcp_up(0),
     n_addresses(0),
     rr_count(0),
     next(NULL),
     testing_now(false),
     login(NULL),
     connect_timeout(0),
     connect_fail_limit(0),
     max_conn(0),
     domain(NULL),
 #if USE_OPENSSL
     sslContext(NULL),
-    sslSession(NULL),
 #endif
     front_end_https(0),
     connection_auth(2 /* auto */)
 {
     memset(&stats, 0, sizeof(stats));
     stats.logged_state = PEER_ALIVE;
 
     memset(&icp, 0, sizeof(icp));
     icp.port = CACHE_ICP_PORT;
     icp.version = ICP_VERSION_CURRENT;
 
 #if USE_HTCP
     memset(&htcp, 0, sizeof(htcp));
 #endif
     memset(&options, 0, sizeof(options));
     memset(&mcast, 0, sizeof(mcast));
     memset(&carp, 0, sizeof(carp));
 #if USE_AUTH
     memset(&userhash, 0, sizeof(userhash));
 #endif
@@ -85,26 +84,23 @@
 
 #if USE_CACHE_DIGESTS
     cbdataReferenceDone(digest);
     xfree(digest_url);
 #endif
 
     delete next;
 
     xfree(login);
 
     delete standby.pool;
 
     // the mgr job will notice that its owner is gone and stop
     PeerPoolMgr::Checkpoint(standby.mgr, "peer gone");
 
     xfree(domain);
 
 #if USE_OPENSSL
     if (sslContext)
         SSL_CTX_free(sslContext);
-
-    if (sslSession)
-        SSL_SESSION_free(sslSession);
 #endif
 }
 

=== modified file 'src/CachePeer.h'
--- src/CachePeer.h	2016-07-18 12:36:38 +0000
+++ src/CachePeer.h	2016-08-01 13:05:56 +0000
@@ -167,30 +167,28 @@
         unsigned int hash;
         double load_multiplier;
         double load_factor; /* normalized weight value */
     } sourcehash;
 
     char *login;        /* Proxy authorization */
     time_t connect_timeout;
     int connect_fail_limit;
     int max_conn;
     struct {
         PconnPool *pool; ///< idle connection pool for this peer
         CbcPointer<PeerPoolMgr> mgr; ///< pool manager
         int limit; ///< the limit itself
         bool waitingForClose; ///< a conn must close before we open a standby conn
     } standby; ///< optional "cache_peer standby=limit" feature
     char *domain;       /* Forced domain */
 
     /// 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;
 };
 
 #endif /* SQUID_CACHEPEER_H_ */
 

=== modified file 'src/Makefile.am'
--- src/Makefile.am	2016-08-01 11:11:47 +0000
+++ src/Makefile.am	2016-08-03 17:42:21 +0000
@@ -529,46 +529,46 @@
 	$(BUILT_SOURCES)
 
 squid_LDADD = \
 	$(AUTH_ACL_LIBS) \
 	ident/libident.la \
 	acl/libacls.la \
 	acl/libstate.la \
 	$(AUTH_LIBS) \
 	acl/libapi.la \
 	clients/libclients.la \
 	servers/libservers.la \
 	ftp/libftp.la \
 	helper/libhelper.la \
 	http/libhttp.la \
 	dns/libdns.la \
 	base/libbase.la \
 	libsquid.la \
 	ip/libip.la \
 	fs/libfs.la \
 	DiskIO/libdiskio.la \
+	comm/libcomm.la \
+	security/libsecurity.la \
 	$(SSL_LIBS) \
 	ipc/libipc.la \
 	mgr/libmgr.la \
 	anyp/libanyp.la \
-	comm/libcomm.la \
-	security/libsecurity.la \
 	parser/libparser.la \
 	eui/libeui.la \
 	icmp/libicmp.la \
 	log/liblog.la \
 	format/libformat.la \
 	sbuf/libsbuf.la \
 	$(XTRA_OBJS) \
 	$(REPL_OBJS) \
 	$(NETTLELIB) \
 	$(CRYPTLIB) \
 	$(REGEXLIB) \
 	$(ADAPTATION_LIBS) \
 	$(ESI_LIBS) \
 	$(SNMP_LIBS) \
 	mem/libmem.la \
 	store/libstore.la \
 	$(top_builddir)/lib/libmisccontainers.la \
 	$(top_builddir)/lib/libmiscencoding.la \
 	$(top_builddir)/lib/libmiscutil.la \
 	$(ATOMICLIB) \

=== modified file 'src/adaptation/icap/ServiceRep.cc'
--- src/adaptation/icap/ServiceRep.cc	2016-07-18 12:36:38 +0000
+++ src/adaptation/icap/ServiceRep.cc	2016-08-01 13:05:56 +0000
@@ -17,43 +17,40 @@
 #include "adaptation/icap/ServiceRep.h"
 #include "base/TextException.h"
 #include "comm/Connection.h"
 #include "ConfigParser.h"
 #include "Debug.h"
 #include "fde.h"
 #include "globals.h"
 #include "HttpReply.h"
 #include "ip/tools.h"
 #include "SquidConfig.h"
 #include "SquidTime.h"
 
 #define DEFAULT_ICAP_PORT   1344
 #define DEFAULT_ICAPS_PORT 11344
 
 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),
     connOverloadReported(false),
     theIdleConns(NULL),
     isSuspended(0), notifying(false),
     updateScheduled(false),
     wasAnnouncedUp(true), // do not announce an "up" service at startup
     isDetached(false)
 {
     setMaxConnections();
     theIdleConns = new IdleConnList("ICAP Service", NULL);
 }
 
 Adaptation::Icap::ServiceRep::~ServiceRep()
 {
     delete theIdleConns;
     Must(!theOptionsFetcher);
     delete theOptions;
 }

=== modified file 'src/adaptation/icap/ServiceRep.h'
--- src/adaptation/icap/ServiceRep.h	2016-07-18 12:36:38 +0000
+++ src/adaptation/icap/ServiceRep.h	2016-08-01 13:05:56 +0000
@@ -94,43 +94,41 @@
 
     void noteNewWaiter() {theAllWaiters++;} ///< New xaction waiting for service to be up or available
     void noteGoneWaiter(); ///< An xaction is not waiting any more for service to be available
     bool existWaiters() const {return (theAllWaiters > 0);} ///< if there are xactions waiting for the service to be available
 
     //AsyncJob virtual methods
     virtual bool doneAll() const { return Adaptation::Initiator::doneAll() && false;}
     virtual void callException(const std::exception &e);
 
     virtual void detach();
     virtual bool detached() const;
 
 public: // treat these as private, they are for callbacks only
     void noteTimeToUpdate();
     void noteTimeToNotify();
 
     // receive either an ICAP OPTIONS response header or an abort message
     virtual void noteAdaptationAnswer(const Answer &answer);
 
     Security::ContextPtr sslContext;
-#if USE_OPENSSL
-    SSL_SESSION *sslSession;
-#endif
+    Security::SessionStatePointer sslSession;
 
 private:
     // stores Prepare() callback info
 
     struct Client {
         Pointer service; // one for each client to preserve service
         AsyncCall::Pointer callback;
     };
 
     typedef std::vector<Client> Clients;
     // TODO: rename to theUpWaiters
     Clients theClients; // all clients waiting for a call back
 
     Options *theOptions;
     CbcPointer<Adaptation::Initiate> theOptionsFetcher; // pending ICAP OPTIONS transaction
     time_t theLastUpdate; // time the options were last updated
 
     /// FIFO queue of xactions waiting for a connection slot and not yet notified
     /// about it; xaction is removed when notification is scheduled
     std::deque<Client> theNotificationWaiters;

=== modified file 'src/adaptation/icap/Xaction.cc'
--- src/adaptation/icap/Xaction.cc	2016-07-27 11:06:57 +0000
+++ src/adaptation/icap/Xaction.cc	2016-08-01 13:34:30 +0000
@@ -693,74 +693,62 @@
 void Adaptation::Icap::Xaction::fillDoneStatus(MemBuf &buf) const
 {
     if (haveConnection() && commEof)
         buf.appendf("Comm(%d)", connection->fd);
 
     if (stopReason != NULL)
         buf.append("Stopped", 7);
 }
 
 bool Adaptation::Icap::Xaction::fillVirginHttpHeader(MemBuf &) const
 {
     return false;
 }
 
 bool
 Ssl::IcapPeerConnector::initializeTls(Security::SessionPointer &serverSession)
 {
     if (!Security::PeerConnector::initializeTls(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<ACLFilledChecklist *>(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
 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::GetSessionResumeData(fd_table[fd].ssl, icapService->sslSession);
 }
 
 void
 Adaptation::Icap::Xaction::handleSecuredPeer(Security::EncryptorAnswer &answer)
 {
     Must(securer != NULL);
     securer = NULL;
 
     if (closer != NULL) {
         if (answer.conn != NULL)
             comm_remove_close_handler(answer.conn->fd, closer);
         else
             closer->cancel("securing completed");
         closer = NULL;
     }
 
     if (answer.error.get()) {
         if (answer.conn != NULL)
             answer.conn->close();
         debugs(93, 2, typeName <<

=== modified file 'src/client_side.cc'
--- src/client_side.cc	2016-08-01 11:11:47 +0000
+++ src/client_side.cc	2016-08-01 13:05:56 +0000
@@ -2638,41 +2638,41 @@
         /* NOTREACHED */
     }
     return 1;
 }
 
 /** negotiate an SSL connection */
 static void
 clientNegotiateSSL(int fd, void *data)
 {
     ConnStateData *conn = (ConnStateData *)data;
     X509 *client_cert;
     auto ssl = fd_table[fd].ssl.get();
 
     int ret;
     if ((ret = Squid_SSL_accept(conn, clientNegotiateSSL)) <= 0) {
         if (ret < 0) // An error
             conn->clientConnection->close();
         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 {
         if (Debug::Enabled(83, 4)) {
             /* Write out the SSL session details.. actually the call below, but
              * OpenSSL headers do strange typecasts confusing GCC.. */
             /* PEM_write_SSL_SESSION(debug_log, SSL_get_session(ssl)); */
 #if defined(OPENSSL_VERSION_NUMBER) && OPENSSL_VERSION_NUMBER >= 0x00908000L
             PEM_ASN1_write((i2d_of_void *)i2d_SSL_SESSION, PEM_STRING_SSL_SESSION, debug_log, (char *)SSL_get_session(ssl), NULL,NULL,0,NULL,NULL);
 
 #elif (ALLOW_ALWAYS_SSL_SESSION_DETAIL == 1)
 
             /* When using gcc 3.3.x and OpenSSL 0.9.7x sometimes a compile error can occur here.
             * This is caused by an unpredicatble gcc behaviour on a cast of the first argument
             * of PEM_ASN1_write(). For this reason this code section is disabled. To enable it,
             * define ALLOW_ALWAYS_SSL_SESSION_DETAIL=1.
             * Because there are two possible usable cast, if you get an error here, try the other
             * commented line. */
 
             PEM_ASN1_write((int(*)())i2d_SSL_SESSION, PEM_STRING_SSL_SESSION, debug_log, (char *)SSL_get_session(ssl), NULL,NULL,0,NULL,NULL);

=== modified file 'src/security/BlindPeerConnector.cc'
--- src/security/BlindPeerConnector.cc	2016-07-27 11:06:57 +0000
+++ src/security/BlindPeerConnector.cc	2016-08-01 13:18:47 +0000
@@ -29,56 +29,49 @@
     return ::Config.ssl_client.sslContext;
 }
 
 bool
 Security::BlindPeerConnector::initializeTls(Security::SessionPointer &serverSession)
 {
     if (!Security::PeerConnector::initializeTls(serverSession))
         return false;
 
     if (const CachePeer *peer = serverConnection()->getPeer()) {
         assert(peer);
 
         // NP: domain may be a raw-IP but it is now always set
         assert(!peer->secure.sslDomain.isEmpty());
 
 #if USE_OPENSSL
         // const loss is okay here, ssl_ex_index_server is only read and not assigned a destructor
         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);
 #endif
     }
     return true;
 }
 
 void
 Security::BlindPeerConnector::noteNegotiationDone(ErrorState *error)
 {
     if (error) {
         // XXX: forward.cc calls peerConnectSucceeded() after an OK TCP connect but
         // we call peerConnectFailed() if SSL failed afterwards. Is that OK?
         // It is not clear whether we should call peerConnectSucceeded/Failed()
         // based on TCP results, SSL results, or both. And the code is probably not
         // consistent in this aspect across tunnelling and forwarding modules.
         if (CachePeer *p = serverConnection()->getPeer())
             peerConnectFailed(p);
         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::GetSessionResumeData(fd_table[fd].ssl, peer->sslSession);
     }
-#endif
 }
 

=== modified file 'src/security/Session.cc'
--- src/security/Session.cc	2016-07-18 12:36:38 +0000
+++ src/security/Session.cc	2016-08-04 05:25:27 +0000
@@ -1,38 +1,77 @@
 /*
  * Copyright (C) 1996-2016 The Squid Software Foundation and contributors
  *
  * Squid software is distributed under GPLv2+ license and includes
  * contributions from numerous individuals and organizations.
  * Please see the COPYING and CONTRIBUTORS files for details.
  */
 
 #include "squid.h"
 #include "anyp/PortCfg.h"
 #include "base/RunnersRegistry.h"
 #include "ipc/MemMap.h"
 #include "security/Session.h"
 #include "SquidConfig.h"
 
 #define SSL_SESSION_ID_SIZE 32
 #define SSL_SESSION_MAX_SIZE 10*1024
 
+bool
+Security::SessionIsResumed(const Security::SessionPointer &s)
+{
+    return
+#if USE_OPENSSL
+        SSL_session_reused(s.get()) == 1;
+#elif USE_GNUTLS
+        gnutls_session_is_resumed(s.get()) != 0;
+#else
+        false;
+#endif
+}
+
+void
+Security::GetSessionResumeData(const Security::SessionPointer &s, Security::SessionStatePointer &data)
+{
+    if (!SessionIsResumed(s)) {
+#if USE_OPENSSL
+        data.reset(SSL_get1_session(s.get()));
+#elif USE_GNUTLS
+        gnutls_datum_t *tmp = nullptr;
+        (void)gnutls_session_get_data2(s.get(), tmp);
+        data.reset(tmp);
+#endif
+    }
+}
+
+void
+Security::SetSessionResumeData(const Security::SessionPointer &s, const Security::SessionStatePointer &data)
+{
+    if (data) {
+#if USE_OPENSSL
+        (void)SSL_set_session(s.get(), data.get());
+#elif USE_GNUTLS
+        (void)gnutls_session_set_data(s.get(), data->data, data->size);
+#endif
+    }
+}
+
 static bool
 isTlsServer()
 {
     for (AnyP::PortCfgPointer s = HttpPortList; s != nullptr; s = s->next) {
         if (s->secure.encryptTransport)
             return true;
         if (s->flags.tunnelSslBumping)
             return true;
     }
 
     return false;
 }
 
 void
 initializeSessionCache()
 {
 #if USE_OPENSSL
     // Check if the MemMap keys and data are enough big to hold
     // session ids and session data
     assert(SSL_SESSION_ID_SIZE >= MEMMAP_SLOT_KEY_SIZE);

=== modified file 'src/security/Session.h'
--- src/security/Session.h	2016-07-18 12:36:38 +0000
+++ src/security/Session.h	2016-08-02 05:17:39 +0000
@@ -1,54 +1,73 @@
 /*
  * Copyright (C) 1996-2016 The Squid Software Foundation and contributors
  *
  * Squid software is distributed under GPLv2+ license and includes
  * contributions from numerous individuals and organizations.
  * Please see the COPYING and CONTRIBUTORS files for details.
  */
 
 #ifndef SQUID_SRC_SECURITY_SESSION_H
 #define SQUID_SRC_SECURITY_SESSION_H
 
+#include "base/HardFun.h"
 #include "security/LockingPointer.h"
 
 #include <memory>
 
 #if USE_OPENSSL
 #if HAVE_OPENSSL_SSL_H
 #include <openssl/ssl.h>
 #endif
 #endif
 
 #if USE_GNUTLS
 #if HAVE_GNUTLS_GNUTLS_H
 #include <gnutls/gnutls.h>
 #endif
 #endif
 
 namespace Security {
 
 #if USE_OPENSSL
 typedef SSL* SessionPtr;
 CtoCpp1(SSL_free, SSL *);
 typedef LockingPointer<SSL, Security::SSL_free_cpp, CRYPTO_LOCK_SSL> SessionPointer;
 
+typedef std::unique_ptr<SSL_SESSION, HardFun<void, SSL_SESSION*, &SSL_SESSION_free>> SessionStatePointer;
+
 #elif USE_GNUTLS
 typedef gnutls_session_t SessionPtr;
 // Locks can be implemented attaching locks counter to gnutls_session_t
 // objects using the gnutls_session_set_ptr()/gnutls_session_get_ptr ()
 // library functions
 CtoCpp1(gnutls_deinit, gnutls_session_t);
 typedef LockingPointer<struct gnutls_session_int, gnutls_deinit_cpp, -1> 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<gnutls_datum_t, HardFun<void, void*, &Security::squid_gnutls_free>> SessionStatePointer;
+
 #else
 // use void* so we can check against NULL
 typedef void* SessionPtr;
 CtoCpp1(xfree, SessionPtr);
 typedef LockingPointer<void, xfree_cpp, -1> SessionPointer;
 
+typedef std::unique_ptr<int> SessionStatePointer;
+
 #endif
 
+/// whether the session is a resumed one
+bool SessionIsResumed(const Security::SessionPointer &);
+
+/// Retrieve the data needed to resume this session on a later connection
+void GetSessionResumeData(const Security::SessionPointer &, Security::SessionStatePointer &);
+
+/// 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 */
 

=== modified file 'src/tests/stub_libsecurity.cc'
--- src/tests/stub_libsecurity.cc	2016-07-27 21:16:35 +0000
+++ src/tests/stub_libsecurity.cc	2016-08-04 05:25:31 +0000
@@ -69,20 +69,27 @@
 Security::PeerOptions Security::ProxyOutgoingConfig;
 void Security::PeerOptions::parse(char const*) STUB
 Security::ContextPtr Security::PeerOptions::createClientContext(bool) STUB_RETVAL(NULL)
 void Security::PeerOptions::updateTlsVersionLimits() STUB
 Security::ContextPtr Security::PeerOptions::createBlankContext() const STUB
 void Security::PeerOptions::updateContextCa(Security::ContextPtr &) STUB
 void Security::PeerOptions::updateContextCrl(Security::ContextPtr &) STUB
 void Security::PeerOptions::dumpCfg(Packable*, char const*) const STUB
 long Security::PeerOptions::parseOptions() STUB_RETVAL(0)
 long Security::PeerOptions::parseFlags() STUB_RETVAL(0)
 void parse_securePeerOptions(Security::PeerOptions *) STUB
 
 #include "security/ServerOptions.h"
 //Security::ServerOptions::ServerOptions(const Security::ServerOptions &) STUB
 void Security::ServerOptions::parse(const char *) STUB
 void Security::ServerOptions::dumpCfg(Packable *, const char *) const STUB
 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 GetSessionResumeData(const Security::SessionPointer &, Security::SessionStatePointer &) STUB
+void SetSessionResumeData(const Security::SessionPointer &, const Security::SessionStatePointer &) STUB
+} // namespace Security
+



More information about the squid-dev mailing list