dns: prevent creation of cross-tenant conflicting DNS zones in the same server

server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java

@@ -184,6 +184,11 @@ public DnsServer addDnsServer(AddDnsServerCmd cmd) {
             publicDomainSuffix = DnsProviderUtil.normalizeDomainForDb(publicDomainSuffix);
         }
 
+        if (isDnsPublic && StringUtils.isBlank(publicDomainSuffix)) {
+            throw new InvalidParameterValueException("A public DNS server requires a public domain suffix so that " +
+                    "DNS zones created by other accounts are contained under it.");
+        }
+
         DnsProviderType type = cmd.getProvider();
         DnsServerVO server = new DnsServerVO(cmd.getName(), cmd.getUrl(), cmd.getPort(), type,
                 cmd.getDnsUserName(), cmd.getDnsApiKey(), isDnsPublic, publicDomainSuffix, cmd.getNameServers(),
@@ -273,12 +278,20 @@ public DnsServer updateDnsServer(UpdateDnsServerCmd cmd) {
         if (accountMgr.isRootAdmin(caller.getId()) || accountMgr.isDomainAdmin(caller.getId())) {
             if (cmd.isPublic() != null) {
                 boolean isPublic = BooleanUtils.isTrue(cmd.isPublic());
-                dnsServer.setPublicServer(isPublic);
 
                 String publicDomainSuffix = null;
-                if (isPublic && StringUtils.isNotBlank(cmd.getPublicDomainSuffix())) {
-                    publicDomainSuffix = DnsProviderUtil.normalizeDomainForDb(cmd.getPublicDomainSuffix());
+                if (isPublic) {
+                    if (StringUtils.isNotBlank(cmd.getPublicDomainSuffix())) {
+                        publicDomainSuffix = DnsProviderUtil.normalizeDomainForDb(cmd.getPublicDomainSuffix());
+                    } else {
+                        publicDomainSuffix = dnsServer.getPublicDomainSuffix();
+                    }
+                    if (StringUtils.isBlank(publicDomainSuffix)) {
+                        throw new InvalidParameterValueException("A public DNS server requires a public domain " +
+                                "suffix so that DNS zones created by other accounts are contained under it.");
+                    }
                 }
+                dnsServer.setPublicServer(isPublic);
                 dnsServer.setPublicDomainSuffix(publicDomainSuffix);
             }
         }
@@ -590,6 +603,7 @@ public DnsZone allocateDnsZone(CreateDnsZoneCmd cmd) {
                 throw new PermissionDeniedException("You do not have permission to use this DNS server.");
             }
             dnsZoneName = DnsProviderUtil.appendPublicSuffixToZone(dnsZoneName, server.getPublicDomainSuffix());
+            checkDnsZoneNameConflictsAcrossAccounts(dnsZoneName, server.getId(), caller.getId());
         }
         DnsZone.ZoneType type = cmd.getType();
         DnsZoneVO existing = dnsZoneDao.findByNameServerAndType(dnsZoneName, server.getId(), type);
@@ -600,6 +614,28 @@ public DnsZone allocateDnsZone(CreateDnsZoneCmd cmd) {
         return dnsZoneDao.persist(dnsZoneVO);
     }
 
+    /**
+     * Rejects a DNS zone name that is equal to, a DNS child of, or a DNS parent of an existing zone owned by a
+     * different account on the same DNS server. Without this, a co-tenant could register e.g.
+     * {@code www.victimzone.<suffix>} on a shared public server and shadow the victim's records in the
+     * authoritative name server, since the more specific zone wins resolution.
+     */
+    private void checkDnsZoneNameConflictsAcrossAccounts(String dnsZoneName, long dnsServerId, long callerAccountId) {
+        String requestedName = dnsZoneName.toLowerCase();
+        List<DnsZoneVO> existingZones = dnsZoneDao.listByDnsServerId(dnsServerId);
+        for (DnsZoneVO zone : existingZones) {
+            if (zone.getAccountId() == callerAccountId) {
+                continue;
+            }
+            String existingName = zone.getName().toLowerCase();
+            if (requestedName.equals(existingName) || requestedName.endsWith("." + existingName)
+                    || existingName.endsWith("." + requestedName)) {
+                throw new PermissionDeniedException(String.format("DNS zone name %s conflicts with an existing DNS " +
+                        "zone owned by another account on this DNS server.", dnsZoneName));
+            }
+        }
+    }
+
     @Override
     public DnsZone provisionDnsZone(long dnsZoneId, boolean isExistingZone) {
         DnsZoneVO dnsZone = dnsZoneDao.findById(dnsZoneId);

server/src/main/java/org/apache/cloudstack/dns/dao/DnsZoneDao.java

@@ -34,4 +34,6 @@ Pair<List<DnsZoneVO>, Integer> searchZones(Long id, Long accountId, List<Long> o
                                                String keyword, Filter filter);
 
     List<Long> findDnsZoneIdsByServerId(long dnsServerId);
+
+    List<DnsZoneVO> listByDnsServerId(long dnsServerId);
 }

server/src/main/java/org/apache/cloudstack/dns/dao/DnsZoneDaoImpl.java

@@ -35,17 +35,22 @@
 
 @Component
 public class DnsZoneDaoImpl extends GenericDaoBase<DnsZoneVO, Long> implements DnsZoneDao {
-    SearchBuilder<DnsZoneVO> DnsServerSearch;
+    SearchBuilder<DnsZoneVO> DnsServerZoneIdsSearch;
+    SearchBuilder<DnsZoneVO> DnsServerZonesSearch;
     SearchBuilder<DnsZoneVO> AccountSearch;
     SearchBuilder<DnsZoneVO> NameServerTypeSearch;
 
     public DnsZoneDaoImpl() {
         super();
 
-        DnsServerSearch  = createSearchBuilder();
-        DnsServerSearch.selectFields(DnsServerSearch.entity().getId());
-        DnsServerSearch.and(ApiConstants.DNS_SERVER_ID, DnsServerSearch.entity().getDnsServerId(), SearchCriteria.Op.EQ);
-        DnsServerSearch.done();
+        DnsServerZoneIdsSearch = createSearchBuilder();
+        DnsServerZoneIdsSearch.selectFields(DnsServerZoneIdsSearch.entity().getId());
+        DnsServerZoneIdsSearch.and(ApiConstants.DNS_SERVER_ID, DnsServerZoneIdsSearch.entity().getDnsServerId(), SearchCriteria.Op.EQ);
+        DnsServerZoneIdsSearch.done();
+
+        DnsServerZonesSearch = createSearchBuilder();
+        DnsServerZonesSearch.and(ApiConstants.DNS_SERVER_ID, DnsServerZonesSearch.entity().getDnsServerId(), SearchCriteria.Op.EQ);
+        DnsServerZonesSearch.done();
 
         AccountSearch = createSearchBuilder();
         AccountSearch.and(ApiConstants.ACCOUNT_ID, AccountSearch.entity().getAccountId(), SearchCriteria.Op.EQ);
@@ -116,8 +121,15 @@ public Pair<List<DnsZoneVO>, Integer> searchZones(Long id, Long accountId, List<
         return searchAndCount(sc, filter);
     }
 
+    @Override
+    public List<DnsZoneVO> listByDnsServerId(long dnsServerId) {
+        SearchCriteria<DnsZoneVO> sc = DnsServerZonesSearch.create();
+        sc.setParameters(ApiConstants.DNS_SERVER_ID, dnsServerId);
+        return listBy(sc);
+    }
+
     public List<Long> findDnsZoneIdsByServerId(long dnsServerId) {
-        SearchCriteria<DnsZoneVO> sc = DnsServerSearch.create();
+        SearchCriteria<DnsZoneVO> sc = DnsServerZoneIdsSearch.create();
         sc.setParameters(ApiConstants.DNS_SERVER_ID, dnsServerId);
         List<DnsZoneVO> dnsZones = listBy(sc);
         if (CollectionUtils.isEmpty(dnsZones)) {

server/src/test/java/org/apache/cloudstack/dns/DnsProviderManagerImplTest.java

@@ -262,6 +262,60 @@ public void testAllocateDnsZoneNonOwnerPrivateServer() {
         manager.allocateDnsZone(cmd);
     }
 
+    @Test(expected = PermissionDeniedException.class)
+    public void testAllocateDnsZoneNonOwnerShadowingOtherAccountZoneRejected() {
+        CreateDnsZoneCmd cmd = mock(CreateDnsZoneCmd.class);
+        when(cmd.getName()).thenReturn("www.tenant1.cloud.example");
+        when(cmd.getDnsServerId()).thenReturn(SERVER_ID);
+        when(dnsServerDao.findById(SERVER_ID)).thenReturn(serverVO);
+        Mockito.doReturn(SERVER_ID).when(serverVO).getId();
+        Mockito.doReturn(ACCOUNT_ID + 99).when(serverVO).getAccountId(); // different owner
+        Mockito.doReturn(true).when(serverVO).getPublicServer();
+        Mockito.doReturn("cloud.example").when(serverVO).getPublicDomainSuffix();
+        DnsZoneVO victimZone = new DnsZoneVO("tenant1.cloud.example", DnsZone.ZoneType.Public, SERVER_ID,
+                ACCOUNT_ID + 50, DOMAIN_ID, "victim zone");
+        when(dnsZoneDao.listByDnsServerId(SERVER_ID)).thenReturn(Collections.singletonList(victimZone));
+
+        manager.allocateDnsZone(cmd);
+    }
+
+    @Test(expected = PermissionDeniedException.class)
+    public void testAllocateDnsZoneNonOwnerParentOfOtherAccountZoneRejected() {
+        CreateDnsZoneCmd cmd = mock(CreateDnsZoneCmd.class);
+        when(cmd.getName()).thenReturn("tenant1.cloud.example");
+        when(cmd.getDnsServerId()).thenReturn(SERVER_ID);
+        when(dnsServerDao.findById(SERVER_ID)).thenReturn(serverVO);
+        Mockito.doReturn(SERVER_ID).when(serverVO).getId();
+        Mockito.doReturn(ACCOUNT_ID + 99).when(serverVO).getAccountId(); // different owner
+        Mockito.doReturn(true).when(serverVO).getPublicServer();
+        Mockito.doReturn("cloud.example").when(serverVO).getPublicDomainSuffix();
+        DnsZoneVO victimZone = new DnsZoneVO("www.tenant1.cloud.example", DnsZone.ZoneType.Public, SERVER_ID,
+                ACCOUNT_ID + 50, DOMAIN_ID, "victim zone");
+        when(dnsZoneDao.listByDnsServerId(SERVER_ID)).thenReturn(Collections.singletonList(victimZone));
+
+        manager.allocateDnsZone(cmd);
+    }
+
+    @Test
+    public void testAllocateDnsZoneNonOwnerPublicServerSuccess() {
+        CreateDnsZoneCmd cmd = mock(CreateDnsZoneCmd.class);
+        when(cmd.getName()).thenReturn("tenant2.cloud.example");
+        when(cmd.getDnsServerId()).thenReturn(SERVER_ID);
+        when(cmd.getType()).thenReturn(DnsZone.ZoneType.Public);
+        when(dnsServerDao.findById(SERVER_ID)).thenReturn(serverVO);
+        Mockito.doReturn(SERVER_ID).when(serverVO).getId();
+        Mockito.doReturn(ACCOUNT_ID + 99).when(serverVO).getAccountId(); // different owner
+        Mockito.doReturn(true).when(serverVO).getPublicServer();
+        Mockito.doReturn("cloud.example").when(serverVO).getPublicDomainSuffix();
+        when(dnsZoneDao.listByDnsServerId(SERVER_ID)).thenReturn(Collections.emptyList());
+        when(dnsZoneDao.findByNameServerAndType(anyString(), anyLong(), any())).thenReturn(null);
+        when(dnsZoneDao.persist(any(DnsZoneVO.class))).thenReturn(zoneVO);
+
+        DnsZone result = manager.allocateDnsZone(cmd);
+        assertNotNull(result);
+        verify(dnsZoneDao).persist(Mockito.argThat(z -> "tenant2.cloud.example".equals(((DnsZoneVO) z).getName())));
+    }
+
     @Test(expected = CloudRuntimeException.class)
     public void testProvisionDnsZoneNotFound() {
         when(dnsZoneDao.findById(ZONE_ID)).thenReturn(null);
@@ -806,6 +860,28 @@ public void testAddDnsServerNormalUser() throws Exception {
                 s -> !((DnsServerVO) s).getPublicServer() && ((DnsServerVO) s).getPublicDomainSuffix() == null));
     }
 
+    @Test(expected = InvalidParameterValueException.class)
+    public void testAddDnsServerPublicWithoutSuffixRejected() {
+        org.apache.cloudstack.api.command.user.dns.AddDnsServerCmd cmd = mock(
+                org.apache.cloudstack.api.command.user.dns.AddDnsServerCmd.class);
+        when(accountMgr.isRootAdmin(callerMock.getId())).thenReturn(true);
+        when(cmd.getUrl()).thenReturn("http://newpdns:8081");
+        when(cmd.isPublic()).thenReturn(true);
+        when(dnsServerDao.findByUrlAndAccount(anyString(), anyLong())).thenReturn(null);
+        manager.addDnsServer(cmd);
+    }
+
+    @Test(expected = InvalidParameterValueException.class)
+    public void testUpdateDnsServerPublicWithoutSuffixRejected() {
+        org.apache.cloudstack.api.command.user.dns.UpdateDnsServerCmd cmd = mock(
+                org.apache.cloudstack.api.command.user.dns.UpdateDnsServerCmd.class);
+        when(cmd.getId()).thenReturn(SERVER_ID);
+        when(cmd.isPublic()).thenReturn(true);
+        when(accountMgr.isRootAdmin(callerMock.getId())).thenReturn(true);
+        when(dnsServerDao.findById(SERVER_ID)).thenReturn(serverVO);
+        manager.updateDnsServer(cmd);
+    }
+
     @Test(expected = CloudRuntimeException.class)
     public void testAddDnsServerValidationFailure() throws Exception {
         org.apache.cloudstack.api.command.user.dns.AddDnsServerCmd cmd = mock(

ui/src/views/network/dns/AddDnsServer.vue

@@ -205,7 +205,10 @@ export default {
       ]
     }
     if (this.isAdminOrDomainAdmin()) {
-      this.rules.publicdomainsuffix = [{ validator: this.validatePublicDomainSuffix }]
+      this.rules.publicdomainsuffix = [{
+        required: true,
+        validator: this.validatePublicDomainSuffix
+      }]
     }
     this.fetchProviders()
   },
@@ -331,7 +334,7 @@ export default {
     validatePublicDomainSuffix (rule, value) {
       const normalized = value?.toLowerCase().trim()
       if (!normalized) {
-        return Promise.resolve()
+        return Promise.reject(new Error(this.$t('message.error.required.input')))
       }
       if (!FQDN_REGEX.test(normalized)) {
         return Promise.reject(new Error('Invalid domain suffix'))

ui/src/views/network/dns/UpdateDnsServer.vue

@@ -164,7 +164,10 @@ export default {
       ]
     }
     if (this.isAdminOrDomainAdmin()) {
-      this.rules.publicdomainsuffix = [{ validator: this.validatePublicDomainSuffix }]
+      this.rules.publicdomainsuffix = [{
+        required: true,
+        validator: this.validatePublicDomainSuffix
+      }]
     }
     this.form.name = this.resource.name
     this.form.url = this.resource.url
@@ -272,7 +275,7 @@ export default {
     validatePublicDomainSuffix (rule, value) {
       const normalized = value?.toLowerCase().trim()
       if (!normalized) {
-        return Promise.resolve()
+        return Promise.reject(new Error(this.$t('message.error.required.input')))
       }
       if (!FQDN_REGEX.test(normalized)) {
         return Promise.reject(new Error('Invalid domain suffix'))