bitflicker64 commented on PR #3189:
URL: https://github.com/apache/hugegraph/pull/3189#issuecomment-5560593174
@imbajin Ready for a review pass when you have time, though please read the
second section first: I got something wrong in this PR and the last three
commits correct it.
**Correction, and the reason for `fdec006b1` and `ffd88d31c`.** This PR
moves the interceptor exclusion from `/actuator/*` to `/actuator/**`, and it
shipped an explanation saying the single star would have caused the auth
interceptor to refuse nested probe paths such as `/actuator/metrics/{name}`.
That explanation is wrong. The interceptor never runs on any actuator path,
with either pattern.
`RestAuthentication` is registered through
`WebMvcConfigurer.addInterceptors`, so it lands in the MVC
`InterceptorRegistry`. Actuator is served by `WebMvcEndpointHandlerMapping`,
and `AbstractHandlerMapping.detectMappedInterceptors` collects
`MappedInterceptor` **beans** only, via `beansOfTypeIncludingAncestors`. The
registry does build a `MappedInterceptor` here, since both include and exclude
patterns are supplied, but it is never published as a bean, so the actuator
mapping does not pick it up. `WebMvcConfigurationSupport.getInterceptors`
pushes the registry's interceptors onto the MVC handler mappings alone.
Checked empirically as well as by reading the sources at the versions
`hg-pd-service` resolves, Spring Boot 2.5.14 and spring-webmvc 5.3.20, on Jetty
to match PD. Four cases, with the interceptor logging every request it saw:
| exclusion | `/actuator/health` | `/actuator/metrics/jvm.memory.used` |
`/v1/foo` |
|---|---|---|---|
| `/actuator/*` | 200 | 200 | 401 |
| a pattern matching nothing | 200 | 200 | 401 |
| no registry interceptor at all | 200 | 200 | 200 |
| same interceptor as a `MappedInterceptor` bean on `/**` | 401 | 401 | 401 |
In the first two the interceptor logged only `/v1/foo`. The third is the
control showing the interceptor is what refuses `/v1/foo`. The fourth is the
positive control: published as a bean, the identical interceptor does reach
actuator paths. So bean detection is the mechanism, not the exclusion pattern.
What actually keeps those paths anonymous is that actuator has its own
handler mapping, and what bounds them is the exposure allowlist, which this PR
narrows from `include: "*"` to `health,metrics,prometheus` in every shipped
config. That is the real security improvement here, and it stands.
`testUnexposedActuatorEndpointIsClosed` is the assertion that guards it.
`/actuator/**` stays in the exclusion list: it states the intent correctly
for nested probe paths and costs nothing. `fdec006b1` and `ffd88d31c` correct
the explanation in `AuthenticationConfigurer`, `RestApiTest`,
`docs/configuration.md`, and the comment above
`management.endpoints.web.exposure.include` in `hg-pd-service/application.yml`,
`hg-pd-dist/conf/application.yml` and its `.template`. Comments and prose only,
no assertion or code path changed. The message on `a890122` still carries the
old wrong reasoning; I left it rather than rewrite public history.
**What else I checked on the branch.** PD booted from the shipped
`hg-pd-dist` config and probed without credentials: every authenticated `/v1`
path answers 401 with the constant body `{"status":-1,"error":"Unauthorized"}`
and a `WWW-Authenticate: Basic realm="hugegraph-pd"` challenge. `/v1/health`
and `/v1/prom/targets/*` stay anonymous by design.
On the previously published placeholder secret PD exits 1 as intended. With
no `auth` block at all PD starts, logs the error once, keeps raft and gRPC
serving, and fails closed on REST. The live secret appears nowhere in `pd.log`,
`logs/` or `pd_data/`, and the refusal log is debug level with method, URI and
reason only. `test-pd-shipped-config.sh` passes against the edited configs.
One wording note on the credential path: `Authentication.authenticate` wraps
everything in a `RuntimeException`, so callers see `BadCredentialsException` or
`AccessDeniedException` as the cause, which is why `RestAuthentication` unwraps
with `e.getCause() != null ? e.getCause() : e`.
**Merge-order conflict with #3185, worth deciding before either lands.**
Both PRs rewrite the same line, `AuthenticationConfigurer.addInterceptors`.
This one makes it `excludePathPatterns("/actuator/**", "/v1/health",
"/v1/prom/targets/*")`. #3185 makes it `excludePathPatterns("/actuator/*",
"/v1/health", "/v1/ready", "/v1/prom/targets/*")`. Whichever lands second
conflicts, and the two changes disagree about two independent things at once.
The correct union is:
```java
excludePathPatterns("/actuator/**", "/v1/health", "/v1/ready",
"/v1/prom/targets/*")
```
Dropping `/v1/ready` puts that endpoint back behind authentication and
defeats #3185. `/v1/ready` is the half that actually matters, since it is on
the MVC mapping where the exclusion list is load-bearing.
**One follow-up I would not hold the merge for.** gRPC on 8686 stays
unauthenticated. The TODO explains why it cannot be enabled yet, which I think
is right for this PR, but it is worth a release-note line so "PD REST auth" is
not read as "PD is authenticated".
**On CI.** The `store` job went red on `a890122` at
`OrderedKvIteratorTest.testConcurrentInitializeFailsWithoutWaitingForSlowSource:165`,
a timing assertion in `hg-store-test` that is unrelated to a commit touching
six comment lines. It passed on the previous head. The push of `ffd88d31c`
retriggers the run.
--
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]
