Fix wildcard issuance (certbot#5620)

* Add is_wildcard_domain to certbot.util.

* Error with --allow-subset-of-names and wildcards.

* Fix issue preventing wildcard cert issuance.

* Kill assumption domain is unique in auth_handler

* fix typo and add test

* update comments

certbot/auth_handler.py

@@ -1,4 +1,5 @@
 """ACME AuthHandler."""
+import collections
 import logging
 import time
 
@@ -17,6 +18,10 @@
 logger = logging.getLogger(__name__)
 
 
+AnnotatedAuthzr = collections.namedtuple("AnnotatedAuthzr", ["authzr", "achalls"])
+"""Stores an authorization resource and its active annotated challenges."""
+
+
 class AuthHandler(object):
     """ACME Authorization Handler for a client.
 
@@ -29,10 +34,8 @@ class AuthHandler(object):
     :ivar account: Client's Account
     :type account: :class:`certbot.account.Account`
 
-    :ivar dict authzr: ACME Authorization Resource dict where keys are domains
-        and values are :class:`acme.messages.AuthorizationResource`
-    :ivar list achalls: DV challenges in the form of
-        :class:`certbot.achallenges.AnnotatedChallenge`
+    :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
 
@@ -42,12 +45,9 @@ def __init__(self, auth, acme, account, pref_challs):
         self.acme = acme
 
         self.account = account
-        self.authzr = dict()
+        self.aauthzrs = []
         self.pref_challs = pref_challs
 
-        # List must be used to keep responses straight.
-        self.achalls = []
-
     def handle_authorizations(self, orderr, best_effort=False):
         """Retrieve all authorizations for challenges.
 
@@ -63,17 +63,15 @@ def handle_authorizations(self, orderr, best_effort=False):
             authorizations
 
         """
-        authzrs = orderr.authorizations
-        for authzr in authzrs:
-            self.authzr[authzr.body.identifier.value] = authzr
-        domains = self.authzr.keys()
+        for authzr in orderr.authorizations:
+            self.aauthzrs.append(AnnotatedAuthzr(authzr, []))
 
-        self._choose_challenges(domains)
+        self._choose_challenges()
         config = zope.component.getUtility(interfaces.IConfig)
         notify = zope.component.getUtility(interfaces.IDisplay).notification
 
         # While there are still challenges remaining...
-        while self.achalls:
+        while self._has_challenges():
             resp = self._solve_challenges()
             logger.info("Waiting for verification...")
             if config.debug_challenges:
@@ -87,50 +85,63 @@ def handle_authorizations(self, orderr, best_effort=False):
         self.verify_authzr_complete()
 
         # Only return valid authorizations
-        retVal = [authzr for authzr in self.authzr.values()
-                  if authzr.body.status == messages.STATUS_VALID]
+        retVal = [aauthzr.authzr for aauthzr in self.aauthzrs
+                  if aauthzr.authzr.body.status == messages.STATUS_VALID]
 
         if not retVal:
             raise errors.AuthorizationError(
                 "Challenges failed for all domains")
 
         return retVal
 
-    def _choose_challenges(self, domains):
+    def _choose_challenges(self):
         """Retrieve necessary challenges to satisfy server."""
         logger.info("Performing the following challenges:")
-        for dom in domains:
-            dom_challenges = self.authzr[dom].body.challenges
+        for aauthzr in self.aauthzrs:
+            aauthzr_challenges = aauthzr.authzr.body.challenges
             if self.acme.acme_version == 1:
-                combinations = self.authzr[dom].body.combinations
+                combinations = aauthzr.authzr.body.combinations
             else:
-                combinations = tuple((i,) for i in range(len(dom_challenges)))
+                combinations = tuple((i,) for i in range(len(aauthzr_challenges)))
 
             path = gen_challenge_path(
-                dom_challenges,
-                self._get_chall_pref(dom),
+                aauthzr_challenges,
+                self._get_chall_pref(aauthzr.authzr.body.identifier.value),
                 combinations)
 
-            dom_achalls = self._challenge_factory(
-                dom, path)
-            self.achalls.extend(dom_achalls)
+            aauthzr_achalls = self._challenge_factory(
+                aauthzr.authzr, path)
+            aauthzr.achalls.extend(aauthzr_achalls)
+
+    def _has_challenges(self):
+        """Do we have any challenges to perform?"""
+        return any(aauthzr.achalls for aauthzr in self.aauthzrs)
 
     def _solve_challenges(self):
         """Get Responses for challenges from authenticators."""
         resp = []
+        all_achalls = self._get_all_achalls()
         with error_handler.ErrorHandler(self._cleanup_challenges):
             try:
-                if self.achalls:
-                    resp = self.auth.perform(self.achalls)
+                if all_achalls:
+                    resp = self.auth.perform(all_achalls)
             except errors.AuthorizationError:
                 logger.critical("Failure in setting up challenges.")
                 logger.info("Attempting to clean up outstanding challenges...")
                 raise
 
-        assert len(resp) == len(self.achalls)
+        assert len(resp) == len(all_achalls)
 
         return resp
 
+    def _get_all_achalls(self):
+        """Return all active challenges."""
+        all_achalls = []
+        for aauthzr in self.aauthzrs:
+            all_achalls.extend(aauthzr.achalls)
+
+        return all_achalls
+
     def _respond(self, resp, best_effort):
         """Send/Receive confirmation of all challenges.
 
@@ -139,69 +150,67 @@ def _respond(self, resp, best_effort):
         """
         # TODO: chall_update is a dirty hack to get around acme-spec #105
         chall_update = dict()
-        active_achalls = self._send_responses(self.achalls,
-                                              resp, chall_update)
+        active_achalls = self._send_responses(resp, chall_update)
 
         # Check for updated status...
         try:
             self._poll_challenges(chall_update, best_effort)
         finally:
-            # This removes challenges from self.achalls
             self._cleanup_challenges(active_achalls)
 
-    def _send_responses(self, achalls, resps, chall_update):
+    def _send_responses(self, resps, chall_update):
         """Send responses and make sure errors are handled.
 
         :param dict chall_update: parameter that is updated to hold
-            authzr -> list of outstanding solved annotated challenges
+            aauthzr index to list of outstanding solved annotated challenges
 
         """
         active_achalls = []
-        for achall, resp in six.moves.zip(achalls, resps):
-            # This line needs to be outside of the if block below to
-            # ensure failed challenges are cleaned up correctly
-            active_achalls.append(achall)
-
-            # Don't send challenges for None and False authenticator responses
-            if resp is not None and resp:
-                self.acme.answer_challenge(achall.challb, resp)
-                # TODO: answer_challenge returns challr, with URI,
-                # that can be used in _find_updated_challr
-                # comparisons...
-                if achall.domain in chall_update:
-                    chall_update[achall.domain].append(achall)
-                else:
-                    chall_update[achall.domain] = [achall]
+        resps_iter = iter(resps)
+        for i, aauthzr in enumerate(self.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
+                active_achalls.append(achall)
+
+                resp = next(resps_iter)
+                # Don't send challenges for None and False authenticator responses
+                if resp:
+                    self.acme.answer_challenge(achall.challb, resp)
+                    # TODO: answer_challenge returns challr, with URI,
+                    # that can be used in _find_updated_challr
+                    # comparisons...
+                    chall_update.setdefault(i, []).append(achall)
 
         return active_achalls
 
     def _poll_challenges(
             self, chall_update, best_effort, min_sleep=3, max_rounds=15):
         """Wait for all challenge results to be determined."""
-        dom_to_check = set(chall_update.keys())
-        comp_domains = set()
+        indices_to_check = set(chall_update.keys())
+        comp_indices = set()
         rounds = 0
 
-        while dom_to_check and rounds < max_rounds:
+        while indices_to_check and rounds < max_rounds:
             # TODO: Use retry-after...
             time.sleep(min_sleep)
             all_failed_achalls = set()
-            for domain in dom_to_check:
+            for index in indices_to_check:
                 comp_achalls, failed_achalls = self._handle_check(
-                    domain, chall_update[domain])
+                    index, chall_update[index])
 
-                if len(comp_achalls) == len(chall_update[domain]):
-                    comp_domains.add(domain)
+                if len(comp_achalls) == len(chall_update[index]):
+                    comp_indices.add(index)
                 elif not failed_achalls:
                     for achall, _ in comp_achalls:
-                        chall_update[domain].remove(achall)
+                        chall_update[index].remove(achall)
                 # We failed some challenges... damage control
                 else:
                     if best_effort:
-                        comp_domains.add(domain)
+                        comp_indices.add(index)
                         logger.warning(
                             "Challenge failed for domain %s",
-                            domain)
+                            self.aauthzrs[index].authzr.body.identifier.value)
                     else:
                         all_failed_achalls.update(
                             updated for _, updated in failed_achalls)
@@ -210,24 +219,26 @@ def _poll_challenges(
                 _report_failed_challs(all_failed_achalls)
                 raise errors.FailedChallenges(all_failed_achalls)
 
-            dom_to_check -= comp_domains
-            comp_domains.clear()
+            indices_to_check -= comp_indices
+            comp_indices.clear()
             rounds += 1
 
-    def _handle_check(self, domain, achalls):
+    def _handle_check(self, index, achalls):
         """Returns tuple of ('completed', 'failed')."""
         completed = []
         failed = []
 
-        self.authzr[domain], _ = self.acme.poll(self.authzr[domain])
-        if self.authzr[domain].body.status == messages.STATUS_VALID:
+        original_aauthzr = self.aauthzrs[index]
+        updated_authzr, _ = self.acme.poll(original_aauthzr.authzr)
+        self.aauthzrs[index] = AnnotatedAuthzr(updated_authzr, original_aauthzr.achalls)
+        if updated_authzr.body.status == messages.STATUS_VALID:
             return achalls, []
 
         # Note: if the whole authorization is invalid, the individual failed
         #     challenges will be determined here...
         for achall in achalls:
             updated_achall = achall.update(challb=self._find_updated_challb(
-                self.authzr[domain], achall))
+                updated_authzr, achall))
 
             # This does nothing for challenges that have yet to be decided yet.
             if updated_achall.status == messages.STATUS_VALID:
@@ -285,14 +296,17 @@ def _cleanup_challenges(self, achall_list=None):
         logger.info("Cleaning up challenges")
 
         if achall_list is None:
-            achalls = self.achalls
+            achalls = self._get_all_achalls()
         else:
             achalls = achall_list
 
         if achalls:
             self.auth.cleanup(achalls)
             for achall in achalls:
-                self.achalls.remove(achall)
+                for aauthzr in self.aauthzrs:
+                    if achall in aauthzr.achalls:
+                        aauthzr.achalls.remove(achall)
+                        break
 
     def verify_authzr_complete(self):
         """Verifies that all authorizations have been decided.
@@ -301,15 +315,16 @@ def verify_authzr_complete(self):
         :rtype: bool
 
         """
-        for authzr in self.authzr.values():
+        for aauthzr in self.aauthzrs:
+            authzr = aauthzr.authzr
             if (authzr.body.status != messages.STATUS_VALID and
                     authzr.body.status != messages.STATUS_INVALID):
                 raise errors.AuthorizationError("Incomplete authorizations")
 
-    def _challenge_factory(self, domain, path):
+    def _challenge_factory(self, authzr, path):
         """Construct Namedtuple Challenges
 
-        :param str domain: domain of the enrollee
+        :param messages.AuthorizationResource authzr: authorization
 
         :param list path: List of indices from `challenges`.
 
@@ -323,8 +338,9 @@ def _challenge_factory(self, domain, path):
         achalls = []
 
         for index in path:
-            challb = self.authzr[domain].body.challenges[index]
-            achalls.append(challb_to_achall(challb, self.account.key, domain))
+            challb = authzr.body.challenges[index]
+            achalls.append(challb_to_achall(
+                challb, self.account.key, authzr.body.identifier.value))
 
         return achalls
 

certbot/cli.py

@@ -599,6 +599,11 @@ def parse_args(self):
         if parsed_args.validate_hooks:
             hooks.validate_hooks(parsed_args)
 
+        if parsed_args.allow_subset_of_names:
+            if any(util.is_wildcard_domain(d) for d in parsed_args.domains):
+                raise errors.Error("Using --allow-subset-of-names with a"
+                                   " wildcard domain is not supported.")
+
         possible_deprecation_warning(parsed_args)
 
         return parsed_args

certbot/client.py

@@ -298,7 +298,12 @@ def obtain_certificate(self, domains):
         auth_domains = set(a.body.identifier.value for a in authzr)
         successful_domains = [d for d in domains if d in auth_domains]
 
-        if successful_domains != domains:
+        # allow_subset_of_names is currently disabled for wildcard
+        # certificates. The reason for this and checking allow_subset_of_names
+        # below is because successful_domains == domains is never true if
+        # domains contains a wildcard because the ACME spec forbids identifiers
+        # in authzs from containing a wildcard character.
+        if self.config.allow_subset_of_names and successful_domains != domains:
             if not self.config.dry_run:
                 os.remove(key.file)
                 os.remove(csr.file)