Repository navigation
[ISSUE #11470] fix(proxy): fail closed on admin methods missing an ACL mapping - #11471
Open
Felix-Gong wants to merge 1 commit into
Open
Felix-Gong wants to merge 1 commit into
Felix-Gong wants to merge 1 commit into
Conversation
… an ACL mapping ProxyAdminAuthInterceptor treated an absent METHOD_PERMISSIONS entry as 'no check needed': the authz branch required resourceAction != null, so a served method without a mapping ran straight through to the handler with authentication only, even with the cluster authorization switch on and even in fail-closed mode. The admin server serves only the 16 mapped Admin RPCs today, but the surface grows through rocketmq-apis (three Admin expansions in recent commits), so the next RPC added without an entry here would be an unauthorized call path including mutations. Deny unmapped methods with PERMISSION_DENIED whenever authorization is enabled, and add a completeness test that walks AdminGrpc.getServiceDescriptor() so CI fails on map drift the moment a new RPC lands. Red/green: unmappedMethodIsDeniedWhenAuthorizationEnabled was verified to fail on the old pass-through code by stashing the fix. Proxy module: 455 tests green, 9 in ProxyAdminAuthInterceptorTest, JDK 11.
RockteMQ-AI
approved these changes
Oct 20, 2026
RockteMQ-AI
left a comment
RockteMQ-AI
left a comment
Contributor
There was a problem hiding this comment.
Review by github-manager-bot
Summary
Security hardening: fail-closed authorization for admin gRPC methods that lack an ACL mapping in ProxyAdminAuthInterceptor.
Findings
- [Info]
ProxyAdminAuthInterceptor.java:196-202— Clean restructuring. The oldresourceAction != null && authzEnabledshort-circuited the entire authz block when a method had no mapping. The new structure correctly enters the authz block first, then denies unmapped methods withPERMISSION_DENIED. This is the right fail-closed pattern. - [Info]
ProxyAdminAuthInterceptorTest.java— Two-layer test coverage is solid:everyServedAdminMethodHasAnAclMapping()acts as a CI drift guard — fails the moment a new proto RPC lands without an ACL entry.unmappedMethodIsDeniedWhenAuthorizationEnabled()verifies the runtime deny behavior with proper mockito verification (never().startCall()+PERMISSION_DENIEDassertion).
- [Info] PR description is excellent — clearly documents the vulnerability window, the fix rationale, red/green verification, and coordination note with PR #11363.
Suggestions
- Minor: the
@SuppressWarnings("unchecked")onunmappedMethodIsDeniedWhenAuthorizationEnabledappears unnecessary — no unchecked operations are visible in the test body. Consider removing it to keep the annotation surface minimal. (Non-blocking.)
Verdict
Well-crafted security fix. The fail-closed approach is the correct pattern for an expanding admin API surface. LGTM.
Automated review by github-manager-bot
RockteMQ-AI
approved these changes
Oct 21, 2026
RockteMQ-AI
left a comment
RockteMQ-AI
left a comment
Contributor
There was a problem hiding this comment.
Review by github-manager-bot
Summary
Fail-closed security fix: when authorization is enabled, admin methods without an ACL mapping are now denied with PERMISSION_DENIED instead of being served with authentication only. Adds a drift guard test that walks AdminGrpc.getServiceDescriptor() and fails CI if a new RPC lands without an entry in METHOD_PERMISSIONS.
Findings
- [Critical → Fixed] The original code (
resourceAction != null && authzEnabled) meant an unmapped method would skip the ACL branch entirely when authorization was enabled. This is a fail-open bug by construction — any future RPC added to the proto without a corresponding map entry would be served without authorization. The fix correctly inverts the logic: ifauthzEnabledis on, a missing mapping is now a hard deny. - [Info] The drift guard test
everyServedAdminMethodHasAnAclMappingis excellent — it pins the proto-to-ACL mapping invariant at CI time, so the next RPC expansion cannot silently introduce an unauthorized path. - [Info] The
PERMISSION_DENIEDstatus with a descriptive message pointing at the map is helpful for developers adding new RPCs — they'll know exactly what to do. - [Info] Open mode (cluster auth fully off) is correctly untouched by this change.
Suggestions
- Consider adding a comment in
METHOD_PERMISSIONS(or a separate doc) listing the process for adding a new admin RPC: add proto method → add map entry → add test. This makes the drift guard self-documenting.
This is an important security hardening. The fix is minimal, correct, and the drift guard test ensures the invariant holds going forward.
Automated review by github-manager-bot
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #11470
ProxyAdminAuthInterceptorskipped authorization for any method missing fromMETHOD_PERMISSIONS— the branch wasresourceAction != null && authzEnabled, so an unmapped served method ran with authentication only, in both normal and fail-closed (grpcAdminServerAuthEnable) configurations. Nothing is unmapped on develop today, but the Admin surface grows through rocketmq-apis (three recent expansions) and nothing pins the proto against the hand-written map, so the next RPC added without an entry would be an unauthorized path by construction.Two changes, one theme:
authorizationEnabledis on, a method without aproxy.admin.*mapping is now closed withPERMISSION_DENIEDinstead of being served. Open mode (cluster auth fully off) is untouched.everyServedAdminMethodHasAnAclMappingwalksAdminGrpc.getServiceDescriptor()and asserts every served RPC resolves to a resource/action, so CI fails the moment a new proto RPC lands without someone deciding its ACL.Red/green:
unmappedMethodIsDeniedWhenAuthorizationEnabledpasses on the current code (verified by stashing the fix — it assertsstartCallnever happens and the status isPERMISSION_DENIED).ProxyAdminAuthInterceptorTest9/9, fullproxymodule 455 tests green on JDK 11.Coordination: PR #11363 (for #11362) changes
resolveSourceIpin the same two files — different concern (which address is evaluated vs whether an authorization decision exists at all). The hunks are adjacent where it updates theresolveSourceIp(call)call site inside the block this PR restructures; a trivial rebase either way, happy to rebase once it lands.