[Fix](auth) Restrict processlist visibility by user identity - #66746
[Fix](auth) Restrict processlist visibility by user identity#66746linrrzqqq wants to merge 1 commit into
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
There was a problem hiding this comment.
Request changes — capped/incomplete after the third and final review round.
The patch correctly filters homogeneous current-version schema/RPC results for native accounts with persisted roles, but it does not yet deliver the stated end-to-end visibility rule. The six inline threads cover eight independently verified issue families: rolling-version fail-open behavior; same-name/different-host SHOW and KILL authorization; loss of authentication-integration ADMIN roles; restricted Basic users reaching root sessions; inconsistent saved-profile ownership across v1, /api, v2, and SQL SHOW surfaces; and default-disabled ADMIN enforcement on live-query REST listing/cancellation. These are in-scope cross-user RBAC or authorization-correctness failures.
Critical checkpoint conclusions:
- Goal and proof: only partially achieved. The new unit/regression tests prove distinct-username behavior in a homogeneous native-role setup, but miss the accepted compatibility, exact-account, effective-role, HTTP, profile, and KILL cases. The identity-less tests explicitly preserve the fail-open behavior.
- Focus and size: the changed code is locally small and clear, but the fix is not end-to-end because functionally parallel authorization paths remain unchanged. There was no additional user-supplied focus beyond full-PR review.
- Concurrency: active-query maps and connection pools are concurrently accessed using their existing snapshot/concurrent-container contracts. The patch adds no unsafe mutation, new lock, lock-order change, heavy locked operation, or deadlock risk.
- Lifecycle/static state: scanner identity is retained by the runtime object pool through synchronous RPC use; query contexts remain valid while registered; the regression victim future is cancelled and joined. No new ownership cycle, leak, or cross-TU/static-initialization issue was found.
- Configuration: no new knob is added, but the existing supported default
enable_all_http_auth=falsemakescheckAdminAuth()a no-op and is load-bearing for one accepted REST issue. - Compatibility: unsafe in both relevant rolling directions. An old BE drops the caller identity before invoking a new FE, while old-FE active-query replay returns unfiltered rows that a new FE merges without re-filtering.
- Parallel paths and conditions: current exact
UserIdentityequality is correct, but missing identity fails open, local SHOW/KILL collapse identity to username, profile code uses literal-name/username ownership, and several HTTP paths lack a real ADMIN/owner decision. - Tests/results: coverage is narrow and lacks mixed versions, same-name host identities, integration-mapped ADMIN, restricted Basic credentials, legacy/v2 profile paths, SQL profile commands, and default-config REST cancellation. Per the review-runner instruction, no build or test command was executed; expected test code was inspected only.
- Observability/error handling: existing RPC status propagation and diagnostics are unchanged and adequate for this patch shape; no new metric or log is required. Authorization failures should fail or filter explicitly rather than silently grant legacy visibility.
- Transactions, persistence, and data writes: not applicable; no EditLog, transaction, storage-format, data-conversion, or committed-data path changes are present.
- FE/BE variables: caller identity is propagated on current BE scanner paths, but compatibility fallbacks and session-only effective roles are not propagated across all BE/FE and FE/FE hops.
- Performance: the added privilege check and row filtering do not introduce a material CPU, memory, or asymptotic regression beyond existing query/connection/profile scans.
Because new valuable findings remained in Round 3, this review is capped/incomplete rather than converged. Every candidate found within the allowed rounds was independently validated, consolidated/deduplicated, accepted, or dismissed with concrete evidence before submission.
dde3c30 to
f44ae79
Compare
f44ae79 to
6e7b3d1
Compare
|
run buildall |
TPC-H: Total hot run time: 17558 ms |
TPC-DS: Total hot run time: 85534 ms |
ClickBench: Total hot run time: 14.64 s |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
FE UT Coverage ReportIncrement line coverage |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
FE Regression Coverage ReportIncrement line coverage |
1 similar comment
FE Regression Coverage ReportIncrement line coverage |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
6e7b3d1 to
2f662cd
Compare
|
run buildall |
TPC-H: Total hot run time: 17795 ms |
TPC-DS: Total hot run time: 86260 ms |
ClickBench: Total hot run time: 14.81 s |
FE Regression Coverage ReportIncrement line coverage |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Problem Summary:
Authenticated non-admin users could inspect sessions and active SQL statements belonging to other users through
information_schema.processlist,SHOW PROCESSLIST, andinformation_schema.active_queries.The schema scanners did not consistently propagate the caller's user identity to FE, and the FE metadata handlers did not always enforce per-user visibility.
Release note
Non-admin users can now see only their own sessions and active queries through processlist and information_schema.active_queries. ADMIN users retain cluster-wide visibility.