github-actions[bot] commented on code in PR #66618:
URL: https://github.com/apache/doris/pull/66618#discussion_r3749566863
##########
fe/fe-core/src/main/java/org/apache/doris/service/FrontendServiceImpl.java:
##########
@@ -84,6 +84,7 @@
import org.apache.doris.common.util.DebugPointUtil;
import org.apache.doris.common.util.DebugPointUtil.DebugPoint;
import org.apache.doris.common.util.PropertyAnalyzer;
+import org.apache.doris.common.util.ThriftLogHelper;
Review Comment:
[P1] Redact the parallel credential-bearing RPC logs
This shared helper is applied only to the selected load RPCs, while live
parallel handlers in this class still stringify the same credential fields. For
example, `FeServiceClient.beginRemoteTxn`/`commitRemoteTxn`/`abortRemoteTxn`
put the external-catalog password into each request immediately before RPC, but
the handlers at lines 2286/2368/2424 log those requests directly. Their
invalid-token branches also include `request.getToken()` in the returned
status, which `RemoteOlapInsertExecutor` logs on failure. The legacy
begin/commit/rollback handlers have the same raw-request pattern. Please use
the log-only sanitizer for every credential-bearing request log and value-free
token errors, with coverage for these production paths.
##########
be/src/service/http/http_request.cpp:
##########
@@ -41,7 +41,7 @@ static std::string s_empty = "";
static bool is_sensitive_header(const std::string& header_name) {
return iequal(header_name, HttpHeaders::AUTHORIZATION) ||
iequal(header_name, HttpHeaders::PROXY_AUTHORIZATION) ||
iequal(header_name, "token") ||
- iequal(header_name, HttpHeaders::AUTH_TOKEN);
+ iequal(header_name, HttpHeaders::AUTH_TOKEN) || iequal(header_name,
"auth_code");
Review Comment:
[P1] Sanitize credential query parameters in debug_string
This predicate is only consulted for `_headers`. `init_from_evhttp()` keeps
the full query in `_uri` and copies it into `_params`, while `debug_string()`
prints both without redaction. `BatchDownloadAction` uses `?token=...` as a
real credential and logs `req->debug_string()` before `_check_token()`; with
all-HTTP auth enabled, the failed-auth warning formats it as well. The token
therefore still reaches logs despite this sensitive-name update. Please render
the URI/params through the same case-insensitive redaction policy and add a
test initialized from a real credential-bearing URI.
##########
fe/fe-core/src/main/java/org/apache/doris/common/util/ThriftLogHelper.java:
##########
@@ -0,0 +1,57 @@
+// 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.
+
+package org.apache.doris.common.util;
+
+import org.apache.thrift.TBase;
+import org.apache.thrift.TFieldIdEnum;
+import org.apache.thrift.meta_data.FieldMetaData;
+
+import java.util.Map;
+
+/** Creates log-only copies of Thrift requests with credential fields masked.
*/
+public final class ThriftLogHelper {
+ private static final String MASKED_CREDENTIAL = "***MASKED***";
+
+ private ThriftLogHelper() {
+ }
+
+ // auth_code is omitted because its numeric field cannot hold the string
mask.
+ public static <T extends TBase<T, F>, F extends TFieldIdEnum> T
requestForLog(T request) {
Review Comment:
[P1] Cover the direct FE bearer/token lifecycle logs
This helper addresses selected Thrift request formatting, but direct FE
token sinks still expose usable credentials. On the default-enabled Arrow
Flight service, `FlightTokenManagerImpl.createToken` stores a new bearer,
returns that same value in `Authorization: Bearer`, then logs it at INFO; it
remains accepted for the default 86400 seconds. Rejected bearer values are also
embedded in exceptions that `FlightBearerTokenAuthenticator` logs and returns.
Separately, FE bootstrap logs the helper node's raw cluster token immediately
before installing it. Please make creation/validation/eviction/bootstrap
messages value-free (or use a non-reversible fingerprint) and add INFO/ERROR
log-capture plus client-error tests.
##########
fe/fe-core/src/main/java/org/apache/doris/httpv2/rest/LoadAction.java:
##########
@@ -752,7 +752,8 @@ private boolean isSensitiveHeader(String headerName) {
|| "Cookie".equalsIgnoreCase(headerName)
|| "Set-Cookie".equalsIgnoreCase(headerName)
|| "token".equalsIgnoreCase(headerName)
- || "Auth-Token".equalsIgnoreCase(headerName);
+ || "Auth-Token".equalsIgnoreCase(headerName)
+ || "auth_code".equalsIgnoreCase(headerName);
Review Comment:
[P1] Share redaction across the FE HTTP credential paths
This predicate protects only `LoadAction`'s normal header dump. Other live
HTTP paths still expose usable credentials: `/copy/upload` and `/copy/query`
log every `Authorization`/`Cookie` header at INFO before authentication, and a
successful cloud group-commit forward places the Basic `user:password` in
`RedirectView` userinfo that the bounded-drain path logs at INFO.
Failure/session paths also bypass this helper: line 134 returns and logs the
rejected token, malformed Basic auth is logged verbatim, and `addSession` logs
the newly issued live session ID. Please centralize safe header/URL/error
rendering across these paths and add credential-bearing end-to-end log tests
(including `auth_code`).
##########
be/src/service/http/http_handler_with_auth.cpp:
##########
@@ -113,13 +113,15 @@ int HttpHandlerWithAuth::on_header(HttpRequest* req) {
auth_result.status.status_code = TStatusCode::type::OK;
auth_result.status.error_msgs.clear();
} else {
- HttpChannel::send_reply(req, HttpStatus::FORBIDDEN);
- return -1;
+ auth_result.status.status_code = TStatusCode::type::ANALYSIS_ERROR;
+ auth_result.status.error_msgs.clear();
}
#endif
Status status(Status::create(auth_result.status));
if (!status.ok()) {
- LOG(WARNING) << "permission verification failed, request: " <<
auth_request;
+ TCheckAuthRequest request_for_log(auth_request);
Review Comment:
[P1] Redact the parallel BE token-authentication sinks
This masks the Basic-auth request, but parallel token paths still log usable
credentials. `DownloadAction::check_token` writes both the submitted token and
`_exec_env->token()` and returns the submitted value; clone download helpers
embed `_cluster_info->token` in URLs logged at WARN/INFO; heartbeat handling
logs the installed cluster and current/last HTTP auth tokens at INFO. These
sinks are independent of `HttpRequest::debug_string()`, so fixing query
rendering alone will not close them. Please use value-free errors/constant
masks for raw tokens, `mask_token` only for token-bearing URLs, and add focused
log-capture coverage.
--
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]
