From f88105a9529b47b91bb29dbe0e766e63b6911204 Mon Sep 17 00:00:00 2001 From: Adrien Ferrand Date: Fri, 28 May 2021 21:17:56 +0200 Subject: [PATCH] Deprecate usage of IConfig as a singleton in Certbot (#8869) * Deprecate usage of IConfig as a singleton in Certbot * Fix local oldest requirements * Add changelog * Add tests for certbot.crypto_util.init_save_* functions Co-authored-by: Brad Warren --- .../configurators/nginx/common.py | 1 - .../certbot_nginx/_internal/configurator.py | 5 +- certbot-nginx/local-oldest-requirements.txt | 2 +- certbot-nginx/setup.py | 2 +- certbot-nginx/tests/test_util.py | 4 - certbot/CHANGELOG.md | 4 + certbot/certbot/_internal/auth_handler.py | 4 +- certbot/certbot/_internal/client.py | 10 +- certbot/certbot/_internal/main.py | 3 + certbot/certbot/_internal/renewal.py | 3 +- certbot/certbot/crypto_util.py | 99 +++++++++++++++---- certbot/tests/auth_handler_test.py | 49 ++++----- certbot/tests/client_test.py | 37 +++---- certbot/tests/crypto_util_test.py | 60 +++++++---- 14 files changed, 192 insertions(+), 91 deletions(-) diff --git a/certbot-compatibility-test/certbot_compatibility_test/configurators/nginx/common.py b/certbot-compatibility-test/certbot_compatibility_test/configurators/nginx/common.py index e209480e3..7cba487cf 100644 --- a/certbot-compatibility-test/certbot_compatibility_test/configurators/nginx/common.py +++ b/certbot-compatibility-test/certbot_compatibility_test/configurators/nginx/common.py @@ -48,7 +48,6 @@ class Proxy(configurators_common.Proxy): setattr(self.le_config, "nginx_" + k, constants.os_constant(k)) conf = configuration.NamespaceConfig(self.le_config) - zope.component.provideUtility(conf) self._configurator = configurator.NginxConfigurator( config=conf, name="nginx") self._configurator.prepare() diff --git a/certbot-nginx/certbot_nginx/_internal/configurator.py b/certbot-nginx/certbot_nginx/_internal/configurator.py index 62122eef5..07397bfe8 100644 --- a/certbot-nginx/certbot_nginx/_internal/configurator.py +++ b/certbot-nginx/certbot_nginx/_internal/configurator.py @@ -678,8 +678,9 @@ class NginxConfigurator(common.Installer): """Generate invalid certs that let us create ssl directives for Nginx""" # TODO: generate only once tmp_dir = os.path.join(self.config.work_dir, "snakeoil") - le_key = crypto_util.init_save_key( - key_size=1024, key_dir=tmp_dir, keyname="key.pem") + le_key = crypto_util.generate_key( + key_size=1024, key_dir=tmp_dir, keyname="key.pem", + strict_permissions=self.config.strict_permissions) key = OpenSSL.crypto.load_privatekey( OpenSSL.crypto.FILETYPE_PEM, le_key.pem) cert = acme_crypto_util.gen_ss_cert(key, domains=[socket.gethostname()]) diff --git a/certbot-nginx/local-oldest-requirements.txt b/certbot-nginx/local-oldest-requirements.txt index 323e315f1..6dc9570cc 100644 --- a/certbot-nginx/local-oldest-requirements.txt +++ b/certbot-nginx/local-oldest-requirements.txt @@ -1,3 +1,3 @@ # Remember to update setup.py to match the package versions below. acme[dev]==1.8.0 -certbot[dev]==1.10.1 +-e certbot[dev] diff --git a/certbot-nginx/setup.py b/certbot-nginx/setup.py index 96d8f7f1b..7be74fa16 100644 --- a/certbot-nginx/setup.py +++ b/certbot-nginx/setup.py @@ -7,7 +7,7 @@ version = '1.16.0.dev0' # acme/certbot version. install_requires = [ 'acme>=1.8.0', - 'certbot>=1.10.1', + 'certbot>=1.16.0.dev0', 'PyOpenSSL>=17.3.0', 'pyparsing>=2.2.0', 'setuptools>=39.0.1', diff --git a/certbot-nginx/tests/test_util.py b/certbot-nginx/tests/test_util.py index 383a15753..97fe05af0 100644 --- a/certbot-nginx/tests/test_util.py +++ b/certbot-nginx/tests/test_util.py @@ -9,7 +9,6 @@ try: except ImportError: # pragma: no cover from unittest import mock # type: ignore import pkg_resources -import zope.component from certbot import util from certbot.compat import os @@ -79,9 +78,6 @@ class NginxTest(test_util.ConfigTestCase): openssl_version=openssl_version) config.prepare() - # Provide general config utility. - zope.component.provideUtility(self.configuration) - return config diff --git a/certbot/CHANGELOG.md b/certbot/CHANGELOG.md index b22576b2f..d23a88c94 100644 --- a/certbot/CHANGELOG.md +++ b/certbot/CHANGELOG.md @@ -19,6 +19,10 @@ Certbot adheres to [Semantic Versioning](https://semver.org/). and clarify output. If you would like to see more verbose output, use the -v or -vv flags. UX improvements are an iterative process and the Certbot team welcomes constructive feedback. +* Functions `certbot.crypto_util.init_save_key` and `certbot.crypto_util.init_save_csr`, + whose behaviors rely on the global Certbot `config` singleton, are deprecated and will + be removed in a future release. Please use `certbot.crypto_util.generate_key` and + `certbot.crypto_util.generate_csr` instead. ### Fixed diff --git a/certbot/certbot/_internal/auth_handler.py b/certbot/certbot/_internal/auth_handler.py index 036cc91e3..41ae492e2 100644 --- a/certbot/certbot/_internal/auth_handler.py +++ b/certbot/certbot/_internal/auth_handler.py @@ -44,11 +44,12 @@ class AuthHandler: self.account = account self.pref_challs = pref_challs - def handle_authorizations(self, orderr, best_effort=False, max_retries=30): + def handle_authorizations(self, orderr, config, best_effort=False, max_retries=30): """ Retrieve all authorizations, perform all challenges required to validate these authorizations, then poll and wait for the authorization to be checked. :param acme.messages.OrderResource orderr: must have authorizations filled in + :param interfaces.IConfig config: current Certbot configuration :param bool best_effort: if True, not all authorizations need to be validated (eg. renew) :param int max_retries: maximum number of retries to poll authorizations :returns: list of all validated authorizations @@ -72,7 +73,6 @@ class AuthHandler: resps = self.auth.perform(achalls) # If debug is on, wait for user input before starting the verification process. - config = zope.component.getUtility(interfaces.IConfig) if config.debug_challenges: notify = zope.component.getUtility(interfaces.IDisplay).notification notify('Challenges loaded. Press continue to submit to CA. ' diff --git a/certbot/certbot/_internal/client.py b/certbot/certbot/_internal/client.py index c9a240de3..5c0bed220 100644 --- a/certbot/certbot/_internal/client.py +++ b/certbot/certbot/_internal/client.py @@ -334,7 +334,7 @@ class Client: key = None key_size = self.config.rsa_key_size - elliptic_curve = None + elliptic_curve = "secp256r1" # key-type defaults to a list, but we are only handling 1 currently if isinstance(self.config.key_type, list): @@ -362,13 +362,15 @@ class Client: data=acme_crypto_util.make_csr( key.pem, domains, self.config.must_staple)) else: - key = key or crypto_util.init_save_key( + key = key or crypto_util.generate_key( key_size=key_size, key_dir=self.config.key_dir, key_type=self.config.key_type, elliptic_curve=elliptic_curve, + strict_permissions=self.config.strict_permissions, ) - csr = crypto_util.init_save_csr(key, domains, self.config.csr_dir) + csr = crypto_util.generate_csr(key, domains, self.config.csr_dir, + self.config.must_staple, self.config.strict_permissions) orderr = self._get_order_and_authorizations(csr.data, self.config.allow_subset_of_names) authzr = orderr.authorizations @@ -420,7 +422,7 @@ class Client: logger.warning("Certbot was unable to obtain fresh authorizations for every domain" ". The dry run will continue, but results may not be accurate.") - authzr = self.auth_handler.handle_authorizations(orderr, best_effort) + authzr = self.auth_handler.handle_authorizations(orderr, self.config, best_effort) return orderr.update(authorizations=authzr) def obtain_and_enroll_certificate(self, domains, certname): diff --git a/certbot/certbot/_internal/main.py b/certbot/certbot/_internal/main.py index 4c6dfac5c..2100a2943 100644 --- a/certbot/certbot/_internal/main.py +++ b/certbot/certbot/_internal/main.py @@ -1525,6 +1525,9 @@ def main(cli_args=None): # note: arg parser internally handles --help (and exits afterwards) args = cli.prepare_and_parse_args(plugins, cli_args) config = configuration.NamespaceConfig(args) + + # This call is done only for retro-compatibility purposes. + # TODO: Remove this call once zope dependencies are removed from Certbot. zope.component.provideUtility(config) # On windows, shell without administrative right cannot create symlinks required by certbot. diff --git a/certbot/certbot/_internal/renewal.py b/certbot/certbot/_internal/renewal.py index acb64d7e2..b4fa6aa01 100644 --- a/certbot/certbot/_internal/renewal.py +++ b/certbot/certbot/_internal/renewal.py @@ -450,7 +450,8 @@ def handle_renewal_request(config): if renewal_candidate is None: parse_failures.append(renewal_file) else: - # XXX: ensure that each call here replaces the previous one + # This call is done only for retro-compatibility purposes. + # TODO: Remove this call once zope dependencies are removed from Certbot. zope.component.provideUtility(lineage_config) renewal_candidate.ensure_deployed() from certbot._internal import main diff --git a/certbot/certbot/crypto_util.py b/certbot/certbot/crypto_util.py index 51ef2c53a..5cb21b510 100644 --- a/certbot/certbot/crypto_util.py +++ b/certbot/certbot/crypto_util.py @@ -9,7 +9,7 @@ import logging import re import warnings -from typing import List +from typing import List, Set # See https://github.com/pyca/cryptography/issues/4275 from cryptography import x509 # type: ignore from cryptography.exceptions import InvalidSignature @@ -38,9 +38,10 @@ logger = logging.getLogger(__name__) # High level functions -def init_save_key( - key_size, key_dir, key_type="rsa", elliptic_curve="secp256r1", keyname="key-certbot.pem" -): + +def generate_key(key_size: int, key_dir: str, key_type: str = "rsa", + elliptic_curve: str = "secp256r1", keyname: str = "key-certbot.pem", + strict_permissions: bool = True) -> util.Key: """Initializes and saves a privkey. Inits key and saves it in PEM format on the filesystem. @@ -53,6 +54,8 @@ def init_save_key( :param str key_type: Key Type [rsa, ecdsa] :param str elliptic_curve: Name of the elliptic curve if key type is ecdsa. :param str keyname: Filename of key + :param bool strict_permissions: If true and key_dir exists, an exception is raised if + the directory doesn't have 0700 permissions or isn't owned by the current user. :returns: Key :rtype: :class:`certbot.util.Key` @@ -69,9 +72,8 @@ def init_save_key( logger.error("Encountered error while making key: %s", str(err)) raise err - config = zope.component.getUtility(interfaces.IConfig) # Save file - util.make_or_verify_dir(key_dir, 0o700, config.strict_permissions) + util.make_or_verify_dir(key_dir, 0o700, strict_permissions) key_f, key_path = util.unique_file( os.path.join(key_dir, keyname), 0o600, "wb") with key_f: @@ -84,9 +86,77 @@ def init_save_key( return util.Key(key_path, key_pem) +# TODO: Remove this call once zope dependencies are removed from Certbot. +def init_save_key(key_size, key_dir, key_type="rsa", elliptic_curve="secp256r1", + keyname="key-certbot.pem"): + """Initializes and saves a privkey. + + Inits key and saves it in PEM format on the filesystem. + + .. note:: keyname is the attempted filename, it may be different if a file + already exists at the path. + + .. deprecated:: 1.16.0 + Use :func:`generate_key` instead. + + :param int key_size: key size in bits if key size is rsa. + :param str key_dir: Key save directory. + :param str key_type: Key Type [rsa, ecdsa] + :param str elliptic_curve: Name of the elliptic curve if key type is ecdsa. + :param str keyname: Filename of key + + :returns: Key + :rtype: :class:`certbot.util.Key` + + :raises ValueError: If unable to generate the key given key_size. + + """ + warnings.warn("certbot.crypto_util.init_save_key is deprecated, please use " + "certbot.crypto_util.generate_key instead.", DeprecationWarning) + + config = zope.component.getUtility(interfaces.IConfig) + + return generate_key(key_size, key_dir, key_type=key_type, elliptic_curve=elliptic_curve, + keyname=keyname, strict_permissions=config.strict_permissions) + + +def generate_csr(privkey: util.Key, names: Set[str], path: str, + must_staple: bool = False, strict_permissions: bool = True) -> util.CSR: + """Initialize a CSR with the given private key. + + :param privkey: Key to include in the CSR + :type privkey: :class:`certbot.util.Key` + :param set names: `str` names to include in the CSR + :param str path: Certificate save directory. + :param bool must_staple: If true, include the TLS Feature extension "OCSP Must Staple" + :param bool strict_permissions: If true and path exists, an exception is raised if + the directory doesn't have 0755 permissions or isn't owned by the current user. + + :returns: CSR + :rtype: :class:`certbot.util.CSR` + + """ + csr_pem = acme_crypto_util.make_csr( + privkey.pem, names, must_staple=must_staple) + + # Save CSR + util.make_or_verify_dir(path, 0o755, strict_permissions) + csr_f, csr_filename = util.unique_file( + os.path.join(path, "csr-certbot.pem"), 0o644, "wb") + with csr_f: + csr_f.write(csr_pem) + logger.debug("Creating CSR: %s", csr_filename) + + return util.CSR(csr_filename, csr_pem, "pem") + + +# TODO: Remove this call once zope dependencies are removed from Certbot. def init_save_csr(privkey, names, path): """Initialize a CSR with the given private key. + .. deprecated:: 1.16.0 + Use :func:`generate_csr` instead. + :param privkey: Key to include in the CSR :type privkey: :class:`certbot.util.Key` @@ -98,20 +168,13 @@ def init_save_csr(privkey, names, path): :rtype: :class:`certbot.util.CSR` """ + warnings.warn("certbot.crypto_util.init_save_csr is deprecated, please use " + "certbot.crypto_util.generate_csr instead.", DeprecationWarning) + config = zope.component.getUtility(interfaces.IConfig) - csr_pem = acme_crypto_util.make_csr( - privkey.pem, names, must_staple=config.must_staple) - - # Save CSR - util.make_or_verify_dir(path, 0o755, config.strict_permissions) - csr_f, csr_filename = util.unique_file( - os.path.join(path, "csr-certbot.pem"), 0o644, "wb") - with csr_f: - csr_f.write(csr_pem) - logger.debug("Creating CSR: %s", csr_filename) - - return util.CSR(csr_filename, csr_pem, "pem") + return generate_csr(privkey, names, path, must_staple=config.must_staple, + strict_permissions=config.strict_permissions) # WARNING: the csr and private key file are possible attack vectors for TOCTOU diff --git a/certbot/tests/auth_handler_test.py b/certbot/tests/auth_handler_test.py index 751c445fe..25b19def7 100644 --- a/certbot/tests/auth_handler_test.py +++ b/certbot/tests/auth_handler_test.py @@ -69,10 +69,9 @@ class HandleAuthorizationsTest(unittest.TestCase): from certbot._internal.auth_handler import AuthHandler self.mock_display = mock.Mock() + self.mock_config = mock.Mock(debug_challenges=False) zope.component.provideUtility( self.mock_display, interfaces.IDisplay) - zope.component.provideUtility( - mock.Mock(debug_challenges=False), interfaces.IConfig) self.mock_auth = mock.MagicMock(name="ApacheConfigurator") @@ -99,7 +98,7 @@ class HandleAuthorizationsTest(unittest.TestCase): self.mock_net.poll.side_effect = _gen_mock_on_poll(retry=1, wait_value=30) with mock.patch('certbot._internal.auth_handler.time') as mock_time: - authzr = self.handler.handle_authorizations(mock_order) + authzr = self.handler.handle_authorizations(mock_order, self.mock_config) self.assertEqual(self.mock_net.answer_challenge.call_count, 1) @@ -131,7 +130,7 @@ class HandleAuthorizationsTest(unittest.TestCase): authzr = gen_dom_authzr(domain="0", challs=acme_util.CHALLENGES, combos=False) mock_order = mock.MagicMock(authorizations=[authzr]) - authzr = self.handler.handle_authorizations(mock_order) + authzr = self.handler.handle_authorizations(mock_order, self.mock_config) self.assertEqual(self.mock_net.answer_challenge.call_count, 2) @@ -152,7 +151,7 @@ class HandleAuthorizationsTest(unittest.TestCase): authzr = gen_dom_authzr(domain="0", challs=acme_util.CHALLENGES, combos=False) mock_order = mock.MagicMock(authorizations=[authzr]) - authzr = self.handler.handle_authorizations(mock_order) + authzr = self.handler.handle_authorizations(mock_order, self.mock_config) self.assertEqual(self.mock_net.answer_challenge.call_count, 1) @@ -176,7 +175,7 @@ class HandleAuthorizationsTest(unittest.TestCase): mock_order = mock.MagicMock(authorizations=authzrs) self.mock_net.poll.side_effect = _gen_mock_on_poll() - authzr = self.handler.handle_authorizations(mock_order) + authzr = self.handler.handle_authorizations(mock_order, self.mock_config) self.assertEqual(self.mock_net.answer_challenge.call_count, 3) @@ -195,14 +194,13 @@ class HandleAuthorizationsTest(unittest.TestCase): self._test_name3_http_01_3_common(combos=False) def test_debug_challenges(self): - zope.component.provideUtility( - mock.Mock(debug_challenges=True), interfaces.IConfig) + config = mock.Mock(debug_challenges=True) authzrs = [gen_dom_authzr(domain="0", challs=acme_util.CHALLENGES)] mock_order = mock.MagicMock(authorizations=authzrs) self.mock_net.poll.side_effect = _gen_mock_on_poll() - self.handler.handle_authorizations(mock_order) + self.handler.handle_authorizations(mock_order, config) self.assertEqual(self.mock_net.answer_challenge.call_count, 1) self.assertEqual(self.mock_display.notification.call_count, 1) @@ -214,7 +212,8 @@ class HandleAuthorizationsTest(unittest.TestCase): self.mock_auth.perform.side_effect = errors.AuthorizationError self.assertRaises( - errors.AuthorizationError, self.handler.handle_authorizations, mock_order) + errors.AuthorizationError, self.handler.handle_authorizations, + mock_order, self.mock_config) def test_max_retries_exceeded(self): authzrs = [gen_dom_authzr(domain="0", challs=acme_util.CHALLENGES)] @@ -225,12 +224,13 @@ class HandleAuthorizationsTest(unittest.TestCase): with self.assertRaises(errors.AuthorizationError) as error: # We retry only once, so retries will be exhausted before STATUS_VALID is returned. - self.handler.handle_authorizations(mock_order, False, 1) + self.handler.handle_authorizations(mock_order, self.mock_config, False, 1) self.assertIn('All authorizations were not finalized by the CA.', str(error.exception)) def test_no_domains(self): mock_order = mock.MagicMock(authorizations=[]) - self.assertRaises(errors.AuthorizationError, self.handler.handle_authorizations, mock_order) + self.assertRaises(errors.AuthorizationError, self.handler.handle_authorizations, + mock_order, self.mock_config) def _test_preferred_challenge_choice_common(self, combos): authzrs = [gen_dom_authzr(domain="0", challs=acme_util.CHALLENGES, combos=combos)] @@ -242,7 +242,7 @@ class HandleAuthorizationsTest(unittest.TestCase): challenges.DNS01.typ,)) self.mock_net.poll.side_effect = _gen_mock_on_poll() - self.handler.handle_authorizations(mock_order) + self.handler.handle_authorizations(mock_order, self.mock_config) self.assertEqual(self.mock_auth.cleanup.call_count, 1) self.assertEqual( @@ -260,7 +260,8 @@ class HandleAuthorizationsTest(unittest.TestCase): mock_order = mock.MagicMock(authorizations=authzrs) self.handler.pref_challs.append(challenges.DNS01.typ) self.assertRaises( - errors.AuthorizationError, self.handler.handle_authorizations, mock_order) + errors.AuthorizationError, self.handler.handle_authorizations, + mock_order, self.mock_config) def test_preferred_challenges_not_supported_acme_1(self): self._test_preferred_challenges_not_supported_common(combos=True) @@ -273,14 +274,16 @@ class HandleAuthorizationsTest(unittest.TestCase): authzrs = [gen_dom_authzr(domain="0", challs=[acme_util.DNS01])] mock_order = mock.MagicMock(authorizations=authzrs) self.assertRaises( - errors.AuthorizationError, self.handler.handle_authorizations, mock_order) + errors.AuthorizationError, self.handler.handle_authorizations, + mock_order, self.mock_config) def test_perform_error(self): self.mock_auth.perform.side_effect = errors.AuthorizationError authzr = gen_dom_authzr(domain="0", challs=acme_util.CHALLENGES, combos=True) mock_order = mock.MagicMock(authorizations=[authzr]) - self.assertRaises(errors.AuthorizationError, self.handler.handle_authorizations, mock_order) + self.assertRaises(errors.AuthorizationError, self.handler.handle_authorizations, + mock_order, self.mock_config) self.assertEqual(self.mock_auth.cleanup.call_count, 1) self.assertEqual( @@ -293,7 +296,8 @@ class HandleAuthorizationsTest(unittest.TestCase): mock_order = mock.MagicMock(authorizations=authzrs) self.assertRaises( - errors.AuthorizationError, self.handler.handle_authorizations, mock_order) + errors.AuthorizationError, self.handler.handle_authorizations, + mock_order, self.mock_config) self.assertEqual(self.mock_auth.cleanup.call_count, 1) self.assertEqual( self.mock_auth.cleanup.call_args[0][0][0].typ, "http-01") @@ -305,7 +309,7 @@ class HandleAuthorizationsTest(unittest.TestCase): with test_util.patch_get_utility(): with self.assertRaises(errors.AuthorizationError) as error: - self.handler.handle_authorizations(mock_order, False) + self.handler.handle_authorizations(mock_order, self.mock_config, False) self.assertIn('Some challenges have failed.', str(error.exception)) self.assertEqual(self.mock_auth.cleanup.call_count, 1) self.assertEqual( @@ -330,7 +334,7 @@ class HandleAuthorizationsTest(unittest.TestCase): with mock.patch('certbot._internal.auth_handler.AuthHandler._report_failed_authzrs') \ as mock_report: - valid_authzr = self.handler.handle_authorizations(mock_order, True) + valid_authzr = self.handler.handle_authorizations(mock_order, self.mock_config, True) # Because best_effort=True, we did not blow up. Instead ... self.assertEqual(len(valid_authzr), 1) # ... the valid authzr has been processed @@ -340,7 +344,7 @@ class HandleAuthorizationsTest(unittest.TestCase): with test_util.patch_get_utility(): with self.assertRaises(errors.AuthorizationError) as error: - self.handler.handle_authorizations(mock_order, True) + self.handler.handle_authorizations(mock_order, self.mock_config, True) # Despite best_effort=True, process will fail because no authzr is valid. self.assertIn('All challenges have failed.', str(error.exception)) @@ -354,7 +358,8 @@ class HandleAuthorizationsTest(unittest.TestCase): [messages.STATUS_PENDING], False) mock_order = mock.MagicMock(authorizations=[authzr]) self.assertRaises( - errors.AuthorizationError, self.handler.handle_authorizations, mock_order) + errors.AuthorizationError, self.handler.handle_authorizations, + mock_order, self.mock_config) # With a validated challenge that is not supported by the plugin, we # expect the challenge to not be solved again and @@ -364,7 +369,7 @@ class HandleAuthorizationsTest(unittest.TestCase): [acme_util.DNS01], [messages.STATUS_VALID], False) mock_order = mock.MagicMock(authorizations=[authzr]) - self.handler.handle_authorizations(mock_order) + self.handler.handle_authorizations(mock_order, self.mock_config) def test_valid_authzrs_deactivated(self): """When we deactivate valid authzrs in an orderr, we expect them to become deactivated diff --git a/certbot/tests/client_test.py b/certbot/tests/client_test.py index 8d9811fa3..51c6767f6 100644 --- a/certbot/tests/client_test.py +++ b/certbot/tests/client_test.py @@ -242,6 +242,7 @@ class ClientTest(ClientTestCommon): self.config.allow_subset_of_names = False self.config.dry_run = False + self.config.strict_permissions = True self.eg_domains = ["example.com", "www.example.com"] self.eg_order = mock.MagicMock( authorizations=[None], @@ -263,6 +264,7 @@ class ClientTest(ClientTestCommon): if auth_count == 1: self.client.auth_handler.handle_authorizations.assert_called_once_with( self.eg_order, + self.config, self.config.allow_subset_of_names) else: self.assertEqual(self.client.auth_handler.handle_authorizations.call_count, auth_count) @@ -273,16 +275,14 @@ class ClientTest(ClientTestCommon): @mock.patch("certbot._internal.client.crypto_util") @mock.patch("certbot._internal.client.logger") - @test_util.patch_get_utility() - def test_obtain_certificate_from_csr(self, unused_mock_get_utility, - mock_logger, mock_crypto_util): + def test_obtain_certificate_from_csr(self, mock_logger, mock_crypto_util): self._mock_obtain_certificate() test_csr = util.CSR(form="pem", file=None, data=CSR_SAN) auth_handler = self.client.auth_handler self._set_mock_from_fullchain(mock_crypto_util.cert_and_chain_from_fullchain) orderr = self.acme.new_order(test_csr.data) - auth_handler.handle_authorizations(orderr, False) + auth_handler.handle_authorizations(orderr, self.config, False) self.assertEqual( (mock.sentinel.cert, mock.sentinel.chain), self.client.obtain_certificate_from_csr( @@ -310,7 +310,7 @@ class ClientTest(ClientTestCommon): self.client.obtain_certificate_from_csr( test_csr, orderr=None)) - auth_handler.handle_authorizations.assert_called_with(self.eg_order, False) + auth_handler.handle_authorizations.assert_called_with(self.eg_order, self.config, False) # Test for no auth_handler self.client.auth_handler = None @@ -323,20 +323,21 @@ class ClientTest(ClientTestCommon): @mock.patch("certbot._internal.client.crypto_util") def test_obtain_certificate(self, mock_crypto_util): csr = util.CSR(form="pem", file=None, data=CSR_SAN) - mock_crypto_util.init_save_csr.return_value = csr - mock_crypto_util.init_save_key.return_value = mock.sentinel.key + mock_crypto_util.generate_csr.return_value = csr + mock_crypto_util.generate_key.return_value = mock.sentinel.key self._set_mock_from_fullchain(mock_crypto_util.cert_and_chain_from_fullchain) self._test_obtain_certificate_common(mock.sentinel.key, csr) - mock_crypto_util.init_save_key.assert_called_once_with( + mock_crypto_util.generate_key.assert_called_once_with( key_size=self.config.rsa_key_size, key_dir=self.config.key_dir, key_type=self.config.key_type, - elliptic_curve=None, # elliptic curve is not set + elliptic_curve="secp256r1", + strict_permissions=True, ) - mock_crypto_util.init_save_csr.assert_called_once_with( - mock.sentinel.key, self.eg_domains, self.config.csr_dir) + mock_crypto_util.generate_csr.assert_called_once_with( + mock.sentinel.key, self.eg_domains, self.config.csr_dir, False, True) mock_crypto_util.cert_and_chain_from_fullchain.assert_called_once_with( self.eg_order.fullchain_pem) @@ -345,16 +346,16 @@ class ClientTest(ClientTestCommon): def test_obtain_certificate_partial_success(self, mock_remove, mock_crypto_util): csr = util.CSR(form="pem", file=mock.sentinel.csr_file, data=CSR_SAN) key = util.CSR(form="pem", file=mock.sentinel.key_file, data=CSR_SAN) - mock_crypto_util.init_save_csr.return_value = csr - mock_crypto_util.init_save_key.return_value = key + mock_crypto_util.generate_csr.return_value = csr + mock_crypto_util.generate_key.return_value = key self._set_mock_from_fullchain(mock_crypto_util.cert_and_chain_from_fullchain) authzr = self._authzr_from_domains(["example.com"]) self.config.allow_subset_of_names = True self._test_obtain_certificate_common(key, csr, authzr_ret=authzr, auth_count=2) - self.assertEqual(mock_crypto_util.init_save_key.call_count, 2) - self.assertEqual(mock_crypto_util.init_save_csr.call_count, 2) + self.assertEqual(mock_crypto_util.generate_key.call_count, 2) + self.assertEqual(mock_crypto_util.generate_csr.call_count, 2) self.assertEqual(mock_remove.call_count, 2) self.assertEqual(mock_crypto_util.cert_and_chain_from_fullchain.call_count, 1) @@ -372,13 +373,13 @@ class ClientTest(ClientTestCommon): mock_crypto.make_key.assert_called_once_with( bits=self.config.rsa_key_size, - elliptic_curve=None, # not making an elliptic private key + elliptic_curve="secp256r1", key_type=self.config.key_type, ) mock_acme_crypto.make_csr.assert_called_once_with( mock.sentinel.key_pem, self.eg_domains, self.config.must_staple) - mock_crypto.init_save_key.assert_not_called() - mock_crypto.init_save_csr.assert_not_called() + mock_crypto.generate_key.assert_not_called() + mock_crypto.generate_csr.assert_not_called() self.assertEqual(mock_crypto.cert_and_chain_from_fullchain.call_count, 1) @mock.patch("certbot._internal.client.logger") diff --git a/certbot/tests/crypto_util_test.py b/certbot/tests/crypto_util_test.py index 34ad4b09a..858db079c 100644 --- a/certbot/tests/crypto_util_test.py +++ b/certbot/tests/crypto_util_test.py @@ -2,15 +2,15 @@ import logging import unittest +import certbot.util + try: import mock except ImportError: # pragma: no cover from unittest import mock import OpenSSL -import zope.component from certbot import errors -from certbot import interfaces from certbot import util from certbot.compat import filesystem from certbot.compat import os @@ -33,8 +33,8 @@ CERT_ISSUER = test_util.load_vector('cert_intermediate_1.pem') CERT_ALT_ISSUER = test_util.load_vector('cert_intermediate_2.pem') -class InitSaveKeyTest(test_util.TempDirTestCase): - """Tests for certbot.crypto_util.init_save_key.""" +class GenerateKeyTest(test_util.TempDirTestCase): + """Tests for certbot.crypto_util.generate_key.""" def setUp(self): super().setUp() @@ -42,8 +42,6 @@ class InitSaveKeyTest(test_util.TempDirTestCase): filesystem.mkdir(self.workdir, mode=0o700) logging.disable(logging.CRITICAL) - zope.component.provideUtility( - mock.Mock(strict_permissions=True), interfaces.IConfig) def tearDown(self): super().tearDown() @@ -52,8 +50,8 @@ class InitSaveKeyTest(test_util.TempDirTestCase): @classmethod def _call(cls, key_size, key_dir): - from certbot.crypto_util import init_save_key - return init_save_key(key_size, key_dir, 'key-certbot.pem') + from certbot.crypto_util import generate_key + return generate_key(key_size, key_dir, 'key-certbot.pem', strict_permissions=True) @mock.patch('certbot.crypto_util.make_key') def test_success(self, mock_make): @@ -69,29 +67,57 @@ class InitSaveKeyTest(test_util.TempDirTestCase): self.assertRaises(ValueError, self._call, 431, self.workdir) -class InitSaveCSRTest(test_util.TempDirTestCase): - """Tests for certbot.crypto_util.init_save_csr.""" +class InitSaveKey(unittest.TestCase): + """Test for certbot.crypto_util.init_save_key.""" + @mock.patch("certbot.crypto_util.generate_key") + @mock.patch("certbot.crypto_util.zope.component") + def test_it(self, mock_zope, mock_generate): + from certbot.crypto_util import init_save_key - def setUp(self): - super().setUp() + mock_zope.getUtility.return_value = mock.MagicMock(strict_permissions=True) - zope.component.provideUtility( - mock.Mock(strict_permissions=True), interfaces.IConfig) + with self.assertWarns(DeprecationWarning): + init_save_key(4096, "/some/path") + mock_generate.assert_called_with(4096, "/some/path", elliptic_curve="secp256r1", + key_type="rsa", keyname="key-certbot.pem", + strict_permissions=True) + + +class GenerateCSRTest(test_util.TempDirTestCase): + """Tests for certbot.crypto_util.generate_csr.""" @mock.patch('acme.crypto_util.make_csr') @mock.patch('certbot.crypto_util.util.make_or_verify_dir') def test_it(self, unused_mock_verify, mock_csr): - from certbot.crypto_util import init_save_csr + from certbot.crypto_util import generate_csr mock_csr.return_value = b'csr_pem' - csr = init_save_csr( - mock.Mock(pem='dummy_key'), 'example.com', self.tempdir) + csr = generate_csr( + mock.Mock(pem='dummy_key'), 'example.com', self.tempdir, strict_permissions=True) self.assertEqual(csr.data, b'csr_pem') self.assertIn('csr-certbot.pem', csr.file) +class InitSaveCsr(unittest.TestCase): + """Tests for certbot.crypto_util.init_save_csr.""" + @mock.patch("certbot.crypto_util.generate_csr") + @mock.patch("certbot.crypto_util.zope.component") + def test_it(self, mock_zope, mock_generate): + from certbot.crypto_util import init_save_csr + + mock_zope.getUtility.return_value = mock.MagicMock(must_staple=True, + strict_permissions=True) + key = certbot.util.Key(file=None, pem=None) + + with self.assertWarns(DeprecationWarning): + init_save_csr(key, {"dummy"}, "/some/path") + + mock_generate.assert_called_with(key, {"dummy"}, "/some/path", + must_staple=True, strict_permissions=True) + + class ValidCSRTest(unittest.TestCase): """Tests for certbot.crypto_util.valid_csr."""