weiqingy commented on code in PR #1005:
URL: https://github.com/apache/flink-agents/pull/1005#discussion_r3837507296
##########
runtime/src/main/java/org/apache/flink/agents/runtime/skill/repository/SkillMaterializer.java:
##########
@@ -254,14 +255,47 @@ private static void copyJarEntries(URL jarUrl, String
prefix, Path extractDir)
* @throws IOException on connect / read failures or HTTP error responses.
*/
public static Path downloadToTempFile(String url, int timeoutMs) throws
IOException {
+ return downloadToTempFile(url, timeoutMs, false);
+ }
+
+ /**
+ * Download {@code url}, optionally permitting plain HTTP transport.
+ *
+ * @throws IOException on connect / read failures or HTTP error responses.
+ */
+ public static Path downloadToTempFile(String url, int timeoutMs, boolean
allowInsecureHttp)
+ throws IOException {
URL u = new URL(url);
+ String initialProtocol = u.getProtocol();
+ if (!("https".equalsIgnoreCase(initialProtocol)
+ || (allowInsecureHttp &&
"http".equalsIgnoreCase(initialProtocol)))) {
+ throw new IOException("Skill URL uses a disallowed transport: " +
url);
+ }
HttpURLConnection conn = (HttpURLConnection) u.openConnection();
conn.setConnectTimeout(timeoutMs);
conn.setReadTimeout(timeoutMs);
conn.setRequestMethod("GET");
+ // HttpURLConnection follows same-protocol redirects but leaves
cross-protocol redirects
+ // unfollowed. Any future HTTP client must preserve that restriction.
+ conn.setInstanceFollowRedirects(true);
Review Comment:
The comment is accurate, the JDK really does decline a scheme-changing
redirect: `followRedirect()` compares the two protocols and bails before taking
it.
The call itself isn't quite a no-op though. `instanceFollowRedirects` is
initialised from the static `followRedirects`, so if anything in the JVM has
called `HttpURLConnection.setFollowRedirects(false)`, which some deployments do
as a hardening step, this line overrides that and re-enables redirect following
for skill downloads specifically. I checked on temurin-11: after
`setFollowRedirects(false)` the instance default reads back `false`, and this
line flips it to `true`. Without it the 3xx would surface and `:284` would fail
closed with a clear message.
Was pinning it to `true` deliberate, or is the comment doing the real work
here?
##########
api/src/test/java/org/apache/flink/agents/api/yaml/YamlLoaderBuildAgentsTest.java:
##########
@@ -133,6 +133,34 @@ void skillsPerAgentAndShared() {
new SkillSourceSpec("local", Map.of("path",
"./more")));
}
+ @Test
+ void rejectsPlainHttpSkillUrlDuringLoading(@TempDir Path tmp) throws
Exception {
Review Comment:
nit: this covers the `urls:` list, and the new four-way test in
`YamlLoaderBuildersTest` covers the `url_sources` shapes that succeed. The case
#1003 is actually about, `url_sources: [{url: http://…}]` with
`allow_insecure_http` absent, isn't asserted on either side, and the issue asks
for "focused Java and Python tests for HTTP rejection or opt-in behavior".
The positive cases do pin the factory choice indirectly, so this is coverage
rather than a hole. Worth one line per language?
##########
python/flink_agents/api/skills.py:
##########
@@ -107,22 +109,96 @@ def from_local_dir(cls, *paths: str) -> Skills:
a zip, its top-level entries are the skill subdirectories.
"""
return cls(
- sources=[
- SkillSourceSpec(scheme="local", params={"path": p}) for p in
paths
- ]
+ sources=[SkillSourceSpec(scheme="local", params={"path": p}) for p
in paths]
)
@classmethod
def from_url(cls, *urls: str) -> Skills:
- """Create a Skills resource from one or more http(s) URLs.
+ """Create a Skills resource from one or more HTTPS URLs.
Each URL must point to a ``.zip`` whose top level is the baseDir
(i.e. skill subdirectories sit at the top of the zip).
"""
+ for url in urls:
+ cls._require_url(url, allow_insecure_http=False)
return cls(
sources=[SkillSourceSpec(scheme="url", params={"url": u}) for u in
urls]
)
+ @classmethod
+ def from_url_with_sha256(cls, url: str, sha256: str) -> Skills:
+ """Create an HTTPS URL source pinned to a SHA-256 archive digest."""
+ cls._require_url(url, allow_insecure_http=False)
+ cls._require_sha256(sha256)
+ return cls(
+ sources=[
+ SkillSourceSpec(scheme="url", params={"url": url, "sha256":
sha256})
+ ]
+ )
+
+ @classmethod
+ def from_url_unsafe(cls, *urls: str) -> Skills:
+ """Create URL sources that explicitly permit plain HTTP transport.
+
+ This compatibility escape hatch should be used only on trusted
networks.
+ Prefer :meth:`from_url` with HTTPS.
+ """
+ for url in urls:
+ cls._require_url(url, allow_insecure_http=True)
+ return cls(
+ sources=[
+ SkillSourceSpec(
+ scheme="url",
+ params={"url": url, "allow_insecure_http": "true"},
+ )
+ for url in urls
+ ]
+ )
+
+ @classmethod
+ def from_url_unsafe_with_sha256(cls, url: str, sha256: str) -> Skills:
+ """Create a digest-pinned source that explicitly permits plain HTTP."""
+ cls._require_url(url, allow_insecure_http=True)
+ cls._require_sha256(sha256)
+ return cls(
+ sources=[
+ SkillSourceSpec(
+ scheme="url",
+ params={
+ "url": url,
+ "sha256": sha256,
+ "allow_insecure_http": "true",
+ },
+ )
+ ]
+ )
+
+ @staticmethod
+ def _require_url(url: str, *, allow_insecure_http: bool) -> None:
+ if not isinstance(url, str):
+ msg = "skill URL must be a string"
+ raise TypeError(msg)
+ parsed = urlparse(url)
Review Comment:
`urlparse` doesn't reject a malformed URL, so `https://exa mple.com/x.zip`
is accepted here and only fails later at download time, while
`Skills.java:163-168` runs the same string through `URI.create` and rethrows it
as `Invalid skill URL:` at call time.
Before these commits neither YAML loader reached either validator. Now both
do, so the split is exercised at load time on both runtimes rather than only
through the code API.
Is the later failure good enough here, or would you rather Python rejected
it at the same point Java does?
##########
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}")) {
Review Comment:
nit: this accepts either case and `URLSkillRepository.java:90` lowercases
before comparing, which matches what `skills.md:144` promises ("lowercase or
uppercase SHA-256 digest"). Every digest literal in the suite is lowercase
though (`"a".repeat(64)` and friends), so nothing walks the uppercase path.
Would flipping one existing digest literal to uppercase cover it?
--
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]
