Merge pull request #75 from GUI/sockproc-fds

Fix sockproc possibly inheriting nginx's sockets/file descriptors

Author: GUI

Makefile

@@ -203,16 +203,7 @@ lint: test_dependencies
 	LUA_PATH="$(TEST_LUA_SHARE_DIR)/?.lua;$(TEST_LUA_SHARE_DIR)/?/init.lua;;" LUA_CPATH="$(TEST_LUA_LIB_DIR)/?.so;;" $(TEST_VENDOR_DIR)/bin/luacheck lib
 
 test: test_dependencies lint
-	sudo mkdir -p /tmp/resty-auto-ssl-test-worker-perms
-	sudo chown nobody /tmp/resty-auto-ssl-test-worker-perms
-	sudo rm -rf $(TEST_RUN_DIR)/servroot* $(TEST_LOGS_DIR)
-	mkdir -p $(TEST_LOGS_DIR)
-	PATH=$(PATH) luarocks make ./lua-resty-auto-ssl-git-1.rockspec
-	pkill sockproc || true
-	sudo pkill -U nobody sockproc || true
-	sudo env TEST_NGINX_RESTY_AUTO_SSL_DIR=/tmp/resty-auto-ssl-test-worker-perms TEST_NGINX_SERVROOT=$(TEST_RUN_DIR)/servroot-worker-perms PATH=$(PATH) PERL5LIB=$(TEST_BUILD_DIR)/lib/perl5 TEST_NGINX_ERROR_LOG=$(TEST_LOGS_DIR)/error-worker-perms.log TEST_NGINX_RESOLVER=$(TEST_NGINX_RESOLVER) prove t/worker_file_permissions.t
-	sudo pkill -U nobody sockproc || true
-	TEST_NGINX_SERVROOT=$(TEST_RUN_DIR)/servroot PATH=$(PATH) PERL5LIB=$(TEST_BUILD_DIR)/lib/perl5 TEST_NGINX_ERROR_LOG=$(TEST_LOGS_DIR)/error.log TEST_NGINX_RESOLVER=$(TEST_NGINX_RESOLVER) prove `find $(ROOT_DIR)/t -maxdepth 1 -name "*.t" -not -name "worker_file_permissions.t"`
+	PATH=$(PATH) ROOT_DIR=$(ROOT_DIR) TEST_RUN_DIR=$(TEST_RUN_DIR) TEST_BUILD_DIR=$(TEST_BUILD_DIR) TEST_LOGS_DIR=$(TEST_LOGS_DIR) TEST_LUA_SHARE_DIR=$(TEST_LUA_SHARE_DIR) TEST_LUA_LIB_DIR=$(TEST_LUA_LIB_DIR) t/run_tests
 
 grind:
 	env TEST_NGINX_USE_VALGRIND=1 TEST_NGINX_SLEEP=5 $(MAKE) test

bin/letsencrypt_hooks

@@ -47,4 +47,4 @@ function unchanged_cert {
   local DOMAIN="${1}" KEYFILE="${2}" CERTFILE="${3}" FULLCHAINFILE="${4}" CHAINFILE="${5}"
 }
 
-HANDLER=$1; shift; $HANDLER $@
+HANDLER=$1; shift; $HANDLER "$@"

bin/start_sockproc

@@ -1,7 +1,16 @@
 #!/usr/bin/env bash
 
-SOCK_FILE=/tmp/shell.sock
-PID_FILE=/tmp/auto-ssl-sockproc.pid
+set -u
+
+sock_file=/tmp/shell.sock
+pid_file=/tmp/auto-ssl-sockproc.pid
+
+# This script inherits the current directory from wherever nginx is launched,
+# which may be a private directory. To prevent the "find" command below from
+# generating "failed to restore initial working directory: Permission denied"
+# errors, change to the root directory where the nginx user will have
+# permissions.
+cd / || (echo "WARNING: start_sockproc could not set current directory to /" 1>&2)
 
 # We want to launch sockproc from inside nginx using os.execute (mainly just to
 # simplify the deployment of auto-ssl, so you don't have to worry about running
@@ -16,34 +25,39 @@ PID_FILE=/tmp/auto-ssl-sockproc.pid
 # See:
 # http://stackoverflow.com/a/4839945/222487
 # http://stackoverflow.com/a/23104923/222487
+file_descriptors_cleared="false"
 if [ -d /proc ]; then
-  SELF=$BASHPID
-  FILE_DESCRIPTORS=$(find /proc/$SELF/fd -type l -exec basename {} ';')
-  for FD in $FILE_DESCRIPTORS; do
-    if ((FD > 2)); then
-      eval "exec $FD>&-"
+  current_pid=$BASHPID
+  file_descriptors=$(find /proc/"$current_pid"/fd -type l)
+  for fd_path in $file_descriptors; do
+    # Extract just the fd number from the end of the path.
+    fd=${fd_path##*/}
+    if ((fd > 2)); then
+      eval "exec $fd>&-" && file_descriptors_cleared="true"
     fi
   done
+fi
 
-# If /proc isn't supported (eg, OS X), fallback to a simpler, naive mechanism
-# to wipe file descriptors > 2.
-else
+# If /proc isn't supported (eg, OS X), or an error occurred during the
+# proc-based "find", then fallback to a simpler, naive mechanism to wipe file
+# descriptors > 2.
+if [ "$file_descriptors_cleared" != "true" ]; then
   END=255
-  for ((FD=3; FD <= END; FD++)); do
-    eval "exec $FD>&-"
+  for ((fd=3; fd <= END; fd++)); do
+    eval "exec $fd>&-"
   done
 fi
 
 # When nginx shuts down, the sockproc daemon continues to run. So when nginx
 # subsequently starts again, we'll also make sure we restart sockproc.
-if [ -e $PID_FILE ]; then
-  PID=$(cat $PID_FILE)
-  kill $PID || true
+if [ -e "$pid_file" ]; then
+  PID=$(cat "$pid_file")
+  kill "$PID" || true
 fi
 
 # Just make sure the socket file is cleaned up.
-rm -f $SOCK_FILE || true
+rm -f "$sock_file" || true
 
 # Start the sockproc daemon.
-BIN_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
-$BIN_DIR/sockproc $SOCK_FILE $PID_FILE
+bin_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
+"$bin_dir"/sockproc "$sock_file" "$pid_file"

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

@@ -2,6 +2,7 @@ local auto_ssl = require "resty.auto-ssl"
 local lock = require "resty.lock"
 
 local function start()
+  ngx.log(ngx.NOTICE, "auto-ssl: starting sockproc")
   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

t/run_tests

@@ -0,0 +1,64 @@
+#!/usr/bin/env bash
+
+set -e -u -x
+
+# Install the library via luarocks (so we test an installed version to ensure
+# all the files are part of the install process).
+luarocks make ./lua-resty-auto-ssl-git-1.rockspec
+
+# Clean the test files before each run.
+sudo rm -rf "$TEST_RUN_DIR"/servroot* "$TEST_LOGS_DIR"/*
+mkdir -p "$TEST_LOGS_DIR"
+
+# Default environment
+export PERL5LIB=$TEST_BUILD_DIR/lib/perl5
+export TEST_NGINX_RESTY_AUTO_SSL_DIR=/tmp/resty-auto-ssl-test
+export TEST_NGINX_SERVROOT=$TEST_RUN_DIR/servroot
+export TEST_NGINX_ERROR_LOG=$TEST_LOGS_DIR/error.log
+
+cleanup_sockproc() {
+  # Cleanup sockproc in between tests, since we run some tests as root, and
+  # others as the current user, which can lead to the sockproc files being
+  # owned by different users (but in a real production environment, this
+  # shouldn't be an issue, since it will typically run as a single user).
+  sudo pkill sockproc || true
+  sudo rm -f /tmp/shell.sock /tmp/auto-ssl-sockproc.pid
+}
+
+# Environment variables to pass along when we run tests as root with nginx
+# workers (we use different directories for these tests, so the file
+# permissions don't conflict with the tests run as the current user).
+worker_env=(
+  "PATH=$PATH"
+  "PERL5LIB=$PERL5LIB"
+  "TEST_NGINX_RESTY_AUTO_SSL_DIR=/tmp/resty-auto-ssl-test-worker-perms"
+  "TEST_NGINX_SERVROOT=$TEST_RUN_DIR/servroot-worker-perms"
+  "TEST_NGINX_ERROR_LOG=$TEST_LOGS_DIR/error-worker-perms.log"
+  "TEST_NGINX_RESOLVER=${TEST_NGINX_RESOLVER:-}"
+)
+
+# Create the worker-perms test directory with the proper permissions.
+sudo mkdir -p /tmp/resty-auto-ssl-test-worker-perms
+sudo chown nobody /tmp/resty-auto-ssl-test-worker-perms
+
+# Run tests as root that test the behavior when nginx has a master process
+# running as root, and worker processes running as a less privileged user.
+cleanup_sockproc
+sudo env "${worker_env[@]}" prove t/worker_file_permissions.t
+
+# Check the behavior of how sockproc gets started (both as the current user and
+# as root with worker processes), to ensure that sockproc doesn't inherit
+# nginx's file descriptors.
+cleanup_sockproc
+sudo env "${worker_env[@]}" prove t/sockproc_file_descriptors.t
+
+# Ensure file descriptors don't get inherited when nginx is started from a
+# directory that the worker processes won't have permissions to (there was
+# previously a bug with how we started sockproc that led to the descriptors not
+# being cleared if the nginx worker user didn't have permissions to the pwd of
+# nginx's master process).
+cleanup_sockproc
+sudo env "${worker_env[@]}" sh -c "cd /root && prove $ROOT_DIR/t/sockproc_file_descriptors.t"
+
+cleanup_sockproc
+find "$ROOT_DIR"/t -maxdepth 1 -name "*.t" -not -name "worker_file_permissions.t" -not -name "sockproc_file_descriptors.t" -print0 | xargs -r0 prove

t/sockproc_file_descriptors.t

@@ -0,0 +1,172 @@
+use strict;
+use warnings;
+use Test::Nginx::Socket::Lua;
+use Cwd qw(abs_path);
+use File::Basename;
+my $root_dir = dirname(dirname(abs_path(__FILE__)));
+require "$root_dir/t/inc/setup.pl";
+AutoSsl::setup();
+
+my ($nobody_user, $nobody_passwd, $nobody_uid, $nobody_gid) = getpwnam "nobody";
+$ENV{TEST_NGINX_NOBODY_USER} = $nobody_user;
+$ENV{TEST_NGINX_NOBODY_GROUP} = getgrgid($nobody_gid);
+
+repeat_each(1);
+
+plan tests => repeat_each() * (blocks() * 7);
+
+check_accum_error_log();
+no_long_string();
+no_shuffle();
+master_on();
+workers(2);
+
+run_tests();
+
+__DATA__
+
+=== TEST 1: issues a new SSL certificate and stores it as a file
+--- main_config
+user $TEST_NGINX_NOBODY_USER $TEST_NGINX_NOBODY_GROUP;
+--- http_config
+  resolver $TEST_NGINX_RESOLVER;
+  lua_shared_dict auto_ssl 1m;
+  lua_shared_dict auto_ssl_settings 64k;
+
+  init_by_lua_block {
+    auto_ssl = (require "resty.auto-ssl").new({
+      dir = "$TEST_NGINX_RESTY_AUTO_SSL_DIR",
+      ca = "https://acme-staging.api.letsencrypt.org/directory",
+      storage_adapter = "resty.auto-ssl.storage_adapters.file",
+      allow_domain = function(domain)
+        return true
+      end,
+    })
+    auto_ssl:init()
+  }
+
+  init_worker_by_lua_block {
+    auto_ssl:init_worker()
+  }
+
+  server {
+    listen 9443 ssl;
+    ssl_certificate $TEST_NGINX_ROOT_DIR/t/certs/example_fallback.crt;
+    ssl_certificate_key $TEST_NGINX_ROOT_DIR/t/certs/example_fallback.key;
+    ssl_certificate_by_lua_block {
+      auto_ssl:ssl_certificate()
+    }
+
+    location /foo {
+      server_tokens off;
+      more_clear_headers Date;
+      echo "foo";
+    }
+  }
+
+  server {
+    listen 9080;
+    location /.well-known/acme-challenge/ {
+      content_by_lua_block {
+        auto_ssl:challenge_server()
+      }
+    }
+  }
+
+  server {
+    listen 127.0.0.1:8999;
+    location / {
+      content_by_lua_block {
+        auto_ssl:hook_server()
+      }
+    }
+  }
+--- config
+  lua_ssl_trusted_certificate $TEST_NGINX_ROOT_DIR/t/certs/letsencrypt_staging_chain.pem;
+  lua_ssl_verify_depth 5;
+  location /t {
+    content_by_lua_block {
+      local run_command = require "resty.auto-ssl.utils.run_command"
+
+      local function cleanup_sockproc()
+        run_command("pkill sockproc")
+        local _, output, err = run_command("rm -f /tmp/shell.sock /tmp/auto-ssl-sockproc.pid")
+        if err then
+          ngx.say("failed to remove sockproc files: ", err)
+          return nil, err
+        end
+      end
+
+      local function print_file_descriptors(as_user)
+        local _, output, err = run_command("lsof -n -P -l -R -c sockproc -a -d '0-255' -F pnf | sed -n '1!p' | sed -e 's/ type=STREAM//g'")
+        if err then
+          ngx.say("failed to run lsof: ", err)
+          return nil, err
+        end
+        ngx.say(output)
+      end
+
+      ngx.say("already running:")
+      print_file_descriptors("root")
+
+      cleanup_sockproc()
+      ngx.say("none running:")
+      print_file_descriptors("root")
+
+      ngx.say("current dir as current user:")
+      cleanup_sockproc()
+      os.execute("umask 0022 && " .. auto_ssl.lua_root .. "/bin/resty-auto-ssl/start_sockproc")
+      print_file_descriptors("root")
+
+      ngx.say("/tmp dir as current user:")
+      cleanup_sockproc()
+      os.execute("cd /tmp && umask 0022 && " .. auto_ssl.lua_root .. "/bin/resty-auto-ssl/start_sockproc")
+      print_file_descriptors("root")
+
+      ngx.say("the end")
+    }
+  }
+--- timeout: 30s
+--- request
+GET /t
+--- response_body
+already running:
+f0
+n/dev/null
+f1
+n/dev/null
+f2
+n/dev/null
+f3
+n/tmp/shell.sock
+
+none running:
+
+current dir as current user:
+f0
+n/dev/null
+f1
+n/dev/null
+f2
+n/dev/null
+f3
+n/tmp/shell.sock
+
+/tmp dir as current user:
+f0
+n/dev/null
+f1
+n/dev/null
+f2
+n/dev/null
+f3
+n/tmp/shell.sock
+
+the end
+--- error_log
+auto-ssl: starting sockproc
+--- no_error_log
+[warn]
+[error]
+[alert]
+[emerg]