From cc24b4e40af5841c8dfddeecd9bde7d1acce62e8 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Thu, 8 Mar 2018 11:12:33 -0800 Subject: [PATCH] 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 | 80 ++++++++++++++++-------------- certbot/tests/auth_handler_test.py | 54 ++++++++++---------- tests/boulder-integration.sh | 13 +++++ tests/manual-dns-auth.sh | 12 +++-- tests/manual-dns-cleanup.sh | 11 ++-- 5 files changed, 99 insertions(+), 71 deletions(-) diff --git a/certbot/auth_handler.py b/certbot/auth_handler.py index 67d36c8cc..51cdf09ee 100644 --- a/certbot/auth_handler.py +++ b/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: - self.aauthzrs.append(AnnotatedAuthzr(authzr, [])) + aauthzrs = [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(): - resp = self._solve_challenges() + while self._has_challenges(aauthzrs): + 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() - with error_handler.ErrorHandler(self._cleanup_challenges): + all_achalls = self._get_all_achalls(aauthzrs) + 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( - self, chall_update, best_effort, min_sleep=3, max_rounds=15): + def _poll_challenges(self, aauthzrs, chall_update, + 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): diff --git a/certbot/tests/auth_handler_test.py b/certbot/tests/auth_handler_test.py index b6af3d0f5..54e284d9e 100644 --- a/certbot/tests/auth_handler_test.py +++ b/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): - for i, aauthzr in enumerate(self.handler.aauthzrs): + def _validate_all(self, aauthzrs, unused_1, unused_2): + 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( - messages.STATUS_PENDING, self.doms[0], - [acme_util.HTTP01, acme_util.TLSSNI01], - [messages.STATUS_PENDING] * 2, False), [])) - self.handler.aauthzrs.append(AnnotatedAuthzr(acme_util.gen_authzr( - messages.STATUS_PENDING, self.doms[1], - acme_util.CHALLENGES, [messages.STATUS_PENDING] * 3, False), [])) - self.handler.aauthzrs.append(AnnotatedAuthzr(acme_util.gen_authzr( - messages.STATUS_PENDING, self.doms[2], - acme_util.CHALLENGES, [messages.STATUS_PENDING] * 3, False), [])) + self.aauthzrs = [ + AnnotatedAuthzr(acme_util.gen_authzr( + messages.STATUS_PENDING, self.doms[0], + [acme_util.HTTP01, acme_util.TLSSNI01], + [messages.STATUS_PENDING] * 2, False), []), + AnnotatedAuthzr(acme_util.gen_authzr( + messages.STATUS_PENDING, self.doms[1], + acme_util.CHALLENGES, [messages.STATUS_PENDING] * 3, False), []), + AnnotatedAuthzr(acme_util.gen_authzr( + 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( - errors.AuthorizationError, self.handler.verify_authzr_complete) + self.assertRaises(errors.AuthorizationError, + 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. diff --git a/tests/boulder-integration.sh b/tests/boulder-integration.sh index 2b92476fd..9748befa3 100755 --- a/tests/boulder-integration.sh +++ b/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 \ diff --git a/tests/manual-dns-auth.sh b/tests/manual-dns-auth.sh index 9b9a1a5eb..febecf455 100755 --- a/tests/manual-dns-auth.sh +++ b/tests/manual-dns-auth.sh @@ -1,4 +1,8 @@ -#!/bin/sh -curl -X POST 'http://localhost:8055/set-txt' -d \ - "{\"host\": \"_acme-challenge.$CERTBOT_DOMAIN.\", \ - \"value\": \"$CERTBOT_VALIDATION\"}" +#!/bin/bash + +# If domain begins with fail, fail the challenge by not completing it. +if [[ "$CERTBOT_DOMAIN" != fail* ]]; then + curl -X POST 'http://localhost:8055/set-txt' -d \ + "{\"host\": \"_acme-challenge.$CERTBOT_DOMAIN.\", \ + \"value\": \"$CERTBOT_VALIDATION\"}" +fi diff --git a/tests/manual-dns-cleanup.sh b/tests/manual-dns-cleanup.sh index 0c5c56b17..1c09e892c 100755 --- a/tests/manual-dns-cleanup.sh +++ b/tests/manual-dns-cleanup.sh @@ -1,3 +1,8 @@ -#!/bin/sh -curl -X POST 'http://localhost:8055/clear-txt' -d \ - "{\"host\": \"_acme-challenge.$CERTBOT_DOMAIN.\"}" +#!/bin/bash + +# 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