Bug 4831: filter chain certificates for validity when loading (#187)

51e09c08a5e6c582e7d93af99a8f2cfcb14ea9e6 adding
GnuTLS support required splitting the way
certificate chains were loaded. This resulted in the
leaf certificate being added twice at the prefix of a
chain in the serverHello.

It turns out that some recipients validate strictly that the
chain delivered by a serverHello does not contain extra
certificates and reject the handshake if they do.

This patch implements the XXX about filtering certificates
for chain sequence order and self-sign properties, added
in the initial PR. Resolving the bug 4831 regression and also
reporting failures at startup/reconfigure for admins.

Also, add debug display of certificate names for simpler
detection and administrative fix when loaded files fail
these tests.

diff --git a/src/security/KeyData.cc b/src/security/KeyData.cc
index 23d1239544aad9927c29d8c8dd22608eb1babbe0..052c64ffd182c97381dbb37f9245c94c162eeb84 100644 (file)
--- a/src/security/KeyData.cc
+++ b/src/security/KeyData.cc
@@ -86,8 +86,6 @@ void
 Security::KeyData::loadX509ChainFromFile()
 {
 #if USE_OPENSSL
-    // XXX: This BIO loads the public cert as first chain cert,
-    //      so the code appending chains sends it twice in handshakes.
     const char *certFilename = certFile.c_str();
     Ssl::BIO_Pointer bio(BIO_new(BIO_s_file()));
     if (!bio || !BIO_read_filename(bio.get(), certFilename)) {
@@ -96,14 +94,41 @@ Security::KeyData::loadX509ChainFromFile()
         return;
     }
 
-    if (X509_check_issued(cert.get(), cert.get()) == X509_V_OK)
-        debugs(83, 5, "Certificate is self-signed, will not be chained");
-    else {
+#if TLS_CHAIN_NO_SELFSIGNED // ignore self-signed certs in the chain
+    if (X509_check_issued(cert.get(), cert.get()) == X509_V_OK) {
+        char *nameStr = X509_NAME_oneline(X509_get_subject_name(cert.get()), nullptr, 0);
+        debugs(83, DBG_PARSE_NOTE(2), "Certificate is self-signed, will not be chained: " << nameStr);
+        OPENSSL_free(nameStr);
+    } else
+#endif
+    {
+        debugs(83, DBG_PARSE_NOTE(3), "Using certificate chain in " << certFile);
         // and add to the chain any other certificate exist in the file
-        while (X509 *ca = PEM_read_bio_X509(bio.get(), nullptr, nullptr, nullptr)) {
-            // XXX: self-signed check should be applied to all certs loaded.
-            // XXX: missing checks that the chained certs are actually part of a chain for validating cert.
-            chain.emplace_front(Security::CertPointer(ca));
+        CertPointer latestCert = cert;
+
+        while (auto ca = PEM_read_bio_X509(bio.get(), nullptr, nullptr, nullptr)) {
+            // get Issuer name of the cert for debug display
+            char *nameStr = X509_NAME_oneline(X509_get_subject_name(ca), nullptr, 0);
+
+#if TLS_CHAIN_NO_SELFSIGNED // ignore self-signed certs in the chain
+            // self-signed certificates are not valid in a sent chain
+            if (X509_check_issued(ca, ca) == X509_V_OK) {
+                debugs(83, DBG_PARSE_NOTE(2), "CA " << nameStr << " is self-signed, will not be chained: " << nameStr);
+                OPENSSL_free(nameStr);
+                continue;
+            }
+#endif
+            // checks that the chained certs are actually part of a chain for validating cert
+            if (X509_check_issued(ca, latestCert.get()) == X509_V_OK) {
+                debugs(83, DBG_PARSE_NOTE(3), "Adding issuer CA: " << nameStr);
+                // OpenSSL API requires that we order certificates such that the
+                // chain can be appended directly into the on-wire traffic.
+                latestCert = CertPointer(ca);
+                chain.emplace_front(latestCert);
+            } else {
+                debugs(83, DBG_PARSE_NOTE(2), "Ignoring non-issuer CA from " << certFile << ": " << nameStr);
+            }
+            OPENSSL_free(nameStr);
         }
     }