imbajin commented on code in PR #3189: URL: https://github.com/apache/hugegraph/pull/3189#discussion_r3933761830
########## hugegraph-pd/hg-pd-test/src/main/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: ‼️ The new `AuthenticationTest` cannot be compiled in `hg-pd-test`. On exact head `ace8913a079810503810584cdc552bb710fe601a`, the `pd`, `hstore`, and `store` jobs, `build-commons (11)`, the macOS jobs, and the Docker build matrix fail in `maven-compiler-plugin` with `cannot find symbol: class Authentication` at this file's lines 36 and 51. Although `hg-pd-test` declares `hg-pd-service`, that module's Spring Boot repackage replaces the main artifact with a fat jar whose classes are under `BOOT-INF/classes`, so javac cannot resolve this new test under `hg-pd-test/src/main/java`; the checks fail before the authentication tests execute. Please move the test into `hg-pd-service`'s test source set or provide/use an un-repackaged service artifact, then rerun the failed checks. ########## 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: ⚠️ The documented unauthenticated actuator allowlist does not cover nested endpoint paths. `AuthenticationConfigurer` still excludes only `/actuator/*`; Spring's path matcher does not match `/actuator/metrics/{name}` or `/actuator/health/{group}`, so these exposed `metrics`/`health` sub-endpoints are intercepted and return 401. Please change the exclusion to `/actuator/**` if all exposed actuator paths are intended to remain probe-accessible, or narrow both the allowlist and docs to exact root paths and add tests. ########## 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: ⚠️ This allowlist is not applied to the other configuration shipped in the same archive. `server-assembly.xml` copies `conf/*`, but `application.yml.template` remains `management.endpoints.web.exposure.include: "*"`. Since `AuthenticationConfigurer` excludes `/actuator/*`, an operator who uses the packaged template can expose `/actuator/env` and `/actuator/beans` without Basic auth even though this changed config claims to close that path. Please update the template too and add a regression check for every shipped config variant. ########## 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: ⚠️ `escape_curlrc` only escapes backslashes and quotes, but the new password is emitted into a line-delimited curl config. On this head, `printf 'user = "store:%s"\n' 'a\nb' | curl -K - ...` warns `'b"' is unknown` and sends credentials for only `store:a`; the PD entrypoint accepts such a newline-containing secret, so Server and PD disagree and `wait-storage` retries until timeout. Please escape LF/CR and other curl-config controls or use a safely generated temporary config, and add LF/CR coverage to `test-wait-storage.sh`. ########## 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: ⚠️ Adding `${HG_PD_AUTH_SECRET_KEY:?…}` makes this Compose file fail before startup, but existing copy-paste deployment docs still omit the new required variable. `hugegraph-pd/README.md:163-185` lists all three PD containers without it, and `hugegraph-store/docs/deployment-guide.md:681` runs the same Compose file with only `HUGEGRAPH_VERSION`. Following those instructions now exits with a missing-variable error. Please update all supported deployment snippets to create/load one persistent secret and pass it to every PD client. ########## 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: ⚠️ This replacement treats the secret as a sed replacement, so legitimate `auth.secret-key` values containing `&`, `#`, or backslashes are corrupted or break the command. Exact reproduction with `HG_PD_AUTH_SECRET_KEY=a&b` produces `operations.pd.password=aoperations.pd.password=b`, while PD accepts arbitrary non-empty strings and the Docker entrypoint safely carries them. Hubble then uses a different password and receives 401. Please write the value through a safe config-generation path (or escape the replacement) and test a non-hex 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]
