[Bug] PD REST auth checks only the service name, never the password, and returns HTTP 200 on refusal; fix both in 1.8.0

[bitflicker64]

Bug Type (问题类型)

logic (逻辑设计问题)

Before submit

• I have confirmed and searched that there are no similar problems in the historical issues and documents

Environment (环境信息)

• Server Version: 1.7.0 (released) and master at 98477f0; the two files involved are byte-identical in behaviour on both, master only adds TODO comments on top
• Backend: hstore, 3 PD x 3 Store x 3 Server on Kubernetes (Helm chart from feat(helm): add HStore deployment chart #3132); the behaviour is in PD and does not depend on the deployment
• OS: measured against hugegraph/pd at revision 98477f0 on a kind cluster and against the same image standalone in Docker
• Data Size: n/a

Expected & Actual behavior (期望与实际表现)

Two defects in PD's REST authentication interceptor. They fail in opposite directions, so they are described separately, but they live in the same two classes and should be fixed together.

Expected. A request to a PD management endpoint carries a Basic credential; the interceptor verifies both the service name and its password; a refused or missing credential gets a 401 so callers and monitors can tell.

Actual, part 1: the password is never read. hg-pd-service/.../service/interceptor/Authentication.java decodes the Basic credential, keeps only the part before the colon, and checks it against a fixed set:

private static final Set<String> innerModules = Set.of("hg", "store", "hubble", "vermeer");
// TODO: password validation is skipped — only service name is checked against
// innerModules. Full credential validation should be added as part of the auth refactor.
//String pwd = info.substring(delim + 1);
if (innerModules.contains(name)) { return call.get(); }
throw new AccessDeniedException("invalid service name");

So any of the four names with any password, including an empty one, is treated as an internal component. The names are in the source, so they are not a secret.

Actual, part 2: every outcome is HTTP 200. RestAuthentication.preHandle writes {"error":"Unauthorized!","status":-1} for a missing header and the AccessDeniedException message for an unknown name, but never calls setStatus. Success, refusal and no credential all return 200 with the result in the body.

Measured on 2026-09-02, 27 requests, all returned HTTP 200:

Credential	/v1/members	/v1/stores	/v1/allInfo
no header	Unauthorized!	Unauthorized!	Unauthorized!
-u hg: (empty password)	leader and member list	store list	full registry
-u hg:wrongpassword	leader and member list	store list	full registry
-u store: -u hubble: -u vermeer:	leader and member list	store list	full registry
-u admin:<real admin password>	invalid service name	invalid service name	invalid service name
-u nobody:x	invalid service name	invalid service name	invalid service name

The real admin password is refused because its name is not in the set, and an empty password with the right name succeeds: the password is inert.

Why it matters: the endpoints behind that check mutate the cluster. Everything under /v1 except /v1/health, /actuator/*, /v1/prom/targets/* (and /v1/ready once #3185 lands) sits behind the interceptor, and that includes:

• POST /v1/members/change, which rewrites the raft peer list (master carries a TODO on this handler saying any caller with network access can trigger it)
• DELETE /v1/store/{storeId}, which removes a Store
• POST /v1/store/{storeId}, which sets a Store's state
• POST /v1/graph/** and POST /v1/graph-spaces/**, which write graph and graphspace configuration
• GET /v1/task/patrolPartitions, balanceLeaders, balancePartitions, which trigger data movement

The 1.7.0 PD README gives docker run ... -p 8620:8620 and says to open port 8620 for REST, with no note about authentication, so a deployment that follows the released docs exposes these to anyone who can reach the port with a four-letter username.

Why part 2 matters on its own. Nothing keyed on a status code can see a refusal: monitors do not fire, curl -f never fails (the shipped compose healthchecks use exactly that), and a refused or bypassed request leaves no 401 anywhere. It also bites documentation: the Helm chart's recovery commands originally ran without a credential, printed 200 three times and did nothing, which is how this was found.

Proposal: fix both in 1.8.0, as one bundle

1. In Authentication.authenticate, read the password and compare it with the token stored for that service name (the PD/TOKEN/<name> path already exists), refusing on mismatch.
2. In RestAuthentication.preHandle, call response.setStatus(401) before writing the error body, for both the missing-header and the refused case.
3. Wire the token into every client that currently gets in on the name alone: the Server's PD registration, the Store, Hubble (operations.pd.password is empty in the shipped configuration) and the compose files. The Helm chart will add a Secret and env wiring on its side (feat(helm): add HStore deployment chart #3132).
4. Document the tokens on every page that names port 8620, and until then add a note to the PD README and the site docs that 8620 must not be exposed off a trusted network.

Probes do not need to change: #3185 adds /v1/ready to the unauthenticated list for exactly that reason.

Why 1.8.0 is the right boundary rather than a patch release. Checking passwords breaks every current client at once, so this cannot ship as 1.7.x; it has to land with the client wiring in the same release. 1.8.0 is already that release: about 100 commits since 1.7.0, including the compose topology rebuilt twice (#2952, #3149), shipped config defaults changed in seven files, outdated backends removed (#3116), the legacy scheduler disabled (#3082), the Server REST API adapted (#3159) and two proto files changed. An operator upgrading has to redo their deployment anyway, and the docs for those pages are being rewritten now. The 1.7.0 install base is small (289 tarball downloads in ten months), so the population that has to set a token is bounded, and it is the same population that is already re-reading its configuration for 1.8.0. Waiting for 1.9.0 means one more release in which a documented deployment exposes a mutating management API behind a public username.

Related

• [Bug] PD /v1/health reports healthy without a raft quorum; add a quorum-aware readiness signal #3183 and feat(pd): add quorum-aware /v1/ready endpoint and raft gauges #3185: the readiness endpoint work; feat(pd): add quorum-aware /v1/ready endpoint and raft gauges #3185 keeps probes off the authenticated surface, which is what lets this fix leave probes alone.
• feat(helm): add HStore deployment chart #3132: the Helm chart, which currently documents this in Limitations and writes its recovery commands as curl -u hg: ... because that is what works today.

[Adarsh-Me]

Hi! I'd like to take this one. Could a maintainer assign it to me? My plan is to reproduce both defects against the PD REST interceptor, fix the credential check so it verifies the password (not just the service name) per the configured auth settings, return a proper 401 instead of HTTP 200 on failure, and add tests covering both the too-permissive and wrong-status paths. Thanks!

PS: since this touches authentication, happy to coordinate through the project's security process instead if you'd prefer.

[bitflicker64]

hey!! @Adarsh-Me thanks for the interest, but i'm already preparing a patch for this one, it's kinda important and it has to land together with the client side wiring (server, store, hubble, compose) for 1.8.0, so i'd rather keep it in one set with #3185 and #3187

since you're interested in the auth side, two open ones nobody has picked up: #3133 is the docker entrypoint auth bootstrap (grep/sed against rest-server.properties breaks on mounted and upgraded configs, concrete cases are listed there), and #3019 is the auth REST docs where /auth/groups dropped the /graphspaces/DEFAULT prefix but the other auth endpoints still have it, so either the docs or the paths need to agree!!

on the security part, the gap is already written in the code as a todo on master, so tracking it here is fine