Copilot commented on code in PR #11354: URL: https://github.com/apache/gravitino/pull/11354#discussion_r3392202184
########## common/src/main/java/org/apache/gravitino/utils/FetchFileUtils.java: ########## @@ -0,0 +1,184 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.gravitino.utils; + +import java.io.File; +import java.io.IOException; +import java.net.URI; +import java.net.URISyntaxException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.StandardCopyOption; +import java.util.Optional; +import java.util.concurrent.ConcurrentHashMap; +import javax.annotation.Nullable; +import org.apache.commons.io.FileUtils; + +/** + * Fetches a file referenced by a URI to a local destination. Supports {@code file}, {@code http}, + * {@code https}, {@code ftp} and {@code hdfs} schemes. This is the single shared implementation + * used by the job manager and the Kerberos clients of the Hive, Iceberg, Hadoop and Paimon + * catalogs. + * + * <p>The {@code hdfs} scheme is resolved reflectively against {@code + * org.apache.hadoop.fs.FileSystem} so that this class can live in the {@code common} module, which + * does not declare a compile-time dependency on Hadoop. Callers that never use {@code hdfs} URIs + * (for example the job manager and the Paimon catalog) simply pass {@code null} for the Hadoop + * configuration. + */ +public final class FetchFileUtils { + + /** The server configuration that controls unsafe remote URI blocking. */ + public static final String BLOCK_UNSAFE_REMOTE_URI_CONFIG = "gravitino.blockUnsafeRemoteUri"; + + /** + * Per-destination lock map used to serialize concurrent symlink creation for the same destination + * file. Keyed by the normalized absolute destination path string to avoid races caused by + * different path spellings referring to the same file. Entries should be removed via {@link + * #removeLock(File)} when the destination file is deleted, so the map size stays bounded by the + * number of live consumers. + */ + private static final ConcurrentHashMap<String, Object> SYMLINK_LOCKS = new ConcurrentHashMap<>(); + + private static volatile boolean blockUnsafeRemoteUri = true; + + private FetchFileUtils() {} + + /** + * Sets whether remote URIs that resolve to unsafe addresses should be blocked. + * + * @param blockUnsafeRemoteUri whether to block unsafe remote URIs + */ + public static void setBlockUnsafeRemoteUri(boolean blockUnsafeRemoteUri) { + FetchFileUtils.blockUnsafeRemoteUri = blockUnsafeRemoteUri; + } + + /** + * Removes the per-destination lock entry for the given file. Should be called when the + * destination file is deleted (for example on a Kerberos client {@code close()}) to prevent + * unbounded map growth. + * + * @param destFile the destination file whose lock entry should be removed + */ + public static void removeLock(File destFile) { + SYMLINK_LOCKS.remove(destFile.toPath().toAbsolutePath().normalize().toString()); + } + + /** + * Fetches the file referenced by {@code fileUri} into {@code destFile}. + * + * @param fileUri the source URI; a missing scheme is treated as {@code file} + * @param destFile the local destination file + * @param timeoutMs the connect/read timeout in milliseconds, applied to remote (http/https/ftp) + * downloads + * @param hadoopConf an {@code org.apache.hadoop.conf.Configuration} instance, required only for + * the {@code hdfs} scheme; may be {@code null} when no {@code hdfs} URI is fetched + * @return the absolute path of {@code destFile} + * @throws IOException if the file cannot be fetched + */ + public static String fetchFileFromUri( + String fileUri, File destFile, int timeoutMs, @Nullable Object hadoopConf) + throws IOException { + try { + URI uri = new URI(fileUri); + String scheme = Optional.ofNullable(uri.getScheme()).orElse("file"); + + switch (scheme) { + case "http": + case "https": + case "ftp": + RemoteUriValidator.validate( + uri, + blockUnsafeRemoteUri, + String.format("'%s' to false", BLOCK_UNSAFE_REMOTE_URI_CONFIG)); + FileUtils.copyURLToFile(uri.toURL(), destFile, timeoutMs, timeoutMs); Review Comment: Remote URI validation is performed using DNS resolution in `RemoteUriValidator.validate(...)`, but the actual download uses `FileUtils.copyURLToFile(uri.toURL(), ...)`, which will resolve the hostname again at connect time. This leaves a DNS rebinding/TOCTOU gap where the validated hostname can later resolve to a blocked address. ########## common/src/main/java/org/apache/gravitino/utils/RemoteUriValidator.java: ########## @@ -0,0 +1,92 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.gravitino.utils; + +import java.io.IOException; +import java.net.InetAddress; +import java.net.URI; + +/** Validates remote URI hosts before server-side downloads. */ +public final class RemoteUriValidator { + + private RemoteUriValidator() {} + + /** + * Resolves the host in the given URI and rejects unsafe addresses when blocking is enabled. + * + * @param uri The remote URI to validate. + * @param blockUnsafeAddress Whether unsafe addresses should be blocked. + * @param blockUnsafeAddressHint The configuration hint that disables unsafe address blocking. + * @throws IOException If host resolution fails. + * @throws IllegalArgumentException If the URI has no host or resolves to an unsafe address. + */ + public static void validate(URI uri, boolean blockUnsafeAddress, String blockUnsafeAddressHint) + throws IOException { + String host = uri.getHost(); + if (host == null) { + throw new IllegalArgumentException("URI has no host: " + uri); + } + + if (!blockUnsafeAddress) { + return; + } + + InetAddress[] addresses = InetAddress.getAllByName(host); + for (InetAddress address : addresses) { + if (isUnsafeAddress(address)) { + throw new IllegalArgumentException( + String.format( + "URI '%s' resolves to blocked address %s from the Gravitino server side. " + + "Access to local, private, link-local, multicast, unspecified, and cloud " + + "metadata addresses is disabled by default to prevent SSRF. If this URI is " + + "trusted and this access is required, set %s.", + uri, address.getHostAddress(), blockUnsafeAddressHint)); + } + } + } + + private static boolean isUnsafeAddress(InetAddress address) { + if (address.isLoopbackAddress() + || address.isLinkLocalAddress() + || address.isSiteLocalAddress() + || address.isMulticastAddress() + || address.isAnyLocalAddress()) { + return true; + } + + byte[] bytes = address.getAddress(); + if (isCloudMetadataAddress(bytes)) { + return true; + } + + return isIpv6UniqueLocalAddress(bytes); + } Review Comment: `isUnsafeAddress` does not account for IPv4-mapped / IPv4-compatible IPv6 literals (e.g. `http://[::ffff:127.0.0.1]/`). For such addresses, `InetAddress` loopback/site-local checks may not trigger, allowing localhost/private ranges to bypass SSRF blocking. ########## docs/apache-hive-catalog.md: ########## @@ -29,19 +29,19 @@ The Hive catalog supports creating, updating, and deleting databases and tables Besides the [common catalog properties](./gravitino-server-config.md#catalog-properties-configuration), the Hive catalog has the following properties: -| Property Name | Description | Default Value | Required | Since Version | -|------------------------------------------|-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|---------------|------------------------------|---------------| -| `metastore.uris` | The Hive metastore service URIs, separate multiple addresses with commas. Such as `thrift://127.0.0.1:9083` | (none) | Yes | 0.2.0 | -| `client.pool-size` | The maximum number of Hive metastore clients in the pool for Gravitino. | 1 | No | 0.2.0 | -| `gravitino.bypass.` | Property name with this prefix passed down to the underlying HMS client for use. Such as `gravitino.bypass.hive.metastore.failure.retries = 3` indicate 3 times of retries upon failure of Thrift metastore calls | (none) | No | 0.2.0 | -| `client.pool-cache.eviction-interval-ms` | The cache pool eviction interval. | 300000 | No | 0.4.0 | -| `impersonation-enable` | Enable user impersonation for Hive catalog. | false | No | 0.4.0 | -| `kerberos.principal` | The Kerberos principal for the catalog. You should configure `gravitino.bypass.hadoop.security.authentication`, `gravitino.bypass.hive.metastore.kerberos.principal` and `gravitino.bypass.hive.metastore.sasl.enabled`if you want to use Kerberos. | (none) | required if you use kerberos | 0.4.0 | -| `kerberos.keytab-uri` | The uri of key tab for the catalog. Now supported protocols are `https`, `http`, `ftp`, `file`. | (none) | required if you use kerberos | 0.4.0 | -| `kerberos.check-interval-sec` | The interval to check validness of the principal | 60 | No | 0.4.0 | -| `kerberos.keytab-fetch-timeout-sec` | The timeout to fetch key tab | 60 | No | 0.4.0 | -| `list-all-tables` | Whether to list all tables in a database, including non-Hive tables such as Iceberg, Paimon, and Hudi. When false, non-Hive tables are filtered out on a best-effort basis; see the note below for known limitations. | false | No | 0.5.1 | -| `default.catalog` | The default catalog name for the Hive3 metastore backend; this configuration is ignored when using a Hive2 metastore. | hive | No | 1.1.0 | +| Property Name | Description | Default Value | Required | Since Version | +|------------------------------------------|-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|----------------|------------------------------|---------------| +| `metastore.uris` | The Hive metastore service URIs, separate multiple addresses with commas. Such as `thrift://127.0.0.1:9083` | (none) | Yes | 0.2.0 | +| `client.pool-size` | The maximum number of Hive metastore clients in the pool for Gravitino. | 1 | No | 0.2.0 | +| `gravitino.bypass.` | Property name with this prefix passed down to the underlying HMS client for use. Such as `gravitino.bypass.hive.metastore.failure.retries = 3` indicate 3 times of retries upon failure of Thrift metastore calls | (none) | No | 0.2.0 | +| `client.pool-cache.eviction-interval-ms` | The cache pool eviction interval. | 300000 | No | 0.4.0 | +| `impersonation-enable` | Enable user impersonation for Hive catalog. | false | No | 0.4.0 | +| `kerberos.principal` | The Kerberos principal for the catalog. You should configure `gravitino.bypass.hadoop.security.authentication`, `gravitino.bypass.hive.metastore.kerberos.principal` and `gravitino.bypass.hive.metastore.sasl.enabled`if you want to use Kerberos. | (none) | required if you use kerberos | 0.4.0 | +| `kerberos.keytab-uri` | The uri of key tab for the catalog. Now supported protocols are `https`, `http`, `ftp`, `file`. | (none) | required if you use kerberos | 0.4.0 | +| `kerberos.check-interval-sec` | The interval to check validness of the principal | 60 | No | 0.4.0 | Review Comment: Wording/grammar: "validness" is nonstandard; prefer "validity" (and consider capitalizing "URI" / using "keytab" consistently). ########## docs/iceberg-rest-service.md: ########## @@ -394,15 +396,15 @@ Refer to [HTTPS Configuration](./security/how-to-use-https.md#apache-iceberg-res For JDBC backend, you can use the `gravitino.iceberg-rest.jdbc-user` and `gravitino.iceberg-rest.jdbc-password` to authenticate the JDBC connection. For Hive backend, you can use the `gravitino.iceberg-rest.authentication.type` to specify the authentication type, and use the `gravitino.iceberg-rest.authentication.kerberos.principal` and `gravitino.iceberg-rest.authentication.kerberos.keytab-uri` to authenticate the Kerberos connection. The detailed configuration items are as follows: -| Configuration item | Description | Default value | Required | Since Version | -|---------------------------------------------------------------------------|--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|---------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------|------------------| -| `gravitino.iceberg-rest.authentication.type` | The type of authentication for Iceberg rest catalog backend. This configuration only applicable for for Hive backend, and only supports `Kerberos`, `simple` currently. As for JDBC backend, only username/password authentication was supported now. | `simple` | No | 0.7.0-incubating | -| `gravitino.iceberg-rest.authentication.impersonation-enable` | Whether to enable impersonation for the Iceberg catalog | `false` | No | 0.7.0-incubating | -| `gravitino.iceberg-rest.hive.metastore.sasl.enabled` | Whether to enable SASL authentication protocol when connect to Kerberos Hive metastore. | `false` | No, This value should be true in most case(Some will use SSL protocol, but it rather rare) if the value of `gravitino.iceberg-rest.authentication.type` is Kerberos. | 0.7.0-incubating | -| `gravitino.iceberg-rest.authentication.kerberos.principal` | The principal of the Kerberos authentication | (none) | required if the value of `gravitino.iceberg-rest.authentication.type` is Kerberos. | 0.7.0-incubating | -| `gravitino.iceberg-rest.authentication.kerberos.keytab-uri` | The URI of The keytab for the Kerberos authentication. | (none) | required if the value of `gravitino.iceberg-rest.authentication.type` is Kerberos. | 0.7.0-incubating | -| `gravitino.iceberg-rest.authentication.kerberos.check-interval-sec` | The check interval of Kerberos credential for Iceberg catalog. | 60 | No | 0.7.0-incubating | -| `gravitino.iceberg-rest.authentication.kerberos.keytab-fetch-timeout-sec` | The fetch timeout of retrieving Kerberos keytab from `authentication.kerberos.keytab-uri`. | 60 | No | 0.7.0-incubating | +| Configuration item | Description | Default value | Required | Since Version | +|---------------------------------------------------------------------------|-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|---------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------|------------------| +| `gravitino.iceberg-rest.authentication.type` | The type of authentication for Iceberg rest catalog backend. This configuration only applicable for for Hive backend, and only supports `Kerberos`, `simple` currently. As for JDBC backend, only username/password authentication was supported now. | `simple` | No | 0.7.0-incubating | Review Comment: Grammar: "only applicable for for Hive backend" has a duplicated "for". ########## docs/lakehouse-hudi-catalog.md: ########## @@ -44,14 +44,14 @@ Tested and verified with Apache Hudi `0.15.0`. Users can use the following properties to configure the security of the catalog backend if needed. For example, if you are using a Kerberos Hive catalog backend, you must set `authentication.type` to `Kerberos` and provide `authentication.kerberos.principal` and `authentication.kerberos.keytab-uri`. -| Property name | Description | Default value | Required | Since Version | -|----------------------------------------------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------|---------------|-------------------------------------------------------------|-------------------| -| `authentication.type` | The type of authentication for hudi catalog backend. This configuration only applicable for for hms backend, and only supports `kerberos`, `simple` currently. | `simple` | No | 1.0.0 | -| `authentication.impersonation-enable` | Whether to enable impersonation for the hudi catalog | `false` | No | 1.0.0 | -| `authentication.kerberos.principal` | The principal of the Kerberos authentication | (none) | required if the value of `authentication.type` is kerberos. | 1.0.0 | -| `authentication.kerberos.keytab-uri` | The URI of The keytab for the Kerberos authentication. | (none) | required if the value of `authentication.type` is kerberos. | 1.0.0 | -| `authentication.kerberos.check-interval-sec` | The check interval of Kerberos credential for hudi catalog. | 60 | No | 1.0.0 | -| `authentication.kerberos.keytab-fetch-timeout-sec` | The fetch timeout of retrieving Kerberos keytab from `authentication.kerberos.keytab-uri`. | 60 | No | 1.0.0 | +| Property name | Description | Default value | Required | Since Version | +|----------------------------------------------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------|----------------|--------------------------------------------------------------|----------------| +| `authentication.type` | The type of authentication for hudi catalog backend. This configuration only applicable for for hms backend, and only supports `kerberos`, `simple` currently. | `simple` | No | 1.0.0 | Review Comment: Grammar: "only applicable for for hms backend" has a duplicated "for". -- 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]
