BobSong-dev commented on PR #7342:
URL: https://github.com/apache/shenyu/pull/7342#issuecomment-5882483503
> The refactor part of this is right and I want it: hoisting the master
check out of the listener loop and using `continue` instead of `return` removes
the dependence on listener iteration order, and the standalone path is
untouched. But I cannot approve as-is, because **the forwarded request is never
authenticated, so in a real cluster this does nothing**.
>
> ### The blocking problem
> `ShiroConfiguration:73-79` puts every path not in `shiro.white-list`
behind `statelessAuth`, and `/cluster/**` is not in that list
(`application.yml:183-202`). `StatelessAuthFilter` returns `false` from
`isAccessAllowed` unconditionally (`:49-54`) and then:
>
> ```java
> String tokenValue = getTokenValue(httpServletRequest);
> if (StringUtils.isBlank(tokenValue)) { ... unionFailResponse(...); return
false; } // :64-69
> ```
>
> i.e. 401 without a valid `X-Access-Token`.
>
> `ClusterDataChangedEventForwarder#forward` sends
>
> ```java
> restTemplate.postForEntity(url, payload, String.class) // :264
> ```
>
> with a bare `RestTemplate` and no headers at all. So the master answers
401, `RestTemplate` throws `HttpClientErrorException.Unauthorized`, which is a
`RuntimeException` and is swallowed at `:271-275` into a `LOG.warn(...
outcome=failed)`. `forward()` returns false, `DataChangedEventDispatcher` logs
"forward to master failed", and the push listeners are skipped **exactly as
they are on master today**. Net effect: #7312 is not fixed, with extra warning
logs.
>
> The existing node-to-node path in this repo already solves this and the
new one does not follow it: `ClusterForwardFilter#forwardRequest:133-136`
copies the incoming request headers into the forwarded request precisely so the
master's Shiro filter accepts it.
>
> The unit tests cannot catch this — `ClusterDataChangedEventForwarderTest`
mocks `RestTemplate`, and `ClusterDataChangedEventControllerTest` calls
`receive(...)` directly, bypassing the filter chain entirely.
>
> ### How I would fix it
> * **Carry the caller's token.** The event is published on the admin
request thread, so reading `X-Access-Token` off the current request
(`RequestContextHolder`) and setting it on the forwarded request reuses the
operator's own credentials, which the master already validates. This is the
same idea as `ClusterForwardFilter`. Events published off a request thread
(scheduled jobs, background sync) have no context, so keep an explicit fallback
for those rather than silently failing.
> * **Or a dedicated node-to-node credential** (shared secret header, or
mTLS) checked in the controller or a filter.
> * **Do not just add the path to `shiro.white-list`.** An unauthenticated
`POST /cluster/data-change-event` would let anyone who can reach the admin port
publish arbitrary `SelectorData` / `RuleData` / `PluginData` into every
connected gateway — a much bigger exposure than the existing `/websocket`
entry, whose comment in `application.yml:190-191` at least acknowledges the
trade-off for a passive read-only channel. This endpoint takes
attacker-controlled config content.
>
> Please also add a test that exercises the filter chain (or at minimum an
integration-style assertion that the forwarder sends the auth header), because
that is the gap that let this through.
>
> ### Non-blocking
> 1. `deserializeSource`'s `default: throw new IllegalStateException` will
surface as a 500 on the master. The switch is exhaustive for the current eight
`ConfigGroupEnum` values (I checked), but returning 400 with the unknown group
name would make adding a ninth value a debuggable rejection instead of a stack
trace.
> 2. Forwarding is synchronous on the admin request thread, so an
unreachable master adds the full connect/read timeout to the user's write API
call, with no retry. Worth at least documenting, or moving off the request
thread once delivery is authenticated.
> 3. `event.getSource().size()` in the three log statements (`:256`, `:268`,
`:274`) NPEs if the source is ever null.
> 4. `ClusterDataChangedEventPayload` has hand-written getters/setters while
the rest of the DTOs in the PR's neighbourhood use Lombok; not wrong, just
inconsistent.
Thanks for catching the auth gap — you are right, the forward as posted was
dead on arrival
behind Shiro. Fixed in 6c43af103 using your option 1:
- `ClusterDataChangedEventForwarder` now reads `X-Access-Token` off the
current request via
`RequestContextHolder` and sends it on the forwarded request, so the
master's
`StatelessAuthFilter` validates the operator's own credentials — same idea
as
`ClusterForwardFilter` copying request headers. `/cluster/**` stays behind
Shiro; the
white-list is untouched.
- Events published off a request thread (discovery upstream sync, scheduled
jobs) have no
token on the thread: the forwarder skips explicitly (`outcome=skipped`,
with the master
identity, group, type and size in the log) instead of sending a doomed
unauthenticated
request. Behavior for those events is unchanged from master; noted as a
known limitation
in the class javadoc.
- Tests: `forwardPostsPayloadToMasterUrlTest` now captures the `HttpEntity`
and asserts the
forwarded request carries the caller's token; added
`forwardWithoutRequestContextSkipsExplicitlyTest`.
Non-blocking points:
1. Unknown group/type now return 400 with the offending name (both for
`valueOf` and for a
group missing from `deserializeSource`'s switch), instead of a 500 stack.
2. Agreed on the sync cost — documented in the forwarder javadoc; happy to
move delivery off
the request thread once the auth approach is settled.
3. `sourceSize(event)` is null-safe now.
4. I checked before converting: `shenyu-admin` has no Lombok dependency at
all, and the DTOs
in `model/dto` (e.g. `ClusterMasterDTO`) use hand-written accessors, so I
kept the payload
consistent with the module it lives in.
Tests: forwarder/controller/dispatcher suites 26/26 green; full
`shenyu-admin` run 1523 tests,
1 skipped, BUILD SUCCESS; checkstyle clean.
--
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]
