From a3fab5a8ec9ebfba7f9d916958406f4eaca53930 Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Fri, 7 Aug 2026 10:47:37 +0200 Subject: [PATCH 1/2] 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 c0b5e8963e9d55f590dfd97964aa37f422f7d1be Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Fri, 7 Aug 2026 11:38:57 +0200 Subject: [PATCH 2/2] 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();