Repository: pyca/pyopenssl
Pull request #1252 (2023-09-20 17:17:34)

Add support for `cryptography` CRLs to `X509Store`
As a follow up to deprecating CRL APIs (https://github.com/pyca/pyopenssl/issues/1249), there is one remaining function that won't be deprecated but accepts the `CRL` type: `X509Store.add_crl(crl: CRL)`. In order to [give users an alternative](https://github.com/pyca/pyopenssl/issues/1249#issuecomment-1726834527) once `CRL` is deprecated, this PR adds support to `add_crl()` so that it also accepts `cryptography`'s `X509.CertificateRevocationList`.

This PR also adds tests for the new code path, by parametrizing existing tests that use `add_crl` so that the CRL being passed as a parameter is either PyOpenSSL's or cryptography's. For example:
```python
    @pytest.mark.parametrize(
        "create_crl",
        [
            pytest.param(
                _make_test_crl,
                id="pyOpenSSL CRL",
            ),
            pytest.param(
                _make_test_crl_cryptography,
                id="cryptography CRL",
            ),
        ],
    )
    def test_verify_with_revoked(self, create_crl):
            #...
            root_crl = create_crl(self.root_cert, self.root_key, certs=[self.intermediate_cert])
            #...
 ```

reaperhulk: This still needs a changelog entry 😄 
facutuesca: > This still needs a changelog entry 😄

@reaperhulk My bad, fixed!

Patch:
diff --git a/CHANGELOG.rst b/CHANGELOG.rst
index 8788405..55cd162 100644
--- a/CHANGELOG.rst
+++ b/CHANGELOG.rst
@@ -30,6 +30,10 @@ Deprecations:
 Changes:
 ^^^^^^^^
 
+- Changed ``OpenSSL.crypto.X509Store.add_crl`` to also accept
+  ``cryptography``'s ``X509.CertificateRevocationList`` arguments in addition
+  to the now deprecated ``OpenSSL.crypto.CRL`` arguments.
+
 23.2.0 (2023-05-30)
 -------------------
 
diff --git a/src/OpenSSL/crypto.py b/src/OpenSSL/crypto.py
index 8ee4b94..1bc88a7 100644
--- a/src/OpenSSL/crypto.py
+++ b/src/OpenSSL/crypto.py
@@ -1720,7 +1720,9 @@ class X509Store:
         res = _lib.X509_STORE_add_cert(self._store, cert._x509)
         _openssl_assert(res == 1)
 
-    def add_crl(self, crl: "CRL") -> None:
+    def add_crl(
+        self, crl: Union["_CRLInternal", x509.CertificateRevocationList]
+    ) -> None:
         """
         Add a certificate revocation list to this store.
 
@@ -1730,11 +1732,29 @@ class X509Store:
 
         .. versionadded:: 16.1.0
 
-        :param CRL crl: The certificate revocation list to add to this store.
+        :param crl: The certificate revocation list to add to this store.
+        :type crl: ``Union[CRL, cryptography.x509.CertificateRevocationList]``
         :return: ``None`` if the certificate revocation list was added
             successfully.
         """
-        _openssl_assert(_lib.X509_STORE_add_crl(self._store, crl._crl) != 0)
+        if isinstance(crl, x509.CertificateRevocationList):
+            from cryptography.hazmat.primitives.serialization import Encoding
+
+            bio = _new_mem_buf(crl.public_bytes(Encoding.DER))
+            openssl_crl = _lib.d2i_X509_CRL_bio(bio, _ffi.NULL)
+            if openssl_crl == _ffi.NULL:
+                _raise_current_error()
+
+            crl = _ffi.gc(openssl_crl, _lib.X509_CRL_free)
+        elif isinstance(crl, _CRLInternal):
+            crl = crl._crl
+        else:
+            raise TypeError(
+                "CRL must be of type OpenSSL.crypto.CRL or "
+                "cryptography.x509.CertificateRevocationList"
+            )
+
+        _openssl_assert(_lib.X509_STORE_add_crl(self._store, crl) != 0)
 
     def set_flags(self, flags: int) -> None:
         """
@@ -2404,7 +2424,7 @@ class CRL:
     @classmethod
     def from_cryptography(
         cls, crypto_crl: x509.CertificateRevocationList
-    ) -> "CRL":
+    ) -> "_CRLInternal":
         """
         Construct based on a ``cryptography`` *crypto_crl*.
 
@@ -2445,7 +2465,7 @@ class CRL:
             return tuple(results)
         return None
 
-    def add_revoked(self, revoked: Revoked) -> None:
+    def add_revoked(self, revoked: _RevokedInternal) -> None:
         """
         Add a revoked (by value not reference) to the CRL structure
 
@@ -3222,7 +3242,7 @@ utils.deprecated(
 )
 
 
-def dump_crl(type: int, crl: CRL) -> bytes:
+def dump_crl(type: int, crl: _CRLInternal) -> bytes:
     """
     Dump a certificate revocation list to a buffer.
 
@@ -3264,7 +3284,7 @@ utils.deprecated(
 )
 
 
-def load_crl(type: int, buffer: Union[str, bytes]) -> CRL:
+def load_crl(type: int, buffer: Union[str, bytes]) -> _CRLInternal:
     """
     Load Certificate Revocation List (CRL) data from a string *buffer*.
     *buffer* encoded with the type *type*.


Test patch:
diff --git a/tests/test_crypto.py b/tests/test_crypto.py
index ac3d669..6351f0d 100644
--- a/tests/test_crypto.py
+++ b/tests/test_crypto.py
@@ -13,7 +13,7 @@ from subprocess import PIPE, Popen
 import flaky
 import pytest
 from cryptography import x509
-from cryptography.hazmat.primitives import serialization
+from cryptography.hazmat.primitives import hashes, serialization
 from cryptography.hazmat.primitives.asymmetric import ec, ed448, ed25519, rsa
 
 from OpenSSL._util import ffi as _ffi
@@ -3505,7 +3505,8 @@ class TestCRL:
         buf = dump_crl(FILETYPE_PEM, crl)
         assert buf == crlData
 
-    def _make_test_crl(self, issuer_cert, issuer_key, certs=()):
+    @staticmethod
+    def _make_test_crl(issuer_cert, issuer_key, certs=()):
         """
         Create a CRL.
 
@@ -3531,7 +3532,55 @@ class TestCRL:
         crl.sign(issuer_cert, issuer_key, digest=b"sha512")
         return crl
 
-    def test_verify_with_revoked(self):
+    @staticmethod
+    def _make_test_crl_cryptography(issuer_cert, issuer_key, certs=()):
+        """
+        Create a CRL using cryptography's API.
+
+        :param list[X509] certs: A list of certificates to revoke.
+        :rtype: ``cryptography.x509.CertificateRevocationList``
+        """
+        from cryptography.x509.extensions import CRLReason, ReasonFlags
+
+        builder = x509.CertificateRevocationListBuilder()
+        builder = builder.issuer_name(
+            X509.to_cryptography(issuer_cert).subject
+        )
+        for cert in certs:
+            revoked = (
+                x509.RevokedCertificateBuilder()
+                .serial_number(cert.get_serial_number())
+                .revocation_date(datetime(2014, 6, 1, 0, 0, 0))
+                .add_extension(CRLReason(ReasonFlags.unspecified), False)
+                .build()
+            )
+            builder = builder.add_revoked_certificate(revoked)
+
+        builder = builder.last_update(datetime(2014, 6, 1, 0, 0, 0))
+        # The year 5000 is far into the future so that this CRL isn't
+        # considered to have expired.
+        builder = builder.next_update(datetime(5000, 6, 1, 0, 0, 0))
+
+        crl = builder.sign(
+            private_key=PKey.to_cryptography_key(issuer_key),
+            algorithm=hashes.SHA512(),
+        )
+        return crl
+
+    @pytest.mark.parametrize(
+        "create_crl",
+        [
+            pytest.param(
+                _make_test_crl.__func__,
+                id="pyOpenSSL CRL",
+            ),
+            pytest.param(
+                _make_test_crl_cryptography.__func__,
+                id="cryptography CRL",
+            ),
+        ],
+    )
+    def test_verify_with_revoked(self, create_crl):
         """
         `verify_certificate` raises error when an intermediate certificate is
         revoked.
@@ -3539,10 +3588,10 @@ class TestCRL:
         store = X509Store()
         store.add_cert(self.root_cert)
         store.add_cert(self.intermediate_cert)
-        root_crl = self._make_test_crl(
+        root_crl = create_crl(
             self.root_cert, self.root_key, certs=[self.intermediate_cert]
         )
-        intermediate_crl = self._make_test_crl(
+        intermediate_crl = create_crl(
             self.intermediate_cert, self.intermediate_key, certs=[]
         )
         store.add_crl(root_crl)
@@ -3555,7 +3604,20 @@ class TestCRL:
             store_ctx.verify_certificate()
         assert str(err.value) == "certificate revoked"
 
-    def test_verify_with_missing_crl(self):
+    @pytest.mark.parametrize(
+        "create_crl",
+        [
+            pytest.param(
+                _make_test_crl.__func__,
+                id="pyOpenSSL CRL",
+            ),
+            pytest.param(
+                _make_test_crl_cryptography.__func__,
+                id="cryptography CRL",
+            ),
+        ],
+    )
+    def test_verify_with_missing_crl(self, create_crl):
         """
         `verify_certificate` raises error when an intermediate certificate's
         CRL is missing.
@@ -3563,7 +3625,7 @@ class TestCRL:
         store = X509Store()
         store.add_cert(self.root_cert)
         store.add_cert(self.intermediate_cert)
-        root_crl = self._make_test_crl(
+        root_crl = create_crl(
             self.root_cert, self.root_key, certs=[self.intermediate_cert]
         )
         store.add_crl(root_crl)