From: Razvan Becheriu Date: Fri, 16 May 2025 16:29:45 +0000 (+0300) Subject: backport #3831 to 2_4 X-Git-Tag: Kea-2.4.2~11 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=309936331f39db5b4cf18f81e50cd8c816d8e445;p=thirdparty%2Fkea.git backport #3831 to 2_4 --- diff --git a/doc/sphinx/arm/ctrl-channel.rst b/doc/sphinx/arm/ctrl-channel.rst index d1d1318431..abcafba875 100644 --- a/doc/sphinx/arm/ctrl-channel.rst +++ b/doc/sphinx/arm/ctrl-channel.rst @@ -511,6 +511,12 @@ An example command invocation looks like this: } } +.. note:: + + As of Kea 2.4.2, the config file file may only be written to the same + directory as the config file used when starting Kea (passed as a ``-c`` + argument). + .. isccmd:: leases-reclaim .. _command-leases-reclaim: diff --git a/src/bin/agent/tests/ca_controller_unittests.cc b/src/bin/agent/tests/ca_controller_unittests.cc index f7b2c8bbe6..54881d4a77 100644 --- a/src/bin/agent/tests/ca_controller_unittests.cc +++ b/src/bin/agent/tests/ca_controller_unittests.cc @@ -512,6 +512,9 @@ TEST_F(CtrlAgentControllerTest, configWrite) { // Now clean up after ourselves. ctrl->registerCommands(); + // Add a config file. + ctrl->setConfigFile(string(TEST_DATA_BUILDDIR) + string("/config.json")); + // First, build the command: string file = string(TEST_DATA_BUILDDIR) + string("/config-write.json"); string cmd_txt = "{ \"command\": \"config-write\" }"; @@ -547,6 +550,54 @@ TEST_F(CtrlAgentControllerTest, configWrite) { ctrl->deregisterCommands(); } +// Tests that config-write fails with a bad path. +TEST_F(CtrlAgentControllerTest, badConfigWrite) { + ASSERT_NO_THROW(initProcess()); + EXPECT_TRUE(checkProcess()); + + // The framework available makes it very difficult to test the actual + // code as CtrlAgentController is not initialized the same way it is + // in production code. In particular, the way CtrlAgentController + // is initialized in tests does not call registerCommands(). + // This is a crude workaround for this problem. Proper solution should + // be developed sooner rather than later. + const DControllerBasePtr& base = getController(); + const CtrlAgentControllerPtr& ctrl + = boost::dynamic_pointer_cast(base); + ASSERT_TRUE(ctrl); + // Now clean up after ourselves. + ctrl->registerCommands(); + + // Add a config file. + ctrl->setConfigFile(string(TEST_DATA_BUILDDIR) + string("/config.json")); + + // First, build the command: + string file("/tmp/config-write.json"); + string cmd_txt = "{ \"command\": \"config-write\" }"; + ConstElementPtr cmd = Element::fromJSON(cmd_txt); + ConstElementPtr params = Element::fromJSON("{\"filename\": \"" + file + "\" }"); + CtrlAgentCommandMgr& mgr_ = CtrlAgentCommandMgr::instance(); + + // Send the command + ConstElementPtr answer = mgr_.handleCommand("config-write", params, cmd); + + // Check that the command failed. + string expected = "not allowed to write config into "; + expected += file; + expected += ": file "; + expected += file; + expected += " must be in the same directory as the config file ("; + expected += string(TEST_DATA_BUILDDIR) + string("/config.json"); + expected += ")"; + checkAnswer(answer, isc::config::CONTROL_RESULT_ERROR, expected); + + // Remove the file. + ::remove(file.c_str()); + + // Now clean up after ourselves. + ctrl->deregisterCommands(); +} + // Tests if config-reload attempts to reload a file and reports that the // file is missing. TEST_F(CtrlAgentControllerTest, configReloadMissingFile) { diff --git a/src/bin/d2/tests/d2_command_unittest.cc b/src/bin/d2/tests/d2_command_unittest.cc index 8b133b174a..1f3a84bd33 100644 --- a/src/bin/d2/tests/d2_command_unittest.cc +++ b/src/bin/d2/tests/d2_command_unittest.cc @@ -1036,6 +1036,9 @@ TEST_F(CtrlChannelD2Test, writeConfigFilename) { EXPECT_NO_THROW(createUnixChannelServer()); string response; + // This is normally set by the command line -c parameter. + server_->setConfigFile("test1.json"); + sendUnixCommand("{ \"command\": \"config-write\", " "\"arguments\": { \"filename\": \"test2.json\" } }", response); @@ -1043,6 +1046,60 @@ TEST_F(CtrlChannelD2Test, writeConfigFilename) { ::remove("test2.json"); } +// Tests if config-write can be called with a valid full path as parameter. +TEST_F(CtrlChannelD2Test, configWriteFullPath) { + createUnixChannelServer(); + std::string response; + + // This is normally set by the command line -c parameter. + server_->setConfigFile("/tmp/test1.json"); + + sendUnixCommand("{ \"command\": \"config-write\", " + "\"arguments\": { \"filename\": \"/tmp/test2.json\" } }", + response); + + checkConfigWrite(response, CONTROL_RESULT_SUCCESS, "/tmp/test2.json"); + ::remove("/tmp/test2.json"); +} + +// Tests if config-write raises an error with invalid path as parameter. +TEST_F(CtrlChannelD2Test, configWriteBadPath) { + createUnixChannelServer(); + std::string response; + + // This is normally set by the command line -c parameter. + server_->setConfigFile("test1.json"); + + sendUnixCommand("{ \"command\": \"config-write\", " + "\"arguments\": { \"filename\": \"/tmp/test2.json\" } }", + response); + + string expected = "not allowed to write config into /tmp/test2.json: "; + expected += "file /tmp/test2.json must be in the same directory "; + expected += "as the config file (test1.json)"; + checkConfigWrite(response, CONTROL_RESULT_ERROR, expected); + ::remove("/tmp/test2.json"); +} + +// Tests if config-write raises an error with invalid full path as parameter. +TEST_F(CtrlChannelD2Test, configWriteBadFullPath) { + createUnixChannelServer(); + std::string response; + + // This is normally set by the command line -c parameter. + server_->setConfigFile("/tmp/kea1/test.json"); + + sendUnixCommand("{ \"command\": \"config-write\", " + "\"arguments\": { \"filename\": \"/tmp/kea2/test.json\" } }", + response); + + string expected = "not allowed to write config into /tmp/kea2/test.json: "; + expected += "file /tmp/kea2/test.json must be in the same directory "; + expected += "as the config file (/tmp/kea1/test.json)"; + checkConfigWrite(response, CONTROL_RESULT_ERROR, expected); + ::remove("/tmp/kea2/test.json"); +} + // Tests if config-reload attempts to reload a file and reports that the // file is missing. TEST_F(CtrlChannelD2Test, configReloadMissingFile) { diff --git a/src/bin/dhcp4/ctrl_dhcp4_srv.cc b/src/bin/dhcp4/ctrl_dhcp4_srv.cc index e5a5e88217..7ad85e2c20 100644 --- a/src/bin/dhcp4/ctrl_dhcp4_srv.cc +++ b/src/bin/dhcp4/ctrl_dhcp4_srv.cc @@ -328,11 +328,19 @@ ControlledDhcpv4Srv::commandConfigWriteHandler(const string&, // filename parameter was not specified, so let's use whatever we remember // from the command-line filename = getConfigFile(); - } - - if (filename.empty()) { - return (createAnswer(CONTROL_RESULT_ERROR, "Unable to determine filename." - "Please specify filename explicitly.")); + if (filename.empty()) { + return (createAnswer(CONTROL_RESULT_ERROR, "Unable to determine filename." + "Please specify filename explicitly.")); + } + } else { + try { + checkWriteConfigFile(filename); + } catch (const isc::Exception& ex) { + std::ostringstream msg; + msg << "not allowed to write config into " << filename + << ": " << ex.what(); + return (createAnswer(CONTROL_RESULT_ERROR, msg.str())); + } } // Ok, it's time to write the file. diff --git a/src/bin/dhcp4/ctrl_dhcp4_srv.h b/src/bin/dhcp4/ctrl_dhcp4_srv.h index e4631db699..569cd72ecf 100644 --- a/src/bin/dhcp4/ctrl_dhcp4_srv.h +++ b/src/bin/dhcp4/ctrl_dhcp4_srv.h @@ -224,9 +224,9 @@ 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. 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. + /// Kea start-up will be used. To avoid any exploits, the target + /// directory must be the same as a security measure against + /// exploiting file writes remotely. /// /// @param command (ignored) /// @param args may contain optional string argument filename diff --git a/src/bin/dhcp4/dhcp4_lexer.cc b/src/bin/dhcp4/dhcp4_lexer.cc index 769f61a588..4163ca92d9 100644 --- a/src/bin/dhcp4/dhcp4_lexer.cc +++ b/src/bin/dhcp4/dhcp4_lexer.cc @@ -1,6 +1,6 @@ -#line 1 "dhcp4_lexer.cc" +#line 2 "dhcp4_lexer.cc" -#line 3 "dhcp4_lexer.cc" +#line 4 "dhcp4_lexer.cc" #define YY_INT_ALIGNED short int @@ -2187,7 +2187,7 @@ using namespace isc::dhcp; /* To avoid the call to exit... oops! */ #define YY_FATAL_ERROR(msg) isc::dhcp::Parser4Context::fatal(msg) -#line 2190 "dhcp4_lexer.cc" +#line 2191 "dhcp4_lexer.cc" /* noyywrap disables automatic rewinding for the next file to parse. Since we always parse only a single string, there's no need to do any wraps. And using yywrap requires linking with -lfl, which provides the default yywrap @@ -2213,8 +2213,8 @@ using namespace isc::dhcp; by moving it ahead by yyleng bytes. yyleng specifies the length of the currently matched token. */ #define YY_USER_ACTION driver.loc_.columns(yyleng); -#line 2216 "dhcp4_lexer.cc" #line 2217 "dhcp4_lexer.cc" +#line 2218 "dhcp4_lexer.cc" #define INITIAL 0 #define COMMENT 1 @@ -2542,7 +2542,7 @@ YY_DECL } -#line 2545 "dhcp4_lexer.cc" +#line 2546 "dhcp4_lexer.cc" while ( /*CONSTCOND*/1 ) /* loops until end-of-file is reached */ { @@ -5578,7 +5578,7 @@ YY_RULE_SETUP #line 2405 "dhcp4_lexer.ll" ECHO; YY_BREAK -#line 5581 "dhcp4_lexer.cc" +#line 5582 "dhcp4_lexer.cc" case YY_END_OF_BUFFER: { diff --git a/src/bin/dhcp4/tests/ctrl_dhcp4_srv_unittest.cc b/src/bin/dhcp4/tests/ctrl_dhcp4_srv_unittest.cc index 3f0f4d678c..0634b68b21 100644 --- a/src/bin/dhcp4/tests/ctrl_dhcp4_srv_unittest.cc +++ b/src/bin/dhcp4/tests/ctrl_dhcp4_srv_unittest.cc @@ -1537,6 +1537,9 @@ TEST_F(CtrlChannelDhcpv4SrvTest, configWriteFilename) { createUnixChannelServer(); std::string response; + // This is normally set by the command line -c parameter. + server_->setConfigFile("test1.json"); + sendUnixCommand("{ \"command\": \"config-write\", " "\"arguments\": { \"filename\": \"test2.json\" } }", response); @@ -1544,6 +1547,57 @@ TEST_F(CtrlChannelDhcpv4SrvTest, configWriteFilename) { ::remove("test2.json"); } +// Tests if config-write can be called with a valid full path as parameter. +TEST_F(CtrlChannelDhcpv4SrvTest, configWriteFullPath) { + createUnixChannelServer(); + std::string response; + + // This is normally set by the command line -c parameter. + server_->setConfigFile("/tmp/test1.json"); + + sendUnixCommand("{ \"command\": \"config-write\", " + "\"arguments\": { \"filename\": \"/tmp/test2.json\" } }", response); + + checkConfigWrite(response, CONTROL_RESULT_SUCCESS, "/tmp/test2.json"); + ::remove("/tmp/test2.json"); +} + +// Tests if config-write raises an error with invalid path as parameter. +TEST_F(CtrlChannelDhcpv4SrvTest, configWriteBadPath) { + createUnixChannelServer(); + std::string response; + + // This is normally set by the command line -c parameter. + server_->setConfigFile("test1.json"); + + sendUnixCommand("{ \"command\": \"config-write\", " + "\"arguments\": { \"filename\": \"/tmp/test2.json\" } }", response); + + string expected = "not allowed to write config into /tmp/test2.json: "; + expected += "file /tmp/test2.json must be in the same directory "; + expected += "as the config file (test1.json)"; + checkConfigWrite(response, CONTROL_RESULT_ERROR, expected); + ::remove("/tmp/test2.json"); +} + +// Tests if config-write raises an error with invalid full path as parameter. +TEST_F(CtrlChannelDhcpv4SrvTest, configWriteBadFullPath) { + createUnixChannelServer(); + std::string response; + + // This is normally set by the command line -c parameter. + server_->setConfigFile("/tmp/kea1/test.json"); + + sendUnixCommand("{ \"command\": \"config-write\", " + "\"arguments\": { \"filename\": \"/tmp/kea2/test.json\" } }", response); + + string expected = "not allowed to write config into /tmp/kea2/test.json: "; + expected += "file /tmp/kea2/test.json must be in the same directory "; + expected += "as the config file (/tmp/kea1/test.json)"; + checkConfigWrite(response, CONTROL_RESULT_ERROR, expected); + ::remove("/tmp/kea2/test.json"); +} + // Tests if config-reload attempts to reload a file and reports that the // file is missing. TEST_F(CtrlChannelDhcpv4SrvTest, configReloadMissingFile) { diff --git a/src/bin/dhcp6/ctrl_dhcp6_srv.cc b/src/bin/dhcp6/ctrl_dhcp6_srv.cc index 94244c14db..199c046a51 100644 --- a/src/bin/dhcp6/ctrl_dhcp6_srv.cc +++ b/src/bin/dhcp6/ctrl_dhcp6_srv.cc @@ -330,11 +330,19 @@ ControlledDhcpv6Srv::commandConfigWriteHandler(const string&, // filename parameter was not specified, so let's use whatever we remember // from the command-line filename = getConfigFile(); - } - - if (filename.empty()) { - return (createAnswer(CONTROL_RESULT_ERROR, "Unable to determine filename." - "Please specify filename explicitly.")); + if (filename.empty()) { + return (createAnswer(CONTROL_RESULT_ERROR, "Unable to determine filename." + "Please specify filename explicitly.")); + } + } else { + try { + checkWriteConfigFile(filename); + } catch (const isc::Exception& ex) { + std::ostringstream msg; + msg << "not allowed to write config into " << filename + << ": " << ex.what(); + return (createAnswer(CONTROL_RESULT_ERROR, msg.str())); + } } // Ok, it's time to write the file. diff --git a/src/bin/dhcp6/ctrl_dhcp6_srv.h b/src/bin/dhcp6/ctrl_dhcp6_srv.h index 756c7d6e8c..dbeabcfed8 100644 --- a/src/bin/dhcp6/ctrl_dhcp6_srv.h +++ b/src/bin/dhcp6/ctrl_dhcp6_srv.h @@ -224,9 +224,9 @@ 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. 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. + /// Kea start-up will be used. To avoid any exploits, the target + /// directory must be the same as a security measure against + /// exploiting file writes remotely. /// /// @param command (ignored) /// @param args may contain optional string argument filename diff --git a/src/bin/dhcp6/tests/ctrl_dhcp6_srv_unittest.cc b/src/bin/dhcp6/tests/ctrl_dhcp6_srv_unittest.cc index ed02e4c979..f6654669e9 100644 --- a/src/bin/dhcp6/tests/ctrl_dhcp6_srv_unittest.cc +++ b/src/bin/dhcp6/tests/ctrl_dhcp6_srv_unittest.cc @@ -1550,6 +1550,9 @@ TEST_F(CtrlChannelDhcpv6SrvTest, configWriteFilename) { createUnixChannelServer(); std::string response; + // This is normally set by the command line -c parameter. + server_->setConfigFile("test1.json"); + sendUnixCommand("{ \"command\": \"config-write\", " "\"arguments\": { \"filename\": \"test2.json\" } }", response); @@ -1557,6 +1560,57 @@ TEST_F(CtrlChannelDhcpv6SrvTest, configWriteFilename) { ::remove("test2.json"); } +// Tests if config-write can be called with a valid full path as parameter. +TEST_F(CtrlChannelDhcpv6SrvTest, configWriteFullPath) { + createUnixChannelServer(); + std::string response; + + // This is normally set by the command line -c parameter. + server_->setConfigFile("/tmp/test1.json"); + + sendUnixCommand("{ \"command\": \"config-write\", " + "\"arguments\": { \"filename\": \"/tmp/test2.json\" } }", response); + + checkConfigWrite(response, CONTROL_RESULT_SUCCESS, "/tmp/test2.json"); + ::remove("/tmp/test2.json"); +} + +// Tests if config-write raises an error with invalid path as parameter. +TEST_F(CtrlChannelDhcpv6SrvTest, configWriteBadPath) { + createUnixChannelServer(); + std::string response; + + // This is normally set by the command line -c parameter. + server_->setConfigFile("test1.json"); + + sendUnixCommand("{ \"command\": \"config-write\", " + "\"arguments\": { \"filename\": \"/tmp/test2.json\" } }", response); + + string expected = "not allowed to write config into /tmp/test2.json: "; + expected += "file /tmp/test2.json must be in the same directory "; + expected += "as the config file (test1.json)"; + checkConfigWrite(response, CONTROL_RESULT_ERROR, expected); + ::remove("/tmp/test2.json"); +} + +// Tests if config-write raises an error with invalid full path as parameter. +TEST_F(CtrlChannelDhcpv6SrvTest, configWriteBadFullPath) { + createUnixChannelServer(); + std::string response; + + // This is normally set by the command line -c parameter. + server_->setConfigFile("/tmp/kea1/test.json"); + + sendUnixCommand("{ \"command\": \"config-write\", " + "\"arguments\": { \"filename\": \"/tmp/kea2/test.json\" } }", response); + + string expected = "not allowed to write config into /tmp/kea2/test.json: "; + expected += "file /tmp/kea2/test.json must be in the same directory "; + expected += "as the config file (/tmp/kea1/test.json)"; + checkConfigWrite(response, CONTROL_RESULT_ERROR, expected); + ::remove("/tmp/kea2/test.json"); +} + // Tests if config-reload attempts to reload a file and reports that the // file is missing. TEST_F(CtrlChannelDhcpv6SrvTest, configReloadMissingFile) { diff --git a/src/hooks/dhcp/lease_cmds/tests/lease_cmds4_unittest.cc b/src/hooks/dhcp/lease_cmds/tests/lease_cmds4_unittest.cc index b02cddfec4..fc33969761 100644 --- a/src/hooks/dhcp/lease_cmds/tests/lease_cmds4_unittest.cc +++ b/src/hooks/dhcp/lease_cmds/tests/lease_cmds4_unittest.cc @@ -3432,13 +3432,13 @@ void Lease4CmdsTest::testLease4Write() { "{\n" " \"command\": \"lease4-write\",\n" " \"arguments\": {" - " \"filename\": \"/tmp/myleases.txt\"\n" + " \"filename\": \"/foo-bar/myleases.txt\"\n" " }\n" "}"; std::ostringstream os; os << "'filename' parameter is invalid: invalid path specified:" - << " '/tmp', supported path is '" << CfgMgr::instance().getDataDir() << "'"; + << " '/foo-bar', supported path is '" << CfgMgr::instance().getDataDir() << "'"; testCommand(txt, CONTROL_RESULT_ERROR, os.str()); } diff --git a/src/hooks/dhcp/lease_cmds/tests/lease_cmds6_unittest.cc b/src/hooks/dhcp/lease_cmds/tests/lease_cmds6_unittest.cc index 466f4daaaa..c8d5f21fb2 100644 --- a/src/hooks/dhcp/lease_cmds/tests/lease_cmds6_unittest.cc +++ b/src/hooks/dhcp/lease_cmds/tests/lease_cmds6_unittest.cc @@ -4191,13 +4191,13 @@ void Lease6CmdsTest::testLease6Write() { "{\n" " \"command\": \"lease6-write\",\n" " \"arguments\": {" - " \"filename\": \"/tmp/myleases.txt\"\n" + " \"filename\": \"/foo-bar/myleases.txt\"\n" " }\n" "}"; std::ostringstream os; os << "'filename' parameter is invalid: invalid path specified:" - << " '/tmp', supported path is '" << CfgMgr::instance().getDataDir() << "'"; + << " '/foo-bar', supported path is '" << CfgMgr::instance().getDataDir() << "'"; } TEST_F(Lease6CmdsTest, lease6AddMissingParams) { diff --git a/src/lib/dhcpsrv/tests/cfgmgr_unittest.cc b/src/lib/dhcpsrv/tests/cfgmgr_unittest.cc index a4d6ad2b4a..d05d5c2963 100644 --- a/src/lib/dhcpsrv/tests/cfgmgr_unittest.cc +++ b/src/lib/dhcpsrv/tests/cfgmgr_unittest.cc @@ -278,6 +278,7 @@ public: } void clear() { + data_dir_env_var_.setValue(); CfgMgr::instance().setFamily(AF_INET); resetDataDir(); CfgMgr::instance().clear(); diff --git a/src/lib/dhcpsrv/tests/memfile_lease_mgr_unittest.cc b/src/lib/dhcpsrv/tests/memfile_lease_mgr_unittest.cc index 6838835409..b89c709833 100644 --- a/src/lib/dhcpsrv/tests/memfile_lease_mgr_unittest.cc +++ b/src/lib/dhcpsrv/tests/memfile_lease_mgr_unittest.cc @@ -116,6 +116,9 @@ public: extra_files_(), data_dir_env_var_("KEA_DHCP_DATA_DIR") { + // Reset the env variable. + data_dir_env_var_.setValue(); + // Save the pre-test data dir and set it to the test directory. CfgMgr::instance().clear(); original_datadir_ = CfgMgr::instance().getDataDir(); diff --git a/src/lib/process/d_controller.cc b/src/lib/process/d_controller.cc index 420442c132..df49a97371 100644 --- a/src/lib/process/d_controller.cc +++ b/src/lib/process/d_controller.cc @@ -498,13 +498,22 @@ DControllerBase::configWriteHandler(const std::string&, "Unable to determine filename." "Please specify filename explicitly.")); } + } else { + try { + checkWriteConfigFile(filename); + } catch (const isc::Exception& ex) { + std::ostringstream msg; + msg << "not allowed to write config into " << filename + << ": " << ex.what(); + return (createAnswer(CONTROL_RESULT_ERROR, msg.str())); + } } // Ok, it's time to write the file. size_t size = 0; - ElementPtr cfg = process_->getCfgMgr()->getContext()->toElement(); try { + ElementPtr cfg = process_->getCfgMgr()->getContext()->toElement(); size = writeConfigFile(filename, cfg); } catch (const isc::Exception& ex) { return (createAnswer(CONTROL_RESULT_ERROR, diff --git a/src/lib/process/d_controller.h b/src/lib/process/d_controller.h index e16253e372..b6de4da370 100644 --- a/src/lib/process/d_controller.h +++ b/src/lib/process/d_controller.h @@ -283,9 +283,9 @@ public: /// 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. 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. + /// Kea start-up will be used. To avoid any exploits, the target + /// directory must be the same as a security measure against + /// exploiting file writes remotely. /// /// @param command (ignored) /// @param args may contain optional string argument filename diff --git a/src/lib/process/daemon.cc b/src/lib/process/daemon.cc index 3ca8579d89..20023ad844 100644 --- a/src/lib/process/daemon.cc +++ b/src/lib/process/daemon.cc @@ -14,7 +14,7 @@ #include #include #include -#include +#include #include #include @@ -23,7 +23,7 @@ #include using namespace isc::data; -namespace ph = std::placeholders; +using namespace isc::util::file; /// @brief provides default implementation for basic daemon operations /// @@ -116,15 +116,37 @@ Daemon::checkConfigFile() const { isc_throw(isc::BadValue, "config file name is not set"); } - // Create Filename instance from the config_file_ pathname, and + // Create Path instance from the config_file_ pathname, and // check the file name component. - isc::util::Filename file(config_file_); - if (file.name().empty()) { + Path file(config_file_); + if (file.stem().empty()) { isc_throw(isc::BadValue, "config file:" << config_file_ << " is missing file name"); } } +void +Daemon::checkWriteConfigFile(std::string& file) { + Path path(file); + // from checkConfigFile(). + if (path.stem().empty()) { + isc_throw(isc::BadValue, "config file:" << file + << " is missing file name"); + } + Path current(config_file_); + if (current.parentDirectory() == path.parentDirectory()) { + // Same parent directories! + return; + } + if (path.parentDirectory().empty()) { + // Note the current parent directory can't be empty here. + file = current.parentDirectory() + file; + return; + } + isc_throw(isc::BadValue, "file " << file << " must be in the same " + << "directory as the config file (" << config_file_ << ")"); +} + std::string Daemon::getProcName() { return (proc_name_); @@ -176,10 +198,10 @@ Daemon::makePIDFileName() const { "Daemon::makePIDFileName config file name is not set"); } - // Create Filename instance from the config_file_ pathname, so we can + // Create Path instance from the config_file_ pathname, so we can // extract the fname component. - isc::util::Filename file(config_file_); - if (file.name().empty()) { + Path file(config_file_); + if (file.stem().empty()) { isc_throw(isc::BadValue, "Daemon::makePIDFileName config file:" << config_file_ << " is missing file name"); } @@ -192,7 +214,7 @@ Daemon::makePIDFileName() const { // Make the pathname for the PID file from the runtime directory, // configuration name and process name. std::ostringstream stream; - stream << pid_file_dir_ << "/" << file.name() + stream << pid_file_dir_ << "/" << file.stem() << "." << proc_name_ << ".pid"; return(stream.str()); diff --git a/src/lib/process/daemon.h b/src/lib/process/daemon.h index d64445a77c..83db87e9af 100644 --- a/src/lib/process/daemon.h +++ b/src/lib/process/daemon.h @@ -139,6 +139,15 @@ public: /// @throw BadValue when the configuration file name is bad. void checkConfigFile() const; + /// @brief Checks the to-be-written configuration file name. + /// + /// @note As a side effect prepend the current config file path + /// when the name does not contain a slash. + /// + /// @param[in][out] file Reference to the TBW configuration file name. + /// @throw BadValue when not in the same directory. + void checkWriteConfigFile(std::string& file); + /// @brief Writes current configuration to specified file /// /// This method writes the current configuration to specified file. diff --git a/src/lib/process/tests/daemon_unittest.cc b/src/lib/process/tests/daemon_unittest.cc index bd80e25a65..252c915221 100644 --- a/src/lib/process/tests/daemon_unittest.cc +++ b/src/lib/process/tests/daemon_unittest.cc @@ -40,8 +40,8 @@ std::string DaemonImpl::getVersion(bool extended) { } } -}; -}; +} +} namespace { @@ -77,7 +77,6 @@ private: std::string env_copy_; }; - // Very simple test. Checks whether Daemon can be instantiated and its // default parameters are sane TEST_F(DaemonTest, constructor) { @@ -113,6 +112,47 @@ TEST_F(DaemonTest, checkConfigFile) { EXPECT_NO_THROW(instance.checkConfigFile()); } +// Verify write config file checker. +TEST_F(DaemonTest, checkWriteConfigFile) { + Daemon instance; + + std::string file(""); + EXPECT_THROW(instance.checkWriteConfigFile(file), BadValue); + file = "/tmp/"; + EXPECT_THROW(instance.checkWriteConfigFile(file), BadValue); + file = "tmp/"; + EXPECT_THROW(instance.checkWriteConfigFile(file), BadValue); + instance.setConfigFile("/tmp/foo"); + file = "/foo/bar"; + EXPECT_THROW(instance.checkWriteConfigFile(file), BadValue); + file = "/tmp/foo/bar"; + EXPECT_THROW(instance.checkWriteConfigFile(file), BadValue); + file = "/tmp/bar"; + EXPECT_NO_THROW(instance.checkWriteConfigFile(file)); + EXPECT_EQ("/tmp/bar", file); + file = "bar"; + EXPECT_NO_THROW(instance.checkWriteConfigFile(file)); + EXPECT_EQ("/tmp/bar", file); + instance.setConfigFile("tmp/foo"); + file = "/tmp/bar"; + EXPECT_THROW(instance.checkWriteConfigFile(file), BadValue); + file = "/tmp/foo/bar"; + EXPECT_THROW(instance.checkWriteConfigFile(file), BadValue); + file = "tmp/bar"; + EXPECT_NO_THROW(instance.checkWriteConfigFile(file)); + EXPECT_EQ("tmp/bar", file); + instance.setConfigFile("foo"); + file = "/tmp/bar"; + EXPECT_THROW(instance.checkWriteConfigFile(file), BadValue); + file = "tmp/bar"; + EXPECT_THROW(instance.checkWriteConfigFile(file), BadValue); + file = "foo/bar"; + EXPECT_THROW(instance.checkWriteConfigFile(file), BadValue); + file = "bar"; + EXPECT_NO_THROW(instance.checkWriteConfigFile(file)); + EXPECT_EQ("bar", file); +} + // Verify process name accessors TEST_F(DaemonTest, getSetProcName) { Daemon instance; @@ -318,7 +358,6 @@ TEST_F(DaemonTest, exitValue) { EXPECT_EQ(77, instance.getExitValue()); } - // More tests will appear here as we develop Daemon class. -}; +} diff --git a/src/lib/util/filesystem.cc b/src/lib/util/filesystem.cc index 1f5f6ccbcb..a28cf3ddf1 100644 --- a/src/lib/util/filesystem.cc +++ b/src/lib/util/filesystem.cc @@ -63,11 +63,14 @@ setUmask() { } Path::Path(string const& full_name) { + dir_present_ = false; if (!full_name.empty()) { - bool dir_present = false; // Find the directory. size_t last_slash = full_name.find_last_of('/'); if (last_slash != string::npos) { + // Found a directory so note the fact. + dir_present_ = true; + // Found the last slash, so extract directory component and // set where the scan for the last_dot should terminate. parent_path_ = full_name.substr(0, last_slash); @@ -76,14 +79,11 @@ Path::Path(string const& full_name) { // do any more searching. return; } - - // Found a directory so note the fact. - dir_present = true; } // Now search backwards for the last ".". size_t last_dot = full_name.find_last_of('.'); - if ((last_dot == string::npos) || (dir_present && (last_dot < last_slash))) { + if ((last_dot == string::npos) || (dir_present_ && (last_dot < last_slash))) { // Last "." either not found or it occurs to the left of the last // slash if a directory was present (so it is part of a directory // name). In this case, the remainder of the string after the slash @@ -105,7 +105,7 @@ Path::Path(string const& full_name) { string Path::str() const { - return (parent_path_ + ((parent_path_.empty() || parent_path_ == "/") ? string() : "/") + stem_ + extension_); + return (parent_path_ + (dir_present_ ? "/" : "") + stem_ + extension_); } string @@ -113,6 +113,11 @@ Path::parentPath() const { return (parent_path_); } +string +Path::parentDirectory() const { + return (parent_path_ + (dir_present_ ? "/" : "")); +} + string Path::stem() const { return (stem_); @@ -147,10 +152,9 @@ Path::replaceExtension(string const& replacement) { Path& Path::replaceParentPath(string const& replacement) { string const trimmed_replacement(trim(replacement)); - if (trimmed_replacement.empty()) { + dir_present_ = (trimmed_replacement.find_last_of('/') != string::npos); + if (trimmed_replacement.empty() || (trimmed_replacement == "/")) { parent_path_ = string(); - } else if (trimmed_replacement == "/") { - parent_path_ = trimmed_replacement; } else if (trimmed_replacement.at(trimmed_replacement.size() - 1) == '/') { parent_path_ = trimmed_replacement.substr(0, trimmed_replacement.size() - 1); } else { diff --git a/src/lib/util/filesystem.h b/src/lib/util/filesystem.h index 7f36b8a84a..8db8fecb41 100644 --- a/src/lib/util/filesystem.h +++ b/src/lib/util/filesystem.h @@ -60,6 +60,14 @@ struct Path { /// @return parent path of current path. std::string parentPath() const; + /// @brief Get the parent directory. + /// + /// Empty if no directory is present, the parent path follwed by + /// a slash otherwise. + /// + /// @return parent directory of current path. + std::string parentDirectory() const; + /// @brief Get the base name of the file without the extension. /// /// Counterpart for std::filesystem::path::stem. @@ -106,6 +114,9 @@ struct Path { Path& replaceParentPath(std::string const& replacement = std::string()); private: + /// @brief Is a directory present. + bool dir_present_; + /// @brief Parent path. std::string parent_path_; diff --git a/src/lib/util/tests/filesystem_unittests.cc b/src/lib/util/tests/filesystem_unittests.cc index 60f18f5bd7..88d84fe5a7 100644 --- a/src/lib/util/tests/filesystem_unittests.cc +++ b/src/lib/util/tests/filesystem_unittests.cc @@ -61,9 +61,55 @@ TEST(PathTest, components) { Path fname("/alpha/beta/gamma.delta"); EXPECT_EQ("/alpha/beta/gamma.delta", fname.str()); EXPECT_EQ("/alpha/beta", fname.parentPath()); + EXPECT_EQ("/alpha/beta/", fname.parentDirectory()); EXPECT_EQ("gamma", fname.stem()); EXPECT_EQ(".delta", fname.extension()); EXPECT_EQ("gamma.delta", fname.filename()); + + // The root. + Path root("/"); + EXPECT_EQ("/", root.str()); + EXPECT_EQ("", root.parentPath()); + EXPECT_EQ("/", root.parentDirectory()); + EXPECT_EQ("", root.stem()); + EXPECT_EQ("", root.extension()); + EXPECT_EQ("", root.filename()); + + // In the root directory. + Path inroot("/gamma.delta"); + EXPECT_EQ("/gamma.delta", inroot.str()); + EXPECT_EQ("", inroot.parentPath()); + EXPECT_EQ("/", inroot.parentDirectory()); + EXPECT_EQ("gamma", inroot.stem()); + EXPECT_EQ(".delta", inroot.extension()); + EXPECT_EQ("gamma.delta", inroot.filename()); + + // No directory. + Path nodir("gamma.delta"); + EXPECT_EQ("gamma.delta", nodir.str()); + EXPECT_EQ("", nodir.parentPath()); + EXPECT_EQ("", nodir.parentDirectory()); + EXPECT_EQ("gamma", nodir.stem()); + EXPECT_EQ(".delta", nodir.extension()); + EXPECT_EQ("gamma.delta", nodir.filename()); + + // Relative name. + Path relative("../alpha/gamma.delta"); + EXPECT_EQ("../alpha/gamma.delta", relative.str()); + EXPECT_EQ("../alpha", relative.parentPath()); + EXPECT_EQ("../alpha/", relative.parentDirectory()); + EXPECT_EQ("gamma", relative.stem()); + EXPECT_EQ(".delta", relative.extension()); + EXPECT_EQ("gamma.delta", relative.filename()); + + // Multiple extensions. + Path extensions("/alpha/beta/gamma.delta.epsilon"); + EXPECT_EQ("/alpha/beta/gamma.delta.epsilon", extensions.str()); + EXPECT_EQ("/alpha/beta", extensions.parentPath()); + EXPECT_EQ("/alpha/beta/", extensions.parentDirectory()); + EXPECT_EQ("gamma.delta", extensions.stem()); + EXPECT_EQ(".epsilon", extensions.extension()); + EXPECT_EQ("gamma.delta.epsilon", extensions.filename()); } /// @brief Check replaceExtension. @@ -83,30 +129,37 @@ TEST(PathTest, replaceExtension) { TEST(PathTest, replaceParentPath) { Path fname("a.b"); EXPECT_EQ("", fname.parentPath()); + EXPECT_EQ("", fname.parentDirectory()); EXPECT_EQ("a.b", fname.str()); fname.replaceParentPath("/just/some/dir/"); EXPECT_EQ("/just/some/dir", fname.parentPath()); + EXPECT_EQ("/just/some/dir/", fname.parentDirectory()); EXPECT_EQ("/just/some/dir/a.b", fname.str()); fname.replaceParentPath("/just/some/dir"); EXPECT_EQ("/just/some/dir", fname.parentPath()); + EXPECT_EQ("/just/some/dir/", fname.parentDirectory()); EXPECT_EQ("/just/some/dir/a.b", fname.str()); fname.replaceParentPath("/"); - EXPECT_EQ("/", fname.parentPath()); + EXPECT_EQ("", fname.parentPath()); + EXPECT_EQ("/", fname.parentDirectory()); EXPECT_EQ("/a.b", fname.str()); fname.replaceParentPath(""); EXPECT_EQ("", fname.parentPath()); + EXPECT_EQ("", fname.parentDirectory()); EXPECT_EQ("a.b", fname.str()); fname = Path("/first/a.b"); EXPECT_EQ("/first", fname.parentPath()); + EXPECT_EQ("/first/", fname.parentDirectory()); EXPECT_EQ("/first/a.b", fname.str()); fname.replaceParentPath("/just/some/dir"); EXPECT_EQ("/just/some/dir", fname.parentPath()); + EXPECT_EQ("/just/some/dir/", fname.parentDirectory()); EXPECT_EQ("/just/some/dir/a.b", fname.str()); }