bitflicker64 commented on code in PR #3189:
URL: https://github.com/apache/hugegraph/pull/3189#discussion_r3924727033
##########
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:
Fixed in 1e3b616, taking the suggestion as written. Verified on the wire:
`HTTP/1.1 401 Unauthorized` now carries `WWW-Authenticate: Basic
realm="hugegraph-pd"`. You are right that the existing coverage could not catch
it, since `curl -u` and the new `RestApiTest` cases both send Basic
preemptively.
##########
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:
Fixed in 1e3b616. `docs/configuration.md` gained a REST Authentication
section with the `auth.secret-key` row, the unauthenticated paths and the
client-side names, and `docs/api-reference.md` gained a note at the top of the
REST API section saying the examples below omit `-u` and which paths do not
need it. Took the `operations-guide.md` `balanceLeaders` procedure now as you
suggested: it and the two calls in the same runbook step carry `-u
hg:"${PD_SECRET}"`, with a note at the top of the file defining it. The wider
sweep across the other Store docs is the follow-up.
##########
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:
Fixed in 1e3b616, taking the second option. The check stays in the base
class, so the gRPC path is not left permanently name-only, and the
`GRpcServerConfig` TODO now records that registering the interceptor first
requires giving the secret to the three clients, naming
`ServiceConstant.AUTHORITY`, `DefaultPdProvider.authority` and `hg-pd-cli` with
their current `""` / `"default"` values. Verified the interceptor is still
unregistered at this head, so nothing changes for gRPC today.
--
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]
