From 1e5e7ef44184ef6aa871f864a09ce3bff7724686 Mon Sep 17 00:00:00 2001 From: Tomek Mrugalski Date: Wed, 15 Mar 2017 17:04:57 +0100 Subject: [PATCH] [5151] get-config, write-config renamed to config-get, config-write --- src/bin/dhcp4/ctrl_dhcp4_srv.cc | 57 ++++++++++--------- src/bin/dhcp4/ctrl_dhcp4_srv.h | 8 +-- .../dhcp4/tests/ctrl_dhcp4_srv_unittest.cc | 38 +++++++------ 3 files changed, 57 insertions(+), 46 deletions(-) diff --git a/src/bin/dhcp4/ctrl_dhcp4_srv.cc b/src/bin/dhcp4/ctrl_dhcp4_srv.cc index 055b6493b0..2accc02b67 100644 --- a/src/bin/dhcp4/ctrl_dhcp4_srv.cc +++ b/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(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 diff --git a/src/bin/dhcp4/ctrl_dhcp4_srv.h b/src/bin/dhcp4/ctrl_dhcp4_srv.h index db1e4379ec..34b30b4977 100644 --- a/src/bin/dhcp4/ctrl_dhcp4_srv.h +++ b/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 diff --git a/src/bin/dhcp4/tests/ctrl_dhcp4_srv_unittest.cc b/src/bin/dhcp4/tests/ctrl_dhcp4_srv_unittest.cc index 06780c6949..26fc9d444b 100644 --- a/src/bin/dhcp4/tests/ctrl_dhcp4_srv_unittest.cc +++ b/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."); } -- 2.47.3