Do not send more than one self-signed certificate (#1058)

Configured chain L-M-R-R was sent as L-M-R-R instead of just L-M-R.

Configured chain R was sent as R-R instead of just R!

... where R is a self-signed (i.e. "Root CA") certificate that signed
(possibly indirectly, via an intermediate certificate M) the traffic
signing (i.e. "Leaf" or "end-entity") certificate L (if any).

Security::KeyData::loadX509ChainFromFile() has other problems, but
properly solving those problems needs significant code refactoring. It
is best done in a dedicated no-functionality-changes PR. The same
refactoring is likely to simplify the existing code logic, handling all
root certificates in one place.

Also removed disabled code so that we do not have to keep maintaining
it. Incorrectly disabling this code at the very end of commit 540e296
work created the bugs we are fixing in this branch. For some, it is
easier to comprehend active code without disabled code misdirection.
This disabled code removal does _not_ mean that Squid should keep
sending Root CA certificates (when they are not the signing/end-entity
certificates). However, stopping that behavior deserves a dedicated PR.

diff --git a/src/security/KeyData.cc b/src/security/KeyData.cc
index e12d0bbba8660bebdbbbe160674a45d91ea9f6c0..73dc5ab23d7e3b07f497e0bbeb1b8004f28bb261 100644 (file)
--- a/src/security/KeyData.cc
+++ b/src/security/KeyData.cc
@@ -97,27 +97,25 @@ Security::KeyData::loadX509ChainFromFile()
         return;
     }
 
-#if TLS_CHAIN_NO_SELFSIGNED // ignore self-signed certs in the chain
     if (SelfSigned(*cert)) {
-        debugs(83, DBG_PARSE_NOTE(2), "Certificate is self-signed, will not be chained: " << *cert);
-    } else
-#endif
-    {
+        debugs(83, DBG_PARSE_NOTE(2), "Signing certificate is self-signed: " << *cert);
+        // TODO: Warn if there are other (unusable) certificates present.
+    } else {
         debugs(83, DBG_PARSE_NOTE(3), "Using certificate chain in " << certFile);
         // and add to the chain any other certificate exist in the file
         CertPointer latestCert = cert;
 
         while (const auto ca = Ssl::ReadOptionalCertificate(bio)) {
-
-#if TLS_CHAIN_NO_SELFSIGNED // ignore self-signed certs in the chain
-            // self-signed certificates are not valid in a sent chain
-            if (SelfSigned(*ca)) {
-                debugs(83, DBG_PARSE_NOTE(2), "CA certificate is self-signed, will not be chained: " << *ca);
-                continue;
-            }
-#endif
             // checks that the chained certs are actually part of a chain for validating cert
             if (IssuedBy(*latestCert, *ca)) {
+
+                if (SelfSigned(*latestCert)) { // TODO: Rename to lastChained
+                    Assure(SelfSigned(*ca));
+                    Assure(!SelfSigned(*cert)); // TODO: Rename to leafCert or signingCert
+                    debugs(83, DBG_PARSE_NOTE(2), "WARNING: Ignoring repeated Root CA: " << *ca);
+                    continue;
+                }
+
                 debugs(83, DBG_PARSE_NOTE(3), "Adding issuer CA: " << *ca);
                 // OpenSSL API requires that we order certificates such that the
                 // chain can be appended directly into the on-wire traffic.