deardeng opened a new pull request, #70006:
URL: https://github.com/apache/doris/pull/70006
Related PR: #69732
Problem Summary: Meta service and recycler write secrets to their logs, and
some into error messages, in plaintext:
- http admin token: `MetaServiceImpl::http` formats the whole http request
(every query string and header) and logs it at INFO for every request,
successful or not, so the `token` in the query string, which can act on every
instance, reaches the log. When the token check fails, meta service and
recycler also echo the supplied token in the 403 body and the WARNING log.
- cloud_unique_id: since #69732 a cloud_unique_id
`${version}:${instance_id}:${node_secret}` authenticates a node, but
`begin_rpc` / `finish_rpc` log the request and response debug strings (e.g.
every `get_cluster` logs the cloud_unique_id of the caller and of every node in
the returned cluster), the http request log contains it, about 60 log lines and
error messages print it, and about 40 dump an instance, cluster or node PB that
contains it.
- ak/sk: when `alter_obj_store_info`, `alter_storage_vault`,
`alter_ram_user` or `update_ak_sk` rejects a request, the error message embeds
the request as JSON, ak/sk included; `finish_rpc` logs it and the caller
receives it. `begin_rpc` hides sk only for CreateInstance, CreateStage and
AlterObjStoreInfo, so `update_ak_sk` and `alter_ram_user` log the sk.
Fix:
1. Mask the `token` query value as `***` in the logged http request, and
drop the echoed token from the 403 message (keep `token=(not given)` when no
token is passed).
2. Add `hide_cloud_unique_id()`, which keeps `${version}:${instance_id}:`
for debugging and replaces the node secret with `***`, and
`hide_cloud_unique_ids()`, which applies it to every cloud_unique_id(s) value
in a protobuf debug string, a JSON string or a `cloud_unique_id=xxx` message.
Apply the latter once to every debug string in `begin_rpc` / `finish_rpc` and
to the logged http request, and apply them at the other log lines and error
messages.
3. Add `request_to_json_for_log()`, which applies `encryt_sk()`, `hide_ak()`
and `hide_cloud_unique_ids()` to the request JSON, and use it wherever
meta_service_resource.cpp puts a request in an error message or a log. Add
UpdateAkSkRequest and AlterRamUserRequest to the requests whose sk `begin_rpc`
hides.
`hide_cloud_unique_ids()` only scans for "cloud_unique_id" and copies the
string once when there is a match. Compared with the `ShortDebugString()` it
wraps, on a Release build it adds 0.65us to a 91-byte request, 0.65ms (7%) to a
1.7MB request with one cloud_unique_id, and 0.4ms (29%) to a `get_cluster`
response with 2000 nodes.
Meta service logs and error messages show cloud_unique_id as
`1:<instance_id>:***`, the sk as an md5 and the ak partially hidden, and no
longer contain the http admin token. The 403 body of meta service and recycler
http APIs is `incorrect token` instead of `incorrect token, token=xxx`.
- Test: Unit Test
- `MetaServiceHttpTest.TokenNotLogged` captures glog output of one
authorized and one rejected http request and checks that neither token value
appears.
- `MetaServiceHttpTest.HideCloudUniqueIds` covers the masking formats.
- `MetaServiceHttpTest.CloudUniqueIdNotLogged` captures glog output of
an http `get_cluster` call and checks that neither the caller's nor the cluster
node's secret is logged (leaks via the http body, `begin_rpc` and `finish_rpc`).
- `MetaServiceHttpTest.AkSkNotLogged` captures glog output of rejected
`alter_obj_store_info`, `alter_storage_vault`, `alter_ram_user` and
`update_ak_sk` calls and checks that neither ak nor sk is logged.
- Each of these fails without the change.
- Behavior changed: Yes. The 403 body no longer echoes the supplied token,
and error messages that contained a cloud_unique_id or ak/sk now contain the
masked form.
- Does this need documentation: No
### What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
### Release note
None
### 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? -->
- Behavior changed:
- [x] No.
- [ ] Yes. <!-- Explain the behavior change -->
- Does this need documentation?
- [x] No.
- [ ] Yes. <!-- Add document PR link here. eg:
https://github.com/apache/doris-website/pull/1214 -->
### Check List (For Reviewer who merge this PR)
- [ ] Confirm the release note
- [ ] Confirm test cases
- [ ] Confirm document
- [ ] Add branch pick label <!-- Add branch pick label that this PR should
merge into -->
--
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]
