mirror of
https://github.com/certbot/certbot.git
synced 2026-01-26 07:41:33 +03:00
Fix TTL mismatch leading to HTTP 412 (#8549)
* Fix TTL mismatch leading to HTTP 412 This PR is a follow up from #8521 where we address the issue of potentially having a mismatch of TTL when executing a DNS change (transaction = deletion + additions). Let's say we have a record `foo.org 30 IN TXT foo-content` with TTL 30s, when creating challenge or cleaning we might need to perform a deletion operation in the transaction. Currently certbot would ask Google API to delete the foo record like this: `foo.org 60 in TXT foo-content` ignoring the record's original TTL and using 60s instead. This leads to HTTP 412 as Google would expect a perfect match of what we want to delete with what it is on the DNS. See also #8523 * remove ttl from default data to avoid confusions * Refactor tests and add a missing case This commit adds a test that covers the case when we are deleting a TXT record which contains a single rrdatas. Also, refactoring a couple of tests. * Make get_existing_txt_rrset documentation more precise about return value * Add missing assertions in tests. * fix linting issues * Mention fix on changelog * Explain fix around user impact * Explain what happens when no records are returned * Update certbot/CHANGELOG.md * Update certbot/CHANGELOG.md
This commit is contained in:
Lorenzo Fundaró
GitHub
@@ -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 <https://cloud.google.com/dns/docs/reference/v1/resourceRecordSets#resource>` 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):
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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.
|
||||
|
||||
|
||||
Reference in New Issue
Block a user