SEM: add strict mode for next-gen builds#67945
Conversation
Signed-off-by: AmoebaProtozoa <8039876+AmoebaProtozoa@users.noreply.github.com>
Signed-off-by: AmoebaProtozoa <8039876+AmoebaProtozoa@users.noreply.github.com>
ti-chi-bot
Bot
added
do-not-merge/invalid-title
do-not-merge/needs-linked-issue
do-not-merge/needs-tests-checked
release-note-none
|
Skipping CI for Draft Pull Request. |
|
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:
📝 WalkthroughWalkthroughAdds a concurrency-safe strict SEM flag, implements strict Starter Edition Mode checks for statements and optimizer hints, integrates strict checks into planner and session (pipelined DML), wires strict enabling at server startup, and adds unit/integration tests and BUILD dependency updates. ChangesStrict SEM Feature Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
ti-chi-bot
Bot
added
size/XXL
|
Hi @AmoebaProtozoa. 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. |
AmoebaProtozoa
changed the title
ti-chi-bot
Bot
added
release-note
AmoebaProtozoa
changed the title
AmoebaProtozoa
changed the title
|
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
ti-chi-bot
Bot
removed
the
do-not-merge/work-in-progress
|
Review failed due to infrastructure/execution failure after retries. Please re-trigger review. ℹ️ Learn more details on Pantheon AI. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/planner/optimize.go (1)
233-244:⚠️ Potential issue | 🟡 MinorStripped-hint warnings may suppress the later "hints ignored, using bindSQL" notice.
originStmtHintsis now parsed from the already-filtered hint list. If strict SEM strips every user hint,hint.ParseStmtHintswill seelen(hints) == 0and setQueryHasHints = false, so the check at line 351 (if originStmtHints.QueryHasHints { ... "The system ignores the hints in the current query ..." }) will silently skip that note for queries whose only hints happened to be restricted. The user still gets a per-hint restricted warning, so this is minor, but worth a conscious decision: either computeQueryHasHintsfrom the pre-filter slice, or accept the reduced note.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/optimize.go` around lines 233 - 244, The current code parses originStmtHints from the already-filtered tableHints so QueryHasHints can become false if all user hints were stripped; to fix, preserve or inspect the pre-filter hint slice when computing QueryHasHints: call hint.ParseStmtHints using the original extracted hints (before filterRestrictedHints) or, after parsing the filtered originStmtHints, explicitly set originStmtHints.QueryHasHints = len(originalExtractedHints) > 0 (where originalExtractedHints is the slice returned by hint.ExtractTableHintsFromStmtNode before filterRestrictedHints). Ensure you still append restrictedHintWarns to sessVars.StmtCtx and then assign sessVars.StmtCtx.StmtHints = originStmtHints so downstream checks (e.g., the QueryHasHints check) reflect whether the user actually provided hints before filtering.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/tidb-server/main.go`:
- Around line 1165-1191: The current setupSEM enables classic sem (sem.Enable())
when SEMConfig is empty which leaves semv2 global state nil and makes
EnableStrictSEM ineffective; change setupSEM so EnableStrictSEM requires
cfg.Security.SEMConfig to be non-empty: if cfg.Security.SEMConfig == "" and
cfg.Security.EnableStrictSEM is true, log a clear warning (via
logutil.BgLogger().Warn) that strict SEM requires a semv2 config and
ignore/disable strict mode, then continue with classic sem; only call
semv2.EnableStrict() after successfully calling
semv2.Enable(cfg.Security.SEMConfig) (i.e., ensure semv2.Enable returned no
error and global semv2 state is initialized before calling semv2.EnableStrict())
and do not call semv2.EnableStrict() when semv2 was not enabled.
In `@pkg/planner/core/planbuilder.go`:
- Around line 6382-6389: The strict SEM check (semv2.IsStrictEnabled()) is being
executed before verifying SEM is enabled, causing strict mode to apply even when
SEM is disabled; update the logic so strict checks only run when
semv2.IsEnabled() is true (e.g., wrap the semv2.IsStrictEnabled() /
semv2.IsRestrictedStatement(stmt) block inside an if semv2.IsEnabled() { ... }
or change it to if semv2.IsEnabled() && semv2.IsStrictEnabled() { if err :=
semv2.IsRestrictedStatement(stmt); err != nil { return err } }).
In `@pkg/session/session.go`:
- Around line 5521-5524: Add a SQL-level test that enables strict SEM via
semv2.EnableStrict(), sets tidb_dml_type='bulk', executes a DML (e.g.,
INSERT/UPDATE) and asserts the presence of the warning text "Pipelined DML is
not supported in this deployment. Fallback to standard mode" using SHOW
WARNINGS; the test should exercise the usePipelinedDmlOrWarn code path (which
calls semv2.IsStrictEnabled()) and then disable strict mode after the test to
avoid global state leakage. Ensure the test file mirrors existing pipelined-DML
tests (e.g., pipelineddml_test.go) for setup/teardown and uses the same
assertion pattern for SHOW WARNINGS.
In `@pkg/util/sem/v2/restricted_hint.go`:
- Around line 30-31: The "resource_group" branch currently unconditionally
returns an error referencing strict SEM; update the branch in restricted_hint.go
(the case for "resource_group" in the switch handling hints) to follow the same
SEM-aware pattern as the other cases: either check IsStrictEnabled() (or the
same sysvar visibility helper used by
memory_quota/read_consistent_replica/max_execution_time) and only return the
"not supported when strict SEM is enabled" error when strict SEM is active, or
change the error text to a SEM-agnostic message that does not mention strict
SEM; pick one approach and apply it in the "resource_group" case so behavior and
messaging match the other hint checks.
---
Outside diff comments:
In `@pkg/planner/optimize.go`:
- Around line 233-244: The current code parses originStmtHints from the
already-filtered tableHints so QueryHasHints can become false if all user hints
were stripped; to fix, preserve or inspect the pre-filter hint slice when
computing QueryHasHints: call hint.ParseStmtHints using the original extracted
hints (before filterRestrictedHints) or, after parsing the filtered
originStmtHints, explicitly set originStmtHints.QueryHasHints =
len(originalExtractedHints) > 0 (where originalExtractedHints is the slice
returned by hint.ExtractTableHintsFromStmtNode before filterRestrictedHints).
Ensure you still append restrictedHintWarns to sessVars.StmtCtx and then assign
sessVars.StmtCtx.StmtHints = originStmtHints so downstream checks (e.g., the
QueryHasHints check) reflect whether the user actually provided hints before
filtering.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: eded8646-e4a3-40db-a5f7-b6b1b77d1b34
📒 Files selected for processing (13)
cmd/tidb-server/main.gopkg/config/config.gopkg/planner/BUILD.bazelpkg/planner/core/planbuilder.gopkg/planner/optimize.gopkg/session/BUILD.bazelpkg/session/session.gopkg/util/sem/v2/BUILD.bazelpkg/util/sem/v2/restricted_hint.gopkg/util/sem/v2/restricted_hint_test.gopkg/util/sem/v2/restricted_statement.gopkg/util/sem/v2/restricted_statement_test.gopkg/util/sem/v2/strict.go
| // Strict SEM applies before the semv2 rule check with no admin bypass. | ||
| if semv2.IsStrictEnabled() { | ||
| if err := semv2.IsRestrictedStatement(stmt); err != nil { | ||
| return err | ||
| } | ||
| } | ||
|
|
||
| if !semv2.IsEnabled() { |
There was a problem hiding this comment.
Gate strict SEM on SEM being enabled.
Line 6383 enforces strict mode before checking semv2.IsEnabled(), so a process with IsStrictEnabled()==true but SEM disabled would still reject statements, contrary to the config contract that strict SEM is ignored unless security.enable-sem=true.
Suggested fix
+ semEnabled := semv2.IsEnabled()
// Strict SEM applies before the semv2 rule check with no admin bypass.
- if semv2.IsStrictEnabled() {
+ if semEnabled && semv2.IsStrictEnabled() {
if err := semv2.IsRestrictedStatement(stmt); err != nil {
return err
}
}
- if !semv2.IsEnabled() {
+ if !semEnabled {
return nil
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/planner/core/planbuilder.go` around lines 6382 - 6389, The strict SEM
check (semv2.IsStrictEnabled()) is being executed before verifying SEM is
enabled, causing strict mode to apply even when SEM is disabled; update the
logic so strict checks only run when semv2.IsEnabled() is true (e.g., wrap the
semv2.IsStrictEnabled() / semv2.IsRestrictedStatement(stmt) block inside an if
semv2.IsEnabled() { ... } or change it to if semv2.IsEnabled() &&
semv2.IsStrictEnabled() { if err := semv2.IsRestrictedStatement(stmt); err !=
nil { return err } }).
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #67945 +/- ##
================================================
- Coverage 77.7126% 75.6064% -2.1062%
================================================
Files 1991 2011 +20
Lines 552142 570709 +18567
================================================
+ Hits 429084 431493 +2409
- Misses 122138 138614 +16476
+ Partials 920 602 -318
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Signed-off-by: AmoebaProtozoa <8039876+AmoebaProtozoa@users.noreply.github.com>
Inserting the StarterEssential field with a leading comment split the Config struct's tag-alignment block, so gofmt wants narrower padding for the post-comment fields. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ict-SEM pipelined-DML test - IsRestrictedHint now self-gates on IsStrictEnabled, matching the documented contract; the four hint tests pivot to enabling strict before each scenario. - Doc-comment now spells out that resource_group is rejected outright under starter/essential because resource group management is unavailable in those deployments. - TestPipelinedDMLNegative now covers the strict-SEM fallback warning. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…IsStarter() Upstream master added a process-wide next-gen deployment mode (pkg/config/deploymode, pingcap#68178). Replace the temporary `starter-essential` TiDB config flag that previously gated the strict-SEM allow-list with `deploymode.IsStarter()`: - Drop Config.StarterEssential (field, default, and the next-gen-only Valid() check) -- deploymode.IsStarter() already returns false on classic builds, so the kernel check is redundant. - setupSEM() now enables strict SEM iff deploymode.IsStarter(); it still requires security.enable-sem=true (strict SEM layers on top of SEM) and still warns + no-ops when Starter mode is set but SEM is disabled. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
|
/retest |
|
@AmoebaProtozoa: 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. |
Signed-off-by: AmoebaProtozoa <8039876+AmoebaProtozoa@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/util/sem/v2/restricted_statement.go (1)
186-186: 💤 Low valueDead code:
AdminStmtcase is unreachable.
AdminStmtis already handled directly inIsRestrictedStatement(line 94-95) before reachingverifySimple. SinceAdminStmtis not in the case list at lines 102-108, this allowlist entry will never match.♻️ Remove unreachable case
case *ast.FlushStmt, *ast.BeginStmt, *ast.CommitStmt, *ast.SavepointStmt, *ast.ReleaseSavepointStmt, *ast.RollbackStmt, *ast.CreateUserStmt, *ast.AlterUserStmt, *ast.SetPwdStmt, *ast.SetSessionStatesStmt, *ast.KillStmt, *ast.BinlogStmt, *ast.DropStatsStmt, - *ast.AdminStmt, *ast.GrantStmt, *ast.RevokeStmt, *ast.NonTransactionalDMLStmt, *ast.UseStmt: return nil🤖 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/util/sem/v2/restricted_statement.go` at line 186, The switch in verifySimple contains an unreachable AdminStmt case because IsRestrictedStatement already handles *ast.AdminStmt earlier; remove the AdminStmt entry from the case list in verifySimple (or from the allowlist used there) so the unreachable branch is deleted, leaving only the actually reachable statement types referenced in verifySimple and ensuring no dead-case remains.
🤖 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.
Inline comments:
In `@pkg/util/sem/v2/restricted_statement.go`:
- Around line 206-219: GrantRoleStmt and RevokeRoleStmt currently only check
Roles but must also block operations that target restricted users similar to
SetDefaultRoleStmt; update the handlers for GrantRoleStmt and RevokeRoleStmt to
iterate over s.Users and call isRestrictedUser on each entry, returning
notSupported with an appropriate message if any target user is restricted. Use
the existing symbols GrantRoleStmt, RevokeRoleStmt, Users, Roles and
isRestrictedUser to locate the code and implement the additional check so
GRANT/REVOKE cannot modify restricted accounts' role sets.
---
Nitpick comments:
In `@pkg/util/sem/v2/restricted_statement.go`:
- Line 186: The switch in verifySimple contains an unreachable AdminStmt case
because IsRestrictedStatement already handles *ast.AdminStmt earlier; remove the
AdminStmt entry from the case list in verifySimple (or from the allowlist used
there) so the unreachable branch is deleted, leaving only the actually reachable
statement types referenced in verifySimple and ensuring no dead-case remains.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 04cdc6d4-3420-4138-9a0a-66f7a686f0a3
📒 Files selected for processing (3)
pkg/util/sem/v2/restricted_statement.gopkg/util/sem/v2/restricted_statement_test.gotests/realtikvtest/pipelineddmltest/pipelineddml_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/realtikvtest/pipelineddmltest/pipelineddml_test.go
| case *ast.GrantRoleStmt: | ||
| for _, role := range s.Roles { | ||
| if isRestrictedRole(role.Username, role.Hostname) { | ||
| return notSupported(fmt.Sprintf("GRANT ROLE %s", role)) | ||
| } | ||
| } | ||
| return nil | ||
| case *ast.RevokeRoleStmt: | ||
| for _, role := range s.Roles { | ||
| if isRestrictedRole(role.Username, role.Hostname) { | ||
| return notSupported(fmt.Sprintf("REVOKE %s", role)) | ||
| } | ||
| } | ||
| return nil |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if GrantRoleStmt and RevokeRoleStmt have a Users field to verify the fix is applicable
ast-grep --pattern $'type GrantRoleStmt struct {
$$$
}'
ast-grep --pattern $'type RevokeRoleStmt struct {
$$$
}'Repository: pingcap/tidb
Length of output: 587
🏁 Script executed:
# Check the actual code at lines 206-219 and surrounding context
cat -n pkg/util/sem/v2/restricted_statement.go | sed -n '200,230p'Repository: pingcap/tidb
Length of output: 1352
🏁 Script executed:
# Check SetDefaultRoleStmt handling around lines 248-252
cat -n pkg/util/sem/v2/restricted_statement.go | sed -n '240,260p'Repository: pingcap/tidb
Length of output: 1060
🏁 Script executed:
# Check what isRestrictedUser and isRestrictedRole functions do
rg -A 5 "func isRestrictedUser|func isRestrictedRole" pkg/util/sem/v2/Repository: pingcap/tidb
Length of output: 864
Block role operations targeting restricted users, consistent with SetDefaultRoleStmt.
GrantRoleStmt and RevokeRoleStmt only check whether the role being granted/revoked is restricted, but do not check the target users. In contrast, SetDefaultRoleStmt (lines 248-251) explicitly blocks operations targeting restricted users with the justification "so [restricted accounts'] default role set cannot be tampered with."
This gap allows operations like GRANT 'some_role'@'%' TO 'cloud_admin'@'%' to pass, directly modifying the restricted account's role set—the same protection SetDefaultRoleStmt guards against. Add target-user checks to both GrantRoleStmt and RevokeRoleStmt handlers using the existing isRestrictedUser function on the Users field.
🤖 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/util/sem/v2/restricted_statement.go` around lines 206 - 219,
GrantRoleStmt and RevokeRoleStmt currently only check Roles but must also block
operations that target restricted users similar to SetDefaultRoleStmt; update
the handlers for GrantRoleStmt and RevokeRoleStmt to iterate over s.Users and
call isRestrictedUser on each entry, returning notSupported with an appropriate
message if any target user is restricted. Use the existing symbols
GrantRoleStmt, RevokeRoleStmt, Users, Roles and isRestrictedUser to locate the
code and implement the additional check so GRANT/REVOKE cannot modify restricted
accounts' role sets.
Signed-off-by: AmoebaProtozoa <8039876+AmoebaProtozoa@users.noreply.github.com>
Signed-off-by: AmoebaProtozoa <8039876+AmoebaProtozoa@users.noreply.github.com>
|
[FORMAT CHECKER NOTIFICATION] Notice: To remove the For example:
📖 For more info, you can check the "Contribute Code" section in the development guide. |
|
@AmoebaProtozoa: The following test failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
1 participant
What problem does this PR solve?
Issue Number: ref #67765
Problem Summary:
Next-gen deployments need tighter restrictions than the current semv2 SQL rules can express: a fixed allow-list of statements and a matching reject list for query hints that could otherwise sidestep SEM-masked sysvars.
What changed and how does it work?
Adds
security.enable-strict-semto tidb.toml (defaultfalse). Honored only whensecurity.enable-sem=trueon a next-gen build; otherwise ignored with a startup warning.When active, the planner rejects statements outside the allow-list with
ErrNotSupportedWithSem(noRESTRICTED_SQL_ADMINbypass), stripsmemory_quota/read_consistent_replica/max_execution_time/resource_grouphints from both user queries and SQL bindings when the matching sysvar is SEM-hidden or SEM-readonly, blocksDROP USER/RENAME USER/GRANT|REVOKE|SET ROLEoncloud_admin@%androot@%, and falls pipelined DML back to standard with a warning.Check List
Tests
Side effects
Documentation
Release note
Summary by CodeRabbit