[Windows|Unix] Avoid to re-execute challenges already validated (#6551)

In response to #5342.

Currently, certbot will execute the operations necessary to validate a challenge even if the challenge has already been validated before against the acme ca server. This can occur for instance if a certificate is asked and issue correctly, then deleted locally, then asked again.

It is a corner case, but it will lead to some heavy operations (like updating a DNS zone, or creating an HTTP server) that are not needed.

This PR corrects this behavior by not executing challenges already validated, and use them directly instead to issue the certificate.

Fixes #5342

* Avoid to execute a given challenge that have been already validated by acme ca server.

* Execute tls challenge on a separate dns name, to avoid reusing the existing valid http challenge.

* Align with master

* Improve log

* Simplify the implementation

* Update changelog

* Add a unit test to ensure that validated challenges are not rerun

CHANGELOG.md

@@ -6,7 +6,8 @@ Certbot adheres to [Semantic Versioning](https://semver.org/).
 
 ### Added
 
-*
+* Avoid to process again challenges that are already validated
+  when a certificate is issued.
 
 ### Changed
 
@@ -22,6 +23,7 @@ all Certbot components during releases for the time being, however, the only
 package with changes other than its version number was:
 
 * acme
+* certbot
 
 More details about these changes can be found on our GitHub repo.
 

certbot-nginx/tests/boulder-integration.conf.sh

@@ -1,3 +1,4 @@
+#!/usr/bin/env bash
 # Based on
 # https://www.exratione.com/2014/03/running-nginx-as-a-non-root-user/
 # https://github.com/exratione/non-root-nginx/blob/9a77f62e5d5cb9c9026fd62eece76b9514011019/nginx.conf
@@ -52,7 +53,7 @@ http {
     listen 5002 $default_server;
     # IPv6.
     listen [::]:5002 $default_server;
-    server_name nginx.wtf nginx2.wtf;
+    server_name nginx.wtf nginx-tls.wtf nginx2.wtf;
 
     root $root/webroot;
 

certbot-nginx/tests/boulder-integration.sh

@@ -39,8 +39,8 @@ nginx -v
 reload_nginx
 certbot_test_nginx --domains nginx.wtf run
 test_deployment_and_rollback nginx.wtf
-certbot_test_nginx --domains nginx.wtf run --preferred-challenges tls-sni
-test_deployment_and_rollback nginx.wtf
+certbot_test_nginx --domains nginx-tls.wtf run --preferred-challenges tls-sni
+test_deployment_and_rollback nginx-tls.wtf
 certbot_test_nginx --domains nginx2.wtf --preferred-challenges http
 test_deployment_and_rollback nginx2.wtf
 # Overlapping location block and server-block-level return 301

certbot/auth_handler.py

@@ -31,7 +31,7 @@ class AuthHandler(object):
         :class:`~acme.challenges.Challenge` types
     :type auth: :class:`certbot.interfaces.IAuthenticator`
 
-    :ivar acme.client.BackwardsCompatibleClientV2 acme: ACME client API.
+    :ivar acme.client.BackwardsCompatibleClientV2 acme_client: ACME client API.
 
     :ivar account: Client's Account
     :type account: :class:`certbot.account.Account`
@@ -40,9 +40,9 @@ class AuthHandler(object):
         type strings with the most preferred challenge listed first
 
     """
-    def __init__(self, auth, acme, account, pref_challs):
+    def __init__(self, auth, acme_client, account, pref_challs):
         self.auth = auth
-        self.acme = acme
+        self.acme = acme_client
 
         self.account = account
         self.pref_challs = pref_challs
@@ -85,19 +85,26 @@ class AuthHandler(object):
         self.verify_authzr_complete(aauthzrs)
 
         # Only return valid authorizations
-        retVal = [aauthzr.authzr for aauthzr in aauthzrs
-                  if aauthzr.authzr.body.status == messages.STATUS_VALID]
+        ret_val = [aauthzr.authzr for aauthzr in aauthzrs
+                   if aauthzr.authzr.body.status == messages.STATUS_VALID]
 
-        if not retVal:
+        if not ret_val:
             raise errors.AuthorizationError(
                 "Challenges failed for all domains")
 
-        return retVal
+        return ret_val
 
     def _choose_challenges(self, aauthzrs):
-        """Retrieve necessary challenges to satisfy server."""
-        logger.info("Performing the following challenges:")
-        for aauthzr in aauthzrs:
+        """
+        Retrieve necessary and pending challenges to satisfy server.
+        NB: Necessary and already validated challenges are not retrieved,
+        as they can be reused for a certificate issuance.
+        """
+        pending_authzrs = [aauthzr for aauthzr in aauthzrs
+                           if aauthzr.authzr.body.status != messages.STATUS_VALID]
+        if pending_authzrs:
+            logger.info("Performing the following challenges:")
+        for aauthzr in pending_authzrs:
             aauthzr_challenges = aauthzr.authzr.body.challenges
             if self.acme.acme_version == 1:
                 combinations = aauthzr.authzr.body.combinations
@@ -125,7 +132,7 @@ class AuthHandler(object):
 
     def _solve_challenges(self, aauthzrs):
         """Get Responses for challenges from authenticators."""
-        resp = []  # type: Collection[acme.challenges.ChallengeResponse]
+        resp = []  # type: Collection[challenges.ChallengeResponse]
         all_achalls = self._get_all_achalls(aauthzrs)
         try:
             if all_achalls:
@@ -531,7 +538,7 @@ def _report_failed_challs(failed_achalls):
 
     """
     problems = collections.defaultdict(list)\
-    # type: DefaultDict[str, List[achallenges.KeyAuthorizationAnnotatedChallenge]]
+        # type: DefaultDict[str, List[achallenges.KeyAuthorizationAnnotatedChallenge]]
     for achall in failed_achalls:
         if achall.error:
             problems[achall.error.typ].append(achall)

certbot/tests/auth_handler_test.py

@@ -57,7 +57,7 @@ class ChallengeFactoryTest(unittest.TestCase):
              errors.Error, self.handler._challenge_factory, authzr, [0])
 
 
-class HandleAuthorizationsTest(unittest.TestCase):
+class HandleAuthorizationsTest(unittest.TestCase):  # pylint: disable=too-many-public-methods
     """handle_authorizations test.
 
     This tests everything except for all functions under _poll_challenges.
@@ -316,6 +316,24 @@ class HandleAuthorizationsTest(unittest.TestCase):
         self.assertEqual(
             self.mock_auth.cleanup.call_args[0][0][0].typ, "tls-sni-01")
 
+    def test_validated_challenge_not_rerun(self):
+        # With pending challenge, we expect the challenge to be tried, and fail.
+        authzr = acme_util.gen_authzr(
+                messages.STATUS_PENDING, "0",
+                [acme_util.HTTP01],
+                [messages.STATUS_PENDING], False)
+        mock_order = mock.MagicMock(authorizations=[authzr])
+        self.assertRaises(
+            errors.AuthorizationError, self.handler.handle_authorizations, mock_order)
+
+        # With validated challenge; we expect the challenge not be tried again, and succeed.
+        authzr = acme_util.gen_authzr(
+                messages.STATUS_VALID, "0",
+                [acme_util.HTTP01],
+                [messages.STATUS_VALID], False)
+        mock_order = mock.MagicMock(authorizations=[authzr])
+        self.handler.handle_authorizations(mock_order)
+
     def _validate_all(self, aauthzrs, unused_1, unused_2):
         for i, aauthzr in enumerate(aauthzrs):
             azr = aauthzr.authzr