Skip to content

Commit 080c2c2

Browse files
vitormattosvitormattos
authored
Merge pull request #6202 from LibreSign/backport/6201/stable31
[stable31] fix: prevent stale configPath and CA ID exposure in root certificate API
2 parents 67ae46a + c81867b commit 080c2c2

21 files changed

Lines changed: 233 additions & 24 deletions

‎lib/Handler/CertificateEngine/AEngineHandler.php‎

Lines changed: 24 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -709,9 +709,10 @@ protected function checkRootCertificateExpiry(): ?ConfigureCheckHelper {
709709
}
710710

711711
public function toArray(): array {
712+
$generated = $this->isSetupOk();
712713
$return = [
713-
'configPath' => $this->getCurrentConfigPath(),
714-
'generated' => $this->isSetupOk(),
714+
'configPath' => $this->getConfigPathForApi($generated),
715+
'generated' => $generated,
715716
'rootCert' => [
716717
'commonName' => $this->getCommonName(),
717718
'names' => [],
@@ -725,12 +726,32 @@ public function toArray(): array {
725726
foreach ($names as $name => $value) {
726727
$return['rootCert']['names'][] = [
727728
'id' => $name,
728-
'value' => $value,
729+
'value' => $this->filterNameValue($name, $value, $generated),
729730
];
730731
}
731732
return $return;
732733
}
733734

735+
private function getConfigPathForApi(bool $generated): string {
736+
return $generated ? $this->getCurrentConfigPath() : '';
737+
}
738+
739+
private function filterNameValue(string $name, mixed $value, bool $generated): mixed {
740+
if ($name === 'OU' && is_array($value) && !$generated) {
741+
$filtered = $this->removeCaIdFromOrganizationalUnit($value);
742+
return empty($filtered) ? null : $filtered;
743+
}
744+
return $value;
745+
}
746+
747+
private function removeCaIdFromOrganizationalUnit(array $organizationalUnits): array {
748+
$filtered = array_filter(
749+
$organizationalUnits,
750+
fn ($item) => !str_starts_with($item, 'libresign-ca-id:')
751+
);
752+
return array_values($filtered);
753+
}
754+
734755
protected function getCrlDistributionUrl(): string {
735756
$caIdParsed = $this->caIdentifierService->getCaIdParsed();
736757
return $this->urlGenerator->linkToRouteAbsolute('libresign.crl.getRevocationList', [

‎tests/php/Unit/Handler/CertificateEngine/AEngineHandlerTest.php‎

Lines changed: 178 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -32,17 +32,14 @@ final class AEngineHandlerTest extends \OCA\Libresign\Tests\Unit\TestCase {
3232

3333
public function setUp(): void {
3434
$this->config = \OCP\Server::get(IConfig::class);
35-
$this->appConfig = $this->getMockAppConfig();
35+
$this->appConfig = $this->getMockAppConfigWithReset();
3636
$this->appDataFactory = \OCP\Server::get(IAppDataFactory::class);
3737
$this->dateTimeFormatter = \OCP\Server::get(IDateTimeFormatter::class);
3838
$this->tempManager = \OCP\Server::get(ITempManager::class);
3939
$this->certificatePolicyService = \OCP\Server::get(CertificatePolicyService::class);
4040
$this->urlGenerator = \OCP\Server::get(IURLGenerator::class);
4141
$this->caIdentifierService = \OCP\Server::get(CaIdentifierService::class);
4242
$this->logger = \OCP\Server::get(LoggerInterface::class);
43-
44-
$this->appConfig->deleteKey(Application::APP_ID, 'certificate_engine');
45-
$this->appConfig->deleteKey(Application::APP_ID, 'identify_methods');
4643
}
4744

4845
private function getInstance(): OpenSslHandler {
@@ -232,4 +229,181 @@ public static function dataProviderIdentifyMethodsOtherEngines(): array {
232229
'First time cfssl' => ['', 'cfssl', null, 'no previous config'],
233230
];
234231
}
232+
233+
#[DataProvider('dataProviderToArray')]
234+
public function testToArrayReturnsExpectedStructure(
235+
bool $isSetupOk,
236+
array $certificateData,
237+
array $expectedKeys,
238+
string $description,
239+
): void {
240+
$instance = $this->getInstance();
241+
242+
foreach ($certificateData as $setter => $value) {
243+
$method = 'set' . ucfirst($setter);
244+
if (method_exists($instance, $method)) {
245+
$instance->$method($value);
246+
}
247+
}
248+
249+
if (!$isSetupOk) {
250+
$this->appConfig->deleteKey(Application::APP_ID, 'config_path');
251+
}
252+
253+
$result = $instance->toArray();
254+
255+
foreach ($expectedKeys as $key) {
256+
$this->assertArrayHasKey($key, $result, "toArray() should contain '$key': $description");
257+
}
258+
259+
$this->assertIsBool($result['generated'], "generated should be boolean: $description");
260+
}
261+
262+
#[DataProvider('dataProviderToArrayConfigPath')]
263+
public function testToArrayFiltersConfigPathWhenNotGenerated(
264+
bool $certificateGenerated,
265+
string $expectedConfigPath,
266+
string $description,
267+
): void {
268+
$instance = $this->getInstance();
269+
270+
$tempPath = $this->tempManager->getTemporaryFolder('test-config');
271+
$instance->setConfigPath($tempPath);
272+
273+
if ($certificateGenerated) {
274+
file_put_contents($tempPath . DIRECTORY_SEPARATOR . 'ca.pem', 'fake-cert');
275+
file_put_contents($tempPath . DIRECTORY_SEPARATOR . 'ca-key.pem', 'fake-key');
276+
}
277+
278+
$result = $instance->toArray();
279+
280+
if ($expectedConfigPath === '') {
281+
$this->assertEmpty($result['configPath'], "configPath should be empty: $description");
282+
} else {
283+
$this->assertNotEmpty($result['configPath'], "configPath should not be empty: $description");
284+
}
285+
286+
$this->assertEquals($certificateGenerated, $result['generated'], "generated flag: $description");
287+
}
288+
289+
#[DataProvider('dataProviderToArrayCaIdFiltering')]
290+
public function testToArrayFiltersCaIdFromOrganizationalUnitWhenNotGenerated(
291+
bool $certificateGenerated,
292+
array $organizationalUnits,
293+
array $expectedOuValues,
294+
string $description,
295+
): void {
296+
$instance = $this->getInstance();
297+
298+
$tempPath = $this->tempManager->getTemporaryFolder('test-config');
299+
$instance->setConfigPath($tempPath);
300+
$instance->setOrganizationalUnit($organizationalUnits);
301+
$instance->setCountry('BR');
302+
303+
if ($certificateGenerated) {
304+
file_put_contents($tempPath . DIRECTORY_SEPARATOR . 'ca.pem', 'fake-cert');
305+
file_put_contents($tempPath . DIRECTORY_SEPARATOR . 'ca-key.pem', 'fake-key');
306+
}
307+
308+
$result = $instance->toArray();
309+
310+
$ouFound = null;
311+
foreach ($result['rootCert']['names'] as $name) {
312+
if ($name['id'] === 'OU') {
313+
$ouFound = $name['value'];
314+
break;
315+
}
316+
}
317+
318+
if (!empty($expectedOuValues)) {
319+
$this->assertNotNull($ouFound, "OU should be present in names array: $description");
320+
$this->assertEquals(
321+
$expectedOuValues,
322+
$ouFound,
323+
"OrganizationalUnit filtering: $description"
324+
);
325+
} else {
326+
$this->assertNull($ouFound, "OU should not be present when filtered to empty: $description");
327+
}
328+
}
329+
330+
public static function dataProviderToArray(): array {
331+
return [
332+
'Basic structure with minimal data' => [
333+
false,
334+
['commonName' => 'Test CA'],
335+
['configPath', 'generated', 'rootCert', 'policySection'],
336+
'minimal certificate data',
337+
],
338+
'Complete certificate data' => [
339+
false,
340+
[
341+
'commonName' => 'LibreSign CA',
342+
'country' => 'BR',
343+
'state' => 'Santa Catarina',
344+
'locality' => 'Florianópolis',
345+
'organization' => 'LibreCode',
346+
'organizationalUnit' => ['Engineering', 'Security'],
347+
],
348+
['configPath', 'generated', 'rootCert', 'policySection'],
349+
'full certificate data',
350+
],
351+
];
352+
}
353+
354+
public static function dataProviderToArrayConfigPath(): array {
355+
return [
356+
'Certificate not generated' => [
357+
false,
358+
'',
359+
'configPath should be empty when certificate not generated',
360+
],
361+
'Certificate generated' => [
362+
true,
363+
'/path/to/config',
364+
'configPath should be set when certificate is generated',
365+
],
366+
];
367+
}
368+
369+
public static function dataProviderToArrayCaIdFiltering(): array {
370+
return [
371+
'OU without CA ID - not generated' => [
372+
false,
373+
['Engineering', 'Security'],
374+
['Engineering', 'Security'],
375+
'normal OU values should pass through when not generated',
376+
],
377+
'OU without CA ID - generated' => [
378+
true,
379+
['Engineering', 'Security'],
380+
['Engineering', 'Security'],
381+
'normal OU values should pass through when generated',
382+
],
383+
'OU with CA ID - not generated (filtered)' => [
384+
false,
385+
['libresign-ca-id:abc123_g:1_e:openssl', 'Engineering', 'Security'],
386+
['Engineering', 'Security'],
387+
'CA ID should be filtered when certificate not generated',
388+
],
389+
'OU with CA ID - generated (kept)' => [
390+
true,
391+
['libresign-ca-id:abc123_g:1_e:openssl', 'Engineering', 'Security'],
392+
['libresign-ca-id:abc123_g:1_e:openssl', 'Engineering', 'Security'],
393+
'CA ID should be kept when certificate is generated',
394+
],
395+
'OU with only CA ID - not generated' => [
396+
false,
397+
['libresign-ca-id:abc123_g:1_e:openssl'],
398+
[],
399+
'OU should be empty when only CA ID and not generated',
400+
],
401+
'OU with only CA ID - generated' => [
402+
true,
403+
['libresign-ca-id:abc123_g:1_e:openssl'],
404+
['libresign-ca-id:abc123_g:1_e:openssl'],
405+
'OU with only CA ID should be kept when generated',
406+
],
407+
];
408+
}
235409
}

‎tests/php/Unit/Handler/CertificateEngine/CertificateEngineFactoryTest.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ public function tearDown(): void {
2828
}
2929

3030
public function setUp(): void {
31-
$this->appConfig = $this->getMockAppConfig();
31+
$this->appConfig = $this->getMockAppConfigWithReset();
3232
$this->openSslHandler = $this->createMock(OpenSslHandler::class);
3333
$this->cfsslHandler = $this->createMock(CfsslHandler::class);
3434
$this->noneHandler = $this->createMock(NoneHandler::class);

‎tests/php/Unit/Handler/CertificateEngine/OpenSslHandlerTest.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ final class OpenSslHandlerTest extends \OCA\Libresign\Tests\Unit\TestCase {
3939
private string $tempDir;
4040
public function setUp(): void {
4141
$this->config = \OCP\Server::get(IConfig::class);
42-
$this->appConfig = $this->getMockAppConfig();
42+
$this->appConfig = $this->getMockAppConfigWithReset();
4343
$this->appDataFactory = \OCP\Server::get(IAppDataFactory::class);
4444
$this->dateTimeFormatter = \OCP\Server::get(IDateTimeFormatter::class);
4545
$this->tempManager = \OCP\Server::get(ITempManager::class);

‎tests/php/Unit/Handler/FooterHandlerTest.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ final class FooterHandlerTest extends \OCA\Libresign\Tests\Unit\TestCase {
2727
private ITempManager $tempManager;
2828
private FooterHandler $footerHandler;
2929
public function setUp(): void {
30-
$this->appConfig = $this->getMockAppConfig();
30+
$this->appConfig = $this->getMockAppConfigWithReset();
3131
$this->pdfParserService = $this->createMock(PdfParserService::class);
3232
$this->urlGenerator = $this->createMock(IURLGenerator::class);
3333
$this->tempManager = \OCP\Server::get(ITempManager::class);

‎tests/php/Unit/Handler/PdfTest.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ final class PdfTest extends \OCA\Libresign\Tests\Unit\TestCase {
2424
public function setUp(): void {
2525
parent::setUp();
2626
$this->javaHelper = $this->createMock(JavaHelper::class);
27-
$this->appConfig = $this->getMockAppConfig();
27+
$this->appConfig = $this->getMockAppConfigWithReset();
2828
$this->l10n = \OCP\Server::get(IL10NFactory::class)->get(Application::APP_ID);
2929
}
3030

‎tests/php/Unit/Handler/SignEngine/JSignPdfHandlerTest.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,7 @@ public static function setUpBeforeClass(): void {
6363
}
6464
}
6565
public function setUp(): void {
66-
$this->appConfig = $this->getMockAppConfig();
66+
$this->appConfig = $this->getMockAppConfigWithReset();
6767
$this->loggerInterface = $this->createMock(LoggerInterface::class);
6868
$this->tempManager = \OCP\Server::get(ITempManager::class);
6969
$this->signatureBackgroundService = $this->createMock(SignatureBackgroundService::class);

‎tests/php/Unit/Handler/SignEngine/Pkcs12HandlerTest.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ final class Pkcs12HandlerTest extends \OCA\Libresign\Tests\Unit\TestCase {
3737

3838
public function setUp(): void {
3939
$this->folderService = $this->createMock(FolderService::class);
40-
$this->appConfig = $this->getMockAppConfig();
40+
$this->appConfig = $this->getMockAppConfigWithReset();
4141
$this->certificateEngineFactory = $this->createMock(CertificateEngineFactory::class);
4242
$this->l10n = \OCP\Server::get(IL10NFactory::class)->get(Application::APP_ID);
4343
$this->footerHandler = $this->createMock(FooterHandler::class);

‎tests/php/Unit/Helper/JavaHelperTest.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ class JavaHelperTest extends \OCA\Libresign\Tests\Unit\TestCase {
2323
public function setUp(): void {
2424
parent::setUp();
2525

26-
$this->appConfig = $this->getMockAppConfig();
26+
$this->appConfig = $this->getMockAppConfigWithReset();
2727
$this->l10n = $this->createMock(IL10N::class);
2828
$this->logger = $this->createMock(LoggerInterface::class);
2929
}

‎tests/php/Unit/Service/CertificatePolicyServiceTest.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ final class CertificatePolicyServiceTest extends \OCA\Libresign\Tests\Unit\TestC
3333
public function setUp(): void {
3434
$this->appData = $this->createMock(IAppData::class);
3535
$this->urlGenerator = $this->createMock(IURLGenerator::class);
36-
$this->appConfig = $this->getMockAppConfig();
36+
$this->appConfig = $this->getMockAppConfigWithReset();
3737
$this->l10n = \OCP\Server::get(IL10NFactory::class)->get(Application::APP_ID);
3838
}
3939

0 commit comments

Comments
 (0)