Delete certificates that can't be renewed #128
Conversation
gohai
force-pushed
the
delete-expired-domains
branch
from
3189c56 to
418b4ae
Compare
|
Thanks for the PR! I probably won’t have a chance to take a look at this for another week or two, so apologies for the delay. But I am interested in getting this functionality merged in, so thanks again. I’ll try to take a look at this soon. |
gohai
force-pushed
the
delete-expired-domains
branch
2 times, most recently
from
547ffbf to
f5e8890
Compare
af15771 and 3b0038d added the field "expiry" to the certificate, which speeds up the renewal check considerably for certificates that have this information set. For existing ones that don't, use the most recent backup timestamp to establish the expiration date, and add this information to storage, so that future renewal jobs are faster for those as well. This is in preparation for the following patch in the series, which uses the "expiry" field to purge certificates where renewal has failed repeatedly.
There are various legitimate reasons why a certificate can't be renewed, such as the domain holder pointing it elsewhere. We don't want to retry those forever.
gohai
force-pushed
the
delete-expired-domains
branch
from
f5e8890 to
353b65a
Compare
| -- If we don't have an expiry date yet, try to update the stored cert | ||
| -- with a date based on backup timestamps | ||
| if not cert["expiry"] then | ||
| local keys, err = storage.adapter:keys_with_prefix(domain .. ":") |
There was a problem hiding this comment.
Rather basing the missing expiry times on the storage key's timestamps, what about instead using run_command to run the same shell commands we use to fetch this directly from the certificate data using openssl? https://github.com/GUI/lua-resty-auto-ssl/blob/v0.12.0/bin/letsencrypt_hooks#L37 It would require writing the cert data to a temporary file on disk, but since this should only need to happen to upgrade pre-v0.12 data once, this overhead doesn't seem like it would be a big deal.
It seems like this approach would ensure better consistency, since we'd be sure we get the accurate expiration time directly out of the certificate (in case the timestamp it was stored at was different for whatever reason). It would also work, even if we made changes to potentially get rid of the old timestamped certs from storage, as discussed in #124
There was a problem hiding this comment.
This idea made sense to me. I went for the timestamps, since it seemed more obtainable (I am still only dabbling in Lua).
Note that getting rid of newly-created backups down the line would not cause an issue here, as all certificates created after af15771 already have the expiry information set.
| @@ -65,6 +65,10 @@ function _M.set_cert(self, domain, fullchain_pem, privkey_pem, cert_pem, expiry) | |||
| return self.adapter:set(domain .. ":latest", string) | |||
| end | |||
|
|
|||
| function _M.delete_cert(self, domain) | |||
| return self.adapter:delete(domain .. ":latest") | |||
There was a problem hiding this comment.
Assuming we keep the ability to retain some old certificates (as discussed in #124), should this delete_cert also delete all of the timestamped older copies for the domain?
While deleting all the versions might somewhat defeat the purpose of retaining those backups, I'm not sure we'd have a very clean way to remove the old timestamped versions once the latest version is gone (since it won't be part of the renewal process), so I'm maybe inclined to delete all of them if this gets triggered. But I could really go either way on this.
| local keys, err = connection:keys(prefixed_key(self, prefix .. "*")) | ||
|
|
||
| if keys and self.options["prefix"] then | ||
| local unprefixed_keys = {} |
There was a problem hiding this comment.
If we change the expiry logic to fetch the date from the certificate using openssl, then I'm not sure we'll need to keep this function (although it could actually be useful for #124 (comment)). In any case, if we do keep this, then I'd just recommend shifting this duplicate logic to generate the unprefixed_keys outside of this method so it can more easily be shared between keys_with_suffix and keys_with_prefix.
|
@GUI Thanks for taking time to review this PR! I'll reply in-line. |
|
Any progress on this. |
|
Hi all. I'm also curious about the status of this PR. It would be very useful for us. I unfortunately don't think that I have the knowledge to contribute here. I am very thankful for everyone's work. |
Change the initial implementation in #128 to extract the expiration dates from the certificate files themselves, rather than from the storage timestamps (particularly needed now that we're no longer storing timestamped records in the storage). Also add test coverage to test how missing expiry dates and expirations removals are handled.
|
Thanks for merging @GUI! |
|
Well, very belated, but this is finally part of a formal release in v0.13.0. Many thanks! |
This is a second attempt after PR #125. Still testing. CC @brianlund