morningman opened a new pull request, #67794:
URL: https://github.com/apache/doris/pull/67794
### What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Two independent gaps found while surveying the FE/BE HTTP operations APIs,
both of which are prerequisites for adding a BE-local operations token later.
Neither changes any authorization decision.
**1. The config update endpoints write secrets to the log in the clear.**
`/api/update_config` takes its argument as `<config name>=<new value>` in
the query string, and logs the request unconditionally on entry. Three places
wrote the value out:
- `HttpRequest::debug_string()` masked headers but not query parameters —
and not the `uri:` line either, which is the raw request URI, query string and
all. This reaches past the config API: the cluster token travels as the `token`
parameter of the download endpoints, and `debug_string()` is also what
`HttpHandlerWithAuth::on_header` logs on every auth failure.
- `/api/show_config` masked exactly one config, `tls_private_key_password`,
by name. The BE had no notion of a config being a secret at all.
- Neither side recorded who changed a config. The BE logged `set_config k=v`
with no identity; the FE logged nothing on success and printed the value in the
clear on failure.
**2. `/api/injection_point/{op}` never reached the auth gate.**
`InjectionPointAction` inherited `HttpHandler` directly, whose `on_header()`
is a plain `return 0`. Six handlers inherit `HttpHandler` directly; the three
stream load ones parse Basic credentials themselves and forward them to the FE,
which is deliberate. This one had no authentication code of any kind. It only
registers under `ENABLE_INJECTION_POINT`, which `build.sh` defaults to `OFF`,
so production builds do not carry the endpoint — the reason to fix it is to
leave one shape of handler in the tree, so that a rule against inheriting
`HttpHandler` directly is easier to state and to review against.
#### How
The BE now marks a config as holding a secret at its declaration, the way it
already marks one as mutable:
```cpp
DECLARE_String_Sensitive(tls_private_key_password); // config.h
DEFINE_String_Sensitive(tls_private_key_password, ""); // config.cpp
```
Only strings can hold a secret, so only strings get the variant.
`Register::Field` carries the bit through a defaulted parameter, so no existing
`DEFINE_FIELD` site changes.
Three configs are marked: `tls_private_key_password` (the one that was
special-cased) plus `test_s3_ak` / `test_s3_sk`. Everything else in `config.h`
that matches on name turns out to be a path or a switch, and a `*_key` pattern
would catch far more ordinary configs than secrets.
`get_config_info()` now runs every value through `mask_config_value()`,
which also covers `information_schema.backend_configuration`. The rendered mask
is the same `******` the special case used, so what those two report is
byte-for-byte unchanged.
Both sides write one audit line per config, whether the update took effect
or not, at the level the outcome was reported at before (INFO on success,
WARNING/WARN on failure):
```
update_config: remote=127.0.0.1, user=root, config=..., old=..., new=...,
persist=false, result=OK
set_config: remote=127.0.0.1, user=root, config=..., old=..., new=...,
persist=false, result=OK
```
The old value is read before the update and masked like the new one. `user`
is the identity the caller *presented*: with `enable_all_http_auth` off the
request is never authenticated, so the field is a claim, and the code says so
in as many words rather than letting a reader of the log assume otherwise.
Two adjacent defects fixed in passing, both called out so they are not
mistaken for part of the masking work:
- `handle_update_config` dereferenced the `end()` iterator of the parameter
map when the request carried no `persist` parameter.
- `InjectionPointAction`'s header had no include guard.
### Release note
None
### Check List (For Author)
- Test
- [x] Unit Test
- [x] Manual test (add detailed scripts or steps below)
Unit tests, all green locally on macOS/arm64 (`BUILD_TYPE_UT=Debug`):
```
ConfigTest.SensitiveConfigsAreMasked OK
HttpRequestTest.sensitive_query_params_are_masked OK
HttpRequestTest.sensitive_config_names_are_masked_as_query_params OK
HttpRequestTest.non_sensitive_query_params_are_untouched OK
HttpRequestTest.uri_masking_handles_query_string_edge_cases OK
```
`uri_masking_handles_query_string_edge_cases` covers the shapes a query
string can take: no query, empty query, a parameter with no value, a trailing
`&`, an empty value, a url-encoded parameter name (`%74oken`), and a value
containing `=`.
Manual check that the `ENABLE_INJECTION_POINT` registration still compiles:
that line is inside `#ifdef ENABLE_INJECTION_POINT`, so a default build does
not see it. `http_service.cpp` was compiled standalone with
`-DENABLE_INJECTION_POINT -fsyntax-only`, as were the other six changed
translation units — the tree builds with unity enabled, which can mask
incomplete-type errors in a merged TU.
- Behavior changed:
- [x] Yes.
The log format changes: anything grepping the BE log for `set_config k=v`
will need to follow the new `update_config:` line. No API response changes, no
system table changes, and no authorization decision changes anywhere.
- Does this need documentation?
- [x] No.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_012tTm4UwDp7rZ1tjMfDx1Ho
--
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]
