branch-4.1: [fix](arrow-flight) Stop writing bearer tokens to fe.log - #67146
Open
CalvinKirs wants to merge 1 commit into
Open
branch-4.1: [fix](arrow-flight) Stop writing bearer tokens to fe.log#67146CalvinKirs wants to merge 1 commit into
CalvinKirs wants to merge 1 commit into
Conversation
### What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Arrow Flight SQL bearer tokens are written to `fe.log` in cleartext.
`FlightTokenManagerImpl` logs the token verbatim at INFO when it is
minted, evicted from either cache, and invalidated, and it also puts the
token into the `IllegalArgumentException` messages that
`FlightBearerTokenAuthenticator.validateBearer` logs at ERROR:
```java
LOG.info("Created flight token for user: {}, token: {}", username, token);
```
A bearer token is a complete credential until it expires —
`arrow_flight_token_alive_time_second` defaults to 86400s. So anyone who
can read `fe.log`, or the log aggregation platform it is shipped to, or
a backup of either, can take a live token, send it as `Authorization:
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 in `MetaService`; 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.info` calls in `FlightTokenManagerImpl`, the four
`IllegalArgumentException` messages in
`validateToken`/`getTokenDetails`, the one in
`FlightSessionsWithTokenManager.createConnectContext`, and the teardown
warning in `FlightSqlConnectPoolMgr.unregisterConnection`. That last one
is worth spelling out: a Flight SQL `ConnectContext`'s **`peerIdentity`
is the bearer token itself** —
`FlightBearerTokenAuthenticator.createAuthResultWithBearerToken` returns
the token as the peer identity, and `FlightSqlConnectPoolMgr` keys its
`flightToken2ConnectionId` map by it — so `ctx.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:
- `Env` logs 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 gets `maskPrefix`, consistent with how
`MetaService` already renders the same token.
- `Auth` echoes `initial_root_password` into 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 the `FlightSqlConnectPoolMgr` case
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: `peerIdentity` had 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 in
`fe/`** 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)
- Test <!-- At least one of them must be included. -->
- [ ] 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 <!-- Add your reason? -->
`TokenMaskerTest` covers 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, and `maskPrefix`
including 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-line `FlightSqlConnectPoolMgr` call,
each fail the build at that line with the new message; restoring them
goes back to green.
- Behavior changed:
- [ ] No.
- [x] Yes. <!-- Explain the behavior change -->
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 of `fe.log` would
need to use the id instead. No API, wire format or configuration
changes.
- Does this need documentation?
- [x] No.
- [ ] Yes. <!-- Add document PR link here. eg:
apache/doris-website#1214 -->
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Member
Author
|
run buildall |
Contributor
FE UT 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.
#66572