diff --git a/certbot/client.py b/certbot/client.py index b9dbaf480..4d6de6375 100644 --- a/certbot/client.py +++ b/certbot/client.py @@ -196,11 +196,6 @@ class Client(object): else: self.auth_handler = None - # Warn if the client is using unsupported config options with an - # installer - if self.installer is not None: - self._verify_all_config_options_supported(config) - def obtain_certificate_from_csr(self, domains, csr, typ=OpenSSL.crypto.FILETYPE_ASN1, authzr=None): """Obtain certificate. @@ -388,16 +383,10 @@ class Client(object): # sites may have been enabled / final cleanup self.installer.restart() - def enhance_config(self, domains, config, chain_path): + def enhance_config(self, domains, chain_path): """Enhance the configuration. :param list domains: list of domains to configure - - :ivar config: Namespace typically produced by - :meth:`argparse.ArgumentParser.parse_args`. - it must have the redirect, hsts and uir attributes. - :type namespace: :class:`argparse.Namespace` - :param chain_path: chain file path :type chain_path: `str` or `None` @@ -405,40 +394,34 @@ class Client(object): client. """ - if self.installer is None: logger.warning("No installer is specified, there isn't any " "configuration to enhance.") raise errors.Error("No installer available") - if config is None: - logger.warning("No config is specified.") - raise errors.Error("No config available") - + enhanced = False + enhancement_info = ( + ("hsts", "ensure-http-header", "Strict-Transport-Security"), + ("redirect", "redirect", None), + ("staple", "staple-ocsp", chain_path), + ("uir", "ensure-http-header", "Upgrade-Insecure-Requests"),) supported = self.installer.supported_enhancements() - redirect = config.redirect if "redirect" in supported else False - hsts = config.hsts if "ensure-http-header" in supported else False - uir = config.uir if "ensure-http-header" in supported else False - staple = config.staple if "staple-ocsp" in supported else False - - if redirect is None: - redirect = enhancements.ask("redirect") - - if redirect: - self.apply_enhancement(domains, "redirect") - - if hsts: - self.apply_enhancement(domains, "ensure-http-header", - "Strict-Transport-Security") - if uir: - self.apply_enhancement(domains, "ensure-http-header", - "Upgrade-Insecure-Requests") - if staple: - self.apply_enhancement(domains, "staple-ocsp", chain_path) + for config_name, enhancement_name, option in enhancement_info: + config_value = getattr(self.config, config_name) + if enhancement_name in supported: + if config_name == "redirect" and config_value is None: + config_value = enhancements.ask(enhancement_name) + if config_value: + self.apply_enhancement(domains, enhancement_name, option) + enhanced = True + elif config_value: + logger.warning( + "Option %s is not supported by the selected installer. " + "Skipping enhancement.", config_name) msg = ("We were unable to restart web server") - if redirect or hsts or uir or staple: + if enhanced: with error_handler.ErrorHandler(self._rollback_and_restart, msg): self.installer.restart() @@ -508,32 +491,6 @@ class Client(object): raise reporter.add_message(success_msg, reporter.HIGH_PRIORITY) - def _verify_all_config_options_supported(self, config): - """Verifies that all config options are supported in current installer. - - :ivar config: Namespace typically produced by - :meth:`argparse.ArgumentParser.parse_args`. - :type namespace: :class:`argparse.Namespace` - """ - # Mapping between config options and string describing config option - # support in installer supported_enhancements. - option_support = { - "redirect": "redirect", - "hsts": "ensure-http-header", - "uir": "ensure-http-header", - "staple": "staple-ocsp" - } - - supported = self.installer.supported_enhancements() - - for config_name, support_string in option_support.items(): - config_value = getattr(config, config_name, None) - if config_value and support_string not in supported: - msg = ("Option %s is not allowed with the current installer. " - "Please disable the option and try again." % - config_name) - logger.warning(msg) - def validate_key_csr(privkey, csr=None): """Validate Key and CSR files. diff --git a/certbot/main.py b/certbot/main.py index 1f3379fd7..37af009e3 100644 --- a/certbot/main.py +++ b/certbot/main.py @@ -435,7 +435,7 @@ def install(config, plugins): le_client.deploy_certificate( domains, config.key_path, config.cert_path, config.chain_path, config.fullchain_path) - le_client.enhance_config(domains, config, config.chain_path) + le_client.enhance_config(domains, config.chain_path) def plugins_cmd(config, plugins): # TODO: Use IDisplay rather than print @@ -532,7 +532,7 @@ def run(config, plugins): # pylint: disable=too-many-branches,too-many-locals domains, lineage.privkey, lineage.cert, lineage.chain, lineage.fullchain) - le_client.enhance_config(domains, config, lineage.chain) + le_client.enhance_config(domains, lineage.chain) if action in ("newcert", "reinstall",): display_ops.success_installation(domains) diff --git a/certbot/tests/client_test.py b/certbot/tests/client_test.py index 02e0bedf9..d79fb1852 100644 --- a/certbot/tests/client_test.py +++ b/certbot/tests/client_test.py @@ -102,15 +102,13 @@ class RegisterTest(unittest.TestCase): mock_client().register.side_effect = [mx_err, mock.MagicMock()] self.assertRaises(messages.Error, self._call) -class ClientTest(unittest.TestCase): - """Tests for certbot.client.Client.""" +class ClientTestCommon(unittest.TestCase): + """Common base class for certbot.client.Client tests.""" def setUp(self): - self.config = mock.MagicMock( - no_verify_ssl=False, config_dir="/etc/letsencrypt", allow_subset_of_names=False) # pylint: disable=star-args self.account = mock.MagicMock(**{"key.pem": KEY}) - self.eg_domains = ["example.com", "www.example.com"] + self.config = mock.MagicMock(no_verify_ssl=False) from certbot.client import Client with mock.patch("certbot.client.acme_client.Client") as acme: @@ -120,16 +118,15 @@ class ClientTest(unittest.TestCase): config=self.config, account_=self.account, auth=None, installer=None) - def test__init___warn_unsupported_config_options(self): - config = ConfigHelper(redirect=True, hsts=True) - installer = mock.MagicMock() - installer.supported_enhancements.return_value = ["redirect"] - with mock.patch('certbot.client.logger') as mock_logger: - from certbot.client import Client - Client(config=config, account_=self.account, auth=None, - installer=installer, acme=self.acme) - mock_logger.warning.assert_called_once_with(mock.ANY) +class ClientTest(ClientTestCommon): + """Tests for certbot.client.Client.""" + def setUp(self): + super(ClientTest, self).setUp() + + self.config.allow_subset_of_names = False + self.config.config_dir = "/etc/letsencrypt" + self.eg_domains = ["example.com", "www.example.com"] def test_init_acme_verify_ssl(self): net = self.acme_client.call_args[1]["net"] @@ -325,147 +322,109 @@ class ClientTest(unittest.TestCase): installer.rollback_checkpoints.assert_called_once_with() self.assertEqual(installer.restart.call_count, 1) - @mock.patch("certbot.client.enhancements") - def test_enhance_config(self, mock_enhancements): - config = ConfigHelper(redirect=True, hsts=False, uir=False) - self.assertRaises(errors.Error, self.client.enhance_config, - ["foo.bar"], config, None) - mock_enhancements.ask.return_value = True - installer = mock.MagicMock() - self.client.installer = installer - installer.supported_enhancements.return_value = ["redirect"] +class EnhanceConfigTest(ClientTestCommon): + """Tests for certbot.client.Client.enhance_config.""" + def setUp(self): + super(EnhanceConfigTest, self).setUp() - self.client.enhance_config(["foo.bar"], config, None) - installer.enhance.assert_called_once_with("foo.bar", "redirect", None) - self.assertEqual(installer.save.call_count, 1) - installer.restart.assert_called_once_with() + self.config.hsts = False + self.config.redirect = False + self.config.staple = False + self.config.uir = False + self.domain = "example.org" + + def test_no_installer(self): + self.assertRaises( + errors.Error, self.client.enhance_config, [self.domain], None) @mock.patch("certbot.client.enhancements") - def test_enhance_config_no_ask(self, mock_enhancements): - config = ConfigHelper(redirect=True, hsts=False, - uir=False, staple=False) - self.assertRaises(errors.Error, self.client.enhance_config, - ["foo.bar"], config, None) + def test_unsupported(self, mock_enhancements): + self.client.installer = mock.MagicMock() + self.client.installer.supported_enhancements.return_value = [] - mock_enhancements.ask.return_value = True - installer = mock.MagicMock() - self.client.installer = installer - installer.supported_enhancements.return_value = [ - "redirect", "ensure-http-header", "staple-ocsp"] - - config = ConfigHelper(redirect=True, hsts=False, - uir=False, staple=False) - self.client.enhance_config(["foo.bar"], config, None) - installer.enhance.assert_called_with("foo.bar", "redirect", None) - - config = ConfigHelper(redirect=False, hsts=True, - uir=False, staple=False) - self.client.enhance_config(["foo.bar"], config, None) - installer.enhance.assert_called_with("foo.bar", "ensure-http-header", - "Strict-Transport-Security") - - config = ConfigHelper(redirect=False, hsts=False, - uir=True, staple=False) - self.client.enhance_config(["foo.bar"], config, None) - installer.enhance.assert_called_with("foo.bar", "ensure-http-header", - "Upgrade-Insecure-Requests") - - config = ConfigHelper(redirect=False, hsts=False, - uir=False, staple=True) - self.client.enhance_config(["foo.bar"], config, None) - installer.enhance.assert_called_with("foo.bar", "staple-ocsp", None) - - self.assertEqual(installer.save.call_count, 4) - self.assertEqual(installer.restart.call_count, 4) - - @mock.patch("certbot.client.enhancements") - def test_enhance_config_unsupported(self, mock_enhancements): - installer = mock.MagicMock() - self.client.installer = installer - installer.supported_enhancements.return_value = [] - - config = ConfigHelper(redirect=None, hsts=True, uir=True) - self.client.enhance_config(["foo.bar"], config, None) - installer.enhance.assert_not_called() + self.config.redirect = None + self.config.hsts = True + with mock.patch("certbot.client.logger") as mock_logger: + self.client.enhance_config([self.domain], None) + self.assertEqual(mock_logger.warning.call_count, 1) + self.client.installer.enhance.assert_not_called() mock_enhancements.ask.assert_not_called() - def test_enhance_config_no_installer(self): - config = ConfigHelper(redirect=True, hsts=False, uir=False) - self.assertRaises(errors.Error, self.client.enhance_config, - ["foo.bar"], config, None) + def test_no_ask_hsts(self): + self.config.hsts = True + self._test_with_all_supported() + self.client.installer.enhance.assert_called_with( + self.domain, "ensure-http-header", "Strict-Transport-Security") - @mock.patch("certbot.client.zope.component.getUtility") - @mock.patch("certbot.client.enhancements") - def test_enhance_config_enhance_failure(self, mock_enhancements, - mock_get_utility): - mock_enhancements.ask.return_value = True + def test_no_ask_redirect(self): + self.config.redirect = True + self._test_with_all_supported() + self.client.installer.enhance.assert_called_with( + self.domain, "redirect", None) + + def test_no_ask_staple(self): + self.config.staple = True + self._test_with_all_supported() + self.client.installer.enhance.assert_called_with( + self.domain, "staple-ocsp", None) + + def test_no_ask_uir(self): + self.config.uir = True + self._test_with_all_supported() + self.client.installer.enhance.assert_called_with( + self.domain, "ensure-http-header", "Upgrade-Insecure-Requests") + + def test_enhance_failure(self): + self.client.installer = mock.MagicMock() + self.client.installer.enhance.side_effect = errors.PluginError + self._test_error() + self.client.installer.recovery_routine.assert_called_once_with() + + def test_save_failure(self): + self.client.installer = mock.MagicMock() + self.client.installer.save.side_effect = errors.PluginError + self._test_error() + self.client.installer.recovery_routine.assert_called_once_with() + self.client.installer.save.assert_called_once_with(mock.ANY) + + def test_restart_failure(self): + self.client.installer = mock.MagicMock() + self.client.installer.restart.side_effect = [errors.PluginError, None] + self._test_error_with_rollback() + + def test_restart_failure2(self): installer = mock.MagicMock() - self.client.installer = installer - installer.supported_enhancements.return_value = ["redirect"] - installer.enhance.side_effect = errors.PluginError - - config = ConfigHelper(redirect=True, hsts=False, uir=False) - - self.assertRaises(errors.PluginError, self.client.enhance_config, - ["foo.bar"], config, None) - installer.recovery_routine.assert_called_once_with() - self.assertEqual(mock_get_utility().add_message.call_count, 1) - - @mock.patch("certbot.client.zope.component.getUtility") - @mock.patch("certbot.client.enhancements") - def test_enhance_config_save_failure(self, mock_enhancements, - mock_get_utility): - mock_enhancements.ask.return_value = True - installer = mock.MagicMock() - self.client.installer = installer - installer.supported_enhancements.return_value = ["redirect"] - installer.save.side_effect = errors.PluginError - - config = ConfigHelper(redirect=True, hsts=False, uir=False) - - self.assertRaises(errors.PluginError, self.client.enhance_config, - ["foo.bar"], config, None) - installer.recovery_routine.assert_called_once_with() - self.assertEqual(mock_get_utility().add_message.call_count, 1) - - @mock.patch("certbot.client.zope.component.getUtility") - @mock.patch("certbot.client.enhancements") - def test_enhance_config_restart_failure(self, mock_enhancements, - mock_get_utility): - mock_enhancements.ask.return_value = True - installer = mock.MagicMock() - self.client.installer = installer - installer.supported_enhancements.return_value = ["redirect"] - installer.restart.side_effect = [errors.PluginError, None] - - config = ConfigHelper(redirect=True, hsts=False, uir=False) - - self.assertRaises(errors.PluginError, self.client.enhance_config, - ["foo.bar"], config, None) - - self.assertEqual(mock_get_utility().add_message.call_count, 1) - installer.rollback_checkpoints.assert_called_once_with() - self.assertEqual(installer.restart.call_count, 2) - - @mock.patch("certbot.client.zope.component.getUtility") - @mock.patch("certbot.client.enhancements") - def test_enhance_config_restart_failure2(self, mock_enhancements, - mock_get_utility): - mock_enhancements.ask.return_value = True - installer = mock.MagicMock() - self.client.installer = installer - installer.supported_enhancements.return_value = ["redirect"] installer.restart.side_effect = errors.PluginError installer.rollback_checkpoints.side_effect = errors.ReverterError + self.client.installer = installer + self._test_error_with_rollback() - config = ConfigHelper(redirect=True, hsts=False, uir=False) + @mock.patch("certbot.client.enhancements.ask") + def test_ask(self, mock_ask): + self.config.redirect = None + mock_ask.return_value = True + self._test_with_all_supported() - self.assertRaises(errors.PluginError, self.client.enhance_config, - ["foo.bar"], config, None) - self.assertEqual(mock_get_utility().add_message.call_count, 1) - installer.rollback_checkpoints.assert_called_once_with() - self.assertEqual(installer.restart.call_count, 1) + def _test_error_with_rollback(self): + self._test_error() + self.assertTrue(self.client.installer.restart.called) + + def _test_error(self): + self.config.redirect = True + with mock.patch("certbot.client.zope.component.getUtility") as mock_gu: + self.assertRaises( + errors.PluginError, self._test_with_all_supported) + self.assertEqual(mock_gu().add_message.call_count, 1) + + def _test_with_all_supported(self): + if self.client.installer is None: + self.client.installer = mock.MagicMock() + self.client.installer.supported_enhancements.return_value = [ + "ensure-http-header", "redirect", "staple-ocsp"] + self.client.enhance_config([self.domain], None) + self.assertEqual(self.client.installer.save.call_count, 1) + self.assertEqual(self.client.installer.restart.call_count, 1) class RollbackTest(unittest.TestCase):