[enhance](nereids) improve masking of user's password for ALTER USER and CREATE USER commands in audit logs#62141
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run feut |
9851a15 to
7ddf5e0
Compare
|
run feut |
|
run feut |
1 similar comment
|
run feut |
FE UT Coverage ReportIncrement line coverage |
|
run feut |
4 similar comments
|
run feut |
|
run feut |
|
run feut |
|
run feut |
53a743b to
be55c79
Compare
|
run feut |
3 similar comments
|
run feut |
|
run feut |
|
run feut |
|
run buildall |
FE UT Coverage ReportIncrement line coverage |
|
/review |
There was a problem hiding this comment.
LGTM overall. I did not find any blocking correctness issues in the changed Nereids audit encryption path.
Critical checkpoint conclusions:
- Goal and correctness: The PR goal is to mask passwords in audit output for
CREATE USERandALTER USER. The final code achieves that in the Nereids path by markingAlterUserCommandasNeedAuditEncryptionand masking the password token invisitGrantUserIdentify, which is the shared parser path used by bothCREATE USERandALTER USER. - Scope and minimality: The change is small and focused. The parser change only labels the existing password literal in
grantUserIdentify, the masking logic is centralized in one existing encryption visitor hook, and the command side change forALTER USERis minimal. - Concurrency: No new concurrency or locking behavior is introduced.
- Lifecycle and initialization: No special lifecycle or static initialization risks introduced.
- Configuration: No new config items added.
- Compatibility: No incompatible storage, protocol, or symbol changes. Grammar label changes are internal to the parser visitor and do not affect external compatibility.
- Parallel paths: I checked the shared
grantUserIdentifypath and the audit hook inAuditLogHelper; the implementation now covers both NereidsCREATE USERandALTER USERconsistently. - Conditional checks: The new null check on
ctx.pwdis straightforward and necessary becauseIDENTIFIED BY ...is optional in the grammar. - Test coverage: The PR adds focused unit coverage for both
CREATE USERandALTER USERmasking. I could not fully validate by runningEncryptSQLTestin this runner because FE Maven resolution failed on missingorg.apache.doris:fe-foundation:1.2-SNAPSHOT, so local execution here was blocked by environment dependency resolution rather than a code failure. - Observability: Existing audit logging path remains the same; no extra observability appears necessary for this change.
- Transaction and persistence: Not applicable.
- Data write and atomicity: Not applicable.
- FE and BE variable passing: Not applicable.
- Performance: The change reuses the existing audit encryption reparse path and adds no meaningful extra overhead beyond paths already using
NeedAuditEncryption. - Other issues: None identified in the reviewed scope.
Residual risk:
- The added tests cover the plain
IDENTIFIED BYform. The shared implementation should also mask theIDENTIFIED BY PASSWORDform, but that variant is not explicitly covered by this PR test coverage.
f2bbb44 to
cd0cedd
Compare
|
run buildall |
1 similar comment
|
run buildall |
FE Regression Coverage ReportIncrement line coverage |
9dbb47f to
117bf77
Compare
|
run buildall |
2 similar comments
|
run buildall |
|
run buildall |
|
Hello, @morrySnow / @starocean999 |
|
run buildall |
FE Compile error |
cf4e875
|
run feut |
|
run buildall |
|
PR approved by at least one committer and no changes requested. |
What problem does this PR solve?
This PR adds masking for users passwords for CREATE USER and ALTER USER commands.
The masked values will be stored in audit table and audit files instead of actual values.
The same functionality already exists for SET USER PASSWORD and SET LDAP_ADMIN_PASSWORD commands, so we other commands related to passwords should be masked as well.
Could you please include this PR into 4.x branches, please!
Issue Number: close #62140
Related PR: #xxx
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Check List (For Reviewer who merge this PR)