bug #6 OCSPValidator improvements (sstok)

This PR was merged into the 1.0-dev branch. Discussion ---------- | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Fixed tickets | | License | MIT - Inform which issuer is missing for a CA - Test with an actual revoked life certificate (from https://digicert-tls-ecc-p384-root-g5-revoked.chain-demos.digicert.com/) - Increase CA list limit to 4 and provide proper translation-id Commits ------- f0dbc03 Run OCSP revocation test with actual connection 940e8f7 Let OCSPValidator fail gracefully with missing deb f71a97a Fix wrong spelling in className
rollerworks · Dec 22, 2023 · 64f91b4 · 64f91b4
2 parents 26ec1b1 + f71a97a
commit 64f91b4
Show file tree

Hide file tree

Showing 8 changed files with 154 additions and 129 deletions.
diff --git a/Resources/translations/validators+intl-icu.en.xliff b/Resources/translations/validators+intl-icu.en.xliff
@@ -51,8 +51,8 @@
                 <target>The certificate host "{provided}" contains an invalid public-suffix wildcard pattern "{suffix_pattern}".</target>
             </trans-unit>
             <trans-unit id="13">
-                <source>Unable to resolve CA of certificate "{name}".</source>
-                <target>Unable to resolve CA of certificate "{name}".</target>
+                <source>Unable to resolve the CA of certificate "{name}", issued by {parent}.</source>
+                <target>Unable to resolve the CA of certificate "{name}", issued by {parent}.</target>
             </trans-unit>
 
             <!-- Revocation reason -->
@@ -88,6 +88,11 @@
                 <source>it is known or suspected that aspects of the AA validated in the attribute certificate have been compromised.</source>
                 <target>it is known or suspected that aspects of the AA validated in the attribute certificate have been compromised.</target>
             </trans-unit>
+
+            <trans-unit id="22">
+                <source>Too many CAs were provided. A maximum of 4 is accepted.</source>
+                <target>Too many CAs were provided. A maximum of 4 is accepted.</target>
+            </trans-unit>
         </body>
     </file>
 </xliff>
diff --git a/src/CAResolverImpl.php b/src/CAResolverImpl.php
@@ -14,7 +14,7 @@
 namespace Rollerworks\Component\X509Validator;
 
 use Rollerworks\Component\X509Validator\Violation\MissingCAExtension;
-use Rollerworks\Component\X509Validator\Violation\ToManyCAsProvided;
+use Rollerworks\Component\X509Validator\Violation\TooManyCAsProvided;
 use Rollerworks\Component\X509Validator\Violation\UnableToResolveParent;
 
 class CAResolverImpl implements CAResolver
@@ -35,8 +35,8 @@ public function resolve(string $certificate, array $caList): ?CA
     {
         // Safety check to prevent DoS attacks
         // Normally only two parents are used, more than three is exceptional
-        if (\count($caList) > 3) {
-            throw new ToManyCAsProvided();
+        if (\count($caList) > 4) {
+            throw new TooManyCAsProvided();
         }
 
         $certData = $this->extractor->extractRawData($certificate, '', true);
@@ -84,7 +84,9 @@ private function resolveCA(string $certificate, array $caList): CA
             return new CA($contents);
         }
 
-        throw new UnableToResolveParent($this->extractor->extractRawData($certificate)->commonName);
+        $x509Info = $this->extractor->extractRawData($certificate);
+
+        throw new UnableToResolveParent($x509Info->commonName, $x509Info->allFields['issuer']['commonName']);
     }
 
     private function validateCA(X509Info $data): void

diff --git a/src/OCSPValidator.php b/src/OCSPValidator.php
@@ -52,6 +52,10 @@ public function __construct(
         X509DataExtractor $dataExtractor = null,
         Ocsp $ocsp = null
     ) {
+        if ($httpClient === null && ! class_exists(HttpClient::class)) {
+            throw new \LogicException(sprintf('The "%s" class requires a "%s" instance, or that the Symfony HttpClient is available. Try running "composer require symfony/http-client".', self::class, HttpClientInterface::class));
+        }
+
         $this->extractor = $dataExtractor ?? new X509DataExtractor();
         $this->httpClient = $httpClient ?? HttpClient::create();
         $this->logger = $logger ?? new NullLogger();

diff --git a/src/Violation/ToManyCAsProvided.php → src/Violation/TooManyCAsProvided.php b/src/Violation/ToManyCAsProvided.php → src/Violation/TooManyCAsProvided.php
@@ -15,15 +15,15 @@
 
 use Rollerworks\Component\X509Validator\Violation;
 
-final class ToManyCAsProvided extends Violation
+final class TooManyCAsProvided extends Violation
 {
     public function __construct()
     {
-        parent::__construct('To many CAs were provided. A maximum of 3 is accepted.');
+        parent::__construct('Too many CAs were provided. A maximum of 4 is accepted.');
     }
 
     public function getTranslatorMsg(): string
     {
-        return 'tls.violation.to_many_ca_provided';
+        return 'Too many CAs were provided. A maximum of 4 is accepted.';
     }
 }
diff --git a/src/Violation/UnableToResolveParent.php b/src/Violation/UnableToResolveParent.php
@@ -17,22 +17,18 @@
 
 final class UnableToResolveParent extends Violation
 {
-    private readonly string $name;
-
-    public function __construct(string $name, int $code = 1)
+    public function __construct(private readonly string $name, private readonly string $issuer, int $code = 1)
     {
-        parent::__construct(sprintf('Unable to resolve the parent CA of certificate "%s".', $name), $code);
-
-        $this->name = $name;
+        parent::__construct(sprintf('Unable to resolve the parent CA of certificate "%s", issued by "%s".', $name, $issuer), $code);
     }
 
     public function getTranslatorMsg(): string
     {
-        return 'Unable to resolve the CA of certificate "{name}".';
+        return 'Unable to resolve the CA of certificate "{name}", issued by {parent}.';
     }
 
     public function getParameters(): array
     {
-        return ['name' => $this->name];
+        return ['name' => $this->name, 'parent' => $this->issuer];
     }
 }
diff --git a/src/X509DataExtractor.php b/src/X509DataExtractor.php
@@ -46,7 +46,7 @@ public function extractRawData(string $contents, string $name = '', bool $withPu
         }
 
         try {
-            $fingerprint = openssl_x509_fingerprint($x509Read, $rawData['signatureTypeSN']) ?: '';
+            $fingerprint = @openssl_x509_fingerprint($x509Read, $rawData['signatureTypeSN']) ?: '';
         } catch (\Throwable) {
             $fingerprint = '';
         }

diff --git a/tests/CAResolverImplTest.php b/tests/CAResolverImplTest.php
@@ -19,7 +19,7 @@
 use Rollerworks\Component\X509Validator\CA;
 use Rollerworks\Component\X509Validator\CAResolverImpl as CAResolver;
 use Rollerworks\Component\X509Validator\Violation\MissingCAExtension;
-use Rollerworks\Component\X509Validator\Violation\ToManyCAsProvided;
+use Rollerworks\Component\X509Validator\Violation\TooManyCAsProvided;
 use Rollerworks\Component\X509Validator\Violation\UnableToResolveParent;
 use Rollerworks\Component\X509Validator\Violation\UnprocessablePEM;
 
@@ -98,7 +98,7 @@ public function it_fails_with_to_many_cas_provided(): void
     {
         $resolver = new CAResolver();
 
-        $this->expectException(ToManyCAsProvided::class);
+        $this->expectException(TooManyCAsProvided::class);
 
         $resolver->resolve(
             <<<'CERT'
@@ -240,6 +240,35 @@ public function it_fails_with_to_many_cas_provided(): void
                     j6tJLp07kzQoH3jOlOrHvdPJbRzeXDLz
                     -----END CERTIFICATE-----
                     CERT,
+                'ca45' => <<<'CERT'
+                    -----BEGIN CERTIFICATE-----
+                    MIIElDCCA3ygAwIBAgIQAf2j627KdciIQ4tyS8+8kTANBgkqhkiG9w0BAQsFADBh
+                    MQswCQYDVQQGEwJVUzEVMBMGA1UEChMMRGlnaUNlcnQgSW5jMRkwFwYDVQQLExB3
+                    d3cuZGlnaWNlcnQuY29tMSAwHgYDVQQDExdEaWdpQ2VydCBHbG9iYWwgUm9vdCBD
+                    QTAeFw0xMzAzMDgxMjAwMDBaFw0yMzAzMDgxMjAwMDBaME0xCzAJBgNVBAYTAlVT
+                    MRUwEwYDVQQKEwxEaWdpQ2VydCBJbmMxJzAlBgNVBAMTHkRpZ2lDZXJ0IFNIQTIg
+                    U2VjdXJlIFNlcnZlciBDQTCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEB
+                    ANyuWJBNwcQwFZA1W248ghX1LFy949v/cUP6ZCWA1O4Yok3wZtAKc24RmDYXZK83
+                    nf36QYSvx6+M/hpzTc8zl5CilodTgyu5pnVILR1WN3vaMTIa16yrBvSqXUu3R0bd
+                    KpPDkC55gIDvEwRqFDu1m5K+wgdlTvza/P96rtxcflUxDOg5B6TXvi/TC2rSsd9f
+                    /ld0Uzs1gN2ujkSYs58O09rg1/RrKatEp0tYhG2SS4HD2nOLEpdIkARFdRrdNzGX
+                    kujNVA075ME/OV4uuPNcfhCOhkEAjUVmR7ChZc6gqikJTvOX6+guqw9ypzAO+sf0
+                    /RR3w6RbKFfCs/mC/bdFWJsCAwEAAaOCAVowggFWMBIGA1UdEwEB/wQIMAYBAf8C
+                    AQAwDgYDVR0PAQH/BAQDAgGGMDQGCCsGAQUFBwEBBCgwJjAkBggrBgEFBQcwAYYY
+                    aHR0cDovL29jc3AuZGlnaWNlcnQuY29tMHsGA1UdHwR0MHIwN6A1oDOGMWh0dHA6
+                    Ly9jcmwzLmRpZ2ljZXJ0LmNvbS9EaWdpQ2VydEdsb2JhbFJvb3RDQS5jcmwwN6A1
+                    oDOGMWh0dHA6Ly9jcmw0LmRpZ2ljZXJ0LmNvbS9EaWdpQ2VydEdsb2JhbFJvb3RD
+                    QS5jcmwwPQYDVR0gBDYwNDAyBgRVHSAAMCowKAYIKwYBBQUHAgEWHGh0dHBzOi8v
+                    d3d3LmRpZ2ljZXJ0LmNvbS9DUFMwHQYDVR0OBBYEFA+AYRyCMWHVLyjnjUY4tCzh
+                    xtniMB8GA1UdIwQYMBaAFAPeUDVW0Uy7ZvCj4hsbw5eyPdFVMA0GCSqGSIb3DQEB
+                    CwUAA4IBAQAjPt9L0jFCpbZ+QlwaRMxp0Wi0XUvgBCFsS+JtzLHgl4+mUwnNqipl
+                    5TlPHoOlblyYoiQm5vuh7ZPHLgLGTUq/sELfeNqzqPlt/yGFUzZgTHbO7Djc1lGA
+                    8MXW5dRNJ2Srm8c+cftIl7gzbckTB+6WohsYFfZcTEDts8Ls/3HB40f/1LkAtDdC
+                    2iDJ6m6K7hQGrn2iWZiIqBtvLfTyyRRfJs8sjX7tN8Cp1Tm5gr8ZDOo0rwAhaPit
+                    c+LJMto4JQtV05od8GiG7S5BNO98pVAdvzr508EIDObtHopYJeS4d60tbvVS3bR0
+                    j6tJLp07kzQoH3jOlOrHvdPJbRzeXDLz
+                    -----END CERTIFICATE-----
+                    CERT,
             ]
         );
     }