diff --git a/certbot/CHANGELOG.md b/certbot/CHANGELOG.md index 4675c4054..3dfaf1db8 100644 --- a/certbot/CHANGELOG.md +++ b/certbot/CHANGELOG.md @@ -16,6 +16,7 @@ Certbot adheres to [Semantic Versioning](https://semver.org/). * Deprecated `gen_ss_cert` in `acme.crypto_util` as it uses deprecated pyOpenSSL API. * Add `make_self_signed_cert` to `acme.crypto_util` to replace `gen_ss_cert. +* Directory hooks are now run on all commands by default, not just `renew` ### Fixed diff --git a/certbot/certbot/_internal/cli/__init__.py b/certbot/certbot/_internal/cli/__init__.py index 0eccef8d8..438556b68 100644 --- a/certbot/certbot/_internal/cli/__init__.py +++ b/certbot/certbot/_internal/cli/__init__.py @@ -442,8 +442,8 @@ def prepare_and_parse_args(plugins: plugins_disco.PluginsRegistry, args: List[st helpful.add( "renew", "--no-directory-hooks", action="store_false", default=flag_default("directory_hooks"), dest="directory_hooks", - help="Disable running executables found in Certbot's hook directories" - " during renewal. (default: False)") + help="Disable running executables found in Certbot's hook directories." + " (default: False)") helpful.add( "renew", "--disable-renew-updates", action="store_true", default=flag_default("disable_renew_updates"), dest="disable_renew_updates", diff --git a/certbot/certbot/_internal/hooks.py b/certbot/certbot/_internal/hooks.py index 1c6214c62..f75ecd02e 100644 --- a/certbot/certbot/_internal/hooks.py +++ b/certbot/certbot/_internal/hooks.py @@ -75,13 +75,11 @@ def pre_hook(config: configuration.NamespaceConfig) -> None: :param configuration.NamespaceConfig config: Certbot settings """ - if config.verb == "renew" and config.directory_hooks: - for hook in list_hooks(config.renewal_pre_hooks_dir): - _run_pre_hook_if_necessary(hook) - - cmd = config.pre_hook - if cmd: - _run_pre_hook_if_necessary(cmd) + all_hooks: List[str] = (list_hooks(config.renewal_pre_hooks_dir) if config.directory_hooks + else []) + all_hooks += [config.pre_hook] if config.pre_hook else [] + for hook in all_hooks: + _run_pre_hook_if_necessary(hook) executed_pre_hooks: Set[str] = set() @@ -125,32 +123,31 @@ def post_hook( """ - cmd = config.post_hook + all_hooks: List[str] = (list_hooks(config.renewal_post_hooks_dir) if config.directory_hooks + else []) + all_hooks += [config.post_hook] if config.post_hook else [] # In the "renew" case, we save these up to run at the end if config.verb == "renew": - if config.directory_hooks: - for hook in list_hooks(config.renewal_post_hooks_dir): - _run_eventually(hook) - if cmd: - _run_eventually(cmd) + for hook in all_hooks: + _run_eventually(hook) # certonly / run - elif cmd: + else: renewed_domains_str = ' '.join(renewed_domains) # 32k is reasonable on Windows and likely quite conservative on other platforms if len(renewed_domains_str) > 32_000: logger.warning("Limiting RENEWED_DOMAINS environment variable to 32k characters") renewed_domains_str = renewed_domains_str[:32_000] - - _run_hook( - "post-hook", - cmd, - { - 'RENEWED_DOMAINS': renewed_domains_str, - # Since other commands stop certbot execution on failure, - # it doesn't make sense to have a FAILED_DOMAINS variable - 'FAILED_DOMAINS': "" - } - ) + for hook in all_hooks: + _run_hook( + "post-hook", + hook, + { + 'RENEWED_DOMAINS': renewed_domains_str, + # Since other commands stop certbot execution on failure, + # it doesn't make sense to have a FAILED_DOMAINS variable + 'FAILED_DOMAINS': "" + } + ) post_hooks: List[str] = [] @@ -228,19 +225,16 @@ def renew_hook(config: configuration.NamespaceConfig, domains: List[str], :param str lineage_path: live directory path for the new cert """ - executed_dir_hooks = set() - if config.directory_hooks: - for hook in list_hooks(config.renewal_deploy_hooks_dir): - _run_deploy_hook(hook, domains, lineage_path, config.dry_run, config.run_deploy_hooks) - executed_dir_hooks.add(hook) - - if config.renew_hook: - if config.renew_hook in executed_dir_hooks: - logger.info("Skipping deploy-hook '%s' as it was already run.", - config.renew_hook) + executed_hooks = set() + all_hooks: List[str] = (list_hooks(config.renewal_deploy_hooks_dir)if config.directory_hooks + else []) + all_hooks += [config.renew_hook] if config.renew_hook else [] + for hook in all_hooks: + if hook in executed_hooks: + logger.info("Skipping deploy-hook '%s' as it was already run.", hook) else: - _run_deploy_hook(config.renew_hook, domains, - lineage_path, config.dry_run, config.run_deploy_hooks) + _run_deploy_hook(hook, domains, lineage_path, config.dry_run, config.run_deploy_hooks) + executed_hooks.add(hook) def _run_deploy_hook(command: str, domains: List[str], lineage_path: str, dry_run: bool, diff --git a/certbot/certbot/_internal/tests/hook_test.py b/certbot/certbot/_internal/tests/hook_test.py index 798f2ac83..268b5a84e 100644 --- a/certbot/certbot/_internal/tests/hook_test.py +++ b/certbot/certbot/_internal/tests/hook_test.py @@ -136,7 +136,8 @@ class PreHookTest(HookTest): def _test_nonrenew_common(self): mock_execute = self._call_with_mock_execute(self.config) - mock_execute.assert_called_once_with("pre-hook", self.config.pre_hook, env=mock.ANY) + mock_execute.assert_any_call("pre-hook", self.dir_hook, env=mock.ANY) + mock_execute.assert_called_with("pre-hook", self.config.pre_hook, env=mock.ANY) self._test_no_executions_common() def test_no_hooks(self): @@ -208,14 +209,16 @@ class PostHookTest(HookTest): for verb in ("certonly", "run",): self.config.verb = verb mock_execute = self._call_with_mock_execute(self.config, []) - mock_execute.assert_called_once_with("post-hook", self.config.post_hook, env=mock.ANY) + mock_execute.assert_any_call("post-hook", self.dir_hook, env=mock.ANY) + mock_execute.assert_called_with("post-hook", self.config.post_hook, env=mock.ANY) assert not self._get_eventually() - def test_cert_only_and_run_without_hook(self): + def test_certonly_and_run_without_cli_hook(self): self.config.post_hook = None for verb in ("certonly", "run",): self.config.verb = verb - assert not self._call_with_mock_execute(self.config, []).called + mock_execute = self._call_with_mock_execute(self.config, []) + mock_execute.assert_called_once_with("post-hook", self.dir_hook, env=mock.ANY) assert not self._get_eventually() def test_renew_env(self): diff --git a/certbot/certbot/_internal/tests/main_test.py b/certbot/certbot/_internal/tests/main_test.py index 8618241fc..343f7f135 100644 --- a/certbot/certbot/_internal/tests/main_test.py +++ b/certbot/certbot/_internal/tests/main_test.py @@ -264,7 +264,7 @@ class CertonlyTest(unittest.TestCase): mock_domains.return_value = domains mock_lineage.names.return_value = domains self._call(('certonly --webroot -d example.com -d test.org ' - '--cert-name example.com').split()) + '--cert-name example.com --no-directory-hooks').split()) assert mock_lineage.call_count == 1 assert mock_domains.call_count == 1 @@ -276,7 +276,7 @@ class CertonlyTest(unittest.TestCase): # user confirms updating lineage with new domains self._call(('certonly --webroot -d example.com -d test.com ' - '--cert-name example.com').split()) + '--cert-name example.com --no-directory-hooks').split()) assert mock_lineage.call_count == 2 assert mock_domains.call_count == 2 assert mock_renew_cert.call_count == 2 @@ -286,7 +286,8 @@ class CertonlyTest(unittest.TestCase): # error in _ask_user_to_confirm_new_names self.mock_get_utility().yesno.return_value = False with pytest.raises(errors.ConfigurationError): - self._call('certonly --webroot -d example.com -d test.com --cert-name example.com'.split()) + self._call('certonly --webroot -d example.com -d test.com --cert-name example.com' + ' --no-directory-hooks'.split()) @mock.patch('certbot._internal.main._report_next_steps') @mock.patch('certbot._internal.cert_manager.domains_for_certname') @@ -299,14 +300,14 @@ class CertonlyTest(unittest.TestCase): # no lineage with this name but we specified domains so create a new cert self._call(('certonly --webroot -d example.com -d test.com ' - '--cert-name example.com').split()) + '--cert-name example.com --no-directory-hooks').split()) assert mock_lineage.call_count == 1 assert mock_report_cert.call_count == 1 # no lineage with this name and we didn't give domains mock_choose_names.return_value = ["somename"] mock_domains_for_certname.return_value = None - self._call(('certonly --webroot --cert-name example.com').split()) + self._call(('certonly --webroot --cert-name example.com --no-directory-hooks').split()) assert mock_choose_names.called is True @mock.patch('certbot._internal.main._report_next_steps') diff --git a/certbot/docs/using.rst b/certbot/docs/using.rst index 942bb9fb7..a2791705d 100644 --- a/certbot/docs/using.rst +++ b/certbot/docs/using.rst @@ -683,10 +683,13 @@ configuration directory. Assuming your configuration directory is ``/etc/letsencrypt/renewal-hooks/pre``, ``/etc/letsencrypt/renewal-hooks/deploy``, and ``/etc/letsencrypt/renewal-hooks/post`` will be run as pre, deploy, and post -hooks respectively when any certificate is renewed with the ``renew`` -subcommand. These hooks are run in alphabetical order and are not run for other -subcommands. (The order the hooks are run is determined by the byte value of -the characters in their filenames and is not dependent on your locale.) +hooks respectively. These hooks are run in alphabetical order. (The order the +hooks are run is determined by the byte value of the characters in their +filenames and is not dependent on your locale.) + +Prior to certbot 3.2.0, hooks in directories were only run when certificates +were renewed with the ``renew`` subcommand, but as of 3.2.0, they are run for +any subcommand. Hooks specified in the command line, :ref:`configuration file `, or :ref:`renewal configuration files ` are