bitflicker64 commented on code in PR #3189:
URL: https://github.com/apache/hugegraph/pull/3189#discussion_r3930914959
##########
docker/test-compose.sh:
##########
@@ -21,6 +21,7 @@ set -Eeuo pipefail
DOCKER_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
PASSWORD="ci-compose-password"
SECRET="0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"
+PD_SECRET="ci-compose-pd-secret"
Review Comment:
Fixed in 3b39132, and the break was mine: I required the variable in the
Compose files without giving it to every caller of `docker compose`.
`compose_active` now passes `HG_PD_AUTH_SECRET_KEY="${PD_SECRET}"` alongside
the other three, so the smoke path interpolates the same way render does. Kept
the two functions separate rather than routing smoke through `compose_auth`,
since `compose_active` carries the project and file selection that smoke needs.
##########
hugegraph-pd/hg-pd-dist/docker/docker-entrypoint.sh:
##########
@@ -57,8 +57,16 @@ require_env "HG_PD_INITIAL_STORE_LIST"
: "${HG_PD_DATA_PATH:=/hugegraph-pd/pd_data}"
: "${HG_PD_INITIAL_STORE_COUNT:=1}"
+# Optional secret for REST Basic authentication (auth.secret-key). When unset,
+# 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}")\" },"
Review Comment:
Fixed in 3b39132. `json_escape` now escapes every C0 control character as
`\uXXXX` rather than only backslash, quote and LF, so CR and TAB no longer
produce invalid JSON.
Added `travis/test-pd-docker-entrypoint.sh`, wired into the `pd` CI job, as
you asked. It drives the real entrypoint with a stub launcher and asserts the
generated `SPRING_APPLICATION_JSON` parses and round-trips the secret for eight
values: plain, CR, TAB, double quote, backslash, backslash-and-quote, non-ASCII
and spaces. It also covers the missing-secret refusal and that the secret never
reaches the log. Verified it catches the old behaviour: the previous escaper
produced `Invalid control character` on the CR case.
##########
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:
Fixed in 3b39132, taking the suggestion. Line 89 is now `new String(decode,
StandardCharsets.UTF_8)`.
Confirmed both directions in isolation before and after: the old decode
rejects `hg:sécrèt-2026` under `-Dfile.encoding=US-ASCII` and accepts it under
UTF-8, the new one accepts it under both. Also end to end on the packaged dist
with the secret in `conf/application.yml` and PD started with `LANG=C LC_ALL=C
-Dfile.encoding=US-ASCII`: the non-ASCII secret now gets 200 and a wrong one
still gets 401.
One nearby case this does not cover, since it is a different path: passing a
non-ASCII secret through `SPRING_APPLICATION_JSON` still depends on how the JVM
decodes the environment (`sun.jnu.encoding`), so it can be mangled before
Spring parses it. The Docker recipe generates a hex secret, so it does not bite
there.
##########
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:
Fixed in 3b39132. You are right that nothing put the value in the operator's
shell, so both the `curl` on line 78 and this `sed` were silently no-ops.
The page now loads `.env` where it previously only set `ADMIN_PASSWORD`,
with a line saying why (Compose reads `.env` itself; this is for the `curl` and
`sed` on the page). The `sed` block takes your guard, refusing to write an
empty value, and the surrounding text names `hstore-ha.properties` for the HA
topology next to `hstore.properties`.
##########
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:
Fixed in 3b39132, taking the suggestion. The secret is generated into an
exported variable first and then passed with `-e
HG_PD_AUTH_SECRET_KEY="${HG_PD_AUTH_SECRET_KEY}"`, with a comment saying every
REST client needs the same value and that re-minting one breaks the clients
already using the old one, so it should be stored somewhere durable rather than
only in that shell.
--
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]
