[ISSUE #11178] Refuse to start when authorization is enabled without authentication

[R0CKing666]

Which Issue(s) This PR Fixes

• Fixes [Bug] Remoting authorization trusts the client-supplied AccessKey when authorization is enabled without authentication #11178

Brief Description

The Remoting (TCP) authorization path derives the caller identity from the client-supplied AccessKey in the request extFields without any signature verification. authorizationEnabled and authenticationEnabled are independent switches, so a broker/proxy started with authorization on but authentication off would let any client impersonate a known user (including a SUPER user) simply by claiming its AccessKey.

This change makes startup fail-closed and covers every component that wires an authorization pipeline:

• AuthConfig#validate() rejects authorizationEnabled && !authenticationEnabled.
• Broker: BrokerController.initialize() validates before the expensive message store load.
• Proxy: the AuthorizationPipeline constructors for both the gRPC and Remoting protocol servers validate on construction.

This mirrors the existing fail-closed guards in ProxyAdminAuthInterceptor for the gRPC admin surface.

Note: this is a config-level (defense-in-depth) guard. A request whose RPC is in the authentication whitelist still skips authentication while authorization derives the subject from the raw AccessKey; a durable fix at the authorization layer (derive the subject only from a verified authentication result) is tracked as a follow-up.

How Did You Test This Change?

• AuthConfigTest (6 tests): valid/invalid enable combinations.
• Proxy AuthorizationPipelineTest for gRPC and Remoting (6 tests): the pipeline constructor rejects authorizationEnabled without authenticationEnabled, and existing authorization behavior is unchanged.
• AccessKeySpoofingReproTest (3 tests): characterization test of the unguarded lower layers (forged AccessKey is authorized when authentication is disabled; unknown AccessKey is denied; enabling authentication rejects the unsigned request).
• Built with mvn -pl proxy -am install -DskipTests.

[RockteMQ-AI]

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.

[RockteMQ-AI]

Summary

This PR contains 412 lines of changes. A detailed code review is recommended.

---

Automated review by github-manager-bot

[RockteMQ-AI]

Summary

Security fix that prevents enabling authorization without authentication. The AuthConfig.validate() method enforces this invariant at startup (called from BrokerController.initialize() and proxy AuthorizationPipeline constructors), blocking the misconfiguration before any vulnerable code path can be reached.

The reproduction test (AccessKeySpoofingReproTest) is excellent — it characterizes the vulnerability in the unguarded lower layers while making clear that the fix is the startup guard. The validation tests cover all four combinations of auth flags.

Well-documented, well-tested, addresses a real identity spoofing risk.

LGTM.

---

Automated review by github-manager-bot