Copilot commented on code in PR #1213:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/1213#discussion_r4535573489
##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/utils/JarUriValidationUtils.java:
##########
@@ -92,11 +140,134 @@ public static Optional<String> validateJarURI(
return Optional.empty();
}
+ private static boolean isExempt(URI uri, Collection<String>
allowedUriPrefixes) {
+ if (uri.getRawUserInfo() != null || hasDotSegment(uri.getPath())) {
+ return false;
+ }
+ return allowedUriPrefixes.stream().anyMatch(prefix ->
matchesPrefix(uri, prefix));
+ }
+
+ private static boolean matchesPrefix(URI uri, String prefix) {
+ URI prefixUri;
+ try {
+ prefixUri = new URI(prefix.trim());
+ } catch (URISyntaxException e) {
+ return false;
+ }
+ if (prefixUri.getScheme() == null
+ || prefixUri.getHost() == null
+ || prefixUri.getRawUserInfo() != null
+ || prefixUri.getRawQuery() != null
+ || prefixUri.getRawFragment() != null
+ || hasDotSegment(prefixUri.getPath())) {
+ return false;
+ }
+ if (!prefixUri.getScheme().equalsIgnoreCase(uri.getScheme())
+ || !prefixUri.getHost().equalsIgnoreCase(uri.getHost())
+ || effectivePort(prefixUri) != effectivePort(uri)) {
+ return false;
+ }
+ String prefixPath = prefixUri.getPath();
+ if (prefixPath.isEmpty() || prefixPath.equals("/")) {
+ return true;
+ }
+ String path = uri.getPath() == null ? "" : uri.getPath();
+ String prefixDir = prefixPath.endsWith("/") ? prefixPath : prefixPath
+ "/";
+ return path.equals(prefixPath) || path.startsWith(prefixDir);
+ }
+
+ private static int effectivePort(URI uri) {
+ if (uri.getPort() != -1) {
+ return uri.getPort();
+ }
+ if ("https".equalsIgnoreCase(uri.getScheme())) {
+ return 443;
+ }
+ if ("http".equalsIgnoreCase(uri.getScheme())) {
+ return 80;
+ }
+ return -1;
+ }
+
+ private static boolean hasDotSegment(String path) {
+ if (path == null) {
+ return false;
+ }
+ for (String segment : path.split("/", -1)) {
Review Comment:
With the allowed prefix `https://10.0.0.1/repo/`, the URI
`https://10.0.0.1/repo/..%5cadmin/job.jar` is exempted because `getPath()`
decodes `%5c` to a backslash, but this check only splits on `/`. An internal
server that treats backslashes as separators can normalize that request to
`/admin/job.jar`, outside the trusted prefix, without a redirect to revalidate.
Treat decoded backslashes as separators (or reject them) before checking dot
segments, and add this case to the negative-prefix tests.
##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/utils/JarUriValidationUtils.java:
##########
@@ -23,22 +23,69 @@
import java.net.URI;
import java.net.URISyntaxException;
import java.net.UnknownHostException;
+import java.util.Arrays;
import java.util.Collection;
+import java.util.List;
import java.util.Locale;
import java.util.Optional;
import java.util.Set;
import java.util.stream.Collectors;
+import java.util.stream.Stream;
/**
* Shared jarURI validation (scheme allowlist plus restricted-host checks),
used both at
* admission/reconcile time and to re-validate every hop an artifact fetch is
redirected through.
*/
public final class JarUriValidationUtils {
+ private static final List<AddressRange> RESTRICTED_RANGES =
+ Stream.of(
+ "0.0.0.0/8",
+ "10.0.0.0/8",
+ "100.64.0.0/10",
+ "127.0.0.0/8",
+ "169.254.0.0/16",
+ "172.16.0.0/12",
+ "192.0.0.0/24",
Review Comment:
Blocking the entire `192.0.0.0/24` also rejects `192.0.0.9` and
`192.0.0.10`, which are globally reachable exceptions in the IANA registry (PCP
and TURN anycast). Both were accepted previously and now produce a
restricted-address error, including when embedded in NAT64 or 6to4 addresses.
Please preserve those exceptions rather than treating every address in the
parent range as non-global, and add positive boundary cases for them.
##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/utils/JarUriValidationUtils.java:
##########
@@ -23,22 +23,69 @@
import java.net.URI;
import java.net.URISyntaxException;
import java.net.UnknownHostException;
+import java.util.Arrays;
import java.util.Collection;
+import java.util.List;
import java.util.Locale;
import java.util.Optional;
import java.util.Set;
import java.util.stream.Collectors;
+import java.util.stream.Stream;
/**
* Shared jarURI validation (scheme allowlist plus restricted-host checks),
used both at
* admission/reconcile time and to re-validate every hop an artifact fetch is
redirected through.
*/
public final class JarUriValidationUtils {
+ private static final List<AddressRange> RESTRICTED_RANGES =
+ Stream.of(
+ "0.0.0.0/8",
+ "10.0.0.0/8",
+ "100.64.0.0/10",
+ "127.0.0.0/8",
+ "169.254.0.0/16",
+ "172.16.0.0/12",
+ "192.0.0.0/24",
+ "192.0.2.0/24",
+ "192.88.99.0/24",
+ "192.168.0.0/16",
+ "198.18.0.0/15",
+ "198.51.100.0/24",
+ "203.0.113.0/24",
+ "224.0.0.0/4",
+ "240.0.0.0/4",
+ "::/128",
+ "::1/128",
+ "64:ff9b:1::/48",
+ "100::/64",
+ "2001:2::/48",
Review Comment:
The IPv6 blocklist still omits non-globally-reachable special-purpose
ranges, including `100:0:0:1::/64` (Dummy IPv6 Prefix) and `2001:10::/28`
(ORCHID). Calling the updated validator with `https://[100:0:0:1::1]/job.jar`
or `https://[2001:10::1]/job.jar`, restricted-host checking enabled, and an
empty allowlist returns no error: neither range is blocked or handled by
`embeddedIpv4`. Please include these ranges and add rejection cases so these
destinations cannot bypass the stated non-global-address policy.
--
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]
