bitflicker64 commented on code in PR #3189:
URL: https://github.com/apache/hugegraph/pull/3189#discussion_r3947215076
##########
hugegraph-pd/hg-pd-dist/docker/docker-entrypoint.sh:
##########
@@ -51,14 +72,22 @@ require_env "HG_PD_GRPC_HOST"
require_env "HG_PD_RAFT_ADDRESS"
require_env "HG_PD_RAFT_PEERS_LIST"
require_env "HG_PD_INITIAL_STORE_LIST"
+# The REST API refuses every authenticated request without this, and the image
+# ships no default because a published secret is not a secret.
+require_env "HG_PD_AUTH_SECRET_KEY"
: "${HG_PD_GRPC_PORT:=8686}"
: "${HG_PD_REST_PORT:=8620}"
: "${HG_PD_DATA_PATH:=/hugegraph-pd/pd_data}"
: "${HG_PD_INITIAL_STORE_COUNT:=1}"
+# Secret for REST Basic authentication (auth.secret-key). Required above and
+# never logged.
+AUTH_JSON="\"auth\": { \"secret-key\": \"$(json_escape
"${HG_PD_AUTH_SECRET_KEY}")\" },"
Review Comment:
⚠️ Putting the secret in `SPRING_APPLICATION_JSON` makes it recoverable from
`/actuator/env` whenever that endpoint is exposed, because Spring Boot's env
sanitizer keys off the property *name*.
`hg-pd-service/pom.xml:92` pins Spring Boot 2.5.14, whose default
`Sanitizer` key list is `password`, `secret`, `key`, `token`,
`.*credentials.*`, `vcap_services`, `sun.java.command`, each compiled as
`.*<key>$` case-insensitively. `auth.secret-key` matches (`...key$`) and is
redacted, but the `systemEnvironment` entry `SPRING_APPLICATION_JSON` matches
none of them and is returned verbatim - secret included. The image's own
`conf/application.yml` now narrows the exposure, so the shipped path is fine;
an operator who bind-mounts a pre-1.8 `conf/application.yml` (the case
`hugegraph-pd/README.md` explicitly anticipates) gets `include: "*"` back and
hands out the secret anonymously on the port this PR is hardening.
Requested change: emit the exposure allowlist in the same JSON document, so
it outranks any mounted config file and the image is safe regardless of what is
mounted:
```bash
MANAGEMENT_JSON="\"management\": { \"endpoints\": { \"web\": { \"exposure\":
{ \"include\": \"health,metrics,prometheus\" } } } },"
```
and add it next to `${AUTH_JSON}` in the heredoc.
##########
hugegraph-pd/README.md:
##########
@@ -280,6 +290,30 @@ 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.
+- `auth.secret-key` has no shipped default, because a secret in the source
+ tree is published to everyone. Generate one per deployment (`openssl rand
+ -hex 24`) and set it in the config file, or through
+ `HG_PD_AUTH_SECRET_KEY`, which the Docker image requires. Give every REST
+ client the same value: the Server's `bin/wait-storage.sh` reads
+ `PD_AUTH_PASSWORD` (and `PD_AUTH_USER`, default `store`), and Hubble reads
+ `operations.pd.password`. A client left on a stale secret gets 401, and
+ `wait-storage.sh` aborts the Server's startup on the first one rather than
+ waiting out `WAIT_STORAGE_TIMEOUT_S`.
+- An existing `conf/application.yml` carried over from an earlier release has
Review Comment:
⚠️ This upgrade bullet covers `auth.secret-key` but not the actuator
allowlist, which leaves the upgraded deployment with exactly the anonymous
surface this PR identifies as the real boundary.
A `conf/application.yml` carried over from an earlier release does not only
lack the `auth` block - it also still has
`management.endpoints.web.exposure.include: "*"`. `AuthenticationConfigurer`
and `docs/configuration.md` both state (correctly) that the interceptor is
never attached to actuator's handler mapping, so the allowlist is the only
thing bounding what is reachable without a credential on 8620. Following this
bullet as written produces a PD that authenticates `/v1/**` while
`/actuator/env`, `/actuator/configprops` and `/actuator/beans` stay open.
Requested change: name the second setting in this bullet too, e.g. "Add
`auth.secret-key` **and** replace `management.endpoints.web.exposure.include:
"*"` with `health,metrics,prometheus` before upgrading."
##########
docker/docker-compose-hstore.yml:
##########
@@ -117,7 +120,7 @@ services:
- "${HUBBLE_PUBLISH_HOST:-127.0.0.1}:8088:8088"
volumes:
- hubble-data:/hubble/data
- -
./conf/hubble/hstore.properties:/hubble/conf/hugegraph-hubble.properties:ro
+ -
./conf/hubble/hstore.local.properties:/hubble/conf/hugegraph-hubble.properties:ro
Review Comment:
⚠️ This mount source is untracked and gitignored, so a missing file degrades
silently instead of failing like the sibling requirement does.
`HG_PD_AUTH_SECRET_KEY` is guarded with `${...:?}` and stops the stack
immediately when unset - the right shape. The other half of the same
requirement, running `set-hubble-pd-password.sh` first, has no guard:
`./conf/hubble/hstore.local.properties` is produced by that script and ignored
by `.gitignore`, so on a first `docker compose up` Docker creates an empty
**directory** at that path, mounts it over
`/hubble/conf/hugegraph-hubble.properties`, and Hubble boots unconfigured while
a stray directory is left in the working tree. `docker/README.md` warns about
it, but nothing enforces it, and `test-compose.sh` always calls
`prepare_hubble_configs` before `render`/`smoke`, so CI cannot catch the
missing-file case.
Requested change: switch both Hubble mounts (here and
`docker-compose-3pd-3store-3server.yml:244`) to the long syntax with
`create_host_path` disabled, so Compose refuses to start rather than inventing
the path:
```yaml
- type: bind
source: ./conf/hubble/hstore.local.properties
target: /hubble/conf/hugegraph-hubble.properties
read_only: true
bind:
create_host_path: false
```
##########
hugegraph-server/hugegraph-dist/src/assembly/static/bin/wait-storage.sh:
##########
@@ -114,12 +150,21 @@ if env | grep '^hugegraph\.' > /dev/null; then
}
until PD_REST=\$(check_any_pd_stores); do
+ if [ \$? -eq 2 ]; then exit 2; fi
Review Comment:
🧹 The 401 fast-abort depends on this being the very first statement in the
loop body, and nothing records that.
The construct is correct today - `bash -c 'f(){ return 2; }; until X=$(f);
do echo "status=$?"; break; done'` prints `status=2`, so `$?` here is
`check_any_pd_stores`'s return. But it is positional: inserting any command
before it (a `log`, a retry counter) overwrites `$?` and silently turns the
credential abort back into a 300s retry loop. The regression would surface in
`test-wait-storage.sh` only as `ERROR: Timeout waiting for storage backend`,
i.e. as the exact misleading symptom this change set out to remove.
Requested change: capture the status as the opening statement and branch on
the copy:
```bash
until PD_REST=\$(check_any_pd_stores); do
rc=\$?
if [ \"\$rc\" -eq 2 ]; then exit 2; fi
```
##########
hugegraph-server/hugegraph-dist/src/assembly/static/bin/wait-storage.sh:
##########
@@ -39,7 +39,33 @@ log() {
echo "[wait-storage] $1"
}
-PD_AUTH_ARGS="-u ${PD_AUTH_USER:-store}:${PD_AUTH_PASSWORD:-admin}"
+# PD REST credential. PD checks the password against its auth.secret-key and
+# ships no default, so this has to be provided by the deployment.
+# The value is deliberately kept out of the inner script's source text and out
+# of curl's argv: the inner shell reads it from the environment and hands it to
+# curl on stdin as a config file.
+PD_AUTH_USER="${PD_AUTH_USER:-store}"
+PD_AUTH_PASSWORD="${PD_AUTH_PASSWORD:-}"
+if [ -z "${PD_AUTH_PASSWORD}" ]; then
+ log "WARN: PD_AUTH_PASSWORD is empty; PD will answer 401 unless it runs
without auth"
Review Comment:
🧹 This warning fires for deployments that never talk to PD, including the
standalone RocksDB quickstart.
Lines 47-51 are at top level, above the `if env | grep '^hugegraph\.'` guard
on line 103 and the `if [ -n "$PD_PEERS" ]` branch on line 111.
`hugegraph-dist/docker/docker-entrypoint.sh:189` runs `wait-storage.sh` on
every first container start, and `docker/docker-compose.yml` (standalone,
RocksDB) sets no `PD_AUTH_PASSWORD` and has no PD - so that container now logs
`WARN: PD_AUTH_PASSWORD is empty; PD will answer 401 ...` and then, two lines
later, `No pd.peers configured, skipping storage wait`.
Requested change: move the `if [ -z "${PD_AUTH_PASSWORD}" ]` warning into
the branch that actually polls PD, next to `log "PD REST peers =
$PD_REST_LIST"` (line 121), and leave only the variable defaults here.
--
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]
