-
Notifications
You must be signed in to change notification settings - Fork 421
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
Conversation
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 pyca#1168.
ret = _lib.ASN1_TIME_new() | ||
_openssl_assert(ret != _ffi.NULL) | ||
ret = _ffi.gc(ret, _lib.ASN1_TIME_free) | ||
_set_asn1_time(ret, when) |
There was a problem hiding this comment.
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. :-)
ret = _lib.ASN1_TIME_new() | ||
_openssl_assert(ret != _ffi.NULL) | ||
ret = _ffi.gc(ret, _lib.ASN1_TIME_free) | ||
_set_asn1_time(ret, when) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
src/OpenSSL/crypto.py
Outdated
|
||
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 bit returns a new ASN1_TIME object. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/bit/but/
Co-authored-by: Alex Gaynor <alex.gaynor@gmail.com>
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.