Repository navigation
Fix certificate expiration during verification - #10510
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #10510 +/- ##
==========================================
+ Coverage 88.72% 88.76% +0.04%
==========================================
Files 153 153
Lines 19938 19956 +18
Branches 2331 2334 +3
==========================================
+ Hits 17689 17713 +24
+ Misses 1951 1945 -6
Partials 298 298
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
There was a problem hiding this comment.
Pull request overview
This pull request fixes a time-of-use gap in Celery’s security certificate handling by enforcing X.509 certificate expiration at verification time, not just when certificates are loaded into a store. This prevents long-running workers from continuing to accept signatures from certificates that have expired since startup.
Changes:
- Added an explicit
has_expired()check inCertificate.verify()that raisesSecurityErrorfor expired certs before signature verification. - Added a regression test that verifies successful validation pre-expiry and rejection post-expiry (via
has_expiredpatching). - Updated serialization tests to avoid failing due to historically expired bundled test certificates (those tests target serialization behavior, not validity windows).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
celery/security/certificate.py |
Enforces certificate expiry at verification time by raising SecurityError when expired. |
t/unit/security/test_certificate.py |
Adds regression coverage ensuring Certificate.verify() rejects expired certificates. |
t/unit/security/test_serialization.py |
Adjusts test setup so expired historical cert fixtures don’t break serialization-focused tests. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
@auvipy I cross-checked PR #10511 against my PR #10510. I don't think anything from #10511 should be incorporated into #10510. The certificate-expiration change in #10511 is the same core fix already implemented here: checking The other changes in #10511, specifically the I also compared the certificate regression tests. My test exercises the actual signing and verification path using a real key/signature, while the alternative test in #10511 mocks the public-key verification for the valid case. I believe the current test is appropriate for this regression because it verifies the actual behavior being fixed. So I recommend keeping #10510 focused on certificate expiration during verification, with no additional changes from #10511. |
|
Missed one thing from #10511 — adding it now: test_serialization.pyuses an autouse fixture to patch has_expired instead of the manualper-instance override in _get_s(). Pushed that update. |
Summary
Fixes #10509
A certificate could be valid when loaded into
FSCertStore, later expire while the worker remained running, and still be used to successfully verify signed messages.This PR enforces certificate expiration at verification time.
Root Cause
FSCertStorealready checkshas_expired()when certificates are loaded. However,Certificate.verify()only performed cryptographic signature verification and did not re-check the certificate's validity.This created a time-of-use gap:
Certificate.verify()is called for a new message.Changes
Certificate.verify().SecurityErrorbefore cryptographic signature verification.FSCertStoreload-time expiration check unchanged.Testing
37 passed, 1 skippedScope
This change is intentionally limited to certificate expiration during verification. It does not change signature algorithms, certificate loading, certificate parsing, or unrelated exception handling.