Copilot commented on code in PR #11354:
URL: https://github.com/apache/gravitino/pull/11354#discussion_r3340049543
##########
core/src/main/java/org/apache/gravitino/job/JobManager.java:
##########
@@ -808,6 +809,7 @@ static String fetchFileFromUri(String uri, File stagingDir,
int timeoutInMs) {
case "http":
case "https":
case "ftp":
+ validateRemoteUri(fileUri);
FileUtils.copyURLToFile(fileUri.toURL(), destFile, timeoutInMs,
timeoutInMs);
break;
Review Comment:
`validateRemoteUri()` resolves the hostname, but
`FileUtils.copyURLToFile(fileUri.toURL(), ...)` will perform its own DNS
resolution again. That means DNS rebinding/TOCTOU can bypass this check if the
hostname’s A/AAAA records change between validation and connection. Consider
connecting using the validated `InetAddress` (or otherwise pinning the resolved
IP) to make the validation effective at request time.
##########
core/src/main/java/org/apache/gravitino/job/JobManager.java:
##########
@@ -830,6 +832,47 @@ static String fetchFileFromUri(String uri, File
stagingDir, int timeoutInMs) {
}
}
+ /**
+ * Resolves the host in the given URI and rejects addresses that should not
be reachable from the
+ * server (loopback, link-local, RFC-1918 private ranges, cloud metadata
endpoints). This is a
+ * defence-in-depth measure against Server-Side Request Forgery (SSRF).
+ */
+ @VisibleForTesting
+ static void validateRemoteUri(URI uri) throws IOException {
+ String host = uri.getHost();
+ if (host == null) {
+ throw new IllegalArgumentException("URI has no host: " + uri);
+ }
+ InetAddress[] addresses = InetAddress.getAllByName(host);
+ for (InetAddress address : addresses) {
+ if (isBlockedAddress(address)) {
+ throw new IllegalArgumentException(
+ String.format(
+ "URI '%s' resolves to blocked address %s, access denied (SSRF
prevention)",
+ uri, address.getHostAddress()));
+ }
+ }
+ }
+
+ private static boolean isBlockedAddress(InetAddress address) {
+ // Covers loopback (127.x.x.x / ::1), link-local (169.254.x.x / fe80::/10
— includes AWS/GCP/
+ // Azure metadata), RFC-1918 private (10.x / 172.16-31.x / 192.168.x),
multicast, unspecified.
+ if (address.isLoopbackAddress()
+ || address.isLinkLocalAddress()
+ || address.isSiteLocalAddress()
+ || address.isMulticastAddress()
+ || address.isAnyLocalAddress()) {
+ return true;
+ }
+ // Alibaba Cloud / Oracle Cloud metadata endpoint: 100.100.100.200
+ byte[] b = address.getAddress();
+ return b.length == 4
+ && (b[0] & 0xFF) == 100
+ && (b[1] & 0xFF) == 100
+ && (b[2] & 0xFF) == 100
+ && (b[3] & 0xFF) == 200;
Review Comment:
`isBlockedAddress()` relies on `isSiteLocalAddress()` for “private” ranges,
but that does not cover IPv6 Unique Local Addresses (RFC 4193, `fc00::/7`). As
a result, URIs resolving to `fc00::/7` can still pass validation, which
undermines the stated SSRF protection for private/internal networks.
##########
core/src/test/java/org/apache/gravitino/job/TestJobManager.java:
##########
@@ -914,6 +915,72 @@ public void
testFetchFileFromUriWithMissingLocalFileShouldFail() throws IOExcept
RuntimeException.class, () -> JobManager.fetchFileFromUri(uri,
stagingDir, 1000));
}
+ @Test
+ public void testFetchFileFromUriSsrfBlocked() {
+ File stagingDir = new File(testStagingDir);
+ Assertions.assertTrue(stagingDir.mkdirs() || stagingDir.exists());
+
+ // Loopback address
+ Assertions.assertThrows(
+ RuntimeException.class,
+ () -> JobManager.fetchFileFromUri("http://127.0.0.1:8090/configs";,
stagingDir, 1000));
+
+ // AWS / GCP / Azure cloud-metadata endpoint (link-local 169.254.x.x)
+ Assertions.assertThrows(
+ RuntimeException.class,
+ () ->
+ JobManager.fetchFileFromUri(
+ "http://169.254.169.254/latest/meta-data/";, stagingDir, 1000));
+
Review Comment:
Same as above: this should verify the failure is due to SSRF validation
(exception cause/message), not just that the fetch fails for any reason.
##########
core/src/test/java/org/apache/gravitino/job/TestJobManager.java:
##########
@@ -914,6 +915,72 @@ public void
testFetchFileFromUriWithMissingLocalFileShouldFail() throws IOExcept
RuntimeException.class, () -> JobManager.fetchFileFromUri(uri,
stagingDir, 1000));
}
+ @Test
+ public void testFetchFileFromUriSsrfBlocked() {
+ File stagingDir = new File(testStagingDir);
+ Assertions.assertTrue(stagingDir.mkdirs() || stagingDir.exists());
+
+ // Loopback address
+ Assertions.assertThrows(
+ RuntimeException.class,
+ () -> JobManager.fetchFileFromUri("http://127.0.0.1:8090/configs";,
stagingDir, 1000));
+
+ // AWS / GCP / Azure cloud-metadata endpoint (link-local 169.254.x.x)
+ Assertions.assertThrows(
+ RuntimeException.class,
+ () ->
+ JobManager.fetchFileFromUri(
+ "http://169.254.169.254/latest/meta-data/";, stagingDir, 1000));
+
+ // RFC-1918 private range
+ Assertions.assertThrows(
+ RuntimeException.class,
+ () -> JobManager.fetchFileFromUri("http://192.168.1.1/";, stagingDir,
1000));
+
+ // Alibaba Cloud / Oracle Cloud metadata endpoint
+ Assertions.assertThrows(
+ RuntimeException.class,
+ () -> JobManager.fetchFileFromUri("http://100.100.100.200/";,
stagingDir, 1000));
+ }
Review Comment:
Same issue: this should assert the fetch failed specifically due to SSRF
prevention (cause/message), not simply because the endpoint is unreachable.
##########
core/src/test/java/org/apache/gravitino/job/TestJobManager.java:
##########
@@ -914,6 +915,72 @@ public void
testFetchFileFromUriWithMissingLocalFileShouldFail() throws IOExcept
RuntimeException.class, () -> JobManager.fetchFileFromUri(uri,
stagingDir, 1000));
}
+ @Test
+ public void testFetchFileFromUriSsrfBlocked() {
+ File stagingDir = new File(testStagingDir);
+ Assertions.assertTrue(stagingDir.mkdirs() || stagingDir.exists());
+
+ // Loopback address
+ Assertions.assertThrows(
+ RuntimeException.class,
+ () -> JobManager.fetchFileFromUri("http://127.0.0.1:8090/configs";,
stagingDir, 1000));
+
Review Comment:
This assertion only checks that *some* `RuntimeException` is thrown. Without
inspecting the cause/message, the test could still pass if the HTTP request
fails for an unrelated reason (e.g., connection refused) and would not actually
prove SSRF blocking is enforced by `validateRemoteUri()`.
##########
core/src/test/java/org/apache/gravitino/job/TestJobManager.java:
##########
@@ -914,6 +915,72 @@ public void
testFetchFileFromUriWithMissingLocalFileShouldFail() throws IOExcept
RuntimeException.class, () -> JobManager.fetchFileFromUri(uri,
stagingDir, 1000));
}
+ @Test
+ public void testFetchFileFromUriSsrfBlocked() {
+ File stagingDir = new File(testStagingDir);
+ Assertions.assertTrue(stagingDir.mkdirs() || stagingDir.exists());
+
+ // Loopback address
+ Assertions.assertThrows(
+ RuntimeException.class,
+ () -> JobManager.fetchFileFromUri("http://127.0.0.1:8090/configs";,
stagingDir, 1000));
+
+ // AWS / GCP / Azure cloud-metadata endpoint (link-local 169.254.x.x)
+ Assertions.assertThrows(
+ RuntimeException.class,
+ () ->
+ JobManager.fetchFileFromUri(
+ "http://169.254.169.254/latest/meta-data/";, stagingDir, 1000));
+
+ // RFC-1918 private range
+ Assertions.assertThrows(
+ RuntimeException.class,
+ () -> JobManager.fetchFileFromUri("http://192.168.1.1/";, stagingDir,
1000));
Review Comment:
Same issue: this assertion should confirm the exception is thrown by the
SSRF validation (cause/message) to avoid a false positive if the network call
fails for unrelated reasons.
##########
core/src/test/java/org/apache/gravitino/job/TestJobManager.java:
##########
@@ -914,6 +915,72 @@ public void
testFetchFileFromUriWithMissingLocalFileShouldFail() throws IOExcept
RuntimeException.class, () -> JobManager.fetchFileFromUri(uri,
stagingDir, 1000));
}
+ @Test
+ public void testFetchFileFromUriSsrfBlocked() {
+ File stagingDir = new File(testStagingDir);
+ Assertions.assertTrue(stagingDir.mkdirs() || stagingDir.exists());
+
+ // Loopback address
+ Assertions.assertThrows(
+ RuntimeException.class,
+ () -> JobManager.fetchFileFromUri("http://127.0.0.1:8090/configs";,
stagingDir, 1000));
+
+ // AWS / GCP / Azure cloud-metadata endpoint (link-local 169.254.x.x)
+ Assertions.assertThrows(
+ RuntimeException.class,
+ () ->
+ JobManager.fetchFileFromUri(
+ "http://169.254.169.254/latest/meta-data/";, stagingDir, 1000));
+
+ // RFC-1918 private range
+ Assertions.assertThrows(
+ RuntimeException.class,
+ () -> JobManager.fetchFileFromUri("http://192.168.1.1/";, stagingDir,
1000));
+
+ // Alibaba Cloud / Oracle Cloud metadata endpoint
+ Assertions.assertThrows(
+ RuntimeException.class,
+ () -> JobManager.fetchFileFromUri("http://100.100.100.200/";,
stagingDir, 1000));
+ }
+
+ @Test
+ public void testValidateRemoteUri() throws Exception {
+ // Loopback
+ Assertions.assertThrows(
+ IllegalArgumentException.class,
+ () -> JobManager.validateRemoteUri(new URI("http://127.0.0.1/";)));
+
+ // Link-local (cloud metadata)
+ Assertions.assertThrows(
+ IllegalArgumentException.class,
+ () -> JobManager.validateRemoteUri(new
URI("http://169.254.169.254/";)));
+
+ // RFC-1918 private 10.x.x.x
+ Assertions.assertThrows(
+ IllegalArgumentException.class,
+ () -> JobManager.validateRemoteUri(new URI("http://10.0.0.1/";)));
+
+ // RFC-1918 private 172.16.x.x
+ Assertions.assertThrows(
+ IllegalArgumentException.class,
+ () -> JobManager.validateRemoteUri(new URI("http://172.16.0.1/";)));
+
+ // RFC-1918 private 192.168.x.x
+ Assertions.assertThrows(
+ IllegalArgumentException.class,
+ () -> JobManager.validateRemoteUri(new URI("http://192.168.0.1/";)));
+
Review Comment:
`validateRemoteUri()` is intended to block private/internal addresses, but
the test suite doesn’t currently cover IPv6 Unique Local Addresses
(`fc00::/7`). Adding a ULA case helps prevent regressions once IPv6
private-range blocking is implemented.
--
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]
