xiangfu0 commented on code in PR #19664: URL: https://github.com/apache/pinot/pull/19664#discussion_r4252242786
########## 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"); + + /// Canonical names of the functions that run a script. Rejected because the broker's groovy policy + /// (`pinot.broker.disable.query.groovy`, the per-table `queryConfig.disableGroovy` override and the static + /// analyzer) only applies to queries, not to a `DELETE`, and the executor may run the predicate on another role. + private static final Set<String> SCRIPT_FUNCTIONS = Set.of("GROOVY"); + + /// Canonical names of the window function operators. A window function is never valid in a `DELETE` predicate, and + /// the compiled expression hides its window specification (and any cross-table or script function call in it) in a + /// string literal, see [#PREDICATE_NODE_CHECKER]. + private static final Set<String> WINDOW_FUNCTIONS = Set.of("OVER"); + + private static final BigDecimal LONG_MIN_VALUE = BigDecimal.valueOf(Long.MIN_VALUE); + private static final BigDecimal LONG_MAX_VALUE = BigDecimal.valueOf(Long.MAX_VALUE); + + /// Shared by the quoted interval literal (`INTERVAL '30' DAY`, a [SqlIntervalLiteral]) and the unquoted one + /// (`INTERVAL 30 DAY` or `INTERVAL (expr) DAY`, a [SqlCall] of kind [SqlKind#INTERVAL]), which the parser accepts + /// with its BABEL conformance. + private static final String INTERVAL_MESSAGE = "Unsupported WHERE clause in DELETE, interval literals are not " + + "supported in a DELETE predicate, their unit is lost (INTERVAL '30' DAY) or not evaluated (INTERVAL 30 DAY): " + + "use TIMESTAMPADD or a millisecond value"; + + /// Rejects the WHERE clause nodes whose single-stage [Expression] selects other rows than the SQL, so that the + /// round-trip check of [#toPinotSql] cannot catch them, and the window functions, whose window specification is + /// compiled into a string literal that hides the functions it calls from [#checkFunctions]. Runs on the parsed + /// [SqlNode] before it is compiled, so that the statement fails with a clear message. + /// + /// - A quoted interval literal compiles to its bare value: `INTERVAL '30' DAY` becomes the literal `'30'`. An + /// unquoted one, `INTERVAL 30 DAY`, compiles to a call of a function `interval` that no engine evaluates. + /// - `BETWEEN SYMMETRIC` compiles to a plain `BETWEEN`, which never matches an inverted range. + /// - `LIKE ... ESCAPE` compiles to a three-operand `LIKE` whose escape character the single-stage filter ignores: + /// the escaped wildcard still matches anything, and the escape character is matched literally. + /// - An exact integer literal outside the long range wraps around when it is compiled. + /// - Every other numeric literal compiles to a double: a decimal literal that a double does not represent exactly + /// (more than about 16 significant digits, e.g. `12345678901234567.89`) is rounded, which moves the bound a + /// `BIG_DECIMAL` column is compared against, and a literal outside the double range (`1E999`, `1E-999`) + /// becomes infinite or zero. The exact value is kept with `CAST('<digits>' AS DECIMAL)`, which compiles to a + /// `BigDecimal` literal. + private static final SqlBasicVisitor<Void> PREDICATE_NODE_CHECKER = new SqlBasicVisitor<>() { + @Override + public Void visit(SqlLiteral literal) { + checkArgument(!(literal instanceof SqlIntervalLiteral), INTERVAL_MESSAGE); + if (literal instanceof SqlNumericLiteral) { + SqlNumericLiteral numericLiteral = (SqlNumericLiteral) literal; + BigDecimal value = numericLiteral.bigDecimalValue(); + checkArgument(value != null, "Unsupported WHERE clause in DELETE, invalid numeric literal: %s", + numericLiteral.toValue()); + if (numericLiteral.isExact() && numericLiteral.isInteger()) { + checkArgument(value.compareTo(LONG_MIN_VALUE) >= 0 && value.compareTo(LONG_MAX_VALUE) <= 0, + "Unsupported WHERE clause in DELETE, integer literal out of the long range: %s", + numericLiteral.toValue()); + } else { + // Compiled as a double (RequestUtils.getLiteral): reject the overflow to an infinity and the underflow to 0 + double doubleValue = value.doubleValue(); + checkArgument(!Double.isInfinite(doubleValue) && (doubleValue != 0 || value.signum() == 0), + "Unsupported WHERE clause in DELETE, numeric literal out of the double range: %s", + numericLiteral.toValue()); + if (numericLiteral.isExact()) { + // BigDecimal.valueOf(double) is new BigDecimal(Double.toString(double)), the bound a BIG_DECIMAL column is + // compared against: the literal is accepted iff it survives the round trip through a double + checkArgument(BigDecimal.valueOf(doubleValue).compareTo(value) == 0, + "Unsupported WHERE clause in DELETE, decimal literal %s loses precision as a double: " + + "use CAST('%s' AS DECIMAL)", numericLiteral.toValue(), numericLiteral.toValue()); + } + } + } + return null; + } + + @Override + public Void visit(SqlCall call) { + checkArgument(call.getKind() != SqlKind.INTERVAL, INTERVAL_MESSAGE); + checkArgument(call.getKind() != SqlKind.OVER && !(call instanceof SqlWindow), + "Unsupported WHERE clause in DELETE, window functions are not supported in a DELETE predicate"); + if (call.getOperator() instanceof SqlBetweenOperator) { + checkArgument(((SqlBetweenOperator) call.getOperator()).flag != SqlBetweenOperator.Flag.SYMMETRIC, + "Unsupported WHERE clause in DELETE, BETWEEN SYMMETRIC is not supported in a DELETE predicate"); + } + // Also NOT LIKE, ILIKE and SIMILAR TO, which share the operator class + if (call.getOperator() instanceof SqlLikeOperator) { Review Comment: Good catch, fixed in de5d618d11: `checkFunctions` now rejects the compiled LIKE family (`like`, `notLike`, `iLike`, `similar` and `similarTo`) whenever the call carries three or more operands, so the function-call spelling fails like the operator form, including a padded `like(col, pattern, escape, x)`; `DeleteStatementTest` covers the rejected spellings, `like(col, 'a%')` accepted, and the operator form still rejected. ########## 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: Agreed that it needs to be loud rather than implied. The controller `AccessControl` has no row-level-security surface, and a DELETE only executes on the controller when a deployment overrides `createSqlQueryExecutor()` there, so in de5d618d11 that hook's Javadoc, `authorizeDelete`'s Javadoc and the PR description all state that the controller applies no row-level security and that a deployment relying on the brokers' row-level security must not install a deleting executor on the controller (or must enforce equivalent filtering in it). I'd rather not add a controller config for this in the PR: the opt-in is already a code override, and a config default of "enabled" would not close the gap while "disabled" would silently break the only reason to override the controller hook. ########## 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: Fixed in de5d618d11: when the resolved name differs from its raw name, `authorizeDelete` now also checks the fine-grained `QUERY` and `DeleteRows` actions on the resolved (typed) name after the raw-name checks, mirroring the broker, with a `PinotQueryResourceTest` case where a denial on the typed name alone refuses the DELETE. ########## 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: Changed in de5d618d11: the controller line now logs the fingerprint of the statement (literals replaced by `?`) instead of the predicate, falling back to `REDACTED` if fingerprinting fails, and the DEBUG trace line logs only the statement kind for DML, so the controller never writes the deleted values to its log. While re-checking that claim, the parser's own rejection messages turned out to echo the WHERE clause with its literals (and the controller logs them at WARN), so `DeleteStatement` now renders a rejected condition with `?` in place of every literal and the numeric-literal messages no longer echo the value. The broker's redaction key is a broker instance config the controller cannot read, so fingerprinting unconditionally seemed the right default for a destructive statement; the Javadoc says so. -- 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]
