-
Notifications
You must be signed in to change notification settings - Fork 1
Add checkers to validate each certificate state #28
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
base: main
Are you sure you want to change the base?
Conversation
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.
Pull request overview
This PR introduces a checker interface abstraction to handle the differing behaviors of three certificate types (valid, revoked, expired) in the test-certs-site. The checker interface defines three key methods: determining if a certificate should be revoked, checking when a certificate is ready to use, and determining when to renew it.
Changes:
- Introduced the
checkerinterface with three implementations:valid,revoked, andexpired - Implemented certificate lifecycle logic including ARI-based renewal for valid certs, CRL checking for revoked certs, and expiry-based renewal for expired certs
- Added comprehensive unit tests for each checker implementation
- Integrated the checker implementations into acme.go with placeholder usage
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| acme/checker.go | Defines the checker interface and utility functions (halfTime, randTime, slogErr) |
| acme/check_valid.go | Implements valid certificate checker with ARI support |
| acme/check_valid_test.go | Unit tests for valid certificate checker including ARI functionality |
| acme/check_revoked.go | Implements revoked certificate checker with CRL validation |
| acme/check_revoked_test.go | Unit tests for revoked certificate checker with mock CA and CRL |
| acme/check_expired.go | Implements expired certificate checker with time-based renewal |
| acme/check_expired_test.go | Unit tests for expired certificate checker |
| acme/acme.go | Instantiates checker implementations for each certificate type |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Returning an error from checkReady indicates the cert is toast, but a CRL fetch error isn't that.
add a bit more test coverage too
Default Client has no timeout
The test-certs-site has some logic which needs to be specific per type of certificate (valid, revoked, expired).
That's abstracted by a "checker" interface, which has the three differences: If the cert should be revoked, when the cert is ready to use, and when to renew the certificate.
If the cert should be revoked is simple - true for revoked, false otherwise.
When the cert is ready differs for all three: Valid certs are ready immediately. Revoked certificates need to appear in a CRL. Expired certs need to have a NotAfter in the past.
When to renew also differs: Valid certs should use ARI if available. Revoked certs can be renewed halfway through their lifetime as we can't use ARI for them. Expired certs should be renewed periodically some time after they expire.
Testing the CRLs is a bit obnoxious as we need to set up a CA, certificates, and the CRL.
This code is instantiated in acme.go, but still isn't used - that's next, and WIP in #18