Skip to content

Commit 6568a42

Browse files
vitormattosvitormattos
authored
Merge pull request #7532 from LibreSign/backport/7531/stable33
[stable33] fix(crl): enforce CRL metadata with scoped legacy backfill
2 parents 13d1c4a + 356409b commit 6568a42

4 files changed

Lines changed: 285 additions & 9 deletions

File tree

‎lib/Migration/Version17004Date20260421000000.php‎

Lines changed: 171 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,171 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
/**
6+
* SPDX-FileCopyrightText: 2026 LibreCode coop and contributors
7+
* SPDX-License-Identifier: AGPL-3.0-or-later
8+
*/
9+
10+
namespace OCA\Libresign\Migration;
11+
12+
use Closure;
13+
use OCA\Libresign\AppInfo\Application;
14+
use OCP\DB\ISchemaWrapper;
15+
use OCP\DB\QueryBuilder\IQueryBuilder;
16+
use OCP\IAppConfig;
17+
use OCP\IConfig;
18+
use OCP\IDBConnection;
19+
use OCP\Migration\IOutput;
20+
use OCP\Migration\SimpleMigrationStep;
21+
22+
class Version17004Date20260421000000 extends SimpleMigrationStep {
23+
public function __construct(
24+
private IConfig $config,
25+
private IAppConfig $appConfig,
26+
private IDBConnection $connection,
27+
) {
28+
}
29+
30+
/**
31+
* @param IOutput $output
32+
* @param Closure(): ISchemaWrapper $schemaClosure
33+
* @param array $options
34+
*/
35+
#[\Override]
36+
public function preSchemaChange(IOutput $output, Closure $schemaClosure, array $options): void {
37+
/** @var ISchemaWrapper $schema */
38+
$schema = $schemaClosure();
39+
if (!$schema->hasTable('libresign_crl')) {
40+
return;
41+
}
42+
43+
$metadata = $this->resolveMetadataDefaults();
44+
if ($metadata === null) {
45+
$output->warning('Skipped CRL metadata backfill because no deterministic metadata source was found.');
46+
return;
47+
}
48+
49+
$updatedInstance = $this->backfillInstanceId($metadata['instanceId']);
50+
$updatedGeneration = $this->backfillGeneration($metadata['generation']);
51+
$updatedEngine = $this->backfillEngine($metadata['engine']);
52+
53+
if ($updatedInstance > 0 || $updatedGeneration > 0 || $updatedEngine > 0) {
54+
$output->warning(sprintf(
55+
'Backfilled CRL metadata for legacy rows (instance_id=%d, generation=%d, engine=%d).',
56+
$updatedInstance,
57+
$updatedGeneration,
58+
$updatedEngine,
59+
));
60+
}
61+
}
62+
63+
/**
64+
* @param IOutput $output
65+
* @param Closure(): ISchemaWrapper $schemaClosure
66+
* @param array $options
67+
* @return null|ISchemaWrapper
68+
*/
69+
#[\Override]
70+
public function changeSchema(IOutput $output, Closure $schemaClosure, array $options): ?ISchemaWrapper {
71+
/** @var ISchemaWrapper $schema */
72+
return $schemaClosure();
73+
}
74+
75+
/**
76+
* @return array{instanceId: string, generation: int, engine: string}|null
77+
*/
78+
private function resolveMetadataDefaults(): ?array {
79+
$engine = $this->appConfig->getValueString(Application::APP_ID, 'certificate_engine', 'openssl');
80+
if ($engine === '' || $engine === 'none') {
81+
$engine = 'openssl';
82+
}
83+
84+
$instanceId = $this->appConfig->getValueString(Application::APP_ID, 'instance_id', '');
85+
$generation = max(1, $this->appConfig->getValueInt(Application::APP_ID, 'ca_generation_counter', 1));
86+
$caId = $this->appConfig->getValueString(Application::APP_ID, 'ca_id', '');
87+
88+
$pattern = '/^libresign-ca-id:(?P<instanceId>[a-z0-9]+)_g:(?P<generation>\d+)_e:(?P<engineType>[oc])$/';
89+
if ($caId !== '' && preg_match($pattern, $caId, $matches)) {
90+
$instanceId = $matches['instanceId'];
91+
$generation = max(1, (int)$matches['generation']);
92+
$engine = $matches['engineType'] === 'c' ? 'cfssl' : 'openssl';
93+
}
94+
95+
if ($instanceId === '') {
96+
$instanceId = $this->config->getSystemValueString('instanceid', '');
97+
}
98+
99+
if ($instanceId === '' || !in_array($engine, ['openssl', 'cfssl'], true)) {
100+
return null;
101+
}
102+
103+
return [
104+
'instanceId' => $instanceId,
105+
'generation' => $generation,
106+
'engine' => $engine,
107+
];
108+
}
109+
110+
private function backfillInstanceId(string $instanceId): int {
111+
$qb = $this->connection->getQueryBuilder();
112+
113+
return $qb->update('libresign_crl')
114+
->set('instance_id', $qb->createNamedParameter($instanceId))
115+
->where($qb->expr()->eq('status', $qb->createNamedParameter('issued')))
116+
->andWhere(
117+
$qb->expr()->orX(
118+
$qb->expr()->isNull('generation'),
119+
$qb->expr()->isNull('engine'),
120+
$qb->expr()->eq('engine', $qb->createNamedParameter('')),
121+
)
122+
)
123+
->andWhere(
124+
$qb->expr()->orX(
125+
$qb->expr()->isNull('instance_id'),
126+
$qb->expr()->eq('instance_id', $qb->createNamedParameter('')),
127+
)
128+
)
129+
->executeStatement();
130+
}
131+
132+
private function backfillGeneration(int $generation): int {
133+
$qb = $this->connection->getQueryBuilder();
134+
135+
return $qb->update('libresign_crl')
136+
->set('generation', $qb->createNamedParameter($generation, IQueryBuilder::PARAM_INT))
137+
->where($qb->expr()->eq('status', $qb->createNamedParameter('issued')))
138+
->andWhere(
139+
$qb->expr()->orX(
140+
$qb->expr()->isNull('instance_id'),
141+
$qb->expr()->eq('instance_id', $qb->createNamedParameter('')),
142+
$qb->expr()->isNull('engine'),
143+
$qb->expr()->eq('engine', $qb->createNamedParameter('')),
144+
)
145+
)
146+
->andWhere($qb->expr()->isNull('generation'))
147+
->executeStatement();
148+
}
149+
150+
private function backfillEngine(string $engine): int {
151+
$qb = $this->connection->getQueryBuilder();
152+
153+
return $qb->update('libresign_crl')
154+
->set('engine', $qb->createNamedParameter($engine))
155+
->where($qb->expr()->eq('status', $qb->createNamedParameter('issued')))
156+
->andWhere(
157+
$qb->expr()->orX(
158+
$qb->expr()->isNull('instance_id'),
159+
$qb->expr()->eq('instance_id', $qb->createNamedParameter('')),
160+
$qb->expr()->isNull('generation'),
161+
)
162+
)
163+
->andWhere(
164+
$qb->expr()->orX(
165+
$qb->expr()->isNull('engine'),
166+
$qb->expr()->eq('engine', $qb->createNamedParameter('')),
167+
)
168+
)
169+
->executeStatement();
170+
}
171+
}

‎lib/Service/Crl/CrlService.php‎

Lines changed: 26 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -64,10 +64,7 @@ public function revokeCertificate(
6464

6565
try {
6666
$certificate = $this->crlMapper->findBySerialNumber($serialNumber);
67-
$instanceId = $certificate->getInstanceId();
68-
$generation = $certificate->getGeneration();
69-
$engineType = $certificate->getEngine();
70-
67+
['instanceId' => $instanceId, 'generation' => $generation, 'engineType' => $engineType] = $this->getCrlMetadata($certificate);
7168
$crlNumber = $this->getNextCrlNumber($instanceId, $generation, $engineType);
7269

7370
$this->crlMapper->revokeCertificateEntity(
@@ -80,7 +77,11 @@ public function revokeCertificate(
8077
);
8178

8279
return true;
83-
} catch (\Exception) {
80+
} catch (\Throwable $exception) {
81+
$this->logger->warning('Failed to revoke certificate {serial}', [
82+
'serial' => $serialNumber,
83+
'error' => $exception->getMessage(),
84+
]);
8485
return false;
8586
}
8687
}
@@ -129,11 +130,8 @@ private function revokeCertificateList(
129130

130131
foreach ($certificates as $certificate) {
131132
try {
132-
$instanceId = $certificate->getInstanceId();
133-
$generation = $certificate->getGeneration();
134-
$engineType = $certificate->getEngine();
135133
$serialNumber = $certificate->getSerialNumber();
136-
134+
['instanceId' => $instanceId, 'generation' => $generation, 'engineType' => $engineType] = $this->getCrlMetadata($certificate);
137135
$crlNumber = $this->getNextCrlNumber($instanceId, $generation, $engineType);
138136

139137
$this->crlMapper->revokeCertificateEntity(
@@ -237,6 +235,25 @@ public function getRevokedCertificates(string $instanceId = '', int $generation
237235
return $result;
238236
}
239237

238+
/**
239+
* @return array{instanceId: string, generation: int, engineType: string}
240+
*/
241+
private function getCrlMetadata(\OCA\Libresign\Db\Crl $certificate): array {
242+
$instanceId = $certificate->getInstanceId();
243+
$generation = $certificate->getGeneration();
244+
$engineType = $certificate->getEngine();
245+
246+
if ($instanceId === null || $generation === null || $engineType === '') {
247+
throw new \RuntimeException('Certificate missing CRL metadata: instance_id, generation or engine');
248+
}
249+
250+
return [
251+
'instanceId' => $instanceId,
252+
'generation' => $generation,
253+
'engineType' => $engineType,
254+
];
255+
}
256+
240257
private function getNextCrlNumber(string $instanceId, int $generation, string $engineType): int {
241258
$lastCrlNumber = $this->crlMapper->getLastCrlNumber($instanceId, $generation, $engineType);
242259

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

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -290,6 +290,37 @@ public function testRevokeCertificateWithValidReasonCode(): void {
290290
$this->assertTrue($result);
291291
}
292292

293+
public function testRevokeCertificateWithoutCrlMetadataFails(): void {
294+
$serialNumber = '654321';
295+
$certificate = new Crl();
296+
$certificate->setSerialNumber($serialNumber);
297+
$certificate->setEngine('openssl');
298+
299+
$this->crlMapper->expects($this->once())
300+
->method('findBySerialNumber')
301+
->with($serialNumber)
302+
->willReturn($certificate);
303+
304+
$this->crlMapper->expects($this->never())
305+
->method('getLastCrlNumber');
306+
307+
$this->crlMapper->expects($this->never())
308+
->method('revokeCertificateEntity');
309+
310+
$this->logger->expects($this->once())
311+
->method('warning')
312+
->with(
313+
'Failed to revoke certificate {serial}',
314+
$this->callback(fn (array $context): bool => $context['serial'] === $serialNumber
315+
&& isset($context['error'])
316+
&& $context['error'] !== '')
317+
);
318+
319+
$result = $this->service->revokeCertificate($serialNumber);
320+
321+
$this->assertFalse($result);
322+
}
323+
293324
public function testGenerateCrlDerReturnsValidBinaryData(): void {
294325
// Create revoked certificates data
295326
$revokedCertificates = [

‎tests/php/Unit/Service/SerialNumberServiceTest.php‎

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,57 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
/**
6+
* SPDX-FileCopyrightText: 2026 LibreCode coop and contributors
7+
* SPDX-License-Identifier: AGPL-3.0-or-later
8+
*/
9+
10+
namespace OCA\Libresign\Tests\Unit\Service;
11+
12+
use DateTime;
13+
use OCA\Libresign\Db\CrlMapper;
14+
use OCA\Libresign\Service\SerialNumberService;
15+
use PHPUnit\Framework\MockObject\MockObject;
16+
use PHPUnit\Framework\TestCase;
17+
18+
class SerialNumberServiceTest extends TestCase {
19+
private CrlMapper&MockObject $crlMapper;
20+
private SerialNumberService $service;
21+
22+
protected function setUp(): void {
23+
parent::setUp();
24+
$this->crlMapper = $this->createMock(CrlMapper::class);
25+
$this->service = new SerialNumberService($this->crlMapper);
26+
}
27+
28+
public function testGenerateUniqueSerialPersistsCrlMetadata(): void {
29+
$expiresAt = new DateTime('2030-01-01 00:00:00');
30+
31+
$this->crlMapper->expects($this->once())
32+
->method('createCertificate')
33+
->with(
34+
$this->callback(static fn (mixed $value): bool => is_string($value) && $value !== ''),
35+
'test-owner',
36+
'openssl',
37+
'inst1234567',
38+
4,
39+
$this->isInstanceOf(DateTime::class),
40+
$expiresAt,
41+
null,
42+
null,
43+
'leaf'
44+
);
45+
46+
$serial = $this->service->generateUniqueSerial(
47+
'test-owner',
48+
'inst1234567',
49+
4,
50+
$expiresAt,
51+
'openssl',
52+
);
53+
54+
$this->assertNotSame('', $serial);
55+
$this->assertSame(1, preg_match('/^[0-9]+$/', $serial));
56+
}
57+
}

0 commit comments

Comments
 (0)