-
-
Notifications
You must be signed in to change notification settings - Fork 10.9k
Simplify x509 time checking #28987
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
Simplify x509 time checking #28987
Conversation
1a37822 to
e5a0687
Compare
a77b1c4 to
bd81a6e
Compare
Sashan
left a comment
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.
looks good to me as far as I can tell.
nhorman
left a comment
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.
see comment above, building for 32 bit platforms (specifically with a 32 bit time_t), the x509_test_internal tests still fail.
|
24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually. |
I think this should now work on those, I was still using ASN1_TIME_adj() in the internal test which is time_t dependent. |
|
sorry, no dice. building at commit 64af80f for linux-x86: |
7d7a422 to
e92b08e
Compare
Bah, I'm an idiot, I was using X509_VERIFY_PARAM_set_time(...) to set the test verification time in the test, Now fixed. And rebased because this was becoming unwieldy. |
This changes x509 verification to use int64 values of epoch seconds internally instead of time_t. While time values from a system will still come from/to a platform dependant time_t which could be range constrained, we can simplify this to convert the certificate time to a posix time and then just do a normal comparison of the int64_t values. This removes the need to do further computation to compare values which potentially do not cover the range of certificate times, and makes the internal functions a bit more readable. This also modifies the tests to ensure the full range of times are tested, without depending on time_t, and adds tests for checking CRL expiry, which were lacking before.
e92b08e to
d355f59
Compare
nhorman
left a comment
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.
works on x86 now, thanks!
|
This pull request is ready to merge |
|
Merged to the master branch. Thank you. |
This changes x509 verification to use int64 values of epoch seconds internally instead of time_t. While time values from a system will still come from/to a platform dependant time_t which could be range constrained, we can simplify this to convert the certificate time to a posix time and then just do a normal comparison of the int64_t values. This removes the need to do further computation to compare values which potentially do not cover the range of certificate times, and makes the internal functions a bit more readable. This also modifies the tests to ensure the full range of times are tested, without depending on time_t, and adds tests for checking CRL expiry, which were lacking before. Reviewed-by: Neil Horman <nhorman@openssl.org> Reviewed-by: Paul Dale <paul.dale@oracle.com> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #28987)
This changes x509 verification to use int64 values of epoch seconds internally instead of time_t. While time values from a system will still come from/to a platform dependant time_t which could be range constrained, we can simplify this to convert the certificate time to a posix time and then just do a normal comparison of the int64_t values. This removes the need to do further computation to compare values which potentially do not cover the range of certificate times, and makes the internal functions a bit more readable.
This also modifies the tests to ensure the full range of times are tested, without depending on time_t, and adds tests for checking CRL expiry, which were lacking before.
Fixes: 1694
Fixes: #29021
Checklist