github-actions[bot] commented on code in PR #66006:
URL: https://github.com/apache/doris/pull/66006#discussion_r3651503206
##########
fe/fe-core/src/test/java/org/apache/doris/qe/StmtExecutorTest.java:
##########
@@ -472,4 +484,74 @@ public void
testShouldDisableCloudVersionCacheOnRetryForE230() {
connectContext.getSessionVariable().cloudTableVersionCacheTtlMs =
originalTableTtl;
}
}
+
+ @Test
+ public void testEmptyOriginStmtSkipsAuditMaskingReparse() throws Exception
{
+ org.apache.doris.nereids.trees.plans.logical.LogicalPlan logicalPlan =
Mockito.mock(
+ org.apache.doris.nereids.trees.plans.logical.LogicalPlan.class,
+ Mockito.withSettings().extraInterfaces(
+
org.apache.doris.nereids.trees.plans.commands.NeedAuditEncryption.class));
+ Mockito.doThrow(new AssertionError("empty SQL should not trigger audit
masking reparse"))
+
.when((org.apache.doris.nereids.trees.plans.commands.NeedAuditEncryption)
logicalPlan)
+ .geneEncryptionSQL("");
+
+ org.apache.doris.analysis.StatementBase parsedStmt = new
org.apache.doris.nereids.glue.LogicalPlanAdapter(
+ logicalPlan, new org.apache.doris.nereids.StatementContext());
+ parsedStmt.setOrigStmt(new OriginStatement("", 0));
+ StmtExecutor executor = new StmtExecutor(connectContext, parsedStmt);
+
+ // Empty internal SQL must bypass audit masking reparsing in both
logging paths.
+ Method getStmtForLogging =
StmtExecutor.class.getDeclaredMethod("getStmtForLogging", String.class);
+ getStmtForLogging.setAccessible(true);
+ Assertions.assertEquals("", getStmtForLogging.invoke(executor, ""));
+
+ Method getStmtForLoggingBeforeParse =
StmtExecutor.class.getDeclaredMethod("getStmtForLoggingBeforeParse");
+ getStmtForLoggingBeforeParse.setAccessible(true);
+ Assertions.assertEquals("",
getStmtForLoggingBeforeParse.invoke(executor));
+ }
+
+ @Test
+ public void testNeedAuditEncryptionStatementLogsMaskedSql() throws
Exception {
+ boolean originalPrintRequest =
Config.enable_print_request_before_execution;
+ Config.enable_print_request_before_execution = true;
+ try (TestLogAppender appender =
TestLogAppender.attach(StmtExecutor.class)) {
+ connectContext.getState().reset();
+ StmtExecutor stmtExecutor = new StmtExecutor(connectContext,
CREATE_AI_RESOURCE_SQL);
+ stmtExecutor.execute();
+
+
Assertions.assertFalse(appender.contains(org.apache.logging.log4j.Level.INFO,
"sk-test-secret"));
+
Assertions.assertTrue(appender.contains(org.apache.logging.log4j.Level.INFO,
"*XXX"));
+ } finally {
+ Config.enable_print_request_before_execution =
originalPrintRequest;
+ }
+ connectContext.getState().reset();
+ StmtExecutor showExecutor = new StmtExecutor(connectContext, "");
+ showExecutor.execute();
+ Assertions.assertEquals(QueryState.MysqlStateType.OK,
connectContext.getState().getStateType());
+ }
+
+ @Test
+ public void testAlterResourceSuccessLogDoesNotPrintResourceObject() throws
Exception {
+ createResource(CREATE_AI_RESOURCE_SQL);
Review Comment:
[P2] Reset the shared resource before each runtime test
`TestWithFeService` uses one `PER_CLASS` FE/Env, but both new tests create
the fixed `ai_resource_log_test` without removing it first. If the CREATE-log
test runs first, the ALTER test's setup fails on the duplicate; if the ALTER
test runs first, the CREATE-log test can take the duplicate-resource error path
and still pass because it never asserts the CREATE state and only observes the
pre-execution INFO log. Drop the resource before each setup (or use unique
names), then assert CREATE/ALTER success and the expected safe name/type log.
##########
fe/fe-core/src/main/java/org/apache/doris/qe/StmtExecutor.java:
##########
@@ -2390,6 +2394,40 @@ public String getOriginStmtInString() {
return "";
}
+ private String getStmtForLogging(String stmt) {
+ if (stmt == null || !(parsedStmt instanceof LogicalPlanAdapter)) {
+ return stmt;
+ }
+ // Internal export outfile tasks use an empty origin SQL, so audit
masking must skip reparsing here.
+ if (stmt.isEmpty()) {
+ return stmt;
+ }
+ LogicalPlan logicalPlan = ((LogicalPlanAdapter)
parsedStmt).getLogicalPlan();
+ if (!(logicalPlan instanceof NeedAuditEncryption)) {
+ return stmt;
+ }
+ return ((NeedAuditEncryption) logicalPlan).geneEncryptionSQL(stmt);
+ }
+
+ private String getStmtForLoggingBeforeParse() {
+ if (originStmt == null || originStmt.originStmt == null) {
+ return null;
+ }
+ // Empty SQL cannot produce a valid parse tree for audit masking, so
keep the original text.
+ if (originStmt.originStmt.isEmpty()) {
+ return originStmt.originStmt;
+ }
+ try {
+ LogicalPlan logicalPlan = new
NereidsParser().parseSingle(originStmt.originStmt);
+ if (!(logicalPlan instanceof NeedAuditEncryption)) {
+ return originStmt.originStmt;
+ }
+ return ((NeedAuditEncryption)
logicalPlan).geneEncryptionSQL(originStmt.originStmt);
+ } catch (Exception e) {
+ return originStmt.originStmt;
Review Comment:
[P1] Do not fall back to the raw statement on masking failure
When `enable_print_request_before_execution` is enabled, any exception from
`parseSingle()` or `geneEncryptionSQL()` reaches this branch and logs the
complete original SQL at INFO. A parser-accepted trigger (later rejected by
target validation) is `ALTER JOB j FROM MYSQL TO DATABASE d
("password"="secret")`: `sourceProperties` is optional, but the encryption
visitor dereferences it unconditionally, so masking throws and this catch
exposes the target password before validation. This path should fail closed
(for example, emit a redacted placeholder like the audit log's `Syntax Error`
handling), never return credential-bearing input.
##########
fe/fe-core/src/main/java/org/apache/doris/qe/StmtExecutor.java:
##########
@@ -756,7 +757,7 @@ public void checkBlockRulesByScan(Planner planner) throws
AnalysisException {
private void executeByNereids(TUniqueId queryId) throws Exception {
if (LOG.isDebugEnabled()) {
- LOG.debug("Nereids start to execute query:\n {}",
originStmt.originStmt);
+ LOG.debug("Nereids start to execute query:\n {}",
getStmtForLogging(originStmt.originStmt));
Review Comment:
[P1] Parse before applying plan-based masking
`getStmtForLogging()` returns `stmt` whenever `parsedStmt` is not yet a
`LogicalPlanAdapter`, but this DEBUG emission runs before `parseByNereids()`.
That is a production path for forwarded requests: the master constructs the
proxy executor from a raw `OriginStatement`, so a forwarded `CREATE/ALTER
RESOURCE` still writes `ai.api_key` verbatim at DEBUG. The new test uses the
same raw constructor and enables DEBUG, but only checks INFO, so it misses the
captured secret. Please move statement-bearing logging after parsing or use a
fail-closed pre-parse masking path, and assert the secret is absent at every
captured level.
##########
fe/fe-core/src/main/java/org/apache/doris/qe/StmtExecutor.java:
##########
@@ -2390,6 +2394,40 @@ public String getOriginStmtInString() {
return "";
}
+ private String getStmtForLogging(String stmt) {
Review Comment:
[P1] Route every statement-bearing log through the masking path
Adding this helper to selected call sites leaves later query logs
unprotected. A `SELECT` from an S3 TVF makes
`UnboundResultSink.needAuditEncryption()` true, so the changed initial/planning
logs mask `s3.secret_key`, but `handleQueryStmt()` still logs
`originStmt.originStmt` at DEBUG and retry paths log the same raw statement at
DEBUG/WARN. Please centralize the masked statement (or use this helper
consistently) across query handling and retries, and add a TVF runtime/retry
test.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/parser/LogicalPlanBuilderForEncryption.java:
##########
@@ -217,9 +227,8 @@ public LogicalPlan
visitTableValuedFunction(DorisParser.TableValuedFunctionConte
// create job select tvf
@Override
public LogicalPlan
visitCreateScheduledJob(DorisParser.CreateScheduledJobContext ctx) {
- if (ctx.supportedDmlStatement() != null) {
- SupportedDmlStatementContext supportedDmlStatementContext =
ctx.supportedDmlStatement();
- visitInsertTable((InsertTableContext)
supportedDmlStatementContext);
+ if (ctx.supportedDmlStatement() instanceof InsertTableContext) {
Review Comment:
[P1] Mask the enclosing and target job properties too
These job visitors can return normally while credential values remain in the
rewritten SQL. They visit only an `InsertTableContext` or `sourceProperties`;
neither the CREATE/ALTER job-level property clause nor the independently
optional `targetProperties` is encrypted. For example, a CREATE JOB with
`PROPERTIES ("password"="job-secret") ... DO INSERT ...` takes the supported
insert branch but leaves `job-secret` unchanged, and a FROM/TO job with
non-null source properties leaves a target `"password"` unchanged. Please
encrypt every non-null job, source, and target property range and add
assertions for those outer ranges.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/parser/LogicalPlanBuilderForEncryption.java:
##########
@@ -217,9 +227,8 @@ public LogicalPlan
visitTableValuedFunction(DorisParser.TableValuedFunctionConte
// create job select tvf
@Override
public LogicalPlan
visitCreateScheduledJob(DorisParser.CreateScheduledJobContext ctx) {
- if (ctx.supportedDmlStatement() != null) {
- SupportedDmlStatementContext supportedDmlStatementContext =
ctx.supportedDmlStatement();
- visitInsertTable((InsertTableContext)
supportedDmlStatementContext);
+ if (ctx.supportedDmlStatement() instanceof InsertTableContext) {
Review Comment:
[P1] Traverse non-insert job DML instead of skipping it
This avoids the cast exception by visiting only `InsertTableContext`, but
`supportedDmlStatement` also includes `INSERT INTO TVF`, `UPDATE`, `DELETE`,
`MERGE`, `LOAD`, `EXPORT`, `REPLAY`, and `COPY`, all of which can contain
sensitive property maps or TVF relations. `super.visitCreateScheduledJob()`
only copies the nested SQL text and does not walk that subtree, so examples
such as `UPDATE ... FROM S3(...)` or job-nested LOAD/EXPORT keep their
credentials unchanged. Please dispatch the encryption visitor over the actual
DML context and add secret-bearing non-insert cases, rather than silently
skipping them. The same issue exists in `visitAlterJob()`.
##########
fe/fe-core/src/main/java/org/apache/doris/qe/StmtExecutor.java:
##########
@@ -2390,6 +2394,40 @@ public String getOriginStmtInString() {
return "";
}
+ private String getStmtForLogging(String stmt) {
+ if (stmt == null || !(parsedStmt instanceof LogicalPlanAdapter)) {
+ return stmt;
+ }
+ // Internal export outfile tasks use an empty origin SQL, so audit
masking must skip reparsing here.
+ if (stmt.isEmpty()) {
+ return stmt;
+ }
+ LogicalPlan logicalPlan = ((LogicalPlanAdapter)
parsedStmt).getLogicalPlan();
+ if (!(logicalPlan instanceof NeedAuditEncryption)) {
+ return stmt;
+ }
+ return ((NeedAuditEncryption) logicalPlan).geneEncryptionSQL(stmt);
Review Comment:
[P1] Keep masking failures from changing command behavior
`geneEncryptionSQL()` can throw here, and this helper is now used in
execution and error paths. A target-only `ALTER JOB ... FROM MYSQL TO DATABASE
... ("table.create.properties.replication_num"="1")` may legitimately omit
`sourceProperties`, but the encryption visitor dereferences that optional
field. With DEBUG enabled, the line-760 log therefore aborts the command before
`doRun()`; while constructing an error message, the same failure can replace
the real command exception. Masking must be non-throwing and fail closed to a
safe placeholder so logging cannot affect execution semantics.
--
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]
