wenjin272 commented on code in PR #1005:
URL: https://github.com/apache/flink-agents/pull/1005#discussion_r3861263507
##########
api/src/main/java/org/apache/flink/agents/api/skills/Skills.java:
##########
@@ -92,17 +94,94 @@ public static Skills fromLocalDir(String... paths) {
}
/**
- * Create a {@link Skills} resource from one or more http(s) URLs.
+ * Create a {@link Skills} resource from one or more HTTPS URLs.
*
* <p>Each URL must point to a {@code .zip} whose top level is the baseDir.
*/
public static Skills fromUrl(String... urls) {
return new Skills(
Arrays.stream(urls)
- .map(u -> new SkillSourceSpec("url", Map.of("url", u)))
+ .map(
+ u -> {
+ requireUrl(u, false);
+ return new SkillSourceSpec("url",
Map.of("url", u));
+ })
.collect(Collectors.toList()));
}
+ /**
+ * Create a {@link Skills} resource from an HTTPS URL pinned to a SHA-256
digest.
+ *
+ * <p>The digest is verified against the downloaded archive before
extraction.
+ */
+ public static Skills fromUrlWithSha256(String url, String sha256) {
+ return urlSource(url, sha256, false);
+ }
+
+ /**
+ * Create a {@link Skills} resource that explicitly permits plain HTTP
transport.
+ *
+ * <p>This compatibility escape hatch should be used only on trusted
networks. Prefer {@link
+ * #fromUrl(String...)} with HTTPS.
+ */
+ public static Skills fromUrlUnsafe(String... urls) {
+ return new Skills(
+ Arrays.stream(urls)
+ .map(
+ u -> {
+ requireUrl(u, true);
+ return new SkillSourceSpec(
+ "url", Map.of("url", u,
"allow_insecure_http", "true"));
+ })
+ .collect(Collectors.toList()));
+ }
+
+ /**
+ * Create a digest-pinned {@link Skills} resource that explicitly permits
plain HTTP transport.
+ */
+ public static Skills fromUrlUnsafeWithSha256(String url, String sha256) {
+ return urlSource(url, sha256, true);
+ }
+
+ private static Skills urlSource(String url, String sha256, boolean
allowInsecureHttp) {
+ requireUrl(url, allowInsecureHttp);
+ if (sha256 == null || !sha256.matches("[0-9a-fA-F]{64}")) {
+ throw new IllegalArgumentException(
+ "sha256 must contain exactly 64 hexadecimal characters");
+ }
+ Map<String, String> params =
+ allowInsecureHttp
+ ? Map.of("url", url, "sha256", sha256,
"allow_insecure_http", "true")
+ : Map.of("url", url, "sha256", sha256);
+ return new Skills(List.of(new SkillSourceSpec("url", params)));
+ }
+
+ private static void requireUrl(String url, boolean allowInsecureHttp) {
+ if (url == null) {
+ throw new IllegalArgumentException("skill URL must not be null");
+ }
+ URI uri;
+ try {
+ uri = URI.create(url);
+ } catch (IllegalArgumentException e) {
+ throw new IllegalArgumentException("Invalid skill URL: " + url, e);
+ }
+ String scheme = uri.getScheme();
+ scheme = scheme == null ? "" : scheme.toLowerCase(Locale.ROOT);
+ if (!(scheme.equals("http") || scheme.equals("https"))) {
+ throw new IllegalArgumentException("Only HTTP(S) skill URLs are
supported: " + url);
+ }
+ if (scheme.equals("http") && !allowInsecureHttp) {
+ throw new IllegalArgumentException(
+ "Plain HTTP skill URLs are disabled by default; use HTTPS
or explicitly allow"
+ + " insecure HTTP for this source: "
+ + url);
+ }
+ if (uri.getRawAuthority() == null || uri.getRawAuthority().isEmpty()) {
Review Comment:
`getRawAuthority()` only checks that the authority string is non-empty, so
URLs like `https://:443/x.zip` and `https://example.com:bad/x.zip` are
accepted. Python's `parsed.netloc` check has the same issue. Could we validate
the actual hostname and port in both implementations?
##########
runtime/src/main/java/org/apache/flink/agents/runtime/skill/repository/URLSkillRepository.java:
##########
@@ -18,43 +18,119 @@
package org.apache.flink.agents.runtime.skill.repository;
+import javax.annotation.Nullable;
+
import java.io.IOException;
+import java.io.InputStream;
+import java.net.URI;
+import java.net.URISyntaxException;
import java.nio.file.Files;
import java.nio.file.Path;
+import java.security.MessageDigest;
+import java.security.NoSuchAlgorithmException;
+import java.util.Locale;
+import java.util.regex.Pattern;
/**
- * Skill repository backed by an http(s) URL pointing to a zip.
+ * Skill repository backed by an HTTPS URL pointing to a zip.
*
* <p>The zip is downloaded to a temp file and extracted into a process-local
temp directory. The
* downloaded zip itself is removed once extraction completes; the extracted
directory is released
* via {@link #close()} (cascaded through {@code SkillManager} → {@code
ResourceContextImpl} →
* {@code ResourceCache} on operator close). A JVM shutdown hook acts as
fallback cleanup if {@code
- * close()} is never called.
+ * close()} is never called. Plain HTTP is rejected unless the caller
explicitly opts in, and an
+ * optional SHA-256 digest is verified before extraction.
*/
public final class URLSkillRepository extends
AbstractMaterializedSkillRepository {
private static final int REQUEST_TIMEOUT_MS = 90_000;
+ private static final Pattern SHA256_PATTERN =
Pattern.compile("[0-9a-fA-F]{64}");
private final String url;
public URLSkillRepository(String url) throws IOException {
- super(materialize(url));
+ this(url, null, false);
+ }
+
+ public URLSkillRepository(String url, @Nullable String sha256, boolean
allowInsecureHttp)
+ throws IOException {
+ super(materialize(url, sha256, allowInsecureHttp));
this.url = url;
}
public String getUrl() {
return url;
}
- private static SkillMaterializer.Materialized materialize(String url)
throws IOException {
- if (!(url.startsWith("http://";) || url.startsWith("https://";))) {
- throw new IllegalArgumentException("Only http(s) URLs are
supported: " + url);
+ private static SkillMaterializer.Materialized materialize(
+ String url, @Nullable String sha256, boolean allowInsecureHttp)
throws IOException {
+ URI uri;
+ if (url == null) {
+ throw new IllegalArgumentException("skill URL must not be null");
}
- Path tmpZip = SkillMaterializer.downloadToTempFile(url,
REQUEST_TIMEOUT_MS);
try {
+ uri = new URI(url);
+ } catch (URISyntaxException e) {
+ throw new IllegalArgumentException("Invalid skill URL: " + url, e);
+ }
+ String scheme = uri.getScheme();
+ scheme = scheme == null ? "" : scheme.toLowerCase(Locale.ROOT);
+ if (!(scheme.equals("http") || scheme.equals("https"))) {
+ throw new IllegalArgumentException("Only HTTP(S) URLs are
supported: " + url);
+ }
+ if (scheme.equals("http") && !allowInsecureHttp) {
+ throw new IllegalArgumentException(
+ "Plain HTTP skill URLs are disabled by default; use HTTPS
or explicitly allow"
+ + " insecure HTTP for this source: "
+ + url);
+ }
+ if (uri.getRawAuthority() == null || uri.getRawAuthority().isEmpty()) {
+ throw new IllegalArgumentException("Skill URL must include a host:
" + url);
+ }
+ String normalizedSha256 = sha256 == null ? null :
sha256.toLowerCase(Locale.ROOT);
+ if (normalizedSha256 != null &&
!SHA256_PATTERN.matcher(normalizedSha256).matches()) {
+ throw new IllegalArgumentException(
+ "sha256 must contain exactly 64 hexadecimal characters");
+ }
+ Path tmpZip =
+ SkillMaterializer.downloadToTempFile(url, REQUEST_TIMEOUT_MS,
allowInsecureHttp);
+ try {
+ if (normalizedSha256 != null) {
+ String actual = sha256(tmpZip);
+ if (!actual.equals(normalizedSha256)) {
+ throw new IllegalArgumentException(
+ "SHA-256 mismatch for skill archive "
+ + url
Review Comment:
This error includes the full configured URL, so signed URLs may expose query
credentials through logs or exception reports. Python has the same behavior,
and both `SkillManager` implementations also include the raw `spec.params`.
Could we sanitize or omit URLs in these error paths?
--
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]
