From b1978ff18837e40d16eedf2090330af53d8ceaa5 Mon Sep 17 00:00:00 2001 From: Paulo Dias <44772900+paulojmdias@users.noreply.github.com> Date: Sun, 27 Aug 2023 00:19:38 +0100 Subject: [PATCH] dns-google: fix condition to don't use private dns zones (#9744) * dns-google: fix condition to don't use private dns zones * update MD * Fix condition * fix condition * update testdata * fix identation * update tests * update changelog * Update dns_google.py * add test for split horizon dns google * add dnsName to managed zones --- AUTHORS.md | 1 + .../_internal/dns_google.py | 2 +- .../_internal/tests/dns_google_test.py | 47 ++++++++++++++----- .../_internal/tests/testdata/discovery.json | 5 ++ certbot/CHANGELOG.md | 1 + 5 files changed, 42 insertions(+), 14 deletions(-) diff --git a/AUTHORS.md b/AUTHORS.md index 88060f42a..f3dca7c69 100644 --- a/AUTHORS.md +++ b/AUTHORS.md @@ -209,6 +209,7 @@ Authors * [Patrick Heppler](https://github.com/PatrickHeppler) * [Paul Buonopane](https://github.com/Zenexer) * [Paul Feitzinger](https://github.com/pfeyz) +* [Paulo Dias](https://github.com/paulojmdias) * [Pavan Gupta](https://github.com/pavgup) * [Pavel Pavlov](https://github.com/ghost355) * [Peter Conrad](https://github.com/pconrad-fb) 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 c2140db14..5fca3f4e6 100644 --- a/certbot-dns-google/certbot_dns_google/_internal/dns_google.py +++ b/certbot-dns-google/certbot_dns_google/_internal/dns_google.py @@ -306,7 +306,7 @@ class _GoogleClient: for zone in zones: zone_id = zone['id'] - if 'privateVisibilityConfig' not in zone: + if zone['visibility'] == "public": logger.debug('Found id of %s for %s using name %s', zone_id, domain, zone_name) return zone_id diff --git a/certbot-dns-google/certbot_dns_google/_internal/tests/dns_google_test.py b/certbot-dns-google/certbot_dns_google/_internal/tests/dns_google_test.py index 445c4110f..65315f23f 100644 --- a/certbot-dns-google/certbot_dns_google/_internal/tests/dns_google_test.py +++ b/certbot-dns-google/certbot_dns_google/_internal/tests/dns_google_test.py @@ -88,6 +88,7 @@ class GoogleClientTest(unittest.TestCase): record_ttl = 42 zone = "ZONE_ID" change = "an-id" + visibility = "public" def _setUp_client_with_mock(self, zone_request_side_effect, rrs_list_side_effect=None): from certbot_dns_google._internal.dns_google import _GoogleClient @@ -183,7 +184,7 @@ class GoogleClientTest(unittest.TestCase): def test_add_txt_record(self, credential_mock): credential_mock.return_value = (mock.MagicMock(), PROJECT_ID) - client, changes = self._setUp_client_with_mock([{'managedZones': [{'id': self.zone}]}]) + client, changes = self._setUp_client_with_mock([{'managedZones': [{'id': self.zone, 'visibility': self.visibility}]}]) credential_mock.assert_called_once_with('/not/a/real/path.json', scopes=SCOPES) client.add_txt_record(DOMAIN, self.record_name, self.record_content, self.record_ttl) @@ -211,7 +212,7 @@ class GoogleClientTest(unittest.TestCase): def test_add_txt_record_and_poll(self, credential_mock): credential_mock.return_value = (mock.MagicMock(), PROJECT_ID) - client, changes = self._setUp_client_with_mock([{'managedZones': [{'id': self.zone}]}]) + client, changes = self._setUp_client_with_mock([{'managedZones': [{'id': self.zone, 'visibility': self.visibility}]}]) changes.create.return_value.execute.return_value = {'status': 'pending', 'id': self.change} changes.get.return_value.execute.return_value = {'status': 'done'} @@ -225,6 +226,26 @@ class GoogleClientTest(unittest.TestCase): managedZone=self.zone, project=PROJECT_ID) + @mock.patch('google.auth.load_credentials_from_file') + @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_and_poll_split_horizon(self, credential_mock): + credential_mock.return_value = (mock.MagicMock(), PROJECT_ID) + + client, changes = self._setUp_client_with_mock([{'managedZones': [{'id': '{zone}-private'.format(zone=self.zone), 'dnsName': DOMAIN, 'visibility': 'private'},{'id': '{zone}-public'.format(zone=self.zone), 'dnsName': DOMAIN, 'visibility': self.visibility}]}]) + changes.create.return_value.execute.return_value = {'status': 'pending', 'id': self.change} + changes.get.return_value.execute.return_value = {'status': 'done'} + + client.add_txt_record(DOMAIN, self.record_name, self.record_content, self.record_ttl) + + changes.create.assert_called_with(body=mock.ANY, + managedZone='{zone}-public'.format(zone=self.zone), + project=PROJECT_ID) + + changes.get.assert_called_with(changeId=self.change, + managedZone='{zone}-public'.format(zone=self.zone), + project=PROJECT_ID) + @mock.patch('google.auth.load_credentials_from_file') @mock.patch('certbot_dns_google._internal.dns_google.open', mock.mock_open(read_data='{"project_id": "' + PROJECT_ID + '"}'), create=True) @@ -232,7 +253,7 @@ class GoogleClientTest(unittest.TestCase): credential_mock.return_value = (mock.MagicMock(), PROJECT_ID) client, changes = self._setUp_client_with_mock( - [{'managedZones': [{'id': self.zone}]}]) + [{'managedZones': [{'id': self.zone, 'visibility': self.visibility}]}]) # 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: @@ -250,7 +271,7 @@ class GoogleClientTest(unittest.TestCase): credential_mock.return_value = (mock.MagicMock(), PROJECT_ID) client, changes = self._setUp_client_with_mock( - [{'managedZones': [{'id': self.zone}]}]) + [{'managedZones': [{'id': self.zone, 'visibility': self.visibility}]}]) # 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: @@ -269,7 +290,7 @@ class GoogleClientTest(unittest.TestCase): credential_mock.return_value = (mock.MagicMock(), PROJECT_ID) client, changes = self._setUp_client_with_mock( - [{'managedZones': [{'id': self.zone}]}]) + [{'managedZones': [{'id': self.zone, 'visibility': self.visibility}]}]) client.add_txt_record(DOMAIN, "_acme-challenge.example.org", "example-txt-contents", self.record_ttl) assert changes.create.called is False @@ -303,7 +324,7 @@ class GoogleClientTest(unittest.TestCase): def test_add_txt_record_error_during_add(self, credential_mock): credential_mock.return_value = (mock.MagicMock(), PROJECT_ID) - client, changes = self._setUp_client_with_mock([{'managedZones': [{'id': self.zone}]}]) + client, changes = self._setUp_client_with_mock([{'managedZones': [{'id': self.zone, 'visibility': self.visibility}]}]) changes.create.side_effect = API_ERROR with pytest.raises(errors.PluginError): @@ -315,7 +336,7 @@ class GoogleClientTest(unittest.TestCase): def test_del_txt_record_multi_rrdatas(self, credential_mock): credential_mock.return_value = (mock.MagicMock(), PROJECT_ID) - client, changes = self._setUp_client_with_mock([{'managedZones': [{'id': self.zone}]}]) + client, changes = self._setUp_client_with_mock([{'managedZones': [{'id': self.zone, 'visibility': self.visibility}]}]) # 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: @@ -356,7 +377,7 @@ class GoogleClientTest(unittest.TestCase): def test_del_txt_record_single_rrdatas(self, credential_mock): credential_mock.return_value = (mock.MagicMock(), PROJECT_ID) - client, changes = self._setUp_client_with_mock([{'managedZones': [{'id': self.zone}]}]) + client, changes = self._setUp_client_with_mock([{'managedZones': [{'id': self.zone, 'visibility': self.visibility}]}]) # 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: @@ -408,7 +429,7 @@ class GoogleClientTest(unittest.TestCase): def test_del_txt_record_error_during_delete(self, credential_mock): credential_mock.return_value = (mock.MagicMock(), PROJECT_ID) - client, changes = self._setUp_client_with_mock([{'managedZones': [{'id': self.zone}]}]) + client, changes = self._setUp_client_with_mock([{'managedZones': [{'id': self.zone, 'visibility': self.visibility}]}]) changes.create.side_effect = API_ERROR client.del_txt_record(DOMAIN, self.record_name, self.record_content, self.record_ttl) @@ -420,7 +441,7 @@ class GoogleClientTest(unittest.TestCase): credential_mock.return_value = (mock.MagicMock(), PROJECT_ID) client, unused_changes = self._setUp_client_with_mock( - [{'managedZones': [{'id': self.zone}]}]) + [{'managedZones': [{'id': self.zone, 'visibility': self.visibility}]}]) # Record name mocked in setUp found = client.get_existing_txt_rrset(self.zone, "_acme-challenge.example.org") assert found["rrdatas"] == ["\"example-txt-contents\""] @@ -433,7 +454,7 @@ class GoogleClientTest(unittest.TestCase): credential_mock.return_value = (mock.MagicMock(), PROJECT_ID) client, unused_changes = self._setUp_client_with_mock( - [{'managedZones': [{'id': self.zone}]}]) + [{'managedZones': [{'id': self.zone, 'visibility': self.visibility}]}]) not_found = client.get_existing_txt_rrset(self.zone, "nonexistent.tld") assert not_found is None @@ -444,7 +465,7 @@ class GoogleClientTest(unittest.TestCase): credential_mock.return_value = (mock.MagicMock(), PROJECT_ID) client, unused_changes = self._setUp_client_with_mock( - [{'managedZones': [{'id': self.zone}]}], API_ERROR) + [{'managedZones': [{'id': self.zone, 'visibility': self.visibility}]}], API_ERROR) # Record name mocked in setUp found = client.get_existing_txt_rrset(self.zone, "_acme-challenge.example.org") assert found is None @@ -456,7 +477,7 @@ class GoogleClientTest(unittest.TestCase): credential_mock.return_value = (mock.MagicMock(), PROJECT_ID) client, unused_changes = self._setUp_client_with_mock( - [{'managedZones': [{'id': self.zone}]}], API_ERROR) + [{'managedZones': [{'id': self.zone, 'visibility': self.visibility}]}], API_ERROR) rrset = client.get_existing_txt_rrset(self.zone, "_acme-challenge.example.org") assert not rrset diff --git a/certbot-dns-google/certbot_dns_google/_internal/tests/testdata/discovery.json b/certbot-dns-google/certbot_dns_google/_internal/tests/testdata/discovery.json index 79a406645..9d1f3c319 100644 --- a/certbot-dns-google/certbot_dns_google/_internal/tests/testdata/discovery.json +++ b/certbot-dns-google/certbot_dns_google/_internal/tests/testdata/discovery.json @@ -389,6 +389,11 @@ "items": { "type": "string" } + }, + "visibility": { + "type": "string", + "description": "The zone's visibility: public zones are exposed to the Internet, while private zones are visible only to Virtual Private Cloud resources.", + "default": "public" } } }, diff --git a/certbot/CHANGELOG.md b/certbot/CHANGELOG.md index 7eae5a0a3..2f136c50d 100644 --- a/certbot/CHANGELOG.md +++ b/certbot/CHANGELOG.md @@ -18,6 +18,7 @@ Certbot adheres to [Semantic Versioning](https://semver.org/). ### Fixed * Do not call deprecated datetime.utcnow() and datetime.utcfromtimestamp() +* Filter zones in `certbot-dns-google` to avoid usage of private DNS zones to create records More details about these changes can be found on our GitHub repo.