Merge pull request #3918 from certbot/save-more-hooks

Save --pre and --post hooks in renewal conf files, and run them in a sophisticated way

certbot/cli.py

@@ -18,7 +18,6 @@ import certbot
 from certbot import constants
 from certbot import crypto_util
 from certbot import errors
-from certbot import hooks
 from certbot import interfaces
 from certbot import util
 
@@ -544,9 +543,6 @@ class HelpfulArgumentParser(object):
         if parsed_args.must_staple:
             parsed_args.staple = True
 
-        if parsed_args.validate_hooks:
-            hooks.validate_hooks(parsed_args)
-
         return parsed_args
 
     def set_test_server(self, parsed_args):
@@ -1019,13 +1015,16 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False):  # pylint: dis
         " Intended primarily for renewal, where it can be used to temporarily"
         " shut down a webserver that might conflict with the standalone"
         " plugin. This will only be called if a certificate is actually to be"
-        " obtained/renewed.")
+        " obtained/renewed. When renewing several certificates that have"
+        " identical pre-hooks, only the first will be executed.")
     helpful.add(
         "renew", "--post-hook",
         help="Command to be run in a shell after attempting to obtain/renew"
         " certificates. Can be used to deploy renewed certificates, or to"
         " restart any servers that were stopped by --pre-hook. This is only"
-        " run if an attempt was made to obtain/renew a certificate.")
+        " run if an attempt was made to obtain/renew a certificate. If"
+        " multiple renewed certificates have identical post-hooks, only"
+        " one will be run.")
     helpful.add(
         "renew", "--renew-hook",
         help="Command to be run in a shell once for each successfully renewed"

certbot/hooks.py

@@ -7,6 +7,9 @@ import os
 from subprocess import Popen, PIPE
 
 from certbot import errors
+from certbot import util
+
+from certbot.plugins import util as plug_util
 
 logger = logging.getLogger(__name__)
 
@@ -17,9 +20,20 @@ def validate_hooks(config):
     validate_hook(config.renew_hook, "renew")
 
 def _prog(shell_cmd):
-    """Extract the program run by a shell command"""
-    cmd = _which(shell_cmd)
-    return os.path.basename(cmd) if cmd else None
+    """Extract the program run by a shell command.
+
+    :param str shell_cmd: command to be executed
+
+    :returns: basename of command or None if the command isn't found
+    :rtype: str or None
+
+    """
+    if not util.exe_exists(shell_cmd):
+        plug_util.path_surgery(shell_cmd)
+        if not util.exe_exists(shell_cmd):
+            return None
+    return os.path.basename(shell_cmd)
+
 
 def validate_hook(shell_cmd, hook_name):
     """Check that a command provided as a hook is plausibly executable.
@@ -36,35 +50,51 @@ def validate_hook(shell_cmd, hook_name):
 
 def pre_hook(config):
     "Run pre-hook if it's defined and hasn't been run."
-    if config.pre_hook and not pre_hook.already:
-        logger.info("Running pre-hook command: %s", config.pre_hook)
-        _run_hook(config.pre_hook)
-    pre_hook.already = True
+    cmd = config.pre_hook
+    if cmd and cmd not in pre_hook.already:
+        logger.info("Running pre-hook command: %s", cmd)
+        _run_hook(cmd)
+        pre_hook.already.add(cmd)
+    elif cmd:
+        logger.info("Pre-hook command already run, skipping: %s", cmd)
 
-pre_hook.already = False
+pre_hook.already = set()
 
-def post_hook(config, final=False):
+
+def post_hook(config):
     """Run post hook if defined.
 
     If the verb is renew, we might have more certs to renew, so we wait until
-    we're called with final=True before actually doing anything.
+    run_saved_post_hooks() is called.
     """
-    if config.post_hook:
-        if not pre_hook.already:
-            logger.info("No renewals attempted, so not running post-hook")
-            if config.verb != "renew":
-                logger.warning("Sanity failure in renewal hooks")
-            return
-        if final or config.verb != "renew":
-            logger.info("Running post-hook command: %s", config.post_hook)
-            _run_hook(config.post_hook)
+
+    cmd = config.post_hook
+    # In the "renew" case, we save these up to run at the end
+    if config.verb == "renew":
+        if cmd and cmd not in post_hook.eventually:
+            post_hook.eventually.append(cmd)
+    # certonly / run
+    elif cmd:
+        logger.info("Running post-hook command: %s", cmd)
+        _run_hook(cmd)
+
+post_hook.eventually = []
+
+def run_saved_post_hooks():
+    """Run any post hooks that were saved up in the course of the 'renew' verb"""
+    for cmd in post_hook.eventually:
+        logger.info("Running post-hook command: %s", cmd)
+        _run_hook(cmd)
+    if not post_hook.eventually:
+        logger.info("No renewals attempted, so not running post-hook")
 
 def renew_hook(config, domains, lineage_path):
-    "Run post-renewal hook if defined."
+    """Run post-renewal hook if defined."""
     if config.renew_hook:
         if not config.dry_run:
             os.environ["RENEWED_DOMAINS"] = " ".join(domains)
             os.environ["RENEWED_LINEAGE"] = lineage_path
+            logger.info("Running renew-hook command: %s", config.renew_hook)
             _run_hook(config.renew_hook)
         else:
             logger.warning("Dry run: skipping renewal hook command: %s", config.renew_hook)
@@ -93,27 +123,7 @@ def execute(shell_cmd):
         logger.error('Hook command "%s" returned error code %d',
                      shell_cmd, cmd.returncode)
     if err:
-        logger.error('Error output from %s:\n%s', _prog(shell_cmd), err)
+        base_cmd = os.path.basename(shell_cmd.split(None, 1)[0])
+        logger.error('Error output from %s:\n%s', base_cmd, err)
     return (err, out)
 
-
-def _is_exe(fpath):
-    return os.path.isfile(fpath) and os.access(fpath, os.X_OK)
-
-def _which(program):
-    """Test if program is in the path."""
-    # Borrowed from:
-    # https://stackoverflow.com/questions/377017/test-if-executable-exists-in-python
-    # XXX May need more porting to handle .exe extensions on Windows
-
-    fpath, _fname = os.path.split(program)
-    if fpath:
-        if _is_exe(program):
-            return program
-    else:
-        for path in os.environ["PATH"].split(os.pathsep):
-            exe_file = os.path.join(path, program)
-            if _is_exe(exe_file):
-                return exe_file
-
-    return None

certbot/main.py

@@ -108,7 +108,7 @@ def _auth_from_available(le_client, config, domains=None, certname=None, lineage
             if lineage is False:
                 raise errors.Error("Certificate could not be obtained")
     finally:
-        hooks.post_hook(config, final=False)
+        hooks.post_hook(config)
 
     if not config.dry_run and not config.verb == "renew":
         _report_new_cert(config, lineage.cert, lineage.fullchain)
@@ -654,7 +654,7 @@ def renew(config, unused_plugins):
     try:
         renewal.handle_renewal_request(config)
     finally:
-        hooks.post_hook(config, final=True)
+        hooks.run_saved_post_hooks()
 
 
 def setup_log_file_handler(config, logfile, fmt):
@@ -804,6 +804,20 @@ def set_displayer(config):
                                              config.force_interactive)
     zope.component.provideUtility(displayer)
 
+def _post_logging_setup(config, plugins, cli_args):
+    """Perform any setup or configuration tasks that require a logger."""
+
+    # This needs logging, but would otherwise be in HelpfulArgumentParser
+    if config.validate_hooks:
+        hooks.validate_hooks(config)
+
+    cli.possible_deprecation_warning(config)
+
+    logger.debug("certbot version: %s", certbot.__version__)
+    # do not log `config`, as it contains sensitive data (e.g. revoke --key)!
+    logger.debug("Arguments: %r", cli_args)
+    logger.debug("Discovered plugins: %r", plugins)
+
 
 def main(cli_args=sys.argv[1:]):
     """Command line argument parsing and main script execution."""
@@ -821,12 +835,7 @@ def main(cli_args=sys.argv[1:]):
     # logger ..." TODO: this should be done before plugins discovery
     setup_logging(config)
 
-    cli.possible_deprecation_warning(config)
-
-    logger.debug("certbot version: %s", certbot.__version__)
-    # do not log `config`, as it contains sensitive data (e.g. revoke --key)!
-    logger.debug("Arguments: %r", cli_args)
-    logger.debug("Discovered plugins: %r", plugins)
+    _post_logging_setup(config, plugins, cli_args)
 
     sys.excepthook = functools.partial(_handle_exception, config=config)
 

certbot/plugins/util.py

@@ -30,12 +30,12 @@ RENEWER_EXTRA_MSG = (
     " needing to stop and start your webserver.")
 
 
-def path_surgery(restart_cmd):
-    """Attempt to perform PATH surgery to find restart_cmd
+def path_surgery(cmd):
+    """Attempt to perform PATH surgery to find cmd
 
     Mitigates https://github.com/certbot/certbot/issues/1833
 
-    :param str restart_cmd: the command that is being searched for in the PATH
+    :param str cmd: the command that is being searched for in the PATH
 
     :returns: True if the operation succeeded, False otherwise
     """
@@ -49,14 +49,14 @@ def path_surgery(restart_cmd):
 
     if any(added):
         logger.debug("Can't find %s, attempting PATH mitigation by adding %s",
-                     restart_cmd, os.pathsep.join(added))
+                     cmd, os.pathsep.join(added))
         os.environ["PATH"] = path
 
-    if util.exe_exists(restart_cmd):
+    if util.exe_exists(cmd):
         return True
     else:
         expanded = " expanded" if any(added) else ""
-        logger.warning("Failed to find %s in%s PATH: %s", restart_cmd,
+        logger.warning("Failed to find %s in%s PATH: %s", cmd,
                        expanded, path)
         return False
 

certbot/renewal.py

@@ -29,7 +29,8 @@ logger = logging.getLogger(__name__)
 # the renewal configuration process loses this information.
 STR_CONFIG_ITEMS = ["config_dir", "logs_dir", "work_dir", "user_agent",
                     "server", "account", "authenticator", "installer",
-                    "standalone_supported_challenges", "renew_hook"]
+                    "standalone_supported_challenges", "renew_hook",
+                    "pre_hook", "post_hook"]
 INT_CONFIG_ITEMS = ["rsa_key_size", "tls_sni_01_port", "http01_port"]
 BOOL_CONFIG_ITEMS = ["must_staple", "allow_subset_of_names"]
 

certbot/tests/hook_test.py

@@ -27,53 +27,60 @@ class HookTest(unittest.TestCase):
         config = mock.MagicMock(pre_hook="explodinator", post_hook="", renew_hook="")
         self.assertRaises(errors.HookCommandNotFound, hooks.validate_hooks, config)
 
-    @mock.patch('certbot.hooks._is_exe')
-    def test_which(self, mock_is_exe):
-        mock_is_exe.return_value = True
-        self.assertEqual(hooks._which("/path/to/something"), "/path/to/something")
-
-        with mock.patch.dict('os.environ', {"PATH": "/floop:/fleep"}):
-            mock_is_exe.return_value = True
-            self.assertEqual(hooks._which("pingify"), "/floop/pingify")
-            mock_is_exe.return_value = False
-            self.assertEqual(hooks._which("pingify"), None)
-        self.assertEqual(hooks._which("/path/to/something"), None)
-
-    @mock.patch('certbot.hooks._which')
-    def test_prog(self, mockwhich):
-        mockwhich.return_value = "/very/very/funky"
+    @mock.patch('certbot.hooks.util.exe_exists')
+    @mock.patch('certbot.hooks.plug_util.path_surgery')
+    def test_prog(self, mock_ps, mock_exe_exists):
+        mock_exe_exists.return_value = True
         self.assertEqual(hooks._prog("funky"), "funky")
-        mockwhich.return_value = None
+        self.assertEqual(mock_ps.call_count, 0)
+        mock_exe_exists.return_value = False
         self.assertEqual(hooks._prog("funky"), None)
+        self.assertEqual(mock_ps.call_count, 1)
 
-    def _test_a_hook(self, config, hook_function, calls_expected):
+    def _test_a_hook(self, config, hook_function, calls_expected, **kwargs):
         with mock.patch('certbot.hooks.logger') as mock_logger:
             mock_logger.warning = mock.MagicMock()
             with mock.patch('certbot.hooks._run_hook') as mock_run_hook:
-                hook_function(config)
-                hook_function(config)
+                hook_function(config, **kwargs)
+                hook_function(config, **kwargs)
                 self.assertEqual(mock_run_hook.call_count, calls_expected)
             return mock_logger.warning
 
     def test_pre_hook(self):
-        hooks.pre_hook.already = False
+        hooks.pre_hook.already = set()
         config = mock.MagicMock(pre_hook="true")
         self._test_a_hook(config, hooks.pre_hook, 1)
+        self._test_a_hook(config, hooks.pre_hook, 0)
+        config = mock.MagicMock(pre_hook="more_true")
+        self._test_a_hook(config, hooks.pre_hook, 1)
+        self._test_a_hook(config, hooks.pre_hook, 0)
         config = mock.MagicMock(pre_hook="")
         self._test_a_hook(config, hooks.pre_hook, 0)
 
-    def test_post_hook(self):
-        hooks.pre_hook.already = False
-        # if pre-hook isn't called, post-hook shouldn't be
-        config = mock.MagicMock(post_hook="true", verb="splonk")
-        self._test_a_hook(config, hooks.post_hook, 0)
+    def _test_renew_post_hooks(self, expected_count):
+        with mock.patch('certbot.hooks.logger.info') as mock_info:
+            with mock.patch('certbot.hooks._run_hook') as mock_run:
+                hooks.run_saved_post_hooks()
+                self.assertEqual(mock_run.call_count, expected_count)
+                if expected_count:
+                    self.assertEqual(mock_info.call_count, expected_count)
+                else:
+                    self.assertEqual(mock_info.call_count, 1)
 
+    def test_post_hooks(self):
         config = mock.MagicMock(post_hook="true", verb="splonk")
-        self._test_a_hook(config, hooks.pre_hook, 1)
         self._test_a_hook(config, hooks.post_hook, 2)
+        self._test_renew_post_hooks(0)
 
         config = mock.MagicMock(post_hook="true", verb="renew")
         self._test_a_hook(config, hooks.post_hook, 0)
+        self._test_renew_post_hooks(1)
+        self._test_a_hook(config, hooks.post_hook, 0)
+        self._test_renew_post_hooks(1)
+
+        config = mock.MagicMock(post_hook="more_true", verb="renew")
+        self._test_a_hook(config, hooks.post_hook, 0)
+        self._test_renew_post_hooks(2)
 
     def test_renew_hook(self):
         with mock.patch.dict('os.environ', {}):

tests/boulder-integration.sh

@@ -33,20 +33,56 @@ common() {
         "$@"
 }
 
+export HOOK_TEST="/tmp/hook$$"
+CheckHooks() {
+    COMMON="wtf2.auth\nwtf2.cleanup\nrenew\nrenew"
+    EXPECTED="/tmp/expected$$"
+    if [ $(head -n1 $HOOK_TEST) = "wtf.pre" ]; then
+        echo "wtf.pre" > "$EXPECTED"
+        echo "wtf2.pre" >> "$EXPECTED"
+        echo $COMMON >> "$EXPECTED"
+        echo "wtf.post" >> "$EXPECTED"
+        echo "wtf2.post" >> "$EXPECTED"
+    else
+        echo "wtf2.pre" > "$EXPECTED"
+        echo "wtf.pre" >> "$EXPECTED"
+        echo $COMMON >> "$EXPECTED"
+        echo "wtf2.post" >> "$EXPECTED"
+        echo "wtf.post" >> "$EXPECTED"
+    fi
+
+    if cmp --quiet "$EXPECTED" "$HOOK_TEST" ; then
+        echo Hooks did not run as expected\; got
+        cat "$HOOK_TEST"
+        echo Expected
+        cat "$EXPECTED"
+    fi
+    rm "$HOOK_TEST"
+}
+
 # We start a server listening on the port for the
 # unrequested challenge to prevent regressions in #3601.
 python -m SimpleHTTPServer $http_01_port &
 python_server_pid=$!
-common --domains le1.wtf --preferred-challenges tls-sni-01 auth
+
+common --domains le1.wtf --preferred-challenges tls-sni-01 auth \
+       --pre-hook 'echo wtf.pre >> "$HOOK_TEST"' \
+       --post-hook 'echo wtf.post >> "$HOOK_TEST"'\
+       --renew-hook 'echo renew >> "$HOOK_TEST"'
 kill $python_server_pid
 python -m SimpleHTTPServer $tls_sni_01_port &
 python_server_pid=$!
-common --domains le2.wtf --preferred-challenges http-01 run
+common --domains le2.wtf --preferred-challenges http-01 run \
+       --pre-hook 'echo wtf.pre >> "$HOOK_TEST"' \
+       --post-hook 'echo wtf.post >> "$HOOK_TEST"'\
+       --renew-hook 'echo renew >> "$HOOK_TEST"'
 kill $python_server_pid
 
 common certonly -a manual -d le.wtf --rsa-key-size 4096 \
-    --manual-auth-hook ./tests/manual-http-auth.sh \
-    --manual-cleanup-hook ./tests/manual-http-cleanup.sh
+    --manual-auth-hook 'echo wtf2.auth >> "$HOOK_TEST" && ./tests/manual-http-auth.sh' \
+    --manual-cleanup-hook 'echo wtf2.cleanup >> "$HOOK_TEST" && ./tests/manual-http-cleanup.sh' \
+    --pre-hook 'echo wtf2.pre >> "$HOOK_TEST"' \
+    --post-hook 'echo wtf2.post >> "$HOOK_TEST"'
 
 common certonly -a manual -d dns.le.wtf --preferred-challenges dns-01 \
     --manual-auth-hook ./tests/manual-dns-auth.sh
@@ -78,8 +114,11 @@ common_no_force_renew renew
 CheckCertCount 1
 
 # --renew-by-default is used, so renewal should occur
+[ -f "$HOOK_TEST" ] && rm -f "$HOOK_TEST"
 common renew
 CheckCertCount 2
+CheckHooks
+
 
 # This will renew because the expiry is less than 10 years from now
 sed -i "4arenew_before_expiry = 4 years" "$root/conf/renewal/le.wtf.conf"