Merge pull request #68 from luto/fix-66

Store hook secret in shared dict, don't overwrite it on reload

Author: GUI

.luacheckrc

@@ -1,5 +1,4 @@
 globals = {
-  "AUTO_SSL_HOOK_SECRET",
   "ngx",
 }
 

README.md

@@ -49,6 +49,10 @@ http {
   # hold your certificate data. 1MB of storage holds certificates for
   # approximately 100 separate domains.
   lua_shared_dict auto_ssl 1m;
+  # The "auto_ssl" shared dict is used to temporarily store various settings
+  # like the secret used by the hook server on port 8999. Do not change or
+  # omit it.
+  lua_shared_dict auto_ssl_settings 64k;
 
   # A DNS resolver must be defined for OCSP stapling to function.
   #

lib/resty/auto-ssl/init_master.lua

@@ -28,9 +28,21 @@ end
 -- secret token is an extra precaution to ensure the server is not accidentally
 -- opened up or proxied to the outside world.
 local function generate_hook_sever_secret()
+  if ngx.shared.auto_ssl_settings:get("hook_server:secret") then
+    -- if we've already got a secret token, do not overwrite it, as this causes
+    -- problems in reload-heavy envrionments.
+    -- See https://github.com/GUI/lua-resty-auto-ssl/issues/66
+    return
+  end
+
   -- Generate the secret token.
   local random = resty_random.bytes(32)
-  AUTO_SSL_HOOK_SECRET = str.to_hex(random)
+  local _, set_err, set_forcible = ngx.shared.auto_ssl_settings:set("hook_server:secret", str.to_hex(random))
+  if set_err then
+    ngx.log(ngx.ERR, "auto-ssl: failed to set shdict for hook_server:secret: ", set_err)
+  elseif set_forcible then
+    ngx.log(ngx.ERR, "auto-ssl: 'lua_shared_dict auto_ssl_settings' might be too small - consider increasing its configured size (old entries were removed while adding hook_server:secret)")
+  end
 end
 
 local function generate_config(auto_ssl_instance)
@@ -82,6 +94,10 @@ local function setup_storage(auto_ssl_instance)
 end
 
 return function(auto_ssl_instance)
+  if not ngx.shared.auto_ssl_settings then
+      ngx.log(ngx.ERR, "auto-ssl: dict auto_ssl_settings could not be found. Please add it to your configuration: `lua_shared_dict auto_ssl_settings 64k;`")
+  end
+
   check_dependencies()
   generate_hook_sever_secret()
   generate_config(auto_ssl_instance)

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

@@ -8,7 +8,7 @@ return function(auto_ssl_instance)
   local path = ngx.var.request_uri
   local params = ngx.req.get_post_args()
 
-  if ngx.var.http_x_hook_secret ~= AUTO_SSL_HOOK_SECRET then
+  if ngx.var.http_x_hook_secret ~= ngx.shared.auto_ssl_settings:get("hook_server:secret") then
     return ngx.exit(ngx.HTTP_FORBIDDEN)
   end
 

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

@@ -15,7 +15,7 @@ function _M.issue_cert(auto_ssl_instance, domain)
   assert(type(hook_port) == "number", "hook_port must be a number")
   assert(hook_port <= 65535, "hook_port must be below 65536")
 
-  local hook_secret = AUTO_SSL_HOOK_SECRET
+  local hook_secret = ngx.shared.auto_ssl_settings:get("hook_server:secret")
   assert(type(hook_secret) == "string", "hook_server:secret must be a string")
 
   local env_vars =

lib/resty/auto-ssl/utils/shell_execute.lua

@@ -6,13 +6,13 @@ return function(command)
   -- (since it's started by only a single worker in init_worker, it's possible
   -- other workers have already finished their init_worker phases before the
   -- process is actually started).
-  if not ngx.shared.auto_ssl:get("sockproc_started") then
+  if not ngx.shared.auto_ssl_settings:get("sockproc_started") then
     start_sockproc()
 
     local wait_time = 0
     local sleep_time = 0.01
     local max_time = 5
-    while not ngx.shared.auto_ssl:get("sockproc_started") do
+    while not ngx.shared.auto_ssl_settings:get("sockproc_started") do
       ngx.sleep(sleep_time)
       wait_time = wait_time + sleep_time
 

lib/resty/auto-ssl/utils/start_sockproc.lua

@@ -5,19 +5,19 @@ local function start()
   local exit_status = os.execute("umask 0022 && " .. auto_ssl.lua_root .. "/bin/resty-auto-ssl/start_sockproc")
   -- Lua 5.2+ returns boolean. Prior versions return status code.
   if exit_status == 0 or exit_status == true then
-    local _, set_err, set_forcible = ngx.shared.auto_ssl:set("sockproc_started", true)
+    local _, set_err, set_forcible = ngx.shared.auto_ssl_settings:set("sockproc_started", true)
     if set_err then
       ngx.log(ngx.ERR, "auto-ssl: failed to set shdict for sockproc_started: ", set_err)
     elseif set_forcible then
-      ngx.log(ngx.ERR, "auto-ssl: 'lua_shared_dict auto_ssl' might be too small - consider increasing its configured size (old entries were removed while adding sockproc_started)")
+      ngx.log(ngx.ERR, "auto-ssl: 'lua_shared_dict auto_ssl_settings' might be too small - consider increasing its configured size (old entries were removed while adding sockproc_started)")
     end
   else
     ngx.log(ngx.ERR, "auto-ssl: failed to start sockproc")
   end
 end
 
 return function(force)
-  if ngx.shared.auto_ssl:get("sockproc_started") and not force then
+  if ngx.shared.auto_ssl_settings:get("sockproc_started") and not force then
     return
   end
 

t/file.t

@@ -20,6 +20,7 @@ __DATA__
 --- http_config
   resolver $TEST_NGINX_RESOLVER;
   lua_shared_dict auto_ssl 1m;
+  lua_shared_dict auto_ssl_settings 1m;
 
   init_by_lua_block {
     auto_ssl = (require "lib.resty.auto-ssl").new({
@@ -144,6 +145,7 @@ auto-ssl: issuing new certificate for
 --- http_config
   resolver $TEST_NGINX_RESOLVER;
   lua_shared_dict auto_ssl 1m;
+  lua_shared_dict auto_ssl_settings 1m;
 
   init_by_lua_block {
     auto_ssl = (require "lib.resty.auto-ssl").new({

t/hook_secret.t

@@ -0,0 +1,85 @@
+use strict;
+use warnings;
+use Test::Nginx::Socket::Lua;
+require "./t/inc/setup.pl";
+AutoSsl::setup();
+
+repeat_each(1);
+
+plan tests => repeat_each() * (5 + 6);
+
+no_long_string();
+no_shuffle();
+
+run_tests();
+
+__DATA__
+
+=== TEST 1: writes a friendly error message when auto_ssl_settings dict is missing
+--- http_config
+  resolver $TEST_NGINX_RESOLVER;
+  lua_shared_dict auto_ssl 1m;
+
+  init_by_lua_block {
+    auto_ssl = (require "lib.resty.auto-ssl").new({
+      dir = "$TEST_NGINX_RESTY_AUTO_SSL_DIR",
+      ca = "https://acme-staging.api.letsencrypt.org/directory",
+    })
+    auto_ssl:init()
+  }
+--- config
+--- timeout: 30s
+--- request
+GET /
+--- must_die
+--- ignore_response
+--- error_log
+auto-ssl: dict auto_ssl_settings could not be found. Please add it to your configuration: `lua_shared_dict auto_ssl_settings 64k;`
+--- no_error_log
+[warn]
+[alert]
+[emerg]
+
+=== TEST 2: doesn't change the hook secret after reloading
+--- http_config
+  resolver $TEST_NGINX_RESOLVER;
+  lua_shared_dict auto_ssl 1m;
+  lua_shared_dict auto_ssl_settings 1m;
+
+  init_by_lua_block {
+    auto_ssl = (require "lib.resty.auto-ssl").new({
+      dir = "$TEST_NGINX_RESTY_AUTO_SSL_DIR",
+      ca = "https://acme-staging.api.letsencrypt.org/directory",
+    })
+    auto_ssl:init()
+  }
+
+  init_worker_by_lua_block {
+    auto_ssl:init_worker()
+  }
+
+--- config
+  location /t {
+    content_by_lua_block {
+      local secret1 = ngx.shared.auto_ssl_settings:get("hook_server:secret")
+      auto_ssl:init()
+      local secret2 = ngx.shared.auto_ssl_settings:get("hook_server:secret")
+
+      if secret1 == secret2 then
+        ngx.say("OK")
+      else
+        ngx.say("NOPE")
+      end
+    }
+  }
+--- timeout: 30s
+--- request
+GET /t
+--- response_body
+OK
+--- error_log
+--- no_error_log
+[warn]
+[error]
+[alert]
+[emerg]

t/memory.t

@@ -19,6 +19,7 @@ __DATA__
 --- http_config
   resolver $TEST_NGINX_RESOLVER;
   lua_shared_dict auto_ssl 1m;
+  lua_shared_dict auto_ssl_settings 1m;
 
   init_by_lua_block {
     auto_ssl = (require "lib.resty.auto-ssl").new({
@@ -127,6 +128,7 @@ auto-ssl: issuing new certificate for
 --- http_config
   resolver $TEST_NGINX_RESOLVER;
   lua_shared_dict auto_ssl 1m;
+  lua_shared_dict auto_ssl_settings 1m;
 
   init_by_lua_block {
     auto_ssl = (require "lib.resty.auto-ssl").new({