Fix --allow-subset-of-names (#5690)

* Remove aauthzr instance variable

* If domain begins with fail, fail the challenge.

* test --allow-subset-of-names

* Fix renewal and add extra check

* test after hook checks

certbot/auth_handler.py

@@ -34,8 +34,6 @@ class AuthHandler(object):
     :ivar account: Client's Account
     :type account: :class:`certbot.account.Account`
 
-    :ivar aauthzrs: ACME Authorization Resources and their active challenges
-    :type aauthzrs: `list` of `AnnotatedAuthzr`
     :ivar list pref_challs: sorted user specified preferred challenges
         type strings with the most preferred challenge listed first
 
@@ -45,7 +43,6 @@ class AuthHandler(object):
         self.acme = acme
 
         self.account = account
-        self.aauthzrs = []
         self.pref_challs = pref_challs
 
     def handle_authorizations(self, orderr, best_effort=False):
@@ -63,29 +60,29 @@ class AuthHandler(object):
             authorizations
 
         """
-        for authzr in orderr.authorizations:
+        aauthzrs = [AnnotatedAuthzr(authzr, [])
-            self.aauthzrs.append(AnnotatedAuthzr(authzr, []))
+                    for authzr in orderr.authorizations]
 
-        self._choose_challenges()
+        self._choose_challenges(aauthzrs)
         config = zope.component.getUtility(interfaces.IConfig)
         notify = zope.component.getUtility(interfaces.IDisplay).notification
 
         # While there are still challenges remaining...
-        while self._has_challenges():
+        while self._has_challenges(aauthzrs):
-            resp = self._solve_challenges()
+            resp = self._solve_challenges(aauthzrs)
             logger.info("Waiting for verification...")
             if config.debug_challenges:
                 notify('Challenges loaded. Press continue to submit to CA. '
                        'Pass "-v" for more info about challenges.', pause=True)
 
             # Send all Responses - this modifies achalls
-            self._respond(resp, best_effort)
+            self._respond(aauthzrs, resp, best_effort)
 
         # Just make sure all decisions are complete.
-        self.verify_authzr_complete()
+        self.verify_authzr_complete(aauthzrs)
 
         # Only return valid authorizations
-        retVal = [aauthzr.authzr for aauthzr in self.aauthzrs
+        retVal = [aauthzr.authzr for aauthzr in aauthzrs
                   if aauthzr.authzr.body.status == messages.STATUS_VALID]
 
         if not retVal:
@@ -94,10 +91,10 @@ class AuthHandler(object):
 
         return retVal
 
-    def _choose_challenges(self):
+    def _choose_challenges(self, aauthzrs):
         """Retrieve necessary challenges to satisfy server."""
         logger.info("Performing the following challenges:")
-        for aauthzr in self.aauthzrs:
+        for aauthzr in aauthzrs:
             aauthzr_challenges = aauthzr.authzr.body.challenges
             if self.acme.acme_version == 1:
                 combinations = aauthzr.authzr.body.combinations
@@ -113,15 +110,15 @@ class AuthHandler(object):
                 aauthzr.authzr, path)
             aauthzr.achalls.extend(aauthzr_achalls)
 
-    def _has_challenges(self):
+    def _has_challenges(self, aauthzrs):
         """Do we have any challenges to perform?"""
-        return any(aauthzr.achalls for aauthzr in self.aauthzrs)
+        return any(aauthzr.achalls for aauthzr in aauthzrs)
 
-    def _solve_challenges(self):
+    def _solve_challenges(self, aauthzrs):
         """Get Responses for challenges from authenticators."""
         resp = []
-        all_achalls = self._get_all_achalls()
+        all_achalls = self._get_all_achalls(aauthzrs)
-        with error_handler.ErrorHandler(self._cleanup_challenges):
+        with error_handler.ErrorHandler(self._cleanup_challenges, all_achalls):
             try:
                 if all_achalls:
                     resp = self.auth.perform(all_achalls)
@@ -134,15 +131,15 @@ class AuthHandler(object):
 
         return resp
 
-    def _get_all_achalls(self):
+    def _get_all_achalls(self, aauthzrs):
         """Return all active challenges."""
         all_achalls = []
-        for aauthzr in self.aauthzrs:
+        for aauthzr in aauthzrs:
             all_achalls.extend(aauthzr.achalls)
 
         return all_achalls
 
-    def _respond(self, resp, best_effort):
+    def _respond(self, aauthzrs, resp, best_effort):
         """Send/Receive confirmation of all challenges.
 
         .. note:: This method also cleans up the auth_handler state.
@@ -150,24 +147,27 @@ class AuthHandler(object):
         """
         # TODO: chall_update is a dirty hack to get around acme-spec #105
         chall_update = dict()
-        active_achalls = self._send_responses(resp, chall_update)
+        active_achalls = self._send_responses(aauthzrs, resp, chall_update)
 
         # Check for updated status...
         try:
-            self._poll_challenges(chall_update, best_effort)
+            self._poll_challenges(aauthzrs, chall_update, best_effort)
         finally:
-            self._cleanup_challenges(active_achalls)
+            self._cleanup_challenges(aauthzrs, active_achalls)
 
-    def _send_responses(self, resps, chall_update):
+    def _send_responses(self, aauthzrs, resps, chall_update):
         """Send responses and make sure errors are handled.
 
+        :param aauthzrs: authorizations and the selected annotated challenges
+            to try and perform
+        :type aauthzrs: `list` of `AnnotatedAuthzr`
         :param dict chall_update: parameter that is updated to hold
             aauthzr index to list of outstanding solved annotated challenges
 
         """
         active_achalls = []
         resps_iter = iter(resps)
-        for i, aauthzr in enumerate(self.aauthzrs):
+        for i, aauthzr in enumerate(aauthzrs):
             for achall in aauthzr.achalls:
                 # This line needs to be outside of the if block below to
                 # ensure failed challenges are cleaned up correctly
@@ -184,8 +184,8 @@ class AuthHandler(object):
 
         return active_achalls
 
-    def _poll_challenges(
+    def _poll_challenges(self, aauthzrs, chall_update,
-            self, chall_update, best_effort, min_sleep=3, max_rounds=15):
+                         best_effort, min_sleep=3, max_rounds=15):
         """Wait for all challenge results to be determined."""
         indices_to_check = set(chall_update.keys())
         comp_indices = set()
@@ -197,7 +197,7 @@ class AuthHandler(object):
             all_failed_achalls = set()
             for index in indices_to_check:
                 comp_achalls, failed_achalls = self._handle_check(
-                    index, chall_update[index])
+                    aauthzrs, index, chall_update[index])
 
                 if len(comp_achalls) == len(chall_update[index]):
                     comp_indices.add(index)
@@ -210,7 +210,7 @@ class AuthHandler(object):
                         comp_indices.add(index)
                         logger.warning(
                             "Challenge failed for domain %s",
-                            self.aauthzrs[index].authzr.body.identifier.value)
+                            aauthzrs[index].authzr.body.identifier.value)
                     else:
                         all_failed_achalls.update(
                             updated for _, updated in failed_achalls)
@@ -223,14 +223,14 @@ class AuthHandler(object):
             comp_indices.clear()
             rounds += 1
 
-    def _handle_check(self, index, achalls):
+    def _handle_check(self, aauthzrs, index, achalls):
         """Returns tuple of ('completed', 'failed')."""
         completed = []
         failed = []
 
-        original_aauthzr = self.aauthzrs[index]
+        original_aauthzr = aauthzrs[index]
         updated_authzr, _ = self.acme.poll(original_aauthzr.authzr)
-        self.aauthzrs[index] = AnnotatedAuthzr(updated_authzr, original_aauthzr.achalls)
+        aauthzrs[index] = AnnotatedAuthzr(updated_authzr, original_aauthzr.achalls)
         if updated_authzr.body.status == messages.STATUS_VALID:
             return achalls, []
 
@@ -287,7 +287,7 @@ class AuthHandler(object):
         chall_prefs.extend(plugin_pref)
         return chall_prefs
 
-    def _cleanup_challenges(self, achall_list=None):
+    def _cleanup_challenges(self, aauthzrs, achall_list=None):
         """Cleanup challenges.
 
         If achall_list is not provided, cleanup all achallenges.
@@ -296,26 +296,30 @@ class AuthHandler(object):
         logger.info("Cleaning up challenges")
 
         if achall_list is None:
-            achalls = self._get_all_achalls()
+            achalls = self._get_all_achalls(aauthzrs)
         else:
             achalls = achall_list
 
         if achalls:
             self.auth.cleanup(achalls)
             for achall in achalls:
-                for aauthzr in self.aauthzrs:
+                for aauthzr in aauthzrs:
                     if achall in aauthzr.achalls:
                         aauthzr.achalls.remove(achall)
                         break
 
-    def verify_authzr_complete(self):
+    def verify_authzr_complete(self, aauthzrs):
         """Verifies that all authorizations have been decided.
 
+        :param aauthzrs: authorizations and their selected annotated
+            challenges
+        :type aauthzrs: `list` of `AnnotatedAuthzr`
+
         :returns: Whether all authzr are complete
         :rtype: bool
 
         """
-        for aauthzr in self.aauthzrs:
+        for aauthzr in aauthzrs:
             authzr = aauthzr.authzr
             if (authzr.body.status != messages.STATUS_VALID and
                     authzr.body.status != messages.STATUS_INVALID):

certbot/tests/auth_handler_test.py

@@ -101,7 +101,7 @@ class HandleAuthorizationsTest(unittest.TestCase):
         self.assertEqual(self.mock_net.answer_challenge.call_count, 1)
 
         self.assertEqual(mock_poll.call_count, 1)
-        chall_update = mock_poll.call_args[0][0]
+        chall_update = mock_poll.call_args[0][1]
         self.assertEqual(list(six.iterkeys(chall_update)), [0])
         self.assertEqual(len(chall_update.values()), 1)
 
@@ -132,7 +132,7 @@ class HandleAuthorizationsTest(unittest.TestCase):
         self.assertEqual(self.mock_net.answer_challenge.call_count, 3)
 
         self.assertEqual(mock_poll.call_count, 1)
-        chall_update = mock_poll.call_args[0][0]
+        chall_update = mock_poll.call_args[0][1]
         self.assertEqual(list(six.iterkeys(chall_update)), [0])
         self.assertEqual(len(chall_update.values()), 1)
 
@@ -158,7 +158,7 @@ class HandleAuthorizationsTest(unittest.TestCase):
         self.assertEqual(self.mock_net.answer_challenge.call_count, 1)
 
         self.assertEqual(mock_poll.call_count, 1)
-        chall_update = mock_poll.call_args[0][0]
+        chall_update = mock_poll.call_args[0][1]
         self.assertEqual(list(six.iterkeys(chall_update)), [0])
         self.assertEqual(len(chall_update.values()), 1)
 
@@ -187,7 +187,7 @@ class HandleAuthorizationsTest(unittest.TestCase):
 
         # Check poll call
         self.assertEqual(mock_poll.call_count, 1)
-        chall_update = mock_poll.call_args[0][0]
+        chall_update = mock_poll.call_args[0][1]
         self.assertEqual(len(list(six.iterkeys(chall_update))), 3)
         self.assertTrue(0 in list(six.iterkeys(chall_update)))
         self.assertEqual(len(chall_update[0]), 1)
@@ -278,8 +278,8 @@ class HandleAuthorizationsTest(unittest.TestCase):
         self.assertRaises(
             errors.AuthorizationError, self.handler.handle_authorizations, mock_order)
 
-    def _validate_all(self, unused_1, unused_2):
+    def _validate_all(self, aauthzrs, unused_1, unused_2):
-        for i, aauthzr in enumerate(self.handler.aauthzrs):
+        for i, aauthzr in enumerate(aauthzrs):
             azr = aauthzr.authzr
             updated_azr = acme_util.gen_authzr(
                 messages.STATUS_VALID,
@@ -287,7 +287,7 @@ class HandleAuthorizationsTest(unittest.TestCase):
                 [challb.chall for challb in azr.body.challenges],
                 [messages.STATUS_VALID] * len(azr.body.challenges),
                 azr.body.combinations)
-            self.handler.aauthzrs[i] = type(aauthzr)(updated_azr, aauthzr.achalls)
+            aauthzrs[i] = type(aauthzr)(updated_azr, aauthzr.achalls)
 
 
 class PollChallengesTest(unittest.TestCase):
@@ -304,19 +304,21 @@ class PollChallengesTest(unittest.TestCase):
             None, self.mock_net, mock.Mock(key="mock_key"), [])
 
         self.doms = ["0", "1", "2"]
-        self.handler.aauthzrs.append(AnnotatedAuthzr(acme_util.gen_authzr(
+        self.aauthzrs = [
-            messages.STATUS_PENDING, self.doms[0],
+            AnnotatedAuthzr(acme_util.gen_authzr(
-            [acme_util.HTTP01, acme_util.TLSSNI01],
+                messages.STATUS_PENDING, self.doms[0],
-            [messages.STATUS_PENDING] * 2, False), []))
+                [acme_util.HTTP01, acme_util.TLSSNI01],
-        self.handler.aauthzrs.append(AnnotatedAuthzr(acme_util.gen_authzr(
+                [messages.STATUS_PENDING] * 2, False), []),
-            messages.STATUS_PENDING, self.doms[1],
+            AnnotatedAuthzr(acme_util.gen_authzr(
-            acme_util.CHALLENGES, [messages.STATUS_PENDING] * 3, False), []))
+                messages.STATUS_PENDING, self.doms[1],
-        self.handler.aauthzrs.append(AnnotatedAuthzr(acme_util.gen_authzr(
+                acme_util.CHALLENGES, [messages.STATUS_PENDING] * 3, False), []),
-            messages.STATUS_PENDING, self.doms[2],
+            AnnotatedAuthzr(acme_util.gen_authzr(
-            acme_util.CHALLENGES, [messages.STATUS_PENDING] * 3, False), []))
+                messages.STATUS_PENDING, self.doms[2],
+                acme_util.CHALLENGES, [messages.STATUS_PENDING] * 3, False), [])
+        ]
 
         self.chall_update = {}
-        for i, aauthzr in enumerate(self.handler.aauthzrs):
+        for i, aauthzr in enumerate(self.aauthzrs):
             self.chall_update[i] = [
                 challb_to_achall(challb, mock.Mock(key="dummy_key"), self.doms[i])
                 for challb in aauthzr.authzr.body.challenges]
@@ -324,17 +326,17 @@ class PollChallengesTest(unittest.TestCase):
     @mock.patch("certbot.auth_handler.time")
     def test_poll_challenges(self, unused_mock_time):
         self.mock_net.poll.side_effect = self._mock_poll_solve_one_valid
-        self.handler._poll_challenges(self.chall_update, False)
+        self.handler._poll_challenges(self.aauthzrs, self.chall_update, False)
 
-        for aauthzr in self.handler.aauthzrs:
+        for aauthzr in self.aauthzrs:
             self.assertEqual(aauthzr.authzr.body.status, messages.STATUS_VALID)
 
     @mock.patch("certbot.auth_handler.time")
     def test_poll_challenges_failure_best_effort(self, unused_mock_time):
         self.mock_net.poll.side_effect = self._mock_poll_solve_one_invalid
-        self.handler._poll_challenges(self.chall_update, True)
+        self.handler._poll_challenges(self.aauthzrs, self.chall_update, True)
 
-        for aauthzr in self.handler.aauthzrs:
+        for aauthzr in self.aauthzrs:
             self.assertEqual(aauthzr.authzr.body.status, messages.STATUS_PENDING)
 
     @mock.patch("certbot.auth_handler.time")
@@ -343,7 +345,7 @@ class PollChallengesTest(unittest.TestCase):
         self.mock_net.poll.side_effect = self._mock_poll_solve_one_invalid
         self.assertRaises(
             errors.AuthorizationError, self.handler._poll_challenges,
-            self.chall_update, False)
+            self.aauthzrs, self.chall_update, False)
 
     @mock.patch("certbot.auth_handler.time")
     def test_unable_to_find_challenge_status(self, unused_mock_time):
@@ -353,11 +355,11 @@ class PollChallengesTest(unittest.TestCase):
             challb_to_achall(acme_util.DNS01_P, "key", self.doms[0]))
         self.assertRaises(
             errors.AuthorizationError, self.handler._poll_challenges,
-            self.chall_update, False)
+            self.aauthzrs, self.chall_update, False)
 
     def test_verify_authzr_failure(self):
-        self.assertRaises(
+        self.assertRaises(errors.AuthorizationError,
-            errors.AuthorizationError, self.handler.verify_authzr_complete)
+                          self.handler.verify_authzr_complete, self.aauthzrs)
 
     def _mock_poll_solve_one_valid(self, authzr):
         # Pending here because my dummy script won't change the full status.

tests/boulder-integration.sh

@@ -327,6 +327,19 @@ CheckDirHooks 1
 common renew --cert-name le2.wtf
 CheckDirHooks 1
 
+# manual-dns-auth.sh will skip completing the challenge for domains that begin
+# with fail.
+common -a manual -d dns1.le.wtf,fail.dns1.le.wtf \
+    --allow-subset-of-names \
+    --preferred-challenges dns,tls-sni \
+    --manual-auth-hook ./tests/manual-dns-auth.sh \
+    --manual-cleanup-hook ./tests/manual-dns-cleanup.sh
+
+if common certificates | grep "fail\.dns1\.le\.wtf"; then
+    echo "certificate should not have been issued for domain!" >&2
+    exit 1
+fi
+
 # ECDSA
 openssl ecparam -genkey -name secp384r1 -out "${root}/privkey-p384.pem"
 SAN="DNS:ecdsa.le.wtf" openssl req -new -sha256 \

tests/manual-dns-auth.sh

@@ -1,4 +1,8 @@
-#!/bin/sh
+#!/bin/bash
-curl -X POST 'http://localhost:8055/set-txt' -d \
+
-    "{\"host\": \"_acme-challenge.$CERTBOT_DOMAIN.\", \
+# If domain begins with fail, fail the challenge by not completing it.
-     \"value\": \"$CERTBOT_VALIDATION\"}"
+if [[ "$CERTBOT_DOMAIN" != fail* ]]; then
+    curl -X POST 'http://localhost:8055/set-txt' -d \
+        "{\"host\": \"_acme-challenge.$CERTBOT_DOMAIN.\", \
+         \"value\": \"$CERTBOT_VALIDATION\"}"
+fi

tests/manual-dns-cleanup.sh

@@ -1,3 +1,8 @@
-#!/bin/sh
+#!/bin/bash
-curl -X POST 'http://localhost:8055/clear-txt' -d \
+
-    "{\"host\": \"_acme-challenge.$CERTBOT_DOMAIN.\"}"
+# If domain begins with fail, we didn't complete the challenge so there is
+# nothing to clean up.
+if [[ "$CERTBOT_DOMAIN" != fail* ]]; then
+    curl -X POST 'http://localhost:8055/clear-txt' -d \
+        "{\"host\": \"_acme-challenge.$CERTBOT_DOMAIN.\"}"
+fi