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

@@ -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

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(

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'

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'],

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

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)
 

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}

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

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]

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.