fix(pd): validate REST credentials and return 401 on refusal - #3189
fix(pd): validate REST credentials and return 401 on refusal#3189bitflicker64 wants to merge 12 commits into
Conversation
The REST authentication interceptor had two defects (apache#3188): 1. Authentication.authenticate decoded the Basic credential but checked only the service name against the innerModules set, so any of the four public names with any password, including an empty one, was accepted, while the password was never read. 2. RestAuthentication.preHandle wrote an error body without calling setStatus, so success, refusal and missing credential all returned HTTP 200 and nothing keyed on a status code could see a refusal. Fix both together: - Compare the password of the Basic credential against the shared secret configured via auth.secret-key (constant-time comparison). A missing or empty secret refuses every request instead of falling back to name-only authentication. - Return 401 for a missing, malformed or refused credential. - Surface auth.secret-key in both shipped application.yml files with a change-in-production note, and let the Docker image set it through a new optional HG_PD_AUTH_SECRET_KEY env (never logged). - Wire the in-repo clients: wait-storage.sh now defaults its PD password to the shipped secret, and the Compose Hubble properties files carry a matching operations.pd.username/password pair. - Update the PD test credentials to the shipped secret and add REST tests asserting 401 for missing credential, wrong password, empty password and unknown service name. - Document the credential and the trusted-network requirement for port 8620 in the PD and Compose READMEs. Probes are unaffected: /v1/health, /actuator/* and /v1/prom/targets/* stay outside the interceptor.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3189 +/- ##
============================================
+ Coverage 37.78% 37.82% +0.03%
- Complexity 6556 6576 +20
============================================
Files 800 800
Lines 68929 68983 +54
Branches 9157 9171 +14
============================================
+ Hits 26046 26092 +46
+ Misses 39824 39823 -1
- Partials 3059 3068 +9 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The gate itself is right (name check, constant-time comparison, fail-closed on an empty secret, and the long-missing setStatus), but one blocker, two important items and four nits below. The blocker: PDConfig.java:72 defaults auth.secret-key to a quoted literal, so a PD whose config file omits the key rejects the secret this PR documents, and wait-storage.sh then burns its 300s timeout and aborts Server startup. The two important ones are the same failure reached by rotating the secret per docker/README.md, and the actuator exposure list on the port being hardened. Evidence: new StandardEnvironment().resolvePlaceholders("${auth.secret-key: 'FXQ...'}") on spring-core 5.3.20, the line spring-boot-starter-web:2.5.14 resolves, returns " 'FXQXbJtbCLxODc6tGci732pkH1cyf8Qg'", leading space and quotes included. CI at f796637 is green on pd, store, struct, CodeQL and every build-server job; the red hstore job is VertexCoreTest.testQueryByJointIndexesWithSearchAndTwoRangeIndexesAndWithin (expected 3, was 1), the index-ordering defect #3182 addresses, not this change.
imbajin
left a comment
imbajin
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: The new REST credential remains a fixed repository-published secret while the default HStore Compose topologies publish PD's management port to the host. This makes the credential reusable by anyone who can reach those ports, so deployment protection still depends on operators noticing the warning. Evidence: exact head f796637; the added shipped configuration contains a fixed auth.secret-key, while docker-compose-hstore.yml and docker-compose-3pd-3store-3server.yml publish PD REST ports. Please require a deployment-provided or generated secret, or fail startup while the public default remains.
Review follow-ups on the REST credential change.
The blocker: PDConfig declared the key as
`@Value("${auth.secret-key: 'FXQ...'}")`. Spring takes the text after the
first colon as a literal default, so a PD whose conf/application.yml has no
auth block resolved the secret to " 'FXQ...'", quotes and leading space
included. That is neither empty nor anything a client sends, so the
fail-closed branch never ran and PD rejected the secret shipped in both
config files, wait-storage.sh and the Hubble properties. Since
start-hugegraph-pd.sh passes -Dspring.config.location, which replaces the
default locations rather than adding to them, an upgrade that keeps an
existing config file hit this and Server startup aborted after the 300s
wait-storage timeout. Drop the default so an absent key yields "", and log
an error naming the parameter once so the refusal is diagnosable.
Also from the review:
- Pass the secret to every consumer from one variable. Both Compose files
now set HG_PD_AUTH_SECRET_KEY on each PD service and PD_AUTH_PASSWORD on
each Server, so rotating one .env value keeps the Server's wait-storage.sh
probe working. Hubble still needs a manual edit of its mounted properties
file, which the docs now say.
- Narrow the actuator exposure from "*" to health,metrics,prometheus in both
configs. /actuator/* is excluded from the interceptor, so anything exposed
there is anonymous on the port this change is hardening.
- Send WWW-Authenticate on the 401, per RFC 7235. Without it clients that
authenticate reactively never retry with credentials.
- Record in the GRpcServerConfig TODO that the secret check now lives in the
shared base class, so enabling the gRPC interceptor also requires giving
the Server, Store and CLI clients the secret.
- Document the credential in the PD configuration and API references, and
credential the balanceLeaders rebalancing procedure in the Store
operations guide, where a 401 reads as a no-op during an incident.
Verified against the packaged dist: with the auth block removed from
conf/application.yml every authenticated request is refused and the error
names auth.secret-key; with it present the matrix is unchanged, the 401
carries the challenge header, and /actuator/env, /beans and /configprops no
longer serve data.
imbajin
left a comment
imbajin
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: REST credential validation and status handling are wired correctly. One edge case remains in the Docker secret override: raw JSON control characters can make the generated Spring configuration invalid. Evidence: an exact-head shell harness with a carriage return in HG_PD_AUTH_SECRET_KEY makes SPRING_APPLICATION_JSON invalid; Codecov failures are non-blocking.
Addresses the two remaining review points. A published default is not a secret. The REST credential shipped with a fixed value that lives in this repository, on a port the HStore Compose files publish to the host, so anyone who could read the source could authenticate as an internal service against endpoints that rewrite the raft peer list, remove stores and move data. Requiring a deployment-provided secret is the only version of this check that means anything. - Both application.yml files now ship auth.secret-key empty, with a comment saying why there is no default and how to generate one. PD already refuses every authenticated REST request while it is empty, naming the key in an error. - PD refuses to start when auth.secret-key is set to the value earlier revisions carried as a placeholder, so a deployment that copied it does not quietly keep a well-known credential. - The Docker image requires HG_PD_AUTH_SECRET_KEY, and both Compose files fail fast when it is unset rather than falling back to a shared value. The .env recipe generates one alongside the JWT secret. The Hubble properties files ship the password empty, with the manual step documented. - travis/start-pd.sh supplies a test-only secret through SPRING_APPLICATION_JSON, matching what the PD suites send, since the shipped configuration no longer authenticates anything. wait-storage.sh no longer interpolates the credential into the inner bash -c string, where a secret containing a space, a backtick or $(...) would have split the arguments or run, and no longer passes it in argv where anything able to read /proc could see it. The inner shell reads the value from the environment and hands it to curl on stdin as a config file. Its test now asserts the credential is absent from argv and present in that config. Verified against the packaged dist: startup is refused with the published placeholder, the shipped configuration starts but answers 401 to every credential, and a deployment-provided secret restores the matrix with the challenge header. PDRestSuiteTest 17/17, PDClientSuiteTest 45/45, test-wait-storage.sh 5/5, compose renders pass and refuse to render without the secret.
imbajin
left a comment
imbajin
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The auth-on hstore smoke path now fails before startup because its Compose invocation does not receive the required PD secret. Evidence: exact-head CI run https://github.com/apache/hugegraph/actions/runs/33798956133/job/100805004445 ended with required variable HG_PD_AUTH_SECRET_KEY is missing a value; the added HG_PD_AUTH_SECRET_KEY is passed only by compose_auth, while smoke calls compose_active.
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The gate now does what the title says: service-name check, constant-time comparison of the Basic password against auth.secret-key, fail-closed on an empty secret, and a 401 carrying a WWW-Authenticate challenge. The block is not one of mine, it is the open docker/test-compose.sh:24 thread, which is why no comment below carries a critical label; imbajin's docker-entrypoint.sh:64 thread on json_escape leaving CR and tab unescaped is also still unanswered at this head. Three new items are below, all about the credential being reachable by the clients that now need it.
Evidence: exact head 5c339c160356ad7683822826389953da542d31a6. build-server (rocksdb, 11) is the only non-Codecov red check, failing with required variable HG_PD_AUTH_SECRET_KEY is missing a value; compose_active at docker/test-compose.sh:271-278 does not pass it while compose_auth does. json_escape at hugegraph-pd/hg-pd-dist/docker/docker-entrypoint.sh:29-32 is unchanged at this head. A JDK harness over Authentication.java:85-93 prints accepted=true under -Dfile.encoding=UTF-8 and accepted=false under -Dfile.encoding=US-ASCII for the same correct non-ASCII secret.
Separately, the PR description still describes the design this head replaced: an optional HG_PD_AUTH_SECRET_KEY, wait-storage.sh defaulting to the shipped secret, and READMEs stating that the shipped default is public. 5c339c1 made the key required and empty by default and added the refuse-to-start guard, so the description is worth refreshing before it becomes the commit message.
Review follow-ups, including the CI failure the previous commit caused. The blocker was mine: requiring HG_PD_AUTH_SECRET_KEY in the Compose files without giving it to every caller of docker compose. Only compose_auth passed it, so the auth-on hstore smoke, which goes through compose_active, could not interpolate the file and failed before it started the stack. compose_active now passes it too. The Basic credential was decoded with the platform default charset while the secret it is compared against comes from the UTF-8 YAML, so a non-ASCII secret authenticated or not depending on the host locale. PD targets JDK 11, where the default charset still follows the locale, and the start script sets no -Dfile.encoding, so a PD started with LANG unset rejected a secret that worked from the operator's terminal, reporting only "invalid credential". Decode as UTF-8, which is what RFC 7617 specifies. Confirmed both ways: the old decode rejects `sécrèt-2026` under -Dfile.encoding=US-ASCII and the new one accepts it, and they agree under UTF-8. json_escape in the PD entrypoint escaped backslash and quote and dropped LF, leaving CR and TAB to produce invalid SPRING_APPLICATION_JSON and a container that failed before startup. It now escapes every C0 control character as \uXXXX. travis/test-pd-docker-entrypoint.sh covers the override: eight secrets including CR, TAB, quote, backslash and non-ASCII round-trip through the generated JSON, the missing-secret case is refused, and the secret never reaches the log. Wired into the pd CI job. Both README recipes were wrong in the same way, generating or expecting a secret the operator never has in their shell: - docker/README.md now loads .env before the commands that use the value, so the curl example stops sending an empty password and the Hubble sed stops rewriting the empty value to itself. It refuses to write an empty secret and names hstore-ha.properties for the HA topology. - hugegraph-pd/README.md generates the secret into a variable first, then passes it to docker run, instead of minting one inside the run line that only the container ever sees.
ServerClientTest.testServiceProxy failed with "bind(..) failed: Address already in use" on port 8090 in hugegraph-commons/hugegraph-rpc, a module this branch does not touch. Empty commit, no source change.
The suites that exercise this talk to a PD in another JVM, so none of the new branches were attributed to the build and codecov/patch read 0%. AuthenticationTest runs the check directly: each inner module accepted with the secret, wrong and empty passwords refused, unknown names refused, a malformed credential refused, an unset secret refusing everyone rather than falling back to a name check, a non-ASCII secret that would fail if the credential were decoded with the platform charset, and PDConfig refusing to start on the published placeholder. Added to PDCoreSuiteTest, which needs no running PD.
imbajin
left a comment
imbajin
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The new authentication test is placed where the exact head cannot compile, and additional rollout/security gaps remain in the timeout, template, documentation, and secret-injection paths. Evidence: exact-head CI reports cannot find symbol: class Authentication; local Bash/curl/Compose checks independently reproduce the remaining integration issues.
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: no on the four items below. Summary: the credential check itself is correct and fails closed (service name must be in innerModules, password compared to auth.secret-key with MessageDigest.isEqual over UTF-8 bytes, an empty or null secret refuses everyone), and the gRPC path is genuinely untouched because GRpcServerConfig.configure still never calls serverBuilder.intercept; what is left are deployment-integration and diagnosability gaps around the new fail-closed default. Evidence: static review of the full diff at ace8913a against merge-base 98477f0f, plus AuthenticationConfigurer, GRpcServerConfig, both compose files, and HubbleOptions.OPERATIONS_PD_PASSWORD in the hugegraph-toolchain checkout. Nothing was compiled and no suite was run here, so the open AuthenticationTest compile failure and the other threads from the latest review round are acknowledged, not re-litigated; CI is red at this head, consistent with that report.
The CI cascade at ace8913 was one mistake: AuthenticationTest lived in hg-pd-test, which declares hg-pd-service but cannot compile against it. The Spring Boot repackage replaces that module's main artifact with a fat jar whose classes sit under BOOT-INF/classes, invisible to javac, so every job that runs `mvn install` (commons, store, hstore, the Docker matrix, CodeQL, macOS) failed before any test ran. It only passed locally because `-am` builds from the reactor's target/classes. The test now lives in hg-pd-service's own test source set, with junit added there at test scope. Verified against the same `mvn install -Dmaven.test.skip=true` path CI uses. Also from the review: - The interceptor excluded /actuator/*, which Spring does not match against /actuator/metrics/{name} or /actuator/health/{group}, so those exposed probe paths returned 401. Now /actuator/**; what is reachable there is bounded by the exposure allowlist. RestApiTest asserts the nested metrics path answers without a credential and that /actuator/env stays closed. - application.yml.template still exposed "*" and carried no auth block, so an operator starting from the packaged template got neither hardening. travis/test-pd-shipped-config.sh now checks every shipped variant for an allowlisted exposure, an empty auth.secret-key and no published secret, wired into the pd job. - wait-storage.sh escaped only backslash and quote for the curl -K config, which is line-delimited: a secret containing a line break ended the option early and sent a truncated credential. LF, CR, TAB and VT are now escaped the way curl's quoted strings expect, and test-wait-storage.sh covers a secret containing CR, LF, backslash and a quote. - The docker/README Hubble step used the secret as a sed replacement, where & # and backslash are special. docker/set-hubble-pd-password.sh writes the value without sed, doubles backslashes for .properties, refuses an empty value or a line break, and keeps the file mode that the read-only mount depends on. test-compose.sh render exercises it with a non-hex secret. - Copy-paste deployment snippets in hugegraph-pd/README.md and hugegraph-store/docs/deployment-guide.md omitted the now-required HG_PD_AUTH_SECRET_KEY, so following them failed on the missing variable.
Four more review items.
Hubble had no path from .env to operations.pd.password: Compose does not
interpolate properties files and the Hubble image has no entrypoint that
reads the environment, so both HStore stacks mounted a tracked file with an
empty password and the documented remedy was to sed a production secret
into that tracked file. The tracked files are now
conf/hubble/{hstore,hstore-ha}.properties.example, the Compose files mount
the generated conf/hubble/<name>.local.properties, that pattern is in
.gitignore next to docker/.env, and set-hubble-pd-password.sh <name>
[secret] generates the file from the example. The .env recipe runs it, the
deployment guide runs it before `up`, and test-compose.sh generates both
files before every render and smoke (a developer's own local files are
restored afterwards) and checks the generator with a non-hex secret, the
resulting mode, and its refusals.
wait-storage.sh could not tell a 401 from an unreachable PD or a cluster
with no Up store: curl sat inside a grep pipeline with no pipefail, so a
wrong or missing secret retried for 300s and then reported a storage
failure. curl now runs alone with -w '\n%{http_code}', a 401 aborts at once
with a message naming PD_AUTH_PASSWORD and auth.secret-key, and the outer
error distinguishes a timeout from an abort. The test mock honours -w and a
new case asserts the abort happens on the first call.
PDConfig now logs the empty auth.secret-key at boot, not only on the first
refused request, since /v1/health keeps answering 200 in between and the
shipped configs leave the key empty on purpose.
The 401 body is the constant {"status":-1,"error":"Unauthorized"}. The old
body carried the exception's toString, which named internal classes and
told an unauthenticated caller whether the service name or the password was
wrong; the reason now goes to a debug log with the method and path.
PD REST now checks the password against auth.secret-key and answers 401 on refusal (apache#3188). The PD image requires HG_PD_AUTH_SECRET_KEY, wait-storage.sh sends PD_AUTH_PASSWORD, and Hubble reads operations.pd.password. Two conflicts with the pull requests merged before it, both resolved as the union: the interceptor exclusion list keeps /v1/ready from apache#3185 alongside the /actuator/** widening from apache#3189, and test-compose.sh runs the startup timeout asserts from apache#3187 followed by the Hubble helper check from apache#3189. render_with_timeout from apache#3187 additionally passes HG_PD_AUTH_SECRET_KEY, which the Compose files require since apache#3189; without it the render step would fail on the two HStore topologies. The chart does not yet supply the PD secret, so PD Pods from an image built at this revision will not start under the chart until that wiring lands.
PD images from 1.8.0 (apache#3189) check the Basic-auth password of every management call against auth.secret-key and refuse to start without one. The chart now keeps that value in a kept release-pd-auth Secret, or in pd.auth.existingSecret, and hands it to the three readers: PD as HG_PD_AUTH_SECRET_KEY, the Server storage wait as PD_AUTH_PASSWORD, and Hubble as operations.pd.password written into its properties file by the existing wrapper. A checksum/pd-auth annotation on the three Pod templates rolls them when the Secret changes; the Server annotations block is now rendered unconditionally for it. Priority and lookup semantics mirror server.auth.token. Older images ignore the password, so the wiring is harmless there. The values schema requires one of existingSecret, value or autoGenerate and refuses newlines, carriage returns and backslashes in an inline value, since it lands in a Java properties file; the template guard repeats the first rule for values that bypass the schema. The three chart-managed variables join the reserved extraEnv lists. README: Chart Details bullet, four parameter rows, Disaster Recovery calls carry the secret, the Limitations bullet separates the 1.7.0 behaviour from 1.8.0, and an upgrade note (PD, Server and Hubble roll once). NOTES prints how to read the secret. New suite pd_auth_secret_test.yaml, 9 tests; 58 in total. Lint on three presets, renders 19/17/20 objects.
PD images from 1.8.0 (apache#3189) check the Basic-auth password of every management call against auth.secret-key and refuse to start without one. The chart now keeps that value in a kept release-pd-auth Secret, or in pd.auth.existingSecret, and hands it to the three readers: PD as HG_PD_AUTH_SECRET_KEY, the Server storage wait as PD_AUTH_PASSWORD, and Hubble as operations.pd.password written into its properties file by the existing wrapper. A checksum/pd-auth annotation on the three Pod templates rolls them when the Secret changes; the Server annotations block is now rendered unconditionally for it. Priority and lookup semantics mirror server.auth.token. Older images ignore the password, so the wiring is harmless on the images the draft currently tracks. The values schema requires one of existingSecret, value or autoGenerate and refuses newlines, carriage returns and backslashes in an inline value, since it lands in a Java properties file; the template guard repeats the first rule for values that bypass the schema. The three chart-managed variables join the reserved extraEnv lists. README: Chart Details bullet, four parameter rows, Disaster Recovery calls carry the secret, and the Limitations bullet separates the 1.7.0 behaviour from 1.8.0. NOTES prints how to read the secret. New suite pd_auth_secret_test.yaml, 9 tests; 58 in total. Lint on three presets; renders 16 objects by default and 19 with Hubble. Measured on a kind cluster with images built from master plus apache#3185, apache#3187 and apache#3189: the Secret is created, PD starts with the variable, the Server storage wait passes with the credential, and Hubble lists all nine nodes.
|
Tested end to end on Kubernetes through the Helm chart on 2026-09-05. Build under test. Tag
Merge notes for whoever lands the second of #3185 and this PR: |
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: the credential gate holds up at this head. The password is compared constant-time against auth.secret-key, the Basic credential is decoded as UTF-8, an empty secret refuses every request instead of falling back to a name check, and refusals return 401 with a challenge header and a constant body. The four findings below are documentation and test-tooling leftovers from the last two fix commits.
Evidence, all at e964b68:
- Built
hg-pd-serviceon JDK 11;AuthenticationTest8/8. travis/test-wait-storage.sh7/7,travis/test-pd-docker-entrypoint.sh10/10 (also underLC_ALL=en_US.UTF-8andC.UTF-8),travis/test-pd-shipped-config.shanddocker/test-compose.sh renderall pass.git check-ignoreon the Hubble generator's temp path.- The only in-repo PD REST clients are
bin/wait-storage.shand Hubble'soperations.pd.*; both are wired here.
Four follow-ups on the previous commit. The .gitignore pattern missed the file the Hubble generator actually writes through: set-hubble-pd-password.sh writes to a mktemp name derived from the output, so the plaintext secret first lands in conf/hubble/hstore.local.properties.aB3xY9, which *.local.properties does not match. A trailing * covers the base name and every suffixed form, while the tracked examples and standalone.properties stay tracked. Both READMEs still told operators that a wrong secret waits out WAIT_STORAGE_TIMEOUT_S and exits with "Timeout waiting for storage backend". The previous commit made a 401 abort on the first attempt with a different message, so the documented string could not appear. They now describe the abort and name the two messages the script really prints. test-pd-shipped-config.sh printed "ok" for a file that had just failed, because the checks only set the global FAIL and the ok line ran unconditionally. It now compares FAIL against its value on entry, using an if rather than a && so the function does not return non-zero under set -e and abort the loop at the first bad file. Its exposure lookup also reads the include: under the actuator exposure block rather than the first include: in the file, so a config that grows an unrelated one cannot pass silently.
AuthenticationConfigurer excludes /actuator/** so that nested probe paths
such as /actuator/metrics/{name} stay reachable, but seven prose and
comment lines still named /actuator/*. The table row in
hugegraph-pd/docs/configuration.md was the misleading one: it claimed
every exposed actuator endpoint is reachable without a credential, which
only holds for the double-star pattern.
Correct all seven to /actuator/**, matching application.yml.template,
which already used it. Documentation only; no behaviour change.
Three comments added by this branch claimed that /actuator/* would make the REST auth interceptor refuse nested actuator paths, and that /actuator/** is what keeps them open. That is not how the request reaches them. Actuator endpoints are served by WebMvcEndpointHandlerMapping. Like every AbstractHandlerMapping it auto-detects MappedInterceptor beans only, and an interceptor registered through WebMvcConfigurer.addInterceptors goes into the MVC registry rather than the bean factory, so it is attached to the MVC handler mappings alone. The interceptor never runs on an actuator path with or without the exclusion. Checked against the versions hg-pd-service resolves, spring-boot-actuator 2.5.14 and spring-webmvc 5.3.20, and reproduced on Boot 2.5.14 with Jetty: excluding /actuator/* and excluding a pattern that matches nothing both left /actuator/metrics/jvm.memory.used at 200 with the interceptor logging only the /v1 request, while the same interceptor published as a MappedInterceptor bean did 401 the actuator paths. /actuator/** stays in the exclusion list. It states the intent for nested probe paths and costs nothing; only the rationale was wrong. The exposure allowlist is what bounds which endpoints exist there. Comments and prose only; no behaviour change and no test assertion change.
The three config comments above management.endpoints.web.exposure.include still said the /actuator/** exclusion is what makes those endpoints anonymous. It is not: actuator is served by its own handler mapping that the REST auth interceptor is never attached to, so the exclusion pattern records intent rather than causing the behaviour. Two of these files ship to users in the dist conf, so they are more likely to be read than the docs table that was corrected earlier. Also says "is never a bean" where the interceptor comment said "is not one", which could be read as claiming the registry does not build a MappedInterceptor. It does; it just never registers one as a bean, and bean detection is what the actuator mapping requires.
|
@imbajin Ready for a review pass when you have time, though please read the second section first: I got something wrong in this PR and the last three commits correct it. Correction, and the reason for
Checked empirically as well as by reading the sources at the versions
In the first two the interceptor logged only What actually keeps those paths anonymous is that actuator has its own handler mapping, and what bounds them is the exposure allowlist, which this PR narrows from
What else I checked on the branch. PD booted from the shipped On the previously published placeholder secret PD exits 1 as intended. With no One wording note on the credential path: Merge-order conflict with #3185, worth deciding before either lands. Both PRs rewrite the same line, excludePathPatterns("/actuator/**", "/v1/health", "/v1/ready", "/v1/prom/targets/*")Dropping One follow-up I would not hold the merge for. gRPC on 8686 stays unauthenticated. The TODO explains why it cannot be enabled yet, which I think is right for this PR, but it is worth a release-note line so "PD REST auth" is not read as "PD is authenticated". On CI. The |
imbajin
left a comment
imbajin
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: The REST credential wiring is covered by the current checks, but the CI startup wrapper leaks PD-only Spring properties into the subsequently sourced Store process, and the shipped-config regression check misses the cluster-test PD template. Evidence: exact-head CI is green; local PD entrypoint, wait-storage, Compose-render, Java compile, and AuthenticationTest checks pass; static call-chain review confirms both residual gaps.
| # conf/application.yml ships auth.secret-key empty on purpose, so PD would | ||
| # refuse every authenticated REST request. Supply a test-only secret; it must | ||
| # match the value the PD test suites send. | ||
| export SPRING_APPLICATION_JSON='{"auth":{"secret-key":"pd-ci-test-secret-not-for-production"}}' |
There was a problem hiding this comment.
install-hstore.sh:22-23 runs . start-pd.sh and then . start-store.sh in the same shell, so this exported SPRING_APPLICATION_JSON remains set for Store. It contains PD's grpc.port=8686, server.port=8620, and PD raft/data properties, while Store expects grpc.port=8500 and server.port=8520 in hugegraph-store/hg-store-node/src/main/resources/application.yml; Spring Boot consumes SPRING_APPLICATION_JSON for both apps. Please scope this JSON to the PD process or unset/restore it before start-pd.sh returns.
| } | ||
|
|
||
| echo "PD shipped configuration hardening" | ||
| for f in "${ROOT}"/hugegraph-pd/hg-pd-dist/src/assembly/static/conf/application.yml* \ |
There was a problem hiding this comment.
hugegraph-cluster-test/hugegraph-clustertest-dist/src/assembly/static/conf/pd-application.yml.template is copied to each PD node by PDNodeWrapper, but it still has management.endpoints.web.exposure.include: "*" and no auth.secret-key. With this PR's PDConfig, REST credentials fail closed while actuator endpoints such as /actuator/env remain anonymously exposed. Please update that shipped template to the same allowlist/empty secret contract and include it in this check.
2 participants
Purpose of the PR
PD's REST interceptor decodes the Basic credential, keeps only the part before the colon, and checks it against the fixed set
hg,store,hubble,vermeer. Any of those names with any password, including an empty one, is treated as an internal component; the password is never read. Separately,RestAuthentication.preHandlewrites an error body without callingsetStatus, so success, refusal and missing credential all return HTTP 200 and nothing keyed on a status code (monitors,curl -f, the shipped healthchecks) can see a refusal. The endpoints behind the interceptor mutate the cluster:POST /v1/members/change,DELETE /v1/store/{storeId}, graph and graphspace writes, and the balance and patrol tasks. The issue has the measured 27-request matrix.Main Changes
Authentication.authenticatereads the password and compares it (constant-time,MessageDigest.isEqual) withauth.secret-key, aPDConfigkey that already existed but was only referenced by dead code. The service-name check stays. The credential is decoded as UTF-8 per RFC 7617 rather than with the platform charset. An empty secret refuses every request instead of falling back to name-only authentication, and PD logs that condition at boot and again on the first refused request.application.ymlfiles and the template carryauth.secret-keyempty with a note saying why there is no safe default, and PD refuses to start if the key is set to the value earlier revisions of this repository carried as a placeholder. The Docker image requiresHG_PD_AUTH_SECRET_KEY(all JSON control characters escaped intoSPRING_APPLICATION_JSON, never logged), and both HStore Compose files fail fast without it; the.envrecipe generates one.RestAuthentication.preHandlereturns 401 with aWWW-Authenticate: Basicchallenge and a constant{"status":-1,"error":"Unauthorized"}body; the refusal reason goes to a debug log rather than to the unauthenticated caller. The interceptor exclusion is/actuator/**so nested probe paths such as/actuator/metrics/{name}stay reachable, and the actuator exposure in every shipped config is narrowed from"*"tohealth,metrics,prometheus.HG_PD_AUTH_SECRET_KEYto each PD andPD_AUTH_PASSWORDto each Server.wait-storage.shreads the credential from the environment and hands it to curl on stdin as an escaped config (never in the script text or argv), and a 401 from PD aborts at once with a message naming the two settings instead of retrying into a 300s storage timeout. Hubble reads a generated, untrackedconf/hubble/<name>.local.propertiesproduced from the tracked.properties.examplebydocker/set-hubble-pd-password.sh, so the secret never lands in a tracked file.AuthenticationTestinhg-pd-servicecovers the gate in process (8 cases, including UTF-8);RestApiTestasserts 401 for a missing, wrong or empty credential and an unknown name, that nested actuator paths answer without one, and that/actuator/envstays closed;test-wait-storage.shcovers the argv/stdin contract, a secret containing CR, LF, backslash and quote, and the 401 abort;test-pd-docker-entrypoint.shround-trips eight secrets through the generated JSON;test-pd-shipped-config.shchecks every shipped config variant for the same hardening;test-compose.sh renderexercises the Hubble generator with a non-hex secret. CI starts its PD with a test-only secret.hugegraph-pd/README.md,docs/configuration.md,docs/api-reference.md,docker/README.md, the Store deployment guide and thebalanceLeadersrunbook step in the operations guide document the credential, the required variable, the three consumers, and that port 8620 must stay on a trusted network.This intentionally refuses any client that sent a valid name with an arbitrary password, and requires an operator-provided secret, which is why it targets 1.8.0 (see the issue for the release-boundary reasoning). Remaining wiring lives outside this repo: Hubble's own PD calls on the toolchain side, the Helm chart's Secret in #3132, and the same note for the website docs. Probes are untouched; #3185 keeps
/v1/readyoff the authenticated surface for exactly that reason. Unrelated but noticed on the way:hugegraph-struct/.../AuthOptions.javauses the same published string as the Server's defaultauth.token_secret, which deserves its own issue.Verifying these changes
/v1/membersand/v1/storesreturn 401 with a constant body for no header, empty, wrong and unknown-name credentials and 200 only for a valid one, the 401 carries the challenge header,/v1/health,/actuator/health,/actuator/metrics/jvm.memory.usedand/actuator/prometheusanswer without a credential, and/actuator/envand/actuator/beansdo not serve data. A non-ASCII secret authenticates under-Dfile.encoding=US-ASCII.PDRestSuiteTest19/19,PDClientSuiteTest45/45,AuthenticationTest8/8,test-wait-storage.sh7/7,test-pd-docker-entrypoint.sh10/10,test-pd-shipped-config.sh,docker/test-compose.sh render, andmvn editorconfig:checkon the touched PD modules pass locally;mvn install -Dmaven.test.skip=trueover the PD modules (the path the commons job takes) compiles clean.Does this PR potentially affect the following parts?
Documentation Status
Doc - TODODoc - DoneDoc - No Need