vsantwana commented on code in PR #1214:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/1214#discussion_r4426093051
##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/config/KubernetesOperatorConfigOptions.java:
##########
@@ -398,6 +398,21 @@ public static String operatorConfigKey(String key) {
+ "site-local, wildcard or multicast
addresses are rejected during validation. "
+ "Disable only if the operator
legitimately needs to fetch from such addresses.");
+ @Documentation.Section(SECTION_SYSTEM_RECONCILE)
+ public static final ConfigOption<List<String>>
JAR_URI_ALLOWED_URI_PREFIXES =
+ operatorConfig("user.artifacts.allowed-uri-prefixes")
+ .stringType()
+ .asList()
+ .defaultValues()
+ .withDescription(
+ "Comma separated list of jarURI prefixes that are
exempt from the '"
Review Comment:
Flink 1.20 doesn't split list options on commas. I checked with flink-core
1.20.4: `a,b` is read as the single entry `"a,b"`, while `a;b` gives two
entries. A YAML list works in `config.yaml` too. An admin who follows this
description therefore gets one prefix that never matches. `allowed-schemes`
above has the same wording, so both descriptions could be fixed together.
##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/utils/JarUriValidationUtils.java:
##########
@@ -69,7 +72,8 @@ public static Optional<String> validateJarURI(
}
if (("http".equalsIgnoreCase(scheme) ||
"https".equalsIgnoreCase(scheme))
- && disallowRestrictedHosts) {
+ && disallowRestrictedHosts
+ && allowedUriPrefixes.stream().noneMatch(jarURI::startsWith)) {
Review Comment:
This compares the prefix with the raw jarURI string, so the check is skipped
for URIs that the admin did not mean to allow. I ran these against
`validateJarURI` at this head:
- With the prefix `https://10.0.0.1`, the URI
`https://[email protected]/latest/meta-data/` is accepted. `10.0.0.1` is
parsed as userinfo, so the operator fetches from `169.254.169.254`.
- With the same prefix, `https://10.0.0.123/job.jar` and
`https://10.0.0.1:8443/job.jar` are accepted too.
- With the prefix `https://10.0.0.1/repo/`,
`https://10.0.0.1/repo/../admin/x` is accepted. `java.net.URL` does not
normalise the path, so a path in the prefix does not restrict which paths on
that host can be fetched.
- An empty entry matches every URI. Flink produces one from `a;;b`.
A trailing `/` in the prefix stops the first two cases, but nothing in the
docs asks for one. If an exemption goes into the consolidated work, could it
parse each configured entry and compare the scheme, host (ignoring case) and
port exactly, reject a jarURI that has userinfo, and match the normalised path
on a `/` boundary? It should also reject blank entries, and each of these cases
needs a negative 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
