[stable32] fix(crl): enforce CRL metadata with scoped legacy backfill

lib/Migration/Version17004Date20260421000000.php

@@ -0,0 +1,171 @@
+<?php
+
+declare(strict_types=1);
+
+/**
+ * SPDX-FileCopyrightText: 2026 LibreCode coop and contributors
+ * SPDX-License-Identifier: AGPL-3.0-or-later
+ */
+
+namespace OCA\Libresign\Migration;
+
+use Closure;
+use OCA\Libresign\AppInfo\Application;
+use OCP\DB\ISchemaWrapper;
+use OCP\DB\QueryBuilder\IQueryBuilder;
+use OCP\IAppConfig;
+use OCP\IConfig;
+use OCP\IDBConnection;
+use OCP\Migration\IOutput;
+use OCP\Migration\SimpleMigrationStep;
+
+class Version17004Date20260421000000 extends SimpleMigrationStep {
+	public function __construct(
+		private IConfig $config,
+		private IAppConfig $appConfig,
+		private IDBConnection $connection,
+	) {
+	}
+
+	/**
+	 * @param IOutput $output
+	 * @param Closure(): ISchemaWrapper $schemaClosure
+	 * @param array $options
+	 */
+	#[\Override]
+	public function preSchemaChange(IOutput $output, Closure $schemaClosure, array $options): void {
+		/** @var ISchemaWrapper $schema */
+		$schema = $schemaClosure();
+		if (!$schema->hasTable('libresign_crl')) {
+			return;
+		}
+
+		$metadata = $this->resolveMetadataDefaults();
+		if ($metadata === null) {
+			$output->warning('Skipped CRL metadata backfill because no deterministic metadata source was found.');
+			return;
+		}
+
+		$updatedInstance = $this->backfillInstanceId($metadata['instanceId']);
+		$updatedGeneration = $this->backfillGeneration($metadata['generation']);
+		$updatedEngine = $this->backfillEngine($metadata['engine']);
+
+		if ($updatedInstance > 0 || $updatedGeneration > 0 || $updatedEngine > 0) {
+			$output->warning(sprintf(
+				'Backfilled CRL metadata for legacy rows (instance_id=%d, generation=%d, engine=%d).',
+				$updatedInstance,
+				$updatedGeneration,
+				$updatedEngine,
+			));
+		}
+	}
+
+	/**
+	 * @param IOutput $output
+	 * @param Closure(): ISchemaWrapper $schemaClosure
+	 * @param array $options
+	 * @return null|ISchemaWrapper
+	 */
+	#[\Override]
+	public function changeSchema(IOutput $output, Closure $schemaClosure, array $options): ?ISchemaWrapper {
+		/** @var ISchemaWrapper $schema */
+		return $schemaClosure();
+	}
+
+	/**
+	 * @return array{instanceId: string, generation: int, engine: string}|null
+	 */
+	private function resolveMetadataDefaults(): ?array {
+		$engine = $this->appConfig->getValueString(Application::APP_ID, 'certificate_engine', 'openssl');
+		if ($engine === '' || $engine === 'none') {
+			$engine = 'openssl';
+		}
+
+		$instanceId = $this->appConfig->getValueString(Application::APP_ID, 'instance_id', '');
+		$generation = max(1, $this->appConfig->getValueInt(Application::APP_ID, 'ca_generation_counter', 1));
+		$caId = $this->appConfig->getValueString(Application::APP_ID, 'ca_id', '');
+
+		$pattern = '/^libresign-ca-id:(?P<instanceId>[a-z0-9]+)_g:(?P<generation>\d+)_e:(?P<engineType>[oc])$/';
+		if ($caId !== '' && preg_match($pattern, $caId, $matches)) {
+			$instanceId = $matches['instanceId'];
+			$generation = max(1, (int)$matches['generation']);
+			$engine = $matches['engineType'] === 'c' ? 'cfssl' : 'openssl';
+		}
+
+		if ($instanceId === '') {
+			$instanceId = $this->config->getSystemValueString('instanceid', '');
+		}
+
+		if ($instanceId === '' || !in_array($engine, ['openssl', 'cfssl'], true)) {
+			return null;
+		}
+
+		return [
+			'instanceId' => $instanceId,
+			'generation' => $generation,
+			'engine' => $engine,
+		];
+	}
+
+	private function backfillInstanceId(string $instanceId): int {
+		$qb = $this->connection->getQueryBuilder();
+
+		return $qb->update('libresign_crl')
+			->set('instance_id', $qb->createNamedParameter($instanceId))
+			->where($qb->expr()->eq('status', $qb->createNamedParameter('issued')))
+			->andWhere(
+				$qb->expr()->orX(
+					$qb->expr()->isNull('generation'),
+					$qb->expr()->isNull('engine'),
+					$qb->expr()->eq('engine', $qb->createNamedParameter('')),
+				)
+			)
+			->andWhere(
+				$qb->expr()->orX(
+					$qb->expr()->isNull('instance_id'),
+					$qb->expr()->eq('instance_id', $qb->createNamedParameter('')),
+				)
+			)
+			->executeStatement();
+	}
+
+	private function backfillGeneration(int $generation): int {
+		$qb = $this->connection->getQueryBuilder();
+
+		return $qb->update('libresign_crl')
+			->set('generation', $qb->createNamedParameter($generation, IQueryBuilder::PARAM_INT))
+			->where($qb->expr()->eq('status', $qb->createNamedParameter('issued')))
+			->andWhere(
+				$qb->expr()->orX(
+					$qb->expr()->isNull('instance_id'),
+					$qb->expr()->eq('instance_id', $qb->createNamedParameter('')),
+					$qb->expr()->isNull('engine'),
+					$qb->expr()->eq('engine', $qb->createNamedParameter('')),
+				)
+			)
+			->andWhere($qb->expr()->isNull('generation'))
+			->executeStatement();
+	}
+
+	private function backfillEngine(string $engine): int {
+		$qb = $this->connection->getQueryBuilder();
+
+		return $qb->update('libresign_crl')
+			->set('engine', $qb->createNamedParameter($engine))
+			->where($qb->expr()->eq('status', $qb->createNamedParameter('issued')))
+			->andWhere(
+				$qb->expr()->orX(
+					$qb->expr()->isNull('instance_id'),
+					$qb->expr()->eq('instance_id', $qb->createNamedParameter('')),
+					$qb->expr()->isNull('generation'),
+				)
+			)
+			->andWhere(
+				$qb->expr()->orX(
+					$qb->expr()->isNull('engine'),
+					$qb->expr()->eq('engine', $qb->createNamedParameter('')),
+				)
+			)
+			->executeStatement();
+	}
+}

lib/Service/Crl/CrlService.php

@@ -64,10 +64,7 @@ public function revokeCertificate(
 
 		try {
 			$certificate = $this->crlMapper->findBySerialNumber($serialNumber);
-			$instanceId = $certificate->getInstanceId();
-			$generation = $certificate->getGeneration();
-			$engineType = $certificate->getEngine();
-
+			['instanceId' => $instanceId, 'generation' => $generation, 'engineType' => $engineType] = $this->getCrlMetadata($certificate);
 			$crlNumber = $this->getNextCrlNumber($instanceId, $generation, $engineType);
 
 			$this->crlMapper->revokeCertificateEntity(
@@ -80,7 +77,11 @@ public function revokeCertificate(
 			);
 
 			return true;
-		} catch (\Exception) {
+		} catch (\Throwable $exception) {
+			$this->logger->warning('Failed to revoke certificate {serial}', [
+				'serial' => $serialNumber,
+				'error' => $exception->getMessage(),
+			]);
 			return false;
 		}
 	}
@@ -129,11 +130,8 @@ private function revokeCertificateList(
 
 		foreach ($certificates as $certificate) {
 			try {
-				$instanceId = $certificate->getInstanceId();
-				$generation = $certificate->getGeneration();
-				$engineType = $certificate->getEngine();
 				$serialNumber = $certificate->getSerialNumber();
-
+				['instanceId' => $instanceId, 'generation' => $generation, 'engineType' => $engineType] = $this->getCrlMetadata($certificate);
 				$crlNumber = $this->getNextCrlNumber($instanceId, $generation, $engineType);
 
 				$this->crlMapper->revokeCertificateEntity(
@@ -237,6 +235,25 @@ public function getRevokedCertificates(string $instanceId = '', int $generation
 		return $result;
 	}
 
+	/**
+	 * @return array{instanceId: string, generation: int, engineType: string}
+	 */
+	private function getCrlMetadata(\OCA\Libresign\Db\Crl $certificate): array {
+		$instanceId = $certificate->getInstanceId();
+		$generation = $certificate->getGeneration();
+		$engineType = $certificate->getEngine();
+
+		if ($instanceId === null || $generation === null || $engineType === '') {
+			throw new \RuntimeException('Certificate missing CRL metadata: instance_id, generation or engine');
+		}
+
+		return [
+			'instanceId' => $instanceId,
+			'generation' => $generation,
+			'engineType' => $engineType,
+		];
+	}
+
 	private function getNextCrlNumber(string $instanceId, int $generation, string $engineType): int {
 		$lastCrlNumber = $this->crlMapper->getLastCrlNumber($instanceId, $generation, $engineType);
 

tests/php/Unit/Service/Crl/CrlServiceTest.php

@@ -290,6 +290,37 @@ public function testRevokeCertificateWithValidReasonCode(): void {
 		$this->assertTrue($result);
 	}
 
+	public function testRevokeCertificateWithoutCrlMetadataFails(): void {
+		$serialNumber = '654321';
+		$certificate = new Crl();
+		$certificate->setSerialNumber($serialNumber);
+		$certificate->setEngine('openssl');
+
+		$this->crlMapper->expects($this->once())
+			->method('findBySerialNumber')
+			->with($serialNumber)
+			->willReturn($certificate);
+
+		$this->crlMapper->expects($this->never())
+			->method('getLastCrlNumber');
+
+		$this->crlMapper->expects($this->never())
+			->method('revokeCertificateEntity');
+
+		$this->logger->expects($this->once())
+			->method('warning')
+			->with(
+				'Failed to revoke certificate {serial}',
+				$this->callback(fn (array $context): bool => $context['serial'] === $serialNumber
+					&& isset($context['error'])
+					&& $context['error'] !== '')
+			);
+
+		$result = $this->service->revokeCertificate($serialNumber);
+
+		$this->assertFalse($result);
+	}
+
 	public function testGenerateCrlDerReturnsValidBinaryData(): void {
 		// Create revoked certificates data
 		$revokedCertificates = [

tests/php/Unit/Service/SerialNumberServiceTest.php

@@ -0,0 +1,57 @@
+<?php
+
+declare(strict_types=1);
+
+/**
+ * SPDX-FileCopyrightText: 2026 LibreCode coop and contributors
+ * SPDX-License-Identifier: AGPL-3.0-or-later
+ */
+
+namespace OCA\Libresign\Tests\Unit\Service;
+
+use DateTime;
+use OCA\Libresign\Db\CrlMapper;
+use OCA\Libresign\Service\SerialNumberService;
+use PHPUnit\Framework\MockObject\MockObject;
+use PHPUnit\Framework\TestCase;
+
+class SerialNumberServiceTest extends TestCase {
+	private CrlMapper&MockObject $crlMapper;
+	private SerialNumberService $service;
+
+	protected function setUp(): void {
+		parent::setUp();
+		$this->crlMapper = $this->createMock(CrlMapper::class);
+		$this->service = new SerialNumberService($this->crlMapper);
+	}
+
+	public function testGenerateUniqueSerialPersistsCrlMetadata(): void {
+		$expiresAt = new DateTime('2030-01-01 00:00:00');
+
+		$this->crlMapper->expects($this->once())
+			->method('createCertificate')
+			->with(
+				$this->callback(static fn (mixed $value): bool => is_string($value) && $value !== ''),
+				'test-owner',
+				'openssl',
+				'inst1234567',
+				4,
+				$this->isInstanceOf(DateTime::class),
+				$expiresAt,
+				null,
+				null,
+				'leaf'
+			);
+
+		$serial = $this->service->generateUniqueSerial(
+			'test-owner',
+			'inst1234567',
+			4,
+			$expiresAt,
+			'openssl',
+		);
+
+		$this->assertNotSame('', $serial);
+		$this->assertSame(1, preg_match('/^[0-9]+$/', $serial));
+	}
+}