This is an automated email from the ASF dual-hosted git repository.
xiangfu0 pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/pinot.git
The following commit(s) were added to refs/heads/master by this push:
new f60f17fd701 Reject cross-table functions under row-level security
(#20169)
f60f17fd701 is described below
commit f60f17fd701cf326ad4e9d28735a11eee7de3d7b
Author: Xiang Fu <[email protected]>
AuthorDate: Fri Nov 6 17:41:30 2026 -0800
Reject cross-table functions under row-level security (#20169)
* Apply row-level security filters to the subquery of
IN_PARTITIONED_SUBQUERY
The broker ANDs the row filters of the principal into the WHERE clause of
the
query and stamps them in the query options, but the subquery of
IN_PARTITIONED_SUBQUERY reaches the server as a string literal that carries
neither. The server compiled that literal and ran it on the segments of the
table as is, so the ID_SET it built covered rows the principal is not
allowed
to see: a principal with row filters could test whether values exist in
other
principals' rows, and enumerate them with GROUP BY, while the response still
reported rlsFiltersApplied.
Server: compile the literal to a PinotQuery, rename its data source to the
queried table (the subquery runs on that table's segments whatever its FROM
clause names), add the table's row filter to the options the subquery was
written with and run RlsFiltersRewriter on it before converting it to a
QueryContext. A subquery nested in the subquery is filtered the same way
through the recursion. A query that carries row filters but none for the
queried table fails instead of running the subquery unfiltered.
Broker: reject the function with access denied when row filters apply to the
table, before the row filters are added. The broker cannot tell a server of
this release from an older one, so it fails closed instead of forwarding a
subquery that an older server would run unfiltered. The new broker config
pinot.broker.rls.reject.in.partitioned.subquery (default true) turns the
rejection off once every server runs this release. Principals without row
filters keep the function.
Both sides match the function name through the new
RequestUtils.isInPartitionedSubquery, the check the server already used
(underscores removed, case folded per character), so a look-alike spelling
the
server resolves cannot slip past the broker.
Upgrade matrix: new broker + old server rejects the query; old broker + new
server filters the subquery through the rlsFilters-<table> option old
brokers
already send; new + new rejects by default. No wire or format change.
Tests: QueryExecutorTest covers the server path, including a subquery whose
FROM clause names another table and a nested subquery;
BaseSingleStageBrokerRequestHandlerTest covers the detection under every
spelling, the rejection, and the config that lifts it; RequestUtilsTest
covers
the matcher; RowLevelSecurityIntegrationTest covers the rejection end to
end.
The server and broker unit tests fail on the code before this change.
* Always reject IN_PARTITIONED_SUBQUERY under row-level security
Drop pinot.broker.rls.reject.in.partitioned.subquery: the broker never
forwards
the function for a principal with row filters, whatever the servers run. The
server-side filter stays as defense in depth for an older broker.
* Decide row-level security for every cross-table function in one place
Add CrossTableFunction to pinot-common: the functions that read rows from
somewhere other than the FROM clause (lookup, IN_SUBQUERY,
IN_PARTITIONED_SUBQUERY) and the one spelling check they share, underscores
dropped and case folded per character, as the server resolves them.
DeleteStatement, the single-stage broker, the multi-stage IdSet subquery
rewriter and the server executor all match through it instead of their own
hand-spelled constants. The broker walks the query once for every
cross-table
call: the walk feeds the lookup() table collection used for table access and
the row-filter check, whose exhaustive switch decides each kind (IN_SUBQUERY
runs as its own request, lookup() is checked against the dimension table's
own row filters, IN_PARTITIONED_SUBQUERY is rejected), so a new cross-table
function cannot be added without deciding how row filters apply to it.
* Require the subquery of IN_PARTITIONED_SUBQUERY to read the queried table
under row filters
Row filters belong to one table. Instead of renaming the subquery's data
source to the queried table so that the outer table's filter applies to
whatever the subquery names, the server now fails a subquery that names
another table when row filters apply, and applies the table's own filter
when it names the queried table. Without row filters nothing changes: the
subquery still runs on the queried table's segments as it always has.
* Cover the fail-closed branch for row filters stamped for another table
QueryExecutorTest now checks that a query carrying row filters for another
table only fails instead of running the subquery unfiltered, and the broker
doc of the cross-table check states that it covers the physical-table path,
since the logical-table path applies no row filters at all today.
* Reject every cross-table function under row-level security
The row filters are added to the WHERE clause of the queried table, which
no cross-table function reads
through. Rather than decide IN_SUBQUERY, lookup() and
IN_PARTITIONED_SUBQUERY each on their own, the
single-stage broker now rejects all of them with 403 for a principal with
row filters, collecting them at
compile time before IN_SUBQUERY calls are replaced by their results. The
server fails IN_PARTITIONED_SUBQUERY
closed when row filters are stamped, for a server behind an older broker,
instead of filtering the subquery.
* Replace the CrossTableFunction enum with a RequestUtils membership check
Per review: RequestUtils.isCrossTableFunction(canonicalName), backed by a
set of canonical names, replaces the enum and its per-function matches()
API. The broker keeps a boolean in CompileResult, collected before the
IN_SUBQUERY rewrite, and rejects with a generic message.
The incidental refactors are reverted: the broker's handleSubquery and
fixColumnName, IdSetSubqueryRewriter, the server's IN_PARTITIONED_SUBQUERY
name check and extractLookupTableNames are back to their specific-function
form, and DeleteStatement keeps its own list, cross-referenced. The
server's row-filter guard is unchanged.
* Inline the row-filter checks and gate the cross-table scan on row/column
auth
Per review: the broker's rejection and the server's IN_PARTITIONED_SUBQUERY
guard each had one caller, so they sit at their call sites with a short
comment, and the broker only scans the query for cross-table functions
when row/column-level auth is enabled, since that is the only consumer.
* Fold the cross-table function name through Locale.ROOT before the broker
check
The parser lower-cases the operator with the default locale, while an older
server resolves IN_PARTITIONED_SUBQUERY with equalsIgnoreCase, so a dotless
ı
written in the query, or made of the I by a Turkish locale, passed the
broker's membership check and ran the subquery unfiltered on a server
without the row-filter guard. The broker now folds the name through
Locale.ROOT in both directions, which matches what the server accepts,
before the check; the dotless-ı case is back in the broker test, with a
Turkish-locale case.
---
.../BaseSingleStageBrokerRequestHandler.java | 66 +++++++-
.../BaseSingleStageBrokerRequestHandlerTest.java | 166 +++++++++++++++++++++
.../pinot/common/utils/request/RequestUtils.java | 20 +++
.../pinot/sql/parsers/dml/DeleteStatement.java | 2 +
.../common/utils/request/RequestUtilsTest.java | 19 +++
.../query/executor/ServerQueryExecutorV1Impl.java | 24 ++-
.../core/query/executor/QueryExecutorTest.java | 44 ++++++
.../tests/RowLevelSecurityIntegrationTest.java | 32 ++++
8 files changed, 363 insertions(+), 10 deletions(-)
diff --git
a/pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseSingleStageBrokerRequestHandler.java
b/pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseSingleStageBrokerRequestHandler.java
index 89aaeee8763..7265f7120ed 100644
---
a/pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseSingleStageBrokerRequestHandler.java
+++
b/pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseSingleStageBrokerRequestHandler.java
@@ -26,6 +26,7 @@ import java.util.ArrayList;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
+import java.util.Locale;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.BlockingQueue;
@@ -112,6 +113,7 @@ import org.apache.pinot.query.parser.utils.ParserUtils;
import org.apache.pinot.spi.accounting.ThreadAccountant;
import org.apache.pinot.spi.auth.AuthProvider;
import org.apache.pinot.spi.auth.AuthorizationResult;
+import org.apache.pinot.spi.auth.BasicAuthorizationResultImpl;
import org.apache.pinot.spi.auth.TableAuthorizationResult;
import org.apache.pinot.spi.auth.TableRowColAccessResult;
import org.apache.pinot.spi.auth.broker.RequesterIdentity;
@@ -407,16 +409,20 @@ public abstract class BaseSingleStageBrokerRequestHandler
extends BaseBrokerRequ
final String _rawTableName;
/// Dimension tables read by `lookup()` calls, which are not part of the
data source
final Set<String> _lookupTableNames;
+ /// Whether the query calls a cross-table function as written, collected
before `handleSubquery()` replaced its
+ /// `IN_SUBQUERY` calls with their results, for the row-filter rejection
in `doHandleRequest()`
+ final boolean _hasCrossTableFunction;
final BrokerResponse _errorOrLiteralOnlyBrokerResponse;
public CompileResult(PinotQuery pinotQuery, PinotQuery serverPinotQuery,
Schema schema, String tableName,
- String rawTableName, Set<String> lookupTableNames) {
+ String rawTableName, Set<String> lookupTableNames, boolean
hasCrossTableFunction) {
_pinotQuery = pinotQuery;
_serverPinotQuery = serverPinotQuery;
_schema = schema;
_tableName = tableName;
_rawTableName = rawTableName;
_lookupTableNames = lookupTableNames;
+ _hasCrossTableFunction = hasCrossTableFunction;
_errorOrLiteralOnlyBrokerResponse = null;
}
@@ -427,6 +433,7 @@ public abstract class BaseSingleStageBrokerRequestHandler
extends BaseBrokerRequ
_tableName = null;
_rawTableName = null;
_lookupTableNames = Set.of();
+ _hasCrossTableFunction = false;
_errorOrLiteralOnlyBrokerResponse = errorOrLiteralOnlyBrokerResponse;
}
}
@@ -561,6 +568,16 @@ public abstract class BaseSingleStageBrokerRequestHandler
extends BaseBrokerRequ
TableRowColAccessResult rlsFilters =
accessControl.getRowColFilters(requesterIdentity, tableName);
List<String> rowFilters = rlsFilters.getRLSFilters().orElse(null);
if (rowFilters != null && !rowFilters.isEmpty()) {
+ // The row filters cover the queried table only, while a cross-table
function (lookup(), IN_SUBQUERY,
+ // IN_PARTITIONED_SUBQUERY) reads rows from somewhere else:
IN_PARTITIONED_SUBQUERY runs its subquery on the
+ // server outside that WHERE clause and leaked the hidden rows.
Reject them all rather than decide each one,
+ // so a new one is covered the day it is added. Physical tables
only: the logical-table path above applies
+ // no row filters at all today.
+ if (compileResult._hasCrossTableFunction) {
+ throwAccessDeniedError(requestId, query, requestContext, tableName,
+ new BasicAuthorizationResultImpl(false, "Cross-table functions
are not allowed on table "
+ + rawTableName + " when row-level security filters
apply"));
+ }
/// Apply RLS to `serverPinotQuery` (the query the server actually
executes), not to
/// `pinotQuery` as the pre-MV codebase did — for gapfill queries
`serverPinotQuery` is
/// the inner stripped query and `pinotQuery` is the outer gapfill
wrapper, so the pre-MV
@@ -1179,6 +1196,13 @@ public abstract class
BaseSingleStageBrokerRequestHandler extends BaseBrokerRequ
return new CompileResult(new
BrokerResponseNative(QueryErrorCode.QUERY_VALIDATION, "Table name not found"));
}
+ // Collected before handleSubquery() replaces each IN_SUBQUERY call with
IN_ID_SET on its result, for
+ // doHandleRequest to reject cross-table functions once it knows whether
row filters apply, and only consumed
+ // under that gate. For a gapfill query the two differ, and only the
stripped one reaches a server.
+ boolean hasCrossTableFunction = _enableRowColumnLevelAuth
+ && (hasCrossTableFunction(serverPinotQuery)
+ || (serverPinotQuery != pinotQuery &&
hasCrossTableFunction(pinotQuery)));
+
try {
handleSubquery(serverPinotQuery, requestId, request, requesterIdentity,
requestContext, httpHeaders,
accessControl);
@@ -1257,7 +1281,8 @@ public abstract class BaseSingleStageBrokerRequestHandler
extends BaseBrokerRequ
Schema schema = _tableCache.getSchema(rawTableName);
_queryOptimizer.optimize(serverPinotQuery, schema);
- return new CompileResult(pinotQuery, serverPinotQuery, schema, tableName,
rawTableName, lookupTableNames);
+ return new CompileResult(pinotQuery, serverPinotQuery, schema, tableName,
rawTableName, lookupTableNames,
+ hasCrossTableFunction);
}
/// Mutable holder returned from [#applyMaterializedViewRewriteAtCompile] —
Java has no out
@@ -2289,6 +2314,43 @@ public abstract class
BaseSingleStageBrokerRequestHandler extends BaseBrokerRequ
collectLookupTableNames(operands, lookupTableNames);
}
+ /// Returns whether the query calls a cross-table function anywhere: in the
select list, the filter, the having
+ /// clause and the group-by and order-by expressions, calls nested in other
calls included. See
+ /// [RequestUtils#isCrossTableFunction(String)] for the functions.
+ @VisibleForTesting
+ static boolean hasCrossTableFunction(PinotQuery pinotQuery) {
+ return hasCrossTableFunction(pinotQuery.getSelectList())
+ || hasCrossTableFunction(pinotQuery.getFilterExpression())
+ || hasCrossTableFunction(pinotQuery.getHavingExpression())
+ || hasCrossTableFunction(pinotQuery.getGroupByList())
+ || hasCrossTableFunction(pinotQuery.getOrderByList());
+ }
+
+ private static boolean hasCrossTableFunction(@Nullable List<Expression>
expressions) {
+ if (expressions != null) {
+ for (Expression expression : expressions) {
+ if (hasCrossTableFunction(expression)) {
+ return true;
+ }
+ }
+ }
+ return false;
+ }
+
+ private static boolean hasCrossTableFunction(@Nullable Expression
expression) {
+ if (expression == null || expression.getType() != ExpressionType.FUNCTION)
{
+ return false;
+ }
+ Function functionCall = expression.getFunctionCall();
+ // The operator of a compiled call is canonical, see
RequestUtils.canonicalizeFunctionName(), but lower-cased with
+ // the default locale, while the server resolves IN_PARTITIONED_SUBQUERY
with equalsIgnoreCase(): fold it through
+ // Locale.ROOT so that a dotless ı (U+0131), written in the query or made
of the I by a Turkish locale, and a long
+ // ſ (U+017F) are matched as the server matches them, or a server without
the row-filter guard would run the
+ // subquery unfiltered behind this broker.
+ String canonicalName =
functionCall.getOperator().toUpperCase(Locale.ROOT).toLowerCase(Locale.ROOT);
+ return RequestUtils.isCrossTableFunction(canonicalName) ||
hasCrossTableFunction(functionCall.getOperands());
+ }
+
/// Fixes the column names to the actual column names in the given
expression.
private static void fixColumnName(String rawTableName, Expression
expression, Map<String, String> columnNameMap,
boolean ignoreCase) {
diff --git
a/pinot-broker/src/test/java/org/apache/pinot/broker/requesthandler/BaseSingleStageBrokerRequestHandlerTest.java
b/pinot-broker/src/test/java/org/apache/pinot/broker/requesthandler/BaseSingleStageBrokerRequestHandlerTest.java
index deae4f9f903..c7864c5ab31 100644
---
a/pinot-broker/src/test/java/org/apache/pinot/broker/requesthandler/BaseSingleStageBrokerRequestHandlerTest.java
+++
b/pinot-broker/src/test/java/org/apache/pinot/broker/requesthandler/BaseSingleStageBrokerRequestHandlerTest.java
@@ -24,6 +24,7 @@ import java.net.InetSocketAddress;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
+import java.util.Locale;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.CompletableFuture;
@@ -33,6 +34,8 @@ import java.util.concurrent.Executors;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicReference;
import javax.annotation.Nullable;
+import javax.ws.rs.WebApplicationException;
+import javax.ws.rs.core.Response;
import org.apache.hc.client5.http.impl.io.PoolingHttpClientConnectionManager;
import org.apache.helix.model.InstanceConfig;
import org.apache.pinot.broker.api.AccessControl;
@@ -97,14 +100,18 @@ import org.testng.annotations.AfterMethod;
import org.testng.annotations.Test;
import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyLong;
import static org.mockito.ArgumentMatchers.anyString;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertFalse;
import static org.testng.Assert.assertNotNull;
import static org.testng.Assert.assertNull;
+import static org.testng.Assert.assertTrue;
+import static org.testng.Assert.expectThrows;
public class BaseSingleStageBrokerRequestHandlerTest {
@@ -870,6 +877,165 @@ public class BaseSingleStageBrokerRequestHandlerTest {
"Expected TOO_MANY_REQUESTS error code");
}
+ @Test
+ public void testHasCrossTableFunction() {
+ String subquery = "SELECT ID_SET(col1) FROM myTable WHERE col2 = 2";
+ // The operator of a compiled call is canonical (underscores removed,
lower-cased), so every spelling counts, the
+ // dotless ı (U+0131) look-alike that the server resolves too included
+ String[] spellings = {
+ "IN_PARTITIONED_SUBQUERY", "inpartitionedsubquery",
"In_Partitioned_Sub_Query", "IN_PART\u0131TIONED_SUBQUERY"
+ };
+ for (String spelling : spellings) {
+ assertTrue(hasCrossTableFunction("SELECT COUNT(*) FROM myTable WHERE " +
spelling + "(col1, '" + subquery
+ + "') = 1"), spelling);
+ }
+ // Nested in other predicates and expressions, and outside the WHERE clause
+ assertTrue(hasCrossTableFunction("SELECT COUNT(*) FROM myTable WHERE col2
= 1 AND NOT (col3 > 2 OR "
+ + "IN_PARTITIONED_SUBQUERY(col1, '" + subquery + "') = 1)"));
+ assertTrue(hasCrossTableFunction("SELECT CASE WHEN
IN_PARTITIONED_SUBQUERY(col1, '" + subquery
+ + "') = 1 THEN 1 ELSE 0 END FROM myTable"));
+ assertTrue(hasCrossTableFunction("SELECT col1, COUNT(*) FROM myTable GROUP
BY col1 HAVING "
+ + "IN_PARTITIONED_SUBQUERY(col1, '" + subquery + "') = 1"));
+ // The other cross-table functions, alone and together, a lookup() nested
in a lookup() included
+ assertTrue(hasCrossTableFunction("SELECT COUNT(*) FROM myTable WHERE
IN_SUBQUERY(col1, '" + subquery
+ + "') = 1"));
+ assertTrue(hasCrossTableFunction("SELECT lookup('dimTable', 'name', 'id',
col1) FROM myTable"));
+ assertTrue(hasCrossTableFunction("SELECT UPPER(lookup('dimTable', 'name',
'id', col1)) FROM myTable"));
+ assertTrue(hasCrossTableFunction("SELECT lookup('dimTable', 'name', 'id',
lookup('other', 'id', 'key', col1)) "
+ + "FROM myTable WHERE In_Sub_Query(col2, '" + subquery + "') = 1 ORDER
BY LOOK_UP('third', 'v', 'k', col3)"));
+ assertTrue(hasCrossTableFunction("SELECT col1 FROM myTable ORDER BY
LOOK_UP('dimTable', 'v', 'k', col1)"));
+ // Other functions, IN_ID_SET on a literal included, are not cross-table
functions
+ assertFalse(hasCrossTableFunction("SELECT COUNT(*) FROM myTable WHERE
IN_ID_SET(col1, 'AQ==') = 1"));
+ assertFalse(hasCrossTableFunction("SELECT COUNT(*) FROM myTable WHERE col1
= 1"));
+ // Under a Turkish default locale the parser lower-cases the I of the
plain spelling to a dotless ı as well
+ Locale defaultLocale = Locale.getDefault();
+ Locale.setDefault(Locale.forLanguageTag("tr-TR"));
+ try {
+ assertTrue(hasCrossTableFunction("SELECT COUNT(*) FROM myTable WHERE
IN_PARTITIONED_SUBQUERY(col1, '" + subquery
+ + "') = 1"));
+ } finally {
+ Locale.setDefault(defaultLocale);
+ }
+ }
+
+ private static boolean hasCrossTableFunction(String sql) {
+ return
BaseSingleStageBrokerRequestHandler.hasCrossTableFunction(CalciteSqlParser.compileToPinotQuery(sql));
+ }
+
+ /// The row filters are added to the WHERE clause of the queried table,
which no cross-table function reads through,
+ /// so a principal with row filters is denied every one of them.
`IN_SUBQUERY` runs a nested request that needs a
+ /// server response, so RowLevelSecurityIntegrationTest covers it and
`testHasCrossTableFunction` its detection.
+ @Test
+ public void testCrossTableFunctionsRejectedWhenRowFiltersApply()
+ throws Exception {
+ String tableName = "myTable_OFFLINE";
+ TableCache tableCache = mock(TableCache.class);
+ TableConfig tableCfg = mock(TableConfig.class);
+ when(tableCache.getActualTableName(anyString())).thenReturn(tableName);
+ when(tableCache.getColumnNameMap(anyString())).thenReturn(Map.of("col1",
"col1"));
+ when(tableCfg.getTenantConfig()).thenReturn(new
TenantConfig("tier_BROKER", "tier_SERVER", null));
+ when(tableCache.getTableConfig(tableName)).thenReturn(tableCfg);
+ BrokerRoutingManager routingManager = mock(BrokerRoutingManager.class);
+ when(routingManager.routingExists(tableName)).thenReturn(true);
+ when(routingManager.getQueryTimeoutMs(tableName)).thenReturn(10000L);
+ RoutingTable rt = mock(RoutingTable.class);
+ when(rt.getServerInstanceToSegmentsMap()).thenReturn(
+ Map.of(new ServerInstance(new InstanceConfig("Server_localhost_9000")),
+ new SegmentsToQuery(List.of("segment01"), List.of())));
+ when(routingManager.getRoutingTable(any(), anyLong())).thenReturn(rt);
+ QueryQuotaManager queryQuotaManager = mock(QueryQuotaManager.class);
+ when(queryQuotaManager.acquire(anyString())).thenReturn(true);
+ when(queryQuotaManager.acquireDatabase(anyString())).thenReturn(true);
+ when(queryQuotaManager.acquireApplication(anyString())).thenReturn(true);
+
+ AtomicReference<List<String>> rowFilters = new
AtomicReference<>(List.of("tenant = 'allowed'"));
+ AccessControl accessControl = new AccessControl() {
+ @Override
+ public AuthorizationResult authorize(RequesterIdentity identity,
BrokerRequest request) {
+ return TableAuthorizationResult.success();
+ }
+
+ @Override
+ public TableAuthorizationResult authorize(RequesterIdentity identity,
Set<String> tables) {
+ return TableAuthorizationResult.success();
+ }
+
+ @Override
+ public TableRowColAccessResult getRowColFilters(RequesterIdentity
identity, String table) {
+ // Row filters on the queried table only: a dimension table with row
filters is rejected by the lookup check
+ return new TableRowColAccessResultImpl(table.equals(tableName) ?
rowFilters.get() : List.of());
+ }
+ };
+ AccessControlFactory accessControlFactory =
mock(AccessControlFactory.class);
+ when(accessControlFactory.create()).thenReturn(accessControl);
+ BrokerMetrics.register(mock(BrokerMetrics.class));
+ PinotConfiguration config =
+ new
PinotConfiguration(Map.of(Broker.CONFIG_OF_BROKER_ENABLE_ROW_COLUMN_LEVEL_AUTH,
"true"));
+ BrokerQueryEventListenerFactory.init(config);
+ AtomicReference<BrokerRequest> serverRequest = new AtomicReference<>();
+ BaseSingleStageBrokerRequestHandler handler =
+ newServerRequestCapturingHandler(config, routingManager,
accessControlFactory, queryQuotaManager, tableCache,
+ serverRequest);
+
+ Map<String, String> queries = Map.of(
+ "IN_PARTITIONED_SUBQUERY",
+ "SELECT COUNT(*) FROM myTable WHERE IN_PARTITIONED_SUBQUERY(col1,
'SELECT ID_SET(col1) FROM myTable') = 1",
+ "lookup", "SELECT lookup('dimTable', 'dimValue', 'dimKey', col1) FROM
myTable");
+ for (Map.Entry<String, String> entry : queries.entrySet()) {
+ String function = entry.getKey();
+ String query = entry.getValue();
+
+ // Rejected before anything reaches a server
+ rowFilters.set(List.of("tenant = 'allowed'"));
+ serverRequest.set(null);
+ WebApplicationException e = expectThrows(WebApplicationException.class,
() -> handler.handleRequest(query));
+ assertEquals(e.getResponse().getStatus(),
Response.Status.FORBIDDEN.getStatusCode(), function);
+ assertTrue(e.getMessage().contains("Cross-table functions are not
allowed"), e.getMessage());
+ assertNull(serverRequest.get(), function);
+
+ // Allowed, and left for the server to resolve, when no row filter
applies to the principal
+ rowFilters.set(List.of());
+ BrokerResponseNative response = (BrokerResponseNative)
handler.handleRequest(query);
+ assertFalse(response.getRLSFiltersApplied(), function);
+ assertNotNull(serverRequest.get(), function);
+
assertTrue(BaseSingleStageBrokerRequestHandler.hasCrossTableFunction(serverRequest.get().getPinotQuery()),
+ function);
+ }
+ }
+
+ /// A handler that records the request it would send to the servers instead
of sending it.
+ private static BaseSingleStageBrokerRequestHandler
newServerRequestCapturingHandler(PinotConfiguration config,
+ BrokerRoutingManager routingManager, AccessControlFactory
accessControlFactory,
+ QueryQuotaManager queryQuotaManager, TableCache tableCache,
AtomicReference<BrokerRequest> serverRequest) {
+ return new BaseSingleStageBrokerRequestHandler(config, "testBrokerId", new
BrokerRequestIdGenerator(),
+ routingManager, accessControlFactory, queryQuotaManager, tableCache,
ThreadAccountantUtils.getNoOpAccountant(),
+ null, null) {
+ @Override
+ public void start() {
+ }
+
+ @Override
+ public void shutDown() {
+ }
+
+ @Override
+ protected BrokerResponseNative processBrokerRequest(long requestId,
BrokerRequest originalBrokerRequest,
+ BrokerRequest serverBrokerRequest, TableRouteInfo route, long
timeoutMs, ServerStats serverStats,
+ RequestContext requestContext) {
+ serverRequest.set(serverBrokerRequest);
+ return BrokerResponseNative.empty();
+ }
+
+ @Override
+ protected BrokerResponseNative
processMaterializedViewSplitBrokerRequest(long requestId,
+ long materializedViewRequestId, BrokerRequest originalBrokerRequest,
TableRouteInfo baseRoute,
+ TableRouteInfo materializedViewRoute, long timeoutMs, ServerStats
serverStats,
+ RequestContext requestContext) {
+ throw new UnsupportedOperationException("Not implemented in test");
+ }
+ };
+ }
+
/// Bug: FULL_REWRITE overwrites tableName to the materialized view table
name, so
/// accessControl.getRowColFilters(requesterIdentity, tableName) fetches RLS
/// policy for the materialized view table instead of the base table.
diff --git
a/pinot-common/src/main/java/org/apache/pinot/common/utils/request/RequestUtils.java
b/pinot-common/src/main/java/org/apache/pinot/common/utils/request/RequestUtils.java
index 0f913137795..6f8623cc952 100644
---
a/pinot-common/src/main/java/org/apache/pinot/common/utils/request/RequestUtils.java
+++
b/pinot-common/src/main/java/org/apache/pinot/common/utils/request/RequestUtils.java
@@ -610,6 +610,26 @@ public class RequestUtils {
return CANONICAL_NAME_TO_SPECIAL_KEY_MAP.getOrDefault(canonicalName,
canonicalName);
}
+ /// Canonical names, see [#canonicalizeFunctionName(String)], of the
functions that read rows from somewhere other
+ /// than the FROM clause of the query that calls them: `lookup()` joins a
dimension table by primary key,
+ /// `IN_SUBQUERY` runs its quoted subquery as a request of its own and
`IN_PARTITIONED_SUBQUERY` runs it on the
+ /// server. The access checks keyed on the FROM clause do not cover them, so
the single-stage broker rejects them
+ /// under row-level security, see [#isCrossTableFunction(String)].
+ ///
+ /// Nothing derives this set from the function registries, so a function
that reads another table or runs a nested
+ /// query must be added here when it is defined, and to
`DeleteStatement#CROSS_TABLE_FUNCTIONS`, which keeps the
+ /// same list in its own fold: `lookup` is `TransformFunctionType#LOOKUP`
(with `LookupTransformFunction` in
+ /// pinot-core), `insubquery` is rewritten by
`BaseSingleStageBrokerRequestHandler#IN_SUBQUERY` in pinot-broker and
+ /// `inpartitionedsubquery` by
`ServerQueryExecutorV1Impl#IN_PARTITIONED_SUBQUERY` in pinot-core.
+ private static final Set<String> CROSS_TABLE_FUNCTIONS = Set.of("lookup",
"insubquery", "inpartitionedsubquery");
+
+ /// Returns whether `canonicalName` names a function that reads rows from
somewhere other than the FROM clause, see
+ /// [#CROSS_TABLE_FUNCTIONS]. The name must already be canonical, see
[#canonicalizeFunctionName(String)], as the
+ /// parser leaves the operator of every compiled function call; a name as
written in a query is not matched.
+ public static boolean isCrossTableFunction(String canonicalName) {
+ return CROSS_TABLE_FUNCTIONS.contains(canonicalName);
+ }
+
/// Returns true iff `expression` is an `AS`-wrapped function call
/// (i.e. shaped like `expr AS alias` after Calcite parsing).
///
diff --git
a/pinot-common/src/main/java/org/apache/pinot/sql/parsers/dml/DeleteStatement.java
b/pinot-common/src/main/java/org/apache/pinot/sql/parsers/dml/DeleteStatement.java
index a2f25f4f068..c0d01d324b4 100644
---
a/pinot-common/src/main/java/org/apache/pinot/sql/parsers/dml/DeleteStatement.java
+++
b/pinot-common/src/main/java/org/apache/pinot/sql/parsers/dml/DeleteStatement.java
@@ -145,6 +145,8 @@ public class DeleteStatement implements
DataManipulationStatement {
/// here when it is defined: `lookUp` is `TransformFunctionType#LOOKUP`
(with `LookupTransformFunction` in
/// pinot-core), `inSubquery` is rewritten by
`BaseSingleStageBrokerRequestHandler#IN_SUBQUERY` in pinot-broker, and
/// `inPartitionedSubquery` by
`ServerQueryExecutorV1Impl#IN_PARTITIONED_SUBQUERY` in pinot-core.
+ /// `RequestUtils#isCrossTableFunction(String)` keeps the same list in lower
case for the single-stage broker's
+ /// row-level security check; add the function there too.
private static final Set<String> CROSS_TABLE_FUNCTIONS = Set.of("LOOKUP",
"INSUBQUERY", "INPARTITIONEDSUBQUERY");
/// Canonical names of the functions that run a script. Rejected because the
broker's groovy policy
diff --git
a/pinot-common/src/test/java/org/apache/pinot/common/utils/request/RequestUtilsTest.java
b/pinot-common/src/test/java/org/apache/pinot/common/utils/request/RequestUtilsTest.java
index a65148b3790..f151815bfe8 100644
---
a/pinot-common/src/test/java/org/apache/pinot/common/utils/request/RequestUtilsTest.java
+++
b/pinot-common/src/test/java/org/apache/pinot/common/utils/request/RequestUtilsTest.java
@@ -267,6 +267,25 @@ public class RequestUtilsTest {
// ---------- helpers ----------
+ @Test
+ public void testIsCrossTableFunction() {
+ // The names the parser leaves on a compiled call, see
RequestUtils.canonicalizeFunctionName()
+ String[] spellings = {
+ "LOOKUP", "look_Up", "IN_SUBQUERY", "inSubquery",
"IN_PARTITIONED_SUBQUERY", "In_Partitioned_Sub_Query"
+ };
+ for (String spelling : spellings) {
+
assertTrue(RequestUtils.isCrossTableFunction(RequestUtils.canonicalizeFunctionName(spelling)),
spelling);
+ }
+ // The input must already be canonical: a name as written in a query is
not matched
+ assertFalse(RequestUtils.isCrossTableFunction("IN_SUBQUERY"));
+ assertFalse(RequestUtils.isCrossTableFunction("inSubquery"));
+ // Other functions, IN_ID_SET on a literal included
+ String[] others = {"inidset", "idset", "lookups", "subquery",
"inpartitionedsubquery2", "as", ""};
+ for (String other : others) {
+ assertFalse(RequestUtils.isCrossTableFunction(other), other);
+ }
+ }
+
private static Expression sumOf(String column) {
Function func = new Function("sum");
func.setOperands(List.of(RequestUtils.getIdentifierExpression(column)));
diff --git
a/pinot-core/src/main/java/org/apache/pinot/core/query/executor/ServerQueryExecutorV1Impl.java
b/pinot-core/src/main/java/org/apache/pinot/core/query/executor/ServerQueryExecutorV1Impl.java
index e0625491dd9..eacca5a1b9f 100644
---
a/pinot-core/src/main/java/org/apache/pinot/core/query/executor/ServerQueryExecutorV1Impl.java
+++
b/pinot-core/src/main/java/org/apache/pinot/core/query/executor/ServerQueryExecutorV1Impl.java
@@ -90,6 +90,7 @@ import org.apache.pinot.spi.trace.Tracer;
import org.apache.pinot.spi.trace.Tracing;
import org.apache.pinot.spi.utils.CommonConstants.Accounting.ScanKillingMode;
import org.apache.pinot.spi.utils.CommonConstants.Server;
+import org.apache.pinot.sql.parsers.rewriter.RlsUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -511,7 +512,7 @@ public class ServerQueryExecutorV1Impl implements
QueryExecutor {
throws Exception {
FilterContext filter = queryContext.getFilter();
if (filter != null && !filter.isConstant()) {
- handleSubquery(filter, tableExecutionInfo, timerContext,
executorService, queryContext.getEndTimeMs());
+ handleSubquery(filter, tableExecutionInfo, timerContext,
executorService, queryContext);
}
}
@@ -519,15 +520,15 @@ public class ServerQueryExecutorV1Impl implements
QueryExecutor {
///
/// Currently only supports subquery within the lhs of the predicate.
private void handleSubquery(FilterContext filter, TableExecutionInfo
executionInfo, TimerContext timerContext,
- ExecutorService executorService, long endTimeMs)
+ ExecutorService executorService, QueryContext queryContext)
throws Exception {
List<FilterContext> children = filter.getChildren();
if (children != null) {
for (FilterContext child : children) {
- handleSubquery(child, executionInfo, timerContext, executorService,
endTimeMs);
+ handleSubquery(child, executionInfo, timerContext, executorService,
queryContext);
}
} else {
- handleSubquery(filter.getPredicate().getLhs(), executionInfo,
timerContext, executorService, endTimeMs);
+ handleSubquery(filter.getPredicate().getLhs(), executionInfo,
timerContext, executorService, queryContext);
}
}
@@ -537,9 +538,10 @@ public class ServerQueryExecutorV1Impl implements
QueryExecutor {
/// rewrites the expression with the subquery response.
///
/// Currently only supports ID_SET subquery within the
IN_PARTITIONED_SUBQUERY transform function, which will be
- /// rewritten to an IN_ID_SET transform function.
+ /// rewritten to an IN_ID_SET transform function. The function is not
allowed under row-level security: the broker
+ /// rejects it, and a server behind an older broker fails the query.
private void handleSubquery(ExpressionContext expression, TableExecutionInfo
executionInfo, TimerContext timerContext,
- ExecutorService executorService, long endTimeMs)
+ ExecutorService executorService, QueryContext queryContext)
throws Exception {
FunctionContext function = expression.getFunction();
if (function == null) {
@@ -547,6 +549,12 @@ public class ServerQueryExecutorV1Impl implements
QueryExecutor {
}
List<ExpressionContext> arguments = function.getArguments();
if (StringUtils.remove(function.getFunctionName(),
'_').equalsIgnoreCase(IN_PARTITIONED_SUBQUERY)) {
+ // Reject under RLS even when an older broker allows the function
through: the subquery is an opaque string
+ // literal the row filters stamped in the options do not reach, so it
would build its ID_SET from every row
+ Map<String, String> rowFilters =
RlsUtils.extractRlsFilters(queryContext.getQueryOptions());
+ Preconditions.checkState(rowFilters.isEmpty(),
+ "IN_PARTITIONED_SUBQUERY is not allowed when row-level security
filters apply, got filters for %s",
+ rowFilters.keySet());
Preconditions.checkArgument(arguments.size() == 2,
"IN_PARTITIONED_SUBQUERY requires 2 arguments: expression,
subquery");
ExpressionContext subqueryExpression = arguments.get(1);
@@ -563,7 +571,7 @@ public class ServerQueryExecutorV1Impl implements
QueryExecutor {
"Subquery in IN_PARTITIONED_SUBQUERY should be an ID_SET aggregation
only query, found: %s",
subqueryExpression.toString());
// Execute the subquery
- subquery.setEndTimeMs(endTimeMs);
+ subquery.setEndTimeMs(queryContext.getEndTimeMs());
// Make a clone of indexSegments because the method might modify the list
InstanceResponseBlock instanceResponse =
executeInternal(executionInfo, subquery, timerContext,
executorService, null, false, null);
@@ -579,7 +587,7 @@ public class ServerQueryExecutorV1Impl implements
QueryExecutor {
arguments.set(1,
ExpressionContext.forLiteral(RequestUtils.getLiteral(((IdSet)
result).toBase64String())));
} else {
for (ExpressionContext argument : arguments) {
- handleSubquery(argument, executionInfo, timerContext, executorService,
endTimeMs);
+ handleSubquery(argument, executionInfo, timerContext, executorService,
queryContext);
}
}
}
diff --git
a/pinot-core/src/test/java/org/apache/pinot/core/query/executor/QueryExecutorTest.java
b/pinot-core/src/test/java/org/apache/pinot/core/query/executor/QueryExecutorTest.java
index 61965252a8a..205b756ff49 100644
---
a/pinot-core/src/test/java/org/apache/pinot/core/query/executor/QueryExecutorTest.java
+++
b/pinot-core/src/test/java/org/apache/pinot/core/query/executor/QueryExecutorTest.java
@@ -35,6 +35,7 @@ import java.util.concurrent.Executors;
import java.util.concurrent.Future;
import java.util.concurrent.TimeUnit;
import java.util.stream.Collectors;
+import javax.annotation.Nullable;
import org.apache.commons.configuration2.PropertiesConfiguration;
import org.apache.commons.io.FileUtils;
import org.apache.helix.HelixManager;
@@ -78,6 +79,7 @@ import org.apache.pinot.spi.utils.ReadMode;
import org.apache.pinot.spi.utils.builder.TableConfigBuilder;
import org.apache.pinot.spi.utils.builder.TableNameBuilder;
import org.apache.pinot.sql.parsers.CalciteSqlCompiler;
+import org.apache.pinot.sql.parsers.rewriter.RlsUtils;
import org.apache.pinot.tsdb.spi.AggInfo;
import org.apache.pinot.tsdb.spi.TimeBuckets;
import org.apache.pinot.tsdb.spi.series.SimpleTimeSeriesBuilderFactory;
@@ -231,6 +233,48 @@ public class QueryExecutorTest {
assertEquals(((AggregationResultsBlock)
instanceResponse.getResultsBlock()).getResults().get(0), 0.0);
}
+ /// The broker rejects IN_PARTITIONED_SUBQUERY for a principal with row
filters. A server behind an older broker,
+ /// which stamps the row filters in the query options but lets the function
through, fails the query instead: the
+ /// subquery reaches it as a string literal the filters do not cover, so it
would build its ID_SET from every row.
+ @Test
+ public void testInPartitionedSubqueryRejectedUnderRowFilters() {
+ String query = "SELECT COUNT(*) FROM " + OFFLINE_TABLE_NAME + " WHERE
IN_PARTITIONED_SUBQUERY(orderItemCount, "
+ + "'SELECT ID_SET(orderItemCount) FROM " + OFFLINE_TABLE_NAME + "') =
1";
+ // Without row filters the function resolves as it always has: every row's
value is in the ID_SET
+ long numRows = (Long) aggregate("SELECT COUNT(*) FROM " +
OFFLINE_TABLE_NAME, null);
+ assertEquals(aggregate(query, null), numRows);
+
+ // With row filters stamped for any table the query fails before the
subquery runs
+ for (String table : new String[]{RAW_TABLE_NAME, "otherTable"}) {
+ Map<String, String> queryOptions =
Map.of(RlsUtils.buildRlsFilterKey(table), "orderItemCount > 0");
+ String error = executeExpectingFailure(query, queryOptions);
+ assertTrue(error.contains("not allowed when row-level security filters
apply"), error);
+ }
+ }
+
+ private Object aggregate(String query, @Nullable Map<String, String>
queryOptions) {
+ InstanceResponseBlock instanceResponse = execute(newInstanceRequest(query,
queryOptions));
+ assertTrue(instanceResponse.getExceptions().isEmpty(),
instanceResponse.getExceptions().toString());
+ assertTrue(instanceResponse.getResultsBlock() instanceof
AggregationResultsBlock);
+ return ((AggregationResultsBlock)
instanceResponse.getResultsBlock()).getResults().get(0);
+ }
+
+ /// Runs the query and returns the text of the exceptions it fails with.
+ private String executeExpectingFailure(String query, @Nullable Map<String,
String> queryOptions) {
+ InstanceResponseBlock instanceResponse = execute(newInstanceRequest(query,
queryOptions));
+ assertFalse(instanceResponse.getExceptions().isEmpty(), "Expected the
query to fail");
+ return String.join("\n", instanceResponse.getExceptions().values());
+ }
+
+ private InstanceRequest newInstanceRequest(String query, @Nullable
Map<String, String> queryOptions) {
+ InstanceRequest instanceRequest = new InstanceRequest(0L,
CalciteSqlCompiler.compileToBrokerRequest(query));
+ if (queryOptions != null) {
+ instanceRequest.getQuery().getPinotQuery().setQueryOptions(new
HashMap<>(queryOptions));
+ }
+ instanceRequest.setSearchSegments(_segmentNames);
+ return instanceRequest;
+ }
+
@Test
public void testNoRemoteAccessStatsWithoutRemoteAccesses() {
String query = "SELECT COUNT(*) FROM " + OFFLINE_TABLE_NAME;
diff --git
a/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/RowLevelSecurityIntegrationTest.java
b/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/RowLevelSecurityIntegrationTest.java
index 663c528d14e..539074177c5 100644
---
a/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/RowLevelSecurityIntegrationTest.java
+++
b/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/RowLevelSecurityIntegrationTest.java
@@ -41,8 +41,10 @@ import static
org.apache.pinot.integration.tests.BasicAuthTestUtils.AUTH_HEADER;
import static
org.apache.pinot.integration.tests.BasicAuthTestUtils.AUTH_HEADER_USER;
import static org.apache.pinot.integration.tests.BasicAuthTestUtils.AUTH_TOKEN;
import static
org.apache.pinot.integration.tests.ClusterIntegrationTestUtils.getBrokerQueryApiUrl;
+import static org.testng.Assert.assertEquals;
import static org.testng.Assert.assertFalse;
import static org.testng.Assert.assertTrue;
+import static org.testng.Assert.fail;
public class RowLevelSecurityIntegrationTest extends
BaseClusterIntegrationTest {
@@ -278,6 +280,36 @@ public class RowLevelSecurityIntegrationTest extends
BaseClusterIntegrationTest
queryBroker(multiStageQuery, AUTH_HEADER_USER_2)));
}
+ /// The row filters are added to the WHERE clause of the queried table,
which no cross-table function reads through:
+ /// the server resolves IN_PARTITIONED_SUBQUERY by running the quoted
subquery on the segments of the table, where
+ /// the filters do not reach it, so the subquery below would see the rows of
every other airline. The broker rejects
+ /// every cross-table function for a principal with row filters, IN_SUBQUERY
included although its nested request
+ /// is filtered on its own.
+ @Test
+ public void testCrossTableFunctionsRejectedForPrincipalWithRowFilters()
+ throws Exception {
+ setUseMultiStageQueryEngine(false);
+ long numRows = queryBroker("SELECT COUNT(*) FROM mytable",
AUTH_HEADER).get("resultTable").get("rows").get(0)
+ .get(0).asLong();
+ for (String function : new String[]{"IN_PARTITIONED_SUBQUERY",
"IN_SUBQUERY"}) {
+ String query = "SELECT COUNT(*) FROM mytable WHERE " + function +
"(AirlineID - AirlineID, "
+ + "'SELECT ID_SET(AirlineID - AirlineID) FROM mytable WHERE
AirlineID <> 19805') = 1";
+
+ // A principal without row filters keeps the function
+ JsonNode adminResponse = queryBroker(query, AUTH_HEADER);
+ assertFalse(adminResponse.get("rlsFiltersApplied").asBoolean(),
function);
+
assertEquals(adminResponse.get("resultTable").get("rows").get(0).get(0).asLong(),
numRows, function);
+
+ // A principal with row filters is rejected
+ try {
+ JsonNode response = queryBroker(query, AUTH_HEADER_USER);
+ fail("Expected " + function + " to be rejected, got: " + response);
+ } catch (IOException e) {
+ assertTrue(e.getMessage().contains("Cross-table functions are not
allowed"), e.getMessage());
+ }
+ }
+ }
+
private JsonNode queryBroker(String query, Map<String, String> headers)
throws Exception {
JsonNode response =
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
