diff --git a/api/src/main/java/org/apache/cloudstack/api/command/user/dns/AddDnsServerCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/user/dns/AddDnsServerCmd.java index 298ddd64a31c..21279b8719fa 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/user/dns/AddDnsServerCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/user/dns/AddDnsServerCmd.java @@ -46,7 +46,7 @@ requestHasSensitiveInfo = true, responseHasSensitiveInfo = false, since = "4.23.0", - authorized = {RoleType.Admin, RoleType.ResourceAdmin, RoleType.DomainAdmin, RoleType.User}) + authorized = {RoleType.Admin}) public class AddDnsServerCmd extends BaseCmd { ///////////////////////////////////////////////////// diff --git a/api/src/main/java/org/apache/cloudstack/api/command/user/dns/DeleteDnsServerCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/user/dns/DeleteDnsServerCmd.java index 099fc62f354c..cb001f69523c 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/user/dns/DeleteDnsServerCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/user/dns/DeleteDnsServerCmd.java @@ -40,7 +40,7 @@ entityType = {DnsServer.class}, requestHasSensitiveInfo = false, responseHasSensitiveInfo = false, since = "4.23.0", - authorized = {RoleType.Admin, RoleType.ResourceAdmin, RoleType.DomainAdmin, RoleType.User}) + authorized = {RoleType.Admin}) public class DeleteDnsServerCmd extends BaseAsyncCmd { ///////////////////////////////////////////////////// diff --git a/api/src/main/java/org/apache/cloudstack/api/command/user/dns/UpdateDnsServerCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/user/dns/UpdateDnsServerCmd.java index 6b790fa8ade8..7a84c54dc666 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/user/dns/UpdateDnsServerCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/user/dns/UpdateDnsServerCmd.java @@ -41,7 +41,7 @@ entityType = {DnsServer.class}, requestHasSensitiveInfo = true, responseHasSensitiveInfo = false, since = "4.23.0", - authorized = {RoleType.Admin, RoleType.ResourceAdmin, RoleType.DomainAdmin, RoleType.User}) + authorized = {RoleType.Admin}) public class UpdateDnsServerCmd extends BaseCmd { ///////////////////////////////////////////////////// diff --git a/engine/schema/src/main/resources/META-INF/db/views/nic_dns_view.sql b/engine/schema/src/main/resources/META-INF/db/views/cloud.nic_dns_view.sql similarity index 100% rename from engine/schema/src/main/resources/META-INF/db/views/nic_dns_view.sql rename to engine/schema/src/main/resources/META-INF/db/views/cloud.nic_dns_view.sql 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..0d08540d129a 100644 --- a/server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java +++ b/server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java @@ -96,6 +96,7 @@ import com.cloud.user.dao.AccountDao; import com.cloud.utils.Pair; import com.cloud.utils.StringUtils; +import com.cloud.utils.UriUtils; import com.cloud.utils.component.ManagerBase; import com.cloud.utils.component.PluggableService; import com.cloud.utils.db.Filter; @@ -107,9 +108,7 @@ import com.cloud.vm.Nic; import com.cloud.vm.VirtualMachine; import com.cloud.vm.VirtualMachineManager; -import com.cloud.vm.dao.NicDao; import com.cloud.vm.dao.NicDetailsDao; -import com.cloud.vm.dao.UserVmDao; import com.cloud.vm.dao.VMInstanceDao; @Component @@ -126,10 +125,6 @@ public class DnsProviderManagerImpl extends ManagerBase implements DnsProviderMa @Inject DnsZoneNetworkMapDao dnsZoneNetworkMapDao; @Inject - UserVmDao userVmDao; - @Inject - NicDao nicDao; - @Inject DomainDao domainDao; @Inject DnsZoneJoinDao dnsZoneJoinDao; @@ -162,14 +157,36 @@ private DnsProvider getProviderByType(DnsProviderType type) { throw new CloudRuntimeException("No plugin found for DNS provider type: " + type); } + /** + * Rejects a DNS provider URL that resolves to an illegal address before any provider client + * is given the chance to connect to it. See {@link UriUtils#validateUrl(String)} for the exact rules + * enforced (including the requirement that the URL declares an {@code http}/{@code https} scheme). + * + * @throws InvalidParameterValueException if the URL is blank, fails validation + */ + private void validateDnsServerUrl(String trimmedUrl) { + if (StringUtils.isBlank(trimmedUrl)) { + throw new InvalidParameterValueException("URL cannot be blank."); + } + try { + UriUtils.validateUrl(trimmedUrl); + } catch (IllegalArgumentException e) { + throw new InvalidParameterValueException(e.getMessage()); + } + } + @Override @ActionEvent(eventType = EventTypes.EVENT_DNS_SERVER_ADD, eventDescription = "Adding a DNS Server") public DnsServer addDnsServer(AddDnsServerCmd cmd) { Account caller = CallContext.current().getCallingAccount(); - DnsServer existing = dnsServerDao.findByUrlAndAccount(cmd.getUrl(), caller.getId()); + enforceRootAdminOnly(caller.getId()); + + String dnsUrl = StringUtils.trim(cmd.getUrl()); + validateDnsServerUrl(dnsUrl); + DnsServer existing = dnsServerDao.findByUrlAndAccount(dnsUrl, caller.getId()); if (existing != null) { throw new InvalidParameterValueException( - "This Account already has a DNS server integration for URL: " + cmd.getUrl()); + "This Account already has a DNS server integration for URL: " + dnsUrl); } boolean isDnsPublic = cmd.isPublic(); @@ -185,7 +202,7 @@ public DnsServer addDnsServer(AddDnsServerCmd cmd) { } DnsProviderType type = cmd.getProvider(); - DnsServerVO server = new DnsServerVO(cmd.getName(), cmd.getUrl(), cmd.getPort(), type, + DnsServerVO server = new DnsServerVO(cmd.getName(), dnsUrl, cmd.getPort(), type, cmd.getDnsUserName(), cmd.getDnsApiKey(), isDnsPublic, publicDomainSuffix, cmd.getNameServers(), caller.getAccountId(), caller.getDomainId()); @@ -240,6 +257,8 @@ public DnsServer updateDnsServer(UpdateDnsServerCmd cmd) { } Account caller = CallContext.current().getCallingAccount(); + enforceRootAdminOnly(caller.getId()); + accountMgr.checkAccess(caller, null, true, dnsServer); boolean validationRequired = false; @@ -250,13 +269,15 @@ public DnsServer updateDnsServer(UpdateDnsServerCmd cmd) { dnsServer.setName(cmd.getName()); } - if (cmd.getUrl() != null) { - if (!cmd.getUrl().equals(originalUrl)) { - DnsServer duplicate = dnsServerDao.findByUrlAndAccount(cmd.getUrl(), dnsServer.getAccountId()); + if (StringUtils.isNotBlank(cmd.getUrl())) { + String dnsUrl = StringUtils.trim(cmd.getUrl()); + if (!dnsUrl.equals(originalUrl)) { + validateDnsServerUrl(dnsUrl); + DnsServer duplicate = dnsServerDao.findByUrlAndAccount(dnsUrl, dnsServer.getAccountId()); if (duplicate != null && duplicate.getId() != dnsServer.getId()) { throw new InvalidParameterValueException("Another DNS server with this URL already exists."); } - dnsServer.setUrl(cmd.getUrl()); + dnsServer.setUrl(dnsUrl); validationRequired = true; } } @@ -317,6 +338,7 @@ public boolean deleteDnsServer(DeleteDnsServerCmd cmd) { throw new InvalidParameterValueException(String.format("DNS server with ID: %s not found.", dnsServerId)); } Account caller = CallContext.current().getCallingAccount(); + enforceRootAdminOnly(caller.getId()); accountMgr.checkAccess(caller, null, true, dnsServer); return Transaction.execute((TransactionCallback) status -> { if (cmd.getCleanup()) { @@ -1227,4 +1249,10 @@ public void syncDnsRecordsState(Long instanceId, String dnsRecordUrl, long dnsZo provider.addRecord(dnsServer, dnsZone, recordIpv6); } } + + void enforceRootAdminOnly(Long callerId) { + if (!accountMgr.isRootAdmin(callerId)) { + throw new PermissionDeniedException("This API can only be called by root admin"); + } + } } 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..d2df647a61bd 100644 --- a/server/src/test/java/org/apache/cloudstack/dns/DnsProviderManagerImplTest.java +++ b/server/src/test/java/org/apache/cloudstack/dns/DnsProviderManagerImplTest.java @@ -175,6 +175,8 @@ public void setUp() throws Exception { doNothing().when(accountMgr).checkAccess(any(Account.class), nullable(org.apache.cloudstack.acl.SecurityChecker.AccessType.class), eq(true), any()); + + when(accountMgr.isRootAdmin(callerMock.getId())).thenReturn(true); } @After @@ -717,8 +719,7 @@ public void testConfigure() throws Exception { public void testAddDnsServerSuccess() throws Exception { 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.getUrl()).thenReturn("http://192.0.2.1:8081"); when(cmd.getProvider()).thenReturn(DnsProviderType.PowerDNS); when(dnsServerDao.findByUrlAndAccount(anyString(), anyLong())).thenReturn(null); when(dnsProviderMock.validateAndResolveServer(any())).thenReturn("resolved-id"); @@ -781,37 +782,63 @@ public void testListDnsZones() { public void testAddDnsServerAlreadyExists() { org.apache.cloudstack.api.command.user.dns.AddDnsServerCmd cmd = mock( org.apache.cloudstack.api.command.user.dns.AddDnsServerCmd.class); - when(cmd.getUrl()).thenReturn("http://newpdns:8081"); + when(cmd.getUrl()).thenReturn("http://192.0.2.1:8081"); when(dnsServerDao.findByUrlAndAccount(anyString(), anyLong())).thenReturn(serverVO); manager.addDnsServer(cmd); } @Test - public void testAddDnsServerNormalUser() throws Exception { + public void testAddDnsServerTrimsUrlBeforeDuplicateCheckAndPersistence() throws Exception { 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(false); - when(accountMgr.isDomainAdmin(callerMock.getId())).thenReturn(false); - when(cmd.getUrl()).thenReturn("http://newpdns:8081"); + when(cmd.getUrl()).thenReturn(" http://192.0.2.1:8081 "); when(cmd.getProvider()).thenReturn(DnsProviderType.PowerDNS); - when(cmd.getNameServers()).thenReturn(Collections.emptyList()); - when(cmd.isPublic()).thenReturn(true); - when(cmd.getPublicDomainSuffix()).thenReturn("example.com"); when(dnsServerDao.findByUrlAndAccount(anyString(), anyLong())).thenReturn(null); when(dnsProviderMock.validateAndResolveServer(any())).thenReturn("resolved-id"); when(dnsServerDao.persist(any())).thenReturn(serverVO); + + manager.addDnsServer(cmd); + + verify(dnsServerDao).findByUrlAndAccount(eq("http://192.0.2.1:8081"), anyLong()); + verify(dnsServerDao).persist(Mockito.argThat(s -> "http://192.0.2.1:8081".equals(((DnsServerVO) s).getUrl()))); + } + + @Test(expected = InvalidParameterValueException.class) + public void testAddDnsServerRejectsLoopbackUrl() { + org.apache.cloudstack.api.command.user.dns.AddDnsServerCmd cmd = mock( + org.apache.cloudstack.api.command.user.dns.AddDnsServerCmd.class); + when(cmd.getUrl()).thenReturn("http://127.0.0.1:8081"); + manager.addDnsServer(cmd); + } + + @Test(expected = InvalidParameterValueException.class) + public void testAddDnsServerRejectsUrlWithoutScheme() { + org.apache.cloudstack.api.command.user.dns.AddDnsServerCmd cmd = mock( + org.apache.cloudstack.api.command.user.dns.AddDnsServerCmd.class); + when(cmd.getUrl()).thenReturn("192.0.2.1:8081"); + manager.addDnsServer(cmd); + } + + @Test + public void testAddDnsServerAllowsPrivateAddressForRootAdmin() throws Exception { + org.apache.cloudstack.api.command.user.dns.AddDnsServerCmd cmd = mock( + org.apache.cloudstack.api.command.user.dns.AddDnsServerCmd.class); + when(cmd.getUrl()).thenReturn("http://192.168.1.1:8081"); + when(cmd.getProvider()).thenReturn(DnsProviderType.PowerDNS); + when(dnsServerDao.findByUrlAndAccount(anyString(), anyLong())).thenReturn(null); + when(dnsProviderMock.validateAndResolveServer(any())).thenReturn("resolved-id"); + when(dnsServerDao.persist(any())).thenReturn(serverVO); + DnsServer result = manager.addDnsServer(cmd); assertNotNull(result); - verify(dnsServerDao).persist(Mockito.argThat( - s -> !((DnsServerVO) s).getPublicServer() && ((DnsServerVO) s).getPublicDomainSuffix() == null)); + verify(dnsServerDao).persist(any()); } @Test(expected = CloudRuntimeException.class) public void testAddDnsServerValidationFailure() throws Exception { 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.getUrl()).thenReturn("http://192.0.2.1:8081"); when(cmd.getProvider()).thenReturn(DnsProviderType.PowerDNS); when(cmd.getNameServers()).thenReturn(Collections.emptyList()); when(dnsServerDao.findByUrlAndAccount(anyString(), anyLong())).thenReturn(null); @@ -819,12 +846,20 @@ public void testAddDnsServerValidationFailure() throws Exception { manager.addDnsServer(cmd); } + @Test(expected = PermissionDeniedException.class) + public void testAddDnsServerNormalUser() throws Exception { + 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(false); + manager.addDnsServer(cmd); + } + @Test(expected = InvalidParameterValueException.class) public void testUpdateDnsServerUrlDuplicate() { 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.getUrl()).thenReturn("http://duplicate:8081"); + when(cmd.getUrl()).thenReturn("http://192.0.2.1:8081"); DnsServerVO existingServer = mock(DnsServerVO.class); when(existingServer.getId()).thenReturn(SERVER_ID + 1); // Different ID implies duplicate @@ -835,12 +870,60 @@ public void testUpdateDnsServerUrlDuplicate() { manager.updateDnsServer(cmd); } + @Test(expected = InvalidParameterValueException.class) + public void testUpdateDnsServerRejectsLoopbackUrl() { + 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.getUrl()).thenReturn("http://127.0.0.1:8081"); + when(dnsServerDao.findById(SERVER_ID)).thenReturn(serverVO); + Mockito.doReturn("http://original:8081").when(serverVO).getUrl(); + + manager.updateDnsServer(cmd); + } + + @Test + public void testUpdateDnsServerAllowsPrivateAddressForRootAdmin() throws Exception { + 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.getUrl()).thenReturn("http://192.168.1.1:8081"); + when(dnsServerDao.findById(SERVER_ID)).thenReturn(serverVO); + Mockito.doReturn("http://original:8081").when(serverVO).getUrl(); + Mockito.doReturn(DnsProviderType.PowerDNS).when(serverVO).getProviderType(); + when(dnsServerDao.findByUrlAndAccount(anyString(), anyLong())).thenReturn(null); + doNothing().when(dnsProviderMock).validate(any()); + when(dnsServerDao.update(anyLong(), any())).thenReturn(true); + + DnsServer result = manager.updateDnsServer(cmd); + assertNotNull(result); + verify(dnsProviderMock).validate(any()); + } + + @Test + public void testUpdateDnsServerTreatsWhitespaceOnlyUrlChangeAsUnchanged() throws Exception { + org.apache.cloudstack.api.command.user.dns.UpdateDnsServerCmd cmd = mock( + org.apache.cloudstack.api.command.user.dns.UpdateDnsServerCmd.class); + Integer unchangedPort = serverVO.getPort(); + when(cmd.getId()).thenReturn(SERVER_ID); + when(cmd.getUrl()).thenReturn(" http://192.0.2.1:8081 "); + when(cmd.getPort()).thenReturn(unchangedPort); + when(dnsServerDao.findById(SERVER_ID)).thenReturn(serverVO); + Mockito.doReturn("http://192.0.2.1:8081").when(serverVO).getUrl(); + when(dnsServerDao.update(anyLong(), any())).thenReturn(true); + + DnsServer result = manager.updateDnsServer(cmd); + assertNotNull(result); + verify(dnsProviderMock, never()).validate(any()); + verify(serverVO, never()).setUrl(anyString()); + } + @Test public void testUpdateDnsServerUrlValid() throws Exception { 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.getUrl()).thenReturn("http://new-url:8081"); + when(cmd.getUrl()).thenReturn("http://192.0.2.1:8081"); when(dnsServerDao.findById(SERVER_ID)).thenReturn(serverVO); Mockito.doReturn("http://original:8081").when(serverVO).getUrl();