Skip to content

Delete certificates that can't be renewed #128

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
May 1, 2019
Merged

Delete certificates that can't be renewed #128

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3bcac63
Update legacy certificates in storage with expiration date
gohai Apr 18, 2018
353b65a
Give up trying to renew certificates in storage once they are expired
gohai Apr 26, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 46 additions & 8 deletions lib/resty/auto-ssl/jobs/renewal.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,8 +79,43 @@ local function renew_check_cert(auto_ssl_instance, storage, domain)
cert = {}
end

-- Attempt to retrieve expiry date from storage. If it is not found try renewal.
-- If expiry date is found, we attempt renewal if it's within 30 days.
if not cert["fullchain_pem"] then
ngx.log(ngx.ERR, "auto-ssl: attempting to renew certificate for domain without certificates in storage: ", domain)
renew_check_cert_unlock(domain, storage, local_lock, distributed_lock_value)
return
end

-- 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 .. ":")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if err then
ngx.log(ngx.ERR, "auto-ssl: error fetching certificate backups from storage for ", domain, ": ", err)
else
local most_recent = 0
for _, key in ipairs(keys) do
local timestamp = string.sub(key, string.find(key, ":") + 1)
timestamp = tonumber(timestamp)
if timestamp and most_recent < timestamp then
most_recent = timestamp
end
end
if most_recent ~= 0 then
-- Backup timestamp used milliseconds, convert to seconds
cert["expiry"] = math.floor(most_recent/1000) + (90 * 24 * 60 * 60)
-- Update stored certificate to include expiry information
ngx.log(ngx.NOTICE, "auto-ssl: setting expiration date of ", domain, " to ", cert["expiry"])
local _, err = storage:set_cert(domain, cert["fullchain_pem"], cert["privkey_pem"], cert["cert_pem"], cert["expiry"])
if err then
ngx.log(ngx.ERR, "auto-ssl: failed to update cert: ", err)
end
else
ngx.log(ngx.ERR, "auto-ssl: no certificate backups in storage for ", domain, ", unable to set expiration date")
end
end
end

-- If expiry date is known, attempt renewal if it's within 30 days.
if cert["expiry"] then
local now = ngx.now()
if now + (30 * 24 * 60 * 60) < cert["expiry"] then
Expand All @@ -90,12 +125,6 @@ local function renew_check_cert(auto_ssl_instance, storage, domain)
end
end

if not cert["fullchain_pem"] then
ngx.log(ngx.ERR, "auto-ssl: attempting to renew certificate for domain without certificates in storage: ", domain)
renew_check_cert_unlock(domain, storage, local_lock, distributed_lock_value)
return
end

-- We didn't previously store the cert.pem (since it can be derived from the
-- fullchain.pem). So for backwards compatibility, set the cert.pem value to
-- the fullchain.pem value, since that should work for our date checking
Expand Down Expand Up @@ -129,6 +158,15 @@ local function renew_check_cert(auto_ssl_instance, storage, domain)
local _, issue_err = ssl_provider.issue_cert(auto_ssl_instance, domain)
if issue_err then
ngx.log(ngx.ERR, "auto-ssl: issuing renewal certificate failed: ", err)
-- Give up on renewing this certificate if we didn't manage to renew
-- it before the expiration date
local now = ngx.now()
if cert["expiry"] then
if cert["expiry"] < now then
ngx.log(ngx.NOTICE, "auto-ssl: existing certificate is expired, deleting: ", domain)
storage:delete_cert(domain)
end
end
end

renew_check_cert_unlock(domain, storage, local_lock, distributed_lock_value)
Expand Down
4 changes: 4 additions & 0 deletions lib/resty/auto-ssl/storage.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

end

function _M.all_cert_domains(self)
local keys, err = self.adapter:keys_with_suffix(":latest")
if err then
Expand Down
17 changes: 17 additions & 0 deletions lib/resty/auto-ssl/storage_adapters/file.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,23 @@ function _M.delete(self, key)
end
end

function _M.keys_with_prefix(self, prefix)
local base_dir = self.options["dir"]
local _, output, err = run_command("find " .. base_dir .. "/storage/file -name '" .. ngx.escape_uri(prefix) .. "*'")
if err then
return nil, err
end

local keys = {}
for path in string.gmatch(output, "[^\r\n]+") do
local filename = ngx.re.sub(path, ".*/", "")
local key = ngx.unescape_uri(filename)
table.insert(keys, key)
end

return keys
end

function _M.keys_with_suffix(self, suffix)
local base_dir = self.options["dir"]
local _, output, err = run_command("find " .. base_dir .. "/storage/file -name '*" .. ngx.escape_uri(suffix) .. "'")
Expand Down
24 changes: 24 additions & 0 deletions lib/resty/auto-ssl/storage_adapters/redis.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,30 @@ function _M.delete(self, key)
return connection:del(prefixed_key(self, key))
end

function _M.keys_with_prefix(self, prefix)
local connection, connection_err = self:get_connection()
if connection_err then
return false, connection_err
end

local keys, err = connection:keys(prefixed_key(self, prefix .. "*"))

if keys and self.options["prefix"] then
local unprefixed_keys = {}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

-- First character past the prefix and a colon
local offset = string.len(self.options["prefix"]) + 2

for _, key in ipairs(keys) do
local unprefixed = string.sub(key, offset)
table.insert(unprefixed_keys, unprefixed)
end

keys = unprefixed_keys
end

return keys, err
end

function _M.keys_with_suffix(self, suffix)
local connection, connection_err = self:get_connection()
if connection_err then
Expand Down