Fix issuer subject_hash ambiguities & bogus results with unknown issuers

application/clicommands/CheckCommand.php

@@ -92,7 +92,16 @@ public function hostAction()
         $validFrom
             ->columns([new Expression('MAX(GREATEST(%s, %s))', ['valid_from', 'issuer_certificate.valid_from'])])
             ->getSelectBase()
+            // Reset the where clause generated within the createSubQuery() method.
             ->resetWhere()
+            // If the issuer of the current cert is an intermediate one, then we should take its valid_to
+            // timestamp into account unconditionally, ...
+            ->where(new Expression("sub_certificate_issuer_certificate.self_signed = 'n'"))
+            // ... otherwise, since we don't enforce the subject_hash of the certificates to be unambiguous,
+            // we might have more than one self-singed/trusted CA with the same CN and hash but different validity
+            // timestamps, and in such situations we need to make sure that we are joining to the correct CA that
+            // is part of the chain, and not some random one that seems to have the same subject_hash.
+            ->orWhere(new Expression('sub_certificate_link.certificate_id = sub_certificate_issuer_certificate.id'))
             ->where(new Expression('sub_certificate_link.certificate_chain_id = target_chain.id'));
 
         // Sub query for `valid_to` column
@@ -102,14 +111,30 @@ public function hostAction()
             ->getSelectBase()
             // Reset the where clause generated within the createSubQuery() method.
             ->resetWhere()
+            // If the issuer of the current cert is an intermediate one, then we should take its valid_to
+            // timestamp into account unconditionally, ...
+            ->where(new Expression("sub_certificate_issuer_certificate.self_signed = 'n'"))
+            // ... otherwise, since we don't enforce the subject_hash of the certificates to be unambiguous,
+            // we might have more than one self-singed/trusted CA with the same CN and hash but different validity
+            // timestamps, and in such situations we need to make sure that we are joining to the correct CA that
+            // is part of the chain, and not some random one that seems to have the same subject_hash.
+            ->orWhere(new Expression('sub_certificate_link.certificate_id = sub_certificate_issuer_certificate.id'))
             ->where(new Expression('sub_certificate_link.certificate_chain_id = target_chain.id'));
 
         list($validFromSelect, $_) = $validFrom->dump();
         list($validToSelect, $_) = $validTo->dump();
         $targets
             ->withColumns([
-                'valid_from' => new Expression($validFromSelect),
-                'valid_to'   => new Expression($validToSelect)
+                // If the current chains invalid reason contains "unable to get local issuer certificate", those
+                // valid_{to,from} subqueries will yield null timestamps, since they're using an inner join on the
+                // issuer, so coalescing it with its actual timestamp not produce bogus outputs like:
+                // CRITICAL - bogus: unable to get local issuer certificate; bogus has expired since 19976 days etc...
+                'valid_from' => new Expression(
+                    sprintf('COALESCE((%s), target_chain_certificate.valid_from)', $validFromSelect)
+                ),
+                'valid_to'   => new Expression(
+                    sprintf('COALESCE((%s), target_chain_certificate.valid_to)', $validToSelect)
+                )
             ])
             ->getSelectBase()
             ->where(new Expression('target_chain_link.order = 0'));

library/X509/Job.php

@@ -721,9 +721,11 @@ protected function processChain($target, $chain)
                     ->columns(['id'])
                     ->filter(Filter::equal('subject_hash', $lastCertInfo[1]))
                     ->filter(Filter::equal('self_signed', true))
+                    // If there are multiple CAs with the same subject hash, pick the newly imported one.
+                    ->orderBy('ctime', SORT_DESC)
                     ->first();
 
-                if ($rootCa && $rootCa->id !== $lastCertInfo[0]) {
+                if ($rootCa && $rootCa->id !== (int) $lastCertInfo[0]) {
                     $this->db->update(
                         'x509_certificate_chain',
                         ['length' => count($chain) + 1],

phpstan-baseline-common.neon

@@ -105,11 +105,6 @@ parameters:
 			count: 1
 			path: application/controllers/CertificateController.php
 
-		-
-			message: "#^Parameter \\#1 \\$cert of method Icinga\\\\Module\\\\X509\\\\CertificateDetails\\:\\:setCert\\(\\) expects Icinga\\\\Module\\\\X509\\\\Model\\\\X509Certificate, Icinga\\\\Module\\\\X509\\\\Model\\\\X509Certificate\\|null given\\.$#"
-			count: 1
-			path: application/controllers/CertificateController.php
-
 		-
 			message: "#^Parameter \\#2 \\$value of static method ipl\\\\Stdlib\\\\Filter\\:\\:equal\\(\\) expects array\\|bool\\|float\\|int\\|string, mixed given\\.$#"
 			count: 1
@@ -155,31 +150,21 @@ parameters:
 			count: 1
 			path: application/controllers/ChainController.php
 
-		-
-			message: "#^Cannot access property \\$target on Icinga\\\\Module\\\\X509\\\\Model\\\\X509CertificateChain\\|null\\.$#"
-			count: 3
-			path: application/controllers/ChainController.php
-
 		-
 			message: "#^Method Icinga\\\\Module\\\\X509\\\\Controllers\\\\ChainController\\:\\:indexAction\\(\\) has no return type specified\\.$#"
 			count: 1
 			path: application/controllers/ChainController.php
 
 		-
-			message: "#^Offset 'invalid_reason' does not exist on Icinga\\\\Module\\\\X509\\\\Model\\\\X509CertificateChain\\|null\\.$#"
-			count: 1
+			message: "#^Parameter \\#2 \\$value of static method ipl\\\\Stdlib\\\\Filter\\:\\:equal\\(\\) expects array\\|bool\\|float\\|int\\|string, mixed given\\.$#"
+			count: 2
 			path: application/controllers/ChainController.php
 
 		-
-			message: "#^Offset 'valid' does not exist on Icinga\\\\Module\\\\X509\\\\Model\\\\X509CertificateChain\\|null\\.$#"
+			message: "#^Parameter \\#2 \\.\\.\\.\\$values of function sprintf expects bool\\|float\\|int\\|string\\|null, mixed given\\.$#"
 			count: 1
 			path: application/controllers/ChainController.php
 
-		-
-			message: "#^Parameter \\#2 \\$value of static method ipl\\\\Stdlib\\\\Filter\\:\\:equal\\(\\) expects array\\|bool\\|float\\|int\\|string, mixed given\\.$#"
-			count: 2
-			path: application/controllers/ChainController.php
-
 		-
 			message: "#^Method Icinga\\\\Module\\\\X509\\\\Controllers\\\\ConfigController\\:\\:backendAction\\(\\) has no return type specified\\.$#"
 			count: 1