mirror of
https://github.com/certbot/certbot.git
synced 2026-06-07 15:52:08 -04:00
Security enhancement cleanup (#3837)
* Stop passing around config and refactor tests * Refactor and warn during enhance_config * Use mock.ANY to make new Pythons happy * Remove verbose enhance_config from test names * Fix spacing in warning
This commit is contained in:
Brad Warren
Peter Eckersley
parent
8b67a58f3c
commit
da3332ccfa
3 changed files with 123 additions and 207 deletions
|
|
@ -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.
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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):
|
||||
|
|
|
|||
Loading…
Reference in a new issue