From 08d91b456be1759a1c61602c590766d39b61105a Mon Sep 17 00:00:00 2001 From: alexzorin Date: Tue, 5 Nov 2019 07:23:25 +1100 Subject: [PATCH] Use fresh authorizations in dry runs (#7442) * acme: re-populate uri in deactivate_authorization * Use fresh authorizations in dry runs --dry-run now deactivates 'valid' authorizations if it encounters them when creating a new order. Resolves #5116. * remove unused code * typo in local-oldest-requirements * better error handling * certbot-ci: AUTHREUSE to 100 + unskip dry-run test * improve test coverage for error cases * restore newline to local-oldest-requirements.txt --- CHANGELOG.md | 2 + acme/acme/client.py | 3 +- .../certbot_tests/test_main.py | 18 ++++++++ .../utils/acme_server.py | 1 + certbot/auth_handler.py | 28 ++++++++++++- certbot/client.py | 14 ++++++- certbot/tests/auth_handler_test.py | 34 +++++++++++++++ certbot/tests/client_test.py | 42 +++++++++++++++++++ local-oldest-requirements.txt | 2 +- setup.py | 2 +- 10 files changed, 141 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 13f425209..4a71d24ad 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,8 @@ Certbot adheres to [Semantic Versioning](https://semver.org/). systems will be asked to do this manually. * `--server` may now be combined with `--dry-run`. Certbot will, as before, use the staging server instead of the live server when `--dry-run` is used. +* `--dry-run` now requests fresh authorizations every time, fixing the issue + where it was prone to falsely reporting success. * Updated certbot-dns-google to depend on newer versions of google-api-python-client and oauth2client. * The OS detection logic again uses distro library for Linux OSes diff --git a/acme/acme/client.py b/acme/acme/client.py index 9698c7609..d2002576e 100644 --- a/acme/acme/client.py +++ b/acme/acme/client.py @@ -136,7 +136,8 @@ class ClientBase(object): # pylint: disable=too-many-instance-attributes """ body = messages.UpdateAuthorization(status='deactivated') response = self._post(authzr.uri, body) - return self._authzr_from_response(response) + return self._authzr_from_response(response, + authzr.body.identifier, authzr.uri) def _authzr_from_response(self, response, identifier=None, uri=None): authzr = messages.AuthorizationResource( diff --git a/certbot-ci/certbot_integration_tests/certbot_tests/test_main.py b/certbot-ci/certbot_integration_tests/certbot_tests/test_main.py index 80fa15584..50c685a57 100644 --- a/certbot-ci/certbot_integration_tests/certbot_tests/test_main.py +++ b/certbot-ci/certbot_integration_tests/certbot_tests/test_main.py @@ -594,3 +594,21 @@ def test_ocsp_status_live(context): assert output.count('INVALID') == 1, 'Expected {0} to be INVALID'.format(cert) assert output.count('REVOKED') == 1, 'Expected {0} to be REVOKED'.format(cert) + + +def test_dry_run_deactivate_authzs(context): + """Test that Certbot deactivates authorizations when performing a dry run""" + + name = context.get_domain('dry-run-authz-deactivation') + args = ['certonly', '--cert-name', name, '-d', name, '--dry-run'] + log_line = 'Recreating order after authz deactivation' + + # First order will not need deactivation + context.certbot(args) + with open(join(context.workspace, 'logs', 'letsencrypt.log'), 'r') as f: + assert log_line not in f.read(), 'First order should not have had any authz reuse' + + # Second order will require deactivation + context.certbot(args) + with open(join(context.workspace, 'logs', 'letsencrypt.log'), 'r') as f: + assert log_line in f.read(), 'Second order should have been recreated due to authz reuse' diff --git a/certbot-ci/certbot_integration_tests/utils/acme_server.py b/certbot-ci/certbot_integration_tests/utils/acme_server.py index f13855d1e..9e7ead916 100755 --- a/certbot-ci/certbot_integration_tests/utils/acme_server.py +++ b/certbot-ci/certbot_integration_tests/utils/acme_server.py @@ -125,6 +125,7 @@ class ACMEServer(object): environ = os.environ.copy() environ['PEBBLE_VA_NOSLEEP'] = '1' environ['PEBBLE_WFE_NONCEREJECT'] = '0' + environ['PEBBLE_AUTHZREUSE'] = '100' self._launch_process( [pebble_path, '-config', pebble_config_path, '-dnsserver', '127.0.0.1:8053'], diff --git a/certbot/auth_handler.py b/certbot/auth_handler.py index 232b246d5..a8c8f6e9b 100644 --- a/certbot/auth_handler.py +++ b/certbot/auth_handler.py @@ -7,8 +7,9 @@ import zope.component from acme import challenges from acme import messages +from acme import errors as acme_errors # pylint: disable=unused-import, no-name-in-module -from acme.magic_typing import Dict, List +from acme.magic_typing import Dict, List, Tuple # pylint: enable=unused-import, no-name-in-module from certbot import achallenges from certbot import errors @@ -97,6 +98,31 @@ class AuthHandler(object): return authzrs_validated + def deactivate_valid_authorizations(self, orderr): + # type: (messages.OrderResource) -> Tuple[List, List] + """ + Deactivate all `valid` authorizations in the order, so that they cannot be re-used + in subsequent orders. + :param messages.OrderResource orderr: must have authorizations filled in + :returns: tuple of list of successfully deactivated authorizations, and + list of unsuccessfully deactivated authorizations. + :rtype: tuple + """ + to_deactivate = [authzr for authzr in orderr.authorizations + if authzr.body.status == messages.STATUS_VALID] + deactivated = [] + failed = [] + + for authzr in to_deactivate: + try: + authzr = self.acme.deactivate_authorization(authzr) + deactivated.append(authzr) + except acme_errors.Error as e: + failed.append(authzr) + logger.debug('Failed to deactivate authorization %s: %s', authzr.uri, e) + + return (deactivated, failed) + def _poll_authorizations(self, authzrs, max_retries, best_effort): """ Poll the ACME CA server, to wait for confirmation that authorizations have their challenges diff --git a/certbot/client.py b/certbot/client.py index c1199daac..1800a301c 100644 --- a/certbot/client.py +++ b/certbot/client.py @@ -15,7 +15,7 @@ from acme import client as acme_client from acme import crypto_util as acme_crypto_util from acme import errors as acme_errors from acme import messages -from acme.magic_typing import Optional # pylint: disable=unused-import,no-name-in-module +from acme.magic_typing import Optional, List # pylint: disable=unused-import,no-name-in-module import certbot from certbot import account @@ -366,6 +366,7 @@ class Client(object): return cert, chain, key, csr def _get_order_and_authorizations(self, csr_pem, best_effort): + # type: (str, bool) -> List[messages.OrderResource] """Request a new order and complete its authorizations. :param str csr_pem: A CSR in PEM format. @@ -381,6 +382,17 @@ class Client(object): except acme_errors.WildcardUnsupportedError: raise errors.Error("The currently selected ACME CA endpoint does" " not support issuing wildcard certificates.") + + # For a dry run, ensure we have an order with fresh authorizations + if orderr and self.config.dry_run: + deactivated, failed = self.auth_handler.deactivate_valid_authorizations(orderr) + if deactivated: + logger.debug("Recreating order after authz deactivations") + orderr = self.acme.new_order(csr_pem) + if failed: + 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) return orderr.update(authorizations=authzr) diff --git a/certbot/tests/auth_handler_test.py b/certbot/tests/auth_handler_test.py index 45eef8e2b..9f58b95d3 100644 --- a/certbot/tests/auth_handler_test.py +++ b/certbot/tests/auth_handler_test.py @@ -9,6 +9,7 @@ import zope.component from acme import challenges from acme import client as acme_client from acme import messages +from acme import errors as acme_errors from certbot import achallenges from certbot import errors @@ -362,6 +363,39 @@ class HandleAuthorizationsTest(unittest.TestCase): # pylint: disable=too-many-p mock_order = mock.MagicMock(authorizations=[authzr]) self.handler.handle_authorizations(mock_order) + def test_valid_authzrs_deactivated(self): + """When we deactivate valid authzrs in an orderr, we expect them to become deactivated + and to receive a list of deactivated authzrs in return.""" + def _mock_deactivate(authzr): + if authzr.body.status == messages.STATUS_VALID: + if authzr.body.identifier.value == "is_valid_but_will_fail": + raise acme_errors.Error("Mock deactivation ACME error") + authzb = authzr.body.update(status=messages.STATUS_DEACTIVATED) + authzr = messages.AuthorizationResource(body=authzb) + else: # pragma: no cover + raise errors.Error("Can't deactivate non-valid authz") + return authzr + + to_deactivate = [("is_valid", messages.STATUS_VALID), + ("is_pending", messages.STATUS_PENDING), + ("is_valid_but_will_fail", messages.STATUS_VALID)] + + to_deactivate = [acme_util.gen_authzr(a[1], a[0], [acme_util.HTTP01], + [a[1], False]) for a in to_deactivate] + orderr = mock.MagicMock(authorizations=to_deactivate) + + self.mock_net.deactivate_authorization.side_effect = _mock_deactivate + + authzrs, failed = self.handler.deactivate_valid_authorizations(orderr) + + self.assertEqual(self.mock_net.deactivate_authorization.call_count, 2) + self.assertEqual(len(authzrs), 1) + self.assertEqual(len(failed), 1) + self.assertEqual(authzrs[0].body.identifier.value, "is_valid") + self.assertEqual(authzrs[0].body.status, messages.STATUS_DEACTIVATED) + self.assertEqual(failed[0].body.identifier.value, "is_valid_but_will_fail") + self.assertEqual(failed[0].body.status, messages.STATUS_VALID) + def _gen_mock_on_poll(status=messages.STATUS_VALID, retry=0, wait_value=1): state = {'count': retry} diff --git a/certbot/tests/client_test.py b/certbot/tests/client_test.py index 8d2cd76b9..f624c5320 100644 --- a/certbot/tests/client_test.py +++ b/certbot/tests/client_test.py @@ -256,6 +256,7 @@ class ClientTest(ClientTestCommon): def _mock_obtain_certificate(self): self.client.auth_handler = mock.MagicMock() self.client.auth_handler.handle_authorizations.return_value = [None] + self.client.auth_handler.deactivate_valid_authorizations.return_value = ([], []) self.acme.finalize_order.return_value = self.eg_order self.acme.new_order.return_value = self.eg_order self.eg_order.update.return_value = self.eg_order @@ -360,6 +361,47 @@ class ClientTest(ClientTestCommon): mock_crypto.init_save_csr.assert_not_called() self.assertEqual(mock_crypto.cert_and_chain_from_fullchain.call_count, 1) + @mock.patch("certbot.client.logger") + @mock.patch("certbot.client.crypto_util") + @mock.patch("certbot.client.acme_crypto_util") + def test_obtain_certificate_dry_run_authz_deactivations_failed(self, mock_acme_crypto, + mock_crypto, mock_log): + from acme import messages + csr = util.CSR(form="pem", file=None, data=CSR_SAN) + mock_acme_crypto.make_csr.return_value = CSR_SAN + mock_crypto.make_key.return_value = mock.sentinel.key_pem + key = util.Key(file=None, pem=mock.sentinel.key_pem) + self._set_mock_from_fullchain(mock_crypto.cert_and_chain_from_fullchain) + + self._mock_obtain_certificate() + self.client.config.dry_run = True + + # Two authzs that are already valid and should get deactivated (dry run) + authzrs = self._authzr_from_domains(["example.com", "www.example.com"]) + for authzr in authzrs: + authzr.body.status = messages.STATUS_VALID + + # One deactivation succeeds, one fails + auth_handler = self.client.auth_handler + auth_handler.deactivate_valid_authorizations.return_value = ([authzrs[0]], [authzrs[1]]) + + # Certificate should get issued despite one failed deactivation + self.eg_order.authorizations = authzrs + self.client.auth_handler.handle_authorizations.return_value = authzrs + with test_util.patch_get_utility(): + result = self.client.obtain_certificate(self.eg_domains) + self.assertEqual(result, (mock.sentinel.cert, mock.sentinel.chain, key, csr)) + self._check_obtain_certificate(1) + + # Deactivation success/failure should have been handled properly + self.assertEqual(auth_handler.deactivate_valid_authorizations.call_count, 1, + "Deactivate authorizations should be called") + self.assertEqual(self.acme.new_order.call_count, 2, + "Order should be recreated due to successfully deactivated authorizations") + mock_log.warning.assert_called_with("Certbot was unable to obtain fresh authorizations for" + " every domain. The dry run will continue, but results" + " may not be accurate.") + def _set_mock_from_fullchain(self, mock_from_fullchain): mock_cert = mock.Mock() mock_cert.encode.return_value = mock.sentinel.cert diff --git a/local-oldest-requirements.txt b/local-oldest-requirements.txt index e2a804e1b..0acc68652 100644 --- a/local-oldest-requirements.txt +++ b/local-oldest-requirements.txt @@ -1,2 +1,2 @@ # Remember to update setup.py to match the package versions below. -acme[dev]==0.29.0 +-e acme[dev] diff --git a/setup.py b/setup.py index d5469bb26..7088f3b11 100644 --- a/setup.py +++ b/setup.py @@ -34,7 +34,7 @@ version = meta['version'] # specified here to avoid masking the more specific request requirements in # acme. See https://github.com/pypa/pip/issues/988 for more info. install_requires = [ - 'acme>=0.29.0', + 'acme>=0.40.0.dev0', # We technically need ConfigArgParse 0.10.0 for Python 2.6 support, but # saying so here causes a runtime error against our temporary fork of 0.9.3 # in which we added 2.6 support (see #2243), so we relax the requirement.