bitflicker64 commented on code in PR #3189: URL: https://github.com/apache/hugegraph/pull/3189#discussion_r3934249764
########## docker/conf/hubble/hstore.properties: ########## @@ -20,6 +20,11 @@ pd.enabled=true server.direct_url=http://server:8080 pd.peers=pd:8686 pd.server=pd:8620 +# PD REST credential. The password must equal PD's auth.secret-key, which has +# no default: set it to the same value as HG_PD_AUTH_SECRET_KEY in .env. While +# it is empty, Hubble's PD-backed views get HTTP 401 from PD. +operations.pd.username=hubble +operations.pd.password= Review Comment: ⚠️ Hubble has no path from `.env` to this value, so its PD-backed views answer 401 in both shipped HStore stacks. The fix spans `docker/README.md`, the two compose files and `.gitignore`, not just this line. `docker-compose-hstore.yml:120` mounts this file verbatim (`./conf/hubble/hstore.properties:/hubble/conf/hugegraph-hubble.properties:ro`), and `docker-compose-3pd-3store-3server.yml:239` does the same for `hstore-ha.properties`. Compose does not interpolate properties files, and the `hugegraph/hubble` image has no entrypoint that rewrites config from the environment: hugegraph-toolchain ships only `hugegraph-hubble/Dockerfile`, whose `ENTRYPOINT` is `./bin/start-hubble.sh -f`, and `operations.pd.password` is read through `HubbleOptions` into a `HugeConfig` from the file, not through Spring relaxed binding. The Server gets the secret via `PD_AUTH_PASSWORD: ${HG_PD_AUTH_SECRET_KEY:?...}`; Hubble gets nothing. The remedy `docker/README.md` documents is `sed -i.bak` over this file, which is tracked in git. That has an operator write a production secret into a tracked file in their clone, one `git add -A` away from being committed. `hstore-ha.properties:27` is identical. Requested change: imbajin already asked for a safe config-generation path on `docker/README.md:107`. When you build it, have it generate an untracked file rather than edit this one in place: ship `conf/hubble/hstore.properties.example`, generate `conf/hubble/hstore.local.properties` from it in the same step that writes `.env`, point the compose mount there, and add `docker/conf/hubble/*.local.properties` to `.gitignore` next to the existing `docker/.env` entry. The generation step has to run before `docker compose up`, otherwise Docker creates an empty directory at the bind path and Hubble starts with no config at all. ########## hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/config/PDConfig.java: ########## @@ -85,6 +98,17 @@ public class PDConfig { private ConfigService configService; private IdService idService; + @Override + public void afterPropertiesSet() { + if (PUBLISHED_SECRET_KEY.equals(this.secretKey)) { Review Comment: 🧹 Follow-on to the resolved `Authentication.java:112` thread, not a reopening of it: the one-shot ERROR you added in `verifySecret` fires on the first authenticated request, so it can arrive long after the misconfiguration, or never. This check rejects only `PUBLISHED_SECRET_KEY`. The empty case is the one both shipped `application.yml` files now produce, and it starts silently. Meanwhile the shipped PD healthchecks are `curl -fsS http://localhost:8620/v1/health`, which `AuthenticationConfigurer` excludes, so a PD with no secret reports healthy while refusing every real REST call. An operator sees it only as a client-side 401 or as the 300s `wait-storage.sh` timeout. Requested change: log it here too. `afterPropertiesSet` is already the startup hook, so adding `@Slf4j` to `PDConfig` and an `else if (StringUtils.isEmpty(this.secretKey))` branch that emits the same "auth.secret-key is not configured" message puts the condition in the boot log rather than only on first use. Throwing instead, for symmetry with the entrypoint's `require_env "HG_PD_AUTH_SECRET_KEY"`, would also work if you are willing to make it a hard upgrade gate. ########## hugegraph-server/hugegraph-dist/src/assembly/static/bin/wait-storage.sh: ########## @@ -101,10 +119,12 @@ if env | grep '^hugegraph\.' > /dev/null; then check_any_pd_stores() { for peer in \$(echo \"\$PD_REST_LIST\" | tr ',' ' '); do - if curl ${PD_AUTH_ARGS} -f -s \ + if printf 'user = \"%s:%s\"\n' \ + \"\$PD_AUTH_CURL_USER\" \"\$PD_AUTH_CURL_PASSWORD\" | \ + curl -K - -f -s \ Review Comment: ⚠️ A 401 from PD is indistinguishable here from "PD unreachable" and from "no store is Up", so a missing or stale secret surfaces as a storage-backend failure. curl's exit status never reaches the `if`: lines 122-128 are `printf ... | curl ... | grep -qi '"state"...'`, and neither this script, `util.sh`, nor the inner `bash -c` sets `pipefail`, so the `if` tests grep alone. On a 401 the body is `{"status":-1,"error":"..."}`, grep finds no `"state": "Up"`, and the `until` loop sleeps 5s and retries until `timeout` fires. The operator's only signal is `ERROR: Timeout waiting for storage backend` after `WAIT_STORAGE_TIMEOUT_S` (300s, line 32), which names the wrong subsystem. The `WARN` at line 50 fires only when `PD_AUTH_PASSWORD` is empty, not when it is set to the wrong value. This is now the default first-boot path for anything outside the two updated compose files: a stock PD tarball ships `secret-key:` empty, so it refuses `/v1/stores`, and a stock Server then hangs five minutes and exits 1 pointing at storage. Requested change: take curl out of the pipeline so its status code is readable, and abort on 401 instead of retrying. Roughly: ```sh body=$(printf 'user = "%s:%s"\n' "$PD_AUTH_CURL_USER" "$PD_AUTH_CURL_PASSWORD" | curl -K - -s -w '\n%{http_code}' --connect-timeout 2 --max-time 3 \ "http://${peer}/v1/stores"; 2>/dev/null) code=${body##*$'\n'} if [ "$code" = 401 ]; then echo "PD at ${peer} refused the credential (401): PD_AUTH_PASSWORD must match PD's" echo "auth.secret-key" exit 1 fi ``` then grep the body as before. Adding `-w` to the existing pipeline would not help, since the code would be appended to the very stdout that feeds grep. ########## hugegraph-pd/hg-pd-service/src/main/java/org/apache/hugegraph/pd/rest/interceptor/RestAuthentication.java: ########## @@ -59,6 +59,10 @@ public boolean preHandle(HttpServletRequest request, HttpServletResponse respons authority = authority.replace("Basic ", ""); return authenticate(authority, token, tokenCall, DEFAULT_HANDLE); } catch (Exception e) { + response.setStatus(HttpServletResponse.SC_UNAUTHORIZED); + // RFC 7235 requires a challenge on a 401; without it clients that + // authenticate reactively never retry with credentials + response.setHeader("WWW-Authenticate", "Basic realm=\"hugegraph-pd\""); response.setContentType("application/json"); response.getWriter().println(new API().toJSON(e)); Review Comment: 🧹 The 401 body echoes the internal exception class to unauthenticated callers. `API.toJSON(Throwable)` (API.java:127) emits `exception.getMessage()`, and `Authentication.authenticate` rethrows as `new RuntimeException(e)`, whose message is `cause.toString()` per `Throwable(Throwable)`. So a caller with no valid credential gets a body equivalent to (key order is Jackson's over a `HashMap`, so not fixed): ```json {"status":-1,"error":"org.springframework.security.authentication.BadCredentialsException: invalid credential"} ``` and `...AccessDeniedException: invalid service name` for a name outside `innerModules`. That puts internal class names on an unauthenticated path and distinguishes "unknown service name" from "wrong password". Requested change: write a constant body on this path, `{"status":-1,"error":"Unauthorized"}`, and move the specific reason to a server-side `log.debug` or `log.warn`. -- 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]
