Detail certificate validation errors during TLS handshake (#770)

Fix certificate validation error handling in Security::Connect/Accept().
The existing validation details were not propagated/copied to IoResult,
requiring the caller to extract them via ssl_ex_index_ssl_error_detail.
The clunky approach even required a special "ErrorDetail generations"
API to figure out which error detail is "primary": the one received in
IoResult or the just extracted one. That API is removed now.

This change is used by the upcoming improvements that fetch missing TLS
v1.3 server certificates, but it also has an immediate positive effect
on the existing reporting of the client certificate validation errors.
Currently, only a general TLS error is reported for those cases because
Security::Accept() code forgot to check ssl_ex_index_ssl_error_detail.

This is a Measurement Factory project.

diff --git a/src/security/ErrorDetail.cc b/src/security/ErrorDetail.cc
index e835442e1a77111dc6df582d358d452617f95f82..8aa54886179a25c5b794f961db77950bc7e750d6 100644 (file)
--- a/src/security/ErrorDetail.cc
+++ b/src/security/ErrorDetail.cc
@@ -438,11 +438,8 @@ Security::ErrorNameFromCode(const ErrorCode err, const bool prefixRawCode)
 
 /* Security::ErrorDetail */
 
-uint64_t Security::ErrorDetail::Generations = 0;
-
 /// helper constructor implementing the logic shared by the two public ones
 Security::ErrorDetail::ErrorDetail(const ErrorCode err, const int aSysErrorNo):
-    generation(++Generations),
     error_no(err),
     // We could restrict errno(3) collection to cases where the TLS library
     // explicitly talks about the errno being set, but correctly detecting those

diff --git a/src/security/ErrorDetail.h b/src/security/ErrorDetail.h
index bc5e9a218f3d3718e6dbe4dd2b21748d4e94487f..0583ef2456da18f176432e74110f72df480b3fa7 100644 (file)
--- a/src/security/ErrorDetail.h
+++ b/src/security/ErrorDetail.h
@@ -55,12 +55,6 @@ public:
     ErrorDetail(ErrorCode anErrorCode, LibErrorCode aLibErrorNo, int aSysErrorNo);
 #endif
 
-    /// \returns whether we (rather than `them`) should detail ErrorState
-    bool takesPriorityOver(const ErrorDetail &them) const {
-        // to reduce pointless updates, return false if us is them
-        return this->generation < them.generation;
-    }
-
     /* ErrorDetail API */
     virtual SBuf brief() const;
     virtual SBuf verbose(const HttpRequestPointer &) const;
@@ -97,9 +91,6 @@ private:
     const char *err_lib_error() const;
     size_t convert(const char *code, const char **value) const;
 
-    static uint64_t Generations; ///< the total number of ErrorDetails ever made
-    uint64_t generation; ///< the number of ErrorDetails made before us plus one
-
     CertPointer peer_cert; ///< A pointer to the peer certificate
     CertPointer broken_cert; ///< A pointer to the broken certificate (peer or intermediate)
 

diff --git a/src/security/Io.cc b/src/security/Io.cc
index 5a189259f645ba19bbfb4d57cbadc093ccfa7515..0a2bec13c9af21a06c55f874cff0dc5f76d69817 100644 (file)
--- a/src/security/Io.cc
+++ b/src/security/Io.cc
@@ -84,9 +84,14 @@ Security::Handshake(Comm::Connection &transport, const ErrorCode topError, Fun i
     }
 
     // now we know that we are dealing with a real problem; detail it
-    const ErrorDetail::Pointer errorDetail =
-        new ErrorDetail(topError, ioError, xerrno);
-
+    ErrorDetail::Pointer errorDetail;
+    if (const auto oldDetail = SSL_get_ex_data(connection, ssl_ex_index_ssl_error_detail)) {
+        errorDetail = *static_cast<ErrorDetail::Pointer*>(oldDetail);
+    } else {
+        errorDetail = new ErrorDetail(topError, ioError, xerrno);
+        if (const auto serverCert = SSL_get_peer_certificate(connection))
+            errorDetail->setPeerCertificate(CertPointer(serverCert));
+    }
     IoResult ioResult(errorDetail);
 
     // collect debugging-related details

diff --git a/src/security/PeerConnector.cc b/src/security/PeerConnector.cc
index dfef50e986827c309d9e513267563b4c2f8a0eb2..f4e4f37f35d70ea836bc3e40919c5920aaf8960c 100644 (file)
--- a/src/security/PeerConnector.cc
+++ b/src/security/PeerConnector.cc
@@ -415,28 +415,13 @@ Security::PeerConnector::noteWantWrite()
 }
 
 void
-Security::PeerConnector::noteNegotiationError(const Security::ErrorDetailPointer &callerDetail)
+Security::PeerConnector::noteNegotiationError(const Security::ErrorDetailPointer &detail)
 {
-    auto primaryDetail = callerDetail;
-#if USE_OPENSSL
-    const auto tlsConnection = fd_table[serverConnection()->fd].ssl.get();
-
-    // find the highest priority detail
-    if (const auto storedDetailRaw = SSL_get_ex_data(tlsConnection, ssl_ex_index_ssl_error_detail)) {
-        const auto &storedDetail = *static_cast<ErrorDetail::Pointer*>(storedDetailRaw);
-        if (storedDetail->takesPriorityOver(*primaryDetail))
-            primaryDetail = storedDetail;
-    }
-
-    if (!primaryDetail->peerCert()) {
-        if (const auto serverCert = SSL_get_peer_certificate(tlsConnection))
-            primaryDetail->setPeerCertificate(CertPointer(serverCert));
-    }
-#endif
-
     const auto anErr = ErrorState::NewForwarding(ERR_SECURE_CONNECT_FAIL, request, al);
-    anErr->xerrno = primaryDetail->sysError();
-    anErr->detailError(primaryDetail);
+    if (detail) {
+        anErr->xerrno = detail->sysError();
+        anErr->detailError(detail);
+    }
     noteNegotiationDone(anErr);
     bail(anErr);
 }

diff --git a/src/ssl/PeekingPeerConnector.cc b/src/ssl/PeekingPeerConnector.cc
index ecf0720a2e1d68066760b72cf2fb7d1395f3ab40..67eb9e13287e0db929028a6ac991cfcae4bd36f6 100644 (file)
--- a/src/ssl/PeekingPeerConnector.cc
+++ b/src/ssl/PeekingPeerConnector.cc
@@ -338,6 +338,9 @@ Ssl::PeekingPeerConnector::noteNegotiationError(const Security::ErrorDetailPoint
     // thus hiding them.
     // Abort if no certificate found probably because of malformed or
     // unsupported server Hello message (TODO: make configurable).
+    // TODO: Add/use a positive "successfully validated server cert" signal
+    // instead of relying on the "![presumably_]validation_error && serverCert"
+    // signal combo.
     if (!SSL_get_ex_data(session.get(), ssl_ex_index_ssl_error_detail) &&
             (srvBio->bumpMode() == Ssl::bumpPeek  || srvBio->bumpMode() == Ssl::bumpStare) && srvBio->holdWrite()) {
         Security::CertPointer serverCert(SSL_get_peer_certificate(session.get()));