bitflicker64 commented on code in PR #3189: URL: https://github.com/apache/hugegraph/pull/3189#discussion_r3939615797
########## hugegraph-pd/hg-pd-service/src/test/java/org/apache/hugegraph/pd/service/interceptor/AuthenticationTest.java: ########## @@ -0,0 +1,127 @@ +/* + * 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. + */ + +package org.apache.hugegraph.pd.service.interceptor; + +import java.lang.reflect.Field; +import java.nio.charset.StandardCharsets; +import java.util.Base64; + +import org.apache.hugegraph.pd.config.PDConfig; +import org.junit.Assert; +import org.junit.Test; + +/** + * In-process cover for the REST credential check. The suites that exercise it + * over HTTP talk to a PD in another JVM, so nothing here is covered by them. + */ +public class AuthenticationTest { + + private static final String SECRET = "unit-test-secret"; + + private static Authentication authWithSecret(String secret) throws Exception { Review Comment: Fixed in 94b1566, and the cascade was mine. Your diagnosis is exactly right: the repackage puts `hg-pd-service` classes under `BOOT-INF/classes`, so `hg-pd-test` can never compile against it in an `install` build, which is why no existing test there imports service classes. `AuthenticationTest` now lives in `hg-pd-service/src/test/java` with junit at test scope, and `PDCoreSuiteTest` no longer references it. Verified with the same `mvn install -Dmaven.test.skip=true` path the commons job runs (clean) and `mvn test -pl hugegraph-pd/hg-pd-service -am` (8/8). ########## hugegraph-server/hugegraph-dist/src/assembly/static/bin/wait-storage.sh: ########## @@ -39,7 +39,25 @@ 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" +fi +# curl -K takes a quoted string, so escape backslash first and then quote Review Comment: Fixed in 94b1566. `escape_curlrc` now maps LF, CR, TAB and VT to the `\n \r \t \v` escapes curl's quoted config strings accept, after backslash and quote. `test-wait-storage.sh` gained a case whose secret contains CR, LF, a backslash and a quote, asserting every config line the mock reads is the single escaped `user = "..."` form; 6/6 pass. ########## hugegraph-pd/hg-pd-dist/src/assembly/static/conf/application.yml: ########## @@ -27,7 +27,9 @@ management: endpoints: web: exposure: - include: "*" + # Allowlist, not "*": /actuator/* is excluded from the REST auth + # interceptor, so anything exposed here is anonymous on this port. + include: "health,metrics,prometheus" Review Comment: Fixed in 94b1566. `application.yml.template` now carries the same allowlist and an empty `auth.secret-key` block. The regression check you asked for is `travis/test-pd-shipped-config.sh`, wired into the `pd` job: for every `hg-pd-dist/.../conf/application.yml*` and the `hg-pd-service` resources config it fails on a wildcard exposure, a missing or non-empty `secret-key`, or the published secret string. ########## docker/docker-compose-3pd-3store-3server.yml: ########## @@ -107,6 +109,7 @@ services: HG_PD_INITIAL_STORE_LIST: store0:8500,store1:8500,store2:8500 Review Comment: Fixed in 94b1566. `hugegraph-pd/README.md` lists `HG_PD_AUTH_SECRET_KEY` on each of the three PD container blocks with a line saying it must be the same value everywhere, and `hugegraph-store/docs/deployment-guide.md` generates and exports it before the `docker compose up` line and adds it to the per-node PD environment table. Both point at the `.env` recipe in `docker/README.md` for the persistent form. ########## docker/README.md: ########## @@ -60,12 +61,56 @@ 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` and +`sed` commands 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`. +- 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. + +Write it in, after loading `.env` as above. Use `hstore.properties` for the +Minimal HStore topology and `hstore-ha.properties` for HA: + +```bash +if [ -n "${HG_PD_AUTH_SECRET_KEY:-}" ]; then + sed -i.bak \ + "s#^operations.pd.password=.*#operations.pd.password=${HG_PD_AUTH_SECRET_KEY}#" \ Review Comment: Fixed in 94b1566. Replaced the inline `sed` with `docker/set-hubble-pd-password.sh`: it drops the old line with `grep -v` and appends via `printf %s`, so the value never enters a replacement pattern, doubles backslashes for `.properties`, refuses an empty value or a line break, and writes with `cat >` to keep the mode the read-only mount needs. `test-compose.sh render` now runs it with `a&b#c\d` and checks the exact line, the mode, and the empty-value refusal. ########## hugegraph-pd/hg-pd-service/src/main/resources/application.yml: ########## @@ -27,7 +27,9 @@ management: endpoints: web: exposure: - include: "*" + # Allowlist, not "*": /actuator/* is excluded from the REST auth + # interceptor, so anything exposed here is anonymous on this port. + include: "health,metrics,prometheus" Review Comment: Fixed in 94b1566, taking `/actuator/**`. The reachable set there is bounded by the exposure allowlist, so widening the exclusion does not open anything the config does not expose. `RestApiTest` now asserts `/actuator/metrics/jvm.memory.used` answers 200 without a credential and `/actuator/env` never serves data; verified live on the packaged dist (nested metrics 200, `env` and `beans` closed, `/v1/members` still 401 without the secret). -- 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]
