CalvinKirs opened a new pull request, #66572:
URL: https://github.com/apache/doris/pull/66572
### 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.
All eight token-valued sites in the Arrow Flight path are converted: the
four `LOG.info` calls in `FlightTokenManagerImpl`, the four
`IllegalArgumentException` messages in `validateToken`/`getTokenDetails`, and
the one in `FlightSessionsWithTokenManager.createConnectContext`.
While auditing for other instances, one more was found and fixed: `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 the same treatment — `maskPrefix`, consistent with how `MetaService`
already renders the same token.
Finally, a checkstyle rule rejects a variable whose name says it holds a
token/password/secret being passed straight into a `LOG.x(...)` call, in either
the parameter or the string-concatenation form. It is a best-effort guard —
being line based, it cannot see a credential that sits on a continuation line
of a multi-line log statement — but it catches exactly the shape that caused
this bug. It reports **no violation anywhere in `fe/`** today, 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 fails the build at
that line with the new message, and removing it 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:
https://github.com/apache/doris-website/pull/1214 -->
--
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]
