bitflicker64 commented on code in PR #3189:
URL: https://github.com/apache/hugegraph/pull/3189#discussion_r3930889066
##########
docker/README.md:
##########
@@ -66,6 +67,37 @@ 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
+no default secret, so `HG_PD_AUTH_SECRET_KEY` is required and the HStore
+Compose files refuse to start without it. The `.env` command above generates
+one. To list registered stores:
+
+```bash
+curl -u "hg:${HG_PD_AUTH_SECRET_KEY}" http://localhost:8620/v1/stores
+```
+
+Three consumers read this credential, and all three have to agree or startup
+fails:
+
+- PD itself, through `HG_PD_AUTH_SECRET_KEY`.
+- The Server, whose `bin/wait-storage.sh` polls `/v1/stores` before the
+ Server starts. Both Compose files pass `PD_AUTH_PASSWORD` to it from the
+ same variable, so setting `HG_PD_AUTH_SECRET_KEY` in `.env` covers it. If
+ the Server sends the wrong secret it retries until
+ `WAIT_STORAGE_TIMEOUT_S` (300s) expires and the container exits with
+ `ERROR: Timeout waiting for storage backend`.
+- Hubble, through `operations.pd.password` in the file under `conf/hubble/`.
+ That file is mounted read-only and is not templated, so write the same value
+ into it by hand. Until you do, Hubble's PD-backed views get 401 from PD;
+ everything else in Hubble works.
+
+```bash
+sed -i.bak
"s#^operations.pd.password=.*#operations.pd.password=${HG_PD_AUTH_SECRET_KEY}#"
\
Review Comment:
⚠️ This command reports success and changes nothing: `HG_PD_AUTH_SECRET_KEY`
is never in the operator's shell at this point.
The `.env` recipe at lines 32-45 runs inside `( ... )`, so the generated
value reaches the file and nothing else. Line 64 does the equivalent step for
the admin password ("set the password in your current shell"); there is no
counterpart for the PD secret. Following the page top to bottom:
- Line 78 expands to `curl -u hg: http://localhost:8620/v1/stores`, i.e. the
empty password, and returns the 401 this section exists to prevent.
- This `sed` expands to
`s#^operations.pd.password=.*#operations.pd.password=#`, rewriting the shipped
empty value to itself and exiting 0. Hubble keeps getting 401 with a
`hstore.properties.bak` sitting next to the file as evidence that something ran.
It also covers only `conf/hubble/hstore.properties`.
`docker-compose-3pd-3store-3server.yml:244` mounts
`conf/hubble/hstore-ha.properties`, which got the same empty
`operations.pd.password` in this PR, so an HA operator who runs this command is
still left with a broken Hubble.
Requested change: load `.env` first, refuse to write an empty value, and
name both files.
```bash
set -a; . ./.env; set +a
if [ -n "${HG_PD_AUTH_SECRET_KEY:-}" ]; then
sed -i.bak
"s#^operations.pd.password=.*#operations.pd.password=${HG_PD_AUTH_SECRET_KEY}#"
\
conf/hubble/hstore.properties # hstore-ha.properties for the HA
topology
else
echo 'HG_PD_AUTH_SECRET_KEY is empty; check .env' >&2
fi
```
##########
hugegraph-pd/hg-pd-service/src/main/java/org/apache/hugegraph/pd/service/interceptor/Authentication.java:
##########
@@ -77,19 +90,39 @@ 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);
Review Comment:
⚠️ The password is decoded with the JVM default charset but compared as
UTF-8, so a non-ASCII secret authenticates or not depending on the host locale.
Line 86 is `String info = new String(decode);`, no charset, so it uses
`Charset.defaultCharset()`. Until this line the only thing read out of that
string was `name`, and the four service names are ASCII, so the charset never
mattered. It does now: line 122 compares `pwd.getBytes(UTF_8)` against
`secret.getBytes(UTF_8)`, and the secret side comes from the YAML config, which
Spring reads as UTF-8. The two agree only when the default charset is UTF-8.
PD targets JDK 11, where the default charset still follows the locale (JEP
400 lands in 18), and `bin/start-hugegraph-pd.sh:171-196` sets no
`-Dfile.encoding`. The tarball path this PR documents is where that bites: a PD
started from systemd or a shell with `LANG` unset gets US-ASCII and rejects the
secret that worked on the operator's own terminal, with `invalid credential` as
the only clue. Reproduced by feeding lines 85-93 a base64 `hg:sécrèt-2026`:
accepted under `-Dfile.encoding=UTF-8`, rejected under
`-Dfile.encoding=US-ASCII` and under `LANG=C LC_ALL=C`. False reject rather
than a bypass, since the secret is not put through the same conversion, but it
is silent and environment-dependent.
Requested change: decode as UTF-8 on line 86, which is what RFC 7617
specifies for Basic credentials. `StandardCharsets` is already imported at line
20.
```java
// line 86
String info = new String(decode, StandardCharsets.UTF_8);
```
##########
hugegraph-pd/README.md:
##########
@@ -241,6 +242,7 @@ docker run -d \
-p 8620:8620 \
-p 8686:8686 \
-p 8610:8610 \
+ -e HG_PD_AUTH_SECRET_KEY="$(openssl rand -hex 24)" \
Review Comment:
⚠️ This recipe generates a secret the operator never sees, and the same
README tells them to give it to every REST client.
`openssl rand -hex 24` runs in a command substitution inside the `docker
run` line, so the value exists only in the container's environment. The
Security section added a few lines below (line 285 onward) then asks the reader
to configure `PD_AUTH_PASSWORD` for `bin/wait-storage.sh` and
`operations.pd.password` for Hubble with the same value, which they cannot do
from this recipe without reaching for `docker inspect`. Re-running it after a
`docker rm` mints a different secret and silently 401s every client that was
working.
The `docker/README.md` `.env` recipe gets this right by persisting the value
to a file first. This is the only `docker run` example for PD, so it is where a
single-node operator starts.
Requested change: generate once, keep it, then pass it.
```bash
# Generate once and keep it; every PD REST client needs this same value
export HG_PD_AUTH_SECRET_KEY="$(openssl rand -hex 24)"
docker run -d \
-p 8620:8620 \
-p 8686:8686 \
-p 8610:8610 \
-e HG_PD_AUTH_SECRET_KEY="${HG_PD_AUTH_SECRET_KEY}" \
...
```
--
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]
