Sort CA certificates in tls-cert=bundle (#1177)

... as opposed to requiring the bundle to contain CA certificates in the
correct on-the-wire/issuing order and warning about (and then ignoring)
out-of-order bundled certificates.

This enhancement makes it easier to configure Squid to send the right
intermediate certificates, especially during certificate upgrades when
intermediate certificates (which may be obtained from a source not
tightly coordinated with the signing certificate source) is likely to
contain a mix of old and new intermediate certificates.

Squid has to (and did) check the certificate issuing order anyway. The
new code uses very similar checks to sort intermediate certificates.

No on-the-wire changes expected for Squid configurations that have
correctly ordered certificate bundles. Other configurations may start
sending the previously missing intermediate certificates.

diff --git a/src/security/KeyData.cc b/src/security/KeyData.cc
index de083e90e630f8cb4652164537b55020fe9ebdb9..80dc696b768580a5517d81100ad754d44d012983 100644 (file)
--- a/src/security/KeyData.cc
+++ b/src/security/KeyData.cc
@@ -44,32 +44,40 @@ Security::KeyData::loadCertificates()
     }
 
     try {
-        // selected bundled certificates in sending order: wireCerts = cert + chain
-        CertList wireCerts(1, cert);
-
-        while (const auto bundledCert = Ssl::ReadOptionalCertificate(bio)) {
-            assert(!wireCerts.empty()); // this->cert is there (at least)
-
-            // We cannot chain any certificate after a self-signed certificate. This
-            // check also protects the IssuedBy() check below from adding duplicated
-            // (i.e. listed multiple times in the bundle) self-signed certificates.
-            if (SelfSigned(*wireCerts.back())) {
-                debugs(83, DBG_PARSE_NOTE(2), "WARNING: Ignoring certificate after a self-signed one: " << *bundledCert);
-                continue; // ... but keep going to report all ignored certificates
-            }
-
-            // add the bundled certificate if it extends the chain further
-            if (IssuedBy(*wireCerts.back(), *bundledCert)) {
-                debugs(83, DBG_PARSE_NOTE(3), "Adding issuer CA: " << *bundledCert);
-                // OpenSSL API requires that we order certificates such that the
-                // chain can be appended directly into the on-wire traffic.
-                chain.emplace_back(bundledCert);
-                wireCerts.emplace_back(bundledCert);
-                continue;
-            }
-
-            debugs(83, DBG_PARSE_NOTE(2), "WARNING: Ignoring certificate that does not extend the chain: " << *bundledCert);
+        // Squid sends `cert` (loaded above) followed by certificates in `chain`
+        // (formed below by loading and sorting the remaining certificates).
+
+        // load all the remaining configured certificates
+        CertList candidates;
+        while (const auto c = Ssl::ReadOptionalCertificate(bio))
+            candidates.emplace_back(c);
+
+        // Push certificates into `chain` in on-the-wire order, as defined by
+        // RFC 8446 Section 4.4.2: "Each following certificate SHOULD directly
+        // certify the one immediately preceding it."
+        while (!candidates.empty()) {
+            const auto precedingCert = chain.empty() ? cert : chain.back();
+
+            // We cannot chain any certificate after a self-signed certificate.
+            // This check also protects the IssuedBy() search below from adding
+            // duplicated (i.e. listed multiple times) self-signed certificates.
+            if (SelfSigned(*precedingCert))
+                break;
+
+            const auto issuerPos = std::find_if(candidates.begin(), candidates.end(), [&](const CertPointer &i) {
+                return IssuedBy(*precedingCert, *i);
+            });
+            if (issuerPos == candidates.end())
+                break;
+
+            const auto &issuer = *issuerPos;
+            debugs(83, DBG_PARSE_NOTE(3), "Adding CA certificate: " << *issuer);
+            chain.emplace_back(issuer);
+            candidates.erase(issuerPos);
         }
+
+        for (const auto &c: candidates)
+            debugs(83, DBG_IMPORTANT, "WARNING: Ignoring certificate that does not extend the chain: " << *c);
     }
     catch (...) {
         // TODO: Reject configs with malformed intermediate certs instead.