DNS Provider URL Validation - #13821
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #13821 +/- ##
=========================================
Coverage 19.64% 19.65%
- Complexity 19786 19799 +13
=========================================
Files 6368 6368
Lines 574889 574902 +13
Branches 70353 70354 +1
=========================================
+ Hits 112957 113002 +45
+ Misses 449660 449631 -29
+ Partials 12272 12269 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR hardens DNS provider configuration by adding pre-connection URL validation in DnsProviderManagerImpl so user-supplied DNS provider endpoints are checked (and normalized via trimming) before being persisted or handed to provider clients.
Changes:
- Add
validateDnsServerUrl()and invoke it inaddDnsServerand whenupdateDnsServerchanges the URL. - Normalize URLs by trimming before duplicate checks and persistence.
- Update and extend unit tests to cover trimming behavior and rejection of invalid URLs (e.g., loopback, missing scheme).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java | Adds URL trimming + validation before duplicate checks/persistence and before provider validation. |
| server/src/test/java/org/apache/cloudstack/dns/DnsProviderManagerImplTest.java | Adjusts existing tests and adds new cases for trimming and invalid URL rejection. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private void validateDnsServerUrl(String url) { | ||
| if (StringUtils.isBlank(url)) { | ||
| return; | ||
| } | ||
| UriUtils.validateUrl(url); | ||
| } |
There was a problem hiding this comment.
I think this comment makes sense (cc @sudo87 ) however, implementing this like co-pilot suggests makes no sense. ftp: or mailto: or many other scheme prefixes would have to be checked. Would we accept any others than http-like ones? (ref https://en.wikipedia.org/wiki/List_of_URI_schemes) I wouldn’t mind dns: (not listed on the page I shared)
There was a problem hiding this comment.
@DaanHoogland @sudo87 @weizhouapache I think the scheme should be ignored here for this validation. The DNS provider should either use the appropriate scheme automatically, or check whether the provided URL has the expected scheme.
|
clgtm, not sure if copilot comment regarding "file" protocol is valid |
a555469 to
6981138
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java:171
- The JavaDoc claims
UriUtils.validateUrl(String)enforces an http/https scheme, butUriUtils.validateUrlalso allowsfile. Either the JavaDoc should be corrected, orvalidateDnsServerUrlshould explicitly enforce http/https (and document that additional restriction).
* 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.
*/
server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java:176
UriUtils.validateUrlpermits thefilescheme (seeUriUtils.validateUrl), so the new DNS server URL validation currently still allowsfile:URLs. For a provider endpoint this is unexpected and can open up local-file / non-HTTP URL handling paths in downstream clients; it also contradicts the intent to require http/https URLs.
private void validateDnsServerUrl(String url) {
if (StringUtils.isBlank(url)) {
throw new IllegalArgumentException("URL cannot be blank.");
}
UriUtils.validateUrl(url);
| if (StringUtils.isBlank(url)) { | ||
| throw new IllegalArgumentException("URL cannot be blank."); | ||
| } | ||
| UriUtils.validateUrl(url); |
There was a problem hiding this comment.
UriUtils.validateUrl does not reject addresses belonging private networks (e.g 192.168.1.1 is still accepted). If this URL specifies a DNS server which the MS will send commands to, then it should be rejected in my opinion.
There was a problem hiding this comment.
@winterhazel , i think RFC1918 adresses are valid targets for site local DNS servers. I do not want to block those. We can consider a blocking feature flag..? (awaiting input from @sudo87 & @weizhouapache )
There was a problem hiding this comment.
Another alternative would be allowing it for root admins, but not for domain admins and regular users, so that only root admins can register DNS servers belonging to the cloud provider's infrastructure
There was a problem hiding this comment.
Agreed with @DaanHoogland, private/site local are valid DNS server targets, plenty of setups will point to an internal DNS server specifically because it's not reachable from the internet, blocking those would make the feature of no use for those scenarios.
Also worth noting that only root admins and domain admins can setup Public DNS server (usable by everyone in the domain/subdomain), while regular users can only setup their own private one.
| private void validateDnsServerUrl(String url) { | ||
| if (StringUtils.isBlank(url)) { | ||
| return; | ||
| } | ||
| UriUtils.validateUrl(url); | ||
| } |
There was a problem hiding this comment.
@DaanHoogland @sudo87 @weizhouapache I think the scheme should be ignored here for this validation. The DNS provider should either use the appropriate scheme automatically, or check whether the provided URL has the expected scheme.
|
🔴 Test Coverage Grade:
|
| Metric | Value |
|---|---|
| Line coverage | 24.51% |
| Branch coverage | 18.67% |
Grade Scale
| Grade | Line Coverage | Meaning |
|---|---|---|
| 🟢 A | ≥ 80% | Excellent - this code sleeps well at night 😴 |
| 🟡 B | 60-79% | Good - almost there, don't stop now 😉 |
| 🟠 C | 40-59% | Acceptable - your code is wearing a seatbelt, but no airbags 😬 |
| 🔴 D | 20-39% | Marginal - boldly shipping where no test has gone before 🖖 |
| ⛔ F | < 20% | Failing - tests? what tests? 🔥 |
Branch coverage is shown as a secondary signal. Grade is determined by line coverage.
View full Actions run
|
@blueorangutan package |
|
@weizhouapache a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 18883 |
Description
DnsProviderManagerImpl.addDnsServer/updateDnsServer accepted a user-controlled URL and passed it straight to PowerDnsProvider/PowerDnsClient with zero host validation — no egress check at all, unlike the template/webhook paths.
Added validateDnsServerUrl(), called at the top of addDnsServer and whenever updateDnsServer changes the URL.
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?