Fix/ocsp extraction error raised (#120)

* added the OCSP data to the certificates and adapted the tests * added test for the no OCSP data case * reverted the responder url to example.com * included the new certs * fixed instantiation of the CertificateAttributeError class * blaked the code * added test for the case where no ocsp data is found * decreased the debug level of get_ocsp_url_for_certificate as not having OCSP data is optional and not an execption * added more debugging info to the get_ocsp_url_for_certificate * blaked the code
EcoG-io · Sep 1, 2022 · 7bf66fe · 7bf66fe
1 parent c1758ed
commit 7bf66fe
Show file tree

Hide file tree

Showing 12 changed files with 95 additions and 42 deletions.
diff --git a/iso15118/shared/pki/configs/contractLeafCert.cnf b/iso15118/shared/pki/configs/contractLeafCert.cnf
@@ -12,4 +12,4 @@ domainComponent			= MO
 basicConstraints		= critical,CA:false
 keyUsage				= critical,digitalSignature,nonRepudiation,keyEncipherment,keyAgreement
 subjectKeyIdentifier	= hash
-
+authorityInfoAccess = OCSP;URI:https://www.example.com/, caIssuers;URI:https://www.example.com/Intermediate-CA.cer
diff --git a/iso15118/shared/pki/configs/moRootCACert.cnf b/iso15118/shared/pki/configs/moRootCACert.cnf
@@ -12,4 +12,5 @@ domainComponent			= MO
 basicConstraints		= critical,CA:true
 keyUsage				= critical,keyCertSign,cRLSign
 subjectKeyIdentifier	= hash
+authorityInfoAccess = OCSP;URI:https://www.example.com/, caIssuers;URI:https://www.example.com/Intermediate-CA.cer
 
diff --git a/iso15118/shared/pki/configs/moSubCA1Cert.cnf b/iso15118/shared/pki/configs/moSubCA1Cert.cnf
@@ -12,4 +12,5 @@ domainComponent			= MO
 basicConstraints		= critical,CA:true,pathlen:1
 keyUsage				= critical,keyCertSign,cRLSign
 subjectKeyIdentifier	= hash
+authorityInfoAccess = OCSP;URI:https://www.example.com/, caIssuers;URI:https://www.example.com/Intermediate-CA.cer
 
diff --git a/iso15118/shared/pki/configs/moSubCA2Cert.cnf b/iso15118/shared/pki/configs/moSubCA2Cert.cnf
@@ -12,4 +12,5 @@ domainComponent			= MO
 basicConstraints		= critical,CA:true,pathlen:0
 keyUsage				= critical,digitalSignature,nonRepudiation,keyCertSign,cRLSign
 subjectKeyIdentifier	= hash
+authorityInfoAccess = OCSP;URI:https://www.example.com/, caIssuers;URI:https://www.example.com/Intermediate-CA.cer
 
diff --git a/iso15118/shared/security.py b/iso15118/shared/security.py
@@ -1219,7 +1219,7 @@ def derive_certificate_hash_data(
             Only SHA256, SHA384, and SHA512 are allowed.
             (3.42 HashAlgorithmEnumType, p. 403, OCPP 2.0.1 Part 2)
     """
-    certificate = load_der_x509_certificate(certificate)
+    certificate: Certificate = load_der_x509_certificate(certificate)
     issuer_certificate = load_der_x509_certificate(issuer_certificate)
     builder = OCSPRequestBuilder().add_certificate(
         certificate, issuer_certificate, certificate.signature_hash_algorithm
@@ -1231,15 +1231,16 @@ def derive_certificate_hash_data(
     # Only SHA256, SHA384, and SHA512 are allowed in OCPP 2.0.1.
     hash_algorithm_for_ocpp = certificate.signature_hash_algorithm.name.upper()
     if hash_algorithm_for_ocpp not in {"SHA256", "SHA384", "SHA512"}:
-        raise CertAttributeError("Unknown hash algorithm")
+        raise CertAttributeError(
+            subject=certificate.subject.__str__(),
+            attr="HashAlgorithm",
+            invalid_value=hash_algorithm_for_ocpp,
+        )
 
     try:
         responder_url = get_ocsp_url_for_certificate(certificate)
-    except (ExtensionNotFound, OCSPServerNotFoundError):
-        # TODO GitHub#96: This may just result in failure down the road.
-        # Should we let this fail on these exceptions, or is there
-        # another way to try to get a responder_url?
-        responder_url = "https://www.example.com/"
+    except (ExtensionNotFound, OCSPServerNotFoundError) as e:
+        raise e
 
     # Some further details on distinguished names,
     # per https://www.ibm.com/docs/en/i/7.2?topic=concepts-distinguished-name :
@@ -1305,7 +1306,10 @@ def get_ocsp_url_for_certificate(certificate: Certificate) -> str:
             ExtensionOID.AUTHORITY_INFORMATION_ACCESS
         ).value
     except ExtensionNotFound:
-        logger.exception("Authority Information Access extension not found.")
+        logger.debug(
+            f"Authority Information Access extension not "
+            f"found for {certificate.subject.__str__()}."
+        )
         raise
 
     ocsps = [
@@ -1323,8 +1327,8 @@ def get_ocsp_url_for_certificate(certificate: Certificate) -> str:
 
 
 def all_certificates_from_chain(
-    certificate_chain: CertificateChainV2, root_cert: Optional[Certificate]
-) -> List[Certificate]:
+    certificate_chain: CertificateChainV2, root_cert: Optional[bytes]
+) -> List[bytes]:
     """Return all certificates from a certificate chain as a list.
 
     The order should be: leaf certificate, sub-CA 2, sub-CA 1, root,
@@ -1348,7 +1352,7 @@ def all_certificates_from_chain(
 
 def get_certificate_hash_data(
     certificate_chain: Optional[CertificateChainV2],
-    root_cert: Optional[Certificate],
+    root_cert: Optional[bytes],
 ) -> Optional[List[Dict[str, str]]]:
     """Return a list of hash data for a contract certificate chain.
 
@@ -1376,21 +1380,30 @@ def get_certificate_hash_data(
         return None
 
     all_certificates = all_certificates_from_chain(certificate_chain, root_cert)
-    # Each certificate is followed by its issuer, except for the root,
+    # The `all_certificates` list will have the following line-up
+    # [leaf, subca2, subca1, root]
+    # Thus, each certificate is followed by its issuer, except for the root,
     # which is self-signed.
-    certificate_and_issuer_pairs = [
-        (all_certificates[i], all_certificates[i + 1])
-        for i in range(len(all_certificates) - 1)
-    ] + [(root_cert, root_cert)]
-
-    return [
-        derive_certificate_hash_data(certificate, issuer)
-        for certificate, issuer in certificate_and_issuer_pairs
-    ]
+    hash_data: List[Dict[str, str]] = []
+    try:
+        for idx, certificate in enumerate(all_certificates):
+            if idx < len(all_certificates) - 1:
+                hash_data.append(
+                    derive_certificate_hash_data(certificate, all_certificates[idx + 1])
+                )
+            else:
+                # the last entry of the list contains the root_cert, which
+                # is a self-signed certificate
+                hash_data.append(derive_certificate_hash_data(root_cert, root_cert))
+    except (ExtensionNotFound, OCSPServerNotFoundError):
+        # if we cant extract the OCSP from one of the certificates,
+        # then there is no point of building the hash data
+        return None
+    return hash_data
 
 
 def build_pem_certificate_chain(
-    certificate_chain: Optional[CertificateChainV2], root_cert: Optional[Certificate]
+    certificate_chain: Optional[CertificateChainV2], root_cert: Optional[bytes]
 ) -> Optional[str]:
     """Return a string of certificates in PEM form concatenated together."""
     if certificate_chain is None:

diff --git a/tests/sample_certs/contractLeafCert.der b/tests/sample_certs/contractLeafCert.der
diff --git a/tests/sample_certs/load_certs.py b/tests/sample_certs/load_certs.py
@@ -41,6 +41,19 @@ def load_root_certificate() -> Certificate:
     return root_certificate
 
 
+def load_no_ocsp_root_certificate() -> Certificate:
+    """Load the sample root certificate.
+
+    Returns:
+        The sample root certificate in DER (binary) form.
+    """
+    cert_dir = get_cert_dir()
+    with open(cert_dir / "moRootCACert_no_ocsp.der", "rb") as root_file:
+        root_certificate = root_file.read()
+
+    return root_certificate
+
+
 def load_sub_ca_1_certificate() -> Certificate:
     """Load the sample sub-CA 1 certificate.
 

diff --git a/tests/sample_certs/moRootCACert.der b/tests/sample_certs/moRootCACert.der
diff --git a/tests/sample_certs/moRootCACert_no_ocsp.der b/tests/sample_certs/moRootCACert_no_ocsp.der
diff --git a/tests/sample_certs/moSubCA1Cert.der b/tests/sample_certs/moSubCA1Cert.der
diff --git a/tests/sample_certs/moSubCA2Cert.der b/tests/sample_certs/moSubCA2Cert.der
diff --git a/tests/test_security.py b/tests/test_security.py
@@ -1,4 +1,7 @@
 """Tests for the iso15118 security module."""
+import pytest
+from cryptography.x509 import ExtensionNotFound
+
 from iso15118.shared.security import (
     all_certificates_from_chain,
     certificate_to_pem_string,
@@ -8,6 +11,7 @@
 from tests.sample_certs.load_certs import (
     load_certificate_chain,
     load_contract_certificate,
+    load_no_ocsp_root_certificate,
     load_root_certificate,
     load_sub_ca_2_certificate,
 )
@@ -22,13 +26,24 @@ def test_derive_certificate_hash_data_root_certificate() -> None:
 
     assert hash_data == {
         "hash_algorithm": "SHA256",
-        "issuer_name_hash": "z4Yn1aMMr-YdbGaRcRFFT_qBHdaYSZdQd0586M-3HHE=",
-        "issuer_key_hash": "S4UASAPa4rzDIgjp2i6pLvhzBOUyh9TGIvJeE-qTLPQ=",
-        "serial_number": "11",
+        "issuer_name_hash": "nYwcUlDO5yz8nuq_z8h5WhCkhcQA5RCWds-otYxz92g=",
+        "issuer_key_hash": "sB_ugYa6EuIbiu8qiK8BueX2bB74WwFiUnPiaoRlK8k=",
+        "serial_number": "12353",
         "responder_url": "https://www.example.com/",
     }
 
 
+def test_no_hash_data_with_no_ocsp() -> None:
+    """Tests no hash data is derived without OCSP data"""
+    # An ExtensionNotFound is raised in this case,
+    # because the field "Authority Information Access"
+    # does not exist in the Certificate
+    certificate_bytes = load_no_ocsp_root_certificate()
+
+    with pytest.raises(ExtensionNotFound):
+        derive_certificate_hash_data(certificate_bytes, certificate_bytes)
+
+
 def test_derive_certificate_hash_data_contract_certificate() -> None:
     """Test that correct certificate data is extracted for a contract certificate."""
     hash_data = derive_certificate_hash_data(
@@ -38,9 +53,9 @@ def test_derive_certificate_hash_data_contract_certificate() -> None:
 
     assert hash_data == {
         "hash_algorithm": "SHA256",
-        "issuer_name_hash": "BQ5EPsVHvmz-iy5lfQkL4H2GonQAuyb79KJfy-shZTQ=",
-        "issuer_key_hash": "Xre37wLCZB7m4TflmE1DcRbucyVyN0BQTRdYR6buWgA=",
-        "serial_number": "15",
+        "issuer_name_hash": "ZycWQ1dc_5ZgLYpfcQPBFfiSwfH925lgYXu_ZmpDuWg=",
+        "issuer_key_hash": "2t1s_HMWXEZ3XCHc62k7szeq2SgB1jEOeOMizgydw0c=",
+        "serial_number": "12356",
         "responder_url": "https://www.example.com/",
     }
 
@@ -53,7 +68,7 @@ def test_certificate_to_pem_string() -> None:
     # very informative, so we just check the PEM format and a small part of the string.
     # It's unlikely that any reasonable implementation of certificate_to_pem_string
     # will violate this.
-    assert pem_string.startswith("-----BEGIN CERTIFICATE-----\nMIIB3jCCAYSg")
+    assert pem_string.startswith("-----BEGIN CERTIFICATE-----\nMIICUTCCAfig")
     assert "-----END CERTIFICATE-----" in pem_string
 
 
@@ -94,35 +109,44 @@ def test_get_certificate_hash_data():
     assert hash_data == [
         {
             "hash_algorithm": "SHA256",
-            "issuer_name_hash": "BQ5EPsVHvmz-iy5lfQkL4H2GonQAuyb79KJfy-shZTQ=",
-            "issuer_key_hash": "Xre37wLCZB7m4TflmE1DcRbucyVyN0BQTRdYR6buWgA=",
-            "serial_number": "15",
+            "issuer_name_hash": "ZycWQ1dc_5ZgLYpfcQPBFfiSwfH925lgYXu_ZmpDuWg=",
+            "issuer_key_hash": "2t1s_HMWXEZ3XCHc62k7szeq2SgB1jEOeOMizgydw0c=",
+            "serial_number": "12356",
             "responder_url": "https://www.example.com/",
         },
         {
             "hash_algorithm": "SHA256",
-            "issuer_name_hash": "btnbzKuhBDrHRQx10P_aItU5RBDQtOcxmI_i08ntkVs=",
-            "issuer_key_hash": "CyRfoEzBwIG-hQAUZeE0KZw8tCzMYSiMgDlRy3m3Cxk=",
-            "serial_number": "13",
+            "issuer_name_hash": "EmoLeXDvkq6gXNIj6788YfEhtuBM6JY-ftm44q3LZrs=",
+            "issuer_key_hash": "iwBpLgtEjxsmCqlluNckS4FJ30rAAJZgbv6bNmq6GSw=",
+            "serial_number": "12355",
             "responder_url": "https://www.example.com/",
         },
         {
             "hash_algorithm": "SHA256",
-            "issuer_name_hash": "z4Yn1aMMr-YdbGaRcRFFT_qBHdaYSZdQd0586M-3HHE=",
-            "issuer_key_hash": "S4UASAPa4rzDIgjp2i6pLvhzBOUyh9TGIvJeE-qTLPQ=",
-            "serial_number": "12",
+            "issuer_name_hash": "nYwcUlDO5yz8nuq_z8h5WhCkhcQA5RCWds-otYxz92g=",
+            "issuer_key_hash": "sB_ugYa6EuIbiu8qiK8BueX2bB74WwFiUnPiaoRlK8k=",
+            "serial_number": "12354",
             "responder_url": "https://www.example.com/",
         },
         {
             "hash_algorithm": "SHA256",
-            "issuer_name_hash": "z4Yn1aMMr-YdbGaRcRFFT_qBHdaYSZdQd0586M-3HHE=",
-            "issuer_key_hash": "S4UASAPa4rzDIgjp2i6pLvhzBOUyh9TGIvJeE-qTLPQ=",
-            "serial_number": "11",
+            "issuer_name_hash": "nYwcUlDO5yz8nuq_z8h5WhCkhcQA5RCWds-otYxz92g=",
+            "issuer_key_hash": "sB_ugYa6EuIbiu8qiK8BueX2bB74WwFiUnPiaoRlK8k=",
+            "serial_number": "12353",
             "responder_url": "https://www.example.com/",
         },
     ]
 
 
+def test_no_hash_data_with_no_ocsp_cert():
+    """If a cert has no OCSP data, then the hash data is not generated"""
+    cert_chain = load_certificate_chain()
+    root_cert = load_no_ocsp_root_certificate()
+    hash_data = get_certificate_hash_data(cert_chain, root_cert)
+
+    assert hash_data is None
+
+
 def test_get_certificate_hash_data_no_chain():
     """Test that get_certificate_hash_data returns None with no chain."""
     cert_chain = None