Skip to content

Commit ad942f0

Browse files
vitormattosvitormattos
committed
chore: Refactor certificate chain processing with robust ordering algorithm and validation
- **Certificate Ordering (OrderCertificatesTrait):** - Implement new leaf-first ordering algorithm with proper chain building - Add Distinguished Name normalization for reliable certificate comparison - Introduce comprehensive chain validation with `validateCertificateChain()` - Handle complex multi-level certificate hierarchies and edge cases - Improve error handling with detailed validation messages - **PDF Signature Processing (Pkcs12Handler):** - Refactor `getCertificateChain()` method with extracted helper functions - Improve TSA timestamp processing and certificate chain extraction - Clean up code organization and remove unnecessary comments - Enhance certificate data enrichment from Poppler utilities - **Test Coverage Improvements:** - Add 300+ lines of comprehensive test cases covering real-world scenarios - Include banking, corporate, and e-commerce certificate chain examples - Add validation tests for complete, incomplete, and invalid chains - Refactor Pkcs12Handler tests to focus on public interface without reflection - Remove excessive mocking in favor of behavioral testing - **Code Quality:** - Follow best practices by testing only public methods - Eliminate reflection-based testing for better maintainability - Improve test reliability and reduce brittleness to internal changes - Clean up code structure and remove redundant documentation This enhancement significantly improves certificate chain handling reliability for digital signature validation in LibreSign, with comprehensive test coverage ensuring robustness across various PKI scenarios. Signed-off-by: Vitor Mattos <1079143+vitormattos@users.noreply.github.com>
1 parent 24c45fa commit ad942f0

4 files changed

Lines changed: 795 additions & 166 deletions

File tree

‎lib/Handler/CertificateEngine/OrderCertificatesTrait.php‎

Lines changed: 121 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -12,66 +12,152 @@
1212

1313
trait OrderCertificatesTrait {
1414
public function orderCertificates(array $certificates): array {
15-
$this->validateCertificateStructure($certificates);
16-
$remainingCerts = [];
15+
if (empty($certificates)) {
16+
throw new InvalidArgumentException('Certificate list cannot be empty');
17+
}
18+
19+
$this->ensureValidStructure($certificates);
20+
21+
return match (true) {
22+
count($certificates) === 1 => $certificates,
23+
default => $this->buildChain($certificates)
24+
};
25+
}
26+
27+
private function buildChain(array $certificates): array {
28+
$leaf = $this->findLeafCertificate($certificates);
29+
if (!$leaf) {
30+
return $certificates;
31+
}
1732

18-
// Add the root cert at ordered list and collect the remaining certs
33+
$ordered = [$leaf];
34+
$remaining = $this->excludeCertificate($certificates, $leaf);
35+
36+
while ($remaining && !$this->isRootCertificate(end($ordered))) {
37+
[$next, $remaining] = $this->findIssuer(end($ordered), $remaining);
38+
if (!$next) {
39+
break;
40+
}
41+
$ordered[] = $next;
42+
}
43+
44+
return [...$ordered, ...$remaining];
45+
}
46+
47+
private function ensureValidStructure(array $certificates): void {
1948
foreach ($certificates as $cert) {
20-
if (!$this->arrayDiffCanonicalized($cert['subject'], $cert['issuer'])) {
21-
$ordered = [$cert];
22-
continue;
49+
if (!is_array($cert) || !isset($cert['subject'], $cert['issuer'], $cert['name'])
50+
|| !is_array($cert['subject']) || !is_array($cert['issuer'])) {
51+
throw new InvalidArgumentException('Invalid certificate structure. Certificate must have "subject", "issuer", and "name".');
2352
}
24-
$remainingCerts[$cert['name']] = $cert;
2553
}
2654

27-
if (!isset($ordered)) {
28-
return $certificates;
55+
$names = array_column($certificates, 'name');
56+
if (count($names) !== count(array_unique($names))) {
57+
throw new InvalidArgumentException('Duplicate certificate names detected');
2958
}
59+
}
3060

61+
private function findLeafCertificate(array $certificates): ?array {
62+
$issuers = [];
63+
foreach ($certificates as $cert) {
64+
if (isset($cert['issuer'])) {
65+
$issuers[] = $this->normalizeDistinguishedName($cert['issuer']);
66+
}
67+
}
3168

32-
while (!empty($remainingCerts)) {
33-
$found = false;
34-
foreach ($remainingCerts as $name => $cert) {
35-
$first = reset($ordered);
36-
if (!$this->arrayDiffCanonicalized($first['subject'], $cert['issuer'])) {
37-
array_unshift($ordered, $cert);
38-
unset($remainingCerts[$name]);
39-
$found = true;
40-
break;
41-
}
69+
foreach ($certificates as $cert) {
70+
if (!isset($cert['subject'])) {
71+
continue;
72+
}
73+
$subject = $this->normalizeDistinguishedName($cert['subject']);
74+
if (!in_array($subject, $issuers, true)) {
75+
return $cert;
4276
}
77+
}
78+
79+
return $certificates[0] ?? null;
80+
}
4381

44-
if (!$found) {
45-
throw new InvalidArgumentException('Certificate chain is incomplete or invalid.');
82+
private function findIssuer(array $cert, array $certificates): array {
83+
foreach ($certificates as $index => $candidate) {
84+
if ($this->isIssuedBy($cert, $candidate)) {
85+
unset($certificates[$index]);
86+
return [$candidate, array_values($certificates)];
4687
}
4788
}
89+
return [null, $certificates];
90+
}
91+
92+
private function excludeCertificate(array $certificates, array $toExclude): array {
93+
return array_values(array_filter($certificates, fn ($cert) => $cert['name'] !== $toExclude['name']));
94+
}
95+
96+
private function isRootCertificate(array $cert): bool {
97+
if (!isset($cert['subject'], $cert['issuer']) || !is_array($cert['subject']) || !is_array($cert['issuer'])) {
98+
return false;
99+
}
100+
return $this->normalizeDistinguishedName($cert['subject']) === $this->normalizeDistinguishedName($cert['issuer']);
101+
}
102+
103+
private function isIssuedBy(array $child, array $parent): bool {
104+
if (!isset($child['issuer'], $parent['subject']) || !is_array($child['issuer']) || !is_array($parent['subject'])) {
105+
return false;
106+
}
107+
return $this->normalizeDistinguishedName($child['issuer']) === $this->normalizeDistinguishedName($parent['subject']);
108+
}
109+
110+
private function normalizeDistinguishedName(array $dn): string {
111+
ksort($dn);
112+
return json_encode($dn, JSON_THROW_ON_ERROR);
113+
}
48114

49-
return $ordered;
115+
public function validateCertificateChain(array $certificates): array {
116+
return [
117+
'valid' => $this->isValidChain($certificates),
118+
'hasRoot' => $this->hasRootCertificate($certificates),
119+
'isComplete' => $this->isCompleteChain($certificates),
120+
'length' => count($certificates),
121+
];
50122
}
51123

52-
private function validateCertificateStructure(array $certificates): void {
124+
private function isValidChain(array $certificates): bool {
53125
if (empty($certificates)) {
54-
throw new InvalidArgumentException('Certificate list cannot be empty');
126+
return false;
55127
}
56128

57129
foreach ($certificates as $cert) {
58-
if (!isset($cert['subject'], $cert['issuer'], $cert['name'])) {
59-
throw new InvalidArgumentException(
60-
'Invalid certificate structure. Certificate must have "subject", "issuer", and "name".'
61-
);
130+
if (!is_array($cert) || !isset($cert['subject'], $cert['issuer'], $cert['name'])
131+
|| !is_array($cert['subject']) || !is_array($cert['issuer'])
132+
|| empty($cert['subject']['CN']) || empty($cert['issuer']['CN'])) {
133+
return false;
62134
}
63135
}
64136

65-
$names = array_column($certificates, 'name');
66-
if (count($names) !== count(array_unique($names))) {
67-
throw new InvalidArgumentException('Duplicate certificate names detected');
137+
return true;
138+
}
139+
140+
private function hasRootCertificate(array $certificates): bool {
141+
foreach ($certificates as $cert) {
142+
if ($this->isRootCertificate($cert)) {
143+
return true;
144+
}
68145
}
146+
return false;
69147
}
70148

71-
private function arrayDiffCanonicalized(array $array1, array $array2): array {
72-
sort($array1);
73-
sort($array2);
149+
private function isCompleteChain(array $certificates): bool {
150+
if (!$this->hasRootCertificate($certificates)) {
151+
return false;
152+
}
153+
154+
$ordered = $this->orderCertificates($certificates);
155+
for ($i = 0; $i < count($ordered) - 1; $i++) {
156+
if (!$this->isIssuedBy($ordered[$i], $ordered[$i + 1])) {
157+
return false;
158+
}
159+
}
74160

75-
return array_diff($array1, $array2);
161+
return true;
76162
}
77163
}

‎lib/Handler/SignEngine/Pkcs12Handler.php‎

Lines changed: 79 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -26,9 +26,6 @@ class Pkcs12Handler extends SignEngineHandler {
2626
use OrderCertificatesTrait;
2727
protected string $certificate = '';
2828
private array $signaturesFromPoppler = [];
29-
/**
30-
* Used by method self::getHandler()
31-
*/
3229
private ?JSignPdfHandler $jSignPdfHandler = null;
3330

3431
public function __construct(
@@ -58,11 +55,8 @@ private function getSignatures($resource): iterable {
5855
}
5956

6057
for ($i = 0; $i < count($bytes['offset1']); $i++) {
61-
// Starting position (in bytes) of the first part of the PDF that will be included in the validation.
6258
$offset1 = (int)$bytes['offset1'][$i];
63-
// Length (in bytes) of the first part.
6459
$length1 = (int)$bytes['length1'][$i];
65-
// Starting position (in bytes) of the second part, immediately after the signature.
6660
$offset2 = (int)$bytes['offset2'][$i];
6761

6862
$signatureStart = $offset1 + $length1 + 1;
@@ -85,59 +79,94 @@ private function getSignatures($resource): iterable {
8579
*/
8680
#[\Override]
8781
public function getCertificateChain($resource): array {
88-
$signerCounter = 0;
8982
$certificates = [];
83+
$signerCounter = 0;
84+
9085
foreach ($this->getSignatures($resource) as $signature) {
91-
// The signature could be invalid
92-
$fromFallback = $this->popplerUtilsPdfSignFallback($resource, $signerCounter);
93-
if ($fromFallback) {
94-
$certificates[$signerCounter] = $fromFallback;
95-
}
96-
if (!$signature) {
97-
$certificates[$signerCounter]['chain'][0]['signature_validation'] = $this->getReadableSigState('Digest Mismatch.');
98-
$signerCounter++;
99-
continue;
86+
$certificates[$signerCounter] = $this->processSignature($resource, $signature, $signerCounter);
87+
$signerCounter++;
88+
}
89+
return $certificates;
90+
}
91+
92+
private function processSignature($resource, ?string $signature, int $signerCounter): array {
93+
$fromFallback = $this->popplerUtilsPdfSignFallback($resource, $signerCounter);
94+
$result = $fromFallback ?: [];
95+
96+
if (!$signature) {
97+
$result['chain'][0]['signature_validation'] = $this->getReadableSigState('Digest Mismatch.');
98+
return $result;
99+
}
100+
101+
$decoded = ASN1::decodeBER($signature);
102+
$result = $this->extractTimestampData($decoded, $result);
103+
104+
$chain = $this->extractCertificateChain($signature);
105+
if (!empty($chain)) {
106+
$result['chain'] = $this->orderCertificates($chain);
107+
$this->enrichLeafWithPopplerData($result, $fromFallback);
108+
}
109+
return $result;
110+
}
111+
112+
private function extractTimestampData(array $decoded, array $result): array {
113+
$tsa = new TSA();
114+
115+
try {
116+
$timestampData = $tsa->extract($decoded);
117+
if (!empty($timestampData['genTime']) || !empty($timestampData['policy']) || !empty($timestampData['serialNumber'])) {
118+
$result['timestamp'] = $timestampData;
100119
}
120+
} catch (\Throwable $e) {
121+
}
101122

102-
$tsa = new TSA();
103-
$decoded = ASN1::decodeBER($signature);
104-
try {
105-
$timestampData = $tsa->extract($decoded);
106-
if (!empty($timestampData['genTime']) || !empty($timestampData['policy']) || !empty($timestampData['serialNumber'])) {
107-
$certificates[$signerCounter]['timestamp'] = $timestampData;
108-
}
109-
} catch (\Throwable $e) {
123+
if (!isset($result['signingTime']) || !$result['signingTime'] instanceof \DateTime) {
124+
$result['signingTime'] = $tsa->getSigninTime($decoded);
125+
}
126+
return $result;
127+
}
128+
129+
private function extractCertificateChain(string $signature): array {
130+
$pkcs7PemSignature = $this->der2pem($signature);
131+
$pemCertificates = [];
132+
133+
if (!openssl_pkcs7_read($pkcs7PemSignature, $pemCertificates)) {
134+
return [];
135+
}
136+
137+
$chain = [];
138+
foreach ($pemCertificates as $index => $pemCertificate) {
139+
$parsed = openssl_x509_parse($pemCertificate);
140+
if ($parsed) {
141+
$parsed['signature_validation'] = [
142+
'id' => 1,
143+
'label' => $this->l10n->t('Signature is valid.'),
144+
];
145+
$chain[$index] = $parsed;
110146
}
147+
}
111148

112-
if (!isset($fromFallback['signingTime']) || !$fromFallback['signingTime'] instanceof \DateTime) {
113-
$certificates[$signerCounter]['signingTime'] = $tsa->getSigninTime($decoded);
149+
return $chain;
150+
}
151+
152+
private function enrichLeafWithPopplerData(array &$result, array $fromFallback): void {
153+
if (empty($fromFallback['chain'][0]) || empty($result['chain'][0])) {
154+
return;
155+
}
156+
157+
$popplerData = $fromFallback['chain'][0];
158+
$leafCert = &$result['chain'][0];
159+
160+
$popplerOnlyFields = ['field', 'range', 'certificate_validation'];
161+
foreach ($popplerOnlyFields as $field) {
162+
if (isset($popplerData[$field])) {
163+
$leafCert[$field] = $popplerData[$field];
114164
}
165+
}
115166

116-
$pkcs7PemSignature = $this->der2pem($signature);
117-
if (openssl_pkcs7_read($pkcs7PemSignature, $pemCertificates)) {
118-
foreach ($pemCertificates as $certificateIndex => $pemCertificate) {
119-
$parsed = openssl_x509_parse($pemCertificate);
120-
foreach ($parsed as $key => $value) {
121-
if (!isset($certificates[$signerCounter]['chain'][$certificateIndex][$key])) {
122-
$certificates[$signerCounter]['chain'][$certificateIndex][$key] = $value;
123-
} elseif ($key === 'name') {
124-
$certificates[$signerCounter]['chain'][$certificateIndex][$key] = $value;
125-
} elseif ($key === 'signatureTypeSN' && $certificates[$signerCounter]['chain'][$certificateIndex][$key] !== $value) {
126-
$certificates[$signerCounter]['chain'][$certificateIndex][$key] = $value;
127-
}
128-
}
129-
if (empty($certificates[$signerCounter]['chain'][$certificateIndex]['signature_validation'])) {
130-
$certificates[$signerCounter]['chain'][$certificateIndex]['signature_validation'] = [
131-
'id' => 1,
132-
'label' => $this->l10n->t('Signature is valid.'),
133-
];
134-
}
135-
}
136-
};
137-
$certificates[$signerCounter]['chain'] = $this->orderCertificates($certificates[$signerCounter]['chain']);
138-
$signerCounter++;
167+
if (isset($popplerData['signature_validation'])) {
168+
$leafCert['signature_validation'] = $popplerData['signature_validation'];
139169
}
140-
return $certificates;
141170
}
142171

143172
private function popplerUtilsPdfSignFallback($resource, int $signerCounter): array {
@@ -167,7 +196,6 @@ private function popplerUtilsPdfSignFallback($resource, int $signerCounter): arr
167196
continue;
168197
}
169198

170-
171199
$match = [];
172200
$isSecondLevel = preg_match('/^\s+-\s(?<key>.+):\s(?<value>.*)/', $item, $match);
173201
if ($isSecondLevel) {

0 commit comments

Comments
 (0)