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 3 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% 36.98% -0.81%
+ Complexity 6556 6355 -201
============================================
Files 800 785 -15
Lines 68929 66873 -2056
Branches 9157 8898 -259
============================================
- Hits 26046 24731 -1315
+ Misses 39824 39202 -622
+ Partials 3059 2940 -119 ☔ 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.
| # the value from conf/application.yml applies. Never logged. | ||
| AUTH_JSON="" | ||
| if [[ -n "${HG_PD_AUTH_SECRET_KEY:-}" ]]; then | ||
| AUTH_JSON="\"auth\": { \"secret-key\": \"$(json_escape "${HG_PD_AUTH_SECRET_KEY}")\" }," |
There was a problem hiding this comment.
json_escape, but that helper only escapes backslashes and quotes and removes LF; it leaves other JSON control characters such as CR and tab unescaped. Reproduction: on this exact head, setting HG_PD_AUTH_SECRET_KEY to a value containing a\rb produces an invalid SPRING_APPLICATION_JSON, so the PD container fails before startup. Please escape all JSON control characters or avoid hand-building this JSON, and add a test for the override.
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.
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.authenticatenow reads the password and compares it (constant-time,MessageDigest.isEqual) with the shared secret fromauth.secret-key, aPDConfigkey that already existed but was only referenced by dead code. The service-name check stays. A missing or empty secret refuses every request instead of falling back to name-only authentication.RestAuthentication.preHandlesets 401 for a missing, malformed or refused credential. The JSON error body is unchanged.auth.secret-keynow appears in both shippedapplication.ymlfiles with a change-in-production note, and the PD Docker image accepts an optionalHG_PD_AUTH_SECRET_KEYenv, written intoSPRING_APPLICATION_JSONand never logged.wait-storage.shdefaults its PD password to the shipped secret (its oldadmindefault only passed because the password was ignored), and the Compose Hubble properties files setoperations.pd.usernameandoperations.pd.password.store:123/store:adminto the shipped secret, andRestApiTestgains four cases asserting 401 for a missing credential, a wrong password, an empty password and an unknown service name.hugegraph-pdanddockerREADMEs document the credential and state that the shipped default secret is public, so operators must change it or keep port 8620 off shared networks.This intentionally refuses any client that sent a valid name with an arbitrary password, 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.Verifying these changes
RestApiTest(run bypd-rest-testin CI against a live PD) now asserts 401 for a missing credential, a wrong password, an empty password and an unknown service name, and keeps asserting 200 with a valid credential on the existing endpoints./v1/membersand/v1/storesreturn 401 for no header,hg:with an empty password,hg:wrongpasswordandnobody:<secret>, and 200 only forhg:<secret>andstore:<secret>;/v1/healthstays 200 without a credential.PDRestSuiteTest17/17,PDClientSuiteTest45/45,test-wait-storage.sh5/5,docker/test-compose.sh renderandmvn editorconfig:checkon the touched PD modules all pass locally.Does this PR potentially affect the following parts?
Documentation Status
Doc - TODODoc - DoneDoc - No Need