From 9cfe724e6a188ea444c90ee00f2453da1b807bfa Mon Sep 17 00:00:00 2001 From: dinglei Date: Tue, 28 Nov 2023 10:04:17 +0800 Subject: [PATCH 1/3] Add validation in broker/namesrv configure updating command (#7584) * Add validation for keys in black list in mqadmin command. * Cancel validation for keys in black list in putKV command. --- .../processor/AdminBrokerProcessor.java | 25 ++++++++++++++-- .../processor/AdminBrokerProcessorTest.java | 12 +++++++- .../apache/rocketmq/common/BrokerConfig.java | 11 +++++++ .../rocketmq/common/ControllerConfig.java | 11 +++++++ .../common/namesrv/NamesrvConfig.java | 10 +++++++ .../processor/ControllerRequestProcessor.java | 27 +++++++++++++---- .../ControllerRequestProcessorTest.java | 23 +++++++++++++- .../processor/DefaultRequestProcessor.java | 30 +++++++++++++++++-- .../processor/RequestProcessorTest.java | 15 ++++++++-- 9 files changed, 149 insertions(+), 15 deletions(-) diff --git a/broker/src/main/java/org/apache/rocketmq/broker/processor/AdminBrokerProcessor.java b/broker/src/main/java/org/apache/rocketmq/broker/processor/AdminBrokerProcessor.java index 863b275d1..978c2e81d 100644 --- a/broker/src/main/java/org/apache/rocketmq/broker/processor/AdminBrokerProcessor.java +++ b/broker/src/main/java/org/apache/rocketmq/broker/processor/AdminBrokerProcessor.java @@ -25,6 +25,7 @@ import java.io.UnsupportedEncodingException; import java.net.UnknownHostException; import java.nio.charset.StandardCharsets; import java.util.ArrayList; +import java.util.Arrays; import java.util.HashMap; import java.util.HashSet; import java.util.Iterator; @@ -193,9 +194,19 @@ import static org.apache.rocketmq.remoting.protocol.RemotingCommand.buildErrorRe public class AdminBrokerProcessor implements NettyRequestProcessor { private static final Logger LOGGER = LoggerFactory.getLogger(LoggerName.BROKER_LOGGER_NAME); protected final BrokerController brokerController; + protected Set configBlackList = new HashSet<>(); public AdminBrokerProcessor(final BrokerController brokerController) { this.brokerController = brokerController; + initConfigBlackList(); + } + + private void initConfigBlackList() { + configBlackList.add("brokerConfigPath"); + configBlackList.add("rocketmqHome"); + configBlackList.add("configBlackList"); + String[] configArray = brokerController.getBrokerConfig().getConfigBlackList().split(";"); + configBlackList.addAll(Arrays.asList(configArray)); } @Override @@ -919,10 +930,9 @@ public class AdminBrokerProcessor implements NettyRequestProcessor { Properties properties = MixAll.string2Properties(bodyStr); if (properties != null) { LOGGER.info("updateBrokerConfig, new config: [{}] client: {} ", properties, callerAddress); - - if (properties.containsKey("brokerConfigPath")) { + if (validateBlackListConfigExist(properties)) { response.setCode(ResponseCode.NO_PERMISSION); - response.setRemark("Can not update config path"); + response.setRemark("Can not update config in black list."); return response; } @@ -2796,4 +2806,13 @@ public class AdminBrokerProcessor implements NettyRequestProcessor { } return false; } + + private boolean validateBlackListConfigExist(Properties properties) { + for (String blackConfig:configBlackList) { + if (properties.containsKey(blackConfig)) { + return true; + } + } + return false; + } } diff --git a/broker/src/test/java/org/apache/rocketmq/broker/processor/AdminBrokerProcessorTest.java b/broker/src/test/java/org/apache/rocketmq/broker/processor/AdminBrokerProcessorTest.java index ec252cece..c6b889bae 100644 --- a/broker/src/test/java/org/apache/rocketmq/broker/processor/AdminBrokerProcessorTest.java +++ b/broker/src/test/java/org/apache/rocketmq/broker/processor/AdminBrokerProcessorTest.java @@ -370,8 +370,18 @@ public class AdminBrokerProcessorTest { assertThat(response).isNotNull(); assertThat(response.getCode()).isEqualTo(ResponseCode.NO_PERMISSION); - assertThat(response.getRemark()).contains("Can not update config path"); + assertThat(response.getRemark()).contains("Can not update config in black list."); + //update disallowed value + properties.clear(); + properties.setProperty("configBlackList", "test;path"); + updateConfigRequest.setBody(MixAll.properties2String(properties).getBytes(StandardCharsets.UTF_8)); + + response = adminBrokerProcessor.processRequest(ctx, updateConfigRequest); + + assertThat(response).isNotNull(); + assertThat(response.getCode()).isEqualTo(ResponseCode.NO_PERMISSION); + assertThat(response.getRemark()).contains("Can not update config in black list."); } @Test diff --git a/common/src/main/java/org/apache/rocketmq/common/BrokerConfig.java b/common/src/main/java/org/apache/rocketmq/common/BrokerConfig.java index c186352d1..96e0f8e91 100644 --- a/common/src/main/java/org/apache/rocketmq/common/BrokerConfig.java +++ b/common/src/main/java/org/apache/rocketmq/common/BrokerConfig.java @@ -406,6 +406,17 @@ public class BrokerConfig extends BrokerIdentity { private int splitRegistrationSize = 800; + /** + * Config in this black list will be not allowed to update by command. + * Try to update this config black list by restart process. + * Try to update configures in black list by restart process. + */ + private String configBlackList = "configBlackList;brokerConfigPath"; + + public String getConfigBlackList() { + return configBlackList; + } + public long getMaxPopPollingSize() { return maxPopPollingSize; } diff --git a/common/src/main/java/org/apache/rocketmq/common/ControllerConfig.java b/common/src/main/java/org/apache/rocketmq/common/ControllerConfig.java index 1e9c80b22..55854cfd2 100644 --- a/common/src/main/java/org/apache/rocketmq/common/ControllerConfig.java +++ b/common/src/main/java/org/apache/rocketmq/common/ControllerConfig.java @@ -83,6 +83,17 @@ public class ControllerConfig { private boolean metricsInDelta = false; + /** + * Config in this black list will be not allowed to update by command. + * Try to update this config black list by restart process. + * Try to update configures in black list by restart process. + */ + private String configBlackList = "configBlackList;configStorePath"; + + public String getConfigBlackList() { + return configBlackList; + } + public String getRocketmqHome() { return rocketmqHome; } diff --git a/common/src/main/java/org/apache/rocketmq/common/namesrv/NamesrvConfig.java b/common/src/main/java/org/apache/rocketmq/common/namesrv/NamesrvConfig.java index 5b8a6dedb..b82d1b8f8 100644 --- a/common/src/main/java/org/apache/rocketmq/common/namesrv/NamesrvConfig.java +++ b/common/src/main/java/org/apache/rocketmq/common/namesrv/NamesrvConfig.java @@ -90,6 +90,16 @@ public class NamesrvConfig { * 2. This flag does not support static topic currently. */ private boolean deleteTopicWithBrokerRegistration = false; + /** + * Config in this black list will be not allowed to update by command. + * Try to update this config black list by restart process. + * Try to update configures in black list by restart process. + */ + private String configBlackList = "configBlackList;configStorePath;kvConfigPath"; + + public String getConfigBlackList() { + return configBlackList; + } public boolean isOrderMessageEnable() { return orderMessageEnable; diff --git a/controller/src/main/java/org/apache/rocketmq/controller/processor/ControllerRequestProcessor.java b/controller/src/main/java/org/apache/rocketmq/controller/processor/ControllerRequestProcessor.java index 93ecbbd9d..a8a3d2587 100644 --- a/controller/src/main/java/org/apache/rocketmq/controller/processor/ControllerRequestProcessor.java +++ b/controller/src/main/java/org/apache/rocketmq/controller/processor/ControllerRequestProcessor.java @@ -20,8 +20,11 @@ import com.google.common.base.Stopwatch; import io.netty.channel.ChannelHandlerContext; import io.opentelemetry.api.common.Attributes; import java.io.UnsupportedEncodingException; +import java.util.Arrays; +import java.util.HashSet; import java.util.List; import java.util.Properties; +import java.util.Set; import java.util.concurrent.CompletableFuture; import java.util.concurrent.TimeUnit; @@ -73,12 +76,20 @@ public class ControllerRequestProcessor implements NettyRequestProcessor { private static final int WAIT_TIMEOUT_OUT = 5; private final ControllerManager controllerManager; private final BrokerHeartbeatManager heartbeatManager; + protected Set configBlackList = new HashSet<>(); public ControllerRequestProcessor(final ControllerManager controllerManager) { this.controllerManager = controllerManager; this.heartbeatManager = controllerManager.getHeartbeatManager(); + initConfigBlackList(); + } + private void initConfigBlackList() { + configBlackList.add("configBlackList"); + configBlackList.add("configStorePath"); + configBlackList.add("rocketmqHome"); + String[] configArray = controllerManager.getControllerConfig().getConfigBlackList().split(";"); + configBlackList.addAll(Arrays.asList(configArray)); } - @Override public RemotingCommand processRequest(ChannelHandlerContext ctx, RemotingCommand request) throws Exception { if (ctx != null) { @@ -280,10 +291,9 @@ public class ControllerRequestProcessor implements NettyRequestProcessor { response.setRemark("string2Properties error"); return response; } - - if (properties.containsKey("configStorePath")) { + if (validateBlackListConfigExist(properties)) { response.setCode(ResponseCode.NO_PERMISSION); - response.setRemark("Can not update config path"); + response.setRemark("Can not update config in black list."); return response; } @@ -319,5 +329,12 @@ public class ControllerRequestProcessor implements NettyRequestProcessor { public boolean rejectRequest() { return false; } - + private boolean validateBlackListConfigExist(Properties properties) { + for (String blackConfig : configBlackList) { + if (properties.containsKey(blackConfig)) { + return true; + } + } + return false; + } } diff --git a/controller/src/test/java/org/apache/rocketmq/controller/ControllerRequestProcessorTest.java b/controller/src/test/java/org/apache/rocketmq/controller/ControllerRequestProcessorTest.java index ede6ca36a..46f86ad32 100644 --- a/controller/src/test/java/org/apache/rocketmq/controller/ControllerRequestProcessorTest.java +++ b/controller/src/test/java/org/apache/rocketmq/controller/ControllerRequestProcessorTest.java @@ -64,7 +64,28 @@ public class ControllerRequestProcessorTest { assertThat(response).isNotNull(); assertThat(response.getCode()).isEqualTo(ResponseCode.NO_PERMISSION); - assertThat(response.getRemark()).contains("Can not update config path"); + assertThat(response.getRemark()).contains("Can not update config in black list."); + // Update disallowed value + properties.clear(); + properties.setProperty("rocketmqHome", "test/path"); + updateConfigRequest.setBody(MixAll.properties2String(properties).getBytes(StandardCharsets.UTF_8)); + + response = controllerRequestProcessor.processRequest(null, updateConfigRequest); + + assertThat(response).isNotNull(); + assertThat(response.getCode()).isEqualTo(ResponseCode.NO_PERMISSION); + assertThat(response.getRemark()).contains("Can not update config in black list."); + + // Update disallowed value + properties.clear(); + properties.setProperty("configBlackList", "test;path"); + updateConfigRequest.setBody(MixAll.properties2String(properties).getBytes(StandardCharsets.UTF_8)); + + response = controllerRequestProcessor.processRequest(null, updateConfigRequest); + + assertThat(response).isNotNull(); + assertThat(response.getCode()).isEqualTo(ResponseCode.NO_PERMISSION); + assertThat(response.getRemark()).contains("Can not update config in black list."); } } diff --git a/namesrv/src/main/java/org/apache/rocketmq/namesrv/processor/DefaultRequestProcessor.java b/namesrv/src/main/java/org/apache/rocketmq/namesrv/processor/DefaultRequestProcessor.java index 485b95c42..2daa95b9b 100644 --- a/namesrv/src/main/java/org/apache/rocketmq/namesrv/processor/DefaultRequestProcessor.java +++ b/namesrv/src/main/java/org/apache/rocketmq/namesrv/processor/DefaultRequestProcessor.java @@ -18,8 +18,11 @@ package org.apache.rocketmq.namesrv.processor; import io.netty.channel.ChannelHandlerContext; import java.io.UnsupportedEncodingException; +import java.util.Arrays; +import java.util.HashSet; import java.util.List; import java.util.Properties; +import java.util.Set; import java.util.concurrent.atomic.AtomicLong; import org.apache.commons.lang3.StringUtils; import org.apache.rocketmq.common.MQVersion; @@ -71,8 +74,20 @@ public class DefaultRequestProcessor implements NettyRequestProcessor { protected final NamesrvController namesrvController; + protected Set configBlackList = new HashSet<>(); + public DefaultRequestProcessor(NamesrvController namesrvController) { this.namesrvController = namesrvController; + initConfigBlackList(); + } + + private void initConfigBlackList() { + configBlackList.add("configBlackList"); + configBlackList.add("configStorePath"); + configBlackList.add("kvConfigPath"); + configBlackList.add("rocketmqHome"); + String[] configArray = namesrvController.getNamesrvConfig().getConfigBlackList().split(";"); + configBlackList.addAll(Arrays.asList(configArray)); } @Override @@ -153,6 +168,7 @@ public class DefaultRequestProcessor implements NettyRequestProcessor { response.setRemark("namespace or key is null"); return response; } + this.namesrvController.getKvConfigManager().putKVConfig( requestHeader.getNamespace(), requestHeader.getKey(), @@ -623,10 +639,9 @@ public class DefaultRequestProcessor implements NettyRequestProcessor { response.setRemark("string2Properties error"); return response; } - - if (properties.containsKey("kvConfigPath") || properties.containsKey("configStorePath")) { + if (validateBlackListConfigExist(properties)) { response.setCode(ResponseCode.NO_PERMISSION); - response.setRemark("Can not update config path"); + response.setRemark("Can not update config in black list."); return response; } @@ -658,4 +673,13 @@ public class DefaultRequestProcessor implements NettyRequestProcessor { return response; } + private boolean validateBlackListConfigExist(Properties properties) { + for (String blackConfig : configBlackList) { + if (properties.containsKey(blackConfig)) { + return true; + } + } + return false; + } + } diff --git a/namesrv/src/test/java/org/apache/rocketmq/namesrv/processor/RequestProcessorTest.java b/namesrv/src/test/java/org/apache/rocketmq/namesrv/processor/RequestProcessorTest.java index 5bdf96d9d..2b2cf6294 100644 --- a/namesrv/src/test/java/org/apache/rocketmq/namesrv/processor/RequestProcessorTest.java +++ b/namesrv/src/test/java/org/apache/rocketmq/namesrv/processor/RequestProcessorTest.java @@ -203,7 +203,7 @@ public class RequestProcessorTest { assertThat(response).isNotNull(); assertThat(response.getCode()).isEqualTo(ResponseCode.NO_PERMISSION); - assertThat(response.getRemark()).contains("Can not update config path"); + assertThat(response.getRemark()).contains("Can not update config in black list."); //update disallowed values properties.clear(); @@ -214,7 +214,18 @@ public class RequestProcessorTest { assertThat(response).isNotNull(); assertThat(response.getCode()).isEqualTo(ResponseCode.NO_PERMISSION); - assertThat(response.getRemark()).contains("Can not update config path"); + assertThat(response.getRemark()).contains("Can not update config in black list"); + + //update disallowed values + properties.clear(); + properties.setProperty("configBlackList", "test;path"); + updateConfigRequest.setBody(MixAll.properties2String(properties).getBytes(StandardCharsets.UTF_8)); + + response = defaultRequestProcessor.processRequest(null, updateConfigRequest); + + assertThat(response).isNotNull(); + assertThat(response.getCode()).isEqualTo(ResponseCode.NO_PERMISSION); + assertThat(response.getRemark()).contains("Can not update config in black list"); } @Test -- 2.32.0.windows.2 From 430ee0a755daf867de31e37b12df417f64811b3a Mon Sep 17 00:00:00 2001 From: rongtong Date: Tue, 28 Nov 2023 16:11:14 +0800 Subject: [PATCH 2/3] Add validation in broker container configure updating command. (#7587) --- .../container/BrokerContainerConfig.java | 16 ++++++++ .../container/BrokerContainerProcessor.java | 40 +++++++++++++++++-- 2 files changed, 52 insertions(+), 4 deletions(-) diff --git a/container/src/main/java/org/apache/rocketmq/container/BrokerContainerConfig.java b/container/src/main/java/org/apache/rocketmq/container/BrokerContainerConfig.java index e03b10c34..03b4b263f 100644 --- a/container/src/main/java/org/apache/rocketmq/container/BrokerContainerConfig.java +++ b/container/src/main/java/org/apache/rocketmq/container/BrokerContainerConfig.java @@ -49,6 +49,14 @@ public class BrokerContainerConfig { */ private long updateNamesrvAddrInterval = 60 * 2 * 1000; + + /** + * Config in this black list will be not allowed to update by command. + * Try to update this config black list by restart process. + * Try to update configures in black list by restart process. + */ + private String configBlackList = "configBlackList;brokerConfigPaths"; + public String getRocketmqHome() { return rocketmqHome; } @@ -108,4 +116,12 @@ public class BrokerContainerConfig { public void setUpdateNamesrvAddrInterval(long updateNamesrvAddrInterval) { this.updateNamesrvAddrInterval = updateNamesrvAddrInterval; } + + public String getConfigBlackList() { + return configBlackList; + } + + public void setConfigBlackList(String configBlackList) { + this.configBlackList = configBlackList; + } } diff --git a/container/src/main/java/org/apache/rocketmq/container/BrokerContainerProcessor.java b/container/src/main/java/org/apache/rocketmq/container/BrokerContainerProcessor.java index 5b825fe81..5ced08257 100644 --- a/container/src/main/java/org/apache/rocketmq/container/BrokerContainerProcessor.java +++ b/container/src/main/java/org/apache/rocketmq/container/BrokerContainerProcessor.java @@ -19,6 +19,9 @@ package org.apache.rocketmq.container; import io.netty.channel.ChannelHandlerContext; import java.io.UnsupportedEncodingException; +import java.util.Arrays; +import java.util.HashSet; +import java.util.Set; import java.util.List; import java.util.Properties; import org.apache.rocketmq.broker.BrokerController; @@ -45,8 +48,19 @@ public class BrokerContainerProcessor implements NettyRequestProcessor { private final BrokerContainer brokerContainer; private List brokerBootHookList; + private final Set configBlackList = new HashSet<>(); + public BrokerContainerProcessor(BrokerContainer brokerContainer) { this.brokerContainer = brokerContainer; + initConfigBlackList(); + } + + private void initConfigBlackList() { + configBlackList.add("brokerConfigPaths"); + configBlackList.add("rocketmqHome"); + configBlackList.add("configBlackList"); + String[] configArray = brokerContainer.getBrokerContainerConfig().getConfigBlackList().split(";"); + configBlackList.addAll(Arrays.asList(configArray)); } @Override @@ -232,15 +246,24 @@ public class BrokerContainerProcessor implements NettyRequestProcessor { try { String bodyStr = new String(body, MixAll.DEFAULT_CHARSET); Properties properties = MixAll.string2Properties(bodyStr); - if (properties != null) { - LOGGER.info("updateSharedBrokerConfig, new config: [{}] client: {} ", properties, ctx.channel().remoteAddress()); - this.brokerContainer.getConfiguration().update(properties); - } else { + + if (properties == null) { LOGGER.error("string2Properties error"); response.setCode(ResponseCode.SYSTEM_ERROR); response.setRemark("string2Properties error"); return response; } + + if (validateBlackListConfigExist(properties)) { + response.setCode(ResponseCode.NO_PERMISSION); + response.setRemark("Can not update config in black list."); + return response; + } + + + LOGGER.info("updateBrokerContainerConfig, new config: [{}] client: {} ", properties, ctx.channel().remoteAddress()); + this.brokerContainer.getConfiguration().update(properties); + } catch (UnsupportedEncodingException e) { LOGGER.error("", e); response.setCode(ResponseCode.SYSTEM_ERROR); @@ -254,6 +277,15 @@ public class BrokerContainerProcessor implements NettyRequestProcessor { return response; } + private boolean validateBlackListConfigExist(Properties properties) { + for (String blackConfig : configBlackList) { + if (properties.containsKey(blackConfig)) { + return true; + } + } + return false; + } + private RemotingCommand getBrokerConfig(ChannelHandlerContext ctx, RemotingCommand request) { final RemotingCommand response = RemotingCommand.createResponseCommand(GetBrokerConfigResponseHeader.class); -- 2.32.0.windows.2 From a194e1eb9a12e08c43a0da65cd0a048ff849e04d Mon Sep 17 00:00:00 2001 From: dinglei Date: Tue, 28 Nov 2023 20:18:53 +0800 Subject: [PATCH 3/3] Add set method for config black list. (#7586) --- .../main/java/org/apache/rocketmq/common/BrokerConfig.java | 4 ++++ .../java/org/apache/rocketmq/common/ControllerConfig.java | 4 ++++ .../org/apache/rocketmq/common/namesrv/NamesrvConfig.java | 4 ++++ 3 files changed, 12 insertions(+) diff --git a/common/src/main/java/org/apache/rocketmq/common/BrokerConfig.java b/common/src/main/java/org/apache/rocketmq/common/BrokerConfig.java index 96e0f8e91..a4a553ad5 100644 --- a/common/src/main/java/org/apache/rocketmq/common/BrokerConfig.java +++ b/common/src/main/java/org/apache/rocketmq/common/BrokerConfig.java @@ -417,6 +417,10 @@ public class BrokerConfig extends BrokerIdentity { return configBlackList; } + public void setConfigBlackList(String configBlackList) { + this.configBlackList = configBlackList; + } + public long getMaxPopPollingSize() { return maxPopPollingSize; } diff --git a/common/src/main/java/org/apache/rocketmq/common/ControllerConfig.java b/common/src/main/java/org/apache/rocketmq/common/ControllerConfig.java index 55854cfd2..1364754a0 100644 --- a/common/src/main/java/org/apache/rocketmq/common/ControllerConfig.java +++ b/common/src/main/java/org/apache/rocketmq/common/ControllerConfig.java @@ -94,6 +94,10 @@ public class ControllerConfig { return configBlackList; } + public void setConfigBlackList(String configBlackList) { + this.configBlackList = configBlackList; + } + public String getRocketmqHome() { return rocketmqHome; } diff --git a/common/src/main/java/org/apache/rocketmq/common/namesrv/NamesrvConfig.java b/common/src/main/java/org/apache/rocketmq/common/namesrv/NamesrvConfig.java index b82d1b8f8..d1cdc7631 100644 --- a/common/src/main/java/org/apache/rocketmq/common/namesrv/NamesrvConfig.java +++ b/common/src/main/java/org/apache/rocketmq/common/namesrv/NamesrvConfig.java @@ -101,6 +101,10 @@ public class NamesrvConfig { return configBlackList; } + public void setConfigBlackList(String configBlackList) { + this.configBlackList = configBlackList; + } + public boolean isOrderMessageEnable() { return orderMessageEnable; } -- 2.32.0.windows.2