RockteMQ-AI commented on code in PR #10997:
URL: https://github.com/apache/rocketmq/pull/10997#discussion_r3911834568
##########
auth/src/main/java/org/apache/rocketmq/auth/config/AuthConfig.java:
##########
@@ -136,11 +183,12 @@ public void setAuthenticationStrategy(String
authenticationStrategy) {
}
public String getAuthenticationWhitelist() {
- return authenticationWhitelist;
+ WhitelistSnapshot snapshot = authenticationWhitelist;
Review Comment:
Nice use of `WhitelistSnapshot` as an immutable value object for thread-safe
whitelist updates. The `volatile` reference swap ensures visibility across
threads without locking.
##########
broker/src/main/java/org/apache/rocketmq/broker/processor/AdminBrokerProcessor.java:
##########
@@ -3031,6 +3063,12 @@ private RemotingCommand
resumeCheckHalfMessage(ChannelHandlerContext ctx,
selectMappedBufferResult = this.brokerController.getMessageStore()
.selectOneMessageByOffset(messageId.getOffset());
MessageExt msg =
MessageDecoder.decode(selectMappedBufferResult.getByteBuffer(), true, false);
+ if (!Objects.equals(requestHeader.getTopic(),
+ msg.getUserProperty(MessageConst.PROPERTY_REAL_TOPIC))) {
+ response.setCode(ResponseCode.NO_PERMISSION);
+ response.setRemark("The topic does not match the transaction
message");
Review Comment:
The topic mismatch check in `resumeCheckHalfMessage` returns `NO_PERMISSION`
— consider whether `SYSTEM_ERROR` or a more specific error code would be more
appropriate, since this is a topic validation failure rather than a permission
denial.
##########
broker/src/main/java/org/apache/rocketmq/broker/processor/AdminBrokerProcessor.java:
##########
@@ -3524,6 +3565,27 @@ private boolean isNotSuperUserLogin(RemotingCommand
request) {
.isSuperUser(accessKey).join();
}
+ private boolean canReadUserPassword(RemotingCommand request, String
username) {
+ if (this.brokerController.getAuthConfig() == null
+ || !this.brokerController.getAuthConfig()
+ .isAuthenticationRequired(String.valueOf(request.getCode()))) {
+ return false;
+ }
+ String accessKey = getAccessKey(request);
+ if (StringUtils.isEmpty(accessKey)) {
+ return false;
+ }
+ if (StringUtils.equals(accessKey, username)) {
Review Comment:
The `canReadUserPassword` logic is well-designed: it requires authentication
to be enabled, checks self-access first, then falls back to super-user check.
Good defense-in-depth.
##########
proxy/src/main/java/org/apache/rocketmq/proxy/grpc/interceptor/HeaderInterceptor.java:
##########
@@ -58,12 +61,12 @@ public <R, W> ServerCall.Listener<R> interceptCall(
Metadata.Key<String> headerKey
= Metadata.Key.of(key.toString(),
Metadata.ASCII_STRING_MARSHALLER);
Review Comment:
Switching from `putHeaderIfNotExist` to `putHeader` (overwrite) is the
correct fix — ensures the proxy authoritative source always wins over
client-supplied values.
--
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]
