From a13359b700077b622b6f6fa288c2cc542dfa1124 Mon Sep 17 00:00:00 2001 From: Yonas Habteab Date: Mon, 13 May 2024 12:23:38 +0200 Subject: [PATCH 1/4] `CheckCommand`: Don't output bougus result with unknown issuers --- application/clicommands/CheckCommand.php | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/application/clicommands/CheckCommand.php b/application/clicommands/CheckCommand.php index 0c369d9c..053ef9a4 100644 --- a/application/clicommands/CheckCommand.php +++ b/application/clicommands/CheckCommand.php @@ -108,8 +108,16 @@ public function hostAction() 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')); From eb71e006a9206872c97824b458fd0d09e9982a8b Mon Sep 17 00:00:00 2001 From: Yonas Habteab Date: Mon, 13 May 2024 13:08:44 +0200 Subject: [PATCH 2/4] 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 d797b572e898035d24fd3ffff49036026ea22fda Mon Sep 17 00:00:00 2001 From: Yonas Habteab Date: Tue, 14 May 2024 16:34:21 +0200 Subject: [PATCH 3/4] 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 ab0edff7d34bf4a9581ec71177f72d86f09c1588 Mon Sep 17 00:00:00 2001 From: Yonas Habteab Date: Tue, 10 Sep 2024 13:13:41 +0200 Subject: [PATCH 4/4] Fix issuer `subject_hash` ambiguities --- application/clicommands/CheckCommand.php | 17 +++++++++++++++++ library/X509/Job.php | 2 ++ 2 files changed, 19 insertions(+) diff --git a/application/clicommands/CheckCommand.php b/application/clicommands/CheckCommand.php index 053ef9a4..75e5d2da 100644 --- a/application/clicommands/CheckCommand.php +++ b/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,6 +111,14 @@ 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(); diff --git a/library/X509/Job.php b/library/X509/Job.php index e800a8ee..bebf57bb 100644 --- a/library/X509/Job.php +++ b/library/X509/Job.php @@ -721,6 +721,8 @@ 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 !== (int) $lastCertInfo[0]) {