bitflicker64 commented on code in PR #3189:
URL: https://github.com/apache/hugegraph/pull/3189#discussion_r3940228462
##########
docker/README.md:
##########
@@ -60,12 +66,59 @@ behind an HTTPS reverse proxy and trusted network controls.
first authenticated startup. Changing `.env` does not rotate an existing
administrator password; use the HugeGraph user API for credential changes.
-For the verification commands below, set the password in your current shell:
+For the verification commands below, load `.env` into your current shell and
+set the password:
```bash
+set -a; . ./.env; set +a
ADMIN_PASSWORD='the-same-password-used-in-.env'
```
+Compose reads `.env` on its own; the line above is so that the `curl` command
and
+the Hubble helper on this page can use `${HG_PD_AUTH_SECRET_KEY}` too.
+
+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`.
Review Comment:
🧹 This bullet documents the failure mode this same head replaced. A wrong
secret no longer waits out `WAIT_STORAGE_TIMEOUT_S`.
`wait-storage.sh:140-142` returns 2 on a 401 and `:152-153` turns that into
`exit 2`, which is not 124, so the outer handler prints `ERROR: storage wait
aborted, see the message above` (`:164`) rather than the timeout message
(`:162`). `travis/test-wait-storage.sh:255` asserts exactly that: `[[
"${CASE_OUTPUT}" != *"Timeout waiting"* ]] || fail "401 was reported as a
timeout"`.
An operator on a stale secret is told to wait 300s and to look for a string
that cannot appear.
```suggestion
the Server sends the wrong secret `wait-storage.sh` aborts on the first
401 rather than waiting out `WAIT_STORAGE_TIMEOUT_S`, and the container
exits with `ERROR: storage wait aborted, see the message above` after
logging `ERROR: PD at <peer> refused the credential (401)`.
```
##########
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 for
+ `wait-storage.sh` that means Server startup aborts after
+ `WAIT_STORAGE_TIMEOUT_S`.
Review Comment:
🧹 The same stale claim as `docker/README.md:98-100`: a 401 no longer costs
`WAIT_STORAGE_TIMEOUT_S`.
`wait-storage.sh:140-142` returns 2 on the first 401 and `:152-153` exits
immediately, so startup aborts on the first attempt, not after the timeout.
```suggestion
`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`.
```
##########
.gitignore:
##########
@@ -44,6 +44,8 @@ build/
.env.test.local
.env.production.local
docker/.env
+# generated by docker/set-hubble-pd-password.sh, carries the PD REST secret
+docker/conf/hubble/*.local.properties
Review Comment:
🧹 This pattern, which I asked for on the `hstore.properties.example` thread,
misses the temporary file the generator writes through.
`set-hubble-pd-password.sh:45` uses `tmp=$(mktemp "${out}.XXXXXX")`, so the
plaintext secret lands first in
`docker/conf/hubble/hstore.local.properties.aB3xY9`, inside the tracked
`conf/hubble/` directory, and `*.local.properties` does not match a name with a
suffix after `.properties`:
```
$ git check-ignore -q docker/conf/hubble/hstore.local.properties && echo
IGNORED
IGNORED
$ git check-ignore -q docker/conf/hubble/hstore.local.properties.aB3xY9 ||
echo "NOT ignored"
NOT ignored
```
The `EXIT` trap clears it on a normal failure but not on SIGKILL or a crash,
and a `git add -A` racing the window before `mv` would stage it. One trailing
`*` closes both:
```suggestion
docker/conf/hubble/*.local.properties*
```
Verified after applying it: the base name and both suffixed forms are
ignored, while `hstore.properties.example`, `hstore-ha.properties.example` and
`standalone.properties` stay tracked.
##########
hugegraph-server/hugegraph-dist/src/assembly/travis/test-pd-shipped-config.sh:
##########
@@ -0,0 +1,53 @@
+#!/usr/bin/env bash
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+# Every PD configuration that ships in the archive or the jar must carry the
+# same REST hardening: no wildcard actuator exposure (that path is anonymous),
+# an auth.secret-key that is present and empty, and no copy of the secret that
+# earlier revisions published. A fix applied to one variant and not the others
+# is what this catches.
+
+set -euo pipefail
+
+ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/../../../../.." && pwd)"
+PUBLISHED_SECRET='FXQXbJtbCLxODc6tGci732pkH1cyf8Qg'
+FAIL=0
+
+check() {
+ local file="$1" rel="${1#"${ROOT}/"}"
+ [[ -f "$file" ]] || { echo " FAIL ${rel}: missing"; FAIL=1; return; }
+
+ local exposure
+ exposure=$(sed -n 's/^[[:space:]]*include:[[:space:]]*//p' "$file" | head
-1)
+ if [[ "$exposure" == *'*'* ]]; then
+ echo " FAIL ${rel}: actuator exposure is a wildcard (${exposure})";
FAIL=1
+ fi
+ if ! grep -qE '^[[:space:]]*secret-key:[[:space:]]*$' "$file"; then
+ echo " FAIL ${rel}: auth.secret-key must be present and empty"; FAIL=1
+ fi
+ if grep -q "${PUBLISHED_SECRET}" "$file"; then
+ echo " FAIL ${rel}: contains the published secret"; FAIL=1
+ fi
+ echo " ok ${rel}"
Review Comment:
🧹 A file that just failed is also reported as `ok`, so the per-file output
contradicts the summary.
The three checks above only set the global `FAIL=1`; this line then runs
unconditionally. Putting `include: "*"` back into one config gives:
```
FAIL hugegraph-pd/hg-pd-service/src/main/resources/application.yml:
actuator exposure is a wildcard ("*")
ok hugegraph-pd/hg-pd-service/src/main/resources/application.yml
shipped PD config check failed
```
The exit status is still 1, so CI fails correctly; only the per-file line,
which is the reason it exists, misleads.
Capture the counter at the top of `check()` (`local file="$1"
rel="${1#"${ROOT}/"}" before="$FAIL"`) and wrap this line in `if [[ "$FAIL" ==
"$before" ]]; then ... fi`. Use `if` rather than `[[ ... ]] && echo`: as the
function's last command the latter returns 1 whenever a check failed, and `set
-e` would then abort the loop at the first bad file.
While here, line 35 takes the first `include:` in the file rather than the
actuator one (`sed -n 's/^[[:space:]]*include:.../p' | head -1`). All three
configs have exactly one today, so the check is correct now, but a config that
grows an unrelated `include:` above the actuator block would pass silently.
--
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]
