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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
