From 7587a3546ba8913619dbceb9421bbf2b99d4fd87 Mon Sep 17 00:00:00 2001 From: Yonas Habteab Date: Thu, 24 Nov 2022 13:14:24 +0100 Subject: [PATCH] Optimize imports & cleanup some codes --- application/clicommands/CheckCommand.php | 30 +++++++------------ .../controllers/CertificateController.php | 1 - .../controllers/CertificatesController.php | 21 +++++-------- application/controllers/UsageController.php | 23 +++++++------- library/X509/CertificateDetails.php | 4 +-- library/X509/DataTable.php | 2 +- library/X509/Job.php | 21 ++++++------- library/X509/Model/X509Certificate.php | 24 +++++++++++++++ .../X509/ProvidedHook/HostsImportSource.php | 16 +++++----- .../ProvidedHook/ServicesImportSource.php | 20 ++++++------- 10 files changed, 85 insertions(+), 77 deletions(-) diff --git a/application/clicommands/CheckCommand.php b/application/clicommands/CheckCommand.php index c2c014b6..fa4e0719 100644 --- a/application/clicommands/CheckCommand.php +++ b/application/clicommands/CheckCommand.php @@ -6,8 +6,8 @@ use Icinga\Application\Logger; use Icinga\Module\X509\Command; -use Icinga\Module\X509\Job; use Icinga\Module\X509\Model\X509Certificate; +use Icinga\Module\X509\Model\X509CertificateChain; use Icinga\Module\X509\Model\X509Target; use ipl\Sql\Expression; use ipl\Stdlib\Filter; @@ -89,36 +89,28 @@ public function hostAction() ]); // Sub queries for (valid_from, valid_to) columns - $validFrom = X509Certificate::on($conn) - ->with(['chain', 'issuer_certificate']) - ->columns([ - new Expression('MAX(GREATEST(%s, %s))', ['valid_from', 'issuer_certificate.valid_from']) - ]); - - $validFrom->getResolver()->setAliasPrefix('sub_'); + $validFrom = $targets->createSubQuery(new X509Certificate(), 'chain.certificate'); $validFrom + ->columns([new Expression('MAX(GREATEST(%s, %s))', ['valid_from', 'issuer_certificate.valid_from'])]) ->getSelectBase() - ->where(new Expression( - 'sub_certificate_link.certificate_chain_id = target_chain.id' - )); + ->resetWhere() + ->where(new Expression('sub_certificate_link.certificate_chain_id = target_chain.id')); $validTo = clone $validFrom; - $validTo->columns([ - new Expression('MIN(LEAST(%s, %s))', ['valid_to', 'issuer_certificate.valid_to']) - ]); + $validTo->columns([new Expression('MIN(LEAST(%s, %s))', ['valid_to', 'issuer_certificate.valid_to'])]); - list($validFromSelect, $validFromValues) = $validFrom->dump(); - list($validToSelect, $validToValues) = $validTo->dump(); + list($validFromSelect, $_) = $validFrom->dump(); + list($validToSelect, $_) = $validTo->dump(); $targets ->withColumns([ - 'valid_from' => new Expression("$validFromSelect", null, ...$validFromValues), - 'valid_to' => new Expression("$validToSelect", null, ...$validToValues) + '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', Job::binary($ip))); + $targets->filter(Filter::equal('ip', $ip)); } if ($hostname !== null) { $targets->filter(Filter::equal('hostname', $hostname)); diff --git a/application/controllers/CertificateController.php b/application/controllers/CertificateController.php index e166b46d..7bf68a6b 100644 --- a/application/controllers/CertificateController.php +++ b/application/controllers/CertificateController.php @@ -8,7 +8,6 @@ use Icinga\Module\X509\CertificateDetails; use Icinga\Module\X509\Controller; use Icinga\Module\X509\Model\X509Certificate; -use ipl\Sql; use ipl\Stdlib\Filter; class CertificateController extends Controller diff --git a/application/controllers/CertificatesController.php b/application/controllers/CertificatesController.php index 5a6e5eb3..44a171f8 100644 --- a/application/controllers/CertificatesController.php +++ b/application/controllers/CertificatesController.php @@ -62,6 +62,7 @@ public function indexAction() } else { $this->addControl($searchBar); $this->sendMultipartUpdate(); + return; } } else { @@ -77,14 +78,7 @@ public function indexAction() $this->addControl($limitControl); $this->addControl($searchBar); - // List of allowed columns to be exported - $exportable = array_flip([ - 'id', 'subject', 'issuer', 'version', 'self_signed', 'ca', 'trusted', - 'pubkey_algo', 'pubkey_bits', 'signature_algo', 'signature_hash_algo', - 'valid_from', 'valid_to' - ]); - - $this->handleFormatRequest($certificates, function (Query $certificates) use ($exportable) { + $this->handleFormatRequest($certificates, function (Query $certificates) { /** @var X509Certificate $cert */ foreach ($certificates as $cert) { $cert['valid_from'] = (new \DateTime()) @@ -94,7 +88,7 @@ public function indexAction() ->setTimestamp($cert['valid_to']) ->format('l F jS, Y H:i:s e'); - yield array_intersect_key(iterator_to_array($cert), $exportable); + yield array_intersect_key(iterator_to_array($cert), array_flip($cert->getExportableColumns())); } }); @@ -107,10 +101,11 @@ public function indexAction() public function completeAction() { - $suggestions = new ObjectSuggestions(); - $suggestions->setModel(X509Certificate::class); - $suggestions->forRequest($this->getServerRequest()); - $this->getDocument()->add($suggestions); + $this->getDocument()->add( + (new ObjectSuggestions()) + ->setModel(X509Certificate::class) + ->forRequest($this->getServerRequest()) + ); } public function searchEditorAction() diff --git a/application/controllers/UsageController.php b/application/controllers/UsageController.php index e02fcc3b..db169aba 100644 --- a/application/controllers/UsageController.php +++ b/application/controllers/UsageController.php @@ -78,6 +78,7 @@ public function indexAction() } else { $this->addControl($searchBar); $this->sendMultipartUpdate(); + return; } } else { @@ -93,13 +94,7 @@ public function indexAction() $this->addControl($limitControl); $this->addControl($searchBar); - $exportable = array_flip([ - 'valid', 'hostname', 'ip', 'port', 'subject', 'issuer', 'version', - 'self_signed', 'ca', 'trusted', 'pubkey_algo', 'pubkey_bits', - 'signature_algo', 'signature_hash_algo', 'valid_from', 'valid_to' - ]); - - $this->handleFormatRequest($targets, function (Query $targets) use ($conn, $exportable) { + $this->handleFormatRequest($targets, function (Query $targets) { foreach ($targets as $usage) { $usage['valid_from'] = (new \DateTime()) ->setTimestamp($usage['valid_from']) @@ -113,7 +108,10 @@ public function indexAction() $usage->port = $usage->chain->target->port; $usage->valid = $usage->chain->valid; - yield array_intersect_key(iterator_to_array($usage), $exportable); + yield array_intersect_key( + iterator_to_array($usage), + array_flip(array_merge(['valid', 'hostname', 'ip', 'port'], $usage->getExportableColumns())) + ); } }); @@ -126,10 +124,11 @@ public function indexAction() public function completeAction() { - $suggestions = new ObjectSuggestions(); - $suggestions->setModel(X509Certificate::class); - $suggestions->forRequest($this->getServerRequest()); - $this->getDocument()->add($suggestions); + $this->getDocument()->add( + (new ObjectSuggestions()) + ->setModel(X509Certificate::class) + ->forRequest($this->getServerRequest()) + ); } public function searchEditorAction() diff --git a/library/X509/CertificateDetails.php b/library/X509/CertificateDetails.php index 0ed136da..919a7664 100644 --- a/library/X509/CertificateDetails.php +++ b/library/X509/CertificateDetails.php @@ -5,9 +5,9 @@ namespace Icinga\Module\X509; use DateTime; +use Icinga\Module\X509\Model\X509Certificate; use ipl\Html\BaseHtmlElement; use ipl\Html\Html; -use ipl\Orm\Model; /** * Widget to display X.509 certificate details @@ -23,7 +23,7 @@ class CertificateDetails extends BaseHtmlElement */ protected $cert; - public function setCert(Model $cert) + public function setCert(X509Certificate $cert) { $this->cert = $cert; diff --git a/library/X509/DataTable.php b/library/X509/DataTable.php index bc1cb7f3..8645fba8 100644 --- a/library/X509/DataTable.php +++ b/library/X509/DataTable.php @@ -84,7 +84,7 @@ protected function renderRow($row) $cells = []; foreach ($this->columns as $key => $column) { - if (! is_int($key) && isset($row->$key)) { + if (! is_int($key) && property_exists($row, $key)) { $data = $row[$key]; } else { $data = null; diff --git a/library/X509/Job.php b/library/X509/Job.php index 0fa4fc65..a4cf1222 100644 --- a/library/X509/Job.php +++ b/library/X509/Job.php @@ -199,7 +199,7 @@ protected function getScanTargets(): Generator // The filter processor doesn't allow us to filter using expressions $targets ->getSelectBase() - ->where(new Expression('last_scan < %d', [$this->sinceLastScan->getTimestamp()])); + ->where(['last_scan < ?' => $this->sinceLastScan->getTimestamp()]); } foreach ($targets as $target) { @@ -438,10 +438,10 @@ protected function processChain($target, $chain) $this->db->transaction(function () use ($target, $chain) { $row = X509Target::on($this->db)->columns(['id']); - $filter = Filter::all(); - $filter->add(Filter::equal('ip', static::binary($target->ip))); - $filter->add(Filter::equal('port', $target->port)); - $filter->add(Filter::equal('hostname', $target->hostname)); + $filter = Filter::all() + ->add(Filter::equal('ip', $target->ip)) + ->add(Filter::equal('port', $target->port)) + ->add(Filter::equal('hostname', $target->hostname)); $row->filter($filter); @@ -463,13 +463,14 @@ protected function processChain($target, $chain) $chainUptodate = false; - $lastChain = X509CertificateChain::on($this->db)->columns(['id']); - $lastChain + $lastChain = X509CertificateChain::on($this->db) + ->columns(['id']) ->filter(Filter::equal('target_id', $targetId)) ->orderBy('id', SORT_DESC) - ->limit(1); + ->limit(1) + ->first(); - if (($lastChain = $lastChain->first())) { + if ($lastChain) { $lastFingerprints = X509Certificate::on($this->db)->utilize('chain'); $lastFingerprints ->columns(['fingerprint']) @@ -495,7 +496,7 @@ protected function processChain($target, $chain) } if ($chainUptodate) { - $chainId = (int) $lastChain->id; + $chainId = $lastChain->id; } else { // TODO: https://github.com/Icinga/ipl-orm/pull/78 $this->db->insert( diff --git a/library/X509/Model/X509Certificate.php b/library/X509/Model/X509Certificate.php index 14f225aa..61060955 100644 --- a/library/X509/Model/X509Certificate.php +++ b/library/X509/Model/X509Certificate.php @@ -86,6 +86,30 @@ public function getSearchColumns() return ['subject', 'issuer']; } + /** + * Get list of allowed columns to be exported + * + * @return string[] + */ + public function getExportableColumns(): array + { + return [ + 'id', + 'subject', + 'issuer', + 'version', + 'self_signed', + 'ca', + 'trusted', + 'pubkey_algo', + 'pubkey_bits', + 'signature_algo', + 'signature_hash_algo', + 'valid_from', + 'valid_to' + ]; + } + public function createBehaviors(Behaviors $behaviors) { $behaviors->add(new Binary([ diff --git a/library/X509/ProvidedHook/HostsImportSource.php b/library/X509/ProvidedHook/HostsImportSource.php index ee4189e5..39fe33c7 100644 --- a/library/X509/ProvidedHook/HostsImportSource.php +++ b/library/X509/ProvidedHook/HostsImportSource.php @@ -4,11 +4,9 @@ namespace Icinga\Module\X509\ProvidedHook; -use Icinga\Module\X509\DbTool; use Icinga\Module\X509\Job; use Icinga\Module\X509\Model\X509Target; use ipl\Sql; -use ipl\Stdlib\Filter; class HostsImportSource extends X509ImportSource { @@ -29,11 +27,11 @@ public function fetchData() if ($this->getDb()->getAdapter() instanceof Sql\Adapter\Pgsql) { $targets->withColumns([ - 'host_ports' => new Sql\Expression('ARRAY_TO_STRING(ARRAY_AGG(DISTINCT port), \',\')') + 'host_ports' => new Sql\Expression("ARRAY_TO_STRING(ARRAY_AGG(DISTINCT port), ',')") ]); } else { $targets->withColumns([ - 'host_ports' => new Sql\Expression('GROUP_CONCAT(DISTINCT port SEPARATOR \',\')') + 'host_ports' => new Sql\Expression("GROUP_CONCAT(DISTINCT port SEPARATOR ',')") ]); } @@ -60,12 +58,12 @@ public function fetchData() $target->host_name_or_ip = $target->host_ip; } - unset($target->ip); // Isn't needed any more!! - unset($target->chain); // We don't need any relation properties anymore + // Target ip is now obsolete and must not be included in the results. + // The relation is only used to utilize the query and must not be in the result set as well. + unset($target->ip); + unset($target->chain); - $properties = iterator_to_array($target); - - $results[$target->host_name_or_ip] = (object) $properties; + $results[$target->host_name_or_ip] = (object) iterator_to_array($target); } return $results; diff --git a/library/X509/ProvidedHook/ServicesImportSource.php b/library/X509/ProvidedHook/ServicesImportSource.php index 69102335..2b341202 100644 --- a/library/X509/ProvidedHook/ServicesImportSource.php +++ b/library/X509/ProvidedHook/ServicesImportSource.php @@ -50,11 +50,11 @@ public function fetchData() if ($this->getDb()->getAdapter() instanceof Sql\Adapter\Pgsql) { $targets ->withColumns([ - 'cert_fingerprint' => new Sql\Expression('ENCODE(%s, \'hex\')', [ + 'cert_fingerprint' => new Sql\Expression("ENCODE(%s, 'hex')", [ 'chain.certificate.fingerprint' ]), 'cert_dn' => new Sql\Expression( - 'ARRAY_TO_STRING(ARRAY_AGG(CONCAT(%s, \'=\', %s)), \',\')', + "ARRAY_TO_STRING(ARRAY_AGG(CONCAT(%s, '=', %s)), ',')", [ 'chain.certificate.dn.key', 'chain.certificate.dn.value' @@ -65,13 +65,13 @@ public function fetchData() ->groupBy(['target_chain_certificate.id', 'target_chain_certificate_issuer_certificate.id']); $certAltName->columns([ - new Sql\Expression('ARRAY_TO_STRING(ARRAY_AGG(CONCAT(%s, \':\', %s)), \',\')', ['type', 'value']) + new Sql\Expression("ARRAY_TO_STRING(ARRAY_AGG(CONCAT(%s, ':', %s)), ',')", ['type', 'value']) ]); } else { $targets->withColumns([ 'cert_fingerprint' => new Sql\Expression('HEX(%s)', ['chain.certificate.fingerprint']), 'cert_dn' => new Sql\Expression( - 'GROUP_CONCAT(CONCAT(%s, \'=\', %s) SEPARATOR \',\')', + "GROUP_CONCAT(CONCAT(%s, '=', %s) SEPARATOR ',')", [ 'chain.certificate.dn.key', 'chain.certificate.dn.value' @@ -80,7 +80,7 @@ public function fetchData() ]); $certAltName->columns([ - new Sql\Expression('GROUP_CONCAT(CONCAT(%s, \':\', %s) SEPARATOR \',\')', ['type', 'value']) + new Sql\Expression("GROUP_CONCAT(CONCAT(%s, ':', %s) SEPARATOR ',')", ['type', 'value']) ]); } @@ -101,12 +101,12 @@ public function fetchData() $target->host_port ); - unset($target->ip); // Isn't needed any more!! - unset($target->chain); // We don't need any relation properties anymore + // Target ip is now obsolete and must not be included in the results. + // The relation is only used to utilize the query and must not be in the result set as well. + unset($target->ip); + unset($target->chain); - $properties = iterator_to_array($target); - - $results[$target->host_name_ip_and_port] = (object) $properties; + $results[$target->host_name_ip_and_port] = (object) iterator_to_array($target); } return $results;