wenzhenghu commented on code in PR #66006:
URL: https://github.com/apache/doris/pull/66006#discussion_r3654107692
##########
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:
Thanks, agreed. This is in scope for this PR because it can still expose
ai.api_key in the pre-parse DEBUG path for forwarded CREATE/ALTER RESOURCE
statements. I changed the DEBUG log to use a pre-parse masking helper, made
that helper fail closed, and added assertions that the secret is absent from
both INFO and DEBUG logs. Fixed in 3207a315876.
##########
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:
Thanks. This is a broader masking-coverage issue for CREATE or ALTER JOB
nested DML, but it does not affect the scoped problem this PR is fixing:
plaintext ai.api_key leakage for CREATE or ALTER RESOURCE in FE logs and the
audit log. I would prefer to keep this PR scoped to the resource statement
paths.
##########
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:
Thanks. This concerns broader CREATE or ALTER JOB masking coverage. It does
not affect the scoped problem this PR is fixing, which is ai.api_key leakage
for CREATE or ALTER RESOURCE log paths, so I am not expanding this PR further
here.
##########
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:
Thanks. This is a broader follow-up about other statement types and later
query or retry logging paths such as TVF runtime logs. It does not affect the
scoped CREATE or ALTER RESOURCE ai.api_key leakage that this PR is fixing, so I
am keeping this PR limited to the resource statement paths.
##########
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:
Thanks, agreed. Logging must not change execution semantics. I made the FE
logging helpers non-throwing and fail closed to a safe placeholder so masking
exceptions no longer abort execution or replace the original error. Added
dedicated unit coverage. Fixed in 3207a315876.
--
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]
