From 441625c6102126f2d63daa964ecac4073e583d0a Mon Sep 17 00:00:00 2001 From: Joona Hoikkala Date: Mon, 5 Mar 2018 22:49:02 +0200 Subject: [PATCH 1/8] Allow Google DNS plugin to write multiple TXT record values (#5652) * Allow Google DNS plugin to write multiple TXT record values in same resourcerecord * Atomic updates * Split rrsets request --- .../certbot_dns_google/dns_google.py | 63 ++++++++++++++++++- .../certbot_dns_google/dns_google_test.py | 61 +++++++++++++++++- 2 files changed, 119 insertions(+), 5 deletions(-) diff --git a/certbot-dns-google/certbot_dns_google/dns_google.py b/certbot-dns-google/certbot_dns_google/dns_google.py index cea754c06..ab8bf20de 100644 --- a/certbot-dns-google/certbot_dns_google/dns_google.py +++ b/certbot-dns-google/certbot_dns_google/dns_google.py @@ -107,6 +107,15 @@ class _GoogleClient(object): zone_id = self._find_managed_zone_id(domain) + record_contents = self.get_existing_txt_rrset(zone_id, record_name) + 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 +123,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 +175,8 @@ class _GoogleClient(object): logger.warn('Error finding zone. Skipping cleanup.') return + record_contents = self.get_existing_txt_rrset(zone_id, record_name) + data = { "kind": "dns#change", "deletions": [ @@ -161,12 +184,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 +212,28 @@ 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. + + :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 + :rtype: `list` of `string` + + """ + rrs_request = self.dns.resourceRecordSets() # pylint: disable=no-member + request = rrs_request.list(managedZone=zone_id, project=self.project_id) + response = request.execute() + # Add dot as the API returns absolute domains + record_name += "." + if response: + for rr in response["rrsets"]: + if rr["name"] == record_name and rr["type"] == "TXT": + return rr["rrdatas"] + return [] + 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..3291b2c3a 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,18 @@ 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, []) + def test_get_project_id(self): from certbot_dns_google.dns_google import _GoogleClient From fe682e779b82ab0dfd72342369df630495c26a20 Mon Sep 17 00:00:00 2001 From: Joona Hoikkala Date: Sun, 4 Mar 2018 16:24:42 +0200 Subject: [PATCH 2/8] ACMEv2 support for Route53 plugin --- .../certbot_dns_route53/dns_route53.py | 26 +++++++-- .../certbot_dns_route53/dns_route53_test.py | 54 +++++++++++++++++++ 2 files changed, 75 insertions(+), 5 deletions(-) diff --git a/certbot-dns-route53/certbot_dns_route53/dns_route53.py b/certbot-dns-route53/certbot_dns_route53/dns_route53.py index 67462e369..c0e8e5495 100644 --- a/certbot-dns-route53/certbot_dns_route53/dns_route53.py +++ b/certbot-dns-route53/certbot_dns_route53/dns_route53.py @@ -85,9 +85,29 @@ class Authenticator(dns_common.DNSAuthenticator): zones.sort(key=lambda z: len(z[0]), reverse=True) return zones[0][1] + def _get_validation_rrset(self, zone_id, validation_domain_name): + validation_domain_name += "." + records = self.r53.list_resource_record_sets(HostedZoneId=zone_id) + for record in records["ResourceRecordSets"]: + if record["Name"] == validation_domain_name and record["Type"] == "TXT": + return record["ResourceRecords"] + return [] + def _change_txt_record(self, action, validation_domain_name, validation): zone_id = self._find_zone_id_for_domain(validation_domain_name) + rrecords = self._get_validation_rrset(zone_id, validation_domain_name) + challenge = {"Value": '"{0}"'.format(validation)} + if action == "DELETE": + if len(rrecords) > 1: + # Need to update instead, as we're not deleting the rrset + action = "UPSERT" + # Remove the record being deleted from the list + rrecords = [rr for rr in rrecords if rr != challenge] + else: + if challenge not in rrecords: + rrecords.append(challenge) + response = self.r53.change_resource_record_sets( HostedZoneId=zone_id, ChangeBatch={ @@ -99,11 +119,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..9aec05b6e 100644 --- a/certbot-dns-route53/certbot_dns_route53/dns_route53_test.py +++ b/certbot-dns-route53/certbot_dns_route53/dns_route53_test.py @@ -178,6 +178,9 @@ class ClientTest(unittest.TestCase): def test_change_txt_record(self): self.client._find_zone_id_for_domain = mock.MagicMock() + self.client._get_validation_rrset = mock.MagicMock( + return_value=[] + ) self.client.r53.change_resource_record_sets = mock.MagicMock( return_value={"ChangeInfo": {"Id": 1}}) @@ -186,6 +189,57 @@ 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_multirecord(self): + self.client._find_zone_id_for_domain = mock.MagicMock() + self.client._get_validation_rrset = mock.MagicMock() + self.client._get_validation_rrset.return_value = [ + {"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_get_validation_rrset(self): + self.client.r53.list_resource_record_sets = mock.MagicMock( + return_value={"ResourceRecordSets": [ + {"Name": "_acme-challenge.example.org.", + "Type": "TXT", + "ResourceRecords": [ + {"Value": "\"validation-token\""}, + {"Value": "\"another-validation-token\""}, + ], + }, + {"Name": "_acme-challenge.example.org.", + "Type": "NS", + "ResourceRecords": [ + {"Value": "ns1.example.com"}, + ], + } + ]}) + rrset = self.client._get_validation_rrset("zoneid", + "_acme-challenge.example.org") + self.assertEquals(len(rrset), 2) + self.assertTrue({"Value": "\"another-validation-token\""} in rrset) + + def test_get_validation_rrset_empty(self): + self.client.r53.list_resource_record_sets = mock.MagicMock( + return_value={"ResourceRecordSets": []}) + rrset = self.client._get_validation_rrset("zoneid", + "_acme-challenge.example.org") + self.assertEquals(rrset, []) + def test_wait_for_change(self): self.client.r53.get_change = mock.MagicMock( side_effect=[{"ChangeInfo": {"Status": "PENDING"}}, From 7bc45121a13537cceef4e4bf53d4738925d55511 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Mon, 5 Mar 2018 18:36:03 -0800 Subject: [PATCH 3/8] Remove the need for route53:ListResourceRecordSets * add test_change_txt_record_delete --- .../certbot_dns_route53/dns_route53.py | 24 ++++----- .../certbot_dns_route53/dns_route53_test.py | 54 ++++++++----------- 2 files changed, 31 insertions(+), 47 deletions(-) diff --git a/certbot-dns-route53/certbot_dns_route53/dns_route53.py b/certbot-dns-route53/certbot_dns_route53/dns_route53.py index c0e8e5495..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" @@ -85,28 +87,22 @@ class Authenticator(dns_common.DNSAuthenticator): zones.sort(key=lambda z: len(z[0]), reverse=True) return zones[0][1] - def _get_validation_rrset(self, zone_id, validation_domain_name): - validation_domain_name += "." - records = self.r53.list_resource_record_sets(HostedZoneId=zone_id) - for record in records["ResourceRecordSets"]: - if record["Name"] == validation_domain_name and record["Type"] == "TXT": - return record["ResourceRecords"] - return [] - def _change_txt_record(self, action, validation_domain_name, validation): zone_id = self._find_zone_id_for_domain(validation_domain_name) - rrecords = self._get_validation_rrset(zone_id, validation_domain_name) + rrecords = self._resource_records[validation_domain_name] challenge = {"Value": '"{0}"'.format(validation)} if action == "DELETE": - if len(rrecords) > 1: + # 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" - # Remove the record being deleted from the list - rrecords = [rr for rr in rrecords if rr != challenge] + else: + # Create a new list containing the record to use with DELETE + rrecords = [challenge] else: - if challenge not in rrecords: - rrecords.append(challenge) + rrecords.append(challenge) response = self.r53.change_resource_record_sets( HostedZoneId=zone_id, 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 9aec05b6e..7534e132c 100644 --- a/certbot-dns-route53/certbot_dns_route53/dns_route53_test.py +++ b/certbot-dns-route53/certbot_dns_route53/dns_route53_test.py @@ -178,9 +178,6 @@ class ClientTest(unittest.TestCase): def test_change_txt_record(self): self.client._find_zone_id_for_domain = mock.MagicMock() - self.client._get_validation_rrset = mock.MagicMock( - return_value=[] - ) self.client.r53.change_resource_record_sets = mock.MagicMock( return_value={"ChangeInfo": {"Id": 1}}) @@ -189,10 +186,30 @@ 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._get_validation_rrset.return_value = [ + self.client._resource_records[DOMAIN] = [ {"Value": "\"pre-existing-value\""}, {"Value": "\"pre-existing-value-two\""}, ] @@ -211,35 +228,6 @@ class ClientTest(unittest.TestCase): self.assertEqual(call_count, 1) - def test_get_validation_rrset(self): - self.client.r53.list_resource_record_sets = mock.MagicMock( - return_value={"ResourceRecordSets": [ - {"Name": "_acme-challenge.example.org.", - "Type": "TXT", - "ResourceRecords": [ - {"Value": "\"validation-token\""}, - {"Value": "\"another-validation-token\""}, - ], - }, - {"Name": "_acme-challenge.example.org.", - "Type": "NS", - "ResourceRecords": [ - {"Value": "ns1.example.com"}, - ], - } - ]}) - rrset = self.client._get_validation_rrset("zoneid", - "_acme-challenge.example.org") - self.assertEquals(len(rrset), 2) - self.assertTrue({"Value": "\"another-validation-token\""} in rrset) - - def test_get_validation_rrset_empty(self): - self.client.r53.list_resource_record_sets = mock.MagicMock( - return_value={"ResourceRecordSets": []}) - rrset = self.client._get_validation_rrset("zoneid", - "_acme-challenge.example.org") - self.assertEquals(rrset, []) - def test_wait_for_change(self): self.client.r53.get_change = mock.MagicMock( side_effect=[{"ChangeInfo": {"Status": "PENDING"}}, From cee9ac586ea43d355ede8eac71f5d145902169a9 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Tue, 6 Mar 2018 07:20:34 -0800 Subject: [PATCH 4/8] Don't report coverage on Apache during integration tests (#5669) * ignore Apache coverage * drop min coverage to 67 --- tests/boulder-integration.sh | 2 +- tests/integration/_common.sh | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/boulder-integration.sh b/tests/boulder-integration.sh index f2b0dcf60..b5a305016 100755 --- a/tests/boulder-integration.sh +++ b/tests/boulder-integration.sh @@ -443,4 +443,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 \ From d62c56f9c91a920a07f338bbc7aa53b7329624ac Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Tue, 6 Mar 2018 07:21:01 -0800 Subject: [PATCH 5/8] Remove the assumption the domain is unique in the manual plugin (#5670) * use entire achall as key * Add manual cleanup hook * use manual cleanup hook --- certbot/plugins/manual.py | 4 ++-- certbot/plugins/manual_test.py | 6 +++--- tests/boulder-integration.sh | 4 +++- tests/manual-dns-cleanup.sh | 3 +++ 4 files changed, 11 insertions(+), 6 deletions(-) create mode 100755 tests/manual-dns-cleanup.sh 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/tests/boulder-integration.sh b/tests/boulder-integration.sh index b5a305016..2b92476fd 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"' @@ -433,7 +434,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. diff --git a/tests/manual-dns-cleanup.sh b/tests/manual-dns-cleanup.sh new file mode 100755 index 000000000..0c5c56b17 --- /dev/null +++ b/tests/manual-dns-cleanup.sh @@ -0,0 +1,3 @@ +#!/bin/sh +curl -X POST 'http://localhost:8055/clear-txt' -d \ + "{\"host\": \"_acme-challenge.$CERTBOT_DOMAIN.\"}" From 6357e051f4841df8af69a8abf5a4ba3dc8578c3c Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Tue, 6 Mar 2018 15:32:22 -0800 Subject: [PATCH 6/8] Fallback without dns.resourceRecordSets.list permission (#5678) * Add rrset list fallback * List dns.resourceRecordSets.list as required * Handle list failures differently for add and del * Quote record content * disable not-callable for iter_entry_points * List update permission --- .../certbot_dns_google/__init__.py | 2 ++ .../certbot_dns_google/dns_google.py | 29 ++++++++++++++----- .../certbot_dns_google/dns_google_test.py | 14 ++++++++- certbot/plugins/disco.py | 1 + 4 files changed, 37 insertions(+), 9 deletions(-) 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 ab8bf20de..e2088b357 100644 --- a/certbot-dns-google/certbot_dns_google/dns_google.py +++ b/certbot-dns-google/certbot_dns_google/dns_google.py @@ -108,6 +108,8 @@ 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: @@ -176,6 +178,8 @@ class _GoogleClient(object): 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", @@ -216,23 +220,32 @@ class _GoogleClient(object): """ 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 - :rtype: `list` of `string` + :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) - response = request.execute() # Add dot as the API returns absolute domains record_name += "." - if response: - for rr in response["rrsets"]: - if rr["name"] == record_name and rr["type"] == "TXT": - return rr["rrdatas"] - return [] + 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): """ 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 3291b2c3a..afab847cf 100644 --- a/certbot-dns-google/certbot_dns_google/dns_google_test.py +++ b/certbot-dns-google/certbot_dns_google/dns_google_test.py @@ -270,7 +270,19 @@ class GoogleClientTest(unittest.TestCase): 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, []) + 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}]}]) + 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/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), From e0ae356aa35adf22d154113e06dd01409df93bba Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Wed, 7 Mar 2018 09:10:47 -0800 Subject: [PATCH 7/8] Upgrade pipstrap to 1.5.1 (#5681) * upgrade pipstrap to 1.5.1 * build leauto --- letsencrypt-auto-source/letsencrypt-auto | 38 +++++++++------------- letsencrypt-auto-source/pieces/pipstrap.py | 38 +++++++++------------- 2 files changed, 30 insertions(+), 46 deletions(-) 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: From f4bac423fb794c14e426defecc492306ea53cbc4 Mon Sep 17 00:00:00 2001 From: sydneyli Date: Wed, 7 Mar 2018 15:09:47 -0800 Subject: [PATCH 8/8] fix(acme): client._revoke sends default content_type (#5687) --- acme/acme/client.py | 3 +-- acme/acme/client_test.py | 6 ++---- 2 files changed, 3 insertions(+), 6 deletions(-) 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):