Skip to content

Commit 249f083

Browse files
vitormattosvitormattos
committed
fix: prevent stale configPath and CA ID exposure in root certificate API
Filter configPath from API response when certificate is not generated to prevent form pre-population with outdated generation numbers that cause validation errors. Filter CA ID (libresign-ca-id:*) from OrganizationalUnit field to prevent users from submitting stale generation values that conflict with certificate validation. Refactor toArray() method by extracting logic into dedicated methods: - getConfigPathForApi(): Returns empty string for non-generated certificates - removeCaIdFromOrganizationalUnit(): Filters CA IDs from OU arrays Signed-off-by: Vitor Mattos <[email protected]>
1 parent 2d3f820 commit 249f083

2 files changed

Lines changed: 181 additions & 3 deletions

File tree

‎lib/Handler/CertificateEngine/AEngineHandler.php‎

Lines changed: 23 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -721,9 +721,10 @@ protected function checkRootCertificateExpiry(): ?ConfigureCheckHelper {
721721

722722
#[\Override]
723723
public function toArray(): array {
724+
$generated = $this->isSetupOk();
724725
$return = [
725-
'configPath' => $this->getCurrentConfigPath(),
726-
'generated' => $this->isSetupOk(),
726+
'configPath' => $this->getConfigPathForApi($generated),
727+
'generated' => $generated,
727728
'rootCert' => [
728729
'commonName' => $this->getCommonName(),
729730
'names' => [],
@@ -737,12 +738,31 @@ public function toArray(): array {
737738
foreach ($names as $name => $value) {
738739
$return['rootCert']['names'][] = [
739740
'id' => $name,
740-
'value' => $value,
741+
'value' => $this->filterNameValue($name, $value),
741742
];
742743
}
743744
return $return;
744745
}
745746

747+
private function getConfigPathForApi(bool $generated): string {
748+
return $generated ? $this->getCurrentConfigPath() : '';
749+
}
750+
751+
private function filterNameValue(string $name, mixed $value): mixed {
752+
if ($name === 'OU' && is_array($value)) {
753+
return $this->removeCaIdFromOrganizationalUnit($value);
754+
}
755+
return $value;
756+
}
757+
758+
private function removeCaIdFromOrganizationalUnit(array $organizationalUnits): array {
759+
$filtered = array_filter(
760+
$organizationalUnits,
761+
fn ($item) => !str_starts_with($item, 'libresign-ca-id:')
762+
);
763+
return array_values($filtered);
764+
}
765+
746766
protected function getCrlDistributionUrl(): string {
747767
$caIdParsed = $this->caIdentifierService->getCaIdParsed();
748768
return $this->urlGenerator->linkToRouteAbsolute('libresign.crl.getRevocationList', [

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

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

0 commit comments

Comments
 (0)