Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix CRL nextUpdate handling. #1169

Merged
merged 3 commits into from Dec 16, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
42 changes: 33 additions & 9 deletions src/OpenSSL/crypto.py
Expand Up @@ -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)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We almost could delete _set_asn1_time and inline it in here. But the problem is the setters for X509 aren't bound. I've left it alone for now, but that could be a small bit of cleanup if you want to do the whole multi-sided change. :-)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't appear to have any callers left, should it just be inlinined here? I think there's a bunch of checks in it that are now unreachable

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ignore me, there's still 1 caller

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, see comment above. :-) We could get rid of it if we did a multi-project thing and bound some more APIs.

return ret


def _get_asn1_time(timestamp: Any) -> Optional[bytes]:
"""
Retrieve the time value of an ASN1 time object.
Expand Down Expand Up @@ -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]:
"""
Expand Down Expand Up @@ -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.
Expand All @@ -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:
"""
Expand All @@ -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:
"""
Expand Down
4 changes: 3 additions & 1 deletion tests/test_crypto.py
Expand Up @@ -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

Expand Down