Run directory hooks during commands other than renew (#10146)

Partially fixes #9869. Fixes #9978.

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
 

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",

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,

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):

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')

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
 <config-file>`, or :ref:`renewal configuration files <renewal-config-file>` are