From 29ef656e323d27e842cdd4ab9fdd2806b74e9df6 Mon Sep 17 00:00:00 2001 From: Yonas Habteab Date: Tue, 14 May 2024 16:34:21 +0200 Subject: [PATCH 1/7] Fix `1 !== "1"` strict comp mismatch --- library/X509/Job.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/X509/Job.php b/library/X509/Job.php index 1e0b3f73..e800a8ee 100644 --- a/library/X509/Job.php +++ b/library/X509/Job.php @@ -723,7 +723,7 @@ protected function processChain($target, $chain) ->filter(Filter::equal('self_signed', true)) ->first(); - if ($rootCa && $rootCa->id !== $lastCertInfo[0]) { + if ($rootCa && $rootCa->id !== (int) $lastCertInfo[0]) { $this->db->update( 'x509_certificate_chain', ['length' => count($chain) + 1], From ea6dccdc5df5967e1103457c1fffd93590d658fc Mon Sep 17 00:00:00 2001 From: Yonas Habteab Date: Mon, 13 May 2024 13:08:44 +0200 Subject: [PATCH 2/7] Fix phpstan errors --- phpstan-baseline-common.neon | 21 +++------------------ 1 file changed, 3 insertions(+), 18 deletions(-) diff --git a/phpstan-baseline-common.neon b/phpstan-baseline-common.neon index 651eb564..6d8c88ce 100644 --- a/phpstan-baseline-common.neon +++ b/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 From cb20034d06f0cee40608b5de6b8ffd33c23a5703 Mon Sep 17 00:00:00 2001 From: Yonas Habteab Date: Thu, 23 Jan 2025 15:29:29 +0100 Subject: [PATCH 3/7] Fix ambiguous issuer `subject_hash` problems --- application/clicommands/ImportCommand.php | 4 ++-- library/X509/CertificateUtils.php | 6 +++--- library/X509/Job.php | 22 ++++++++++++++-------- 3 files changed, 19 insertions(+), 13 deletions(-) diff --git a/application/clicommands/ImportCommand.php b/application/clicommands/ImportCommand.php index 2e7b1578..56c2bbb4 100644 --- a/application/clicommands/ImportCommand.php +++ b/application/clicommands/ImportCommand.php @@ -41,7 +41,7 @@ public function indexAction() foreach ($bundle as $data) { $cert = openssl_x509_read($data); - list($id, $_) = CertificateUtils::findOrInsertCert($db, $cert); + $certInfo = CertificateUtils::findOrInsertCert($db, $cert); $db->update( 'x509_certificate', @@ -49,7 +49,7 @@ public function indexAction() 'trusted' => 'y', 'mtime' => new Expression('UNIX_TIMESTAMP() * 1000') ], - ['id = ?' => $id] + ['id = ?' => $certInfo->certId] ); $count++; diff --git a/library/X509/CertificateUtils.php b/library/X509/CertificateUtils.php index e524024e..164c77c3 100644 --- a/library/X509/CertificateUtils.php +++ b/library/X509/CertificateUtils.php @@ -206,7 +206,7 @@ public static function parseBundle($file) * @param Connection $db * @param mixed $cert * - * @return array + * @return object */ public static function findOrInsertCert(Connection $db, $cert) { @@ -223,7 +223,7 @@ public static function findOrInsertCert(Connection $db, $cert) $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 @@ public static function findOrInsertCert(Connection $db, $cert) 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 diff --git a/library/X509/Job.php b/library/X509/Job.php index e800a8ee..78468a15 100644 --- a/library/X509/Job.php +++ b/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 @@ protected function processChain($target, $chain) $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 !== (int) $lastCertInfo[0]) { + if ($rootCa && $rootCa->id !== (int) $lastCertInfo->certId) { $this->db->update( 'x509_certificate_chain', ['length' => count($chain) + 1], @@ -734,7 +740,7 @@ protected function processChain($target, $chain) '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') ] From 8a1c49a181c66362a3bd46abca9dfd133eb5a858 Mon Sep 17 00:00:00 2001 From: Yonas Habteab Date: Thu, 23 Jan 2025 15:33:58 +0100 Subject: [PATCH 4/7] CheckCommand: Drop superfluous `issuer_certificate` joins It doesn't make sense to directly join on the `issuer` here at all. The join was utilized to determine if the certificate with `link.order = 0` is self-seigned, but this often results in incorrect conclusions, as most of the issuers are self-signed. Instead, the `self_signed` column available for each certificate should be used for this purpose. --- application/clicommands/CheckCommand.php | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/application/clicommands/CheckCommand.php b/application/clicommands/CheckCommand.php index 0c369d9c..596b76d8 100644 --- a/application/clicommands/CheckCommand.php +++ b/application/clicommands/CheckCommand.php @@ -68,23 +68,16 @@ public function hostAction() 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 From 1fb40f961fe3d4662968770ca06986a8ed94efe4 Mon Sep 17 00:00:00 2001 From: Yonas Habteab Date: Thu, 23 Jan 2025 15:45:35 +0100 Subject: [PATCH 5/7] CheckCommand: Remove superfluous `LEAST()/GREATEST()` usages --- application/clicommands/CheckCommand.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/application/clicommands/CheckCommand.php b/application/clicommands/CheckCommand.php index 596b76d8..947933f4 100644 --- a/application/clicommands/CheckCommand.php +++ b/application/clicommands/CheckCommand.php @@ -83,7 +83,7 @@ public function hostAction() // 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')); @@ -91,7 +91,7 @@ public function hostAction() // 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() From 0fa141d50392597277fb08ca67fb94f2b206de9b Mon Sep 17 00:00:00 2001 From: Yonas Habteab Date: Thu, 23 Jan 2025 16:03:59 +0100 Subject: [PATCH 6/7] Don't suggest `DateTime` objects --- library/X509/Web/Control/SearchBar/ObjectSuggestions.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/library/X509/Web/Control/SearchBar/ObjectSuggestions.php b/library/X509/Web/Control/SearchBar/ObjectSuggestions.php index ca9630f3..3187cae2 100644 --- a/library/X509/Web/Control/SearchBar/ObjectSuggestions.php +++ b/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())); From b0203ab759a3fccc8fba0758faf906b5eb8d6517 Mon Sep 17 00:00:00 2001 From: Yonas Habteab Date: Thu, 23 Jan 2025 17:07:27 +0100 Subject: [PATCH 7/7] CheckCommand: Process duplicate certs from greatest to least --- application/clicommands/CheckCommand.php | 3 +++ 1 file changed, 3 insertions(+) diff --git a/application/clicommands/CheckCommand.php b/application/clicommands/CheckCommand.php index 947933f4..215a8150 100644 --- a/application/clicommands/CheckCommand.php +++ b/application/clicommands/CheckCommand.php @@ -100,6 +100,9 @@ public function hostAction() 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)