[improvement](fe) add fe_meta_auth_token for FE meta-service internal HTTP auth#65551
Open
CalvinKirs wants to merge 1 commit into
Open
[improvement](fe) add fe_meta_auth_token for FE meta-service internal HTTP auth#65551CalvinKirs 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 |
Contributor
TPC-H: Total hot run time: 29657 ms |
Contributor
TPC-DS: Total hot run time: 180017 ms |
Contributor
ClickBench: Total hot run time: 25.13 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
FE Regression Coverage ReportIncrement line coverage |
CalvinKirs
force-pushed
the
fe-meta-auth-token
branch
from
d58d9b2 to
ec7a7cd
Compare
Member
Author
|
/review |
Member
Author
|
run buildall |
gavinchou
reviewed
Jul 14, 2026
gavinchou
reviewed
Jul 14, 2026
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review complete. I found two blocking issues around the new FE meta token/config surfaces:
/rest/v2/manager/node/configand the FE branch of/configuration_infodo not actually enforce ADMIN in the default configuration becausecheckAdminAuthis conditional onenable_all_http_auth.fe_meta_auth_tokenis exposed through the legacy/rest/v1/config/feAPI to Basic-authenticated on-prem users.
Checkpoint conclusions: the FE-to-FE meta request propagation, /put port handling, storage-token response behavior, empty-token compatibility, and /dump password check look consistent from static review. Concurrency, lifecycle, persistence, transaction, and data-write concerns are not materially involved in this PR. The blocker is config/auth handling: the new sensitive token is a plain config field and the HTTP config readers are not consistently ADMIN-gated or masked. Tests cover token propagation, /put port behavior, and /dump password invocation, but they miss non-admin denial for both config APIs. No additional user focus was provided. Validation was static only because this review environment lacks .worktree_initialized, thirdparty/installed, and thirdparty/installed/bin/protoc, and the prompt forbids builds or code modifications.
| // with the SQL "SHOW FRONTEND CONFIG". The internal FE-to-FE fan-out (configurationInfo) | ||
| // is itself ADMIN-gated and forwards the admin credential, so it is unaffected. | ||
| ActionAuthorizationInfo authInfo = executeCheckPassword(request, response); | ||
| checkAdminAuth(authInfo.userIdentity); |
Contributor
There was a problem hiding this comment.
This still does not enforce ADMIN in the default configuration. checkAdminAuth only calls checkGlobalAuth(..., ADMIN) when Config.enable_all_http_auth is true; with the default false, it returns without checking privileges. This endpoint then returns raw ConfigBase.getConfigInfo(null) values, including sensitive config such as auth_token and the new fe_meta_auth_token, so an authenticated non-admin can still read them through /rest/v2/manager/node/config (and through the FE branch of /configuration_info, which uses the same helper before fan-out). If this should match SHOW FRONTEND CONFIG, use an unconditional ADMIN check here, for example checkGlobalAuth(authInfo.userIdentity, PrivPredicate.ADMIN), and apply the same gate before FE config fan-out.
| + "image/role/check/put/journal_id)在原有 node-host 校验之上,额外要求调用方携带匹配的 token 头。" | ||
| + "为空(默认)时维持仅 node-host 校验的旧行为,存量集群与滚动升级不受影响。必须在所有 FE 上取值一致," | ||
| + "并在启用前写入 fe.conf,否则 FE 之间会互相拒绝。"}) | ||
| public static String fe_meta_auth_token = ""; |
Contributor
There was a problem hiding this comment.
This token is added as a regular @ConfField, so it is also returned by the legacy config API. Config.dump() includes every annotated field with the raw value, and /rest/v1/config/fe?conf_item=fe_meta_auth_token serves that map. Although /rest/v1/** goes through AuthInterceptor, the Basic Authorization path only checks the password and skips ADMIN_OR_NODE unless Config.isCloudMode() is true, so a normal authenticated on-prem user can read the meta-service token. Please either gate this config reader with the same unconditional ADMIN check as SHOW FRONTEND CONFIG, or add a sensitive/masked config mechanism and mark this token so every config dump API masks or omits it.
gavinchou
reviewed
Jul 14, 2026
| if (!Strings.isNullOrEmpty(clusterToken)) { | ||
| String requestToken = request.getHeader(MetaBaseAction.TOKEN); | ||
| if (!clusterToken.equals(requestToken)) { | ||
| LOG.warn("reject meta request with invalid token. client: {}, {}, request from: {}", |
Contributor
There was a problem hiding this comment.
Here you can output the prefix to check, the rest is masked, for example, abc*** is not equal to bbc***, and so on, both confidential and can debug
Member
Author
There was a problem hiding this comment.
Why would there be a need for debugging?
Contributor
There was a problem hiding this comment.
sometimes we may need to know why the token is invalid, result inequality hides too much.
especially when we support rotation for the auth token, once we encounter auth fail, we know reason instantly by looking at this log.
observability matters.
CalvinKirs
force-pushed
the
fe-meta-auth-token
branch
from
ec7a7cd to
e745f08
Compare
gavinchou
reviewed
Jul 14, 2026
| // because here we need to obtain selfNode through the official service catalog. | ||
| HostInfo selfNode = Env.getServingEnv().getSelfNode(); | ||
| Env env = Env.getServingEnv(); | ||
| HostInfo selfNode = env.getSelfNode(); |
Contributor
There was a problem hiding this comment.
why change this? it doesnt make scense to make one-line into 2.
gavinchou
reviewed
Jul 14, 2026
| HostInfo selfNode = env.getSelfNode(); | ||
| conn.setRequestProperty(Env.CLIENT_NODE_HOST_KEY, selfNode.getHost()); | ||
| conn.setRequestProperty(Env.CLIENT_NODE_PORT_KEY, selfNode.getPort() + ""); | ||
| setClusterToken(conn, getClusterToken()); |
Contributor
There was a problem hiding this comment.
is this wrapper necessary?
gavinchou
reviewed
Jul 14, 2026
| @@ -20,8 +20,10 @@ | |||
| import org.apache.doris.catalog.Env; | |||
Contributor
There was a problem hiding this comment.
I remember there is another util like httpurlutil, InternalHttpsUtils.java
does it matter?
Member
Author
There was a problem hiding this comment.
InternalHttpsUtils only provides the shared SSLContext/truststore for TLS; HttpURLUtil builds the JDK HttpURLConnection and adds the node-ident + token headers, delegating TLS to InternalHttpsUtils. They're layered, not duplicated. The token is an auth header on the checkFromValidFe-protected meta endpoints, all of which go through HttpURLUtil, and it is sent unconditionally so it rides over HTTPS as well — so InternalHttpsUtils is orthogonal to this change and HTTPS works.
CalvinKirs
force-pushed
the
fe-meta-auth-token
branch
from
e745f08 to
ede0012
Compare
Member
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29519 ms |
Contributor
TPC-DS: Total hot run time: 177555 ms |
Contributor
ClickBench: Total hot run time: 25.05 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
CalvinKirs
force-pushed
the
fe-meta-auth-token
branch
from
ede0012 to
860a29d
Compare
Member
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 28986 ms |
Contributor
TPC-DS: Total hot run time: 177527 ms |
Contributor
ClickBench: Total hot run time: 24.95 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
CalvinKirs
force-pushed
the
fe-meta-auth-token
branch
from
860a29d to
2704914
Compare
Member
Author
|
run buildall |
Member
Author
|
/review |
Contributor
TPC-H: Total hot run time: 29156 ms |
Contributor
There was a problem hiding this comment.
Review complete. I found two blocking issues:
/dumpis now password-gated but still not ADMIN-gated before it creates a full metadata image and returns the server-side path.- The new sensitive-config masking is opt-in, so
auth_tokenremains unmasked through the legacy/rest/v1/config/fepath for default on-prem Basic-authenticated users.
Checkpoint conclusions: the FE-to-FE token propagation, empty-token compatibility, /put port validation, HTTPS path, invalid-token handling, and v2 manager config ADMIN gates look consistent from static review. The PR is focused and mostly follows existing helper paths. I did not find material concurrency, lifecycle, persistence, transaction, FE-BE protocol, or data-write issues beyond the metadata dump side effect noted inline. Tests cover the new positive token/config paths, but they miss the non-admin /dump denial and existing-secret masking cases called out below.
User focus: No additional user-provided review focus was supplied.
Validation: Static review only; this review prompt forbids builds, tests, and source modifications.
|
|
||
| // If true, the value is a secret (e.g. a token or password) and is masked in every | ||
| // config dump API (Config.dump / getConfigInfo), so it is never returned in plaintext. | ||
| boolean sensitive() default false; |
Contributor
There was a problem hiding this comment.
Because this new masking is opt-in, marking only fe_meta_auth_token still leaves existing internal secrets visible. Config.auth_token is still a regular @ConfField, and /rest/v1/config/fe still serves Config.dump(); on the default on-prem Basic-auth path the interceptor authenticates the password but does not require ADMIN, so conf_item=auth_token can still return the raw cluster token. Please either make the legacy config reader ADMIN-only as well, or mark existing real secrets (at least auth_token) as sensitive = true and add coverage for that path.
| if (Config.enable_all_http_auth) { | ||
| executeCheckPassword(request, response); | ||
| } | ||
| executeCheckPassword(request, response); |
Contributor
There was a problem hiding this comment.
executeCheckPassword() only proves the caller has valid credentials; it does not enforce any privilege. Since /dump is excluded from AuthInterceptor, this lets any Basic-authenticated user call Env.dumpImage(), which takes the catalog/db/table locks, writes a full metadata image, and returns its server-side path. This should be ADMIN-gated like the other metadata/debug operations: capture the returned ActionAuthorizationInfo and call checkGlobalAuth(authInfo.userIdentity, PrivPredicate.ADMIN) before dumping, with a negative test for an authenticated non-admin user.
Contributor
TPC-DS: Total hot run time: 177700 ms |
Contributor
ClickBench: Total hot run time: 24.94 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
… HTTP auth The FE meta-service endpoints (image/role/check/put/journal_id, etc.) only authenticate callers by the CLIENT_NODE_HOST/PORT headers, i.e. whether the claimed host:port is a registered FE. That is a plaintext claim with no secret, so any host that knows a valid FE address can impersonate it. Add an optional cluster token, fe_meta_auth_token: - When empty (default), behavior is unchanged: node-host check only. Existing clusters and rolling upgrades are unaffected. - When set, checkFromValidFe additionally requires the request to carry a matching token header, on top of the existing node-host check (the token check is additive, it does not replace the host check). The token is a static fe.conf item, so a scaling-out FE already holds it before the bootstrap handshake -- no chicken-and-egg with the token that /check hands out. Must be identical on all FEs. Keep the token out of every config dump API. Add a `sensitive` attribute to @ConfField and mark fe_meta_auth_token sensitive; ConfigBase.dump() and getConfigInfo() then return a mask instead of the real value. This closes the plaintext leak through both /rest/v2/manager/node/config and the legacy /rest/v1/config/fe (Config.dump), regardless of how each endpoint is gated. An empty value is left as-is so "unset" stays visible. Also gate the FE config readers with an unconditional ADMIN check (checkGlobalAuth(..., ADMIN)) on /rest/v2/manager/node/config and the FE fan-out of /configuration_info, matching the SQL "SHOW FRONTEND CONFIG". Note that the existing checkAdminAuth helper only enforces ADMIN when enable_all_http_auth is true, so it is a no-op by default and cannot be relied on here. Also harden the meta-service: - /put rejects a port other than the expected FE HTTP port. The expected port follows HttpURLUtil.getHttpPort() (https_port when enable_https=true, else http_port), matching what the master sends, so the check is correct under HTTPS instead of always comparing against http_port. - /dump always checks the admin password. - meta-helper logs header names only (never the token value). Tests: MetaServiceTest covers matching/missing/wrong token, no-token-when- unconfigured, unknown-host-rejected-even-with-token, /put port check under both HTTP and HTTPS, and /dump auth; HttpURLUtilTest covers token header emission and internal URL building; ConfigTest covers sensitive-config masking (set value is masked in dump/getConfigInfo, empty value is not).
CalvinKirs
force-pushed
the
fe-meta-auth-token
branch
from
2704914 to
72c0015
Compare
Member
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29105 ms |
Contributor
TPC-DS: Total hot run time: 177281 ms |
Contributor
ClickBench: Total hot run time: 25.13 s |
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
3 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?
The FE meta-service endpoints (
image/role/check/put/journal_id, etc.) authenticate callers only by theCLIENT_NODE_HOST/CLIENT_NODE_PORTheaders — i.e. whether the claimed host:port is a registered FE. That is a plaintext claim with no secret, so any host that knows a valid FE address can impersonate it.What this PR does
Add an optional cluster token
fe_meta_auth_token:checkFromValidFeadditionally requires the request to carry a matching token header, on top of the existing node-host check (additive, does not replace the host check).The token is a static
fe.confitem, so a scaling-out FE already holds it before the bootstrap handshake — no chicken-and-egg with the token that/checkitself hands out. It must be identical on all FEs.Additional hardening of the meta-service:
/putrejects a port other than the FE HTTP port./dumpalways checks the admin password.Tests
MetaServiceTest: matching / missing / wrong token, no-token-when-unconfigured, unknown-host-rejected-even-with-token,/putport check,/dumpauth.HttpURLUtilTest: token header emission (present/absent) and internal URL building.This supersedes #63782 (rebased onto latest master with a cleaner, switch-free design).