diff --git a/certbot/cli.py b/certbot/cli.py index 6c513b4a1..f2b5bd21d 100644 --- a/certbot/cli.py +++ b/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" diff --git a/certbot/hooks.py b/certbot/hooks.py index 63afba091..ce4f16262 100644 --- a/certbot/hooks.py +++ b/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 diff --git a/certbot/main.py b/certbot/main.py index 838416822..91b860dbb 100644 --- a/certbot/main.py +++ b/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) diff --git a/certbot/plugins/util.py b/certbot/plugins/util.py index 1964ae349..20b0fdce7 100644 --- a/certbot/plugins/util.py +++ b/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 diff --git a/certbot/renewal.py b/certbot/renewal.py index 6448e5a8e..3bc7a30f9 100644 --- a/certbot/renewal.py +++ b/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"] diff --git a/certbot/tests/hook_test.py b/certbot/tests/hook_test.py index be7fb852d..3165a67fc 100644 --- a/certbot/tests/hook_test.py +++ b/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', {}): diff --git a/tests/boulder-integration.sh b/tests/boulder-integration.sh index e7975454b..fe2f223ef 100755 --- a/tests/boulder-integration.sh +++ b/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"