You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by "DaanHoogland (via GitHub)" <gi...@apache.org> on 2026/08/07 10:03:22 UTC
[PR] DNS Provider URL Validation [cloudstack]
DaanHoogland opened a new pull request, #13821:
URL: https://github.com/apache/cloudstack/pull/13821
### 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.
<!--- Describe your changes in DETAIL - And how has behaviour functionally changed. -->
<!-- For new features, provide link to FS, dev ML discussion etc. -->
<!-- In case of bug fix, the expected and actual behaviours, steps to reproduce. -->
<!-- When "Fixes: #<id>" is specified, the issue/PR will automatically be closed when this PR gets merged -->
<!-- For addressing multiple issues/PRs, use multiple "Fixes: #<id>" -->
<!-- Fixes: # -->
<!--- ******************************************************************************* -->
<!--- NOTE: AUTOMATION USES THE DESCRIPTIONS TO SET LABELS AND PRODUCE DOCUMENTATION. -->
<!--- PLEASE PUT AN 'X' in only **ONE** box -->
<!--- ******************************************************************************* -->
### Types of changes
- [ ] Breaking change (fix or feature that would cause existing functionality to change)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Bug fix (non-breaking change which fixes an issue)
- [ ] Enhancement (improves an existing feature and functionality)
- [ ] Cleanup (Code refactoring and cleanup, that may add test cases)
- [ ] Build/CI
- [ ] Test (unit or integration test code)
### Feature/Enhancement Scale or Bug Severity
#### Feature/Enhancement Scale
- [ ] Major
- [ ] Minor
#### Bug Severity
- [ ] BLOCKER
- [ ] Critical
- [ ] Major
- [ ] Minor
- [ ] Trivial
### Screenshots (if appropriate):
### How Has This Been Tested?
<!-- Please describe in detail how you tested your changes. -->
<!-- Include details of your testing environment, and the tests you ran to -->
#### How did you try to break this feature and the system with this change?
<!-- see how your change affects other areas of the code, etc. -->
<!-- Please read the [CONTRIBUTING](https://github.com/apache/cloudstack/blob/main/CONTRIBUTING.md) document -->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#issuecomment-5397198849
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 18972
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#issuecomment-5313447550
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 18883
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "sudo87 (via GitHub)" <gi...@apache.org>.
sudo87 commented on code in PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#discussion_r3803085331
##########
server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java:
##########
@@ -162,14 +164,43 @@ private DnsProvider getProviderByType(DnsProviderType type) {
throw new CloudRuntimeException("No plugin found for DNS provider type: " + 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, 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);
+ if (StringUtils.isBlank(trimmedUrl)) {
+ throw new InvalidParameterValueException("URL cannot be blank.");
+ }
+ Pair<String, Integer> hostAndPort;
+ try {
+ 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.");
+ }
Review Comment:
UriUtils.validateUrl() rejects loopback/link-local/multicast/any-local for every caller, including root
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#issuecomment-5368826786
@blueorangutan package
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "winterhazel (via GitHub)" <gi...@apache.org>.
winterhazel commented on code in PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#discussion_r3813845487
##########
server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java:
##########
@@ -162,14 +158,41 @@ private DnsProvider getProviderByType(DnsProviderType type) {
throw new CloudRuntimeException("No plugin found for DNS provider type: " + 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.
+ */
+ private void validateDnsServerUrl(String trimmedUrl, Account caller) {
+ if (StringUtils.isBlank(trimmedUrl)) {
+ throw new InvalidParameterValueException("URL cannot be blank.");
+ }
+ Pair<String, Integer> hostAndPort;
+ try {
+ 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.");
+ }
Review Comment:
@sudo87 this suggestion by Copilot seems to make sense
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "sudo87 (via GitHub)" <gi...@apache.org>.
sudo87 commented on code in PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#discussion_r3819581623
##########
server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java:
##########
@@ -162,14 +158,41 @@ private DnsProvider getProviderByType(DnsProviderType type) {
throw new CloudRuntimeException("No plugin found for DNS provider type: " + 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.
+ */
+ private void validateDnsServerUrl(String trimmedUrl, Account caller) {
+ if (StringUtils.isBlank(trimmedUrl)) {
+ throw new InvalidParameterValueException("URL cannot be blank.");
+ }
+ Pair<String, Integer> hostAndPort;
+ try {
+ 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.");
+ }
Review Comment:
@winterhazel There are few options:
1. Only allow root admin to perform add/update/delete DNS server operations. This will be sufficient to prevent it
2. Restrict non-root users to provide IP for DNS registration and it will enable users to setup their dns server
3. We resolve ip for the url for each transaction and ip must adhere to the restrictions placed for non-root users. We can add some global settings to define those. This will introduce additional latency for each request.
We prefer 2 option
cc: @weizhouapache
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#issuecomment-5372678652
## [](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=13821) **Quality Gate passed**
Issues
 [9 New issues](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=13821&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=13821&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=13821&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [69.7% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=13821&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=13821&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=13821)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache merged PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "winterhazel (via GitHub)" <gi...@apache.org>.
winterhazel commented on code in PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#discussion_r3830597306
##########
server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java:
##########
@@ -162,14 +157,36 @@ private DnsProvider getProviderByType(DnsProviderType type) {
throw new CloudRuntimeException("No plugin found for DNS provider type: " + 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).
+ *
+ * @throws InvalidParameterValueException if the URL is blank, fails validation
+ */
+ private void validateDnsServerUrl(String trimmedUrl, Account caller) {
Review Comment:
(from the SonarCloud code analysis)
`caller` is not used anymore.
##########
server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java:
##########
@@ -162,14 +157,36 @@ private DnsProvider getProviderByType(DnsProviderType type) {
throw new CloudRuntimeException("No plugin found for DNS provider type: " + type);
}
+ /**
+ * Trims and rejects a DNS provider URL that resolves to an illegal address before any provider client
Review Comment:
```suggestion
* Rejects a DNS provider URL that resolves to an illegal address before any provider client
```
It does not trim anymore
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#issuecomment-5394253920
@blueorangutan test
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#issuecomment-5396985021
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 18971
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#issuecomment-5216245918
## [](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=13821) **Quality Gate passed**
Issues
 [5 New issues](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=13821&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=13821&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=13821&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [64.7% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=13821&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=13821&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=13821)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#issuecomment-5215711010
## [Codecov](https://app.codecov.io/gh/apache/cloudstack/pull/13821?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 3.41%. Comparing base ([`4f11707`](https://app.codecov.io/gh/apache/cloudstack/commit/4f117071c9397b1e4714c8fb61c384883c872842?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`c0b5e89`](https://app.codecov.io/gh/apache/cloudstack/commit/c0b5e8963e9d55f590dfd97964aa37f422f7d1be?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 3 commits behind head on main.
> :exclamation: There is a different number of reports uploaded between BASE (4f11707) and HEAD (c0b5e89). Click for more details.
>
> <details><summary>HEAD has 1 upload less than BASE</summary>
>
>| Flag | BASE (4f11707) | HEAD (c0b5e89) |
>|------|------|------|
>|unittests|1|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #13821 +/- ##
=============================================
- Coverage 19.65% 3.41% -16.24%
=============================================
Files 6368 487 -5881
Lines 574881 41867 -533014
Branches 70351 7912 -62439
=============================================
- Hits 112970 1429 -111541
+ Misses 449639 40238 -409401
+ Partials 12272 200 -12072
```
| [Flag](https://app.codecov.io/gh/apache/cloudstack/pull/13821/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [uitests](https://app.codecov.io/gh/apache/cloudstack/pull/13821/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `3.41% <ø> (ø)` | |
| [unittests](https://app.codecov.io/gh/apache/cloudstack/pull/13821/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/cloudstack/pull/13821?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "winterhazel (via GitHub)" <gi...@apache.org>.
winterhazel commented on code in PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#discussion_r3793301483
##########
server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java:
##########
@@ -162,14 +163,29 @@ private DnsProvider getProviderByType(DnsProviderType type) {
throw new CloudRuntimeException("No plugin found for DNS provider type: " + type);
}
+ /**
+ * Rejects a DNS provider URL that resolves to an illegal address before any provider client is given
+ * the chance to connect to it. See {@link UriUtils#validateUrl(String)} for the exact rules enforced
+ * (including the requirement that the URL declares an {@code http}/{@code https} scheme).
+ * Expects {@code url} to already be trimmed.
+ */
+ private void validateDnsServerUrl(String url) {
+ if (StringUtils.isBlank(url)) {
+ throw new IllegalArgumentException("URL cannot be blank.");
+ }
+ UriUtils.validateUrl(url);
Review 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on code in PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#discussion_r3803274230
##########
server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java:
##########
@@ -162,14 +164,43 @@ private DnsProvider getProviderByType(DnsProviderType type) {
throw new CloudRuntimeException("No plugin found for DNS provider type: " + 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, 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);
+ if (StringUtils.isBlank(trimmedUrl)) {
+ throw new InvalidParameterValueException("URL cannot be blank.");
+ }
+ Pair<String, Integer> hostAndPort;
+ try {
+ 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) {
Account caller = CallContext.current().getCallingAccount();
- DnsServer existing = dnsServerDao.findByUrlAndAccount(cmd.getUrl(), caller.getId());
+ String url = validateDnsServerUrl(cmd.getUrl(), caller);
Review Comment:
same as line 285
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#issuecomment-5408433680
<b>[SF] Trillian test result (tid-16814)</b>
Environment: kvm-ol8 (x2), zone: Advanced Networking with Mgmt server ol8
Total time taken: 61279 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr13821-t16814-kvm-ol8.zip
Smoke tests completed. 152 look OK, 4 have errors, 0 did not run
Only failed and skipped tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
ContextSuite context=TestClusterDRS>:setup | `Error` | 0.00 | test_cluster_drs.py
test_09_extension_deploy_vm | `Error` | 54.50 | test_extension_lifecycle.py
test_02_L2_persistent_network | `Failure` | 102.11 | test_persistent_network.py
test_03_deploy_and_destroy_VM_and_verify_network_resources_persist | `Failure` | 113.87 | test_persistent_network.py
test_01_ssl_offloading_isolated_network | `Failure` | 715.01 | test_ssl_offloading.py
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "sudo87 (via GitHub)" <gi...@apache.org>.
sudo87 commented on code in PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#discussion_r3802944109
##########
server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java:
##########
@@ -162,14 +163,29 @@ private DnsProvider getProviderByType(DnsProviderType type) {
throw new CloudRuntimeException("No plugin found for DNS provider type: " + type);
}
+ /**
+ * Rejects a DNS provider URL that resolves to an illegal address before any provider client is given
+ * the chance to connect to it. See {@link UriUtils#validateUrl(String)} for the exact rules enforced
+ * (including the requirement that the URL declares an {@code http}/{@code https} scheme).
+ * Expects {@code url} to already be trimmed.
+ */
+ private void validateDnsServerUrl(String url) {
+ if (StringUtils.isBlank(url)) {
+ throw new IllegalArgumentException("URL cannot be blank.");
+ }
+ UriUtils.validateUrl(url);
Review Comment:
Thanks @winterhazel and @wido, updated the PR to restrict private/internal DNS server addresses to root admin only.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#issuecomment-5333043573
@blueorangutan test
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#discussion_r3804086140
##########
server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java:
##########
@@ -162,14 +164,43 @@ private DnsProvider getProviderByType(DnsProviderType type) {
throw new CloudRuntimeException("No plugin found for DNS provider type: " + 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, fails validation, or is a private address
+ * requested by a non-root-admin caller.
+ */
+ private String validateDnsServerUrl(String trimmedUrl, Account caller) {
+ if (StringUtils.isBlank(trimmedUrl)) {
+ throw new InvalidParameterValueException("URL cannot be blank.");
+ }
+ Pair<String, Integer> hostAndPort;
+ try {
+ 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;
+ }
Review Comment:
The new validation only blocks site-local (RFC1918) addresses for non-root admins. It does not explicitly block other internal/unsafe destinations (e.g., loopback 127.0.0.0/8 / ::1, link-local 169.254.0.0/16 / fe80::/10, multicast, unspecified), which can still enable SSRF/egress to local services. This is also inconsistent with the newly added tests that expect loopback URLs to be rejected. Consider extending the check to reject loopback/link-local/etc. for all callers (or at least non-root), and ensure hostname inputs are validated against their resolved IPs (not just the raw host string) if `UriUtils.validateUrl` doesn’t already do that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#issuecomment-5332059125
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "winterhazel (via GitHub)" <gi...@apache.org>.
winterhazel commented on code in PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#discussion_r3813822529
##########
server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java:
##########
@@ -162,14 +158,41 @@ private DnsProvider getProviderByType(DnsProviderType type) {
throw new CloudRuntimeException("No plugin found for DNS provider type: " + 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.
+ */
+ private void validateDnsServerUrl(String trimmedUrl, Account caller) {
+ if (StringUtils.isBlank(trimmedUrl)) {
+ throw new InvalidParameterValueException("URL cannot be blank.");
+ }
+ Pair<String, Integer> hostAndPort;
+ try {
+ hostAndPort = UriUtils.validateUrl(trimmedUrl);
+ } catch (IllegalArgumentException e) {
+ throw new InvalidParameterValueException(e.getMessage());
+ }
+ if (!accountMgr.isRootAdmin(caller.getId()) && NetUtils.isSiteLocalAddress(hostAndPort.first())) {
Review Comment:
```suggestion
if (NetUtils.isSiteLocalAddress(hostAndPort.first()) && !accountMgr.isRootAdmin(caller.getId())) {
```
I would invert these two because `isSiteLocalAddress` is a simpler operation that does not query the database
##########
server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java:
##########
@@ -162,14 +158,41 @@ private DnsProvider getProviderByType(DnsProviderType type) {
throw new CloudRuntimeException("No plugin found for DNS provider type: " + 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.
+ */
+ private void validateDnsServerUrl(String trimmedUrl, Account caller) {
+ if (StringUtils.isBlank(trimmedUrl)) {
+ throw new InvalidParameterValueException("URL cannot be blank.");
+ }
+ Pair<String, Integer> hostAndPort;
+ try {
+ 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.");
+ }
Review Comment:
@sudo87 this suggestion seems to make sense
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#issuecomment-5369996742
@DaanHoogland @winterhazel @wido
could you please review this PR ? thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#issuecomment-5301407815
## [](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=13821) **Quality Gate passed**
Issues
 [5 New issues](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=13821&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=13821&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=13821&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [69.2% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=13821&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=13821&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=13821)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "sudo87 (via GitHub)" <gi...@apache.org>.
sudo87 commented on code in PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#discussion_r3829182390
##########
server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java:
##########
@@ -162,14 +158,41 @@ private DnsProvider getProviderByType(DnsProviderType type) {
throw new CloudRuntimeException("No plugin found for DNS provider type: " + 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.
+ */
+ private void validateDnsServerUrl(String trimmedUrl, Account caller) {
+ if (StringUtils.isBlank(trimmedUrl)) {
+ throw new InvalidParameterValueException("URL cannot be blank.");
+ }
+ Pair<String, Integer> hostAndPort;
+ try {
+ 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.");
+ }
Review Comment:
First option is implemented for now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on code in PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#discussion_r3735010162
##########
server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java:
##########
@@ -162,14 +163,29 @@ private DnsProvider getProviderByType(DnsProviderType type) {
throw new CloudRuntimeException("No plugin found for DNS provider type: " + type);
}
+ /**
+ * Rejects a DNS provider URL that resolves to an illegal address before any provider client is given
+ * the chance to connect to it. See {@link UriUtils#validateUrl(String)} for the exact rules enforced
+ * (including the requirement that the URL declares an {@code http}/{@code https} scheme).
+ * Expects {@code url} to already be trimmed.
+ */
+ private void validateDnsServerUrl(String url) {
+ if (StringUtils.isBlank(url)) {
+ return;
Review Comment:
URL is a required API parameter, maybe throw an exception here ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "sudo87 (via GitHub)" <gi...@apache.org>.
sudo87 commented on code in PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#discussion_r3802068917
##########
server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java:
##########
@@ -162,14 +163,29 @@ private DnsProvider getProviderByType(DnsProviderType type) {
throw new CloudRuntimeException("No plugin found for DNS provider type: " + type);
}
+ /**
+ * Rejects a DNS provider URL that resolves to an illegal address before any provider client is given
+ * the chance to connect to it. See {@link UriUtils#validateUrl(String)} for the exact rules enforced
+ * (including the requirement that the URL declares an {@code http}/{@code https} scheme).
+ * Expects {@code url} to already be trimmed.
+ */
+ private void validateDnsServerUrl(String url) {
+ if (StringUtils.isBlank(url)) {
+ throw new IllegalArgumentException("URL cannot be blank.");
+ }
+ UriUtils.validateUrl(url);
Review Comment:
Yeah I am fine with this restriction, since the MS is connecting to this URL directly. Just want to flag the tradeoff: it means a domain admin can no longer set up a private DNS server for their own domain and make it public, and regular users can't register a private one for themselves either, only root admin gets to use a private/internal address at all.
To not lose that use case entirely, we would probably want to let root admin set up a DNS server on behalf of a specific domain or account, so a domain admin/user can still end up with a private DNS server, provisioned by root admin rather than self-serve. Could be considered for future enhancement.
cc: @wido
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "winterhazel (via GitHub)" <gi...@apache.org>.
winterhazel commented on code in PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#discussion_r3785839025
##########
server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java:
##########
@@ -162,14 +163,29 @@ private DnsProvider getProviderByType(DnsProviderType type) {
throw new CloudRuntimeException("No plugin found for DNS provider type: " + type);
}
+ /**
+ * Rejects a DNS provider URL that resolves to an illegal address before any provider client is given
+ * the chance to connect to it. See {@link UriUtils#validateUrl(String)} for the exact rules enforced
+ * (including the requirement that the URL declares an {@code http}/{@code https} scheme).
+ * Expects {@code url} to already be trimmed.
+ */
+ private void validateDnsServerUrl(String url) {
+ if (StringUtils.isBlank(url)) {
+ return;
+ }
+ UriUtils.validateUrl(url);
+ }
Review Comment:
@DaanHoogland @sudo87 @weizhouapache I think the scheme should be ignored here for the validation. The DNS provider should either use the appropriate scheme automatically, or check whether the provided URL has the expected scheme.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#issuecomment-5396342615
@blueorangutan package
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#issuecomment-5333061218
@weizhouapache a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "winterhazel (via GitHub)" <gi...@apache.org>.
winterhazel commented on code in PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#discussion_r3785686431
##########
server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java:
##########
@@ -162,14 +163,29 @@ private DnsProvider getProviderByType(DnsProviderType type) {
throw new CloudRuntimeException("No plugin found for DNS provider type: " + type);
}
+ /**
+ * Rejects a DNS provider URL that resolves to an illegal address before any provider client is given
+ * the chance to connect to it. See {@link UriUtils#validateUrl(String)} for the exact rules enforced
+ * (including the requirement that the URL declares an {@code http}/{@code https} scheme).
+ * Expects {@code url} to already be trimmed.
Review Comment:
Can we trim the URL inside the method instead of expecting it to be trimmed?
##########
server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java:
##########
@@ -162,14 +163,29 @@ private DnsProvider getProviderByType(DnsProviderType type) {
throw new CloudRuntimeException("No plugin found for DNS provider type: " + type);
}
+ /**
+ * Rejects a DNS provider URL that resolves to an illegal address before any provider client is given
+ * the chance to connect to it. See {@link UriUtils#validateUrl(String)} for the exact rules enforced
+ * (including the requirement that the URL declares an {@code http}/{@code https} scheme).
+ * Expects {@code url} to already be trimmed.
+ */
+ private void validateDnsServerUrl(String url) {
+ if (StringUtils.isBlank(url)) {
+ throw new IllegalArgumentException("URL cannot be blank.");
Review Comment:
```suggestion
throw new InvalidParameterValueException("URL cannot be blank.");
```
A InvalidParameterValueException makes more sense. Also, can we catch the exception of UriUtils.validateUrl and throw a InvalidParameterValueException too?
##########
server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java:
##########
@@ -162,14 +163,29 @@ private DnsProvider getProviderByType(DnsProviderType type) {
throw new CloudRuntimeException("No plugin found for DNS provider type: " + type);
}
+ /**
+ * Rejects a DNS provider URL that resolves to an illegal address before any provider client is given
+ * the chance to connect to it. See {@link UriUtils#validateUrl(String)} for the exact rules enforced
+ * (including the requirement that the URL declares an {@code http}/{@code https} scheme).
+ * Expects {@code url} to already be trimmed.
+ */
+ private void validateDnsServerUrl(String url) {
+ if (StringUtils.isBlank(url)) {
+ throw new IllegalArgumentException("URL cannot be blank.");
+ }
+ UriUtils.validateUrl(url);
Review Comment:
`UriUtils.validateUrl` does not reject addresses belonging private networks (e.g `192.168.1.1`) are still accepted. If this URL specifies a DNS server which the MS will send commands to, then it should be rejected in my opinion.
cc @sudo87 @weizhouapache
##########
server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java:
##########
@@ -162,14 +163,29 @@ private DnsProvider getProviderByType(DnsProviderType type) {
throw new CloudRuntimeException("No plugin found for DNS provider type: " + type);
}
+ /**
+ * Rejects a DNS provider URL that resolves to an illegal address before any provider client is given
+ * the chance to connect to it. See {@link UriUtils#validateUrl(String)} for the exact rules enforced
+ * (including the requirement that the URL declares an {@code http}/{@code https} scheme).
+ * Expects {@code url} to already be trimmed.
+ */
+ private void validateDnsServerUrl(String url) {
+ if (StringUtils.isBlank(url)) {
+ return;
+ }
+ UriUtils.validateUrl(url);
+ }
Review Comment:
@DaanHoogland @sudo87 @weizhouapache I think the scheme should be ignored here for the validation. The provider should either use the appropriate scheme automatically, or check whether the provided URL has the expected scheme.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on code in PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#discussion_r3749434599
##########
server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java:
##########
@@ -162,14 +163,29 @@ private DnsProvider getProviderByType(DnsProviderType type) {
throw new CloudRuntimeException("No plugin found for DNS provider type: " + type);
}
+ /**
+ * Rejects a DNS provider URL that resolves to an illegal address before any provider client is given
+ * the chance to connect to it. See {@link UriUtils#validateUrl(String)} for the exact rules enforced
+ * (including the requirement that the URL declares an {@code http}/{@code https} scheme).
+ * Expects {@code url} to already be trimmed.
+ */
+ private void validateDnsServerUrl(String url) {
+ if (StringUtils.isBlank(url)) {
+ return;
+ }
+ UriUtils.validateUrl(url);
+ }
Review 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)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on code in PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#discussion_r3829452751
##########
server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java:
##########
@@ -251,12 +270,14 @@ public DnsServer updateDnsServer(UpdateDnsServerCmd cmd) {
}
if (cmd.getUrl() != null) {
Review Comment:
use StringUtils.isNotBlank ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#issuecomment-5369680223
## [](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=13821) **Quality Gate passed**
Issues
 [10 New issues](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=13821&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=13821&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=13821&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [69.7% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=13821&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=13821&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=13821)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#issuecomment-5369994283
@blueorangutan test
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#issuecomment-5368852817
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache closed pull request #13821: DNS Provider URL Validation
URL: https://github.com/apache/cloudstack/pull/13821
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#issuecomment-5397587382
## [](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=13821) **Quality Gate passed**
Issues
 [10 New issues](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=13821&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=13821&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=13821&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [69.7% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=13821&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=13821&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=13821)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#issuecomment-5397616716
@weizhouapache a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#issuecomment-5397601681
@blueorangutan test
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#issuecomment-5327217068
## [](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=13821) **Quality Gate passed**
Issues
 [8 New issues](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=13821&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=13821&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=13821&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [70.4% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=13821&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=13821&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=13821)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on code in PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#discussion_r3804035307
##########
server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java:
##########
@@ -162,14 +164,43 @@ private DnsProvider getProviderByType(DnsProviderType type) {
throw new CloudRuntimeException("No plugin found for DNS provider type: " + 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, fails validation, or is a private address
+ * requested by a non-root-admin caller.
+ */
+ private String validateDnsServerUrl(String trimmedUrl, Account caller) {
+ if (StringUtils.isBlank(trimmedUrl)) {
+ throw new InvalidParameterValueException("URL cannot be blank.");
+ }
+ Pair<String, Integer> hostAndPort;
+ try {
+ 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;
Review Comment:
`return` a url seems not needed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on code in PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#discussion_r3810774285
##########
server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java:
##########
@@ -162,14 +164,43 @@ private DnsProvider getProviderByType(DnsProviderType type) {
throw new CloudRuntimeException("No plugin found for DNS provider type: " + 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, 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);
+ if (StringUtils.isBlank(trimmedUrl)) {
+ throw new InvalidParameterValueException("URL cannot be blank.");
+ }
+ Pair<String, Integer> hostAndPort;
+ try {
+ 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) {
Account caller = CallContext.current().getCallingAccount();
- DnsServer existing = dnsServerDao.findByUrlAndAccount(cmd.getUrl(), caller.getId());
+ String url = validateDnsServerUrl(cmd.getUrl(), caller);
Review Comment:
if we trim before every use of the method, we might as well keep the trim internal to the method.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#discussion_r3735071718
##########
server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java:
##########
@@ -162,14 +163,29 @@ private DnsProvider getProviderByType(DnsProviderType type) {
throw new CloudRuntimeException("No plugin found for DNS provider type: " + type);
}
+ /**
+ * Rejects a DNS provider URL that resolves to an illegal address before any provider client is given
+ * the chance to connect to it. See {@link UriUtils#validateUrl(String)} for the exact rules enforced
+ * (including the requirement that the URL declares an {@code http}/{@code https} scheme).
+ * Expects {@code url} to already be trimmed.
+ */
+ private void validateDnsServerUrl(String url) {
+ if (StringUtils.isBlank(url)) {
+ return;
+ }
+ UriUtils.validateUrl(url);
+ }
Review Comment:
`validateDnsServerUrl` currently returns early for blank/empty values, which allows `null`/"" URLs to pass through to the duplicate check and persistence. Also, the Javadoc claims only `http/https` are allowed, but `UriUtils.validateUrl` accepts `file` URLs (e.g. `file://192.0.2.1/...`) which is unlikely to be a valid DNS provider endpoint and can open up unintended access paths. Consider rejecting blank URLs and explicitly disallowing `file:` before delegating to `UriUtils.validateUrl`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "sudo87 (via GitHub)" <gi...@apache.org>.
sudo87 commented on code in PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#discussion_r3818503504
##########
server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java:
##########
@@ -162,14 +158,41 @@ private DnsProvider getProviderByType(DnsProviderType type) {
throw new CloudRuntimeException("No plugin found for DNS provider type: " + 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.
+ */
+ private void validateDnsServerUrl(String trimmedUrl, Account caller) {
+ if (StringUtils.isBlank(trimmedUrl)) {
+ throw new InvalidParameterValueException("URL cannot be blank.");
+ }
+ Pair<String, Integer> hostAndPort;
+ try {
+ 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.");
+ }
Review Comment:
@winterhazel sure, will try to address this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#discussion_r3804380644
##########
server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java:
##########
@@ -162,14 +158,41 @@ private DnsProvider getProviderByType(DnsProviderType type) {
throw new CloudRuntimeException("No plugin found for DNS provider type: " + 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.
+ */
+ private void validateDnsServerUrl(String trimmedUrl, Account caller) {
+ if (StringUtils.isBlank(trimmedUrl)) {
+ throw new InvalidParameterValueException("URL cannot be blank.");
+ }
+ Pair<String, Integer> hostAndPort;
+ try {
+ 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.");
+ }
Review Comment:
If `hostAndPort.first()` can be a hostname (not a literal IP), a single “is private?” check at validation time can be bypassed via DNS rebinding (public IP at validation time, private/loopback later when the provider connects). Consider mitigating by (mandatory) requiring literal IPs for non-root users, or (optional but stronger) resolving once and persisting/using the resolved IP (and/or re-validating right before any outbound connection and pinning the resolution result).
##########
server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java:
##########
@@ -162,14 +158,41 @@ private DnsProvider getProviderByType(DnsProviderType type) {
throw new CloudRuntimeException("No plugin found for DNS provider type: " + 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.
+ */
+ private void validateDnsServerUrl(String trimmedUrl, Account caller) {
+ if (StringUtils.isBlank(trimmedUrl)) {
+ throw new InvalidParameterValueException("URL cannot be blank.");
+ }
+ Pair<String, Integer> hostAndPort;
+ try {
+ 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.");
+ }
+ }
Review Comment:
The validation currently only blocks “site-local” addresses for non-root callers. That does not necessarily cover other clearly-illegal SSRF targets like loopback (127.0.0.0/8, ::1), link-local (169.254.0.0/16, fe80::/10), multicast, or “any local” (0.0.0.0). Given new tests expect loopback rejection, add explicit checks for these address classes (ideally after resolving/parsing the host) and fail with `InvalidParameterValueException` when matched.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#issuecomment-5329193314
@winterhazel @DaanHoogland
can you please review ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#discussion_r3749433014
##########
server/src/test/java/org/apache/cloudstack/dns/DnsProviderManagerImplTest.java:
##########
@@ -781,18 +781,51 @@ public void testListDnsZones() {
public void testAddDnsServerAlreadyExists() {
org.apache.cloudstack.api.command.user.dns.AddDnsServerCmd cmd = mock(
org.apache.cloudstack.api.command.user.dns.AddDnsServerCmd.class);
- when(cmd.getUrl()).thenReturn("http://newpdns:8081");
+ when(cmd.getUrl()).thenReturn("http://192.0.2.1:8081");
when(dnsServerDao.findByUrlAndAccount(anyString(), anyLong())).thenReturn(serverVO);
manager.addDnsServer(cmd);
}
+ @Test
+ public void 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(
+ org.apache.cloudstack.api.command.user.dns.AddDnsServerCmd.class);
+ when(cmd.getUrl()).thenReturn("http://127.0.0.1:8081");
+ 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);
+ }
+
Review Comment:
URL validation tests cover loopback and missing/unsupported schemes, but there’s no test ensuring `file:` URLs are rejected. Given `UriUtils.validateUrl` accepts `file`, this is an important security edge case to lock down with a unit test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#issuecomment-5341727948
@blueorangutan test
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#issuecomment-5369376151
@blueorangutan package
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#issuecomment-5372437284
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 18948
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "sudo87 (via GitHub)" <gi...@apache.org>.
sudo87 commented on code in PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#discussion_r3831490560
##########
server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java:
##########
@@ -162,14 +157,36 @@ private DnsProvider getProviderByType(DnsProviderType type) {
throw new CloudRuntimeException("No plugin found for DNS provider type: " + 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).
+ *
+ * @throws InvalidParameterValueException if the URL is blank, fails validation
+ */
+ private void validateDnsServerUrl(String trimmedUrl, Account caller) {
Review Comment:
oh, missed to clean up both. Thank you for the suggestion.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "winterhazel (via GitHub)" <gi...@apache.org>.
winterhazel commented on PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#issuecomment-5371964403
@blueorangutan package
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "winterhazel (via GitHub)" <gi...@apache.org>.
winterhazel commented on code in PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#discussion_r3797346357
##########
server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java:
##########
@@ -162,14 +163,29 @@ private DnsProvider getProviderByType(DnsProviderType type) {
throw new CloudRuntimeException("No plugin found for DNS provider type: " + type);
}
+ /**
+ * Rejects a DNS provider URL that resolves to an illegal address before any provider client is given
+ * the chance to connect to it. See {@link UriUtils#validateUrl(String)} for the exact rules enforced
+ * (including the requirement that the URL declares an {@code http}/{@code https} scheme).
+ * Expects {@code url} to already be trimmed.
+ */
+ private void validateDnsServerUrl(String url) {
+ if (StringUtils.isBlank(url)) {
+ throw new IllegalArgumentException("URL cannot be blank.");
+ }
+ UriUtils.validateUrl(url);
Review Comment:
@sudo87 I know that they can be valid DNS servers. I do agree that it might make sense to allow root admins to register these DNS servers. However, my point is regarding whether we should allow non-root admin accounts (domain admins, in this case), which we typically assume that do not have knowledge about the cloud provider's internal infrastructure, to register a DNS server (or any other internal address) belonging to the cloud provider's internal infrastructure. Could this be considered a security concern?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "winterhazel (via GitHub)" <gi...@apache.org>.
winterhazel commented on code in PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#discussion_r3797346357
##########
server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java:
##########
@@ -162,14 +163,29 @@ private DnsProvider getProviderByType(DnsProviderType type) {
throw new CloudRuntimeException("No plugin found for DNS provider type: " + type);
}
+ /**
+ * Rejects a DNS provider URL that resolves to an illegal address before any provider client is given
+ * the chance to connect to it. See {@link UriUtils#validateUrl(String)} for the exact rules enforced
+ * (including the requirement that the URL declares an {@code http}/{@code https} scheme).
+ * Expects {@code url} to already be trimmed.
+ */
+ private void validateDnsServerUrl(String url) {
+ if (StringUtils.isBlank(url)) {
+ throw new IllegalArgumentException("URL cannot be blank.");
+ }
+ UriUtils.validateUrl(url);
Review Comment:
@sudo87 I know that they can be valid DNS servers. I do agree that it might make sense to allow root admins to register these DNS servers. However, my point is regarding whether we should allow non-root admin accounts, which we typically assume that do not have knowledge about the cloud provider's internal infrastructure, to register a DNS server (or any other internal address) belonging to the cloud provider's internal infrastructure. Could this be considered a security concern?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "sudo87 (via GitHub)" <gi...@apache.org>.
sudo87 commented on PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#issuecomment-5216331330
clgtm, not sure if copilot comment regarding "file" protocol is valid
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#issuecomment-5240927799
## [](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=13821) **Quality Gate passed**
Issues
 [5 New issues](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=13821&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=13821&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=13821&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [64.7% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=13821&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=13821&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=13821)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "wido (via GitHub)" <gi...@apache.org>.
wido commented on code in PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#discussion_r3802133834
##########
server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java:
##########
@@ -162,14 +163,29 @@ private DnsProvider getProviderByType(DnsProviderType type) {
throw new CloudRuntimeException("No plugin found for DNS provider type: " + type);
}
+ /**
+ * Rejects a DNS provider URL that resolves to an illegal address before any provider client is given
+ * the chance to connect to it. See {@link UriUtils#validateUrl(String)} for the exact rules enforced
+ * (including the requirement that the URL declares an {@code http}/{@code https} scheme).
+ * Expects {@code url} to already be trimmed.
+ */
+ private void validateDnsServerUrl(String url) {
+ if (StringUtils.isBlank(url)) {
+ throw new IllegalArgumentException("URL cannot be blank.");
+ }
+ UriUtils.validateUrl(url);
Review Comment:
> Yeah I am fine with this restriction, since the MS is connecting to this URL directly. Just want to flag the tradeoff: it means a domain admin can no longer set up a private DNS server for their own domain and make it public, and regular users can't register a private one for themselves either, only root admin gets to use a private/internal address at all.
>
> To not lose that use case entirely, we would probably want to let root admin set up a DNS server on behalf of a specific domain or account, so a domain admin/user can still end up with a private DNS server, provisioned by root admin rather than self-serve. Could be considered for future enhancement.
>
> cc: @wido
Agree. I think setting up should be done by the root-admin (for now) and then assigned to a specific domain/account or made public.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "winterhazel (via GitHub)" <gi...@apache.org>.
winterhazel commented on PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#issuecomment-5395334830
I performed the following tests:
- As a root admin, register, update and delete a PowerDNS server in a private network: ok
- As a root admin, attempt to register a DNS server providing a any local/link local/loopback/multicast address: results in an error (illegal URL), ok
- As a domain admin, attempt to register, update and delete a PowerDNS server in a private network: results in an error (restricted to root admins), ok
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#issuecomment-5368591504
@blueorangutan package
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#issuecomment-5369006551
Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 18941
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#issuecomment-5369384134
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "sudo87 (via GitHub)" <gi...@apache.org>.
sudo87 commented on code in PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#discussion_r3819581623
##########
server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java:
##########
@@ -162,14 +158,41 @@ private DnsProvider getProviderByType(DnsProviderType type) {
throw new CloudRuntimeException("No plugin found for DNS provider type: " + 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.
+ */
+ private void validateDnsServerUrl(String trimmedUrl, Account caller) {
+ if (StringUtils.isBlank(trimmedUrl)) {
+ throw new InvalidParameterValueException("URL cannot be blank.");
+ }
+ Pair<String, Integer> hostAndPort;
+ try {
+ 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.");
+ }
Review Comment:
@winterhazel There are few options:
1. Only allow root admin to perform add/update/delete DNS server operations. This will be sufficient to prevent it
2. Restrict non-root users to provide IP for DNS registration and it will enable users to setup their dns server
3. We resolve ip for the url for each transaction and ip must adhere to the restrictions placed for non-root users. We can add some global settings to define those.
We prefer 2 option
cc: @weizhouapache
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#issuecomment-5351125372
<b>[SF] Trillian test result (tid-16782)</b>
Environment: kvm-ol8 (x2), zone: Advanced Networking with Mgmt server ol8
Total time taken: 53832 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr13821-t16782-kvm-ol8.zip
Smoke tests completed. 156 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "winterhazel (via GitHub)" <gi...@apache.org>.
winterhazel commented on code in PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#discussion_r3785839025
##########
server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java:
##########
@@ -162,14 +163,29 @@ private DnsProvider getProviderByType(DnsProviderType type) {
throw new CloudRuntimeException("No plugin found for DNS provider type: " + type);
}
+ /**
+ * Rejects a DNS provider URL that resolves to an illegal address before any provider client is given
+ * the chance to connect to it. See {@link UriUtils#validateUrl(String)} for the exact rules enforced
+ * (including the requirement that the URL declares an {@code http}/{@code https} scheme).
+ * Expects {@code url} to already be trimmed.
+ */
+ private void validateDnsServerUrl(String url) {
+ if (StringUtils.isBlank(url)) {
+ return;
+ }
+ UriUtils.validateUrl(url);
+ }
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#issuecomment-5313018252
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on code in PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#discussion_r3749391657
##########
server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java:
##########
@@ -162,14 +163,29 @@ private DnsProvider getProviderByType(DnsProviderType type) {
throw new CloudRuntimeException("No plugin found for DNS provider type: " + type);
}
+ /**
+ * Rejects a DNS provider URL that resolves to an illegal address before any provider client is given
+ * the chance to connect to it. See {@link UriUtils#validateUrl(String)} for the exact rules enforced
+ * (including the requirement that the URL declares an {@code http}/{@code https} scheme).
+ * Expects {@code url} to already be trimmed.
+ */
+ private void validateDnsServerUrl(String url) {
+ if (StringUtils.isBlank(url)) {
+ return;
Review Comment:
ok
```suggestion
throw new IllegalArgumentException("URL cannot be blank.");
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#issuecomment-5315902618
@weizhouapache a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on code in PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#discussion_r3803270326
##########
server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java:
##########
@@ -162,14 +164,43 @@ private DnsProvider getProviderByType(DnsProviderType type) {
throw new CloudRuntimeException("No plugin found for DNS provider type: " + 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, 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);
+ if (StringUtils.isBlank(trimmedUrl)) {
+ throw new InvalidParameterValueException("URL cannot be blank.");
+ }
+ Pair<String, Integer> hostAndPort;
+ try {
+ 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) {
Account caller = CallContext.current().getCallingAccount();
- DnsServer existing = dnsServerDao.findByUrlAndAccount(cmd.getUrl(), caller.getId());
+ String url = validateDnsServerUrl(cmd.getUrl(), caller);
Review Comment:
should we trim the url before validation so the trimmed url will be checked for duplication and saved in database ?
@sudo87
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#issuecomment-5327221767
<!-- coverage-grade-bot -->
## 🔴 Test Coverage Grade: `D` — Marginal
| Metric | Value |
|--------|-------|
| Line coverage | **24.51%** |
| Branch coverage | **18.68%** |
### 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](https://github.com/apache/cloudstack/actions/runs/32123005448)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#issuecomment-5332042244
@blueorangutan package
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#issuecomment-5324042006
<b>[SF] Trillian test result (tid-16764)</b>
Environment: kvm-ol8 (x2), zone: Advanced Networking with Mgmt server ol8
Total time taken: 58666 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr13821-t16764-kvm-ol8.zip
Smoke tests completed. 156 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#issuecomment-5332496978
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 18906
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#issuecomment-5342500634
<b>[SF] Trillian test result (tid-16780)</b>
Environment: kvm-ol8 (x2), zone: Advanced Networking with Mgmt server ol8
Total time taken: 57998 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr13821-t16780-kvm-ol8.zip
Smoke tests completed. 155 look OK, 1 have errors, 0 did not run
Only failed and skipped tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
test_01_ssl_offloading_isolated_network | `Failure` | 710.67 | test_ssl_offloading.py
test_02_ssl_offloading_project_vpc | `Failure` | 687.94 | test_ssl_offloading.py
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#issuecomment-5371980427
@winterhazel 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#issuecomment-5396091138
Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 18970
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#issuecomment-5395940417
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#issuecomment-5396356196
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#issuecomment-5396553135
@blueorangutan package
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "winterhazel (via GitHub)" <gi...@apache.org>.
winterhazel commented on code in PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#discussion_r3785831329
##########
server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java:
##########
@@ -162,14 +163,29 @@ private DnsProvider getProviderByType(DnsProviderType type) {
throw new CloudRuntimeException("No plugin found for DNS provider type: " + type);
}
+ /**
+ * Rejects a DNS provider URL that resolves to an illegal address before any provider client is given
+ * the chance to connect to it. See {@link UriUtils#validateUrl(String)} for the exact rules enforced
+ * (including the requirement that the URL declares an {@code http}/{@code https} scheme).
+ * Expects {@code url} to already be trimmed.
+ */
+ private void validateDnsServerUrl(String url) {
+ if (StringUtils.isBlank(url)) {
+ throw new IllegalArgumentException("URL cannot be blank.");
+ }
+ UriUtils.validateUrl(url);
Review 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.
cc @sudo87 @weizhouapache
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "sudo87 (via GitHub)" <gi...@apache.org>.
sudo87 commented on code in PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#discussion_r3794634722
##########
server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java:
##########
@@ -162,14 +163,29 @@ private DnsProvider getProviderByType(DnsProviderType type) {
throw new CloudRuntimeException("No plugin found for DNS provider type: " + type);
}
+ /**
+ * Rejects a DNS provider URL that resolves to an illegal address before any provider client is given
+ * the chance to connect to it. See {@link UriUtils#validateUrl(String)} for the exact rules enforced
+ * (including the requirement that the URL declares an {@code http}/{@code https} scheme).
+ * Expects {@code url} to already be trimmed.
+ */
+ private void validateDnsServerUrl(String url) {
+ if (StringUtils.isBlank(url)) {
+ throw new IllegalArgumentException("URL cannot be blank.");
+ }
+ UriUtils.validateUrl(url);
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#issuecomment-5329379686
## [](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=13821) **Quality Gate passed**
Issues
 [8 New issues](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=13821&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=13821&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=13821&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [69.8% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=13821&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=13821&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=13821)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "sudo87 (via GitHub)" <gi...@apache.org>.
sudo87 commented on code in PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#discussion_r3803547791
##########
server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java:
##########
@@ -162,14 +164,43 @@ private DnsProvider getProviderByType(DnsProviderType type) {
throw new CloudRuntimeException("No plugin found for DNS provider type: " + 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, 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);
+ if (StringUtils.isBlank(trimmedUrl)) {
+ throw new InvalidParameterValueException("URL cannot be blank.");
+ }
+ Pair<String, Integer> hostAndPort;
+ try {
+ 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) {
Account caller = CallContext.current().getCallingAccount();
- DnsServer existing = dnsServerDao.findByUrlAndAccount(cmd.getUrl(), caller.getId());
+ String url = validateDnsServerUrl(cmd.getUrl(), caller);
Review Comment:
yeah, we can take out trim logic from validation method. let me update it in some time
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on code in PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#discussion_r3810765039
##########
server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java:
##########
@@ -162,14 +164,43 @@ private DnsProvider getProviderByType(DnsProviderType type) {
throw new CloudRuntimeException("No plugin found for DNS provider type: " + 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, fails validation, or is a private address
+ * requested by a non-root-admin caller.
+ */
+ private String validateDnsServerUrl(String trimmedUrl, Account caller) {
+ if (StringUtils.isBlank(trimmedUrl)) {
+ throw new InvalidParameterValueException("URL cannot be blank.");
+ }
+ Pair<String, Integer> hostAndPort;
+ try {
+ 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;
Review Comment:
that was because of the trim.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#issuecomment-5341745163
@weizhouapache a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#issuecomment-5395921731
@blueorangutan package
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#issuecomment-5394276289
@weizhouapache a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "winterhazel (via GitHub)" <gi...@apache.org>.
winterhazel commented on code in PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#discussion_r3830552658
##########
server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java:
##########
@@ -162,14 +158,41 @@ private DnsProvider getProviderByType(DnsProviderType type) {
throw new CloudRuntimeException("No plugin found for DNS provider type: " + 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.
+ */
+ private void validateDnsServerUrl(String trimmedUrl, Account caller) {
+ if (StringUtils.isBlank(trimmedUrl)) {
+ throw new InvalidParameterValueException("URL cannot be blank.");
+ }
+ Pair<String, Integer> hostAndPort;
+ try {
+ 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.");
+ }
Review Comment:
@sudo87 ok, let's go with option 1 for 4.23.
For future releases, 3 seems like the most interesting option to me. I think that it is worth to check how much latency this adds to the operations.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#issuecomment-5315898121
@blueorangutan test
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#issuecomment-5216250367
## 🔴 Test Coverage Grade: `D` — Marginal
| 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](https://github.com/apache/cloudstack/actions/runs/31168496215)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#issuecomment-5240932164
## 🔴 Test Coverage Grade: `D` — Marginal
| 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](https://github.com/apache/cloudstack/actions/runs/31387596169)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#issuecomment-5301409336
## 🔴 Test Coverage Grade: `D` — Marginal
| 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](https://github.com/apache/cloudstack/actions/runs/31871993120)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on code in PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#discussion_r3788832545
##########
server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java:
##########
@@ -162,14 +163,29 @@ private DnsProvider getProviderByType(DnsProviderType type) {
throw new CloudRuntimeException("No plugin found for DNS provider type: " + type);
}
+ /**
+ * Rejects a DNS provider URL that resolves to an illegal address before any provider client is given
+ * the chance to connect to it. See {@link UriUtils#validateUrl(String)} for the exact rules enforced
+ * (including the requirement that the URL declares an {@code http}/{@code https} scheme).
+ * Expects {@code url} to already be trimmed.
+ */
+ private void validateDnsServerUrl(String url) {
+ if (StringUtils.isBlank(url)) {
+ throw new IllegalArgumentException("URL cannot be blank.");
+ }
+ UriUtils.validateUrl(url);
Review 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 )
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#issuecomment-5313010223
@blueorangutan package
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#discussion_r3803008529
##########
server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java:
##########
@@ -162,14 +164,43 @@ private DnsProvider getProviderByType(DnsProviderType type) {
throw new CloudRuntimeException("No plugin found for DNS provider type: " + 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, 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);
+ if (StringUtils.isBlank(trimmedUrl)) {
+ throw new InvalidParameterValueException("URL cannot be blank.");
+ }
+ Pair<String, Integer> hostAndPort;
+ try {
+ 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.");
+ }
Review Comment:
The validation currently only blocks site-local/private address literals for non-root admins; it does not block loopback (e.g., 127.0.0.1 / ::1), link-local, multicast, unspecified/any-local, nor does it handle hostnames that resolve to private/loopback ranges. This leaves an SSRF/egress gap if a user supplies a hostname that resolves internally, and it also won’t satisfy the new loopback-rejection tests. Recommendation (mandatory): resolve the host to one or more InetAddress values and reject any loopback/link-local/multicast/any-local addresses for all callers, and reject private/site-local/ULA ranges for non-root admins (apply the check to every resolved address).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#issuecomment-5407387250
merging based on smoke tests (failures are unrelated) and manual verification .
thanks @DaanHoogland @sudo87 @winterhazel
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#issuecomment-5395925605
@blueorangutan package
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "sudo87 (via GitHub)" <gi...@apache.org>.
sudo87 commented on code in PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#discussion_r3819581623
##########
server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java:
##########
@@ -162,14 +158,41 @@ private DnsProvider getProviderByType(DnsProviderType type) {
throw new CloudRuntimeException("No plugin found for DNS provider type: " + 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.
+ */
+ private void validateDnsServerUrl(String trimmedUrl, Account caller) {
+ if (StringUtils.isBlank(trimmedUrl)) {
+ throw new InvalidParameterValueException("URL cannot be blank.");
+ }
+ Pair<String, Integer> hostAndPort;
+ try {
+ 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.");
+ }
Review Comment:
@winterhazel we will update the PR to only allow root admin to perform add/update/delete DNS server operations. This will be sufficient to prevent it. cc: @weizhouapache
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#issuecomment-5403907359
<b>[SF] Trillian test result (tid-16813)</b>
Environment: kvm-ol8 (x2), zone: Advanced Networking with Mgmt server ol8
Total time taken: 49513 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr13821-t16813-kvm-ol8.zip
Smoke tests completed. 144 look OK, 1 have errors, 11 did not run
Only failed and skipped tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
test_02_ssl_offloading_project_vpc | `Failure` | 721.97 | test_ssl_offloading.py
all_test_volumes | `Skipped` | --- | test_volumes.py
all_test_vpc_conserve_mode | `Skipped` | --- | test_vpc_conserve_mode.py
all_test_vpc_firewall_rules | `Skipped` | --- | test_vpc_firewall_rules.py
all_test_vpc_ipv6 | `Skipped` | --- | test_vpc_ipv6.py
all_test_vpc_redundant | `Skipped` | --- | test_vpc_redundant.py
all_test_vpc_router_nics | `Skipped` | --- | test_vpc_router_nics.py
all_test_vpc_vpn | `Skipped` | --- | test_vpc_vpn.py
all_test_webhook_delivery | `Skipped` | --- | test_webhook_delivery.py
all_test_webhook_lifecycle | `Skipped` | --- | test_webhook_lifecycle.py
all_test_host_maintenance | `Skipped` | --- | test_host_maintenance.py
all_test_hostha_kvm | `Skipped` | --- | test_hostha_kvm.py
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#issuecomment-5396573447
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DNS Provider URL Validation [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13821:
URL: https://github.com/apache/cloudstack/pull/13821#issuecomment-5369830385
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 18942
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org