Repository navigation
[Feature](nereids) support encrypt sql for audit log - #47595
Merged
Merged
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
morrySnow
reviewed
Feb 11, 2025
Contributor
|
CreateTableCommand need to be encrypted in some case, and please check if CreateSqlBlockRuleCommand or other command is needed too |
Contributor
Author
SQL of |
|
run buildall |
starocean999
previously approved these changes
Feb 14, 2025
Contributor
|
PR approved by at least one committer and no changes requested. |
github-actions
Bot
added
the
approved
Contributor
|
PR approved by anyone and no changes requested. |
morrySnow
previously approved these changes
Feb 14, 2025
TPC-H: Total hot run time: 31492 ms |
TPC-DS: Total hot run time: 189899 ms |
ClickBench: Total hot run time: 30.17 s |
|
run buildall |
TPC-H: Total hot run time: 31617 ms |
TPC-DS: Total hot run time: 189210 ms |
ClickBench: Total hot run time: 30.18 s |
TPC-DS: Total hot run time: 190852 ms |
ClickBench: Total hot run time: 30.16 s |
morrySnow
approved these changes
Feb 19, 2025
starocean999
approved these changes
Feb 19, 2025
lzyy2024
pushed a commit
to lzyy2024/doris
that referenced
this pull request
Feb 21, 2025
### What problem does this PR solve? The new optimizer does not support the encryption of sql sensitive information well, so it implements a general encryption framework Co-authored-by: garenshi <[email protected]>
4 of 16 tasks
koarz
pushed a commit
to koarz/doris
that referenced
this pull request
Jun 4, 2025
### What problem does this PR solve? The new optimizer does not support the encryption of sql sensitive information well, so it implements a general encryption framework Co-authored-by: garenshi <[email protected]>
4 of 16 tasks
morningman
pushed a commit
that referenced
this pull request
Oct 30, 2025
…48527) ### What problem does this PR solve? Issue Number: close #xxx Related PR: #47595 Problem Summary: For PR #47595 , set `stmt` to `error messsage` of `org.apache.doris.qe.QueryState.ErrType#ANALYSIS_ERR` of query audit, so many of the original SQL will be abandoned, this is very unreasonable, It is now more appropriate to set stmt to error message only for queries that are syntax parsed incorrectly Now only sql with syntax errors is ignored Co-authored-by: garenshi <[email protected]>
github-actions Bot
pushed a commit
that referenced
this pull request
Oct 30, 2025
…48527) ### What problem does this PR solve? Issue Number: close #xxx Related PR: #47595 Problem Summary: For PR #47595 , set `stmt` to `error messsage` of `org.apache.doris.qe.QueryState.ErrType#ANALYSIS_ERR` of query audit, so many of the original SQL will be abandoned, this is very unreasonable, It is now more appropriate to set stmt to error message only for queries that are syntax parsed incorrectly Now only sql with syntax errors is ignored Co-authored-by: garenshi <[email protected]>
dwdwqfwe
pushed a commit
to dwdwqfwe/doris
that referenced
this pull request
Oct 31, 2025
…pache#48527) ### What problem does this PR solve? Issue Number: close #xxx Related PR: apache#47595 Problem Summary: For PR apache#47595 , set `stmt` to `error messsage` of `org.apache.doris.qe.QueryState.ErrType#ANALYSIS_ERR` of query audit, so many of the original SQL will be abandoned, this is very unreasonable, It is now more appropriate to set stmt to error message only for queries that are syntax parsed incorrectly Now only sql with syntax errors is ignored Co-authored-by: garenshi <[email protected]>
yiguolei
pushed a commit
that referenced
this pull request
Nov 5, 2025
…48527) ### What problem does this PR solve? Issue Number: close #xxx Related PR: #47595 Problem Summary: For PR #47595 , set `stmt` to `error messsage` of `org.apache.doris.qe.QueryState.ErrType#ANALYSIS_ERR` of query audit, so many of the original SQL will be abandoned, this is very unreasonable, It is now more appropriate to set stmt to error message only for queries that are syntax parsed incorrectly Now only sql with syntax errors is ignored Co-authored-by: garenshi <[email protected]>
starocean999
added a commit
to starocean999/incubator-doris
that referenced
this pull request
Nov 28, 2025
morrySnow
pushed a commit
that referenced
this pull request
Dec 1, 2025
morningman
added a commit
to morningman/doris
that referenced
this pull request
Nov 13, 2026
…characters ### Background Several FE features rewrite the text of a statement by cutting the original SQL string at token positions from the parser. Audit masking replaces credential properties and passwords with `*XXX` before the statement reaches `fe.audit.log`, `__internal_schema.audit_log` and the FE log; CREATE / ALTER VIEW and CREATE MATERIALIZED VIEW expand `*` and qualify names in the definition they store; the parser reads `/*+ ... */` hints from comment tokens. The lexer counts its input by Unicode code point, while a Java `String` holds a supplementary character, such as an emoji, as two `char`s. ### What problem does this PR solve? Issue Number: None Related PR: apache#47595 (introduced the bug in audit masking), apache#32743 (introduced the bug in view rewriting), apache#70338 (its review found the masking leak) PR Type: Bugfix Problem Summary: After each supplementary character in a statement, these token positions point one `char` too early, so the text is cut at the wrong place. A credential after or inside such characters stays partly or fully in clear text in the audit log, CREATE VIEW fails with a syntax error, CREATE MATERIALIZED VIEW stores a broken query, and a hint after such a character is silently ignored. This PR converts the positions to `String` offsets, or reads the text through the token, at the five places that cut the original SQL. Tests are 89% of the diff. ### Details - **Change map:** production code +33 -33 in 3 files; tests +240 in 5 files (FE unit tests and one regression suite); generated `.out` +16. (1) `BaseViewInfo.rewriteSql`, shared by audit masking, views and MTMV, converts its positions; the sync-MV builder drops its private copy of it, and `rewriteProjectsToUserDefineAlias` replaces the select list through it. (2) `NereidsParser.getHintMap` reads hints from the token text. (3) `NereidsParser.tryParseExplainPlan` converts the start of the EXPLAIN PLAN body. - **Frequency:** Always, when a character above U+FFFF (an emoji, CJK Extension B) comes before or inside the rewritten part, for example a catalog COMMENT of emoji in front of `'s3.secret_key' = 'secret'`. - **Root cause:** `CaseInsensitiveStream.fromString` keeps ANTLR's code-point stream for any input with a surrogate (before apache#67452 every input used `CharStreams.fromString`), so `Token.getStartIndex()` and `getStopIndex()` count code points. The five places in the Change map used them as `String` indexes. - **Why existing tests missed it:** Unit tests: the masking (`EncryptSQLTest`, `RepositoryAuditEncryptionTest`), view and MTMV (`CreateViewTest`, `CreateMTMVCommandTest`) and hint tests (`LeanTokenModeTest`) use only BMP text, where both kinds of position coincide; `CaseInsensitiveStreamTest` uses an emoji but checks only the stream's own positions. Regression: `audit/test_external_catalog_credential_masking` and the `ddl_p0/create_view_nereids` suites have no supplementary characters, and the masking suite only checks that the whole credential is absent, which a leaked tail passes. - **Fix:** Convert where the text is cut. `rewriteSql` converts each position with `String.offsetByCodePoints`, walking the sorted positions once so the cost stays linear; `rewriteProjectsToUserDefineAlias` reads expressions through the token's input stream; `getHintMap` reads the hint from the token text, as `removeCommentAndTrimBlank` does. Changing the stream to report `String` positions was rejected: apache#67452 made code-point positions part of `CaseInsensitiveStream`'s contract, and `CaseInsensitiveStreamTest` pins them. - **Other paths and branches:** Fixed here: the 19 masking visitors of `LogicalPlanBuilderForEncryption` with every caller of `geneEncryptionSQL` (audit log, FE statement log, `_query_plan`) and the streaming insert job; the view, MTMV and sync-MV rewrites. Not affected: the other uses of token positions read text through the token's own stream (such as `getOriginSql`) or only compare positions with each other. branch-3.1, branch-4.0, branch-4.1 and branch-4.2 have the same code. Introduced by apache#32743 (views), apache#47595 (masking), apache#33699 (sync MV) and apache#33262 (EXPLAIN PLAN); hints since at least apache#39590. ### Testing - New unit tests fail on master `f1bc8b3690c` and pass with the fix, run locally: `EncryptSQLTest#testEncryptionAfterSupplementaryCharacters`, through `MysqlConnectProcessor` to the audit event (emoji inside a credential, in a catalog COMMENT, in the first of two property clauses, in a user name before `PASSWORD(...)`; master left `secret`, `abc`, `123` and `123456` in clear text); `CreateViewTest#testCreateViewRewritesAfterSupplementaryCharacters` (master: `mismatched input '`e`'`); `CreateMTMVCommandTest#testCreateMTMVRewritesQuerySqlAfterSupplementaryCharacters` (master kept ``SELECT '😀😀' A`e` e`internal`...``); `SupplementaryCharacterOffsetTest` (master dropped the hint, cut the EXPLAIN PLAN body to `AN SELECT 100`, and rewrote the sync MV to `avg_stateg(k)))`). - New regression suite `ddl_p0/create_view_nereids/test_create_view_supplementary_chars`: fails without the fix with the same syntax error and passes with it; the `.out` is generated by the script. Run locally on one FE and one BE, an older master FE build with the three changed classes swapped in. - 27 existing FE UT classes around the change (467 tests) pass. Reverting each part of the fix alone, moving the end of a replaced span by one, or not advancing the position cursor each fails a new test; the off-by-one also fails existing ASCII cases. - By hand on that cluster, without the fix: the secret stayed in `audit_log`, `/* 😀 */ select /*+ SET_VAR(query_timeout=1) */ sleep(3)` ran for 3 s, and an MTMV over `select '😀😀' as e, ...` was created but failed to refresh (`Nereids parse failed`). With the fix: `"s3.secret_key" = "*XXX")`, and the query times out after 1 s. ### Notes for Reviewers - `getHintMap` loses its `sql` parameter, which nothing reads any more; its three callers are in `NereidsParser`. - `rewriteProjectsToUserDefineAlias` replaces the select list through `rewriteSql` instead of `StringUtils.overlay`, so the conversion lives in one method. - Stored definitions are not repaired: an MTMV created by an affected version from such a query keeps its broken definition and has to be recreated. ### Release note Fix passwords and credentials left partly or fully unmasked in the audit log and the FE log when a supplementary character, such as an emoji, comes before or inside them. Also fix CREATE VIEW failing, CREATE MATERIALIZED VIEW storing a broken query, and hints such as SET_VAR being ignored after such a character; materialized views already created with a broken query must be recreated. ### Check List (For Author) - Behavior changed: No - Does this need documentation: No - Pre-submission status: Ready ### AI Disclosure - Tool: Claude Code (Claude Opus 5.5) - Generated: the fix, the tests, and this description - Human review: The author reviewed part of the changes. - Prompts: After the review bot on apache#70338 reported the masking leak, the author confirmed it is an existing engine defect shared by all masking paths and asked for a separate engine PR that fixes it, followed by a reply on apache#70338 with the link. Co-Authored-By: Claude Opus 5.5 <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: close #xxx
The new optimizer does not support the encryption of sql sensitive information well, so it implements a general encryption framework
Related PR: #xxx
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)