[fix](load) Avoid global BE auth precheck for stream load - #66629
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
|
run buildall |
TPC-H: Total hot run time: 28367 ms |
TPC-DS: Total hot run time: 158317 ms |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes.
The common direct-BE stream-load path is now table-scoped, but three blocking gaps remain: 2PC authorization is not bound to the selected transaction, cloud group-commit forwarding retains the global gate, and the added FE-redirect regression never reaches a redirect.
Critical checkpoints:
- Goal and proof: The direct BE path and ordinary FE-redirect design use table-level
LOAD, and the table-specific 2PC happy path is wired, but the transaction and forwarding gaps leave the goal incomplete; the redirect test does not currently prove its path. - Scope and reuse: The request-local helper is focused, but
/_stream_load_forwardis a functional parallel path and should share the same target-scoped policy instead of retaining a separate global check. - Concurrency and lifecycle: The overrides add only request-owned data and local Thrift objects. No new shared mutable state, locks, threads, ownership transfer, teardown path, or nontrivial static-initialization dependency was introduced.
- Configuration and compatibility: No protocol, persisted format, or new configuration changes. The documented database-level fallback for the legacy 2PC route remains intentional; the defects appear on existing all-HTTP-auth, cloud 2PC, and group-forward configurations.
- Transactions, data correctness, and RBAC: FE-backed live-table 2PC checks actual targets, but the cloud txn-ID abort path bypasses that binding and can operate an unrelated transaction. Authorization must be tied to the transaction's recorded target set.
- Tests: The new suite covers common allow/deny and matching 2PC commit intent, but the FE request lacks the mandatory
Expect: 100-continue; mismatched transaction-target abort and multi-BE group-forward cases are also needed. No builds or test suites were run locally because the review bundle prohibits them. At the final refresh, compile, formatter, Checkstyle, license, and secret checks passed; BE and regression suites were still pending. - Performance, observability, and persistence: The bounded decoding/object construction adds no material hot-path or synchronization concern. There are no EditLog, storage-format, or observability changes.
- User focus: No additional review focus was supplied.
- Convergence: After the three findings were validated and deduplicated, every normal and risk-focused reviewer in round 2 returned
NO_NEW_VALUABLE_FINDINGS.
| if (table.empty()) { | ||
| priv_ctrl.__set_priv_hier(TPrivilegeHier::DATABASE); | ||
| } else { | ||
| priv_ctrl.__set_priv_hier(TPrivilegeHier::TABLE); |
There was a problem hiding this comment.
[P1] Bind 2PC authorization to the selected transaction
This authorizes only the caller-controlled table in the URL, but handle() discards that table. In cloud mode, every txn-ID abort goes through CloudStreamLoadExecutor::operate_txn_2pc() to CloudMetaMgr::abort_txn(), which sends only the instance credential and txn ID; Meta Service then resolves and aborts the real transaction without a user/table check. A user with LOAD on db.allowed can therefore call /api/db/allowed/_stream_load_2pc with another live precommitted transaction ID—even from another database in the same instance—and abort it. Bind the check to the transaction's recorded tables (or force this operation through the FE transaction-aware path), and add a mismatched URL-table/transaction abort test.
| } | ||
| return 0; | ||
| } | ||
|
|
There was a problem hiding this comment.
[P1] Apply table-scoped auth to the forwarding route
Cloud group commit redirects through /_stream_load_forward when the selected endpoint differs from the table's target BE. StreamLoadForwardHandler still constructs the base handler with GLOBAL/LOAD and performs that check before forwarding, so with all-HTTP-auth and group forwarding enabled a table-only user is rejected at the proxy and never reaches this updated handler. Share the same target-bound policy with the forward handler while keeping it aligned with the forwarded path, and cover the multi-BE forwarding branch.
| "curl", "--noproxy", "*", "-sS", "-w", "\n%{http_code}", | ||
| "-u", "${user}:${pwd}", | ||
| "-H", "label:${label}", | ||
| "-H", "column_separator:,", |
There was a problem hiding this comment.
[P1] Send the required Expect header before testing FE redirect
LoadAction.executeWithoutPassword() returns There is no 100-continue header before selecting a backend. This fixture is only 18 bytes, and curl 8.5.0 sends Content-Length: 18 but no Expect for this -T upload, so the redirected call gets an HTTP-200 failed REST body and redirectedJson.Status is absent. Add the header so the assertion actually reaches BE authorization.
| "-H", "column_separator:,", | |
| "-H", "column_separator:,", | |
| "-H", "Expect:100-continue", |
ClickBench: Total hot run time: 23.86 s |
|
fix #60761 |
bobhan1
left a comment
bobhan1
left a comment
There was a problem hiding this comment.
The common direct-BE Stream Load path and the ordinary FE 307 redirect path now look correctly scoped to TABLE/LOAD. However, I think two compatibility/correctness gaps remain:
-
The legacy database-only 2PC endpoint still rejects users that only have table-level
LOAD_PRIV. When the request is/api/{db}/_stream_load_2pc,set_load_privilege()maps it toDATABASE/LOAD.checkDbPriv(..., LOAD)does not consider a table-level grant, even thoughFrontendServiceImpl.loadTxn2PCImpl()subsequently resolves the transaction's actual table list and checksLOADon every table. As a result, a table-only user that could use this endpoint before the generic HTTP pre-auth change still receives 403. This route is still used by the regression framework (regression-test/framework/src/main/groovy/org/apache/doris/regression/action/StreamLoadAction.groovy:303), so it is not only a dead compatibility route. For the database-only form, please consider doing authentication-only at the BE and relying on the existing transaction-bound FE checks, or otherwise authorize against the transaction's actual tables. A regression case using the database-only 2PC URL with only table-levelLOAD_PRIVwould cover this. -
Cloud group-commit forwarding still goes through a
GLOBAL/LOADgate.StreamLoadForwardHandleris still constructed withHttpHandlerWithAuth(exec_env, TPrivilegeHier::GLOBAL, TPrivilegeType::LOAD), and itson_header()calls the parent check before forwarding. Withenable_all_http_auth=trueand group-commit forwarding enabled, a table-only user can therefore receive 403 on the first BE and never reach the table-scopedStreamLoadActionfixed by this PR. Please apply the same db/table-basedTABLE/LOADpolicy to/_stream_load_forwardand add a forwarding-path regression case.
These are residual paths rather than regressions introduced by the new table-scoped implementation, but they leave valid Stream Load flows inconsistent under all-HTTP-auth.
0139c27 to
bd12e5a
Compare
gavinchou
changed the title
|
run buildall |
|
/review |
There was a problem hiding this comment.
Request changes.
The ordinary direct-BE, non-group Stream Load path now delegates to FE table-scoped authorization, but removing the common hook from three handlers is not safe across their parallel execution paths. Four new blocking issues remain: public auth_code becomes a trusted credential marker; explicit/config-forced group commit skips the only target LOAD check; HTTP Stream authorizes before it has both a verified identity and resolved target; and cloud label abort discards FE failure while reporting success.
The existing threads already own the cloud txn-ID transaction-binding issue (including its optional direct-commit sibling), the group-forward handler's GLOBAL gate, and the missing Expect: 100-continue test precondition. I did not duplicate those comments.
Critical checkpoint conclusions:
- Goal and proof: The stated goal is only achieved for ordinary non-group Stream Load with normal Basic credentials (and table-property group commit, which authenticates before FE selects the mode). The four inline findings show that the common downstream-auth premise does not hold for all three changed handlers. The new test does not prove those variants.
- Scope and reuse: The inheritance edits are mechanically small and focused, but a blanket removal is not the smallest correct policy because the handlers do not share one equivalent downstream authorization boundary. A verified identity plus resolved-target check should be reused at the actual common planning/transaction boundaries.
- Concurrency and thread safety: No new thread, shared mutable state, lock, atomic, or lock-order change is introduced. Request contexts and metrics retain their existing threading model; no new deadlock or heavy-under-lock issue was found.
- Lifecycle and static initialization: The added raw
ExecEnv*members are non-owning replacements for the inherited pointer and do not change destruction order or ownership. No circular ownership, cross-TU static initialization, or teardown issue was introduced. HTTP Stream's existing FE-created transaction lifecycle does expose the pending-transaction consequence described inline when token validation is delayed until commit. - Configuration: No new configuration is added. Existing
enable_all_http_auth,wait_internal_group_commit_finish,enable_stream_load_commit_txn_on_be, group-forwarding, cloud/shared-nothing, and TSO fences materially select different authorization paths; the implementation and tests do not cover them consistently. - Compatibility: No Thrift/storage format, persisted state, function ABI, or rolling-upgrade protocol change is added. Behavior compatibility is nevertheless broken for scoped HTTP Stream users, and enabling all-HTTP-auth no longer gates the legacy/token forms on these handlers.
- Parallel paths and conditions: Normal versus explicit/forced/table-property group commit, HTTP Stream, FE redirect, BE forwarding, and 2PC label versus txn-ID commit/abort were traced. Table-property group commit and FE-routed transaction-aware 2PC are the safe counterexamples; the accepted and existing comments cover the unsafe branches. The condition comments in the patch overstate FE coverage for those branches.
- Test coverage: The cloud one-BE suites cover only ordinary Basic direct-BE allow/deny before their FE upload. They omit explicit/forced and table-property group commit, HTTP Stream,
auth_code/invalid token, label abort, mismatched transaction, multi-BE forwarding, shared-nothing, TSO, and direct cloud commit. One BE cannot exercise forwarding. - Test results: The 18-byte FE upload lacks the mandatory
Expect: 100-continue, so the already-reported assertion fails before redirect and later statements in each suite are unreachable. No expected-result file is changed. I did not build or run tests because the authoritative review prompt forbids it; the PR description also says the docker suite was not executed. - Error handling, memory safety, and data correctness: No new buffer ownership, allocation, nullable-column, or memory-accounting issue was found. Error propagation is incorrect in cloud label abort because
TLoadTxnRollbackResult.statusis ignored. Authorization gaps can admit writes, and false success can leave a PRECOMMITTED transaction unchanged. - Transactions, persistence, and data writes: No EditLog schema or persistence path changes. Ordinary transaction mechanics and failover handling are unchanged, but authorization must precede planning/mutation and be bound to the transaction's recorded tables. The label-abort result must be propagated so the response matches transaction state.
- FE/BE variable propagation: No new cross-process field is introduced. Existing
auth_code, token, db/table, SQL, label, and txn-ID fields are populated inconsistently across the changed routes; the inline findings identify the missing trusted-identity/target bindings. - Observability: Existing request, load, and transaction logs/metrics are sufficient to diagnose these paths; no separate observability blocker was found. Returning
Successfor rejected label abort is itself misleading and must be fixed rather than masked with logging. - Performance: Removing one redundant auth RPC is beneficial only where an equivalent target-bound check exists. No separate CPU, memory, I/O, or hot-lock regression was found; restoring authorization at the common boundaries should remain bounded per request.
- Other/security boundary: Under Doris's threat model, direct BE HTTP on port 8040 is a Zone-2/internal surface, so the direct-only variants are reported as supported-workflow authorization/correctness regressions, not as claims of internet-exposed vulnerabilities. FE-routed scoped-user rejection and cloud label-abort false success also remain relevant after normal FE authentication.
- User focus: No additional user-provided review focus was supplied; the full PR was reviewed.
- Completion: The review converged in two rounds. Both normal full-coverage reviewers and the separate risk-focused reviewer returned
NO_NEW_VALUABLE_FINDINGSafter the HTTP Stream token scope was narrowed to the verified explicit/config-forced group-commit trigger. All candidates are accepted, merged, or duplicate-fenced.
| : HttpHandlerWithAuth(exec_env, TPrivilegeHier::GLOBAL, TPrivilegeType::LOAD) { | ||
| // Use LOAD privilege type: requires LOAD permission | ||
| StreamLoadAction::StreamLoadAction(ExecEnv* exec_env) : _exec_env(exec_env) { | ||
| // Stream load forwards the parsed HTTP credentials to FE load RPCs, where LOAD |
There was a problem hiding this comment.
[P1] Do not trust the public auth_code header
With enable_all_http_auth=true, parse_basic_auth(AuthInfo*) accepts any numeric auth_code without Basic credentials and set_request_auth() forwards it. FE loadTxnBeginImpl() and loadTxnRollbackImpl() treat isSetAuthCode() as a deprecated trusted path and skip password, token, and LOAD checks, so auth_code: 0 can perform an ordinary direct-BE load and a cloud label abort. The removed hook previously sent this form through checkAuth with empty credentials and rejected it. Remove or gate the public legacy header, or establish a verified identity and resolved target before setting the trusted field; add negative normal-load and label-abort tests.
| StreamLoadAction::StreamLoadAction(ExecEnv* exec_env) : _exec_env(exec_env) { | ||
| // Stream load forwards the parsed HTTP credentials to FE load RPCs, where LOAD | ||
| // privilege is checked against the actual db/table/txn. A generic BE HTTP | ||
| // pre-check cannot model every stream-load variant and would duplicate that |
There was a problem hiding this comment.
[P1] Authenticate explicit group commit before planning
_handle_group_commit() sets ctx->group_commit for explicit sync/async mode and for wait_internal_group_commit_finish before _on_header(), so the loadTxnBegin() RPC is skipped. That RPC is the only ordinary password and table-LOAD check: the remaining FE planning path calls analyzeWithoutCheckPriv(). A direct-BE request can therefore group-commit with a wrong password in shared-nothing, or as a cloud user with compute-group access but no LOAD on the target. Table-property group commit is safe because it authenticates before FE selects the mode. Put target-bound authentication on the common group-planning boundary and add explicit/forced cloud and shared-nothing negative tests.
| // Use LOAD privilege type: requires LOAD permission | ||
| // Note: _exec_env is set by parent class HttpHandlerWithAuth | ||
| HttpStreamAction::HttpStreamAction(ExecEnv* exec_env) : _exec_env(exec_env) { | ||
| // HTTP stream derives db/table from the SQL header and then forwards the |
There was a problem hiding this comment.
[P1] Establish the HTTP Stream identity before target planning
/api/_http_stream has no db/table path fields, and this handler forwards load_sql without populating request db/tbl. FE therefore rejects valid table- or database-scoped Basic users against empty strings before parsing the SQL target. Conversely, merely setting Auth-Token skips that precheck and installs the Basic username as ConnectContext identity without validating the token or password. Ordinary mode catches a junk token only at final commit, after staging a transaction with no immediate rollback; explicit/config-forced group commit skips that boundary and can complete as the claimed user. Authenticate first, resolve the SQL target, then check LOAD on it, with scoped-user and invalid-token group tests.
| // Use LOAD privilege type: requires LOAD permission | ||
| // Note: _exec_env is set by parent class HttpHandlerWithAuth | ||
| StreamLoad2PCAction::StreamLoad2PCAction(ExecEnv* exec_env) : _exec_env(exec_env) { | ||
| // 2PC commit/abort resolves the transaction's table list in FE and checks LOAD |
There was a problem hiding this comment.
[P1] Propagate the label-abort failure instead of returning Success
In cloud mode, a label-based abort reaches generic rollback_txn() with tbl="" because this handler records the database and label but not the route table. FE rejects a table-scoped user on that empty table before resolving the label, but the BE ignores TLoadTxnRollbackResult.status; CloudStreamLoadExecutor then returns OK and this handler reports Success while the transaction remains PRECOMMITTED. Route label abort through transaction-aware 2PC authorization, or return and propagate the rollback status after resolving the transaction tables, and assert both response and transaction state in a label-abort test.
TPC-H: Total hot run time: 29364 ms |
TPC-DS: Total hot run time: 158300 ms |
ClickBench: Total hot run time: 23.86 s |
github-actions
Bot
added
the
approved
|
PR approved by at least one committer and no changes requested. |
|
skip check_coverage |
## Summary - Remove the generic BE HTTP auth precheck from Stream Load, Stream Load 2PC, and HTTP Stream. - Keep the existing load-specific path: BE parses Basic auth and FE load RPCs check LOAD against the actual db/table/txn. - Add docker regression coverage for table-level and database-level LOAD users with BE enable_all_http_auth both off and on. ## Testing - sh format_code.sh on changed BE files - git diff --check - ninja -C be/ut_build_ASAN stream_load.cpp.o stream_load_2pc.cpp.o http_stream.cpp.o - run-regression-test selected both new docker suites; local regression config skips docker execution, but framework compilation and suite discovery passed Co-authored-by: gavinchou <gavinchou@apache.org>
4 participants
Summary
Testing