-
Notifications
You must be signed in to change notification settings - Fork 45
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
Add timezone (utc) info into the cert not_valid_after field #701
Conversation
… field. Signed-off-by: Christian S. Perone <[email protected]>
sigstore/sign.py
Outdated
if ( | ||
datetime.now(timezone.utc).timestamp() | ||
> self.__cached_signing_certificate.cert.not_valid_after.timestamp() | ||
): | ||
not_valid_after = self.__cached_signing_certificate.cert.not_valid_after | ||
not_valid_after_timestamp = not_valid_after.replace( | ||
tzinfo=timezone.utc | ||
).timestamp() | ||
|
||
if datetime.now(timezone.utc).timestamp() > not_valid_after_timestamp: |
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.
Hmm, I think we can simplify this even further by just comparing the datetime
objects directly: cryptography
guarantees that Certificate
times are always UTC, so this should work:
if datetime.now(timezone.utc) > self.__cached_signing_certificate.cert.not_valid_after:
...
That way we don't have to do any timestamp conversions at all 🙂
Could you confirm that this works for you, @perone?
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.
(NB: This might fail because of an aware vs. non-aware comparison, in which case we can use datetime.utcnow()
instead of datetime.now(timezone.utc)
).
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.
Thanks for reviewing @woodruffw. I would avoid the utcnow()
[1] [2]. I will test it to check how it behaves.
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.
Indeed it triggers an exception:
TypeError: can't compare offset-naive and offset-aware datetimes
Another alternative is to go with the option of having both tz-aware datetimes but without timestamp conversion:
not_valid_after = self.__cached_signing_certificate.cert.not_valid_after
not_valid_after_tzutc = not_valid_after.replace(
tzinfo=timezone.utc
)
if datetime.now(timezone.utc) > not_valid_after_tzutc:
raise ExpiredCertificate
What do you think ?
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.
That works for me -- frankly I'd also be okay with utcnow
despite the footguns, since equality isn't defined between the two kinds of datetimes (the TypeError
you noticed) and it's what cryptography
uses internally (and this object never gets exposed anywhere).
That being said, I'm fine with either, so whichever you prefer to do!
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.
(I take back what I said about utcnow
being acceptable -- Python is marking it deprecated starting with 3.12, so we shouldn't use it at all. My bad!)
Signed-off-by: Christian S. Perone <[email protected]>
743e1d3
to
b9ba028
Compare
PTAL @woodruffw |
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.
LGTM, thank you @perone!
/gcbrun |
Thanks again @perone, especially for testing our RCs! |
Thanks for the quick reviews @woodruffw ! |
Summary
This commit fixes the issue described in #700 where the certificate
not_valid_after
datetime is not tz-aware, making the call totimestamp()
to assume local time, triggering theExpiredCertificate
exception all the time.Resolves: #700
Release Note