From 77810fefb95769467c24a35fa49a6182289bba07 Mon Sep 17 00:00:00 2001 From: Fabricio Duarte Date: Fri, 21 Aug 2026 17:08:18 -0300 Subject: [PATCH 1/2] dns: prevent cross-tenant record shadowing --- .../dns/DnsProviderManagerImpl.java | 42 +++++++++- .../apache/cloudstack/dns/dao/DnsZoneDao.java | 2 + .../cloudstack/dns/dao/DnsZoneDaoImpl.java | 24 ++++-- .../dns/DnsProviderManagerImplTest.java | 76 +++++++++++++++++++ 4 files changed, 135 insertions(+), 9 deletions(-) diff --git a/server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java b/server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java index b451da1baf72..e1ace5d7a3cf 100644 --- a/server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java +++ b/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.} 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 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); diff --git a/server/src/main/java/org/apache/cloudstack/dns/dao/DnsZoneDao.java b/server/src/main/java/org/apache/cloudstack/dns/dao/DnsZoneDao.java index 43bf60818d54..adcf35421898 100644 --- a/server/src/main/java/org/apache/cloudstack/dns/dao/DnsZoneDao.java +++ b/server/src/main/java/org/apache/cloudstack/dns/dao/DnsZoneDao.java @@ -34,4 +34,6 @@ Pair, Integer> searchZones(Long id, Long accountId, List o String keyword, Filter filter); List findDnsZoneIdsByServerId(long dnsServerId); + + List listByDnsServerId(long dnsServerId); } diff --git a/server/src/main/java/org/apache/cloudstack/dns/dao/DnsZoneDaoImpl.java b/server/src/main/java/org/apache/cloudstack/dns/dao/DnsZoneDaoImpl.java index 2487f2fee20f..c8fdf8b84524 100644 --- a/server/src/main/java/org/apache/cloudstack/dns/dao/DnsZoneDaoImpl.java +++ b/server/src/main/java/org/apache/cloudstack/dns/dao/DnsZoneDaoImpl.java @@ -35,17 +35,22 @@ @Component public class DnsZoneDaoImpl extends GenericDaoBase implements DnsZoneDao { - SearchBuilder DnsServerSearch; + SearchBuilder DnsServerZoneIdsSearch; + SearchBuilder DnsServerZonesSearch; SearchBuilder AccountSearch; SearchBuilder 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, Integer> searchZones(Long id, Long accountId, List< return searchAndCount(sc, filter); } + @Override + public List listByDnsServerId(long dnsServerId) { + SearchCriteria sc = DnsServerZonesSearch.create(); + sc.setParameters(ApiConstants.DNS_SERVER_ID, dnsServerId); + return listBy(sc); + } + public List findDnsZoneIdsByServerId(long dnsServerId) { - SearchCriteria sc = DnsServerSearch.create(); + SearchCriteria sc = DnsServerZoneIdsSearch.create(); sc.setParameters(ApiConstants.DNS_SERVER_ID, dnsServerId); List dnsZones = listBy(sc); if (CollectionUtils.isEmpty(dnsZones)) { diff --git a/server/src/test/java/org/apache/cloudstack/dns/DnsProviderManagerImplTest.java b/server/src/test/java/org/apache/cloudstack/dns/DnsProviderManagerImplTest.java index 309f5e5d9cfd..8ce31d4e7c72 100644 --- a/server/src/test/java/org/apache/cloudstack/dns/DnsProviderManagerImplTest.java +++ b/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( From dde26535ae931ecd79d7e3b5e18983164401c583 Mon Sep 17 00:00:00 2001 From: Fabricio Duarte Date: Fri, 21 Aug 2026 18:06:42 -0300 Subject: [PATCH 2/2] address ui --- ui/src/views/network/dns/AddDnsServer.vue | 7 +++++-- ui/src/views/network/dns/UpdateDnsServer.vue | 7 +++++-- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/ui/src/views/network/dns/AddDnsServer.vue b/ui/src/views/network/dns/AddDnsServer.vue index 3950c85248d0..054b0e37863a 100644 --- a/ui/src/views/network/dns/AddDnsServer.vue +++ b/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')) diff --git a/ui/src/views/network/dns/UpdateDnsServer.vue b/ui/src/views/network/dns/UpdateDnsServer.vue index b8bd4f352e47..728151a21538 100644 --- a/ui/src/views/network/dns/UpdateDnsServer.vue +++ b/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'))