jerryshao commented on code in PR #13478: URL: https://github.com/apache/gravitino/pull/13478#discussion_r4256160617
########## core/src/main/java/org/apache/gravitino/policy/rego/RestrictedRegoSourceParser.java: ########## @@ -0,0 +1,353 @@ +/* + * 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.gravitino.policy.rego; + +import java.math.BigDecimal; +import java.nio.charset.StandardCharsets; +import java.util.ArrayList; +import java.util.List; +import org.apache.gravitino.policy.rego.RestrictedRegoLexer.Token; +import org.apache.gravitino.policy.rego.RestrictedRegoLexer.TokenType; +import org.apache.gravitino.policy.rego.RestrictedRegoProgram.ColumnMask; +import org.apache.gravitino.policy.rego.RestrictedRegoProgram.FilterBranch; +import org.apache.gravitino.policy.rego.RestrictedRegoProgram.MaskAction; +import org.apache.gravitino.policy.rego.RestrictedRegoProgram.MaskBranch; +import org.apache.gravitino.policy.rego.RestrictedRegoProgram.RowFilter; + +/** Builds a {@code restricted-rego-v1} program from lexer tokens. */ +final class RestrictedRegoSourceParser { + private final RestrictedRegoLexer lexer; + private Token current; + private int parenthesisNesting; + + RestrictedRegoSourceParser(String source) { + this.lexer = new RestrictedRegoLexer(source); + this.current = lexer.nextToken(); + } + + RestrictedRegoProgram parseProgram() { + RestrictedRegoProgram program; + if (check(TokenType.FILTER)) { + program = parseRowFilter(); + } else if (check(TokenType.MASK)) { + program = parseColumnMask(); + } else { + throw error("expected filter or mask rule"); + } + expect(TokenType.EOF, "expected end of program"); + return program; + } + + private RowFilter parseRowFilter() { + expect(TokenType.FILTER, "expected filter"); + expect(TokenType.ASSIGN, "expected := after filter"); + CanonicalExpression result = parseExpression(); + if (!match(TokenType.IF)) { + return new RowFilter(new ArrayList<>(), result); + } + + List<FilterBranch> branches = new ArrayList<>(); + branches.add(new FilterBranch(result, parseExpression())); + while (true) { + expect(TokenType.ELSE, "expected final else branch"); + expect(TokenType.ASSIGN, "expected := after else"); + CanonicalExpression nextResult = parseExpression(); + if (!match(TokenType.IF)) { + return new RowFilter(branches, nextResult); + } + branches.add(new FilterBranch(nextResult, parseExpression())); + } + } + + private ColumnMask parseColumnMask() { + expect(TokenType.MASK, "expected mask"); + expect(TokenType.ASSIGN, "expected := after mask"); + MaskAction action = parseMaskAction(); + if (!match(TokenType.IF)) { + return new ColumnMask(new ArrayList<>(), action); + } + + List<MaskBranch> branches = new ArrayList<>(); + branches.add(new MaskBranch(action, parseExpression())); + while (true) { + expect(TokenType.ELSE, "expected final else branch"); + expect(TokenType.ASSIGN, "expected := after else"); + MaskAction nextAction = parseMaskAction(); + if (!match(TokenType.IF)) { + return new ColumnMask(branches, nextAction); + } + branches.add(new MaskBranch(nextAction, parseExpression())); + } + } + + private MaskAction parseMaskAction() { + expect(TokenType.ACTION, "expected action"); + expect(TokenType.LPAREN, "expected ( after action"); + Token actionToken = expect(TokenType.STRING, "expected mask action string"); + String action = decodeString(actionToken); + expect(TokenType.RPAREN, "expected ) after mask action"); + try { + return MaskAction.fromValue(action); + } catch (IllegalArgumentException exception) { + throw error(actionToken, exception.getMessage()); + } + } + + private CanonicalExpression parseExpression() { + return parseOrExpression(); + } + + private CanonicalExpression parseOrExpression() { + List<CanonicalExpression> operands = new ArrayList<>(); + operands.add(parseAndExpression()); + while (match(TokenType.OR)) { + operands.add(parseAndExpression()); + } + return LogicalExpression.of(LogicalOperator.OR, operands); + } + + private CanonicalExpression parseAndExpression() { + List<CanonicalExpression> operands = new ArrayList<>(); + operands.add(parseNotExpression()); + while (match(TokenType.AND)) { + operands.add(parseNotExpression()); + } + return LogicalExpression.of(LogicalOperator.AND, operands); + } + + private CanonicalExpression parseNotExpression() { + int notCount = 0; + while (match(TokenType.NOT)) { + notCount++; + } Review Comment: [Important] The depth limit is checked after the recursive `validate()` walk, so an unbounded `not` run can overflow the stack before the limit applies. `parseNotExpression` counts `not` iteratively but builds one `Not` node per token with no cap, and `RestrictedRegoSourceValidator.validate` calls `program.validate()` (RestrictedRegoSourceValidator.java:35) before the `MAX_SOURCE_DEPTH` check at :41. `Not.validate()` recurses one frame per level (Not.java:43), so a 16 KiB source of `not ` - up to 4090 levels - is walked in full before anything rejects it. `depth()` (Not.java:32) recurses the same way. Measured on this head, running the chain on a thread with an explicit stack size: at 1 MiB and at 576 KiB it rejects correctly with `restricted-rego-v1 source depth must not exceed 8`; at 512 KiB and 256 KiB the same input raises `StackOverflowError` instead, with every top frame in `Not.validate`. `PolicyExceptionHandler` maps `IllegalArgumentException` to 400, but an `Error` is not a bad request, so below ~560 KiB of stack a 16 KiB policy body stops being a 400. The repo sets no `-Xss`, so a default 1 MiB deployment is unaffected and this is not a merge blocker. But the margin is under 2x, `-Xss512k` is a common tuning for high thread counts, and the guarantee the limits exist to provide is currently a property of the stack size rather than of the parser. The count is already in hand right here, so the limit can be applied while parsing - reject when `notCount >= RestrictedRegoLimits.MAX_SOURCE_DEPTH`. That is exactly equivalent at the accept/reject boundary: N negations give depth >= N + 1, because the innermost predicate has depth >= 1, so a chain of more than 7 `not`s can never pass the depth-8 check anyway. A test with a 4090-`not` source on a 512 KiB thread would pin it. Verified by: compiled the `policy/rego` package standalone (stubbing `Preconditions`, `StringUtils` and `ReadRestrictionContent`) and ran `validateRowFilter` on `filter := ` plus 1000/2000/4000/4090 `not`s inside `new Thread(null, task, name, stackSize)`; printed the `StackOverflowError` top frames to confirm the site. Then applied the four-line guard above to the standalone copy: the 4090 case rejects at 512 KiB with the depth message, and regenerating `restricted-rego-v1-cases.golden` from the committed case list still reproduces the committed file byte-for-byte, so no existing case changes. ########## core/src/main/java/org/apache/gravitino/policy/rego/ExpressionValidator.java: ########## @@ -0,0 +1,146 @@ +/* + * 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.gravitino.policy.rego; + +import com.google.common.base.Preconditions; + +/** Semantic validation helpers for unresolved restricted Rego expressions. */ +final class ExpressionValidator { + private ExpressionValidator() {} + + static boolean isPredicate(CanonicalExpression expression) { + if (expression instanceof Comparison + || expression instanceof Not + || expression instanceof LogicalExpression + || expression instanceof GroupMembership) { + return true; + } + return expression instanceof Literal + && ((Literal) expression).literalType() == LiteralType.BOOLEAN; + } + + static boolean isContextOnly(CanonicalExpression expression) { + if (expression instanceof Column) { + return false; + } + if (expression instanceof Comparison) { + Comparison comparison = (Comparison) expression; + return isContextOnly(comparison.left()) && isContextOnly(comparison.right()); + } + if (expression instanceof Not) { + return isContextOnly(((Not) expression).operand()); + } + if (expression instanceof LogicalExpression) { + for (CanonicalExpression child : ((LogicalExpression) expression).operands()) { + if (!isContextOnly(child)) { + return false; + } + } + } + return true; + } + + static void validateComparison( + ComparisonOperator operator, CanonicalExpression left, CanonicalExpression right) { + if (operator == ComparisonOperator.IN) { + Preconditions.checkArgument( + right instanceof LiteralArray, "right operand of in must be a literal array"); + Preconditions.checkArgument( + left instanceof Column || left instanceof SessionUser, + "left operand of in must be col(...) or session_user()"); + if (left instanceof SessionUser) { + LiteralArray array = (LiteralArray) right; + Preconditions.checkArgument( + array.elementType() == LiteralType.STRING, + "session_user() can only be tested against a string array"); + } + return; + } + + Preconditions.checkArgument( + !(left instanceof LiteralArray) && !(right instanceof LiteralArray), + "%s does not support array operands", + operator.sourceToken()); + + boolean equality = operator == ComparisonOperator.EQ || operator == ComparisonOperator.NEQ; + if (isColumnLiteralPair(left, right)) { + Literal literal = left instanceof Literal ? (Literal) left : (Literal) right; + Preconditions.checkArgument( + literal.literalType() != LiteralType.NULL || equality, "null supports only == and !="); + Preconditions.checkArgument( + literal.literalType() != LiteralType.BOOLEAN || equality, + "boolean literal supports only == and !="); + return; + } + + if (isColumnSessionUserPair(left, right)) { + Preconditions.checkArgument(equality, "session_user() supports only == and !="); + return; + } + + if (isSessionUserStringPair(left, right)) { + Preconditions.checkArgument(equality, "session_user() supports only == and !="); + return; + } + + throw new IllegalArgumentException( + String.format("Unsupported operands for %s in restricted-rego-v1", operator.sourceToken())); + } + + static void validateUnicodeScalars(String value, String description) { + for (int index = 0; index < value.length(); index++) { + char current = value.charAt(index); + Preconditions.checkArgument(current != '\0', "%s cannot contain NUL", description); Review Comment: [Nit] Every escaped control character except NUL is accepted in column names, group names and string values. The lexer rejects raw control characters (RestrictedRegoLexer.java:104) and this line rejects NUL, but nothing rejects the other C0 controls once they arrive as `\u` escapes. Measured on this head, all of these parse and would be stored: `filter := col("a\u0001b") == "x"`, `col("a\u001fb")`, `col("a\u000ab")`, the `\n` shorthand `col("a\nb")`, `filter := is_group_member("a\u0001b")`, and `filter := col("x") == "a\u0001b"`. `\u007f` passes too. `docs/policies.md` is literally accurate here - it says *raw* control characters are rejected - so this is not documentation drift. But the argument that got NUL rejected in the first round, that a control character bound to a schema field or resolved against metalake membership cannot be what an author meant, applies to `\u0001`-`\u001f` the same way, and a column name containing a newline is hard to read in any error message or audit log that quotes it. Extending the check on this line, for example rejecting `current < 0x20` rather than only `'\0'`, would cover all three call sites at once, since `Column.validate()` (Column.java:39), `GroupMembership.validate()` and the STRING case of `Literal.validate()` (Literal.java:60) all route through this helper. Whether to also reject `\u007f` is a judgement call worth stating in the docs either way. Verified by: ran each of the listed programs through `RestrictedRegoExpressionParserFacade.validateRowFilter` on the standalone build of this head; all accepted, while the raw-tab form is rejected by the lexer with `unescaped control character in string` and the NUL form with `column name cannot contain NUL`. -- 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]
