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
certbot · Aug 26, 2023 · b1978ff · b1978ff
1 parent 579b39d
commit b1978ff
Show file tree

Hide file tree

Showing 5 changed files with 42 additions and 14 deletions.
diff --git 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
@@ -306,7 +306,7 @@ def _find_managed_zone_id(self, domain: str) -> str:
 
             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
@@ -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 @@ def test_client_with_project_id(self, credential_mock, unused_discovery_mock):
     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 @@ def test_add_txt_record(self, credential_mock):
     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,14 +226,34 @@ def test_add_txt_record_and_poll(self, credential_mock):
                                             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)
     def test_add_txt_record_delete_old(self, credential_mock):
         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 @@ def test_add_txt_record_delete_old_ttl_case(self, credential_mock):
         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 @@ def test_add_txt_record_noop(self, credential_mock):
         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 @@ def test_add_txt_record_zone_not_found(self, credential_mock):
     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 @@ def test_add_txt_record_error_during_add(self, credential_mock):
     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 @@ def test_del_txt_record_multi_rrdatas(self, credential_mock):
     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 @@ def test_del_txt_record_zone_not_found(self, credential_mock):
     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 @@ def test_get_existing_found(self, credential_mock):
         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 @@ def test_get_existing_not_found(self, credential_mock):
         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 @@ def test_get_existing_with_error(self, credential_mock):
         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 @@ def test_get_existing_fallback(self, credential_mock):
         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
@@ -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
@@ -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.