bitflicker64 commented on code in PR #3189:
URL: https://github.com/apache/hugegraph/pull/3189#discussion_r3923380792
##########
hugegraph-pd/hg-pd-service/src/main/java/org/apache/hugegraph/pd/service/interceptor/Authentication.java:
##########
@@ -77,19 +84,33 @@ protected <T> T authenticate(String authority, String
token, Function<String, T>
}
String name = info.substring(0, delim);
- // TODO: password validation is skipped — only service name is
checked against
- // innerModules. Full credential validation should be added as
part of the auth refactor.
- //String pwd = info.substring(delim + 1);
- if (innerModules.contains(name)) {
- return call.get();
- } else {
+ String pwd = info.substring(delim + 1);
+ if (!innerModules.contains(name)) {
throw new AccessDeniedException("invalid service name");
}
+ if (!verifySecret(pwd)) {
+ throw new BadCredentialsException("invalid credential");
+ }
+ return call.get();
} catch (Exception e) {
throw new RuntimeException(e);
}
}
+ /**
+ * Compare the password of the Basic credential with the shared secret
+ * configured via `auth.secret-key`. A missing or empty secret refuses
every
+ * request instead of falling back to name-only authentication.
Review Comment:
‼️ This contract does not hold. A PD whose config omits `auth.secret-key`
does not refuse every request, it authenticates against a string no client will
send.
`PDConfig.java:72` declares the field as `@Value("${auth.secret-key:
'FXQXbJtbCLxODc6tGci732pkH1cyf8Qg'}")`. Spring's `PropertyPlaceholderHelper`
takes everything after the first `:` as a literal default, with no trimming and
no quote stripping. Against spring-core 5.3.20, the line
`spring-boot-starter-web:2.5.14` resolves:
```
resolved=[ 'FXQXbJtbCLxODc6tGci732pkH1cyf8Qg']
equalsDocumented=false
```
So `getSecretKey()` returns `" 'FXQXbJtbCLxODc6tGci732pkH1cyf8Qg'"`,
`StringUtils.isEmpty` is false, the branch below never runs, and `verifySecret`
rejects the value this PR ships in both `application.yml` files,
`wait-storage.sh` and the Hubble properties.
`start-hugegraph-pd.sh:178` passes
`-Dspring.config.location=${CONF}/application.yml`, which replaces the default
config locations rather than adding to them, so only that one file is read. An
in-place upgrade keeping the operator's existing `conf/application.yml` has no
`auth` block, and the failure is not a warning: every client gets 401,
`wait-storage.sh:118-124` retries until `WAIT_STORAGE_TIMEOUT_S=300` expires,
then exits 1.
Requested change: in `PDConfig.java:72` use `@Value("${auth.secret-key:}")`
so an absent key yields `""` and the fail-closed path this javadoc describes
actually runs, and log an ERROR naming `auth.secret-key` when it is empty. That
makes the failure diagnosable rather than preventing it, so the upgrade still
needs a release note telling operators to add the key to an existing
`conf/application.yml` first. `PDConfig.java:57` has the same quoted-default
shape for `pd.initial-store-list`, worth a separate pass.
##########
hugegraph-pd/README.md:
##########
@@ -280,6 +281,19 @@ 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.
Review Comment:
⚠️ `/actuator/*` covers more than the probes this line describes, on the
port the PR is hardening.
Both shipped configs set `management.endpoints.web.exposure.include: "*"` on
port 8620 (`hg-pd-dist/src/assembly/static/conf/application.yml:30`,
`hg-pd-service/src/main/resources/application.yml:30`), and
`AuthenticationConfigurer.java:35` excludes `/actuator/*` from the interceptor.
Spring Boot 2.5's full actuator set includes endpoints that dump process and
configuration state, and every one of them whose path is a single segment is
anonymous on 8620. The three the docs actually reference are `health`,
`metrics` and `prometheus` (`hugegraph-pd/README.md:198`,
`hugegraph-pd/docs/configuration.md:705,718,731`).
Pre-existing configuration, so not a regression, but this PR is where the
port gets a credential worth protecting, and
`hugegraph-pd/docs/configuration.md:262` already documents the narrower form.
Requested change: allowlist the endpoints in both `application.yml` files
instead of exposing all of them.
management:
endpoints:
web:
exposure:
include: "health,metrics,prometheus"
Then this bullet is accurate as written. A follow-up PR is fine too if you
want to keep this one narrow.
##########
hugegraph-server/hugegraph-dist/src/assembly/static/bin/wait-storage.sh:
##########
@@ -39,7 +39,9 @@ log() {
echo "[wait-storage] $1"
}
-PD_AUTH_ARGS="-u ${PD_AUTH_USER:-store}:${PD_AUTH_PASSWORD:-admin}"
+# PD validates the password against its auth.secret-key; the default below
+# matches PD's shipped default. Override both when the PD secret is changed.
+PD_AUTH_ARGS="-u
${PD_AUTH_USER:-store}:${PD_AUTH_PASSWORD:-FXQXbJtbCLxODc6tGci732pkH1cyf8Qg}"
Review Comment:
🧹 This value ends up as shell source, so a secret containing shell
metacharacters breaks startup or runs.
Line 106 interpolates `${PD_AUTH_ARGS}` unquoted into the `bash -c "..."`
string opened at line 100. Everything else inside that string is escaped
against the outer shell (`\$peer`, `\"$PD_REST_LIST\"`); this one is not, so
the expanded credential becomes part of the inner script's source text. A
secret containing a space splits into two curl arguments, one containing `$` or
a backtick expands inside the inner shell, and `;` or `$(...)` executes. The
failure mode is the 300s timeout and `exit 1` at line 124, with nothing
pointing at the password.
Pre-existing shape, but this PR is what starts telling operators to set an
arbitrary `PD_AUTH_PASSWORD`, so it is worth closing here.
While in this line: `curl -u user:secret` puts the credential in the process
argv, readable by anything that can see `/proc` in the container.
Requested change: keep the credential out of both the inner script text and
argv. Export the two values and let the inner shell read them from the
environment, for example `export PD_AUTH_USER PD_AUTH_PASSWORD` here and `curl
--user "$PD_AUTH_USER:$PD_AUTH_PASSWORD"` at line 106 with the `$` escaped so
the inner shell expands it, or feed a config file with `curl -K -`.
##########
docker/README.md:
##########
@@ -66,6 +66,22 @@ For the verification commands below, set the password in
your current shell:
ADMIN_PASSWORD='the-same-password-used-in-.env'
```
+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
+with a default secret in `conf/application.yml` (`auth.secret-key`), and the
+Hubble files under `conf/hubble/` carry the matching `operations.pd.password`.
+With the shipped default, list registered stores like this:
+
+```bash
+curl -u hg:FXQXbJtbCLxODc6tGci732pkH1cyf8Qg http://localhost:8620/v1/stores
+```
+
+The default secret is public (it is in the source tree), so it only keeps
+casual traffic out. On any shared network, change it: set
+`HG_PD_AUTH_SECRET_KEY` on the PD services and put the same value in the
+Hubble properties files, or do not publish port 8620 at all.
Review Comment:
⚠️ Following this procedure as written stops the Server container coming up.
The Server is a PD REST client too, with its own copy of the default.
`wait-storage.sh:44` sets `PD_AUTH_ARGS` from
`${PD_AUTH_PASSWORD:-FXQXbJtbCLxODc6tGci732pkH1cyf8Qg}`, and the Server
entrypoint runs the script on first boot
(`hugegraph-server/hugegraph-dist/docker/docker-entrypoint.sh:189`). Rotate
`HG_PD_AUTH_SECRET_KEY` on the PD services and update the Hubble files, exactly
as this paragraph says, and the Server still sends the shipped secret: `curl -f
.../v1/stores` gets 401, the `until` loop at `wait-storage.sh:118-121` never
succeeds, `timeout 300s` fires and line 124 exits 1. The only clue is `ERROR:
Timeout waiting for storage backend`.
The plumbing is already there. `env "${WAIT_ENV[@]}" ./bin/wait-storage.sh`
adds to the inherited environment rather than replacing it, so a
container-level `PD_AUTH_PASSWORD` reaches the script. `grep -rn
PD_AUTH_PASSWORD` finds it only in `wait-storage.sh` and its test, so nothing
sets it.
Requested change: name the Server side in this paragraph, and wire it in
both Compose files so one `.env` value drives all three consumers. In
`docker-compose-3pd-3store-3server.yml` that means the `x-pd-common` anchor
(line 34):
HG_PD_AUTH_SECRET_KEY: ${HG_PD_AUTH_SECRET_KEY:-}
and the `x-server-environment` anchor (line 62), plus the `server` service
in `docker-compose-hstore.yml`:
PD_AUTH_PASSWORD:
${HG_PD_AUTH_SECRET_KEY:-FXQXbJtbCLxODc6tGci732pkH1cyf8Qg}
The tarball path needs the same sentence: `hugegraph-pd/README.md:293-295`
should list `PD_AUTH_PASSWORD` for `bin/wait-storage.sh` next to Hubble's
`operations.pd.password`.
##########
hugegraph-pd/hg-pd-service/src/main/java/org/apache/hugegraph/pd/rest/interceptor/RestAuthentication.java:
##########
@@ -59,6 +59,7 @@ 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);
Review Comment:
🧹 A 401 with no `WWW-Authenticate` is not a challenge, so clients that
authenticate reactively never retry with credentials.
RFC 7235 §3.1 makes the header mandatory on a 401. `curl -u` sends Basic
preemptively, which is why the manual matrix in the PR description and the new
`RestApiTest` cases do not notice, but
`java.net.http.HttpClient.authenticator(...)`, `HttpURLConnection` with a
default `Authenticator`, Apache HttpClient without preemptive auth configured,
and browsers all wait for the challenge and simply fail.
Requested change:
```suggestion
response.setStatus(HttpServletResponse.SC_UNAUTHORIZED);
response.setHeader("WWW-Authenticate", "Basic
realm=\"hugegraph-pd\"");
```
##########
hugegraph-pd/hg-pd-service/src/main/java/org/apache/hugegraph/pd/service/interceptor/Authentication.java:
##########
@@ -77,19 +84,33 @@ protected <T> T authenticate(String authority, String
token, Function<String, T>
}
String name = info.substring(0, delim);
- // TODO: password validation is skipped — only service name is
checked against
- // innerModules. Full credential validation should be added as
part of the auth refactor.
- //String pwd = info.substring(delim + 1);
- if (innerModules.contains(name)) {
- return call.get();
- } else {
+ String pwd = info.substring(delim + 1);
+ if (!innerModules.contains(name)) {
throw new AccessDeniedException("invalid service name");
}
+ if (!verifySecret(pwd)) {
+ throw new BadCredentialsException("invalid credential");
+ }
Review Comment:
🧹 The new check lands in the shared base class, which arms a trap on the
gRPC path.
`GrpcAuthentication extends Authentication` and calls this same method
(`GrpcAuthentication.java:56`). It is inert at this head: lognet's
`GRpcServerRunner.java:65-69` (grpc-spring-boot-starter 4.5.5) registers only
`ServerInterceptor` beans annotated `@GRpcGlobalInterceptor`,
`GrpcAuthentication` carries just `@Service`, and `GRpcServerConfig.java:43-44`
has the TODO saying so. This PR does not break the cluster today.
But every in-repo gRPC client sends a password this method now rejects:
`ServiceConstant.AUTHORITY = ""` for Server
(`hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/constant/ServiceConstant.java:28`),
`DefaultPdProvider.authority = "default"` for Store (line 70), `""` for
`hg-pd-cli` (`Command.java:35`). Whoever clears that TODO gets a cluster that
cannot register a store, with no hint that this PR is where the requirement
came from.
Requested change: either move the secret check into the REST subclass so the
base class keeps the name-only behaviour those clients were written against, or
extend the TODO at `GRpcServerConfig.java:43` to record that enabling the
interceptor now also requires giving those three clients the `auth.secret-key`
value.
##########
hugegraph-pd/README.md:
##########
@@ -100,6 +100,7 @@ Key configuration file: `conf/application.yml`
| `raft.address` | `127.0.0.1:8610` | Raft service address for this PD node |
| `raft.peers-list` | `127.0.0.1:8610` | Comma-separated list of all PD nodes
in the Raft cluster |
| `pd.data-path` | `./pd_data` | Directory for storing PD metadata and Raft
logs |
+| `auth.secret-key` | (public default) | Password required by the REST API
with an internal service name (`hg`, `store`, `hubble`, `vermeer`) via HTTP
Basic auth. Change it in production and configure every REST client (e.g.
Hubble's `operations.pd.password`) with the same value |
Review Comment:
🧹 This row is the only place the credential is documented. The two files an
operator reaches for next do not have it.
`hugegraph-pd/docs/configuration.md` is the PD configuration reference, with
a parameter table per section, and contains zero occurrences of `auth`.
`hugegraph-pd/docs/api-reference.md` is the REST reference and never mentions
authentication either: its REST API section opens at line 759 and the examples
at 804 and 832 (`/v1/partitions?graph_name=...`, `/v1/stores`) carry no
credential.
Requested change: add the `auth.secret-key` row to `configuration.md` and a
two-line authentication note at the top of the REST API section in
`api-reference.md`.
The runbooks under `hugegraph-store/docs/` have the same problem at roughly
twenty more call sites
(`operations-guide.md:92,231,261,280,337,486,496,579,607,613,619`,
`deployment-guide.md:475,589,869,872,875,878`,
`integration-guide.md:709,736,739`, `hugegraph-store/README.md:263`), which is
a reasonable follow-up rather than something to load onto this PR. One is worth
fixing now: `operations-guide.md:613` is `curl -X POST
http://192.168.1.10:8620/v1/balanceLeaders`, a mutating step an operator runs
during an incident and reads as a no-op when it 401s. The `/actuator/*` and
`/v1/health` examples in all of those files stay valid.
--
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]
