Add validation for keys in black list in mqadmin command.

apache · Nov 27, 2023 · 252d2d2 · 252d2d2
1 parent e955e43
commit 252d2d2
Show file tree

Hide file tree

Showing 9 changed files with 169 additions and 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
@@ -25,6 +25,7 @@
 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 @@
 public class AdminBrokerProcessor implements NettyRequestProcessor {
     private static final Logger LOGGER = LoggerFactory.getLogger(LoggerName.BROKER_LOGGER_NAME);
     protected final BrokerController brokerController;
+    protected Set<String> 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 @@ private synchronized RemotingCommand updateBrokerConfig(ChannelHandlerContext ct
                 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 @@ private boolean validateSlave(RemotingCommand response) {
         }
         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
@@ -370,8 +370,18 @@ public void testProcessRequest_UpdateConfigPath() throws RemotingCommandExceptio
 
         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
@@ -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
@@ -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
@@ -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/...er/src/main/java/org/apache/rocketmq/controller/processor/ControllerRequestProcessor.java b/...er/src/main/java/org/apache/rocketmq/controller/processor/ControllerRequestProcessor.java
@@ -20,8 +20,11 @@
 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<String> 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 @@ private RemotingCommand handleUpdateControllerConfig(ChannelHandlerContext ctx,
                 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 @@ private RemotingCommand handleGetControllerConfig(ChannelHandlerContext ctx, Rem
     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
@@ -64,7 +64,28 @@ public void testProcessRequest_UpdateConfigPath() throws Exception {
 
         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
@@ -18,8 +18,11 @@
 
 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<String> 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,11 @@ public RemotingCommand putKVConfig(ChannelHandlerContext ctx,
             response.setRemark("namespace or key is null");
             return response;
         }
+        if (validateBlackListConfigExist(requestHeader.getKey())) {
+            response.setCode(ResponseCode.NO_PERMISSION);
+            response.setRemark("Can not update config in black list.");
+            return response;
+        }
         this.namesrvController.getKvConfigManager().putKVConfig(
             requestHeader.getNamespace(),
             requestHeader.getKey(),
@@ -623,10 +643,9 @@ private RemotingCommand updateConfig(ChannelHandlerContext ctx, RemotingCommand
                 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 +677,16 @@ private RemotingCommand getConfig(ChannelHandlerContext ctx, RemotingCommand req
         return response;
     }
 
+    private boolean validateBlackListConfigExist(Properties properties) {
+        for (String blackConfig : configBlackList) {
+            if (properties.containsKey(blackConfig)) {
+                return true;
+            }
+        }
+        return false;
+    }
+
+    private boolean validateBlackListConfigExist(String key) {
+        return configBlackList.contains(key);
+    }
 }
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
@@ -110,6 +110,19 @@ public void testProcessRequest_PutKVConfig() throws RemotingCommandException {
 
         assertThat(namesrvController.getKvConfigManager().getKVConfig("namespace", "key"))
             .isEqualTo("value");
+
+        // use key in black list
+        RemotingCommand requestFailed = RemotingCommand.createRequestCommand(RequestCode.PUT_KV_CONFIG,
+                header);
+        requestFailed.addExtField("namespace", "namespace");
+        requestFailed.addExtField("key", "configBlackList");
+        requestFailed.addExtField("value", "value");
+
+        RemotingCommand responseFailed = defaultRequestProcessor.processRequest(null, requestFailed);
+
+        assertThat(responseFailed).isNotNull();
+        assertThat(responseFailed.getCode()).isEqualTo(ResponseCode.NO_PERMISSION);
+        assertThat(responseFailed.getRemark()).contains("Can not update config in black list.");
     }
 
     @Test
@@ -203,7 +216,7 @@ public void testProcessRequest_UpdateConfigPath() throws RemotingCommandExceptio
 
         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 +227,18 @@ public void testProcessRequest_UpdateConfigPath() throws RemotingCommandExceptio
 
         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