bitflicker64 commented on code in PR #3189:
URL: https://github.com/apache/hugegraph/pull/3189#discussion_r3964898180
##########
hugegraph-store/docs/operations-guide.md:
##########
@@ -2,6 +2,21 @@
This guide covers monitoring, troubleshooting, backup & recovery, and
operational procedures for HugeGraph Store in production.
+> **PD REST credential.** Calls to a PD REST endpoint on port 8620, other than
+> `/v1/health`, `/actuator/**` and `/v1/prom/targets/*`, need HTTP Basic auth:
Review Comment:
🧹 `integration-guide.md` is the one Store guide that got neither this banner
nor the `-u` flag, and its PD examples now return 401.
This PR covers the sibling docs: `deployment-guide.md` gained `-u
hg:"${PD_SECRET}"` on its PD calls, `hugegraph-store/README.md:265` gained it
with an explanatory comment, and this file gained the banner above plus `-u` on
the add-a-Store runbook. `hugegraph-store/docs/integration-guide.md` is
untouched and still has, in its troubleshooting sections:
```
709: curl http://192.168.1.10:8620/v1/partitions
736: curl http://192.168.1.10:8620/v1/partitions | grep leader
739: curl http://192.168.1.10:8620/v1/stores
```
All three are outside `excludePathPatterns`, so after this change they
return `{"status":-1,"error":"Unauthorized"}`. There is no `-f`, so curl exits
0 and line 736's `grep leader` simply matches nothing — the diagnostic for
"Raft leader not found" quietly answers as if no partition has a leader. Line
680's `/v1/health` in the same file is fine and stays unauthenticated.
Requested change: add `-u hg:"${PD_SECRET}"` to those three commands and put
the same banner at the top of `integration-guide.md`, or at minimum add the
one-line note this file's banner already carries ("Some PD examples in this
guide still omit the credential; add `-u hg:"${PD_SECRET}"` when a call returns
401") to that file too, so a reader is not left interpreting a 401 body as a
cluster symptom.
##########
hugegraph-pd/docs/configuration.md:
##########
@@ -79,6 +79,41 @@ server:
- Metrics: `http://<host>:8620/actuator/metrics`
- Prometheus: `http://<host>:8620/actuator/prometheus`
+### REST Authentication Settings
+
+Every REST request except the probes below must carry HTTP Basic auth: one of
+the internal service names (`hg`, `store`, `hubble`, `vermeer`) as the user,
+and the shared secret as the password. A missing or wrong credential gets
+HTTP 401. Unauthenticated paths: `/v1/health`, `/actuator/**` and
+`/v1/prom/targets/*`.
Review Comment:
🧹 The list of unauthenticated paths is missing `/v1/ready`, in all four
places this PR spells it out.
`AuthenticationConfigurer.addInterceptors` excludes four patterns at this
head, unchanged from the merge base in this respect:
```java
.excludePathPatterns("/actuator/**", "/v1/health", "/v1/ready",
"/v1/prom/targets/*");
```
This line, `hugegraph-pd/docs/api-reference.md:770`,
`hugegraph-pd/README.md:301` and `hugegraph-store/docs/operations-guide.md:6`
all name only `/v1/health`, `/actuator/**` and `/v1/prom/targets/*`.
`/v1/ready` answers without a credential and returns raft state (`ready`,
`isLeader`, `state` — see `StoreAPI.checkReady` and `StoreAPIReadyTest`), so it
belongs in an inventory operators read to decide what to firewall and which
probe paths work. The same PR contradicts itself on it: `api-reference.md:803`
shows `curl -i http://localhost:8620/v1/ready` and `docker/README.md:284`
recommends `curl -fsS http://localhost:8620/v1/ready | grep -q '"ready":true'`,
both without `-u`.
Requested change: add `/v1/ready` to the unauthenticated-path list in all
four passages so the documented anonymous surface matches `excludePathPatterns`
exactly.
##########
hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/config/PDConfig.java:
##########
@@ -85,6 +100,25 @@ public class PDConfig {
private ConfigService configService;
private IdService idService;
+ @Override
+ public void afterPropertiesSet() {
+ if (PUBLISHED_SECRET_KEY.equals(this.secretKey)) {
+ throw new IllegalStateException(
+ "auth.secret-key is set to the value published in the
HugeGraph source " +
+ "tree, which authenticates anyone who can read it. Set a "
+
+ "deployment-specific secret in conf/application.yml, or
through the " +
+ "HG_PD_AUTH_SECRET_KEY environment variable for the Docker
image.");
+ }
+ // The shipped configs leave this empty on purpose. Say so in the boot
log:
+ // the REST interceptor also logs it, but only on the first refused
request,
+ // and /v1/health keeps answering 200 in the meantime.
+ if (this.secretKey == null || this.secretKey.isEmpty()) {
Review Comment:
⚠️ The wildcard-actuator guard exists only in the Docker image, so the
in-place upgrade this PR documents as leaving actuator anonymous has nothing
that says so at boot.
`hugegraph-pd/hg-pd-dist/docker/docker-entrypoint.sh:91-95` refuses
`HG_PD_ACTUATOR_EXPOSURE` containing `*` with `exit 2`, and
`travis/test-pd-docker-entrypoint.sh` pins it (I ran it: 15 passed, including
"wildcard exposure '*' is refused" and "wildcard exposure 'health,*' is
refused"). The tarball path has no equivalent. `bin/start-hugegraph-pd.sh:178`
passes `-Dspring.config.location=${CONF}/application.yml`, which *replaces* the
default config locations rather than adding to them, so only that one file is
read — an operator who keeps their existing `conf/application.yml` across an
upgrade keeps `management.endpoints.web.exposure.include: "*"` and PD starts
with no complaint.
This PR already names that case, in `hugegraph-pd/README.md:313-320`: "An
existing `conf/application.yml` carried over from an earlier release has no
`auth` block, and still carries `management.endpoints.web.exposure.include:
\"*\"` … while `/actuator/env`, `/actuator/configprops` and `/actuator/beans`
stay anonymously readable on `8620`." `"*"` exposes every default-enabled
endpoint, so `/actuator/heapdump` is in that set too, and a heap dump carries
the REST secret regardless of the property sanitizer that redacts it on
`/actuator/env`. The empty-secret branch you added right here is the boot-time
signal for the other half of the same misconfiguration; the exposure half gets
nothing.
Requested change: make this method symmetric with the entrypoint. Add
`@Value("${management.endpoints.web.exposure.include:}")` to `PDConfig` and, in
`afterPropertiesSet`, log an ERROR when the value contains `*` — naming
`management.endpoints.web.exposure.include`, the endpoints it opens on 8620,
and the `health,metrics,prometheus` allowlist the shipped configs now use.
Throwing instead would match `docker-entrypoint.sh` exactly, but that turns an
in-place upgrade into a hard stop, so an ERROR next to the existing
`auth.secret-key` one is probably the right level here.
##########
hugegraph-pd/hg-pd-service/src/main/java/org/apache/hugegraph/pd/rest/interceptor/RestAuthentication.java:
##########
@@ -59,9 +59,18 @@ public boolean preHandle(HttpServletRequest request,
HttpServletResponse respons
authority = authority.replace("Basic ", "");
Review Comment:
🧹 The `Basic` scheme is matched case-sensitively, and with a global
substring replace rather than a prefix strip.
RFC 7235 §2.1 makes the auth-scheme token case-insensitive, so
`Authorization: basic <base64>` is a conforming request. Here `replace("Basic
", "")` leaves it untouched, `Base64.getDecoder().decode` then throws on the
space, `Authentication.authenticate` wraps it in a `RuntimeException`, and the
caller gets 401 with the constant body — the outcome is fail-closed, so this is
a conformance and diagnosability gap rather than a bypass. Worth fixing here
because this is the entry point of the gate the PR is hardening and the comment
you added two lines below cites RFC 7617 for exactly this kind of reason.
`String.replace` is also a whole-string replace, not a prefix strip; the Base64
alphabet excludes the space so no payload can currently be mangled by it, but
the code reads as if it strips a prefix and does not.
Requested change: strip the scheme as a case-insensitive prefix, and refuse
anything else, roughly:
```java
if (authority.regionMatches(true, 0, "Basic ", 0, 6)) {
authority = authority.substring(6);
} else {
throw new BadCredentialsException("unsupported authentication scheme");
}
```
A case for a lowercase scheme in `AuthenticationTest` would pin it,
alongside the malformed-credential cases already there.
--
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]
