Repository navigation
ddl,privilege,parser: add masking policy dynamic privileges and error codes - #69107
tiancaiamao wants to merge 5 commits into
Conversation
… codes - Register CREATE/ALTER/DROP MASKING POLICY as dynamic privileges - Add privilege checks in all masking policy executor entry points - Replace generic errors with ErrMaskingPolicyExists/ErrMaskingPolicyNotExists - Support unquoted masking policy dynamic privileges in GRANT/REVOKE syntax - Add integration tests for masking policy privilege isolation
|
Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
ti-chi-bot
Bot
added
do-not-merge/needs-triage-completed
do-not-merge/release-note-label-needed
|
@tiancaiamao I've received your pull request and will start the review. I'll conduct a thorough review covering code quality, potential issues, and implementation details. ⏳ This process typically takes 10-30 minutes depending on the complexity of the changes. ℹ️ Learn more details on Pantheon AI. |
ti-chi-bot
Bot
added
the
size/XXL
|
Hi @tiancaiamao. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughRegisters CREATE/ALTER/DROP MASKING POLICY as dynamic privileges with typed error codes, extends the parser to recognize these privilege forms, implements dynamic privilege verification in masking-policy DDL operations via a shared helper, standardizes masking-policy existence/uniqueness error handling with typed variables, and validates enforcement through integration tests. ChangesMasking Policy Dynamic Privilege Enforcement
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)Command failed Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/ddl/executor.go (1)
6478-6522:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRequire
ALTER MASKING POLICYforCREATE OR REPLACE.Line 6517 switches this path to replace semantics, and Line 6810 forwards
ReplaceOnExist, so this statement can overwrite an existing masking policy after only passing theCREATE MASKING POLICYcheck at Line 6482. That gives a create-only user a way to mutate an existing policy. RequireALTER MASKING POLICYtoo whenstmt.OrReplaceis set; doing it unconditionally avoids a TOCTOU gap between precheck and job execution.Suggested fix
if stmt.OrReplace && stmt.IfNotExists { return dbterror.ErrWrongUsage.GenWithStackByArgs("OR REPLACE", "IF NOT EXISTS") } if err := requireMaskingPolicyDynamicPrivilege(ctx, "CREATE MASKING POLICY"); err != nil { return err } + if stmt.OrReplace { + if err := requireMaskingPolicyDynamicPrivilege(ctx, "ALTER MASKING POLICY"); err != nil { + return err + } + }Also applies to: 6808-6811
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/ddl/executor.go` around lines 6478 - 6522, The CreateMaskingPolicy path currently only calls requireMaskingPolicyDynamicPrivilege("CREATE MASKING POLICY") which allows a user with create-only rights to use stmt.OrReplace to overwrite existing policies; update CreateMaskingPolicy so that when stmt.OrReplace is true it also requires the ALTER MASKING POLICY privilege (call requireMaskingPolicyDynamicPrivilege with "ALTER MASKING POLICY") before proceeding, and perform this check unconditionally in the pre-check section (before calling createMaskingPolicyWithInfo and before determining onExist/OnExistReplace) to avoid any TOCTOU gap between privilege verification and job execution.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@pkg/ddl/executor.go`:
- Around line 6478-6522: The CreateMaskingPolicy path currently only calls
requireMaskingPolicyDynamicPrivilege("CREATE MASKING POLICY") which allows a
user with create-only rights to use stmt.OrReplace to overwrite existing
policies; update CreateMaskingPolicy so that when stmt.OrReplace is true it also
requires the ALTER MASKING POLICY privilege (call
requireMaskingPolicyDynamicPrivilege with "ALTER MASKING POLICY") before
proceeding, and perform this check unconditionally in the pre-check section
(before calling createMaskingPolicyWithInfo and before determining
onExist/OnExistReplace) to avoid any TOCTOU gap between privilege verification
and job execution.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3e354e14-556a-4213-aebf-c0e44784f622
📒 Files selected for processing (8)
pkg/ddl/executor.gopkg/ddl/masking_policy.gopkg/parser/parser.gopkg/parser/parser.ypkg/privilege/privileges/privileges.gopkg/util/dbterror/ddl_terror.gotests/integrationtest/r/executor/executor.resulttests/integrationtest/t/privilege/privileges.test
tiancaiamao
removed
do-not-merge/release-note-label-needed
|
Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
ti-chi-bot
Bot
added
do-not-merge/release-note-label-needed
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #69107 +/- ##
================================================
+ Coverage 76.3235% 76.4523% +0.1287%
================================================
Files 2041 2059 +18
Lines 562593 580729 +18136
================================================
+ Hits 429391 443981 +14590
- Misses 132291 134319 +2028
- Partials 911 2429 +1518
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
| if stmt.OrReplace && stmt.IfNotExists { | ||
| return dbterror.ErrWrongUsage.GenWithStackByArgs("OR REPLACE", "IF NOT EXISTS") | ||
| } | ||
| if err := requireMaskingPolicyDynamicPrivilege(ctx, "CREATE MASKING POLICY"); err != nil { |
There was a problem hiding this comment.
CREATE OR REPLACE only checks CREATE MASKING POLICY. It should also require ALTER MASKING POLICY for replacement.
There was a problem hiding this comment.
This is by design.
ALTER MASKING POLICY is not used for alter the masking policy's name.
It's changing the value of a masking policy (enable or disable, set a new expression, change RESTRICT ON etc)
There was a problem hiding this comment.
Rename a masking policy becomes recreate one, and it requires CREATE MASKING POLICY
| drop table if exists privilege__privileges.mask_t; | ||
| create table privilege__privileges.mask_t (c char(10)); | ||
| CREATE USER mask_creator, mask_alter, mask_drop, mask_none; | ||
| GRANT ALTER ON privilege__privileges.mask_t TO mask_creator; |
There was a problem hiding this comment.
CREATE MASKING POLICY still misses table ALTER permission. A user can add a policy without table ALTER.
|
[FORMAT CHECKER NOTIFICATION] Notice: To remove the For example:
📖 For more info, you can check the "Contribute Code" section in the development guide. |
|
/retest |
|
@tiancaiamao: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@expxiaoli: adding LGTM is restricted to approvers and reviewers in OWNERS files. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: expxiaoli The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
[LGTM Timeline notifier]Timeline:
|
This branch has not been deployed
2 participants
What problem does this PR solve?
Issue Number: ref #65744
Problem Summary: Masking policy DDL operations had no privilege checks. Any user with table-level ALTER privilege could create/drop/modify masking policies. Generic
errors.Errorfwere used instead of proper error codes.What is changed and how it works?
Dynamic privileges — Register
CREATE MASKING POLICY,ALTER MASKING POLICY,DROP MASKING POLICYas dynamic privileges inpkg/privilege/privileges/privileges.go.Privilege enforcement — Add
requireMaskingPolicyDynamicPrivilege()helper inpkg/ddl/executor.goand call it at all 6 masking policy executor entry points:CreateMaskingPolicy→ CREATE MASKING POLICYAddMaskingPolicy→ CREATE MASKING POLICYAlterTableMaskingPolicyState→ ALTER MASKING POLICYDropMaskingPolicy→ DROP MASKING POLICYModifyMaskingPolicyExpression→ ALTER MASKING POLICYModifyMaskingPolicyRestrictOn→ ALTER MASKING POLICYError codes — Replace
errors.Errorf("masking policy ...")withdbterror.ErrMaskingPolicyExists/dbterror.ErrMaskingPolicyNotExistsin executor and worker layers.Parser — Add GRANT/REVOKE parsing rules for unquoted
CREATE MASKING POLICY,ALTER MASKING POLICY,DROP MASKING POLICYinpkg/parser/parser.y.Tests — Add privilege isolation integration tests.
Check List
Tests
Side effects
SHOW PRIVILEGESoutput updated (3 new dynamic privilege rows)Summary by CodeRabbit
New Features
Bug Fixes
Tests