diff options
author | Will Greenberg <willg@eff.org> | 2022-04-27 04:51:57 +0300 |
---|---|---|
committer | GitHub <noreply@github.com> | 2022-04-27 04:51:57 +0300 |
commit | 8066f230f57034429d83a718ed234b332bdfd597 (patch) | |
tree | ac5e989ab310388962838840c384087278380333 | |
parent | 3b6f3450c2ff4cba31a39d295676ffbc3176550a (diff) |
If an installer is provided to certonly, restart after cert issuance (#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>
-rw-r--r-- | AUTHORS.md | 1 | ||||
-rw-r--r-- | certbot/CHANGELOG.md | 3 | ||||
-rw-r--r-- | certbot/certbot/_internal/main.py | 35 | ||||
-rw-r--r-- | certbot/certbot/_internal/plugins/selection.py | 15 | ||||
-rw-r--r-- | certbot/tests/main_test.py | 30 | ||||
-rw-r--r-- | certbot/tests/plugins/selection_test.py | 66 |
6 files changed, 142 insertions, 8 deletions
diff --git a/AUTHORS.md b/AUTHORS.md index 28b52dd05..44bbe02ab 100644 --- a/AUTHORS.md +++ b/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) diff --git a/certbot/CHANGELOG.md b/certbot/CHANGELOG.md index 99d77b80f..42f96f3f6 100644 --- a/certbot/CHANGELOG.md +++ b/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 diff --git a/certbot/certbot/_internal/main.py b/certbot/certbot/_internal/main.py index 85213c9d3..098ce3243 100644 --- a/certbot/certbot/_internal/main.py +++ b/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) diff --git a/certbot/certbot/_internal/plugins/selection.py b/certbot/certbot/_internal/plugins/selection.py index f62db4089..cde6bd221 100644 --- a/certbot/certbot/_internal/plugins/selection.py +++ b/certbot/certbot/_internal/plugins/selection.py @@ -249,7 +249,6 @@ 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: @@ -257,6 +256,20 @@ def choose_configurator_plugins(config: configuration.NamespaceConfig, 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 diff --git a/certbot/tests/main_test.py b/certbot/tests/main_test.py index f168b0bca..61632fc8f 100644 --- a/certbot/tests/main_test.py +++ b/certbot/tests/main_test.py @@ -314,6 +314,36 @@ class CertonlyTest(unittest.TestCase): 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.""" diff --git a/certbot/tests/plugins/selection_test.py b/certbot/tests/plugins/selection_test.py index 3c129859f..fb090f05d 100644 --- a/certbot/tests/plugins/selection_test.py +++ b/certbot/tests/plugins/selection_test.py @@ -199,5 +199,71 @@ class GetUnpreparedInstallerTest(test_util.ConfigTestCase): 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 |