github-actions[bot] commented on code in PR #66629:
URL: https://github.com/apache/doris/pull/66629#discussion_r3755233290
##########
be/src/service/http/action/stream_load.cpp:
##########
@@ -94,9 +94,11 @@ static const std::string ASYNC_MODE = "async_mode";
TStreamLoadPutResult k_stream_load_put_result;
#endif
-StreamLoadAction::StreamLoadAction(ExecEnv* exec_env)
- : 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
Review 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.
##########
be/src/service/http/action/stream_load_2pc.cpp:
##########
@@ -39,10 +39,10 @@
namespace doris {
-StreamLoad2PCAction::StreamLoad2PCAction(ExecEnv* exec_env)
- : HttpHandlerWithAuth(exec_env, TPrivilegeHier::GLOBAL,
TPrivilegeType::LOAD) {
- // 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
Review 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.
##########
be/src/service/http/action/http_stream.cpp:
##########
@@ -92,10 +92,10 @@
DEFINE_COUNTER_METRIC_PROTOTYPE_2ARG(http_stream_requests_total, MetricUnit::REQ
DEFINE_COUNTER_METRIC_PROTOTYPE_2ARG(http_stream_duration_ms,
MetricUnit::MILLISECONDS);
DEFINE_GAUGE_METRIC_PROTOTYPE_2ARG(http_stream_current_processing,
MetricUnit::REQUESTS);
-HttpStreamAction::HttpStreamAction(ExecEnv* exec_env)
- : HttpHandlerWithAuth(exec_env, TPrivilegeHier::GLOBAL,
TPrivilegeType::LOAD) {
- // 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
Review 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.
##########
be/src/service/http/action/stream_load.cpp:
##########
@@ -94,9 +94,11 @@ static const std::string ASYNC_MODE = "async_mode";
TStreamLoadPutResult k_stream_load_put_result;
#endif
-StreamLoadAction::StreamLoadAction(ExecEnv* exec_env)
- : 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
+ // 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
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
