DNS Provider URL Validation

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 {
 
     /////////////////////////////////////////////////////

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 {
 
     /////////////////////////////////////////////////////

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 {
 
     /////////////////////////////////////////////////////

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 @@
     @Inject
     DnsZoneNetworkMapDao dnsZoneNetworkMapDao;
     @Inject
-    UserVmDao userVmDao;
-    @Inject
-    NicDao nicDao;
-    @Inject
     DomainDao domainDao;
     @Inject
     DnsZoneJoinDao dnsZoneJoinDao;
@@ -162,14 +157,36 @@
         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 @@
         }
 
         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 @@
         }
 
         Account caller = CallContext.current().getCallingAccount();
+        enforceRootAdminOnly(caller.getId());
+
         accountMgr.checkAccess(caller, null, true, dnsServer);
 
         boolean validationRequired = false;
@@ -250,13 +269,15 @@
             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 @@
             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<Boolean>) status -> {
             if (cmd.getCleanup()) {
@@ -1227,4 +1249,10 @@
             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");
+        }
+    }
 }

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

@@ -175,6 +175,8 @@
 
         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 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,50 +782,84 @@
     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);
         when(dnsProviderMock.validateAndResolveServer(any())).thenThrow(new CloudRuntimeException("Validation failed"));
         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 @@
         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();