Fix CRL nextUpdate handling. (#1169)

* Fix CRL nextUpdate handling.

When setting the nextUpdate field of a CRL, this code grabbed the
nextUpdate ASN1_TIME field from the CRL and set its time. But nextUpdate
is optional in a CRL so that field is usually NULL. But OpenSSL's
ASN1_TIME_set_string succeeds when the destination argument is NULL, so
it was silently a no-op.

Given that, the call in a test to set the nextUpdate field suddenly
starts working and sets the time to 2018, thus causing the CRL to be
considered expired and breaking the test. So this change also changes
the expiry year far into the future.

Additionally, the other CRL and Revoked setters violate const in the
API.

Fixes #1168.

* Replace self-check with an assert for coverage

* Update src/OpenSSL/crypto.py

Co-authored-by: Alex Gaynor <[email protected]>

Co-authored-by: Alex Gaynor <[email protected]>

Author: davidben

src/OpenSSL/crypto.py

@@ -168,12 +168,34 @@ def _set_asn1_time(boundary: Any, when: bytes) -> None:
     """
     if not isinstance(when, bytes):
         raise TypeError("when must be a byte string")
+    # ASN1_TIME_set_string validates the string without writing anything
+    # when the destination is NULL.
+    _openssl_assert(boundary != _ffi.NULL)
 
     set_result = _lib.ASN1_TIME_set_string(boundary, when)
     if set_result == 0:
         raise ValueError("Invalid string")
 
 
+def _new_asn1_time(when: bytes) -> Any:
+    """
+    Behaves like _set_asn1_time but returns a new ASN1_TIME object.
+
+    @param when: A string representation of the desired time value.
+
+    @raise TypeError: If C{when} is not a L{bytes} string.
+    @raise ValueError: If C{when} does not represent a time in the required
+        format.
+    @raise RuntimeError: If the time value cannot be set for some other
+        (unspecified) reason.
+    """
+    ret = _lib.ASN1_TIME_new()
+    _openssl_assert(ret != _ffi.NULL)
+    ret = _ffi.gc(ret, _lib.ASN1_TIME_free)
+    _set_asn1_time(ret, when)
+    return ret
+
+
 def _get_asn1_time(timestamp: Any) -> Optional[bytes]:
     """
     Retrieve the time value of an ASN1 time object.
@@ -2283,8 +2305,11 @@ def set_rev_date(self, when: bytes) -> None:
             as ASN.1 TIME.
         :return: ``None``
         """
-        dt = _lib.X509_REVOKED_get0_revocationDate(self._revoked)
-        return _set_asn1_time(dt, when)
+        revocationDate = _new_asn1_time(when)
+        ret = _lib.X509_REVOKED_set_revocationDate(
+            self._revoked, revocationDate
+        )
+        _openssl_assert(ret == 1)
 
     def get_rev_date(self) -> Optional[bytes]:
         """
@@ -2406,11 +2431,6 @@ def set_version(self, version: int) -> None:
         """
         _openssl_assert(_lib.X509_CRL_set_version(self._crl, version) != 0)
 
-    def _set_boundary_time(
-        self, which: Callable[..., Any], when: bytes
-    ) -> None:
-        return _set_asn1_time(which(self._crl), when)
-
     def set_lastUpdate(self, when: bytes) -> None:
         """
         Set when the CRL was last updated.
@@ -2424,7 +2444,9 @@ def set_lastUpdate(self, when: bytes) -> None:
         :param bytes when: A timestamp string.
         :return: ``None``
         """
-        return self._set_boundary_time(_lib.X509_CRL_get0_lastUpdate, when)
+        lastUpdate = _new_asn1_time(when)
+        ret = _lib.X509_CRL_set1_lastUpdate(self._crl, lastUpdate)
+        _openssl_assert(ret == 1)
 
     def set_nextUpdate(self, when: bytes) -> None:
         """
@@ -2439,7 +2461,9 @@ def set_nextUpdate(self, when: bytes) -> None:
         :param bytes when: A timestamp string.
         :return: ``None``
         """
-        return self._set_boundary_time(_lib.X509_CRL_get0_nextUpdate, when)
+        nextUpdate = _new_asn1_time(when)
+        ret = _lib.X509_CRL_set1_nextUpdate(self._crl, nextUpdate)
+        _openssl_assert(ret == 1)
 
     def sign(self, issuer_cert: X509, issuer_key: PKey, digest: bytes) -> None:
         """

tests/test_crypto.py

@@ -3850,7 +3850,9 @@ def _make_test_crl(self, issuer_cert, issuer_key, certs=()):
             crl.add_revoked(revoked)
         crl.set_version(1)
         crl.set_lastUpdate(b"20140601000000Z")
-        crl.set_nextUpdate(b"20180601000000Z")
+        # The year 5000 is far into the future so that this CRL isn't
+        # considered to have expired.
+        crl.set_nextUpdate(b"50000601000000Z")
         crl.sign(issuer_cert, issuer_key, digest=b"sha512")
         return crl