[fix](arrow-flight) Stop writing bearer tokens to fe.log - #66572
Open
CalvinKirs wants to merge 1 commit into
Open
[fix](arrow-flight) Stop writing bearer tokens to fe.log#66572CalvinKirs wants to merge 1 commit into
CalvinKirs wants to merge 1 commit into
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Member
Author
|
run buildall |
Member
Author
|
/review |
Contributor
|
Codex automated review failed and did not complete. Error: All Codex review accounts are usage-limited; earliest retry is 2026-08-08T03:32:00Z. Please trigger /review again after that time. |
Contributor
TPC-H: Total hot run time: 28848 ms |
Arrow Flight SQL bearer tokens were logged verbatim at INFO when they were minted, evicted and invalidated, and were embedded in the IllegalArgumentExceptions that FlightBearerTokenAuthenticator logs at ERROR. A bearer token is a full credential until it expires (arrow_flight_token_alive_time_second, one day by default), so anyone able to read fe.log or the log pipeline - operators without any database privilege, log aggregation users, backups - could replay one against the Arrow Flight SQL port and run queries as that user. Add TokenMasker with the two renderings a secret may have in a message: - tokenId(): a truncated SHA-256, e.g. "sha256:1a2b3c4d". Stable, so log lines and the error message handed back to the client still refer to the same token, but no part of the secret is disclosed. Used for flight tokens; the "search for this token in fe.log" hint keeps working, on the id instead. - maskPrefix(): reveals a short leading prefix, for the case where a human has to recognize which configured secret was involved. This is the helper that already lived privately in MetaService, moved here and reused. Note that a Flight SQL ConnectContext's peerIdentity IS the bearer token, so the unregisterConnection() teardown log in FlightSqlConnectPoolMgr is masked too. Two more credentials that had the same problem elsewhere: the cluster token adopted from a helper node in Env.getClusterIdFromStorage(), and initial_root_password in Auth, which was echoed by the branch that runs when the configured value is not a 2-staged SHA-1 hash - that is, when it is most likely a plaintext password. Finally, a checkstyle rule rejects a value whose name says it holds a token/password/secret/peer identity being passed straight to a log call. It matches across lines, since the argument often sits on a continuation line. It only knows the naming convention, so it is a backstop rather than a substitute for review - peerIdentity had to be taught to it by hand. The rule reports no violation on the current tree.
Contributor
TPC-DS: Total hot run time: 158367 ms |
Contributor
ClickBench: Total hot run time: 23.65 s |
Member
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29319 ms |
Contributor
TPC-DS: Total hot run time: 157830 ms |
Contributor
ClickBench: Total hot run time: 23.91 s |
Contributor
FE Regression Coverage ReportIncrement line coverage |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Arrow Flight SQL bearer tokens are written to
fe.login cleartext.FlightTokenManagerImpllogs the token verbatim at INFO when it is minted, evicted from either cache, and invalidated, and it also puts the token into theIllegalArgumentExceptionmessages thatFlightBearerTokenAuthenticator.validateBearerlogs at ERROR:A bearer token is a complete credential until it expires —
arrow_flight_token_alive_time_seconddefaults to 86400s. So anyone who can readfe.log, or the log aggregation platform it is shipped to, or a backup of either, can take a live token, send it asAuthorization: Bearer <token>to the Arrow Flight SQL port (arrow_flight_sql_port, default 8070), and run queries as that user without ever knowing their password. Logs routinely reach a much wider audience than the credential store does, which is what makes this worth fixing even though the log file itself is not world readable.What this PR does
Adds
org.apache.doris.common.util.TokenMasker, which offers the two renderings a secret can reasonably have in a message:tokenId(t)→sha256:1a2b3c4d, a truncated SHA-256. It is stable, so a log line and the error message returned to the client still point at the same token and can be matched up, but no part of the secret survives in it. This is what the flight token paths now use. The existing "search for this token in fe.log to see the evict reason" hint therefore still works — it now says token id, and the id appears both in the client's error and in the log.maskPrefix(t)→abc***, revealing only a short leading prefix, for the case where a human has to recognize which configured secret was involved (token rotation). This is the helper that already existed privately inMetaService; it is moved into the utility and reused rather than duplicated.Every token-valued site in the Arrow Flight path is converted: the four
LOG.infocalls inFlightTokenManagerImpl, the fourIllegalArgumentExceptionmessages invalidateToken/getTokenDetails, the one inFlightSessionsWithTokenManager.createConnectContext, and the teardown warning inFlightSqlConnectPoolMgr.unregisterConnection. That last one is worth spelling out: a Flight SQLConnectContext'speerIdentityis the bearer token itself —FlightBearerTokenAuthenticator.createAuthResultWithBearerTokenreturns the token as the peer identity, andFlightSqlConnectPoolMgrkeys itsflightToken2ConnectionIdmap by it — soctx.getPeerIdentity()in a log line leaks a live token under a name that does not look like one.Two more credentials with the same problem, found while auditing for other instances:
Envlogs the cluster token adopted from a helper node at INFO (get token from helper node. token={}). That token authenticates metadata access between FE nodes, so it getsmaskPrefix, consistent with howMetaServicealready renders the same token.Authechoesinitial_root_passwordinto a WARN — and it does so from the branch that runs when the configured value failed 2-staged SHA-1 validation, which is exactly the case where an operator put a plaintext password in the config. The value is simply dropped from the message; it adds nothing to the diagnosis that the config key name does not already give.Finally, a checkstyle rule rejects a value whose name says it holds a token/password/secret/peer identity being passed straight into a
LOG.x(...)call, as a parameter or concatenated into the message. It matches across lines, because the credential argument frequently sits on a continuation line — that is true of theFlightSqlConnectPoolMgrcase above, which a line-based rule silently misses.It is a backstop, not a substitute for review, and the honest limitation is that it only knows the naming convention:
peerIdentityhad to be taught to it by hand once it turned out to be a token, and any other alias would be equally invisible. It reports no violation anywhere infe/after this PR, so it lands without a single suppression.Release note
Arrow Flight SQL bearer tokens are no longer written to
fe.log. Log lines and error messages now carry a non-reversible token id (sha256:prefix) instead of the token itself.Check List (For Author)
TokenMaskerTestcovers that the token id is a digest and cannot contain any part of the token, that it is stable for the same token and differs across tokens, the empty/null handling, andmaskPrefixincluding its too-short-to-reveal branch.The checkstyle rule was verified to actually fire, not just to be quiet: re-adding the original
LOG.info(..., username, token)line, and separately un-masking the multi-lineFlightSqlConnectPoolMgrcall, each fail the build at that line with the new message; restoring them goes back to green.The text of some Arrow Flight error messages changes: where they used to echo the bearer token, they now carry
token id: sha256:.... Anything that parsed the token out of an error message or out offe.logwould need to use the id instead. No API, wire format or configuration changes.