bitflicker64 commented on code in PR #3189:
URL: https://github.com/apache/hugegraph/pull/3189#discussion_r3924725808
##########
hugegraph-pd/hg-pd-service/src/main/java/org/apache/hugegraph/pd/service/interceptor/Authentication.java:
##########
@@ -77,19 +84,33 @@ protected <T> T authenticate(String authority, String
token, Function<String, T>
}
String name = info.substring(0, delim);
- // 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();
- } else {
+ String pwd = info.substring(delim + 1);
+ if (!innerModules.contains(name)) {
throw new AccessDeniedException("invalid service name");
}
+ if (!verifySecret(pwd)) {
+ throw new BadCredentialsException("invalid credential");
+ }
+ return call.get();
} catch (Exception e) {
throw new RuntimeException(e);
}
}
+ /**
+ * Compare the password of the Basic credential with the shared secret
+ * configured via `auth.secret-key`. A missing or empty secret refuses
every
+ * request instead of falling back to name-only authentication.
Review Comment:
Fixed in 1e3b616. Confirmed your resolution independently against
spring-core 5.3.20: `resolved=[ 'FXQXbJtbCLxODc6tGci732pkH1cyf8Qg']`, 35 chars.
`PDConfig.java:72` is now `@Value("${auth.secret-key:}")`, so an absent key
yields `""` and this contract holds, plus a one-shot ERROR naming the parameter
and the fix. Verified on the packaged dist with the `auth` block stripped from
`conf/application.yml`: the shipped secret gets 401, the old quoted-literal
string gets 401, and the log line appears. The upgrade note is in the PD README
security section. `pd.initial-store-list` at line 57 left alone for a separate
pass, as you suggested.
##########
hugegraph-pd/README.md:
##########
@@ -280,6 +281,19 @@ docker/docker-compose-3pd-3store-3server.yml
- Ensure low latency (<5ms) between PD nodes for Raft consensus
- Open required ports: `8620` (REST), `8686` (gRPC), `8610` (Raft)
+### Security
+
+- Keep all three ports on a trusted network. The REST API on `8620` includes
+ management endpoints that mutate the cluster (peer changes, store removal,
+ data movement), and the gRPC and Raft ports carry no authentication.
+- REST requests need HTTP Basic auth: one of the internal service names
+ (`hg`, `store`, `hubble`, `vermeer`) with the `auth.secret-key` value as
+ the password. Health probes (`/v1/health`, `/actuator/*`,
+ `/v1/prom/targets/*`) stay unauthenticated.
Review Comment:
Fixed in 1e3b616. Narrowed to `health,metrics,prometheus` in both
`application.yml` files and updated the matching row and example in
`docs/configuration.md`, which documented `"*"`. Verified on the running dist:
`/actuator/health`, `/metrics` and `/prometheus` still 200, while
`/actuator/env`, `/beans` and `/configprops` no longer serve data. They return
401 rather than 404, because the unmapped dispatch re-enters the interceptor,
which seems the better of the two.
##########
docker/README.md:
##########
@@ -66,6 +66,22 @@ For the verification commands below, set the password in
your current shell:
ADMIN_PASSWORD='the-same-password-used-in-.env'
```
+The PD REST API (port 8620, HStore topologies only) has its own credential:
+requests other than health probes need HTTP Basic auth with an internal
+service name (for example `hg`) and the PD secret as the password. PD ships
+with a default secret in `conf/application.yml` (`auth.secret-key`), and the
+Hubble files under `conf/hubble/` carry the matching `operations.pd.password`.
+With the shipped default, list registered stores like this:
+
+```bash
+curl -u hg:FXQXbJtbCLxODc6tGci732pkH1cyf8Qg http://localhost:8620/v1/stores
+```
+
+The default secret is public (it is in the source tree), so it only keeps
+casual traffic out. On any shared network, change it: set
+`HG_PD_AUTH_SECRET_KEY` on the PD services and put the same value in the
+Hubble properties files, or do not publish port 8620 at all.
Review Comment:
Fixed in 1e3b616, with one correction to the suggestion.
`HG_PD_AUTH_SECRET_KEY` cannot go in the `x-pd-common` anchor: each pd service
defines its own `environment:` block, and a service-level key replaces the
merged mapping wholesale instead of deep-merging, so it would be silently
dropped. It is set per service instead, on pd0/pd1/pd2 and the single `pd` in
the hstore file, with `PD_AUTH_PASSWORD` on the server anchor and the hstore
server. `docker compose config` with `HG_PD_AUTH_SECRET_KEY=rotated-secret-xyz`
renders that value at every consumer. The paragraph now names all three
consumers, the timeout symptom, and that Hubble's mounted file needs a manual
edit.
##########
hugegraph-server/hugegraph-dist/src/assembly/static/bin/wait-storage.sh:
##########
@@ -39,7 +39,9 @@ log() {
echo "[wait-storage] $1"
}
-PD_AUTH_ARGS="-u ${PD_AUTH_USER:-store}:${PD_AUTH_PASSWORD:-admin}"
+# PD validates the password against its auth.secret-key; the default below
+# matches PD's shipped default. Override both when the PD secret is changed.
+PD_AUTH_ARGS="-u
${PD_AUTH_USER:-store}:${PD_AUTH_PASSWORD:-FXQXbJtbCLxODc6tGci732pkH1cyf8Qg}"
Review Comment:
Agreed on both counts, and not fixed in 1e3b616. Keeping it out of the inner
script text and out of argv is a behaviour change to the startup path that I
would rather not fold into this PR's verification surface. Happy to do it here
if you prefer, otherwise it goes in the follow-up with the remaining docs
sweep. Flagging that the rotation path this PR documents now makes an arbitrary
secret reachable, so it should not sit for long.
--
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]
