github-actions[bot] commented on code in PR #66629: URL: https://github.com/apache/doris/pull/66629#discussion_r3751814461
########## regression-test/suites/auth_call/test_dml_stream_load_be_auth_docker.groovy: ########## @@ -0,0 +1,158 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +import org.apache.doris.regression.suite.ClusterOptions + +suite("test_dml_stream_load_be_auth_docker", "docker,auth_call") { + def options = new ClusterOptions() + options.cloudMode = true + options.feNum = 1 + options.beNum = 1 + options.beConfigs.add("enable_all_http_auth=true") + + docker(options) { + String suiteName = "test_dml_stream_load_be_auth_docker" + String user = "${suiteName}_user" + String pwd = "C123_567p" + String dbName = "${suiteName}_db" + String tableName = "${suiteName}_tb" + String otherTableName = "${suiteName}_other_tb" + String dataFile = "${context.file.parent}/../../data/auth_call/stream_load_data.csv" + + def fe = cluster.getFeByIndex(1) + def be = cluster.getAllBackends(true).get(0) + String feHttpAddress = "${fe.host}:${fe.httpPort}" + String beHttpAddress = "${be.host}:${be.httpPort}" + + def parseCurlResult = { String out, String err -> + def lines = out.readLines() + assertTrue("curl output should include http code, output=${out}, err=${err}", !lines.isEmpty()) + def httpCode = lines.last() as int + def body = lines.size() == 1 ? "" : lines[0..-2].join("\n") + return [httpCode, body, err] + } + + def streamLoad = { String endpoint, String table, String label, boolean followRedirect, + List extraHeaders -> + def command = [ + "curl", "--noproxy", "*", "-sS", "-w", "\n%{http_code}", + "-u", "${user}:${pwd}", + "-H", "label:${label}", + "-H", "column_separator:,", Review 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. ```suggestion "-H", "column_separator:,", "-H", "Expect:100-continue", ``` ########## be/src/service/http/action/stream_load_2pc.cpp: ########## @@ -36,9 +36,38 @@ #include "service/http/http_request.h" #include "service/http/http_status.h" #include "service/http/utils.h" +#include "util/url_coding.h" namespace doris { +namespace { +constexpr const char* INTERNAL_CATALOG_NAME = "internal"; + +bool set_load_privilege(const HttpRequest& req, TCheckAuthRequest& auth_request) { + std::string db; + std::string table; + url_decode(req.param(HTTP_DB_KEY), &db); + url_decode(req.param(HTTP_TABLE_KEY), &table); + if (db.empty()) { + return false; + } + + TPrivilegeCtrl priv_ctrl; + priv_ctrl.__set_ctl(INTERNAL_CATALOG_NAME); + priv_ctrl.__set_db(db); + if (table.empty()) { + priv_ctrl.__set_priv_hier(TPrivilegeHier::DATABASE); + } else { + priv_ctrl.__set_priv_hier(TPrivilegeHier::TABLE); Review 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. ########## be/src/service/http/action/stream_load.cpp: ########## @@ -289,6 +318,10 @@ int StreamLoadAction::on_header(HttpRequest* req) { return 0; } Review 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. -- 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]
