[5151] get-config, write-config renamed to config-get, config-write

src/bin/dhcp4/ctrl_dhcp4_srv.cc

@@ -67,7 +67,7 @@ ControlledDhcpv4Srv::commandConfigReloadHandler(const string&,
 }
 
 ConstElementPtr
-ControlledDhcpv4Srv::commandGetConfigHandler(const string&,
+ControlledDhcpv4Srv::commandConfigGetHandler(const string&,
                                              ConstElementPtr /*args*/) {
     ConstElementPtr config = CfgMgr::instance().getCurrentCfg()->toElement();
 
@@ -75,8 +75,8 @@ ControlledDhcpv4Srv::commandGetConfigHandler(const string&,
 }
 
 ConstElementPtr
-ControlledDhcpv4Srv::commandWriteConfigHandler(const string&,
-                                             ConstElementPtr args) {
+ControlledDhcpv4Srv::commandConfigWriteHandler(const string&,
+                                               ConstElementPtr args) {
     ConstElementPtr config = CfgMgr::instance().getCurrentCfg()->toElement();
 
     string filename;
@@ -124,6 +124,7 @@ ControlledDhcpv4Srv::commandWriteConfigHandler(const string&,
                              "Absolute path in filename is not allowed."));
     }
 
+    // Ok, it's time to write the file.
     size_t size = 0;
     try {
         size = writeConfigFile(filename);
@@ -136,7 +137,7 @@ ControlledDhcpv4Srv::commandWriteConfigHandler(const string&,
                              + filename));
     }
 
-    // Ok, it's time to return the successful response
+    // Ok, it's time to return the successful response.
     ElementPtr params = Element::createMap();
     params->set("size", Element::create(static_cast<long long>(size)));
     params->set("filename", Element::create(filename));
@@ -260,13 +261,15 @@ ControlledDhcpv4Srv::processCommand(const string& command,
         } else if (command == "set-config") {
             return (srv->commandSetConfigHandler(command, args));
 
-        } else if (command == "get-config") {
-            return (srv->commandGetConfigHandler(command, args));
+        } else if (command == "config-get") {
+            return (srv->commandConfigGetHandler(command, args));
 
         } else if (command == "leases-reclaim") {
             return (srv->commandLeasesReclaimHandler(command, args));
-        } else if (command == "write-config") {
-            return (srv->commandWriteConfigHandler(command, args));
+
+        } else if (command == "config-write") {
+            return (srv->commandConfigWriteHandler(command, args));
+
         }
         ConstElementPtr answer = isc::config::createAnswer(1,
                                  "Unrecognized command:" + command);
@@ -395,23 +398,27 @@ ControlledDhcpv4Srv::ControlledDhcpv4Srv(uint16_t port /*= DHCP4_SERVER_PORT*/)
     }
     server_ = this; // remember this instance for later use in handlers
 
-    // Register supported commands in CommandMgr
-    CommandMgr::instance().registerCommand("shutdown",
-        boost::bind(&ControlledDhcpv4Srv::commandShutdownHandler, this, _1, _2));
+    // These are the commands always supported by the DHCPv4 server.
+    // Please keep the list in alphabetic order.
+    CommandMgr::instance().registerCommand("config-get",
+        boost::bind(&ControlledDhcpv4Srv::commandConfigGetHandler, this, _1, _2));
 
     /// @todo: register config-reload (see CtrlDhcpv4Srv::commandConfigReloadHandler)
 
+    CommandMgr::instance().registerCommand("config-write",
+        boost::bind(&ControlledDhcpv4Srv::commandConfigWriteHandler, this, _1, _2));
+
     CommandMgr::instance().registerCommand("libreload",
         boost::bind(&ControlledDhcpv4Srv::commandLibReloadHandler, this, _1, _2));
 
+    CommandMgr::instance().registerCommand("leases-reclaim",
+        boost::bind(&ControlledDhcpv4Srv::commandLeasesReclaimHandler, this, _1, _2));
+
     CommandMgr::instance().registerCommand("set-config",
         boost::bind(&ControlledDhcpv4Srv::commandSetConfigHandler, this, _1, _2));
 
-    CommandMgr::instance().registerCommand("get-config",
-        boost::bind(&ControlledDhcpv4Srv::commandGetConfigHandler, this, _1, _2));
-
-    CommandMgr::instance().registerCommand("leases-reclaim",
-        boost::bind(&ControlledDhcpv4Srv::commandLeasesReclaimHandler, this, _1, _2));
+    CommandMgr::instance().registerCommand("shutdown",
+        boost::bind(&ControlledDhcpv4Srv::commandShutdownHandler, this, _1, _2));
 
     // Register statistic related commands
     CommandMgr::instance().registerCommand("statistic-get",
@@ -432,8 +439,6 @@ ControlledDhcpv4Srv::ControlledDhcpv4Srv(uint16_t port /*= DHCP4_SERVER_PORT*/)
     CommandMgr::instance().registerCommand("statistic-remove-all",
         boost::bind(&StatsMgr::statisticRemoveAllHandler, _1, _2));
 
-    CommandMgr::instance().registerCommand("write-config",
-        boost::bind(&ControlledDhcpv4Srv::commandWriteConfigHandler, this, _1, _2));
 }
 
 void ControlledDhcpv4Srv::shutdown() {
@@ -453,19 +458,19 @@ ControlledDhcpv4Srv::~ControlledDhcpv4Srv() {
         // Close the command socket (if it exists).
         CommandMgr::instance().closeCommandSocket();
 
-        // Deregister any registered commands
-        CommandMgr::instance().deregisterCommand("get-config");
-        CommandMgr::instance().deregisterCommand("shutdown");
+        // Deregister any registered commands (please keep in alphabetic order)
+        CommandMgr::instance().deregisterCommand("config-get");
+        CommandMgr::instance().deregisterCommand("config-write");
+        CommandMgr::instance().deregisterCommand("leases-reclaim");
         CommandMgr::instance().deregisterCommand("libreload");
         CommandMgr::instance().deregisterCommand("set-config");
-        CommandMgr::instance().deregisterCommand("leases-reclaim");
+        CommandMgr::instance().deregisterCommand("shutdown");
         CommandMgr::instance().deregisterCommand("statistic-get");
-        CommandMgr::instance().deregisterCommand("statistic-reset");
-        CommandMgr::instance().deregisterCommand("statistic-remove");
         CommandMgr::instance().deregisterCommand("statistic-get-all");
-        CommandMgr::instance().deregisterCommand("statistic-reset-all");
+        CommandMgr::instance().deregisterCommand("statistic-remove");
         CommandMgr::instance().deregisterCommand("statistic-remove-all");
-        CommandMgr::instance().deregisterCommand("write-config");
+        CommandMgr::instance().deregisterCommand("statistic-reset");
+        CommandMgr::instance().deregisterCommand("statistic-reset-all");
 
     } catch (...) {
         // Don't want to throw exceptions from the destructor. The server

src/bin/dhcp4/ctrl_dhcp4_srv.h

@@ -153,7 +153,7 @@ private:
     /// @param args (ignored)
     /// @return current configuration wrapped in a response
     isc::data::ConstElementPtr
-    commandGetConfigHandler(const std::string& command,
+    commandConfigGetHandler(const std::string& command,
                             isc::data::ConstElementPtr args);
 
     /// @brief handler for processing 'write-config' command
@@ -162,15 +162,15 @@ private:
     /// current configuration to disk. This command takes one optional
     /// parameter called filename. If specified, the current configuration
     /// will be written to that file. If not specified, the file used during
-    /// Kea start-up will be used. The filename must be within the
-    /// {prefix} directory specified during Kea compilation. This is
+    /// Kea start-up will be used. To avoid any exploits, the path is
+    /// always relative and .. is not allowed in the filename. This is
     /// a security measure against exploiting file writes remotely.
     ///
     /// @param command (ignored)
     /// @param args may contain optional string argument filename
     /// @return status of the configuration file write
     isc::data::ConstElementPtr
-    commandWriteConfigHandler(const std::string& command,
+    commandConfigWriteHandler(const std::string& command,
                               isc::data::ConstElementPtr args);
 
     /// @brief handler for processing 'set-config' command

src/bin/dhcp4/tests/ctrl_dhcp4_srv_unittest.cc

@@ -234,7 +234,7 @@ public:
     /// @param exp_status expected status (0 success, 1 failure)
     /// @param exp_txt for success cases this defines the expected filename,
     ///                for failure cases this defines the expected error message
-    void checkWriteConfig(const std::string& response_txt, int exp_status,
+    void checkConfigWrite(const std::string& response_txt, int exp_status,
                           const std::string& exp_txt = "") {
 
         cout << "#### response=" << response_txt << endl;
@@ -724,7 +724,8 @@ TEST_F(CtrlChannelDhcpv4SrvTest, listCommands) {
     EXPECT_NO_THROW(rsp = Element::fromJSON(response));
 
     // We expect the server to report at least the following commands:
-    checkListCommands(rsp, "get-config");
+    checkListCommands(rsp, "config-get");
+    checkListCommands(rsp, "config-write");
     checkListCommands(rsp, "list-commands");
     checkListCommands(rsp, "leases-reclaim");
     checkListCommands(rsp, "libreload");
@@ -736,17 +737,16 @@ TEST_F(CtrlChannelDhcpv4SrvTest, listCommands) {
     checkListCommands(rsp, "statistic-remove-all");
     checkListCommands(rsp, "statistic-reset");
     checkListCommands(rsp, "statistic-reset-all");
-    checkListCommands(rsp, "write-config");
 }
 
 // Tests if the server returns its configuration using get-config.
 // Note there are separate tests that verify if toElement() called by the
 // get-config handler are actually converting the configuration correctly.
-TEST_F(CtrlChannelDhcpv4SrvTest, getConfig) {
+TEST_F(CtrlChannelDhcpv4SrvTest, configGet) {
     createUnixChannelServer();
     std::string response;
 
-    sendUnixCommand("{ \"command\": \"get-config\" }", response);
+    sendUnixCommand("{ \"command\": \"config-get\" }", response);
     ConstElementPtr rsp;
 
     // The response should be a valid JSON.
@@ -763,7 +763,7 @@ TEST_F(CtrlChannelDhcpv4SrvTest, getConfig) {
     EXPECT_TRUE(cfg->get("Dhcp4"));
 }
 
-
+// Tests if config-write can be called without any parameters.
 TEST_F(CtrlChannelDhcpv4SrvTest, writeConfigNoFilename) {
     createUnixChannelServer();
     std::string response;
@@ -773,50 +773,56 @@ TEST_F(CtrlChannelDhcpv4SrvTest, writeConfigNoFilename) {
 
     // If the filename is not explicitly specified, the name used
     // in -c command line switch is used.
-    sendUnixCommand("{ \"command\": \"write-config\" }", response);
+    sendUnixCommand("{ \"command\": \"config-write\" }", response);
 
-    checkWriteConfig(response, CONTROL_RESULT_SUCCESS, "test1.json");
+    checkConfigWrite(response, CONTROL_RESULT_SUCCESS, "test1.json");
     ::remove("test1.json");
 }
 
+// Tests if config-write can be called with a valid filename as parameter.
 TEST_F(CtrlChannelDhcpv4SrvTest, writeConfigFilename) {
     createUnixChannelServer();
     std::string response;
 
-    sendUnixCommand("{ \"command\": \"write-config\", "
+    sendUnixCommand("{ \"command\": \"config-write\", "
                     "\"arguments\": { \"filename\": \"test2.json\" } }", response);
-    checkWriteConfig(response, CONTROL_RESULT_SUCCESS, "test2.json");
+    checkConfigWrite(response, CONTROL_RESULT_SUCCESS, "test2.json");
     ::remove("test2.json");
 }
 
+// Tests if config-write rejects invalid filename (a one that tries to escape
+// the current directory).
 TEST_F(CtrlChannelDhcpv4SrvTest, writeConfigInvalidJailEscape) {
     createUnixChannelServer();
     std::string response;
 
-    sendUnixCommand("{ \"command\": \"write-config\", \"arguments\": "
+    sendUnixCommand("{ \"command\": \"config-write\", \"arguments\": "
                     "{ \"filename\": \"../test3.json\" } }", response);
-    checkWriteConfig(response, CONTROL_RESULT_ERROR,
+    checkConfigWrite(response, CONTROL_RESULT_ERROR,
                      "Using '..' in filename is not allowed.");
 }
 
+// Tests if config-write rejects invalid filename (absolute paths are not allowed)
 TEST_F(CtrlChannelDhcpv4SrvTest, writeConfigInvalidAbsPath) {
     createUnixChannelServer();
     std::string response;
 
-    sendUnixCommand("{ \"command\": \"write-config\", \"arguments\": "
+    sendUnixCommand("{ \"command\": \"config-write\", \"arguments\": "
                     "{ \"filename\": \"/tmp/test4.json\" } }", response);
-    checkWriteConfig(response, CONTROL_RESULT_ERROR,
+    checkConfigWrite(response, CONTROL_RESULT_ERROR,
                      "Absolute path in filename is not allowed.");
 }
 
+// Tests if config-write rejects invalid filename (one with backslashes, which may
+// lead to some other tricks)
 TEST_F(CtrlChannelDhcpv4SrvTest, writeConfigInvalidEscape) {
     createUnixChannelServer();
     std::string response;
 
     // This will be converted to foo(single backslash)test5.json
-    sendUnixCommand("{ \"command\": \"write-config\", \"arguments\": "
+    sendUnixCommand("{ \"command\": \"config-write\", \"arguments\": "
                     "{ \"filename\": \"foo\\\\test5.json\" } }", response);
-    checkWriteConfig(response, CONTROL_RESULT_ERROR,
+    checkConfigWrite(response, CONTROL_RESULT_ERROR,
                      "Using \\ in filename is not allowed.");
 }