bitflicker64 commented on code in PR #3189:
URL: https://github.com/apache/hugegraph/pull/3189#discussion_r3943249865
##########
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:
Fixed in 94a4a0f, taking the suggestion. The bullet now describes the abort
and names both strings the script actually prints, `ERROR: PD at <peer> refused
the credential (401)` and `ERROR: storage wait aborted, see the message above`;
I grepped the script for each to be sure the docs do not promise a string that
cannot appear again.
##########
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:
Fixed in 94a4a0f, taking the suggestion. Same correction as
`docker/README.md`: startup aborts on the first 401 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:
Fixed in 94a4a0f, taking `docker/conf/hubble/*.local.properties*`. Confirmed
your reproduction first (the base name ignored,
`hstore.local.properties.aB3xY9` not), and after the change both forms are
ignored while `hstore.properties.example`, `hstore-ha.properties.example` and
`standalone.properties` stay tracked. Good catch on the crash window; the trap
only covers a clean exit.
##########
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:
Fixed in 94a4a0f, both parts. `check()` captures `before="$FAIL"` and guards
the ok line with an `if` rather than `&&`, for the `set -e` reason you gave.
Reproduced the contradiction first, then confirmed the failing file now prints
only its FAIL line while the loop still visits every file and the exit stays 1.
The exposure lookup is now an awk that takes the `include:` under the actuator
`exposure:` block instead of the first one in the file.
--
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]
