If an installer is provided to certonly, restart after cert issuance (c…

…ertbot#9184)

* If an installer is provided to certonly, restart after cert issuance

* Add myself to AUTHORS.md

* Handle certonly's "installer" error case

* Handle interactive case, use lazy interpolation

* fix trailing whitespace

* fix whitespace in error message, re-raise exception

* Handle cases where user specified an authenticator but no installer

* make tox happy

* Clarify comment in selection.py

Co-authored-by: ohemorange <ebportnoy@gmail.com>

* Add tests for the certonly installer changes

Co-authored-by: ohemorange <ebportnoy@gmail.com>

AUTHORS.md

@@ -274,6 +274,7 @@ Authors
 * [Wilfried Teiken](https://github.com/wteiken)
 * [Willem Fibbe](https://github.com/fibbers)
 * [William Budington](https://github.com/Hainish)
+* [Will Greenberg](https://github.com/wgreenberg)
 * [Will Newby](https://github.com/willnewby)
 * [Will Oller](https://github.com/willoller)
 * [Yan](https://github.com/diracdeltas)

certbot/CHANGELOG.md

@@ -100,6 +100,9 @@ More details about these changes can be found on our GitHub repo.
 
 ## 1.23.0 - 2022-02-08
 
+* When `certonly` is run with an installer specified (e.g.  `--nginx`),
+  `certonly` will now also run `restart` for that installer
+
 ### Added
 
 * Added `show_account` subcommand, which will fetch the account information

certbot/certbot/_internal/main.py

@@ -536,12 +536,21 @@ def _report_next_steps(config: configuration.NamespaceConfig, installer_err: Opt
 
     # If the installation or enhancement raised an error, show advice on trying again
     if installer_err:
-        steps.append(
-            "The certificate was saved, but could not be installed (installer: "
-            f"{config.installer}). After fixing the error shown below, try installing it again "
-            f"by running:\n  {cli.cli_command} install --cert-name "
-            f"{_cert_name_from_config_or_lineage(config, lineage)}"
-        )
+        # Special case where either --nginx or --apache were used, causing us to
+        # run the "installer" (i.e. reloading the nginx/apache config)
+        if config.verb == 'certonly':
+            steps.append(
+                "The certificate was saved, but was not successfully loaded by the installer "
+                f"({config.installer}) due to the installer failing to reload. "
+                f"After fixing the error shown below, try reloading {config.installer} manually."
+            )
+        else:
+            steps.append(
+                "The certificate was saved, but could not be installed (installer: "
+                f"{config.installer}). After fixing the error shown below, try installing it again "
+                f"by running:\n  {cli.cli_command} install --cert-name "
+                f"{_cert_name_from_config_or_lineage(config, lineage)}"
+            )
 
     # If a certificate was obtained or renewed, show applicable renewal advice
     if new_or_renewed_cert:
@@ -1581,12 +1590,24 @@ def certonly(config: configuration.NamespaceConfig, plugins: plugins_disco.Plugi
 
     lineage = _get_and_save_cert(le_client, config, domains, certname, lineage)
 
+    # If a new cert was issued and we were passed an installer, we can safely
+    # run `installer.restart()` to load the newly issued certificate
+    installer_err: Optional[errors.Error] = None
+    if lineage and installer and not config.dry_run:
+        logger.info("Reloading %s server after certificate issuance", config.installer)
+        try:
+            installer.restart()
+        except errors.Error as e:
+            installer_err = e
+
     cert_path = lineage.cert_path if lineage else None
     fullchain_path = lineage.fullchain_path if lineage else None
     key_path = lineage.key_path if lineage else None
     _report_new_cert(config, cert_path, fullchain_path, key_path)
-    _report_next_steps(config, None, lineage,
+    _report_next_steps(config, installer_err, lineage,
                        new_or_renewed_cert=should_get_cert and not config.dry_run)
+    if installer_err:
+        raise installer_err
     _suggest_donation_if_appropriate(config)
     eff.handle_subscription(config, le_client.account)
 

certbot/certbot/_internal/plugins/selection.py

@@ -249,14 +249,27 @@ def choose_configurator_plugins(config: configuration.NamespaceConfig,
             installer = pick_installer(config, req_inst, plugins, installer_question)
         if need_auth:
             authenticator = pick_authenticator(config, req_auth, plugins)
-    logger.debug("Selected authenticator %s and installer %s", authenticator, installer)
 
     # Report on any failures
     if need_inst and not installer:
         diagnose_configurator_problem("installer", req_inst, plugins)
     if need_auth and not authenticator:
         diagnose_configurator_problem("authenticator", req_auth, plugins)
 
+    # As a special case for certonly, if a user selected apache or nginx, set
+    # the relevant installer (unless the user specifically specified no
+    # installer or only specified an authenticator on the command line)
+    if verb == "certonly" and authenticator is not None:
+        # user specified --nginx or --apache on CLI
+        selected_configurator = config.nginx or config.apache
+        # user didn't request an authenticator, and so interactively chose nginx
+        # or apache
+        interactively_selected = req_auth is None and authenticator.name in ("nginx", "apache")
+
+        if selected_configurator or interactively_selected:
+            installer = cast(Optional[interfaces.Installer], authenticator)
+    logger.debug("Selected authenticator %s and installer %s", authenticator, installer)
+
     record_chosen_plugins(config, plugins, authenticator, installer)
     return installer, authenticator
 

certbot/tests/main_test.py

@@ -314,6 +314,36 @@ def test_dryrun_next_steps_no_cert_saved(self, mock_lineage, mock_csr_get_cert,
                 mock.ANY, mock.ANY, mock.ANY, new_or_renewed_cert=False)
             mock_report_next_steps.reset_mock()
 
+    @mock.patch('certbot._internal.main._report_next_steps')
+    @mock.patch('certbot._internal.main._report_new_cert')
+    @mock.patch('certbot._internal.main._find_cert')
+    @mock.patch('certbot._internal.main._get_and_save_cert')
+    @mock.patch('certbot._internal.plugins.selection.choose_configurator_plugins')
+    def test_installer_runs_restart(self, mock_sel, mock_get_cert, mock_find_cert,
+                                    unused_report_new, unused_report_next):
+        mock_installer = mock.MagicMock()
+        mock_sel.return_value = (mock_installer, None)
+        mock_get_cert.return_value = mock.MagicMock()
+        mock_find_cert.return_value = (True, None)
+
+        self._call('certonly --nginx -d example.com'.split())
+        mock_installer.restart.assert_called_once()
+
+    @mock.patch('certbot._internal.main._report_next_steps')
+    @mock.patch('certbot._internal.main._report_new_cert')
+    @mock.patch('certbot._internal.main._find_cert')
+    @mock.patch('certbot._internal.main._get_and_save_cert')
+    @mock.patch('certbot._internal.plugins.selection.choose_configurator_plugins')
+    def test_dryrun_installer_doesnt_restart(self, mock_sel, mock_get_cert, mock_find_cert,
+                                             unused_report_new, unused_report_next):
+        mock_installer = mock.MagicMock()
+        mock_sel.return_value = (mock_installer, None)
+        mock_get_cert.return_value = mock.MagicMock()
+        mock_find_cert.return_value = (True, None)
+
+        self._call('certonly --nginx -d example.com --dry-run'.split())
+        mock_installer.restart.assert_not_called()
+
 
 class FindDomainsOrCertnameTest(unittest.TestCase):
     """Tests for certbot._internal.main._find_domains_or_certname."""

certbot/tests/plugins/selection_test.py

@@ -199,5 +199,71 @@ def test_multiple_installers_returned(self):
         self.assertRaises(errors.PluginSelectionError, self._call)
 
 
+class TestChooseConfiguratorPlugins(unittest.TestCase):
+    """Tests for certbot._internal.plugins.selection.choose_configurator_plugins."""
+
+    def _setupMockPlugin(self, name):
+        mock_ep = mock.Mock(
+            description_with_name=name)
+        mock_ep.check_name = lambda n: n == name
+        mock_plugin = mock.MagicMock()
+        mock_plugin.name = name
+        mock_ep.init.return_value = mock_plugin
+        mock_ep.misconfigured = False
+        return mock_ep
+
+    def _parseArgs(self, args):
+        from certbot import configuration
+        from certbot._internal import cli
+        return configuration.NamespaceConfig(
+            cli.prepare_and_parse_args(self.plugins, args.split()))
+
+    def setUp(self):
+        self.plugins = PluginsRegistry({
+            "nginx": self._setupMockPlugin("nginx"),
+            "apache": self._setupMockPlugin("apache"),
+            "manual": self._setupMockPlugin("manual"),
+        })
+
+    def _runWithArgs(self, args):
+        from certbot._internal.plugins.selection import choose_configurator_plugins
+        return choose_configurator_plugins(self._parseArgs(args), self.plugins, "certonly")
+
+    def test_noninteractive_configurator(self):
+        # For certonly, setting either the nginx or apache configurators should
+        # return both an installer and authenticator
+        inst, auth = self._runWithArgs("certonly --nginx")
+        self.assertEqual(inst.name, "nginx")
+        self.assertEqual(auth.name, "nginx")
+
+        inst, auth = self._runWithArgs("certonly --apache")
+        self.assertEqual(inst.name, "apache")
+        self.assertEqual(auth.name, "apache")
+
+    def test_noninteractive_inst_arg(self):
+        # For certonly, if an installer arg is set, it should be returned as expected
+        inst, auth = self._runWithArgs("certonly -a nginx -i nginx")
+        self.assertEqual(inst.name, "nginx")
+        self.assertEqual(auth.name, "nginx")
+
+        inst, auth = self._runWithArgs("certonly -a apache -i apache")
+        self.assertEqual(inst.name, "apache")
+        self.assertEqual(auth.name, "apache")
+
+        # if no installer arg is set (or it's set to none), one shouldn't be returned
+        inst, auth = self._runWithArgs("certonly -a nginx")
+        self.assertEqual(inst, None)
+        self.assertEqual(auth.name, "nginx")
+        inst, auth = self._runWithArgs("certonly -a nginx -i none")
+        self.assertEqual(inst, None)
+        self.assertEqual(auth.name, "nginx")
+
+        inst, auth = self._runWithArgs("certonly -a apache")
+        self.assertEqual(inst, None)
+        self.assertEqual(auth.name, "apache")
+        inst, auth = self._runWithArgs("certonly -a apache -i none")
+        self.assertEqual(inst, None)
+        self.assertEqual(auth.name, "apache")
+
 if __name__ == "__main__":
     unittest.main()  # pragma: no cover