henrib commented on code in PR #6812:
URL: https://github.com/apache/hive/pull/6812#discussion_r4124466509
##########
iceberg/iceberg-catalog/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -241,6 +241,10 @@ public String name() {
return name;
}
+ public FileIO io() {
Review Comment:
Removed — `io()` is gone entirely now that `validateRegisterTable` no longer
needs it (see below).
##########
iceberg/iceberg-catalog/src/main/java/org/apache/iceberg/hive/ScopedDeleteFileIO.java:
##########
@@ -0,0 +1,91 @@
+/*
+ * 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.iceberg.hive;
+
+import java.util.Map;
+import org.apache.hadoop.fs.Path;
+import org.apache.iceberg.io.FileIO;
+import org.apache.iceberg.io.InputFile;
+import org.apache.iceberg.io.OutputFile;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A {@link FileIO} decorator used by {@link
HiveCatalog#dropTable(org.apache.iceberg.catalog.TableIdentifier,
+ * boolean)} to fence purge deletions to files under a table's own location,
regardless of what a table's
+ * metadata or manifests actually reference.
+ *
+ * <p>This does not implement {@link
org.apache.iceberg.io.SupportsBulkOperations} or
+ * {@link org.apache.iceberg.io.SupportsPrefixOperations} even when the
delegate does, so that
+ * {@code CatalogUtil.dropTableData} is forced to route every deletion through
{@link #deleteFile(String)}.
+ */
+class ScopedDeleteFileIO implements FileIO {
+ private static final Logger LOG =
LoggerFactory.getLogger(ScopedDeleteFileIO.class);
+
+ private final FileIO delegate;
+ private final String location;
+
+ ScopedDeleteFileIO(FileIO delegate, String location) {
+ this.delegate = delegate;
+ this.location = normalize(location);
+ }
+
+ @Override
+ public InputFile newInputFile(String path) {
+ return delegate.newInputFile(path);
+ }
+
+ @Override
+ public OutputFile newOutputFile(String path) {
+ return delegate.newOutputFile(path);
+ }
+
+ @Override
+ public void deleteFile(String path) {
+ if (!isContained(location, normalize(path))) {
+ LOG.warn("Skipping delete outside table location {}: {}", location,
path);
+ return;
+ }
Review Comment:
Downgraded to DEBUG.
##########
iceberg/iceberg-catalog/src/main/java/org/apache/iceberg/hive/ScopedDeleteFileIO.java:
##########
@@ -0,0 +1,91 @@
+/*
+ * 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.iceberg.hive;
+
+import java.util.Map;
+import org.apache.hadoop.fs.Path;
+import org.apache.iceberg.io.FileIO;
+import org.apache.iceberg.io.InputFile;
+import org.apache.iceberg.io.OutputFile;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A {@link FileIO} decorator used by {@link
HiveCatalog#dropTable(org.apache.iceberg.catalog.TableIdentifier,
+ * boolean)} to fence purge deletions to files under a table's own location,
regardless of what a table's
+ * metadata or manifests actually reference.
+ *
+ * <p>This does not implement {@link
org.apache.iceberg.io.SupportsBulkOperations} or
+ * {@link org.apache.iceberg.io.SupportsPrefixOperations} even when the
delegate does, so that
+ * {@code CatalogUtil.dropTableData} is forced to route every deletion through
{@link #deleteFile(String)}.
+ */
+class ScopedDeleteFileIO implements FileIO {
+ private static final Logger LOG =
LoggerFactory.getLogger(ScopedDeleteFileIO.class);
+
+ private final FileIO delegate;
+ private final String location;
+
+ ScopedDeleteFileIO(FileIO delegate, String location) {
+ this.delegate = delegate;
+ this.location = normalize(location);
+ }
+
+ @Override
+ public InputFile newInputFile(String path) {
+ return delegate.newInputFile(path);
+ }
+
+ @Override
+ public OutputFile newOutputFile(String path) {
+ return delegate.newOutputFile(path);
+ }
+
+ @Override
+ public void deleteFile(String path) {
+ if (!isContained(location, normalize(path))) {
+ LOG.warn("Skipping delete outside table location {}: {}", location,
path);
+ return;
+ }
+ delegate.deleteFile(path);
+ }
+
+ @Override
+ public Map<String, String> properties() {
+ return delegate.properties();
+ }
+
+ @Override
+ public void initialize(Map<String, String> properties) {
+ delegate.initialize(properties);
+ }
+
+ @Override
+ public void close() {
+ delegate.close();
+ }
+
+ private static boolean isContained(String root, String candidate) {
+ return candidate.equals(root) || candidate.startsWith(root.endsWith("/") ?
root : root + "/");
+ }
+
+ private static String normalize(String location) {
+ return new Path(location).toUri().normalize().toString();
+ }
Review Comment:
Extracted to `FileUtils.isPathWithinSubtree` in `metastore-common`, shared
by both.
##########
standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCatalogAdapter.java:
##########
@@ -320,6 +325,10 @@ private LoadTableResponse loadTable(Map<String, String>
vars) {
private LoadTableResponse registerTable(Map<String, String> vars, Object
body) {
Namespace namespace = namespaceFromPathVars(vars);
RegisterTableRequest request = castRequest(RegisterTableRequest.class,
body);
+ request.validate();
+ Map<String, String> namespaceMetadata =
asNamespaceCatalog.loadNamespaceMetadata(namespace);
+ FileIO io = ((HiveCatalog) catalog).io();
+ icebergAuthorizer.validateRegisterTable(catalogName, namespace,
namespaceMetadata, request, io);
return castResponse(LoadTableResponse.class,
CatalogHandlers.registerTable(catalog, namespace, request));
Review Comment:
No longer casts to `HiveCatalog` at all — the `io`-based check it needed is
removed.
##########
standalone-metastore/metastore-rest-catalog/src/test/java/org/apache/iceberg/rest/BaseRESTCatalogTests.java:
##########
@@ -277,4 +284,56 @@ void testStageCreateTableWithDeniedLocation() {
Assertions.assertThrows(ForbiddenException.class,
builder::createTransaction);
Assertions.assertThrows(NoSuchTableException.class, () ->
catalog.loadTable(tableIdentifier));
}
+
+ private static String writeMetadataFile(String directory, String
tableLocation) throws IOException {
+ var metadataLocation = directory + "/v1.metadata.json";
+ Files.deleteIfExists(java.nio.file.Path.of(metadataLocation));
+ var io = new HadoopFileIO(new Configuration(false));
+ var metadata = TableMetadata.newTableMetadata(new Schema(),
PartitionSpec.unpartitioned(), tableLocation,
+ Collections.emptyMap());
+ TableMetadataParser.write(metadata, io.newOutputFile(metadataLocation));
+ return metadataLocation;
+ }
Review Comment:
Leaving as-is: every location in this suite is rooted under the local test
warehouse dir, so this isn't reachable in practice.
##########
standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/IcebergAuthorizer.java:
##########
@@ -161,4 +175,133 @@ void validateStageCreateTable(String catalogName,
Namespace namespace, Map<Strin
throw new IllegalStateException("Failed to check privileges
stage-create", e);
}
}
+
+ /**
+ * Enforces authorization for REGISTER_TABLE. The request's {@code
metadataLocation} is fetched with the
+ * catalog's shared, service-level {@link FileIO}, so both that location and
the {@code location()} embedded in
+ * the metadata file it points to (which becomes the table's HMS {@code
StorageDescriptor.location}, and is what
+ * a later purge trusts as its deletion root, see {@link
#validateDropTablePurge}) must be authorized. Otherwise
+ * REGISTER_TABLE is an arbitrary-file-read primitive that returns any
metadata file's contents to the caller.
+ *
+ * <p>When no {@code HiveAuthorizer} is configured, falls back to requiring
both locations to be contained in
+ * the namespace's external or managed root, since there is no policy to
otherwise decide whether the caller may
+ * read an arbitrary location with service credentials.
+ *
+ * @param catalogName the Hive catalog name
+ * @param namespace the Iceberg namespace
+ * @param namespaceMetadata the Iceberg namespace metadata
+ * @param request the register table request
+ * @param io the {@link FileIO} used to read the metadata file
+ * @throws ForbiddenException if a location is not authorized, or not
contained in the namespace
+ * @throws IllegalStateException if the authorization plugin fails
+ */
+ void validateRegisterTable(String catalogName, Namespace namespace,
Map<String, String> namespaceMetadata,
+ RegisterTableRequest request, FileIO io) {
+ Preconditions.checkArgument(namespace.levels().length == 1, "Hive does not
support multi-level namespaces");
+ var databaseName = namespace.level(0);
+ var commandString = "register table " + request.name();
+ checkLocationAuthorized(catalogName, databaseName, namespaceMetadata,
request.metadataLocation(), commandString);
+
+ var metadata = TableMetadataParser.read(io, request.metadataLocation());
+ checkLocationAuthorized(catalogName, databaseName, namespaceMetadata,
metadata.location(), commandString);
+ }
+
+ /**
+ * Enforces authorization for DROP_TABLE with {@code purge=true}. Purge
deletes every file referenced by the
+ * table's current metadata using the catalog's shared, service-level {@link
FileIO}, so the location must be
+ * authorized like any other DFS_URI access.
Review Comment:
Agreed, keeping the check (per HIVE-28804).
##########
standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCatalogAdapter.java:
##########
@@ -320,6 +325,10 @@ private LoadTableResponse loadTable(Map<String, String>
vars) {
private LoadTableResponse registerTable(Map<String, String> vars, Object
body) {
Namespace namespace = namespaceFromPathVars(vars);
RegisterTableRequest request = castRequest(RegisterTableRequest.class,
body);
+ request.validate();
+ Map<String, String> namespaceMetadata =
asNamespaceCatalog.loadNamespaceMetadata(namespace);
+ FileIO io = ((HiveCatalog) catalog).io();
Review Comment:
Agreed — removed.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
