[fix](auth) Mask credentials in authentication and Stream Load logs - #66618
[fix](auth) Mask credentials in authentication and Stream Load logs#66618bobhan1 wants to merge 2 commits into
Conversation
### What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary: A failed HTTP authentication request caused the BE warning log to print the complete TCheckAuthRequest, including its plaintext password. The FE checkAuth debug log exposed the same field. Copy the request before logging and replace only the password with the standard ***MASKED*** marker, preserving the user, client IP, privilege, and other non-secret diagnostic fields. The existing HTTP request formatter continues to mask authentication tokens and Authorization headers.
### Release note
Mask passwords in BE and FE HTTP authentication logs while retaining non-sensitive request context.
### Check List (For Author)
- Test
- [ ] Regression test
- [x] Unit Test
- [ ] Manual test (add detailed scripts or steps below)
- [ ] No need to test or manual test. Explain why:
- [ ] This is a refactor/code format and no logic has been changed.
- [ ] Previous test can cover this change.
- [ ] No code files have been changed.
- [ ] Other reason
- Behavior changed:
- [ ] No.
- [x] Yes. Password values are replaced with ***MASKED*** in authentication logs; other request fields are unchanged.
- Does this need documentation?
- [x] No.
- [ ] Yes.
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
bobhan1
changed the title
|
/review |
|
run buildall |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes: the local masking helpers are sound for their current call sites, but the patch does not achieve its end-to-end credential-redaction goal. Five inline findings identify live paths that still emit usable passwords, bearer/session/cluster tokens, or rejected credential values.
Review status: capped/incomplete. A distinct Arrow Flight/bootstrap token finding appeared in the third and final allowed review round. All currently known candidates have been independently verified, deduplicated, and adjudicated, but the workflow did not reach a clean no-new-findings round.
Critical checkpoints:
- Goal and proof: Not met. The focused tests prove selected Thrift/header serializers, but production query parameters, parallel RPCs, redirects/COPY/session handling, direct BE token paths, and Arrow Flight token lifecycle logs remain credential-bearing.
- Scope/focus: The individual edits are small, but the redaction policy is split among local helpers and does not cover functionally parallel paths; a shared safe-rendering boundary is needed.
- Concurrency: No production thread/lock or shared-state behavior is changed. The copied request objects are local to each logging call, and no race or lock-order issue was found.
- Lifecycle/static initialization: No new production lifecycle or cross-TU static-initialization hazard was found. The new BE mask constant is compile-time initialized and the Java utility is stateless.
- Configuration: No configuration item is added or changed. Existing cloud-forwarding and default-enabled Arrow Flight conditions expose missed paths, as described inline.
- Compatibility: No wire, storage, symbol, or rolling-upgrade compatibility change is introduced.
- Parallel paths: Blocking gaps remain across FE transaction RPCs, FE HTTP/COPY/session paths, BE query/direct-token paths, and FE Arrow Flight/bootstrap token handling.
- Special conditions: The current helpers correctly handle password, token, deprecated auth code/UUID, and certificate branches at their actual call sites, but equivalent conditional/error paths outside those call sites remain unredacted.
- Test coverage: The new unit tests are deterministic and useful for the touched serializers, but they omit a real credential-bearing URI and end-to-end coverage for every blocking parallel path.
- Test results: No result files are changed. Per the automated-review constraint I did not run local builds/tests; the author reports 7 BE and 24 FE unit tests passing. Current live style/license/secret/dependency checks pass, while BE UT (macOS) is still in progress.
- Observability: Preserving non-secret request fields is appropriate, but remaining INFO/WARN/DEBUG/ERROR sinks defeat the intended safe observability. No new metric is required for this hardening.
- Transactions/persistence: The patch does not change transaction outcomes, EditLog behavior, failover semantics, or persisted data.
- Data writes/crash safety: No data-write or atomicity behavior is modified.
- FE/BE variables: No new cross-process variable or protocol field is introduced.
- Performance: The Thrift copies are behind DEBUG/VLOG guards and no unacceptable default-path CPU/memory cost was found.
- Other issues/security disposition: No additional distinct defect survived the final sweep. BE 8040/heartbeat/bootstrap instances are internal operational-hardening issues under the threat model; FE Arrow Flight 8070 is explicitly client-facing.
User focus: no additional review focus was supplied.
| 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"); |
There was a problem hiding this 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.
| 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; |
There was a problem hiding this 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.
| || "token".equalsIgnoreCase(headerName) | ||
| || "Auth-Token".equalsIgnoreCase(headerName); | ||
| || "Auth-Token".equalsIgnoreCase(headerName) | ||
| || "auth_code".equalsIgnoreCase(headerName); |
There was a problem hiding this 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).
| Status status(Status::create(auth_result.status)); | ||
| if (!status.ok()) { | ||
| LOG(WARNING) << "permission verification failed, request: " << auth_request; | ||
| TCheckAuthRequest request_for_log(auth_request); |
There was a problem hiding this 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.
| } | ||
|
|
||
| // 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) { |
There was a problem hiding this 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.
TPC-H: Total hot run time: 29127 ms |
TPC-DS: Total hot run time: 158346 ms |
ClickBench: Total hot run time: 23.86 s |
FE UT Coverage ReportIncrement line coverage |
FE Regression Coverage ReportIncrement line coverage |
2 participants
What problem does this PR solve?
Issue Number: CIR-21336
Problem Summary: Authentication credentials could be exposed by HTTP authentication and Stream Load diagnostic logs. In addition to the original HTTP authentication request logging, several FE and BE Stream Load paths formatted complete Thrift requests containing
passwd,token,auth_code, orauth_code_uuid. The deprecatedauth_codeHTTP header was also missing from the sensitive-header filter, and two Stream Load authentication errors included the invalid token value.This PR logs sanitized copies of authentication-related requests. Credential values are masked or omitted while non-sensitive request fields remain available for diagnostics.
Release note
Mask authentication credentials in HTTP authentication and Stream Load logs while retaining non-sensitive request context.
Check List (For Author)
Test
./run-be-ut.sh --run --filter=HttpAuthTest.* -j100(7 tests passed)./run-fe-ut.sh --run org.apache.doris.common.util.ThriftLogHelperTest,org.apache.doris.service.FrontendServiceImplTest,org.apache.doris.load.StreamLoadHandlerTest(24 tests passed)clang-format --dry-run --Werror be/src/load/stream_load/stream_load_executor.cpp be/src/service/http/http_request.cppgit diff --checkBehavior changed:
Does this need documentation?
Implementation notes
passwd,token, andauth_code_uuid) are replaced with***MASKED***; the numeric deprecatedauth_codeis omitted from log-only copies.auth_codeheader as sensitive.