Fix issuer subject_hash ambiguities & bogus results with unknown issuers

application/clicommands/CheckCommand.php

@@ -1,4 +1,4 @@
 <?php
 
 // Icinga Web 2 X.509 Module | (c) 2019 Icinga GmbH | GPLv2
 
@@ -68,37 +68,30 @@
             exit(3);
         }
 
-        $targets = X509Target::on(Database::get())->with([
-            'chain',
-            'chain.certificate',
-            'chain.certificate.issuer_certificate'
-        ]);
-
-        $targets->getWith()['target.chain.certificate.issuer_certificate']->setJoinType('LEFT');
+        $targets = X509Target::on(Database::get())
+            ->with(['chain', 'chain.certificate'])
+            ->without('target.chain.certificate.issuer_certificate');
 
         $targets->columns([
             'port',
             'chain.valid',
             'chain.invalid_reason',
             'subject'     => 'chain.certificate.subject',
-            'self_signed' => new Expression('COALESCE(%s, %s)', [
-                'chain.certificate.issuer_certificate.self_signed',
-                'chain.certificate.self_signed'
-            ])
+            'self_signed' => 'chain.certificate.self_signed'
         ]);
 
         // Sub query for `valid_from` column
         $validFrom = $targets->createSubQuery(new X509Certificate(), 'chain.certificate');
         $validFrom
-            ->columns([new Expression('MAX(GREATEST(%s, %s))', ['valid_from', 'issuer_certificate.valid_from'])])
+            ->columns([new Expression('MAX(%s)', ['valid_from'])])
             ->getSelectBase()
             ->resetWhere()
             ->where(new Expression('sub_certificate_link.certificate_chain_id = target_chain.id'));
 
         // Sub query for `valid_to` column
         $validTo = $targets->createSubQuery(new X509Certificate(), 'chain.certificate');
         $validTo
-            ->columns([new Expression('MIN(LEAST(%s, %s))', ['valid_to', 'issuer_certificate.valid_to'])])
+            ->columns([new Expression('MIN(%s)', ['valid_to'])])
             ->getSelectBase()
             // Reset the where clause generated within the createSubQuery() method.
             ->resetWhere()
@@ -107,21 +100,24 @@
         list($validFromSelect, $_) = $validFrom->dump();
         list($validToSelect, $_) = $validTo->dump();
         $targets
+            // When the host or IP being checked is part of multiple targets and the user did not provide a filter
+            // on a specific port, we want to render the result with least valid_to timestamp.
+            ->orderBy('valid_to', SORT_DESC)
             ->withColumns([
                 'valid_from' => new Expression($validFromSelect),
                 'valid_to'   => new Expression($validToSelect)
             ])
             ->getSelectBase()
             ->where(new Expression('target_chain_link.order = 0'));
 
         if ($ip !== null) {
             $targets->filter(Filter::equal('ip', $ip));
         }
         if ($hostname !== null) {
             $targets->filter(Filter::equal('hostname', $hostname));
         }
         if ($this->params->has('port')) {
             $targets->filter(Filter::equal('port', $this->params->get('port')));
         }
 
         $allowSelfSigned = (bool) $this->params->get('allow-self-signed', false);
@@ -133,8 +129,8 @@
 
         $state = 3;
         foreach ($targets as $target) {
             if (! $target->chain->valid && (! $target['self_signed'] || ! $allowSelfSigned)) {
                 $invalidMessage = $target['subject'] . ': ' . $target->chain->invalid_reason;
                 $output[$invalidMessage] = $invalidMessage;
                 $state = 2;
             }

application/clicommands/ImportCommand.php

@@ -41,15 +41,15 @@
             foreach ($bundle as $data) {
                 $cert = openssl_x509_read($data);
 
-                list($id, $_) = CertificateUtils::findOrInsertCert($db, $cert);
+                $certInfo = CertificateUtils::findOrInsertCert($db, $cert);
 
                 $db->update(
                     'x509_certificate',
                     [
                         'trusted' => 'y',
                         'mtime'   => new Expression('UNIX_TIMESTAMP() * 1000')
                     ],
-                    ['id = ?' => $id]
+                    ['id = ?' => $certInfo->certId]
                 );
 
                 $count++;

library/X509/CertificateUtils.php

@@ -1,4 +1,4 @@
 <?php
 
 // Icinga Web 2 X.509 Module | (c) 2018 Icinga GmbH | GPLv2
 
@@ -206,7 +206,7 @@
      * @param   Connection  $db
      * @param   mixed       $cert
      *
-     * @return  array
+     * @return object
      */
     public static function findOrInsertCert(Connection $db, $cert)
     {
@@ -223,7 +223,7 @@
 
         $row = $row->first();
         if ($row) {
-            return [$row->id, $row->issuer_hash];
+            return (object)['certId' => $row->id, 'issuerHash' => $row->issuer_hash, 'fingerprint' => $fingerprint];
         }
 
         Logger::debug("Importing certificate: %s", $certInfo['name']);
@@ -281,7 +281,7 @@
 
         CertificateUtils::insertSANs($db, $certId, $sans);
 
-        return [$certId, $issuerHash];
+        return (object)['certId' => $certId, 'issuerHash' => $issuerHash, 'fingerprint' => $fingerprint];
     }
 
     private static function insertSANs($db, $certId, iterable $sans): void

library/X509/Job.php

@@ -17,6 +17,8 @@
 use Icinga\Module\X509\Model\X509Target;
 use Icinga\Module\X509\React\StreamOptsCaptureConnector;
 use Icinga\Util\Json;
+use ipl\Orm\Behavior\Binary;
+use ipl\Orm\Query;
 use ipl\Scheduler\Common\TaskProperties;
 use ipl\Scheduler\Contract\Task;
 use ipl\Sql\Connection;
@@ -696,34 +698,38 @@
 
                 $chainId = $this->db->lastInsertId();
 
-                $lastCertInfo = [];
+                $lastCertInfo = null;
                 foreach ($chain as $index => $cert) {
                     $lastCertInfo = CertificateUtils::findOrInsertCert($this->db, $cert);
-                    list($certId, $_) = $lastCertInfo;
-
                     $this->db->insert(
                         'x509_certificate_chain_link',
                         [
                             'certificate_chain_id'              => $chainId,
                             $this->db->quoteIdentifier('order') => $index,
-                            'certificate_id'                    => $certId,
+                            'certificate_id'                    => $lastCertInfo->certId,
                             'ctime'                             => new Expression('UNIX_TIMESTAMP() * 1000')
                         ]
                     );
 
-                    $lastCertInfo[] = $index;
+                    $lastCertInfo->order = $index;
                 }
 
                 // There might be chains that do not include the self-signed top-level Ca,
                 // so we need to include it manually here, as we need to display the full
                 // chain in the UI.
+                $binaryBehavior = (new Binary([]))->setQuery((new Query())->setDb($this->db));
                 $rootCa = X509Certificate::on($this->db)
                     ->columns(['id'])
-                    ->filter(Filter::equal('subject_hash', $lastCertInfo[1]))
+                    ->filter(Filter::equal('subject_hash', $lastCertInfo->issuerHash))
                     ->filter(Filter::equal('self_signed', true))
+                    // Since we don't enforce the subject_hash of the certificates to be unambiguous, we might end up
+                    // with more than one self-signed CA with the same hash and CN but different validity timestamps,
+                    // and in such situations we need to make sure to retrieve the correct certificate (the one
+                    // containing the expected fingerprint).
+                    ->orderBy(new Expression('%s = ?', ['fingerprint'], $binaryBehavior->toDb($lastCertInfo->fingerprint, 'fingerprint', '')), SORT_DESC)
                     ->first();
 
-                if ($rootCa && $rootCa->id !== $lastCertInfo[0]) {
+                if ($rootCa && $rootCa->id !== (int) $lastCertInfo->certId) {
                     $this->db->update(
                         'x509_certificate_chain',
                         ['length' => count($chain) + 1],
@@ -734,7 +740,7 @@
                         'x509_certificate_chain_link',
                         [
                             'certificate_chain_id'              => $chainId,
-                            $this->db->quoteIdentifier('order') => $lastCertInfo[2] + 1,
+                            $this->db->quoteIdentifier('order') => $lastCertInfo->order + 1,
                             'certificate_id'                    => $rootCa->id,
                             'ctime'                             => new Expression('UNIX_TIMESTAMP() * 1000')
                         ]

library/X509/Web/Control/SearchBar/ObjectSuggestions.php

@@ -2,6 +2,7 @@
 
 namespace Icinga\Module\X509\Web\Control\SearchBar;
 
+use DateTime;
 use Exception;
 use Icinga\Module\X509\Common\Database;
 use ipl\Orm\Exception\InvalidColumnException;
@@ -137,7 +138,7 @@ protected function fetchValueSuggestions($column, $searchTerm, Filter\Chain $sea
                     $value = $value ? 'y' : 'n';
                 }
 
-                yield $value;
+                yield $value instanceof DateTime ? $value->getTimestamp() :  $value;
             }
         } catch (InvalidColumnException $e) {
             throw new SearchException(sprintf(t('"%s" is not a valid column'), $e->getColumn()));

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