From 7f502da8809e4f1b88a64f742268895c343b0246 Mon Sep 17 00:00:00 2001 From: Vitor Mattos <1079143+vitormattos@users.noreply.github.com> Date: Sat, 25 Apr 2026 11:38:02 -0300 Subject: [PATCH 01/11] fix: allow legacy certificates without CRL metadata Signed-off-by: Vitor Mattos <1079143+vitormattos@users.noreply.github.com> --- lib/Service/IdentifyMethod/SignatureMethod/Password.php | 8 ++++++++ tests/php/Unit/Service/IdentifyMethod/PasswordTest.php | 6 ++---- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/lib/Service/IdentifyMethod/SignatureMethod/Password.php b/lib/Service/IdentifyMethod/SignatureMethod/Password.php index eb9a7aeede..2debf615cb 100644 --- a/lib/Service/IdentifyMethod/SignatureMethod/Password.php +++ b/lib/Service/IdentifyMethod/SignatureMethod/Password.php @@ -59,6 +59,14 @@ private function validateCertificateRevocation(array $certificateData): void { if ($status === CrlValidationStatus::DISABLED) { return; } + // Backward compatibility for legacy certificates issued before CRL metadata existed. + if ($status === CrlValidationStatus::MISSING) { + $this->identifyService->getLogger()->warning('Signing allowed for certificate without revocation metadata', [ + 'status' => $status->value, + 'signer_uid' => $this->userSession->getUser()?->getUID(), + ]); + return; + } $this->logRevocationBlockedSigning($status); throw new LibresignException($this->getRevocationErrorMessage($status), 422); } diff --git a/tests/php/Unit/Service/IdentifyMethod/PasswordTest.php b/tests/php/Unit/Service/IdentifyMethod/PasswordTest.php index f335561a32..5f1d864c23 100644 --- a/tests/php/Unit/Service/IdentifyMethod/PasswordTest.php +++ b/tests/php/Unit/Service/IdentifyMethod/PasswordTest.php @@ -290,8 +290,7 @@ public static function providerValidateToSignWithCertificateData(): array { 'validTo_time_t' => $futureTimestamp, 'crl_validation' => CrlValidationStatus::MISSING, ], - 'shouldThrow' => true, - 'expectedCode' => 422, + 'shouldThrow' => false, ], 'revoked and expired certificate' => [ 'certificateData' => [ @@ -306,8 +305,7 @@ public static function providerValidateToSignWithCertificateData(): array { 'validTo_time_t' => $futureTimestamp, 'crl_validation' => CrlValidationStatus::MISSING, ], - 'shouldThrow' => true, - 'expectedCode' => 422, + 'shouldThrow' => false, ], 'valid certificate - old date but valid (1970s timestamp)' => [ 'certificateData' => [ From 534ca79a5254424a6c02576efe5144ff071f63a1 Mon Sep 17 00:00:00 2001 From: Vitor Mattos <1079143+vitormattos@users.noreply.github.com> Date: Sat, 25 Apr 2026 13:01:05 -0300 Subject: [PATCH 02/11] fix(crl): detect distribution points across extension key variants Signed-off-by: Vitor Mattos <1079143+vitormattos@users.noreply.github.com> --- .../CertificateEngine/AEngineHandler.php | 73 ++++++++++++++++--- .../SignatureMethod/Password.php | 8 -- .../CertificateEngine/OpenSslHandlerTest.php | 28 +++++++ .../Service/IdentifyMethod/PasswordTest.php | 6 +- 4 files changed, 95 insertions(+), 20 deletions(-) diff --git a/lib/Handler/CertificateEngine/AEngineHandler.php b/lib/Handler/CertificateEngine/AEngineHandler.php index 6faf1bdd10..d0dfd9daea 100644 --- a/lib/Handler/CertificateEngine/AEngineHandler.php +++ b/lib/Handler/CertificateEngine/AEngineHandler.php @@ -182,23 +182,76 @@ private function parseX509(string $x509): array { } private function addCrlValidationInfo(array &$certData, string $certPem): void { - if (isset($certData['extensions']['crlDistributionPoints'])) { - preg_match_all('/URI:([^\s,\n]+)/', $certData['extensions']['crlDistributionPoints'], $matches); - $extractedUrls = $matches[1] ?? []; - - $certData['crl_urls'] = $extractedUrls; - $crlDetails = $this->crlRevocationChecker->validate($extractedUrls, $certPem); - $certData['crl_validation'] = $crlDetails['status']; - if (!empty($crlDetails['revoked_at'])) { - $certData['crl_revoked_at'] = $crlDetails['revoked_at']; + $extensions = $certData['extensions'] ?? []; + if (is_array($extensions)) { + ['hasExtension' => $hasCrlExtension, 'urls' => $extractedUrls] = $this->extractCrlUrlsFromExtensions($extensions); + if ($hasCrlExtension) { + $certData['crl_urls'] = $extractedUrls; + if (empty($extractedUrls)) { + $certData['crl_validation'] = CrlValidationStatus::NO_URLS; + return; + } + + $crlDetails = $this->crlRevocationChecker->validate($extractedUrls, $certPem); + $certData['crl_validation'] = $crlDetails['status']; + if (!empty($crlDetails['revoked_at'])) { + $certData['crl_revoked_at'] = $crlDetails['revoked_at']; + } + return; } - } else { + } + $externalValidationEnabled = $this->appConfig->getValueBool(Application::APP_ID, 'crl_external_validation_enabled', true); $certData['crl_validation'] = $externalValidationEnabled ? CrlValidationStatus::MISSING : CrlValidationStatus::DISABLED; $certData['crl_urls'] = []; + } + + /** + * @return array{hasExtension: bool, urls: array} + */ + private function extractCrlUrlsFromExtensions(array $extensions): array { + $values = []; + foreach ($extensions as $extensionName => $extensionValue) { + if (!is_string($extensionName)) { + continue; + } + + $normalizedName = strtolower(trim($extensionName)); + $isCrlDistributionPoints = + $normalizedName === 'crldistributionpoints' + || $normalizedName === 'x509v3 crl distribution points' + || $normalizedName === '2.5.29.31' + || str_contains($normalizedName, 'crl distribution points'); + + if (!$isCrlDistributionPoints) { + continue; + } + + if (is_string($extensionValue)) { + $values[] = $extensionValue; + } elseif (is_array($extensionValue)) { + $values[] = implode("\n", array_filter($extensionValue, 'is_string')); + } + } + + if (empty($values)) { + return ['hasExtension' => false, 'urls' => []]; } + + $urls = []; + foreach ($values as $value) { + preg_match_all('/URI\s*:\s*([^\s,\n]+)/i', $value, $matches); + if (!empty($matches[1])) { + $urls = [...$urls, ...$matches[1]]; + } + } + + return [ + 'hasExtension' => true, + 'urls' => array_values(array_unique($urls)), + ]; } private static function convertArrayToUtf8($array) { diff --git a/lib/Service/IdentifyMethod/SignatureMethod/Password.php b/lib/Service/IdentifyMethod/SignatureMethod/Password.php index 2debf615cb..eb9a7aeede 100644 --- a/lib/Service/IdentifyMethod/SignatureMethod/Password.php +++ b/lib/Service/IdentifyMethod/SignatureMethod/Password.php @@ -59,14 +59,6 @@ private function validateCertificateRevocation(array $certificateData): void { if ($status === CrlValidationStatus::DISABLED) { return; } - // Backward compatibility for legacy certificates issued before CRL metadata existed. - if ($status === CrlValidationStatus::MISSING) { - $this->identifyService->getLogger()->warning('Signing allowed for certificate without revocation metadata', [ - 'status' => $status->value, - 'signer_uid' => $this->userSession->getUser()?->getUID(), - ]); - return; - } $this->logRevocationBlockedSigning($status); throw new LibresignException($this->getRevocationErrorMessage($status), 422); } diff --git a/tests/php/Unit/Handler/CertificateEngine/OpenSslHandlerTest.php b/tests/php/Unit/Handler/CertificateEngine/OpenSslHandlerTest.php index f9f1e06d56..4447ed8136 100644 --- a/tests/php/Unit/Handler/CertificateEngine/OpenSslHandlerTest.php +++ b/tests/php/Unit/Handler/CertificateEngine/OpenSslHandlerTest.php @@ -468,6 +468,34 @@ public function testUniqueSerialNumbers(): void { $this->assertCount($numCertificates, array_unique($serialNumbers), 'All serial numbers should be unique'); } + public function testExtractCrlUrlsFromOidExtensionName(): void { + $handler = $this->getInstance(); + + $method = new \ReflectionMethod('OCA\\Libresign\\Handler\\CertificateEngine\\AEngineHandler', 'extractCrlUrlsFromExtensions'); + $method->setAccessible(true); + + $result = $method->invoke($handler, [ + '2.5.29.31' => "Full Name:\nURI:https://example.org/crl/root.crl", + ]); + + $this->assertTrue($result['hasExtension']); + $this->assertSame(['https://example.org/crl/root.crl'], $result['urls']); + } + + public function testExtractCrlUrlsFromX509LabelExtensionName(): void { + $handler = $this->getInstance(); + + $method = new \ReflectionMethod('OCA\\Libresign\\Handler\\CertificateEngine\\AEngineHandler', 'extractCrlUrlsFromExtensions'); + $method->setAccessible(true); + + $result = $method->invoke($handler, [ + 'X509v3 CRL Distribution Points' => "Full Name:\n URI : https://example.org/crl/issuer.crl", + ]); + + $this->assertTrue($result['hasExtension']); + $this->assertSame(['https://example.org/crl/issuer.crl'], $result['urls']); + } + public function testRealCertificateRevocationInCrl(): void { $this->caIdentifierService->generateCaId('openssl'); diff --git a/tests/php/Unit/Service/IdentifyMethod/PasswordTest.php b/tests/php/Unit/Service/IdentifyMethod/PasswordTest.php index 5f1d864c23..f335561a32 100644 --- a/tests/php/Unit/Service/IdentifyMethod/PasswordTest.php +++ b/tests/php/Unit/Service/IdentifyMethod/PasswordTest.php @@ -290,7 +290,8 @@ public static function providerValidateToSignWithCertificateData(): array { 'validTo_time_t' => $futureTimestamp, 'crl_validation' => CrlValidationStatus::MISSING, ], - 'shouldThrow' => false, + 'shouldThrow' => true, + 'expectedCode' => 422, ], 'revoked and expired certificate' => [ 'certificateData' => [ @@ -305,7 +306,8 @@ public static function providerValidateToSignWithCertificateData(): array { 'validTo_time_t' => $futureTimestamp, 'crl_validation' => CrlValidationStatus::MISSING, ], - 'shouldThrow' => false, + 'shouldThrow' => true, + 'expectedCode' => 422, ], 'valid certificate - old date but valid (1970s timestamp)' => [ 'certificateData' => [ From 3db21dd9a49361383393164d487cbc0a070e1cc0 Mon Sep 17 00:00:00 2001 From: Vitor Mattos <1079143+vitormattos@users.noreply.github.com> Date: Sat, 25 Apr 2026 13:34:35 -0300 Subject: [PATCH 03/11] fix: remove speculative CRL extension fallback matching Signed-off-by: Vitor Mattos <1079143+vitormattos@users.noreply.github.com> --- lib/Handler/CertificateEngine/AEngineHandler.php | 11 ++++++----- .../CertificateEngine/OpenSslHandlerTest.php | 14 ++++++++++++++ 2 files changed, 20 insertions(+), 5 deletions(-) diff --git a/lib/Handler/CertificateEngine/AEngineHandler.php b/lib/Handler/CertificateEngine/AEngineHandler.php index d0dfd9daea..3a27b4c030 100644 --- a/lib/Handler/CertificateEngine/AEngineHandler.php +++ b/lib/Handler/CertificateEngine/AEngineHandler.php @@ -213,17 +213,18 @@ private function addCrlValidationInfo(array &$certData, string $certPem): void { */ private function extractCrlUrlsFromExtensions(array $extensions): array { $values = []; + $acceptedCrlExtensionNames = [ + 'crldistributionpoints', + 'x509v3 crl distribution points', + '2.5.29.31', + ]; foreach ($extensions as $extensionName => $extensionValue) { if (!is_string($extensionName)) { continue; } $normalizedName = strtolower(trim($extensionName)); - $isCrlDistributionPoints = - $normalizedName === 'crldistributionpoints' - || $normalizedName === 'x509v3 crl distribution points' - || $normalizedName === '2.5.29.31' - || str_contains($normalizedName, 'crl distribution points'); + $isCrlDistributionPoints = in_array($normalizedName, $acceptedCrlExtensionNames, true); if (!$isCrlDistributionPoints) { continue; diff --git a/tests/php/Unit/Handler/CertificateEngine/OpenSslHandlerTest.php b/tests/php/Unit/Handler/CertificateEngine/OpenSslHandlerTest.php index 4447ed8136..ef38c3bb0a 100644 --- a/tests/php/Unit/Handler/CertificateEngine/OpenSslHandlerTest.php +++ b/tests/php/Unit/Handler/CertificateEngine/OpenSslHandlerTest.php @@ -496,6 +496,20 @@ public function testExtractCrlUrlsFromX509LabelExtensionName(): void { $this->assertSame(['https://example.org/crl/issuer.crl'], $result['urls']); } + public function testExtractCrlUrlsIgnoreUnknownExtensionNameWithSimilarText(): void { + $handler = $this->getInstance(); + + $method = new \ReflectionMethod('OCA\\Libresign\\Handler\\CertificateEngine\\AEngineHandler', 'extractCrlUrlsFromExtensions'); + $method->setAccessible(true); + + $result = $method->invoke($handler, [ + 'Issuer CRL Distribution Points' => "Full Name:\nURI:https://example.org/crl/issuer.crl", + ]); + + $this->assertFalse($result['hasExtension']); + $this->assertSame([], $result['urls']); + } + public function testRealCertificateRevocationInCrl(): void { $this->caIdentifierService->generateCaId('openssl'); From 5a3281ab5c9c95dd2b394773a0b82c3dbda05c26 Mon Sep 17 00:00:00 2001 From: Vitor Mattos <1079143+vitormattos@users.noreply.github.com> Date: Sat, 25 Apr 2026 13:43:59 -0300 Subject: [PATCH 04/11] refactor: extract CRL extension parser and remove reflection tests Signed-off-by: Vitor Mattos <1079143+vitormattos@users.noreply.github.com> --- .../CertificateEngine/AEngineHandler.php | 63 +++---------------- .../Crl/CrlDistributionPointsExtractor.php | 63 +++++++++++++++++++ .../CertificateEngine/OpenSslHandlerTest.php | 42 ------------- .../CrlDistributionPointsExtractorTest.php | 48 ++++++++++++++ 4 files changed, 121 insertions(+), 95 deletions(-) create mode 100644 lib/Service/Crl/CrlDistributionPointsExtractor.php create mode 100644 tests/php/Unit/Service/Crl/CrlDistributionPointsExtractorTest.php diff --git a/lib/Handler/CertificateEngine/AEngineHandler.php b/lib/Handler/CertificateEngine/AEngineHandler.php index 3a27b4c030..3b360bd9b7 100644 --- a/lib/Handler/CertificateEngine/AEngineHandler.php +++ b/lib/Handler/CertificateEngine/AEngineHandler.php @@ -17,6 +17,7 @@ use OCA\Libresign\Helper\MagicGetterSetterTrait; use OCA\Libresign\Service\CaIdentifierService; use OCA\Libresign\Service\CertificatePolicyService; +use OCA\Libresign\Service\Crl\CrlDistributionPointsExtractor; use OCA\Libresign\Service\Crl\CrlRevocationChecker; use OCP\Files\AppData\IAppDataFactory; use OCP\Files\IAppData; @@ -73,6 +74,7 @@ abstract class AEngineHandler implements IEngineHandler { protected string $certificate = ''; protected string $currentCaId = ''; protected IAppData $appData; + private CrlDistributionPointsExtractor $crlDistributionPointsExtractor; public function __construct( protected IConfig $config, @@ -85,8 +87,10 @@ public function __construct( protected CaIdentifierService $caIdentifierService, protected LoggerInterface $logger, private CrlRevocationChecker $crlRevocationChecker, + ?CrlDistributionPointsExtractor $crlDistributionPointsExtractor = null, ) { $this->appData = $appDataFactory->get('libresign'); + $this->crlDistributionPointsExtractor = $crlDistributionPointsExtractor ?? new CrlDistributionPointsExtractor(); } protected function exportToPkcs12( @@ -184,7 +188,7 @@ private function parseX509(string $x509): array { private function addCrlValidationInfo(array &$certData, string $certPem): void { $extensions = $certData['extensions'] ?? []; if (is_array($extensions)) { - ['hasExtension' => $hasCrlExtension, 'urls' => $extractedUrls] = $this->extractCrlUrlsFromExtensions($extensions); + ['hasExtension' => $hasCrlExtension, 'urls' => $extractedUrls] = $this->crlDistributionPointsExtractor->extractFromExtensions($extensions); if ($hasCrlExtension) { $certData['crl_urls'] = $extractedUrls; if (empty($extractedUrls)) { @@ -201,58 +205,11 @@ private function addCrlValidationInfo(array &$certData, string $certPem): void { } } - $externalValidationEnabled = $this->appConfig->getValueBool(Application::APP_ID, 'crl_external_validation_enabled', true); - $certData['crl_validation'] = $externalValidationEnabled - ? CrlValidationStatus::MISSING - : CrlValidationStatus::DISABLED; - $certData['crl_urls'] = []; - } - - /** - * @return array{hasExtension: bool, urls: array} - */ - private function extractCrlUrlsFromExtensions(array $extensions): array { - $values = []; - $acceptedCrlExtensionNames = [ - 'crldistributionpoints', - 'x509v3 crl distribution points', - '2.5.29.31', - ]; - foreach ($extensions as $extensionName => $extensionValue) { - if (!is_string($extensionName)) { - continue; - } - - $normalizedName = strtolower(trim($extensionName)); - $isCrlDistributionPoints = in_array($normalizedName, $acceptedCrlExtensionNames, true); - - if (!$isCrlDistributionPoints) { - continue; - } - - if (is_string($extensionValue)) { - $values[] = $extensionValue; - } elseif (is_array($extensionValue)) { - $values[] = implode("\n", array_filter($extensionValue, 'is_string')); - } - } - - if (empty($values)) { - return ['hasExtension' => false, 'urls' => []]; - } - - $urls = []; - foreach ($values as $value) { - preg_match_all('/URI\s*:\s*([^\s,\n]+)/i', $value, $matches); - if (!empty($matches[1])) { - $urls = [...$urls, ...$matches[1]]; - } - } - - return [ - 'hasExtension' => true, - 'urls' => array_values(array_unique($urls)), - ]; + $externalValidationEnabled = $this->appConfig->getValueBool(Application::APP_ID, 'crl_external_validation_enabled', true); + $certData['crl_validation'] = $externalValidationEnabled + ? CrlValidationStatus::MISSING + : CrlValidationStatus::DISABLED; + $certData['crl_urls'] = []; } private static function convertArrayToUtf8($array) { diff --git a/lib/Service/Crl/CrlDistributionPointsExtractor.php b/lib/Service/Crl/CrlDistributionPointsExtractor.php new file mode 100644 index 0000000000..8da9a5516d --- /dev/null +++ b/lib/Service/Crl/CrlDistributionPointsExtractor.php @@ -0,0 +1,63 @@ + */ + private const ACCEPTED_EXTENSION_NAMES = [ + 'crldistributionpoints', + 'x509v3 crl distribution points', + '2.5.29.31', + ]; + + /** + * @param array $extensions + * @return array{hasExtension: bool, urls: list} + */ + public function extractFromExtensions(array $extensions): array { + $values = []; + foreach ($extensions as $extensionName => $extensionValue) { + if (!is_string($extensionName)) { + continue; + } + + $normalizedName = strtolower(trim($extensionName)); + if (!in_array($normalizedName, self::ACCEPTED_EXTENSION_NAMES, true)) { + continue; + } + + if (is_string($extensionValue)) { + $values[] = $extensionValue; + } elseif (is_array($extensionValue)) { + $values[] = implode("\n", array_filter($extensionValue, 'is_string')); + } + } + + if (empty($values)) { + return ['hasExtension' => false, 'urls' => []]; + } + + $urls = []; + foreach ($values as $value) { + preg_match_all('/URI\s*:\s*([^\s,\n]+)/i', $value, $matches); + if (!empty($matches[1])) { + $urls = [...$urls, ...$matches[1]]; + } + } + + /** @var list $uniqueUrls */ + $uniqueUrls = array_values(array_unique($urls)); + + return [ + 'hasExtension' => true, + 'urls' => $uniqueUrls, + ]; + } +} \ No newline at end of file diff --git a/tests/php/Unit/Handler/CertificateEngine/OpenSslHandlerTest.php b/tests/php/Unit/Handler/CertificateEngine/OpenSslHandlerTest.php index ef38c3bb0a..f9f1e06d56 100644 --- a/tests/php/Unit/Handler/CertificateEngine/OpenSslHandlerTest.php +++ b/tests/php/Unit/Handler/CertificateEngine/OpenSslHandlerTest.php @@ -468,48 +468,6 @@ public function testUniqueSerialNumbers(): void { $this->assertCount($numCertificates, array_unique($serialNumbers), 'All serial numbers should be unique'); } - public function testExtractCrlUrlsFromOidExtensionName(): void { - $handler = $this->getInstance(); - - $method = new \ReflectionMethod('OCA\\Libresign\\Handler\\CertificateEngine\\AEngineHandler', 'extractCrlUrlsFromExtensions'); - $method->setAccessible(true); - - $result = $method->invoke($handler, [ - '2.5.29.31' => "Full Name:\nURI:https://example.org/crl/root.crl", - ]); - - $this->assertTrue($result['hasExtension']); - $this->assertSame(['https://example.org/crl/root.crl'], $result['urls']); - } - - public function testExtractCrlUrlsFromX509LabelExtensionName(): void { - $handler = $this->getInstance(); - - $method = new \ReflectionMethod('OCA\\Libresign\\Handler\\CertificateEngine\\AEngineHandler', 'extractCrlUrlsFromExtensions'); - $method->setAccessible(true); - - $result = $method->invoke($handler, [ - 'X509v3 CRL Distribution Points' => "Full Name:\n URI : https://example.org/crl/issuer.crl", - ]); - - $this->assertTrue($result['hasExtension']); - $this->assertSame(['https://example.org/crl/issuer.crl'], $result['urls']); - } - - public function testExtractCrlUrlsIgnoreUnknownExtensionNameWithSimilarText(): void { - $handler = $this->getInstance(); - - $method = new \ReflectionMethod('OCA\\Libresign\\Handler\\CertificateEngine\\AEngineHandler', 'extractCrlUrlsFromExtensions'); - $method->setAccessible(true); - - $result = $method->invoke($handler, [ - 'Issuer CRL Distribution Points' => "Full Name:\nURI:https://example.org/crl/issuer.crl", - ]); - - $this->assertFalse($result['hasExtension']); - $this->assertSame([], $result['urls']); - } - public function testRealCertificateRevocationInCrl(): void { $this->caIdentifierService->generateCaId('openssl'); diff --git a/tests/php/Unit/Service/Crl/CrlDistributionPointsExtractorTest.php b/tests/php/Unit/Service/Crl/CrlDistributionPointsExtractorTest.php new file mode 100644 index 0000000000..17a5423416 --- /dev/null +++ b/tests/php/Unit/Service/Crl/CrlDistributionPointsExtractorTest.php @@ -0,0 +1,48 @@ +extractor = new CrlDistributionPointsExtractor(); + } + + public function testExtractFromOidExtensionName(): void { + $result = $this->extractor->extractFromExtensions([ + '2.5.29.31' => "Full Name:\nURI:https://example.org/crl/root.crl", + ]); + + $this->assertTrue($result['hasExtension']); + $this->assertSame(['https://example.org/crl/root.crl'], $result['urls']); + } + + public function testExtractFromX509LabelExtensionName(): void { + $result = $this->extractor->extractFromExtensions([ + 'X509v3 CRL Distribution Points' => "Full Name:\n URI : https://example.org/crl/issuer.crl", + ]); + + $this->assertTrue($result['hasExtension']); + $this->assertSame(['https://example.org/crl/issuer.crl'], $result['urls']); + } + + public function testIgnoreUnknownExtensionNameWithSimilarText(): void { + $result = $this->extractor->extractFromExtensions([ + 'Issuer CRL Distribution Points' => "Full Name:\nURI:https://example.org/crl/issuer.crl", + ]); + + $this->assertFalse($result['hasExtension']); + $this->assertSame([], $result['urls']); + } +} From 7e887a615f77723154129901e84c6225a7a5abb9 Mon Sep 17 00:00:00 2001 From: Vitor Mattos <1079143+vitormattos@users.noreply.github.com> Date: Sat, 25 Apr 2026 13:47:05 -0300 Subject: [PATCH 05/11] test: expand CRL extractor coverage with RFC-driven data provider Signed-off-by: Vitor Mattos <1079143+vitormattos@users.noreply.github.com> --- .../Crl/CrlDistributionPointsExtractor.php | 2 +- .../CrlDistributionPointsExtractorTest.php | 94 ++++++++++++++----- 2 files changed, 73 insertions(+), 23 deletions(-) diff --git a/lib/Service/Crl/CrlDistributionPointsExtractor.php b/lib/Service/Crl/CrlDistributionPointsExtractor.php index 8da9a5516d..6ed2ae8b0a 100644 --- a/lib/Service/Crl/CrlDistributionPointsExtractor.php +++ b/lib/Service/Crl/CrlDistributionPointsExtractor.php @@ -46,7 +46,7 @@ public function extractFromExtensions(array $extensions): array { $urls = []; foreach ($values as $value) { - preg_match_all('/URI\s*:\s*([^\s,\n]+)/i', $value, $matches); + preg_match_all('/URI\s*:\s*([^\s\n]+)/i', $value, $matches); if (!empty($matches[1])) { $urls = [...$urls, ...$matches[1]]; } diff --git a/tests/php/Unit/Service/Crl/CrlDistributionPointsExtractorTest.php b/tests/php/Unit/Service/Crl/CrlDistributionPointsExtractorTest.php index 17a5423416..7dacd54bdb 100644 --- a/tests/php/Unit/Service/Crl/CrlDistributionPointsExtractorTest.php +++ b/tests/php/Unit/Service/Crl/CrlDistributionPointsExtractorTest.php @@ -10,6 +10,7 @@ namespace OCA\Libresign\Tests\Unit\Service\Crl; use OCA\Libresign\Service\Crl\CrlDistributionPointsExtractor; +use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\TestCase; final class CrlDistributionPointsExtractorTest extends TestCase { @@ -19,30 +20,79 @@ protected function setUp(): void { $this->extractor = new CrlDistributionPointsExtractor(); } - public function testExtractFromOidExtensionName(): void { - $result = $this->extractor->extractFromExtensions([ - '2.5.29.31' => "Full Name:\nURI:https://example.org/crl/root.crl", - ]); + #[DataProvider('crlDistributionPointExtractionProvider')] + public function testExtractFromExtensions(array $extensions, bool $expectedHasExtension, array $expectedUrls): void { + $result = $this->extractor->extractFromExtensions($extensions); - $this->assertTrue($result['hasExtension']); - $this->assertSame(['https://example.org/crl/root.crl'], $result['urls']); + $this->assertSame($expectedHasExtension, $result['hasExtension']); + $this->assertSame($expectedUrls, $result['urls']); } - public function testExtractFromX509LabelExtensionName(): void { - $result = $this->extractor->extractFromExtensions([ - 'X509v3 CRL Distribution Points' => "Full Name:\n URI : https://example.org/crl/issuer.crl", - ]); - - $this->assertTrue($result['hasExtension']); - $this->assertSame(['https://example.org/crl/issuer.crl'], $result['urls']); - } - - public function testIgnoreUnknownExtensionNameWithSimilarText(): void { - $result = $this->extractor->extractFromExtensions([ - 'Issuer CRL Distribution Points' => "Full Name:\nURI:https://example.org/crl/issuer.crl", - ]); - - $this->assertFalse($result['hasExtension']); - $this->assertSame([], $result['urls']); + /** + * RFC 5280 4.2.1.13 defines cRLDistributionPoints as DistributionPointName + * with URI represented in GeneralNames. Tests cover common OpenSSL textual + * outputs for HTTP and LDAP URIs and multiple distribution points. + * + * @return array, 1: bool, 2: list}> + */ + public static function crlDistributionPointExtractionProvider(): array { + return [ + 'oid-extension-with-http-uri' => [ + [ + '2.5.29.31' => "Full Name:\nURI:https://example.org/crl/root.crl", + ], + true, + ['https://example.org/crl/root.crl'], + ], + 'x509v3-label-with-http-uri' => [ + [ + 'X509v3 CRL Distribution Points' => "Full Name:\n URI : https://example.org/crl/issuer.crl", + ], + true, + ['https://example.org/crl/issuer.crl'], + ], + 'rfc-ldap-uri-with-dn-and-query' => [ + [ + 'crlDistributionPoints' => "Full Name:\nURI:ldap://ldap.example.com/cn=Example%20CA,ou=PKI,dc=example,dc=com?certificateRevocationList;binary", + ], + true, + ['ldap://ldap.example.com/cn=Example%20CA,ou=PKI,dc=example,dc=com?certificateRevocationList;binary'], + ], + 'multiple-distribution-points-in-single-extension' => [ + [ + '2.5.29.31' => "Full Name:\nURI:https://pki.example.org/root.crl\nFull Name:\nURI:ldap://ldap.example.org/cn=RootCA,dc=example,dc=org?certificateRevocationList;binary", + ], + true, + [ + 'https://pki.example.org/root.crl', + 'ldap://ldap.example.org/cn=RootCA,dc=example,dc=org?certificateRevocationList;binary', + ], + ], + 'array-extension-value-and-duplicates' => [ + [ + '2.5.29.31' => [ + 'Full Name:', + 'URI:https://example.org/crl/root.crl', + 'URI:https://example.org/crl/root.crl', + ], + ], + true, + ['https://example.org/crl/root.crl'], + ], + 'known-extension-without-uri' => [ + [ + '2.5.29.31' => 'Distribution Point Name: relativeName=CN=DP1', + ], + true, + [], + ], + 'unknown-extension-name-should-not-match' => [ + [ + 'Issuer CRL Distribution Points' => "Full Name:\nURI:https://example.org/crl/issuer.crl", + ], + false, + [], + ], + ]; } } From ec048875d9bc92ded5d249abf739ac9aad57e7b8 Mon Sep 17 00:00:00 2001 From: Vitor Mattos <1079143+vitormattos@users.noreply.github.com> Date: Sat, 25 Apr 2026 13:48:59 -0300 Subject: [PATCH 06/11] test: add RFC-focused CRL distribution point scenarios Signed-off-by: Vitor Mattos <1079143+vitormattos@users.noreply.github.com> --- .../CrlDistributionPointsExtractorTest.php | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/tests/php/Unit/Service/Crl/CrlDistributionPointsExtractorTest.php b/tests/php/Unit/Service/Crl/CrlDistributionPointsExtractorTest.php index 7dacd54bdb..d657ccca34 100644 --- a/tests/php/Unit/Service/Crl/CrlDistributionPointsExtractorTest.php +++ b/tests/php/Unit/Service/Crl/CrlDistributionPointsExtractorTest.php @@ -68,6 +68,38 @@ public static function crlDistributionPointExtractionProvider(): array { 'ldap://ldap.example.org/cn=RootCA,dc=example,dc=org?certificateRevocationList;binary', ], ], + 'rfc-structure-with-reasons-and-crl-issuer' => [ + [ + '2.5.29.31' => "Full Name:\n URI:http://crl.example.org/root.crl\nReasons: keyCompromise, cACompromise\nCRL Issuer:\n DirName:/C=BR/O=Example/CN=Example CRL Issuer", + ], + true, + ['http://crl.example.org/root.crl'], + ], + 'extension-name-is-trimmed-and-case-insensitive' => [ + [ + ' X509V3 CRL Distribution Points ' => "Full Name:\n URI:https://example.org/crl/mixed-case.crl", + ], + true, + ['https://example.org/crl/mixed-case.crl'], + ], + 'uri-token-is-case-insensitive' => [ + [ + '2.5.29.31' => "Full Name:\nuri:ldap://ldap.example.net/cn=CA,dc=example,dc=net?certificateRevocationList;binary", + ], + true, + ['ldap://ldap.example.net/cn=CA,dc=example,dc=net?certificateRevocationList;binary'], + ], + 'multiple-supported-extension-keys-are-merged-and-deduplicated' => [ + [ + '2.5.29.31' => "Full Name:\nURI:https://example.org/crl/shared.crl", + 'crlDistributionPoints' => "Full Name:\nURI:https://example.org/crl/shared.crl\nURI:https://example.org/crl/extra.crl", + ], + true, + [ + 'https://example.org/crl/shared.crl', + 'https://example.org/crl/extra.crl', + ], + ], 'array-extension-value-and-duplicates' => [ [ '2.5.29.31' => [ From fdb2c61241ceb910305a0c6c65a41a9cb64be0c1 Mon Sep 17 00:00:00 2001 From: Vitor Mattos <1079143+vitormattos@users.noreply.github.com> Date: Sat, 25 Apr 2026 13:50:19 -0300 Subject: [PATCH 07/11] test: extend CRL provider edge coverage Signed-off-by: Vitor Mattos <1079143+vitormattos@users.noreply.github.com> --- .../Crl/CrlDistributionPointsExtractor.php | 6 +++- .../CrlDistributionPointsExtractorTest.php | 32 +++++++++++++++++++ 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/lib/Service/Crl/CrlDistributionPointsExtractor.php b/lib/Service/Crl/CrlDistributionPointsExtractor.php index 6ed2ae8b0a..451fceeae5 100644 --- a/lib/Service/Crl/CrlDistributionPointsExtractor.php +++ b/lib/Service/Crl/CrlDistributionPointsExtractor.php @@ -48,7 +48,11 @@ public function extractFromExtensions(array $extensions): array { foreach ($values as $value) { preg_match_all('/URI\s*:\s*([^\s\n]+)/i', $value, $matches); if (!empty($matches[1])) { - $urls = [...$urls, ...$matches[1]]; + $normalizedUrls = array_map( + static fn (string $url): string => rtrim($url, ")]"), + $matches[1], + ); + $urls = [...$urls, ...$normalizedUrls]; } } diff --git a/tests/php/Unit/Service/Crl/CrlDistributionPointsExtractorTest.php b/tests/php/Unit/Service/Crl/CrlDistributionPointsExtractorTest.php index d657ccca34..53f392394e 100644 --- a/tests/php/Unit/Service/Crl/CrlDistributionPointsExtractorTest.php +++ b/tests/php/Unit/Service/Crl/CrlDistributionPointsExtractorTest.php @@ -89,6 +89,20 @@ public static function crlDistributionPointExtractionProvider(): array { true, ['ldap://ldap.example.net/cn=CA,dc=example,dc=net?certificateRevocationList;binary'], ], + 'uri-with-tabs-and-extra-whitespace' => [ + [ + '2.5.29.31' => "Full Name:\n\tURI\t:\t https://example.org/crl/with-tabs.crl", + ], + true, + ['https://example.org/crl/with-tabs.crl'], + ], + 'uri-line-with-closing-parenthesis-from-formatted-output' => [ + [ + '2.5.29.31' => "Distribution Point (1):\nURI:https://example.org/crl/formatted.crl)", + ], + true, + ['https://example.org/crl/formatted.crl'], + ], 'multiple-supported-extension-keys-are-merged-and-deduplicated' => [ [ '2.5.29.31' => "Full Name:\nURI:https://example.org/crl/shared.crl", @@ -118,6 +132,24 @@ public static function crlDistributionPointExtractionProvider(): array { true, [], ], + 'known-extension-with-general-names-but-no-uri' => [ + [ + 'X509v3 CRL Distribution Points' => "Full Name:\nDNS:crl.example.org\nDirName:/C=BR/O=Example/CN=CRL Directory", + ], + true, + [], + ], + 'multiple-supported-keys-preserve-first-seen-order' => [ + [ + 'crlDistributionPoints' => "Full Name:\nURI:https://example.org/crl/first.crl", + '2.5.29.31' => "Full Name:\nURI:https://example.org/crl/second.crl", + ], + true, + [ + 'https://example.org/crl/first.crl', + 'https://example.org/crl/second.crl', + ], + ], 'unknown-extension-name-should-not-match' => [ [ 'Issuer CRL Distribution Points' => "Full Name:\nURI:https://example.org/crl/issuer.crl", From 2c99c075b2be39e151612898f0bbef7d9cb3f22e Mon Sep 17 00:00:00 2001 From: Vitor Mattos <1079143+vitormattos@users.noreply.github.com> Date: Sat, 25 Apr 2026 13:53:32 -0300 Subject: [PATCH 08/11] refactor: optimize CRL distribution point extraction Signed-off-by: Vitor Mattos <1079143+vitormattos@users.noreply.github.com> --- .../Crl/CrlDistributionPointsExtractor.php | 59 ++++++++++++------- 1 file changed, 38 insertions(+), 21 deletions(-) diff --git a/lib/Service/Crl/CrlDistributionPointsExtractor.php b/lib/Service/Crl/CrlDistributionPointsExtractor.php index 451fceeae5..68e61765a9 100644 --- a/lib/Service/Crl/CrlDistributionPointsExtractor.php +++ b/lib/Service/Crl/CrlDistributionPointsExtractor.php @@ -10,52 +10,48 @@ namespace OCA\Libresign\Service\Crl; final class CrlDistributionPointsExtractor { - /** @var list */ + /** @var array */ private const ACCEPTED_EXTENSION_NAMES = [ - 'crldistributionpoints', - 'x509v3 crl distribution points', - '2.5.29.31', + 'crldistributionpoints' => true, + 'x509v3 crl distribution points' => true, + '2.5.29.31' => true, ]; + private const URI_PATTERN = '/URI\s*:\s*([^\s\n]+)/i'; + /** * @param array $extensions * @return array{hasExtension: bool, urls: list} */ public function extractFromExtensions(array $extensions): array { - $values = []; + $hasCrlExtension = false; + $urls = []; foreach ($extensions as $extensionName => $extensionValue) { if (!is_string($extensionName)) { continue; } $normalizedName = strtolower(trim($extensionName)); - if (!in_array($normalizedName, self::ACCEPTED_EXTENSION_NAMES, true)) { + if (!isset(self::ACCEPTED_EXTENSION_NAMES[$normalizedName])) { continue; } + $hasCrlExtension = true; if (is_string($extensionValue)) { - $values[] = $extensionValue; + $this->appendUrlsFromText($extensionValue, $urls); } elseif (is_array($extensionValue)) { - $values[] = implode("\n", array_filter($extensionValue, 'is_string')); + foreach ($extensionValue as $extensionPart) { + if (is_string($extensionPart)) { + $this->appendUrlsFromText($extensionPart, $urls); + } + } } } - if (empty($values)) { + if (!$hasCrlExtension) { return ['hasExtension' => false, 'urls' => []]; } - $urls = []; - foreach ($values as $value) { - preg_match_all('/URI\s*:\s*([^\s\n]+)/i', $value, $matches); - if (!empty($matches[1])) { - $normalizedUrls = array_map( - static fn (string $url): string => rtrim($url, ")]"), - $matches[1], - ); - $urls = [...$urls, ...$normalizedUrls]; - } - } - /** @var list $uniqueUrls */ $uniqueUrls = array_values(array_unique($urls)); @@ -64,4 +60,25 @@ public function extractFromExtensions(array $extensions): array { 'urls' => $uniqueUrls, ]; } + + /** + * @param list $urls + */ + private function appendUrlsFromText(string $value, array &$urls): void { + preg_match_all(self::URI_PATTERN, $value, $matches); + if (empty($matches[1])) { + return; + } + + foreach ($matches[1] as $url) { + $normalizedUrl = $this->normalizeUrlToken($url); + if ($normalizedUrl !== '') { + $urls[] = $normalizedUrl; + } + } + } + + private function normalizeUrlToken(string $url): string { + return rtrim($url, ")]"); + } } \ No newline at end of file From acdd212747a466647c507f8aaea2d52c0260076b Mon Sep 17 00:00:00 2001 From: Vitor Mattos <1079143+vitormattos@users.noreply.github.com> Date: Sat, 25 Apr 2026 13:54:40 -0300 Subject: [PATCH 09/11] perf: reduce CRL extractor regex and dedup overhead Signed-off-by: Vitor Mattos <1079143+vitormattos@users.noreply.github.com> --- .../Crl/CrlDistributionPointsExtractor.php | 28 +++++++++++-------- 1 file changed, 17 insertions(+), 11 deletions(-) diff --git a/lib/Service/Crl/CrlDistributionPointsExtractor.php b/lib/Service/Crl/CrlDistributionPointsExtractor.php index 68e61765a9..f5f04b2077 100644 --- a/lib/Service/Crl/CrlDistributionPointsExtractor.php +++ b/lib/Service/Crl/CrlDistributionPointsExtractor.php @@ -25,7 +25,8 @@ final class CrlDistributionPointsExtractor { */ public function extractFromExtensions(array $extensions): array { $hasCrlExtension = false; - $urls = []; + $orderedUrls = []; + $seenUrls = []; foreach ($extensions as $extensionName => $extensionValue) { if (!is_string($extensionName)) { continue; @@ -38,11 +39,11 @@ public function extractFromExtensions(array $extensions): array { $hasCrlExtension = true; if (is_string($extensionValue)) { - $this->appendUrlsFromText($extensionValue, $urls); + $this->appendUrlsFromText($extensionValue, $orderedUrls, $seenUrls); } elseif (is_array($extensionValue)) { foreach ($extensionValue as $extensionPart) { if (is_string($extensionPart)) { - $this->appendUrlsFromText($extensionPart, $urls); + $this->appendUrlsFromText($extensionPart, $orderedUrls, $seenUrls); } } } @@ -52,19 +53,21 @@ public function extractFromExtensions(array $extensions): array { return ['hasExtension' => false, 'urls' => []]; } - /** @var list $uniqueUrls */ - $uniqueUrls = array_values(array_unique($urls)); - return [ 'hasExtension' => true, - 'urls' => $uniqueUrls, + 'urls' => $orderedUrls, ]; } /** - * @param list $urls + * @param list $orderedUrls + * @param array $seenUrls */ - private function appendUrlsFromText(string $value, array &$urls): void { + private function appendUrlsFromText(string $value, array &$orderedUrls, array &$seenUrls): void { + if (stripos($value, 'URI') === false) { + return; + } + preg_match_all(self::URI_PATTERN, $value, $matches); if (empty($matches[1])) { return; @@ -72,9 +75,12 @@ private function appendUrlsFromText(string $value, array &$urls): void { foreach ($matches[1] as $url) { $normalizedUrl = $this->normalizeUrlToken($url); - if ($normalizedUrl !== '') { - $urls[] = $normalizedUrl; + if ($normalizedUrl === '' || isset($seenUrls[$normalizedUrl])) { + continue; } + + $seenUrls[$normalizedUrl] = true; + $orderedUrls[] = $normalizedUrl; } } From 7067037a71411a49f33be53aec25e626ab3649b0 Mon Sep 17 00:00:00 2001 From: Vitor Mattos <1079143+vitormattos@users.noreply.github.com> Date: Sat, 25 Apr 2026 14:02:13 -0300 Subject: [PATCH 10/11] fix: cs Signed-off-by: Vitor Mattos <1079143+vitormattos@users.noreply.github.com> --- lib/Service/Crl/CrlDistributionPointsExtractor.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/Service/Crl/CrlDistributionPointsExtractor.php b/lib/Service/Crl/CrlDistributionPointsExtractor.php index f5f04b2077..09e2a5d3c0 100644 --- a/lib/Service/Crl/CrlDistributionPointsExtractor.php +++ b/lib/Service/Crl/CrlDistributionPointsExtractor.php @@ -85,6 +85,6 @@ private function appendUrlsFromText(string $value, array &$orderedUrls, array &$ } private function normalizeUrlToken(string $url): string { - return rtrim($url, ")]"); + return rtrim($url, ')]'); } -} \ No newline at end of file +} From 88f183d536bc65f0d333f6e86feceda9cbc0d190 Mon Sep 17 00:00:00 2001 From: Vitor Mattos <1079143+vitormattos@users.noreply.github.com> Date: Sat, 25 Apr 2026 14:08:03 -0300 Subject: [PATCH 11/11] refactor: remove unused extractor constructor injection Signed-off-by: Vitor Mattos <1079143+vitormattos@users.noreply.github.com> --- lib/Handler/CertificateEngine/AEngineHandler.php | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/Handler/CertificateEngine/AEngineHandler.php b/lib/Handler/CertificateEngine/AEngineHandler.php index 3b360bd9b7..7aca24face 100644 --- a/lib/Handler/CertificateEngine/AEngineHandler.php +++ b/lib/Handler/CertificateEngine/AEngineHandler.php @@ -87,10 +87,9 @@ public function __construct( protected CaIdentifierService $caIdentifierService, protected LoggerInterface $logger, private CrlRevocationChecker $crlRevocationChecker, - ?CrlDistributionPointsExtractor $crlDistributionPointsExtractor = null, ) { $this->appData = $appDataFactory->get('libresign'); - $this->crlDistributionPointsExtractor = $crlDistributionPointsExtractor ?? new CrlDistributionPointsExtractor(); + $this->crlDistributionPointsExtractor = new CrlDistributionPointsExtractor(); } protected function exportToPkcs12(