Merge branch 'Cargo-delete-dehydrated-cert-files-failures'

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

@@ -36,6 +36,13 @@ function _M.issue_cert(auto_ssl_instance, domain)
     "--config", base_dir .. "/letsencrypt/config",
     "--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)
     return nil, "dehydrated failure"
@@ -51,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"
@@ -93,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()