Skip to content

Commit 77810fe

Browse files
winterhazelwinterhazel
committed
dns: prevent cross-tenant record shadowing
1 parent ba67a62 commit 77810fe

4 files changed

Lines changed: 135 additions & 9 deletions

File tree

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

Lines changed: 39 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -184,6 +184,11 @@ public DnsServer addDnsServer(AddDnsServerCmd cmd) {
184184
publicDomainSuffix = DnsProviderUtil.normalizeDomainForDb(publicDomainSuffix);
185185
}
186186

187+
if (isDnsPublic && StringUtils.isBlank(publicDomainSuffix)) {
188+
throw new InvalidParameterValueException("A public DNS server requires a public domain suffix so that " +
189+
"DNS zones created by other accounts are contained under it.");
190+
}
191+
187192
DnsProviderType type = cmd.getProvider();
188193
DnsServerVO server = new DnsServerVO(cmd.getName(), cmd.getUrl(), cmd.getPort(), type,
189194
cmd.getDnsUserName(), cmd.getDnsApiKey(), isDnsPublic, publicDomainSuffix, cmd.getNameServers(),
@@ -273,12 +278,20 @@ public DnsServer updateDnsServer(UpdateDnsServerCmd cmd) {
273278
if (accountMgr.isRootAdmin(caller.getId()) || accountMgr.isDomainAdmin(caller.getId())) {
274279
if (cmd.isPublic() != null) {
275280
boolean isPublic = BooleanUtils.isTrue(cmd.isPublic());
276-
dnsServer.setPublicServer(isPublic);
277281

278282
String publicDomainSuffix = null;
279-
if (isPublic && StringUtils.isNotBlank(cmd.getPublicDomainSuffix())) {
280-
publicDomainSuffix = DnsProviderUtil.normalizeDomainForDb(cmd.getPublicDomainSuffix());
283+
if (isPublic) {
284+
if (StringUtils.isNotBlank(cmd.getPublicDomainSuffix())) {
285+
publicDomainSuffix = DnsProviderUtil.normalizeDomainForDb(cmd.getPublicDomainSuffix());
286+
} else {
287+
publicDomainSuffix = dnsServer.getPublicDomainSuffix();
288+
}
289+
if (StringUtils.isBlank(publicDomainSuffix)) {
290+
throw new InvalidParameterValueException("A public DNS server requires a public domain " +
291+
"suffix so that DNS zones created by other accounts are contained under it.");
292+
}
281293
}
294+
dnsServer.setPublicServer(isPublic);
282295
dnsServer.setPublicDomainSuffix(publicDomainSuffix);
283296
}
284297
}
@@ -590,6 +603,7 @@ public DnsZone allocateDnsZone(CreateDnsZoneCmd cmd) {
590603
throw new PermissionDeniedException("You do not have permission to use this DNS server.");
591604
}
592605
dnsZoneName = DnsProviderUtil.appendPublicSuffixToZone(dnsZoneName, server.getPublicDomainSuffix());
606+
checkDnsZoneNameConflictsAcrossAccounts(dnsZoneName, server.getId(), caller.getId());
593607
}
594608
DnsZone.ZoneType type = cmd.getType();
595609
DnsZoneVO existing = dnsZoneDao.findByNameServerAndType(dnsZoneName, server.getId(), type);
@@ -600,6 +614,28 @@ public DnsZone allocateDnsZone(CreateDnsZoneCmd cmd) {
600614
return dnsZoneDao.persist(dnsZoneVO);
601615
}
602616

617+
/**
618+
* Rejects a DNS zone name that is equal to, a DNS child of, or a DNS parent of an existing zone owned by a
619+
* different account on the same DNS server. Without this, a co-tenant could register e.g.
620+
* {@code www.victimzone.<suffix>} on a shared public server and shadow the victim's records in the
621+
* authoritative name server, since the more specific zone wins resolution.
622+
*/
623+
private void checkDnsZoneNameConflictsAcrossAccounts(String dnsZoneName, long dnsServerId, long callerAccountId) {
624+
String requestedName = dnsZoneName.toLowerCase();
625+
List<DnsZoneVO> existingZones = dnsZoneDao.listByDnsServerId(dnsServerId);
626+
for (DnsZoneVO zone : existingZones) {
627+
if (zone.getAccountId() == callerAccountId) {
628+
continue;
629+
}
630+
String existingName = zone.getName().toLowerCase();
631+
if (requestedName.equals(existingName) || requestedName.endsWith("." + existingName)
632+
|| existingName.endsWith("." + requestedName)) {
633+
throw new PermissionDeniedException(String.format("DNS zone name %s conflicts with an existing DNS " +
634+
"zone owned by another account on this DNS server.", dnsZoneName));
635+
}
636+
}
637+
}
638+
603639
@Override
604640
public DnsZone provisionDnsZone(long dnsZoneId, boolean isExistingZone) {
605641
DnsZoneVO dnsZone = dnsZoneDao.findById(dnsZoneId);

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,4 +34,6 @@ Pair<List<DnsZoneVO>, Integer> searchZones(Long id, Long accountId, List<Long> o
3434
String keyword, Filter filter);
3535

3636
List<Long> findDnsZoneIdsByServerId(long dnsServerId);
37+
38+
List<DnsZoneVO> listByDnsServerId(long dnsServerId);
3739
}

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

Lines changed: 18 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -35,17 +35,22 @@
3535

3636
@Component
3737
public class DnsZoneDaoImpl extends GenericDaoBase<DnsZoneVO, Long> implements DnsZoneDao {
38-
SearchBuilder<DnsZoneVO> DnsServerSearch;
38+
SearchBuilder<DnsZoneVO> DnsServerZoneIdsSearch;
39+
SearchBuilder<DnsZoneVO> DnsServerZonesSearch;
3940
SearchBuilder<DnsZoneVO> AccountSearch;
4041
SearchBuilder<DnsZoneVO> NameServerTypeSearch;
4142

4243
public DnsZoneDaoImpl() {
4344
super();
4445

45-
DnsServerSearch = createSearchBuilder();
46-
DnsServerSearch.selectFields(DnsServerSearch.entity().getId());
47-
DnsServerSearch.and(ApiConstants.DNS_SERVER_ID, DnsServerSearch.entity().getDnsServerId(), SearchCriteria.Op.EQ);
48-
DnsServerSearch.done();
46+
DnsServerZoneIdsSearch = createSearchBuilder();
47+
DnsServerZoneIdsSearch.selectFields(DnsServerZoneIdsSearch.entity().getId());
48+
DnsServerZoneIdsSearch.and(ApiConstants.DNS_SERVER_ID, DnsServerZoneIdsSearch.entity().getDnsServerId(), SearchCriteria.Op.EQ);
49+
DnsServerZoneIdsSearch.done();
50+
51+
DnsServerZonesSearch = createSearchBuilder();
52+
DnsServerZonesSearch.and(ApiConstants.DNS_SERVER_ID, DnsServerZonesSearch.entity().getDnsServerId(), SearchCriteria.Op.EQ);
53+
DnsServerZonesSearch.done();
4954

5055
AccountSearch = createSearchBuilder();
5156
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<
116121
return searchAndCount(sc, filter);
117122
}
118123

124+
@Override
125+
public List<DnsZoneVO> listByDnsServerId(long dnsServerId) {
126+
SearchCriteria<DnsZoneVO> sc = DnsServerZonesSearch.create();
127+
sc.setParameters(ApiConstants.DNS_SERVER_ID, dnsServerId);
128+
return listBy(sc);
129+
}
130+
119131
public List<Long> findDnsZoneIdsByServerId(long dnsServerId) {
120-
SearchCriteria<DnsZoneVO> sc = DnsServerSearch.create();
132+
SearchCriteria<DnsZoneVO> sc = DnsServerZoneIdsSearch.create();
121133
sc.setParameters(ApiConstants.DNS_SERVER_ID, dnsServerId);
122134
List<DnsZoneVO> dnsZones = listBy(sc);
123135
if (CollectionUtils.isEmpty(dnsZones)) {

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

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -262,6 +262,60 @@ public void testAllocateDnsZoneNonOwnerPrivateServer() {
262262
manager.allocateDnsZone(cmd);
263263
}
264264

265+
@Test(expected = PermissionDeniedException.class)
266+
public void testAllocateDnsZoneNonOwnerShadowingOtherAccountZoneRejected() {
267+
CreateDnsZoneCmd cmd = mock(CreateDnsZoneCmd.class);
268+
when(cmd.getName()).thenReturn("www.tenant1.cloud.example");
269+
when(cmd.getDnsServerId()).thenReturn(SERVER_ID);
270+
when(dnsServerDao.findById(SERVER_ID)).thenReturn(serverVO);
271+
Mockito.doReturn(SERVER_ID).when(serverVO).getId();
272+
Mockito.doReturn(ACCOUNT_ID + 99).when(serverVO).getAccountId(); // different owner
273+
Mockito.doReturn(true).when(serverVO).getPublicServer();
274+
Mockito.doReturn("cloud.example").when(serverVO).getPublicDomainSuffix();
275+
DnsZoneVO victimZone = new DnsZoneVO("tenant1.cloud.example", DnsZone.ZoneType.Public, SERVER_ID,
276+
ACCOUNT_ID + 50, DOMAIN_ID, "victim zone");
277+
when(dnsZoneDao.listByDnsServerId(SERVER_ID)).thenReturn(Collections.singletonList(victimZone));
278+
279+
manager.allocateDnsZone(cmd);
280+
}
281+
282+
@Test(expected = PermissionDeniedException.class)
283+
public void testAllocateDnsZoneNonOwnerParentOfOtherAccountZoneRejected() {
284+
CreateDnsZoneCmd cmd = mock(CreateDnsZoneCmd.class);
285+
when(cmd.getName()).thenReturn("tenant1.cloud.example");
286+
when(cmd.getDnsServerId()).thenReturn(SERVER_ID);
287+
when(dnsServerDao.findById(SERVER_ID)).thenReturn(serverVO);
288+
Mockito.doReturn(SERVER_ID).when(serverVO).getId();
289+
Mockito.doReturn(ACCOUNT_ID + 99).when(serverVO).getAccountId(); // different owner
290+
Mockito.doReturn(true).when(serverVO).getPublicServer();
291+
Mockito.doReturn("cloud.example").when(serverVO).getPublicDomainSuffix();
292+
DnsZoneVO victimZone = new DnsZoneVO("www.tenant1.cloud.example", DnsZone.ZoneType.Public, SERVER_ID,
293+
ACCOUNT_ID + 50, DOMAIN_ID, "victim zone");
294+
when(dnsZoneDao.listByDnsServerId(SERVER_ID)).thenReturn(Collections.singletonList(victimZone));
295+
296+
manager.allocateDnsZone(cmd);
297+
}
298+
299+
@Test
300+
public void testAllocateDnsZoneNonOwnerPublicServerSuccess() {
301+
CreateDnsZoneCmd cmd = mock(CreateDnsZoneCmd.class);
302+
when(cmd.getName()).thenReturn("tenant2.cloud.example");
303+
when(cmd.getDnsServerId()).thenReturn(SERVER_ID);
304+
when(cmd.getType()).thenReturn(DnsZone.ZoneType.Public);
305+
when(dnsServerDao.findById(SERVER_ID)).thenReturn(serverVO);
306+
Mockito.doReturn(SERVER_ID).when(serverVO).getId();
307+
Mockito.doReturn(ACCOUNT_ID + 99).when(serverVO).getAccountId(); // different owner
308+
Mockito.doReturn(true).when(serverVO).getPublicServer();
309+
Mockito.doReturn("cloud.example").when(serverVO).getPublicDomainSuffix();
310+
when(dnsZoneDao.listByDnsServerId(SERVER_ID)).thenReturn(Collections.emptyList());
311+
when(dnsZoneDao.findByNameServerAndType(anyString(), anyLong(), any())).thenReturn(null);
312+
when(dnsZoneDao.persist(any(DnsZoneVO.class))).thenReturn(zoneVO);
313+
314+
DnsZone result = manager.allocateDnsZone(cmd);
315+
assertNotNull(result);
316+
verify(dnsZoneDao).persist(Mockito.argThat(z -> "tenant2.cloud.example".equals(((DnsZoneVO) z).getName())));
317+
}
318+
265319
@Test(expected = CloudRuntimeException.class)
266320
public void testProvisionDnsZoneNotFound() {
267321
when(dnsZoneDao.findById(ZONE_ID)).thenReturn(null);
@@ -806,6 +860,28 @@ public void testAddDnsServerNormalUser() throws Exception {
806860
s -> !((DnsServerVO) s).getPublicServer() && ((DnsServerVO) s).getPublicDomainSuffix() == null));
807861
}
808862

863+
@Test(expected = InvalidParameterValueException.class)
864+
public void testAddDnsServerPublicWithoutSuffixRejected() {
865+
org.apache.cloudstack.api.command.user.dns.AddDnsServerCmd cmd = mock(
866+
org.apache.cloudstack.api.command.user.dns.AddDnsServerCmd.class);
867+
when(accountMgr.isRootAdmin(callerMock.getId())).thenReturn(true);
868+
when(cmd.getUrl()).thenReturn("http://newpdns:8081");
869+
when(cmd.isPublic()).thenReturn(true);
870+
when(dnsServerDao.findByUrlAndAccount(anyString(), anyLong())).thenReturn(null);
871+
manager.addDnsServer(cmd);
872+
}
873+
874+
@Test(expected = InvalidParameterValueException.class)
875+
public void testUpdateDnsServerPublicWithoutSuffixRejected() {
876+
org.apache.cloudstack.api.command.user.dns.UpdateDnsServerCmd cmd = mock(
877+
org.apache.cloudstack.api.command.user.dns.UpdateDnsServerCmd.class);
878+
when(cmd.getId()).thenReturn(SERVER_ID);
879+
when(cmd.isPublic()).thenReturn(true);
880+
when(accountMgr.isRootAdmin(callerMock.getId())).thenReturn(true);
881+
when(dnsServerDao.findById(SERVER_ID)).thenReturn(serverVO);
882+
manager.updateDnsServer(cmd);
883+
}
884+
809885
@Test(expected = CloudRuntimeException.class)
810886
public void testAddDnsServerValidationFailure() throws Exception {
811887
org.apache.cloudstack.api.command.user.dns.AddDnsServerCmd cmd = mock(

0 commit comments

Comments
 (0)