Copilot commented on code in PR #6884:
URL: https://github.com/apache/hive/pull/6884#discussion_r4407697799
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/handler/BaseHandler.java:
##########
@@ -874,6 +874,28 @@ private void authorizeProxyPrivilege() throws TException {
}
}
+ /**
+ * Tells whether the caller may be treated as a trusted proxy/operator (e.g.
HiveServer2 or the compactor),
+ * using the same rules as {@link #authorizeProxyPrivilege()} but without
failing the call: the caller is
+ * trusted in embedded mode, when the proxy check is disabled, in test mode,
or when the user has host-proxy
+ * privileges.
+ */
+ protected boolean hasProxyPrivilege() throws TException {
+ if (!HiveMetaStore.isMetaStoreRemote() ||
+ !MetastoreConf.getBoolVar(conf,
MetastoreConf.ConfVars.EVENT_DB_NOTIFICATION_API_AUTH) ||
+ conf.getBoolean(HIVE_IN_TEST.getVarname(), false)) {
Review Comment:
Disabling `metastore.event.db.notification.api.auth`, documented as
notification-only, now lets every remote caller bypass table authorization,
run-as derivation, and cleanup-path validation in both maintenance APIs. Remove
this setting from the trust decision so notification access does not also grant
compaction and cleanup privileges. This is VALID under THREAT_MODEL.md §13:
direct HMS clients are in scope (§3.3), and configured authorization must be
enforced (§5).
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/handler/BaseHandler.java:
##########
@@ -874,6 +874,28 @@ private void authorizeProxyPrivilege() throws TException {
}
}
+ /**
+ * Tells whether the caller may be treated as a trusted proxy/operator (e.g.
HiveServer2 or the compactor),
+ * using the same rules as {@link #authorizeProxyPrivilege()} but without
failing the call: the caller is
+ * trusted in embedded mode, when the proxy check is disabled, in test mode,
or when the user has host-proxy
+ * privileges.
+ */
+ protected boolean hasProxyPrivilege() throws TException {
+ if (!HiveMetaStore.isMetaStoreRemote() ||
+ !MetastoreConf.getBoolVar(conf,
MetastoreConf.ConfVars.EVENT_DB_NOTIFICATION_API_AUTH) ||
+ conf.getBoolean(HIVE_IN_TEST.getVarname(), false)) {
+ return true;
+ }
+ String user;
+ try {
+ user = SecurityUtils.getUGI().getShortUserName();
Review Comment:
For SASL PLAIN clients on Kerberized Hadoop, the transport records
`remoteUser` but invokes the handler without `doAs`, so this check uses the HMS
service identity and can bypass authorization and request validation if that
identity has proxy-host privileges. Use the authenticated transport identity,
fail closed when it is unavailable, and add a non-proxy PLAIN-client regression
test. This is VALID under THREAT_MODEL.md §13 because direct HMS clients are in
scope (§3.3) and authorization must use the authenticated caller (§5).
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/handler/TransactionHandler.java:
##########
@@ -505,9 +524,87 @@ public ShowCompactResponse show_compact(ShowCompactRequest
rqst) throws TExcepti
@Override
public boolean submit_for_cleanup(CompactionRequest rqst, long
highestWriteId, long txnId)
throws TException {
+ if (!hasProxyPrivilege()) {
+ try {
+ authorizeAndPrepareCompactionRequest(rqst);
+ } catch (NoSuchObjectException e) {
+ // The table/partition does not exist (anymore): there is nothing an
untrusted caller may clean up.
+ LOG.info("Ignoring the cleanup request of {}.{}: {}",
rqst.getDbname(), rqst.getTablename(),
+ e.getMessage());
+ return false;
+ } catch (Exception e) {
+ rethrowException(e);
+ }
+ }
return getTxnHandler().submitForCleanup(rqst, highestWriteId, txnId);
}
+ /**
+ * Authorize a compaction/cleanup request of a caller that has no proxy
privilege and derive the
+ * server-side request attributes: the table is authorized with a {@link
PreAlterTableEvent}, the
+ * canonical db/table names are used for the queued request, the run-as user
is derived from the
+ * storage location and a caller-supplied cleanup location must stay inside
the table/partition location.
+ *
+ * @throws NoSuchObjectException if the table or the partition does not exist
+ */
+ private void authorizeAndPrepareCompactionRequest(CompactionRequest rqst)
throws TException {
+ 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];
+ if (!getDefaultCatalog(conf).equalsIgnoreCase(catName)) {
+ // the compaction queue has no notion of catalogs, so the request would
end up on the default catalog
+ throw new MetaException("Compaction and cleanup requests are only
supported for the default catalog, got "
+ + catName);
+ }
+
+ GetTableRequest getTableRequest = new GetTableRequest(dbName,
rqst.getTablename());
+ getTableRequest.setCatName(catName);
+ Table table = get_table_core(getTableRequest);
+ firePreEvent(new PreAlterTableEvent(table, table, this));
Review Comment:
Passing the same `Table` as both old and new causes the automatically
installed `TransactionalValidationListener` to mistake existing full-ACID
tables for conversions: it removes `transactional` from the new parameters
before reading the old value. For tables containing normal `bucket_*` data
files, conversion validation then rejects the compaction or cleanup request
before queueing. Pass a deep copy as the new table to preserve the old state.
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/handler/TransactionHandler.java:
##########
@@ -505,9 +524,87 @@ public ShowCompactResponse show_compact(ShowCompactRequest
rqst) throws TExcepti
@Override
public boolean submit_for_cleanup(CompactionRequest rqst, long
highestWriteId, long txnId)
throws TException {
+ if (!hasProxyPrivilege()) {
+ try {
+ authorizeAndPrepareCompactionRequest(rqst);
+ } catch (NoSuchObjectException e) {
+ // The table/partition does not exist (anymore): there is nothing an
untrusted caller may clean up.
+ LOG.info("Ignoring the cleanup request of {}.{}: {}",
rqst.getDbname(), rqst.getTablename(),
+ e.getMessage());
+ return false;
+ } catch (Exception e) {
+ rethrowException(e);
+ }
+ }
return getTxnHandler().submitForCleanup(rqst, highestWriteId, txnId);
}
+ /**
+ * Authorize a compaction/cleanup request of a caller that has no proxy
privilege and derive the
+ * server-side request attributes: the table is authorized with a {@link
PreAlterTableEvent}, the
+ * canonical db/table names are used for the queued request, the run-as user
is derived from the
+ * storage location and a caller-supplied cleanup location must stay inside
the table/partition location.
+ *
+ * @throws NoSuchObjectException if the table or the partition does not exist
+ */
+ private void authorizeAndPrepareCompactionRequest(CompactionRequest rqst)
throws TException {
+ 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];
+ if (!getDefaultCatalog(conf).equalsIgnoreCase(catName)) {
+ // the compaction queue has no notion of catalogs, so the request would
end up on the default catalog
+ throw new MetaException("Compaction and cleanup requests are only
supported for the default catalog, got "
+ + catName);
+ }
+
+ GetTableRequest getTableRequest = new GetTableRequest(dbName,
rqst.getTablename());
+ getTableRequest.setCatName(catName);
+ Table table = get_table_core(getTableRequest);
+ firePreEvent(new PreAlterTableEvent(table, table, this));
+
+ // queue the request for the table that has been authorized
+ rqst.setDbname(table.getDbName());
+ rqst.setTablename(table.getTableName());
+
+ 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, table.getDbName(),
table.getTableName(), partVals);
Review Comment:
Authorized non-proxy callers can no longer submit partition-scoped Iceberg
compactions because this lookup requires an HMS partition record. Iceberg
partition names come from storage-handler metadata and are resolved by the
Iceberg compaction implementation, not HMS. Distinguish native partitions from
storage-handler partitions, retaining table authorization and using the table
location for run-as derivation in the latter case.
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/handler/TransactionHandler.java:
##########
@@ -505,9 +524,87 @@ public ShowCompactResponse show_compact(ShowCompactRequest
rqst) throws TExcepti
@Override
public boolean submit_for_cleanup(CompactionRequest rqst, long
highestWriteId, long txnId)
throws TException {
+ if (!hasProxyPrivilege()) {
+ try {
+ authorizeAndPrepareCompactionRequest(rqst);
+ } catch (NoSuchObjectException e) {
+ // The table/partition does not exist (anymore): there is nothing an
untrusted caller may clean up.
+ LOG.info("Ignoring the cleanup request of {}.{}: {}",
rqst.getDbname(), rqst.getTablename(),
+ e.getMessage());
+ return false;
+ } catch (Exception e) {
+ rethrowException(e);
+ }
+ }
return getTxnHandler().submitForCleanup(rqst, highestWriteId, txnId);
}
+ /**
+ * Authorize a compaction/cleanup request of a caller that has no proxy
privilege and derive the
+ * server-side request attributes: the table is authorized with a {@link
PreAlterTableEvent}, the
+ * canonical db/table names are used for the queued request, the run-as user
is derived from the
+ * storage location and a caller-supplied cleanup location must stay inside
the table/partition location.
+ *
+ * @throws NoSuchObjectException if the table or the partition does not exist
+ */
+ private void authorizeAndPrepareCompactionRequest(CompactionRequest rqst)
throws TException {
+ 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];
+ if (!getDefaultCatalog(conf).equalsIgnoreCase(catName)) {
+ // the compaction queue has no notion of catalogs, so the request would
end up on the default catalog
+ throw new MetaException("Compaction and cleanup requests are only
supported for the default catalog, got "
+ + catName);
+ }
+
+ GetTableRequest getTableRequest = new GetTableRequest(dbName,
rqst.getTablename());
+ getTableRequest.setCatName(catName);
+ Table table = get_table_core(getTableRequest);
+ firePreEvent(new PreAlterTableEvent(table, table, this));
+
+ // queue the request for the table that has been authorized
+ rqst.setDbname(table.getDbName());
+ rqst.setTablename(table.getTableName());
+
+ 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, table.getDbName(),
table.getTableName(), partVals);
Review Comment:
This lookup accepts `p=a/extra=b` for an existing partition `p=a`, but
leaves the original name in the queued request. With a supplied cleanup
location, the cleaner's later lookup of that raw name can fail and trigger its
dropped-partition purge path for a live partition. Canonicalize the queued name
from the fetched partition so validation and execution target the same
partition.
--
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]
