RockteMQ-AI commented on PR #11179:
URL: https://github.com/apache/rocketmq/pull/11179#issuecomment-5741516622
Verified the claims against the codebase (base is newer than this clone's
`develop`, so I inspected the nearest available commit plus the diff). The
vulnerability analysis is confirmed:
`DefaultAuthorizationContextBuilder.build(ChannelHandlerContext,
RemotingCommand)` sets the subject directly from `extFields[AccessKey]` with no
verification, and `UserAuthorizationHandler` short-circuits `SUPER` users to
allow.
---
## Review
Overall the fix is correct in intent and minimal. `AuthConfig.validate()` is
the right predicate: authorization derives identity from an unverified
`AccessKey`, so authZ without authN is not a meaningful configuration. However,
placement, coverage, and the tests need work.
### 1. Guard runs too late in broker startup — `BrokerController.java:1147`
`initialRequestPipeline()` is the last step of `recoverAndInitService()`,
after `messageStore.load()` (commit-log recovery can take minutes),
`initializeRemotingServer()`, `initializeResources()` and
`initializeScheduledTasks()`. A misconfigured broker does all that work, then
throws `IllegalArgumentException` out of `initialize()`.
`BrokerStartup.createBrokerController` catches `Throwable` with
`e.printStackTrace(); System.exit(-1)` — and unlike the `!initResult` path, it
skips `controller.shutdown()`, so executors/threads created moments earlier are
abandoned to the exiting JVM. For a pure configuration error this is poor
operator UX. Suggest validating where the config is bound, e.g. in
`BrokerStartup.buildBrokerController()` immediately after `properties2Object`
(clear log + deliberate exit), or at the top of `BrokerController.initialize()`
before store load.
### 2. Coverage: only the broker Remoting path is guarded
The proxy owns a separate `AuthConfig`
(`ConfigurationManager.getAuthConfig()`) and wires the *same*
`AuthorizationPipeline`/context builder for gRPC
(`GrpcMessagingApplication.create`) and Remoting (`RemotingProtocolServer`).
The guard as written does nothing for those endpoints. If an equivalent check
already exists for the "gRPC admin surface", please confirm the proxy Remoting
path is covered too; otherwise moving `validate()` into
`AuthorizationPipeline`'s constructor (all three components construct it during
startup) would close all paths with one change.
### 3. Whitelist re-opens the hole even with authN enabled —
`AuthConfig.java` (`isAuthenticationRequired`)
Whitelisted RPCs skip authentication, but `AuthorizationPipeline` does not
consult the whitelist and still builds the subject from
`command.getExtFields()`. So `authenticationWhitelist=<rpc>` + authZ reproduces
exactly the reported impersonation. The durable fix is authorization-layer:
derive the subject only from a verified authentication result (channel
attribute/`AuthenticationContext` set after successful verification), never
from raw extFields, treating a null subject as anonymous-deny. Consider
tracking this as a follow-up; the startup guard is at best defense-in-depth.
### 4. Tests do not exercise the fix — `AccessKeySpoofingReproTest.java`
The repro test calls
`DefaultAuthorizationContextBuilder`/`AuthorizationFactory` directly, bypassing
`validate()` and `BrokerController` entirely. It passes identically with and
without this PR, so it provides zero regression protection — and it permanently
encodes "forged AccessKey is authorized" as expected behavior. Also
`Assert.assertTrue("forged access key was authorized", true)` (~L131) is a
tautology. Please add a test that hits the guard (BrokerController-level, or at
minimum that `initialRequestPipeline` rejects the bad combination) and either
delete the repro or clearly mark it as a characterization test of the unguarded
lower layers.
### 5. Test hygiene (minor)
- `Files.createTempDirectory("rmq-auth-repro")` (~L88) creates a new RocksDB
store per test; `tearDown` only calls `shutdown()`, so temp dirs accumulate
across runs. Use `TemporaryFolder` or delete in `tearDown`.
- `MockitoJUnitRunner.Silent` hides unnecessary stubbing; the ~90 lines of
`ChannelId`/`Attribute` stubs could reuse `AuthTestHelper`/existing test
scaffolding.
- `AuthConfigTest` mixes `org.junit.Assert` with the file's AssertJ style;
`assertThatThrownBy(...).isInstanceOf(...).hasMessageContaining(...)` would
also lock in the message.
### Not an issue
No performance impact (O(1) startup check), no API/protocol break
(`validate()` is additive), and `IllegalArgumentException` is caught before the
remoting server starts accepting traffic, so there is no window where a broker
serves with the unsafe config. The four-way truth-table tests in
`AuthConfigTest` are appropriate.
**Verdict:** security rationale is sound, but please move the check earlier
(Finding 1), confirm proxy coverage (Finding 2), and add a test that actually
fails without the guard (Finding 4) before merging.
--
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]
