Implement GnuTLS session creation

Convert SessionPointer to use std::shared_ptr so that gnuts_deinit() can
be run at the correct time.

Also, improve debugging in PeerConnector and BlindPeerConnector negotiation
methods.

Also, add error handling for TLS connection handshake failures.

diff --git a/src/security/BlindPeerConnector.cc b/src/security/BlindPeerConnector.cc
index 1a1a3a3bfce84f7b74842b3763fe1e87a734778d..b27c7727359433c2b90a848424631079e12ad9e4 100644 (file)
--- a/src/security/BlindPeerConnector.cc
+++ b/src/security/BlindPeerConnector.cc
@@ -32,8 +32,10 @@ Security::BlindPeerConnector::getTlsContext()
 bool
 Security::BlindPeerConnector::initialize(Security::SessionPointer &serverSession)
 {
-    if (!Security::PeerConnector::initialize(serverSession))
+    if (!Security::PeerConnector::initialize(serverSession)) {
+        debugs(83, 5, "Security::PeerConnector::initialize failed");
         return false;
+    }
 
     if (const CachePeer *peer = serverConnection()->getPeer()) {
         assert(peer);
@@ -52,6 +54,8 @@ Security::BlindPeerConnector::initialize(Security::SessionPointer &serverSession
         SSL_set_ex_data(serverSession.get(), ssl_ex_index_server, (void*)hostName);
 #endif
     }
+
+    debugs(83, 5, "success");
     return true;
 }
 
@@ -59,6 +63,7 @@ void
 Security::BlindPeerConnector::noteNegotiationDone(ErrorState *error)
 {
     if (error) {
+        debugs(83, 5, "error=" << (void*)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()

diff --git a/src/security/PeerConnector.cc b/src/security/PeerConnector.cc
index 3677c5d7b81e057f7b805856b52b4fa033a0138b..3f5a256d6169b02b4857cd745ed68b1196fc696e 100644 (file)
--- a/src/security/PeerConnector.cc
+++ b/src/security/PeerConnector.cc
@@ -58,6 +58,7 @@ void
 Security::PeerConnector::start()
 {
     AsyncJob::start();
+    debugs(83, 5, "this=" << (void*)this);
 
     Security::SessionPointer tmp;
     if (prepareSocket() && initialize(tmp))
@@ -76,6 +77,7 @@ Security::PeerConnector::commCloseHandler(const CommCloseCbParams &params)
 void
 Security::PeerConnector::connectionClosed(const char *reason)
 {
+    debugs(83, 5, reason << " socket closed/closing. this=" << (void*)this);
     mustStop(reason);
     callback = NULL;
 }
@@ -83,16 +85,18 @@ Security::PeerConnector::connectionClosed(const char *reason)
 bool
 Security::PeerConnector::prepareSocket()
 {
-    const int fd = serverConnection()->fd;
-    if (!Comm::IsConnOpen(serverConn) || fd_table[serverConn->fd].closing()) {
+    debugs(83, 5, serverConnection() << ", this=" << (void*)this);
+    if (!Comm::IsConnOpen(serverConnection()) || fd_table[serverConnection()->fd].closing()) {
         connectionClosed("Security::PeerConnector::prepareSocket");
         return false;
     }
 
+    debugs(83, 5, serverConnection());
+
     // watch for external connection closures
     typedef CommCbMemFunT<Security::PeerConnector, CommCloseCbParams> Dialer;
     closeHandler = JobCallback(9, 5, Dialer, this, Security::PeerConnector::commCloseHandler);
-    comm_add_close_handler(fd, closeHandler);
+    comm_add_close_handler(serverConnection()->fd, closeHandler);
     return true;
 }
 
@@ -100,6 +104,7 @@ bool
 Security::PeerConnector::initialize(Security::SessionPointer &serverSession)
 {
     Security::ContextPointer ctx(getTlsContext());
+    debugs(83, 5, serverConnection() << ", ctx=" << (void*)ctx.get());
 
     if (!ctx || !Security::CreateClientSession(ctx, serverConnection(), "server https start")) {
         const auto xerrno = errno;
@@ -115,6 +120,7 @@ Security::PeerConnector::initialize(Security::SessionPointer &serverSession)
 
     // A TLS/SSL session has now been created for the connection and stored in fd_table
     serverSession = fd_table[serverConnection()->fd].ssl;
+    debugs(83, 5, serverConnection() << ", session=" << (void*)serverSession.get());
 
 #if USE_OPENSSL
     // If CertValidation Helper used do not lookup checklist for errors,
@@ -177,13 +183,19 @@ Security::PeerConnector::negotiate()
         return;
 
 #if USE_OPENSSL
-    const int result = SSL_connect(fd_table[fd].ssl.get());
+    auto session = fd_table[fd].ssl.get();
+    debugs(83, 5, "SSL_connect session=" << (void*)session);
+    const int result = SSL_connect(session);
+    if (result <= 0) {
 #elif USE_GNUTLS
-    const int result = gnutls_handshake(fd_table[fd].ssl.get());
+    auto session = fd_table[fd].ssl.get();
+    debugs(83, 5, "gnutls_handshake session=" << (void*)session);
+    const int result = gnutls_handshake(session);
+    if (result != GNUTLS_E_SUCCESS) {
+        debugs(83, 5, "gnutls_handshake session=" << (void*)session << ", result=" << result);
 #else
-    const int result = -1;
+    if (const int result = -1) {
 #endif
-    if (result <= 0) {
         handleNegotiateError(result);
         return; // we might be gone by now
     }
@@ -205,11 +217,11 @@ Security::PeerConnector::sslFinalized()
         Security::SessionPointer session(fd_table[fd].ssl);
 
         Ssl::CertValidationRequest validationRequest;
-        // WARNING: Currently we do not use any locking for any of the
-        // members of the Ssl::CertValidationRequest class. In this code the
+        // WARNING: Currently we do not use any locking for 'errors' member
+        // of the Ssl::CertValidationRequest class. In this code the
         // Ssl::CertValidationRequest object used only to pass data to
         // Ssl::CertValidationHelper::submit method.
-        validationRequest.ssl = session.get();
+        validationRequest.ssl = session;
         if (SBuf *dName = (SBuf *)SSL_get_ex_data(session.get(), ssl_ex_index_server))
             validationRequest.domainName = dName->c_str();
         if (Security::CertErrors *errs = static_cast<Security::CertErrors *>(SSL_get_ex_data(session.get(), ssl_ex_index_ssl_errors)))
@@ -360,10 +372,11 @@ Security::PeerConnector::NegotiateSsl(int, void *data)
 void
 Security::PeerConnector::handleNegotiateError(const int ret)
 {
-#if USE_OPENSSL
     const int fd = serverConnection()->fd;
-    unsigned long ssl_lib_error = SSL_ERROR_NONE;
-    Security::SessionPointer session(fd_table[fd].ssl);
+    const Security::SessionPointer session(fd_table[fd].ssl);
+    unsigned long ssl_lib_error = ret;
+
+#if USE_OPENSSL
     const int ssl_error = SSL_get_error(session.get(), ret);
 
     switch (ssl_error) {
@@ -382,20 +395,42 @@ Security::PeerConnector::handleNegotiateError(const int ret)
         break;
     default:
         // no special error handling for all other errors
+        ssl_lib_error = SSL_ERROR_NONE;
         break;
     }
 
-    // Log connection details, if any
-    recordNegotiationDetails();
-    noteNegotiationError(ret, ssl_error, ssl_lib_error);
-
 #elif USE_GNUTLS
-    // TODO: handle specific errors individually like above
-    if (gnutls_error_is_fatal(ret) == 0) {
-        noteWantRead();
+    const int ssl_error = ret;
+
+    switch (ret) {
+    case GNUTLS_E_WARNING_ALERT_RECEIVED: {
+            auto alert = gnutls_alert_get(session.get());
+            debugs(83, DBG_IMPORTANT, "TLS ALERT: " << gnutls_alert_get_name(alert));
+        }
+        // drop through to next case
+
+    case GNUTLS_E_AGAIN:
+    case GNUTLS_E_INTERRUPTED:
+        if (gnutls_record_get_direction(session.get()) == 0)
+            noteWantWrite();
+        else
+            noteWantRead();
         return;
+
+    default:
+        // no special error handling for all other errors
+        break;
     }
+
+#else
+    // this avoids unused variable compiler warnings.
+    Must(!session);
+    const int ssl_error = ret;
 #endif
+
+    // Log connection details, if any
+    recordNegotiationDetails();
+    noteNegotiationError(ret, ssl_error, ssl_lib_error);
 }
 
 void
@@ -439,19 +474,20 @@ Security::PeerConnector::noteWantWrite()
 void
 Security::PeerConnector::noteNegotiationError(const int ret, const int ssl_error, const int ssl_lib_error)
 {
-#if USE_OPENSSL // not used unless OpenSSL enabled.
 #if defined(EPROTO)
     int sysErrNo = EPROTO;
 #else
     int sysErrNo = EACCES;
 #endif
 
+#if USE_OPENSSL
     // store/report errno when ssl_error is SSL_ERROR_SYSCALL, ssl_lib_error is 0, and ret is -1
     if (ssl_error == SSL_ERROR_SYSCALL && ret == -1 && ssl_lib_error == 0)
         sysErrNo = errno;
+#endif
 
     const int fd = serverConnection()->fd;
-    debugs(83, DBG_IMPORTANT, "Error negotiating SSL on FD " << fd <<
+    debugs(83, DBG_IMPORTANT, "ERROR: negotiating TLS on FD " << fd <<
            ": " << Security::ErrorString(ssl_lib_error) << " (" <<
            ssl_error << "/" << ret << "/" << errno << ")");
 
@@ -462,6 +498,7 @@ Security::PeerConnector::noteNegotiationError(const int ret, const int ssl_error
         anErr = new ErrorState(ERR_SECURE_CONNECT_FAIL, Http::scServiceUnavailable, NULL);
     anErr->xerrno = sysErrNo;
 
+#if USE_OPENSSL
     Security::SessionPointer session(fd_table[fd].ssl);
     Ssl::ErrorDetail *errFromFailure = static_cast<Ssl::ErrorDetail *>(SSL_get_ex_data(session.get(), ssl_ex_index_ssl_error_detail));
     if (errFromFailure != NULL) {
@@ -478,10 +515,10 @@ Security::PeerConnector::noteNegotiationError(const int ret, const int ssl_error
 
     if (ssl_lib_error != SSL_ERROR_NONE)
         anErr->detail->setLibError(ssl_lib_error);
+#endif
 
     noteNegotiationDone(anErr);
     bail(anErr);
-#endif
 }
 
 void
@@ -500,11 +537,20 @@ Security::PeerConnector::bail(ErrorState *error)
     // the recepient before the fd-closure notification), but we would rather
     // minimize the number of fd-closure notifications and let the recepient
     // manage the TCP state of the connection.
+
+#if USE_GNUTLS
+    // but we do need to release the bad TLS related details in fd_table
+    // ... or GnuTLS will SEGFAULT.
+    const int fd = serverConnection()->fd;
+    Security::SessionClose(fd_table[fd].ssl, fd);
+#endif
 }
 
 void
 Security::PeerConnector::callBack()
 {
+    debugs(83, 5, "TLS setup ended for " << serverConnection());
+
     AsyncCall::Pointer cb = callback;
     // Do this now so that if we throw below, swanSong() assert that we _tried_
     // to call back holds.

diff --git a/src/security/Session.cc b/src/security/Session.cc
index 3dcf102b262d6bab4fe41ecf52027b2c39bee37f..0d66a6361c858dbe3d3470b0e53d98d76c843af5 100644 (file)
--- a/src/security/Session.cc
+++ b/src/security/Session.cc
@@ -41,6 +41,7 @@ tls_read_method(int fd, char *buf, int len)
     int i = gnutls_record_recv(session, buf, len);
 #endif
     if (i > 0) {
+        debugs(83, 8, "TLS FD " << fd << " session=" << (void*)session << " " << i << " bytes");
         (void)VALGRIND_MAKE_MEM_DEFINED(buf, i);
     }
 
@@ -75,6 +76,9 @@ tls_write_method(int fd, const char *buf, int len)
     int i = gnutls_record_send(session, buf, len);
 #endif
 
+    if (i > 0) {
+        debugs(83, 8, "TLS FD " << fd << " session=" << (void*)session << " " << i << " bytes");
+    }
     return i;
 }
 #endif
@@ -92,7 +96,11 @@ CreateSession(const Security::ContextPointer &ctx, const Comm::ConnectionPointer
     const char *errAction = "with no TLS/SSL library";
     int errCode = 0;
 #if USE_OPENSSL
-    Security::SessionPointer session(SSL_new(ctx.get()));
+    Security::SessionPointer session(SSL_new(ctx.get()), [](SSL *p) {
+            debugs(83, 5, "SSL_free session=" << (void*)p);
+            SSL_free(p);
+        });
+    debugs(83, 5, "SSL_new session=" << (void*)session.get());
     if (!session) {
         errCode = ERR_get_error();
         errAction = "failed to allocate handle";
@@ -100,7 +108,11 @@ CreateSession(const Security::ContextPointer &ctx, const Comm::ConnectionPointer
 #elif USE_GNUTLS
     gnutls_session_t tmp;
     errCode = gnutls_init(&tmp, static_cast<unsigned int>(type));
-    Security::SessionPointer session(tmp);
+    Security::SessionPointer session(tmp, [](gnutls_session_t p) {
+            debugs(83, 5, "gnutls_deinit session=" << (void*)p);
+            gnutls_deinit(p); }
+    });
+    debugs(83, 5, "gnutls_init session=" << (void*)session.get());
     if (errCode != GNUTLS_E_SUCCESS) {
         session.reset();
         errAction = "failed to initialize session";
@@ -119,6 +131,7 @@ CreateSession(const Security::ContextPointer &ctx, const Comm::ConnectionPointer
         if (errCode == GNUTLS_E_SUCCESS) {
 #endif
 
+            debugs(83, 5, "link FD " << fd << " to TLS session=" << (void*)session.get());
             fd_table[fd].ssl = session;
             fd_table[fd].read_method = &tls_read_method;
             fd_table[fd].write_method = &tls_write_method;
@@ -153,16 +166,27 @@ Security::CreateServerSession(const Security::ContextPointer &ctx, const Comm::C
 }
 
 void
-Security::SessionClose(const Security::SessionPointer &s)
+Security::SessionClose(const Security::SessionPointer &s, const int fdOnError)
 {
     debugs(83, 5, "session=" << (void*)s.get());
-    if (s) {
+    if (s && fdOnError == -1) {
 #if USE_OPENSSL
         SSL_shutdown(s.get());
 #elif USE_GNUTLS
         gnutls_bye(s.get(), GNUTLS_SHUT_RDWR);
 #endif
     }
+
+#if USE_GNUTLS
+    // XXX: should probably be done for OpenSSL too, but that needs testing.
+    if (fdOnError != -1) {
+        debugs(83, 5, "unlink FD " << fdOnError << " from TLS session=" << (void*)fd_table[fdOnError].ssl.get());
+        fd_table[fdOnError].ssl.reset();
+        fd_table[fdOnError].read_method = &default_read_method;
+        fd_table[fdOnError].write_method = &default_write_method;
+        fd_note(fdOnError, "TLS error");
+    }
+#endif
 }
 
 bool

diff --git a/src/security/Session.h b/src/security/Session.h
index 182ca8da57805f612fdc2dc7982756704ee8e9e1..6dbb6db9c5714e89bf4e82c605227dca6e7a7cac 100644 (file)
--- a/src/security/Session.h
+++ b/src/security/Session.h
@@ -38,36 +38,28 @@ bool CreateClientSession(const Security::ContextPointer &, const Comm::Connectio
 bool CreateServerSession(const Security::ContextPointer &, const Comm::ConnectionPointer &, const char *squidCtx);
 
 #if USE_OPENSSL
-CtoCpp1(SSL_free, SSL *);
-#if defined(CRYPTO_LOCK_SSL) // OpenSSL 1.0
-inline int SSL_up_ref(SSL *t) {if (t) CRYPTO_add(&t->references, 1, CRYPTO_LOCK_SSL); return 0;}
-#endif
-typedef Security::LockingPointer<SSL, Security::SSL_free_cpp, HardFun<int, SSL *, SSL_up_ref> > SessionPointer;
+typedef std::shared_ptr<SSL> SessionPointer;
 
 typedef std::unique_ptr<SSL_SESSION, HardFun<void, SSL_SESSION*, &SSL_SESSION_free>> SessionStatePointer;
 
 #elif USE_GNUTLS
-// 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 Security::LockingPointer<struct gnutls_session_int, gnutls_deinit_cpp> SessionPointer;
+typedef std::shared_ptr<struct gnutls_session_int> 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
-CtoCpp1(xfree, void *);
-typedef Security::LockingPointer<void, xfree_cpp> SessionPointer;
+typedef std::shared_ptr<void> SessionPointer;
 
 typedef std::unique_ptr<int> SessionStatePointer;
 
 #endif
 
-/// close an active TLS session
-void SessionClose(const Security::SessionPointer &);
+/// close an active TLS session.
+/// set fdOnError to the connection FD when the session is being closed
+/// due to an encryption error, otherwise omit.
+void SessionClose(const Security::SessionPointer &, int fdOnError = -1);
 
 /// whether the session is a resumed one
 bool SessionIsResumed(const Security::SessionPointer &);