saihemanth-cloudera commented on code in PR #6884:
URL: https://github.com/apache/hive/pull/6884#discussion_r4332109240
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/handler/TransactionHandler.java:
##########
@@ -505,9 +522,108 @@ public ShowCompactResponse
show_compact(ShowCompactRequest rqst) throws TExcepti
@Override
public boolean submit_for_cleanup(CompactionRequest rqst, long
highestWriteId, long txnId)
throws TException {
+ try {
+ authorizeAndPrepareCompactionRequest(rqst, true);
+ } catch (Exception e) {
+ rethrowException(e);
+ }
return getTxnHandler().submitForCleanup(rqst, highestWriteId, txnId);
}
+ /**
+ * Authorize client-initiated compaction/cleanup requests and derive
server-side run-as identity.
+ * Internal callers (e.g. {@link
org.apache.hadoop.hive.metastore.AcidEventListener}) invoke
+ * {@code TxnHandler} directly and are not subject to this path.
+ *
+ * @param allowMissingTable only for cleanup requests: a rolled-back CTAS
(see {@code HiveQueryLifeTimeHook})
+ * asks for cleanup of a directory whose table was never created or has
already been dropped. In that case
+ * the request is authorized against the database and the requested
location must be strictly inside the
+ * database's managed/external location.
+ */
+ private void authorizeAndPrepareCompactionRequest(CompactionRequest rqst,
boolean allowMissingTable)
+ throws TException, IOException, InterruptedException {
+ if (rqst.getDbname() == null || rqst.getTablename() == null) {
+ throw new MetaException("The DB and table name cannot be null.");
+ }
+ String[] parsedDbName = parseDbName(rqst.getDbname(), conf);
+ String catName = parsedDbName[CAT_NAME];
+ String dbName = parsedDbName[DB_NAME];
+ String tblName = rqst.getTablename();
+ rqst.setDbname(dbName);
+
+ FilterUtils.checkDbAndTableFilters(isServerFilterEnabled, filterHook,
catName, dbName, tblName);
+
+ GetTableRequest getTableRequest = new GetTableRequest(dbName, tblName);
+ getTableRequest.setCatName(catName);
+ Table table;
+ try {
+ table = get_table_core(getTableRequest);
+ } catch (NoSuchObjectException e) {
+ if (!allowMissingTable || rqst.isSetPartitionname()) {
+ throw e;
+ }
+ authorizeCleanupOfMissingTable(rqst, catName, dbName);
+ return;
+ }
+ firePreEvent(new PreAlterTableEvent(table, table, this));
+
+ String location = table.getSd() == null ? null :
table.getSd().getLocation();
+ if (rqst.isSetPartitionname() && rqst.getPartitionname() != null) {
+ List<String> partVals = getPartValsFromName(table,
rqst.getPartitionname());
+ Partition partition = getMS().getPartition(catName, dbName, tblName,
partVals);
+ location = partition.getSd() == null ? null :
partition.getSd().getLocation();
+ }
+ if (location == null) {
+ throw new MetaException("Cannot determine storage location of " + dbName
+ "." + tblName
+ + "; compaction is not supported for this table or partition");
+ }
+ rqst.setRunas(TxnUtils.findUserToRunAs(location, table, conf));
Review Comment:
Existing-table cleanup still accepts the caller’s location, so Cleaner may
delete a path different from the authorized table.
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/handler/TransactionHandler.java:
##########
@@ -505,9 +522,108 @@ public ShowCompactResponse
show_compact(ShowCompactRequest rqst) throws TExcepti
@Override
public boolean submit_for_cleanup(CompactionRequest rqst, long
highestWriteId, long txnId)
throws TException {
+ try {
+ authorizeAndPrepareCompactionRequest(rqst, true);
+ } catch (Exception e) {
+ rethrowException(e);
+ }
return getTxnHandler().submitForCleanup(rqst, highestWriteId, txnId);
}
+ /**
+ * Authorize client-initiated compaction/cleanup requests and derive
server-side run-as identity.
+ * Internal callers (e.g. {@link
org.apache.hadoop.hive.metastore.AcidEventListener}) invoke
+ * {@code TxnHandler} directly and are not subject to this path.
+ *
+ * @param allowMissingTable only for cleanup requests: a rolled-back CTAS
(see {@code HiveQueryLifeTimeHook})
+ * asks for cleanup of a directory whose table was never created or has
already been dropped. In that case
+ * the request is authorized against the database and the requested
location must be strictly inside the
+ * database's managed/external location.
+ */
+ private void authorizeAndPrepareCompactionRequest(CompactionRequest rqst,
boolean allowMissingTable)
+ throws TException, IOException, InterruptedException {
+ if (rqst.getDbname() == null || rqst.getTablename() == null) {
+ throw new MetaException("The DB and table name cannot be null.");
+ }
+ String[] parsedDbName = parseDbName(rqst.getDbname(), conf);
+ String catName = parsedDbName[CAT_NAME];
+ String dbName = parsedDbName[DB_NAME];
+ String tblName = rqst.getTablename();
+ rqst.setDbname(dbName);
Review Comment:
The catalog is removed after authorization, so the queued operation may
affect a same-named table in the default catalog. Also, check if you need
normalize catName, dbName, and tblName.
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/handler/TransactionHandler.java:
##########
@@ -505,9 +522,108 @@ public ShowCompactResponse
show_compact(ShowCompactRequest rqst) throws TExcepti
@Override
public boolean submit_for_cleanup(CompactionRequest rqst, long
highestWriteId, long txnId)
throws TException {
+ try {
+ authorizeAndPrepareCompactionRequest(rqst, true);
+ } catch (Exception e) {
+ rethrowException(e);
+ }
return getTxnHandler().submitForCleanup(rqst, highestWriteId, txnId);
}
+ /**
+ * Authorize client-initiated compaction/cleanup requests and derive
server-side run-as identity.
+ * Internal callers (e.g. {@link
org.apache.hadoop.hive.metastore.AcidEventListener}) invoke
+ * {@code TxnHandler} directly and are not subject to this path.
+ *
+ * @param allowMissingTable only for cleanup requests: a rolled-back CTAS
(see {@code HiveQueryLifeTimeHook})
+ * asks for cleanup of a directory whose table was never created or has
already been dropped. In that case
+ * the request is authorized against the database and the requested
location must be strictly inside the
+ * database's managed/external location.
+ */
+ private void authorizeAndPrepareCompactionRequest(CompactionRequest rqst,
boolean allowMissingTable)
+ throws TException, IOException, InterruptedException {
+ if (rqst.getDbname() == null || rqst.getTablename() == null) {
+ throw new MetaException("The DB and table name cannot be null.");
+ }
+ String[] parsedDbName = parseDbName(rqst.getDbname(), conf);
+ String catName = parsedDbName[CAT_NAME];
+ String dbName = parsedDbName[DB_NAME];
+ String tblName = rqst.getTablename();
+ rqst.setDbname(dbName);
+
+ FilterUtils.checkDbAndTableFilters(isServerFilterEnabled, filterHook,
catName, dbName, tblName);
+
+ GetTableRequest getTableRequest = new GetTableRequest(dbName, tblName);
+ getTableRequest.setCatName(catName);
+ Table table;
+ try {
+ table = get_table_core(getTableRequest);
+ } catch (NoSuchObjectException e) {
+ if (!allowMissingTable || rqst.isSetPartitionname()) {
+ throw e;
+ }
+ authorizeCleanupOfMissingTable(rqst, catName, dbName);
+ return;
+ }
+ firePreEvent(new PreAlterTableEvent(table, table, this));
Review Comment:
This authorizes compaction as ADD COLUMNS. Could we use the actual COMPACT
operation instead?
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/handler/TransactionHandler.java:
##########
@@ -505,9 +522,108 @@ public ShowCompactResponse
show_compact(ShowCompactRequest rqst) throws TExcepti
@Override
public boolean submit_for_cleanup(CompactionRequest rqst, long
highestWriteId, long txnId)
throws TException {
+ try {
+ authorizeAndPrepareCompactionRequest(rqst, true);
+ } catch (Exception e) {
+ rethrowException(e);
+ }
return getTxnHandler().submitForCleanup(rqst, highestWriteId, txnId);
}
+ /**
+ * Authorize client-initiated compaction/cleanup requests and derive
server-side run-as identity.
+ * Internal callers (e.g. {@link
org.apache.hadoop.hive.metastore.AcidEventListener}) invoke
+ * {@code TxnHandler} directly and are not subject to this path.
+ *
+ * @param allowMissingTable only for cleanup requests: a rolled-back CTAS
(see {@code HiveQueryLifeTimeHook})
+ * asks for cleanup of a directory whose table was never created or has
already been dropped. In that case
+ * the request is authorized against the database and the requested
location must be strictly inside the
+ * database's managed/external location.
+ */
+ private void authorizeAndPrepareCompactionRequest(CompactionRequest rqst,
boolean allowMissingTable)
+ throws TException, IOException, InterruptedException {
+ if (rqst.getDbname() == null || rqst.getTablename() == null) {
+ throw new MetaException("The DB and table name cannot be null.");
+ }
+ String[] parsedDbName = parseDbName(rqst.getDbname(), conf);
+ String catName = parsedDbName[CAT_NAME];
+ String dbName = parsedDbName[DB_NAME];
+ String tblName = rqst.getTablename();
+ rqst.setDbname(dbName);
+
+ FilterUtils.checkDbAndTableFilters(isServerFilterEnabled, filterHook,
catName, dbName, tblName);
+
+ GetTableRequest getTableRequest = new GetTableRequest(dbName, tblName);
+ getTableRequest.setCatName(catName);
+ Table table;
+ try {
+ table = get_table_core(getTableRequest);
+ } catch (NoSuchObjectException e) {
+ if (!allowMissingTable || rqst.isSetPartitionname()) {
+ throw e;
+ }
+ authorizeCleanupOfMissingTable(rqst, catName, dbName);
+ return;
+ }
+ firePreEvent(new PreAlterTableEvent(table, table, this));
+
+ String location = table.getSd() == null ? null :
table.getSd().getLocation();
+ if (rqst.isSetPartitionname() && rqst.getPartitionname() != null) {
+ List<String> partVals = getPartValsFromName(table,
rqst.getPartitionname());
+ Partition partition = getMS().getPartition(catName, dbName, tblName,
partVals);
+ location = partition.getSd() == null ? null :
partition.getSd().getLocation();
+ }
+ if (location == null) {
+ throw new MetaException("Cannot determine storage location of " + dbName
+ "." + tblName
+ + "; compaction is not supported for this table or partition");
+ }
+ rqst.setRunas(TxnUtils.findUserToRunAs(location, table, conf));
+ }
+
+ /**
+ * Cleanup of a directory that has no backing table: authorize against the
database, require that the
+ * requested location is strictly inside the database's locations (so that a
client cannot get an arbitrary
+ * path purged by the Cleaner) and derive the run-as user from the directory
owner.
+ */
+ private void authorizeCleanupOfMissingTable(CompactionRequest rqst, String
catName, String dbName)
+ throws TException, IOException, InterruptedException {
+ Database db = get_database_core(catName, dbName);
+ firePreEvent(new PreAlterDatabaseEvent(db, db, this));
+
+ String requested = rqst.getProperties() == null ? null :
rqst.getProperties().get(META_TABLE_LOCATION);
+ if (requested == null || requested.isEmpty()) {
+ throw new MetaException("Cannot determine storage location of " + dbName
+ "." + rqst.getTablename()
+ + "; the table does not exist and no location was supplied for the
cleanup");
+ }
+ Warehouse warehouse = getWh();
+ Path location = normalizePath(warehouse, new Path(requested));
+ List<Path> allowedRoots = new ArrayList<>(3);
+ if (db.getLocationUri() != null) {
+ allowedRoots.add(normalizePath(warehouse, new
Path(db.getLocationUri())));
+ }
+ if (db.getManagedLocationUri() != null) {
+ allowedRoots.add(normalizePath(warehouse, new
Path(db.getManagedLocationUri())));
+ }
+ allowedRoots.add(normalizePath(warehouse,
warehouse.getDatabaseManagedPath(db)));
+ boolean allowed = allowedRoots.stream()
Review Comment:
Database containment isn’t enough. A fake table name can still target
another table’s directory right?
--
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]
