From 9714982136b5f02446679f5f85227b91ae4ea188 Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Fri, 7 Aug 2026 10:47:37 +0200 Subject: [PATCH 01/11] validate DNS server URLs in provider framework --- .../dns/DnsProviderManagerImpl.java | 21 ++++++++++++ .../dns/DnsProviderManagerImplTest.java | 32 +++++++++++++++---- 2 files changed, 47 insertions(+), 6 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..3718967ba5aa 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; @@ -162,9 +163,28 @@ private DnsProvider getProviderByType(DnsProviderType type) { throw new CloudRuntimeException("No plugin found for DNS provider type: " + type); } + /** + * Rejects DNS provider URLs that resolve to an illegal address (per {@link UriUtils#validateUrl(String)}, + * currently any-local/link-local/loopback/multicast; RFC1918 site-local coverage follows once #271/#277 + * lands) before any provider client is given the chance to connect to it. A scheme is assumed to be + * `http` when the caller omits one, matching how DNS provider clients (e.g. PowerDnsClient) already + * tolerate bare host/IP values. + */ + private void validateDnsServerUrl(String url) { + if (StringUtils.isBlank(url)) { + return; + } + String urlToValidate = url.trim(); + if (!urlToValidate.startsWith("http://") && !urlToValidate.startsWith("https://")) { + urlToValidate = "http://" + urlToValidate; + } + UriUtils.validateUrl(urlToValidate); + } + @Override @ActionEvent(eventType = EventTypes.EVENT_DNS_SERVER_ADD, eventDescription = "Adding a DNS Server") public DnsServer addDnsServer(AddDnsServerCmd cmd) { + validateDnsServerUrl(cmd.getUrl()); Account caller = CallContext.current().getCallingAccount(); DnsServer existing = dnsServerDao.findByUrlAndAccount(cmd.getUrl(), caller.getId()); if (existing != null) { @@ -252,6 +272,7 @@ public DnsServer updateDnsServer(UpdateDnsServerCmd cmd) { if (cmd.getUrl() != null) { if (!cmd.getUrl().equals(originalUrl)) { + validateDnsServerUrl(cmd.getUrl()); DnsServer duplicate = dnsServerDao.findByUrlAndAccount(cmd.getUrl(), dnsServer.getAccountId()); if (duplicate != null && duplicate.getId() != dnsServer.getId()) { throw new InvalidParameterValueException("Another DNS server with this URL already exists."); 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..ec239239abdb 100644 --- a/server/src/test/java/org/apache/cloudstack/dns/DnsProviderManagerImplTest.java +++ b/server/src/test/java/org/apache/cloudstack/dns/DnsProviderManagerImplTest.java @@ -718,7 +718,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://93.184.216.34:8081"); when(cmd.getProvider()).thenReturn(DnsProviderType.PowerDNS); when(dnsServerDao.findByUrlAndAccount(anyString(), anyLong())).thenReturn(null); when(dnsProviderMock.validateAndResolveServer(any())).thenReturn("resolved-id"); @@ -781,18 +781,26 @@ 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://93.184.216.34:8081"); when(dnsServerDao.findByUrlAndAccount(anyString(), anyLong())).thenReturn(serverVO); manager.addDnsServer(cmd); } + @Test(expected = IllegalArgumentException.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 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); when(accountMgr.isDomainAdmin(callerMock.getId())).thenReturn(false); - when(cmd.getUrl()).thenReturn("http://newpdns:8081"); + when(cmd.getUrl()).thenReturn("http://93.184.216.34:8081"); when(cmd.getProvider()).thenReturn(DnsProviderType.PowerDNS); when(cmd.getNameServers()).thenReturn(Collections.emptyList()); when(cmd.isPublic()).thenReturn(true); @@ -811,7 +819,7 @@ 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://93.184.216.34:8081"); when(cmd.getProvider()).thenReturn(DnsProviderType.PowerDNS); when(cmd.getNameServers()).thenReturn(Collections.emptyList()); when(dnsServerDao.findByUrlAndAccount(anyString(), anyLong())).thenReturn(null); @@ -824,7 +832,7 @@ 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://93.184.216.34:8081"); DnsServerVO existingServer = mock(DnsServerVO.class); when(existingServer.getId()).thenReturn(SERVER_ID + 1); // Different ID implies duplicate @@ -835,12 +843,24 @@ public void testUpdateDnsServerUrlDuplicate() { manager.updateDnsServer(cmd); } + @Test(expected = IllegalArgumentException.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 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://93.184.216.34:8081"); when(dnsServerDao.findById(SERVER_ID)).thenReturn(serverVO); Mockito.doReturn("http://original:8081").when(serverVO).getUrl(); From 1fa16387a05526981a3acecf9021932e485dcc94 Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Fri, 7 Aug 2026 11:38:57 +0200 Subject: [PATCH 02/11] fixes --- .../dns/DnsProviderManagerImpl.java | 33 +++++------ .../dns/DnsProviderManagerImplTest.java | 55 +++++++++++++++++-- 2 files changed, 64 insertions(+), 24 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 3718967ba5aa..f8a0aa5dd84f 100644 --- a/server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java +++ b/server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java @@ -164,32 +164,28 @@ private DnsProvider getProviderByType(DnsProviderType type) { } /** - * Rejects DNS provider URLs that resolve to an illegal address (per {@link UriUtils#validateUrl(String)}, - * currently any-local/link-local/loopback/multicast; RFC1918 site-local coverage follows once #271/#277 - * lands) before any provider client is given the chance to connect to it. A scheme is assumed to be - * `http` when the caller omits one, matching how DNS provider clients (e.g. PowerDnsClient) already - * tolerate bare host/IP values. + * 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). + * Expects {@code url} to already be trimmed. */ private void validateDnsServerUrl(String url) { if (StringUtils.isBlank(url)) { return; } - String urlToValidate = url.trim(); - if (!urlToValidate.startsWith("http://") && !urlToValidate.startsWith("https://")) { - urlToValidate = "http://" + urlToValidate; - } - UriUtils.validateUrl(urlToValidate); + UriUtils.validateUrl(url); } @Override @ActionEvent(eventType = EventTypes.EVENT_DNS_SERVER_ADD, eventDescription = "Adding a DNS Server") public DnsServer addDnsServer(AddDnsServerCmd cmd) { - validateDnsServerUrl(cmd.getUrl()); + String url = StringUtils.trim(cmd.getUrl()); + validateDnsServerUrl(url); Account caller = CallContext.current().getCallingAccount(); - DnsServer existing = dnsServerDao.findByUrlAndAccount(cmd.getUrl(), caller.getId()); + DnsServer existing = dnsServerDao.findByUrlAndAccount(url, 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: " + url); } boolean isDnsPublic = cmd.isPublic(); @@ -205,7 +201,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(), url, cmd.getPort(), type, cmd.getDnsUserName(), cmd.getDnsApiKey(), isDnsPublic, publicDomainSuffix, cmd.getNameServers(), caller.getAccountId(), caller.getDomainId()); @@ -271,13 +267,14 @@ public DnsServer updateDnsServer(UpdateDnsServerCmd cmd) { } if (cmd.getUrl() != null) { - if (!cmd.getUrl().equals(originalUrl)) { - validateDnsServerUrl(cmd.getUrl()); - DnsServer duplicate = dnsServerDao.findByUrlAndAccount(cmd.getUrl(), dnsServer.getAccountId()); + String url = StringUtils.trim(cmd.getUrl()); + if (!url.equals(originalUrl)) { + validateDnsServerUrl(url); + DnsServer duplicate = dnsServerDao.findByUrlAndAccount(url, 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(url); validationRequired = true; } } 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 ec239239abdb..94efacad2859 100644 --- a/server/src/test/java/org/apache/cloudstack/dns/DnsProviderManagerImplTest.java +++ b/server/src/test/java/org/apache/cloudstack/dns/DnsProviderManagerImplTest.java @@ -718,7 +718,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://93.184.216.34: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,11 +781,28 @@ 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://93.184.216.34:8081"); + when(cmd.getUrl()).thenReturn("http://192.0.2.1:8081"); when(dnsServerDao.findByUrlAndAccount(anyString(), anyLong())).thenReturn(serverVO); manager.addDnsServer(cmd); } + @Test + 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(true); + 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"); + 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 = IllegalArgumentException.class) public void testAddDnsServerRejectsLoopbackUrl() { org.apache.cloudstack.api.command.user.dns.AddDnsServerCmd cmd = mock( @@ -794,13 +811,21 @@ public void testAddDnsServerRejectsLoopbackUrl() { manager.addDnsServer(cmd); } + @Test(expected = IllegalArgumentException.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 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); when(accountMgr.isDomainAdmin(callerMock.getId())).thenReturn(false); - when(cmd.getUrl()).thenReturn("http://93.184.216.34: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); @@ -819,7 +844,7 @@ 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://93.184.216.34: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); @@ -832,7 +857,7 @@ 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://93.184.216.34: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 @@ -855,12 +880,30 @@ public void testUpdateDnsServerRejectsLoopbackUrl() { manager.updateDnsServer(cmd); } + @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://93.184.216.34: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(); From 6981138f89f17682426345da2038e721af6d13e8 Mon Sep 17 00:00:00 2001 From: dahn Date: Mon, 10 Aug 2026 14:20:28 +0200 Subject: [PATCH 03/11] Apply suggestion from @DaanHoogland --- .../java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 f8a0aa5dd84f..69b202d3b40f 100644 --- a/server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java +++ b/server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java @@ -171,7 +171,7 @@ private DnsProvider getProviderByType(DnsProviderType type) { */ private void validateDnsServerUrl(String url) { if (StringUtils.isBlank(url)) { - return; + throw new IllegalArgumentException("URL cannot be blank."); } UriUtils.validateUrl(url); } From b88552badd9c630171363db2759f9792d6e549de Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Sat, 15 Aug 2026 09:27:15 +0200 Subject: [PATCH 04/11] address (some) review comments --- .../dns/DnsProviderManagerImpl.java | 29 ++++++++++++------- .../dns/DnsProviderManagerImplTest.java | 6 ++-- 2 files changed, 21 insertions(+), 14 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 69b202d3b40f..83c9bb36e7f3 100644 --- a/server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java +++ b/server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java @@ -164,23 +164,30 @@ private DnsProvider getProviderByType(DnsProviderType 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). - * Expects {@code url} to already be trimmed. + * Trims and 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). + * + * @return the trimmed URL. + * @throws InvalidParameterValueException if the URL is blank or fails validation. */ - private void validateDnsServerUrl(String url) { - if (StringUtils.isBlank(url)) { - throw new IllegalArgumentException("URL cannot be blank."); + private String validateDnsServerUrl(String url) { + String trimmedUrl = StringUtils.trim(url); + if (StringUtils.isBlank(trimmedUrl)) { + throw new InvalidParameterValueException("URL cannot be blank."); } - UriUtils.validateUrl(url); + try { + UriUtils.validateUrl(trimmedUrl); + } catch (IllegalArgumentException e) { + throw new InvalidParameterValueException(e.getMessage()); + } + return trimmedUrl; } @Override @ActionEvent(eventType = EventTypes.EVENT_DNS_SERVER_ADD, eventDescription = "Adding a DNS Server") public DnsServer addDnsServer(AddDnsServerCmd cmd) { - String url = StringUtils.trim(cmd.getUrl()); - validateDnsServerUrl(url); + String url = validateDnsServerUrl(cmd.getUrl()); Account caller = CallContext.current().getCallingAccount(); DnsServer existing = dnsServerDao.findByUrlAndAccount(url, caller.getId()); if (existing != null) { @@ -269,7 +276,7 @@ public DnsServer updateDnsServer(UpdateDnsServerCmd cmd) { if (cmd.getUrl() != null) { String url = StringUtils.trim(cmd.getUrl()); if (!url.equals(originalUrl)) { - validateDnsServerUrl(url); + url = validateDnsServerUrl(url); DnsServer duplicate = dnsServerDao.findByUrlAndAccount(url, dnsServer.getAccountId()); if (duplicate != null && duplicate.getId() != dnsServer.getId()) { throw new InvalidParameterValueException("Another DNS server with this URL already exists."); 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 94efacad2859..7008edf1bd1c 100644 --- a/server/src/test/java/org/apache/cloudstack/dns/DnsProviderManagerImplTest.java +++ b/server/src/test/java/org/apache/cloudstack/dns/DnsProviderManagerImplTest.java @@ -803,7 +803,7 @@ public void testAddDnsServerTrimsUrlBeforeDuplicateCheckAndPersistence() throws verify(dnsServerDao).persist(Mockito.argThat(s -> "http://192.0.2.1:8081".equals(((DnsServerVO) s).getUrl()))); } - @Test(expected = IllegalArgumentException.class) + @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); @@ -811,7 +811,7 @@ public void testAddDnsServerRejectsLoopbackUrl() { manager.addDnsServer(cmd); } - @Test(expected = IllegalArgumentException.class) + @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); @@ -868,7 +868,7 @@ public void testUpdateDnsServerUrlDuplicate() { manager.updateDnsServer(cmd); } - @Test(expected = IllegalArgumentException.class) + @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); From 1545adde35dd68a90291687fc3f7cec23b1e13f6 Mon Sep 17 00:00:00 2001 From: Manoj Kumar Date: Tue, 18 Aug 2026 15:12:06 +0530 Subject: [PATCH 05/11] restrict pvt/site-local urls to root admin only --- .../dns/DnsProviderManagerImpl.java | 18 ++++-- .../dns/DnsProviderManagerImplTest.java | 57 +++++++++++++++++++ 2 files changed, 70 insertions(+), 5 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 83c9bb36e7f3..fbb06b27511f 100644 --- a/server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java +++ b/server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java @@ -97,6 +97,7 @@ import com.cloud.utils.Pair; import com.cloud.utils.StringUtils; import com.cloud.utils.UriUtils; +import com.cloud.utils.net.NetUtils; import com.cloud.utils.component.ManagerBase; import com.cloud.utils.component.PluggableService; import com.cloud.utils.db.Filter; @@ -167,28 +168,35 @@ private DnsProvider getProviderByType(DnsProviderType type) { * Trims and 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). + * Private/site-local addresses (e.g. {@code 192.168.0.0/16}) are only permitted for root admin callers. * * @return the trimmed URL. - * @throws InvalidParameterValueException if the URL is blank or fails validation. + * @throws InvalidParameterValueException if the URL is blank, fails validation, or is a private address + * requested by a non-root-admin caller. */ - private String validateDnsServerUrl(String url) { + private String validateDnsServerUrl(String url, Account caller) { String trimmedUrl = StringUtils.trim(url); if (StringUtils.isBlank(trimmedUrl)) { throw new InvalidParameterValueException("URL cannot be blank."); } + Pair hostAndPort; try { - UriUtils.validateUrl(trimmedUrl); + hostAndPort = UriUtils.validateUrl(trimmedUrl); } catch (IllegalArgumentException e) { throw new InvalidParameterValueException(e.getMessage()); } + if (!accountMgr.isRootAdmin(caller.getId()) && NetUtils.isSiteLocalAddress(hostAndPort.first())) { + throw new InvalidParameterValueException( + "Only root admin accounts can configure a DNS server on a private/internal network address."); + } return trimmedUrl; } @Override @ActionEvent(eventType = EventTypes.EVENT_DNS_SERVER_ADD, eventDescription = "Adding a DNS Server") public DnsServer addDnsServer(AddDnsServerCmd cmd) { - String url = validateDnsServerUrl(cmd.getUrl()); Account caller = CallContext.current().getCallingAccount(); + String url = validateDnsServerUrl(cmd.getUrl(), caller); DnsServer existing = dnsServerDao.findByUrlAndAccount(url, caller.getId()); if (existing != null) { throw new InvalidParameterValueException( @@ -276,7 +284,7 @@ public DnsServer updateDnsServer(UpdateDnsServerCmd cmd) { if (cmd.getUrl() != null) { String url = StringUtils.trim(cmd.getUrl()); if (!url.equals(originalUrl)) { - url = validateDnsServerUrl(url); + url = validateDnsServerUrl(url, caller); DnsServer duplicate = dnsServerDao.findByUrlAndAccount(url, dnsServer.getAccountId()); if (duplicate != null && duplicate.getId() != dnsServer.getId()) { throw new InvalidParameterValueException("Another DNS server with this URL already exists."); 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 7008edf1bd1c..512c417ccc5c 100644 --- a/server/src/test/java/org/apache/cloudstack/dns/DnsProviderManagerImplTest.java +++ b/server/src/test/java/org/apache/cloudstack/dns/DnsProviderManagerImplTest.java @@ -819,6 +819,31 @@ public void testAddDnsServerRejectsUrlWithoutScheme() { manager.addDnsServer(cmd); } + @Test(expected = InvalidParameterValueException.class) + public void testAddDnsServerRejectsPrivateAddressForNonRootAdmin() { + 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(cmd.getUrl()).thenReturn("http://192.168.1.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(accountMgr.isRootAdmin(callerMock.getId())).thenReturn(true); + 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(any()); + } + @Test public void testAddDnsServerNormalUser() throws Exception { org.apache.cloudstack.api.command.user.dns.AddDnsServerCmd cmd = mock( @@ -880,6 +905,38 @@ public void testUpdateDnsServerRejectsLoopbackUrl() { manager.updateDnsServer(cmd); } + @Test(expected = InvalidParameterValueException.class) + public void testUpdateDnsServerRejectsPrivateAddressForNonRootAdmin() { + org.apache.cloudstack.api.command.user.dns.UpdateDnsServerCmd cmd = mock( + org.apache.cloudstack.api.command.user.dns.UpdateDnsServerCmd.class); + when(accountMgr.isRootAdmin(callerMock.getId())).thenReturn(false); + 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(); + + 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(accountMgr.isRootAdmin(callerMock.getId())).thenReturn(true); + 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( From 28624dfa2a45b2036f8d927b6ce5715bf0a7228c Mon Sep 17 00:00:00 2001 From: Manoj Kumar Date: Tue, 18 Aug 2026 17:51:04 +0530 Subject: [PATCH 06/11] trim url before passing to validation method --- .../dns/DnsProviderManagerImpl.java | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 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 fbb06b27511f..af0208e18aa7 100644 --- a/server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java +++ b/server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java @@ -174,8 +174,7 @@ private DnsProvider getProviderByType(DnsProviderType type) { * @throws InvalidParameterValueException if the URL is blank, fails validation, or is a private address * requested by a non-root-admin caller. */ - private String validateDnsServerUrl(String url, Account caller) { - String trimmedUrl = StringUtils.trim(url); + private String validateDnsServerUrl(String trimmedUrl, Account caller) { if (StringUtils.isBlank(trimmedUrl)) { throw new InvalidParameterValueException("URL cannot be blank."); } @@ -196,11 +195,12 @@ private String validateDnsServerUrl(String url, Account caller) { @ActionEvent(eventType = EventTypes.EVENT_DNS_SERVER_ADD, eventDescription = "Adding a DNS Server") public DnsServer addDnsServer(AddDnsServerCmd cmd) { Account caller = CallContext.current().getCallingAccount(); - String url = validateDnsServerUrl(cmd.getUrl(), caller); - DnsServer existing = dnsServerDao.findByUrlAndAccount(url, caller.getId()); + String trimmedUrl = StringUtils.trim(cmd.getUrl()); + String dnsUrl = validateDnsServerUrl(trimmedUrl, caller); + DnsServer existing = dnsServerDao.findByUrlAndAccount(dnsUrl, caller.getId()); if (existing != null) { throw new InvalidParameterValueException( - "This Account already has a DNS server integration for URL: " + url); + "This Account already has a DNS server integration for URL: " + dnsUrl); } boolean isDnsPublic = cmd.isPublic(); @@ -216,7 +216,7 @@ public DnsServer addDnsServer(AddDnsServerCmd cmd) { } DnsProviderType type = cmd.getProvider(); - DnsServerVO server = new DnsServerVO(cmd.getName(), url, 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()); @@ -282,14 +282,14 @@ public DnsServer updateDnsServer(UpdateDnsServerCmd cmd) { } if (cmd.getUrl() != null) { - String url = StringUtils.trim(cmd.getUrl()); - if (!url.equals(originalUrl)) { - url = validateDnsServerUrl(url, caller); - DnsServer duplicate = dnsServerDao.findByUrlAndAccount(url, dnsServer.getAccountId()); + String trimmedUrl = StringUtils.trim(cmd.getUrl()); + if (!trimmedUrl.equals(originalUrl)) { + String dnsUrl = validateDnsServerUrl(trimmedUrl, caller); + 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(url); + dnsServer.setUrl(dnsUrl); validationRequired = true; } } From 05b86b3ad379d7cd4623d320f78a0fe6803667e0 Mon Sep 17 00:00:00 2001 From: Manoj Kumar Date: Tue, 18 Aug 2026 18:28:55 +0530 Subject: [PATCH 07/11] fix minor comment --- .../dns/DnsProviderManagerImpl.java | 20 ++++++------------- 1 file changed, 6 insertions(+), 14 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 af0208e18aa7..752f70efbe9b 100644 --- a/server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java +++ b/server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java @@ -109,9 +109,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 @@ -128,10 +126,6 @@ public class DnsProviderManagerImpl extends ManagerBase implements DnsProviderMa @Inject DnsZoneNetworkMapDao dnsZoneNetworkMapDao; @Inject - UserVmDao userVmDao; - @Inject - NicDao nicDao; - @Inject DomainDao domainDao; @Inject DnsZoneJoinDao dnsZoneJoinDao; @@ -170,11 +164,10 @@ private DnsProvider getProviderByType(DnsProviderType type) { * enforced (including the requirement that the URL declares an {@code http}/{@code https} scheme). * Private/site-local addresses (e.g. {@code 192.168.0.0/16}) are only permitted for root admin callers. * - * @return the trimmed URL. * @throws InvalidParameterValueException if the URL is blank, fails validation, or is a private address * requested by a non-root-admin caller. */ - private String validateDnsServerUrl(String trimmedUrl, Account caller) { + private void validateDnsServerUrl(String trimmedUrl, Account caller) { if (StringUtils.isBlank(trimmedUrl)) { throw new InvalidParameterValueException("URL cannot be blank."); } @@ -188,15 +181,14 @@ private String validateDnsServerUrl(String trimmedUrl, Account caller) { throw new InvalidParameterValueException( "Only root admin accounts can configure a DNS server on a private/internal network address."); } - return trimmedUrl; } @Override @ActionEvent(eventType = EventTypes.EVENT_DNS_SERVER_ADD, eventDescription = "Adding a DNS Server") public DnsServer addDnsServer(AddDnsServerCmd cmd) { Account caller = CallContext.current().getCallingAccount(); - String trimmedUrl = StringUtils.trim(cmd.getUrl()); - String dnsUrl = validateDnsServerUrl(trimmedUrl, caller); + String dnsUrl = StringUtils.trim(cmd.getUrl()); + validateDnsServerUrl(dnsUrl, caller); DnsServer existing = dnsServerDao.findByUrlAndAccount(dnsUrl, caller.getId()); if (existing != null) { throw new InvalidParameterValueException( @@ -282,9 +274,9 @@ public DnsServer updateDnsServer(UpdateDnsServerCmd cmd) { } if (cmd.getUrl() != null) { - String trimmedUrl = StringUtils.trim(cmd.getUrl()); - if (!trimmedUrl.equals(originalUrl)) { - String dnsUrl = validateDnsServerUrl(trimmedUrl, caller); + String dnsUrl = StringUtils.trim(cmd.getUrl()); + if (!dnsUrl.equals(originalUrl)) { + validateDnsServerUrl(dnsUrl, caller); 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."); From 34f233d17a610bc56ae5fce278ab7db260107b79 Mon Sep 17 00:00:00 2001 From: Manoj Kumar Date: Fri, 21 Aug 2026 15:43:19 +0530 Subject: [PATCH 08/11] restrict Add/Update/Delete Dns server api for root admin --- .../api/command/user/dns/AddDnsServerCmd.java | 2 +- .../command/user/dns/DeleteDnsServerCmd.java | 2 +- .../command/user/dns/UpdateDnsServerCmd.java | 2 +- ...ic_dns_view.sql => cloud.nic_dns_view.sql} | 0 .../dns/DnsProviderManagerImpl.java | 23 +++++----- .../dns/DnsProviderManagerImplTest.java | 42 ------------------- 6 files changed, 16 insertions(+), 55 deletions(-) rename engine/schema/src/main/resources/META-INF/db/views/{nic_dns_view.sql => cloud.nic_dns_view.sql} (100%) 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 752f70efbe9b..08377966a742 100644 --- a/server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java +++ b/server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java @@ -97,7 +97,6 @@ import com.cloud.utils.Pair; import com.cloud.utils.StringUtils; import com.cloud.utils.UriUtils; -import com.cloud.utils.net.NetUtils; import com.cloud.utils.component.ManagerBase; import com.cloud.utils.component.PluggableService; import com.cloud.utils.db.Filter; @@ -162,31 +161,26 @@ private DnsProvider getProviderByType(DnsProviderType type) { * Trims and 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). - * Private/site-local addresses (e.g. {@code 192.168.0.0/16}) are only permitted for root admin callers. * - * @throws InvalidParameterValueException if the URL is blank, fails validation, or is a private address - * requested by a non-root-admin caller. + * @throws InvalidParameterValueException if the URL is blank, fails validation */ private void validateDnsServerUrl(String trimmedUrl, Account caller) { if (StringUtils.isBlank(trimmedUrl)) { throw new InvalidParameterValueException("URL cannot be blank."); } - Pair hostAndPort; try { - hostAndPort = UriUtils.validateUrl(trimmedUrl); + UriUtils.validateUrl(trimmedUrl); } catch (IllegalArgumentException e) { throw new InvalidParameterValueException(e.getMessage()); } - if (!accountMgr.isRootAdmin(caller.getId()) && NetUtils.isSiteLocalAddress(hostAndPort.first())) { - throw new InvalidParameterValueException( - "Only root admin accounts can configure a DNS server on a private/internal network address."); - } } @Override @ActionEvent(eventType = EventTypes.EVENT_DNS_SERVER_ADD, eventDescription = "Adding a DNS Server") public DnsServer addDnsServer(AddDnsServerCmd cmd) { Account caller = CallContext.current().getCallingAccount(); + enforceRootAdminOnly(caller.getId()); + String dnsUrl = StringUtils.trim(cmd.getUrl()); validateDnsServerUrl(dnsUrl, caller); DnsServer existing = dnsServerDao.findByUrlAndAccount(dnsUrl, caller.getId()); @@ -263,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; @@ -342,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()) { @@ -1252,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 512c417ccc5c..ff1680d2fc07 100644 --- a/server/src/test/java/org/apache/cloudstack/dns/DnsProviderManagerImplTest.java +++ b/server/src/test/java/org/apache/cloudstack/dns/DnsProviderManagerImplTest.java @@ -819,15 +819,6 @@ public void testAddDnsServerRejectsUrlWithoutScheme() { manager.addDnsServer(cmd); } - @Test(expected = InvalidParameterValueException.class) - public void testAddDnsServerRejectsPrivateAddressForNonRootAdmin() { - 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(cmd.getUrl()).thenReturn("http://192.168.1.1:8081"); - manager.addDnsServer(cmd); - } - @Test public void testAddDnsServerAllowsPrivateAddressForRootAdmin() throws Exception { org.apache.cloudstack.api.command.user.dns.AddDnsServerCmd cmd = mock( @@ -844,26 +835,6 @@ public void testAddDnsServerAllowsPrivateAddressForRootAdmin() throws Exception verify(dnsServerDao).persist(any()); } - @Test - 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); - when(accountMgr.isDomainAdmin(callerMock.getId())).thenReturn(false); - 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); - DnsServer result = manager.addDnsServer(cmd); - assertNotNull(result); - verify(dnsServerDao).persist(Mockito.argThat( - s -> !((DnsServerVO) s).getPublicServer() && ((DnsServerVO) s).getPublicDomainSuffix() == null)); - } - @Test(expected = CloudRuntimeException.class) public void testAddDnsServerValidationFailure() throws Exception { org.apache.cloudstack.api.command.user.dns.AddDnsServerCmd cmd = mock( @@ -905,19 +876,6 @@ public void testUpdateDnsServerRejectsLoopbackUrl() { manager.updateDnsServer(cmd); } - @Test(expected = InvalidParameterValueException.class) - public void testUpdateDnsServerRejectsPrivateAddressForNonRootAdmin() { - org.apache.cloudstack.api.command.user.dns.UpdateDnsServerCmd cmd = mock( - org.apache.cloudstack.api.command.user.dns.UpdateDnsServerCmd.class); - when(accountMgr.isRootAdmin(callerMock.getId())).thenReturn(false); - 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(); - - manager.updateDnsServer(cmd); - } - @Test public void testUpdateDnsServerAllowsPrivateAddressForRootAdmin() throws Exception { org.apache.cloudstack.api.command.user.dns.UpdateDnsServerCmd cmd = mock( From 265dab6c93f54e7cf9ddd4da7f8bb4cb099d2b8d Mon Sep 17 00:00:00 2001 From: Manoj Kumar Date: Fri, 21 Aug 2026 15:59:20 +0530 Subject: [PATCH 09/11] fix minor comment --- .../java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 08377966a742..f86d23ae43c8 100644 --- a/server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java +++ b/server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java @@ -269,7 +269,7 @@ public DnsServer updateDnsServer(UpdateDnsServerCmd cmd) { dnsServer.setName(cmd.getName()); } - if (cmd.getUrl() != null) { + if (StringUtils.isNotBlank(cmd.getUrl())) { String dnsUrl = StringUtils.trim(cmd.getUrl()); if (!dnsUrl.equals(originalUrl)) { validateDnsServerUrl(dnsUrl, caller); From 8968e1b5e197f4d90ce715849ac5cbf4c1f56871 Mon Sep 17 00:00:00 2001 From: Manoj Kumar Date: Fri, 21 Aug 2026 16:43:08 +0530 Subject: [PATCH 10/11] fix unit test --- .../dns/DnsProviderManagerImplTest.java | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) 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 ff1680d2fc07..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,7 +719,6 @@ 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://192.0.2.1:8081"); when(cmd.getProvider()).thenReturn(DnsProviderType.PowerDNS); when(dnsServerDao.findByUrlAndAccount(anyString(), anyLong())).thenReturn(null); @@ -790,7 +791,6 @@ public void testAddDnsServerAlreadyExists() { 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(true); when(cmd.getUrl()).thenReturn(" http://192.0.2.1:8081 "); when(cmd.getProvider()).thenReturn(DnsProviderType.PowerDNS); when(dnsServerDao.findByUrlAndAccount(anyString(), anyLong())).thenReturn(null); @@ -823,7 +823,6 @@ public void testAddDnsServerRejectsUrlWithoutScheme() { 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(accountMgr.isRootAdmin(callerMock.getId())).thenReturn(true); when(cmd.getUrl()).thenReturn("http://192.168.1.1:8081"); when(cmd.getProvider()).thenReturn(DnsProviderType.PowerDNS); when(dnsServerDao.findByUrlAndAccount(anyString(), anyLong())).thenReturn(null); @@ -839,7 +838,6 @@ public void testAddDnsServerAllowsPrivateAddressForRootAdmin() throws Exception 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://192.0.2.1:8081"); when(cmd.getProvider()).thenReturn(DnsProviderType.PowerDNS); when(cmd.getNameServers()).thenReturn(Collections.emptyList()); @@ -848,6 +846,14 @@ 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( @@ -880,7 +886,6 @@ public void testUpdateDnsServerRejectsLoopbackUrl() { 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(accountMgr.isRootAdmin(callerMock.getId())).thenReturn(true); when(cmd.getId()).thenReturn(SERVER_ID); when(cmd.getUrl()).thenReturn("http://192.168.1.1:8081"); when(dnsServerDao.findById(SERVER_ID)).thenReturn(serverVO); From 738bc24408cdecd848d4893480c0ff634cb75092 Mon Sep 17 00:00:00 2001 From: Manoj Kumar Date: Fri, 21 Aug 2026 20:57:25 +0530 Subject: [PATCH 11/11] address review comments --- .../org/apache/cloudstack/dns/DnsProviderManagerImpl.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 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 f86d23ae43c8..0d08540d129a 100644 --- a/server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java +++ b/server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java @@ -158,13 +158,13 @@ private DnsProvider getProviderByType(DnsProviderType type) { } /** - * Trims and rejects a DNS provider URL that resolves to an illegal address before any provider client + * 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, Account caller) { + private void validateDnsServerUrl(String trimmedUrl) { if (StringUtils.isBlank(trimmedUrl)) { throw new InvalidParameterValueException("URL cannot be blank."); } @@ -182,7 +182,7 @@ public DnsServer addDnsServer(AddDnsServerCmd cmd) { enforceRootAdminOnly(caller.getId()); String dnsUrl = StringUtils.trim(cmd.getUrl()); - validateDnsServerUrl(dnsUrl, caller); + validateDnsServerUrl(dnsUrl); DnsServer existing = dnsServerDao.findByUrlAndAccount(dnsUrl, caller.getId()); if (existing != null) { throw new InvalidParameterValueException( @@ -272,7 +272,7 @@ public DnsServer updateDnsServer(UpdateDnsServerCmd cmd) { if (StringUtils.isNotBlank(cmd.getUrl())) { String dnsUrl = StringUtils.trim(cmd.getUrl()); if (!dnsUrl.equals(originalUrl)) { - validateDnsServerUrl(dnsUrl, caller); + 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.");