diff --git a/acme/acme/client.py b/acme/acme/client.py index d52c82a5c..9e2478afe 100644 --- a/acme/acme/client.py +++ b/acme/acme/client.py @@ -227,8 +227,7 @@ class ClientBase(object): # pylint: disable=too-many-instance-attributes response = self._post(url, messages.Revocation( certificate=cert, - reason=rsn), - content_type=None) + reason=rsn)) if response.status_code != http_client.OK: raise errors.ClientError( 'Successful revocation must return HTTP OK status') diff --git a/acme/acme/client_test.py b/acme/acme/client_test.py index a0c27e74f..00b9e19dd 100644 --- a/acme/acme/client_test.py +++ b/acme/acme/client_test.py @@ -635,8 +635,7 @@ class ClientTest(ClientTestBase): def test_revoke(self): self.client.revoke(self.certr.body, self.rsn) self.net.post.assert_called_once_with( - self.directory[messages.Revocation], mock.ANY, content_type=None, - acme_version=1) + self.directory[messages.Revocation], mock.ANY, acme_version=1) def test_revocation_payload(self): obj = messages.Revocation(certificate=self.certr.body, reason=self.rsn) @@ -776,8 +775,7 @@ class ClientV2Test(ClientTestBase): def test_revoke(self): self.client.revoke(messages_test.CERT, self.rsn) self.net.post.assert_called_once_with( - self.directory["revokeCert"], mock.ANY, content_type=None, - acme_version=2) + self.directory["revokeCert"], mock.ANY, acme_version=2) class MockJSONDeSerializable(jose.JSONDeSerializable): diff --git a/certbot-dns-google/certbot_dns_google/__init__.py b/certbot-dns-google/certbot_dns_google/__init__.py index 7349a7696..f19266737 100644 --- a/certbot-dns-google/certbot_dns_google/__init__.py +++ b/certbot-dns-google/certbot_dns_google/__init__.py @@ -29,6 +29,8 @@ for an account with the following permissions: * ``dns.managedZones.list`` * ``dns.resourceRecordSets.create`` * ``dns.resourceRecordSets.delete`` +* ``dns.resourceRecordSets.list`` +* ``dns.resourceRecordSets.update`` Google provides instructions for `creating a service account `_ and diff --git a/certbot-dns-google/certbot_dns_google/dns_google.py b/certbot-dns-google/certbot_dns_google/dns_google.py index cea754c06..e2088b357 100644 --- a/certbot-dns-google/certbot_dns_google/dns_google.py +++ b/certbot-dns-google/certbot_dns_google/dns_google.py @@ -107,6 +107,17 @@ class _GoogleClient(object): zone_id = self._find_managed_zone_id(domain) + record_contents = self.get_existing_txt_rrset(zone_id, record_name) + if record_contents is None: + record_contents = [] + add_records = record_contents[:] + + if "\""+record_content+"\"" in record_contents: + # The process was interrupted previously and validation token exists + return + + add_records.append(record_content) + data = { "kind": "dns#change", "additions": [ @@ -114,12 +125,24 @@ class _GoogleClient(object): "kind": "dns#resourceRecordSet", "type": "TXT", "name": record_name + ".", - "rrdatas": [record_content, ], + "rrdatas": add_records, "ttl": record_ttl, }, ], } + if record_contents: + # We need to remove old records in the same request + data["deletions"] = [ + { + "kind": "dns#resourceRecordSet", + "type": "TXT", + "name": record_name + ".", + "rrdatas": record_contents, + "ttl": record_ttl, + }, + ] + changes = self.dns.changes() # changes | pylint: disable=no-member try: @@ -154,6 +177,10 @@ class _GoogleClient(object): logger.warn('Error finding zone. Skipping cleanup.') return + record_contents = self.get_existing_txt_rrset(zone_id, record_name) + if record_contents is None: + record_contents = ["\"" + record_content + "\""] + data = { "kind": "dns#change", "deletions": [ @@ -161,12 +188,26 @@ class _GoogleClient(object): "kind": "dns#resourceRecordSet", "type": "TXT", "name": record_name + ".", - "rrdatas": [record_content, ], + "rrdatas": record_contents, "ttl": record_ttl, }, ], } + # Remove the record being deleted from the list + readd_contents = [r for r in record_contents if r != "\"" + record_content + "\""] + if readd_contents: + # We need to remove old records in the same request + data["additions"] = [ + { + "kind": "dns#resourceRecordSet", + "type": "TXT", + "name": record_name + ".", + "rrdatas": readd_contents, + "ttl": record_ttl, + }, + ] + changes = self.dns.changes() # changes | pylint: disable=no-member try: @@ -175,6 +216,37 @@ class _GoogleClient(object): except googleapiclient_errors.Error as e: logger.warn('Encountered error deleting TXT record: %s', e) + def get_existing_txt_rrset(self, zone_id, record_name): + """ + Get existing TXT records from the RRset for the record name. + + If an error occurs while requesting the record set, it is suppressed + and None is returned. + + :param str zone_id: The ID of the managed zone. + :param str record_name: The record name (typically beginning with '_acme-challenge.'). + + :returns: List of TXT record values or None + :rtype: `list` of `string` or `None` + + """ + rrs_request = self.dns.resourceRecordSets() # pylint: disable=no-member + request = rrs_request.list(managedZone=zone_id, project=self.project_id) + # Add dot as the API returns absolute domains + record_name += "." + try: + response = request.execute() + except googleapiclient_errors.Error: + logger.info("Unable to list existing records. If you're " + "requesting a wildcard certificate, this might not work.") + logger.debug("Error was:", exc_info=True) + else: + if response: + for rr in response["rrsets"]: + if rr["name"] == record_name and rr["type"] == "TXT": + return rr["rrdatas"] + return None + def _find_managed_zone_id(self, domain): """ Find the managed zone for a given domain. diff --git a/certbot-dns-google/certbot_dns_google/dns_google_test.py b/certbot-dns-google/certbot_dns_google/dns_google_test.py index 53f84dd6e..72b8be8af 100644 --- a/certbot-dns-google/certbot_dns_google/dns_google_test.py +++ b/certbot-dns-google/certbot_dns_google/dns_google_test.py @@ -74,10 +74,15 @@ class GoogleClientTest(unittest.TestCase): mock_mz = mock.MagicMock() mock_mz.list.return_value.execute.side_effect = zone_request_side_effect + mock_rrs = mock.MagicMock() + rrsets = {"rrsets": [{"name": "_acme-challenge.example.org.", "type": "TXT", + "rrdatas": ["\"example-txt-contents\""]}]} + mock_rrs.list.return_value.execute.return_value = rrsets mock_changes = mock.MagicMock() client.dns.managedZones = mock.MagicMock(return_value=mock_mz) client.dns.changes = mock.MagicMock(return_value=mock_changes) + client.dns.resourceRecordSets = mock.MagicMock(return_value=mock_rrs) return client, mock_changes @@ -137,6 +142,30 @@ class GoogleClientTest(unittest.TestCase): managedZone=self.zone, project=PROJECT_ID) + @mock.patch('oauth2client.service_account.ServiceAccountCredentials.from_json_keyfile_name') + @mock.patch('certbot_dns_google.dns_google.open', + mock.mock_open(read_data='{"project_id": "' + PROJECT_ID + '"}'), create=True) + def test_add_txt_record_delete_old(self, unused_credential_mock): + client, changes = self._setUp_client_with_mock( + [{'managedZones': [{'id': self.zone}]}]) + mock_get_rrs = "certbot_dns_google.dns_google._GoogleClient.get_existing_txt_rrset" + with mock.patch(mock_get_rrs) as mock_rrs: + mock_rrs.return_value = ["sample-txt-contents"] + client.add_txt_record(DOMAIN, self.record_name, self.record_content, self.record_ttl) + self.assertTrue(changes.create.called) + self.assertTrue("sample-txt-contents" in + changes.create.call_args_list[0][1]["body"]["deletions"][0]["rrdatas"]) + + @mock.patch('oauth2client.service_account.ServiceAccountCredentials.from_json_keyfile_name') + @mock.patch('certbot_dns_google.dns_google.open', + mock.mock_open(read_data='{"project_id": "' + PROJECT_ID + '"}'), create=True) + def test_add_txt_record_noop(self, unused_credential_mock): + client, changes = self._setUp_client_with_mock( + [{'managedZones': [{'id': self.zone}]}]) + client.add_txt_record(DOMAIN, "_acme-challenge.example.org", + "example-txt-contents", self.record_ttl) + self.assertFalse(changes.create.called) + @mock.patch('oauth2client.service_account.ServiceAccountCredentials.from_json_keyfile_name') @mock.patch('certbot_dns_google.dns_google.open', mock.mock_open(read_data='{"project_id": "' + PROJECT_ID + '"}'), create=True) @@ -172,7 +201,12 @@ class GoogleClientTest(unittest.TestCase): def test_del_txt_record(self, unused_credential_mock): client, changes = self._setUp_client_with_mock([{'managedZones': [{'id': self.zone}]}]) - client.del_txt_record(DOMAIN, self.record_name, self.record_content, self.record_ttl) + mock_get_rrs = "certbot_dns_google.dns_google._GoogleClient.get_existing_txt_rrset" + with mock.patch(mock_get_rrs) as mock_rrs: + mock_rrs.return_value = ["\"sample-txt-contents\"", + "\"example-txt-contents\""] + client.del_txt_record(DOMAIN, "_acme-challenge.example.org", + "example-txt-contents", self.record_ttl) expected_body = { "kind": "dns#change", @@ -180,8 +214,17 @@ class GoogleClientTest(unittest.TestCase): { "kind": "dns#resourceRecordSet", "type": "TXT", - "name": self.record_name + ".", - "rrdatas": [self.record_content, ], + "name": "_acme-challenge.example.org.", + "rrdatas": ["\"sample-txt-contents\"", "\"example-txt-contents\""], + "ttl": self.record_ttl, + }, + ], + "additions": [ + { + "kind": "dns#resourceRecordSet", + "type": "TXT", + "name": "_acme-challenge.example.org.", + "rrdatas": ["\"sample-txt-contents\"", ], "ttl": self.record_ttl, }, ], @@ -217,6 +260,31 @@ class GoogleClientTest(unittest.TestCase): client.del_txt_record(DOMAIN, self.record_name, self.record_content, self.record_ttl) + @mock.patch('oauth2client.service_account.ServiceAccountCredentials.from_json_keyfile_name') + @mock.patch('certbot_dns_google.dns_google.open', + mock.mock_open(read_data='{"project_id": "' + PROJECT_ID + '"}'), create=True) + def test_get_existing(self, unused_credential_mock): + client, unused_changes = self._setUp_client_with_mock( + [{'managedZones': [{'id': self.zone}]}]) + # Record name mocked in setUp + found = client.get_existing_txt_rrset(self.zone, "_acme-challenge.example.org") + self.assertEquals(found, ["\"example-txt-contents\""]) + not_found = client.get_existing_txt_rrset(self.zone, "nonexistent.tld") + self.assertEquals(not_found, None) + + @mock.patch('oauth2client.service_account.ServiceAccountCredentials.from_json_keyfile_name') + @mock.patch('certbot_dns_google.dns_google.open', + mock.mock_open(read_data='{"project_id": "' + PROJECT_ID + '"}'), create=True) + def test_get_existing_fallback(self, unused_credential_mock): + client, unused_changes = self._setUp_client_with_mock( + [{'managedZones': [{'id': self.zone}]}]) + # pylint: disable=no-member + mock_execute = client.dns.resourceRecordSets.return_value.list.return_value.execute + mock_execute.side_effect = API_ERROR + + rrset = client.get_existing_txt_rrset(self.zone, "_acme-challenge.example.org") + self.assertFalse(rrset) + def test_get_project_id(self): from certbot_dns_google.dns_google import _GoogleClient diff --git a/certbot-dns-route53/certbot_dns_route53/dns_route53.py b/certbot-dns-route53/certbot_dns_route53/dns_route53.py index 67462e369..08b1d03f0 100644 --- a/certbot-dns-route53/certbot_dns_route53/dns_route53.py +++ b/certbot-dns-route53/certbot_dns_route53/dns_route53.py @@ -1,4 +1,5 @@ """Certbot Route53 authenticator plugin.""" +import collections import logging import time @@ -33,6 +34,7 @@ class Authenticator(dns_common.DNSAuthenticator): def __init__(self, *args, **kwargs): super(Authenticator, self).__init__(*args, **kwargs) self.r53 = boto3.client("route53") + self._resource_records = collections.defaultdict(list) def more_info(self): # pylint: disable=missing-docstring,no-self-use return "Solve a DNS01 challenge using AWS Route53" @@ -88,6 +90,20 @@ class Authenticator(dns_common.DNSAuthenticator): def _change_txt_record(self, action, validation_domain_name, validation): zone_id = self._find_zone_id_for_domain(validation_domain_name) + rrecords = self._resource_records[validation_domain_name] + challenge = {"Value": '"{0}"'.format(validation)} + if action == "DELETE": + # Remove the record being deleted from the list of tracked records + rrecords.remove(challenge) + if rrecords: + # Need to update instead, as we're not deleting the rrset + action = "UPSERT" + else: + # Create a new list containing the record to use with DELETE + rrecords = [challenge] + else: + rrecords.append(challenge) + response = self.r53.change_resource_record_sets( HostedZoneId=zone_id, ChangeBatch={ @@ -99,11 +115,7 @@ class Authenticator(dns_common.DNSAuthenticator): "Name": validation_domain_name, "Type": "TXT", "TTL": self.ttl, - "ResourceRecords": [ - # For some reason TXT records need to be - # manually quoted. - {"Value": '"{0}"'.format(validation)} - ], + "ResourceRecords": rrecords, } } ] diff --git a/certbot-dns-route53/certbot_dns_route53/dns_route53_test.py b/certbot-dns-route53/certbot_dns_route53/dns_route53_test.py index d5f1b2816..7534e132c 100644 --- a/certbot-dns-route53/certbot_dns_route53/dns_route53_test.py +++ b/certbot-dns-route53/certbot_dns_route53/dns_route53_test.py @@ -186,6 +186,48 @@ class ClientTest(unittest.TestCase): call_count = self.client.r53.change_resource_record_sets.call_count self.assertEqual(call_count, 1) + def test_change_txt_record_delete(self): + self.client._find_zone_id_for_domain = mock.MagicMock() + self.client.r53.change_resource_record_sets = mock.MagicMock( + return_value={"ChangeInfo": {"Id": 1}}) + + validation = "some-value" + validation_record = {"Value": '"{0}"'.format(validation)} + self.client._resource_records[DOMAIN] = [validation_record] + + self.client._change_txt_record("DELETE", DOMAIN, validation) + + call_count = self.client.r53.change_resource_record_sets.call_count + self.assertEqual(call_count, 1) + call_args = self.client.r53.change_resource_record_sets.call_args_list[0][1] + call_args_batch = call_args["ChangeBatch"]["Changes"][0] + self.assertEqual(call_args_batch["Action"], "DELETE") + self.assertEqual( + call_args_batch["ResourceRecordSet"]["ResourceRecords"], + [validation_record]) + + def test_change_txt_record_multirecord(self): + self.client._find_zone_id_for_domain = mock.MagicMock() + self.client._get_validation_rrset = mock.MagicMock() + self.client._resource_records[DOMAIN] = [ + {"Value": "\"pre-existing-value\""}, + {"Value": "\"pre-existing-value-two\""}, + ] + self.client.r53.change_resource_record_sets = mock.MagicMock( + return_value={"ChangeInfo": {"Id": 1}}) + + self.client._change_txt_record("DELETE", DOMAIN, "pre-existing-value") + + call_count = self.client.r53.change_resource_record_sets.call_count + call_args = self.client.r53.change_resource_record_sets.call_args_list[0][1] + call_args_batch = call_args["ChangeBatch"]["Changes"][0] + self.assertEqual(call_args_batch["Action"], "UPSERT") + self.assertEqual( + call_args_batch["ResourceRecordSet"]["ResourceRecords"], + [{"Value": "\"pre-existing-value-two\""}]) + + self.assertEqual(call_count, 1) + def test_wait_for_change(self): self.client.r53.get_change = mock.MagicMock( side_effect=[{"ChangeInfo": {"Status": "PENDING"}}, 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/plugins/disco.py b/certbot/plugins/disco.py index 5a7e07ec0..062c11650 100644 --- a/certbot/plugins/disco.py +++ b/certbot/plugins/disco.py @@ -190,6 +190,7 @@ class PluginsRegistry(collections.Mapping): def find_all(cls): """Find plugins using setuptools entry points.""" plugins = {} + # pylint: disable=not-callable entry_points = itertools.chain( pkg_resources.iter_entry_points( constants.SETUPTOOLS_PLUGINS_ENTRY_POINT), diff --git a/certbot/plugins/manual.py b/certbot/plugins/manual.py index 07371ad34..614449d34 100644 --- a/certbot/plugins/manual.py +++ b/certbot/plugins/manual.py @@ -189,7 +189,7 @@ when it receives a TLS ClientHello with the SNI extension set to os.environ.update(env) _, out = hooks.execute(self.conf('auth-hook')) env['CERTBOT_AUTH_OUTPUT'] = out.strip() - self.env[achall.domain] = env + self.env[achall] = env def _perform_achall_manually(self, achall): validation = achall.validation(achall.account_key) @@ -215,7 +215,7 @@ when it receives a TLS ClientHello with the SNI extension set to def cleanup(self, achalls): # pylint: disable=missing-docstring if self.conf('cleanup-hook'): for achall in achalls: - env = self.env.pop(achall.domain) + env = self.env.pop(achall) if 'CERTBOT_TOKEN' not in env: os.environ.pop('CERTBOT_TOKEN', None) os.environ.update(env) diff --git a/certbot/plugins/manual_test.py b/certbot/plugins/manual_test.py index ac528e81c..e5c22b377 100644 --- a/certbot/plugins/manual_test.py +++ b/certbot/plugins/manual_test.py @@ -93,10 +93,10 @@ class AuthenticatorTest(test_util.TempDirTestCase): self.auth.perform(self.achalls), [achall.response(achall.account_key) for achall in self.achalls]) self.assertEqual( - self.auth.env[self.dns_achall.domain]['CERTBOT_AUTH_OUTPUT'], + self.auth.env[self.dns_achall]['CERTBOT_AUTH_OUTPUT'], dns_expected) self.assertEqual( - self.auth.env[self.http_achall.domain]['CERTBOT_AUTH_OUTPUT'], + self.auth.env[self.http_achall]['CERTBOT_AUTH_OUTPUT'], http_expected) # tls_sni_01 challenge must be perform()ed above before we can # get the cert_path and key_path. @@ -107,7 +107,7 @@ class AuthenticatorTest(test_util.TempDirTestCase): self.auth.tls_sni_01.get_z_domain(self.tls_sni_achall), 'novalidation') self.assertEqual( - self.auth.env[self.tls_sni_achall.domain]['CERTBOT_AUTH_OUTPUT'], + self.auth.env[self.tls_sni_achall]['CERTBOT_AUTH_OUTPUT'], tls_sni_expected) @test_util.patch_get_utility() 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/letsencrypt-auto-source/letsencrypt-auto b/letsencrypt-auto-source/letsencrypt-auto index 9ff1c1386..f97dc078d 100755 --- a/letsencrypt-auto-source/letsencrypt-auto +++ b/letsencrypt-auto-source/letsencrypt-auto @@ -1216,7 +1216,7 @@ UNLIKELY_EOF # ------------------------------------------------------------------------- cat << "UNLIKELY_EOF" > "$TEMP_DIR/pipstrap.py" #!/usr/bin/env python -"""A small script that can act as a trust root for installing pip 8 +"""A small script that can act as a trust root for installing pip >=8 Embed this in your project, and your VCS checkout is all you have to trust. In a post-peep era, this lets you claw your way to a hash-checking version of pip, @@ -1274,7 +1274,7 @@ except ImportError: from urllib.parse import urlparse # 3.4 -__version__ = 1, 5, 0 +__version__ = 1, 5, 1 PIP_VERSION = '9.0.1' DEFAULT_INDEX_BASE = 'https://pypi.python.org' @@ -1287,14 +1287,11 @@ maybe_argparse = ( if version_info < (2, 7, 0) else []) -# Pip has no dependencies, as it vendors everything: -PIP_PACKAGE = [ +PACKAGES = maybe_argparse + [ + # Pip has no dependencies, as it vendors everything: ('11/b6/abcb525026a4be042b486df43905d6893fb04f05aac21c32c638e939e447/' 'pip-{0}.tar.gz'.format(PIP_VERSION), - '09f243e1a7b461f654c26a725fa373211bb7ff17a9300058b205c61658ca940d')] - - -OTHER_PACKAGES = maybe_argparse + [ + '09f243e1a7b461f654c26a725fa373211bb7ff17a9300058b205c61658ca940d'), # This version of setuptools has only optional dependencies: ('59/88/2f3990916931a5de6fa9706d6d75eb32ee8b78627bb2abaab7ed9e6d0622/' 'setuptools-29.0.1.tar.gz', @@ -1379,21 +1376,16 @@ def main(): index_base = get_index_base() temp = mkdtemp(prefix='pipstrap-') try: - # We download and install pip first, then the rest, to avoid the bug - # https://github.com/certbot/certbot/issues/4938. - pip_downloads, other_downloads = [ - [hashed_download(index_base + '/packages/' + path, - temp, - digest) - for path, digest in packages] - for packages in (PIP_PACKAGE, OTHER_PACKAGES)] - for downloads in (pip_downloads, other_downloads): - check_output('pip install --no-index --no-deps -U ' + - # Disable cache since we're not using it and it - # otherwise sometimes throws permission warnings: - ('--no-cache-dir ' if has_pip_cache else '') + - ' '.join(quote(d) for d in downloads), - shell=True) + downloads = [hashed_download(index_base + '/packages/' + path, + temp, + digest) + for path, digest in PACKAGES] + check_output('pip install --no-index --no-deps -U ' + + # Disable cache since we're not using it and it otherwise + # sometimes throws permission warnings: + ('--no-cache-dir ' if has_pip_cache else '') + + ' '.join(quote(d) for d in downloads), + shell=True) except HashError as exc: print(exc) except Exception: diff --git a/letsencrypt-auto-source/pieces/pipstrap.py b/letsencrypt-auto-source/pieces/pipstrap.py index ed55b37e9..d55d5bceb 100755 --- a/letsencrypt-auto-source/pieces/pipstrap.py +++ b/letsencrypt-auto-source/pieces/pipstrap.py @@ -1,5 +1,5 @@ #!/usr/bin/env python -"""A small script that can act as a trust root for installing pip 8 +"""A small script that can act as a trust root for installing pip >=8 Embed this in your project, and your VCS checkout is all you have to trust. In a post-peep era, this lets you claw your way to a hash-checking version of pip, @@ -57,7 +57,7 @@ except ImportError: from urllib.parse import urlparse # 3.4 -__version__ = 1, 5, 0 +__version__ = 1, 5, 1 PIP_VERSION = '9.0.1' DEFAULT_INDEX_BASE = 'https://pypi.python.org' @@ -70,14 +70,11 @@ maybe_argparse = ( if version_info < (2, 7, 0) else []) -# Pip has no dependencies, as it vendors everything: -PIP_PACKAGE = [ +PACKAGES = maybe_argparse + [ + # Pip has no dependencies, as it vendors everything: ('11/b6/abcb525026a4be042b486df43905d6893fb04f05aac21c32c638e939e447/' 'pip-{0}.tar.gz'.format(PIP_VERSION), - '09f243e1a7b461f654c26a725fa373211bb7ff17a9300058b205c61658ca940d')] - - -OTHER_PACKAGES = maybe_argparse + [ + '09f243e1a7b461f654c26a725fa373211bb7ff17a9300058b205c61658ca940d'), # This version of setuptools has only optional dependencies: ('59/88/2f3990916931a5de6fa9706d6d75eb32ee8b78627bb2abaab7ed9e6d0622/' 'setuptools-29.0.1.tar.gz', @@ -162,21 +159,16 @@ def main(): index_base = get_index_base() temp = mkdtemp(prefix='pipstrap-') try: - # We download and install pip first, then the rest, to avoid the bug - # https://github.com/certbot/certbot/issues/4938. - pip_downloads, other_downloads = [ - [hashed_download(index_base + '/packages/' + path, - temp, - digest) - for path, digest in packages] - for packages in (PIP_PACKAGE, OTHER_PACKAGES)] - for downloads in (pip_downloads, other_downloads): - check_output('pip install --no-index --no-deps -U ' + - # Disable cache since we're not using it and it - # otherwise sometimes throws permission warnings: - ('--no-cache-dir ' if has_pip_cache else '') + - ' '.join(quote(d) for d in downloads), - shell=True) + downloads = [hashed_download(index_base + '/packages/' + path, + temp, + digest) + for path, digest in PACKAGES] + check_output('pip install --no-index --no-deps -U ' + + # Disable cache since we're not using it and it otherwise + # sometimes throws permission warnings: + ('--no-cache-dir ' if has_pip_cache else '') + + ' '.join(quote(d) for d in downloads), + shell=True) except HashError as exc: print(exc) except Exception: diff --git a/tests/boulder-integration.sh b/tests/boulder-integration.sh index f2b0dcf60..9748befa3 100755 --- a/tests/boulder-integration.sh +++ b/tests/boulder-integration.sh @@ -233,6 +233,7 @@ certname="dns.le.wtf" common -a manual -d dns.le.wtf --preferred-challenges dns,tls-sni run \ --cert-name $certname \ --manual-auth-hook ./tests/manual-dns-auth.sh \ + --manual-cleanup-hook ./tests/manual-dns-cleanup.sh \ --pre-hook 'echo wtf2.pre >> "$HOOK_TEST"' \ --post-hook 'echo wtf2.post >> "$HOOK_TEST"' \ --renew-hook 'echo deploy >> "$HOOK_TEST"' @@ -326,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 \ @@ -433,7 +447,8 @@ done # Test ACMEv2-only features if [ "${BOULDER_INTEGRATION:-v1}" = "v2" ]; then common -a manual -d '*.le4.wtf,le4.wtf' --preferred-challenges dns \ - --manual-auth-hook ./tests/manual-dns-auth.sh + --manual-auth-hook ./tests/manual-dns-auth.sh \ + --manual-cleanup-hook ./tests/manual-dns-cleanup.sh fi # Most CI systems set this variable to true. @@ -443,4 +458,4 @@ then . ./certbot-nginx/tests/boulder-integration.sh fi -coverage report --fail-under 63 -m +coverage report --fail-under 67 -m diff --git a/tests/integration/_common.sh b/tests/integration/_common.sh index 236090a14..a8d35ed89 100755 --- a/tests/integration/_common.sh +++ b/tests/integration/_common.sh @@ -23,7 +23,7 @@ fi certbot_test_no_force_renew () { omit_patterns="*/*.egg-info/*,*/dns_common*,*/setup.py,*/test_*,*/tests/*" - omit_patterns="$omit_patterns,*_test.py,*_test_*," + omit_patterns="$omit_patterns,*_test.py,*_test_*,certbot-apache/*" omit_patterns="$omit_patterns,certbot-compatibility-test/*,certbot-dns*/" coverage run \ --append \ 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 new file mode 100755 index 000000000..1c09e892c --- /dev/null +++ b/tests/manual-dns-cleanup.sh @@ -0,0 +1,8 @@ +#!/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