Shift around some of the dehydrated cleanup logic.

Building on top of
#206, some tweaks:

- There was similar logic to cleanup dehydrated files both in the
  hook file (if the hook succeeded) and in the SSL provider (in case the
  dehydrated call failed). Since this means the files are deleted in
  either case (success or failure), try to simplify this by shifting the
  logic to always run in the SSL provider regardless of success status.
- For the new delete logic introduced in the SSL provider, switch it to
  use non-blocking shell execution to prevent the `rm` from potentially
  blocking (even though this should be quick).
- Since dehydrated's files are always being removed now, remove the
  logic we had in place to handle an edge-case of storage failing, but
  files remaining in dehydrated's cache, since this scenario is now
  moot.
- Update tests to account for new delete logic..

Author: GUI

lib/resty/auto-ssl/servers/hook.lua

@@ -1,5 +1,4 @@
 local parse_openssl_time = require "resty.auto-ssl.utils.parse_openssl_time"
-local shell_blocking = require "shell-games"
 
 -- This server provides an internal-only API for the dehydrated bash hook
 -- script to call. This allows for storing the tokens or certificates in the
@@ -54,14 +53,6 @@ return function(auto_ssl_instance)
       ngx.log(ngx.ERR, "auto-ssl: failed to set cert: ", err)
       return ngx.exit(ngx.HTTP_INTERNAL_SERVER_ERROR)
     end
-    -- remove the extra copy of the certificate files in dehydrated's cert directory
-    assert(string.find(params["domain"], "/") == nil)
-    assert(string.find(params["domain"], "%.%.") == nil)
-    local dir = auto_ssl_instance:get("dir") .. "/letsencrypt/certs/" .. params["domain"]
-    local _, rm_err = shell_blocking.capture_combined({ "rm", "-rf", dir })
-    if rm_err then
-      ngx.log(ngx.ERR, "auto-ssl: failed to cleanup certs: ", rm_err)
-    end
   else
     ngx.log(ngx.ERR, "auto-ssl: unknown request to hook server: ", path)
     return ngx.exit(ngx.HTTP_NOT_FOUND)

lib/resty/auto-ssl/ssl_providers/lets_encrypt.lua

@@ -1,6 +1,5 @@
 local _M = {}
 
-local shell_blocking = require "shell-games"
 local shell_execute = require "resty.auto-ssl.utils.shell_execute"
 
 function _M.issue_cert(auto_ssl_instance, domain)
@@ -38,16 +37,14 @@ function _M.issue_cert(auto_ssl_instance, domain)
     "--hook", lua_root .. "/bin/resty-auto-ssl/letsencrypt_hooks",
   })
 
+  -- Cleanup dehydrated files after running to prevent temp files from piling
+  -- up. This always runs, regardless of whether or not dehydrated succeeds (in
+  -- which case the certs should be installed in storage) or dehydrated fails
+  -- (in which case these files aren't of much additional use).
+  _M.cleanup(auto_ssl_instance, domain)
+
   if result["status"] ~= 0 then
     ngx.log(ngx.ERR, "auto-ssl: dehydrated failed: ", result["command"], " status: ", result["status"], " out: ", result["output"], " err: ", err)
-    -- remove the created files from dehydrated's cert directory
-    assert(string.find(domain, "/") == nil)
-    assert(string.find(domain, "%.%.") == nil)
-    local dir = auto_ssl_instance:get("dir") .. "/letsencrypt/certs/" .. domain
-    local _, rm_err = shell_blocking.capture_combined({ "rm", "-rf", dir })
-    if rm_err then
-      ngx.log(ngx.ERR, "auto-ssl: failed to cleanup certs: ", rm_err)
-    end
     return nil, "dehydrated failure"
   end
 
@@ -61,40 +58,6 @@ function _M.issue_cert(auto_ssl_instance, domain)
     ngx.log(ngx.ERR, "auto-ssl: error fetching certificate from storage for ", domain, ": ", get_cert_err)
   end
 
-  -- If dehydrated succeeded, but we still don't have any certs in storage, the
-  -- issue might be that dehydrated succeeded and has local certs cached, but
-  -- the initial attempt to deploy them and save them into storage failed (eg,
-  -- storage was temporarily unavailable). If this occurs, try to manually fire
-  -- the deploy_cert hook again to populate our storage with dehydrated's local
-  -- copies.
-  if not cert or not cert["fullchain_pem"] or not cert["privkey_pem"] then
-    ngx.log(ngx.WARN, "auto-ssl: dehydrated succeeded, but certs still missing from storage - trying to manually copy - domain: " .. domain)
-
-    result, err = shell_execute({
-      "env",
-      "HOOK_SECRET=" .. hook_secret,
-      "HOOK_SERVER_PORT=" .. hook_port,
-      lua_root .. "/bin/resty-auto-ssl/letsencrypt_hooks",
-      "deploy_cert",
-      domain,
-      base_dir .. "/letsencrypt/certs/" .. domain .. "/privkey.pem",
-      base_dir .. "/letsencrypt/certs/" .. domain .. "/cert.pem",
-      base_dir .. "/letsencrypt/certs/" .. domain .. "/fullchain.pem",
-      base_dir .. "/letsencrypt/certs/" .. domain .. "/chain.pem",
-      math.floor(ngx.now()),
-    })
-    if result["status"] ~= 0 then
-      ngx.log(ngx.ERR, "auto-ssl: dehydrated manual hook.sh failed: ", result["command"], " status: ", result["status"], " out: ", result["output"], " err: ", err)
-      return nil, "dehydrated failure"
-    end
-
-    -- Try fetching again.
-    cert, get_cert_err = storage:get_cert(domain)
-    if get_cert_err then
-      ngx.log(ngx.ERR, "auto-ssl: error fetching certificate from storage for ", domain, ": ", get_cert_err)
-    end
-  end
-
   -- Return error if things are still unexpectedly missing.
   if not cert or not cert["fullchain_pem"] or not cert["privkey_pem"] then
     return nil, "dehydrated succeeded, but no certs present"
@@ -103,4 +66,15 @@ function _M.issue_cert(auto_ssl_instance, domain)
   return cert
 end
 
+function _M.cleanup(auto_ssl_instance, domain)
+  assert(string.find(domain, "/") == nil)
+  assert(string.find(domain, "%.%.") == nil)
+
+  local dir = auto_ssl_instance:get("dir") .. "/letsencrypt/certs/" .. domain
+  local _, rm_err = shell_execute({ "rm", "-rf", dir })
+  if rm_err then
+    ngx.log(ngx.ERR, "auto-ssl: failed to cleanup certs: ", rm_err)
+  end
+end
+
 return _M

spec/sanity_spec.lua

@@ -197,6 +197,52 @@ describe("sanity", function()
     assert.Not.matches("[emerg]", error_log, nil, true)
   end)
 
+  it("cleans up dehydrated files on certificate registration failure", function()
+    server.start()
+
+    local ls_before_result, ls_before_err = shell_blocking.capture_combined({ "ls", "-1", server.current_test_dir .. "/auto-ssl/letsencrypt" })
+    assert.equal(nil, ls_before_err)
+    local expected_ls_before = {
+      "conf.d",
+      "config",
+      "locks",
+    }
+    if server.dehydrated_cached_accounts then
+      table.insert(expected_ls_before, "accounts")
+    end
+    table.sort(expected_ls_before)
+    assert.same(expected_ls_before, pl_utils.split(ls_before_result["output"]))
+
+    local httpc = http.new()
+    local _, connect_err = httpc:connect("127.0.0.1", 9443)
+    assert.equal(nil, connect_err)
+
+    local _, ssl_err = httpc:ssl_handshake(nil, "unresolvable-sdjfklsdjf.example", true)
+    assert.equal("18: self signed certificate", ssl_err)
+
+    local error_log = server.read_error_log()
+    assert.matches("auto-ssl: issuing new certificate for unresolvable-sdjfklsdjf.example", error_log, nil, true)
+    assert.matches("auto-ssl: dehydrated failed", error_log, nil, true)
+    assert.matches("auto-ssl: could not get certificate for unresolvable-sdjfklsdjf.example", error_log, nil, true)
+    assert.Not.matches("[alert]", error_log, nil, true)
+    assert.Not.matches("[emerg]", error_log, nil, true)
+
+    local ls_result, ls_err = shell_blocking.capture_combined({ "ls", "-1", server.current_test_dir .. "/auto-ssl/letsencrypt" })
+    assert.equal(nil, ls_err)
+    assert.same({
+      "accounts",
+      "certs",
+      "chains",
+      "conf.d",
+      "config",
+      "locks",
+    }, pl_utils.split(ls_result["output"]))
+
+    local ls_certs_result, ls_certs_err = shell_blocking.capture_combined({ "ls", "-1", server.current_test_dir .. "/auto-ssl/letsencrypt/certs" })
+    assert.equal(nil, ls_certs_err)
+    assert.same({}, pl_utils.split(ls_certs_result["output"]))
+  end)
+
   it("allows for custom logic to control domain name to handle lack of SNI support", function()
     server.start({
       auto_ssl_pre_new = [[
@@ -378,7 +424,7 @@ describe("sanity", function()
     assert.Not.matches("[emerg]", error_log, nil, true)
   end)
 
-  it("retains dehydrated temporary files if cert deployment fails", function()
+  it("deletes dehydrated temporary files if cert deployment fails", function()
     server.start()
 
     -- Create a directory where the storage file would normally belong so
@@ -429,9 +475,7 @@ describe("sanity", function()
 
     local ls_certs_result, ls_certs_err = shell_blocking.capture_combined({ "ls", "-1", server.current_test_dir .. "/auto-ssl/letsencrypt/certs" })
     assert.equal(nil, ls_certs_err)
-    assert.same({
-      server.ngrok_hostname,
-    }, pl_utils.split(ls_certs_result["output"]))
+    assert.same({}, pl_utils.split(ls_certs_result["output"]))
 
     assert(dir.rmtree(server.current_test_dir .. "/auto-ssl/storage/file/" .. ngx.escape_uri(server.ngrok_hostname .. ":latest")))
 
@@ -452,8 +496,9 @@ describe("sanity", function()
 
       local error_log = server.nginx_error_log_tail:read()
       assert.matches("auto-ssl: issuing new certificate for", error_log, nil, true)
-      assert.matches("Checking domain name(s) of existing cert... unchanged.", error_log, nil, true)
-      assert.matches("auto-ssl: dehydrated succeeded, but certs still missing from storage - trying to manually copy", error_log, nil, true)
+      assert.Not.matches("[error]", error_log, nil, true)
+      assert.Not.matches("[alert]", error_log, nil, true)
+      assert.Not.matches("[emerg]", error_log, nil, true)
     end
 
     local error_log = server.read_error_log()