xiangfu0 commented on code in PR #19664:
URL: https://github.com/apache/pinot/pull/19664#discussion_r4244659757
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java:
##########
@@ -424,6 +445,116 @@ private StreamingOutput executeSqlQuery(@Context
HttpHeaders httpHeaders, String
}
}
+ /// Executes a `DELETE` once the caller is authorized to delete rows from
its table, see [#authorizeDelete].
+ ///
+ /// The caller is first checked without a table (the `READ` access type with
the `/sql` endpoint, as for a
+ /// multi-stage query), before the statement is parsed and its table looked
up: with basic auth, an unauthenticated
+ /// caller gets HTTP 401 here, before the table cache is consulted. The
table is then resolved, with the database of
+ /// the request and in the case it is defined with, and the caller
authorized to delete rows from the resolved
+ /// table. Only then is the table checked not to be a logical table and to
exist, and every error message names the
+ /// table as the caller wrote it, so that a caller who is not authorized for
a table learns neither whether it exists
+ /// nor the case it is defined with, nor whether the name is a logical
table. The executor deletes rows from the
+ /// exact resolved table.
+ ///
+ /// The executor runs before the response streams, so that an exception it
lets escape is mapped to an error
+ /// response by [#executeSqlQueryCatching] rather than failing the request
with HTTP 500.
+ ///
+ /// @throws QueryException with [QueryErrorCode#ACCESS_DENIED] if the caller
is not authorized, with
+ /// [QueryErrorCode#QUERY_VALIDATION] if the table is
a logical table, with
+ /// [QueryErrorCode#TABLE_DOES_NOT_EXIST] if the
table does not exist, and with the errors of
+ /// [DataManipulationStatementParser#parse] and
[DeleteStatement#resolveTableName]
+ private StreamingOutput executeDelete(SqlNodeAndOptions sqlNodeAndOptions,
HttpHeaders httpHeaders) {
+ AccessControl accessControl = _accessControlFactory.create();
+ if (!accessControl.hasAccess(AccessType.READ, httpHeaders, SQL_ENDPOINT)) {
+ throw QueryErrorCode.ACCESS_DENIED.asException("Permission denied to
delete rows");
+ }
+ DeleteStatement parsed = (DeleteStatement)
DataManipulationStatementParser.parse(sqlNodeAndOptions);
+ DeleteStatement statement =
parsed.resolveTableName(httpHeaders.getHeaderString(CommonConstants.DATABASE),
+ _pinotHelixResourceManager.getTableCache());
+ // Authorized on the resolved table, reported with the name as written
+ String writtenTableName = parsed.getTableName();
+ authorizeDelete(accessControl, statement.getTableName(), writtenTableName,
httpHeaders);
+ if (statement.isLogicalTable()) {
+ throw QueryErrorCode.QUERY_VALIDATION.asException("DELETE does not
support logical tables: " + writtenTableName);
+ }
+ if (!statement.tableExists()) {
+ throw QueryErrorCode.TABLE_DOES_NOT_EXIST.asException("Table does not
exist: " + writtenTableName);
+ }
+ LOGGER.info(deleteLogMessage(statement, httpHeaders));
+ BrokerResponse response = _sqlQueryExecutor.executeStatement(statement,
extractHeaders(httpHeaders));
+ return output -> {
+ try (OutputStream os = output) {
+ response.toOutputStream(os);
+ }
+ };
+ }
+
+ /// Formats the log line of a `DELETE` the executor is about to run: the
resolved table, the predicate, the option
+ /// keys and the client of the request, in the format of the broker's line
so that it parses the same way in the
+ /// logs of both roles. The predicate, the option keys and the client are
kept on a single line (see
+ /// [#toSingleLine]), so that a line ending in the statement, in a request
option or in a proxy header cannot forge
+ /// a log record.
+ @VisibleForTesting
+ static String deleteLogMessage(DeleteStatement statement, HttpHeaders
httpHeaders) {
+ return "Executing DELETE on table: " + statement.getTableName() + ",
predicate: "
+ + toSingleLine(statement.getPredicate()) + ", options: "
+ + toSingleLine(statement.getOptions().keySet().toString()) + ",
client: "
+ + toSingleLine(getClientIp(httpHeaders));
+ }
+
+ /// Client of the request for the log: the `X-Forwarded-For` header, else
the `X-Real-IP` header, else
+ /// [CommonConstants#UNKNOWN]. The addresses of a comma-separated
`X-Forwarded-For` value are joined with `;`, the
+ /// format the broker's `HttpRequesterIdentity#getClientIp` logs, so that
the field parses the same way in the logs
+ /// of both roles. Both headers are set by the caller unless a proxy
overwrites them.
+ private static String getClientIp(HttpHeaders httpHeaders) {
+ String forwardedFor = httpHeaders.getHeaderString("X-Forwarded-For");
+ if (forwardedFor != null) {
+ return forwardedFor.replace(',', ';');
+ }
+ String realIp = httpHeaders.getHeaderString("X-Real-IP");
+ return realIp != null ? realIp : CommonConstants.UNKNOWN;
+ }
+
+ /// Escapes backslashes, CR and LF as `\\`, `\r` and `\n`, as the broker's
query log does, so that a value the
+ /// caller controls (a string literal or a quoted identifier of the
predicate, an option key from `SET`, the legacy
+ /// `OPTION(...)` suffix or the request `queryOptions`, a proxy header)
cannot split a log record or append a line
+ /// that looks like a log entry.
+ private static String toSingleLine(String value) {
+ if (value.indexOf('\\') < 0 && value.indexOf('\n') < 0 &&
value.indexOf('\r') < 0) {
+ return value;
+ }
+ return value.replace("\\", "\\\\").replace("\r", "\\r").replace("\n",
"\\n");
+ }
+
+ /// Authorizes the caller, who passed the caller-level check of
[#executeDelete], to delete rows from the table.
+ ///
+ /// It checks, on the raw table name and in this order, the `READ` access
type with the `/sql` endpoint (the request
+ /// path, as the authentication filter passes for the other endpoints), the
fine-grained [Actions.Table#QUERY]
+ /// action, the `DELETE` access type with the `/sql` endpoint and the
fine-grained [Actions.Table#DELETE_ROWS]
+ /// action. The `READ` and `QUERY` checks are there because the WHERE clause
reads the table, the `DELETE` and
+ /// `DELETE_ROWS` ones because rows are deleted, as the other deletions of
the controller (e.g. of segments) check
+ /// `DELETE` on the raw table name. This is stricter than the controller's
query path, which only checks the `READ`
+ /// access type with `"Query"` as the endpoint and no fine-grained action,
and it does not apply the row-level
+ /// security of the brokers. Unlike the broker, which has no access type to
tell a deletion from a query and denies
+ /// it by default, an access control that does not tell access types apart
and allows every fine-grained action
+ /// lets every reader of the table delete rows, as it lets them delete
segments.
+ ///
+ /// @param tableName resolved table name the caller is authorized on
+ /// @param writtenTableName table name as the caller wrote it, which the
denial names so that the denial does not
+ /// reveal the case the table is defined with (i.e.
that it exists)
+ /// @throws QueryException with [QueryErrorCode#ACCESS_DENIED] if the caller
is not authorized, as for queries
+ private void authorizeDelete(AccessControl accessControl, String tableName,
String writtenTableName,
+ HttpHeaders httpHeaders) {
+ String rawTableName = TableNameBuilder.extractRawTableName(tableName);
+ if (!accessControl.hasAccess(rawTableName, AccessType.READ, httpHeaders,
SQL_ENDPOINT)
+ || !accessControl.hasAccess(httpHeaders, TargetType.TABLE,
rawTableName, Actions.Table.QUERY)
+ || !accessControl.hasAccess(rawTableName, AccessType.DELETE,
httpHeaders, SQL_ENDPOINT)
+ || !accessControl.hasAccess(httpHeaders, TargetType.TABLE,
rawTableName, Actions.Table.DELETE_ROWS)) {
Review Comment:
**[CONFIRMED — authorization asymmetry] The controller `/sql` DELETE path
applies no row-level security, so a broker RLS refusal is bypassable here.**
The broker denies a DELETE when RLS filters apply to the table, but this
path checks only `READ`/`QUERY`/`DELETE`/`DELETE_ROWS` (the controller
`AccessControl` has no RLS surface). A principal whose DELETE the broker
refuses with 403 "Row-level security applies to the table" can POST the
identical statement to the controller `/sql` and have it executed — deleting
rows outside their row scope. Note `BasicAuthPrincipal.hasPermission` grants
all permissions to a principal with no permission list, so the same credentials
plausibly pass here.
The javadoc acknowledges the gap ("does not apply the row-level security of
the brokers"), but since both endpoints reach the same executor, consider
denying DELETE on the controller when the deployment uses broker RLS (e.g. a
controller-side opt-out config), or documenting loudly that RLS-protected
clusters must not install a deleting executor on the controller.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java:
##########
@@ -424,6 +445,116 @@ private StreamingOutput executeSqlQuery(@Context
HttpHeaders httpHeaders, String
}
}
+ /// Executes a `DELETE` once the caller is authorized to delete rows from
its table, see [#authorizeDelete].
+ ///
+ /// The caller is first checked without a table (the `READ` access type with
the `/sql` endpoint, as for a
+ /// multi-stage query), before the statement is parsed and its table looked
up: with basic auth, an unauthenticated
+ /// caller gets HTTP 401 here, before the table cache is consulted. The
table is then resolved, with the database of
+ /// the request and in the case it is defined with, and the caller
authorized to delete rows from the resolved
+ /// table. Only then is the table checked not to be a logical table and to
exist, and every error message names the
+ /// table as the caller wrote it, so that a caller who is not authorized for
a table learns neither whether it exists
+ /// nor the case it is defined with, nor whether the name is a logical
table. The executor deletes rows from the
+ /// exact resolved table.
+ ///
+ /// The executor runs before the response streams, so that an exception it
lets escape is mapped to an error
+ /// response by [#executeSqlQueryCatching] rather than failing the request
with HTTP 500.
+ ///
+ /// @throws QueryException with [QueryErrorCode#ACCESS_DENIED] if the caller
is not authorized, with
+ /// [QueryErrorCode#QUERY_VALIDATION] if the table is
a logical table, with
+ /// [QueryErrorCode#TABLE_DOES_NOT_EXIST] if the
table does not exist, and with the errors of
+ /// [DataManipulationStatementParser#parse] and
[DeleteStatement#resolveTableName]
+ private StreamingOutput executeDelete(SqlNodeAndOptions sqlNodeAndOptions,
HttpHeaders httpHeaders) {
+ AccessControl accessControl = _accessControlFactory.create();
+ if (!accessControl.hasAccess(AccessType.READ, httpHeaders, SQL_ENDPOINT)) {
+ throw QueryErrorCode.ACCESS_DENIED.asException("Permission denied to
delete rows");
+ }
+ DeleteStatement parsed = (DeleteStatement)
DataManipulationStatementParser.parse(sqlNodeAndOptions);
+ DeleteStatement statement =
parsed.resolveTableName(httpHeaders.getHeaderString(CommonConstants.DATABASE),
+ _pinotHelixResourceManager.getTableCache());
+ // Authorized on the resolved table, reported with the name as written
+ String writtenTableName = parsed.getTableName();
+ authorizeDelete(accessControl, statement.getTableName(), writtenTableName,
httpHeaders);
+ if (statement.isLogicalTable()) {
+ throw QueryErrorCode.QUERY_VALIDATION.asException("DELETE does not
support logical tables: " + writtenTableName);
+ }
+ if (!statement.tableExists()) {
+ throw QueryErrorCode.TABLE_DOES_NOT_EXIST.asException("Table does not
exist: " + writtenTableName);
+ }
+ LOGGER.info(deleteLogMessage(statement, httpHeaders));
+ BrokerResponse response = _sqlQueryExecutor.executeStatement(statement,
extractHeaders(httpHeaders));
+ return output -> {
+ try (OutputStream os = output) {
+ response.toOutputStream(os);
+ }
+ };
+ }
+
+ /// Formats the log line of a `DELETE` the executor is about to run: the
resolved table, the predicate, the option
+ /// keys and the client of the request, in the format of the broker's line
so that it parses the same way in the
+ /// logs of both roles. The predicate, the option keys and the client are
kept on a single line (see
+ /// [#toSingleLine]), so that a line ending in the statement, in a request
option or in a proxy header cannot forge
+ /// a log record.
+ @VisibleForTesting
+ static String deleteLogMessage(DeleteStatement statement, HttpHeaders
httpHeaders) {
+ return "Executing DELETE on table: " + statement.getTableName() + ",
predicate: "
+ + toSingleLine(statement.getPredicate()) + ", options: "
+ + toSingleLine(statement.getOptions().keySet().toString()) + ",
client: "
+ + toSingleLine(getClientIp(httpHeaders));
+ }
+
+ /// Client of the request for the log: the `X-Forwarded-For` header, else
the `X-Real-IP` header, else
+ /// [CommonConstants#UNKNOWN]. The addresses of a comma-separated
`X-Forwarded-For` value are joined with `;`, the
+ /// format the broker's `HttpRequesterIdentity#getClientIp` logs, so that
the field parses the same way in the logs
+ /// of both roles. Both headers are set by the caller unless a proxy
overwrites them.
+ private static String getClientIp(HttpHeaders httpHeaders) {
+ String forwardedFor = httpHeaders.getHeaderString("X-Forwarded-For");
+ if (forwardedFor != null) {
+ return forwardedFor.replace(',', ';');
+ }
+ String realIp = httpHeaders.getHeaderString("X-Real-IP");
+ return realIp != null ? realIp : CommonConstants.UNKNOWN;
+ }
+
+ /// Escapes backslashes, CR and LF as `\\`, `\r` and `\n`, as the broker's
query log does, so that a value the
+ /// caller controls (a string literal or a quoted identifier of the
predicate, an option key from `SET`, the legacy
+ /// `OPTION(...)` suffix or the request `queryOptions`, a proxy header)
cannot split a log record or append a line
+ /// that looks like a log entry.
+ private static String toSingleLine(String value) {
+ if (value.indexOf('\\') < 0 && value.indexOf('\n') < 0 &&
value.indexOf('\r') < 0) {
+ return value;
+ }
+ return value.replace("\\", "\\\\").replace("\r", "\\r").replace("\n",
"\\n");
+ }
+
+ /// Authorizes the caller, who passed the caller-level check of
[#executeDelete], to delete rows from the table.
+ ///
+ /// It checks, on the raw table name and in this order, the `READ` access
type with the `/sql` endpoint (the request
+ /// path, as the authentication filter passes for the other endpoints), the
fine-grained [Actions.Table#QUERY]
+ /// action, the `DELETE` access type with the `/sql` endpoint and the
fine-grained [Actions.Table#DELETE_ROWS]
+ /// action. The `READ` and `QUERY` checks are there because the WHERE clause
reads the table, the `DELETE` and
+ /// `DELETE_ROWS` ones because rows are deleted, as the other deletions of
the controller (e.g. of segments) check
+ /// `DELETE` on the raw table name. This is stricter than the controller's
query path, which only checks the `READ`
+ /// access type with `"Query"` as the endpoint and no fine-grained action,
and it does not apply the row-level
+ /// security of the brokers. Unlike the broker, which has no access type to
tell a deletion from a query and denies
+ /// it by default, an access control that does not tell access types apart
and allows every fine-grained action
+ /// lets every reader of the table delete rows, as it lets them delete
segments.
+ ///
+ /// @param tableName resolved table name the caller is authorized on
+ /// @param writtenTableName table name as the caller wrote it, which the
denial names so that the denial does not
+ /// reveal the case the table is defined with (i.e.
that it exists)
+ /// @throws QueryException with [QueryErrorCode#ACCESS_DENIED] if the caller
is not authorized, as for queries
+ private void authorizeDelete(AccessControl accessControl, String tableName,
String writtenTableName,
+ HttpHeaders httpHeaders) {
+ String rawTableName = TableNameBuilder.extractRawTableName(tableName);
Review Comment:
**[CONFIRMED — authorization asymmetry] Fine-grained rules keyed on typed
table names are never consulted on the controller DELETE path.**
All four checks here run on `extractRawTableName(tableName)` only, while the
broker checks QUERY/DELETE_ROWS on the resolved (possibly typed) name *and*
DELETE_ROWS on the raw name "so that a type suffix cannot bypass a rule on the
raw name". The inverse bypass remains here: a `FineGrainedAccessControl`
denying `DeleteRows` on `mytable_OFFLINE` blocks the DELETE on the broker but
not on the controller. Consider checking the resolved name as well when it
differs from the raw name, matching the broker.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java:
##########
@@ -424,6 +445,116 @@ private StreamingOutput executeSqlQuery(@Context
HttpHeaders httpHeaders, String
}
}
+ /// Executes a `DELETE` once the caller is authorized to delete rows from
its table, see [#authorizeDelete].
+ ///
+ /// The caller is first checked without a table (the `READ` access type with
the `/sql` endpoint, as for a
+ /// multi-stage query), before the statement is parsed and its table looked
up: with basic auth, an unauthenticated
+ /// caller gets HTTP 401 here, before the table cache is consulted. The
table is then resolved, with the database of
+ /// the request and in the case it is defined with, and the caller
authorized to delete rows from the resolved
+ /// table. Only then is the table checked not to be a logical table and to
exist, and every error message names the
+ /// table as the caller wrote it, so that a caller who is not authorized for
a table learns neither whether it exists
+ /// nor the case it is defined with, nor whether the name is a logical
table. The executor deletes rows from the
+ /// exact resolved table.
+ ///
+ /// The executor runs before the response streams, so that an exception it
lets escape is mapped to an error
+ /// response by [#executeSqlQueryCatching] rather than failing the request
with HTTP 500.
+ ///
+ /// @throws QueryException with [QueryErrorCode#ACCESS_DENIED] if the caller
is not authorized, with
+ /// [QueryErrorCode#QUERY_VALIDATION] if the table is
a logical table, with
+ /// [QueryErrorCode#TABLE_DOES_NOT_EXIST] if the
table does not exist, and with the errors of
+ /// [DataManipulationStatementParser#parse] and
[DeleteStatement#resolveTableName]
+ private StreamingOutput executeDelete(SqlNodeAndOptions sqlNodeAndOptions,
HttpHeaders httpHeaders) {
+ AccessControl accessControl = _accessControlFactory.create();
+ if (!accessControl.hasAccess(AccessType.READ, httpHeaders, SQL_ENDPOINT)) {
+ throw QueryErrorCode.ACCESS_DENIED.asException("Permission denied to
delete rows");
+ }
+ DeleteStatement parsed = (DeleteStatement)
DataManipulationStatementParser.parse(sqlNodeAndOptions);
+ DeleteStatement statement =
parsed.resolveTableName(httpHeaders.getHeaderString(CommonConstants.DATABASE),
+ _pinotHelixResourceManager.getTableCache());
+ // Authorized on the resolved table, reported with the name as written
+ String writtenTableName = parsed.getTableName();
+ authorizeDelete(accessControl, statement.getTableName(), writtenTableName,
httpHeaders);
+ if (statement.isLogicalTable()) {
+ throw QueryErrorCode.QUERY_VALIDATION.asException("DELETE does not
support logical tables: " + writtenTableName);
+ }
+ if (!statement.tableExists()) {
+ throw QueryErrorCode.TABLE_DOES_NOT_EXIST.asException("Table does not
exist: " + writtenTableName);
+ }
+ LOGGER.info(deleteLogMessage(statement, httpHeaders));
+ BrokerResponse response = _sqlQueryExecutor.executeStatement(statement,
extractHeaders(httpHeaders));
+ return output -> {
+ try (OutputStream os = output) {
+ response.toOutputStream(os);
+ }
+ };
+ }
+
+ /// Formats the log line of a `DELETE` the executor is about to run: the
resolved table, the predicate, the option
+ /// keys and the client of the request, in the format of the broker's line
so that it parses the same way in the
+ /// logs of both roles. The predicate, the option keys and the client are
kept on a single line (see
+ /// [#toSingleLine]), so that a line ending in the statement, in a request
option or in a proxy header cannot forge
+ /// a log record.
+ @VisibleForTesting
+ static String deleteLogMessage(DeleteStatement statement, HttpHeaders
httpHeaders) {
+ return "Executing DELETE on table: " + statement.getTableName() + ",
predicate: "
Review Comment:
**[CONFIRMED — privacy] The controller logs the DELETE predicate verbatim,
with no redaction.**
The broker's `deleteLogMessage` applies
`pinot.broker.query.log.sqlRedaction` precisely because "its literals are the
very values a privacy-driven delete removes" — that rationale applies
identically here. On a redaction-configured cluster, `DELETE FROM users WHERE
email = '[email protected]'` POSTed to controller `/sql` writes the PII literal
into the controller log while the broker logs `REDACTED`. There is currently no
controller-side redaction config at all, so consider reading the same
broker-namespaced key (as the legacy-syntax mode listener already does) or at
minimum documenting the gap.
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotClientRequest.java:
##########
@@ -703,7 +726,13 @@ private BrokerResponse executeSqlQuery(ObjectNode
sqlRequestJson, HttpRequesterI
Map<String, String> headers = new HashMap<>();
httpRequesterIdentity.getHttpHeaders().entries()
.forEach(entry -> headers.put(entry.getKey(), entry.getValue()));
+ if (sqlNodeAndOptions.getSqlNode() instanceof SqlDelete) {
+ return executeDelete(sqlNodeAndOptions, headers,
httpRequesterIdentity, httpHeaders);
Review Comment:
**[CONFIRMED] A DELETE sent through the cursor endpoint (`getCursor=true`)
is silently accepted with bogus cursor options.**
The `getCursor` block earlier in `executeSqlQuery` runs before the
statement-type switch: it calls `setExtraOptions({getCursor:true,
cursorNumRows:N})` and increments `CURSOR_QUERIES_GLOBAL`, then the DML branch
executes the DELETE anyway. The executor receives `getCursor`/`cursorNumRows`
in `DeleteStatement.getOptions()`, the client gets a plain non-cursor response
(its follow-up cursor fetch fails), and cursor metrics are skewed —
contradicting this PR's own principle that DML options must fail loudly rather
than be silently misapplied. Suggested fix: reject non-DQL statements when
`getCursor` is true (mirroring the `onlyDql` guard).
##########
pinot-broker/src/main/java/org/apache/pinot/broker/broker/BasicAuthAccessControlFactory.java:
##########
@@ -133,6 +144,28 @@ public TableAuthorizationResult
authorize(RequesterIdentity requesterIdentity, S
return new TableAuthorizationResult(failedTables);
}
+ @Override
+ public AuthorizationResult authorizeDeleteRows(RequesterIdentity
requesterIdentity,
+ @Nullable HttpHeaders httpHeaders, String tableName) {
+ Optional<BasicAuthPrincipal> principalOpt =
getPrincipalOpt(requesterIdentity);
+ if (principalOpt.isEmpty()) {
+ return new BasicAuthorizationResultImpl(false, "Missing or invalid
credentials");
+ }
+ BasicAuthPrincipal principal = principalOpt.get();
+ // `tables` matches the name as given, as for queries, while
`excludeTables` lists raw names: also check the
+ // exclusion list on the raw name, so that a name with a type suffix
cannot bypass an excluded table
+ if (!principal.hasTable(tableName)
+ ||
principal.isTableExcluded(TableNameBuilder.extractRawTableName(tableName))) {
Review Comment:
**[CONFIRMED — pre-existing, fix is DELETE-only] The excludeTables
type-suffix bypass remains on the query path.**
This line correctly checks `isTableExcluded(extractRawTableName(tableName))`
for DELETE, but `hasTable`/`authorize(Set)` on the query path still match
exclusions verbatim: with `excludeTables=[t]`, a principal can still *query*
`t_OFFLINE`. The underlying issue is that
`BasicAuthPrincipal.isTableNotExcluded` compares the name as given. Consider
normalizing to the raw name inside `BasicAuthPrincipal` so every caller (query
and DELETE) gets consistent suffix handling, rather than patching only this
path. (Pre-existing bug; this PR just makes the inconsistency visible.)
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotClientRequest.java:
##########
@@ -713,6 +742,231 @@ private BrokerResponse executeSqlQuery(ObjectNode
sqlRequestJson, HttpRequesterI
}
}
+ /// Executes a `DELETE` once the caller is authorized to delete rows from
its table.
+ ///
+ /// The first-step access control runs first, as for queries, followed by
the table check without a table, so that
+ /// an access control that only authenticates the caller in that check
rejects an unauthenticated caller before the
+ /// table is looked up. The table is then resolved with the database of the
request and in the case it is defined
+ /// with, the caller is authorized to delete rows from it (see
[#authorizeDelete]), and the executor deletes rows
+ /// from that exact table. A logical table fails with
[QueryErrorCode#QUERY_VALIDATION], and a table the table cache
+ /// does not know with [QueryErrorCode#TABLE_DOES_NOT_EXIST], as a query
does, but only once the caller is
+ /// authorized for it, and every message names the table as the caller wrote
it (see [#deleteAccessDenied] for the
+ /// reason of a denial), so that neither the existence of a table nor its
name as defined is leaked to a caller who
+ /// is not authorized for it.
+ private BrokerResponse executeDelete(SqlNodeAndOptions sqlNodeAndOptions,
Map<String, String> headers,
+ HttpRequesterIdentity requesterIdentity, @Nullable HttpHeaders
httpHeaders) {
+ AccessControl accessControl = _accessControlFactory.create();
+ // The first-step access control runs before the table is looked up, as
for queries
+ AuthorizationResult authorizationResult =
accessControl.authorize(requesterIdentity);
+ if (!authorizationResult.hasAccess()) {
+ throw deleteAccessDenied(null, null, authorizationResult);
+ }
+ // Access controls that authenticate the caller in their table check
rather than in the first step (e.g. the
+ // ZooKeeper basic auth) reject an unauthenticated caller here, before the
table is looked up
+ authorizationResult = authorizeTables(accessControl, requesterIdentity,
Set.of());
+ if (!authorizationResult.hasAccess()) {
+ throw deleteAccessDenied(null, null, authorizationResult);
+ }
+ if (_tableCache == null) {
+ return new BrokerResponseNative(QueryErrorCode.QUERY_VALIDATION,
+ "DELETE is not supported by this broker: no table cache was
configured");
+ }
+ DeleteStatement parsed;
+ DeleteStatement statement;
+ try {
+ String databaseHeader = httpHeaders != null ?
httpHeaders.getHeaderString(CommonConstants.DATABASE) : null;
+ parsed = (DeleteStatement)
DataManipulationStatementParser.parse(sqlNodeAndOptions);
+ statement = parsed.resolveTableName(databaseHeader, _tableCache);
+ } catch (QueryException e) {
+ // e.g. an invalid statement, or a database header that does not match
the statement
+ return new BrokerResponseNative(e.getErrorCode(), e.getMessage());
+ }
+ // The caller is authorized for the table as resolved, but the messages
name it as written
+ String writtenTableName = parsed.getTableName();
+ authorizeDelete(accessControl, statement.getTableName(), writtenTableName,
requesterIdentity, httpHeaders);
+ if (statement.isLogicalTable()) {
+ return new BrokerResponseNative(QueryErrorCode.QUERY_VALIDATION,
+ "DELETE does not support logical tables: " + writtenTableName);
+ }
+ if (!statement.tableExists()) {
+ return new BrokerResponseNative(QueryErrorCode.TABLE_DOES_NOT_EXIST,
+ "Table does not exist: " + writtenTableName);
+ }
+ LOGGER.info(deleteLogMessage(statement, sqlNodeAndOptions,
requesterIdentity));
+ return _sqlQueryExecutor.executeStatement(statement, headers);
+ }
+
+ /// Formats the log line of a `DELETE` the executor is about to run. The
predicate is redacted with the SQL
+ /// redaction mode of the query log (`pinot.broker.query.log.sqlRedaction`),
as the queries are, since its literals
+ /// are the very values a privacy-driven delete removes: the
`literal_values` mode logs the fingerprint of the
+ /// statement (`DELETE FROM t WHERE c = ?`), which keeps its shape as the
fingerprint of a query does, in place of
+ /// the predicate. The client is only logged when the query log logs client
IPs
+ /// (`pinot.broker.request.client.ip.logging`), since it comes from request
headers the caller controls. The option
+ /// keys and the client are kept on a single line, as the query log keeps
the predicate, so that a line ending in
+ /// the statement or in a header cannot forge a log record.
+ ///
+ /// @param sqlNodeAndOptions parsed statement, which the `literal_values`
mode fingerprints
+ @VisibleForTesting
+ String deleteLogMessage(DeleteStatement statement, SqlNodeAndOptions
sqlNodeAndOptions,
+ HttpRequesterIdentity requesterIdentity) {
+ SqlRedactionMode sqlRedactionMode = SqlRedactionMode.fromString(
+
_brokerConf.getProperty(CommonConstants.Broker.CONFIG_OF_BROKER_QUERY_LOG_SQL_REDACTION,
+ CommonConstants.Broker.DEFAULT_BROKER_QUERY_LOG_SQL_REDACTION));
+ QueryFingerprint fingerprint = null;
+ if (sqlRedactionMode == SqlRedactionMode.LITERAL_VALUES) {
+ try {
+ fingerprint =
QueryFingerprintUtils.generateFingerprint(sqlNodeAndOptions);
+ } catch (Exception e) {
+ // Logged as the query log logs a query it could not fingerprint
+ LOGGER.warn("Failed to generate the fingerprint of a DELETE on table:
{}", statement.getTableName(), e);
+ }
+ }
+ String predicate = QueryLogger.redactQuery(statement.getPredicate(),
sqlRedactionMode, fingerprint);
+ String options = toSingleLine(statement.getOptions().keySet().toString());
+ String client =
_brokerConf.getProperty(CommonConstants.Broker.CONFIG_OF_BROKER_REQUEST_CLIENT_IP_LOGGING,
+ CommonConstants.Broker.DEFAULT_BROKER_REQUEST_CLIENT_IP_LOGGING)
+ ? toSingleLine(requesterIdentity.getClientIp()) :
CommonConstants.UNKNOWN;
+ return "Executing DELETE on table: " + statement.getTableName() + ",
predicate: " + predicate + ", options: "
+ + options + ", client: " + client;
+ }
+
+ /// Escapes backslashes, CR and LF as `\\`, `\r` and `\n`, as `QueryLogger`
does to log a query on a single line.
+ private static String toSingleLine(String text) {
+ return text.replace("\\", "\\\\").replace("\r", "\\r").replace("\n",
"\\n");
+ }
+
+ /// Authorizes the caller, who passed the first-step access control, to
delete rows from the table.
+ ///
+ /// The checks run in this order, and the first one that denies answers HTTP
403:
+ /// 1. [AccessControl#authorize(RequesterIdentity, Set)] with the table as
its only element, since the WHERE clause
+ /// reads it. [AccessControl#authorize(RequesterIdentity, BrokerRequest)]
is not called, since a `DELETE` is not a
+ /// broker request: table restrictions that an access control only
applies in that overload are not consulted,
+ /// and must be applied again in [AccessControl#authorizeDeleteRows].
+ /// 2. The fine-grained [Actions.Table#QUERY] action on the table name, as
for a query.
+ /// 3. The fine-grained [Actions.Table#DELETE_ROWS] action on the table
name, then on its raw name when the name has
+ /// a type suffix, which the controller checks, so that a type suffix
cannot bypass a rule on the raw name.
+ /// 4. [AccessControl#authorizeDeleteRows], which denies by default: the
fine-grained checks allow every action by
+ /// default, so they cannot tell an access control written before
`DELETE` existed from one that allows it.
+ /// 5. No row-level security filter applies to the table (see
[AccessControl#getRowColFilters], when the broker
+ /// enables row-level security), since it would not restrict the rows the
statement deletes.
+ ///
+ /// @param tableName table as resolved, which the checks authorize
+ /// @param writtenTableName table as the caller wrote it, which a denial
names
+ /// @throws WebApplicationException with status 403 if the caller is not
authorized
+ private void authorizeDelete(AccessControl accessControl, String tableName,
String writtenTableName,
+ HttpRequesterIdentity requesterIdentity, @Nullable HttpHeaders
httpHeaders) {
+ checkDeleteAccess(authorizeTables(accessControl, requesterIdentity,
Set.of(tableName)), tableName,
+ writtenTableName);
+ checkDeleteAccess(accessControl.authorize(httpHeaders, TargetType.TABLE,
tableName, Actions.Table.QUERY),
+ tableName, writtenTableName);
+ checkDeleteAccess(accessControl.authorize(httpHeaders, TargetType.TABLE,
tableName, Actions.Table.DELETE_ROWS),
+ tableName, writtenTableName);
+ String rawTableName = TableNameBuilder.extractRawTableName(tableName);
+ if (!rawTableName.equals(tableName)) {
+ checkDeleteAccess(
+ accessControl.authorize(httpHeaders, TargetType.TABLE, rawTableName,
Actions.Table.DELETE_ROWS), tableName,
+ writtenTableName);
+ }
+ checkDeleteAccess(accessControl.authorizeDeleteRows(requesterIdentity,
httpHeaders, tableName), tableName,
+ writtenTableName);
+ if (hasRowFilters(accessControl, requesterIdentity, tableName)) {
+ throw deleteAccessDenied(tableName, writtenTableName, new
BasicAuthorizationResultImpl(false,
+ "Row-level security applies to the table, and would not restrict the
rows the statement deletes"));
+ }
+ }
+
+ /// Throws the HTTP 403 error of a `DELETE` if the check denied it.
+ private void checkDeleteAccess(AuthorizationResult authorizationResult,
String tableName, String writtenTableName) {
+ if (!authorizationResult.hasAccess()) {
+ throw deleteAccessDenied(tableName, writtenTableName,
authorizationResult);
+ }
+ }
+
+ /// Runs the table check of the access control, denying the `DELETE` when
the access control cannot run it: an
+ /// access control written for single-stage queries only, which overrides
+ /// [AccessControl#hasAccess(RequesterIdentity, BrokerRequest)] but not the
table check, throws
+ /// [UnsupportedOperationException] from the default table check (see
+ /// [AccessControl#hasAccess(RequesterIdentity, Set)]). A `DELETE` is not a
broker request, so it cannot be
+ /// authorized by such an access control, and is denied rather than failed
as an uncaught error.
+ private static AuthorizationResult authorizeTables(AccessControl
accessControl, RequesterIdentity requesterIdentity,
+ Set<String> tables) {
+ try {
+ return accessControl.authorize(requesterIdentity, tables);
+ } catch (UnsupportedOperationException e) {
+ LOGGER.warn("The access control {} cannot authorize a DELETE: it
implements the broker-request check only",
+ accessControl.getClass().getName(), e);
+ return new BasicAuthorizationResultImpl(false,
+ "The access control of the broker cannot authorize a DELETE: it
implements the broker-request check only");
+ }
+ }
+
+ /// Records a `DELETE` denied by the access control (metric, and a log line
once the table is known) and returns its
+ /// HTTP 403 error to throw. The error only names the table once the caller
passed the first-step access control,
+ /// and names it as the caller wrote it rather than as resolved, so that a
caller who is not authorized for the
+ /// table does not learn its name as defined (e.g. its case, which
`enable.case.insensitive` resolves).
+ ///
+ /// The reason of the access control is only sent to the caller when it does
not mention the table, since the
+ /// access control echoes the name it authorized, which is the resolved one
(e.g. the basic auth: "Authorization
+ /// Failed for tables: [MyTable]"). Rewriting the resolved name in the
reason would not do: it would only apply
+ /// when the table cache resolved the name, i.e. when the table exists, so
the reason would read differently for a
+ /// table that does not exist. The rule applies whether or not the resolved
name differs from the written one, so
+ /// that the message is the same for a table that exists and for one that
does not. The log line keeps the full
+ /// reason and the resolved name for the operator.
+ private WebApplicationException deleteAccessDenied(@Nullable String
tableName, @Nullable String writtenTableName,
+ AuthorizationResult authorizationResult) {
+
_brokerMetrics.addMeteredGlobalValue(BrokerMeter.REQUEST_DROPPED_DUE_TO_ACCESS_ERROR,
1);
+ String reason = authorizationResult.getFailureMessage();
+ String message;
+ if (tableName != null) {
+ // The name, which is the one written when the table cache does not know
it, and the reason come from the
+ // request and from the access control: escaped as the predicate of the
log line of a DELETE is, so that they
+ // cannot forge a log record
+ LOGGER.info("Access denied to delete rows from table: {}, reason: {}",
toSingleLine(tableName),
+ reason != null ? toSingleLine(reason) : null);
+ if (reason != null && (mentionsTable(reason, tableName) ||
mentionsTable(reason, writtenTableName))) {
+ reason = null;
+ }
+ message = "Permission denied to delete rows from table: " +
writtenTableName;
+ } else {
+ message = "Permission denied to delete rows";
+ }
+ if (reason != null && !reason.isBlank()) {
+ message += ". Reason: " + reason;
+ }
+ return new WebApplicationException(message, Response.Status.FORBIDDEN);
+ }
+
+ /// Whether the reason of a denial mentions the table name, or its raw name
(the fine-grained check of the right to
+ /// delete rows also runs on the raw name), ignoring case, since
`enable.case.insensitive` resolves it.
+ private static boolean mentionsTable(String reason, @Nullable String
tableName) {
Review Comment:
**[CONFIRMED — minor] `mentionsTable` substring matching over-suppresses
denial reasons.**
`containsIgnoreCase(reason, tableName)` false-positives on incidental
substrings: for a table named `rows`, the default reason "The access control of
the broker does not allow deleting rows" is suppressed; for short names like
`a`, virtually every reason is. Impact is cosmetic (the operator log keeps the
full reason; no leak), but callers lose the explanation exactly when they need
it. Word-boundary matching, or the simpler rule of never echoing the
access-control reason once a table is involved, would avoid the heuristic.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/BaseControllerStarter.java:
##########
@@ -715,6 +714,12 @@ private void setUpPinotController() {
// max.segment.completion.time.millis is seeded from cluster config before
serving requests.
_clusterConfigChangeHandler.registerClusterConfigChangeListener(_pinotLLCRealtimeSegmentManager);
LOGGER.info("Registered PinotLLCRealtimeSegmentManager as cluster config
change listener");
+ // Keeps the legacy OPTION(...) syntax mode cluster config in effect for
the statements the controller parses on
+ // /sql, including the DML it executes itself (a DELETE carrying legacy
options fails in IGNORE and REJECT mode).
+ // The query option validation mode is not applied here: the broker
validates every query the controller forwards,
+ // and the REJECT allowlist of registered SET keys is per JVM (broker
plugins register their keys in the broker
+ // only), so applying it here would reject options the broker accepts
+
_clusterConfigChangeHandler.registerClusterConfigChangeListener(QueryOptionConfigListener.legacySyntaxModeOnly());
Review Comment:
**[PLAUSIBLE — unacknowledged blast radius] The legacy-syntax mode now
applies to every controller-side parse, not just `/sql` DML.**
`QueryOptionsUtils._legacyOptionSyntaxMode` is a JVM-wide static read by
`compileToSqlNodeAndOptions`, so this registration also changes the recommender
(`InputManager.compileToPinotQuery`) and `/query/tableNames`
(`TableNameExtractor`) on clusters with `legacySyntaxMode=REJECT/IGNORE`:
queries with `OPTION(...)` those endpoints accepted pre-PR now fail. Arguably
the intended cluster-policy consistency (the broker would reject the same query
at runtime), but worth an explicit note in the comment/release notes since
those endpoints aren't mentioned.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java:
##########
@@ -424,6 +445,116 @@ private StreamingOutput executeSqlQuery(@Context
HttpHeaders httpHeaders, String
}
}
+ /// Executes a `DELETE` once the caller is authorized to delete rows from
its table, see [#authorizeDelete].
+ ///
+ /// The caller is first checked without a table (the `READ` access type with
the `/sql` endpoint, as for a
+ /// multi-stage query), before the statement is parsed and its table looked
up: with basic auth, an unauthenticated
+ /// caller gets HTTP 401 here, before the table cache is consulted. The
table is then resolved, with the database of
+ /// the request and in the case it is defined with, and the caller
authorized to delete rows from the resolved
+ /// table. Only then is the table checked not to be a logical table and to
exist, and every error message names the
+ /// table as the caller wrote it, so that a caller who is not authorized for
a table learns neither whether it exists
+ /// nor the case it is defined with, nor whether the name is a logical
table. The executor deletes rows from the
+ /// exact resolved table.
+ ///
+ /// The executor runs before the response streams, so that an exception it
lets escape is mapped to an error
+ /// response by [#executeSqlQueryCatching] rather than failing the request
with HTTP 500.
+ ///
+ /// @throws QueryException with [QueryErrorCode#ACCESS_DENIED] if the caller
is not authorized, with
+ /// [QueryErrorCode#QUERY_VALIDATION] if the table is
a logical table, with
+ /// [QueryErrorCode#TABLE_DOES_NOT_EXIST] if the
table does not exist, and with the errors of
+ /// [DataManipulationStatementParser#parse] and
[DeleteStatement#resolveTableName]
+ private StreamingOutput executeDelete(SqlNodeAndOptions sqlNodeAndOptions,
HttpHeaders httpHeaders) {
+ AccessControl accessControl = _accessControlFactory.create();
+ if (!accessControl.hasAccess(AccessType.READ, httpHeaders, SQL_ENDPOINT)) {
+ throw QueryErrorCode.ACCESS_DENIED.asException("Permission denied to
delete rows");
+ }
+ DeleteStatement parsed = (DeleteStatement)
DataManipulationStatementParser.parse(sqlNodeAndOptions);
+ DeleteStatement statement =
parsed.resolveTableName(httpHeaders.getHeaderString(CommonConstants.DATABASE),
+ _pinotHelixResourceManager.getTableCache());
+ // Authorized on the resolved table, reported with the name as written
+ String writtenTableName = parsed.getTableName();
+ authorizeDelete(accessControl, statement.getTableName(), writtenTableName,
httpHeaders);
+ if (statement.isLogicalTable()) {
+ throw QueryErrorCode.QUERY_VALIDATION.asException("DELETE does not
support logical tables: " + writtenTableName);
+ }
+ if (!statement.tableExists()) {
+ throw QueryErrorCode.TABLE_DOES_NOT_EXIST.asException("Table does not
exist: " + writtenTableName);
+ }
+ LOGGER.info(deleteLogMessage(statement, httpHeaders));
+ BrokerResponse response = _sqlQueryExecutor.executeStatement(statement,
extractHeaders(httpHeaders));
+ return output -> {
+ try (OutputStream os = output) {
+ response.toOutputStream(os);
+ }
+ };
+ }
+
+ /// Formats the log line of a `DELETE` the executor is about to run: the
resolved table, the predicate, the option
+ /// keys and the client of the request, in the format of the broker's line
so that it parses the same way in the
+ /// logs of both roles. The predicate, the option keys and the client are
kept on a single line (see
+ /// [#toSingleLine]), so that a line ending in the statement, in a request
option or in a proxy header cannot forge
+ /// a log record.
+ @VisibleForTesting
+ static String deleteLogMessage(DeleteStatement statement, HttpHeaders
httpHeaders) {
+ return "Executing DELETE on table: " + statement.getTableName() + ",
predicate: "
+ + toSingleLine(statement.getPredicate()) + ", options: "
+ + toSingleLine(statement.getOptions().keySet().toString()) + ",
client: "
+ + toSingleLine(getClientIp(httpHeaders));
+ }
+
+ /// Client of the request for the log: the `X-Forwarded-For` header, else
the `X-Real-IP` header, else
+ /// [CommonConstants#UNKNOWN]. The addresses of a comma-separated
`X-Forwarded-For` value are joined with `;`, the
+ /// format the broker's `HttpRequesterIdentity#getClientIp` logs, so that
the field parses the same way in the logs
+ /// of both roles. Both headers are set by the caller unless a proxy
overwrites them.
+ private static String getClientIp(HttpHeaders httpHeaders) {
+ String forwardedFor = httpHeaders.getHeaderString("X-Forwarded-For");
Review Comment:
**[PLAUSIBLE — minor hardening] The DELETE audit log's `client` field is
header-only, ungated, with no remote-address fallback.**
It is derived solely from caller-controlled `X-Forwarded-For`/`X-Real-IP`
(spoofable, as the javadoc notes), logged unconditionally (the broker gates on
`pinot.broker.request.client.ip.logging`), and falls back to `unknown` even
though the real peer address is available via `@Context
Request#getRemoteAddr()` (used elsewhere in the controller, e.g.
`PinotSegmentUploadDownloadRestletResource`). Appending the socket address
would make the audit line trustworthy for forensics at negligible cost.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java:
##########
@@ -424,6 +445,116 @@ private StreamingOutput executeSqlQuery(@Context
HttpHeaders httpHeaders, String
}
}
+ /// Executes a `DELETE` once the caller is authorized to delete rows from
its table, see [#authorizeDelete].
+ ///
+ /// The caller is first checked without a table (the `READ` access type with
the `/sql` endpoint, as for a
+ /// multi-stage query), before the statement is parsed and its table looked
up: with basic auth, an unauthenticated
+ /// caller gets HTTP 401 here, before the table cache is consulted. The
table is then resolved, with the database of
+ /// the request and in the case it is defined with, and the caller
authorized to delete rows from the resolved
+ /// table. Only then is the table checked not to be a logical table and to
exist, and every error message names the
+ /// table as the caller wrote it, so that a caller who is not authorized for
a table learns neither whether it exists
+ /// nor the case it is defined with, nor whether the name is a logical
table. The executor deletes rows from the
+ /// exact resolved table.
+ ///
+ /// The executor runs before the response streams, so that an exception it
lets escape is mapped to an error
+ /// response by [#executeSqlQueryCatching] rather than failing the request
with HTTP 500.
+ ///
+ /// @throws QueryException with [QueryErrorCode#ACCESS_DENIED] if the caller
is not authorized, with
+ /// [QueryErrorCode#QUERY_VALIDATION] if the table is
a logical table, with
+ /// [QueryErrorCode#TABLE_DOES_NOT_EXIST] if the
table does not exist, and with the errors of
+ /// [DataManipulationStatementParser#parse] and
[DeleteStatement#resolveTableName]
+ private StreamingOutput executeDelete(SqlNodeAndOptions sqlNodeAndOptions,
HttpHeaders httpHeaders) {
+ AccessControl accessControl = _accessControlFactory.create();
+ if (!accessControl.hasAccess(AccessType.READ, httpHeaders, SQL_ENDPOINT)) {
+ throw QueryErrorCode.ACCESS_DENIED.asException("Permission denied to
delete rows");
+ }
+ DeleteStatement parsed = (DeleteStatement)
DataManipulationStatementParser.parse(sqlNodeAndOptions);
+ DeleteStatement statement =
parsed.resolveTableName(httpHeaders.getHeaderString(CommonConstants.DATABASE),
+ _pinotHelixResourceManager.getTableCache());
+ // Authorized on the resolved table, reported with the name as written
+ String writtenTableName = parsed.getTableName();
+ authorizeDelete(accessControl, statement.getTableName(), writtenTableName,
httpHeaders);
+ if (statement.isLogicalTable()) {
+ throw QueryErrorCode.QUERY_VALIDATION.asException("DELETE does not
support logical tables: " + writtenTableName);
+ }
+ if (!statement.tableExists()) {
+ throw QueryErrorCode.TABLE_DOES_NOT_EXIST.asException("Table does not
exist: " + writtenTableName);
+ }
+ LOGGER.info(deleteLogMessage(statement, httpHeaders));
+ BrokerResponse response = _sqlQueryExecutor.executeStatement(statement,
extractHeaders(httpHeaders));
+ return output -> {
+ try (OutputStream os = output) {
+ response.toOutputStream(os);
+ }
+ };
+ }
+
+ /// Formats the log line of a `DELETE` the executor is about to run: the
resolved table, the predicate, the option
+ /// keys and the client of the request, in the format of the broker's line
so that it parses the same way in the
+ /// logs of both roles. The predicate, the option keys and the client are
kept on a single line (see
+ /// [#toSingleLine]), so that a line ending in the statement, in a request
option or in a proxy header cannot forge
+ /// a log record.
+ @VisibleForTesting
+ static String deleteLogMessage(DeleteStatement statement, HttpHeaders
httpHeaders) {
+ return "Executing DELETE on table: " + statement.getTableName() + ",
predicate: "
+ + toSingleLine(statement.getPredicate()) + ", options: "
+ + toSingleLine(statement.getOptions().keySet().toString()) + ",
client: "
+ + toSingleLine(getClientIp(httpHeaders));
+ }
+
+ /// Client of the request for the log: the `X-Forwarded-For` header, else
the `X-Real-IP` header, else
+ /// [CommonConstants#UNKNOWN]. The addresses of a comma-separated
`X-Forwarded-For` value are joined with `;`, the
+ /// format the broker's `HttpRequesterIdentity#getClientIp` logs, so that
the field parses the same way in the logs
+ /// of both roles. Both headers are set by the caller unless a proxy
overwrites them.
+ private static String getClientIp(HttpHeaders httpHeaders) {
+ String forwardedFor = httpHeaders.getHeaderString("X-Forwarded-For");
+ if (forwardedFor != null) {
+ return forwardedFor.replace(',', ';');
+ }
+ String realIp = httpHeaders.getHeaderString("X-Real-IP");
+ return realIp != null ? realIp : CommonConstants.UNKNOWN;
+ }
+
+ /// Escapes backslashes, CR and LF as `\\`, `\r` and `\n`, as the broker's
query log does, so that a value the
+ /// caller controls (a string literal or a quoted identifier of the
predicate, an option key from `SET`, the legacy
+ /// `OPTION(...)` suffix or the request `queryOptions`, a proxy header)
cannot split a log record or append a line
+ /// that looks like a log entry.
+ private static String toSingleLine(String value) {
Review Comment:
**[Cleanup — reuse] Third copy of the `toSingleLine` escaping helper.**
This is byte-identical to `QueryLogger.toSingleLine` (private) and
`PinotClientRequest.toSingleLine`. The javadocs promise the broker and
controller lines "parse the same way" — a promise three hand-synced copies will
eventually break. Since the PR already made `QueryLogger.redactQuery` public
static for cross-class use, hoisting one `toSingleLine` into pinot-common (or
making QueryLogger's public) and calling it from all three places would pin the
contract.
##########
pinot-broker/src/main/java/org/apache/pinot/broker/broker/ZkBasicAuthAccessControlFactory.java:
##########
@@ -115,6 +123,26 @@ public TableAuthorizationResult
authorize(RequesterIdentity requesterIdentity, S
return new TableAuthorizationResult(failedTables);
}
+ @Override
+ public AuthorizationResult authorizeDeleteRows(RequesterIdentity
requesterIdentity,
Review Comment:
**[Cleanup — duplication] `authorizeDeleteRows` is copy-pasted between the
file-based and ZK basic-auth factories.**
Same principal lookup, same three failure messages, differing only in
table-name matching (raw here, as-given + raw exclusion in the file-based
variant). A fix to one (message wording, name-matching subtleties) won't reach
the other. A shared helper next to `BasicAuthPrincipal` taking the principal
lookup and a name-matching predicate would keep the two implementations in
lockstep.
##########
pinot-common/src/main/java/org/apache/pinot/sql/parsers/dml/DeleteStatement.java:
##########
@@ -0,0 +1,480 @@
+/**
+ * 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.pinot.sql.parsers.dml;
+
+import java.math.BigDecimal;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Locale;
+import java.util.Map;
+import java.util.Set;
+import javax.annotation.Nullable;
+import org.apache.calcite.avatica.util.Casing;
+import org.apache.calcite.sql.SqlCall;
+import org.apache.calcite.sql.SqlDelete;
+import org.apache.calcite.sql.SqlDialect;
+import org.apache.calcite.sql.SqlIdentifier;
+import org.apache.calcite.sql.SqlIntervalLiteral;
+import org.apache.calcite.sql.SqlKind;
+import org.apache.calcite.sql.SqlLiteral;
+import org.apache.calcite.sql.SqlNode;
+import org.apache.calcite.sql.SqlNumericLiteral;
+import org.apache.calcite.sql.SqlOperator;
+import org.apache.calcite.sql.SqlSyntax;
+import org.apache.calcite.sql.SqlWindow;
+import org.apache.calcite.sql.fun.SqlBetweenOperator;
+import org.apache.calcite.sql.fun.SqlLikeOperator;
+import org.apache.calcite.sql.fun.SqlStdOperatorTable;
+import org.apache.calcite.sql.parser.SqlParserPos;
+import org.apache.calcite.sql.util.SqlBasicVisitor;
+import org.apache.calcite.sql.util.SqlShuttle;
+import org.apache.calcite.sql.validate.SqlValidatorUtil;
+import org.apache.pinot.common.config.provider.TableCache;
+import org.apache.pinot.common.request.Expression;
+import org.apache.pinot.common.request.ExpressionType;
+import org.apache.pinot.common.request.Function;
+import org.apache.pinot.common.utils.DataSchema;
+import org.apache.pinot.common.utils.DatabaseUtils;
+import org.apache.pinot.spi.config.task.AdhocTaskConfig;
+import org.apache.pinot.spi.exception.QueryErrorCode;
+import org.apache.pinot.spi.exception.QueryException;
+import org.apache.pinot.spi.utils.CommonConstants;
+import org.apache.pinot.sql.parsers.CalciteSqlParser;
+import org.apache.pinot.sql.parsers.SqlNodeAndOptions;
+
+import static com.google.common.base.Preconditions.checkArgument;
+
+
+/// A SQL `DELETE FROM <table> WHERE <predicate>` statement.
+///
+/// Pinot parses `DELETE` but does not delete rows itself: the default SQL
executor answers that it is not supported,
+/// and a deployment that can delete rows, e.g. by purging the matching rows
from the segments with a minion task,
+/// executes the parsed statement by overriding
`SqlQueryExecutor#executeDelete`. Its execution type is
+/// [ExecutionType#EXECUTOR]: the generic [#execute()], [#getResultSchema()]
and [#generateAdhocTaskConfig()] do not
+/// apply to it.
+///
+/// Before handing the statement to the executor, the broker and the
controller resolve its table with
+/// [#resolveTableName], authorize the caller to delete rows from that table,
and check that the table is not a
+/// logical table with [#isLogicalTable()] and that it exists with
[#tableExists()].
+///
+/// Its options are the `SET` statements, the legacy `OPTION(...)` suffix and
the request `queryOptions` of the
+/// statement (`SET` takes precedence). The `database` option only qualifies
the table name, see [#resolveTableName].
+/// Every other option, including query options such as `timeoutMs`, is passed
to the executor through
+/// [#getOptions()], so that an option the executor relies on (e.g. a dry run)
cannot be reclassified as a query option
+/// and silently dropped.
+///
+/// Instances are immutable and thread-safe.
+public class DeleteStatement implements DataManipulationStatement {
+ public static final String NOT_SUPPORTED_MESSAGE =
+ "DELETE is not supported by this endpoint, it requires a SQL executor
that implements row deletion";
+
+ /// Serializes the WHERE clause into SQL that the Pinot parser reads back
into the same expression. Combined with
+ /// `quoteAllIdentifiers = false`, identifiers are only quoted (with double
quotes) when they were quoted.
+ private static final SqlDialect PINOT_SQL_DIALECT = new
SqlDialect(SqlDialect.EMPTY_CONTEXT
+ .withIdentifierQuoteString("\"")
+ .withLiteralQuoteString("'")
+ .withLiteralEscapedQuoteString("''")
+ .withUnquotedCasing(Casing.UNCHANGED)
+ .withQuotedCasing(Casing.UNCHANGED)
+ .withCaseSensitive(true));
+
+ /// Quotes the unquoted identifiers named like a SQL function without
arguments, e.g. `user`, `pi` or `current_date`:
+ /// Calcite unparses them as upper-cased keywords, while Pinot reads them as
columns, so they would name another
+ /// column. Mirrors the check of `SqlUtil.unparseSqlIdentifierSyntax`.
+ private static final SqlShuttle KEYWORD_IDENTIFIER_QUOTER = new SqlShuttle()
{
+ @Override
+ public SqlNode visit(SqlIdentifier identifier) {
+ if (identifier.isSimple() && !identifier.getParserPosition().isQuoted())
{
+ SqlOperator operator =
+
SqlValidatorUtil.lookupSqlFunctionByID(SqlStdOperatorTable.instance(),
identifier, null);
+ if (operator != null && (operator.getSyntax() == SqlSyntax.FUNCTION_ID
+ || operator.getSyntax() == SqlSyntax.FUNCTION_ID_CONSTANT)) {
+ return new SqlIdentifier(identifier.names, null,
SqlParserPos.QUOTED_ZERO,
+ List.of(SqlParserPos.QUOTED_ZERO));
+ }
+ }
+ return identifier;
+ }
+ };
+
+ /// Canonical names (upper case, without underscores) of the functions that
read another table than the one the
+ /// statement deletes from, which the caller is not authorized to read.
Upper case because the compiled function
+ /// names are lower-cased with the default locale, and lower-casing with a
Turkish locale turns `I` into the dotless
+ /// `ı` (U+0131), which only an upper-case fold with [Locale#ROOT] maps back
to `I`.
+ private static final Set<String> CROSS_TABLE_FUNCTIONS = Set.of("LOOKUP",
"INSUBQUERY", "INPARTITIONEDSUBQUERY");
Review Comment:
**[Cleanup — altitude] The cross-table/script function blocklist will
silently go stale.**
Nothing ties `CROSS_TABLE_FUNCTIONS`/`SCRIPT_FUNCTIONS` to where such
functions are registered: a future cross-table or script function added in
another module passes into DELETE predicates by default, re-opening the
authorization/groovy-policy hole this set closes. Consider a central
classification (a flag on the function registry) checked here, or at minimum a
pointer comment at the registration site(s) so new functions update this set.
The `like()` 3-operand gap found in this review is an instance of the same
fail-open-by-default shape.
--
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]
