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 7bec4098d66b77e3d5a79bde7666a2172a5f9d6f..8803e2ca7b4f5f8200e67bbd722ff5ad05cdc094 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 89e3be994731809b4873e979f50c6f2d6baecb1b..4bb780b540e8003a357710fb4aa8ded07d01d180 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 cc648e4df4d28f44def6c0a25d0b3fb12d5cef0b..d6714049b32f5af9ec1702adb70b203bfb1f14b8 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 83986912fb7d555fcb183394f92b3cbd067ac5f8..8a2a69a1382324267681b9d2c4eacfcfac73b0b2 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 5ea4e4ba91b9cf0212147cb48eba8b4a11164b33..804148f24577e3a59743977b6ac461824f4b1791 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()));