[fix](point query) Revalidate security before prepared point-query reuse - #67880
Open
morrySnow wants to merge 2 commits into
Open
[fix](point query) Revalidate security before prepared point-query reuse#67880morrySnow wants to merge 2 commits into
morrySnow wants to merge 2 commits into
Conversation
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: Server-side prepared point queries retain a direct short-circuit execution context after their first execution. Reusing that context bypassed the normal privilege and policy analysis passes, so a SELECT revocation or a changed row-filter or data-mask policy could leave the retained plan authorized with stale decisions. Record the planning identity, checked columns, complete row-filter answers, and data-mask answers (including negative answers), then revalidate them before every direct reuse. Missing dependencies and authorization-source failures reject reuse and fall back to normal planning, where the authoritative check returns the standard result or error.
### Release note
Prepared point queries now honor current SELECT privileges and row-filter/data-mask policies on every execution.
### Check List (For Author)
- Test: Unit tests and regression test
- `SecurityDependencyContextTest`, `ExecuteCommandTest`, and `ShortCircuitQueryContextTest`
- `prepared_short_circuit_security_refresh`
- Behavior changed: Yes. A stale prepared point-query context is rebuilt after security decisions change.
- Does this need documentation: No
morrySnow
requested review from
924060929,
englefly and
starocean999
as code owners
September 11, 2026 16:51
Contributor
Author
|
run buildall |
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
morrySnow
changed the title
924060929
previously approved these changes
Sep 11, 2026
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
TPC-H: Total hot run time: 16519 ms |
Contributor
TPC-DS: Total hot run time: 81258 ms |
Contributor
ClickBench: Total hot run time: 14.55 s |
morrySnow
marked this pull request as draft
September 12, 2026 07:45
morrySnow
marked this pull request as ready for review
September 12, 2026 11:22
Contributor
Author
|
/review |
Contributor
|
Codex automated review failed and did not complete. Error: Selected model is at capacity. Please try a different model. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Contributor
Author
|
/review |
Avoid catalog, privilege, row-policy, and mask lookups on every prepared point-query execution when Doris built-in authorization is unchanged. External authorization and LDAP continue to use fail-closed full validation.
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
|
Codex automated review failed and did not complete. Error: Codex completed, but no new pull request review was submitted for the current head SHA. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Static review result: REQUEST_CHANGES. Review status: capped/incomplete because a distinct valuable finding was accepted in the third and final permitted convergence round.
- Goal and proof: the patch correctly invalidates ordinary built-in privilege/role and row-policy mutations and rechecks custom relation decisions, but it does not fully secure reusable prepared point plans. Three P1 stale-plan paths remain (authorization-subject change, inlined-view definition change, and database-namespace change), and the P2 revoke test can pass on an unrelated SQLException.
- Scope and clarity: the implementation is focused and the dependency abstraction is understandable, but its declared security-dependency boundary omits subject and metadata-generation inputs that materially change the meaning or authority of the cached plan.
- Concurrency: Auth and PolicyMgr writers publish volatile epochs under their existing writer discipline, replay is ordered, and per-connection MySQL commands are serial. No distinct heavy-under-lock, lock-order, deadlock, or shared-cache race was found; overlapping post-validation mutation has the same boundary as ordinary planning.
- Lifecycle and error handling: invalid-cache refresh reparses into a fresh StatementContext, clears stale publication before normal execution, fails closed on validation errors, and cannot republish the old context after failure. The remaining defects are unfenced subject and metadata lifecycles, not refresh rollback.
- Configuration, compatibility, and persistence: no configuration item, FE-BE field, EditLog payload, storage format, public symbol, or rolling-upgrade protocol changes are introduced. Process-local epochs need not persist because prepared consumers do not survive FE restart; replay updates the applicable local epochs.
- Parallel and conditional paths: built-in authorization uses O(1) epoch validation; LDAP/custom sources take full validation; forwarded, Arrow Flight, group-commit, and ineligible scan shapes were checked. Direct cloud lookup independently rechecks compute-group USAGE through CloudSystemInfoService.getCurrentClusterId, so that proposed candidate was dismissed with evidence.
- Tests and results: all changed Java tests and the Groovy/.out pair were reviewed statically; the expected rows are internally consistent. Missing negative coverage is called out inline for subject, view-definition, and database-namespace transitions, and the current denial oracle is too broad. Per task constraints, no build or test was run, so CI/author claims were not independently verified.
- Observability and performance: existing DEBUG refresh/rollback logging is adequate, built-in validation is O(1), and custom validation is bounded by the eligible single-table plan. No separate metric, CPU, or memory issue was substantiated.
- Transactions, data writes, and variable passing: no transactional write, storage-visibility, data-atomicity, or new FE-BE variable-passing checkpoint applies.
- User focus: no additional focus was provided.
All candidates have an evidence-backed submitted or dismissed disposition, but the convergence result remains incomplete under the three-round cap.
| try { | ||
| Env env = connectContext.getEnv(); | ||
| if (useVersionValidation) { | ||
| return usesAuthorizationChecks(connectContext) && versionsAreCurrent(env); |
Contributor
There was a problem hiding this comment.
[P1] Bind this shortcut to the planning authorization subject
The cached context stores no planning identity, so this branch can return true without examining the current subject. A connection can prepare and cache ResultSink -> Project(secret) -> Filter(pk=?) -> OlapScan(t) as user A, then successfully issue COM_CHANGE_USER for pre-existing user B: that handler changes currentUserIdentity but does not clear preparedStatementContextMap, and neither global epoch advances. ExecuteCommand then calls the analysis-free direct executor with A's plan, letting B bypass a missing SELECT grant or B's stricter row policy. Capture and compare the effective planning subject (including session-authenticated roles), or invalidate server prepared statements on change-user, before allowing this path; please add a binary-protocol A-to-B regression.
| } | ||
|
|
||
| /** Freeze the decisions used by a completed plan before storing them in a reusable context. */ | ||
| public synchronized SecurityDependencyContext snapshot() { |
Contributor
There was a problem hiding this comment.
[P1] Fence reuse on inlined view definitions
This snapshot records a view's privilege/policy answers but not the definition that was inlined into the cached physical plan. A simple view can be reduced to the eligible Project -> Filter -> OlapScan shape; ALTER VIEW then mutates and re-registers the same view name/id without advancing either security epoch or changing the underlying OlapTable fences. The version branch accepts unchanged epochs, and full validation only resolves the same view and repeats its unchanged grant/policies. Thus a user granted only on v can warm a query while v exposes a sensitive column, an administrator can alter v to redact it or add a tenant predicate, and the next EXECUTE still runs the old inlined project/filter. Capture and compare a view-definition/schema generation for every inlined view (in both validation modes), or invalidate these caches on view mutation; please add an ALTER VIEW regression.
| } | ||
| Auth auth = env.getAuth(); | ||
| PolicyMgr policyMgr = env.getPolicyMgr(); | ||
| return auth != null && policyMgr != null |
Contributor
There was a problem hiding this comment.
[P1] Revalidate the cached resource after namespace changes
This version-only branch never resolves the recorded FullTableName. A restricted user can warm a point-query cache with SELECT on old_db.t; an administrator can then rename that database to new_db, which mutates the retained table object's qualified database name without marking it dropped, changing its table name/schema/topology versions, or advancing either security epoch. Built-in grants are name-scoped, so fresh analysis would no longer authorize/resolve the old resource, yet this method and ShortCircuitQueryContext.isReusable still permit direct execution of the retained table. Bind the snapshot to the current catalog/database/table identity (or advance a metadata namespace generation) before using the version shortcut, and add a database-rename regression.
| boolean denied = false | ||
| try { | ||
| prepared.executeQuery().close() | ||
| } catch (SQLException e) { |
Contributor
There was a problem hiding this comment.
[P2] Verify the expected SELECT denial
This marks every SQLException as success, so a reparse, placeholder-transfer, planner, or runtime failure would also make this security regression pass. Please assert the stable Doris/MySQL access-denied code or message fragment here (and rethrow anything else) so the test proves cache rejection reached the authoritative privilege check.
Contributor
TPC-H: Total hot run time: 16836 ms |
Contributor
TPC-DS: Total hot run time: 81482 ms |
Contributor
ClickBench: Total hot run time: 14.58 s |
Contributor
FE Regression Coverage ReportIncrement line coverage |
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
3 participants
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.
Problem
A server-side prepared point query can keep a direct short-circuit execution context across executions. If the user's SELECT access or a row-filter/data-mask policy changes after the context is created, direct reuse must not execute with the earlier security decisions.
Root cause
The prepared-statement fast path checked table schema, table name, partition topology, and a relevant session limit before reusing
ShortCircuitQueryContext. It then called the direct executor without running the normal Nereids privilege and policy analysis passes. The retained context did not capture those security dependencies, so it had no way to detect that authorization had changed.Reproduction
Fix
Tests
./run-fe-ut.sh --run org.apache.doris.nereids.SecurityDependencyContextTest,org.apache.doris.qe.ShortCircuitQueryContextTest,org.apache.doris.nereids.trees.plans.commands.ExecuteCommandTest(22 tests passed)cd fe && mvn -pl fe-core checkstyle:check(0 violations)DISABLE_BUILD_UI=ON ./build.sh --feprepared_short_circuit_security_refreshregression suite on a deployed FE, including an EXPLAIN assertion forSHORT-CIRCUIT, row-policy add/drop, and SELECT revoke/grant using the same server-prepared statement