diff --git a/certbot-dns-google/certbot_dns_google/_internal/dns_google.py b/certbot-dns-google/certbot_dns_google/_internal/dns_google.py index cd4b2d2d5..4b0d91463 100644 --- a/certbot-dns-google/certbot_dns_google/_internal/dns_google.py +++ b/certbot-dns-google/certbot_dns_google/_internal/dns_google.py @@ -118,10 +118,13 @@ class _GoogleClient(object): record_contents = self.get_existing_txt_rrset(zone_id, record_name) if record_contents is None: - record_contents = [] - add_records = record_contents[:] + # If it wasn't possible to fetch the records at this label (missing .list permission), + # assume there aren't any (#5678). If there are actually records here, this will fail + # with HTTP 409/412 API errors. + record_contents = {"rrdatas": []} + add_records = record_contents["rrdatas"][:] - if "\""+record_content+"\"" in record_contents: + if "\""+record_content+"\"" in record_contents["rrdatas"]: # The process was interrupted previously and validation token exists return @@ -140,15 +143,15 @@ class _GoogleClient(object): ], } - if record_contents: + if record_contents["rrdatas"]: # 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, + "rrdatas": record_contents["rrdatas"], + "ttl": record_contents["ttl"], }, ] @@ -188,7 +191,10 @@ class _GoogleClient(object): record_contents = self.get_existing_txt_rrset(zone_id, record_name) if record_contents is None: - record_contents = ["\"" + record_content + "\""] + # If it wasn't possible to fetch the records at this label (missing .list permission), + # assume there aren't any (#5678). If there are actually records here, this will fail + # with HTTP 409/412 API errors. + record_contents = {"rrdatas": ["\"" + record_content + "\""], "ttl": record_ttl} data = { "kind": "dns#change", @@ -197,14 +203,15 @@ class _GoogleClient(object): "kind": "dns#resourceRecordSet", "type": "TXT", "name": record_name + ".", - "rrdatas": record_contents, - "ttl": record_ttl, + "rrdatas": record_contents["rrdatas"], + "ttl": record_contents["ttl"], }, ], } # Remove the record being deleted from the list - readd_contents = [r for r in record_contents if r != "\"" + record_content + "\""] + readd_contents = [r for r in record_contents["rrdatas"] + if r != "\"" + record_content + "\""] if readd_contents: # We need to remove old records in the same request data["additions"] = [ @@ -213,7 +220,7 @@ class _GoogleClient(object): "type": "TXT", "name": record_name + ".", "rrdatas": readd_contents, - "ttl": record_ttl, + "ttl": record_contents["ttl"], }, ] @@ -235,8 +242,8 @@ class _GoogleClient(object): :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` + :returns: The resourceRecordSet corresponding to `record_name` or None + :rtype: `resourceRecordSet ` or `None` # pylint: disable=line-too-long """ rrs_request = self.dns.resourceRecordSets() @@ -252,7 +259,7 @@ class _GoogleClient(object): logger.debug("Error was:", exc_info=True) else: if response and response["rrsets"]: - return response["rrsets"][0]["rrdatas"] + return response["rrsets"][0] return None def _find_managed_zone_id(self, domain): diff --git a/certbot-dns-google/tests/dns_google_test.py b/certbot-dns-google/tests/dns_google_test.py index bcb6bb80f..396a6c8bd 100644 --- a/certbot-dns-google/tests/dns_google_test.py +++ b/certbot-dns-google/tests/dns_google_test.py @@ -90,7 +90,7 @@ class GoogleClientTest(unittest.TestCase): response = {"rrsets": []} if name == "_acme-challenge.example.org.": response = {"rrsets": [{"name": "_acme-challenge.example.org.", "type": "TXT", - "rrdatas": ["\"example-txt-contents\""]}]} + "rrdatas": ["\"example-txt-contents\""], "ttl": 60}]} mock_return = mock.MagicMock() mock_return.execute.return_value = response mock_return.execute.side_effect = rrs_list_side_effect @@ -180,11 +180,29 @@ class GoogleClientTest(unittest.TestCase): # pylint: disable=line-too-long mock_get_rrs = "certbot_dns_google._internal.dns_google._GoogleClient.get_existing_txt_rrset" with mock.patch(mock_get_rrs) as mock_rrs: - mock_rrs.return_value = ["sample-txt-contents"] + mock_rrs.return_value = {"rrdatas": ["sample-txt-contents"], "ttl": self.record_ttl} 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"]) + deletions = changes.create.call_args_list[0][1]["body"]["deletions"][0] + self.assertTrue("sample-txt-contents" in deletions["rrdatas"]) + self.assertEqual(self.record_ttl, deletions["ttl"]) + + @mock.patch('oauth2client.service_account.ServiceAccountCredentials.from_json_keyfile_name') + @mock.patch('certbot_dns_google._internal.dns_google.open', + mock.mock_open(read_data='{"project_id": "' + PROJECT_ID + '"}'), create=True) + def test_add_txt_record_delete_old_ttl_case(self, unused_credential_mock): + client, changes = self._setUp_client_with_mock( + [{'managedZones': [{'id': self.zone}]}]) + # pylint: disable=line-too-long + mock_get_rrs = "certbot_dns_google._internal.dns_google._GoogleClient.get_existing_txt_rrset" + with mock.patch(mock_get_rrs) as mock_rrs: + custom_ttl = 300 + mock_rrs.return_value = {"rrdatas": ["sample-txt-contents"], "ttl": custom_ttl} + client.add_txt_record(DOMAIN, self.record_name, self.record_content, self.record_ttl) + self.assertTrue(changes.create.called) + deletions = changes.create.call_args_list[0][1]["body"]["deletions"][0] + self.assertTrue("sample-txt-contents" in deletions["rrdatas"]) + self.assertEqual(custom_ttl, deletions["ttl"]) #otherwise HTTP 412 @mock.patch('oauth2client.service_account.ServiceAccountCredentials.from_json_keyfile_name') @mock.patch('certbot_dns_google._internal.dns_google.open', @@ -228,14 +246,13 @@ class GoogleClientTest(unittest.TestCase): @mock.patch('oauth2client.service_account.ServiceAccountCredentials.from_json_keyfile_name') @mock.patch('certbot_dns_google._internal.dns_google.open', mock.mock_open(read_data='{"project_id": "' + PROJECT_ID + '"}'), create=True) - def test_del_txt_record(self, unused_credential_mock): + def test_del_txt_record_multi_rrdatas(self, unused_credential_mock): client, changes = self._setUp_client_with_mock([{'managedZones': [{'id': self.zone}]}]) - # pylint: disable=line-too-long mock_get_rrs = "certbot_dns_google._internal.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\""] + mock_rrs.return_value = {"rrdatas": ["\"sample-txt-contents\"", + "\"example-txt-contents\""], "ttl": self.record_ttl} client.del_txt_record(DOMAIN, "_acme-challenge.example.org", "example-txt-contents", self.record_ttl) @@ -268,19 +285,48 @@ class GoogleClientTest(unittest.TestCase): @mock.patch('oauth2client.service_account.ServiceAccountCredentials.from_json_keyfile_name') @mock.patch('certbot_dns_google._internal.dns_google.open', mock.mock_open(read_data='{"project_id": "' + PROJECT_ID + '"}'), create=True) - def test_del_txt_record_error_during_zone_lookup(self, unused_credential_mock): - client, unused_changes = self._setUp_client_with_mock(API_ERROR) + def test_del_txt_record_single_rrdatas(self, unused_credential_mock): + client, changes = self._setUp_client_with_mock([{'managedZones': [{'id': self.zone}]}]) + # pylint: disable=line-too-long + mock_get_rrs = "certbot_dns_google._internal.dns_google._GoogleClient.get_existing_txt_rrset" + with mock.patch(mock_get_rrs) as mock_rrs: + mock_rrs.return_value = {"rrdatas": ["\"example-txt-contents\""], "ttl": self.record_ttl} + client.del_txt_record(DOMAIN, "_acme-challenge.example.org", + "example-txt-contents", self.record_ttl) + expected_body = { + "kind": "dns#change", + "deletions": [ + { + "kind": "dns#resourceRecordSet", + "type": "TXT", + "name": "_acme-challenge.example.org.", + "rrdatas": ["\"example-txt-contents\""], + "ttl": self.record_ttl, + }, + ], + } + + changes.create.assert_called_with(body=expected_body, + managedZone=self.zone, + project=PROJECT_ID) + + @mock.patch('oauth2client.service_account.ServiceAccountCredentials.from_json_keyfile_name') + @mock.patch('certbot_dns_google._internal.dns_google.open', + mock.mock_open(read_data='{"project_id": "' + PROJECT_ID + '"}'), create=True) + def test_del_txt_record_error_during_zone_lookup(self, unused_credential_mock): + client, changes = self._setUp_client_with_mock(API_ERROR) client.del_txt_record(DOMAIN, self.record_name, self.record_content, self.record_ttl) + changes.create.assert_not_called() @mock.patch('oauth2client.service_account.ServiceAccountCredentials.from_json_keyfile_name') @mock.patch('certbot_dns_google._internal.dns_google.open', mock.mock_open(read_data='{"project_id": "' + PROJECT_ID + '"}'), create=True) def test_del_txt_record_zone_not_found(self, unused_credential_mock): - client, unused_changes = self._setUp_client_with_mock([{'managedZones': []}, + client, changes = self._setUp_client_with_mock([{'managedZones': []}, {'managedZones': []}]) - client.del_txt_record(DOMAIN, self.record_name, self.record_content, self.record_ttl) + changes.create.assert_not_called() @mock.patch('oauth2client.service_account.ServiceAccountCredentials.from_json_keyfile_name') @mock.patch('certbot_dns_google._internal.dns_google.open', @@ -299,7 +345,8 @@ class GoogleClientTest(unittest.TestCase): [{'managedZones': [{'id': self.zone}]}]) # Record name mocked in setUp found = client.get_existing_txt_rrset(self.zone, "_acme-challenge.example.org") - self.assertEqual(found, ["\"example-txt-contents\""]) + self.assertEqual(found["rrdatas"], ["\"example-txt-contents\""]) + self.assertEqual(found["ttl"], 60) @mock.patch('oauth2client.service_account.ServiceAccountCredentials.from_json_keyfile_name') @mock.patch('certbot_dns_google._internal.dns_google.open', @@ -310,6 +357,16 @@ class GoogleClientTest(unittest.TestCase): not_found = client.get_existing_txt_rrset(self.zone, "nonexistent.tld") self.assertEqual(not_found, None) + @mock.patch('oauth2client.service_account.ServiceAccountCredentials.from_json_keyfile_name') + @mock.patch('certbot_dns_google._internal.dns_google.open', + mock.mock_open(read_data='{"project_id": "' + PROJECT_ID + '"}'), create=True) + def test_get_existing_with_error(self, unused_credential_mock): + client, unused_changes = self._setUp_client_with_mock( + [{'managedZones': [{'id': self.zone}]}], API_ERROR) + # Record name mocked in setUp + found = client.get_existing_txt_rrset(self.zone, "_acme-challenge.example.org") + self.assertEqual(found, None) + @mock.patch('oauth2client.service_account.ServiceAccountCredentials.from_json_keyfile_name') @mock.patch('certbot_dns_google._internal.dns_google.open', mock.mock_open(read_data='{"project_id": "' + PROJECT_ID + '"}'), create=True) diff --git a/certbot/CHANGELOG.md b/certbot/CHANGELOG.md index 8bfee52be..4c8de11af 100644 --- a/certbot/CHANGELOG.md +++ b/certbot/CHANGELOG.md @@ -22,7 +22,8 @@ Certbot adheres to [Semantic Versioning](https://semver.org/). * The Certbot snap no longer loads packages installed via `pip install --user`. This was unintended and DNS plugins should be installed via `snap` instead. -* `certbot-dns-google` would sometimes crash with HTTP 409/412 errors when used with very large zones (#6036) +* `certbot-dns-google` would sometimes crash with HTTP 409/412 errors when used with very large zones. See [#6036](https://github.com/certbot/certbot/issues/6036). +* `certbot-dns-google` would sometimes crash with an HTTP 412 error if preexisting records had an unexpected TTL, i.e.: different than Certbot's default TTL for this plugin. See [#8551](https://github.com/certbot/certbot/issues/8551). More details about these changes can be found on our GitHub repo.